fix(web): verify space membership before listing space and folder videos

[DPS0340]

Problem

getSpaceVideoIds and getFolderVideoIds are both "use server" actions that authenticate the caller and then trust the spaceId argument:

const user = await getCurrentUser();
if (!user || !user.id) throw new Error("Unauthorized");

const isAllSpacesEntry = user.activeOrganizationId === spaceId;

const videoIds = isAllSpacesEntry
  ? /* scoped to the user's own org */
  : await db().select({ videoId: spaceVideos.videoId })
      .from(spaceVideos)
      .where(and(eq(spaceVideos.spaceId, spaceId), isNull(spaceVideos.folderId)));

In the else branch there is no membership check at all. Any authenticated user can pass an arbitrary spaceId and get back the list of video IDs in a space belonging to an organization they have nothing to do with. getFolderVideoIds has the identical shape for folder contents.

Video IDs are the sensitive part here — they're the key into /s/{videoId} and into the other video endpoints, so leaking the set of IDs in a private space is a meaningful disclosure even before any per-video check runs.

Fix

Reuse the existing getSpaceAccess helper from @/actions/organization/space-authorization, which already resolves both organization-level and space-level roles:

if (!isAllSpacesEntry) {
  const access = await getSpaceAccess(user.id, spaceId);
  if (!access || (!access.organizationRole && !access.spaceRole)) {
    throw new Error("Space not found");
  }
}

Two deliberate choices:

• getSpaceAccess, not requireSpaceManager. These are read paths. requireSpaceManager would have rejected ordinary members, breaking legitimate use. Requiring either an org role or a space role matches who can actually see a space today.
• The isAllSpacesEntry branch is left untouched. It keys off user.activeOrganizationId, which is read from the user's own DB record rather than supplied by the caller, so that branch is already scoped correctly. Adding a check there would have been noise.

Error text is "Space not found" / "Folder not found" rather than "forbidden", so this doesn't become a probe for which space IDs exist.

Verification

• biome check on both files — clean.
• tsc --noEmit on apps/web: the TS7006 diagnostics on these two files are pre-existing. Confirmed by stashing the change and re-running — they appear identically, just at the earlier line numbers (38→49, 46→57). No new diagnostics.
• @cap/web-domain, @cap/web-backend, @cap/database build clean.

Diff: 2 files, +20/-0.

Related, same class of missing access check: #2029 (newComment, #1982) and #2030 (getVideoAnalytics).

[tembo]

Good hardening. One concern: this new access check keys off the caller-supplied spaceId, but the actual query below only filters by folderId (no spaceId / org constraint). If someone can obtain/guess a folderId from another space/org, they could pass a space they do have access to and still enumerate that folder’s video IDs. Might be worth also scoping the query by spaceVideos.spaceId in the space branch, and by sharedVideos.organizationId (ideally user.activeOrganizationId, not spaceId) in the all-spaces branch.

[DPS0340]

@tembo please review

[DPS0340]

Good catch — that was a real hole in my fix, not a nitpick. Pushed 5c58b58.

You're right that the membership check alone was insufficient: folderId is also caller-supplied, and the queries only filtered on it. So a caller could pass a space they legitimately belong to together with a folder from another space and still read its contents — the check would pass and the query would happily return the wrong folder's rows.

Both queries are now constrained to the space/org that was just authorized:

// space branch
.where(and(eq(spaceVideos.folderId, folderId), eq(spaceVideos.spaceId, spaceId)))

// all-spaces branch
.where(and(eq(sharedVideos.folderId, folderId), eq(sharedVideos.organizationId, spaceId)))

On your activeOrganizationId vs spaceId point for the all-spaces branch: they're provably equal there, since that branch is only taken when user.activeOrganizationId === spaceId. I used spaceId to keep both branches reading the same way, but I'm happy to switch it to user.activeOrganizationId if you'd rather the authorization source be explicit at the point of use — that's a fair readability argument.

