Fixed member session cookies not scoped to subdirectory path#27554
Conversation
Ghost supports subdirectory installs via the `url` config (e.g. `https://example.com/tr/`), and some hosting setups run two Ghost sites on the same domain — one at the root path and one in a subdirectory. In this scenario, member session cookies from the subdirectory site are sent to the root site on every request (because the cookie Path defaults to `/`). The root site cannot validate the foreign cookie and immediately expires it with a `Set-Cookie: ghost-members-ssr=; Max-Age=0; Path=/`, which deletes the member's session on the subdirectory site. The admin session cookie (`ghost-admin-api-session`) already handles this correctly by setting `path: urlUtils.getSubdir() + '/ghost'`. This commit applies the same treatment to the members-side cookies: - `ghost-members-ssr` (primary member session): pass `cookiePath` derived from `urlUtils.getSubdir()` when constructing `MembersSSR` in `service.js`. The option already exists in the `@tryghost/members-ssr` package but was never wired up. - `ghost-access` / `ghost-access-hmac` (cache tier cookies in `middleware.js`): replace hardcoded `Path=/` string literals with a value derived from `urlUtils.getSubdir()`. For root installs `getSubdir()` returns `''`, so the path falls back to `'/'` — no behaviour change for the common case.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughCookie Path handling for members cookies now uses 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
ghost/core/test/unit/server/services/members/middleware.test.js (1)
324-423: LGTM — covers both the set and clear branches across root vs subdir.Tests directly assert the regression is fixed for: (1) active free-tier subscription (cookie set with
Path=/vsPath=/subdir), and (2)member === nullclearing path (ghost-access=nullwith the correspondingPath). Triggering theon-headerscallback viares.writeHead(200)is a clean way to exercisesetAccessCookieswithout exporting it.A couple of minor cleanups you can take or leave:
- Line 346:
onHeaders(res, function () {});is redundant —accessInfoSessionitself registers anonHeaderslistener, which already patchesres.writeHead.- Lines 401 and 414:
res.getHeader.returns([])re-stubs the same default already set inbeforeEach(line 339); these lines can be removed.♻️ Optional cleanup
- // on-headers patches writeHead so registered callbacks fire before it - onHeaders(res, function () {}); next = sinon.stub();it('clears cookies with Path=/ for root site installs', async function () { sinon.stub(urlUtils, 'getSubdir').returns(''); req.headers.cookie = 'ghost-access=stale'; - res.getHeader.returns([]); await runAndFlushHeaders(null);it('clears cookies with Path=/subdir for subdirectory site installs', async function () { sinon.stub(urlUtils, 'getSubdir').returns('/subdir'); req.headers.cookie = 'ghost-access=stale'; - res.getHeader.returns([]); await runAndFlushHeaders(null);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/test/unit/server/services/members/middleware.test.js` around lines 324 - 423, Remove the redundant on-headers patch and duplicate stubs: delete the explicit onHeaders(res, function () {}) call (it's unnecessary because membersMiddleware.accessInfoSession already registers an onHeaders listener) and remove the duplicated res.getHeader.returns([]) calls inside the two "clears cookies" tests (they repeat the beforeEach default). Keep the rest of the tests and the runAndFlushHeaders helper unchanged.ghost/core/core/server/services/members/middleware.js (1)
47-49: LGTM —Pathcorrectly derived from subdir on both clear and set paths.Both branches now scope
ghost-access/ghost-access-hmacto the configured subdirectory, matching the newcookiePathpassed toMembersSSRinservice.js. RFC 6265 path matching makes a value likePath=/tr(no trailing slash) correctly match/tr/...without leaking into siblings such as/track, so the format is safe.Optional: the expression
urlUtils.getSubdir() || '/'is now repeated three times acrossservice.js:137,middleware.js:47, andmiddleware.js:72. If you'd like, factor it into a small helper (e.g.urlUtils.getCookiePath()or a localgetCookiePath()) so the fallback logic only lives in one place.Also applies to: 72-74
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/core/server/services/members/middleware.js` around lines 47 - 49, The repeated expression urlUtils.getSubdir() || '/' is duplicated across MembersSSR in service.js and the cookie-setting/clearing code in middleware.js (where ghost-access and ghost-access-hmac Path is computed); create a single helper (either add urlUtils.getCookiePath() or a local getCookiePath()) that returns urlUtils.getSubdir() || '/' and replace the three occurrences (the call in MembersSSR in service.js and the two cookie Path computations in middleware.js) to use that helper so the fallback logic is centralized.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@ghost/core/core/server/services/members/middleware.js`:
- Around line 47-49: The repeated expression urlUtils.getSubdir() || '/' is
duplicated across MembersSSR in service.js and the cookie-setting/clearing code
in middleware.js (where ghost-access and ghost-access-hmac Path is computed);
create a single helper (either add urlUtils.getCookiePath() or a local
getCookiePath()) that returns urlUtils.getSubdir() || '/' and replace the three
occurrences (the call in MembersSSR in service.js and the two cookie Path
computations in middleware.js) to use that helper so the fallback logic is
centralized.
In `@ghost/core/test/unit/server/services/members/middleware.test.js`:
- Around line 324-423: Remove the redundant on-headers patch and duplicate
stubs: delete the explicit onHeaders(res, function () {}) call (it's unnecessary
because membersMiddleware.accessInfoSession already registers an onHeaders
listener) and remove the duplicated res.getHeader.returns([]) calls inside the
two "clears cookies" tests (they repeat the beforeEach default). Keep the rest
of the tests and the runAndFlushHeaders helper unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0a2306dc-2bb6-4f43-bc3b-f492cbb707f5
📒 Files selected for processing (3)
ghost/core/core/server/services/members/middleware.jsghost/core/core/server/services/members/service.jsghost/core/test/unit/server/services/members/middleware.test.js
tiersService.api is null until init() runs, so sinon.stub(tiersService.api, 'browse') throws "Trying to stub property of null". Replace with a direct assignment of a mock object, restored in afterEach.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
ghost/core/test/unit/server/services/members/middleware.test.js (2)
385-385: BrittlePath=/;match relies onPathnot being the last cookie attribute.The assertions require a trailing
;after the path (Path=/;,Path=/subdir;, and the regexesPath=\/;/Path=\/subdir;). If the underlying cookie serializer ever emitsPathas the last attribute (e.g.... HttpOnly; Path=/), every assertion in this block silently fails even though the production behaviour is correct. Anchoring on(;|$)(or matching; Path=/<value>and accepting end-of-string) would make the tests resilient to attribute-order changes without weakening them.Also applies to: 400-400, 412-412, 424-424
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/test/unit/server/services/members/middleware.test.js` at line 385, The test assertions that check cookie strings (e.g., the assert.ok(cookies.some(c => c.includes('Path=/;'))) predicate) are brittle because they require a trailing semicolon; change these checks to accept either a trailing semicolon or end-of-string by using a regex or adjusted includes check (for example test for /Path=\/(;|$)/ or /Path=\/subdir(;|$)/) wherever the current assertions use 'Path=/;' or 'Path=/subdir;'. Update the lambda predicates (cookies.some(c => ...)) in the test cases to use the new regex match so attribute ordering (Path last or not) won’t break the tests.
349-354: Prefersinon.stub(tiersService, 'api').get(...)over direct property mutation.Reassigning
tiersService.apiand manually restoring it inafterEachworks, but the rest of this file usessinon.stub(membersService, 'api').get(() => …)(e.g. line 251) for the same pattern. Switching to that style means you don't needoriginalTiersApi, theafterEachordering becomes irrelevant, andsinon.restore()handles teardown uniformly. Purely a consistency nit — feel free to ignore.Also applies to: 362-362
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/test/unit/server/services/members/middleware.test.js` around lines 349 - 354, The test mutates tiersService.api directly and saves originalTiersApi; instead use sinon.stub(tiersService, 'api').get(() => ({ browse: sinon.stub().resolves({data: [{id: freeTierId, type: 'free'}]}) })) so teardown is handled by sinon.restore() and you can remove originalTiersApi and any manual afterEach restore; apply the same change to the other occurrence at line 362 and ensure any references to tiersService.api in the test read from the stubbed getter.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ghost/core/test/unit/server/services/members/middleware.test.js`:
- Around line 381-400: Update the tests to assert the path (and null-clearing)
for each cookie individually instead of using .some(...); locate the assertions
after runAndFlushHeaders in middleware.test.js and replace the loose check that
inspects the Set-Cookie array with explicit checks that the Set-Cookie entries
for 'ghost-access' and 'ghost-access-hmac' each contain the expected Path (e.g.,
'Path=/' or 'Path=/subdir') and that clearing behavior asserts null for both
cookies; reference the test helper runAndFlushHeaders and the cookie names
'ghost-access' and 'ghost-access-hmac' when making the targeted assertions so
both cookie code paths in middleware.js are verified.
---
Nitpick comments:
In `@ghost/core/test/unit/server/services/members/middleware.test.js`:
- Line 385: The test assertions that check cookie strings (e.g., the
assert.ok(cookies.some(c => c.includes('Path=/;'))) predicate) are brittle
because they require a trailing semicolon; change these checks to accept either
a trailing semicolon or end-of-string by using a regex or adjusted includes
check (for example test for /Path=\/(;|$)/ or /Path=\/subdir(;|$)/) wherever the
current assertions use 'Path=/;' or 'Path=/subdir;'. Update the lambda
predicates (cookies.some(c => ...)) in the test cases to use the new regex match
so attribute ordering (Path last or not) won’t break the tests.
- Around line 349-354: The test mutates tiersService.api directly and saves
originalTiersApi; instead use sinon.stub(tiersService, 'api').get(() => ({
browse: sinon.stub().resolves({data: [{id: freeTierId, type: 'free'}]}) })) so
teardown is handled by sinon.restore() and you can remove originalTiersApi and
any manual afterEach restore; apply the same change to the other occurrence at
line 362 and ensure any references to tiersService.api in the test read from the
stubbed getter.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d587b79f-04ef-4ce0-898c-83bf0ffaf27d
📒 Files selected for processing (1)
ghost/core/test/unit/server/services/members/middleware.test.js
There was a problem hiding this comment.
🧹 Nitpick comments (2)
ghost/core/test/unit/server/services/members/middleware.test.js (2)
339-339: Nit:_headers: {}is unused.The
_headersfield on the mockresis never read by the tests or bysetAccessCookies(which usesres.getHeader/res.setHeader). Safe to drop.🧹 Cleanup
res = { - _headers: {}, getHeader: sinon.stub().returns([]), setHeader: sinon.stub(),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/test/unit/server/services/members/middleware.test.js` at line 339, The test's mock response object includes an unused property `_headers` which is never read by the tests or by setAccessCookies (which interacts via res.getHeader/res.setHeader); remove the `_headers` field from the mock `res` object in the unit tests (references: the mock `res` definition in middleware.test.js and the setAccessCookies codepath) so the mock only exposes the headers via getHeader/setHeader and does not include `_headers`.
373-443: Optional: extract a small helper to deduplicate the 4 tests.Each test repeats the same extraction-and-assertion block (find
Set-Cookie, locate both cookies, assert their presence, then checkPath=...). A tiny helper would make the path/clearing matrix easier to read and extend (e.g., for an HMAC-disabled case in the future).♻️ Example refactor
+ function getAccessCookies() { + const setCookieArgs = res.setHeader.args.find(args => args[0] === 'Set-Cookie'); + assert.ok(setCookieArgs, 'Set-Cookie header should be set'); + const cookies = setCookieArgs[1]; + const accessCookie = cookies.find(c => c.startsWith('ghost-access=')); + const hmacCookie = cookies.find(c => c.startsWith('ghost-access-hmac=')); + assert.ok(accessCookie, 'ghost-access cookie should be set'); + assert.ok(hmacCookie, 'ghost-access-hmac cookie should be set'); + return {accessCookie, hmacCookie}; + } + it('uses Path=/ for root site installs', async function () { sinon.stub(urlUtils, 'getSubdir').returns(''); - - const member = { - subscriptions: [{status: 'active', tier: {id: freeTierId}}] - }; - await runAndFlushHeaders(member); - - const setCookieArgs = res.setHeader.args.find(args => args[0] === 'Set-Cookie'); - assert.ok(setCookieArgs, 'Set-Cookie header should be set'); - const cookies = setCookieArgs[1]; - const accessCookie = cookies.find(c => c.startsWith('ghost-access=')); - const hmacCookie = cookies.find(c => c.startsWith('ghost-access-hmac=')); - assert.ok(accessCookie, 'ghost-access cookie should be set'); - assert.ok(hmacCookie, 'ghost-access-hmac cookie should be set'); - assert.ok(accessCookie.includes('Path=/;'), `Expected Path=/ in ghost-access: ${accessCookie}`); - assert.ok(hmacCookie.includes('Path=/;'), `Expected Path=/ in ghost-access-hmac: ${hmacCookie}`); + await runAndFlushHeaders({subscriptions: [{status: 'active', tier: {id: freeTierId}}]}); + const {accessCookie, hmacCookie} = getAccessCookies(); + assert.ok(accessCookie.includes('Path=/;'), `Expected Path=/ in ghost-access: ${accessCookie}`); + assert.ok(hmacCookie.includes('Path=/;'), `Expected Path=/ in ghost-access-hmac: ${hmacCookie}`); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/test/unit/server/services/members/middleware.test.js` around lines 373 - 443, Extract a small test helper (e.g., checkCookiesPath(expectedPath, isClearing=false)) to replace the duplicated extraction-and-assertion logic used in the four tests that call runAndFlushHeaders; the helper should locate the Set-Cookie header via res.setHeader.args.find, pull the cookies array, find the strings starting with 'ghost-access=' and 'ghost-access-hmac=', assert both exist, and then assert the correct Path value (and for clearing tests assert value null via a regex or startsWith check). Replace repeated blocks in the tests titled "uses Path=..." and "clears cookies..." to call this helper with expectedPath '' mapped to '/' or '/subdir' and isClearing true for the clearing variants; keep existing stubbing of urlUtils.getSubdir and the runAndFlushHeaders calls as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@ghost/core/test/unit/server/services/members/middleware.test.js`:
- Line 339: The test's mock response object includes an unused property
`_headers` which is never read by the tests or by setAccessCookies (which
interacts via res.getHeader/res.setHeader); remove the `_headers` field from the
mock `res` object in the unit tests (references: the mock `res` definition in
middleware.test.js and the setAccessCookies codepath) so the mock only exposes
the headers via getHeader/setHeader and does not include `_headers`.
- Around line 373-443: Extract a small test helper (e.g.,
checkCookiesPath(expectedPath, isClearing=false)) to replace the duplicated
extraction-and-assertion logic used in the four tests that call
runAndFlushHeaders; the helper should locate the Set-Cookie header via
res.setHeader.args.find, pull the cookies array, find the strings starting with
'ghost-access=' and 'ghost-access-hmac=', assert both exist, and then assert the
correct Path value (and for clearing tests assert value null via a regex or
startsWith check). Replace repeated blocks in the tests titled "uses Path=..."
and "clears cookies..." to call this helper with expectedPath '' mapped to '/'
or '/subdir' and isClearing true for the clearing variants; keep existing
stubbing of urlUtils.getSubdir and the runAndFlushHeaders calls as before.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: cfa1363c-2b1e-4dd3-bccf-069de69f31f6
📒 Files selected for processing (1)
ghost/core/test/unit/server/services/members/middleware.test.js
|
Hey @JohnONolan, can someone have a look at this PR? It's kind of a blocker to run multiple Ghost sites under the same domain. Thank you! |
|
On it |
9larsons
left a comment
9larsons
left a comment
There was a problem hiding this comment.
Looks sound, sorry for the delay.
I have some nits on how this is all structured (e.g. why are we setting the path in so many places) but that's well out of scope here!
3 participants
Ghost supports subdirectory installs via the
urlconfig (e.g.https://example.com/tr/), and some hosting setups run two Ghost sites on the same domain — one at the root path and one in a subdirectory. In this scenario, member session cookies from the subdirectory site are sent to the root site on every request (because the cookie Path defaults to/). The root site cannot validate the foreign cookie and immediately expires it with aSet-Cookie: ghost-members-ssr=; Max-Age=0; Path=/, which deletes the member's session on the subdirectory site.The admin session cookie (
ghost-admin-api-session) already handles this correctly by settingpath: urlUtils.getSubdir() + '/ghost'. This commit applies the same treatment to the members-side cookies:ghost-members-ssr(primary member session): passcookiePathderived fromurlUtils.getSubdir()when constructingMembersSSRinservice.js. The option already exists in the@tryghost/members-ssrpackage but was never wired up.ghost-access/ghost-access-hmac(cache tier cookies inmiddleware.js): replace hardcodedPath=/string literals with a value derived fromurlUtils.getSubdir().For root installs
getSubdir()returns'', so the path falls back to'/'— no behaviour change for the common case.Got some code for us? Awesome 🎊!
Please take a minute to explain the change you're making:
Please check your PR against these items:
We appreciate your contribution! 🙏