feat(preview): inline comments, replies, resolve and anonymous commenting - #2079
Conversation
…ting Anchored text comments with highlights, one-level replies, org-scoped resolve/reopen, named anonymous comments behind optional invisible reCAPTCHA v2 and the IP throttler, and a dashboard restyle of /p/:id.
Strix Security Review1 open security finding on this PR:
Review summaryReviewed all 33 changed files in PR #2079 (anonymous and authenticated inline preview comments, replies, resolve/reopen, reCAPTCHA, anchor offsets, and throttling). The new comment routes, DTO validation, anchor-offset validation, resolve authorization, and reCAPTCHA verification are correctly implemented. Comment content, display names, and anchor quotes are rendered with React auto-escaping, and post content remains sanitized via DOMPurify. No new security issues were identified in the changed code. The previously reported per-IP rate-limit bypass (spoofed X-Forwarded-For) remains present but was acknowledged and deferred by the maintainer as a broader trusted-proxy change, and the removal of the organization check on comment creation was confirmed as intended for public preview links. Fixed the findings? re-run the review, or tag Updated for Reviewed by Strix |
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| @Param('id') id: string, | ||
| @Body() body: { comment: string } | ||
| @Body() body: CreatePublicCommentDto, | ||
| @RealIP() ip: string | ||
| ) { | ||
| return this._postsService.createComment(org.id, user.id, id, body.comment); | ||
| return this._postsService.createPublicComment(id, body, user.id, ip); |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
This is intended. Preview links are public and the feature is built so that anyone with the link can comment, including people outside the organization such as clients and sponsors, signed in or not. The new public route accepts anonymous comments on any post the visitor has the link for, so a signed in user from another organization gains nothing extra through this route; their comment just carries their profile name instead of a typed one. The previous version of this endpoint had no check that the post belongs to the caller's organization either; it only stamped the caller's organization id on the comment, which stored the wrong id. The service now takes the organization from the post itself. Actions that must stay scoped are checked: resolving a thread verifies that the comment's post belongs to the caller's organization and returns 404 otherwise.
![strix-security[bot]](https://avatars.githubusercontent.com/in/2749804?s=60&v=4){kind=small}
| if (!req.org) { | ||
| const forwarded = String(req.headers?.['x-forwarded-for'] || '') | ||
| .split(',')[0] | ||
| .trim(); | ||
| return 'ip_' + (forwarded || req.ip || 'unknown'); | ||
| } |
There was a problem hiding this comment.
🟢 Per-IP rate limiting on anonymous comment route bypassable via spoofed X-Forwarded-For header
Severity: LOW · CWE-799
The new anonymous comment route POST /public/posts/:id/comments is protected against abuse solely by the ThrottlerBehindProxyGuard per-IP limit when reCAPTCHA is unset (the default self-hosted configuration). The throttle key is derived from the first entry of the X-Forwarded-For header, which is fully attacker-controlled: the backend never configures app.set('trust proxy', ...), and the bundled reverse-proxy config appends ($proxy_add_x_forwarded_for) rather than overwrites the header, so the first entry is always the client-supplied value. Rotating this header per request yields a unique tracker key each time, defeating the rate limit and enabling unbounded anonymous comment spam/spam on any post preview.
| if (!req.org) { | |
| const forwarded = String(req.headers?.['x-forwarded-for'] || '') | |
| .split(',')[0] | |
| .trim(); | |
| return 'ip_' + (forwarded || req.ip || 'unknown'); | |
| } | |
| if (!req.org) { | |
| return 'ip_' + (req.ip || 'unknown'); | |
| } |
Prompt to fix with AI
This is a security vulnerability found during a code review.
Vulnerability: Per-IP rate limiting on anonymous comment route bypassable via spoofed X-Forwarded-For header
Severity: LOW
CWE: CWE-799
The new anonymous comment route `POST /public/posts/:id/comments` is protected against abuse solely by the `ThrottlerBehindProxyGuard` per-IP limit when reCAPTCHA is unset (the default self-hosted configuration). The throttle key is derived from the first entry of the `X-Forwarded-For` header, which is fully attacker-controlled: the backend never configures `app.set('trust proxy', ...)`, and the bundled reverse-proxy config appends (`$proxy_add_x_forwarded_for`) rather than overwrites the header, so the first entry is always the client-supplied value. Rotating this header per request yields a unique tracker key each time, defeating the rate limit and enabling unbounded anonymous comment spam/spam on any post preview.
Location: libraries/nestjs-libraries/src/throttler/throttler.provider.ts:27-32
Context: Derive IP-agnostic throttle key from req.ip (trust-proxy aware) instead of untrusted X-Forwarded-For
```
// Before:
if (!req.org) {
const forwarded = String(req.headers?.['x-forwarded-for'] || '')
.split(',')[0]
.trim();
return 'ip_' + (forwarded || req.ip || 'unknown');
}
// After:
if (!req.org) {
return 'ip_' + (req.ip || 'unknown');
}
```
How to fix:
Do not trust the raw `X-Forwarded-For` header for the throttle key. Either (1) configure a trusted-proxy allowlist via `app.set('trust proxy', ...)` so Express only honors `X-Forwarded-For` from the known upstream proxy, and key the throttle off `req.ip`; or (2) key off a header the trusted reverse proxy overwrites with the true client address (the repo's nginx config already sets `X-Real-IP` to `$remote_addr`), consistent with the `@RealIP()` decorator. Ensure the same trusted-proxy allowlist is applied so the header cannot be spoofed by direct access to the backend.
Please fix this vulnerability. If you propose a fix, make it concise and minimal.React 👍 / 👎 to tune Strix for this repo. A repo collaborator (or the PR author) can resolve this thread to dismiss the finding.
There was a problem hiding this comment.
Thanks, the observation is correct: the first X-Forwarded-For entry is client controlled. The suggested fix would break rate limiting in the bundled setup, though: the backend runs behind nginx on localhost:3000 with no trust proxy configured, so req.ip is the same local address for every visitor and all anonymous reviewers would share one bucket, turning the limit into a site wide cap. A correct key depends on the deployment's proxy chain and needs a trusted proxy setting, which is a broader change than this PR and applies to every place the app resolves client IPs. For this route the primary control in production is reCAPTCHA: every anonymous comment requires a token verified by Google, which a spoofed header does not bypass. Leaving this as is for this PR.
…omments # Conflicts: # apps/frontend/src/app/(app)/layout.tsx # libraries/nestjs-libraries/src/database/prisma/posts/posts.service.ts
This comment was marked as off-topic.
This comment was marked as off-topic.
postiz-contribution
Bot
added
the
guard:blocked
| if ( | ||
| body.anchorQuote !== plainText.slice(body.anchorStart!, body.anchorEnd!) | ||
| ) { | ||
| throw new BadRequestException('Anchor does not match the post text'); | ||
| } |
There was a problem hiding this comment.
Bug: The API returns a misleading error if anchorStart is provided without anchorQuote because anchorQuote is incorrectly marked as optional in the DTO for this case.
Severity: LOW
Suggested Fix
Update the DTO to enforce that anchorQuote must be a non-empty string if anchorStart is present. This can be achieved using a custom validation decorator like @ValidateIf from class-validator to create a conditional requirement. Alternatively, add an explicit check at the start of the service logic: if (hasStart && typeof body.anchorQuote !== 'string') { throw new BadRequestException('anchorQuote is required when an anchor is provided'); }.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location:
libraries/nestjs-libraries/src/database/prisma/posts/posts.service.ts#L1442-L1446
Potential issue: The DTO for creating a comment with an anchor has `anchorQuote` marked
as optional, while `anchorStart` and `anchorEnd` are conditionally required. However,
the backend validation logic at `posts.service.ts` implicitly assumes `anchorQuote` will
be a string if `anchorStart` is present. If an API client submits a request with
`anchorStart` and `anchorEnd` but omits `anchorQuote`, the comparison `body.anchorQuote
!== plainText.slice(...)` becomes `undefined !== "some text"`. This incorrectly triggers
a `BadRequestException` with the misleading message "Anchor does not match the post
text", when the actual issue is the missing `anchorQuote` field. This behavior is
confined to direct API interaction, as the client-side implementation always sends the
complete anchor object.
nevo-david
added
guard:approved
and removed
guard:blocked
3 participants
What kind of change does this PR introduce?
Feature (backend + frontend, post preview page
/p/:idand its comments). Reviewers can now comment on a selected span of a post item's text, reply to comments (one level), and signed-in members of the post's organization can resolve and reopen threads. Commenting no longer requires a Postiz account: an anonymous reviewer is asked for a name, and anonymous writes are protected by optional invisible reCAPTCHA v2 plus the per-IP throttler. The whole page is restyled with the dashboard surfaces and controls.Concretely: the
Commentsmodel gains nullabledisplayName,parentId,anchorStart,anchorEnd,anchorQuoteandresolvedAtcolumns anduserIdbecomes nullable;GET /public/posts/:id/commentsreturns the comments of every item in the thread with a display name; newPOST /public/posts/:id/comments(anonymous) andPUT /posts/comments/:commentId/resolve(authenticated) routes;POST /posts/:id/commentsnow takes the same DTO as the public route. Saving a post detaches the highlight of any anchored comment whose quoted text no longer matches, keeping the quote. The editor, notifications, comment deletion and tier gating are deliberately unchanged.Why was this change needed?
Requested by a customer who shares post previews with clients and needs them to give precise, in-context feedback without creating a Postiz account. The approved spec is in #2069.
Previously only signed-in users could comment, comments could only target the post as a whole, and commenters showed up as "User1", "User2".
Other information:
pnpm run prisma-db-push.POST /posts/:id/commentsbody changed from{ comment }to{ content }. The preview sidebar was its only caller in this repo.RECAPTCHA_SITE_KEY/RECAPTCHA_SECRET_KEYunset (documented in.env.example), anonymous comments are accepted and Google's script is never loaded, so self-hosters are unaffected. With keys set it is invisible v2: most reviewers see nothing, and Google shows a puzzle only for suspicious traffic, so a real user can still get their comment through. Production needs an invisible v2 key pair.API_LIMITper hour), keyed by IP for the public comment route, rather than adding a separate bucket.postContentPlainTextnext tosanitizePostContent).mainhas no test infrastructure yet; they can land together with it.ka_geis not a lingo.dev target, so it falls back to English as before):preview_comment_name_required,preview_comment_your_name,preview_comment_failed,preview_comment_detach_hint,preview_comment_text_changed,preview_no_comments_yet,add_a_comment,add_a_comment_placeholder,write_a_reply,reply,reviewer,resolved,resolve,reopen,collapse,comment.QA
Testing done on this branch: verified end to end in the browser, signed out and signed in, with real invisible reCAPTCHA v2 keys: general and anchored anonymous comments (selection by drag, double-click and triple-click), highlights and card/highlight hover and click sync, replies, resolve and reopen, a dismissed puzzle (the Post button returns to idle and a retry works), a wrong puzzle answer, and a solved puzzle. Forged and missing tokens were rejected with 400. Without reCAPTCHA keys no Google script is loaded. Frontend, backend and orchestrator build and typecheck.
pnpm run prisma-db-pushand confirm the diff only adds nullable columns and indexes toComments/p/<postId>): the old comments still show, now under their authors' real namesRECAPTCHA_*set: anonymous posting works and norecaptcha/api.jsrequest is madecurl -X POST <backend>/public/posts/<postId>/comments -H 'Content-Type: application/json' -d '{"content":"x","displayName":"Eve","recaptchaToken":"forged"}'returns 400 "Captcha verification failed"Checklist:
🤖 Generated with Claude Code
https://claude.ai/code/session_01QNeur7rLjKfuLvGH34BzX9