Skip to content

fix(path): keep sanitizers responsive on adversarial input - #75

Merged
vincentkoc merged 1 commit into
mainfrom
fix/codeql-redos-linear-scans-20260728
Jul 28, 2026
Merged

fix(path): keep sanitizers responsive on adversarial input#75
vincentkoc merged 1 commit into
mainfrom
fix/codeql-redos-linear-scans-20260728

Conversation

@vincentkoc

Copy link
Copy Markdown
Member

What Problem This Solves

Fixes an issue where consumers passing long attacker-controlled path segments, temp-name inputs, or Windows device paths could trigger excessive CPU use in sanitizer regexes.

Why This Change Was Made

Backtracking-prone edge and suffix regexes are replaced with bounded character scans while preserving existing path validation, fallback behavior, and Windows device detection. A dedicated regression suite covers the flagged input shapes.

User Impact

Path and temp-name sanitization remains responsive for very long hostile inputs without changing the public API or accepted output shapes.

Evidence

  • focused test run: 61 tests passed

  • pnpm build

  • file-size lint

  • git diff --check

  • autoreview: clean, no accepted/actionable findings

  • 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

@clawsweeper clawsweeper Bot added rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. P2 Normal priority bug or improvement with limited blast radius. merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. labels Jul 28, 2026
@clawsweeper

clawsweeper Bot commented Jul 28, 2026

Copy link
Copy Markdown

Codex review: needs maintainer review before merge. Reviewed July 28, 2026, 2:45 AM ET / 06:45 UTC.

ClawSweeper review

What this changes

This PR replaces backtracking-prone cleanup regexes in path-segment, temporary-name, and Windows device-path sanitizers with bounded character scans, adds long hostile-input regression tests, and records the security hardening in the changelog.

Merge readiness

⚠️ Ready for maintainer review - 2 items remain

Keep this PR open for explicit maintainer review: it is a member-authored, security-sensitive change to public path-sanitization behavior. The proposed bounded scans and focused adversarial-input coverage are directionally appropriate, and no concrete patch defect is established from the available review evidence.

Priority: P2
Reviewed head: b5515eb6cbbf5409b16ab65a0209dd28edc4f0c4

Review scores

Measure Result What it means
Overall readiness 🐚 platinum hermit (4/6) The patch is focused, adds targeted regression coverage, and has no concrete correctness finding from the available evidence, while still needing normal maintainer review of security-contract equivalence.
Proof confidence 🌊 off-meta tidepool Not applicable: This member-authored PR is not subject to the external-contributor real-behavior-proof gate; its supplied focused tests and CI remain useful supplemental validation.
Patch quality 🐚 platinum hermit (4/6) No actionable review findings were identified.

Verification

Check Result Evidence
Real behavior Not applicable Not applicable: This member-authored PR is not subject to the external-contributor real-behavior-proof gate; its supplied focused tests and CI remain useful supplemental validation.
Evidence reviewed 3 items Proposed security hardening: The PR replaces edge and suffix regex cleanup in three production sanitizers with explicit character scans, and adds a 100,000-character regression suite for Windows separators, hyphens, prefixes, and extension candidates.
Current release predates the proposal: The provided repository context identifies v0.5.0 at 66201c1 as the latest release, while this PR head is b5515eb; the proposed hardening is not established as released or already implemented on current main.
Repository policy applies: The fully provided AGENTS.md classifies path normalization and Windows device handling as security-sensitive public contracts and requires preserving fail-closed behavior plus focused regression coverage.
Findings None None.
Security None None.

How this fits together

@openclaw/fs-safe accepts caller-controlled path-like strings and converts them into validated safe names before callers use guarded filesystem primitives. The changed helpers feed temporary-target generation and Windows device-path rejection, so their normalization behavior is part of the package's security and compatibility boundary.

flowchart LR
  A[Caller-controlled path input] --> B[Path segment sanitizer]
  A --> C[Temporary name builder]
  A --> D[Windows device-path check]
  B --> E[Validated safe name]
  C --> E
  D --> F[Allow or reject device target]
  E --> G[Guarded filesystem operation]
  F --> G
Loading

Before merge

  • Resolve merge risk (P2) - These helpers are public compatibility and security boundaries: a subtle deviation from the prior regex semantics could change fallback selection, temp-file extension handling, or Windows reserved-device detection for existing callers.
  • Complete next step (P2) - The PR is member-authored and changes security-sensitive public sanitizer semantics, so it should remain open for maintainer review rather than enter an automated repair lane.
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Changed safety boundaries 3 production modules changed The patch alters each of the public path-segment, temporary-name, and Windows device sanitization paths.
Adversarial regression coverage 1 new suite, 4 focused cases The added tests target the long-input shapes named in the PR rather than only ordinary naming behavior.

