Fixed: Bracketed IPv6 hosts, JS IPv6 detection

[ahankinson]

Previously, IPv6 hosts were not parsed and returned correctly in the URI parser due to two different issues.

1. For JS targets, there was a bug in the char detection that prevented IPv6 addresses from being correctly identified. This was corrected and now JS targets are parsed correctly. The issue for this was twofold:

a. Bracketed IPv6 addresses were being rejected out of hand because the opening bracket, [, char 91, was not included as a valid character.
b. Logic errors in the char ranges meant that the ranges were exclusive and not inclusive. For example, 48 >= char == char <= 48, so that would only match characters NOT in the range of 48 - 57, which was the intended behaviour.

2. For Erlang targets, the Erlang behaviour was not RFC-compliant and parsed IPv6 addresses were not being produced as bracketed versions. A decision was made to diverge from Erlang and introduce RFC-compliant bracketed formatting, so once hosts were parsed, brackets were re-introduced to the result. Note that IPv6 detection is done by looking for a : in the Host value.

AI Disclosure: OpenAI Codex was used to diagnose and propose the code changes, but these were verified, reviewed, and the tests run by me to ensure they do what they say. The commit messages and PR text are also non-AI.

Fixes #881

[lpil]

Hello! I think this will now permit [[[[[[[[[[] as a host? I think the bug in the original code was to leave the [ on the host on line 205. Could we pop it off there instead?

[ahankinson]

Ah, ok, let me test this and will update.

[lpil]

Thank you! Good catch on finding the bug btw 💜

[ahankinson]

So I've added a few tests:

pub fn parse_ipv6_host_with_path_query_fragment_test() {
  let assert Ok(parsed) = uri.parse("http://[2001:db8::2:1]/foo/bar?baz=bif#blah")
  assert parsed.scheme == Some("http")
  assert parsed.host == Some("[2001:db8::2:1]")
  assert parsed.path == "/foo/bar"
  assert parsed.query == Some("baz=bif")
  assert parsed.fragment == Some("blah")
  assert parsed.port == None
  assert parsed.userinfo == None
}

pub fn parse_malformed_many_opening_brackets_in_host_test() {
  assert uri.parse("http://[[[[[[[[[[]/") == Error(Nil)
}

pub fn parse_malformed_nested_opening_bracket_in_host_test() {
  assert uri.parse("http://[[::1]/") == Error(Nil)
}

pub fn parse_malformed_unclosed_bracket_host_test() {
  assert uri.parse("http://[::1[/") == Error(Nil)
}

pub fn parse_malformed_question_mark_within_bracket_host_test() {
  assert uri.parse("http://[::1?bad]/") == Error(Nil)
}

pub fn parse_malformed_slash_within_bracket_host_test() {
  assert uri.parse("http://[::1/bad]/") == Error(Nil)
}

The question is what to do with malformed URIs. The current behaviour is actually to accept them!

"[" <> _ -> parse_host_within_brackets(uri_string, pieces) (L205) (as far as I can tell) doesn't pop the first bracket. So we can change this to "[" <> rest -> parse_host_within_brackets(rest, pieces).

But parse_host_within_brackets_loop (L229 & ff) doesn't reject the URI with an error if it doesn't detect an end ] before detecting other characters (e.g., /, ?, etc.). Currently, these are parsed as part of the host component, as far as I can tell?

The tests that are added are an attempt to try and determine the behaviour of the parsing. I have a patch that will also fix it and reject malformed IPv6 hosts, but I want to make sure I'm not missing something. So I will commit the tests and the patch separately so the patch can be reverted.

[ahankinson]

OK, two more commits added. I think there was a deeper bug with parsing IPv6 hosts within brackets; now, chars that are not strictly alphanumeric or : and . will raise an error. I couldn't quite determine what "other format" the comment was trying to communicate, since the only format in this function should be an IPv6 host within brackets, and other characters shouldn't be valid within those brackets.

[lupodevelop]

@ahankinson Looking at your implementation, you could also extend the tests to include "weird" cases like:

pub fn parse_ipv6_with_invalid_char_test() {
  // 'g' is not a hex digit
  assert uri.parse("http://[::g]/") == Error(Nil)
}

pub fn parse_bracket_followed_by_text_error_test() {
  // characters immediately after closing bracket and before slash should error
  assert uri.parse("http://[::1]foo") == Error(Nil)
}

pub fn parse_double_closing_bracket_test() {
  assert uri.parse("http://[::1]]/") == Error(Nil)
}

nothing transcendental, but since you're at it, they're fine 😆

---

EDIT: New little ideas

pub fn ipv6_uppercase_test() {
  // ensure A–F upper case are accepted
  let assert Ok(parsed) = uri.parse("http://[2001:DB8::1]")
  assert parsed.host == Some("[2001:DB8::1]")
  assert uri.to_string(parsed) == "http://[2001:DB8::1]/"
}

pub fn ipv6_mixedcase_test() {
  let assert Ok(parsed) = uri.parse("http://[2001:dB8:ABcd::]")
  assert parsed.host == Some("[2001:dB8:ABcd::]")

[ahankinson]

Oh good call! Will do that after lunch

[ahankinson]

OK; fixed

[lpil]

Super nice work! I've left one tiny request inline 🙏

[lpil]

Slice one character before and one character after rather than slicing and then appending please 🙏

[ahankinson]

I'm not quite sure if I've fixed it the way you were thinking. I've extended size by 1 to capture the closing bracket on original, but the opening bracket is already consumed (L205 -- [ <> rest, and rest is what is passed in to the loop.) So I've had to re-add it back in.

If I missed something obvious, though, please let me know.

[ahankinson]

I noticed that the extra character checks were now redundant with is_valid_host_within_brackets_char so I removed those case branches.