Repository navigation
Respect path-segment boundaries when stripping the configured base - #17701
Conversation
🦋 Changeset detectedLatest commit: c69b9d7 The changes in this PR will be included in the next version bump. This PR includes changesets to release 434 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
…-boundary # Conflicts: # packages/astro/src/core/fetch/fetch-state.ts
There was a problem hiding this comment.
This PR correctly centralizes request base-stripping into stripRequestBase for BaseApp.removeBase, FetchState.#computePathname, and computePathnameFromDomain. The changeset and new regression test are in place. However, BaseApp.match / matchRequest still use a fourth, prefix-only base-stripping implementation, leaving the described base-prefix authorization bypass reachable for any adapter that calls app.match() and passes the resulting routeData to app.render(). The new tests also do not cover app.match() or the routeData-provided render path.
Additional findings
[high][security]: matchRequest still uses prefix-only base stripping
packages/astro/src/core/app/base.ts:326 RIGHT
BaseApp.match() delegates to matchRequest() in packages/astro/src/core/routing/match-request.ts, whose local removeBase() still does a plain pathname.startsWith(manifest.base) prefix check. If an adapter calls app.match(req) and then app.render(req, { routeData }), a request like /appX/admin will match the /admin route, and because FetchState skips re-routing when routeData is provided, the protected page renders while middleware still sees /appX/admin. The PR should consolidate matchRequest to use the new stripRequestBase helper so all base-stripping paths respect segment boundaries.
This review was made by an LLM. The analysis may be wrong, and reports might be incorrect.
This PR contains the following updates: | Package | Change | [Age](https://docs.renovatebot.com/merge-confidence/) | [Confidence](https://docs.renovatebot.com/merge-confidence/) | |---|---|---|---| | [@astrojs/markdown-remark](https://astro.build) ([source](https://github.com/withastro/astro/tree/HEAD/packages/markdown/remark)) | [`7.2.3` → `7.2.4`](https://renovatebot.com/diffs/npm/@astrojs%2fmarkdown-remark/7.2.3/7.2.4) |  |  | | [astro](https://astro.build) ([source](https://github.com/withastro/astro/tree/HEAD/packages/astro)) | [`7.2.3` → `7.2.4`](https://renovatebot.com/diffs/npm/astro/7.2.3/7.2.4) |  |  | --- ### Release Notes <details> <summary>withastro/astro (@​astrojs/markdown-remark)</summary> ### [`v7.2.4`](https://github.com/withastro/astro/blob/HEAD/packages/markdown/remark/CHANGELOG.md#724) [Compare Source](https://github.com/withastro/astro/compare/@astrojs/markdown-remark@7.2.3...@astrojs/markdown-remark@7.2.4) ##### Patch Changes - Updated dependencies \[[`05763a0`](withastro/astro@05763a0)]: - [@​astrojs/internal-helpers](https://github.com/astrojs/internal-helpers)@​0.10.4 </details> <details> <summary>withastro/astro (astro)</summary> ### [`v7.2.4`](https://github.com/withastro/astro/blob/HEAD/packages/astro/CHANGELOG.md#724) [Compare Source](https://github.com/withastro/astro/compare/astro@7.2.3...astro@7.2.4) ##### Patch Changes - [#​17747](withastro/astro#17747) [`a90ff66`](withastro/astro@a90ff66) Thanks [@​Princesseuh](https://github.com/Princesseuh)! - Fixes builds hanging when an image file is malformed - [#​17701](withastro/astro#17701) [`05763a0`](withastro/astro@05763a0) Thanks [@​matthewp](https://github.com/matthewp)! - Fixes base path stripping to respect path-segment boundaries. With a configured `base` such as `/docs`, a request like `/docs-archive/page` is no longer treated as being under the base, so routing and `context.url.pathname` now agree on the same pathname. - [#​17742](withastro/astro#17742) [`70b449d`](withastro/astro@70b449d) Thanks [@​Kjubikstronk](https://github.com/Kjubikstronk)! - Fixes `astro build` throwing `TypeError: Missing parameter` for dynamic routes when `build.format: 'preserve'` and `trailingSlash: 'always'` are used together. Stripping the framework-injected `.html` suffix dropped the trailing slash that the compiled route pattern requires, so the route no longer matched itself and its params resolved as empty. - [#​17703](withastro/astro#17703) [`771b0a9`](withastro/astro@771b0a9) Thanks [@​astrobot-houston](https://github.com/astrobot-houston)! - Fixes `Astro.site` always being `undefined` when rendering components via the Container API, even when `site` is set in `astroConfig` - Updated dependencies \[[`05763a0`](withastro/astro@05763a0), [`bc171af`](withastro/astro@bc171af)]: - [@​astrojs/internal-helpers](https://github.com/astrojs/internal-helpers)@​0.10.4 - [@​astrojs/markdown-satteri](https://github.com/astrojs/markdown-satteri)@​0.3.7 - [@​astrojs/markdown-remark](https://github.com/astrojs/markdown-remark)@​7.2.4 </details> --- ### Configuration 📅 **Schedule**: (UTC) - Branch creation - At any time (no schedule defined) - Automerge - At any time (no schedule defined) 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about these updates again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Mend Renovate CLI](https://github.com/renovatebot/renovate). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiI0NC4zMS4wIiwidXBkYXRlZEluVmVyIjoiNDQuMzEuMCIsInRhcmdldEJyYW5jaCI6Im1haW4iLCJsYWJlbHMiOltdfQ==-->
Changes
basewhen the pathname is the base itself or continues at a path-segment boundary. Withbase: '/docs', a request like/docs-archive/pageis treated as outside the base instead of being rewritten to/page. This keeps route matching andcontext.url.pathnamein agreement.BaseApp.removeBase,FetchState.#computePathname, and the i18n domain helper) into a single sharedstripRequestBasehelper in@astrojs/internal-helpers, matching the boundary logic the router already uses instripBase.Testing
base-prefix-boundary.test.tscovering single-character extensions of the base prefix (/docsX/...,/docs2/...,/docs-/...), asserting they do not resolve to a route under the base.Docs