Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(routing): emit error for forbidden rewrite #12339

Open
wants to merge 4 commits into
base: next
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions .changeset/proud-games-repair.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
---
'astro': patch
---

Now Astro emits an error when `Astro.rewrite` is used to rewrite an on-demand route with a static route, when using the `"server"` output.

This isn't possible because Astro can't retrieve the emitted static route at runtime, because it's served by the hosting platform, and not Astro itself.
Comment on lines +5 to +7
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Now Astro emits an error when `Astro.rewrite` is used to rewrite an on-demand route with a static route, when using the `"server"` output.
This isn't possible because Astro can't retrieve the emitted static route at runtime, because it's served by the hosting platform, and not Astro itself.
Adds an error when `Astro.rewrite()` is used to rewrite an on-demand route with a static route when using the `"server"` output.
This is a forbidden rewrite because Astro can't retrieve the emitted static route at runtime. This route is served by the hosting platform, and not Astro itself.

17 changes: 17 additions & 0 deletions packages/astro/src/core/errors/errors-data.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1260,6 +1260,23 @@ export const RewriteWithBodyUsed = {
'Astro.rewrite() cannot be used if the request body has already been read. If you need to read the body, first clone the request.',
} satisfies ErrorData;

/**
* @docs
* @description
* `Astro.rewrite()` can't be used to rewrite an on-demand route with a static route, when using the `"server"` output.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* `Astro.rewrite()` can't be used to rewrite an on-demand route with a static route, when using the `"server"` output.
* `Astro.rewrite()` can't be used to rewrite an on-demand route with a static route when using the `"server"` output.

*
*/
export const ForbiddenRewrite = {
name: 'ForbiddenRewrite',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just noting that there may eventually be other kinds of "forbidden" rewrites, so not sure whether you want to add more context like ForbiddenRewriteToStatic or something like that?

title: "Can't use `Astro.rewrite()` from a on-demand route to static route with 'server' output.",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
title: "Can't use `Astro.rewrite()` from a on-demand route to static route with 'server' output.",
title: "Can't use `Astro.rewrite()` from an on-demand route to static route with 'server' output.",

typo fix here at the very least, but in fact, see suggestion for title below also.

Copy link
Member

@sarah11918 sarah11918 Nov 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At the very least, the typo needs to be corrected to an on-demand route.

Also noting that this is a little longer than other titles are, and almost repeats the docs text exactly. So this could instead be a shorter thing like:

Forbidden rewrite to a static route

and then let the docs text explain what's happening?

message: (from: string, to: string, component: string) =>
`You tried to rewrite the on-demand route '${from}' with the static route '${to}', when using the 'server' output. \n\nThe static route '${to}' is rendered by the component
'${component}', which is marked as prerendered. This is a forbidden operation because during the build the component '${component}' is compiled to an
HTML file, which is can't be retrieved at runtime by Astro.`,
Comment on lines +1273 to +1275
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
`You tried to rewrite the on-demand route '${from}' with the static route '${to}', when using the 'server' output. \n\nThe static route '${to}' is rendered by the component
'${component}', which is marked as prerendered. This is a forbidden operation because during the build the component '${component}' is compiled to an
HTML file, which is can't be retrieved at runtime by Astro.`,
`You tried to rewrite the on-demand route '${from}' with the static route '${to}', when using the 'server' output. \n\nThe static route '${to}' is rendered by the component
'${component}', which is marked as prerendered. This is a forbidden operation because during the build the component '${component}' is compiled to an
HTML file, which can't be retrieved at runtime by Astro.`,

hint: (component: string) =>
`Add \`export const prerender = false\` to the component '${component}', or use a Astro.redirect().`,
} satisfies ErrorData;

