2.1.0: opt-in login filter, test-credential detection, overridable beans - #107
Open
devondragon wants to merge 12 commits into
Open
2.1.0: opt-in login filter, test-credential detection, overridable beans#107devondragon wants to merge 12 commits into
devondragon wants to merge 12 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Implements the 2.1.0 auto-configuration hygiene work for SpringCFTurnstile: makes the login captcha filter opt-in, adds test-credential detection (with a startup WARN banner), and allows consumers to override the library’s key beans without conflicts.
Changes:
- Gate
TurnstileCaptchaFilterbehindds.cf.turnstile.login.enabled=true(default off) and add metadata/tests/docs for the opt-in behavior. - Add
TurnstileValidationService.isUsingTestCredentials()plus a startup WARN banner, with unit tests covering Cloudflare’s documented test credentials. - Make
TurnstileValidationService(type-based) andturnstileRestClient(name-based) beans overridable via@ConditionalOnMissingBean, with dedicated context-runner regression tests.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/main/java/com/digitalsanctuary/cf/turnstile/filter/TurnstileCaptchaFilter.java | Makes the servlet filter opt-in via @ConditionalOnProperty. |
| src/main/java/com/digitalsanctuary/cf/turnstile/service/TurnstileValidationService.java | Adds Cloudflare test-credential detection and a startup WARN banner. |
| src/main/java/com/digitalsanctuary/cf/turnstile/config/TurnstileServiceConfig.java | Adds @ConditionalOnMissingBean to allow consumer overrides (type-based for service; name-based for RestClient). |
| src/main/resources/META-INF/additional-spring-configuration-metadata.json | Documents the new ds.cf.turnstile.login.enabled property for IDE metadata/completion. |
| src/test/java/com/digitalsanctuary/cf/test/turnstile/TurnstileValidationServiceTest.java | Adds tests for isUsingTestCredentials() across published test keys/secrets. |
| src/test/java/com/digitalsanctuary/cf/test/turnstile/TurnstileBeanOverrideTest.java | Adds context tests verifying bean override/backoff behavior for service + RestClient. |
| src/test/java/com/digitalsanctuary/cf/test/turnstile/filter/TurnstileCaptchaFilterTest.java | Updates existing filter test to explicitly enable the filter property. |
| src/test/java/com/digitalsanctuary/cf/test/turnstile/filter/TurnstileCaptchaFilterOptInTest.java | Adds tests verifying the filter is not registered by default and is registered when enabled. |
| README.md | Updates docs for 2.1.0, opt-in filter behavior, and test-credential detection. |
| docs/RELEASE-NOTES-2.1.0.md | Adds upgrade/release notes calling out the opt-in behavior change and other additions. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+171
to
+176
| private TurnstileValidationService buildServiceWithCredentials(String sitekey, String secret) { | ||
| TurnstileConfigProperties testProperties = new TurnstileConfigProperties(); | ||
| testProperties.setSitekey(sitekey); | ||
| testProperties.setSecret(secret); | ||
| return new TurnstileValidationService(null, testProperties, new NoOpTurnstileMetrics()); | ||
| } |
Comment on lines
+102
to
+108
| if (isUsingTestCredentials()) { | ||
| log.warn("========================================================"); | ||
| log.warn("Turnstile is configured with Cloudflare TEST credentials."); | ||
| log.warn("Captcha validation is running in test mode and provides NO protection."); | ||
| log.warn("Do not use these credentials in production."); | ||
| log.warn("========================================================"); | ||
| } |
The missing-secret/missing-URL ERROR logs and the Cloudflare test-credential WARN banner lived in TurnstileValidationService.onStartup(), so a consumer supplying their own service bean silently lost all three checks. They now live in a new TurnstileStartupReporter registered unconditionally by TurnstileConfiguration. Also: - Correct the banner wording. It claimed test credentials provide "NO protection", which is wrong for the always-fail keys (2x sitekeys, 2x/3x secrets) — those block every user. The banner now says validation will always pass or always fail depending on the key. - Log the login captcha filter registration state at INFO, so an app that lost the filter in the 2.1.0 opt-in change can see that from its startup log. - Add TurnstileValidationService.isTestCredentials(String, String) holding the detection logic; the existing public isUsingTestCredentials() instance method delegates to it and is unchanged for callers. Covered by TurnstileStartupReporterTest (logback ListAppender): banner fires for test sitekey and test secret, does not fire for real-looking credentials, and the filter state line reports ENABLED/DISABLED in both directions.
TurnstileCaptchaFilter read its three settings through @value placeholders, which do not do relaxed binding. The kebab-case names shipped in additional-spring-configuration-metadata.json (ds.cf.turnstile.login.submission-path, login.redirect-url, token.parameter-name) therefore never reached the filter — the IDE offered them, the defaults silently won. They are now nested Login/Token classes on TurnstileConfigProperties, following the existing Metrics pattern, and the filter injects the properties bean. Backward compatible: the camelCase forms from the old README still bind via relaxed binding, and kebab-case now works too instead of being silently dead. Removed the camelCase defaults from config/turnstile.properties; defaults now come from the field initializers. Also add ConfigurationPropertiesAutoConfiguration to the context-runner tests. Without it those runners never bound @ConfigurationProperties at all, so any property assertion in them was vacuous. Covered by TurnstileCaptchaFilterOptInTest: defaults when unset, and binding from kebab-case and camelCase property names.
isUsingTestCredentials() was only observable from the startup log, which is gone by the time anyone looks at a running deployment. The turnstile health contributor now reports it as a usingTestCredentials detail alongside the existing counters. Adds TurnstileHealthIndicatorTest (there was no test for this class): the detail in both directions, the DOWN path for a missing secret, and the DOWN path for an error rate over threshold.
) Release notes: - Spell out the silent case of the filter opt-in change: an app that relied on servlet auto-registration and never injected the filter gets no startup error, just unchecked login POSTs. Note the two mitigations (the new startup INFO line, and that the auto-registered filter ran behind the Spring Security chain so form-login apps were not effectively protected by it anyway). - Temper the override claim. Health and metrics do not automatically follow a consumer subclass: the counters only move when the library's validation methods run, so a subclass that does not delegate to super leaves health permanently UP with zero counts. Recommend delegating or overriding the getters. - Document the health detail, the unconditional startup reporter, and the kebab-case property fix. README: - The filter config block is labeled "only needed if using TurnstileCaptchaFilter" but showed enabled: false, so copy-pasting it produced a non-working setup. Set it to true and note the default. - Fix the mechanism wording: the bean is not created unless the property is true (and direct injection then fails startup) — it is not present-but-inert. - Use kebab-case in the property examples, with a note that camelCase still binds.
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
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.
Implements #106 for the 2.1.0 release. Closes #106.
Changes
TurnstileCaptchaFilteris now opt-in — registers only whends.cf.turnstile.login.enabled=true(new property, defaultfalse, with config metadata). This is the release's one intentional behavior change: previously the filter auto-registered whenever the jar was on the classpath, intercepting POSTs to/login. Apps that inject the filter directly (per the old README pattern) will fail startup withNoSuchBeanDefinitionExceptionuntil the property is set — called out in the release notes.TurnstileValidationService.isUsingTestCredentials()— returns true when the configured sitekey or secret is one of Cloudflare's published test credentials, plus a WARN banner inonStartup()so always-pass test keys can't reach production unnoticed. All 8 documented test keys individually covered by tests.@ConditionalOnMissingBean(TurnstileValidationService.class)(type-based) on the service;@ConditionalOnMissingBean(name = "turnstileRestClient")(deliberately name-based — a type-based condition would back off on any unrelated consumerRestClientbean). Both behaviors pinned by context tests, including the regression case where an unrelatedRestClientbean must NOT suppress the library's client.docs/RELEASE-NOTES-2.1.0.mdwith the upgrade callout.No changes to the validation API:
validateTurnstileResponse(...),validateTurnstileResponseDetailed(...),getClientIpAddress(...), andgetTurnstileSitekey()are untouched. Downstream, SpringUserFramework #346 compiles againstisUsingTestCredentials()— the signature is a contract.Deliberately not included
A master
ds.cf.turnstile.enabledswitch (issue #106 listed it as "worth considering"): skipped as YAGNI for this release — downstream doesn't need it andspring.autoconfigure.excluderemains available.Verification
./gradlew buildgreen (32 tests passing, JDK 17 and 21 test tasks). Work was implemented task-by-task with per-task spec/quality reviews and a final whole-branch review; the final review's one Important finding (RestClient override behavior untested) was fixed and re-reviewed.After merge, release with:
Second review wave (external findings)
A follow-up review pass produced verified findings, all applied:
TurnstileStartupReporterbean — the missing-secret/URL errors and the test-credential banner previously lived onTurnstileValidationService, which this PR made overridable, so overrides could silently lose them. The reporter also logs the login filter's registration state (ENABLED/DISABLED) at startup, covering the silent upgrade path for apps that relied on servlet-container auto-registration.2x/3xtest keys always fail (locking out all users), so the banner now says "always pass (NO bot protection) or always fail (ALL users blocked)" instead of only "provides NO protection". The WARN is now covered by tests in both directions (issue Auto-configuration hygiene for 2.1.0: opt-in login filter, test-credential detection, overridable beans #106 acceptance criterion 3).@ValueontoTurnstileConfigProperties(login.submission-path,login.redirect-url,token.parameter-name): the config metadata has always advertised kebab-case keys, but@Valueplaceholders do exact-key lookup, so kebab-case settings were silently dead (and shadowed by the library's own defaults). Relaxed binding now makes both kebab-case and the old camelCase forms work — both pinned by tests. Defaults moved to field initializers;config/turnstile.propertiescamelCase entries removed.usingTestCredentialsdetail, so the machine-readable health surface no longer reads maximally healthy while running on always-pass keys.ConfigurationPropertiesAutoConfiguration(binding was previously a no-op in those tests), plus a real-subclass override test and a pin that the startup reporter stays unconditional under consumer overrides.enabled: true, bean-not-created mechanism wording, tempered health/metrics override claim).50 tests green on JDK 17 and 21.