External SSH port #215

New issue

Closed

opened 2026-09-08 20:04:48 +00:00 by crueber · 3 comments

crueber commented

2026-09-08 20:04:48 +00:00

Owner

The external SSH port should be something that can be setup in the setup section.

This is what it looks like right now:

The port needs to be 12222 externally, but internally it is still 2222.

The external SSH port should be something that can be setup in the setup section. This is what it looks like right now: ![image](/attachments/980e585e-f105-400d-88d8-14b3dd3ebf65) The port needs to be 12222 externally, but internally it is still 2222.

image.png

16 KiB

crueber referenced this issue from a commit

2026-09-09 00:04:34 +00:00

Fix #215: external SSH port config (17_ssh.md §3, decision 17.5)

crueber referenced this issue from a pull request that will close it,

2026-09-09 00:04:40 +00:00

Fix #215: external SSH port config #222

crueber commented

2026-09-09 00:04:45 +00:00

Author

Owner

Fixed by PR #222 (branch fix/issue-215): new server.ssh.external_port setup key (0 = listen port) advertised as ssh_clone_url; clone menu SSH tab shows the external port (verified live in Chromium: ssh://git@host:12222/…, zero console errors).

Fixed by PR #222 (branch fix/issue-215): new `server.ssh.external_port` setup key (0 = listen port) advertised as `ssh_clone_url`; clone menu SSH tab shows the external port (verified live in Chromium: `ssh://git@host:12222/…`, zero console errors).

crueber commented

2026-09-09 00:30:13 +00:00

Author

Owner

Review of PR #222 (fix/issue-215, +4bc4279 doc fix) — verified in scratch worktree, main untouched.

Verdict: ready to merge (one doc gap found and fixed by me, pushed to the branch).

Review dimensions:

Default behavior (0 = listen port): CORRECT. Old code dropped the port entirely (BEFORE shot: bare ssh://git@host/…, implicit :22). New code advertises the listen port when no external override is set. The only behavior change vs old is listen-non-22-without-override (was broken — cloned against :22 — now works). Listen :22 omits the port (identical to old); SSH-disabled advertises nothing and the UI falls back to the old port-less derivation (identical). internal/config/validate.go:59, internal/api/refs.go:64. Nit: PR blurb 'unset behavior is identical' is slightly overstated — it is identical except the previously-broken case, which is the fix direction. Not a blocker.
Hostname source (public_url-or-Host): same treatment as clone_url. sshCloneURL derives the host from baseURL (refs.go:38), the exact source the https clone_url uses. No new spoofing surface — Host-header input was already trusted for display-only clone URLs.
:22 omission + IPv6: correct. net.JoinHostPort brackets v6 when a port is shown; the :22-omission path manually brackets a colon-containing host (refs.go:79-84). Host comes from url.Hostname() (brackets already stripped), so no double-bracketing.
Validation + fail-closed: correct. checkSSH rejects out-of-range ports (validate.go:48); AdvertisedSSHPort clamps invalid to 0 = omit advertisement. Setup save 422s on bad input (covered by TestSetupSSHExternalPortRoundTrip).
requires_restart: correct to list. No hot-reload (setup_api.go:561-562); advertisement reads boot-loaded config per request, so restart is genuinely needed. Test asserts the key is listed.
Env overlay verified: WALHUB__SERVER__SSH__EXTERNAL_PORT=12222 applies via the generic path (throwaway test passed, then removed).
Setup example both sides: client FIELDS ex 12222 covered by the 'every example validates' loop; server /setup/test accepts 12222/2222 and 422s 70000/-1/many (round-trip test). UI min:1/max:65535 rejects 0; empty = unset = listen port, consistent with the note.
UI fallback correct: sshCloneUrlFrom prefers advertisement verbatim, falls back to the old hostname derivation (clone.js:49-53). EmptyRepoGuide uses the same helper — consistent.
Coverage: config 95.4%, api 95.3%, server 95.6% (all ≥95% gate). No new non-stdlib imports (net/net/url/strconv only).
Docs: 17_ssh §3 + decision 17.5, 11_config_cli table + rule 10, 07_api summary/overview, 12_web_ui amendment all accurate. Gap I fixed: 07_api.md:439 create-body enumeration omitted ssh_clone_url? although writeCreateJSON emits it and its comment claims all fields are enumerated there (+4bc4279).

Test results (scratch worktree): go test -race config/api/server all pass; node --test web/test/unit/*.test.js 428 pass / 0 fail / 3 skipped (smoke skips — needs a live server; note: an ambient :8080 on this box makes smoke run live and its undici keep-alive hangs the runner, environmental, unrelated); gofmt/vet clean. Two environmental scaffolds used for the run only (copied built web/dist + public/concepts gifs, symlinked main node_modules); none committed.

Resolution: doc fix pushed to origin/fix/issue-215. No code changes needed. Do NOT merge from my side per instructions — over to you.

Review of PR #222 (fix/issue-215, +4bc4279 doc fix) — verified in scratch worktree, main untouched. **Verdict: ready to merge** (one doc gap found and fixed by me, pushed to the branch). **Review dimensions:** - **Default behavior (0 = listen port): CORRECT.** Old code dropped the port entirely (BEFORE shot: bare `ssh://git@host/…`, implicit :22). New code advertises the listen port when no external override is set. The only behavior change vs old is listen-non-22-without-override (was broken — cloned against :22 — now works). Listen :22 omits the port (identical to old); SSH-disabled advertises nothing and the UI falls back to the old port-less derivation (identical). `internal/config/validate.go:59`, `internal/api/refs.go:64`. Nit: PR blurb 'unset behavior is identical' is slightly overstated — it is identical except the previously-broken case, which is the fix direction. Not a blocker. - **Hostname source (public_url-or-Host): same treatment as clone_url.** `sshCloneURL` derives the host from `baseURL` (`refs.go:38`), the exact source the https `clone_url` uses. No new spoofing surface — Host-header input was already trusted for display-only clone URLs. - **:22 omission + IPv6: correct.** `net.JoinHostPort` brackets v6 when a port is shown; the :22-omission path manually brackets a colon-containing host (`refs.go:79-84`). Host comes from `url.Hostname()` (brackets already stripped), so no double-bracketing. - **Validation + fail-closed: correct.** `checkSSH` rejects out-of-range ports (`validate.go:48`); `AdvertisedSSHPort` clamps invalid to 0 = omit advertisement. Setup save 422s on bad input (covered by `TestSetupSSHExternalPortRoundTrip`). - **requires_restart: correct to list.** No hot-reload (`setup_api.go:561-562`); advertisement reads boot-loaded config per request, so restart is genuinely needed. Test asserts the key is listed. - **Env overlay verified:** `WALHUB__SERVER__SSH__EXTERNAL_PORT=12222` applies via the generic path (throwaway test passed, then removed). - **Setup example both sides:** client `FIELDS` ex `12222` covered by the 'every example validates' loop; server `/setup/test` accepts 12222/2222 and 422s 70000/-1/many (round-trip test). UI `min:1/max:65535` rejects `0`; empty = unset = listen port, consistent with the note. - **UI fallback correct:** `sshCloneUrlFrom` prefers advertisement verbatim, falls back to the old hostname derivation (`clone.js:49-53`). EmptyRepoGuide uses the same helper — consistent. - **Coverage:** config 95.4%, api 95.3%, server 95.6% (all ≥95% gate). No new non-stdlib imports (net/net/url/strconv only). - **Docs:** 17_ssh §3 + decision 17.5, 11_config_cli table + rule 10, 07_api summary/overview, 12_web_ui amendment all accurate. **Gap I fixed:** 07_api.md:439 create-body enumeration omitted `ssh_clone_url?` although writeCreateJSON emits it and its comment claims all fields are enumerated there (+4bc4279). **Test results (scratch worktree):** `go test -race` config/api/server all pass; `node --test web/test/unit/*.test.js` 428 pass / 0 fail / 3 skipped (smoke skips — needs a live server; note: an ambient :8080 on this box makes smoke run live and its undici keep-alive hangs the runner, environmental, unrelated); gofmt/vet clean. Two environmental scaffolds used for the run only (copied built web/dist + public/concepts gifs, symlinked main node_modules); none committed. **Resolution:** doc fix pushed to `origin/fix/issue-215`. No code changes needed. Do NOT merge from my side per instructions — over to you.

crueber closed this issue

2026-09-09 00:30:40 +00:00

crueber commented

2026-09-09 00:30:40 +00:00

Author

Owner

Fixed by PR #222 incl. review doc fix (advertised port with correct defaults; 95.4/95.3/95.6% + 428 node tests), merged. Closing.

crueber referenced this issue from a commit

2026-09-09 00:30:41 +00:00

Merge pull request 'Fix #215: external SSH port config' (#222) from fix/issue-215 into main

crueber referenced this issue

2026-09-10 13:58:09 +00:00