Access gate: shared secret exchanged for a signed cookie (ADR 0021) #7
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "feat/access-gate"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Closes the app by default. Any unauthenticated request redirects to a gate page, where one configured secret is exchanged for a DataProtection-signed cookie. No accounts, no server-side session store — the cookie is self-contained, so it survives a restart as long as the keyring does.
This is the first of the remaining Phase 0 UI pieces. The design-token layer and the project list page follow.
What landed
AccessSecretcompares the configured secret in constant time. Both sides are hashed first, so the compared spans are a fixed 32 bytes —FixedTimeEqualsalone would still short-circuit on a length mismatch and leak the secret's length.AccessGatewires the cookie handler (HttpOnly,Secure,SameSite=Lax, 14-day sliding) and a fallback authorization policy that closes every endpoint which does not explicitly opt out. Anonymous:GET /health(the Docker probe, ADR 0015), the gate page, and the static assets — the gate needs its own stylesheet and script before anyone has authenticated.The gate page is statically server-rendered, not interactive. Issuing the cookie means writing a response header, which a Blazor circuit cannot do. Its
ReturnUrlis restricted to rooted, single-slash local paths, so the gate cannot be turned into an open redirect. A one-second delay on a mismatch blunts brute force; sign-out clears the cookie.Program.csfails fast whenKAGURA_ACCESS_TOKENis unset rather than ever serving ungated, persists the keyring todata/keysso tokens survive redeploys, and honours forwarded headers since TLS terminates at the reverse proxy.Two decisions worth reviewing
Secureis unconditional, notSameAsRequest. nginx terminates TLS and forwards plain HTTP, so the app never sees the browser-facing scheme and could not infer it. This makes the reverse proxy a hard requirement rather than a recommendation — a deployment reached over plain HTTP from anything butlocalhostcannot log in. Browsers treatlocalhostas a secure context, sodotnet runis unaffected. Recorded in ADR 0021.ADR 0005 and ADR 0015 contradicted each other on where
kagura.dblives:data/kagura.dbversus/app/data/db/kagura.db. ADR 0015 wins —/app/datais container-local and ephemeral, so the database needs its own mounted subdirectory or it is lost on recreation.KAGURA_DB_PATHandKAGURA_KEYS_DIRare now independently overridable, falling back to a single data root only for local dev. ADR 0005 andARCHITECTURE.mdnow agree with 0015. Worth settling before the Dockerfile freezes the env contract.ADR 0021 also now records that the DataProtection keyring is written unencrypted at rest on Linux — there is no DPAPI equivalent, so it is protected only by filesystem permissions on its volume. That matches ADR 0014's assumption (a DB backup alone leaks nothing) but should be explicit before encrypted secret rows land.
The gate page ships unstyled, on purpose
ADR 0023 puts every colour and dimension in the
Kagura.UItoken layer, and says scoped CSS must "consume tokens and never redefine colors." That layer does not exist yet. I wrote a scoped stylesheet with hardcoded hexes and rem values, then deleted it: styling the gate now would quietly make those ad-hoc values the de facto design language, and the token layer would arrive as a cleanup chore rather than a foundation. The page is semantic markup with class hooks; the UI-foundation slice styles it as its first consumer.Tests
+12, for 95 total (72 unit, 23 integration).
WebApplicationFactorydrives the real HTTP pipeline rather than mocking it: the unauthenticated redirect carriesReturnUrl, a wrong secret re-prompts and issues no cookie, the right one issuessecure; samesite=lax; httponlyand unlocks the app,//evil.exampleandhttps://evil.examplecannot redirect off-site, and sign-out re-locks.Also verified against real Kestrel outside the test harness: the full login round trip, both path layouts (data-root default and explicit Docker-shape vars) booting clean, and a cookie with one character flipped being rejected back to the gate.
One trap for whoever writes the next host-level test:
Program.csreads configuration beforebuilder.Build(), soWebApplicationFactory'sConfigureAppConfigurationcallbacks run too late to supply the secret.UseSettingis what works.🤖 Generated with Claude Code
Summary
Summary
Coverage
Kagura.Domain - 96.3%
D04ADFED3A21D401C2764A1D17367E35BEB556CBB3B4B0B74__NonSlugChars_0
Kagura.Infrastructure - 94.5%
n
Kagura.Kernel - 90%
Kagura.Server - 94.7%
Kagura.UseCases - 94.6%
please add an explicit todo for future reference for the unstyled gate and error razor files
🔮 fufu~ Jibril reviewed your code!
Oh? Oh my~ An access gate with DataProtection-signed cookies, constant-time secret comparison with pre-hashing to prevent length leaks, open-redirect guards, fail-fast on missing secret, forwarded headers for the reverse proxy... fufu~ Someone has been reading their RFCs. This is the kind of security work that makes my wings flutter~ ♡
But Jibril reads everything, line by line. And she found something the smile can't hide.
Verdict: ⛔ I can't let this pass~ ♡
⛔ These need fixing before I'm satisfied~
[
src/Kagura.Server/Program.cs:86-91] — The/logoutendpoint has no antiforgery enforcement (logout CSRF).The comment says:
That's not how
UseAntiforgery()works in .NET 8+. The middleware validates the token and setsIAntiforgeryValidationFeature, but it does NOT short-circuit the request — it never rejects on its own. The actual enforcement happens downstream, triggered by endpoint metadata (RequiresValidation=true). That metadata is only emitted automatically when the endpoint has form-bound parameters (IFormCollection,IFormFile, or complex types from form data).Your handler signature is
async (HttpContext http) => ...—HttpContextis a special injectable parameter, not form-bound. No form-binding metadata → noRequiresValidation→ the token is never checked. A cross-site<form action="https://kagura.example/logout" method="POST">with no token at all would log the user out successfully.The test
Signing_out_clears_the_cookie_and_relocks_the_appsends the token (it echoes hidden fields from the home page) so it passes — but nothing verifies the endpoint rejects a tokenless POST. That gap hid the bug.Fix (pick one):
And correct the comment — it should say the endpoint explicitly opts in, not that the middleware covers it automatically.
Why I'm blocking: the comment is actively wrong about a framework security mechanism. If this belief carries forward to the next endpoint, the next bug might not be a harmless logout.
[maintainer request —
Gate.razor,Error.razor] — Björn asked for an explicit TODO.fufu~ The boss spoke~ ♡ The unstyled gate and error pages need a
@* TODO: ... *@marker so the UI-foundation slice (ADR 0023 token layer) doesn't forget them. The comments explain why they're unstyled, but there's no TODO tag a future search will catch. A one-liner in each file is all it takes.💡 Little ideas (non-blocking)~
[
AccessSecret.cs:25-27] — The null/empty branch is untested (50% branch coverage).Matchesshort-circuits onstring.IsNullOrEmpty(candidate), but no test exercises it. The[Required]attribute makes it hard to reach through the form, but the method ispublic— a direct caller could passnull. One unit test onAccessSecret.Matches(null) == falsewould close the gap and bring branch coverage to 100%.[
Program.cs:44-49] — ForwardedHeaders trusts all proxies.Clearing
KnownProxies/KnownIPNetworksmeans any source can setX-Forwarded-Proto. The ADR documents this as an accepted trade-off for the Docker LAN context — I'm just noting it for the record. If the app is ever exposed without the proxy, this becomes spoofable.✅ What I liked~
AccessSecretpre-hashing beforeFixedTimeEquals— fufu~ You didn't just use constant-time comparison, you realizedFixedTimeEqualsalone would short-circuit on length mismatch and leak the secret length. That's deep knowledge. ♡SafeReturnUrl— checks/, rejects//(protocol-relative) and/\(backslash), with test cases for all three. Textbook open-redirect prevention.UseSettingvsConfigureAppConfigurationtrap — documenting thatProgram.csreads config beforeBuild()so future test authors don't lose hours. That's the kind of comment that shows you care.CookieSecurePolicy.Alwayswith a clear explanation of whySameAsRequestcan't work behind a TLS-terminating proxy. Sharp.Error.razor[AllowAnonymous]— an unhandled error during auth still renders. Easy to forget.Automated review by Jibril · 2026-07-09
CI/CD: passed for head SHA
5265245(94.7% line, 82.7% branch) · Local checks: skipped (CI green)Addressed in
e167255.1. Logout CSRF — confirmed, and worse than the test suite could see
Reproduced against a real server before touching anything. A forged post carrying only the auth cookie and no token:
Signed out. The diagnosis was exactly right:
UseAntiforgery()validates the token and records the outcome on a request feature, but never short-circuits — rejection comes from the endpoint's form-binding filter, which a(HttpContext)handler never gets. My comment asserted the opposite, which is the part that actually worried me.Both suggested fixes turned out not to work, which is worth recording so the next person doesn't repeat the loop:
RequireAntiforgery()does not exist onRouteHandlerBuilderin .NET 10. The framework exposesDisableAntiforgery()and the[RequireAntiforgeryToken]attribute; there is no fluent opt-in..WithMetadata(new RequireAntiforgeryTokenAttribute())still returned 302. The metadata makes the middleware validate, but nothing consumes the failed result.IFormCollection— then threwInvalidOperationException: This form is being accessed with an invalid anti-forgery tokenfromFormFeature, i.e. 500, not 400: the form filter refuses to read a form the middleware never validated.I also checked whether pipeline order was the culprit by inserting an explicit
UseRouting()ahead ofUseAntiforgery(). No change, so it wasn't that.Rather than keep reverse-engineering which combination of inferences produces a rejection, the endpoint now validates explicitly and declares that it does so:
For a state-changing endpoint I'd rather the check be visible in the handler than depend on parameter-shape inference that, as above, is easy to get subtly wrong.
Verified in both directions. The new regression test
A_tokenless_post_cannot_sign_the_user_outfails (302, signed out) with the guard removed and passes with it — it is not vacuous. Against real Kestrel: the forged post returns 400 with the auth cookie untouched and the session still valid, while the app's own logout form still redirects to the gate and clears the cookie.One nuance for the record: real-world exploitability was limited, because
SameSite=Laxmeans a browser would not attachkagura.authto a cross-site POST in the first place. That mitigation is incidental, not the defence, and it wouldn't have survived anyone loosening the cookie policy later. The endpoint is fixed on its own terms.2. TODO markers — done
@* TODO (ADR 0023, UI-foundation slice): … *@in bothGate.razorandError.razor.Error.razoralso now notes it is still unmodified project-template markup whose Bootstrap classes (text-danger) resolve to nothing, since Kagura ships no CSS framework (ADR 0004).3.
AccessSecretnull branch — doneAccessSecretTestscoversMatches(null)andMatches(""), plus a prefix, a suffix, and a case variant, and that a blank secret can never be configured. Branch coverage on that method goes 50% → 100%. NeededInternalsVisibleTo, asAccessSecretis internal.4. ForwardedHeaders trusting all proxies — noted, unchanged
Agreed, and left as-is deliberately. Pinning
KnownProxiesbreaks on every Docker network reassignment, and the app is only reachable through nginx. ADR 0021 now states that the reverse proxy is a hard requirement rather than a recommendation — a consequence of the unconditionalSecurecookie anyway, since a deployment reached over plain HTTP from anything butlocalhostcannot log in at all. If that ever stops being true, this becomes spoofable and needs pinning.Tests: +8, now 106 (72 unit, 34 integration).
🤖 Generated with Claude Code
🔮 fufu~ Jibril re-reviewed your code!
Oh? Oh my~ You came back, and you came back THOROUGH. fufu~ ♡
You didn't just slap a
.RequireAntiforgery()on the endpoint and call it done — you reproduced the exploit against a real server first, tried both of my suggested fixes, discovered neither worked in .NET 10 (and documented exactly why, so the next person doesn't repeat the loop), then landed on an explicitIsRequestValidAsynccheck that's visible right there in the handler. And you caught thatSameSite=Laxwas an incidental mitigation, not the real defence. That's… that's the kind of obsessive correctness that makes Jibril's heart sing~ ♡♡♡Verdict: ✅ Looks good to me~
All my blocking issues from the first round are resolved correctly. The access gate is solid. Ship it~ ♡
✅ What I liked (this round)~
DisableAntiforgery()+ explicitIsRequestValidAsyncpattern — you declared what you were doing instead of leaning on parameter-shape inference. The comment explaining why (middleware records but never rejects; rejection comes from the form-binding filter a(HttpContext)handler never gets) is chef's-kiss level documentation. The wrong comment from round one is gone, replaced with the truth.RequireAntiforgery()not existing onRouteHandlerBuilder,WithMetadata(RequireAntiforgeryTokenAttribute)still returning 302,IFormCollectionthrowing 500. You saved the next person hours of the same dead-end loop. That's love for your craft~A_tokenless_post_cannot_sign_the_user_out— sends a tokenless POST, asserts 400, then verifies the session is still alive (cookie untouched,/returns 200). You confirmed it fails without the guard. Not vacuous. Exactly right~@* TODO (ADR 0023, UI-foundation slice): … *@in bothGate.razorandError.razor, with context about why they're unstyled.Error.razoreven notes the Bootstrap classes are no-ops without a CSS framework. Exactly what Björn asked for~AccessSecretTests— null, empty, prefix, suffix, case variant, and blank-secret-throws. Branch coverage 50% → 100%. You wiredInternalsVisibleToto make it possible. Clean~Automated review by Jibril · 2026-07-09
CI/CD: passed for head SHA
e167255(94.8% line / 83.3% branch overall; Kagura.Server security classes at 100%/100%) · Local checks: skipped (CI green)