Fix host parsing for IPv6 URI #1112
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1112 +/- ##
==========================================
+ Coverage 83.12% 83.14% +0.01%
==========================================
Files 56 56
Lines 5755 5760 +5
==========================================
+ Hits 4784 4789 +5
Misses 971 971 ☔ View full report in Codecov by Sentry. |
waahm7
changed the title
| @@ -509,7 +509,7 @@ static int s_test_uri_ipv6_parse(struct aws_allocator *allocator, void *ctx) { | |||
| aws_byte_cursor_from_c_str("[2001:db8:85a3:8d3:1319:8a2e:370:7348%25en0]:443"); | |||
| ASSERT_BIN_ARRAYS_EQUALS(expected_authority.ptr, expected_authority.len, uri.authority.ptr, uri.authority.len); | |||
|
|
|||
| struct aws_byte_cursor expected_host = aws_byte_cursor_from_c_str("[2001:db8:85a3:8d3:1319:8a2e:370:7348%25en0]"); | |||
There was a problem hiding this comment.
can you also verify, whether we should be stripping zone idx or not? what do other libs do?
source/uri.c
Outdated
| if (!port_delim) { | ||
| parser->uri->port = 0; | ||
| parser->uri->host_name = authority_parse_csr; | ||
| parser->uri->host_name.ptr = authority_parse_csr.ptr + host_name_offset; | ||
| parser->uri->host_name.len = authority_parse_csr.len - host_name_length_correction; |
There was a problem hiding this comment.
ugh, not a big fan of manual cursor manipulation?
assign authority_parse_csr to host and then advance and dec len by 1 to remove brackets?
There was a problem hiding this comment.
Thanks, I have updated the code to advance and decrement the length. Initially, I wrote the following code: 1127cad, but later refactored it to reduce code duplication. We can also refactor that version to use 'advance' and 'decrement length.' If you like that version better, I am conflicted between them.
There was a problem hiding this comment.
thanks. i think intent of what you are trying to do is much clearer in the latest version.
one further cleanup we could potentially do is to use aws_byte_cursor_next_split to separate host from port in authority. we can probably even get rid of memchr using that approach. but its a bigger refactoring, so we can do it in future
Issue #, if available:
Description of changes:
If it's IPv6 literal uri like
http://[::1]:80/path, the host is not[::1]and is::1only.RFC: https://datatracker.ietf.org/doc/html/rfc3986#section-3.2.2
urllib3:
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.