/**
* @docs
* @description
Expand Down
18 changes: 18 additions & 0 deletions packages/astro/src/core/middleware/sequence.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
import type { MiddlewareHandler, RewritePayload } from '../../types/public/common.js';
import type { APIContext } from '../../types/public/context.js';
import { AstroCookies } from '../cookies/cookies.js';
import { ForbiddenRewrite } from '../errors/errors-data.js';
import { AstroError } from '../errors/index.js';
import { apiContextRoutesSymbol } from '../render-context.js';
import { type Pipeline, getParams } from '../render/index.js';
import { defineMiddleware } from './index.js';
Expand Down Expand Up @@ -49,6 +51,22 @@ export function sequence(...handlers: MiddlewareHandler[]): MiddlewareHandler {
payload,
handleContext.request,
);

// This is a case where the user tries to rewrite from a SSR route to a prerendered route (SSG).
// This case isn't valid because when building for SSR, the prerendered route disappears from the server output because it becomes an HTML file,
// so Astro can't retrieve it from the emitted manifest.
if (
pipeline.serverLike === true &&
handleContext.isPrerendered === false &&
routeData.prerender === true
) {
throw new AstroError({
...ForbiddenRewrite,
message: ForbiddenRewrite.message(pathname, pathname, routeData.component),
hint: ForbiddenRewrite.hint(routeData.component),
});
}

carriedPayload = payload;
handleContext.request = newRequest;
handleContext.url = new URL(newRequest.url);
Expand Down
32 changes: 32 additions & 0 deletions packages/astro/src/core/render-context.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import {
} from './constants.js';
import { AstroCookies, attachCookiesToResponse } from './cookies/index.js';
import { getCookiesFromResponse } from './cookies/response.js';
import { ForbiddenRewrite } from './errors/errors-data.js';
import { AstroError, AstroErrorData } from './errors/index.js';
import { callMiddleware } from './middleware/callMiddleware.js';
import { sequence } from './middleware/index.js';
Expand Down Expand Up @@ -145,6 +146,22 @@ export class RenderContext {
pathname,
newUrl,
} = await pipeline.tryRewrite(payload, this.request);

// This is a case where the user tries to rewrite from a SSR route to a prerendered route (SSG).
// This case isn't valid because when building for SSR, the prerendered route disappears from the server output because it becomes an HTML file,
// so Astro can't retrieve it from the emitted manifest.
if (
this.pipeline.serverLike === true &&
this.routeData.prerender === false &&
routeData.prerender === true
) {
throw new AstroError({
...ForbiddenRewrite,
message: ForbiddenRewrite.message(this.pathname, pathname, routeData.component),
hint: ForbiddenRewrite.hint(routeData.component),
});
}

this.routeData = routeData;
componentInstance = newComponent;
if (payload instanceof Request) {
Expand Down Expand Up @@ -246,6 +263,21 @@ export class RenderContext {
reroutePayload,
this.request,
);
// This is a case where the user tries to rewrite from a SSR route to a prerendered route (SSG).
// This case isn't valid because when building for SSR, the prerendered route disappears from the server output because it becomes an HTML file,
// so Astro can't retrieve it from the emitted manifest.
if (
this.pipeline.serverLike === true &&
this.routeData.prerender === false &&
routeData.prerender === true
) {
throw new AstroError({
...ForbiddenRewrite,
message: ForbiddenRewrite.message(this.pathname, pathname, routeData.component),
hint: ForbiddenRewrite.hint(routeData.component),
});
}

this.routeData = routeData;
if (reroutePayload instanceof Request) {
this.request = reroutePayload;
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
---
return Astro.rewrite("/forbidden/static")
export const prerender = false
---
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
---
export const prerender = true
---
7 changes: 7 additions & 0 deletions packages/astro/test/rewrite.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -149,6 +149,13 @@ describe('Dev rewrite, hybrid/server', () => {

assert.equal($('title').text(), 'RewriteWithBodyUsed');
});

it('should error when rewriting from a SSR route to a SSG route', async () => {
const html = await fixture.fetch('/forbidden/dynamic').then((res) => res.text());
const $ = cheerioLoad(html);

assert.match($('title').text(), /ForbiddenRewrite/);
});
});

describe('Build reroute', () => {
Expand Down
Loading