feat(security): remove web search config from docker build - #1835
Merged
Conversation
Migrate Brave API key to OpenShell generic provider system to inject tokens at proxy egress, eliminating plaintext credential leaks in build logs and 'openclaw.json' baked configurations.
Also fix onboard.test.ts signature mismatch.
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:
📝 WalkthroughWalkthroughRemoved build-time base64 web config and added Changes
Sequence Diagram(s)sequenceDiagram
participant Builder as Builder (Dockerfile)
participant Image as Image (openclaw.json)
participant Onboard as Onboard Service
participant Cred as Credential Source
participant Sandbox as Sandbox (runtime)
Builder->>Image: Build with ARG NEMOCLAW_WEB_SEARCH_ENABLED
alt NEMOCLAW_WEB_SEARCH_ENABLED == "1"
Image->>Image: emit tools.web with Brave provider\napiKey resolver: openshell:resolve:env:BRAVE_API_KEY\nfetch.enabled: true
else
Image->>Image: omit tools.web block
end
Onboard->>Cred: request BRAVE_API_KEY (getCredential / env)
Cred-->>Onboard: return key or empty
alt key non-empty and fetch enabled
Onboard->>Sandbox: create sandbox with BRAVE_API_KEY env
else
Onboard->>Sandbox: create sandbox without BRAVE_API_KEY
end
Sandbox-->>Onboard: sandbox ready
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 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 |
ksapru
marked this pull request as ready for review
April 13, 2026 13:36
wscurran
added
security
dependencies
Contributor
|
✨ Thanks for submitting this PR, which proposes a fix for a security vulnerability by removing web search configuration from the Docker build process. Related issue: #1741 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Dockerfile (1)
160-171:⚠️ Potential issue | 🟠 MajorPreserve web-search gating to avoid config/runtime drift.
Line 166 and Line 168 hard-enable
tools.web.searchandtools.web.fetch, butsrc/lib/onboard.ts:2680-2701only injectsBRAVE_API_KEYwhenwebSearchConfig?.fetchEnabledis true. This makesfetchEnabled=falseineffective and can produce always-on web config with missing credentials. Keep the secret handling change, but restore a non-secret enablement gate (or remove the toggle everywhere and align behavior explicitly).Proposed fix (non-secret flag, secrets still runtime-only)
@@ ARG NEMOCLAW_PROXY_HOST=10.200.0.1 ARG NEMOCLAW_PROXY_PORT=3128 +ARG NEMOCLAW_WEB_FETCH_ENABLED=0 @@ ENV NEMOCLAW_MODEL=${NEMOCLAW_MODEL} \ @@ NEMOCLAW_PROXY_HOST=${NEMOCLAW_PROXY_HOST} \ - NEMOCLAW_PROXY_PORT=${NEMOCLAW_PROXY_PORT} + NEMOCLAW_PROXY_PORT=${NEMOCLAW_PROXY_PORT} \ + NEMOCLAW_WEB_FETCH_ENABLED=${NEMOCLAW_WEB_FETCH_ENABLED} @@ -config.update({ \ - 'tools': { \ - 'web': { \ - 'search': { \ - 'enabled': True, \ - 'provider': 'brave', \ - 'apiKey': 'openshell:resolve:env:BRAVE_API_KEY' \ - }, \ - 'fetch': {'enabled': True} \ - } \ - } \ -}); \ +web_fetch_enabled = os.environ.get('NEMOCLAW_WEB_FETCH_ENABLED', '') == '1'; \ +if web_fetch_enabled: \ + config.update({ \ + 'tools': { \ + 'web': { \ + 'search': { \ + 'enabled': True, \ + 'provider': 'brave', \ + 'apiKey': 'openshell:resolve:env:BRAVE_API_KEY' \ + }, \ + 'fetch': {'enabled': True} \ + } \ + } \ + }); \🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Dockerfile` around lines 160 - 171, The config block currently hard-sets tools.web.search.enabled and tools.web.fetch.enabled to true, which ignores the non-secret gate used by the runtime secret injector (BRAVE_API_KEY) and causes config/runtime drift; change the config.update call that sets 'tools.web.search.enabled' and 'tools.web.fetch.enabled' to use a non-secret enablement flag (e.g., a boolean variable or env flag) instead of hard true so the runtime logic that checks webSearchConfig?.fetchEnabled still controls whether BRAVE_API_KEY is injected, or remove the redundant toggle everywhere and make enablement explicit in one place; look for the config.update call that sets 'tools.web.search'/'tools.web.fetch' and the runtime injector that references BRAVE_API_KEY and ensure only the non-secret gate (not the secret presence) decides fetchEnabled.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@Dockerfile`:
- Around line 160-171: The config block currently hard-sets
tools.web.search.enabled and tools.web.fetch.enabled to true, which ignores the
non-secret gate used by the runtime secret injector (BRAVE_API_KEY) and causes
config/runtime drift; change the config.update call that sets
'tools.web.search.enabled' and 'tools.web.fetch.enabled' to use a non-secret
enablement flag (e.g., a boolean variable or env flag) instead of hard true so
the runtime logic that checks webSearchConfig?.fetchEnabled still controls
whether BRAVE_API_KEY is injected, or remove the redundant toggle everywhere and
make enablement explicit in one place; look for the config.update call that sets
'tools.web.search'/'tools.web.fetch' and the runtime injector that references
BRAVE_API_KEY and ensure only the non-secret gate (not the secret presence)
decides fetchEnabled.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 25f81872-8320-42ec-95ba-13c57a45a06d
📒 Files selected for processing (2)
Dockerfilesrc/lib/onboard.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lib/onboard.ts
Address CodeRabbit review: the Dockerfile always enabled web search with a placeholder API key, even when the user didn't configure Brave. This caused config/runtime drift — openclaw.json had search.enabled but no actual key at runtime. - Add NEMOCLAW_WEB_SEARCH_ENABLED build arg (non-secret boolean) - Gate the web search config block on this flag - Replace NEMOCLAW_WEB_CONFIG_B64 patching with NEMOCLAW_WEB_SEARCH_ENABLED - Update tests to match new arg name Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com> Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Collaborator
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/onboard.test.ts (1)
642-642: Add a regression guard for legacyNEMOCLAW_WEB_CONFIG_B64removal.The enabled-path assertion should also verify the deprecated build arg is absent, so secret-bearing config injection can’t silently reappear.
Patch suggestion
const patched = fs.readFileSync(dockerfilePath, "utf8"); assert.match(patched, /^ARG NEMOCLAW_WEB_SEARCH_ENABLED=1$/m); + assert.doesNotMatch(patched, /^ARG NEMOCLAW_WEB_CONFIG_B64=/m);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/onboard.test.ts` at line 642, The test currently only asserts the enabled build arg exists (assert.match(patched, /^ARG NEMOCLAW_WEB_SEARCH_ENABLED=1$/m)); update this test to also assert the deprecated secret-bearing arg is absent by adding a negative assertion against "NEMOCLAW_WEB_CONFIG_B64" (e.g., use assert.doesNotMatch or assert.notMatch on the same patched string) so the legacy build arg cannot silently reappear — locate the assertion on the patched variable in onboard.test.ts and add the complementary absence check.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/onboard.test.ts`:
- Line 642: The test currently only asserts the enabled build arg exists
(assert.match(patched, /^ARG NEMOCLAW_WEB_SEARCH_ENABLED=1$/m)); update this
test to also assert the deprecated secret-bearing arg is absent by adding a
negative assertion against "NEMOCLAW_WEB_CONFIG_B64" (e.g., use
assert.doesNotMatch or assert.notMatch on the same patched string) so the legacy
build arg cannot silently reappear — locate the assertion on the patched
variable in onboard.test.ts and add the complementary absence check.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 444abca2-2176-4295-bb44-173b29909b4c
📒 Files selected for processing (3)
Dockerfilesrc/lib/onboard.tstest/onboard.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lib/onboard.ts
The Dockerfile's Python script is a single-line python3 -c command.
An if-block with indented body is invalid in this context. Use the
inline ternary form: config.update({...}) if condition else None.
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/onboard.test.ts (1)
642-642: Add a negative assertion to lock out legacy config regressions.Consider explicitly asserting the patched Dockerfile no longer contains
NEMOCLAW_WEB_CONFIG_B64.✅ Suggested assertion addition
const patched = fs.readFileSync(dockerfilePath, "utf8"); assert.match(patched, /^ARG NEMOCLAW_WEB_SEARCH_ENABLED=1$/m); + assert.doesNotMatch(patched, /^ARG NEMOCLAW_WEB_CONFIG_B64=/m);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/onboard.test.ts` at line 642, Add a negative assertion to the existing test that verifies the patched Dockerfile no longer contains the legacy variable by asserting the string "NEMOCLAW_WEB_CONFIG_B64" is not present in the `patched` variable used in test/onboard.test.ts (near the existing assert.match(patched, /^ARG NEMOCLAW_WEB_SEARCH_ENABLED=1$/m)). Insert an assertion using the test framework's negative match (e.g., assert.doesNotMatch or assert.notInclude depending on project conventions) to explicitly fail if `NEMOCLAW_WEB_CONFIG_B64` appears, preventing regressions to the legacy config.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/onboard.test.ts`:
- Line 642: Add a negative assertion to the existing test that verifies the
patched Dockerfile no longer contains the legacy variable by asserting the
string "NEMOCLAW_WEB_CONFIG_B64" is not present in the `patched` variable used
in test/onboard.test.ts (near the existing assert.match(patched, /^ARG
NEMOCLAW_WEB_SEARCH_ENABLED=1$/m)). Insert an assertion using the test
framework's negative match (e.g., assert.doesNotMatch or assert.notInclude
depending on project conventions) to explicitly fail if
`NEMOCLAW_WEB_CONFIG_B64` appears, preventing regressions to the legacy config.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 27b0ccaa-1486-417e-b309-b9a365dde385
📒 Files selected for processing (3)
Dockerfilesrc/lib/onboard.tstest/onboard.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lib/onboard.ts
- Add negative assertion: patched Dockerfile must NOT contain NEMOCLAW_WEB_CONFIG_B64 (prevents secret-bearing arg reintroduction) - Remove dead buildWebSearchDockerConfig() and encodeDockerJsonArg() from web-search.ts — no longer called after build arg removal - Update web-search tests to match simplified module Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com> Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
ericksoa
approved these changes
Apr 15, 2026
ericksoa
left a comment
ericksoa
left a comment
Contributor
There was a problem hiding this comment.
LGTM — clean removal of build-time secret exposure. Runtime injection via openshell:resolve:env is the right pattern. Net deletion, dead code removed, regression guard in tests.
7 tasks
wscurran
added
chore
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
5 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
This PR remediates a security vulnerability where web search configuration—including API key placeholders—was exposed during the Docker build process. By migrating configuration from build-time
ARGinjection to runtime environment resolution via OpenShell, we ensure that no sensitive-looking strings appear in build logs, image metadata, or Docker history.Related Issue
Fixes a security exposure where
NEMOCLAW_WEB_CONFIG_B64was leaked indocker buildlogs.Issue.
Changes
Dockerfile:ARG NEMOCLAW_WEB_CONFIG_B64and its correspondingENVdeclaration.web.searchblock using the safeopenshell:resolve:env:BRAVE_API_KEYplaceholder.src/lib/onboard.ts:Dockerfilemodification for web search.BRAVE_API_KEYdirectly into the sandbox's environment variables duringopenshell sandbox create.scripts/install-openshell.sh:Type of Change
Testing
npx prek run --all-filespasses (manually verified).docker buildlogs no longer containNEMOCLAW_WEB_CONFIG_B64.docker inspect <image>anddocker history <image>show no trace of the web search configuration.Checklist
General
Code Changes
Signed-off-by: Krish Sapru ksapru@bu.edu
Summary by CodeRabbit