mirror of
https://github.com/QwenLM/qwen-code.git
synced 2026-11-07 06:19:43 -10:00
* feat(verify-pr): add local Docker recipe with a host-side model proxy A local /verify-pr round needs the same isolation the CI lane provides, but the skill only documented the boundary check, not how to run the model credential through it. Add scripts/verify-pr-model-proxy.mjs, a host-side relay that holds the real key and lets the container call the model with a per-run bearer token, mirroring the CI lane's inline proxy (POST /chat/completions only, nonce health check, per-chunk idle watchdog). The listen address is a flag so the proxy can bind the Docker bridge, and readiness is printed as one JSON line. The SKILL recipe pairs it with a docker run template: the token travels as QWEN_VERIFY_PROXY_TOKEN (not OPENAI_API_KEY, which the isolation gate refuses), and the container's provider entry reads it via envKey. The CI lane keeps its inline copy — the proxy holds the real key, so it must not execute a script from the PR-head checkout. Verified: 11 new pin tests plus all existing verify-pr suites (120 passed), and a real docker smoke — the gate passes inside node:22-bookworm with the proxy token, refuses OPENAI_API_KEY with exit 4, and a container POST reaches a mock upstream with the real key injected and the /v1 prefix preserved. * 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. * fix(verify-pr): address review findings in the model proxy and checker Round-1 review (12 inline findings at70aed891bf). Proxy: abort the upstream when the client disconnects before headers instead of paying for a completion nobody reads (R1-1); make the idle watchdog pause-aware, re-arm on drain, and match the client's own 240 s stream tolerance (R1-3); bracket IPv6 listen hosts via an exported formatter and normalize base paths once (R1-12). Checker: add --context so a host-held snapshot pins the expected head instead of the writable artifact copy, and refuse an unusable snapshot rather than silently falling back (R1-9). Each guard is pinned by a test that was verified to go red when the guard is removed (six mutations: pre-header abort, pause guard, per-chunk re-arm, body error handler, IPv6 bracket, --context). * docs(verify-pr): harden the local container recipe and record the design Recipe: run the key-holding proxy and the checker from a trusted checkout, never the PR tree (R1-6, R1-9); background the proxy with a pid file and stop it at the end (R1-4); read the key instead of inlining it (R1-5); use a named detached container driven by docker exec (R1-7); require a standalone clone, the host uid and a fresh HOME (R1-8); mount the full host-held metadata snapshot rather than the four-key artifact copy (R1-10); and attest only inside, after rejecting credential-shaped ignored files (R1-11). Add the bilingual design record (R1-2), cross-link it from the local-round design, and pin the recipe's trust lines in tests so reverting them turns the suite red. * fix(verify-pr): address round-2 verification findings Round 2 at3509c2d1aareported one Low and four Suggestions. - Relay the exact <basePath>/chat/completions path. A suffix match also forwarded /admin/chat/completions to the upstream. - Pin the response content-encoding strip with a gzip upstream: undici hands the proxy decoded bytes, so forwarding the header makes the client gunzip plain text. - Reject credential-shaped ignored entries at any depth (a nested secrets/.env included) instead of only a root .env/.env.*. - Name the round's engine as the released CLI and offer the tree build for a round that must exercise the PR's own CLI. The CI inline copies keep their existing two-header strip: that lane is untouched by design and a change there needs its own workflow review. Tests: 141 passed. Three mutations verified red (exact path, gzip header, nested credential pathspec). The review-pr check at this PR is red because the bot lane exhausted ten retries on "429 rpm exceeded", not because of this diff. * fix(verify-pr): close round-3 trust-boundary findings Round 3 at3ccf38697dreported two Criticals against the recipe, one fix-induced Critical, and four Suggestions. - prepare, the credential scan, the isolation gate and the checker now all come from <trusted-clone>; prepare runs with the PR clone as its cwd so the HEAD check still applies (R2-1). - The base commit's .qwen is extracted and mounted read-only over the tree, so the round's instructions and project settings cannot come from the PR (R2-2). - The attestation is passed to the shell that starts the round, because every docker exec is a fresh shell (R1-11, fix-induced). - The proxy token is exported from proxy.json and passed with a bare -e; the file is chmod 600 and removed at cleanup (R2-3). - Connection-nominated hop-by-hop headers are stripped (R2-6). - Recipe pins now enumerate every checker call site, require a non-empty docker-run slice, and seed the writable checker snapshot so a fallback regression is caught (R2-4, R2-5). - New verify-pr-credential-scan.mjs lists ignored files one by one, refuses credential-shaped names at any depth, and leaves node_modules and dist alone — replacing the pathspec check that refused every installed or built tree (round-3 Low). Tests: 148 passed; ten mutations verified red. * fix(verify-pr): address round-4 review findings Four P2 findings at441a523927: - The credential scan now limits git enumeration with credential pathspecs and depth-aware node_modules/dist excludes, so an installed tree's multi-megabyte ignored listing never overflows the output buffer. - Those excludes apply inside workspace packages too, so packages/*/dist and packages/*/node_modules are not mistaken for project credentials. - The recipe waits for proxy.json to carry a token and validates the exported value, so a truncated background write cannot reach Docker. - The pre-header disconnect test waits until the mock upstream has recorded the request before destroying the downstream socket, instead of racing the proxy's own (correct) early abort. Tests: 150 passed; three mutations verified red (enumeration pathspecs, excludes, readiness loop). * fix(verify-pr): refuse nested repositories in the credential scan Round-4 verification at19b14b1a8fleft three Suggestions. - A nested git repository is now refused instead of silently skipped: git does not descend into it, so a .env under tmp/pr-clone/ would pass. A vendored .git under node_modules/dist stays excluded like the files there. - isExcluded is removed: the pathspecs already exclude those directories, and the surviving JS filter was unpinnable. - The spaced Connection-nomination list is pinned; dropping trim made the proxy answer 502. - The node_modules/dist credential exclusions are documented as deliberate. Tests: 152 passed; two mutations verified red (nested refusal, trim). * fix(verify-pr): address round-5 review findings Four Criticals: - The key entry works without a TTY: an interactive shell prompts, an agent's shell reads REVIEW_OPENAI_KEY_FILE and fails with a named error. - prepare disables fsmonitor, hooks and credential helpers on the PR-controlled clone, and the scan refuses credential-bearing remote URLs. - The proxy token is re-derived from proxy.json in the docker run step, because each step is its own shell. - The credential scan covers .env~, *.p12, id_rsa*, gha-creds-*, and the other credential families the repo's own .gitignore names. Suggestions: - Guard call site moved to <trusted-clone>, <clone> defined for watch mode. - The newest verify comment is snapshotted as previous-report.md. - The real key is unset once the proxy owns it; the token is unset at cleanup. - `{1..100}` replaces seq for macOS; --user and the proxy token's non-credential status are pinned; the scan abort is structural (`|| exit 1`). - Step 6 wording matches the base-pin; the docs narrow the base-pin scope for root AGENTS.md/QWEN.md, list the real CI divergence, explain content-encoding in English, fix the zh token wording, and record the checker --context contract and the ownership attribution. - The readiness gate compares the health nonce; --port is removed. Test gaps closed: gzip header assertion, mid-body disconnect, reset event, PUT, bare-base exact path, positive header passthrough, 502 transport failure, full paused-stream equality, token shape, publish --context pin, checker second positional, scan path fixtures, headers-only idle window. Tests: 159 passed; twelve mutations verified red. * fix(verify-pr): close round-5 verification findings Two Lows reproduced atb5c717a5d5: - prepare's hardening named clean filters but only disabled fsmonitor, hooks and credential helpers. It now enumerates every local filter.*.clean/smudge/process and empties it with a -c override before any git read; a planted-filter fixture proves the override with a positive control that fires without it. - The nested-repository refusal over-refused installed trees whose node_modules holds a real vendored git repo (git reports the directory entry, which the exclude pathspec cannot match). The enumeration is now segment-filtered in JS, so vendored dependencies stay excluded while a nested project repository is still refused. Suggestions and nits: - The remote-URL guard reads url.*.insteadOf as well as remote.*.url and refuses any http(s) userinfo, including a token used as the username; ssh://git@ stays benign. - Public keys (.pub) are no longer treated as credentials, and a backup.env~ fixture pins the trailing-tilde family. Tests: 163 passed; six mutations verified red (filter overrides, vendored segment filter, userinfo breadth, insteadOf enumeration, public-key exclusion, backup.env~ pathspec). * fix(verify-pr): close round-6 verification findings Co-authored-by: Qwen-Coder <[email protected]> * fix(verify-pr): close round-7 verification findings Co-authored-by: Qwen-Coder <[email protected]> * fix(verify-pr): address round-8 review findings Co-authored-by: Qwen-Coder <[email protected]> * fix(verify-pr): close round-9 review findings Co-authored-by: Qwen-Coder <[email protected]> * fix(verify-pr): close round-10 review findings Co-authored-by: Qwen-Coder <[email protected]> --------- Co-authored-by: Qwen-Coder <[email protected]>
346 lines
12 KiB
JavaScript
346 lines
12 KiB
JavaScript
#!/usr/bin/env node
|
|
|
|
/**
|
|
* @license
|
|
* Copyright 2026 Qwen Team
|
|
* SPDX-License-Identifier: Apache-2.0
|
|
*/
|
|
|
|
/**
|
|
* Host-side model proxy for a local `/verify-pr` round.
|
|
*
|
|
* A local round needs the same isolation the CI lane provides: the container
|
|
* 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 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.
|
|
*
|
|
* Usage (from a trusted checkout, never the PR-head tree — this process holds
|
|
* the real key and a PR can rewrite a script in its own checkout):
|
|
* if [ -t 0 ]; then
|
|
* read -rsp 'REVIEW_OPENAI_API_KEY: ' REVIEW_OPENAI_API_KEY; echo
|
|
* else
|
|
* IFS= read -r REVIEW_OPENAI_API_KEY \
|
|
* < ~/.config/qwen-verify/review-openai-key
|
|
* fi
|
|
* export REVIEW_OPENAI_API_KEY
|
|
* REVIEW_OPENAI_BASE_URL=<url> \
|
|
* node <trusted-clone>/scripts/verify-pr-model-proxy.mjs \
|
|
* [--listen <host>]
|
|
*
|
|
* Reading the key from a terminal when one exists and from a file otherwise
|
|
* keeps it out of shell history (an inline assignment is recorded) and keeps
|
|
* an agent-driven run, whose shell has no TTY, from blocking on `read`.
|
|
*
|
|
* Options:
|
|
* --listen <host> address to bind (default 127.0.0.1; use the Docker
|
|
* bridge address, e.g. 172.17.0.1, for container clients).
|
|
* The port is always ephemeral: a fixed port is a binding
|
|
* race the CI lane avoids too.
|
|
*
|
|
* Environment:
|
|
* REVIEW_OPENAI_BASE_URL upstream base URL, may carry a path prefix
|
|
* REVIEW_OPENAI_API_KEY the real key; never leaves this process
|
|
* QWEN_VERIFY_PROXY_TOKEN bearer clients must present (default: random)
|
|
*
|
|
* On readiness, prints one JSON line: {port, baseUrl, token, nonce}. Give the
|
|
* container `baseUrl` as the provider base URL and `token` as its credential;
|
|
* `GET /__health` echoes `nonce`. Only `POST <basePath>/chat/completions` is
|
|
* relayed — the path is matched exactly, not as a suffix.
|
|
*/
|
|
|
|
import { randomBytes } from 'node:crypto';
|
|
import http from 'node:http';
|
|
import { Readable } from 'node:stream';
|
|
import { isEntryPoint } from './lib/entry-point.js';
|
|
|
|
export const DEFAULT_IDLE_TIMEOUT_MS = 240_000;
|
|
|
|
// The qwen client tolerates 240 s of silence inside one stream
|
|
// (DEFAULT_STREAM_IDLE_TIMEOUT_MS in the OpenAI content generator), so a
|
|
// tighter proxy window would cut a completion its own client would have
|
|
// waited out. Keep the default equal; a caller may still pass its own value.
|
|
|
|
// 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' };
|
|
for (let i = 0; i < argv.length; i++) {
|
|
const arg = argv[i];
|
|
if (arg === '--listen') {
|
|
const value = argv[++i];
|
|
// An empty value would reach server.listen(port, ''), which binds
|
|
// every interface — past the bridge this proxy is meant to stay on.
|
|
if (value === undefined || value === '' || value.startsWith('--')) {
|
|
throw new Error('--listen requires a value');
|
|
}
|
|
options.listen = value;
|
|
} else {
|
|
throw new Error(`unknown argument: ${arg}`);
|
|
}
|
|
}
|
|
return options;
|
|
}
|
|
|
|
/** Strips the trailing slashes a base path may carry; `''` means none. */
|
|
export function normalizeBasePath(pathname) {
|
|
const trimmed = (pathname ?? '').replace(/\/+$/, '');
|
|
return trimmed === '/' ? '' : trimmed;
|
|
}
|
|
|
|
/**
|
|
* Assembles the base URL the maintainer hands to the container. A host that
|
|
* carries a colon is an IPv6 literal and needs brackets, or `new URL()`
|
|
* rejects the string downstream.
|
|
*/
|
|
export function formatLocalBaseUrl(listen, port, basePath = '') {
|
|
const host = listen.includes(':') ? `[${listen}]` : listen;
|
|
return `http://${host}:${port}${basePath}`;
|
|
}
|
|
|
|
/**
|
|
* Returns an http.Server that relays only POST /chat/completions to the
|
|
* upstream, replacing the client's bearer token with the real key. The idle
|
|
* watchdog is per-chunk, not total: qwen tolerates minutes of silence inside
|
|
* one stream, so a total cap would cut healthy long completions. It also
|
|
* stays quiet while the downstream is paused — backpressure from a slow but
|
|
* live client is not upstream idleness — and re-arms on 'drain'.
|
|
*/
|
|
export function createModelProxy({
|
|
baseUrl,
|
|
apiKey,
|
|
token,
|
|
nonce,
|
|
idleTimeoutMs = DEFAULT_IDLE_TIMEOUT_MS,
|
|
}) {
|
|
if (!baseUrl || !apiKey || !token || !nonce) {
|
|
throw new Error('baseUrl, apiKey, token and nonce are all required');
|
|
}
|
|
const base = new URL(baseUrl);
|
|
const basePath = normalizeBasePath(base.pathname);
|
|
|
|
return http.createServer(async (req, res) => {
|
|
if (req.url === '/__health') {
|
|
// Identity, not just liveness: a squatter cannot know the nonce.
|
|
res.writeHead(200, { 'content-type': 'text/plain' });
|
|
res.end(nonce);
|
|
return;
|
|
}
|
|
|
|
let timer;
|
|
try {
|
|
const incoming = new URL(req.url || '/', 'http://127.0.0.1');
|
|
const target = new URL(base.origin);
|
|
|
|
let path = incoming.pathname;
|
|
if (
|
|
basePath &&
|
|
basePath !== '/' &&
|
|
path !== basePath &&
|
|
!path.startsWith(`${basePath}/`)
|
|
) {
|
|
path = `${basePath}${path.startsWith('/') ? '' : '/'}${path}`;
|
|
}
|
|
target.pathname = path;
|
|
target.search = incoming.search;
|
|
|
|
// Exact path, not a suffix: `endsWith('/chat/completions')` would also
|
|
// relay `/admin/chat/completions` to the upstream.
|
|
const allowedPath = basePath
|
|
? `${basePath}/chat/completions`
|
|
: '/chat/completions';
|
|
if (req.method !== 'POST' || target.pathname !== allowedPath) {
|
|
res.writeHead(403, { 'content-type': 'text/plain' });
|
|
res.end(`proxy: only POST ${allowedPath} is allowed\n`);
|
|
return;
|
|
}
|
|
|
|
// Only the agent knows this run's token; anything else that finds the
|
|
// port cannot get the key used.
|
|
if (req.headers.authorization !== `Bearer ${token}`) {
|
|
res.writeHead(401, { 'content-type': 'text/plain' });
|
|
res.end('proxy: unauthorized\n');
|
|
return;
|
|
}
|
|
|
|
const headers = new Headers(req.headers);
|
|
for (const name of STRIPPED_REQUEST_HEADERS) headers.delete(name);
|
|
// RFC 7230 §6.1: a header nominated by Connection is hop-by-hop too,
|
|
// so a client cannot smuggle one through under an unlisted name.
|
|
const nominated = String(req.headers.connection ?? '')
|
|
.split(',')
|
|
.map((name) => name.trim().toLowerCase())
|
|
.filter(Boolean);
|
|
for (const name of nominated) headers.delete(name);
|
|
headers.set('authorization', `Bearer ${apiKey}`);
|
|
|
|
const init = { method: req.method, headers };
|
|
init.body = req;
|
|
init.duplex = 'half';
|
|
|
|
const controller = new AbortController();
|
|
let body = null;
|
|
let finished = false;
|
|
const arm = () => {
|
|
if (finished) return;
|
|
clearTimeout(timer);
|
|
timer = setTimeout(() => {
|
|
// A paused body is downstream backpressure, not upstream silence:
|
|
// the client is alive but slow, and aborting here would cut a
|
|
// healthy completion. The 'drain' listener re-arms this window.
|
|
if (body?.isPaused()) {
|
|
arm();
|
|
return;
|
|
}
|
|
controller.abort();
|
|
}, idleTimeoutMs);
|
|
};
|
|
// Registered before the fetch: a client that disconnects while the
|
|
// upstream headers are still in flight has already emitted 'close', so
|
|
// registering after the response would never abort that request.
|
|
res.on('close', () => {
|
|
finished = true;
|
|
clearTimeout(timer);
|
|
controller.abort();
|
|
});
|
|
res.on('drain', arm);
|
|
// A slow request UPLOAD is activity too: fetch resolves only on the
|
|
// upstream headers, so without this a large prompt on a slow link
|
|
// would be torn down mid-upload by a window meant for stream silence.
|
|
// 'readable', not 'data': a data listener switches the request to
|
|
// flowing mode and steals the body chunks from the relayed stream.
|
|
req.on('readable', arm);
|
|
arm();
|
|
let upstream;
|
|
try {
|
|
upstream = await fetch(target, { ...init, signal: controller.signal });
|
|
} catch (error) {
|
|
if (error instanceof Error && error.name === 'AbortError') {
|
|
if (!res.destroyed && !res.writableEnded) {
|
|
res.writeHead(504, { 'content-type': 'text/plain' });
|
|
res.end('proxy error: upstream request timed out\n');
|
|
}
|
|
return;
|
|
}
|
|
throw error;
|
|
}
|
|
if (res.destroyed) {
|
|
finished = true;
|
|
clearTimeout(timer);
|
|
return;
|
|
}
|
|
// The timer is deliberately NOT cleared here: fetch() resolves on
|
|
// HEADERS, and the armed timer doubles as the first idle window for
|
|
// the body; the per-chunk handler below refreshes it.
|
|
const responseHeaders = {};
|
|
upstream.headers.forEach((value, key) => {
|
|
const lower = key.toLowerCase();
|
|
if (lower !== 'content-encoding' && lower !== 'content-length') {
|
|
responseHeaders[key] = value;
|
|
}
|
|
});
|
|
res.writeHead(upstream.status, responseHeaders);
|
|
if (upstream.body) {
|
|
body = Readable.fromWeb(upstream.body);
|
|
const done = () => {
|
|
finished = true;
|
|
clearTimeout(timer);
|
|
};
|
|
body.on('end', done);
|
|
// An idle watchdog firing mid-body aborts the upstream, which
|
|
// surfaces here as an error: tear down the downstream response too.
|
|
body.on('error', () => {
|
|
done();
|
|
res.destroy();
|
|
});
|
|
body.pipe(res);
|
|
body.on('data', arm);
|
|
} else {
|
|
finished = true;
|
|
clearTimeout(timer);
|
|
res.end();
|
|
}
|
|
} catch (error) {
|
|
clearTimeout(timer);
|
|
console.error('proxy upstream failure:', error);
|
|
res.writeHead(502, { 'content-type': 'text/plain' });
|
|
res.end('proxy error: upstream request failed\n');
|
|
}
|
|
});
|
|
}
|
|
|
|
async function main(argv) {
|
|
let options;
|
|
try {
|
|
options = parseArgs(argv);
|
|
} catch (error) {
|
|
process.stderr.write(`${error.message}\n`);
|
|
return 2;
|
|
}
|
|
const baseUrl = process.env.REVIEW_OPENAI_BASE_URL;
|
|
const apiKey = process.env.REVIEW_OPENAI_API_KEY;
|
|
if (!baseUrl || !apiKey) {
|
|
process.stderr.write(
|
|
'REVIEW_OPENAI_BASE_URL and REVIEW_OPENAI_API_KEY must both be set\n',
|
|
);
|
|
return 2;
|
|
}
|
|
const token =
|
|
process.env.QWEN_VERIFY_PROXY_TOKEN || randomBytes(24).toString('hex');
|
|
const nonce = randomBytes(24).toString('hex');
|
|
const server = createModelProxy({ baseUrl, apiKey, token, nonce });
|
|
await new Promise((resolve, reject) => {
|
|
server.once('error', reject);
|
|
server.listen(0, options.listen, resolve);
|
|
});
|
|
const { port } = server.address();
|
|
const localBaseUrl = formatLocalBaseUrl(
|
|
options.listen,
|
|
port,
|
|
normalizeBasePath(new URL(baseUrl).pathname),
|
|
);
|
|
// One JSON line, on stdout: the maintainer pastes baseUrl/token into the
|
|
// container's provider configuration. The real key is never printed.
|
|
process.stdout.write(
|
|
`${JSON.stringify({ port, baseUrl: localBaseUrl, token, nonce })}\n`,
|
|
);
|
|
return 0;
|
|
}
|
|
|
|
if (isEntryPoint(import.meta.url, process.argv[1])) {
|
|
main(process.argv.slice(2)).then(
|
|
(code) => {
|
|
process.exitCode = code;
|
|
},
|
|
(error) => {
|
|
console.error(error);
|
|
process.exitCode = 1;
|
|
},
|
|
);
|
|
}
|