Closed Bug 1280370 Opened 8 years ago Closed 6 years ago

contextMenus do not support other protocols (ex. 'magnet:*', 'acestream:*', 'sop:*')

Categories

(WebExtensions :: Untriaged, defect, P3)

Product:
WebExtensions
Component:
Untriaged
Version:
50 Branch
Type:
defect

Tracking

(firefox63 verified)

Status:
VERIFIED FIXED
Milestone:
mozilla63
Iteration:
63.3 - Aug 6
Tracking Flags:
Tracking Status
firefox63 --- verified

People

(Reporter: macieksitarz, Assigned: robwu)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete, Whiteboard: [contextMenus] triaged)

Attachments

(6 files)

Bug 1280370 - Properly parse MatchPatterns with "data:" scheme
59 bytes, text/x-review-board-request
zombie
: review+
mixedpuppy
: review+
Details
Bug 1280370 - Don't set MatchPattern::mMatchSubdomain if the scheme does not support hosts
59 bytes, text/x-review-board-request
zombie
: review+
Details
Bug 1280370 - Properly parse MatchPattern for schemes with no authority
59 bytes, text/x-review-board-request
zombie
: review+
mixedpuppy
: review+
Details
Bug 1280370 - Allow any scheme in targetUrlPatterns
59 bytes, text/x-review-board-request
mixedpuppy
: review+
Details
Bug 1280370 - Remove unneeded DomainIsWildcard check
59 bytes, text/x-review-board-request
mixedpuppy
: review+
Details
Bug1280370.png
33.30 KB, image/png
Details 
User Agent: Mozilla/5.0 (X11; Fedora; Linux x86_64; rv:47.0) Gecko/20100101 Firefox/47.0
Build ID: 20160606100313

Steps to reproduce:

I'm porting an extension from Chrome to Firefox. It uses context menus, different ones for each type of target link.
The problem is in Chrome context menus are added for MatchPatterns:
"['magnet:*', 'acestream:*', 'sop:*']" but in Firefox I get an error in Browser Console.

Sample code to add context menu:
function onCreated(n) {
  if (chrome.runtime.lastError) {
    console.log("error creating item:" + chrome.runtime.lastError);
  } else {
    console.log("item created successfully");
  }
}
chrome.contextMenus.create({
  id: "test4",
  title: "TEST targetUrlPatterns magnet",
  contexts: ["link"],
  targetUrlPatterns: ['magnet:*', 'acestream:*', 'sop:*']
}, onCreated);

I'm aware that MatchPatterns documentation lists those kind of patterns as not proper, but I think the behaviour should be the same between the browsers. Also different protocols should be supported, as the URIs are not only http/https/ftp.

To test the functionality in Firefox run this on Browser Console:
Cu.import("resource://gre/modules/MatchPattern.jsm");
Cu.import("resource://gre/modules/BrowserUtils.jsm");
var match = new MatchPattern("magnet:*");



Actual results:

Firefox:
Error is thrown:
Invalid match pattern: 'magnet:*' MatchPattern.jsm:52
Invalid match pattern: 'acestream:*' MatchPattern.jsm:52
Invalid match pattern: 'sop:*'  MatchPattern.jsm:52

Chrome:
Pattern is created properly.


Expected results:

Firefox:
Pattern is created properly.

Chrome:
Pattern is created properly.
Switching context menus from MatchPattern to MatchGlob should fix this.
Summary: MatchPattern does not support other protocols (ex. 'magnet:*', 'acestream:*', 'sop:*') → contextMenus do not support other protocols (ex. 'magnet:*', 'acestream:*', 'sop:*')
I suppose we need different rules for matching links than we do for things like content scripts.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Priority: -- → P4
Whiteboard: [good first bug, context menues] triaged
Whiteboard: [good first bug, context menues] triaged → [good first bug][contextMenus] triaged
Mentor: aswan
Kris, can you please add some more details about the different rules we'd need?
Mentor: aswan → kmaglione+bmo
This isn't a good first bug.
Whiteboard: [good first bug][contextMenus] triaged → [contextMenus] triaged
Mentor: kmaglione+bmo
Priority: P4 → P3
Product: Toolkit → WebExtensions
Blocks: 1466876
Assignee: nobody → rob
Status: NEW → ASSIGNED
Iteration: --- → 63.2 - July 23
Note to :zombie (reviewer of the first three patches).
The third patch ensures that MatchPatterns accepts patterns for URLs that do not use "://" as scheme separator.
The first patch could have been part of the third patch, but I separated it anyway because "data" is already a kind-of-supported scheme in MatchPattern. Due to the bug that I found, the only way to match it is via <all_urls> though.

