fix: security - #355
Conversation
📝 WalkthroughWalkthroughThe TUS hooks route now validates the X-File-Path header against a regex allowlist, enforces backup storage authorization via database lookups, checks for path traversal before file operations, and improves error handling. Nginx is updated to block all external requests to the hooks endpoint with a 404. ChangesTUS Hook Security Hardening
Estimated code review effort: 4 (Complex) | ~40 minutes Sequence Diagram(s)sequenceDiagram
participant TusServer
participant HooksRoute
participant Database
participant Filesystem
TusServer->>HooksRoute: POST success event (X-File-Path, X-Backup-Storage-Id)
HooksRoute->>HooksRoute: validate X-File-Path against FILE_PATH_RE
alt X-Backup-Storage-Id present
HooksRoute->>Database: verify db + backup storage linkage
Database-->>HooksRoute: matching records or none
HooksRoute->>HooksRoute: return 403 if mismatch/missing
else missing header
HooksRoute->>HooksRoute: log legacy agent warning
end
HooksRoute->>HooksRoute: resolve uploadDir, detect path traversal
HooksRoute->>Filesystem: rename temp file, clean .info/metadata
HooksRoute-->>TusServer: 200 / 400 / 403 / 500 response
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
app/api/tus/hooks/route.tsOops! Something went wrong! :( ESLint: 9.39.4 TypeError: Converting circular structure to JSON ... [truncated 446 characters] ... c/dist/eslintrc.cjs:3261:25) 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@app/api/tus/hooks/route.ts`:
- Around line 43-44: Remove the leftover raw console.log debug statements in the
tus hook route and use the existing structured logger instead. Locate the
backup/generated ID prints in the route handler around the backupStorageId and
generatedId handling, and either delete them entirely or convert them to
log.debug on the existing log child logger so they are not emitted
unconditionally to stdout.
- Around line 61-71: The TUS hook gate in route.ts needs two fixes: the
backupStorage lookup currently only matches by id/database, so it should also
require the row to be in pending status before allowing the request. Also
validate the X-Backup-Storage-Id and X-Generated-Id values before calling
db.query.backupStorage.findFirst, so malformed UUIDs are rejected cleanly
instead of surfacing as 500s. Use the existing variables backupStorageId,
generatedId, and the TUS hook handler logic to place the validation and status
check together before the database query.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 60bc51c7-bf07-45b0-ac50-e10d39f00deb
📒 Files selected for processing (2)
app/api/tus/hooks/route.tsdocker/nginx/nginx.conf
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: build-image
🔇 Additional comments (5)
app/api/tus/hooks/route.ts (4)
4-13: LGTM!
34-37: LGTM!
76-86: LGTM!
126-129: LGTM!docker/nginx/nginx.conf (1)
33-36: 🔒 Security & Privacy
deny allis redundant herereturn 404already blocks the request; the exact-match location is sufficient for the canonical Next.js route path.> Likely an incorrect or invalid review comment.
1 participant
Summary by CodeRabbit