[PM-22610] validate file within max length; log deletion of invalid uploads (#5960)

src/Core/Tools/SendFeatures/Commands/NonAnonymousSendCommand.cs

@@ -8,6 +8,7 @@ using Bit.Core.Tools.Repositories;
 using Bit.Core.Tools.SendFeatures.Commands.Interfaces;
 using Bit.Core.Tools.Services;
 using Bit.Core.Utilities;
+using Microsoft.Extensions.Logging;
 
 namespace Bit.Core.Tools.SendFeatures.Commands;
 
@@ -18,19 +19,22 @@ public class NonAnonymousSendCommand : INonAnonymousSendCommand
     private readonly IPushNotificationService _pushNotificationService;
     private readonly ISendValidationService _sendValidationService;
     private readonly ISendCoreHelperService _sendCoreHelperService;
+    private readonly ILogger<NonAnonymousSendCommand> _logger;
 
     public NonAnonymousSendCommand(ISendRepository sendRepository,
         ISendFileStorageService sendFileStorageService,
         IPushNotificationService pushNotificationService,
         ISendAuthorizationService sendAuthorizationService,
         ISendValidationService sendValidationService,
-        ISendCoreHelperService sendCoreHelperService)
+        ISendCoreHelperService sendCoreHelperService,
+        ILogger<NonAnonymousSendCommand> logger)
     {
         _sendRepository = sendRepository;
         _sendFileStorageService = sendFileStorageService;
         _pushNotificationService = pushNotificationService;
         _sendValidationService = sendValidationService;
         _sendCoreHelperService = sendCoreHelperService;
+        _logger = logger;
     }
 
     public async Task SaveSendAsync(Send send)
@@ -63,6 +67,11 @@ public class NonAnonymousSendCommand : INonAnonymousSendCommand
             throw new BadRequestException("No file data.");
         }
 
+        if (fileLength > SendFileSettingHelper.MAX_FILE_SIZE)
+        {
+            throw new BadRequestException($"Max file size is {SendFileSettingHelper.MAX_FILE_SIZE_READABLE}.");
+        }
+
         var storageBytesRemaining = await _sendValidationService.StorageRemainingForSendAsync(send);
 
         if (storageBytesRemaining < fileLength)
@@ -77,13 +86,17 @@ public class NonAnonymousSendCommand : INonAnonymousSendCommand
             data.Id = fileId;
             data.Size = fileLength;
             data.Validated = false;
