Closed Bug 1209162 Opened 9 years ago Closed 9 years ago

Create OriginAttributes subtypes for different uses and define conversion routines between them

Categories

(Core :: Security: CAPS, defect)

Product:
Core
Component:
Security: CAPS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla45
Tracking Flags:
Tracking Status
firefox45 --- fixed

People

(Reporter: allstars.chh, Assigned: allstars.chh)

References

Details

(Whiteboard: [userContextId])

Attachments

(3 files, 9 obsolete files)

OriginAttributes_in_iframes.JPG
1.71 MB, image/jpeg
Details
interdiff_v2_v3.patch
15.69 KB, patch
sicking
: review+
Details | Diff | Splinter Review
Patch v4.
184.46 KB, patch
allstars.chh
: review+
Details | Diff | Splinter Review 
In https://bugzilla.mozilla.org/show_bug.cgi?id=1165466#c86 and 
https://bugzilla.mozilla.org/show_bug.cgi?id=1165466#c92
Jonas and Bobby have discussed the naming of function that will copy OriginAttributes, so I'll finish them in this bug.
Status: NEW → ASSIGNED
Morphing the bug per today's IRL discussion. I'm working up a patch.
Assignee: allstars.chh → bobbyholley
Summary: rename InheritFromDocShellParent in BasePrincipal → Create OriginAttributes subtypes for different uses and define conversion routines between them
I didn't end up needing NeckoOriginAttributes, as far as I could tell. The
HttpChannel stuff seems served by just having an explicit propagation routine.
Attachment #8667661 - Flags: review?(jonas)
Blocks: 1211590
Comment on attachment 8667661 [details] [diff] [review]
Part 1 - Separate subtypes for different OriginAttributes usages. v1

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

::: caps/BasePrincipal.cpp
@@ +40,5 @@
>  {
>    mAppId = aParent.mAppId;
>    mInBrowser = aParent.mInBrowser;
>    mUserContextId = aParent.mUserContextId;
>    mSignedPkg = aParent.mSignedPkg;

So this isn't correct. The mSignedPkg identifier should only be inherited if the parent and child docshell were both loaded from the same package. So this function needs to take a child docshell as well argument as well.

Actually, this problem exists in DocShellOriginAttributes::InheritFromDocToChildDocShell as well.

Also, do we ever inherit docshell to docshell? Doesn't the docshell inherit from the parent document's pricipaloriginattributes?

@@ +56,3 @@
>  
> +void
> +PrincipalOriginAttributes::InheritFromDocShellToChannel(const DocShellOriginAttributes& aDSAttrs)

I still think that we need 

NeckoOriginAttributes::InheritFromPrincipal(const PricipalOriginAttributes& aDocAttrs)
NeckoOriginAttributes::InheritFromDocShell(const PricipalOriginAttributes& aDSAttrs)


Where the former is used when grabbing the cookie-jar info from loadcontext, and the latter when grabbing it from loadinfo. Both of these would drop the mSignedPkg property.

Necko might not use this currently. But it needs to in order to properly drop the mSignedPkg property.

::: docshell/base/LoadContext.cpp
@@ +23,5 @@
>    , mIsNotNull(true)
>  #endif
>  {
> +  // XXXbholley: Is this right?
> +  mOriginAttributes.InheritFromDocToChildDocShell(BasePrincipal::Cast(aPrincipal)->OriginAttributesRef());

I thought this LoadContext class mainly existed to let us send information that normally lives in the docshell to the parent process.

So I would have thought that we should inherit from the docshell here, not from a principal. Do we not have OriginAttributes in the docshell yet?

Or is this a separate constructor used for something else?

::: docshell/base/nsDocShell.cpp
@@ +9388,5 @@
>  nsDocShell::CreatePrincipalFromReferrer(nsIURI* aReferrer,
>                                          nsIPrincipal** aResult)
>  {
> +  PrincipalOriginAttributes attrs;
> +  attrs.InheritFromDocShellToDoc(GetOriginAttributes());

Hmm... come to think of it. The way that we inherit attributes here too depends on the URI of the document.

For example, if the docshell has mSignedPkg set, then that should be inherited into the document if the document has a URL that is within the package, but not if it's a URL that's from elsewhere.

So I think we need to pass the document URL to InheritFromDocShellToDoc as well.

@@ +13929,5 @@
>  
>    if (aIsNavigate || nsContentUtils::IsWorkerLoad(aLoadContentType)) {
> +    // XXXbholley: Is this right?
> +    PrincipalOriginAttributes attrs;
> +    attrs.InheritFromDocShellToDoc(GetOriginAttributes());

Not sure what this code does, so I don't know :(

@@ +14027,5 @@
>    bool isReload = mLoadType & LOAD_CMD_RELOAD;
>  
> +  // XXXbholley: Is this right?
> +  PrincipalOriginAttributes attrs;
> +  attrs.InheritFromDocShellToDoc(GetOriginAttributes());

Same here.

::: netwerk/protocol/http/HttpChannelParent.cpp
@@ +520,5 @@
>        }
>      }
>  
>      if (setChooseApplicationCache) {
> +      PrincipalOriginAttributes attrs;

This one should use NeckoOriginAttributes I think.
Attachment #8667661 - Flags: review?(jonas) → review-
Comment on attachment 8667662 [details] [diff] [review]
Part 2 - Update the propagation routines to reflect the policy we want. v1

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

::: caps/BasePrincipal.cpp
@@ +36,2 @@
>    mUserContextId = aDocAttrs.mUserContextId;
> +  // signedPkg is not propagated to child docshells.

It is if the child docshell has a URI which comes from the package.

@@ +45,2 @@
>    mUserContextId = aParent.mUserContextId;
> +  // signedPkg is not propagated to child docshells.

Same here. Though again, I'm not sure if this function makes sense. I think we generally should inherit from document to docshell.

One reason for this is that if the document doesn't have mSignedPkg set (due to not being loaded from the package), then any <iframe> that it creates should get a docshell without a mSignedPkg. No matter what the parent docshell has as mSignedPkg.
Attachment #8667662 - Flags: review?(jonas) → review-
I'm pretty busy with other things at the moment. Yoshi, do you or someone on the NSEC team have the cycles to work through the review feedback with Jonas and get it landed?
Flags: needinfo?(allstars.chh)
yeah, I'll work on it.
Assignee: bobbyholley → allstars.chh
Flags: needinfo?(allstars.chh)
This is the OriginAttributes in all different iframs we discussed during Mountain View.
Comment on attachment 8667661 [details] [diff] [review]
Part 1 - Separate subtypes for different OriginAttributes usages. v1

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

::: caps/BasePrincipal.cpp
@@ +56,3 @@
>  
> +void
> +PrincipalOriginAttributes::InheritFromDocShellToChannel(const DocShellOriginAttributes& aDSAttrs)

Hi Jonas,
I thought we're going to replace LoadContext in LoadInfo in Bug 1125916, can you explain why we need two different calls for LoadContext and LoadInfo seperately ?

Also could you explain why mSignedPkg should be dropped in NeckoOriginAttributes?
In that case SignedPackage will use the same OriginAttributes as regular web pages (since mSignedPkg is ignored), wouldn't this cause some problems?
Flags: needinfo?(jonas)
> I thought we're going to replace LoadContext in LoadInfo in Bug 1125916, can
> you explain why we need two different calls for LoadContext and LoadInfo
> seperately ?

Simply because it's going to take time before bug 1125916 is fixed. Waiting for that bug to get fixed will only add minor simplifications to this bug, so we should not wait.

> Also could you explain why mSignedPkg should be dropped in
> NeckoOriginAttributes?
> In that case SignedPackage will use the same OriginAttributes as regular web
> pages (since mSignedPkg is ignored), wouldn't this cause some problems?

NeckoOriginAttributes will only be used to add cookies to a request, and where to cache the resulting response. It doesn't affect IndexedDB, localStorage, ServiceWorkers or similar things.

When a page in a signed package creates an <img> or XHR loading data from (for example) facebook.com, we *don't* want to use a separate cookie jar for the cookies in that request.

In that case we want to use the same cookies as when the user browses to facebook.com, or when a normal website creates an <img> or XHR request to facebook.com.

Also note that we never send cookies when loading data from a signed package. When loading the signed package itself, we should use the normal cookies since at that point we are still "outside" the signed package. So here too we want to clear mSignedPkg when determining which cookies to use.

And in all of these cases, since we are using "normal" cookies, we can store the response in the "normal" http cache. The only reason we separate http-cache through OriginAttributes is because the requests are done with different cookies and so the responses might be different. So we should always use the same OriginAttributes to store in the httpcache as was used to determine which cookies to add to a request.

When loading a file within the signed package, we don't send any cookies at all since the package is already downloaded.

All this is explained in https://wiki.mozilla.org/FirefoxOS/New_security_model
Flags: needinfo?(jonas)
(In reply to Jonas Sicking (:sicking) from comment #10)
Hi Jonas
I'd like to ask about InheritFromDocToChildDocShell,
which should be used to inherit OriginAttributes from document when an iframe is created
(from your comment https://bugzilla.mozilla.org/show_bug.cgi?id=1165466#c86)

And see the image from Comment 71,
Inside the signed package, there are three frames, black, red, and green.
The URL of black iframe will be inside the package,
and the URL of red iframe will be outside the package,

When you reviewed bholley's part 2 in Comment 5
you said inside InheritFromDocToChildDocShell,
we need to check if child DocShell's URI is from the same package to decide whether we should inherit mSignedPkg or not.
so black frame should inherit mSignedPkg, and red one shouldn't.

But what about the green frame (mozbrowser frame inside signed package)?
let's say its URI is also *outside* the package,
from your comment 5, the green frame should NOT inherit the signedPkg,

So my first question is 
Is there something wrong about the picture?
Or we need some special handling for the green frame?


> NeckoOriginAttributes will only be used to add cookies to a request, and
> where to cache the resulting response. It doesn't affect IndexedDB,
> localStorage, ServiceWorkers or similar things.
> 
> When a page in a signed package creates an <img> or XHR loading data from
> (for example) facebook.com, we *don't* want to use a separate cookie jar for
> the cookies in that request.
> 
> In that case we want to use the same cookies as when the user browses to
> facebook.com, or when a normal website creates an <img> or XHR request to
> facebook.com.
> 
> Also note that we never send cookies when loading data from a signed
> package. When loading the signed package itself, we should use the normal
> cookies since at that point we are still "outside" the signed package. So
> here too we want to clear mSignedPkg when determining which cookies to use.
> 
but above comment is for the 'share cookie jar' feature in
signed package, right?

What about the green frame again?
In my memory I thought the green frame will have its own cookie jar,
i.e the red frame won't share the cookie with the green frame, even they both navigated to facebook.com

If the green frame should inherit the signedPkg attribute (Question 1),
My second question is,
Should NeckoOriginAttributes inherit the signedPkg attribute from the tabContext instead of 'dropping signedPkg',
so the NeckoOriginAttributes from green frame will have signedPkg attr, and those from the red frame won't have signedPkg attr.

Is my understanding correct?

Thanks
(In reply to Yoshi Huang[:allstars.chh] from comment #11)
> (In reply to Jonas Sicking (:sicking) from comment #10)
> Hi Jonas
> I'd like to ask about InheritFromDocToChildDocShell,
> which should be used to inherit OriginAttributes from document when an
> iframe is created
> (from your comment https://bugzilla.mozilla.org/show_bug.cgi?id=1165466#c86)
> 
> And see the image from Comment 71,
> Inside the signed package, there are three frames, black, red, and green.
> The URL of black iframe will be inside the package,
> and the URL of red iframe will be outside the package,

That picture shows what OriginAttributes the *document*s in these frames have. That doesn't have to be the same as what the *docshell*s in these frames have. That distinction is important.

The OriginAttributes stored in a docshell doesn't directly affect what IndexedDB/localStorage/permissions/cookies that the pages in that docshell get.

The only thing that the OriginAttributes in the docshell should be used for is to help create the correct OriginAttributes for the documents inside that docshell. This does not mean that we simply copy information from the docshell's OriginAttributes to the document's OriginAttributes. It only means that the docshell's OriginAttributes needs to provide enough information to the PrincipalOriginAttributes::InheritFromDocShellToDoc function that it can create the OriginAttributes values that match the ones in the picture.

Consider for example an iframe inside the "signed package" outermost black frame in the picture from comment 71.

As we can see in that picture, if that iframe navigates to a URL inside the package, then the *document* inside the iframe should use the {A:5, B:t, S:'foo'} OriginAttributes. But if you then navigate the same iframe to a URL outside the package, then the *document* inside the iframe should use {A:5, B:t, S:''} OriginAttributes.

However when you navigate an iframe from one URL to another we don't create a new docshell. So how do we get different OriginAttributes in the documents despite that the documents live in the same docshell?

We do this by putting logic in PrincipalOriginAttributes::InheritFromDocShellToDoc which looks at the URL that you navigate to and create different OriginAttributes depending on if the URL is inside or outside of the package. I.e. it compares the URL to the package identified by the mSignedPkg of the docshell. If the URL is inside the same package then we inherit mSignedPkg, otherwise we set mSignedPkg to empty.

> But what about the green frame (mozbrowser frame inside signed package)?
> let's say its URI is also *outside* the package,
> from your comment 5, the green frame should NOT inherit the signedPkg,

When the green frame is created we will call DocShellOriginAttributes::InheritFromDocToChildDocShell. This information needs to get additional parameters indicating if it's a mozbrowser iframe or not. If it is a mozbrowser iframe, then we inherit all the OriginAttributes from the document, except for mSignedPkgInBrowser to true.

> So my first question is 
> Is there something wrong about the picture?

No, the picture is correct, but it shows what OriginAttributes the *documents* should have. Not what the docshells should have.

> Or we need some special handling for the green frame?

All frames here have some form of special handling in some form or another :)

> > NeckoOriginAttributes will only be used to add cookies to a request, and
> > where to cache the resulting response. It doesn't affect IndexedDB,
> > localStorage, ServiceWorkers or similar things.
> > 
> > When a page in a signed package creates an <img> or XHR loading data from
> > (for example) facebook.com, we *don't* want to use a separate cookie jar for
> > the cookies in that request.
> > 
> > In that case we want to use the same cookies as when the user browses to
> > facebook.com, or when a normal website creates an <img> or XHR request to
> > facebook.com.
> > 
> > Also note that we never send cookies when loading data from a signed
> > package. When loading the signed package itself, we should use the normal
> > cookies since at that point we are still "outside" the signed package. So
> > here too we want to clear mSignedPkg when determining which cookies to use.
> > 
> but above comment is for the 'share cookie jar' feature in
> signed package, right?
> 
> What about the green frame again?
> In my memory I thought the green frame will have its own cookie jar,
> i.e the red frame won't share the cookie with the green frame, even they
> both navigated to facebook.com

This is a good point. NeckoOriginAttributes should only clear mSignedPkg if mSignedPkgInBrowser is false. If it is true we leave mSignedPkg as-is.
Thanks for the detail reply, Jonas.
Now I have some detail questions.
1. For 
- PrincipalOriginAttributes.InheritFromDocShellToDoc
- DocShellOriginAttributes.InheritFromDocToChildDocShell
you suggested that we need one additional argument to determine the doc or docshell is *inside the package*,
but how do we know it's inside the package?

Do you imply that we need *two* nsIURIs?
for example, it should look like
- PrincipalOriginAttributes.InheritFromDocShellToDoc(nsIURI* aDocShellURI, nsIURI* aDocURI, DocShellOriginAttributes& aDOA);

Or should we add some helper in PackageAppServiceUtils, and make it looke like 
PrincipalOriginAttributes.InheritFromDocShellToDoc(DocShellOriginAttributes& aDOA, bool aCopy);

2. The OA in nsILoadInfo could be overwritten, but I looked through Bug 1125916 it didn't mention when to override OA in detail, could you explain more on this? Or where are the related bugs for deciding 'when to override OA in nsILoadInfo' ?

Thanks
(In reply to Yoshi Huang[:allstars.chh] from comment #13)
> Thanks for the detail reply, Jonas.
> Now I have some detail questions.
> 1. For 
> - PrincipalOriginAttributes.InheritFromDocShellToDoc
> - DocShellOriginAttributes.InheritFromDocToChildDocShell
> you suggested that we need one additional argument to determine the doc or
> docshell is *inside the package*,
> but how do we know it's inside the package?

For PrincipalOriginAttributes.InheritFromDocShellToDoc you are indeed right that we need to know if the document is inside the package. I would say that for now, just pass the nsIChannel in to the PrincipalOriginAttributes.InheritFromDocShellToDoc function. Or the nsIURI if we don't have access to the channel.

Eventually we probably need to call into some service, like PackageAppServiceUtils, which tracks which signed packages the user has browsed to, similar to the application registry that we have today. This service can then answer questions like if a URL is part of a package.

But that is better to do in a separate bug. In this bug we should just make sure that we have access to the appropriate information in PrincipalOriginAttributes.InheritFromDocShellToDoc.

So PrincipalOriginAttributes.InheritFromDocShellToDoc should look like:
PrincipalOriginAttributes.InheritFromDocShellToDoc(DocShellOriginAttributes& aDOA, nsIChannel* documentChannel);

The function DocShellOriginAttributes.InheritFromDocToChildDocShell should be removed. I don't think that child docshells should inherit from their parent docshell. They should inherit from their parent document using PrincipalOriginAttributes.InheritFromDocShellToDoc.

> 2. The OA in nsILoadInfo could be overwritten, but I looked through Bug
> 1125916 it didn't mention when to override OA in detail, could you explain
> more on this? Or where are the related bugs for deciding 'when to override
> OA in nsILoadInfo' ?

The code in this bug should not overwrite the values in nsILoadInfo.

The code in this bug should add

NeckoOriginAttributes.InheritFromChannel(nsIChannel* aChannel);

For now the implementation of that function should actually ignore the loadinfo and should instead just grab the information from the nsILoadContext. Changing from using loadcontext to use loadinfo is a separate bug.
(In reply to Jonas Sicking (:sicking) from comment #14)
> 
> The function DocShellOriginAttributes.InheritFromDocToChildDocShell should
> be removed. 
Sorry, but do you mean
DocShellOriginAttributes::InheritFromDocShellToChildDocShell should be removed?

> I don't think that child docshells should inherit from their
> parent docshell. They should inherit from their parent document using
> PrincipalOriginAttributes.InheritFromDocShellToDoc.
> 
And this should be 
DocShellOriginAttributes.InheritFromDocToChildDocShell
Attached patch WIP Patch. (obsolete) — — Splinter Review 
So far this patch still has lots of TODO, and still a lot of crashes.
The problem I have right now is still the nsILoadContext,
for necko, it should be NeckoOriginAttributes, but for other cases, it will be inherited by nsDocShell hence should be DocShellOriginAttributes.

I am still figuring out how to make it work in nsILoadContext.
I think nsILoadContext should return a DocShellOriginAttributes. Necko should then convert that into a NeckoOriginAttributes function.

One solution would be to put a function in nsNetUtils.h like

NS_GetChannelOriginAttributes(nsIChannel* aChannel, NeckoOriginAttributes& aAttributes)
{
  nsCOMPtr<nsILoadContext> loadContext = somefunction(aChannel); 
  aAttributes.InheritFromDocShellToNecko(loadContext.getOriginAttributes();
}

Probably a little bit more code will be needed, but hopefully the idea makes sense?
Attached patch Patch. (obsolete) — — Splinter Review 
convert to DocShellOriginAttributes in nsILoadContext
Attachment #8667661 - Attachment is obsolete: true
Attachment #8667662 - Attachment is obsolete: true
Attachment #8683621 - Attachment is obsolete: true
Comment on attachment 8684152 [details] [diff] [review]
Patch.

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

Hi Jonas
Basically I've added
- PrincipalOriginAttributes.InheritFromDocShellToDoc (and will be used in nsDocShell.cpp and nsScriptSecurityManager.cpp)
- DocShellOriginAttributes.InheritFromDocToChildDocShell (see TODO in nsFrameLoader.cpp)
- NeckoOriginAttributes.InheritFromDocShell in necko.

And add TODOs for the inheriting mSignedPkg if the URI is located in the same signed package.

There are still comments neither You or Bobby are not sure in the last review,
I'll check those in detail and fix those soon.

So I'd like request for your feedback here.

Thanks
Attachment #8684152 - Flags: feedback?(jonas)
Attached patch Patch. (obsolete) — — Splinter Review 
- fixed TODO in ShouldPrepareIntercept and ChannelIntercepted
- Removed InheritFromDocShellToChildDocShell
Attachment #8684152 - Attachment is obsolete: true
Attachment #8684152 - Flags: feedback?(jonas)
Comment on attachment 8684856 [details] [diff] [review]
Patch.

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

Generally looks fine. A few comments below.

Next time you attach a patch, please do so as an interdiff though so that I don't have to look through the whole patch again :)

::: dom/base/nsFrameLoader.cpp
@@ +3045,5 @@
> +  if (!aPackageId.IsEmpty()) {
> +    // Only when aPackageId is not empty would signed package origin
> +    // be meaningful.
> +    nsPrincipal::GetOriginForURI(aURI, signedPkgOrigin);
> +  }

I'm not actually sure what aPackageId is?

@@ +3066,5 @@
>  
> +  DocShellOriginAttributes docShellAttrs;
> +  docShellAttrs.InheritFromDocToChildDocShell(attrs, aURI);
> +  bool tabContextUpdated =
> +    aTabContext->SetTabContext(ownApp, containingApp, docShellAttrs, signedPkgOrigin);

Why do we pass the signedPkgOrigin separate from docShellAttrs here? I.e. why not just set docShellAttrs.mSignedPkg instead?

The idea of OriginAttributes is that we should pass that around rather than the individual components separately. But maybe there's a special reason here?

Ok to fix this as a followup though.

::: dom/ipc/TabContext.cpp
@@ -270,5 @@
>        // Otherwise, we're a new app window and we inherit from our
>        // opener app.
> -
> -      // FIXME bug 1212250 - use InheritFromDocToChildDocshell instead
> -      // of copying attributes directly.

Why remove this comment?

::: dom/quota/QuotaManager.cpp
@@ +5287,5 @@
>          if (originProps.mAppId == kUnknownAppId) {
>            rv = secMan->GetSimpleCodebasePrincipal(uri,
>                                                    getter_AddRefs(principal));
>          } else {
> +          PrincipalOriginAttributes attrs(originProps.mAppId, originProps.mInMozBrowser);

'originProps' should definitely use an OriginAttributes instead. Though I guess that will be part of bug 1209349

::: gfx/thebes/gfxSVGGlyphs.cpp
@@ +346,5 @@
>   
>      rv = NS_NewURI(getter_AddRefs(uri), mSVGGlyphsDocumentURI);
>      NS_ENSURE_SUCCESS(rv, rv);
>  
> +    PrincipalOriginAttributes attrs;

Huh! This looks all sorts of wrong. Can you file a followup on making sure that this code grabs an appropriate OriginAttributes from somewhere?

::: netwerk/cookie/CookieServiceParent.cpp
@@ +33,4 @@
>  {
>    MOZ_ASSERT(aAttrs.mAppId != nsIScriptSecurityManager::UNKNOWN_APP_ID);
>  
> +  PrincipalOriginAttributes attrs(aAttrs.mAppId, aAttrs.mInBrowser);

This doesn't look right. We should copy *all* origin attributes over here I think?

::: uriloader/prefetch/OfflineCacheUpdateParent.cpp
@@ +52,5 @@
>  //-----------------------------------------------------------------------------
>  // OfflineCacheUpdateParent <public>
>  //-----------------------------------------------------------------------------
>  
> +OfflineCacheUpdateParent::OfflineCacheUpdateParent(const DocShellOriginAttributes& aAttrs)

I would think we should use PrincipalOriginAttributes here...

@@ +94,5 @@
>  
>      bool offlinePermissionAllowed = false;
>  
> +    PrincipalOriginAttributes principalAttrs;
> +    principalAttrs.InheritFromDocShellToDoc(mOriginAttributes, manifestURI);

...and avoid this conversion. It'll also make it more similar to service workers which are entirely PrincipalOriginAttributes based.

However this code is going to go away soon, so it's not terribly important what we do here.
Attachment #8684856 - Flags: feedback?(jonas) → feedback+
(In reply to Jonas Sicking (:sicking) from comment #27)
> Generally looks fine. A few comments below.
> 
> Next time you attach a patch, please do so as an interdiff though so that I
> don't have to look through the whole patch again :)
> 
Thanks, I'll try to split my patch into differnt parts to make the review easier.

> ::: dom/base/nsFrameLoader.cpp
> @@ +3045,5 @@
> > +  if (!aPackageId.IsEmpty()) {
> > +    // Only when aPackageId is not empty would signed package origin
> > +    // be meaningful.
> > +    nsPrincipal::GetOriginForURI(aURI, signedPkgOrigin);
> > +  }
> 
> I'm not actually sure what aPackageId is?
> 
It's the package id of the signed package.

> @@ +3066,5 @@
> >  
> > +  DocShellOriginAttributes docShellAttrs;
> > +  docShellAttrs.InheritFromDocToChildDocShell(attrs, aURI);
> > +  bool tabContextUpdated =
> > +    aTabContext->SetTabContext(ownApp, containingApp, docShellAttrs, signedPkgOrigin);
> 
> Why do we pass the signedPkgOrigin separate from docShellAttrs here? I.e.
> why not just set docShellAttrs.mSignedPkg instead?
> 
> The idea of OriginAttributes is that we should pass that around rather than
> the individual components separately. But maybe there's a special reason
> here?
> 
> Ok to fix this as a followup though.
Will use DocShellOriginAttributes directly.

> 
> ::: dom/ipc/TabContext.cpp
> @@ -270,5 @@
> >        // Otherwise, we're a new app window and we inherit from our
> >        // opener app.
> > -
> > -      // FIXME bug 1212250 - use InheritFromDocToChildDocshell instead
> > -      // of copying attributes directly.
> 
> Why remove this comment?
> 
I discussed with Kanru, and fixed this in nsFrameLoader.
Will split this as a seperate part patch.

> ::: gfx/thebes/gfxSVGGlyphs.cpp
> @@ +346,5 @@
> >   
> >      rv = NS_NewURI(getter_AddRefs(uri), mSVGGlyphsDocumentURI);
> >      NS_ENSURE_SUCCESS(rv, rv);
> >  
> > +    PrincipalOriginAttributes attrs;
> 
> Huh! This looks all sorts of wrong. Can you file a followup on making sure
> that this code grabs an appropriate OriginAttributes from somewhere?
> 
Bug 1225053

> ::: netwerk/cookie/CookieServiceParent.cpp
> @@ +33,4 @@
> >  {
> >    MOZ_ASSERT(aAttrs.mAppId != nsIScriptSecurityManager::UNKNOWN_APP_ID);
> >  
> > +  PrincipalOriginAttributes attrs(aAttrs.mAppId, aAttrs.mInBrowser);
> 
> This doesn't look right. We should copy *all* origin attributes over here I
> think?
> 
I'll add an conversion cstor
explict PrincipalOriginAttributes(const NeckoOriginAttributes& aAttrs);

> ::: uriloader/prefetch/OfflineCacheUpdateParent.cpp
> @@ +52,5 @@
> >  //-----------------------------------------------------------------------------
> >  // OfflineCacheUpdateParent <public>
> >  //-----------------------------------------------------------------------------
> >  
> > +OfflineCacheUpdateParent::OfflineCacheUpdateParent(const DocShellOriginAttributes& aAttrs)
> 
> I would think we should use PrincipalOriginAttributes here...

It is called from https://dxr.mozilla.org/mozilla-central/source/dom/ipc/ContentParent.cpp?case=true&from=dom/ipc/ContentParent.cpp#5192,
and use the origin attributes from TabContext, which stores DocShellOriginAttributes,
so I'll keep it as it is now.
> Thanks, I'll try to split my patch into differnt parts to make the review
> easier.

Don't split into separate patches. That would still mean I'd have to look at it all again. Please do an interdiff.

Though I guess, if you want or think it'll make things easier, feel free to split up the interdiff. Up to you.
Attached patch Create OriginAttributes subtypes (obsolete) — — Splinter Review 
From 96337ca0ab0f8c2f45585959a947898ae4c5f81a Mon Sep 17 00:00:00 2001
---
 caps/BasePrincipal.cpp                             |  75 +++++++++---
 caps/BasePrincipal.h                               | 126 ++++++++++++++------
 caps/nsJSPrincipals.cpp                            |   2 +-
 caps/nsNullPrincipal.cpp                           |   4 +-
 caps/nsNullPrincipal.h                             |   4 +-
 caps/nsPrincipal.cpp                               |   4 +-
 caps/nsPrincipal.h                                 |   2 +-
 caps/nsScriptSecurityManager.cpp                   |  30 ++---
 caps/nsScriptSecurityManager.h                     |   4 +-
 caps/tests/gtest/TestOriginAttributes.cpp          |  18 +--
 docshell/base/LoadContext.cpp                      |   3 +-
 docshell/base/LoadContext.h                        |   8 +-
 docshell/base/SerializedLoadContext.h              |   2 +-
 docshell/base/nsDocShell.cpp                       |  29 +++--
 docshell/base/nsDocShell.h                         |   2 +-
 docshell/base/nsILoadContext.idl                   |   7 +-
 dom/base/ChromeUtils.cpp                           |   4 +-
 dom/base/nsFrameLoader.cpp                         |  27 ++---
 dom/base/nsFrameLoader.h                           |   2 +-
 dom/base/nsGlobalWindow.cpp                        |   4 +-
 dom/cache/DBSchema.cpp                             |   2 +-
 dom/datastore/DataStoreService.cpp                 |   4 +-
 dom/ipc/AppProcessChecker.cpp                      |   2 +-
 dom/ipc/ContentChild.cpp                           |   2 +-
 dom/ipc/TabContext.cpp                             |   9 +-
 dom/ipc/TabContext.h                               |  12 +-
 dom/ipc/TabParent.cpp                              |   6 +-
 dom/quota/QuotaManager.cpp                         |   4 +-
 dom/webidl/ChromeUtils.webidl                      |   2 +-
 dom/workers/PServiceWorkerManager.ipdl             |   6 +-
 dom/workers/ServiceWorkerManager.cpp               |  28 ++---
 dom/workers/ServiceWorkerManager.h                 |  18 +--
 dom/workers/ServiceWorkerManagerChild.cpp          |   2 +-
 dom/workers/ServiceWorkerManagerChild.h            |   4 +-
 dom/workers/ServiceWorkerManagerParent.cpp         |   2 +-
 dom/workers/ServiceWorkerManagerParent.h           |   4 +-
 dom/workers/ServiceWorkerManagerService.cpp        |   2 +-
 dom/workers/ServiceWorkerManagerService.h          |   4 +-
 dom/workers/ServiceWorkerPrivate.cpp               |   2 +-
 dom/workers/ServiceWorkerRegistrar.cpp             |   2 +-
 dom/workers/test/gtest/TestReadWrite.cpp           |   4 +-
 extensions/cookie/nsPermission.cpp                 |   6 +-
 extensions/cookie/nsPermissionManager.cpp          |   8 +-
 gfx/thebes/gfxSVGGlyphs.cpp                        |   3 +-
 ipc/glue/BackgroundUtils.h                         |  24 +++-
 ipc/glue/PBackgroundSharedTypes.ipdlh              |   4 +-
 js/xpconnect/src/Sandbox.cpp                       |   2 +-
 netwerk/base/LoadContextInfo.cpp                   |  27 +++--
 netwerk/base/LoadContextInfo.h                     |   6 +-
 netwerk/base/LoadInfo.cpp                          |  11 +-
 netwerk/base/LoadInfo.h                            |   4 +-
 netwerk/base/Predictor.cpp                         |   2 +-
 netwerk/base/nsILoadContextInfo.idl                |   6 +-
 netwerk/base/nsILoadInfo.idl                       |  12 +-
 netwerk/base/nsNetUtil.cpp                         |   6 +-
 netwerk/base/nsNetUtil.h                           |   6 +-
 netwerk/cache/nsApplicationCacheService.cpp        |   4 +-
 netwerk/cache/nsDiskCacheDeviceSQL.cpp             |  12 +-
 netwerk/cache/nsDiskCacheDeviceSQL.h               |   4 +-
 netwerk/cache2/AppCacheStorage.cpp                 |   2 +-
 netwerk/cache2/CacheFileMetadata.h                 |   4 +-
 netwerk/cache2/CacheFileUtils.cpp                  |   4 +-
 netwerk/cache2/CacheObserver.cpp                   |   8 +-
 netwerk/cache2/OldWrappers.cpp                     |   2 +-
 netwerk/cookie/CookieServiceParent.cpp             |  26 ++--
 netwerk/cookie/CookieServiceParent.h               |   4 +-
 netwerk/cookie/nsCookieService.cpp                 |  28 ++---
 netwerk/cookie/nsCookieService.h                   |  14 +--
 netwerk/ipc/NeckoChannelParams.ipdlh               |   3 +-
 netwerk/ipc/NeckoParent.cpp                        |  15 +--
 netwerk/ipc/NeckoParent.h                          |   2 +-
 netwerk/protocol/http/HttpChannelParent.cpp        |  12 +-
 netwerk/protocol/http/PackagedAppService.cpp       |   2 +-
 netwerk/protocol/http/PackagedAppVerifier.cpp      |   2 +-
 netwerk/protocol/http/nsHttpAuthCache.cpp          |   4 +-
 .../protocol/http/nsHttpChannelAuthProvider.cpp    |   2 +-
 netwerk/protocol/wyciwyg/nsWyciwygChannel.h        |   2 +-
 netwerk/test/mochitests/mochitest.ini              |   1 +
 .../test/mochitests/signed_web_packaged_app.sjs    |   1 +
 .../test_origin_attributes_conversion.html         | 132 +++++++++++++++++++++
 .../components/downloads/ApplicationReputation.cpp |   4 +-
 .../nsUrlClassifierStreamUpdater.cpp               |   2 +-
 uriloader/prefetch/OfflineCacheUpdateParent.cpp    |   9 +-
 uriloader/prefetch/OfflineCacheUpdateParent.h      |   4 +-
 uriloader/prefetch/nsOfflineCacheUpdateService.cpp |   4 +-
 85 files changed, 604 insertions(+), 314 deletions(-)
 create mode 100644 netwerk/test/mochitests/test_origin_attributes_conversion.html
Attachment #8684856 - Attachment is obsolete: true
Attached patch Patch v2. (obsolete) — — Splinter Review 
add skip-if for e10s and only for browser
Attachment #8688281 - Attachment is obsolete: true
Attached patch interdiff_v1_v2.patch (obsolete) — — Splinter Review 
interdiff v1-v2
Comment on attachment 8688460 [details] [diff] [review]
interdiff_v1_v2.patch

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

Hi Jonas
This is the interdiff as you requested.
I rebase my v1 and v2 patches on the same latest commit so should make it easier to looking into the interdiff.

Note:
Since I modified the test signed package used in mochitest(signed_web_packaged_app.sjs), which means I have to update the signature as well, will provide another patch to fix the signature soon, right now this patch will have the verification fail in mochitest, but shouldn't affect the patch itself to fix the inheritance of origin attributes.

Thanks
Attachment #8688460 - Flags: review?(jonas)
(In reply to Yoshi Huang[:allstars.chh] from comment #28)
> > 
> > ::: dom/ipc/TabContext.cpp
> > @@ -270,5 @@
> > >        // Otherwise, we're a new app window and we inherit from our
> > >        // opener app.
> > > -
> > > -      // FIXME bug 1212250 - use InheritFromDocToChildDocshell instead
> > > -      // of copying attributes directly.
> > 
> > Why remove this comment?
> > 
> I discussed with Kanru, and fixed this in nsFrameLoader.
> Will split this as a seperate part patch.
> 
Explained in Bug 1212250.
 
> > ::: netwerk/cookie/CookieServiceParent.cpp
> > @@ +33,4 @@
> > >  {
> > >    MOZ_ASSERT(aAttrs.mAppId != nsIScriptSecurityManager::UNKNOWN_APP_ID);
> > >  
> > > +  PrincipalOriginAttributes attrs(aAttrs.mAppId, aAttrs.mInBrowser);
> > 
> > This doesn't look right. We should copy *all* origin attributes over here I
> > think?
> > 
> I'll add an conversion cstor
> explict PrincipalOriginAttributes(const NeckoOriginAttributes& aAttrs);
>
In the end I use origin suffix to do the conversion.
NeckoOA -> suffix -> PrincipalOA.
> > ::: dom/base/nsFrameLoader.cpp
> > @@ +3045,5 @@
> > > +  if (!aPackageId.IsEmpty()) {
> > > +    // Only when aPackageId is not empty would signed package origin
> > > +    // be meaningful.
> > > +    nsPrincipal::GetOriginForURI(aURI, signedPkgOrigin);
> > > +  }
> > 
> > I'm not actually sure what aPackageId is?
> > 
> It's the package id of the signed package.

If this is only for signed packages, can you rename this to aSignedPackageId?
Comment on attachment 8688460 [details] [diff] [review]
interdiff_v1_v2.patch

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

Getting close. A couple of more comments still.

::: b/caps/BasePrincipal.cpp
@@ +63,1 @@
>    mSignedPkg = aAttrs.mSignedPkg;

Actually, I forgot about this before, but DocShellOriginAttributes::InheritFromDocToChildDocShell doesn't depend on the URI that we are opening.

I.e. when we inherit from a document into a docshell then the inherited OriginAttributes are the same independent of what url is loaded. 

However when we then load a document into that docshell, the inherited properties depend on the uri of that document. But PrincipalOriginAttributes::InheritFromDocShellToDoc will take care of that.

So, in short, remove the 'aURI' argument from DocShellOriginAttributes::InheritFromDocToChildDocShell, and remove the comment above. The inheritance here is correct as they are now.

However we might want to pass in the iframe element that we're doing the inheritance for. See frameloader comment below.

::: b/docshell/base/nsDocShell.cpp
@@ +13832,5 @@
>    }
>  
> +  if (!mSignedPkg.IsEmpty()) {
> +    attrs.mSignedPkg = mSignedPkg;
> +  }

Why this change? This doesn't look like it will make any difference one way or another since attrs.mSignedPkg should be empty here.

::: b/dom/base/nsFrameLoader.cpp
@@ +3067,2 @@
>    bool tabContextUpdated =
> +    aTabContext->SetTabContext(ownApp, containingApp, attrs, signedPkgOrigin);

This function is generally pretty bad. I.e. it means that most of the members in OriginAttributes will be cleared.

Can you file a followup on cleaning it up.

Maybe we should add a function like:

DocShellOriginAttributes::InheritFromDocToChildDocShell(PrincipalOriginAttributes& aAttrs, nsIContent* iframeElement)

This function could then look at the mozbrowser attributes and set the appropriate values on the docshelloriginattributes.

But it's fine to do this as a followup.

::: b/dom/ipc/TabParent.cpp
@@ +2824,5 @@
>    } else {
> +    nsAutoCString suffix;
> +    OriginAttributesRef().CreateSuffix(suffix);
> +    DocShellOriginAttributes attrs;
> +    attrs.PopulateFromSuffix(suffix);

I don't understand what this code is trying to do? How is 'attrs' different from the attributes returned by 'OriginAttributesRef()'? Doesn't OriginAttributesRef() already return a DocShellOriginAttributes?

::: b/netwerk/cookie/CookieServiceParent.cpp
@@ +36,5 @@
> +  // conversion from NeckoOA -> Suffix -> PrincipalOA.
> +  nsAutoCString suffix;
> +  aAttrs.CreateSuffix(suffix);
> +  PrincipalOriginAttributes attrs;
> +  attrs.PopulateFromSuffix(suffix);

Add a function like PrincipalOriginAttributes.InheritFromNecko(NeckoOriginAttributes& aAttrs) instead.

The purpose of the *OA.InheritFrom* functions is so that we have a single point where we can apply logic specific to certain members in the OA structs as they flow through different parts of gecko.

Doing convertion through the types by going through a string defeats that purpose. If that's ever needed just add more InheritFrom functions instead.

::: b/netwerk/protocol/http/HttpChannelParent.cpp
@@ +569,5 @@
>  
> +      nsAutoCString suffix;
> +      docShellAttrs.CreateSuffix(suffix);
> +      PrincipalOriginAttributes attrs;
> +      attrs.PopulateFromSuffix(suffix);

Same here. We should use some form of InheritFrom function here.
Attachment #8688460 - Flags: review?(jonas) → review-
The interdiff was very helpful! For the next round, can you attach both interdiff and a final diff?
Comment on attachment 8688460 [details] [diff] [review]
interdiff_v1_v2.patch

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

::: b/docshell/base/nsDocShell.cpp
@@ +13832,5 @@
>    }
>  
> +  if (!mSignedPkg.IsEmpty()) {
> +    attrs.mSignedPkg = mSignedPkg;
> +  }

In Bug 1178526 we've set the mSignedPkg on the topmost docshell, so this if-clause should be used for the topmost docshell, i.e. the one lives in TabChild. I'll move this to the else part of 'if (parent)'
Attached patch Patch. v3 (obsolete) — — Splinter Review 

        Attachment #8688459 -
        Attachment is obsolete: true

    
    

    
  
Comment on attachment 8690022 [details] [diff] [review]
interdiff_v2_v3.patch

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

Hi Jonas
I've addressed your comments, could you help to check this again?
And the final patch is located in https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=1209162&attachment=8690021
This is the interdiff one.

Also since I've modified the test signed package in signed_web_packaged_app.sjs, so I have to update the signature as well.
And our signing tool will generate a new package-identifier each time, therefore I also have to update package-identifier in the tests.
(will discuss this with :jhao who made the signing tool).

        Attachment #8690022 -
        Flags: review?(jonas)

    
    

    
  
Comment on attachment 8690022 [details] [diff] [review]
interdiff_v2_v3.patch

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

Looks good! Just one more followup comment below.

::: b/docshell/base/nsDocShell.cpp
@@ +13828,1 @@
>    }

Ah, I see how this function works now.

I think we can make this simpler and more generic, but as a followup.

What we do right now (if I understand correctly) is that whenever we create a docshell, we call various functions like SetOwnOrContainingAppId, SetFrameType and SetSignedPackageId(). All of which set various members in the docshell.

What we instead should do is let docshell have a mOriginAttributes member, and then have a function like nsDocShell::SetOriginAttributes(DocShellOriginAttributes& aAttrs) which sets that member.

Then the nsDocShell::GetOriginAttributes() function can simply return that member.

Can you make sure we file a followup?

We can probably do this at the same time as we fix up nsFrameLoader::GetNewTabContext since they both deal with inheriting into child docshells.

        Attachment #8690022 -
        Flags: review?(jonas) → review+

    
    

    
  
(In reply to Yoshi Huang[:allstars.chh] from comment #45)
> And our signing tool will generate a new package-identifier each time,
> therefore I also have to update package-identifier in the tests.
Discussed with jhao and I found that the signing tool could keep the package identifier.
I'll revert those changes of package-identifier in tests.


(In reply to Jonas Sicking (:sicking) from comment #46)
> 
> What we instead should do is let docshell have a mOriginAttributes member,
> and then have a function like
> nsDocShell::SetOriginAttributes(DocShellOriginAttributes& aAttrs) which sets
> that member.
> 
> Then the nsDocShell::GetOriginAttributes() function can simply return that
> member.
> 
> Can you make sure we file a followup?
> 
Bug 1227861.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch v4.
        — 
          — Splinter Review
      

    


  
revert package-identifier changes in tests.

        Attachment #8690021 -
        Attachment is obsolete: true

        Attachment #8691769 -
        Flags: review+

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/0e4c4db3b90f
Status: ASSIGNED → RESOLVED
Closed: 9 years ago

          status-firefox45:
          --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45

    
  
Blocks: 1228900
Whiteboard: [userContextId]

    
    

    
  
Could someone explain addonID usage now? It is something very different to other origin attributes (and if it is such, we should remove it being an origin attribute).
There used to be code like
-  mAddonId = attrs.mAddonId;
before the patch, but now it is like
+  // addonId is computed from the principal URI and never propagated
Flags: needinfo?(jonas)
Flags: needinfo?(allstars.chh)

    
    

    
  
Sorry I am not sure about addonId, AFAIK it is designed/implemented by Bholley and billm
Flags: needinfo?(allstars.chh)

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: