Fixes url parsing for .888 hosts #14588
Conversation
| const char* cases[] = { | ||
| "test.888", // Non-ipv4 component case. | ||
| "test1.test2.888", // Non-ipv4 component case. | ||
| "1.2.3.4.888", // Too many components case. |
There was a problem hiding this comment.
Please also add 1.2.3.888 to check that it's a valid URL but not a valid IPv4 URL.
There was a problem hiding this comment.
added in a6ecf5c
It actually didn't pass as I didn't cover residual bits case in parse(I wrongly assumed that was not .888 case)
There was a problem hiding this comment.
Now DoIPv4AddressToNumber never reports BROKEN when last component is .888
There was a problem hiding this comment.
Now DoIPv4AddressToNumber never reports BROKEN when last component is .888
Is that the right behavior? I mean if you have 1.2.3.888, which is not a valid IPv4 and try to convert this IPv4 URL to a number, it seems like it should fail because the input was not an IPv4 address in the first place, no?
There was a problem hiding this comment.
For correct IPv4 address parser should say that it is exactly IPv4 address, so we don't break it
For Url parser now says it is not a broken ipv4 address, but something else.
There was a problem hiding this comment.
I see, so we now have this?
- 1.2.3.4 -> valid URL, valid IPv4
- 1.2.3.889 -> invalid URL, invalid IPv4
- 1.2.3.888 -> valid URL, but not an IPv4 at all
It might be worth adding 1.2.3.889 as a test case too just to make sure that we don't change that one.
There was a problem hiding this comment.
added 692c0ff
also now checking for 888 ending being an exact string to avoid 888 in hex or oct form.
|
I originally thought it was an implementation detail to forbid such urls, but now it appears it is explicitly specified behavior for url parsing: Actually appearance of .888 tld in UD contracts happened around same time, so that was a technically legal tld when they were designing it: While with this PR browser is able to handle 'something.888' and redirect to resolved content hash link I'm concerned now if is a stable solution. Chromium may always enforce this specification requirements in some other place. |
my opinion is that it's not the risk for us to support this feature if it both goes against the whatwg spec and chromium's implementation. we should instead encourage folks to deprecate this TLD. seems very fragile otherwise; and also the URL parsers that many web developers use like NodeJS's probably won't understand .888 |
|
https://github.com/brave/brave-browser/wiki/Resolve-Methods-for-Unstoppable-Domains claims that we support |
|
@fmarier I believe so, we can resolve wallet address but URL bar resolution was never working correctly. We should update our wiki and messaging in the code, and communicate with UD that we can't support .888 due to security. |
|
should we close this one, @supermassive ? |
|
Closing as we decided not to support .888. |
Resolves brave/brave-browser#24585
Sec review: https://github.com/brave/security/issues/981
Submitter Checklist:
QA/YesorQA/No;release-notes/includeorrelease-notes/exclude;OS/...) to the associated issuenpm run test -- brave_browser_tests,npm run test -- brave_unit_tests,npm run lint,npm run gn_check,npm run tslintgit rebase master(if needed)Reviewer Checklist:
gnAfter-merge Checklist:
changes has landed on
Test Plan: