Backend of #197 Bundle A (auth posture). Owner decisions: single API key; Api:RequireKeyForReads defaults true (whole /api surface gated; /iptv streaming + guide unaffected — outside the filter's /api scope). - #280 S1: writes are fail-closed. New IApiKeyProvider resolves the key once (Api:WriteKey config, else persisted /config/api.key, else a generated 256-bit key written 0600). The empty-key open branch is gone; there is no open mode. - #282 S3/S5: reads under /api require the key when Api:RequireKeyForReads (default true) or the endpoint carries the new [RequiresApiKey]. Applied [RequiresApiKey] to Troubleshoot/Logs/Settings/Maintenance so the sensitive tier stays gated even if reads are opened. OPTIONS preflight is exempt. - #281 S2: delete SortController (dead Blazor SortableJS residue; SPA uses PUT /api/collections/{id}/custom-order) and AccountController (dead OIDC logout) — both non-/api persistent surfaces that bypassed the key. - #284 S6: replace CORS AllowAll with an opt-in exact-origin allowlist (Api:CorsAllowedOrigins; permits X-Api-Key/If-Match, exposes ETag). Default is no cross-origin (SPA is same-origin). - #285 S7/S10: gc GET->POST (spec regenerated); ForwardedHeaders trust configurable via ForwardedHeaders:KnownProxies/KnownNetworks (warns when unrestricted); ScannerController gains [LocalhostOnly] (scanner always calls back over localhost). Filter unit tests rewritten for fail-closed + read-gating + tier + OPTIONS; ApiControllerSecurityTests assert the sensitive tier + scanner-loopback reflectively. search/all-items paging deferred (SPA add-all coupling) — exposure closed by read-gating. Refs #197 #280 #281 #282 #284 #285 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
161 lines
6.6 KiB
C#
161 lines
6.6 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 ApiKeyAuthorizationFilterIsGlobal = IsApiKeyAuthorizationFilterRegisteredGlobally();
|
|
|
|
[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<SkipApiKeyAuthorizationAttribute>(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<SkipApiKeyAuthorizationAttribute>(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 ScannerController_Should_Be_Only_Api_Key_Exempt_Api_Controller()
|
|
{
|
|
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<SkipApiKeyAuthorizationAttribute>(inherit: true).Any())
|
|
.ToArray();
|
|
|
|
exemptControllers.ShouldBe([typeof(ScannerController)]);
|
|
}
|
|
|
|
[Test]
|
|
public void Startup_Should_Register_ApiKeyAuthorizationFilter_Globally()
|
|
{
|
|
ApiKeyAuthorizationFilterIsGlobal.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<RequiresApiKeyAttribute>(inherit: true).Any()
|
|
.ShouldBeTrue($"{controllerType.Name} must carry [RequiresApiKey]");
|
|
}
|
|
}
|
|
|
|
[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() => ApiKeyAuthorizationFilterIsGlobal;
|
|
|
|
private static bool IsApiKeyAuthorizationFilterRegisteredGlobally()
|
|
{
|
|
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(ApiKeyAuthorizationFilter));
|
|
}
|
|
}
|