Closed Bug 1371543 Opened 6 years ago Closed 6 years ago

Support Screenshots page action with UITour

Categories

(Firefox :: Tours, enhancement, P1)

Product:
Firefox
Component:
Tours
Type:
enhancement

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 57
Tracking Flags:
Tracking Status
firefox57 --- verified

People

(Reporter: gasolin, Assigned: Fischer)

References

Details

(Whiteboard: [photon-onboarding])

Attachments

(1 file)

Bug 1371543 - Support Screenshots page action with UITour,
59 bytes, text/x-review-board-request
dveditz
: review+
Gijs
: review+
Details 
Currently screenshots is listed as a icon in the toolbar. The photon preview spec shows screenshots will be moved into the page action section, and we should able to select `screenshot` page-action with UITour.
Ian, we are planning to do a screenshots tour in the newtab page (the fox icon), and the tour should able to focus on the screenshots icon wherever it is.

Do you know if screenshots will be moved to page-action in 57? Or we'll just hightlight the screenshots on toolbar.
Flags: qe-verify+
Flags: needinfo?(ianb)
Priority: -- → P2
Target Milestone: --- → Firefox 57
Flags: needinfo?(ianb) → needinfo?(jgruen)
Flags: needinfo?(jgruen)
Added two dependency bugs here. Tracking integration of SS into page action and library menus for 57.
Depends on: 1366026, 1366041
Whiteboard: [photon-onboarding]
QA Contact: jwilliams
See Also: → 1374563
Assignee: nobody → fliu
Status: NEW → ASSIGNED
Depends on: 1382579
No longer blocks: 1371535
Blocks: 1371538
No longer depends on: 1371538
Priority: P2 → P1
Summary: support select `screenshots` page-action with UITour → Support Screenshot page action with UITour
This bug purpose is to make UITour support the Screenshot button inside the page action menu not about the library panel so remove the dependency.
No longer depends on: 1366026
This is an assigned P1 bug without activity in two weeks. 

If you intend to continue working on this bug for the current release/iteration/sprint, remove the 'stale-bug' keyword.

