Skip to content

fix(api): rewrite SSH tunnel HTTP requests over raw duplex stream - #322

Merged
Hydralerne merged 1 commit into
oblien:mainfrom
DiogoDuart3:fix/ssh-tunnel-http-over-duplex
Jul 31, 2026
Merged

fix(api): rewrite SSH tunnel HTTP requests over raw duplex stream#322
Hydralerne merged 1 commit into
oblien:mainfrom
DiogoDuart3:fix/ssh-tunnel-http-over-duplex

Conversation

@DiogoDuart3

Copy link
Copy Markdown
Contributor

What / why

tunnelRequest() in apps/api/src/lib/ssh-tunnel.ts used Node's http.request()
with a custom createConnection that returned the SSH tunnel's duplex stream
directly, instead of opening a real TCP socket.

Root cause

Under Bun's node:http polyfill, a caller-supplied createConnection is not
honored — http.request() still attempts to open its own connection under the
hood regardless of what createConnection returns. Every request made through
this path failed with ECONNREFUSED, which the existing req.on("error", ...)
handler quietly resolved to null.

This was caught on a real self-hosted install: analytics scraping for a remote
server reached over an SSH-tunnel executor consistently returned zero data,
even though the target service was healthy and reachable by every other check.
Tracing tunnelRequest() directly (bypassing the higher-level analytics code)
reproduced the ECONNREFUSED immediately.

Fix

Write the HTTP/1.1 request directly onto the tunnel's raw duplex stream and
parse the response by hand — status line, headers, Content-Length/chunked
transfer-encoding body framing, and a connection-close fallback for responses
with neither. This mirrors the approach this same file's tunnelStream()
already uses for streaming responses, so the file is now internally
consistent and no longer depends on node:http understanding a
non-standard socket.

Test plan

  • tsc --noEmit on apps/api — clean.
  • Verified directly against a live self-hosted install: before the fix,
    tunnelRequest() against a real SSH-tunneled service returned null on
    every call; after the fix, it returns the correct status/headers/body, and
    analytics scraping for that server started reporting real data again.
  • No existing unit test file covers this module; the fix is a drop-in
    replacement of the request/response mechanics with the same public
    signature (tunnelRequest(...)) and return type, so no callers change.

tunnelRequest() used Node's http.request() with a custom createConnection
returning the SSH tunnel's duplex stream. Under Bun's node:http polyfill,
createConnection is not honored — http.request() still attempts its own
socket connection underneath, so every call through this path failed with
ECONNREFUSED and was silently swallowed to null by the existing error
handler.

This broke analytics scraping on a self-hosted install using the SSH-tunnel
executor path: every request made through tunnelRequest() came back empty,
so analytics showed zero data even though the remote service was healthy
and reachable.

Fix: write the HTTP/1.1 request directly onto the tunnel's raw duplex and
parse the response by hand (status line, headers, Content-Length/chunked
body framing, connection-close fallback) — mirroring the same approach this
file's tunnelStream() already used for streaming responses. No more
dependency on node:http understanding a foreign socket.
@DiogoDuart3

Copy link
Copy Markdown
Contributor Author

The failing Test check here is a pre-existing failure on main's own tip (currently 6a677363, e.g. run 30536159910) — @repo/dashboard#test fails identically with zero changes from this PR. Typecheck passes. None of this PR's files touch apps/dashboard.

@Hydralerne

Copy link
Copy Markdown
Member

Confirmed the root cause independently: bun 1.3.3 ignores createConnection → ECONNREFUSED, node 22 honours it → 200. Good find, and raw HTTP on the duplex is the right fix.

Two things to fix first — both re-create the silent-no-data symptom this PR is fixing:

  1. transfer-encoding value isn't lowercased. Transfer-Encoding: Chunked and gzip, chunked both miss === "chunked". With no Content-Length it then falls to the close handler and returns raw chunk-framed bytes (1a4\r\n{…}) as the body with status 200 → the caller's JSON.parse throws.

const te = String(parsedHeaders["transfer-encoding"] ?? "").toLowerCase();
const isChunked = te.split(",").some((t) => t.trim() === "chunked");

  1. The close fallback resolves success for a truncated response. If chunked never reached the 0 chunk, or Content-Length wasn't satisfied, it still returns a partial body as 200 — http.request surfaced that as an error → null. Hoist contentLength/isChunked out of the data handler and gate it:

if (headerEnd !== -1 && contentLength === null && !isChunked) finish({ … });
else finish(null);

  1. (fine as a follow-up) This adds a second hand-rolled head parser — tunnelStream:188 has the same logic but with buffer += chunk.toString(), which corrupts multi-byte UTF-8 across chunk boundaries. One shared parseHttpHead() dedupes both and fixes that.

Nits: caller headers are appended after the computed ones, so a caller-passed Content-Length duplicates; no CRLF validation now that http.request isn't doing it (not reachable today — all callers encodeURIComponent); the comment mentions an end listener that isn't registered; a malformed chunk size returns null, so it hangs to timeout instead of failing fast.

Content-Length off the Buffer, the settled guard, and chunk-extension handling are all correct.

@Hydralerne
Hydralerne merged commit bd9bb69 into oblien:main Jul 31, 2026
1 of 2 checks passed
Hydralerne pushed a commit that referenced this pull request Jul 31, 2026
…unnelRequest

`tunnelRequest` decided whether to de-chunk the body with an exact string
compare:

    const isChunked = parsedHeaders["transfer-encoding"] === "chunked";

Transfer-coding names are case-insensitive tokens and the header carries the
applied codings as a comma-separated list (RFC 9110 §10.1.4, RFC 9112 §6.1),
so `Chunked` and `gzip, chunked` both missed. Executed against the parser
with a stubbed duplex, replying `Transfer-Encoding: <value>` followed by
`7\r\n{"a":1}\r\n0\r\n\r\n`:

    "chunked"       -> body '{"a":1}'
    "Chunked"       -> body '7\r\n{"a":1}\r\n0\r\n\r\n'
    "gzip, chunked" -> body '7\r\n{"a":1}\r\n0\r\n\r\n'

The raw chunk framing is returned as the body with a 200, so the caller's
`JSON.parse` throws and `fetchMgmt` swallows it to `null` — the same
silent-no-data symptom #322 was opened to fix.

Extracts `isChunkedEncoding()`: lowercase, split on `,`, match any coding
equal to `chunked`. This is the shape suggested in the review on #322.

Not observed in production: the OpenResty management API this path talks to
emits lowercase `chunked`. This is robustness on a general-purpose HTTP/1.1
parser, not a fix for a reported outage.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Hydralerne pushed a commit that referenced this pull request Jul 31, 2026
The `close` handler treated *any* buffered bytes as a complete response as
long as the head had been parsed, so a connection that dropped mid-body
resolved successfully with a partial payload. Executed against the parser
with a stubbed duplex:

| response written before close | before | after |
|---|---|---|
| `Transfer-Encoding: chunked` + `7\r\n{"a":1}\r\n` (no terminating 0-chunk) | `{statusCode:200, body:'7\r\n{"a":1}\r\n'}` | `null` |
| `Content-Length: 20` + 7 bytes | `{statusCode:200, body:'{"a":1}'}` | `null` |

`http.request` surfaced both of these as errors, so #322's hand-rolled
client converted a hard failure into a silent partial success. The realistic
trigger is the SSH channel dropping mid-scrape: the caller gets a 200 and
parses a truncated body rather than retrying or reporting the server
unreachable.

`contentLength` and `isChunked` are hoisted out of the `data` handler (they
are derived from the head and never change afterwards) so `close` can tell
the three framings apart. A response with no length framing at all is still
terminated by the close itself and still resolves — that is the only case
where a close is a legitimate end-of-body signal. Anything with a declared
framing that has not already resolved by the time the socket closes is
incomplete, and now returns `null`.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Hydralerne pushed a commit that referenced this pull request Jul 31, 2026
Both `tunnelRequest` and `tunnelStream` assemble the request line and header
lines by string concatenation and write them straight onto the tunnel, with
no check that the components are single-line. A CR or LF in the path, a
header name or a header value terminates its own line and appends whatever
follows to the request head. Executed with a stubbed duplex:

    tunnelRequest("srv-1", 9145,
      "/health\r\nX-Injected: yes\r\n\r\nGET /rules HTTP/1.1\r\nHost: 127.0.0.1:9145")

wrote two complete HTTP requests onto one socket. A header value does the
same:

    headers: { "X-Trace": "abc\r\nX-Injected: yes" }
    -> "...Connection: close\r\nX-Trace: abc\r\nX-Injected: yes\r\n\r\n"

`tunnelStream` (which predates #322) has the identical defect.

This is not reachable today and the PR that introduced it says so: all eight
management-API path builders `encodeURIComponent` their only user-influenced
value (CR -> %0D, LF -> %0A) or pass a derived number, the sole `headers`
object ever passed is the literal `{"Content-Type": "application/json"}`, and
`method` is only ever the literal "GET" or "POST". What changed is that
`http.request({ method, path, headers, createConnection })` used to enforce
this itself - it throws `ERR_UNESCAPED_CHARACTERS` on such a path and
`ERR_INVALID_CHAR` on such a header value, under both Node 22 and Bun - so
replacing it with a hand-rolled client removed a guard that was doing real
work for free.

Both functions now return `null` before opening the SSH channel if any
component of the head contains CR or LF, which is the failure mode both
already document for a request they cannot make. Nothing that is accepted
today changes: the tests pin that a percent-encoded path and a normal
`Content-Type` header still go out byte-identical.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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