From 4a6d42767313ea09c1d92156b309a561da2f1da1 Mon Sep 17 00:00:00 2001 From: Chris Raible Date: Thu, 6 Jun 2024 16:28:36 -0700 Subject: [PATCH] Removed members caching cookies when no member is logged in (#20349) ref https://linear.app/tryghost/issue/KTLO-58/dont-send-ghost-acess-cookies-if-no-member-is-logged-in - Currently when member's caching is enabled, but no member is logged in, we always send `ghost-access=null;` and `ghost-access-hmac=null;` cookies in the requests to `/members/api/member/`. This is done to clear the cookies, but an unintended consequence is that these requests can never be cached since there is a cookie in the response. - This PR removes the cookies from the requests when no member is logged in, the cookies will not be sent, allowing the requests to be cached - It also unsets the cookies when deleting a member's session, so that the cookies are not sent in the requests after the member logs out - This should improve the cache hit ratio with members caching enabled --- .../server/services/members/middleware.js | 15 ++++-- .../__snapshots__/middleware.test.js.snap | 52 +++++++++++++++++++ .../test/e2e-api/members/middleware.test.js | 32 ++++++++++++ ghost/members-ssr/lib/members-ssr.js | 5 ++ 4 files changed, 100 insertions(+), 4 deletions(-) diff --git a/ghost/core/core/server/services/members/middleware.js b/ghost/core/core/server/services/members/middleware.js index d1f3ee6efd..e31d407a90 100644 --- a/ghost/core/core/server/services/members/middleware.js +++ b/ghost/core/core/server/services/members/middleware.js @@ -29,12 +29,19 @@ const getFreeTier = async function getFreeTier() { /** * Sets the ghost-access and ghost-access-hmac cookies on the response object - * @param {object} member - The member object + * @param {Object} member - The member object + * @param {import('express').Request} req - The member object * @param {import('express').Response} res - The express response object to set the cookies on + * @param {Object} freeTier - The free tier object * @returns */ -const setAccessCookies = function setAccessCookies(member = undefined, res, freeTier) { +const setAccessCookies = function setAccessCookies(member, req, res, freeTier) { if (!member) { + // If there is no cookie sent with the request, return early + if (!req.headers.cookie || !req.headers.cookie.includes('ghost-access')) { + return; + } + // If there are cookies sent with the request, set them to null and expire them immediately const accessCookie = `ghost-access=null; Max-Age=0; Path=/; HttpOnly; SameSite=Strict;`; const hmacCookie = `ghost-access-hmac=null; Max-Age=0; Path=/; HttpOnly; SameSite=Strict;`; const existingCookies = res.getHeader('Set-Cookie') || []; @@ -70,7 +77,7 @@ const setAccessCookies = function setAccessCookies(member = undefined, res, free const accessInfoSession = async function accessInfoSession(req, res, next) { const freeTier = await getFreeTier(); onHeaders(res, function () { - setAccessCookies(req.member, res, freeTier); + setAccessCookies(req.member, req, res, freeTier); }); next(); }; @@ -304,7 +311,7 @@ const createSessionFromMagicLink = async function createSessionFromMagicLink(req // Set the ghost-access cookies to enable tier-based caching try { const freeTier = await getFreeTier(); - setAccessCookies(member, res, freeTier); + setAccessCookies(member, req, res, freeTier); } catch { // This is a non-critical operation, so we can safely ignore any errors } diff --git a/ghost/core/test/e2e-api/members/__snapshots__/middleware.test.js.snap b/ghost/core/test/e2e-api/members/__snapshots__/middleware.test.js.snap index 07de1e638b..252480f8d0 100644 --- a/ghost/core/test/e2e-api/members/__snapshots__/middleware.test.js.snap +++ b/ghost/core/test/e2e-api/members/__snapshots__/middleware.test.js.snap @@ -461,6 +461,58 @@ Object { } `; +exports[`Comments API when caching members content is enabled sets ghost-access and ghost-access-hmac cookies to null when not authenticated but a cookie is sent 1: [body] 1`] = ` +Object { + "avatar_image": null, + "created_at": StringMatching /\\\\d\\{4\\}-\\\\d\\{2\\}-\\\\d\\{2\\}T\\\\d\\{2\\}:\\\\d\\{2\\}:\\\\d\\{2\\}\\\\\\.000Z/, + "email": "member@example.com", + "email_suppression": Object { + "info": null, + "suppressed": false, + }, + "enable_comment_notifications": true, + "expertise": null, + "firstname": null, + "name": null, + "newsletters": Array [ + Object { + "description": null, + "id": StringMatching /\\[a-f0-9\\]\\{24\\}/, + "name": "Default Newsletter", + "sort_order": 0, + "uuid": StringMatching /\\[a-f0-9\\]\\{8\\}-\\[a-f0-9\\]\\{4\\}-\\[a-f0-9\\]\\{4\\}-\\[a-f0-9\\]\\{4\\}-\\[a-f0-9\\]\\{12\\}/, + }, + Object { + "description": null, + "id": StringMatching /\\[a-f0-9\\]\\{24\\}/, + "name": "Weekly newsletter", + "sort_order": 2, + "uuid": StringMatching /\\[a-f0-9\\]\\{8\\}-\\[a-f0-9\\]\\{4\\}-\\[a-f0-9\\]\\{4\\}-\\[a-f0-9\\]\\{4\\}-\\[a-f0-9\\]\\{12\\}/, + }, + ], + "paid": false, + "subscribed": false, + "subscriptions": Array [], + "uuid": StringMatching /\\[a-f0-9\\]\\{8\\}-\\[a-f0-9\\]\\{4\\}-\\[a-f0-9\\]\\{4\\}-\\[a-f0-9\\]\\{4\\}-\\[a-f0-9\\]\\{12\\}/, +} +`; + +exports[`Comments API when caching members content is enabled sets ghost-access and ghost-access-hmac cookies to null when not authenticated but a cookie is sent 2: [headers] 1`] = ` +Object { + "access-control-allow-origin": "*", + "cache-control": "no-cache, private, no-store, must-revalidate, max-stale=0, post-check=0, pre-check=0", + "content-length": "621", + "content-type": "application/json; charset=utf-8", + "etag": StringMatching /\\(\\?:W\\\\/\\)\\?"\\(\\?:\\[ !#-\\\\x7E\\\\x80-\\\\xFF\\]\\*\\|\\\\r\\\\n\\[\\\\t \\]\\|\\\\\\\\\\.\\)\\*"/, + "set-cookie": Array [ + StringMatching /\\^ghost-access=\\[0-9a-fA-F\\]\\{24\\}:\\\\d\\{10\\}/, + StringMatching /\\^ghost-access-hmac=\\[a-fA-F0-9\\]\\{64\\}/, + ], + "vary": "Accept-Encoding", + "x-powered-by": "Express", +} +`; + exports[`Comments API when not authenticated but enabled can update comment notifications 1: [body] 1`] = ` Object { "email": "member1@test.com", diff --git a/ghost/core/test/e2e-api/members/middleware.test.js b/ghost/core/test/e2e-api/members/middleware.test.js index a90b3dfab0..7ce1e96bfb 100644 --- a/ghost/core/test/e2e-api/members/middleware.test.js +++ b/ghost/core/test/e2e-api/members/middleware.test.js @@ -249,5 +249,37 @@ describe('Comments API', function () { body.email.should.eql(member.get('email')); }); }); + + it('does not set ghost-access and ghost-access-hmac cookies when not authenticated', async function () { + configUtils.set('cacheMembersContent:enabled', true); + configUtils.set('cacheMembersContent:hmacSecret', crypto.randomBytes(64).toString('base64')); + membersAgent = await agentProvider.getMembersAPIAgent(); + await fixtureManager.init('newsletters', 'members:newsletters'); + await membersAgent + .get(`/api/member/`) + .expectStatus(204) + .expectEmptyBody() + .expect(({headers}) => { + should.not.exist(headers['set-cookie']); + }); + }); + + it('sets ghost-access and ghost-access-hmac cookies to null when not authenticated but a cookie is sent', async function () { + // This is to ensure that the cookies are reset when a user logs out + configUtils.set('cacheMembersContent:enabled', true); + configUtils.set('cacheMembersContent:hmacSecret', crypto.randomBytes(64).toString('base64')); + membersAgent = await agentProvider.getMembersAPIAgent(); + await fixtureManager.init('newsletters', 'members:newsletters'); + // Send a ghost-access cookie but without a valid member session + await membersAgent.jar.setCookie('ghost-access=fake;'); + await membersAgent + .get('/api/member/') + .expect(({headers}) => { + should.exist(headers['set-cookie']); + headers['set-cookie'].should.matchAny(/ghost-access=null;/); + }) + .expectStatus(204) + .expectEmptyBody(); + }); }); }); diff --git a/ghost/members-ssr/lib/members-ssr.js b/ghost/members-ssr/lib/members-ssr.js index fed91c8dbd..3511503a4c 100644 --- a/ghost/members-ssr/lib/members-ssr.js +++ b/ghost/members-ssr/lib/members-ssr.js @@ -111,6 +111,11 @@ class MembersSSR { _removeSessionCookie(req, res) { const cookies = this._getCookies(req, res); cookies.set(this.sessionCookieName, null, this.sessionCookieOptions); + // If members caching cookies are set, remove them + if (cookies.get('ghost-access') || cookies.get('ghost-access-hmac')) { + cookies.set('ghost-access', null, {...this.sessionCookieOptions, signed: false}); + cookies.set('ghost-access-hmac', null, {...this.sessionCookieOptions, signed: false}); + } } /**