Share captcha validation policy#1121
Open
BenjaminMichaelis wants to merge 8 commits into
Open
Conversation
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Pull request overview
Consolidates per-page hCaptcha handling into a single ICaptchaValidationService that returns explicit outcomes (Disabled / MissingToken / Unavailable / Invalid / Valid), and updates chat plus the Identity pages (Login, Register, ForgotPassword, ResetPassword, ResendEmailConfirmation) to use it. Register retains its detailed hCaptcha error-code mapping; other callers use a generic message. Adds TUnit tests for the new service.
Changes:
- New shared service
CaptchaValidationService, result record, and outcome enum centralizing captcha policy. - Identity pages and
ChatControllermigrated off directICaptchaServiceusage to the shared validation layer. - Added
CaptchaValidationServiceTestscovering disabled, missing token, unavailable, invalid, and valid paths.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| EssentialCSharp.Web/Services/ICaptchaValidationService.cs | New interface with two ValidateAsync overloads. |
| EssentialCSharp.Web/Services/CaptchaValidationService.cs | Implements the shared policy: disabled / missing / unavailable / invalid / valid. |
| EssentialCSharp.Web/Services/CaptchaValidationResult.cs | Result record exposing ShouldProceed. |
| EssentialCSharp.Web/Services/CaptchaValidationOutcome.cs | Enum of validation outcomes. |
| EssentialCSharp.Web/Extensions/IServiceCollectionExtensions.cs | Registers ICaptchaValidationService as singleton. |
| EssentialCSharp.Web/Controllers/ChatController.cs | Replaces inline captcha logic with shared service; preserves 503/403 responses and logging. |
| EssentialCSharp.Web/Areas/Identity/Pages/Account/Login.cshtml.cs | Switches to shared validator; keeps generic failure message. |
| EssentialCSharp.Web/Areas/Identity/Pages/Account/Register.cshtml.cs | Restructures captcha branch to use outcome-based switch while retaining detailed error mapping. |
| EssentialCSharp.Web/Areas/Identity/Pages/Account/ForgotPassword.cshtml.cs | Uses shared validator with generic failure message. |
| EssentialCSharp.Web/Areas/Identity/Pages/Account/ResetPassword.cshtml.cs | Uses shared validator with generic failure message. |
| EssentialCSharp.Web/Areas/Identity/Pages/Account/ResendEmailConfirmation.cshtml.cs | Uses shared validator with generic failure message. |
| EssentialCSharp.Web.Tests/CaptchaValidationServiceTests.cs | TUnit tests for all five outcomes via a stub captcha service. |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
EssentialCSharp.Web/Areas/Identity/Pages/Account/Login.cshtml.cs:75
- This change quietly alters behavior for the identity pages when captcha configuration is missing. Previously,
CaptchaService.VerifyAsyncthrewInvalidOperationExceptionifSecretKey/SiteKeywere null (CaptchaService.cs:36-37), which surfaced misconfiguration loudly. Now theDisabledoutcome falls through this check and the user simply sees "Human verification failed. Please try again." with no log entry, making misconfiguration silently break sign-in/forgot-password/etc. Consider either explicitly handlingCaptchaValidationOutcome.Disabledwith a dedicated log (similar toLogCaptchaConfigurationMissinginChatController) or with a distinct user-facing message so this state is observable. The same concern applies toForgotPassword.cshtml.cs,ResendEmailConfirmation.cshtml.cs, andResetPassword.cshtml.cs.
CaptchaValidationResult captchaResult = await captchaValidationService.ValidateAsync(captchaToken, HttpContext.Connection.RemoteIpAddress?.ToString());
if (!captchaResult.ShouldProceed)
{
ModelState.AddModelError(string.Empty, "Human verification failed. Please try again.");
ExternalLogins = (await signInManager.GetExternalAuthenticationSchemesAsync()).ToList();
return Page();
…er-giggle # Conflicts: # EssentialCSharp.Web/Areas/Identity/Pages/Account/Login.cshtml.cs # EssentialCSharp.Web/Areas/Identity/Pages/Account/Register.cshtml.cs
- Remove invalid namespace import (Microsoft.Extensions.Http.Resilience doesn't export a Resilience namespace) - Add missing OpenAI NuGet package (v2.10.0) for ResponsesClient and CreateResponseOptions types - Fix syntax error in Register.cshtml.cs merge conflict resolution by removing unreachable code - Ensure compatibility with Microsoft.Extensions.AI.OpenAI dependency chain Fixes CodeQL and build failures on PR #1121.
…mentation - Accept 3254200755: Remove dead code overload from StubCaptchaService - Accept 3254200763: Fail closed when captcha is disabled in config (add error page) - Accept 3254200766: Document stricter validation requiring both keys - Accept 3256514826: Replace unreachable null check with assertion - Accept 3256514865: Extract helper methods to eliminate 54+ lines duplication - Accept 3254200636: Add explicit null guard for details variable Changes: - ChatController: Extracted HandleCaptchaValidationFailure helpers for both endpoints - Register.cshtml.cs: Added Disabled outcome handler, null assertion, explicit guard - CaptchaValidationService: Added XML documentation for behavior clarity - CaptchaValidationServiceTests: Removed unreachable dead code overload All tests passing (111 pass), build clean.
- enforce fail-closed fallback for unhandled non-proceeding captcha outcomes - keep defensive handling when Invalid outcome lacks response payload - remove redundant null-conditional access after non-null guard - consolidate chat disabled/unavailable handling into shared helpers - clarify captcha configuration completeness check in validation service
- ignore local test output artifact to prevent noisy diffs - cache chat captcha unavailable response payload - add tests for partial captcha configuration guard paths
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.
This change removes the per-page captcha handling drift and centralizes the policy in one shared validation service.
What changed
ICaptchaValidationServicewith explicit outcomes for missing token, unavailable, invalid, and valid captcha states.Notes
ICaptchaServicestill owns raw hCaptcha verification.