Otherwise we'll reset the priority of the bug back to '--' on Monday, August 28th.
Keywords: stale-bug
Keywords: stale-bug
Blocks: 1393668
We want to highlight the Screenshots page action button when arriving https://screenshots.firefox.com website (see 1393668). We should add https://screenshots.firefox.com into the UITour permission list as well.
Summary: Support Screenshot page action with UITour → Support Screenshots page action with UITour
(In reply to Fischer [:Fischer] from comment #8)
> Comment on attachment 8902578 [details]
> Bug 1371543 - Support Screenshots page action with UITour,
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/174192/diff/1-2/
TRY: https://treeherder.mozilla.org/#/jobs?repo=try&revision=bccd478575fe38ad1c43755ef34b0d6019fecdbe
Comment on attachment 8902578 [details]
Bug 1371543 - Support Screenshots page action with UITour,

https://reviewboard.mozilla.org/r/174192/#review180454

::: browser/app/permissions:11
(Diff revision 2)
>  # * permission is an integer between 1 and 15
>  # See nsPermissionManager.cpp for more...
>  
>  # UITour
>  origin	uitour	1	https://www.mozilla.org
> +origin	uitour	1	https://screenshots.firefox.com

This allows screenshots to do a lot more than just highlighting this button... why do we need to do this? Can we avoid extending this permission just for the tour? Why can't the tour live on one of the mozilla.org domains that already have permission? Do we have a sec review for the risk associated with adding screenshots here?

::: browser/components/uitour/test/browser_UITour_availableTargets.js:178
(Diff revision 2)
> +    let listener = {
> +      onWidgetAfterCreation(widgetid) {
> +        if (widgetid == "screenshots_mozilla_org-browser-action") {
> +          CustomizableUI.removeListener(listener);
> +          resolve();
> +        }
> +      }
> +    };
> +    CustomizableUI.addListener(listener);

Please don't depend on the browser action existing. Instead, just use waitForCondition() to check for the item in the page action menu directly, and just set the prefs with pushPrefEnv(). That will significantly simplify this helper function.
Attachment #8902578 - Flags: review?(gijskruitbosch+bugs)
(In reply to :Gijs (queue backed up, slow) from comment #10)
> Comment on attachment 8902578 [details]
> Bug 1371543 - Support Screenshots page action with UITour,
> 
> https://reviewboard.mozilla.org/r/174192/#review180454
> 
> ::: browser/app/permissions:11
> (Diff revision 2)
> >  # * permission is an integer between 1 and 15
> >  # See nsPermissionManager.cpp for more...
> >  
> >  # UITour
> >  origin	uitour	1	https://www.mozilla.org
> > +origin	uitour	1	https://screenshots.firefox.com
> 
> This allows screenshots to do a lot more than just highlighting this
> button... why do we need to do this? Can we avoid extending this permission
> just for the tour? Why can't the tour live on one of the mozilla.org domains
> that already have permission? Do we have a sec review for the risk
> associated with adding screenshots here?

See also e.g. https://bugzilla.mozilla.org/show_bug.cgi?id=1256598#c3:

(In reply to Matthew N. [:MattN] (huge backlog; PM if requests are blocking you) from comment #3)
> I'm assuming that untrusted content (e.g. add-on developer HTML) on AMO
> won't have any way to run JS on those domains.

So like, if I were a malicious person, could I craft an API request to the screenshots server that uploaded an svg file with script and then create a sharable link to that file that would run said script, and then escalate to accessing stuff in Firefox through the UITour API?

AMO mitigates this by having quite strong restrictions on what the content pages can have as user-submitted content (add-on descriptions etc.) and hosting the actual add-on content elsewhere (different domain). I have no idea what measures screenshots have in place, and I don't see this being discussed here or in bug 1393668.
(In reply to :Gijs (queue backed up, slow) from comment #11)
> (In reply to :Gijs (queue backed up, slow) from comment #10)
> > Comment on attachment 8902578 [details]
> > Bug 1371543 - Support Screenshots page action with UITour,
> > 
> > https://reviewboard.mozilla.org/r/174192/#review180454
> > 
> > ::: browser/app/permissions:11
> > (Diff revision 2)
> > >  # * permission is an integer between 1 and 15
> > >  # See nsPermissionManager.cpp for more...
> > >  
> > >  # UITour
> > >  origin	uitour	1	https://www.mozilla.org
> > > +origin	uitour	1	https://screenshots.firefox.com
> > 
> > This allows screenshots to do a lot more than just highlighting this
> > button... why do we need to do this? Can we avoid extending this permission
> > just for the tour? Why can't the tour live on one of the mozilla.org domains
> > that already have permission? Do we have a sec review for the risk
> > associated with adding screenshots here?
> 
> See also e.g. https://bugzilla.mozilla.org/show_bug.cgi?id=1256598#c3:
> 
> (In reply to Matthew N. [:MattN] (huge backlog; PM if requests are blocking
> you) from comment #3)
> > I'm assuming that untrusted content (e.g. add-on developer HTML) on AMO
> > won't have any way to run JS on those domains.
> 
> So like, if I were a malicious person, could I craft an API request to the
> screenshots server that uploaded an svg file with script and then create a
> sharable link to that file that would run said script, and then escalate to
> accessing stuff in Firefox through the UITour API?
> 
> AMO mitigates this by having quite strong restrictions on what the content
> pages can have as user-submitted content (add-on descriptions etc.) and
> hosting the actual add-on content elsewhere (different domain). I have no
> idea what measures screenshots have in place, and I don't see this being
> discussed here or in bug 1393668.
OK, will bring the screenshots team into this security concern. AFAIK, user only could view, share, delete and download screenshots on the screenshots page.
(In reply to :Gijs (queue backed up, slow) from comment #11)
> (In reply to :Gijs (queue backed up, slow) from comment #10)
> > Comment on attachment 8902578 [details]
> > Bug 1371543 - Support Screenshots page action with UITour,
> > 
> > https://reviewboard.mozilla.org/r/174192/#review180454
> > 
> > ::: browser/app/permissions:11
> > (Diff revision 2)
> > >  # * permission is an integer between 1 and 15
> > >  # See nsPermissionManager.cpp for more...
> > >  
> > >  # UITour
> > >  origin	uitour	1	https://www.mozilla.org
> > > +origin	uitour	1	https://screenshots.firefox.com
> > 
> > This allows screenshots to do a lot more than just highlighting this
> > button... why do we need to do this? Can we avoid extending this permission
> > just for the tour? Why can't the tour live on one of the mozilla.org domains
> > that already have permission? Do we have a sec review for the risk
> > associated with adding screenshots here?
> 
> See also e.g. https://bugzilla.mozilla.org/show_bug.cgi?id=1256598#c3:
> 
> (In reply to Matthew N. [:MattN] (huge backlog; PM if requests are blocking
> you) from comment #3)
> > I'm assuming that untrusted content (e.g. add-on developer HTML) on AMO
> > won't have any way to run JS on those domains.
> 
> So like, if I were a malicious person, could I craft an API request to the
> screenshots server that uploaded an svg file with script and then create a
> sharable link to that file that would run said script, and then escalate to
> accessing stuff in Firefox through the UITour API?
> 
> AMO mitigates this by having quite strong restrictions on what the content
> pages can have as user-submitted content (add-on descriptions etc.) and
> hosting the actual add-on content elsewhere (different domain). I have no
> idea what measures screenshots have in place, and I don't see this being
> discussed here or in bug 1393668.
Hi Ian and John,

As the above concern about highlighting the Screenshots page action, would like to discuss some security concerns with you:
- is there some entry point where external party could upload image file to https://screenshots.firefox.com? If yes, how do we manage that?
- if we didn't do the screenshots website, we may do other screenshots introduction page hosted on https://www.mozilla.org or https://support.mozilla.org, which have UITour permission already. Do we have other screenshots introduction page?

Thanks
Flags: needinfo?(jgruen)
Flags: needinfo?(ianb)
> Hi Ian and John,
> 
> As the above concern about highlighting the Screenshots page action, would
> like to discuss some security concerns with you:
> - is there some entry point where external party could upload image file to
> https://screenshots.firefox.com? If yes, how do we manage that?

I don't think so but we don't have anything like the kind of strict CSP as exists on AMO. Part of this is on puropose b/c we want people to be able take shot on the screenshots homepage so we can't block content scripts.

> - if we didn't do the screenshots website, we may do other screenshots
> introduction page hosted on https://www.mozilla.org or
> https://support.mozilla.org, which have UITour permission already. Do we
> have other screenshots introduction page?

I'm strongly against the idea of standing up another webpage just for the uiTour. It's extremely late in the game, and this would require considerable effort on the part of my engineering team. If there's a viable security threat to showing the UI tour on screenshots, we should just send users to /#start and walk them through our tour directly.
Flags: needinfo?(jgruen)
(In reply to [:jgruen] from comment #14)
> > Hi Ian and John,
> > 
> > As the above concern about highlighting the Screenshots page action, would
> > like to discuss some security concerns with you:
> > - is there some entry point where external party could upload image file to
> > https://screenshots.firefox.com? If yes, how do we manage that?
> 
> I don't think so but we don't have anything like the kind of strict CSP as
> exists on AMO. Part of this is on puropose b/c we want people to be able
> take shot on the screenshots homepage so we can't block content scripts.

You could block inline scripts, inline event handlers and eval, right? Maybe you could even limit the CSP to allow your own scripts plus ones from extension: schemes?

> > - if we didn't do the screenshots website, we may do other screenshots
> > introduction page hosted on https://www.mozilla.org or
> > https://support.mozilla.org, which have UITour permission already. Do we
> > have other screenshots introduction page?
> 
> I'm strongly against the idea of standing up another webpage just for the
> uiTour. It's extremely late in the game, and this would require considerable
> effort on the part of my engineering team. 

I'm not suggesting that - a separate page on the same domain wouldn't help at all for the threat model I'm concerned about - I'm suggesting that previously we've used pages on mozilla.org to run the entirety of a tour for a release, and I assumed we were doing the same thing for 57, so presumably that page can just make the relevant calls to highlight the screenshots action. Is there something I'm missing here? Where is the rest of the tour running, given that presumably, although screenshots are important, it's not the *only* bit of 57 we're highlighting?

> If there's a viable security
> threat to showing the UI tour on screenshots, we should just send users to
> /#start and walk them through our tour directly.

That's presumably on the same screenshots.mozilla.com domain? I don't really understand this suggestion - are you suggesting not using UITour from screenshots, then? Or something else?

To be clear, the threat model I'm concerned about here is:

1) malicious actor uploads something that runs on screenshots.firefox.com
2) actor shares link either directly to that page, or to a screenshots page that embeds that content somehow.
3) that malicious contents can now do other stuff to Firefox that it wouldn't otherwise be able to do, because screenshots.firefox.com has UITour privileges.
Flags: needinfo?(jgruen)
(In reply to :Gijs (queue backed up, slow) from comment #15)
> (In reply to [:jgruen] from comment #14)
> > > Hi Ian and John,
> > > 
> > > As the above concern about highlighting the Screenshots page action, would
> > > like to discuss some security concerns with you:
> > > - is there some entry point where external party could upload image file to
> > > https://screenshots.firefox.com? If yes, how do we manage that?
> > 
> > I don't think so but we don't have anything like the kind of strict CSP as
> > exists on AMO. Part of this is on puropose b/c we want people to be able
> > take shot on the screenshots homepage so we can't block content scripts.
> 
> You could block inline scripts, inline event handlers and eval, right? Maybe
> you could even limit the CSP to allow your own scripts plus ones from
> extension: schemes?

We may already be doing this. I'm not super familar with our CSP set up. That would be a question for Ian or Relud.
> 
> > > - if we didn't do the screenshots website, we may do other screenshots
> > > introduction page hosted on https://www.mozilla.org or
> > > https://support.mozilla.org, which have UITour permission already. Do we
> > > have other screenshots introduction page?
> > 
> > I'm strongly against the idea of standing up another webpage just for the
> > uiTour. It's extremely late in the game, and this would require considerable
> > effort on the part of my engineering team. 
> 
> I'm not suggesting that - a separate page on the same domain wouldn't help
> at all for the threat model I'm concerned about - I'm suggesting that
> previously we've used pages on mozilla.org to run the entirety of a tour for
> a release, and I assumed we were doing the same thing for 57, so presumably
> that page can just make the relevant calls to highlight the screenshots
> action. Is there something I'm missing here? Where is the rest of the tour
> running, given that presumably, although screenshots are important, it's not
> the *only* bit of 57 we're highlighting?

Yeah, it's important insofar as screenshots is the only thing that highlights the new page action menu. I will admit i have not been following FF onboarding work very closely, but my understanding is that the original plan was to trigger the page action on about:newtab but, of course, there is no page action on about:newtab. That's why we're in this position.


> 
> > If there's a viable security
> > threat to showing the UI tour on screenshots, we should just send users to
> > /#start and walk them through our tour directly.
> 
> That's presumably on the same screenshots.mozilla.com domain? I don't really
> understand this suggestion - are you suggesting not using UITour from
> screenshots, then? Or something else?
> 
> To be clear, the threat model I'm concerned about here is:
> 
> 1) malicious actor uploads something that runs on screenshots.firefox.com
> 2) actor shares link either directly to that page, or to a screenshots page
> that embeds that content somehow.
> 3) that malicious contents can now do other stuff to Firefox that it
> wouldn't otherwise be able to do, because screenshots.firefox.com has UITour
> privileges.

I'm not sure that's an open attack vector atm, since i don't think its possible to upload to our servers except via the shot add-on, but again will defer to Ian.
Flags: needinfo?(jgruen)
User-submitted content on Firefox Screenshots is hosted on https://screenshots.firefoxusercontent.com

Our CSP rules look like:

default-src 'self';
img-src 'self' ${FXA_SERVER} www.google-analytics.com ${CONTENT_NAME} data:;
script-src 'self' www.google-analytics.com 'nonce-${uuid}';
style-src 'self' 'unsafe-inline' https://code.cdn.mozilla.net;
connect-src 'self' www.google-analytics.com ${dsn};
font-src https://code.cdn.mozilla.net;
frame-ancestors 'none';
object-src 'none';

${FXA_SERVER} is the FxA profile server that holds avatar images. ${CONTENT_NAME} is screenshots.firefoxusercontent.com. ${dsn} is our Sentry server.

The screenshots site has had both internal and external security reviews.
Flags: needinfo?(ianb)
Hi Ian,
Thanks for this info. Looks like we have a pretty good CSP policy.
(In reply to Ian Bicking (:ianbicking) from comment #17)
> User-submitted content on Firefox Screenshots is hosted on https://screenshots.firefoxusercontent.com
The screenshots taken through the Screenshots page action would be uploaded to screenshots.firefoxusercontent.com.
This kind of user-submitted content should be less risky since the content is generated by us.
Is there other types of content which could be uploaded to  screenshots.firefoxusercontent.com?

> Our CSP rules look like:
> 
> default-src 'self';
> img-src 'self' ${FXA_SERVER} www.google-analytics.com ${CONTENT_NAME} data:;
> script-src 'self' www.google-analytics.com 'nonce-${uuid}';
With `script-src` basically we could avoid the inline script attack.
Could we know what is ${uuid} for and when is its usage?

> style-src 'self' 'unsafe-inline' https://code.cdn.mozilla.net;
> connect-src 'self' www.google-analytics.com ${dsn};
> font-src https://code.cdn.mozilla.net;
> frame-ancestors 'none';
> object-src 'none';
> 
> ${FXA_SERVER} is the FxA profile server that holds avatar images.
> ${CONTENT_NAME} is screenshots.firefoxusercontent.com. ${dsn} is our Sentry
> server.
> 
> The screenshots site has had both internal and external security reviews.
Flags: needinfo?(ianb)
Based on the updated comments I would r+ a patch that added screenshots.firefox.com to the whitelist, assuming it fixed the other notes I made.

Matt, have we thought about having more granular permissions here? It would make patches like this less scary...
Flags: needinfo?(MattN+bmo)
(In reply to :Gijs (queue backed up, slow) from comment #19)
> Based on the updated comments I would r+ a patch that added
> screenshots.firefox.com to the whitelist, assuming it fixed the other notes
> I made.
> 
> Matt, have we thought about having more granular permissions here? It would
> make patches like this less scary...
This is what I'm thinking as well. We could divide the UITour apis into different levels. Just grant websites the needed permission level.
Comment on attachment 8902578 [details]
Bug 1371543 - Support Screenshots page action with UITour,

https://reviewboard.mozilla.org/r/174192/#review180454

> Please don't depend on the browser action existing. Instead, just use waitForCondition() to check for the item in the page action menu directly, and just set the prefs with pushPrefEnv(). That will significantly simplify this helper function.

Thanks for pointing this out. I accidentally mixed the browser action screenshots with the page action screenshots. However, duing my test, some test issue happened. We may need to listen to the onWidgetDestroyed event. Please see my code comments, thanks.
Comment on attachment 8902578 [details]
Bug 1371543 - Support Screenshots page action with UITour,

https://reviewboard.mozilla.org/r/174192/#review181238

::: browser/components/uitour/test/browser_UITour_availableTargets.js:43
(Diff revision 3)
>  }
>  
>  add_task(setup_UITourTest);
>  
>  add_UITour_task(async function test_availableTargets() {
> +  await ensureScreenshotsEnabled();

In fact, to ensure once at the 1st test is enough. I was thinking add
```
add_UITour_task(async function() {
  await ensureScreenshotsEnabled();
})
```
in the top. Then though maybe no need. Please let me know your thought, thanks

::: browser/components/uitour/test/browser_UITour_availableTargets.js:175
(Diff revision 3)
> +    // Although the screenshots extension would destroy the browser action screenshots widget
> +    // after it is added. However, `availableTargetsCache` would still be cleared so
> +    // we should proceed after the browser action screenshots widget is detroyed.
> +    // Otherwise, we would failed at the checking the `availableTargetsCache`.
> +    CustomizableUI.addListener({
> +      onWidgetDestroyed(wid) {

Need to listen to the onWidgetDestroyed event. When enabling the screenshots, both the page action screenshots and the browser action screenshots would be added. The screenshots extension would listen to the onWidgetAfterCreation event then destroy the browser action screenshots widget[1]. As a result, we only see the page action screenshots button.

However, in UITour, it would do `clearAvailableTargetsCache` on widget added[2]. It we didn’t proceed after the onWidgetDestroyed event, we would failed at [3] because the `availableTargetsCache` was cleared when the browser action screenshots widget was added.

[1] http://searchfox.org/mozilla-central/rev/f2a1911ad310bf8651f342d719e4f4ca0a7b9bfb/browser/extensions/screenshots/bootstrap.js#273
[2] http://searchfox.org/mozilla-central/rev/f2a1911ad310bf8651f342d719e4f4ca0a7b9bfb/browser/components/uitour/UITour.jsm#280
[3] http://searchfox.org/mozilla-central/rev/f2a1911ad310bf8651f342d719e4f4ca0a7b9bfb/browser/components/uitour/test/browser_UITour_availableTargets.js#45
Comment on attachment 8902578 [details]
Bug 1371543 - Support Screenshots page action with UITour,

https://reviewboard.mozilla.org/r/174192/#review181324

::: browser/components/uitour/test/browser_UITour_availableTargets.js:175
(Diff revision 3)
> +    // Although the screenshots extension would destroy the browser action screenshots widget
> +    // after it is added. However, `availableTargetsCache` would still be cleared so
> +    // we should proceed after the browser action screenshots widget is detroyed.
> +    // Otherwise, we would failed at the checking the `availableTargetsCache`.
> +    CustomizableUI.addListener({
> +      onWidgetDestroyed(wid) {

From this comment, I still don't understand why you can't just use `waitForCondition` and check that the page action element exists and that the browser action doesn't exist (`CustomizableUI.getWidget(...)` returns null).

Even if we kept the current method, it seems like the early return introduces a race condition where we might not wait for onWidgetDestroyed, so that would still not seem right.
Attachment #8902578 - Flags: review?(gijskruitbosch+bugs)
(In reply to Fischer [:Fischer] from comment #18)
> (In reply to Ian Bicking (:ianbicking) from comment #17)
> > User-submitted content on Firefox Screenshots is hosted on https://screenshots.firefoxusercontent.com
> The screenshots taken through the Screenshots page action would be uploaded
> to screenshots.firefoxusercontent.com.
> This kind of user-submitted content should be less risky since the content
> is generated by us.
> Is there other types of content which could be uploaded to 
> screenshots.firefoxusercontent.com?

No, we only allow image/png, and check the first few bytes to confirm it is a png (not perfect, but some additional measure). We'll allow jpegs soon, but generally keep a content-type whitelist. We have X-Content-Type-Options: nosniff as another protection.

> > Our CSP rules look like:
> > 
> > default-src 'self';
> > img-src 'self' ${FXA_SERVER} www.google-analytics.com ${CONTENT_NAME} data:;
> > script-src 'self' www.google-analytics.com 'nonce-${uuid}';
> With `script-src` basically we could avoid the inline script attack.
> Could we know what is ${uuid} for and when is its usage?

It is created per-request, and is used on any inline scripts. We dynamically create some JavaScript at the bottom of the page and use this to verify those script tags. It occurs to me we could remove this sometime, filed this as https://github.com/mozilla-services/screenshots/issues/3465
Flags: needinfo?(ianb)
(In reply to :Gijs (queue backed up, slow) from comment #19)
> Based on the updated comments I would r+ a patch that added
> screenshots.firefox.com to the whitelist, assuming it fixed the other notes
> I made.

I personally would defer to dveditz on this.

> Matt, have we thought about having more granular permissions here? It would
> make patches like this less scary...

No, but I can think of two ways to make it more granular:
* by capabilities granted – this seems like a decent scope of work for unclear benefit since UITour isn't supposed to be super powerful though the data surface area affecting privacy has been increasing.
* by the URL path (though generally trust decisions should be made at origin boundaries…) – this would be complicated since we use permission manager and it doesn't know about paths IIUC.

Neither of these have really been considered seriously before.

An alternative solution to this bug would be to not add the new domain to the whitelist but have the extension register a WebChannel for https://screenshots.firefox.com and then open the panel via a privileged call from the extension to UITour.jsm (assuming it's not a pure webextension). The disadvantage is that you may not get UITour teardown (e.g. on tab switch) for free unless the init method is also called for the <browser>. It's an approach to consider if we don't want to grant approval to all of UITour.
Flags: needinfo?(MattN+bmo)
(In reply to Matthew N. [:MattN] (huge backlog; PM if requests are blocking you) from comment #26)
> (In reply to :Gijs (queue backed up, slow) from comment #19)
> > Based on the updated comments I would r+ a patch that added
> > screenshots.firefox.com to the whitelist, assuming it fixed the other notes
> > I made.
> 
> I personally would defer to dveditz on this.

This is probably a good idea. Fischer, please request an additional review from Dan for the next version of the patch.
(In reply to :Gijs (queue backed up, slow) from comment #27)
> (In reply to Matthew N. [:MattN] (huge backlog; PM if requests are blocking
> you) from comment #26)
> > (In reply to :Gijs (queue backed up, slow) from comment #19)
> > > Based on the updated comments I would r+ a patch that added
> > > screenshots.firefox.com to the whitelist, assuming it fixed the other notes
> > > I made.
> > 
> > I personally would defer to dveditz on this.
> 
> This is probably a good idea. Fischer, please request an additional review from Dan for the next version of the patch.
OK, will include Dan

(In reply to :Gijs (queue backed up, slow) from comment #24)
> > +    CustomizableUI.addListener({
> > +      onWidgetDestroyed(wid) {
> 
> From this comment, I still don't understand why you can't just use
> `waitForCondition` and check that the page action element exists and that
> the browser action doesn't exist (`CustomizableUI.getWidget(...)` returns
> null).
> 
> Even if we kept the current method, it seems like the early return
> introduces a race condition where we might not wait for onWidgetDestroyed,
> so that would still not seem right.
`CustomizableUI.getWidget` would help this out. should have thought of it in the 1st place, thanks.
Comment on attachment 8902578 [details]
Bug 1371543 - Support Screenshots page action with UITour,

https://reviewboard.mozilla.org/r/174192/#review181728

::: browser/app/permissions:11
(Diff revision 4)
>  # * permission is an integer between 1 and 15
>  # See nsPermissionManager.cpp for more...
>  
>  # UITour
>  origin	uitour	1	https://www.mozilla.org
> +origin	uitour	1	https://screenshots.firefox.com

Per the bug discussions[1][2][3], the security  protections about screenshots.firefox.com as below:
- user only could view, share, delete and download screenshots on the screenshots page, not including upload.
- the screenshots upload entry point is at https://screenshots.firefoxusercontent.com and these screenshots are generated by the Screenshots add-on
- screenshots.firefoxusercontent.com has a content-type whitelist and `X-Content-Type-Options: nosniff` header for protections
- The CSP policy of screenshots.firefox.com[2], basically script-src
 and img-src are under control.

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1371543#c14
[2] https://bugzilla.mozilla.org/show_bug.cgi?id=1371543#c17
[3] https://bugzilla.mozilla.org/show_bug.cgi?id=1371543#c25
(In reply to Fischer [:Fischer] from comment #29)
> Comment on attachment 8902578 [details]
> Bug 1371543 - Support Screenshots page action with UITour,
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/174192/diff/3-4/
Hi Dan,
We are adding https://screenshots.firefox.com into UITour permission list so that the screenshots website could highlight the Screenshots page action to users. That would be great that you could have a look at the security aspect, thank you.
Comment on attachment 8902578 [details]
Bug 1371543 - Support Screenshots page action with UITour,

https://reviewboard.mozilla.org/r/174192/#review181814

r=me but please wait for dveditz's OK before landing.
Attachment #8902578 - Flags: review?(gijskruitbosch+bugs) → review+
(In reply to Fischer [:Fischer] from comment #30)
> - the screenshots upload entry point is at https://screenshots.firefoxusercontent.com
> and these screenshots are generated by the Screenshots add-on

They're supposed to be, anyway. In practice if it were advantageous to do so fake "screenshots" would be hand-crafted malicious payloads and uploaded using some other tool.

The other protections are what count: basic "is this upload an image" checking, content-type whitelist, nosniff, separate domain for user content, CSP that doesn't load anything but images from the usercontent site. Looks good.

All of uitour has the problem that any traffic-inspection proxy (whether a corporate security device, local antivirus, or hidden adware like Superfish that was found preinstalled on some Lenovo laptops) gets to have all the uitour powers if it wants. This patch doesn't make anything worse; the new target is limited in power.
Comment on attachment 8902578 [details]
Bug 1371543 - Support Screenshots page action with UITour,

https://reviewboard.mozilla.org/r/174192/#review182086

I'm not overjoyed we're expanding the uitour list, but the incremental ability here doesn't make the existing sites more dangerous and the site controls seem reasonable.
Attachment #8902578 - Flags: review?(dveditz) → review+
Keywords: checkin-needed
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s fa8ccdf53140 -d 3c5539322b57: rebasing 418587:fa8ccdf53140 "Bug 1371543 - Support Screenshots page action with UITour, r=dveditz,Gijs" (tip)
merging browser/components/uitour/test/browser_UITour_availableTargets.js
warning: conflicts while merging browser/components/uitour/test/browser_UITour_availableTargets.js! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Rebased
Keywords: checkin-needed
Pushed by mozilla@noorenberghe.ca:
https://hg.mozilla.org/integration/autoland/rev/34db5bec9cad
Support Screenshots page action with UITour, r=dveditz,Gijs
Keywords: checkin-needed
Backed out for failing browser-chrome's browser/components/uitour/test/browser_UITour_availableTargets.js:

https://hg.mozilla.org/integration/autoland/rev/cd88f7fba8bdc30c1b1d885e6065a81a2960c3cd

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=34db5bec9cad5bd0b83235b3d1a9ea1ebce8692a&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=129492746&repo=autoland

02:27:15     INFO - TEST-PASS | browser/components/uitour/test/browser_UITour_availableTargets.js | data.targets should be an array - 
02:27:15     INFO - Buffered messages finished
02:27:15     INFO - TEST-UNEXPECTED-FAIL | browser/components/uitour/test/browser_UITour_availableTargets.js | Targets should be as expected - Got accountStatus,addons,appMenu,backForward,customize,devtools,help,home,library,pageAction-bookmark,pageAction-copyURL,pageAction-emailLink,pageAction-sendToDevice,pageActionButton,pocket,privateWindow,readerMode-urlBar,screenshots,trackingProtection,urlbar, expected accountStatus,addons,appMenu,backForward,customize,devtools,help,home,library,pageAction-bookmark,pageAction-copyURL,pageAction-emailLink,pageAction-sendToDevice,pageActionButton,pocket,privateWindow,readerMode-urlBar,screenshots,search,searchIcon,trackingProtection,urlbar
Flags: needinfo?(fliu)
(In reply to Sebastian Hengst [:aryx][:archaeopteryx] (needinfo on intermittent or backout) from comment #39)
> Backed out for failing browser-chrome's
> browser/components/uitour/test/browser_UITour_availableTargets.js:
> 
> https://hg.mozilla.org/integration/autoland/rev/
> cd88f7fba8bdc30c1b1d885e6065a81a2960c3cd
> 
> Push with failures:
> https://treeherder.mozilla.org/#/
> jobs?repo=autoland&revision=34db5bec9cad5bd0b83235b3d1a9ea1ebce8692a&filter-
> resultStatus=testfailed&filter-resultStatus=busted&filter-
> resultStatus=exception&filter-resultStatus=retry&filter-
> resultStatus=usercancel&filter-resultStatus=runnable
> Failure log:
> https://treeherder.mozilla.org/logviewer.html#?job_id=129492746&repo=autoland
> 
> 02:27:15     INFO - TEST-PASS |
> browser/components/uitour/test/browser_UITour_availableTargets.js |
> data.targets should be an array - 
> 02:27:15     INFO - Buffered messages finished
> 02:27:15     INFO - TEST-UNEXPECTED-FAIL |
> browser/components/uitour/test/browser_UITour_availableTargets.js | Targets
> should be as expected - Got
> accountStatus,addons,appMenu,backForward,customize,devtools,help,home,
> library,pageAction-bookmark,pageAction-copyURL,pageAction-emailLink,
> pageAction-sendToDevice,pageActionButton,pocket,privateWindow,readerMode-
> urlBar,screenshots,trackingProtection,urlbar, expected
> accountStatus,addons,appMenu,backForward,customize,devtools,help,home,
> library,pageAction-bookmark,pageAction-copyURL,pageAction-emailLink,
> pageAction-sendToDevice,pageActionButton,pocket,privateWindow,readerMode-
> urlBar,screenshots,search,searchIcon,trackingProtection,urlbar
Oh no, "search" and "searchIcon" was no longer on the Central. Accidentally added them back during rebasing.
Flags: needinfo?(fliu)
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/6f6a270a020f
Support Screenshots page action with UITour, r=dveditz,Gijs
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/6f6a270a020f
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
I have verified that this bug is fixed on today's nightly build.
Status: RESOLVED → VERIFIED
I can confirm this issue is fixed in Fx 57.0b8, I verified on Windows 10 x64, mac OS X 10.13 and Ubuntu 14.04 LTS.
status-firefox57: fixed → verified
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.