Disable cookies by default (Azure#26477)

JoshLove-msft · web-flow · commit 88fe50a7ac6c · 2022-01-20T23:26:20.000Z
* Disable cookies by default

* PR fb

* PR fb

* fix

* tab

* Fix race condition

* Specify cert path

* Add constants

* Move to ctor
diff --git a/sdk/core/Azure.Core.TestFramework/src/TestEnvironment.cs b/sdk/core/Azure.Core.TestFramework/src/TestEnvironment.cs
@@ -28,6 +28,10 @@ public abstract class TestEnvironment
         [EditorBrowsableAttribute(EditorBrowsableState.Never)]
         public static string RepositoryRoot { get; }
 
+        public static string DevCertPath { get; }
+
+        public const string DevCertPassword = "password";
+
         private static readonly Dictionary<Type, Task> s_environmentStateCache = new();
 
         private readonly string _prefix;
@@ -122,6 +126,13 @@ static TestEnvironment()
             }
 
             RepositoryRoot = directoryInfo?.Parent?.FullName;
+
+            DevCertPath = Path.Combine(
+                RepositoryRoot,
+                "eng",
+                "common",
+                "testproxy",
+                "dotnet-devcert.pfx");
         }
 
         public RecordedTestMode? Mode { get; set; }
diff --git a/sdk/core/Azure.Core.TestFramework/src/TestProxy.cs b/sdk/core/Azure.Core.TestFramework/src/TestProxy.cs
@@ -73,13 +73,8 @@ private TestProxy(string proxyPath)
                     ["ASPNETCORE_URLS"] = $"http://{IpAddress}:0;https://{IpAddress}:0",
                     ["Logging__LogLevel__Default"] = "Error",
                     ["Logging__LogLevel__Microsoft.Hosting.Lifetime"] = "Information",
-                    ["ASPNETCORE_Kestrel__Certificates__Default__Path"] = Path.Combine(
-                        TestEnvironment.RepositoryRoot,
-                        "eng",
-                        "common",
-                        "testproxy",
-                        "dotnet-devcert.pfx"),
-                    ["ASPNETCORE_Kestrel__Certificates__Default__Password"] = "password"
+                    ["ASPNETCORE_Kestrel__Certificates__Default__Path"] = TestEnvironment.DevCertPath,
+                    ["ASPNETCORE_Kestrel__Certificates__Default__Password"] = TestEnvironment.DevCertPassword
                 }
             };
 
diff --git a/sdk/core/Azure.Core.TestFramework/src/TestServer.cs b/sdk/core/Azure.Core.TestFramework/src/TestServer.cs
@@ -2,6 +2,7 @@
 // Licensed under the MIT License.
 
 using System;
+using System.IO;
 using System.Linq;
 using System.Net;
 using System.Reflection;
@@ -36,7 +37,7 @@ public TestServer(RequestDelegate app, bool https = false)
                     {
                         if (https)
                         {
-                            listenOptions.UseHttps();
+                            listenOptions.UseHttps(TestEnvironment.DevCertPath, TestEnvironment.DevCertPassword);
                         }
                     });
                 })
diff --git a/sdk/core/Azure.Core/CHANGELOG.md b/sdk/core/Azure.Core/CHANGELOG.md
@@ -6,6 +6,13 @@
 
 ### Breaking Changes
 
+- Cookies are no longer set on requests by default. Cookies can be re-enabled for `HttpClientTransport` by either setting an AppContext switch named "Azure.Core.Pipeline.HttpClientTransport.EnableCookies" to true or by setting the environment variable, "AZURE_CORE_HTTPCLIENT_ENABLE_COOKIES" to "true". Note: AppContext switches can also be configured via configuration like below:
+```xml
+  <ItemGroup>
+    <RuntimeHostConfigurationOption Include="Azure.Core.Pipeline.HttpClientTransport.EnableCookies" Value="true" />
+  </ItemGroup>
+```
+
 ### Bugs Fixed
 
 ### Other Changes
diff --git a/sdk/core/Azure.Core/src/Pipeline/HttpClientTransport.cs b/sdk/core/Azure.Core/src/Pipeline/HttpClientTransport.cs
@@ -159,13 +159,14 @@ private static HttpMessageHandler CreateDefaultHandler(HttpPipelineTransportOpti
         {
             if (RuntimeInformation.IsOSPlatform(OSPlatform.Create("BROWSER")))
             {
+                // UseCookies is not supported on "browser"
                 return new HttpClientHandler();
             }
 
 #if NETCOREAPP
-            return ApplyOptionsToHandler(new SocketsHttpHandler { AllowAutoRedirect = false }, options);
+            return ApplyOptionsToHandler(new SocketsHttpHandler { AllowAutoRedirect = false, UseCookies = UseCookies() }, options);
 #else
-            return ApplyOptionsToHandler(new HttpClientHandler { AllowAutoRedirect = false }, options);
+            return ApplyOptionsToHandler(new HttpClientHandler { AllowAutoRedirect = false, UseCookies = UseCookies() }, options);
 #endif
         }
 
@@ -613,5 +614,9 @@ private static void SetPropertiesOrOptions<T>(HttpRequestMessage httpRequest, st
             httpRequest.Properties[name] = value;
 #endif
         }
+
+        private static bool UseCookies() => AppContextSwitchHelper.GetConfigValue(
+            "Azure.Core.Pipeline.HttpClientTransport.EnableCookies",
+            "AZURE_CORE_HTTPCLIENT_ENABLE_COOKIES");
     }
 }
