diff --git a/src/Http/Http/src/Features/ResponseCookiesFeature.cs b/src/Http/Http/src/Features/ResponseCookiesFeature.cs index 7e3cabb28e00..b51b99831ff6 100644 --- a/src/Http/Http/src/Features/ResponseCookiesFeature.cs +++ b/src/Http/Http/src/Features/ResponseCookiesFeature.cs @@ -15,7 +15,7 @@ public class ResponseCookiesFeature : IResponseCookiesFeature // Lambda hoisted to static readonly field to improve inlining https://github.com/dotnet/roslyn/issues/13624 private readonly static Func _nullResponseFeature = f => null; - private FeatureReferences _features; + private readonly IFeatureCollection _features; private IResponseCookies? _cookiesCollection; /// @@ -27,12 +27,7 @@ public class ResponseCookiesFeature : IResponseCookiesFeature /// public ResponseCookiesFeature(IFeatureCollection features) { - if (features == null) - { - throw new ArgumentNullException(nameof(features)); - } - - _features.Initalize(features); + _features = features ?? throw new ArgumentNullException(nameof(features)); } /// @@ -46,16 +41,9 @@ public ResponseCookiesFeature(IFeatureCollection features) [Obsolete("This constructor is obsolete and will be removed in a future version.")] public ResponseCookiesFeature(IFeatureCollection features, ObjectPool? builderPool) { - if (features == null) - { - throw new ArgumentNullException(nameof(features)); - } - - _features.Initalize(features); + _features = features ?? throw new ArgumentNullException(nameof(features)); } - private IHttpResponseFeature HttpResponseFeature => _features.Fetch(ref _features.Cache, _nullResponseFeature)!; - /// public IResponseCookies Cookies { @@ -63,8 +51,7 @@ public IResponseCookies Cookies { if (_cookiesCollection == null) { - var headers = HttpResponseFeature.Headers; - _cookiesCollection = new ResponseCookies(headers); + _cookiesCollection = new ResponseCookies(_features); } return _cookiesCollection; diff --git a/src/Http/Http/src/Internal/EventIds.cs b/src/Http/Http/src/Internal/EventIds.cs new file mode 100644 index 000000000000..ddefeceecf11 --- /dev/null +++ b/src/Http/Http/src/Internal/EventIds.cs @@ -0,0 +1,12 @@ +// Copyright (c) .NET Foundation. All rights reserved. +// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. + +using Microsoft.Extensions.Logging; + +namespace Microsoft.AspNetCore.Http +{ + internal static class EventIds + { + public static readonly EventId SameSiteNotSecure = new EventId(1, "SameSiteNotSecure"); + } +} diff --git a/src/Http/Http/src/Internal/ResponseCookies.cs b/src/Http/Http/src/Internal/ResponseCookies.cs index d9adfb69f16b..e6e582bfda0b 100644 --- a/src/Http/Http/src/Internal/ResponseCookies.cs +++ b/src/Http/Http/src/Internal/ResponseCookies.cs @@ -3,6 +3,9 @@ using System; using System.Collections.Generic; +using Microsoft.AspNetCore.Http.Features; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Logging; using Microsoft.Extensions.Primitives; using Microsoft.Net.Http.Headers; @@ -16,18 +19,16 @@ internal class ResponseCookies : IResponseCookies internal const string EnableCookieNameEncoding = "Microsoft.AspNetCore.Http.EnableCookieNameEncoding"; internal bool _enableCookieNameEncoding = AppContext.TryGetSwitch(EnableCookieNameEncoding, out var enabled) && enabled; + private readonly IFeatureCollection _features; + private ILogger? _logger; + /// /// Create a new wrapper. /// - /// The for the response. - public ResponseCookies(IHeaderDictionary headers) + internal ResponseCookies(IFeatureCollection features) { - if (headers == null) - { - throw new ArgumentNullException(nameof(headers)); - } - - Headers = headers; + _features = features; + Headers = _features.Get().Headers; } private IHeaderDictionary Headers { get; set; } @@ -54,6 +55,21 @@ public void Append(string key, string value, CookieOptions options) throw new ArgumentNullException(nameof(options)); } + // SameSite=None cookies must be marked as Secure. + if (!options.Secure && options.SameSite == SameSiteMode.None) + { + if (_logger == null) + { + var services = _features.Get()?.RequestServices; + _logger = services?.GetService>(); + } + + if (_logger != null) + { + Log.SameSiteCookieNotSecure(_logger, key); + } + } + var setCookieHeaderValue = new SetCookieHeaderValue( _enableCookieNameEncoding ? Uri.EscapeDataString(key) : key, Uri.EscapeDataString(value)) @@ -135,5 +151,18 @@ public void Delete(string key, CookieOptions options) SameSite = options.SameSite }); } + + private static class Log + { + private static readonly Action _samesiteNotSecure = LoggerMessage.Define( + LogLevel.Warning, + EventIds.SameSiteNotSecure, + "The cookie '{name}' has set 'SameSite=None' and must also set 'Secure'."); + + public static void SameSiteCookieNotSecure(ILogger logger, string name) + { + _samesiteNotSecure(logger, name, null); + } + } } } diff --git a/src/Http/Http/test/ResponseCookiesTest.cs b/src/Http/Http/test/ResponseCookiesTest.cs index 8e4e60aaa9ab..2c73a75ec1f3 100644 --- a/src/Http/Http/test/ResponseCookiesTest.cs +++ b/src/Http/Http/test/ResponseCookiesTest.cs @@ -2,6 +2,10 @@ // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. using System; +using Microsoft.AspNetCore.Http.Features; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Logging; +using Microsoft.Extensions.Logging.Testing; using Microsoft.Net.Http.Headers; using Xunit; @@ -9,11 +13,56 @@ namespace Microsoft.AspNetCore.Http.Tests { public class ResponseCookiesTest { + private IFeatureCollection MakeFeatures(IHeaderDictionary headers) + { + var responseFeature = new HttpResponseFeature() + { + Headers = headers + }; + var features = new FeatureCollection(); + features.Set(responseFeature); + return features; + } + + [Fact] + public void AppendSameSiteNoneWithoutSecureLogsWarning() + { + var headers = new HeaderDictionary(); + var features = MakeFeatures(headers); + var services = new ServiceCollection(); + + var sink = new TestSink(TestSink.EnableWithTypeName); + var loggerFactory = new TestLoggerFactory(sink, enabled: true); + services.AddLogging(); + services.AddSingleton(loggerFactory); + + features.Set(new ServiceProvidersFeature() { RequestServices = services.BuildServiceProvider() }); + + var cookies = new ResponseCookies(features); + var testCookie = "TestCookie"; + + cookies.Append(testCookie, "value", new CookieOptions() + { + SameSite = SameSiteMode.None, + }); + + var cookieHeaderValues = headers[HeaderNames.SetCookie]; + Assert.Single(cookieHeaderValues); + Assert.StartsWith(testCookie, cookieHeaderValues[0]); + Assert.Contains("path=/", cookieHeaderValues[0]); + Assert.Contains("samesite=none", cookieHeaderValues[0]); + Assert.DoesNotContain("secure", cookieHeaderValues[0]); + + var writeContext = Assert.Single(sink.Writes); + Assert.Equal("The cookie 'TestCookie' has set 'SameSite=None' and must also set 'Secure'.", writeContext.Message); + } + [Fact] public void DeleteCookieShouldSetDefaultPath() { var headers = new HeaderDictionary(); - var cookies = new ResponseCookies(headers); + var features = MakeFeatures(headers); + var cookies = new ResponseCookies(features); var testCookie = "TestCookie"; cookies.Delete(testCookie); @@ -29,7 +78,8 @@ public void DeleteCookieShouldSetDefaultPath() public void DeleteCookieWithCookieOptionsShouldKeepPropertiesOfCookieOptions() { var headers = new HeaderDictionary(); - var cookies = new ResponseCookies(headers); + var features = MakeFeatures(headers); + var cookies = new ResponseCookies(features); var testCookie = "TestCookie"; var time = new DateTimeOffset(2000, 1, 1, 1, 1, 1, 1, TimeSpan.Zero); var options = new CookieOptions @@ -58,7 +108,8 @@ public void DeleteCookieWithCookieOptionsShouldKeepPropertiesOfCookieOptions() public void NoParamsDeleteRemovesCookieCreatedByAdd() { var headers = new HeaderDictionary(); - var cookies = new ResponseCookies(headers); + var features = MakeFeatures(headers); + var cookies = new ResponseCookies(features); var testCookie = "TestCookie"; cookies.Append(testCookie, testCookie); @@ -75,7 +126,8 @@ public void NoParamsDeleteRemovesCookieCreatedByAdd() public void ProvidesMaxAgeWithCookieOptionsArgumentExpectMaxAgeToBeSet() { var headers = new HeaderDictionary(); - var cookies = new ResponseCookies(headers); + var features = MakeFeatures(headers); + var cookies = new ResponseCookies(features); var cookieOptions = new CookieOptions(); var maxAgeTime = TimeSpan.FromHours(1); cookieOptions.MaxAge = TimeSpan.FromHours(1); @@ -96,7 +148,8 @@ public void ProvidesMaxAgeWithCookieOptionsArgumentExpectMaxAgeToBeSet() public void EscapesValuesBeforeSettingCookie(string value, string expected) { var headers = new HeaderDictionary(); - var cookies = new ResponseCookies(headers); + var features = MakeFeatures(headers); + var cookies = new ResponseCookies(features); cookies.Append("key", value); @@ -111,7 +164,8 @@ public void EscapesValuesBeforeSettingCookie(string value, string expected) public void InvalidKeysThrow(string key) { var headers = new HeaderDictionary(); - var cookies = new ResponseCookies(headers); + var features = MakeFeatures(headers); + var cookies = new ResponseCookies(features); Assert.Throws(() => cookies.Append(key, "1")); } @@ -124,7 +178,8 @@ public void InvalidKeysThrow(string key) public void AppContextSwitchEscapesKeysAndValuesBeforeSettingCookie(string key, string value, string expected) { var headers = new HeaderDictionary(); - var cookies = new ResponseCookies(headers); + var features = MakeFeatures(headers); + var cookies = new ResponseCookies(features); cookies._enableCookieNameEncoding = true; cookies.Append(key, value);