Closed Bug 1270306 Opened 8 years ago Closed 8 years ago

isAllowedFileSchemeAccess should return `false` rather than `true`

Categories

(WebExtensions :: Untriaged, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla49

People

(Reporter: kmag, Assigned: rpl, Mentored)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete, Whiteboard: [extension][good first bug][berlin])

Attachments

(1 file)

+++ 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.
Flags: blocking-webextensions+
tracking for 48/49.
[Tracking Requested - why for this release]:
Apologies, misunderstood what this was tracking for.
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.
Attachment #8755985 - Flags: review?(kmaglione+bmo)
Attachment #8755985 - Flags: review?(kmaglione+bmo) → review+
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
Attachment #8755985 - Flags: approval-mozilla-aurora?
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.
Keywords: checkin-needed
We must indeed fix this before we go live with WebExtensions. Tracking it.
https://hg.mozilla.org/mozilla-central/rev/661db976e5e8
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Assignee: cgrebs → lgreco
Attachment #8755985 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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.
Flags: needinfo?(kmaglione+bmo)
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/
Product: Toolkit → WebExtensions
Flags: needinfo?(kmaglione+bmo)
You need to log in before you can comment on or make changes to this bug.