Added Admin API endpoint for creating comments with member impersonation #23894
Conversation
ref CON-301 This change allows comment creation via Admin API, opening up the possibility to import comments from other platforms by enabling external tools to create comments on behalf of existing members.
WalkthroughA new "add" endpoint was introduced to the comments API, enabling creation of comments with required fields for member ID and HTML content, and requiring either a post ID (for top-level comments) or a parent ID (for replies). This endpoint supports an optional 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
ghost/core/core/server/web/api/endpoints/admin/middleware.js (1)
69-70: Allow-listing looks correct; consider documenting the new surface area
comments: ['GET', 'POST', 'PUT']aligns with the new routes and deliberately omitsDELETE. From a security/ops standpoint it would be helpful to add a short comment or update the admin-API docs so that integrators are aware that comment deletion is not allowed through integration tokens.Please confirm whether docs/generated OpenAPI spec also lists the new permissions; if not, they should be regenerated.
ghost/core/test/e2e-api/admin/comments-add.test.js (2)
13-44: Happy-path test is solid; add an assertion forLocationheaderBesides validating the body, asserting
res.headers.location(or lack thereof) would fully exercise the 201 semantics and catch regressions if the controller later decides to send a canonical URL.-const response = await agent - .post('comments/') - .body({comments: [commentData]}) - .expectStatus(201); +const response = await agent + .post('comments/') + .body({comments: [commentData]}) + .expectStatus(201); + +// Optional: verify Location header if the API ever sets it +should.exist(response.headers); +// should.exist(response.headers.location);
46-61: Fixtures not initialised for unauthenticated agent – harmless but noisyThe 401 check doesn’t need fixtures, yet the post/member IDs supplied are obviously invalid. To avoid future confusion, consider trimming the payload to
{}or adding a comment that IDs are irrelevant because authentication fails first.ghost/core/core/server/api/endpoints/comments.js (1)
112-119: Cache-header toggle is contradictory
headers.cacheInvalidateis set tofalse, yet custom invalidation headers are still emitted viahandleCacheHeaders. That works, but it’s easy for a future maintainer to misinterpret the intent.Add a short code comment explaining why manual invalidation is desirable here while the automatic mechanism is disabled.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
ghost/core/core/server/api/endpoints/comments.js(1 hunks)ghost/core/core/server/web/api/endpoints/admin/middleware.js(1 hunks)ghost/core/core/server/web/api/endpoints/admin/routes.js(1 hunks)ghost/core/test/e2e-api/admin/comments-add.test.js(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
ghost/core/core/server/web/api/endpoints/admin/routes.js (1)
ghost/core/core/server/web/api/endpoints/content/routes.js (3)
router(12-12)mw(5-5)api(3-3)
ghost/core/core/server/api/endpoints/comments.js (2)
ghost/core/core/server/services/comments/CommentsController.js (1)
data(161-161)ghost/core/core/server/api/endpoints/comment-replies.js (1)
commentsService(3-3)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: Acceptance tests (Node 22.13.1, mysql8)
- GitHub Check: Acceptance tests (Node 20.11.1, mysql8)
- GitHub Check: Unit tests (Node 22.13.1)
- GitHub Check: Legacy tests (Node 20.11.1, sqlite3)
- GitHub Check: Unit tests (Node 20.11.1)
- GitHub Check: Legacy tests (Node 20.11.1, mysql8)
- GitHub Check: Ghost-CLI tests
- GitHub Check: Lint
🔇 Additional comments (3)
ghost/core/core/server/web/api/endpoints/admin/routes.js (1)
46-51: Route addition LGTMThe POST route is placed next to the other comment endpoints and follows existing conventions (
mw.authAdminApi,http(api.*)wrapper). Express’ default non-strict routing will match both/commentsand/comments/, so test paths with a trailing slash will pass.No further issues spotted.
ghost/core/core/server/api/endpoints/comments.js (2)
69-72: Good catch returning the browse resultReturning
resultprevents an implicitundefinedfrom leaking to the transport layer.
73-122: ```shell
#!/bin/bash
set -e
echo "Searching for frame.options.context usage across the repository"
rg --color=never "frame\.options\.context" -n .</details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
ghost/core/test/e2e-api/admin/comments.test.js (2)
695-704: Keep header-snapshot parity with the rest of the suiteAlmost every request assertion above records the
etagheader to guard against accidental cache regressions.
For consistency add the same check here:-const response = await adminApi - .post('comments/') - .body({comments: [commentData]}) - .expectStatus(201); +const response = await adminApi + .post('comments/') + .body({comments: [commentData]}) + .expectStatus(201) + .matchHeaderSnapshot({etag: anyEtag});
712-721: Strengthen the negative-path test – assert the error payloadThe test only checks for a
400status. Verifying the error structure (type, message, property names) protects against silent contract breaks.await adminApi .post('comments/') .body({comments: [commentData]}) - .expectStatus(400); + .expectStatus(400) + .matchBodySnapshot({ + errors: [{ + type: 'ValidationError' + }] + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ghost/core/test/e2e-api/admin/comments.test.js(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Ghost-CLI tests
- GitHub Check: Legacy tests (Node 20.11.1, sqlite3)
- GitHub Check: Acceptance tests (Node 20.11.1, sqlite3)
- GitHub Check: Unit tests (Node 22.13.1)
- GitHub Check: Unit tests (Node 20.11.1)
- GitHub Check: Legacy tests (Node 20.11.1, mysql8)
- GitHub Check: Acceptance tests (Node 22.13.1, mysql8)
- GitHub Check: Acceptance tests (Node 20.11.1, mysql8)
- GitHub Check: Lint
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (2)
ghost/core/core/server/services/comments/CommentsController.js (1)
159-172: Missing trust-boundary check betweenmember_idin payload and impersonated member
The Admin endpoint relies on the caller to setframe.options.context.member.id(via the API layer) but the body also contains an explicitmember_id. A malicious client could supply mismatching values and create comments on behalf of a different member.
Add a guard before calling the service layer:+ // Ensure the request body cannot impersonate an unintended member + if (data.member_id && data.member_id !== frame.options.context.member.id) { + throw new errors.BadRequestError({ + message: 'member_id does not match impersonated member' + }); + }ghost/core/core/server/services/comments/CommentsService.js (1)
219-256: Validate and normalisecreatedAtbefore persisting
createdAtcan be any value – including invalid strings or numbers – and is written straight to the database. Perform a sanity check and convert to aDateobject:- if (createdAt) { - commentData.created_at = createdAt; + if (createdAt) { + const ts = new Date(createdAt); + if (Number.isNaN(ts.getTime())) { + throw new errors.ValidationError({ + message: 'Invalid created_at timestamp' + }); + } + commentData.created_at = ts; }Apply the same validation in
replyToComment.
🧹 Nitpick comments (3)
ghost/core/core/server/services/comments/CommentsService.js (1)
330-344: Duplicate logic – extract helper to build comment payload
BothcommentOnPostandreplyToCommentbuild almost identicalcommentDataobjects. Consider extracting this into a small private method to keep the service DRY and easier to change in one place.ghost/core/test/e2e-api/admin/comments.test.js (2)
731-735: Fragile equality assertion oncreated_at
The API may return the timestamp with millisecond precision or a different UTC offset, causing this strict equality check to break. Prefer the existinganyISODateTimematcher or compare withnew Date(reply.created_at).getTime() === new Date(customTimestamp).getTime().
807-828: Missing body validation assertions
The negative-path tests only assert on status codes. Also assert the errortype/messageso that un-related validation failures don’t pass silently.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
ghost/core/core/server/api/endpoints/comments.js(3 hunks)ghost/core/core/server/services/comments/CommentsController.js(1 hunks)ghost/core/core/server/services/comments/CommentsService.js(5 hunks)ghost/core/test/e2e-api/admin/comments.test.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- ghost/core/core/server/api/endpoints/comments.js
🧰 Additional context used
🧬 Code Graph Analysis (1)
ghost/core/test/e2e-api/admin/comments.test.js (2)
ghost/core/test/e2e-api/members-comments/comments.test.js (30)
commentData(60-60)comment(525-527)comment(532-534)comment(550-552)comment(558-560)comment(949-951)comment(971-973)comment(984-986)comment(1054-1056)comment(1074-1076)comment(1082-1085)comment(1105-1107)comment(1126-1128)comment(1150-1152)comment(1169-1171)dbFns(18-102)dbFns(886-891)dbFns(901-907)dbFns(915-921)dbFns(1013-1018)dbFns(1038-1043)dbFns(1217-1224)reply(987-990)reply(1322-1328)reply(1341-1346)reply(1379-1385)reply(1404-1409)reply(1428-1433)reply(1456-1462)reply(1483-1490)ghost/core/test/unit/server/data/schema/fixtures/fixture-manager.test.js (1)
fixtureManager(9-9)
🪛 Biome (1.9.4)
ghost/core/core/server/services/comments/CommentsService.js
[error] 334-334: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Acceptance tests (Node 22.13.1, mysql8)
- GitHub Check: Unit tests (Node 20.11.1)
- GitHub Check: Unit tests (Node 22.13.1)
- GitHub Check: Legacy tests (Node 20.11.1, sqlite3)
- GitHub Check: Acceptance tests (Node 20.11.1, sqlite3)
- GitHub Check: Legacy tests (Node 20.11.1, mysql8)
- GitHub Check: Ghost-CLI tests
- GitHub Check: Acceptance tests (Node 20.11.1, mysql8)
- GitHub Check: Lint
| validateCommentData(data); | ||
| setupMemberContext(frame, data.member_id); | ||
|
|
||
| const result = await commentsService.controller.add(frame); |
There was a problem hiding this comment.
Not necessarily a blocker, but I think this would be simpler to just call the commentsService functions directly, rather than going through the controller — i.e. instead of commentsService.controller.add(), just call commentsService.commentOnPost().
This would remove the need for the setupMemberContext function
There was a problem hiding this comment.
Will look at this 👍
There was a problem hiding this comment.
This change has been made in the latest version 👍
|
Something else worth confirming: does this send any webhooks or email notifications for comments when it's used by an integration? I'd assume in most cases we wouldn't want that, but that behavior might be automatic, in which case we'd need to carve out some kind of exception. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
ghost/core/core/server/api/endpoints/comments.js (1)
24-41: Date-validation helper is duplicated & slightly leaky
- The same logic lives in
CommentsController.#validateCreatedAt.- The helper silently swallows invalid/future dates by returning
undefined; callers have to know this convention. Exposing an explicit error (or at least a comment) would be safer.- Converting a
Dateinstance withnew Date(createdAt)allocates unnecessarily.Consider exporting a single helper from the service layer:
- const date = new Date(createdAt); + const date = createdAt instanceof Date ? createdAt : new Date(createdAt);…and re-using it from both places.
ghost/core/test/e2e-api/admin/comments.test.js (3)
730-738: Fragilecreated_atequality assertionRelying on an exact string match for
created_atis brittle—DB precision or timezone normalisation can cause false negatives.- comment.should.have.property('created_at', customTimestamp); + new Date(comment.created_at).toISOString().should.eql(customTimestamp);
830-892: Time-window assertion can flake on slow CI
timeDiff.should.be.lessThan(10000);gives the test 10 s headroom, yet a busy CI runner or debugging run can exceed that and produce intermittent failures.Consider asserting ordering instead of an absolute delta:
new Date(comment.created_at).should.be.belowOrEqual(new Date());
678-1075: Heavy duplication – factor helper to DRY the test setupThe new suite repeats the same three-step pattern (build payload → POST → common assertions) ~10 times. Extracting a small helper (e.g.
createCommentViaAdmin(payload)) would shrink the suite and improve readability/maintenance.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
ghost/core/core/server/api/endpoints/comments.js(3 hunks)ghost/core/core/server/services/comments/CommentsController.js(1 hunks)ghost/core/test/e2e-api/admin/comments.test.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- ghost/core/core/server/services/comments/CommentsController.js
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Setup
🔇 Additional comments (2)
ghost/core/core/server/api/endpoints/comments.js (1)
132-136: Overwrites the existing request context
frame.options.contextmay already containapi_key,user, etc.
Replacing it wholesale risks losing that metadata for downstream hooks/middleware.- if (!frame.options.context) { - frame.options.context = {}; - } - frame.options.context.internal = true; + frame.options.context = {...frame.options.context, internal: true};Likely an incorrect or invalid review comment.
ghost/core/test/e2e-api/admin/comments.test.js (1)
806-815: Expected status code may be wrongValidation failures in Ghost usually surface as
422 Unprocessable Entity(errors.ValidationError).
If the framework keeps that convention, this test will fail.- .expectStatus(400); + .expectStatus(422);Please verify the actual response code.
@cmraible not sure why I couldn't reply to this directly ¯_(ツ)_/¯ We set the context to 'internal' so it doesn't kick off emails. I've also added a test to ensure this |
There was a problem hiding this comment.
Looking good! I think all the changes in this file are not needed now, right?
There was a problem hiding this comment.
How come? Would we not still want to validate the created_at value? We don't validate inside commentOnPost or replyToComment as far as I can tell.
(FWIW this change was added in response to a coderabbit review comment, so I'm not married to it - that being said it seems to make sense to have it)
Co-authored-by: Chris Raible <chris@ghost.org>
Co-authored-by: Chris Raible <chris@ghost.org>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
ghost/core/core/server/services/comments/CommentsService.js (1)
334-334: Consider using optional chaining for safer property access.The static analysis tool suggests using optional chaining here for safer property access.
Apply this diff to use optional chaining:
- in_reply_to_id: inReplyToComment && inReplyToComment.get('id'), + in_reply_to_id: inReplyToComment?.get('id'),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ghost/core/core/server/services/comments/CommentsService.js(5 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
ghost/core/core/server/services/comments/CommentsService.js
[error] 334-334: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Setup
🔇 Additional comments (5)
ghost/core/core/server/services/comments/CommentsService.js (5)
207-207: LGTM: Bug fix for error property name.Good catch fixing the typo from
messagestomessagein the error object property.
219-221: LGTM: Parameter addition follows JSDoc best practices.The optional parameter syntax and function signature update are correctly implemented.
243-255: LGTM: Clean implementation of conditional timestamp assignment.The approach of building the comment data object first and then conditionally adding the
created_atfield is clean and readable. This avoids object spread complications and makes the conditional logic clear.
277-279: LGTM: Consistent parameter addition across methods.The JSDoc and function signature updates for
replyToCommentare consistent with thecommentOnPostmethod changes.
330-343: LGTM: Consistent implementation pattern for replies.The implementation follows the same clean pattern as
commentOnPost, maintaining consistency across the codebase.
ref CON-301
This change allows comment creation via Admin API, opening up the possibility to import comments from other platforms by enabling external tools to create comments on behalf of existing members.