fix(web): gate /api/video/transcribe/status on video access policy#2032
fix(web): gate /api/video/transcribe/status on video access policy#2032DPS0340 wants to merge 4 commits into
Conversation
The route authenticated the caller but then looked the video up by the caller-supplied videoId with no access check, so any signed-in user could read the transcription status of another user's private recording. The getVideoStatus server action returns this same field and already gates on VideosPolicy.canView; apply the same policy here.
| const videosPolicy = yield* VideosPolicy; | ||
|
|
||
| return yield* Effect.promise(() => | ||
| db().select().from(videos).where(eq(videos.id, videoId)), |
There was a problem hiding this comment.
Minor perf: since videos.id is unique, consider adding .limit(1) so the DB can stop early.
| db().select().from(videos).where(eq(videos.id, videoId)), | |
| return yield* Effect.promise(() => | |
| db().select().from(videos).where(eq(videos.id, videoId)).limit(1), | |
| ).pipe(Policy.withPublicPolicy(videosPolicy.canView(videoId))); |
| ).pipe(Policy.withPublicPolicy(videosPolicy.canView(videoId))); | ||
| }).pipe(provideOptionalAuth, EffectRuntime.runPromiseExit); | ||
|
|
||
| if (Exit.isFailure(exit)) { |
There was a problem hiding this comment.
Exit.isFailure(exit) can also represent unexpected errors (DB connectivity, etc.). If you want to keep the 404-for-deny behavior to avoid an existence oracle, it might be worth distinguishing policy denial from other failures (and logging/returning 500 for the latter) so real outages aren't silently masked.
There was a problem hiding this comment.
I was wrong to skip this one, and re-checking it showed the case is real. Adopted in 236db3478.
Why my first answer was wrong. I'd argued the failure channel here was effectively only PolicyDenied, so collapsing everything to 404 was safe. It isn't. canView reads through VideosRepo.getById, spacesRepo.passwordsForVideo, orgsRepo.membershipForVideo and orgsRepo.allowedEmailDomain — all of which go through Database.use, which wraps failures as DatabaseError:
// packages/web-backend/src/Database.ts
use: <T>(cb) => Effect.tryPromise({ try: () => cb(db()), catch: (cause) => new DatabaseError({ cause }) })So DatabaseError shares the failure channel with PolicyDeniedError, and my code mapped both to 404 Video does not exist. During a database blip, a user asking about a video they genuinely own gets told it doesn't exist, and nothing is logged.
The fix keys off the tag rather than treating every failure alike, using the same Schema.is narrowing OrganisationsRpcs.ts uses for error mapping:
if (Option.isSome(failure) && Schema.is(DatabaseError)(failure.value)) → 500 + console.error
otherwise → 404I kept the 404-for-denied behaviour, since as you noted that's the existence-oracle defence and it's worth keeping.
Proof it bites, rather than just asserting it. Added __tests__/unit/transcribe-status-access.test.ts (6 cases: 401, 400, denial→404, DB failure→500, success, missing row→404). All 6 pass with the fix. Reverting only route.ts to the previous commit and re-running:
× returns 500, not 404, when the access check hits a database failure
Tests 1 failed | 5 passed (6)
Exactly the one case, so the test is pinned to this behaviour and not to incidental structure.
tsc --noEmit clean for both files, biome check clean.
Note on verifying this locally: tsc in apps/web reports TS6305 for every workspace import until npx tsc --build packages/web-domain packages/web-backend packages/database has been run — worth knowing before trusting a "type error" in this app.
|
@tembo please review |
Addresses review feedback: videos.id is unique so the DB can stop early.
|
Took one of the two, and want to explain why I skipped the other rather than silently ignoring it. Adopted — Not adopted — distinguishing But I checked how this repo actually handles it, and no API route collapsing an
So I'd rather not smuggle an error-handling convention change into a security fix. If you do want the distinction, it seems like it should land as its own change across all four routes at once — happy to open that separately if you're interested, but I didn't want to decide that unilaterally inside this PR. |
|
@tembo please re-review |
Same redundancy tembo flagged on CapSoftware#2029: the route already requires auth and holds the user, so provideOptionalAuth would re-run getServerSession() and re-query users to build the same layer.
|
Applied the Also dropped the three |
|
@tembo please re-review |
1 similar comment
|
@tembo please re-review |
|
@tembo please re-review |
Problem
/api/video/transcribe/statusauthenticates the caller and then trusts thevideoIdquery parameter:Being signed in is the only barrier, so any authenticated user can read the transcription status of any video by ID — including private recordings belonging to someone else. It also doubles as an existence oracle, since a missing video returns 404 while an existing one returns a status.
The clearest evidence that this is a gap rather than intentional: the
getVideoStatusserver action returns this exact same field and already gates onVideosPolicy.canView(apps/web/actions/videos/get-status.ts). Two entry points to the same data, only one of them checked.Fix
Apply the same policy the server action uses, folded onto the query that was already being made — no extra round-trip:
Reusing
canViewkeeps every legitimate caller working — owners, org members, space members, public videos, and password-protected videos with a valid cookie.The denial response deliberately reuses the existing
"Video does not exist"404 rather than a distinct 403, so an unauthorized caller cannot distinguish "exists but private" from "does not exist".Verification
biome check— clean.tsc --noEmitonapps/web: theTS2488/TS18046diagnostics on this file are artifacts of the Effect generator pattern under an unbuilt workspace, not regressions — the untouchedactions/videos/get-status.ts, which uses the identical pattern, emits the same pair. Verified by diffing the two files' diagnostics.@cap/web-domain,@cap/web-backend,@cap/databasebuild clean.Diff: 1 file, +21/-2.
Same class of missing access check as #2029 (
newComment, closes #1982), #2030 (getVideoAnalytics), and #2031 (space/folder video listing). This one is the lastvideoId-taking endpoint I found without a check.