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

@@ -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.

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;

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<Response> {
 		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
 					});
 				}
 			}

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;

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<RenderContext, 'pathname' | 'pipeline' | 'request' | 'routeData'> &
+	}: Pick<RenderContext, 'pathname' | 'pipeline' | 'request' | 'routeData' | 'clientAddress'> &
 		Partial<
 			Pick<RenderContext, 'locals' | 'middleware' | 'status' | 'props' | 'partial'>
 		>): Promise<RenderContext> {
@@ -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,16 +522,24 @@ 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 (request.body === null) {
+		if(routeData.prerender) {
 			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) {
 			throw new AstroError({
 				...AstroErrorData.ClientAddressNotAvailable,

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 ?? {});

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);
 		}

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);
+});