fix(varlock): finish the response when leak detection throws on end - #1065
Conversation
scanForLeaks threw before the original ServerResponse.end ran, so the client hung. Send a 500 (clearing stale Content-Length and Content-Encoding) or destroy the socket when headers are already sent, then rethrow so callers still see the leak error. Fixes #897
|
The changes in this PR will be included in the next version bump.
|
📦 Bundle size
dist/ only; native binaries are versioned separately and not counted here. |
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
varlock-website | 196d52d | Commit Preview URL Branch Preview URL |
Sep 03 2026, 08:51 PM |
@env-spec/parser
varlock
@varlock/native-helper-darwin
@varlock/native-helper-linux-arm64
@varlock/native-helper-linux-x64
@varlock/native-helper-win32-x64
@varlock/aws-sigv4-plugin
commit: |
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
The replacement response itself completes, but the synchronous rethrow triggers a second end() in the primary Next.js Pages Router flow and produces ERR_STREAM_WRITE_AFTER_END. The response metadata also retains the original Next-generated ETag.
Reviewed changes in c143a334, covering the response finalization helper, both end() scan paths, socket-level regression tests, documentation, and release metadata.
- Leak response finalization: Replaces an unsent leaking response with a plaintext 500, or destroys an already-started response, before rethrowing the detection error.
- Framing cleanup: Rewrites
Content-Lengthand removes stale compression and transfer encoding headers for the replacement body. - Regression coverage: Adds detached-response and real HTTP tests for response completion and stale headers.
- User-facing guidance: Documents the new behavior and records a
varlockpatch release entry.
azure/gpt-5.6-sol ｜ 𝕏
… response Rewriting the rejected response into a plaintext 500 meant inheriting the headers that described the body being thrown away: Cache-Control (an s-maxage would have a CDN cache the error), Set-Cookie, the ETag next.js computes from the body before calling end(), and anything route-specific. It was also mostly unreachable - next.js in production emits the headers from its compression layer before varlock ever sees the body, so headersSent is already true. Destroy the connection in both cases, which is what the write() path already does mid-stream, and leave the leak report on the server log as the diagnostic. This also absorbs the second end() that framework error handling makes in response to the rethrown error (next.js pages router apiResolver -> sendError), which node ignores on a destroyed response. Adds next.js coverage for a pages-router API route, where res.json() reaches the scanner at end() with no preceding write(): dev redacts and still completes, production kills the response and keeps serving.
There was a problem hiding this comment.
Important
The production Next.js regression scenario still accepts the original client hang as a successful outcome.
Reviewed changes since the prior Pullfrog review at c143a334, covering the revised connection teardown and its new integration coverage.
- Destroyed rejected responses: Replaced the plaintext 500 path with
ServerResponse.destroy()before rethrowing, avoiding stale representation metadata and absorbing framework retries. - Expanded regression coverage: Added direct HTTP tests and a Next.js Pages Router API fixture for uncompressed, compressed, and second-
end()behavior. - Updated user guidance: Revised the documentation and release note to describe failed requests caused by connection closure.
azure/gpt-5.6-sol ｜ 𝕏
… hang allowRequestFailure recorded the same empty synthetic response for every error, so the pages-router API route scenario accepted a client-side timeout exactly as it accepted a promptly killed connection. A regression back to the original hang would have passed it. Requests can now declare `expectedFailure: 'network' | 'timeout'`, which keeps the failure kind on the result and asserts it (and shortens the per-attempt timeout, since an expected failure should be prompt). Verified live: the scenario reports 'network', and flipping the expectation to 'timeout' fails it.
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes since the prior Pullfrog review at 35397275, covering the harness assertion added for the no-hang regression.
- Classified request failures: Preserved whether an allowed fetch failure came from a client timeout or a network error.
- Strengthened the Next.js regression: Required the leaking Pages Router request to fail with a network error, so the original timeout hang can no longer satisfy the scenario.
azure/gpt-5.6-sol ｜ 𝕏
fetchWithRetry defaults to 3 retries with a 500ms delay, so a request whose expected outcome is a failure spent 4 attempts and a full second of sleeping on the path where the test passes. Retrying something that is supposed to fail buys nothing.
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes since the prior Pullfrog review at 1698c0ea, covering the expected-failure request retry adjustment.
- Limited expected failures to one attempt: Set
retries: 0for requests that should fail, avoiding unnecessary retry delays while retaining the shortened timeout that detects a restored hang. - Preserved regression integrity: Confirmed the surrounding successful requests and required leak diagnostic prevent an unrelated transient network failure from satisfying the complete Next.js scenario.
azure/gpt-5.6-sol ｜ 𝕏
Destroying unconditionally regressed vite: its dev middleware reaches end() with the headers still unsent, and the existing leak scenario asserts a 500 from /api/leak (frameworks/vite: 8 failures across v5-v8). Both branches are load-bearing. Next.js in production reaches end() through a compression layer that has already emitted the headers, so an api route there is destroyed; a vite middleware still has a response to replace, and a plaintext 500 beats a dead socket when one is achievable. The replacement clears the whole header table rather than the few obviously-wrong entries, so nothing describing the rejected body survives onto it: Cache-Control (an s-maxage would have a CDN cache the error), Set-Cookie, the ETag next.js computes from the body, and route-specific headers. Ending the response also brings back the second-end hazard, so the response is flagged as finalized and later writes/ends through the patched methods are no-ops (next.js pages router apiResolver -> sendError ends it again, which node turns into an ERR_STREAM_WRITE_AFTER_END error event on a response that has no listener). Tests cover both branches over a real connection, and all seven fail against main.
There was a problem hiding this comment.
ℹ️ No critical issues; one minor suggestion inline.
Reviewed changes since the prior Pullfrog review at 42a91b2b, covering the restored pre-header response replacement behavior.
- Restored the unsent-header 500 path: Finished an uncommitted leaking response with a complete plaintext 500 while continuing to destroy responses whose headers are already sent.
- Cleared rejected response metadata: Removed all existing headers before declaring the replacement body's content type and length.
- Expanded branch-specific coverage: Added real-connection assertions for the replacement 500, stale-header removal, compressed bodies, and the post-header connection teardown path.
- Updated user guidance: Documented when clients receive a plaintext 500 versus a closed connection.
azure/gpt-5.6-sol ｜ 𝕏
Node only falls back to the standard reason when statusMessage is unset, so a route that set its own kept it across the status code change and the replacement went out as `500 <their reason>`.
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes since the prior Pullfrog review at 8e503ea8, covering the replacement response reason phrase fix.
- Reset the replacement reason phrase: Set
statusMessagetoInternal Server Erroralongside the 500 status so route-provided text cannot survive onto the rejected response. - Added wire-level coverage: Added a real HTTP route with a custom reason phrase and asserted the client receives
500 Internal Server Error.
azure/gpt-5.6-sol ｜ 𝕏
The leak prevention paragraph spelled out which branch a caught leak takes, which is implementation detail a reader does not need. What matters is that the body is never delivered, the request fails rather than hanging, and the diagnostic is on the server log. Also: the ServerResponse scanner handles deflate, brotli and zstd, not just gzip; split the dense --redact-stdout sentence; "Node.js internal console internals"; a missing space in the scan output description.
There was a problem hiding this comment.
ℹ️ No critical issues; one documentation correction inline.
Reviewed changes since the prior Pullfrog review at 57048071, covering the secrets-guide cleanup in 196d52d8.
- Simplified leak-prevention guidance: Replaced implementation-level response-finalization details with a shorter description of the client-visible outcome.
- Corrected adjacent documentation: Clarified stdout redaction flags, generalized compressed-response wording, simplified the Node console description, and fixed punctuation.
azure/gpt-5.6-sol ｜ 𝕏
| _Only available in JavaScript/Node.js projects using varlock's runtime integrations._ | ||
|
|
||
| Varlock scans outgoing HTTP responses at runtime to detect if any sensitive values are being accidentally sent to clients. If a leak is detected, varlock throws an error with a detailed diagnostic message including the config item key and where the leak was detected. | ||
| Varlock scans outgoing HTTP responses at runtime to detect if any sensitive values are being accidentally sent to clients. If a leak is detected, varlock throws an error with a detailed diagnostic message including the config item key and where the leak was detected. The rejected body is never delivered and the request fails rather than hanging, so look to your server log for the diagnostic. |
There was a problem hiding this comment.
This now says every detected leak makes the request fail, but integrations using redactInsteadOfThrow complete the request with a redacted body, as the new development-mode Next.js scenario demonstrates. Please scope the failure behavior to throw mode or mention the redaction outcome so this guide remains accurate in development.
There was a problem hiding this comment.
its mostly internal, used during development only - so probably ok
2 participants
Fixes #897.
Split 2 of 3 out of #1061 by @WalksWithASwagger, who wrote the original fix. Commit authorship is preserved; the three slices touch disjoint files so they review and land independently.
scanForLeaksthrew before the originalServerResponse.endran, so the response was never finished. In a Next.js Pages Routerres.json()the client got a 200 whoseContent-Lengthpromised more bytes than were ever sent, and sat waiting for the rest. This matches the TODO already sitting inpatch-server-response.ts.Now the response is finished before the leak error is rethrown, so the client is not left waiting:
The replacement clears the whole header table rather than the few obviously-wrong entries, so nothing describing the rejected body rides onto it:
Cache-Control: s-maxage=...would have a CDN cache the error,Set-Cookiewould still be set, and next.js computes anETagfrom the body before callingend().Ending the response means framework error handling can end it a second time (
apiResolvercatches and callssendError), which node turns into anERR_STREAM_WRITE_AFTER_ENDerror event on a response that has no listener. The response is flagged as finalized, so later writes and ends through the patched methods are no-ops.Covered on both the compressed and uncompressed
endpaths with a real HTTP client, including the case where nothing else finishes the response, which is the reported hang. Next.js gets a pages-router API route fixture (res.json()reaches the scanner atend()with no precedingwrite()): dev redacts and still completes, production kills the response and keeps serving.The framework harness gained
expectedFailure: 'network' | 'timeout'for this:allowRequestFailurecollapsed every error into the same empty synthetic response, so a "does not hang" scenario would have passed on a hang.Note this is wired at
endonly. A leak caught mid-stream inwritestill throws as before.Test plan
cd packages/varlock && bunx vitest run(123 files, 1953 passed)cd framework-tests && bunx vitest run frameworks/vite frameworks/nextjs/nextjs-v15.test.ts(276 passed)mainstalls the client on a truncated 200, this branch fails the request immediately and the server keeps serving