add environment variables and working directory support to command exec #204
Conversation
🦋 Changeset detectedLatest commit: 510fe63 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
commit: |
🐳 Docker Image PublishedFROM cloudflare/sandbox:0.0.0-pr-204-84e692eVersion: You can use this Docker image with the preview package from this PR. |
|
(Sharing some thoughts here about Overall, I quite like the features this PR is adding and think this is an important one to clean up and get right. It's been fragile for a long time and I'm happy we're fixing this! So with env, I generally want us to make sure there are 4 layers of env setting and all of them work cohesively:
And env is mainly critical for all the flows that involve the default session and the ones created explicitly using cwd is much easier to reason with as an option, and is definitely quite useful when working in the scope of either the default session or the manually created session, but sometimes developers just want to run a command elsewhere and don't want to do |
ghostwriternr
left a comment
ghostwriternr
left a comment
There was a problem hiding this comment.
Besides these (commandWithEnv is the only blocking one though), I think we could do a couple of things more:
- Add
setEnvVarsto theISandboxtype. I just realised this is not there currently, but should be. - In
createSession, IF you think my earlier comment aboutsetEnvVarsbehaviour on the global sandbox object vs the session object makes sense, we can mergethis.envVarswith the session-specific ones before creating a session from the session object.
Updates documentation for exec(), execStream(), and startProcess() methods to reflect new environment variable and working directory support added in cloudflare/sandbox-sdk#204. Changes: - Add env and cwd parameters to exec() options - Add env and cwd parameters to execStream() options - Expand startProcess() documentation with all available options - Add examples showing usage of new parameters 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Add support for environment variables and working directory in command execution.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Document new per-command environment variable and working directory options added to exec(), execStream(), and startProcess() methods. Changes: - Add env and cwd parameters to exec() and execStream() API docs - Add practical examples showing per-command env variable scoping - Add working directory override examples - Update environment variables configuration guide - Clarify that per-command options do not persist in session Related PR: cloudflare/sandbox-sdk#204 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
There was a problem hiding this comment.
Claude Code Review
The shell injection via env var names is fixed (line 667-669). Value escaping looks secure.
However, I found a critical bug in the variable restoration logic at line 684:
cleanupLines.push(` eval "export ${key}=$${prevVar}"`);In bash, $$ is the process ID, not variable expansion. This line generates:
eval "export KEY=12345{prevVar}" # Where 12345 is the PIDShould be:
cleanupLines.push(` eval "export ${key}=${prevVar}"`);Since prevVar already contains the shell-quoted value from printf '%q' on line 679, no extra escaping is needed.
Impact: Environment variable restoration will fail. The test at session.test.ts:192-214 should catch this, but I couldn't verify if tests actually pass (build system issues locally).
Additional Issues:
- SDK tests (sandbox.test.ts:155-170) verify options are forwarded but don't test actual env/cwd behavior
- Minor: Inconsistent use of
env &&vsenv !== undefined(lines 661, 695) - works correctly but style inconsistency
Needs fixes before merge.
| setupLines.push(` export ${key}='${escapedValue}'`); | ||
|
|
||
| cleanupLines.push(` if [ "$${hasVar}" = "1" ]; then`); | ||
| cleanupLines.push(` eval "export ${key}=$${prevVar}"`); |
There was a problem hiding this comment.
CRITICAL BUG: $$ in bash is the process ID, not variable expansion.
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 printf '%q' which already shell-quotes the value, no extra escaping is needed.
| expect(result.stdout.trim()).toBe("it's got 'quotes'"); | ||
| }); | ||
|
|
||
| it('should restore existing env vars with special characters', async () => { |
There was a problem hiding this comment.
This test verifies restoration of env vars with special chars. It should fail with the current bug at session.ts:684 where $$ expands to PID instead of the variable.
Can you confirm this test actually passes in CI? If it does pass, there may be a subtle bash behavior I'm missing.
2 participants
No description provided.