fix: clone session data on .set() (#12883)

* fix: clone session data on .set()

* weird race condition, +1 test for URL() thing

* cleanup

* ensure directory exists before using fs-lite sessions

* minor wording change

* alternate ensure-dir-exists implementation

* await session persistence before returning response

* update changeset

* formatting

* two changesets

.changeset/slimy-oranges-argue.md

@@ -0,0 +1,5 @@
+---
+'astro': patch
+---
+
+Fixes a bug where responses can be returned before session data is saved

.changeset/tough-cars-yawn.md

@@ -0,0 +1,5 @@
+---
+'astro': patch
+---
+
+Fixes a bug where session data could be corrupted if it is changed after calling .set()

.gitignore

@@ -5,6 +5,7 @@ dist/
 .vercel
 .netlify
 _site/
+.astro/
 scripts/smoke/*-main/
 scripts/memory/project/src/pages/
 benchmark/projects/

packages/astro/src/core/app/index.ts

@@ -298,7 +298,7 @@ export class App {
 			this.#logger.error(null, err.stack || err.message || String(err));
 			return this.#renderError(request, { locals, status: 500, error: err, clientAddress });
 		} finally {
-			session?.[PERSIST_SYMBOL]();
+			await session?.[PERSIST_SYMBOL]();
 		}
 
 		if (
@@ -412,7 +412,7 @@ export class App {
 					});
 				}
 			} finally {
-				session?.[PERSIST_SYMBOL]();
+				await session?.[PERSIST_SYMBOL]();
 			}
 		}
 

packages/astro/src/core/session.ts

@@ -1,4 +1,4 @@
-import { stringify, unflatten } from 'devalue';
+import { stringify as rawStringify, unflatten as rawUnflatten } from 'devalue';
 import {
 	type BuiltinDriverOptions,
 	type Driver,
@@ -26,6 +26,20 @@ interface SessionEntry {
 	expires?: number;
 }
 
+const unflatten: typeof rawUnflatten = (parsed, _) => {
+	// Revive URL objects
+	return rawUnflatten(parsed, {
+		URL: (href) => new URL(href),
+	});
+};
+
+const stringify: typeof rawStringify = (data, _) => {
+	return rawStringify(data, {
+		// Support URL objects
+		URL: (val) => val instanceof URL && val.href,
+	});
+};
+
 export class AstroSession<TDriver extends SessionDriverName = any> {
 	// The cookies object.
 	#cookies: AstroCookies;
@@ -138,9 +152,12 @@ export class AstroSession<TDriver extends SessionDriverName = any> {
 				message: 'The session key was not provided.',
 			});
 		}
+		// save a clone of the passed in object so later updates are not
+		// persisted into the store. Attempting to serialize also allows
+		// us to throw an error early if needed.
+		let cloned: T;
 		try {
-			// Attempt to serialize the value so we can throw an error early if needed
-			stringify(value);
+			cloned = unflatten(JSON.parse(stringify(value)));
 		} catch (err) {
 			throw new AstroError(
 				{
@@ -160,7 +177,7 @@ export class AstroSession<TDriver extends SessionDriverName = any> {
 		// If ttl is numeric, it is the number of seconds until expiry. To get an expiry timestamp, we convert to milliseconds and add to the current time.
 		const expires = typeof lifetime === 'number' ? Date.now() + lifetime * 1000 : lifetime;
 		this.#data.set(key, {
-			data: value,
+			data: cloned,
 			expires,
 		});
 		this.#dirty = true;
@@ -221,10 +238,7 @@ export class AstroSession<TDriver extends SessionDriverName = any> {
 			const key = this.#ensureSessionID();
 			let serialized;
 			try {
-				serialized = stringify(data, {
-					// Support URL objects
-					URL: (val) => val instanceof URL && val.href,
-				});
+				serialized = stringify(data);
 			} catch (err) {
 				throw new AstroError(
 					{
@@ -293,10 +307,7 @@ export class AstroSession<TDriver extends SessionDriverName = any> {
 		}
 
 		try {
-			const storedMap = unflatten(raw, {
-				// Revive URL objects
-				URL: (href) => new URL(href),
-			});
+			const storedMap = unflatten(raw);
 			if (!(storedMap instanceof Map)) {
 				await this.#destroySafe();
 				throw new AstroError({

packages/astro/test/fixtures/sessions/src/actions/index.ts

@@ -21,7 +21,16 @@ export const server = {
 		accept: 'json',
 		handler: async (input, context) => {
 			await context.session.set('cart', []);
-			return {cart: [], message: 'Cart cleared at ' + new Date().toTimeString() };
+			return { cart: [], message: 'Cart cleared at ' + new Date().toTimeString() };
 		},
 	}),
-};
+	addUrl: defineAction({
+		input: z.object({ favoriteUrl: z.string().url() }),
+		handler: async (input, context) => {
+			const previousFavoriteUrl = await context.session.get<URL>('favoriteUrl');
+			const url = new URL(input.favoriteUrl);
+			context.session.set('favoriteUrl', url);
+			return { message: 'Favorite URL set to ' + url.href + ' from ' + (previousFavoriteUrl?.href ?? "nothing") };
+		}
+	})
+}

packages/astro/test/fixtures/sessions/src/pages/update.ts

@@ -0,0 +1,10 @@
+import type { APIRoute } from 'astro';
+
+export const GET: APIRoute = async (context) => {
+	const previousObject = await context.session.get("key") ?? { value: "none" };
+	const previousValue = previousObject.value;
+	const sessionData = { value: "expected" };
+	context.session.set("key", sessionData);
+	sessionData.value = "unexpected";
+	return Response.json({previousValue});
+};

packages/astro/test/sessions.test.js

@@ -1,5 +1,6 @@
 import assert from 'node:assert/strict';
 import { before, describe, it } from 'node:test';
+import * as devalue from 'devalue';
 import testAdapter from './test-adapter.js';
 import { loadFixture } from './test-utils.js';
 
@@ -43,5 +44,52 @@ describe('Astro.session', () => {
 			const secondSessionId = secondHeaders[0].split(';')[0].split('=')[1];
 			assert.notEqual(firstSessionId, secondSessionId);
 		});
+
+		it('can save session data by value', async () => {
+			const firstResponse = await fetchResponse('/update', { method: 'GET' });
+			const firstValue = await firstResponse.json();
+			assert.equal(firstValue.previousValue, 'none');
+
+			const firstHeaders = Array.from(app.setCookieHeaders(firstResponse));
+			const firstSessionId = firstHeaders[0].split(';')[0].split('=')[1];
+			const secondResponse = await fetchResponse('/update', {
+				method: 'GET',
+				headers: {
+					cookie: `astro-session=${firstSessionId}`,
+				},
+			});
+			const secondValue = await secondResponse.json();
+			assert.equal(secondValue.previousValue, 'expected');
+		});
+
+		it('can save and restore URLs in session data', async () => {
+			const firstResponse = await fetchResponse('/_actions/addUrl', {
+				method: 'POST',
+				headers: {
+					'Content-Type': 'application/json',
+				},
+				body: JSON.stringify({ favoriteUrl: 'https://domain.invalid' }),
+			});
+
+			assert.equal(firstResponse.ok, true);
+			const firstHeaders = Array.from(app.setCookieHeaders(firstResponse));
+			const firstSessionId = firstHeaders[0].split(';')[0].split('=')[1];
+
+			const data = devalue.parse(await firstResponse.text());
+			assert.equal(data.message, 'Favorite URL set to https://domain.invalid/ from nothing');
+			const secondResponse = await fetchResponse('/_actions/addUrl', {
+				method: 'POST',
+				headers: {
+					'Content-Type': 'application/json',
+					cookie: `astro-session=${firstSessionId}`,
+				},
+				body: JSON.stringify({ favoriteUrl: 'https://example.com' }),
+			});
+			const secondData = devalue.parse(await secondResponse.text());
+			assert.equal(
+				secondData.message,
+				'Favorite URL set to https://example.com/ from https://domain.invalid/',
+			);
+		});
 	});
 });