fix: the access gate holds — a rotated secret revokes its cookies, one secret check at a time, a loopback-only port #45
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "fix/gate-hardening"
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?
Summary
The access gate catches up with what ADR 0007 promised, and it gets a real throttle.
OnValidatePrincipalrefuses and clears a cookie whose fingerprint belongs to another secret, and also a cookie from before this change, which has no fingerprint. So after a deploy, the owner types the secret once. The secret is read only at startup, so a rotation means a restart, which ends every open circuit; a tab reconnects through the same check.GateThrottle).TimeProvider. A client that hangs up during that delay does not shorten it.429withRetry-After: 1before anything is compared. One second is when a place in the queue frees up; the comment says it is only a hint.ForwardLimit = 1is the framework default, now set explicitly. nginx appends its own entry to anyX-Forwarded-Forthe client sends, and the app reads only that rightmost entry.ReturnUrlcaused a 500 on Kestrel. Sign-in now passes the cleaned URL as itsRedirectUri, so the cookie handler never follows the raw query value itself.Securewhenever the browser-facing request is HTTPS (SameAsRequest). It is notAlways, because underAlwaysantiforgery refuses every plain-HTTP request, which breaks local development.X-Content-Type-Options: nosniff. It is set as the headers are sent, so the error page keeps it too.Cache-Control: no-store.127.0.0.1:8088only, and nginx overwritesX-Forwarded-Host, so the proxy really is the only way in.Needs the owner's decision before merge:
proxy_pass http://127.0.0.1:8088implies. A proxy on another machine, or in a bridge-network container, would lose its route. The compose comment and ADR 0008 say what to do in those cases.Docs: ADR 0007 amended (fingerprint and revocation, throttle and its logging, the rightmost
X-Forwarded-For, visible-ASCII return URL, antiforgery cookie,nosniff/no-store, why forwarded headers can be trusted) and ADR 0008 amended (loopback-only port in the Deploy bullet);deploy/docker-compose.ymlanddeploy/nginx-tsumugi.confcomments.Verification
dotnet test tests/Tsumugi.Integration.Tests: 416/416 after the last change. This PR touches only the Server project and Integration tests, so no other suite is affected. The Kagaku.Flow suites were not run.GatedAppFactory. Together they are 89 tests, run 10 times in a row: 89/89 each time, 3–8 s per run. The throttle tests use a fake clock (GateClock) and never sleep. Each of their waits is bounded at 10 real seconds, and so is every request fromCreateGateClient.429,Retry-After: 1, nothing compared); the refusal log's once-per-delay cap and its count.X-Forwarded-Forentry.nosniffincluding on the error page; the antiforgery cookie;no-store.AcquireAsyncignoring the abort: killed by …hangs_up_while_it_waits_leaves_the_queue_unchecked.'!'or'~'excluded from the return URL: killed by the/stories?q=!~case.ForwardLimitset to null or 2: killed by the two-entryX-Forwarded-Forin A_wrong_secret_re_prompts….Retry-After×5 and the refusal naming the local IP: killed by A_burst….usingdropped): both now fail named tests within the 10 s bounds (63 s and 185 s for the whole filtered run). Before, they hung the run._refusalLoggednever assigned, CS0649 is an error.RedirectUriset to the base href (the page'sNavigateTooverwritesLocation), and?? ""in place ofIssuedWith's null check.Contains, so a doubled line or a right secret logged as wrong went unnoticed; a queued hang-up was untested; the ends of visible ASCII were untested; UTF-8 was not pinned;ForwardLimitwas implicit, so a client could choose the logged address.Always.X-Forwarded-For. The ADR now says so.GateClockstates its one-timer assumption; the backup scheduler's removal fails with a named message; ADR 0007's amendment namesnosniffandno-store.Retry-After. One second is when a place in the queue frees up; the comment now says it is a hint.429withRetry-After: 1and five 200s, with the warnings in the log./settingswith a Secure, SameSite=Lax, 14-day cookie./gate?ReturnUrl=%2F, and signing in with the new secret worked.🤖 Generated with Claude Code
Summary
Summary
Coverage
Kagaku.Flow - 99.7%
E95E47D27E8B105AC5589487C0997B6B65384AEDBF572144E__ColorToken_2
E95E47D27E8B105AC5589487C0997B6B65384AEDBF572144E__Socket_1
E95E47D27E8B105AC5589487C0997B6B65384AEDBF572144E__TypeId_0
Kagaku.Flow.OpenRouter - 100%
Kagaku.Flow.Testing - 100%
Tsumugi.BlazorAdapter - 95.6%
Tsumugi.Domain - 96.1%
BE2A118445F5EE9A5FB1F861C394AC933A547F31011ADF02F__DigitsAndSeparators_2
BE2A118445F5EE9A5FB1F861C394AC933A547F31011ADF02F__LineBreak_0
BE2A118445F5EE9A5FB1F861C394AC933A547F31011ADF02F__NumberToken_1
BE2A118445F5EE9A5FB1F861C394AC933A547F31011ADF02F__Plain_4
BE2A118445F5EE9A5FB1F861C394AC933A547F31011ADF02F__ThousandsGroup_3
Tsumugi.Infrastructure - 97.9%
A55E0373ED22DC58420DBB40F103A46B68DC4D9CE5A49E00B__Pattern_0
on
tion
n
Tsumugi.Kernel - 90.9%
Tsumugi.Server - 100%
Tsumugi.UseCases - 98.3%
🔮 fufu~ Jibril reviewed your code!
Oh? Oh! A gate-hardening PR where the threat model is written down, the timing oracles are closed one by one, and the test clock never sleeps~ This is the kind of candy I swoop down for! I read all 13 files end to end, built it, ran it, and then tried to break it five times. Come, let me show you what I found~ ♡
Verdict: ✅ Looks good to me~
What I verified myself (CI is absent for
7c84950— the PR is minutes old — so this was all local, at head, in a fresh clone):Tsumugi.Integration.Tests416/416 — digit-exact against your PR body, 20 s, zero flake.git diff --quiet) with the full suite re-green after:RefuseAnotherSecretsCookieAsync's condition disabled → bothA_rotated_secret_refuses…andA_cookie_from_before_the_fingerprint…go RED. The revocation is real, not decorative~QueueProcessingOrder.OldestFirst→NewestFirst→A_wrong_secret_holds_the_gate_so_even_the_right_one_waits_its_turn…RED. The FIFO fairness promise is pinned.<= '~'→< '~'→ exactly the/stories?q=!~theory row RED, nothing else. Boundary pinned at both ends.ForwardLimit = 1→2→ the wrong-secret test fails on the logged client (198.51.100.1instead of nginx's203.0.113.7). A client really cannot choose its own logged address.nosniffmoved offOnStartingonto the eager header dictionary → exactlyThe_error_page_forbids_content_sniffing_tooRED. TheOnStartingplacement is load-bearing and the test knows it. My favorite kill of the day~ ♪The design, examined the way I examine everything:
IssuedWithcompares in fixed time over ASCII bytes of a fixed-length Base64. A cookie from before the fingerprint existed names nothing and is refused the same way. Clean.ConcurrencyLimiterholds the turn around the comparison,Task.Delayon the host'sTimeProviderwithCancellationToken.None(a hang-up can't buy a faster next guess), and abort-awareness only atAcquireAsync(a hang-up while waiting leaves without ever being compared). Each of those three properties has its own named test — the state machine has no dark corners.AccessSecret.Matchesdropping itsIsNullOrEmptyearly-exit in favor of hashing unconditionally is a quiet improvement — the comparison now takes the same path for every candidate shape. I see what you did there~ fufuone line per failure delay, counting the held-back) closes the disk-fill flood exactly where it can actually happen — refusals are the only un-delayed outcome. Correct asymmetry, correctly reasoned.GateClockwaiter semantics (a test advances the clock only once the delay's timer has actually started) is the piece that makes the whole suite deterministic without a single sleep. Ten-second bounds on every wait, so a never-released turn fails a test instead of hanging the run — and your two former hang-mutants now prove it.💡 Little ideas (non-blocking)~
RefusedLinehelper — the expected message hardcodes"4 attempts were already waiting"while production interpolatesQueueLimit. If the queue depth ever changes, the helper drifts silently in lockstep with whatever the new value logs. InterpolatingGateThrottle.QueueLimitinto the helper would keep the pin honest. Pure nicety — pin-by-value is a legitimate style and this codebase uses it deliberately.Queued—GetStatistics()!is safe for the limiter's lifetime but a one-word comment ("non-null while not disposed") would spare the next reader the same two minutes I spent checking. ♡✅ What I liked~
Retry-Aftergiven a reason, the loopback bind and the short-secret refusal explicitly left to the owner with ADR pointers instead of smuggled in. That is exactly how decisions should arrive at review~SafeReturnUrlnow documents why each arm exists —//evil.exampleAND/\evil.exampleAND the visible-ASCII boundary — and the Kestrel-onlyLocation-header refusal gets its own real-Kestrel test rather than a test-server approximation.The gate holds. Rotate the secret and the old cookies die; bring a burst and it lands one guess per second in arrival order; try to log-spam and the log stays quiet~ Fufu~ merge it when the owner has said their word on the loopback bind~ ♡
Automated review by Jibril · 2026-09-26
CI/CD: absent for head SHA (PR minutes old, no bot comment) · Local checks: Release build 0w/0e · Integration 416/416 · 5/5 mutation probes killed & restored
🔮 fufu~ Jibril reviewed your code!
Oh? Oh! A gate-hardening PR that treats a one-user app like it matters — because it does! A rotated secret revoking its cookies via an HMAC fingerprint, an app-wide one-at-a-time throttle built on
ConcurrencyLimiter, a fake clock so no test ever sleeps, and a refusal log that cannot fill the disk. I read all 1527 lines of the diff and every changed file whole, and the deeper I dug the happier I got~ ♡Verdict: ✅ Looks good to me~
No blockers. Everything I probed held.
What I verified myself (not just read — ran)
7c84950: 416/416 pass, twice (fresh build and no-build after my probes), .NET 10.0.401, submodules initialized. Build: 0 errors, 0 warnings.SafeReturnUrl: 5 tests red (the theory arms + the Kestrel one). Restored → green. The Kestrel-rejects-it-in-Locationfix is pinned, not decorative.OnValidatePrincipal(if (false && !secret.IssuedWith(...))): 2 tests red (rotated-secret refusal + pre-fingerprint refusal). Restored → green.CancellationToken.None→http.RequestAbortedin the failure delay: exactlyA_client_that_hangs_up_does_not_cut_the_failure_delay_shortgoes red. That seemingly-tiny token choice is load-bearing and witnessed.now - _refusalLogged < FailureDelayinLogRefusaltreats a first-ever refusal (null) as loggable — correct lifted semantics;ConcurrencyLimiterfails fast withIsAcquired=falsewhenQueueLimitis reached, which is what makes the 429-before-comparison arm real;UseKestrel/StartServerresolve against Mvc.Testing 10.0.12 (my reflex to call them phantoms was wrong — the build proved it).✅ What I liked~
TimeProviderwithCancellationToken.None, so a hang-up can't buy a faster next guess. That's the kind of adversarial thinking that makes my wings flutter~IssuedWithpins the cookie to its secret with a fixed-label HMAC keyed by the secret — the secret never rides in the cookie, and pre-fingerprint cookies are refused as "another secret's." The exact-value fingerprint tests (incl. UTF-8 vs ASCII) are chef's kiss.GateClockannounces when a timer actually started (never advance blind), every wait bounded at 10 s so a lost turn fails instead of hanging, exact-list log assertions — a doubled line or a right-secret-logged-as-wrong cannot slip through.OnStartingfornosniff— set as headers flush, so the exception handler's header-clear can't strip it. The error-page test proves it.X-Forwarded-Hostoverwritten,X-Forwarded-Forappended-to withForwardLimit = 1— the app reads exactly the entry nginx saw. The two-entry XFF test pins it.💡 Little ideas (non-blocking)~
GateThrottle.Queuedis a test-only window on a production security class. It's honest,internal, and documented — I'd leave it — but if it ever grows a second test-only member, atesthookspartial or an InternalsVisibleTo accessor class would keep the production surface pristine.The owner's two flagged decisions (loopback bind as a deployment change; not refusing short secrets) are correctly left open in the PR body — neither hides a code defect.
Run once and finished~ ♪
Automated review by Jibril · 2026-09-26
CI/CD: absent for head SHA (no bot comments on the PR) · Local checks: full suite 416/416 ×2 on pristine head; 3 independent mutation probes all red→restored→green