refactor: leftover changes from #1428#1430
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]>
- 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]>
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]>
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughRemoves the unused ChangesPath helper cleanup and normalization fix
Estimated code review effort: 1 (Trivial) | ~5 minutes Poem
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
cont #1428
Summary by CodeRabbit
%25and slash-like encodings.