Build ErsatzTV Image / Docs update reminder (pull_request) Successful in 7s
Build ErsatzTV Image / EF migration integrity (SQLite + MySql) (pull_request) Successful in 4m28s
Build ErsatzTV Image / Build & test (.NET) (pull_request) Successful in 5m42s
Build ErsatzTV Image / Build & push image (amd64) (pull_request) Has been skipped
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) <noreply@anthropic.com>
165 lines
7.0 KiB
C#
165 lines
7.0 KiB
C#
using System.Reflection;
|
|
using ErsatzTV;
|
|
using ErsatzTV.Controllers.Api;
|
|
using ErsatzTV.Filters;
|
|
using Microsoft.AspNetCore.Hosting;
|
|
using Microsoft.AspNetCore.Mvc;
|
|
using Microsoft.AspNetCore.Mvc.Routing;
|
|
using Microsoft.Extensions.Configuration;
|
|
using Microsoft.Extensions.DependencyInjection;
|
|
using Microsoft.Extensions.FileProviders;
|
|
using Microsoft.Extensions.Options;
|
|
using NSubstitute;
|
|
using NUnit.Framework;
|
|
using Shouldly;
|
|
|
|
namespace ErsatzTV.Tests.Controllers;
|
|
|
|
[TestFixture]
|
|
public class ApiControllerSecurityTests
|
|
{
|
|
private static readonly bool ApiAuthorizationFilterIsGlobal = IsApiAuthorizationFilterRegisteredGlobally();
|
|
|
|
[Test]
|
|
public void Every_Mutating_Api_Action_Should_Be_Globally_Protected_Or_Explicitly_Exempt()
|
|
{
|
|
// Scan the assembly instead of hand-maintaining a list of controllers here: a
|
|
// hardcoded array silently drifts as new controllers are added (9 were missing at
|
|
// one point, including two with real mutating actions - ArtworkUploadController POST
|
|
// and ChannelTemplateController POST/PUT/DELETE - that were never actually checked).
|
|
// Note: not every controller in this namespace derives from ControllerBase (e.g.
|
|
// ChannelController, ScannerController, SessionController, MaintenanceController,
|
|
// TroubleshootController) - filter on [ApiController] + concrete class only, matching
|
|
// the pattern already used by ScannerController_Should_Be_Only_Api_Key_Exempt_Api_Controller
|
|
// below, so those controllers stay covered rather than silently dropping out of the scan.
|
|
Type[] apiControllers = typeof(CollectionController)
|
|
.Assembly
|
|
.GetTypes()
|
|
.Where(t => t.Namespace == typeof(CollectionController).Namespace)
|
|
.Where(t => t is { IsClass: true, IsAbstract: false })
|
|
.Where(t => t.GetCustomAttributes<ApiControllerAttribute>(inherit: true).Any())
|
|
.ToArray();
|
|
|
|
// Guard against the scan silently matching nothing (e.g. a namespace rename) and
|
|
// giving this test a false pass.
|
|
apiControllers.Length.ShouldBeGreaterThanOrEqualTo(20);
|
|
|
|
foreach (Type controllerType in apiControllers)
|
|
{
|
|
bool controllerSkipsApiKey = controllerType
|
|
.GetCustomAttributes<SkipApiAuthorizationAttribute>(inherit: true)
|
|
.Any();
|
|
|
|
foreach (MethodInfo action in controllerType
|
|
.GetMethods(BindingFlags.Public | BindingFlags.Instance | BindingFlags.DeclaredOnly))
|
|
{
|
|
bool isMutating = action
|
|
.GetCustomAttributes<HttpMethodAttribute>(inherit: true)
|
|
.SelectMany(a => a.HttpMethods)
|
|
.Any(m => m is "POST" or "PUT" or "PATCH" or "DELETE");
|
|
|
|
if (!isMutating)
|
|
{
|
|
continue;
|
|
}
|
|
|
|
bool actionSkipsApiKey = action
|
|
.GetCustomAttributes<SkipApiAuthorizationAttribute>(inherit: true)
|
|
.Any();
|
|
|
|
(controllerSkipsApiKey || actionSkipsApiKey || IsGloballyProtected())
|
|
.ShouldBeTrue($"{controllerType.Name}.{action.Name} must be covered by global API write auth or explicitly exempt");
|
|
}
|
|
}
|
|
}
|
|
|
|
[Test]
|
|
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<ApiControllerAttribute>(inherit: true).Any())
|
|
.Where(t => t.GetCustomAttributes<SkipApiAuthorizationAttribute>(inherit: true).Any())
|
|
.ToArray();
|
|
|
|
exemptControllers.ShouldBe([typeof(ScannerController), typeof(AuthController)], ignoreOrder: true);
|
|
}
|
|
|
|
[Test]
|
|
public void Startup_Should_Register_ApiAuthorizationFilter_Globally()
|
|
{
|
|
ApiAuthorizationFilterIsGlobal.ShouldBeTrue();
|
|
}
|
|
|
|
[Test]
|
|
public void Sensitive_Read_Controllers_Should_Require_Api_Key()
|
|
{
|
|
// These GET surfaces disclose secrets/paths or trigger work; they must stay gated even if an
|
|
// operator disables Api:RequireKeyForReads (issue #282). Asserted reflectively so the tier
|
|
// can't silently drop the attribute.
|
|
Type[] sensitiveControllers =
|
|
[
|
|
typeof(TroubleshootController),
|
|
typeof(LogsController),
|
|
typeof(SettingsController),
|
|
typeof(MaintenanceController)
|
|
];
|
|
|
|
foreach (Type controllerType in sensitiveControllers)
|
|
{
|
|
controllerType.GetCustomAttributes<RequiresAuthenticationAttribute>(inherit: true).Any()
|
|
.ShouldBeTrue($"{controllerType.Name} must carry [RequiresAuthentication]");
|
|
}
|
|
}
|
|
|
|
[Test]
|
|
public void ScannerController_Should_Be_Localhost_Only()
|
|
{
|
|
// The scanner callback is exempt from the API key (guessable scan-id GUID); a loopback check
|
|
// replaces the GUID as the real gate (issue #285).
|
|
typeof(ScannerController)
|
|
.GetCustomAttributes<LocalhostOnlyAttribute>(inherit: true)
|
|
.Any()
|
|
.ShouldBeTrue();
|
|
}
|
|
|
|
private static bool IsGloballyProtected() => ApiAuthorizationFilterIsGlobal;
|
|
|
|
private static bool IsApiAuthorizationFilterRegisteredGlobally()
|
|
{
|
|
var settings = new Dictionary<string, string?>
|
|
{
|
|
["provider"] = "sqlite",
|
|
["ConnectionStrings:Data"] = "Data Source=:memory:"
|
|
};
|
|
|
|
IConfiguration configuration = new ConfigurationBuilder()
|
|
.AddInMemoryCollection(settings)
|
|
.Build();
|
|
|
|
var environment = Substitute.For<IWebHostEnvironment>();
|
|
environment.ApplicationName.Returns("ErsatzTV");
|
|
environment.EnvironmentName.Returns("Development");
|
|
environment.ContentRootPath.Returns(TestContext.CurrentContext.TestDirectory);
|
|
environment.WebRootPath.Returns(TestContext.CurrentContext.TestDirectory);
|
|
environment.ContentRootFileProvider.Returns(new NullFileProvider());
|
|
environment.WebRootFileProvider.Returns(new NullFileProvider());
|
|
|
|
var services = new ServiceCollection();
|
|
new Startup(configuration, environment).ConfigureServices(services);
|
|
|
|
using ServiceProvider provider = services.BuildServiceProvider();
|
|
MvcOptions options = provider.GetRequiredService<IOptions<MvcOptions>>().Value;
|
|
|
|
return options.Filters
|
|
.OfType<ServiceFilterAttribute>()
|
|
.Any(a => a.ServiceType == typeof(ApiAuthorizationFilter));
|
|
}
|
|
}
|