fix(path): reject Windows reserved basenames in sanitizeUntrustedFileName

[SebTardif]

What Problem This Solves

Fixes an issue where consumers using sanitizeUntrustedFileName() (via staging, external output, and sibling-temp helpers) would accept Windows reserved device basenames such as CON, NUL.txt, or COM1 from untrusted input. On Windows those names open devices rather than ordinary files, which breaks portable name sanitization already used for C0/C1 and Windows-invalid characters on every host.

Why This Change Was Made

Reuse the reserved-name set and normalization already used by the device-path read guards (isWindowsReservedDeviceBaseName) inside sanitizeUntrustedFileName(). Reserved basenames (including extensions and trailing spaces/dots) return the caller-supplied fallback. Non-goals: changing path resolution or read guards; only the untrusted basename sanitizer.

User Impact

Callers that pass reserved basenames now receive the same fallback they already get for empty, ., or .. names. Valid names such as console.txt and null.pdf are unchanged. No public API signature changes.

Evidence

Red / green

• Red: pnpm exec vitest run test/filename.test.ts failed with expected 'CON' to be 'fallback.bin' before the production change.
• Green: same file, 4/4 tests pass after the fix; full pnpm check (598 passed, 22 skipped).

Runtime sample (built dist/)

"CON" -> fallback.bin
"nul.txt" -> fallback.bin
"COM1" -> fallback.bin
"LPT9.doc" -> fallback.bin
"aux " -> fallback.bin
"console.txt" -> console.txt
"report.pdf" -> report.pdf

Checklist

• Tests added or updated when behavior changed
• Security and compatibility impact considered
• CHANGELOG.md updated when release-relevant
• No credentials, private paths, private hosts, or sensitive contents included

Related

• Same-repo reserved-name read guards: #22
• Hardened staging / portable basename sanitization: #62
• Node.js path reserved-name edge cases: nodejs/node#64266

Real behavior proof

• Behavior or issue addressed: Untrusted basenames that are Windows reserved devices must not pass through sanitizeUntrustedFileName; they must fall back.

• Real environment tested: macOS arm64, Node from repo toolchain, @openclaw/fs-safe branch fix/windows-reserved-basenames after pnpm build.

• Exact steps or command run after this patch:

  pnpm install --frozen-lockfile
  pnpm exec vitest run test/filename.test.ts
  pnpm build
  node --input-type=module -e "import { sanitizeUntrustedFileName } from './dist/filename.js';
  for (const c of ['CON','nul.txt','console.txt']) console.log(c, '->', sanitizeUntrustedFileName(c, 'fallback.bin'));"
  pnpm check

• Evidence after fix: reserved cases map to fallback.bin; non-reserved console.txt kept; full check green.

• Observed result after fix: sanitizeUntrustedFileName('CON', 'fallback.bin') === 'fallback.bin' and siblings; staging helpers inherit the safer basename policy.

• What was not tested: Live Windows NTFS create of reserved names (behavior is documented OS semantics; this PR blocks the name before open).

[clawsweeper]

Codex review: found issues before merge. Reviewed August 1, 2026, 7:54 AM ET / 11:54 UTC.

ClawSweeper review

What this changes

The PR makes sanitizeUntrustedFileName() return its fallback for Windows reserved device basenames, reusing the device-path normalization logic and adding focused tests and changelog text.

Merge readiness

⚠️ Needs maintainer review before merge - 5 items remain

Keep this PR open for a maintainer compatibility decision. Its runtime proof is strong, but the proposed fallback behavior reverses a released, explicitly documented opt-in policy for sanitizeUntrustedFileName(); the prior P1 blocker remains on unchanged branch code.

Priority: P2
Reviewed head: c47b4e59718f1eacd2b26645a3445d02c80b372d
Owner decision: Required. See Decision needed.

Review scores

Measure	Result	What it means
Overall readiness	🦐 gold shrimp (3/6)	The PR has strong real-behavior proof, but its implementation remains blocked by an unresolved released-contract compatibility change.
Proof confidence	🦞 diamond lobster (5/6)	Sufficient (terminal): The PR body provides post-build terminal output for reserved and ordinary names, plus focused-test and full-check results; the proof directly demonstrates the proposed behavior.
Patch quality	🦐 gold shrimp (3/6)	1 actionable review finding remain.

Verification

