Repository navigation
CORE-1: ASP.NET Core cookie auth + authorization policies + security stamp (#183) - #253
Merged
Merged
Conversation
…stamp Replace the ~22 hand-copied per-action auth checks with framework cookie authentication and centralized authorization. Design: docs/audits/feature-audit-2026-10/design-auth-middleware.md. Authentication: - AddCookie on the BookWheel.Auth cookie (HttpOnly, SameSite=Strict, 8h sliding), SecurePolicy = AlwaysSecureCookie ? Always : SameAsRequest (preserves OPS-1 behaviour), and 401/403 events instead of HTML redirects. - Login/logout go through SignInAsync/SignOutAsync with claims (user id, username, Admin role, security stamp, issued-at). Logout is per-device (client cookie cleared), per owner decision. Authorization: - Fallback RequireAuthenticatedUser policy; [AllowAnonymous] on the 8 pre-auth endpoints and MigrationController; [Authorize(Policy="Admin")] on Users, Metrics, and Stats aggregate. Controllers read identity from claims and drop the manual null/IsAdmin/403 preambles. /, /index.html, /sw.js, /api/version and the health endpoints are AllowAnonymous; UseStaticFiles runs before auth. Session revocation (AUTH-1, preserved) + absolute cap: - Self-contained cookies can't be deleted server-side, so a 256-bit security stamp is carried as a claim and re-checked every request in OnValidatePrincipal; the stamp rotates on password set, forced reset, and any change to role/disabled/locked/force-reset/username. OnValidatePrincipal rejects on missing/disabled/locked/force-reset/stamp-mismatch. - Absolute 8h cap pinned at sign-in via an issued-at claim (survives sliding renewals, unlike the ticket IssuedUtc), closing the indefinite-slide window this change would otherwise introduce (owner decision; partial AUTH-2). - The lookup rate limiter (runs before UseAuthentication) reads the user id from the Data-Protection-encrypted ticket, keeping LOOKUP-2's per-user keying. Migration AddUserSecurityStamp adds the non-null SecurityStamp column and backfills existing rows. Existing login cookies invalidate on deploy (everyone re-logs in once) — expected for an auth change. Tests: AuthorizationCoverageTests (reflection guard that every action is AllowAnonymous or authorized), SessionAbsoluteLifetimeTests (cap predicate), and all 11 merged revocation/cookie/rate-limit tests stay green. Full suite 460 passing. No version change (rides unreleased 3.8.0). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NnmDYLFwxwNNrPn8G5cJEW
4 tasks
This was referenced Oct 11, 2026
jasonkryst
added a commit
that referenced
this pull request
Oct 11, 2026
* CORE-2: persist auto-lockout state to the database (#184); bump to 3.9.0 Re-scoped after CORE-1: sessions are already stateless (encrypted cookies + persisted Data Protection keys survive restarts and work multi-instance), so the only remaining per-instance in-memory auth state worth persisting is the username auto-lockout counter. This moves it to the database so a restart no longer clears an active lockout and the state is consistent across instances. - New login_throttle table (migration AddLoginThrottle) keyed by normalized username — deliberately NOT a FK to users, so unknown usernames still lock (preserves the anti-enumeration behaviour) without an enumeration oracle. - ILoginThrottleRepository + Postgres impl. RegisterFailedAttemptAsync is a single parameterized INSERT ... ON CONFLICT DO UPDATE ... RETURNING, so the increment, threshold decision and lockout are atomic under the row lock (10 parallel failures end at exactly count 10). GetActiveLockout/Clear/DeleteStale round it out. - AuthService.ValidateCredentialsAsync uses the repo with the identical three-step sequence and the same LoginValidationResult shapes; _failedLogins and FailedLoginRecord removed. - LoginThrottleCleanupService (hosted) prunes stale rows daily so arbitrary- username attempts can't grow the table unboundedly; UpdatedAtUtc is indexed. - Scope kept tight: the password-reset/forgot-username rate-limit windows stay in-memory, and distributed per-IP rate limiting (Redis) is out of scope — both noted as deferred follow-ups on #184. Version bumped 3.8.0 -> 3.9.0: v3.8.0 is released and contains only #251; the CSP/lookup (#252) and cookie-auth overhaul (#253) work plus this sit unreleased on main, so 3.9.0 covers the batch. Tests: PostgresLoginThrottleRepositoryTests (threshold/lock/reset, atomic under concurrency, expiry, clear, stale cleanup) and AuthService-level tests proving the lockout survives a simulated restart and that unknown usernames lock. Full suite 469 passing. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NnmDYLFwxwNNrPn8G5cJEW * Address review: key lockout by resolved account, not raw submitted username The lockout key came from the submitted username via ToLowerInvariant, while credential matching uses Postgres citext (DB-locale lower()). Those case-folders can diverge on some Unicode input, so an attacker could spell one real account several ways that all citext-match it but land in different lockout buckets, spreading failed attempts to dodge the per-username threshold (parser differential → lockout bypass). ASCII usernames are unaffected, but having two normalizers decide the same thing is the defect. ValidateCredentialsAsync now resolves the account first (FindByUsernameAsync) and keys the lockout on the account's canonical username, so every spelling that maps to one account shares a single bucket. Unknown usernames fall back to the submitted value and still lock (anti-enumeration preserved). Test: AuthService_Lockout_Is_Keyed_By_Resolved_Account_Across_Spellings. Full suite 470 passing. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NnmDYLFwxwNNrPn8G5cJEW --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
This was referenced Oct 11, 2026
Adopt ASP.NET Core authentication/authorization middleware instead of per-action session checks
#183
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Replaces the ~22 hand-copied per-action auth checks with framework cookie authentication and centralized authorization policies. Design doc:
docs/audits/feature-audit-2026-10/design-auth-middleware.md. No version change (rides the unreleased 3.8.0).Authentication
AddCookieon the existingBookWheel.Authcookie:HttpOnly,SameSite=Strict, 8h sliding,SecurePolicy = AlwaysSecureCookie ? Always : SameAsRequest(preserves OPS-1's behaviour behind a proxy), and401/403events instead of HTML login/access-denied redirects.SignInAsync/SignOutAsyncwith claims: user id, username,Adminrole, security stamp, issued-at. Logout is per-device (clears this device's cookie) per the owner decision.Authorization
RequireAuthenticatedUserpolicy so nothing is unprotected by omission;[AllowAnonymous]on the 8 pre-auth AuthController endpoints and MigrationController;[Authorize(Policy="Admin")]on UsersController, MetricsController, and Statsaggregate.Userclaims and drop the manual null/IsAdmin/403 preambles./,/index.html,/sw.js,/api/version, and the health endpoints areAllowAnonymous;UseStaticFilesruns before auth so the SPA assets stay public.Session revocation (AUTH-1 preserved) + absolute cap
OnValidatePrincipal. The stamp rotates on password set, forced reset, and any change to role / disabled / locked / force-reset / username.OnValidatePrincipalrejects on missing / disabled / locked / force-reset / stamp-mismatch.IssuedUtc), closing the indefinite-slide window this change would otherwise open. Owner decision; this also covers most of AUTH-2 (Auth cookie has a fixed 8h expiry while the server session slides — users are logged out mid-use #193)./api/books/lookup|inforate limiter runs beforeUseAuthentication, so it reads the user id from the Data-Protection-encrypted ticket — LOOKUP-2's per-user keying is preserved, forged cookies fall back to IP.Migration
AddUserSecurityStampadds the non-nullSecurityStampcolumn and backfills existing rows with a 64-hex value. Existing login cookies invalidate on deploy — everyone re-logs in once, expected for an auth change.Tests
AuthorizationCoverageTests— reflection guard asserting every controller action is either[AllowAnonymous]or covered by an authorize policy (fails if a new action is added unprotected); plus the exact anonymous/admin sets.SessionAbsoluteLifetimeTests— deterministic coverage of the cap predicate.Login_Cookie_Is_Secure_*.Security-review findings (both addressed in this PR)
Addresses two deviations from the original brief (both correct)
UseStaticFilesmoved beforeUseAuthentication(otherwise the fallback policy 401s/js/app.jsetc.).Out of scope
Persistent/DB-backed sessions (CORE-2 #184), TOTP (AUTH-6), OIDC (AUTH-7), retiring MigrationController + JSON repos (CORE-7 #200).
🤖 Generated with Claude Code
https://claude.ai/code/session_01NnmDYLFwxwNNrPn8G5cJEW