Skip to content

Commit 2fc546e

Browse files
committed
Remove command-based argument validation in CI mode
The previous approach of parsing command arguments to validate file access was fundamentally flawed and caused bugs like incorrectly treating grep patterns as file paths (e.g., grep "pattern" file.txt). Changes: - Removed extractFilePaths() method with command-specific parsing logic - Removed validateCommand() method that attempted to validate file access - Updated CI mode to clearly warn that sandboxing is disabled - Updated security tests to skip when running in CI mode - Added documentation that CI environments run without sandboxing The command-based validation was security theater that: 1. Could be bypassed with shell redirections, symlinks, etc. 2. Broke legitimate use cases (grep, echo, find, etc.) 3. Was impossible to maintain for all command syntaxes Real security comes from bubblewrap isolation. In CI environments without bubblewrap support, users should use Docker containers.
1 parent c2da51e commit 2fc546e

2 files changed

Lines changed: 20 additions & 257 deletions

File tree

src/__tests__/security-validation.test.ts

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ import { join } from 'path';
1212
describe('Security Validation Tests', () => {
1313
const testDir = process.cwd();
1414
let sandbox: PlatformSandbox;
15+
const isCI = process.env.CI === 'true' || process.env.GITHUB_ACTIONS === 'true';
1516

1617
beforeAll(async () => {
1718
const isAvailable = await PlatformSandbox.isAvailable();
@@ -21,17 +22,27 @@ describe('Security Validation Tests', () => {
2122
);
2223
}
2324

25+
if (isCI) {
26+
console.warn('⚠️ Running in CI mode - sandboxing is disabled, security tests will be skipped');
27+
}
28+
2429
const config = getDefaultConfig(testDir);
2530
sandbox = new PlatformSandbox(config);
2631
});
2732

2833
/**
2934
* TEST 1: Block SSH Key Access
3035
* Claim: "Automatically blocks access to SSH keys (~/.ssh)"
36+
*
37+
* NOTE: This test is skipped in CI environments where bubblewrap is unavailable.
38+
* CI environments run without sandboxing and cannot enforce security boundaries.
3139
*/
3240
it('TEST 1: Should block access to SSH private keys', async () => {
3341
const isAvailable = await PlatformSandbox.isAvailable();
34-
if (!isAvailable) return;
42+
if (!isAvailable || isCI) {
43+
console.log('Skipped: Sandboxing not available');
44+
return;
45+
}
3546

3647
const sshKeyPath = join(homedir(), '.ssh', 'id_rsa');
3748

src/filesystem-sandbox.ts

Lines changed: 8 additions & 256 deletions
Original file line numberDiff line numberDiff line change
@@ -10,13 +10,15 @@ export class FilesystemSandbox {
1010

1111
constructor(private config: SandboxConfig) {
1212
// In CI environments without user namespace support, we can't use bubblewrap's
13-
// mount isolation features. Fall back to direct execution with permission checking.
13+
// mount isolation features. Fall back to direct execution WITHOUT sandboxing.
1414
const isCI = process.env.CI === 'true' || process.env.GITHUB_ACTIONS === 'true';
1515
if (isCI) {
1616
// GitHub Actions and similar CI environments typically don't support user namespaces
1717
// which are required for bubblewrap's bind mounts. Use direct execution instead.
1818
this.useDirectExecution = true;
19-
console.log('CI environment detected - using direct execution with permission validation');
19+
console.warn('⚠️ CI environment detected - bubblewrap sandboxing is DISABLED');
20+
console.warn('⚠️ Commands will run with full filesystem access');
21+
console.warn('⚠️ For secure sandboxing in CI, use Docker or similar containerization');
2022
}
2123
}
2224

@@ -78,25 +80,16 @@ export class FilesystemSandbox {
7880

7981
/**
8082
* Execute command directly without bubblewrap (for CI environments)
81-
* Validates paths and blocks forbidden access
83+
*
84+
* WARNING: This mode does NOT provide security sandboxing.
85+
* Commands run with full access to the filesystem.
86+
* For secure sandboxing in CI, use Docker or similar containerization.
8287
*/
8388
private executeDirectly(
8489
command: string[],
8590
options: ExecuteOptions,
8691
startTime: number
8792
): Promise<CommandResult> {
88-
// Validate command for forbidden file access
89-
const validation = this.validateCommand(command);
90-
if (!validation.allowed) {
91-
const duration = Date.now() - startTime;
92-
return Promise.resolve({
93-
exitCode: 1,
94-
stdout: '',
95-
stderr: `Permission denied: ${validation.reason}`,
96-
duration,
97-
});
98-
}
99-
10093
return new Promise((resolve, reject) => {
10194
const [cmd, ...args] = command;
10295
const proc = spawn(cmd, args, {
@@ -139,247 +132,6 @@ export class FilesystemSandbox {
139132
});
140133
}
141134

142-
/**
143-
* Extract file paths from command arguments based on command-specific parsing rules
144-
*/
145-
private extractFilePaths(cmd: string, args: string[]): { readPaths: string[]; writePaths: string[] } {
146-
const readPaths: string[] = [];
147-
const writePaths: string[] = [];
148-
149-
// Flags that take arguments (not file paths)
150-
const flagsWithArgs: Record<string, string[]> = {
151-
grep: ['-e', '-f', '-m', '-A', '-B', '-C', '--regexp', '--file', '--max-count'],
152-
head: ['-n', '-c', '--lines', '--bytes'],
153-
tail: ['-n', '-c', '--lines', '--bytes'],
154-
find: ['-name', '-type', '-size', '-user', '-group', '-perm', '-exec', '-maxdepth', '-mindepth'],
155-
};
156-
157-
const commandFlags = flagsWithArgs[cmd] || [];
158-
let skipNext = false;
159-
160-
switch (cmd) {
161-
case 'cat':
162-
case 'head':
163-
case 'tail':
164-
case 'less':
165-
case 'more':
166-
// All non-flag arguments are file paths
167-
for (let i = 0; i < args.length; i++) {
168-
if (skipNext) {
169-
skipNext = false;
170-
continue;
171-
}
172-
const arg = args[i];
173-
if (arg.startsWith('-')) {
174-
// Check if this flag takes an argument
175-
if (commandFlags.includes(arg)) {
176-
skipNext = true;
177-
}
178-
} else {
179-
readPaths.push(arg);
180-
}
181-
}
182-
break;
183-
184-
case 'grep':
185-
// First non-flag argument is the pattern, rest are files
186-
let foundPattern = false;
187-
for (let i = 0; i < args.length; i++) {
188-
if (skipNext) {
189-
skipNext = false;
190-
continue;
191-
}
192-
const arg = args[i];
193-
if (arg.startsWith('-')) {
194-
// Check if this flag takes an argument
195-
if (commandFlags.includes(arg)) {
196-
skipNext = true;
197-
}
198-
} else {
199-
if (!foundPattern) {
200-
// This is the pattern, not a file
201-
foundPattern = true;
202-
} else {
203-
// These are files
204-
readPaths.push(arg);
205-
}
206-
}
207-
}
208-
break;
209-
210-
case 'find':
211-
// First non-flag argument is the directory, then predicates
212-
let foundDirectory = false;
213-
for (let i = 0; i < args.length; i++) {
214-
if (skipNext) {
215-
skipNext = false;
216-
continue;
217-
}
218-
const arg = args[i];
219-
if (!foundDirectory && !arg.startsWith('-')) {
220-
// First non-flag arg is the directory to search
221-
readPaths.push(arg);
222-
foundDirectory = true;
223-
} else if (arg.startsWith('-')) {
224-
// Check if this flag takes an argument
225-
if (commandFlags.includes(arg)) {
226-
skipNext = true;
227-
}
228-
}
229-
// Other args are predicates, not file paths
230-
}
231-
break;
232-
233-
case 'touch':
234-
case 'tee':
235-
// All non-flag arguments are file paths (write access)
236-
for (let i = 0; i < args.length; i++) {
237-
if (skipNext) {
238-
skipNext = false;
239-
continue;
240-
}
241-
const arg = args[i];
242-
if (arg.startsWith('-')) {
243-
if (commandFlags.includes(arg)) {
244-
skipNext = true;
245-
}
246-
} else {
247-
writePaths.push(arg);
248-
}
249-
}
250-
break;
251-
252-
case 'echo':
253-
// Echo arguments are not file paths (unless redirected, handled separately)
254-
// No file paths to extract
255-
break;
256-
257-
case 'dd':
258-
// dd uses if=/path and of=/path syntax
259-
for (const arg of args) {
260-
if (arg.startsWith('if=')) {
261-
readPaths.push(arg.substring(3));
262-
} else if (arg.startsWith('of=')) {
263-
writePaths.push(arg.substring(3));
264-
}
265-
}
266-
break;
267-
268-
case 'test':
269-
case '[':
270-
case '[[':
271-
// Test commands: look for file test operators
272-
// Common patterns: -f file, -d dir, -e path, file1 -nt file2, etc.
273-
for (let i = 0; i < args.length; i++) {
274-
const arg = args[i];
275-
if (arg === ']') continue; // Skip closing bracket
276-
277-
// File test operators that take a path argument
278-
if (['-f', '-d', '-e', '-r', '-w', '-x', '-s', '-L', '-h'].includes(arg)) {
279-
if (i + 1 < args.length) {
280-
readPaths.push(args[i + 1]);
281-
i++; // Skip the path
282-
}
283-
}
284-
// Binary operators with file paths on both sides
285-
else if (['-nt', '-ot', '-ef'].includes(arg)) {
286-
// Previous and next arguments are file paths
287-
if (i > 0 && args[i - 1] !== ']' && !args[i - 1].startsWith('-')) {
288-
// Previous arg already processed, just add next
289-
}
290-
if (i + 1 < args.length) {
291-
readPaths.push(args[i + 1]);
292-
i++; // Skip the path
293-
}
294-
}
295-
// Standalone file path (e.g., [ -f file ] or [ file ])
296-
else if (
297-
!arg.startsWith('-') &&
298-
(i === 0 || !['-eq', '-ne', '-lt', '-le', '-gt', '-ge', '=', '!=', '-z', '-n'].includes(args[i - 1]))
299-
) {
300-
readPaths.push(arg);
301-
}
302-
}
303-
break;
304-
305-
default:
306-
// Unknown command - don't extract any paths
307-
break;
308-
}
309-
310-
return { readPaths, writePaths };
311-
}
312-
313-
/**
314-
* Validate command for forbidden file access
315-
*/
316-
private validateCommand(command: string[]): { allowed: boolean; reason?: string } {
317-
if (command.length === 0) {
318-
return { allowed: true };
319-
}
320-
321-
const [cmd, ...args] = command;
322-
323-
// Commands that read files
324-
const readCommands = ['cat', 'head', 'tail', 'less', 'more', 'grep', 'find'];
325-
// Commands that write files
326-
const writeCommands = ['touch', 'echo', 'tee', 'dd'];
327-
// Commands that check file existence
328-
const testCommands = ['test', '[', '[['];
329-
330-
// Use command-aware argument parsing
331-
if (readCommands.includes(cmd) || writeCommands.includes(cmd) || testCommands.includes(cmd)) {
332-
const { readPaths, writePaths } = this.extractFilePaths(cmd, args);
333-
334-
// Validate read paths
335-
for (const path of readPaths) {
336-
if (!this.isReadAllowed(path)) {
337-
return { allowed: false, reason: `Read access denied to ${path}` };
338-
}
339-
}
340-
341-
// Validate write paths
342-
for (const path of writePaths) {
343-
if (!this.isWriteAllowed(path)) {
344-
return { allowed: false, reason: `Write access denied to ${path}` };
345-
}
346-
}
347-
}
348-
349-
// Check shell commands (sh -c "...")
350-
if (cmd === 'sh' && args.length >= 2 && args[0] === '-c') {
351-
const shellScript = args[1];
352-
353-
// Parse shell script for file operations
354-
// Look for output redirections (>, >>)
355-
const writeRedirects = shellScript.match(/>\s*([^\s;&|]+)/g);
356-
if (writeRedirects) {
357-
for (const match of writeRedirects) {
358-
const path = match.replace(/^>\s*/, '').replace(/^"([^"]+)"$/, '$1');
359-
if (!this.isWriteAllowed(path)) {
360-
return { allowed: false, reason: `Write access denied to ${path}` };
361-
}
362-
}
363-
}
364-
365-
// Look for file read operations (cat, head, tail, etc.)
366-
for (const readCmd of readCommands) {
367-
const pattern = new RegExp(`${readCmd}\\s+([^\\s;&|]+)`, 'g');
368-
const matches = shellScript.matchAll(pattern);
369-
for (const match of matches) {
370-
if (match[1]) {
371-
const path = match[1].replace(/^"([^"]+)"$/, '$1').replace(/^'([^']+)'$/, '$1');
372-
if (!path.startsWith('-') && !this.isReadAllowed(path)) {
373-
return { allowed: false, reason: `Read access denied to ${path}` };
374-
}
375-
}
376-
}
377-
}
378-
}
379-
380-
return { allowed: true };
381-
}
382-
383135
/**
384136
* Build resource-limited command wrapper
385137
*/

0 commit comments

Comments
 (0)