Skip to content

refactor: leftover changes from #1428#1430

Merged
pi0 merged 12 commits into
mainfrom
refactor/1428-leftovers
Jul 3, 2026
Merged

refactor: leftover changes from #1428#1430
pi0 merged 12 commits into
mainfrom
refactor/1428-leftovers

Conversation

@pi0

@pi0 pi0 commented Jul 3, 2026

Copy link
Copy Markdown
Member

cont #1428

Summary by CodeRabbit

  • Bug Fixes
    • Improved handling of encoded characters in URL paths, especially around %25 and slash-like encodings.
    • Fixed leading-slash normalization so paths are consistently handled with a single leading slash.
    • Added coverage to prevent regressions when serving static files with encoded percent signs in their names.

pi0 and others added 12 commits July 3, 2026 10:03
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]>
@coderabbitai

coderabbitai Bot commented Jul 3, 2026

Copy link
Copy Markdown

Review Change Stack

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: c222aa88-925f-4238-9c9d-774dc6904f21

📥 Commits

Reviewing files that changed from the base of the PR and between 30ca780 and 6bb5592.

📒 Files selected for processing (3)
  • src/utils/internal/path.ts
  • src/utils/path.ts
  • test/static.test.ts

📝 Walkthrough

Walkthrough

Removes the unused withLeadingSlash helper from internal path utilities, rewrites decodeSlashes documentation, refactors ENCODED_SEP_RE_G to derive from ENCODED_SEP_RE, simplifies leading-slash normalization logic, and adds a regression test verifying preserved percent-encoded filenames in static serving.

Changes

Path helper cleanup and normalization fix

Layer / File(s) Summary
resolveDotSegments docs, regex, and normalization update
src/utils/path.ts
Documentation for decodeSlashes is clarified, ENCODED_SEP_RE_G is now derived from ENCODED_SEP_RE, and the leading-slash collapse in the return path is simplified to an unconditional replace.
Removal of unused helper
src/utils/internal/path.ts
Deletes the exported withLeadingSlash function, which is no longer used.
Static serving regression test
test/static.test.ts
Adds a test asserting that /50%25.png requests keep the encoded %25 in the served id rather than decoding to /50%.png.

Estimated code review effort: 1 (Trivial) | ~5 minutes

Poem

A slash too many, hopped away,
One helper gone, cleaner today.
The percent sign stays just as it should,
%25 preserved, all is good.
thump thump — tests confirm the code's true state! 🐰

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch refactor/1428-leftovers

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.

@pi0 pi0 changed the title Refactor/1428 leftovers refactor: leftover changes from #1428 Jul 3, 2026
@pi0
pi0 merged commit 86bb3e4 into main Jul 3, 2026
7 of 9 checks passed
@pi0
pi0 deleted the refactor/1428-leftovers branch July 3, 2026 20:59
@codecov

codecov Bot commented Jul 3, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

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.

1 participant