1218479 - (createCodebasePrincipal) [META] Fixing createCodebasePrincipal callers to support contextual identities.

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P3)

Description

[Dave Huseby [:huseby]]

This bug tracks all of the patches fixing the platform-only calls of createCodebasePrincipal to support contextual identities.

Comment 1

[Dave Huseby [:huseby]]

Attachment: Bug_1218479.1.patch

browser/components/feeds/FeedConverter.js
  line 252
        // Now load the actual XUL document.
        var aboutFeedsURI = ios.newURI("about:feeds", null, null);
        chromeChannel = ios.newChannelFromURIWithLoadInfo(aboutFeedsURI, loadInfo);
        chromeChannel.originalURI = result.uri;
        chromeChannel.owner =
          Services.scriptSecurityManager.createCodebasePrincipal(aboutFeedsURI, {});

Pass loadInfo.originAttributes.userContextId to createCodebasePrincipal.

Comment 2

[Bobby Holley (:bholley)]

Comment on attachment 8679011 [details] [diff] [review]
Bug_1218479.1.patch

Review of attachment 8679011 [details] [diff] [review]:
-----------------------------------------------------------------

This isn't right. The point of OriginAttributes is that they provide a container by which we can generically propagate things like userContextId in the right ways, so as to reduce the work required to add new such attributes. We should be able to add a "userContextId2" OriginAttribute with O(1) effort. So spot-fixes like this are the wrong approach.

Comment 3

[Bobby Holley (:bholley)]

See also discussions in bug 1211590 and bug 1209162.

Comment 4

[Tanvi Vyas[:tanvi]]

Comment on attachment 8679011 [details] [diff] [review]
Bug_1218479.1.patch

+++ b/browser/components/feeds/FeedConverter.js
@@ -249,17 +249,18 @@ FeedConverter.prototype = {
         // page can access it.
         feedService.addFeedResult(result);
 
         // Now load the actual XUL document.
         var aboutFeedsURI = ios.newURI("about:feeds", null, null);
         chromeChannel = ios.newChannelFromURIWithLoadInfo(aboutFeedsURI, loadInfo);
         chromeChannel.originalURI = result.uri;
         chromeChannel.owner =
-          Services.scriptSecurityManager.createCodebasePrincipal(aboutFeedsURI, {});
+          Services.scriptSecurityManager.createCodebasePrincipal(
+            aboutFeedsURI, { userContextId: loadInfo.originAttributes.userContextId });

Sounds like you need to pass all originAttributes here.  Do we get them from loadInfo?

+            aboutFeedsURI, loadInfo.originAttributes);

Or is it more complicated than that?

Comment 5

[Tanvi Vyas[:tanvi]]

For userContextID, we should always inherit from the parent docshell.  The userContextID is set by tab and not changeable.  However, for specific pages like about:permissions, about:preferences, etc having a userContextID doesn't make sense.  In those cases, we actually want to pull data from all possible values of the userContextID.  How can we accomplish this?

Comment 6

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I don't want to have separate code paths for how we inherit separate parts of OriginAttributes. The whole point of OriginAttributes is to have a system where adding additional attributes is easy.

The patches in bug 1209162 lays the infrastructure for how we inherit OriginAttributes and provides choke points for where we can add logic or hooks in cases where we want special rules.

Comment 7

[Dave Huseby [:huseby]]

So here are the call sites in my audit notes that need to be fixed:

