As referenced here [1], plugins should be denied from non-HTTP/HTTPS protocols. [1] https://bugzilla.mozilla.org/show_bug.cgi?id=1307604#c61

Should this be part of flash blocking (pref-ed off behind |plugins.flashBlock.enabled|) or not?

This bug is not about restricting Flash to HTTPS. It's about banning non-HTTP non-HTTPS (e.g. file:) protocols.

No, this should be entirely independent. It's something that we should ship and release-note separately.

Here's the preliminary site compatibility doc: https://www.fxsitecompat.com/en-CA/docs/2017/flash-will-be-loaded-only-from-https-sites/

Stefan, heads-up about this change. I'm not going to land this until 55 so this has some time to bake. This *will* break some Flash authoring tools which do Flash debugging against file: URIs, and that is an unfortunate but intended change. I don't think we need extensive testing of this; I've tested this manually on ftp and the automated tests here cover the following cases: * plugins still work from HTTP * plugins aren't available from file: * plugins aren't available from chrome: * plugins aren't available if a user loads about:blank directly in the browser * plugins are available if a HTTP site loads about:blank in a subframe * plugins are available if a HTTP site loads about:blank in a window

Comment on attachment 8843455 [details] Bug 1335475 - Deny plugins from non-HTTP/HTTPS origins. https://reviewboard.mozilla.org/r/117202/#review118860 ::: commit-message-a8f45:1 (Diff revision 1) > +Bug 1335475 - Deny plugins from non-HTTP/HTTPS origins. r?qdot r?ksteuber Did you mean to commit a file with the commit message?

Comment on attachment 8843455 [details] Bug 1335475 - Deny plugins from non-HTTP/HTTPS origins. https://reviewboard.mozilla.org/r/117202/#review118864 This seems fine to me, though I have suggested a few small changes. ::: dom/base/nsDocument.cpp:13050 (Diff revision 1) > rv = principal->GetURI(getter_AddRefs(classificationURI)); > if (NS_FAILED(rv) || !classificationURI) { > return FlashClassification::Denied; > } > > + if (Preferences::GetBool("plugins.http_https_only", true)) { Two things about this pref. 1) It does not seem to exist in modules/libpref/init/all.js which it seems like it should be. 2) The name of the pref seems to imply that it applies to all plugins when it only applies to Flash. On the other hand, though, Flash is now the only plugin left, so perhaps it is ok to use these terms interchangebly? ::: dom/plugins/test/mochitest/browser.ini:16 (Diff revision 1) > skip-if = (!e10s || os != "win") > [browser_tabswitchbetweenplugins.js] > skip-if = (!e10s || os != "win") > [browser_pluginscroll.js] > skip-if = (true || !e10s || os != "win") # Bug 1213631 > +[browser_bug1335475.js] Could this test have a bit more descriptive of a name than bug1335475?

> 1) It does not seem to exist in modules/libpref/init/all.js which it seems > like it should be. will fix. > 2) The name of the pref seems to imply that it applies to all plugins when > it only applies to Flash. On the other hand, though, Flash is now the only > plugin left, so perhaps it is ok to use these terms interchangebly? It probably is, but I'm also not sure this is correct. nsDocument::GetAllowPlugins calls ::DocumentFlashClassification without regard to whether this is the Flash plugin or any other plugin. So I think that actually the name "DocumentFlashClassification" is actually in practice "DocumentPluginClassification". Did I get some flash check wrong? > > +[browser_bug1335475.js] > > Could this test have a bit more descriptive of a name than bug1335475? I've seen people do both and not clear guidance on what to prefer. Is there a particular reason to prefer one or the other?

