Open Bug 1306387 Opened 3 years ago Updated 11 months ago

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

Categories

(Core :: Networking, defect, P5)

defect

Tracking

()

People

(Reporter: ochameau, Assigned: ochameau)

References

Details

(Whiteboard: [necko-would-take])

Attachments

(1 file)

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.
Can you take a look?
Flags: needinfo?(tanvi)
or Christoph?
I imagine that's more something for Boris or Bill who were involved into bug 1161831.
I'm not too familiar with the addonId Origin Attribute, so I would try Boris or Bill.
Flags: needinfo?(tanvi)
What about codebase principals created from a URI directly, without a channel being involved at all?
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...
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.
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 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.
Attachment #8796223 - Flags: review?(bobbyholley)
> 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.
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.
Flags: needinfo?(wmccloskey)
Assignee: nobody → poirot.alex
Whiteboard: [necko-active]
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.
Attachment #8796223 - Flags: review?(bobbyholley)
(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.
Flags: needinfo?(wmccloskey)
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.
Flags: needinfo?(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.
Flags: needinfo?(bzbarsky)
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.
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?
(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).
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.
Whiteboard: [necko-active] → [necko-would-take]
Bulk change to priority: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258
Priority: -- → P5
You need to log in before you can comment on or make changes to this bug.