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

Tighten 'same site' checks to include 'scheme'. #449

Closed
wants to merge 1 commit into from
Closed

Tighten 'same site' checks to include 'scheme'. #449

wants to merge 1 commit into from

Conversation

mikewest
Copy link
Member

@mikewest mikewest commented Sep 13, 2019

This patch introduces 'schemelessly same site' on both 'host' and 'URL',
moves 'same site' to URL, and tightens it by requiring a scheme match in
addition to a registrable domain match. This, hopefully, will create
stronger security boundaries in places where we perform "same site"
checks, while giving us the flexibility to grandparent in existing
behavior we decide to keep.

Partially addresses #448.


Preview | Diff

This patch introduces 'schemelessly same site' on both 'host' and 'URL',
moves 'same site' to URL, and tightens it by requiring a scheme match in
addition to a registrable domain match. This, hopefully, will create
stronger security boundaries in places where we perform "same site"
checks, while giving us the flexibility to grandparent in existing
behavior we decide to keep.

Partially addresses #448.
@mikewest
Copy link
Member Author

(Nevermind, I apparently already had a fork of the repo lying around. :) )

<p>Two <a for=/>URL</a>s, <var>A</var> and <var>B</var> are said to be
<dfn for=url>schemelessly same site</dfn> with each other if <var>A</var>'s
<a for=url>host</a> is <a for=host>schemelessly same site</a> with <var>B</var>'s
<a for=url>host</a>.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A URL's host can be null. So probably needs to be about as complicated (or you need to assert a certain scheme).

@domenic
Copy link
Member

domenic commented Nov 6, 2019

I ran into the same confusion today writing an explainer where I wanted to use "same site" and "site isolation" to mean the same thing. I'm happy to try to drive this change over the finish line, but I can't understand @annevk's outstanding review comment. (In particular the second sentence seems to be missing some words.)

@annevk
Copy link
Member

annevk commented Nov 7, 2019

Sorry, I meant about as complicated as the comparison done above for hosts. In that you have to null check host before comparing and returning false if a host is null.

Also, reading the original issue again the idea was that we'd also define same-site for origins in HTML and defer the URL comparison to that. Meaning that for a URL you first compute the origin and then see if it's same-site.

Also, HTML has this "agent cluster key" concept. Should we define that as "site" and also define "schemeless site" so we can talk about these things concretely without referring to the comparisons?

@domenic
Copy link
Member

domenic commented Nov 7, 2019

Those all seem like reasonable things to do. The definitions for "site" and "schemeless site" seem like they should be able to be done in the URL spec.

If we changed same-site (and probably schemelessly-same-site) to operate on origins I'm unsure whether it'd still be useful to have definitions for URLs in this spec.

@annevk
Copy link
Member

annevk commented Nov 7, 2019

The usage in https://fetch.spec.whatwg.org/#cross-origin-resource-policy-check could be done on top of origins entirely.

HTML's https://html.spec.whatwg.org/multipage/origin.html#is-a-registrable-domain-suffix-of-or-is-equal-to is also used by WebAuthn, but uses registrable domain and public suffix directly (and has to).

I think HTML's algorithm could be used for cookies too, reading https://tools.ietf.org/html/draft-ietf-httpbis-rfc6265bis-03#section-5.1.3.

HTML's agent stuff would be a little cleaner if you could obtain a site from an origin (i.e., "obtain an agent cluster key" renamed).

No other uses come to mind, but it seems like you had at least one other and maybe Mike knows more.

domenic added a commit that referenced this pull request Nov 11, 2019
@domenic domenic closed this in #457 Nov 20, 2019
domenic added a commit that referenced this pull request Nov 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants