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: needs changes before merge. Reviewed August 1, 2026, 8:15 PM ET / August 2, 2026, 00:15 UTC.

ClawSweeper review

What this changes

The PR changes sanitizeUntrustedFileName() so Windows reserved device basenames such as CON and NUL.txt return the caller-provided fallback, with tests and changelog coverage.

Merge readiness

⛔ Blocked by patch quality or review findings - 8 items remain

This PR has a concrete P1 compatibility regression: the released filename-sanitizer contract deliberately preserves Windows reserved basenames and documents caller-side opt-in rejection, while the branch silently changes all callers to receive the fallback. The previous finding remains unresolved on the same PR head, so this should stay open for a narrow repair or explicit maintainer approval of a breaking contract change.

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

Review scores

Measure	Result	What it means
Overall readiness	🦪 silver shellfish (2/6)	The behavior proof is useful, but the patch conflicts with an explicit released compatibility contract and has a P1 blocker.
Proof confidence	🐚 platinum hermit (4/6)	Sufficient (live_output): The PR body includes after-fix built-package terminal output for reserved and non-reserved names plus focused and full-check results; no sensitive information is shown.
Patch quality	🦪 silver shellfish (2/6)	1 actionable review finding remain.

Verification

Check	Result	Evidence
Real behavior	Verified	Sufficient (live_output): The PR body includes after-fix built-package terminal output for reserved and non-reserved names plus focused and full-check results; no sensitive information is shown.
Evidence reviewed	5 items	Released contract excludes this behavior: The current documentation explicitly says the sanitizer deliberately does not reject Windows reserved names and supplies a caller-side opt-in rejection example; v0.5.0 contains the same text. Branch changes the documented output contract: The PR adds a reserved-device check that returns fallbackName, changing existing callers that currently receive the sanitized reserved basename. Current-main ownership and contract provenance: Blame attributes both the sanitizer implementation and the deliberate opt-in policy documentation to the v0.5.0 release commit by Peter Steinberger.
Findings	1 actionable finding	[P1] Preserve the documented opt-in reserved-name policy
Security	None	None.

How this fits together

sanitizeUntrustedFileName() is the package’s lightweight portable-basename filter for untrusted names. Staging and external-output helpers consume its result before creating or replacing files, so its output policy is a public compatibility contract.

flowchart LR
  A[Untrusted filename] --> B[Filename sanitizer]
  B --> C[Portable basename policy]
  C --> D[Staging and output helpers]
  D --> E[Filesystem creation or replacement]
  F[Caller-specific strict policy] --> C

Loading

Decision needed

Question	Recommendation
Should sanitizeUntrustedFileName() retain its released default of preserving reserved Windows basenames, or should the package intentionally make rejection the new default?	Retain the documented default: Remove the fallback change and keep reserved-name rejection as caller-side policy, preserving existing output behavior.

Why: The branch reverses an explicit v0.5.0 public contract and changes output for all existing callers; choosing a permanent default is product and compatibility policy, not a mechanical security repair.

Before merge

• Preserve the documented opt-in reserved-name policy (P1) - The released contract explicitly says this sanitizer does not reject CON/NUL-style names and provides a caller-side opt-in pattern in docs/filename.md. Returning the fallback here silently changes outputs for every existing caller and staging helper; retain the default or obtain explicit approval for a documented compatibility change. This is the unresolved prior finding on the unchanged head.
• Resolve merge risk (P1) - Merging changes observable output for existing callers that intentionally preserve reserved basenames, despite the released documentation promising that behavior.
• Resolve merge risk (P1) - The branch remains incompatible with the current public documentation and its caller-side strict-policy example.
• Resolve merge risk (P1) - The PR is currently dirty against main, so it needs a refreshed merge result after the compatibility decision.
• Complete next step (P2) - The P1 blocker has a narrow mechanical repair: restore the released default behavior and remove the conflicting tests and changelog entry; any new strict API remains subject to maintainer direction.
• Improve patch quality - Resolve the default-policy decision by restoring the documented behavior or obtaining explicit approval for a breaking change.
• Improve patch quality - Rebase onto current main and rerun the focused filename check and pnpm check after the policy change.

Findings

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

Agent review details

Security

None.

Review metrics

Metric	Value	Why it matters
Patch scope	4 files affected; 30 additions, 0 deletions	A small patch changes a shared public output contract used by staging and external-output flows.
Behavior coverage	6 reserved-name fallbacks and 2 non-reserved controls added	The tests clearly establish the intended new behavior, which is exactly the compatibility behavior requiring approval.

Merge-risk options

Maintainer options:

1. Restore the released sanitizer contract (recommended)
   Remove the unconditional reserved-name fallback, restore the existing behavior tests and changelog wording, then rebase and rerun the focused and full checks.
2. Approve and document a breaking contract change
   If maintainers want rejection by default, update the public contract and provide explicit upgrade guidance before merging.
3. Pause the branch for API direction
   Close or pause this implementation until maintainers decide whether an opt-in strict sanitizer belongs in the public API.

Copy recommended automerge instruction

@clawsweeper automerge

Special instructions:
Restore the documented opt-in reserved-name policy, remove the default fallback behavior and conflicting tests/changelog entry, then rebase against current main and run the focused filename test plus pnpm check.

Technical review

Best possible solution:

Keep the existing narrow sanitizer default and its documented caller-side strict pattern; if the package should offer reserved-name rejection, add it as an explicitly chosen opt-in API or separately approved major compatibility change.

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

Yes for the compatibility regression: on current main, sanitizeUntrustedFileName("CON", "fallback.bin") returns CON, as v0.5.0 documentation explicitly requires; the branch changes it to fallback.bin. The contributor’s built-package sample also demonstrates the proposed behavior.

