fix: Allow tunnels to local port 80 - #188
Conversation
Creating or editing a tunnel whose local address used port 80 failed with "Failed to create tunnel". The tunnel service adds an http:// scheme to the address before parsing it with url::Url. For a URL whose port is the scheme's default, url::Url drops the port, so Url::port() returns None. parse_target then failed with "missing port" before any request reached the API. Other ports, including other privileged ones, were not affected. strip_scheme had the same flaw. For http://127.0.0.1:80 it returned the endpoint unchanged, so the local proxy state would have used "http://127.0.0.1" as the host. Both functions now use Url::port_or_known_default(), which returns 80 for http and 443 for https when the port matches the default. New unit tests cover port 80 and a non-default port. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Mcy6SZHB23ewuDivM6uAUi
ecv
changed the title
|
Both reviewers returned merge on this head. pr-adversary confirmed the root cause: url::Url::port() returns None for a scheme-default port, and parse_target and strip_scheme were the only two call sites reaching that gap, so the fix and its new test cover the actual failure. It checked that the desktop app's validate_tunnel_address already rejects a portless address before this code runs, so the fallback carries no side effect. It flagged one decision: nobody has run the manual create and edit port-80 tunnel test in the desktop app, only unit tests and CI cover it. The human already decided to proceed to ready and auto-merge, leaving the required human approval as the gate and the manual test for the approver. pr-conventions-reviewer confirmed this diff has no overlap with the other open pull requests touching lib/src/tunnels.rs, numbers 176, 158, and 96. I applied the three nit findings from the pass, all in the PR title and body: split the Summary into three short paragraphs running symptom, cause, and fix; folded the former Side effect section into the last paragraph of the Summary; and capitalized the title after the colon. None of the three needed a code change, so the branch carries no new commit. CI was green on the head. Release and update-homebrew skipped as expected on a non-release push. The bundle job, all three CI matrix jobs, the CLA check, and the project field automation passed. Auto-merge was enabled with merge, the only method the base branch's organization ruleset allows. That ruleset requires one approving review, a code-owner review, and approval of the last push, and dismisses stale reviews on every push. mattdjenkinson's approval already satisfied that gate, so the merge completed as soon as auto-merge was requested rather than waiting further. The two manual port-80 tunnel test-plan items are left unticked for whoever verifies them. |
2 participants
Summary
Tunnels to local port 80 could not be created or edited, while other ports worked.
The app adds "http://" to the address before parsing it, and the URL parser drops a port that matches the scheme's default, so the app saw no port and gave up.
It now falls back to the scheme's default port, so port 80 tunnels save and forward to the right local host. An address with no port still fails, since the app rejects it before this point, so this doesn't change what users see.
Test plan
Fixes #172
🤖 Generated with Claude Code
https://claude.ai/code/session_01Mcy6SZHB23ewuDivM6uAUi