-            send.Data = JsonSerializer.Serialize(data,
+            send.Data = JsonSerializer.Serialize(data, JsonHelpers.IgnoreWritingNull);
-                JsonHelpers.IgnoreWritingNull);
             await SaveSendAsync(send);
             return await _sendFileStorageService.GetSendFileUploadUrlAsync(send, fileId);
         }
         catch
         {
+            _logger.LogWarning(
+                "Deleted file from {SendId} because an error occurred when creating the upload URL.",
+                send.Id
+            );
+
             // Clean up since this is not transactional
             await _sendFileStorageService.DeleteFileAsync(send, fileId);
             throw;
@@ -135,23 +148,31 @@ public class NonAnonymousSendCommand : INonAnonymousSendCommand
     {
         var fileData = JsonSerializer.Deserialize<SendFileData>(send.Data);
 
-        var (valid, realSize) = await _sendFileStorageService.ValidateFileAsync(send, fileData.Id, fileData.Size, SendFileSettingHelper.FILE_SIZE_LEEWAY);
+        var minimum = fileData.Size - SendFileSettingHelper.FILE_SIZE_LEEWAY;
+        var maximum = Math.Min(
+            fileData.Size + SendFileSettingHelper.FILE_SIZE_LEEWAY,
+            SendFileSettingHelper.MAX_FILE_SIZE
+        );
+        var (valid, size) = await _sendFileStorageService.ValidateFileAsync(send, fileData.Id, minimum, maximum);
 
-        if (!valid || realSize > SendFileSettingHelper.FILE_SIZE_LEEWAY)
+        // protect file service from upload hijacking by deleting invalid sends
+        if (!valid)
         {
-            // File reported differs in size from that promised. Must be a rogue client. Delete Send
+            _logger.LogWarning(
+                "Deleted {SendId} because its reported size {Size} was outside the expected range ({Minimum} - {Maximum}).",
+                send.Id,
+                size,
+                minimum,
+                maximum
+            );
             await DeleteSendAsync(send);
             return false;
         }
 
-        // Update Send data if necessary
+        // replace expected size with validated size
-        if (realSize != fileData.Size)
+        fileData.Size = size;
-        {
-            fileData.Size = realSize.Value;
-        }
         fileData.Validated = true;
-        send.Data = JsonSerializer.Serialize(fileData,
+        send.Data = JsonSerializer.Serialize(fileData, JsonHelpers.IgnoreWritingNull);
-            JsonHelpers.IgnoreWritingNull);
         await SaveSendAsync(send);
 
         return valid;

src/Core/Tools/SendFeatures/Services/AzureSendFileStorageService.cs

@@ -88,7 +88,7 @@ public class AzureSendFileStorageService : ISendFileStorageService
         return sasUri.ToString();
     }
 
-    public async Task<(bool, long?)> ValidateFileAsync(Send send, string fileId, long expectedFileSize, long leeway)
+    public async Task<(bool, long)> ValidateFileAsync(Send send, string fileId, long minimum, long maximum)
     {
         await InitAsync();
 
@@ -116,17 +116,14 @@ public class AzureSendFileStorageService : ISendFileStorageService
             await blobClient.SetHttpHeadersAsync(headers);
 
             var length = blobProperties.Value.ContentLength;
-            if (length < expectedFileSize - leeway || length > expectedFileSize + leeway)
+            var valid = minimum <= length || length <= maximum;
-            {
-                return (false, length);
-            }
 
-            return (true, length);
+            return (valid, length);
         }
         catch (Exception ex)
         {
-            _logger.LogError(ex, "Unhandled error in ValidateFileAsync");
+            _logger.LogError(ex, $"A storage operation failed in {nameof(ValidateFileAsync)}");
-            return (false, null);
+            return (false, -1);
         }
     }
 

src/Core/Tools/SendFeatures/Services/Interfaces/ISendStorageService.cs

@@ -56,16 +56,13 @@ public interface ISendFileStorageService
     /// </summary>
     /// <param name="send"><see cref="Send" /> used to help validate file</param>
     /// <param name="fileId">File id to identify which file to validate</param>
-    /// <param name="expectedFileSize">Expected file size of the file</param>
+    /// <param name="minimum">The minimum allowed length of the stored file in bytes.</param>
-    /// <param name="leeway">
+    /// <param name="maximum">The maximuim allowed length of the stored file in bytes</param>
-    /// Send file size tolerance in bytes. When an uploaded file's `expectedFileSize`
+    /// <returns>
-    /// is outside of the leeway, the storage operation fails.
+    /// A task that completes when validation is finished. The first element of the tuple is
-    /// </param>
+    /// <see langword="true" /> when validation succeeded, and false otherwise. The second element
-    /// <throws>
+    /// of the tuple contains the observed file length in bytes. If an error occurs during validation,
-    /// ❌ Fill this in with an explanation of the error thrown when `leeway` is incorrect
+    /// this returns `-1`.
-    /// </throws>
-    /// <returns>Task object for async operations with Tuple of boolean that determines if file was valid and long that
-    /// the actual file size of the file.
     /// </returns>
-    Task<(bool, long?)> ValidateFileAsync(Send send, string fileId, long expectedFileSize, long leeway);
+    Task<(bool valid, long length)> ValidateFileAsync(Send send, string fileId, long minimum, long maximum);
 }

src/Core/Tools/SendFeatures/Services/LocalSendStorageService.cs

@@ -85,9 +85,9 @@ public class LocalSendStorageService : ISendFileStorageService
     public Task<string> GetSendFileUploadUrlAsync(Send send, string fileId)
         => Task.FromResult($"/sends/{send.Id}/file/{fileId}");
 
