fix(web): enforce video access policy before creating comments (#1982)

[DPS0340]

Fixes #1982 (GHSA-5r3r-v2f5-wqwh).

Problem

The newComment server action authenticates the caller, then inserts the comment without ever checking whether that user is allowed to access the target video:

const user = await getCurrentUser();
if (!user) throw new Error("User not authenticated");
// ...no video access check...
await db().insert(comments).values(newComment);

videoId is a client-supplied argument to a server action, so being signed in is the only barrier. Any authenticated user can post comments — or emoji reactions, or threaded replies — into any video by ID, including another user's private recording. The comment then shows up in the owner's activity feed and triggers a notification to them via createNotification.

Worth noting the same file already treats deletion correctly: deleteComment verifies authorId before removing anything. Creation was the gap.

Fix

Gate the insert behind the existing VideosPolicy.canView policy before any write happens.

I deliberately did not invent a new access-check shape — this mirrors the pattern already used in this exact directory by getTranscript (apps/web/actions/videos/get-transcript.ts), and it is the same policy the /api/video/ai fix in #1926 used to close the sibling advisory #1981:

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");
}

Because it reuses canView, it inherits the full existing access model for free — owner, org membership, space membership, public videos, password-protected videos, and email-domain restrictions — rather than approximating it with an ownership check that would have broken legitimate commenting on shared videos.

The error message is deliberately "Video not found" rather than "Access denied", so this does not become an oracle for probing which video IDs exist.

Verification

• biome check apps/web/actions/videos/new-comment.ts — clean.
• tsc --noEmit on apps/web: the remaining diagnostics on this file (TS6305, plus TS2488/TS18046 on the Effect generator) are not introduced by this change — the untouched get-transcript.ts emits the identical set, since it uses the same pattern and the workspace tsconfig project references aren't built in a plain tsc run. Verified by diffing the two files' diagnostics.
• @cap/web-domain, @cap/web-backend and @cap/database build clean via turbo.

Diff: 1 file, +22/-2.

[tembo]

Minor: splitting the Exit.isFailure check from the .value access lets the type guard narrow cleanly (and matches the pattern used in get-transcript.ts).

Suggested change

	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");
	}

[DPS0340]

@tembo please review

[DPS0340]

Adopted — pushed. Split the branches so the type guard narrows cleanly, matching get-transcript.ts. Also added .limit(1) to the lookup while I was in there, since videos.id is unique.

[DPS0340]

@tembo please re-review

[tembo]

Minor perf/consistency nit: provideOptionalAuth will call getServerSession() + query users again. Since this action already requires auth and you already have user, you can provide the current-user layer directly and skip the extra DB hit.

Suggested change

	import type { ImageUpload } from "@cap/web-domain";
	import { makeCurrentUserLayer, VideosPolicy } from "@cap/web-backend";

[tembo]

Following on the above import, this avoids re-fetching the current user inside the Effect env.

Suggested change

	}).pipe(Effect.provide(makeCurrentUserLayer(user)), EffectRuntime.runPromiseExit);

[DPS0340]

Adopted — pushed.

Verified the redundancy before changing it: provideOptionalAuth is literally

const user = yield* getCurrentUser;   // getServerSession() + select from users
return yield* user.pipe(Option.match({
  onNone: () => app,
  onSome: (user) => app.pipe(Effect.provide(makeCurrentUserLayer(user))),
}));

so on a path that has already established user it re-runs both the session lookup and the users query to arrive at the same layer. Since this action throws on !user before the policy check, the onNone branch is unreachable here — provideOptionalAuth was pure overhead.

Now Effect.provide(makeCurrentUserLayer(user)), which matches what dashboard/folder/[id]/page.tsx and dashboard/spaces/[spaceId]/page.tsx already do.

Small bonus: this also removed a TS18046 'accessExit.value' is of type 'unknown' diagnostic — providing the layer concretely gives the compiler a narrower env type than the provideOptionalAuth wrapper did.

[DPS0340]

@tembo please re-review

[DPS0340]

@tembo please re-review

[DPS0340]

@richiemcilroy — nine PRs from me are sitting green and I don't want them to read as a wall, so here they are ordered by review cost. Every one is MERGEABLE, rebased on current main (5b81ee5b), and every @tembo note is answered.

