Make path traversal in Orbit more robust - #46570
Conversation
Make path traversal in Orbit more robust.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
|
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)
✅ Files skipped from review due to trivial changes (1)
WalkthroughThis PR hardens client-side file handling by preventing path traversal attacks through HTTP 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
client/base_client_test.go (1)
212-261: ⚡ Quick winAdd one case for the
filename="."fallback path.This suite exercises
.., but the new logic also special-cases empty/.names. A small test withDestFileset would pin that branch and catch regressions in the new fallback behavior.🤖 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 `@client/base_client_test.go` around lines 212 - 261, Add a new subtest in TestFileResponseHandlePathTraversal that exercises the fallback path when Content-Disposition contains filename=".": create fr := &FileResponse{DestPath: destDir, DestFile: "fallback.txt"} and a resp whose Header has `Content-Disposition: []string{`attachment;filename="."`}` then call fr.Handle(resp) and assert no error, that filepath.Base(fr.DestFilePath) == "fallback.txt", and that fr.DestFilePath is inside destDir; this pins the FileResponse.Handle branch that special-cases empty/"." names and verifies the DestFile fallback behavior.
🤖 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 `@server/service/software_installers.go`:
- Line 623: Normalize backslashes in the uploaded filename before deriving the
download basename: in the code paths that set Content-Disposition (references:
r.payload.Filename used at the Content-Disposition header setting lines and the
similar usage around line 1078), replace Windows separators (`\`) with `/` (or
use path.Base from the "path" package which treats `/`) and then call
filepath.Base (or path.Base) on the normalized string; update the header
assignment to use the sanitized basename so backslash-containing names cannot
leak into the header.
---
Nitpick comments:
In `@client/base_client_test.go`:
- Around line 212-261: Add a new subtest in TestFileResponseHandlePathTraversal
that exercises the fallback path when Content-Disposition contains filename=".":
create fr := &FileResponse{DestPath: destDir, DestFile: "fallback.txt"} and a
resp whose Header has `Content-Disposition: []string{`attachment;filename="."`}`
then call fr.Handle(resp) and assert no error, that
filepath.Base(fr.DestFilePath) == "fallback.txt", and that fr.DestFilePath is
inside destDir; this pins the FileResponse.Handle branch that special-cases
empty/"." names and verifies the DestFile fallback behavior.
🪄 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: c19977fe-e031-4467-85ba-f9ad897f8d8c
📒 Files selected for processing (3)
client/base_client.goclient/base_client_test.goserver/service/software_installers.go
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #46570 +/- ##
=======================================
Coverage 66.85% 66.86%
=======================================
Files 2809 2809
Lines 223741 223734 -7
Branches 11347 11347
=======================================
+ Hits 149592 149601 +9
+ Misses 60562 60547 -15
+ Partials 13587 13586 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| // Confirm the resolved path is still inside DestPath. | ||
| cleanDest, err := filepath.Abs(f.DestPath) | ||
| if err != nil { | ||
| return fmt.Errorf("resolving destination directory: %w", err) | ||
| } | ||
| cleanFile, err := filepath.Abs(f.DestFilePath) | ||
| if err != nil { | ||
| return fmt.Errorf("resolving destination file path: %w", err) | ||
| } | ||
| if cleanFile != cleanDest && !strings.HasPrefix(cleanFile, cleanDest+string(filepath.Separator)) { | ||
| return errors.New("invalid filename: path escapes destination directory") | ||
| } |
There was a problem hiding this comment.
What is this part protecting from?
There was a problem hiding this comment.
After filepath.Join, verify the result path starts with the intended DestPath directory
There was a problem hiding this comment.
Sorry. I'm still not following why this is needed.
If filename was already checked what else is this checking? (f.DestPath is not user-provided)
lucasmrod
left a comment
lucasmrod
left a comment
There was a problem hiding this comment.
LGTM.
- Missing
orbit/changes/file. - Left one question around the changes in the orbit endpoints.
- Also if we are keeping changes in
server/service/software_installers.gothen we need a changes/ file
PR #46570 — Path traversal hardening validationAll four sub-tests in
|
3 participants
Make path traversal in Orbit more robust.
Summary by CodeRabbit
Bug Fixes
Tests