-    public Task<(bool, long?)> ValidateFileAsync(Send send, string fileId, long expectedFileSize, long leeway)
+    public Task<(bool, long)> ValidateFileAsync(Send send, string fileId, long minimum, long maximum)
     {
-        long? length = null;
+        long length = -1;
         var path = FilePath(send, fileId);
         if (!File.Exists(path))
         {
@@ -95,11 +95,7 @@ public class LocalSendStorageService : ISendFileStorageService
         }
 
         length = new FileInfo(path).Length;
-        if (expectedFileSize < length - leeway || expectedFileSize > length + leeway)
+        var valid = minimum < length || length < maximum;
-        {
+        return Task.FromResult((valid, length));
-            return Task.FromResult((false, length));
-        }
-
-        return Task.FromResult((true, length));
     }
 }

src/Core/Tools/Services/NoopImplementations/NoopSendFileStorageService.cs

@@ -37,8 +37,8 @@ public class NoopSendFileStorageService : ISendFileStorageService
         return Task.FromResult((string)null);
     }
 
-    public Task<(bool, long?)> ValidateFileAsync(Send send, string fileId, long expectedFileSize, long leeway)
+    public Task<(bool, long)> ValidateFileAsync(Send send, string fileId, long minimum, long maximum)
     {
-        return Task.FromResult((false, default(long?)));
+        return Task.FromResult((false, -1L));
     }
 }

test/Core.Test/Tools/Services/NonAnonymousSendCommandTests.cs

@@ -10,10 +10,10 @@ using Bit.Core.Tools.Entities;
 using Bit.Core.Tools.Enums;
 using Bit.Core.Tools.Models.Data;
 using Bit.Core.Tools.Repositories;
-using Bit.Core.Tools.SendFeatures;
 using Bit.Core.Tools.SendFeatures.Commands;
 using Bit.Core.Tools.Services;
 using Bit.Test.Common.AutoFixture.Attributes;
+using Microsoft.Extensions.Logging;
 using NSubstitute;
 using NSubstitute.ExceptionExtensions;
 using Xunit;
@@ -35,6 +35,8 @@ public class NonAnonymousSendCommandTests
     private readonly ISendCoreHelperService _sendCoreHelperService;
     private readonly NonAnonymousSendCommand _nonAnonymousSendCommand;
 
+    private readonly ILogger<NonAnonymousSendCommand> _logger;
+
     public NonAnonymousSendCommandTests()
     {
         _sendRepository = Substitute.For<ISendRepository>();
@@ -45,6 +47,7 @@ public class NonAnonymousSendCommandTests
         _sendValidationService = Substitute.For<ISendValidationService>();
         _currentContext = Substitute.For<ICurrentContext>();
         _sendCoreHelperService = Substitute.For<ISendCoreHelperService>();
+        _logger = Substitute.For<ILogger<NonAnonymousSendCommand>>();
 
         _nonAnonymousSendCommand = new NonAnonymousSendCommand(
             _sendRepository,
@@ -52,7 +55,8 @@ public class NonAnonymousSendCommandTests
             _pushNotificationService,
             _sendAuthorizationService,
             _sendValidationService,
-            _sendCoreHelperService
+            _sendCoreHelperService,
+            _logger
         );
     }
 
@@ -652,11 +656,11 @@ public class NonAnonymousSendCommandTests
             UserId = userId
         };
         var fileData = new SendFileData();
-        var fileLength = 15L * 1024L * 1024L * 1024L; // 15GB
+        var fileLength = 15L * 1024L * 1024L; // 15 MB
 
-        // Configure validation service to return large but insufficient storage (10GB for self-hosted non-premium)
+        // Configure validation service to return insufficient storage
         _sendValidationService.StorageRemainingForSendAsync(send)
-            .Returns(10L * 1024L * 1024L * 1024L); // 10GB remaining (self-hosted default)
+            .Returns(10L * 1024L * 1024L); // 10 MB remaining
 
         // Act & Assert
         var exception = await Assert.ThrowsAsync<BadRequestException>(() =>
@@ -687,11 +691,40 @@ public class NonAnonymousSendCommandTests
             UserId = userId
         };
         var fileData = new SendFileData();
-        var fileLength = 2L * 1024L * 1024L * 1024L; // 2GB
+        var fileLength = 2L * 1024L * 1024L * 1024L; // 2MB
 
