From 330379bfc1a59e7a6117b79e90434d578f44f0a1 Mon Sep 17 00:00:00 2001 From: Olena Ponomarova Date: Mon, 20 Jul 2026 22:26:25 +0300 Subject: [PATCH 1/4] fix(comments): require canView access before posting a comment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- apps/web/actions/videos/new-comment.ts | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/apps/web/actions/videos/new-comment.ts b/apps/web/actions/videos/new-comment.ts index 6b7814a466c..cc99dbd5042 100644 --- a/apps/web/actions/videos/new-comment.ts +++ b/apps/web/actions/videos/new-comment.ts @@ -4,10 +4,13 @@ import { db } from "@cap/database"; import { getCurrentUser } from "@cap/database/auth/session"; import { nanoId } from "@cap/database/helpers"; import { comments } from "@cap/database/schema"; +import { provideOptionalAuth, VideosPolicy } from "@cap/web-backend"; import type { ImageUpload } from "@cap/web-domain"; -import { Comment, type Video } from "@cap/web-domain"; +import { Comment, Policy, type Video } from "@cap/web-domain"; +import { Effect, Exit } from "effect"; import { revalidatePath } from "next/cache"; import { createNotification } from "@/lib/Notification"; +import * as EffectRuntime from "@/lib/server"; export async function newComment(data: { content: string; @@ -37,6 +40,20 @@ export async function newComment(data: { if (!content || !videoId) { throw new Error("Content and videoId are required"); } + + // Authentication alone isn't authorization: without this, any logged-in + // user could comment on someone else's private video (CVE-worthy IDOR). + const accessExit = await Effect.gen(function* () { + const videosPolicy = yield* VideosPolicy; + return yield* Effect.void.pipe( + Policy.withPublicPolicy(videosPolicy.canView(videoId)), + ); + }).pipe(provideOptionalAuth, EffectRuntime.runPromiseExit); + + if (Exit.isFailure(accessExit)) { + throw new Error("Video not found"); + } + const id = Comment.CommentId.make(nanoId()); const newComment = { From bcec25272cca0c9e35973a55a270078d34648864 Mon Sep 17 00:00:00 2001 From: Olena Ponomarova Date: Mon, 20 Jul 2026 22:37:59 +0300 Subject: [PATCH 2/4] address review: neutral wording, reject nonexistent videoId - 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. --- apps/web/actions/videos/new-comment.ts | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/apps/web/actions/videos/new-comment.ts b/apps/web/actions/videos/new-comment.ts index cc99dbd5042..114f93a7587 100644 --- a/apps/web/actions/videos/new-comment.ts +++ b/apps/web/actions/videos/new-comment.ts @@ -3,10 +3,11 @@ import { db } from "@cap/database"; import { getCurrentUser } from "@cap/database/auth/session"; import { nanoId } from "@cap/database/helpers"; -import { comments } from "@cap/database/schema"; +import { comments, videos } from "@cap/database/schema"; import { provideOptionalAuth, VideosPolicy } from "@cap/web-backend"; import type { ImageUpload } from "@cap/web-domain"; import { Comment, Policy, type Video } from "@cap/web-domain"; +import { eq } from "drizzle-orm"; import { Effect, Exit } from "effect"; import { revalidatePath } from "next/cache"; import { createNotification } from "@/lib/Notification"; @@ -41,16 +42,20 @@ export async function newComment(data: { throw new Error("Content and videoId are required"); } - // Authentication alone isn't authorization: without this, any logged-in - // user could comment on someone else's private video (CVE-worthy IDOR). + // Authentication alone isn't authorization: without this, any logged-in user could + // comment on someone else's private video by guessing its id. + // + // This also fetches the video row (rather than gating a no-op) because + // canView returns true for a nonexistent videoId by design, so a bogus id + // would otherwise reach the insert below. const accessExit = await Effect.gen(function* () { const videosPolicy = yield* VideosPolicy; - return yield* Effect.void.pipe( - Policy.withPublicPolicy(videosPolicy.canView(videoId)), - ); + return yield* Effect.promise(() => + db().select({ id: videos.id }).from(videos).where(eq(videos.id, videoId)), + ).pipe(Policy.withPublicPolicy(videosPolicy.canView(videoId))); }).pipe(provideOptionalAuth, EffectRuntime.runPromiseExit); - if (Exit.isFailure(accessExit)) { + if (Exit.isFailure(accessExit) || accessExit.value.length === 0) { throw new Error("Video not found"); } From ca3e73f36a0144a7385c7b5a9f04e9464afed47a Mon Sep 17 00:00:00 2001 From: Olena Ponomarova Date: Mon, 20 Jul 2026 22:43:41 +0300 Subject: [PATCH 3/4] address review: log operational failures separately from not-found 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. --- apps/web/actions/videos/new-comment.ts | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/apps/web/actions/videos/new-comment.ts b/apps/web/actions/videos/new-comment.ts index 114f93a7587..ea6a41ed190 100644 --- a/apps/web/actions/videos/new-comment.ts +++ b/apps/web/actions/videos/new-comment.ts @@ -55,7 +55,12 @@ export async function newComment(data: { ).pipe(Policy.withPublicPolicy(videosPolicy.canView(videoId))); }).pipe(provideOptionalAuth, EffectRuntime.runPromiseExit); - if (Exit.isFailure(accessExit) || accessExit.value.length === 0) { + if (Exit.isFailure(accessExit)) { + console.error("Video access check failed:", accessExit); + throw new Error("Video not found"); + } + + if (accessExit.value.length === 0) { throw new Error("Video not found"); } From 374cc7c37b23f2d00474ab6387a4fc2c7283a4b3 Mon Sep 17 00:00:00 2001 From: Olena Ponomarova Date: Mon, 20 Jul 2026 22:49:27 +0300 Subject: [PATCH 4/4] address review: cap existence check with .limit(1) Makes the intent (existence check, not a full row scan) explicit. --- apps/web/actions/videos/new-comment.ts | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/apps/web/actions/videos/new-comment.ts b/apps/web/actions/videos/new-comment.ts index ea6a41ed190..8cb9b62fe01 100644 --- a/apps/web/actions/videos/new-comment.ts +++ b/apps/web/actions/videos/new-comment.ts @@ -51,7 +51,11 @@ export async function newComment(data: { const accessExit = await Effect.gen(function* () { const videosPolicy = yield* VideosPolicy; return yield* Effect.promise(() => - db().select({ id: videos.id }).from(videos).where(eq(videos.id, videoId)), + db() + .select({ id: videos.id }) + .from(videos) + .where(eq(videos.id, videoId)) + .limit(1), ).pipe(Policy.withPublicPolicy(videosPolicy.canView(videoId))); }).pipe(provideOptionalAuth, EffectRuntime.runPromiseExit);