fix: code-review remediation — /scan/iris quota (H1), signing semaphore (M1), CF-IP (M2) - #8
fix: code-review remediation — /scan/iris quota (H1), signing semaphore (M1), CF-IP (M2)#8Hans1132 wants to merge 5 commits into
Conversation
…er JSON asyncSign released the concurrency semaphore on the success branch and then, if JSON.parse threw on malformed sign-report.py output, routed into fail() which released it a SECOND time. The unconditional _active-- drifted negative, silently raising effective concurrency above SIGN_CONCURRENCY exactly in the degraded state the cap exists to protect (CLAUDE.md sharp edge: semaphore must be released on every exit path — here it was released twice on one path). Add a settled guard so the semaphore is released and the promise settled exactly once, regardless of which listener fires (close/error/stdin EPIPE) or a parse failure. Regression test feeds a python stub that exits 0 with invalid JSON in a burst and asserts the in-flight counter returns to baseline. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HG8kPjhmqwRyjnZAYiYkTb
…/scan/free H1: /scan/iris ran checkBlacklist+validateSolanaAddress but NOT the atomic checkFreeQuota middleware; it read getQuotaStatus() then called consumeFreeQuota(), which is a no-op outside the middleware. So `used` never incremented, `remaining` stayed at the limit, and the documented 3/IP/day free cap never tripped (only the in-memory 10 req/min limiter applied). Route the endpoint through checkFreeQuota (atomic check+consume, internal calls skipped) and drop the dead inline block. M2: /scan/free used req.ip for the internal-A2A CAPTCHA bypass, abuse logging, and quota keying, violating sharp edge #1 (trust only CF-Connecting-IP). Swap to _getClientIp(req) to match the rest of the codebase and remove the XFF spoof surface. Same no-op consumeFreeQuota pattern remains at two other call sites (server.js ~1963/1973 and /scan/free 4565/4633) — flagged for follow-up, out of this approved scope. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HG8kPjhmqwRyjnZAYiYkTb
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThree bug fixes: free-quota middleware is enhanced with atomic ChangesBackend/Security Remediation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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 |
|
@coderabbitai review Generated by Claude Code |
|
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server.js (1)
4553-4567:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftReplace
/scan/freeinline quota consumption with the atomic middleware path.Line 4553 now keys quota by the trusted helper, but this route still uses the same
getQuotaStatus()+consumeFreeQuota()pattern that the/scan/irisfix documents as ineffective outside middleware. This leaves/scan/freequota enforcement bypassable even with correct IP attribution. Extract CAPTCHA/type/address validation ahead of the scan handler, then runcheckFreeQuotabefore executing or caching a free scan, usingreq.freeQuotafor response metadata.Also applies to: 4621-4624
🤖 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 `@server.js` around lines 4553 - 4567, The /scan/free route currently uses an inline quota consumption pattern with getQuotaStatus() and consumeFreeQuota() which is vulnerable to bypass because it's not atomic. Replace this approach by extracting CAPTCHA, type, and address validation logic ahead of the scan handler, then apply the atomic checkFreeQuota middleware before the scan is executed or cached. After the middleware validates quota, use req.freeQuota to access quota metadata for response construction instead of calling getQuotaStatus() inline. This ensures quota enforcement is atomic and cannot be bypassed.
🤖 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 `@memory.md`:
- Around line 15-20: The 2026-06-17 memory.md entry documenting code-review
remediation (H1/M1/M2) is missing the required backup path field as specified in
the coding guideline. Add a new bullet point under the existing entry that
documents the backup or rollback path (such as a database backup location with
timestamp or a git commit hash for rollback), following the pattern used in
other entries like the 2026-05-21 ones. This is particularly important for
security-sensitive changes affecting payment and signing functionality noted as
§6 requiring Hans's manual review.
In `@server.js`:
- Around line 4463-4465: The CAPTCHA bypass logic in the isInternalA2A variable
uses _getClientIp() which has unsafe fallback behavior, creating a security
vulnerability where any request with the x-a2a-caller header could bypass
CAPTCHA if the CF-Connecting-IP header is missing. Replace the _getClientIp()
authorization check with isInternalCall() helper instead, which properly
validates socket locality rather than relying on header-based IP checks. Keep
_getClientIp() available only for logging and attribution purposes if needed
elsewhere in the code.
---
Outside diff comments:
In `@server.js`:
- Around line 4553-4567: The /scan/free route currently uses an inline quota
consumption pattern with getQuotaStatus() and consumeFreeQuota() which is
vulnerable to bypass because it's not atomic. Replace this approach by
extracting CAPTCHA, type, and address validation logic ahead of the scan
handler, then apply the atomic checkFreeQuota middleware before the scan is
executed or cached. After the middleware validates quota, use req.freeQuota to
access quota metadata for response construction instead of calling
getQuotaStatus() inline. This ensures quota enforcement is atomic and cannot be
bypassed.
🪄 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 Plus
Run ID: 989bde8f-17e0-400e-b256-b48710e0b424
📒 Files selected for processing (4)
memory.mdserver.jssrc/crypto/sign.jstests/crypto/sign-spof.test.js
…back fallback Addresses CodeRabbit review on PR #8. _getClientIp(req) returns the CF-Connecting-IP header or falls back to 127.0.0.1 when it is absent, so gating the internal-A2A CAPTCHA bypass on `_getClientIp(req) === '127.0.0.1'` would let `x-a2a-caller: 1` skip CAPTCHA on any request that reaches Express without the CF header. Use the canonical isInternalCall(req) predicate (loopback set OR x-internal-secret) for the bypass; _getClientIp stays for attribution/logging. Also add the §10 backup/rollback field to the 2026-06-17 memory.md entry. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HG8kPjhmqwRyjnZAYiYkTb
|
Re: the outside-diff finding on Confirmed valid. The maintainer has decided to defer this to a follow-up rather than expand this PR's scope. Unlike Tracked in this PR's description ("Not in scope") and in Generated by Claude Code |
Addresses CodeRabbit CR-3 on PR #8. /scan/quick and /scan/free read quota via getQuotaStatus() then called consumeFreeQuota(), which is a no-op outside the checkFreeQuota middleware — so neither route ever incremented the per-IP/global counters and their shared 3/IP/day free cap was bypassable (same class as the /scan/iris H1 fix). Neither route can simply adopt the checkFreeQuota route middleware: /scan/quick must skip quota for paid (API-key / verified x402) callers, and /scan/free must consume only after CAPTCHA + a cache miss (cache hits stay free). So add an atomic tryConsumeFreeQuota(ip) helper (wraps the existing tested checkAndConsumeTx transaction, fails open on DB error) and call it inline, inside each route's existing paid/CAPTCHA/cache gating and guarded by !isInternalCall(req). The 3/IP/day budget is now shared and enforced across /scan/iris, /scan/quick, and /scan/free. consumeFreeQuota stays a no-op for backward compat. Adds tryConsumeFreeQuota unit tests (atomic increment, denial past limit, shared budget with the middleware). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HG8kPjhmqwRyjnZAYiYkTb
|
Update on the Approach: both routes can't use the Generated by Claude Code |
|
@coderabbitai review Generated by Claude Code |
|
✏️ Learnings added
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/middleware/free-quota.test.js (1)
221-255: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd branch coverage for
tryConsumeFreeQuotaglobal and DB-error paths.The new tests validate IP-limit behavior, but they miss two new helper branches:
denied: 'global'and fail-open{ ok: true, dbError: true }. Adding those two cases will lock in the intended behavior and prevent silent regressions.🤖 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 `@tests/middleware/free-quota.test.js` around lines 221 - 255, The test suite for tryConsumeFreeQuota is missing coverage for two important branches: the global quota exhaustion case that should return denied with value 'global', and the database error fail-open path that should return ok: true with dbError: true. Add two new test cases after the existing tryConsumeFreeQuota tests: one that exhausts the global quota limit (PER_GLOBAL_DAILY_LIMIT) and verifies the denied: 'global' response is returned, and another that simulates a database error condition and verifies that the function fails open by returning ok: true with dbError: true to allow the request through while logging the error.
🤖 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 `@server.js`:
- Around line 1953-1959: The tryConsumeFreeQuota function can return denied as
either 'global' or a per-IP denial, but the current code at the quota.denied
check treats all denials as per-IP denials and always responds with per-IP quota
messages and data. You need to separate the handling of global denials from
per-IP denials: add a specific check for quota.denied === 'global' to handle
global cap denials separately with different response data (using globalUsed and
global_limit fields), and keep the existing per-IP quota response for non-global
denials. Apply this same fix in both locations where this pattern appears
(around line 1954 and the other location mentioned at lines 4627-4634).
---
Nitpick comments:
In `@tests/middleware/free-quota.test.js`:
- Around line 221-255: The test suite for tryConsumeFreeQuota is missing
coverage for two important branches: the global quota exhaustion case that
should return denied with value 'global', and the database error fail-open path
that should return ok: true with dbError: true. Add two new test cases after the
existing tryConsumeFreeQuota tests: one that exhausts the global quota limit
(PER_GLOBAL_DAILY_LIMIT) and verifies the denied: 'global' response is returned,
and another that simulates a database error condition and verifies that the
function fails open by returning ok: true with dbError: true to allow the
request through while logging the error.
🪄 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 Plus
Run ID: 40ff3b16-5932-4e49-8be2-37cf8d28025b
📒 Files selected for processing (4)
memory.mdserver.jssrc/middleware/free-quota.jstests/middleware/free-quota.test.js
✅ Files skipped from review due to trivial changes (1)
- memory.md
|
@coderabbitai full review Generated by Claude Code |
|
✅ Action performedFull review finished. |
…responses Addresses CodeRabbit review on PR #8. tryConsumeFreeQuota() can deny with either 'ip' (per-IP daily cap) or 'global' (global daily cap), but the inline handlers on /scan/quick and /scan/free treated every denial as per-IP — mislabeling a global-cap denial and dropping the global_used/global_limit fields. Branch on denied === 'global' first (matching the global-cap response shape already used elsewhere in /scan/free), then fall through to the per-IP response. Adds free-quota tests for the denied:'global' branch and the DB-error fail-open path ({ ok: true, dbError: true }). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HG8kPjhmqwRyjnZAYiYkTb
2 participants
Context
Remediation of three findings from a code review of the payment / signing / input-validation
surfaces. Payment- and crypto-sensitive — opened as draft for Hans's manual review (§6).
Each change is surgical; the full review (with discarded false positives) is in the session.
Changes
H1 —
/scan/irisnever consumed the daily free quota (fix(scan))The route ran
checkBlacklist + validateSolanaAddressbut not the atomiccheckFreeQuotamiddleware; it read
getQuotaStatus()then calledconsumeFreeQuota(), which is a no-opoutside the middleware. So
usednever incremented,remainingstayed at the limit, and thedocumented 3/IP/day cap never tripped — only the in-memory 10 req/min limiter applied.
→ Route through
checkFreeQuota(atomic check+consume, internal calls skipped); delete thedead inline block.
M1 — signing semaphore double-released on invalid signer output (
fix(security))asyncSignreleased the semaphore on the success branch, then aJSON.parsefailure routedinto
fail()which released it again. The unconditional_active--drifted negative,raising effective concurrency above
SIGN_CONCURRENCYexactly in the degraded state the capprotects.
→
settledguard releases the semaphore / settles the promise exactly once, for anylistener (close/error/EPIPE) or parse failure. Regression test asserts the in-flight counter
returns to baseline after a burst of invalid-JSON-but-exit-0 signs.
M2 —
req.ipinstead ofCF-Connecting-IPin/scan/free(fix(scan))req.ipwas used for the internal-A2A CAPTCHA bypass, abuse logging, and quota keying —violating sharp-edge #1. →
_getClientIp(req)at all three sites, matching the rest of thecodebase and removing the XFF-spoof surface.
Verification
scripts/test-gate.sh: 216 assertions pass, 0 fail across all suites.tests/crypto/sign-spof.test.js(3/3);free-quota.test.js15/15.npm auditstep (aqsDoS CVE in themcp/subpackage deps) — unrelated to this diff; left untouched (lockfile change needsyour OK per §5).
Not in scope (flagged for follow-up)
consumeFreeQuotapattern survives at two more call sites(
server.js~1963/1973 and/scan/free4565/4633) — left for a follow-up to avoid scope creep.credit-waste on tx-fetch failure, non-canonical snapshot hash, etc.) remain open.
🤖 Generated with Claude Code
Generated by Claude Code
Summary by CodeRabbit
Release Notes