Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Consider deprecating the ability for scope to match query parameters #1469

Open
mgiuca opened this issue Sep 6, 2019 · 3 comments
Open

Consider deprecating the ability for scope to match query parameters #1469

mgiuca opened this issue Sep 6, 2019 · 3 comments

Comments

@mgiuca
Copy link

mgiuca commented Sep 6, 2019

This has come up in regards to @wanderview 's scope pattern matching proposal; in particular on issue #4. I've come to think that the current ability to match the query / search part of a URL in the service worker scope is totally broken; it will almost never produce the desired results and there isn't a way to make it useful. So I would like us to consider deprecating or removing it.

The scope matching algorithm performs an exact prefix match to determine which URLs are in scope. It is never explicitly stated in the SW spec, but this applies to the query part of the scope as well. Even when applied to the path, this is less than desirable, but when applied to the query, it makes almost no sense. The general form of a query string in a URL is that it represents an unordered collection of key=value pairs, delimited by &s.

As an example, if we imagine a scope /foo?bar=baz, the author probably intended this to match URLs with a key "bar" having the value "baz", but there are two mistakes here:

  • This won't match any URLs with "bar=baz" that isn't the first query parameter. For example, /foo?bar=baz&boo=hoo matches, but /foo?boo=hoo&bar=baz won't. So the order matters.
    • Note that query strings are often generated by placing the key/value pairs in a hash map and then serializing the hash map into the URL syntax, which means the parameters will often appear in a pseudo-random order; thus whether or not the URL matches will be flaky. (For example, Python explicitly randomizes hash table insertion order on each startup, so the order won't even be stable from one day to the next if the URLs are coming from a Python server.)
  • This will match any key "bar" having a value that starts with "baz" (it is a prefix match, not an exact match). /foo?bar=bazzah will also (probably unintentionally) match.

There is no way to fix either of these issues with the current scope syntax. For example, you could try adding a '&' to the end of the scope, which would limit it to an exact match, but then it would require that there be another query parameter after "bar". And there is no way to make the query parameter matching order-independent.

Supporting this in the future represents an ongoing headache:

  • Consider a string syntax instead of a dictionary whatwg/urlpattern#4 demonstrates that it would be difficult to incorporate this legacy behaviour within a "smart" pattern matching system.
  • The Web App Manifest scope matching algorithm is the same as service worker except that it ignores the query part of the scope. I would like to unify the two algorithms into the same definition, but doing so would currently require a flag to determine whether to ignore the query part or not.

Given this, I can't imagine anybody is using this feature correctly. I would like to propose deprecation of this feature (e.g., a large red box in the spec, and recommending that user agents display a warning in the console if a '?' appears in the scope during registration), and perhaps removal if its usage is sufficiently low. https://github.com/wanderview/service-worker-scope-pattern-matching proposes a way to match query parameters correctly.

@jakearchibald
Copy link
Contributor

The scope is a prefix, so it seems pretty obvious that order of things in the query string matters.

I don't think we can just add pattern matching to regular scope strings (and other places we use URLs). * crops up in most patterns, and it's also valid in a URL.

If someone deliberately puts ?foo in a scope string, it seems weird to just ignore it. Shouldn't it do something, or throw?

@mgiuca
Copy link
Author

mgiuca commented Sep 16, 2019

The scope is a prefix, so it seems pretty obvious that order of things in the query string matters.

Yes, but I don't think it's particularly obvious that the scope is a prefix match (if you just learn by example, as opposed to reading the spec, I think it's fairly unintuitive both a) that the path is prefix-based as opposed to hierarchical; see #1272, and b) that the query is prefix-based as opposed to matching individual key=value pairs).

Up until now, I hadn't really thought about the query part, and I was surprised to realise that it behaves this way (even though I had previously read the spec that it was a prefix match, I didn't put 2 and 2 together to realise that this imposes a strict ordering on the query part of a URL). As I said above, I don't think there's really any way to use this feature correctly.

I don't think we can just add pattern matching to regular scope strings (and other places we use URLs). * crops up in most patterns, and it's also valid in a URL.

I think @wanderview is doing a good job figuring out how to add more flexible scope matching (on wanderview/service-worker-scope-pattern-matching) so I'm not proposing any new syntax here. I just think it would be good to remove this mis-feature.

If someone deliberately puts ?foo in a scope string, it seems weird to just ignore it. Shouldn't it do something, or throw?

It should probably log a warning. Even if we don't ignore it, I think we should still advise implementations to log a warning because of the surprising behaviour.

Note that the Manifest spec does just ignore the query.

@wanderview
Copy link
Member

If we want to remove this feature I think we would just strip the search params from the navigation url before performing the prefix match. We would probably need to collect data to see how much this is being used in the wild.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants