Skip to content

Commit 0ecd0fc

Browse files
carlos-almcarlos-alm
authored
fix(benchmark): spawn npm via shell on Windows (#973)
* fix(benchmark): spawn npm via shell on Windows Node refuses to spawnSync .cmd/.bat files without shell: true since Node 18.20 / 20.15. `npm run benchmark --npm` failed on Windows because scripts/lib/bench-config.ts invoked execFileSync('npm', ...) directly. Introduce NPM_CMD / NPM_SHELL constants derived from os.platform() and apply them at all three npm install call sites. Fixes #966 * fix(benchmark): simplify npm spawn + harden install specs (#973) Addresses Greptile review feedback on PR #973: - Drop redundant NPM_CMD constant. When shell: true on Windows, cmd.exe resolves bare `npm` to `npm.cmd` automatically, so a single NPM_SHELL boolean is sufficient and all three execFileSync calls can use 'npm' directly. - Add assertSafePkgName / assertSafePkgVersion guards and apply them to every registry- or package.json-sourced string that gets interpolated into an `npm install` spec. This closes the shell-injection surface flagged in the PR description without waiting on a separate follow-up. Impact: 3 functions changed, 7 affected * fix(benchmark): validate version before logging (#973) Addresses Greptile review nit on PR #973: the log line at the top of the npm mode path emitted the raw version string before assertSafePkgVersion fired. Move the validation up-front so no unvalidated value is ever logged or interpolated. Same for the native package log: validate before logging, not after. Impact: 1 functions changed, 6 affected
1 parent fc5bfe9 commit 0ecd0fc

1 file changed

Lines changed: 59 additions & 15 deletions

File tree

‎scripts/lib/bench-config.ts‎

Lines changed: 59 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,35 @@ import { pathToFileURL } from 'node:url';
1515

1616
import { getBenchmarkVersion } from '../bench-version.js';
1717

18+
// On Windows, `npm` is `npm.cmd` and Node refuses to spawn `.cmd`/`.bat`
19+
// without `shell: true` (since Node 18.20 / 20.15). When `shell: true`, the
20+
// Windows `cmd.exe` shell resolves bare `npm` to `npm.cmd` automatically, so
21+
// a single boolean is sufficient.
22+
const NPM_SHELL = os.platform() === 'win32';
23+
24+
// Strict guards for any string that gets interpolated into an npm install
25+
// spec while running with `shell: true`. Registry-sourced values are
26+
// constrained by npm itself, but we enforce the constraint locally so a
27+
// compromised upstream or local `package.json` can't inject shell metacharacters.
28+
const PKG_NAME_RE = /^(?:@[a-z0-9][a-z0-9._-]*\/)?[a-z0-9][a-z0-9._-]*$/i;
29+
// Permit exact versions, ranges, dist-tags, and common operators — but no
30+
// shell metacharacters. Intentionally conservative; tighten if needed.
31+
const PKG_VERSION_RE = /^[a-z0-9._+\-~^>=<|* ]+$/i;
32+
33+
function assertSafePkgName(name: string): string {
34+
if (!PKG_NAME_RE.test(name)) {
35+
throw new Error(`Refusing to install package with unsafe name: ${JSON.stringify(name)}`);
36+
}
37+
return name;
38+
}
39+
40+
function assertSafePkgVersion(version: string): string {
41+
if (!PKG_VERSION_RE.test(version)) {
42+
throw new Error(`Refusing to install package with unsafe version: ${JSON.stringify(version)}`);
43+
}
44+
return version;
45+
}
46+
1847
/**
1948
* Parse `--version <v>` and `--npm` from process.argv.
2049
*/
@@ -56,11 +85,14 @@ export async function resolveBenchmarkSource() {
5685
};
5786
}
5887

59-
// npm mode — install @optave/codegraph@<version> into a temp dir
60-
const version = cliVersion || 'latest';
88+
// npm mode — install @optave/codegraph@<version> into a temp dir.
89+
// Validate the version up-front so we never log or interpolate an
90+
// unvalidated string (with `shell: true` on Windows, bad input would be a
91+
// shell-injection surface).
92+
const safeVersion = assertSafePkgVersion(cliVersion || 'latest');
6193
const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'codegraph-bench-'));
6294

63-
console.error(`Installing @optave/codegraph@${version} into ${tmpDir}...`);
95+
console.error(`Installing @optave/codegraph@${safeVersion} into ${tmpDir}...`);
6496

