Skip to content

fix(cli): extend the #8663 loader denylist and harden its scrub lifecycle #8763

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
wenshao merged 12 commits into from
Aug 10, 2026
Merged

fix(cli): extend the #8663 loader denylist and harden its scrub lifecycle #8763

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
92c525f
fix(cli): extend the #8663 loader denylist and harden its scrub lifec…
wenshao Aug 8, 2026
eaac7ba
Merge branch 'main' into fix/8663-loader-denylist-followup
qwen-code-dev-bot Aug 8, 2026
abee0fc
Merge branch 'main' into fix/8663-loader-denylist-followup
qwen-code-dev-bot Aug 8, 2026
e34df6d
fix(cli): keep the loader-scrub process.env access in the serve guard…
wenshao Aug 8, 2026
46e2690
fix(cli): block GIT_SSH and GIT_CONFIG_PARAMETERS in the project-env …
qwen-code-dev-bot Aug 9, 2026
ade3833
fix(cli): extend the project-env denylist across git exec, TLS, and r…
qwen-code-dev-bot Aug 9, 2026
b77fc00
Merge remote-tracking branch 'origin/main' into fix/8663-loader-denyl…
qwen-code-dev-bot Aug 9, 2026
eca8ad2
fix(cli): harden the project-env denylist and nested scrub snapshot (…
qwen-code-dev-bot Aug 9, 2026
3b70728
Merge branch 'main' into fix/8663-loader-denylist-followup
qwen-code-dev-bot Aug 9, 2026
146c448
fix(cli): merge the loader-env scrub snapshot into one pass (#8763)
qwen-code-dev-bot Aug 10, 2026
28352d5
fix(cli): block the round-4 exec-redirect env keys from project files…
qwen-code-dev-bot Aug 10, 2026
02fcc3b
Merge branch 'main' into fix/8663-loader-denylist-followup
qwen-code-dev-bot Aug 10, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
fix(cli): keep the loader-scrub process.env access in the serve guard… 
… surface

The refcounted acquireInheritedLoaderEnvScrub read/wrote process.env from
config/shared-env-keys.ts, which the serve process.env guard does not scan —
moving the access out of run-qwen-serve.ts dropped its allowlisted count and
failed process-env-guard.test.ts. Pass the env into the coordinator instead so
run-qwen-serve.ts still owns the process.env reference (matching the existing
scrub helpers), and update the allowlist to the new count.
  • Loading branch information
@wenshao
wenshao committed Aug 8, 2026
commit e34df6db92fe7442eca8fe8f24923c55bceb38f6
18 changes: 15 additions & 3 deletions packages/cli/src/config/shared-env-keys.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -489,12 +489,20 @@ describe('acquireInheritedLoaderEnvScrub', () => {
.mockImplementation(() => true);
try {
// Daemon A boots and scrubs the shared env.
const daemonA = acquireInheritedLoaderEnvScrub('qwen serve', 'daemon');
const daemonA = acquireInheritedLoaderEnvScrub(
process.env,
'qwen serve',
'daemon',
);
expect(daemonA.removedKeys).toContain('NODE_OPTIONS');
expect(process.env['NODE_OPTIONS']).toBeUndefined();

// Daemon B boots into the already-scrubbed env: nothing left to remove.
const daemonB = acquireInheritedLoaderEnvScrub('qwen serve', 'daemon');
const daemonB = acquireInheritedLoaderEnvScrub(
process.env,
'qwen serve',
'daemon',
);
expect(daemonB.removedKeys).toEqual([]);
expect(process.env['NODE_OPTIONS']).toBeUndefined();

Expand All @@ -518,7 +526,11 @@ describe('acquireInheritedLoaderEnvScrub', () => {
.spyOn(process.stderr, 'write')
.mockImplementation(() => true);
try {
const handle = acquireInheritedLoaderEnvScrub('qwen serve', 'daemon');
const handle = acquireInheritedLoaderEnvScrub(
process.env,
'qwen serve',
'daemon',
);
expect(process.env['LD_PRELOAD']).toBeUndefined();
// A legitimate re-assignment before release must survive the restore.
process.env['LD_PRELOAD'] = '/legit.so';
Expand Down
20 changes: 12 additions & 8 deletions packages/cli/src/config/shared-env-keys.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -281,25 +281,29 @@ export interface InheritedLoaderEnvScrubHandle {
release(): void;
}

// Scrubs loader vars from the live `process.env`, reference-counted so it is
// safe to call from overlapping daemon instances in one process. Use this at
// process-env boundaries; `scrubAndReportInheritedLoaderEnv` remains for
// one-shot scrubs of a private env object (ACP child / channel worker boot).
// Scrubs loader vars from a shared, live env (the daemon passes its own
// `process.env`), reference-counted so it is safe to call from overlapping
// daemon instances in one process. The caller owns the `process.env`
// reference so the serve-surface process.env guard still sees the access;
// `scrubAndReportInheritedLoaderEnv` remains for one-shot scrubs of a private
// env object (ACP child / channel worker boot). Concurrent callers must pass
// the same shared env object for the snapshot/restore refcount to be correct.
export function acquireInheritedLoaderEnvScrub(
env: NodeJS.ProcessEnv,
commandLabel: string,
processLabel: string,
): InheritedLoaderEnvScrubHandle {
if (sharedProcessEnvScrubDepth === 0) {
sharedProcessEnvScrubOriginals.clear();
for (const key of Object.keys(process.env)) {
for (const key of Object.keys(env)) {
if (!isLoaderEnvKey(key)) continue;
const value = process.env[key];
const value = env[key];
if (value !== undefined) sharedProcessEnvScrubOriginals.set(key, value);

@qwen-code-dev-bot qwen-code-dev-bot Aug 9, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deferred to a follow-up. Valid surviving mutant: no test pins when the undefined-guard must not fire. The negative case (an explicitly undefined loader key is neither snapshotted nor restored) shares its setup with the two-cycle acquire/release test and will land with it.

中文说明

延迟到下一轮。存活变异体属实：没有测试钉住 undefined 守卫"不应触发"的情形。负例（显式为 undefined 的 loader 键既不被快照也不被恢复）与双周期 acquire/release 测试共用 setup，将与其一起合入。

}
}
sharedProcessEnvScrubDepth++;
const removedKeys = scrubAndReportInheritedLoaderEnv(
process.env,
env,
Comment on lines +398 to +407

@wenshao wenshao Aug 9, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Critical] R1-2: A nested scrub acquisition deletes a later host assignment without snapshotting it. — Failure scenario: daemon A acquires, the embedding host assigns NODE_OPTIONS, then daemon B acquires and scrubs it; the final release leaves the value absent or restores an older snapshot, corrupting the embedding process environment. Preserve newly present loader-key assignments before nested scrubs and add the exact A → assign → B → release regression test.

中文说明

R1-2： 嵌套 scrub acquire 会删除后续 host 赋值，却没有把它记入快照。失败场景：daemon A acquire 后，嵌入宿主设置 NODE_OPTIONS，随后 daemon B acquire 并将其剥离；最终 release 后该值保持缺失或恢复成更旧的快照，破坏嵌入进程环境。请在嵌套剥离前保存新出现的 loader 键赋值，并增加精确的 A → 赋值 → B → release 回归测试。

— gpt-5.6-sol via Qwen Code /review (v0.21.8)

commandLabel,
processLabel,
);
Expand All @@ -313,7 +317,7 @@ export function acquireInheritedLoaderEnvScrub(
if (sharedProcessEnvScrubDepth > 0) return;
for (const [key, value] of sharedProcessEnvScrubOriginals) {
Comment on lines +419 to +420

@doudouOUC doudouOUC Aug 10, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] Mutant-survived: deleting the sharedProcessEnvScrubOriginals.clear() at release time (line 397) leaves every affected test green. — Failure scenario: Same as the acquire-time clear — a future removal would not be caught by any test. Suggested fix: Confirm or add coverage for this reset path.

中文说明

变体存活：删除 release 时的 sharedProcessEnvScrubOriginals.clear()（第 397 行）所有测试仍然通过。建议：确认或补充此重置路径的测试覆盖。

— deepseek-v4-flash via Qwen Code /review (v0.21.8)

// A later legitimate assignment wins over the restore.
if (!Object.hasOwn(process.env, key)) process.env[key] = value;
if (!Object.hasOwn(env, key)) env[key] = value;
}
sharedProcessEnvScrubOriginals.clear();
Comment on lines +422 to +424

@qwen-code-dev-bot qwen-code-dev-bot Aug 9, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deferred to a follow-up. Valid surviving mutant with the same masking chain as the acquire-time clear; the shared two-cycle acquire/release test covers both and will land in the follow-up round.

中文说明

延迟到下一轮。存活变异体属实，掩盖链与 acquire 时的 clear 相同；共用的双周期 acquire/release 测试同时覆盖两者，将在下一轮合入。

},
Expand Down
3 changes: 1 addition & 2 deletions packages/cli/src/serve/process-env-guard.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,13 +155,12 @@ const allowedProcessEnvAccesses = normalizeAllowances([
'computed:QWEN_SERVE_PROMPT_DEADLINE_MS_ENV': 1,
'computed:QWEN_SERVE_WRITER_IDLE_TIMEOUT_MS_ENV': 1,
'computed:RUNTIME_STARTUP_TIMEOUT_ENV': 1,
'computed:key': 1,
'key:DEV': 1,
'key:QWEN_CODE_IDE_WORKSPACE_PATH': 1,
'key:QWEN_SERVE_NO_MCP_POOL': 1,
'key:QWEN_SERVE_NO_PERSISTENT_REGISTRATION': 1,
'key:VITEST_WORKER_ID': 1,
whole: 7,
whole: 5,
},
},
],
Expand Down
6 changes: 5 additions & 1 deletion packages/cli/src/serve/run-qwen-serve.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2141,7 +2141,11 @@ async function runQwenServeImpl(
// process do not restore each other's loader vars mid-flight, and reverted
// on close() so an embedded caller reusing the host process gets its launch
// environment back.
const loaderEnvScrub = acquireInheritedLoaderEnvScrub('qwen serve', 'daemon');
const loaderEnvScrub = acquireInheritedLoaderEnvScrub(
process.env,
'qwen serve',
'daemon',
);
const scrubbedLoaderEnvKeys = loaderEnvScrub.removedKeys;
Comment on lines +2148 to +2153

@qwen-code-dev-bot qwen-code-dev-bot Aug 9, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Declined this round (recorded, not dropped): the finding is diagnostics-only — reject/restore behavior itself is correct and test-pinned, as the finding notes. Durably logging the coordination state when a handle joins an already-active scrub, and surfacing the depth-0 restore through the callback channel before daemonLog.close(), is a non-trivial rework of the serve daemon lifecycle (new callback surface plus durable-log ordering), which is disproportionate diff growth for a diagnostics gap in this budget-constrained round. The audit-trail scenario stays traceable through the first daemon's log. Happy to take this as a follow-up if a maintainer wants it sooner.

中文说明

本轮已记录并拒绝（非静默丢弃）：该发现仅影响诊断——正如发现本身所述，拒绝/恢复行为是正确的，且有测试钉住。在 handle 加入已激活的剥离时持久记录协调状态、并在 daemonLog.close() 之前通过回调通道上报 depth-0 恢复，需要对 serve daemon 生命周期做不小的改造（新增回调面 + 持久日志排序），对于预算受限轮次中的一个诊断缺口而言属于不成比例的 diff 增长。该审计线索场景仍可通过第一个 daemon 的日志追溯。若维护者希望更早解决，愿意作为后续跟进处理。

const restoreScrubbedLoaderEnv = (): void => {
loaderEnvScrub.release();
Expand Down
Loading