diff --git a/sdk/core/Azure.Core/tests/HttpClientTransportFunctionalTest.cs b/sdk/core/Azure.Core/tests/HttpClientTransportFunctionalTest.cs
@@ -3,6 +3,7 @@
 
 using System;
 using System.Collections;
+using System.Linq;
 using System.Net;
 using System.Net.Http;
 using System.Net.Security;
@@ -12,10 +13,13 @@
 using System.Threading;
 using System.Threading.Tasks;
 using Azure.Core.Pipeline;
+using Azure.Core.TestFramework;
+using Microsoft.AspNetCore.Http;
 using NUnit.Framework;
 
 namespace Azure.Core.Tests
 {
+    [NonParallelizable]
     public class HttpClientTransportFunctionalTest : TransportFunctionalTests
     {
         private static RemoteCertificateValidationCallback certCallback = (_, _, _, _) => true;
@@ -34,6 +38,7 @@ protected override HttpPipelineTransport GetTransport(bool https = false, HttpPi
         }
 
 #if NET461
+        // These setup and teardown actions require that entire test class be NonParallelizable.
         [OneTimeSetUp]
         public void TestSetup()
         {
@@ -86,6 +91,59 @@ public void DisposeDoesNotDisposesSharedPipeline()
             Assert.DoesNotThrow(() => transport.Client.CancelPendingRequests());
         }
 
+        [Test]
+        public async Task CookiesCanBeEnabledUsingSwitch()
+        {
+            using var appContextSwitch = new TestAppContextSwitch("Azure.Core.Pipeline.HttpClientTransport.EnableCookies", "true");
+
+            await TestCookiesEnabled();
+        }
+
+        [Test]
+        public async Task CookiesCanBeEnabledUsingEnvVar()
+        {
+            using var envVar = new TestEnvVar("AZURE_CORE_HTTPCLIENT_ENABLE_COOKIES", "true");
+
+            await TestCookiesEnabled();
+        }
+
+        private async Task TestCookiesEnabled()
+        {
+            int requestCount = 0;
+            using (TestServer testServer = new TestServer(
+                async context =>
+                {
+                    if (requestCount++ == 1)
+                    {
+                        Assert.IsTrue(context.Request.Headers.ContainsKey("cookie"));
+                        Assert.AreEqual("stsservicecookie=estsfd",
+                            context.Request.Headers["cookie"].First());
+                    }
+
+                    context.Response.StatusCode = 200;
+                    context.Response.Headers.Add(
+                        "set-cookie",
+                        "stsservicecookie=estsfd; path=/; secure; samesite=none; httponly");
+                    await context.Response.WriteAsync("");
+                },
+                https: true))
+            {
+                var transport = GetTransport(https: true);
+                Request request = transport.CreateRequest();
+                request.Method = RequestMethod.Post;
+                request.Uri.Reset(testServer.Address);
+                request.Content = RequestContent.Create("Hello");
+                await ExecuteRequest(request, transport);
+
+                // create a second request to verify cookies not set
+                request = transport.CreateRequest();
+                request.Method = RequestMethod.Post;
+                request.Uri.Reset(testServer.Address);
+                request.Content = RequestContent.Create("Hello");
+                await ExecuteRequest(request, transport);
+            }
+        }
+
         private static object GetHandler(HttpClient transportClient)
         {
             return typeof(HttpMessageInvoker).GetField("_handler", BindingFlags.Instance | BindingFlags.NonPublic).GetValue(transportClient);
diff --git a/sdk/core/Azure.Core/tests/TransportFunctionalTests.cs b/sdk/core/Azure.Core/tests/TransportFunctionalTests.cs
@@ -881,7 +881,6 @@ public void TransportExceptionsAreWrappedAndInnerExceptionIsPopulated()
         public Task ThrowsTaskCanceledExceptionWhenCancelled() => ThrowsTaskCanceledExceptionWhenCancelled(false);
 
         [Test]
-        [RunOnlyOnPlatforms(Linux = true, Windows = true, OSX = false, Reason = "https://github.com/Azure/azure-sdk-for-net/issues/17986")]
         public Task ThrowsTaskCanceledExceptionWhenCancelledHttps() => ThrowsTaskCanceledExceptionWhenCancelled(true);
 
         private async Task ThrowsTaskCanceledExceptionWhenCancelled(bool https)
@@ -924,7 +923,6 @@ private async Task ThrowsTaskCanceledExceptionWhenCancelled(bool https)
         public Task CanCancelContentUpload() => CanCancelContentUpload(false);
 
         [Test]
-        [RunOnlyOnPlatforms(Linux = true, Windows = true, OSX = false, Reason = "https://github.com/Azure/azure-sdk-for-net/issues/17986")]
         public Task CanCancelContentUploadHttps() => CanCancelContentUpload(true);
 
         private async Task CanCancelContentUpload(bool https)
@@ -995,7 +993,6 @@ public async Task StreamReadingExceptionsAreIOExceptions()
         }
 
         [Test]
-        [RunOnlyOnPlatforms(Linux = true, Windows = true, OSX = false, Reason = "https://github.com/Azure/azure-sdk-for-net/issues/17986")]
         public async Task ServerCertificateCustomValidationCallbackIsHonored([Values(true, false)] bool setCertCallback, [Values(true, false)] bool isValidCert)
         {
             // This test assumes ServicePointManager.ServerCertificateValidationCallback will be unset.
@@ -1114,6 +1111,35 @@ public async Task CanSendExpect100Continue()
             }
         }
 
+        [Test]
+        public async Task CookiesDisabledByDefault()
+        {
+            using (TestServer testServer = new TestServer(
+                context =>
+                {
+                    Assert.IsFalse(context.Request.Headers.ContainsKey("cookie"));
+                    context.Response.StatusCode = 200;
+                    context.Response.Headers.Add(
+                        "set-cookie",
+                        "stsservicecookie=estsfd; path=/; secure; samesite=none; httponly");
+                }))
+            {
+                var transport = GetTransport();
+                Request request = transport.CreateRequest();
+                request.Method = RequestMethod.Post;
+                request.Uri.Reset(testServer.Address);
+                request.Content = RequestContent.Create("Hello");
+                await ExecuteRequest(request, transport);
+
+                // create a second request to verify cookies not set
+                request = transport.CreateRequest();
+                request.Method = RequestMethod.Post;
+                request.Uri.Reset(testServer.Address);
+                request.Content = RequestContent.Create("Hello");
+                await ExecuteRequest(request, transport);
+            }
+        }
+
         private static Request CreateRequest(HttpPipelineTransport transport, TestServer server, byte[] bytes = null)
         {
             Request request = transport.CreateRequest();

Original file line number	Diff line number	Diff line change
`@@ -2,6 +2,7 @@`
`2`	`2`	`// Licensed under the MIT License.`
`3`	`3`
`4`	`4`	`using System;`
	`5`	`+using System.IO;`
`5`	`6`	`using System.Linq;`
`6`	`7`	`using System.Net;`
`7`	`8`	`using System.Reflection;`
`@@ -36,7 +37,7 @@ public TestServer(RequestDelegate app, bool https = false)`
`36`	`37`	`{`
`37`	`38`	`if (https)`
`38`	`39`	`{`
`39`		`- listenOptions.UseHttps();`
	`40`	`+ listenOptions.UseHttps(TestEnvironment.DevCertPath, TestEnvironment.DevCertPassword);`
`40`	`41`	`}`
`41`	`42`	`});`
`42`	`43`	`})`
Original file line number	Diff line number	Diff line change
`@@ -159,13 +159,14 @@ private static HttpMessageHandler CreateDefaultHandler(HttpPipelineTransportOpti`
`159`	`159`	`{`
`160`	`160`	`if (RuntimeInformation.IsOSPlatform(OSPlatform.Create("BROWSER")))`
`161`	`161`	`{`
	`162`	`+ // UseCookies is not supported on "browser"`
`162`	`163`	`return new HttpClientHandler();`
`163`	`164`	`}`
`164`	`165`
`165`	`166`	`#if NETCOREAPP`
`166`		`- return ApplyOptionsToHandler(new SocketsHttpHandler { AllowAutoRedirect = false }, options);`
	`167`	`+ return ApplyOptionsToHandler(new SocketsHttpHandler { AllowAutoRedirect = false, UseCookies = UseCookies() }, options);`
`167`	`168`	`#else`
`168`		`- return ApplyOptionsToHandler(new HttpClientHandler { AllowAutoRedirect = false }, options);`
	`169`	`+ return ApplyOptionsToHandler(new HttpClientHandler { AllowAutoRedirect = false, UseCookies = UseCookies() }, options);`
`169`	`170`	`#endif`
`170`	`171`	`}`
`171`	`172`
`@@ -613,5 +614,9 @@ private static void SetPropertiesOrOptions<T>(HttpRequestMessage httpRequest, st`
`613`	`614`	`httpRequest.Properties[name] = value;`
`614`	`615`	`#endif`
`615`	`616`	`}`
	`617`	`+`
	`618`	`+ private static bool UseCookies() => AppContextSwitchHelper.GetConfigValue(`
	`619`	`+ "Azure.Core.Pipeline.HttpClientTransport.EnableCookies",`
	`620`	`+ "AZURE_CORE_HTTPCLIENT_ENABLE_COOKIES");`
`616`	`621`	`}`
`617`	`622`	`}`