Skip to content

Optional Turnstile CAPTCHA for unauthenticated email-sending API actions - #347

Merged
devondragon merged 14 commits into
mainfrom
feature/issue-346-captcha
Aug 12, 2026
Merged

Optional Turnstile CAPTCHA for unauthenticated email-sending API actions#347
devondragon merged 14 commits into
mainfrom
feature/issue-346-captcha

Conversation

@devondragon

@devondragon devondragon commented Aug 9, 2026

Copy link
Copy Markdown
Owner

Implements #346. Closes #346.

Summary

Adds optional CAPTCHA protection (Cloudflare Turnstile) for the framework's unauthenticated, email-sending API actions: POST /user/registration, POST /user/registration/passwordless, POST /user/resetPassword, and POST /user/resendRegistrationToken. Off by default, so existing consumers see no behavior change and pay no dependency cost unless they opt in.

Configuration

  • user.security.captcha.enabled (default false) — master switch.
  • user.security.captcha.provider (default turnstile) — only turnstile is built in; a consumer-supplied CaptchaService bean takes precedence over it.
  • user.security.captcha.allow-unusable-provider (default false) — whether to start when the provider reports it cannot verify anything. See Fail-closed behavior.
  • user.security.captcha.protect.registration / .passwordless-registration / .reset-password / .resend-registration-token (default true each) — per-action toggles, effective only when enabled=true.

With enabled=false, no CAPTCHA interceptor or provider beans register — no CaptchaValidationInterceptor, no CaptchaService, no captchaSiteKey model attribute, and no requests are inspected. (CaptchaConfigProperties and CaptchaStartupValidator do always register; the validator early-returns when disabled.) The framework compiles and runs without ds-spring-cf-turnstile on the classpath.

Token transport contract

The protected endpoints consume JSON request bodies, so the CAPTCHA token cannot be read from the body pre-handler (the interceptor runs before Spring MVC binds/parses the body). The token must be sent as either:

  • The X-Captcha-Token request header (preferred), or
  • The cf-turnstile-response query parameter (fallback — note that query strings reach access logs, proxy logs, and Referer headers)

On failure the API returns HTTP 403 with the same JSONResponse shape as other API errors: {"success":false,"redirectUrl":null,"code":8,"messages":["..."],"data":null}, message customizable via message.captcha.validation-failed. The body is produced by serializing an actual JSONResponse, so it cannot drift from the shape other endpoints return.

Provider SPI

CaptchaService is the provider-neutral extension point, shaped to match the existing RegistrationGuard SPI:

CaptchaVerification verify(CaptchaContext context);
default Optional<String> siteKey() { return Optional.empty(); }
default List<String> configurationWarnings() { return List.of(); }
default List<String> configurationErrors() { return List.of(); }

CaptchaContext carries the CaptchaAction being verified, the token, a client IP resolved once by the framework, and the request. Passing the action lets providers bind a token to the challenge it was issued for (reCAPTCHA v3 actions) or apply per-action score thresholds.

CaptchaVerification is a three-way outcome — VERIFIED / REJECTED / ERROR — rather than a boolean. That distinction is what makes fail-closed enforceable by the framework instead of by each implementer: a provider reports error(...) when it could not complete verification, and the framework decides that rejects the request. An implementation cannot accidentally fail open by returning true during an outage.

CaptchaAction is the single source of truth for the protected paths, used for interceptor registration, request matching, and the per-action toggles, so those three cannot drift apart.

Fail-closed behavior

  • enabled=true with no resolvable CaptchaService (Turnstile jar missing, unknown provider, no custom bean) fails application startup.
  • enabled=true with a provider that resolves but reports it cannot verify anything — missing Turnstile secret or site key — also fails startup, because such a provider rejects every request to every protected endpoint while the application otherwise looks healthy. Set allow-unusable-provider=true to start anyway and take a startup ERROR banner instead.
  • A provider outage, a provider that throws, or one that returns null all reject the request rather than letting it through. The framework wraps the SPI call, so a third-party implementation cannot break the documented 403 contract.
  • A missing/blank token, an unparseable request path, or a path matching no known action all reject.
  • Cloudflare's always-pass test keys are detected at startup (TurnstileValidationService.isUsingTestCredentials(), requires ds-spring-cf-turnstile 2.1.0+) and logged as a prominent WARN banner.

