fix(agents): detect C1 control characters in hasControlCharacter by lsr911 · Pull Request #103379 · openclaw/openclaw

lsr911 · 2026-07-10T04:45:41Z

What Problem This Solves

buildProviderReauthCommand() in src/agents/failover-error.ts builds the copy-pasteable operator command shown in a failover remediation hint (Re-authenticate with: openclaw models auth login --provider '<provider>' --force). Before rendering, it rejects provider ids that contain control characters via hasControlCharacter().

That guard only covered the C0 range (0x00-0x1f) and DEL (0x7f); it did not cover the C1 control range (0x80-0x9f). The C1 range includes the CSI introducer U+009B, an alternative ANSI escape prefix equivalent to ESC [. A provider id carrying a C1 byte therefore passed the guard and was rendered inside the reauthentication command string, so the C1 escape could reach a terminal that later prints/copies that remediation hint.

Scope note: this change hardens the generated provider reauth command specifically. It does not claim to filter all failover error display text — only the buildProviderReauthCommand output.

Change

Extend hasControlCharacter() to also flag 0x80-0x9f:

if (code < 0x20 || code === 0x7f || (code >= 0x80 && code <= 0x9f)) {
  return true;
}

When the provider id contains any C1 byte, buildProviderReauthCommand now returns undefined (no command rendered), matching the existing behavior for C0/DEL. Same C1 completion as #103226 (sanitizeForConsole) and #103274 (renderTerminalBufferText).

Testing / Proof

Extended the existing buildProviderReauthCommand coverage in src/agents/failover-error.test.ts with a focused C1 case: a provider id containing U+009B (and every byte across 0x80-0x9f) yields undefined, while a clean provider id still renders the expected command.

$ node scripts/run-vitest.mjs run src/agents/failover-error.test.ts

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

 Test Files  1 passed (1)
      Tests  98 passed (98)
   Duration  6.10s

The test calls the real exported buildProviderReauthCommand, so the passing run is the before/after proof: on current main buildProviderReauthCommand("anthropic" + U+009B + "[2J") renders a command with the raw C1 byte embedded in the shell literal; with this change it returns undefined.

hasControlCharacter() checked for C0 control characters (0x00-0x1f) and DEL (0x7f) but did not check for C1 controls (0x80-0x9f). The C1 range includes the CSI introducer (0x9b) and other control codes that should not appear in user-facing error text. Add the C1 range to the check. Same pattern as the C1 fixes in sanitizeForConsole (openclaw#103226) and renderTerminalBufferText (openclaw#103274). Co-Authored-By: Claude <[email protected]>

clawsweeper · 2026-07-10T10:36:17Z

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

Summary
The PR extends failover provider reauthentication-command validation to reject C1 control characters and adds focused regression coverage.

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

Reproducibility: yes. Current main deterministically accepts U+009B in buildProviderReauthCommand(), and its production caller appends the generated remediation hint to the final fallback error.

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:

Post redacted terminal, copied live output, or logs showing a C1-bearing provider omits the remediation hint while a clean provider still renders; updating the PR body should trigger review, or a maintainer can comment @clawsweeper re-review.

Proof guidance:

[P1] Needs real behavior proof before merge: The PR provides a focused test transcript against the real helper, but no after-fix terminal, copied live output, or redacted log from the operator-visible fallback path; private data should be redacted before posting proof. 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.

Risk before merge

[P2] No after-fix evidence yet shows the unsafe remediation hint is omitted in the actual operator-visible fallback error while an ordinary provider still renders normally.

Maintainer options:

Decide the mitigation before merge
Keep the narrow C1 rejection and regression test, then add redacted terminal, copied live output, or logs from the fallback path proving unsafe-hint suppression and normal-provider rendering.
Pause or close
Do not merge this PR until maintainers decide whether the risk is worth taking.

Next step before merge

[P1] Keep this PR open for contributor-supplied real behavior proof; no discrete code repair remains for ClawSweeper automation.

Security
Cleared: The patch tightens a terminal-facing validation boundary and introduces no dependency, workflow, permission, secret, download, or package-resolution change.

Review details

Best possible solution:

Keep the narrow C1 rejection and regression test, then add redacted terminal, copied live output, or logs from the fallback path proving unsafe-hint suppression and normal-provider rendering.

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

Yes. Current main deterministically accepts U+009B in buildProviderReauthCommand(), and its production caller appends the generated remediation hint to the final fallback error.

Is this the best way to solve the issue?

Yes. Extending the existing private fail-closed predicate is the narrowest maintainable fix and avoids changing unrelated text-sanitization contracts.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 81ce206ee84f.

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 is a narrow hardening fix for uncommon terminal controls in an operator-facing authentication remediation command.
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 provides a focused test transcript against the real helper, but no after-fix terminal, copied live output, or redacted log from the operator-visible fallback path; private data should be redacted before posting proof. 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 0, Tests +15. Total +15 across 2 files.

View PR surface stats

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

What I checked:

Current-main vulnerable path: The command helper rejects C0 and DEL but accepts C1 code units, and the resulting remediation hint is appended directly to the final fallback summary. (src/agents/failover-error.ts:686, 81ce206ee84f)
Focused repair: The branch extends the existing fail-closed predicate through U+009F without changing normal provider command formatting. (src/agents/failover-error.ts:689, 6e43dd4b4566)
Regression coverage: The updated head tests U+009B, every C1 code point, and the unchanged clean-provider command through the real exported helper. (src/agents/failover-error.test.ts:1439, 015d1dbf4a4b)
Sibling policy: The merged terminal-buffer hardening recognizes C1 CSI as a terminal control boundary, but it covers a distinct rendering surface and does not supersede this command-generation fix. (packages/terminal-core/src/ansi.ts:2, a3f9f3567f71)
Proof gate: The latest exact-head Real behavior proof check failed; the PR body supplies a unit-test transcript but no terminal output, copied live output, or redacted log from the operator-visible fallback path. (015d1dbf4a4b)
Current-main status: Current main still has the C0/DEL-only predicate, so this work remains necessary and is not implemented or superseded. (src/agents/failover-error.ts:689, 81ce206ee84f)

Likely related people:

MertBasar0: Commit 0ff5fe3 introduced force re-login recovery and fallback auth skips in the central failover files, making this a strong feature-history routing signal. (role: introduced adjacent auth-recovery behavior; confidence: high; commits: 0ff5fe3a80ce; files: src/agents/failover-error.ts, src/agents/failover-error.test.ts)
steipete: Peter Steinberger has several recent commits on failover classification and tests, including documentation and centralization work in the same files. (role: recent area contributor; confidence: high; commits: fbf3e009d49e, 2cd0c8b515d8, 0a8c98e7cbeb; files: src/agents/failover-error.ts, src/agents/failover-error.test.ts)
SunnyShu0925: The most recent pre-PR change to both central failover files adjusted classification behavior and regression coverage. (role: recent area contributor; confidence: medium; commits: 1690afe304ea; files: src/agents/failover-error.ts, src/agents/failover-error.test.ts)
lsr911: Beyond this proposal, lsr911 authored the merged sibling C1 terminal-buffer hardening, providing current history with the same terminal-control class. (role: recent adjacent contributor; confidence: medium; commits: a3f9f3567f71; files: src/gateway/terminal/buffer-text.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:34:18.702Z sha 6e43dd4 :: needs real behavior proof before merge. :: none

Add a focused regression case asserting that a provider id containing any C1 byte (0x80-0x9f), including the U+009B CSI introducer, is rejected so no reauthentication command with an embedded C1 escape is rendered, while a clean provider id still produces the expected command. Signed-off-by: lsr911 <[email protected]>

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

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.

openclaw-barnacle Bot added agents Agent runtime and tooling size: XS labels 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 the triage: needs-pr-context Candidate: external PR body lacks required problem context or evidence. label 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(agents): detect C1 control characters in hasControlCharacter#103379

fix(agents): detect C1 control characters in hasControlCharacter#103379
lsr911 wants to merge 2 commits into
openclaw:mainfrom
lsr911:fix/failover-error-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