Skip to content

Commit 937b19d

Browse files
fix: CWE-78 shell injection via unsafe argument escaping in command executor (#289)
* fix: replace vulnerable shell escaping with POSIX single-quote method and sanitize predicate/version inputs - command.ts: Replace hand-rolled double-quote regex escaping with the existing shellEscapeArg() helper that uses POSIX single-quote wrapping. The previous regex missed $(), backticks, and newlines inside double quotes, allowing command injection when useShell=true. - log_capture.ts: Add escapePredicateString() to sanitize bundleId and custom subsystem values before interpolating them into NSPredicate strings, preventing predicate injection via crafted double-quotes or backslashes. - generate-version.ts: Validate version fields against a version pattern and use JSON.stringify() instead of raw string interpolation to prevent code injection via a compromised package.json. * fix: allow hyphens in semver pre-release/build-metadata regex * test: add regression tests for shell escaping, predicate escaping, version validation, and document unfixed bundle-id injection vectors * fix: remove unused ChildProcess import (dead code) * chore(eslint): ignore generated src/version.ts * test: rebase bundle-id injection tests onto current main APIs Update fixture-style tests so they compile and run against the current CommandResponse shape (process is now required) and the void-returning get_mac_bundle_idLogic, which now reads handler context internally and needs runLogic to provide it. * test: replace 'as any' with typed cast in log_capture_escape mocks Project rule disallows 'any' types unless absolutely necessary. The mock ChildProcess and WriteStream stubs use 'as unknown as <Type>' instead, matching the pattern used in the new bundle-id-injection tests. * refactor: drop redundant inline comments in shellEscapeArg The function docstring already explains the POSIX single-quote escape technique. The inline comments restated the same thing. Project style discourages narrative inline comments. --------- Co-authored-by: Cameron Cooke <web@cameroncooke.com>
1 parent 6265454 commit 937b19d

10 files changed

Lines changed: 557 additions & 17 deletions

eslint.config.js

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ export default [
1313
'coverage/**',
1414
'src/core/generated-plugins.ts',
1515
'src/core/generated-resources.ts',
16+
'src/version.ts',
1617
],
1718
},
1819
{

scripts/generate-version.ts

Lines changed: 20 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,16 @@ function parseGitHubOwnerAndName(url: string): { owner: string; name: string } {
1919
return { owner: match[1], name: match[2] };
2020
}
2121

22+
const VERSION_REGEX = /^v?[0-9]+\.[0-9]+\.[0-9]+(-[a-zA-Z0-9.\-]+)?(\+[a-zA-Z0-9.\-]+)?$/;
23+
24+
function validateVersion(name: string, value: string): void {
25+
if (!VERSION_REGEX.test(value)) {
26+
throw new Error(
27+
`Invalid ${name} in package.json: ${JSON.stringify(value)}. Expected a version string.`,
28+
);
29+
}
30+
}
31+
2232
async function main(): Promise<void> {
2333
const repoRoot = process.cwd();
2434
const packagePath = path.join(repoRoot, 'package.json');
@@ -34,13 +44,17 @@ async function main(): Promise<void> {
3444

3545
const repo = parseGitHubOwnerAndName(repoUrl);
3646

47+
validateVersion('version', pkg.version);
48+
validateVersion('iOSTemplateVersion', pkg.iOSTemplateVersion);
49+
validateVersion('macOSTemplateVersion', pkg.macOSTemplateVersion);
50+
3751
const content =
38-
`export const version = '${pkg.version}';\n` +
39-
`export const iOSTemplateVersion = '${pkg.iOSTemplateVersion}';\n` +
40-
`export const macOSTemplateVersion = '${pkg.macOSTemplateVersion}';\n` +
41-
`export const packageName = '${pkg.name}';\n` +
42-
`export const repositoryOwner = '${repo.owner}';\n` +
43-
`export const repositoryName = '${repo.name}';\n`;
52+
`export const version = ${JSON.stringify(pkg.version)};\n` +
53+
`export const iOSTemplateVersion = ${JSON.stringify(pkg.iOSTemplateVersion)};\n` +
54+
`export const macOSTemplateVersion = ${JSON.stringify(pkg.macOSTemplateVersion)};\n` +
55+
`export const packageName = ${JSON.stringify(pkg.name)};\n` +
56+
`export const repositoryOwner = ${JSON.stringify(repo.owner)};\n` +
57+
`export const repositoryName = ${JSON.stringify(repo.name)};\n`;
4458

4559
await writeFile(versionPath, content, 'utf8');
4660
}
Lines changed: 100 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,100 @@
1+
import { describe, it, expect } from 'vitest';
2+
import type { ChildProcess } from 'node:child_process';
3+
import { extractBundleIdFromAppPath } from '../bundle-id.ts';
4+
import type { CommandExecutor } from '../CommandExecutor.ts';
5+
6+
/**
7+
* CWE-78 regression tests for bundle-id.ts
8+
*
9+
* These tests verify that user-supplied appPath values containing shell
10+
* metacharacters do NOT result in shell injection when passed through
11+
* the executeSyncCommand → /bin/sh -c pipeline.
12+
*
13+
* CURRENT STATUS: These tests demonstrate the UNFIXED injection vectors
14+
* identified in the review. The command string passed to /bin/sh -c
15+
* contains unescaped user input, which would allow command injection.
16+
*/
17+
18+
type CapturedCall = {
19+
command: string[];
20+
logPrefix?: string;
21+
};
22+
23+
const stubProcess = { pid: 1, on: () => stubProcess } as unknown as ChildProcess;
24+
25+
function createCapturingExecutor(calls: CapturedCall[]): CommandExecutor {
26+
return async (command, logPrefix) => {
27+
calls.push({ command: [...command], logPrefix });
28+
// Simulate 'defaults' returning a fake bundle ID
29+
return { success: true, output: 'com.example.app', process: stubProcess };
30+
};
31+
}
32+
33+
describe('bundle-id.ts — CWE-78 shell injection vectors', () => {
34+
it('UNFIXED: double-quote breakout in appPath reaches /bin/sh -c unescaped', async () => {
35+
const calls: CapturedCall[] = [];
36+
const executor = createCapturingExecutor(calls);
37+
38+
// Malicious appPath that breaks out of the double-quoted context
39+
const maliciousPath = '/tmp/evil" $(id) "bar';
40+
await extractBundleIdFromAppPath(maliciousPath, executor);
41+
42+
expect(calls).toHaveLength(1);
43+
const shellCommand = calls[0].command;
44+
45+
// The command is ['/bin/sh', '-c', '...']
46+
expect(shellCommand[0]).toBe('/bin/sh');
47+
expect(shellCommand[1]).toBe('-c');
48+
49+
const cmdString = shellCommand[2];
50+
51+
// VULNERABILITY: The raw user input is interpolated directly into the
52+
// shell command string. The $(id) is NOT escaped and would execute.
53+
// A safe implementation would either:
54+
// 1. Not use shell at all (pass args array to spawn directly), or
55+
// 2. Properly escape the appPath with shellEscapeArg
56+
//
57+
// This test documents the current vulnerable behavior.
58+
// When the fix is applied, update the assertion to verify safety.
59+
expect(cmdString).toContain('$(id)');
60+
61+
// Verify the command reaches shell — it's using /bin/sh -c
62+
expect(shellCommand[0]).toBe('/bin/sh');
63+
});
64+
65+
it('UNFIXED: semicolon injection in appPath allows command chaining', async () => {
66+
const calls: CapturedCall[] = [];
67+
const executor = createCapturingExecutor(calls);
68+
69+
const maliciousPath = '/tmp/foo"; rm -rf / ; echo "';
70+
await extractBundleIdFromAppPath(maliciousPath, executor);
71+
72+
const cmdString = calls[0].command[2];
73+
74+
// The rm -rf command is embedded in the shell string unescaped
75+
expect(cmdString).toContain('rm -rf');
76+
});
77+
78+
it('UNFIXED: backtick injection in appPath', async () => {
79+
const calls: CapturedCall[] = [];
80+
const executor = createCapturingExecutor(calls);
81+
82+
const maliciousPath = '/tmp/`touch /tmp/pwned`';
83+
await extractBundleIdFromAppPath(maliciousPath, executor);
84+
85+
const cmdString = calls[0].command[2];
86+
expect(cmdString).toContain('`touch /tmp/pwned`');
87+
});
88+
89+
it('safe appPath without metacharacters works normally', async () => {
90+
const calls: CapturedCall[] = [];
91+
const executor = createCapturingExecutor(calls);
92+
93+
const safePath = '/Users/dev/Build/Products/Debug/MyApp.app';
94+
const result = await extractBundleIdFromAppPath(safePath, executor);
95+
96+
expect(result).toBe('com.example.app');
97+
expect(calls).toHaveLength(1);
98+
expect(calls[0].command[2]).toContain(safePath);
99+
});
100+
});
Lines changed: 71 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,71 @@
1+
import { describe, it, expect } from 'vitest';
2+
3+
// We cannot easily import the generate-version script (it runs main() immediately),
4+
// so we extract and test the core logic: VERSION_REGEX and JSON.stringify defense.
5+
6+
const VERSION_REGEX = /^v?[0-9]+\.[0-9]+\.[0-9]+(-[a-zA-Z0-9.\-]+)?(\+[a-zA-Z0-9.\-]+)?$/;
7+
8+
describe('generate-version: VERSION_REGEX validation', () => {
9+
it('accepts standard semver', () => {
10+
expect(VERSION_REGEX.test('2.3.0')).toBe(true);
11+
});
12+
13+
it('accepts v-prefixed semver', () => {
14+
expect(VERSION_REGEX.test('v1.0.8')).toBe(true);
15+
});
16+
17+
it('accepts pre-release semver', () => {
18+
expect(VERSION_REGEX.test('1.0.0-beta.1')).toBe(true);
19+
});
20+
21+
it('accepts pre-release with hyphens', () => {
22+
expect(VERSION_REGEX.test('1.0.0-rc-1')).toBe(true);
23+
});
24+
25+
it('accepts build metadata', () => {
26+
expect(VERSION_REGEX.test('1.0.0+build.123')).toBe(true);
27+
});
28+
29+
it('accepts pre-release + build metadata', () => {
30+
expect(VERSION_REGEX.test('1.0.0-alpha.1+meta')).toBe(true);
31+
});
32+
33+
it('rejects injection payloads with single quotes', () => {
34+
expect(VERSION_REGEX.test("'; process.exit(1); //")).toBe(false);
35+
});
36+
37+
it('rejects injection payloads with template literals', () => {
38+
expect(VERSION_REGEX.test('${process.exit(1)}')).toBe(false);
39+
});
40+
41+
it('rejects empty string', () => {
42+
expect(VERSION_REGEX.test('')).toBe(false);
43+
});
44+
45+
it('rejects arbitrary text', () => {
46+
expect(VERSION_REGEX.test('not-a-version')).toBe(false);
47+
});
48+
});
49+
50+
describe('generate-version: JSON.stringify defense-in-depth', () => {
51+
it('produces safe code even if a value somehow contains quotes', () => {
52+
const malicious = "1.0.0'; process.exit(1); //";
53+
const generated = `const version = ${JSON.stringify(malicious)};\n`;
54+
// The output should use escaped double-quoted string, not break out
55+
expect(generated).toContain('"1.0.0');
56+
expect(generated).not.toContain("'1.0.0'; process.exit(1)");
57+
// Should be parseable JS (using const instead of export for Function() compat)
58+
expect(() => new Function(generated)).not.toThrow();
59+
});
60+
61+
it('JSON.stringify properly escapes backslashes and control characters', () => {
62+
const tricky = '1.0.0\n";process.exit(1);//';
63+
const serialized = JSON.stringify(tricky);
64+
// The newline should be escaped as \\n, and the quote should be escaped
65+
expect(serialized).toContain('\\n');
66+
expect(serialized).toContain('\\"');
67+
// The resulting assignment should be valid JS
68+
const code = `const v = ${serialized};`;
69+
expect(() => new Function(code)).not.toThrow();
70+
});
71+
});

0 commit comments

Comments
 (0)