Startup validation runs in @PostConstruct rather than on ContextRefreshedEvent, so a consuming application that configures an async applicationEventMulticaster cannot silently discard the failure.

Path matching

Enforcement compiles its patterns with PathPatternParser.defaultInstance — the same engine MappedInterceptor uses for registration — and matches against the parsed request path. This closes a class of matcher-mismatch bypass where a variant like /user/registration;jsessionid=x or /user/%72egistration reaches the handler while a raw-URI string comparison misses it. Both variants have regression tests at the unit and full-stack level.

Observability

Rejections publish an AuditEvent (action=CaptchaValidation, actionStatus=Failure), so rejection volume is visible the same way every other API rejection is. The client IP is resolved once and used for both the provider call and the audit/log record, so they cannot disagree about who the client was. Sessions are not created for rejected requests.

Deliberately out of scope

Login has no CAPTCHA toggle. Login is handled by a Spring Security filter, not an MVC handler, so it falls outside this interceptor-based approach, and per-account lockout (user.security.failedLoginAttempts) already provides brute-force protection there. Consumers who want CAPTCHA on login can enable ds-spring-cf-turnstile's own login filter (ds.cf.turnstile.login.enabled=true) or use Cloudflare edge challenges.

Docs

  • README: new "CAPTCHA Protection (Cloudflare Turnstile)" section under Security Features — what it protects, setup, client contract, Thymeleaf widget-rendering snippet, custom-provider example, fail-closed semantics, scope notes.
  • CONFIG.md: property reference entries for the user.security.captcha.* properties.
  • CLAUDE.md: configuration property-group entry, and a corrected note that the CAPTCHA package is the one place using conditional auto-configuration annotations.

Verification

  • ./gradlew build — BUILD SUCCESSFUL (compile, full test suite, check).
  • All seven user.security.captcha.* properties confirmed present in the generated spring-configuration-metadata.json.
  • 79 CAPTCHA tests across nine classes: SPI contract, fail-closed paths (throwing provider, null-returning provider, absent provider, blank token, unknown path), per-action toggles, path-variant bypasses, startup failure and its escape hatch, both client-IP resolution paths, audit publication, and full-stack integration for both token transports.

