fix(verify-pr): harden the model proxy after adversarial audit

Audit round 1 found the relay forwarded client hop-by-hop headers
(connection, transfer-encoding, expect, proxy-authorization, ...) to the
upstream, which RFC 7230 forbids and which fights undici's own framing
for the relayed stream. Strip them, with a chunked-body test that pins
the behaviour. Round 1 also found the CLI entry had no coverage: add
tests for the missing-config exit code and the printed JSON coordinates
(token override, base-path prefix, no real-key leak, health nonce), and
document that the printed baseUrl uses the listen address, so binding
0.0.0.0 yields a URL clients cannot dial.

Rounds 2 and 3 (open-ended re-read plus adversarial passes over the
updated diff) found nothing further. All verify-pr suites: 123 passed.

.qwen/skills/verify-pr/SKILL.md

@@ -208,7 +208,8 @@ The boundary and the model credential are separate problems: the container
 isolates PR code, and a host-side proxy keeps the real model key out of the
 container. `scripts/verify-pr-model-proxy.mjs` is that proxy — the same relay
 rules as the CI lane's inline one (only `POST /chat/completions`, a per-run
-bearer token, a per-chunk idle watchdog). The CI lane deliberately keeps its
+bearer token, a per-chunk idle watchdog), plus hop-by-hop request header
+stripping. The CI lane deliberately keeps its
 own inline copy: the proxy holds the real key, so it must never execute a
 script from the PR-head checkout.
 

scripts/tests/verify-pr-model-proxy.test.js

@@ -4,13 +4,19 @@
  * SPDX-License-Identifier: Apache-2.0
  */
 
+import { spawn, spawnSync } from 'node:child_process';
 import http from 'node:http';
+import { fileURLToPath } from 'node:url';
 import { afterEach, describe, expect, it } from 'vitest';
 import {
   createModelProxy,
   parseArgs,
 } from '../../scripts/verify-pr-model-proxy.mjs';
 
+const SCRIPT = fileURLToPath(
+  new URL('../../scripts/verify-pr-model-proxy.mjs', import.meta.url),
+);
+
 const TOKEN = 'test-proxy-token';
 const NONCE = 'test-nonce';
 const REAL_KEY = 'real-upstream-key';