> browser/components/sessionstore/SessionStorage.jsm
>   line 107
>   restore: function (aDocShell, aStorageData) {
>     for (let origin of Object.keys(aStorageData)) {
>       let data = aStorageData[origin];
>       let principal = Services.scriptSecurityManager.createCodebasePrincipalFromOrigin(origin);
>       let storageManager = aDocShell.QueryInterface(Ci.nsIDOMStorageManager);
>       let window = aDocShell.QueryInterface(Ci.nsIInterfaceRequestor).getInterface(Ci.nsIDOMWindow);
> 
>       // There is no need to pass documentURI, it's only used to fill documentURI property of
>       // domstorage event, which in this case has no consumer. Prevention of events in case
>       // of missing documentURI will be solved in a followup bug to bug 600307.
>       let storage = storageManager.createStorage(window, principal, "", aDocShell.usePrivateBrowsing);
> 
> browser/components/feeds/FeedConverter.js
>   line 252
>         // Now load the actual XUL document.
>         var aboutFeedsURI = ios.newURI("about:feeds", null, null);
>         chromeChannel = ios.newChannelFromURIWithLoadInfo(aboutFeedsURI, loadInfo);
>         chromeChannel.originalURI = result.uri;
>         chromeChannel.owner =
>           Services.scriptSecurityManager.createCodebasePrincipal(aboutFeedsURI, {});
> 
> NOTE: Pass loadInfo.originAttributes.userContextId to createCodebasePrincipal.
> 
> toolkit/modules/NewTabUtils.jsm
>   line 26
>     XPCOMUtils.defineLazyGetter(this, "gPrincipal", function () {
>       let uri = Services.io.newURI("about:newtab", null, null);
>       return Services.scriptSecurityManager.createCodebasePrincipal(uri, {});
>     });
> 
> NOTE: so this is a UX problem really.  what happens when you're have a context
> tab open and you click the new tab "plus"?  does it open a new tab in the
> current context?  does it open a new non-context tab?  if it were up to me, i'd
> make the new tab plus have a drop-down arrow so the user can choose what kind
> of new tab.  i'd also make it possible to set a default context for all new
> tabs.

Comment 8

[Dave Huseby [:huseby]]

The rest of my audit notes on the call sites that don't need to be fixed:

> =============================
> ===== DON'T NEED TO FIX =====
> =============================
> 
> browser/modules/Feeds.jsm
>   line 69
>       let principalToCheck =
>         Services.scriptSecurityManager.createCodebasePrincipal(principalURI, {});
> 
> NOTE: don't need to fix because we aren't going to isolate feed subscriptions by user context.
> 
> browser/components/preferences/permissions.js
>   line 96
>       try {
>         uri = Services.io.newURI(input_url, null, null);
>         principal = Services.scriptSecurityManager.createCodebasePrincipal(uri, {});
>         // If we have ended up with an unknown scheme, the following will throw.
>         principal.origin;
>       } catch(ex) {
>         uri = Services.io.newURI("http://" + input_url, null, null);
>         principal = Services.scriptSecurityManager.createCodebasePrincipal(uri, {});
>         // If we have ended up with an unknown scheme, the following will throw.
>         principal.origin;
>       }
> 
> NOTE: we don't need to fix these because the permissions manager only uses the
> URI for indexing permssions in the hash table.
> 
> browser/extensions/pdfjs/content/PdfStreamConverter.jsm
>   line 1006
>     if ('createCodebasePrincipal' in ssm) {
>       resourcePrincipal = ssm.createCodebasePrincipal(uri, {});
>     } else if ('getNoAppCodebasePrincipal' in ssm) {
>       resourcePrincipal = ssm.getNoAppCodebasePrincipal(uri)
>     } else {
>       resourcePrincipal = ssm.getCodebasePrincipal(uri);
>     }
> 
> NOTE: maybe don't need to fix this?  it uses a channel created using the system
> principal and loads everything as if it is a browser-loaded resource.
> 
> browser/extensions/shumway/chrome/SpecialStorage.jsm
>   line 27
>     var principal = ssm.createCodebasePrincipal(uri, {});
> 
> NOTE: need to fix shumway?
> 
> services/sync/Weave.js
>   line 189
>     let ssm = Cc["@mozilla.org/scriptsecuritymanager;1"]
>                 .getService(Ci.nsIScriptSecurityManager);
>     let principal = ssm.createCodebasePrincipal(uri, {});
> 
> NOTE: need to fix weave?
> 
> toolkit/components/downloads/ApplicationReputation.cpp
>   line 299
>     nsCOMPtr<nsIPrincipal> principal =
>       BasePrincipal::CreateCodebasePrincipal(uri, attrs);
> 
> NOTE: don't need to fix this.
> 
> toolkit/components/places/BookmarkJSONUtils.jsm
>   line 210
>       let channel = NetUtil.newChannel({
>         uri,
>         loadingPrincipal: Services.scriptSecurityManager.createCodebasePrincipal(uri, {}),
>         contentPolicyType: Ci.nsIContentPolicy.TYPE_INTERNAL_XMLHTTPREQUEST
>       });
> 
> NOTE: don't need to fix this because we're importing bookmarks and empty origin
> attributes are appropriate.
> 
> toolkit/components/social/SocialService.jsm
>   line 158
>     let principal = Services.scriptSecurityManager.createCodebasePrincipal(URI, {});
> 
>   line 518
>     principal = Services.scriptSecurityManager.createCodebasePrincipal(URI, {});
> 
>   line 719
>     this.principal = Services.scriptSecurityManager.createCodebasePrincipal(originUri, {});
> 
> NOTE: don't need to fix this up...or at least the social api code needs to be
> refactored to support origin attributes.  it appears crufty.  the manifests for
> the social services don't store any origin attributes.
> 
> toolkit/webapps/NativeApp.jsm
>   line 459
>     let principal =
>       aIconURI.schemeIs("chrome") ?
>         Services.scriptSecurityManager.getSystemPrincipal() :
>         Services.scriptSecurityManager.createCodebasePrincipal(aIconURI, {});
> 
> NOTE: don't need to fix since native apps aren't installed/run in a user
> context...yet.
> 
> uriloader/prefetch/nsOfflineCacheUpdateService.cpp
>   line 722
>     NS_IMETHODIMP
>     nsOfflineCacheUpdateService::OfflineAppAllowedForURI(nsIURI *aURI,
>                                                          nsIPrefBranch *aPrefBranch,
>                                                          bool *aAllowed)
>     {
>         OriginAttributes attrs;
>         nsCOMPtr<nsIPrincipal> principal =
>             BasePrincipal::CreateCodebasePrincipal(aURI, attrs);
>         return OfflineAppPermForPrincipal(principal, aPrefBranch, false, aAllowed);
>     }
> 
>     nsresult
>     nsOfflineCacheUpdateService::OfflineAppPinnedForURI(nsIURI *aDocumentURI,
>                                                         nsIPrefBranch *aPrefBranch,
>                                                         bool *aPinned)
>     {
>         OriginAttributes attrs;
>         nsCOMPtr<nsIPrincipal> principal =
>             BasePrincipal::CreateCodebasePrincipal(aDocumentURI, attrs);
>         return OfflineAppPermForPrincipal(principal, aPrefBranch, true, aPinned);
>     }
> 
> NOTE: don't need to ix this since offline apps aren't sandboxed to a user context
> yet.
>

Comment 9

[Dave Huseby [:huseby]]

See Comment 7.  I'm not sure what to do in the case of SessionStorage.jsm.  I'm not exactly sure what it's used for.  On the surface it appears to be restoring the DOM storage for a session, which should be isolated on user context.  If that's the case, we'll just pull it from the docshell that's passed in.

Comment 10

[Dave Huseby [:huseby]]

For Comment 8, I list all of the callsites that I ultimately decided didn't need to be fixed.  I put my rationale in the "NOTE" after each all site.  Will you please double check my reasoning and confirm that we don't need to fix them.  In the case of shumway and weave, I'm not sure what to do.  In the case of the social api, we're running into crufty code.  the social api needs to be refactored to store origin attributes in the social service manifests AFAICT.  do we want to do that?

Comment 11

[Tanvi Vyas[:tanvi]]

(In reply to Dave Huseby [:huseby] from comment #7)
> > toolkit/modules/NewTabUtils.jsm
> >   line 26
> >     XPCOMUtils.defineLazyGetter(this, "gPrincipal", function () {
> >       let uri = Services.io.newURI("about:newtab", null, null);
> >       return Services.scriptSecurityManager.createCodebasePrincipal(uri, {});
> >     });
> > 
> > NOTE: so this is a UX problem really.  what happens when you're have a context
> > tab open and you click the new tab "plus"?  does it open a new tab in the
> > current context?  does it open a new non-context tab?  if it were up to me, i'd
> > make the new tab plus have a drop-down arrow so the user can choose what kind
> > of new tab.  i'd also make it possible to set a default context for all new
> > tabs.

For now, make it open a new tab in the default context (id=0).  In future versions of Contextual Identity, will figure out what the right behavior should be here (have a drop down, open the current context, etc).  But for v1, we make the user go to File->New Container Tab to open a tab in the non-default container.

Comment 12

[Tanvi Vyas[:tanvi]]

(In reply to Dave Huseby [:huseby] from comment #8)
> The rest of my audit notes on the call sites that don't need to be fixed:
> 
> > =============================
> > ===== DON'T NEED TO FIX =====
> > =============================
We should probably still update these to explicitly give them the default user context id.  What happens if the usercontext id isn't set on a principal?  Do consumers get null or the default value of 0?


(In reply to Dave Huseby [:huseby] from comment #7)
> > browser/components/feeds/FeedConverter.js
> >   line 252
> >         // Now load the actual XUL document.
> >         var aboutFeedsURI = ios.newURI("about:feeds", null, null);
> >         chromeChannel = ios.newChannelFromURIWithLoadInfo(aboutFeedsURI, loadInfo);
> >         chromeChannel.originalURI = result.uri;
> >         chromeChannel.owner =
> >           Services.scriptSecurityManager.createCodebasePrincipal(aboutFeedsURI, {});
> > 
> > NOTE: Pass loadInfo.originAttributes.userContextId to createCodebasePrincipal.
> > 


(In reply to Dave Huseby [:huseby] from comment #8)
> > =============================
> > ===== DON'T NEED TO FIX =====
> > =============================
> > 
> > browser/modules/Feeds.jsm
> >   line 69
> >       let principalToCheck =
> >         Services.scriptSecurityManager.createCodebasePrincipal(principalURI, {});
> > 
> > NOTE: don't need to fix because we aren't going to isolate feed subscriptions by user context.
> > 

What are the differences between Feeds.jsm and FeedConverter.js?  Why would we fix one and not the other?

Comment 13

[Tanvi Vyas[:tanvi]]

> browser/extensions/pdfjs/content/PdfStreamConverter.jsm
>   line 1006
>     if ('createCodebasePrincipal' in ssm) {
>       resourcePrincipal = ssm.createCodebasePrincipal(uri, {});
>     } else if ('getNoAppCodebasePrincipal' in ssm) {
>       resourcePrincipal = ssm.getNoAppCodebasePrincipal(uri)
>     } else {
>       resourcePrincipal = ssm.getCodebasePrincipal(uri);
>     }
> 
> NOTE: maybe don't need to fix this?  it uses a channel created using the system
> principal and loads everything as if it is a browser-loaded resource.

Can pdfs cause the browser to store anything?  i.e. flash can set flash cookies.

Comment 14

[Tanvi Vyas[:tanvi]]

(In reply to Dave Huseby [:huseby] from comment #8)
> > uriloader/prefetch/nsOfflineCacheUpdateService.cpp
> >   line 722
> >     NS_IMETHODIMP
> >     nsOfflineCacheUpdateService::OfflineAppAllowedForURI(nsIURI *aURI,
> >                                                          nsIPrefBranch *aPrefBranch,
> >                                                          bool *aAllowed)
> >     {
> >         OriginAttributes attrs;
> >         nsCOMPtr<nsIPrincipal> principal =
> >             BasePrincipal::CreateCodebasePrincipal(aURI, attrs);
> >         return OfflineAppPermForPrincipal(principal, aPrefBranch, false, aAllowed);
> >     }
> > 
> >     nsresult
> >     nsOfflineCacheUpdateService::OfflineAppPinnedForURI(nsIURI *aDocumentURI,
> >                                                         nsIPrefBranch *aPrefBranch,
> >                                                         bool *aPinned)
> >     {
> >         OriginAttributes attrs;
> >         nsCOMPtr<nsIPrincipal> principal =
> >             BasePrincipal::CreateCodebasePrincipal(aDocumentURI, attrs);
> >         return OfflineAppPermForPrincipal(principal, aPrefBranch, true, aPinned);
> >     }
> > 
> > NOTE: don't need to ix this since offline apps aren't sandboxed to a user context
> > yet.
> >
What is nsOfflineCacheUpdateService.cpp for exactly?  offline apps only?  or is it used for cached website content?

Comment 15

[Bobby Holley (:bholley)]

I don't have the bandwidth to look at this - if input is still needed, please flag sicking.

Comment 16

[Tanvi Vyas[:tanvi]]

(In reply to Dave Huseby [:huseby] from comment #9)
> See Comment 7.  I'm not sure what to do in the case of SessionStorage.jsm. 
> I'm not exactly sure what it's used for.  On the surface it appears to be
> restoring the DOM storage for a session, which should be isolated on user
> context.  If that's the case, we'll just pull it from the docshell that's
> passed in.

I'm not sure what this does either.  Maybe it is used when the browser is restarted and we restore previous tabs?  I would ask some of the folks who have previously edited the file over irc.  There is a docshell, so pulling originattributes from that seems like the right idea.

Comment 17

[Dave Huseby [:huseby]]

This is now the meta bug for all of the patches to fix the call sites of createCodebasePrincipal.

Comment 18

[Tanvi Vyas[:tanvi]]

Setting to P3 since this is a meta bug - https://wiki.mozilla.org/Bugmasters/Process/Triage#This_doesn.27t_fit_into_a_P1.2C_P2.2C_P3.2C_or_P5_framework

Comment 19

[Danny Colin [:sdk]]

Closing since all "depends on" are marked as fixed. Feel free to re-open if needed.