Log for SameSite=None without Secure (#24970)

* Log for SameSite=None without Secure

* Update src/Http/Http/src/Internal/EventIds.cs

Co-authored-by: campersau <buchholz.bastian@googlemail.com>

Co-authored-by: campersau <buchholz.bastian@googlemail.com>

src/Http/Http/src/Features/ResponseCookiesFeature.cs

@@ -15,7 +15,7 @@ namespace Microsoft.AspNetCore.Http.Features
         // Lambda hoisted to static readonly field to improve inlining https://github.com/dotnet/roslyn/issues/13624
         private readonly static Func<IFeatureCollection, IHttpResponseFeature?> _nullResponseFeature = f => null;
 
-        private FeatureReferences<IHttpResponseFeature> _features;
+        private readonly IFeatureCollection _features;
         private IResponseCookies? _cookiesCollection;
 
         /// <summary>
@@ -27,12 +27,7 @@ namespace Microsoft.AspNetCore.Http.Features
         /// </param>
         public ResponseCookiesFeature(IFeatureCollection features)
         {
-            if (features == null)
+            _features = features ?? throw new ArgumentNullException(nameof(features));
-            {
-                throw new ArgumentNullException(nameof(features));
-            }
-
-            _features.Initalize(features);
         }
 
         /// <summary>
@@ -46,16 +41,9 @@ namespace Microsoft.AspNetCore.Http.Features
         [Obsolete("This constructor is obsolete and will be removed in a future version.")]
         public ResponseCookiesFeature(IFeatureCollection features, ObjectPool<StringBuilder>? builderPool)
         {
-            if (features == null)
+            _features = features ?? throw new ArgumentNullException(nameof(features));
-            {
-                throw new ArgumentNullException(nameof(features));
-            }
-
-            _features.Initalize(features);
         }
 
-        private IHttpResponseFeature HttpResponseFeature => _features.Fetch(ref _features.Cache, _nullResponseFeature)!;
-
         /// <inheritdoc />
         public IResponseCookies Cookies
         {
@@ -63,8 +51,7 @@ namespace Microsoft.AspNetCore.Http.Features
             {
                 if (_cookiesCollection == null)
                 {
-                    var headers = HttpResponseFeature.Headers;
+                    _cookiesCollection = new ResponseCookies(_features);
-                    _cookiesCollection = new ResponseCookies(headers);
                 }
 
                 return _cookiesCollection;

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");
+    }
+}

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 @@ namespace Microsoft.AspNetCore.Http
         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;
+
         /// <summary>
         /// Create a new wrapper.
         /// </summary>
-        /// <param name="headers">The <see cref="IHeaderDictionary"/> for the response.</param>
+        internal ResponseCookies(IFeatureCollection features)
-        public ResponseCookies(IHeaderDictionary headers)
         {
-            if (headers == null)
+            _features = features;
-            {
+            Headers = _features.Get<IHttpResponseFeature>().Headers;
-                throw new ArgumentNullException(nameof(headers));
-            }
-
-            Headers = headers;
         }
 
         private IHeaderDictionary Headers { get; set; }
@@ -54,6 +55,21 @@ namespace Microsoft.AspNetCore.Http
                 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<Features.IServiceProvidersFeature>()?.RequestServices;
+                    _logger = services?.GetService<ILogger<ResponseCookies>>();
+                }
+
+                if (_logger != null)
+                {
+                    Log.SameSiteCookieNotSecure(_logger, key);
+                }
+            }
+
             var setCookieHeaderValue = new SetCookieHeaderValue(
                 _enableCookieNameEncoding ? Uri.EscapeDataString(key) : key,
                 Uri.EscapeDataString(value))
@@ -135,5 +151,18 @@ namespace Microsoft.AspNetCore.Http
                 SameSite = options.SameSite
             });
         }
+
+        private static class Log
+        {
+            private static readonly Action<ILogger, string, Exception?> _samesiteNotSecure = LoggerMessage.Define<string>(
+                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);
+            }
+        }
     }
 }

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<IHttpResponseFeature>(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<ResponseCookies>);
+            var loggerFactory = new TestLoggerFactory(sink, enabled: true);
+            services.AddLogging();
+            services.AddSingleton<ILoggerFactory>(loggerFactory);
+
+            features.Set<IServiceProvidersFeature>(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 @@ namespace Microsoft.AspNetCore.Http.Tests
         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 @@ namespace Microsoft.AspNetCore.Http.Tests
         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 @@ namespace Microsoft.AspNetCore.Http.Tests
         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 @@ namespace Microsoft.AspNetCore.Http.Tests
         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 @@ namespace Microsoft.AspNetCore.Http.Tests
         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<ArgumentException>(() => cookies.Append(key, "1"));
         }
@@ -124,7 +178,8 @@ namespace Microsoft.AspNetCore.Http.Tests
         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);