Share captcha validation policy#1121
Open
BenjaminMichaelis wants to merge 6 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. |
Comment on lines
+109
to
+111
| public Task<HCaptchaResult?> VerifyAsync(string secret, string response, string sitekey, CancellationToken cancellationToken = default) | ||
| => throw new NotSupportedException(); | ||
|
|
Member
Author
There was a problem hiding this comment.
Accepted - the dead code overload is unreachable through ICaptchaService, so removing it improves clarity of the stub without affecting any tests. The two interface-defined overloads remain untouched.
| } | ||
|
|
||
| if (string.IsNullOrEmpty(hCaptcha_response)) | ||
| CaptchaValidationResult captchaResult = await captchaValidationService.ValidateAsync(hCaptcha_response, HttpContext.Connection.RemoteIpAddress?.ToString()); |
Member
Author
There was a problem hiding this comment.
Accepted - added explicit handling for the Disabled outcome to fail closed: when captcha config is missing, the registration flow now returns an error page instead of silently proceeding. This includes a warning log and user-facing error message for configuration drift visibility.
Comment on lines
+15
to
+17
| if (string.IsNullOrWhiteSpace(Options.SecretKey) || string.IsNullOrWhiteSpace(Options.SiteKey)) | ||
| return new CaptchaValidationResult(CaptchaValidationOutcome.Disabled, null); | ||
|
|
Member
Author
There was a problem hiding this comment.
Accepted - the stricter validation requiring both SecretKey and SiteKey is intentional and correct since HCaptcha requires both keys to function. Added documentation to CaptchaValidationService explaining this behavior and signaling it as a deployment consideration.
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(); | ||
| } |
Member
Author
There was a problem hiding this comment.
Accepted - replaced the unreachable null check with ArgumentNullException.ThrowIfNull to express the invariant that Response is never null for Invalid outcomes, making the contract clearer and aiding static analyzers.
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" }); |
Member
Author
There was a problem hiding this comment.
Accepted - extracted a reusable HandleCaptchaValidationFailure helper method (plus async variant for StreamMessage) to eliminate 54+ lines of duplication. Both Disabled and Unavailable outcomes now use the same error response format with appropriate logging.
…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.
Comment on lines
+92
to
+104
| CaptchaValidationResult captchaResult = await captchaValidationService.ValidateAsync(hCaptcha_response, HttpContext.Connection.RemoteIpAddress?.ToString()); | ||
| if (!captchaResult.ShouldProceed) | ||
| { | ||
| ModelState.AddModelError(string.Empty, HCaptchaErrorDetails.GetValue(HCaptchaErrorDetails.MissingInputResponse).FriendlyDescription); | ||
| return Page(); | ||
| } | ||
|
|
||
| HCaptchaResult? response = await captchaService.VerifyAsync(hCaptcha_response, HttpContext.Connection.RemoteIpAddress?.ToString()); | ||
|
|
||
| if (response is null) | ||
| { | ||
| ModelState.AddModelError(string.Empty, "Captcha verification is temporarily unavailable. Please try again later."); | ||
| return Page(); | ||
| } | ||
|
|
||
| // The JSON should also return a field "success" as true | ||
| // https://docs.hcaptcha.com/#verify-the-user-response-server-side | ||
| if (response.Success) | ||
| { | ||
| EssentialCSharpWebUser user = CreateUser(); | ||
| user.FirstName = Input.FirstName; | ||
| user.LastName = Input.LastName; | ||
|
|
||
| await userStore.SetUserNameAsync(user, Input.UserName, CancellationToken.None); | ||
| await emailStore.SetEmailAsync(user, Input.Email, CancellationToken.None); | ||
| if (Input.Password is null) | ||
| if (captchaResult.Outcome == CaptchaValidationOutcome.MissingToken) | ||
| { | ||
| LogPasswordNull(logger); | ||
| ModelState.AddModelError(string.Empty, "Error: Password null; please enter in a password"); | ||
| ModelState.AddModelError(string.Empty, HCaptchaErrorDetails.GetValue(HCaptchaErrorDetails.MissingInputResponse).FriendlyDescription); | ||
| return Page(); | ||
| } | ||
| IdentityResult result = await userManager.CreateAsync(user, Input.Password); | ||
|
|
||
| if (result.Succeeded) | ||
| if (captchaResult.Outcome == CaptchaValidationOutcome.Unavailable) | ||
| { | ||
| LogUserCreatedWithPassword(logger); | ||
|
|
||
| string userId = await userManager.GetUserIdAsync(user); | ||
| string code = await userManager.GenerateEmailConfirmationTokenAsync(user); | ||
| code = WebEncoders.Base64UrlEncode(Encoding.UTF8.GetBytes(code)); | ||
| string? callbackUrl = Url.Page( | ||
| "/Account/ConfirmEmail", | ||
| pageHandler: null, | ||
| values: new { area = "Identity", userId = userId, code = code, returnUrl = returnUrl }, | ||
| protocol: Request.Scheme); | ||
|
|
||
| if (callbackUrl is null) | ||
| { | ||
| ModelState.AddModelError(string.Empty, "Error: callback url unexpectedly null."); | ||
| return Page(); | ||
| } | ||
| if (Input.Email is null) | ||
| ModelState.AddModelError(string.Empty, "Captcha verification is temporarily unavailable. Please try again later."); | ||
| return Page(); | ||
| } |
…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.
| ModelState.AddModelError(string.Empty, "Captcha verification is temporarily unavailable. Please try again later."); | ||
| return Page(); | ||
| default: | ||
| LogHCaptchaUnknownErrorCode(logger, details?.ErrorCode); |
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.