Copilot AI lite review requested due to automatic review settings August 9, 2026 19:54
}
CaptchaService captchaService = captchaServiceProvider.getIfAvailable();
if (captchaService == null) {
log.error("CAPTCHA is enabled but no CaptchaService is available. Failing closed for {}.", path);

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 an opt-in CAPTCHA (Cloudflare Turnstile) protection layer to this library’s unauthenticated, email-sending UserAPI endpoints to reduce automated abuse while keeping the default behavior unchanged for existing consumers.

Changes:

  • Introduces a provider-neutral CaptchaService SPI plus Turnstile adapter, auto-configuration, startup validation, and an MVC interceptor that fail-closes with a JSONResponse-shaped 403.
  • Adds integration/unit tests covering enable/disable behavior, per-endpoint toggles, JSON failure shape, and Turnstile adapter behavior.
  • Updates docs and configuration reference; adds optional compileOnly/test dependency on ds-spring-cf-turnstile.

Reviewed changes

Copilot reviewed 20 out of 21 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
src/test/resources/application-test.properties Excludes Turnstile auto-config in tests to avoid global side effects when the dependency is present.
src/test/java/com/digitalsanctuary/spring/user/captcha/TurnstileCaptchaServiceTest.java Unit tests for Turnstile adapter behavior (fail-closed, site key, warnings).
src/test/java/com/digitalsanctuary/spring/user/captcha/CaptchaValidationInterceptorTest.java Unit tests for token resolution + 403 JSON response behavior.
src/test/java/com/digitalsanctuary/spring/user/captcha/CaptchaToggleIntegrationTest.java Integration test proving per-action toggles behave independently.
src/test/java/com/digitalsanctuary/spring/user/captcha/CaptchaSiteKeyControllerAdviceTest.java Tests MVC model attribute exposure for captchaSiteKey.
src/test/java/com/digitalsanctuary/spring/user/captcha/CaptchaProtectionIntegrationTest.java Full-context integration tests enforcing CAPTCHA on the three target endpoints.
src/test/java/com/digitalsanctuary/spring/user/captcha/CaptchaConfigPropertiesTest.java Ensures property defaults and kebab-case binding work as documented.
src/test/java/com/digitalsanctuary/spring/user/captcha/CaptchaAutoConfigurationTest.java Verifies auto-config activation rules and fail-fast startup behavior.
src/main/resources/META-INF/spring/org.springframework.boot.autoconfigure.AutoConfiguration.imports Registers the new CAPTCHA auto-configuration with Spring Boot.
src/main/resources/messages/dsspringusermessages.properties Adds localized default message key for CAPTCHA validation failure.
src/main/java/com/digitalsanctuary/spring/user/captcha/TurnstileCaptchaService.java Implements Turnstile-backed CaptchaService adapter with fail-closed semantics.
src/main/java/com/digitalsanctuary/spring/user/captcha/CaptchaValidationInterceptor.java Interceptor enforcing CAPTCHA token presence/validity and writing JSONResponse-shaped 403s.
src/main/java/com/digitalsanctuary/spring/user/captcha/CaptchaStartupValidator.java Startup validator enforcing “enabled must have provider” and surfacing provider warnings.
src/main/java/com/digitalsanctuary/spring/user/captcha/CaptchaSiteKeyControllerAdvice.java Exposes captchaSiteKey model attribute to MVC controllers when enabled.
src/main/java/com/digitalsanctuary/spring/user/captcha/CaptchaService.java Defines the provider-neutral CAPTCHA verification SPI.
src/main/java/com/digitalsanctuary/spring/user/captcha/CaptchaConfigProperties.java Adds user.security.captcha.* configuration properties and per-action toggles.
src/main/java/com/digitalsanctuary/spring/user/captcha/CaptchaAutoConfiguration.java Wires beans conditionally and registers the interceptor when enabled.
README.md Documents setup, client contract, widget rendering, and fail-closed behavior.
CONFIG.md Adds property reference entries and usage notes for user.security.captcha.*.
CLAUDE.md Updates the documented configuration property group list.
build.gradle Adds optional compileOnly and test dependency on ds-spring-cf-turnstile.

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

Comment on lines +40 to +44
@Bean
public CaptchaStartupValidator captchaStartupValidator(CaptchaConfigProperties captchaConfigProperties,
ObjectProvider<CaptchaService> captchaServiceProvider) {
return new CaptchaStartupValidator(captchaConfigProperties, captchaServiceProvider);
}
@claude

claude Bot commented Aug 9, 2026

Copy link
Copy Markdown

Review: Optional Turnstile CAPTCHA for unauthenticated email-sending API actions

Nicely scoped and defensively built feature. Went through the auto-configuration, SPI, interceptor, startup validator, and the six new test classes. Overall this is solid: fail-closed semantics are enforced consistently, the opt-in design keeps the "off by default, zero-cost" promise, and the test suite covers the interesting edge cases (missing token, invalid token, per-action toggles, missing classpath dependency, unknown provider, provider exceptions).

Strengths:

  • Fail-closed is actually enforced, not just claimed. CaptchaStartupValidator fails app startup if enabled=true but no CaptchaService resolves; TurnstileCaptchaService catches RuntimeException around every Turnstile call and returns false/null/a warning rather than propagating; the interceptor rejects on missing token, missing service, or failed verification. All the failure modes collapse to "reject the request," which is the right default for a CAPTCHA gate.
  • Zero-cost opt-out is real. TurnstileCaptchaConfiguration is gated on both @ConditionalOnClass(TurnstileValidationService.class) and the enabled=true property, so consumers without the Turnstile jar on the classpath, or with the feature off, get no beans, no interceptor, and no behavior change. Verified against AutoConfiguration.imports and the auto-config tests (shouldStartCleanlyWhenDisabledAndTurnstileClassAbsent).
  • Test-credential detection (isUsingTestCredentials() surfaced as a loud startup WARN) is a nice touch that will save someone from shipping Cloudflare's always-pass test keys to prod.
  • Test coverage is thorough: unit tests isolate the interceptor and the Turnstile adapter from Spring entirely (fast, deterministic), while the two full-context integration tests prove the acceptance criteria end-to-end, including "no email sent on reject" and independent per-action toggles. The isolated H2 DB names per integration test class and the SAME_THREAD execution plus mail-executor draining are a bit of test-infrastructure overhead, but the comments explain why, and it avoids flakiness from shared mutable test state.

Minor findings (non-blocking):

  1. Indentation inconsistency: CaptchaService.java, TurnstileCaptchaService.java, and TurnstileCaptchaServiceTest.java are tab-indented, while every other new file in this PR (CaptchaAutoConfiguration, CaptchaConfigProperties, CaptchaValidationInterceptor, CaptchaStartupValidator, CaptchaSiteKeyControllerAdvice, and the rest of the tests) uses 4 spaces, matching CLAUDE.md's documented style ("Indentation: 4 spaces"). Worth a quick reformat for consistency.
  2. The query-param fallback name is provider-specific: CaptchaValidationInterceptor.TOKEN_PARAMETER is hardcoded to cf-turnstile-response, even though CaptchaService is documented as a "provider-neutral SPI." A consumer plugging in a different provider via a custom CaptchaService bean only gets a vendor-neutral transport via the X-Captcha-Token header; the query-param fallback stays Turnstile-branded. Probably fine in practice (header is documented as preferred), but might be worth a one-line callout in the CaptchaService javadoc so it's clearly a Turnstile-specific fallback rather than part of the neutral contract.
  3. ERROR_CODE_CAPTCHA_FAILED = 8: no collision with existing JSONResponse codes in UserAPI (0-6 are used, 7 looks reserved). Just flagging that it'd be worth double-checking there isn't a reservation for 7/8 already spoken for by another in-flight feature, since these codes aren't centrally enumerated.

Things I checked and did not find issues with:

  • Path matching in the interceptor (getRequestURI() minus context path, exact-match switch) lines up with the addPathPatterns registration and is covered by a context-path test.
  • The interceptor only gates POST, matching the three @PostMapping handlers in UserAPI; no other HTTP verbs hit these paths.
  • Hand-rolled JSON in CaptchaValidationInterceptor.reject() (rather than using JSONResponse plus a message converter) mirrors the existing precedent in HtmxAwareAuthenticationEntryPoint, and the one dynamic field (the message) is escaped via StringEscapeUtils.escapeJson with a test asserting valid JSON even with embedded quotes.
  • No secrets or raw tokens are logged; only path/remote-addr on rejection.
  • TurnstileCaptchaConfiguration's "class present but bean absent" case (e.g., Turnstile's own auto-config excluded) does not fail startup, but that is intentional and covered: TurnstileCaptchaService.configurationWarnings() still surfaces a loud WARN in that case since the CaptchaService bean technically resolves.

