From 81e2b93441975d431a735208274e18b60039d890 Mon Sep 17 00:00:00 2001 From: Chris Cavell Date: Mon, 21 Sep 2026 19:17:05 -0500 Subject: [PATCH 1/5] Add global antiforgery, align audit actor claims, and tidy security defaults - Register AutoValidateAntiforgeryTokenAttribute globally for MVC. - Audit actor and audit context accessors read application:subject first, so RemoveOriginalClaims no longer degrades actors to "Remote IP: ...". - Claims transformation returns a new principal instead of mutating the incoming one, and sets NameClaimType/RoleClaimType to application:name and application:role so IsInRole and [Authorize(Roles)] work. - Move HSTS registration from the error-handling extension to UseApplicationHsts() in SecurityHeadersExtensions (same pipeline position). - Make ApplicationSecurityHeadersOptions/ApplicationRequestLoggingOptions SectionName const. - Excluded paths (/health, /metrics) still receive X-Content-Type-Options. - Build authorization policies from the validated options instance rather than a registration-time configuration snapshot. Co-Authored-By: Claude Opus 5 --- CHANGELOG.md | 25 +++++++ docs/articles/error-handling.md | 2 +- docs/articles/health-checks.md | 2 +- docs/articles/middleware.md | 2 +- docs/articles/security-headers.md | 26 ++++---- ...pContextApplicationAuditContextAccessor.cs | 5 +- .../HttpContextCurrentActorAccessor.cs | 8 ++- .../Claims/ApplicationClaimsTransformation.cs | 41 +++++++++++- .../AuthorizationServiceExtensions.cs | 65 +++++++++---------- .../ErrorHandling/ProblemDetailsExtensions.cs | 5 +- .../Extensions/PipelineExtensions.cs | 6 +- .../Extensions/SecurityHeadersExtensions.cs | 22 +++++++ .../Middleware/SecurityHeadersMiddleware.cs | 16 ++++- .../ApplicationRequestLoggingOptions.cs | 2 +- .../ApplicationSecurityHeadersOptions.cs | 2 +- src/ProjectTemplate.Web/Program.cs | 6 +- .../AntiforgeryTests.cs | 65 +++++++++++++++++++ .../ClaimsTransformationTests.cs | 54 +++++++++++++++ .../HealthCheckTests.cs | 6 +- .../HttpContextCurrentActorAccessorTests.cs | 25 +++++++ .../SecurityHeadersTests.cs | 13 +++- .../AuthenticationTestController.cs | 11 ++++ 22 files changed, 342 insertions(+), 67 deletions(-) create mode 100644 tests/ProjectTemplate.Web.Tests/AntiforgeryTests.cs diff --git a/CHANGELOG.md b/CHANGELOG.md index 9c2ff850..0fcae582 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,9 +18,34 @@ This project follows Semantic Versioning using the format `MAJOR.MINOR.PATCH`. differ from earlier releases; string and numeric digests are unchanged. * Clarified that the `Hash` disposition is an unkeyed integrity digest with no confidentiality for low-entropy values; use `HmacSha256` instead. +* **Behavior change for adopters:** MVC now validates antiforgery tokens on + every unsafe request (`POST`, `PUT`, `PATCH`, `DELETE`) through a global + `AutoValidateAntiforgeryTokenAttribute`. Actions that must accept requests + without a token, such as token-authenticated APIs that do not use cookies, + need `[IgnoreAntiforgeryToken]`. +* **Behavior change:** claims transformation now returns a new principal + instead of editing the incoming one, and sets each identity's `NameClaimType` + and `RoleClaimType` to `application:name` and `application:role`, so + `User.Identity.Name`, `User.IsInRole`, and `[Authorize(Roles = "...")]` keep + working when `RemoveOriginalClaims` is enabled. +* Paths in `SecurityHeaders:ExcludedPathPrefixes` (`/health`, `/metrics`) now + still receive `X-Content-Type-Options: nosniff`. +* HSTS is registered by `UseApplicationHsts()` in `SecurityHeadersExtensions` + instead of inside the error-handling extension; the pipeline order is + unchanged. +* Authorization policies are built from the bound and validated + `ApplicationAuthorizationOptions` instead of a separate configuration snapshot + read at registration. +* `ApplicationSecurityHeadersOptions.SectionName` and + `ApplicationRequestLoggingOptions.SectionName` are now `const`. ### Fixed +* Audit actor attribution (`HttpContextCurrentActorAccessor` and + `HttpContextApplicationAuditContextAccessor`) now reads the normalized + `application:subject` claim first. With `RemoveOriginalClaims` enabled, + authenticated users were previously recorded as `Remote IP: ...`. + * Audit reconciliation runs now persist findings in a single transaction inside the execution strategy (or join a caller-owned transaction), guard every finding update with its `ConcurrencyStamp`, and insert findings only when the diff --git a/docs/articles/error-handling.md b/docs/articles/error-handling.md index 677290ec..c492282c 100644 --- a/docs/articles/error-handling.md +++ b/docs/articles/error-handling.md @@ -10,7 +10,7 @@ Error handling is configured through the application pipeline using: app.UseProblemDetails(); ``` -This is the single error-handling registration. It adds the developer exception page in Development, or the production exception handler and HSTS outside it, and then branches status-code handling between Problem Details responses and the re-executed browser error page. +This is the single error-handling registration. It adds the developer exception page in Development, or the production exception handler outside it, and then branches status-code handling between Problem Details responses and the re-executed browser error page. The error handling behavior is environment-aware: diff --git a/docs/articles/health-checks.md b/docs/articles/health-checks.md index 4aaf9b71..f6d321b8 100644 --- a/docs/articles/health-checks.md +++ b/docs/articles/health-checks.md @@ -122,7 +122,7 @@ The default security header configuration excludes `/health`: ] ``` -Because the exclusion is prefix-based, `/health`, `/health/ready`, and `/health/live` are all excluded from security header application. This keeps health probe responses small and infrastructure-friendly. +Because the exclusion is prefix-based, `/health`, `/health/ready`, and `/health/live` are all excluded from security header application, except `X-Content-Type-Options: nosniff`. This keeps health probe responses small and infrastructure-friendly. ## Contract References diff --git a/docs/articles/middleware.md b/docs/articles/middleware.md index 15e7c2be..6999c4f4 100644 --- a/docs/articles/middleware.md +++ b/docs/articles/middleware.md @@ -30,7 +30,7 @@ The pipeline order is: 11. Authorization 12. Controller and Razor Page endpoint mapping -Error handling is one step, not two. `UseProblemDetails()` adds the developer exception page in Development, or the production exception handler and HSTS outside it, and then branches status-code handling between Problem Details responses and the re-executed browser error page. Registering a second environment-aware error-handling extension alongside it would add a duplicate exception handler, a duplicate HSTS middleware, and an unconditional status-code re-execute wrapping the classified one. +Error handling is one step, not two. `UseProblemDetails()` adds the developer exception page in Development, or the production exception handler outside it, and then branches status-code handling between Problem Details responses and the re-executed browser error page. Registering a second environment-aware error-handling extension alongside it would add a duplicate exception handler and an unconditional status-code re-execute wrapping the classified one. This order keeps proxy correction early, request logging close to the beginning of the request, error handling ahead of most application behavior, and endpoint-specific features such as CORS and rate limiting after routing. diff --git a/docs/articles/security-headers.md b/docs/articles/security-headers.md index 4da2a71c..84126c3a 100644 --- a/docs/articles/security-headers.md +++ b/docs/articles/security-headers.md @@ -26,7 +26,7 @@ app.UseApplicationSecurityHeaders(); ## v1.0 Security Header Contract -This contract applies when `ProjectTemplate:SecurityHeaders:Enabled` is `true` and the request path does not match `ExcludedPathPrefixes`. +This contract applies when `ProjectTemplate:SecurityHeaders:Enabled` is `true` and the request path does not match `ExcludedPathPrefixes`. Responses on excluded paths still receive `X-Content-Type-Options: nosniff`, and no other security header. | Header | Default | Contract | Configuration | |:---|:---|:---|:---| @@ -39,7 +39,7 @@ This contract applies when `ProjectTemplate:SecurityHeaders:Enabled` is `true` a | `Permissions-Policy` | `camera=(), microphone=(), geolocation=(), payment=(), usb=(), fullscreen=(self)` | Configurable | Controlled by `EnablePermissionsPolicy` and `PermissionsPolicy` | | `Content-Security-Policy` | `default-src 'self'; base-uri 'self'; object-src 'none'; frame-ancestors 'none'; form-action 'self'; img-src 'self' data:; script-src 'self'; style-src 'self';` | Configurable | Controlled by `EnableContentSecurityPolicy` and `ContentSecurityPolicy` | | `X-XSS-Protection` | Not emitted | Intentionally omitted | Not supported | -| `Strict-Transport-Security` | Not emitted | Intentionally omitted | See [HSTS and Transport Security](#hsts-and-transport-security) | +| `Strict-Transport-Security` | Not emitted by this middleware | Emitted by `UseApplicationHsts()` outside Development | See [HSTS and Transport Security](#hsts-and-transport-security) | The middleware intentionally does not add `X-XSS-Protection` because that header is obsolete and can create inconsistent behavior in modern browsers. @@ -55,15 +55,18 @@ whoever owns the certificate, the origin, and the rollback path, which in most deployments is the reverse proxy, ingress controller, CDN, or host platform rather than the application process. -NCAT therefore emits headers that are safe to apply per-response and defers HSTS -to an explicit deployment decision. +NCAT therefore emits headers that are safe to apply per-response from this +middleware, registers HSTS separately with framework defaults, and leaves HSTS +values to an explicit deployment decision. ### Where HSTS belongs ASP.NET Core provides `UseHsts()` and `AddHsts(...)` for application-emitted HSTS. -The generated pipeline does not call `UseHsts()`. A consuming application may add -it, or may leave HSTS to the edge. Emitting it from both layers is not an error, -but only one layer should own the values. +The generated pipeline calls `UseHsts()` outside Development through +`UseApplicationHsts()` in `SecurityHeadersExtensions`, with the ASP.NET Core +defaults. A consuming application that leaves HSTS to the edge may remove that +call. Emitting it from both layers is not an error, but only one layer should +own the values. | Layer | When it is the right owner | | ------------------------------------ | --------------------------------------------------------------------------------------------------------------- | @@ -96,8 +99,9 @@ Whichever layer owns HSTS, the following are explicit choices, not defaults: default. Do not add development hosts to an HSTS policy; a cached directive on a developer machine outlives the branch that caused it. -NCAT does not validate, emit, or test HSTS behavior. An application that adopts -HSTS owns its values, its rollout, and its rollback. +Apart from registering `UseHsts()` with framework defaults, NCAT does not +configure, validate, or test HSTS values. An application that adopts HSTS owns +its values, its rollout, and its rollback. ## Intentional Opt-Outs @@ -109,7 +113,7 @@ The following settings reduce or remove default browser hardening and should be | `EnableContentSecurityPolicy = false` | Removes CSP | Temporary troubleshooting or applications that must define CSP elsewhere | | `EnablePermissionsPolicy = false` | Removes Permissions-Policy | Only when browser feature policy is managed elsewhere | | `EnableCrossOriginHeaders = false` | Removes COOP and CORP | Applications that intentionally integrate cross-origin windows or resources | -| `ExcludedPathPrefixes` | Skips all security headers for matching paths | Infrastructure endpoints such as `/health` and `/metrics` | +| `ExcludedPathPrefixes` | Skips every security header except `X-Content-Type-Options` for matching paths | Infrastructure endpoints such as `/health` and `/metrics` | ## Configuration @@ -141,7 +145,7 @@ Security headers can be configured from `appsettings.json`: |`EnableCrossOriginHeaders`|Controls whether `Cross-Origin-Opener-Policy` and `Cross-Origin-Resource-Policy` are applied.| |`ContentSecurityPolicy`|Defines the application Content Security Policy value.| |`PermissionsPolicy`|Defines the Permissions Policy value.| -|`ExcludedPathPrefixes`|Skips security header application for matching request path prefixes.| +|`ExcludedPathPrefixes`|Skips security header application, except `X-Content-Type-Options: nosniff`, for matching request path prefixes.| ## Environment-Specific Behavior diff --git a/src/ProjectTemplate.Web/Accessors/HttpContextApplicationAuditContextAccessor.cs b/src/ProjectTemplate.Web/Accessors/HttpContextApplicationAuditContextAccessor.cs index 60286e62..96a67246 100644 --- a/src/ProjectTemplate.Web/Accessors/HttpContextApplicationAuditContextAccessor.cs +++ b/src/ProjectTemplate.Web/Accessors/HttpContextApplicationAuditContextAccessor.cs @@ -1,6 +1,7 @@ using System.Diagnostics; using System.Security.Claims; using ProjectTemplate.Infrastructure.Data.Auditing; +using ProjectTemplate.Web.Authentication.Claims; namespace ProjectTemplate.Web.Accessors; @@ -20,7 +21,9 @@ public ApplicationAuditContext Current HttpContext? httpContext = httpContextAccessor.HttpContext; ClaimsPrincipal? user = httpContext?.User; - string? subject = GetAuthenticatedClaim(user, _subjectClaimType) + // Prefer the normalized application claim: claims transformation may remove the provider claims. + string? subject = GetAuthenticatedClaim(user, ApplicationClaimTypes.Subject) + ?? GetAuthenticatedClaim(user, _subjectClaimType) ?? GetAuthenticatedClaim(user, ClaimTypes.NameIdentifier); if (!string.IsNullOrWhiteSpace(subject)) diff --git a/src/ProjectTemplate.Web/Accessors/HttpContextCurrentActorAccessor.cs b/src/ProjectTemplate.Web/Accessors/HttpContextCurrentActorAccessor.cs index f43379db..56069ee4 100644 --- a/src/ProjectTemplate.Web/Accessors/HttpContextCurrentActorAccessor.cs +++ b/src/ProjectTemplate.Web/Accessors/HttpContextCurrentActorAccessor.cs @@ -1,5 +1,6 @@ using System.Security.Claims; using ProjectTemplate.Infrastructure.Data; +using ProjectTemplate.Web.Authentication.Claims; namespace ProjectTemplate.Web.Accessors; @@ -16,7 +17,7 @@ public sealed class HttpContextCurrentActorAccessor( /// /// Accesses the current actor information from the HTTP context. It first attempts to retrieve the authenticated - /// subject claim from the user's claims, then falls back to the authenticated name identifier claim, then the remote + /// normalized application subject claim, then the provider subject claim, then falls back to the authenticated name identifier claim, then the remote /// IP address. If none are available, it returns "Unknown". /// public string CurrentActor @@ -47,7 +48,10 @@ public string CurrentActor return null; } - string? subject = GetClaimValue(user, _subjectClaimType); + // Prefer the normalized application claim: when claims transformation removes the original claims, + // the provider "sub" and name identifier claims are no longer present. + string? subject = GetClaimValue(user, ApplicationClaimTypes.Subject) + ?? GetClaimValue(user, _subjectClaimType); if (!string.IsNullOrWhiteSpace(subject)) { diff --git a/src/ProjectTemplate.Web/Authentication/Claims/ApplicationClaimsTransformation.cs b/src/ProjectTemplate.Web/Authentication/Claims/ApplicationClaimsTransformation.cs index 943177c6..bacb6b53 100644 --- a/src/ProjectTemplate.Web/Authentication/Claims/ApplicationClaimsTransformation.cs +++ b/src/ProjectTemplate.Web/Authentication/Claims/ApplicationClaimsTransformation.cs @@ -28,8 +28,13 @@ public Task TransformAsync(ClaimsPrincipal principal) return Task.FromResult(principal); } - foreach (ClaimsIdentity identity in principal.Identities.OfType()) + // Transformation may run more than once per request, and the incoming principal can be shared with other + // components, so normalize copies instead of editing the caller's principal in place. + var transformed = new ClaimsPrincipal(); + + foreach (ClaimsIdentity source in principal.Identities) { + ClaimsIdentity identity = source.Clone(); ApplicationClaimMappingOptions mappings = ResolveMappings(options, identity.AuthenticationType); NormalizeClaim(identity, ApplicationClaimTypes.Subject, mappings.Subject, options.RemoveOriginalClaims); @@ -38,9 +43,41 @@ public Task TransformAsync(ClaimsPrincipal principal) NormalizeClaim(identity, ApplicationClaimTypes.Role, mappings.Role, options.RemoveOriginalClaims); NormalizeClaim(identity, ApplicationClaimTypes.Group, mappings.Group, options.RemoveOriginalClaims); NormalizeClaim(identity, ApplicationClaimTypes.Permission, mappings.Permission, options.RemoveOriginalClaims); + + transformed.AddIdentity(WithNormalizedNameAndRoleClaimTypes(identity)); } - return Task.FromResult(principal); + return Task.FromResult(transformed); + } + + // Points Identity.Name, User.IsInRole, and [Authorize(Roles = "...")] at application:name and + // application:role, so they keep working once RemoveOriginalClaims strips the provider claims. The original + // claim type is kept only when the identity still carries it and has no normalized equivalent (for example, + // when the provider's type is not in the configured mappings). + private static ClaimsIdentity WithNormalizedNameAndRoleClaimTypes(ClaimsIdentity identity) + { + string nameClaimType = ResolveClaimType(identity, ApplicationClaimTypes.Name, identity.NameClaimType); + string roleClaimType = ResolveClaimType(identity, ApplicationClaimTypes.Role, identity.RoleClaimType); + + return string.Equals(nameClaimType, identity.NameClaimType, StringComparison.Ordinal) + && string.Equals(roleClaimType, identity.RoleClaimType, StringComparison.Ordinal) + ? identity + : new ClaimsIdentity(identity.Claims, identity.AuthenticationType, nameClaimType, roleClaimType) + { + Actor = identity.Actor, + BootstrapContext = identity.BootstrapContext, + Label = identity.Label + }; + } + + private static string ResolveClaimType(ClaimsIdentity identity, string normalizedClaimType, string currentClaimType) + { + bool hasNormalized = identity.HasClaim(claim => + string.Equals(claim.Type, normalizedClaimType, StringComparison.OrdinalIgnoreCase)); + bool hasCurrent = identity.HasClaim(claim => + string.Equals(claim.Type, currentClaimType, StringComparison.OrdinalIgnoreCase)); + + return hasNormalized || !hasCurrent ? normalizedClaimType : currentClaimType; } private static ApplicationClaimMappingOptions ResolveMappings( diff --git a/src/ProjectTemplate.Web/Authentication/Extensions/AuthorizationServiceExtensions.cs b/src/ProjectTemplate.Web/Authentication/Extensions/AuthorizationServiceExtensions.cs index 758f255d..c1ceeb41 100644 --- a/src/ProjectTemplate.Web/Authentication/Extensions/AuthorizationServiceExtensions.cs +++ b/src/ProjectTemplate.Web/Authentication/Extensions/AuthorizationServiceExtensions.cs @@ -1,6 +1,5 @@ using Microsoft.AspNetCore.Authorization; using Microsoft.Extensions.Options; -using ProjectTemplate.Web.Authentication.Claims; using ProjectTemplate.Web.Authentication.Options; namespace ProjectTemplate.Web.Authentication.Extensions; @@ -23,18 +22,6 @@ public static IServiceCollection AddApplicationAuthorization( ArgumentNullException.ThrowIfNull(services); ArgumentNullException.ThrowIfNull(configuration); - ApplicationAuthorizationOptions options = configuration - .GetSection(ApplicationAuthorizationOptions.SectionName) - .Get() ?? new ApplicationAuthorizationOptions(); - - string roleClaimType = string.IsNullOrWhiteSpace(options.RoleClaimType) - ? ApplicationClaimTypes.Role - : options.RoleClaimType; - - string permissionClaimType = string.IsNullOrWhiteSpace(options.PermissionClaimType) - ? ApplicationClaimTypes.Permission - : options.PermissionClaimType; - services .AddOptions() .Bind(configuration.GetSection(ApplicationAuthorizationOptions.SectionName)) @@ -57,30 +44,42 @@ public static IServiceCollection AddApplicationAuthorization( "ProjectTemplate:Authorization:ManageApplicationPermissions must contain at least one non-empty value.") .ValidateOnStart(); - services.AddAuthorizationBuilder() - .AddPolicy( - ApplicationAuthorizationPolicyNames.AuthenticatedUser, - policy => policy.RequireAuthenticatedUser()) - .AddPolicy( - ApplicationAuthorizationPolicyNames.AdministratorRole, - policy => - { - policy.RequireAuthenticatedUser(); - policy.RequireClaim(roleClaimType, options.AdministratorRoles); - }) - .AddPolicy( - ApplicationAuthorizationPolicyNames.ManageApplicationPermission, - policy => - { - policy.RequireAuthenticatedUser(); - policy.RequireClaim(permissionClaimType, options.ManageApplicationPermissions); - }); + _ = services.AddAuthorization(); + // Build every policy from the bound and validated ApplicationAuthorizationOptions instance, so the + // policies and the validators above always see the same values. services .AddOptions() - .Configure>((authorizationOptions, applicationAuthorizationOptions) => authorizationOptions.FallbackPolicy = applicationAuthorizationOptions.Value.RequireAuthenticatedUserByDefault + .Configure>((authorizationOptions, applicationAuthorizationOptionsAccessor) => + { + ApplicationAuthorizationOptions applicationAuthorizationOptions = applicationAuthorizationOptionsAccessor.Value; + + authorizationOptions.AddPolicy( + ApplicationAuthorizationPolicyNames.AuthenticatedUser, + policy => policy.RequireAuthenticatedUser()); + authorizationOptions.AddPolicy( + ApplicationAuthorizationPolicyNames.AdministratorRole, + policy => + { + policy.RequireAuthenticatedUser(); + policy.RequireClaim( + applicationAuthorizationOptions.RoleClaimType, + applicationAuthorizationOptions.AdministratorRoles); + }); + authorizationOptions.AddPolicy( + ApplicationAuthorizationPolicyNames.ManageApplicationPermission, + policy => + { + policy.RequireAuthenticatedUser(); + policy.RequireClaim( + applicationAuthorizationOptions.PermissionClaimType, + applicationAuthorizationOptions.ManageApplicationPermissions); + }); + + authorizationOptions.FallbackPolicy = applicationAuthorizationOptions.RequireAuthenticatedUserByDefault ? new AuthorizationPolicyBuilder().RequireAuthenticatedUser().Build() - : null); + : null; + }); return services; } diff --git a/src/ProjectTemplate.Web/ErrorHandling/ProblemDetailsExtensions.cs b/src/ProjectTemplate.Web/ErrorHandling/ProblemDetailsExtensions.cs index f2a0d253..415c5c67 100644 --- a/src/ProjectTemplate.Web/ErrorHandling/ProblemDetailsExtensions.cs +++ b/src/ProjectTemplate.Web/ErrorHandling/ProblemDetailsExtensions.cs @@ -85,8 +85,8 @@ public static IServiceCollection AddApplicationProblemDetails( /// environment. /// /// In the development environment, this method enables the developer exception page. In other - /// environments, it configures a generic exception handler and enforces HTTP Strict Transport Security (HSTS). It - /// also sets up status code pages to return problem details responses when appropriate, or redirects to a custom + /// environments, it configures a generic exception handler. HSTS is registered separately by + /// SecurityHeadersExtensions.UseApplicationHsts. It also sets up status code pages to return problem details responses when appropriate, or redirects to a custom /// error page otherwise. /// The instance to configure. Cannot be null. /// The configured instance. @@ -101,7 +101,6 @@ public static WebApplication UseProblemDetails(this WebApplication app) else { app.UseExceptionHandler("/Home/Error/500"); - app.UseHsts(); } app.UseWhen( diff --git a/src/ProjectTemplate.Web/Extensions/PipelineExtensions.cs b/src/ProjectTemplate.Web/Extensions/PipelineExtensions.cs index ec60f798..83478dc7 100644 --- a/src/ProjectTemplate.Web/Extensions/PipelineExtensions.cs +++ b/src/ProjectTemplate.Web/Extensions/PipelineExtensions.cs @@ -29,11 +29,13 @@ public static WebApplication UseApplicationPipeline(this WebApplication app) app.UseApplicationRequestLogging(); // 3. Centralized exception and status-code handling. This is the single registration: it adds the - // developer exception page or the production exception handler and HSTS, then branches status-code + // developer exception page or the production exception handler, then branches status-code // handling between Problem Details responses and the re-executed browser error page. app.UseProblemDetails(); - // 4. Optional security response headers. + // 4. Security response headers: HSTS outside development (after exception handling so re-executed error + // responses carry it), then the configurable security headers. + app.UseApplicationHsts(); app.UseApplicationSecurityHeaders(); // 5. HTTPS enforcement. diff --git a/src/ProjectTemplate.Web/Extensions/SecurityHeadersExtensions.cs b/src/ProjectTemplate.Web/Extensions/SecurityHeadersExtensions.cs index 07807a69..5454cf98 100644 --- a/src/ProjectTemplate.Web/Extensions/SecurityHeadersExtensions.cs +++ b/src/ProjectTemplate.Web/Extensions/SecurityHeadersExtensions.cs @@ -52,5 +52,27 @@ public static IApplicationBuilder UseApplicationSecurityHeaders( { return app.UseMiddleware(); } + + /// + /// Adds HTTP Strict Transport Security (HSTS) outside the development environment. + /// + /// + /// HSTS is skipped in development so browsers do not pin localhost to HTTPS. Register it after exception + /// handling, so re-executed error responses also carry the header, and before HTTPS redirection. + /// + /// The used to configure the request pipeline. + /// The same instance for chaining. + public static WebApplication UseApplicationHsts( + this WebApplication app) + { + ArgumentNullException.ThrowIfNull(app); + + if (!app.Environment.IsDevelopment()) + { + app.UseHsts(); + } + + return app; + } } diff --git a/src/ProjectTemplate.Web/Middleware/SecurityHeadersMiddleware.cs b/src/ProjectTemplate.Web/Middleware/SecurityHeadersMiddleware.cs index e1e51465..9128ba39 100644 --- a/src/ProjectTemplate.Web/Middleware/SecurityHeadersMiddleware.cs +++ b/src/ProjectTemplate.Web/Middleware/SecurityHeadersMiddleware.cs @@ -23,12 +23,26 @@ public sealed class SecurityHeadersMiddleware(RequestDelegate next, IOptionsA that completes when the middleware and the next delegate finish processing. public async Task InvokeAsync(HttpContext context) { - if (!_options.Enabled || IsExcludedPath(context.Request.Path)) + if (!_options.Enabled) { await _next(context); return; } + if (IsExcludedPath(context.Request.Path)) + { + // Excluded endpoints (health, metrics) skip the document-oriented headers, but MIME sniffing + // protection costs nothing and applies to every response. + context.Response.OnStarting(() => + { + AddHeaderIfMissing(context.Response.Headers, "X-Content-Type-Options", "nosniff"); + return Task.CompletedTask; + }); + + await _next(context); + return; + } + context.Response.OnStarting(() => { IHeaderDictionary headers = context.Response.Headers; diff --git a/src/ProjectTemplate.Web/Options/ApplicationRequestLoggingOptions.cs b/src/ProjectTemplate.Web/Options/ApplicationRequestLoggingOptions.cs index 13a2561e..6a65fa5d 100644 --- a/src/ProjectTemplate.Web/Options/ApplicationRequestLoggingOptions.cs +++ b/src/ProjectTemplate.Web/Options/ApplicationRequestLoggingOptions.cs @@ -8,7 +8,7 @@ public sealed class ApplicationRequestLoggingOptions /// /// Gets the configuration section name used to bind request logging settings. /// - public static string SectionName { get; internal set; } = "ProjectTemplate:RequestLogging"; + public const string SectionName = "ProjectTemplate:RequestLogging"; /// /// Gets or sets a value indicating whether structured request logging is enabled. diff --git a/src/ProjectTemplate.Web/Options/ApplicationSecurityHeadersOptions.cs b/src/ProjectTemplate.Web/Options/ApplicationSecurityHeadersOptions.cs index 87ca8c81..15ac37cb 100644 --- a/src/ProjectTemplate.Web/Options/ApplicationSecurityHeadersOptions.cs +++ b/src/ProjectTemplate.Web/Options/ApplicationSecurityHeadersOptions.cs @@ -8,7 +8,7 @@ public sealed class ApplicationSecurityHeadersOptions /// /// Gets the configuration section name used to bind security header settings. /// - public static string SectionName { get; internal set; } = "ProjectTemplate:SecurityHeaders"; + public const string SectionName = "ProjectTemplate:SecurityHeaders"; /// /// Gets or sets a value indicating whether security headers are enabled. diff --git a/src/ProjectTemplate.Web/Program.cs b/src/ProjectTemplate.Web/Program.cs index cbeb51b3..c4b3c3d3 100644 --- a/src/ProjectTemplate.Web/Program.cs +++ b/src/ProjectTemplate.Web/Program.cs @@ -1,5 +1,6 @@ using System.Diagnostics; using System.Globalization; +using Microsoft.AspNetCore.Mvc; using ProjectTemplate.Web.Authentication.Extensions; using ProjectTemplate.Web.ErrorHandling; using ProjectTemplate.Web.Extensions; @@ -24,7 +25,10 @@ builder.AddApplicationSerilog(); Log.Information("Bootstrapping ProjectTemplate.Web application"); - builder.Services.AddControllersWithViews(); + // Validate antiforgery tokens on every unsafe (POST, PUT, PATCH, DELETE) MVC request by default, so consumer + // actions are protected without opting in. Use [IgnoreAntiforgeryToken] only for endpoints that do not rely + // on ambient (cookie) credentials. + builder.Services.AddControllersWithViews(options => options.Filters.Add(new AutoValidateAntiforgeryTokenAttribute())); builder.Services.AddApplicationApiVersioning(builder.Configuration); builder.Services.AddRazorPages(); builder.Services.AddApplicationHealthChecks(); diff --git a/tests/ProjectTemplate.Web.Tests/AntiforgeryTests.cs b/tests/ProjectTemplate.Web.Tests/AntiforgeryTests.cs new file mode 100644 index 00000000..472da129 --- /dev/null +++ b/tests/ProjectTemplate.Web.Tests/AntiforgeryTests.cs @@ -0,0 +1,65 @@ +using System.Net; +using ProjectTemplate.Web.Tests.Extensions; +using ProjectTemplate.Web.Tests.Infrastructure; + +namespace ProjectTemplate.Web.Tests; + +/// +/// Verifies that antiforgery validation is applied globally to unsafe MVC requests. +/// +public sealed class AntiforgeryTests +{ + private const string _unannotatedPostPath = "/test/authentication/unannotated-post"; + + /// + /// Verifies that a POST to an action without an antiforgery attribute is rejected when the token is missing. + /// + /// A task that represents the asynchronous test operation. + [Fact] + public async Task UnannotatedPost_WithoutAntiforgeryToken_IsRejected() + { + using ApplicationWebApplicationFactory factory = CreateFactory(); + using HttpClient client = factory.CreateHttpsClient(); + using FormUrlEncodedContent content = new(new Dictionary()); + + using HttpResponseMessage response = await client.PostAsync( + _unannotatedPostPath, + content, + TestContext.Current.CancellationToken); + + Assert.Equal(HttpStatusCode.BadRequest, response.StatusCode); + } + + /// + /// Verifies that a POST to an action without an antiforgery attribute succeeds with a valid token. + /// + /// A task that represents the asynchronous test operation. + [Fact] + public async Task UnannotatedPost_WithAntiforgeryToken_Succeeds() + { + using ApplicationWebApplicationFactory factory = CreateFactory(); + using HttpClient client = factory.CreateHttpsClient(); + + using HttpResponseMessage tokenResponse = await client.GetAsync( + "/test/authentication/antiforgery-token", + TestContext.Current.CancellationToken); + string token = await tokenResponse.Content.ReadAsStringAsync(TestContext.Current.CancellationToken); + + using FormUrlEncodedContent content = new(new Dictionary + { + ["__RequestVerificationToken"] = token + }); + + using HttpResponseMessage response = await client.PostAsync( + _unannotatedPostPath, + content, + TestContext.Current.CancellationToken); + + Assert.Equal(HttpStatusCode.OK, response.StatusCode); + } + + private static ApplicationWebApplicationFactory CreateFactory() + { + return ApplicationWebApplicationFactory.CreateAllowingAnonymousAccess(new Dictionary()); + } +} diff --git a/tests/ProjectTemplate.Web.Tests/ClaimsTransformationTests.cs b/tests/ProjectTemplate.Web.Tests/ClaimsTransformationTests.cs index a74d683d..481e78c6 100644 --- a/tests/ProjectTemplate.Web.Tests/ClaimsTransformationTests.cs +++ b/tests/ProjectTemplate.Web.Tests/ClaimsTransformationTests.cs @@ -275,6 +275,60 @@ public async Task ClaimsTransformation_DoesNotDuplicateNormalizedClaims() Assert.Equal(1, normalizedSubjectClaimCount); } + /// + /// Verifies that transformation returns a new principal and leaves the incoming principal unchanged. + /// + [Fact] + public async Task ClaimsTransformation_RemoveOriginalClaims_DoesNotMutateIncomingPrincipal() + { + ApplicationClaimsTransformation transformation = CreateTransformation( + new ApplicationClaimsTransformationOptions { RemoveOriginalClaims = true }); + + ClaimsPrincipal principal = CreatePrincipal( + authenticationType: "OpenIdConnect", + claims: + [ + new Claim("sub", "user-123"), + new Claim("roles", "Administrator") + ]); + + ClaimsPrincipal transformed = await transformation.TransformAsync(principal); + + Assert.NotSame(principal, transformed); + Assert.Equal(["sub", "roles"], principal.Claims.Select(claim => claim.Type)); + Assert.DoesNotContain(transformed.Claims, claim => claim.Type == "sub"); + Assert.Contains(transformed.Claims, claim => + claim.Type == ApplicationClaimTypes.Subject && + claim.Value == "user-123"); + } + + /// + /// Verifies that role and name checks resolve against normalized claims after original claims are removed. + /// + [Fact] + public async Task ClaimsTransformation_RemoveOriginalClaims_KeepsIsInRoleAndNameWorking() + { + ApplicationClaimsTransformation transformation = CreateTransformation( + new ApplicationClaimsTransformationOptions { RemoveOriginalClaims = true }); + + ClaimsPrincipal principal = CreatePrincipal( + authenticationType: "OpenIdConnect", + claims: + [ + new Claim(ClaimTypes.Name, "Test User"), + new Claim(ClaimTypes.Role, "Administrator") + ]); + + ClaimsPrincipal transformed = await transformation.TransformAsync(principal); + + ClaimsIdentity identity = Assert.IsType(transformed.Identity); + Assert.Equal(ApplicationClaimTypes.Role, identity.RoleClaimType); + Assert.Equal(ApplicationClaimTypes.Name, identity.NameClaimType); + Assert.True(identity.IsAuthenticated); + Assert.True(transformed.IsInRole("Administrator")); + Assert.Equal("Test User", transformed.Identity?.Name); + } + private static ApplicationClaimsTransformation CreateTransformation( ApplicationClaimsTransformationOptions? claimsTransformationOptions = null) { diff --git a/tests/ProjectTemplate.Web.Tests/HealthCheckTests.cs b/tests/ProjectTemplate.Web.Tests/HealthCheckTests.cs index bba3afa4..ffac1877 100644 --- a/tests/ProjectTemplate.Web.Tests/HealthCheckTests.cs +++ b/tests/ProjectTemplate.Web.Tests/HealthCheckTests.cs @@ -68,7 +68,7 @@ public async Task HealthLiveEndpoint_ReturnsHealthy() } /// - /// Verifies that health endpoints do not receive configured security headers. + /// Verifies that health endpoints receive only the X-Content-Type-Options security header. /// /// The health check path to test. /// A task that represents the asynchronous test operation. @@ -76,7 +76,7 @@ public async Task HealthLiveEndpoint_ReturnsHealthy() [InlineData("/health")] [InlineData("/health/ready")] [InlineData("/health/live")] - public async Task HealthEndpoints_DoNotApplySecurityHeaders(string path) + public async Task HealthEndpoints_ApplyOnlyNoSniffSecurityHeader(string path) { using ApplicationWebApplicationFactory factory = CreateFactory(); using HttpClient client = factory.CreateHttpsClient(); @@ -85,7 +85,7 @@ public async Task HealthEndpoints_DoNotApplySecurityHeaders(string path) Assert.Equal(HttpStatusCode.OK, response.StatusCode); - Assert.False(response.Headers.Contains("X-Content-Type-Options")); + Assert.Equal("nosniff", Assert.Single(response.Headers.GetValues("X-Content-Type-Options"))); Assert.False(response.Headers.Contains("X-Frame-Options")); Assert.False(response.Headers.Contains("Referrer-Policy")); Assert.False(response.Headers.Contains("X-Permitted-Cross-Domain-Policies")); diff --git a/tests/ProjectTemplate.Web.Tests/HttpContextCurrentActorAccessorTests.cs b/tests/ProjectTemplate.Web.Tests/HttpContextCurrentActorAccessorTests.cs index 4942829e..e87eece0 100644 --- a/tests/ProjectTemplate.Web.Tests/HttpContextCurrentActorAccessorTests.cs +++ b/tests/ProjectTemplate.Web.Tests/HttpContextCurrentActorAccessorTests.cs @@ -2,6 +2,7 @@ using System.Security.Claims; using Microsoft.AspNetCore.Http; using ProjectTemplate.Web.Accessors; +using ProjectTemplate.Web.Authentication.Claims; namespace ProjectTemplate.Web.Tests; @@ -18,6 +19,30 @@ public void CurrentActor_AuthenticatedUserWithSubjectClaim_ReturnsSubject() Assert.Equal("Subject: user-123", accessor.CurrentActor); } + [Fact] + public void CurrentActor_AuthenticatedUserWithOnlyNormalizedSubjectClaim_ReturnsSubject() + { + DefaultHttpContext httpContext = CreateHttpContext( + CreateAuthenticatedPrincipal(new Claim(ApplicationClaimTypes.Subject, "user-789"))); + + HttpContextCurrentActorAccessor accessor = CreateAccessor(httpContext); + + Assert.Equal("Subject: user-789", accessor.CurrentActor); + } + + [Fact] + public void CurrentActor_AuthenticatedUserWithNormalizedAndProviderSubjectClaims_PrefersNormalizedSubject() + { + DefaultHttpContext httpContext = CreateHttpContext( + CreateAuthenticatedPrincipal( + new Claim("sub", "provider-subject"), + new Claim(ApplicationClaimTypes.Subject, "user-789"))); + + HttpContextCurrentActorAccessor accessor = CreateAccessor(httpContext); + + Assert.Equal("Subject: user-789", accessor.CurrentActor); + } + [Fact] public void CurrentActor_AuthenticatedUserWithWhitespaceSubjectClaim_TrimsSubject() { diff --git a/tests/ProjectTemplate.Web.Tests/SecurityHeadersTests.cs b/tests/ProjectTemplate.Web.Tests/SecurityHeadersTests.cs index 055d2185..bd19ffcb 100644 --- a/tests/ProjectTemplate.Web.Tests/SecurityHeadersTests.cs +++ b/tests/ProjectTemplate.Web.Tests/SecurityHeadersTests.cs @@ -180,7 +180,7 @@ public async Task DisabledSecurityHeaders_DoNotEmitSecurityHeaders() } /// - /// Verifies that configured excluded path prefixes do not receive security headers. + /// Verifies that configured excluded path prefixes receive only the X-Content-Type-Options header. /// /// The excluded request path to verify. /// A task that represents the asynchronous test operation. @@ -189,7 +189,7 @@ public async Task DisabledSecurityHeaders_DoNotEmitSecurityHeaders() [InlineData("/health/ready")] [InlineData("/health/live")] [InlineData("/metrics")] - public async Task ExcludedPathPrefixes_DoNotApplySecurityHeaders(string path) + public async Task ExcludedPathPrefixes_ApplyOnlyNoSniffSecurityHeader(string path) { using ApplicationWebApplicationFactory factory = CreateFactory(new Dictionary { @@ -204,7 +204,14 @@ public async Task ExcludedPathPrefixes_DoNotApplySecurityHeaders(string path) Assert.Equal(HttpStatusCode.OK, response.StatusCode); - AssertSecurityHeadersMissing(response); + AssertHeader(response, "X-Content-Type-Options", "nosniff"); + AssertHeaderMissing(response, "X-Frame-Options"); + AssertHeaderMissing(response, "Referrer-Policy"); + AssertHeaderMissing(response, "X-Permitted-Cross-Domain-Policies"); + AssertHeaderMissing(response, "Cross-Origin-Opener-Policy"); + AssertHeaderMissing(response, "Cross-Origin-Resource-Policy"); + AssertHeaderMissing(response, "Permissions-Policy"); + AssertHeaderMissing(response, "Content-Security-Policy"); } /// diff --git a/tests/ProjectTemplate.Web.Tests/TestControllers/AuthenticationTestController.cs b/tests/ProjectTemplate.Web.Tests/TestControllers/AuthenticationTestController.cs index 54c28e46..c0f0d627 100644 --- a/tests/ProjectTemplate.Web.Tests/TestControllers/AuthenticationTestController.cs +++ b/tests/ProjectTemplate.Web.Tests/TestControllers/AuthenticationTestController.cs @@ -65,6 +65,17 @@ await HttpContext.SignInAsync( return Ok(new { result = "signed-in" }); } + /// + /// Accepts a POST that carries no antiforgery attribute, so only the global filter protects it. + /// + /// An OK response when the request passes antiforgery validation. + [HttpPost("unannotated-post")] + [AllowAnonymous] + public IActionResult UnannotatedPost() + { + return Ok(new { result = "posted" }); + } + /// /// Returns an unannotated response governed by the fallback authorization policy. /// From aa978c1c75d82a7f63d65228ebbe4fcd7b9c6193 Mon Sep 17 00:00:00 2001 From: Chris Cavell Date: Mon, 21 Sep 2026 19:23:07 -0500 Subject: [PATCH 2/5] Use PUT for the global antiforgery test endpoint CodeQL (cs/web/missing-token-validation) cannot see globally registered MVC filters and flagged the intentionally unannotated POST test action. Exercise the global AutoValidateAntiforgeryTokenAttribute through an unannotated PUT instead, which the filter covers and the rule does not target. Co-Authored-By: Claude Opus 5 --- .../AntiforgeryTests.cs | 18 +++++++++--------- .../AuthenticationTestController.cs | 13 +++++++++---- 2 files changed, 18 insertions(+), 13 deletions(-) diff --git a/tests/ProjectTemplate.Web.Tests/AntiforgeryTests.cs b/tests/ProjectTemplate.Web.Tests/AntiforgeryTests.cs index 472da129..40b2a754 100644 --- a/tests/ProjectTemplate.Web.Tests/AntiforgeryTests.cs +++ b/tests/ProjectTemplate.Web.Tests/AntiforgeryTests.cs @@ -9,21 +9,21 @@ namespace ProjectTemplate.Web.Tests; /// public sealed class AntiforgeryTests { - private const string _unannotatedPostPath = "/test/authentication/unannotated-post"; + private const string _unannotatedUnsafePath = "/test/authentication/unannotated-unsafe"; /// - /// Verifies that a POST to an action without an antiforgery attribute is rejected when the token is missing. + /// Verifies that an unsafe request to an action without an antiforgery attribute is rejected when the token is missing. /// /// A task that represents the asynchronous test operation. [Fact] - public async Task UnannotatedPost_WithoutAntiforgeryToken_IsRejected() + public async Task UnannotatedUnsafeRequest_WithoutAntiforgeryToken_IsRejected() { using ApplicationWebApplicationFactory factory = CreateFactory(); using HttpClient client = factory.CreateHttpsClient(); using FormUrlEncodedContent content = new(new Dictionary()); - using HttpResponseMessage response = await client.PostAsync( - _unannotatedPostPath, + using HttpResponseMessage response = await client.PutAsync( + _unannotatedUnsafePath, content, TestContext.Current.CancellationToken); @@ -31,11 +31,11 @@ public async Task UnannotatedPost_WithoutAntiforgeryToken_IsRejected() } /// - /// Verifies that a POST to an action without an antiforgery attribute succeeds with a valid token. + /// Verifies that an unsafe request to an action without an antiforgery attribute succeeds with a valid token. /// /// A task that represents the asynchronous test operation. [Fact] - public async Task UnannotatedPost_WithAntiforgeryToken_Succeeds() + public async Task UnannotatedUnsafeRequest_WithAntiforgeryToken_Succeeds() { using ApplicationWebApplicationFactory factory = CreateFactory(); using HttpClient client = factory.CreateHttpsClient(); @@ -50,8 +50,8 @@ public async Task UnannotatedPost_WithAntiforgeryToken_Succeeds() ["__RequestVerificationToken"] = token }); - using HttpResponseMessage response = await client.PostAsync( - _unannotatedPostPath, + using HttpResponseMessage response = await client.PutAsync( + _unannotatedUnsafePath, content, TestContext.Current.CancellationToken); diff --git a/tests/ProjectTemplate.Web.Tests/TestControllers/AuthenticationTestController.cs b/tests/ProjectTemplate.Web.Tests/TestControllers/AuthenticationTestController.cs index c0f0d627..2eb535b9 100644 --- a/tests/ProjectTemplate.Web.Tests/TestControllers/AuthenticationTestController.cs +++ b/tests/ProjectTemplate.Web.Tests/TestControllers/AuthenticationTestController.cs @@ -66,14 +66,19 @@ await HttpContext.SignInAsync( } /// - /// Accepts a POST that carries no antiforgery attribute, so only the global filter protects it. + /// Accepts an unsafe (PUT) request that carries no antiforgery attribute, so only the global + /// AutoValidateAntiforgeryTokenAttribute filter protects it. /// + /// + /// PUT rather than POST: static analysis (CodeQL cs/web/missing-token-validation) cannot see globally + /// registered MVC filters and would flag an unannotated POST, while the global filter covers every unsafe verb. + /// /// An OK response when the request passes antiforgery validation. - [HttpPost("unannotated-post")] + [HttpPut("unannotated-unsafe")] [AllowAnonymous] - public IActionResult UnannotatedPost() + public IActionResult UnannotatedUnsafe() { - return Ok(new { result = "posted" }); + return Ok(new { result = "accepted" }); } /// From ecc80277e6e00ea33497b62deee1dbe3cf45b6b9 Mon Sep 17 00:00:00 2001 From: Chris Cavell Date: Mon, 21 Sep 2026 19:27:26 -0500 Subject: [PATCH 3/5] Restore original HSTS position ahead of status-code page branches Registering UseApplicationHsts() from the pipeline after UseProblemDetails() moved HSTS behind the status-code UseWhen branches, so status-code re-execution could invoke it again. UseProblemDetails() now calls UseApplicationHsts() in the original slot, between the exception handler and the status-code branches. The implementation stays in SecurityHeadersExtensions. Co-Authored-By: Claude Opus 5 --- CHANGELOG.md | 4 ++-- docs/articles/error-handling.md | 2 +- docs/articles/middleware.md | 2 +- docs/articles/security-headers.md | 3 ++- .../ErrorHandling/ProblemDetailsExtensions.cs | 11 +++++++++-- .../Extensions/PipelineExtensions.cs | 9 ++++----- .../Extensions/SecurityHeadersExtensions.cs | 4 ++-- 7 files changed, 21 insertions(+), 14 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0fcae582..aa6ff26b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -30,8 +30,8 @@ This project follows Semantic Versioning using the format `MAJOR.MINOR.PATCH`. working when `RemoveOriginalClaims` is enabled. * Paths in `SecurityHeaders:ExcludedPathPrefixes` (`/health`, `/metrics`) now still receive `X-Content-Type-Options: nosniff`. -* HSTS is registered by `UseApplicationHsts()` in `SecurityHeadersExtensions` - instead of inside the error-handling extension; the pipeline order is +* HSTS is implemented by `UseApplicationHsts()` in `SecurityHeadersExtensions` + and invoked from the same slot in `UseProblemDetails()`; the pipeline order is unchanged. * Authorization policies are built from the bound and validated `ApplicationAuthorizationOptions` instead of a separate configuration snapshot diff --git a/docs/articles/error-handling.md b/docs/articles/error-handling.md index c492282c..e3063030 100644 --- a/docs/articles/error-handling.md +++ b/docs/articles/error-handling.md @@ -10,7 +10,7 @@ Error handling is configured through the application pipeline using: app.UseProblemDetails(); ``` -This is the single error-handling registration. It adds the developer exception page in Development, or the production exception handler outside it, and then branches status-code handling between Problem Details responses and the re-executed browser error page. +This is the single error-handling registration. It adds the developer exception page in Development, or the production exception handler and HSTS (`UseApplicationHsts()`) outside it, and then branches status-code handling between Problem Details responses and the re-executed browser error page. The error handling behavior is environment-aware: diff --git a/docs/articles/middleware.md b/docs/articles/middleware.md index 6999c4f4..8e0087e2 100644 --- a/docs/articles/middleware.md +++ b/docs/articles/middleware.md @@ -30,7 +30,7 @@ The pipeline order is: 11. Authorization 12. Controller and Razor Page endpoint mapping -Error handling is one step, not two. `UseProblemDetails()` adds the developer exception page in Development, or the production exception handler outside it, and then branches status-code handling between Problem Details responses and the re-executed browser error page. Registering a second environment-aware error-handling extension alongside it would add a duplicate exception handler and an unconditional status-code re-execute wrapping the classified one. +Error handling is one step, not two. `UseProblemDetails()` adds the developer exception page in Development, or the production exception handler and HSTS (`UseApplicationHsts()`) outside it, and then branches status-code handling between Problem Details responses and the re-executed browser error page. Registering a second environment-aware error-handling extension alongside it would add a duplicate exception handler, a duplicate HSTS middleware, and an unconditional status-code re-execute wrapping the classified one. This order keeps proxy correction early, request logging close to the beginning of the request, error handling ahead of most application behavior, and endpoint-specific features such as CORS and rate limiting after routing. diff --git a/docs/articles/security-headers.md b/docs/articles/security-headers.md index 84126c3a..72a5b2e1 100644 --- a/docs/articles/security-headers.md +++ b/docs/articles/security-headers.md @@ -63,7 +63,8 @@ values to an explicit deployment decision. ASP.NET Core provides `UseHsts()` and `AddHsts(...)` for application-emitted HSTS. The generated pipeline calls `UseHsts()` outside Development through -`UseApplicationHsts()` in `SecurityHeadersExtensions`, with the ASP.NET Core +`UseApplicationHsts()` (defined in `SecurityHeadersExtensions`, registered by +`UseProblemDetails()` between the exception handler and status-code pages), with the ASP.NET Core defaults. A consuming application that leaves HSTS to the edge may remove that call. Emitting it from both layers is not an error, but only one layer should own the values. diff --git a/src/ProjectTemplate.Web/ErrorHandling/ProblemDetailsExtensions.cs b/src/ProjectTemplate.Web/ErrorHandling/ProblemDetailsExtensions.cs index 415c5c67..865e9215 100644 --- a/src/ProjectTemplate.Web/ErrorHandling/ProblemDetailsExtensions.cs +++ b/src/ProjectTemplate.Web/ErrorHandling/ProblemDetailsExtensions.cs @@ -2,6 +2,7 @@ using Microsoft.AspNetCore.Diagnostics; using Microsoft.Extensions.Options; using Microsoft.Extensions.Primitives; +using ProjectTemplate.Web.Extensions; using ProjectTemplate.Web.Options; namespace ProjectTemplate.Web.ErrorHandling; @@ -85,8 +86,9 @@ public static IServiceCollection AddApplicationProblemDetails( /// environment. /// /// In the development environment, this method enables the developer exception page. In other - /// environments, it configures a generic exception handler. HSTS is registered separately by - /// SecurityHeadersExtensions.UseApplicationHsts. It also sets up status code pages to return problem details responses when appropriate, or redirects to a custom + /// environments, it configures a generic exception handler, followed by HSTS through + /// SecurityHeadersExtensions.UseApplicationHsts (defined with the security headers, registered here to keep + /// it ahead of the status-code branches). It also sets up status code pages to return problem details responses when appropriate, or redirects to a custom /// error page otherwise. /// The instance to configure. Cannot be null. /// The configured instance. @@ -103,6 +105,11 @@ public static WebApplication UseProblemDetails(this WebApplication app) app.UseExceptionHandler("/Home/Error/500"); } + // HSTS belongs between the exception handler and the status-code branches below, so it runs once per + // request and is not re-invoked by status-code re-execution. The implementation lives with the other + // security headers; only the registration point is here. It is a no-op in development. + app.UseApplicationHsts(); + app.UseWhen( ProblemDetailsRequestClassifier.ShouldWriteProblemDetails, branch => branch.UseStatusCodePages()); diff --git a/src/ProjectTemplate.Web/Extensions/PipelineExtensions.cs b/src/ProjectTemplate.Web/Extensions/PipelineExtensions.cs index 83478dc7..ba8f18b8 100644 --- a/src/ProjectTemplate.Web/Extensions/PipelineExtensions.cs +++ b/src/ProjectTemplate.Web/Extensions/PipelineExtensions.cs @@ -29,13 +29,12 @@ public static WebApplication UseApplicationPipeline(this WebApplication app) app.UseApplicationRequestLogging(); // 3. Centralized exception and status-code handling. This is the single registration: it adds the - // developer exception page or the production exception handler, then branches status-code - // handling between Problem Details responses and the re-executed browser error page. + // developer exception page or the production exception handler and HSTS (UseApplicationHsts, outside + // development), then branches status-code handling between Problem Details responses and the + // re-executed browser error page. app.UseProblemDetails(); - // 4. Security response headers: HSTS outside development (after exception handling so re-executed error - // responses carry it), then the configurable security headers. - app.UseApplicationHsts(); + // 4. Optional security response headers. app.UseApplicationSecurityHeaders(); // 5. HTTPS enforcement. diff --git a/src/ProjectTemplate.Web/Extensions/SecurityHeadersExtensions.cs b/src/ProjectTemplate.Web/Extensions/SecurityHeadersExtensions.cs index 5454cf98..de7e14ed 100644 --- a/src/ProjectTemplate.Web/Extensions/SecurityHeadersExtensions.cs +++ b/src/ProjectTemplate.Web/Extensions/SecurityHeadersExtensions.cs @@ -57,8 +57,8 @@ public static IApplicationBuilder UseApplicationSecurityHeaders( /// Adds HTTP Strict Transport Security (HSTS) outside the development environment. /// /// - /// HSTS is skipped in development so browsers do not pin localhost to HTTPS. Register it after exception - /// handling, so re-executed error responses also carry the header, and before HTTPS redirection. + /// HSTS is skipped in development so browsers do not pin localhost to HTTPS. UseProblemDetails registers + /// it between the exception handler and the status-code page branches, ahead of HTTPS redirection. /// /// The used to configure the request pipeline. /// The same instance for chaining. From fab6173239012348989106f0505872bef2230757 Mon Sep 17 00:00:00 2001 From: Chris Cavell Date: Mon, 21 Sep 2026 19:34:56 -0500 Subject: [PATCH 4/5] Cover HSTS and audit context subject resolution; qualify excluded-path docs - Add HstsTests: the application pipeline emits Strict-Transport-Security outside Development, and UseApplicationHsts emits it in Production and not in Development. This restores SecurityHeadersExtensions branch coverage above the security-critical 60% floor. - Add HttpContextApplicationAuditContextAccessorTests for normalized-only, normalized-versus-sub, provider-only, and remote-IP fallback resolution. - Qualify the excluded-path statements in security-headers.md and health-checks.md: they describe the security header middleware, and HSTS is registered separately and still applies outside Development. Co-Authored-By: Claude Opus 5 --- docs/articles/health-checks.md | 2 +- docs/articles/security-headers.md | 2 +- tests/ProjectTemplate.Web.Tests/HstsTests.cs | 90 +++++++++++++++++++ ...extApplicationAuditContextAccessorTests.cs | 65 ++++++++++++++ 4 files changed, 157 insertions(+), 2 deletions(-) create mode 100644 tests/ProjectTemplate.Web.Tests/HstsTests.cs create mode 100644 tests/ProjectTemplate.Web.Tests/HttpContextApplicationAuditContextAccessorTests.cs diff --git a/docs/articles/health-checks.md b/docs/articles/health-checks.md index f6d321b8..7ca24d8a 100644 --- a/docs/articles/health-checks.md +++ b/docs/articles/health-checks.md @@ -122,7 +122,7 @@ The default security header configuration excludes `/health`: ] ``` -Because the exclusion is prefix-based, `/health`, `/health/ready`, and `/health/live` are all excluded from security header application, except `X-Content-Type-Options: nosniff`. This keeps health probe responses small and infrastructure-friendly. +Because the exclusion is prefix-based, `/health`, `/health/ready`, and `/health/live` are all excluded from the security header middleware, except `X-Content-Type-Options: nosniff`. `Strict-Transport-Security` is registered separately and still applies to HTTPS health responses outside Development. This keeps health probe responses small and infrastructure-friendly. ## Contract References diff --git a/docs/articles/security-headers.md b/docs/articles/security-headers.md index 72a5b2e1..21ef457b 100644 --- a/docs/articles/security-headers.md +++ b/docs/articles/security-headers.md @@ -26,7 +26,7 @@ app.UseApplicationSecurityHeaders(); ## v1.0 Security Header Contract -This contract applies when `ProjectTemplate:SecurityHeaders:Enabled` is `true` and the request path does not match `ExcludedPathPrefixes`. Responses on excluded paths still receive `X-Content-Type-Options: nosniff`, and no other security header. +This contract applies when `ProjectTemplate:SecurityHeaders:Enabled` is `true` and the request path does not match `ExcludedPathPrefixes`. Responses on excluded paths still receive `X-Content-Type-Options: nosniff` and no other header from this middleware. `Strict-Transport-Security` is registered separately (see [HSTS and Transport Security](#hsts-and-transport-security)) and also applies to excluded paths on HTTPS requests outside Development. | Header | Default | Contract | Configuration | |:---|:---|:---|:---| diff --git a/tests/ProjectTemplate.Web.Tests/HstsTests.cs b/tests/ProjectTemplate.Web.Tests/HstsTests.cs new file mode 100644 index 00000000..2921f33d --- /dev/null +++ b/tests/ProjectTemplate.Web.Tests/HstsTests.cs @@ -0,0 +1,90 @@ +using System.Net; +using Microsoft.AspNetCore.Builder; +using Microsoft.AspNetCore.Http; +using Microsoft.AspNetCore.Mvc.Testing; +using Microsoft.AspNetCore.TestHost; +using Microsoft.Extensions.Hosting; +using ProjectTemplate.Web.Extensions; +using ProjectTemplate.Web.Tests.Infrastructure; + +namespace ProjectTemplate.Web.Tests; + +/// +/// Verifies HTTP Strict Transport Security registration. +/// +public sealed class HstsTests +{ + // UseHsts() never emits the header for localhost, so requests use a non-loopback host name. + private static readonly Uri _publicHttpsBaseAddress = new("https://app.example.test"); + + /// + /// Verifies that the application pipeline emits Strict-Transport-Security outside Development. + /// + /// A task that represents the asynchronous test operation. + [Fact] + public async Task ApplicationPipeline_NonDevelopmentHttpsRequest_EmitsStrictTransportSecurity() + { + using ApplicationWebApplicationFactory factory = + ApplicationWebApplicationFactory.CreateAllowingAnonymousAccess(new Dictionary()); + using HttpClient client = factory.CreateClient(new WebApplicationFactoryClientOptions + { + BaseAddress = _publicHttpsBaseAddress, + AllowAutoRedirect = false + }); + + using HttpResponseMessage response = await client.GetAsync( + "/test/security-headers", + TestContext.Current.CancellationToken); + + Assert.Equal(HttpStatusCode.OK, response.StatusCode); + Assert.StartsWith( + "max-age=", + Assert.Single(response.Headers.GetValues("Strict-Transport-Security")), + StringComparison.Ordinal); + } + + /// + /// Verifies that emits the header outside Development. + /// + /// A task that represents the asynchronous test operation. + [Fact] + public async Task UseApplicationHsts_Production_EmitsStrictTransportSecurity() + { + using HttpResponseMessage response = await SendThroughHstsAsync(Environments.Production); + + Assert.True(response.Headers.Contains("Strict-Transport-Security")); + } + + /// + /// Verifies that does not emit the header in Development. + /// + /// A task that represents the asynchronous test operation. + [Fact] + public async Task UseApplicationHsts_Development_DoesNotEmitStrictTransportSecurity() + { + using HttpResponseMessage response = await SendThroughHstsAsync(Environments.Development); + + Assert.False(response.Headers.Contains("Strict-Transport-Security")); + } + + private static async Task SendThroughHstsAsync(string environmentName) + { + WebApplicationBuilder builder = WebApplication.CreateBuilder(new WebApplicationOptions + { + EnvironmentName = environmentName + }); + _ = builder.WebHost.UseTestServer(); + + await using WebApplication app = builder.Build(); + _ = app.UseApplicationHsts(); + _ = app.MapGet("/", () => Results.Ok()); + await app.StartAsync(TestContext.Current.CancellationToken); + + using HttpClient client = app.GetTestClient(); + client.BaseAddress = _publicHttpsBaseAddress; + + HttpResponseMessage response = await client.GetAsync("/", TestContext.Current.CancellationToken); + await app.StopAsync(TestContext.Current.CancellationToken); + return response; + } +} diff --git a/tests/ProjectTemplate.Web.Tests/HttpContextApplicationAuditContextAccessorTests.cs b/tests/ProjectTemplate.Web.Tests/HttpContextApplicationAuditContextAccessorTests.cs new file mode 100644 index 00000000..f792ee61 --- /dev/null +++ b/tests/ProjectTemplate.Web.Tests/HttpContextApplicationAuditContextAccessorTests.cs @@ -0,0 +1,65 @@ +using System.Net; +using System.Security.Claims; +using Microsoft.AspNetCore.Http; +using ProjectTemplate.Infrastructure.Data.Auditing; +using ProjectTemplate.Web.Accessors; +using ProjectTemplate.Web.Authentication.Claims; + +namespace ProjectTemplate.Web.Tests; + +public sealed class HttpContextApplicationAuditContextAccessorTests +{ + [Fact] + public void Current_AuthenticatedUserWithOnlyNormalizedSubjectClaim_UsesSubject() + { + ApplicationAuditContext context = CreateAccessor( + CreateAuthenticatedPrincipal(new Claim(ApplicationClaimTypes.Subject, "user-789"))).Current; + + Assert.Equal("user-789", context.ActorId); + Assert.Equal(ApplicationAuditActorTypes.Human, context.ActorType); + } + + [Fact] + public void Current_AuthenticatedUserWithNormalizedAndProviderSubjectClaims_PrefersNormalizedSubject() + { + ApplicationAuditContext context = CreateAccessor( + CreateAuthenticatedPrincipal( + new Claim("sub", "provider-subject"), + new Claim(ApplicationClaimTypes.Subject, "user-789"))).Current; + + Assert.Equal("user-789", context.ActorId); + Assert.Equal(ApplicationAuditActorTypes.Human, context.ActorType); + } + + [Fact] + public void Current_AuthenticatedUserWithOnlyProviderSubjectClaim_UsesProviderSubject() + { + ApplicationAuditContext context = CreateAccessor( + CreateAuthenticatedPrincipal(new Claim("sub", "provider-subject"))).Current; + + Assert.Equal("provider-subject", context.ActorId); + } + + [Fact] + public void Current_AuthenticatedUserWithoutSubjectClaims_UsesRemoteIpAddress() + { + ApplicationAuditContext context = CreateAccessor(CreateAuthenticatedPrincipal()).Current; + + Assert.Equal("192.0.2.10", context.ActorId); + Assert.Equal(ApplicationAuditActorTypes.Network, context.ActorType); + } + + private static HttpContextApplicationAuditContextAccessor CreateAccessor(ClaimsPrincipal user) + { + var httpContext = new DefaultHttpContext { User = user }; + httpContext.Connection.RemoteIpAddress = IPAddress.Parse("192.0.2.10"); + + return new HttpContextApplicationAuditContextAccessor( + new HttpContextAccessor { HttpContext = httpContext }); + } + + private static ClaimsPrincipal CreateAuthenticatedPrincipal(params Claim[] claims) + { + return new ClaimsPrincipal(new ClaimsIdentity(claims, authenticationType: "Test")); + } +} From c26033d70dbc9f9ff0e0b1fa95758441c6b895d8 Mon Sep 17 00:00:00 2001 From: Chris Cavell Date: Mon, 21 Sep 2026 19:39:13 -0500 Subject: [PATCH 5/5] (fmt): IDE0007: use 'var' instead of explicit type --- tests/ProjectTemplate.Web.Tests/HstsTests.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/ProjectTemplate.Web.Tests/HstsTests.cs b/tests/ProjectTemplate.Web.Tests/HstsTests.cs index 2921f33d..006fe455 100644 --- a/tests/ProjectTemplate.Web.Tests/HstsTests.cs +++ b/tests/ProjectTemplate.Web.Tests/HstsTests.cs @@ -24,7 +24,7 @@ public sealed class HstsTests [Fact] public async Task ApplicationPipeline_NonDevelopmentHttpsRequest_EmitsStrictTransportSecurity() { - using ApplicationWebApplicationFactory factory = + using var factory = ApplicationWebApplicationFactory.CreateAllowingAnonymousAccess(new Dictionary()); using HttpClient client = factory.CreateClient(new WebApplicationFactoryClientOptions {