6597
// Write a minimal package.json so npm install works
6698
fs.writeFileSync(path.join(tmpDir, 'package.json'), JSON.stringify({ private: true }));
@@ -69,17 +101,18 @@ export async function resolveBenchmarkSource() {
69101
const maxRetries = 5;
70102
for (let attempt = 1; attempt <= maxRetries; attempt++) {
71103
try {
72-
execFileSync('npm', ['install', `@optave/codegraph@${version}`, '--no-audit', '--no-fund'], {
104+
execFileSync('npm', ['install', `@optave/codegraph@${safeVersion}`, '--no-audit', '--no-fund'], {
73105
cwd: tmpDir,
74106
stdio: 'pipe',
75107
timeout: 120_000,
108+
shell: NPM_SHELL,
76109
});
77110
break;
78111
} catch (err) {
79112
if (attempt === maxRetries) {
80113
// Clean up before throwing
81114
fs.rmSync(tmpDir, { recursive: true, force: true });
82-
throw new Error(`Failed to install @optave/codegraph@${version} after ${maxRetries} attempts: ${err.message}`);
115+
throw new Error(`Failed to install @optave/codegraph@${safeVersion} after ${maxRetries} attempts: ${err.message}`);
83116
}
84117
const delay = attempt * 15_000; // 15s, 30s, 45s, 60s
85118
console.error(` Attempt ${attempt} failed, retrying in ${delay / 1000}s...`);
@@ -109,14 +142,19 @@ export async function resolveBenchmarkSource() {
109142
const platformKey = `codegraph-${platform}-${arch}${libcSuffix}`;
110143
const nativePkg = Object.keys(optDeps).find((name) => name.includes(platformKey));
111144
if (nativePkg) {
112-
const nativeVersion = optDeps[nativePkg];
113-
console.error(`Installing native package ${nativePkg}@${nativeVersion}...`);
145+
// Even though these originate from the installed package's
146+
// optionalDependencies (i.e. the npm registry), validate before
147+
// logging or interpolating into a `shell: true` command line.
148+
const safeNativePkg = assertSafePkgName(nativePkg);
149+
const safeNativeVersion = assertSafePkgVersion(optDeps[nativePkg]);
150+
console.error(`Installing native package ${safeNativePkg}@${safeNativeVersion}...`);
114151
for (let attempt = 1; attempt <= maxRetries; attempt++) {
115152
try {
116-
execFileSync('npm', ['install', `${nativePkg}@${nativeVersion}`, '--no-audit', '--no-fund', '--no-save'], {
153+
execFileSync('npm', ['install', `${safeNativePkg}@${safeNativeVersion}`, '--no-audit', '--no-fund', '--no-save'], {
117154
cwd: tmpDir,
118155
stdio: 'pipe',
119156
timeout: 120_000,
157+
shell: NPM_SHELL,
120158
});
121159
break;
122160
} catch (innerErr) {
@@ -126,7 +164,7 @@ export async function resolveBenchmarkSource() {
126164
await new Promise((resolve) => setTimeout(resolve, delay));
127165
}
128166
}
129-
console.error(`Installed ${nativePkg}@${nativeVersion}`);
167+
console.error(`Installed ${safeNativePkg}@${safeNativeVersion}`);
130168
} else {
131169
console.error(`No native package found for platform ${platform}-${arch}${libcSuffix}, skipping`);
132170
}
@@ -143,12 +181,18 @@ export async function resolveBenchmarkSource() {
143181
);
144182
const hfVersion = localPkg.devDependencies?.['@huggingface/transformers'];
145183
if (hfVersion) {
146-
console.error(`Installing @huggingface/transformers@${hfVersion} for embedding benchmarks...`);
147-
execFileSync('npm', ['install', `@huggingface/transformers@${hfVersion}`, '--no-audit', '--no-fund', '--no-save'], {
148-
cwd: tmpDir,
149-
stdio: 'pipe',
150-
timeout: 120_000,
151-
});
184+
const safeHfVersion = assertSafePkgVersion(hfVersion);
185+
console.error(`Installing @huggingface/transformers@${safeHfVersion} for embedding benchmarks...`);
186+
execFileSync(
187+
'npm',
188+
['install', `@huggingface/transformers@${safeHfVersion}`, '--no-audit', '--no-fund', '--no-save'],
189+
{
190+
cwd: tmpDir,
191+
stdio: 'pipe',
192+
timeout: 120_000,
193+
shell: NPM_SHELL,
194+
},
195+
);
152196
console.error('Installed @huggingface/transformers');
153197
}
154198
} catch (err) {

0 commit comments

Comments
 (0)