fix(sandbox): detect C1 control characters in hasUnsafeControlChars by lsr911 · Pull Request #103381 · openclaw/openclaw

lsr911 · 2026-07-10T04:46:49Z

What Problem This Solves

When sandbox tool policy blocks a tool, formatSandboxToolPolicyBlockedMessage() in src/agents/sandbox/runtime-status.ts renders a single-line denial message that includes a redacted Session: label and a openclaw sandbox explain ... command. Two code paths handle an untrusted session key, and both must be hardened against the C1 control range (0x80-0x9f), which includes the CSI introducer U+009B (an alternative ANSI escape prefix equivalent to ESC [):

Explain-command gating — hasUnsafeControlChars() decides whether the command uses the raw key (--session '<key>') or the agent-only fallback (--agent <id>). It only covered C0/DEL, so a C1-bearing key was still passed to --session.
Visible Session label — redactSessionKey() → sanitizeForSingleLineDisplay() → the shared escapeControlCharsVisible() (in src/agents/tool-policy-audit.ts). It only escaped C0/DEL, so a C1 byte in the redacted first/last-6 of the key reached the visible Session: line raw.

The first review pass only fixed path (1). This revision fixes both, matching ClawSweeper's recommended solution ("extend the shared visible-control escaping contract through C1, retain the agent-only fallback for unsafe keys, and test both outputs").

Change

hasUnsafeControlChars now treats 0x80-0x9f as unsafe → agent-only explain fallback for C1-bearing keys.
escapeControlCharsVisible (shared) now escapes 0x80-0x9f as \xNN → the visible Session label no longer emits raw C1 bytes.

Testing / Proof

Added an end-to-end regression in src/agents/sandbox-explain.test.ts that calls the real formatSandboxToolPolicyBlockedMessage with a session key whose trailing (visible) 6 chars contain U+009B, asserting both outputs:

the Session: line contains the escaped \x9b and not the raw C1 byte;
the explain command falls back to openclaw sandbox explain --agent ... and never emits --session with the raw key.

$ node scripts/run-vitest.mjs run src/agents/sandbox-explain.test.ts

 RUN  v4.1.8 D:/IdeaWork/openclaw-repo

 Test Files  1 passed (1)
      Tests  6 passed (6)
   Duration  2.79s

The test drives the real formatting path, so the passing run is the before/after proof: on current main the same input leaves the raw U+009B in the Session line and routes to --session; with this change it is escaped and routed to --agent.

hasUnsafeControlChars() checked for C0 control characters but not C1 controls (0x80-0x9f). Add the C1 range. Same pattern as openclaw#103226, openclaw#103274, openclaw#103379, openclaw#103380. Co-Authored-By: Claude <[email protected]>

clawsweeper · 2026-07-10T10:37:14Z

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

Summary
Extends sandbox session-key command gating and shared visible-control escaping through the C1 range, with regression coverage for the displayed Session label and agent-only explain fallback.

PR surface: Source +3, Tests +35. Total +38 across 3 files.

Reproducibility: yes. at source level. Current main’s C0/DEL-only predicates allow U+009B through the retained Session-label segment and select the raw-session explain command, though no live current-main invocation was run during this read-only review.

Review metrics: none identified.

Merge readiness
Overall: 🦪 silver shellfish
Proof: 🦪 silver shellfish
Patch quality: 🐚 platinum hermit
Result: blocked until real behavior proof from a real setup is added.

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

Rank-up moves:

Attach redacted exact-head terminal output, live output, or logs showing the Session line contains \x9b and the suggested command uses --agent without --session; remove private values such as keys, phone numbers, IP addresses, and non-public endpoints.

Proof guidance:

[P1] Needs real behavior proof before merge: The PR body provides a focused Vitest transcript but no redacted exact-head terminal output or runtime log from an actual sandbox policy-block flow; after adding proof, updating the PR body should trigger review, or a maintainer can request @clawsweeper re-review. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Next step before merge

[P1] No code repair is indicated; the contributor must add real behavior proof from the exact head before merge.

Security
Cleared: The patch reduces terminal-control exposure and introduces no dependency, workflow, permission, secret-handling, artifact-download, or package-resolution changes.

Review details

Best possible solution:

Keep the canonical shared C0/DEL/C1 escaping contract and unsafe-key fallback, then merge after redacted exact-head runtime output demonstrates both corrected sandbox denial outputs.

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

Yes, at source level. Current main’s C0/DEL-only predicates allow U+009B through the retained Session-label segment and select the raw-session explain command, though no live current-main invocation was run during this read-only review.

Is this the best way to solve the issue?

Yes. Extending the existing shared visible-escaping helper and established unsafe-session fallback is narrower and more maintainable than introducing a parallel sanitizer.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 7bf80dc2c64e.

Label changes

Label changes:

add rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish and patch quality is 🐚 platinum hermit.
remove rating: 🧂 unranked krab: Current PR rating is rating: 🦪 silver shellfish, so this older rating label is no longer current.

Label justifications:

P2: This fixes a concrete but limited-blast-radius terminal-control rendering gap in sandbox diagnostics.
rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish and patch quality is 🐚 platinum hermit.
status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The PR body provides a focused Vitest transcript but no redacted exact-head terminal output or runtime log from an actual sandbox policy-block flow; after adding proof, updating the PR body should trigger review, or a maintainer can request @clawsweeper re-review. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Evidence reviewed

PR surface:

Source +3, Tests +35. Total +38 across 3 files.

View PR surface stats

Area	Files	Added	Removed	Net
Source	2	5	2	+3
Tests	1	35	0	+35
Docs	0	0	0	0
Config	0	0	0	0
Generated	0	0	0	0
Other	0	0	0	0
Total	3	40	2	+38

What I checked:

Current-main reproduction: Current main treats only C0 and DEL as unsafe, so U+009B remains eligible for the raw --session command path. (src/agents/sandbox/runtime-status.ts:104, 7bf80dc2c64e)
Shared display gap: The current shared visible-control helper escapes C0 and DEL but not C1, allowing a retained U+009B to reach single-line output unchanged. (src/agents/tool-policy-audit.ts:134, 7bf80dc2c64e)
Established safety contract: Existing coverage already requires control-bearing session keys to render visibly escaped and use the agent-scoped explain fallback; this PR extends that invariant from C0 to C1. (src/agents/sandbox/tool-policy.test.ts:369, 7bf80dc2c64e)
Prior finding resolved: The current PR head adds C1 handling to both the shared display helper and unsafe-key command gate, plus a composed formatter regression test. (src/agents/tool-policy-audit.ts:134, 578c2ea763bf)
Exact-head validation state: Sixty-five checks pass; the sole failing check bucket is Real behavior proof. (578c2ea763bf)
Feature history: Path history ties the original tool-policy audit behavior to Vincent Koc, recent sandbox session-label safety to mushuiyu886, and subsequent shared-helper maintenance to Peter Steinberger and lsr911. (src/agents/sandbox/runtime-status.ts:108, b714fdb3ae47)

Likely related people:

steipete: Authored several recent refactors and safety-helper updates on the sandbox runtime-status path, including shared-helper consolidation. (role: recent area contributor; confidence: high; commits: 49cc59b1e88e, b81cc598bfc7; files: src/agents/sandbox/runtime-status.ts, src/agents/tool-policy-audit.ts)
vincentkoc: Introduced the tool-policy audit and sandbox policy-block behavior from which the affected shared output path derives. (role: introduced behavior; confidence: high; commits: 09dd051e7813; files: src/agents/sandbox/runtime-status.ts, src/agents/tool-policy-audit.ts)
mushuiyu886: Recently hardened redacted sandbox session-key rendering for UTF-16-safe boundaries in the same formatter and tests. (role: recent adjacent contributor; confidence: high; commits: b714fdb3ae47; files: src/agents/sandbox/runtime-status.ts, src/agents/sandbox/tool-policy.test.ts)
lsr911: Previously landed UTF-16-safe truncation maintenance in the shared tool-policy audit helper, establishing relevant merged history beyond authoring this PR. (role: recent adjacent contributor; confidence: high; commits: 472e5167d38a; files: src/agents/tool-policy-audit.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:35:43.187Z sha ed5d522 :: needs real behavior proof before merge. :: [P2] Escape C1 characters in the displayed session label

…h paths The prior change extended hasUnsafeControlChars (which gates the explain command to an agent-only fallback) through the C1 range. But the parallel visible Session-label path went through escapeControlCharsVisible, which only escaped C0/DEL, so a C1 byte (e.g. U+009B CSI) in a redacted session key still reached the single-line Session output raw. Extend the shared escapeControlCharsVisible contract through C1 (0x80-0x9f) so the visible label escapes those bytes too, and add an end-to-end regression in sandbox-explain.test.ts asserting a C1-bearing session key both escapes in the Session line and triggers the agent-only explain fallback. Signed-off-by: lsr911 <[email protected]>

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

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.

lsr911 requested a review from a team as a code owner July 10, 2026 04:46

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

lsr911 mentioned this pull request Jul 10, 2026

fix(agents): detect C1 control characters in attachment name validation #103382

Closed

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

clawsweeper Bot mentioned this pull request Jul 10, 2026

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

Closed

openclaw-barnacle Bot added docker Docker and sandbox tooling triage: needs-pr-context Candidate: external PR body lacks required problem context or evidence. labels Jul 11, 2026

clawsweeper Bot added rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. and removed rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. labels Jul 11, 2026

lsr911 mentioned this pull request Jul 11, 2026

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(sandbox): detect C1 control characters in hasUnsafeControlChars#103381

fix(sandbox): detect C1 control characters in hasUnsafeControlChars#103381
lsr911 wants to merge 2 commits into
openclaw:mainfrom
lsr911:fix/runtime-status-c1-control-chars

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