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): don't trigger get of headers #12937

Merged
merged 2 commits into from
Jan 8, 2025
Merged
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
5 changes: 5 additions & 0 deletions .changeset/sweet-zoos-move.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'astro': patch
---

Fixes a bug where users could use `Astro.request.headers` during a rewrite inside prerendered routes. This an invalid behaviour, and now Astro will show a warning if this happens.
5 changes: 5 additions & 0 deletions .changeset/tame-pianos-approve.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'astro': patch
---

Fixes an issue where the use of `Astro.rewrite` would trigger the invalid use of `Astro.request.headers`
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these two separate issues beign fixed? It's a little unclear.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. What should I reword?

Copy link
Contributor

Choose a reason for hiding this comment

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

That's fine. Just wanted to confirm that they're separate.

18 changes: 16 additions & 2 deletions packages/astro/src/core/render-context.ts
Original file line number Diff line number Diff line change
Expand Up @@ -175,7 +175,14 @@ export class RenderContext {
if (payload instanceof Request) {
this.request = payload;
} else {
this.request = copyRequest(newUrl, this.request);
this.request = copyRequest(
newUrl,
this.request,
// need to send the flag of the previous routeData
routeData.prerender,
this.pipeline.logger,
this.routeData.route,
);
}
this.isRewriting = true;
this.url = new URL(this.request.url);
Expand Down Expand Up @@ -290,7 +297,14 @@ export class RenderContext {
if (reroutePayload instanceof Request) {
this.request = reroutePayload;
} else {
this.request = copyRequest(newUrl, this.request);
this.request = copyRequest(
newUrl,
this.request,
// need to send the flag of the previous routeData
routeData.prerender,
this.pipeline.logger,
this.routeData.route,
);
}
this.url = new URL(this.request.url);
this.cookies = new AstroCookies(this.request);
Expand Down
14 changes: 12 additions & 2 deletions packages/astro/src/core/request.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,20 @@ import type { IncomingHttpHeaders } from 'node:http';
import type { Logger } from './logger/core.js';

type HeaderType = Headers | Record<string, any> | IncomingHttpHeaders;
type RequestBody = ArrayBuffer | Blob | ReadableStream | URLSearchParams | FormData;
type RequestBody =
| ArrayBuffer
| Blob
| ReadableStream
| URLSearchParams
| FormData
| ReadableStream<Uint8Array>;
Copy link
Member Author

Choose a reason for hiding this comment

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

This is type provided by Request.body

Copy link
Member

Choose a reason for hiding this comment

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

Could we use Request['body'] instead?

Copy link
Member Author

@ematipico ematipico Jan 9, 2025

Choose a reason for hiding this comment

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

Sorry @florian-lefebvre, I completely missed your comment. I'll follow up with another PR


export interface CreateRequestOptions {
url: URL | string;
clientAddress?: string | undefined;
headers: HeaderType;
method?: string;
body?: RequestBody | undefined;
body?: RequestBody | undefined | null;
Copy link
Member Author

Choose a reason for hiding this comment

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

I added null because Request.body is ReadableStream<Uint8Array> | null

logger: Logger;
locals?: object | undefined;
/**
Expand All @@ -22,6 +28,8 @@ export interface CreateRequestOptions {
isPrerendered?: boolean;

routePattern: string;

init?: RequestInit;
}

/**
Expand All @@ -39,6 +47,7 @@ export function createRequest({
logger,
isPrerendered = false,
routePattern,
init,
}: CreateRequestOptions): Request {
// headers are made available on the created request only if the request is for a page that will be on-demand rendered
const headersObj = isPrerendered
Expand All @@ -65,6 +74,7 @@ export function createRequest({
headers: headersObj,
// body is made available only if the request is for a page that will be on-demand rendered
body: isPrerendered ? null : body,
...init,
});

if (isPrerendered) {
Expand Down
47 changes: 32 additions & 15 deletions packages/astro/src/core/routing/rewrite.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,9 @@ import type { RouteData } from '../../types/public/internal.js';
import { shouldAppendForwardSlash } from '../build/util.js';
import { originPathnameSymbol } from '../constants.js';
import { AstroError, AstroErrorData } from '../errors/index.js';
import type { Logger } from '../logger/core.js';
import { appendForwardSlash, removeTrailingForwardSlash } from '../path.js';
import { createRequest } from '../request.js';
import { DEFAULT_404_ROUTE } from './astro-designed-error-pages.js';

export type FindRouteToRewrite = {
Expand Down Expand Up @@ -80,27 +82,42 @@ export function findRouteToRewrite({
*
* @param newUrl The new `URL`
* @param oldRequest The old `Request`
* @param isPrerendered It needs to be the flag of the previous routeData, before the rewrite
* @param logger
* @param routePattern
*/
export function copyRequest(newUrl: URL, oldRequest: Request): Request {
export function copyRequest(
newUrl: URL,
oldRequest: Request,
isPrerendered: boolean,
logger: Logger,
routePattern: string,
): Request {
if (oldRequest.bodyUsed) {
throw new AstroError(AstroErrorData.RewriteWithBodyUsed);
}
return new Request(newUrl, {
return createRequest({
url: newUrl,
method: oldRequest.method,
headers: oldRequest.headers,
body: oldRequest.body,
referrer: oldRequest.referrer,
referrerPolicy: oldRequest.referrerPolicy,
mode: oldRequest.mode,
credentials: oldRequest.credentials,
cache: oldRequest.cache,
redirect: oldRequest.redirect,
integrity: oldRequest.integrity,
signal: oldRequest.signal,
keepalive: oldRequest.keepalive,
// https://fetch.spec.whatwg.org/#dom-request-duplex
// @ts-expect-error It isn't part of the types, but undici accepts it and it allows to carry over the body to a new request
duplex: 'half',
isPrerendered,
logger,
headers: isPrerendered ? {} : oldRequest.headers,
routePattern,
init: {
referrer: oldRequest.referrer,
referrerPolicy: oldRequest.referrerPolicy,
mode: oldRequest.mode,
credentials: oldRequest.credentials,
cache: oldRequest.cache,
redirect: oldRequest.redirect,
integrity: oldRequest.integrity,
signal: oldRequest.signal,
keepalive: oldRequest.keepalive,
// https://fetch.spec.whatwg.org/#dom-request-duplex
// @ts-expect-error It isn't part of the types, but undici accepts it and it allows to carry over the body to a new request
duplex: 'half',
},
});
}

Expand Down
Loading