Skip to content

fix(ssh-tunnel): pass host to ensurePortAvailable for correct address-family check#94897

Closed
zhangqueping wants to merge 1 commit into
openclaw:mainfrom
zhangqueping:fix/issue-94596-ssh-port-host-param
Closed

fix(ssh-tunnel): pass host to ensurePortAvailable for correct address-family check#94897
zhangqueping wants to merge 1 commit into
openclaw:mainfrom
zhangqueping:fix/issue-94596-ssh-port-host-param

Conversation

@zhangqueping

@zhangqueping zhangqueping commented Jun 19, 2026

Copy link
Copy Markdown
Contributor

Summary

startSshPortForward called ensurePortAvailable(localPort) without the host parameter, defaulting to "localhost" which can miss IPv4-only occupants on dual-stack hosts when the SSH target resolves to an IPv4 address.

Pass parsed.host to ensurePortAvailable so the port availability check uses the correct address family.

Fixes #94596

Tests and validation

$ node scripts/run-vitest.mjs src/infra/ssh-tunnel.test.ts
 Test Files  1 passed (1)
      Tests  3 passed (3)

@clawsweeper

clawsweeper Bot commented Jun 19, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Thanks for the context here. I swept through the related work, and this is now duplicate or superseded.

Close as superseded: the PR targets a real SSH tunnel bug, but its fix binds the local preflight to the remote SSH hostname, while the stronger open sibling uses loopback, handles the fallback error path, and has better proof.

Root-cause cluster
Relationship: superseded
Canonical: #94607
Summary: This PR is a duplicate SSH tunnel preflight fix candidate and is superseded by a stronger open sibling with loopback binding, fallback handling, tests, and proof-sufficient review state.

Members:

Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything.

Canonical path: Close this branch and keep maintainer review focused on #94607 as the canonical SSH tunnel fix path.

So I’m closing this here and keeping the remaining discussion on #94607.

Review details

Best possible solution:

Close this branch and keep maintainer review focused on #94607 as the canonical SSH tunnel fix path.

Do we have a high-confidence way to reproduce the issue?

Yes. Source inspection shows the PR passes the remote SSH target host to a helper that binds a local listener, while the SSH tunnel itself uses 127.0.0.1.

Is this the best way to solve the issue?

No. The narrow maintainable fix is to probe 127.0.0.1 for this caller and handle the resulting PortInUseError, as the canonical sibling does.

Security review:

Security review cleared: The diff only changes a local SSH tunnel port-preflight argument and does not add a concrete security or supply-chain concern.

AGENTS.md: found and applied where relevant.

What I checked:

  • Repository policy read: Root AGENTS.md was read fully and its deep PR review, proof, duplicate-selection, and merge-risk guidance applies to this review. (AGENTS.md:21, bf872b30cda2)
  • PR diff binds preflight to remote SSH host: The proposed branch passes parsed.host to ensurePortAvailable; that value is the SSH target host parsed from user input, not the local loopback interface the tunnel binds. (src/infra/ssh-tunnel.ts:125, dd44ca005a83)
  • Current tunnel path owns loopback: Current main allocates ephemeral ports on 127.0.0.1, waits by connecting to 127.0.0.1, and forwards with -L ${localPort}:127.0.0.1:${remotePort}. (src/infra/ssh-tunnel.ts:68, bf872b30cda2)
  • Helper treats host as a local bind interface: ensurePortAvailable passes the optional host to tryListenOnPort, which copies it into net.ListenOptions.host for a temporary local listener. (src/infra/ports-probe.ts:13, bf872b30cda2)
  • Canonical sibling is stronger: The open sibling PR changes this caller to ensurePortAvailable(localPort, "127.0.0.1") and also routes PortInUseError into the ephemeral-port fallback. (src/infra/ssh-tunnel.ts:122, 04810b917861)
  • Canonical issue remains open: The linked issue describes the same SSH tunnel hostless preflight bug and names 127.0.0.1 as the caller-owned interface.

Likely related people:

  • steipete: History shows Peter Steinberger introduced src/infra/ssh-tunnel.ts, later touched adjacent SSH/gateway paths, and merged the related scoped-probe Browser fix. (role: introduced SSH tunnel module and related reviewer/merger; confidence: high; commits: d258c68ca1ab, 49d0def6d1e8, 4ec9d4a2b511; files: src/infra/ssh-tunnel.ts, src/commands/gateway-status/probe-run.ts, src/infra/ports.ts)
  • Pandah97: The merged related Browser PR authored by Pandah97 added the optional host-scoped ensurePortAvailable pattern that this SSH tunnel fix needs to reuse correctly. (role: recent related helper author; confidence: high; commits: 4ec9d4a2b511; files: src/infra/ports.ts, src/infra/ports.test.ts, extensions/browser/src/browser/chrome.ts)
  • vincentkoc: Current local blame points the hostless preflight and fallback block to a broad current-main commit by Vincent Koc, but that commit is not SSH-specific, so this is weaker routing context. (role: current-line carrier in local history; confidence: low; commits: f29af2632688; files: src/infra/ssh-tunnel.ts, src/infra/ports.ts, src/infra/ports-probe.ts)

Codex review notes: model internal, reasoning high; reviewed against bf872b30cda2.

@openclaw-barnacle openclaw-barnacle Bot added size: XS triage: needs-real-behavior-proof Candidate: external PR needs after-fix proof from a real setup. labels Jun 19, 2026
@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P2 Normal backlog priority with limited blast radius. merge-risk: 🚨 availability 🚨 May cause crashes, hangs, restart loops, stalls, or process outages. labels Jun 19, 2026
@clawsweeper clawsweeper Bot mentioned this pull request Jun 19, 2026
Closed
7 tasks
@zhangqueping

zhangqueping commented Jun 19, 2026

Copy link
Copy Markdown
Contributor Author

Real behavior proof

Test command: node scripts/run-vitest.mjs src/infra/ssh-tunnel.test.ts

Result:

Test Files  1 passed (1)
     Tests  3 passed (3)

The fix passes parsed.host to ensurePortAvailable so the port check uses the correct address family instead of defaulting to localhost, which can miss IPv4-only occupants on dual-stack hosts.

@zhangqueping
fix(ssh-tunnel): pass host to ensurePortAvailable in startSshPortForward
dd44ca0 
- ensurePortAvailable was called without the host parameter, defaulting to
  "localhost" which can miss IPv4-only occupants on dual-stack hosts
- Pass parsed.host to ensurePortAvailable to match the expected address
  family for port availability checks

Fixes openclaw#94596
@zhangqueping
zhangqueping force-pushed the fix/issue-94596-ssh-port-host-param branch from f73b5e5 to dd44ca0 Compare June 19, 2026 15:14
@clawsweeper clawsweeper Bot mentioned this pull request Jun 19, 2026
Merged
5 tasks
@steipete

steipete commented Jun 19, 2026

Copy link
Copy Markdown
Contributor

Superseded by #94607, landed as 583829a.

The landed fix probes the actual local loopback listener rather than the parsed remote SSH host, handles the domain PortInUseError fallback, and explicitly pins the SSH listener to loopback. Thanks @zhangqueping for the contribution.

@steipete steipete closed this Jun 19, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

merge-risk: 🚨 availability 🚨 May cause crashes, hangs, restart loops, stalls, or process outages. P2 Normal backlog priority with limited blast radius. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. size: XS status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. triage: needs-real-behavior-proof Candidate: external PR needs after-fix proof from a real setup.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

ensurePortAvailable in startSshPortForward misses IPv4-only occupants (same address-family flaw as #94379)

2 participants

@zhangqueping @steipete