From c8a13087a612371b4fafe4caf25097b2e1a3c8c9 Mon Sep 17 00:00:00 2001 From: Pranav K Date: Tue, 20 Jan 2015 00:49:15 -0800 Subject: [PATCH] Revert "Added SetAntiForgeryCookieAndHeader method that sets cookie token and header" This reverts commit 951ed05893d10beafbbe6449652684c19f2384ae. --- .../AntiForgery/AntiForgery.cs | 9 -- .../AntiForgery/AntiForgeryWorker.cs | 71 +++-------- src/Microsoft.AspNet.Mvc.Razor/RazorPage.cs | 19 +-- .../AntiXsrf/AntiForgeryWorkerTest.cs | 23 ---- .../AntiForgeryTests.cs | 112 ------------------ .../Controllers/AccountController.cs | 36 ------ .../Views/Account/FlushAsyncLogin.cshtml | 43 ------- .../Account/FlushWithoutUpdatingHeader.cshtml | 37 ------ .../Views/Shared/_FlushAsyncLayout.cshtml | 15 --- 9 files changed, 20 insertions(+), 345 deletions(-) delete mode 100644 test/WebSites/AntiForgeryWebSite/Views/Account/FlushAsyncLogin.cshtml delete mode 100644 test/WebSites/AntiForgeryWebSite/Views/Account/FlushWithoutUpdatingHeader.cshtml delete mode 100644 test/WebSites/AntiForgeryWebSite/Views/Shared/_FlushAsyncLayout.cshtml diff --git a/src/Microsoft.AspNet.Mvc.Core/AntiForgery/AntiForgery.cs b/src/Microsoft.AspNet.Mvc.Core/AntiForgery/AntiForgery.cs index aedab3ceb5..644ee99287 100644 --- a/src/Microsoft.AspNet.Mvc.Core/AntiForgery/AntiForgery.cs +++ b/src/Microsoft.AspNet.Mvc.Core/AntiForgery/AntiForgery.cs @@ -97,14 +97,5 @@ namespace Microsoft.AspNet.Mvc { Validate(context, antiForgeryTokenSet.CookieToken, antiForgeryTokenSet.FormToken); } - - /// - /// Generates and sets an anti-forgery cookie if one is not available or not valid. Also sets response headers. - /// - /// The HTTP context associated with the current call. - public void SetCookieTokenAndHeader([NotNull] HttpContext context) - { - _worker.SetCookieTokenAndHeader(context); - } } } \ No newline at end of file diff --git a/src/Microsoft.AspNet.Mvc.Core/AntiForgery/AntiForgeryWorker.cs b/src/Microsoft.AspNet.Mvc.Core/AntiForgery/AntiForgeryWorker.cs index 564fef4064..a2056fe0e3 100644 --- a/src/Microsoft.AspNet.Mvc.Core/AntiForgery/AntiForgeryWorker.cs +++ b/src/Microsoft.AspNet.Mvc.Core/AntiForgery/AntiForgeryWorker.cs @@ -102,8 +102,19 @@ namespace Microsoft.AspNet.Mvc var tokenSet = GetTokens(httpContext, oldCookieToken); var newCookieToken = tokenSet.CookieToken; var formToken = tokenSet.FormToken; + if (newCookieToken != null) + { + // If a new cookie was generated, persist it. + _tokenStore.SaveCookieToken(httpContext, newCookieToken); + } - SaveCookieTokenAndHeader(httpContext, newCookieToken); + if (!_config.SuppressXFrameOptionsHeader) + { + // Adding X-Frame-Options header to prevent ClickJacking. See + // http://tools.ietf.org/html/draft-ietf-websec-x-frame-options-10 + // for more information. + httpContext.Response.Headers.Set("X-Frame-Options", "SAMEORIGIN"); + } // var retVal = new TagBuilder("input"); @@ -132,11 +143,15 @@ namespace Microsoft.AspNet.Mvc private AntiForgeryTokenSetInternal GetTokens(HttpContext httpContext, AntiForgeryToken oldCookieToken) { - var newCookieToken = ValidateAndGenerateNewToken(oldCookieToken); - if (newCookieToken != null) + AntiForgeryToken newCookieToken = null; + if (!_validator.IsCookieTokenValid(oldCookieToken)) { - oldCookieToken = newCookieToken; + // Need to make sure we're always operating with a good cookie token. + oldCookieToken = newCookieToken = _generator.GenerateCookieToken(); } + + Debug.Assert(_validator.IsCookieTokenValid(oldCookieToken)); + var formToken = _generator.GenerateFormToken( httpContext, ExtractIdentity(httpContext), @@ -189,54 +204,6 @@ namespace Microsoft.AspNet.Mvc deserializedFormToken); } - - /// - /// Generates and sets an anti-forgery cookie if one is not available or not valid. Also sets response headers. - /// - /// The HTTP context associated with the current call. - public void SetCookieTokenAndHeader([NotNull] HttpContext httpContext) - { - CheckSSLConfig(httpContext); - - var oldCookieToken = GetCookieTokenNoThrow(httpContext); - var newCookieToken = ValidateAndGenerateNewToken(oldCookieToken); - - SaveCookieTokenAndHeader(httpContext, newCookieToken); - } - - // This method returns null if oldCookieToken is valid. - private AntiForgeryToken ValidateAndGenerateNewToken(AntiForgeryToken oldCookieToken) - { - if (!_validator.IsCookieTokenValid(oldCookieToken)) - { - // Need to make sure we're always operating with a good cookie token. - var newCookieToken = _generator.GenerateCookieToken(); - Debug.Assert(_validator.IsCookieTokenValid(newCookieToken)); - return newCookieToken; - } - - return null; - } - - private void SaveCookieTokenAndHeader( - [NotNull] HttpContext httpContext, - AntiForgeryToken newCookieToken) - { - if (newCookieToken != null) - { - // Persist the new cookie if it is not null. - _tokenStore.SaveCookieToken(httpContext, newCookieToken); - } - - if (!_config.SuppressXFrameOptionsHeader) - { - // Adding X-Frame-Options header to prevent ClickJacking. See - // http://tools.ietf.org/html/draft-ietf-websec-x-frame-options-10 - // for more information. - httpContext.Response.Headers.Set("X-Frame-Options", "SAMEORIGIN"); - } - } - private class AntiForgeryTokenSetInternal { public AntiForgeryToken FormToken { get; set; } diff --git a/src/Microsoft.AspNet.Mvc.Razor/RazorPage.cs b/src/Microsoft.AspNet.Mvc.Razor/RazorPage.cs index 630510435b..ccbcda94af 100644 --- a/src/Microsoft.AspNet.Mvc.Razor/RazorPage.cs +++ b/src/Microsoft.AspNet.Mvc.Razor/RazorPage.cs @@ -561,10 +561,7 @@ namespace Microsoft.AspNet.Mvc.Razor /// A that represents the asynchronous flush operation and on /// completion returns a . /// The value returned is a token value that allows FlushAsync to work directly in an HTML - /// section. However the value does not represent the rendered content. - /// This method also writes out headers, so any modifications to headers must be done before FulshAsync is - /// called. For example, call to send anti-forgery cookie token - /// and X-Frame-Options header to client before this method flushes headers out. + /// section. However the value does not represent the rendered content. public async Task FlushAsync() { // If there are active writing scopes then we should throw. Cannot flush content that has the potential to @@ -620,20 +617,6 @@ namespace Microsoft.AspNet.Mvc.Razor PageExecutionContext?.EndContext(); } - /// - /// Sets anti-forgery cookie and X-Frame-Options header on the response. - /// - /// A that returns a . - /// Call this method to send anti-forgery cookie token and X-Frame-Options header to client - /// before flushes the headers. - public virtual HtmlString SetAntiForgeryCookieAndHeader() - { - var antiForgery = Context.RequestServices.GetRequiredService(); - antiForgery.SetCookieTokenAndHeader(Context); - - return HtmlString.Empty; - } - private void EnsureMethodCanBeInvoked(string methodName) { if (PreviousSectionWriters == null) diff --git a/test/Microsoft.AspNet.Mvc.Core.Test/AntiXsrf/AntiForgeryWorkerTest.cs b/test/Microsoft.AspNet.Mvc.Core.Test/AntiXsrf/AntiForgeryWorkerTest.cs index 70f59312cf..ba111c3711 100644 --- a/test/Microsoft.AspNet.Mvc.Core.Test/AntiXsrf/AntiForgeryWorkerTest.cs +++ b/test/Microsoft.AspNet.Mvc.Core.Test/AntiXsrf/AntiForgeryWorkerTest.cs @@ -379,29 +379,6 @@ namespace Microsoft.AspNet.Mvc.Core.Test context.TokenProvider.Verify(); } - [Theory] - [InlineData(false, "SAMEORIGIN")] - [InlineData(true, null)] - public void SetCookieTokenAndHeader_AddsXFrameOptionsHeader(bool suppressXFrameOptions, string expectedHeaderValue) - { - // Arrange - var options = new AntiForgeryOptions() - { - SuppressXFrameOptionsHeader = suppressXFrameOptions - }; - - // Genreate a new cookie. - var context = GetAntiForgeryWorkerContext(options, useOldCookie: false, isOldCookieValid: false); - var worker = GetAntiForgeryWorker(context); - - // Act - worker.SetCookieTokenAndHeader(context.HttpContext.Object); - - // Assert - var xFrameOptions = context.HttpContext.Object.Response.Headers["X-Frame-Options"]; - Assert.Equal(expectedHeaderValue, xFrameOptions); - } - private AntiForgeryWorker GetAntiForgeryWorker(AntiForgeryWorkerContext context) { return new AntiForgeryWorker( diff --git a/test/Microsoft.AspNet.Mvc.FunctionalTests/AntiForgeryTests.cs b/test/Microsoft.AspNet.Mvc.FunctionalTests/AntiForgeryTests.cs index f0c6402499..4a19cbe97a 100644 --- a/test/Microsoft.AspNet.Mvc.FunctionalTests/AntiForgeryTests.cs +++ b/test/Microsoft.AspNet.Mvc.FunctionalTests/AntiForgeryTests.cs @@ -234,117 +234,5 @@ namespace Microsoft.AspNet.Mvc.FunctionalTests Assert.Equal("The required anti-forgery form field \"__RequestVerificationToken\" is not present.", exception.ExceptionMessage); } - - [Fact] - public async Task SetCookieAndHeaderBeforeFlushAsync_GeneratesCookieTokenAndHeader() - { - // Arrange - var server = TestServer.Create(_services, _app); - var client = server.CreateClient(); - - // Act - var response = await client.GetAsync("http://localhost/Account/FlushAsyncLogin"); - - // Assert - var header = Assert.Single(response.Headers.GetValues("X-Frame-Options")); - Assert.Equal("SAMEORIGIN", header); - - var setCookieHeader = response.Headers.GetValues("Set-Cookie").ToArray(); - - var cookie = Assert.Single(setCookieHeader); - Assert.True(cookie.StartsWith("__RequestVerificationToken")); - } - - [Fact] - public async Task SetCookieAndHeaderBeforeFlushAsync_PostToForm() - { - // Arrange - var server = TestServer.Create(_services, _app); - var client = server.CreateClient(); - - // do a get response. - var getResponse = await client.GetAsync("http://localhost/Account/FlushAsyncLogin"); - var resposneBody = await getResponse.Content.ReadAsStringAsync(); - - var formToken = AntiForgeryTestHelper.RetrieveAntiForgeryToken(resposneBody, "Account/FlushAsyncLogin"); - var cookieToken = AntiForgeryTestHelper.RetrieveAntiForgeryCookie(getResponse); - - var request = new HttpRequestMessage(HttpMethod.Post, "http://localhost/Account/FlushAsyncLogin"); - request.Headers.Add("Cookie", "__RequestVerificationToken=" + cookieToken); - var nameValueCollection = new List> - { - new KeyValuePair("__RequestVerificationToken", formToken), - new KeyValuePair("UserName", "test"), - new KeyValuePair("Password", "password"), - }; - - request.Content = new FormUrlEncodedContent(nameValueCollection); - - // Act - var response = await client.SendAsync(request); - - // Assert - Assert.Equal(HttpStatusCode.OK, response.StatusCode); - Assert.Equal("OK", await response.Content.ReadAsStringAsync()); - } - - [Fact] - public async Task FlushAsyncBeforeAntiForgery_CookieAndHeaderNotInResponse() - { - // Arrange - var server = TestServer.Create(_services, _app); - var client = server.CreateClient(); - - // Act - var response = await client.GetAsync("http://localhost/Account/FlushWithoutUpdatingHeader"); - - // Assert - IEnumerable returnList; - Assert.False(response.Headers.TryGetValues("Set-Cookie", out returnList)); - Assert.False(response.Headers.TryGetValues("X-Frame-Options", out returnList)); - } - - [Fact] - public async Task FlushAsyncBeforeAntiForgery_PostToForm() - { - // Arrange - var server = TestServer.Create(_services, _app); - var client = server.CreateClient(); - - // do a get response. - var getResponse = await client.GetAsync("http://localhost/Account/FlushWithoutUpdatingHeader"); - var resposneBody = await getResponse.Content.ReadAsStringAsync(); - - // Assert - 1 - IEnumerable returnList; - Assert.False(getResponse.Headers.TryGetValues("Set-Cookie", out returnList)); - Assert.False(getResponse.Headers.TryGetValues("X-Frame-Options", out returnList)); - - var formToken = AntiForgeryTestHelper.RetrieveAntiForgeryToken( - resposneBody, - "Account/FlushWithoutUpdatingHeader"); - - var request = new HttpRequestMessage( - HttpMethod.Post, - "http://localhost/Account/FlushWithoutUpdatingHeader"); - - var nameValueCollection = new List> - { - new KeyValuePair("__RequestVerificationToken", formToken), - new KeyValuePair("UserName", "test"), - new KeyValuePair("Password", "password"), - }; - - request.Content = new FormUrlEncodedContent(nameValueCollection); - - // Act - var response = await client.SendAsync(request); - - // Assert - 2 - var exception = response.GetServerException(); - Assert.Equal("The required anti-forgery cookie \"__RequestVerificationToken\" is not present.", - exception.ExceptionMessage); - } - } } \ No newline at end of file diff --git a/test/WebSites/AntiForgeryWebSite/Controllers/AccountController.cs b/test/WebSites/AntiForgeryWebSite/Controllers/AccountController.cs index 1a32e742c5..701b421a39 100644 --- a/test/WebSites/AntiForgeryWebSite/Controllers/AccountController.cs +++ b/test/WebSites/AntiForgeryWebSite/Controllers/AccountController.cs @@ -31,41 +31,5 @@ namespace AntiForgeryWebSite { return "OK"; } - - // GET: /Account/FlushAsyncLogin - [AllowAnonymous] - public ActionResult FlushAsyncLogin(string returnUrl = null) - { - ViewBag.ReturnUrl = returnUrl; - - return View(); - } - - // POST: /Account/FlushAsyncLogin - [HttpPost] - [AllowAnonymous] - [ValidateAntiForgeryToken] - public string FlushAsyncLogin(LoginViewModel model) - { - return "OK"; - } - - // GET: /Account/FlushWithoutUpdatingHeader - [AllowAnonymous] - public ActionResult FlushWithoutUpdatingHeader(string returnUrl = null) - { - ViewBag.ReturnUrl = returnUrl; - - return View(); - } - - // POST: /Account/FlushWithoutUpdatingHeader - [HttpPost] - [AllowAnonymous] - [ValidateAntiForgeryToken] - public string FlushWithoutUpdatingHeader(LoginViewModel model) - { - return "OK"; - } } } \ No newline at end of file diff --git a/test/WebSites/AntiForgeryWebSite/Views/Account/FlushAsyncLogin.cshtml b/test/WebSites/AntiForgeryWebSite/Views/Account/FlushAsyncLogin.cshtml deleted file mode 100644 index 65ee7aedd1..0000000000 --- a/test/WebSites/AntiForgeryWebSite/Views/Account/FlushAsyncLogin.cshtml +++ /dev/null @@ -1,43 +0,0 @@ -@model AntiForgeryWebSite.LoginViewModel - -@{ - ViewBag.Title = "Log in"; - Layout = "/Views/Shared/_FlushAsyncLayout.cshtml"; -} - -@section Login -{ -

