From 0b23d4b6b115f36cf41d0215bb2740670732b6b8 Mon Sep 17 00:00:00 2001 From: Timothy Date: Sun, 12 Jul 2026 16:14:54 +0200 Subject: [PATCH 1/5] =?UTF-8?q?feat(api):=20#295=20PR1=20=E2=80=94=20brows?= =?UTF-8?q?er=20SPA=20session=20auth=20(session-OR-key=20gate,=20server-on?= =?UTF-8?q?ly)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Implements the ratified #295 design (PR1, server-only, backward compatible). The /api surface now accepts a valid X-Api-Key (machine) OR an authenticated session (browser cookie, local login or OIDC), gated by the evolved ApiAuthorizationFilter (renamed from ApiKeyAuthorizationFilter; same fail-closed EndpointRequiresKey predicate). Machine/key behavior is byte-identical and the SPA keeps working via its stored key — the SPA login flow lands in PR2. - ApiAuthorizationFilter: key-first (CSRF-immune) then session; session-authed mutations require the X-CSRF header (403 otherwise). Attributes renamed [RequiresApiKey]->[RequiresAuthentication], [SkipApiKeyAuthorization]->[SkipApiAuthorization]. - Cookie scheme ctv-session always registered (Lax/SameAsRequest/14d sliding, 401 not redirect for /api); OIDC handler revived when configured (profile scope, userinfo, auth-method claim); UseAuthentication/UseAuthorization/UseRateLimiter revived in the legacy MapWhen branch. - Local admin = single credential in ConfigElement rows (username / PBKDF2 hash via Microsoft.Extensions.Identity.Core / rotating security stamp) — NO DB migration. Password change rotates the stamp; CookieSecurityStampValidator revokes stale local sessions. Env-seed recovery (Auth:LocalAdmin:*) via LocalAdminSeedService. - AuthController /api/auth/{config,session,setup,login,logout,password} + browser-nav GET /auth/oidc/login; excluded from OpenAPI (machine-audience spec). Per-IP rate limit on login/setup/password; dummy-hash verify (no user enumeration). - ForwardedHeaders now strict opt-in: X-Forwarded-* ignored unless KnownProxies/Networks configured (rate-limiter IP + cookie-Secure integrity). Deployment: operators behind a proxy must set ForwardedHeaders:KnownProxies. - Tests: session/CSRF filter cases + 17 Application/Auth handler tests; full ErsatzTV.Tests green (1499). No OpenAPI/generated-artifact drift. - Docs: api-conventions section 9 rewritten; decisions.md entry (supersedes #206 inert-OIDC note). Refs #295 #197 #206 #58 Co-Authored-By: Claude Opus 4.8 (1M context) --- Directory.Packages.props | 1 + ErsatzTV.Application/Auth/AuthConstants.cs | 28 ++ .../Auth/ChangeLocalAdminPassword.cs | 10 + .../Auth/ChangeLocalAdminPasswordHandler.cs | 53 ++++ ErsatzTV.Application/Auth/ClaimLocalAdmin.cs | 9 + .../Auth/ClaimLocalAdminHandler.cs | 49 ++++ .../Auth/GetLocalAdminSecurityStamp.cs | 8 + .../Auth/GetLocalAdminSecurityStampHandler.cs | 11 + .../Auth/ILocalPasswordHasher.cs | 27 ++ .../Auth/IsLocalAdminConfigured.cs | 4 + .../Auth/IsLocalAdminConfiguredHandler.cs | 15 ++ .../Auth/LocalAdminHelpers.cs | 10 + .../Auth/LocalAdminPrincipal.cs | 9 + .../Auth/LocalPasswordHasher.cs | 33 +++ .../Auth/SeedLocalAdminFromEnvironment.cs | 11 + .../SeedLocalAdminFromEnvironmentHandler.cs | 37 +++ .../Auth/VerifyLocalAdminLogin.cs | 9 + .../Auth/VerifyLocalAdminLoginHandler.cs | 51 ++++ .../ErsatzTV.Application.csproj | 1 + ErsatzTV.Core/Domain/ConfigElementKey.cs | 7 + .../ChangeLocalAdminPasswordHandlerTests.cs | 122 +++++++++ .../Auth/ClaimLocalAdminHandlerTests.cs | 127 +++++++++ ...edLocalAdminFromEnvironmentHandlerTests.cs | 103 +++++++ .../Auth/VerifyLocalAdminLoginHandlerTests.cs | 122 +++++++++ .../Controllers/ApiControllerSecurityTests.cs | 30 ++- ...ests.cs => ApiAuthorizationFilterTests.cs} | 82 +++++- .../Filters/ApiKeyEndpointRequiresKeyTests.cs | 24 +- ErsatzTV/Auth/CookieSecurityStampValidator.cs | 38 +++ ErsatzTV/Controllers/Api/AuthController.cs | 161 +++++++++++ ErsatzTV/Controllers/Api/LogsController.cs | 2 +- .../Controllers/Api/MaintenanceController.cs | 2 +- .../Controllers/Api/Requests/AuthRequests.cs | 7 + ErsatzTV/Controllers/Api/ScannerController.cs | 2 +- .../Controllers/Api/SettingsController.cs | 2 +- .../Controllers/Api/TroubleshootController.cs | 5 +- ErsatzTV/Filters/ApiAuthorizationFilter.cs | 144 ++++++++++ ErsatzTV/Filters/ApiKeyAuthorizationFilter.cs | 98 ------- ErsatzTV/Filters/RequiresApiKeyAttribute.cs | 12 - .../RequiresAuthenticationAttribute.cs | 14 + .../Filters/SkipApiAuthorizationAttribute.cs | 12 + .../SkipApiKeyAuthorizationAttribute.cs | 9 - .../ApiSecurityOperationTransformer.cs | 4 +- .../ApiSecuritySchemeDocumentTransformer.cs | 2 +- ErsatzTV/Services/ApiKeyProvider.cs | 4 +- ErsatzTV/Services/LocalAdminSeedService.cs | 44 +++ ErsatzTV/Startup.cs | 253 ++++++++++++------ docs/api-conventions.md | 85 ++++-- docs/decisions.md | 64 +++++ 48 files changed, 1683 insertions(+), 274 deletions(-) create mode 100644 ErsatzTV.Application/Auth/AuthConstants.cs create mode 100644 ErsatzTV.Application/Auth/ChangeLocalAdminPassword.cs create mode 100644 ErsatzTV.Application/Auth/ChangeLocalAdminPasswordHandler.cs create mode 100644 ErsatzTV.Application/Auth/ClaimLocalAdmin.cs create mode 100644 ErsatzTV.Application/Auth/ClaimLocalAdminHandler.cs create mode 100644 ErsatzTV.Application/Auth/GetLocalAdminSecurityStamp.cs create mode 100644 ErsatzTV.Application/Auth/GetLocalAdminSecurityStampHandler.cs create mode 100644 ErsatzTV.Application/Auth/ILocalPasswordHasher.cs create mode 100644 ErsatzTV.Application/Auth/IsLocalAdminConfigured.cs create mode 100644 ErsatzTV.Application/Auth/IsLocalAdminConfiguredHandler.cs create mode 100644 ErsatzTV.Application/Auth/LocalAdminHelpers.cs create mode 100644 ErsatzTV.Application/Auth/LocalAdminPrincipal.cs create mode 100644 ErsatzTV.Application/Auth/LocalPasswordHasher.cs create mode 100644 ErsatzTV.Application/Auth/SeedLocalAdminFromEnvironment.cs create mode 100644 ErsatzTV.Application/Auth/SeedLocalAdminFromEnvironmentHandler.cs create mode 100644 ErsatzTV.Application/Auth/VerifyLocalAdminLogin.cs create mode 100644 ErsatzTV.Application/Auth/VerifyLocalAdminLoginHandler.cs create mode 100644 ErsatzTV.Tests/Application/Auth/ChangeLocalAdminPasswordHandlerTests.cs create mode 100644 ErsatzTV.Tests/Application/Auth/ClaimLocalAdminHandlerTests.cs create mode 100644 ErsatzTV.Tests/Application/Auth/SeedLocalAdminFromEnvironmentHandlerTests.cs create mode 100644 ErsatzTV.Tests/Application/Auth/VerifyLocalAdminLoginHandlerTests.cs rename ErsatzTV.Tests/Filters/{ApiKeyAuthorizationFilterTests.cs => ApiAuthorizationFilterTests.cs} (63%) create mode 100644 ErsatzTV/Auth/CookieSecurityStampValidator.cs create mode 100644 ErsatzTV/Controllers/Api/AuthController.cs create mode 100644 ErsatzTV/Controllers/Api/Requests/AuthRequests.cs create mode 100644 ErsatzTV/Filters/ApiAuthorizationFilter.cs delete mode 100644 ErsatzTV/Filters/ApiKeyAuthorizationFilter.cs delete mode 100644 ErsatzTV/Filters/RequiresApiKeyAttribute.cs create mode 100644 ErsatzTV/Filters/RequiresAuthenticationAttribute.cs create mode 100644 ErsatzTV/Filters/SkipApiAuthorizationAttribute.cs delete mode 100644 ErsatzTV/Filters/SkipApiKeyAuthorizationAttribute.cs create mode 100644 ErsatzTV/Services/LocalAdminSeedService.cs diff --git a/Directory.Packages.props b/Directory.Packages.props index 7bb487fd9..59e56e359 100644 --- a/Directory.Packages.props +++ b/Directory.Packages.props @@ -32,6 +32,7 @@ + diff --git a/ErsatzTV.Application/Auth/AuthConstants.cs b/ErsatzTV.Application/Auth/AuthConstants.cs new file mode 100644 index 000000000..19e080aa3 --- /dev/null +++ b/ErsatzTV.Application/Auth/AuthConstants.cs @@ -0,0 +1,28 @@ +namespace ErsatzTV.Application.Auth; + +/// +/// Shared constants for the browser-SPA session authentication (issue #295): the cookie scheme name, +/// the custom claim types the local-login path stamps onto the principal, and the auth-method marker +/// values. The web host (cookie OnValidatePrincipal, AuthController) and the Application +/// handlers both reference these so the claim contract has a single definition. +/// +public static class AuthConstants +{ + /// The cookie authentication scheme name shared by local login and the OIDC callback. + public const string CookieScheme = "cookie"; + + /// The OIDC challenge scheme name. + public const string OidcScheme = "oidc"; + + /// Claim type recording how the principal signed in ( / ). + public const string AuthMethodClaim = "etv:auth_method"; + + /// Claim type carrying the local admin's security stamp (checked on every request to revoke sessions). + public const string SecurityStampClaim = "etv:security_stamp"; + + public const string MethodLocal = "local"; + public const string MethodOidc = "oidc"; + + /// Minimum length for a local admin password. + public const int MinPasswordLength = 8; +} diff --git a/ErsatzTV.Application/Auth/ChangeLocalAdminPassword.cs b/ErsatzTV.Application/Auth/ChangeLocalAdminPassword.cs new file mode 100644 index 000000000..5b35f8525 --- /dev/null +++ b/ErsatzTV.Application/Auth/ChangeLocalAdminPassword.cs @@ -0,0 +1,10 @@ +using ErsatzTV.Core; + +namespace ErsatzTV.Application.Auth; + +/// +/// Changes the local admin password after verifying the current one. Rotates the security stamp so all +/// other sessions are revoked. is the signed-in principal's name. +/// +public record ChangeLocalAdminPassword(string Username, string CurrentPassword, string NewPassword) + : IRequest>; diff --git a/ErsatzTV.Application/Auth/ChangeLocalAdminPasswordHandler.cs b/ErsatzTV.Application/Auth/ChangeLocalAdminPasswordHandler.cs new file mode 100644 index 000000000..a1ccab28d --- /dev/null +++ b/ErsatzTV.Application/Auth/ChangeLocalAdminPasswordHandler.cs @@ -0,0 +1,53 @@ +using ErsatzTV.Core; +using ErsatzTV.Core.Domain; +using ErsatzTV.Core.Interfaces.Repositories; + +namespace ErsatzTV.Application.Auth; + +public class ChangeLocalAdminPasswordHandler( + IConfigElementRepository configElementRepository, + ILocalPasswordHasher passwordHasher) + : IRequestHandler> +{ + public async Task> Handle( + ChangeLocalAdminPassword request, + CancellationToken cancellationToken) + { + if (string.IsNullOrEmpty(request.NewPassword) || request.NewPassword.Length < AuthConstants.MinPasswordLength) + { + return BaseError.New($"Password must be at least {AuthConstants.MinPasswordLength} characters"); + } + + Option storedUser = + await configElementRepository.GetValue(ConfigElementKey.AuthLocalAdminUsername, cancellationToken); + Option storedHash = + await configElementRepository.GetValue(ConfigElementKey.AuthLocalAdminPasswordHash, cancellationToken); + + if (storedHash.IsNone) + { + return BaseError.New("No local administrator is configured"); + } + + string username = (request.Username ?? string.Empty).Trim(); + bool userMatches = storedUser.Match( + Some: u => string.Equals(u, username, StringComparison.OrdinalIgnoreCase), + None: () => false); + + LocalPasswordVerification result = + passwordHasher.Verify(storedHash.IfNone(passwordHasher.DummyHash), request.CurrentPassword ?? string.Empty); + + if (!userMatches || result == LocalPasswordVerification.Failed) + { + return BaseError.New("Current password is incorrect"); + } + + string stamp = LocalAdminHelpers.NewSecurityStamp(); + await configElementRepository.Upsert( + ConfigElementKey.AuthLocalAdminPasswordHash, + passwordHasher.Hash(request.NewPassword), + cancellationToken); + await configElementRepository.Upsert(ConfigElementKey.AuthSecurityStamp, stamp, cancellationToken); + + return new LocalAdminPrincipal(storedUser.IfNone(username), stamp); + } +} diff --git a/ErsatzTV.Application/Auth/ClaimLocalAdmin.cs b/ErsatzTV.Application/Auth/ClaimLocalAdmin.cs new file mode 100644 index 000000000..d35a3d57b --- /dev/null +++ b/ErsatzTV.Application/Auth/ClaimLocalAdmin.cs @@ -0,0 +1,9 @@ +using ErsatzTV.Core; + +namespace ErsatzTV.Application.Auth; + +/// +/// First-run setup-claim: creates the single local administrator. Fails if one already exists +/// (first-claim-wins), so a later anonymous call cannot take over the account. +/// +public record ClaimLocalAdmin(string Username, string Password) : IRequest>; diff --git a/ErsatzTV.Application/Auth/ClaimLocalAdminHandler.cs b/ErsatzTV.Application/Auth/ClaimLocalAdminHandler.cs new file mode 100644 index 000000000..8e604e6dc --- /dev/null +++ b/ErsatzTV.Application/Auth/ClaimLocalAdminHandler.cs @@ -0,0 +1,49 @@ +using ErsatzTV.Core; +using ErsatzTV.Core.Domain; +using ErsatzTV.Core.Interfaces.Repositories; + +namespace ErsatzTV.Application.Auth; + +public class ClaimLocalAdminHandler(IConfigElementRepository configElementRepository, ILocalPasswordHasher passwordHasher) + : IRequestHandler> +{ + public async Task> Handle( + ClaimLocalAdmin request, + CancellationToken cancellationToken) + { + string username = (request.Username ?? string.Empty).Trim(); + if (username.Length == 0) + { + return BaseError.New("Username is required"); + } + + if (username.Length > 256) + { + return BaseError.New("Username is too long"); + } + + if (string.IsNullOrEmpty(request.Password) || request.Password.Length < AuthConstants.MinPasswordLength) + { + return BaseError.New($"Password must be at least {AuthConstants.MinPasswordLength} characters"); + } + + // First-claim-wins: refuse if an admin already exists. (The tiny check-then-write race is accepted + // per the design; /api stays closed regardless of who wins.) + Option existing = + await configElementRepository.GetConfigElement(ConfigElementKey.AuthLocalAdminPasswordHash, cancellationToken); + if (existing.IsSome) + { + return BaseError.New("A local administrator has already been configured"); + } + + string stamp = LocalAdminHelpers.NewSecurityStamp(); + await configElementRepository.Upsert(ConfigElementKey.AuthLocalAdminUsername, username, cancellationToken); + await configElementRepository.Upsert( + ConfigElementKey.AuthLocalAdminPasswordHash, + passwordHasher.Hash(request.Password), + cancellationToken); + await configElementRepository.Upsert(ConfigElementKey.AuthSecurityStamp, stamp, cancellationToken); + + return new LocalAdminPrincipal(username, stamp); + } +} diff --git a/ErsatzTV.Application/Auth/GetLocalAdminSecurityStamp.cs b/ErsatzTV.Application/Auth/GetLocalAdminSecurityStamp.cs new file mode 100644 index 000000000..f9fcbf180 --- /dev/null +++ b/ErsatzTV.Application/Auth/GetLocalAdminSecurityStamp.cs @@ -0,0 +1,8 @@ +namespace ErsatzTV.Application.Auth; + +/// +/// The current local-admin security stamp, or None if no local admin is configured. The cookie +/// OnValidatePrincipal compares this to the principal's stamp claim on every request; a mismatch +/// (i.e. the password was changed) rejects the session. +/// +public record GetLocalAdminSecurityStamp : IRequest>; diff --git a/ErsatzTV.Application/Auth/GetLocalAdminSecurityStampHandler.cs b/ErsatzTV.Application/Auth/GetLocalAdminSecurityStampHandler.cs new file mode 100644 index 000000000..f86c0b2b9 --- /dev/null +++ b/ErsatzTV.Application/Auth/GetLocalAdminSecurityStampHandler.cs @@ -0,0 +1,11 @@ +using ErsatzTV.Core.Domain; +using ErsatzTV.Core.Interfaces.Repositories; + +namespace ErsatzTV.Application.Auth; + +public class GetLocalAdminSecurityStampHandler(IConfigElementRepository configElementRepository) + : IRequestHandler> +{ + public async Task> Handle(GetLocalAdminSecurityStamp request, CancellationToken cancellationToken) => + await configElementRepository.GetValue(ConfigElementKey.AuthSecurityStamp, cancellationToken); +} diff --git a/ErsatzTV.Application/Auth/ILocalPasswordHasher.cs b/ErsatzTV.Application/Auth/ILocalPasswordHasher.cs new file mode 100644 index 000000000..5fd772887 --- /dev/null +++ b/ErsatzTV.Application/Auth/ILocalPasswordHasher.cs @@ -0,0 +1,27 @@ +namespace ErsatzTV.Application.Auth; + +public enum LocalPasswordVerification +{ + Failed, + Success, + SuccessRehashNeeded +} + +/// +/// Wraps ASP.NET Core Identity's PasswordHasher (PBKDF2) behind a minimal, framework-agnostic +/// surface so the Auth handlers don't depend on Identity types directly. +/// +public interface ILocalPasswordHasher +{ + /// Hashes a password for storage (random per-hash salt embedded in the returned string). + string Hash(string password); + + /// Verifies a password against a stored hash in constant time (delegated to Identity). + LocalPasswordVerification Verify(string hash, string password); + + /// + /// A stable, valid hash of a throwaway password. Verify against this when no real credential exists + /// so an unknown-username / unconfigured login costs the same as a real one (no user enumeration). + /// + string DummyHash { get; } +} diff --git a/ErsatzTV.Application/Auth/IsLocalAdminConfigured.cs b/ErsatzTV.Application/Auth/IsLocalAdminConfigured.cs new file mode 100644 index 000000000..463c16b24 --- /dev/null +++ b/ErsatzTV.Application/Auth/IsLocalAdminConfigured.cs @@ -0,0 +1,4 @@ +namespace ErsatzTV.Application.Auth; + +/// True once a local administrator credential has been set (first-run setup is complete). +public record IsLocalAdminConfigured : IRequest; diff --git a/ErsatzTV.Application/Auth/IsLocalAdminConfiguredHandler.cs b/ErsatzTV.Application/Auth/IsLocalAdminConfiguredHandler.cs new file mode 100644 index 000000000..2dd484fd9 --- /dev/null +++ b/ErsatzTV.Application/Auth/IsLocalAdminConfiguredHandler.cs @@ -0,0 +1,15 @@ +using ErsatzTV.Core.Domain; +using ErsatzTV.Core.Interfaces.Repositories; + +namespace ErsatzTV.Application.Auth; + +public class IsLocalAdminConfiguredHandler(IConfigElementRepository configElementRepository) + : IRequestHandler +{ + public async Task Handle(IsLocalAdminConfigured request, CancellationToken cancellationToken) + { + Option hash = + await configElementRepository.GetConfigElement(ConfigElementKey.AuthLocalAdminPasswordHash, cancellationToken); + return hash.IsSome; + } +} diff --git a/ErsatzTV.Application/Auth/LocalAdminHelpers.cs b/ErsatzTV.Application/Auth/LocalAdminHelpers.cs new file mode 100644 index 000000000..e4c905b9b --- /dev/null +++ b/ErsatzTV.Application/Auth/LocalAdminHelpers.cs @@ -0,0 +1,10 @@ +using System.Security.Cryptography; + +namespace ErsatzTV.Application.Auth; + +internal static class LocalAdminHelpers +{ + /// 128 bits of random, lowercase hex. Rotated on every password change to revoke sessions. + public static string NewSecurityStamp() => + Convert.ToHexString(RandomNumberGenerator.GetBytes(16)).ToLowerInvariant(); +} diff --git a/ErsatzTV.Application/Auth/LocalAdminPrincipal.cs b/ErsatzTV.Application/Auth/LocalAdminPrincipal.cs new file mode 100644 index 000000000..26656b0b9 --- /dev/null +++ b/ErsatzTV.Application/Auth/LocalAdminPrincipal.cs @@ -0,0 +1,9 @@ +namespace ErsatzTV.Application.Auth; + +/// +/// The identity of the single local administrator, as returned by a successful claim / login / password +/// change. The web host turns this into a cookie principal: becomes the name claim +/// and is stamped as so a later +/// password change (which rotates the stamp) revokes the session. +/// +public record LocalAdminPrincipal(string Username, string SecurityStamp); diff --git a/ErsatzTV.Application/Auth/LocalPasswordHasher.cs b/ErsatzTV.Application/Auth/LocalPasswordHasher.cs new file mode 100644 index 000000000..7805fc526 --- /dev/null +++ b/ErsatzTV.Application/Auth/LocalPasswordHasher.cs @@ -0,0 +1,33 @@ +using Microsoft.AspNetCore.Identity; + +namespace ErsatzTV.Application.Auth; + +/// +/// backed by ASP.NET Core Identity's +/// (PBKDF2-HMAC-SHA512, per-hash random salt, format-versioned so a future work-factor bump is a +/// transparent rehash-on-verify). Stateless and thread-safe → registered as a singleton. +/// +public sealed class LocalPasswordHasher : ILocalPasswordHasher +{ + // The generic user parameter is unused by the hasher (it takes no per-user data), so a shared sentinel + // is fine. + private static readonly object Sentinel = new(); + + private readonly PasswordHasher _hasher = new(); + private readonly Lazy _dummyHash; + + public LocalPasswordHasher() => + _dummyHash = new Lazy(() => _hasher.HashPassword(Sentinel, "not-a-real-password")); + + public string DummyHash => _dummyHash.Value; + + public string Hash(string password) => _hasher.HashPassword(Sentinel, password); + + public LocalPasswordVerification Verify(string hash, string password) => + _hasher.VerifyHashedPassword(Sentinel, hash, password) switch + { + PasswordVerificationResult.Success => LocalPasswordVerification.Success, + PasswordVerificationResult.SuccessRehashNeeded => LocalPasswordVerification.SuccessRehashNeeded, + _ => LocalPasswordVerification.Failed + }; +} diff --git a/ErsatzTV.Application/Auth/SeedLocalAdminFromEnvironment.cs b/ErsatzTV.Application/Auth/SeedLocalAdminFromEnvironment.cs new file mode 100644 index 000000000..e960e0bcd --- /dev/null +++ b/ErsatzTV.Application/Auth/SeedLocalAdminFromEnvironment.cs @@ -0,0 +1,11 @@ +using ErsatzTV.Core; + +namespace ErsatzTV.Application.Auth; + +/// +/// Recovery/bootstrap path: (re)sets the local admin from configuration (env +/// Auth:LocalAdmin:Username/Password). Overwrites any existing credential and rotates the +/// stamp (revoking sessions), so an operator who is locked out can reset by setting the env and +/// restarting. Runs at startup only when a password is configured. +/// +public record SeedLocalAdminFromEnvironment(string Username, string Password) : IRequest>; diff --git a/ErsatzTV.Application/Auth/SeedLocalAdminFromEnvironmentHandler.cs b/ErsatzTV.Application/Auth/SeedLocalAdminFromEnvironmentHandler.cs new file mode 100644 index 000000000..cbc061c21 --- /dev/null +++ b/ErsatzTV.Application/Auth/SeedLocalAdminFromEnvironmentHandler.cs @@ -0,0 +1,37 @@ +using ErsatzTV.Core; +using ErsatzTV.Core.Domain; +using ErsatzTV.Core.Interfaces.Repositories; + +namespace ErsatzTV.Application.Auth; + +public class SeedLocalAdminFromEnvironmentHandler( + IConfigElementRepository configElementRepository, + ILocalPasswordHasher passwordHasher) + : IRequestHandler> +{ + public async Task> Handle( + SeedLocalAdminFromEnvironment request, + CancellationToken cancellationToken) + { + string username = (request.Username ?? string.Empty).Trim(); + if (username.Length == 0) + { + username = "admin"; + } + + if (string.IsNullOrEmpty(request.Password) || request.Password.Length < AuthConstants.MinPasswordLength) + { + return BaseError.New($"Seed password must be at least {AuthConstants.MinPasswordLength} characters"); + } + + string stamp = LocalAdminHelpers.NewSecurityStamp(); + await configElementRepository.Upsert(ConfigElementKey.AuthLocalAdminUsername, username, cancellationToken); + await configElementRepository.Upsert( + ConfigElementKey.AuthLocalAdminPasswordHash, + passwordHasher.Hash(request.Password), + cancellationToken); + await configElementRepository.Upsert(ConfigElementKey.AuthSecurityStamp, stamp, cancellationToken); + + return Unit.Default; + } +} diff --git a/ErsatzTV.Application/Auth/VerifyLocalAdminLogin.cs b/ErsatzTV.Application/Auth/VerifyLocalAdminLogin.cs new file mode 100644 index 000000000..18ace3b6f --- /dev/null +++ b/ErsatzTV.Application/Auth/VerifyLocalAdminLogin.cs @@ -0,0 +1,9 @@ +using ErsatzTV.Core; + +namespace ErsatzTV.Application.Auth; + +/// +/// Verifies a local-login username/password. On success returns the principal (username + current +/// security stamp) to sign into a cookie. A generic error (no username enumeration) on any failure. +/// +public record VerifyLocalAdminLogin(string Username, string Password) : IRequest>; diff --git a/ErsatzTV.Application/Auth/VerifyLocalAdminLoginHandler.cs b/ErsatzTV.Application/Auth/VerifyLocalAdminLoginHandler.cs new file mode 100644 index 000000000..4dd0b06b3 --- /dev/null +++ b/ErsatzTV.Application/Auth/VerifyLocalAdminLoginHandler.cs @@ -0,0 +1,51 @@ +using ErsatzTV.Core; +using ErsatzTV.Core.Domain; +using ErsatzTV.Core.Interfaces.Repositories; + +namespace ErsatzTV.Application.Auth; + +public class VerifyLocalAdminLoginHandler( + IConfigElementRepository configElementRepository, + ILocalPasswordHasher passwordHasher) + : IRequestHandler> +{ + private static readonly BaseError InvalidCredentials = BaseError.New("Invalid username or password"); + + public async Task> Handle( + VerifyLocalAdminLogin request, + CancellationToken cancellationToken) + { + string username = (request.Username ?? string.Empty).Trim(); + Option storedUser = + await configElementRepository.GetValue(ConfigElementKey.AuthLocalAdminUsername, cancellationToken); + Option storedHash = + await configElementRepository.GetValue(ConfigElementKey.AuthLocalAdminPasswordHash, cancellationToken); + + // Always run exactly one verify — against a dummy hash when unconfigured/unknown — so response + // timing does not reveal whether the account exists (no user enumeration). + string candidateHash = storedHash.IfNone(passwordHasher.DummyHash); + LocalPasswordVerification result = passwordHasher.Verify(candidateHash, request.Password ?? string.Empty); + + bool userMatches = storedUser.Match( + Some: u => string.Equals(u, username, StringComparison.OrdinalIgnoreCase), + None: () => false); + + if (storedHash.IsNone || !userMatches || result == LocalPasswordVerification.Failed) + { + return InvalidCredentials; + } + + // Transparent upgrade if Identity's work factor was bumped since this hash was written. + if (result == LocalPasswordVerification.SuccessRehashNeeded) + { + await configElementRepository.Upsert( + ConfigElementKey.AuthLocalAdminPasswordHash, + passwordHasher.Hash(request.Password!), + cancellationToken); + } + + Option stamp = + await configElementRepository.GetValue(ConfigElementKey.AuthSecurityStamp, cancellationToken); + return new LocalAdminPrincipal(storedUser.IfNone(username), stamp.IfNone(string.Empty)); + } +} diff --git a/ErsatzTV.Application/ErsatzTV.Application.csproj b/ErsatzTV.Application/ErsatzTV.Application.csproj index 8d4a2baeb..73d35a499 100644 --- a/ErsatzTV.Application/ErsatzTV.Application.csproj +++ b/ErsatzTV.Application/ErsatzTV.Application.csproj @@ -15,6 +15,7 @@ + diff --git a/ErsatzTV.Core/Domain/ConfigElementKey.cs b/ErsatzTV.Core/Domain/ConfigElementKey.cs index 2b7f5fdc8..42de2398e 100644 --- a/ErsatzTV.Core/Domain/ConfigElementKey.cs +++ b/ErsatzTV.Core/Domain/ConfigElementKey.cs @@ -61,4 +61,11 @@ public class ConfigElementKey public static ConfigElementKey XmltvTimeZone => new("xmltv.time_zone"); public static ConfigElementKey XmltvDaysToBuild => new("xmltv.days_to_build"); public static ConfigElementKey XmltvBlockBehavior => new("xmltv.block_behavior"); + + // Browser SPA authentication (issue #295). The single local-admin credential lives in ConfigElement + // rows (no DB migration): a username, a PBKDF2 password hash, and a security stamp that is rotated on + // every password change so a stamp mismatch in OnValidatePrincipal revokes all outstanding sessions. + public static ConfigElementKey AuthLocalAdminUsername => new("auth.local_admin.username"); + public static ConfigElementKey AuthLocalAdminPasswordHash => new("auth.local_admin.password_hash"); + public static ConfigElementKey AuthSecurityStamp => new("auth.security_stamp"); } diff --git a/ErsatzTV.Tests/Application/Auth/ChangeLocalAdminPasswordHandlerTests.cs b/ErsatzTV.Tests/Application/Auth/ChangeLocalAdminPasswordHandlerTests.cs new file mode 100644 index 000000000..0e068516b --- /dev/null +++ b/ErsatzTV.Tests/Application/Auth/ChangeLocalAdminPasswordHandlerTests.cs @@ -0,0 +1,122 @@ +using ErsatzTV.Application.Auth; +using ErsatzTV.Core; +using ErsatzTV.Core.Domain; +using ErsatzTV.Core.Interfaces.Repositories; +using ErsatzTV.Infrastructure.Data.Repositories; +using ErsatzTV.Tests.Support; +using LanguageExt; +using NUnit.Framework; +using Shouldly; + +namespace ErsatzTV.Tests.Application.Auth; + +[TestFixture] +public class ChangeLocalAdminPasswordHandlerTests +{ + private InMemoryTvContext _db = null!; + private IConfigElementRepository _configElementRepository = null!; + private ILocalPasswordHasher _passwordHasher = null!; + + private const string Username = "Operator"; + private const string CurrentPassword = "supersecret"; + private const string NewPassword = "evenbettersecret"; + + [SetUp] + public async Task SetUp() + { + _db = await InMemoryTvContext.CreateAsync(); + _configElementRepository = new ConfigElementRepository(_db.Factory); + _passwordHasher = new LocalPasswordHasher(); + } + + [TearDown] + public async Task TearDown() => await _db.DisposeAsync(); + + private ChangeLocalAdminPasswordHandler MakeHandler() => + new(_configElementRepository, _passwordHasher); + + private async Task SeedAdmin() + { + var claim = new ClaimLocalAdminHandler(_configElementRepository, _passwordHasher); + (await claim.Handle(new ClaimLocalAdmin(Username, CurrentPassword), CancellationToken.None)) + .IsRight.ShouldBeTrue(); + } + + private async Task StoredHash() => + (await _configElementRepository.GetValue( + ConfigElementKey.AuthLocalAdminPasswordHash, + CancellationToken.None)).IfNone(""); + + private async Task StoredStamp() => + (await _configElementRepository.GetValue( + ConfigElementKey.AuthSecurityStamp, + CancellationToken.None)).IfNone(""); + + [Test] + public async Task Handle_Should_Change_Password_And_Rotate_Stamp_On_Valid_Current_Password() + { + await SeedAdmin(); + string originalStamp = await StoredStamp(); + ChangeLocalAdminPasswordHandler handler = MakeHandler(); + + Either result = await handler.Handle( + new ChangeLocalAdminPassword(Username, CurrentPassword, NewPassword), + CancellationToken.None); + + result.IsRight.ShouldBeTrue(); + + string storedHash = await StoredHash(); + _passwordHasher.Verify(storedHash, NewPassword).ShouldNotBe(LocalPasswordVerification.Failed); + _passwordHasher.Verify(storedHash, CurrentPassword).ShouldBe(LocalPasswordVerification.Failed); + + string newStamp = await StoredStamp(); + newStamp.ShouldNotBe(originalStamp); + LocalAdminPrincipal principal = result.Match( + Left: e => throw new ShouldAssertException(e.ToString()), + Right: p => p); + principal.SecurityStamp.ShouldBe(newStamp); + } + + [Test] + public async Task Handle_Should_Fail_On_Wrong_Current_Password_And_Change_Nothing() + { + await SeedAdmin(); + string originalHash = await StoredHash(); + string originalStamp = await StoredStamp(); + ChangeLocalAdminPasswordHandler handler = MakeHandler(); + + Either result = await handler.Handle( + new ChangeLocalAdminPassword(Username, "notthecurrentpassword", NewPassword), + CancellationToken.None); + + result.IsLeft.ShouldBeTrue(); + (await StoredHash()).ShouldBe(originalHash); + (await StoredStamp()).ShouldBe(originalStamp); + } + + [Test] + public async Task Handle_Should_Reject_Short_New_Password() + { + await SeedAdmin(); + string shortPassword = new('a', AuthConstants.MinPasswordLength - 1); + ChangeLocalAdminPasswordHandler handler = MakeHandler(); + + Either result = await handler.Handle( + new ChangeLocalAdminPassword(Username, CurrentPassword, shortPassword), + CancellationToken.None); + + result.IsLeft.ShouldBeTrue(); + } + + [Test] + public async Task Handle_Should_Fail_On_Unconfigured_Db() + { + ChangeLocalAdminPasswordHandler handler = MakeHandler(); + + Either result = await handler.Handle( + new ChangeLocalAdminPassword(Username, CurrentPassword, NewPassword), + CancellationToken.None); + + result.IsLeft.ShouldBeTrue(); + } +} diff --git a/ErsatzTV.Tests/Application/Auth/ClaimLocalAdminHandlerTests.cs b/ErsatzTV.Tests/Application/Auth/ClaimLocalAdminHandlerTests.cs new file mode 100644 index 000000000..2be7e14ce --- /dev/null +++ b/ErsatzTV.Tests/Application/Auth/ClaimLocalAdminHandlerTests.cs @@ -0,0 +1,127 @@ +using ErsatzTV.Application.Auth; +using ErsatzTV.Core; +using ErsatzTV.Core.Domain; +using ErsatzTV.Core.Interfaces.Repositories; +using ErsatzTV.Infrastructure.Data.Repositories; +using ErsatzTV.Tests.Support; +using LanguageExt; +using NUnit.Framework; +using Shouldly; + +namespace ErsatzTV.Tests.Application.Auth; + +[TestFixture] +public class ClaimLocalAdminHandlerTests +{ + private InMemoryTvContext _db = null!; + private IConfigElementRepository _configElementRepository = null!; + private ILocalPasswordHasher _passwordHasher = null!; + + [SetUp] + public async Task SetUp() + { + _db = await InMemoryTvContext.CreateAsync(); + _configElementRepository = new ConfigElementRepository(_db.Factory); + _passwordHasher = new LocalPasswordHasher(); + } + + [TearDown] + public async Task TearDown() => await _db.DisposeAsync(); + + private ClaimLocalAdminHandler MakeHandler() => + new(_configElementRepository, _passwordHasher); + + [Test] + public async Task Handle_Should_Claim_Fresh_Admin_And_Persist_All_Config_Elements() + { + ClaimLocalAdminHandler handler = MakeHandler(); + + Either result = await handler.Handle( + new ClaimLocalAdmin("Operator", "supersecret"), + CancellationToken.None); + + result.IsRight.ShouldBeTrue(); + LocalAdminPrincipal principal = result.Match( + Left: e => throw new ShouldAssertException(e.ToString()), + Right: p => p); + principal.Username.ShouldBe("Operator"); + principal.SecurityStamp.ShouldNotBeNullOrEmpty(); + + Option storedUser = await _configElementRepository.GetValue( + ConfigElementKey.AuthLocalAdminUsername, + CancellationToken.None); + storedUser.IfNone("").ShouldBe("Operator"); + + Option storedStamp = await _configElementRepository.GetValue( + ConfigElementKey.AuthSecurityStamp, + CancellationToken.None); + storedStamp.IfNone("").ShouldBe(principal.SecurityStamp); + + Option storedHash = await _configElementRepository.GetValue( + ConfigElementKey.AuthLocalAdminPasswordHash, + CancellationToken.None); + storedHash.IsSome.ShouldBeTrue(); + _passwordHasher.Verify(storedHash.IfNone(""), "supersecret") + .ShouldNotBe(LocalPasswordVerification.Failed); + } + + [Test] + public async Task Handle_Should_Refuse_Second_Claim_When_Admin_Already_Configured() + { + ClaimLocalAdminHandler handler = MakeHandler(); + (await handler.Handle(new ClaimLocalAdmin("First", "supersecret"), CancellationToken.None)) + .IsRight.ShouldBeTrue(); + + Either result = await handler.Handle( + new ClaimLocalAdmin("Second", "anothersecret"), + CancellationToken.None); + + result.IsLeft.ShouldBeTrue(); + + // The original credential is untouched. + Option storedUser = await _configElementRepository.GetValue( + ConfigElementKey.AuthLocalAdminUsername, + CancellationToken.None); + storedUser.IfNone("").ShouldBe("First"); + } + + [Test] + public async Task Handle_Should_Reject_Whitespace_Username_And_Persist_Nothing() + { + ClaimLocalAdminHandler handler = MakeHandler(); + + Either result = await handler.Handle( + new ClaimLocalAdmin(" ", "supersecret"), + CancellationToken.None); + + result.IsLeft.ShouldBeTrue(); + await AssertNothingPersisted(); + } + + [Test] + public async Task Handle_Should_Reject_Short_Password_And_Persist_Nothing() + { + string shortPassword = new('a', AuthConstants.MinPasswordLength - 1); + ClaimLocalAdminHandler handler = MakeHandler(); + + Either result = await handler.Handle( + new ClaimLocalAdmin("Operator", shortPassword), + CancellationToken.None); + + result.IsLeft.ShouldBeTrue(); + await AssertNothingPersisted(); + } + + private async Task AssertNothingPersisted() + { + (await _configElementRepository.GetConfigElement( + ConfigElementKey.AuthLocalAdminUsername, + CancellationToken.None)).IsNone.ShouldBeTrue(); + (await _configElementRepository.GetConfigElement( + ConfigElementKey.AuthLocalAdminPasswordHash, + CancellationToken.None)).IsNone.ShouldBeTrue(); + (await _configElementRepository.GetConfigElement( + ConfigElementKey.AuthSecurityStamp, + CancellationToken.None)).IsNone.ShouldBeTrue(); + } +} diff --git a/ErsatzTV.Tests/Application/Auth/SeedLocalAdminFromEnvironmentHandlerTests.cs b/ErsatzTV.Tests/Application/Auth/SeedLocalAdminFromEnvironmentHandlerTests.cs new file mode 100644 index 000000000..d1abb15c1 --- /dev/null +++ b/ErsatzTV.Tests/Application/Auth/SeedLocalAdminFromEnvironmentHandlerTests.cs @@ -0,0 +1,103 @@ +using ErsatzTV.Application.Auth; +using ErsatzTV.Core; +using ErsatzTV.Core.Domain; +using ErsatzTV.Core.Interfaces.Repositories; +using ErsatzTV.Infrastructure.Data.Repositories; +using ErsatzTV.Tests.Support; +using LanguageExt; +using NUnit.Framework; +using Shouldly; + +namespace ErsatzTV.Tests.Application.Auth; + +[TestFixture] +public class SeedLocalAdminFromEnvironmentHandlerTests +{ + private InMemoryTvContext _db = null!; + private IConfigElementRepository _configElementRepository = null!; + private ILocalPasswordHasher _passwordHasher = null!; + + [SetUp] + public async Task SetUp() + { + _db = await InMemoryTvContext.CreateAsync(); + _configElementRepository = new ConfigElementRepository(_db.Factory); + _passwordHasher = new LocalPasswordHasher(); + } + + [TearDown] + public async Task TearDown() => await _db.DisposeAsync(); + + private SeedLocalAdminFromEnvironmentHandler MakeHandler() => + new(_configElementRepository, _passwordHasher); + + private async Task StoredValue(ConfigElementKey key) => + (await _configElementRepository.GetValue(key, CancellationToken.None)).IfNone(""); + + [Test] + public async Task Handle_Should_Seed_Username_Hash_And_Stamp_On_Fresh_Db() + { + SeedLocalAdminFromEnvironmentHandler handler = MakeHandler(); + + Either result = await handler.Handle( + new SeedLocalAdminFromEnvironment("Operator", "supersecret"), + CancellationToken.None); + + result.IsRight.ShouldBeTrue(); + (await StoredValue(ConfigElementKey.AuthLocalAdminUsername)).ShouldBe("Operator"); + (await StoredValue(ConfigElementKey.AuthSecurityStamp)).ShouldNotBeNullOrEmpty(); + _passwordHasher.Verify(await StoredValue(ConfigElementKey.AuthLocalAdminPasswordHash), "supersecret") + .ShouldNotBe(LocalPasswordVerification.Failed); + } + + [Test] + public async Task Handle_Should_Overwrite_Existing_Credential_And_Rotate_Stamp() + { + var claim = new ClaimLocalAdminHandler(_configElementRepository, _passwordHasher); + (await claim.Handle(new ClaimLocalAdmin("Original", "originalsecret"), CancellationToken.None)) + .IsRight.ShouldBeTrue(); + string originalStamp = await StoredValue(ConfigElementKey.AuthSecurityStamp); + + SeedLocalAdminFromEnvironmentHandler handler = MakeHandler(); + Either result = await handler.Handle( + new SeedLocalAdminFromEnvironment("Replacement", "replacementsecret"), + CancellationToken.None); + + result.IsRight.ShouldBeTrue(); + (await StoredValue(ConfigElementKey.AuthLocalAdminUsername)).ShouldBe("Replacement"); + (await StoredValue(ConfigElementKey.AuthSecurityStamp)).ShouldNotBe(originalStamp); + + string storedHash = await StoredValue(ConfigElementKey.AuthLocalAdminPasswordHash); + _passwordHasher.Verify(storedHash, "replacementsecret").ShouldNotBe(LocalPasswordVerification.Failed); + _passwordHasher.Verify(storedHash, "originalsecret").ShouldBe(LocalPasswordVerification.Failed); + } + + [Test] + public async Task Handle_Should_Default_Empty_Username_To_Admin() + { + SeedLocalAdminFromEnvironmentHandler handler = MakeHandler(); + + Either result = await handler.Handle( + new SeedLocalAdminFromEnvironment(" ", "supersecret"), + CancellationToken.None); + + result.IsRight.ShouldBeTrue(); + (await StoredValue(ConfigElementKey.AuthLocalAdminUsername)).ShouldBe("admin"); + } + + [Test] + public async Task Handle_Should_Reject_Short_Password() + { + string shortPassword = new('a', AuthConstants.MinPasswordLength - 1); + SeedLocalAdminFromEnvironmentHandler handler = MakeHandler(); + + Either result = await handler.Handle( + new SeedLocalAdminFromEnvironment("Operator", shortPassword), + CancellationToken.None); + + result.IsLeft.ShouldBeTrue(); + (await _configElementRepository.GetConfigElement( + ConfigElementKey.AuthLocalAdminPasswordHash, + CancellationToken.None)).IsNone.ShouldBeTrue(); + } +} diff --git a/ErsatzTV.Tests/Application/Auth/VerifyLocalAdminLoginHandlerTests.cs b/ErsatzTV.Tests/Application/Auth/VerifyLocalAdminLoginHandlerTests.cs new file mode 100644 index 000000000..7bb20b3e3 --- /dev/null +++ b/ErsatzTV.Tests/Application/Auth/VerifyLocalAdminLoginHandlerTests.cs @@ -0,0 +1,122 @@ +using ErsatzTV.Application.Auth; +using ErsatzTV.Core; +using ErsatzTV.Core.Domain; +using ErsatzTV.Core.Interfaces.Repositories; +using ErsatzTV.Infrastructure.Data.Repositories; +using ErsatzTV.Tests.Support; +using LanguageExt; +using NUnit.Framework; +using Shouldly; + +namespace ErsatzTV.Tests.Application.Auth; + +[TestFixture] +public class VerifyLocalAdminLoginHandlerTests +{ + private InMemoryTvContext _db = null!; + private IConfigElementRepository _configElementRepository = null!; + private ILocalPasswordHasher _passwordHasher = null!; + + private const string Username = "Operator"; + private const string Password = "supersecret"; + + [SetUp] + public async Task SetUp() + { + _db = await InMemoryTvContext.CreateAsync(); + _configElementRepository = new ConfigElementRepository(_db.Factory); + _passwordHasher = new LocalPasswordHasher(); + } + + [TearDown] + public async Task TearDown() => await _db.DisposeAsync(); + + private VerifyLocalAdminLoginHandler MakeHandler() => + new(_configElementRepository, _passwordHasher); + + private async Task SeedAdmin() + { + var claim = new ClaimLocalAdminHandler(_configElementRepository, _passwordHasher); + (await claim.Handle(new ClaimLocalAdmin(Username, Password), CancellationToken.None)) + .IsRight.ShouldBeTrue(); + } + + private async Task StoredStamp() => + (await _configElementRepository.GetValue( + ConfigElementKey.AuthSecurityStamp, + CancellationToken.None)).IfNone(""); + + [Test] + public async Task Handle_Should_Return_Principal_With_Stored_Stamp_On_Valid_Credentials() + { + await SeedAdmin(); + VerifyLocalAdminLoginHandler handler = MakeHandler(); + + Either result = await handler.Handle( + new VerifyLocalAdminLogin(Username, Password), + CancellationToken.None); + + result.IsRight.ShouldBeTrue(); + LocalAdminPrincipal principal = result.Match( + Left: e => throw new ShouldAssertException(e.ToString()), + Right: p => p); + principal.Username.ShouldBe(Username); + principal.SecurityStamp.ShouldBe(await StoredStamp()); + } + + [Test] + public async Task Handle_Should_Fail_On_Wrong_Password() + { + await SeedAdmin(); + VerifyLocalAdminLoginHandler handler = MakeHandler(); + + Either result = await handler.Handle( + new VerifyLocalAdminLogin(Username, "wrongpassword"), + CancellationToken.None); + + result.IsLeft.ShouldBeTrue(); + } + + [Test] + public async Task Handle_Should_Fail_On_Wrong_Username() + { + await SeedAdmin(); + VerifyLocalAdminLoginHandler handler = MakeHandler(); + + Either result = await handler.Handle( + new VerifyLocalAdminLogin("SomebodyElse", Password), + CancellationToken.None); + + result.IsLeft.ShouldBeTrue(); + } + + [Test] + public async Task Handle_Should_Fail_On_Unconfigured_Db() + { + VerifyLocalAdminLoginHandler handler = MakeHandler(); + + Either result = await handler.Handle( + new VerifyLocalAdminLogin(Username, Password), + CancellationToken.None); + + result.IsLeft.ShouldBeTrue(); + } + + [Test] + public async Task Handle_Should_Match_Username_Case_Insensitively() + { + await SeedAdmin(); + VerifyLocalAdminLoginHandler handler = MakeHandler(); + + Either result = await handler.Handle( + new VerifyLocalAdminLogin(Username.ToUpperInvariant(), Password), + CancellationToken.None); + + result.IsRight.ShouldBeTrue(); + LocalAdminPrincipal principal = result.Match( + Left: e => throw new ShouldAssertException(e.ToString()), + Right: p => p); + // The stored (canonical) username is returned, not the differently-cased input. + principal.Username.ShouldBe(Username); + } +} diff --git a/ErsatzTV.Tests/Controllers/ApiControllerSecurityTests.cs b/ErsatzTV.Tests/Controllers/ApiControllerSecurityTests.cs index 8b335b2b7..46775e5e7 100644 --- a/ErsatzTV.Tests/Controllers/ApiControllerSecurityTests.cs +++ b/ErsatzTV.Tests/Controllers/ApiControllerSecurityTests.cs @@ -18,7 +18,7 @@ namespace ErsatzTV.Tests.Controllers; [TestFixture] public class ApiControllerSecurityTests { - private static readonly bool ApiKeyAuthorizationFilterIsGlobal = IsApiKeyAuthorizationFilterRegisteredGlobally(); + private static readonly bool ApiAuthorizationFilterIsGlobal = IsApiAuthorizationFilterRegisteredGlobally(); [Test] public void Every_Mutating_Api_Action_Should_Be_Globally_Protected_Or_Explicitly_Exempt() @@ -47,7 +47,7 @@ public class ApiControllerSecurityTests foreach (Type controllerType in apiControllers) { bool controllerSkipsApiKey = controllerType - .GetCustomAttributes(inherit: true) + .GetCustomAttributes(inherit: true) .Any(); foreach (MethodInfo action in controllerType @@ -64,7 +64,7 @@ public class ApiControllerSecurityTests } bool actionSkipsApiKey = action - .GetCustomAttributes(inherit: true) + .GetCustomAttributes(inherit: true) .Any(); (controllerSkipsApiKey || actionSkipsApiKey || IsGloballyProtected()) @@ -74,23 +74,27 @@ public class ApiControllerSecurityTests } [Test] - public void ScannerController_Should_Be_Only_Api_Key_Exempt_Api_Controller() + public void Only_Scanner_And_Auth_Controllers_Should_Be_Auth_Exempt() { + // ScannerController: internal loopback callback, gated by [LocalhostOnly] instead of a credential. + // AuthController: the /api/auth/* surface itself must be reachable before a caller is authenticated + // (config/session/login/setup) — its sensitive action (password change) self-checks the principal. + // Any OTHER [SkipApiAuthorization] controller is a fail-open hole and must be caught here. Type[] exemptControllers = typeof(ScannerController) .Assembly .GetTypes() .Where(t => t.Namespace == typeof(ScannerController).Namespace) .Where(t => t.GetCustomAttributes(inherit: true).Any()) - .Where(t => t.GetCustomAttributes(inherit: true).Any()) + .Where(t => t.GetCustomAttributes(inherit: true).Any()) .ToArray(); - exemptControllers.ShouldBe([typeof(ScannerController)]); + exemptControllers.ShouldBe([typeof(ScannerController), typeof(AuthController)], ignoreOrder: true); } [Test] - public void Startup_Should_Register_ApiKeyAuthorizationFilter_Globally() + public void Startup_Should_Register_ApiAuthorizationFilter_Globally() { - ApiKeyAuthorizationFilterIsGlobal.ShouldBeTrue(); + ApiAuthorizationFilterIsGlobal.ShouldBeTrue(); } [Test] @@ -109,8 +113,8 @@ public class ApiControllerSecurityTests foreach (Type controllerType in sensitiveControllers) { - controllerType.GetCustomAttributes(inherit: true).Any() - .ShouldBeTrue($"{controllerType.Name} must carry [RequiresApiKey]"); + controllerType.GetCustomAttributes(inherit: true).Any() + .ShouldBeTrue($"{controllerType.Name} must carry [RequiresAuthentication]"); } } @@ -125,9 +129,9 @@ public class ApiControllerSecurityTests .ShouldBeTrue(); } - private static bool IsGloballyProtected() => ApiKeyAuthorizationFilterIsGlobal; + private static bool IsGloballyProtected() => ApiAuthorizationFilterIsGlobal; - private static bool IsApiKeyAuthorizationFilterRegisteredGlobally() + private static bool IsApiAuthorizationFilterRegisteredGlobally() { var settings = new Dictionary { @@ -155,6 +159,6 @@ public class ApiControllerSecurityTests return options.Filters .OfType() - .Any(a => a.ServiceType == typeof(ApiKeyAuthorizationFilter)); + .Any(a => a.ServiceType == typeof(ApiAuthorizationFilter)); } } diff --git a/ErsatzTV.Tests/Filters/ApiKeyAuthorizationFilterTests.cs b/ErsatzTV.Tests/Filters/ApiAuthorizationFilterTests.cs similarity index 63% rename from ErsatzTV.Tests/Filters/ApiKeyAuthorizationFilterTests.cs rename to ErsatzTV.Tests/Filters/ApiAuthorizationFilterTests.cs index 61d0cb1b5..a700c27db 100644 --- a/ErsatzTV.Tests/Filters/ApiKeyAuthorizationFilterTests.cs +++ b/ErsatzTV.Tests/Filters/ApiAuthorizationFilterTests.cs @@ -1,4 +1,5 @@ using System.Collections.Generic; +using System.Security.Claims; using ErsatzTV.Filters; using ErsatzTV.Services; using Microsoft.AspNetCore.Http; @@ -12,7 +13,7 @@ using Shouldly; namespace ErsatzTV.Tests.Filters; [TestFixture] -public class ApiKeyAuthorizationFilterTests +public class ApiAuthorizationFilterTests { private const string Key = "secret"; @@ -27,25 +28,38 @@ public class ApiKeyAuthorizationFilterTests string? apiKeyHeader, string path = "/api/channels", bool skipApiKeyAuthorization = false, - bool requiresApiKey = false) + bool requiresApiKey = false, + bool authenticatedSession = false, + bool csrfHeader = false) { var httpContext = new DefaultHttpContext(); httpContext.Request.Method = method; httpContext.Request.Path = path; if (apiKeyHeader is not null) { - httpContext.Request.Headers[ApiKeyAuthorizationFilter.HeaderName] = apiKeyHeader; + httpContext.Request.Headers[ApiAuthorizationFilter.HeaderName] = apiKeyHeader; + } + + if (csrfHeader) + { + httpContext.Request.Headers[ApiAuthorizationFilter.CsrfHeaderName] = "1"; + } + + if (authenticatedSession) + { + // A ClaimsIdentity with an authentication type reports IsAuthenticated == true. + httpContext.User = new ClaimsPrincipal(new ClaimsIdentity(authenticationType: "cookie")); } var metadata = new List(); if (skipApiKeyAuthorization) { - metadata.Add(new SkipApiKeyAuthorizationAttribute()); + metadata.Add(new SkipApiAuthorizationAttribute()); } if (requiresApiKey) { - metadata.Add(new RequiresApiKeyAttribute()); + metadata.Add(new RequiresAuthenticationAttribute()); } var actionDescriptor = new ActionDescriptor { EndpointMetadata = metadata }; @@ -53,7 +67,7 @@ public class ApiKeyAuthorizationFilterTests return new AuthorizationFilterContext(actionContext, new List()); } - private static ApiKeyAuthorizationFilter MakeFilter(bool requireKeyForReads = true) => + private static ApiAuthorizationFilter MakeFilter(bool requireKeyForReads = true) => new(new FakeApiKeyProvider(Key, requireKeyForReads)); private static void ShouldBeUnauthorized(AuthorizationFilterContext context) @@ -124,10 +138,10 @@ public class ApiKeyAuthorizationFilterTests context.Result.ShouldBeNull(); } - // ---- the sensitive-read tier stays gated even with reads open ([RequiresApiKey]) ---- + // ---- the sensitive-read tier stays gated even with reads open ([RequiresAuthentication]) ---- [Test] - public void Should_Reject_Read_On_RequiresApiKey_Endpoint_Even_When_Reads_Not_Required() + public void Should_Reject_Read_On_RequiresAuthentication_Endpoint_Even_When_Reads_Not_Required() { AuthorizationFilterContext context = MakeContext("GET", apiKeyHeader: null, requiresApiKey: true); MakeFilter(requireKeyForReads: false).OnAuthorization(context); @@ -135,7 +149,7 @@ public class ApiKeyAuthorizationFilterTests } [Test] - public void Should_Allow_Read_On_RequiresApiKey_Endpoint_When_Key_Correct() + public void Should_Allow_Read_On_RequiresAuthentication_Endpoint_When_Key_Correct() { AuthorizationFilterContext context = MakeContext("GET", apiKeyHeader: Key, requiresApiKey: true); MakeFilter(requireKeyForReads: false).OnAuthorization(context); @@ -172,4 +186,54 @@ public class ApiKeyAuthorizationFilterTests MakeFilter(requireKeyForReads: true).OnAuthorization(context); context.Result.ShouldBeNull(); } + + // ---- session (cookie) authentication accepted as an alternative credential (issue #295) ---- + + [Test] + public void Should_Allow_Read_When_Session_Authenticated_Without_Key() + { + AuthorizationFilterContext context = MakeContext("GET", apiKeyHeader: null, authenticatedSession: true); + MakeFilter(requireKeyForReads: true).OnAuthorization(context); + context.Result.ShouldBeNull(); + } + + [Test] + public void Should_Allow_Session_Mutation_When_Csrf_Header_Present() + { + AuthorizationFilterContext context = + MakeContext("POST", apiKeyHeader: null, authenticatedSession: true, csrfHeader: true); + MakeFilter().OnAuthorization(context); + context.Result.ShouldBeNull(); + } + + [Test] + public void Should_Reject_Session_Mutation_When_Csrf_Header_Missing() + { + AuthorizationFilterContext context = + MakeContext("POST", apiKeyHeader: null, authenticatedSession: true, csrfHeader: false); + MakeFilter().OnAuthorization(context); + + var result = context.Result.ShouldBeOfType(); + result.StatusCode.ShouldBe(StatusCodes.Status403Forbidden); + var problemDetails = result.Value.ShouldBeOfType(); + problemDetails.Status.ShouldBe(StatusCodes.Status403Forbidden); + } + + [Test] + public void Should_Reject_When_Neither_Key_Nor_Session_Present() + { + AuthorizationFilterContext context = MakeContext("GET", apiKeyHeader: null, authenticatedSession: false); + MakeFilter(requireKeyForReads: true).OnAuthorization(context); + ShouldBeUnauthorized(context); + } + + [Test] + public void Should_Prefer_Machine_Key_Over_Session_And_Skip_Csrf() + { + // A valid X-Api-Key is CSRF-immune: a session cookie riding along must not force a CSRF check. + AuthorizationFilterContext context = + MakeContext("POST", apiKeyHeader: Key, authenticatedSession: true, csrfHeader: false); + MakeFilter().OnAuthorization(context); + context.Result.ShouldBeNull(); + } } diff --git a/ErsatzTV.Tests/Filters/ApiKeyEndpointRequiresKeyTests.cs b/ErsatzTV.Tests/Filters/ApiKeyEndpointRequiresKeyTests.cs index 5aaf8f39d..fbcc5b190 100644 --- a/ErsatzTV.Tests/Filters/ApiKeyEndpointRequiresKeyTests.cs +++ b/ErsatzTV.Tests/Filters/ApiKeyEndpointRequiresKeyTests.cs @@ -12,7 +12,7 @@ using Shouldly; namespace ErsatzTV.Tests.Filters; /// -/// Proves the shared predicate — the one +/// Proves the shared predicate — the one /// the OpenAPI security/401 transformer consumes — agrees with what the filter actually enforces at /// runtime, across a representative matrix. If the two ever diverged, the spec could claim an endpoint /// is open while the filter gates it (or vice-versa); this test is the anti-drift guard (#287). @@ -37,10 +37,10 @@ public class ApiKeyEndpointRequiresKeyTests new object[] { "DELETE", false, false, false, true }, new object[] { "GET", false, false, true, true }, // read gated when reads-required new object[] { "GET", false, false, false, false }, // read open when reads-not-required - new object[] { "GET", true, false, false, true }, // [RequiresApiKey] gates read even so + new object[] { "GET", true, false, false, true }, // [RequiresAuthentication] gates read even so new object[] { "HEAD", false, false, false, false }, // read verb, open new object[] { "OPTIONS", false, false, true, false },// preflight always exempt - new object[] { "POST", false, true, false, false } // [SkipApiKeyAuthorization] exempt + new object[] { "POST", false, true, false, false } // [SkipApiAuthorization] exempt ]; [TestCaseSource(nameof(Matrix))] @@ -54,15 +54,15 @@ public class ApiKeyEndpointRequiresKeyTests var metadata = new List(); if (requiresApiKey) { - metadata.Add(new RequiresApiKeyAttribute()); + metadata.Add(new RequiresAuthenticationAttribute()); } if (skip) { - metadata.Add(new SkipApiKeyAuthorizationAttribute()); + metadata.Add(new SkipApiAuthorizationAttribute()); } - ApiKeyAuthorizationFilter.EndpointRequiresKey(method, metadata, requireKeyForReads) + ApiAuthorizationFilter.EndpointRequiresKey(method, metadata, requireKeyForReads) .ShouldBe(expected); } @@ -77,12 +77,12 @@ public class ApiKeyEndpointRequiresKeyTests // The filter, on an /api path with the header MISSING, produces a 401 exactly when the predicate // says the endpoint requires a key. Drive the real filter and compare its decision to the predicate. AuthorizationFilterContext context = MakeContext(method, requiresApiKey, skip); - new ApiKeyAuthorizationFilter(new FakeApiKeyProvider(requireKeyForReads)).OnAuthorization(context); + new ApiAuthorizationFilter(new FakeApiKeyProvider(requireKeyForReads)).OnAuthorization(context); bool filterGated = context.Result is not null; filterGated.ShouldBe(expected); filterGated.ShouldBe( - ApiKeyAuthorizationFilter.EndpointRequiresKey(method, Metadata(requiresApiKey, skip), requireKeyForReads)); + ApiAuthorizationFilter.EndpointRequiresKey(method, Metadata(requiresApiKey, skip), requireKeyForReads)); } [Test] @@ -90,11 +90,11 @@ public class ApiKeyEndpointRequiresKeyTests { // The predicate assumes an /api endpoint; the filter's path scoping precedes it. A mutating request // outside /api must pass untouched even though the predicate (given the same method) returns true. - ApiKeyAuthorizationFilter.EndpointRequiresKey("POST", new List(), requireKeyForReads: true) + ApiAuthorizationFilter.EndpointRequiresKey("POST", new List(), requireKeyForReads: true) .ShouldBeTrue(); AuthorizationFilterContext context = MakeContext("POST", requiresApiKey: false, skip: false, path: "/iptv/x.m3u"); - new ApiKeyAuthorizationFilter(new FakeApiKeyProvider(requireKeyForReads: true)).OnAuthorization(context); + new ApiAuthorizationFilter(new FakeApiKeyProvider(requireKeyForReads: true)).OnAuthorization(context); context.Result.ShouldBeNull(); } @@ -104,12 +104,12 @@ public class ApiKeyEndpointRequiresKeyTests var metadata = new List(); if (requiresApiKey) { - metadata.Add(new RequiresApiKeyAttribute()); + metadata.Add(new RequiresAuthenticationAttribute()); } if (skip) { - metadata.Add(new SkipApiKeyAuthorizationAttribute()); + metadata.Add(new SkipApiAuthorizationAttribute()); } return metadata; diff --git a/ErsatzTV/Auth/CookieSecurityStampValidator.cs b/ErsatzTV/Auth/CookieSecurityStampValidator.cs new file mode 100644 index 000000000..354817f98 --- /dev/null +++ b/ErsatzTV/Auth/CookieSecurityStampValidator.cs @@ -0,0 +1,38 @@ +using System.Security.Claims; +using ErsatzTV.Application.Auth; +using LanguageExt; +using MediatR; +using Microsoft.AspNetCore.Authentication; +using Microsoft.AspNetCore.Authentication.Cookies; + +namespace ErsatzTV.Auth; + +/// +/// Cookie OnValidatePrincipal handler enforcing local-admin security-stamp revocation (#295): +/// a local-login session carries the admin's security stamp as a claim; a password change rotates the +/// stored stamp, so a mismatch here rejects the (now stale) session on its next request. OIDC sessions +/// carry no stamp claim and are governed by the IdP, so they are skipped. +/// +public static class CookieSecurityStampValidator +{ + public static async Task ValidateAsync(CookieValidatePrincipalContext context) + { + ClaimsPrincipal principal = context.Principal; + string method = principal?.FindFirst(AuthConstants.AuthMethodClaim)?.Value; + if (!string.Equals(method, AuthConstants.MethodLocal, StringComparison.Ordinal)) + { + return; + } + + string presented = principal.FindFirst(AuthConstants.SecurityStampClaim)?.Value; + IMediator mediator = context.HttpContext.RequestServices.GetRequiredService(); + Option stored = await mediator.Send(new GetLocalAdminSecurityStamp()); + string current = stored.IfNone(string.Empty); + + if (string.IsNullOrEmpty(presented) || !string.Equals(presented, current, StringComparison.Ordinal)) + { + context.RejectPrincipal(); + await context.HttpContext.SignOutAsync(AuthConstants.CookieScheme); + } + } +} diff --git a/ErsatzTV/Controllers/Api/AuthController.cs b/ErsatzTV/Controllers/Api/AuthController.cs new file mode 100644 index 000000000..7734c2306 --- /dev/null +++ b/ErsatzTV/Controllers/Api/AuthController.cs @@ -0,0 +1,161 @@ +using System.Security.Claims; +using ErsatzTV.Application.Auth; +using ErsatzTV.Controllers.Api.Requests; +using ErsatzTV.Core; +using ErsatzTV.Extensions; +using ErsatzTV.Filters; +using MediatR; +using Microsoft.AspNetCore.Authentication; +using Microsoft.AspNetCore.Http; +using Microsoft.AspNetCore.Mvc; +using Microsoft.AspNetCore.RateLimiting; + +namespace ErsatzTV.Controllers.Api; + +/// +/// Browser SPA session authentication (issue #295). Excluded from the OpenAPI document — the spec's +/// audience is machine X-Api-Key clients, and a browser-interactive cookie login is not something a +/// generated client drives — and exempt from the global (this surface +/// must be reachable before a caller has a session). Sensitive operations self-check the principal. +/// +[ApiController] +[ApiExplorerSettings(IgnoreApi = true)] +[SkipApiAuthorization] +public class AuthController(IMediator mediator) : ControllerBase +{ + /// Public: what auth options exist + whether first-run setup is still needed (drives the SPA boot gate). + [HttpGet("/api/auth/config")] + public async Task Config(CancellationToken cancellationToken) + { + bool configured = await mediator.Send(new IsLocalAdminConfigured(), cancellationToken); + return Ok(new AuthConfigResponse(OidcHelper.IsEnabled, true, !configured)); + } + + /// The current session (anonymous is a 200 with authenticated=false, never a 401). + [HttpGet("/api/auth/session")] + public IActionResult Session() + { + if (User.Identity?.IsAuthenticated != true) + { + return Ok(new AuthSessionResponse(false, null, null)); + } + + return Ok(new AuthSessionResponse(true, User.Identity?.Name, User.FindFirst(AuthConstants.AuthMethodClaim)?.Value)); + } + + /// First-run setup-claim: create the local admin. Fails 409 if one already exists. + [HttpPost("/api/auth/setup")] + [EnableRateLimiting("auth")] + public async Task Setup([FromBody] SetupRequest request, CancellationToken cancellationToken) + { + if (await mediator.Send(new IsLocalAdminConfigured(), cancellationToken)) + { + return ApiResults.ConflictProblem("Already configured", "A local administrator already exists."); + } + + Either result = + await mediator.Send(new ClaimLocalAdmin(request.Username, request.Password), cancellationToken); + + return await result.Match( + Left: error => Task.FromResult(error.ToErrorResult()), + Right: async principal => + { + await IssueLocalCookieAsync(principal); + return (IActionResult)Ok(new AuthSessionResponse(true, principal.Username, AuthConstants.MethodLocal)); + }); + } + + /// Local username/password login. A generic 401 on any failure (no username enumeration). + [HttpPost("/api/auth/login")] + [EnableRateLimiting("auth")] + public async Task Login([FromBody] LoginRequest request, CancellationToken cancellationToken) + { + Either result = + await mediator.Send(new VerifyLocalAdminLogin(request.Username, request.Password), cancellationToken); + + return await result.Match( + Left: _ => Task.FromResult((IActionResult)Unauthorized(new ProblemDetails + { + Status = StatusCodes.Status401Unauthorized, + Title = "Unauthorized", + Detail = "Invalid username or password." + })), + Right: async principal => + { + await IssueLocalCookieAsync(principal); + return (IActionResult)Ok(new AuthSessionResponse(true, principal.Username, AuthConstants.MethodLocal)); + }); + } + + /// Sign out of the cookie session. + [HttpPost("/api/auth/logout")] + public async Task Logout() + { + await HttpContext.SignOutAsync(AuthConstants.CookieScheme); + return NoContent(); + } + + /// Change the local admin password (requires a local-login session); rotates the stamp, revoking other sessions. + [HttpPost("/api/auth/password")] + [EnableRateLimiting("auth")] + public async Task ChangePassword([FromBody] ChangePasswordRequest request, CancellationToken cancellationToken) + { + // A machine key must not be able to rotate the local admin's password — require a local session. + if (User.Identity?.IsAuthenticated != true || + !string.Equals( + User.FindFirst(AuthConstants.AuthMethodClaim)?.Value, + AuthConstants.MethodLocal, + StringComparison.Ordinal)) + { + return Unauthorized(new ProblemDetails + { + Status = StatusCodes.Status401Unauthorized, + Title = "Unauthorized", + Detail = "A local-login session is required to change the password." + }); + } + + Either result = await mediator.Send( + new ChangeLocalAdminPassword(User.Identity?.Name, request.CurrentPassword, request.NewPassword), + cancellationToken); + + return await result.Match( + Left: error => Task.FromResult(error.ToErrorResult()), + Right: async principal => + { + // Re-issue the cookie with the rotated stamp so THIS session survives while others are revoked. + await IssueLocalCookieAsync(principal); + return (IActionResult)NoContent(); + }); + } + + /// Browser-navigation OIDC challenge. Outside /api (a top-level GET redirect). 404 when OIDC is off. + [HttpGet("/auth/oidc/login")] + public IActionResult OidcLogin() + { + if (!OidcHelper.IsEnabled) + { + return NotFound(); + } + + return Challenge( + new AuthenticationProperties { RedirectUri = $"{Request.PathBase}/app" }, + AuthConstants.OidcScheme); + } + + private Task IssueLocalCookieAsync(LocalAdminPrincipal principal) + { + var claims = new List + { + new(ClaimTypes.Name, principal.Username), + new(AuthConstants.AuthMethodClaim, AuthConstants.MethodLocal), + new(AuthConstants.SecurityStampClaim, principal.SecurityStamp) + }; + var identity = new ClaimsIdentity(claims, AuthConstants.CookieScheme); + return HttpContext.SignInAsync(AuthConstants.CookieScheme, new ClaimsPrincipal(identity)); + } +} + +public record AuthConfigResponse(bool OidcEnabled, bool LocalLoginEnabled, bool SetupRequired); + +public record AuthSessionResponse(bool Authenticated, string Username, string Method); diff --git a/ErsatzTV/Controllers/Api/LogsController.cs b/ErsatzTV/Controllers/Api/LogsController.cs index 301e4b65b..55746db02 100644 --- a/ErsatzTV/Controllers/Api/LogsController.cs +++ b/ErsatzTV/Controllers/Api/LogsController.cs @@ -9,7 +9,7 @@ using Microsoft.AspNetCore.Mvc; namespace ErsatzTV.Controllers.Api; [ApiController] -[RequiresApiKey] +[RequiresAuthentication] public class LogsController(IMediator mediator) : ControllerBase { private const int MaxPageSize = 100; diff --git a/ErsatzTV/Controllers/Api/MaintenanceController.cs b/ErsatzTV/Controllers/Api/MaintenanceController.cs index 8330a43d1..e8d66527a 100644 --- a/ErsatzTV/Controllers/Api/MaintenanceController.cs +++ b/ErsatzTV/Controllers/Api/MaintenanceController.cs @@ -11,7 +11,7 @@ namespace ErsatzTV.Controllers.Api; [ApiController] [EndpointGroupName("general")] -[RequiresApiKey] +[RequiresAuthentication] public class MaintenanceController(IMediator mediator, ChannelWriter workerChannel) { [HttpPost("/api/maintenance/gc")] diff --git a/ErsatzTV/Controllers/Api/Requests/AuthRequests.cs b/ErsatzTV/Controllers/Api/Requests/AuthRequests.cs new file mode 100644 index 000000000..82d7028f5 --- /dev/null +++ b/ErsatzTV/Controllers/Api/Requests/AuthRequests.cs @@ -0,0 +1,7 @@ +namespace ErsatzTV.Controllers.Api.Requests; + +public record LoginRequest(string Username, string Password); + +public record SetupRequest(string Username, string Password); + +public record ChangePasswordRequest(string CurrentPassword, string NewPassword); diff --git a/ErsatzTV/Controllers/Api/ScannerController.cs b/ErsatzTV/Controllers/Api/ScannerController.cs index a466cf58d..cc428b669 100644 --- a/ErsatzTV/Controllers/Api/ScannerController.cs +++ b/ErsatzTV/Controllers/Api/ScannerController.cs @@ -9,7 +9,7 @@ namespace ErsatzTV.Controllers.Api; [ApiController] [ApiExplorerSettings(IgnoreApi = true)] -[SkipApiKeyAuthorization] +[SkipApiAuthorization] [LocalhostOnly] [Route("api/scan/{scanId:guid}")] public class ScannerController( diff --git a/ErsatzTV/Controllers/Api/SettingsController.cs b/ErsatzTV/Controllers/Api/SettingsController.cs index a29063b3d..5d16159b7 100644 --- a/ErsatzTV/Controllers/Api/SettingsController.cs +++ b/ErsatzTV/Controllers/Api/SettingsController.cs @@ -19,7 +19,7 @@ namespace ErsatzTV.Controllers.Api; [ApiController] [EndpointGroupName("general")] -[RequiresApiKey] +[RequiresAuthentication] public class SettingsController(IMediator mediator) : ControllerBase { // FFmpeg settings diff --git a/ErsatzTV/Controllers/Api/TroubleshootController.cs b/ErsatzTV/Controllers/Api/TroubleshootController.cs index ca313de74..f2b2158b0 100644 --- a/ErsatzTV/Controllers/Api/TroubleshootController.cs +++ b/ErsatzTV/Controllers/Api/TroubleshootController.cs @@ -27,7 +27,7 @@ using Serilog.Context; namespace ErsatzTV.Controllers.Api; [ApiController] -[RequiresApiKey] +[RequiresAuthentication] public class TroubleshootController( ChannelWriter channelWriter, IFileSystem fileSystem, @@ -92,7 +92,8 @@ public class TroubleshootController( [ProducesResponseType(typeof(ValidateSequentialScheduleResponseModel), StatusCodes.Status200OK)] [ProducesResponseType(typeof(ProblemDetails), StatusCodes.Status400BadRequest)] public async Task ValidateSchedule( - [Required] [FromBody] ValidateSequentialScheduleRequest request, + [Required] [FromBody] + ValidateSequentialScheduleRequest request, CancellationToken cancellationToken) { if (string.IsNullOrWhiteSpace(request.Yaml)) diff --git a/ErsatzTV/Filters/ApiAuthorizationFilter.cs b/ErsatzTV/Filters/ApiAuthorizationFilter.cs new file mode 100644 index 000000000..3c0b87bb1 --- /dev/null +++ b/ErsatzTV/Filters/ApiAuthorizationFilter.cs @@ -0,0 +1,144 @@ +using System.Security.Cryptography; +using System.Text; +using ErsatzTV.Services; +using Microsoft.AspNetCore.Http; +using Microsoft.AspNetCore.Mvc; +using Microsoft.AspNetCore.Mvc.Filters; +using Microsoft.Extensions.Primitives; + +namespace ErsatzTV.Filters; + +/// +/// Authorization for the JSON API. A request under /api/* that requires authentication is +/// accepted on either of two credentials (issue #295): +/// +/// a matching machine X-Api-Key header (MCP / external clients), or +/// an authenticated session principal (browser cookie, from OIDC or local login). +/// +/// Which endpoints require authentication is decided by — the +/// single predicate shared with OpenAPI generation — so the spec can never drift from enforcement: +/// mutating verbs are always fail-closed; reads are gated when Api:RequireKeyForReads is set +/// (the default) or the endpoint carries . Endpoints +/// marked and anything outside /api (e.g. +/// /iptv/*, /artwork/*, the SPA) are never affected. +/// +/// The machine key is CSRF-immune (a browser cannot set a custom header cross-origin without a +/// credentialed CORS grant, which is never issued). A cookie session is not: session-authenticated +/// mutations must additionally carry the header, which — being a +/// custom header — forces a CORS preflight that a cross-site attacker page cannot satisfy. +/// +/// +public class ApiAuthorizationFilter(IApiKeyProvider apiKeyProvider) : IAuthorizationFilter +{ + public const string HeaderName = "X-Api-Key"; + + /// + /// Required on session-authenticated mutating requests as a CSRF defense. Presence is the whole + /// check — a custom request header cannot be set by a cross-site form/navigation and forces a CORS + /// preflight for cross-origin XHR, so only same-origin (the SPA) can send it. + /// + public const string CsrfHeaderName = "X-CSRF"; + + public void OnAuthorization(AuthorizationFilterContext context) + { + HttpRequest request = context.HttpContext.Request; + + // Never gate anything outside the JSON API surface. + if (!request.Path.StartsWithSegments("/api", StringComparison.OrdinalIgnoreCase)) + { + return; + } + + if (!EndpointRequiresKey(request.Method, context.ActionDescriptor.EndpointMetadata, apiKeyProvider.RequireKeyForReads)) + { + return; + } + + // 1. Machine key wins and is CSRF-immune. Check it first so a browser that happens to hold both + // a cookie and a key is still treated as a machine caller (no CSRF header required). + if (request.Headers.TryGetValue(HeaderName, out StringValues provided) + && KeysMatch(provided.ToString(), apiKeyProvider.ApiKey)) + { + return; + } + + // 2. Authenticated session (cookie principal from OIDC or local login). + if (context.HttpContext.User?.Identity?.IsAuthenticated == true) + { + // Session-authenticated mutations require the CSRF header. Reads are safe (SameSite=Lax + + // no credentialed CORS means a cross-site read can't be issued with the cookie either). + if (IsMutating(request.Method) && !request.Headers.ContainsKey(CsrfHeaderName)) + { + context.Result = new ObjectResult(new ProblemDetails + { + Status = StatusCodes.Status403Forbidden, + Title = "Forbidden", + Detail = $"Session-authenticated writes require the '{CsrfHeaderName}' header." + }) + { + StatusCode = StatusCodes.Status403Forbidden + }; + } + + return; + } + + // 3. Neither credential presented. + context.Result = new UnauthorizedObjectResult(new ProblemDetails + { + Status = StatusCodes.Status401Unauthorized, + Title = "Unauthorized", + Detail = $"Authentication is required. Provide a valid '{HeaderName}' header (machine clients) " + + "or sign in to obtain a session (browser)." + }); + } + + /// + /// The single decision shared between runtime enforcement (this filter) and the OpenAPI document + /// generation (ApiSecurityOperationTransformer), so the spec's declared + /// security/401 can never drift from what is actually enforced. Assumes the endpoint + /// is already known to be under /api (the caller's responsibility). Returns + /// when the endpoint requires authentication: any mutating verb + /// (POST/PUT/PATCH/DELETE) is always fail-closed; reads are gated when + /// is set or the endpoint carries + /// . OPTIONS preflight and endpoints marked + /// are exempt. (The name is retained from the + /// API-key-only era for spec-generation stability; "key" here means "credential".) + /// + public static bool EndpointRequiresKey( + string httpMethod, + IEnumerable endpointMetadata, + bool requireKeyForReads) + { + // CORS preflight carries no custom headers and is handled by the CORS middleware. + if (HttpMethods.IsOptions(httpMethod)) + { + return false; + } + + // Explicit opt-out for internal, separately-guarded endpoints (scanner callback, /api/auth/*). + if (endpointMetadata.OfType().Any()) + { + return false; + } + + // Writes are always fail-closed; reads are gated by the global flag or a per-endpoint opt-in. + return IsMutating(httpMethod) + || requireKeyForReads + || endpointMetadata.OfType().Any(); + } + + private static bool IsMutating(string httpMethod) => + HttpMethods.IsPost(httpMethod) + || HttpMethods.IsPut(httpMethod) + || HttpMethods.IsPatch(httpMethod) + || HttpMethods.IsDelete(httpMethod); + + // Compare in constant time so a remote attacker cannot use response-timing to recover the + // key prefix-by-prefix. FixedTimeEquals also short-circuits length differences without + // leaking anything beyond "lengths differ" (still not the matching-prefix length). + private static bool KeysMatch(string provided, string configured) => + CryptographicOperations.FixedTimeEquals( + Encoding.UTF8.GetBytes(provided), + Encoding.UTF8.GetBytes(configured)); +} diff --git a/ErsatzTV/Filters/ApiKeyAuthorizationFilter.cs b/ErsatzTV/Filters/ApiKeyAuthorizationFilter.cs deleted file mode 100644 index 419b2af27..000000000 --- a/ErsatzTV/Filters/ApiKeyAuthorizationFilter.cs +++ /dev/null @@ -1,98 +0,0 @@ -using System.Security.Cryptography; -using System.Text; -using ErsatzTV.Services; -using Microsoft.AspNetCore.Http; -using Microsoft.AspNetCore.Mvc; -using Microsoft.AspNetCore.Mvc.Filters; -using Microsoft.Extensions.Primitives; - -namespace ErsatzTV.Filters; - -/// -/// API-key authorization for the JSON API. The effective key (from ) -/// is never empty, so this is fail-closed by construction (issue #280): every mutating request -/// under /api/* must present a matching X-Api-Key header. Read (GET/HEAD) requests -/// under /api/* are also gated when Api:RequireKeyForReads is enabled (the default) -/// or the endpoint carries . Endpoints marked -/// and anything outside /api (e.g. -/// /iptv/*, /artwork/*, the SPA) are never affected. Independent of -/// . -/// -public class ApiKeyAuthorizationFilter(IApiKeyProvider apiKeyProvider) : IAuthorizationFilter -{ - public const string HeaderName = "X-Api-Key"; - - public void OnAuthorization(AuthorizationFilterContext context) - { - HttpRequest request = context.HttpContext.Request; - - // Never gate anything outside the JSON API surface. - if (!request.Path.StartsWithSegments("/api", StringComparison.OrdinalIgnoreCase)) - { - return; - } - - if (!EndpointRequiresKey(request.Method, context.ActionDescriptor.EndpointMetadata, apiKeyProvider.RequireKeyForReads)) - { - return; - } - - if (!request.Headers.TryGetValue(HeaderName, out StringValues provided) - || !KeysMatch(provided.ToString(), apiKeyProvider.ApiKey)) - { - context.Result = new UnauthorizedObjectResult(new ProblemDetails - { - Status = StatusCodes.Status401Unauthorized, - Title = "Unauthorized", - Detail = $"A valid API key is required. Provide it via the '{HeaderName}' header." - }); - } - } - - /// - /// The single decision shared between runtime enforcement (this filter) and the OpenAPI - /// document generation (ApiSecurityOperationTransformer), so the spec's declared - /// security/401 can never drift from what is actually enforced. Assumes the - /// endpoint is already known to be under /api (the caller's responsibility). Returns - /// when the endpoint requires a valid X-Api-Key: any mutating - /// verb (POST/PUT/PATCH/DELETE) is always fail-closed; reads are gated when - /// is set or the endpoint carries - /// . OPTIONS preflight and endpoints marked - /// are exempt. - /// - public static bool EndpointRequiresKey( - string httpMethod, - IEnumerable endpointMetadata, - bool requireKeyForReads) - { - // CORS preflight carries no custom headers and is handled by the CORS middleware. - if (HttpMethods.IsOptions(httpMethod)) - { - return false; - } - - // Explicit opt-out for internal, separately-guarded endpoints (e.g. the scanner callback). - if (endpointMetadata.OfType().Any()) - { - return false; - } - - bool isMutating = HttpMethods.IsPost(httpMethod) - || HttpMethods.IsPut(httpMethod) - || HttpMethods.IsPatch(httpMethod) - || HttpMethods.IsDelete(httpMethod); - - // Writes are always fail-closed; reads are gated by the global flag or a per-endpoint opt-in. - return isMutating - || requireKeyForReads - || endpointMetadata.OfType().Any(); - } - - // Compare in constant time so a remote attacker cannot use response-timing to recover the - // key prefix-by-prefix. FixedTimeEquals also short-circuits length differences without - // leaking anything beyond "lengths differ" (still not the matching-prefix length). - private static bool KeysMatch(string provided, string configured) => - CryptographicOperations.FixedTimeEquals( - Encoding.UTF8.GetBytes(provided), - Encoding.UTF8.GetBytes(configured)); -} diff --git a/ErsatzTV/Filters/RequiresApiKeyAttribute.cs b/ErsatzTV/Filters/RequiresApiKeyAttribute.cs deleted file mode 100644 index 4e444d29d..000000000 --- a/ErsatzTV/Filters/RequiresApiKeyAttribute.cs +++ /dev/null @@ -1,12 +0,0 @@ -using Microsoft.AspNetCore.Mvc.Filters; - -namespace ErsatzTV.Filters; - -/// -/// Marks a read (GET/HEAD) API endpoint as always requiring the X-Api-Key header, even -/// when Api:RequireKeyForReads is disabled. Applied to the sensitive-read tier -/// (troubleshoot, logs, settings, maintenance) that discloses secrets/paths or triggers work. -/// The mirror of . See issue #282. -/// -[AttributeUsage(AttributeTargets.Class | AttributeTargets.Method)] -public sealed class RequiresApiKeyAttribute : Attribute, IFilterMetadata; diff --git a/ErsatzTV/Filters/RequiresAuthenticationAttribute.cs b/ErsatzTV/Filters/RequiresAuthenticationAttribute.cs new file mode 100644 index 000000000..7ab24596c --- /dev/null +++ b/ErsatzTV/Filters/RequiresAuthenticationAttribute.cs @@ -0,0 +1,14 @@ +using Microsoft.AspNetCore.Mvc.Filters; + +namespace ErsatzTV.Filters; + +/// +/// Marks a read (GET/HEAD) API endpoint as always requiring authentication (a valid machine +/// X-Api-Key header or an authenticated session), even when Api:RequireKeyForReads +/// is disabled. Applied to the sensitive-read tier (troubleshoot, logs, settings, maintenance) +/// that discloses secrets/paths or triggers work. An authenticated session satisfies this tier +/// just as the machine key does. The mirror of . +/// See issues #282, #295. +/// +[AttributeUsage(AttributeTargets.Class | AttributeTargets.Method)] +public sealed class RequiresAuthenticationAttribute : Attribute, IFilterMetadata; diff --git a/ErsatzTV/Filters/SkipApiAuthorizationAttribute.cs b/ErsatzTV/Filters/SkipApiAuthorizationAttribute.cs new file mode 100644 index 000000000..a9d428855 --- /dev/null +++ b/ErsatzTV/Filters/SkipApiAuthorizationAttribute.cs @@ -0,0 +1,12 @@ +using Microsoft.AspNetCore.Mvc.Filters; + +namespace ErsatzTV.Filters; + +/// +/// Marks an internal API endpoint as exempt from the global +/// (neither a machine key nor a session is required). Used for endpoints that are guarded by a +/// different mechanism (e.g. the scanner callback's localhost-only check) and for the +/// /api/auth/* surface itself, which must be reachable before a caller is authenticated. +/// +[AttributeUsage(AttributeTargets.Class | AttributeTargets.Method)] +public sealed class SkipApiAuthorizationAttribute : Attribute, IFilterMetadata; diff --git a/ErsatzTV/Filters/SkipApiKeyAuthorizationAttribute.cs b/ErsatzTV/Filters/SkipApiKeyAuthorizationAttribute.cs deleted file mode 100644 index 4a1f7e7ea..000000000 --- a/ErsatzTV/Filters/SkipApiKeyAuthorizationAttribute.cs +++ /dev/null @@ -1,9 +0,0 @@ -using Microsoft.AspNetCore.Mvc.Filters; - -namespace ErsatzTV.Filters; - -/// -/// Marks an internal API endpoint as exempt from global API-key write authorization. -/// -[AttributeUsage(AttributeTargets.Class | AttributeTargets.Method)] -public sealed class SkipApiKeyAuthorizationAttribute : Attribute, IFilterMetadata; diff --git a/ErsatzTV/Serialization/ApiSecurityOperationTransformer.cs b/ErsatzTV/Serialization/ApiSecurityOperationTransformer.cs index 8ee72f88b..4fef0c4d5 100644 --- a/ErsatzTV/Serialization/ApiSecurityOperationTransformer.cs +++ b/ErsatzTV/Serialization/ApiSecurityOperationTransformer.cs @@ -11,7 +11,7 @@ namespace ErsatzTV.Serialization; /// every operation that actually requires the X-Api-Key header it injects the /// ApiKey security requirement and a documented 401 response. The "requires a key" /// decision is the exact same predicate the runtime filter enforces -/// (), so the spec can never claim an +/// (), so the spec can never claim an /// endpoint is open when it is gated (or vice-versa). The ApiKey scheme itself and the /// ProblemDetails schema the 401 references are declared by /// . See issues #286/#287. @@ -28,7 +28,7 @@ public sealed class ApiSecurityOperationTransformer(IApiKeyProvider apiKeyProvid string method = context.Description.HttpMethod ?? string.Empty; IEnumerable metadata = context.Description.ActionDescriptor.EndpointMetadata; - if (!ApiKeyAuthorizationFilter.EndpointRequiresKey(method, metadata, apiKeyProvider.RequireKeyForReads)) + if (!ApiAuthorizationFilter.EndpointRequiresKey(method, metadata, apiKeyProvider.RequireKeyForReads)) { return Task.CompletedTask; } diff --git a/ErsatzTV/Serialization/ApiSecuritySchemeDocumentTransformer.cs b/ErsatzTV/Serialization/ApiSecuritySchemeDocumentTransformer.cs index 6b00164f1..254befc4d 100644 --- a/ErsatzTV/Serialization/ApiSecuritySchemeDocumentTransformer.cs +++ b/ErsatzTV/Serialization/ApiSecuritySchemeDocumentTransformer.cs @@ -28,7 +28,7 @@ public static class ApiSecuritySchemeDocumentTransformer document.Components.SecuritySchemes[ApiSecurityOperationTransformer.SchemeName] = new OpenApiSecurityScheme { Type = SecuritySchemeType.ApiKey, - Name = ApiKeyAuthorizationFilter.HeaderName, + Name = ApiAuthorizationFilter.HeaderName, In = ParameterLocation.Header, Description = "API key sent in the 'X-Api-Key' request header. Required for all mutating requests and, " + diff --git a/ErsatzTV/Services/ApiKeyProvider.cs b/ErsatzTV/Services/ApiKeyProvider.cs index e2ef782e6..91b1c1dbe 100644 --- a/ErsatzTV/Services/ApiKeyProvider.cs +++ b/ErsatzTV/Services/ApiKeyProvider.cs @@ -5,7 +5,7 @@ using ErsatzTV.Core; namespace ErsatzTV.Services; /// -/// Resolves the effective API key used by and +/// Resolves the effective API key used by and /// the read-gating policy. Resolved once at startup: the configured Api:WriteKey wins; /// otherwise a previously-persisted key is loaded from the config volume; otherwise a fresh /// 256-bit key is generated and persisted. The key is never empty, so write authorization is @@ -136,7 +136,7 @@ public sealed class ApiKeyProvider : IApiKeyProvider "Generated a new API key and saved it to {Path}. Send it via the '{HeaderName}' header " + "(ChicoryTV: Settings → API Key). The key is required for API requests.", path, - Filters.ApiKeyAuthorizationFilter.HeaderName); + Filters.ApiAuthorizationFilter.HeaderName); } catch (Exception ex) { diff --git a/ErsatzTV/Services/LocalAdminSeedService.cs b/ErsatzTV/Services/LocalAdminSeedService.cs new file mode 100644 index 000000000..773e72525 --- /dev/null +++ b/ErsatzTV/Services/LocalAdminSeedService.cs @@ -0,0 +1,44 @@ +using ErsatzTV.Application.Auth; +using ErsatzTV.Core; +using LanguageExt; +using MediatR; + +namespace ErsatzTV.Services; + +/// +/// Recovery/bootstrap: when Auth:LocalAdmin:Password is configured, (re)seeds the single local +/// administrator at startup (issue #295). Overwrites any existing credential and rotates the security +/// stamp, so an operator locked out of the browser UI can reset by setting the env and restarting. Runs +/// after so the schema exists. A no-op when unset. +/// +public class LocalAdminSeedService( + IServiceScopeFactory serviceScopeFactory, + IConfiguration configuration, + ILogger logger) : IHostedService +{ + public async Task StartAsync(CancellationToken cancellationToken) + { + string password = configuration["Auth:LocalAdmin:Password"]; + if (string.IsNullOrWhiteSpace(password)) + { + return; + } + + string username = configuration["Auth:LocalAdmin:Username"]; + + using IServiceScope scope = serviceScopeFactory.CreateScope(); + IMediator mediator = scope.ServiceProvider.GetRequiredService(); + Either result = + await mediator.Send(new SeedLocalAdminFromEnvironment(username, password), cancellationToken); + + result.Match( + Right: _ => logger.LogWarning( + "Seeded the local administrator from Auth:LocalAdmin:* configuration (any existing credential " + + "was overwritten and all sessions revoked). Unset Auth:LocalAdmin:Password after signing in."), + Left: error => logger.LogError( + "Failed to seed the local administrator from configuration: {Error}", + error.Value)); + } + + public Task StopAsync(CancellationToken cancellationToken) => Task.CompletedTask; +} diff --git a/ErsatzTV/Startup.cs b/ErsatzTV/Startup.cs index ab2c76e8d..590818fb6 100644 --- a/ErsatzTV/Startup.cs +++ b/ErsatzTV/Startup.cs @@ -7,11 +7,15 @@ using System.Runtime.InteropServices; using System.Text; using System.Text.Json.Nodes; using System.Text.Json.Serialization; +using System.Security.Claims; using System.Threading.Channels; +using System.Threading.RateLimiting; using Dapper; using ErsatzTV.Application; +using ErsatzTV.Application.Auth; using ErsatzTV.Application.Channels; using ErsatzTV.Application.Streaming; +using ErsatzTV.Auth; using ErsatzTV.Core; using ErsatzTV.Core.Emby; using ErsatzTV.Core.Errors; @@ -81,6 +85,7 @@ using ErsatzTV.Services.RunOnce; using ErsatzTV.Services.Validators; using FluentValidation; using FluentValidation.AspNetCore; +using Microsoft.AspNetCore.Authentication; using Microsoft.AspNetCore.Authentication.Cookies; using Microsoft.AspNetCore.Authentication.JwtBearer; using Microsoft.AspNetCore.Authentication.OpenIdConnect; @@ -88,6 +93,7 @@ using Microsoft.AspNetCore.Authorization; using Microsoft.AspNetCore.DataProtection; using Microsoft.AspNetCore.HttpOverrides; using Microsoft.AspNetCore.Localization; +using Microsoft.AspNetCore.RateLimiting; using Microsoft.AspNetCore.StaticFiles; using Microsoft.EntityFrameworkCore; using Microsoft.Extensions.FileProviders; @@ -210,10 +216,16 @@ public class Startup if (!trustRestricted) { + // No trusted proxy configured → ignore X-Forwarded-* entirely rather than trust any peer + // (#295). Trusting spoofable forwarded headers would let a LAN peer forge the client IP the + // login rate limiter keys on (and the scheme the cookie Secure policy reads). Behind a proxy, + // set ForwardedHeaders:KnownProxies / :KnownNetworks to re-enable them (required for correct + // client IP + HTTPS detection). + options.ForwardedHeaders = ForwardedHeaders.None; Log.Warning( - "ForwardedHeaders trusts X-Forwarded-* from any peer (spoofable). Set " + - "ForwardedHeaders:KnownProxies and/or ForwardedHeaders:KnownNetworks to restrict " + - "trust to your reverse proxy when exposing ErsatzTV beyond a trusted LAN."); + "ForwardedHeaders trust is disabled (no ForwardedHeaders:KnownProxies/KnownNetworks " + + "configured); X-Forwarded-* headers are ignored. Set them to your reverse proxy when " + + "exposing ErsatzTV beyond a trusted LAN so client IP and HTTPS are detected correctly."); } }); @@ -277,64 +289,121 @@ public class Startup JwtHelper.Init(Configuration); SearchHelper.Init(Configuration); + // Browser SPA authentication (#295). A cookie session is ALWAYS registered — local username/password + // login and the OIDC callback both sign into it. OIDC is added only when configured; the /iptv JWT + // bearer is added only when configured. The /api surface accepts a session OR the machine X-Api-Key + // (see ApiAuthorizationFilter); real enforcement is that filter, not a DefaultPolicy. + AuthenticationBuilder authenticationBuilder = services.AddAuthentication(options => + { + options.DefaultScheme = AuthConstants.CookieScheme; + if (OidcHelper.IsEnabled) + { + options.DefaultChallengeScheme = AuthConstants.OidcScheme; + } + }) + .AddCookie( + AuthConstants.CookieScheme, + options => + { + options.CookieManager = new ChunkingCookieManager(); + + options.Cookie.Name = "ctv-session"; + options.Cookie.HttpOnly = true; + // Lax + no credentialed CORS keeps the cookie same-origin (the SPA is served from /app); + // this is a core CSRF defense alongside the required X-CSRF header on session mutations. + options.Cookie.SameSite = SameSiteMode.Lax; + // SameAsRequest (not Always) so a plain-HTTP LAN deployment is not locked out; behind a + // TLS-terminating proxy the app sees https once ForwardedHeaders:KnownProxies is set. + options.Cookie.SecurePolicy = CookieSecurePolicy.SameAsRequest; + + options.ExpireTimeSpan = TimeSpan.FromDays(14); + options.SlidingExpiration = true; + + options.Events = new CookieAuthenticationEvents + { + // /api is an XHR surface — answer 401/403 rather than redirecting to a login page. + OnRedirectToLogin = context => + { + if (context.Request.Path.StartsWithSegments("/api")) + { + context.Response.StatusCode = StatusCodes.Status401Unauthorized; + return Task.CompletedTask; + } + + context.Response.Redirect(context.RedirectUri); + return Task.CompletedTask; + }, + OnRedirectToAccessDenied = context => + { + if (context.Request.Path.StartsWithSegments("/api")) + { + context.Response.StatusCode = StatusCodes.Status403Forbidden; + return Task.CompletedTask; + } + + context.Response.Redirect(context.RedirectUri); + return Task.CompletedTask; + }, + OnValidatePrincipal = CookieSecurityStampValidator.ValidateAsync + }; + }); + if (OidcHelper.IsEnabled) { - services.AddAuthentication(options => + authenticationBuilder.AddOpenIdConnect( + AuthConstants.OidcScheme, + options => { - options.DefaultScheme = "cookie"; - options.DefaultChallengeScheme = "oidc"; - }) - .AddCookie( - "cookie", - options => + options.Authority = OidcHelper.Authority; + options.ClientId = OidcHelper.ClientId; + options.ClientSecret = OidcHelper.ClientSecret; + + options.ResponseType = OpenIdConnectResponseType.Code; + options.UsePkce = true; + options.ResponseMode = OpenIdConnectResponseMode.Query; + + options.Scope.Clear(); + options.Scope.Add("openid"); + options.Scope.Add("profile"); + options.GetClaimsFromUserInfoEndpoint = true; + + options.CallbackPath = new PathString("/callback"); + + options.SaveTokens = true; + + options.NonceCookie.SecurePolicy = CookieSecurePolicy.SameAsRequest; + options.CorrelationCookie.SecurePolicy = CookieSecurePolicy.SameAsRequest; + + options.Events = new OpenIdConnectEvents { - options.CookieManager = new ChunkingCookieManager(); - - options.Cookie.HttpOnly = true; - options.Cookie.SameSite = SameSiteMode.None; - options.Cookie.SecurePolicy = CookieSecurePolicy.Always; - }) - .AddOpenIdConnect( - "oidc", - options => - { - options.Authority = OidcHelper.Authority; - options.ClientId = OidcHelper.ClientId; - options.ClientSecret = OidcHelper.ClientSecret; - - options.ResponseType = OpenIdConnectResponseType.Code; - options.UsePkce = true; - options.ResponseMode = OpenIdConnectResponseMode.Query; - - options.Scope.Clear(); - options.Scope.Add("openid"); - - options.CallbackPath = new PathString("/callback"); - - options.SaveTokens = true; - - options.NonceCookie.SecurePolicy = CookieSecurePolicy.Always; - options.CorrelationCookie.SecurePolicy = CookieSecurePolicy.Always; - - if (!string.IsNullOrWhiteSpace(OidcHelper.LogoutUri)) + // Mark the session as OIDC so the cookie stamp validator skips it (the stamp is a + // local-login concept; OIDC sessions are governed by the IdP). + OnTokenValidated = context => { - options.Events = new OpenIdConnectEvents + if (context.Principal?.Identity is ClaimsIdentity identity) { - OnRedirectToIdentityProviderForSignOut = context => - { - context.Response.Redirect(OidcHelper.LogoutUri); - context.HandleResponse(); + identity.AddClaim(new Claim(AuthConstants.AuthMethodClaim, AuthConstants.MethodOidc)); + } - return Task.CompletedTask; - } - }; + return Task.CompletedTask; + }, + OnRedirectToIdentityProviderForSignOut = context => + { + if (!string.IsNullOrWhiteSpace(OidcHelper.LogoutUri)) + { + context.Response.Redirect(OidcHelper.LogoutUri); + context.HandleResponse(); + } + + return Task.CompletedTask; } - }); + }; + }); } if (JwtHelper.IsEnabled) { - services.AddAuthentication().AddJwtBearer( + authenticationBuilder.AddJwtBearer( "jwt", options => { @@ -362,34 +431,38 @@ public class Startup }); } - if (OidcHelper.IsEnabled || JwtHelper.IsEnabled) + // Authorization is always registered now that the pipeline always runs UseAuthorization (the cookie + // scheme is always present). No DefaultPolicy: /api is gated by ApiAuthorizationFilter, and no + // endpoint carries [Authorize]. The JWT-only policy stays for /iptv's ConditionalIptvAuthorizeFilter. + services.AddAuthorization(options => { - services.AddAuthorization(options => - { - if (OidcHelper.IsEnabled) + if (JwtHelper.IsEnabled) + { + options.AddPolicy( + "JwtOnlyScheme", + new AuthorizationPolicyBuilder("jwt") + .RequireAuthenticatedUser() + .Build()); + } + }); + + // Per-IP rate limit for the unauthenticated auth surface (login/setup/password) — blunts local + // password brute-force. Keyed on the connection remote IP (accurate only when + // ForwardedHeaders:KnownProxies is configured behind a proxy — see UseForwardedHeaders below). + services.AddRateLimiter(options => + { + options.RejectionStatusCode = StatusCodes.Status429TooManyRequests; + options.AddPolicy( + "auth", + httpContext => RateLimitPartition.GetFixedWindowLimiter( + httpContext.Connection.RemoteIpAddress?.ToString() ?? "unknown", + _ => new FixedWindowRateLimiterOptions { - var defaultAuthorizationPolicyBuilder = new AuthorizationPolicyBuilder( - "cookie", - "oidc"); - - defaultAuthorizationPolicyBuilder = - defaultAuthorizationPolicyBuilder.RequireAuthenticatedUser(); - - options.DefaultPolicy = defaultAuthorizationPolicyBuilder.Build(); - } - - if (JwtHelper.IsEnabled) - { - var onlyJwtSchemePolicyBuilder = new AuthorizationPolicyBuilder("jwt"); - options.AddPolicy( - "JwtOnlyScheme", - onlyJwtSchemePolicyBuilder - .RequireAuthenticatedUser() - .Build()); - } - } - ); - } + PermitLimit = 10, + Window = TimeSpan.FromMinutes(5), + QueueLimit = 0 + })); + }); services.AddCors(o => o.AddPolicy( "ApiCors", @@ -406,9 +479,12 @@ public class Startup "Accept", "Content-Type", "Authorization", - ApiKeyAuthorizationFilter.HeaderName, + ApiAuthorizationFilter.HeaderName, + ApiAuthorizationFilter.CsrfHeaderName, "If-Match") .WithExposedHeaders("ETag"); + // Note: AllowCredentials is deliberately NOT set — cross-origin cookie auth is impossible + // by design (a CSRF defense). Cross-origin clients authenticate with X-Api-Key. } // No configured origins => no cross-origin access (the SPA is same-origin from /app). @@ -423,7 +499,7 @@ public class Startup options.OutputFormatters.Insert(0, new ChannelGuideOutputFormatter()); options.OutputFormatters.Insert(0, new DeviceXmlOutputFormatter()); options.OutputFormatters.Insert(0, new HdhrJsonOutputFormatter()); - options.Filters.AddService(); + options.Filters.AddService(); }) .AddNewtonsoftJson(opt => { @@ -438,7 +514,10 @@ public class Startup // API-key authorization for the JSON API (independent of JWT/OIDC). The provider resolves the // effective key once (config, else persisted, else generated) so writes are fail-closed. services.AddSingleton(); - services.AddScoped(); + services.AddScoped(); + + // Local-admin password hashing (browser SPA session auth, #295). Stateless → singleton. + services.AddSingleton(); services.AddFluentValidationAutoValidation(); services.AddValidatorsFromAssemblyContaining(); @@ -820,12 +899,16 @@ public class Startup legacy.UseRouting(); - // Blazor removal (#91 phase b, #206): the OIDC challenge's only attachment point - // was the now-deleted Razor Pages / Blazor UI (AuthorizeFolder("/") + the Blazor - // hub). The OIDC/JWT *service* wiring stays registered (inert unless configured); - // real SPA/API auth is #197. /iptv keeps its own ConditionalIptvAuthorizeFilter - // and mutating /api/* its ApiKeyAuthorizationFilter — both independent of this - // middleware. + // Browser SPA / API authentication (#295). This branch hosts /api, the OIDC /callback, and + // /docs. UseAuthentication populates HttpContext.User from the cookie (default scheme) — the + // credential ApiAuthorizationFilter accepts alongside the machine X-Api-Key — and lets the + // OIDC middleware intercept /callback. UseAuthorization is required for the middleware to run + // (no [Authorize] endpoints; /api is gated by ApiAuthorizationFilter, /iptv by its own + // ConditionalIptvAuthorizeFilter). UseRateLimiter enforces the "auth" per-IP policy on the + // login/setup/password endpoints. + legacy.UseAuthentication(); + legacy.UseAuthorization(); + legacy.UseRateLimiter(); legacy.UseEndpoints(endpoints => { @@ -1061,6 +1144,8 @@ public class Startup // run-once/blocking startup services services.AddHostedService(); services.AddHostedService(); + // Must run after DatabaseMigratorService so the ConfigElements table exists (#295 env seed). + services.AddHostedService(); services.AddHostedService(); services.AddHostedService(); services.AddHostedService(); diff --git a/docs/api-conventions.md b/docs/api-conventions.md index b8e301bc4..59d1f9b1f 100644 --- a/docs/api-conventions.md +++ b/docs/api-conventions.md @@ -512,38 +512,71 @@ for items that have no group, so the SPA can render an "ungrouped" bucket concept elsewhere, this is the established pattern to follow — but be aware it means `Id` is not a reliable real-entity id for those synthetic rows. -## 9. Authentication — API key posture (fail-closed) +## 9. Authentication — session-or-key posture (fail-closed) -The whole `/api` surface is gated by the `X-Api-Key` header via the global `ApiKeyAuthorizationFilter` -(issue #197 Bundle A). When you add an endpoint: +The whole `/api` surface is gated by the global `ApiAuthorizationFilter` (renamed from +`ApiKeyAuthorizationFilter` in #295). A request that requires authentication is accepted on **either** +credential: -- **Do nothing** for the common case. Writes (POST/PUT/PATCH/DELETE) always require the key - (fail-closed — there is no "open" mode). Reads (GET/HEAD) require the key when - `Api:RequireKeyForReads` is enabled, which is the **default** (`true`). `OPTIONS` preflight is exempt. -- The effective key comes from `IApiKeyProvider` (`ErsatzTV/Services/ApiKeyProvider.cs`): `Api:WriteKey` - if configured, else a key persisted at `FileSystemLayout.ApiKeyPath` (`/config/api.key`, `0600`), else - a freshly generated 256-bit key. It is never empty. -- **Sensitive-read GETs** that disclose secrets/paths or trigger work must carry `[RequiresApiKey]` so - they stay gated even if an operator sets `Api:RequireKeyForReads=false`. Current tier: - `Troubleshoot`/`Logs`/`Settings`/`Maintenance`. `ApiControllerSecurityTests` asserts this reflectively. -- **Internal loopback callbacks** (the scanner's `/api/scan/*`) use `[SkipApiKeyAuthorization]` + - `[LocalhostOnly]` — the API key is a poor fit for a co-located child process, so the gate is the - loopback check (sound only because `ForwardedHeaders` trust is restricted via - `ForwardedHeaders:KnownProxies`/`KnownNetworks`). +1. a matching machine **`X-Api-Key`** header (MCP / external clients — issue #197 Bundle A), or +2. an **authenticated session** principal (browser cookie `ctv-session`, from local login or OIDC — #295). + +**Which endpoints require authentication is unchanged** and still decided by the single shared predicate +`ApiAuthorizationFilter.EndpointRequiresKey(httpMethod, endpointMetadata, requireKeyForReads)` (also used by +OpenAPI generation, so the spec can't drift). When you add an endpoint: + +- **Do nothing** for the common case. Writes (POST/PUT/PATCH/DELETE) always require a credential + (fail-closed — there is no "open" mode). Reads (GET/HEAD) require one when `Api:RequireKeyForReads` is + enabled, the **default** (`true`). `OPTIONS` preflight is exempt. +- The effective machine key comes from `IApiKeyProvider` (`ErsatzTV/Services/ApiKeyProvider.cs`): + `Api:WriteKey` if configured, else a key persisted at `FileSystemLayout.ApiKeyPath` (`/config/api.key`, + `0600`), else a freshly generated 256-bit key. It is never empty. +- **CSRF (session credential only).** The machine key is CSRF-immune (a browser can't set a custom header + cross-origin without a credentialed CORS grant, which is never issued). A cookie session is not: a + **session-authenticated mutation** must additionally carry the `X-CSRF` header (`ApiAuthorizationFilter.CsrfHeaderName`) + or it is rejected **403** — presence is the whole check (a custom header forces a CORS preflight a + cross-site page can't satisfy; reinforced by `SameSite=Lax` + CORS without `AllowCredentials`). Key-authed + requests are exempt. When you add a SPA mutation, send `X-CSRF: 1` (the SPA client does this centrally). +- **Sensitive-read GETs** that disclose secrets/paths or trigger work must carry `[RequiresAuthentication]` + (renamed from `[RequiresApiKey]`) so they stay gated even if an operator sets `Api:RequireKeyForReads=false`. + A valid session satisfies this tier just as the key does. Current tier: `Troubleshoot`/`Logs`/`Settings`/ + `Maintenance`. `ApiControllerSecurityTests` asserts this reflectively. +- **Internal loopback callbacks** (the scanner's `/api/scan/*`) and the **`/api/auth/*` surface itself** use + `[SkipApiAuthorization]` (renamed from `[SkipApiKeyAuthorization]`). The scanner adds `[LocalhostOnly]`; the + auth surface must be reachable before a caller is authenticated, and its one sensitive action + (`POST /api/auth/password`) self-checks the principal. `ApiControllerSecurityTests` asserts these two are the + **only** auth-exempt controllers. +- **The `/api/auth/*` surface** (`AuthController`, `[ApiExplorerSettings(IgnoreApi = true)]` → excluded from the + OpenAPI doc, whose audience is machine clients): `GET config` (what auth options exist + `setupRequired`), + `GET session`, `POST setup` (first-run claim), `POST login`, `POST logout`, `POST password`. The browser-nav + OIDC challenge is `GET /auth/oidc/login` (outside `/api`). `login`/`setup`/`password` carry a per-IP rate + limit (`[EnableRateLimiting("auth")]`, 10 / 5 min). The local admin is a single credential in `ConfigElement` + rows (`AuthLocalAdminUsername`/`AuthLocalAdminPasswordHash` (PBKDF2) / `AuthSecurityStamp`) — **no DB + migration**; a password change rotates the stamp, revoking sessions via the cookie `OnValidatePrincipal`. + Recovery/bootstrap without the browser: set `Auth:LocalAdmin:Password` (+ optional `…:Username`, default + `admin`) and restart (`LocalAdminSeedService` reseeds + rotates the stamp). - **CORS** is opt-in: no cross-origin access by default (the SPA is same-origin from `/app`); set - `Api:CorsAllowedOrigins` (semicolon-separated exact origins) to allow specific browser origins — the - policy already permits `X-Api-Key`/`If-Match` and exposes `ETag`. -- The SPA sends the stored key (`ctv-api-key`) on **every** request; users enter it on the keyless - **API Key** screen (`web/src/screens/ApiKeyScreen.tsx`, route `/app/api-key`). See spa-conventions §5e. + `Api:CorsAllowedOrigins` (semicolon-separated exact origins). `AllowCredentials` is deliberately **not** + set — cross-origin cookie auth is impossible by design (a CSRF defense); cross-origin machine clients use + `X-Api-Key`. The policy permits `X-Api-Key`/`X-CSRF`/`If-Match` and exposes `ETag`. +- **ForwardedHeaders is strict opt-in (#295).** With no `ForwardedHeaders:KnownProxies`/`:KnownNetworks` + configured, `X-Forwarded-*` are **ignored** (not trusted from any peer) — otherwise a LAN peer could forge + the client IP the login rate limiter keys on / the scheme the cookie `Secure` policy reads. Behind a reverse + proxy you **must** set `KnownProxies`/`KnownNetworks` for correct client-IP + HTTPS detection. +- **PR1 scope note (this change is server-only).** The SPA still authenticates with the stored key + (`ctv-api-key`, `X-Api-Key`) until the SPA login flow lands (PR2) — PR1 is backward compatible. The + `ApiKeyScreen` → machine-key-management repurpose, the SPA login/setup screens, and `spa-conventions §5e` + update all land in PR2. -**The OpenAPI "v1" document now declares this posture by construction (#287).** An `ApiKey` security +**The OpenAPI "v1" document declares the machine posture by construction (#287).** An `ApiKey` security scheme (`X-Api-Key`, `in: header`) is declared in `components.securitySchemes`, and `ApiSecurityOperationTransformer` injects a per-operation `security` requirement + a documented `401` -for exactly the operations that require the key — using the **same** shared predicate -`ApiKeyAuthorizationFilter.EndpointRequiresKey(httpMethod, endpointMetadata, requireKeyForReads)` that -the runtime filter enforces, so the spec can never drift from enforcement. The document is generated -against the effective default (`Api:RequireKeyForReads=true`), under which every documented operation -requires the key. Two companion transformers run on the "v1" document only: +for exactly the operations that require a credential — using the **same** shared predicate +`ApiAuthorizationFilter.EndpointRequiresKey(...)` the runtime filter enforces, so the spec can never drift from +enforcement. The document is generated against the effective default (`Api:RequireKeyForReads=true`), under +which every documented operation requires the credential; the browser-session path is an additional accepted +credential the spec (machine audience) needn't express. Two companion transformers run on the "v1" document +only: `OperationIdOpenApiTransformer` synthesizes a stable `operationId` (from controller+action) for the ~90 operations that lacked a `Name=` — and disambiguates the HEAD/GET pairs that share a controller+action **structurally, independent of ApiExplorer visitation order** (#197 Bundle C): when 2+ synthesized ops diff --git a/docs/decisions.md b/docs/decisions.md index b2f8d8b0a..f74eee93c 100644 --- a/docs/decisions.md +++ b/docs/decisions.md @@ -1082,3 +1082,67 @@ so the immutable int PK is the canonical key for all `/api/channels/*` single-it headers. `Number` remains the identity on broadcast surfaces only (IPTV/M3U/XMLTV), a separate contract. A number-based lookup endpoint may be added additively later; `UniqueId` (Guid) stays out of the REST contract absent a federation requirement. + +## 2026-07-12 — Browser SPA session auth: `/api` accepts session OR machine key (#295 PR1, server-only) + +Implements the ratified #295 design (Fable [PLAN-MODE] pass, issue comment 9548). Supersedes the #206 +"OIDC wiring stays inert until #197" note: the retained OIDC service registration is now **revived**, and a +cookie session becomes a first-class `/api` credential alongside the machine `X-Api-Key`. **PR1 is +server-only and backward compatible** — the SPA keeps sending its stored key; the SPA login flow, the +`ApiKeyScreen`→machine-key repurpose, and `spa-conventions §5e` land in **PR2**. + +**One gate, evolved (not `[Authorize]`-per-controller).** `ApiKeyAuthorizationFilter` → `ApiAuthorizationFilter`, +same fail-closed-by-omission logic (a forgotten `[Authorize]` fails *open* — the #280 failure mode — so the +global filter stays the gate). It now accepts a request when a valid `X-Api-Key` matches **OR** the principal +is an authenticated session; the "does this endpoint need auth?" decision is still the single shared +`EndpointRequiresKey(...)` predicate (also drives OpenAPI, so the spec can't drift). Attributes renamed to +match the widened meaning: `[RequiresApiKey]`→`[RequiresAuthentication]`, `[SkipApiKeyAuthorization]`→ +`[SkipApiAuthorization]`. `IApiKeyProvider`, the `X-Api-Key` header, and `Api:WriteKey`/`Api:RequireKeyForReads` +are unchanged — **machine/key behavior is byte-identical** (verified: no OpenAPI drift, existing filter tests +still green). + +**CSRF (session only).** The machine key is CSRF-immune (a browser can't set a custom header cross-origin +without a credentialed CORS grant we never issue). A cookie session is not: a session-authenticated **mutation** +must carry the `X-CSRF` header (presence-only — a custom header forces a CORS preflight a cross-site page can't +satisfy) or is rejected **403**. Reinforced by `SameSite=Lax` + CORS without `AllowCredentials` (cross-origin +cookie auth is impossible by design). No antiforgery-token machinery. + +**Cookie `ctv-session`.** Always registered (local login works with no IdP); OIDC handler added only when +`OIDC:*` is configured. `HttpOnly`, `SameSite=Lax`, `SecurePolicy=SameAsRequest` (so a plain-HTTP LAN isn't +bricked), 14-day sliding. `/api` XHR gets **401/403, not a redirect** (`OnRedirectToLogin`/`AccessDenied`). +The `UseAuthentication`/`UseAuthorization` middleware — deleted with Blazor in #91b — is **revived in the +`legacy` `MapWhen` branch only** (hosts `/api` + OIDC `/callback` + `/docs`; `/iptv` and `/app` untouched). + +**Local store = `ConfigElement` rows, single admin, NO migration** (owner ruling F2): +`AuthLocalAdminUsername`, `AuthLocalAdminPasswordHash` (ASP.NET `PasswordHasher`, PBKDF2, via +`Microsoft.Extensions.Identity.Core`), `AuthSecurityStamp`. A password change rotates the stamp; the cookie +`OnValidatePrincipal` (`CookieSecurityStampValidator`) compares the claim to the stored stamp and rejects a +stale session (revocation). OIDC sessions carry an `etv:auth_method=oidc` claim and skip the stamp check +(governed by the IdP). + +**Fail-closed out of the box + recovery.** An unconfigured instance keeps `/api` gated (the key still works); +first-run is a **setup-claim** (`POST /api/auth/setup`, first-claim-wins, only valid while unconfigured — +owner ruling F1). Recovery without the browser: `Auth:LocalAdmin:Password` env seed (`LocalAdminSeedService`, +overwrites + rotates the stamp on startup) or the machine key. Login hardening: per-IP rate limit +(`[EnableRateLimiting("auth")]`, 10 / 5 min) on login/setup/password, dummy-hash verify on unknown/unconfigured +user (no enumeration). + +**Authelia = app-owned OIDC session; never trust proxy identity headers** (owner ruling F3): the container is +LAN-reachable bypassing the proxy, so `Remote-User`/`Remote-Email` header trust is spoofable. OIDC→Authelia +gives SSO without a double login. Relatedly, **`ForwardedHeaders` is now strict opt-in**: with no +`KnownProxies`/`KnownNetworks` configured, `X-Forwarded-*` are **ignored** (was: trusted-from-any-peer with a +warning, #285) — otherwise a LAN peer could forge the client IP the rate limiter keys on or the scheme the +cookie `Secure` policy reads. **Deployment coordination:** operators behind a proxy must set +`ForwardedHeaders:KnownProxies`/`:KnownNetworks` (part of the #295 rollout checklist). This only cosmetically +affects the current key-only prod (logs the proxy IP); the host/port guard keys on the `Host` header + +`LocalPort`, not the remote IP. + +**OpenAPI = `ApiKey`-only; `/api/auth/*` excluded** (owner ruling F4): the spec's audience is machine/MCP +clients, and a browser-interactive cookie login isn't something a generated client drives, so the cookie path +is an additional accepted credential the doc needn't express. `AuthController` is `[ApiExplorerSettings(IgnoreApi += true)]`. Verified: no `v1.json`/`v1.d.ts`/`endpoint-index` drift from this PR. + +**Phasing.** PR1 = this (server only, no migration). PR2 = SPA (drop the key header for browser calls + add +`X-CSRF`, `AuthContext` + boot gate, login/setup screens, `ApiKeyScreen`→machine-key management, E2E, +`spa-conventions §5e`). PR3 = key rotation + operator docs (Authelia client + env reference). Rollout: PR1→PR2 +same release, then a manual Authelia round-trip checklist before the prod pin bump. From 6ac5150fd0b955d5afe1ece69cac4903d769ef0e Mon Sep 17 00:00:00 2001 From: Timothy Date: Sun, 12 Jul 2026 16:40:25 +0200 Subject: [PATCH 2/5] =?UTF-8?q?fix(api):=20#295=20PR1=20=E2=80=94=20fold?= =?UTF-8?q?=20in=20cold-fork=20+=20Codex=20review=20findings?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Independent review (cold fork = MERGEABLE-WITH-NITS; Codex = BLOCKED, caught concurrency defects the fork missed). All actionable findings folded in: - HIGH (Codex) atomic first-claim-wins: ClaimLocalAdmin now writes the three credential rows in ONE transaction guarded by the unique ConfigElement.Key index (lost race -> DbUpdateException -> 409), so concurrent claims can't produce a mixed-state credential. - HIGH (Codex) consistent login snapshot: VerifyLocalAdminLogin reads hash+stamp in one query and drops rehash-on-verify, so a login racing a password change can't capture a stamp newer than the hash it verified (concurrent change -> old password fails, or the issued cookie carries the pre-change stamp -> revoked next request). - MEDIUM (Codex) env-seed migration race: LocalAdminSeedService is now a RunOnce BackgroundService that awaits SystemStartup.WaitForDatabase (the migrator is a BackgroundService; registration order didn't guarantee the schema) + try/catch. - MEDIUM (fork M1) ForwardedHeaders: reverted the strict-opt-in flip — it would regress /iptv M3U/XMLTV/HLS absolute-URL generation (Request.Scheme) behind a proxy without KnownProxies. Kept #285 behavior; KnownProxies still recommended. - LOW (Codex/fork) require X-CSRF on /api/auth/logout + /password (the [SkipApiAuthorization] surface isn't covered by the filter's CSRF check; closes forced-logout CSRF). - ChangeLocalAdminPassword also writes hash+stamp atomically. Input length caps on username/password. Deferred with a tracked gate: MEDIUM (Codex) side-effecting [RequiresAuthentication] GETs (troubleshoot playback/archive) aren't CSRF-covered -> #301, gates PR2 (latent in PR1: the SPA still uses the machine key). Verify: full ErsatzTV.Tests green (1501); no OpenAPI/generated drift. Docs updated (api-conventions §9, decisions.md). Refs #295 #301 Co-Authored-By: Claude Opus 4.8 (1M context) --- .../Auth/ChangeLocalAdminPasswordHandler.cs | 53 ++++++++++------ .../Auth/ClaimLocalAdminHandler.cs | 56 +++++++++-------- .../Auth/LocalAdminHelpers.cs | 39 ++++++++++++ .../SeedLocalAdminFromEnvironmentHandler.cs | 48 +++++++++++---- .../Auth/VerifyLocalAdminLoginHandler.cs | 50 ++++++++-------- .../ChangeLocalAdminPasswordHandlerTests.cs | 4 +- .../Auth/ClaimLocalAdminHandlerTests.cs | 17 +++++- ...edLocalAdminFromEnvironmentHandlerTests.cs | 21 ++++++- .../Auth/VerifyLocalAdminLoginHandlerTests.cs | 4 +- ErsatzTV/Controllers/Api/AuthController.cs | 38 +++++++++++- ErsatzTV/Services/LocalAdminSeedService.cs | 44 -------------- .../Services/RunOnce/LocalAdminSeedService.cs | 60 +++++++++++++++++++ ErsatzTV/Startup.cs | 22 +++---- docs/api-conventions.md | 16 +++-- docs/decisions.md | 31 +++++++--- 15 files changed, 352 insertions(+), 151 deletions(-) delete mode 100644 ErsatzTV/Services/LocalAdminSeedService.cs create mode 100644 ErsatzTV/Services/RunOnce/LocalAdminSeedService.cs diff --git a/ErsatzTV.Application/Auth/ChangeLocalAdminPasswordHandler.cs b/ErsatzTV.Application/Auth/ChangeLocalAdminPasswordHandler.cs index a1ccab28d..fe96c5472 100644 --- a/ErsatzTV.Application/Auth/ChangeLocalAdminPasswordHandler.cs +++ b/ErsatzTV.Application/Auth/ChangeLocalAdminPasswordHandler.cs @@ -1,11 +1,12 @@ using ErsatzTV.Core; using ErsatzTV.Core.Domain; -using ErsatzTV.Core.Interfaces.Repositories; +using ErsatzTV.Infrastructure.Data; +using Microsoft.EntityFrameworkCore; namespace ErsatzTV.Application.Auth; public class ChangeLocalAdminPasswordHandler( - IConfigElementRepository configElementRepository, + IDbContextFactory dbContextFactory, ILocalPasswordHasher passwordHasher) : IRequestHandler> { @@ -13,41 +14,55 @@ public class ChangeLocalAdminPasswordHandler( ChangeLocalAdminPassword request, CancellationToken cancellationToken) { - if (string.IsNullOrEmpty(request.NewPassword) || request.NewPassword.Length < AuthConstants.MinPasswordLength) + foreach (BaseError error in LocalAdminHelpers.ValidatePassword(request.NewPassword)) { - return BaseError.New($"Password must be at least {AuthConstants.MinPasswordLength} characters"); + return error; } - Option storedUser = - await configElementRepository.GetValue(ConfigElementKey.AuthLocalAdminUsername, cancellationToken); - Option storedHash = - await configElementRepository.GetValue(ConfigElementKey.AuthLocalAdminPasswordHash, cancellationToken); + await using TvContext dbContext = await dbContextFactory.CreateDbContextAsync(cancellationToken); - if (storedHash.IsNone) + List rows = await dbContext.ConfigElements + .Where(c => c.Key == ConfigElementKey.AuthLocalAdminUsername.Key + || c.Key == ConfigElementKey.AuthLocalAdminPasswordHash.Key + || c.Key == ConfigElementKey.AuthSecurityStamp.Key) + .ToListAsync(cancellationToken); + + ConfigElement userRow = rows.Find(r => r.Key == ConfigElementKey.AuthLocalAdminUsername.Key); + ConfigElement hashRow = rows.Find(r => r.Key == ConfigElementKey.AuthLocalAdminPasswordHash.Key); + ConfigElement stampRow = rows.Find(r => r.Key == ConfigElementKey.AuthSecurityStamp.Key); + + if (hashRow is null) { return BaseError.New("No local administrator is configured"); } string username = (request.Username ?? string.Empty).Trim(); - bool userMatches = storedUser.Match( - Some: u => string.Equals(u, username, StringComparison.OrdinalIgnoreCase), - None: () => false); + bool userMatches = userRow is not null + && string.Equals(userRow.Value, username, StringComparison.OrdinalIgnoreCase); LocalPasswordVerification result = - passwordHasher.Verify(storedHash.IfNone(passwordHasher.DummyHash), request.CurrentPassword ?? string.Empty); + passwordHasher.Verify(hashRow.Value, request.CurrentPassword ?? string.Empty); if (!userMatches || result == LocalPasswordVerification.Failed) { return BaseError.New("Current password is incorrect"); } + // Atomic: the new hash and rotated stamp commit together, so a crash can't leave the new password + // active with the old stamp still authorizing revoked sessions. string stamp = LocalAdminHelpers.NewSecurityStamp(); - await configElementRepository.Upsert( - ConfigElementKey.AuthLocalAdminPasswordHash, - passwordHasher.Hash(request.NewPassword), - cancellationToken); - await configElementRepository.Upsert(ConfigElementKey.AuthSecurityStamp, stamp, cancellationToken); + hashRow.Value = passwordHasher.Hash(request.NewPassword); + if (stampRow is null) + { + dbContext.ConfigElements.Add(new ConfigElement { Key = ConfigElementKey.AuthSecurityStamp.Key, Value = stamp }); + } + else + { + stampRow.Value = stamp; + } - return new LocalAdminPrincipal(storedUser.IfNone(username), stamp); + await dbContext.SaveChangesAsync(cancellationToken); + + return new LocalAdminPrincipal(userRow.Value, stamp); } } diff --git a/ErsatzTV.Application/Auth/ClaimLocalAdminHandler.cs b/ErsatzTV.Application/Auth/ClaimLocalAdminHandler.cs index 8e604e6dc..ba43c0cc1 100644 --- a/ErsatzTV.Application/Auth/ClaimLocalAdminHandler.cs +++ b/ErsatzTV.Application/Auth/ClaimLocalAdminHandler.cs @@ -1,48 +1,56 @@ using ErsatzTV.Core; using ErsatzTV.Core.Domain; -using ErsatzTV.Core.Interfaces.Repositories; +using ErsatzTV.Infrastructure.Data; +using Microsoft.EntityFrameworkCore; namespace ErsatzTV.Application.Auth; -public class ClaimLocalAdminHandler(IConfigElementRepository configElementRepository, ILocalPasswordHasher passwordHasher) +public class ClaimLocalAdminHandler(IDbContextFactory dbContextFactory, ILocalPasswordHasher passwordHasher) : IRequestHandler> { public async Task> Handle( ClaimLocalAdmin request, CancellationToken cancellationToken) { - string username = (request.Username ?? string.Empty).Trim(); - if (username.Length == 0) + foreach (BaseError error in LocalAdminHelpers.ValidateNewCredentials(request.Username, request.Password)) { - return BaseError.New("Username is required"); + return error; } - if (username.Length > 256) - { - return BaseError.New("Username is too long"); - } + string username = request.Username.Trim(); - if (string.IsNullOrEmpty(request.Password) || request.Password.Length < AuthConstants.MinPasswordLength) - { - return BaseError.New($"Password must be at least {AuthConstants.MinPasswordLength} characters"); - } + await using TvContext dbContext = await dbContextFactory.CreateDbContextAsync(cancellationToken); - // First-claim-wins: refuse if an admin already exists. (The tiny check-then-write race is accepted - // per the design; /api stays closed regardless of who wins.) - Option existing = - await configElementRepository.GetConfigElement(ConfigElementKey.AuthLocalAdminPasswordHash, cancellationToken); - if (existing.IsSome) + // Fast path for the common already-configured case (clean 409). The real first-claim-wins guard is + // the unique index on ConfigElement.Key + the single atomic SaveChanges below: two concurrent claims + // both pass this check, but only one INSERT of the three credential rows commits — the loser's + // SaveChanges violates the unique Key index and rolls back wholesale (no mixed-state credential). + bool alreadyConfigured = await dbContext.ConfigElements + .AnyAsync(c => c.Key == ConfigElementKey.AuthLocalAdminPasswordHash.Key, cancellationToken); + if (alreadyConfigured) { return BaseError.New("A local administrator has already been configured"); } string stamp = LocalAdminHelpers.NewSecurityStamp(); - await configElementRepository.Upsert(ConfigElementKey.AuthLocalAdminUsername, username, cancellationToken); - await configElementRepository.Upsert( - ConfigElementKey.AuthLocalAdminPasswordHash, - passwordHasher.Hash(request.Password), - cancellationToken); - await configElementRepository.Upsert(ConfigElementKey.AuthSecurityStamp, stamp, cancellationToken); + dbContext.ConfigElements.AddRange( + new ConfigElement { Key = ConfigElementKey.AuthLocalAdminUsername.Key, Value = username }, + new ConfigElement + { + Key = ConfigElementKey.AuthLocalAdminPasswordHash.Key, + Value = passwordHasher.Hash(request.Password) + }, + new ConfigElement { Key = ConfigElementKey.AuthSecurityStamp.Key, Value = stamp }); + + try + { + await dbContext.SaveChangesAsync(cancellationToken); + } + catch (DbUpdateException) + { + // Lost the first-claim race — a concurrent claim inserted these keys first (unique Key index). + return BaseError.New("A local administrator has already been configured"); + } return new LocalAdminPrincipal(username, stamp); } diff --git a/ErsatzTV.Application/Auth/LocalAdminHelpers.cs b/ErsatzTV.Application/Auth/LocalAdminHelpers.cs index e4c905b9b..2cf753c96 100644 --- a/ErsatzTV.Application/Auth/LocalAdminHelpers.cs +++ b/ErsatzTV.Application/Auth/LocalAdminHelpers.cs @@ -1,10 +1,49 @@ using System.Security.Cryptography; +using ErsatzTV.Core; namespace ErsatzTV.Application.Auth; internal static class LocalAdminHelpers { + public const int MaxUsernameLength = 256; + + // Upper bound so an absurdly long password can't burn CPU in PBKDF2 (the request body is also capped + // by Kestrel, #283; this is defense-in-depth on the field itself). + public const int MaxPasswordLength = 1024; + /// 128 bits of random, lowercase hex. Rotated on every password change to revoke sessions. public static string NewSecurityStamp() => Convert.ToHexString(RandomNumberGenerator.GetBytes(16)).ToLowerInvariant(); + + /// Validates a new username + password. Returns the error, or None if valid. + public static Option ValidateNewCredentials(string username, string password) + { + string trimmed = (username ?? string.Empty).Trim(); + if (trimmed.Length == 0) + { + return BaseError.New("Username is required"); + } + + if (trimmed.Length > MaxUsernameLength) + { + return BaseError.New("Username is too long"); + } + + return ValidatePassword(password); + } + + public static Option ValidatePassword(string password) + { + if (string.IsNullOrEmpty(password) || password.Length < AuthConstants.MinPasswordLength) + { + return BaseError.New($"Password must be at least {AuthConstants.MinPasswordLength} characters"); + } + + if (password.Length > MaxPasswordLength) + { + return BaseError.New($"Password must be at most {MaxPasswordLength} characters"); + } + + return Option.None; + } } diff --git a/ErsatzTV.Application/Auth/SeedLocalAdminFromEnvironmentHandler.cs b/ErsatzTV.Application/Auth/SeedLocalAdminFromEnvironmentHandler.cs index cbc061c21..3d793cebe 100644 --- a/ErsatzTV.Application/Auth/SeedLocalAdminFromEnvironmentHandler.cs +++ b/ErsatzTV.Application/Auth/SeedLocalAdminFromEnvironmentHandler.cs @@ -1,11 +1,12 @@ using ErsatzTV.Core; using ErsatzTV.Core.Domain; -using ErsatzTV.Core.Interfaces.Repositories; +using ErsatzTV.Infrastructure.Data; +using Microsoft.EntityFrameworkCore; namespace ErsatzTV.Application.Auth; public class SeedLocalAdminFromEnvironmentHandler( - IConfigElementRepository configElementRepository, + IDbContextFactory dbContextFactory, ILocalPasswordHasher passwordHasher) : IRequestHandler> { @@ -19,19 +20,44 @@ public class SeedLocalAdminFromEnvironmentHandler( username = "admin"; } - if (string.IsNullOrEmpty(request.Password) || request.Password.Length < AuthConstants.MinPasswordLength) + if (username.Length > LocalAdminHelpers.MaxUsernameLength) { - return BaseError.New($"Seed password must be at least {AuthConstants.MinPasswordLength} characters"); + return BaseError.New("Seed username is too long"); } - string stamp = LocalAdminHelpers.NewSecurityStamp(); - await configElementRepository.Upsert(ConfigElementKey.AuthLocalAdminUsername, username, cancellationToken); - await configElementRepository.Upsert( - ConfigElementKey.AuthLocalAdminPasswordHash, - passwordHasher.Hash(request.Password), - cancellationToken); - await configElementRepository.Upsert(ConfigElementKey.AuthSecurityStamp, stamp, cancellationToken); + foreach (BaseError error in LocalAdminHelpers.ValidatePassword(request.Password)) + { + return error; + } + + await using TvContext dbContext = await dbContextFactory.CreateDbContextAsync(cancellationToken); + + List rows = await dbContext.ConfigElements + .Where(c => c.Key == ConfigElementKey.AuthLocalAdminUsername.Key + || c.Key == ConfigElementKey.AuthLocalAdminPasswordHash.Key + || c.Key == ConfigElementKey.AuthSecurityStamp.Key) + .ToListAsync(cancellationToken); + + // Overwrite (recovery/bootstrap) atomically: username + new hash + rotated stamp commit together. + Upsert(dbContext, rows, ConfigElementKey.AuthLocalAdminUsername.Key, username); + Upsert(dbContext, rows, ConfigElementKey.AuthLocalAdminPasswordHash.Key, passwordHasher.Hash(request.Password)); + Upsert(dbContext, rows, ConfigElementKey.AuthSecurityStamp.Key, LocalAdminHelpers.NewSecurityStamp()); + + await dbContext.SaveChangesAsync(cancellationToken); return Unit.Default; } + + private static void Upsert(TvContext dbContext, List existing, string key, string value) + { + ConfigElement row = existing.Find(r => r.Key == key); + if (row is null) + { + dbContext.ConfigElements.Add(new ConfigElement { Key = key, Value = value }); + } + else + { + row.Value = value; + } + } } diff --git a/ErsatzTV.Application/Auth/VerifyLocalAdminLoginHandler.cs b/ErsatzTV.Application/Auth/VerifyLocalAdminLoginHandler.cs index 4dd0b06b3..e1d5ecadb 100644 --- a/ErsatzTV.Application/Auth/VerifyLocalAdminLoginHandler.cs +++ b/ErsatzTV.Application/Auth/VerifyLocalAdminLoginHandler.cs @@ -1,11 +1,12 @@ using ErsatzTV.Core; using ErsatzTV.Core.Domain; -using ErsatzTV.Core.Interfaces.Repositories; +using ErsatzTV.Infrastructure.Data; +using Microsoft.EntityFrameworkCore; namespace ErsatzTV.Application.Auth; public class VerifyLocalAdminLoginHandler( - IConfigElementRepository configElementRepository, + IDbContextFactory dbContextFactory, ILocalPasswordHasher passwordHasher) : IRequestHandler> { @@ -16,36 +17,37 @@ public class VerifyLocalAdminLoginHandler( CancellationToken cancellationToken) { string username = (request.Username ?? string.Empty).Trim(); - Option storedUser = - await configElementRepository.GetValue(ConfigElementKey.AuthLocalAdminUsername, cancellationToken); - Option storedHash = - await configElementRepository.GetValue(ConfigElementKey.AuthLocalAdminPasswordHash, cancellationToken); - // Always run exactly one verify — against a dummy hash when unconfigured/unknown — so response + await using TvContext dbContext = await dbContextFactory.CreateDbContextAsync(cancellationToken); + + // Read the hash and stamp in ONE snapshot so they are consistent (issue: a login racing a password + // change must not return a stamp newer than the hash it verified). A concurrent change is then either + // wholly before this read (the old password fails to verify) or wholly after it (we return the + // pre-change stamp, so the cookie AuthController issues is revoked on its very next request by + // CookieSecurityStampValidator). No writes happen here, so there is nothing to clobber. + Dictionary config = await dbContext.ConfigElements + .Where(c => c.Key == ConfigElementKey.AuthLocalAdminUsername.Key + || c.Key == ConfigElementKey.AuthLocalAdminPasswordHash.Key + || c.Key == ConfigElementKey.AuthSecurityStamp.Key) + .ToDictionaryAsync(c => c.Key, c => c.Value, cancellationToken); + + config.TryGetValue(ConfigElementKey.AuthLocalAdminUsername.Key, out string storedUser); + config.TryGetValue(ConfigElementKey.AuthLocalAdminPasswordHash.Key, out string storedHash); + config.TryGetValue(ConfigElementKey.AuthSecurityStamp.Key, out string stamp); + + // Always run exactly one PBKDF2 verify — against a dummy hash when unconfigured/unknown — so response // timing does not reveal whether the account exists (no user enumeration). - string candidateHash = storedHash.IfNone(passwordHasher.DummyHash); + string candidateHash = storedHash ?? passwordHasher.DummyHash; LocalPasswordVerification result = passwordHasher.Verify(candidateHash, request.Password ?? string.Empty); - bool userMatches = storedUser.Match( - Some: u => string.Equals(u, username, StringComparison.OrdinalIgnoreCase), - None: () => false); + bool userMatches = storedUser is not null + && string.Equals(storedUser, username, StringComparison.OrdinalIgnoreCase); - if (storedHash.IsNone || !userMatches || result == LocalPasswordVerification.Failed) + if (storedHash is null || !userMatches || result == LocalPasswordVerification.Failed) { return InvalidCredentials; } - // Transparent upgrade if Identity's work factor was bumped since this hash was written. - if (result == LocalPasswordVerification.SuccessRehashNeeded) - { - await configElementRepository.Upsert( - ConfigElementKey.AuthLocalAdminPasswordHash, - passwordHasher.Hash(request.Password!), - cancellationToken); - } - - Option stamp = - await configElementRepository.GetValue(ConfigElementKey.AuthSecurityStamp, cancellationToken); - return new LocalAdminPrincipal(storedUser.IfNone(username), stamp.IfNone(string.Empty)); + return new LocalAdminPrincipal(storedUser, stamp ?? string.Empty); } } diff --git a/ErsatzTV.Tests/Application/Auth/ChangeLocalAdminPasswordHandlerTests.cs b/ErsatzTV.Tests/Application/Auth/ChangeLocalAdminPasswordHandlerTests.cs index 0e068516b..8f132cf4c 100644 --- a/ErsatzTV.Tests/Application/Auth/ChangeLocalAdminPasswordHandlerTests.cs +++ b/ErsatzTV.Tests/Application/Auth/ChangeLocalAdminPasswordHandlerTests.cs @@ -33,11 +33,11 @@ public class ChangeLocalAdminPasswordHandlerTests public async Task TearDown() => await _db.DisposeAsync(); private ChangeLocalAdminPasswordHandler MakeHandler() => - new(_configElementRepository, _passwordHasher); + new(_db.Factory, _passwordHasher); private async Task SeedAdmin() { - var claim = new ClaimLocalAdminHandler(_configElementRepository, _passwordHasher); + var claim = new ClaimLocalAdminHandler(_db.Factory, _passwordHasher); (await claim.Handle(new ClaimLocalAdmin(Username, CurrentPassword), CancellationToken.None)) .IsRight.ShouldBeTrue(); } diff --git a/ErsatzTV.Tests/Application/Auth/ClaimLocalAdminHandlerTests.cs b/ErsatzTV.Tests/Application/Auth/ClaimLocalAdminHandlerTests.cs index 2be7e14ce..10deab78f 100644 --- a/ErsatzTV.Tests/Application/Auth/ClaimLocalAdminHandlerTests.cs +++ b/ErsatzTV.Tests/Application/Auth/ClaimLocalAdminHandlerTests.cs @@ -29,7 +29,7 @@ public class ClaimLocalAdminHandlerTests public async Task TearDown() => await _db.DisposeAsync(); private ClaimLocalAdminHandler MakeHandler() => - new(_configElementRepository, _passwordHasher); + new(_db.Factory, _passwordHasher); [Test] public async Task Handle_Should_Claim_Fresh_Admin_And_Persist_All_Config_Elements() @@ -112,6 +112,21 @@ public class ClaimLocalAdminHandlerTests await AssertNothingPersisted(); } + [Test] + public async Task Handle_Should_Reject_Over_Long_Password_And_Persist_Nothing() + { + // LocalAdminHelpers.MaxPasswordLength (1024) is internal; use the documented bound directly. + string longPassword = new('a', 1024 + 1); + ClaimLocalAdminHandler handler = MakeHandler(); + + Either result = await handler.Handle( + new ClaimLocalAdmin("Operator", longPassword), + CancellationToken.None); + + result.IsLeft.ShouldBeTrue(); + await AssertNothingPersisted(); + } + private async Task AssertNothingPersisted() { (await _configElementRepository.GetConfigElement( diff --git a/ErsatzTV.Tests/Application/Auth/SeedLocalAdminFromEnvironmentHandlerTests.cs b/ErsatzTV.Tests/Application/Auth/SeedLocalAdminFromEnvironmentHandlerTests.cs index d1abb15c1..f70af03b4 100644 --- a/ErsatzTV.Tests/Application/Auth/SeedLocalAdminFromEnvironmentHandlerTests.cs +++ b/ErsatzTV.Tests/Application/Auth/SeedLocalAdminFromEnvironmentHandlerTests.cs @@ -29,7 +29,7 @@ public class SeedLocalAdminFromEnvironmentHandlerTests public async Task TearDown() => await _db.DisposeAsync(); private SeedLocalAdminFromEnvironmentHandler MakeHandler() => - new(_configElementRepository, _passwordHasher); + new(_db.Factory, _passwordHasher); private async Task StoredValue(ConfigElementKey key) => (await _configElementRepository.GetValue(key, CancellationToken.None)).IfNone(""); @@ -53,7 +53,7 @@ public class SeedLocalAdminFromEnvironmentHandlerTests [Test] public async Task Handle_Should_Overwrite_Existing_Credential_And_Rotate_Stamp() { - var claim = new ClaimLocalAdminHandler(_configElementRepository, _passwordHasher); + var claim = new ClaimLocalAdminHandler(_db.Factory, _passwordHasher); (await claim.Handle(new ClaimLocalAdmin("Original", "originalsecret"), CancellationToken.None)) .IsRight.ShouldBeTrue(); string originalStamp = await StoredValue(ConfigElementKey.AuthSecurityStamp); @@ -85,6 +85,23 @@ public class SeedLocalAdminFromEnvironmentHandlerTests (await StoredValue(ConfigElementKey.AuthLocalAdminUsername)).ShouldBe("admin"); } + [Test] + public async Task Handle_Should_Reject_Over_Long_Username() + { + // LocalAdminHelpers.MaxUsernameLength (256) is internal; use the documented bound directly. + string longUsername = new('u', 256 + 1); + SeedLocalAdminFromEnvironmentHandler handler = MakeHandler(); + + Either result = await handler.Handle( + new SeedLocalAdminFromEnvironment(longUsername, "supersecret"), + CancellationToken.None); + + result.IsLeft.ShouldBeTrue(); + (await _configElementRepository.GetConfigElement( + ConfigElementKey.AuthLocalAdminUsername, + CancellationToken.None)).IsNone.ShouldBeTrue(); + } + [Test] public async Task Handle_Should_Reject_Short_Password() { diff --git a/ErsatzTV.Tests/Application/Auth/VerifyLocalAdminLoginHandlerTests.cs b/ErsatzTV.Tests/Application/Auth/VerifyLocalAdminLoginHandlerTests.cs index 7bb20b3e3..9a08680d4 100644 --- a/ErsatzTV.Tests/Application/Auth/VerifyLocalAdminLoginHandlerTests.cs +++ b/ErsatzTV.Tests/Application/Auth/VerifyLocalAdminLoginHandlerTests.cs @@ -32,11 +32,11 @@ public class VerifyLocalAdminLoginHandlerTests public async Task TearDown() => await _db.DisposeAsync(); private VerifyLocalAdminLoginHandler MakeHandler() => - new(_configElementRepository, _passwordHasher); + new(_db.Factory, _passwordHasher); private async Task SeedAdmin() { - var claim = new ClaimLocalAdminHandler(_configElementRepository, _passwordHasher); + var claim = new ClaimLocalAdminHandler(_db.Factory, _passwordHasher); (await claim.Handle(new ClaimLocalAdmin(Username, Password), CancellationToken.None)) .IsRight.ShouldBeTrue(); } diff --git a/ErsatzTV/Controllers/Api/AuthController.cs b/ErsatzTV/Controllers/Api/AuthController.cs index 7734c2306..682bc93e2 100644 --- a/ErsatzTV/Controllers/Api/AuthController.cs +++ b/ErsatzTV/Controllers/Api/AuthController.cs @@ -87,10 +87,17 @@ public class AuthController(IMediator mediator) : ControllerBase }); } - /// Sign out of the cookie session. + /// Sign out of the cookie session. Requires the CSRF header (this controller is filter-exempt). [HttpPost("/api/auth/logout")] public async Task Logout() { + // Prevent forced-logout CSRF: a same-site form POST carries the Lax cookie but can't set a custom + // header. The whole controller is [SkipApiAuthorization], so the filter's CSRF check doesn't apply. + if (RequiresCsrf(out IActionResult csrfError)) + { + return csrfError; + } + await HttpContext.SignOutAsync(AuthConstants.CookieScheme); return NoContent(); } @@ -100,6 +107,12 @@ public class AuthController(IMediator mediator) : ControllerBase [EnableRateLimiting("auth")] public async Task ChangePassword([FromBody] ChangePasswordRequest request, CancellationToken cancellationToken) { + // Filter-exempt controller, so apply the session-mutation CSRF check here explicitly. + if (RequiresCsrf(out IActionResult csrfError)) + { + return csrfError; + } + // A machine key must not be able to rotate the local admin's password — require a local session. if (User.Identity?.IsAuthenticated != true || !string.Equals( @@ -143,6 +156,29 @@ public class AuthController(IMediator mediator) : ControllerBase AuthConstants.OidcScheme); } + // Session-mutation CSRF gate for the [SkipApiAuthorization] auth surface (the global filter doesn't see + // it). Presence-only: a custom header can't be set by a cross-site form/navigation. Returns true (with a + // 403 result) when the header is missing. + private bool RequiresCsrf(out IActionResult error) + { + if (Request.Headers.ContainsKey(ApiAuthorizationFilter.CsrfHeaderName)) + { + error = null; + return false; + } + + error = new ObjectResult(new ProblemDetails + { + Status = StatusCodes.Status403Forbidden, + Title = "Forbidden", + Detail = $"This request requires the '{ApiAuthorizationFilter.CsrfHeaderName}' header." + }) + { + StatusCode = StatusCodes.Status403Forbidden + }; + return true; + } + private Task IssueLocalCookieAsync(LocalAdminPrincipal principal) { var claims = new List diff --git a/ErsatzTV/Services/LocalAdminSeedService.cs b/ErsatzTV/Services/LocalAdminSeedService.cs deleted file mode 100644 index 773e72525..000000000 --- a/ErsatzTV/Services/LocalAdminSeedService.cs +++ /dev/null @@ -1,44 +0,0 @@ -using ErsatzTV.Application.Auth; -using ErsatzTV.Core; -using LanguageExt; -using MediatR; - -namespace ErsatzTV.Services; - -/// -/// Recovery/bootstrap: when Auth:LocalAdmin:Password is configured, (re)seeds the single local -/// administrator at startup (issue #295). Overwrites any existing credential and rotates the security -/// stamp, so an operator locked out of the browser UI can reset by setting the env and restarting. Runs -/// after so the schema exists. A no-op when unset. -/// -public class LocalAdminSeedService( - IServiceScopeFactory serviceScopeFactory, - IConfiguration configuration, - ILogger logger) : IHostedService -{ - public async Task StartAsync(CancellationToken cancellationToken) - { - string password = configuration["Auth:LocalAdmin:Password"]; - if (string.IsNullOrWhiteSpace(password)) - { - return; - } - - string username = configuration["Auth:LocalAdmin:Username"]; - - using IServiceScope scope = serviceScopeFactory.CreateScope(); - IMediator mediator = scope.ServiceProvider.GetRequiredService(); - Either result = - await mediator.Send(new SeedLocalAdminFromEnvironment(username, password), cancellationToken); - - result.Match( - Right: _ => logger.LogWarning( - "Seeded the local administrator from Auth:LocalAdmin:* configuration (any existing credential " - + "was overwritten and all sessions revoked). Unset Auth:LocalAdmin:Password after signing in."), - Left: error => logger.LogError( - "Failed to seed the local administrator from configuration: {Error}", - error.Value)); - } - - public Task StopAsync(CancellationToken cancellationToken) => Task.CompletedTask; -} diff --git a/ErsatzTV/Services/RunOnce/LocalAdminSeedService.cs b/ErsatzTV/Services/RunOnce/LocalAdminSeedService.cs new file mode 100644 index 000000000..45d7b23da --- /dev/null +++ b/ErsatzTV/Services/RunOnce/LocalAdminSeedService.cs @@ -0,0 +1,60 @@ +using ErsatzTV.Application.Auth; +using ErsatzTV.Core; +using LanguageExt; +using MediatR; + +namespace ErsatzTV.Services.RunOnce; + +/// +/// Recovery/bootstrap: when Auth:LocalAdmin:Password is configured, (re)seeds the single local +/// administrator at startup (issue #295). Overwrites any existing credential and rotates the security +/// stamp, so an operator locked out of the browser UI can reset by setting the env and restarting. A +/// no-op when unset. Follows the RunOnce pattern (waits for the database to be ready — the migrator is a +/// BackgroundService, so registration order alone does not guarantee the schema exists). +/// +public class LocalAdminSeedService( + IServiceScopeFactory serviceScopeFactory, + IConfiguration configuration, + SystemStartup systemStartup, + ILogger logger) : BackgroundService +{ + protected override async Task ExecuteAsync(CancellationToken stoppingToken) + { + await Task.Yield(); + + string password = configuration["Auth:LocalAdmin:Password"]; + if (string.IsNullOrWhiteSpace(password)) + { + return; + } + + await systemStartup.WaitForDatabase(stoppingToken); + if (stoppingToken.IsCancellationRequested) + { + return; + } + + string username = configuration["Auth:LocalAdmin:Username"]; + + try + { + using IServiceScope scope = serviceScopeFactory.CreateScope(); + IMediator mediator = scope.ServiceProvider.GetRequiredService(); + Either result = + await mediator.Send(new SeedLocalAdminFromEnvironment(username, password), stoppingToken); + + result.Match( + Right: _ => logger.LogWarning( + "Seeded the local administrator from Auth:LocalAdmin:* configuration (any existing " + + "credential was overwritten and all sessions revoked). Unset Auth:LocalAdmin:Password " + + "after signing in."), + Left: error => logger.LogError( + "Failed to seed the local administrator from configuration: {Error}", + error.Value)); + } + catch (Exception ex) + { + logger.LogError(ex, "Failed to seed the local administrator from configuration"); + } + } +} diff --git a/ErsatzTV/Startup.cs b/ErsatzTV/Startup.cs index 590818fb6..6c65141d4 100644 --- a/ErsatzTV/Startup.cs +++ b/ErsatzTV/Startup.cs @@ -216,16 +216,17 @@ public class Startup if (!trustRestricted) { - // No trusted proxy configured → ignore X-Forwarded-* entirely rather than trust any peer - // (#295). Trusting spoofable forwarded headers would let a LAN peer forge the client IP the - // login rate limiter keys on (and the scheme the cookie Secure policy reads). Behind a proxy, - // set ForwardedHeaders:KnownProxies / :KnownNetworks to re-enable them (required for correct - // client IP + HTTPS detection). - options.ForwardedHeaders = ForwardedHeaders.None; + // Kept as-is from #285 (trust any peer, warn) rather than flipped to ForwardedHeaders.None: + // the forwarded scheme/host feed /iptv M3U/XMLTV/HLS absolute-URL generation + // (Request.Scheme in GetChannelGuideHandler / IptvController), so ignoring them would regress + // stream URLs to http/internal-host for a proxied deployment that hasn't set KnownProxies. + // Setting ForwardedHeaders:KnownProxies/:KnownNetworks is still strongly recommended when + // exposing ErsatzTV beyond a trusted LAN — it also gives the #295 login rate limiter an + // unspoofable client IP and lets the session cookie be marked Secure behind TLS. Log.Warning( - "ForwardedHeaders trust is disabled (no ForwardedHeaders:KnownProxies/KnownNetworks " + - "configured); X-Forwarded-* headers are ignored. Set them to your reverse proxy when " + - "exposing ErsatzTV beyond a trusted LAN so client IP and HTTPS are detected correctly."); + "ForwardedHeaders trusts X-Forwarded-* from any peer (spoofable). Set " + + "ForwardedHeaders:KnownProxies and/or ForwardedHeaders:KnownNetworks to restrict " + + "trust to your reverse proxy when exposing ErsatzTV beyond a trusted LAN."); } }); @@ -1144,7 +1145,8 @@ public class Startup // run-once/blocking startup services services.AddHostedService(); services.AddHostedService(); - // Must run after DatabaseMigratorService so the ConfigElements table exists (#295 env seed). + // Waits on SystemStartup.WaitForDatabase before seeding (#295 env seed) — the migrator is a + // BackgroundService, so registration order alone does not guarantee the schema exists. services.AddHostedService(); services.AddHostedService(); services.AddHostedService(); diff --git a/docs/api-conventions.md b/docs/api-conventions.md index 59d1f9b1f..739aaee48 100644 --- a/docs/api-conventions.md +++ b/docs/api-conventions.md @@ -559,10 +559,18 @@ OpenAPI generation, so the spec can't drift). When you add an endpoint: `Api:CorsAllowedOrigins` (semicolon-separated exact origins). `AllowCredentials` is deliberately **not** set — cross-origin cookie auth is impossible by design (a CSRF defense); cross-origin machine clients use `X-Api-Key`. The policy permits `X-Api-Key`/`X-CSRF`/`If-Match` and exposes `ETag`. -- **ForwardedHeaders is strict opt-in (#295).** With no `ForwardedHeaders:KnownProxies`/`:KnownNetworks` - configured, `X-Forwarded-*` are **ignored** (not trusted from any peer) — otherwise a LAN peer could forge - the client IP the login rate limiter keys on / the scheme the cookie `Secure` policy reads. Behind a reverse - proxy you **must** set `KnownProxies`/`KnownNetworks` for correct client-IP + HTTPS detection. +- **ForwardedHeaders trust is unchanged from #285** (trust `X-Forwarded-*` from any peer by default, with a + warning; restrict via `ForwardedHeaders:KnownProxies`/`:KnownNetworks`). A stricter "ignore unless a proxy is + configured" default was considered for #295 but **reverted** — it would regress `/iptv` M3U/XMLTV/HLS + absolute-URL generation (which reads `Request.Scheme`/`Host`) for a proxied deployment that hasn't set + `KnownProxies`. **Strongly set `KnownProxies`/`:KnownNetworks`** when exposing ErsatzTV behind a proxy — it + also gives the #295 login rate limiter an unspoofable client IP and lets the session cookie be marked + `Secure` behind TLS. +- **Side-effecting GET endpoints are not CSRF-covered by the filter** (it only checks mutating verbs). A few + `[RequiresAuthentication]` GETs have side effects (e.g. `GET /api/troubleshoot/playback.m3u8` starts playback). + Once a session cookie is a normal SPA credential (PR2), those need POST-ification or an explicit `X-CSRF` gate + — tracked as **#301**, which gates PR2 (latent in PR1: the SPA still uses the machine key, so no session + reaches them in normal use). - **PR1 scope note (this change is server-only).** The SPA still authenticates with the stored key (`ctv-api-key`, `X-Api-Key`) until the SPA login flow lands (PR2) — PR1 is backward compatible. The `ApiKeyScreen` → machine-key-management repurpose, the SPA login/setup screens, and `spa-conventions §5e` diff --git a/docs/decisions.md b/docs/decisions.md index f74eee93c..b386bd43d 100644 --- a/docs/decisions.md +++ b/docs/decisions.md @@ -1129,13 +1129,30 @@ user (no enumeration). **Authelia = app-owned OIDC session; never trust proxy identity headers** (owner ruling F3): the container is LAN-reachable bypassing the proxy, so `Remote-User`/`Remote-Email` header trust is spoofable. OIDC→Authelia -gives SSO without a double login. Relatedly, **`ForwardedHeaders` is now strict opt-in**: with no -`KnownProxies`/`KnownNetworks` configured, `X-Forwarded-*` are **ignored** (was: trusted-from-any-peer with a -warning, #285) — otherwise a LAN peer could forge the client IP the rate limiter keys on or the scheme the -cookie `Secure` policy reads. **Deployment coordination:** operators behind a proxy must set -`ForwardedHeaders:KnownProxies`/`:KnownNetworks` (part of the #295 rollout checklist). This only cosmetically -affects the current key-only prod (logs the proxy IP); the host/port guard keys on the `Host` header + -`LocalPort`, not the remote IP. +gives SSO without a double login. **`ForwardedHeaders` behaviour is kept unchanged from #285** (trust any peer +with a warning; restrict via `KnownProxies`/`:KnownNetworks`). A stricter "ignore `X-Forwarded-*` unless a proxy +is configured" default was implemented and then **reverted** after review (cold fork M1): the forwarded +scheme/host feed `/iptv` M3U/XMLTV/HLS absolute-URL generation (`Request.Scheme` in `GetChannelGuideHandler`/ +`IptvController`), so ignoring them would regress stream URLs to `http`/internal-host for a proxied deployment +that hasn't set `KnownProxies`. **Deployment coordination:** operators behind a proxy should set +`ForwardedHeaders:KnownProxies`/`:KnownNetworks` — it gives the login rate limiter an unspoofable client IP and +marks the session cookie `Secure` behind TLS. The residual (a direct LAN peer can spoof `X-Forwarded-For` to +evade the per-IP login limit when unrestricted) is accepted defense-in-depth loss, mitigated by PBKDF2 + +no-enumeration. + +**Review hardening (fork + independent Codex pass, folded into PR1).** Codex caught concurrency defects the +fork missed — folded in: (a) **atomic first-claim-wins** — setup writes the three credential rows in one +transaction guarded by the unique `ConfigElement.Key` index (a lost race → `DbUpdateException` → 409), so a +concurrent claim can't produce a mixed-state credential; (b) **consistent login snapshot** — login reads the +hash + stamp in one query and no longer rehashes-on-verify, so a login racing a password change can't capture a +newer stamp than the hash it verified (a concurrent change either fails the old password or leaves the issued +cookie carrying the pre-change stamp → revoked next request); (c) **env-seed waits on +`SystemStartup.WaitForDatabase`** (the migrator is a `BackgroundService`, so registration order alone didn't +guarantee the schema existed) — moved to `Services/RunOnce/`. Also: **logout + password require `X-CSRF`** +(the `[SkipApiAuthorization]` auth surface isn't covered by the filter's CSRF check → forced-logout CSRF), and +input length caps on username/password. **Deferred with a tracked gate:** side-effecting `[RequiresAuthentication]` +GETs (troubleshoot playback/archive) aren't CSRF-covered — **#301**, gating PR2 (latent in PR1: the SPA still +uses the machine key). OIDC-session revocation lever (no local stamp) noted for PR3 operator docs. **OpenAPI = `ApiKey`-only; `/api/auth/*` excluded** (owner ruling F4): the spec's audience is machine/MCP clients, and a browser-interactive cookie login isn't something a generated client drives, so the cookie path From 9a9aaf0740d4b3607c4a42e7fcebbe78f01bb91a Mon Sep 17 00:00:00 2001 From: Timothy Date: Sun, 12 Jul 2026 16:46:41 +0200 Subject: [PATCH 3/5] =?UTF-8?q?fix(api):=20#295=20PR1=20=E2=80=94=20logout?= =?UTF-8?q?=20ends=20the=20session=20server-side=20(E2E-caught)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Live E2E found that replaying a pre-logout cookie still authenticated (200, not 401): SignOutAsync only clears the CLIENT cookie, but the stateless encrypted cookie ticket stays valid server-side because its security stamp is unchanged — a captured cookie was replayable after logout until ticket expiry. Fix: logout now rotates the local-admin security stamp (RotateLocalAdminSecurityStamp), so every outstanding local session (old stamp) fails OnValidatePrincipal on its next request. For the single admin this is "log out everywhere". Gated on an authenticated local session so an unauthenticated caller can't force-revoke the admin. OIDC sessions (no stamp) are unaffected; SignOutAsync still clears the client cookie for UX. +2 handler tests (rotate-when-configured / no-op-when-unconfigured). Auth suite green (21). Docs: decisions.md note updated. Refs #295 Co-Authored-By: Claude Opus 4.8 (1M context) --- .../Auth/RotateLocalAdminSecurityStamp.cs | 9 +++ .../RotateLocalAdminSecurityStampHandler.cs | 28 ++++++++ ...tateLocalAdminSecurityStampHandlerTests.cs | 65 +++++++++++++++++++ ErsatzTV/Controllers/Api/AuthController.cs | 19 +++++- docs/decisions.md | 6 +- 5 files changed, 124 insertions(+), 3 deletions(-) create mode 100644 ErsatzTV.Application/Auth/RotateLocalAdminSecurityStamp.cs create mode 100644 ErsatzTV.Application/Auth/RotateLocalAdminSecurityStampHandler.cs create mode 100644 ErsatzTV.Tests/Application/Auth/RotateLocalAdminSecurityStampHandlerTests.cs diff --git a/ErsatzTV.Application/Auth/RotateLocalAdminSecurityStamp.cs b/ErsatzTV.Application/Auth/RotateLocalAdminSecurityStamp.cs new file mode 100644 index 000000000..d083e7058 --- /dev/null +++ b/ErsatzTV.Application/Auth/RotateLocalAdminSecurityStamp.cs @@ -0,0 +1,9 @@ +namespace ErsatzTV.Application.Auth; + +/// +/// Rotates the local admin security stamp, revoking every outstanding local session server-side (their +/// cookies carry the old stamp and fail OnValidatePrincipal on their next request). Used by logout +/// so signing out actually ends the session server-side, not just client-side. A no-op when no local +/// admin is configured. OIDC sessions are unaffected (they carry no stamp). +/// +public record RotateLocalAdminSecurityStamp : IRequest; diff --git a/ErsatzTV.Application/Auth/RotateLocalAdminSecurityStampHandler.cs b/ErsatzTV.Application/Auth/RotateLocalAdminSecurityStampHandler.cs new file mode 100644 index 000000000..4f01ee161 --- /dev/null +++ b/ErsatzTV.Application/Auth/RotateLocalAdminSecurityStampHandler.cs @@ -0,0 +1,28 @@ +using ErsatzTV.Core.Domain; +using ErsatzTV.Infrastructure.Data; +using Microsoft.EntityFrameworkCore; + +namespace ErsatzTV.Application.Auth; + +public class RotateLocalAdminSecurityStampHandler(IDbContextFactory dbContextFactory) + : IRequestHandler +{ + public async Task Handle(RotateLocalAdminSecurityStamp request, CancellationToken cancellationToken) + { + await using TvContext dbContext = await dbContextFactory.CreateDbContextAsync(cancellationToken); + + ConfigElement stampRow = await dbContext.ConfigElements + .FirstOrDefaultAsync(c => c.Key == ConfigElementKey.AuthSecurityStamp.Key, cancellationToken); + + // No local admin configured → nothing to revoke. + if (stampRow is null) + { + return Unit.Default; + } + + stampRow.Value = LocalAdminHelpers.NewSecurityStamp(); + await dbContext.SaveChangesAsync(cancellationToken); + + return Unit.Default; + } +} diff --git a/ErsatzTV.Tests/Application/Auth/RotateLocalAdminSecurityStampHandlerTests.cs b/ErsatzTV.Tests/Application/Auth/RotateLocalAdminSecurityStampHandlerTests.cs new file mode 100644 index 000000000..7b1209e6f --- /dev/null +++ b/ErsatzTV.Tests/Application/Auth/RotateLocalAdminSecurityStampHandlerTests.cs @@ -0,0 +1,65 @@ +using ErsatzTV.Application.Auth; +using ErsatzTV.Core; +using ErsatzTV.Core.Domain; +using ErsatzTV.Core.Interfaces.Repositories; +using ErsatzTV.Infrastructure.Data.Repositories; +using ErsatzTV.Tests.Support; +using LanguageExt; +using NUnit.Framework; +using Shouldly; + +namespace ErsatzTV.Tests.Application.Auth; + +[TestFixture] +public class RotateLocalAdminSecurityStampHandlerTests +{ + private InMemoryTvContext _db = null!; + private IConfigElementRepository _configElementRepository = null!; + private ILocalPasswordHasher _passwordHasher = null!; + + [SetUp] + public async Task SetUp() + { + _db = await InMemoryTvContext.CreateAsync(); + _configElementRepository = new ConfigElementRepository(_db.Factory); + _passwordHasher = new LocalPasswordHasher(); + } + + [TearDown] + public async Task TearDown() => await _db.DisposeAsync(); + + private RotateLocalAdminSecurityStampHandler MakeHandler() => new(_db.Factory); + + [Test] + public async Task Handle_Should_Rotate_The_Stamp_When_Configured() + { + // Arrange: claim an admin so a stamp exists. + Either claim = await new ClaimLocalAdminHandler(_db.Factory, _passwordHasher) + .Handle(new ClaimLocalAdmin("admin", "supersecret"), CancellationToken.None); + claim.IsRight.ShouldBeTrue(); + + Option before = + await _configElementRepository.GetValue(ConfigElementKey.AuthSecurityStamp, CancellationToken.None); + string originalStamp = before.Match(s => s, () => throw new ShouldAssertException("expected a stamp")); + + // Act + await MakeHandler().Handle(new RotateLocalAdminSecurityStamp(), CancellationToken.None); + + // Assert: the stamp changed (all outstanding sessions carrying the old stamp are now stale). + Option after = + await _configElementRepository.GetValue(ConfigElementKey.AuthSecurityStamp, CancellationToken.None); + string rotatedStamp = after.Match(s => s, () => throw new ShouldAssertException("expected a stamp")); + rotatedStamp.ShouldNotBe(originalStamp); + rotatedStamp.ShouldNotBeNullOrEmpty(); + } + + [Test] + public async Task Handle_Should_Be_A_No_Op_When_Unconfigured() + { + await MakeHandler().Handle(new RotateLocalAdminSecurityStamp(), CancellationToken.None); + + Option stamp = + await _configElementRepository.GetValue(ConfigElementKey.AuthSecurityStamp, CancellationToken.None); + stamp.IsNone.ShouldBeTrue(); + } +} diff --git a/ErsatzTV/Controllers/Api/AuthController.cs b/ErsatzTV/Controllers/Api/AuthController.cs index 682bc93e2..52dbca279 100644 --- a/ErsatzTV/Controllers/Api/AuthController.cs +++ b/ErsatzTV/Controllers/Api/AuthController.cs @@ -87,9 +87,15 @@ public class AuthController(IMediator mediator) : ControllerBase }); } - /// Sign out of the cookie session. Requires the CSRF header (this controller is filter-exempt). + /// + /// Sign out of the cookie session. Requires the CSRF header (this controller is filter-exempt). For a + /// local session this rotates the security stamp, ending the session server-side (a captured + /// cookie can't be replayed after logout) — which, for the single local admin, revokes all local + /// sessions ("log out everywhere"). Gated on an authenticated session so an unauthenticated caller + /// can't force-revoke the admin. + /// [HttpPost("/api/auth/logout")] - public async Task Logout() + public async Task Logout(CancellationToken cancellationToken) { // Prevent forced-logout CSRF: a same-site form POST carries the Lax cookie but can't set a custom // header. The whole controller is [SkipApiAuthorization], so the filter's CSRF check doesn't apply. @@ -98,6 +104,15 @@ public class AuthController(IMediator mediator) : ControllerBase return csrfError; } + if (User.Identity?.IsAuthenticated == true && + string.Equals( + User.FindFirst(AuthConstants.AuthMethodClaim)?.Value, + AuthConstants.MethodLocal, + StringComparison.Ordinal)) + { + await mediator.Send(new RotateLocalAdminSecurityStamp(), cancellationToken); + } + await HttpContext.SignOutAsync(AuthConstants.CookieScheme); return NoContent(); } diff --git a/docs/decisions.md b/docs/decisions.md index b386bd43d..2eb2899d9 100644 --- a/docs/decisions.md +++ b/docs/decisions.md @@ -1150,7 +1150,11 @@ cookie carrying the pre-change stamp → revoked next request); (c) **env-seed w `SystemStartup.WaitForDatabase`** (the migrator is a `BackgroundService`, so registration order alone didn't guarantee the schema existed) — moved to `Services/RunOnce/`. Also: **logout + password require `X-CSRF`** (the `[SkipApiAuthorization]` auth surface isn't covered by the filter's CSRF check → forced-logout CSRF), and -input length caps on username/password. **Deferred with a tracked gate:** side-effecting `[RequiresAuthentication]` +input length caps on username/password. **Logout rotates the security stamp** when called from a local session +(E2E-caught: `SignOutAsync` alone only clears the *client* cookie, leaving the stateless encrypted ticket +replayable server-side) — so signing out actually ends the session server-side; for the single admin this +revokes all local sessions ("log out everywhere"). Gated on an authenticated session so an unauthenticated +caller can't force-revoke the admin. **Deferred with a tracked gate:** side-effecting `[RequiresAuthentication]` GETs (troubleshoot playback/archive) aren't CSRF-covered — **#301**, gating PR2 (latent in PR1: the SPA still uses the machine key). OIDC-session revocation lever (no local stamp) noted for PR3 operator docs. From e8c3481ea56a60d2def4b5dd47b575c03493efda Mon Sep 17 00:00:00 2001 From: Timothy Date: Sun, 12 Jul 2026 16:52:23 +0200 Subject: [PATCH 4/5] =?UTF-8?q?fix(api):=20#295=20PR1=20=E2=80=94=20fold?= =?UTF-8?q?=20in=20fix-commit=20re-review=20(2nd=20Codex=20round)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fix-commit re-review confirmed the 1st-round fixes resolved and caught a 2nd round: - HIGH — env-seed vs. setup race: an attacker could claim admin in the startup window before LocalAdminSeedService runs, and the seed's insert would then be swallowed (attacker credential persists, defeating env recovery). Fixed structurally: the setup-claim endpoint is CLOSED (409) whenever Auth:LocalAdmin:Password is configured — the env seed owns the credential, so there's no claim to race (also strengthens the setup-claim TOFU posture). Config.setupRequired reflects it. - LOW — a concurrent setup race-loser now returns 409 (not 422); ClaimLocalAdmin's DbUpdateException catch re-checks existence and rethrows genuine/transient DB errors instead of masking them as "already configured". - MEDIUM (accepted, documented) — two simultaneous authenticated password changes are a non-serializable lost-update; accepted for a single-admin system (self-healing via re-login, implausible timing). +3 AuthController tests (env-seed closes setup / setupRequired gating). Full ErsatzTV.Tests green (1506); no generated drift. Docs updated. Refs #295 Co-Authored-By: Claude Opus 4.8 (1M context) --- .../Auth/ClaimLocalAdminHandler.cs | 15 +++- .../Controllers/AuthControllerTests.cs | 71 +++++++++++++++++++ ErsatzTV/Controllers/Api/AuthController.cs | 28 +++++++- docs/api-conventions.md | 5 +- docs/decisions.md | 14 ++++ 5 files changed, 127 insertions(+), 6 deletions(-) create mode 100644 ErsatzTV.Tests/Controllers/AuthControllerTests.cs diff --git a/ErsatzTV.Application/Auth/ClaimLocalAdminHandler.cs b/ErsatzTV.Application/Auth/ClaimLocalAdminHandler.cs index ba43c0cc1..550fc1cdb 100644 --- a/ErsatzTV.Application/Auth/ClaimLocalAdminHandler.cs +++ b/ErsatzTV.Application/Auth/ClaimLocalAdminHandler.cs @@ -48,8 +48,19 @@ public class ClaimLocalAdminHandler(IDbContextFactory dbContextFactor } catch (DbUpdateException) { - // Lost the first-claim race — a concurrent claim inserted these keys first (unique Key index). - return BaseError.New("A local administrator has already been configured"); + // A write conflict here is (almost always) a lost first-claim race — a concurrent claim inserted + // these keys first (unique Key index). Confirm the row now exists on a fresh context before + // reporting "already configured"; otherwise this was a genuine/transient DB error → rethrow rather + // than mask it. + await using TvContext verifyContext = await dbContextFactory.CreateDbContextAsync(cancellationToken); + bool nowConfigured = await verifyContext.ConfigElements + .AnyAsync(c => c.Key == ConfigElementKey.AuthLocalAdminPasswordHash.Key, cancellationToken); + if (nowConfigured) + { + return BaseError.New("A local administrator has already been configured"); + } + + throw; } return new LocalAdminPrincipal(username, stamp); diff --git a/ErsatzTV.Tests/Controllers/AuthControllerTests.cs b/ErsatzTV.Tests/Controllers/AuthControllerTests.cs new file mode 100644 index 000000000..84d34c320 --- /dev/null +++ b/ErsatzTV.Tests/Controllers/AuthControllerTests.cs @@ -0,0 +1,71 @@ +using System.Collections.Generic; +using ErsatzTV.Application.Auth; +using ErsatzTV.Controllers.Api; +using ErsatzTV.Controllers.Api.Requests; +using MediatR; +using Microsoft.AspNetCore.Http; +using Microsoft.AspNetCore.Mvc; +using Microsoft.Extensions.Configuration; +using NSubstitute; +using NUnit.Framework; +using Shouldly; + +namespace ErsatzTV.Tests.Controllers; + +[TestFixture] +public class AuthControllerTests +{ + private static IConfiguration Config(bool envSeed) => + new ConfigurationBuilder() + .AddInMemoryCollection( + envSeed + ? new Dictionary { ["Auth:LocalAdmin:Password"] = "seed-password" } + : new Dictionary()) + .Build(); + + [Test] + public async Task Config_Reports_Setup_Not_Required_When_Env_Seed_Configured() + { + var mediator = Substitute.For(); + mediator.Send(Arg.Any(), Arg.Any()).Returns(false); + + var controller = new AuthController(mediator, Config(envSeed: true)); + + var result = await controller.Config(CancellationToken.None) as OkObjectResult; + var body = result!.Value.ShouldBeOfType(); + + // Env seed owns the credential → the SPA must not offer the browser setup-claim. + body.SetupRequired.ShouldBeFalse(); + } + + [Test] + public async Task Config_Reports_Setup_Required_When_Unconfigured_And_No_Env_Seed() + { + var mediator = Substitute.For(); + mediator.Send(Arg.Any(), Arg.Any()).Returns(false); + + var controller = new AuthController(mediator, Config(envSeed: false)); + + var result = await controller.Config(CancellationToken.None) as OkObjectResult; + var body = result!.Value.ShouldBeOfType(); + + body.SetupRequired.ShouldBeTrue(); + } + + [Test] + public async Task Setup_Is_Closed_With_409_When_Env_Seed_Configured() + { + var mediator = Substitute.For(); + var controller = new AuthController(mediator, Config(envSeed: true)) + { + ControllerContext = new ControllerContext { HttpContext = new DefaultHttpContext() } + }; + + var result = await controller.Setup(new SetupRequest("admin", "hunter2pw"), CancellationToken.None); + + var problem = result.ShouldBeOfType(); + problem.StatusCode.ShouldBe(StatusCodes.Status409Conflict); + // The claim must never be attempted while the env seed owns the credential. + await mediator.DidNotReceive().Send(Arg.Any(), Arg.Any()); + } +} diff --git a/ErsatzTV/Controllers/Api/AuthController.cs b/ErsatzTV/Controllers/Api/AuthController.cs index 52dbca279..cc633a61d 100644 --- a/ErsatzTV/Controllers/Api/AuthController.cs +++ b/ErsatzTV/Controllers/Api/AuthController.cs @@ -21,14 +21,19 @@ namespace ErsatzTV.Controllers.Api; [ApiController] [ApiExplorerSettings(IgnoreApi = true)] [SkipApiAuthorization] -public class AuthController(IMediator mediator) : ControllerBase +public class AuthController(IMediator mediator, IConfiguration configuration) : ControllerBase { + // When the operator has set an env seed, the local admin is managed via configuration — the browser + // setup-claim is closed. This also eliminates the startup race where an attacker could claim admin in the + // window before LocalAdminSeedService runs (the seed would then lose and its insert be swallowed). + private bool EnvSeedConfigured => !string.IsNullOrWhiteSpace(configuration["Auth:LocalAdmin:Password"]); + /// Public: what auth options exist + whether first-run setup is still needed (drives the SPA boot gate). [HttpGet("/api/auth/config")] public async Task Config(CancellationToken cancellationToken) { bool configured = await mediator.Send(new IsLocalAdminConfigured(), cancellationToken); - return Ok(new AuthConfigResponse(OidcHelper.IsEnabled, true, !configured)); + return Ok(new AuthConfigResponse(OidcHelper.IsEnabled, true, !configured && !EnvSeedConfigured)); } /// The current session (anonymous is a 200 with authenticated=false, never a 401). @@ -48,6 +53,13 @@ public class AuthController(IMediator mediator) : ControllerBase [EnableRateLimiting("auth")] public async Task Setup([FromBody] SetupRequest request, CancellationToken cancellationToken) { + if (EnvSeedConfigured) + { + return ApiResults.ConflictProblem( + "Managed via configuration", + "The local administrator is provisioned from Auth:LocalAdmin:* configuration; browser setup is disabled."); + } + if (await mediator.Send(new IsLocalAdminConfigured(), cancellationToken)) { return ApiResults.ConflictProblem("Already configured", "A local administrator already exists."); @@ -57,7 +69,17 @@ public class AuthController(IMediator mediator) : ControllerBase await mediator.Send(new ClaimLocalAdmin(request.Username, request.Password), cancellationToken); return await result.Match( - Left: error => Task.FromResult(error.ToErrorResult()), + Left: async error => + { + // A concurrent claim that lost the race reports 409 (the record now exists), not the 422 a + // validation error gets. + if (await mediator.Send(new IsLocalAdminConfigured(), cancellationToken)) + { + return ApiResults.ConflictProblem("Already configured", "A local administrator already exists."); + } + + return error.ToErrorResult(); + }, Right: async principal => { await IssueLocalCookieAsync(principal); diff --git a/docs/api-conventions.md b/docs/api-conventions.md index 739aaee48..4920318e3 100644 --- a/docs/api-conventions.md +++ b/docs/api-conventions.md @@ -554,7 +554,10 @@ OpenAPI generation, so the spec can't drift). When you add an endpoint: rows (`AuthLocalAdminUsername`/`AuthLocalAdminPasswordHash` (PBKDF2) / `AuthSecurityStamp`) — **no DB migration**; a password change rotates the stamp, revoking sessions via the cookie `OnValidatePrincipal`. Recovery/bootstrap without the browser: set `Auth:LocalAdmin:Password` (+ optional `…:Username`, default - `admin`) and restart (`LocalAdminSeedService` reseeds + rotates the stamp). + `admin`) and restart (`LocalAdminSeedService` reseeds + rotates the stamp). While that env is set the + **browser setup-claim is disabled** (409) — the env seed owns the credential, which also removes the + startup setup-vs-seed race. Logout rotates the security stamp for a local session (ends it server-side, + "log out everywhere"), gated on an authenticated session. - **CORS** is opt-in: no cross-origin access by default (the SPA is same-origin from `/app`); set `Api:CorsAllowedOrigins` (semicolon-separated exact origins). `AllowCredentials` is deliberately **not** set — cross-origin cookie auth is impossible by design (a CSRF defense); cross-origin machine clients use diff --git a/docs/decisions.md b/docs/decisions.md index 2eb2899d9..aa11cd3ab 100644 --- a/docs/decisions.md +++ b/docs/decisions.md @@ -1158,6 +1158,20 @@ caller can't force-revoke the admin. **Deferred with a tracked gate:** side-effe GETs (troubleshoot playback/archive) aren't CSRF-covered — **#301**, gating PR2 (latent in PR1: the SPA still uses the machine key). OIDC-session revocation lever (no local stamp) noted for PR3 operator docs. +A **fix-commit re-review** (Codex, #242 discipline) then confirmed the above resolved and caught a second round: +(a) **HIGH — env-seed vs. setup race**: an attacker could claim admin in the startup window before +`LocalAdminSeedService` runs, and the seed's insert would then be swallowed (attacker's credential persists, +defeating the env recovery path). Fixed structurally: **the setup-claim endpoint is closed whenever +`Auth:LocalAdmin:Password` is configured** — the env seed owns the credential, so there is no claim to race +(this also strengthens the setup-claim TOFU posture: an operator on an untrusted network sets the env password +and browser setup is disabled). (b) **LOW**: a concurrent setup race-loser now returns **409** (not 422), and +`ClaimLocalAdmin`'s `DbUpdateException` catch re-checks existence and **rethrows genuine/transient DB errors** +rather than masking them as "already configured". (c) **MEDIUM — accepted**: two *simultaneous* authenticated +password changes are a non-serializable lost-update (last-write-wins; the loser's cookie may be immediately +revoked). Accepted for a **single-admin** system: it needs two concurrent authenticated sessions both submitting +the correct current password at the same instant, and the outcome is self-healing (re-login). Adding EF +optimistic concurrency to the credential rows is disproportionate here. + **OpenAPI = `ApiKey`-only; `/api/auth/*` excluded** (owner ruling F4): the spec's audience is machine/MCP clients, and a browser-interactive cookie login isn't something a generated client drives, so the cookie path is an additional accepted credential the doc needn't express. `AuthController` is `[ApiExplorerSettings(IgnoreApi From 0badff811d968dcca685229b45f65947e41eb9aa Mon Sep 17 00:00:00 2001 From: Timothy Date: Sun, 12 Jul 2026 17:29:34 +0200 Subject: [PATCH 5/5] feat(process): #303 H6 merge-consent derived from ## Done-when checklist MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Wave 2 hook H6: derive merge-consent from state instead of memory. An issue's ## Done-when checklist (issue body) becomes the machine-readable source of truth for whether its PR may merge — the structural fix for the queue-drift #303 tracks (status was living in append-only prose). - pretooluse-merge-consent.sh (Claude PreToolUse on mcp__gitea__ pull_request_write): a merge is ALLOWED only when the PR's CI is green AND every ## Done-when box on the linked issue (fixes #N) is ticked; DENY on an unticked box / red CI; ASK (human prompt) when state isn't derivable (no linked issue, no section, no creds, Gitea down). Docs/ process-only PRs exempt. - .husky/pre-push -> prepush-donewhen.sh: fail-open backstop for a direct `git push origin main`; blocks only on a positively-proven unticked box. Gitea auth from env only (ETV_GITEA_BASICAUTH / ETV_GITEA_TOKEN, ETV_GITEA_URL) — nothing committed; without creds the gate degrades to today's manual confirmation, never a silent pass. Non-breaking rollout: until issues adopt ## Done-when the merge hook simply asks. Pipe-tested: non-merge->allow, no-creds->ask, docs-only->allow, checklist parser (unit), linked-issue extraction, and a live end-to-end block path (temp Done-when on #303 -> exit 1 -> restored). Docs: CLAUDE.md Task Completion Protocol + decisions.md entry. Refs #303. Co-Authored-By: Claude Opus 4.8 (1M context) --- .claude/hooks/prepush-donewhen.sh | 69 +++++++++++++++ .claude/hooks/pretooluse-merge-consent.sh | 101 ++++++++++++++++++++++ .claude/settings.json | 10 +++ .husky/pre-push | 6 ++ CLAUDE.md | 6 ++ docs/decisions.md | 24 +++++ 6 files changed, 216 insertions(+) create mode 100755 .claude/hooks/prepush-donewhen.sh create mode 100755 .claude/hooks/pretooluse-merge-consent.sh diff --git a/.claude/hooks/prepush-donewhen.sh b/.claude/hooks/prepush-donewhen.sh new file mode 100755 index 000000000..4ca45400b --- /dev/null +++ b/.claude/hooks/prepush-donewhen.sh @@ -0,0 +1,69 @@ +#!/usr/bin/env bash +# Husky pre-push backstop for ersatztv#303 H6 — the fast-forward-to-main path the Claude merge +# hook (pretooluse-merge-consent.sh) can't see. Reads git's pre-push ref lines on stdin; for a push +# to main it scans the pushed commits for a Gitea close-keyword (`fixes #N`), and if the linked +# issue's "## Done-when" checklist still has unticked boxes it BLOCKS the push. +# +# A git hook has no interactive "ask", so this is deliberately fail-OPEN: it only blocks when it can +# positively prove an unticked box (creds present, issue fetched, non-docs change). No creds, Gitea +# unreachable, docs-only diff, or no linked issue -> allow (a loud warning at most). The authoritative +# gate is the merge hook; this just catches a direct `git push origin main`. +# +# Auth (never committed): ETV_GITEA_TOKEN or ETV_GITEA_BASICAUTH; ETV_GITEA_URL overrides the base. +set -euo pipefail + +# git passes " " lines on stdin. +refs=$(cat || true) +printf '%s\n' "$refs" | grep -q 'refs/heads/main' || exit 0 # only gate pushes to main + +base_url="${ETV_GITEA_URL:-http://192.168.1.95:3000}/api/v1" +if [ -z "${ETV_GITEA_TOKEN:-}" ] && [ -z "${ETV_GITEA_BASICAUTH:-}" ]; then + exit 0 # can't verify -> fail-open (the merge hook is the real gate) +fi +gq() { + if [ -n "${ETV_GITEA_TOKEN:-}" ]; then + curl -sf -H "Authorization: token $ETV_GITEA_TOKEN" "$base_url/$1" 2>/dev/null || true + else + curl -sf -u "$ETV_GITEA_BASICAUTH" "$base_url/$1" 2>/dev/null || true + fi +} + +zero=0000000000000000000000000000000000000000 +blocked="" +while read -r localref localsha remoteref remotesha; do + [ "$remoteref" = "refs/heads/main" ] || continue + [ "$localsha" = "$zero" ] && continue # branch deletion + # Commit range being pushed. New branch (remotesha all-zero) -> just the tip, don't rescan history. + if [ "$remotesha" = "$zero" ]; then range="$localsha -1"; else range="$remotesha..$localsha"; fi + msgs=$(git log --format='%B' $range 2>/dev/null || true) + issues=$(printf '%s' "$msgs" | grep -ioE '(close[sd]?|fix(e[sd])?|resolve[sd]?) +#[0-9]+' | grep -oE '[0-9]+' | sort -u || true) + [ -n "$issues" ] || continue + + # Docs-only exemption over the pushed range. + changed=$(git diff --name-only $range 2>/dev/null || true) + if [ -n "$changed" ] && ! printf '%s\n' "$changed" | grep -qvE '^(docs/|\.claude/|\.husky/|\.gitea/|.*\.md$)'; then + continue + fi + + for n in $issues; do + ibody=$(gq "repos/timothy/ersatztv/issues/$n" | jq -r '.body // ""' 2>/dev/null || true) + [ -n "$ibody" ] || continue # can't fetch -> fail-open + unchecked=$(printf '%s\n' "$ibody" | awk ' + /^##[[:space:]]+[Dd]one-when/ {grab=1; next} + grab && /^##[[:space:]]/ {grab=0} + grab {print}' | grep -cE '^[[:space:]]*[-*][[:space:]]+\[[[:space:]]\]' || true) + if [ "${unchecked:-0}" -gt 0 ]; then + blocked="${blocked} - issue #$n has $unchecked unticked ## Done-when box(es)\n" + fi + done +done <&2 + printf '%b' "$blocked" >&2 + printf 'Finish/tick every Done-when criterion (incl. adversarial review) first, or push a docs-only change.\n' >&2 + exit 1 +fi +exit 0 diff --git a/.claude/hooks/pretooluse-merge-consent.sh b/.claude/hooks/pretooluse-merge-consent.sh new file mode 100755 index 000000000..5b8b6457e --- /dev/null +++ b/.claude/hooks/pretooluse-merge-consent.sh @@ -0,0 +1,101 @@ +#!/usr/bin/env bash +# PreToolUse / mcp__gitea__pull_request_write — derive merge consent from STATE instead of +# trusting the agent's judgment (ersatztv#303 H6). A PR merge is the one irreversible op; allow it +# only when BOTH are true: +# (a) the PR's CI combined status is green, AND +# (b) every checkbox in the linked issue's "## Done-when" section is ticked. +# The "## Done-when" issue-body checklist is the convention (docs/decisions.md, CLAUDE.md Task +# Completion Protocol). One box is "adversarial review passed"; the others are per-issue. +# +# Decision policy — a CONSENT gate, so it does NOT fail silently open: +# - state derivable and NOT satisfied -> deny (actionable reason) +# - state derivable and satisfied -> allow +# - state NOT derivable (no creds, Gitea down, +# no linked issue, no Done-when section) -> ask (surface to a human/session judgment) +# Only a real merge is gated; every other pull_request_write method is allowed untouched. +# +# Gitea auth from env (never committed): ETV_GITEA_TOKEN (a token) OR ETV_GITEA_BASICAUTH (user:pass). +# ETV_GITEA_URL overrides the base (default: the LAN instance; a LAN address, not a secret). +set -euo pipefail +input=$(cat) + +decide() { # $1=allow|deny|ask $2=reason + case "$1" in + allow) exit 0 ;; + deny) jq -n --arg r "$2" '{hookSpecificOutput:{hookEventName:"PreToolUse",permissionDecision:"deny",permissionDecisionReason:$r}}'; exit 0 ;; + ask) jq -n --arg r "$2" '{hookSpecificOutput:{hookEventName:"PreToolUse",permissionDecision:"ask",permissionDecisionReason:$r}}'; exit 0 ;; + esac +} + +method=$(printf '%s' "$input" | jq -r '.tool_input.method // ""' 2>/dev/null || true) +[ "$method" = "merge" ] || decide allow "" + +owner=$(printf '%s' "$input" | jq -r '.tool_input.owner // ""' 2>/dev/null || true) +repo=$(printf '%s' "$input" | jq -r '.tool_input.repo // ""' 2>/dev/null || true) +pr=$(printf '%s' "$input" | jq -r '.tool_input.pull_number // ""' 2>/dev/null || true) +mwcs=$(printf '%s' "$input" | jq -r '.tool_input.merge_when_checks_succeed // false' 2>/dev/null || true) +[ -n "$owner" ] && [ -n "$repo" ] && [ -n "$pr" ] || decide ask "H6 merge gate: could not read owner/repo/pull_number from the merge call; confirm manually that CI is green and the issue's Done-when boxes are ticked." + +base_url="${ETV_GITEA_URL:-http://192.168.1.95:3000}/api/v1" +# curl wrapper carrying whichever auth is configured; empty output on any failure. +gq() { + local path="$1" + if [ -n "${ETV_GITEA_TOKEN:-}" ]; then + curl -sf -H "Authorization: token $ETV_GITEA_TOKEN" "$base_url/$path" 2>/dev/null || true + elif [ -n "${ETV_GITEA_BASICAUTH:-}" ]; then + curl -sf -u "$ETV_GITEA_BASICAUTH" "$base_url/$path" 2>/dev/null || true + else + return 1 + fi +} +if [ -z "${ETV_GITEA_TOKEN:-}" ] && [ -z "${ETV_GITEA_BASICAUTH:-}" ]; then + decide ask "H6 merge gate: no Gitea credentials in env (ETV_GITEA_TOKEN or ETV_GITEA_BASICAUTH), so CI/Done-when state can't be verified. Confirm manually that CI is green and the linked issue's Done-when boxes are all ticked, then approve." +fi + +prjson=$(gq "repos/$owner/$repo/pulls/$pr") +[ -n "$prjson" ] || decide ask "H6 merge gate: could not fetch PR #$pr from Gitea (unreachable or auth rejected). Verify CI-green + Done-when manually before merging." + +sha=$(printf '%s' "$prjson" | jq -r '.head.sha // ""' 2>/dev/null || true) +body=$(printf '%s' "$prjson" | jq -r '.body // ""' 2>/dev/null || true) + +# --- Docs-only exemption: if every changed file is docs/process, skip the gate. --- +files=$(gq "repos/$owner/$repo/pulls/$pr/files?limit=100" | jq -r '.[].filename // empty' 2>/dev/null || true) +if [ -n "$files" ] && ! printf '%s\n' "$files" | grep -qvE '^(docs/|\.claude/|\.husky/|\.gitea/|.*\.md$)'; then + decide allow "" # all changed files are docs/process-only +fi + +# --- Linked issue: Gitea auto-close keywords in the PR body. --- +issues=$(printf '%s' "$body" | grep -ioE '(close[sd]?|fix(e[sd])?|resolve[sd]?) +#[0-9]+' | grep -oE '[0-9]+' | sort -u || true) +[ -n "$issues" ] || decide ask "H6 merge gate: PR #$pr has no linked issue (no 'fixes #N' / 'closes #N' in its body), so there is no Done-when checklist to derive consent from. Confirm the work is complete + reviewed, then approve." + +# --- (b) Done-when checkboxes: every linked issue must have an all-ticked section. --- +for n in $issues; do + ibody=$(gq "repos/$owner/$repo/issues/$n" | jq -r '.body // ""' 2>/dev/null || true) + [ -n "$ibody" ] || decide ask "H6 merge gate: could not fetch linked issue #$n. Verify its Done-when checklist manually before merging." + # Slice the "## Done-when" section: from that header to the next "## " (or EOF). + section=$(printf '%s\n' "$ibody" | awk ' + /^##[[:space:]]+[Dd]one-when/ {grab=1; next} + grab && /^##[[:space:]]/ {grab=0} + grab {print}') + if [ -z "$(printf '%s' "$section" | tr -d '[:space:]')" ]; then + decide ask "H6 merge gate: linked issue #$n has no '## Done-when' checklist section (the merge-consent convention — see CLAUDE.md Task Completion Protocol). Add one, or confirm completion manually and approve." + fi + unchecked=$(printf '%s\n' "$section" | grep -cE '^[[:space:]]*[-*][[:space:]]+\[[[:space:]]\]' || true) + if [ "${unchecked:-0}" -gt 0 ]; then + decide deny "H6 merge gate: BLOCKED — linked issue #$n has $unchecked unticked box(es) in its ## Done-when checklist. Finish (or explicitly tick) every completion criterion — including the adversarial-review box — before merging PR #$pr." + fi +done + +# --- (a) CI combined status must be green (unless deferring to Gitea's own check-gate). --- +if [ "$mwcs" != "true" ]; then + [ -n "$sha" ] || decide ask "H6 merge gate: could not resolve PR #$pr head sha to check CI. Verify CI is green before merging." + state=$(gq "repos/$owner/$repo/commits/$sha/status" | jq -r '.state // ""' 2>/dev/null || true) + case "$state" in + success) : ;; + "") decide ask "H6 merge gate: could not read CI status for PR #$pr ($sha). Verify CI is green before merging." ;; + *) decide deny "H6 merge gate: BLOCKED — PR #$pr CI status is '$state', not 'success'. Wait for a green build (or pass merge_when_checks_succeed to let Gitea gate it) before merging." ;; + esac +fi + +# Both derivable and satisfied -> allow. +decide allow "" diff --git a/.claude/settings.json b/.claude/settings.json index 44e2d99c4..da1c83a68 100644 --- a/.claude/settings.json +++ b/.claude/settings.json @@ -36,6 +36,16 @@ "timeout": 10 } ] + }, + { + "matcher": "mcp__gitea__pull_request_write", + "hooks": [ + { + "type": "command", + "command": "\"$CLAUDE_PROJECT_DIR/.claude/hooks/pretooluse-merge-consent.sh\"", + "timeout": 15 + } + ] } ], "PostToolUse": [ diff --git a/.husky/pre-push b/.husky/pre-push index cbc7cc985..327f063c0 100755 --- a/.husky/pre-push +++ b/.husky/pre-push @@ -1,3 +1,9 @@ +# H6 merge-consent backstop (ersatztv#303): gate a direct push to main on the linked issue's +# ## Done-when checklist. Read git's pre-push ref lines FIRST (before the web checks below, which +# may consume stdin) and forward them. Fail-open: no creds / not main / docs-only -> allow. +_prepush_refs="$(cat)" +printf '%s\n' "$_prepush_refs" | ./.claude/hooks/prepush-donewhen.sh || exit 1 + # Git exports GIT_DIR/GIT_WORK_TREE/GIT_INDEX_FILE while running hooks. In a worktree # (or any subdir), an explicit GIT_DIR makes nested `git` commands mislocate the working # tree — notably `check:api`'s `git diff --exit-code` (run from web/) silently reports "no diff --git a/CLAUDE.md b/CLAUDE.md index d65c4a8b1..3a095ee8c 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -82,6 +82,12 @@ docker build -f docker/Dockerfile -t ersatztv:dev . Every task that closes a Gitea issue MUST complete ALL of these before it is considered done. Use `/done ` to run through this automatically. +**Merge-consent is derived from state, not asserted (`## Done-when` convention — ersatztv#303 H6).** Any issue whose PR will merge to `main` should carry a `## Done-when` section in its **issue body** — a checklist of completion criteria (always include an "adversarial review passed" box; add per-issue criteria like tests-green, docs-updated, live-E2E). Two hooks derive merge-consent from it so a premature merge is blocked *by construction*, not by memory: +- `pretooluse-merge-consent.sh` (Claude PreToolUse on the Gitea merge tool) — **allows** a merge only when the PR's CI is green **and** every `## Done-when` box on the linked issue (`fixes #N`) is ticked; **denies** on an unticked box or red CI; **asks** (falls back to a human prompt) when it can't derive state (no linked issue, no `## Done-when` section, no creds, Gitea down). +- `.husky/pre-push` → `prepush-donewhen.sh` — a fail-open backstop that blocks a direct `git push origin main` whose commits `fix #N` an issue with unticked boxes. + +Both need Gitea read creds in the env to enforce (**`ETV_GITEA_BASICAUTH=user:pass`** or `ETV_GITEA_TOKEN`; `ETV_GITEA_URL` overrides the base). Without them the merge hook asks and the push backstop is a no-op — the gate degrades to today's manual confirmation, never a silent pass. Docs-only PRs/pushes are exempt. + 1. **Root cause** (bug fixes / incidents only): Document WHY the problem existed, not just what was changed. If root cause is unknown, say so explicitly and open a follow-up investigation issue. Fixing symptoms without understanding causes creates recurring problems. 2. **Comment on issues** as you work — what you found, what approach you're taking, any deviations from the suggested fix. 3. **Push changes**: `git push` all commits before closing. Use `fixes #N` in commit messages to auto-close where appropriate. diff --git a/docs/decisions.md b/docs/decisions.md index 01f7e710f..2788a1dbd 100644 --- a/docs/decisions.md +++ b/docs/decisions.md @@ -1233,3 +1233,27 @@ is an additional accepted credential the doc needn't express. `AuthController` i `X-CSRF`, `AuthContext` + boot gate, login/setup screens, `ApiKeyScreen`→machine-key management, E2E, `spa-conventions §5e`). PR3 = key rotation + operator docs (Authelia client + env reference). Rollout: PR1→PR2 same release, then a manual Authelia round-trip checklist before the prod pin bump. + +--- + +## 2026-07-12 — Merge-consent derived from state via a `## Done-when` issue checklist (#303 H6) + +**An issue's `## Done-when` checklist (in the issue body) is the machine-readable source of truth for whether +its PR may merge; consent is *derived*, not asserted.** Rationale: DONE/OPEN status used to live in +append-only prose that lags live Gitea state (the queue-drift #303 fixes) — so the completion gate moves out +of memory and into a checklist two hooks read. Convention: the issue body carries a `## Done-when` section +(always an "adversarial review passed" box, plus per-issue criteria); a merge is allowed only when the PR's CI +is green **and** every box on the linked issue (`fixes #N`) is ticked. + +Enforcement (both fail *safe*, never a silent pass): +- `pretooluse-merge-consent.sh` — Claude PreToolUse on `mcp__gitea__pull_request_write` merge: **deny** on an + unticked box or non-green CI; **allow** when both satisfied; **ask** (human prompt) when state isn't + derivable (no linked issue, no `## Done-when`, no creds, Gitea unreachable). Docs-only PRs exempt. +- `.husky/pre-push` → `prepush-donewhen.sh` — backstop for a direct `git push origin main`; fail-*open* (a + git hook has no "ask"), blocks only on a positively-proven unticked box. + +Both authenticate to Gitea from env only (`ETV_GITEA_BASICAUTH` / `ETV_GITEA_TOKEN`, `ETV_GITEA_URL`) — no +creds committed; without them the gate degrades to today's manual confirmation. Rollout is non-breaking: until +issues adopt `## Done-when`, the merge hook simply *asks* rather than auto-allowing. See CLAUDE.md → Task +Completion Protocol. (H6 lives with H1/H2/H8 in `.claude/settings.json`; H7 worktree-owner guard is its +sibling Wave-2 hook.)