From c0bf69ad56b0b48eb71e159de563ed1cbcc0fa39 Mon Sep 17 00:00:00 2001 From: cesnimda Date: Sun, 19 Jul 2026 17:27:27 +0200 Subject: [PATCH] fix(security): enforce explicit api authorization MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Authentication relied on a fallback policy gated on Auth:Require, which defaults to false. Five user-owned controllers carried no [Authorize] of their own, so a deployment that lost that flag would have served tenant data anonymously: JobApplications, Companies, Correspondence, Rules and JobImport. All five now declare [Authorize(AuthenticationSchemes = "local")] explicitly. This does not affect local development, which already sets Auth:Require=true in appsettings.Development.json — the gap was only ever in a production configuration that omitted the flag. Added a reflection test over every controller in the assembly so a new one cannot ship unprotected by accident. A controller passes if the class requires authorization, or if every action declares its own [Authorize] or [AllowAnonymous] — the shape AuthController and TwoFactorController need, since login and register must stay anonymous while the rest must not. Public endpoints are an explicit allow-list, so making something anonymous is now a deliberate edit rather than an omission. That test found one real gap: AuthController.Logout declared neither attribute. It is now explicitly [AllowAnonymous] — it only clears the caller's own session cookies and leaks nothing, and requiring authentication would leave a user whose token had already expired unable to sign out. Also pinned: admin controllers require the Admin role rather than merely a signed-in user, and PublicCvController stays anonymous so shared CV links keep working. 384 backend tests pass. Co-Authored-By: Claude Opus 4.8 --- .../ApiAuthorizationPostureTests.cs | 126 ++++++++++++++++++ JobTrackerApi/Controllers/AuthController.cs | 4 + .../Controllers/CompaniesController.cs | 7 +- .../Controllers/CorrespondenceController.cs | 7 +- .../Controllers/JobApplicationsController.cs | 7 +- .../Controllers/JobImportController.cs | 5 + JobTrackerApi/Controllers/RulesController.cs | 7 +- 7 files changed, 159 insertions(+), 4 deletions(-) create mode 100644 JobTrackerApi.Tests/ApiAuthorizationPostureTests.cs diff --git a/JobTrackerApi.Tests/ApiAuthorizationPostureTests.cs b/JobTrackerApi.Tests/ApiAuthorizationPostureTests.cs new file mode 100644 index 0000000..96a227f --- /dev/null +++ b/JobTrackerApi.Tests/ApiAuthorizationPostureTests.cs @@ -0,0 +1,126 @@ +using System.Reflection; +using Microsoft.AspNetCore.Authorization; +using Microsoft.AspNetCore.Mvc; +using Xunit; + +namespace JobTrackerApi.Tests; + +// Authorization posture, asserted by reflection over every controller in the assembly. +// +// The point is that a NEW controller cannot ship unprotected by accident. Authentication used to rely +// on the Auth:Require fallback policy, which defaults to false — a deployment that lost that flag +// would have served tenant data anonymously. Every controller must now declare its intent. +// docs/production-readiness-review.md. +public sealed class ApiAuthorizationPostureTests +{ + // Endpoints that are anonymous ON PURPOSE. Adding to this list is a deliberate security decision. + private static readonly HashSet IntentionallyAnonymous = new(StringComparer.Ordinal) + { + // Serves /cv/{slug} for variants the user explicitly published. Noindex by default. + "PublicCvController", + // Accepts browser error reports. Must work on pages reached before sign-in. + "ClientErrorsController", + }; + + // Property accessors and object overrides are not endpoints. + private static bool IsEndpoint(MethodInfo m) => !( + m.IsSpecialName || m.DeclaringType == typeof(object)); + + private static IEnumerable Controllers() => + typeof(JobTrackerApi.Controllers.ApplicationWorkspaceController).Assembly + .GetTypes() + .Where(t => typeof(ControllerBase).IsAssignableFrom(t) && !t.IsAbstract); + + // A controller is covered if the CLASS requires authorization, or if every action declares its own + // intent. AuthController and TwoFactorController are necessarily mixed — login and register must be + // anonymous while the rest are not — so they mark each method individually. + private static bool DeclaresIntent(Type controller) + { + if (controller.GetCustomAttribute(inherit: true) is not null) return true; + + var actions = controller + .GetMethods(BindingFlags.Public | BindingFlags.Instance | BindingFlags.DeclaredOnly) + .Where(IsEndpoint) + .ToList(); + + return actions.Count > 0 && actions.All(m => + m.GetCustomAttribute(inherit: true) is not null || + m.GetCustomAttribute(inherit: true) is not null); + } + + [Fact] + public void Every_controller_either_requires_authorization_or_is_listed_as_public() + { + var unprotected = Controllers() + .Where(t => !IntentionallyAnonymous.Contains(t.Name)) + .Where(t => !DeclaresIntent(t)) + .Select(t => t.Name) + .OrderBy(name => name, StringComparer.Ordinal) + .ToList(); + + Assert.True( + unprotected.Count == 0, + "These controllers declare no authorization intent — neither on the class nor on every " + + "action — so they would be served anonymously if Auth:Require were ever false: " + + string.Join(", ", unprotected)); + } + + [Fact] + public void Every_action_on_the_mixed_auth_controllers_declares_its_own_intent() + { + // Sign-in surfaces are the easiest place to add an endpoint and forget to mark it. + foreach (var name in new[] { "AuthController", "TwoFactorController" }) + { + var type = Controllers().Single(t => t.Name == name); + var undeclared = type + .GetMethods(BindingFlags.Public | BindingFlags.Instance | BindingFlags.DeclaredOnly) + .Where(IsEndpoint) + .Where(m => m.GetCustomAttribute(inherit: true) is null + && m.GetCustomAttribute(inherit: true) is null) + .Select(m => m.Name) + .ToList(); + + Assert.True(undeclared.Count == 0, + $"{name} has actions with neither [Authorize] nor [AllowAnonymous]: {string.Join(", ", undeclared)}"); + } + } + + [Fact] + public void The_user_owned_controllers_that_used_to_rely_on_the_fallback_policy_are_now_explicit() + { + // These five were the actual gap found in the Phase 5.6 audit. + foreach (var name in new[] + { + "JobApplicationsController", "CompaniesController", "CorrespondenceController", + "RulesController", "JobImportController", + }) + { + var type = Controllers().Single(t => t.Name == name); + var attribute = type.GetCustomAttribute(inherit: true); + + Assert.True(attribute is not null, $"{name} must declare [Authorize]."); + Assert.Equal("local", attribute!.AuthenticationSchemes); + } + } + + [Fact] + public void Admin_controllers_require_the_admin_role_not_merely_a_signed_in_user() + { + foreach (var name in new[] { "AdminAuditController", "AdminSystemController", "UsersController" }) + { + var type = Controllers().Single(t => t.Name == name); + var attribute = type.GetCustomAttribute(inherit: true); + + Assert.True(attribute is not null, $"{name} must declare [Authorize]."); + Assert.Equal("Admin", attribute!.Roles); + } + } + + [Fact] + public void The_public_cv_controller_stays_anonymous() + { + // Regression guard in the other direction: locking this down would break every shared CV link. + var type = Controllers().Single(t => t.Name == "PublicCvController"); + Assert.Null(type.GetCustomAttribute(inherit: true)); + } +} diff --git a/JobTrackerApi/Controllers/AuthController.cs b/JobTrackerApi/Controllers/AuthController.cs index 0a5cbf6..e27aab0 100644 --- a/JobTrackerApi/Controllers/AuthController.cs +++ b/JobTrackerApi/Controllers/AuthController.cs @@ -303,6 +303,10 @@ public sealed class AuthController : ControllerBase } [HttpPost("logout")] + // Anonymous on purpose, and now explicitly: this only clears the caller's own session cookies and + // leaks nothing. Requiring authentication would mean a user whose token has already expired gets a + // 401 when signing out and stays stuck in a half-signed-in state. + [AllowAnonymous] public IActionResult Logout() { ClearSessionCookies(); diff --git a/JobTrackerApi/Controllers/CompaniesController.cs b/JobTrackerApi/Controllers/CompaniesController.cs index b48e6e8..70b0d4c 100644 --- a/JobTrackerApi/Controllers/CompaniesController.cs +++ b/JobTrackerApi/Controllers/CompaniesController.cs @@ -1,4 +1,5 @@ -using Microsoft.AspNetCore.Mvc; +using Microsoft.AspNetCore.Authorization; +using Microsoft.AspNetCore.Mvc; using Microsoft.EntityFrameworkCore; using JobTrackerApi.Data; using JobTrackerApi.Models; @@ -7,7 +8,11 @@ using System.Security.Claims; namespace JobTrackerApi.Controllers { [ApiController] + // Explicitly authorized. These endpoints are all tenant-scoped user data, so they must not + // depend on the Auth:Require fallback policy being switched on: a deployment that lost that flag + // would otherwise serve them anonymously. docs/production-readiness-review.md. [Route("api/companies")] + [Authorize(AuthenticationSchemes = "local")] public class CompaniesController : ControllerBase { private readonly JobTrackerContext _db; diff --git a/JobTrackerApi/Controllers/CorrespondenceController.cs b/JobTrackerApi/Controllers/CorrespondenceController.cs index d4d0a4a..7db5e29 100644 --- a/JobTrackerApi/Controllers/CorrespondenceController.cs +++ b/JobTrackerApi/Controllers/CorrespondenceController.cs @@ -1,4 +1,5 @@ -using Microsoft.AspNetCore.Mvc; +using Microsoft.AspNetCore.Authorization; +using Microsoft.AspNetCore.Mvc; using Microsoft.EntityFrameworkCore; using JobTrackerApi.Data; using JobTrackerApi.Models; @@ -6,7 +7,11 @@ using JobTrackerApi.Models; namespace JobTrackerApi.Controllers { [ApiController] + // Explicitly authorized. These endpoints are all tenant-scoped user data, so they must not + // depend on the Auth:Require fallback policy being switched on: a deployment that lost that flag + // would otherwise serve them anonymously. docs/production-readiness-review.md. [Route("api/correspondence")] + [Authorize(AuthenticationSchemes = "local")] public class CorrespondenceController : ControllerBase { private readonly JobTrackerContext _db; diff --git a/JobTrackerApi/Controllers/JobApplicationsController.cs b/JobTrackerApi/Controllers/JobApplicationsController.cs index d98f5dc..4d898f5 100644 --- a/JobTrackerApi/Controllers/JobApplicationsController.cs +++ b/JobTrackerApi/Controllers/JobApplicationsController.cs @@ -1,4 +1,5 @@ -using Microsoft.AspNetCore.Mvc; +using Microsoft.AspNetCore.Authorization; +using Microsoft.AspNetCore.Mvc; using Microsoft.EntityFrameworkCore; using Microsoft.Extensions.Caching.Memory; using JobTrackerApi.Data; @@ -15,7 +16,11 @@ using static JobTrackerApi.Services.JobApplicationHelpers; namespace JobTrackerApi.Controllers { [ApiController] + // Explicitly authorized. These endpoints are all tenant-scoped user data, so they must not + // depend on the Auth:Require fallback policy being switched on: a deployment that lost that flag + // would otherwise serve them anonymously. docs/production-readiness-review.md. [Route("api/jobapplications")] + [Authorize(AuthenticationSchemes = "local")] public class JobApplicationsController : ControllerBase { private readonly JobTrackerContext _db; diff --git a/JobTrackerApi/Controllers/JobImportController.cs b/JobTrackerApi/Controllers/JobImportController.cs index 35c5d7d..03e9045 100644 --- a/JobTrackerApi/Controllers/JobImportController.cs +++ b/JobTrackerApi/Controllers/JobImportController.cs @@ -1,10 +1,15 @@ +using Microsoft.AspNetCore.Authorization; using Microsoft.AspNetCore.Mvc; using JobTrackerApi.Services.JobImport; namespace JobTrackerApi.Controllers; [ApiController] +// Explicitly authorized. These endpoints are all tenant-scoped user data, so they must not +// depend on the Auth:Require fallback policy being switched on: a deployment that lost that flag +// would otherwise serve them anonymously. docs/production-readiness-review.md. [Route("api/jobimport")] +[Authorize(AuthenticationSchemes = "local")] public sealed class JobImportController : ControllerBase { private readonly JobImportService _import; diff --git a/JobTrackerApi/Controllers/RulesController.cs b/JobTrackerApi/Controllers/RulesController.cs index 5db0e15..6b2ba2c 100644 --- a/JobTrackerApi/Controllers/RulesController.cs +++ b/JobTrackerApi/Controllers/RulesController.cs @@ -1,4 +1,5 @@ -using Microsoft.AspNetCore.Mvc; +using Microsoft.AspNetCore.Authorization; +using Microsoft.AspNetCore.Mvc; using Microsoft.EntityFrameworkCore; using JobTrackerApi.Data; using JobTrackerApi.Models; @@ -6,7 +7,11 @@ using JobTrackerApi.Models; namespace JobTrackerApi.Controllers { [ApiController] + // Explicitly authorized. These endpoints are all tenant-scoped user data, so they must not + // depend on the Auth:Require fallback policy being switched on: a deployment that lost that flag + // would otherwise serve them anonymously. docs/production-readiness-review.md. [Route("api/rules")] + [Authorize(AuthenticationSchemes = "local")] public class RulesController : ControllerBase { private readonly JobTrackerContext _db;