Last Comment Bug 774585 - Update GetCodebasePrincipal callers to use the correct "data jar"
: Update GetCodebasePrincipal callers to use the correct "data jar"
Status: RESOLVED FIXED
[qa-]
: addon-compat
Product: Core
Classification: Components
Component: Security: CAPS (show other bugs)
: Trunk
: All All
: -- normal with 1 vote (vote)
: mozilla17
Assigned To: Jonas Sicking (:sicking) No longer reading bugmail consistently
:
Mentors:
Depends on: 786009 804411
Blocks: app-data-jars 776402 776577 784041 787810
  Show dependency treegraph
 
Reported: 2012-07-17 00:09 PDT by Jonas Sicking (:sicking) No longer reading bugmail consistently
Modified: 2012-10-29 15:21 PDT (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
-


Attachments
Update about: redirectors to remove getCodebasePrincipal calls (3.32 KB, patch)
2012-07-17 00:15 PDT, Jonas Sicking (:sicking) No longer reading bugmail consistently
no flags Details | Diff | Splinter Review
Make sessionstorage code pass in docshell when creating principal (8.52 KB, patch)
2012-07-17 00:24 PDT, Jonas Sicking (:sicking) No longer reading bugmail consistently
no flags Details | Diff | Splinter Review
Remove unused aTarget argument (4.76 KB, patch)
2012-07-17 00:25 PDT, Jonas Sicking (:sicking) No longer reading bugmail consistently
mrbkap: review+
jonas: checkin+
Details | Diff | Splinter Review
Fix xpc-sandbox creation code (1.04 KB, patch)
2012-07-17 00:26 PDT, Jonas Sicking (:sicking) No longer reading bugmail consistently
mrbkap: review+
Details | Diff | Splinter Review
Fix browser code and tests (4.00 KB, patch)
2012-07-17 00:27 PDT, Jonas Sicking (:sicking) No longer reading bugmail consistently
mounir: review+
Details | Diff | Splinter Review
Update about: redirectors to remove getCodebasePrincipal calls (4.92 KB, patch)
2012-07-17 12:54 PDT, Jonas Sicking (:sicking) No longer reading bugmail consistently
bzbarsky: review+
jonas: checkin+
Details | Diff | Splinter Review
Make sessionStorage/localStorage code create the correct principals (8.52 KB, patch)
2012-07-18 17:09 PDT, Jonas Sicking (:sicking) No longer reading bugmail consistently
mounir: review+
Details | Diff | Splinter Review
Fix browser code and tests (6.87 KB, patch)
2012-07-18 17:12 PDT, Jonas Sicking (:sicking) No longer reading bugmail consistently
mounir: review+
Details | Diff | Splinter Review
Add GetDocShellCodebasePrincipal (5.19 KB, patch)
2012-07-19 23:37 PDT, Jonas Sicking (:sicking) No longer reading bugmail consistently
mounir: review+
Details | Diff | Splinter Review
Make getChannelPrincipal get the correct app principal (9.35 KB, patch)
2012-07-19 23:38 PDT, Jonas Sicking (:sicking) No longer reading bugmail consistently
mounir: review+
jonas: review+
Details | Diff | Splinter Review
Make sessionStorage code create the correct principals (8.78 KB, patch)
2012-07-20 01:05 PDT, Jonas Sicking (:sicking) No longer reading bugmail consistently
mounir: review+
Details | Diff | Splinter Review
Rename getCodebasePrincipal to getSimpleCodebasePrincipal (19.17 KB, patch)
2012-07-20 01:13 PDT, Jonas Sicking (:sicking) No longer reading bugmail consistently
mounir: review+
Details | Diff | Splinter Review

Description Jonas Sicking (:sicking) No longer reading bugmail consistently 2012-07-17 00:09:43 PDT
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.
Comment 1 Jonas Sicking (:sicking) No longer reading bugmail consistently 2012-07-17 00:15:36 PDT
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.
Comment 2 Jonas Sicking (:sicking) No longer reading bugmail consistently 2012-07-17 00:24:17 PDT
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.
Comment 3 Jonas Sicking (:sicking) No longer reading bugmail consistently 2012-07-17 00:25:12 PDT
Created attachment 642878 [details] [diff] [review]
Remove unused aTarget argument
Comment 4 Jonas Sicking (:sicking) No longer reading bugmail consistently 2012-07-17 00:26:04 PDT
Created attachment 642879 [details] [diff] [review]
Fix xpc-sandbox creation code
Comment 5 Jonas Sicking (:sicking) No longer reading bugmail consistently 2012-07-17 00:27:28 PDT
Created attachment 642880 [details] [diff] [review]
Fix browser code and tests
Comment 6 Jonas Sicking (:sicking) No longer reading bugmail consistently 2012-07-17 12:54:31 PDT
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.
Comment 7 Mounir Lamouri (:mounir) 2012-07-17 13:02:53 PDT
(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 ;)
Comment 8 Mounir Lamouri (:mounir) 2012-07-17 15:34:58 PDT
Could you also change calls from GetCodebasePrincipal() to GetSimpleCodebasePrincipal() so we make sure all consumers know what they are doing, see bug 758258.
Comment 9 Blake Kaplan (:mrbkap) 2012-07-17 18:59:35 PDT
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?
Comment 10 Boris Zbarsky [:bz] (TPAC) 2012-07-17 21:36:19 PDT
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
Comment 11 Jonas Sicking (:sicking) No longer reading bugmail consistently 2012-07-17 22:25:20 PDT
Comment on attachment 643102 [details] [diff] [review]
Update about: redirectors to remove getCodebasePrincipal calls

https://hg.mozilla.org/integration/mozilla-inbound/rev/096a755c48ea
Comment 12 Jonas Sicking (:sicking) No longer reading bugmail consistently 2012-07-17 22:37:49 PDT
Comment on attachment 642878 [details] [diff] [review]
Remove unused aTarget argument

https://hg.mozilla.org/integration/mozilla-inbound/rev/ebfa4531ca57
Comment 14 Jonas Sicking (:sicking) No longer reading bugmail consistently 2012-07-18 10:22:48 PDT
I must have forgotten to hit save when adding [leave open] to the whiteboard.
Comment 15 Jonas Sicking (:sicking) No longer reading bugmail consistently 2012-07-18 17:09:43 PDT
Created attachment 643682 [details] [diff] [review]
Make sessionStorage/localStorage code create the correct principals
Comment 16 Jonas Sicking (:sicking) No longer reading bugmail consistently 2012-07-18 17:12:22 PDT
Created attachment 643683 [details] [diff] [review]
Fix browser code and tests
Comment 17 Mounir Lamouri (:mounir) 2012-07-18 17:23:38 PDT
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.
Comment 18 Jonas Sicking (:sicking) No longer reading bugmail consistently 2012-07-19 23:37:31 PDT
Created attachment 644198 [details] [diff] [review]
Add GetDocShellCodebasePrincipal

I wish we could have called this method GetAppCodebasePrincipal, but XPCOM idl doesn't allow overloads :(
Comment 19 Jonas Sicking (:sicking) No longer reading bugmail consistently 2012-07-19 23:38:24 PDT
Created attachment 644199 [details] [diff] [review]
Make getChannelPrincipal get the correct app principal
Comment 20 Jonas Sicking (:sicking) No longer reading bugmail consistently 2012-07-19 23:39:24 PDT
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
Comment 21 Jonas Sicking (:sicking) No longer reading bugmail consistently 2012-07-20 01:05:13 PDT
Created attachment 644219 [details] [diff] [review]
Make sessionStorage code create the correct principals

Actually passes tests now
Comment 22 Jonas Sicking (:sicking) No longer reading bugmail consistently 2012-07-20 01:13:07 PDT
Created attachment 644220 [details] [diff] [review]
Rename getCodebasePrincipal to getSimpleCodebasePrincipal
Comment 23 Mounir Lamouri (:mounir) 2012-07-20 01:21:25 PDT
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.
Comment 25 Ed Morley [:emorley] 2012-07-20 05:03:45 PDT
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.
Comment 29 Adam Kowalczyk 2012-07-22 19:23:06 PDT
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.
Comment 30 Ian Nartowicz 2012-10-14 04:45:51 PDT
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.
Comment 31 Jonas Sicking (:sicking) No longer reading bugmail consistently 2012-10-14 22:30:35 PDT
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.
Comment 32 Jorge Villalobos [:jorgev] 2012-10-26 16:54:17 PDT
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?
Comment 33 Boris Zbarsky [:bz] (TPAC) 2012-10-26 18:19:25 PDT
All that changed was the name, not the signature.

I do think we should change the name back....
Comment 34 Alex Keybl [:akeybl] 2012-10-29 15:21:41 PDT
(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.

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