(In reply to Benjamin Smedberg [:bsmedberg] from comment #9) > > 2) The name of the pref seems to imply that it applies to all plugins when > > it only applies to Flash. On the other hand, though, Flash is now the only > > plugin left, so perhaps it is ok to use these terms interchangebly? > > It probably is, but I'm also not sure this is correct. > nsDocument::GetAllowPlugins calls ::DocumentFlashClassification without > regard to whether this is the Flash plugin or any other plugin. So I think > that actually the name "DocumentFlashClassification" is actually in practice > "DocumentPluginClassification". > > Did I get some flash check wrong? I originally meant for |nsIDocument::DocumentFlashClassification| to be used for Flash only. The reason that it is used in a more generic way in |nsDocument::GetAllowPlugins| is because you asked for it to be used that way [1]. Like I said, perhaps it is now ok to use "Flash" and "Plugins" interchangeably. I think it is a little confusing, but I will leave it up to you to decide. > > > > +[browser_bug1335475.js] > > > > Could this test have a bit more descriptive of a name than bug1335475? > > I've seen people do both and not clear guidance on what to prefer. Is there > a particular reason to prefer one or the other? I'm afraid the only evidence I can offer for my opinion on test naming is that :Mossop once told me that descriptive test names are preferred over test names that are bug numbers. If you think it is clear enough this way though, feel free to leave it as it is. [1] https://bugzilla.mozilla.org/show_bug.cgi?id=1323064#c4

Comment on attachment 8844509 [details] Bug 1335475 - Switch browser_tab_dragdrop.js to load a plugin from HTTP instead of a no-privilege data: URI which doesn't work any more. Also re-enable this test on Linux now that we don't have windowed-mode plugins any more, so this won't assert (undo bu https://reviewboard.mozilla.org/r/117916/#review119642 ::: browser/base/content/test/general/browser.ini:18 (Diff revision 1) > browser_bug970746.xhtml > browser_fxa_web_channel.html > browser_registerProtocolHandler_notification.html > browser_star_hsts.sjs > browser_tab_dragdrop2_frame1.xul > + browser_tab_dragdrop_embed.html I don't see this file in the patch nor in mozilla-central.

Comment on attachment 8844509 [details] Bug 1335475 - Switch browser_tab_dragdrop.js to load a plugin from HTTP instead of a no-privilege data: URI which doesn't work any more. Also re-enable this test on Linux now that we don't have windowed-mode plugins any more, so this won't assert (undo bu https://reviewboard.mozilla.org/r/117916/#review119676

Comment on attachment 8845413 [details] Bug 1335475 - Move test_refresh_navigator_plugins to a plain mochitest. https://reviewboard.mozilla.org/r/118604/#review120578 Solid patch, and the test is more readible too to boot. Thanks bsmedberg! ::: commit-message-8b20e:1 (Diff revision 1) > +Bug 1335475 - Move test_refresh_navigator_plugins to a plain mochitest. Note that it currently is broken/disabled in e10s mode. That's covered by bug 1090576, but this patch doesn't make it worse because chrome mochitests run in local mode always anyway. r?mconley Everything after the first "mochitest.", minus the review flag, can probably go onto the next line(s).

Comment on attachment 8845412 [details] Bug 1335475 - Fix test_chrome_over_plugin_window to not load plugins from a data: URI, https://reviewboard.mozilla.org/r/118602/#review120636

Comment on attachment 8845414 [details] Bug 1335475 - Remove test about loading Java from file: origins because it's no longer relevant several times over. https://reviewboard.mozilla.org/r/118606/#review120662 ::: commit-message-e6715:1 (Diff revision 1) > +Bug 1335475 - Remove test about loading Java from file: origins because it's no longer relevant several times over. r?qdot Looks like you commited a commit-message file again? Or is this something I don't know about mozreview?

Comment on attachment 8845414 [details] Bug 1335475 - Remove test about loading Java from file: origins because it's no longer relevant several times over. https://reviewboard.mozilla.org/r/118606/#review120662 > Looks like you commited a commit-message file again? Or is this something I don't know about mozreview? Ok, just saw the dev-platform email about this. Nevermind.

Comment on attachment 8845415 [details] Bug 1335475 - Load reftests that use plugins from HTTP since file: doesn't allow plugins any more, https://reviewboard.mozilla.org/r/118608/#review120674 How did you verify that this was the complete set of tests that needed to be switched to HTTP? r=dbaron assuming that the way you did that was appropriately thorough and covered all platforms... though it's info that would be useful to have in the commit message for the reviewer (and potentially others looking at it later)

Pushed by bsmedberg@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/b0838d45e9b6 Deny plugins from non-HTTP/HTTPS origins. r=bytesized,qdot https://hg.mozilla.org/integration/autoland/rev/19bf052b3949 Switch browser_tab_dragdrop.js to load a plugin from HTTP instead of a no-privilege data: URI which doesn't work any more. Also re-enable this test on Linux now that we don't have windowed-mode plugins any more, so this won't assert (undo bug 1331320). r=jaws https://hg.mozilla.org/integration/autoland/rev/732eb7783912 Fix test_chrome_over_plugin_window to not load plugins from a data: URI, r=dvander https://hg.mozilla.org/integration/autoland/rev/19c91edc6549 Move test_refresh_navigator_plugins to a plain mochitest.r=mconley https://hg.mozilla.org/integration/autoland/rev/9900d421e24e Remove test about loading Java from file: origins because it's no longer relevant several times over. r=qdot https://hg.mozilla.org/integration/autoland/rev/ab966fb76f78 Reftest harness needs to check for the test plugin without using navigator.plugins. Load reftests that use plugins from HTTP since file: doesn't allow plugins any more, r=dbaron

Backed out for failing e.g. dom/plugins/test/crashtests/539897-1.html: https://hg.mozilla.org/integration/autoland/rev/5269e230d77e73ce9fea400c446742d7c88680ad https://hg.mozilla.org/integration/autoland/rev/5be714033fbd7b76ec3018cd125309f7de799b32 https://hg.mozilla.org/integration/autoland/rev/e162d597d4d361c90d1fed0facf59b6079c9ec8e https://hg.mozilla.org/integration/autoland/rev/577c3261adf8ed01e462b1e2c7f2e54277e42ab4 https://hg.mozilla.org/integration/autoland/rev/ed96183557d896c6b0ffdf0d0d74c2068dd53ca9 https://hg.mozilla.org/integration/autoland/rev/337a2eb6aa5c80defedd32cd3e63c3258f492e23 Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=ab966fb76f781459def4babad8c41651820936b9&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=83124386&repo=autoland [task 2017-03-10T21:53:05.801019Z] 21:53:05 INFO - REFTEST TEST-START | file:///home/worker/workspace/build/tests/reftest/tests/dom/plugins/test/crashtests/539897-1.html [task 2017-03-10T21:53:05.802370Z] 21:53:05 INFO - REFTEST TEST-LOAD | file:///home/worker/workspace/build/tests/reftest/tests/dom/plugins/test/crashtests/539897-1.html | 555 / 3171 (17%) [task 2017-03-10T21:53:05.809867Z] 21:53:05 INFO - ++DOMWINDOW == 49 (0x7f7be8152c00) [pid = 1100] [serial = 1414] [outer = 0x7f7beca07400] [task 2017-03-10T21:53:05.834042Z] 21:53:05 INFO - JavaScript error: file:///home/worker/workspace/build/tests/reftest/tests/dom/plugins/test/crashtests/539897-1.html, line 7: TypeError: plugin.reinitWidget is not a function [task 2017-03-10T21:53:05.861509Z] 21:53:05 INFO - REFTEST TEST-UNEXPECTED-FAIL | file:///home/worker/workspace/build/tests/reftest/tests/dom/plugins/test/crashtests/539897-1.html | plugin should not crash item 1 There is more: REFTEST TEST-UNEXPECTED-FAIL | file:///home/worker/workspace/build/tests/reftest/tests/dom/plugins/test/crashtests/540114-1.html | plugin should not crash item 1 REFTEST TEST-UNEXPECTED-FAIL | file:///home/worker/workspace/build/tests/reftest/tests/dom/plugins/test/crashtests/598862.html | load failed: timed out waiting for reftest-wait to be removed This also makes some servo reftests fail, please check the push: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=ab966fb76f781459def4babad8c41651820936b9&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable

Pushed by felipc@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/897169cb6bec Switch browser_tab_dragdrop.js to load a plugin from HTTP instead of a no-privilege data: URI which doesn't work any more. Also re-enable this test on Linux now that we don't have windowed-mode plugins any more, so this won't assert (undo bug 1331320). r=jaws https://hg.mozilla.org/integration/mozilla-inbound/rev/336d4724cc59 Fix test_chrome_over_plugin_window to not load plugins from a data: URI, r=dvander https://hg.mozilla.org/integration/mozilla-inbound/rev/f9f2b99c9577 Move test_refresh_navigator_plugins to a plain mochitest.r=mconley https://hg.mozilla.org/integration/mozilla-inbound/rev/08748e75a1ff Reftest harness needs to check for the test plugin without using navigator.plugins. r=dbaron

https://hg.mozilla.org/mozilla-central/rev/897169cb6bec https://hg.mozilla.org/mozilla-central/rev/336d4724cc59 https://hg.mozilla.org/mozilla-central/rev/f9f2b99c9577 https://hg.mozilla.org/mozilla-central/rev/08748e75a1ff

Try build, everything looks green, planning on autolanding this: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8e3c5e2f37679e15ad3ec049b8313388887eaa24&selectedJob=99203846

Pushed by bsmedberg@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/17a2effb01e2 Load reftests that use plugins from HTTP since file: doesn't allow plugins any more, r=dbaron https://hg.mozilla.org/integration/autoland/rev/8068fb1cc45e Deny plugins from non-HTTP/HTTPS origins. r=bytesized,qdot

https://hg.mozilla.org/mozilla-central/rev/17a2effb01e2 https://hg.mozilla.org/mozilla-central/rev/8068fb1cc45e

QA has verified this bug. This was tested on: Windows 10 x64, Windows 7 x86, Ubuntu 16.04 x64, OS X 10.12 (Sierra) with 32 and 64 bits Nightly builds. The test covered the following scenarios: Flash does not load from: URI, PDF, FTP, FTPS, file:// Flash does load from: HTTP and HTTPS If you have any questions regarding the testing, please let me know.

I have documented this by adding a note here: https://developer.mozilla.org/en-US/docs/Plugins And to the Fx55 rel notes: https://developer.mozilla.org/en-US/Firefox/Releases/55#Security

We had a functionality where a local file had Flash content embedded within. After upgrading to FF55, this functionality is broken. I no long see flash content data/charts after upgrade. Also, I noticed that the api navigator.plugins do not return 'Shockwave Flash' any more. Could you please confirm whether these are expected behavior due to this bug fix. Thanks, Subba

Yes, that is expected behavior as a result of the changes made here.