From 3a2d09b0662709c99b1f26168b00d0067b3f6c1b Mon Sep 17 00:00:00 2001 From: Ryan Nowak Date: Thu, 4 Feb 2016 08:36:35 -0800 Subject: [PATCH] Move exceptions from the store to the facade My earlier change to add TryValidateRequestAsync didn't go far enough, because the store will still throw when the tokens aren't present. This change is to make the store just return null tokens in these cases, and move the exceptions to DefaultAntiforgery. --- .../AntiforgeryTokenSet.cs | 9 -- .../Internal/DefaultAntiforgery.cs | 35 +++++- .../Internal/DefaultAntiforgeryTokenStore.cs | 32 +----- .../Internal/IAntiforgeryTokenStore.cs | 4 +- .../Internal/DefaultAntiforgeryTest.cs | 103 ++++++++++++++++++ .../DefaultAntiforgeryTokenStoreTest.cs | 35 +++--- 6 files changed, 156 insertions(+), 62 deletions(-) diff --git a/src/Microsoft.AspNetCore.Antiforgery/AntiforgeryTokenSet.cs b/src/Microsoft.AspNetCore.Antiforgery/AntiforgeryTokenSet.cs index 2b99141894..033e5e0731 100644 --- a/src/Microsoft.AspNetCore.Antiforgery/AntiforgeryTokenSet.cs +++ b/src/Microsoft.AspNetCore.Antiforgery/AntiforgeryTokenSet.cs @@ -23,22 +23,13 @@ namespace Microsoft.AspNetCore.Antiforgery string formFieldName, string headerName) { - if (string.IsNullOrEmpty(requestToken)) - { - throw new ArgumentException(Resources.ArgumentCannotBeNullOrEmpty, nameof(requestToken)); - } - if (formFieldName == null) { throw new ArgumentNullException(nameof(formFieldName)); } RequestToken = requestToken; - - // Cookie Token is allowed to be null in the case when the old cookie is valid - // and there is no new cookieToken generated. CookieToken = cookieToken; - FormFieldName = formFieldName; HeaderName = headerName; } diff --git a/src/Microsoft.AspNetCore.Antiforgery/Internal/DefaultAntiforgery.cs b/src/Microsoft.AspNetCore.Antiforgery/Internal/DefaultAntiforgery.cs index 6d52cfa4da..1a581a5f54 100644 --- a/src/Microsoft.AspNetCore.Antiforgery/Internal/DefaultAntiforgery.cs +++ b/src/Microsoft.AspNetCore.Antiforgery/Internal/DefaultAntiforgery.cs @@ -93,6 +93,10 @@ namespace Microsoft.AspNetCore.Antiforgery.Internal CheckSSLConfig(httpContext); var tokens = await _tokenStore.GetRequestTokensAsync(httpContext); + if (tokens.CookieToken == null || tokens.RequestToken == null) + { + return false; + } // Extract cookie & request tokens var deserializedCookieToken = _tokenSerializer.Deserialize(tokens.CookieToken); @@ -118,6 +122,33 @@ namespace Microsoft.AspNetCore.Antiforgery.Internal CheckSSLConfig(httpContext); var tokens = await _tokenStore.GetRequestTokensAsync(httpContext); + if (tokens.CookieToken == null) + { + throw new AntiforgeryValidationException( + Resources.FormatAntiforgery_CookieToken_MustBeProvided(_options.CookieName)); + } + + if (tokens.RequestToken == null) + { + if (_options.HeaderName == null) + { + var message = Resources.FormatAntiforgery_FormToken_MustBeProvided(_options.FormFieldName); + throw new AntiforgeryValidationException(message); + } + else if (!httpContext.Request.HasFormContentType) + { + var message = Resources.FormatAntiforgery_HeaderToken_MustBeProvided(_options.HeaderName); + throw new AntiforgeryValidationException(message); + } + else + { + var message = Resources.FormatAntiforgery_RequestToken_MustBeProvided( + _options.FormFieldName, + _options.HeaderName); + throw new AntiforgeryValidationException(message); + } + } + ValidateTokens(httpContext, tokens); } @@ -262,8 +293,8 @@ namespace Microsoft.AspNetCore.Antiforgery.Internal private AntiforgeryTokenSet Serialize(AntiforgeryTokenSetInternal tokenSet) { return new AntiforgeryTokenSet( - tokenSet.RequestToken != null ? _tokenSerializer.Serialize(tokenSet.RequestToken) : null, - tokenSet.CookieToken != null ? _tokenSerializer.Serialize(tokenSet.CookieToken) : null, + _tokenSerializer.Serialize(tokenSet.RequestToken), + _tokenSerializer.Serialize(tokenSet.CookieToken), _options.FormFieldName, _options.HeaderName); } diff --git a/src/Microsoft.AspNetCore.Antiforgery/Internal/DefaultAntiforgeryTokenStore.cs b/src/Microsoft.AspNetCore.Antiforgery/Internal/DefaultAntiforgeryTokenStore.cs index 52d852a27c..5b0f41426a 100644 --- a/src/Microsoft.AspNetCore.Antiforgery/Internal/DefaultAntiforgeryTokenStore.cs +++ b/src/Microsoft.AspNetCore.Antiforgery/Internal/DefaultAntiforgeryTokenStore.cs @@ -65,18 +65,13 @@ namespace Microsoft.AspNetCore.Antiforgery.Internal throw new ArgumentNullException(nameof(httpContext)); } - var requestCookie = httpContext.Request.Cookies[_options.CookieName]; - if (string.IsNullOrEmpty(requestCookie)) - { - throw new AntiforgeryValidationException( - Resources.FormatAntiforgery_CookieToken_MustBeProvided(_options.CookieName)); - } + var cookieToken = httpContext.Request.Cookies[_options.CookieName]; StringValues requestToken; if (httpContext.Request.HasFormContentType) { // Check the content-type before accessing the form collection to make sure - // we throw gracefully. + // we report errors gracefully. var form = await httpContext.Request.ReadFormAsync(); requestToken = form[_options.FormFieldName]; } @@ -87,28 +82,7 @@ namespace Microsoft.AspNetCore.Antiforgery.Internal requestToken = httpContext.Request.Headers[_options.HeaderName]; } - if (requestToken.Count == 0) - { - if (_options.HeaderName == null) - { - var message = Resources.FormatAntiforgery_FormToken_MustBeProvided(_options.FormFieldName); - throw new AntiforgeryValidationException(message); - } - else if (!httpContext.Request.HasFormContentType) - { - var message = Resources.FormatAntiforgery_HeaderToken_MustBeProvided(_options.HeaderName); - throw new AntiforgeryValidationException(message); - } - else - { - var message = Resources.FormatAntiforgery_RequestToken_MustBeProvided( - _options.FormFieldName, - _options.HeaderName); - throw new AntiforgeryValidationException(message); - } - } - - return new AntiforgeryTokenSet(requestToken, requestCookie, _options.FormFieldName, _options.HeaderName); + return new AntiforgeryTokenSet(requestToken, cookieToken, _options.FormFieldName, _options.HeaderName); } public void SaveCookieToken(HttpContext httpContext, AntiforgeryToken token) diff --git a/src/Microsoft.AspNetCore.Antiforgery/Internal/IAntiforgeryTokenStore.cs b/src/Microsoft.AspNetCore.Antiforgery/Internal/IAntiforgeryTokenStore.cs index bff9360ff9..967887a44f 100644 --- a/src/Microsoft.AspNetCore.Antiforgery/Internal/IAntiforgeryTokenStore.cs +++ b/src/Microsoft.AspNetCore.Antiforgery/Internal/IAntiforgeryTokenStore.cs @@ -6,14 +6,12 @@ using Microsoft.AspNetCore.Http; namespace Microsoft.AspNetCore.Antiforgery.Internal { - // Provides an abstraction around how tokens are persisted and retrieved for a request public interface IAntiforgeryTokenStore { AntiforgeryToken GetCookieToken(HttpContext httpContext); /// - /// Gets the cookie and request tokens from the request. Will throw an exception if either token is - /// not present. + /// Gets the cookie and request tokens from the request. /// /// The for the current request. /// The . diff --git a/test/Microsoft.AspNetCore.Antiforgery.Test/Internal/DefaultAntiforgeryTest.cs b/test/Microsoft.AspNetCore.Antiforgery.Test/Internal/DefaultAntiforgeryTest.cs index 0e8dfeb8dd..040cf866a4 100644 --- a/test/Microsoft.AspNetCore.Antiforgery.Test/Internal/DefaultAntiforgeryTest.cs +++ b/test/Microsoft.AspNetCore.Antiforgery.Test/Internal/DefaultAntiforgeryTest.cs @@ -591,6 +591,109 @@ namespace Microsoft.AspNetCore.Antiforgery.Internal context.TokenGenerator.Verify(); } + [Fact] + public async Task ValidateRequestAsync_NoCookieToken_Throws() + { + // Arrange + var context = CreateMockContext(new AntiforgeryOptions() + { + CookieName = "cookie-name", + FormFieldName = "form-field-name", + HeaderName = null, + }); + + var tokenSet = new AntiforgeryTokenSet(null, null, "form-field-name", null); + context.TokenStore + .Setup(s => s.GetRequestTokensAsync(context.HttpContext)) + .Returns(Task.FromResult(tokenSet)); + + var antiforgery = GetAntiforgery(context); + + // Act & Assert + var exception = await Assert.ThrowsAsync( + () => antiforgery.ValidateRequestAsync(context.HttpContext)); + Assert.Equal("The required antiforgery cookie \"cookie-name\" is not present.", exception.Message); + } + + [Fact] + public async Task ValidateRequestAsync_NonFormRequest_HeaderDisabled_Throws() + { + // Arrange + var context = CreateMockContext(new AntiforgeryOptions() + { + CookieName = "cookie-name", + FormFieldName = "form-field-name", + HeaderName = null, + }); + + var tokenSet = new AntiforgeryTokenSet(null, "cookie-token", "form-field-name", null); + context.TokenStore + .Setup(s => s.GetRequestTokensAsync(context.HttpContext)) + .Returns(Task.FromResult(tokenSet)); + + var antiforgery = GetAntiforgery(context); + + // Act & Assert + var exception = await Assert.ThrowsAsync( + () => antiforgery.ValidateRequestAsync(context.HttpContext)); + Assert.Equal("The required antiforgery form field \"form-field-name\" is not present.", exception.Message); + } + + [Fact] + public async Task ValidateRequestAsync_NonFormRequest_NoHeaderValue_Throws() + { + // Arrange + var context = CreateMockContext(new AntiforgeryOptions() + { + CookieName = "cookie-name", + FormFieldName = "form-field-name", + HeaderName = "header-name", + }); + + context.HttpContext.Request.ContentType = "application/json"; + + var tokenSet = new AntiforgeryTokenSet(null, "cookie-token", "form-field-name", "header-name"); + context.TokenStore + .Setup(s => s.GetRequestTokensAsync(context.HttpContext)) + .Returns(Task.FromResult(tokenSet)); + + var antiforgery = GetAntiforgery(context); + + // Act & Assert + var exception = await Assert.ThrowsAsync( + () => antiforgery.ValidateRequestAsync(context.HttpContext)); + Assert.Equal("The required antiforgery header value \"header-name\" is not present.", exception.Message); + } + + [Fact] + public async Task ValidateRequestAsync_FormRequest_NoRequestTokenValue_Throws() + { + // Arrange + var context = CreateMockContext(new AntiforgeryOptions() + { + CookieName = "cookie-name", + FormFieldName = "form-field-name", + HeaderName = "header-name", + }); + + context.HttpContext.Request.ContentType = "application/x-www-form-urlencoded"; + + var tokenSet = new AntiforgeryTokenSet(null, "cookie-token", "form-field-name", "header-name"); + context.TokenStore + .Setup(s => s.GetRequestTokensAsync(context.HttpContext)) + .Returns(Task.FromResult(tokenSet)); + + var antiforgery = GetAntiforgery(context); + + // Act & Assert + var exception = await Assert.ThrowsAsync( + () => antiforgery.ValidateRequestAsync(context.HttpContext)); + Assert.Equal( + "The required antiforgery request token was not provided in either form field \"form-field-name\" " + + "or header value \"header-name\".", + exception.Message); + } + [Theory] [InlineData(false, "SAMEORIGIN")] [InlineData(true, null)] diff --git a/test/Microsoft.AspNetCore.Antiforgery.Test/Internal/DefaultAntiforgeryTokenStoreTest.cs b/test/Microsoft.AspNetCore.Antiforgery.Test/Internal/DefaultAntiforgeryTokenStoreTest.cs index 13f3ff7082..16ad4857a1 100644 --- a/test/Microsoft.AspNetCore.Antiforgery.Test/Internal/DefaultAntiforgeryTokenStoreTest.cs +++ b/test/Microsoft.AspNetCore.Antiforgery.Test/Internal/DefaultAntiforgeryTokenStoreTest.cs @@ -145,7 +145,7 @@ namespace Microsoft.AspNetCore.Antiforgery.Internal } [Fact] - public async Task GetRequestTokens_CookieIsEmpty_Throws() + public async Task GetRequestTokens_CookieIsEmpty_ReturnsNullTokens() { // Arrange var httpContext = GetHttpContext(new RequestCookieCollection()); @@ -162,15 +162,15 @@ namespace Microsoft.AspNetCore.Antiforgery.Internal tokenSerializer: Mock.Of()); // Act - var exception = await Assert.ThrowsAsync( - async () => await tokenStore.GetRequestTokensAsync(httpContext)); + var tokenSet = await tokenStore.GetRequestTokensAsync(httpContext); // Assert - Assert.Equal("The required antiforgery cookie \"cookie-name\" is not present.", exception.Message); + Assert.Null(tokenSet.CookieToken); + Assert.Null(tokenSet.RequestToken); } [Fact] - public async Task GetRequestTokens_NonFormContentType_HeaderDisabled_Throws() + public async Task GetRequestTokens_NonFormContentType_HeaderDisabled_ReturnsNullToken() { // Arrange var httpContext = GetHttpContext("cookie-name", "cookie-value"); @@ -191,11 +191,11 @@ namespace Microsoft.AspNetCore.Antiforgery.Internal tokenSerializer: new DefaultAntiforgeryTokenSerializer(new EphemeralDataProtectionProvider(), _pool)); // Act - var exception = await Assert.ThrowsAsync( - async () => await tokenStore.GetRequestTokensAsync(httpContext)); + var tokenSet = await tokenStore.GetRequestTokensAsync(httpContext); // Assert - Assert.Equal("The required antiforgery form field \"form-field-name\" is not present.", exception.Message); + Assert.Equal("cookie-value", tokenSet.CookieToken); + Assert.Null(tokenSet.RequestToken); } [Fact] @@ -257,7 +257,7 @@ namespace Microsoft.AspNetCore.Antiforgery.Internal } [Fact] - public async Task GetRequestTokens_NonFormContentType_UsesHeaderToken_ThrowsOnMissingValue() + public async Task GetRequestTokens_NonFormContentType_NoHeaderToken_ReturnsNullToken() { // Arrange var httpContext = GetHttpContext("cookie-name", "cookie-value"); @@ -278,15 +278,15 @@ namespace Microsoft.AspNetCore.Antiforgery.Internal tokenSerializer: new DefaultAntiforgeryTokenSerializer(new EphemeralDataProtectionProvider(), _pool)); // Act - var exception = await Assert.ThrowsAsync( - async () => await tokenStore.GetRequestTokensAsync(httpContext)); + var tokenSet = await tokenStore.GetRequestTokensAsync(httpContext); // Assert - Assert.Equal("The required antiforgery header value \"header-name\" is not present.", exception.Message); + Assert.Equal("cookie-value", tokenSet.CookieToken); + Assert.Null(tokenSet.RequestToken); } [Fact] - public async Task GetRequestTokens_BothFieldsEmpty_Throws() + public async Task GetRequestTokens_BothFieldsEmpty_ReturnsNullTokens() { // Arrange var httpContext = GetHttpContext("cookie-name", "cookie-value"); @@ -305,14 +305,11 @@ namespace Microsoft.AspNetCore.Antiforgery.Internal tokenSerializer: Mock.Of()); // Act - var exception = await Assert.ThrowsAsync( - async () => await tokenStore.GetRequestTokensAsync(httpContext)); + var tokenSet = await tokenStore.GetRequestTokensAsync(httpContext); // Assert - Assert.Equal( - "The required antiforgery request token was not provided in either form field \"form-field-name\" " + - "or header value \"header-name\".", - exception.Message); + Assert.Equal("cookie-value", tokenSet.CookieToken); + Assert.Null(tokenSet.RequestToken); } [Fact]