cherry-pick: Fix static plugin serving nothing when urlPath is configured (#1584 → v5.1)#1731
Conversation
* Fix static plugin serving nothing when urlPath is configured (#1583) Two stacked defects: the Scope server proxy passed the raw config value ('assets') as the route mount, which can never match leading-slash pathnames; and the routing chain strips the mount prefix from req.pathname while the static maps were keyed by the full entry URL path. normalizeUrlPath now ensures a leading slash, the Scope proxy resolves urlPath via resolveBaseURLPath (same normalization as the entry pipeline), and static.ts keys its maps relative to the mount base captured at registration. urlPath runtime changes now request a restart (the route mount cannot be re-registered live), and the mount root disambiguates the no-slash form via the unstripped request so the root index 301s to the trailing-slash path. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Expose originalPathname from stripPrefix for runtime-agnostic mount-root disambiguation Codex review: BunRequest sets _nodeRequest to null, so the mount-root redirect silently never fired on Bun. stripPrefix now exposes the unstripped pathname as an explicit property instead of handlers reaching into runtime internals. Verified on both runtimes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Preserve query strings on static trailing-slash redirects Gemini review: both 301 Location headers dropped the original query string. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * perf(static): hoist query-string parse into redirect branches Keep the common index-serve path (200, no redirect) allocation-free by computing queryIndex/query lazily inside each 301 branch instead of unconditionally on every index request. Static serving is in the request hot path. Follow-up to review on #1584. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(lint): import from node:assert, not node:assert/strict no-restricted-imports bans the node:assert/strict subpath; node:assert exports the same strictEqual/ok. Unblocks runLinter on #1584. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Kyle Bernhardy <kyle.bernhardy@gmail.com> Co-authored-by: Claude Fable 5 <noreply@anthropic.com> Co-authored-by: Kris Zyp <kris@harperdb.io> Co-authored-by: Kris Zyp <kriszyp@gmail.com> (cherry picked from commit 5e6d9f1)
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request resolves issue #1583 where the static plugin failed to serve files when a slash-less urlPath was configured. It normalizes the urlPath by ensuring a leading slash, exposes the unstripped originalPathname in stripPrefix to handle directory redirects, and updates the static plugin to key files relative to the mount base. The review feedback highlights a potential issue with nested proxies when retrieving originalPathname and suggests safeguarding against undefined values for req.url to prevent runtime TypeErrors.
| if (prop === 'originalPathname') { | ||
| return target.pathname ?? '/'; | ||
| } |
There was a problem hiding this comment.
When stripPrefix is nested (i.e., wrapping an already proxied request), accessing originalPathname on the outer proxy will return the partially stripped pathname of the inner proxy (target.pathname) instead of the truly unstripped original pathname.
To preserve the original unstripped pathname across nested proxies, we should check if target already exposes originalPathname and prefer that.
| if (prop === 'originalPathname') { | |
| return target.pathname ?? '/'; | |
| } | |
| if (prop === 'originalPathname') { | |
| return target.originalPathname ?? target.pathname ?? '/'; | |
| } |
| const queryIndex = (req.url as string).indexOf('?'); | ||
| const query = queryIndex === -1 ? '' : (req.url as string).slice(queryIndex); |
There was a problem hiding this comment.
If req.url is undefined (which can happen in certain test environments or custom request objects), casting it to string and calling .indexOf() will throw a TypeError at runtime.
We should safely fallback to an empty string before performing string operations.
const reqUrl = req.url ?? '';
const queryIndex = reqUrl.indexOf('?');
const query = queryIndex === -1 ? '' : reqUrl.slice(queryIndex);| const queryIndex = (req.url as string).indexOf('?'); | ||
| const query = queryIndex === -1 ? '' : (req.url as string).slice(queryIndex); |
There was a problem hiding this comment.
If req.url is undefined (which can happen in certain test environments or custom request objects), casting it to string and calling .indexOf() will throw a TypeError at runtime.
We should safely fallback to an empty string before performing string operations.
const reqUrl = req.url ?? '';
const queryIndex = reqUrl.indexOf('?');
const query = queryIndex === -1 ? '' : reqUrl.slice(queryIndex);
|
Reviewed; no blockers found. |
2 participants
Cherry-pick of #1584 onto `v5.1`. Clean cherry-pick — no conflicts.
Fixes #1730 (`static.urlPath` subpath mounts return 404 in 5.1.x) on the v5.1 line. #1584 landed on `main` (5.2); this ships the same fix as a 5.1.18+ patch.
What it fixes
Two stacked defects that made `static` with a configured `urlPath` serve 404 at every path:
Plus the review-driven behaviors from #1584: `urlPath` runtime changes request a restart (the mount can't re-register live), and the mount root disambiguates the no-slash form via `originalPathname` (301 `/app` → `/app/`, query string preserved).
Provenance
Cherry-picked squash commit `5e6d9f1` (`git cherry-pick -x`); `server/middlewareChain.ts` auto-merged cleanly against v5.1 context. Includes the integration test `integrationTests/components/static-urlpath.test.ts` (file-under-mount, directory index, root index at `/app/`, `/app` → `/app/` 301) and the `normalizeUrlPath`/slash-less `matchesRoute` unit tests.
Once merged, #1730 ships fixed on 5.1.
— KrAIs 🤖