1270306 - isAllowedFileSchemeAccess should return `false` rather than `true`

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect)

Description

[Kris Maglione [:kmag]]

+++ This bug was initially created as a clone of Bug #1253646 +++

We currently don't allow extensions to load file: URLs, so until that's changed, this method should return false, rather than true.

We should also uplift this fix to 48, since some extensions rely on this flag.

Chris, do you mind taking this since you did the initial implementation?

[Tracking Requested - why for this release]:

Firefox 48 is meant to be the first stable release version for WebExtensions.
The current behavior of this API may lead extensions to expect things to work
in ways that aren't currently supported. Multiple bugs have already been filed
by affected developers.

The change is trivial enough that it should be safe to uplift.

Comment 1

[Marcia Knous [:marcia]]

tracking for 48/49.

Comment 2

[Andy McKay]

[Tracking Requested - why for this release]:
Apologies, misunderstood what this was tracking for.

Comment 3

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: 1-Bug_1270306____webext__extension_isAllowedFileSchemeAccess_should_return_null__r_kmag.patch

Hi Kris, 
I'm helping Christopher on this patch and its uplift to aurora:

This is the patch that makes isAllowedFileSchemeAccess to return false as described above.

Comment 4

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8755985 [details] [diff] [review]
1-Bug_1270306____webext__extension_isAllowedFileSchemeAccess_should_return_null__r_kmag.patch

Approval Request Comment

[Feature/regressing bug #]: 1253646

[User impact if declined]: 
Firefox 48 is meant to be the first stable release version for WebExtensions. The current behavior of this API may lead extensions to expect things to work in ways that aren't currently supported. Multiple bugs have already been filed by affected developers.
 
[Describe test coverage new/current, TreeHerder]: 
The existent test case has been changed to pass on the new expected value (which is false instead of true).

[Risks and why]: 
Minimal. The changes are trivial and well-tested. The only risk is to existing extension code relying on the current behavior, but the current (incorrect) behavior is much more likely to break extensions than the new behavior.

[String/UUID change made/needed]: none

Comment 5

[Luca Greco [:rpl] [:luca] [:lgreco]]

push to try on a recent tip:

- https://treeherder.mozilla.org/#/jobs?repo=try&revision=929649820554

Comment 6

[Luca Greco [:rpl] [:luca] [:lgreco]]

Besides the above request to uplift the fix on aurora (Firefox 48), 
this patch needs to land on Firefox 49 first.

Added checkin-needed keyword.

Comment 7

[Luca Greco [:rpl] [:luca] [:lgreco]]

push to try of the patch applied to aurora:

- https://treeherder.mozilla.org/#/jobs?repo=try&revision=7223e1d51946

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/661db976e5e8

Comment 9

[Sylvestre Ledru [:Sylvestre]]

We must indeed fix this before we go live with WebExtensions. Tracking it.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/661db976e5e8

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/4fc43b4a5d20

Comment 12

[Alex Sirota (iosart)]

I'm encountering a problem due to the current implementation of this flag. The WebExtensions version ColorZilla has been back ported from Chrome. It injects a content script into the current page to sample colors. In Chrome, the isAllowedFileSchemeAccess flag indicates if the addon has access to the current tab if a file:// url is loaded there - if set to true the addon can inject and operate if false, the user is prompted to enable local access. In Firefox, currently, this flag always returns false, but when I attempted to inject the content script - it succeeds and everything works fine - which is a great thing (!). 

So the issue is really the semantic differences in meaning of this flag between browsers - in Firefox it currently means "false -- cannot load file:// urls, but *can* access them" in Chrome, it means "false - cannot access file:// urls". Because the name of the flag is "isAllowedFileSchemeAccess" (and not isAllowedOpenFileScheme") I believe the Chrome meaning is more accurate, and in any case, I believe that the semantics should be aligned between browsers to ensure cross-browser compatibility.

Comment 13

[Matthew Tylee Atkinson]

I'm developing an extension to find Landmark regions on the page [1] and I'm encountering the same problem as Alex above, that Firefox and Chrome differ on the meaning of this flag.  In Chrome it signifies whether the browser will allow a content script to run in a file:// document.  Chrome provides a UI setting to allow/disallow content scripts from running in local file documents.  In Firefox, this function's return value signifies whether my extension can load a local file by itself.

This should be implemented the same way across browsers. Has a standard meaning for this been decided by the W3 group?  I would tend to lean on using Chrome's interpretation of it by default, as Alex suggests above.

Cheers,


Matthew

[1] http://matatk.agrip.org.uk/landmarks/