Improved checkout session request limits#28316
Conversation
WalkthroughThis PR adds brute-force rate limiting to the Stripe checkout session creation endpoint. It introduces two configurable limiters: checkoutSessionGlobal (IP-based) and checkoutSessionEmail (keyed by normalized customer email), implemented as ExpressBrute factories with Knex-backed stores and custom TooManyRequestsError messages. The brute module exposes thin delegation middlewares, the members POST route applies JSON parsing plus both middlewares before the createCheckoutSession handler, and unit tests and fixtures were updated for the new config and exports. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ee41cea9af
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
ghost/core/core/server/web/shared/middleware/brute.js (1)
131-133: 💤 Low valueInconsistency:
checkoutSessionGlobaluses.prevent()without IP/key configuration.The
checkoutSessionGlobalmiddleware uses.prevent()directly (likemembersAuthEnumeration), which means it relies on the default keying behavior. However,membersAuthEnumerationis intended for IP-only enumeration protection.Looking at
globalBlock(lines 12-19), which also does per-route-per-IP blocking, it uses.getMiddleware()withignoreIP: falseand a pathname-based key. The current implementation will work, but for consistency and explicit control over the key (like appending acheckout_globalsuffix to avoid collisions), consider using.getMiddleware()with explicit options.This is a minor observation as express-brute defaults to using
req.ip, so it should work correctly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ghost/core/core/server/web/shared/middleware/brute.js` around lines 131 - 133, checkoutSessionGlobal currently calls spamPrevention.checkoutSessionGlobal().prevent(req,res,next) which relies on default IP/key behavior; update it to use .getMiddleware(...) like globalBlock so you explicitly set ignoreIP: false and provide a pathname-based key (e.g. include req.path plus a "checkout_global" suffix) to avoid key collisions and match the explicit behavior used by globalBlock and membersAuthEnumeration.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@ghost/core/core/server/web/shared/middleware/brute.js`:
- Around line 131-133: checkoutSessionGlobal currently calls
spamPrevention.checkoutSessionGlobal().prevent(req,res,next) which relies on
default IP/key behavior; update it to use .getMiddleware(...) like globalBlock
so you explicitly set ignoreIP: false and provide a pathname-based key (e.g.
include req.path plus a "checkout_global" suffix) to avoid key collisions and
match the explicit behavior used by globalBlock and membersAuthEnumeration.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f80c388b-3c9b-47c9-bcf1-3f13c9a68913
📒 Files selected for processing (4)
ghost/core/core/server/web/members/app.jsghost/core/core/server/web/shared/middleware/api/spam-prevention.jsghost/core/core/server/web/shared/middleware/brute.jsghost/core/test/unit/server/web/shared/middleware/brute.test.js
kevinansfield
force-pushed
the
improved-checkout-session-limits
branch
from
ee41cea to
0fd503e
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
ghost/core/test/unit/server/web/shared/middleware/brute.test.js (2)
36-48: ⚡ Quick winAssert the delegated
prevent()call.This only proves the factory was touched. If
brute.checkoutSessionGlobalstops calling.prevent(req, res, next), the test still passes because the thrownTypeErroris swallowed. Return a fake limiter and assertpreventgets the middleware args.Proposed test tightening
describe('checkoutSessionGlobal', function () { it('calls the checkoutSessionGlobal method of spam prevention', function () { const spamPrevention = require('../../../../../../core/server/web/shared/middleware/api/spam-prevention'); - const checkoutSessionGlobalStub = sinon.stub(spamPrevention, 'checkoutSessionGlobal'); - - try { - brute.checkoutSessionGlobal(); - } catch (err) { - // We only care that the spam prevention method is called. - } finally { - sinon.assert.called(checkoutSessionGlobalStub); - } + const prevent = sinon.stub(); + const checkoutSessionGlobalStub = sinon.stub(spamPrevention, 'checkoutSessionGlobal').returns({prevent}); + const req = {}; + const res = {}; + const next = sinon.stub(); + + brute.checkoutSessionGlobal(req, res, next); + + sinon.assert.calledOnce(checkoutSessionGlobalStub); + sinon.assert.calledOnceWithExactly(prevent, req, res, next); }); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ghost/core/test/unit/server/web/shared/middleware/brute.test.js` around lines 36 - 48, The test currently only stubs spam-prevention.checkoutSessionGlobal and swallows the TypeError, so update it to return a fake limiter object from the stub that has a prevent method (e.g., const fakeLimiter = { prevent: sinon.stub() }) when stubbing spam-prevention.checkoutSessionGlobal, call brute.checkoutSessionGlobal with fake req, res, next, and then assert that fakeLimiter.prevent was called with those req, res, next arguments; keep references to brute.checkoutSessionGlobal, the spam-prevention checkoutSessionGlobal stub, and the fakeLimiter.prevent stub to locate the changes.
51-64: ⚡ Quick winCover the email-keying contract.
This test has the same gap as the global one, and it also misses the important behavior in
ghost/core/core/server/web/shared/middleware/brute.js: trimming/lowercasingcustomerEmailbefore building the limiter key. Without asserting that, a casing/whitespace regression could bypass the per-email limiter unnoticed.Proposed coverage upgrade
describe('checkoutSessionEmail', function () { it('calls the checkoutSessionEmail method of spam prevention', function () { const spamPrevention = require('../../../../../../core/server/web/shared/middleware/api/spam-prevention'); - const checkoutSessionEmailStub = sinon.stub(spamPrevention, 'checkoutSessionEmail'); - - try { - brute.checkoutSessionEmail(); - } catch (err) { - // We only care that the spam prevention method is called. - } finally { - sinon.assert.called(checkoutSessionEmailStub); - } + const getMiddleware = sinon.stub().callsFake(({key}) => { + return (req, res, next) => key(req, res, (value) => { + assert.equal(value, 'test@example.comcheckout'); + next(); + }); + }); + const checkoutSessionEmailStub = sinon.stub(spamPrevention, 'checkoutSessionEmail').returns({getMiddleware}); + const req = {body: {customerEmail: ' Test@Example.com '}}; + const res = {}; + const next = sinon.stub(); + + brute.checkoutSessionEmail(req, res, next); + + sinon.assert.calledOnce(checkoutSessionEmailStub); + sinon.assert.calledOnce(getMiddleware); }); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ghost/core/test/unit/server/web/shared/middleware/brute.test.js` around lines 51 - 64, Update the test to assert the email normalization contract: call brute.checkoutSessionEmail with a mock request whose body.customerEmail contains mixed case and surrounding whitespace (e.g. " Foo@Example.COM "), stub/spy spam-prevention.checkoutSessionEmail and verify it was called with the trimmed and lowercased email (i.e. "foo@example.com") so the test checks that brute.checkoutSessionEmail performs trimming and lowercasing before building the limiter key.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@ghost/core/test/unit/server/web/shared/middleware/brute.test.js`:
- Around line 36-48: The test currently only stubs
spam-prevention.checkoutSessionGlobal and swallows the TypeError, so update it
to return a fake limiter object from the stub that has a prevent method (e.g.,
const fakeLimiter = { prevent: sinon.stub() }) when stubbing
spam-prevention.checkoutSessionGlobal, call brute.checkoutSessionGlobal with
fake req, res, next, and then assert that fakeLimiter.prevent was called with
those req, res, next arguments; keep references to brute.checkoutSessionGlobal,
the spam-prevention checkoutSessionGlobal stub, and the fakeLimiter.prevent stub
to locate the changes.
- Around line 51-64: Update the test to assert the email normalization contract:
call brute.checkoutSessionEmail with a mock request whose body.customerEmail
contains mixed case and surrounding whitespace (e.g. " Foo@Example.COM "),
stub/spy spam-prevention.checkoutSessionEmail and verify it was called with the
trimmed and lowercased email (i.e. "foo@example.com") so the test checks that
brute.checkoutSessionEmail performs trimming and lowercasing before building the
limiter key.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 97a76339-0c6f-4398-b84d-90f53103bf9a
📒 Files selected for processing (6)
ghost/core/core/server/web/members/app.jsghost/core/core/server/web/shared/middleware/api/spam-prevention.jsghost/core/core/server/web/shared/middleware/brute.jsghost/core/core/shared/config/defaults.jsonghost/core/test/unit/server/web/shared/middleware/brute.test.jsghost/core/test/utils/fixtures/config/defaults.json
✅ Files skipped from review due to trivial changes (1)
- ghost/core/test/utils/fixtures/config/defaults.json
🚧 Files skipped from review as they are similar to previous changes (3)
- ghost/core/core/server/web/members/app.js
- ghost/core/core/server/web/shared/middleware/brute.js
- ghost/core/core/server/web/shared/middleware/api/spam-prevention.js
no issue Checkout session creation now has dedicated request limits so checkout traffic can be tuned independently from sign-in traffic.
kevinansfield
force-pushed
the
improved-checkout-session-limits
branch
from
0fd503e to
ac2c3d6
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ghost/core/core/server/web/members/app.js`:
- Around line 104-108: The route handler registration calls bodyParser.json()
before the IP-based global limiter; change the middleware order so
shared.middleware.brute.checkoutSessionGlobal runs before bodyParser.json() to
ensure IP-level rate limiting occurs prior to any JSON parsing work. Update the
membersApp.post registration that defines '/api/create-stripe-checkout-session'
to list shared.middleware.brute.checkoutSessionGlobal (and then
checkoutSessionEmail) before bodyParser.json() so the global limiter can
short-circuit abusive requests early.
🪄 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: f6f569e9-5a34-4024-86e8-94ba1e0976fa
📒 Files selected for processing (6)
ghost/core/core/server/web/members/app.jsghost/core/core/server/web/shared/middleware/api/spam-prevention.jsghost/core/core/server/web/shared/middleware/brute.jsghost/core/core/shared/config/defaults.jsonghost/core/test/unit/server/web/shared/middleware/brute.test.jsghost/core/test/utils/fixtures/config/defaults.json
✅ Files skipped from review due to trivial changes (2)
- ghost/core/core/shared/config/defaults.json
- ghost/core/test/utils/fixtures/config/defaults.json
🚧 Files skipped from review as they are similar to previous changes (3)
- ghost/core/test/unit/server/web/shared/middleware/brute.test.js
- ghost/core/core/server/web/shared/middleware/brute.js
- ghost/core/core/server/web/shared/middleware/api/spam-prevention.js
| message: `Too many checkout attempts, try again in ${moment(nextValidRequestDate).fromNow(true)}`, | ||
| context: tpl(messages.tooManySigninAttempts.context), | ||
| help: tpl(messages.tooManySigninAttempts.context) | ||
| })); |
There was a problem hiding this comment.
nit: the context / help messages read as "Too many login attempts.", let's change it to "Too many checkout attempts." ?
| } | ||
|
|
||
| return spamPrevention.checkoutSessionEmail().getMiddleware({ | ||
| ignoreIP: false, |
There was a problem hiding this comment.
question: should we set ignoreIP to true? That way, same email but different IP is counted in the same bucket:
1.2.3.4 + test@example.com -> counter A
5.6.7.8 + test@example.com -> counter A
9.9.9.9 + test@example.com -> counter A
We do that for OTC verification too:
Ghost/ghost/core/core/server/web/shared/middleware/brute.js
Lines 160 to 161 in ac2c3d6
2 participants
What changed
Why
Checkout session creation has different traffic patterns from sign-in, so it should be tunable independently instead of sharing sign-in request limits.
Validation
pnpm --dir ghost/core test:single test/unit/server/web/shared/middleware/brute.test.js