Add the chrome.publicSuffix extensions API [chromium/src : main]
Dave Vandyke (Gerrit)
Dave Vandyke added 2 comments![]( "Open in Gerrit")
Hey Devlin, any chance you could take a first pass on these changes to see if the overall approach looks OK? From there I figure I could loop in some more folks. It's working well in my testing (luckily the spec proposal is detailed and has a lot of useful examples), but I've not added a sync API before.
Also, I want to call out that I generated the changes with AI to start with, and then iterated on the changes from there. I kind of regret doing that, because I didn't get the learning experience and understanding from hashing it out myself - I think next time I'll just use it to review what I already came up with.
// TODO: Consider if handling these cases more strictly than
// net::CanonicalizeHost() is worthwhile.
// 1. %2e is normalized to ".".
// ExpectInvalidHostname("subdomain%2edomain.com");
// 2. Domains with underscores are accepted.
// ExpectInvalidHostname("foo_bar.example");
// 3. "。" and similar are normalized to "."
// ExpectInvalidHostname("co。uk");WDYT? I figure these cases probably aren't a big deal, but if you like we could add extra logic to reject them.
Related details
- Devlin Cronin
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Devlin Cronin (Gerrit)
Devlin Cronin added 13 comments![]( "Open in Gerrit")
Thanks, Dave! This largely looks good; nothing too major.
I'd definitely recommend we pull in someone more familiar with the registry-controlled domains code at some point; my knowledge there is pretty cursory.
function availability() {IMO, this isn't really necessary. Everything else will catastrophically fail without these, and we have pretty extensive testing for our general bindings system
},can we add a test for private registries?
function unavailableWithoutPermission() {ditto here, arguably unnecessary (but you can keep this one if you like)
// Whether known public suffixes may be returned as domains. Defaults to false.nit: wrap at 80 char
namespace extensions::api::public_suffix {this file is only needed in the renderer, right? If so, let's put it there.
if (encoding != DomainEncoding::kDisplay) {nit: prefer using a switch statement so that if we add more encoding types, we update this appropriately.
const size_t registry_length = GetCanonicalHostRegistryLength(it's generally preferred to use [GetRegistryLength](https://source.chromium.org/chromium/chromium/src/+/main:net/base/registry_controlled_domains/registry_controlled_domain.h;l=248;drc=916279bfbd81b1585ebbeb9ac1c83d4283f50680); is there a reason to use this particular method here?
std::string canonical_host = net::CanonicalizeHost(hostname, &host_info); std::string_view normalized_host = TrimLeadingDot(hostname.value);doesn't the CanonicalHostname already trim the leading dot?
ExpectIsKnownSuffix("jp", true);these lead to unhelpful error messages when they go awry, because it just says that the expectation in the helper method failed.
This is mitigated by the SCOPED_TRACE you include in each of these methods, but that's still a little clunky. Would it make sense to instead have these return values? e.g., return a string from GetKnownSuffix() and a bool from IsKnownSuffix() and then use EXPECT_EQ / EXPECT_TRUE / EXPECT_FALSE at these callsites?
// TODO: Consider if handling these cases more strictly than
// net::CanonicalizeHost() is worthwhile.
// 1. %2e is normalized to ".".
// ExpectInvalidHostname("subdomain%2edomain.com");
// 2. Domains with underscores are accepted.
// ExpectInvalidHostname("foo_bar.example");
// 3. "。" and similar are normalized to "."
// ExpectInvalidHostname("co。uk");WDYT? I figure these cases probably aren't a big deal, but if you like we could add extra logic to reject them.
I have no strong feelings, as long as the behavior is a) reasonable and b) safe : )
TEST_F(PublicSuffixHooksDelegateTest, UnknownMethodIsNotHandled) {arguably, this whole file is unnecessary with the api test (providing end-to-end "real" coverage) + unittests for the utility methods. Unless there's a reason to keep this, I'd be inclined to remove it.
Related details
- Dave Vandyke
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dave Vandyke (Gerrit)
Dave Vandyke added 13 comments![]( "Open in Gerrit")
IMO, this isn't really necessary. Everything else will catastrophically fail without these, and we have pretty extensive testing for our general bindings system
Done.
can we add a test for private registries?
I haven't done that so far since I think these browser tests are using the real PSL, unlike the unit tests that are using the dummy PSL. Therefore, I worried that if we are too specific about the domains (e.g. by adding tests for `github.io` or similar) here, the tests could start failing one day. WDYT?
ditto here, arguably unnecessary (but you can keep this one if you like)
Done
// Whether known public suffixes may be returned as domains. Defaults to false.nit: wrap at 80 char
Done
this file is only needed in the renderer, right? If so, let's put it there.
Done
nit: prefer using a switch statement so that if we add more encoding types, we update this appropriately.
Done
const size_t registry_length = GetCanonicalHostRegistryLength(it's generally preferred to use [GetRegistryLength](https://source.chromium.org/chromium/chromium/src/+/main:net/base/registry_controlled_domains/registry_controlled_domain.h;l=248;drc=916279bfbd81b1585ebbeb9ac1c83d4283f50680); is there a reason to use this particular method here?
I figure it makes sense since we already ran the hostname through canoncialisation, and this way we don't need to convert it back to a GURL. But open to suggestions, sounds like I might be missing some nuance here?
std::string canonical_host = net::CanonicalizeHost(hostname, &host_info);This appears deprecated: https://source.chromium.org/chromium/chromium/src/+/main:url/url_canon.h;l=429-442;drc=cf08f47476acbc08a7617acc4f1b9ad4e5286c7f
I think I'm using [net::CanonicalizeHost()](https://source.chromium.org/chromium/chromium/src/+/main:net/base/url_util.h;l=177-182;drc=cf08f47476acbc08a7617acc4f1b9ad4e5286c7f?q=net%2Fbase%2Furl&ss=chromium%2Fchromium%2Fsrc) not [url::CanonicalizeHost()](https://source.chromium.org/chromium/chromium/src/+/main:url/url_canon.h;l=429-442;drc=cf08f47476acbc08a7617acc4f1b9ad4e5286c7f). *(Though honestly I don't really understand why the codebase needs both... seems confusing 🙃.)*
std::string_view normalized_host = TrimLeadingDot(hostname.value);doesn't the CanonicalHostname already trim the leading dot?
I have reworked this, since the name `CanonicalHostname` was kind of confusing, and like you say we can trim the leading dot when we parse/validate the hostname. WDYT?
these lead to unhelpful error messages when they go awry, because it just says that the expectation in the helper method failed.
This is mitigated by the SCOPED_TRACE you include in each of these methods, but that's still a little clunky. Would it make sense to instead have these return values? e.g., return a string from GetKnownSuffix() and a bool from IsKnownSuffix() and then use EXPECT_EQ / EXPECT_TRUE / EXPECT_FALSE at these callsites?
Done
// TODO: Consider if handling these cases more strictly than
// net::CanonicalizeHost() is worthwhile.
// 1. %2e is normalized to ".".
// ExpectInvalidHostname("subdomain%2edomain.com");
// 2. Domains with underscores are accepted.
// ExpectInvalidHostname("foo_bar.example");
// 3. "。" and similar are normalized to "."
// ExpectInvalidHostname("co。uk");Devlin CroninWDYT? I figure these cases probably aren't a big deal, but if you like we could add extra logic to reject them.
I have no strong feelings, as long as the behavior is a) reasonable and b) safe : )
Same here, I'm leaning towards not worrying about these cases at all, if that makes the code simpler. I will leave this open for now, in case other folks feel strongly.
TEST_F(PublicSuffixHooksDelegateTest, UnknownMethodIsNotHandled) {arguably, this whole file is unnecessary with the api test (providing end-to-end "real" coverage) + unittests for the utility methods. Unless there's a reason to keep this, I'd be inclined to remove it.
SGTM, Done. (I figured it was needed for code coverage stats, but if that's not the case then I don't see what it added either.)
Related details
- Devlin Cronin
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Devlin Cronin (Gerrit)
Devlin Cronin added 7 comments![]( "Open in Gerrit")
Thanks, Dave!
I'm also adding cfredric here as the owner of registry_controlled_domains code -- Chris, can you take a look at the implementation in public_suffix_util.* and see if that looks reasonable?
Dave Vandykecan we add a test for private registries?
I haven't done that so far since I think these browser tests are using the real PSL, unlike the unit tests that are using the dummy PSL. Therefore, I worried that if we are too specific about the domains (e.g. by adding tests for `github.io` or similar) here, the tests could start failing one day. WDYT?
I think that's probably okay.
The PSL is bundled with the browser, so if it starts failing, it will do so deterministically, and we can just use a new private registry. There's existing precedence for this in other places. And my guess is that github is going to stay around awhile, so hopefully this isn't a common occurrence ; ) (appspot is another common choice)
const size_t registry_length = GetCanonicalHostRegistryLength(Dave Vandykeit's generally preferred to use [GetRegistryLength](https://source.chromium.org/chromium/chromium/src/+/main:net/base/registry_controlled_domains/registry_controlled_domain.h;l=248;drc=916279bfbd81b1585ebbeb9ac1c83d4283f50680); is there a reason to use this particular method here?
I figure it makes sense since we already ran the hostname through canoncialisation, and this way we don't need to convert it back to a GURL. But open to suggestions, sounds like I might be missing some nuance here?
I was just mentioning it because of the guidance on the function, but I think you're right that it _shouldn't_ matter in this case (since we've canonicalized it).
std::string canonical_host = net::CanonicalizeHost(hostname, &host_info);Dave VandykeThis appears deprecated: https://source.chromium.org/chromium/chromium/src/+/main:url/url_canon.h;l=429-442;drc=cf08f47476acbc08a7617acc4f1b9ad4e5286c7f
I think I'm using [net::CanonicalizeHost()](https://source.chromium.org/chromium/chromium/src/+/main:net/base/url_util.h;l=177-182;drc=cf08f47476acbc08a7617acc4f1b9ad4e5286c7f?q=net%2Fbase%2Furl&ss=chromium%2Fchromium%2Fsrc) not [url::CanonicalizeHost()](https://source.chromium.org/chromium/chromium/src/+/main:url/url_canon.h;l=429-442;drc=cf08f47476acbc08a7617acc4f1b9ad4e5286c7f). *(Though honestly I don't really understand why the codebase needs both... seems confusing 🙃.)*
:facepalm:. Thanks; my mistake : )
std::string_view normalized_host = TrimLeadingDot(hostname.value);Dave Vandykedoesn't the CanonicalHostname already trim the leading dot?
I have reworked this, since the name `CanonicalHostname` was kind of confusing, and like you say we can trim the leading dot when we parse/validate the hostname. WDYT?
Works for me, but I'll lead the detailed review to someone more familiar with the net code
RequestResult result(RequestResult::INVALID_INVOCATION);test coverage bots indicate this branch isn't exercised, but seems like it could (legitimately) occur. Can we update the apitest to have a flow that reaches here?
(It's slightly superfluous since getDomain goes through the same path, but it seems cheap enough : ))
return std::string(nit: is this wrapping necessary? (The return type of substr is a string already)
Related details
- Chris Fredrickson
- Dave Vandyke
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dave Vandyke (Gerrit)
Dave Vandyke added 3 comments![]( "Open in Gerrit")
Dave Vandykecan we add a test for private registries?
Devlin CroninI haven't done that so far since I think these browser tests are using the real PSL, unlike the unit tests that are using the dummy PSL. Therefore, I worried that if we are too specific about the domains (e.g. by adding tests for `github.io` or similar) here, the tests could start failing one day. WDYT?
I think that's probably okay.
The PSL is bundled with the browser, so if it starts failing, it will do so deterministically, and we can just use a new private registry. There's existing precedence for this in other places. And my guess is that github is going to stay around awhile, so hopefully this isn't a common occurrence ; ) (appspot is another common choice)
Phew OK, in that case I've added some more test cases. Done
RequestResult result(RequestResult::INVALID_INVOCATION);test coverage bots indicate this branch isn't exercised, but seems like it could (legitimately) occur. Can we update the apitest to have a flow that reaches here?
(It's slightly superfluous since getDomain goes through the same path, but it seems cheap enough : ))
Done
nit: is this wrapping necessary? (The return type of substr is a string already)
Done
Related details
- Chris Fredrickson
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chris Fredrickson (Gerrit)
Chris Fredrickson added 5 comments![]( "Open in Gerrit")
Thanks, Dave!
I'm also adding cfredric here as the owner of registry_controlled_domains code -- Chris, can you take a look at the implementation in public_suffix_util.* and see if that looks reasonable?
That implementation looks reasonable to me!
if (*known_suffix == hostname.value) {Consider using `operator==(const std::optional<std::string>&, const std::string&)` here, since the entire body of the `if (known_suffix.has_value())` block is the other if statement and its block.
return ApplyEncoding(std::string(hostname.value), options.encoding);This is already a std::string, and will get copied automatically by the function call.
```suggestion
return ApplyEncoding(hostname.value, options.encoding);
```
domain = std::string(hostname.value);```suggestion
domain = hostname.value;
```
(Although you could avoid a string-copy by just returning directly from here, instead of falling down to the return on line 129.)
std::optional<bool> IsKnownSuffix(const std::string& hostname) {Why not bool (since that's what the underlying API returns)?
```suggestion
bool IsKnownSuffix(const std::string& hostname) {
```
Related details
- Devlin Cronin
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dave Vandyke (Gerrit)
Dave Vandyke added 4 comments![]( "Open in Gerrit")
if (*known_suffix == hostname.value) {Consider using `operator==(const std::optional<std::string>&, const std::string&)` here, since the entire body of the `if (known_suffix.has_value())` block is the other if statement and its block.
I gave this a try, but I think that the separate `known_suffix.has_value()` check is still useful due to the else block. We only want to enforce `allow_unknown_suffix` if there wasn't a value. WDYT?
return ApplyEncoding(std::string(hostname.value), options.encoding);This is already a std::string, and will get copied automatically by the function call.
```suggestion
return ApplyEncoding(hostname.value, options.encoding);
```
Done
domain = std::string(hostname.value);```suggestion
domain = hostname.value;
```(Although you could avoid a string-copy by just returning directly from here, instead of falling down to the return on line 129.)
Done
std::optional<bool> IsKnownSuffix(const std::string& hostname) {Why not bool (since that's what the underlying API returns)?
```suggestion
bool IsKnownSuffix(const std::string& hostname) {
```
- Chris Fredrickson
- Devlin Cronin
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dave Vandyke (Gerrit)
Dave Vandyke added 1 comment![]( "Open in Gerrit")
// TODO: Consider if handling these cases more strictly than
// net::CanonicalizeHost() is worthwhile.
// 1. %2e is normalized to ".".
// ExpectInvalidHostname("subdomain%2edomain.com");
// 2. Domains with underscores are accepted.
// ExpectInvalidHostname("foo_bar.example");
// 3. "。" and similar are normalized to "."
// ExpectInvalidHostname("co。uk");Devlin CroninWDYT? I figure these cases probably aren't a big deal, but if you like we could add extra logic to reject them.
Dave VandykeI have no strong feelings, as long as the behavior is a) reasonable and b) safe : )
Same here, I'm leaning towards not worrying about these cases at all, if that makes the code simpler. I will leave this open for now, in case other folks feel strongly.
Oh, and I forgot about this one. WDYT Chris?