-
Notifications
You must be signed in to change notification settings - Fork 173
security(video): move temp files out of public directory and require Supabase signed URLs #828
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
ceed420
1947f0f
54afe99
723bcbb
92b9f8e
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 |
|---|---|---|
| @@ -0,0 +1,21 @@ | ||
| import fs from "fs"; | ||
| import os from "os"; | ||
| import path from "path"; | ||
| import { cleanupVideoArtifacts } from "../../lib/video/pipeline"; | ||
|
|
||
| describe("cleanupVideoArtifacts", () => { | ||
| it("removes both the rendered output and the temp directory", async () => { | ||
| const tempRoot = fs.mkdtempSync(path.join(os.tmpdir(), "doubtdesk-test-")); | ||
| const tempDir = path.join(tempRoot, "audio-run"); | ||
| const outputLocation = path.join(tempRoot, "video.mp4"); | ||
|
|
||
| fs.mkdirSync(tempDir, { recursive: true }); | ||
| fs.writeFileSync(path.join(tempDir, "audio.mp3"), "dummy"); | ||
| fs.writeFileSync(outputLocation, "dummy video"); | ||
|
|
||
| await cleanupVideoArtifacts(tempDir, outputLocation); | ||
|
|
||
| expect(fs.existsSync(outputLocation)).toBe(false); | ||
| expect(fs.existsSync(tempDir)).toBe(false); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,30 +4,28 @@ import fs from "fs"; | |
| // Bucket that holds rendered videos. Override with SUPABASE_VIDEO_BUCKET. | ||
| const VIDEO_BUCKET = process.env.SUPABASE_VIDEO_BUCKET || "videos"; | ||
|
|
||
| // How long generated video signed URLs remain valid (1 hour). | ||
| const SIGNED_URL_EXPIRY_SECONDS = 60 * 60; | ||
|
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. Suggestion: The function now returns a 1-hour signed URL and that URL is persisted in Severity Level: Critical 🚨❌ Completed videos become inaccessible after signed URL expiry.
❌ Status endpoint serves expired links, confusing client behavior.
⚠️ Users lose ability to revisit generated explanations later.Steps of Reproduction ✅1. Start a video generation by calling POST /api/video/generate
(src/app/api/video/generate/route.ts:22-84), which inserts a video_jobs row and sends a
"video/generate.requested" event with jobId to Inngest.
2. The generateVideo function in src/inngest/functions.ts:33-75 calls runVideoPipeline;
when runVideoPipeline finishes, generateVideo updates videoJobsTable to set videoUrl and
videoType from result.videoUrl and result.videoType at lines 60-71, persisting the
returned videoUrl string in the database.
3. Inside runVideoPipeline (src/lib/video/pipeline.ts:243-250),
uploadVideo(outputLocation, objectName) is called; uploadVideo in
src/lib/video/storage.ts:19-49 uploads the MP4 and then creates a Supabase signed URL via
createSignedUrl(objectName, SIGNED_URL_EXPIRY_SECONDS) at line 42, where
SIGNED_URL_EXPIRY_SECONDS is defined as 60 * 60 (1 hour) at line 8, and returns this
short-lived signedUrl.
4. A client streams GET /api/video/status?jobId=… from
src/app/api/video/status/route.ts:31-151, receiving JobSnapshot events that include
videoUrl and videoType (lines 101-109); the client uses snapshot.videoUrl for playback. If
the user revisits the job after more than one hour, the SSE stream still reports status
"completed" and the same stored videoUrl from videoJobsTable, but Supabase rejects
requests to that expired signed URL, so the completed video can no longer be viewed even
though the job is marked successful.(Use Cmd/Ctrl + Click for best experience) Prompt for AI Agent 🤖This is a comment left during a code review.
**Path:** src/lib/video/storage.ts
**Line:** 8:8
**Comment:**
*Api Mismatch: The function now returns a 1-hour signed URL and that URL is persisted in `video_jobs`, so later status reads will serve an expired link and completed videos become unusable after expiry. Store a stable object key in the job record and generate a fresh signed URL when clients request status/result instead of persisting a short-lived signed URL.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
coderabbitai[bot] marked this conversation as resolved.
|
||
|
|
||
| function getStorageClient() { | ||
| const url = process.env.NEXT_PUBLIC_SUPABASE_URL; | ||
| // Prefer a service-role key for server-side writes; fall back to the anon key | ||
| // (which requires a public bucket / permissive insert policy). | ||
| const key = | ||
| process.env.SUPABASE_SERVICE_ROLE_KEY || | ||
| process.env.NEXT_PUBLIC_SUPABASE_ANON_KEY; | ||
| if (!url || !key) return null; | ||
| return createClient(url, key, { auth: { persistSession: false } }); | ||
| } | ||
|
|
||
| /** | ||
| * Upload a rendered video file to durable object storage (Supabase Storage) and | ||
| * return its public URL (issue #321). | ||
| * | ||
| * Returns `null` when storage isn't configured, so callers can fall back to a | ||
| * local/ephemeral path in development. Throws if an upload is attempted and fails. | ||
| */ | ||
| export async function uploadVideo( | ||
| localPath: string, | ||
| objectName: string, | ||
| ): Promise<string | null> { | ||
| ): Promise<string> { | ||
| const supabase = getStorageClient(); | ||
| if (!supabase) return null; | ||
| if (!supabase) { | ||
| throw new Error( | ||
| "Video generation requires Supabase Storage. Please configure NEXT_PUBLIC_SUPABASE_URL and SUPABASE_SERVICE_ROLE_KEY.", | ||
| ); | ||
| } | ||
|
|
||
| const fileBuffer = await fs.promises.readFile(localPath); | ||
| const { error } = await supabase.storage | ||
|
|
@@ -40,6 +38,26 @@ export async function uploadVideo( | |
| throw new Error(`Video upload to storage failed: ${error.message}`); | ||
| } | ||
|
|
||
| const { data } = supabase.storage.from(VIDEO_BUCKET).getPublicUrl(objectName); | ||
| return data.publicUrl; | ||
| return objectName; | ||
| } | ||
|
|
||
| export async function getVideoSignedUrl(objectName: string): Promise<string> { | ||
| const supabase = getStorageClient(); | ||
| if (!supabase) { | ||
| throw new Error( | ||
| "Video streaming requires Supabase Storage. Please configure NEXT_PUBLIC_SUPABASE_URL and SUPABASE_SERVICE_ROLE_KEY.", | ||
| ); | ||
| } | ||
|
|
||
| const { data, error: signError } = await supabase.storage | ||
| .from(VIDEO_BUCKET) | ||
| .createSignedUrl(objectName, SIGNED_URL_EXPIRY_SECONDS); | ||
|
|
||
| if (signError || !data?.signedUrl) { | ||
| throw new Error( | ||
| `Failed to create signed URL for video${signError ? `: ${signError.message}` : ""}`, | ||
| ); | ||
| } | ||
|
|
||
| return data.signedUrl; | ||
| } | ||
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.
Suggestion: Each run creates a unique temp directory with
mkdtempSync, but only individual audio files are deleted and the directory itself is never removed, causing unbounded empty-directory buildup in/tmp. Remove the temp directory explicitly after file cleanup (and in failure paths). [missing cleanup]Severity Level: Major⚠️
Steps of Reproduction ✅
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