fix(config): redact browser CDP URLs in config snapshots#53418
fix(config): redact browser CDP URLs in config snapshots#53418coygeek wants to merge 2 commits intoopenclaw:mainfrom
Conversation
Generated by staged fix workflow.
Greptile SummaryThis PR fixes a credential-leak vulnerability where
Confidence Score: 4/5The security fix is correct and well-tested, but a P1 runtime bug in the unrelated UI change breaks the /model slash command in the Web UI. The core security fix (schema hints, zod registration, fallback path, tests) is solid and complete. However, this PR also modifies slash-command-executor.ts and introduces a reference to an undefined state variable that causes a ReferenceError whenever /model value is used — that's a P1 bug on the changed path that should be resolved before merging. ui/src/ui/chat/slash-command-executor.ts — line 165 references undefined Prompt To Fix All With AIThis is a comment left during a code review.
Path: ui/src/ui/chat/slash-command-executor.ts
Line: 165
Comment:
**`state` is not defined in this scope**
`state` is referenced at line 165 but is not a parameter to `executeSlashCommand` or `executeModel`, and is not imported or declared anywhere in this module. At runtime, this throws `ReferenceError: state is not defined`, which is caught by the surrounding try/catch and returned as `"Failed to set model: ReferenceError: state is not defined"` — so `/model <value>` silently misfires and the `sessionPatch.modelOverride` in the response is never set.
The caller in `app-chat.ts` is `executeSlashCommand(host.client, targetSessionKey, name, args)` and does not pass catalog state. The function signature needs a `catalog` (or `state`) parameter, or the catalog should be fetched inside `executeModel` (which already fetches session and model data via the client).
```suggestion
? normalizeChatModelOverrideValue(rawOverride, []) ||
```
(Temporary safe fallback — the proper fix is to either pass the catalog as a parameter to `executeSlashCommand`/`executeModel`, or fetch it from `models.list` which is already called in the `!args` branch.)
How can I resolve this? If you propose a fix, please make it concise.Reviews (3): Last reviewed commit: "fix(de-dny-remote-cdp-url-redaction-bypa..." | Re-trigger Greptile |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b0d95821b6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| enabled: z.boolean().optional(), | ||
| evaluateEnabled: z.boolean().optional(), | ||
| cdpUrl: z.string().optional(), | ||
| cdpUrl: z.string().optional().register(sensitive), |
There was a problem hiding this comment.
Regenerate shipped schema hints for cdpUrl redaction
Marking cdpUrl as sensitive in OpenClawSchema is not sufficient by itself because config.get redaction uses schema.uiHints from GENERATED_BASE_CONFIG_SCHEMA (src/config/schema.ts), and this commit does not update src/config/schema.base.generated.ts where browser.cdpUrl / browser.profiles.*.cdpUrl still lack sensitive: true. In that state, CDP URLs that carry only query credentials (for example ?token=... without user:pass@) are still returned unredacted in config snapshots, so the security fix remains incomplete until the generated schema payload is regenerated and committed.
Useful? React with 👍 / 👎.
| it("redacts browser.cdpUrl and browser.profiles.*.cdpUrl via schema hints", () => { | ||
| const snapshot = makeSnapshot({ | ||
| browser: { | ||
| cdpUrl: "wss://chrome.browserless.io/?token=secret-token-123", | ||
| profiles: { | ||
| remote: { | ||
| cdpUrl: "https://user:pass@browserbase.example.test/connect", | ||
| cdpPort: 9222, | ||
| }, | ||
| }, | ||
| }, | ||
| }); | ||
|
|
||
| const result = redactConfigSnapshot(snapshot, mainSchemaHints); | ||
| const config = result.config as typeof snapshot.config; | ||
| expect(config.browser.cdpUrl).toBe(REDACTED_SENTINEL); | ||
| expect(config.browser.profiles.remote.cdpUrl).toBe(REDACTED_SENTINEL); | ||
| expect(config.browser.profiles.remote.cdpPort).toBe(9222); | ||
|
|
||
| const restored = restoreRedactedValues(result.config, snapshot.config, mainSchemaHints); | ||
| expect(restored.browser.cdpUrl).toBe("wss://chrome.browserless.io/?token=secret-token-123"); | ||
| expect(restored.browser.profiles.remote.cdpUrl).toBe( | ||
| "https://user:pass@browserbase.example.test/connect", | ||
| ); | ||
| }); |
There was a problem hiding this comment.
This test imports redactSnapshotTestHints (from redact-snapshot.test-hints.ts) aliased as mainSchemaHints. The test fixture was updated to include "browser.cdpUrl": { sensitive: true }, so the test passes. However, the actual production-facing hints file (src/config/schema.base.generated.ts) was NOT updated — it still lacks sensitive: true for both "browser.cdpUrl" and "browser.profiles.*.cdpUrl":
// Current state of schema.base.generated.ts (unchanged by this PR):
"browser.cdpUrl": {
label: "Browser CDP URL",
tags: ["advanced"],
// sensitive: true <-- MISSING
},
"browser.profiles.*.cdpUrl": {
label: "Browser Profile CDP URL",
tags: ["storage"],
// sensitive: true <-- MISSING
},The PR description explicitly names regenerating this file as one of three fix layers, but the generator (scripts/generate-base-config-schema.ts) was not re-run after adding .register(sensitive) to zod-schema.ts.
Concrete exposure: a CDP URL that uses only a query-string token with no embedded user:pass@ credentials would pass through unredacted when real schema hints (from GENERATED_BASE_CONFIG_SCHEMA) are used. stripUrlUserInfo only strips userinfo credentials; query-string tokens are left intact. The test only passes because the fixture has sensitive: true, masking this gap.
The fix is to re-run the schema generator so schema.base.generated.ts includes sensitive: true for both cdpUrl hint entries.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/config/redact-snapshot.schema.test.ts
Line: 78-102
Comment:
**Schema test validates fixture, not the real generated hints**
This test imports `redactSnapshotTestHints` (from `redact-snapshot.test-hints.ts`) aliased as `mainSchemaHints`. The test fixture was updated to include `"browser.cdpUrl": { sensitive: true }`, so the test passes. However, the actual production-facing hints file (`src/config/schema.base.generated.ts`) was **NOT updated** — it still lacks `sensitive: true` for both `"browser.cdpUrl"` and `"browser.profiles.*.cdpUrl"`:
```ts
// Current state of schema.base.generated.ts (unchanged by this PR):
"browser.cdpUrl": {
label: "Browser CDP URL",
tags: ["advanced"],
// sensitive: true <-- MISSING
},
"browser.profiles.*.cdpUrl": {
label: "Browser Profile CDP URL",
tags: ["storage"],
// sensitive: true <-- MISSING
},
```
The PR description explicitly names regenerating this file as one of three fix layers, but the generator (`scripts/generate-base-config-schema.ts`) was not re-run after adding `.register(sensitive)` to `zod-schema.ts`.
**Concrete exposure**: a CDP URL that uses only a query-string token with no embedded `user:pass@` credentials would pass through unredacted when real schema hints (from `GENERATED_BASE_CONFIG_SCHEMA`) are used. `stripUrlUserInfo` only strips userinfo credentials; query-string tokens are left intact. The test only passes because the fixture has `sensitive: true`, masking this gap.
The fix is to re-run the schema generator so `schema.base.generated.ts` includes `sensitive: true` for both `cdpUrl` hint entries.
How can I resolve this? If you propose a fix, please make it concise.Generated by staged fix workflow.
|
Re: Greptile review — Verified: running The test fixture approach ( No code changes needed; the diff is already correct via the proper codegen path. |
fabianwilliams
left a comment
There was a problem hiding this comment.
Solid security fix. CDP URLs can contain embedded credentials (tokens, userinfo) and should absolutely be treated as sensitive.
What's done right:
- Both
browser.cdpUrlandbrowser.profiles.*.cdpUrlmarkedsensitive: truein schema + config baseline isUserInfoUrlPathextended to catch.cdpUrlendings — catches credential-in-URL patterns- Zod schema registers both paths with the
sensitivemarker - Comprehensive tests: embedded credentials in both global and per-profile cdpUrl, plus raw config string redaction
- Restore round-trip verified (redact → restore returns original values)
- Non-sensitive fields like
cdpPortconfirmed unaffected
The UI import reformatting is incidental but clean. LGTM — approve.
|
@greptile-apps review |
| const resolvedValue = rawOverride | ||
| ? (normalizeChatModelOverrideValue(rawOverride, state.chatModelCatalog ?? []) || | ||
| resolveServerChatModelValue(patchedModel, patched.resolved?.modelProvider)) | ||
| ? normalizeChatModelOverrideValue(rawOverride, state.chatModelCatalog ?? []) || |
There was a problem hiding this comment.
state is not defined in this scope
state is referenced at line 165 but is not a parameter to executeSlashCommand or executeModel, and is not imported or declared anywhere in this module. At runtime, this throws ReferenceError: state is not defined, which is caught by the surrounding try/catch and returned as "Failed to set model: ReferenceError: state is not defined" — so /model <value> silently misfires and the sessionPatch.modelOverride in the response is never set.
The caller in app-chat.ts is executeSlashCommand(host.client, targetSessionKey, name, args) and does not pass catalog state. The function signature needs a catalog (or state) parameter, or the catalog should be fetched inside executeModel (which already fetches session and model data via the client).
| ? normalizeChatModelOverrideValue(rawOverride, state.chatModelCatalog ?? []) || | |
| ? normalizeChatModelOverrideValue(rawOverride, []) || |
(Temporary safe fallback — the proper fix is to either pass the catalog as a parameter to executeSlashCommand/executeModel, or fetch it from models.list which is already called in the !args branch.)
Prompt To Fix With AI
This is a comment left during a code review.
Path: ui/src/ui/chat/slash-command-executor.ts
Line: 165
Comment:
**`state` is not defined in this scope**
`state` is referenced at line 165 but is not a parameter to `executeSlashCommand` or `executeModel`, and is not imported or declared anywhere in this module. At runtime, this throws `ReferenceError: state is not defined`, which is caught by the surrounding try/catch and returned as `"Failed to set model: ReferenceError: state is not defined"` — so `/model <value>` silently misfires and the `sessionPatch.modelOverride` in the response is never set.
The caller in `app-chat.ts` is `executeSlashCommand(host.client, targetSessionKey, name, args)` and does not pass catalog state. The function signature needs a `catalog` (or `state`) parameter, or the catalog should be fetched inside `executeModel` (which already fetches session and model data via the client).
```suggestion
? normalizeChatModelOverrideValue(rawOverride, []) ||
```
(Temporary safe fallback — the proper fix is to either pass the catalog as a parameter to `executeSlashCommand`/`executeModel`, or fetch it from `models.list` which is already called in the `!args` branch.)
How can I resolve this? If you propose a fix, please make it concise.
Fix Summary
Authenticated clients with only
operator.readscope can callconfig.getand receive the full, unredactedbrowser.cdpUrlandbrowser.profiles.*.cdpUrlvalues, including embedded query tokens (e.g., Browserless?token=...) and HTTP Basic credentials (e.g.,user:pass@host). The leaked credentials are reusable outside OpenClaw to connect directly to the upstream browser service, bypassing OpenClaw's audit trail and scope checks.Issue Linkage
Fixes #53433
Security Snapshot
Implementation Details
Files Changed
docs/.generated/config-baseline.json(+5/-3)docs/.generated/config-baseline.jsonl(+2/-2)src/config/redact-snapshot.schema.test.ts(+26/-0)src/config/redact-snapshot.test-hints.ts(+2/-0)src/config/redact-snapshot.test.ts(+55/-0)src/config/redact-snapshot.ts(+1/-1)src/config/schema.base.generated.ts(+4/-2)src/config/zod-schema.ts(+2/-2)ui/src/ui/chat/slash-command-executor.ts(+7/-3)Technical Analysis
operator.readscope.config.get.cdpUrlinpayload.config.browser.profiles.remote.cdpUrl, including the query token and Basic-auth userinfo.Validation Evidence
pnpm exec oxlint src/ && pnpm build && pnpm check && pnpm testRisk and Compatibility
non-breaking; no known regression impact
AI-Assisted Disclosure