1306387 - Set addonId origin attribute from moz-extension protocol handler rather than hacking it from nsScriptSecurityManager

Status: NEW

(Core :: Networking, defect, P5)

Description

[Alexandre Poirot [:ochameau]]

Bug 1161831 introduced moz-extension scheme but complexified nsScriptSecurityManager by introducing calls to MaybeSetAddonIdFromURI from various places. It is not clear why we have anything specific to addons in there, and especially this kind of hack around origin attributes.

It looks like, the protocol handler implementing moz-extension should set the correct addonId field in the origin attribute in the first place.

Comment 1

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1306387 - Simplify addon ID origin attribute by setting it from the moz-extension protocol handler. r=?

Review commit: https://reviewboard.mozilla.org/r/82128/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/82128/

Comment 2

[Alexandre Poirot [:ochameau]]

Try seems to be happy with that:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=04cb160ff8e3

Comment 3

[Dragana Damjanovic [:dragana]]

Can you take a look?

Comment 4

[Dragana Damjanovic [:dragana]]

or Christoph?

Comment 5

[Alexandre Poirot [:ochameau]]

I imagine that's more something for Boris or Bill who were involved into bug 1161831.

Comment 6

[Tanvi Vyas[:tanvi]]

I'm not too familiar with the addonId Origin Attribute, so I would try Boris or Bill.

Comment 7

[Boris Zbarsky [:bzbarsky]]

What about codebase principals created from a URI directly, without a channel being involved at all?

Comment 8

[Alexandre Poirot [:ochameau]]

Was that also hooked?
It looks like we only hacked the addonId from nsScriptSecurityManager::GetChannelURIPrincipal, nsScriptSecurityManager::GetLoadContextCodebasePrincipal and nsScriptSecurityManager::GetDocShellCodebasePrincipal. None of this seems to be involved for CreateCodebasePrincipal?

Also, when looking at possible usages of createCodebasePrincipal, most are explicitely putting addonId in originAttributes:
http://searchfox.org/mozilla-central/search?q=createCodebasePrincipal(&case=false&regexp=false&path=extension
Except the AddonContentPolicy component:
http://searchfox.org/mozilla-central/source/toolkit/mozapps/extensions/AddonContentPolicy.cpp#431
I've not idea if that causes any issue regarding the CSP checks it does...

Comment 9

[Boris Zbarsky [:bzbarsky]]

Ah, good point.  So what calls GetLoadContextCodebasePrincipal and GetDocShellCodebasePrincipal, since those are the things whose behavior would actually change?

Might also be worth checking with bholley to see what the idea here was.

Comment 10

[Alexandre Poirot [:ochameau]]

Actually, there is also nsScriptSecurityManager::GetChannelURIPrincipal which sets the addonID attribute.
And this is the main thing being called from my testings.
This is being called by nsDocument::Reset:
  http://searchfox.org/mozilla-central/source/dom/base/nsDocument.cpp#1991
And also from nsScriptLoader::PrepareLoadedRequest:
  http://searchfox.org/mozilla-central/source/dom/base/nsScriptLoader.cpp#2655
There is other occurances in source, but I haven't seen them being called when testing simple addon.
Regarding nsScriptSecurityManager::GetLoadContextCodebasePrincipal and nsScriptSecurityManager::GetDocShellCodebasePrincipal, I haven't seen any call.
But GetDocShellCodebasePrincipal is only called from nsDocShell for edge cases about appCache.
GetLoadContextCodebasePrincipal is more likely to be called as it is called from nsDocument::ResetToURI.

Having said that, GetLoadContextCodebasePrincipal and GetChannelURIPrincipal should work the same with this patch given that it uses channel or loadcontext which I imagine comes from a channel.

Comment 11

[Alexandre Poirot [:ochameau]]

Comment on attachment 8796223 [details]
Bug 1306387 - Simplify addon ID origin attribute by setting it from the moz-extension protocol handler. r=?

Bobby, you told us in bug 1281440 to avoid having special cases in the ScriptSecurityManager. Here is a patch to avoid one regarding moz-extension.