Nothing here blocks merging. The indentation nit is the only thing I would actually ask to fix before merge; the other two are food for thought.

@claude

claude Bot commented Aug 9, 2026

Copy link
Copy Markdown

Review: Optional Turnstile CAPTCHA for unauthenticated email-sending API actions

Nice feature overall: off-by-default design, fail-closed startup validation, and a provider-neutral CaptchaService SPI are all solid choices, and the test suite is unusually thorough (auto-config wiring, per-action toggles, JSON escaping, adapter error paths). A few things worth addressing before merge, most importantly a likely bypass in the path-matching logic.

Potential bypass: interceptor compares the raw (encoded) request URI, not the decoded path

CaptchaValidationInterceptor.preHandle derives the path like this:

String path = request.getRequestURI().substring(request.getContextPath().length());
if (!isActionProtected(path)) {
    return true;
}

HttpServletRequest#getRequestURI() is documented to return the raw, percent-encoded URI, not the decoded path that Spring MVC actually routes on. Since isActionProtected() does an exact string match against literal constants (/user/registration, etc.), a request whose path differs only in encoding, for example POST /user/regist%72ation (percent-encoded r), or one carrying a path parameter like POST /user/registration;x=1, should still:

  1. Route to UserAPI.registration(...), because Spring handler mapping decodes/normalizes the path before matching @PostMapping("/registration").
  2. Still trigger this interceptor preHandle, since the addPathPatterns(...) registration that decides whether to invoke the interceptor uses the same decoded routing infrastructure.
  3. Fail isActionProtected() literal string match (because path here is still raw/encoded), fall through to default -> false, and return true from preHandle, skipping the CAPTCHA check entirely.

