🔒 Fixed SQL injection vulnerability in click event query #25804
Conversation
WalkthroughA new helper method Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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. Comment |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
ghost/core/core/server/services/members/members-api/repositories/EventRepository.js (1)
489-505: Excellent security fix! Consider adding JSDoc documentation.The validation logic correctly prevents SQL injection by ensuring only valid ObjectIDs are used in queries. The implementation handles both direct and
$andfilter structures appropriately.📝 Suggested improvements
- Add JSDoc documentation since this is now part of the public API:
+ /** + * Extracts and validates a post_id from a filter object. + * @param {Object} filter - Filter object that may contain data.post_id + * @returns {ObjectID|null} Valid ObjectID instance or null if invalid/missing + */ getPostIdFromFilter(filter) {
- Minor: The empty string initialization on line 490 is unnecessary:
- let postIdString = ''; + let postIdString;
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
ghost/core/core/server/services/members/members-api/repositories/EventRepository.jsghost/core/test/unit/server/services/members/members-api/repositories/event-repository.test.js
🧰 Additional context used
🧬 Code graph analysis (1)
ghost/core/test/unit/server/services/members/members-api/repositories/event-repository.test.js (1)
ghost/core/core/server/services/members/members-api/repositories/EventRepository.js (1)
require(4-4)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: Ghost-CLI tests
- GitHub Check: Acceptance tests (Node 22.18.0, sqlite3)
- GitHub Check: Lint
- GitHub Check: Legacy tests (Node 22.18.0, sqlite3)
- GitHub Check: Acceptance tests (Node 22.18.0, mysql8)
- GitHub Check: Unit tests (Node 22.18.0)
- GitHub Check: Legacy tests (Node 22.18.0, mysql8)
- GitHub Check: Cursor Bugbot
- GitHub Check: Build & Push Docker Image
🔇 Additional comments (6)
ghost/core/core/server/services/members/members-api/repositories/EventRepository.js (3)
510-528: LGTM! SQL injection vulnerability successfully mitigated.The refactoring correctly validates
postIdbefore using it in the SQL query. UsingtoHexString()on the validated ObjectID ensures only safe hex strings are interpolated into the query, effectively preventing SQL injection.While parameterized queries would be ideal, the current approach with validated ObjectID hex strings is secure and doesn't require extensive refactoring of the query construction logic.
5-5: All API methods are compatible with bson-objectid v2.0.4.The import statement and the three methods used (
ObjectID.isValid(),ObjectID.createFromHexString(), and.toHexString()) are all available and correctly implemented in the bson-objectid v2.0.4 release. No changes needed.
489-607: SQL injection vulnerability at line 528 is isolated; no other similar patterns found in the codebase.Verification confirms that the
postId.toHexString()interpolation at line 528 is the only instance where user-controlled input is directly embedded in raw SQL within the members-api repositories. No other methods construct raw SQL queries with similar interpolation patterns.ghost/core/test/unit/server/services/members/members-api/repositories/event-repository.test.js (3)
1-1: LGTM! Good use of strict assertions.The addition of
node:assert/strictprovides stricter equality checks, which is appropriate for testing security-sensitive validation logic. The test suite setup follows the existing pattern in the file.Also applies to: 119-132
134-151: Excellent test coverage for valid inputs.These tests verify both direct and nested filter structures, ensuring the method correctly extracts and validates ObjectIDs in common use cases. The assertions confirm the ObjectID is properly constructed and can be safely serialized.
153-169: Outstanding security test coverage! 🔒These tests directly validate the SQL injection fix by attempting a classic SQL injection payload (
'; DROP TABLE posts; --) and confirming it's rejected. The comprehensive coverage of invalid inputs (malformed ObjectIDs, SQL injection, null/undefined/empty) ensures the validation logic is robust and secure.The test on lines 159-163 is especially critical as it demonstrates the vulnerability is properly mitigated.
Validated postId as an ObjectID before using it in database queries. This ensures we are not passing arbitrary SQL strings and limits the postId variable to a string format we expect. - Extracted postId parsing to separate getPostIdFromFilter method - Used ObjectID ValueObject for type safety and validation - Added unit tests for the extraction method Credit: Sho Odagiri GMO Cybersecurity by Ierae, Inc.
ecb2d8f to
0518876
Compare
React E2E Tests FailedTo view the Playwright test report locally, run: REPORT_DIR=$(mktemp -d) && gh run download 20813768094 -n playwright-report-react -D "$REPORT_DIR" && npx playwright show-report "$REPORT_DIR" |
1 similar comment
React E2E Tests FailedTo view the Playwright test report locally, run: REPORT_DIR=$(mktemp -d) && gh run download 20813768094 -n playwright-report-react -D "$REPORT_DIR" && npx playwright show-report "$REPORT_DIR" |
- Added indexnow_api_key to dynamicDefault in settings model - Removed getOrCreateApiKey logic from indexnow service - Added 'coming-soon' to default post slugs to skip - Separated database migration to PR TryGhost#25804
Validated postId as an ObjectID before using it in database queries. This ensures we are not passing arbitrary SQL strings and limits the postId variable to a string format we expect. - Extracted postId parsing to separate getPostIdFromFilter method - Used ObjectID ValueObject for type safety and validation - Added unit tests for the extraction method Credit: Sho Odagiri GMO Cybersecurity by Ierae, Inc.
- Added indexnow_api_key to dynamicDefault in settings model - Removed getOrCreateApiKey logic from indexnow service - Added 'coming-soon' to default post slugs to skip - Separated database migration to PR TryGhost#25804
Validated postId as an ObjectID before using it in database queries. This ensures we are not passing arbitrary SQL strings and limits the postId variable to a string format we expect. - Extracted postId parsing to separate getPostIdFromFilter method - Used ObjectID ValueObject for type safety and validation - Added unit tests for the extraction method Credit: Sho Odagiri GMO Cybersecurity by Ierae, Inc.
- Added indexnow_api_key to dynamicDefault in settings model - Removed getOrCreateApiKey logic from indexnow service - Added 'coming-soon' to default post slugs to skip - Separated database migration to PR TryGhost#25804
- Added indexnow_api_key to dynamicDefault in settings model - Removed getOrCreateApiKey logic from indexnow service - Added 'coming-soon' to default post slugs to skip - Separated database migration to PR TryGhost#25804
Validated postId as an ObjectID before using it in database queries. This ensures we are not passing arbitrary SQL strings and limits the postId variable to a string format we expect.
Credit:
Sho Odagiri
GMO Cybersecurity by Ierae, Inc.
Note
Strengthens handling of
data.post_idand secures the aggregated click events query.getPostIdFromFilterto parse/validatedata.post_idwithbson-objectid, returning anObjectIDornullgetAggregatedClickEventsto use the validatedObjectID(toHexString()) in raw SQL and removesdata.post_idfrom NQL filters to avoid unsafe interpolation$andextraction, injection attempts, and missing filtersWritten by Cursor Bugbot for commit 0518876. This will update automatically on new commits. Configure here.