1281847 - [disco-pane] UiTour does't work with disco pane origins

Status: RESOLVED WONTFIX

(Toolkit :: Add-ons Manager, defect, P2)

Description

[[e-mail]]

When running the new disco pane from https://discovery.addons-dev.allizom.org if you set the pref browser.uitour.testingOrigins to https://discovery.addons-dev.allizom.org it doesn't work unless you're looking at discovery.addons-dev.allizom.org directly and not via about:addons.

To get it to work on about:addons I needed to use about:addons in browser.uitour.testingOrigins.

STR (using nightly)

 * Setup to test the disco pane on -dev with the following: https://wiki.mozilla.org/Add-ons/Projects/DiscoveryImprovements#Testing
 * Additionally set browser.uitour.testingOrigins to https://discovery.addons-dev.allizom.org
 * Install an add-on.

What happens: 

 * After the add-on is installed you don't see any uitour notification

What should happen:

 * You should see the UITour notification

If you set browser.uitour.testingOrigins to about:addons it works.

Comment 1

[Andrew Swan [:aswan]]

Hm, unlike the mozAddonManager permission checking which checks the origin for the frame from which it is being used and all its parents all the way up to the root (https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/AddonManagerWebAPI.cpp#74-130) it looks like the permission manager just does a simple check on the top level document url.

So the direct "fix" would be to add about:addons to the permissions list for uitour, but that makes me nervous, if there is any way for a user to inject a frame that they control anywhere under about:addons, they could get undesired access to uitour.

MattN, you looked at the original bug 1256598, do you have any thoughts on this?  (fuller context is that about:addons is xul, when a user is looking at "get add-ons" which we call the discovery pane or disco pane for short, that's using a <browser> that loads a page from discovery.addons.mozilla.org.  back in the original bug we added that domain to the permissions list for uitour but as explained above, it doesn't work when viewed from about:addons).

Comment 2

[Andy McKay]

While I don't think the UITour can do anything bad, we do know that addons can inject into about:addons (admittedly in an iframe inside the xul inside an iframe using options_ui). I think we should do the right thing, rather than the convenient thing.

That's implementing a check similar to the one Mossop did for the mozAddonsManager that reaches down through the frames to do the check.

I don't think that kind of check is something we should be uplifting to beta and instead we should go for: https://github.com/mozilla/addons-frontend/issues/658

Comment 3

[Andrew Swan [:aswan]]

To make the needinfo more concrete: MattN, would you accept patches that move the logic from the AddonManagerWebAPI code referenced in comment 1 into the permission manager and then use that method for deciding when to grant permission to use uitour?
I'm happy to do the work but want to make sure we have consensus about the approach before diving in.

Comment 4

[Matthew N. [:MattN]]

Could you tell me exactly where in the code we're taking the wrong path? I can think of multiple reasons for this not working but I'm not sure which one you're hitting off-hand? For example, is it ensureTrustedOrigin in content-UITour.js that is the problem or is it the handling of the <browser> in UITour.jsm?

Logging with browser.uitour.loglevel=All may be enough to figure out what is going on.

Note that we intentionally don't allow UITour to be used from subframes but that shouldn't be a problem if you're loading the page using the tour in a <browser> which is setup to be treated as a top-level frame.

Once I hear where the problem is I can suggest a solution.

Comment 5

[Andrew Swan [:aswan]]

Sorry, I think I didn't explain the problem clearly.  When using the add-ons manager, the top-level document URL remains fixed at "about:addons".  That gets mapped to a XUL page, but the chrome: URL for the XUL page is never exposed anywhere.  Under the covers, the XUL page uses an embedded browser to load a page from addons.mozilla.org (actually from discovery.a.m.o) but that URL is also never exposed.

The problem is that we would like that embedded page to be able to user uitour, but the uitour access check (https://dxr.mozilla.org/mozilla-central/source/browser/components/uitour/content-UITour.js#58-75) looks at the top-level document URL which in this case is about:addons.  Adding about:addons as a permitted origin for uitour would probably not be a good idea since things like options pages appear framed within about:addons but they are arbitrary user-generated content that shouldn't have access to uitour.

Apropos the third paragraph of your previous comment, the embedded <browser> is not a top-level frame, and I think that is intended.  The access checking for mozAddonManager handles this by checking for permission on the entire chain of frames up to the root.  If there's a way to make this work without that sort of change, that would be great but I'm not seeing it.

If you want to try this out yourself, there are some instructions here: https://wiki.mozilla.org/Add-ons/Projects/DiscoveryImprovements#Testing (for your purposes you can skip steps 4-6).

Comment 6

[Matthew N. [:MattN]]

(In reply to Andrew Swan [:aswan] from comment #5)
> Apropos the third paragraph of your previous comment, the embedded <browser>
> is not a top-level frame, and I think that is intended.

Why do you think this is intended? Does it provide some benefit? https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/Attribute/browser.type implies that window.top should === window inside <browser type="content"> but it doesn't seem to be the case here. If the content-uitour.js script was loaded directly in the inner about:addons <browser> then I think we wouldn't have this problem.

Comment 7

[Andrew Swan [:aswan]]

(In reply to Matthew N. [:MattN] (PTO Jun. 29 - Jul. 1) from comment #6)
> (In reply to Andrew Swan [:aswan] from comment #5)
> > Apropos the third paragraph of your previous comment, the embedded <browser>
> > is not a top-level frame, and I think that is intended.
> 
> Why do you think this is intended? Does it provide some benefit?
> https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/Attribute/browser.
> type implies that window.top should === window inside <browser
> type="content"> but it doesn't seem to be the case here.

I fear I may be misunderstanding the question here but the contents of extensions.xul (toolkit/mozapps/extensions/content/extensions.xul) is not part of the chrome, it is loaded as content, so it ends up inside a <browser> with type="content" that is just part of the generic browser chrome.  It (extensions.xul) contains a nested <browser> element but as the MDN page you referenced says:
"The type attribute on all frames in content documents is ignored".

As for why it is done this way, I believe it is meant to separate stable user-facing URLs (i.e., about:addons) from the details of how it is implemented (i.e., some specific page on AMO).

> If the
> content-uitour.js script was loaded directly in the inner about:addons
> <browser> then I think we wouldn't have this problem.

Sorry, I don't understand what this means?

Comment 8

[Matthew N. [:MattN]]

Attachment: Bug 1281847 - Make UITour work properly with nested <browser> not necessarily part of gBrowser

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

Comment 9

[Matthew N. [:MattN]]

Attachment: Bug 1281847 - Load the UITour content script in the AOM discover <browser>

As-is there is a layering violation since toolkit/ can't depend on browser/

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

Comment 10

[Matthew N. [:MattN]]

(In reply to Andrew Swan [:aswan] from comment #7)
> (In reply to Matthew N. [:MattN] (PTO Jun. 29 - Jul. 1) from comment #6)
> > (In reply to Andrew Swan [:aswan] from comment #5)
> > > Apropos the third paragraph of your previous comment, the embedded <browser>
> > > is not a top-level frame, and I think that is intended.
> > 
> > Why do you think this is intended? Does it provide some benefit?
> > https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/Attribute/browser.
> > type implies that window.top should === window inside <browser
> > type="content"> but it doesn't seem to be the case here.
> 
> I fear I may be misunderstanding the question here but the contents of
> extensions.xul (toolkit/mozapps/extensions/content/extensions.xul) is not
> part of the chrome, it is loaded as content, so it ends up inside a
> <browser> with type="content" that is just part of the generic browser
> chrome.

Yeah, I understand all of this. Take a look at my ideas in the patches which don't change the security checks but depend on bug 1285048.

>  It (extensions.xul) contains a nested <browser> element but as the
> MDN page you referenced says:
> "The type attribute on all frames in content documents is ignored".

I assumed the docs are referring to content-privileged documents in that case. We wouldn't want the type attribute to be ignored or it would mean the discover URL would have chrome privileges.

> As for why it is done this way, I believe it is meant to separate stable
> user-facing URLs (i.e., about:addons) from the details of how it is
> implemented (i.e., some specific page on AMO).
> 
> > If the
> > content-uitour.js script was loaded directly in the inner about:addons
> > <browser> then I think we wouldn't have this problem.
> 
> Sorry, I don't understand what this means?

See attachment 8768664 [details] for what I mean. The problem with this is that it's a layering violation since toolkit/ can't depend on browser/. Do you already have a solution for that? It seems like it's a problem with relying on UITour in general. Is it only going to be used as an enhancement but things will work fine without it?

Ping me when you're available tomorrow so we can discuss live (if the patches didn't clarify things) since I think there may be some confusion. I'm unavailable 1pm - 2pm PDT tomorrow.

Comment 11

[Andrew Swan [:aswan]]

Attachment: Bug 1281847 Part 1 switch disco browser to <iframe mozbrowser>

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

Comment 12

[Andrew Swan [:aswan]]

The patch I just attached is my first attempt at switching the disco pane <browser> over to an <iframe>.  The good news is that the changes are pretty simple and straightforward, the bad news is that it doesn't actually work yet, for reasons that I haven't yet figured out.
I'm not going to be able to look at this any further today, but here's the work-in-progress.

Comment 13

[[e-mail]]

(In reply to Andrew Swan [:aswan] from comment #12)
> The patch I just attached is my first attempt at switching the disco pane
> <browser> over to an <iframe>.  The good news is that the changes are pretty
> simple and straightforward, the bad news is that it doesn't actually work
> yet, for reasons that I haven't yet figured out.
> I'm not going to be able to look at this any further today, but here's the
> work-in-progress.

Will you hit issues with the headers sent by the disco pane page to prevent iframing? If you recall we discussed this a bit in London. Currently the x-frame-options: DENY header is ignored and we can't easily use frame-ancestors from CSP to say that only the disco pane page can frame discovery.a.m.o.

Comment 14

[Andrew Swan [:aswan]]

(In reply to Stuart Colville [:scolville] [:muffinresearch] from comment #13)
> Will you hit issues with the headers sent by the disco pane page to prevent
> iframing?

Ah, quite possibly.  Is there an easy way to disable that header for testing purposes?

> Currently the
> x-frame-options: DENY header is ignored and we can't easily use
> frame-ancestors from CSP to say that only the disco pane page can frame
> discovery.a.m.o.

I do remember discussing it, can you remind me why frame-ancestors isn't practical?  Was it related to specifying about:addons as the ancestor rather than an https://  url?

Comment 15

[Matthew N. [:MattN]]

(In reply to Andrew Swan [:aswan] from comment #14)
> (In reply to Stuart Colville [:scolville] [:muffinresearch] from comment #13)
> > Will you hit issues with the headers sent by the disco pane page to prevent
> > iframing?
> 
> Ah, quite possibly.  Is there an easy way to disable that header for testing
> purposes?

That shouldn't be a problem with <iframe mozbrowser>.

Comment 16

[Andrew Swan [:aswan]]

I finally got back to the patch and tracked this down to E10sUtils.shouldLoadURI() (https://dxr.mozilla.org/mozilla-central/source/browser/modules/E10SUtils.jsm#90) blocking loading the remote page in the chrome process.  So, I can add make the iframe remote but now the code that tries to attach a progress listener fails (since we can't just use QI to grab the docshell since it is now remote).
I suspect there must be an existing way to get progress events from a remote docshell without writing a ton of new code, I'll go digging further when I get a chance unless somebody who knows this area well wants to help out and point me in the right direction?

Comment 17

[Andrew Swan [:aswan]]

Alright this has now become obsolete.  As part of the work to expose permissions, we'll have the browser itself display prompts after an extension is installed from the disco pane so we don't need to do it from content.  See bug 1308310 in particular.