@ViewBag.Title.

-
-
-
- @using (Html.BeginForm("FlushAsyncLogin", "Account", FormMethod.Post, new { @class = "form-horizontal", role = "form" })) - { - @Html.AntiForgeryToken() -

Use a local account to log in.

-
- @Html.ValidationSummary(true) -
- @Html.LabelFor(m => m.UserName, new { @class = "col-md-2", ClAsS = "col-md-2 control-label" }) -
- @Html.TextBoxFor(m => m.UserName, new { @class = "...", cLass = "form-control" }) - @Html.ValidationMessageFor(m => m.UserName) -
-
-
- @Html.LabelFor(m => m.Password, new { @class = "col-md-2 control-label" }) -
- @Html.PasswordFor(m => m.Password, new { @class = "form-control" }) - @Html.ValidationMessageFor(m => m.Password) -
-
-
-
- -
-
- } -
-
-
-} diff --git a/test/WebSites/AntiForgeryWebSite/Views/Account/FlushWithoutUpdatingHeader.cshtml b/test/WebSites/AntiForgeryWebSite/Views/Account/FlushWithoutUpdatingHeader.cshtml deleted file mode 100644 index 97cf95a94e..0000000000 --- a/test/WebSites/AntiForgeryWebSite/Views/Account/FlushWithoutUpdatingHeader.cshtml +++ /dev/null @@ -1,37 +0,0 @@ -@model AntiForgeryWebSite.LoginViewModel - -@await FlushAsync() - -

@ViewBag.Title.

-
-
-
- @using (Html.BeginForm("FlushWithoutUpdatingHeader", "Account", FormMethod.Post, new { @class = "form-horizontal", role = "form" })) - { - @Html.AntiForgeryToken() -

Use a local account to log in.

-
- @Html.ValidationSummary(true) -
- @Html.LabelFor(m => m.UserName, new { @class = "col-md-2", ClAsS = "col-md-2 control-label" }) -
- @Html.TextBoxFor(m => m.UserName, new { @class = "...", cLass = "form-control" }) - @Html.ValidationMessageFor(m => m.UserName) -
-
-
- @Html.LabelFor(m => m.Password, new { @class = "col-md-2 control-label" }) -
- @Html.PasswordFor(m => m.Password, new { @class = "form-control" }) - @Html.ValidationMessageFor(m => m.Password) -
-
-
-
- -
-
- } -
-
-
diff --git a/test/WebSites/AntiForgeryWebSite/Views/Shared/_FlushAsyncLayout.cshtml b/test/WebSites/AntiForgeryWebSite/Views/Shared/_FlushAsyncLayout.cshtml deleted file mode 100644 index 08c9240c47..0000000000 --- a/test/WebSites/AntiForgeryWebSite/Views/Shared/_FlushAsyncLayout.cshtml +++ /dev/null @@ -1,15 +0,0 @@ - - - @ViewBag.Title – AntiForgery Functional Tests - -@SetAntiForgeryCookieAndHeader() -@await FlushAsync() - - - @Html.ActionLink("Log in", "Login", "Account", routeValues: null, htmlAttributes: new { id = "loginLink" }) -
- @RenderBody() - @await RenderSectionAsync("Login", required: false) -
- - \ No newline at end of file