fix(service-defaults): security trio — middleware order, JWT, error t…#25
Merged
Conversation
…race ID Three small but distinct security hardening fixes in ServiceDefaults (addresses architecture-review findings #3, #5, #6). 1. MIDDLEWARE ORDER — CorrelationIdMiddleware after UseAuthentication The middleware reads ClaimTypes.NameIdentifier from context.User to populate the UserId logger scope key. Pre-auth, context.User is empty, so UserId was silently always null for every authenticated request — defeating half the context-propagation pipeline. UserId never made it into log scopes from the HTTP entry point and never reached Wolverine envelope headers via the ActivityBaggage → OutgoingContextMiddleware path. Reordered: UseExceptionHandler → UseAuthentication → UseAuthorization → CorrelationIdMiddleware. ExceptionHandler stays first so it still wraps everything below it. 2. JWT — explicit ValidateIssuerSigningKey + tightened ClockSkew ValidateIssuerSigningKey was implicit (JWT Bearer's default validates against JWKS-discovered keys). Made explicit so it's auditable + prevents a future config change from silently disabling it. ClockSkew defaulted to 5 MINUTES. Revoked/expired tokens were accepted for 5 extra minutes — material on typical 15-minute access-token lifetimes. Set to 30 seconds: covers reasonable inter-server clock drift, doesn't give attackers a long replay window. 3. EXCEPTION HANDLER — TraceId only, not full Activity.Id Activity.Current?.Id returns the full W3C traceparent ("00-<trace>-<span>-<flags>") and we were embedding that in every ProblemDetails response. The span ID is information about server-side handler call structure that clients have no business seeing. Switched to Activity.Current?.TraceId.ToString() — same correlation utility, no leak of internal call graph. None of the three needed any consumer changes; all are internal to ServiceDefaults. Build clean (the one CS0436 warning is the pre-existing benchmarks Program-conflict; unaffected). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
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 (2)
WalkthroughMiddleware ordering changed so CorrelationIdMiddleware runs after authentication and before authorization; JWT bearer TokenValidationParameters explicitly enable issuer signing key validation and set ClockSkew to 30s; GlobalExceptionHandler returns Activity.TraceId (or HttpContext.TraceIdentifier) and omits raw exception messages. ChangesService Defaults Security and Observability Hardening
Sequence DiagramsequenceDiagram
participant Client
participant UseExceptionHandler
participant UseAuthentication
participant CorrelationIdMiddleware
participant UseAuthorization
participant Endpoint
Client->>UseExceptionHandler: incoming request
UseExceptionHandler->>UseAuthentication: pass to authentication
UseAuthentication->>CorrelationIdMiddleware: authentication completed (context.User available)
CorrelationIdMiddleware->>UseAuthorization: attach correlation id for audit/authorization
UseAuthorization->>Endpoint: authorize then route
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@NextAurora.ServiceDefaults/Extensions.cs`:
- Around line 144-146: The CorrelationIdMiddleware is registered after
UseAuthorization which allows authorization to short-circuit before the
middleware runs; move the middleware registration so it executes between
UseAuthentication and UseAuthorization by reordering the calls: call
app.UseAuthentication(), then app.UseMiddleware<CorrelationIdMiddleware>(), then
app.UseAuthorization() so that CorrelationIdMiddleware runs for
authenticated/authorization flows and applies correlation headers and scoped
logging.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 9d4d0281-fb4d-4711-a866-5cf545183f13
📒 Files selected for processing (2)
NextAurora.ServiceDefaults/Extensions.csNextAurora.ServiceDefaults/GlobalExceptionHandler.cs
… Authz CodeRabbit (PR #25) flagged that the previous fix moved the middleware too far down — to AFTER UseAuthorization — which works for populating UserId but drops the audit trail for 401/403 unauthorized-attempt logs. Correct placement is BETWEEN UseAuthentication and UseAuthorization: app.UseExceptionHandler(); app.UseAuthentication(); // populates context.User app.UseMiddleware<CorrelationIdMiddleware>(); // reads User, opens scope app.UseAuthorization(); // any 401/403 here has UserId in scope Two reasons: 1. CorrelationIdMiddleware needs context.User populated to read NameIdentifier (the original ordering bug — pre-auth, User was empty and UserId was always null). 2. Placing it BEFORE Authorization means the UserId scope is active during the authorization decision. If a user is rejected, the log line says "user X tried Y and was denied" — the audit trail we need. My previous fix dropped this by placing the middleware after Authorization, which would only attribute requests that actually passed authorization. Build clean. Single-line move of one app.UseMiddleware call. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…andler
Covers the production-code changes in this PR so Codecov's patch-coverage
gate stops complaining (was 8.33%; the prior fixes were uncovered because
ServiceDefaults had no dedicated tests).
GlobalExceptionHandlerTests (3):
- TryHandleAsync_WhenActivityIsActive_ReturnsTraceIdOnly_NotFullActivityId:
pins the security-critical change (Activity.Id → TraceId.ToString()).
Asserts the response traceId equals activity.TraceId.ToString(), NOT
activity.Id (which is the full W3C traceparent including span ID).
Also asserts no hyphens — distinguishes the bare trace from the full
"00-trace-span-flags" format.
- TryHandleAsync_WhenNoActivity_FallsBackToHttpContextTraceIdentifier:
covers the null-coalesce path.
- TryHandleAsync_AlwaysReturnsProblemDetailsWithGenericDetail_NeverExceptionMessage:
defense against information disclosure (CLAUDE.md "Error Handling").
Asserts the raw exception message doesn't appear in the response body
even when the message contains sensitive-looking content like SQL.
ServiceDefaultsJwtTests (3):
- AddServiceDefaults_WhenAuthorityConfigured_SetsExplicitSigningKeyValidation
- AddServiceDefaults_WhenAuthorityConfigured_SetsTightClockSkew
- AddServiceDefaults_WhenAuthorityConfigured_RetainsCoreValidations
All three boot a real Host.CreateApplicationBuilder + AddServiceDefaults
with a configured authority, resolve the IOptionsMonitor<JwtBearerOptions>,
and assert the TokenValidationParameters. Pins both the new hardening
(ClockSkew=30s, ValidateIssuerSigningKey=true) AND the pre-existing core
validations (Audience/Issuer/Lifetime) so a future refactor can't drop
either silently.
All 6 tests pass in 0.7s. Build clean (the one CS0436 warning is the
pre-existing benchmarks Program-conflict; unaffected).
Test placement follows the existing convention — ServiceDefaults tests
live in service-test projects (OrderService.Tests.Unit/Application/
already has ContextPropagationMiddlewareTests, CorrelationIdMiddlewareTests,
OutgoingContextMiddlewareTests). Not ideal long-term (duplication risk
across the 4 service test projects), but matches the established pattern.
The middleware-ordering change in Extensions.cs is harder to unit-test —
it requires booting WebApplicationFactory and asserting the order of
middleware execution at request time. Filed as a project-board issue for
later; the JWT and ExceptionHandler tests cover the highest-risk lines
of this PR (security hardening that could silently regress).
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
…race ID
Three small but distinct security hardening fixes in ServiceDefaults (addresses architecture-review findings #3, #5, #6).
The middleware reads ClaimTypes.NameIdentifier from context.User to populate the UserId logger scope key. Pre-auth, context.User is empty, so UserId was silently always null for every authenticated request — defeating half the context-propagation pipeline. UserId never made it into log scopes from the HTTP entry point and never reached Wolverine envelope headers via the ActivityBaggage → OutgoingContextMiddleware path.
Reordered: UseExceptionHandler → UseAuthentication → UseAuthorization → CorrelationIdMiddleware. ExceptionHandler stays first so it still wraps everything below it.
ValidateIssuerSigningKey was implicit (JWT Bearer's default validates against JWKS-discovered keys). Made explicit so it's auditable + prevents a future config change from silently disabling it.
ClockSkew defaulted to 5 MINUTES. Revoked/expired tokens were accepted for 5 extra minutes — material on typical 15-minute access-token lifetimes. Set to 30 seconds: covers reasonable inter-server clock drift, doesn't give attackers a long replay window.
Activity.Current?.Id returns the full W3C traceparent ("00---") and we were embedding that in every ProblemDetails response. The span ID is information about server-side handler call structure that clients have no business seeing. Switched to Activity.Current?.TraceId.ToString() — same correlation utility, no leak of internal call graph.
None of the three needed any consumer changes; all are internal to ServiceDefaults. Build clean (the one CS0436 warning is the pre-existing benchmarks Program-conflict; unaffected).
What changed
How it was built
Verification
Touches (check only if applies)
See CLAUDE.mdparaphrase (run/check-rulesto audit drift)Summary by CodeRabbit
Security Improvements
Bug Fixes
Tests