Use Captcha middleware in members API #22014
Conversation
WalkthroughThe pull request introduces CAPTCHA functionality across multiple Ghost core files. The changes involve adding a new function Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
🪧 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: 0
🧹 Nitpick comments (4)
ghost/core/core/server/api/endpoints/settings-public.js (1)
7-16: Add validation and error handling for Captcha settings.The implementation should validate the config values and handle cases where the config is missing or invalid.
const getCaptchaSettings = () => { if (labs.isSet('captcha')) { + const enabled = config.get('captcha:enabled'); + const siteKey = config.get('captcha:siteKey'); + + if (enabled && !siteKey) { + logging.error('Captcha is enabled but siteKey is not configured'); + } + return { - captcha_enabled: config.get('captcha:enabled'), - captcha_sitekey: config.get('captcha:siteKey') + captcha_enabled: enabled, + captcha_sitekey: siteKey }; } else { return {}; } };ghost/core/core/server/services/members/api.js (1)
244-248: Add validation for Captcha scoreThreshold.The scoreThreshold should be validated to ensure it's within a valid range (typically 0.0 to 1.0).
captchaService: new CaptchaService({ enabled: labsService.isSet('captcha') && config.get('captcha:enabled'), - scoreThreshold: config.get('captcha:scoreThreshold'), + scoreThreshold: validateScoreThreshold(config.get('captcha:scoreThreshold')), secretKey: config.get('captcha:secretKey') }) +function validateScoreThreshold(threshold) { + const score = parseFloat(threshold); + if (isNaN(score) || score < 0.0 || score > 1.0) { + logging.warn(`Invalid captcha:scoreThreshold ${threshold}. Using default 0.5`); + return 0.5; + } + return score; +}ghost/core/test/e2e-api/content/settings.test.js (2)
32-59: Add test cases for disabled and invalid configurations.The test suite should include additional cases:
- When labs flag is disabled
- When configuration is invalid/missing
describe('Captcha settings', function () { beforeEach(function () { configUtils.set('captcha', { enabled: true, siteKey: 'testkey' }); }); afterEach(function () { configUtils.restore(); }); it('Can request captcha settings', async function () { await agent.get('settings/') .expectStatus(200) .matchHeaderSnapshot({ etag: anyEtag, 'content-version': anyContentVersion, 'content-length': anyContentLength }) .matchBodySnapshot({ settings: { captcha_enabled: true, captcha_sitekey: 'testkey' } }); }); + + it('Returns empty object when labs flag is disabled', async function () { + labsService.disable('captcha'); + await agent.get('settings/') + .expectStatus(200) + .matchBodySnapshot({ + settings: {} + }); + }); + + it('Handles missing configuration gracefully', async function () { + configUtils.set('captcha', {}); + await agent.get('settings/') + .expectStatus(200) + .matchBodySnapshot({ + settings: { + captcha_enabled: false, + captcha_sitekey: undefined + } + }); + }); });
1-1: Security Review: Implementation is solid with minor improvements needed.The PR successfully enhances the security of the members send-magic-link API by adding Captcha protection. The implementation is well-structured with:
- Feature flag control
- Proper middleware integration
- Configuration flexibility
Consider the suggested improvements for validation and error handling to make the implementation more robust.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
ghost/core/test/e2e-api/content/__snapshots__/settings.test.js.snapis excluded by!**/*.snap
📒 Files selected for processing (4)
ghost/core/core/server/api/endpoints/settings-public.js(2 hunks)ghost/core/core/server/services/members/api.js(5 hunks)ghost/core/test/e2e-api/content/settings.test.js(2 hunks)ghost/members-api/lib/members-api.js(2 hunks)
🔇 Additional comments (1)
ghost/members-api/lib/members-api.js (1)
339-339: LGTM! Captcha middleware is correctly integrated.The Captcha middleware is properly added to the sendMagicLink route, enhancing security while maintaining the existing error handling pattern.
sam-lord
force-pushed
the
captcha-enable-middleware
branch
from
5c855d6 to
f194f77
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
ghost/core/core/server/api/endpoints/settings-public.js (1)
34-35: Consider caching the CAPTCHA settings.The settings are fetched on every request. Consider caching the result of
getCaptchaSettings()since the values only change when the lab flag or configuration changes.ghost/core/core/server/services/members/api.js (1)
244-248: Consider adding error monitoring for CAPTCHA service.Add Sentry monitoring for CAPTCHA verification failures to track potential abuse patterns.
captchaService: new CaptchaService({ enabled: labsService.isSet('captcha') && sharedConfig.get('captcha:enabled'), scoreThreshold: sharedConfig.get('captcha:scoreThreshold'), - secretKey: sharedConfig.get('captcha:secretKey') + secretKey: sharedConfig.get('captcha:secretKey'), + onError: (error) => { + sentry.captureException(error); + } }),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
ghost/core/test/e2e-api/content/__snapshots__/settings.test.js.snapis excluded by!**/*.snap
📒 Files selected for processing (6)
ghost/core/content/themes/casper(1 hunks)ghost/core/content/themes/source(1 hunks)ghost/core/core/server/api/endpoints/settings-public.js(2 hunks)ghost/core/core/server/services/members/api.js(2 hunks)ghost/core/test/e2e-api/content/settings.test.js(2 hunks)ghost/members-api/lib/members-api.js(2 hunks)
✅ Files skipped from review due to trivial changes (2)
- ghost/core/content/themes/source
- ghost/core/content/themes/casper
🚧 Files skipped from review as they are similar to previous changes (2)
- ghost/members-api/lib/members-api.js
- ghost/core/test/e2e-api/content/settings.test.js
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Setup
🔇 Additional comments (2)
ghost/core/core/server/api/endpoints/settings-public.js (1)
4-16: LGTM! Well-structured implementation of CAPTCHA settings.The implementation correctly:
- Uses feature flag to control CAPTCHA availability
- Only exposes public CAPTCHA configuration (siteKey)
- Maintains security by not exposing sensitive configuration
ghost/core/core/server/services/members/api.js (1)
21-21: Verify CAPTCHA configuration values.The implementation looks secure, but please ensure:
- The
scoreThresholdis set to an appropriate value (recommended: 0.5)- The
secretKeyis properly set in the configuration- Error handling is implemented in the middleware using this service
Run this script to check the configuration:
Also applies to: 244-248
✅ Verification successful
CAPTCHA implementation is properly secured and configured.
The implementation includes:
- Robust error handling in the middleware
- Strict score threshold (0.8)
- Proper secret key configuration
- Comprehensive security logging
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify CAPTCHA configuration values and error handling # Check if scoreThreshold is configured rg -A 1 "captcha:scoreThreshold" # Verify error handling in middleware ast-grep --pattern 'captchaService.verify($_)'Length of output: 308
Script:
#!/bin/bash # Search for default configuration and implementation rg -g '!*.lock' -g '!*.md' "captcha" -B 2 -A 2 # Search for potential error handling patterns ast-grep --pattern 'try { $$$ captcha $$$ } catch'Length of output: 17242
sam-lord
force-pushed
the
captcha-enable-middleware
branch
2 times, most recently
from
3a465fb to
a6d81a7
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
ghost/core/test/e2e-api/content/settings.test.js (1)
32-59: Consider adding more test cases for Captcha settings.While the current test case verifies the happy path (captcha enabled with a site key), consider adding test cases for:
- Captcha disabled scenario
- Missing or invalid site key
- Edge cases in configuration
This will ensure robust testing of the Captcha settings endpoint.
Would you like me to help generate additional test cases?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
ghost/core/test/e2e-api/content/__snapshots__/settings.test.js.snapis excluded by!**/*.snap
📒 Files selected for processing (3)
ghost/core/core/server/services/members/api.js(2 hunks)ghost/core/test/e2e-api/content/settings.test.js(2 hunks)ghost/members-api/lib/members-api.js(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- ghost/members-api/lib/members-api.js
🔇 Additional comments (3)
ghost/core/test/e2e-api/content/settings.test.js (1)
3-3: LGTM! Import statement is correctly placed.The configUtils import is appropriately placed with other test utility imports.
ghost/core/core/server/services/members/api.js (2)
21-21: LGTM! Import statement is correctly placed.The CaptchaService import is appropriately placed with other service imports.
244-248:⚠️ Potential issueVerify security implications of Captcha configuration.
The implementation correctly uses the labs flag as a feature toggle, but let's ensure:
- The secretKey is properly secured
- The configuration is not exposed in logs or error messages
Let's check for potential security issues:
Consider:
- Adding input validation for the secretKey
- Ensuring the secretKey is never logged or exposed in error messages
- Using environment variables for sensitive configuration
✅ Verification successful
Captcha configuration security verification completed successfully
The implementation demonstrates secure handling of sensitive configuration:
- Secret key is properly encapsulated using private class fields
- No exposure of sensitive data in logs or error messages
- Clear separation between public and private configuration
- Proper feature flag control
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for potential security issues in Captcha configuration # Search for logging of secretKey rg -A 5 'console\.log|logging\.' --type js | grep -i 'secret|key' # Search for error handling that might expose secretKey rg -A 5 'catch|error|throw' --type js | grep -i 'secret|key'Length of output: 130
Script:
#!/bin/bash # Search for captcha-related configuration files and implementation rg -l -g '!node_modules' -g '!test' 'captcha' # Check environment variable handling for captcha rg -A 5 -B 5 'process\.env.*captcha|config.*captcha' --type js # Look for shared configuration implementation fd -e js -e json config rg -A 5 'sharedConfig.*get' --type js # Search for captcha error handling with broader context rg -A 10 'captchaService|CaptchaService' --type jsLength of output: 18597
| captchaService: new CaptchaService({ | ||
| enabled: labsService.isSet('captcha') && sharedConfig.get('captcha:enabled'), | ||
| scoreThreshold: sharedConfig.get('captcha:scoreThreshold'), | ||
| secretKey: sharedConfig.get('captcha:secretKey') | ||
| }), |
There was a problem hiding this comment.
💡 Codebase verification
Add validation and defaults for CaptchaService configuration
The CaptchaService lacks proper configuration validation which could lead to runtime failures:
- When enabled, validate that secretKey is present and properly formatted
- Add a default value for scoreThreshold (e.g., 0.5)
- Add validation in constructor to fail fast when enabled without proper configuration
🔗 Analysis chain
Verify error handling for missing Captcha configuration.
The CaptchaService initialization looks good, but consider the following:
- What happens if the configuration values are missing?
- Are there defaults for scoreThreshold?
- Should we validate the secretKey format?
Let's check the configuration handling:
Consider adding validation and default values in the configuration to ensure the service fails gracefully when configuration is missing or invalid.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for error handling in CaptchaService configuration
# Search for default values or validation in CaptchaService
ast-grep --pattern 'class CaptchaService {
constructor($_) {
$$$
}
}'
# Search for usage of scoreThreshold and secretKey
rg -A 5 'scoreThreshold|secretKey' --type js
Length of output: 21307
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
ghost/members-api/lib/members-api.js (2)
77-77: Add validation for captchaService parameter.Consider adding validation to ensure captchaService is properly initialized before use.
captchaService, +if (!captchaService?.isEnabled) { + throw new errors.IncorrectUsageError({ + message: 'CaptchaService must be properly initialized' + }); +}
338-351: Optimize middleware structure and enhance error handling.
- The middleware structure can be optimized to reduce duplication.
- Consider adding specific error handling for captcha validation failures.
const getMagicLinkMiddleware = () => { + const router = Router().use(body.json()); + if (captchaService.isEnabled()) { - return Router().use( - body.json(), + router.use( captchaService.getMiddleware(), forwardError((req, res) => routerController.sendMagicLink(req, res)) ); } else { - return Router().use( - body.json(), + router.use( forwardError((req, res) => routerController.sendMagicLink(req, res)) ); } + return router; };Also, consider adding specific error handling for captcha validation failures:
const handleCaptchaError = (err, req, res, next) => { if (err.code === 'CAPTCHA_VALIDATION_FAILED') { res.status(400).json({ errors: [{ message: 'Captcha validation failed', type: 'BadRequestError' }] }); return; } next(err); }; // Add after captchaService.getMiddleware() handleCaptchaError,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ghost/members-api/lib/members-api.js(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Regression tests (Node 20.11.1, sqlite3)
- GitHub Check: Ghost-CLI tests
- GitHub Check: Regression tests (Node 20.11.1, mysql8)
- GitHub Check: Unit tests (Node 20.11.1)
- GitHub Check: Database tests (Node 20.11.1, mysql8)
- GitHub Check: Unit tests (Node 18.12.1)
- GitHub Check: Database tests (Node 18.12.1, mysql8)
- GitHub Check: Lint
- GitHub Check: i18n
🔇 Additional comments (1)
ghost/members-api/lib/members-api.js (1)
354-354: LGTM! Clean integration of captcha middleware.The middleware integration is well-structured and properly implements the captcha protection for the magic link endpoint.
ghost/members-api/lib/members-api.js
Outdated
| const getMagicLinkMiddleware = () => { | ||
| if (captchaService.isEnabled()) { | ||
| return Router().use( | ||
| body.json(), | ||
| captchaService.getMiddleware(), | ||
| forwardError((req, res) => routerController.sendMagicLink(req, res)) | ||
| ); | ||
| } else { | ||
| return Router().use( | ||
| body.json(), | ||
| forwardError((req, res) => routerController.sendMagicLink(req, res)) | ||
| ); | ||
| } | ||
| }; | ||
|
|
||
| const middleware = { | ||
| sendMagicLink: Router().use( | ||
| body.json(), | ||
| forwardError((req, res) => routerController.sendMagicLink(req, res)) | ||
| ), | ||
| sendMagicLink: getMagicLinkMiddleware(), |
There was a problem hiding this comment.
💡 Codebase verification
Missing test coverage for captcha integration in magic link flow
The implementation requires additional test coverage for:
- Integration between captcha middleware and magic link endpoint
- Captcha validation during magic link requests
- Feature flag behavior affecting the middleware
Specific test cases needed:
- Successful magic link request with valid captcha token
- Failed request with invalid captcha token
- Request behavior when captcha is disabled
🔗 Analysis chain
Verify captcha integration with end-to-end tests.
The implementation looks solid, but please ensure comprehensive testing of:
- Captcha validation success/failure scenarios
- Feature flag enabling/disabling behavior
- Integration with the magic link flow
Let's verify the test coverage:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for related test files and their coverage of captcha scenarios
# Look for test files related to magic link and captcha
echo "Searching for related test files..."
rg -l "magic.*link|captcha" --type test
# Look for captcha-related test scenarios
echo -e "\nSearching for captcha test scenarios..."
rg "test.*captcha" --type test -A 5
# Check if getMagicLinkMiddleware is tested
echo -e "\nChecking for middleware tests..."
rg "getMagicLinkMiddleware" --type test -A 5
Length of output: 468
Script:
#!/bin/bash
# Search for test files and their coverage
echo "Searching for magic link and captcha test files..."
rg -l "(magic.*link|captcha).*(test|spec)" -g "*.js"
echo -e "\nSearching for captcha test scenarios..."
rg "describe.*captcha|test.*captcha|it.*captcha" -g "*.js" -A 5
echo -e "\nChecking for magic link middleware tests..."
rg "getMagicLinkMiddleware|sendMagicLink" -g "*.(test|spec).js" -A 5
# Also check test directories specifically
echo -e "\nChecking test directories..."
fd "test" --type d
Length of output: 5987
Script:
#!/bin/bash
# Check content of magic link test file
echo "Checking magic link test content..."
cat "ghost/core/test/e2e-api/members/send-magic-link.test.js"
# Look for integration tests between captcha and magic link
echo -e "\nChecking for captcha integration in magic link tests..."
rg -B 5 -A 10 "sendMagicLink.*captcha|captcha.*sendMagicLink" ghost/core/test/e2e-api/members/send-magic-link.test.js
Length of output: 11131
sam-lord
force-pushed
the
captcha-enable-middleware
branch
from
1722292 to
a6d81a7
Compare
ref BAE-104 Fixup
sam-lord
force-pushed
the
captcha-enable-middleware
branch
from
a6d81a7 to
0dd90c5
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
ghost/core/test/e2e-api/content/settings.test.js (1)
44-58: Add test case for disabled captcha settings.The test suite should also verify the behavior when captcha is disabled to ensure complete coverage of the feature flag.
Add a test case like this:
it('Returns correct settings when captcha is disabled', async function () { configUtils.set('captcha', { enabled: false }); await agent.get('settings/') .expectStatus(200) .matchHeaderSnapshot({ etag: anyEtag, 'content-version': anyContentVersion, 'content-length': anyContentLength }) .matchBodySnapshot({ settings: Object.assign({}, settingsMatcher, { captcha_enabled: false, captcha_sitekey: null }) }); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
ghost/core/test/e2e-api/content/__snapshots__/settings.test.js.snapis excluded by!**/*.snap
📒 Files selected for processing (5)
ghost/core/core/server/api/endpoints/settings-public.js(2 hunks)ghost/core/core/server/services/members/api.js(2 hunks)ghost/core/package.json(1 hunks)ghost/core/test/e2e-api/content/settings.test.js(2 hunks)ghost/members-api/lib/members-api.js(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- ghost/core/core/server/api/endpoints/settings-public.js
🔇 Additional comments (5)
ghost/core/test/e2e-api/content/settings.test.js (1)
33-38: LGTM!The test setup correctly configures the captcha settings for the test suite.
ghost/core/core/server/services/members/api.js (1)
244-248: Add validation and defaults for CaptchaService configuration.The CaptchaService lacks proper configuration validation which could lead to runtime failures:
- When enabled, validate that secretKey is present and properly formatted
- Add a default value for scoreThreshold (e.g., 0.5)
- Add validation in constructor to fail fast when enabled without proper configuration
ghost/core/package.json (1)
74-74: Pin the captcha-service dependency to a stable version.The version "0.0.0" indicates a development version. Before deploying to production, ensure the dependency is pinned to a stable release version.
Run this script to check available versions:
ghost/members-api/lib/members-api.js (2)
77-77: LGTM!The captcha service parameter is correctly added to the function signature.
341-341: Missing test coverage for captcha integration in magic link flow.The implementation requires additional test coverage for:
- Integration between captcha middleware and magic link endpoint
- Captcha validation during magic link requests
- Feature flag behavior affecting the middleware
ref BAE-104
The members send-magic-link API should be protected by Captcha. This required initialising the Captcha service in the members API, and putting the middleware into the send-magic-link API.
If it's enabled via lab flag and config, then the service will prevent API calls that don't have a valid Captcha response.
@coderabbitai