Is this the best way to solve the issue?

No; unconditionally changing the default is not the best path because the released contract deliberately leaves this policy to callers. Preserve that default or add an explicitly opt-in strict API after maintainer direction.

Full review comments:

• [P1] Preserve the documented opt-in reserved-name policy — src/filename.ts:32
  The released contract explicitly says this sanitizer does not reject CON/NUL-style names and provides a caller-side opt-in pattern in docs/filename.md. Returning the fallback here silently changes outputs for every existing caller and staging helper; retain the default or obtain explicit approval for a documented compatibility change. This is the unresolved prior finding on the unchanged head.
  Confidence: 0.99

Overall correctness: patch is incorrect
Overall confidence: 0.99

AGENTS.md: found and applied where relevant.

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

Labels

Label changes:

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

Label justifications:

• P2: This is a bounded but user-visible filename-sanitization compatibility change affecting shared output helpers.
• merge-risk: 🚨 compatibility: The branch changes a documented return-value policy for existing sanitizer callers without an opt-in or migration path.
• rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🐚 platinum hermit and patch quality is 🦪 silver shellfish.
• status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action. Sufficient (live_output): The PR body includes after-fix built-package terminal output for reserved and non-reserved names plus focused and full-check results; no sensitive information is shown.
• proof: sufficient: Contributor real behavior proof is sufficient. The PR body includes after-fix built-package terminal output for reserved and non-reserved names plus focused and full-check results; no sensitive information is shown.

Evidence

Acceptance criteria:

• [P1] pnpm exec vitest run test/filename.test.ts.
• [P1] pnpm check.
• [P1] git diff --check.

What I checked:

• Released contract excludes this behavior: The current documentation explicitly says the sanitizer deliberately does not reject Windows reserved names and supplies a caller-side opt-in rejection example; v0.5.0 contains the same text. (docs/filename.md:48, 66201c1f347a)
• Branch changes the documented output contract: The PR adds a reserved-device check that returns fallbackName, changing existing callers that currently receive the sanitized reserved basename. (src/filename.ts:32, c47b4e59718f)
• Current-main ownership and contract provenance: Blame attributes both the sanitizer implementation and the deliberate opt-in policy documentation to the v0.5.0 release commit by Peter Steinberger. (docs/filename.md:48, 66201c1f347a)
• Related device-read guard is a separate boundary: The reserved-name guard originated in the merged unsafe-device-read work; reusing its matcher does not establish that the filename sanitizer should change its independently documented semantics. (src/device-path.ts:139, 5bf301406851)
• Real-behavior proof supplied by contributor: The PR body reports a built-package runtime sample showing reserved names fall back and ordinary names remain unchanged, alongside focused tests and a full check. (test/filename.test.ts:24, c47b4e59718f)

Likely related people:

• Peter Steinberger: Current implementation and the explicit reserved-name opt-in documentation blame to the v0.5.0 release commit; he also authored the recent staging/output work that consumes the sanitizer. (role: current contract author and recent adjacent contributor; confidence: high; commits: 66201c1f347a, 66e59e0ce95e; files: src/filename.ts, docs/filename.md)
• Alex Knight: Authored the merged Windows reserved-device read guard whose internal matcher this PR exposes and reuses. (role: related device-path guard author; confidence: high; commits: 5bf301406851; files: src/device-path.ts)

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 (18 earlier review cycles; latest 8 shown)

• 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
• reviewed 2026-08-01T11:54:30.630Z sha c47b4e5 :: found issues before merge. :: [P1] Preserve the documented opt-in reserved-name policy
• reviewed 2026-08-01T20:14:46.036Z sha c47b4e5 :: found issues before merge. :: [P1] Preserve the documented opt-in reserved-name policy
• reviewed 2026-08-01T22:37:23.927Z sha c47b4e5 :: found issues before merge. :: [P1] Preserve the documented opt-in reserved-name policy

[steipete]

Maintainer-side reproduction supports CLOSE for the current proposal.

On current main (ab933820c089a6fc6991ed4d80ce0a931ce70567), docs/filename.md explicitly defines the released contract: sanitizeUntrustedFileName() deliberately does not reject Windows reserved names, and callers that need that stricter policy layer it on top. After a real build, the exported behavior is:

CON -> CON
nul.txt -> nul.txt
console.txt -> console.txt

On this PR's exact head (c47b4e59718f1eacd2b26645a3445d02c80b372d), the focused filename test passes (4/4) and the JavaScript emitted by the attempted branch build demonstrates the intended reversal:

CON -> fallback.bin
nul.txt -> fallback.bin
console.txt -> console.txt

That is not a bug repair within the existing contract; it is a global compatibility-policy change for every sanitizer and staging/output caller. The existing documented caller-side opt-in is the appropriate boundary unless a separately approved API/default redesign is undertaken.

The branch is also stale/dirty against current main. A frozen offline install on this head reverted to its older nine-workspace graph and pnpm build failed at src/archive-tar-runtime.ts because the tar module was unavailable, so the exact head is not a current buildable candidate.

No contributor revision is requested: the recommendation is to close this implementation and retain the documented default.

[steipete]

Closing because the proposed default conflicts with the documented sanitizeUntrustedFileName contract: Windows reserved-basename policy remains an explicit caller policy rather than a silent universal sanitizer default. Full reproduction and contract rationale: #67 (comment)

[steipete]

The contract discussion here led to an approved redesign. It has now been implemented fresh on top of current main—not by reviving this branch—with underscore suffixing that preserves case and extensions (CON → CON_, nul.txt → nul_.txt) and uses the complete shared device-name set on every platform.

Commit: 5ca756d

Review PR: #83

Thanks @SebTardif for prompting the contract discussion and the original portability idea.