fix(auth): deny by default with an authorization FallbackPolicy - #327
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #327 +/- ##
==========================================
- Coverage 45.05% 45.04% -0.01%
==========================================
Files 941 941
Lines 49175 49175
Branches 6594 6594
==========================================
- Hits 22155 22152 -3
- Misses 26076 26079 +3
Partials 944 944
Flags with carried forward coverage won't be shown. Click here to find out more.
|
There was a problem hiding this comment.
Pull request overview
This PR hardens VIPER’s authorization posture by introducing an ASP.NET Core FallbackPolicy so endpoints without explicit authorization metadata are denied by default, preventing accidental public exposure of attribute-routed controllers.
Changes:
- Set
options.FallbackPolicy = options.DefaultPolicyto deny endpoints lacking auth metadata. - Explicitly opt specific controllers and health-check endpoint mappings out of the fallback via
[AllowAnonymous]/.AllowAnonymous(). - Add a unit test that pins the three intentionally-anonymous controllers to require
[AllowAnonymous].
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| web/Program.cs | Adds the authorization fallback policy to deny-by-default for endpoints without auth metadata. |
| web/Controllers/LoggedInUserController.cs | Marks the logged-in-user API as explicitly anonymous. |
| web/Controllers/LayoutController.cs | Marks the layout/nav API as explicitly anonymous. |
| web/Classes/HealthChecks/HealthCheckExtensions.cs | Applies .AllowAnonymous() to health endpoints and the healthchecks UI mappings to preserve liveness/diagnostics. |
| web/Areas/CMS/Controllers/CMSController.cs | Marks CMS file serving controller as explicitly anonymous (permission enforced internally). |
| test/Classes/AuthorizationFallbackTests.cs | Adds a reflection-based test ensuring key controllers retain [AllowAnonymous]. |
Suppressed comments (1)
test/Classes/AuthorizationFallbackTests.cs:23
- The test name and failure message read like they’re asserting the fallback policy behavior, but the assertion is specifically about presence of
[AllowAnonymous]on the controller type. Renaming and tweaking the message makes failures more actionable.
public void AnonymousControllers_OptOutOfTheFallbackPolicy(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");
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
5f76d9e to
49e8e8c
Compare
Bundle ReportBundle size has no change ✅ |
49e8e8c to
885a541
Compare
|
@coderabbitai full review |
✅ Action performedFull review finished. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe application now applies the default authorization policy to unmatched endpoints. CMS, layout, logged-in-user, and health-check endpoints explicitly allow anonymous access. Tests verify the required controller annotations. ChangesAuthorization fallback enforcement
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to This PR changes authorization to deny by default while preserving explicitly public endpoints and adding focused coverage. No actionable merge-blocking risk remains at the current head beyond normal checks and review. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Comment |
885a541 to
731c515
Compare
b2ac674 to
26ed8c9
Compare
26ed8c9 to
3c78620
Compare
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
The review-pr workflow writes .review-pr-ignored-<pr> in the repo root to track dismissed threads. It was untracked but not ignored, so a git add -A swept one into a commit.
3c78620 to
13984e4
Compare
Stacked on #326.
Attribute-routed controllers aren't covered by the
RequireAuthorization()on the conventional routes, so a controller that forgets[Authorize]is public. That is howMembersControllerended up exposed in #324. This makes the failure mode "denied" instead.What changed
options.FallbackPolicy = options.DefaultPolicyinProgram.cs.[AllowAnonymous]on what answers anonymously by design:CMSController(public files, checks per-file permissions itself),LayoutController(permission-filtered nav),LoggedInUserController(must answer before the caller knows if anyone is signed in).HomeControlleralready had it..AllowAnonymous()on/health,/health/detail, and the/healthchecksUI. The non-obvious part: these are endpoint mappings, not controllers, so they carry no authorization metadata and the fallback would have caught all three. Jenkins has no CAS credentials, and/health/detailis deliberately reachable when auth is degraded. Hangfire already had.RequireAuthorization().Second commit adds
.review-pr-ignored-*to.gitignore. Unrelated.Before merging
The endpoint audit was static, from routing and attributes. Worth exercising sign-in, the public nav, and a Jenkins
/healthprobe on TEST first.