🔥 Enforced max ?limit of 100 in API requests
#24118
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
WalkthroughA new middleware, 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
2012ed0 to
b225e08
Compare
b225e08 to
2f9043b
Compare
?limit of 100 in API requests
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
ghost/core/test/e2e-api/members-comments/max-limit-cap.test.js (1)
40-40: Fix test description comment.The comment says "browsing posts" but this test is actually browsing comments for a specific post.
- it('should call maxLimitCap middleware when browsing posts', async function () { + it('should call maxLimitCap middleware when browsing comments', async function () {ghost/core/test/unit/server/web/shared/middleware/max-limit-cap.test.js (2)
87-97: Remove misleading comment about skipped test.The comment says this test is skipped, but it's actually running. The
configUtils.set()call should work for testing dynamic config changes.it('should allow "all" when allowLimitAll is true', function () { - // This test is skipped because config values are loaded at module load time - // and cannot be changed dynamically in tests configUtils.set('optimization:allowLimitAll', true);
101-111: Remove misleading comment about skipped test.Similar to the previous test, this comment is misleading since the test is actually running and should work with
configUtils.set().it('should use custom maxLimit value', function () { - // This test is skipped because config values are loaded at module load time - // and cannot be changed dynamically in tests configUtils.set('optimization:maxLimit', 50);ghost/core/test/e2e-api/admin/max-limit-cap.test.js (2)
308-310: Fix inconsistent comment.The comment mentions 14 posts but the assertion allows up to 15, which is inconsistent.
- // Since we have 14 posts in test data, we get all 14 - body.posts.length.should.be.lessThanOrEqual(15); + // limit=0 uses Ghost's default behavior, actual count may vary + body.posts.length.should.be.greaterThan(0);
188-192: Consider more robust assertions for member count tests.The conditional assertion pattern
if (body.members.length === 5)could make tests less reliable. Consider ensuring you have sufficient test data or using more deterministic assertions.- // Even though we requested 10, we should only get max 5 - body.members.length.should.be.lessThanOrEqual(5); - if (body.members.length === 5) { - body.meta.pagination.limit.should.equal(5); - } + // Even though we requested 10, we should only get max 5 + body.members.length.should.be.lessThanOrEqual(5); + body.meta.pagination.limit.should.equal(5);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
ghost/core/core/server/web/api/app.js(2 hunks)ghost/core/core/server/web/comments/routes.js(1 hunks)ghost/core/core/server/web/shared/middleware/index.js(1 hunks)ghost/core/core/server/web/shared/middleware/max-limit-cap.js(1 hunks)ghost/core/core/shared/config/defaults.json(1 hunks)ghost/core/test/e2e-api/admin/max-limit-cap.test.js(1 hunks)ghost/core/test/e2e-api/content/max-limit-cap.test.js(1 hunks)ghost/core/test/e2e-api/members-comments/max-limit-cap.test.js(1 hunks)ghost/core/test/unit/server/web/shared/middleware/max-limit-cap.test.js(1 hunks)
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: ErisDS
PR: TryGhost/Ghost#23582
File: ghost/core/.c8rc.json:28-28
Timestamp: 2025-05-29T07:47:00.748Z
Learning: In the Ghost codebase, middleware directories should be included in unit test coverage because they contain actual business logic that should be tested, not just routing or setup code.
ghost/core/core/server/web/comments/routes.js (1)
Learnt from: 55sketch
PR: TryGhost/Ghost#23894
File: ghost/core/core/server/api/endpoints/comments.js:16-22
Timestamp: 2025-06-18T10:56:19.906Z
Learning: The validateCommentData function in ghost/core/core/server/api/endpoints/comments.js is not duplicated elsewhere in the codebase - it's a unique validation function that ensures either post_id or parent_id is provided for comment creation.
ghost/core/core/server/web/api/app.js (1)
Learnt from: ErisDS
PR: TryGhost/Ghost#23582
File: ghost/core/.c8rc.json:28-28
Timestamp: 2025-05-29T07:47:00.748Z
Learning: In the Ghost codebase, middleware directories should be included in unit test coverage because they contain actual business logic that should be tested, not just routing or setup code.
ghost/core/core/shared/config/defaults.json (1)
Learnt from: allouis
PR: TryGhost/Ghost#22127
File: .github/scripts/release-apps.js:72-74
Timestamp: 2025-02-06T10:30:53.440Z
Learning: In the Ghost repository, the path to defaults.json is correctly structured as 'ghost/core/core/shared/config/defaults.json' with an intentional double 'core' in the path.
ghost/core/test/e2e-api/members-comments/max-limit-cap.test.js (2)
Learnt from: mike182uk
PR: TryGhost/Ghost#22471
File: apps/admin-x-activitypub/src/utils/pending-activity.ts:13-71
Timestamp: 2025-03-13T09:00:20.205Z
Learning: The pending activity utilities in the Ghost ActivityPub module are covered by tests in the file `apps/admin-x-activitypub/test/unit/utils/pending-activity.ts`.
Learnt from: 55sketch
PR: TryGhost/Ghost#23894
File: ghost/core/core/server/api/endpoints/comments.js:16-22
Timestamp: 2025-06-18T10:56:19.906Z
Learning: The validateCommentData function in ghost/core/core/server/api/endpoints/comments.js is not duplicated elsewhere in the codebase - it's a unique validation function that ensures either post_id or parent_id is provided for comment creation.
ghost/core/test/e2e-api/content/max-limit-cap.test.js (5)
Learnt from: mike182uk
PR: TryGhost/Ghost#22471
File: apps/admin-x-activitypub/src/utils/pending-activity.ts:13-71
Timestamp: 2025-03-13T09:00:20.205Z
Learning: The pending activity utilities in the Ghost ActivityPub module are covered by tests in the file `apps/admin-x-activitypub/test/unit/utils/pending-activity.ts`.
Learnt from: ErisDS
PR: TryGhost/Ghost#23582
File: ghost/core/.c8rc.json:28-28
Timestamp: 2025-05-29T07:47:00.748Z
Learning: In the Ghost codebase, middleware directories should be included in unit test coverage because they contain actual business logic that should be tested, not just routing or setup code.
Learnt from: ErisDS
PR: TryGhost/Ghost#23588
File: ghost/core/test/e2e-api/admin/backup.test.js:136-148
Timestamp: 2025-05-29T10:37:26.369Z
Learning: In the Ghost test framework, when testing CSV exports via the admin API, the response `body` field is empty while the actual CSV data is provided through the `text` field. Tests should use `expectEmptyBody()` and then validate the CSV content via `.expect(({text}) => ...)` - this is not contradictory behavior.
Learnt from: ErisDS
PR: TryGhost/Ghost#23582
File: ghost/core/.c8rc.json:24-24
Timestamp: 2025-05-29T07:45:35.714Z
Learning: In Ghost project, app.js files under core/server/web are intentionally excluded from unit test coverage because they are not easily unit-testable due to being entry points with initialization code and side effects.
Learnt from: mike182uk
PR: TryGhost/Ghost#22471
File: apps/admin-x-activitypub/src/utils/pending-activity.ts:13-71
Timestamp: 2025-03-13T09:00:20.205Z
Learning: The pending activity utilities in Ghost's ActivityPub module are thoroughly tested in `apps/admin-x-activitypub/test/unit/utils/pending-activity.ts`, which covers `generatePendingActivityId`, `isPendingActivity`, and `generatePendingActivity` functions.
ghost/core/test/unit/server/web/shared/middleware/max-limit-cap.test.js (4)
Learnt from: ErisDS
PR: TryGhost/Ghost#23582
File: ghost/core/.c8rc.json:28-28
Timestamp: 2025-05-29T07:47:00.748Z
Learning: In the Ghost codebase, middleware directories should be included in unit test coverage because they contain actual business logic that should be tested, not just routing or setup code.
Learnt from: mike182uk
PR: TryGhost/Ghost#22471
File: apps/admin-x-activitypub/src/utils/pending-activity.ts:13-71
Timestamp: 2025-03-13T09:00:20.205Z
Learning: The pending activity utilities in the Ghost ActivityPub module are covered by tests in the file `apps/admin-x-activitypub/test/unit/utils/pending-activity.ts`.
Learnt from: ErisDS
PR: TryGhost/Ghost#23582
File: ghost/core/.c8rc.json:24-24
Timestamp: 2025-05-29T07:45:35.714Z
Learning: In Ghost project, app.js files under core/server/web are intentionally excluded from unit test coverage because they are not easily unit-testable due to being entry points with initialization code and side effects.
Learnt from: mike182uk
PR: TryGhost/Ghost#22471
File: apps/admin-x-activitypub/src/utils/pending-activity.ts:13-71
Timestamp: 2025-03-13T09:00:20.205Z
Learning: The pending activity utilities in Ghost's ActivityPub module are thoroughly tested in `apps/admin-x-activitypub/test/unit/utils/pending-activity.ts`, which covers `generatePendingActivityId`, `isPendingActivity`, and `generatePendingActivity` functions.
ghost/core/test/e2e-api/admin/max-limit-cap.test.js (4)
Learnt from: mike182uk
PR: TryGhost/Ghost#22471
File: apps/admin-x-activitypub/src/utils/pending-activity.ts:13-71
Timestamp: 2025-03-13T09:00:20.205Z
Learning: The pending activity utilities in the Ghost ActivityPub module are covered by tests in the file `apps/admin-x-activitypub/test/unit/utils/pending-activity.ts`.
Learnt from: mike182uk
PR: TryGhost/Ghost#22471
File: apps/admin-x-activitypub/src/utils/pending-activity.ts:13-71
Timestamp: 2025-03-13T09:00:20.205Z
Learning: The pending activity utilities in Ghost's ActivityPub module are thoroughly tested in `apps/admin-x-activitypub/test/unit/utils/pending-activity.ts`, which covers `generatePendingActivityId`, `isPendingActivity`, and `generatePendingActivity` functions.
Learnt from: ErisDS
PR: TryGhost/Ghost#23588
File: ghost/core/test/e2e-api/admin/backup.test.js:136-148
Timestamp: 2025-05-29T10:37:26.369Z
Learning: In the Ghost test framework, when testing CSV exports via the admin API, the response `body` field is empty while the actual CSV data is provided through the `text` field. Tests should use `expectEmptyBody()` and then validate the CSV content via `.expect(({text}) => ...)` - this is not contradictory behavior.
Learnt from: ErisDS
PR: TryGhost/Ghost#23582
File: ghost/core/.c8rc.json:24-24
Timestamp: 2025-05-29T07:45:35.714Z
Learning: In Ghost project, app.js files under core/server/web are intentionally excluded from unit test coverage because they are not easily unit-testable due to being entry points with initialization code and side effects.
🧬 Code Graph Analysis (1)
ghost/core/test/e2e-api/content/max-limit-cap.test.js (2)
ghost/core/test/e2e-api/admin/max-limit-cap.test.js (12)
require(1-1)should(2-2)agent(8-8)agent(150-151)agent(159-160)agent(168-169)agent(184-185)agent(195-196)agent(208-209)agent(219-220)agent(295-296)agent(304-305)ghost/core/test/e2e-api/members-comments/max-limit-cap.test.js (8)
require(1-1)should(2-2)sinon(3-3)sharedMiddleware(5-5)agent(8-8)originalMiddleware(10-10)middlewareSpy(11-11)req(49-49)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: E2E tests
- GitHub Check: Acceptance tests (Node 22.13.1, mysql8)
- GitHub Check: Legacy tests (Node 22.13.1, mysql8)
- GitHub Check: Unit tests (Node 22.13.1)
- GitHub Check: Acceptance tests (Node 22.13.1, sqlite3)
- GitHub Check: Legacy tests (Node 22.13.1, sqlite3)
- GitHub Check: Ghost-CLI tests
- GitHub Check: Lint
🔇 Additional comments (5)
ghost/core/core/shared/config/defaults.json (1)
174-175: Configuration values look appropriate for API limit capping.The default values of
maxLimit: 100andallowLimitAll: falsealign well with the PR objectives and provide reasonable performance constraints for API pagination.ghost/core/core/server/web/shared/middleware/index.js (1)
14-16: Middleware export follows established patterns.The new
maxLimitCapgetter follows the same lazy-loading pattern as other middleware exports and maintains consistent code structure.ghost/core/core/server/web/api/app.js (1)
23-24: Excellent middleware placement and documentation.The
maxLimitCapmiddleware is correctly positioned after version compatibility checks but before endpoint routing, ensuring all API requests are processed. The descriptive comment clearly explains its purpose.ghost/core/core/server/web/comments/routes.js (1)
26-27: Proper middleware placement for authenticated comment routes.The middleware is correctly positioned after member session loading but before route handlers, ensuring limit capping applies to all authenticated comment operations. The implementation is consistent with the main API app.
ghost/core/test/e2e-api/content/max-limit-cap.test.js (1)
1-41: Comprehensive e2e test coverage for Content API limit capping.The test implementation follows established Ghost testing patterns and provides thorough coverage:
- Proper setup with authenticated agent and fixtures
- Smart use of Sinon spy to monitor middleware behavior without disrupting functionality
- Verifies both middleware invocation and actual query parameter modification
- Clean teardown to restore original middleware
The test effectively validates that
limit=allrequests are properly capped at 100, ensuring the middleware works as expected in the Content API context.
sagzy
approved these changes
Jun 26, 2025
ErisDS
reviewed
Jun 26, 2025
|
|
||
| const limit = req.query.limit; | ||
|
|
||
| if (!limit) { |
There was a problem hiding this comment.
If not removing the config reads, maybe move them below this line 🤔
But I would probably move them up. We can override the config in testing with them outside the function so seems worth it.
There was a problem hiding this comment.
👍 I've moved them up. I had it that way before but tests were failing and I thought it was because the module-level variables weren't being overridden after the require but they seem to be passing now so maybe it was from something else not being right at the time 🤔
There was a problem hiding this comment.
No, I was right that we couldn't override in tests when they were module-scope. Added a workaround in cd0a383 which feels ok although maybe a bit magical 😅
2f9043b to
727997c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
ghost/core/core/server/web/shared/middleware/max-limit-cap.js (2)
41-47: Improve numeric validation for better edge case handling.The current logic handles most cases well, but could be more explicit about edge cases like negative numbers and very large numbers.
Consider this more explicit approach:
- // Convert to number for comparison - const numericLimit = parseInt(limit, 10); - - // If it's not a valid number or exceeds maxLimit, cap it - if (isNaN(numericLimit) || numericLimit > maxLimit) { + // Convert to number for comparison + const numericLimit = parseInt(limit, 10); + + // If it's not a valid number, negative, or exceeds maxLimit, cap it + if (isNaN(numericLimit) || numericLimit < 1 || numericLimit > maxLimit) { req.query.limit = maxLimit; }This makes the handling of negative numbers more explicit and ensures limits are positive.
18-50: Consider adding logging for limit cap enforcement.The middleware silently caps limits without logging, which could make debugging difficult in production.
Consider adding debug logging when limits are capped:
const logging = require('../../../../shared/logging'); // In the middleware function, add logging where limits are capped: if (limit === 'all') { logging.debug(`Capping limit 'all' to ${maxLimit} for ${req.originalUrl}`); req.query.limit = maxLimit; return next(); } // And for numeric limits: if (isNaN(numericLimit) || numericLimit > maxLimit) { logging.debug(`Capping limit ${limit} to ${maxLimit} for ${req.originalUrl}`); req.query.limit = maxLimit; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
ghost/core/core/server/web/api/app.js(2 hunks)ghost/core/core/server/web/comments/routes.js(1 hunks)ghost/core/core/server/web/shared/middleware/index.js(1 hunks)ghost/core/core/server/web/shared/middleware/max-limit-cap.js(1 hunks)ghost/core/core/shared/config/defaults.json(1 hunks)ghost/core/test/e2e-api/admin/max-limit-cap.test.js(1 hunks)ghost/core/test/e2e-api/content/max-limit-cap.test.js(1 hunks)ghost/core/test/e2e-api/members-comments/max-limit-cap.test.js(1 hunks)ghost/core/test/unit/server/web/shared/middleware/max-limit-cap.test.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- ghost/core/core/server/web/comments/routes.js
- ghost/core/core/server/web/api/app.js
- ghost/core/core/server/web/shared/middleware/index.js
- ghost/core/test/e2e-api/content/max-limit-cap.test.js
- ghost/core/core/shared/config/defaults.json
- ghost/core/test/e2e-api/members-comments/max-limit-cap.test.js
- ghost/core/test/unit/server/web/shared/middleware/max-limit-cap.test.js
- ghost/core/test/e2e-api/admin/max-limit-cap.test.js
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: ErisDS
PR: TryGhost/Ghost#23582
File: ghost/core/.c8rc.json:28-28
Timestamp: 2025-05-29T07:47:00.748Z
Learning: In the Ghost codebase, middleware directories should be included in unit test coverage because they contain actual business logic that should be tested, not just routing or setup code.
ghost/core/core/server/web/shared/middleware/max-limit-cap.js (1)
Learnt from: aileen
PR: TryGhost/Ghost#24073
File: apps/admin-x-settings/src/components/settings/advanced/labs/BetaFeatures.tsx:22-32
Timestamp: 2025-06-27T06:04:37.910Z
Learning: In Ghost's limiter implementation, the pattern of first checking `limiter.isLimited(limitName)` and then calling `limiter.errorIfWouldGoOverLimit(limitName)` is required and not redundant. This pattern allows for early bailout when no limit exists while still performing the full validation that can throw HostLimitError when needed.
🔇 Additional comments (1)
ghost/core/core/server/web/shared/middleware/max-limit-cap.js (1)
15-16: Good performance optimization - config reads moved to module level.The configuration values are now read at module load time rather than on each request, which addresses the performance concern raised in previous reviews.
Comment on lines
30
to
34
| // Check exception endpoints - they bypass all limits | ||
| if (EXCEPTION_ENDPOINTS.some(endpoint => req.originalUrl.startsWith(endpoint))) { | ||
| return next(); | ||
| } |
There was a problem hiding this comment.
Potential security risk: exception endpoint matching is too broad.
The startsWith check could match unintended endpoints. For example, /ghost/api/admin/posts/export/ would also match /ghost/api/admin/posts/export/malicious-endpoint.
Consider using exact path matching or a more precise pattern:
- if (EXCEPTION_ENDPOINTS.some(endpoint => req.originalUrl.startsWith(endpoint))) {
+ if (EXCEPTION_ENDPOINTS.some(endpoint => {
+ const url = req.originalUrl;
+ return url === endpoint || url.startsWith(endpoint) && url[endpoint.length] === '?';
+ })) {Or define more specific patterns for the email endpoint:
const EXCEPTION_ENDPOINTS = [
'/ghost/api/admin/posts/export/',
- '/ghost/api/admin/emails/' // /:id/batches/ and /:id/recipient-failures/
+ '/ghost/api/admin/emails/',
+ '/ghost/api/admin/emails/*/batches/',
+ '/ghost/api/admin/emails/*/recipient-failures/'
];Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In ghost/core/core/server/web/shared/middleware/max-limit-cap.js around lines 30
to 33, the current use of startsWith for matching exception endpoints is too
broad and may allow unintended endpoints to bypass limits. Replace the
startsWith check with exact path matching or use a more precise pattern matching
approach such as regular expressions to ensure only the intended endpoints are
matched. Update the EXCEPTION_ENDPOINTS list or matching logic to define
specific patterns that accurately represent the allowed exception paths.
ref https://linear.app/ghost/issue/PROD-1605/ - adds `max-limit-cap` middleware that rewrites `?limit` parameters to a capped limit, including `?limit=all` - has an exceptions list for endpoints that still require a larger limit param - has config escape valves for exceptional use-cases after 6.0 upgrade - by rewriting the `query.limit` on the request it allows everything lower in the stack to behave as if the user requested the capped limit, avoiding need to adjust code anywhere else - sets default capped limit to 100 - applies the middleware to Admin API, Content API, and Comments API
6260717 to
cd0a383
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
ghost/core/test/e2e-api/admin/max-limit-cap.test.js (2)
183-205: Consider more robust assertions for members API tests.The use of
lessThanOrEqualwith conditional limit checking suggests uncertainty about test data. Consider ensuring predictable test data or using more specific assertions.- // Even though we requested 10, we should only get max 5 - body.members.length.should.be.lessThanOrEqual(5); - if (body.members.length === 5) { - body.meta.pagination.limit.should.equal(5); - } + // Even though we requested 10, we should only get max 5 + body.members.length.should.be.lessThanOrEqual(5); + body.meta.pagination.limit.should.equal(5);
308-311: Clarify the expected behavior for limit=0.The comment suggests uncertainty about Ghost's internal handling of
limit=0. Consider verifying the exact behavior or making the assertion more specific.- // limit=0 is treated as no limit by Ghost, which uses default page size - // The actual behavior depends on Ghost's internal handling - // Since we have 14 posts in test data, we get all 14 - body.posts.length.should.be.lessThanOrEqual(15); + // limit=0 should use Ghost's default page size (typically 15) + // or return all available posts if less than default + body.posts.length.should.be.greaterThan(0); + body.posts.length.should.be.lessThanOrEqual(15);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
ghost/core/core/server/web/api/app.js(2 hunks)ghost/core/core/server/web/comments/routes.js(1 hunks)ghost/core/core/server/web/shared/middleware/index.js(1 hunks)ghost/core/core/server/web/shared/middleware/max-limit-cap.js(1 hunks)ghost/core/core/shared/config/defaults.json(1 hunks)ghost/core/test/e2e-api/admin/max-limit-cap.test.js(1 hunks)ghost/core/test/e2e-api/content/max-limit-cap.test.js(1 hunks)ghost/core/test/e2e-api/members-comments/max-limit-cap.test.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- ghost/core/core/server/web/shared/middleware/index.js
- ghost/core/core/server/web/comments/routes.js
- ghost/core/core/server/web/api/app.js
- ghost/core/core/shared/config/defaults.json
- ghost/core/test/e2e-api/content/max-limit-cap.test.js
- ghost/core/test/e2e-api/members-comments/max-limit-cap.test.js
- ghost/core/core/server/web/shared/middleware/max-limit-cap.js
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: ErisDS
PR: TryGhost/Ghost#23582
File: ghost/core/.c8rc.json:28-28
Timestamp: 2025-05-29T07:47:00.748Z
Learning: In the Ghost codebase, middleware directories should be included in unit test coverage because they contain actual business logic that should be tested, not just routing or setup code.
Learnt from: aileen
PR: TryGhost/Ghost#24073
File: apps/admin-x-settings/src/components/settings/advanced/labs/BetaFeatures.tsx:22-32
Timestamp: 2025-06-27T06:04:37.910Z
Learning: In Ghost's limiter implementation, the pattern of first checking `limiter.isLimited(limitName)` and then calling `limiter.errorIfWouldGoOverLimit(limitName)` is required and not redundant. This pattern allows for early bailout when no limit exists while still performing the full validation that can throw HostLimitError when needed.
ghost/core/test/e2e-api/admin/max-limit-cap.test.js (4)
Learnt from: mike182uk
PR: TryGhost/Ghost#22471
File: apps/admin-x-activitypub/src/utils/pending-activity.ts:13-71
Timestamp: 2025-03-13T09:00:20.205Z
Learning: The pending activity utilities in the Ghost ActivityPub module are covered by tests in the file `apps/admin-x-activitypub/test/unit/utils/pending-activity.ts`.
Learnt from: mike182uk
PR: TryGhost/Ghost#22471
File: apps/admin-x-activitypub/src/utils/pending-activity.ts:13-71
Timestamp: 2025-03-13T09:00:20.205Z
Learning: The pending activity utilities in Ghost's ActivityPub module are thoroughly tested in `apps/admin-x-activitypub/test/unit/utils/pending-activity.ts`, which covers `generatePendingActivityId`, `isPendingActivity`, and `generatePendingActivity` functions.
Learnt from: ErisDS
PR: TryGhost/Ghost#23588
File: ghost/core/test/e2e-api/admin/backup.test.js:136-148
Timestamp: 2025-05-29T10:37:26.369Z
Learning: In the Ghost test framework, when testing CSV exports via the admin API, the response `body` field is empty while the actual CSV data is provided through the `text` field. Tests should use `expectEmptyBody()` and then validate the CSV content via `.expect(({text}) => ...)` - this is not contradictory behavior.
Learnt from: ErisDS
PR: TryGhost/Ghost#23582
File: ghost/core/.c8rc.json:24-24
Timestamp: 2025-05-29T07:45:35.714Z
Learning: In Ghost project, app.js files under core/server/web are intentionally excluded from unit test coverage because they are not easily unit-testable due to being entry points with initialization code and side effects.
🧬 Code Graph Analysis (1)
ghost/core/test/e2e-api/admin/max-limit-cap.test.js (4)
ghost/core/core/server/web/comments/routes.js (1)
require(4-4)ghost/core/test/e2e-api/content/max-limit-cap.test.js (4)
require(1-1)should(2-2)sinon(3-3)agent(7-7)ghost/core/test/e2e-api/members-comments/max-limit-cap.test.js (5)
require(1-1)should(2-2)sinon(3-3)agent(8-8)postId(9-9)ghost/core/test/unit/server/web/shared/middleware/max-limit-cap.test.js (2)
sinon(2-2)maxLimitCap(6-6)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Setup
🔇 Additional comments (5)
ghost/core/test/e2e-api/admin/max-limit-cap.test.js (5)
1-6: LGTM - Standard test setup with appropriate imports.The imports are correctly structured using Ghost's E2E test framework utilities, including the necessary testing libraries and database access.
13-14: Good practice stubbing configuration for isolated testing.Stubbing the
maxLimitto 5 provides predictable test conditions and makes assertions more reliable.
149-181: Comprehensive posts API testing with good coverage.The tests effectively cover both limit capping scenarios and exception handling for the export endpoint.
232-255: Excellent exception endpoint testing with proper verification.The test correctly verifies that regular endpoints are capped while exception endpoints bypass the limit. The dual verification approach strengthens the test.
294-313: Good edge case coverage for robustness.The tests handle non-numeric limits and special cases like
limit=0, which are important for API resilience.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
ghost/core/test/e2e-api/admin/max-limit-cap.test.js (2)
217-237: Consider more consistent assertion patterns.The use of
lessThanOrEqual(5)with conditional limit checks creates inconsistent test behavior compared to the Posts API tests that use exact equality. This pattern might mask issues where the limit cap isn't properly applied.Consider either ensuring sufficient test data exists to use exact assertions, or adding explicit checks for the expected scenario.
it('should cap limit to 5 when limit exceeds max', async function () { const {body} = await agent.get('members/?limit=10') .expectStatus(200); - // Even though we requested 10, we should only get max 5 - body.members.length.should.be.lessThanOrEqual(5); - if (body.members.length === 5) { - body.meta.pagination.limit.should.equal(5); - } + // Even though we requested 10, we should only get max 5 + body.members.length.should.be.lessThanOrEqual(5); + body.meta.pagination.limit.should.equal(5); // Always check the limit was capped });
337-345: Fix inconsistency between comment and assertion.The comment states "we have 14 posts in test data, we get all 14" but the assertion checks for
lessThanOrEqual(15). This inconsistency could be confusing for maintainers.// limit=0 is treated as no limit by Ghost, which uses default page size // The actual behavior depends on Ghost's internal handling - // Since we have 14 posts in test data, we get all 14 - body.posts.length.should.be.lessThanOrEqual(15); + // The exact count depends on Ghost's default behavior for limit=0 + body.posts.length.should.be.lessThanOrEqual(15);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ghost/core/test/e2e-api/admin/max-limit-cap.test.js(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: ErisDS
PR: TryGhost/Ghost#23582
File: ghost/core/.c8rc.json:28-28
Timestamp: 2025-05-29T07:47:00.748Z
Learning: In the Ghost codebase, middleware directories should be included in unit test coverage because they contain actual business logic that should be tested, not just routing or setup code.
Learnt from: aileen
PR: TryGhost/Ghost#24073
File: apps/admin-x-settings/src/components/settings/advanced/labs/BetaFeatures.tsx:22-32
Timestamp: 2025-06-27T06:04:37.910Z
Learning: In Ghost's limiter implementation, the pattern of first checking `limiter.isLimited(limitName)` and then calling `limiter.errorIfWouldGoOverLimit(limitName)` is required and not redundant. This pattern allows for early bailout when no limit exists while still performing the full validation that can throw HostLimitError when needed.
ghost/core/test/e2e-api/admin/max-limit-cap.test.js (4)
Learnt from: mike182uk
PR: TryGhost/Ghost#22471
File: apps/admin-x-activitypub/src/utils/pending-activity.ts:13-71
Timestamp: 2025-03-13T09:00:20.205Z
Learning: The pending activity utilities in the Ghost ActivityPub module are covered by tests in the file `apps/admin-x-activitypub/test/unit/utils/pending-activity.ts`.
Learnt from: mike182uk
PR: TryGhost/Ghost#22471
File: apps/admin-x-activitypub/src/utils/pending-activity.ts:13-71
Timestamp: 2025-03-13T09:00:20.205Z
Learning: The pending activity utilities in Ghost's ActivityPub module are thoroughly tested in `apps/admin-x-activitypub/test/unit/utils/pending-activity.ts`, which covers `generatePendingActivityId`, `isPendingActivity`, and `generatePendingActivity` functions.
Learnt from: ErisDS
PR: TryGhost/Ghost#23588
File: ghost/core/test/e2e-api/admin/backup.test.js:136-148
Timestamp: 2025-05-29T10:37:26.369Z
Learning: In the Ghost test framework, when testing CSV exports via the admin API, the response `body` field is empty while the actual CSV data is provided through the `text` field. Tests should use `expectEmptyBody()` and then validate the CSV content via `.expect(({text}) => ...)` - this is not contradictory behavior.
Learnt from: ErisDS
PR: TryGhost/Ghost#23582
File: ghost/core/.c8rc.json:24-24
Timestamp: 2025-05-29T07:45:35.714Z
Learning: In Ghost project, app.js files under core/server/web are intentionally excluded from unit test coverage because they are not easily unit-testable due to being entry points with initialization code and side effects.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Setup
🔇 Additional comments (3)
ghost/core/test/e2e-api/admin/max-limit-cap.test.js (3)
1-27: Setup and imports look good.The test setup properly stubs the max limit configuration for testing and follows Ghost's E2E testing patterns with appropriate cleanup.
28-181: Excellent refactoring of test data creation.The helper functions are well-organized and focused, successfully addressing the previous feedback about breaking down the large
createBulkTestDatafunction. The separation of concerns makes the code much more maintainable.
264-325: Exception endpoint tests are well-designed.The tests properly verify that exception endpoints bypass the limit cap while confirming regular endpoints remain capped. Good defensive programming with the testEmail existence checks.
ErisDS
pushed a commit
that referenced
this pull request
Jun 30, 2025
ref https://linear.app/ghost/issue/PROD-1605/ - adds `max-limit-cap` middleware that rewrites `?limit` parameters to a capped limit, including `?limit=all` - has an exceptions list for endpoints that still require a larger limit param - has config escape valves for exceptional use-cases after 6.0 upgrade - by rewriting the `query.limit` on the request it allows everything lower in the stack to behave as if the user requested the capped limit, avoiding need to adjust code anywhere else - sets default capped limit to 100 - applies the middleware to Admin API, Content API, and Comments API
ErisDS
pushed a commit
that referenced
this pull request
Jul 1, 2025
ref https://linear.app/ghost/issue/PROD-1605/ - adds `max-limit-cap` middleware that rewrites `?limit` parameters to a capped limit, including `?limit=all` - has an exceptions list for endpoints that still require a larger limit param - has config escape valves for exceptional use-cases after 6.0 upgrade - by rewriting the `query.limit` on the request it allows everything lower in the stack to behave as if the user requested the capped limit, avoiding need to adjust code anywhere else - sets default capped limit to 100 - applies the middleware to Admin API, Content API, and Comments API
ErisDS
pushed a commit
that referenced
this pull request
Jul 2, 2025
ref https://linear.app/ghost/issue/PROD-1605/ - adds `max-limit-cap` middleware that rewrites `?limit` parameters to a capped limit, including `?limit=all` - has an exceptions list for endpoints that still require a larger limit param - has config escape valves for exceptional use-cases after 6.0 upgrade - by rewriting the `query.limit` on the request it allows everything lower in the stack to behave as if the user requested the capped limit, avoiding need to adjust code anywhere else - sets default capped limit to 100 - applies the middleware to Admin API, Content API, and Comments API
ErisDS
pushed a commit
that referenced
this pull request
Jul 2, 2025
ref https://linear.app/ghost/issue/PROD-1605/ - adds `max-limit-cap` middleware that rewrites `?limit` parameters to a capped limit, including `?limit=all` - has an exceptions list for endpoints that still require a larger limit param - has config escape valves for exceptional use-cases after 6.0 upgrade - by rewriting the `query.limit` on the request it allows everything lower in the stack to behave as if the user requested the capped limit, avoiding need to adjust code anywhere else - sets default capped limit to 100 - applies the middleware to Admin API, Content API, and Comments API
ErisDS
pushed a commit
that referenced
this pull request
Jul 10, 2025
ref https://linear.app/ghost/issue/PROD-1605/ - adds `max-limit-cap` middleware that rewrites `?limit` parameters to a capped limit, including `?limit=all` - has an exceptions list for endpoints that still require a larger limit param - has config escape valves for exceptional use-cases after 6.0 upgrade - by rewriting the `query.limit` on the request it allows everything lower in the stack to behave as if the user requested the capped limit, avoiding need to adjust code anywhere else - sets default capped limit to 100 - applies the middleware to Admin API, Content API, and Comments API
ErisDS
pushed a commit
that referenced
this pull request
Jul 10, 2025
ref https://linear.app/ghost/issue/PROD-1605/ - adds `max-limit-cap` middleware that rewrites `?limit` parameters to a capped limit, including `?limit=all` - has an exceptions list for endpoints that still require a larger limit param - has config escape valves for exceptional use-cases after 6.0 upgrade - by rewriting the `query.limit` on the request it allows everything lower in the stack to behave as if the user requested the capped limit, avoiding need to adjust code anywhere else - sets default capped limit to 100 - applies the middleware to Admin API, Content API, and Comments API
ErisDS
pushed a commit
that referenced
this pull request
Jul 11, 2025
ref https://linear.app/ghost/issue/PROD-1605/ - adds `max-limit-cap` middleware that rewrites `?limit` parameters to a capped limit, including `?limit=all` - has an exceptions list for endpoints that still require a larger limit param - has config escape valves for exceptional use-cases after 6.0 upgrade - by rewriting the `query.limit` on the request it allows everything lower in the stack to behave as if the user requested the capped limit, avoiding need to adjust code anywhere else - sets default capped limit to 100 - applies the middleware to Admin API, Content API, and Comments API
ErisDS
pushed a commit
that referenced
this pull request
Jul 14, 2025
ref https://linear.app/ghost/issue/PROD-1605/ - adds `max-limit-cap` middleware that rewrites `?limit` parameters to a capped limit, including `?limit=all` - has an exceptions list for endpoints that still require a larger limit param - has config escape valves for exceptional use-cases after 6.0 upgrade - by rewriting the `query.limit` on the request it allows everything lower in the stack to behave as if the user requested the capped limit, avoiding need to adjust code anywhere else - sets default capped limit to 100 - applies the middleware to Admin API, Content API, and Comments API
ErisDS
pushed a commit
that referenced
this pull request
Jul 15, 2025
ref https://linear.app/ghost/issue/PROD-1605/ - adds `max-limit-cap` middleware that rewrites `?limit` parameters to a capped limit, including `?limit=all` - has an exceptions list for endpoints that still require a larger limit param - has config escape valves for exceptional use-cases after 6.0 upgrade - by rewriting the `query.limit` on the request it allows everything lower in the stack to behave as if the user requested the capped limit, avoiding need to adjust code anywhere else - sets default capped limit to 100 - applies the middleware to Admin API, Content API, and Comments API
ErisDS
pushed a commit
that referenced
this pull request
Jul 16, 2025
ref https://linear.app/ghost/issue/PROD-1605/ - adds `max-limit-cap` middleware that rewrites `?limit` parameters to a capped limit, including `?limit=all` - has an exceptions list for endpoints that still require a larger limit param - has config escape valves for exceptional use-cases after 6.0 upgrade - by rewriting the `query.limit` on the request it allows everything lower in the stack to behave as if the user requested the capped limit, avoiding need to adjust code anywhere else - sets default capped limit to 100 - applies the middleware to Admin API, Content API, and Comments API
ErisDS
pushed a commit
that referenced
this pull request
Jul 17, 2025
ref https://linear.app/ghost/issue/PROD-1605/ - adds `max-limit-cap` middleware that rewrites `?limit` parameters to a capped limit, including `?limit=all` - has an exceptions list for endpoints that still require a larger limit param - has config escape valves for exceptional use-cases after 6.0 upgrade - by rewriting the `query.limit` on the request it allows everything lower in the stack to behave as if the user requested the capped limit, avoiding need to adjust code anywhere else - sets default capped limit to 100 - applies the middleware to Admin API, Content API, and Comments API
ErisDS
pushed a commit
that referenced
this pull request
Jul 17, 2025
ref https://linear.app/ghost/issue/PROD-1605/ - adds `max-limit-cap` middleware that rewrites `?limit` parameters to a capped limit, including `?limit=all` - has an exceptions list for endpoints that still require a larger limit param - has config escape valves for exceptional use-cases after 6.0 upgrade - by rewriting the `query.limit` on the request it allows everything lower in the stack to behave as if the user requested the capped limit, avoiding need to adjust code anywhere else - sets default capped limit to 100 - applies the middleware to Admin API, Content API, and Comments API
ErisDS
pushed a commit
that referenced
this pull request
Jul 18, 2025
ref https://linear.app/ghost/issue/PROD-1605/ - adds `max-limit-cap` middleware that rewrites `?limit` parameters to a capped limit, including `?limit=all` - has an exceptions list for endpoints that still require a larger limit param - has config escape valves for exceptional use-cases after 6.0 upgrade - by rewriting the `query.limit` on the request it allows everything lower in the stack to behave as if the user requested the capped limit, avoiding need to adjust code anywhere else - sets default capped limit to 100 - applies the middleware to Admin API, Content API, and Comments API
ErisDS
pushed a commit
that referenced
this pull request
Jul 20, 2025
ref https://linear.app/ghost/issue/PROD-1605/ - adds `max-limit-cap` middleware that rewrites `?limit` parameters to a capped limit, including `?limit=all` - has an exceptions list for endpoints that still require a larger limit param - has config escape valves for exceptional use-cases after 6.0 upgrade - by rewriting the `query.limit` on the request it allows everything lower in the stack to behave as if the user requested the capped limit, avoiding need to adjust code anywhere else - sets default capped limit to 100 - applies the middleware to Admin API, Content API, and Comments API
ErisDS
pushed a commit
that referenced
this pull request
Jul 21, 2025
ref https://linear.app/ghost/issue/PROD-1605/ - adds `max-limit-cap` middleware that rewrites `?limit` parameters to a capped limit, including `?limit=all` - has an exceptions list for endpoints that still require a larger limit param - has config escape valves for exceptional use-cases after 6.0 upgrade - by rewriting the `query.limit` on the request it allows everything lower in the stack to behave as if the user requested the capped limit, avoiding need to adjust code anywhere else - sets default capped limit to 100 - applies the middleware to Admin API, Content API, and Comments API
ErisDS
pushed a commit
that referenced
this pull request
Jul 21, 2025
ref https://linear.app/ghost/issue/PROD-1605/ - adds `max-limit-cap` middleware that rewrites `?limit` parameters to a capped limit, including `?limit=all` - has an exceptions list for endpoints that still require a larger limit param - has config escape valves for exceptional use-cases after 6.0 upgrade - by rewriting the `query.limit` on the request it allows everything lower in the stack to behave as if the user requested the capped limit, avoiding need to adjust code anywhere else - sets default capped limit to 100 - applies the middleware to Admin API, Content API, and Comments API
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
ref https://linear.app/ghost/issue/PROD-1605/
max-limit-capmiddleware that rewrites?limitparameters to a capped limit, including?limit=allquery.limiton the request it allows everything lower in the stack to behave as if the user requested the capped limit, avoiding need to adjust code anywhere else