-
Notifications
You must be signed in to change notification settings - Fork 3k
fix(scripts): avoid shell injection in sandbox command detection #6108
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.
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,72 @@ | ||
| /** | ||
| * @license | ||
| * Copyright 2026 Qwen Team | ||
| * SPDX-License-Identifier: Apache-2.0 | ||
| */ | ||
|
|
||
| import { execFileSync } from 'node:child_process'; | ||
| import { fileURLToPath } from 'node:url'; | ||
| import path from 'node:path'; | ||
| import { describe, expect, it } from 'vitest'; | ||
|
|
||
| const scriptPath = path.join( | ||
| path.dirname(fileURLToPath(import.meta.url)), | ||
| '..', | ||
| 'sandbox_command.js', | ||
| ); | ||
|
|
||
| /** | ||
| * Runs sandbox_command.js as a subprocess with the given QWEN_SANDBOX value. | ||
| * Returns { status, stdout, stderr }. Never throws on a non-zero exit so the | ||
| * caller can assert on the exit code. | ||
| */ | ||
| function runSandboxCommand(sandboxValue) { | ||
| try { | ||
| const stdout = execFileSync(process.execPath, [scriptPath, '-q'], { | ||
| encoding: 'utf8', | ||
| env: { ...process.env, QWEN_SANDBOX: sandboxValue }, | ||
| }); | ||
| return { status: 0, stdout, stderr: '' }; | ||
| } catch (err) { | ||
| return { | ||
| status: err.status ?? 1, | ||
| stdout: err.stdout?.toString() ?? '', | ||
| stderr: err.stderr?.toString() ?? '', | ||
| }; | ||
| } | ||
| } | ||
|
|
||
| describe('sandbox_command.js QWEN_SANDBOX handling', () => { | ||
| // Each payload appends a command that would exit 0 if the shell ever split | ||
| // the value on the metacharacter. A vulnerable build runs e.g. | ||
| // `command -v doesnotexist; true`, which exits 0, so commandExists() returns | ||
| // true and the script echoes the payload and exits 0. The hardened build | ||
| // treats the whole string as a single command name, fails to find it, and | ||
| // exits non-zero — so a regression here flips these assertions. | ||
| const injectionPayloads = [ | ||
| 'doesnotexist; true', | ||
| 'doesnotexist && true', | ||
| 'doesnotexist | true', | ||
| 'doesnotexist; echo pwned', | ||
| '$(true)', | ||
| '`true`', | ||
| ]; | ||
|
|
||
| for (const payload of injectionPayloads) { | ||
| it(`rejects the injection payload ${JSON.stringify(payload)} instead of executing it`, () => { | ||
| const { status, stdout } = runSandboxCommand(payload); | ||
| expect(status).not.toBe(0); | ||
| // The payload must never be accepted as a resolved sandbox command. | ||
| expect(stdout.trim()).toBe(''); | ||
| }); | ||
| } | ||
|
|
||
| it('reports the raw value as a single missing command (no shell splitting)', () => { | ||
| const payload = 'doesnotexist; echo pwned'; | ||
| const { status, stderr } = runSandboxCommand(payload); | ||
| expect(status).not.toBe(0); | ||
| // The entire string is echoed back verbatim, proving it was treated as one | ||
| // opaque command name rather than parsed by a shell. | ||
| expect(stderr).toContain(`missing sandbox command '${payload}'`); | ||
| }); | ||
| }); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] The test suite only covers rejection paths. Consider adding a happy-path test that verifies a legitimate sandbox command (e.g. it('accepts a legitimate sandbox command that exists', () => {
const { status, stdout } = runSandboxCommand('node');
expect(status).toBe(0);
expect(stdout.trim()).toBe('node');
});— qwen3.7-max via Qwen Code /review |
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Suggestion] Consider adding
||and embedded-newline payloads to the injection set.||is the most common "fail-open" injection vector (complementing the existing&&), and literal newlines are a classic IFS-splitting vector:— qwen3.7-max via Qwen Code /review