From 306c9f9a9ae08d194ca2a066ab71cde02eeb0874 Mon Sep 17 00:00:00 2001 From: Matthew Phillips Date: Thu, 5 Dec 2024 08:10:21 -0500 Subject: [PATCH] Keep clientAddress on cloned requests (#12613) * Keep clientAddress on cloned requests User observed that calling actions resulted in an error about not having clientRequest available. This is because the user had a middleware that cloned the request, which loses all of the symbols. The fix is to pass the clientAddress directly into the RenderContext. This deprecates the `clientAddressSymbol`, but we need to keep it for now because some adapters set the clientAddress that way. Note that similar fixes should be done for other symbol usage on the Request object (locals is one). * changeset * fix build stuff * Update packages/astro/src/core/render-context.ts * Update changeset --- .changeset/wild-geckos-draw.md | 9 ++++++ packages/astro/src/container/index.ts | 1 + packages/astro/src/core/app/index.ts | 17 ++++++----- packages/astro/src/core/build/generate.ts | 1 + packages/astro/src/core/render-context.ts | 29 +++++++++++++------ packages/astro/src/core/request.ts | 5 ---- .../src/vite-plugin-astro-server/route.ts | 3 +- .../fixtures/client-address/src/middleware.ts | 12 ++++++++ 8 files changed, 55 insertions(+), 22 deletions(-) create mode 100644 .changeset/wild-geckos-draw.md create mode 100644 packages/astro/test/fixtures/client-address/src/middleware.ts diff --git a/.changeset/wild-geckos-draw.md b/.changeset/wild-geckos-draw.md new file mode 100644 index 0000000000..2965af1599 --- /dev/null +++ b/.changeset/wild-geckos-draw.md @@ -0,0 +1,9 @@ +--- +'astro': patch +--- + +Fix use of cloned requests in middleware with clientAddress + +When using `context.clientAddress` or `Astro.clientAddress` Astro looks up the address in a hidden property. Cloning a request can cause this hidden property to be lost. + +The fix is to pass the address as an internal property instead, decoupling it from the request. diff --git a/packages/astro/src/container/index.ts b/packages/astro/src/container/index.ts index 2564e28750..c762773d3b 100644 --- a/packages/astro/src/container/index.ts +++ b/packages/astro/src/container/index.ts @@ -508,6 +508,7 @@ export class experimental_AstroContainer { pathname: url.pathname, locals: options?.locals ?? {}, partial: options?.partial ?? true, + clientAddress: '' }); if (options.params) { renderContext.params = options.params; diff --git a/packages/astro/src/core/app/index.ts b/packages/astro/src/core/app/index.ts index 09b3edea12..da5f917c1f 100644 --- a/packages/astro/src/core/app/index.ts +++ b/packages/astro/src/core/app/index.ts @@ -72,6 +72,7 @@ export interface RenderErrorOptions { * Allows passing an error to 500.astro. It will be available through `Astro.props.error`. */ error?: unknown; + clientAddress: string | undefined; } export class App { @@ -240,7 +241,7 @@ export class App { let addCookieHeader: boolean | undefined; addCookieHeader = renderOptions?.addCookieHeader; - clientAddress = renderOptions?.clientAddress; + clientAddress = renderOptions?.clientAddress ?? Reflect.get(request,clientAddressSymbol); routeData = renderOptions?.routeData; locals = renderOptions?.locals; @@ -256,13 +257,10 @@ export class App { if (typeof locals !== 'object') { const error = new AstroError(AstroErrorData.LocalsNotAnObject); this.#logger.error(null, error.stack!); - return this.#renderError(request, { status: 500, error }); + return this.#renderError(request, { status: 500, error, clientAddress }); } Reflect.set(request, clientLocalsSymbol, locals); } - if (clientAddress) { - Reflect.set(request, clientAddressSymbol, clientAddress); - } if (!routeData) { routeData = this.match(request); this.#logger.debug('router', 'Astro matched the following route for ' + request.url); @@ -271,7 +269,7 @@ export class App { if (!routeData) { this.#logger.debug('router', "Astro hasn't found routes that match " + request.url); this.#logger.debug('router', "Here's the available routes:\n", this.#manifestData); - return this.#renderError(request, { locals, status: 404 }); + return this.#renderError(request, { locals, status: 404, clientAddress }); } const pathname = this.#getPathnameFromRequest(request); const defaultStatus = this.#getDefaultStatusCode(routeData, pathname); @@ -288,11 +286,12 @@ export class App { request, routeData, status: defaultStatus, + clientAddress }); response = await renderContext.render(await mod.page()); } catch (err: any) { this.#logger.error(null, err.stack || err.message || String(err)); - return this.#renderError(request, { locals, status: 500, error: err }); + return this.#renderError(request, { locals, status: 500, error: err, clientAddress }); } if ( @@ -306,6 +305,7 @@ export class App { // We don't have an error to report here. Passing null means we pass nothing intentionally // while undefined means there's no error error: response.status === 500 ? null : undefined, + clientAddress }); } @@ -353,6 +353,7 @@ export class App { response: originalResponse, skipMiddleware = false, error, + clientAddress, }: RenderErrorOptions, ): Promise { const errorRoutePath = `/${status}${this.#manifest.trailingSlash === 'always' ? '/' : ''}`; @@ -386,6 +387,7 @@ export class App { routeData: errorRouteData, status, props: { error }, + clientAddress }); const response = await renderContext.render(await mod.page()); return this.#mergeResponses(response, originalResponse); @@ -397,6 +399,7 @@ export class App { status, response: originalResponse, skipMiddleware: true, + clientAddress }); } } diff --git a/packages/astro/src/core/build/generate.ts b/packages/astro/src/core/build/generate.ts index 5bdd6dc2f4..6fe6351cb6 100644 --- a/packages/astro/src/core/build/generate.ts +++ b/packages/astro/src/core/build/generate.ts @@ -444,6 +444,7 @@ async function generatePath( pathname: pathname, request, routeData: route, + clientAddress: undefined }); let body: string | Uint8Array; diff --git a/packages/astro/src/core/render-context.ts b/packages/astro/src/core/render-context.ts index 611b9cbcbe..614777ac5f 100644 --- a/packages/astro/src/core/render-context.ts +++ b/packages/astro/src/core/render-context.ts @@ -48,6 +48,7 @@ export class RenderContext { public request: Request, public routeData: RouteData, public status: number, + public clientAddress: string | undefined, protected cookies = new AstroCookies(request), public params = getParams(routeData, pathname), protected url = new URL(request.url), @@ -71,10 +72,11 @@ export class RenderContext { pipeline, request, routeData, + clientAddress, status = 200, props, partial = undefined, - }: Pick & + }: Pick & Partial< Pick >): Promise { @@ -88,6 +90,7 @@ export class RenderContext { request, routeData, status, + clientAddress, undefined, undefined, undefined, @@ -309,7 +312,7 @@ export class RenderContext { routePattern: this.routeData.route, isPrerendered: this.routeData.prerender, get clientAddress() { - return renderContext.clientAddress(); + return renderContext.getClientAddress(); }, get currentLocale() { return renderContext.computeCurrentLocale(); @@ -490,7 +493,7 @@ export class RenderContext { isPrerendered: this.routeData.prerender, cookies, get clientAddress() { - return renderContext.clientAddress(); + return renderContext.getClientAddress(); }, get currentLocale() { return renderContext.computeCurrentLocale(); @@ -519,14 +522,22 @@ export class RenderContext { }; } - clientAddress() { - const { pipeline, request } = this; - if (clientAddressSymbol in request) { - return Reflect.get(request, clientAddressSymbol) as string; + getClientAddress() { + const { pipeline, request, routeData, clientAddress } = this; + + if(routeData.prerender) { + throw new AstroError(AstroErrorData.PrerenderClientAddressNotAvailable); } - if (request.body === null) { - throw new AstroError(AstroErrorData.PrerenderClientAddressNotAvailable); + if(clientAddress) { + return clientAddress; + } + + // TODO: Legacy, should not need to get here. + // Some adapters set this symbol so we can't remove support yet. + // Adapters should be updated to provide it via RenderOptions instead. + if (clientAddressSymbol in request) { + return Reflect.get(request, clientAddressSymbol) as string; } if (pipeline.adapterName) { diff --git a/packages/astro/src/core/request.ts b/packages/astro/src/core/request.ts index 71e7da83c9..b8cc89fb84 100644 --- a/packages/astro/src/core/request.ts +++ b/packages/astro/src/core/request.ts @@ -24,7 +24,6 @@ export interface CreateRequestOptions { routePattern: string; } -const clientAddressSymbol = Symbol.for('astro.clientAddress'); const clientLocalsSymbol = Symbol.for('astro.locals'); /** @@ -37,7 +36,6 @@ const clientLocalsSymbol = Symbol.for('astro.locals'); export function createRequest({ url, headers, - clientAddress, method = 'GET', body = undefined, logger, @@ -93,9 +91,6 @@ export function createRequest({ _headers = newHeaders; }, }); - } else if (clientAddress) { - // clientAddress is stored to be read by RenderContext, only if the request is for a page that will be on-demand rendered - Reflect.set(request, clientAddressSymbol, clientAddress); } Reflect.set(request, clientLocalsSymbol, locals ?? {}); diff --git a/packages/astro/src/vite-plugin-astro-server/route.ts b/packages/astro/src/vite-plugin-astro-server/route.ts index eb06846499..3498571e87 100644 --- a/packages/astro/src/vite-plugin-astro-server/route.ts +++ b/packages/astro/src/vite-plugin-astro-server/route.ts @@ -172,7 +172,6 @@ export async function handleRoute({ method: incomingRequest.method, body, logger, - clientAddress: incomingRequest.socket.remoteAddress, isPrerendered: route.prerender, routePattern: route.component, }); @@ -191,6 +190,7 @@ export async function handleRoute({ middleware: isDefaultPrerendered404(matchedRoute.route) ? undefined : middleware, request, routeData: route, + clientAddress: incomingRequest.socket.remoteAddress }); let response; @@ -248,6 +248,7 @@ export async function handleRoute({ middleware: isDefaultPrerendered404(fourOhFourRoute.route) ? undefined : middleware, request, routeData: fourOhFourRoute.route, + clientAddress: incomingRequest.socket.remoteAddress }); response = await renderContext.render(fourOhFourRoute.preloadedComponent); } diff --git a/packages/astro/test/fixtures/client-address/src/middleware.ts b/packages/astro/test/fixtures/client-address/src/middleware.ts new file mode 100644 index 0000000000..9709a5595f --- /dev/null +++ b/packages/astro/test/fixtures/client-address/src/middleware.ts @@ -0,0 +1,12 @@ +import { defineMiddleware } from 'astro:middleware'; + +export const onRequest = defineMiddleware(async (ctx, next) => { + // Clone a request, losing all symbols + const clonedRequest = ctx.request.clone(); + const safeInternalRequest = new Request(clonedRequest, { + method: clonedRequest.method, + headers: clonedRequest.headers, + }); + + return next(safeInternalRequest); +});