-
Notifications
You must be signed in to change notification settings - Fork 38
add environment variables and working directory support to command exec #204
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
base: main
Are you sure you want to change the base?
Changes from all commits
a7bae24
00ff59e
9ef41fe
67ebcab
4238cb7
d69e436
a4ea45b
39cf60b
a6f3209
abbb218
32f744b
d9b18cb
cad3f21
c0b8622
510fe63
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,5 @@ | ||
| --- | ||
| '@cloudflare/sandbox': patch | ||
| --- | ||
|
|
||
| add environment variables and working directory support to command exec |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -88,6 +88,8 @@ export interface RawExecResult { | |
| interface ExecOptions { | ||
| /** Override working directory for this command only */ | ||
| cwd?: string; | ||
| /** Environment variables for this command only (does not persist in session) */ | ||
| env?: Record<string, string>; | ||
| } | ||
|
|
||
| /** Command handle for tracking and killing running commands */ | ||
|
|
@@ -233,7 +235,8 @@ export class Session { | |
| logFile, | ||
| exitCodeFile, | ||
| options?.cwd, | ||
| false | ||
| false, | ||
| options?.env | ||
| ); | ||
|
|
||
| // Write script to shell's stdin | ||
|
|
@@ -333,7 +336,8 @@ export class Session { | |
| logFile, | ||
| exitCodeFile, | ||
| options?.cwd, | ||
| true | ||
| true, | ||
| options?.env | ||
| ); | ||
|
|
||
| if (this.shell!.stdin && typeof this.shell!.stdin !== 'number') { | ||
|
|
@@ -624,7 +628,8 @@ export class Session { | |
| logFile: string, | ||
| exitCodeFile: string, | ||
| cwd?: string, | ||
| isBackground = false | ||
| isBackground = false, | ||
| env?: Record<string, string> | ||
| ): string { | ||
| // Create unique FIFO names to prevent collisions | ||
| const stdoutPipe = join(this.sessionDir!, `${cmdId}.stdout.pipe`); | ||
|
|
@@ -639,6 +644,77 @@ export class Session { | |
| const safeSessionDir = this.escapeShellPath(this.sessionDir!); | ||
| const safePidFile = this.escapeShellPath(pidFile); | ||
|
|
||
| const indentLines = (input: string, spaces: number) => { | ||
| const prefix = ' '.repeat(spaces); | ||
| return input | ||
| .split('\n') | ||
| .map((line) => (line.length > 0 ? `${prefix}${line}` : '')) | ||
| .join('\n'); | ||
| }; | ||
|
|
||
| const sanitizeIdentifier = (value: string) => | ||
| value.replace(/[^A-Za-z0-9_]/g, '_'); | ||
|
|
||
| let envSetupBlock = ''; | ||
| let envCleanupBlock = ''; | ||
|
|
||
| if (env && Object.keys(env).length > 0) { | ||
claude[bot] marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| const setupLines: string[] = []; | ||
| const cleanupLines: string[] = []; | ||
| const cmdSuffix = sanitizeIdentifier(cmdId); | ||
|
|
||
| Object.entries(env).forEach(([key, value], index) => { | ||
| if (!/^[A-Za-z_][A-Za-z0-9_]*$/.test(key)) { | ||
| throw new Error(`Invalid environment variable name: ${key}`); | ||
| } | ||
|
|
||
| const escapedValue = value.replace(/'/g, "'\\''"); | ||
| const stateSuffix = `${cmdSuffix}_${index}`; | ||
| const hasVar = `__SANDBOX_HAS_${stateSuffix}`; | ||
| const prevVar = `__SANDBOX_PREV_${stateSuffix}`; | ||
|
|
||
| setupLines.push(` ${hasVar}=0`); | ||
| setupLines.push(` if [ "\${${key}+x}" = "x" ]; then`); | ||
| setupLines.push(` ${hasVar}=1`); | ||
| setupLines.push(` ${prevVar}=$(printf '%q' "\${${key}}")`); | ||
| setupLines.push(' fi'); | ||
| setupLines.push(` export ${key}='${escapedValue}'`); | ||
|
|
||
| cleanupLines.push(` if [ "$${hasVar}" = "1" ]; then`); | ||
| cleanupLines.push(` eval "export ${key}=$${prevVar}"`); | ||
|
Contributor
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. CRITICAL BUG: This generates: eval "export KEY=12345__SANDBOX_PREV_suffix_0"Where 12345 is the PID. Fix: cleanupLines.push(` eval "export ${key}=${prevVar}"`);Since line 679 uses |
||
| cleanupLines.push(' else'); | ||
| cleanupLines.push(` unset ${key}`); | ||
| cleanupLines.push(' fi'); | ||
| cleanupLines.push(` unset ${hasVar} ${prevVar}`); | ||
| }); | ||
|
|
||
| envSetupBlock = setupLines.join('\n'); | ||
| envCleanupBlock = cleanupLines.join('\n'); | ||
| } | ||
|
|
||
| const hasScopedEnv = env && Object.keys(env).length > 0; | ||
|
|
||
| const buildCommandBlock = (exitVar: string): string => { | ||
| const lines: string[] = []; | ||
| if (hasScopedEnv && envSetupBlock) { | ||
| lines.push(envSetupBlock); | ||
| } | ||
| lines.push(` ${command}`); | ||
| lines.push(` ${exitVar}=$?`); | ||
| if (hasScopedEnv && envCleanupBlock) { | ||
| lines.push(envCleanupBlock); | ||
| } | ||
| return lines.join('\n'); | ||
| }; | ||
|
|
||
| const buildCommandSection = (exitVar: string, indent: number): string => { | ||
| if (hasScopedEnv) { | ||
| return `${indentLines(buildCommandBlock(exitVar), indent)}\n`; | ||
| } | ||
| const padding = ' '.repeat(indent); | ||
| return `${padding}${command}\n${padding}${exitVar}=$?\n`; | ||
| }; | ||
|
|
||
| // Build the FIFO script | ||
| // For background: monitor handles cleanup (no trap needed) | ||
| // For foreground: trap handles cleanup (standard pattern) | ||
|
|
@@ -684,8 +760,7 @@ export class Session { | |
| script += ` if cd ${safeCwd}; then\n`; | ||
| script += ` # Execute command in BACKGROUND (runs in subshell, enables concurrency)\n`; | ||
| script += ` {\n`; | ||
| script += ` ${command}\n`; | ||
| script += ` CMD_EXIT=$?\n`; | ||
| script += `${indentLines(buildCommandBlock('CMD_EXIT'), 6)}\n`; | ||
| script += ` # Write exit code\n`; | ||
| script += ` echo "$CMD_EXIT" > ${safeExitCodeFile}.tmp\n`; | ||
| script += ` mv ${safeExitCodeFile}.tmp ${safeExitCodeFile}\n`; | ||
|
|
@@ -708,8 +783,7 @@ export class Session { | |
| } else { | ||
| script += ` # Execute command in BACKGROUND (runs in subshell, enables concurrency)\n`; | ||
| script += ` {\n`; | ||
| script += ` ${command}\n`; | ||
| script += ` CMD_EXIT=$?\n`; | ||
| script += `${indentLines(buildCommandBlock('CMD_EXIT'), 4)}\n`; | ||
| script += ` # Write exit code\n`; | ||
| script += ` echo "$CMD_EXIT" > ${safeExitCodeFile}.tmp\n`; | ||
| script += ` mv ${safeExitCodeFile}.tmp ${safeExitCodeFile}\n`; | ||
|
|
@@ -738,8 +812,9 @@ export class Session { | |
| script += ` PREV_DIR=$(pwd)\n`; | ||
| script += ` if cd ${safeCwd}; then\n`; | ||
| script += ` # Execute command, redirect to temp files\n`; | ||
| script += ` { ${command}; } < /dev/null > "$log.stdout" 2> "$log.stderr"\n`; | ||
| script += ` EXIT_CODE=$?\n`; | ||
| script += ` {\n`; | ||
| script += `${indentLines(buildCommandBlock('EXIT_CODE'), 6)}\n`; | ||
| script += ` } < /dev/null > "$log.stdout" 2> "$log.stderr"\n`; | ||
| script += ` # Restore directory\n`; | ||
| script += ` cd "$PREV_DIR"\n`; | ||
| script += ` else\n`; | ||
|
|
@@ -748,8 +823,9 @@ export class Session { | |
| script += ` fi\n`; | ||
| } else { | ||
| script += ` # Execute command, redirect to temp files\n`; | ||
| script += ` { ${command}; } < /dev/null > "$log.stdout" 2> "$log.stderr"\n`; | ||
| script += ` EXIT_CODE=$?\n`; | ||
| script += ` {\n`; | ||
| script += `${indentLines(buildCommandBlock('EXIT_CODE'), 4)}\n`; | ||
| script += ` } < /dev/null > "$log.stdout" 2> "$log.stderr"\n`; | ||
| } | ||
|
|
||
| script += ` \n`; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -151,6 +151,82 @@ describe('Session', () => { | |
| expect(result2.stdout.trim()).toContain('subdir'); | ||
| }); | ||
|
|
||
| it('should scope per-command environment variables', async () => { | ||
|
Contributor
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. Test gap: This verifies NEW variables disappear, but doesn't test the critical case of overriding EXISTING variables and restoring original values. Add: it('should restore overridden environment variables', async () => {
await session.exec('export EXISTING="original"');
await session.exec('echo $EXISTING', { env: { EXISTING: 'temp' }});
const result = await session.exec('echo $EXISTING');
expect(result.stdout.trim()).toBe('original');
});This test will expose the bug at line 684. |
||
| const result = await session.exec('printenv TEMP_CMD_VAR', { | ||
| env: { TEMP_CMD_VAR: 'scoped-value' } | ||
| }); | ||
|
|
||
| expect(result.exitCode).toBe(0); | ||
| expect(result.stdout.trim()).toBe('scoped-value'); | ||
|
|
||
| const verify = await session.exec('printenv TEMP_CMD_VAR'); | ||
| expect(verify.exitCode).not.toBe(0); | ||
| }); | ||
|
|
||
| it('should reject invalid per-command environment variable names', async () => { | ||
| await expect( | ||
| session.exec('pwd', { | ||
| env: { 'INVALID-NAME': 'value' } | ||
| }) | ||
| ).rejects.toThrow(/Invalid environment variable name/); | ||
| }); | ||
|
Contributor
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. Add tests for env var values containing shell metacharacters: it('should safely handle env values with shell special chars', async () => {
const result = await session.exec('echo "$SPECIAL"', {
env: { SPECIAL: '$(whoami) `date` $PATH' }
});
expect(result.stdout.trim()).toBe('$(whoami) `date` $PATH'); // Literal
});
it('should handle env values with quotes', async () => {
const result = await session.exec('echo "$QUOTED"', {
env: { QUOTED: "it's got 'quotes'" }
});
expect(result.stdout.trim()).toBe("it's got 'quotes'");
});This ensures the escaping at line 671 works correctly for all edge cases. |
||
|
|
||
| it('should safely handle env values with shell special chars', async () => { | ||
| const result = await session.exec('echo "$SPECIAL"', { | ||
| env: { SPECIAL: '$(whoami) `date` $PATH' } | ||
| }); | ||
|
|
||
| expect(result.exitCode).toBe(0); | ||
| expect(result.stdout.trim()).toBe('$(whoami) `date` $PATH'); | ||
| }); | ||
|
|
||
| it('should handle env values with quotes', async () => { | ||
| const result = await session.exec('echo "$QUOTED"', { | ||
| env: { QUOTED: "it's got 'quotes'" } | ||
| }); | ||
|
|
||
| expect(result.exitCode).toBe(0); | ||
| expect(result.stdout.trim()).toBe("it's got 'quotes'"); | ||
| }); | ||
|
|
||
| it('should restore existing env vars with special characters', async () => { | ||
|
Contributor
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. This test verifies restoration of env vars with special chars. It should fail with the current bug at session.ts:684 where Can you confirm this test actually passes in CI? If it does pass, there may be a subtle bash behavior I'm missing. |
||
| await session.destroy(); | ||
| session = new Session({ | ||
| id: 'test-exec', | ||
| cwd: testDir, | ||
| env: { RESTORE_VAR: '$(whoami) $PATH' } | ||
| }); | ||
| await session.initialize(); | ||
|
|
||
| const initial = await session.exec('echo "$RESTORE_VAR"'); | ||
| expect(initial.exitCode).toBe(0); | ||
| expect(initial.stdout.trim()).toBe('$(whoami) $PATH'); | ||
|
|
||
| const overrideResult = await session.exec('echo "$RESTORE_VAR"', { | ||
| env: { RESTORE_VAR: 'temporary-value' } | ||
| }); | ||
| expect(overrideResult.exitCode).toBe(0); | ||
| expect(overrideResult.stdout.trim()).toBe('temporary-value'); | ||
|
|
||
| const restoredResult = await session.exec('echo "$RESTORE_VAR"'); | ||
| expect(restoredResult.exitCode).toBe(0); | ||
| expect(restoredResult.stdout.trim()).toBe('$(whoami) $PATH'); | ||
| }); | ||
|
|
||
| it('should restore overridden environment variables', async () => { | ||
| await session.exec('export EXISTING="original"'); | ||
|
|
||
| const overrideResult = await session.exec('echo "$EXISTING"', { | ||
| env: { EXISTING: 'temp' } | ||
| }); | ||
| expect(overrideResult.exitCode).toBe(0); | ||
| expect(overrideResult.stdout.trim()).toBe('temp'); | ||
|
|
||
| const restoredResult = await session.exec('echo "$EXISTING"'); | ||
| expect(restoredResult.exitCode).toBe(0); | ||
| expect(restoredResult.stdout.trim()).toBe('original'); | ||
| }); | ||
|
|
||
| it('should override cwd temporarily when option provided', async () => { | ||
| // Create a subdirectory | ||
| await session.exec('mkdir -p tempdir'); | ||
|
|
||
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.
Collision risk:
sanitizeIdentifier('abc-123')andsanitizeIdentifier('abc_123')both return'abc_123'. If two commands with similar IDs run concurrently (background mode), their state variables will collide.Since cmdId values are UUIDs (already shell-safe), consider using them directly without sanitization, or add a hash suffix to guarantee uniqueness.