fix(varlock): finish the response when leak detection throws on end

.bumpy/leak-scan-finish-response.md

@@ -0,0 +1,5 @@
+---
+varlock: patch
+---
+
+A leak detected in `ServerResponse.end` no longer leaves the HTTP client hanging. The response is finished before the leak error is rethrown (a plaintext 500 if the headers have not gone out yet, otherwise the connection is closed), so a Next.js Pages Router `res.json()` that would have leaked a sensitive value fails the request instead of stalling the client on a body that never arrives.

framework-tests/frameworks/nextjs/files/pages-router/leaky-api-route.ts

@@ -0,0 +1,12 @@
+import type { NextApiRequest, NextApiResponse } from 'next';
+
+import { ENV } from 'varlock/env';
+
+// Deliberately leak a sensitive value from a pages-router API route. `res.json()`
+// writes the whole body through a single `ServerResponse.end()` call, so the leak is
+// caught there rather than in `write()`. Varlock has to finish the response itself
+// (the client would otherwise hang), and next's `apiResolver` catch path then calls
+// `res.end()` a second time - which must not take the dev server down.
+export default function handler(req: NextApiRequest, res: NextApiResponse) {
+  res.status(200).json({ leaked: ENV.SENSITIVE_VAR });
+}

framework-tests/frameworks/nextjs/nextjs-shared.ts

@@ -175,6 +175,7 @@ export function defineNextjsTests(versionOrCanary: number | 'canary', testDir: s
             'app/page.tsx': 'pages/basic-page.tsx',
             'pages/pages-ssr.tsx': 'pages-router/ssr-page.tsx',
             'pages/leaky-ssr.tsx': 'pages-router/leaky-ssr-page.tsx',
+            'pages/api/leaky.ts': 'pages-router/leaky-api-route.ts',
             'middleware.ts': 'middleware/middleware.ts',
           },
           requests: [
@@ -267,6 +268,19 @@ export function defineNextjsTests(versionOrCanary: number | 'canary', testDir: s
                 shouldNotContain: ['super-secret-var'],
               },
             },
+            {
+              // API route leak: `res.json()` sends the whole body through a single
+              // `end()` with no `write()`, which is the only path where the scanner
+              // sees the body for the first time at `end`. In dev the integration
+              // patches in redact mode, so the response still completes - the secret
+              // just comes out scrubbed, and the client is never left hanging.
+              label: 'runtime leak detection redacts a pages-router API route body',
+              path: '/api/leaky',
+              bodyAssertions: {
+                shouldContain: ['leaked'],
+                shouldNotContain: ['super-secret-var'],
+              },
+            },
             {
               // The fail-closed kill must only affect that one response — the dev
               // server has to keep serving afterwards. The gzip header assertion
@@ -652,6 +666,9 @@ export function defineNextjsTests(versionOrCanary: number | 'canary', testDir: s
                 replacements: { '// OUTPUT-MODE': "output: 'standalone'," },
               },
               'app/page.tsx': 'pages/runtime-boot-page.tsx',
+              // production is the only mode where the response scanner throws rather
+              // than redacts, so this is where an `end()`-only leak has to be handled
+              'pages/api/leaky.ts': 'pages-router/leaky-api-route.ts',
             },
             requests: [
               {
@@ -667,6 +684,38 @@ export function defineNextjsTests(versionOrCanary: number | 'canary', testDir: s
                   ],
                 },
               },
+              {
+                // `res.json()` reaches the scanner at `end()` with no preceding
+                // `write()`, but next's compression layer has already emitted the
+                // headers by then, so the response cannot be rewritten and the
+                // connection is killed instead. Before this was handled, the client got
+                // a 200 whose Content-Length promised more bytes than were ever sent,
+                // and hung waiting for them.
+                label: 'leaking pages-router API route does not leave the client hanging',
+                path: '/api/leaky',
+                // a network failure, specifically: accepting any failure would let the
+                // original hang (a client-side timeout) pass this scenario
+                expectedFailure: 'network',
+                bodyAssertions: {
+                  shouldNotContain: ['super-secret-var'],
+                },
+              },
+              {
+                // next answers the rethrown leak error by calling `res.end()` a second
+                // time; the server has to survive that and keep serving.
+                label: 'server still serves after a leaking API route response',
+                path: '/',
+                bodyAssertions: {
+                  shouldContain: ['Varlock Framework Test - runtime boot'],
+                },
+              },
+            ],
+            outputAssertions: [
+              {
+                description: 'leak detection fires for the API route, without logging the secret',
+                shouldContain: ['DETECTED LEAKED SENSITIVE CONFIG'],
+                shouldNotContain: ['super-secret-var'],
+              },
             ],
           });
         });

