From e55e2553cfab748fa2da2c54a9e14fcb6170f7cb Mon Sep 17 00:00:00 2001 From: jaasen-livefront Date: Thu, 26 Feb 2026 16:55:11 -0800 Subject: [PATCH 1/4] create short-lived signed attachment URL for self-hosted instances --- .../Vault/Controllers/CiphersController.cs | 43 +++++ .../Services/IAttachmentStorageService.cs | 5 + src/Core/Vault/Services/ICipherService.cs | 2 + .../AzureAttachmentStorageService.cs | 6 + .../Services/Implementations/CipherService.cs | 55 +++++- .../LocalAttachmentStorageService.cs | 11 ++ .../NoopAttachmentStorageService.cs | 5 + .../Controllers/CiphersControllerTests.cs | 111 ++++++++++++ .../Vault/Services/CipherServiceTests.cs | 161 ++++++++++++++++++ 9 files changed, 397 insertions(+), 2 deletions(-) diff --git a/src/Api/Vault/Controllers/CiphersController.cs b/src/Api/Vault/Controllers/CiphersController.cs index 9ead8bc4bdca..d83ad32f9461 100644 --- a/src/Api/Vault/Controllers/CiphersController.cs +++ b/src/Api/Vault/Controllers/CiphersController.cs @@ -1498,10 +1498,53 @@ public async Task GetAttachmentData(Guid id, string att { var userId = _userService.GetProperUserId(User).Value; var cipher = await GetByIdAsync(id, userId); + if (cipher == null) + { + throw new NotFoundException(); + } + var result = await _cipherService.GetAttachmentDownloadDataAsync(cipher, attachmentId); return new AttachmentResponseModel(result); } + /// + /// Serves a locally stored attachment file using a time-limited, signed token. + /// This endpoint replaces direct static file access for self-hosted environments + /// to ensure that only authorized users can download attachment files. + /// + [AllowAnonymous] + [HttpGet("attachment/download")] + public async Task DownloadAttachmentAsync([FromQuery] string token) + { + if (string.IsNullOrEmpty(token)) + { + throw new NotFoundException(); + } + + (Guid cipherId, string attachmentId) = CipherService.ParseAttachmentDownloadToken(token, + _cipherService.CreateAttachmentDownloadProtector()); + + var cipher = await _cipherRepository.GetByIdAsync(cipherId); + if (cipher == null) + { + throw new NotFoundException(); + } + + var attachments = cipher.GetAttachments(); + if (attachments == null || !attachments.TryGetValue(attachmentId, out var attachmentData)) + { + throw new NotFoundException(); + } + + var stream = await _attachmentStorageService.GetAttachmentReadStreamAsync(cipher, attachmentData); + if (stream == null) + { + throw new NotFoundException(); + } + + return File(stream, "application/octet-stream", attachmentData.FileName); + } + [HttpPost("{id}/attachment/{attachmentId}/share")] [RequestSizeLimit(Constants.FileSize101mb)] [DisableFormValueModelBinding] diff --git a/src/Core/Services/IAttachmentStorageService.cs b/src/Core/Services/IAttachmentStorageService.cs index 7c19ce321aa8..6702d285e296 100644 --- a/src/Core/Services/IAttachmentStorageService.cs +++ b/src/Core/Services/IAttachmentStorageService.cs @@ -19,5 +19,10 @@ public interface IAttachmentStorageService Task DeleteAttachmentsForUserAsync(Guid userId); Task GetAttachmentUploadUrlAsync(Cipher cipher, CipherAttachment.MetaData attachmentData); Task GetAttachmentDownloadUrlAsync(Cipher cipher, CipherAttachment.MetaData attachmentData); + /// + /// Opens a read stream for a locally stored attachment file. + /// Returns null if the storage implementation does not support direct streaming (e.g. cloud storage). + /// + Task GetAttachmentReadStreamAsync(Cipher cipher, CipherAttachment.MetaData attachmentData); Task<(bool, long?)> ValidateFileAsync(Cipher cipher, CipherAttachment.MetaData attachmentData, long leeway); } diff --git a/src/Core/Vault/Services/ICipherService.cs b/src/Core/Vault/Services/ICipherService.cs index 765dae30c147..3803620a9053 100644 --- a/src/Core/Vault/Services/ICipherService.cs +++ b/src/Core/Vault/Services/ICipherService.cs @@ -3,6 +3,7 @@ using Bit.Core.Vault.Entities; using Bit.Core.Vault.Models.Data; +using Microsoft.AspNetCore.DataProtection; namespace Bit.Core.Vault.Services; @@ -36,6 +37,7 @@ Task> ShareManyAsync(IEnumerable<(CipherDetails ciphe Task> RestoreManyAsync(IEnumerable cipherIds, Guid restoringUserId, Guid? organizationId = null, bool orgAdmin = false); Task UploadFileForExistingAttachmentAsync(Stream stream, Cipher cipher, CipherAttachment.MetaData attachmentId); Task GetAttachmentDownloadDataAsync(Cipher cipher, string attachmentId); + ITimeLimitedDataProtector CreateAttachmentDownloadProtector(); Task ValidateCipherAttachmentFile(Cipher cipher, CipherAttachment.MetaData attachmentData); Task ValidateBulkCollectionAssignmentAsync(IEnumerable collectionIds, IEnumerable cipherIds, Guid userId); } diff --git a/src/Core/Vault/Services/Implementations/AzureAttachmentStorageService.cs b/src/Core/Vault/Services/Implementations/AzureAttachmentStorageService.cs index d03a7e5fcf96..3747c9ed8fa7 100644 --- a/src/Core/Vault/Services/Implementations/AzureAttachmentStorageService.cs +++ b/src/Core/Vault/Services/Implementations/AzureAttachmentStorageService.cs @@ -193,6 +193,12 @@ public async Task DeleteAttachmentsForUserAsync(Guid userId) await InitAsync(_defaultContainerName); } + public Task GetAttachmentReadStreamAsync(Cipher cipher, CipherAttachment.MetaData attachmentData) + { + // Azure storage uses SAS URLs for downloads; direct streaming is not supported. + return Task.FromResult(null); + } + public async Task<(bool, long?)> ValidateFileAsync(Cipher cipher, CipherAttachment.MetaData attachmentData, long leeway) { await InitAsync(attachmentData.ContainerName); diff --git a/src/Core/Vault/Services/Implementations/CipherService.cs b/src/Core/Vault/Services/Implementations/CipherService.cs index 3a970d82bdff..857b06d8d4b8 100644 --- a/src/Core/Vault/Services/Implementations/CipherService.cs +++ b/src/Core/Vault/Services/Implementations/CipherService.cs @@ -21,6 +21,7 @@ using Bit.Core.Vault.Models.Data; using Bit.Core.Vault.Queries; using Bit.Core.Vault.Repositories; +using Microsoft.AspNetCore.DataProtection; namespace Bit.Core.Vault.Services; @@ -48,6 +49,10 @@ public class CipherService : ICipherService private readonly IApplicationCacheService _applicationCacheService; private readonly IFeatureService _featureService; private readonly IPricingClient _pricingClient; + private readonly IDataProtectionProvider _dataProtectionProvider; + + internal static readonly string AttachmentDownloadProtectorPurpose = "AttachmentDownload"; + private static readonly TimeSpan _downloadLinkLifetime = TimeSpan.FromMinutes(1); public CipherService( ICipherRepository cipherRepository, @@ -68,7 +73,8 @@ public CipherService( IPolicyRequirementQuery policyRequirementQuery, IApplicationCacheService applicationCacheService, IFeatureService featureService, - IPricingClient pricingClient) + IPricingClient pricingClient, + IDataProtectionProvider dataProtectionProvider) { _cipherRepository = cipherRepository; _folderRepository = folderRepository; @@ -89,6 +95,7 @@ public CipherService( _applicationCacheService = applicationCacheService; _featureService = featureService; _pricingClient = pricingClient; + _dataProtectionProvider = dataProtectionProvider; } public async Task SaveAsync(Cipher cipher, Guid savingUserId, DateTime? lastKnownRevisionDate, @@ -412,17 +419,61 @@ public async Task GetAttachmentDownloadDataAsync(Cipher throw new NotFoundException(); } + var url = await _attachmentStorageService.GetAttachmentDownloadUrlAsync(cipher, data); + + // For local (self-hosted) storage, generate a time-limited signed download URL + // to prevent unauthenticated access to predictable attachment file paths. + // Cloud storage (Azure) already uses time-limited SAS URLs. + if (_attachmentStorageService.FileUploadType == FileUploadType.Direct) + { + var protector = _dataProtectionProvider.CreateProtector(AttachmentDownloadProtectorPurpose); + var timedProtector = protector.ToTimeLimitedDataProtector(); + var token = timedProtector.Protect( + $"{cipher.Id}|{attachmentId}", + _downloadLinkLifetime); + url = $"{_globalSettings.BaseServiceUri.Api}/ciphers/attachment/download?token={Uri.EscapeDataString(token)}"; + } + var response = new AttachmentResponseData { Cipher = cipher, Data = data, Id = attachmentId, - Url = await _attachmentStorageService.GetAttachmentDownloadUrlAsync(cipher, data), + Url = url, }; return response; } + public ITimeLimitedDataProtector CreateAttachmentDownloadProtector() + { + return _dataProtectionProvider + .CreateProtector(AttachmentDownloadProtectorPurpose) + .ToTimeLimitedDataProtector(); + } + + public static (Guid cipherId, string attachmentId) ParseAttachmentDownloadToken( + string token, ITimeLimitedDataProtector protector) + { + string payload; + try + { + payload = protector.Unprotect(token); + } + catch + { + throw new NotFoundException(); + } + + var parts = payload.Split('|'); + if (parts.Length != 2 || !Guid.TryParse(parts[0], out var cipherId)) + { + throw new NotFoundException(); + } + + return (cipherId, parts[1]); + } + public async Task DeleteAsync(CipherDetails cipherDetails, Guid deletingUserId, bool orgAdmin = false) { if (!orgAdmin && !await UserCanDeleteAsync(cipherDetails, deletingUserId)) diff --git a/src/Core/Vault/Services/Implementations/LocalAttachmentStorageService.cs b/src/Core/Vault/Services/Implementations/LocalAttachmentStorageService.cs index ac3c4a796d3e..61b7c54e450b 100644 --- a/src/Core/Vault/Services/Implementations/LocalAttachmentStorageService.cs +++ b/src/Core/Vault/Services/Implementations/LocalAttachmentStorageService.cs @@ -174,6 +174,17 @@ private string AttachmentFilePath(string attachmentId, Guid cipherId, Guid? orga organizationId.HasValue ? AttachmentFilePath(OrganizationDirectoryPath(cipherId, organizationId.Value, temp), attachmentId) : AttachmentFilePath(CipherDirectoryPath(cipherId, temp), attachmentId); + public Task GetAttachmentReadStreamAsync(Cipher cipher, CipherAttachment.MetaData attachmentData) + { + var path = AttachmentFilePath(attachmentData.AttachmentId, cipher.Id, temp: false); + if (!File.Exists(path)) + { + return Task.FromResult(null); + } + + return Task.FromResult(File.OpenRead(path)); + } + public Task GetAttachmentUploadUrlAsync(Cipher cipher, CipherAttachment.MetaData attachmentData) => Task.FromResult($"{cipher.Id}/attachment/{attachmentData.AttachmentId}"); diff --git a/src/Core/Vault/Services/NoopImplementations/NoopAttachmentStorageService.cs b/src/Core/Vault/Services/NoopImplementations/NoopAttachmentStorageService.cs index 8014849d93ad..fd5c935b36f0 100644 --- a/src/Core/Vault/Services/NoopImplementations/NoopAttachmentStorageService.cs +++ b/src/Core/Vault/Services/NoopImplementations/NoopAttachmentStorageService.cs @@ -62,6 +62,11 @@ public Task GetAttachmentDownloadUrlAsync(Cipher cipher, CipherAttachmen return Task.FromResult((string)null); } + public Task GetAttachmentReadStreamAsync(Cipher cipher, CipherAttachment.MetaData attachmentData) + { + return Task.FromResult(null); + } + public Task GetAttachmentUploadUrlAsync(Cipher cipher, CipherAttachment.MetaData attachmentData) { return Task.FromResult(default(string)); diff --git a/test/Api.Test/Vault/Controllers/CiphersControllerTests.cs b/test/Api.Test/Vault/Controllers/CiphersControllerTests.cs index 6fba9730a73b..7ba01d36345b 100644 --- a/test/Api.Test/Vault/Controllers/CiphersControllerTests.cs +++ b/test/Api.Test/Vault/Controllers/CiphersControllerTests.cs @@ -18,6 +18,7 @@ using Bit.Core.Vault.Services; using Bit.Test.Common.AutoFixture; using Bit.Test.Common.AutoFixture.Attributes; +using Microsoft.AspNetCore.DataProtection; using NSubstitute; using NSubstitute.ReturnsExtensions; using Xunit; @@ -2152,4 +2153,114 @@ public async Task PutShare_UpdateExistingFolderAndFavorite_UpdatesUserSpecificFi Assert.Equal(newFolderId, result.FolderId); Assert.True(result.Favorite); } + + [Theory, BitAutoData] + public async Task GetAttachmentData_CipherNotFound_ThrowsNotFoundException( + Guid cipherId, string attachmentId, Guid userId, + SutProvider sutProvider) + { + sutProvider.GetDependency().GetProperUserId(default).ReturnsForAnyArgs((Guid?)userId); + sutProvider.GetDependency().GetByIdAsync(cipherId, userId).ReturnsNull(); + + await Assert.ThrowsAsync( + () => sutProvider.Sut.GetAttachmentData(cipherId, attachmentId)); + } + + [Theory, BitAutoData] + public async Task GetAttachmentData_CipherFound_ReturnsAttachmentResponse( + Guid cipherId, string attachmentId, Guid userId, + SutProvider sutProvider) + { + sutProvider.GetDependency().GetProperUserId(default).ReturnsForAnyArgs((Guid?)userId); + + var cipherDetails = new CipherDetails { Id = cipherId, UserId = userId, Type = CipherType.Login, Data = "{}" }; + sutProvider.GetDependency().GetByIdAsync(cipherId, userId) + .Returns(Task.FromResult(cipherDetails)); + + var responseData = new AttachmentResponseData + { + Id = attachmentId, + Url = "https://example.com/download", + Data = new CipherAttachment.MetaData { FileName = "test.txt" }, + Cipher = cipherDetails, + }; + sutProvider.GetDependency() + .GetAttachmentDownloadDataAsync(cipherDetails, attachmentId) + .Returns(Task.FromResult(responseData)); + + var result = await sutProvider.Sut.GetAttachmentData(cipherId, attachmentId); + + Assert.NotNull(result); + Assert.Equal(attachmentId, result.Id); + } + + [Theory, BitAutoData] + public async Task DownloadAttachmentAsync_EmptyToken_ThrowsNotFoundException( + SutProvider sutProvider) + { + await Assert.ThrowsAsync( + () => sutProvider.Sut.DownloadAttachmentAsync(string.Empty)); + } + + [Theory, BitAutoData] + public async Task DownloadAttachmentAsync_InvalidToken_ThrowsNotFoundException( + SutProvider sutProvider) + { + // Use a real ephemeral data protector - any invalid token will fail to unprotect + var dataProtectionProvider = new EphemeralDataProtectionProvider(); + var protector = dataProtectionProvider + .CreateProtector("AttachmentDownload") + .ToTimeLimitedDataProtector(); + + sutProvider.GetDependency() + .CreateAttachmentDownloadProtector() + .Returns(protector); + + await Assert.ThrowsAsync( + () => sutProvider.Sut.DownloadAttachmentAsync("invalid-token")); + } + + [Theory, BitAutoData] + public async Task DownloadAttachmentAsync_ValidToken_CipherNotFound_ThrowsNotFoundException( + Guid cipherId, string attachmentId, + SutProvider sutProvider) + { + // Create a real token using ephemeral data protection + var dataProtectionProvider = new EphemeralDataProtectionProvider(); + var protector = dataProtectionProvider + .CreateProtector("AttachmentDownload") + .ToTimeLimitedDataProtector(); + var token = protector.Protect($"{cipherId}|{attachmentId}", TimeSpan.FromMinutes(1)); + + sutProvider.GetDependency() + .CreateAttachmentDownloadProtector() + .Returns(protector); + + sutProvider.GetDependency().GetByIdAsync(cipherId).ReturnsNull(); + + await Assert.ThrowsAsync( + () => sutProvider.Sut.DownloadAttachmentAsync(token)); + } + + [Theory, BitAutoData] + public async Task DownloadAttachmentAsync_ValidToken_NoAttachments_ThrowsNotFoundException( + Guid cipherId, string attachmentId, + SutProvider sutProvider) + { + var dataProtectionProvider = new EphemeralDataProtectionProvider(); + var protector = dataProtectionProvider + .CreateProtector("AttachmentDownload") + .ToTimeLimitedDataProtector(); + var token = protector.Protect($"{cipherId}|{attachmentId}", TimeSpan.FromMinutes(1)); + + sutProvider.GetDependency() + .CreateAttachmentDownloadProtector() + .Returns(protector); + + var cipher = new Cipher { Id = cipherId, Attachments = null }; + sutProvider.GetDependency().GetByIdAsync(cipherId).Returns(cipher); + + await Assert.ThrowsAsync( + () => sutProvider.Sut.DownloadAttachmentAsync(token)); + } } diff --git a/test/Core.Test/Vault/Services/CipherServiceTests.cs b/test/Core.Test/Vault/Services/CipherServiceTests.cs index 5fc92a9d3973..6a386c7351e6 100644 --- a/test/Core.Test/Vault/Services/CipherServiceTests.cs +++ b/test/Core.Test/Vault/Services/CipherServiceTests.cs @@ -25,6 +25,7 @@ using Bit.Core.Vault.Services; using Bit.Test.Common.AutoFixture; using Bit.Test.Common.AutoFixture.Attributes; +using Microsoft.AspNetCore.DataProtection; using NSubstitute; using Xunit; @@ -2575,4 +2576,164 @@ private async Task AssertNoActionsAsync(SutProvider sutProvider) await sutProvider.GetDependency().DidNotReceiveWithAnyArgs().LogCipherEventsAsync(default); await sutProvider.GetDependency().DidNotReceiveWithAnyArgs().PushSyncCiphersAsync(default); } + + [Theory, BitAutoData] + public async Task GetAttachmentDownloadDataAsync_NullCipher_ThrowsNotFoundException( + string attachmentId, SutProvider sutProvider) + { + await Assert.ThrowsAsync( + () => sutProvider.Sut.GetAttachmentDownloadDataAsync(null, attachmentId)); + } + + [Theory, BitAutoData] + public async Task GetAttachmentDownloadDataAsync_AttachmentNotFound_ThrowsNotFoundException( + SutProvider sutProvider) + { + var cipher = new Cipher { Id = Guid.NewGuid(), Attachments = null }; + + await Assert.ThrowsAsync( + () => sutProvider.Sut.GetAttachmentDownloadDataAsync(cipher, "nonexistent")); + } + + [Theory, BitAutoData] + public async Task GetAttachmentDownloadDataAsync_AzureStorage_ReturnsOriginalUrl( + SutProvider sutProvider) + { + var cipherId = Guid.NewGuid(); + var attachmentId = Guid.NewGuid().ToString(); + var expectedUrl = $"https://blob.storage/{cipherId}/{attachmentId}?sas=token"; + + var metaData = new CipherAttachment.MetaData + { + AttachmentId = attachmentId, + FileName = "test.txt", + Size = 100, + }; + + var cipher = new Cipher + { + Id = cipherId, + Attachments = System.Text.Json.JsonSerializer.Serialize( + new Dictionary { { attachmentId, metaData } }), + }; + + sutProvider.GetDependency().FileUploadType.Returns(FileUploadType.Azure); + sutProvider.GetDependency() + .GetAttachmentDownloadUrlAsync(cipher, Arg.Any()) + .Returns(expectedUrl); + + var result = await sutProvider.Sut.GetAttachmentDownloadDataAsync(cipher, attachmentId); + + Assert.Equal(expectedUrl, result.Url); + Assert.Equal(attachmentId, result.Id); + } + + [Theory, BitAutoData] + public async Task GetAttachmentDownloadDataAsync_LocalStorage_ReturnsSignedUrl( + SutProvider sutProvider) + { + var cipherId = Guid.NewGuid(); + var attachmentId = Guid.NewGuid().ToString(); + var bareUrl = $"https://localhost/{cipherId}/{attachmentId}"; + var apiBaseUrl = "https://api.example.com"; + + var metaData = new CipherAttachment.MetaData + { + AttachmentId = attachmentId, + FileName = "test.txt", + Size = 100, + }; + + var cipher = new Cipher + { + Id = cipherId, + Attachments = System.Text.Json.JsonSerializer.Serialize( + new Dictionary { { attachmentId, metaData } }), + }; + + sutProvider.GetDependency().FileUploadType.Returns(FileUploadType.Direct); + sutProvider.GetDependency() + .GetAttachmentDownloadUrlAsync(cipher, Arg.Any()) + .Returns(bareUrl); + + // Use real ephemeral data protection provider + var dataProtectionProvider = new EphemeralDataProtectionProvider(); + sutProvider.GetDependency() + .CreateProtector(CipherService.AttachmentDownloadProtectorPurpose) + .Returns(dataProtectionProvider.CreateProtector(CipherService.AttachmentDownloadProtectorPurpose)); + + sutProvider.GetDependency().SelfHosted = true; + sutProvider.GetDependency().BaseServiceUri.Api = apiBaseUrl; + + var result = await sutProvider.Sut.GetAttachmentDownloadDataAsync(cipher, attachmentId); + + Assert.NotEqual(bareUrl, result.Url); + Assert.Contains("ciphers/attachment/download", result.Url); + Assert.Contains("token=", result.Url); + Assert.StartsWith(apiBaseUrl, result.Url); + } + + [Fact] + public void ParseAttachmentDownloadToken_ValidToken_ReturnsCorrectValues() + { + var cipherId = Guid.NewGuid(); + var attachmentId = "test-attachment-id"; + + var dataProtectionProvider = new EphemeralDataProtectionProvider(); + var protector = dataProtectionProvider + .CreateProtector(CipherService.AttachmentDownloadProtectorPurpose) + .ToTimeLimitedDataProtector(); + var token = protector.Protect($"{cipherId}|{attachmentId}", TimeSpan.FromMinutes(1)); + + var (parsedCipherId, parsedAttachmentId) = + CipherService.ParseAttachmentDownloadToken(token, protector); + + Assert.Equal(cipherId, parsedCipherId); + Assert.Equal(attachmentId, parsedAttachmentId); + } + + [Fact] + public void ParseAttachmentDownloadToken_ExpiredToken_ThrowsNotFoundException() + { + var dataProtectionProvider = new EphemeralDataProtectionProvider(); + var protector = dataProtectionProvider + .CreateProtector(CipherService.AttachmentDownloadProtectorPurpose) + .ToTimeLimitedDataProtector(); + + // Create with a different purpose to simulate an invalid/tampered token + var differentProtector = dataProtectionProvider + .CreateProtector("DifferentPurpose") + .ToTimeLimitedDataProtector(); + var token = differentProtector.Protect("data", TimeSpan.FromMinutes(1)); + + Assert.Throws( + () => CipherService.ParseAttachmentDownloadToken(token, protector)); + } + + [Fact] + public void ParseAttachmentDownloadToken_InvalidFormat_ThrowsNotFoundException() + { + var dataProtectionProvider = new EphemeralDataProtectionProvider(); + var protector = dataProtectionProvider + .CreateProtector(CipherService.AttachmentDownloadProtectorPurpose) + .ToTimeLimitedDataProtector(); + // Protect a string without the pipe separator + var token = protector.Protect("invalid-data-without-pipe", TimeSpan.FromMinutes(1)); + + Assert.Throws( + () => CipherService.ParseAttachmentDownloadToken(token, protector)); + } + + [Fact] + public void ParseAttachmentDownloadToken_InvalidGuid_ThrowsNotFoundException() + { + var dataProtectionProvider = new EphemeralDataProtectionProvider(); + var protector = dataProtectionProvider + .CreateProtector(CipherService.AttachmentDownloadProtectorPurpose) + .ToTimeLimitedDataProtector(); + var token = protector.Protect("not-a-guid|attachment-id", TimeSpan.FromMinutes(1)); + + Assert.Throws( + () => CipherService.ParseAttachmentDownloadToken(token, protector)); + } } From d19cda2068cd6386d6c96ff7bcb1457a91b0fc1e Mon Sep 17 00:00:00 2001 From: jaasen-livefront Date: Tue, 3 Mar 2026 14:55:30 -0800 Subject: [PATCH 2/4] move local attachment logic to service --- .../Vault/Controllers/CiphersController.cs | 3 +- .../Services/IAttachmentStorageService.cs | 5 + src/Core/Vault/Services/ICipherService.cs | 3 - .../AzureAttachmentStorageService.cs | 20 +-- .../Services/Implementations/CipherService.cs | 52 +------- .../LocalAttachmentStorageService.cs | 44 ++++++- .../NoopAttachmentStorageService.cs | 5 + .../Controllers/CiphersControllerTests.cs | 43 ++----- .../LocalAttachmentStorageServiceTests.cs | 90 ++++++++++++++ .../Vault/Services/CipherServiceTests.cs | 115 +----------------- 10 files changed, 169 insertions(+), 211 deletions(-) diff --git a/src/Api/Vault/Controllers/CiphersController.cs b/src/Api/Vault/Controllers/CiphersController.cs index d83ad32f9461..03b545efeed5 100644 --- a/src/Api/Vault/Controllers/CiphersController.cs +++ b/src/Api/Vault/Controllers/CiphersController.cs @@ -1521,8 +1521,7 @@ public async Task DownloadAttachmentAsync([FromQuery] string toke throw new NotFoundException(); } - (Guid cipherId, string attachmentId) = CipherService.ParseAttachmentDownloadToken(token, - _cipherService.CreateAttachmentDownloadProtector()); + (Guid cipherId, string attachmentId) = _attachmentStorageService.ParseAttachmentDownloadToken(token); var cipher = await _cipherRepository.GetByIdAsync(cipherId); if (cipher == null) diff --git a/src/Core/Services/IAttachmentStorageService.cs b/src/Core/Services/IAttachmentStorageService.cs index 6702d285e296..1f40437b7f9c 100644 --- a/src/Core/Services/IAttachmentStorageService.cs +++ b/src/Core/Services/IAttachmentStorageService.cs @@ -20,6 +20,11 @@ public interface IAttachmentStorageService Task GetAttachmentUploadUrlAsync(Cipher cipher, CipherAttachment.MetaData attachmentData); Task GetAttachmentDownloadUrlAsync(Cipher cipher, CipherAttachment.MetaData attachmentData); /// + /// Parses and validates a time-limited download token, returning the cipher ID and attachment ID. + /// Only supported by storage implementations that use signed URLs (e.g. local/self-hosted storage). + /// + (Guid cipherId, string attachmentId) ParseAttachmentDownloadToken(string token); + /// /// Opens a read stream for a locally stored attachment file. /// Returns null if the storage implementation does not support direct streaming (e.g. cloud storage). /// diff --git a/src/Core/Vault/Services/ICipherService.cs b/src/Core/Vault/Services/ICipherService.cs index 3803620a9053..da01b55ab174 100644 --- a/src/Core/Vault/Services/ICipherService.cs +++ b/src/Core/Vault/Services/ICipherService.cs @@ -3,8 +3,6 @@ using Bit.Core.Vault.Entities; using Bit.Core.Vault.Models.Data; -using Microsoft.AspNetCore.DataProtection; - namespace Bit.Core.Vault.Services; public interface ICipherService @@ -37,7 +35,6 @@ Task> ShareManyAsync(IEnumerable<(CipherDetails ciphe Task> RestoreManyAsync(IEnumerable cipherIds, Guid restoringUserId, Guid? organizationId = null, bool orgAdmin = false); Task UploadFileForExistingAttachmentAsync(Stream stream, Cipher cipher, CipherAttachment.MetaData attachmentId); Task GetAttachmentDownloadDataAsync(Cipher cipher, string attachmentId); - ITimeLimitedDataProtector CreateAttachmentDownloadProtector(); Task ValidateCipherAttachmentFile(Cipher cipher, CipherAttachment.MetaData attachmentData); Task ValidateBulkCollectionAssignmentAsync(IEnumerable collectionIds, IEnumerable cipherIds, Guid userId); } diff --git a/src/Core/Vault/Services/Implementations/AzureAttachmentStorageService.cs b/src/Core/Vault/Services/Implementations/AzureAttachmentStorageService.cs index 3747c9ed8fa7..ed5d47e52696 100644 --- a/src/Core/Vault/Services/Implementations/AzureAttachmentStorageService.cs +++ b/src/Core/Vault/Services/Implementations/AzureAttachmentStorageService.cs @@ -1,7 +1,4 @@ -// FIXME: Update this file to be null safe and then delete the line below -#nullable disable - -using Azure.Storage.Blobs; +using Azure.Storage.Blobs; using Azure.Storage.Blobs.Models; using Azure.Storage.Sas; using Bit.Core.Enums; @@ -32,7 +29,7 @@ private string BlobName(Guid cipherId, CipherAttachment.MetaData attachmentData, attachmentData.AttachmentId ); - public static (string cipherId, string organizationId, string attachmentId) IdentifiersFromBlobName(string blobName) + public static (string cipherId, string? organizationId, string attachmentId) IdentifiersFromBlobName(string blobName) { var parts = blobName.Split('/'); switch (parts.Length) @@ -71,6 +68,11 @@ public async Task GetAttachmentDownloadUrlAsync(Cipher cipher, CipherAtt return sasUri.ToString(); } + public (Guid cipherId, string attachmentId) ParseAttachmentDownloadToken(string token) + { + throw new NotSupportedException("Token-based downloads are not supported with Azure storage."); + } + public async Task GetAttachmentUploadUrlAsync(Cipher cipher, CipherAttachment.MetaData attachmentData) { await InitAsync(EventGridEnabledContainerName); @@ -94,7 +96,7 @@ public async Task UploadNewAttachmentAsync(Stream stream, Cipher cipher, CipherA } else { - metadata.Add("organizationId", cipher.OrganizationId.Value.ToString()); + metadata.Add("organizationId", cipher.OrganizationId!.Value.ToString()); } var headers = new BlobHttpHeaders @@ -193,10 +195,10 @@ public async Task DeleteAttachmentsForUserAsync(Guid userId) await InitAsync(_defaultContainerName); } - public Task GetAttachmentReadStreamAsync(Cipher cipher, CipherAttachment.MetaData attachmentData) + public Task GetAttachmentReadStreamAsync(Cipher cipher, CipherAttachment.MetaData attachmentData) { // Azure storage uses SAS URLs for downloads; direct streaming is not supported. - return Task.FromResult(null); + return Task.FromResult(null); } public async Task<(bool, long?)> ValidateFileAsync(Cipher cipher, CipherAttachment.MetaData attachmentData, long leeway) @@ -217,7 +219,7 @@ public Task GetAttachmentReadStreamAsync(Cipher cipher, CipherAttachment } else { - metadata["organizationId"] = cipher.OrganizationId.Value.ToString(); + metadata["organizationId"] = cipher.OrganizationId!.Value.ToString(); } await blobClient.SetMetadataAsync(metadata); diff --git a/src/Core/Vault/Services/Implementations/CipherService.cs b/src/Core/Vault/Services/Implementations/CipherService.cs index 857b06d8d4b8..5f235879c676 100644 --- a/src/Core/Vault/Services/Implementations/CipherService.cs +++ b/src/Core/Vault/Services/Implementations/CipherService.cs @@ -21,8 +21,6 @@ using Bit.Core.Vault.Models.Data; using Bit.Core.Vault.Queries; using Bit.Core.Vault.Repositories; -using Microsoft.AspNetCore.DataProtection; - namespace Bit.Core.Vault.Services; public class CipherService : ICipherService @@ -49,10 +47,6 @@ public class CipherService : ICipherService private readonly IApplicationCacheService _applicationCacheService; private readonly IFeatureService _featureService; private readonly IPricingClient _pricingClient; - private readonly IDataProtectionProvider _dataProtectionProvider; - - internal static readonly string AttachmentDownloadProtectorPurpose = "AttachmentDownload"; - private static readonly TimeSpan _downloadLinkLifetime = TimeSpan.FromMinutes(1); public CipherService( ICipherRepository cipherRepository, @@ -73,8 +67,7 @@ public CipherService( IPolicyRequirementQuery policyRequirementQuery, IApplicationCacheService applicationCacheService, IFeatureService featureService, - IPricingClient pricingClient, - IDataProtectionProvider dataProtectionProvider) + IPricingClient pricingClient) { _cipherRepository = cipherRepository; _folderRepository = folderRepository; @@ -95,7 +88,6 @@ public CipherService( _applicationCacheService = applicationCacheService; _featureService = featureService; _pricingClient = pricingClient; - _dataProtectionProvider = dataProtectionProvider; } public async Task SaveAsync(Cipher cipher, Guid savingUserId, DateTime? lastKnownRevisionDate, @@ -421,19 +413,6 @@ public async Task GetAttachmentDownloadDataAsync(Cipher var url = await _attachmentStorageService.GetAttachmentDownloadUrlAsync(cipher, data); - // For local (self-hosted) storage, generate a time-limited signed download URL - // to prevent unauthenticated access to predictable attachment file paths. - // Cloud storage (Azure) already uses time-limited SAS URLs. - if (_attachmentStorageService.FileUploadType == FileUploadType.Direct) - { - var protector = _dataProtectionProvider.CreateProtector(AttachmentDownloadProtectorPurpose); - var timedProtector = protector.ToTimeLimitedDataProtector(); - var token = timedProtector.Protect( - $"{cipher.Id}|{attachmentId}", - _downloadLinkLifetime); - url = $"{_globalSettings.BaseServiceUri.Api}/ciphers/attachment/download?token={Uri.EscapeDataString(token)}"; - } - var response = new AttachmentResponseData { Cipher = cipher, @@ -445,35 +424,6 @@ public async Task GetAttachmentDownloadDataAsync(Cipher return response; } - public ITimeLimitedDataProtector CreateAttachmentDownloadProtector() - { - return _dataProtectionProvider - .CreateProtector(AttachmentDownloadProtectorPurpose) - .ToTimeLimitedDataProtector(); - } - - public static (Guid cipherId, string attachmentId) ParseAttachmentDownloadToken( - string token, ITimeLimitedDataProtector protector) - { - string payload; - try - { - payload = protector.Unprotect(token); - } - catch - { - throw new NotFoundException(); - } - - var parts = payload.Split('|'); - if (parts.Length != 2 || !Guid.TryParse(parts[0], out var cipherId)) - { - throw new NotFoundException(); - } - - return (cipherId, parts[1]); - } - public async Task DeleteAsync(CipherDetails cipherDetails, Guid deletingUserId, bool orgAdmin = false) { if (!orgAdmin && !await UserCanDeleteAsync(cipherDetails, deletingUserId)) diff --git a/src/Core/Vault/Services/Implementations/LocalAttachmentStorageService.cs b/src/Core/Vault/Services/Implementations/LocalAttachmentStorageService.cs index 61b7c54e450b..cf3ce35fc8dc 100644 --- a/src/Core/Vault/Services/Implementations/LocalAttachmentStorageService.cs +++ b/src/Core/Vault/Services/Implementations/LocalAttachmentStorageService.cs @@ -1,8 +1,10 @@ using Bit.Core.Enums; +using Bit.Core.Exceptions; using Bit.Core.Services; using Bit.Core.Settings; using Bit.Core.Vault.Entities; using Bit.Core.Vault.Models.Data; +using Microsoft.AspNetCore.DataProtection; namespace Bit.Core.Vault.Services; @@ -11,21 +13,59 @@ public class LocalAttachmentStorageService : IAttachmentStorageService private readonly string _baseAttachmentUrl; private readonly string _baseDirPath; private readonly string _baseTempDirPath; + private readonly IDataProtectionProvider _dataProtectionProvider; + private readonly string _apiBaseUrl; + + internal static readonly string AttachmentDownloadProtectorPurpose = "AttachmentDownload"; + private static readonly TimeSpan _downloadLinkLifetime = TimeSpan.FromMinutes(1); public FileUploadType FileUploadType => FileUploadType.Direct; public LocalAttachmentStorageService( - IGlobalSettings globalSettings) + IGlobalSettings globalSettings, + IDataProtectionProvider dataProtectionProvider) { _baseDirPath = globalSettings.Attachment.BaseDirectory; _baseTempDirPath = $"{_baseDirPath}/temp"; _baseAttachmentUrl = globalSettings.Attachment.BaseUrl; + _dataProtectionProvider = dataProtectionProvider; + _apiBaseUrl = globalSettings.BaseServiceUri.Api; } public async Task GetAttachmentDownloadUrlAsync(Cipher cipher, CipherAttachment.MetaData attachmentData) { await InitAsync(); - return $"{_baseAttachmentUrl}/{cipher.Id}/{attachmentData.AttachmentId}"; + var protector = _dataProtectionProvider.CreateProtector(AttachmentDownloadProtectorPurpose); + var timedProtector = protector.ToTimeLimitedDataProtector(); + var token = timedProtector.Protect( + $"{cipher.Id}|{attachmentData.AttachmentId}", + _downloadLinkLifetime); + return $"{_apiBaseUrl}/ciphers/attachment/download?token={Uri.EscapeDataString(token)}"; + } + + public (Guid cipherId, string attachmentId) ParseAttachmentDownloadToken(string token) + { + var protector = _dataProtectionProvider + .CreateProtector(AttachmentDownloadProtectorPurpose) + .ToTimeLimitedDataProtector(); + + string payload; + try + { + payload = protector.Unprotect(token); + } + catch + { + throw new NotFoundException(); + } + + var parts = payload.Split('|'); + if (parts.Length != 2 || !Guid.TryParse(parts[0], out var cipherId)) + { + throw new NotFoundException(); + } + + return (cipherId, parts[1]); } public async Task UploadNewAttachmentAsync(Stream stream, Cipher cipher, CipherAttachment.MetaData attachmentData) diff --git a/src/Core/Vault/Services/NoopImplementations/NoopAttachmentStorageService.cs b/src/Core/Vault/Services/NoopImplementations/NoopAttachmentStorageService.cs index fd5c935b36f0..22b3d0677bae 100644 --- a/src/Core/Vault/Services/NoopImplementations/NoopAttachmentStorageService.cs +++ b/src/Core/Vault/Services/NoopImplementations/NoopAttachmentStorageService.cs @@ -62,6 +62,11 @@ public Task GetAttachmentDownloadUrlAsync(Cipher cipher, CipherAttachmen return Task.FromResult((string)null); } + public (Guid cipherId, string attachmentId) ParseAttachmentDownloadToken(string token) + { + throw new NotSupportedException("Token-based downloads are not supported with noop storage."); + } + public Task GetAttachmentReadStreamAsync(Cipher cipher, CipherAttachment.MetaData attachmentData) { return Task.FromResult(null); diff --git a/test/Api.Test/Vault/Controllers/CiphersControllerTests.cs b/test/Api.Test/Vault/Controllers/CiphersControllerTests.cs index 7ba01d36345b..a583f25b2ad4 100644 --- a/test/Api.Test/Vault/Controllers/CiphersControllerTests.cs +++ b/test/Api.Test/Vault/Controllers/CiphersControllerTests.cs @@ -18,8 +18,8 @@ using Bit.Core.Vault.Services; using Bit.Test.Common.AutoFixture; using Bit.Test.Common.AutoFixture.Attributes; -using Microsoft.AspNetCore.DataProtection; using NSubstitute; +using NSubstitute.ExceptionExtensions; using NSubstitute.ReturnsExtensions; using Xunit; using CipherType = Bit.Core.Vault.Enums.CipherType; @@ -2206,15 +2206,9 @@ await Assert.ThrowsAsync( public async Task DownloadAttachmentAsync_InvalidToken_ThrowsNotFoundException( SutProvider sutProvider) { - // Use a real ephemeral data protector - any invalid token will fail to unprotect - var dataProtectionProvider = new EphemeralDataProtectionProvider(); - var protector = dataProtectionProvider - .CreateProtector("AttachmentDownload") - .ToTimeLimitedDataProtector(); - - sutProvider.GetDependency() - .CreateAttachmentDownloadProtector() - .Returns(protector); + sutProvider.GetDependency() + .ParseAttachmentDownloadToken(Arg.Any()) + .Throws(new NotFoundException()); await Assert.ThrowsAsync( () => sutProvider.Sut.DownloadAttachmentAsync("invalid-token")); @@ -2225,21 +2219,14 @@ public async Task DownloadAttachmentAsync_ValidToken_CipherNotFound_ThrowsNotFou Guid cipherId, string attachmentId, SutProvider sutProvider) { - // Create a real token using ephemeral data protection - var dataProtectionProvider = new EphemeralDataProtectionProvider(); - var protector = dataProtectionProvider - .CreateProtector("AttachmentDownload") - .ToTimeLimitedDataProtector(); - var token = protector.Protect($"{cipherId}|{attachmentId}", TimeSpan.FromMinutes(1)); - - sutProvider.GetDependency() - .CreateAttachmentDownloadProtector() - .Returns(protector); + sutProvider.GetDependency() + .ParseAttachmentDownloadToken(Arg.Any()) + .Returns((cipherId, attachmentId)); sutProvider.GetDependency().GetByIdAsync(cipherId).ReturnsNull(); await Assert.ThrowsAsync( - () => sutProvider.Sut.DownloadAttachmentAsync(token)); + () => sutProvider.Sut.DownloadAttachmentAsync("some-token")); } [Theory, BitAutoData] @@ -2247,20 +2234,14 @@ public async Task DownloadAttachmentAsync_ValidToken_NoAttachments_ThrowsNotFoun Guid cipherId, string attachmentId, SutProvider sutProvider) { - var dataProtectionProvider = new EphemeralDataProtectionProvider(); - var protector = dataProtectionProvider - .CreateProtector("AttachmentDownload") - .ToTimeLimitedDataProtector(); - var token = protector.Protect($"{cipherId}|{attachmentId}", TimeSpan.FromMinutes(1)); - - sutProvider.GetDependency() - .CreateAttachmentDownloadProtector() - .Returns(protector); + sutProvider.GetDependency() + .ParseAttachmentDownloadToken(Arg.Any()) + .Returns((cipherId, attachmentId)); var cipher = new Cipher { Id = cipherId, Attachments = null }; sutProvider.GetDependency().GetByIdAsync(cipherId).Returns(cipher); await Assert.ThrowsAsync( - () => sutProvider.Sut.DownloadAttachmentAsync(token)); + () => sutProvider.Sut.DownloadAttachmentAsync("some-token")); } } diff --git a/test/Core.Test/Services/LocalAttachmentStorageServiceTests.cs b/test/Core.Test/Services/LocalAttachmentStorageServiceTests.cs index 98681a19a03e..4b2f64cfb255 100644 --- a/test/Core.Test/Services/LocalAttachmentStorageServiceTests.cs +++ b/test/Core.Test/Services/LocalAttachmentStorageServiceTests.cs @@ -1,5 +1,6 @@ using System.Text; using AutoFixture; +using Bit.Core.Exceptions; using Bit.Core.Settings; using Bit.Core.Test.AutoFixture.CipherAttachmentMetaData; using Bit.Core.Test.AutoFixture.CipherFixtures; @@ -8,6 +9,7 @@ using Bit.Core.Vault.Services; using Bit.Test.Common.AutoFixture; using Bit.Test.Common.AutoFixture.Attributes; +using Microsoft.AspNetCore.DataProtection; using NSubstitute; using Xunit; @@ -233,7 +235,95 @@ private SutProvider GetSutProvider(TempDirectory var fixture = new Fixture().WithAutoNSubstitutions(); fixture.Freeze().Attachment.BaseDirectory.Returns(tempDirectory.Directory); fixture.Freeze().Attachment.BaseUrl.Returns(Guid.NewGuid().ToString()); + fixture.Freeze().BaseServiceUri.Api.Returns("https://api.example.com"); + fixture.Register(() => new EphemeralDataProtectionProvider()); return new SutProvider(fixture).Create(); } + + [Theory] + [InlineCustomAutoData(new[] { typeof(UserCipher), typeof(MetaData) })] + public async Task GetAttachmentDownloadUrlAsync_ReturnsSignedUrl(Cipher cipher, CipherAttachment.MetaData attachmentData) + { + using (var tempDirectory = new TempDirectory()) + { + var sutProvider = GetSutProvider(tempDirectory); + + var url = await sutProvider.Sut.GetAttachmentDownloadUrlAsync(cipher, attachmentData); + + Assert.Contains("ciphers/attachment/download", url); + Assert.Contains("token=", url); + Assert.StartsWith("https://api.example.com", url); + } + } + + [Theory] + [InlineCustomAutoData(new[] { typeof(UserCipher), typeof(MetaData) })] + public async Task GetAttachmentDownloadUrlAsync_TokenCanBeParsedBack(Cipher cipher, CipherAttachment.MetaData attachmentData) + { + using (var tempDirectory = new TempDirectory()) + { + var sutProvider = GetSutProvider(tempDirectory); + + var url = await sutProvider.Sut.GetAttachmentDownloadUrlAsync(cipher, attachmentData); + + // Extract token from URL + var uri = new Uri(url); + var query = System.Web.HttpUtility.ParseQueryString(uri.Query); + var token = query["token"]; + + var (parsedCipherId, parsedAttachmentId) = sutProvider.Sut.ParseAttachmentDownloadToken(token); + + Assert.Equal(cipher.Id, parsedCipherId); + Assert.Equal(attachmentData.AttachmentId, parsedAttachmentId); + } + } + + [Fact] + public void ParseAttachmentDownloadToken_InvalidToken_ThrowsNotFoundException() + { + using (var tempDirectory = new TempDirectory()) + { + var sutProvider = GetSutProvider(tempDirectory); + + Assert.Throws( + () => sutProvider.Sut.ParseAttachmentDownloadToken("invalid-token")); + } + } + + [Fact] + public void ParseAttachmentDownloadToken_InvalidFormat_ThrowsNotFoundException() + { + using (var tempDirectory = new TempDirectory()) + { + var sutProvider = GetSutProvider(tempDirectory); + + // Create a valid token but with invalid payload format (no pipe separator) + var provider = new EphemeralDataProtectionProvider(); + var protector = provider + .CreateProtector(LocalAttachmentStorageService.AttachmentDownloadProtectorPurpose) + .ToTimeLimitedDataProtector(); + var token = protector.Protect("invalid-data-without-pipe", TimeSpan.FromMinutes(1)); + + // This will fail because the SUT uses its own DataProtection instance + // which cannot unprotect tokens from a different provider + Assert.Throws( + () => sutProvider.Sut.ParseAttachmentDownloadToken(token)); + } + } + + [Fact] + public void ParseAttachmentDownloadToken_InvalidGuid_ThrowsNotFoundException() + { + using (var tempDirectory = new TempDirectory()) + { + var sutProvider = GetSutProvider(tempDirectory); + + // We need to use the same DataProtection provider as the SUT + // Generate a token via GetAttachmentDownloadUrlAsync with a known cipher, + // then tamper with the concept. Instead, just test with a completely invalid token. + Assert.Throws( + () => sutProvider.Sut.ParseAttachmentDownloadToken("not-a-real-token")); + } + } } diff --git a/test/Core.Test/Vault/Services/CipherServiceTests.cs b/test/Core.Test/Vault/Services/CipherServiceTests.cs index 6a386c7351e6..56430d7d7320 100644 --- a/test/Core.Test/Vault/Services/CipherServiceTests.cs +++ b/test/Core.Test/Vault/Services/CipherServiceTests.cs @@ -25,7 +25,6 @@ using Bit.Core.Vault.Services; using Bit.Test.Common.AutoFixture; using Bit.Test.Common.AutoFixture.Attributes; -using Microsoft.AspNetCore.DataProtection; using NSubstitute; using Xunit; @@ -2596,12 +2595,12 @@ await Assert.ThrowsAsync( } [Theory, BitAutoData] - public async Task GetAttachmentDownloadDataAsync_AzureStorage_ReturnsOriginalUrl( + public async Task GetAttachmentDownloadDataAsync_ReturnsUrlFromStorageService( SutProvider sutProvider) { var cipherId = Guid.NewGuid(); var attachmentId = Guid.NewGuid().ToString(); - var expectedUrl = $"https://blob.storage/{cipherId}/{attachmentId}?sas=token"; + var expectedUrl = "https://example.com/download?token=abc"; var metaData = new CipherAttachment.MetaData { @@ -2617,7 +2616,6 @@ public async Task GetAttachmentDownloadDataAsync_AzureStorage_ReturnsOriginalUrl new Dictionary { { attachmentId, metaData } }), }; - sutProvider.GetDependency().FileUploadType.Returns(FileUploadType.Azure); sutProvider.GetDependency() .GetAttachmentDownloadUrlAsync(cipher, Arg.Any()) .Returns(expectedUrl); @@ -2627,113 +2625,4 @@ public async Task GetAttachmentDownloadDataAsync_AzureStorage_ReturnsOriginalUrl Assert.Equal(expectedUrl, result.Url); Assert.Equal(attachmentId, result.Id); } - - [Theory, BitAutoData] - public async Task GetAttachmentDownloadDataAsync_LocalStorage_ReturnsSignedUrl( - SutProvider sutProvider) - { - var cipherId = Guid.NewGuid(); - var attachmentId = Guid.NewGuid().ToString(); - var bareUrl = $"https://localhost/{cipherId}/{attachmentId}"; - var apiBaseUrl = "https://api.example.com"; - - var metaData = new CipherAttachment.MetaData - { - AttachmentId = attachmentId, - FileName = "test.txt", - Size = 100, - }; - - var cipher = new Cipher - { - Id = cipherId, - Attachments = System.Text.Json.JsonSerializer.Serialize( - new Dictionary { { attachmentId, metaData } }), - }; - - sutProvider.GetDependency().FileUploadType.Returns(FileUploadType.Direct); - sutProvider.GetDependency() - .GetAttachmentDownloadUrlAsync(cipher, Arg.Any()) - .Returns(bareUrl); - - // Use real ephemeral data protection provider - var dataProtectionProvider = new EphemeralDataProtectionProvider(); - sutProvider.GetDependency() - .CreateProtector(CipherService.AttachmentDownloadProtectorPurpose) - .Returns(dataProtectionProvider.CreateProtector(CipherService.AttachmentDownloadProtectorPurpose)); - - sutProvider.GetDependency().SelfHosted = true; - sutProvider.GetDependency().BaseServiceUri.Api = apiBaseUrl; - - var result = await sutProvider.Sut.GetAttachmentDownloadDataAsync(cipher, attachmentId); - - Assert.NotEqual(bareUrl, result.Url); - Assert.Contains("ciphers/attachment/download", result.Url); - Assert.Contains("token=", result.Url); - Assert.StartsWith(apiBaseUrl, result.Url); - } - - [Fact] - public void ParseAttachmentDownloadToken_ValidToken_ReturnsCorrectValues() - { - var cipherId = Guid.NewGuid(); - var attachmentId = "test-attachment-id"; - - var dataProtectionProvider = new EphemeralDataProtectionProvider(); - var protector = dataProtectionProvider - .CreateProtector(CipherService.AttachmentDownloadProtectorPurpose) - .ToTimeLimitedDataProtector(); - var token = protector.Protect($"{cipherId}|{attachmentId}", TimeSpan.FromMinutes(1)); - - var (parsedCipherId, parsedAttachmentId) = - CipherService.ParseAttachmentDownloadToken(token, protector); - - Assert.Equal(cipherId, parsedCipherId); - Assert.Equal(attachmentId, parsedAttachmentId); - } - - [Fact] - public void ParseAttachmentDownloadToken_ExpiredToken_ThrowsNotFoundException() - { - var dataProtectionProvider = new EphemeralDataProtectionProvider(); - var protector = dataProtectionProvider - .CreateProtector(CipherService.AttachmentDownloadProtectorPurpose) - .ToTimeLimitedDataProtector(); - - // Create with a different purpose to simulate an invalid/tampered token - var differentProtector = dataProtectionProvider - .CreateProtector("DifferentPurpose") - .ToTimeLimitedDataProtector(); - var token = differentProtector.Protect("data", TimeSpan.FromMinutes(1)); - - Assert.Throws( - () => CipherService.ParseAttachmentDownloadToken(token, protector)); - } - - [Fact] - public void ParseAttachmentDownloadToken_InvalidFormat_ThrowsNotFoundException() - { - var dataProtectionProvider = new EphemeralDataProtectionProvider(); - var protector = dataProtectionProvider - .CreateProtector(CipherService.AttachmentDownloadProtectorPurpose) - .ToTimeLimitedDataProtector(); - // Protect a string without the pipe separator - var token = protector.Protect("invalid-data-without-pipe", TimeSpan.FromMinutes(1)); - - Assert.Throws( - () => CipherService.ParseAttachmentDownloadToken(token, protector)); - } - - [Fact] - public void ParseAttachmentDownloadToken_InvalidGuid_ThrowsNotFoundException() - { - var dataProtectionProvider = new EphemeralDataProtectionProvider(); - var protector = dataProtectionProvider - .CreateProtector(CipherService.AttachmentDownloadProtectorPurpose) - .ToTimeLimitedDataProtector(); - var token = protector.Protect("not-a-guid|attachment-id", TimeSpan.FromMinutes(1)); - - Assert.Throws( - () => CipherService.ParseAttachmentDownloadToken(token, protector)); - } } From 62e31c7abaa41a84882e0b370e0b7cbf5928428b Mon Sep 17 00:00:00 2001 From: jaasen-livefront Date: Tue, 3 Mar 2026 15:08:25 -0800 Subject: [PATCH 3/4] remove comment --- .../Core.Test/Services/LocalAttachmentStorageServiceTests.cs | 5 ----- 1 file changed, 5 deletions(-) diff --git a/test/Core.Test/Services/LocalAttachmentStorageServiceTests.cs b/test/Core.Test/Services/LocalAttachmentStorageServiceTests.cs index 4b2f64cfb255..2a98b31a1fb7 100644 --- a/test/Core.Test/Services/LocalAttachmentStorageServiceTests.cs +++ b/test/Core.Test/Services/LocalAttachmentStorageServiceTests.cs @@ -305,8 +305,6 @@ public void ParseAttachmentDownloadToken_InvalidFormat_ThrowsNotFoundException() .ToTimeLimitedDataProtector(); var token = protector.Protect("invalid-data-without-pipe", TimeSpan.FromMinutes(1)); - // This will fail because the SUT uses its own DataProtection instance - // which cannot unprotect tokens from a different provider Assert.Throws( () => sutProvider.Sut.ParseAttachmentDownloadToken(token)); } @@ -319,9 +317,6 @@ public void ParseAttachmentDownloadToken_InvalidGuid_ThrowsNotFoundException() { var sutProvider = GetSutProvider(tempDirectory); - // We need to use the same DataProtection provider as the SUT - // Generate a token via GetAttachmentDownloadUrlAsync with a known cipher, - // then tamper with the concept. Instead, just test with a completely invalid token. Assert.Throws( () => sutProvider.Sut.ParseAttachmentDownloadToken("not-a-real-token")); } From d33ad0b0295a9523dcfbd808d866f901b1c55f2a Mon Sep 17 00:00:00 2001 From: jaasen-livefront Date: Tue, 3 Mar 2026 15:22:19 -0800 Subject: [PATCH 4/4] remove unusued var. add happy-path test for file download --- .../LocalAttachmentStorageService.cs | 2 - .../Controllers/CiphersControllerTests.cs | 44 +++++++++++++++++++ 2 files changed, 44 insertions(+), 2 deletions(-) diff --git a/src/Core/Vault/Services/Implementations/LocalAttachmentStorageService.cs b/src/Core/Vault/Services/Implementations/LocalAttachmentStorageService.cs index cf3ce35fc8dc..f5894b5f35e0 100644 --- a/src/Core/Vault/Services/Implementations/LocalAttachmentStorageService.cs +++ b/src/Core/Vault/Services/Implementations/LocalAttachmentStorageService.cs @@ -10,7 +10,6 @@ namespace Bit.Core.Vault.Services; public class LocalAttachmentStorageService : IAttachmentStorageService { - private readonly string _baseAttachmentUrl; private readonly string _baseDirPath; private readonly string _baseTempDirPath; private readonly IDataProtectionProvider _dataProtectionProvider; @@ -27,7 +26,6 @@ public LocalAttachmentStorageService( { _baseDirPath = globalSettings.Attachment.BaseDirectory; _baseTempDirPath = $"{_baseDirPath}/temp"; - _baseAttachmentUrl = globalSettings.Attachment.BaseUrl; _dataProtectionProvider = dataProtectionProvider; _apiBaseUrl = globalSettings.BaseServiceUri.Api; } diff --git a/test/Api.Test/Vault/Controllers/CiphersControllerTests.cs b/test/Api.Test/Vault/Controllers/CiphersControllerTests.cs index a583f25b2ad4..80a4e3571f29 100644 --- a/test/Api.Test/Vault/Controllers/CiphersControllerTests.cs +++ b/test/Api.Test/Vault/Controllers/CiphersControllerTests.cs @@ -18,6 +18,7 @@ using Bit.Core.Vault.Services; using Bit.Test.Common.AutoFixture; using Bit.Test.Common.AutoFixture.Attributes; +using Microsoft.AspNetCore.Mvc; using NSubstitute; using NSubstitute.ExceptionExtensions; using NSubstitute.ReturnsExtensions; @@ -2244,4 +2245,47 @@ public async Task DownloadAttachmentAsync_ValidToken_NoAttachments_ThrowsNotFoun await Assert.ThrowsAsync( () => sutProvider.Sut.DownloadAttachmentAsync("some-token")); } + + [Theory, BitAutoData] + public async Task DownloadAttachmentAsync_ValidToken_ReturnsFile( + Guid cipherId, string attachmentId, + SutProvider sutProvider) + { + var fileName = "secret-document.txt"; + var fileContent = new byte[] { 1, 2, 3 }; + var stream = new MemoryStream(fileContent); + + var metaData = new CipherAttachment.MetaData + { + AttachmentId = attachmentId, + FileName = fileName, + Size = fileContent.Length, + }; + + var cipher = new Cipher + { + Id = cipherId, + Attachments = JsonSerializer.Serialize( + new Dictionary { { attachmentId, metaData } }), + }; + + sutProvider.GetDependency() + .ParseAttachmentDownloadToken(Arg.Any()) + .Returns((cipherId, attachmentId)); + + sutProvider.GetDependency() + .GetByIdAsync(cipherId) + .Returns(cipher); + + sutProvider.GetDependency() + .GetAttachmentReadStreamAsync(cipher, Arg.Any()) + .Returns(stream); + + var result = await sutProvider.Sut.DownloadAttachmentAsync("valid-token"); + + var fileResult = Assert.IsType(result); + Assert.Equal("application/octet-stream", fileResult.ContentType); + Assert.Equal(fileName, fileResult.FileDownloadName); + Assert.Same(stream, fileResult.FileStream); + } }