Bug 1218479 (createCodebasePrincipal)

[META] Fixing createCodebasePrincipal callers to support contextual identities.

NEW
Unassigned

Status

()

Core
DOM
P3
normal
3 years ago
8 months ago

People

(Reporter: huseby, Unassigned)

Tracking

(Blocks: 2 bugs, {meta})

unspecified
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 affected)

Details

(Whiteboard: [userContextId])

Attachments

(1 obsolete attachment)

(Reporter)

Description

3 years ago
This bug tracks all of the patches fixing the platform-only calls of createCodebasePrincipal to support contextual identities.
(Reporter)

Updated

3 years ago
Blocks: 1197283
(Reporter)

Comment 1

3 years ago
Created attachment 8679011 [details] [diff] [review]
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.
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.
(Reporter)

Updated

3 years ago
QA Contact: huseby
(Reporter)

Updated

3 years ago
QA Contact: huseby
(Reporter)

Updated

3 years ago
Assignee: nobody → huseby

Comment 4

3 years ago
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

3 years ago
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.
(Reporter)

Updated

3 years ago
Depends on: 1209162
(Reporter)

Updated

3 years ago
Depends on: 1229222
(Reporter)

Comment 7

3 years ago
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.
(Reporter)

Comment 8

3 years ago
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.
>
(Reporter)

Comment 9

3 years ago
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)
(Reporter)

Comment 10

3 years ago
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

3 years ago
(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)

Comment 12

3 years ago
(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

3 years ago
> 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

3 years ago
(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?
(Reporter)

Updated

3 years ago
Status: NEW → ASSIGNED
I don't have the bandwidth to look at this - if input is still needed, please flag sicking.
Flags: needinfo?(bobbyholley)

Comment 16

3 years ago
(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.
(Reporter)

Updated

3 years ago
Attachment #8679011 - Attachment is obsolete: true
(Reporter)

Comment 17

3 years ago
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.
(Reporter)

Updated

3 years ago
Depends on: 1233885
(Reporter)

Updated

3 years ago
Depends on: 1233886
(Reporter)

Updated

3 years ago
Depends on: 1233895
(Reporter)

Updated

3 years ago
Depends on: 1233899
(Reporter)

Updated

3 years ago
Depends on: 1233903
(Reporter)

Updated

3 years ago
Depends on: 1233904
(Reporter)

Updated

3 years ago
Depends on: 1233906
(Reporter)

Updated

3 years ago
Depends on: 1233907
(Reporter)

Updated

3 years ago
Depends on: 1233908
(Reporter)

Updated

3 years ago
Depends on: 1233911
(Reporter)

Updated

3 years ago
Depends on: 1233917
(Reporter)

Updated

3 years ago
Alias: createCodebasePrincipal
Blocks: 1233903
No longer depends on: 1233903
(Reporter)

Updated

2 years ago
Depends on: 1196644
(Reporter)

Updated

2 years ago
Component: Security → DOM: Security
Product: Firefox → Core
(Reporter)

Updated

2 years ago
Component: DOM: Security → DOM
(Reporter)

Updated

2 years ago
Depends on: 1237475
(Reporter)

Updated

2 years ago
Depends on: 1237479
(Reporter)

Updated

2 years ago
Depends on: 1237908
(Reporter)

Updated

2 years ago
Depends on: 1237911
(Reporter)

Updated

2 years ago
Depends on: 1237913
(Reporter)

Updated

2 years ago
Depends on: 1237915
(Reporter)

Updated

2 years ago
Depends on: 1237916
(Reporter)

Updated

2 years ago
Depends on: 1237917
(Reporter)

Updated

2 years ago
Whiteboard: [userContextId]
(Reporter)

Updated

2 years ago
Depends on: 1238177
(Reporter)

Updated

2 years ago
Depends on: 1238182
(Reporter)

Updated

2 years ago
Depends on: 1238183
(Reporter)

Updated

2 years ago
Depends on: 1238722
(Reporter)

Updated

2 years ago
Depends on: 1238723
(Reporter)

Updated

2 years ago
Depends on: 1238727
(Reporter)

Updated

2 years ago
Depends on: 1225053
(Reporter)

Updated

2 years ago
Depends on: 1240620
(Reporter)

Updated

2 years ago
Depends on: 1240623
(Reporter)

Updated

2 years ago
Depends on: 1240624
(Reporter)

Updated

2 years ago
Depends on: 1250983
(Reporter)

Updated

2 years ago
Depends on: 1259871
(Reporter)

Updated

2 years ago
Depends on: 1260906
(Reporter)

Updated

2 years ago
Depends on: 1260907
(Reporter)

Updated

2 years ago
Depends on: 1260911
(Reporter)

Updated

2 years ago
Depends on: 1260915
(Reporter)

Updated

2 years ago
Depends on: 1260917
(Reporter)

Updated

2 years ago
Depends on: 1260921
(Reporter)

Updated

2 years ago
Depends on: 1260923
Keywords: meta
(Reporter)

Updated

2 years ago
Assignee: huseby → nobody
Status: ASSIGNED → NEW

Updated

2 years ago
Depends on: 1272416
Blocks: 1191418

Comment 18

8 months ago
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
status-firefox57: --- → affected
Priority: -- → P3
You need to log in before you can comment on or make changes to this bug.