[codex] fix(security): bundle sandbox, Telegram, and update hardening#1416
Open
13ernkastel wants to merge 8 commits intoNVIDIA:mainfrom
Open
[codex] fix(security): bundle sandbox, Telegram, and update hardening#141613ernkastel wants to merge 8 commits intoNVIDIA:mainfrom
13ernkastel wants to merge 8 commits intoNVIDIA:mainfrom
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a new argv-style execution helper ( Changes
Sequence DiagramsequenceDiagram
participant Client
participant Onboard as bin/lib/onboard.js
participant Runner as bin/lib/runner.js
participant Child as childProcess.spawnSync
participant CredStore as Credential / Env
Client->>Onboard: start onboarding (may include sandboxNameOverride)
Onboard->>CredStore: get saved API key / read env
CredStore-->>Onboard: apiKey or null
Onboard->>Onboard: ensureValidatedBraveSearchCredential(nonInteractive?)
alt brave validation fails (non-interactive)
Onboard->>Client: throw error
else validation ok
Onboard->>Onboard: validateName(sandboxName, "sandbox name")
Onboard->>Runner: runFile("bash", [setup-dns-proxy.sh, GATEWAY, sandboxName], {ignoreError:true})
Runner->>Child: spawnSync("bash", args, opts)
Child-->>Runner: result
Runner-->>Onboard: result
Onboard->>Runner: runCaptureOpenshell([..., "sandbox","exec", sandboxName, "curl", "http://localhost:PORT/"])
Runner->>Child: spawnSync(...argv..., opts)
Child-->>Runner: result
Runner-->>Onboard: captured output => ready
Onboard->>Client: createSandbox complete (persist webSearchConfig if set)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
bin/lib/runner.js (1)
60-75: Collapse the spawn helpers into one internal path.
run(),runInteractive(), andrunFile()now repeat the samespawnSync/redaction/exit handling. A shared helper would make future hardening changes much harder to miss in one branch.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/lib/runner.js` around lines 60 - 75, The three functions run(), runInteractive(), and runFile() duplicate spawnSync/stdio/env/redaction/exit handling; extract that logic into a single internal helper (e.g., spawnAndHandle or _spawnSyncWithRedaction) that takes (fileOrCmd, args, opts, stdio) and performs spawnSync with cwd ROOT, merged env, calls writeRedactedResult(result, stdio), logs the redacted rendered command on non-zero exit and process.exit, and returns result; then refactor run(), runInteractive(), and runFile() to call this helper with the appropriate stdio and ignoreError behavior, removing the duplicated spawnSync and exit handling from each function.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bin/lib/onboard.js`:
- Around line 2095-2098: The prompt validation and the final boundary check are
out of sync: replace the `sandboxNameOverride || (await
promptValidatedSandboxName())` expression with `sandboxNameOverride ?? (await
promptValidatedSandboxName())` to prevent empty string falling through, and
modify `promptValidatedSandboxName()` to call and return `validateName(...)`
(instead of using the RFC-1123 regex directly) so the interactive retry loop
enforces the same 63-character/lowercase rules as `validateName`; ensure
`validateName` is used for both override and prompted values so failures
re-prompt rather than abort.
---
Nitpick comments:
In `@bin/lib/runner.js`:
- Around line 60-75: The three functions run(), runInteractive(), and runFile()
duplicate spawnSync/stdio/env/redaction/exit handling; extract that logic into a
single internal helper (e.g., spawnAndHandle or _spawnSyncWithRedaction) that
takes (fileOrCmd, args, opts, stdio) and performs spawnSync with cwd ROOT,
merged env, calls writeRedactedResult(result, stdio), logs the redacted rendered
command on non-zero exit and process.exit, and returns result; then refactor
run(), runInteractive(), and runFile() to call this helper with the appropriate
stdio and ignoreError behavior, removing the duplicated spawnSync and exit
handling from each function.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4277283d-a5ab-4474-9d2b-b9e92316bc0b
📒 Files selected for processing (5)
bin/lib/onboard.jsbin/lib/runner.jstest/onboard.test.jstest/runner.test.jstest/shellquote-sandbox.test.js
wscurran
added
security
Contributor
|
✨ Thanks for submitting this pull request, which proposes a way to improve security by hardening sandbox command execution in OpenShell. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bin/lib/onboard.js`:
- Around line 4049-4073: The current logic updates
onboardSession.webSearchConfig immediately after checking Brave Search
(ensureValidatedBraveSearchCredential / configureWebSearch), which can
desynchronize session state from an existing sandbox because
patchStagedDockerfile() only applies the brave preset during createSandbox();
defer mutating onboardSession.webSearchConfig until after the sandbox-reuse
decision (the code block that decides whether to recreate or reuse the sandbox),
so instead only compute a candidate webSearchConfig locally (using
ensureValidatedBraveSearchCredential and configureWebSearch) and only call
onboardSession.updateSession(...) to set webSearchConfig after the
reuse-vs-recreate branch completes and createSandbox() is invoked if needed.
- Around line 1056-1090: ensureValidatedBraveSearchCredential currently prompts
via promptBraveSearchApiKey()/promptBraveSearchRecovery() when there's no saved
key or validation fails; update it to fail fast in non-interactive runs by
detecting the non-interactive flag (either accept a new parameter like
nonInteractive or read the existing global/non-interactive indicator used by
onboard()) and when nonInteractive is true: if no saved credential from
getCredential(webSearch.BRAVE_API_KEY_ENV) or validateBraveSearchApiKey(apiKey)
fails, immediately throw a clear Error or return null (consistent with callers)
instead of calling promptBraveSearchApiKey()/promptBraveSearchRecovery(); keep
the existing saveCredential/process.env assignment path for interactive flows.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ecd12c58-cdeb-451f-820c-2ba6aa238d5f
📒 Files selected for processing (9)
bin/lib/onboard.jsbin/lib/registry.jsbin/lib/runner.jssrc/lib/onboard-session.test.tssrc/lib/onboard-session.tstest/onboard.test.jstest/registry.test.jstest/runner.test.jstest/shellquote-sandbox.test.js
✅ Files skipped from review due to trivial changes (2)
- src/lib/onboard-session.test.ts
- test/shellquote-sandbox.test.js
🚧 Files skipped from review as they are similar to previous changes (3)
- test/runner.test.js
- bin/lib/runner.js
- test/onboard.test.js
13ernkastel
force-pushed
the
codex/followup-shellquote-sandbox-hardening
branch
from
bb1cd5f to
f024982
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bin/lib/runner.js`:
- Around line 64-72: The runFile helper currently spreads opts into spawnSync
allowing a caller to re-enable shell execution; modify runFile (the function
named runFile and the spawnSync invocation) to explicitly forbid opts.shell by
checking if opts.shell is present and throwing or rejecting, and always pass
shell: false in the options passed to spawnSync (i.e., merge {...opts, shell:
false, stdio, cwd: ROOT, env: {...}}) so callers cannot enable shell parsing.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 06e0bccf-c6a0-4b8d-848d-a1dfbe93698b
📒 Files selected for processing (7)
bin/lib/onboard.jsbin/lib/runner.jssrc/lib/onboard-session.test.tstest/onboard.test.jstest/registry.test.jstest/runner.test.jstest/shellquote-sandbox.test.js
✅ Files skipped from review due to trivial changes (2)
- src/lib/onboard-session.test.ts
- test/shellquote-sandbox.test.js
🚧 Files skipped from review as they are similar to previous changes (2)
- test/registry.test.js
- test/runner.test.js
Contributor
Author
|
Addressed the remaining review feedback on top of the rebased branch. Latest follow-up commits:
Regression coverage added/updated for both areas. Checks run:
|
Rebase the sandbox command-hardening changes onto current main. Add argv-based sandbox command checks and align the onboarding harness with the hardened dashboard and DNS helper calls.
This was referenced Apr 5, 2026
13ernkastel
force-pushed
the
codex/followup-shellquote-sandbox-hardening
branch
from
ddd49d7 to
a804a2b
Compare
13ernkastel
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Bundles the remaining sandbox command-hardening work with the Telegram fail-closed cleanup and the unsupported self-update-hint fix.
This now includes the original
#1416scope plus the changes that had temporarily been split into#1500.#1499remains separate on purpose.Linked Issues
Related PRs / Issues
#1392#1218#1215#1500#1499separate#896Changes
createSandbox()boundary and removes the remaining shell-string dependency from follow-on sandbox command pathsrunFile()and uses argv-style execution forsetup-dns-proxy.shnemoclaw telegramsubcommands andnemoclaw start --discover-chat-idmainWhy
These changes all tighten the default security posture around operator-managed sandboxes:
Keeping them together in
#1416makes the remaining security review surface smaller while still leaving the separate immutable-hardening follow-up in#1499alone.Validation
npm run build:clinpx vitest run src/lib/deploy.test.ts src/lib/onboard-session.test.ts test/onboard.test.js test/cli.test.js test/runner.test.js test/service-env.test.js test/registry.test.js test/shellquote-sandbox.test.jsRisks / Notes
npm run typecheck:clistill hits the repo's existingsrc/lib/*.test.ts -> ../../dist/lib/*type-resolution issue in this environment, so validation here relies on the targeted build plus Vitest coverage above#1499remains separate on purpose