feat(cli): add credential redaction for worker stderr forwarding - #6146
Merged
Conversation
Add a `redactLogCredentials` function that strips credentials from worker log lines before they reach the daemon's stderr and log file. Covers Bearer/QQBot tokens, Authorization headers, common API key prefixes, env-var secret assignments, URL-embedded credentials, JSON secret fields, and platform-specific headers (DingTalk). Integrate the redaction into both stderr forwarding paths: - ACP children: `createStderrForwarder` now applies redaction in both the normal flush and 64 KiB forced-truncation code paths. - Daemon channel worker: change supervisor stdio from `'inherit'` to `'pipe'` for stderr, add a line-buffered forwarder with redaction, 64 KiB buffer cap, and try-catch to prevent daemon crashes. Wire `onDiagnosticLine` so worker stderr also reaches the daemon log file (previously it only went to the daemon's terminal). Issue: #5976 (V1.5 follow-up)
doudouOUC
force-pushed
the
worktree-jolly-noodling-twilight
branch
from
July 1, 2026 23:43
f6ae3f8 to
87eab8f
Compare
Contributor
|
Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. 中文请勿对活跃的 PR 执行 rebase 或 force-push，因为这会使已有的评审评论失效。另外，供日后参考：作为集成流程的一部分，机器人始终会自动将所有改动压缩（squash）为单个提交。 |
Contributor
There was a problem hiding this comment.
Pull request overview
Adds credential redaction for worker stderr forwarding so sensitive tokens/keys don’t leak into the daemon’s stderr stream or persisted daemon logs, covering both ACP child processes and the daemon-managed channel worker path.
Changes:
- Introduces
redactLogCredentials()with a set of credential/token redaction patterns plus unit tests. - Applies redaction to ACP stderr forwarding (including the forced-truncation path) and to the daemon channel worker’s new piped-stderr forwarding.
- Wires daemon worker diagnostic lines into the daemon logger via an
onDiagnosticLinecallback, and updates test/alias plumbing accordingly.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| packages/cli/vitest.config.ts | Adds a Vitest alias so CLI tests can import the acp-bridge redaction implementation from source. |
| packages/cli/src/serve/run-qwen-serve.ts | Passes a diagnostic sink into the channel worker supervisor so forwarded stderr also reaches daemon logs. |
| packages/cli/src/serve/channel-worker-supervisor.ts | Switches worker stderr to pipe and forwards stderr lines through credential redaction into daemon stderr/logging. |
| packages/cli/src/serve/channel-worker-supervisor.test.ts | Adds coverage asserting worker stderr forwarding is redacted and prefixed. |
| packages/acp-bridge/src/spawnChannel.ts | Applies credential redaction to ACP child stderr forwarding (normal + forced-truncation paths). |
| packages/acp-bridge/src/spawnChannel.test.ts | Adds tests verifying redaction for forwarded and forced-truncated stderr output. |
| packages/acp-bridge/src/logRedaction.ts | Implements line-based credential redaction via sequential regex replacements. |
| packages/acp-bridge/src/logRedaction.test.ts | Adds unit tests covering the supported credential patterns and edge cases. |
| packages/acp-bridge/package.json | Exposes ./logRedaction as a package subpath export for consumers like the CLI. |
Comments suppressed due to low confidence (1)
packages/cli/src/serve/channel-worker-supervisor.ts:270
process.stderr.write()can returnfalsewhen stderr is a slow pipe (e.g., systemd/docker). The current forwarder ignores this, so sustained worker stderr can accumulate in Node’s internal write buffer and increase daemon memory usage.
Consider pausing child.stderr when write() backpressures and resuming on 'drain' to keep forwarding bounded under slow stderr consumers.
daemonUrl: string;
daemonToken?: string;
workspace: string;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Collaborator
|
Thanks for the PR! Template looks good ✓ — all required sections present, bilingual, clear test plan. On direction: this is a straightforward security gap fix — worker stderr was forwarding raw credentials into daemon logs. It follows directly from the post-merge review of PR #6098 / issue #5976. Clearly within scope; credential redaction in log paths is core daemon hygiene. On approach: the scope feels right. Three logical changes bundled together:
The heartbeat timestamp hardening ( One observation: Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ — 所有章节齐全，双语，测试计划清晰。 方向：这是一个直接的安全缺口修复——worker stderr 将原始凭据转发到 daemon 日志中。源自 PR #6098 合并后评审 / Issue #5976。属于核心 daemon 安全范畴。 方案：范围合理。三个逻辑变更合在一起：
心跳时间戳加固（用 一个观察： 进入代码审查 🔍 — Qwen Code · qwen3.7-max |
Collaborator
Code ReviewIndependent proposal: Before reading the diff, I'd create a Comparison with the PR: The PR's approach matches my proposal closely — same module structure, same regex patterns, same integration points. The patterns are well-ordered (specific before broad), have backtracking protection on complex ones, and the test coverage is thorough. Findings: The code is clean. Two things worth noting:
No correctness bugs, security holes, or AGENTS.md violations found. Real-Scenario TestingVerified the redaction function end-to-end via All 5 test inputs produce correct output — Bearer token, key=value secret, URL credentials, AWS key ID redacted; normal log line passed through unchanged. Unit tests (worktree, PR branch):
CI (GitHub Actions): Ubuntu tests pass, coverage pass. All green. TypeScript: This is a non-user-visible infrastructure change (credential redaction in daemon logs), so no before/after TUI comparison applies. 中文说明代码审查独立方案： 在查看 diff 之前，我会创建一个 与 PR 对比： PR 方案与我的独立方案高度一致——相同的模块结构、正则模式、集成点。模式排序合理（具体在前，宽泛在后），复杂模式有回溯保护，测试覆盖充分。 发现：
未发现正确性 bug、安全漏洞或 AGENTS.md 违规。 真实场景测试通过 这是非用户可见的基础设施变更，无需 TUI 前后对比。 — Qwen Code · qwen3.7-max |
Collaborator
|
This PR does exactly what it says — plugs a credential leak in worker stderr forwarding paths that was identified during post-merge review of PR #6098. The implementation is clean and well-scoped. Looking at the full picture:
My one reservation: the duplicate Approving. ✅ 中文说明这个 PR 做了它该做的事——修补了 worker stderr 转发路径中的凭据泄露漏洞，该问题在 PR #6098 合并后评审中发现。实现干净且范围合理。 全局来看：
唯一保留意见： 批准合并 ✅ — Qwen Code · qwen3.7-max |
qwen-code-ci-bot
previously approved these changes
Jul 1, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
Contributor
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
DragonnZhang
reviewed
Jul 2, 2026
DragonnZhang
left a comment
DragonnZhang
left a comment
Collaborator
There was a problem hiding this comment.
Automated Review (credential-redaction focus)
Reviewed all 7 changed files across the credential redaction, worker supervisor, TLS, and webui session changes. No high-confidence correctness or security defects found.
Observations (informational, no action required):
-
Double URL redaction in worker log streams:
createWorkerLogRedactorinchannel-worker-supervisor.tsapplies a worker-level URL regex (replacing with<redacted>) then callsredactLogCredentialswhich has its own URL pattern (replacing with***REDACTED***). The second pattern re-matches the<redacted>@artifact from the first pass, producing***REDACTED***@output. This is over-redaction (security-safe) but the inconsistent markers could complicate programmatic log parsing. -
Scope: This PR bundles TLS support, worker heartbeat/restart policy, webui session management overhaul, reasoning-effort settings, and i18n updates under a credential-redaction title. Each area looks individually sound, but the combined blast radius of a revert is large.
-
Regex safety: All credential patterns use bounded quantifiers and non-overlapping character classes. The compound env-var pattern explicitly caps segment lengths to prevent backtracking. No ReDoS vectors identified.
- Change redaction marker from ***REDACTED*** to <redacted> to match
the supervisor's existing convention and avoid double-redaction output
mismatches in tests.
- Bound URL credential regex scheme to {0,31} chars (matching the
supervisor's pattern) to prevent O(n²) backtracking on long strings
of scheme-like characters.
…v secrets Two fixes for unresolved review threads from #6098: - Heartbeat: use daemon's own `new Date().toISOString()` instead of reflecting the worker-supplied `message.at` value. Prevents a compromised adapter from injecting arbitrary data into `/daemon/status`. - PEM multiline: split multi-line sensitive env values (e.g. PEM keys) into per-line redaction patterns so individual logged lines match. The full value is kept as a pattern too for single-line matches.
Collaborator
|
@qwen-code /review |
qwen-code-ci-bot
requested changes
Jul 2, 2026
- Fix sensitiveEnvValues: change `lines.length > 1` to `> 0` so single matching lines from multi-line env values are also added as patterns. - Add hyphens to sk- charset for compound prefixes (sk-proj-, sk-ant-). - Add github_pat_ (fine-grained PATs) and ghu_ (app user tokens). - Add ASIA prefix for AWS STS temporary credentials alongside AKIA. - Update Authorization catch-all comment to accurately describe the 2-token limitation.
Collaborator
Author
Review feedback summary (commit 519f5bf)
|
wenshao
reviewed
Jul 2, 2026
Add comment explaining why heartbeat uses daemon clock instead of worker-supplied message.at (security: compromised adapter injection).
Add test verifying credential redaction on partial lines flushed via forwarder.onEnd(). Suppress pre-existing vitest/no-conditional-expect lint errors in getAcpMemoryArgs tests (system-dependent heapArg).
Move the onEnd credential redaction test to logRedaction.test.ts to avoid triggering pre-existing vitest/no-conditional-expect lint errors in spawnChannel.test.ts. Revert the spawnChannel test file to its upstream state.
Collaborator
CI build-failure investigationRoot cause: ESLint, not a compile/test failure. The only red step is Why the directives are "unused"This PR added two ESLint flat config defaults FixRemove the four now-redundant directive lines (409, 415, 421, 426). Since the rule is off, the conditional-expect pattern is allowed as-is: it('always includes --expose-gc and optionally --max-old-space-size', () => {
const args = getAcpMemoryArgs();
expect(args).toContain('--expose-gc');
const heapArg = args.find((a) => a.startsWith('--max-old-space-size='));
if (heapArg) {
const sizeMB = Number(heapArg.split('=')[1]);
expect(sizeMB).toBeGreaterThan(0);
expect(sizeMB).toBeLessThanOrEqual(16_384);
}
});
it('respects the 16GB cap', () => {
const args = getAcpMemoryArgs();
const heapArg = args.find((a) => a.startsWith('--max-old-space-size='));
if (heapArg) {
const sizeMB = Number(heapArg.split('=')[1]);
expect(sizeMB).toBeLessThanOrEqual(16_384);
}
});Notes on the other check states (not additional problems)
中文说明CI 构建失败调查根因是 ESLint，不是编译或测试失败。 唯一变红的步骤是 为什么这两个指令是"未使用"本 PR 在 ESLint flat config 的 修复方式删除这 4 行现在多余的指令（第 409、415、421、426 行）。既然规则本就关闭，条件 expect 写法可原样保留： it('always includes --expose-gc and optionally --max-old-space-size', () => {
const args = getAcpMemoryArgs();
expect(args).toContain('--expose-gc');
const heapArg = args.find((a) => a.startsWith('--max-old-space-size='));
if (heapArg) {
const sizeMB = Number(heapArg.split('=')[1]);
expect(sizeMB).toBeGreaterThan(0);
expect(sizeMB).toBeLessThanOrEqual(16_384);
}
});
it('respects the 16GB cap', () => {
const args = getAcpMemoryArgs();
const heapArg = args.find((a) => a.startsWith('--max-old-space-size='));
if (heapArg) {
const sizeMB = Number(heapArg.split('=')[1]);
expect(sizeMB).toBeLessThanOrEqual(16_384);
}
});关于其它检查状态的说明（并非额外问题）
|
vitest/no-conditional-expect is not enabled in this repo's eslint config. The directives cause CI failure via reportUnusedDisableDirectives warn + --max-warnings 0.
DragonnZhang
reviewed
Jul 2, 2026
DragonnZhang
left a comment
DragonnZhang
left a comment
Collaborator
There was a problem hiding this comment.
This PR adds a redactLogCredentials function to logRedaction.ts with comprehensive regex patterns covering Bearer/QQBot tokens, Authorization headers, API key prefixes (sk-, ghp_, glpat-, xoxb-, etc.), AWS access key IDs, key=value secret assignments, JSON-quoted secret fields, and URL-embedded credentials. The integration into spawnChannel.ts and channel-worker-supervisor.ts correctly applies redaction at both the flush and truncation paths. The heartbeat timestamp change to use daemon clock instead of worker-supplied message.at is a sound security hardening. Test coverage is thorough with 30+ test cases covering happy paths, edge cases, and integration with the stderr forwarder. Looks correct.
— qwen3-coder via Qwen Code /review
Collaborator
|
@qwen-code /triage |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
— qwen3.7-max via Qwen Code /review
| pattern: | ||
| /("(?:api_key|api-key|apikey|token|secret|password|pwd|access_token|client_secret|app_secret|authorization)"\s*:\s*")[^"]{10,}(")/gi, | ||
| replacement: `$1${REDACTED}$2`, | ||
| }, |
Collaborator
There was a problem hiding this comment.
[Suggestion] The URL-embedded credentials regex (?:[^/\s]+@)+ allows @ inside the inner character class [^/\s], creating ambiguity about which @ is the delimiter vs. content. While bounded in practice by the mandatory @ per iteration, excluding @ from the inner class eliminates all backtracking ambiguity:
Suggested change
| }, | |
| pattern: /\b([a-z][a-z0-9+.-]{0,31}:\/\/)(?:[^/@\s]+@)+/gi, |
— qwen3.7-max via Qwen Code /review
| // Key=value assignments for simple secret names (token=, secret=, etc.) | ||
| { | ||
| pattern: | ||
| /((?:api[_-]?key|token|secret|password|pwd)[_-]?[=:]\s*)\S{10,}/gi, |
Collaborator
There was a problem hiding this comment.
[Suggestion] Two gaps in credential keyword coverage:
-
The key=value pattern alternation
api[_-]?key|token|secret|password|pwdis missingpasswd. A log line likeDB_PASSWD=wJalrXUtnFEMI/K7MDENGis not caught. The compound env-var pattern also requires_PASSWORDsuffix, not_PASSWD. -
The AWS access key pattern
(?:AKIA|ASIA)[A-Z0-9]{16}has no trailing boundary assertion. Adjacent alphanumeric characters after the 20-char key are left unredacted.
Suggested fixes:
Suggested change
| /((?:api[_-]?key|token|secret|password|pwd)[_-]?[=:]\s*)\S{10,}/gi, | |
| pattern: | |
| /((?:api[_-]?key|token|secret|password|passwd|pwd)[_-]?[=:]\s*)\S{10,}/gi, |
Suggested change
| /((?:api[_-]?key|token|secret|password|pwd)[_-]?[=:]\s*)\S{10,}/gi, | |
| pattern: /(?:AKIA|ASIA)[A-Z0-9]{16}\b/g, |
And add PASSWD to the compound env-var suffix:
_(?:KEY|TOKEN|SECRET|PASSWORD|PASSWD)\s*[=:]\s*
— qwen3.7-max via Qwen Code /review
| // Includes github_pat_ (fine-grained PATs) and ghu_ (app user tokens). | ||
| // Slack tokens use hyphens as separators: xoxb-NNN-NNN-alphanum. | ||
| { | ||
| pattern: /(?:ghp_|gho_|ghs_|ghu_|github_pat_|glpat-|xoxb-|xoxp-)[a-zA-Z0-9_-]{20,}/g, |
Collaborator
There was a problem hiding this comment.
[Suggestion] The GitHub/GitLab/Slack token pattern covers xoxb- and xoxp- but omits xapp- (Slack app-level tokens used for Socket Mode). The project's own secret-scanner.ts in packages/core already recognizes xapp- as a credential type (slack-app-token). Consider adding xapp- to the prefix list:
Suggested change
| pattern: /(?:ghp_|gho_|ghs_|ghu_|github_pat_|glpat-|xoxb-|xoxp-)[a-zA-Z0-9_-]{20,}/g, | |
| pattern: /(?:ghp_|gho_|ghs_|ghu_|github_pat_|glpat-|xoxb-|xoxp-|xapp-)[a-zA-Z0-9_-]{20,}/g, |
— qwen3.7-max via Qwen Code /review
| expect( | ||
| redactLogCredentials('Authorization: Bearer eyJhbGciOi.xyz.abc'), | ||
| ).toBe(`Authorization: ${R}`); | ||
| }); |
Collaborator
There was a problem hiding this comment.
[Suggestion] This test asserts toBe('Authorization: ${R}') — the same output the Authorization catch-all test expects. The Bearer pattern's unique contribution (preserving the Bearer keyword as Authorization: Bearer <redacted>) is never asserted. If CREDENTIAL_PATTERNS are accidentally reordered (catch-all before Bearer), both tests still pass silently.
Fix: assert the specific Bearer output format to guard the ordering dependency:
Suggested change
| }); | |
| expect( | |
| redactLogCredentials('Authorization: Bearer eyJhbGciOi.xyz.abc'), | |
| ).toBe(`Authorization: Bearer ${R}`); |
— qwen3.7-max via Qwen Code /review
| expect(() => redactLogCredentials(longLine)).not.toThrow(); | ||
| }); | ||
| }); | ||
|
|
Collaborator
There was a problem hiding this comment.
[Suggestion] This describe('createStderrForwarder redaction integration') block is a near-duplicate of spawnChannel.test.ts:239-253 ("redacts credentials flushed via onEnd (partial line)"). Both create a createStderrForwarder, feed 'Bearer secrettoken123', call onEnd(), and assert the same things. This test exercises createStderrForwarder from spawnChannel.ts, not redactLogCredentials, so it belongs in spawnChannel.test.ts where it already exists.
Consider removing this block to avoid maintaining the same test in two files.
— qwen3.7-max via Qwen Code /review
| * file. The patterns cover Bearer/QQBot tokens, Authorization headers, | ||
| * common API key prefixes, secret env assignments, URL-embedded | ||
| * credentials, and platform-specific headers (DingTalk). | ||
| * |
Collaborator
There was a problem hiding this comment.
[Suggestion] redactLogCredentials runs 11 sequential .replace() calls on every log line, even for lines with no credential-like content (e.g., [INFO] Server started on port 4170). For high-throughput worker output, this multiplies per-line regex cost by 11.
A fast-path guard would skip the full scan for the common case:
export function redactLogCredentials(line: string): string {
if (!/[Bb]earer|QQBot|[Aa]uthorization|x-acs-|sk-|gh[psou]_|github_pat_|glpat-|xox[bp]p-|AKIA|ASIA|:\/\//.test(line)
&& !/(?:token|secret|password|pwd|api[_-]?key)[_-]?[=:]/i.test(line)
&& !/_(?:KEY|TOKEN|SECRET|PASSWORD)\s*[=:]/.test(line)) {
return line;
}
let result = line;
for (const { pattern, replacement } of CREDENTIAL_PATTERNS) {
result = result.replace(pattern, replacement);
}
return result;
}— qwen3.7-max via Qwen Code /review
| return Object.entries(env) | ||
| .filter(([key, value]) => sensitiveKey.test(key) && value !== undefined) | ||
| .map(([, value]) => value!) | ||
| .flatMap(([, value]) => { |
Collaborator
There was a problem hiding this comment.
[Suggestion] The sensitiveEnvValues flatMap returns [value!, ...lines] — both the full multiline value AND its individual lines. For a 30-line PEM certificate, this creates 31 regex patterns. The full value regex matches first and replaces the entire secret, leaving the per-line patterns with nothing to match (wasted work).
If the goal is to catch individual lines that appear standalone in log output, drop the full value when lines are successfully extracted:
Suggested change
| .flatMap(([, value]) => { | |
| .flatMap(([, value]) => { | |
| const lines = value!.split('\n').filter((l) => l.length >= 4); | |
| return lines.length > 0 ? lines : [value!]; | |
| }) |
— qwen3.7-max via Qwen Code /review
qwen-code-ci-bot
approved these changes
Jul 2, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
wenshao
reviewed
Jul 2, 2026
| { | ||
| pattern: /(?:ghp_|gho_|ghs_|ghu_|github_pat_|glpat-|xoxb-|xoxp-)[a-zA-Z0-9_-]{20,}/g, | ||
| replacement: REDACTED, | ||
| }, |
Collaborator
There was a problem hiding this comment.
[Suggestion] The regex includes gho_, ghs_, ghu_, and xoxp- prefixes, but none of these have positive tests. Only ghp_, github_pat_, glpat-, and xoxb- are tested. A typo in the alternation (e.g., gho missing the underscore) would go undetected.
it('redacts gho_ OAuth tokens', () => {
expect(redactLogCredentials('gho_' + 'A'.repeat(36))).toBe(R);
});
it('redacts ghs_ server-to-server tokens', () => {
expect(redactLogCredentials('ghs_' + 'A'.repeat(36))).toBe(R);
});
it('redacts ghu_ app user tokens', () => {
expect(redactLogCredentials('ghu_' + 'A'.repeat(36))).toBe(R);
});
it('redacts xoxp- Slack user tokens', () => {
expect(redactLogCredentials('xoxp-' + '1'.repeat(20))).toBe(R);
});— qwen3.7-max via Qwen Code /review
wenshao
approved these changes
Jul 2, 2026
Collaborator
✅ Maintainer verification report — real daemon (tmux) E2E + adversarial probeVerified locally at head 1. Unit baselines — all green
2. Adversarial probe of the compiled
|
| 套件 | 结果 |
|---|---|
acp-bridge/src/logRedaction.test.ts |
38/38 通过 |
acp-bridge/src/spawnChannel.test.ts |
27/27 通过 |
cli/src/serve/channel-worker-supervisor.test.ts |
51/51 通过 |
小提示：PR 描述测试计划第 3 步写的是 supervisor 套件"14 通过 / 7 个既有失败"。但在当前 head 是完全绿的（51/51）——本 PR 加的
vitest.config.tsalias（加上后续提交）已修好。是描述过时，代码没问题。
2. 对编译后 redactLogCredentials 的对抗性探测
用真实输入驱动构建产物 dist/logRedaction.js（不是 src）：
- 正确性 21/21：所有声称覆盖的凭据形态都被脱敏（Bearer/JWT、裸 Bearer、QQBot、Authorization Basic、DingTalk、
sk-/sk-proj-、ghp_/github_pat_/glpat-/xoxb-、AWSAKIA/ASIA、token=/password=、复合键AWS_SECRET_ACCESS_KEY/QWEN_DAEMON_TOKEN、JSON"token"/"client_secret"、URL 内嵌凭据含多@）。 - 无过度脱敏 5/5：良性日志行原样保留（普通日志、无凭据 URL、中文、短
sk-）。 - 抗 ReDoS：所有病态输入（10万–50万字符：长复合环境变量临界串、
http://+ 5万个a@、10万个_、超长token=值、Authorization:+ 10万 token）均在 < 5 毫秒完成。有界量词（{0,50}、{1,50}{0,10}）生效——无灾难性回溯。
3. 完整守护进程 E2E（真实 qwen serve --channel，tmux）
真实守护进程 + 隔离 HOME + mock WebSocket 平台 + 一个 plugin-example 变体，其 connect() 会把运行期获取的凭据（不在 worker 的 env 里）写入 stderr。验证了 3 进程隔离（serve / --acp / daemon-worker），以及 worker 与 mock 平台保持活跃 WS 连接。
FIX（原样 head）—— ~/.qwen/debug/daemon/<id>.log 中每个 secret 都被清除：
channel worker stderr: E2E-STDERR-PROBE http-debug Authorization: <redacted>
channel worker stderr: E2E-STDERR-PROBE config apiKey=<redacted>
channel worker stderr: E2E-STDERR-PROBE proxy https://<redacted>@internal.example.com/webhook
channel worker stderr: E2E-STDERR-PROBE json payload {"client_secret":"<redacted>"}
channel worker stderr: E2E-STDERR-PROBE CONTROL-LINE channel starting normally, no secrets here ← 良性行未被动
8 个诱饵 token 均确认在日志文件中不存在。
4. 承重性 A/B 变异（关键证据）
只把编译后 supervisor 里的 return redactLogCredentials(redacted) 改成 return redacted，重启同一守护进程：
NOFIX → 泄露: Authorization: Bearer eyJhbGci…DECOYbearer… ← 运行期 token 原样泄露
NOFIX → 泄露: apiKey=sk-proj-DECOY…, ghp_DECOY…, AKIADECOY…, {"client_secret":"DECOY…"}
NOFIX → 仍脱敏: https://<redacted>@… ← 对照：既有 URL 正则，不是新增调用
去掉新增行后有 7 种运行期凭据形态泄露；URL 凭据仍被脱敏（它由新增调用之前运行的既有正则捕获）。恢复该行 → 全部重新脱敏（A→B→A 确定性）。这证明新增的 redactLogCredentials 调用正是补上了"worker 运行期获取、守护进程基于 env 的脱敏无法看到"的那类凭据的缺口。
5. 类型检查
三个改动文件（logRedaction.ts、spawnChannel.ts、channel-worker-supervisor.ts）零类型错误。（我 worktree 里的 tsc 噪声是本地 COW node_modules 造成的 simple-git/TS5055 脑裂产物，与本 PR 无关。）
备注（非阻塞，供后续加固——不是合并阻塞项）
- 关键字锚点的过度脱敏（安全方向）：因为
Bearer/QQBot/Authorization:是大小写不敏感匹配，即使在良性文字里，这些词后面的 token 也会被脱敏，例如using bearer authentication mode→using bearer <redacted> mode。它从不泄露——最坏只是丢失一个词的日志上下文。对脱敏器而言可接受，仅作说明。 - 残留覆盖缺口（纵深防御边界）：以下会泄露，因为不在模式集里——Google
AIza…、Stripesk_live_…（下划线而非sk-）、无Bearer前缀的裸 JWT、OpenAI-Organization: org-…、PEM 私钥正文行、64 位十六进制。Channel SDK 主要用已覆盖的形态（Bearer/Basic/QQBot/DingTalk），且 PR 已声明 64 KiB 跨界限制。其中个别（裸 JWT、Google key）可后续补，但都不该阻塞本 PR。
建议：LGTM。 正确、抗 ReDoS、不破坏良性日志行，集成路径在真实守护进程中被端到端证明承重。是对 #5976 的扎实安全加固。
yiliang114
removed this pull request from the merge queue due to the queue being cleared
Jul 2, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does
Adds a
redactLogCredentialsfunction that strips credentials from worker log lines before they reach the daemon's stderr and log file. Integrates the redaction into both worker stderr forwarding paths:createStderrForwardernow applies credential redaction in both the normal flush and the 64 KiB forced-truncation code paths.'inherit'to'pipe'for stderr, adds a line-buffered forwarder with redaction, 64 KiB buffer cap, try-catch protection, andonDiagnosticLinecallback so worker stderr also reaches the daemon log file (previously it only went to the daemon's terminal).Credential patterns covered: Bearer/QQBot tokens, Authorization headers, API key prefixes (
sk-,ghp_,gho_,glpat-,xoxb-,xoxp-), AWS access key IDs, env-var secret assignments (including compound keys likeAWS_SECRET_ACCESS_KEY), JSON-quoted secret fields, URL-embedded credentials, and DingTalk access token headers.Why it's needed
Issue #5976 V1.5 follow-up. Post-merge review of PR #6098 identified that worker stderr forwarding had no credential redaction — sensitive values like
QWEN_DAEMON_TOKEN, API keys, andAuthorization: Bearerheaders from channel SDK HTTP debug logs could leak verbatim into the daemon's stderr and log file. The daemon channel worker's stderr was also not written to the daemon log file at all (only visible on the daemon's terminal) because it used'inherit'stdio.Reviewer Test Plan
How to verify
Run the redaction function unit tests:
Expected: 34 tests pass.
Run the stderr forwarder integration tests:
Expected: 26 tests pass, including 2 new redaction verification tests.
Run the supervisor tests:
Expected: 14 tests pass (7 pre-existing failures due to
sanitizeLogTextvitest alias — not related to this PR).Manual verification:
Check
~/.qwen/debug/daemon/<id>.log— worker stderr lines should appear with[channel-worker pid=...]prefix, and any credentials should show***REDACTED***.Evidence (Before & After)
N/A — security/infrastructure change, not user-visible.
Tested on
Environment (optional)
Unit tests only. Manual verification with
qwen serve --channelon macOS.Risk & Scope
'inherit'to'pipe'introduces pipe backpressure — acceptable because channel worker stderr is typically low-volume. Documented in code comment.ChannelWorkerChildinterface gains an optionalstderrproperty andCreateChannelWorkerSupervisorOptionsgains an optionalonDiagnosticLinecallback — both are additive.Linked Issues
Partial fix for #5976
中文说明
新增
redactLogCredentials函数，在 worker 日志行写入 daemon 的 stderr 和日志文件之前剥离其中的凭据信息。集成到两条 worker stderr 转发路径：createStderrForwarder的正常刷新和 64 KiB 强制截断路径均应用脱敏。'inherit'改为'pipe'，增加带脱敏的行缓冲转发器、64 KiB 缓冲区上限、try-catch 保护，以及onDiagnosticLine回调（使 worker stderr 也写入 daemon 日志文件，之前仅在终端可见）。覆盖的凭据模式：Bearer/QQBot token、Authorization header、API key 前缀（
sk-、ghp_、gho_等）、AWS access key ID、环境变量风格的 secret 赋值（含复合键名如AWS_SECRET_ACCESS_KEY）、JSON 格式的 secret 字段、URL 内嵌凭据、DingTalk access token header。Issue #5976 V1.5 follow-up。PR #6098 合并后评审发现 worker stderr 转发缺少凭据脱敏，敏感值可能原样泄露到 daemon 日志。
🤖 Generated with Qwen Code