The second patch is unrelated to this patch; I found the issue while working on this code and decided to fix it.

The fourth patch builds upon the changes from the previous patches.
Comment on attachment 8994547 [details]
Bug 1280370 - Properly parse MatchPattern for schemes with no authority

https://reviewboard.mozilla.org/r/259098/#review266452

::: toolkit/components/extensions/MatchPattern.cpp:269
(Diff revision 1)
>   *****************************************************************************/
>  
>  const char* PERMITTED_SCHEMES[] = {"http", "https", "ws", "wss", "file", "ftp", "data", nullptr};
>  
> +// Known schemes that are followed by "://" instead of ":".
> +const char* STANDARD_SCHEMES[] = {"http", "https", "ws", "wss", "file", "ftp", "moz-extension", "chrome", "resource", "moz", "moz-icon", "moz-gio", nullptr};

"chrome", "resource", "moz", "moz-icon", "moz-gio"

Why do we have restricted schemes here?
Comment on attachment 8994547 [details]
Bug 1280370 - Properly parse MatchPattern for schemes with no authority

https://reviewboard.mozilla.org/r/259098/#review266452

> "chrome", "resource", "moz", "moz-icon", "moz-gio"
> 
> Why do we have restricted schemes here?

Without these schemes here, MatchPattern would fail at correctly parsing restricted schemes, because in this patch, I changed the default from "URL uses :// after scheme" to "URL uses : after scheme" (the latter is closer to how Firefox parses unknown URLs).

These schemes can only be used when restrictSchemes is set to false. This only happens when an extension has the "mozillaAddons" permission, or in a later patch, when a MatchPattern is created for the targetUrlPatterns property in the contextMenus/menus API.
(In reply to Rob Wu [:robwu] from comment #11)
> Comment on attachment 8994547 [details]
> Bug 1280370 - Properly parse MatchPattern for schemes with no authority
> 
> https://reviewboard.mozilla.org/r/259098/#review266452
> 
> > "chrome", "resource", "moz", "moz-icon", "moz-gio"
> > 
> > Why do we have restricted schemes here?
> 
> Without these schemes here, MatchPattern would fail at correctly parsing
> restricted schemes, because in this patch, I changed the default from "URL
> uses :// after scheme" to "URL uses : after scheme" (the latter is closer to
> how Firefox parses unknown URLs).

I understand that already.  

> These schemes can only be used when restrictSchemes is set to false. This
> only happens when an extension has the "mozillaAddons" permission, or in a
> later patch, when a MatchPattern is created for the targetUrlPatterns
> property in the contextMenus/menus API.

