fix: add file type validation to Tesseract OCR processing#778
Conversation
|
@anshul23102 is attempting to deploy a commit to the Karan Mani Tripathi 's projects Team on Vercel. A member of the Team first needs to authorize it. |
|
CodeAnt AI is reviewing your PR. |
Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
github-actions
Bot
added
gssoc'26
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThe video pipeline now validates remote OCR image URLs with a timed HEAD request and an allowed MIME-type list before invoking Tesseract. ChangesVideo OCR validation
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
|
|
||
| async function validateImageUrl(url: string): Promise<void> { | ||
| try { | ||
| const response = await axios.head(url, { timeout: 5000 }); |
There was a problem hiding this comment.
Suggestion: The new URL validation performs a server-side request to a user-controlled URL without any private-network or host allowlist checks, which enables SSRF against internal services. Validate and reject localhost/private IP ranges (and optionally enforce an allowlist) before making the outbound request. [ssrf]
Severity Level: Critical 🚨
- ❌ Video pipeline can issue SSRF requests to internal hosts.
- ❌ POST /api/video/generate accepts attacker-controlled imageUrl targets.
- ⚠️ Internal metadata or admin APIs potentially reachable via HEAD.
- ⚠️ Background Inngest worker can be abused for network scanning.Steps of Reproduction ✅
1. Issue an authenticated POST request to `/api/video/generate` (handler at
`src/app/api/video/generate/route.ts:22-105`) with a JSON body that passes
`generateVideoSchema` (defined in `src/lib/validations/video.ts:4-10`), setting `content`
to `null` and `imageUrl` to a URL pointing at an internal or otherwise sensitive endpoint,
e.g. `http://169.254.169.254/latest/meta-data/` or an RFC1918 IP address.
2. The route handler parses and validates the request via `parseAndValidateRequest(req,
generateVideoSchema)` at `route.ts:42`, then enqueues an Inngest event
`video/generate.requested` including the user-supplied `imageUrl` in the payload at
`route.ts:81-90`.
3. The Inngest function `generateVideo` at `src/inngest/functions.ts:72-80` receives the
event, extracts `imageUrl` from `event.data` (lines 79-80), and then calls
`runVideoPipeline({ content, imageUrl, baseUrl }, ...)` at `functions.ts:11-22`.
4. Inside `runVideoPipeline` in `src/lib/video/pipeline.ts:114-129`, when `content` is
undefined and `imageUrl` is present, the code enters the OCR branch at
`pipeline.ts:121-129`, calls `await validateImageUrl(imageUrl)` at `pipeline.ts:124`, and
`validateImageUrl` performs `axios.head(url, { timeout: 5000 })` at `pipeline.ts:43`,
causing the backend to issue a HEAD request directly to the attacker-controlled `imageUrl`
host, thereby enabling SSRF against internal 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/pipeline.ts
**Line:** 43:43
**Comment:**
*Ssrf: The new URL validation performs a server-side request to a user-controlled URL without any private-network or host allowlist checks, which enables SSRF against internal services. Validate and reject localhost/private IP ranges (and optionally enforce an allowlist) before making the outbound request.
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 (!ALLOWED_IMAGE_TYPES.some(type => contentType.includes(type))) { | ||
| throw new Error(`Invalid file type: ${contentType || "unknown"}. Only image files are allowed.`); | ||
| } |
There was a problem hiding this comment.
Suggestion: The MIME-type check uses substring matching, so crafted or malformed content-type values that merely contain an allowed token can bypass validation. Parse the MIME type up to ; and compare exact normalized values instead of using includes. [incorrect condition logic]
Severity Level: Major ⚠️
- ❌ Non-image payloads can bypass image-only validation logic.
- ⚠️ Tesseract OCR may process malformed or malicious files.
- ⚠️ Defensive filter weaker than intended due to substring matching.
- ⚠️ Potential crashes or undefined behavior in OCR stage.Steps of Reproduction ✅
1. An attacker controls the HTTP server referenced by `imageUrl` and hosts a non-image
payload (for example, a text or binary file) while configuring the `Content-Type` header
to a non-standard value that embeds an allowed token, such as `application/x-image/jpeg`
or `foo/image/png`. This is fully under attacker control because `imageUrl` is only
constrained by `safeUrl` (standard URL format) in `src/lib/validations/common.ts:3-4` and
`generateVideoSchema` at `src/lib/validations/video.ts:4-7`.
2. The attacker sends a POST request to `/api/video/generate` (handler at
`src/app/api/video/generate/route.ts:22-105`), setting `content` to `null` and `imageUrl`
to the crafted URL. The request passes `generateVideoSchema` at `route.ts:42` and is
enqueued as `video/generate.requested` with the unmodified `imageUrl` at `route.ts:81-90`.
3. The Inngest `generateVideo` function in `src/inngest/functions.ts:72-80` receives the
event, extracts `imageUrl`, and calls `runVideoPipeline` at `functions.ts:11-22`. Inside
`runVideoPipeline` in `src/lib/video/pipeline.ts:121-129`, the OCR branch executes and
calls `await validateImageUrl(imageUrl)` at line 124.
4. `validateImageUrl` performs `axios.head(url, { timeout: 5000 })` at `pipeline.ts:43`,
reads the crafted `Content-Type` header into `contentType` at `pipeline.ts:44`, and
evaluates `ALLOWED_IMAGE_TYPES.some(type => contentType.includes(type))` at
`pipeline.ts:46`. Because the header string contains the substring `image/jpeg` or
`image/png`, the check passes even though the MIME type is non-standard for an image. No
error is thrown, and `runVideoPipeline` proceeds to call `Tesseract.recognize(imageUrl,
"eng")` at `pipeline.ts:125-127` on a non-image payload, defeating the intended MIME-type
validation.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/lib/video/pipeline.ts
**Line:** 46:48
**Comment:**
*Incorrect Condition Logic: The MIME-type check uses substring matching, so crafted or malformed `content-type` values that merely contain an allowed token can bypass validation. Parse the MIME type up to `;` and compare exact normalized values instead of using `includes`.
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
|
CodeAnt AI finished reviewing your PR. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/lib/video/pipeline.ts (1)
49-54: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueError-type discrimination via string prefix matching is fragile.
Distinguishing the "invalid type" case from other failures by checking
error.message.startsWith("Invalid file type")breaks silently if the message wording ever changes. A custom error class (or aninstanceofcheck) would be more robust and clearer for callers.♻️ Suggested refactor
+class InvalidImageTypeError extends Error {} + async function validateImageUrl(url: string): Promise<void> { try { const response = await axios.head(url, { timeout: 5000 }); const contentType = String(response.headers["content-type"] || "").toLowerCase(); if (!ALLOWED_IMAGE_TYPES.some(type => contentType.includes(type))) { - throw new Error(`Invalid file type: ${contentType || "unknown"}. Only image files are allowed.`); + throw new InvalidImageTypeError(`Invalid file type: ${contentType || "unknown"}. Only image files are allowed.`); } } catch (error) { - if (error instanceof Error && error.message.startsWith("Invalid file type")) { + if (error instanceof InvalidImageTypeError) { throw error; } throw new Error("Failed to validate image file: unable to fetch file headers"); } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/video/pipeline.ts` around lines 49 - 54, The error handling in validate image file logic is relying on brittle message-prefix matching to detect the invalid file type case. Update the catch path in src/lib/video/pipeline.ts around the validation flow to use a dedicated error type or another explicit discriminator from the validation helper instead of checking error.message.startsWith("Invalid file type"). Make the thrown invalid-type error identifiable via instanceof or a custom class, and keep the fallback "Failed to validate image file: unable to fetch file headers" for all other failures.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lib/video/pipeline.ts`:
- Line 46: The content-type validation in the pipeline is too loose because
`contentType.includes(type)` can accept unintended matches; update the check in
`pipeline` to use a stricter exact or prefix comparison after normalizing the
header by removing any charset or parameter suffixes. Use the existing
`ALLOWED_IMAGE_TYPES` logic in `src/lib/video/pipeline.ts` and adjust the
validation so only true image MIME types are accepted, not substrings embedded
in other content types.
- Around line 39-56: The current validateImageUrl flow in pipeline.ts only
trusts the HEAD content-type, but OCR later re-fetches the URL, so the same
input must be validated from the actual bytes. Update validateImageUrl and the
Tesseract.recognize call site to fetch the image once, verify its magic bytes
from the downloaded buffer, and reuse that buffer for OCR instead of passing the
URL. Keep the image-type validation logic anchored around validateImageUrl and
the OCR invocation in pipeline.ts so the check cannot be bypassed by mismatched
HEAD vs body content.
---
Nitpick comments:
In `@src/lib/video/pipeline.ts`:
- Around line 49-54: The error handling in validate image file logic is relying
on brittle message-prefix matching to detect the invalid file type case. Update
the catch path in src/lib/video/pipeline.ts around the validation flow to use a
dedicated error type or another explicit discriminator from the validation
helper instead of checking error.message.startsWith("Invalid file type"). Make
the thrown invalid-type error identifiable via instanceof or a custom class, and
keep the fallback "Failed to validate image file: unable to fetch file headers"
for all other failures.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 959d1198-d016-4925-bfa5-e09a366c5416
📒 Files selected for processing (1)
src/lib/video/pipeline.ts
| const ALLOWED_IMAGE_TYPES = ["image/jpeg", "image/png", "image/gif", "image/webp", "image/bmp", "image/svg+xml", "image/tiff"]; | ||
|
|
||
| async function validateImageUrl(url: string): Promise<void> { | ||
| try { | ||
| const response = await axios.head(url, { timeout: 5000 }); | ||
| const contentType = String(response.headers["content-type"] || "").toLowerCase(); | ||
|
|
||
| if (!ALLOWED_IMAGE_TYPES.some(type => contentType.includes(type))) { | ||
| throw new Error(`Invalid file type: ${contentType || "unknown"}. Only image files are allowed.`); | ||
| } | ||
| } catch (error) { | ||
| if (error instanceof Error && error.message.startsWith("Invalid file type")) { | ||
| throw error; | ||
| } | ||
| throw new Error("Failed to validate image file: unable to fetch file headers"); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== file list ==\n'
git ls-files src/lib/video/pipeline.ts
printf '\n== outline ==\n'
ast-grep outline src/lib/video/pipeline.ts --view expanded || true
printf '\n== relevant slices ==\n'
sed -n '1,220p' src/lib/video/pipeline.ts | cat -nRepository: knoxiboy/DoubtDesk
Length of output: 10370
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== search for validateImageUrl and Tesseract.recognize usages ==\n'
rg -n "validateImageUrl|Tesseract\.recognize|content-type|ALLOWED_IMAGE_TYPES|axios\.head|axios\.get" src lib . -g '!node_modules' || trueRepository: knoxiboy/DoubtDesk
Length of output: 2631
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== surrounding lines near reported locations ==\n'
sed -n '30,150p' src/lib/video/pipeline.ts | cat -n
printf '\n== tail slice ==\n'
sed -n '150,320p' src/lib/video/pipeline.ts | cat -nRepository: knoxiboy/DoubtDesk
Length of output: 11358
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== git diff stat for pipeline file ==\n'
git diff --stat -- src/lib/video/pipeline.ts || true
printf '\n== file size ==\n'
wc -l src/lib/video/pipeline.tsRepository: knoxiboy/DoubtDesk
Length of output: 242
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== exact call flow from pipeline.ts ==\n'
python3 - <<'PY'
from pathlib import Path
p=Path('src/lib/video/pipeline.ts')
text=p.read_text()
for needle in ['validateImageUrl(', 'Tesseract.recognize(', 'axios.head(', 'axios.get(']:
print(f"\n--- {needle} ---")
for i,line in enumerate(text.splitlines(),1):
if needle in line:
start=max(1,i-8); end=min(len(text.splitlines()), i+12)
for j in range(start,end+1):
print(f"{j:4d}: {text.splitlines()[j-1]}")
break
PYRepository: knoxiboy/DoubtDesk
Length of output: 3133
Validate the OCR input bytes, not the HEAD header src/lib/video/pipeline.ts:41-56,124-127
validateImageUrl only checks a HEAD content-type, then Tesseract.recognize(imageUrl, "eng") fetches the URL again. A server can return image/* for HEAD and different bytes for the actual image, so this check can be bypassed. Read the file once, inspect its magic bytes, and pass that same buffer into OCR.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/video/pipeline.ts` around lines 39 - 56, The current validateImageUrl
flow in pipeline.ts only trusts the HEAD content-type, but OCR later re-fetches
the URL, so the same input must be validated from the actual bytes. Update
validateImageUrl and the Tesseract.recognize call site to fetch the image once,
verify its magic bytes from the downloaded buffer, and reuse that buffer for OCR
instead of passing the URL. Keep the image-type validation logic anchored around
validateImageUrl and the OCR invocation in pipeline.ts so the check cannot be
bypassed by mismatched HEAD vs body content.
| const response = await axios.head(url, { timeout: 5000 }); | ||
| const contentType = String(response.headers["content-type"] || "").toLowerCase(); | ||
|
|
||
| if (!ALLOWED_IMAGE_TYPES.some(type => contentType.includes(type))) { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Substring match on content-type is weak.
contentType.includes(type) can match unintended values (e.g., a multipart/vendor content-type embedding one of the allowed strings as a substring). Prefer exact/prefix comparison after stripping charset parameters.
♻️ Suggested tightening
- const contentType = String(response.headers["content-type"] || "").toLowerCase();
-
- if (!ALLOWED_IMAGE_TYPES.some(type => contentType.includes(type))) {
+ const contentType = String(response.headers["content-type"] || "")
+ .toLowerCase()
+ .split(";")[0]
+ .trim();
+
+ if (!ALLOWED_IMAGE_TYPES.includes(contentType)) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (!ALLOWED_IMAGE_TYPES.some(type => contentType.includes(type))) { | |
| const contentType = String(response.headers["content-type"] || "") | |
| .toLowerCase() | |
| .split(";")[0] | |
| .trim(); | |
| if (!ALLOWED_IMAGE_TYPES.includes(contentType)) { |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/video/pipeline.ts` at line 46, The content-type validation in the
pipeline is too loose because `contentType.includes(type)` can accept unintended
matches; update the check in `pipeline` to use a stricter exact or prefix
comparison after normalizing the header by removing any charset or parameter
suffixes. Use the existing `ALLOWED_IMAGE_TYPES` logic in
`src/lib/video/pipeline.ts` and adjust the validation so only true image MIME
types are accepted, not substrings embedded in other content types.
d713100 to
cdf4400
Compare
cdf4400 to
54c139c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/configs/schema.ts (1)
42-62: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConsider a dedicated index on
organizationIdinorganizationMembershipsTable.The only index touching
organizationIdhere is the composite unique constraint(userEmail, organizationId), withuserEmailas the leading column. Queries that filter byorganizationIdalone (e.g., listing all members of an organization for an admin view) won't benefit from this composite index efficiently. Drizzle's own guidance recommends indexing FK columns explicitly for one-to-many lookups from the "one" side.♻️ Proposed addition
(table) => ({ organizationIdFk: foreignKey({ columns: [table.organizationId], foreignColumns: [organizationsTable.id], }).onDelete("cascade"), + organizationIdIndex: index("org_memberships_orgId_idx").on(table.organizationId), userEmailFk: foreignKey({ columns: [table.userEmail], foreignColumns: [usersTable.email], }).onDelete("cascade"), orgMembershipUnique: unique("org_memberships_userEmail_orgId_unique").on(table.userEmail, table.organizationId), }),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/configs/schema.ts` around lines 42 - 62, Add an explicit index for organizationId in organizationMembershipsTable because the existing unique constraint orgMembershipUnique starts with userEmail and won’t efficiently support organization-only lookups. Update the schema in src/configs/schema.ts by extending the pgTable config for organizationMembershipsTable with a dedicated index on table.organizationId, keeping the existing foreign keys and unique constraint unchanged.src/app/api/analytics/route.ts (1)
58-58: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAvoid widening callback params to
any; the inferred Drizzle row types are already available here.
orgClassrooms,userMemberships,mostAskedTopics, andtopContributorsare all typed results ofdb.select({...}), so Drizzle already infers the row shape (e.g.,{ id: number }[],{ classroomId: number }[]). Casting the callback params toany(lines 58, 70, 202, 267-268) throws away that inference instead of using it, and makes it easy to silently diverge from theAnalyticsData/WeakTopic/TopContributorcontract consumed by the dashboard. Also,indexon line 202 is unused and doesn't need a type at all.♻️ Proposed fix
- activeClassroomIds = orgClassrooms.map((c: any) => c.id); + activeClassroomIds = orgClassrooms.map((c) => c.id);- activeClassroomIds = userMemberships.map((m: any) => m.classroomId); + activeClassroomIds = userMemberships.map((m) => m.classroomId);- const weakTopics = mostAskedTopics.map((topic: any, index: any) => { + const weakTopics = mostAskedTopics.map((topic) => {- weakTopics: weakTopics.filter((t: any) => t.severity !== 'Low'), - topContributors: topContributors.map((c: any) => ({ name: c.name, replyCount: Number(c.replyCount) })), + weakTopics: weakTopics.filter((t) => t.severity !== 'Low'), + topContributors: topContributors.map((c) => ({ name: c.name, replyCount: Number(c.replyCount) })),Also applies to: 70-70, 202-202, 267-268
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/api/analytics/route.ts` at line 58, Remove the unnecessary any annotations from the callback parameters in the analytics route so Drizzle’s inferred row types are preserved. Update the map/filter callbacks on orgClassrooms, userMemberships, mostAskedTopics, and topContributors to rely on the inferred shapes from db.select({...}), and drop the unused index parameter where it appears in the topic mapping. Keep the existing logic intact while using the typed row fields directly in route.ts.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/__tests__/configs/db.test.ts`:
- Around line 70-74: The test cleanup in the db config suite is restoring
NODE_ENV incorrectly when there was no original value, leaving the key present
instead of removing it. Update the restoration logic in the test teardown around
process.env.NODE_ENV so that when originalNodeEnv is absent you delete the
property rather than redefining it, while keeping the existing restore path for
the original value intact.
---
Nitpick comments:
In `@src/app/api/analytics/route.ts`:
- Line 58: Remove the unnecessary any annotations from the callback parameters
in the analytics route so Drizzle’s inferred row types are preserved. Update the
map/filter callbacks on orgClassrooms, userMemberships, mostAskedTopics, and
topContributors to rely on the inferred shapes from db.select({...}), and drop
the unused index parameter where it appears in the topic mapping. Keep the
existing logic intact while using the typed row fields directly in route.ts.
In `@src/configs/schema.ts`:
- Around line 42-62: Add an explicit index for organizationId in
organizationMembershipsTable because the existing unique constraint
orgMembershipUnique starts with userEmail and won’t efficiently support
organization-only lookups. Update the schema in src/configs/schema.ts by
extending the pgTable config for organizationMembershipsTable with a dedicated
index on table.organizationId, keeping the existing foreign keys and unique
constraint unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e4795c54-8325-49d3-ab2f-1d07ff60ef9b
📒 Files selected for processing (36)
drizzle/0017_audit_logs.sqldrizzle/0018_silky_gateway.sqldrizzle/meta/_journal.jsonsrc/__tests__/configs/db.test.tssrc/app/api/admin/moderation/route.tssrc/app/api/admin/overview/route.tssrc/app/api/analytics/export/route.tssrc/app/api/analytics/personal/route.tssrc/app/api/analytics/route.tssrc/app/api/bookmarks/route.tssrc/app/api/classrooms/[id]/export/route.tssrc/app/api/doubts/[id]/upvote/route.tssrc/app/api/doubts/action/[id]/route.tssrc/app/api/doubts/check-similarity/route.tssrc/app/api/doubts/route.tssrc/app/api/karma/route.tssrc/app/api/organizations/route.tssrc/app/api/profile/route.tssrc/app/api/recommendations/route.tssrc/app/api/replies/vote/route.tssrc/app/api/resume-analyzer/history/route.tssrc/app/api/roadmap/history/route.tssrc/app/api/rooms/join/route.tssrc/app/api/rooms/members/route.tssrc/app/api/rooms/route.tssrc/app/api/teacher/analytics/route.tssrc/app/api/teacher/insights/route.tssrc/configs/db.tsxsrc/configs/schema.tssrc/inngest/functions.tssrc/inngest/karma.tssrc/lib/ai/embeddings.tssrc/lib/karma-utils.tssrc/lib/moderation.tssrc/lib/notifications/service.tssrc/lib/video/pipeline.ts
✅ Files skipped from review due to trivial changes (26)
- src/app/api/roadmap/history/route.ts
- src/app/api/rooms/members/route.ts
- src/app/api/recommendations/route.ts
- src/app/api/profile/route.ts
- src/app/api/organizations/route.ts
- src/app/api/rooms/join/route.ts
- src/app/api/doubts/[id]/upvote/route.ts
- src/app/api/resume-analyzer/history/route.ts
- src/app/api/doubts/check-similarity/route.ts
- src/app/api/analytics/personal/route.ts
- src/app/api/admin/moderation/route.ts
- src/app/api/teacher/insights/route.ts
- src/inngest/karma.ts
- src/app/api/replies/vote/route.ts
- src/lib/notifications/service.ts
- src/lib/karma-utils.ts
- src/app/api/rooms/route.ts
- src/app/api/teacher/analytics/route.ts
- src/lib/moderation.ts
- src/inngest/functions.ts
- src/app/api/bookmarks/route.ts
- src/lib/ai/embeddings.ts
- src/app/api/admin/overview/route.ts
- src/app/api/doubts/action/[id]/route.ts
- src/app/api/doubts/route.ts
- src/app/api/analytics/export/route.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lib/video/pipeline.ts
|
CodeAnt AI is running Incremental review |
Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
|
CodeAnt AI Incremental review completed. |
github-actions
Bot
added
the
merge-conflict
Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
|
Merge conflicts resolved. Branch synced with main and ready for CI checks. |
github-actions
Bot
removed
the
merge-conflict
|
CodeAnt AI Incremental review completed. |
github-actions
Bot
added
the
merge-conflict
|
CodeAnt AI is running Incremental review |
Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
|
CodeAnt AI Incremental review completed. |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
.github/workflows/pr-labeler.yml (1)
711-723: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueSecurity posture of the new
pull_request_targetjob is sound; consider SHA-pinning the action.Good: this job performs no
checkoutof PR head code and keepspermissionsminimal (issues/pull-requests: write), so running underpull_request_targetdoes not expose secrets to untrusted PR code.Per path instructions (pinned action versions),
actions/github-script@v7is a moving major-version tag rather than a pinned commit SHA. This matches the rest of the file, so it's optional, but consider pinning to a full SHA for supply-chain hardening.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/pr-labeler.yml around lines 711 - 723, Pin the actions/github-script action used by the pr-closed-cleanup job to a full immutable commit SHA instead of the moving v7 tag, while preserving the existing action version and job behavior.Source: Path instructions
src/app/api/unsubscribe/route.ts (1)
228-241: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winPrefer Node.js built-in
crypto.timingSafeEqualover the manual implementation.The caller at Line 169 already guards
formNonce.length !== cookieNonce.length, so by the timetimingSafeEqualis invoked both strings are guaranteed equal-length. The built-incrypto.timingSafeEqual(which throws on length mismatch) is safe to use here and avoids maintaining custom constant-time comparison code.♻️ Proposed refactor using built-in crypto.timingSafeEqual
import { randomBytes } from "crypto"; +import { timingSafeEqual as cryptoTimingSafeEqual } from "crypto"; // ... existing code ... -/** - * Constant-time string comparison to prevent timing attacks on the nonce. - * Both strings must be the same length (checked before calling). - */ -function timingSafeEqual(a: string, b: string): boolean { - const bufA = Buffer.from(a, "utf8"); - const bufB = Buffer.from(b, "utf8"); - if (bufA.length !== bufB.length) return false; - let diff = 0; - for (let i = 0; i < bufA.length; i++) { - diff |= bufA[i] ^ bufB[i]; - } - return diff === 0; -} +/** Constant-time string comparison to prevent timing attacks on the nonce. */ +function timingSafeEqual(a: string, b: string): boolean { + const bufA = Buffer.from(a, "utf8"); + const bufB = Buffer.from(b, "utf8"); + if (bufA.length !== bufB.length) return false; + return cryptoTimingSafeEqual(bufA, bufB); +}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/api/unsubscribe/route.ts` around lines 228 - 241, Replace the manual implementation in timingSafeEqual with Node.js crypto.timingSafeEqual, converting both strings to UTF-8 buffers and returning its comparison result. Preserve the existing equal-length guard at the caller before timingSafeEqual is invoked, since the built-in throws for mismatched buffer lengths.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/app/api/rooms/join/route.ts`:
- Line 15: Update the IP extraction in the join route to split the
x-forwarded-for header on commas, trim the entries, and use the first IP before
falling back to x-real-ip or unknown. Keep the existing fallback order and
ensure the resulting rate-limit key matches the unsubscribe route’s getClientIp
behavior.
In `@src/inngest/functions.ts`:
- Around line 47-65: Guard each iteration of the cleanup loop in the
stale-artifact cleanup function with a per-entry try/catch, including stat and
removal operations. On filesystem errors such as EACCES, EPERM, or ENOENT, skip
that entry and continue scanning the remaining entries without aborting the
cleanup run.
In `@src/lib/video/pipeline.ts`:
- Line 264: Remove the leftover merge-conflict marker from the video pipeline
and search the entire branch for any remaining <<<<<<<, =======, or >>>>>>>
markers. Resolve any discovered conflicts by retaining the intended
implementation, leaving the code syntactically valid and buildable.
- Line 42: Restrict the URL handling around the axios.head call and
Tesseract.recognize to validated public HTTPS URLs only: enforce the https
scheme, resolve and reject private, loopback, link-local, and other reserved IP
destinations, and prevent redirects from bypassing validation. Apply the same
validated URL to both server-side fetches and reject invalid or unsafe imageUrl
values before either request.
---
Nitpick comments:
In @.github/workflows/pr-labeler.yml:
- Around line 711-723: Pin the actions/github-script action used by the
pr-closed-cleanup job to a full immutable commit SHA instead of the moving v7
tag, while preserving the existing action version and job behavior.
In `@src/app/api/unsubscribe/route.ts`:
- Around line 228-241: Replace the manual implementation in timingSafeEqual with
Node.js crypto.timingSafeEqual, converting both strings to UTF-8 buffers and
returning its comparison result. Preserve the existing equal-length guard at the
caller before timingSafeEqual is invoked, since the built-in throws for
mismatched buffer lengths.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d1cb13fd-1cf6-4ac6-ad43-dac72bdd8772
📒 Files selected for processing (22)
.github/workflows/pr-labeler.ymljest.setup.tssrc/__tests__/api/doubts-scoped-likes.test.tssrc/__tests__/api/replies.test.tssrc/__tests__/api/unsubscribe.test.tssrc/__tests__/inngest/digest-functions.test.tssrc/__tests__/middleware/onboarding-cache.test.tssrc/__tests__/video/pipeline-cleanup.test.tssrc/app/api/doubts/route.tssrc/app/api/invites/[token]/join/route.tssrc/app/api/replies/route.tssrc/app/api/rooms/join/route.tssrc/app/api/unsubscribe/route.tssrc/app/api/video/generate/route.tssrc/app/api/video/status/route.tssrc/inngest/functions.tssrc/lib/moderation/moderation.tssrc/lib/ratelimit/ratelimit.tssrc/lib/utils/utils.tssrc/lib/video/pipeline.tssrc/lib/video/storage.tssrc/middleware.tsx
💤 Files with no reviewable changes (1)
- src/app/api/video/generate/route.ts
✅ Files skipped from review due to trivial changes (2)
- src/tests/middleware/onboarding-cache.test.ts
- src/app/api/invites/[token]/join/route.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/tests/inngest/digest-functions.test.ts
| export async function POST(req: NextRequest) { | ||
| try { | ||
| // 0. Rate limit by IP (prevent brute-force enumeration of invite codes) | ||
| const ip = req.headers.get('x-forwarded-for') || req.headers.get('x-real-ip') || 'unknown'; |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Extract the first IP from x-forwarded-for for consistent rate-limit bucketing.
The join route uses the raw x-forwarded-for header value as the rate-limit key, but when a proxy chain appends IPs (e.g. "1.2.3.4, 10.0.0.1"), the entire comma-separated string becomes the key. This is inconsistent with the unsubscribe route's getClientIp which splits and trims to the first IP. Different proxy edge nodes producing different append patterns would create distinct keys for the same client, weakening rate-limit effectiveness.
🛡️ Proposed fix
- const ip = req.headers.get('x-forwarded-for') || req.headers.get('x-real-ip') || 'unknown';
+ const ip = req.headers.get('x-forwarded-for')?.split(',')[0]?.trim()
+ || req.headers.get('x-real-ip')
+ || 'unknown';📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const ip = req.headers.get('x-forwarded-for') || req.headers.get('x-real-ip') || 'unknown'; | |
| const ip = req.headers.get('x-forwarded-for')?.split(',')[0]?.trim() | |
| || req.headers.get('x-real-ip') | |
| || 'unknown'; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/app/api/rooms/join/route.ts` at line 15, Update the IP extraction in the
join route to split the x-forwarded-for header on commas, trim the entries, and
use the first IP before falling back to x-real-ip or unknown. Keep the existing
fallback order and ensure the resulting rate-limit key matches the unsubscribe
route’s getClientIp behavior.
| if (fs.existsSync(tmpRoot)) { | ||
| const entries = fs.readdirSync(tmpRoot); | ||
| for (const entry of entries) { | ||
| const entryPath = path.join(tmpRoot, entry); | ||
| const stats = fs.statSync(entryPath); | ||
| const isStale = now - stats.mtimeMs > retentionMs; | ||
|
|
||
| if (entry.startsWith("doubtdesk-audio-") && stats.isDirectory() && isStale) { | ||
| fs.rmSync(entryPath, { recursive: true, force: true }); | ||
| count++; | ||
| continue; | ||
| } | ||
|
|
||
| if (/^video-.*\.mp4$/i.test(entry) && stats.isFile() && isStale) { | ||
| fs.unlinkSync(entryPath); | ||
| count++; | ||
| } | ||
| } | ||
| }; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Per-entry filesystem errors abort the whole cleanup run.
The loop scans every entry in the shared os.tmpdir(), including files/dirs owned by other processes. An unguarded fs.statSync(entryPath) (Line 51) can throw EACCES/EPERM on entries you can't stat, or ENOENT if an entry disappears mid-scan (TOCTOU). Any single throw propagates out and fails the entire cron, leaving remaining stale artifacts uncleaned. Wrap the per-entry work in try/catch and continue.
🛡️ Proposed guard
for (const entry of entries) {
const entryPath = path.join(tmpRoot, entry);
- const stats = fs.statSync(entryPath);
- const isStale = now - stats.mtimeMs > retentionMs;
-
- if (entry.startsWith("doubtdesk-audio-") && stats.isDirectory() && isStale) {
- fs.rmSync(entryPath, { recursive: true, force: true });
- count++;
- continue;
- }
-
- if (/^video-.*\.mp4$/i.test(entry) && stats.isFile() && isStale) {
- fs.unlinkSync(entryPath);
- count++;
- }
+ try {
+ const stats = fs.statSync(entryPath);
+ const isStale = now - stats.mtimeMs > retentionMs;
+
+ if (entry.startsWith("doubtdesk-audio-") && stats.isDirectory() && isStale) {
+ fs.rmSync(entryPath, { recursive: true, force: true });
+ count++;
+ continue;
+ }
+
+ if (/^video-.*\.mp4$/i.test(entry) && stats.isFile() && isStale) {
+ fs.unlinkSync(entryPath);
+ count++;
+ }
+ } catch {
+ // Skip entries we can't stat/remove (permissions, races).
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (fs.existsSync(tmpRoot)) { | |
| const entries = fs.readdirSync(tmpRoot); | |
| for (const entry of entries) { | |
| const entryPath = path.join(tmpRoot, entry); | |
| const stats = fs.statSync(entryPath); | |
| const isStale = now - stats.mtimeMs > retentionMs; | |
| if (entry.startsWith("doubtdesk-audio-") && stats.isDirectory() && isStale) { | |
| fs.rmSync(entryPath, { recursive: true, force: true }); | |
| count++; | |
| continue; | |
| } | |
| if (/^video-.*\.mp4$/i.test(entry) && stats.isFile() && isStale) { | |
| fs.unlinkSync(entryPath); | |
| count++; | |
| } | |
| } | |
| }; | |
| } | |
| if (fs.existsSync(tmpRoot)) { | |
| const entries = fs.readdirSync(tmpRoot); | |
| for (const entry of entries) { | |
| const entryPath = path.join(tmpRoot, entry); | |
| try { | |
| const stats = fs.statSync(entryPath); | |
| const isStale = now - stats.mtimeMs > retentionMs; | |
| if (entry.startsWith("doubtdesk-audio-") && stats.isDirectory() && isStale) { | |
| fs.rmSync(entryPath, { recursive: true, force: true }); | |
| count++; | |
| continue; | |
| } | |
| if (/^video-.*\.mp4$/i.test(entry) && stats.isFile() && isStale) { | |
| fs.unlinkSync(entryPath); | |
| count++; | |
| } | |
| } catch { | |
| // Skip entries we can't stat/remove (permissions, races). | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/inngest/functions.ts` around lines 47 - 65, Guard each iteration of the
cleanup loop in the stale-artifact cleanup function with a per-entry try/catch,
including stat and removal operations. On filesystem errors such as EACCES,
EPERM, or ENOENT, skip that entry and continue scanning the remaining entries
without aborting the cleanup run.
|
|
||
| async function validateImageUrl(url: string): Promise<void> { | ||
| try { | ||
| const response = await axios.head(url, { timeout: 5000 }); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the target file and nearby symbols first.
ast-grep outline src/lib/video/pipeline.ts --view expanded || true
# Show the relevant section with line numbers.
sed -n '1,220p' src/lib/video/pipeline.ts | cat -n
# Search for the event payload source and any URL validation/allow-listing helpers.
rg -n "video/generate\.requested|imageUrl|axios\.head|Tesseract\.recognize|allow-list|private|link-local|https?://" src/lib/video -S || trueRepository: knoxiboy/DoubtDesk
Length of output: 10882
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the event handler and any request schema that populates `imageUrl`.
rg -n "video/generate\.requested|runVideoPipeline\(|imageUrl" . -S
# Inspect the likely event consumer / API entrypoints if present.
fd -H -t f ".*(api|inngest|event|video).*" . || trueRepository: knoxiboy/DoubtDesk
Length of output: 10917
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the file that defines the event/request flow if search results are manageable.
rg -n "video/generate\.requested|runVideoPipeline\(" src app api lib -S || trueRepository: knoxiboy/DoubtDesk
Length of output: 744
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the video validation and the shared `safeUrl` definition.
sed -n '1,220p' src/lib/validations/video.ts | cat -n
printf '\n----\n'
rg -n "const safeUrl|safeUrl\s*=|safeUrl:" src/lib -S
printf '\n----\n'
# Read the file that defines safeUrl if found.
fd -H -t f ".*validations.*" src/lib | sortRepository: knoxiboy/DoubtDesk
Length of output: 678
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and inspect the shared URL validator.
rg -n "safeUrl" src -SRepository: knoxiboy/DoubtDesk
Length of output: 1004
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the request/event flow around the video generation endpoint and Inngest consumer.
sed -n '1,140p' src/app/api/video/generate/route.ts | cat -n
printf '\n----\n'
sed -n '330,380p' src/inngest/functions.ts | cat -n
printf '\n----\n'
sed -n '1,120p' src/lib/validations/common.ts | cat -nRepository: knoxiboy/DoubtDesk
Length of output: 6716
Restrict server-side image fetches
imageUrl is only syntax-validated, so a caller can still point it at internal or link-local hosts. axios.head(url) and Tesseract.recognize(imageUrl, "eng") fetch that URL server-side without a host/scheme allow-list, private-IP check, or redirect restriction. Limit this to public https URLs and reject private/reserved destinations.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/video/pipeline.ts` at line 42, Restrict the URL handling around the
axios.head call and Tesseract.recognize to validated public HTTPS URLs only:
enforce the https scheme, resolve and reject private, loopback, link-local, and
other reserved IP destinations, and prevent redirects from bypassing validation.
Apply the same validated URL to both server-side fetches and reject invalid or
unsafe imageUrl values before either request.
- Validate image MIME type before passing to Tesseract.recognize() - Only allow common image formats (JPEG, PNG, GIF, WebP, BMP, SVG, TIFF) - Prevent processing of non-image files or malicious file types - Use HEAD request to check Content-Type header efficiently Closes knoxiboy#757
d7bbd49 to
d4c766d
Compare
github-actions
Bot
added
size/xs
and removed
size/xxl
merge-conflict
github-actions
Bot
added
gssoc:approved
2 participants
User description
Prevents Tesseract OCR from processing non-image files.
Closes #757
Summary by CodeRabbit
CodeAnt-AI Description
Tighten form security, limit invite-code brute forcing, and return usable video links
What Changed
Impact
✅ Fewer fake unsubscribe submissions✅ Slower invite-code guessing✅ More reliable video playback links💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.