If this reasoning holds, an attacker could bypass CAPTCHA on registration, reset-password, and resend-token with a trivial encoding trick, undermining the anti-abuse goal of the PR. None of the new tests exercise an encoded or parameterized path (they all use plain literal paths), so this would not have been caught. Worth confirming in a real servlet container, and if confirmed, deriving the path the way the rest of the routing stack does instead of re-implementing it, e.g. UrlPathHelper.getInstance().getLookupPathForRequest(request), or the HandlerMapping.PATH_WITHIN_HANDLER_MAPPING_ATTRIBUTE request attribute that DispatcherServlet already sets before preHandle runs.

CaptchaSiteKeyControllerAdvice scope does not match its own doc comment

The javadoc says it is targeted at @Controller classes so REST responses are unaffected, but @ControllerAdvice(annotations = Controller.class) matches via AnnotatedElementUtils, which follows meta-annotations, and @RestController is itself meta-annotated with @Controller. So this advice ModelAttribute("captchaSiteKey") method should also run for @RestController beans, including UserAPI itself and, since there is no basePackages/assignableTypes restriction, any @RestController in the consuming application too.

This looks functionally harmless once CAPTCHA is enabled, since the model attribute is simply discarded for @ResponseBody handlers, but it does mean an extra captchaServiceProvider.getIfAvailable() plus getSiteKey() call on every REST request app-wide, which contradicts the stated intent. Worth either tightening the condition or correcting the comment.

Minor: hand-rolled JSON in CaptchaValidationInterceptor.reject()

The codebase already has com.digitalsanctuary.spring.user.util.JSONResponse, used by UserAPI, GdprAPI, and DevLoginController for exactly this response shape, serialized through the normal Jackson message-converter path. Here the same shape is built by hand-concatenating strings (correctly escaped via commons-text StringEscapeUtils.escapeJson, with a test proving it stays valid JSON even with embedded quotes), which works today but duplicates the response contract outside the type system, so it can silently drift if JSONResponse ever changes shape.

Other notes

  • build.gradle: compileOnly/testImplementation for ds-spring-cf-turnstile correctly follows the "library, not an app" dependency convention from CLAUDE.md.
  • Fail-closed behavior (missing provider fails startup, provider errors/exceptions reject the request, test-credential detection logged at WARN) is well thought through and well tested.
  • CaptchaService.java, TurnstileCaptchaService.java, and TurnstileCaptchaServiceTest.java are tab-indented while the rest of the new files use 4 spaces, which is what CLAUDE.md style guide documents; worth a quick reformat for consistency with the rest of this PR own files.
  • Docs (README/CONFIG.md/CLAUDE.md) are clear and give a concrete client-side snippet, which is helpful given the somewhat unusual header/query-param token contract forced by the JSON body constraint.

Nice work on the fail-closed philosophy and the provider-neutral SPI; the path-matching question above is the one thing worth nailing down before this ships, since if it holds it undermines the core guarantee the feature is meant to provide.

#346)

The interceptor was registered via PathPattern (which URL-decodes segments
and strips matrix parameters) but enforced via an exact string switch on
the raw request URI. Requests like POST /user/%72egistration or
/user/registration;a=b matched the registration pattern, ran the
interceptor, fell through the switch's default -> false, and reached the
email-sending handler with no CAPTCHA.

preHandle now matches the same parsed RequestPath the dispatcher cached
for handler mapping (falling back to an identical fresh parse) against
PathPatterns compiled with the same default parser the registration uses,
so enforcement cannot disagree with registration. Unparseable or
unmatched paths on an invoked interceptor fail closed as protected.

