Portal: Added HCaptcha element to signup/signin pages #22009
Conversation
ref BAE-371 Added the HCaptcha react component & related utils to enable it / disable it based on the Captcha labs flag. At the moment this does not include the same functionality on forms using the data-attributes.
|
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
apps/portal/src/components/pages/SigninPage.jsOops! Something went wrong! :( ESLint: 8.44.0 ESLint couldn't find the plugin "eslint-plugin-i18next". (The package "eslint-plugin-i18next" was not found when loaded as a Node module from the directory "/apps/portal".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "eslint-plugin-i18next" was referenced from the config file in "apps/portal/package.json". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team. WalkthroughThe pull request introduces hCaptcha integration into the Ghost Portal application. The changes span multiple files to add captcha verification for signup and signin processes. A new dependency Changes
Sequence DiagramsequenceDiagram
participant User
participant SignupPage
participant HCaptcha
participant AuthService
User->>SignupPage: Initiate Signup
alt Captcha Enabled
SignupPage->>HCaptcha: Execute Verification
HCaptcha-->>SignupPage: Return Token
SignupPage->>AuthService: Signup with Token
else Captcha Disabled
SignupPage->>AuthService: Direct Signup
end
AuthService-->>SignupPage: Signup Response
SignupPage-->>User: Show Result
Poem
Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (10)
apps/portal/src/utils/helpers.js (1)
518-520: Consider validating the sitekey format.While the implementation is clean, consider adding validation for the sitekey format to prevent potential issues with invalid keys being passed to the HCaptcha component.
export function getCaptchaSitekey({site}) { - return site?.captcha_sitekey || ''; + const sitekey = site?.captcha_sitekey || ''; + // HCaptcha sitekeys are typically 40 characters long + if (sitekey && !/^[0-9a-f]{40}$/i.test(sitekey)) { + console.warn('[Portal] Invalid HCaptcha sitekey format'); + } + return sitekey; }apps/portal/src/components/pages/SignupPage.test.js (2)
216-216: Document test environment limitations.The comment about hCaptcha's test environment limitations should be expanded to provide more context for other developers.
- // Cannot test using hCaptcha component, as it cannot run in a test environment + // Note: Limited testing scope due to hCaptcha's runtime requirements. + // The hCaptcha component requires a browser environment with full JavaScript execution. + // We can only verify the component's presence, not its functionality.
217-231: Enhance test coverage for captcha functionality.While the current test verifies the presence of the hCaptcha element, consider adding more test cases:
- Verify captcha is not rendered when disabled
- Test error states and loading states
- Test the interaction between captcha state and form submission
Example implementation:
describe('when captcha is enabled', () => { const getById = queryByAttribute.bind(null, 'id'); test('renders hCaptcha when enabled', () => { setup({ site: getSiteData({ captchaEnabled: true, captchaSiteKey: '20000000-ffff-ffff-ffff-000000000002' }) }); const hcaptchaElement = getById(document.body, 'hcaptcha-signup'); expect(hcaptchaElement).toBeInTheDocument(); }); test('does not render hCaptcha when disabled', () => { setup({ site: getSiteData({ captchaEnabled: false }) }); const hcaptchaElement = getById(document.body, 'hcaptcha-signup'); expect(hcaptchaElement).not.toBeInTheDocument(); }); test('submit button is disabled until captcha is verified', () => { const {submitButton} = setup({ site: getSiteData({ captchaEnabled: true, captchaSiteKey: '20000000-ffff-ffff-ffff-000000000002' }) }); expect(submitButton).toBeDisabled(); }); });apps/portal/src/utils/api.js (2)
273-275: Good security implementation with room for minor improvement.The implementation combines multiple security measures:
- Honeypot field using a hidden phone number input
- HCaptcha token verification
Consider maintaining comment style consistency with the rest of the file.
- // we don't actually use a phone #, this is from a hidden field to prevent bot activity + /* We don't actually use a phone #, this is from a hidden field to prevent bot activity */
Line range hint
261-275: Consider updating API documentation.Since this is a significant security enhancement, consider:
- Adding JSDoc comments to document the new
tokenparameter- Updating any API documentation to reflect the captcha requirements
- Adding examples of error handling for captcha validation failures
+/** + * Sends a magic link for authentication + * @param {Object} options + * @param {string} options.email - User's email address + * @param {string} [options.token] - HCaptcha verification token (required when captcha is enabled) + * ... + * @throws {Error} When captcha validation fails + */ async sendMagicLink({email, emailType, labels, name, oldEmail, newsletters, redirect, integrityToken, phonenumber, customUrlHistory, token, autoRedirect = true}) {apps/portal/src/utils/fixtures-generator.js (1)
76-78: Consider adding JSDoc for the new captcha parameters.While the implementation looks good and follows the existing property naming convention (snake_case), adding JSDoc comments would help document the purpose and expected values of these new captcha-related parameters.
export function getSiteData({ + /** @type {boolean} Whether captcha verification is enabled */ captchaEnabled = false, + /** @type {string} The site key for HCaptcha integration */ captchaSiteKeyapps/portal/src/components/pages/SignupPage.js (2)
747-756: Consider handling captcha loading state.The captcha integration looks good, but the user experience could be improved by:
- Disabling the submit button while captcha is loading
- Showing a loading indicator during verification
{(hasCaptchaEnabled({site}) && <HCaptcha size="invisible" sitekey={getCaptchaSitekey({site})} - onLoad={() => this.setState({captchaLoaded: true})} + onLoad={() => { + this.setState({ + captchaLoaded: true, + captchaError: null + }); + }} + onError={(error) => { + this.setState({ + captchaError: 'Captcha verification failed', + isSubmitting: false + }); + }} onVerify={token => this.setState({token: token}, this.doSignup)} ref={this.captchaRef} id="hcaptcha-signup" /> )}
Line range hint
391-392: Add captcha cleanup in componentWillUnmount.To prevent potential memory leaks, consider resetting the captcha state when the component unmounts.
componentWillUnmount() { clearTimeout(this.timeoutId); + if (this.captchaRef.current) { + this.captchaRef.current.resetCaptcha(); + } }apps/portal/src/components/pages/SigninPage.js (2)
18-23: Document the token state type.Consider adding JSDoc comments to document the expected type and format of the token state, which would help with maintainability and type checking.
constructor(props) { super(props); + /** @type {{ + email: string, + captchaLoaded: boolean, + token: string | undefined + }} */ this.state = { email: '', captchaLoaded: false, token: undefined }; this.captchaRef = React.createRef(); }
175-184: Enhance accessibility and loading state handling.
- Add aria-label for screen readers
- Handle loading state in the submit button
{(hasCaptchaEnabled({site}) && <HCaptcha size="invisible" sitekey={getCaptchaSitekey({site})} onLoad={() => this.setState({captchaLoaded: true})} onVerify={token => this.setState({token: token}, this.doSignin)} ref={this.captchaRef} id="hcaptcha-signin" + aria-label={t('Security verification')} /> )}Also, modify the
renderSubmitButtonmethod to handle the captcha loading state:renderSubmitButton() { const {action, t} = this.context; - const isRunning = (action === 'signin:running'); + const isRunning = (action === 'signin:running' || + (hasCaptchaEnabled({site}) && !this.state.captchaLoaded)); let label = isRunning ? t('Sending login link...') : t('Continue');
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (7)
apps/portal/package.json(1 hunks)apps/portal/src/components/pages/SigninPage.js(3 hunks)apps/portal/src/components/pages/SignupPage.js(6 hunks)apps/portal/src/components/pages/SignupPage.test.js(2 hunks)apps/portal/src/utils/api.js(2 hunks)apps/portal/src/utils/fixtures-generator.js(2 hunks)apps/portal/src/utils/helpers.js(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Setup
🔇 Additional comments (10)
apps/portal/src/utils/helpers.js (1)
514-516: Clean implementation of captcha check!The function follows a consistent pattern with other utility functions and has a clear, focused responsibility.
apps/portal/src/components/pages/SignupPage.test.js (1)
3-3: LGTM!The addition of
queryByAttributeto the imports is appropriate for the new test functionality.apps/portal/src/utils/api.js (1)
261-261: LGTM! Method signature updated to support captcha verification.The addition of the optional
tokenparameter aligns with the PR objectives for HCaptcha integration.apps/portal/package.json (1)
101-102: LGTM! Package structure follows best practices.The dependency is correctly placed under
dependenciesand the version is appropriately pinned for reproducible builds.apps/portal/src/utils/fixtures-generator.js (2)
46-48: LGTM! Parameters follow existing conventions.The new parameters
recommendationsEnabled,captchaEnabled, andcaptchaSiteKeymaintain consistency with the existing parameter naming convention (camelCase).
77-78: Verify captcha_sitekey handling.The
captcha_sitekeyis assigned directly without validation or default value. Consider:
- Whether a default value should be provided for testing scenarios
- If validation is needed to ensure the key format is correct
Let's check how this key is used across the codebase:
✅ Verification successful
The current captcha_sitekey handling is appropriate
The implementation is correct because:
- The
getCaptchaSitekeyhelper provides an empty string fallback- Key validation is handled by the hCaptcha component
- The optional nature of captcha (controlled by
captcha_enabled) makes the current implementation sufficient🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usages of captcha_sitekey to understand validation requirements rg -A 3 "captcha_sitekey|captchaSiteKey" --type jsLength of output: 1063
Script:
#!/bin/bash # Search for captcha component implementation rg -A 5 "ReCAPTCHA|hCaptcha|Captcha" --type jsLength of output: 12465
apps/portal/src/components/pages/SignupPage.js (2)
10-10: LGTM! Clean integration of HCaptcha dependencies.The imports and constructor changes are well-organized, with proper initialization of the captcha reference.
Also applies to: 13-13, 360-360
750-750: Verify captcha site key configuration.Ensure that the
getCaptchaSitekeyutility is retrieving the correct site key for both development and production environments.✅ Verification successful
Captcha implementation is properly configured ✓
The
getCaptchaSitekeyutility correctly retrieves the site key from the site configuration. The implementation includes proper feature flagging, error handling, and follows hCaptcha integration best practices.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for hardcoded site keys in the codebase rg -g '!node_modules' -g '!dist' -i 'hcaptcha.*sitekey' # Check for environment-specific configurations rg -g '!node_modules' -g '!dist' 'HCAPTCHA_SITE_KEY'Length of output: 110
Script:
#!/bin/bash # Search for the getCaptchaSitekey implementation ast-grep --pattern 'function getCaptchaSitekey' # Look for any captcha-related files or configurations rg -g '!node_modules' -g '!dist' -i 'captcha' # Search for environment variable patterns in config files fd -e js -e json -e env --exec grep -l -i "captcha" {} \;Length of output: 10871
apps/portal/src/components/pages/SigninPage.js (2)
10-10: Verify the hCaptcha dependency version and security.Let's ensure we're using a secure version of the hCaptcha React component.
✅ Verification successful
hCaptcha dependency is secure but could be updated
The current version (1.11.0) is secure with no known vulnerabilities. Consider updating to the latest version (1.11.1) in a future maintenance cycle for potential bug fixes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for security advisories and latest versions curl -s https://registry.npmjs.org/@hcaptcha/react-hcaptcha | jq '.["dist-tags"].latest' # Check for known vulnerabilities gh api graphql -f query=' { securityVulnerabilities(first: 5, ecosystem: NPM, package: "@hcaptcha/react-hcaptcha") { nodes { advisory { summary severity publishedAt } vulnerableVersionRange firstPatchedVersion { identifier } } } }'Length of output: 457
Script:
#!/bin/bash # Check package.json for the actual version fd package.json --type f --exec sh -c 'echo "=== {} ==="; cat {} | jq -r "select(.dependencies?.\"@hcaptcha/react-hcaptcha\" != null) | .dependencies?.\"@hcaptcha/react-hcaptcha\""' # Check yarn.lock or package-lock.json for the resolved version fd "yarn.lock|package-lock.json" --type f --exec sh -c 'echo "=== {} ==="; cat {}'Length of output: 71002
179-180: Handle captcha token expiration.The hCaptcha token can expire. Consider adding the
onExpirecallback to reset the token state and show an appropriate message to the user.<HCaptcha size="invisible" sitekey={getCaptchaSitekey({site})} onLoad={() => this.setState({captchaLoaded: true})} onVerify={token => this.setState({token: token}, this.doSignin)} + onExpire={() => { + this.setState({ + token: undefined, + errors: { + captcha: this.context.t('Security verification expired. Please try again.') + } + }); + }} ref={this.captchaRef} id="hcaptcha-signin" />✅ Verification successful
The concern about hCaptcha token expiration is valid.
The codebase currently lacks token expiration handling, which could lead to failed signin attempts if users take too long to complete the form. The suggested solution to add the
onExpirecallback with appropriate error messaging follows good practices for user experience and security.🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash # Find HCaptcha component usage patterns echo "=== HCaptcha Component Usage ===" ast-grep --pattern 'HCaptcha $$$' echo -e "\n=== Token Expiration Handling ===" rg -i "expire|token.*expire|captcha.*expire" -A 2 echo -e "\n=== Error Messages Related to Captcha ===" rg -i "captcha.*error|security.*verification" -A 1Length of output: 76425
| "dependencies": { | ||
| "@hcaptcha/react-hcaptcha": "1.11.0" |
There was a problem hiding this comment.
💡 Codebase verification
Update to latest patch version 1.11.1
The package is compatible with React 17 and has no security vulnerabilities, but there's a newer patch version available.
- Update
@hcaptcha/react-hcaptchafrom1.11.0to1.11.1inapps/portal/package.json
🔗 Analysis chain
Verify HCaptcha package compatibility and security.
The addition of @hcaptcha/react-hcaptcha looks good, but let's verify a few important aspects:
Run this script to check compatibility with React 17 and latest versions:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify hcaptcha package compatibility and check for newer versions
# Check package.json from @hcaptcha/react-hcaptcha for peer dependencies
curl -s https://registry.npmjs.org/@hcaptcha/react-hcaptcha/1.11.0 | jq -r '.peerDependencies'
# Get latest version
curl -s https://registry.npmjs.org/@hcaptcha/react-hcaptcha/latest | jq -r '.version'
# Check for security advisories
gh api graphql -f query='
{
securityVulnerabilities(first: 5, ecosystem: NPM, package: "@hcaptcha/react-hcaptcha") {
nodes {
advisory {
summary
severity
publishedAt
}
vulnerableVersionRange
firstPatchedVersion {
identifier
}
}
}
}'
Length of output: 600
| doSignupWithChecks() { | ||
| const {site} = this.context; | ||
| if (hasCaptchaEnabled({site})) { | ||
| // hCaptcha's callback will call doSignup | ||
| return this.captchaRef.current.execute(); | ||
| } else { | ||
| this.doSignup(); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for captcha verification failures.
While the captcha integration looks good, consider handling potential failure scenarios:
- Network errors during captcha verification
- Invalid or expired captcha tokens
- User cancellation of captcha
doSignupWithChecks() {
const {site} = this.context;
if (hasCaptchaEnabled({site})) {
- // hCaptcha's callback will call doSignup
- return this.captchaRef.current.execute();
+ try {
+ return this.captchaRef.current.execute();
+ } catch (error) {
+ this.setState({
+ errors: {
+ captcha: 'Failed to verify captcha. Please try again.'
+ }
+ });
+ }
} else {
this.doSignup();
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| doSignupWithChecks() { | |
| const {site} = this.context; | |
| if (hasCaptchaEnabled({site})) { | |
| // hCaptcha's callback will call doSignup | |
| return this.captchaRef.current.execute(); | |
| } else { | |
| this.doSignup(); | |
| } | |
| } | |
| doSignupWithChecks() { | |
| const {site} = this.context; | |
| if (hasCaptchaEnabled({site})) { | |
| try { | |
| return this.captchaRef.current.execute(); | |
| } catch (error) { | |
| this.setState({ | |
| errors: { | |
| captcha: 'Failed to verify captcha. Please try again.' | |
| } | |
| }); | |
| } | |
| } else { | |
| this.doSignup(); | |
| } | |
| } |
|
|
||
| const {site} = this.context; | ||
| if (hasCaptchaEnabled({site})) { | ||
| // hCaptcha's callback will call doSignin | ||
| return this.captchaRef.current.execute(); | ||
| } else { | ||
| this.doSignin(); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for captcha execution failures.
The captcha execution could fail for various reasons (network issues, timeouts, etc.). Consider adding try-catch block and error state handling.
handleSignin(e) {
e.preventDefault();
const {site} = this.context;
if (hasCaptchaEnabled({site})) {
- // hCaptcha's callback will call doSignin
- return this.captchaRef.current.execute();
+ try {
+ // hCaptcha's callback will call doSignin
+ return this.captchaRef.current.execute();
+ } catch (error) {
+ this.setState({
+ errors: {
+ captcha: this.context.t('Failed to load captcha. Please try again.')
+ }
+ });
+ }
} else {
this.doSignin();
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const {site} = this.context; | |
| if (hasCaptchaEnabled({site})) { | |
| // hCaptcha's callback will call doSignin | |
| return this.captchaRef.current.execute(); | |
| } else { | |
| this.doSignin(); | |
| } | |
| } | |
| handleSignin(e) { | |
| e.preventDefault(); | |
| const {site} = this.context; | |
| if (hasCaptchaEnabled({site})) { | |
| try { | |
| // hCaptcha's callback will call doSignin | |
| return this.captchaRef.current.execute(); | |
| } catch (error) { | |
| this.setState({ | |
| errors: { | |
| captcha: this.context.t('Failed to load captcha. Please try again.') | |
| } | |
| }); | |
| } | |
| } else { | |
| this.doSignin(); | |
| } | |
| } |
ref BAE-371
Added the HCaptcha react component & related utils to enable it / disable it based on the Captcha labs flag. At the moment this does not include the same functionality on forms using the data-attributes.
Testing AI code review:
@coderabbitai
Summary by CodeRabbit
Release Notes
New Features
Dependencies
@hcaptcha/react-hcaptchapackage (version 1.11.0)Improvements