Skip to content

feat: audit and notify on WebAuthn credential registration - #367

Merged
devondragon merged 1 commit into
mainfrom
feat/webauthn-enrollment-notification
Aug 21, 2026
Merged

feat: audit and notify on WebAuthn credential registration#367
devondragon merged 1 commit into
mainfrom
feat/webauthn-enrollment-notification

Conversation

@devondragon

Copy link
Copy Markdown
Owner

Why

Enrolling a passkey grants a durable new way into an account, and it outlives a password change: session invalidation ends sessions, not credentials. An attacker who reaches an authenticated session can leave themselves a way back in that survives the victim's response.

Until now that produced no audit record and no notification. Spring Security owns POST /webauthn/register and nothing in this framework observed it, so a passkey could be added to an account with no trace and no signal to the owner.

Found while reviewing #365. This is independent of that feature and applies to 5.3.2 as shipped, which is why it is a separate PR.

What

  • WebAuthnCredentialRegisteredEvent carrying the user, credential id, and label.
  • Published from the framework's own JpaUserCredentialRepository, which every enrollment writes through regardless of which endpoint triggered it.
  • WebAuthnCredentialRegistrationListener records a PasskeyRegistration audit event, then emails the owner.
  • user.webauthn.notifyOnRegistration (default true) gates the email only. The audit event is unconditional, and a mail failure cannot lose it.
  • New mail template and message keys that name the specific risk: a passkey survives a password change, so recovering means changing the password and removing the unrecognized passkey.

The trap this had to avoid

UserCredentialRepository.save() is also called inside authenticate(), to persist the updated signature count. Publishing on every save would have emailed the user on every passkey login.

save() already resolves the row with findById(...).orElseGet(WebAuthnCredential::new), so an absent row identifies a genuine registration with no extra query. shouldPublishNoEventWhenCredentialIsUpdated pins it; replacing the guard with if (true) fails that test.

Scope

Detective, not preventive. Preventing enrollment from a session-only actor is step-up (#335 / #365), which is off by default and does not currently gate enrollment at all. This lands regardless of what happens to that branch, and is useful on its own.

Verification

./gradlew check green, ./gradlew javadoc clean. Six new tests, written test-first.

Not verified in a browser: no end-to-end run confirming the email renders and arrives on a real enrollment. Worth a demo-app pass before release.

Compatibility

New emails will be sent on passkey registration for deployments with WebAuthn enabled, which is a visible behavior change on upgrade. Default is true deliberately: a security notification that is off by default protects nobody. Set user.webauthn.notifyOnRegistration=false to opt out.

Enrolling a passkey grants a durable new way into an account, and it
outlives a password change, because session invalidation ends sessions
rather than credentials. An attacker who reaches an authenticated
session can therefore leave themselves a way back in that survives the
victim's response. Until now that enrollment produced no audit record
and no notification: Spring Security owns POST /webauthn/register, and
nothing in this framework observed it.

The framework does own the JPA UserCredentialRepository every enrollment
writes through, so the event is published from there and catches any
path that registers a credential.

The trap, and why the discriminator matters: Spring Security also calls
UserCredentialRepository.save() inside authenticate(), to persist the
updated signature count. Publishing on every save would email the user
on each passkey login. save() already resolves the row with
findById(...).orElseGet(new), so an absent row identifies a genuine
registration with no extra query. A test pins this by asserting no event
on the update path; sabotaging the guard fails it.

- WebAuthnCredentialRegisteredEvent carrying the user, credential id, and label
- WebAuthnCredentialRegistrationListener publishing a PasskeyRegistration
  audit event, then emailing the owner
- user.webauthn.notifyOnRegistration (default true) gates the email only;
  the audit event is unconditional, and a mail failure cannot lose it
- New mail template and message keys naming the specific risk: a passkey
  outlives a password change, so both steps are needed to recover

This is detective, not preventive. Preventing enrollment from a
session-only actor is step-up (#335 / #365), which is off by default and
does not yet gate enrollment. This applies whether or not step-up is on.

./gradlew check: green. ./gradlew javadoc: clean.
Copilot AI lite review requested due to automatic review settings August 20, 2026 20:01
@claude

claude Bot commented Aug 20, 2026

Copy link
Copy Markdown

Review: passkey registration audit + notification

Good addition — closing a real gap (no signal at all today when a passkey is enrolled). A few things worth fixing before merge, roughly in order of importance.

1. HTML injection in the new notification email (security)

webauthn-credential-registered.html renders the passkey label unescaped:

<p th:utext="#{email.passkey-registered.intro(${label})}"></p>

label is attacker-controlled: it's whatever string is submitted in the POST /webauthn/register body, and in the exact threat model this PR targets — an attacker who has hijacked a session enrolling their own passkey — the attacker chooses it. Compare this to forgot-password-token.html, which deliberately renders user.firstName (also raw user input) via th:text (escaped), not th:utext. As written, an attacker can set label to arbitrary markup and have it injected unescaped into the very security notification meant to alert the victim.

Suggest rendering label via th:text on its own element rather than splicing it into an #{...} message and rendering the result with th:utext.

(For what it's worth, the audit log path is fine — FileAuditLogWriter.sanitizeField already strips \r, \n, and | from every field before writing, so the pipe-delimited audit record can't be forged the same way.)

2. Misplaced Javadoc in UserEmailService.java

sendPasskeyRegisteredNotification (and its own Javadoc block) got inserted between the existing Javadoc for createEmailVariables and the createEmailVariables method itself:

/**
 * Creates the email variables.
 * ...
 * @return the map
 * @throws IllegalArgumentException if appUrl is invalid (for admin-initiated resets)
 */
/**
 * Notifies the account owner that a passkey was registered on their account.
 * ...
 */
public void sendPasskeyRegisteredNotification(final User user, final String label) { ... }

private Map<String, Object> createEmailVariables(final User user, final String appUrl, final String token, final String confirmationPath) {

The first block now floats above an unrelated method and no longer documents anything; createEmailVariables is left with no Javadoc at all. Looks like a copy/paste landed in the wrong spot — worth moving the new method (with its own doc comment) down next to the other send* methods.

3. Event fires before the transaction that creates the row commits

JpaUserCredentialRepository.save() is @Transactional, and eventPublisher.publishEvent(new WebAuthnCredentialRegisteredEvent(...)) is the last statement in the method body — i.e. it runs before the surrounding transaction commits (the JPA save() call only queues the write; flush/commit happens when the @Transactional proxy returns). If that transaction then rolls back — a constraint violation, a connection drop during flush, or (if save() joins an already-open caller transaction) a later failure in that same transaction — the audit event and "a passkey was added to your account" email will already have gone out for a credential that was never actually persisted.

This project already has a pattern for exactly this class of problem: UserDeletedEvent/UserDisabledEvent are published from a TransactionSynchronization.afterCommit callback for this reason. Might be worth the same treatment here, though I'd call this low-severity in practice since nothing else appears to run after save() in the registration ceremony.

What looks right

  • The registration-vs-login disambiguation (existing.isEmpty()) is correct and directly tested (shouldPublishNoEventWhenCredentialIsUpdated) — this was the trap called out in the PR description and it's solidly pinned.
  • notifyOnRegistration correctly gates only the email, not the audit record, matching the stated design ("a mail failure cannot lose it").
  • Both the audit publish and the email send end up dispatched asynchronously under the hood (AuditEventListener is @Async, MailService.sendTemplateMessage is @Async("dsMailExecutor") with its own retry/recover), so the synchronous WebAuthnCredentialRegistrationListener doesn't add real latency to the WebAuthn registration response despite not being @Async itself.
  • Test coverage is well targeted at the specific risk (registration vs. every login, audit surviving a mail failure, notification respecting the config flag).

Nothing here should be hard to fix — #1 and #2 are small, targeted changes; #3 is optional depending on how much you want to invest in this specific corner.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Adds framework-level observability for WebAuthn (passkey) enrollment by publishing a dedicated registration event from the JPA credential repository, then handling it to produce an audit trail and (optionally) notify the account owner via email. This fits the framework’s security/audit posture by ensuring credential enrollment is visible even when Spring Security owns the enrollment endpoint.

Changes:

  • Publish WebAuthnCredentialRegisteredEvent only when a credential row is first created (not on signature-count updates).
  • Add WebAuthnCredentialRegistrationListener to emit a PasskeyRegistration audit event and optionally send a notification email (user.webauthn.notifyOnRegistration, default true).
  • Add a new mail template + message keys and document the new configuration property.

Reviewed changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
src/test/java/com/digitalsanctuary/spring/user/security/WebAuthnCredentialRegistrationEventTest.java Pins “new vs updated credential” behavior to avoid notifications on login signature-count saves.
src/test/java/com/digitalsanctuary/spring/user/listener/WebAuthnCredentialRegistrationListenerTest.java Verifies audit publication, email gating, and resilience to notification failures.
src/main/resources/templates/mail/webauthn-credential-registered.html Adds the new passkey-registration email template.
src/main/resources/messages/dsspringusermessages.properties Adds message keys describing passkey registration risk and user guidance.
src/main/resources/config/dsspringuserconfig.properties Documents and defaults user.webauthn.notifyOnRegistration=true.
src/main/java/com/digitalsanctuary/spring/user/service/UserEmailService.java Adds sendPasskeyRegisteredNotification(...) email sender for passkey enrollment.
src/main/java/com/digitalsanctuary/spring/user/security/WebAuthnRepositoryConfig.java Publishes registration event from JpaUserCredentialRepository.save() only when the credential is new.
src/main/java/com/digitalsanctuary/spring/user/security/WebAuthnConfigProperties.java Introduces notifyOnRegistration configuration property (default true).
src/main/java/com/digitalsanctuary/spring/user/listener/WebAuthnCredentialRegistrationListener.java Listens for registration event, emits audit event, and conditionally sends notification email.
src/main/java/com/digitalsanctuary/spring/user/event/WebAuthnCredentialRegisteredEvent.java Defines the event carrying user, credential id, and label.
CONFIG.md Documents the new WebAuthn registration-notification property and its rationale.
Suppressed comments (1)

src/main/java/com/digitalsanctuary/spring/user/listener/WebAuthnCredentialRegistrationListener.java:53

  • This comment says the registration flow "has already committed", but the listener runs synchronously from JpaUserCredentialRepository.save() which is @Transactional, so commit happens after this method returns. Reword to avoid implying the DB write is already committed.
            // Never let a mail failure propagate into the registration flow, which has already committed.

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

<body>
<div>
<span th:text="${user.firstName}"></span>, <br />
<p th:utext="#{email.passkey-registered.intro(${label})}"></p>
Comment on lines +42 to +44
eventPublisher.publishEvent(AuditEvent.builder().source(this).user(event.getUser())
.action("PasskeyRegistration").actionStatus("Success")
.message("Passkey registered: " + event.getLabel()).build());
Comment on lines 168 to 177
/**
* Creates the email variables.
*
* @param user the user
* @param appUrl the app url
* @param token the token
* @param confirmationPath the confirmation path
* @return the map
* @throws IllegalArgumentException if appUrl is invalid (for admin-initiated resets)
*/
@devondragon
devondragon merged commit 1544b68 into main Aug 21, 2026
5 checks passed
@devondragon
devondragon deleted the feat/webauthn-enrollment-notification branch August 21, 2026 02:04
devondragon added a commit that referenced this pull request Aug 21, 2026
)

Implements #335. Off by default; no behavior change until `user.security.stepUp.enabled=true`.

Step-up is a freshness requirement on a Spring Security factor rather than a bespoke ceremony. `WebAuthnAuthenticationProvider` stamps `FACTOR_WEBAUTHN` with the moment of assertion, and re-running the ordinary passkey login while already authenticated merges a newly stamped factor onto the session. The ceremony is therefore the login flow the application already has, re-run: no challenge endpoint, no server-side challenge state, no step-up token.

What it gates: `POST /user/setPassword`, passkey delete and rename on passwordless accounts, and passkey enrollment. Rejections are 401 with error code `step-up-required` on the passkey endpoints.

Notable decisions and the reasoning behind them:

- **Enrollment is gated too** (`enrollmentTtlSeconds`, default 600). Without it the feature protected nothing: an attacker holding only a session cookie could register their own passkey, assert with it to mint a genuinely fresh `FACTOR_WEBAUTHN`, and satisfy every other gate. Any factor satisfies enrollment, deliberately not the configured `factors` list, since with the default `[WEBAUTHN]` that would demand a passkey in order to register a first passkey.

- **Three login paths that stamped no factor now do.** `OidcAuthorizationCodeAuthenticationProvider` stamps nothing (unlike the password and plain-OAuth2 providers), and `authWithoutPassword` builds its own `Authentication`. Two of its three call sites are production paths, so without this every newly registered user's first session carried no factor and could never enroll a first passkey. Dev login still stamps nothing by design, so enrollment is unavailable under `user.dev.auto-login-enabled` while step-up is on.

- **`StepUpService` gains one `default` method**, `canSatisfyStepUp`, distinguishing "has not authenticated recently" from "could never satisfy this". Without it, enabling step-up gave every social-login account a permanent 401 on `setPassword`, since `hasPassword()` is a null check and OAuth users have no password. Defaults to `true`, so existing implementations are unchanged in source and behavior.

- **Delete and rename key off the property, not bean presence**, so applications that adopted the SPI in 5.3.1 for `setPassword` alone see no change on upgrade.

- **`FactorAuthorityNameValidator`** rejects `FACTOR_`-prefixed role and privilege names, which would otherwise satisfy MFA enforcement without the factor ever being completed. It checks the `role`/`privilege` tables as well as configuration, because `RolePrivilegeSetupService` never deletes, and runs on `ContextRefreshedEvent` so lazy initialization cannot skip it.

Residual risks, documented rather than fixed: within either window an attacker sharing the session concurrently can piggyback, and a session stolen within `enrollmentTtlSeconds` of a genuine login can still enroll. That is the trade-off GitHub's sudo mode accepts. A `PasskeyRegistration` audit event and owner notification (#367) cover the detective side.

Not verified in a browser. Step-up is a full re-login, so session fixation, the duplicate `InteractiveAuthenticationSuccessEvent`, and the login JSON response landing mid-flow still need the demo app: devondragon/SpringUserFrameworkDemoApp#75.

Acceptance criteria and status: #335 (25 of 26 met, browser verification outstanding).
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.

2 participants