Skip to content

Add global antiforgery, align audit actor claims, and tidy security defaults - #577

Merged
cdcavell merged 5 commits into
mainfrom
fix/antiforgery-claims-and-security-defaults
Sep 22, 2026
Merged

cdcavell merged 5 commits into
mainfrom
fix/antiforgery-claims-and-security-defaults

Conversation

@cdcavell

Copy link
Copy Markdown
Collaborator

Summary

4. Global antiforgery filter (MEDIUM)

AddControllersWithViews(options => options.Filters.Add(new AutoValidateAntiforgeryTokenAttribute())) in Program.cs. Every unsafe MVC request is now validated by default. Razor Pages already validate by default.
Behavior change for adopters: API actions called without cookies (bearer-token clients) need [IgnoreAntiforgeryToken]. None of the template's own actions are affected.

5. Claims types disagree between transformation and audit actor (MEDIUM)

  • HttpContextCurrentActorAccessor and HttpContextApplicationAuditContextAccessor (which had the same bug) now read application:subject first, then sub, then NameIdentifier. With RemoveOriginalClaims = true, authenticated users are attributed correctly instead of as Remote IP: ….
  • ApplicationClaimsTransformation now clones each identity and returns a new principal, so the incoming principal is never mutated.
  • Each transformed identity's NameClaimType / RoleClaimType is set to application:name / application:role, so User.Identity.Name, User.IsInRole and [Authorize(Roles = "...")] work. The original type is kept only when the identity still has claims of that type and no normalized equivalent (for example, an unmapped provider type), so existing checks don't silently break.

7. Structural / secure-default nits (LOW)

  • HSTS moved out of ErrorHandling/ProblemDetailsExtensions into SecurityHeadersExtensions.UseApplicationHsts(). It is called right after UseProblemDetails(), so the effective middleware order is unchanged. The docs, which wrongly said the pipeline doesn't call UseHsts(), now match the code.
  • SectionName on ApplicationSecurityHeadersOptions and ApplicationRequestLoggingOptions is now const.
  • Excluded paths (/health, /metrics) now still get X-Content-Type-Options: nosniff. They still skip the other headers.
  • Authorization policies are now built in Configure<IOptions<ApplicationAuthorizationOptions>> from the bound, validated options instance. The separate configuration.Get<>() snapshot is gone. Note: policies are still materialized once, when AuthorizationOptions is first resolved, so they are not reload-aware.

Tests

  • New AntiforgeryTests: a POST without an antiforgery attribute returns 400 without a token and 200 with one.
  • ClaimsTransformationTests: the incoming principal is not mutated, and IsInRole and Identity.Name work after RemoveOriginalClaims.
  • HttpContextCurrentActorAccessorTests: the normalized subject is used on its own and preferred over sub.
  • Updated excluded-path tests to expect nosniff only.
  • dotnet build -c Release (warnings as errors) passes; dotnet test: 462/462 passed.

🤖 Generated with Claude Code

…efaults

- 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 <noreply@anthropic.com>
Copilot AI lite review requested due to automatic review settings September 22, 2026 00:17

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

HSTS is now registered after the status-code-page branches, so the promised unchanged middleware ordering is not preserved.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1 Medium severity

Open (1)
What changed in this PR

Adds global antiforgery validation, normalizes claims safely, aligns audit actor resolution, and refines security-header/HSTS configuration.

Changes:

  • Applies global MVC antiforgery validation with integration tests.
  • Clones transformed principals and updates authorization/audit claim handling.
  • Refactors HSTS and excluded-path security headers, updating documentation and tests.
