Conversation
ref https://ghost.slack.com/archives/C018EKC56JF/p1771527734858719 The existing JWT is used for identity and only contains the email address. It has a TTL of 10 minutes which is reasonable for the use case. We have another use case where integrations often want to validate the source, but are left with no option but to hit the members Admin API endpoint with an Admin API key because the email isn't sufficient to know if the member has access to a tier, is paid, isn't specific to email (which could change), etc., all of which are used for integrations.
WalkthroughAdds member entitlement-token support: TokenService gains 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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. Comment |
9larsons
changed the title
cathysarisky
left a comment
cathysarisky
left a comment
There was a problem hiding this comment.
This would solve a number of cases (only some of which I wrote) where an external integration uses the jwt to validate who the member is (based on email), but then has to use an admin api key for whatever information is needed. I'd love to reduce the need to store an admin api key when all I need is to read a little information about the member, like a non-changing ID, or what tier's privs they should get.
![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/core/server/services/members/members-ssr.js (1)
209-212: Missing JSDoc on both new methods — inconsistent with the rest of the class.Every other
_privateand public method inMembersSSRcarries JSDoc. The new_getMemberEntitlementTokenandgetEntitlementTokenForMemberFromSessionare the only ones without it.📝 Proposed JSDoc additions
+ /** + * `@method` _getMemberEntitlementToken + * + * `@param` {string} transientId + * + * `@returns` {Promise<JWT>} entitlement token + */ async _getMemberEntitlementToken(transientId) { const api = await this._getMembersApi(); return api.getMemberEntitlementToken(transientId); }+ /** + * `@method` getEntitlementTokenForMemberFromSession + * + * `@param` {Request} req + * `@param` {Response} res + * + * `@returns` {Promise<JWT>} entitlement token + */ async getEntitlementTokenForMemberFromSession(req, res) {Also applies to: 331-341
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/core/server/services/members/members-ssr.js` around lines 209 - 212, Add JSDoc comments for the two new methods to match the rest of MembersSSR: add a JSDoc block above async _getMemberEntitlementToken(transientId) describing the method (private helper to retrieve an entitlement token via this._getMembersApi), document the transientId parameter type and purpose, and the Promise return type; do the same for getEntitlementTokenForMemberFromSession(session) (describe that it extracts member info from a session and returns an entitlement token, document the session parameter and return Promise<string|null> or appropriate type). Ensure tags (`@private` or `@public` as appropriate, `@param`, `@returns`) and wording/style follow existing JSDoc conventions used in MembersSSR.ghost/core/test/e2e-frontend/members.test.js (1)
122-136: Strengthenactive_tier_idsassertion and add a paid-member coverage case.Two gaps in the current test:
Line 134 only asserts the field is an array. For
member1@test.com(free member) it should always be[]— assert the exact value to prevent silent regressions.There is no test covering a paid member, which is the only scenario where
activeTierIdsis non-empty and whereisActiveSubscriptionStatusis actually invoked. Given the concern about that method's existence (seemembers-api.js), a paid-member scenario is the critical missing coverage.♻️ Tighten existing assertion
- assert(Array.isArray(decodedToken.active_tier_ids)); + assert.deepEqual(decodedToken.active_tier_ids, []);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/test/e2e-frontend/members.test.js` around lines 122 - 136, Update the existing test "returns an entitlement token for a valid member session" to assert that decodedToken.active_tier_ids is exactly [] for the free member returned by loginAsMember('member1@test.com') (use jwt.decode(res.text) as already done), and then add a separate test case that logs in a paid member (e.g., loginAsMember for a paid account or create a paid member fixture), calls request.get('/members/api/entitlements'), decodes the token with jwt.decode, and asserts decodedToken.paid is true and decodedToken.active_tier_ids is non-empty (and contains the expected tier UUID(s)); keep using the same decodedToken field checks (sub, scope, member_uuid, exp-iat) to validate the paid scenario as well.
🤖 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/members-ssr.js`:
- Around line 209-212: Add JSDoc comments for the two new methods to match the
rest of MembersSSR: add a JSDoc block above async
_getMemberEntitlementToken(transientId) describing the method (private helper to
retrieve an entitlement token via this._getMembersApi), document the transientId
parameter type and purpose, and the Promise return type; do the same for
getEntitlementTokenForMemberFromSession(session) (describe that it extracts
member info from a session and returns an entitlement token, document the
session parameter and return Promise<string|null> or appropriate type). Ensure
tags (`@private` or `@public` as appropriate, `@param`, `@returns`) and wording/style
follow existing JSDoc conventions used in MembersSSR.
In `@ghost/core/test/e2e-frontend/members.test.js`:
- Around line 122-136: Update the existing test "returns an entitlement token
for a valid member session" to assert that decodedToken.active_tier_ids is
exactly [] for the free member returned by loginAsMember('member1@test.com')
(use jwt.decode(res.text) as already done), and then add a separate test case
that logs in a paid member (e.g., loginAsMember for a paid account or create a
paid member fixture), calls request.get('/members/api/entitlements'), decodes
the token with jwt.decode, and asserts decodedToken.paid is true and
decodedToken.active_tier_ids is non-empty (and contains the expected tier
UUID(s)); keep using the same decodedToken field checks (sub, scope,
member_uuid, exp-iat) to validate the paid scenario as well.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
ghost/core/core/server/services/members/members-api/members-api.jsghost/core/core/server/services/members/members-api/services/token-service.jsghost/core/core/server/services/members/members-ssr.jsghost/core/core/server/services/members/middleware.jsghost/core/core/server/web/members/app.jsghost/core/test/e2e-frontend/members.test.jsghost/core/test/unit/server/services/members/members-api/services/token-service.test.js
rob-ghost
left a comment
rob-ghost
left a comment
There was a problem hiding this comment.
Question: What are the alternatives considered and why is the JWT approach the best one?
Question: What's the blast radius of this change? Goes this affect any of the following:
- Ghost(pro)
- Self-hosters
- Clients (first-party, third-party)
- Other teams - do any docs / schemas need updating?
Question: Have we considered the security of this? If the JWT was exposed and we're sharing membership subscription status, is that a concern?
ghost/core/core/server/services/members/members-api/services/token-service.js
Outdated
Show resolved
Hide resolved
ghost/core/core/server/services/members/members-api/services/token-service.js
Show resolved
Hide resolved
| return tokenService.encodeEntitlementToken({ | ||
| sub: member.email, | ||
| memberUuid: member.uuid, | ||
| paid: member.status !== 'free', |
There was a problem hiding this comment.
Question: Should we derive paid from activeTierIds? Could they disagree (perhaps in a comp'd scenario where they're not free, but have no active tiers, depending on how comps are implemented)?
There was a problem hiding this comment.
They should already be in agreement, afaict. If someone is comped they'll have an active tier id, and anyone free should have an empty active tier id.
There was a problem hiding this comment.
I /think that a subscription with cancel_at_period_end:true still has status:active, correct? Definitely want those members to be "paid" until the subscription actually cancels.
|
"Question: Have we considered the security of this? If the JWT was exposed and we're sharing membership subscription status, is that a concern?" Not my PR, but I'll comment here. If a user has a membership somewhere embarrassing and that fact leaks, that's bad. The existing /members/api/session already has the member's email and that a membership exists. Is it a marginally worse leak if it's a paid membership? I guess arguably? But... in the current situation, every integration that needs access to any membership data beyond what's in the current JWT is asking the site owner to give them unscoped admin api access. There are a lot of admin API keys out there that don't need to be, because there's not currently another way to figure out basic user entitlements. The blast radius on a hypothetical admin api key leak from an integration is significant, and would include the ability to exfiltrate all membership data, send phishing/malware emails, deface the site, etc. In the absence of being able to do tightly-scoped keys, I think this is a net security enhancement. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
ghost/core/core/server/services/members/members-api/services/token-service.js (1)
39-51:aud == issmay trip up external integration verifiersBoth
audienceandissuerare set tothis._issuer(the Ghost members API URL). For the existing identity token this is fine since it's consumed within Ghost, but the entitlement token is explicitly designed for external integrations to independently verify. JWT libraries that strictly enforce theaudclaim will require callers to configurethis._issuer(e.g.https://example.com/members/api) as the expected audience — which is an unusual value for a token addressed to a third-party integration.If the token is ultimately meant to be verified by external services (Discord bots, membership-gated platforms, etc.), consider either: (a) making the audience configurable per-call, or (b) documenting that verifiers must set
audience: <ghost-site-url>/members/apito accept the token.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/core/server/services/members/members-api/services/token-service.js` around lines 39 - 51, The JWT audience is currently set to this._issuer in the jwt.sign call (the entitlement token block using jwk.kid, this._privateKey, algorithm 'RS512'), which forces external verifiers to expect the Ghost URL as aud; change the jwt.sign invocation to accept a configurable audience parameter (e.g., pass an optional audience arg into the entitlement token generation method or options object) and use that value for the audience claim instead of always this._issuer, falling back to a sensible default if not provided, and update the entitlement-token generation method signature and any callers to pass the intended external audience when issuing tokens.
🤖 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/core/server/services/members/members-api/services/token-service.js`:
- Line 45: The jti claim is being set with crypto.randomUUID() in
token-service.js but there's no server-side blocklist/replay-protection
implemented; add a short clarifying comment next to the jti assignment (where
jti: crypto.randomUUID() is set in the token generation logic) stating that jti
is used for observability/audit only and that no replay protection/blocklist is
in place (and optionally note how to add one later, e.g., store jti in a
short-lived blocklist on logout/revoke). Ensure the comment mentions the symbols
jti and crypto.randomUUID() so future contributors don't assume replay
protection is provided.
---
Nitpick comments:
In
`@ghost/core/core/server/services/members/members-api/services/token-service.js`:
- Around line 39-51: The JWT audience is currently set to this._issuer in the
jwt.sign call (the entitlement token block using jwk.kid, this._privateKey,
algorithm 'RS512'), which forces external verifiers to expect the Ghost URL as
aud; change the jwt.sign invocation to accept a configurable audience parameter
(e.g., pass an optional audience arg into the entitlement token generation
method or options object) and use that value for the audience claim instead of
always this._issuer, falling back to a sensible default if not provided, and
update the entitlement-token generation method signature and any callers to pass
the intended external audience when issuing tokens.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
ghost/core/core/server/services/members/members-api/members-api.jsghost/core/core/server/services/members/members-api/services/token-service.jsghost/core/test/e2e-frontend/members.test.jsghost/core/test/unit/server/services/members/members-api/services/token-service.test.js
🚧 Files skipped from review as they are similar to previous changes (3)
- ghost/core/core/server/services/members/members-api/members-api.js
- ghost/core/test/e2e-frontend/members.test.js
- ghost/core/test/unit/server/services/members/members-api/services/token-service.test.js
| scope: 'members:entitlements:read', | ||
| paid, | ||
| active_tier_ids: activeTierIds, | ||
| jti: crypto.randomUUID() |
There was a problem hiding this comment.
jti is present but there is no replay-protection backing it
crypto.randomUUID() provides a unique jti per token, which is good for auditability. However, without a server-side jti blocklist, the claim offers no actual replay-protection within the 5-minute validity window. This is fine if jti is intentionally included only for observability/logging purposes, but it would be worth a brief comment to set expectations and prevent a future contributor from assuming replay protection is already in place.
📝 Suggested clarifying comment
return jwt.sign({
sub,
kid: jwk.kid,
scope: 'members:entitlements:read',
paid,
active_tier_ids: activeTierIds,
- jti: crypto.randomUUID()
+ jti: crypto.randomUUID() // unique token id for auditability; no server-side blocklist, so replay protection within the 5m window is not enforced
}, this._privateKey, {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@ghost/core/core/server/services/members/members-api/services/token-service.js`
at line 45, The jti claim is being set with crypto.randomUUID() in
token-service.js but there's no server-side blocklist/replay-protection
implemented; add a short clarifying comment next to the jti assignment (where
jti: crypto.randomUUID() is set in the token generation logic) stating that jti
is used for observability/audit only and that no replay protection/blocklist is
in place (and optionally note how to add one later, e.g., store jti in a
short-lived blocklist on logout/revoke). Ensure the comment mentions the symbols
jti and crypto.randomUUID() so future contributors don't assume replay
protection is provided.
cathysarisky
left a comment
cathysarisky
left a comment
There was a problem hiding this comment.
I think the ideal (from an integrations standpoint) jwt would contain both the uuid and the member email. Integrations may need either or both, and it'd be annoying to have to get both this jwt and the session jwt to get them. I don't have any strong opinion on which one occupies the 'sub' spot.
| return tokenService.encodeEntitlementToken({ | ||
| sub: member.email, | ||
| memberUuid: member.uuid, | ||
| paid: member.status !== 'free', |
There was a problem hiding this comment.
I /think that a subscription with cancel_at_period_end:true still has status:active, correct? Definitely want those members to be "paid" until the subscription actually cancels.
ghost/core/test/unit/server/services/members/members-api/services/token-service.test.js
Outdated
Show resolved
Hide resolved
This reverts commit 21dbff8.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
ghost/core/core/server/services/members/members-api/services/token-service.js (1)
47-47:jtihas no server-side replay protection backing it.
jtican be used to prevent a JWT from being replayed, but without a server-side blocklist there is no actual enforcement within the 5-minute window. If this is intentional (i.e.,jtiis included only for auditability/traceability), a brief inline comment would prevent future contributors from assuming replay protection is already in place.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/core/server/services/members/members-api/services/token-service.js` at line 47, The jti field (jti: crypto.randomUUID()) in token-service.js is currently not backed by any server-side replay/blocklist; either implement server-side enforcement (e.g., persist jti to a short-lived blocklist store like Redis on revoke and check it during token validation) or, if jti is only for audit/traceability, add a clear inline comment next to the jti property stating that no replay protection is enforced and why (and reference the intended TTL/validation behavior); locate the jti occurrence (the jti: crypto.randomUUID() line) in token-service.js and apply one of these two fixes so future contributors won’t assume replay protection exists.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@ghost/core/core/server/services/members/members-api/services/token-service.js`:
- Line 47: The jti field (jti: crypto.randomUUID()) in token-service.js is
currently not backed by any server-side replay/blocklist; either implement
server-side enforcement (e.g., persist jti to a short-lived blocklist store like
Redis on revoke and check it during token validation) or, if jti is only for
audit/traceability, add a clear inline comment next to the jti property stating
that no replay protection is enforced and why (and reference the intended
TTL/validation behavior); locate the jti occurrence (the jti:
crypto.randomUUID() line) in token-service.js and apply one of these two fixes
so future contributors won’t assume replay protection exists.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
ghost/core/core/server/services/members/members-api/members-api.jsghost/core/core/server/services/members/members-api/services/token-service.jsghost/core/test/e2e-frontend/members.test.jsghost/core/test/unit/server/services/members/members-api/services/token-service.test.js
🚧 Files skipped from review as they are similar to previous changes (2)
- ghost/core/test/e2e-frontend/members.test.js
- ghost/core/test/unit/server/services/members/members-api/services/token-service.test.js
3 participants
ref https://ghost.slack.com/archives/C018EKC56JF/p1771527734858719
The existing JWT is used for identity and only contains the email address. It has a TTL of 10 minutes which is reasonable for the use case but little else.
We have another use case where integrations often want to validate the source, but are left with no option but to hit the members Admin API endpoint with an Admin API key because the email isn't sufficient to know if the member has access to a tier, is paid, isn't specific to email (which could change), etc., all of which are used for integrations.
This endpoint would still require the members session cookie and has the following props, e.g.: