Skip to content

Allow for domain name trailing dot in PublicSuffixDatabase#6111

Merged
yschimke merged 4 commits intosquare:masterfrom
yschimke:dot
Jun 4, 2020
Merged

Allow for domain name trailing dot in PublicSuffixDatabase#6111
yschimke merged 4 commits intosquare:masterfrom
yschimke:dot

Conversation

@yschimke
Copy link
Copy Markdown
Collaborator

@yschimke yschimke commented Jun 3, 2020

No description provided.

val unicodeDomain = IDN.toUnicode(domain)
val domainLabels = unicodeDomain.split('.')

if (domainLabels.lastOrNull() == "") {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

There's no need for OrNull here; split always returns at least one string.

assertThat(parse("https://127.0.0.1").topPrivateDomain()).isNull();

// https://github.com/square/okhttp/issues/6109
assertThat(parse("http://a./").topPrivateDomain()).isNull();
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Also test these?

a..
..a
a..b
.a
.
..

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

My interpretation is that hostname ending with '.' is the one special case since it's actually the blessed form of dns, and we mostly just all agreed to drop that trailing '.'

Or is there some special behaviour we want for these?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I really don’t know. I was mostly worried that we’d return squareup.com as the top private domain, which seemed wrong. But I’d forgotten about the trailing . thing in DNS.

Now I’m curious, What Does Guava Do?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

The awkward thing with this change is that HttpUrl parsing is our shield, but also known to be non exact. And as much as it doesn't match URI, it will be painful and cause bugs in various places e.g. calling through to JDK methods like ProxySelector.

Copy link
Copy Markdown
Collaborator

@dave-r12 dave-r12 left a comment

Choose a reason for hiding this comment

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

Nice one.

@swankjesse
Copy link
Copy Markdown
Collaborator

Does this return the wrong answer for say, https://squareup.com.

@yschimke
Copy link
Copy Markdown
Collaborator Author

yschimke commented Jun 3, 2020

I feel like this is a strong hint :)
I'll check. Thanks!

@yschimke
Copy link
Copy Markdown
Collaborator Author

yschimke commented Jun 3, 2020

Does this return the wrong answer for say, https://squareup.com.

Ok, you win this round Mr Swank.

assertThat(parse("https://127.0.0.1").topPrivateDomain()).isNull();

// https://github.com/square/okhttp/issues/6109
assertThat(parse("http://a./").topPrivateDomain()).isNull();
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I really don’t know. I was mostly worried that we’d return squareup.com as the top private domain, which seemed wrong. But I’d forgotten about the trailing . thing in DNS.

Now I’m curious, What Does Guava Do?

@yschimke yschimke merged commit ef4e0ba into square:master Jun 4, 2020
@yschimke yschimke deleted the dot branch October 30, 2020 08:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants