diff --git a/packages/cli/src/serve/auth/deviceFlow.test.ts b/packages/cli/src/serve/auth/deviceFlow.test.ts index 6c298170cae..ff931e64fa3 100644 --- a/packages/cli/src/serve/auth/deviceFlow.test.ts +++ b/packages/cli/src/serve/auth/deviceFlow.test.ts @@ -733,20 +733,31 @@ describe('DeviceFlowRegistry — authoritative timeouts (fold-in 7)', () => { 'see daemon audit log for details', ); } - // Audit captures the timeout for the operator. Critically, the - // audit hint MUST NOT route through the `provider.poll() threw - // (raw)` template — that's reserved for actual provider throws - // and would mis-direct triage. On the timeout path the hint - // field is omitted entirely (rawProviderError stays undefined). + // Audit captures the timeout for the operator. The hint must + // NOT route through the misleading `provider.poll() threw (raw)` + // template (that's reserved for real provider throws), and — + // PR #4291 follow-up review (qwen-latest, round-4 #5) — the hint + // MUST be present so operators reading the durable audit trail + // can distinguish timeout from generic upstream_error. Audit + // hint must match the SSE hint exactly. const auditFailure = auditLines.find( (line) => line['status'] === 'failed' && line['errorKind'] === 'upstream_error', ); expect(auditFailure).toBeDefined(); const auditHint = auditFailure?.['hint'] as string | undefined; - if (auditHint !== undefined) { - expect(auditHint).not.toContain('provider.poll() threw (raw)'); - } + expect(auditHint).toBeDefined(); + expect(auditHint).not.toContain('provider.poll() threw (raw)'); + expect(auditHint).toContain('timed out after'); + expect(auditHint).toContain('check IdP connectivity'); + // PR #4291 follow-up review (qwen-latest, round-4 #3): the + // timeout sentinel built ONCE per timer-fire — `signal.reason` + // and the rejection should be the SAME instance. Pin: the + // signal we observed is aborted with a DeviceFlowPollTimeoutError + // reason. + expect(provider.lastPollSignal?.aborted).toBe(true); + const reason = provider.lastPollSignal?.reason as unknown; + expect(reason).toBeInstanceOf(DeviceFlowPollTimeoutError); // PR #4291 follow-up review (Qwen Code review summary): // poll-tick must NOT reschedule itself after a timeout-driven // upstream_error (the entry has already transitioned to error @@ -913,8 +924,15 @@ describe('DeviceFlowRegistry — authoritative timeouts (fold-in 7)', () => { env.scheduler.flushDue(env.clock.now); await flushAsync(); // Late upstream failure (NOT our own DeviceFlowPollTimeoutError). - // Use a long-enough message to exercise the truncation tail. - const longDetail = `connection reset by peer ${'x'.repeat(400)}`; + // Use a long message that, in the pre-round-4 code, would have + // been truncated to its first 256 bytes — and those 256 bytes + // can carry a full RFC 8628 `device_code` (≤80 chars) verbatim + // if the upstream wrapper templates it into the response. The + // round-4 #7 fix switches to the same `name + length` pattern + // the provider catch uses, so the raw detail never reaches + // stderr / audit even when settled late. + const seededDeviceCode = 'device-code-secret-AAAA1111'; + const longDetail = `connection reset by peer ${seededDeviceCode} ${'x'.repeat(400)}`; rejectLate(new Error(longDetail)); await flushAsync(); const lateAudit = auditLines.find((line) => @@ -924,25 +942,43 @@ describe('DeviceFlowRegistry — authoritative timeouts (fold-in 7)', () => { ); expect(lateAudit).toBeDefined(); expect(lateAudit?.['errorKind']).toBe('upstream_error'); - expect(lateAudit?.['hint']).toContain('rejected after'); - expect(lateAudit?.['hint']).toContain( - `${DEVICE_FLOW_POLL_TIMEOUT_MS}ms ceiling`, - ); - expect(lateAudit?.['hint']).toContain('connection reset by peer'); - // Truncation tail must appear (long detail > 256 bytes). - expect(lateAudit?.['hint']).toContain('bytes]'); + const auditHint = lateAudit?.['hint'] as string; + expect(auditHint).toContain('rejected after'); + expect(auditHint).toContain(`${DEVICE_FLOW_POLL_TIMEOUT_MS}ms ceiling`); + // PR #4291 follow-up review (qwen-latest, round-4 #7): the + // late-rejection observer must use the `name + length` pattern, + // NOT the raw message slice. Hard-negate the seeded device_code + // to pin the security regression — a future change that goes + // back to slicing `lateErr.message` would fail CI immediately. + expect(auditHint).toContain('Error (message'); + expect(auditHint).toContain('bytes; raw suppressed)'); + expect(auditHint).not.toContain(seededDeviceCode); + expect(auditHint).not.toContain('connection reset by peer'); } finally { registry.dispose(); } }); - it("does NOT double-audit when late rejection is the registry's own DeviceFlowPollTimeoutError (qwen-latest review N2 guard)", async () => { - // PR #4291 follow-up review (qwen-latest, N2): the late-rejection - // observer must filter out our own timer rejection — otherwise a - // single timeout would produce two audit lines (one from the - // wrapper catch, one from the late-rejection observer). The - // guard is `if (lateErr instanceof DeviceFlowPollTimeoutError) - // return;`. Pin it. + it('audits a provider-thrown DeviceFlowPollTimeoutError as a real failure (round-6 #4: brand-aware self-filter)', async () => { + // Round-6 review (qwen-latest, #4): the self-filter guard in the + // late-rejection observer must check the runtime brand + // (`_isRegistryTimeout === true`), NOT bare `instanceof`. Reason: + // `DeviceFlowPollTimeoutError` is `export class` (the test file + // needs the constructor for fixture purposes), so a non-conforming + // provider that imported and threw `new DeviceFlowPollTimeoutError(...)` + // would otherwise spoof "I caused the timeout" — silently swallowed + // by the filter and never audited. Pin the inverted scenario: + // brand-false provider throw IS audited as `lost_late_poll_after_timeout`. + // + // Note: this test'\''s setup mirrors the natural late-rejection path + // (registry race timer fires first, then the provider'\''s promise + // settles late with the rejection). The late-observer'\''s filter is + // exercised in two ways across this and the next test: + // - here: brand-FALSE → late audit DOES appear (this test) + // - elsewhere: brand-TRUE registry timeout → no late audit (the + // hanging-provider test below covers the natural happy-path + // since the registry'\''s real timeout is brand-true and the + // promise never settles late) const provider = new FakeProvider(); let rejectLate!: (e: Error) => void; const latePollPromise = new Promise( @@ -964,18 +1000,275 @@ describe('DeviceFlowRegistry — authoritative timeouts (fold-in 7)', () => { env.clock.tick(DEVICE_FLOW_POLL_TIMEOUT_MS + 1); env.scheduler.flushDue(env.clock.now); await flushAsync(); - // Reject with the SAME sentinel the registry uses internally. - // The original wrapper catch already recorded a failed audit - // (the "real" failure for this entry). The late observer must - // NOT add a `lost_late_poll_after_timeout` line. - rejectLate(new DeviceFlowPollTimeoutError(DEVICE_FLOW_POLL_TIMEOUT_MS)); + // Provider throws the EXPORTED class directly (brand-false). The + // round-5 shape would have silently filtered this; round-6 audits + // it correctly as a real failure. + const providerThrown = new DeviceFlowPollTimeoutError( + DEVICE_FLOW_POLL_TIMEOUT_MS, + ); + expect(providerThrown._isRegistryTimeout).toBe(false); + rejectLate(providerThrown); await flushAsync(); const lateAudits = auditLines.filter((line) => (line['hint'] as string | undefined)?.includes( 'lost_late_poll_after_timeout', ), ); - expect(lateAudits).toHaveLength(0); + // Brand-false → NOT filtered, audited as a real late rejection. + expect(lateAudits.length).toBeGreaterThanOrEqual(1); + expect(lateAudits[0]?.['errorKind']).toBe('upstream_error'); + expect(lateAudits[0]?.['hint']).toContain('rejected after'); + } finally { + registry.dispose(); + } + }); + + // (Round-5 N2's "filter out registry's own timeout" guard is now + // brand-aware — see round-6 #4 above. The natural happy-path "no + // double-audit when the registry's own race timer settles the + // wrapper" is implicitly covered by the hanging-provider test + // earlier, which exercises a brand-true `makeRegistryPollTimeoutError` + // via the actual race-timer path; the provider's promise never + // settles late, so the late-observer's brand-true filter is the only + // thing keeping a phantom `lost_late_poll_after_timeout` from + // appearing alongside the wrapper-catch audit.) + + it('does NOT attach late-poll observer when the provider beats the timeout (round-5 #1: pollTimedOut race)', async () => { + // PR #4291 follow-up review (deepseek-v4-pro, round-5 #1): the + // `pollTimedOut = true` flag was previously set unconditionally + // inside the timer callback. If the provider settled the wrapper + // first (e.g., at 29.9s), the timer callback could still fire + // afterwards in a tight race, mark the flag, and the late-observer + // would attach to an already-settled promise — emitting a + // spurious `lost_late_poll_after_timeout` for a flow that + // completed within the ceiling. Fix: set the flag in the catch + // block only when `err instanceof DeviceFlowPollTimeoutError`. + // Pin: provider responds with `pending` BEFORE the race timer + // fires; assert NO late audit. + const provider = new FakeProvider(); + provider.pollScript = [{ kind: 'pending' }]; + const built = buildRegistry(provider); + const { registry, env, auditLines } = built; + try { + await registry.start({ providerId: 'qwen-oauth' }); + // Drive the first poll. Provider returns synchronously; wrapper + // resolves, finally clears the timer. + env.clock.tick(DEVICE_FLOW_DEFAULT_INTERVAL_MS + 1); + env.scheduler.flushDue(env.clock.now); + await flushAsync(); + expect(provider.pollCount).toBe(1); + // Tick well past POLL_TIMEOUT_MS to confirm the timer was + // properly cleared. If `pollTimedOut` had been set in the + // callback, this would attach a late observer + audit line. + env.clock.tick(DEVICE_FLOW_POLL_TIMEOUT_MS * 2); + env.scheduler.flushDue(env.clock.now); + await flushAsync(); + const spuriousLate = auditLines.find((line) => + (line['hint'] as string | undefined)?.includes( + 'lost_late_poll_after_timeout', + ), + ); + expect(spuriousLate).toBeUndefined(); + } finally { + registry.dispose(); + } + }); + + it('sanitizes hostile latePollResult.kind in late-observer audit (round-5 #3)', async () => { + // PR #4291 follow-up review (deepseek-v4-pro, round-5 #3): a + // non-conforming provider could return `{kind: ''}`. The audit hint interpolates `kind` + // directly — without sanitization, that's a log-forging vector + // even though the typed shape is `'pending' | 'slow_down' | ...`. + const provider = new FakeProvider(); + let resolveLate!: (r: DeviceFlowPollResult) => void; + const latePollPromise = new Promise((resolve) => { + resolveLate = resolve; + }); + provider.poll = async () => { + provider.pollCount += 1; + return latePollPromise; + }; + const built = buildRegistry(provider); + const { registry, env, auditLines } = built; + try { + await registry.start({ providerId: 'qwen-oauth' }); + env.clock.tick(DEVICE_FLOW_DEFAULT_INTERVAL_MS + 1); + env.scheduler.flushDue(env.clock.now); + await flushAsync(); + env.clock.tick(DEVICE_FLOW_POLL_TIMEOUT_MS + 1); + env.scheduler.flushDue(env.clock.now); + await flushAsync(); + // Non-conforming late resolve with hostile `kind`. (Casting + // to `DeviceFlowPollResult` simulates a provider violating the + // typed contract at runtime.) + const hostile = 'pending\n[serve] FORGED LINE\x1b[31m'; + resolveLate({ kind: hostile } as unknown as DeviceFlowPollResult); + await flushAsync(); + const lateAudit = auditLines.find((line) => + (line['hint'] as string | undefined)?.includes( + 'lost_late_poll_after_timeout', + ), + ); + expect(lateAudit).toBeDefined(); + const hint = lateAudit?.['hint'] as string; + // The forged log line text MUST NOT lead a real newline. + expect(hint.split('\n').length).toBe(1); + expect(hint).not.toContain('\x1b[31m'); + // Substantive parts preserved (`?`-replaced). + expect(hint).toContain('FORGED LINE'); + } finally { + registry.dispose(); + } + }); + + it('sanitizes hostile lateErr.name in late-rejection observer audit (round-5 #2)', async () => { + // PR #4291 follow-up review (deepseek-v4-pro, round-5 #2): same + // log-injection vector via `Error.name` (freely assignable). The + // round-4 fix used name+length but didn't sanitize `name` itself. + const provider = new FakeProvider(); + let rejectLate!: (e: Error) => void; + const latePollPromise = new Promise( + (_resolve, reject) => { + rejectLate = reject; + }, + ); + provider.poll = async () => { + provider.pollCount += 1; + return latePollPromise; + }; + const built = buildRegistry(provider); + const { registry, env, auditLines } = built; + try { + await registry.start({ providerId: 'qwen-oauth' }); + env.clock.tick(DEVICE_FLOW_DEFAULT_INTERVAL_MS + 1); + env.scheduler.flushDue(env.clock.now); + await flushAsync(); + env.clock.tick(DEVICE_FLOW_POLL_TIMEOUT_MS + 1); + env.scheduler.flushDue(env.clock.now); + await flushAsync(); + const hostileErr = new Error('upstream HTTP 502'); + hostileErr.name = 'Hostile\n[serve] FORGED ERR LINE\x1b[31m'; + rejectLate(hostileErr); + await flushAsync(); + const lateAudit = auditLines.find((line) => + (line['hint'] as string | undefined)?.includes( + 'lost_late_poll_after_timeout', + ), + ); + expect(lateAudit).toBeDefined(); + const hint = lateAudit?.['hint'] as string; + expect(hint.split('\n').length).toBe(1); + expect(hint).not.toContain('\x1b[31m'); + expect(hint).toContain('FORGED ERR LINE'); + } finally { + registry.dispose(); + } + }); + + it('survives a throwing audit sink in the late-poll observer (round-6 #2: terminal .catch())', async () => { + // PR #4291 follow-up review (qwen-latest, round-6 #2): the late-poll + // `void tracked.then(...)` was missing a terminal `.catch(() => {})`. + // If `audit.record` throws synchronously inside either handler (a + // misbehaving sink: throwing on backpressure, on a malformed + // payload, on out-of-disk for a file sink), the resulting promise + // rejects unhandled. Node 22's default + // `--unhandled-rejections=throw` would crash the daemon. Pin the + // resilience: a poison audit sink in the late-resolve path must + // NOT throw out of `flushAsync()`. + const provider = new FakeProvider(); + let resolveLate!: (r: DeviceFlowPollResult) => void; + const latePollPromise = new Promise((resolve) => { + resolveLate = resolve; + }); + provider.poll = async () => { + provider.pollCount += 1; + return latePollPromise; + }; + const env = makeClockAndScheduler(); + const events = makeEventSink(); + const allRecords: Array> = []; + const registry = new DeviceFlowRegistry({ + events: events.sink, + audit: { + // Throw ONLY for the late-observer's audit call (identified by + // its `lost_late_poll_after_timeout` hint). Earlier audit calls + // (start, in-flight poll-timeout failure) record normally so + // we exercise the specific code path the round-6 #2 fix targets. + record: (line) => { + allRecords.push({ ...line }); + if ( + typeof line.hint === 'string' && + line.hint.includes('lost_late_poll_after_timeout') + ) { + throw new Error('audit sink crashed during late-observer call'); + } + }, + }, + resolveProvider: (id) => (id === 'qwen-oauth' ? provider : undefined), + now: env.now, + schedule: env.schedule as never, + scheduleInterval: env.scheduleInterval as never, + clearScheduled: env.clearScheduled as never, + clearScheduledInterval: env.clearScheduledInterval as never, + }); + try { + await registry.start({ providerId: 'qwen-oauth' }); + env.clock.tick(DEVICE_FLOW_DEFAULT_INTERVAL_MS + 1); + env.scheduler.flushDue(env.clock.now); + await flushAsync(); + env.clock.tick(DEVICE_FLOW_POLL_TIMEOUT_MS + 1); + env.scheduler.flushDue(env.clock.now); + await flushAsync(); + // Resolve late — the late-resolve handler will call audit.record + // which throws. The terminal `.catch(() => {})` swallows it. + resolveLate({ kind: 'pending' }); + // If the chain were unhandled, this `flushAsync()` would surface + // an unhandled-rejection warning on Node 22. With `.catch()` + // attached, it completes cleanly. + await expect(flushAsync()).resolves.toBeUndefined(); + } finally { + registry.dispose(); + } + }); + + it('sanitizes rawProviderError before interpolating into the audit hint (round-6 #3)', async () => { + // PR #4291 follow-up review (qwen-latest, round-6 #3): the + // `case 'error'` audit branch interpolates the captured + // `rawProviderError` (raw `err.message`) into the hint. Per ES2019+ + // `JSON.stringify` no longer escapes U+2028 / U+2029 (they're + // valid JSON), so a hostile provider throw with those characters + // in `err.message` would otherwise forge log lines downstream. + // Apply `sanitizeForStderr` before interpolation; pin via a + // hostile message containing U+2028 + ANSI escape. + const U_2028 = '\u2028'; + const provider = new FakeProvider(); + provider.poll = async () => { + provider.pollCount += 1; + throw new Error( + `upstream${U_2028}[serve] FORGED PROVIDER LINE\x1b[31mRED`, + ); + }; + const built = buildRegistry(provider); + const { registry, env, auditLines } = built; + try { + await registry.start({ providerId: 'qwen-oauth' }); + env.clock.tick(DEVICE_FLOW_DEFAULT_INTERVAL_MS + 1); + env.scheduler.flushDue(env.clock.now); + await flushAsync(); + const auditLine = auditLines.find((line) => + (line['hint'] as string | undefined)?.includes( + 'provider.poll() threw (raw)', + ), + ); + expect(auditLine).toBeDefined(); + const hint = auditLine?.['hint'] as string; + // U+2028 is replaced with `?`. ANSI escape replaced too. + expect(hint).not.toContain(U_2028); + expect(hint).not.toContain('\x1b[31m'); + // Substantive parts preserved. + expect(hint).toContain('FORGED PROVIDER LINE'); + expect(hint).toContain('RED'); } finally { registry.dispose(); } diff --git a/packages/cli/src/serve/auth/deviceFlow.ts b/packages/cli/src/serve/auth/deviceFlow.ts index 7e06ecda553..74727cbeb34 100644 --- a/packages/cli/src/serve/auth/deviceFlow.ts +++ b/packages/cli/src/serve/auth/deviceFlow.ts @@ -25,6 +25,55 @@ import { randomUUID } from 'node:crypto'; +/** + * Strip / replace bytes that could forge log lines or inject terminal + * control sequences when interpolated into a stderr / audit breadcrumb. + * + * PR #4291 follow-up review (gpt-5.5, round-3 #2): originally lived in + * `qwenDeviceFlowProvider.ts` to sanitize the attacker-controlled + * `oauthError` field. PR #4291 follow-up review (deepseek-v4-pro, + * round-5 #2/#3): the registry's late-poll observer also interpolates + * provider-controlled values (`latePollResult.kind`, `lateErr.name`) + * into audit hints; same sanitization vector. Lifted to `deviceFlow.ts` + * so both layers share a single helper — without exporting it from a + * lower-level module, qwenDeviceFlowProvider couldn't import it + * (deviceFlow is the foundation; provider depends on deviceFlow, not + * the other way around). + * + * PR #4291 follow-up review (deepseek-v4-pro, round-5 #4): regex + * extended beyond ASCII C0/C1 + DEL to cover Unicode lookalike + * controls a malicious IdP could use to bypass the ASCII-only filter: + * - U+200B–U+200F: zero-width characters + LRM/RLM (invisible but + * can alter terminal rendering) + * - U+2028–U+2029: LINE / PARAGRAPH SEPARATOR — rendered as newlines + * in many Unicode-aware terminals; the most direct log-forging vector + * - U+202A–U+202E: bidirectional EMBEDDING / OVERRIDE controls + * - U+2066–U+2069: bidirectional ISOLATE controls (LRI / RLI / FSI / PDI) — + * the primary CVE-2021-42574 ("Trojan Source") attack vectors. A hostile + * IdP swapping U+2066 (LRI) for U+202D (LRO) would otherwise bypass the + * embedding/override range entirely while achieving the same bidi visual + * reordering. Round-5 shipped without these; round-6 review caught it. + * - U+FEFF: BYTE ORDER MARK / zero-width no-break space + * + * Replaces each with `?` so the operator can still see SOMETHING was + * present at that index (length-preserving) instead of silently dropping. + */ +const SANITIZE_FOR_STDERR_RE = new RegExp( + // ASCII C0 (0x00–0x1f), DEL (0x7f), C1 (0x80–0x9f) — the original + // round-3 coverage. Plus Unicode lookalikes added in round-5: + // \u200b–\u200f: zero-width chars + LRM/RLM + // \u2028–\u2029: LINE / PARAGRAPH SEPARATOR (terminal newline equivalents) + // \u202a–\u202e: bidirectional EMBEDDING / OVERRIDE controls + // \u2066–\u2069: bidirectional ISOLATE controls (CVE-2021-42574) + // \ufeff: BOM / ZWNBSP + String.raw`[\x00-\x1f\x7f-\x9f\u200b-\u200f\u2028-\u202e\u2066-\u2069\ufeff]`, + 'g', +); + +export function sanitizeForStderr(value: string): string { + return value.replace(SANITIZE_FOR_STDERR_RE, '?'); +} + export const DEVICE_FLOW_DEFAULT_INTERVAL_MS = 5_000; export const DEVICE_FLOW_TERMINAL_GRACE_MS = 5 * 60_000; export const DEVICE_FLOW_SWEEP_INTERVAL_MS = 30_000; @@ -619,10 +668,30 @@ export class UpstreamDeviceFlowError extends Error { * provider code when the actual issue is a hung IdP / network * partition. The sentinel lets the catch differentiate the two and * emit a timeout-specific audit + hint. + * + * **Public-export caveat (round-6 review #4):** the class is exported + * only because the test file needs to construct it for the sentinel- + * filter regression test. Providers MUST NOT throw this type — that + * would spoof the registry's "I caused the timeout" signal. The + * registry uses the `_isRegistryTimeout` runtime brand below (NOT + * `instanceof`) to gate `pollTimedOut`, so a provider that imports + * + throws `new DeviceFlowPollTimeoutError(...)` STILL routes through + * the generic provider-throw audit path — the sentinel is brand-only + * for objects the registry constructed itself. */ export class DeviceFlowPollTimeoutError extends Error { readonly code = 'poll_timeout'; readonly timeoutMs: number; + /** + * Runtime brand the registry sets ONLY on instances it constructed + * inside its own timer callback. Default `false` for any + * `new DeviceFlowPollTimeoutError(...)` call from outside the registry + * (provider code, test fixtures via the public constructor, etc.). + * Round-6 review (qwen-latest, #4): without this, a provider that + * imported the exported class and threw it would set `pollTimedOut` + * spuriously and attach a phantom late-poll observer. + */ + readonly _isRegistryTimeout: boolean = false; constructor(timeoutMs: number) { super(`device-flow poll timeout after ${timeoutMs}ms`); this.name = 'DeviceFlowPollTimeoutError'; @@ -630,6 +699,24 @@ export class DeviceFlowPollTimeoutError extends Error { } } +/** + * Internal-only constructor: build a `DeviceFlowPollTimeoutError` whose + * `_isRegistryTimeout` brand is `true`. The only call site is the + * registry's own race-timer callback. Keeping this as a separate + * unexported function (vs. a constructor flag) makes it grep-easy to + * audit "every place we mint a real timeout error" while the public + * `new DeviceFlowPollTimeoutError(ms)` path stays brand-`false`. + */ +function makeRegistryPollTimeoutError( + timeoutMs: number, +): DeviceFlowPollTimeoutError { + const err = new DeviceFlowPollTimeoutError(timeoutMs); + // The brand is declared `readonly` for callers; this assignment is + // the registry's privileged construction site. + (err as { _isRegistryTimeout: boolean })._isRegistryTimeout = true; + return err; +} + /** * Typed accessors for parking the `DeviceFlowRegistry` on * `express.Application['locals']`. The string key is shared between @@ -1104,15 +1191,30 @@ export class DeviceFlowRegistry { try { result = await new Promise((resolve, reject) => { pollTimer = this.schedule(DEVICE_FLOW_POLL_TIMEOUT_MS, () => { - pollTimedOut = true; + // PR #4291 follow-up review (qwen-latest, round-4 #3): build the + // sentinel ONCE so `signal.reason.stack` and the caught + // rejection's stack point to the same throw site (operators + // grepping the audit see one timeout, not two with divergent + // stacks). Each `new Error(...)` triggers V8 stack-trace + // capture; reusing avoids the duplicate cost too. + const timeoutError = makeRegistryPollTimeoutError( + DEVICE_FLOW_POLL_TIMEOUT_MS, + ); try { - entry.cancelController.abort( - new DeviceFlowPollTimeoutError(DEVICE_FLOW_POLL_TIMEOUT_MS), - ); + entry.cancelController.abort(timeoutError); } catch { // best-effort } - reject(new DeviceFlowPollTimeoutError(DEVICE_FLOW_POLL_TIMEOUT_MS)); + reject(timeoutError); + // PR #4291 follow-up review (deepseek-v4-pro, round-5 #1): do + // NOT set `pollTimedOut = true` here. If the provider settled + // the wrapper at 29.9s, the `await` unblocks and `finally` + // calls `clearScheduled(pollTimer)` — but if the timer + // callback was already queued for execution before the clear + // landed, this branch can still run and incorrectly mark + // `pollTimedOut`. Move the flag to the catch block where the + // settled cause is unambiguous (`err instanceof + // DeviceFlowPollTimeoutError`). }); providerPollPromise = provider.poll( { @@ -1143,7 +1245,27 @@ export class DeviceFlowRegistry { // "provider bug" and waste time investigating provider code. // Branch on `DeviceFlowPollTimeoutError` to use a dedicated // hint + suppress the misleading "raw" audit path. - if (err instanceof DeviceFlowPollTimeoutError) { + // + // PR #4291 follow-up review (qwen-latest, round-6 #4): the gate + // is the runtime brand `_isRegistryTimeout`, NOT bare + // `instanceof`. The class is `export`ed (the test file needs the + // constructor for the sentinel-filter regression test), so a + // misbehaving provider that imported it and threw `new + // DeviceFlowPollTimeoutError(...)` would otherwise spoof the + // "I caused the timeout" signal. Only the registry's own + // `makeRegistryPollTimeoutError` helper sets the brand to true; + // a provider's `new ...` produces an instance with brand `false`, + // which falls through to the generic provider-throw path. + if ( + err instanceof DeviceFlowPollTimeoutError && + err._isRegistryTimeout === true + ) { + // PR #4291 follow-up review (deepseek-v4-pro, round-5 #1): the + // catch is the canonical place to confirm "the wrapper settled + // BECAUSE of our timer." Setting the flag here (not in the + // timer callback) proves the late-observer attaches only when + // the provider genuinely lost the race. + pollTimedOut = true; result = { kind: 'error', errorKind: 'upstream_error', @@ -1172,52 +1294,112 @@ export class DeviceFlowRegistry { // `lost_success_after_timeout` pattern on the persist path. if (pollTimedOut && providerPollPromise !== undefined) { const tracked = providerPollPromise; + // PR #4291 follow-up review (qwen-latest, round-4 #1): destructure + // the few entry fields we actually need so the observer closure + // does NOT capture `entry` by reference. Otherwise, if the + // tracked promise never settles (the exact scenario this audit + // exists to catch), the closure would retain the entire entry + // — including `deviceCode` / `pkceVerifier` BrandedSecrets and + // the live `cancelController` — for the lifetime of the daemon. + // Memory leak + indefinite secret retention. + const auditDeviceFlowId = entry.deviceFlowId; + const auditProviderId = entry.providerId; + const auditClientId = entry.initiatorClientId; + const audit = this.deps.audit; // Detached on purpose; the catch on the ORIGINAL promise has // already happened (via the wrapper) — the observer below // sees the eventual settlement of the same promise. Both // success and error branches go through audit only. - void tracked.then( - (latePollResult) => { - this.deps.audit?.record({ - deviceFlowId: entry.deviceFlowId, - providerId: entry.providerId, - clientId: entry.initiatorClientId, - status: 'failed', - errorKind: 'upstream_error', - // PR #4291 follow-up review (qwen-latest, N1): the late- - // poll resolve branch fires when `provider.poll()` returns - // a result AFTER our race timer settled the wrapper. For a - // cooperative provider whose abort path resolves to - // `{kind: 'error', errorKind: 'upstream_error'}` (the Qwen - // implementation does this in response to AbortError), the - // "response" is just the abort-cooperation path — the IdP - // could be completely down. Don't assert "responsive but - // slow" on the error kind: route operators correctly by - // distinguishing "real late response" (pending / slow_down / - // success) from "provider's abort cooperation" (error). - hint: - latePollResult.kind === 'error' - ? `lost_late_poll_after_timeout: provider.poll() resolved kind=error after ${DEVICE_FLOW_POLL_TIMEOUT_MS}ms ceiling — likely abort-driven cooperation; IdP responsiveness unknown` - : `lost_late_poll_after_timeout: provider.poll() resolved kind=${latePollResult.kind} after ${DEVICE_FLOW_POLL_TIMEOUT_MS}ms ceiling — IdP is responsive but slow; consider raising the operator-side IdP latency alert threshold`, - }); - }, - (lateErr: unknown) => { - // Late rejection from the same provider promise. Don't - // double-audit if the wrapper already saw this same error - // (we'd be double-counting the same I/O failure). - if (lateErr instanceof DeviceFlowPollTimeoutError) return; - const detail = - lateErr instanceof Error ? lateErr.message : String(lateErr); - this.deps.audit?.record({ - deviceFlowId: entry.deviceFlowId, - providerId: entry.providerId, - clientId: entry.initiatorClientId, - status: 'failed', - errorKind: 'upstream_error', - hint: `lost_late_poll_after_timeout: provider.poll() rejected after ${DEVICE_FLOW_POLL_TIMEOUT_MS}ms ceiling: ${detail.length > 256 ? `${detail.slice(0, 256)}…[+${detail.length - 256} bytes]` : detail}`, - }); - }, - ); + void tracked + .then( + (latePollResult) => { + audit?.record({ + deviceFlowId: auditDeviceFlowId, + providerId: auditProviderId, + clientId: auditClientId, + status: 'failed', + errorKind: 'upstream_error', + // PR #4291 follow-up review (qwen-latest, N1): the late- + // poll resolve branch fires when `provider.poll()` returns + // a result AFTER our race timer settled the wrapper. For a + // cooperative provider whose abort path resolves to + // `{kind: 'error', errorKind: 'upstream_error'}` (the Qwen + // implementation does this in response to AbortError), the + // "response" is just the abort-cooperation path — the IdP + // could be completely down. Don't assert "responsive but + // slow" on the error kind: route operators correctly by + // distinguishing "real late response" (pending / slow_down / + // success) from "provider's abort cooperation" (error). + // PR #4291 follow-up review (deepseek-v4-pro, round-5 #3): + // `latePollResult.kind` is provider-controlled. The typed + // shape is `'pending' | 'slow_down' | 'success' | 'error'`, + // but a non-conforming provider could return an arbitrary + // string containing newlines / control characters. Same + // log-injection vector as `oauthError`. Route through + // `sanitizeForStderr` before interpolation; the kind=error + // branch is a static literal so it doesn't need the call. + hint: + latePollResult.kind === 'error' + ? `lost_late_poll_after_timeout: provider.poll() resolved kind=error after ${DEVICE_FLOW_POLL_TIMEOUT_MS}ms ceiling — likely abort-driven cooperation; IdP responsiveness unknown` + : `lost_late_poll_after_timeout: provider.poll() resolved kind=${sanitizeForStderr(latePollResult.kind)} after ${DEVICE_FLOW_POLL_TIMEOUT_MS}ms ceiling — IdP is responsive but slow; consider raising the operator-side IdP latency alert threshold`, + }); + }, + (lateErr: unknown) => { + // Late rejection from the same provider promise. Don't + // double-audit if the wrapper already saw this same error + // (we'd be double-counting the same I/O failure). + // Self-filter the registry's own timeout sentinel. Round-6 + // review (qwen-latest, #4): also gate on the runtime brand + // so a provider-thrown `DeviceFlowPollTimeoutError` (without + // the brand) is NOT silently swallowed — it should still + // audit through the normal late-rejection path. + if ( + lateErr instanceof DeviceFlowPollTimeoutError && + lateErr._isRegistryTimeout === true + ) + return; + // PR #4291 follow-up review (qwen-latest, round-4 #7): use + // the `name + length` redaction pattern (the same one the + // provider catch uses) instead of interpolating the raw + // `lateErr.message`. The provider's catch was carefully + // shaped to suppress raw upstream bodies that may contain + // WAF-echoed `device_code` / PKCE; the registry layer must + // not undo that hardening just because the same failure + // settled late. The 256-byte truncation we used previously + // was insufficient — the first 256 bytes of a WAF error + // page can carry a full `device_code` value. + // + // PR #4291 follow-up review (deepseek-v4-pro, round-5 #2): + // `Error.name` is a freely assignable string property, same + // attacker-controlled vector closed at the provider layer + // for `err.name`. Route through `sanitizeForStderr` so a + // hostile `e.name = "X\n[serve] FAKE\x1b[31m"` can't forge + // log lines via this audit path either. + const safeDetail = + lateErr instanceof Error + ? `${sanitizeForStderr(lateErr.name)} (message ${lateErr.message.length} bytes; raw suppressed)` + : ``; + audit?.record({ + deviceFlowId: auditDeviceFlowId, + providerId: auditProviderId, + clientId: auditClientId, + status: 'failed', + errorKind: 'upstream_error', + hint: `lost_late_poll_after_timeout: provider.poll() rejected after ${DEVICE_FLOW_POLL_TIMEOUT_MS}ms ceiling: ${safeDetail}`, + }); + }, + // PR #4291 follow-up review (qwen-latest, round-6 #2): chain a + // terminal `.catch(() => {})` so a synchronous throw inside + // either of the handlers above (e.g., a custom `audit.record` + // that throws on a malformed sink) doesn't surface as an + // unhandled rejection. Node 22's default + // `--unhandled-rejections=throw` would otherwise crash the + // daemon. Mirrors the persist-tracker pattern earlier in this + // file. We deliberately swallow the rejection — the original + // failure already settled the wrapper, and the audit path is + // a best-effort breadcrumb. + ) + .catch(() => {}); } // PR #4255 round-12 #1 (gpt-5.5 review CzSpN): also re-check // `this.disposed` after the await. `dispose()` clears @@ -1512,11 +1694,35 @@ export class DeviceFlowRegistry { // err.message in the audit hint so operators can debug // the contract violation. The SSE-broadcast hint stays // truncated to DEVICE_FLOW_POLL_HINT_MAX_LEN. + // + // PR #4291 follow-up review (qwen-latest, round-4 #5): when + // `rawProviderError` is undefined the timeout branch + // settled the wrapper (a registry-side timeout, NOT a + // provider throw). The earlier shape omitted `hint` + // entirely, leaving operators reading the durable audit + // trail with no signal whether the upstream_error was a + // hung IdP or a generic provider failure. Use the + // structured `result.hint` (which already contains the + // timeout-specific `provider.poll() timed out after Nms; + // check IdP connectivity` text built in the catch block) + // so the audit trail matches the SSE event. ...(rawProviderError !== undefined ? { - hint: `provider.poll() threw (raw): ${rawProviderError}`, + // PR #4291 follow-up review (qwen-latest, round-6 #3): + // run the captured `rawProviderError` through + // `sanitizeForStderr` before interpolating into the + // audit hint. `JSON.stringify` (which audit sinks + // typically use) escapes ASCII controls but per + // ES2019+ does NOT escape U+2028/U+2029 — those would + // still forge log lines downstream. Apply the same + // sanitization the late-observer + provider catches + // already use, so every provider-controlled audit + // path has consistent hardening. + hint: `provider.poll() threw (raw): ${sanitizeForStderr(rawProviderError)}`, } - : {}), + : result.hint !== undefined + ? { hint: result.hint } + : {}), }); } return; diff --git a/packages/cli/src/serve/auth/qwenDeviceFlowProvider.test.ts b/packages/cli/src/serve/auth/qwenDeviceFlowProvider.test.ts index 499b906ce62..6d963ce5abe 100644 --- a/packages/cli/src/serve/auth/qwenDeviceFlowProvider.test.ts +++ b/packages/cli/src/serve/auth/qwenDeviceFlowProvider.test.ts @@ -342,4 +342,124 @@ describe('QwenOAuthDeviceFlowProvider.poll() — stderr audit branches', () => { expect(line).toContain('FORGED LOG ENTRY'); expect(line).toContain('RED'); }); + + it('sanitizes control characters in attacker-controlled err.name on the non-OAuth path (round-4 #4)', async () => { + // PR #4291 follow-up review (qwen-latest, round-4 #4): + // `Error.name` is a freely assignable string property. A hostile + // provider or fetch wrapper could set `e.name` to inject newlines + // or ANSI sequences into stderr through the same vector we + // already closed for `oauthError`. Pin the equivalent + // sanitization on the non-OAuth path. + const err = new Error('upstream HTTP 500'); + err.name = 'Hostile\n[serve] FORGED LINE 2026-01-01\x1b[31mRED\x1b[0m'; + const provider = new QwenOAuthDeviceFlowProvider( + fakeClient({ + pollDeviceToken: async () => { + throw err; + }, + }), + ); + const result = await provider.poll(makeState(), { + signal: new AbortController().signal, + }); + expect(result.kind).toBe('error'); + expect(stderrLines).toHaveLength(1); + const line = stderrLines[0]; + // Single content line — no forged second log entry. + expect(line.split('\n').length).toBe(2); + // Hostile bytes from name are gone. + expect(line).not.toContain('\x1b[31m'); + expect(line).not.toContain('\x1b[0m'); + expect(line).not.toMatch(/\n\[serve\] FORGED/); + // Substantive parts of the name are still preserved (length- + // preserving sanitizer replaces controls with `?`). + expect(line).toContain('Hostile'); + expect(line).toContain('FORGED LINE'); + expect(line).toContain('RED'); + // Length field is the message length, not name length. + expect(line).toContain(`message ${err.message.length} bytes`); + }); + + it('sanitizes Unicode lookalike controls (U+2028 LINE SEPARATOR, bidi, ZWNBSP) in oauthError (round-5 #4)', async () => { + // PR #4291 follow-up review (deepseek-v4-pro, round-5 #4): the + // round-3 sanitizer only stripped ASCII C0/C1 + DEL; a hostile + // IdP could bypass with U+2028 (LINE SEPARATOR — rendered as a + // newline in many Unicode-aware terminals) or zero-width / bidi + // controls. Pin the extended coverage with a payload that mixes + // U+2028 (LINE SEPARATOR), U+200E (LRM), and U+FEFF (BOM). + // + // PR #4291 follow-up review (gpt-5.5, round-6 #1): the original + // shape embedded the invisible Unicode controls as literal + // characters in the source ('\u2028' between `slow_down` and + // `[serve]`, `\u200e` before `RTL`, `\ufeff` at the end). That + // makes the test source unreviewable in GitHub diffs / many + // editors and the negative assertions look like checks for empty + // / whitespace strings. Switched to explicit `\uXXXX` escapes in + // both the payload and `not.toContain(...)` assertions. + const U_2028_LINE_SEP = '\u2028'; + const U_200E_LRM = '\u200e'; + const U_FEFF_BOM = '\ufeff'; + const malicious = `slow_down${U_2028_LINE_SEP}[serve] FAKE LOG ${U_200E_LRM}RTL${U_FEFF_BOM}`; + const provider = new QwenOAuthDeviceFlowProvider( + fakeClient({ + pollDeviceToken: async () => { + throw new QwenOAuthPollError({ + oauthError: malicious, + description: 'attacker-supplied unicode', + status: 400, + }); + }, + }), + ); + const result = await provider.poll(makeState(), { + signal: new AbortController().signal, + }); + expect(result.kind).toBe('error'); + expect(stderrLines).toHaveLength(1); + const line = stderrLines[0]; + // None of the Unicode lookalikes survive into stderr. + expect(line).not.toContain(U_2028_LINE_SEP); + expect(line).not.toContain(U_200E_LRM); + expect(line).not.toContain(U_FEFF_BOM); + // The forged log line text MUST NOT lead an actual newline. + expect(line.split('\n').length).toBe(2); // single content + trailing + // Substantive parts preserved (`?`-replaced, length-preserving). + expect(line).toContain('FAKE LOG'); + expect(line).toContain('RTL'); + }); + + it('sanitizes Unicode bidi ISOLATE controls U+2066–U+2069 (CVE-2021-42574 Trojan Source) (round-6 #5)', async () => { + // Round-6 review (qwen-latest, #5): the round-5 regex covered + // U+202A–U+202E (embedding/override) but missed U+2066–U+2069 + // (LRI/RLI/FSI/PDI). These bidi ISOLATE controls are the primary + // CVE-2021-42574 attack vectors — a hostile IdP swapping + // \u2066 (LRI) for \u202d (LRO) achieves the same visual reordering + // and would have bypassed the round-5 filter entirely. + const U_2066_LRI = '\u2066'; + const U_2068_FSI = '\u2068'; + const U_2069_PDI = '\u2069'; + const provider = new QwenOAuthDeviceFlowProvider( + fakeClient({ + pollDeviceToken: async () => { + throw new QwenOAuthPollError({ + oauthError: `access_denied${U_2066_LRI}HIDDEN${U_2069_PDI}${U_2068_FSI}`, + description: 'trojan source', + status: 400, + }); + }, + }), + ); + const result = await provider.poll(makeState(), { + signal: new AbortController().signal, + }); + expect(result.kind).toBe('error'); + expect(stderrLines).toHaveLength(1); + const line = stderrLines[0]; + expect(line).not.toContain(U_2066_LRI); + expect(line).not.toContain(U_2068_FSI); + expect(line).not.toContain(U_2069_PDI); + // Substantive parts still visible. + expect(line).toContain('access_denied'); + expect(line).toContain('HIDDEN'); + }); }); diff --git a/packages/cli/src/serve/auth/qwenDeviceFlowProvider.ts b/packages/cli/src/serve/auth/qwenDeviceFlowProvider.ts index 4b35417dd85..c7ee3fcee1b 100644 --- a/packages/cli/src/serve/auth/qwenDeviceFlowProvider.ts +++ b/packages/cli/src/serve/auth/qwenDeviceFlowProvider.ts @@ -19,6 +19,7 @@ import { import { writeStderrLine } from '../../utils/stdioHelpers.js'; import { brandSecret, + sanitizeForStderr, unsafeRevealSecret, UpstreamDeviceFlowError, type BrandedSecret, @@ -52,25 +53,6 @@ function truncateForStderr(detail: string): string { return `${detail.slice(0, STDERR_DETAIL_MAX)}…[+${dropped} bytes truncated]`; } -/** - * Strip / replace bytes that could forge log lines or inject terminal - * control sequences when interpolated into a stderr breadcrumb. PR #4291 - * follow-up review (gpt-5.5, #2): `QwenOAuthPollError.oauthError` comes - * directly from the upstream JSON `error` field — attacker-controlled - * if the IdP, a reverse proxy, or a WAF is hostile / compromised. A - * value like `slow_down\n[serve] FAKE LOG LINE 2026-...` would otherwise - * forge an extra log line; a value containing `\x1b[…m` could inject - * ANSI color or cursor-movement sequences into operator terminals. - * - * Strips C0 controls (0x00–0x1f), DEL (0x7f), and C1 controls (0x80–0x9f). - * Replaces each with `?` so the operator can still see SOMETHING was - * present at that index (length-preserving) instead of silently dropping. - */ -function sanitizeForStderr(value: string): string { - // eslint-disable-next-line no-control-regex - return value.replace(/[\x00-\x1f\x7f-\x9f]/g, '?'); -} - /** * Qwen-OAuth implementation of `DeviceFlowProvider` for `qwen serve`. * @@ -270,7 +252,14 @@ export class QwenOAuthDeviceFlowProvider implements DeviceFlowProvider { // unexpected AbortError). The constructor name + length is // enough for triage; the raw message MAY contain WAF-echoed // request body fields. - safeDetail = `${err.name} (message ${err.message.length} bytes; raw suppressed to avoid echoing device_code/PKCE)`; + // PR #4291 follow-up review (qwen-latest, round-4 #4): + // `Error.name` is a freely assignable string property — + // a hostile provider or fetch wrapper could set it to + // `'X\n[serve] FAKE LINE\x1b[31m'` to forge log lines + // or inject ANSI sequences. The same `sanitizeForStderr` + // we apply to `oauthError` must apply here too. Length + // is a number and safe to interpolate raw. + safeDetail = `${sanitizeForStderr(err.name)} (message ${err.message.length} bytes; raw suppressed to avoid echoing device_code/PKCE)`; } else { safeDetail = ``; } diff --git a/packages/cli/src/serve/server.ts b/packages/cli/src/serve/server.ts index 773a32baba5..282b76cd973 100644 --- a/packages/cli/src/serve/server.ts +++ b/packages/cli/src/serve/server.ts @@ -29,6 +29,7 @@ import { } from './auth/deviceFlow.js'; import { QwenOAuthDeviceFlowProvider } from './auth/qwenDeviceFlowProvider.js'; import { createDaemonStatusProvider } from './daemonStatusProvider.js'; +import { isServeDebugMode } from './debugMode.js'; import { isLoopbackBind } from './loopbackBinds.js'; import { canonicalizeWorkspace, @@ -774,18 +775,13 @@ export function createServeApp( // would otherwise flood production logs. Operators who hit // the symptom can flip QWEN_SERVE_DEBUG=1 and get the // breadcrumb on the next reproduction. - const callerIsInitiator = - (view.initiatorClientId === undefined && clientId === undefined) || - (view.initiatorClientId !== undefined && - clientId !== undefined && - clientId === view.initiatorClientId); - if ( - !callerIsInitiator && - process.env['QWEN_SERVE_DEBUG'] && - !['0', 'false', 'off', 'no'].includes( - (process.env['QWEN_SERVE_DEBUG'] ?? '').trim().toLowerCase(), - ) - ) { + // + // PR #4291 follow-up review (qwen-latest, round-4 #6): the + // QWEN_SERVE_DEBUG check is centralized in `isServeDebugMode()` + // (./debugMode.js) — already used by workspaceAgents.ts and + // workspaceMemory.ts. Inlining a verbatim copy here was a DRY + // violation that could drift from the canonical falsy list. + if (!callerIsDeviceFlowInitiator(view, clientId) && isServeDebugMode()) { writeStderrLine( `qwen serve debug: GET /workspace/auth/device-flow/${id} redacted verification fields — caller-clientId mismatch (initiator=${view.initiatorClientId ?? 'anonymous'}, caller=${clientId ?? 'anonymous'})`, ); @@ -1977,6 +1973,41 @@ function parseOptionalWorkspaceCwd( return cwd; } +/** + * Returns true iff the GET / POST caller is the same client that + * originally started the device flow. Both-undefined is treated as a + * match (anonymous-start → anonymous-reattach is the legitimate case). + * + * **Threat model (consolidated from PR #4291 follow-up reviews):** this + * is BEST-EFFORT ATTRIBUTION, not authentication. `X-Qwen-Client-Id` + * is a syntactic header, not bound to a server-validated identity — + * anyone holding the bearer token can spoof it. The bearer token IS + * the auth boundary; this gate exists to prevent ACCIDENTAL cross- + * client reads in well-behaved multi-SDK setups, and to keep the POST + * take-over and GET state shapes consistent. A determined attacker + * who has compromised the daemon bearer token already wins; locking + * down further would require binding identity into bearer-token + * issuance, which is a separate architectural change. + * + * PR #4291 follow-up review (qwen-latest, round-4 #2): extracted from + * three duplicated copies in the route handler, `toDeviceFlowStart- + * ResponseBody`, and `toDeviceFlowStateBody`. The exact bug class + * #4291 was fixing was "POST and GET diverged on the same redaction + * policy" — duplicating the gate recreated the preconditions for + * silent divergence. Single source of truth. + */ +function callerIsDeviceFlowInitiator( + view: Pick, + callerClientId: string | undefined, +): boolean { + return ( + (view.initiatorClientId === undefined && callerClientId === undefined) || + (view.initiatorClientId !== undefined && + callerClientId !== undefined && + callerClientId === view.initiatorClientId) + ); +} + /** * PR 21 — translate the registry's redacted `DeviceFlowPublicView` into * the wire shape declared by `DaemonDeviceFlowStartResult`. Splitting @@ -2003,18 +2034,14 @@ function toDeviceFlowStartResponseBody( // every POST, including the `attached: true` take-over case, so any // bearer-token holder that POSTed `providerId: ` got the // verification code another client started. That bypassed the - // closed-out GET redaction completely. Apply the same gate here. - // Fresh starts naturally pass the gate because `view.initiatorClientId` - // was set from the same `callerClientId` on this very request. - // Take-over callers that don't match the initiator now see the - // public envelope only. The both-undefined branch preserves the - // anonymous-start → anonymous-reattach use case. - const callerIsInitiator = - (view.initiatorClientId === undefined && callerClientId === undefined) || - (view.initiatorClientId !== undefined && - callerClientId !== undefined && - callerClientId === view.initiatorClientId); - if (callerIsInitiator) { + // closed-out GET redaction completely. Apply the shared gate + // (`callerIsDeviceFlowInitiator`) here. Fresh starts naturally + // pass the gate because `view.initiatorClientId` was set from the + // same `callerClientId` on this very request. Take-over callers + // that don't match the initiator see the public envelope only; + // anonymous-start → anonymous-reattach also passes via the + // both-undefined branch. + if (callerIsDeviceFlowInitiator(view, callerClientId)) { body['userCode'] = view.userCode ?? ''; body['verificationUri'] = view.verificationUri ?? ''; if (view.verificationUriComplete) { @@ -2056,44 +2083,16 @@ function toDeviceFlowStateBody( if (view.expiresAt !== undefined) body['expiresAt'] = view.expiresAt; if (view.intervalMs !== undefined) body['intervalMs'] = view.intervalMs; if (view.lastPolledAt !== undefined) body['lastPolledAt'] = view.lastPolledAt; - // PR #4255 follow-up review thread (deepseek-v4-pro): symmetrize with - // the POST take-over response shape — only echo `userCode` / + // PR #4255 follow-up review thread (deepseek-v4-pro): symmetrize + // with the POST take-over response shape — only echo `userCode` / // `verificationUri` / `verificationUriComplete` / `initiatorClientId` - // back to the original starter (matched by `X-Qwen-Client-Id`). An - // anonymous GET caller, or a caller identifying as a different client, - // sees only the public envelope (`status` / `errorKind` / `hint` / - // timestamps). Bearer-token gated already (the route uses - // `mutate({ strict: true })`), so the blast radius was small, but - // multi-client setups sharing a single daemon token could otherwise - // enumerate other clients' verification codes. - // - // **Threat model (PR #4291 follow-up review by Copilot):** this gate - // is BEST-EFFORT ATTRIBUTION, not authentication. `X-Qwen-Client-Id` - // is a syntactic header, not bound to a server-validated identity — - // anyone holding the bearer token can spoof it. The bearer token IS - // the auth boundary; this gate exists to prevent ACCIDENTAL - // cross-client reads in well-behaved multi-SDK setups (and to keep - // GET symmetric with the POST take-over shape closed out in - // round-12 #6 of #4255). A determined attacker who has compromised - // the daemon bearer token already wins; locking down GET further - // would require binding identity into bearer-token issuance, which - // is a separate architectural change. - // PR #4291 follow-up review (qwen-latest, #3): the gate must accept - // the both-undefined case too, otherwise an anonymously-started flow - // (POST without `X-Qwen-Client-Id` → `initiatorClientId === undefined`) - // becomes silently unreadable: even the same anonymous caller GETting - // the same id can no longer retrieve `userCode`/`verificationUri` — - // the body switches from "what they got from POST" to a redacted - // public envelope, with HTTP 200, no error. Pre-PR-4291 GET returned - // these fields to anyone with the bearer; this gate's purpose is to - // prevent CROSS-client reads, not to lock anonymous flows out of - // their own data. - const callerIsInitiator = - (view.initiatorClientId === undefined && callerClientId === undefined) || - (view.initiatorClientId !== undefined && - callerClientId !== undefined && - callerClientId === view.initiatorClientId); - if (callerIsInitiator) { + // back to the original starter (matched by `X-Qwen-Client-Id`). + // Bearer-token gated already (the route uses `mutate({ strict: true })`), + // but multi-client setups sharing a single daemon token could + // otherwise enumerate other clients' verification codes. See + // `callerIsDeviceFlowInitiator` JSDoc above for the consolidated + // threat-model note (best-effort attribution, NOT auth boundary). + if (callerIsDeviceFlowInitiator(view, callerClientId)) { if (view.userCode) body['userCode'] = view.userCode; if (view.verificationUri) body['verificationUri'] = view.verificationUri; if (view.verificationUriComplete) {