Removed member id/uuid from public comments API responses#28122
Removed member id/uuid from public comments API responses#281229larsons wants to merge 1 commit into
Conversation
WalkthroughThis pull request refactors how the comments system determines whether a comment was authored by the current user. The change replaces UUID equality checks with an explicit Suggested reviewers
Possibly related PRs
🚥 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)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.42.3)ghost/core/test/e2e-api/members-comments/comments.test.jsThanks 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 |
|
| Command | Status | Duration | Result |
|---|---|---|---|
nx run ghost:test:ci:e2e |
❌ Failed | 24s | View ↗ |
nx run ghost:test:ci:legacy |
✅ Succeeded | 3m 6s | View ↗ |
nx build @tryghost/activitypub |
✅ Succeeded | 1s | View ↗ |
nx build @tryghost/announcement-bar |
✅ Succeeded | <1s | View ↗ |
nx build @tryghost/signup-form |
✅ Succeeded | <1s | View ↗ |
nx build @tryghost/comments-ui |
✅ Succeeded | <1s | View ↗ |
nx build @tryghost/portal |
✅ Succeeded | <1s | View ↗ |
nx build @tryghost/sodo-search |
✅ Succeeded | <1s | View ↗ |
Additional runs (7) |
✅ Succeeded | ... | View ↗ |
☁️ Nx Cloud last updated this comment at 2026-05-26 00:29:06 UTC
- the public comments endpoint exposed every comment author's id and uuid, which is a stable identifier that lets any reader correlate all comments by a given member across a site - the only consumer (comments-ui) used those identifiers solely to check "is this comment mine?" for the edit/delete/expertise UI - replaced them with a per-viewer is_author boolean computed from the authenticated member session (same pattern as the existing liked field), so the client gets exactly what it needs and nothing more - also dropped the data-member-uuid DOM attribute which leaked the same identifier into the rendered page
9larsons
force-pushed
the
comments-remove-member-uuid
branch
from
9b526d9 to
0af4fcc
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@apps/comments-ui/test/utils/mocked-api.ts`:
- Line 29: The constructor signature advertises a members?: any[] option but
never reads it; remove members from the constructor parameter type and the
destructured default list so callers can't pass a no-op option. Update the
constructor definition (the constructor({postId = 'ABC', comments = [], member =
undefined, settings = {}, labs = {}}: {postId?: string, comments?: any[],
member?: any, settings?: any, labs?: any}) declaration) by deleting the
members?: any[] entry from the type and ensuring the destructured params no
longer include members; if the class actually needs a members array instead,
alternatively accept members in the destructuring and assign it to the instance
(this.members) — pick one consistent approach and update tests/call sites
accordingly.
- Around line 238-245: `#withIsAuthor` currently derives is_author from the
instance-level this.member?.id which ignores request-scoped member UUIDs (e.g.,
admin impersonation), so update the API mock to accept a request-specific
memberId parameter and thread it through: change `#withIsAuthor` to signature like
`#withIsAuthor`(comment, memberId) and have browseComments and browseReplies pass
their request memberId (including impersonation paths) into the recursive calls
so each comment/reply computes is_author as comment.member?.id === memberId
rather than using this.member.
In `@ghost/core/test/e2e-api/members-comments/comments.test.js`:
- Around line 110-111: The test currently asserts on a removed field
(fetchedComment.member.id !== memberId); update the assertion to use the new
member contract by removing the id check and instead assert the PUT result via
fetchedComment.member.is_author (and/or other properties in commentMatcher).
Locate the assertion that references fetchedComment.member.id (near the
assertion around the PUT fetch) and replace it with an assertion that
fetchedComment.member.is_author matches the expected boolean after the PUT,
keeping the rest of the commentMatcher check intact.
🪄 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: 042d0730-0479-485e-9110-661344beb2a3
⛔ Files ignored due to path filters (1)
ghost/core/test/e2e-api/members-comments/__snapshots__/comments.test.js.snapis excluded by!**/*.snap
📒 Files selected for processing (8)
apps/comments-ui/src/app-context.tsapps/comments-ui/src/components/content/comment.tsxapps/comments-ui/src/components/content/context-menus/comment-context-menu.tsxapps/comments-ui/test/unit/components/content/comment.test.jsxapps/comments-ui/test/unit/components/content/context-menus/comment-context-menu.test.jsxapps/comments-ui/test/utils/mocked-api.tsghost/core/core/server/api/endpoints/utils/serializers/output/mappers/comments.jsghost/core/test/e2e-api/members-comments/comments.test.js
💤 Files with no reviewable changes (1)
- apps/comments-ui/test/unit/components/content/comment.test.jsx
| } | ||
|
|
||
| constructor({postId = 'ABC', comments = [], member = undefined, settings = {}, members = [], labs = {}}: {postId?: string, comments?: any[], member?: any, settings?: any, members?: any[], labs?: any}) { | ||
| constructor({postId = 'ABC', comments = [], member = undefined, settings = {}, labs = {}}: {postId?: string, comments?: any[], member?: any, settings?: any, members?: any[], labs?: any}) { |
There was a problem hiding this comment.
Constructor options still advertise members but silently ignore it.
members?: any[] remains in the constructor type, but the value is no longer read. This makes tests pass an option that has no effect and hides setup mistakes.
Suggested fix
-constructor({postId = 'ABC', comments = [], member = undefined, settings = {}, labs = {}}: {postId?: string, comments?: any[], member?: any, settings?: any, members?: any[], labs?: any}) {
+constructor({postId = 'ABC', comments = [], member = undefined, settings = {}, labs = {}}: {postId?: string, comments?: any[], member?: any, settings?: any, labs?: any}) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| constructor({postId = 'ABC', comments = [], member = undefined, settings = {}, labs = {}}: {postId?: string, comments?: any[], member?: any, settings?: any, members?: any[], labs?: any}) { | |
| constructor({postId = 'ABC', comments = [], member = undefined, settings = {}, labs = {}}: {postId?: string, comments?: any[], member?: any, settings?: any, labs?: any}) { |
🤖 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 `@apps/comments-ui/test/utils/mocked-api.ts` at line 29, The constructor
signature advertises a members?: any[] option but never reads it; remove members
from the constructor parameter type and the destructured default list so callers
can't pass a no-op option. Update the constructor definition (the
constructor({postId = 'ABC', comments = [], member = undefined, settings = {},
labs = {}}: {postId?: string, comments?: any[], member?: any, settings?: any,
labs?: any}) declaration) by deleting the members?: any[] entry from the type
and ensuring the destructured params no longer include members; if the class
actually needs a members array instead, alternatively accept members in the
destructuring and assign it to the instance (this.members) — pick one consistent
approach and update tests/call sites accordingly.
| #withIsAuthor(comment: any): any { | ||
| const memberId = this.member?.id; | ||
| return { | ||
| ...comment, | ||
| member: comment.member ? {...comment.member, is_author: !!memberId && comment.member.id === memberId} : null, | ||
| replies: (comment.replies ?? []).map((reply: any) => this.#withIsAuthor(reply)) | ||
| }; | ||
| } |
There was a problem hiding this comment.
is_author derivation is global-member based, not request-scoped.
#withIsAuthor always compares against this.member?.id, so admin/impersonation paths that provide a request member UUID can be misrepresented in mocked responses. That can mask ownership-path regressions in tests.
Suggested direction
-#withIsAuthor(comment: any): any {
- const memberId = this.member?.id;
+#withIsAuthor(comment: any, memberId?: string): any {
return {
...comment,
member: comment.member ? {...comment.member, is_author: !!memberId && comment.member.id === memberId} : null,
- replies: (comment.replies ?? []).map((reply: any) => this.#withIsAuthor(reply))
+ replies: (comment.replies ?? []).map((reply: any) => this.#withIsAuthor(reply, memberId))
};
}Then pass a request-specific memberId from browseComments/browseReplies call sites (including admin impersonation flows).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #withIsAuthor(comment: any): any { | |
| const memberId = this.member?.id; | |
| return { | |
| ...comment, | |
| member: comment.member ? {...comment.member, is_author: !!memberId && comment.member.id === memberId} : null, | |
| replies: (comment.replies ?? []).map((reply: any) => this.#withIsAuthor(reply)) | |
| }; | |
| } | |
| `#withIsAuthor`(comment: any, memberId?: string): any { | |
| return { | |
| ...comment, | |
| member: comment.member ? {...comment.member, is_author: !!memberId && comment.member.id === memberId} : null, | |
| replies: (comment.replies ?? []).map((reply: any) => this.#withIsAuthor(reply, memberId)) | |
| }; | |
| } |
🤖 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 `@apps/comments-ui/test/utils/mocked-api.ts` around lines 238 - 245,
`#withIsAuthor` currently derives is_author from the instance-level
this.member?.id which ignores request-scoped member UUIDs (e.g., admin
impersonation), so update the API mock to accept a request-specific memberId
parameter and thread it through: change `#withIsAuthor` to signature like
`#withIsAuthor`(comment, memberId) and have browseComments and browseReplies pass
their request memberId (including impersonation paths) into the recursive calls
so each comment/reply computes is_author as comment.member?.id === memberId
rather than using this.member.
| member: { | ||
| id: anyObjectId, | ||
| uuid: anyUuid | ||
| is_author: anyBoolean |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Find test assertions or code that access member.id or member.uuid in comment responses
# Search for patterns like .member.id, .member.uuid, comment.member.id, etc.
rg -n --type=js -C3 '\b(comment|fetchedComment|body\.comments\[\d+\])\.member\.(id|uuid)\b' ghost/core/test/e2e-api/members-comments/
# Also check for destructuring patterns
rg -n --type=js -C3 'member:\s*\{\s*(id|uuid)' ghost/core/test/e2e-api/members-comments/Repository: TryGhost/Ghost
Length of output: 710
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="ghost/core/test/e2e-api/members-comments/comments.test.js"
echo "== Matcher definition around lines 90-130 =="
nl -ba "$FILE" | sed -n '90,140p'
echo "== All occurrences of .member.id or .member.uuid =="
rg -n --type=js '\.member\.(id|uuid)\b' "$FILE"
echo "== Context around the line 1514 match =="
nl -ba "$FILE" | sed -n '1490,1535p'
echo "== Any usage of is_author in expectations =="
rg -n --type=js '\bis_author\b' "$FILE" | head -n 50
echo "== Any usage of memberId/memberUuid variables =="
rg -n --type=js '\bmember(Id|UUID|Uuid|UUID)\b' "$FILE"
rg -n --type=js '\bmemberId\b|\bmember_uuid\b|\bmember\.uuid\b' "$FILE"Repository: TryGhost/Ghost
Length of output: 164
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="ghost/core/test/e2e-api/members-comments/comments.test.js"
echo "== Matcher definition around lines 90-140 =="
cat -n "$FILE" | sed -n '90,140p'
echo "== All occurrences of .member.id or .member.uuid =="
rg -n --type=js '\.member\.(id|uuid)\b' "$FILE"
echo "== Context around the line ~1514 match =="
cat -n "$FILE" | sed -n '1490,1535p'
echo "== Any usage of is_author in expectations =="
rg -n --type=js '\bis_author\b' "$FILE" | head -n 50
echo "== Any usage of memberId/memberUuid variables =="
rg -n --type=js '\bmember(Id|UUID|Uuid|UUID)\b' "$FILE" || true
rg -n --type=js '\bmemberId\b|\bmember_uuid\b|\bmember\.uuid\b' "$FILE" || trueRepository: TryGhost/Ghost
Length of output: 4447
Update the test assertion to avoid relying on removed member.id/member.uuid fields
commentMatcher now expects member: { is_author: anyBoolean } (no member.id/member.uuid). The only remaining direct dependency is assert(fetchedComment.member.id !== memberId); at ~line 1514; there are no .member.uuid usages in this test file. If the API no longer returns member.id, this check can silently become non-meaningful. Update the assertion to validate the PUT behavior using the new contract (e.g., fetchedComment.member.is_author) instead of fetchedComment.member.id.
🤖 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/e2e-api/members-comments/comments.test.js` around lines 110 -
111, The test currently asserts on a removed field (fetchedComment.member.id !==
memberId); update the assertion to use the new member contract by removing the
id check and instead assert the PUT result via fetchedComment.member.is_author
(and/or other properties in commentMatcher). Locate the assertion that
references fetchedComment.member.id (near the assertion around the PUT fetch)
and replace it with an assertion that fetchedComment.member.is_author matches
the expected boolean after the PUT, keeping the rest of the commentMatcher check
intact.
E2E Tests FailedTo view the Playwright test report locally, run: REPORT_DIR=$(mktemp -d) && gh run download 26425306583 -n playwright-report -D "$REPORT_DIR" && npx playwright show-report "$REPORT_DIR" |
1 participant
Summary
idanduuid— stable identifiers that let any reader correlate every comment by a given member across a site.apps/comments-ui) used them solely to check "is this comment mine?" for the edit/delete/expertise UI.is_authorboolean computed server-side from the authenticated member session (same pattern as the existinglikedfield). The admin API response is unchanged.data-member-uuidDOM attribute, which leaked the same identifier into the rendered page.Test plan
pnpm --filter comments-ui test:unitpassespnpm --filter comments-ui test:acceptancepasses (mocked API now decorates responses withis_author)cd ghost/core && pnpm test:single test/e2e-api/members-comments/comments.test.jspasses against updated snapshots