framework-tests/harness/dev-server.ts

@@ -49,6 +49,16 @@ const sleep = (ms: number) => new Promise<void>((r) => {
   setTimeout(r, ms);
 });
 
+/**
+ * A client-side abort (`AbortSignal.timeout`) means the server never answered - the
+ * request hung. Anything else is the connection itself failing, which is what a killed
+ * response looks like to the client.
+ */
+function classifyRequestFailure(err: unknown): 'network' | 'timeout' {
+  const name = (err as { name?: string } | undefined)?.name;
+  return name === 'TimeoutError' || name === 'AbortError' ? 'timeout' : 'network';
+}
+
 /**
  * Fetch a URL with retries (server may report ready before accepting connections).
  *
@@ -70,6 +80,8 @@ async function fetchWithRetry(
   } = {},
 ): Promise<DevServerRequestResult> {
   const {
+    // `retries: 0` means a single attempt with no retry sleeps, for requests whose
+    // expected outcome is a failure
     retries = 3, delayMs = 500, timeoutMs = 15_000,
     retryWhileUnavailable = true, unavailableTimeoutMs = 30_000,
   } = opts;
@@ -364,15 +376,25 @@ export async function runDevServer(
       const url = `${currentUrl}${req.path}`;
       log(`Request ${i + 1}/${scenario.requests.length}: GET ${url}`);
       try {
-        const result = await fetchWithRetry(url, { retryWhileUnavailable: req.expectedStatus !== 503 });
+        const result = await fetchWithRetry(url, {
+          retryWhileUnavailable: req.expectedStatus !== 503,
+          // a request that is expected to fail gets one attempt: retrying it only buys
+          // `delayMs` of sleeping per retry on the path where the test passes, and a
+          // shorter timeout bounds the wait if a hang regression puts one back
+          ...req.expectedFailure ? { timeoutMs: 5_000, retries: 0 } : {},
+        });
         log(`Response: status=${result.status}, body=${result.body.length} bytes`);
         responses.push(result);
       } catch (err) {
-        if (req.allowRequestFailure) {
+        if (req.allowRequestFailure || req.expectedFailure) {
           // e.g. runtime leak detection kills the response mid-stream — record a
-          // synthetic result so scenario/log assertions can still run
-          log(`Request failed (allowed): ${(err as Error).message}`);
-          responses.push({ status: 0, body: '', headers: {} });
+          // synthetic result so scenario/log assertions can still run, keeping the
+          // kind of failure so `expectedFailure` can tell a reset from a hang
+          const failure = classifyRequestFailure(err);
+          log(`Request failed (allowed, ${failure}): ${(err as Error).message}`);
+          responses.push({
+            status: 0, body: '', headers: {}, failure,
+          });
           continue;
         }
         logError(`Request failed: ${(err as Error).message}`);

framework-tests/harness/test-fixture.ts

@@ -422,8 +422,15 @@ export class FrameworkTestEnv {
         test(testLabel, () => {
           const resp = ctx.result!.responses[i];
           expect(resp, `No response for request ${i} (GET ${req.path})`).toBeDefined();
+          if (req.expectedFailure) {
+            expect(
+              resp.failure,
+              `Expected GET ${req.path} to fail with a ${req.expectedFailure} error`
+              + `${resp.failure ? `, got ${resp.failure}` : ` but it returned ${resp.status}`}`,
+            ).toBe(req.expectedFailure);
+          }
           // requests marked allowRequestFailure only assert status when explicitly set
-          if (req.expectedStatus !== undefined || !req.allowRequestFailure) {
+          if (req.expectedStatus !== undefined || !(req.allowRequestFailure || req.expectedFailure)) {
             const expectedStatus = req.expectedStatus ?? 200;
             expect(resp.status, `Expected status ${expectedStatus} for GET ${req.path}, got ${resp.status}`).toBe(expectedStatus);
           }

framework-tests/harness/types.ts

@@ -167,6 +167,16 @@ export interface DevServerRequest {
    * unless `expectedStatus` is set explicitly.
    */
   allowRequestFailure?: boolean;
+  /**
+   * Assert HOW the request failed, not merely that it did. `allowRequestFailure` on its
+   * own records the same empty synthetic response for every error, so a request that hung
+   * until the client timed out is indistinguishable from one the server killed promptly -
+   * which makes a "does not hang" regression test pass even when the hang comes back.
+   * `network` is a refused/reset connection, `timeout` is the client giving up.
+   * Implies `allowRequestFailure`, and shortens the per-attempt timeout since the failure
+   * is expected to be prompt.
+   */
+  expectedFailure?: 'network' | 'timeout';
 }
 
 /** Result of a single HTTP request to the dev server */
