The default bug view has changed. See this FAQ.

Update GetCodebasePrincipal callers to use the correct "data jar"

RESOLVED FIXED in mozilla17

Status

()

Core
Security: CAPS
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: sicking, Assigned: sicking)

Tracking

(Blocks: 1 bug, {addon-compat})

Trunk
mozilla17
addon-compat
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-basecamp:+, firefox17-)

Details

(Whiteboard: [qa-])

Attachments

(8 attachments, 4 obsolete attachments)

4.76 KB, patch
mrbkap
: review+
sicking
: checkin+
Details | Diff | Splinter Review
1.04 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
4.92 KB, patch
sicking
: checkin+
Details | Diff | Splinter Review
6.87 KB, patch
mounir
: review+
Details | Diff | Splinter Review
5.19 KB, patch
mounir
: review+
Details | Diff | Splinter Review
9.35 KB, patch
mounir
: review+
sicking
: review+
Details | Diff | Splinter Review
8.78 KB, patch
mounir
: review+
Details | Diff | Splinter Review
19.17 KB, patch
mounir
: review+
Details | Diff | Splinter Review
Right now GetCodebasePrincipal only takes a single argument, the URI to create a principal for.

We're turning this into three separate functions:
GetCodebasePrincipal: Used when we don't know or don't care which data jar is used. These principals are not allowed to be used to access local data.
GetAppCodebasePrincipal: Takes an additional app-id and mozbrowser flag and creates a principal which has full access to local data.
GetNoAppCodebasePrincipal: Same as GetAppCodebasePrincipal but always uses the "null app".

Patches coming up which changes necessary GetCodebasePrincipal callers.
Created attachment 642876 [details] [diff] [review]
Update about: redirectors to remove getCodebasePrincipal calls

Currently we are very careful to ensure that when a channel is created for a about: URI, we always call SetOwner with a "about:foo"-uri codebase principal. This to avoid that the channel gets a system principal.

However the only reason that the channel would get a system principal is because the chrome channel calls SetOwner(systemPrincipal). Otherwise we should be getting the correct principal.

So this patch simply makes us call SetOwner(null) so that about: uris that shouldn't have the system principal follows the normal codepaths in GetChannelPrincipal and gets a codebase principal for the original about: URI.
Assignee: nobody → jonas
Attachment #642876 - Flags: review?(bzbarsky)
Created attachment 642877 [details] [diff] [review]
Make sessionstorage code pass in docshell when creating principal

This makes sessionstorage call GetAppCodebasePrincipal when creating principals. There's also some cleanup to use principal.extendedOrigin rather than using URI.prePath and remove the only getSessionStorageForURI caller.
Created attachment 642878 [details] [diff] [review]
Remove unused aTarget argument
Attachment #642878 - Flags: review?(mrbkap)
Created attachment 642879 [details] [diff] [review]
Fix xpc-sandbox creation code
Attachment #642879 - Flags: review?(mrbkap)
Created attachment 642880 [details] [diff] [review]
Fix browser code and tests
Attachment #642880 - Flags: review?(mounir)
Blocks: 756644
blocking-basecamp: --- → +
OS: Mac OS X → All
Hardware: x86 → All
Attachment #642880 - Flags: review?(mounir) → review+
Created attachment 643102 [details] [diff] [review]
Update about: redirectors to remove getCodebasePrincipal calls

Somehow I missed the mobile about: redirectors. This patch includes them too.
Attachment #642876 - Attachment is obsolete: true
Attachment #642876 - Flags: review?(bzbarsky)
Attachment #643102 - Flags: review?
(In reply to Jonas Sicking (:sicking) from comment #6)
> Somehow I missed the mobile about: redirectors. This patch includes them too.

You also missed the reviewer ;)
Could you also change calls from GetCodebasePrincipal() to GetSimpleCodebasePrincipal() so we make sure all consumers know what they are doing, see bug 758258.
Attachment #643102 - Flags: review? → review?(bzbarsky)

Updated

5 years ago
Attachment #642878 - Flags: review?(mrbkap) → review+
Comment on attachment 642879 [details] [diff] [review]
Fix xpc-sandbox creation code

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

::: js/xpconnect/src/XPCComponents.cpp
@@ +3407,5 @@
>          do_GetService(kScriptSecurityManagerContractID);
>      NS_ENSURE_TRUE(secman, NS_ERROR_FAILURE);
>  
> +    // It would be cool to allow passing in the app-id and mozbrowser info
> +    // to the sandbox constructor. But right now we won't have that ability.

Isn't this use-case taken care of by passing in a window instead of a string?
Attachment #642879 - Flags: review?(mrbkap) → review+
Comment on attachment 643102 [details] [diff] [review]
Update about: redirectors to remove getCodebasePrincipal calls

Please add some comments explaining why setting null will do the right thing, ok?

r=me
Attachment #643102 - Flags: review?(bzbarsky) → review+
Comment on attachment 643102 [details] [diff] [review]
Update about: redirectors to remove getCodebasePrincipal calls

https://hg.mozilla.org/integration/mozilla-inbound/rev/096a755c48ea
Attachment #643102 - Flags: checkin+
Comment on attachment 642878 [details] [diff] [review]
Remove unused aTarget argument

https://hg.mozilla.org/integration/mozilla-inbound/rev/ebfa4531ca57
Attachment #642878 - Flags: checkin+
https://hg.mozilla.org/mozilla-central/rev/096a755c48ea
https://hg.mozilla.org/mozilla-central/rev/ebfa4531ca57
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
I must have forgotten to hit save when adding [leave open] to the whiteboard.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Whiteboard: [leave open]
Created attachment 643682 [details] [diff] [review]
Make sessionStorage/localStorage code create the correct principals
Attachment #642877 - Attachment is obsolete: true
Attachment #643682 - Flags: review?(mounir)
Created attachment 643683 [details] [diff] [review]
Fix browser code and tests
Attachment #643683 - Flags: review?(mounir)
Attachment #642880 - Attachment is obsolete: true
Attachment #643683 - Flags: review?(mounir) → review+
Comment on attachment 643682 [details] [diff] [review]
Make sessionStorage/localStorage code create the correct principals

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

r=me with the comments applied.

::: browser/components/sessionstore/src/SessionStorage.jsm
@@ +56,2 @@
>  
> +      if (principal) {

As long as you are here:
if (!principal) {
  return data;
}

@@ +140,5 @@
>     * Returns a given history entry's URI.
>     * @param aHistory
>     *        That tab's session history
>     * @param aIndex
>     *        The history entry's index

Update the doc.
Attachment #643682 - Flags: review?(mounir) → review+
Created attachment 644198 [details] [diff] [review]
Add GetDocShellCodebasePrincipal

I wish we could have called this method GetAppCodebasePrincipal, but XPCOM idl doesn't allow overloads :(
Attachment #644198 - Flags: review?(mounir)
Created attachment 644199 [details] [diff] [review]
Make getChannelPrincipal get the correct app principal
Attachment #644199 - Flags: review?(mounir)
Comment on attachment 644199 [details] [diff] [review]
Make getChannelPrincipal get the correct app principal

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

the test changes here are by Mounir and are rs=me
Attachment #644199 - Flags: review+
Attachment #644198 - Flags: review?(mounir) → review+
Attachment #644199 - Flags: review?(mounir) → review+
Created attachment 644219 [details] [diff] [review]
Make sessionStorage code create the correct principals

Actually passes tests now
Attachment #643682 - Attachment is obsolete: true
Attachment #644219 - Flags: review?(mounir)
Created attachment 644220 [details] [diff] [review]
Rename getCodebasePrincipal to getSimpleCodebasePrincipal
Attachment #644220 - Flags: review?(mounir)
Attachment #644219 - Flags: review?(mounir) → review+
Comment on attachment 644220 [details] [diff] [review]
Rename getCodebasePrincipal to getSimpleCodebasePrincipal

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

You should probably update nsIScriptSecurityManager uuid. Not a big deal given that it has been updated many times for a lot of related patches.
Attachment #644220 - Flags: review?(mounir) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/d2bac914080d
https://hg.mozilla.org/integration/mozilla-inbound/rev/365d6054c8b6
https://hg.mozilla.org/integration/mozilla-inbound/rev/8906997e57e6

Plus followup:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1f976bd5e456
Even after a landing, backout, relanding, multiple followups, disable test_principal_extendedorigin_appid_appstatus.html still unfortunately failed, so I've just disabled the whole test for now:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d7ff9297eedf

I realise the current Try situation is dire (bug 772458), but inbound has now been busted for 40 consecutive pushes (~90 changesets worth); and pretty much all due to this bug and those related to it. This causes delays for all other devs, means some people start dumping on mozilla-central - and so when I finally get green and merge the ~140 outstanding inbound changesets to m-c, I'm much more likely to get conflicts (in areas of the codebase I have no idea about). We'll also likely have more bustage later today, as everyone who has been delayed sees the tree is open and dumps on it straight away.

I really do feel your pain about Try (and the urgency surrounding B2G goals), which is why I've tried to be more flexible in this instance (eg international calls to mounir to wake him up rather than just back everything out), but I don't see this as being very sustainable moving forwards.

The best course of action would be for you to both ask your manager to put pressure on those responsible for bug 772458 - I've tried to help out on it in my spare time, but I'm not really that familiar with the build/infra stuff, so it needs some #build/#it/#???? loving.
https://hg.mozilla.org/mozilla-central/rev/d2bac914080d
https://hg.mozilla.org/mozilla-central/rev/365d6054c8b6
https://hg.mozilla.org/mozilla-central/rev/8906997e57e6
https://hg.mozilla.org/mozilla-central/rev/d7ff9297eedf
https://hg.mozilla.org/integration/mozilla-inbound/rev/2328647c5d6d
https://hg.mozilla.org/integration/mozilla-inbound/rev/c3972de3f4a5
https://hg.mozilla.org/integration/mozilla-inbound/rev/224e9f2148b2
https://hg.mozilla.org/integration/mozilla-inbound/rev/24d60aa478bb
Whiteboard: [leave open]
https://hg.mozilla.org/mozilla-central/rev/24d60aa478bb
https://hg.mozilla.org/mozilla-central/rev/224e9f2148b2
https://hg.mozilla.org/mozilla-central/rev/c3972de3f4a5
https://hg.mozilla.org/mozilla-central/rev/2328647c5d6d
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
Depends on: 776402

Comment 29

5 years ago
I've just tripped over this change in my extension. I don't know how common it is for extensions to use this interface (probably less than it should...) but I'm gonna mark it addon-compat.
Keywords: addon-compat

Updated

5 years ago
Depends on: 776577
Blocks: 776577, 776402
No longer depends on: 776577, 776402
Blocks: 784041
Depends on: 786009

Updated

5 years ago
Whiteboard: [qa-]
Depends on: 787810

Updated

5 years ago
Blocks: 787810
No longer depends on: 787810

Comment 30

5 years ago
This breaks addons.  I don't know how many, but one of mine at least.  Renaming the deprecated method (is it going to go completely one day?) with no transition period is not exactly helpful to people who need to provide support to older versions while making an addon work with the latest version.
The reason we removed the existing functions was that enough of the security model changed enough that we couldn't keep both models around, and we wanted everyone to make sure that all code call the appropriate function.

However the new model has turned out to be more B2G specific than we intended. The plan is to fix that as soon as we have time (and as soon as Firefox intends to take advantage of cookie jars).

I'd be fine with naming back getNoAppCodebasePrincipal to getCodebasePrincipal if it's really needed. But if it's not affecting too many addons I'd prefer to keep things as-is.

Updated

5 years ago
Depends on: 804411
getCodebasePrincipal is used by at least a couple dozen add-ons. Does naming the function back as getCodebasePrincipal fixes the compatibility issue, though? Didn't its signature change in any way?
All that changed was the name, not the signature.

I do think we should change the name back....
tracking-firefox17: --- → ?
(In reply to Jonas Sicking (:sicking) from comment #31)
> I'd be fine with naming back getNoAppCodebasePrincipal to
> getCodebasePrincipal if it's really needed. But if it's not affecting too
> many addons I'd prefer to keep things as-is.

Let's track this in bug 806587.
tracking-firefox17: ? → -
You need to log in before you can comment on or make changes to this bug.