From 58fa80bc5b5cbbefe59c9bfaf11a2c1d8b45d4cf Mon Sep 17 00:00:00 2001 From: Heimdall Date: Mon, 9 Nov 2026 11:24:02 +0000 Subject: [PATCH] Make the attempt cookie's deletion Secure so a browser honours it A __Host- cookie is only removed by a Set-Cookie that is itself Secure. The hardening test now checks Secure on every header, deletion included, and the completion tests pin that success and refusal both clear the attempt cookie with Secure, Path=/ and no Domain. Co-Authored-By: Claude Sonnet 5.5 --- .../Features/Heimdall/HeimdallAuthService.cs | 7 ++--- .../Bifrost.Web.Tests/CookieHardeningTests.cs | 7 +++-- .../HeimdallCompletionTests.cs | 26 +++++++++++++++++-- 3 files changed, 31 insertions(+), 9 deletions(-) diff --git a/src/Bifrost.Web/Features/Heimdall/HeimdallAuthService.cs b/src/Bifrost.Web/Features/Heimdall/HeimdallAuthService.cs index 7749402..0d39c2f 100644 --- a/src/Bifrost.Web/Features/Heimdall/HeimdallAuthService.cs +++ b/src/Bifrost.Web/Features/Heimdall/HeimdallAuthService.cs @@ -470,8 +470,9 @@ public static class HeimdallAttemptCookie }; /// - /// The options a deletion carries: only the attempt cookie's Path, which a cookie is matched on. It carries no - /// Max-Age, which would take precedence over the expiry Delete writes and leave the emptied cookie alive. + /// The options a deletion carries: the attempt cookie's Path, which a cookie is matched on, and Secure, without + /// which a browser ignores a Set-Cookie for a __Host- name. It carries no Max-Age, which would take precedence + /// over the expiry Delete writes and leave the emptied cookie alive. /// - public static CookieOptions DeleteOptions() => new() { Path = BifrostCookies.Path }; + public static CookieOptions DeleteOptions() => new() { Path = BifrostCookies.Path, Secure = true }; } diff --git a/tests/Bifrost.Web.Tests/CookieHardeningTests.cs b/tests/Bifrost.Web.Tests/CookieHardeningTests.cs index 3a43ebc..287e8ac 100644 --- a/tests/Bifrost.Web.Tests/CookieHardeningTests.cs +++ b/tests/Bifrost.Web.Tests/CookieHardeningTests.cs @@ -84,11 +84,10 @@ public sealed class CookieHardeningTests(CookieHardeningTests.OAuthConfiguredFac Assert.DoesNotContain(attributes, x => x.StartsWith("domain", StringComparison.Ordinal)); } + // A browser ignores a __Host- Set-Cookie that lacks Secure, a deletion included. + Assert.All(headers, header => Assert.Contains("secure", header!.Split(';').Select(x => x.Trim().ToLowerInvariant()))); Assert.All(headers.Take(2), header => - { - Assert.Contains("secure", header!.Split(';').Select(x => x.Trim().ToLowerInvariant())); - Assert.Contains("httponly", header!.Split(';').Select(x => x.Trim().ToLowerInvariant())); - }); + Assert.Contains("httponly", header!.Split(';').Select(x => x.Trim().ToLowerInvariant()))); } private static void AssertHostOnly(string? name, CookieSecurePolicy secure, string? path, string? domain) diff --git a/tests/Bifrost.Web.Tests/HeimdallCompletionTests.cs b/tests/Bifrost.Web.Tests/HeimdallCompletionTests.cs index 68f0a94..532f669 100644 --- a/tests/Bifrost.Web.Tests/HeimdallCompletionTests.cs +++ b/tests/Bifrost.Web.Tests/HeimdallCompletionTests.cs @@ -49,6 +49,29 @@ public sealed class HeimdallCompletionTests private static void AssertTokenRefused(HeimdallHost host, string rule) => AssertLogged(host, $"Heimdall token refused: {rule}."); + /// A __Host- cookie is only deleted by a Set-Cookie that is itself Secure at Path=/; a browser ignores any other. + private static void AssertAttemptCookieCleared(string? header) + { + Assert.NotNull(header); + var attributes = header!.Split(';').Select(x => x.Trim().ToLowerInvariant()).ToArray(); + Assert.Contains(attributes, x => x.StartsWith("expires=", StringComparison.Ordinal) && x.Contains("1970")); + Assert.Contains("secure", attributes); + Assert.Contains("path=/", attributes); + Assert.DoesNotContain(attributes, x => x.StartsWith("domain", StringComparison.Ordinal)); + } + + [Fact] + public async Task A_refused_completion_clears_the_attempt_cookie_with_a_set_cookie_the_browser_honours() + { + using var host = new HeimdallHost(); + var attempt = await host.StartAsync(); + + var refused = await host.CompleteAsync(attempt.Nonce, "unknown", attempt.Cookie); + + AssertRefusedRedirect(refused); + AssertAttemptCookieCleared(HeimdallHost.SetCookieHeader(refused.Response, attempt.CookieName!)); + } + private static void AssertRefusedRedirect(CompletionResult result) { Assert.Equal(HttpStatusCode.Redirect, result.Response.StatusCode); @@ -81,8 +104,7 @@ public sealed class HeimdallCompletionTests Assert.Equal(MembershipStatus.Authenticated, account.Membership!.Status); Assert.Empty(account.Membership.RoleAssignments); var cleared = HeimdallHost.SetCookieHeader(done.Response, attempt.CookieName!); - Assert.NotNull(cleared); - Assert.Contains("1970", cleared); + AssertAttemptCookieCleared(cleared); var again = await host.CompleteAsync(attempt, code);