@@ -175,6 +185,8 @@ export interface DevServerRequestResult {
   body: string;
   /** response headers, lowercase keys */
   headers: Record<string, string>;
+  /** set only when the request failed and the scenario allowed it */
+  failure?: 'network' | 'timeout';
 }
 
 /** Configuration for a dev server scenario */

packages/varlock-website/src/content/docs/guides/secrets.mdx

@@ -176,7 +176,7 @@ The `reveal` command displays values in an **alternate screen buffer**. When you
 
 When the output of [`varlock run`](/reference/cli/load-and-run/#run) is piped or redirected (CI logs, files, `| tee`, etc.), the child process's stdout/stderr is piped through the same redaction engine, so any sensitive values that end up in your application's output will be masked automatically before they persist anywhere.
 
-When output is attached to an interactive terminal, it passes straight through instead. Piping would break interactive tools (e.g., `psql`, `claude`) that rely on TTY detection, and a human at the terminal already has access to the secrets. Use `--no-redact-stdout` to force-disable redaction for piped output, or `--redact-stdout` to force it (e.g., to override `@redactLogs=false`; errors if output is attached to an interactive terminal, where redaction is not possible without breaking TTY behavior):
+When output is attached to an interactive terminal, it passes straight through instead. Piping would break interactive tools (e.g., `psql`, `claude`) that rely on TTY detection, and a human at the terminal already has access to the secrets. `--no-redact-stdout` disables redaction entirely. `--redact-stdout` forces it on piped output, which is how you override `@redactLogs=false`; it errors when output is attached to an interactive terminal, where redaction would break TTY behavior.
 
 ```bash
 varlock run --no-redact-stdout -- node app.js | tee log.txt  # force-disable redaction for piped output
@@ -196,7 +196,7 @@ console.log(process.env.SECRET_KEY);
 // outputs "my▒▒▒▒▒" instead of "my-secret-value"
 ```
 
-This works by intercepting Node.js internal console internals, with an additional layer that wraps the console methods themselves to handle environments where `console.log` has been patched by the platform (e.g., AWS Lambda) or where those internals do not exist at all (edge runtimes like Cloudflare Workers and Vercel Edge).
+This works by intercepting Node's console internals, with an additional layer that wraps the console methods themselves to handle environments where `console.log` has been patched by the platform (e.g., AWS Lambda) or where those internals do not exist at all (edge runtimes like Cloudflare Workers and Vercel Edge).
 
 Redaction covers whatever you pass to the console methods: strings, arrays, plain objects, and `Error` objects (including messages, stack traces, and anything nested inside). Varlock logs a redacted copy rather than modifying what you passed in.
 
@@ -214,10 +214,10 @@ To disable runtime log redaction, set the [`@redactLogs`](/reference/root-decora
 
 _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.
 
 This works by patching:
-- **Node.js `ServerResponse`**: intercepts `write()` and `end()` calls, scanning text and JSON response bodies (including gzip-compressed responses)
+- **Node.js `ServerResponse`**: intercepts `write()` and `end()` calls, scanning text and JSON response bodies, including compressed ones
 - **Global `Response` constructor**: intercepts the `Response` class used in edge runtimes (e.g., Cloudflare Workers), scanning bodies passed to the constructor and `Response.json()`
 
 Streamed responses are scanned across chunk boundaries, so a sensitive value that gets split between two writes is still detected. When the end of a chunk looks like the start of a sensitive value, that trailing text is held back briefly (up to 15ms, or until the next chunk arrives) so the full value can be caught before any of it is sent.
@@ -238,7 +238,7 @@ The [`varlock scan` command](/reference/cli/project/#scan) checks your project f
 varlock scan
 ```
 
-This is intended to be used as a pre-commit git hook to prevent accidentally committing secrets into version control. If no sensitive values are found in plaintext, it exits successfully. If any are detected, it reports the file, line number,and which secret was found, then exits with a non-zero status code.
+This is intended to be used as a pre-commit git hook to prevent accidentally committing secrets into version control. If no sensitive values are found in plaintext, it exits successfully. If any are detected, it reports the file, line number, and which secret was found, then exits with a non-zero status code.
 
 It can also be used to scan build output as an extra step to prevent accidentally bundling secrets into client-facing code. Our [drop-in integrations](/integrations/overview/) usually do this automatically, but this can be useful in some scenarios.