Share captcha validation policy#1121
Open
BenjaminMichaelis wants to merge 3 commits into
Open
Conversation
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
| { | ||
| if (HCaptchaErrorDetails.TryGetValue(response.ErrorCodes.Single(), out HCaptchaErrorDetails? details)) | ||
| { | ||
| switch (details.ErrorCode) |
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. |
Comment on lines
+109
to
+111
| public Task<HCaptchaResult?> VerifyAsync(string secret, string response, string sitekey, CancellationToken cancellationToken = default) | ||
| => throw new NotSupportedException(); | ||
|
|
| } | ||
|
|
||
| if (string.IsNullOrEmpty(hCaptcha_response)) | ||
| CaptchaValidationResult captchaResult = await captchaValidationService.ValidateAsync(hCaptcha_response, HttpContext.Connection.RemoteIpAddress?.ToString()); |
Comment on lines
+15
to
+17
| if (string.IsNullOrWhiteSpace(Options.SecretKey) || string.IsNullOrWhiteSpace(Options.SiteKey)) | ||
| return new CaptchaValidationResult(CaptchaValidationOutcome.Disabled, null); | ||
|
|
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();
Comment on lines
+110
to
115
| if (response is null) | ||
| { | ||
| ModelState.AddModelError(string.Empty, "Error: Email may not be null."); | ||
| LogHCaptchaNullErrorCodes(logger); | ||
| ModelState.AddModelError(string.Empty, "Captcha verification failed. Please try again."); | ||
| return Page(); | ||
| } |
Comment on lines
+49
to
66
| if (captchaValidation.Outcome == CaptchaValidationOutcome.Disabled) | ||
| { | ||
| LogCaptchaConfigurationMissing(_Logger); | ||
| return StatusCode(503, new { error = "Human verification is temporarily unavailable. Please try again later.", errorCode = "captcha_unavailable" }); | ||
| } | ||
|
|
||
| if (captchaValidation.Outcome == CaptchaValidationOutcome.Unavailable) | ||
| { | ||
| LogCaptchaServiceUnavailable(_Logger); | ||
| return StatusCode(503, new { error = "Human verification is temporarily unavailable. Please try again later.", errorCode = "captcha_unavailable" }); | ||
| } | ||
|
|
||
| if (captchaValidation.Outcome == CaptchaValidationOutcome.Invalid) | ||
| { | ||
| LogCaptchaValidationFailed(_Logger, string.Join(',', captchaValidation.Response?.ErrorCodes ?? [])); | ||
| } | ||
|
|
||
| return StatusCode(403, new { error = "Human verification required.", errorCode = "captcha_failed" }); |
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.