@@ -51,6 +57,7 @@ async function startUpstream({ status = 200, chunks = ['data: ok\n\n'] } = {}) {
       requests.push({
         method: req.method,
         url: req.url,
+        headers: req.headers,
         authorization: req.headers.authorization,
         body: Buffer.concat(body).toString('utf8'),
       });
@@ -148,6 +155,44 @@ describe('verify-pr-model-proxy', () => {
     expect(upstream.requests[0].url).toBe('/v1/chat/completions');
   });
 
+  it('strips hop-by-hop headers and relays a chunked request body', async () => {
+    const upstream = await startUpstream();
+    const proxy = await startProxy(upstream);
+    const { port } = new URL(proxy);
+    const status = await new Promise((resolve, reject) => {
+      const req = http.request(
+        {
+          host: '127.0.0.1',
+          port,
+          path: '/chat/completions',
+          method: 'POST',
+          headers: {
+            authorization: `Bearer ${TOKEN}`,
+            connection: 'x-client-hop',
+            'proxy-authorization': 'Basic client-proxy-secret',
+            'transfer-encoding': 'chunked',
+            expect: '100-continue',
+            'content-type': 'application/json',
+          },
+        },
+        (res) => {
+          res.resume();
+          res.on('end', () => resolve(res.statusCode));
+        },
+      );
+      req.on('error', reject);
+      req.write('{"model":');
+      req.end('"m"}');
+    });
+    expect(status).toBe(200);
+    const seen = upstream.requests[0];
+    expect(seen.body).toBe('{"model":"m"}');
+    expect(seen.authorization).toBe(`Bearer ${REAL_KEY}`);
+    expect(seen.headers['proxy-authorization']).toBeUndefined();
+    expect(seen.headers.expect).toBeUndefined();
+    expect(seen.headers.connection).not.toBe('x-client-hop');
+  });
+
   it('propagates an upstream error status and body', async () => {
     const upstream = await startUpstream({
       status: 429,
@@ -201,3 +246,50 @@ describe('parseArgs', () => {
     expect(() => parseArgs(['--port', '-1'])).toThrow('--port');
   });
 });
+
+describe('CLI', () => {
+  it('exits 2 when the upstream configuration is missing', () => {
+    const res = spawnSync(process.execPath, [SCRIPT], {
+      env: { PATH: process.env.PATH },
+      encoding: 'utf8',
+    });
+    expect(res.status).toBe(2);
+    expect(res.stderr).toContain('REVIEW_OPENAI_BASE_URL');
+  });
+
+  it('prints the listen coordinates as JSON and honors the token override', async () => {
+    const upstream = await startUpstream();
+    const child = spawn(process.execPath, [SCRIPT], {
+      env: {
+        PATH: process.env.PATH,
+        REVIEW_OPENAI_BASE_URL: `http://127.0.0.1:${upstream.port}/v1`,
+        REVIEW_OPENAI_API_KEY: REAL_KEY,
+        QWEN_VERIFY_PROXY_TOKEN: 'fixed-token',
+      },
+      stdio: ['ignore', 'pipe', 'inherit'],
+    });
+    cleanups.push(async () => {
+      child.kill('SIGTERM');
+    });
+    const line = await new Promise((resolve, reject) => {
+      let buffer = '';
+      child.stdout.on('data', (data) => {
+        buffer += data;
+        if (buffer.includes('\n')) resolve(buffer.split('\n')[0]);
+      });
+      child.on('error', reject);
+      child.on('exit', (code) =>
+        reject(new Error(`proxy exited before printing: ${code}`)),
+      );
+    });
+    const info = JSON.parse(line);
+    expect(info.token).toBe('fixed-token');
+    expect(info.baseUrl).toBe(`http://127.0.0.1:${info.port}/v1`);
+    expect(info.nonce).toMatch(/^[0-9a-f]{48}$/);
+    // The printed baseUrl must not leak the real key, and the health
+    // endpoint must echo the printed nonce.
+    expect(line).not.toContain(REAL_KEY);
+    const health = await fetch(`http://127.0.0.1:${info.port}/__health`);
+    expect(await health.text()).toBe(info.nonce);
+  });
+});

scripts/verify-pr-model-proxy.mjs

@@ -13,10 +13,13 @@
  * executes untrusted PR code, so the real model credential must stay outside
  * it. This proxy holds the real key on the host and lets the container reach
  * the model with a per-run bearer token instead. It mirrors the CI lane's
- * inline proxy (qwen-triage.yml `start_openai_proxy`), with two changes the
- * local shape needs: the listen address is a flag (a container cannot reach
- * the host's loopback, so bind the Docker bridge address instead), and the
+ * inline proxy (qwen-triage.yml `start_openai_proxy`), with changes the local
+ * shape needs: the listen address is a flag (a container cannot reach the
+ * host's loopback, so bind the Docker bridge address instead), and the
  * coordinates are printed as one JSON line for the maintainer to consume.
+ * Hop-by-hop request headers are stripped rather than forwarded.
+ * The printed baseUrl uses the listen address, so bind the address clients
+ * dial — 0.0.0.0 listens on every interface but is not a dialable URL.
  *
  * The CI lane deliberately keeps its own inline copy: the proxy handles the
  * real key, so it must not execute a script from the PR-head checkout.
@@ -47,6 +50,24 @@ import { isEntryPoint } from './lib/entry-point.js';
 
 const DEFAULT_IDLE_TIMEOUT_MS = 120_000;
 
+// RFC 7230 hop-by-hop headers describe the client's connection to us, not the
+// upstream one. Forwarding them is never correct, and a copied
+// transfer-encoding or expect also fights the framing undici chooses for the
+// relayed stream. `host` and `content-length` ride along for the same reason.
+const STRIPPED_REQUEST_HEADERS = [
+  'host',
+  'content-length',
+  'connection',
+  'keep-alive',
+  'proxy-authorization',
+  'proxy-authenticate',
+  'te',
+  'trailer',
+  'transfer-encoding',
+  'upgrade',
+  'expect',
+];
+
 export function parseArgs(argv) {
   const options = { listen: '127.0.0.1', port: 0 };
   for (let i = 0; i < argv.length; i++) {
@@ -134,8 +155,7 @@ export function createModelProxy({
       }
 
       const headers = new Headers(req.headers);
-      headers.delete('host');
-      headers.delete('content-length');
+      for (const name of STRIPPED_REQUEST_HEADERS) headers.delete(name);
       headers.set('authorization', `Bearer ${apiKey}`);
 
       const init = { method: req.method, headers };