fix(gateway): detect C1 control characters in config lookup path sanitizer by lsr911 · Pull Request #103402 · openclaw/openclaw

lsr911 · 2026-07-10T05:04:55Z

What Problem This Solves

sanitizeLookupPathForLog() in src/gateway/server-methods/config.ts replaces control characters in a config path with ? before the path is embedded in the gateway warning logs (config.schema.lookup produced invalid payload for ... and config.openFile failed path=...). It only replaced the C0 range (0x00-0x1f) and DEL (0x7f), not the C1 control range (0x80-0x9f), which includes the CSI introducer U+009B — an alternative ANSI escape prefix equivalent to ESC [. A C1 byte in the path therefore passed unchanged into both warning-log callers.

Change

Extend the replacement predicate to also cover 0x80-0x9f:

return code < 0x20 || code === 0x7f || (code >= 0x80 && code <= 0x9f) ? "?" : char;

Testing / Proof

Added a regression to the existing config.openFile suite in src/gateway/server-methods/config.test.ts. It drives the real config.openFile handler with a config path containing U+009B, forces an open failure, and asserts the emitted logGateway.warn message replaces the C1 byte with ? (cfg?.json) and contains no raw C1 byte. The assertion is scoped to the sanitized filename so it is independent of the OS-specific path prefix.

$ node scripts/run-vitest.mjs run src/gateway/server-methods/config.test.ts
...
      Tests  8 failed | 14 passed (22)

The new replaces C1 control characters in the logged failed config path test passes (passed count rises from 12 → 14, i.e. +1 in each of the two gateway vitest projects).

Note on the 8 "failed": these are 4 pre-existing sibling config.openFile tests (× 2 projects) that hard-code POSIX /tmp/... paths and assert the full path string. On my local Windows checkout the handler resolves those to D:\tmp\..., so they fail identically on main — they are unrelated to this change and pass on Linux CI. This PR's new test deliberately avoids the path prefix and asserts only the sanitized cfg?.json fragment, so it is green on both platforms.

Same C1 gap pattern as openclaw#103274. Add full C1 range (0x80-0x9f). Co-Authored-By: Claude <[email protected]>

clawsweeper · 2026-07-10T10:45:34Z

Codex review: needs real behavior proof before merge. Reviewed July 11, 2026, 4:39 AM ET / 08:39 UTC.

Summary
The PR extends gateway config-path log sanitization to replace C1 control characters and adds a regression test for the config-open warning path.

PR surface: Source 0, Tests +21. Total +21 across 2 files.

Reproducibility: yes. at source and dependency-contract level with high confidence: current code passes C1 characters through the helper, and a real Node child-process failure confirms that its error message can echo the same raw path into the warning suffix.

Review metrics: none identified.

Merge readiness
Overall: 🧂 unranked krab
Proof: 🧂 unranked krab
Patch quality: 🦪 silver shellfish
Result: blocked until real behavior proof is added.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

[P1] Sanitize the child-process error field and add an exec-style full-warning regression.
Replace the String(unknown) test extraction with a typed or explicitly narrowed logger argument.
Post redacted terminal or log output from a real failed config.openFile call containing U+009B.

Proof guidance:

[P1] Needs real behavior proof before merge: No redacted after-fix gateway warning from a real opener failure is provided; tests and CI are supplemental only. After adding terminal output or logs, redact private paths, IP addresses, keys, phone numbers, endpoints, and other private details, then update the PR body to trigger review or ask a maintainer to comment @clawsweeper re-review.

Risk before merge

[P1] The PR still lacks redacted after-fix output from a real gateway opener failure, so the external-contributor proof gate remains unmet.
[P1] The related sanitizer discussion requests sink-wide reasoning; correcting only the explicit path field would continue to leave a competing unsanitized dynamic field in the same warning.

Maintainer options:

Decide the mitigation before merge
Sanitize every untrusted dynamic field in the complete gateway warning, preserve the bounded and UTF-16-safe path rendering, add an exec-style regression that echoes the U+009B-bearing path, and attach redacted real warning output before merge.
Pause or close
Do not merge this PR until maintainers decide whether the risk is worth taking.

Next step before merge

[P1] The contributor must correct the current branch and supply real-setup proof; ClawSweeper should not create a separate repair branch while that contributor-only proof gate remains open.

Security
Needs attention: The intended log-injection hardening is incomplete because real child-process error text can reintroduce the same control-bearing path.

Review findings

[P1] Sanitize the child-process error text too — src/gateway/server-methods/config.ts:358
[P2] Use a typed warning argument instead of stringifying unknown — src/gateway/server-methods/config.test.ts:159-162

Review details

Best possible solution:

Sanitize every untrusted dynamic field in the complete gateway warning, preserve the bounded and UTF-16-safe path rendering, add an exec-style regression that echoes the U+009B-bearing path, and attach redacted real warning output before merge.

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

Yes at source and dependency-contract level with high confidence: current code passes C1 characters through the helper, and a real Node child-process failure confirms that its error message can echo the same raw path into the warning suffix.

Is this the best way to solve the issue?

No, not as submitted. Extending the path predicate is correct, but the full warning must also sanitize child-process error text that can repeat the path, and the maintained test must pass repository lint.

Full review comments:

[P1] Sanitize the child-process error text too — src/gateway/server-methods/config.ts:358
The warning appends errorMessage after the newly sanitized path, but real nonzero execFile errors include command arguments in Error.message. A U+009B-bearing config path can therefore still reach this warning through the suffix, while the test's synthetic Error("open failed") misses that path. Sanitize the dynamic error field and cover the entire warning with an exec-style echoed-path error. This is a late finding because the same interpolation was present at the previously reviewed head.
Confidence: 0.98
Late finding: first raised on code an earlier review cycle already covered.
[P2] Use a typed warning argument instead of stringifying unknown — src/gateway/server-methods/config.test.ts:159-162
Current-head lint rejects String(mock.calls...?.[0]) because the value is typed as unknown and may use [object Object] stringification. Read the mock call through its actual logger argument type or narrow the value to string before asserting.
Confidence: 0.99

Overall correctness: patch is incorrect
Overall confidence: 0.98

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 36e54a883038.

Label changes

Label justifications:

P2: This is a bounded gateway log-hardening defect with limited runtime blast radius and a focused repair path.
rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab and patch quality is 🦪 silver shellfish.
status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: No redacted after-fix gateway warning from a real opener failure is provided; tests and CI are supplemental only. After adding terminal output or logs, redact private paths, IP addresses, keys, phone numbers, endpoints, and other private details, then update the PR body to trigger review or ask a maintainer to comment @clawsweeper re-review.

Evidence reviewed

PR surface:

Source 0, Tests +21. Total +21 across 2 files.

View PR surface stats

Area	Files	Added	Removed	Net
Source	1	1	1	0
Tests	1	21	0	+21
Docs	0	0	0	0
Config	0	0	0	0
Generated	0	0	0	0
Other	0	0	0	0
Total	2	22	1	+21

Security concerns:

[medium] Raw exec error bypasses path-only sanitization — src/gateway/server-methods/config.ts:1015
A nonzero execFile error includes command arguments in its message, and the gateway warning appends that message without ANSI or C0/C1 sanitization.
Confidence: 0.98

What I checked:

Path predicate change: The patch correctly adds U+0080–U+009F to the path helper's existing C0 and DEL replacement predicate. (src/gateway/server-methods/config.ts:358, 38d1d18d11b9)
Unsanitized error suffix: The config-open warning sanitizes the explicit path but appends errorMessage directly, allowing process error text to carry another raw copy of the path. (src/gateway/server-methods/config.ts:1015, 36e54a883038)
Child-process reproduction: A real Node execFile('/bin/false', [path]) failure produced Command failed: /bin/false /tmp/cfg<U+009B>.json and retained the raw C1 character in Error.message.
Shared log-sanitization contract: The terminal-core log sanitizer removes ANSI sequences plus C0, DEL, and the full C1 range to prevent terminal escape injection and log forging. (packages/terminal-core/src/ansi.ts:211, 36e54a883038)
Regression coverage limitation: The new handler test uses Error("open failed"), so it cannot detect the raw echoed-path copy present in real nonzero child-process errors. (src/gateway/server-methods/config.test.ts:148, 38d1d18d11b9)
Lint failure: Current-head CI rejects the test's String(unknown) conversion under typescript(no-base-to-string). (src/gateway/server-methods/config.test.ts:159, 38d1d18d11b9)

Likely related people:

mmaps: Commit 5cc0bc9 introduced config.openFile, its child-process execution, warning construction, and adjacent tests. (role: introduced config open behavior; confidence: high; commits: 5cc0bc936c4d; files: src/gateway/server-methods/config.ts, src/gateway/server-methods/config.test.ts)
gumadeiras: Commit ff97195 introduced config.schema.lookup, the other current warning caller using this path sanitizer. (role: introduced schema lookup behavior; confidence: high; commits: ff971955009a; files: src/gateway/server-methods/config.ts)
lsr911: The author also contributed merged commit a3f9f35, which completed C1 filtering for the distinct gateway terminal-buffer sink. (role: recent adjacent sanitizer contributor; confidence: medium; commits: a3f9f3567f71; files: src/gateway/terminal/buffer-text.ts)
steipete: The related gateway terminal-buffer C1 fix was prepared, reviewed, and merged with explicit sink-boundary guidance by this contributor. (role: reviewer and merger of adjacent behavior; confidence: medium; commits: a3f9f3567f71, f9ebefba29a0; files: src/gateway/terminal/buffer-text.ts, src/gateway/server-methods/config.ts)

What the crustacean ranks mean

🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
🌊 off-meta tidepool: rating does not apply to this item.

Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

How this review workflow works

ClawSweeper keeps one durable marker-backed review comment per issue or PR.
Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
Maintainers can also comment @clawsweeper review to request a fresh review only.
Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

Review history (1 earlier review cycle)

reviewed 2026-07-10T10:43:51.296Z sha f217516 :: needs real behavior proof before merge. :: none

Add a regression asserting that a config path carrying a C1 byte (U+009B CSI) is replaced with '?' by sanitizeLookupPathForLog before it reaches the config.openFile warning log, and that no raw C1 byte survives. Assertion is scoped to the sanitized filename so it is OS-path-prefix independent. Signed-off-by: lsr911 <[email protected]>

lsr911 · 2026-07-11T09:44:40Z

Superseded by #104362 — a single sink-wide C1 (0x80–0x9f) pass that folds this sink's fix in with a valid U+009B regression, alongside an audit of every other control-character sink (already-covered / deferred / out-of-scope). This consolidates the isolated C1 PRs per the direction on #103226. Closing in favor of the consolidated review.

…g sinks Terminal-control sanitizers guard untrusted text before it reaches a terminal or log (CWE-117). The canonical range is C0 (0x00-0x1f), DEL (0x7f), and C1 (0x80-0x9f) - the range already used by the shared sanitizeForLog and by renderTerminalBufferText (openclaw#103274). C1 includes the 8-bit CSI introducer U+009B. Deliberate sink-wide pass: closes the C1 gap in twelve terminal/log output sinks, each keeping its own strip/escape/replace/truncate/reject action and gaining a focused valid-U+009B regression test. The PR body audits every other control-char sink (already-covered, deferred, or out-of-scope). Consolidates the isolated PRs openclaw#103379 openclaw#103380 openclaw#103381 openclaw#103382 openclaw#103383 openclaw#103402 Signed-off-by: lsr911 <[email protected]> openclaw#103405 and re-lands the sanitizeForConsole fix from openclaw#103226 cleanly.

fix(gateway): detect C1 control characters in config path sanitizer

f217516

Same C1 gap pattern as openclaw#103274. Add full C1 range (0x80-0x9f). Co-Authored-By: Claude <[email protected]>

openclaw-barnacle Bot added gateway Gateway runtime size: XS triage: needs-pr-context Candidate: external PR body lacks required problem context or evidence. and removed triage: needs-pr-context Candidate: external PR body lacks required problem context or evidence. labels Jul 10, 2026

openclaw-barnacle Bot added the triage: needs-pr-context Candidate: external PR body lacks required problem context or evidence. label Jul 11, 2026

This was referenced Jul 11, 2026

fix(agents): strip C1 control characters (0x80-0x9f) in sanitizeForConsole #103226

Closed

fix: complete C1 control-character (0x80-0x9f) coverage across terminal/log sinks #104362

Closed

lsr911 closed this Jul 11, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

fix(gateway): detect C1 control characters in config lookup path sanitizer#103402

fix(gateway): detect C1 control characters in config lookup path sanitizer#103402
lsr911 wants to merge 2 commits into
openclaw:mainfrom
lsr911:fix/config-c1

lsr911 commented Jul 10, 2026 •

edited

Loading

Uh oh!

clawsweeper Bot commented Jul 10, 2026 •

edited

Loading

Uh oh!

lsr911 commented Jul 11, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Uh oh!

Conversation

lsr911 commented Jul 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What Problem This Solves

Change

Testing / Proof

Uh oh!

clawsweeper Bot commented Jul 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lsr911 commented Jul 11, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

lsr911 commented Jul 10, 2026 •

edited

Loading

clawsweeper Bot commented Jul 10, 2026 •

edited

Loading