Optional Turnstile CAPTCHA for unauthenticated email-sending API actions - #347
Conversation
| } | ||
| CaptchaService captchaService = captchaServiceProvider.getIfAvailable(); | ||
| if (captchaService == null) { | ||
| log.error("CAPTCHA is enabled but no CaptchaService is available. Failing closed for {}.", path); |
There was a problem hiding this comment.
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
CaptchaServiceSPI plus Turnstile adapter, auto-configuration, startup validation, and an MVC interceptor that fail-closes with aJSONResponse-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 onds-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.
| @Bean | ||
| public CaptchaStartupValidator captchaStartupValidator(CaptchaConfigProperties captchaConfigProperties, | ||
| ObjectProvider<CaptchaService> captchaServiceProvider) { | ||
| return new CaptchaStartupValidator(captchaConfigProperties, captchaServiceProvider); | ||
| } |
|
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:
Minor findings (non-blocking):
Things I checked and did not find issues with:
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. |
Review: Optional Turnstile CAPTCHA for unauthenticated email-sending API actionsNice feature overall: off-by-default design, fail-closed startup validation, and a provider-neutral Potential bypass: interceptor compares the raw (encoded) request URI, not the decoded path
String path = request.getRequestURI().substring(request.getContextPath().length());
if (!isActionProtected(path)) {
return true;
}
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. CaptchaSiteKeyControllerAdvice scope does not match its own doc comment The javadoc says it is targeted at This looks functionally harmless once CAPTCHA is enabled, since the model attribute is simply discarded for Minor: hand-rolled JSON in CaptchaValidationInterceptor.reject() The codebase already has Other notes
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.
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.
|
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); |
| 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); |
Review: Optional Turnstile CAPTCHA for unauthenticated email-sending API actionsWent 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
Minor / non-blocking observations
Bugs / security / performanceNo 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 Nice work — this is ready modulo maintainer preference on the two documentation nits above. |
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, andPOST /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(defaultfalse) — master switch.user.security.captcha.provider(defaultturnstile) — onlyturnstileis built in; a consumer-suppliedCaptchaServicebean takes precedence over it.user.security.captcha.allow-unusable-provider(defaultfalse) — 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(defaulttrueeach) — per-action toggles, effective only whenenabled=true.With
enabled=false, no CAPTCHA interceptor or provider beans register — noCaptchaValidationInterceptor, noCaptchaService, nocaptchaSiteKeymodel attribute, and no requests are inspected. (CaptchaConfigPropertiesandCaptchaStartupValidatordo always register; the validator early-returns when disabled.) The framework compiles and runs withoutds-spring-cf-turnstileon 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:
X-Captcha-Tokenrequest header (preferred), orcf-turnstile-responsequery parameter (fallback — note that query strings reach access logs, proxy logs, andRefererheaders)On failure the API returns
HTTP 403with the sameJSONResponseshape as other API errors:{"success":false,"redirectUrl":null,"code":8,"messages":["..."],"data":null}, message customizable viamessage.captcha.validation-failed. The body is produced by serializing an actualJSONResponse, so it cannot drift from the shape other endpoints return.Provider SPI
CaptchaServiceis the provider-neutral extension point, shaped to match the existingRegistrationGuardSPI:CaptchaContextcarries theCaptchaActionbeing 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.CaptchaVerificationis 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 reportserror(...)when it could not complete verification, and the framework decides that rejects the request. An implementation cannot accidentally fail open by returningtrueduring an outage.CaptchaActionis 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=truewith no resolvableCaptchaService(Turnstile jar missing, unknown provider, no custom bean) fails application startup.enabled=truewith 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. Setallow-unusable-provider=trueto start anyway and take a startup ERROR banner instead.TurnstileValidationService.isUsingTestCredentials(), requiresds-spring-cf-turnstile2.1.0+) and logged as a prominentWARNbanner.Startup validation runs in
@PostConstructrather than onContextRefreshedEvent, so a consuming application that configures an asyncapplicationEventMulticastercannot silently discard the failure.Path matching
Enforcement compiles its patterns with
PathPatternParser.defaultInstance— the same engineMappedInterceptoruses for registration — and matches against the parsed request path. This closes a class of matcher-mismatch bypass where a variant like/user/registration;jsessionid=xor/user/%72egistrationreaches 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 enableds-spring-cf-turnstile's own login filter (ds.cf.turnstile.login.enabled=true) or use Cloudflare edge challenges.Docs
user.security.captcha.*properties.Verification
./gradlew build— BUILD SUCCESSFUL (compile, full test suite, check).user.security.captcha.*properties confirmed present in the generatedspring-configuration-metadata.json.