Uh oh!
There was an error while loading. Please reload this page.
Add kaptcha verification to signup module - #638
Conversation
* Added kaptchaText field to SignupForm * SignUpController.SignUpApiAction.execute() now checks the session kaptcha value, rejects mismatches, and clears the attribute on success * Populated error_message in the JSON response for invalid-email and validateSignupForm error paths so the wiki form can display them See ai/todos/active/TODO-LK-20260517_add-kaptcha-to-signup.md Co-Authored-By: Claude <noreply@anthropic.com>
* Extracted verifyCaptcha, parseAndValidateEmail, sendConfirmationEmail helpers; both actions now run the same sequence (captcha -> blank fields -> email format -> userExists -> send) * Added emailConfirm field to SignupForm; validateSignupForm now also checks blank + match * sendConfirmationEmail wraps the TempUser insert + sendEmail in a DbScope transaction so the row rolls back on send failure * SignUpApiAction now also runs EmailValidator.isValid (closes a real divergence from BeginAction) and no longer falls through to status=USER_ADDED after a sendEmail failure * Rewrote signupPage.jsp using <labkey:form>/<labkey:input> with Confirm Email and kaptcha sections See ai/todos/active/TODO-LK-20260517_add-kaptcha-to-signup.md Co-Authored-By: Claude <noreply@anthropic.com>
ERROR: A pull request from |
- Catch ConfigurationException from sendEmail (was only catching MessagingException) - Add status="ERROR" to API captcha-failure response for consistency with other response paths - Remove hardcoded "six characters" from kaptcha instruction text - Add aria-label to kaptcha input field - Use method reference in errorsToMessages stream Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Adds bot protection and improves correctness of the self-sign-up flow by introducing server-side captcha verification and aligning validation + email delivery handling across both the form POST and API signup paths.
Changes:
- Added server-side captcha verification and shared helper methods used by both
BeginActionandSignUpApiAction. - Added
emailConfirmfield and related validation to reduce email-entry mistakes. - Wrapped TempUser creation + confirmation email sending in a transaction to avoid persisting rows when email delivery fails; refactored signup JSP to use LabKey taglibs.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| signup/src/org/labkey/signup/signupPage.jsp | Reworked signup form markup to use <labkey:form> / <labkey:input>, added confirm-email field and captcha UI with reload behavior. |
| signup/src/org/labkey/signup/SignUpController.java | Added captcha verification + shared validation helpers, added confirm-email field support, and refactored email send + TempUser insert to be transactional. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
* SignUpApiAction.execute: add status="ERROR" to blank-field, bad-email,
and send-failure error paths (was inconsistent with captcha path)
* verifyCaptcha: use parameterized logging instead of string concatenation
* verifyCaptcha: fix typo in comment ("a a" -> "a")
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>| { | ||
| errors.reject(ERROR_MSG, e.getMessage()); | ||
| } | ||
| return false; |
There was a problem hiding this comment.
Do this with caution. Failing to commit() a transaction can leave things in an unwanted state. I think you'll be OK here because there's no wrapping transaction that will try to continue and later commit.
There was a problem hiding this comment.
You are right that the callers of this method don't have a wrapping transaction. But to be safe, I
refactored createUserAndSendEmail to throw the exceptions instead of catching internally. Both callers catch the exception and build their own error response.
Fixed in 5fe8e95
…eption | ConfigurationException; callers handle exception. - verifyCaptcha no longer clears the session attribute. Captcha stays valid for retry if a later step fails (matching LoginController's RegisterUserAction pattern) - Added new clearCaptcha() that is called only after the full operation succeeds in BeginAction and SignUpApiAction - validateSignupForm uses equalsIgnoreCase for email/emailConfirm comparison - signupPage.jsp: restored explicit action URL (urlFor(BeginAction.class)) lost in the rewrite Co-Authored-By: Claude <noreply@anthropic.com>
Rationale
We need to add bot protection to the self-sign-up form.
Changes
SignUpApiActionandBeginActionemailConfirmfield requiring users to enter their email address twiceEmailValidator.isValidwas not called, and asendEmailfailure was falling through tostatus=USER_ADDEDsignupPage.jspto use<labkey:form>and<labkey:input>taglibsManual test plan
status=USER_EXISTSSee ai/todos/active/TODO-LK-20260517_add-kaptcha-to-signup.md
Co-Authored-By: Claude noreply@anthropic.com