-
Notifications
You must be signed in to change notification settings - Fork 1.6k
fix(web): require canView on transcript and AI metadata endpoints #1926
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,10 +3,11 @@ | |
| import { db } from "@cap/database"; | ||
| import { getCurrentUser } from "@cap/database/auth/session"; | ||
| import { videos } from "@cap/database/schema"; | ||
| import { Storage } from "@cap/web-backend"; | ||
| import type { Video } from "@cap/web-domain"; | ||
| import { provideOptionalAuth, Storage, VideosPolicy } from "@cap/web-backend"; | ||
| import { Policy, type Video } from "@cap/web-domain"; | ||
| import { eq } from "drizzle-orm"; | ||
| import { Effect, Option } from "effect"; | ||
| import { Effect, Exit, Option } from "effect"; | ||
| import * as EffectRuntime from "@/lib/server"; | ||
| import { runPromise } from "@/lib/server"; | ||
| import { decodeStorageVideo } from "@/lib/video-storage"; | ||
|
|
||
|
|
@@ -22,10 +23,19 @@ export async function getTranscript( | |
| }; | ||
| } | ||
|
|
||
| const query = await db() | ||
| .select({ video: videos }) | ||
| .from(videos) | ||
| .where(eq(videos.id, videoId)); | ||
| const exit = await Effect.gen(function* () { | ||
| const videosPolicy = yield* VideosPolicy; | ||
|
|
||
| return yield* Effect.promise(() => | ||
| db().select({ video: videos }).from(videos).where(eq(videos.id, videoId)), | ||
| ).pipe(Policy.withPublicPolicy(videosPolicy.canView(videoId))); | ||
| }).pipe(provideOptionalAuth, EffectRuntime.runPromiseExit); | ||
|
|
||
| if (Exit.isFailure(exit)) { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Minor: this treats any failure (including unexpected DB/runtime errors) as "Video not found". If that’s intentional for the client response, it might still be worth logging the failure so real issues don’t get silently masked in production. |
||
| return { success: false, message: "Video not found" }; | ||
| } | ||
|
|
||
| const query = exit.value; | ||
|
|
||
| if (query.length === 0) { | ||
| return { success: false, message: "Video not found" }; | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,76 @@ | ||||||||||||||||||||||||
| import { checkRateLimit } from "@vercel/firewall"; | ||||||||||||||||||||||||
| import { headers as nextHeaders } from "next/headers"; | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||
| * Best-effort per-key rate limiting backed by the Vercel Firewall. | ||||||||||||||||||||||||
| * | ||||||||||||||||||||||||
| * IMPORTANT: each `ruleId` passed here must also be configured as a Rate Limit | ||||||||||||||||||||||||
| * rule in the Vercel Firewall dashboard (Firewall → Rate Limiting) with a | ||||||||||||||||||||||||
| * `@vercel/firewall` rule condition whose ID matches `ruleId`, plus the desired | ||||||||||||||||||||||||
| * window / limit / action. An ID that has no matching dashboard rule fails | ||||||||||||||||||||||||
| * OPEN (`checkRateLimit` returns `{ rateLimited: false, error: "not-found" }`), | ||||||||||||||||||||||||
| * so this helper never breaks self-hosted deploys that lack the firewall — but | ||||||||||||||||||||||||
| * it also provides no protection until the rule exists. | ||||||||||||||||||||||||
| * | ||||||||||||||||||||||||
| * Mirrors the existing pattern in `actions/collections/password.ts` and | ||||||||||||||||||||||||
| * `actions/send-download-link.ts`: | ||||||||||||||||||||||||
| * - only enforced in production, | ||||||||||||||||||||||||
| * - best-effort (any error → not limited) so a firewall/IP-header outage can | ||||||||||||||||||||||||
| * never take down the underlying feature. | ||||||||||||||||||||||||
| * | ||||||||||||||||||||||||
| * @param ruleId Stable rule id, also configured in the Vercel Firewall. | ||||||||||||||||||||||||
| * @param opts.key Optional bucket key (e.g. per-email / per-user). Defaults to | ||||||||||||||||||||||||
| * the caller IP (the firewall's default behaviour). | ||||||||||||||||||||||||
| * @param opts.headers Optional request headers (required inside Hono handlers | ||||||||||||||||||||||||
| * where `next/headers` is unavailable; defaults to the App | ||||||||||||||||||||||||
| * Router request headers). | ||||||||||||||||||||||||
| * @returns `true` when the request should be rejected. | ||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||
| export async function isRateLimited( | ||||||||||||||||||||||||
| ruleId: string, | ||||||||||||||||||||||||
| opts?: { key?: string; headers?: Headers }, | ||||||||||||||||||||||||
| ): Promise<boolean> { | ||||||||||||||||||||||||
| if (process.env.NODE_ENV !== "production") return false; | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| try { | ||||||||||||||||||||||||
| const headersList = opts?.headers ?? (await nextHeaders()); | ||||||||||||||||||||||||
| const request = new Request("https://cap.so/api/rate-limit", { | ||||||||||||||||||||||||
| method: "POST", | ||||||||||||||||||||||||
| headers: headersList, | ||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| const { rateLimited } = await checkRateLimit(ruleId, { | ||||||||||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since
Suggested change
|
||||||||||||||||||||||||
| request, | ||||||||||||||||||||||||
| ...(opts?.key ? { rateLimitKey: opts.key } : {}), | ||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| return rateLimited; | ||||||||||||||||||||||||
| } catch (error) { | ||||||||||||||||||||||||
| console.warn(`Rate limit check failed for "${ruleId}":`, error); | ||||||||||||||||||||||||
| return false; | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||
| * Canonical Vercel Firewall rate-limit rule ids introduced by the security | ||||||||||||||||||||||||
| * hardening pass. Each MUST be created in the Vercel Firewall dashboard for the | ||||||||||||||||||||||||
| * corresponding protection to take effect (see `isRateLimited`). | ||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||
| export const RATE_LIMIT_IDS = { | ||||||||||||||||||||||||
| /** Email OTP verification attempts (brute-force guard). Suggested: 10 / 10m per key (email). */ | ||||||||||||||||||||||||
| AUTH_OTP_VERIFY: "rl_auth_otp_verify", | ||||||||||||||||||||||||
| /** Email OTP / magic-link send (mailbomb + token-reseed guard). Suggested: 5 / 10m per key (email). */ | ||||||||||||||||||||||||
| AUTH_OTP_SEND: "rl_auth_otp_send", | ||||||||||||||||||||||||
| /** Unauthed Loom download/convert (ffmpeg + memory DoS). Suggested: 10 / 1m per IP. */ | ||||||||||||||||||||||||
| LOOM_DOWNLOAD: "rl_loom_download", | ||||||||||||||||||||||||
| /** Unauthed transcript translation (Groq cost). Suggested: 10 / 1m per IP. */ | ||||||||||||||||||||||||
| TRANSLATE_TRANSCRIPT: "rl_translate_transcript", | ||||||||||||||||||||||||
| /** Anonymous support-chat messages (Groq + Supermemory cost). Suggested: 20 / 1m per IP. */ | ||||||||||||||||||||||||
| MESSENGER_MESSAGE: "rl_messenger_message", | ||||||||||||||||||||||||
| /** Unauthed analytics view tracking (Tinybird ingest + notifications). Suggested: 60 / 1m per IP. */ | ||||||||||||||||||||||||
| ANALYTICS_TRACK: "rl_analytics_track", | ||||||||||||||||||||||||
| /** Unauthed guest checkout (Stripe object/cost abuse). Suggested: 10 / 10m per IP. */ | ||||||||||||||||||||||||
| GUEST_CHECKOUT: "rl_guest_checkout", | ||||||||||||||||||||||||
| /** Unauthed desktop log → Discord forwarding (spam). Suggested: 10 / 1m per IP. */ | ||||||||||||||||||||||||
| DESKTOP_LOGS: "rl_desktop_logs", | ||||||||||||||||||||||||
| } as const; | ||||||||||||||||||||||||
|
Comment on lines
+59
to
+76
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Several IDs — Prompt To Fix With AIThis is a comment left during a code review.
Path: apps/web/lib/rate-limit.ts
Line: 59-76
Comment:
**`RATE_LIMIT_IDS` entries without corresponding call-sites in this PR**
Several IDs — `AUTH_OTP_VERIFY`, `AUTH_OTP_SEND`, `LOOM_DOWNLOAD`, `MESSENGER_MESSAGE`, `ANALYTICS_TRACK`, `GUEST_CHECKOUT`, `DESKTOP_LOGS` — are defined here but `isRateLimited` is only wired up for `TRANSLATE_TRANSCRIPT` in this PR. The doc-comment already explains that an ID with no matching Vercel Firewall dashboard rule fails open, so the constants themselves are harmless. That said, it is worth verifying the IDs are either already used elsewhere or are tracked for follow-up — a constant that silently provides no protection could be misleading.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time! |
||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
VideosPolicy.canViewcallsrepo.getById(videoId)internally (seebuildCanViewinpackages/web-backend/src/Videos/VideosPolicy.ts), then the outerEffect.promise(() => db().select({ video: videos })…)fetches the exact same row a second time. The same pattern is present inget-available-translations.ts,translate-transcript.ts, andapi/video/ai/route.ts. SincewithPublicPolicyruns the policy before the wrapped effect, every request now incurs two sequential reads for the same video. If this becomes a measurable latency concern, the results fromcanView's internal fetch could be threaded out and reused instead of re-querying.Prompt To Fix With AI