Merge-risk options

Maintainer options:

  1. Confirm compatibility before merging (recommended)
    Review the changed normalization paths against prior outputs for fallback names, extensions, and Windows device aliases, then merge if the behavioral contract remains intact.
  2. Pause for narrower security review
    Pause this PR if maintainers cannot establish semantic equivalence, then split or revise the affected sanitizer with focused compatibility cases.

Technical review

Best possible solution:

Land the linear scans only after maintainer review confirms equivalence with the existing sanitizer contract across ordinary inputs and adversarial boundary cases, retaining the new regression coverage as protection against excessive CPU use.

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

Yes, at source level: the PR supplies focused long-input cases for each claimed regex hotspot, but this read-only review did not independently execute the timing tests against a pre-fix revision.

Is this the best way to solve the issue?

Yes in principle: replacing vulnerable edge/suffix matching with bounded scans is the narrowest maintainable ReDoS-hardening approach, provided maintainers confirm the scans preserve the existing public normalization contract.

AGENTS.md: found and applied where relevant.

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

Labels

Label changes:

  • add P2: This is a bounded security-hardening change to path sanitization with meaningful but not demonstrated emergency user impact.
  • add merge-risk: 🚨 compatibility: Existing users may depend on exact sanitizer output, fallback behavior, extension extraction, and Windows reserved-name handling.
  • add merge-risk: 🚨 security-boundary: The diff changes classification and normalization of attacker-controlled path-like inputs at a filesystem safety boundary.
  • add rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🌊 off-meta tidepool and patch quality is 🐚 platinum hermit.
  • add status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Not applicable: This member-authored PR is not subject to the external-contributor real-behavior-proof gate; its supplied focused tests and CI remain useful supplemental validation.

Label justifications:

  • P2: This is a bounded security-hardening change to path sanitization with meaningful but not demonstrated emergency user impact.
  • merge-risk: 🚨 compatibility: Existing users may depend on exact sanitizer output, fallback behavior, extension extraction, and Windows reserved-name handling.
  • merge-risk: 🚨 security-boundary: The diff changes classification and normalization of attacker-controlled path-like inputs at a filesystem safety boundary.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🌊 off-meta tidepool and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Not applicable: This member-authored PR is not subject to the external-contributor real-behavior-proof gate; its supplied focused tests and CI remain useful supplemental validation.

Evidence

What I checked:

  • Proposed security hardening: The PR replaces edge and suffix regex cleanup in three production sanitizers with explicit character scans, and adds a 100,000-character regression suite for Windows separators, hyphens, prefixes, and extension candidates. (test/codeql-redos-regression.test.ts:9, b5515eb6cbbf)
  • Current release predates the proposal: The provided repository context identifies v0.5.0 at 66201c1 as the latest release, while this PR head is b5515eb; the proposed hardening is not established as released or already implemented on current main. (CHANGELOG.md:3, b5515eb6cbbf)
  • Repository policy applies: The fully provided AGENTS.md classifies path normalization and Windows device handling as security-sensitive public contracts and requires preserving fail-closed behavior plus focused regression coverage. (AGENTS.md:1, 0206c4b567b4)

Likely related people:

  • vincentkoc: The available PR metadata identifies this repository member as the author of the current sanitizer hardening commit; no deeper current-main ownership trail was available in the supplied review evidence. (role: recent area contributor; confidence: low; commits: b5515eb6cbbf; files: src/device-path.ts, src/safe-path-segment.ts, src/temp-target.ts)

Rank-up moves

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

  • Review prior and proposed sanitizer outputs for fallback, extension, and Windows reserved-device edge cases before merge.

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.

@vincentkoc

Copy link
Copy Markdown
Member Author

Maintainer review completed against the prior regex semantics. I ran a deterministic differential check over 50,000 randomized inputs for each changed boundary: path-segment sanitization (both dot-prefix modes), temporary prefix/extension output, and Windows device classification, plus a curated reserved-device corpus. Old and new outputs matched in every case. Existing CI is green across Node 22/24, macOS/Linux/Windows, native, package smoke, coverage, clippy/audit, and workflow lint.

@vincentkoc
vincentkoc marked this pull request as ready for review July 28, 2026 06:54
@vincentkoc
vincentkoc requested a review from a team as a code owner July 28, 2026 06:54
@vincentkoc
vincentkoc merged commit ab93382 into main Jul 28, 2026
24 checks passed
@vincentkoc
vincentkoc deleted the fix/codeql-redos-linear-scans-20260728 branch July 28, 2026 06:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. P2 Normal priority bug or improvement with limited blast radius. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant