Open Bug 1218479 (createCodebasePrincipal) Opened 4 years ago Updated 7 months ago

[META] Fixing createCodebasePrincipal callers to support contextual identities.

Categories

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

defect

Tracking

()

Tracking Status
firefox57 --- affected

People

(Reporter: huseby, Unassigned)

References

(Blocks 2 open bugs)

Details

(Keywords: meta, Whiteboard: [userContextId])

Attachments

(1 obsolete file)

This bug tracks all of the patches fixing the platform-only calls of createCodebasePrincipal to support contextual identities.
Attached patch Bug_1218479.1.patch (obsolete) — Splinter Review
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.
Attachment #8679011 - Flags: review?(bobbyholley)
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.
Attachment #8679011 - Flags: review?(bobbyholley) → review-
See also discussions in bug 1211590 and bug 1209162.
QA Contact: huseby
QA Contact: huseby
Assignee: nobody → huseby
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?
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?
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.
Depends on: 1209162
Depends on: 1229222
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.
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.
>
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.
Flags: needinfo?(tanvi)
Flags: needinfo?(bobbyholley)
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?
(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.
Flags: needinfo?(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?
> 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.
(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?
Status: NEW → ASSIGNED
I don't have the bandwidth to look at this - if input is still needed, please flag sicking.
Flags: needinfo?(bobbyholley)
(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.
Attachment #8679011 - Attachment is obsolete: true
This is now the meta bug for all of the patches to fix the call sites of createCodebasePrincipal.
Summary: Fixing createCodebasePrincipal callers to support contextual identities. → [META] Fixing createCodebasePrincipal callers to support contextual identities.
Depends on: 1233885
Depends on: 1233886
Depends on: 1233895
Depends on: 1233899
Depends on: 1233903
Depends on: 1233904
Depends on: 1233906
Depends on: 1233907
Depends on: 1233908
Depends on: 1233911
Depends on: 1233917
Alias: createCodebasePrincipal
Blocks: 1233903
No longer depends on: 1233903
Depends on: 1196644
Component: Security → DOM: Security
Product: Firefox → Core
Component: DOM: Security → DOM
Depends on: 1237475
Depends on: 1237479
Depends on: 1237908
Depends on: 1237911
Depends on: 1237913
Depends on: 1237915
Depends on: 1237916
Depends on: 1237917
Whiteboard: [userContextId]
Depends on: 1238177
Depends on: 1238182
Depends on: 1238183
Depends on: 1238722
Depends on: 1238723
Depends on: 1238727
Depends on: 1225053
Depends on: 1240620
Depends on: 1240623
Depends on: 1240624
Depends on: 1250983
Depends on: 1259871
Depends on: 1260906
Depends on: 1260907
Depends on: 1260911
Depends on: 1260915
Depends on: 1260917
Depends on: 1260921
Depends on: 1260923
Assignee: huseby → nobody
Status: ASSIGNED → NEW
Depends on: 1272416
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.