-
Notifications
You must be signed in to change notification settings - Fork 726
[merge 10/27/25] validate individual part ranges/size in upload/download #3202
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| { | ||
| "id": "9cf590d7-3764-44d0-8d7d-959651964337", | ||
| "type": "feature", | ||
| "description": "Add durability checks to validate part count and range for upload/download. You can disable this with `DisableValidateParts` in upload/download options, though doing so is not recommended because it damages the durability posture of your application.", | ||
| "modules": [ | ||
| "feature/s3/manager" | ||
| ] | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -77,6 +77,14 @@ type Downloader struct { | |
| // operation requests made by the downloader. | ||
| ClientOptions []func(*s3.Options) | ||
|
|
||
| // By default, the downloader verifies that individual part ranges align | ||
| // based on the configured part size. | ||
| // | ||
| // You can disable that with this flag, however, Amazon S3 recommends | ||
| // against doing so because it damages the durability posture of object | ||
| // downloads. | ||
| DisableValidateParts bool | ||
|
|
||
| // Defines the buffer strategy used when downloading a part. | ||
| // | ||
| // If a WriterReadFromProvider is given the Download manager | ||
|
|
@@ -404,6 +412,15 @@ func (d *downloader) tryDownloadChunk(params *s3.GetObjectInput, w io.Writer) (i | |
| if err != nil { | ||
| return 0, err | ||
| } | ||
|
|
||
| if !d.cfg.DisableValidateParts && params.Range != nil && resp.ContentRange != nil { | ||
| expectStart, expectEnd := parseContentRange(*params.Range) | ||
| actualStart, actualEnd := parseContentRange(*resp.ContentRange) | ||
| if isRangeMismatch(expectStart, expectEnd, actualStart, actualEnd) { | ||
| return 0, fmt.Errorf("invalid content range: expect %d-%d, got %d-%d", expectStart, expectEnd, actualStart, actualEnd) | ||
| } | ||
| } | ||
|
|
||
| d.setTotalBytes(resp) // Set total if not yet set. | ||
| d.once.Do(func() { | ||
| d.etag = aws.ToString(resp.ETag) | ||
|
|
@@ -422,6 +439,46 @@ func (d *downloader) tryDownloadChunk(params *s3.GetObjectInput, w io.Writer) (i | |
| return n, nil | ||
| } | ||
|
|
||
| func parseContentRange(v string) (int, int) { | ||
| parts := strings.Split(v, "/") // chop the total off, if it's there | ||
|
|
||
| // we send "bytes=" but S3 appears to return "bytes ", handle both | ||
| trimmed := strings.TrimPrefix(parts[0], "bytes ") | ||
| trimmed = strings.TrimPrefix(trimmed, "bytes=") | ||
|
|
||
| parts = strings.Split(trimmed, "-") | ||
| if len(parts) != 2 { | ||
| return -1, -1 | ||
| } | ||
|
|
||
| start, err := strconv.Atoi(parts[0]) | ||
| if err != nil { | ||
| return -1, -1 | ||
| } | ||
|
|
||
| end, err := strconv.Atoi(parts[1]) | ||
| if err != nil { | ||
| return -1, -1 | ||
| } | ||
|
|
||
| return start, end | ||
| } | ||
|
|
||
| func isRangeMismatch(expectStart, expectEnd, actualStart, actualEnd int) bool { | ||
| if expectStart == -1 || expectEnd == -1 || actualStart == -1 || actualEnd == -1 { | ||
| return false // we don't know, one of the ranges was missing or unparseable | ||
| } | ||
|
|
||
| // for the final chunk (or the first chunk if it's smaller) we still | ||
| // request a full chunk but we get back the actual final part of the | ||
| // object, which will be smaller | ||
| if expectStart == actualStart && actualEnd < expectEnd { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nice capture |
||
| return false | ||
| } | ||
|
|
||
| return expectStart != actualStart || expectEnd != actualEnd | ||
| } | ||
|
|
||
| // getTotalBytes is a thread-safe getter for retrieving the total byte status. | ||
| func (d *downloader) getTotalBytes() int64 { | ||
| d.m.Lock() | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,52 @@ | ||
| package manager_test | ||
|
|
||
| import ( | ||
| "context" | ||
| "strings" | ||
| "testing" | ||
|
|
||
| "github.com/aws/aws-sdk-go-v2/aws" | ||
| "github.com/aws/aws-sdk-go-v2/feature/s3/manager" | ||
| "github.com/aws/aws-sdk-go-v2/service/s3" | ||
| ) | ||
|
|
||
| type invalidRangeClient struct { | ||
| } | ||
|
|
||
| func TestDownload_RangeMismatch(t *testing.T) { | ||
| c, _, _ := newDownloadBadRangeClient(buf12MB) | ||
|
|
||
| d := manager.NewDownloader(c, func(d *manager.Downloader) { | ||
| d.Concurrency = 1 | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can we add a test when we use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes |
||
| }) | ||
|
|
||
| w := manager.NewWriteAtBuffer(make([]byte, len(buf12MB))) | ||
| _, err := d.Download(context.Background(), w, &s3.GetObjectInput{ | ||
| Bucket: aws.String("bucket"), | ||
| Key: aws.String("key"), | ||
| }) | ||
| if err == nil { | ||
| t.Fatalf("expect err, got none") | ||
| } | ||
| if !strings.Contains(err.Error(), "invalid content range") { | ||
| t.Errorf("error mismatch:\n%v !=\n%v", err, "invalid content range") | ||
| } | ||
| } | ||
|
|
||
| func TestDownload_RangeMismatchDisabled(t *testing.T) { | ||
| c, _, _ := newDownloadBadRangeClient(buf12MB) | ||
|
|
||
| d := manager.NewDownloader(c, func(d *manager.Downloader) { | ||
| d.Concurrency = 1 | ||
| d.DisableValidateParts = true | ||
| }) | ||
|
|
||
| w := manager.NewWriteAtBuffer(make([]byte, len(buf12MB))) | ||
| _, err := d.Download(context.Background(), w, &s3.GetObjectInput{ | ||
| Bucket: aws.String("bucket"), | ||
| Key: aws.String("key"), | ||
| }) | ||
| if err != nil { | ||
| t.Fatalf("expect no err, got %v", err) | ||
| } | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: IIRC
params.Rangewill always be set at old line 361 indownloadChunk