Skip to content

fix(comments): require canView access before posting a comment - #2070

Merged
richiemcilroy merged 4 commits into
mainfrom
codex/fix-comment-idor
Aug 3, 2026
Merged

fix(comments): require canView access before posting a comment#2070
richiemcilroy merged 4 commits into
mainfrom
codex/fix-comment-idor

Conversation

@richiemcilroy

@richiemcilroy richiemcilroy commented Aug 3, 2026

Copy link
Copy Markdown
Member

Closes #1982.

Problem

The newComment server action inserted a comment for any authenticated user on any caller-supplied videoId, with no check that the caller can view the video. Any authenticated user could inject comments, replies, and reactions into other users' private videos (IDOR).

Fix

Gate the insert on the existing view policy, following the exact pattern used in get-transcript.ts / get-status.ts: fetch the video id inside an Effect.gen, run it through Policy.withPublicPolicy(videosPolicy.canView(videoId)), and throw "Video not found" when the policy denies or the row does not exist. The id-only select doubles as an existence check, since canView returns true for nonexistent videos by design.

Denial is reported as "Video not found" rather than "Forbidden" so the endpoint does not become an existence oracle for private videos.

Verification

  • Every currently-working comment flow maps to a canView grant path: owner, org/space share, public video, password-protected (the verified x-cap-password cookie is attached by CookiePasswordAttachmentLive in the shared runtime), and email-restricted shares. Anonymous comments were never a working flow (the action already required auth).
  • All other comment/reaction write paths (mobile, v1 agent API, delete routes) already enforce view access or ownership; this action was the only unguarded write.
  • Scoped biome check passes.

Note: denied comments fail silently in the UI after an optimistic insert, which matches the pre-existing failure UX of this action. A sibling low-severity leak (per-day comment counts via dashboard analytics capId) was found during review and will be filed as a follow-up; it is out of scope here.

Greptile Summary

The PR closes the comment-posting IDOR by requiring the authenticated caller to pass the existing video view policy and by verifying that the referenced video exists before insertion.

  • Adds the shared VideosPolicy.canView authorization gate to newComment.
  • Preserves non-disclosure behavior by reporting denied and nonexistent videos as "Video not found".
  • Uses the shared Effect runtime so owner, organization, space, email-restricted, and password-authorized access contexts remain available.

Confidence Score: 5/5

The PR appears safe to merge and closes the unauthorized private-video comment path without breaking established access flows.

The new gate uses the repository's established optional-auth and password-aware Effect runtime pattern, denies callers who cannot view the video, and separately rejects nonexistent video IDs before insertion.

Important Files Changed

Filename Overview
apps/web/actions/videos/new-comment.ts Adds a policy-backed existence and view-access check before comment insertion; no actionable regression was identified.

Reviews (1): Last reviewed commit: "address review: cap existence check with..." | Re-trigger Greptile

Context used:

Olena Ponomarova added 4 commits August 3, 2026 21:07
newComment only checked that the requester was authenticated, not that
they could actually view the target video. Any logged-in user could
POST a comment (or reply/reaction) onto someone else's private video
by supplying its videoId directly — an IDOR.

Guard the insert with VideosPolicy.canView, matching the pattern
already used in get-transcript.ts (and the canView guards added to
other video endpoints in #1926/#1927/#1936). Access failures are
reported as "Video not found" rather than "Forbidden" to avoid
leaking video existence to unauthorized users.
- Drop subjective 'CVE-worthy' framing from the code comment per review.
- canView returns true when a video doesn't exist (by design, so
  not-found doesn't leak existence elsewhere). Fetch the video row
  under the same policy, like get-transcript.ts does, so a bogus
  videoId is rejected instead of reaching the insert.
Splitting these means a genuine failure (e.g. a DB error) is logged
before surfacing the same user-facing 'Video not found' message,
instead of silently looking identical to a real not-found case.
Makes the intent (existence check, not a full row scan) explicit.
@cursor

cursor Bot commented Aug 3, 2026

Copy link
Copy Markdown

Bugbot is not enabled for your account, so this pull request was not reviewed.

Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs.

@richiemcilroy
richiemcilroy merged commit 1b812d8 into main Aug 3, 2026
22 of 23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Authenticated Users Can Post Comments on Private Videos Without Access Permission

1 participant