From c5238390787afb31cc9be15f557ffd9dcc42b049 Mon Sep 17 00:00:00 2001 From: John Luo Date: Wed, 24 May 2017 17:04:14 -0700 Subject: [PATCH] Update default settings for SameSite - Need Lax policy for social authentication - Need None policy for OIDC --- .gitignore | 1 + .../OpenIdConnect.AzureAdSample/Program.cs | 1 + .../CookieAuthenticationOptions.cs | 18 +++--- .../OpenIdConnectHandler.cs | 6 +- .../TwitterHandler.cs | 2 +- .../RemoteAuthenticationHandler.cs | 8 +-- .../CookiePolicyOptions.cs | 2 +- .../CookiePolicyTests.cs | 56 +++++++++---------- 8 files changed, 49 insertions(+), 45 deletions(-) diff --git a/.gitignore b/.gitignore index bcc811de9..d5717b3f3 100644 --- a/.gitignore +++ b/.gitignore @@ -28,4 +28,5 @@ project.lock.json .build/ .testPublish/ /.vs/ +.vscode/ global.json diff --git a/samples/OpenIdConnect.AzureAdSample/Program.cs b/samples/OpenIdConnect.AzureAdSample/Program.cs index 9de0185a4..0e1285a9c 100644 --- a/samples/OpenIdConnect.AzureAdSample/Program.cs +++ b/samples/OpenIdConnect.AzureAdSample/Program.cs @@ -15,6 +15,7 @@ public static void Main(string[] args) factory.AddFilter("Console", level => level >= LogLevel.Information); }) .UseKestrel() + .UseUrls("http://localhost:42023") .UseContentRoot(Directory.GetCurrentDirectory()) .UseIISIntegration() .UseStartup() diff --git a/src/Microsoft.AspNetCore.Authentication.Cookies/CookieAuthenticationOptions.cs b/src/Microsoft.AspNetCore.Authentication.Cookies/CookieAuthenticationOptions.cs index 02d0361b7..01a5ae9c9 100644 --- a/src/Microsoft.AspNetCore.Authentication.Cookies/CookieAuthenticationOptions.cs +++ b/src/Microsoft.AspNetCore.Authentication.Cookies/CookieAuthenticationOptions.cs @@ -22,7 +22,8 @@ public CookieAuthenticationOptions() ReturnUrlParameter = CookieAuthenticationDefaults.ReturnUrlParameter; ExpireTimeSpan = TimeSpan.FromDays(14); SlidingExpiration = true; - CookieSameSite = SameSiteMode.Strict; + // To support OAuth authentication, a lax mode is required, see https://github.com/aspnet/Security/issues/1231. + CookieSameSite = SameSiteMode.Lax; CookieHttpOnly = true; CookieSecure = CookieSecurePolicy.SameAsRequest; Events = new CookieAuthenticationEvents(); @@ -59,7 +60,8 @@ public string CookieName /// /// Determines if the browser should allow the cookie to be attached to same-site or cross-site requests. The - /// default is Strict, which means the cookie is only allowed to be attached to same-site requests. + /// default is Lax, which means the cookie is only allowed to be attached to cross-site requests using safe + /// HTTP methods and same-site requests. /// public SameSiteMode CookieSameSite { get; set; } @@ -84,8 +86,8 @@ public string CookieName /// /// Controls how much time the cookie will remain valid from the point it is created. The expiration - /// information is in the protected cookie ticket. Because of that an expired cookie will be ignored - /// even if it is passed to the server after the browser should have purged it + /// information is in the protected cookie ticket. Because of that an expired cookie will be ignored + /// even if it is passed to the server after the browser should have purged it /// public TimeSpan ExpireTimeSpan { get; set; } @@ -99,7 +101,7 @@ public string CookieName /// The LoginPath property informs the handler that it should change an outgoing 401 Unauthorized status /// code into a 302 redirection onto the given login path. The current url which generated the 401 is added /// to the LoginPath as a query string parameter named by the ReturnUrlParameter. Once a request to the - /// LoginPath grants a new SignIn identity, the ReturnUrlParameter value is used to redirect the browser back + /// LoginPath grants a new SignIn identity, the ReturnUrlParameter value is used to redirect the browser back /// to the url which caused the original unauthorized status code. /// public PathString LoginPath { get; set; } @@ -117,15 +119,15 @@ public string CookieName /// /// The ReturnUrlParameter determines the name of the query string parameter which is appended by the handler - /// when a 401 Unauthorized status code is changed to a 302 redirect onto the login path. This is also the query - /// string parameter looked for when a request arrives on the login path or logout path, in order to return to the + /// when a 401 Unauthorized status code is changed to a 302 redirect onto the login path. This is also the query + /// string parameter looked for when a request arrives on the login path or logout path, in order to return to the /// original url after the action is performed. /// public string ReturnUrlParameter { get; set; } /// /// The Provider may be assigned to an instance of an object created by the application at startup time. The handler - /// calls methods on the provider which give the application control at certain points where processing is occurring. + /// calls methods on the provider which give the application control at certain points where processing is occurring. /// If it is not provided a default instance is supplied which does nothing when the methods are called. /// public new CookieAuthenticationEvents Events diff --git a/src/Microsoft.AspNetCore.Authentication.OpenIdConnect/OpenIdConnectHandler.cs b/src/Microsoft.AspNetCore.Authentication.OpenIdConnect/OpenIdConnectHandler.cs index 9c8f4ecc6..89d949a00 100644 --- a/src/Microsoft.AspNetCore.Authentication.OpenIdConnect/OpenIdConnectHandler.cs +++ b/src/Microsoft.AspNetCore.Authentication.OpenIdConnect/OpenIdConnectHandler.cs @@ -62,7 +62,7 @@ public OpenIdConnectHandler(IOptionsSnapshot options, ILog } /// - /// The handler calls methods on the events which give the application control at certain points where processing is occurring. + /// The handler calls methods on the events which give the application control at certain points where processing is occurring. /// If it is not provided a default instance is supplied which does nothing when the methods are called. /// protected new OpenIdConnectEvents Events @@ -892,7 +892,7 @@ private void WriteNonceCookie(string nonce) new CookieOptions { HttpOnly = true, - SameSite = Http.SameSiteMode.Lax, + SameSite = Http.SameSiteMode.None, Secure = Request.IsHttps, Expires = Clock.UtcNow.Add(Options.ProtocolValidator.NonceLifetime) }); @@ -924,7 +924,7 @@ private string ReadNonceCookie(string nonce) var cookieOptions = new CookieOptions { HttpOnly = true, - SameSite = Http.SameSiteMode.Lax, + SameSite = Http.SameSiteMode.None, Secure = Request.IsHttps }; diff --git a/src/Microsoft.AspNetCore.Authentication.Twitter/TwitterHandler.cs b/src/Microsoft.AspNetCore.Authentication.Twitter/TwitterHandler.cs index 63a18e83b..0eab83e41 100644 --- a/src/Microsoft.AspNetCore.Authentication.Twitter/TwitterHandler.cs +++ b/src/Microsoft.AspNetCore.Authentication.Twitter/TwitterHandler.cs @@ -31,7 +31,7 @@ internal class TwitterHandler : RemoteAuthenticationHandler private HttpClient Backchannel => Options.Backchannel; /// - /// The handler calls methods on the events which give the application control at certain points where processing is occurring. + /// The handler calls methods on the events which give the application control at certain points where processing is occurring. /// If it is not provided a default instance is supplied which does nothing when the methods are called. /// protected new TwitterEvents Events diff --git a/src/Microsoft.AspNetCore.Authentication/RemoteAuthenticationHandler.cs b/src/Microsoft.AspNetCore.Authentication/RemoteAuthenticationHandler.cs index 9394b7553..2ff06062d 100644 --- a/src/Microsoft.AspNetCore.Authentication/RemoteAuthenticationHandler.cs +++ b/src/Microsoft.AspNetCore.Authentication/RemoteAuthenticationHandler.cs @@ -12,7 +12,7 @@ namespace Microsoft.AspNetCore.Authentication { - public abstract class RemoteAuthenticationHandler : AuthenticationHandler, IAuthenticationRequestHandler + public abstract class RemoteAuthenticationHandler : AuthenticationHandler, IAuthenticationRequestHandler where TOptions : RemoteAuthenticationOptions, new() { private const string CorrelationPrefix = ".AspNetCore.Correlation."; @@ -25,7 +25,7 @@ public abstract class RemoteAuthenticationHandler : AuthenticationHand protected string SignInScheme => Options.SignInScheme; /// - /// The handler calls methods on the events which give the application control at certain points where processing is occurring. + /// The handler calls methods on the events which give the application control at certain points where processing is occurring. /// If it is not provided a default instance is supplied which does nothing when the methods are called. /// protected new RemoteAuthenticationEvents Events @@ -203,7 +203,7 @@ protected virtual void GenerateCorrelationId(AuthenticationProperties properties var cookieOptions = new CookieOptions { HttpOnly = true, - SameSite = SameSiteMode.Lax, + SameSite = SameSiteMode.None, Secure = Request.IsHttps, Expires = Clock.UtcNow.Add(Options.RemoteAuthenticationTimeout), }; @@ -243,7 +243,7 @@ protected virtual bool ValidateCorrelationId(AuthenticationProperties properties var cookieOptions = new CookieOptions { HttpOnly = true, - SameSite = SameSiteMode.Lax, + SameSite = SameSiteMode.None, Secure = Request.IsHttps }; Response.Cookies.Delete(cookieName, cookieOptions); diff --git a/src/Microsoft.AspNetCore.CookiePolicy/CookiePolicyOptions.cs b/src/Microsoft.AspNetCore.CookiePolicy/CookiePolicyOptions.cs index 7203e73e6..1e474bfe2 100644 --- a/src/Microsoft.AspNetCore.CookiePolicy/CookiePolicyOptions.cs +++ b/src/Microsoft.AspNetCore.CookiePolicy/CookiePolicyOptions.cs @@ -15,7 +15,7 @@ public class CookiePolicyOptions /// /// Affects the cookie's same site attribute. /// - public SameSiteMode MinimumSameSitePolicy { get; set; } = SameSiteMode.Strict; + public SameSiteMode MinimumSameSitePolicy { get; set; } = SameSiteMode.Lax; /// /// Affects whether cookies must be HttpOnly. diff --git a/test/Microsoft.AspNetCore.CookiePolicy.Test/CookiePolicyTests.cs b/test/Microsoft.AspNetCore.CookiePolicy.Test/CookiePolicyTests.cs index 737c12dc3..b6b2776a8 100644 --- a/test/Microsoft.AspNetCore.CookiePolicy.Test/CookiePolicyTests.cs +++ b/test/Microsoft.AspNetCore.CookiePolicy.Test/CookiePolicyTests.cs @@ -59,10 +59,10 @@ await RunTest("/secureAlways", transaction => { Assert.NotNull(transaction.SetCookie); - Assert.Equal("A=A; path=/; secure; samesite=strict", transaction.SetCookie[0]); - Assert.Equal("B=B; path=/; secure; samesite=strict", transaction.SetCookie[1]); - Assert.Equal("C=C; path=/; secure; samesite=strict", transaction.SetCookie[2]); - Assert.Equal("D=D; path=/; secure; samesite=strict", transaction.SetCookie[3]); + Assert.Equal("A=A; path=/; secure; samesite=lax", transaction.SetCookie[0]); + Assert.Equal("B=B; path=/; secure; samesite=lax", transaction.SetCookie[1]); + Assert.Equal("C=C; path=/; secure; samesite=lax", transaction.SetCookie[2]); + Assert.Equal("D=D; path=/; secure; samesite=lax", transaction.SetCookie[3]); })); } @@ -79,10 +79,10 @@ await RunTest("/secureNone", transaction => { Assert.NotNull(transaction.SetCookie); - Assert.Equal("A=A; path=/; samesite=strict", transaction.SetCookie[0]); - Assert.Equal("B=B; path=/; samesite=strict", transaction.SetCookie[1]); - Assert.Equal("C=C; path=/; samesite=strict", transaction.SetCookie[2]); - Assert.Equal("D=D; path=/; secure; samesite=strict", transaction.SetCookie[3]); + Assert.Equal("A=A; path=/; samesite=lax", transaction.SetCookie[0]); + Assert.Equal("B=B; path=/; samesite=lax", transaction.SetCookie[1]); + Assert.Equal("C=C; path=/; samesite=lax", transaction.SetCookie[2]); + Assert.Equal("D=D; path=/; secure; samesite=lax", transaction.SetCookie[3]); })); } @@ -99,19 +99,19 @@ await RunTest("/secureSame", transaction => { Assert.NotNull(transaction.SetCookie); - Assert.Equal("A=A; path=/; samesite=strict", transaction.SetCookie[0]); - Assert.Equal("B=B; path=/; samesite=strict", transaction.SetCookie[1]); - Assert.Equal("C=C; path=/; samesite=strict", transaction.SetCookie[2]); - Assert.Equal("D=D; path=/; samesite=strict", transaction.SetCookie[3]); + Assert.Equal("A=A; path=/; samesite=lax", transaction.SetCookie[0]); + Assert.Equal("B=B; path=/; samesite=lax", transaction.SetCookie[1]); + Assert.Equal("C=C; path=/; samesite=lax", transaction.SetCookie[2]); + Assert.Equal("D=D; path=/; samesite=lax", transaction.SetCookie[3]); }), new RequestTest("https://example.com/secureSame", transaction => { Assert.NotNull(transaction.SetCookie); - Assert.Equal("A=A; path=/; secure; samesite=strict", transaction.SetCookie[0]); - Assert.Equal("B=B; path=/; secure; samesite=strict", transaction.SetCookie[1]); - Assert.Equal("C=C; path=/; secure; samesite=strict", transaction.SetCookie[2]); - Assert.Equal("D=D; path=/; secure; samesite=strict", transaction.SetCookie[3]); + Assert.Equal("A=A; path=/; secure; samesite=lax", transaction.SetCookie[0]); + Assert.Equal("B=B; path=/; secure; samesite=lax", transaction.SetCookie[1]); + Assert.Equal("C=C; path=/; secure; samesite=lax", transaction.SetCookie[2]); + Assert.Equal("D=D; path=/; secure; samesite=lax", transaction.SetCookie[3]); })); } @@ -128,10 +128,10 @@ await RunTest("/httpOnlyAlways", transaction => { Assert.NotNull(transaction.SetCookie); - Assert.Equal("A=A; path=/; samesite=strict; httponly", transaction.SetCookie[0]); - Assert.Equal("B=B; path=/; samesite=strict; httponly", transaction.SetCookie[1]); - Assert.Equal("C=C; path=/; samesite=strict; httponly", transaction.SetCookie[2]); - Assert.Equal("D=D; path=/; samesite=strict; httponly", transaction.SetCookie[3]); + Assert.Equal("A=A; path=/; samesite=lax; httponly", transaction.SetCookie[0]); + Assert.Equal("B=B; path=/; samesite=lax; httponly", transaction.SetCookie[1]); + Assert.Equal("C=C; path=/; samesite=lax; httponly", transaction.SetCookie[2]); + Assert.Equal("D=D; path=/; samesite=lax; httponly", transaction.SetCookie[3]); })); } @@ -148,10 +148,10 @@ await RunTest("/httpOnlyNone", transaction => { Assert.NotNull(transaction.SetCookie); - Assert.Equal("A=A; path=/; samesite=strict", transaction.SetCookie[0]); - Assert.Equal("B=B; path=/; samesite=strict", transaction.SetCookie[1]); - Assert.Equal("C=C; path=/; samesite=strict", transaction.SetCookie[2]); - Assert.Equal("D=D; path=/; samesite=strict; httponly", transaction.SetCookie[3]); + Assert.Equal("A=A; path=/; samesite=lax", transaction.SetCookie[0]); + Assert.Equal("B=B; path=/; samesite=lax", transaction.SetCookie[1]); + Assert.Equal("C=C; path=/; samesite=lax", transaction.SetCookie[2]); + Assert.Equal("D=D; path=/; samesite=lax; httponly", transaction.SetCookie[3]); })); } @@ -242,10 +242,10 @@ public async Task CookiePolicyCanHijackAppend() var transaction = await server.SendAsync("http://example.com/login"); Assert.NotNull(transaction.SetCookie); - Assert.Equal("Hao=Hao; path=/; samesite=strict", transaction.SetCookie[0]); - Assert.Equal("Hao=Hao; path=/; samesite=strict", transaction.SetCookie[1]); - Assert.Equal("Hao=Hao; path=/; samesite=strict", transaction.SetCookie[2]); - Assert.Equal("Hao=Hao; path=/; secure; samesite=strict", transaction.SetCookie[3]); + Assert.Equal("Hao=Hao; path=/; samesite=lax", transaction.SetCookie[0]); + Assert.Equal("Hao=Hao; path=/; samesite=lax", transaction.SetCookie[1]); + Assert.Equal("Hao=Hao; path=/; samesite=lax", transaction.SetCookie[2]); + Assert.Equal("Hao=Hao; path=/; secure; samesite=lax", transaction.SetCookie[3]); } [Fact]