feat: export resolveDotSegments as a public path utility#1428
Conversation
Downstream tools that do their own prefix/scope matching against `event.url.pathname` (e.g. nitro's route-rules) need the same `.`/`..` resolution and encoded-dot-segment handling `serveStatic` already relies on internally, but had no way to reuse it since it lived under `utils/internal/path.ts`. Move it to a public `utils/path.ts` module and add an opt-in `decodeSlashes` option for callers that need to anticipate a downstream `%2f`/`%5c` decode for security scope checks — left off by default since decoding path separators changes segment count, and therefore route dispatch. Co-Authored-By: Claude Sonnet 5 <[email protected]>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe path resolver is moved into a public utility module, re-exported at the package entrypoint, wired into static serving, and covered by new unit tests, export snapshot updates, and generated docs. ChangesresolveDotSegments relocation and enhancement
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
test/unit/path.test.ts (1)
1-62: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winGood coverage; consider adding regression tests for the fast-path/root-guard edge cases.
Once the backslash-only fast-path and relative-path root-guard behaviors in
src/utils/path.tsare addressed, add cases likeresolveDotSegments("/a\\b")(no dots) and a relative-path traversal ("a/../b") to lock in the fix.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/unit/path.test.ts` around lines 1 - 62, Add regression tests in resolveDotSegments to cover the backslash-only fast path and the relative-path root-guard behavior. Update the existing describe("resolveDotSegments") suite in the path test file to include a case like resolveDotSegments("/a\\b") that stays unchanged when there are no dot segments, and a relative traversal case like resolveDotSegments("a/../b") that should not escape above the starting point. Use the resolveDotSegments symbol to keep the tests aligned with the implementation in src/utils/path.ts.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/utils/path.ts`:
- Around line 36-44: The fast-path in path normalization is skipping literal
backslashes, so inputs like "/a\\b" return before normalization and violate the
contract in the path utility. Update the early-return condition in
src/utils/path.ts so the function still falls through to the replaceAll("\\",
"/") normalization whenever a backslash is present, while preserving the
existing dot/%2 and decodeSlashes handling in the path normalization logic.
- Around line 45-58: The path normalization logic in src/utils/path.ts assumes a
leading slash by using resolved.length > 1 as the root guard, which breaks
relative inputs in the segment loop. Update the normalization in the
path-resolving function to explicitly distinguish absolute versus relative
paths, so only the real root of an absolute path is protected while relative
paths like a/../b can collapse correctly. If the API is meant to accept only
absolute paths, add a clear doc note on the path utility or its public entry
point to state that expectation.
---
Nitpick comments:
In `@test/unit/path.test.ts`:
- Around line 1-62: Add regression tests in resolveDotSegments to cover the
backslash-only fast path and the relative-path root-guard behavior. Update the
existing describe("resolveDotSegments") suite in the path test file to include a
case like resolveDotSegments("/a\\b") that stays unchanged when there are no dot
segments, and a relative traversal case like resolveDotSegments("a/../b") that
should not escape above the starting point. Use the resolveDotSegments symbol to
keep the tests aligned with the implementation in src/utils/path.ts.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fb24243f-0a03-4fbe-b4ac-acdcead80330
📒 Files selected for processing (6)
src/index.tssrc/utils/internal/path.tssrc/utils/path.tssrc/utils/static.tstest/unit/package.test.tstest/unit/path.test.ts
💤 Files with no reviewable changes (1)
- src/utils/internal/path.ts
- Collapse leading slashes so a decoded/normalized separator can never produce a protocol-relative (`//host`) result (open-redirect/SSRF). - Root paths up front so the `..` clamp is well-defined for relative inputs (`a/../../b` -> `/b` instead of `a/b`). - Never skip `\`->`/` normalization on the fast path, matching the documented invariant. - Collapse the two `%2f`/`%5c` replaces into one pass gated on actual presence; skip backslash replace when absent. - Return `/` consistently for empty/consumed inputs and document the single decode level of `decodeSlashes`. Co-Authored-By: Claude Fable 5 <[email protected]>
- Add an automd:jsdocs block for src/utils/path.ts under the Security page so the new public util appears on the docs site like its peers. - Add a TODO(perf) noting the coarse fast-path guard for a future, benchmarked follow-up. Co-Authored-By: Claude Fable 5 <[email protected]>
Strict single-level decoding made the decodeSlashes contract unsound: the JSDoc told callers to re-run resolveDotSegments until stable to handle double-encoding, but %252f is a fixed point on the first call (the function never decodes %25), so a scope check following the docs verbatim stayed bypassable by a downstream that decodes more than once. Decode the three path-special escapes (%2e, %2f, %5c) at any %25-nesting depth instead. Fail-closed: other escapes are still never decoded. Co-Authored-By: Claude Fable 5 <[email protected]>
resolveDotSegments now guarantees a single-rooted result for any input, and URL pathnames always start with /, so the wrapper was a no-op. Co-Authored-By: Claude Fable 5 <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/utils/static.ts`:
- Line 87: serveStatic is decoding the request pathname twice, which can turn
double-encoded traversal sequences into dot segments before resolveDotSegments()
validates them. Update the pathname handling in static.ts so the event layer
performs the single required decode and the originalId computation uses the
already-decoded value, removing the extra decodeURI() call from the serveStatic
path flow.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1fb16c11-003a-477b-9feb-c933e10b8267
📒 Files selected for processing (4)
docs/2.utils/4.security.mdsrc/utils/path.tssrc/utils/static.tstest/unit/path.test.ts
✅ Files skipped from review due to trivial changes (1)
- docs/2.utils/4.security.md
🚧 Files skipped from review as they are similar to previous changes (1)
- src/utils/path.ts
Make explicit the two properties a downstream scope/route-match consumer (e.g. nitro route-rules) relies on: everything except `.`/`..` and the documented decodes is left byte-for-byte intact so the result stays in the same representation as an un-decoded pathname, and interior empty segments are preserved so exact prefix matching must normalize both sides. Co-Authored-By: Claude Fable 5 <[email protected]>
Each fast/slow pair is length- and segment-matched, differing only in the char class that flips the fast-path guard (`.`/`%`/`\`), so the reported multiplier is pure slow-path overhead rather than a string-length artifact. Covers the TODO(perf) case (a dotted filename with no real dot segment still takes the slow path) plus literal/encoded/backslash/decodeSlashes triggers, with correctness + equal-length assertions guarding each pair. Run: node --expose-gc --allow-natives-syntax ./test/bench/path.ts Co-Authored-By: Claude Fable 5 <[email protected]>
The coarse `path.includes(".") || path.includes("%2")` guard sent every
dotted filename (`app.js`) and every `%2x` escape (`%20`) down the slow
split/normalize/join loop even with no real dot segment — and real static
assets almost all contain a `.`, so the fast path essentially never fired.
Replace it with a boundary-aware regex that matches only a whole `.`/`..`
segment (literal or `%2e`-encoded at any `%25` depth), plus the existing
precise backslash / encoded-separator checks. Also skip the per-segment
`%2e` decode for `%`-free segments. Behavior is identical: any input without
a dot segment, backslash, or encoded separator was already returned
unchanged by the old slow path.
Realistic serveStatic paths drop ~7-9x (/index.html ~268ns -> ~38ns,
app.[hash].js ~347ns -> ~40ns); genuine traversal still resolves and stays
on the slow path. Benchmark updated to a fast-path group (regression guard)
and length-matched slow-path pairs.
Co-Authored-By: Claude Fable 5 <[email protected]>
serveStatic ran decodeURI on event.url.pathname, which the event layer already decoded once (decodePathname, preserving %25). The extra decode peeled a second %25 level, so a double-encoded sequence resolved to a different id than the rest of h3 routes on — e.g. /files/a%252fb was served as /files/a%2fb instead of the dispatched /files/a%252fb. resolveDotSegments now decodes nested-encoded dot segments itself (%(?:25)*2e), so the extra decodeURI is redundant for traversal safety. Drop it and resolve dot segments on the already-canonical pathname. Reported by CodeRabbit on #1428. Co-Authored-By: Claude Fable 5 <[email protected]>
Summary
resolveDotSegmentsout ofsrc/utils/internal/path.tsinto a new publicsrc/utils/path.ts, exported fromsrc/index.ts(serveStaticalready used it internally to resolve./..and encoded dot segments before touching disk).decodeSlashesoption that also collapses encoded path separators into real/boundaries — off by default, since decoding a path separator changes segment count and therefore which route matches, not just security scope.%2e,%2f,%5c) is pessimistic: nested%25-encodings (%252f,%25252f, …) collapse too, so a downstream that decodes any number of times cannot smuggle a separator or dot segment past a scope check. All other escapes (%20,%C3%A9, …) are never decoded.serveStaticbehavior is unchanged for normal request paths; a few hostile/edge inputs are now hardened (in the fail-closed direction) and can produce a different computed asset id: backslash-only paths (/a\b→/a/b), extra leading separators (//foo→/foo), and multi-encoded dot segments now resolve as traversal instead of passing through opaquely. Also drops a now-redundantwithLeadingSlashwrapper inserveStatic.Why
Downstream tools that do their own prefix/scope matching against
event.url.pathname(e.g. nitro's route-rules, which independently matches rules like/secure/**against a path and needs to guard against an encoded separator like%2fbypassing a narrower rule that a downstream proxy target/redirect would later decode) need the exact same./..resolution + encoded-dot-segment handling h3 already has and battle-tested forserveStatic. There was previously no way to reuse it since it lived underutils/internal/, so nitro had to reimplement the same tricky, security-sensitive logic from scratch (nitrojs/nitro#4396). This exports the single hardened implementation so it can be reused directly instead of re-derived per consumer.%2fdecoding stays opt-in and separate from routing/dispatch: it must never be baked intoevent.url.pathnameglobally, since some apps intentionally rely on%2Fto keep an id containing a literal slash as one opaque path segment (e.g./files/:id). ThedecodeSlashesoption lets a caller ask for the "canonical path a downstream will eventually see" purely for an out-of-band scope check, without affecting what h3 itself routes on.Nested
%25-encodings are collapsed (rather than decoding strictly once) because a strict single-level decode makes the scope-check use case unsound:/allowed/..%252f..%252fadminwould look safely under/allowedwhile a downstream that decodes twice sees/admin. SincedecodeSlashesexists precisely to anticipate downstream decodes, the resolver handles arbitrary decode depth itself instead of pushing a fixpoint loop onto every security-sensitive caller.Test plan
pnpm test— full suite (lint + typecheck + coverage) passes,src/utils/path.tsat 100% coveragetest/unit/path.test.tscovers: plain paths, literal./.., root-clamping,%2edecoding, backslash normalization, default opaque%2f/%5c, thedecodeSlashesoption (including traversal revealed only after decoding separators), and nested%25-encoded separators/dots at multiple depthstest/unit/package.test.tsexport snapshot🤖 Generated with Claude Code
Summary by CodeRabbit
resolveDotSegmentsto safely normalize./.., normalize backslashes, and decode percent-encoded dot segments.decodeSlashesoption to also decode percent-encoded path separators when needed./and avoiding protocol-relative outputs.resolveDotSegmentsanddecodeSlashesin the security utilities guide.