fix: escape passkey notification label and defer registration event to after-commit

[devondragon]

Post-release review follow-up on the 5.3.3 WebAuthn step-up + credential-registration audit work (#365, #367). A second adversarial pass (a fresh review plus an independent Fable agent working the threat model from code) revised the review's severities down and left a few small, low-risk fixes worth shipping.

Changes

1. Escape the passkey label in the notification email (was reported High, actually Medium).
webauthn-credential-registered.html renders the client-supplied credential label with th:utext (unescaped, so the message's own <strong> markup survives). The label flows unsanitized from the WebAuthn registration record, is capped only by the 64-char DB column, and the email is on by default. A crafted label could inject an HTML link into the very email that warns the owner of an unrecognized passkey. UserEmailService.sendPasskeyRegisteredNotification now HtmlUtils.htmlEscapes the label before it reaches the template. The separate audit-log path is already sanitized by FileAuditLogWriter.sanitizeField, so it is unchanged.

2. Defer the registration listener to AFTER_COMMIT (was reported High, actually Low; fixes a distinct phantom-notification bug).
WebAuthnCredentialRegisteredEvent is published inside the @Transactional save() before commit, and the listener was a synchronous ApplicationListener. A commit failure (e.g. a label longer than the 64-char column) therefore sent a "new passkey" email and wrote an audit entry for a registration that never persisted. The listener is now @TransactionalEventListener(phase = AFTER_COMMIT, fallbackExecution = true).

3. Return 401 + step-up-required JSON on passkey enrollment denial (was reported Medium consistency gap).
The enrollment gate is a filter-chain authorizeHttpRequests rule, so its denial is raised before any controller and never reaches WebAuthnManagementAPIAdvice. It returned a bare 403, the only one of the four step-up-gated operations not returning 401 + {"error":"step-up-required"}, leaving a client unable to tell "re-run your login ceremony and retry" from an ordinary authorization failure. New StepUpEnrollmentAccessDeniedHandler, scoped to POST /webauthn/register via defaultAccessDeniedHandlerFor, renders an AuthorizationDeniedException as the 401 step-up contract and delegates everything else (e.g. CSRF) to the default handler. The scoping leaves the existing MFA missing-authority handler in place for every other path. Enforcement (the authorization manager) is unchanged; only the denial response is.

4. Clarify CONFIG.md on custom StepUpService precedence (Medium doc-vs-code).
The old text claimed a custom bean applies "whatever enabled is set to." That is true for setPassword (keyed on bean presence, the original 5.3.1 SPI surface) but not for the passkey delete/rename/remove-password endpoints, which key on user.security.stepUp.enabled=true by design (WebAuthnManagementAPI:274-277) so 5.3.1 SPI adopters don't get step-up newly enforced on upgrade. Doc now states the per-endpoint behavior. No code change: the divergence is deliberate.

Not changed (deliberately)

The review's headline "High" (enrollment gate accepts any factor) was downgraded to Low after tracing the code: for password accounts the credential-altering ops are gated by the current-password path, not step-up, so an attacker holding the password already owns the account; for passwordless passkey-only accounts the only mintable factor is WEBAUTHN. The one genuine net-new gap (passwordless OAuth+passkey within the 600s enrollment window) is the residual already documented in CONFIG.md, is audited and emailed, and the proposed "require configured factors" fix is both bypassable in the headline scenario and regresses legitimate second-device enrollment. Left as designed.

Tests

• New UserEmailServiceTest cases: label is HTML-escaped, null label falls back to "Passkey", no-op when the recipient has no email.
• New WebAuthnCredentialRegistrationListenerTest guard pinning the AFTER_COMMIT phase and fallbackExecution=true.
• WebAuthnEnrollmentGateIntegrationTest updated to assert the 401 + step-up-required contract through the real filter chain, and that the fresh-factor case is not step-up-denied.
• Full ./gradlew test suite passes locally, including the MFA missing-authority redirect tests that share the access-denied handler.

https://claude.ai/code/session_01KL5qvKVGQLLQDjHToy34vj

[Copilot]

🟢 Approval recommended

The changes are low-risk, align with the stated intent (escaping + after-commit listener), and are covered by targeted new tests.

Pull request overview

This PR is a small post-release hardening follow-up for the framework’s WebAuthn (passkey) enrollment audit + notification flow: it prevents HTML injection in the “new passkey added” email by escaping the user-supplied credential label, and ensures the enrollment listener only reacts after the enrolling transaction commits to avoid phantom audit/notification on rollback. It also updates the configuration documentation to accurately describe StepUpService precedence and which endpoints consult it.

Changes:

• Escape the passkey label (HtmlUtils.htmlEscape) before passing it into the Thymeleaf th:utext notification template.
• Convert the WebAuthn credential registration listener to an @TransactionalEventListener running AFTER_COMMIT (with fallbackExecution=true) and update/add tests to pin this behavior.
• Clarify CONFIG.md wording around custom StepUpService behavior across endpoints.

File summaries

File	Description
src/main/java/com/digitalsanctuary/spring/user/service/UserEmailService.java	Escapes the passkey label before rendering in the HTML email template.
src/main/java/com/digitalsanctuary/spring/user/listener/WebAuthnCredentialRegistrationListener.java	Defers listener execution to after transaction commit via @TransactionalEventListener.
src/test/java/com/digitalsanctuary/spring/user/service/UserEmailServiceTest.java	Adds tests validating label escaping, null-label fallback, and no-op when user has no email.
src/test/java/com/digitalsanctuary/spring/user/listener/WebAuthnCredentialRegistrationListenerTest.java	Updates tests for the new handler method and adds an assertion pinning AFTER_COMMIT + fallbackExecution.
CONFIG.md	Updates documentation to reflect actual endpoint-by-endpoint StepUpService consultation behavior.

Review details

• Files reviewed: 5/5 changed files
• Comments generated: 0
• Review effort level: Lite

---

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

[claude]

Review

Reviewed the diff, the touched files in full context (WebSecurityConfig, FreshFactorAuthorizationManager, WebAuthnStepUpRequiredException, GenericResponse, the mail template/message bundle, WebAuthnManagementAPI), and the new/updated tests. Overall this is a well-reasoned, low-risk follow-up: each fix traces back to a concrete failure scenario, and the severity downgrades in the description hold up against the code.

Verified correct

• Label escaping (UserEmailService): confirmed email.passkey-registered.intro=...({0})... is rendered via th:utext, so an unescaped {0} really would let a crafted label break out of the tag. HtmlUtils.htmlEscape on the label closes that; the audit path uses a separate, already-sanitized field, so leaving it alone is correct.
• AFTER_COMMIT listener deferral: confirmed JpaUserCredentialRepository.save() is @transactional and publishes WebAuthnCredentialRegisteredEvent before the method returns (i.e. before commit). Moving the listener to @TransactionalEventListener(phase = AFTER_COMMIT, fallbackExecution = true) is the right fix for the phantom-notification bug described, and fallbackExecution = true correctly preserves firing when no transaction is active.
• 401 step-up-required on enrollment denial: FreshFactorAuthorizationManager denies via AuthorizationDecision(false), which Spring Security AuthorizationManager.verify() surfaces as AuthorizationDeniedException (a subtype of AccessDeniedException) - so gating StepUpEnrollmentAccessDeniedHandler on instanceof AuthorizationDeniedException and delegating everything else (e.g. CsrfException, also an AccessDeniedException subtype) to AccessDeniedHandlerImpl is the right discriminator. The {message, error} JSON shape matches GenericResponse/WebAuthnManagementAPIAdvice exactly. Scoping via defaultAccessDeniedHandlerFor plus a request-matcher is the correct way to avoid disturbing setupMfa() global DelegatingMissingAuthorityAccessDeniedHandler - Spring builds a RequestMatcherDelegatingAccessDeniedHandler from all defaultAccessDeniedHandlerFor registrations plus the global handler as fallback, regardless of call order, so this works whether or not MFA is enabled.
• CONFIG.md wording: cross-checked against UserAPI.setPassword (gates on bean presence alone) and WebAuthnManagementAPI.requireCredentialProof (gates on stepUpConfigProperties.isEnabled(), by design, per the inline comment) - the new doc text accurately reflects the per-endpoint divergence.

Minor suggestions (non-blocking)

1. No direct test for the handler delegate branch. StepUpEnrollmentAccessDeniedHandler exists specifically to not intercept non-freshness denials (e.g. a CSRF failure on POST /webauthn/register should still get a plain 403, not the step-up 401). The only coverage is the integration test AuthorizationDeniedException path; there is no test, unit or integration, that drives the delegate-to-AccessDeniedHandlerImpl branch. A small unit test (new StepUpEnrollmentAccessDeniedHandler(mockDelegate).handle(..., new AccessDeniedException("x")) then verify delegate invoked) would pin the behavior the class own Javadoc promises.
2. shouldReactAfterCommit is a reflection check, not a behavioral one. It confirms the annotation phase/fallbackExecution attributes are set correctly, but does not exercise an actual transaction (i.e. prove a rolled-back save really suppresses the email/audit, and a committed one really fires it). Given the class is otherwise tested with plain Mockito (no Spring context), this is a reasonable trade-off - just flagging that true regression coverage for "rollback implies no notification" would need a @DataJpaTest/full-context test with a real PlatformTransactionManager, which is a bigger lift than this PR scope.

Neither of these blocks merging - they are gaps in an otherwise solid test suite for the actual bugs being fixed. No security, correctness, or performance issues found beyond what is already addressed by this PR.

[claude]

Review

Went through the full diff plus the surrounding call sites (WebAuthnRepositoryConfig.save(), the message template, FileAuditLogWriter.sanitizeField, GenericResponse, WebAuthnManagementAPIAdvice). This is a tight, well-reasoned follow-up — each fix traces back to a concrete, verifiable code path rather than a hypothetical, and the "not changed" section is a good sign the threat model was actually re-derived rather than rubber-stamped.

Fix 1 — HTML-escape the passkey label

Confirmed the vulnerability was real: email.passkey-registered.intro=...<strong>{0}</strong>... in dsspringusermessages.properties is substituted into the message string before Thymeleaf renders it via th:utext, so nothing upstream was escaping {0}. Fixing it at UserEmailService.sendPasskeyRegisteredNotification (escape once, at the point where untrusted data crosses into a rendering context) is the right layer — cheaper and more robust than trying to sanitize at the template. Good call leaving the audit-log path alone since sanitizeField there is solving a different problem (log-line injection via CR/LF, not HTML).

Fix 2 — AFTER_COMMIT transactional listener

Verified JpaUserCredentialRepository.save() is @Transactional and publishes the event before credentialRepository.save(entity) returns/commits — so the pre-existing bug (email + audit entry for a registration that then rolls back) was real. fallbackExecution = true is the correct guard against the listener silently going dead if the event is ever published outside a transaction. The reflection-based test that pins phase()/fallbackExecution() is a reasonable way to guard against a future refactor accidentally dropping the annotation config.

Minor: catching RuntimeException around the audit publish is fine for the audit publisher's known failure modes, but it's worth double-checking that mock/test ApplicationEventPublisher never throws Error/checked-adjacent constructs that would still propagate. Not a real risk today, just flagging since AFTER_COMMIT listeners fail silently by default.

Fix 3 — 401 step-up-required on enrollment denial

This is the most delicate part of the PR and it's also where the third commit's own history is instructive: the second commit's defaultAccessDeniedHandlerFor approach genuinely didn't scope (per Spring's ExceptionHandlingConfigurer.createDefaultAccessDeniedHandler collapsing a single mapping to the bare handler), and would have made every AuthorizationDeniedException in the app return step-up-required — including the DELETE /webauthn/register/** denyAll() rule a few lines above. Good that this was caught and fixed with the explicit RequestMatcherDelegatingAccessDeniedHandler composition, and that regression tests were added for exactly that failure mode (shouldNotApplyStepUpContractToNonEnrollmentDenials, shouldDelegateCsrfFailureOnEnrollmentPath). That's the kind of thing that's easy to get wrong silently — glad it's pinned by a test now rather than just fixed once.

The StepUpEnrollmentAccessDeniedHandler javadoc is honest about its own blast radius: it documents that the exception-type classification only holds while the freshness gate is the only authorization rule on that path, and that adding a second rule later would silently mislabel an unrelated denial as step-up-required. That's a real maintenance trap for someone who adds a role check to /webauthn/register later without reading the class doc — there's no code-level guard against it (can't easily add one given how authorizeHttpRequests composes), so this is really just something to keep in mind during future changes to that endpoint, not something to fix now.

Small robustness note: the JSON body in handle() is hand-assembled via string concatenation rather than serialized. It's safe today because both WebAuthnStepUpRequiredException.ERROR_CODE and the message are compile-time constants, and the comment says as much — but if anyone later parameterizes the message (e.g. includes the request path or a reason), this becomes a JSON-injection footgun with no test to catch it. Not asking for a change, just flag it for whoever touches this handler next.

Fix 4 — CONFIG.md clarification

Verified against WebAuthnManagementAPI:274-277-style property-gated checks vs. the bean-presence check for setPassword — the doc now accurately reflects the per-endpoint asymmetry instead of the previous blanket claim. Good catch that this was actually a doc/code divergence rather than a bug.

Test coverage

Solid — new tests target the actual regression each fix closes rather than just re-asserting the happy path (the AFTER_COMMIT phase pin, the escaped-label assertion, the two access-denied-handler regression guards). One gap worth considering: there's no integration test combining MFA and WebAuthn step-up both enabled to confirm the composed handler correctly routes a non-enrollment missing-authority denial to the MFA handler rather than the plain-403 fallback — the unit-level composition looks correct by inspection, but that combination isn't exercised end-to-end anywhere in this diff.

Overall

No blocking issues found. The self-critical framing in the PR description (severities revised down, one thing deliberately left alone) matches what the code and tests actually show. Nice work tightening the access-denied-handler scoping between commits 2 and 3 — that's exactly the kind of bug an adversarial second pass should catch.