-        // Configure validation service to return 1GB storage (cloud non-premium default)
+        // Act & Assert
+        var exception = await Assert.ThrowsAsync<BadRequestException>(() =>
+            _nonAnonymousSendCommand.SaveFileSendAsync(send, fileData, fileLength));
+
+        Assert.Contains("Max file size is ", exception.Message);
+
+        // Verify no further methods were called
+        await _sendValidationService.DidNotReceive().StorageRemainingForSendAsync(Arg.Any<Send>());
+        await _sendRepository.DidNotReceive().CreateAsync(Arg.Any<Send>());
+        await _sendRepository.DidNotReceive().UpsertAsync(Arg.Any<Send>());
+        await _sendFileStorageService.DidNotReceive().GetSendFileUploadUrlAsync(Arg.Any<Send>(), Arg.Any<string>());
+        await _pushNotificationService.DidNotReceive().PushSyncSendCreateAsync(Arg.Any<Send>());
+        await _pushNotificationService.DidNotReceive().PushSyncSendUpdateAsync(Arg.Any<Send>());
+    }
+
+    [Fact]
+    public async Task SaveFileSendAsync_UserCanAccessPremium_IsNotPremium_IsNotSelfHosted_NotEnoughSpace_ThrowsBadRequest()
+    {
+        // Arrange
+        var userId = Guid.NewGuid();
+        var send = new Send
+        {
+            Id = Guid.NewGuid(),
+            Type = SendType.File,
+            UserId = userId
+        };
+        var fileData = new SendFileData();
+        var fileLength = 2L * 1024L * 1024L; // 2MB
+
+        // Configure validation service to return 1 MB storage remaining
         _sendValidationService.StorageRemainingForSendAsync(send)
-            .Returns(1L * 1024L * 1024L * 1024L); // 1GB remaining (cloud default)
+            .Returns(1L * 1024L * 1024L);
 
         // Act & Assert
         var exception = await Assert.ThrowsAsync<BadRequestException>(() =>
@@ -756,7 +789,7 @@ public class NonAnonymousSendCommandTests
             UserId = null
         };
         var fileData = new SendFileData();
-        var fileLength = 2L * 1024L * 1024L * 1024L; // 2GB
+        var fileLength = 2L * 1024L * 1024L; // 2 MB
 
         // Configure validation service to throw BadRequest when checking storage for org without storage
         _sendValidationService.StorageRemainingForSendAsync(send)
@@ -792,11 +825,10 @@ public class NonAnonymousSendCommandTests
             UserId = null
         };
         var fileData = new SendFileData();
-        var fileLength = 2L * 1024L * 1024L * 1024L; // 2GB
+        var fileLength = 2L * 1024L * 1024L; // 2 MB
 
-        // Configure validation service to return 1GB storage (org's max storage limit)
         _sendValidationService.StorageRemainingForSendAsync(send)
-            .Returns(1L * 1024L * 1024L * 1024L); // 1GB remaining
+            .Returns(1L * 1024L * 1024L); // 1 MB remaining
 
         // Act & Assert
         var exception = await Assert.ThrowsAsync<BadRequestException>(() =>
@@ -980,7 +1012,7 @@ public class NonAnonymousSendCommandTests
         };
 
         // Setup validation to succeed
-        _sendFileStorageService.ValidateFileAsync(send, sendFileData.Id, sendFileData.Size, SendFileSettingHelper.FILE_SIZE_LEEWAY).Returns((true, sendFileData.Size));
+        _sendFileStorageService.ValidateFileAsync(send, sendFileData.Id, Arg.Any<long>(), Arg.Any<long>()).Returns((true, sendFileData.Size));
 
         // Act
         await _nonAnonymousSendCommand.UploadFileToExistingSendAsync(stream, send);
@@ -1014,7 +1046,7 @@ public class NonAnonymousSendCommandTests
             Data = JsonSerializer.Serialize(sendFileData)
         };
 
-        _sendFileStorageService.ValidateFileAsync(send, sendFileData.Id, sendFileData.Size, SendFileSettingHelper.FILE_SIZE_LEEWAY).Returns((true, sendFileData.Size));
+        _sendFileStorageService.ValidateFileAsync(send, sendFileData.Id, Arg.Any<long>(), Arg.Any<long>()).Returns((true, sendFileData.Size));
 
         // Act
         await _nonAnonymousSendCommand.UploadFileToExistingSendAsync(stream, send);