diff --git a/src/Bifrost.Web/Features/Heimdall/HeimdallAuthService.cs b/src/Bifrost.Web/Features/Heimdall/HeimdallAuthService.cs index 7f32908..1672c96 100644 --- a/src/Bifrost.Web/Features/Heimdall/HeimdallAuthService.cs +++ b/src/Bifrost.Web/Features/Heimdall/HeimdallAuthService.cs @@ -418,4 +418,16 @@ public static class HeimdallAttemptCookie Path = "/auth/heimdall", MaxAge = Lifetime }; + + /// + /// The options a deletion carries: the attempt cookie's Path, which a cookie is matched on, and no Max-Age, + /// which would take precedence over the expiry Delete writes and leave the emptied cookie alive. + /// + public static CookieOptions DeleteOptions() => new() + { + HttpOnly = true, + Secure = true, + SameSite = SameSiteMode.Lax, + Path = "/auth/heimdall" + }; } diff --git a/src/Bifrost.Web/Features/Shared/LocalReturnPath.cs b/src/Bifrost.Web/Features/Shared/LocalReturnPath.cs index e9c97d1..1de135b 100644 --- a/src/Bifrost.Web/Features/Shared/LocalReturnPath.cs +++ b/src/Bifrost.Web/Features/Shared/LocalReturnPath.cs @@ -2,13 +2,16 @@ namespace Bifrost.Web.Features.Shared; /// /// A sign-in return target that stays on this site. The only way to make one is , which -/// accepts a path that starts with a single slash and holds no control character, and otherwise yields -/// . Callers parse once at entry and carry the typed value from there. +/// accepts a path of at most characters that starts with a single slash and holds no +/// control character, and otherwise yields . Callers parse once at entry and carry the typed value from there. /// public sealed record LocalReturnPath { private LocalReturnPath(string value) => Value = value; + /// Bounds the path so the attempt cookie that carries it stays far below a browser's 4096-byte limit. + public const int MaxLength = 256; + public static LocalReturnPath Default { get; } = new("/App"); public string Value { get; } @@ -16,6 +19,7 @@ public sealed record LocalReturnPath public static LocalReturnPath Parse(string? candidate) { if (string.IsNullOrEmpty(candidate) || + candidate.Length > MaxLength || candidate[0] != '/' || candidate.Any(char.IsControl) || (candidate.Length > 1 && (candidate[1] == '/' || candidate[1] == '\\'))) diff --git a/src/Bifrost.Web/Program.cs b/src/Bifrost.Web/Program.cs index 4bb73ac..1c96ea0 100644 --- a/src/Bifrost.Web/Program.cs +++ b/src/Bifrost.Web/Program.cs @@ -125,7 +125,8 @@ builder.Services.AddScoped(); builder.Services.AddSingleton(); builder.Services.AddScoped(); builder.Services.AddHttpClient((serviceProvider, client) => - HeimdallAuthService.ConfigureClient(client, serviceProvider.GetRequiredService>().Value)); + HeimdallAuthService.ConfigureClient(client, serviceProvider.GetRequiredService>().Value)) + .RedactLoggedHeaders(header => string.Equals(header, HeimdallAuthService.AppSecretHeader, StringComparison.OrdinalIgnoreCase)); builder.Services.AddSingleton(); builder.Services.AddHttpClient(HeimdallTokenVerifier.ClientName, (serviceProvider, client) => HeimdallTokenVerifier.ConfigureClient(client, serviceProvider.GetRequiredService>().Value)); @@ -537,7 +538,7 @@ app.MapPost("/auth/heimdall/complete", async ( form?["code"].ToString(), cancellationToken); await httpContext.SignInAsync(CookieAuthenticationDefaults.AuthenticationScheme, signIn.Principal); - httpContext.Response.Cookies.Delete(cookieName!, HeimdallAttemptCookie.Options()); + httpContext.Response.Cookies.Delete(cookieName!, HeimdallAttemptCookie.DeleteOptions()); return Results.LocalRedirect(signIn.ReturnPath.Value); } catch (HeimdallAuthException error) @@ -545,7 +546,7 @@ app.MapPost("/auth/heimdall/complete", async ( logger.LogWarning("Heimdall completion refused: {Reason}", error.Message); if (cookieName is not null) { - httpContext.Response.Cookies.Delete(cookieName, HeimdallAttemptCookie.Options()); + httpContext.Response.Cookies.Delete(cookieName, HeimdallAttemptCookie.DeleteOptions()); } return Results.Redirect(HeimdallAuthService.ErrorRedirect); diff --git a/tests/Bifrost.Web.Tests/HeimdallCompletionTests.cs b/tests/Bifrost.Web.Tests/HeimdallCompletionTests.cs index 1f2da00..0544438 100644 --- a/tests/Bifrost.Web.Tests/HeimdallCompletionTests.cs +++ b/tests/Bifrost.Web.Tests/HeimdallCompletionTests.cs @@ -4,10 +4,14 @@ using System.Text.Json; using Bifrost.Web.Domain; using Bifrost.Web.Features.Heimdall; using Bifrost.Web.Features.Membership; +using Bifrost.Web.Features.Shared; using Bifrost.Web.Tests.Support; using Microsoft.AspNetCore.Authentication.OAuth; +using Microsoft.AspNetCore.Hosting; +using Microsoft.AspNetCore.Mvc.Testing; using Microsoft.AspNetCore.WebUtilities; using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Logging; using Microsoft.Extensions.Options; namespace Bifrost.Web.Tests; @@ -1139,4 +1143,109 @@ public sealed class HeimdallCompletionTests Assert.DoesNotContain(canary, logs); } } + + [Fact] + public async Task The_app_secret_reaches_Heimdall_but_is_never_logged_even_at_trace() + { + const string canary = "CANARY-app-secret-7c1e"; + using var host = new HeimdallHost(appSecret: canary); + host.Heimdall.RequiredAppSecret = canary; + using var traced = host.WithWebHostBuilder(builder => builder.ConfigureLogging(logging => + { + logging.SetMinimumLevel(LogLevel.Trace); + logging.AddFilter((string? _, LogLevel _) => true); + logging.AddFilter("System.Net.Http", LogLevel.Trace); + logging.AddFilter("Microsoft", LogLevel.Trace); + })); + using var client = traced.CreateClient(new WebApplicationFactoryClientOptions + { + AllowAutoRedirect = false, + HandleCookies = false, + BaseAddress = new Uri("https://localhost") + }); + + using var start = await client.GetAsync("/auth/heimdall/discord?access=member&returnUrl=/App"); + + Assert.Equal(HttpStatusCode.Redirect, start.StatusCode); + Assert.Equal(canary, Assert.Single(host.Heimdall.StartSecrets)); + var logs = string.Join(" +", host.Logs.Lines); + Assert.Contains(HeimdallAuthService.AppSecretHeader, logs, StringComparison.OrdinalIgnoreCase); + Assert.DoesNotContain(canary, logs); + } + + private static void AssertAttemptCookieExpired(HttpResponseMessage response, string nonce) + { + var name = HeimdallAttemptCookie.Prefix + nonce; + var header = HeimdallHost.SetCookieHeader(response, name); + Assert.NotNull(header); + Assert.StartsWith(name + "=;", header); + var attributes = header!.Split(';').Select(x => x.Trim().ToLowerInvariant()).ToArray(); + Assert.Contains("path=/auth/heimdall", attributes); + Assert.Contains(attributes, x => x.StartsWith("expires=", StringComparison.Ordinal) && x.Contains("1970")); + Assert.DoesNotContain(attributes, x => x.StartsWith("max-age", StringComparison.Ordinal)); + } + + [Fact] + public async Task The_attempt_cookie_is_expired_at_its_path_on_success_and_on_refusal() + { + using var host = new HeimdallHost(); + var accountId = HeimdallShapes.NewAccountId(); + + var signedAttempt = await host.StartAsync(); + var signedIn = await Genuine(host, signedAttempt, accountId); + Assert.True(signedIn.SignedIn); + AssertAttemptCookieExpired(signedIn.Response, signedAttempt.Nonce); + + var unknownCode = await host.StartAsync(); + var gone = await host.CompleteAsync(unknownCode, "unknown-code"); + AssertRefusedRedirect(gone); + AssertAttemptCookieExpired(gone.Response, unknownCode.Nonce); + + var tampered = await host.StartAsync(); + var refused = await host.CompleteAsync( + tampered.Nonce, host.CodeFor(tampered, accountId), Corrupt(tampered.Cookie!)); + AssertRefusedRedirect(refused); + AssertAttemptCookieExpired(refused.Response, tampered.Nonce); + } + + [Fact] + public async Task Return_paths_are_bounded_so_the_attempt_cookie_stays_small() + { + using var host = new HeimdallHost(); + var atBound = "/" + new string('a', LocalReturnPath.MaxLength - 1); + var overBound = atBound + "a"; + var worstCase = "/" + new string('<', LocalReturnPath.MaxLength - 1); + + Assert.Equal(atBound, LocalReturnPath.Parse(atBound).Value); + Assert.Equal(LocalReturnPath.Default, LocalReturnPath.Parse(overBound)); + + var kept = await host.StartAsync(returnUrl: atBound); + var keptSignIn = await Genuine(host, kept, HeimdallShapes.NewAccountId()); + Assert.True(keptSignIn.SignedIn); + Assert.Equal(atBound, keptSignIn.Response.Headers.Location!.OriginalString); + + var over = await host.StartAsync(returnUrl: overBound); + var overSignIn = await Genuine(host, over, HeimdallShapes.NewAccountId()); + Assert.True(overSignIn.SignedIn); + 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)!; + Assert.InRange(header.Split(';')[0].Length, 1, 3072); + } + + [Fact] + public async Task A_blank_app_slug_is_refused_before_Heimdall_is_called() + { + using var host = new HeimdallHost(appSlug: " "); + + var refused = await host.StartAsync(); + + Assert.Equal(ErrorRedirect, refused.Response.Headers.Location!.OriginalString); + Assert.Empty(host.Heimdall.StartBodies); + Assert.False(refused.Response.Headers.Contains("Set-Cookie")); + Assert.Contains("Heimdall start refused: Heimdall is not configured.", string.Join(" +", host.Logs.Lines)); + } } diff --git a/tests/Bifrost.Web.Tests/Support/HeimdallTestKit.cs b/tests/Bifrost.Web.Tests/Support/HeimdallTestKit.cs index c4992a7..40e024b 100644 --- a/tests/Bifrost.Web.Tests/Support/HeimdallTestKit.cs +++ b/tests/Bifrost.Web.Tests/Support/HeimdallTestKit.cs @@ -347,6 +347,7 @@ public sealed class HeimdallHost : TestWebApplicationFactory public const string AppSecret = "test-heimdall-app-secret"; private readonly string _appSecret; + private readonly string _appSlug; private readonly string _baseUrl; private readonly string _publicBaseUrl; private readonly bool _discordRoles; @@ -363,9 +364,11 @@ public sealed class HeimdallHost : TestWebApplicationFactory string appSecret = AppSecret, string baseUrl = HeimdallShapes.Issuer, string publicBaseUrl = "https://localhost", - bool discordRoles = true) + bool discordRoles = true, + string appSlug = "bifrost") { _appSecret = appSecret; + _appSlug = appSlug; _baseUrl = baseUrl; _publicBaseUrl = publicBaseUrl; _discordRoles = discordRoles; @@ -385,6 +388,7 @@ public sealed class HeimdallHost : TestWebApplicationFactory { ["Heimdall:BaseUrl"] = _baseUrl, ["Heimdall:AppSecret"] = _appSecret, + ["Heimdall:AppSlug"] = _appSlug, ["Heimdall:DiscordGuildId"] = "guild-1", ["Host:PublicBaseUrl"] = _publicBaseUrl });