Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Set the cookie "Secure" flag iff the ACS post back URL is HTTPS #1135

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions Sustainsys.Saml2.AspNetCore2/CommandResultExtensions.cs
Expand Up @@ -35,6 +35,7 @@ static class CommandResultExtensions
new CookieOptions()
{
HttpOnly = true,
Secure = commandResult.SetCookieSecureFlag,
// We are expecting a different site to POST back to us,
// so the ASP.Net Core default of Lax is not appropriate in this case
SameSite = SameSiteMode.None,
Expand Down
Expand Up @@ -42,7 +42,8 @@ public static void ApplyCookies(this CommandResult commandResult, HttpResponseBa
commandResult.SetCookieName,
protectedData)
{
HttpOnly = true
HttpOnly = true,
Secure = commandResult.SetCookieSecureFlag,
});
}

Expand Down
1 change: 1 addition & 0 deletions Sustainsys.Saml2.Owin/CommandResultExtensions.cs
Expand Up @@ -71,6 +71,7 @@ private static void ApplyCookies(CommandResult commandResult, IOwinContext conte
new CookieOptions()
{
HttpOnly = true,
Secure = commandResult.SetCookieSecureFlag,
});
}

Expand Down
5 changes: 5 additions & 0 deletions Sustainsys.Saml2/WebSSO/CommandResult.cs
Expand Up @@ -67,6 +67,11 @@ public class CommandResult
/// </summary>
public string SetCookieName { get; set; }

/// <summary>
/// Value of the "Secure" flag for the cookie (relevant if <see cref="SetCookieName"/> != null).
/// </summary>
public bool SetCookieSecureFlag { get; set; }

/// <summary>
/// SAML RelayState value
/// </summary>
Expand Down
2 changes: 2 additions & 0 deletions Sustainsys.Saml2/WebSSO/SignInCommand.cs
Expand Up @@ -157,6 +157,8 @@ private static CommandResult InitiateLoginToIdp(IOptions options, IDictionary<st

commandResult.RequestState = new StoredRequestState(
idp.EntityId, returnUrl, authnRequest.Id, relayData);

commandResult.SetCookieSecureFlag = string.Equals(urls.AssertionConsumerServiceUrl.Scheme, "https", StringComparison.OrdinalIgnoreCase);
commandResult.SetCookieName = StoredRequestState.CookieNameBase + authnRequest.RelayState;

options.Notifications.SignInCommandResultCreated(commandResult, relayData);
Expand Down
4 changes: 3 additions & 1 deletion Tests/AspNetCore2.Tests/CommandResultExtensionsTests.cs
Expand Up @@ -41,6 +41,7 @@ public async Task CommandResultExtensions_Apply()
HttpStatusCode = System.Net.HttpStatusCode.Redirect,
Location = new Uri(redirectLocation),
SetCookieName = "Saml2.123",
SetCookieSecureFlag = true,
RelayState = "123",
RequestState = state,
ContentType = "application/json",
Expand Down Expand Up @@ -78,7 +79,8 @@ public async Task CommandResultExtensions_Apply()
context.Response.Headers["Location"].SingleOrDefault()
.Should().Be(redirectLocation, "location header should be set");
context.Response.Cookies.Received().Append(
"Saml2.123", expectedCookieData, Arg.Is<CookieOptions>(co => co.HttpOnly && co.SameSite == SameSiteMode.None));
"Saml2.123", expectedCookieData, Arg.Is<CookieOptions>(
co => co.HttpOnly && co.Secure && co.SameSite == SameSiteMode.None));

context.Response.Cookies.Received().Delete("Clear-Cookie");

Expand Down
4 changes: 3 additions & 1 deletion Tests/HttpModule.Tests/CommandResultHttpTests.cs
Expand Up @@ -52,6 +52,7 @@ public void CommandResultHttp_Apply_SetsCookie()
new CommandResult()
{
SetCookieName = "CookieName",
SetCookieSecureFlag = true,
RequestState = new StoredRequestState(
new EntityId("http://idp.example.com"),
null,
Expand All @@ -64,7 +65,8 @@ public void CommandResultHttp_Apply_SetsCookie()
c.Name == "CookieName"
&& c.Value.All(ch => ch != '/' && ch != '+' && ch != '=')
&& new StoredRequestState(DecryptCookieData(c.Value)).Idp.Id == "http://idp.example.com"
&& c.HttpOnly == true));
&& c.HttpOnly == true
&& c.Secure == true));
}

private byte[] DecryptCookieData(string data)
Expand Down
5 changes: 3 additions & 2 deletions Tests/Owin.Tests/CommandResultExtensionsTests.cs
Expand Up @@ -62,7 +62,8 @@ public void CommandResultExtensions_Apply_Cookie()
new Uri("http://sp.example.com/loggedout"),
new Saml2Id("id123"),
null),
SetCookieName = "CookieName"
SetCookieName = "CookieName",
SetCookieSecureFlag = true,
};

var context = OwinTestHelpers.CreateOwinContext();
Expand All @@ -75,7 +76,7 @@ public void CommandResultExtensions_Apply_Cookie()
var protectedData = HttpRequestData.ConvertBinaryData(
StubDataProtector.Protect(cr.GetSerializedRequestState()));

var expected = $"CookieName={protectedData}; path=/; HttpOnly";
var expected = $"CookieName={protectedData}; path=/; secure; HttpOnly";

setCookieHeader.Should().Be(expected);
}
Expand Down
36 changes: 36 additions & 0 deletions Tests/Tests.Shared/WebSSO/SignInCommandTests.cs
Expand Up @@ -412,5 +412,41 @@ public void SignInCommand_Run_RedirectToDsWorksWithoutSpecifiedReturnPath()

a.Should().NotThrow();
}

[TestMethod]
public void SignInCommand_WithHttpUrl_DoesNotSetSecureCookieFlag()
{
var options = StubFactory.CreateOptions();
var httpRequest = new HttpRequestData("GET", new Uri("http://localhost"));

var actual = SignInCommand.Run(options.IdentityProviders.Default.EntityId, null, httpRequest, options, null);

actual.SetCookieName.Should().StartWith(StoredRequestState.CookieNameBase);
actual.SetCookieSecureFlag.Should().BeFalse();
}

[TestMethod]
public void SignInCommand_WithHttpsUrl_SetsSecureCookieFlag()
{
var options = StubFactory.CreateOptions();
var httpRequest = new HttpRequestData("GET", new Uri("https://localhost"));

var actual = SignInCommand.Run(options.IdentityProviders.Default.EntityId, null, httpRequest, options, null);

actual.SetCookieName.Should().StartWith(StoredRequestState.CookieNameBase);
actual.SetCookieSecureFlag.Should().BeTrue();
}

[TestMethod]
public void SignInCommand_WithHttpsPublicOrigin_SetsSecureCookieFlag()
{
var options = StubFactory.CreateOptionsPublicOrigin(new Uri("https://my.public.origin:8443"));
var httpRequest = new HttpRequestData("GET", new Uri("http://localhost"));

var actual = SignInCommand.Run(options.IdentityProviders.Default.EntityId, null, httpRequest, options, null);

actual.SetCookieName.Should().StartWith(StoredRequestState.CookieNameBase);
actual.SetCookieSecureFlag.Should().BeTrue();
}
}
}