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

Match Service Worker Registration should assert same-origin? #1118

Closed
estark37 opened this issue Apr 14, 2017 · 11 comments
Closed

Match Service Worker Registration should assert same-origin? #1118

estark37 opened this issue Apr 14, 2017 · 11 comments
Labels

Comments

@estark37
Copy link

It's non-obvious from reading https://w3c.github.io/ServiceWorker/#scope-match-algorithm that clientURL can't match a cross-origin service worker registration. AFAICT it relies on URLs serializing with a slash at the end. Can Step 5 assert the matchingScope is same-origin with clientURL?

@jungkees
Copy link
Collaborator

I think the serialized URLs include the scheme, host, and port as well as the path. So, it won't match to any cross-origin URLs. But reviewing the algorithm, I found we compare the serialized client URL to URL records. I'll fix this after double-checking it.

@noncombatant
Copy link

I think it would be better to compare URL records to URL records, rather than to compare strings to strings. It would make the text and the code less fragile and subject to misinterpretation, thanks to the typefulness. No?

@jungkees
Copy link
Collaborator

jungkees commented May 4, 2017

I think comparing the serialized URLs would be better here. The registration matching is a prefix-based string matching rather than a path structural. So, we'd have to compare the strings for the paths at the end anyway. Also, parsing the given URLs to the register() API and serializing them in the matching algorithm nicely abstract away the details of the URL comparison. (URL standard doesn't provide a URL equality check algorithm itself, either.)

@jungkees
Copy link
Collaborator

jungkees commented May 4, 2017

Having reviewed the matching algorithm and the other algorithms that access to the scope to registration map, I found they are all considering the type of the key of the map as a string. What needs to be changed is the type of the key of the scope to registration map (from a URL to a string.)

@jungkees jungkees added the bug label May 4, 2017
@noncombatant
Copy link

noncombatant commented May 10, 2017

Emily and I still think it's necessary to perform a simple check that the origins (not the URLs) are equal. Origin equality is well-defined: are the schemes equal (case-insensitive string comparison), are the hostnames equal (case-insensitive string comparison), and are the ports equal (int16_t equality).

Origin-matching is a crucial security guarantee on the web, and so we don't feel entirely comfortable without a clear statement (in the spec and in the implementation) that an origin equality check is required.

So, we hope you will re-open this bug and add spec and implementation changes. :)

@jungkees
Copy link
Collaborator

Sorry. Re-opened.

I thought URL parser and URL serializer guarantee the same origin requirement for the matching URL strings. We explicitly invoke URL serializer when storing a new registration, and later compare them to serialzed URLs. I'd like to hear @annevk's comment on it.

Meantime, I just checked out the implementations. Both Chrome and Firefox get the list of the registrations for the origin first and match the longest possible URL from that list. Not sure if it was to reflect the security requirement or performance requirement, or both. That considered, I'm happy to align the algorithm to the implementations. If I change this, I plan to change the type of the scope url of the scope to registration map from a string to a URL record and adjust the references accordingly.

/cc @mattto @wanderview @aliams

@jungkees jungkees reopened this May 11, 2017
@annevk
Copy link
Member

annevk commented May 11, 2017

Comparing URL strings instead of origins works here because only HTTPS URLs can enter the system, right? That's probably useful to point out in a note.

(The URL parser takes care of lowercasing the scheme, normalizing the host (case-insensitive is far from enough; IDNA), and normalizing the port (removing it when it's the default port for the scheme).)

Having said that, it seems that the example in step 5 is somewhat misleading as it omits rather crucial parts of the URL which might have led folks astray.

@jungkees
Copy link
Collaborator

Comparing URL strings instead of origins works here because only HTTPS URLs can enter the system, right?

Yes and because the URL parser normalizes the URLs as you pointed.

@noncombatant, @estark37, are you okay with @annevk's comments above? If so, I'd prefer to add notes as suggested rather than changing the algorithms.

@estark37
Copy link
Author

I believe the URL string comparison is only safe because http/https URLs serialize with a trailing slash. If it weren't for the trailing slash, one URL could be a prefix-match for another non-same-origin URL.

Relying on the trailing slash serialization seems extremely fragile and non-obvious to me, which is why I suggested adding an assertion that the matched URL is same-origin.

@jungkees
Copy link
Collaborator

I believe the URL string comparison is only safe because http/https URLs serialize with a trailing slash.
Relying on the trailing slash serialization seems extremely fragile and non-obvious to me

That's right. Now I understand your concern better. Thanks for having convinced me.

Can Step 5 assert the matchingScope is same-origin with clientURL?

I'll add an assertion as you suggested and improve words on the note.

@estark37
Copy link
Author

Thanks @jungkees! Sorry if I didn't explain it well before :)

jungkees added a commit that referenced this issue May 12, 2017
This patch adds an assertion in the Match Service Worker Registration
that the given URL and the matched URL are the same origin. This also
adds a note that the URL serializer serializes the URLs with a trailing
slash at the end of the origin part of the URLs.

Fixes #1118.
jungkees added a commit that referenced this issue May 12, 2017
This patch adds an assertion in the Match Service Worker Registration
that the given URL and the matched URL are the same origin. This also
adds a note that the URL serializer serializes the URLs with a trailing
slash at the end of the origin part of the URLs.

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

No branches or pull requests

4 participants