Require user interaction for downloads.open()

RESOLVED FIXED in Firefox 56

Status

defect
RESOLVED FIXED
2 years ago
11 months ago

People

(Reporter: mstriemer, Assigned: mstriemer)

Tracking

({dev-doc-complete})

unspecified
mozilla56
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox55 wontfix, firefox56 fixed)

Details

(Whiteboard: triaged)

Attachments

(2 attachments)

Assignee

Description

2 years ago
An extension should only be able to call downloads.open() when initiated by a user interaction like a button click or shortcut command. If this is not the case then a user might not be able to tell why files are opening on their computer which could be confusing.

This is how Chrome works, as noted in a Chromium bug [1] but it is not documented.

[1] https://bugs.chromium.org/p/chromium/issues/detail?id=349715
Assignee

Updated

2 years ago
Duplicate of this bug: 1367529
Comment hidden (mozreview-request)

Comment 3

2 years ago
mozreview-review
Comment on attachment 8874458 [details]
Bug 1369782 - Require user interaction for downloads.open()

https://reviewboard.mozilla.org/r/145824/#review149890

A couple of questions:
1. What about a test of calling `download.open()` successfully?
2. Does this have the same issues as bug 1369577?  If so, can you add notes there that the bug covers both permissions.request and download.open pending bug 1350151

::: toolkit/components/extensions/test/mochitest/test_chrome_ext_downloads_open.html:44
(Diff revision 1)
> +    // TODO: Once downloads.{pause,cancel,resume} lands (bug 1245602) test that this gives a good
> +    // error when called with an incompleted download.

Hm, I see you just copied this but pause/resume/cancel landed some time ago.  Can you open a new bug for this?

::: toolkit/components/extensions/test/mochitest/test_chrome_ext_downloads_open.html:90
(Diff revision 1)
> +    // TODO: Once downloads.{pause,cancel,resume} lands (bug 1245602) test that this gives a good
> +    // error when called with an incompleted download.

and this should be removed
Comment hidden (mozreview-request)
Assignee

Comment 5

2 years ago
mozreview-review-reply
Comment on attachment 8874458 [details]
Bug 1369782 - Require user interaction for downloads.open()

https://reviewboard.mozilla.org/r/145824/#review149890

> A couple of questions:
> 1. What about a test of calling download.open() successfully?

I had a test for it, but it would download the text file to ~/Downloads and open TextEdit when it worked.
> 2. Does this have the same issues as bug 1369577?  If so, can you add notes there that the bug covers both permissions.request and download.open pending bug 1350151

It does. I've commented on both bugs.

> Hm, I see you just copied this but pause/resume/cancel landed some time ago.  Can you open a new bug for this?

Filed bug 1370623.

> and this should be removed

I removed both comments since there's a bug now.

Comment 6

2 years ago
mozreview-review
Comment on attachment 8874458 [details]
Bug 1369782 - Require user interaction for downloads.open()

https://reviewboard.mozilla.org/r/145824/#review150440
Attachment #8874458 - Flags: review?(aswan) → review+

Comment 7

2 years ago
mozreview-review-reply
Comment on attachment 8874458 [details]
Bug 1369782 - Require user interaction for downloads.open()

https://reviewboard.mozilla.org/r/145824/#review149890

On question 1, is it feasible to override the associated application just for the duration of the test?
That doesn't need to hold this up though...
Assignee

Comment 8

2 years ago
mozreview-review-reply
Comment on attachment 8874458 [details]
Bug 1369782 - Require user interaction for downloads.open()

https://reviewboard.mozilla.org/r/145824/#review149890

I'm not sure, it could be. We had discussed this briefly over IRC but it seemed like it wasn't easy to do.

I filed bug 1371814 to track adding that test.
Assignee

Updated

2 years ago
Keywords: checkin-needed
Assignee

Updated

2 years ago
See Also: → 1369577
Assignee

Comment 9

2 years ago
This needs the fix from bug 1369577 to function in all cases. That will likely need to be uplifted.
See Also: → 1350151

Comment 10

2 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/fe04c0fec69b
Require user interaction for downloads.open() r=aswan
Keywords: checkin-needed
Backed out for eslint failure at toolkit/components/extensions/test/mochitest/test_chrome_ext_downloads_open.html:60:

https://hg.mozilla.org/integration/autoland/rev/44a734e1209d8c164a9b1b7e56fc9cb65440efab

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=fe04c0fec69b97608c8b7090e4b7e6ff73ac0f6d&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=106068140&repo=autoland

TEST-UNEXPECTED-ERROR | /home/worker/checkouts/gecko/toolkit/components/extensions/test/mochitest/test_chrome_ext_downloads_open.html:60:12 | No corresponding 'removeEventListener(keypress)' was found. (mozilla/balanced-listeners)
TEST-UNEXPECTED-ERROR | /home/worker/checkouts/gecko/toolkit/components/extensions/test/mochitest/test_chrome_ext_downloads_open.html:63:58 | There should be no space after '{'. (object-curly-spacing)
TEST-UNEXPECTED-ERROR | /home/worker/checkouts/gecko/toolkit/components/extensions/test/mochitest/test_chrome_ext_downloads_open.html:63:74 | There should be no space before '}'. (object-curly-spacing)
Flags: needinfo?(mstriemer)
This also fails the test:

