[client, management, relay, misc] Use net.JoinHostPort for IPv6-safe host:port handling#5836
Merged
lixmal merged 1 commit intoproto-ipv6-overlayfrom Apr 10, 2026
Merged
Conversation
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
lixmal
force-pushed
the
client-ipv6-hostport-fixes
branch
from
34a8682 to
331dfca
Compare
lixmal
force-pushed
the
client-ipv6-hostport-fixes
branch
from
331dfca to
12752b3
Compare
|
pappz
approved these changes
Apr 9, 2026
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
client/ssh/server/server.go (1)
922-934: Consider reusing a singlehostPortvalue across all branches.This removes repeated formatting and keeps log output construction consistent in one place.
♻️ Suggested refactor
+ hostPort := net.JoinHostPort(payload.Host, strconv.Itoa(int(payload.Port))) if !allowLocal { - logger.Warnf("local port forwarding denied for %s: disabled", net.JoinHostPort(payload.Host, strconv.Itoa(int(payload.Port)))) + logger.Warnf("local port forwarding denied for %s: disabled", hostPort) _ = newChan.Reject(cryptossh.Prohibited, "local port forwarding disabled") return } if err := s.checkPortForwardingPrivileges(ctx, "local", payload.Port); err != nil { - logger.Warnf("local port forwarding denied for %s: %v", net.JoinHostPort(payload.Host, strconv.Itoa(int(payload.Port))), err) + logger.Warnf("local port forwarding denied for %s: %v", hostPort, err) _ = newChan.Reject(cryptossh.Prohibited, "insufficient privileges") return } - hostPort := net.JoinHostPort(payload.Host, strconv.Itoa(int(payload.Port))) forwardAddr := "-L " + hostPort🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/ssh/server/server.go` around lines 922 - 934, Extract hostPort := net.JoinHostPort(payload.Host, strconv.Itoa(int(payload.Port))) before the permission checks and reuse it in the subsequent log messages, Reject calls, and when building forwardAddr (replace the inline net.JoinHostPort(...) and the later hostPort assignment); update logger.Warnf lines and forwardAddr := "-L "+hostPort to use this single hostPort variable and keep formatting consistent.management/server/http/handlers/dns/nameservers_handler_test.go (1)
250-256: Add a bracketed IPv6 test case for full regression coverage.Current IPv6 coverage validates unbracketed input only; please add a case like
"[2001:4860:4860::8888]"to explicitly guard bracket-handling behavior too.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@management/server/http/handlers/dns/nameservers_handler_test.go` around lines 250 - 256, Add a new test case in nameservers_handler_test.go alongside the existing IPv6 case that uses a bracketed IPv6 string to validate bracket handling: add a struct with name "IPv6 (bracketed)" and input []api.Nameserver{{Ip: "[2001:4860:4860::8888]", NsType: "udp", Port: 53}} and set expectIP to netip.MustParseAddr("2001:4860:4860::8888") so the same parsing assertion used by the existing IPv6 test verifies bracket stripping; place it near the existing IPv6 test entry to ensure full regression coverage.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@client/ssh/server/port_forwarding.go`:
- Around line 315-316: The listener is being stored under the requested port
(payload.Port) which can be zero; change storeRemoteForwardListener to use the
actual allocated port instead so cancel-tcpip-forward lookups match—construct
the forward key with forwardKey(net.JoinHostPort(payload.Host,
strconv.Itoa(int(actualPort)))) (using the actualPort returned/assigned by the
listener) and call s.storeRemoteForwardListener with that key and ln, ensuring
any later cancel handling that derives keys from the allocated port will find
the stored listener.
---
Nitpick comments:
In `@client/ssh/server/server.go`:
- Around line 922-934: Extract hostPort := net.JoinHostPort(payload.Host,
strconv.Itoa(int(payload.Port))) before the permission checks and reuse it in
the subsequent log messages, Reject calls, and when building forwardAddr
(replace the inline net.JoinHostPort(...) and the later hostPort assignment);
update logger.Warnf lines and forwardAddr := "-L "+hostPort to use this single
hostPort variable and keep formatting consistent.
In `@management/server/http/handlers/dns/nameservers_handler_test.go`:
- Around line 250-256: Add a new test case in nameservers_handler_test.go
alongside the existing IPv6 case that uses a bracketed IPv6 string to validate
bracket handling: add a struct with name "IPv6 (bracketed)" and input
[]api.Nameserver{{Ip: "[2001:4860:4860::8888]", NsType: "udp", Port: 53}} and
set expectIP to netip.MustParseAddr("2001:4860:4860::8888") so the same parsing
assertion used by the existing IPv6 test verifies bracket stripping; place it
near the existing IPv6 test entry to ensure full regression coverage.
🪄 Autofix (Beta)
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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ae3aad38-519a-4e5c-b6eb-5f9a80e28af9
📒 Files selected for processing (21)
client/anonymize/anonymize.goclient/anonymize/anonymize_test.goclient/cmd/ssh.goclient/firewall/uspfilter/conntrack/common_test.goclient/firewall/uspfilter/conntrack/icmp.goclient/firewall/uspfilter/conntrack/icmp_test.goclient/firewall/uspfilter/tracer.goclient/internal/profilemanager/config.goclient/internal/relay/relay.goclient/internal/rosenpass/manager.goclient/internal/rosenpass/manager_test.goclient/ssh/proxy/proxy.goclient/ssh/server/port_forwarding.goclient/ssh/server/server.gocombined/cmd/config.gomanagement/internals/modules/reverseproxy/service/manager/manager.gomanagement/server/http/handlers/dns/nameservers_handler.gomanagement/server/http/handlers/dns/nameservers_handler_test.gorelay/test/benchmark_test.gorelay/testec2/turn_allocator.goupload-server/server/s3_test.go
Comment on lines
+315
to
316
| key := forwardKey(net.JoinHostPort(payload.Host, strconv.Itoa(int(payload.Port)))) | ||
| s.storeRemoteForwardListener(key, ln) |
Contributor
There was a problem hiding this comment.
Use the allocated port for forwardKey to keep cancel lookups consistent.
When remote forwarding requests port 0, the server allocates actualPort. Storing the listener under payload.Port (0) can break cancel-tcpip-forward lookup when cancellation uses the allocated port.
🐛 Proposed fix
- key := forwardKey(net.JoinHostPort(payload.Host, strconv.Itoa(int(payload.Port))))
+ key := forwardKey(net.JoinHostPort(payload.Host, strconv.Itoa(int(actualPort))))
s.storeRemoteForwardListener(key, ln)📝 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.
Suggested change
| key := forwardKey(net.JoinHostPort(payload.Host, strconv.Itoa(int(payload.Port)))) | |
| s.storeRemoteForwardListener(key, ln) | |
| key := forwardKey(net.JoinHostPort(payload.Host, strconv.Itoa(int(actualPort)))) | |
| s.storeRemoteForwardListener(key, ln) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@client/ssh/server/port_forwarding.go` around lines 315 - 316, The listener is
being stored under the requested port (payload.Port) which can be zero; change
storeRemoteForwardListener to use the actual allocated port instead so
cancel-tcpip-forward lookups match—construct the forward key with
forwardKey(net.JoinHostPort(payload.Host, strconv.Itoa(int(actualPort)))) (using
the actualPort returned/assigned by the listener) and call
s.storeRemoteForwardListener with that key and ln, ensuring any later cancel
handling that derives keys from the allocated port will find the stored
listener.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Describe your changes
Replace
fmt.Sprintf("%s:%d", host, port)andstrings.Split(addr, ":")withnet.JoinHostPort/net.SplitHostPortacross the codebase. Without this, IPv6 addresses produce malformed strings like2001:db8::1:3478instead of[2001:db8::1]:3478.net.JoinHostPortin SSH port forwarding, relay TURN probing, DNS nameserver URL parsing, STUN URI construction, management URL construction, reverse proxy service URLs, and packet tracer displaystrings.Spliton:withnet.SplitHostPortin rosenpass UDP port parsingnet.JoinHostPortso bracketed IPv6 addresses are preserved throughAnonymizeStringConnKey.String(),ICMPConnKey.String(),findRandomAvailableUDPPort,toServerNSListwith IPv6, andAnonymizeStringwith IPv6 STUN/HTTPS URIsIssue ticket number and link
Stack
Checklist
Documentation
Select exactly one:
Summary by CodeRabbit
Bug Fixes
Tests