fix(agents): detect C1 control characters in sanitizeModelWarningValue#103380
fix(agents): detect C1 control characters in sanitizeModelWarningValue#103380lsr911 wants to merge 2 commits into
Conversation
sanitizeModelWarningValue() detected C0 control characters but not C1 controls (0x80-0x9f). Add the C1 range to the control boundary detection. Same pattern as openclaw#103226, openclaw#103274, openclaw#103379. Co-Authored-By: Claude <[email protected]>
openclaw-barnacle
Bot
added
agents
openclaw-barnacle
Bot
removed
the
triage: needs-pr-context
clawsweeper
Bot
added
rating: 🧂 unranked krab
|
Codex review: needs real behavior proof before merge. Reviewed July 11, 2026, 4:46 AM ET / 08:46 UTC. Summary PR surface: Source 0, Tests +51. Total +51 across 2 files. Reproducibility: yes. at source level with high confidence: U+0080 survives ANSI stripping, current main misses it as the truncation boundary, and the shared log sanitizer removes the byte while retaining the following suffix. No real OpenClaw runtime reproduction has been provided. Review metrics: none identified. Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Proof guidance:
Risk before merge
Maintainer options:
Next step before merge
Security Review detailsBest possible solution: Keep the narrow C1 boundary extension and regression, then require a redacted terminal or runtime-log capture showing a real providerless-model warning drops the suffix after U+0080 before merge. Do we have a high-confidence way to reproduce the issue? Yes at source level with high confidence: U+0080 survives ANSI stripping, current main misses it as the truncation boundary, and the shared log sanitizer removes the byte while retaining the following suffix. No real OpenClaw runtime reproduction has been provided. Is this the best way to solve the issue? Yes. Extending the existing first-control boundary through C1 is the narrowest maintainable fix, preserves model resolution, and the revised head directly covers the previously untested behavior. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against b01affeaee7f. Label changesLabel justifications:
Evidence reviewedPR surface: Source 0, Tests +51. Total +51 across 2 files. View PR surface stats
What I checked:
Likely related people:
What the crustacean ranks mean
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
Review history (1 earlier review cycle)
|
Add a warning-level regression driving the real resolveConfiguredModelRef
providerless-model path: a value with a residual C1 control byte (U+0080)
followed by visible text truncates the logged warning at the C1 boundary
("gpt4") instead of retaining the attacker-controlled suffix, and a clean
value is preserved. Locks down the new C1 branch in sanitizeModelWarningValue.
Signed-off-by: lsr911 <[email protected]>
openclaw-barnacle
Bot
added
size: S
triage: needs-pr-context
|
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.
1 participant
What Problem This Solves
sanitizeModelWarningValue()insrc/agents/model-selection-shared.tsbuilds the value shown in the "providerless model" warning log (Model "<value>" specified without provider. Falling back to ...). It truncates the value at the first control character so attacker-controlled text after a control byte can't ride along into the log. The truncation boundary only covered the C0 range (0x00-0x1f) and DEL (0x7f), not the C1 control range (0x80-0x9f). C1 includes the CSI introducer U+009B (an alternative ANSI escape prefix equivalent toESC [).A residual C1 byte — one that survives the preceding
stripAnsi()pass, e.g. U+0080 — was therefore not treated as a truncation boundary:stripAnsi/sanitizeForLogremoved the byte itself but the visible suffix after it was retained in the warning.Change
Add
(code >= 0x80 && code <= 0x9f)to the boundary predicate so a residual C1 byte truncates the warning value at that position.Testing / Proof
Added
src/agents/model-selection-shared.test.tsdriving the real exportedresolveConfiguredModelRefproviderless-model path (logger mocked) withgpt4<U+0080>EVIL, asserting the warning truncates toModel "gpt4" ...and drops theEVILsuffix (and carries no raw C1 byte).Before/after on the exact same test, toggling only the one-line predicate:
Before (current
main, boundary = C0/DEL only): test fails —The residual C1 byte is dropped but the attacker suffix
EVILis retained.After (this change, boundary includes C1): test passes —
The warning is truncated to
Model "gpt4" ...; the suffix after the C1 byte is dropped.