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

Build upon URL rather than PSL #3735

Merged
merged 2 commits into from
Jun 7, 2018
Merged

Conversation

annevk
Copy link
Member

@annevk annevk commented Jun 4, 2018

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 )

@annevk
Copy link
Member Author

annevk commented Jun 4, 2018

@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.

@annevk
Copy link
Member Author

annevk commented Jun 4, 2018

(Note: this relies upon landing whatwg/url#391 first.)

@annevk annevk added the do not merge yet Pull request must not be merged per rationale in comment label Jun 4, 2018
@mikewest
Copy link
Member

mikewest commented Jun 4, 2018

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?

Copy link
Member

@mikewest mikewest left a 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.

@annevk
Copy link
Member Author

annevk commented Jun 4, 2018

Hmm, this is wrong. The problem is that on x.example.com you can use example.com, but you cannot use y.x.example.com. So I guess we need some kind of public suffix check here.

@mikewest
Copy link
Member

mikewest commented Jun 4, 2018

Oh. Ha. You're entirely correct. I should have thought about this more deeply before hitting the "yes!" button. :(

@annevk
Copy link
Member Author

annevk commented Jun 4, 2018

I gave this another shot. I think it's correct now.

@annevk
Copy link
Member Author

annevk commented Jun 6, 2018

@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.
@annevk annevk force-pushed the annevk/stop-building-on-psl-directly branch from 1bb524c to eaeb47f Compare June 6, 2018 12:26
Copy link
Member

@mikewest mikewest left a 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>
Copy link
Member

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).

@annevk annevk merged commit 326c644 into master Jun 7, 2018
@annevk annevk deleted the annevk/stop-building-on-psl-directly branch June 7, 2018 09:31
@annevk annevk removed the do not merge yet Pull request must not be merged per rationale in comment label Jun 7, 2018
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.

"If the given value is not a registrable domain ..."
2 participants