Skip to content

fix: don't dial a ProxyJump hop on the target's port - #337

Open
NatLee wants to merge 1 commit into
jeanp413:masterfrom
NatLee:fix/proxy-jump-port
Open

fix: don't dial a ProxyJump hop on the target's port#337
NatLee wants to merge 1 commit into
jeanp413:masterfrom
NatLee:fix/proxy-jump-port

Conversation

@NatLee

@NatLee NatLee commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

Split out of #334 at @GitMensch's suggestion, so the host key work stays purely about host keys.

The bug

The port of a ProxyJump hop was computed in two places, and they disagreed:

// connecting to the hop
const proxyPort = proxyHostConfig['Port'] ? parseInt(proxyHostConfig['Port'], 10) : (proxy.port || sshPort);
// forwardOut reaching the next hop
const destPort = nextProxyJump ? ((...) || nextProxyJump[0].port || 22) : sshPort;

With no Port on the hop, the first falls back to the target's port and the second to 22. A jump host doesn't inherit the target's Port, so a target declared on a non-default port had its first hop dialed on that port.

That's the first half of #109: Host te sets Port 11111, so its jump host was dialed on 11111 — matching the report that first and second connect but te doesn't. (Nested ProxyJump not being expanded, the other half of that report, is untouched here.)

Both sites now use one helper that falls back to 22, like ssh.

Tests

Proxies had no coverage at all, so this adds both levels:

  • test/proxy-jump-port.test.ts — 5 unit tests pinning the resolution order (hop Port > port in the ProxyJump value > 22). The last two fail if the fallback goes back to the target's port, so the regression is actually guarded.
  • test/proxy-jump.test.ts — e2e over two containers on a private network. Only the jump is published; the target is reachable solely by its container name inside the network, so connecting at all proves the hop was used.

Full suite passes locally (12 tests, 5 fixtures + forward-agent + the 6 added here).

One note on the e2e: container names become DNS labels on the network, so it uses a short suffix — a full UUID pushes the name past the 63 character limit and resolution fails.

Not covered

The default-to-22 path can't be exercised end to end without a jump host on port 22, which isn't reasonable to bind in CI, so that case is pinned by the unit tests instead.

The port of a jump host was computed in two places with different
fallbacks: connecting to the hop fell back to the target's port, while
the forwardOut reaching the next hop fell back to 22. A jump host does
not inherit the target's Port, so a target declared on a non-default
port had its first hop dialed on that port.

Use one helper for both, falling back to 22 like ssh does, and cover the
ProxyJump path with unit tests for the resolution order plus a two
container e2e where the target is only reachable through the hop.
@NatLee

NatLee commented Aug 25, 2026

Copy link
Copy Markdown
Contributor Author

CI red is npm audit on nanoid (GHSA-2v37-7h3g-55p8), same repo-wide pattern as the brace-expansion one — tests and lint pass, and this branch doesn't touch package.json/package-lock.json.

@GitMensch GitMensch left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants