Skip to content

Commit 60be10b

Browse files
soodokuclaude
andauthored
Claude/fix argument parsing 01 dw1t4 n33m s5g13c sr8u y bw (#10)
* WIP: Improve argument parsing with command-aware extraction Implements command-specific parsing to correctly distinguish file paths from other arguments (e.g., grep patterns, echo text). - Added extractFilePaths() method with per-command parsing logic - Handles grep (pattern vs files), find (directory vs predicates), dd (if=/of= syntax), test commands (operators), etc. - Fixes issue where any non-flag arg was incorrectly treated as file path This is work in progress - discussing with user whether command-based filtering should be removed entirely vs improved. * 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. --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 576bedd commit 60be10b

2 files changed

Lines changed: 20 additions & 93 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 & 92 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,83 +132,6 @@ export class FilesystemSandbox {
139132
});
140133
}
141134

142-
/**
143-
* Validate command for forbidden file access
144-
*/
145-
private validateCommand(command: string[]): { allowed: boolean; reason?: string } {
146-
if (command.length === 0) {
147-
return { allowed: true };
148-
}
149-
150-
const [cmd, ...args] = command;
151-
152-
// Commands that read files
153-
const readCommands = ['cat', 'head', 'tail', 'less', 'more', 'grep', 'find'];
154-
// Commands that write files
155-
const writeCommands = ['touch', 'echo', 'tee', 'dd'];
156-
// Commands that check file existence
157-
const testCommands = ['test', '[', '[['];
158-
159-
// Check direct file access commands
160-
if (readCommands.includes(cmd)) {
161-
for (const arg of args) {
162-
if (!arg.startsWith('-') && !this.isReadAllowed(arg)) {
163-
return { allowed: false, reason: `Read access denied to ${arg}` };
164-
}
165-
}
166-
}
167-
168-
if (writeCommands.includes(cmd)) {
169-
for (const arg of args) {
170-
if (!arg.startsWith('-') && !this.isWriteAllowed(arg)) {
171-
return { allowed: false, reason: `Write access denied to ${arg}` };
172-
}
173-
}
174-
}
175-
176-
if (testCommands.includes(cmd)) {
177-
// Test commands check file existence - treat as read
178-
for (const arg of args) {
179-
if (!arg.startsWith('-') && arg !== ']' && !this.isReadAllowed(arg)) {
180-
return { allowed: false, reason: `Access denied to ${arg}` };
181-
}
182-
}
183-
}
184-
185-
// Check shell commands (sh -c "...")
186-
if (cmd === 'sh' && args.length >= 2 && args[0] === '-c') {
187-
const shellScript = args[1];
188-
189-
// Parse shell script for file operations
190-
// Look for output redirections (>, >>)
191-
const writeRedirects = shellScript.match(/>\s*([^\s;&|]+)/g);
192-
if (writeRedirects) {
193-
for (const match of writeRedirects) {
194-
const path = match.replace(/^>\s*/, '').replace(/^"([^"]+)"$/, '$1');
195-
if (!this.isWriteAllowed(path)) {
196-
return { allowed: false, reason: `Write access denied to ${path}` };
197-
}
198-
}
199-
}
200-
201-
// Look for file read operations (cat, head, tail, etc.)
202-
for (const readCmd of readCommands) {
203-
const pattern = new RegExp(`${readCmd}\\s+([^\\s;&|]+)`, 'g');
204-
const matches = shellScript.matchAll(pattern);
205-
for (const match of matches) {
206-
if (match[1]) {
207-
const path = match[1].replace(/^"([^"]+)"$/, '$1').replace(/^'([^']+)'$/, '$1');
208-
if (!path.startsWith('-') && !this.isReadAllowed(path)) {
209-
return { allowed: false, reason: `Read access denied to ${path}` };
210-
}
211-
}
212-
}
213-
}
214-
}
215-
216-
return { allowed: true };
217-
}
218-
219135
/**
220136
* Build resource-limited command wrapper
221137
*/

0 commit comments

Comments
 (0)