From 1cbb9c36d391936166bd85c8c6f7a8ece0674e6b Mon Sep 17 00:00:00 2001 From: Ike Kottlowski Date: Fri, 6 Mar 2026 12:10:56 -0500 Subject: [PATCH 1/7] feat: add enumeration protection to email protected sends --- .../SendEmailOtpRequestValidator.cs | 46 +++++++++---------- 1 file changed, 21 insertions(+), 25 deletions(-) diff --git a/src/Identity/IdentityServer/RequestValidators/SendAccess/SendEmailOtpRequestValidator.cs b/src/Identity/IdentityServer/RequestValidators/SendAccess/SendEmailOtpRequestValidator.cs index 02442d8c7e0b..a73d9d0f14ac 100644 --- a/src/Identity/IdentityServer/RequestValidators/SendAccess/SendEmailOtpRequestValidator.cs +++ b/src/Identity/IdentityServer/RequestValidators/SendAccess/SendEmailOtpRequestValidator.cs @@ -1,4 +1,5 @@ -using System.Security.Claims; +using System.Globalization; +using System.Security.Claims; using Bit.Core.Auth.Identity; using Bit.Core.Auth.Identity.TokenProviders; using Bit.Core.Services; @@ -9,7 +10,12 @@ namespace Bit.Identity.IdentityServer.RequestValidators.SendAccess; +/** +* The error responses here do not fully match the standard for OAuth with respect to Invalid Request vs Invalid Grant. This is intended to better protect +* against enumeration. We return Invalid Request for all errors related to the email and OTP, even if in some cases Invalid Grant might be more appropriate. +*/ public class SendEmailOtpRequestValidator( + ILogger logger, IOtpTokenProvider otpTokenProvider, IMailService mailService) : ISendAuthenticationMethodValidator { @@ -20,8 +26,7 @@ public class SendEmailOtpRequestValidator( private static readonly Dictionary _sendEmailOtpValidatorErrorDescriptions = new() { { SendAccessConstants.EmailOtpValidatorResults.EmailRequired, $"{SendAccessConstants.TokenRequest.Email} is required." }, - { SendAccessConstants.EmailOtpValidatorResults.EmailAndOtpRequired, $"{SendAccessConstants.TokenRequest.Email} and {SendAccessConstants.TokenRequest.Otp} are required." }, - { SendAccessConstants.EmailOtpValidatorResults.EmailOtpInvalid, $"{SendAccessConstants.TokenRequest.Email} otp is invalid." }, + { SendAccessConstants.EmailOtpValidatorResults.EmailAndOtpRequired, $"{SendAccessConstants.TokenRequest.Email} and {SendAccessConstants.TokenRequest.Otp} are required." } }; public async Task ValidateRequestAsync(ExtensionGrantValidationContext context, EmailOtp authMethod, Guid sendId) @@ -37,21 +42,14 @@ public async Task ValidateRequestAsync(ExtensionGrantVali return BuildErrorResult(SendAccessConstants.EmailOtpValidatorResults.EmailRequired); } - /* - * This is somewhat contradictory to our process where a poor shape means invalid_request and invalid - * data is invalid_grant. - * In this case the shape is correct and the data is invalid but to protect against enumeration we treat incorrect emails - * as invalid requests. The response for a request with a correct email which needs an OTP and a request - * that has an invalid email need to be the same otherwise an attacker could enumerate until a valid email is found. - */ if (!authMethod.emails.Contains(email, StringComparer.OrdinalIgnoreCase)) { - return BuildErrorResult(SendAccessConstants.EmailOtpValidatorResults.EmailAndOtpRequired); + return BuildErrorResult(); } // get otp from request var requestOtp = request.Get(SendAccessConstants.TokenRequest.Otp); - var uniqueIdentifierForTokenCache = string.Format(SendAccessConstants.OtpToken.TokenUniqueIdentifier, sendId, email); + var uniqueIdentifierForTokenCache = string.Format(CultureInfo.InvariantCulture, SendAccessConstants.OtpToken.TokenUniqueIdentifier, sendId, email); if (string.IsNullOrEmpty(requestOtp)) { // Since the request doesn't have an OTP, generate one @@ -63,15 +61,16 @@ public async Task ValidateRequestAsync(ExtensionGrantVali // Verify that the OTP is generated if (string.IsNullOrEmpty(token)) { - return BuildErrorResult(SendAccessConstants.EmailOtpValidatorResults.OtpGenerationFailed); + logger.LogWarning("Failed to generate OTP for sendId: {SendId}", sendId); + return BuildErrorResult(); } await mailService.SendSendEmailOtpEmailAsync( email, token, - string.Format(SendAccessConstants.OtpEmail.Subject, token)); + string.Format(CultureInfo.CurrentCulture, SendAccessConstants.OtpEmail.Subject, token)); - return BuildErrorResult(SendAccessConstants.EmailOtpValidatorResults.EmailAndOtpRequired); + return BuildErrorResult(); } // validate request otp @@ -84,13 +83,18 @@ await mailService.SendSendEmailOtpEmailAsync( // If OTP is invalid return error result if (!otpResult) { - return BuildErrorResult(SendAccessConstants.EmailOtpValidatorResults.EmailOtpInvalid); + return BuildErrorResult(); } return BuildSuccessResult(sendId, email!); } - private static GrantValidationResult BuildErrorResult(string error) + /// + /// Build the error response for the SendEmailOtpRequestValidator. + /// + /// The error code to use for the validation result. This is defaulted to EmailAndOtpRequired if not specified because it is the most common response. + /// A GrantValidationResult representing the error. + private static GrantValidationResult BuildErrorResult(string error = SendAccessConstants.EmailOtpValidatorResults.EmailAndOtpRequired) { switch (error) { @@ -102,14 +106,6 @@ private static GrantValidationResult BuildErrorResult(string error) { { SendAccessConstants.SendAccessError, error } }); - case SendAccessConstants.EmailOtpValidatorResults.EmailOtpInvalid: - return new GrantValidationResult( - TokenRequestErrors.InvalidGrant, - errorDescription: _sendEmailOtpValidatorErrorDescriptions[error], - new Dictionary - { - { SendAccessConstants.SendAccessError, error } - }); default: return new GrantValidationResult( TokenRequestErrors.InvalidRequest, From 992e7d64914109c8cb09b4775f997e476f0ee042 Mon Sep 17 00:00:00 2001 From: Ike Kottlowski Date: Fri, 6 Mar 2026 12:12:28 -0500 Subject: [PATCH 2/7] test: update validator tests --- .../SendEmailOtpRequestValidatorTests.cs | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/test/Identity.Test/IdentityServer/SendAccess/SendEmailOtpRequestValidatorTests.cs b/test/Identity.Test/IdentityServer/SendAccess/SendEmailOtpRequestValidatorTests.cs index 5001ac88da31..7b5f1fc45132 100644 --- a/test/Identity.Test/IdentityServer/SendAccess/SendEmailOtpRequestValidatorTests.cs +++ b/test/Identity.Test/IdentityServer/SendAccess/SendEmailOtpRequestValidatorTests.cs @@ -1,4 +1,5 @@ -using Bit.Core.Auth.Identity; +using System.Globalization; +using Bit.Core.Auth.Identity; using Bit.Core.Auth.Identity.TokenProviders; using Bit.Core.Services; using Bit.Core.Tools.Models.Data; @@ -96,7 +97,7 @@ public async Task ValidateRequestAsync_EmailWithoutOtp_GeneratesAndSendsOtp( Request = tokenRequest }; - var expectedUniqueId = string.Format(SendAccessConstants.OtpToken.TokenUniqueIdentifier, sendId, email); + var expectedUniqueId = string.Format(CultureInfo.InvariantCulture, SendAccessConstants.OtpToken.TokenUniqueIdentifier, sendId, email); sutProvider.GetDependency>() .GenerateTokenAsync( @@ -181,7 +182,7 @@ public async Task ValidateRequestAsync_ValidOtp_ReturnsSuccess( emailOtp = emailOtp with { emails = [email] }; - var expectedUniqueId = string.Format(SendAccessConstants.OtpToken.TokenUniqueIdentifier, sendId, email); + var expectedUniqueId = string.Format(CultureInfo.InvariantCulture, SendAccessConstants.OtpToken.TokenUniqueIdentifier, sendId, email); sutProvider.GetDependency>() .ValidateTokenAsync( @@ -216,7 +217,7 @@ await sutProvider.GetDependency() } [Theory, BitAutoData] - public async Task ValidateRequestAsync_InvalidOtp_ReturnsInvalidGrant( + public async Task ValidateRequestAsync_InvalidOtp_ReturnsInvalidRequest( SutProvider sutProvider, [AutoFixture.ValidatedTokenRequest] ValidatedTokenRequest tokenRequest, EmailOtp emailOtp, @@ -233,7 +234,7 @@ public async Task ValidateRequestAsync_InvalidOtp_ReturnsInvalidGrant( emailOtp = emailOtp with { emails = [email] }; - var expectedUniqueId = string.Format(SendAccessConstants.OtpToken.TokenUniqueIdentifier, sendId, email); + var expectedUniqueId = string.Format(CultureInfo.InvariantCulture, SendAccessConstants.OtpToken.TokenUniqueIdentifier, sendId, email); sutProvider.GetDependency>() .ValidateTokenAsync(invalidOtp, @@ -247,8 +248,8 @@ public async Task ValidateRequestAsync_InvalidOtp_ReturnsInvalidGrant( // Assert Assert.True(result.IsError); - Assert.Equal(OidcConstants.TokenErrors.InvalidGrant, result.Error); - Assert.Equal("email otp is invalid.", result.ErrorDescription); + Assert.Equal(OidcConstants.TokenErrors.InvalidRequest, result.Error); + Assert.Equal($"{SendAccessConstants.TokenRequest.Email} and {SendAccessConstants.TokenRequest.Otp} are required.", result.ErrorDescription); // Verify OTP validation was attempted await sutProvider.GetDependency>() @@ -265,8 +266,9 @@ public void Constructor_WithValidParameters_CreatesInstance() // Arrange var otpTokenProvider = Substitute.For>(); var mailService = Substitute.For(); + var logger = Substitute.For>(); // Act - var validator = new SendEmailOtpRequestValidator(otpTokenProvider, mailService); + var validator = new SendEmailOtpRequestValidator(logger, otpTokenProvider, mailService); // Assert Assert.NotNull(validator); From b2436bfa9c73b831d6d197245e07f1950f3deb86 Mon Sep 17 00:00:00 2001 From: Ike Kottlowski Date: Fri, 6 Mar 2026 12:12:39 -0500 Subject: [PATCH 3/7] feat: remove vestigial constants. --- .../RequestValidators/SendAccess/SendAccessConstants.cs | 8 -------- 1 file changed, 8 deletions(-) diff --git a/src/Identity/IdentityServer/RequestValidators/SendAccess/SendAccessConstants.cs b/src/Identity/IdentityServer/RequestValidators/SendAccess/SendAccessConstants.cs index f38a4a880f1d..3ed76a93d05c 100644 --- a/src/Identity/IdentityServer/RequestValidators/SendAccess/SendAccessConstants.cs +++ b/src/Identity/IdentityServer/RequestValidators/SendAccess/SendAccessConstants.cs @@ -72,14 +72,6 @@ public static class EmailOtpValidatorResults /// Represents the status indicating that both email and OTP are required, and the OTP has been sent. /// public const string EmailAndOtpRequired = "email_and_otp_required"; - /// - /// Represents the status indicating that both email and OTP are required, and the OTP is invalid. - /// - public const string EmailOtpInvalid = "otp_invalid"; - /// - /// For what ever reason the OTP was not able to be generated - /// - public const string OtpGenerationFailed = "otp_generation_failed"; } /// From a08465f3fdba7e70b3a8bf085d136eaa92c0ad2d Mon Sep 17 00:00:00 2001 From: Ike Kottlowski Date: Fri, 6 Mar 2026 12:12:51 -0500 Subject: [PATCH 4/7] test: fix snapshot test for SendAccess --- .../IdentityServer/SendAccess/SendConstantsSnapshotTests.cs | 2 -- 1 file changed, 2 deletions(-) diff --git a/test/Identity.Test/IdentityServer/SendAccess/SendConstantsSnapshotTests.cs b/test/Identity.Test/IdentityServer/SendAccess/SendConstantsSnapshotTests.cs index 76a40410c9e8..41458f22dc9f 100644 --- a/test/Identity.Test/IdentityServer/SendAccess/SendConstantsSnapshotTests.cs +++ b/test/Identity.Test/IdentityServer/SendAccess/SendConstantsSnapshotTests.cs @@ -50,8 +50,6 @@ public void EmailOtpValidatorResults_Constants_HaveCorrectValues() // Assert Assert.Equal("email_required", SendAccessConstants.EmailOtpValidatorResults.EmailRequired); Assert.Equal("email_and_otp_required", SendAccessConstants.EmailOtpValidatorResults.EmailAndOtpRequired); - Assert.Equal("otp_invalid", SendAccessConstants.EmailOtpValidatorResults.EmailOtpInvalid); - Assert.Equal("otp_generation_failed", SendAccessConstants.EmailOtpValidatorResults.OtpGenerationFailed); } [Fact] From 90031768916fbc3b19e55f827767771faffcf8a2 Mon Sep 17 00:00:00 2001 From: Ike <137194738+ike-kottlowski@users.noreply.github.com> Date: Fri, 6 Mar 2026 14:22:56 -0500 Subject: [PATCH 5/7] Update warning message for OTP generation failure --- .../SendAccess/SendEmailOtpRequestValidator.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Identity/IdentityServer/RequestValidators/SendAccess/SendEmailOtpRequestValidator.cs b/src/Identity/IdentityServer/RequestValidators/SendAccess/SendEmailOtpRequestValidator.cs index a73d9d0f14ac..89584f434788 100644 --- a/src/Identity/IdentityServer/RequestValidators/SendAccess/SendEmailOtpRequestValidator.cs +++ b/src/Identity/IdentityServer/RequestValidators/SendAccess/SendEmailOtpRequestValidator.cs @@ -61,7 +61,7 @@ public async Task ValidateRequestAsync(ExtensionGrantVali // Verify that the OTP is generated if (string.IsNullOrEmpty(token)) { - logger.LogWarning("Failed to generate OTP for sendId: {SendId}", sendId); + logger.LogWarning("Failed to generate OTP for SendAccess"); return BuildErrorResult(); } From ef44dc4a60ded83b5f51a7664d1104f306d47d8b Mon Sep 17 00:00:00 2001 From: Ike <137194738+ike-kottlowski@users.noreply.github.com> Date: Fri, 6 Mar 2026 14:28:42 -0500 Subject: [PATCH 6/7] Change log level from warning to error for OTP failure --- .../SendAccess/SendEmailOtpRequestValidator.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Identity/IdentityServer/RequestValidators/SendAccess/SendEmailOtpRequestValidator.cs b/src/Identity/IdentityServer/RequestValidators/SendAccess/SendEmailOtpRequestValidator.cs index 89584f434788..90c41193f7c6 100644 --- a/src/Identity/IdentityServer/RequestValidators/SendAccess/SendEmailOtpRequestValidator.cs +++ b/src/Identity/IdentityServer/RequestValidators/SendAccess/SendEmailOtpRequestValidator.cs @@ -61,7 +61,7 @@ public async Task ValidateRequestAsync(ExtensionGrantVali // Verify that the OTP is generated if (string.IsNullOrEmpty(token)) { - logger.LogWarning("Failed to generate OTP for SendAccess"); + logger.LogError("Failed to generate OTP for SendAccess"); return BuildErrorResult(); } From fdc43dc3baa33649593d4e232dfac7aba7af2aa5 Mon Sep 17 00:00:00 2001 From: Ike Kottlowski Date: Fri, 6 Mar 2026 17:33:50 -0500 Subject: [PATCH 7/7] fix: fix failing test --- .../SendEmailOtpReqestValidatorIntegrationTests.cs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/test/Identity.IntegrationTest/RequestValidation/SendAccess/SendEmailOtpReqestValidatorIntegrationTests.cs b/test/Identity.IntegrationTest/RequestValidation/SendAccess/SendEmailOtpReqestValidatorIntegrationTests.cs index 9250859de119..63d8829bc341 100644 --- a/test/Identity.IntegrationTest/RequestValidation/SendAccess/SendEmailOtpReqestValidatorIntegrationTests.cs +++ b/test/Identity.IntegrationTest/RequestValidation/SendAccess/SendEmailOtpReqestValidatorIntegrationTests.cs @@ -2,6 +2,7 @@ using Bit.Core.Services; using Bit.Core.Tools.Models.Data; using Bit.Core.Tools.SendFeatures.Queries.Interfaces; +using Bit.Identity.IdentityServer.RequestValidators.SendAccess; using Bit.IntegrationTestCommon.Factories; using Duende.IdentityModel; using NSubstitute; @@ -132,7 +133,7 @@ public async Task SendAccess_EmailOtpProtectedSend_ValidOtp_ReturnsAccessToken() } [Fact] - public async Task SendAccess_EmailOtpProtectedSend_InvalidOtp_ReturnsInvalidGrant() + public async Task SendAccess_EmailOtpProtectedSend_InvalidOtp_ReturnsInvalidRequest() { // Arrange var sendId = Guid.NewGuid(); @@ -170,8 +171,8 @@ public async Task SendAccess_EmailOtpProtectedSend_InvalidOtp_ReturnsInvalidGran // Assert var content = await response.Content.ReadAsStringAsync(); - Assert.Contains(OidcConstants.TokenErrors.InvalidGrant, content); - Assert.Contains("email otp is invalid", content); + Assert.Contains(OidcConstants.TokenErrors.InvalidRequest, content); + Assert.Contains($"{SendAccessConstants.TokenRequest.Email} and {SendAccessConstants.TokenRequest.Otp} are required.", content); } [Fact]