Skip to content

fix: bound free-mode firewall-api requests with a timeout (SURF-1041)#19

Open
John-David Dalton (jdalton) wants to merge 1 commit into
mainfrom
surf-1041-unauth-fetch-timeout
Open

fix: bound free-mode firewall-api requests with a timeout (SURF-1041)#19
John-David Dalton (jdalton) wants to merge 1 commit into
mainfrom
surf-1041-unauth-fetch-timeout

Conversation

@jdalton

Copy link
Copy Markdown
Collaborator

What this fixes

In free mode (no SOCKET_API_TOKEN), the scanner asks firewall-api.socket.dev about each package while bun install runs. Each request was a bare fetch(url, { headers }) with no timeout and no abort signal. If the server accepted the connection but never sent a response, that request would wait forever. Because the batch fans out with Promise.all, one hung request stalls the whole batch — and with it, the user's bun install — indefinitely, with no error ever shown.

The authenticated path does not have this problem: it goes through @socketsecurity/sdk, which already applies a 30-second per-request timeout (DEFAULT_HTTP_TIMEOUT) plus retries. Only the free-mode path was unbounded.

The fix

Every free-mode request now carries signal: AbortSignal.timeout(30_000). When 30 seconds pass with no response, fetch rejects with a TimeoutError. That rejection travels through the existing fail-fast Promise.all and out of the async generator, so it surfaces to the user as a normal scan error — exactly like the existing non-200 handling — instead of hanging. The 30-second budget is the same value the SDK uses for authenticated mode, so both paths now behave consistently.

Fixes SURF-1041.

Audit detail: what was checked, and why the authenticated path is fine

SURF-1041 asked whether the concurrent Socket API requests are bounded by sane per-request and overall timeouts, and whether timeouts surface as errors rather than silently dropping results or blocking. Here is what each path does:

Authenticated mode (src/modes/authenticated.ts) — already safe. It calls new SocketSdk(apiToken, { userAgent }) and uses batchPackageStream. The SDK constructor defaults timeout to DEFAULT_HTTP_TIMEOUT (30_000 ms) and retries to 3 with exponential backoff, even when the caller passes no timeout. Inside the SDK's HTTP client, the underlying Node request registers request.on('timeout', ...), which destroys the socket and rejects with a clear "... request timed out after 30000ms ..." error. So per-request timeout, retry, and error-surfacing are all present.

Unauthenticated / free mode (src/modes/unauthenticated.ts) — the gap. This path does not use the SDK. It called fetch directly with only headers, so there was no per-request deadline, no abort propagation, and no retry. A stalled connection had no way to fail; it just blocked. This is the defect this PR fixes.

Overall deadline. Concurrency is already bounded (maxSending: 20), and with a per-request timeout in place, the total wall-clock time is now bounded rather than open-ended. A single global scan deadline would be a larger design change; the unbounded per-request wait was the actual defect, and a per-request timeout is what the authenticated path already relies on, so this keeps the two consistent.

The regression test and how it proves the fix

test/modes/unauthenticated.test.ts gets a new test: unauthenticated scanner surfaces a request timeout as an error.

It stubs the global fetch (the same genuinely-external HTTP boundary the other tests in this file already stub via spyOn(global, 'fetch')) with a "hung connection" that only ever settles when its AbortSignal aborts — it rejects with the signal's reason and otherwise never resolves. It then uses Bun fake timers to advance past the 30-second budget so the real AbortSignal.timeout fires deterministically, with no wall-clock wait. The test asserts the scan rejects with a TimeoutError rather than hanging.

Proof it catches the bug: with the pre-fix source (no signal on the fetch), the stub never settles, so the scan never completes and the whole test run hangs until the harness timeout kills it (bun test exited 124). The two existing toHaveBeenCalledWith assertions were also updated to expect signal: expect.any(AbortSignal), and they fail against the pre-fix source. With the fix, all six tests in the file pass.

Verification

  • bun test — full suite passes except the two test/live.test.ts cases, which need a real SOCKET_API_TOKEN and network and are unrelated to this change (they time out with no credentials).
  • bun test test/modes/unauthenticated.test.ts — 6 pass, 0 fail.
  • Pre-fix proof — reverting only src/modes/unauthenticated.ts makes the new test hang the run (exit 124) and the updated assertions fail.
  • bun run build — succeeds.
  • Lint — no new warnings or errors in the changed files.

Not included

SURF-633 (the in-flight-artifact race in scanner-factory.ts) is already fixed on main (commits bced321 and f0f9a0a) with its own regression tests, so it needed no change here. This PR is scoped to the timeout audit only, and does not touch scanner-factory.ts.

The unauthenticated (free-mode) scanner fetched firewall-api.socket.dev
with a bare `fetch(url, { headers })` — no timeout, no abort signal. The
scan runs during `bun install`, and the batch fans out with
`Promise.all`, so a single hung connection had no deadline and blocked
the whole install indefinitely instead of surfacing an error. The
authenticated path is already bounded (the SDK applies a 30s
DEFAULT_HTTP_TIMEOUT plus retries).

Pass `signal: AbortSignal.timeout(30_000)` on every free-mode request,
matching the SDK default. When the deadline fires, fetch rejects with a
TimeoutError that propagates through the fail-fast Promise.all and out of
the async generator as a scan error.

Adds a deterministic regression test: a stub modelling a hung connection
(settles only when its AbortSignal aborts) plus fake timers advanced past
the budget. It hangs against the pre-fix code and passes with the fix.
@jdalton
John-David Dalton (jdalton) force-pushed the surf-1041-unauth-fetch-timeout branch from 1b0fc44 to b2d2554 Compare July 25, 2026 04:41
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.

1 participant