diff --git a/.changeset/red-quail-bite.md b/.changeset/red-quail-bite.md new file mode 100644 index 000000000000..0dca2db5d325 --- /dev/null +++ b/.changeset/red-quail-bite.md @@ -0,0 +1,5 @@ +--- +'astro': patch +--- + +Fixes an issue where a session ID from a cookie with no matching server-side data was accepted as-is. The session now generates a new ID when the cookie value has no corresponding storage entry. diff --git a/packages/astro/src/core/session/runtime.ts b/packages/astro/src/core/session/runtime.ts index d814ca95e528..a72f12ffa232 100644 --- a/packages/astro/src/core/session/runtime.ts +++ b/packages/astro/src/core/session/runtime.ts @@ -53,6 +53,8 @@ export class AstroSession { #dirty = false; // Whether the session cookie has been set. #cookieSet = false; + // Whether the session ID was sourced from a client cookie rather than freshly generated. + #sessionIDFromCookie = false; // The local data is "partial" if it has not been loaded from storage yet and only // contains values that have been set or deleted in-memory locally. // We do this to avoid the need to block on loading data when it is only being set. @@ -242,6 +244,7 @@ export class AstroSession { // Create new session this.#sessionID = crypto.randomUUID(); + this.#sessionIDFromCookie = false; this.#data = data; this.#dirty = true; await this.#setCookie(); @@ -349,6 +352,16 @@ export class AstroSession { // We stored this as a devalue string, but unstorage will have parsed it as JSON const raw = await storage.get(this.#ensureSessionID()); if (!raw) { + if (this.#sessionIDFromCookie) { + // The session ID was supplied by the client cookie but has no corresponding + // server-side data. Generate a new server-controlled ID rather than + // accepting an unrecognized value from the client. + this.#sessionID = crypto.randomUUID(); + this.#sessionIDFromCookie = false; + if (this.#cookieSet) { + await this.#setCookie(); + } + } // If there is no existing data in storage we don't need to merge anything // and can just return the existing local data. return this.#data; @@ -404,7 +417,15 @@ export class AstroSession { * Returns the session ID, generating a new one if it does not exist. */ #ensureSessionID() { - this.#sessionID ??= this.#cookies.get(this.#cookieName)?.value ?? crypto.randomUUID(); + if (!this.#sessionID) { + const cookieValue = this.#cookies.get(this.#cookieName)?.value; + if (cookieValue) { + this.#sessionID = cookieValue; + this.#sessionIDFromCookie = true; + } else { + this.#sessionID = crypto.randomUUID(); + } + } return this.#sessionID; } diff --git a/packages/astro/test/fixtures/sessions/src/pages/login-safe.ts b/packages/astro/test/fixtures/sessions/src/pages/login-safe.ts new file mode 100644 index 000000000000..6f5c69ca28a0 --- /dev/null +++ b/packages/astro/test/fixtures/sessions/src/pages/login-safe.ts @@ -0,0 +1,8 @@ +import type { APIRoute } from 'astro'; + +export const POST: APIRoute = async (context) => { + const body = await context.request.json() as any; + await context.session.regenerate(); + context.session.set('user', body.username); + return Response.json({ username: body.username }); +}; diff --git a/packages/astro/test/fixtures/sessions/src/pages/login.ts b/packages/astro/test/fixtures/sessions/src/pages/login.ts new file mode 100644 index 000000000000..025cd8fa3856 --- /dev/null +++ b/packages/astro/test/fixtures/sessions/src/pages/login.ts @@ -0,0 +1,7 @@ +import type { APIRoute } from 'astro'; + +export const POST: APIRoute = async (context) => { + const body = await context.request.json() as any; + context.session.set('user', body.username); + return Response.json({ username: body.username }); +}; diff --git a/packages/astro/test/sessions.test.js b/packages/astro/test/sessions.test.js index b65201e43d60..0ea12c40e3ad 100644 --- a/packages/astro/test/sessions.test.js +++ b/packages/astro/test/sessions.test.js @@ -101,6 +101,88 @@ describe('Astro.session', () => { ); }); + it('generates a new session ID when cookie value has no server-side data', async () => { + const unknownId = 'nonexistent-session-id-12345'; + const response = await fetchResponse('/login', { + method: 'POST', + headers: { + 'Content-Type': 'application/json', + cookie: `astro-session=${unknownId}`, + }, + body: JSON.stringify({ username: 'testuser' }), + }); + assert.equal(response.ok, true); + const headers = Array.from(app.setCookieHeaders(response)); + const sessionId = headers[0].split(';')[0].split('=')[1]; + // A new ID should be generated since the cookie value had no stored data + assert.notEqual(sessionId, unknownId, 'Should not adopt a session ID with no stored data'); + + // The original ID should not give access to the new session's data + const secondResponse = await fetchResponse('/update', { + method: 'GET', + headers: { + cookie: `astro-session=${unknownId}`, + }, + }); + const secondData = await secondResponse.json(); + assert.equal(secondData.previousValue, 'none', 'Original ID should not have session data'); + }); + + it('preserves session ID when cookie value has existing server-side data', async () => { + const firstResponse = await fetchResponse('/update'); + 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 secondHeaders = Array.from(app.setCookieHeaders(secondResponse)); + const secondSessionId = secondHeaders[0].split(';')[0].split('=')[1]; + assert.equal(secondSessionId, firstSessionId, 'Valid session ID should be preserved'); + }); + + it('regenerate() creates a new ID and cleans up the old session', async () => { + const firstResponse = await fetchResponse('/update', { + method: 'GET', + }); + const firstHeaders = Array.from(app.setCookieHeaders(firstResponse)); + const firstSessionId = firstHeaders[0].split(';')[0].split('=')[1]; + + const secondResponse = await fetchResponse('/login-safe', { + method: 'POST', + headers: { + 'Content-Type': 'application/json', + cookie: `astro-session=${firstSessionId}`, + }, + body: JSON.stringify({ username: 'testuser' }), + }); + assert.equal(secondResponse.ok, true); + const secondHeaders = Array.from(app.setCookieHeaders(secondResponse)); + const secondSessionIds = secondHeaders + .filter((h) => h.startsWith('astro-session=')) + .map((h) => h.split(';')[0].split('=')[1]); + const secondSessionId = secondSessionIds[secondSessionIds.length - 1]; + + assert.notEqual(secondSessionId, firstSessionId, 'regenerate() should create new ID'); + + // Old session ID should no longer have data after regeneration + const thirdResponse = await fetchResponse('/update', { + method: 'GET', + headers: { + cookie: `astro-session=${firstSessionId}`, + }, + }); + const thirdData = await thirdResponse.json(); + assert.equal( + thirdData.previousValue, + 'none', + 'Old session ID should have no data after regeneration', + ); + }); + it('can load a session by ID', async () => { const firstResponse = await fetchResponse('/_actions/addToCart', { method: 'POST',