From 07a2f1de06282a3a83ab578e815e7d2428e29e01 Mon Sep 17 00:00:00 2001 From: Kiran Challa Date: Tue, 8 Nov 2016 11:56:00 -0800 Subject: [PATCH] Fixes CookieTempDataProvider to set the secure attribute of a cookie only if a request is secure --- .../ViewFeatures/CookieTempDataProvider.cs | 2 +- .../TempDataInCookiesTest.cs | 34 ++++++++++++- .../CookieTempDataProviderTest.cs | 51 ++++++++++++++++++- 3 files changed, 82 insertions(+), 5 deletions(-) diff --git a/src/Microsoft.AspNetCore.Mvc.ViewFeatures/ViewFeatures/CookieTempDataProvider.cs b/src/Microsoft.AspNetCore.Mvc.ViewFeatures/ViewFeatures/CookieTempDataProvider.cs index e59f37175e..f35c9913d2 100644 --- a/src/Microsoft.AspNetCore.Mvc.ViewFeatures/ViewFeatures/CookieTempDataProvider.cs +++ b/src/Microsoft.AspNetCore.Mvc.ViewFeatures/ViewFeatures/CookieTempDataProvider.cs @@ -68,7 +68,7 @@ public void SaveTempData(HttpContext context, IDictionary values Path = string.IsNullOrEmpty(_options.Value.Path) ? GetPathBase(context) : _options.Value.Path, Domain = string.IsNullOrEmpty(_options.Value.Domain) ? null : _options.Value.Domain, HttpOnly = true, - Secure = true + Secure = context.Request.IsHttps, }; var hasValues = (values != null && values.Count > 0); diff --git a/test/Microsoft.AspNetCore.Mvc.FunctionalTests/TempDataInCookiesTest.cs b/test/Microsoft.AspNetCore.Mvc.FunctionalTests/TempDataInCookiesTest.cs index 3eedb90ff3..03648a534e 100644 --- a/test/Microsoft.AspNetCore.Mvc.FunctionalTests/TempDataInCookiesTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.FunctionalTests/TempDataInCookiesTest.cs @@ -56,7 +56,7 @@ public async Task RoundTripLargeData_WorksWithChunkingCookies(int size) Assert.NotNull(cookieTempDataProviderCookie.Value); Assert.Equal("/", cookieTempDataProviderCookie.Path); Assert.Null(cookieTempDataProviderCookie.Domain); - Assert.True(cookieTempDataProviderCookie.Secure); + Assert.False(cookieTempDataProviderCookie.Secure); } // Act 2 @@ -107,7 +107,7 @@ public async Task Redirect_RetainsTempData_EvenIfAccessed_AndSetsAppropriateCook Assert.NotNull(setCookieHeader); Assert.Equal("/", setCookieHeader.Path); Assert.Null(setCookieHeader.Domain); - Assert.True(setCookieHeader.Secure); + Assert.False(setCookieHeader.Secure); Assert.Null(setCookieHeader.Expires); // Act 2 @@ -134,5 +134,35 @@ public async Task Redirect_RetainsTempData_EvenIfAccessed_AndSetsAppropriateCook Assert.NotNull(setCookieHeader.Expires); Assert.True(setCookieHeader.Expires < DateTimeOffset.Now); // expired cookie } + + [Theory] + [InlineData(true)] + [InlineData(false)] + public async Task CookieTempDataProviderCookie_SetsSecureAttributeOnCookie_OnlyIfRequestIsSecure(bool secureRequest) + { + // Arrange + var protocol = secureRequest ? "https" : "http"; + var nameValueCollection = new List> + { + new KeyValuePair("value", "Foo"), + }; + var content = new FormUrlEncodedContent(nameValueCollection); + + // Act + var response = await Client.PostAsync($"{protocol}://localhost/TempData/SetTempData", content); + + // Assert + Assert.Equal(HttpStatusCode.OK, response.StatusCode); + IEnumerable setCookieValues; + Assert.True(response.Headers.TryGetValues(HeaderNames.SetCookie, out setCookieValues)); + var setCookieHeader = setCookieValues + .Select(setCookieValue => SetCookieHeaderValue.Parse(setCookieValue)) + .FirstOrDefault(setCookieHeaderValue => setCookieHeaderValue.Name == CookieTempDataProvider.CookieName); + Assert.NotNull(setCookieHeader); + Assert.Equal("/", setCookieHeader.Path); + Assert.Null(setCookieHeader.Domain); + Assert.Equal(secureRequest, setCookieHeader.Secure); + Assert.Null(setCookieHeader.Expires); + } } } \ No newline at end of file diff --git a/test/Microsoft.AspNetCore.Mvc.ViewFeatures.Test/ViewFeatures/CookieTempDataProviderTest.cs b/test/Microsoft.AspNetCore.Mvc.ViewFeatures.Test/ViewFeatures/CookieTempDataProviderTest.cs index 6d9a646686..15a910cd79 100644 --- a/test/Microsoft.AspNetCore.Mvc.ViewFeatures.Test/ViewFeatures/CookieTempDataProviderTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.ViewFeatures.Test/ViewFeatures/CookieTempDataProviderTest.cs @@ -90,6 +90,47 @@ public void SaveTempData_ProtectsAnd_Base64UrlEncodesDataAnd_SetsCookie() Assert.Equal(expectedDataToProtect, dataProtector.PlainTextToProtect); } + [Theory] + [InlineData(true)] + [InlineData(false)] + public void SaveTempData_SetsSecureAttributeOnCookie_OnlyIfRequestIsSecure(bool isSecure) + { + // Arrange + var values = new Dictionary(); + values.Add("int", 10); + var tempDataProviderStore = new TempDataSerializer(); + var expectedDataToProtect = tempDataProviderStore.Serialize(values); + var expectedDataInCookie = Base64UrlTextEncoder.Encode(expectedDataToProtect); + var dataProtector = new PassThroughDataProtector(); + var tempDataProvider = GetProvider(dataProtector); + var responseCookies = new MockResponseCookieCollection(); + var httpContext = new Mock(); + httpContext + .SetupGet(hc => hc.Request.PathBase) + .Returns("/"); + httpContext + .SetupGet(hc => hc.Request.IsHttps) + .Returns(isSecure); + httpContext + .Setup(hc => hc.Response.Cookies) + .Returns(responseCookies); + + // Act + tempDataProvider.SaveTempData(httpContext.Object, values); + + // Assert + Assert.Equal(1, responseCookies.Count); + var cookieInfo = responseCookies[CookieTempDataProvider.CookieName]; + Assert.NotNull(cookieInfo); + Assert.Equal(expectedDataInCookie, cookieInfo.Value); + Assert.Equal(expectedDataToProtect, dataProtector.PlainTextToProtect); + Assert.Equal("/", cookieInfo.Options.Path); + Assert.Equal(isSecure, cookieInfo.Options.Secure); + Assert.True(cookieInfo.Options.HttpOnly); + Assert.Null(cookieInfo.Options.Expires); + Assert.Null(cookieInfo.Options.Domain); + } + [Theory] [InlineData(null, "/")] [InlineData("", "/")] @@ -112,6 +153,9 @@ public void SaveTempData_DefaultProviderOptions_SetsCookie_WithAppropriateCookie httpContext .SetupGet(hc => hc.Request.PathBase) .Returns(pathBase); + httpContext + .SetupGet(hc => hc.Request.IsHttps) + .Returns(false); httpContext .Setup(hc => hc.Response.Cookies) .Returns(responseCookies); @@ -126,7 +170,7 @@ public void SaveTempData_DefaultProviderOptions_SetsCookie_WithAppropriateCookie Assert.Equal(expectedDataInCookie, cookieInfo.Value); Assert.Equal(expectedDataToProtect, dataProtector.PlainTextToProtect); Assert.Equal(expectedCookiePath, cookieInfo.Options.Path); - Assert.True(cookieInfo.Options.Secure); + Assert.False(cookieInfo.Options.Secure); Assert.True(cookieInfo.Options.HttpOnly); Assert.Null(cookieInfo.Options.Expires); Assert.Null(cookieInfo.Options.Domain); @@ -154,6 +198,9 @@ public void SaveTempData_CustomProviderOptions_SetsCookie_WithAppropriateCookieO new CookieTempDataProviderOptions() { Path = optionsPath, Domain = optionsDomain }); var responseCookies = new MockResponseCookieCollection(); var httpContext = new Mock(); + httpContext + .SetupGet(hc => hc.Request.IsHttps) + .Returns(false); httpContext .SetupGet(hc => hc.Request.PathBase) .Returns(requestPathBase); @@ -172,7 +219,7 @@ public void SaveTempData_CustomProviderOptions_SetsCookie_WithAppropriateCookieO Assert.Equal(expectedDataToProtect, dataProtector.PlainTextToProtect); Assert.Equal(expectedCookiePath, cookieInfo.Options.Path); Assert.Equal(expectedDomain, cookieInfo.Options.Domain); - Assert.True(cookieInfo.Options.Secure); + Assert.False(cookieInfo.Options.Secure); Assert.True(cookieInfo.Options.HttpOnly); Assert.Null(cookieInfo.Options.Expires); }