From a0426109f5943487c61da1ad92a96225747ac3ac Mon Sep 17 00:00:00 2001 From: Rex Lorenzo Date: Fri, 28 Aug 2026 11:11:18 -0700 Subject: [PATCH 1/2] fix(auth): deny by default with an authorization FallbackPolicy Attribute-routed controllers are not covered by the RequireAuthorization() on the conventional routes, so a controller that forgot [Authorize] was reachable unauthenticated. That is what let MembersController drift. - Exempt the endpoints that answer anonymously by design: the CAS sign-in and error actions already carried [AllowAnonymous], CMS serves public files and checks per-file permissions itself, Layout returns a permission-filtered nav, and LoggedInUser reports who is signed in - Exempt the health endpoints, which are deliberately reachable when the auth subsystems are degraded and have no CAS credentials from Jenkins --- test/Classes/AuthorizationFallbackTests.cs | 27 +++++++++++++++++++ web/Areas/CMS/Controllers/CMSController.cs | 4 +++ .../HealthChecks/HealthCheckExtensions.cs | 6 ++--- web/Controllers/LayoutController.cs | 3 +++ web/Controllers/LoggedInUserController.cs | 5 +++- web/Program.cs | 6 +++++ 6 files changed, 47 insertions(+), 4 deletions(-) create mode 100644 test/Classes/AuthorizationFallbackTests.cs diff --git a/test/Classes/AuthorizationFallbackTests.cs b/test/Classes/AuthorizationFallbackTests.cs new file mode 100644 index 000000000..b1a775a5c --- /dev/null +++ b/test/Classes/AuthorizationFallbackTests.cs @@ -0,0 +1,27 @@ +using Microsoft.AspNetCore.Authorization; +using Viper.Areas.CMS.Controllers; +using Viper.Controllers; + +namespace Viper.test.Classes +{ + /// + /// Asserts that the controllers which answer anonymously by design still carry [AllowAnonymous]. + /// The FallbackPolicy in Program.cs is not asserted here: this only pins the opt-outs, because + /// losing one would deny the endpoint at runtime and break sign-in or the public nav rather + /// than fail a build. + /// + public class AuthorizationFallbackTests + { + public static TheoryData AnonymousControllers() => + new(typeof(CMSController), typeof(LayoutController), typeof(LoggedInUserController)); + + [Theory] + [MemberData(nameof(AnonymousControllers))] + public void AnonymousControllers_CarryAllowAnonymous(Type controller) + { + Assert.True(controller.GetCustomAttributes(typeof(AllowAnonymousAttribute), inherit: true).Length > 0, + $"{controller.Name} answers anonymously by design and must carry [AllowAnonymous], " + + "otherwise the FallbackPolicy denies it"); + } + } +} diff --git a/web/Areas/CMS/Controllers/CMSController.cs b/web/Areas/CMS/Controllers/CMSController.cs index 0223a9fc9..bca024a3b 100644 --- a/web/Areas/CMS/Controllers/CMSController.cs +++ b/web/Areas/CMS/Controllers/CMSController.cs @@ -1,3 +1,4 @@ +using Microsoft.AspNetCore.Authorization; using Microsoft.AspNetCore.Mvc; using Microsoft.AspNetCore.RateLimiting; using Viper.Areas.CMS.Services; @@ -7,6 +8,9 @@ namespace Viper.Areas.CMS.Controllers { [Route("/CMS/[action]")] + // Serves public files and enforces per-file permissions itself, so it opts out of the + // fallback policy rather than sitting behind a blanket authenticated-user check. + [AllowAnonymous] public class CMSController : Controller { private readonly VIPERContext _viperContext; diff --git a/web/Classes/HealthChecks/HealthCheckExtensions.cs b/web/Classes/HealthChecks/HealthCheckExtensions.cs index bd34f2e87..f05e56725 100644 --- a/web/Classes/HealthChecks/HealthCheckExtensions.cs +++ b/web/Classes/HealthChecks/HealthCheckExtensions.cs @@ -263,7 +263,7 @@ public static WebApplication UseViperHealthChecks(this WebApplication app) app.MapHealthChecks("/health", new HealthCheckOptions { Predicate = _ => false, - }); + }).AllowAnonymous(); // /health/detail - per-check JSON (UI format), IP-allowlisted to SVM // infra via InternalAllowlist. Intentionally not CAS-gated so the @@ -286,7 +286,7 @@ public static WebApplication UseViperHealthChecks(this WebApplication app) return null; } return await next(ctx); - }); + }).AllowAnonymous(); // IP-gate every UI sub-path (HTML page, API, resource files, webhook config). app.UseWhen( @@ -358,7 +358,7 @@ public static WebApplication UseViperHealthChecks(this WebApplication app) options.AddCustomStylesheet(Path.Join( app.Environment.ContentRootPath, "wwwroot", "css", "healthchecks-ui-branding.css")); - }); + }).AllowAnonymous(); return app; } diff --git a/web/Controllers/LayoutController.cs b/web/Controllers/LayoutController.cs index b2024fafa..20ace4192 100644 --- a/web/Controllers/LayoutController.cs +++ b/web/Controllers/LayoutController.cs @@ -1,3 +1,4 @@ +using Microsoft.AspNetCore.Authorization; using Microsoft.AspNetCore.Mvc; using Viper.Areas.ClinicalScheduler.Services; using Viper.Areas.CMS.Data; @@ -9,6 +10,8 @@ namespace Viper.Controllers { [Route("/api/layout")] + // Returns a permission-filtered nav to anonymous visitors, who get the public subset. + [AllowAnonymous] public class LayoutController : ApiController { private readonly RAPSContext _context; diff --git a/web/Controllers/LoggedInUserController.cs b/web/Controllers/LoggedInUserController.cs index 14c9b64db..7951b39d2 100644 --- a/web/Controllers/LoggedInUserController.cs +++ b/web/Controllers/LoggedInUserController.cs @@ -1,4 +1,5 @@ using Microsoft.AspNetCore.Antiforgery; +using Microsoft.AspNetCore.Authorization; using Microsoft.AspNetCore.Mvc; using Viper.Classes; using Viper.Classes.SQLContext; @@ -6,7 +7,9 @@ namespace Viper.Controllers { [Route("/api/loggedInUser")] - //[Permission(Allow = "SVMSecure")] + // Answers anonymously by design: the front end asks who is logged in before it knows + // whether anyone is, and gets a null user rather than a 401. + [AllowAnonymous] public class LoggedInUserController : ApiController { private readonly IAntiforgery _antiforgery; diff --git a/web/Program.cs b/web/Program.cs index 636320a04..c4ff81f24 100644 --- a/web/Program.cs +++ b/web/Program.cs @@ -169,6 +169,12 @@ .RequireAuthenticatedUser() .AddRequirements(new AuthorizationPolicyBuilder().RequireClaim(ClaimTypes.AuthenticationMethod, "CAS").Build().Requirements.ToArray()) .Build(); + + // Attribute-routed controllers are not covered by RequireAuthorization() on the conventional + // routes, so a controller that forgets [Authorize] is public. Deny by default instead. The + // handful that answer anonymously opt out explicitly: [AllowAnonymous] on controllers, + // .AllowAnonymous() on endpoint mappings such as the health checks. + options.FallbackPolicy = options.DefaultPolicy; }); // Add services necessary for nonces in CSP, 32-byte nonces From 13984e48d183b661bb11207dce7bbaa46ef9ad29 Mon Sep 17 00:00:00 2001 From: Rex Lorenzo Date: Fri, 28 Aug 2026 19:12:37 -0700 Subject: [PATCH 2/2] chore: ignore review-pr scratch files The review-pr workflow writes .review-pr-ignored- in the repo root to track dismissed threads. It was untracked but not ignored, so a git add -A swept one into a commit. --- .gitignore | 1 + 1 file changed, 1 insertion(+) diff --git a/.gitignore b/.gitignore index aabccddee..174d32e0d 100644 --- a/.gitignore +++ b/.gitignore @@ -10,6 +10,7 @@ # Agent AI development scratch docs (plans, smoke-test guides); not committed PLAN-*.md SMOKETEST-*.md +.review-pr-ignored-* # User-specific files *.rsuser