fix: url parser should throw error on invalid ipv4

[anonrig]

Fixes: #42914

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/43780/

[Trott]

@nodejs/url

[Trott]

It looks like this results in many other tests failing?

[anonrig]

Hi @Trott I'm trying to pinpoint the reason behind why the C++ code does not work.

[Trott]

Hi @Trott I'm trying to pinpoint the reason behind why the C++ code does not work.

I'm not terribly familiar with that code or frankly C++ in general, plus I'm kinda occupied with other things right now, but a quick git blame on the relevant part of the file and a git shortlog on the file suggest that @TimothyGu or @addaleax might be people who can provide some direction/guidance. Neither of them are as active on the project as they once were, but they're both still around.

[anonrig]

So, the problem was that when the parser saw more than 4 . character, it just forgot about the input being a ipv4 and just continued parsing the input as a domain. Because of this particular line:

bool is_ipv4;
ParseIPv4Host(decoded.c_str(), decoded.length(), &is_ipv4);
if (is_ipv4)
  return;

Another big issue is around this line. Since, if parse number fails, it should also return a FAILURE, but before it should define is_ipv4 = true.

int64_t n = ParseNumber(mark, pointer);
if (n < 0)
  return;

[targos]

test/fixtures/wpt should be updated using https://github.com/nodejs/node-core-utils/blob/main/docs/git-node.md#git-node-wpt

[anonrig]

This will introduce more failing tests. I've opened an additional issue in order to keep track of spec compliance and fix those issues.

[anonrig]

@aduh95 can you re-run the failed test?

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/43802/

[Trott]

Microscopic nit-pick on the commit message. Can be ignored (someone can fix it while landing if they feel strongly about it), but FYI for future stuff: First word should be an imperative verb (so an action). Instead of "should validate...", just "validate....".

[anonrig]

Thank you @Trott. You're absolutely right! I'll definitely take this feedback into consideration in my next commit. BTW, one of the test always goes to timeout. Is this a common problem or related to this pull request?

[Trott]

one of the test always goes to timeout. Is this a common problem or related to this pull request?

I haven't looked to see what test you are talking about specifically, but test reliability is definitely an issue we struggle with, so likely a common problem.

[Trott]

Landed in 87d0d7a

[Trott]

Thanks for the contribution! 🎉

[anonrig]

Thanks @Trott!

[sepehrst]

@anonrig @aduh95 @targos @Trott This is not the right fix.
It now throws on new URL('http://256.256.256.256.com')
Sorry I don't have the time to open an issue.

[aduh95]

This PR changed quite a lot between the time I reviewed it and the time it landed, and #42915 (comment) looks like a blocker to me. I think we should revert.