Check	Result	Evidence
Real behavior	Verified	Sufficient (terminal): The PR body provides post-build terminal output for reserved and ordinary names, plus focused-test and full-check results; the proof directly demonstrates the proposed behavior.
Evidence reviewed	6 items	Released public contract: The filename documentation explicitly says the sanitizer does not reject Windows reserved names and gives callers a separate strict-layer example, establishing that preserved reserved-looking names are intentional API behavior. Current-main implementation: Current main returns the cleaned basename unless it is empty, . or ..; it has no reserved-device fallback branch, matching the documented narrow contract. Proposed compatibility change: The PR adds a branch that returns fallbackName for reserved basenames, changing existing output values rather than only hardening a lower-level file-open boundary.
Findings	1 actionable finding	[P1] Preserve the documented opt-in reserved-name policy
Security	None	None.

How this fits together

sanitizeUntrustedFileName() converts attacker-controlled filenames into one portable path segment before staging and external-output helpers form filesystem destinations. Its public output is consumed by those downstream helpers, so changing the sanitizer changes caller-visible behavior on every host.

flowchart LR
  A[Untrusted basename] --> B[Public filename sanitizer]
  B --> C{Reserved-name policy}
  C --> D[Sanitized or fallback basename]
  D --> E[Staging and output helpers]
  E --> F[Guarded filesystem destination]

Loading

Decision needed

Question	Recommendation
Should the released default of sanitizeUntrustedFileName() begin rejecting Windows reserved basenames despite documentation that intentionally leaves this as caller-selected strict handling?	Preserve the documented default: Keep reserved-name rejection caller-selected through the existing layering pattern or a future explicit strict API.

Why: The safety goal is valid, but selecting a cross-platform breaking output change for a public package contract requires maintainer intent rather than an automated repair decision.

Before merge

• Preserve the documented opt-in reserved-name policy (P1) - This returns fallbackName for values the released documentation explicitly says this deliberately narrow API retains, and whose stricter handling it shows callers how to layer themselves. That changes existing outputs for all hosts and downstream staging callers; retain the default or obtain explicit approval for a documented compatibility change before merging.
• Resolve merge risk (P2) - Merging changes a released public API result: callers on every host that currently retain names such as CON or NUL.txt would instead receive their fallback name.
• Resolve merge risk (P1) - The branch is not cleanly mergeable against current main, whose sanitizer implementation was rewritten after the PR base; any approved direction needs a rebase and refreshed validation.
• Complete next step (P2) - A maintainer must choose the public compatibility policy before a safe implementation path can be selected.

Findings

• [P1] Preserve the documented opt-in reserved-name policy — src/filename.ts:32-33

Agent review details

Security

None.

Review metrics

Metric	Value	Why it matters
Patch scope	4 files changed; 30 additions, 0 deletions	A small patch alters a released helper, its tests, and release-facing changelog text.
Known consumers	3 downstream helper modules	Current source uses the sanitizer in sibling temporary-file and external-output flows, expanding the compatibility surface beyond direct callers.

Merge-risk options

Maintainer options:

1. Keep the current default (recommended)
   Revise or replace the patch so the existing sanitizer output remains unchanged and strict reserved-name handling stays explicit at the caller boundary.
2. Approve the breaking behavior
   A maintainer may accept the fallback result as a documented public contract change, then require a rebase and upgrade-focused validation.
3. Pause the proposal
   Defer or close this PR if maintainers do not want to add an opt-in strict surface or change the released default.

Technical review

Best possible solution:

Keep the documented default sanitizer behavior and use the existing caller-layered reserved-name check; if maintainers want built-in strict handling, design it as an explicit opt-in or deliberately versioned compatibility change with both default and strict-path coverage.

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

Not applicable as a current-main bug: the reported desired behavior is deliberately excluded by the released documentation, while the PR body convincingly demonstrates the proposed new behavior after its patch.

Is this the best way to solve the issue?

No. The patch is a reasonable strict-policy implementation but not the best default solution while the public documentation explicitly directs consumers to layer this decision themselves; an opt-in or explicit maintainer-approved compatibility change is safer.

Full review comments:

• [P1] Preserve the documented opt-in reserved-name policy — src/filename.ts:32-33
  This returns fallbackName for values the released documentation explicitly says this deliberately narrow API retains, and whose stricter handling it shows callers how to layer themselves. That changes existing outputs for all hosts and downstream staging callers; retain the default or obtain explicit approval for a documented compatibility change before merging.
  Confidence: 0.98

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 ab933820c089.

Labels

Label justifications:

