fix(storage): reject object keys containing path traversal (#683)#697
fix(storage): reject object keys containing path traversal (#683)#697jackthepunished wants to merge 1 commit into
Conversation
Adds validateObjectKey and calls it from getBucketAndKeyRequired so every storage handler (Upload, Download, Delete, multipart, presigned, list versions) rejects keys containing `..` segments or NUL bytes before the key reaches the backend. Closes poyrazK#683.
|
Warning Review limit reached
Your plan includes 1 review of capacity. Refill in 59 minutes and 2 seconds. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more review capacity refills, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than trial, open-source, and free plans. In all cases, review capacity refills continuously over time. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✨ 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 |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds validation for object keys to prevent unsafe keys from reaching the storage backend, along with regression tests for path traversal handling.
Changes:
- Added
validateObjectKeyand integrated it intogetBucketAndKeyRequired. - Added handler tests to ensure path traversal-like keys are rejected and nested keys are accepted.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| internal/handlers/helper.go | Adds object-key validation and calls it before returning bucket/key. |
| internal/handlers/helper_test.go | Adds regression tests for rejecting traversal keys and allowing nested keys. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func validateObjectKey(key string) error { | ||
| if strings.ContainsRune(key, 0x00) { | ||
| return errors.New(errors.InvalidInput, "invalid characters in key") | ||
| } | ||
|
|
||
| for _, seg := range strings.Split(key, "/") { | ||
| if seg == ".." { | ||
| return errors.New(errors.InvalidInput, "path traversal in key") | ||
| } | ||
| } | ||
|
|
||
| // Reject keys whose canonical form would be just the root or empty. | ||
| cleaned := path.Clean("/" + strings.TrimPrefix(key, "/")) | ||
| if cleaned == "/" { | ||
| return errors.New(errors.InvalidInput, "invalid key") | ||
| } | ||
| return nil | ||
| } |
| // validateObjectKey rejects object keys that could be used for path traversal, | ||
| // contain control characters, or would otherwise be unsafe when used as a | ||
| // backend object name. Any `..` segment in the raw key is rejected (we do | ||
| // not silently collapse it, because the user request explicitly tried to | ||
| // reference a parent directory). NUL bytes are rejected outright because | ||
| // they can truncate strings in some backends. |
| t.Run("rejects path traversal", func(t *testing.T) { | ||
| badKeys := []string{"../foo", "a/../b", "../../etc/passwd", "..\x00", "/../escape"} | ||
| for _, k := range badKeys { | ||
| w := httptest.NewRecorder() | ||
| c, _ := gin.CreateTestContext(w) | ||
| c.Params = []gin.Param{ | ||
| {Key: "bucket", Value: testBucket}, | ||
| {Key: "key", Value: k}, | ||
| } | ||
| _, _, ok := getBucketAndKeyRequired(c) | ||
| assert.Falsef(t, ok, "expected key %q to be rejected", k) | ||
| assert.Equal(t, http.StatusBadRequest, w.Code) | ||
| } | ||
| }) |
Adds validateObjectKey and calls it from getBucketAndKeyRequired so every storage handler (Upload, Download, Delete, multipart, presigned, list versions) rejects keys containing
..segments or NUL bytes before the key reaches the backend.Closes #683.