Repository navigation
feat: Captcha Auto mode & other improvements - #540
Conversation
|
Caution Review failedFailed to post review comments 📝 WalkthroughWalkthroughAdds adaptive CAPTCHA PoW (manual/auto), structured verification and Redis-based outcome counters; enriches analytics with ClickHouse meta-to-SQL helpers, multi-series CAPTCHA charts and summary stats; updates controllers, DTOs, DB migrations, frontend settings, chart/UI, docs, and client validator defaults. ChangesCAPTCHA Core & Analytics
Frontend: types, settings UI & analytics view
Sequence Diagram(s) sequenceDiagram
participant Client
participant CaptchaController
participant CaptchaService
participant Redis
participant ClickHouse
Client->>CaptchaController: generate/verify/validate request (headers, ip, payload)
CaptchaController->>CaptchaService: generateChallenge / verifyPoW / validateToken (with headers, ip)
CaptchaService->>Redis: read/write challenge, increment counters (selectDifficulty, recordOutcome, token replay NX)
CaptchaService->>ClickHouse: eventTransformer -> insert enriched captcha event
CaptchaService-->>CaptchaController: structured PowVerificationResult / validation response
CaptchaController-->>Client: HTTP response (token/result)
Estimated code review effort 🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web/app/pages/Project/tabs/Captcha/CaptchaView.tsx (1)
189-193:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon’t gate chart rendering on
resultsonly.Line 189 currently requires
chart.results, which drops valid multi-series payloads (e.g., onlygenerated/passed/failed/...) and hides the chart.Proposed diff
- if (chart && chart.x && chart.results) { + const hasChartSeries = Boolean( + chart && + (chart.results?.length || + chart.generated?.length || + chart.passed?.length || + chart.failed?.length || + chart.validationFailed?.length || + chart.replayed?.length) + ) + + if (chart?.x?.length && hasChartSeries) { setChartData(chart) } else { setChartData(null) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/app/pages/Project/tabs/Captcha/CaptchaView.tsx` around lines 189 - 193, The code is incorrectly gating chart rendering on chart.results which causes valid multi-series payloads to be dropped; update the conditional in the component (where chart and setChartData are used) to check for chart and chart.x (or other required axis/series existence) instead of requiring chart.results so that multi-series charts (e.g., generated/passed/failed) are accepted; modify the block that currently reads "if (chart && chart.x && chart.results) { setChartData(chart) } else { setChartData(null) }" to only require the necessary axis/series presence (e.g., chart && chart.x) before calling setChartData(chart).
🧹 Nitpick comments (6)
backend/apps/community/src/captcha/captcha.service.ts (1)
335-335: 💤 Low valueIncorrect method name in error log.
Same issue as in the cloud version - the error message references
logCaptchaPassbut this is the genericlogCaptchaEventmethod.📝 Proposed fix
- this.logger.error(`[CaptchaService -> logCaptchaPass] ${e}`) + this.logger.error(`[CaptchaService -> logCaptchaEvent] ${e}`)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/apps/community/src/captcha/captcha.service.ts` at line 335, The error log in the logCaptchaEvent method incorrectly references `logCaptchaPass`; update the logger call in `logCaptchaEvent` to reference the correct method name (`logCaptchaEvent`) and include the caught error `e` for context (i.e., change the message string passed to `this.logger.error` to mention `logCaptchaEvent` while still logging the error variable). Ensure the change is made in the `logCaptchaEvent` method so the log message accurately reflects the originating function.backend/apps/cloud/src/captcha/captcha.service.ts (2)
335-335: 💤 Low valueIncorrect method name in error log.
The error message references
logCaptchaPassbut this is the genericlogCaptchaEventmethod.📝 Proposed fix
- this.logger.error(`[CaptchaService -> logCaptchaPass] ${e}`) + this.logger.error(`[CaptchaService -> logCaptchaEvent] ${e}`)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/apps/cloud/src/captcha/captcha.service.ts` at line 335, The error log in the catch block incorrectly names the method as logCaptchaPass; update the log message to reference the actual method name logCaptchaEvent (or use a generic contextual message) so it accurately reflects where the error occurred; locate the catch in the logCaptchaEvent function and change the logger.error call from referencing logCaptchaPass to logCaptchaEvent (include the error object in the log as currently done).
156-164: 💤 Low valuePotential race condition on TTL expiration.
incrCounterchecksvalue === 1afterINCRto set TTL, but in high-concurrency scenarios, two concurrent calls could both seevalue > 1if one finishes before the other'sINCRreturns. This could leave a key without TTL if the first increment races past beforeexpireis called.Consider using atomic
INCR+EXPIREvia pipeline or Lua script, or just always callexpire(Redis is idempotent for this).♻️ Safer approach using Redis pipeline
const incrCounter = async (key: string, ttl: number): Promise<number> => { - const value = await redis.incr(key) - - if (value === 1) { - await redis.expire(key, ttl) - } - - return value + const pipeline = redis.pipeline() + pipeline.incr(key) + pipeline.expire(key, ttl) + const results = await pipeline.exec() + return results?.[0]?.[1] as number }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/apps/cloud/src/captcha/captcha.service.ts` around lines 156 - 164, The incrCounter function can lose the TTL in high-concurrency races because it calls redis.incr(key) then conditionally redis.expire(key, ttl); fix by making the INCR+EXPIRE atomic: either use a Redis pipeline/transaction to call incr and expire together, or execute a small Lua script that runs INCR and sets EXPIRE when the new value is 1, or simply always call redis.expire(key, ttl) after redis.incr(key) to rely on Redis idempotency; update the incrCounter function to use one of these approaches (referencing incrCounter, redis.incr, redis.expire) so the TTL is reliably set.backend/apps/community/src/captcha/captcha.controller.ts (1)
126-143: 💤 Low valueCatch block assumes all verifyPoW exceptions are replays.
Same observation as the cloud controller - the catch block logs
logCaptchaReplayfor any exception fromverifyPoW, but not all exceptions indicate replays.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/apps/community/src/captcha/captcha.controller.ts` around lines 126 - 143, The catch block around this.captchaService.verifyPoW currently treats every thrown error as a replay; change it to inspect the thrown error (e.g., check error type/class or a distinguishing property like error.code or error.message for a replay marker such as 'REPLAY' or a CaptchaReplayError) and only call this.captchaService.logCaptchaReplay(pid, headers, timestamp, ip) when the error indicates an actual replay; for all other errors rethrow immediately (preserving the original error) and avoid calling logCaptchaReplay. Ensure the logic references verifyPoW and logCaptchaReplay and preserves original exceptions when not handling replay cases.backend/apps/cloud/src/captcha/captcha.controller.ts (1)
126-143: 💤 Low valueCatch block assumes all verifyPoW exceptions are replays.
The catch block logs
logCaptchaReplayfor any exception fromverifyPoW, butverifyPoWthrowsBadRequestExceptionfor both "Invalid or expired challenge" (replay-like) and "Invalid challenge difficulty" or "Challenge was issued for a different project" (not replays). This could misclassify some failures as replays in analytics.Consider checking the exception message or introducing distinct exception types.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/apps/cloud/src/captcha/captcha.controller.ts` around lines 126 - 143, The catch in the verify handler treats every error from captchaService.verifyPoW as a replay and calls logCaptchaReplay; change it to only call logCaptchaReplay for genuine replay errors by distinguishing error types/messages: inspect the thrown error from verifyPoW (e.g., check instanceof BadRequestException and the error.message or introduce and check a dedicated ReplayCaptchaException from captchaService.verifyPoW) and only invoke this.captchaService.logCaptchaReplay(pid, headers, timestamp, ip) when the error indicates "Invalid or expired challenge" (or the new ReplayCaptchaException); rethrow the original error otherwise. Ensure references: captchaService.verifyPoW, captchaService.logCaptchaReplay, and the controller's verify catch block are updated accordingly.web/app/routes/projects.settings.$id.tsx (1)
130-133: ⚡ Quick winValidate
captchaDifficultyModebefore forwarding to backend.
captchaDifficultyModeis accepted as any string and passed through. Add a strict'manual' | 'auto'guard at the route boundary to avoid propagating invalid values.Proposed hardening
- const captchaDifficultyMode = formData - .get('captchaDifficultyMode') - ?.toString() + const rawCaptchaDifficultyMode = formData + .get('captchaDifficultyMode') + ?.toString() + const captchaDifficultyMode = + rawCaptchaDifficultyMode === 'manual' || + rawCaptchaDifficultyMode === 'auto' + ? rawCaptchaDifficultyMode + : undefined + + if ( + rawCaptchaDifficultyMode !== undefined && + captchaDifficultyMode === undefined + ) { + return data<ProjectSettingsActionData>( + { intent, error: 'Invalid captcha difficulty mode' }, + { status: 400 }, + ) + } @@ if (captchaDifficultyMode !== undefined) updateData.captchaDifficultyMode = captchaDifficultyModeAlso applies to: 177-178
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/app/routes/projects.settings`.$id.tsx around lines 130 - 133, The code reads captchaDifficultyMode from formData and forwards it without validation; add a strict guard that only accepts 'manual' or 'auto' (or omits the field) before sending to the backend. In the route handler where formData.get('captchaDifficultyMode') is used (variable captchaDifficultyMode), validate the string against the allowed union ('manual' | 'auto') and coerce to undefined/null when it doesn't match; apply the same guard at the other occurrence that reads captchaDifficultyMode (lines referenced in the review). Reject or sanitize any other values so only 'manual' or 'auto' are propagated.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@web/app/api/api.server.ts`:
- Around line 1535-1565: The CaptchaChartData interface incorrectly requires
results even though charts may now be sent as series-only
(generated/passed/failed/validationFailed/replayed); update CaptchaChartData
(the interface named CaptchaChartData) to make results optional (results?:
number[]) and verify any downstream guards or consumers that access results in
code handling CaptchaDataResponse.chart handle undefined (e.g., check for
chart.results before using it or prefer the new generated/passed/... arrays).
---
Outside diff comments:
In `@web/app/pages/Project/tabs/Captcha/CaptchaView.tsx`:
- Around line 189-193: The code is incorrectly gating chart rendering on
chart.results which causes valid multi-series payloads to be dropped; update the
conditional in the component (where chart and setChartData are used) to check
for chart and chart.x (or other required axis/series existence) instead of
requiring chart.results so that multi-series charts (e.g.,
generated/passed/failed) are accepted; modify the block that currently reads "if
(chart && chart.x && chart.results) { setChartData(chart) } else {
setChartData(null) }" to only require the necessary axis/series presence (e.g.,
chart && chart.x) before calling setChartData(chart).
---
Nitpick comments:
In `@backend/apps/cloud/src/captcha/captcha.controller.ts`:
- Around line 126-143: The catch in the verify handler treats every error from
captchaService.verifyPoW as a replay and calls logCaptchaReplay; change it to
only call logCaptchaReplay for genuine replay errors by distinguishing error
types/messages: inspect the thrown error from verifyPoW (e.g., check instanceof
BadRequestException and the error.message or introduce and check a dedicated
ReplayCaptchaException from captchaService.verifyPoW) and only invoke
this.captchaService.logCaptchaReplay(pid, headers, timestamp, ip) when the error
indicates "Invalid or expired challenge" (or the new ReplayCaptchaException);
rethrow the original error otherwise. Ensure references:
captchaService.verifyPoW, captchaService.logCaptchaReplay, and the controller's
verify catch block are updated accordingly.
In `@backend/apps/cloud/src/captcha/captcha.service.ts`:
- Line 335: The error log in the catch block incorrectly names the method as
logCaptchaPass; update the log message to reference the actual method name
logCaptchaEvent (or use a generic contextual message) so it accurately reflects
where the error occurred; locate the catch in the logCaptchaEvent function and
change the logger.error call from referencing logCaptchaPass to logCaptchaEvent
(include the error object in the log as currently done).
- Around line 156-164: The incrCounter function can lose the TTL in
high-concurrency races because it calls redis.incr(key) then conditionally
redis.expire(key, ttl); fix by making the INCR+EXPIRE atomic: either use a Redis
pipeline/transaction to call incr and expire together, or execute a small Lua
script that runs INCR and sets EXPIRE when the new value is 1, or simply always
call redis.expire(key, ttl) after redis.incr(key) to rely on Redis idempotency;
update the incrCounter function to use one of these approaches (referencing
incrCounter, redis.incr, redis.expire) so the TTL is reliably set.
In `@backend/apps/community/src/captcha/captcha.controller.ts`:
- Around line 126-143: The catch block around this.captchaService.verifyPoW
currently treats every thrown error as a replay; change it to inspect the thrown
error (e.g., check error type/class or a distinguishing property like error.code
or error.message for a replay marker such as 'REPLAY' or a CaptchaReplayError)
and only call this.captchaService.logCaptchaReplay(pid, headers, timestamp, ip)
when the error indicates an actual replay; for all other errors rethrow
immediately (preserving the original error) and avoid calling logCaptchaReplay.
Ensure the logic references verifyPoW and logCaptchaReplay and preserves
original exceptions when not handling replay cases.
In `@backend/apps/community/src/captcha/captcha.service.ts`:
- Line 335: The error log in the logCaptchaEvent method incorrectly references
`logCaptchaPass`; update the logger call in `logCaptchaEvent` to reference the
correct method name (`logCaptchaEvent`) and include the caught error `e` for
context (i.e., change the message string passed to `this.logger.error` to
mention `logCaptchaEvent` while still logging the error variable). Ensure the
change is made in the `logCaptchaEvent` method so the log message accurately
reflects the originating function.
In `@web/app/routes/projects.settings`.$id.tsx:
- Around line 130-133: The code reads captchaDifficultyMode from formData and
forwards it without validation; add a strict guard that only accepts 'manual' or
'auto' (or omits the field) before sending to the backend. In the route handler
where formData.get('captchaDifficultyMode') is used (variable
captchaDifficultyMode), validate the string against the allowed union ('manual'
| 'auto') and coerce to undefined/null when it doesn't match; apply the same
guard at the other occurrence that reads captchaDifficultyMode (lines referenced
in the review). Reject or sanitize any other values so only 'manual' or 'auto'
are propagated.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a1c2085f-9f30-4d97-92fa-8f0d77f0342c
📒 Files selected for processing (39)
backend/apps/cloud/src/analytics/analytics.service.tsbackend/apps/cloud/src/analytics/utils/transformers.tsbackend/apps/cloud/src/captcha/captcha.controller.tsbackend/apps/cloud/src/captcha/captcha.service.tsbackend/apps/cloud/src/captcha/dtos/validate.dto.tsbackend/apps/cloud/src/common/constants.tsbackend/apps/cloud/src/project/dto/update-project.dto.tsbackend/apps/cloud/src/project/entity/project.entity.tsbackend/apps/cloud/src/project/project.controller.tsbackend/apps/cloud/src/project/project.service.tsbackend/apps/community/src/analytics/analytics.service.tsbackend/apps/community/src/analytics/utils/transformers.tsbackend/apps/community/src/captcha/captcha.controller.tsbackend/apps/community/src/captcha/captcha.service.tsbackend/apps/community/src/captcha/dtos/validate.dto.tsbackend/apps/community/src/common/constants.tsbackend/apps/community/src/common/utils.tsbackend/apps/community/src/project/dto/update-project.dto.tsbackend/apps/community/src/project/entity/project.entity.tsbackend/apps/community/src/project/project.controller.tsbackend/migrations/clickhouse/initialise_selfhosted.jsbackend/migrations/clickhouse/selfhosted_2026_01_02_captcha.jsbackend/migrations/clickhouse/selfhosted_2026_05_20_captcha_auto_difficulty.jsbackend/migrations/mysql/2026_05_20_captcha_auto_difficulty.sqldocs/content/docs/analytics-dashboard/captcha.mdxdocs/content/docs/captcha/introduction.mdxdocs/content/docs/captcha/server-side-validation.mdxpackages/captcha-validator/README.mdpackages/captcha-validator/src/index.tspackages/captcha-validator/src/utils.tsweb/app/api/api.server.tsweb/app/lib/models/Project.tsweb/app/pages/Project/Settings/ProjectSettings.tsxweb/app/pages/Project/View/ViewProject.helpers.tsxweb/app/pages/Project/tabs/Captcha/CaptchaChart.tsxweb/app/pages/Project/tabs/Captcha/CaptchaView.tsxweb/app/routes/projects.settings.$id.tsxweb/app/ui/CaptchaSetup.tsxweb/public/locales/en.json
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/apps/cloud/src/project/project.controller.ts (1)
801-806:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMissing length limit on
countryBlacklist.
ipBlacklistis sliced toBLACKLIST_ITEMS_MAXIMUMat line 795, butcountryBlacklisthas no such limit. This inconsistency may leave the CodeQL "Loop bound injection" issue unresolved for this field.Proposed fix
if (projectDTO.countryBlacklist !== undefined) { project.countryBlacklist = projectDTO.countryBlacklist && Array.isArray(projectDTO.countryBlacklist) - ? (_map(projectDTO.countryBlacklist, _trim) as string[]) + ? (_map( + projectDTO.countryBlacklist.slice(0, BLACKLIST_ITEMS_MAXIMUM), + _trim, + ) as string[]) : null }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/apps/cloud/src/project/project.controller.ts` around lines 801 - 806, The countryBlacklist field is not being bounded like ipBlacklist and can trigger a Loop bound injection; update the same method that slices ipBlacklist (referencing ipBlacklist and BLACKLIST_ITEMS_MAXIMUM) to also slice/limit countryBlacklist to BLACKLIST_ITEMS_MAXIMUM before any loop/processing and before returning the response (look for usages of countryBlacklist in the controller method to ensure the truncated array is used everywhere).
♻️ Duplicate comments (1)
backend/apps/community/src/project/project.controller.ts (1)
1063-1067:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMissing length limit on
origins.The CodeQL "Loop bound injection" issue at line 1065 persists because
projectDTO.originsis mapped without any length bound. The cloud version uses.slice(0, PROJECTS_MAXIMUM)for this field.Proposed fix
+const PROJECTS_MAXIMUM = 50 + if (projectDTO.origins !== undefined) { - project.origins = projectDTO.origins - ? (_map(projectDTO.origins, _trim) as string[]) + const safeOrigins = Array.isArray(projectDTO.origins) + ? projectDTO.origins.slice(0, PROJECTS_MAXIMUM) + : [] + + project.origins = safeOrigins.length > 0 + ? (_map(safeOrigins, _trim) as string[]) : [] }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/apps/community/src/project/project.controller.ts` around lines 1063 - 1067, The assignment to project.origins maps projectDTO.origins without bounding its length, allowing a Loop bound injection; before mapping in ProjectController (the block handling projectDTO.origins), limit the input array to PROJECTS_MAXIMUM (e.g., use projectDTO.origins.slice(0, PROJECTS_MAXIMUM)) and then apply _map(..., _trim) so project.origins is set from the sliced-and-trimmed array (or [] when falsy), ensuring the same length cap used elsewhere.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@backend/apps/cloud/src/project/project.controller.ts`:
- Around line 801-806: The countryBlacklist field is not being bounded like
ipBlacklist and can trigger a Loop bound injection; update the same method that
slices ipBlacklist (referencing ipBlacklist and BLACKLIST_ITEMS_MAXIMUM) to also
slice/limit countryBlacklist to BLACKLIST_ITEMS_MAXIMUM before any
loop/processing and before returning the response (look for usages of
countryBlacklist in the controller method to ensure the truncated array is used
everywhere).
---
Duplicate comments:
In `@backend/apps/community/src/project/project.controller.ts`:
- Around line 1063-1067: The assignment to project.origins maps
projectDTO.origins without bounding its length, allowing a Loop bound injection;
before mapping in ProjectController (the block handling projectDTO.origins),
limit the input array to PROJECTS_MAXIMUM (e.g., use projectDTO.origins.slice(0,
PROJECTS_MAXIMUM)) and then apply _map(..., _trim) so project.origins is set
from the sliced-and-trimmed array (or [] when falsy), ensuring the same length
cap used elsewhere.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ca7dba24-4a60-4ddf-bee7-5d17f13c4ea0
📒 Files selected for processing (2)
backend/apps/cloud/src/project/project.controller.tsbackend/apps/community/src/project/project.controller.ts
Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Changes
If applicable, please describe what changes were made in this pull request.
Community Edition support
Database migrations
Documentation
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Style/UX