fix(sandboxes): prevent shell injection in setupOpenClaw#147
fix(sandboxes): prevent shell injection in setupOpenClaw#147Juanpaulo1128 wants to merge 1 commit intorecoupable:mainfrom
Conversation
accountId, RECOUP_API_KEY, and GITHUB_TOKEN were interpolated into a `node -e "..."` shell command. accountId is validated only as z.string().min(1), so an apostrophe in the value would break out of the JS string literal and execute attacker-controlled code in the sandbox. Pass the secrets as process env vars to a constant script body instead. No more interpolation; runCommand uses argv form, no shell layer.
📝 WalkthroughWalkthroughThe ChangesOpenClaw Environment Injection Security Improvement
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/sandboxes/setupOpenClaw.ts (1)
57-68: Removeas anycast and properly type therunCommandpayload.The
as anycast bypasses TypeScript's type checking for a security-critical call. All environment variable values here are strings (recoupApiKeyandaccountIdare strings,OPENCLAW_DEFAULT_MODELis a string constant, andgithubTokenis conditionally spread), so they matchRunCommandParams.env's expectedRecord<string, string>type. UseParameters<Sandbox["runCommand"]>[0]to extract the correct parameter type from the overloadedrunCommandsignature instead of the overly permissiveRecord<string, unknown>.Suggested fix
- const injectEnvOpts: Record<string, unknown> = { + const injectEnvOpts: Parameters<Sandbox["runCommand"]>[0] = { cmd: "node", args: ["-e", INJECT_ENV_SCRIPT], env: { RECOUP_API_KEY: recoupApiKey, RECOUP_ACCOUNT_ID: accountId, OPENCLAW_DEFAULT_MODEL, ...(githubToken ? { GITHUB_TOKEN: githubToken } : {}), }, }; - const injectEnv = await sandbox.runCommand(injectEnvOpts as any); + const injectEnv = await sandbox.runCommand(injectEnvOpts);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/sandboxes/setupOpenClaw.ts` around lines 57 - 68, The runCommand call currently uses an unsafe any cast; change injectEnvOpts to use the exact parameter type for sandbox.runCommand (e.g. let injectEnvOpts: Parameters<Sandbox["runCommand"]>[0]) and ensure the env property is typed as Record<string,string> with all values as strings (recoupApiKey, accountId, OPENCLAW_DEFAULT_MODEL and conditional GITHUB_TOKEN when githubToken is present) so you can remove the as any cast and call sandbox.runCommand(injectEnvOpts).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/sandboxes/setupOpenClaw.ts`:
- Around line 51-55: The current logger.log call in setupOpenClaw.ts leaks
sensitive fragments and raw account identifiers (logger.log(... {
RECOUP_API_KEY: `${recoupApiKey.slice(0, 4)}...`, RECOUP_ACCOUNT_ID: accountId,
... })); change it to emit only presence booleans or redacted flags: replace
RECOUP_API_KEY with something like hasRecoupApiKey: Boolean(recoupApiKey) (no
fragments), replace RECOUP_ACCOUNT_ID with hasRecoupAccountId:
Boolean(accountId) (do not log the raw ID), and keep GITHUB_TOKEN as the
existing presence check; update the logger.log invocation and any related
callers to use these presence-only fields.
- Around line 18-20: When populating c.env in setupOpenClaw (the c variable in
src/sandboxes/setupOpenClaw.ts), ensure you explicitly clear c.env.GITHUB_TOKEN
when process.env.GITHUB_TOKEN is not set; currently only the presence branch
sets the token so old values persist in the persisted openclaw.json—add logic to
set c.env.GITHUB_TOKEN = undefined (or delete c.env.GITHUB_TOKEN) in the else
path to remove any previously saved credential.
---
Nitpick comments:
In `@src/sandboxes/setupOpenClaw.ts`:
- Around line 57-68: The runCommand call currently uses an unsafe any cast;
change injectEnvOpts to use the exact parameter type for sandbox.runCommand
(e.g. let injectEnvOpts: Parameters<Sandbox["runCommand"]>[0]) and ensure the
env property is typed as Record<string,string> with all values as strings
(recoupApiKey, accountId, OPENCLAW_DEFAULT_MODEL and conditional GITHUB_TOKEN
when githubToken is present) so you can remove the as any cast and call
sandbox.runCommand(injectEnvOpts).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7c525641-c002-4a58-bfce-369b0035b142
📒 Files selected for processing (1)
src/sandboxes/setupOpenClaw.ts
| if (process.env.GITHUB_TOKEN) { | ||
| c.env.GITHUB_TOKEN = process.env.GITHUB_TOKEN; | ||
| } |
There was a problem hiding this comment.
Clear GITHUB_TOKEN when it is absent.
openclaw.json is persisted, so skipping this branch leaves any previous c.env.GITHUB_TOKEN in place. A later run without GITHUB_TOKEN will keep using the old credential.
Suggested fix
c.env.RECOUP_API_KEY = process.env.RECOUP_API_KEY;
c.env.RECOUP_ACCOUNT_ID = process.env.RECOUP_ACCOUNT_ID;
if (process.env.GITHUB_TOKEN) {
c.env.GITHUB_TOKEN = process.env.GITHUB_TOKEN;
+ } else {
+ delete c.env.GITHUB_TOKEN;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (process.env.GITHUB_TOKEN) { | |
| c.env.GITHUB_TOKEN = process.env.GITHUB_TOKEN; | |
| } | |
| if (process.env.GITHUB_TOKEN) { | |
| c.env.GITHUB_TOKEN = process.env.GITHUB_TOKEN; | |
| } else { | |
| delete c.env.GITHUB_TOKEN; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/sandboxes/setupOpenClaw.ts` around lines 18 - 20, When populating c.env
in setupOpenClaw (the c variable in src/sandboxes/setupOpenClaw.ts), ensure you
explicitly clear c.env.GITHUB_TOKEN when process.env.GITHUB_TOKEN is not set;
currently only the presence branch sets the token so old values persist in the
persisted openclaw.json—add logic to set c.env.GITHUB_TOKEN = undefined (or
delete c.env.GITHUB_TOKEN) in the else path to remove any previously saved
credential.
| logger.log("Injecting env vars into openclaw.json", { | ||
| RECOUP_API_KEY: `${process.env.RECOUP_API_KEY.slice(0, 4)}...`, | ||
| RECOUP_API_KEY: `${recoupApiKey.slice(0, 4)}...`, | ||
| RECOUP_ACCOUNT_ID: accountId, | ||
| GITHUB_TOKEN: githubToken ? "present" : "missing", | ||
| }); |
There was a problem hiding this comment.
Stop logging secret fragments and raw account IDs.
This security fix still emits a stable prefix of RECOUP_API_KEY and the raw tenant identifier into logs. Prefer presence-only fields here.
Suggested fix
logger.log("Injecting env vars into openclaw.json", {
- RECOUP_API_KEY: `${recoupApiKey.slice(0, 4)}...`,
- RECOUP_ACCOUNT_ID: accountId,
+ RECOUP_API_KEY: "present",
+ RECOUP_ACCOUNT_ID: "present",
GITHUB_TOKEN: githubToken ? "present" : "missing",
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| logger.log("Injecting env vars into openclaw.json", { | |
| RECOUP_API_KEY: `${process.env.RECOUP_API_KEY.slice(0, 4)}...`, | |
| RECOUP_API_KEY: `${recoupApiKey.slice(0, 4)}...`, | |
| RECOUP_ACCOUNT_ID: accountId, | |
| GITHUB_TOKEN: githubToken ? "present" : "missing", | |
| }); | |
| logger.log("Injecting env vars into openclaw.json", { | |
| RECOUP_API_KEY: "present", | |
| RECOUP_ACCOUNT_ID: "present", | |
| GITHUB_TOKEN: githubToken ? "present" : "missing", | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/sandboxes/setupOpenClaw.ts` around lines 51 - 55, The current logger.log
call in setupOpenClaw.ts leaks sensitive fragments and raw account identifiers
(logger.log(... { RECOUP_API_KEY: `${recoupApiKey.slice(0, 4)}...`,
RECOUP_ACCOUNT_ID: accountId, ... })); change it to emit only presence booleans
or redacted flags: replace RECOUP_API_KEY with something like hasRecoupApiKey:
Boolean(recoupApiKey) (no fragments), replace RECOUP_ACCOUNT_ID with
hasRecoupAccountId: Boolean(accountId) (do not log the raw ID), and keep
GITHUB_TOKEN as the existing presence check; update the logger.log invocation
and any related callers to use these presence-only fields.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
1 participant
Summary
setupOpenClawinterpolatedaccountId,RECOUP_API_KEY, andGITHUB_TOKENdirectly into anode -e "..."shell command.accountIdis validated only asz.string().min(1), so a value containing an apostrophe (e.g.o'malley) breaks out of the JS string literal and executes attacker-controlled code inside the Vercel Sandbox — with the API key and GitHub token already in scope.Severity: Critical (shell + JS injection on user-controlled input, secrets in scope)
Affected:
src/sandboxes/setupOpenClaw.tsonlyReachable from: any task that calls
setupOpenClaw(sandbox, accountId)—setupSandboxTask,runSandboxCommandTask, etc.Root cause