I also checked get-space-videos.ts for the same problem: it was already scoping by spaceVideos.spaceId / sharedVideos.organizationId, so only the folder path was affected.

Updated the comment to explain both layers, since the reason the query scoping is load-bearing isn't obvious from the check alone.

[DPS0340]

@tembo please re-review

[tembo]

Minor: consider validating spaceId up front (like getSpaceVideoIds), so a missing/empty value doesn't fall through to getSpaceAccess and return a misleading "Folder not found".

Suggested change

	const isAllSpacesEntry = user.activeOrganizationId === spaceId;
	if (!spaceId) {
	throw new Error("Space ID is required");
	}

[DPS0340]

Adopted — pushed. You're right that it produced a misleading error: an empty spaceId would reach getSpaceAccess, find nothing, and surface "Folder not found" when the actual problem was a missing argument. Now validated up front, matching getSpaceVideoIds.

[DPS0340]

@tembo please re-review

[tembo]

Small readability/type-safety thing: in this branch we already know spaceId === user.activeOrganizationId, so using user.activeOrganizationId here makes it explicit we're scoping by a trusted org id (and avoids leaning on the caller-supplied arg in the query).

Suggested change

	eq(sharedVideos.organizationId, spaceId),
	eq(sharedVideos.organizationId, user.activeOrganizationId),

[DPS0340]

Adopted — pushed. This is the same point I raised in my earlier reply and you're right that explicit is better: in that branch spaceId === user.activeOrganizationId holds by construction, so reading the trusted value directly means the query no longer appears to depend on caller input even though the two are equal there.

That leaves a clean split in the file: the space branch scopes by the spaceId we just authorized via getSpaceAccess, and the all-spaces branch scopes by the org id off the user record. Neither query now takes an unvalidated argument.

tsc is happy with it — activeOrganizationId narrows fine inside the branch since the isAllSpacesEntry comparison already established it.

[DPS0340]

@tembo please re-review

[DPS0340]

@richiemcilroy — your own #1928 already covers this, and it covers more. Please take that one over this.

I missed it when I opened this. #1928 (19 June, yours) touches both files I touch here and four more:

#1928  add-videos.ts, get-folder-videos.ts, moveVideoToFolder.ts,
       get-organization-videos.ts, get-space-videos.ts, folder/[id]/page.tsx
#2031  get-folder-videos.ts, get-space-videos.ts

Same approach, too — gate on getSpaceAccess / getOrganizationAccess before disclosing contents. Yours additionally covers the mutating paths (add-videos, moveVideoToFolder), which is the more important half and which I did not touch at all.

There is nothing in mine worth salvaging on top of yours. Closing this in favour of #1928 unless you want it kept as a fallback.

---

Two more of mine overlap open work, which I should have checked before opening any of them:

• fix(web): enforce video access policy before creating comments (#1982) #2029 duplicates @eponomariova-dotcom's fix(comments): require canView access before posting a comment #2021 (20 July, same file, more review rounds) — noted there, same recommendation.
• fix(web): wire three unused rate-limit ids and guard against new ones #2040 touches three files also touched by your fix(web): rate-limit expensive endpoints and stop analytics tenant spoofing #1929. That one is not a duplicate — fix(web): rate-limit expensive endpoints and stop analytics tenant spoofing #1929 rate-limits endpoints, mine wires up three RATE_LIMIT_IDS that are declared but never called and adds a test so a new unused id fails CI instead of shipping silently. But they will conflict textually, and yours should land first.

The check I skipped is cheap and I'm sorry for the noise: for each file a PR touches, list open PRs touching the same file. I've now run it across all nine of mine — the other six (#2030, #2032, #2034, #2035, #2036, #2038) have no functional overlap with anything open.

[DPS0340]

Closing as promised — #1928 (yours) 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 #1928 (yours) lands, I'll send it as a small follow-up against that.