fix(agents): detect C1 control characters in attachment name validation by lsr911 · Pull Request #103382 · openclaw/openclaw

lsr911 · 2026-07-10T04:47:30Z

What Problem This Solves

validateAttachmentName() in src/agents/subagent-attachments.ts rejects unsafe sessions_spawn attachment names. It checked for control characters but only covered the C0 range (0x00-0x1f) and DEL (0x7f), missing the C1 control range (0x80-0x9f). The C1 range includes the CSI introducer U+009B, an alternative ANSI escape prefix equivalent to ESC [.

Two issues, both addressed here:

Detection gap — a name carrying a C1 byte was accepted.
Diagnostic leak (raised by ClawSweeper) — the rejection error attachments_invalid_name (${name}) interpolated the raw untrusted name, so even after rejection the same control bytes were re-introduced into an error string that can reach a terminal or log.

Change

Extend the control-character predicate to cover 0x80-0x9f (C1).
Add describeRejectedName() which escapes C0/DEL/C1 to visible \xNN sequences and caps the name by code point, and use it in every attachments_invalid_name (...) diagnostic. The rejected name is now safe to display and never carries raw control bytes.

Testing / Proof

Added src/agents/subagent-attachments.test.ts driving the real exported resolveAcpSessionsSpawnImageAttachments path:

a C1-bearing name (bad<U+009B>name.png) returns status: "error" with attachments_invalid_name, the byte rendered as escaped \x9b, and no raw C1 byte in the returned error;
every byte across the full 0x80-0x9f range is rejected with no raw byte leaking;
an ordinary image attachment (photo.png) still succeeds (status: "ok").

$ node scripts/run-vitest.mjs run src/agents/subagent-attachments.test.ts

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

 Test Files  1 passed (1)
      Tests  3 passed (3)
   Duration  1.64s

The test exercises the real validation + diagnostic path, so the passing run is the before/after proof: on current main the C1 name is accepted and, once other checks reject it, the raw byte is echoed back in the error; with this change the name is rejected and the diagnostic is escaped.

Attachment name validation 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, openclaw#103381. Co-Authored-By: Claude <[email protected]>

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

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

Summary
The PR extends attachment-name validation to reject C1 control characters and safely escape rejected names in diagnostics, with focused regression tests.

PR surface: Source +16, Tests +76. Total +92 across 2 files.

Reproducibility: yes. from source inspection: current main accepts C1 characters in attachment names and directly reflects rejected names into returned diagnostics. A live current-main invocation was not performed 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:

Post redacted terminal output or logs from a real sessions_spawn call showing \x9b in the rejection and a normal attachment succeeding.

Proof guidance:

[P1] Needs real behavior proof before merge: The PR shows focused Vitest output but no after-fix real sessions_spawn invocation with redacted terminal or log evidence; add that proof, update the PR body, and request @clawsweeper re-review if a fresh review does not trigger automatically.

Risk before merge

[P1] Test-only evidence does not establish how the escaped diagnostic appears through an actual sessions_spawn invocation and its terminal or log surface.

Maintainer options:

Decide the mitigation before merge
Attachment validation should reject every terminal control range and return bounded escaped diagnostics while preserving ordinary Unicode and valid attachment flows.
Pause or close
Do not merge this PR until maintainers decide whether the risk is worth taking.

Next step before merge

[P1] Keep the PR human-gated until the contributor provides the required real sessions_spawn behavior proof; no additional code repair is currently identified.

Security
Cleared: The patch reduces terminal-control exposure from untrusted attachment names and adds no dependency, permission, secret, or supply-chain surface.

Review details

Best possible solution:

Attachment validation should reject every terminal control range and return bounded escaped diagnostics while preserving ordinary Unicode and valid attachment flows.

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

Yes, from source inspection: current main accepts C1 characters in attachment names and directly reflects rejected names into returned diagnostics. A live current-main invocation was not performed during this read-only review.

Is this the best way to solve the issue?

Yes. The patch stays within the attachment validation boundary, rejects the missing control range, safely renders rejected names, and preserves the successful ordinary-name path.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 20fe3f46dbbe.

Label changes

Label justifications:

P2: The PR fixes a bounded unsafe attachment-name validation and diagnostic handling gap with limited blast radius.
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 shows focused Vitest output but no after-fix real sessions_spawn invocation with redacted terminal or log evidence; add that proof, update the PR body, and request @clawsweeper re-review if a fresh review does not trigger automatically.

Evidence reviewed

PR surface:

Source +16, Tests +76. Total +92 across 2 files.

View PR surface stats

Area	Files	Added	Removed	Net
Source	1	20	4	+16
Tests	1	76	0	+76
Docs	0	0	0	0
Config	0	0	0	0
Generated	0	0	0	0
Other	0	0	0	0
Total	2	96	4	+92

What I checked:

Current-main reproduction: Current main rejects C0 and DEL characters but omits the C1 range, and its invalid-name branches interpolate the untrusted name directly into the returned diagnostic. (src/agents/subagent-attachments.ts:155, 20fe3f46dbbe)
Proposed implementation: The PR adds bounded diagnostic rendering that escapes C0, DEL, and C1 code points, then uses it for every attachments_invalid_name branch while extending validation through 0x9f. (src/agents/subagent-attachments.ts:160, e8ab8a4ca873)
Regression coverage: The added tests exercise the exported ACP attachment resolver, verify rejection and non-reflection across the full C1 range, and retain a successful ordinary-name case. (src/agents/subagent-attachments.test.ts:19, e8ab8a4ca873)
Prior finding resolved: The second commit replaces the raw rejected-name interpolation identified in the previous review and adds focused regression coverage; the prior blocker should not be raised again. (src/agents/subagent-attachments.ts:176, e8ab8a4ca873)
Current-module provenance: Git history traces the current extracted attachment service and its validation path to the spawned-metadata and attachments refactor. (src/agents/subagent-attachments.ts:151, eed403dc74b5)
Feature provenance: The attachment validation boundary derives from the merged sessions_spawn inline attachment feature, which introduced filename checks and was merged through the original feature PR. (a9f118878542)

Likely related people:

steipete: Authored the refactor that extracted the current attachment service and its validation path, making this the strongest current-code routing signal. (role: major refactor author; confidence: high; commits: eed403dc74b5; files: src/agents/subagent-attachments.ts)
napetrov: Authored the merged sessions_spawn inline attachment feature that introduced attachment filename validation and its security boundary. (role: introduced behavior; confidence: high; commits: a9f118878542; files: src/agents/tools/sessions-spawn-tool.ts, src/agents/subagent-attachments.ts)
tyler6204: Merged the original sessions_spawn attachment feature and is a relevant reviewer for follow-up changes to that behavior. (role: merger; confidence: medium; commits: a9f118878542; files: src/agents/tools/sessions-spawn-tool.ts, src/agents/subagent-attachments.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:48.725Z sha 00d7401 :: needs real behavior proof before merge. :: [P2] Do not reflect the rejected C1 name into the error

…test Besides detecting C1 controls (0x80-0x9f) in attachment names, the rejection diagnostic (attachments_invalid_name) interpolated the raw untrusted name, re-introducing the same control bytes (e.g. U+009B CSI) into an error string that can reach a terminal. Escape C0/DEL/C1 to visible \xNN and cap the name by code point before embedding it. Add a regression test driving the real resolveAcpSessionsSpawnImageAttachments path: a C1-bearing name is rejected with the byte escaped (never raw) across the full 0x80-0x9f range, and an ordinary image name still succeeds. Signed-off-by: lsr911 <[email protected]>

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

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 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(doctor): strip C1 control characters from doctor error messages #103383

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 size: S triage: needs-pr-context Candidate: external PR body lacks required problem context or evidence. and removed size: XS 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 attachment name validation#103382

fix(agents): detect C1 control characters in attachment name validation#103382
lsr911 wants to merge 2 commits into
openclaw:mainfrom
lsr911:fix/subagent-attachments-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