Skip to content

fix(web): require an explicit share for org video downloads - #2067

Merged
richiemcilroy merged 1 commit into
mainfrom
download-org-share-check
Aug 3, 2026
Merged

fix(web): require an explicit share for org video downloads#2067
richiemcilroy merged 1 commit into
mainfrom
download-org-share-check

Conversation

@richiemcilroy

@richiemcilroy richiemcilroy commented Aug 3, 2026

Copy link
Copy Markdown
Member

Download access was broader than view access.

VideosPolicy.canView grants an org member access only through an explicit sharedVideos row (via OrganisationsRepo.membershipForVideo), and no video-creation path writes one, so a private recording is invisible to colleagues. canUserDownloadVideo additionally trusted the video's own orgId, which meant any member of the owner's organization could download any video created in that org, including ones they get a 404 on. getVideoDownloadInfo is a directly invocable server action with no second guard: it authenticates, calls this helper, and returns a signed S3 URL.

This drops orgId from the lookup so download requires an explicit org or space share. Owners keep access, explicit shares are unchanged, and no listing surface is affected (dashboard, folders and share page all join through sharedVideos).

One deliberate behaviour change worth a decision

For private videos this is pure over-grant removal: the people who lose download already could not open the video.

For a public video owned by an org member that was never explicitly shared, org colleagues can view it (anyone can) and today can also download it; after this change they cannot. Note that anonymous viewers of that same public video already could not download it, since the share page only offers download to signed-in users who pass this check, so the result is consistent rather than novel. If we would rather public videos stay downloadable by anyone signed in, that should be an explicit video.public branch here rather than an implicit consequence of org membership.

Credit to @wasim-builds, who reported and fixed this in #2052. This version takes the permission change on its own, without the unrelated sdk-recorder edit, and replaces the test (the original had a broken import path and asserted by stringifying Drizzle internals).

Validated: the new test fails 3 of 6 cases against the old logic, including the exploit case, and passes here; full web unit suite 1256/1257, the one failure a pre-existing unrelated Slack manifest test; typecheck and Biome clean.

Greptile Summary

This PR narrows video-download authorization so organization membership grants access only when the video has an explicit organization share, matching existing view authorization.

  • Removes the video's owning organization from download permission checks.
  • Updates the server action and share page to use the narrowed helper signature.
  • Adds unit coverage for owner, organization-share, and space-share permission states.

Confidence Score: 5/5

The PR appears safe to merge, with download authorization now consistently requiring ownership or an explicit organization or space share.

The changed helper matches the repository's view-policy semantics, all current callers use its new signature, and no concrete regression remains.

Important Files Changed

Filename Overview
apps/web/lib/video-download-permissions.ts Removes implicit access through the video's owning organization while preserving owner and explicit organization or space-share access.
apps/web/actions/videos/download.ts Updates the signed-download server action to call the narrowed permission helper without the video's organization ID.
apps/web/app/s/[videoId]/page.tsx Updates share-page download availability to use the same explicit-share-only permission check.
apps/web/tests/unit/video-download-permissions.test.ts Adds focused coverage for owner access, unshared organization colleagues, explicit organization shares, and space shares.

Reviews (1): Last reviewed commit: "fix(web): require an explicit share for ..." | Re-trigger Greptile

Context used:

@richiemcilroy
richiemcilroy marked this pull request as ready for review August 3, 2026 00:58
@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 464cee7 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.

1 participant