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

Should the scope terminate by "/"? #554

Closed
xxyzzzq opened this issue Feb 10, 2017 · 15 comments · Fixed by #590
Closed

Should the scope terminate by "/"? #554

xxyzzzq opened this issue Feb 10, 2017 · 15 comments · Fixed by #590
Assignees
Labels
scope member Related to scope member

Comments

@xxyzzzq
Copy link

xxyzzzq commented Feb 10, 2017

The step 3 of "within scope algorithm" says:

If target is same origin as scope and target's pathname starts with scope's pathname, return true.

Suppose the scope is "https://example.com/foo", and target is "https://example.com/foobar.html", it is within scope. Is this WAI?

@marcoscaceres
Copy link
Member

marcoscaceres commented Feb 13, 2017

@xxyzzzq, that is correct. It would be in scope.

const scope = new URL("https://example.com/foo"); 
const target = new URL("https://example.com/foobar.html");
console.log(scope.origin === target.origin && target.pathname.startsWith(scope.pathname)); // true

@xxyzzzq
Copy link
Author

xxyzzzq commented Feb 14, 2017

OK, I see. Thanks :)

@xxyzzzq xxyzzzq closed this as completed Feb 14, 2017
@mgiuca
Copy link
Collaborator

mgiuca commented Jul 24, 2017

Is this the originally intended behaviour, or a historical accident? It seems rather unintuitive.

To get technical...

If ... target's pathname starts with scope's pathname, return true.

target and scope are URLs, not URL objects (URL = spec-level object, URL = user-facing object). I think specs should be using the URL path concept, not the URL.pathname interface attribute.

The important difference is that URL path is a list of strings, one for each path segment. If you used that, you wouldn't get this weird behaviour where the scope matches path segments with the same prefix. The only text change would be:

If ... target's path starts with scope's path, return true.

I wonder if it isn't too late to change this. Though there are existing implementations, there might not be much code in the wild that relies on this behaviour.

@mgiuca mgiuca reopened this Jul 24, 2017
@mgiuca
Copy link
Collaborator

mgiuca commented Jul 24, 2017

If this isn't changed, we should have a non-normative recommendation that you always put a trailing slash at the end of scope, to avoid triggering this behaviour.

@marcoscaceres
Copy link
Member

I'm ok with changing this.

@delapuente
Copy link

Notice that this behaviour is already on the platform. Service Worker's scope does the same way.

@mgiuca
Copy link
Collaborator

mgiuca commented Jul 25, 2017

@delapuente Yep, you're right:

https://www.w3.org/TR/service-workers-1/#scope-match-algorithm

The URL string matching in this step is prefix-based rather than path-structural (e.g. a client URL string with "/prefix-of/resource.html" will match a registration for a scope with "/prefix").

That sucks, and although we don't need to use SW scope as a precedent (manifest scope != SW scope), it's a strong precedent and we should probably maintain compatibility. So let's keep it but add a similar non-normative note to Manifest. Marcos are you happy with that decision?

@marcoscaceres
Copy link
Member

Happy. We on purpose reused sw scope matching. Could you please send a PR?

mgiuca added a commit to mgiuca/manifest that referenced this issue Jul 26, 2017
mgiuca added a commit to mgiuca/manifest that referenced this issue Jul 26, 2017
@mgiuca
Copy link
Collaborator

mgiuca commented Feb 1, 2018

