From 682dceec8f2cb46fb36c8f277a9f64de71ad46fa Mon Sep 17 00:00:00 2001 From: Timothy Date: Mon, 13 Jul 2026 00:40:58 +0200 Subject: [PATCH] =?UTF-8?q?fix(api):=20#286=20review=20=E2=80=94=20allowli?= =?UTF-8?q?st=20non-/api=20routes=20in=20the=20versioning=20test;=20base-u?= =?UTF-8?q?rl-aware=20deprecation=20Link?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Cold-fork + Codex review of PR #326: - ApiRouteVersioningTests: iterate IRouteTemplateProvider (covers a template-less [HttpGet] paired with an action-level [Route]) and assert any non-/api route against an explicit KnownNonApiRoutes allowlist instead of silently skipping — an accidental absolute non-/api route (which would also escape ApiAuthorizationFilter's /api-scoped gate) now fails the test. - ApiVersionRewriteMiddleware: root the deprecation Link at Request.PathBase so it stays correct under ETV_BASE_URL (, not host-root ). refs #286 #197 Co-Authored-By: Claude Opus 4.8 (1M context) --- .../Controllers/ApiRouteVersioningTests.cs | 30 ++++++++++++++----- .../Middleware/ApiVersionRewriteMiddleware.cs | 4 ++- 2 files changed, 25 insertions(+), 9 deletions(-) diff --git a/ErsatzTV.Tests/Controllers/ApiRouteVersioningTests.cs b/ErsatzTV.Tests/Controllers/ApiRouteVersioningTests.cs index 689adb191..4559e4066 100644 --- a/ErsatzTV.Tests/Controllers/ApiRouteVersioningTests.cs +++ b/ErsatzTV.Tests/Controllers/ApiRouteVersioningTests.cs @@ -21,6 +21,13 @@ public class ApiRouteVersioningTests { private static readonly Regex VersionedAbsolute = new(@"^/api/v\d+/", RegexOptions.Compiled); + // Routes on an API controller that deliberately live OUTSIDE /api because they are browser-navigation + // endpoints, not part of the JSON API surface (api-conventions §9). This is an explicit allowlist, NOT + // a blanket skip: an accidental non-/api absolute route (e.g. a stray [HttpPost("/channels")]) must fail + // here, because it would also escape ApiAuthorizationFilter's /api-scoped gate → an unauthenticated + // mutation. The only intentional entry today is AuthController's OIDC challenge. + private static readonly string[] KnownNonApiRoutes = ["/auth/oidc/login"]; + [Test] public void Every_Api_Controller_Action_Route_Should_Be_Versioned_And_Absolute() { @@ -49,24 +56,31 @@ public class ApiRouteVersioningTests foreach (MethodInfo action in controllerType .GetMethods(BindingFlags.Public | BindingFlags.Instance | BindingFlags.DeclaredOnly)) { - foreach (HttpMethodAttribute httpAttribute in action - .GetCustomAttributes(inherit: true)) + // IRouteTemplateProvider covers BOTH [HttpGet("...")] (HttpMethodAttribute) and a bare + // action-level [Route("...")] — so a template-less [HttpGet] paired with [Route("...")] + // can't slip an unversioned route past this net (cold-review nit, #326). + foreach (IRouteTemplateProvider routeProvider in action + .GetCustomAttributes(inherit: true) + .OfType()) { - string? effective = CombineRoute(controllerTemplate, httpAttribute.Template); + string? effective = CombineRoute(controllerTemplate, routeProvider.Template); if (effective is null) { // No route on the controller or the action → not a routable API endpoint; skip. continue; } - // A few actions on API controllers deliberately live OUTSIDE /api because they are - // browser-navigation endpoints, not part of the JSON API surface — e.g. - // AuthController's GET /auth/oidc/login OIDC challenge (api-conventions §9). The - // versioning convention only governs the /api surface; a non-/api nav route is a - // different surface and must NOT be forced to /api/v1. + // A route outside /api must be a KNOWN, intentional browser-nav endpoint — never a + // silent skip (see KnownNonApiRoutes above for why: it would also escape the /api-scoped + // auth filter). A new one fails here until it's explicitly allowlisted or versioned. if (!effective.StartsWith("/api/", StringComparison.OrdinalIgnoreCase) && !effective.Equals("/api", StringComparison.OrdinalIgnoreCase)) { + KnownNonApiRoutes.ShouldContain( + effective, + $"{controllerType.Name}.{action.Name} route '{effective}' is neither versioned " + + "(/api/v1/…) nor a known non-/api browser-nav endpoint — version it, or add it to " + + "KnownNonApiRoutes if it is intentionally outside the JSON API surface."); continue; } diff --git a/ErsatzTV/Middleware/ApiVersionRewriteMiddleware.cs b/ErsatzTV/Middleware/ApiVersionRewriteMiddleware.cs index 6422b6243..d24d4c7c2 100644 --- a/ErsatzTV/Middleware/ApiVersionRewriteMiddleware.cs +++ b/ErsatzTV/Middleware/ApiVersionRewriteMiddleware.cs @@ -37,7 +37,9 @@ public sealed class ApiVersionRewriteMiddleware { context.Request.Path = rewritten; context.Response.Headers["Deprecation"] = "true"; - context.Response.Headers["Link"] = "; rel=\"deprecation\""; + // Root the docs link at the request PathBase so it stays correct under a reverse-proxy + // base URL (ETV_BASE_URL): empty PathBase → , PathBase "/etv" → . + context.Response.Headers["Link"] = $"<{context.Request.PathBase}/docs>; rel=\"deprecation\""; if (!string.IsNullOrWhiteSpace(_sunset)) { context.Response.Headers["Sunset"] = _sunset;