Cheapest three — one file each, no behavioural surface beyond the fix:

PR	size	what
#2036	+6/-0, 2 files	self-hosting: AI provider keys never reached the web container
#2035	+29/-0, 4 files	self-hosting: same for CAP_CHROME_EXTENSION_ID (#1993)
#2030	+30/-8, 1 file	getVideoAnalytics server action wasn't gated on the video access policy

The access-policy set — these are one theme, and reviewing #2029 first makes the rest fast:

• fix(web): enforce video access policy before creating comments (#1982) #2029 (+36/-2) — comments could be created on videos the caller can't view (Authenticated Users Can Post Comments on Private Videos Without Access Permission #1982)
• fix(web): verify space membership before listing space and folder videos #2031 (+45/-3) — space/folder video listing didn't verify space membership
• fix(web): gate /api/video/transcribe/status on video access policy #2032 (+185/-2) — /api/video/transcribe/status ungated
• fix(web): require an explicit share to download, matching the view policy #2038 (+149/-18) — download permission granted the owner's whole org while view required an explicit share; this aligns them

Two standalone:

• fix(export): match ProRes swscale output to the declared full colour range #2034 (+78/-3) — ProRes swscale output didn't match the declared full colour range
• fix(web): wire three unused rate-limit ids and guard against new ones #2040 (+145/-0) — 3 of 13 RATE_LIMIT_IDS were declared but never called, so those endpoints had no rate limit; also adds a guard so a new unused id fails a test instead of silently shipping

Two notes so review time goes to the code and not to context:

• The Vercel — Authorization required to deploy red mark is not mine. It's the fork-preview authorisation, and it's present on merged PRs too — fix(recording): prevent double-count of VideoStartGate trim in mic_start_time #1966 carries the identical failure. Everything else (Socket, Superagent, Contributor trust) is green.
• fix(web): require an explicit share to download, matching the view policy #2038 had a real defect that @tembo caught — a signature change with a stale call site in apps/web/app/s/[videoId]/page.tsx that would have broken the build. Fixed in b988753, and I re-grepped the other call sites rather than assuming that was the only one.

No rush on any individual one — happy for you to take the three cheap ones and leave the rest, if that's the useful split.

[DPS0340]

@richiemcilroy — #2021 already fixes this, and it got there first. Please review that one instead of this.

I opened this on 25 July without noticing @eponomariova-dotcom's PR from 20 July. They are the same fix in the same file, and theirs is the more finished one:

	#2021 (theirs, 20 Jul)	#2029 (mine, 25 Jul)
file	apps/web/actions/videos/new-comment.ts	same
size	+33/-2	+36/-2
review rounds	8 @tembo notes, all addressed	3

Their version also independently caught the subtlety I did — canView returns true for a nonexistent videoId by design, so gating a no-op still lets a bogus id reach the insert; both fetch the row under the policy. On one point theirs is better: @tembo pointed out that a bare Exit.isFailure also swallows operational errors (DB connection) as "Video not found", and they split and logged it. Mine still collapses both.

The only real difference left is provideOptionalAuth (theirs) vs Effect.provide(makeCurrentUserLayer(user)) (mine). I chose the latter to avoid re-running getServerSession() and re-querying users when the action has already authenticated — a small saving, and not worth preferring a later, less-reviewed PR over an earlier one. If it's useful it's a two-line follow-up to theirs, and I'm happy to send that once #2021 lands.

I'll close this in favour of #2021 unless you'd rather I keep it open as a fallback — say the word and it's gone.

Sorry for the duplicated review surface. I checked #1982 for existing linked PRs but not for open PRs referencing it, which is the check that would have caught this before I wrote anything. My other eight PRs don't overlap #2021 — I verified each touches a different file.

[DPS0340]

Closing as promised — #2021 by @eponomariova-dotcom covers this and got there first.

No action needed from anyone; I'm removing it from the review queue rather than leaving it sitting as something you'd have to diff and dismiss. Nothing here is lost: if any piece turns out to be worth having after #2021 by @eponomariova-dotcom lands, I'll send it as a small follow-up against that.