This bug doesn't appear to have a scope of exposing those schemes at all, no discussion, no blocking bugs, is there some requirements doc somewhere that caused this to be necessary?
(In reply to Shane Caraveo (:mixedpuppy) from comment #12)
> (In reply to Rob Wu [:robwu] from comment #11)
> > Comment on attachment 8994547 [details]
> > Bug 1280370 - Properly parse MatchPattern for schemes with no authority
> > 
> > https://reviewboard.mozilla.org/r/259098/#review266452
> > 
> > > "chrome", "resource", "moz", "moz-icon", "moz-gio"
> > > 
> > > Why do we have restricted schemes here?
> > 
> > These schemes can only be used when restrictSchemes is set to false. This
> > only happens when an extension has the "mozillaAddons" permission, or in a
> > later patch, when a MatchPattern is created for the targetUrlPatterns
> > property in the contextMenus/menus API.
> 
> This bug doesn't appear to have a scope of exposing those schemes at all, no
> discussion, no blocking bugs, is there some requirements doc somewhere that
> caused this to be necessary?

Support for the "resource" scheme was explicitly added in bug 1466349, so this must be supported at the very least.

The other schemes (that I explicitly list in my patch) were already supported before my patch, and I did not have a reason to change that existing behavior. Especially since changing existing behavior also implied that there are some schemes that cannot be matched in targetUrlPatterns (and either leave this undocumented, or add the seemingly odd note to the docs: "targetUrlPatterns can match anything except for chrome, moz, ...").

But if others (:zombie in particular, since he authored the original support for these schemes) want to exclude these schemes, then I will update the patch and remove these schemes and write this change in the commit message.
Iteration: 63.2 - July 23 → 63.3 - Aug 6
Comment on attachment 8994545 [details]
Bug 1280370 - Properly parse MatchPatterns with "data:" scheme

https://reviewboard.mozilla.org/r/259094/#review268468

I think this changes `data:` to be one of the permitted schemas for use in content scripts and permissions, which we currently don't support directly, but via <all_urls> and (poorly named) match_abut_blank option instead.  We probably don't want to change that, as (I believe) they still may inherit the principal of the owner document in some circumstances, which could perhaps allow bypassing permissions?

If that's the case, we'll want to move it to restricted schemes, and if not, can you please add some content script / permissions tests that assert it?

::: commit-message-4f12d:3
(Diff revision 1)
> +Bug 1280370 - Properly parse MatchPatterns with "data:" scheme
> +
> +The previous parser expected "data://", which is not a valid data:-URL.

nit: commit messages usually describe current or new state of things, not what old code did.
Attachment #8994545 - Flags: review?(tomica)
Comment on attachment 8994546 [details]
Bug 1280370 - Don't set MatchPattern::mMatchSubdomain if the scheme does not support hosts

https://reviewboard.mozilla.org/r/259096/#review268470

I couldn't even find where we use `explicit` at all, is this actually a bug in practice, or just code cleanup?
Attachment #8994546 - Flags: review?(tomica) → review+
Comment on attachment 8994547 [details]
Bug 1280370 - Properly parse MatchPattern for schemes with no authority

https://reviewboard.mozilla.org/r/259098/#review268472

Looks good, though I'm not comfortable approving C++ changes.  Please ask Shane or Kris to take a look at these patches as well.

::: toolkit/components/extensions/MatchPattern.cpp:269
(Diff revision 1)
>   *****************************************************************************/
>  
>  const char* PERMITTED_SCHEMES[] = {"http", "https", "ws", "wss", "file", "ftp", "data", nullptr};
>  
> +// Known schemes that are followed by "://" instead of ":".
> +const char* STANDARD_SCHEMES[] = {"http", "https", "ws", "wss", "file", "ftp", "moz-extension", "chrome", "resource", "moz", "moz-icon", "moz-gio", nullptr};

nit: not too happy with the name, but meh.  Is the term "standard schemes" already used somewhere? 

SCHEMES_WITH_HOST maybe?

::: toolkit/components/extensions/MatchPattern.cpp:315
(Diff revision 1)
>    if (index <= 0) {
>      aRv.Throw(NS_ERROR_INVALID_ARG);
>      return;
>    }
>  
> +  bool isStandardSeparator = true;

nit: `schemeHasHost` or `isStandardScheme` perhaps?

::: toolkit/components/extensions/MatchPattern.cpp:324
(Diff revision 1)
>    } else if (!aRestrictSchemes ||
>               permittedSchemes->Contains(scheme) ||
>               scheme == nsGkAtoms::moz_extension) {
>      mSchemes = new AtomSet({scheme});
> +    RefPtr<AtomSet> standardSchemes = AtomSet::Get<STANDARD_SCHEMES>();
> +    if (!standardSchemes->Contains(scheme)) {

`schemeHasHost = standardSchemes->Contains(scheme)`
Attachment #8994547 - Flags: review?(tomica) → review+
Comment on attachment 8994545 [details]
Bug 1280370 - Properly parse MatchPatterns with "data:" scheme

https://reviewboard.mozilla.org/r/259094/#review268468

Content scripts and permissions enforce a restricted subset of permissions that are accepted by MatchPattern.cpp, as defined in https://searchfox.org/mozilla-central/rev/3fdc491e118c5cdfbaf6e2d52f3466d2b27ad1de/toolkit/components/extensions/schemas/manifest.json#448-504,515,521

I don't think that we need tests for data:-URLs in content scripts (because of the strict schema).
(Side note: content scripts do currently not work in data:-URLs at all, see bug 1475831).
Comment on attachment 8994547 [details]
Bug 1280370 - Properly parse MatchPattern for schemes with no authority

https://reviewboard.mozilla.org/r/259098/#review268472

> nit: not too happy with the name, but meh.  Is the term "standard schemes" already used somewhere? 
> 
> SCHEMES_WITH_HOST maybe?

I wanted the variable to end with `_SCHEMES` for consistency with the other two scheme arrays around this line.

Can also use "KNOWN_SCHEMES" if you'd like.
Comment on attachment 8994545 [details]
Bug 1280370 - Properly parse MatchPatterns with "data:" scheme

https://reviewboard.mozilla.org/r/259094/#review268524
Attachment #8994545 - Flags: review+
> Can also use "KNOWN_SCHEMES" if you'd like.

That's not exactly correct, but as I said, meh.
Comment on attachment 8994547 [details]
Bug 1280370 - Properly parse MatchPattern for schemes with no authority

https://reviewboard.mozilla.org/r/259098/#review268826

::: toolkit/components/extensions/MatchPattern.cpp:269
(Diff revision 1)
>   *****************************************************************************/
>  
>  const char* PERMITTED_SCHEMES[] = {"http", "https", "ws", "wss", "file", "ftp", "data", nullptr};
>  
> +// Known schemes that are followed by "://" instead of ":".
> +const char* STANDARD_SCHEMES[] = {"http", "https", "ws", "wss", "file", "ftp", "moz-extension", "chrome", "resource", "moz", "moz-icon", "moz-gio", nullptr};

"standard" is going to drive me nuts, they are not standard.  Please use HOST_LOCATOR_SCHEMES for lack of a better name.

::: toolkit/components/extensions/MatchPattern.cpp:323
(Diff revision 1)
> +    RefPtr<AtomSet> standardSchemes = AtomSet::Get<STANDARD_SCHEMES>();
> +    if (!standardSchemes->Contains(scheme)) {
> +      isStandardSeparator = false;
> +    }

Move this out of this block, after the else/throw.

bool requireHostLocatorScheme = hostLocatorSchemes->contains(scheme)

then see the next comment

::: toolkit/components/extensions/MatchPattern.cpp:338
(Diff revision 1)
> -  if (scheme == nsGkAtoms::about || scheme == nsGkAtoms::data) {
> -    // about: and data: URIs don't have hosts, so just match on the path.
> +  if (!isStandardSeparator) {
> +    // Unrecognized schemes and some schemes such as about: and data: URIs
> +    // don't have hosts, so just match on the path.
>      // And so, ignorePath doesn't make sense for these matchers.
>      aIgnorePath = false;
>    } else {
>      if (!StringHead(tail, 2).EqualsLiteral("//")) {
>        aRv.Throw(NS_ERROR_INVALID_ARG);
>        return;
>      }
>  

I'm thinking something more like the following will force "://" for schemes we know require it, but otherwise be flexible enough to handle any unknown schemes without requiring they have a host or not.

// I haven't looked, but assuming tail starts after :
bool hasHostSplitter = !!StringHead(tail, 2).EqualsLiteral("//");

// To be honest, I'm not even sure we really need this check except to maybe help users of this api avoid WTF moments.
if (!hasHostSplitter && requireHostLocatorScheme) {
  // moved from else below
  aRv.Throw(NS_ERROR_INVALID_ARG);
  return;
}

// Because we do no validation on about|data, I think we should keep the check here.
if (!hasHostSplitter || scheme == nsGkAtoms::about || scheme == nsGkAtoms::data) {
  ignorePath = false;
} else {
  ....
Comment on attachment 8994545 [details]
Bug 1280370 - Properly parse MatchPatterns with "data:" scheme

https://reviewboard.mozilla.org/r/259094/#review268838

fix the commit message
Attachment #8994545 - Flags: review+
Comment on attachment 8994546 [details]
Bug 1280370 - Don't set MatchPattern::mMatchSubdomain if the scheme does not support hosts

https://reviewboard.mozilla.org/r/259096/#review268842

::: toolkit/components/extensions/MatchPattern.cpp:428
(Diff revision 1)
>    if (!DomainIsWildcard() && !MatchesDomain(aURL.Host())) {
>      return false;
>    }

Also drop the "!DomainIsWildcard()" part here.  MatchesDomain already checks that up front.
Attachment #8994546 - Flags: review+
Comment on attachment 8994548 [details]
Bug 1280370 - Allow any scheme in targetUrlPatterns

https://reviewboard.mozilla.org/r/259100/#review268866
Attachment #8994548 - Flags: review?(mixedpuppy) → review+
Comment on attachment 8994545 [details]
Bug 1280370 - Properly parse MatchPatterns with "data:" scheme

https://reviewboard.mozilla.org/r/259094/#review268956
Attachment #8994545 - Flags: review+
Comment on attachment 8998575 [details]
Bug 1280370 - Remove unneeded DomainIsWildcard check

https://reviewboard.mozilla.org/r/261632/#review268958
Attachment #8998575 - Flags: review?(mixedpuppy) → review+
Comment on attachment 8994547 [details]
Bug 1280370 - Properly parse MatchPattern for schemes with no authority

https://reviewboard.mozilla.org/r/259098/#review268960
Attachment #8994547 - Flags: review+
Pushed by rob@robwu.nl:
https://hg.mozilla.org/integration/autoland/rev/587e951a256e
Properly parse MatchPatterns with "data:" scheme r=mixedpuppy,zombie
https://hg.mozilla.org/integration/autoland/rev/b6cb27b7a579
Don't set MatchPattern::mMatchSubdomain if the scheme does not support hosts r=mixedpuppy,zombie
https://hg.mozilla.org/integration/autoland/rev/0134a2124266
Properly parse MatchPattern for schemes with no authority r=mixedpuppy,zombie
https://hg.mozilla.org/integration/autoland/rev/d01b3ac48bdc
Allow any scheme in targetUrlPatterns r=mixedpuppy
https://hg.mozilla.org/integration/autoland/rev/72dd828f2481
Remove unneeded DomainIsWildcard check r=mixedpuppy
Attached image Bug1280370.png — 
This issue is verified as fixed on Firefox 63.0a1(20180809101613) under Win 7 64-bit and Mac OS X 10.13.3.

Please see the attached video.
Status: RESOLVED → VERIFIED
status-firefox63: fixed → verified
These patches did not only fix targetUrlPattern in the menus API, but they also allow the tabs.query API to match data:-URLs, .e.g

// Open data:,x in a new tab and run:
browser.tabs.query({url: "data:*"}).then(tabs => console.log(tabs[0]));

// Nightly 63:
Object { ... url: "data:,x" ... }

// Firefox 61 / 62
[Exception... "Illegal value"  nsresult: "0x80070057 (NS_ERROR_ILLEGAL_VALUE)"  location: "JS frame :: chrome://browser/content/parent/ext-tabs.js :: query :: line 747"  data: no]
Error: An unexpected error occurred
Keywords: dev-doc-needed
My understanding is that other URL schemes are properly matched (allowed?) but I'm not sure which ones we should be telling people about. I looked at the chrome documentation and it is even more cryptic than ours, not saying what schemes are considered valid and only lists: '*' | 'http' | 'https' | 'file' | 'ftp'

What is the final result here? Anything goes or the addition of a few new valid schemes?
Flags: needinfo?(rob)
https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/Match_patterns
needs to mention support for "data:"-URLs as a match pattern.
Before this bug, these could only be matched by <all_urls>; Starting with Firefox 63 these can also be matched via MatchPatterns with the "data" scheme, as shown in https://hg.mozilla.org/mozilla-central/rev/587e951a256e .

You should not add any more schemes to the "Match patterns" documentation.


The "targetUrlPatterns" field at menus.create/update should be updated to mention that they support any scheme (even those that are usually not allowed in a match pattern).
Flags: needinfo?(rob)
(In reply to Rob Wu [:robwu] from comment #41)
> https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/
> Match_patterns
> needs to mention support for "data:"-URLs as a match pattern.
> Before this bug, these could only be matched by <all_urls>; Starting with
> Firefox 63 these can also be matched via MatchPatterns with the "data"
> scheme, as shown in https://hg.mozilla.org/mozilla-central/rev/587e951a256e .
> 
> You should not add any more schemes to the "Match patterns" documentation.
> 
> 
> The "targetUrlPatterns" field at menus.create/update should be updated to
> mention that they support any scheme (even those that are usually not
> allowed in a match pattern).

Thanks!
As suggested, I added this sentence to the description of the targetUrlPatterns parameter of menus.create() (https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/menus/create):

"This parameter supports any URL scheme, even those that are usually not allowed in a match pattern."

I also made sure that "data" was added as a valid URL scheme on the match patterns page. (https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/Match_patterns)

Finally, I added two entries to the 63 release notes:

The menus.create() method now supports any URL scheme, even those that are usually not allowed in a match pattern.

and

Match patterns for URLs now explicitly match the "data" URL scheme ({{bug(1280370)}}).
Flags: needinfo?(rob)
Keywords: dev-doc-needed → dev-doc-complete
Looks good, thanks. I have made a small modification of the 63 release notes, to clarify that it's the "targetUrlPatterns" parameter (to menus.create and menus.update) that supports more schemes (instead of "menus.create").

https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/63$compare?locale=en-US&to=1422814&from=1422804
Flags: needinfo?(rob)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: