fix: parse IPv6 hosts correctly in AnalyzeURL - #354
Conversation
AnalyzeURL prepends http:// to any input that has no scheme, then hands it to url.Parse. For a bare IPv6 host this is wrong, because url.Parse needs the host inside brackets to tell it apart from the colons used as the port separator. Without brackets, an address like ::1/health was parsed as host ':' and port '1', instead of host '::1' with no port. This adds a step before the http:// prefix is added: look at the authority part of the input (everything before the first slash), and if it parses as an IPv6 address, wrap it in brackets first. If the authority splits on its last colon into an IPv6 host and an all-digit port, only the host part gets bracketed, so the port is preserved. Anything that is not IPv6, or is already bracketed, is left alone. Since the parsed host is discarded later and only the port and path end up in the output, the test asserts on that output for a set of IPv6 and regression cases, matching the case table from the issue. Fixes AC-3 in kubescape#334 Signed-off-by: Arnesh Banerjee <linkrinku13@gmail.com>
📝 WalkthroughWalkthroughChangesIPv6 URL normalization
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related issues
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/registry/file/dynamicpathdetector/analyze_endpoints_internal_test.go`:
- Around line 22-30: Add the missing “bare_ipv6_with_port” test case to the
endpoint analysis table, using an unbracketed IPv6 host with port 65535 and
expecting “:65535/x”. Keep it alongside the existing IPv6 cases so it exercises
the fallback in addBracketsIfIPv6.
In `@pkg/registry/file/dynamicpathdetector/analyze_endpoints.go`:
- Around line 114-116: Update the authority-splitting logic in the endpoint
analysis flow around strings.IndexByte so it finds the earliest delimiter among
'/', '?', and '#', preserving the remaining suffix for parsing. Add a regression
test covering an IPv6 address followed by a query, such as 2001:db8::1?probe=1,
and verify the trailing IPv6 group is not interpreted as a port.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 3eb676f5-020d-4ae0-904c-57c852afb63d
📒 Files selected for processing (2)
pkg/registry/file/dynamicpathdetector/analyze_endpoints.gopkg/registry/file/dynamicpathdetector/analyze_endpoints_internal_test.go
| {"bare_loopback", "::1/health", ":/health"}, | ||
| {"bare_ipv6_no_port", "2001:db8::1/health", ":/health"}, | ||
| {"trailing_hextet_not_a_port", "2001:db8::8080/x", ":/x"}, | ||
| {"already_bracketed_with_port", "[2001:db8::1]:8080/x", ":8080/x"}, | ||
| {"unbracketed_ipv6_with_trailing_group", "2001:db8::1:8080/x", ":/x"}, | ||
| {"ipv4_host_with_port_regression", "example.com:80/users/123", ":80/users/123"}, | ||
| {"canonical_form_regression", ":80/users/123", ":80/users/123"}, | ||
| {"scheme_pass_through_regression", "http://example.com:80/x", ":80/x"}, | ||
| {"ipv4_with_port_regression", "192.168.1.1:8080/path", ":8080/path"}, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Cover the unbracketed IPv6 host-port fallback.
The current port test is already bracketed, so it exits addBracketsIfIPv6 before lines 127-132. Add {"bare_ipv6_with_port", "2001:db8::1:65535/x", ":65535/x"}. This input exercises the new fallback because 65535 cannot be an IPv6 hextet but is a valid numeric port.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/registry/file/dynamicpathdetector/analyze_endpoints_internal_test.go`
around lines 22 - 30, Add the missing “bare_ipv6_with_port” test case to the
endpoint analysis table, using an unbracketed IPv6 host with port 65535 and
expecting “:65535/x”. Keep it alongside the existing IPv6 cases so it exercises
the fallback in addBracketsIfIPv6.
| if idx := strings.IndexByte(urlString, '/'); idx >= 0 { | ||
| authority = urlString[:idx] | ||
| rest = urlString[idx:] |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Split the authority before a query string.
Line 114 only detects / as the end of the authority. For 2001:db8::1?probe=1, authority includes ?probe=1, so the IPv6 host is not bracketed before parsing. Parse the authority before /, ?, or #. Add a regression test that confirms the IPv6 trailing group does not become a port.
Proposed fix
- if idx := strings.IndexByte(urlString, '/'); idx >= 0 {
+ if idx := strings.IndexAny(urlString, "/?#"); idx >= 0 {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if idx := strings.IndexByte(urlString, '/'); idx >= 0 { | |
| authority = urlString[:idx] | |
| rest = urlString[idx:] | |
| if idx := strings.IndexAny(urlString, "/?#"); idx >= 0 { | |
| authority = urlString[:idx] | |
| rest = urlString[idx:] |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/registry/file/dynamicpathdetector/analyze_endpoints.go` around lines 114
- 116, Update the authority-splitting logic in the endpoint analysis flow around
strings.IndexByte so it finds the earliest delimiter among '/', '?', and '#',
preserving the remaining suffix for parsing. Add a regression test covering an
IPv6 address followed by a query, such as 2001:db8::1?probe=1, and verify the
trailing IPv6 group is not interpreted as a port.
matthyx
left a comment
matthyx
left a comment
There was a problem hiding this comment.
Reviewed against AC-3 of #334. No blockers — approving. All nine table rows from the issue are implemented and asserted, and the fix is well scoped.
What I verified locally (branch head 8845959, base 72c61d4)
go build ./...,go vet,gofmt -lclean;go test ./pkg/registry/file/...green (5 packages).- Differential sweep, 1064 inputs (19 authorities × 7 port forms × 8 path/query/fragment suffixes), base vs PR head:
- 182 outputs changed — every one of them an IPv6 authority. Zero changes for hostnames, IPv4, already-bracketed hosts, or the canonical
:<port><path>form, so the "must not change IPv4/hostname behavior" constraint holds. - Zero success→error transitions — no endpoint that used to parse is now dropped by
AnalyzeEndpoints/ProcessEndpoint. 30 error→success improvements, e.g.::1:abc/health(a genuinely valid IPv6 address) went frominvalid port ":abc"to:/health. - Zone IDs (
fe80::1%eth0) still error before and after, only with a different message — matches the "declared unsupported" call in the issue.
- 182 outputs changed — every one of them an IPv6 authority. Zero changes for hostnames, IPv4, already-bracketed hosts, or the canonical
Non-blocking comments
1. ? and # don't terminate the authority (same as CodeRabbit's inline note). strings.IndexByte(urlString, '/') misses query/fragment-only inputs, so 2001:db8::1?probe=1 still yields :1. — the exact bug this PR fixes, just with a ? instead of a /. It is not a regression (base does the same), so it doesn't block, but the one-liner is free:
if idx := strings.IndexAny(urlString, "/?#"); idx >= 0 {I applied it locally: 2001:db8::1?probe=1 → :. and the whole package suite stays green.
2. The [host]:port fallback is untested and currently unobservable. Two checks:
- Mutation: deleting the entire
strings.LastIndexbranch andisAllDigitsleaves the full package suite passing — no test pins it. (The case CodeRabbit suggests,2001:db8::1:65535/x→:65535/x, is worth adding, but note it passes with or without the branch, so it documents the contract rather than pinning the code.) - Differential: for every input I could construct that reaches the branch —
2001:db8::1:65535/x,::1:65535/x,0:0:0:0:0:0:0:1:8080/x,fe80::abcd:12345/x,::ffff:192.168.1.1:8080/x,2001:db8:85a3::8a2e:370:7334:65000/api/v1— the output is identical with and without it.url.Parseis lenient enough to split the last colon into the same port on the unbracketed string, and the host is discarded downstream.
So it's currently dead weight in output terms. I'd keep it (it makes the parse RFC-correct instead of leaning on url.Parse leniency, and it's what the issue specified), but a one-line comment saying it exists for parse correctness rather than for a different :<port><path> result would stop the next reader from hunting for the behavior difference.
3. Heads-up, not a change request: on IPv6 clusters, endpoints already recorded as ::1:8080/x will re-key once from :8080/x to :/x, and ::1:0/x stops being treated as the :0 wildcard port. That is exactly the RFC 3986 call made in row 5 of the issue table, just worth knowing before it lands.
Also pre-existing and out of scope: a scheme-less input with no path (::1, example.com) produces the odd :. output from AnalyzePath("") — unchanged by this PR.
|
Thanks! |
2 participants
Part of #334 (AC-3)
AnalyzeURL adds http:// to any input that has no scheme, then hands it to url.Parse. For a bare IPv6 host this is wrong, because url.Parse needs the host inside brackets to tell it apart from the colons used as the port separator. Without brackets, something like ::1/health was parsed as host ":" and port "1", instead of host "::1" with no port at all.
This adds a step before the http:// prefix goes on: look at the authority part of the input, meaning everything before the first slash, and if it parses as an IPv6 address, wrap it in brackets. If the authority splits on its last colon into an IPv6 host and an all digit port, only the host part gets bracketed, so the port is kept. Anything that is not IPv6, or is already bracketed, is left alone.
The parsed host itself is discarded later in AnalyzeURL, only the port and path end up in the output, so the test asserts on that output. I used the same set of cases listed in the issue, IPv6 with and without a port, an IPv6 address with a trailing all digit group that looks like a port but isn't, and a few IPv4 and hostname cases to check nothing there changed.
Ran go test on the package and gofmt, both clean.
Summary by CodeRabbit