https://treeherder.mozilla.org/logviewer.html#?job_id=106072993&repo=autoland

TEST-UNEXPECTED-FAIL | toolkit/components/extensions/test/mochitest/test_chrome_ext_downloads_open.html | Opening download fails. - got true, expected false
TEST-UNEXPECTED-FAIL | toolkit/components/extensions/test/mochitest/test_chrome_ext_downloads_open.html | The error is informative. - got undefined, expected "Invalid download id 10"
Comment hidden (mozreview-request)

Comment 14

2 years ago
mozreview-review
Comment on attachment 8874458 [details]
Bug 1369782 - Require user interaction for downloads.open()

https://reviewboard.mozilla.org/r/145824/#review152200

::: toolkit/components/extensions/test/mochitest/test_chrome_ext_downloads_open.html:60
(Diff revisions 2 - 3)
>    await extension.unload();
>  });
>  
>  add_task(async function downloads_open_invalid_id() {
>    async function pageScript() {
> -    window.addEventListener("keypress", () => {
> +    window.addEventListener("keypress", async function handler() {

You can also write `async () => { ...`
And if you pass `{once: true}` as the third parameter to `addEventListener()` then you don't need to explicitly remove the listener.
Assignee

Comment 15

2 years ago
mozreview-review-reply
Comment on attachment 8874458 [details]
Bug 1369782 - Require user interaction for downloads.open()

https://reviewboard.mozilla.org/r/145824/#review152200

> You can also write `async () => { ...`
> And if you pass `{once: true}` as the third parameter to `addEventListener()` then you don't need to explicitly remove the listener.

Ah, that's handy. Thanks!
Assignee

Comment 16

2 years ago
Tests/lint fixed.
Flags: needinfo?(mstriemer)
Keywords: checkin-needed

Updated

2 years ago
Whiteboard: triaged

Comment 17

2 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/8bb2840c6ee3
Require user interaction for downloads.open() r=aswan
Keywords: checkin-needed

Comment 18

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/8bb2840c6ee3
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Assignee

Comment 19

2 years ago
Extension to help testing.
Assignee

Comment 20

2 years ago
Comment on attachment 8874458 [details]
Bug 1369782 - Require user interaction for downloads.open()

Approval Request Comment
[Feature/Bug causing the regression]: bug 1245636
[User impact if declined]: Add-ons would be able to open downloads without a user action which could cause confusion or lead to users opening files they did not intend to open.
[Is this code covered by automated tests?]: Partially
[Has the fix been verified in Nightly?]: I have verified it, following STR below
[Needs manual test from QE? If yes, steps to reproduce]: 
  1. Extract and install the attached add-on in about:debugging.
  2. Click the browser action button for the add-on.
  3. Click the "Start download" button.
    * In release, beta: text file download will complete and be opened
    * In nightly: text file is downloaded and error is reported on the add-on page
  4. Click "Open download" button.
    * Download opens in all versions.
  5. Click "View Firefox logo" button.
  6. Save image with Right click > Save image as...
    * In release, beta: image download will complete and be opened
    * In nightly: image is downloaded and error is reported on the add-on page
[List of other uplifts needed for the feature/fix]: bug 1369577 (that bug also affects bug 1197420)
[Is the change risky?]: No
[Why is the change risky/not risky?]:
  * The code is very similar to permissions.request() which has been on Nightly for a few months.
  * This could cause some existing add-ons to stop working but is compatible with Chrome. The add-ons that will stop working could lead to a bad user experience, however.
[String changes made/needed]: No
Attachment #8874458 - Flags: approval-mozilla-beta?
Bug 1369577 doesn't seem fixed yet.  Does this mean I should hold off on this uplift request, or can this go in independently?  Also, since bug 1245636 is not new (it landed in 48), what's the reason for this skipping a train?
Flags: needinfo?(mstriemer)
Assignee

Comment 22

2 years ago
Comment on attachment 8874458 [details]
Bug 1369782 - Require user interaction for downloads.open()

After some more discussion we've decided that since bug 1369577 has hit a bit of a snag we'll let this ride the train to 56.
Flags: needinfo?(mstriemer)
Attachment #8874458 - Flags: approval-mozilla-beta?

Updated

2 years ago
See Also: → CVE-2018-5105
I've added something about this: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/downloads/open. Please let me know if this covers it. I didn't document that this is new in Firefox 56, since advertising the old behavior doesn't seem to make sense.
Flags: needinfo?(mstriemer)
Assignee

Comment 25

2 years ago
That makes sense to me, thanks!
Flags: needinfo?(mstriemer)

Updated

11 months ago
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.