Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
136 changes: 136 additions & 0 deletions apps/web/__tests__/unit/transcribe-status-access.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,136 @@
import { Exit, Layer } from "effect";
import { beforeEach, describe, expect, it, vi } from "vitest";

const mockDbChain = {
select: vi.fn(),
from: vi.fn(),
where: vi.fn(),
limit: vi.fn(),
};

function resetDbChain() {
for (const fn of Object.values(mockDbChain)) fn.mockClear();
mockDbChain.select.mockReturnValue(mockDbChain);
mockDbChain.from.mockReturnValue(mockDbChain);
mockDbChain.where.mockReturnValue(mockDbChain);
mockDbChain.limit.mockReturnValue(
Promise.resolve([{ id: "video-1", transcriptionStatus: "COMPLETE" }]),
);
}

vi.mock("@cap/database", () => ({ db: () => mockDbChain }));

vi.mock("@cap/database/schema", () => ({
videos: { id: "id" },
}));

const mockGetCurrentUser = vi.fn();
vi.mock("@cap/database/auth/session", () => ({
getCurrentUser: () => mockGetCurrentUser(),
}));

const mockCanView = vi.fn();
vi.mock("@cap/web-backend", () => ({
makeCurrentUserLayer: () => Layer.empty,
VideosPolicy: {
key: "VideosPolicy",
},
}));

// The route runs its query through the Effect runtime; the parts under test are
// which failures reach the handler and how it maps them, so the runtime is
// replaced with one that returns the exit the policy would have produced.
vi.mock("@/lib/server", () => ({
runPromiseExit: () => mockCanView(),
}));

vi.mock("@cap/web-domain", async () => {
const actual =
await vi.importActual<typeof import("@cap/web-domain")>("@cap/web-domain");
return {
...actual,
Policy: {
...actual.Policy,
withPublicPolicy: () => (self: unknown) => self,
},
};
});

const { GET } = await import("@/app/api/video/transcribe/status/route");
const { DatabaseError, Policy } = await import("@cap/web-domain");

const request = (videoId: string) =>
new Request(
`https://cap.test/api/video/transcribe/status?videoId=${videoId}`,
) as never;

describe("GET /api/video/transcribe/status", () => {
beforeEach(() => {
resetDbChain();
mockGetCurrentUser.mockResolvedValue({ id: "user-1" });
mockCanView.mockReset();
});

it("returns 401 when there is no session", async () => {
mockGetCurrentUser.mockResolvedValue(null);

const res = await GET(request("video-1"));

expect(res.status).toBe(401);
});

it("returns 400 when videoId is missing", async () => {
const res = await GET(
new Request("https://cap.test/api/video/transcribe/status") as never,
);

expect(res.status).toBe(400);
});

it("returns 404 when the access policy denies the caller", async () => {
mockCanView.mockResolvedValue(
Exit.fail(new Policy.PolicyDeniedError({ reason: "denied" })),
);

const res = await GET(request("video-1"));

expect(res.status).toBe(404);
await expect(res.json()).resolves.toMatchObject({
message: "Video does not exist",
});
});

it("returns 500, not 404, when the access check hits a database failure", async () => {
mockCanView.mockResolvedValue(
Exit.fail(new DatabaseError({ cause: new Error("connection reset") })),
);

const res = await GET(request("video-1"));

expect(res.status).toBe(500);
await expect(res.json()).resolves.toMatchObject({
message: "Failed to fetch transcription status",
});
});

it("returns the transcription status when access is granted", async () => {
mockCanView.mockResolvedValue(
Exit.succeed([{ id: "video-1", transcriptionStatus: "COMPLETE" }]),
);

const res = await GET(request("video-1"));

expect(res.status).toBe(200);
await expect(res.json()).resolves.toEqual({
transcriptionStatus: "COMPLETE",
});
});

it("returns 404 when the video row is missing", async () => {
mockCanView.mockResolvedValue(Exit.succeed([]));

const res = await GET(request("video-1"));

expect(res.status).toBe(404);
});
});
51 changes: 49 additions & 2 deletions apps/web/app/api/video/transcribe/status/route.ts
Original file line number Diff line number Diff line change
@@ -1,9 +1,12 @@
import { db } from "@cap/database";
import { getCurrentUser } from "@cap/database/auth/session";
import { videos } from "@cap/database/schema";
import type { Video } from "@cap/web-domain";
import { makeCurrentUserLayer, VideosPolicy } from "@cap/web-backend";
import { DatabaseError, Policy, type Video } from "@cap/web-domain";
import { eq } from "drizzle-orm";
import { Cause, Effect, Exit, Option, Schema } from "effect";
import type { NextRequest } from "next/server";
import * as EffectRuntime from "@/lib/server";

export const dynamic = "force-dynamic";

Expand All @@ -23,7 +26,51 @@ export async function GET(request: NextRequest) {
);
}

const video = await db().select().from(videos).where(eq(videos.id, videoId));
// `videoId` is caller-supplied, so an authenticated session is not enough:
// without this any signed-in user could read the transcription status of
// someone else's private recording. The `getVideoStatus` server action,
// which returns this same field, already gates on `canView`.
const exit = await Effect.gen(function* () {
const videosPolicy = yield* VideosPolicy;

return yield* Effect.promise(() =>
db().select().from(videos).where(eq(videos.id, videoId)).limit(1),
).pipe(Policy.withPublicPolicy(videosPolicy.canView(videoId)));
}).pipe(
// The route 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(exit)) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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                                                              404

I 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.

const failure = Cause.failureOption(exit.cause);

// The 404-for-denied behaviour below is deliberate, so this endpoint
// can't be used to probe which video IDs exist. A database failure is
// not a denial though: `canView` reads the video plus its org/space
// memberships, so an outage in any of those would otherwise be reported
// as "video does not exist" for a video the caller can actually see.
if (Option.isSome(failure) && Schema.is(DatabaseError)(failure.value)) {
console.error(
"[transcribe/status] database error while checking video access:",
failure.value.cause,
);

return Response.json(
{ error: true, message: "Failed to fetch transcription status" },
{ status: 500 },
);
}

return Response.json(
{ error: true, message: "Video does not exist" },
{ status: 404 },
);
}

const video = exit.value;

if (video.length === 0 || !video[0]) {
return Response.json(
Expand Down