Skip to content

Build upon URL rather than PSL#3735

Merged
annevk merged 2 commits into
masterfrom
annevk/stop-building-on-psl-directly
Jun 7, 2018
Merged

Build upon URL rather than PSL#3735
annevk merged 2 commits into
masterfrom
annevk/stop-building-on-psl-directly

Conversation

@annevk

@annevk annevk commented Jun 4, 2018

Copy link
Copy Markdown
Member

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

annevk commented Jun 4, 2018

Copy link
Copy Markdown
Member Author

@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

annevk commented Jun 4, 2018

Copy link
Copy Markdown
Member Author

(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

mikewest commented Jun 4, 2018

Copy link
Copy Markdown
Member

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?

@mikewest mikewest left a comment

Copy link
Copy Markdown
Member

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

annevk commented Jun 4, 2018

Copy link
Copy Markdown
Member Author

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

mikewest commented Jun 4, 2018

Copy link
Copy Markdown
Member

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

@annevk

annevk commented Jun 4, 2018

Copy link
Copy Markdown
Member Author

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

@annevk

annevk commented Jun 6, 2018

Copy link
Copy Markdown
Member Author

@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

@mikewest mikewest left a comment

Copy link
Copy Markdown
Member

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.

Comment thread 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
Copy Markdown
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