[client] Add excluded port range handling for PKCE flow#4853
Conversation
Introduce support for handling system-excluded port ranges in PKCE flow. Update port validation to skip excluded and in-use ports, including integration of Windows-specific logic to retrieve excluded ranges from the registry. Add tests for various usage scenarios across platforms.
WalkthroughAdds platform-aware port exclusion handling to the PKCE authentication flow. The code now obtains system-excluded TCP port ranges on Windows (via netsh), skips those ports when selecting redirect URLs, adds helpers and tests for port-range checks, and provides a non-Windows no-op variant. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Caller
participant PKCE as PKCE Flow
participant Sys as System (netsh / stub)
participant Net as Network
Caller->>PKCE: NewPKCEAuthorizationFlow(config)
PKCE->>Sys: getSystemExcludedPortRanges()
alt Windows build
Sys->>PKCE: run `netsh ... excludedportrange` output
PKCE->>PKCE: parseExcludedPortRanges(output)
else Non-Windows build
Sys-->>PKCE: return nil (no excluded ranges)
end
PKCE->>PKCE: iterate redirect URL candidates
PKCE->>PKCE: extract port from URL
PKCE->>PKCE: isPortInExcludedRange(port, excludedRanges)
alt Port excluded
PKCE-->>PKCE: warn & skip candidate
else Port not excluded
PKCE->>Net: check port reachability (is in use)
alt Port in use
PKCE-->>PKCE: skip candidate
else Port free
PKCE-->>Caller: select this redirect URL
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
client/internal/auth/pkce_flow_test.go (1)
165-218: Clarify intended behavior for malformed redirect URLs
TestIsRedirectURLPortUseddoes a nice job covering excluded ranges, actually-used ports (via a live listener), unused ports, and malformed URLs.One thing to double‑check: in the
"Invalid URL without port"case, the helper returnsfalse(i.e., “port not used”), which meansNewPKCEAuthorizationFlowwill happily select such a redirect URL even thoughWaitTokenlater expects a concrete port. If configs can ever contain malformed URLs, you might instead want to treat “no port / unparsable port” as unavailable and skip those entries early, to fail fast at configuration time rather than later at server startup.Would you like to tighten
isRedirectURLPortUsedto returntruewhenparsedURL.Port()is empty (or when parsing fails), and then adjust this test accordingly?client/internal/auth/pkce_flow.go (2)
287-314: Handle missing ports explicitly inisRedirectURLPortUsedThe overall logic—parse URL, short‑circuit on excluded ranges, then
DialTimeoutto detect an already‑in‑use port—is sound.However, when
parsedURL.Port()is empty,addr := ":"is passed tonet.DialTimeout, which will fail and cause the function to returnfalse(“port not used”). In combination withNewPKCEAuthorizationFlow, that means a redirect URL without an explicit port can be chosen and only later fail when starting the HTTP server.You may want to treat “no port present” as invalid/unusable and return
truein that case, logging at warn/debug level, e.g.:func isRedirectURLPortUsed(redirectURL string, excludedRanges []excludedPortRange) bool { parsedURL, err := url.Parse(redirectURL) if err != nil { log.Errorf("failed to parse redirect URL: %v", err) return true } - port := parsedURL.Port() + port := parsedURL.Port() + if port == "" { + log.Warnf("redirect URL %q has no explicit port; treating as unavailable", redirectURL) + return true + }This keeps misconfigured URLs from being selected and surfaces the issue earlier.
316-341: Port-range helper is correct; consider small robustness tweaks
excludedPortRangeplusisPortInExcludedRangeis straightforward and works as expected with the tests:
- Short‑circuit when
len(excludedRanges) == 0.- Safely ignore non‑numeric ports with a debug log.
- Inclusive comparison
start <= port <= endis intuitive for registry “start-end” specs.If you want to harden this further, you could optionally:
- Normalize/swallow obviously invalid ranges (e.g.,
start <= 0,end > 65535, orstart > end) either when building the slice or when checking.- Document explicitly in the comment that the range is inclusive on both ends.
These are nice‑to‑haves; the current implementation is functionally fine.
client/internal/auth/pkce_flow_test_windows.go (1)
17-137: Windows registry test is effective; be mindful of side effectsThe test does a solid job validating end‑to‑end behavior:
- It snapshots
ReservedPorts, overrides it with test ranges, and restores or deletes it afterward.- It uses live listeners to simulate “ports in use” and asserts that
NewPKCEAuthorizationFlowpicks the expected port or errors out when none are available.- The use of
errfromGetStringsValuein the restore defer is correct despite later shadowing inside other blocks.Two considerations:
- This test mutates a system‑wide HKLM key. Even though you restore it, it still requires admin rights and can briefly affect other software. Skipping on lack of privileges is good; you might also want to call this out in a comment so future maintainers understand the implications.
- For extra clarity, you could replace the
errcheck in the restore defer with an explicit boolean likehadReservedPorts := err != registry.ErrNotExistset right afterGetStringsValue, which would make the intent a bit easier to follow.Functionally, the test looks correct as written.
client/internal/auth/pkce_flow_windows.go (2)
14-22: Surface registry read failures at least at debug level
getSystemExcludedPortRangescurrently ignores errors fromgetExcludedPortRangesFromRegistryand just returnsranges(which will benilon error). That’s functionally acceptable—the flow simply falls back to “no excluded ranges”—but it makes debugging environment issues harder.Consider logging at debug (or info) when the registry read fails, and simplifying the function:
func getSystemExcludedPortRanges() []excludedPortRange { - ranges, err := getExcludedPortRangesFromRegistry() - if err == nil && len(ranges) > 0 { - return ranges - } - - return ranges + ranges, err := getExcludedPortRangesFromRegistry() + if err != nil { + log.Debugf("failed to read Windows ReservedPorts; proceeding without excluded ranges: %v", err) + return nil + } + return ranges }This preserves behavior while giving operators a clue when the system configuration can’t be read.
24-64: Registry parsing is straightforward; consider minor validation
getExcludedPortRangesFromRegistryis clean:
- Opens the key with
QUERY_VALUEonly and defers close with debug logging.- Reads
ReservedPortsas a string slice and parses"start-end"pairs with trimming.- Skips malformed entries or parse errors without failing the whole call.
Two optional robustness improvements:
- Treat
registry.ErrNotExistfromGetStringsValueas “no ranges configured” rather than an error, returning an empty slice instead of an error.- Optionally filter out ranges where
start > endor values are out of[0, 65535]to avoid bogus data if the registry is misconfigured.These are defensive edges; the core logic is solid.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
client/internal/auth/pkce_flow.go(4 hunks)client/internal/auth/pkce_flow_other.go(1 hunks)client/internal/auth/pkce_flow_test.go(2 hunks)client/internal/auth/pkce_flow_test_windows.go(1 hunks)client/internal/auth/pkce_flow_windows.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
client/internal/auth/pkce_flow_test.go (2)
shared/relay/client/dialer/quic/conn.go (1)
Addr(20-22)client/firewall/manager/port.go (1)
Port(10-16)
client/internal/auth/pkce_flow_test_windows.go (2)
client/internal/pkce_auth.go (1)
PKCEAuthProviderConfig(24-49)client/internal/auth/pkce_flow.go (1)
NewPKCEAuthorizationFlow(47-78)
🪛 GitHub Check: SonarCloud Code Analysis
client/internal/auth/pkce_flow_test_windows.go
[warning] 17-17: This function has 110 lines of code, which is greater than the 100 authorized. Split it into smaller functions.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (18)
- GitHub Check: Management / Unit (amd64, postgres)
- GitHub Check: Client / Unit (amd64)
- GitHub Check: Client / Unit (386)
- GitHub Check: Management / Unit (amd64, sqlite)
- GitHub Check: Client (Docker) / Unit
- GitHub Check: Management / Unit (amd64, mysql)
- GitHub Check: Relay / Unit (amd64, -race)
- GitHub Check: Relay / Unit (386)
- GitHub Check: Android / Build
- GitHub Check: Client / Unit
- GitHub Check: Darwin
- GitHub Check: release_ui_darwin
- GitHub Check: Linux
- GitHub Check: release
- GitHub Check: Windows
- GitHub Check: JS / Lint
- GitHub Check: Client / Unit
- GitHub Check: Client / Unit
🔇 Additional comments (3)
client/internal/auth/pkce_flow_other.go (1)
5-7: Non-Windows stub behavior is appropriateReturning
nilhere cleanly disables excluded-range checks on non-Windows, andisPortInExcludedRangecorrectly handleslen(nil) == 0, so semantics remain unchanged off Windows.client/internal/auth/pkce_flow_test.go (1)
76-163: Port-range tests are thorough and cover key edge casesThe table in
TestIsPortInExcludedRangeexercises in-range, boundary, out-of-range, empty/nil slice, multiple ranges, and invalid/empty port strings. This gives good confidence thatisPortInExcludedRangebehaves as intended for realistic inputs and failure modes.client/internal/auth/pkce_flow.go (1)
50-57: Port exclusion integration in flow constructor looks correctFetching
excludedRanges := getSystemExcludedPortRanges()once perNewPKCEAuthorizationFlowand passing them intoisRedirectURLPortUsedkeeps the selection loop simple and avoids repeatedly hitting the registry. The error path (availableRedirectURL == "") clearly reports misconfiguration when all configured URLs are unusable.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
client/internal/auth/pkce_flow_windows_test.go (3)
17-41: Be careful with mutating HKLM TCP/IP settings in a unit testThis test directly changes
HKLM\SYSTEM\CurrentControlSet\Services\Tcpip\Parameters\ReservedPorts, which is a global machine setting and requires elevated privileges. You do restore the value in a defer, but for the test duration you’re still altering system TCP/IP configuration.Consider guarding this as an explicit integration test (e.g., behind an env flag) so it doesn’t run on regular developer machines/CI by default, and documenting that it mutates
ReservedPortswhile running.
43-51: Avoid fully overwritingReservedPorts; append test ranges insteadRight now the test replaces the entire
ReservedPortsvalue withtestExcludedRanges, temporarily dropping any existing reserved ranges the system had configured. Even though you restore the original value afterward, during the test window other processes will see only the test ranges.It would be safer to:
- Read the existing list.
- Append your test ranges to it.
- Restore the original list in the defer.
This keeps the system’s reserved ports in place while still exercising port-exclusion behavior.
113-135: Table-driven coverage of excluded vs used ports looks solidThe table tests cleanly cover excluded ranges, multiple ranges, ports in use, and the “no ports available” error path, and they exercise the public
NewPKCEAuthorizationFlowAPI with realistic config values. Assertions on both error and selected redirect port make the behavior clear.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
client/internal/auth/pkce_flow_windows_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
client/internal/auth/pkce_flow_windows_test.go (3)
client/firewall/manager/port.go (1)
Port(10-16)client/internal/pkce_auth.go (1)
PKCEAuthProviderConfig(24-49)client/internal/auth/pkce_flow.go (1)
NewPKCEAuthorizationFlow(47-78)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (15)
- GitHub Check: Management / Unit (amd64, mysql)
- GitHub Check: Management / Unit (amd64, postgres)
- GitHub Check: Client / Unit (amd64)
- GitHub Check: Relay / Unit (amd64, -race)
- GitHub Check: Client / Unit (386)
- GitHub Check: Client (Docker) / Unit
- GitHub Check: Client / Unit
- GitHub Check: Windows
- GitHub Check: Darwin
- GitHub Check: Linux
- GitHub Check: Android / Build
- GitHub Check: Client / Unit
- GitHub Check: release
- GitHub Check: Client / Unit
- GitHub Check: JS / Lint
…ow on Windows Replace Windows registry-based logic with `netsh` command for retrieving excluded port ranges in PKCE flow. Refactor related parsing functions and update tests to reflect the new implementation.
|
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
client/internal/auth/pkce_flow_windows_test.go (1)
96-96: Hard-coded port can cause test flakiness.The past review comment correctly identifies that using a fixed port
65432can make this test flaky if that port is already in use on the test machine. The suggestion to dynamically allocate a free port (similar tousedPort1) and then close the listener before running the test is the right approach.
🧹 Nitpick comments (2)
client/internal/auth/pkce_flow_windows.go (2)
28-29: Consider adding a timeout for the netsh command.While
netshis a system command that typically executes quickly, adding a timeout context (e.g., 5-10 seconds) would prevent indefinite blocking in edge cases.You can apply this pattern:
func getExcludedPortRangesFromNetsh() ([]excludedPortRange, error) { - cmd := exec.Command("netsh", "interface", "ipv4", "show", "excludedportrange", "protocol=tcp") + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + cmd := exec.CommandContext(ctx, "netsh", "interface", "ipv4", "show", "excludedportrange", "protocol=tcp") output, err := cmd.Output()And add the
contextandtimeimports at the top of the file.
68-78: Consider validating port range ordering.While the system should provide valid ranges, adding a sanity check that
startPort <= endPortwould make the code more defensive and help detect unexpected netsh output format changes.endPort, err := strconv.Atoi(fields[1]) if err != nil { continue } + + if startPort > endPort { + continue + } ranges = append(ranges, excludedPortRange{start: startPort, end: endPort})
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
client/internal/auth/pkce_flow_windows.go(1 hunks)client/internal/auth/pkce_flow_windows_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
client/internal/auth/pkce_flow_windows_test.go (2)
client/internal/pkce_auth.go (1)
PKCEAuthProviderConfig(24-49)client/internal/auth/pkce_flow.go (1)
NewPKCEAuthorizationFlow(47-78)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (18)
- GitHub Check: Management / Unit (amd64, sqlite)
- GitHub Check: Management / Unit (amd64, postgres)
- GitHub Check: Relay / Unit (386)
- GitHub Check: Management / Unit (amd64, mysql)
- GitHub Check: Relay / Unit (amd64, -race)
- GitHub Check: Client / Unit (386)
- GitHub Check: Client / Unit (amd64)
- GitHub Check: Client (Docker) / Unit
- GitHub Check: release
- GitHub Check: release_ui_darwin
- GitHub Check: Android / Build
- GitHub Check: Windows
- GitHub Check: Linux
- GitHub Check: Darwin
- GitHub Check: Client / Unit
- GitHub Check: JS / Lint
- GitHub Check: Client / Unit
- GitHub Check: Client / Unit
🔇 Additional comments (2)
client/internal/auth/pkce_flow_windows_test.go (1)
16-83: Well-structured parsing tests.The table-driven tests cover multiple scenarios comprehensively (multiple ranges, empty output, single range) and use appropriate assertions.
client/internal/auth/pkce_flow_windows.go (1)
15-24: Graceful degradation on error is appropriate.Logging a debug message and returning
nilwhennetshfails allows the PKCE flow to proceed without port exclusions, which is a reasonable fallback for resilience.
2 participants
Describe your changes
Introduce support for handling system-excluded port ranges in PKCE flow. Update port validation to skip excluded and in-use ports, including integration of Windows-specific logic to retrieve excluded ranges from the registry. Add tests for various usage scenarios across platforms.
Issue ticket number and link
Stack
Checklist
Documentation
Select exactly one:
Docs PR URL (required if "docs added" is checked)
Paste the PR link from https://github.com/netbirdio/docs here:
https://github.com/netbirdio/docs/pull/__
Summary by CodeRabbit
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.