Comment 12

[Boris Zbarsky [:bzbarsky]]

> Actually, there is also nsScriptSecurityManager::GetChannelURIPrincipal

Yes, but you're fixing that one by setting stuff on the channel, so I'm not worrying about it.  ;)

> But GetDocShellCodebasePrincipal is only called from nsDocShell for edge cases about appCache.

It's called from browser/components/sessionstore/SessionStorage.jsm too.  But maybe for the uses there we don't care about the addonid?  I expect we do, though.

> given that it uses channel or loadcontext which I imagine comes from a channel.

The loadcontext doesn't come from a channel in nsDocument::ResetToURI.  More to the point, the relevant origin attribute is never stored on the loadcontext with your patch, so the GetLoadContextCodebasePrincipal caller is going to no longer end up with the right addonid.

Looks like this only affects calls to nsDocument::Reset that have no channel, which in practice means some XSLT code that shouldn't really get hit anyway.  So seems to me like this part is OK.

Comment 13

[Bobby Holley (:bholley)]

Bill, ISTR concluding that we were going to get rid of the addonID OA. Is that correct? If so it seems like it would remove the need for this patch.

Comment 14

[Bobby Holley (:bholley)]

Comment on attachment 8796223 [details]
Bug 1306387 - Simplify addon ID origin attribute by setting it from the moz-extension protocol handler. r=?

Canceling review for now until we figure out if we're getting rid of the addonID OA.

Comment 15

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #13)
> Bill, ISTR concluding that we were going to get rid of the addonID OA. Is
> that correct? If so it seems like it would remove the need for this patch.

That's true, but that's a somewhat unknown amount of work and no one has volunteered to do it (I'm pretty busy right now). If this patch improves things, maybe we should take it.

Comment 16

[Bobby Holley (:bholley)]

Ok. Boris, do you have any opinions on whether we want to take this patch? I don't really have cycles to think about it much right now.

Comment 17

[Boris Zbarsky [:bzbarsky]]

I honestly don't either....

It may be somewhat of an improvement in the overall understandability of the code, and I _think_ it doesn't change observable behavior.  But I'd really want someone other than me to carefully double-check that.

Comment 18

[Bobby Holley (:bholley)]

Ok. Given that the overall plan is for addonId to go away and we just need somebody to do it, I think we shouldn't take this patch for now.

Comment 19

[Alexandre Poirot [:ochameau]]

This is a bit depressing regarding contribution. Couldn't gabor/billm review such thing someday?
It looks like we are waiting for a refactoring that may never come (is there a bug opened for that?) whereas this one is here with a green try. Or may be it is fine having all these tryHackingTheAddonsFromScriptSecurityManager calls?

Comment 20

[Bobby Holley (:bholley)]

(In reply to Alexandre Poirot [:ochameau] from comment #19)
> This is a bit depressing regarding contribution.

Yeah, sorry about that. :-(

> Couldn't gabor/billm review
> such thing someday?

Maybe, but the issue is that we need somebody who understands this machinery who has cycles to think through the implications, figure out how it interacts with the broader plan, and be on the hook for regressions. I don't have time to do so right now, and I suspect bz and bill don't either. I don't think Gabor is super familiar with all of this stuff. The only other candidate would be smaug, though he has objected to the current setup (see bug 1278804).

Comment 21

[Alexandre Poirot [:ochameau]]

Thanks the the heads up.

I'm wondering if my patch replies to the propagation uncertainty.
Putting addonId via the protocol makes it clearly related to the URL and should then apply default propagation rules everywhere else?
But yes, I can't say exactly how the edge cases are going to behave.
May be we could add some mochitests to assert the final privileges around moz-extensions?
If there is some existing tests, I could augment them, but I don't know exactly what we expect and all the combinations we should assert.

Comment 22

[Firefox Bug Husbandry Bot]

Bulk change to priority: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258

Comment 23

[Stacey Greenly]

Attachment: 2DVF.pdf