-
Notifications
You must be signed in to change notification settings - Fork 870
Add additional validation to UploadPartRequests in Transfer Utility #4083
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
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,12 @@ | ||
| { | ||
| "services": [ | ||
| { | ||
| "serviceName": "S3", | ||
| "type": "patch", | ||
| "changeLogMessages": [ | ||
| "Fixed issue where PartSize and IsLastPart fields were not properly set on Transfer Utility Upload Part Request.", | ||
| "Add additional validations for Transfer Utility requests to ensure Upload Parts have the proper Content Length and File Offsets." | ||
| ] | ||
| } | ||
| ] | ||
| } |
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
|
|
@@ -31,6 +31,8 @@ internal partial class MultipartUploadCommand : BaseCommand | |||
| { | ||||
| public SemaphoreSlim AsyncThrottler { get; set; } | ||||
|
|
||||
| Dictionary<int, ExpectedUploadPart> _expectedUploadParts = new Dictionary<int, ExpectedUploadPart>(); | ||||
|
|
||||
| public override async Task ExecuteAsync(CancellationToken cancellationToken) | ||||
| { | ||||
| // Fire transfer initiated event FIRST, before choosing path | ||||
|
|
@@ -69,6 +71,29 @@ public override async Task ExecuteAsync(CancellationToken cancellationToken) | |||
| cancellationToken.ThrowIfCancellationRequested(); | ||||
|
|
||||
| var uploadRequest = ConstructUploadPartRequest(i, filePosition, initResponse); | ||||
|
|
||||
| var expectedFileOffset = (i - 1) * this._partSize; | ||||
| // Calculating how many bytes are remaining to be uploaded from the current part. | ||||
| // This is mainly used for the last part scenario. | ||||
| var remainingBytes = this._contentLength - expectedFileOffset; | ||||
| // We then check based on the remaining bytes and the content length if this is the last part. | ||||
| var isLastPart = calculateIsLastPart(remainingBytes); | ||||
| // To maintain the same behavior as the ConstructUploadPartRequest. | ||||
| // We are setting the remainingBytes/partSize when using the IAmazonS3Encryption client to 0. | ||||
| if (isLastPart | ||||
| && _s3Client is Amazon.S3.Internal.IAmazonS3Encryption) | ||||
philasmar marked this conversation as resolved.
Show resolved
Hide resolved
|
||||
| { | ||||
| remainingBytes = 0; | ||||
| } | ||||
| this._expectedUploadParts.Add(i, new ExpectedUploadPart { | ||||
| PartNumber = i, | ||||
| ExpectedContentLength = | ||||
| isLastPart ? | ||||
| remainingBytes : | ||||
| this._partSize, | ||||
| ExpectedFileOffset = expectedFileOffset, | ||||
| IsLastPart = isLastPart | ||||
| }); | ||||
| this._partsToUpload.Enqueue(uploadRequest); | ||||
| filePosition += this._partSize; | ||||
| } | ||||
|
|
@@ -150,8 +175,50 @@ private async Task<UploadPartResponse> UploadPartAsync(UploadPartRequest uploadR | |||
| { | ||||
| try | ||||
| { | ||||
| return await _s3Client.UploadPartAsync(uploadRequest, internalCts.Token) | ||||
| var response = await _s3Client.UploadPartAsync(uploadRequest, internalCts.Token) | ||||
| .ConfigureAwait(continueOnCapturedContext: false); | ||||
|
|
||||
|
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. i think all of the validation looks good. just a general question. it looks like we do the other validation for total number of parts = expected here aws-sdk-net/sdk/src/Services/S3/Custom/Transfer/Internal/MultipartUploadCommand.cs Line 164 in 6147c3f
i dont think it matters to me where we do it too much. what do you think? 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. also will having the logic here fail/work for unseekable streams? i think the reason the other code is in the other place is because for unseekable streams they skip validation. wondering if this code path gets executed for unseekable stream or we need to skip it for it? 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. The validation |
||||
| if (response.PartNumber is null) | ||||
| { | ||||
| throw new ArgumentNullException(nameof(response.PartNumber)); | ||||
| } | ||||
| else | ||||
| { | ||||
| if (this._expectedUploadParts.TryGetValue((int) response.PartNumber, out var expectedUploadPart)) | ||||
| { | ||||
| var actualContentLength = uploadRequest.PartSize; | ||||
| if (actualContentLength != expectedUploadPart.ExpectedContentLength) | ||||
| { | ||||
| throw new InvalidOperationException($"Cannot complete multipart upload request. The expected content length of part {expectedUploadPart.PartNumber} " + | ||||
| $"does not equal the actual content length."); | ||||
| } | ||||
|
|
||||
| if (expectedUploadPart.IsLastPart) | ||||
| { | ||||
| if (actualContentLength < 0 || | ||||
| actualContentLength > expectedUploadPart.ExpectedContentLength) | ||||
philasmar marked this conversation as resolved.
Show resolved
Hide resolved
|
||||
| { | ||||
| throw new InvalidOperationException($"Cannot complete multipart upload request. The last part " + | ||||
| $"has an invalid content length."); | ||||
| } | ||||
| } | ||||
|
|
||||
| var actualFileOsset = uploadRequest.FilePosition; | ||||
| if (uploadRequest.IsSetFilePath() && | ||||
| actualFileOsset != expectedUploadPart.ExpectedFileOffset) | ||||
| { | ||||
| throw new InvalidOperationException($"Cannot complete multipart upload request. The expected file offset of part {expectedUploadPart.PartNumber} " + | ||||
| $"does not equal the actual file offset."); | ||||
| } | ||||
| } | ||||
| else | ||||
| { | ||||
| throw new InvalidOperationException("Multipart upload request part was unexpected."); | ||||
| } | ||||
| } | ||||
|
|
||||
|
|
||||
| return response; | ||||
| } | ||||
| catch (Exception exception) | ||||
| { | ||||
|
|
@@ -326,5 +393,13 @@ await _s3Client.AbortMultipartUploadAsync(new AbortMultipartUploadRequest() | |||
| throw; | ||||
| } | ||||
| } | ||||
|
|
||||
| private class ExpectedUploadPart | ||||
| { | ||||
| public int PartNumber { get; set; } | ||||
| public long? ExpectedContentLength { get; set; } | ||||
| public long? ExpectedFileOffset { get; set; } | ||||
| public bool IsLastPart { get; set; } | ||||
| } | ||||
| } | ||||
| } | ||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,133 @@ | ||
| using Amazon.S3; | ||
| using Amazon.S3.Model; | ||
| using Amazon.S3.Transfer; | ||
| using Amazon.S3.Transfer.Internal; | ||
| using Amazon.S3.Util; | ||
| using Microsoft.VisualStudio.TestTools.UnitTesting; | ||
| using Moq; | ||
| using System; | ||
| using System.Collections.Generic; | ||
| using System.IO; | ||
| using System.Linq; | ||
| using System.Text; | ||
| using System.Threading; | ||
| using System.Threading.Tasks; | ||
|
|
||
| namespace AWSSDK.UnitTests | ||
| { | ||
| [TestClass] | ||
| public class MultipartUploadValidationTests | ||
| { | ||
| private static string _tempFilePath; | ||
| private const long fileSizeInBytes = 40 * 1024 * 1024; | ||
|
|
||
| [ClassInitialize] | ||
| public static void ClassInitialize(TestContext context) | ||
| { | ||
| _tempFilePath = Path.GetTempFileName(); | ||
|
|
||
| CreateFileWithSpecificSize(_tempFilePath, fileSizeInBytes); | ||
| } | ||
|
|
||
| [ClassCleanup] | ||
| public static void ClassCleanup() | ||
| { | ||
| if (File.Exists(_tempFilePath)) | ||
| { | ||
| File.Delete(_tempFilePath); | ||
| } | ||
| } | ||
|
|
||
| private static void CreateFileWithSpecificSize(string path, long size) | ||
| { | ||
| using (var fileStream = new FileStream(path, FileMode.Create, FileAccess.Write)) | ||
| { | ||
| fileStream.SetLength(size); | ||
| } | ||
| } | ||
|
|
||
| [TestMethod] | ||
| [TestCategory("S3")] | ||
| public async Task Validation_HappyPath() | ||
| { | ||
| var initiateMultipartUploadResponse = new InitiateMultipartUploadResponse | ||
| { | ||
| UploadId = "test" | ||
| }; | ||
|
|
||
| var s3Client = new Mock<AmazonS3Client>(); | ||
| s3Client | ||
| .Setup(x => x.InitiateMultipartUploadAsync( | ||
| It.IsAny<InitiateMultipartUploadRequest>(), | ||
| It.IsAny<CancellationToken>())) | ||
| .ReturnsAsync(initiateMultipartUploadResponse); | ||
|
|
||
| s3Client | ||
| .Setup(x => x.UploadPartAsync(It.IsAny<UploadPartRequest>(), It.IsAny<CancellationToken>())) | ||
| .ReturnsAsync((UploadPartRequest request, CancellationToken cancellationToken) => | ||
| { | ||
| return new UploadPartResponse { PartNumber = request.PartNumber }; | ||
| }); | ||
|
|
||
| var uploadRequest = new TransferUtilityUploadRequest | ||
| { | ||
| FilePath = _tempFilePath, | ||
| BucketName = "test-bucket", | ||
| Key = "test" | ||
| }; | ||
| var multipartUpload = new MultipartUploadCommand(s3Client.Object, new TransferUtilityConfig(), uploadRequest); | ||
| await multipartUpload.ExecuteAsync(new CancellationToken()); | ||
| } | ||
|
|
||
| [TestMethod] | ||
| [TestCategory("S3")] | ||
| public void Validation_ConstructUploadPartRequest() | ||
| { | ||
| var initiateMultipartUploadResponse = new InitiateMultipartUploadResponse | ||
| { | ||
| UploadId = "test" | ||
| }; | ||
|
|
||
| var s3Client = new Mock<AmazonS3Client>(); | ||
|
|
||
| s3Client | ||
| .Setup(x => x.InitiateMultipartUploadAsync( | ||
| It.IsAny<InitiateMultipartUploadRequest>(), | ||
| It.IsAny<CancellationToken>())) | ||
| .ReturnsAsync(initiateMultipartUploadResponse); | ||
|
|
||
| var uploadRequest = new TransferUtilityUploadRequest | ||
| { | ||
| FilePath = _tempFilePath, | ||
| BucketName = "test-bucket", | ||
| Key = "test" | ||
| }; | ||
|
|
||
| var multipartUpload = new MultipartUploadCommand(s3Client.Object, new TransferUtilityConfig(), uploadRequest); | ||
|
|
||
| var partSize = Math.Max(S3Constants.DefaultPartSize, uploadRequest.ContentLength / S3Constants.MaxNumberOfParts); | ||
|
|
||
| long filePosition = 0; | ||
| for (int i = 1; filePosition < uploadRequest.ContentLength; i++) | ||
| { | ||
| var constructUploadPartRequest = multipartUpload.ConstructUploadPartRequest(i, filePosition, initiateMultipartUploadResponse); | ||
|
|
||
| var expectedFileOffset = (i - 1) * partSize; | ||
| var remainingBytes = uploadRequest.ContentLength - expectedFileOffset; | ||
| var isLastPart = false; | ||
| if (remainingBytes <= partSize) | ||
| isLastPart = true; | ||
|
|
||
| Assert.AreEqual(i, constructUploadPartRequest.PartNumber); | ||
| Assert.AreEqual(isLastPart, constructUploadPartRequest.IsLastPart); | ||
| Assert.AreEqual( | ||
| isLastPart ? remainingBytes : partSize, | ||
| constructUploadPartRequest.PartSize); | ||
| Assert.AreEqual(expectedFileOffset, constructUploadPartRequest.FilePosition); | ||
|
|
||
| filePosition += partSize; | ||
| } | ||
|
|
||
|
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. is there any easy way to right a unit test/integration tests which would reach the code path that throws exception? somehow mock the ConstructUploadPartRequest call to put in incorrect values and then when the response is sent and validation is reached we assert validation error? 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. Not without restructuring a lot of existing code. Most of the stuff is created within the functions and not really mockable. Let me know if you have thoughts on how to do it. |
||
| } | ||
| } | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.