From 94187b9d4d4a6e9aaff20a996b1fa468a6ec5ee1 Mon Sep 17 00:00:00 2001 From: Jiho Lee Date: Sat, 25 Jul 2026 18:39:59 +0900 Subject: [PATCH 1/3] fix(web): enforce video access policy before creating comments The newComment server action authenticated the caller but never checked whether they may access the target video. Since videoId comes from the client, any signed-in user could insert comments into another user's private recording. Gate the insert behind VideosPolicy.canView, matching the pattern already used by getTranscript and the /api/video/ai fix in #1926. --- apps/web/actions/videos/new-comment.ts | 24 ++++++++++++++++++++++-- 1 file changed, 22 insertions(+), 2 deletions(-) diff --git a/apps/web/actions/videos/new-comment.ts b/apps/web/actions/videos/new-comment.ts index 6b7814a466c..5cc20138227 100644 --- a/apps/web/actions/videos/new-comment.ts +++ b/apps/web/actions/videos/new-comment.ts @@ -3,11 +3,15 @@ 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, type Video } 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"; +import * as EffectRuntime from "@/lib/server"; export async function newComment(data: { content: string; @@ -37,6 +41,22 @@ export async function newComment(data: { if (!content || !videoId) { throw new Error("Content and videoId are required"); } + + // The caller controls `videoId`, so being signed in is not enough — without + // this check any authenticated user can insert comments into a video they + // cannot view, including someone else's private recording. + 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)), + ).pipe(Policy.withPublicPolicy(videosPolicy.canView(videoId))); + }).pipe(provideOptionalAuth, EffectRuntime.runPromiseExit); + + if (Exit.isFailure(accessExit) || accessExit.value.length === 0) { + throw new Error("Video not found"); + } + const id = Comment.CommentId.make(nanoId()); const newComment = { From fe06b7bd4d5fe44e2df3ea1a00d93335b753e6cc Mon Sep 17 00:00:00 2001 From: Jiho Lee Date: Sat, 25 Jul 2026 18:56:32 +0900 Subject: [PATCH 2/3] refactor(web): split the access-check failure branches in newComment Addresses review feedback: separating Exit.isFailure from the .value access lets the type guard narrow cleanly and matches get-transcript.ts. Also limits the lookup to one row since videos.id is unique. --- apps/web/actions/videos/new-comment.ts | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/apps/web/actions/videos/new-comment.ts b/apps/web/actions/videos/new-comment.ts index 5cc20138227..45f486380fb 100644 --- a/apps/web/actions/videos/new-comment.ts +++ b/apps/web/actions/videos/new-comment.ts @@ -49,11 +49,19 @@ export async function newComment(data: { 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); - if (Exit.isFailure(accessExit) || accessExit.value.length === 0) { + if (Exit.isFailure(accessExit)) { + throw new Error("Video not found"); + } + + if (accessExit.value.length === 0) { throw new Error("Video not found"); } From 5808e9590c7c268d75edb6d27c60b11cc4c1172b Mon Sep 17 00:00:00 2001 From: Jiho Lee Date: Sat, 25 Jul 2026 19:02:10 +0900 Subject: [PATCH 3/3] perf(web): provide the already-loaded user instead of re-resolving it Addresses review feedback: newComment already requires auth and holds the user, so provideOptionalAuth would redundantly call getServerSession() and re-query users. Provide the current-user layer directly. --- apps/web/actions/videos/new-comment.ts | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/apps/web/actions/videos/new-comment.ts b/apps/web/actions/videos/new-comment.ts index 45f486380fb..be2a6f3a10c 100644 --- a/apps/web/actions/videos/new-comment.ts +++ b/apps/web/actions/videos/new-comment.ts @@ -4,7 +4,7 @@ import { db } from "@cap/database"; import { getCurrentUser } from "@cap/database/auth/session"; import { nanoId } from "@cap/database/helpers"; import { comments, videos } from "@cap/database/schema"; -import { provideOptionalAuth, VideosPolicy } from "@cap/web-backend"; +import { makeCurrentUserLayer, 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"; @@ -55,7 +55,13 @@ export async function newComment(data: { .where(eq(videos.id, videoId)) .limit(1), ).pipe(Policy.withPublicPolicy(videosPolicy.canView(videoId))); - }).pipe(provideOptionalAuth, EffectRuntime.runPromiseExit); + }).pipe( + // This action already required auth above, so provide the user we have + // rather than `provideOptionalAuth`, which would re-run getServerSession() + // and re-query `users`. + Effect.provide(makeCurrentUserLayer(user)), + EffectRuntime.runPromiseExit, + ); if (Exit.isFailure(accessExit)) { throw new Error("Video not found");