Reopening. I just discovered why this is worse than I initially thought (sorry for not spotting this earlier). There's a fundamental problem here with a real-world case.

  • If the scope does not end with a '/', it over-matches on any paths that start with the same characters. This was the original bug, and you work around it by always adding a '/' (as I suggested in the non-normative note I added in Scope matching algorithm: fix and explain #590 ). However,
  • If the scope does end with a '/', the user agent won't capture deep links to the slashless version.

Example: There's been some public discussion around the newly launched Google Maps PWA, https://www.google.com/maps?force=pwa

The manifest specifies "scope" as "https://www.google.com/maps". I reported this to the Maps team, suggesting that they add a trailing '/' to the scope, and they said they couldn't, because otherwise any incoming links to "https://www.google.com/maps" would not be captured by the app. Good point!

"https://www.google.com/maps" automatically redirects to "https://www.google.com/maps/", which then gets captured by the app, but now we've opened a non-application context which then redirects into an application context. How this manifests depends on the UA but this seems to involve browser windows opening and closing.

We're essentially forcing developers to choose between their scope over-matching on all URLs that start with the same characters, even if they are in a sibling path, or the site's canonical URL not being matched by the scope. There is no work-around for this. Devs just have to choose between one set of bad behaviour or another.

Ideally, a scope of "https://www.google.com/maps" would match "https://www.google.com/maps" and "https://www.google.com/maps/", but not "https://www.google.com/mapsearch" (a hypothetical future URL for an unrelated product).

Edit: Another suggestion by @g-ortuno was to keep the string match, but make a special rule that a scope with a trailing slash matches the exact path without the slash. So scope "https://www.google.com/maps/" matches "https://www.google.com/maps" and "https://www.google.com/maps/anything", but not "https://www.google.com/mapsearch". Scope "https://www.google.com/maps" still matches "https://www.google.com/mapsearch" as it does now.

Maybe we can break compatibility with SWs here. Note: I think this is also an issue for SWs, but for a different reason (if you click a link to https://www.google.com/maps, you need to be online to get redirected to the offline-available https://www.google.com/maps/). I'll file a separate issue on SWs.

@delapuente
Copy link

I read this issue on the SW GitHub too. Although I agree with you that the URL schema is a reminiscence of UNIX paths and, indeed, the standard defines a hierarchical relationship of URL path segments, the standard does not equate /maps with /maps/. Those are effectively two different URLs and the decision of both redirecting to the same content is done at application-level and so, I think it should be solved at application-level. For the maps example, it is just a matter of renaming the url to /searchingmaps and that's all.

@mgiuca
Copy link
Collaborator

mgiuca commented Mar 9, 2018

I don't think it's reasonable to ask web site developers to rename their entire site around the fact that there's a deficiency in what should be a very simple path scope match.

True, /maps and /maps/ are not the same. I'm not asking for them to be the same. I'm asking for a mechanism that does not force the developer to consider /mapsearch a child of /maps. No reasonable software developer would ever expect /mapsearch to be a child node of /maps in a path hierarchy.

A good example is if you had a PWA at /warcraft and then you wanted to make /warcraft2. You'd realise you're stuck because /warcraft2 is inside the /warcraft PWA scope, and there's no sensible other name for that directory.

Anyway, I've spoken with @slightlyoff and @jakearchibald and it looks like (for this and other reasons) we want to introduce "auxiliary scopes" (so you can specify multiple separate scope paths which can be either prefix match or exact match). That would solve this issue. This was for SW, but we can introduce the same concept into Manifest, to match.

@marcoscaceres
Copy link
Member

That would solve this issue. This was for SW, but we can introduce the same concept into Manifest, to match.

Sounds good. Looking forward to hearing more.

@kenchris
Copy link
Collaborator

kenchris commented Mar 9, 2018

I am sure @boyofgreen will be very interested in this. When you have something to share, please do.

@marcoscaceres marcoscaceres added the scope member Related to scope member label Sep 8, 2019
@marcoscaceres
Copy link
Member

@mgiuca, do you think you could relook at this one? Or give us an opinion of how you would like us to proceed.

@mgiuca
Copy link
Collaborator

mgiuca commented May 25, 2020

We don't want to break backwards compatibility now. I think this falls under the category of expanding the scope matching syntax, which Ben Kelly is exploring on wanderview/service-worker-scope-pattern-matching. Let's close this out now and assume it'll be addressed by the expanded scope pattern syntax.

@mgiuca mgiuca closed this as completed May 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
scope member Related to scope member
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants