Jonas Sicking (:sicking) No longer reading bugmail consistently Assignee
	Description • 12 years ago

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.

	Jonas Sicking (:sicking) No longer reading bugmail consistently Assignee
	Comment 1 • 12 years ago

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.

	Jonas Sicking (:sicking) No longer reading bugmail consistently Assignee
	Comment 2 • 12 years ago

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.

	Jonas Sicking (:sicking) No longer reading bugmail consistently Assignee
	Comment 6 • 12 years ago

Somehow I missed the mobile about: redirectors. This patch includes them too.

	Mounir Lamouri (:mounir)
	Comment 7 • 12 years ago

(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 ;)

	Mounir Lamouri (:mounir)
	Comment 8 • 12 years ago

Could you also change calls from GetCodebasePrincipal() to GetSimpleCodebasePrincipal() so we make sure all consumers know what they are doing, see bug 758258.

	Blake Kaplan (:mrbkap) (inactive)
	Comment 9 • 12 years ago

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?

	Boris Zbarsky [:bzbarsky]
	Comment 10 • 12 years ago

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

	Jonas Sicking (:sicking) No longer reading bugmail consistently Assignee
	Comment 11 • 12 years ago

Comment on attachment 643102 [details] [diff] [review]
Update about: redirectors to remove getCodebasePrincipal calls

https://hg.mozilla.org/integration/mozilla-inbound/rev/096a755c48ea

	Jonas Sicking (:sicking) No longer reading bugmail consistently Assignee
	Comment 12 • 12 years ago

Comment on attachment 642878 [details] [diff] [review]
Remove unused aTarget argument

https://hg.mozilla.org/integration/mozilla-inbound/rev/ebfa4531ca57

	Ed Morley [:emorley]
	Comment 13 • 12 years ago

https://hg.mozilla.org/mozilla-central/rev/096a755c48ea
https://hg.mozilla.org/mozilla-central/rev/ebfa4531ca57

	Jonas Sicking (:sicking) No longer reading bugmail consistently Assignee
	Comment 14 • 12 years ago

I must have forgotten to hit save when adding [leave open] to the whiteboard.

	Mounir Lamouri (:mounir)
	Comment 17 • 12 years ago

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.

	Jonas Sicking (:sicking) No longer reading bugmail consistently Assignee
	Comment 18 • 12 years ago

I wish we could have called this method GetAppCodebasePrincipal, but XPCOM idl doesn't allow overloads :(

	Jonas Sicking (:sicking) No longer reading bugmail consistently Assignee
	Comment 20 • 12 years ago

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

	Jonas Sicking (:sicking) No longer reading bugmail consistently Assignee
	Comment 21 • 12 years ago

Actually passes tests now

	Mounir Lamouri (:mounir)
	Comment 23 • 12 years ago

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.

	Ed Morley [:emorley]
	Comment 24 • 12 years ago

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

	Ed Morley [:emorley]
	Comment 25 • 12 years ago

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.

	Ed Morley [:emorley]
	Comment 26 • 12 years ago

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

	Jonas Sicking (:sicking) No longer reading bugmail consistently Assignee
	Comment 27 • 12 years ago

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

	Ryan VanderMeulen [:RyanVM]
	Comment 28 • 12 years ago

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

	Adam Kowalczyk
	Comment 29 • 12 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.

	Ian Nartowicz
	Comment 30 • 12 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.

	Jonas Sicking (:sicking) No longer reading bugmail consistently Assignee
	Comment 31 • 12 years ago

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.

	Jorge Villalobos [:jorgev] (he/him)
	Comment 32 • 12 years ago

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?

	Boris Zbarsky [:bzbarsky]
	Comment 33 • 12 years ago

All that changed was the name, not the signature.

I do think we should change the name back....

	Alex Keybl [:akeybl]
	Comment 34 • 12 years ago

(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.