fix: correctly serve ISR pages with trailing slash set to 'always' - #5
anurag6569201 wants to merge 1 commit into
Conversation
Source PR: sveltejs#16903 Source head: 8ab1c3e
⛔ Shipwright · BlockedRecommendation: do not merge PR #5 · Tier
Findings (7)
Fireworks usage: 15,108 input · 806 output · 15,914 total tokens · $0.0039 · 13s · 0 fix iteration(s) Open the Shipwright check for full evidence and the audit bundle. Use |
![shipwright-agent[bot]](https://avatars.githubusercontent.com/u/140162738?s=60&v=4){kind=small}
| } | ||
| } | ||
|
|
||
| const filesystem = static_config.routes.findIndex((route) => route.handle === 'filesystem'); |
There was a problem hiding this comment.
Shipwright · CRITICAL
The new static ISR route insertion assumes static_config.routes always contains a route with handle === 'filesystem'.
Impact: The new static ISR route insertion assumes static_config.routes always contains a route with handle === 'filesystem'. If that route is absent, findIndex returns -1 and splice(-1, 0, ...static_isr_routes) inserts before the last route instead of before the filesystem phase, silently breaking routing order for static ISR routes.
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
| // as a search parameter, so we need to extract it | ||
| const url = new URL(request.url); | ||
| let pathname = url.searchParams.get('__pathname'); | ||
| const pathname = url.searchParams.get('__pathname'); |
There was a problem hiding this comment.
Shipwright · CRITICAL
The ISR pathname is now taken directly from the route regex capture group and assigned to url.pathname without normalization.
Impact: The ISR pathname is now taken directly from the route regex capture group and assigned to url.pathname without normalization. Previously the code collapsed repeated slashes and reconstructed the data suffix. With optional/rest route patterns, the captured value can contain empty or repeated slash segments, producing malformed pathnames such as /foo//bar or losing the __data.json suffix behavior, which can cause fals…
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
| vercel-token: ${{ secrets.VERCEL_TOKEN }} | ||
| vercel-org-id: ${{ secrets.VERCEL_ORG_ID }} | ||
|
|
||
| # avoid stale cache results when testing ISR routes |
There was a problem hiding this comment.
Shipwright · CRITICAL
The workflow change exposes VERCEL_TOKEN and VERCEL_ORG_ID as environment variables and also interpolates VERCEL_TOKEN directly into the command line.
Impact: The workflow change exposes VERCEL_TOKEN and VERCEL_ORG_ID as environment variables and also interpolates VERCEL_TOKEN directly into the command line. The token is already available as a secret in the step, so the explicit env block and inline --token interpolation increase the risk of accidental logging or command-line disclosure. Use the existing vercel-action secret handling or pass the token via stdin/env withou…
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
| }); | ||
| } | ||
|
|
||
| // Vercel's filesystem phase serves a function at its own path, with or without a |
There was a problem hiding this comment.
Shipwright · HIGH
The new static_isr_routes array is typed as any[] and the routing logic branches on route.segments.some((segment) => segment.dynamic) with no explanation of why static and dynamic
Impact: The new static_isr_routes array is typed as any[] and the routing logic branches on route.segments.some((segment) => segment.dynamic) with no explanation of why static and dynamic ISR routes must be ordered differently. A future maintainer cannot tell from the code why static routes must precede filesystem while dynamic routes must follow it.
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
| test('__data.json function exists in Vercel routing configuration', () => { | ||
| assert.ok(isr_page_route_sources.some((src) => src.includes('__data.json'))); | ||
| }); | ||
|
|
There was a problem hiding this comment.
Shipwright · HIGH
The test only asserts the generated config for one static ISR route and one dynamic ISR route.
Impact: The test only asserts the generated config for one static ISR route and one dynamic ISR route. It does not cover optional parameters, rest parameters, or routes with multiple dynamic segments, which are exactly the cases where the removed get_pathname and regex normalization logic previously prevented malformed __pathname values.
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
| vercel-org-id: ${{ secrets.VERCEL_ORG_ID }} | ||
|
|
||
| # avoid stale cache results when testing ISR routes | ||
| - name: Purge Vercel CDN Cache |
There was a problem hiding this comment.
Shipwright · HIGH
The CDN cache purge runs unconditionally before the platform test and uses --yes.
Impact: The CDN cache purge runs unconditionally before the platform test and uses --yes. If VERCEL_PROJECT_ID_BASIC is unset or wrong, the purge may target the wrong project or fail in a way that leaves stale cache entries, causing flaky ISR tests that are hard to diagnose.
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
| } | ||
|
|
||
| const q = `?__pathname=/${pathname}`; | ||
| const routes = route.segments.some((segment) => segment.dynamic) |
There was a problem hiding this comment.
Shipwright · HIGH
The route destination is built by interpolating src-derived pathname into a query string without URL-encoding.
Impact: The route destination is built by interpolating src-derived pathname into a query string without URL-encoding. If a route pattern contains characters that are special in query strings, the generated __pathname parameter can be malformed or allow query-parameter injection into the ISR function request.
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
1 participant
Fixed sveltejs#12556
Please don't delete this checklist! Before submitting the PR, please make sure you do the following:
Tests
pnpm testand lint the project withpnpm lintandpnpm checkChangesets
pnpm changesetand following the prompts. Changesets that add features should beminorand those that fix bugs should bepatch. Please prefix changeset messages withfeat:,fix:, orchore:.Edits
Source merge-base:
0544d5f13686bd5e6f6daa5f074b064990134833Source head:
8ab1c3eafee04b2d06867377a7805e828e4754e0