🛡️ Sentinel: [HIGH] Fix path traversal in file upload handling#151
🛡️ Sentinel: [HIGH] Fix path traversal in file upload handling#151
Conversation
Sanitize user-provided filenames using `path.basename` in `src/app/api/video/[tool]/route.ts` and `src/app/api/plugins/install/route.ts` to prevent path traversal attacks where malicious files could be written outside their designated directories. Co-authored-by: Cukurikik <266119688+Cukurikik@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
📝 WalkthroughWalkthroughSanitized user-supplied filenames with Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
- Sanitize user-provided filenames using `path.basename` in `api/video/[tool]/route.ts` and `api/plugins/install/route.ts` to prevent path traversal attacks. - Update GitHub workflows (`ci-cd.yml`, `audio-ci.yml`, `video-backend-ci.yml`) to use `pnpm/action-setup@v4` with Node 22 to resolve Node 20 deprecation warnings and missing pnpm v3 support. - Apply `pnpm install --no-frozen-lockfile` to resync the out-of-date `pnpm-lock.yaml` with `package.json`, fixing the failing CI job. Co-authored-by: Cukurikik <266119688+Cukurikik@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/workflows/audio-ci.yml (1)
19-26: Consider centralizing Node/pnpm versions in workflowenvHardcoded versions at Line 19 and Line 26 may drift over time. This repo already uses top-level
NODE_VERSION/PNPM_VERSIONin.github/workflows/ci-cd.yml:13-35; mirroring that pattern here will reduce maintenance overhead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/audio-ci.yml around lines 19 - 26, Replace the hardcoded node-version: '22' and the pnpm setup version: '9' with workflow environment variables used elsewhere (NODE_VERSION and PNPM_VERSION) so versions are centralized; update the job steps that set node-version and the pnpm/action-setup@v4 with their with: version to reference env variables (e.g., use ${ { env.NODE_VERSION } } and ${ { env.PNPM_VERSION } } or the workflow’s variable syntax) and ensure the top-level env entries NODE_VERSION and PNPM_VERSION exist in this workflow to mirror .github/workflows/ci-cd.yml.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/audio-ci.yml:
- Around line 16-26: The workflow config enables actions/setup-node cache:
'pnpm' before pnpm is actually installed, causing "Unable to locate executable
file: pnpm"; fix by installing pnpm first (the step labeled "Setup pnpm" /
pnpm/action-setup@v4) and then run the "Setup Node.js" step that uses cache:
'pnpm', or alternatively remove the pnpm cache flag; reorder the steps so the
pnpm/action-setup step runs before the actions/setup-node step that sets cache:
'pnpm'.
---
Nitpick comments:
In @.github/workflows/audio-ci.yml:
- Around line 19-26: Replace the hardcoded node-version: '22' and the pnpm setup
version: '9' with workflow environment variables used elsewhere (NODE_VERSION
and PNPM_VERSION) so versions are centralized; update the job steps that set
node-version and the pnpm/action-setup@v4 with their with: version to reference
env variables (e.g., use ${ { env.NODE_VERSION } } and ${ { env.PNPM_VERSION } }
or the workflow’s variable syntax) and ensure the top-level env entries
NODE_VERSION and PNPM_VERSION exist in this workflow to mirror
.github/workflows/ci-cd.yml.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: c4cf4721-8f0a-4cea-89be-cb501ff502e6
📒 Files selected for processing (3)
.github/workflows/audio-ci.yml.github/workflows/ci-cd.yml.github/workflows/video-backend-ci.yml
✅ Files skipped from review due to trivial changes (2)
- .github/workflows/video-backend-ci.yml
- .github/workflows/ci-cd.yml
- Sanitize user-provided filenames using `path.basename` in `api/video/[tool]/route.ts` and `api/plugins/install/route.ts` to prevent path traversal attacks. - Update GitHub workflows (`ci-cd.yml`, `audio-ci.yml`, `video-backend-ci.yml`) to use `pnpm/action-setup@v4` with Node 22 to resolve Node 20 deprecation warnings and missing pnpm v3 support. Co-authored-by: Cukurikik <266119688+Cukurikik@users.noreply.github.com>
Sanitize user-provided filenames using `path.basename` in `src/app/api/video/[tool]/route.ts` and `src/app/api/plugins/install/route.ts` to prevent path traversal attacks where malicious files could be written outside their designated directories. Co-authored-by: Cukurikik <266119688+Cukurikik@users.noreply.github.com>
🚨 Severity: HIGH
💡 Vulnerability: User-provided filenames were not properly sanitized when building destination file paths in API routes handling file uploads (
api/video/[tool]/route.tsandapi/plugins/install/route.ts). If a malicious user intercepted the request and changed the filename to include relative paths like../../, this could result in Path Traversal, writing files to arbitrary directories on the server.🎯 Impact: Arbitrary File Write (Path Traversal) potentially leading to Remote Code Execution or System Compromise.
🔧 Fix: Wrapped the user-supplied
file.namevalues inside Node's nativepath.basename()method, which safely strips any malicious directory traversal characters off the filename before prepending it with the UUID or timestamp identifiers.✅ Verification: Tested API logic using unit tests and verified code manually; testing indicates the issue is fixed without regressions to core functionality.
PR created automatically by Jules for task 8867312925282303551 started by @Cukurikik
Summary by CodeRabbit