fix: bound free-mode firewall-api requests with a timeout (SURF-1041)#19
Open
John-David Dalton (jdalton) wants to merge 1 commit into
Open
fix: bound free-mode firewall-api requests with a timeout (SURF-1041)#19John-David Dalton (jdalton) wants to merge 1 commit into
John-David Dalton (jdalton) wants to merge 1 commit into
Conversation
John-David Dalton (jdalton)
force-pushed
the
surf-1041-unauth-fetch-timeout
branch
from
July 24, 2026 22:03
b9318b1 to
1b0fc44
Compare
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.
John-David Dalton (jdalton)
force-pushed
the
surf-1041-unauth-fetch-timeout
branch
from
July 25, 2026 04:41
1b0fc44 to
b2d2554
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this fixes
In free mode (no
SOCKET_API_TOKEN), the scanner asksfirewall-api.socket.devabout each package whilebun installruns. Each request was a barefetch(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 withPromise.all, one hung request stalls the whole batch — and with it, the user'sbun 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,fetchrejects with aTimeoutError. That rejection travels through the existing fail-fastPromise.alland 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 callsnew SocketSdk(apiToken, { userAgent })and usesbatchPackageStream. The SDK constructor defaultstimeouttoDEFAULT_HTTP_TIMEOUT(30_000 ms) andretriesto 3 with exponential backoff, even when the caller passes notimeout. Inside the SDK's HTTP client, the underlying Node request registersrequest.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 calledfetchdirectly with onlyheaders, 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.tsgets 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 viaspyOn(global, 'fetch')) with a "hung connection" that only ever settles when itsAbortSignalaborts — 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 realAbortSignal.timeoutfires deterministically, with no wall-clock wait. The test asserts the scan rejects with aTimeoutErrorrather than hanging.Proof it catches the bug: with the pre-fix source (no
signalon the fetch), the stub never settles, so the scan never completes and the whole test run hangs until the harness timeout kills it (bun testexited 124). The two existingtoHaveBeenCalledWithassertions were also updated to expectsignal: 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 twotest/live.test.tscases, which need a realSOCKET_API_TOKENand network and are unrelated to this change (they time out with no credentials).bun test test/modes/unauthenticated.test.ts— 6 pass, 0 fail.src/modes/unauthenticated.tsmakes the new test hang the run (exit 124) and the updated assertions fail.bun run build— succeeds.Not included
SURF-633 (the in-flight-artifact race in
scanner-factory.ts) is already fixed onmain(commitsbced321andf0f9a0a) with its own regression tests, so it needed no change here. This PR is scoped to the timeout audit only, and does not touchscanner-factory.ts.