File Description
src/​ProjectTemplate.Web/​Program.cs Registers global antiforgery validation.
src/​ProjectTemplate.Web/​Authentication/​Claims/​ApplicationClaimsTransformation.cs Clones principals and normalizes identity claim types.
src/​ProjectTemplate.Web/​Authentication/​Extensions/​AuthorizationServiceExtensions.cs Builds policies from validated options.
src/​ProjectTemplate.Web/​Accessors/​HttpContextCurrentActorAccessor.cs Prefers normalized subjects.
src/​ProjectTemplate.Web/​Accessors/​HttpContextApplicationAuditContextAccessor.cs Aligns audit subject resolution.
src/​ProjectTemplate.Web/​Middleware/​SecurityHeadersMiddleware.cs Applies nosniff to excluded paths.
src/​ProjectTemplate.Web/​Extensions/​SecurityHeadersExtensions.cs Adds HSTS pipeline support.
src/​ProjectTemplate.Web/​Extensions/​PipelineExtensions.cs Registers HSTS and security headers.
src/​ProjectTemplate.Web/​ErrorHandling/​ProblemDetailsExtensions.cs Removes embedded HSTS registration.
src/​ProjectTemplate.Web/​Options/​ApplicationSecurityHeadersOptions.cs Makes section name constant.
src/​ProjectTemplate.Web/​Options/​ApplicationRequestLoggingOptions.cs Makes section name constant.
tests/​ProjectTemplate.Web.Tests/​AntiforgeryTests.cs Tests global antiforgery behavior.
tests/​ProjectTemplate.Web.Tests/​ClaimsTransformationTests.cs Tests cloning and normalized identity behavior.
tests/​ProjectTemplate.Web.Tests/​HttpContextCurrentActorAccessorTests.cs Tests normalized subject precedence.
tests/​ProjectTemplate.Web.Tests/​SecurityHeadersTests.cs Updates excluded-header expectations.
tests/​ProjectTemplate.Web.Tests/​HealthCheckTests.cs Updates health-header expectations.
tests/​ProjectTemplate.Web.Tests/​TestControllers/​AuthenticationTestController.cs Adds an unannotated POST test endpoint.
docs/​articles/​security-headers.md Documents HSTS and excluded-path behavior.
docs/​articles/​middleware.md Updates pipeline documentation.
docs/​articles/​health-checks.md Documents nosniff on health endpoints.
docs/​articles/​error-handling.md Removes outdated HSTS description.
CHANGELOG.md Records the behavior and security changes.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/ProjectTemplate.Web/Extensions/PipelineExtensions.cs Outdated
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 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings September 22, 2026 00:23

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

Excluded-path HSTS documentation is inaccurate, and audit attribution and HSTS behavior need regression coverage.

Review effort: Lite
Findings: 1 Medium severity

Open (1)
Previously missed (4)

In code that hasn't changed since last review

Medium severity Add regression tests for normalized audit subject precedence

src/​ProjectTemplate.Web/​Accessors/​HttpContextApplicationAuditContextAccessor.cs:27

This change fixes normalized-subject precedence in the audit accessor, but the test suite has no test that resolves HttpContextApplicationAuditContextAccessor.Current (the added actor tests cover only HttpContextCurrentActorAccessor). A regression here could still silently record authenticated requests as remote-IP actors; add focused coverage for normalized-only and normalized-versus-sub claims.

Medium severity Add integration coverage for the non-Development HSTS header

src/​ProjectTemplate.Web/​Extensions/​SecurityHeadersExtensions.cs:72

The new non-Development HSTS registration is not asserted by the integration suite: the factory uses the Testing environment and these clients use HTTPS, but no test checks Strict-Transport-Security. Add a focused response assertion so moving or accidentally removing this security middleware cannot pass without detection.

Low severity Clarify health response header behavior with HSTS

docs/​articles/​health-checks.md:125

This description says health responses receive only X-Content-Type-Options, but UseApplicationHsts() is still applied to HTTPS health responses outside Development. Qualify the statement to the configurable security-header middleware or document the separate HSTS header, otherwise the health-check contract is misleading.

Low severity Clarify excluded-path security header behavior with HSTS

docs/​articles/​security-headers.md:29

UseApplicationHsts() runs before this middleware for every non-Development HTTPS request, including excluded paths, so those responses can also contain Strict-Transport-Security. Saying excluded paths receive no other security header conflicts with the HSTS contract documented immediately below; qualify this statement as applying to headers emitted by SecurityHeadersMiddleware, or mention the separately registered HSTS header.

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 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings September 22, 2026 00:27

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

The security documentation overstates excluded-path header behavior, and the changelog contains a grammar error.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1 Medium severity · 1 Low severity

Open (2)
Previously missed (1)

In code that hasn't changed since last review

Low severity Use plural verb for the two accessors

CHANGELOG.md:47

The subject here is the two accessors, so the verb should be plural (read). Please correct this new changelog entry's grammar.

Comment thread docs/articles/security-headers.md Outdated
…h 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 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings September 22, 2026 00:34

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟢 Approval recommended

The changed implementation, tests, documentation, and security defaults were reviewed with no unresolved correctness issues.

Review effort: Lite
Findings: 1 Medium severity

Open (1)
Resolved since last review (1)

Copilot AI review requested due to automatic review settings September 22, 2026 00:39

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟢 Approval recommended

The changed behavior is consistently implemented, documented, and covered by focused tests with no blocking defects identified.

Review effort: Lite
Findings: 1 Medium severity

Open (1)

@cdcavell
cdcavell merged commit a779fb5 into main Sep 22, 2026
18 checks passed
@cdcavell
cdcavell deleted the fix/antiforgery-claims-and-security-defaults branch September 22, 2026 00:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants