Skip to content

feat: export resolveDotSegments as a public path utility#1428

Merged
pi0 merged 9 commits into
mainfrom
feat/resolve-dot-segments-public
Jul 3, 2026
Merged

feat: export resolveDotSegments as a public path utility#1428
pi0 merged 9 commits into
mainfrom
feat/resolve-dot-segments-public

Conversation

@pi0x

@pi0x pi0x commented Jul 3, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Moves resolveDotSegments out of src/utils/internal/path.ts into a new public src/utils/path.ts, exported from src/index.ts (serveStatic already used it internally to resolve ./.. and encoded dot segments before touching disk).
  • Adds an opt-in decodeSlashes option 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.
  • Decoding of the three path-special escapes (%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.
  • serveStatic behavior 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-redundant withLeadingSlash wrapper in serveStatic.

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 %2f bypassing 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 for serveStatic. There was previously no way to reuse it since it lived under utils/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.

%2f decoding stays opt-in and separate from routing/dispatch: it must never be baked into event.url.pathname globally, since some apps intentionally rely on %2F to keep an id containing a literal slash as one opaque path segment (e.g. /files/:id). The decodeSlashes option 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..%252fadmin would look safely under /allowed while a downstream that decodes twice sees /admin. Since decodeSlashes exists 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.ts at 100% coverage
  • test/unit/path.test.ts covers: plain paths, literal ./.., root-clamping, %2e decoding, backslash normalization, default opaque %2f/%5c, the decodeSlashes option (including traversal revealed only after decoding separators), and nested %25-encoded separators/dots at multiple depths
  • Updated test/unit/package.test.ts export snapshot

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features
    • Added a new public path utility resolveDotSegments to safely normalize ./.., normalize backslashes, and decode percent-encoded dot segments.
    • Added decodeSlashes option to also decode percent-encoded path separators when needed.
  • Bug Fixes
    • Improved static file URL handling by resolving dot segments after URL decoding, preventing traversal above / and avoiding protocol-relative outputs.
  • Documentation
    • Documented resolveDotSegments and decodeSlashes in the security utilities guide.
  • Tests
    • Expanded unit tests for normalization, encoded edge cases, and clamping/traversal hardening.

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]>
@pi0x
pi0x requested a review from pi0 as a code owner July 3, 2026 10:06
@coderabbitai

coderabbitai Bot commented Jul 3, 2026

Copy link
Copy Markdown

Review Change Stack

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

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

Changes

resolveDotSegments relocation and enhancement

Layer / File(s) Summary
Path resolver implementation
src/utils/path.ts, src/utils/internal/path.ts
Introduces ResolveDotSegmentsOptions and resolveDotSegments in src/utils/path.ts, with decodePathname remaining exported in src/utils/internal/path.ts and documented there.
Public export and static wiring
src/index.ts, src/utils/static.ts
Re-exports ResolveDotSegmentsOptions and resolveDotSegments from src/index.ts, updates src/utils/static.ts to import resolveDotSegments from src/utils/path.ts, and changes serveStatic to resolve the normalized pathname directly.
Validation and documentation
test/unit/package.test.ts, test/unit/path.test.ts, docs/2.utils/4.security.md
Extends the package export snapshot, adds unit coverage for resolver behavior and security cases, and adds the generated path documentation section.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Suggested reviewers: pi0

Poem

A bunny hopped through paths so neat,
Where dots and slashes found their seat.
%2e now knows its place,
And roots stay home in every case.
Hop hop—public paths take flight,
Clean and clear, and bounded right. 🐇

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main change: making resolveDotSegments a public path utility.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/resolve-dot-segments-public

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (1)
test/unit/path.test.ts (1)

1-62: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Good 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.ts are addressed, add cases like resolveDotSegments("/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

📥 Commits

Reviewing files that changed from the base of the PR and between dae12fe and 4ca9e38.

📒 Files selected for processing (6)
  • src/index.ts
  • src/utils/internal/path.ts
  • src/utils/path.ts
  • src/utils/static.ts
  • test/unit/package.test.ts
  • test/unit/path.test.ts
💤 Files with no reviewable changes (1)
  • src/utils/internal/path.ts

Comment thread src/utils/path.ts Outdated
Comment thread src/utils/path.ts Outdated
pi0 and others added 4 commits July 3, 2026 10:22
- 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]>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between 3bea522 and 407696d.

📒 Files selected for processing (4)
  • docs/2.utils/4.security.md
  • src/utils/path.ts
  • src/utils/static.ts
  • test/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

Comment thread src/utils/static.ts Outdated
pi0 and others added 4 commits July 3, 2026 11:12
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]>
@pi0
pi0 merged commit 30ca780 into main Jul 3, 2026
9 checks passed
@pi0
pi0 deleted the feat/resolve-dot-segments-public branch July 3, 2026 14:50
pi0x pushed a commit that referenced this pull request Jul 3, 2026
pi0 added a commit that referenced this pull request Jul 3, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants