From fbac92f8bdbb5ee1312726b2a535a81271b3f7d6 Mon Sep 17 00:00:00 2001 From: Chris Kanich Date: Sun, 5 Jan 2025 13:54:41 -0600 Subject: [PATCH] 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 | 5 ++ .changeset/tough-cars-yawn.md | 5 ++ .gitignore | 1 + packages/astro/src/core/app/index.ts | 4 +- packages/astro/src/core/session.ts | 35 +++++++++----- .../fixtures/sessions/src/actions/index.ts | 13 ++++- .../fixtures/sessions/src/pages/update.ts | 10 ++++ packages/astro/test/sessions.test.js | 48 +++++++++++++++++++ 8 files changed, 105 insertions(+), 16 deletions(-) create mode 100644 .changeset/slimy-oranges-argue.md create mode 100644 .changeset/tough-cars-yawn.md create mode 100644 packages/astro/test/fixtures/sessions/src/pages/update.ts diff --git a/.changeset/slimy-oranges-argue.md b/.changeset/slimy-oranges-argue.md new file mode 100644 index 0000000000..f28ff49e1a --- /dev/null +++ b/.changeset/slimy-oranges-argue.md @@ -0,0 +1,5 @@ +--- +'astro': patch +--- + +Fixes a bug where responses can be returned before session data is saved diff --git a/.changeset/tough-cars-yawn.md b/.changeset/tough-cars-yawn.md new file mode 100644 index 0000000000..534018e45f --- /dev/null +++ b/.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() diff --git a/.gitignore b/.gitignore index d6a28ec1b1..8626a30a0e 100644 --- a/.gitignore +++ b/.gitignore @@ -5,6 +5,7 @@ dist/ .vercel .netlify _site/ +.astro/ scripts/smoke/*-main/ scripts/memory/project/src/pages/ benchmark/projects/ diff --git a/packages/astro/src/core/app/index.ts b/packages/astro/src/core/app/index.ts index 7d33ef1d0f..b232ebbff5 100644 --- a/packages/astro/src/core/app/index.ts +++ b/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](); } } diff --git a/packages/astro/src/core/session.ts b/packages/astro/src/core/session.ts index 9b5e4eb9e3..ccb01ef611 100644 --- a/packages/astro/src/core/session.ts +++ b/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 { // The cookies object. #cookies: AstroCookies; @@ -138,9 +152,12 @@ export class AstroSession { 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 { // 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 { 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 { } 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({ diff --git a/packages/astro/test/fixtures/sessions/src/actions/index.ts b/packages/astro/test/fixtures/sessions/src/actions/index.ts index 33ac1cb653..856f68ba8f 100644 --- a/packages/astro/test/fixtures/sessions/src/actions/index.ts +++ b/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('favoriteUrl'); + const url = new URL(input.favoriteUrl); + context.session.set('favoriteUrl', url); + return { message: 'Favorite URL set to ' + url.href + ' from ' + (previousFavoriteUrl?.href ?? "nothing") }; + } + }) +} diff --git a/packages/astro/test/fixtures/sessions/src/pages/update.ts b/packages/astro/test/fixtures/sessions/src/pages/update.ts new file mode 100644 index 0000000000..71b058e753 --- /dev/null +++ b/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}); +}; diff --git a/packages/astro/test/sessions.test.js b/packages/astro/test/sessions.test.js index 8490e78ba3..1dbc304bd2 100644 --- a/packages/astro/test/sessions.test.js +++ b/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/', + ); + }); }); });