• P2: This is a bounded but public filesystem-name compatibility decision with concrete downstream consumers.
• merge-risk: 🚨 compatibility: The patch converts formerly retained, documented filename outputs into caller fallback values on all platforms.
• rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🦞 diamond lobster and patch quality is 🦐 gold shrimp.
• status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action. Sufficient (terminal): The PR body provides post-build terminal output for reserved and ordinary names, plus focused-test and full-check results; the proof directly demonstrates the proposed behavior.
• proof: sufficient: Contributor real behavior proof is sufficient. The PR body provides post-build terminal output for reserved and ordinary names, plus focused-test and full-check results; the proof directly demonstrates the proposed behavior.

Evidence

What I checked:

• Released public contract: The filename documentation explicitly says the sanitizer does not reject Windows reserved names and gives callers a separate strict-layer example, establishing that preserved reserved-looking names are intentional API behavior. (docs/filename.md:48, 66201c1f347a)
• Current-main implementation: Current main returns the cleaned basename unless it is empty, . or ..; it has no reserved-device fallback branch, matching the documented narrow contract. (src/filename.ts:25, ab933820c089)
• Proposed compatibility change: The PR adds a branch that returns fallbackName for reserved basenames, changing existing output values rather than only hardening a lower-level file-open boundary. (src/filename.ts:32, c47b4e59718f)
• Policy provenance: History attributes the reserved-name opt-in documentation and layered strict example to the v0.5.0 release commit, so this is a recent released contract rather than an undocumented omission. (docs/filename.md:48, 66201c1f347a)
• Release provenance: The commit containing the documented contract is included in the shipped v0.5.0 tag. (CHANGELOG.md:1, 66201c1f347a)
• Downstream reach: Current main routes sanitizer output into sibling temporary-file and external-output naming helpers, so the output change affects more than one direct caller. (src/output.ts:27, ab933820c089)

Likely related people:

• Peter Steinberger: The v0.5.0 commit introduced the explicit reserved-name opt-in documentation and shipped that public contract. (role: documented-policy and release author; confidence: high; commits: 66201c1f347a; files: docs/filename.md, CHANGELOG.md)
• Vincent Koc: Authored the current-main linear-time sanitizer rewrite, making this person a relevant routing candidate for implementation compatibility after policy is decided. (role: recent sanitizer contributor; confidence: high; commits: ab933820c089; files: src/filename.ts)

Rank-up moves

Optional improvements that raise the rating; they are not merge blockers.

• Obtain explicit maintainer approval for the changed default, or revise the design to preserve the documented default.
• Rebase on current main and rerun focused and full validation after the policy decision.

Rating scale

Score	Internal tier	Crab rank	Meaning
6/6	S	🦀 challenger crab	Exceptional readiness
5/6	A	🦞 diamond lobster	Very strong readiness
4/6	B	🐚 platinum hermit	Good normal PR; ordinary maintainer review
3/6	C	🦐 gold shrimp	Useful, but confidence is limited
2/6	D	🦪 silver shellfish	Proof or implementation needs work
1/6	F	🧂 unranked krab	Not merge-ready
N/A	NA	🌊 off-meta tidepool	Rating does not apply

Overall follows the weaker of proof and patch quality.
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.

Workflow

• 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.

History

Review history (15 earlier review cycles; latest 8 shown)

• reviewed 2026-07-31T12:29:28.573Z sha c47b4e5 :: needs maintainer review before merge. :: none
• reviewed 2026-07-31T15:21:41.080Z sha c47b4e5 :: needs maintainer review before merge. :: none
• reviewed 2026-07-31T17:20:20.385Z sha c47b4e5 :: found issues before merge. :: [P1] Preserve the documented opt-in reserved-name policy
• reviewed 2026-07-31T19:12:07.573Z sha c47b4e5 :: found issues before merge. :: [P1] Preserve the documented opt-in reserved-name policy
• reviewed 2026-07-31T22:38:59.727Z sha c47b4e5 :: found issues before merge. :: [P1] Preserve the documented opt-in reserved-name policy
• reviewed 2026-07-31T23:50:55.733Z sha c47b4e5 :: found issues before merge. :: [P1] Preserve the documented opt-in reserved-name policy
• reviewed 2026-08-01T05:08:32.373Z sha c47b4e5 :: found issues before merge. :: [P1] Preserve the documented opt-in reserved-name policy
• reviewed 2026-08-01T07:59:14.595Z sha c47b4e5 :: found issues before merge. :: [P1] Preserve the documented opt-in reserved-name policy