Verified empirically: the percent-encoded variant routes to the handler
through the real dispatcher (was 200 pre-fix, now 403); the matrix-param
variant is rejected pre-dispatch by StrictHttpFirewall (400) and is
additionally 403'd by the interceptor if a consumer relaxes the firewall.
@claude

claude Bot commented Aug 9, 2026

Copy link
Copy Markdown

Review: Optional Turnstile CAPTCHA (#347)

Nicely scoped feature — off by default, fail-closed throughout, path-matching bypass already found and fixed within the branch's own history, and solid test coverage (unit + full-context integration for both the happy path and the per-action toggles). A few observations, roughly ordered by importance:

1. CaptchaService bean can resolve to a permanently-nonfunctional adapter without failing startup

CaptchaStartupValidator only fails startup when captchaServiceProvider.getIfAvailable() returns null (CaptchaStartupValidator.java:486-493). But when ds-spring-cf-turnstile is on the classpath (@ConditionalOnClass(TurnstileValidationService.class) matches) and enabled=true, CaptchaAutoConfiguration.TurnstileCaptchaConfiguration.captchaService() always creates a TurnstileCaptchaService — even if the underlying TurnstileValidationService bean itself never registers (e.g. its own auto-config conditions aren't satisfied, sitekey/secret missing, or a consumer excludes TurnstileConfiguration as your own test's application-test.properties does). In that case the validator sees a non-null CaptchaService, logs a WARN banner via configurationWarnings(), and lets the app start — but TurnstileCaptchaService.verify() will return false forever, so every registration/reset-password/resend-token request 403s permanently.

That's fail-closed from a security standpoint, so it's not a vulnerability, but it's a foot-gun relative to the PR description's claim that "no resolvable CaptchaService... fails application startup" — this is a resolvable-but-dead CaptchaService, so a misconfigured deployment degrades silently into "registration/reset/resend are all broken" rather than failing fast the way the "jar missing entirely" case does. Worth considering whether CaptchaStartupValidator should also fail hard when configurationWarnings() reports the provider is unusable (vs. just the test-credentials warning), or at minimum calling this scenario out explicitly in the README's fail-closed section. It also isn't covered by CaptchaAutoConfigurationTest (which only tests the classloader-filtered "jar absent" case and the "unknown provider" case, not "jar present, bean absent").

2. Provider-neutral SPI leaks a Turnstile-specific fallback parameter name

CaptchaService/CaptchaValidationInterceptor are documented as provider-neutral (a consumer's own CaptchaService bean "takes precedence over the built-in one"), and the header contract (X-Captcha-Token) is appropriately generic. But the fallback query parameter is hardcoded to cf-turnstile-response (CaptchaValidationInterceptor.java:565), which is Cloudflare/Turnstile's own convention baked into the shared interceptor. A consumer plugging in a different provider via a custom CaptchaService bean only gets the generic header, not a matching fallback param — minor, but worth a doc note (or just drop the Turnstile-specific param name from the "provider-neutral" framing) so it doesn't surprise someone reading the SPI docs.

3. Hand-rolled JSON in the rejection response

CaptchaValidationInterceptor.reject() builds the 403 body via raw string concatenation (CaptchaValidationInterceptor.java:671-672) instead of reusing the framework's existing JSONResponse builder (used everywhere else in UserAPI) plus an injected ObjectMapper. You did correctly escape the one dynamic field with StringEscapeUtils.escapeJson (and there's a good regression test for the quoted-message case), so this isn't currently exploitable, but it's a maintenance risk — the pattern breaks silently (malformed JSON, or a reintroduced injection) the moment someone adds another interpolated field here without remembering to escape it. Since this interceptor runs ahead of Spring MVC's message conversion, you'd need to inject an ObjectMapper directly rather than return the object from a handler, but that's a small change and would remove the hand-rolled string entirely.

Everything else looked good

  • Path-matching parity between interceptor registration and enforcement (PathPatternParser.defaultInstance, reusing the DispatcherServlet-cached RequestPath) is a genuinely careful fix for the matrix-parameter/percent-encoding bypass class of bug, and it's well tested at both the unit (CaptchaValidationInterceptorTest) and integration level.
  • Fail-closed behavior is consistent everywhere it matters: missing token, missing provider bean, provider exceptions, and unknown provider config all reject rather than pass through.
  • compileOnly/testImplementation placement for ds-spring-cf-turnstile matches the library-not-app convention in CLAUDE.md, and the @ConditionalOnClass guard means the framework still runs without the dependency.
  • Per-action toggles and the CaptchaService bean override (@ConditionalOnMissingBean) are both exercised by dedicated tests.
  • Docs (README/CONFIG.md/CLAUDE.md) are thorough and match the implementation as far as I traced it, aside from the startup-failure nuance in point 1.

None of these are blockers IMO — #1 is the one I'd actually want a decision on before merge (fail harder vs. document the gap); #2 and #3 are cheap to address whenever.

Addresses the multi-agent PR review of #347.

SPI redesign:
- CaptchaService.verify now takes a CaptchaContext (action, token,
  framework-resolved client IP, request) and returns a three-way
  CaptchaVerification (VERIFIED/REJECTED/ERROR) instead of a boolean,
  so a provider outage is distinguishable from a bot and the framework,
  not the implementation, owns the fail-closed rule. Throwing or
  null-returning providers are caught and rejected with the documented
  403 body.
- Protected paths live on the CaptchaAction enum — single source of
  truth for registration, enforcement, and per-action toggles; unknown
  actions fail closed.

Scope and behavior:
- POST /user/registration/passwordless is now CAPTCHA-protected
  (protect.passwordless-registration, default true) — it sends a
  verification email for an unauthenticated caller like /registration.
- configurationErrors() SPI hook + startup gate: a resolvable provider
  that cannot verify anything (missing Turnstile secret or site key,
  absent service bean) now fails startup instead of booting into a 100%
  rejection rate; user.security.captcha.allow-unusable-provider=true
  downgrades that to an ERROR banner.
- Startup validation moved to @PostConstruct so an async event
  multicaster cannot swallow the fail-fast exception.
- Rejections publish AuditEvents, log the X-Forwarded-For client IP
  (matching what the provider is told), and serialize the real
  JSONResponse instead of a hand-built string.
- CaptchaSiteKeyControllerAdvice tolerates a throwing consumer
  provider instead of breaking every MVC page.

Tests:
- Per-action toggle coverage for every action, over-blocking probes
  (unprotected /user/savePassword and unknown paths stay untouched),
  advice wiring pinned in the full context, and a new wiring test that
  runs the real ds-spring-cf-turnstile auto-configuration instead of
  mocks (no network — verify is never called).
- Class-wide lenient Mockito strictness replaced with per-stub
  lenient(), which surfaced and removed one dead stub.

Docs: README (Thymeleaf-correct widget snippet, token reset on
failure, custom-provider guide), CONFIG.md, CLAUDE.md updated to match.
// Unreachable through the real dispatcher: the interceptor is registered against
// exactly the CaptchaAction paths, matched with this same engine. Reached only if a
// request path cannot be parsed, or if registration and enforcement ever disagree.
log.warn("CAPTCHA interceptor invoked for unrecognized path {} from {}. Rejecting (fail closed).", path,
// exactly the CaptchaAction paths, matched with this same engine. Reached only if a
// request path cannot be parsed, or if registration and enforcement ever disagree.
log.warn("CAPTCHA interceptor invoked for unrecognized path {} from {}. Rejecting (fail closed).", path,
clientIp);
}
String token = resolveToken(request);
if (token == null || token.isBlank()) {
log.warn("CAPTCHA token missing on {} from {}. Rejecting request.", path, clientIp);
}
String token = resolveToken(request);
if (token == null || token.isBlank()) {
log.warn("CAPTCHA token missing on {} from {}. Rejecting request.", path, clientIp);
Comment on lines +131 to +132
log.warn("CAPTCHA {} on {} from {}. Rejecting request. Detail: {}", verification.outcome(), path, clientIp,
verification.detail());
try {
path = resolvePathWithinApplication(request);
} catch (RuntimeException e) {
log.warn("Could not parse request path {}. Failing closed.", request.getRequestURI(), e);
@claude

claude Bot commented Aug 12, 2026

Copy link
Copy Markdown

Review: Optional Turnstile CAPTCHA for unauthenticated email-sending API actions

Went through the full diff (29 files, all four commits' worth of the current head state). Overall this is a very solid, well-engineered PR — noting up front that the second-to-last commit already addresses a prior multi-agent review round, which shows in the quality of the fail-closed design and the depth of the regression tests.

Strengths

  • Fail-closed design is genuinely robust. The three-way CaptchaVerification outcome (VERIFIED/REJECTED/ERROR) correctly moves the "what does an outage mean" decision from provider implementations to the framework, so a third-party CaptchaService can't accidentally fail open. verifySafely() in CaptchaValidationInterceptor also catches thrown exceptions and null returns from the SPI, so a misbehaving consumer implementation still gets the documented 403 contract.
  • The path-matching bypass fix (commit 3ca7026) is the standout piece of this PR. Registering the interceptor via PathPatternParser but originally enforcing via raw URI string comparison is an easy trap to fall into, and the fix (matching against the same cached RequestPath/PathPattern engine used for registration) is correct. The regression tests for both the percent-encoded (/user/%72egistration) and matrix-parameter (;jsessionid=x) variants, at both the unit and full-stack level, are exactly the right coverage for this class of bug.
  • Startup validation is well thought out. Distinguishing "no resolvable provider" (always fails startup) from "resolvable but unusable provider" (fails startup unless allow-unusable-provider=true) correctly targets the likeliest real misconfiguration — a missing Turnstile secret — that would otherwise silently produce a 100%-rejection-rate production deploy. Using @PostConstruct instead of ContextRefreshedEvent specifically to defeat async event multicasters is a nice catch.
  • Test coverage is exceptional — per-action toggle independence, both token transports, audit publication, the site-key controller advice degrading gracefully when a consumer provider throws, and TurnstileAutoConfigurationWiringTest validating against the real ds-spring-cf-turnstile auto-configuration rather than only mocks (catching drift a purely-mocked suite would miss).
  • Default-off behavior with no interceptor/provider beans registered when disabled keeps this a true zero-cost opt-in for existing consumers.

Minor / non-blocking observations

  1. X-Forwarded-For is trusted without a trusted-proxy check (CaptchaValidationInterceptor.resolveClientIp). The leftmost entry is taken as-is and used both for the audit log ipAddress and (via CaptchaContext.remoteIp()) the value reported to the CAPTCHA provider. Since it's client-suppliable, an app not sitting behind a proxy that overwrites this header could have its CAPTCHA audit trail (and Turnstile's remoteip scoring input) spoofed. This is called out as "advisory" in the docs and doesn't let a request bypass CAPTCHA outright, but it's worth a one-line callout in the README's Scope section alongside the existing PathPatternParser caveat, since operators reading the audit log for abuse investigation may not realize the IP is unverified.
  2. CaptchaSiteKeyControllerAdvice runs on every @Controller-annotated request, including @RestControllers (meta-annotation matching means annotations = Controller.class picks up @RestController too), invoking CaptchaService.siteKey() even for JSON API responses that never render a model attribute. This is already documented as intentional and the SPI docs ask implementers to cache/avoid I/O in siteKey(), so it's a documented contract rather than a bug — just flagging that nothing enforces that contract if a custom provider does I/O there.
  3. In TurnstileCaptchaService.configurationErrors(), if getTurnstileSitekey() throws, siteKey() swallows the exception internally (returns Optional.empty()), so the resulting error message says "no site key configured" rather than distinguishing "site key retrieval failed." Cosmetic — the outcome (fail startup) is correct either way, just a slightly misleading message for that specific edge case.

Bugs / security / performance

No functional bugs found. No security concerns beyond the advisory IP-trust note above (which is already scoped correctly — it can't be used to bypass verification, only to skew logging/scoring). Performance impact is negligible: an EnumMap lookup plus one provider call per protected POST, and the design explicitly avoids adding overhead when the feature is disabled.

Nice work — this is ready modulo maintainer preference on the two documentation nits above.

@devondragon
devondragon merged commit 7dd430b into main Aug 12, 2026
11 checks passed
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.

Optional Turnstile CAPTCHA verification for the unauthenticated email-sending API actions

3 participants