-
Notifications
You must be signed in to change notification settings - Fork 169
Validate video OCR image bytes before Tesseract #882
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 |
|---|---|---|
| @@ -0,0 +1,74 @@ | ||
| import { POST } from "@/app/api/video/generate/route"; | ||
|
|
||
| jest.mock("@clerk/nextjs/server", () => ({ | ||
| currentUser: jest.fn().mockResolvedValue({ | ||
| id: "user_123", | ||
| primaryEmailAddress: { emailAddress: "student@example.com" }, | ||
| }), | ||
| })); | ||
|
|
||
| jest.mock("@/lib/ratelimit/api-rate-limit", () => ({ | ||
| enforceApiRateLimit: jest.fn().mockResolvedValue(null), | ||
| })); | ||
|
|
||
| jest.mock("@/lib/auth/auth-utils", () => ({ | ||
| checkUserBlock: jest.fn().mockResolvedValue({ | ||
| isBlocked: false, | ||
| errorResponse: undefined, | ||
| dbUser: undefined, | ||
| }), | ||
| })); | ||
|
|
||
| jest.mock("@/configs/db", () => ({ | ||
| db: { | ||
| insert: jest.fn(), | ||
| }, | ||
| })); | ||
|
|
||
| jest.mock("@/configs/schema", () => ({ | ||
| videoJobsTable: {}, | ||
| })); | ||
|
|
||
| jest.mock("@/inngest/client", () => ({ | ||
| inngest: { | ||
| send: jest.fn(), | ||
| }, | ||
| })); | ||
|
|
||
| jest.mock("@/lib/ratelimit/ratelimit", () => ({ | ||
| videoLimiter: { | ||
| limit: jest.fn(), | ||
| }, | ||
| redisClient: { | ||
| set: jest.fn(), | ||
| del: jest.fn(), | ||
| }, | ||
| })); | ||
|
|
||
| describe("video generate route", () => { | ||
| it("rejects malformed image content before queueing OCR work", async () => { | ||
| const fetchSpy = jest.spyOn(globalThis, "fetch").mockResolvedValueOnce( | ||
| new Response("not an image", { | ||
| status: 200, | ||
| headers: { "Content-Type": "image/jpeg" }, | ||
| }), | ||
| ); | ||
|
|
||
| const res = await POST( | ||
| new Request("http://localhost/api/video/generate", { | ||
| method: "POST", | ||
| headers: { "Content-Type": "application/json" }, | ||
| body: JSON.stringify({ | ||
| imageUrl: "https://example.com/malformed.jpg", | ||
| }), | ||
| }), | ||
| ); | ||
|
|
||
| expect(fetchSpy).toHaveBeenCalledTimes(1); | ||
| expect(res.status).toBe(422); | ||
| await expect(res.json()).resolves.toEqual({ | ||
| error: "Please upload a valid PNG, JPG, or WEBP image.", | ||
| code: "INVALID_IMAGE_PAYLOAD", | ||
| }); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,45 @@ | ||
| import { | ||
| detectVideoImageMimeType, | ||
| validateVideoImageUrl, | ||
| } from "@/lib/video/image-validation"; | ||
|
|
||
| describe("video image validation", () => { | ||
| it("detects PNG, JPEG, and WEBP magic bytes", () => { | ||
| expect( | ||
| detectVideoImageMimeType( | ||
| new Uint8Array([0x89, 0x50, 0x4e, 0x47, 0x0d, 0x0a, 0x1a, 0x0a]), | ||
| ), | ||
| ).toBe("image/png"); | ||
| expect(detectVideoImageMimeType(new Uint8Array([0xff, 0xd8, 0xff, 0x00]))).toBe( | ||
| "image/jpeg", | ||
| ); | ||
| expect( | ||
| detectVideoImageMimeType( | ||
| new Uint8Array([ | ||
| 0x52, 0x49, 0x46, 0x46, 0x00, 0x00, 0x00, 0x00, 0x57, 0x45, 0x42, 0x50, | ||
| ]), | ||
| ), | ||
| ).toBe("image/webp"); | ||
| }); | ||
|
|
||
| it("rejects non-image payloads before OCR", async () => { | ||
| const fetchSpy = jest.spyOn(globalThis, "fetch").mockResolvedValueOnce( | ||
| new Response("not an image", { | ||
| status: 200, | ||
| headers: { "Content-Type": "image/jpeg" }, | ||
| }), | ||
| ); | ||
|
|
||
| const result = await validateVideoImageUrl("https://example.com/malformed.jpg"); | ||
|
|
||
| expect(fetchSpy).toHaveBeenCalledWith("https://example.com/malformed.jpg", { | ||
| cache: "no-store", | ||
| }); | ||
| expect(result).toEqual({ | ||
| ok: false, | ||
| status: 422, | ||
| code: "INVALID_IMAGE_PAYLOAD", | ||
| error: "Please upload a valid PNG, JPG, or WEBP image.", | ||
| }); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,82 @@ | ||
| const PNG_SIGNATURE = [0x89, 0x50, 0x4e, 0x47, 0x0d, 0x0a, 0x1a, 0x0a]; | ||
| const JPEG_SIGNATURE = [0xff, 0xd8, 0xff]; | ||
| const WEBP_SIGNATURE_PREFIX = [0x52, 0x49, 0x46, 0x46]; | ||
| const WEBP_SIGNATURE_SUFFIX = [0x57, 0x45, 0x42, 0x50]; | ||
|
|
||
| export const VIDEO_IMAGE_ALLOWED_MIME_TYPES = [ | ||
| 'image/png', | ||
| 'image/jpeg', | ||
| 'image/webp', | ||
| ] as const; | ||
|
|
||
| export const VIDEO_IMAGE_ALLOWED_TYPES_LABEL = 'PNG, JPG, or WEBP'; | ||
|
|
||
| export type VideoImageValidationResult = | ||
| | { ok: true; mimeType: string } | ||
| | { | ||
| ok: false; | ||
| status: 422 | 500; | ||
| code: string; | ||
| error: string; | ||
| }; | ||
|
|
||
| export function isAllowedVideoImageMimeType(mimeType: string) { | ||
| return (VIDEO_IMAGE_ALLOWED_MIME_TYPES as readonly string[]).includes( | ||
| mimeType.toLowerCase(), | ||
| ); | ||
| } | ||
|
|
||
| export function detectVideoImageMimeType(bytes: Uint8Array): string | null { | ||
| if (bytes.length >= PNG_SIGNATURE.length && PNG_SIGNATURE.every((byte, index) => bytes[index] === byte)) { | ||
| return 'image/png'; | ||
| } | ||
|
|
||
| if (bytes.length >= JPEG_SIGNATURE.length && JPEG_SIGNATURE.every((byte, index) => bytes[index] === byte)) { | ||
| return 'image/jpeg'; | ||
| } | ||
|
Comment on lines
+34
to
+36
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 JPEG detector accepts any 3-byte prefix Severity Level: Major
|
||
|
|
||
| if ( | ||
| bytes.length >= 12 && | ||
| WEBP_SIGNATURE_PREFIX.every((byte, index) => bytes[index] === byte) && | ||
| WEBP_SIGNATURE_SUFFIX.every((byte, index) => bytes[index + 8] === byte) | ||
| ) { | ||
| return 'image/webp'; | ||
| } | ||
|
|
||
| return null; | ||
| } | ||
|
|
||
| export async function validateVideoImageUrl(imageUrl: string): Promise<VideoImageValidationResult> { | ||
| try { | ||
| const response = await fetch(imageUrl, { cache: 'no-store' }); | ||
|
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 server fetches a user-controlled URL directly, which enables SSRF against internal network targets (for example cloud metadata or private services). Restrict allowed hosts/IP ranges (block localhost, link-local, RFC1918, etc.) before fetching, and reject non-public destinations. [ssrf] Severity Level: Critical 🚨- ❌ Video generation endpoint can query internal metadata services.
- ❌ Attackers can reach private network services via imageUrl.
- ⚠️ SSRF may bypass perimeter network protections entirely.Steps of Reproduction ✅1. An attacker sends a POST request to `/api/video/generate` (handled by `POST` in
`src/app/api/video/generate/route.ts:23`) with a JSON body where `imageUrl` is a URL to an
internal resource, such as `http://169.254.169.254/latest/meta-data/...`, and optionally
omits `content`.
2. The body is parsed by `parseAndValidateRequest` against `generateVideoSchema` in
`src/lib/validations/video.ts:4-9`, where `imageUrl` is validated only as a `safeUrl`
(`trimmedString.url().max(2048)` in `src/lib/validations/common.ts:3-4`) with no host or
IP-range restrictions, so internal network endpoints are accepted.
3. Because `data.imageUrl` is present, the route calls
`validateVideoImageUrl(data.imageUrl)` at `src/app/api/video/generate/route.ts:46-47`,
which in turn executes `fetch(imageUrl, { cache: 'no-store' })` at
`src/lib/video/image-validation.ts:51` directly against the attacker-controlled URL from
the server environment.
4. On infrastructure where the backend has network access to cloud metadata services or
other internal systems, this design allows the attacker to cause server-side requests to
those internal endpoints (SSRF) and observe success or failure via the validation response
from `validateVideoImageUrl`, potentially exfiltrating sensitive information or
interacting with private services.(Use Cmd/Ctrl + Click for best experience) Prompt for AI Agent 🤖This is a comment left during a code review.
**Path:** src/lib/video/image-validation.ts
**Line:** 51:51
**Comment:**
*Ssrf: The server fetches a user-controlled URL directly, which enables SSRF against internal network targets (for example cloud metadata or private services). Restrict allowed hosts/IP ranges (block localhost, link-local, RFC1918, etc.) before fetching, and reject non-public destinations.
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 |
||
| if (!response.ok) { | ||
| return { | ||
| ok: false, | ||
| status: 422, | ||
| code: 'INVALID_IMAGE_PAYLOAD', | ||
| error: `Please upload a valid ${VIDEO_IMAGE_ALLOWED_TYPES_LABEL} image.`, | ||
| }; | ||
| } | ||
|
|
||
| const bytes = new Uint8Array(await response.arrayBuffer()); | ||
|
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: This reads the entire remote response into memory with no size cap, so a very large payload can exhaust memory and crash or degrade the server. Enforce a strict maximum byte limit (via Content-Length check plus streamed read cap) and fail once the cap is exceeded. [performance] Severity Level: Major
|
||
| const mimeType = detectVideoImageMimeType(bytes); | ||
|
|
||
| if (!mimeType || !isAllowedVideoImageMimeType(mimeType)) { | ||
| return { | ||
| ok: false, | ||
| status: 422, | ||
| code: 'INVALID_IMAGE_PAYLOAD', | ||
| error: `Please upload a valid ${VIDEO_IMAGE_ALLOWED_TYPES_LABEL} image.`, | ||
| }; | ||
| } | ||
|
|
||
| return { ok: true, mimeType }; | ||
| } catch { | ||
| return { | ||
| ok: false, | ||
| status: 422, | ||
| code: 'INVALID_IMAGE_PAYLOAD', | ||
| error: `Please upload a valid ${VIDEO_IMAGE_ALLOWED_TYPES_LABEL} image.`, | ||
| }; | ||
| } | ||
| } | ||
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: The new validation runs whenever an image URL is present, even when text content is already provided. In this flow OCR would not use the image, so valid text submissions can now fail incorrectly due to an unrelated image URL; gate validation to only run when OCR is actually needed. [incorrect condition logic]
Severity Level: Major⚠️
Steps of Reproduction ✅
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