From 15f96c408c3d711044bd93af94bfde98891d9918 Mon Sep 17 00:00:00 2001 From: Meta Vens Date: Sun, 11 Oct 2026 22:25:24 +0200 Subject: [PATCH] Heimdall sign-in: attempt cookies live in a ring of two fixed slots, so the Cookie header is bounded by construction; pin the completion form's id Start takes a free slot or overwrites the one expiring first; parallel starts carry the same request cookies, choose the same slot and overwrite each other. Complete finds its attempt by scanning the slots for the posted nonce; an overwritten attempt finds none and is refused like any other. The per-request counting sweep, its byte budget and the nonce-named cookie are deleted. Co-Authored-By: Claude Sonnet 5.5 --- .../Features/Heimdall/HeimdallAuthService.cs | 107 +++++++++--------- src/Bifrost.Web/Program.cs | 16 +-- .../HeimdallCompletionTests.cs | 100 +++++++++------- .../Support/HeimdallTestKit.cs | 20 ++-- 4 files changed, 130 insertions(+), 113 deletions(-) diff --git a/src/Bifrost.Web/Features/Heimdall/HeimdallAuthService.cs b/src/Bifrost.Web/Features/Heimdall/HeimdallAuthService.cs index dc5e302..63fd7cc 100644 --- a/src/Bifrost.Web/Features/Heimdall/HeimdallAuthService.cs +++ b/src/Bifrost.Web/Features/Heimdall/HeimdallAuthService.cs @@ -58,6 +58,7 @@ public sealed class HeimdallAuthService( string callbackBaseUrl, LocalReturnPath returnPath, bool requireMemberAccess, + IEnumerable> requestCookies, CancellationToken cancellationToken) { if (!AllowedProviders.Contains(provider)) @@ -106,54 +107,53 @@ public sealed class HeimdallAuthService( returnPath, timeProvider.GetUtcNow() + HeimdallAttemptCookie.Lifetime); return new HeimdallAuthStartResult( - new Uri(payload.AuthorizationUrl, UriKind.Absolute), HeimdallAttemptCookie.NameFor(nonce)!, Protect(attempt)); + new Uri(payload.AuthorizationUrl, UriKind.Absolute), ChooseSlot(requestCookies), Protect(attempt)); } /// - /// The attempt cookies Start must delete so the Cookie header the browser sends to /auth/heimdall stays under - /// the proxy's header limit: every request cookie that is no live attempt, then the live attempts that expire - /// first until the new attempt and the survivors fit . - /// The new attempt is always kept. + /// The slot the new attempt's cookie takes: the first slot that holds no live attempt, else the one whose + /// attempt expires first. Parallel starts carry the same request cookies, so they choose the same slot and + /// overwrite each other; the Cookie header holds at most attempt + /// cookies however many starts a browser makes. /// - public IReadOnlyList SupersededAttemptCookies( - IEnumerable> requestCookies, - HeimdallAuthStartResult start) + private string ChooseSlot(IEnumerable> requestCookies) + { + var cookies = requestCookies.ToLookup(x => x.Key, x => x.Value, StringComparer.Ordinal); + string? oldest = null; + var oldestExpiry = DateTimeOffset.MaxValue; + foreach (var name in HeimdallAttemptCookie.SlotNames) + { + var live = Read(cookies[name].FirstOrDefault()); + if (live is null) + { + return name; + } + + if (live.ExpiresAt < oldestExpiry) + { + oldest = name; + oldestExpiry = live.ExpiresAt; + } + } + + return oldest!; + } + + /// + /// The slot cookie that holds the attempt for this nonce: its name and value, or null when the browser holds + /// none (never started here, expired, or overwritten by a later start). + /// + public (string Name, string Value)? FindAttemptCookie(IEnumerable> requestCookies, string? nonce) { - var used = HeimdallAttemptCookie.EntrySize(start.CookieName, start.CookieValue); - var superseded = new List(); - var live = new List<(string Name, int Size, DateTimeOffset ExpiresAt)>(); foreach (var (name, value) in requestCookies) { - if (!name.StartsWith(HeimdallAttemptCookie.Prefix, StringComparison.Ordinal) || - string.Equals(name, start.CookieName, StringComparison.Ordinal)) + if (HeimdallAttemptCookie.SlotNames.Contains(name) && Unprotect(value, nonce) is not null) { - continue; - } - - var attempt = Unprotect(value, name[HeimdallAttemptCookie.Prefix.Length..]); - if (attempt is null) - { - superseded.Add(name); - } - else - { - live.Add((name, HeimdallAttemptCookie.EntrySize(name, value), attempt.ExpiresAt)); + return (name, value); } } - foreach (var cookie in live.OrderByDescending(x => x.ExpiresAt)) - { - if (used + cookie.Size <= HeimdallAttemptCookie.MaxAggregateBytes) - { - used += cookie.Size; - } - else - { - superseded.Add(cookie.Name); - } - } - - return superseded; + return null; } public async Task CompleteAsync( @@ -223,9 +223,13 @@ public sealed class HeimdallAuthService( _protector.Protect(JsonSerializer.Serialize(new AttemptWire( attempt.Nonce, attempt.Provider, attempt.RequiresMemberAccess, attempt.ReturnPath.Value, attempt.ExpiresAt))); - private HeimdallAttempt? Unprotect(string? cookieValue, string? nonce) + private HeimdallAttempt? Unprotect(string? cookieValue, string? nonce) => + string.IsNullOrEmpty(nonce) ? null : Read(cookieValue, nonce); + + /// The live attempt a cookie value protects (for this nonce when given), else null. + private HeimdallAttempt? Read(string? cookieValue, string? nonce = null) { - if (string.IsNullOrEmpty(cookieValue) || string.IsNullOrEmpty(nonce)) + if (string.IsNullOrEmpty(cookieValue)) { return null; } @@ -234,7 +238,7 @@ public sealed class HeimdallAuthService( { var wire = JsonSerializer.Deserialize(_protector.Unprotect(cookieValue)); if (wire is null || - !string.Equals(wire.Nonce, nonce, StringComparison.Ordinal) || + (nonce is not null && !string.Equals(wire.Nonce, nonce, StringComparison.Ordinal)) || wire.ExpiresAt <= timeProvider.GetUtcNow()) { return null; @@ -442,29 +446,20 @@ public sealed record HeimdallAccount( [property: JsonPropertyName("id")] string Id); /// -/// The per-attempt cookie: named for the attempt's nonce so two attempts in one browser both complete. -/// A name is only built from a well-formed nonce, so request input never becomes an arbitrary cookie name. +/// The attempt cookies: a ring of fixed names, so the Cookie header a browser sends to +/// /auth/heimdall is bounded by construction (a worst-case attempt is about 2.4 KB; nginx's line limit is 8 KB). +/// A start takes a free slot or overwrites the one expiring first; an overwritten attempt finds no cookie and +/// is refused. The attempt's nonce lives inside the protected value, never in the cookie name. /// public static class HeimdallAttemptCookie { public const string Prefix = "bifrost_heimdall_"; - public const int NonceLength = 43; + public const int SlotCount = 2; public static readonly TimeSpan Lifetime = TimeSpan.FromMinutes(10); - /// - /// The most bytes of attempt cookies a browser may hold at once. One worst-case cookie is about 2.4 KB, and - /// the Cookie header must stay under nginx's 8 KB with room for the session and antiforgery cookies. - /// - public const int MaxAggregateBytes = 5120; - - /// A cookie's bytes in a Cookie header: name, '=', value and the '; ' separator. - public static int EntrySize(string name, string value) => name.Length + 1 + value.Length + 2; - - public static string? NameFor(string? nonce) => - nonce is { Length: NonceLength } && nonce.All(c => char.IsAsciiLetterOrDigit(c) || c is '-' or '_') - ? Prefix + nonce - : null; + public static readonly IReadOnlyList SlotNames = + Enumerable.Range(0, SlotCount).Select(x => Prefix + x).ToArray(); public static CookieOptions Options() => new() { diff --git a/src/Bifrost.Web/Program.cs b/src/Bifrost.Web/Program.cs index 5c1e2fa..d44128b 100644 --- a/src/Bifrost.Web/Program.cs +++ b/src/Bifrost.Web/Program.cs @@ -378,13 +378,9 @@ app.MapGet("/auth/heimdall/{provider}", async ( hostOptions.Value.PublicBaseUrl, returnPath, requireMemberAccess, + httpContext.Request.Cookies, cancellationToken); - foreach (var superseded in heimdallAuthService.SupersededAttemptCookies(httpContext.Request.Cookies, start)) - { - httpContext.Response.Cookies.Delete(superseded, HeimdallAttemptCookie.DeleteOptions()); - } - httpContext.Response.Cookies.Append(start.CookieName, start.CookieValue, HeimdallAttemptCookie.Options()); return Results.Redirect(start.AuthorizationUrl.ToString()); } @@ -533,25 +529,25 @@ app.MapPost("/auth/heimdall/complete", async ( { var form = await ReadFormOrNullAsync(httpContext.Request, cancellationToken); var attempt = form?["attempt"].ToString(); - var cookieName = HeimdallAttemptCookie.NameFor(attempt); + var held = heimdallAuthService.FindAttemptCookie(httpContext.Request.Cookies, attempt); try { var signIn = await heimdallAuthService.CompleteAsync( hostOptions.Value.PublicBaseUrl, attempt, - cookieName is null ? null : httpContext.Request.Cookies[cookieName], + held?.Value, form?["code"].ToString(), cancellationToken); await httpContext.SignInAsync(CookieAuthenticationDefaults.AuthenticationScheme, signIn.Principal); - httpContext.Response.Cookies.Delete(cookieName!, HeimdallAttemptCookie.DeleteOptions()); + httpContext.Response.Cookies.Delete(held!.Value.Name, HeimdallAttemptCookie.DeleteOptions()); return Results.LocalRedirect(signIn.ReturnPath.Value); } catch (HeimdallAuthException error) { logger.LogWarning("Heimdall completion refused: {Reason}", error.Message); - if (cookieName is not null) + if (held is not null) { - httpContext.Response.Cookies.Delete(cookieName, HeimdallAttemptCookie.DeleteOptions()); + httpContext.Response.Cookies.Delete(held.Value.Name, HeimdallAttemptCookie.DeleteOptions()); } return Results.Redirect(HeimdallAuthService.ErrorRedirect); diff --git a/tests/Bifrost.Web.Tests/HeimdallCompletionTests.cs b/tests/Bifrost.Web.Tests/HeimdallCompletionTests.cs index f358f53..33e3465 100644 --- a/tests/Bifrost.Web.Tests/HeimdallCompletionTests.cs +++ b/tests/Bifrost.Web.Tests/HeimdallCompletionTests.cs @@ -80,7 +80,7 @@ public sealed class HeimdallCompletionTests Assert.Equal("Genuine Name", account!.DisplayName); Assert.Equal(MembershipStatus.Authenticated, account.Membership!.Status); Assert.Empty(account.Membership.RoleAssignments); - var cleared = HeimdallHost.SetCookieHeader(done.Response, HeimdallAttemptCookie.Prefix + attempt.Nonce); + var cleared = HeimdallHost.SetCookieHeader(done.Response, attempt.CookieName!); Assert.NotNull(cleared); Assert.Contains("1970", cleared); @@ -515,7 +515,7 @@ public sealed class HeimdallCompletionTests Assert.Equal("https://heimdall.test/oauth/discord/authorize?state=fixture", first.Response.Headers.Location!.OriginalString); Assert.Equal(43, first.Nonce.Length); Assert.NotEqual(first.Nonce, second.Nonce); - var header = HeimdallHost.SetCookieHeader(first.Response, "bifrost_heimdall_" + first.Nonce); + var header = HeimdallHost.SetCookieHeader(first.Response, first.CookieName!); Assert.NotNull(header); var attributes = header!.Split(';').Select(x => x.Trim().ToLowerInvariant()).ToArray(); Assert.Contains("httponly", attributes); @@ -689,7 +689,7 @@ public sealed class HeimdallCompletionTests "Heimdall completion has no attempt for this browser."), ("bad cookie", await host.CompleteAsync(attempt.Nonce, host.CodeFor(attempt, accountId), Corrupt(attempt.Cookie!)), "Heimdall completion has no attempt for this browser."), - ("no attempt field", await host.CompleteAsync(null, "code", attempt.Cookie, HeimdallAttemptCookie.Prefix + attempt.Nonce), + ("no attempt field", await host.CompleteAsync(null, "code", attempt.Cookie), "Heimdall completion has no attempt for this browser."), ("malformed attempt", await host.CompleteAsync("short", "code", attempt.Cookie, "bifrost_heimdall_short"), "Heimdall completion has no attempt for this browser."), @@ -1174,9 +1174,8 @@ public sealed class HeimdallCompletionTests Assert.DoesNotContain(canary, logs); } - private static void AssertAttemptCookieExpired(HttpResponseMessage response, string nonce) + private static void AssertAttemptCookieExpired(HttpResponseMessage response, string name) { - var name = HeimdallAttemptCookie.Prefix + nonce; var header = HeimdallHost.SetCookieHeader(response, name); Assert.NotNull(header); Assert.StartsWith(name + "=;", header); @@ -1231,7 +1230,7 @@ public sealed class HeimdallCompletionTests Assert.Equal("/App", overSignIn.Response.Headers.Location!.OriginalString); var heaviest = await host.StartAsync(returnUrl: worstCase); - var header = HeimdallHost.SetCookieHeader(heaviest.Response, HeimdallAttemptCookie.Prefix + heaviest.Nonce)!; + var header = HeimdallHost.SetCookieHeader(heaviest.Response, heaviest.CookieName!)!; Assert.InRange(header.Split(';')[0].Length, 1, 3072); } @@ -1278,62 +1277,80 @@ public sealed class HeimdallCompletionTests } [Fact] - public async Task Attempts_opened_past_the_cookie_bound_keep_the_header_small_and_the_newest_completes() + public async Task Parallel_starts_cannot_push_the_Cookie_header_past_the_bound_and_the_last_start_completes() { using var host = new HeimdallHost(); var worstCase = "/" + new string('<', LocalReturnPath.MaxLength - 1); var jar = new Dictionary(); - var started = new List(); - var deletions = 0; + var largest = 0; - for (var i = 0; i < 12; i++) + // Rounds of parallel starts: every start of a round carries the same Cookie header, as a browser's + // simultaneous requests do before any Set-Cookie lands. + for (var round = 0; round < 3; round++) { - host.Clock.Advance(TimeSpan.FromSeconds(1)); - var attempt = await host.StartAsync(returnUrl: worstCase, cookieHeader: jar.Count == 0 ? null : CookieHeader(jar)); - Assert.NotNull(attempt.Cookie); - deletions += Deletions(attempt.Response); - ApplySetCookies(jar, attempt.Response); - started.Add(attempt); - Assert.True(CookieHeader(jar).Length <= 6144, $"Cookie header is {CookieHeader(jar).Length} bytes after start {i}"); - Assert.Contains(HeimdallAttemptCookie.Prefix + attempt.Nonce, jar.Keys); + var header = jar.Count == 0 ? null : CookieHeader(jar); + var parallel = new List(); + for (var i = 0; i < 6; i++) + { + host.Clock.Advance(TimeSpan.FromSeconds(1)); + parallel.Add(await host.StartAsync(returnUrl: worstCase, cookieHeader: header)); + } + + foreach (var attempt in parallel) + { + Assert.NotNull(attempt.Cookie); + Assert.Equal(0, Deletions(attempt.Response)); + ApplySetCookies(jar, attempt.Response); + largest = Math.Max(largest, CookieHeader(jar).Length); + } + + // The browser holds the last-started attempt and it completes; an overwritten one fails cleanly. + var last = parallel[^1]; + var overwritten = parallel[0]; + var lost = await host.CompleteWithHeaderAsync( + overwritten.Nonce, host.CodeFor(overwritten, HeimdallShapes.NewAccountId()), CookieHeader(jar)); + Assert.False(lost.SignedIn); + Assert.Equal(HttpStatusCode.Redirect, lost.Response.StatusCode); + Assert.Equal(ErrorRedirect, lost.Response.Headers.Location!.OriginalString); + Assert.DoesNotContain(overwritten.Nonce, string.Join("\n", lost.Response.Headers.SelectMany(x => x.Value))); + var done = await host.CompleteWithHeaderAsync( + last.Nonce, host.CodeFor(last, HeimdallShapes.NewAccountId()), CookieHeader(jar)); + Assert.True(done.SignedIn); + ApplySetCookies(jar, done.Response); } - Assert.True(deletions > 0, "the bound was never reached"); - Assert.True(jar.Count < started.Count); - - // The oldest attempts were the ones given up: the newest completes, the first has no cookie left. - Assert.True((await Genuine(host, started[^1], HeimdallShapes.NewAccountId())).SignedIn); - Assert.DoesNotContain(HeimdallAttemptCookie.Prefix + started[0].Nonce, jar.Keys); - Assert.Contains(HeimdallAttemptCookie.Prefix + started[^2].Nonce, jar.Keys); + Assert.True(largest <= 6144, $"Cookie header reached {largest} bytes"); + Assert.True(largest > 4096, "the probe never held two worst-case attempts"); } [Fact] - public async Task Small_attempts_all_survive_under_the_cookie_bound_and_dead_cookies_are_swept() + public async Task A_third_sequential_attempt_takes_the_oldest_slot_and_the_others_still_complete() { using var host = new HeimdallHost(); var jar = new Dictionary(); var started = new List(); - for (var i = 0; i < 5; i++) + for (var i = 0; i < 3; i++) { host.Clock.Advance(TimeSpan.FromSeconds(1)); - var unrelated = ".AspNetCore.Cookies=session; x=1"; var attempt = await host.StartAsync( - returnUrl: "/App", cookieHeader: jar.Count == 0 ? unrelated : CookieHeader(jar) + "; " + unrelated); - Assert.NotNull(attempt.Cookie); - Assert.Equal(0, Deletions(attempt.Response)); + returnUrl: "/App", cookieHeader: (jar.Count == 0 ? "" : CookieHeader(jar) + "; ") + ".AspNetCore.Cookies=session; x=1"); ApplySetCookies(jar, attempt.Response); started.Add(attempt); } - Assert.Equal(5, jar.Count); - foreach (var attempt in started) - { - Assert.True((await Genuine(host, attempt, HeimdallShapes.NewAccountId())).SignedIn); - } + Assert.Equal(HeimdallAttemptCookie.SlotCount, jar.Keys.Count(x => x.StartsWith(HeimdallAttemptCookie.Prefix, StringComparison.Ordinal))); + Assert.Equal(started[0].CookieName, started[2].CookieName); - var deadName = HeimdallAttemptCookie.Prefix + new string('A', HeimdallAttemptCookie.NonceLength); - var next = await host.StartAsync(cookieHeader: deadName + "=not-protected"); - Assert.Contains("1970", HeimdallHost.SetCookieHeader(next.Response, deadName)); + var oldest = await host.CompleteWithHeaderAsync( + started[0].Nonce, host.CodeFor(started[0], HeimdallShapes.NewAccountId()), CookieHeader(jar)); + Assert.False(oldest.SignedIn); + Assert.Equal(ErrorRedirect, oldest.Response.Headers.Location!.OriginalString); + foreach (var attempt in started.Skip(1)) + { + var done = await host.CompleteWithHeaderAsync( + attempt.Nonce, host.CodeFor(attempt, HeimdallShapes.NewAccountId()), CookieHeader(jar)); + Assert.True(done.SignedIn); + } } [Fact] @@ -1346,6 +1363,9 @@ public sealed class HeimdallCompletionTests var form = Regex.Match(page, @"]*)>(.*?)", RegexOptions.Singleline); Assert.True(form.Success); Assert.Matches(@"\bmethod=""post""", form.Groups[1].Value); + var formId = Regex.Match(form.Groups[1].Value, @"\bid=""([^""]*)""").Groups[1].Value; + Assert.NotEmpty(formId); + Assert.Contains($"getElementById(\"{formId}\")", page); var action = Regex.Match(form.Groups[1].Value, @"\baction=""([^""]*)""").Groups[1].Value; Assert.Equal("/auth/heimdall/complete", action); var inputs = Regex.Matches(form.Groups[2].Value, @"]*)>"); @@ -1358,7 +1378,7 @@ public sealed class HeimdallCompletionTests var code = host.CodeFor(attempt, HeimdallShapes.NewAccountId()); var fields = new Dictionary { [names[0]] = attempt.Nonce, [names[1]] = code }; using var post = new HttpRequestMessage(HttpMethod.Post, action) { Content = new FormUrlEncodedContent(fields) }; - post.Headers.Add("Cookie", $"{HeimdallAttemptCookie.Prefix + attempt.Nonce}={attempt.Cookie}"); + post.Headers.Add("Cookie", $"{attempt.CookieName}={attempt.Cookie}"); using var response = await client.SendAsync(post); Assert.False(string.IsNullOrEmpty(HeimdallHost.CookieValue(response, ".AspNetCore.Cookies"))); } diff --git a/tests/Bifrost.Web.Tests/Support/HeimdallTestKit.cs b/tests/Bifrost.Web.Tests/Support/HeimdallTestKit.cs index e0719e9..4d86991 100644 --- a/tests/Bifrost.Web.Tests/Support/HeimdallTestKit.cs +++ b/tests/Bifrost.Web.Tests/Support/HeimdallTestKit.cs @@ -337,7 +337,7 @@ public sealed class CapturedLogs : ILoggerProvider } /// An attempt Start issued: its nonce, the cookie it set, and the returnTo it sent Heimdall. -public sealed record StartedAttempt(string Nonce, string? Cookie, string ReturnTo, HttpResponseMessage Response); +public sealed record StartedAttempt(string Nonce, string? CookieName, string? Cookie, string ReturnTo, HttpResponseMessage Response); public sealed record CompletionResult(HttpResponseMessage Response, bool SignedIn); @@ -437,12 +437,13 @@ public sealed class HeimdallHost : TestWebApplicationFactory var response = await client.SendAsync(request); if (Heimdall.StartBodies.Count == before) { - return new StartedAttempt(string.Empty, null, string.Empty, response); + return new StartedAttempt(string.Empty, null, null, string.Empty, response); } var returnTo = JsonDocument.Parse(Heimdall.StartBodies[^1]).RootElement.GetProperty("returnTo").GetString()!; var nonce = QueryHelpers.ParseQuery(new Uri(returnTo).Query)["attempt"].ToString(); - return new StartedAttempt(nonce, CookieValue(response, HeimdallAttemptCookie.Prefix + nonce), returnTo, response); + var slot = HeimdallAttemptCookie.SlotNames.FirstOrDefault(x => CookieValue(response, x) is { Length: > 0 }); + return new StartedAttempt(nonce, slot, slot is null ? null : CookieValue(response, slot), returnTo, response); } /// Registers a completion code as Heimdall would mint it after authenticating this attempt. @@ -455,7 +456,12 @@ public sealed class HeimdallHost : TestWebApplicationFactory } /// The browser's POST from the complete page: the attempt and code in a form, the attempt cookie by name. - public async Task CompleteAsync(string? nonce, string? code, string? cookie, string? cookieName = null) + public Task CompleteAsync(string? nonce, string? code, string? cookie, string? cookieName = null) => + CompleteWithHeaderAsync( + nonce, code, cookie is null ? null : $"{cookieName ?? HeimdallAttemptCookie.SlotNames[0]}={cookie}"); + + /// The same POST carrying a browser's whole Cookie header (a jar), not one attempt cookie. + public async Task CompleteWithHeaderAsync(string? nonce, string? code, string? cookieHeader) { using var client = NewClient(); var fields = new Dictionary(); @@ -470,9 +476,9 @@ public sealed class HeimdallHost : TestWebApplicationFactory } using var request = new HttpRequestMessage(HttpMethod.Post, "/auth/heimdall/complete") { Content = new FormUrlEncodedContent(fields) }; - if (cookie is not null) + if (cookieHeader is not null) { - request.Headers.Add("Cookie", $"{cookieName ?? HeimdallAttemptCookie.Prefix + nonce}={cookie}"); + request.Headers.Add("Cookie", cookieHeader); } var response = await client.SendAsync(request); @@ -481,7 +487,7 @@ public sealed class HeimdallHost : TestWebApplicationFactory } public Task CompleteAsync(StartedAttempt attempt, string code) => - CompleteAsync(attempt.Nonce, code, attempt.Cookie); + CompleteAsync(attempt.Nonce, code, attempt.Cookie, attempt.CookieName); public static string? CookieValue(HttpResponseMessage response, string name) {