FileSystemRouter: fix slice panic in URLPath::parse on leading '?' or empty-decoding path#32669
FileSystemRouter: fix slice panic in URLPath::parse on leading '?' or empty-decoding path#32669robobun wants to merge 1 commit into
Conversation
… empty-decoding path
URLPath::parse assumed the decoded pathname was non-empty and had a
leading byte to skip. Two user-reachable inputs violate that:
router.match("?") -> question_mark_i=0 -> &decoded[1..0]
router.match("%PUBLIC_URL%") -> decoded becomes "" -> &decoded[1..]
Both produce a slice-bounds panic that takes down the process. The
existing empty/'/' guard in filesystem_router.rs runs before decoding
and does not catch the leading-'?' case, so request-derived strings
passed to router.match() could trigger this.
Fix by normalising an empty decoded pathname to '/' and clamping the
start index of the path / first_segment slices to the end index so a
bare query string yields an empty path instead of a reversed range.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Walkthrough
ChangesURLPath::parse degenerate input fixes
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — minimal, well-scoped bounds fix for a real slice-panic crash vector, with a regression test that follows the established subprocess pattern.
Extended reasoning...
Overview
This PR fixes two slice-bounds panics in URLPath::parse (src/http_types/URLPath.rs) that FileSystemRouter.match() could trigger on degenerate inputs: a path beginning with ? (producing a reversed [1..0] range) and an input like %PUBLIC_URL% that the fault-tolerant percent decoder consumes to an empty string (producing [1..] on a zero-length slice). The fix is two small, defensive changes: normalise an empty decoded pathname to b"/" before scanning, and clamp the slice start index to 1.min(end) for both the path and first_segment slices. A new subprocess-based regression test in filesystem_router.test.ts exercises all four shapes and asserts they resolve to the index route with the correct query.
Security risks
The bug being fixed is itself the security concern — request-derived strings reaching router.match() could crash the process (DoS). The fix introduces no new attack surface: it only narrows existing slice bounds and substitutes a static b"/" for the empty case. Setting decoded_storage = None when falling back to the static literal is correct for the lifetime-laundering scheme in this file, since all resulting field slices then point into 'static memory rather than the discarded decode buffer.
Level of scrutiny
Moderate. This is request-path parsing in core Rust, but the change is purely mechanical bounds-clamping with no control-flow or semantic changes for well-formed inputs (when path_end >= 1 and first_segment_end_u >= 1, 1.min(x) == 1 and behaviour is identical to before). I traced each of the four test inputs through the new code and confirmed no remaining out-of-range slices and that pathname/path/query_string end up with the values the test asserts.
Other factors
The test mirrors the existing subprocess pattern used by several neighbouring tests in the same file (tempDir + Bun.spawn + JSON over stdout), so a regression would surface as a nonzero exit rather than killing the runner. No prior reviewer comments to address; the bug-hunting pass found nothing.
1 participant
What
URLPath::parsepanics on two classes of input thatFileSystemRouter.match()forwards from user code:Since applications pass request-derived strings to
router.match(), this is a remote process-crash vector.Cause
parse()assumes the decoded pathname is non-empty and has a leading byte to strip:?givesquestion_mark_i == 0, so the path slice is[1..0].%PUBLIC_URL%is consumed entirely by the fault-tolerant percent decoder (the create-react-app placeholder special case), leavingdecoded_pathnameempty, so[1..]on a zero-length slice panics.The empty/
"/"guard infilesystem_router.rsruns on the raw input before decoding and does not catch either shape.Fix
In
URLPath::parse:"/"before scanning.1.min(end)..end) so a bare query string yields an empty path rather than a reversed range.Both inputs now resolve to the index route with the query string preserved.
Verification
New test in
test/js/bun/util/filesystem_router.test.tsspawns a subprocess, callsmatch()with"?","?foo=bar","%PUBLIC_URL%", and"%PUBLIC_URL%?x=1", and asserts each returns the index route with the expected query. Before this change the subprocess aborts with the slice-bounds panic above; after, all four resolve cleanly.