Skip to content

fix(sandbox): honor explicit docker env#82763

Merged
joshavant merged 2 commits into
mainfrom
fix/sandbox-explicit-docker-env
May 16, 2026
Merged

fix(sandbox): honor explicit docker env#82763
joshavant merged 2 commits into
mainfrom
fix/sandbox-explicit-docker-env

Conversation

@joshavant
Copy link
Copy Markdown
Contributor

Summary

  • Treat agents.defaults.sandbox.docker.env as explicit Docker sandbox configuration so credential-looking configured keys are passed to container creation.
  • Keep inherited host/process env sanitization unchanged, preserve the public sandbox create option for SDK compatibility, and add env-policy hash invalidation for regular and browser sandbox containers.
  • Document Docker metadata exposure for configured sandbox env values.

Verification

  • node scripts/run-vitest.mjs src/agents/sandbox-create-args.test.ts src/agents/sandbox/sanitize-env-vars.test.ts src/agents/sandbox/docker.config-hash-recreate.test.ts src/agents/sandbox/config-hash.test.ts src/agents/sandbox/browser.create.test.ts
  • git diff --check
  • pnpm format:docs:check
  • codex review --uncommitted

Behavior addressed: Explicit configured Docker sandbox env vars with secret-looking names are no longer filtered out during container creation.
Real environment tested: Direct AWS Crabbox and Blacksmith Testbox-through-Crabbox with real Docker sandbox containers and dummy env values only.
Exact steps or command run after this patch: Testbox targeted-synced this branch patch, ran focused sandbox Vitest files, pulled debian:bookworm-slim, created a Docker sandbox via ensureSandboxContainer, inspected .Config.Env, and removed the container.
Evidence after fix: AWS Crabbox cbx_469a6efa4d95 printed ISSUE_82695_FIXED=1; Blacksmith Testbox tbx_01krse65vpk7hzr95e3ej34r9j printed ISSUE_82695_FIXED_TESTBOX=1.
Observed result after fix: All configured keys from the issue repro were present, missingConfigured was [], and OPENCLAW_CLI=1 was present.
What was not tested: Real third-party service credentials were not used; all values were dummy strings.

Fixes #82695

@joshavant joshavant requested a review from a team as a code owner May 16, 2026 22:26
@openclaw-barnacle openclaw-barnacle Bot added docs Improvements or additions to documentation gateway Gateway runtime docker Docker and sandbox tooling agents Agent runtime and tooling size: M maintainer Maintainer-authored PR labels May 16, 2026
@clawsweeper
Copy link
Copy Markdown
Contributor

clawsweeper Bot commented May 16, 2026

Codex review: needs maintainer review before merge.

Summary
The branch treats configured Docker sandbox env as explicit container env, adds hash invalidation and focused tests for regular and browser sandboxes, and documents Docker metadata exposure.

Reproducibility: yes. The linked issue gives concrete Docker steps, and current source shows configured Docker env is filtered by sensitive-name patterns before Docker --env emission; I did not run a live container in this read-only review.

Real behavior proof
Sufficient (live_output): The PR body includes after-fix real Docker container proof from AWS Crabbox and Blacksmith Testbox using dummy env values, with all configured keys present and missingConfigured as [].

Next step before merge
No automated repair is needed; this maintainer-labeled, policy-sensitive sandbox credentials change should wait for maintainer review and remaining CI.

Security
Cleared: Security-sensitive behavior was reviewed; the patch changes only explicit sandbox config env, leaves inherited host env sanitization intact, and documents Docker metadata exposure.

Review details

Best possible solution:

Land this PR after maintainer policy signoff and green CI if the intended contract is that explicit Docker sandbox env is persisted as container Config.Env with clear metadata-exposure docs.

Do we have a high-confidence way to reproduce the issue?

Yes. The linked issue gives concrete Docker steps, and current source shows configured Docker env is filtered by sensitive-name patterns before Docker --env emission; I did not run a live container in this read-only review.

Is this the best way to solve the issue?

Yes. The PR makes the smallest maintainable split I see: explicit sandbox config env bypasses inherited-host sensitive-name filtering, inherited/process env sanitization remains in place, and affected containers are invalidated by a targeted hash epoch.

Acceptance criteria:

  • Wait for remaining check runs on 4c3c90ec9dde064fff7252fd2175e1f907769178 to complete.
  • Review the PR body's Crabbox/Testbox Docker proof and confirm the Docker metadata exposure contract is acceptable.

What I checked:

  • Current main drops the reported configured keys: Current main sends params.cfg.env through sanitizeEnvVars before emitting Docker --env flags, so configured secret-looking names are filtered before container creation. (src/agents/sandbox/docker.ts:428, 36e88f5dddd3)
  • Sensitive-name filter matches the issue repro: The sanitizer blocks GEMINI_API_KEY directly and broadly blocks names ending in API_KEY, TOKEN, PASSWORD, PRIVATE_KEY, or SECRET, matching the omitted keys in Sandbox Docker env only partially injects configured keys into fresh containers #82695. (src/agents/sandbox/sanitize-env-vars.ts:1, 36e88f5dddd3)
  • Docs define configured Docker env as the supported sandbox path: The skills docs state sandboxed skill processes do not inherit host process.env and direct users to agents.defaults.sandbox.docker.env or per-agent Docker env for sandboxed skill variables. Public docs: docs/tools/skills-config.md. (docs/tools/skills-config.md:165, 36e88f5dddd3)
  • PR changes the implicated boundary: The patch replaces Docker create-time env handling with sanitizeExplicitSandboxEnvVars, adds resolveDockerEnvPolicyEpoch, and includes the epoch in regular and browser sandbox config hashes so old filtered containers are recreated when needed. (src/agents/sandbox/docker.ts:461, 4af2ab80a2a1)
  • PR includes after-fix real Docker proof: The PR body reports AWS Crabbox cbx_469a6efa4d95 and Blacksmith Testbox tbx_01krse65vpk7hzr95e3ej34r9j with real Docker sandbox containers, dummy env values, missingConfigured as [], and OPENCLAW_CLI=1 present. (4c3c90ec9dde)
  • Review and CI state: Live PR metadata shows the PR is open, mergeable, maintainer-labeled, and at head 4c3c90ec...; check-runs summary showed 113 checks with two still in progress and no failed checks in the first-page summary I queried. (4c3c90ec9dde)

Likely related people:

  • Vincent Koc: Current blame for the Docker create env handling, sanitizer patterns, and nearby sandbox docs points to commit 7d09ff89ee3a.... (role: recent area contributor; confidence: high; commits: 7d09ff89ee3a; files: src/agents/sandbox/docker.ts, src/agents/sandbox/sanitize-env-vars.ts, docs/tools/skills-config.md)
  • Peter Steinberger: The related issue review identified prior sandbox env sanitization and Docker-create work by this contributor in commits near the affected path; local history is shallow, so this is based on the related-item evidence rather than fresh full-history blame. (role: adjacent owner; confidence: medium; commits: 575936473d13, 5487c9adebb0, 638853c6d260; files: src/agents/sandbox/docker.ts, src/agents/sandbox/sanitize-env-vars.ts)
  • George Pickett: The related issue review traces earlier work that passed sandbox.docker.env into containers to this contributor, which is the behavior now being repaired at the create boundary. (role: introduced behavior; confidence: low; commits: a067565db577; files: src/agents/sandbox/docker.ts, src/agents/sandbox-create-args.test.ts)

Remaining risk / open question:

  • Two head check runs were still in progress when queried, so this should not be treated as ready to land until CI finishes.
  • The change intentionally persists explicitly configured secret-looking values in Docker container metadata; the diff documents that exposure, but maintainer policy signoff is still appropriate.

Codex review notes: model gpt-5.5, reasoning high; reviewed against 81a578fd6bff.

@clawsweeper clawsweeper Bot added proof: sufficient ClawSweeper judged the real behavior proof convincing. P1 High-priority user-facing bug, regression, or broken workflow. labels May 16, 2026
@joshavant joshavant merged commit 045a581 into main May 16, 2026
115 of 119 checks passed
@joshavant joshavant deleted the fix/sandbox-explicit-docker-env branch May 16, 2026 22:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agents Agent runtime and tooling docker Docker and sandbox tooling docs Improvements or additions to documentation gateway Gateway runtime maintainer Maintainer-authored PR P1 High-priority user-facing bug, regression, or broken workflow. proof: sufficient ClawSweeper judged the real behavior proof convincing. size: M

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Sandbox Docker env only partially injects configured keys into fresh containers

1 participant