-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Build upon URL rather than PSL #3735
Conversation
@mikewest what do you think? It might be worth trying to take this further and remove "is a registrable domain suffix of or is equal to", but that'll require changes to WebAuthn. |
(Note: this relies upon landing whatwg/url#391 first.) |
I do think it's simpler if we could just remove "is a registrable domain suffix of or is equal to" entirely, as it isn't providing much value if it's just wrapping "same site" with a host parser. It seems like WebAuthn could do that directly, and it would end up being clearer: @equalsJeffH, WDYT? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Happy to have the broader discussion about whether to keep the algorithm at all in a separate bug if you prefer to land this as-is.
Hmm, this is wrong. The problem is that on |
Oh. Ha. You're entirely correct. I should have thought about this more deeply before hitting the "yes!" button. :( |
I gave this another shot. I think it's correct now. |
@mikewest could you take another look? Would be nice to know if the primitives in URL actually work. |
Instead of "parsing" the Public Suffix List directly, use new terminology from the URL Standard. Fixes #3711.
1bb524c
to
eaeb47f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this does look correct now. Thanks for taking another pass, and sorry I missed it the first time around. One optional nit that might clarify things, but otherwise LGTM.
source
Outdated
<p>Suffixes must be compared after applying the <span>host parser</span> algorithm.</p> | ||
</li> | ||
<li><p>If <var>host</var>'s <span>registrable domain</span> is null, then return false. <ref | ||
spec=URL></p></li> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Optional nit: It might make this clearer if you stick with comparing |host| with its public suffix, rather than checking that its registrable domain is null (as the latter requires more knowledge of how URL actually works).
Instead of "parsing" the Public Suffix List directly, use new terminology from the URL Standard.
Fixes #3711.
/infrastructure.html ( diff )
/origin.html ( diff )
/references.html ( diff )