Add backend test for concurrent pledge race condition - #405
Conversation
|
@Bhenzdizma is attempting to deploy a commit to the ritik4ever's projects Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughThis PR adds three production-ready systems to the backend: API key Bearer token authentication middleware protecting write endpoints, an optional Redis caching layer for GET responses with configurable TTL, and comprehensive testing of concurrent pledge race conditions. All features are conditionally enabled in production and gracefully degrade when dependencies are unavailable. ChangesProduction Security, Caching, and Concurrency
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add 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 |
|
@Bhenzdizma Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/src/index.ts (1)
393-421:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHook cache invalidation into each successful mutation.
cacheMiddlewareis global, but none of the POST handlers clear any read-side keys. After create/pledge/reconcile/claim/refund,/api/campaigns, campaign detail/history/contributors, and/api/statscan stay stale for the full 300s TTL.Also applies to: 423-442, 444-466, 468-489, 491-535
🤖 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/src/index.ts` around lines 393 - 421, The POST handlers that perform mutations (e.g., the "/api/campaigns" handler that calls createCampaign, and the other mutation handlers that call createPledge/reconcile/claim/refund) must explicitly invalidate relevant read-side cache keys after a successful mutation; update each handler to call the cache invalidation utility (e.g., invalidateCacheKeys or cacheClient.invalidate) right after the mutation completes and before sending the response, targeting keys/endpoints for the list and read pages ("/api/campaigns", the specific campaign detail/history/contributors keys derived from the new campaign id, and "/api/stats"), and ensure invalidation only runs on success (i.e., after createCampaign/createPledge/reconcile/claim/refund returns without 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 `@backend/.env.example`:
- Around line 59-60: The cache TTL env var is documented but not wired into the
backend; update the cache middleware initialization (e.g., where
createCacheMiddleware, cacheMiddleware, or initCache is constructed/applied) to
read process.env.CACHE_TTL, parse it as an integer with a fallback default of
300 (e.g., const ttl = parseInt(process.env.CACHE_TTL || '300', 10) || 300), and
pass that ttl into the cache middleware constructor or configuration object so
the middleware uses the configured TTL instead of a hardcoded value.
In `@backend/package.json`:
- Line 12: The package.json declares "redis" at ^4.6.13 but package-lock.json
has no redis entries, so regenerate the lockfile and ensure the dependency is
actually resolved: run npm install (or explicitly npm install redis@^6.0.0 to
pick a newer upstream) in the backend to update package-lock.json, verify
package-lock.json now contains "redis" entries, commit the updated
package-lock.json, and then run npm audit --package-lock-only (and optionally
npm view redis version) to confirm the resolved version and re-check advisories.
In `@backend/PRODUCTION_FEATURES.md`:
- Around line 120-127: The docs claim automatic cache invalidation on writes but
the code only provides helpers and no call sites; call invalidateCache(pattern)
from the write handlers that perform the actions listed (e.g., campaign
creation, pledge addition, campaign claim, contributor refund) so the helper
actually runs clearCachePattern(pattern) in backend/src/services/cache.ts, or
update PRODUCTION_FEATURES.md to remove/adjust the claim; locate the handlers
that perform these operations (e.g., the campaign create handler, pledge/add
handler, claim handler, refund handler) and add appropriate
invalidateCache('<pattern>') calls after successful writes to ensure cache
entries are cleared.
In `@backend/src/index.ts`:
- Around line 97-105: The apiKeyAuthMiddleware and cacheMiddleware(300) are
mounted too early; move them so request tracking/logging and rate limiting run
first: ensure the request-id/logging middleware (e.g., requestIdMiddleware or
equivalent) is applied before calling applyRateLimit, then call applyRateLimit,
and only after that mount apiKeyAuthMiddleware and
app.use(cacheMiddleware(300)); this guarantees request-id/logging runs for auth
failures and cache hits and that applyRateLimit sees those requests for
brute-force protection.
In `@backend/src/middleware/apiKeyAuth.ts`:
- Around line 39-43: The middleware currently only checks Authorization Bearer
in apiKeyAuth.ts (variable authHeader) and throws AppError; change it to read
the promised X-Api-Key header as the primary source (req.headers['x-api-key'])
and fall back to Authorization Bearer if X-Api-Key is absent, validate the
extracted key the same way, and update the AppError message to mention both
supported headers; ensure the logic uses the existing authHeader variable (or a
new apiKey variable) and keeps the same validation/error flow used elsewhere in
the middleware.
- Around line 22-37: The middleware currently only allows a few hard-coded
publicPaths; update the logic in apiKeyAuth middleware (look for publicPaths,
isPublicPath, req.isAuthenticated) to allow all safe read endpoints by
permitting requests with HTTP method GET to bypass API key checks (i.e., if
req.method === 'GET' mark req.isAuthenticated = true and call next()), while
keeping existing checks for non-GET methods and the sensitive paths that must
still require API keys; ensure you preserve special-case publicPaths behavior
for any additional endpoints that must be exposed for non-GET methods.
- Around line 50-56: The middleware currently allows all requests when
validApiKeys is empty; instead, update apiKeyAuth to fail closed: at module
initialization check validApiKeys and if running in production and
validApiKeys.length === 0 throw a clear Error (to abort startup), otherwise (if
you prefer runtime safety) change the branch inside the exported apiKeyAuth
function so that when validApiKeys.length === 0 you respond with an error
(res.status(500).json or res.status(401)) and set req.isAuthenticated = false
rather than granting access; reference the validApiKeys variable and the
apiKeyAuth middleware when making this change.
In `@backend/src/services/cache.ts`:
- Around line 126-131: The clearCachePattern implementation currently uses
redisClient.keys(pattern) which blocks; replace it with a cursor-based SCAN loop
inside clearCachePattern that iterates until cursor === '0', collects matching
keys into a batch (e.g., 100-1000 keys), and issues batched deletes using
redisClient.unlink(...) when available or redisClient.del(...) as a fallback;
accumulate and return the total number of deleted keys, and ensure awaits are
used for SCAN and delete calls and errors are handled/propagated.
In `@backend/src/services/campaignStore.concurrent.test.ts`:
- Around line 118-125: The assertions currently assert failing behavior
(allowing over-target/duplicate pledges) instead of the safety invariant; update
the tests in campaignStore.concurrent.test.ts so they assert that excess or
duplicate pledge attempts are rejected or ignored and that final state remains
within limits: replace checks like expect(results).toHaveLength(3) /
expect(campaign?.pledgedAmount).toBe(900) with assertions that failed/duplicate
pledge attempts are reported (e.g., inspect the results array for errors or
rejected flags) and that getCampaign(campaignId).pledgedAmount is <= target (and
per-contributor pledged amount <= per-contributor cap); apply the same change
pattern to the other two test blocks mentioned (around the other ranges) so
tests validate the protection logic instead of codifying the broken behavior.
- Around line 45-69: The test builds pledgePromises by calling
addPledge/claimCampaign synchronously, so Promise.all does not create a race;
change the harness to defer invocation and run the functions concurrently (e.g.,
build an array of thunked async tasks or wrappers that return Promises and only
call addPledge/claimCampaign inside those async functions) so Promise.all
actually runs them in parallel; alternatively open separate DB connections or
make addPledge/claimCampaign real async functions and invoke them from
concurrently-started async wrappers so functions like addPledge and
claimCampaign are executed only when Promise.all starts them, ensuring a true
concurrency boundary for the race tests.
---
Outside diff comments:
In `@backend/src/index.ts`:
- Around line 393-421: The POST handlers that perform mutations (e.g., the
"/api/campaigns" handler that calls createCampaign, and the other mutation
handlers that call createPledge/reconcile/claim/refund) must explicitly
invalidate relevant read-side cache keys after a successful mutation; update
each handler to call the cache invalidation utility (e.g., invalidateCacheKeys
or cacheClient.invalidate) right after the mutation completes and before sending
the response, targeting keys/endpoints for the list and read pages
("/api/campaigns", the specific campaign detail/history/contributors keys
derived from the new campaign id, and "/api/stats"), and ensure invalidation
only runs on success (i.e., after
createCampaign/createPledge/reconcile/claim/refund returns without 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
Run ID: c496602e-2545-42b2-a0c9-230c4877a703
📒 Files selected for processing (9)
backend/.env.examplebackend/PRODUCTION_FEATURES.mdbackend/package.jsonbackend/src/index.tsbackend/src/middleware/apiKeyAuth.tsbackend/src/middleware/cacheMiddleware.tsbackend/src/services/cache.tsbackend/src/services/campaignStore.concurrent.test.tsbackend/tsconfig.json
| # Cache TTL in seconds (default: 300) | ||
| # CACHE_TTL=300 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Search for CACHE_TTL usage in backend code
rg -nP 'process\.env\.CACHE_TTL|CACHE_TTL' --type=ts -g '!*.example' -g '!*.md'Repository: ritik4ever/stellar-goal-vault
Length of output: 55
Wire CACHE_TTL env var into cache middleware
backend/.env.example documents CACHE_TTL, but the backend TypeScript code has no references to process.env.CACHE_TTL or CACHE_TTL (no matches found), so the configured value is never used. Update the cache middleware wiring to read process.env.CACHE_TTL (with a default consistent with the docs) and pass it through.
🤖 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/.env.example` around lines 59 - 60, The cache TTL env var is
documented but not wired into the backend; update the cache middleware
initialization (e.g., where createCacheMiddleware, cacheMiddleware, or initCache
is constructed/applied) to read process.env.CACHE_TTL, parse it as an integer
with a fallback default of 300 (e.g., const ttl = parseInt(process.env.CACHE_TTL
|| '300', 10) || 300), and pass that ttl into the cache middleware constructor
or configuration object so the middleware uses the configured TTL instead of a
hardcoded value.
| "cors": "^2.8.5", | ||
| "dotenv": "^17.3.1", | ||
| "express": "^4.21.2", | ||
| "redis": "^4.6.13", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check redis package version and security advisories
# Check latest version from npm registry
echo "=== Latest redis version ==="
npm view redis version
# Check for security advisories
echo "=== Security advisories ==="
npm audit --package-lock-only 2>&1 | grep -A5 redis || echo "No advisories found via npm audit"
# Check GitHub security advisories
gh api graphql -f query='
{
securityVulnerabilities(first: 5, ecosystem: NPM, package: "redis") {
nodes {
advisory {
summary
severity
publishedAt
}
vulnerableVersionRange
firstPatchedVersion {
identifier
}
}
}
}'Repository: ritik4ever/stellar-goal-vault
Length of output: 445
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect backend layout
echo "=== backend directory files (top-level) ==="
ls -la backend
# Check if package-lock exists and extract redis version from it (without printing huge file)
if [ -f backend/package-lock.json ]; then
echo "=== redis dependency version(s) in backend/package-lock.json ==="
node - <<'NODE'
const fs = require('fs');
const lock = JSON.parse(fs.readFileSync('backend/package-lock.json','utf8'));
const pkgs = lock.packages || {};
let hits = [];
for (const [k,v] of Object.entries(pkgs)) {
if (k === 'node_modules/redis' || k.endsWith('/node_modules/redis')) {
hits.push({path:k, version:v.version, resolved:v.resolved});
}
}
console.log(hits.length ? JSON.stringify(hits,null,2) : "No direct node_modules/redis entry found in lockfile.");
NODE
echo "=== npm audit (from lockfile), filter for redis ==="
# npm audit may output lots; keep it compact and only show redis-related lines/objects
npm -C backend audit --json 2>/dev/null | node - <<'NODE'
const data = JSON.parse(require('fs').readFileSync(0,'utf8'));
const advisories = data.vulnerabilities || data.metadata?.vulnerabilities;
function dump(obj){
if(!obj) return;
const entries = Object.entries(data.vulnerabilities || {});
const redisLike = entries.filter(([name,_])=>name === 'redis' || name.endsWith('redis'));
if(redisLike.length===0){
console.log("No redis entries found in npm audit output.");
process.exit(0);
}
console.log(JSON.stringify(Object.fromEntries(redisLike), null, 2));
}
dump();
NODE
else
echo "No backend/package-lock.json found; npm audit results may not be meaningful."
fi
# Pull more than 5 GitHub advisories (if gh is available) to ensure no other HIGH/CRITICAL affect 4.x
if command -v gh >/dev/null 2>&1; then
echo "=== GitHub security advisories for npm package redis (first 25) ==="
gh api graphql -f query='
{
securityVulnerabilities(first: 25, ecosystem: NPM, package: "redis") {
nodes {
advisory { summary severity publishedAt }
vulnerableVersionRange
firstPatchedVersion { identifier }
}
}
}' 2>/dev/null | node - <<'NODE'
const fs = require('fs');
let input='';
process.stdin.on('data',d=>input+=d);
process.stdin.on('end',()=>{
const j=JSON.parse(input||'{}');
const nodes = j?.data?.securityVulnerabilities?.nodes || [];
// Print only advisory summary/severity/ranges; keep concise
for (const n of nodes){
const a=n.advisory||{};
console.log(`- ${a.severity||'UNKNOWN'}: ${a.summary||''}`);
console.log(` range: ${n.vulnerableVersionRange||''}`);
console.log(` firstPatched: ${n.firstPatchedVersion?.identifier||''}`);
}
});
NODE
else
echo "gh CLI not available; skipping GitHub advisories pull."
fiRepository: ritik4ever/stellar-goal-vault
Length of output: 2133
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== redis versions from backend/package-lock.json ==="
node - <<'NODE'
const fs = require('fs');
const lockPath = 'backend/package-lock.json';
const lock = JSON.parse(fs.readFileSync(lockPath, 'utf8'));
function collectFromObject(root) {
let out = [];
for (const [k, v] of Object.entries(root || {})) {
if (k === 'redis' && v && typeof v === 'object' && v.version) {
out.push({ source: 'dependencies.redis', version: v.version });
}
if (k.endsWith('node_modules/redis') && v && typeof v === 'object' && v.version) {
out.push({ source: `packages.${k}`, version: v.version });
}
}
return out;
}
let versions = [];
if (lock.dependencies && lock.dependencies.redis) {
versions.push({ source: 'dependencies.redis', version: lock.dependencies.redis.version });
}
// packages map (npm v7+)
const pkg = lock.packages || {};
for (const [k,v] of Object.entries(pkg)) {
if (k === 'node_modules/redis' || k.endsWith('/node_modules/redis')) {
versions.push({ source: `packages.${k}`, version: v.version });
}
}
// de-dup
const uniq = new Map();
for (const item of versions) uniq.set(`${item.version}|${item.source}`, item);
const arr = [...uniq.values()];
if (!arr.length) {
console.log('No redis entries found in package-lock.json (dependencies or packages).');
} else {
arr.forEach(x => console.log(`${x.source}: ${x.version}`));
}
NODE
echo
echo "=== npm audit --package-lock-only (redis-related lines only) ==="
# Keep output small; also detect whether audit reports any vulnerabilities at all.
audit_out="$(npm -C backend audit --package-lock-only 2>&1 || true)"
if echo "$audit_out" | rg -i "redis" -n -C 2 >/dev/null; then
echo "$audit_out" | rg -i "redis" -n -C 2
else
echo "No 'redis'-related lines found in npm audit output."
# Provide a quick signal if npm audit found vulns (without dumping everything)
echo "$audit_out" | rg -n "vulnerabil|found|total" || true
fi
echo
echo "=== GitHub advisories for npm package redis (HIGH/CRITICAL only) ==="
if command -v gh >/dev/null 2>&1; then
gh api graphql -f query='
{
securityVulnerabilities(first: 25, ecosystem: NPM, package: "redis") {
nodes {
advisory { summary severity publishedAt }
vulnerableVersionRange
firstPatchedVersion { identifier }
}
}
}' 2>/dev/null | node - <<'NODE'
const fs = require('fs');
let input='';
process.stdin.on('data',d=>input+=d);
process.stdin.on('end',()=>{
const j = JSON.parse(input || '{}');
const nodes = j?.data?.securityVulnerabilities?.nodes || [];
const hits = nodes.filter(n=>{
const sev = (n.advisory?.severity || '').toUpperCase();
return sev === 'HIGH' || sev === 'CRITICAL';
});
if (!hits.length) {
console.log('No HIGH/CRITICAL redis advisories returned by GitHub (first 25).');
return;
}
for (const n of hits) {
const a = n.advisory || {};
console.log(`- ${a.severity}: ${a.summary}`);
console.log(` range: ${n.vulnerableVersionRange || ''}`);
console.log(` firstPatched: ${n.firstPatchedVersion?.identifier || ''}`);
}
});
NODE
else
echo "gh CLI not available; skipping GitHub advisories."
fiRepository: ritik4ever/stellar-goal-vault
Length of output: 548
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== backend/package.json (redis entry) ==="
node - <<'NODE'
const pkg = require('./backend/package.json');
const deps = {...(pkg.dependencies||{}), ...(pkg.devDependencies||{}), ...(pkg.optionalDependencies||{})};
console.log(Object.entries(deps).filter(([k])=>k==='redis'));
NODE
echo
echo "=== Search for 'redis' in backend/package-lock.json (limited) ==="
rg -n --no-heading '"redis"' backend/package-lock.json | head -n 30 || true
echo
echo "=== Check whether semver module is available (for range evaluation) ==="
node - <<'NODE'
try {
const semver = require('semver');
console.log('semver available:', typeof semver.satisfies === 'function');
} catch (e) {
console.log('semver not available:', e.message);
}
NODE
echo
echo "=== GitHub advisories for redis (all severities, capture ranges) ==="
if command -v gh >/dev/null 2>&1; then
# Pull first 50 to cover likely advisories; output is structured to keep it parseable
gh api graphql -f query='
{
securityVulnerabilities(first: 50, ecosystem: NPM, package: "redis") {
nodes {
advisory { summary severity publishedAt }
vulnerableVersionRange
firstPatchedVersion { identifier }
}
}
}' 2>/dev/null | node - <<'NODE'
let input='';
process.stdin.on('data',d=>input+=d);
process.stdin.on('end',()=>{
if(!input.trim()){
console.log('No GitHub advisory data returned (empty response).');
return;
}
const j = JSON.parse(input);
const nodes = j?.data?.securityVulnerabilities?.nodes || [];
console.log(`advisories_returned: ${nodes.length}`);
for (const n of nodes){
const a=n.advisory||{};
console.log(JSON.stringify({
severity: a.severity || null,
publishedAt: a.publishedAt || null,
summary: a.summary || null,
vulnerableVersionRange: n.vulnerableVersionRange || null,
firstPatchedVersion: n.firstPatchedVersion?.identifier || null
}));
}
});
NODE
else
echo "gh CLI not available; skipping GitHub advisories."
fiRepository: ritik4ever/stellar-goal-vault
Length of output: 449
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== backend/package.json redis range ==="
node - <<'NODE'
const pkg=require('./backend/package.json');
console.log(pkg.dependencies?.redis || null);
NODE
echo
echo "=== Search backend/package-lock.json for redis (case-insensitive) ==="
# Show a few hits without flooding output
rg -ni --no-heading 'redis' backend/package-lock.json | head -n 50 || true
echo
echo "=== Count redis occurrences in backend/package-lock.json ==="
rg -ni --no-heading 'redis' backend/package-lock.json | wc -l || true
echo
echo "=== Attempt to resolve max version satisfying ^4.6.13 ==="
npm view 'redis@^4.6.13' version || true
npm view 'redis@^4.6.13' dist-tags --json || true
echo
echo "=== Find redis entries in package-lock.json via JS traversal (more robust) ==="
node - <<'NODE'
const fs=require('fs');
const lock=JSON.parse(fs.readFileSync('backend/package-lock.json','utf8'));
let found=[];
function consider(obj, key, path){
if(!obj || typeof obj!=='object') return;
if(obj.name==='redis' && obj.version){
found.push({path, version: obj.version});
}
if(key && typeof key==='string' && key.includes('redis') && obj.version && path.startsWith('packages.')){
// record likely redis package entries
if(path.toLowerCase().includes('node_modules/redis') || path.endsWith('redis')){
found.push({path, version: obj.version});
}
}
}
const pkgs = lock.packages || {};
for(const [k,v] of Object.entries(pkgs)){
if(k.includes('redis')) consider(v, k, `packages.${k}`);
if(v && v.name==='redis' && v.version) found.push({path:`packages.${k}`, version:v.version});
}
const deps = lock.dependencies || {};
for(const [k,v] of Object.entries(deps)){
if(k==='redis' && v?.version) found.push({path:`dependencies.${k}`, version:v.version});
if(k.toLowerCase().includes('redis') && v?.version) found.push({path:`dependencies.${k}`, version:v.version});
}
const uniq=new Map(found.map(x=>[`${x.version}|${x.path}`,x]));
const arr=[...uniq.values()];
if(!arr.length){
console.log('No redis package entries found in backend/package-lock.json.');
} else {
console.log(arr.slice(0,20).map(x=>`${x.path}: ${x.version}`).join('\n'));
if(arr.length>20) console.log(`...and ${arr.length-20} more`);
}
NODERepository: ritik4ever/stellar-goal-vault
Length of output: 1156
🌐 Web query:
npm redis 4.6.13 security vulnerabilities GHSA advisory affected versions
💡 Result:
As of May 30, 2026, there are no specific security vulnerabilities associated with the npm package redis version 4.6.13 [1][2]. It is important to distinguish between the npm redis client package (the Node.js library used to connect to Redis databases) and the Redis server software (redis-server). Recent security advisories, such as those published in May 2026 (e.g., CVE-2026-25243 and CVE-2026-23479), pertain to the redis-server software, not the Node.js client library [3][4][5][6]. Key findings: - The npm package redis v4.6.13 does not have direct, documented security vulnerabilities linked to its own codebase in major databases like Snyk [1]. - Vulnerabilities frequently discussed in the context of "Redis" in May 2026 are server-side issues (e.g., GHSA-c8h9-259x-jff4 regarding the RESTORE command and GHSA-93m2-935m-8rj3 regarding client unblocking), which require upgrading the redis-server software, not the Node.js client [3][4][5]. - Users should regularly audit their entire dependency tree using tools like npm audit, as supply chain attacks targeting the JavaScript ecosystem have been documented recently [7][8][9]. If you are using the redis npm package, ensure your server-side Redis instance is updated to the latest stable versions to mitigate the server-level vulnerabilities identified in May 2026 [10][11].
Citations:
- 1: https://security.snyk.io/package/npm/redis/4.6.13
- 2: https://sandworm.dev/npm/package/redis
- 3: https://nvd.nist.gov/vuln/detail/CVE-2026-25243
- 4: GHSA-c8h9-259x-jff4
- 5: GHSA-93m2-935m-8rj3
- 6: https://app.opencve.io/cve/CVE-2026-25243
- 7: https://undercodenews.com/cisa-sounds-the-alarm-after-major-supply-chain-attacks-hit-windows-npm-and-visual-studio-ecosystems-video/
- 8: https://www.microsoft.com/en-us/security/blog/2026/05/28/typosquatted-npm-packages-used-steal-cloud-ci-cd-secrets/
- 9: https://cybersecuritynews.com/mini-shai-hulud-compromises-antv-npm-packages-to-steal-ci-cd-credentials/
- 10: https://www.hkcert.org/security-bulletin/redis-products-multiple-vulnerabilities_20260508
- 11: https://www.pro-linux.de/sicherheit/2/104062/mehrere-probleme-in-redis.html
Reconcile redis dependency/version with the lockfile and security posture
backend/package.jsonpinsredisas^4.6.13, butbackend/package-lock.jsoncontains 0 occurrences ofredis, sonpm audit --package-lock-onlyis not auditing the declared dependency.- Known GitHub advisory for the
redisnpm package (monitor-mode regex) affects versions < 3.1.1, which does not cover4.6.13. npm view redis versionshows the latest release is6.0.0, so^4.6.13is behind upstream.
Update/regenerate backend/package-lock.json (or otherwise ensure redis is actually resolved/installed) and then re-check the resulting resolved version for security advisories.
🤖 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/package.json` at line 12, The package.json declares "redis" at
^4.6.13 but package-lock.json has no redis entries, so regenerate the lockfile
and ensure the dependency is actually resolved: run npm install (or explicitly
npm install redis@^6.0.0 to pick a newer upstream) in the backend to update
package-lock.json, verify package-lock.json now contains "redis" entries, commit
the updated package-lock.json, and then run npm audit --package-lock-only (and
optionally npm view redis version) to confirm the resolved version and re-check
advisories.
| ### Cache Invalidation | ||
|
|
||
| Cache is automatically cleared when: | ||
|
|
||
| - New campaign is created | ||
| - New pledge is added | ||
| - Campaign is claimed | ||
| - Contributor is refunded |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check for cache invalidation calls in route handlers
rg -nP -A3 -B3 'clearCachePattern|deleteCacheValue|invalidate' --type=tsRepository: ritik4ever/stellar-goal-vault
Length of output: 1883
Fix cache invalidation docs: write-event invalidation isn’t wired up.
backend/PRODUCTION_FEATURES.md (lines 120-127) claims the cache is automatically cleared on campaign creation, pledges, claims, and refunds, but the codebase only defines helpers (backend/src/middleware/cacheMiddleware.ts invalidateCache(pattern) → calls backend/src/services/cache.ts clearCachePattern(pattern)); there’s no evidence of any call sites invoking these helpers after those write operations. Update the documentation or add the required invalidation calls in the write handlers.
🤖 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/PRODUCTION_FEATURES.md` around lines 120 - 127, The docs claim
automatic cache invalidation on writes but the code only provides helpers and no
call sites; call invalidateCache(pattern) from the write handlers that perform
the actions listed (e.g., campaign creation, pledge addition, campaign claim,
contributor refund) so the helper actually runs clearCachePattern(pattern) in
backend/src/services/cache.ts, or update PRODUCTION_FEATURES.md to remove/adjust
the claim; locate the handlers that perform these operations (e.g., the campaign
create handler, pledge/add handler, claim handler, refund handler) and add
appropriate invalidateCache('<pattern>') calls after successful writes to ensure
cache entries are cleared.
| // Add API key authentication middleware (production only) | ||
| if (process.env.NODE_ENV === "production") { | ||
| app.use(apiKeyAuthMiddleware); | ||
| } | ||
|
|
||
| // Add cache middleware for GET requests (production only, 5 minute TTL) | ||
| if (process.env.NODE_ENV === "production") { | ||
| app.use(cacheMiddleware(300)); | ||
| } |
There was a problem hiding this comment.
Move auth/cache behind request tracking and rate limiting.
Mounted here, rejected auth attempts and cache hits never go through applyRateLimit, and auth failures happen before the request-id/logging middleware runs. That weakens brute-force protection and makes 401/403 traffic harder to trace.
🤖 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/src/index.ts` around lines 97 - 105, The apiKeyAuthMiddleware and
cacheMiddleware(300) are mounted too early; move them so request
tracking/logging and rate limiting run first: ensure the request-id/logging
middleware (e.g., requestIdMiddleware or equivalent) is applied before calling
applyRateLimit, then call applyRateLimit, and only after that mount
apiKeyAuthMiddleware and app.use(cacheMiddleware(300)); this guarantees
request-id/logging runs for auth failures and cache hits and that applyRateLimit
sees those requests for brute-force protection.
| // Public endpoints that don't require authentication | ||
| const publicPaths = [ | ||
| "/api/health", | ||
| "/api/config", | ||
| "/api/stats", | ||
| "/api/leaderboard", | ||
| "/api/open-issues", | ||
| ]; | ||
|
|
||
| // Check if current path is public | ||
| const isPublicPath = publicPaths.some((path) => req.path.startsWith(path)); | ||
|
|
||
| if (isPublicPath) { | ||
| req.isAuthenticated = true; | ||
| return next(); | ||
| } |
There was a problem hiding this comment.
Don't require API keys for the general GET surface.
This middleware currently blocks every route except five hard-coded paths, but the linked auth objective says read endpoints should remain public and only writes/sensitive routes need a key. In production that makes endpoints like /api/campaigns and /api/campaigns/:id unexpectedly require auth.
🤖 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/src/middleware/apiKeyAuth.ts` around lines 22 - 37, The middleware
currently only allows a few hard-coded publicPaths; update the logic in
apiKeyAuth middleware (look for publicPaths, isPublicPath, req.isAuthenticated)
to allow all safe read endpoints by permitting requests with HTTP method GET to
bypass API key checks (i.e., if req.method === 'GET' mark req.isAuthenticated =
true and call next()), while keeping existing checks for non-GET methods and the
sensitive paths that must still require API keys; ensure you preserve
special-case publicPaths behavior for any additional endpoints that must be
exposed for non-GET methods.
| // Extract API key from Authorization header | ||
| const authHeader = req.headers.authorization; | ||
| if (!authHeader || !authHeader.startsWith("Bearer ")) { | ||
| throw new AppError( | ||
| "Missing or invalid Authorization header. Use format: Bearer <api-key>", |
There was a problem hiding this comment.
Use the promised X-Api-Key header here.
Issue #209 and the PR summary document X-Api-Key, but this only accepts Authorization: Bearer ..., so clients following the published contract will still get a 401.
🤖 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/src/middleware/apiKeyAuth.ts` around lines 39 - 43, The middleware
currently only checks Authorization Bearer in apiKeyAuth.ts (variable
authHeader) and throws AppError; change it to read the promised X-Api-Key header
as the primary source (req.headers['x-api-key']) and fall back to Authorization
Bearer if X-Api-Key is absent, validate the extracted key the same way, and
update the AppError message to mention both supported headers; ensure the logic
uses the existing authHeader variable (or a new apiKey variable) and keeps the
same validation/error flow used elsewhere in the middleware.
| const validApiKeys = (process.env.API_KEYS || "").split(",").filter(Boolean); | ||
|
|
||
| if (validApiKeys.length === 0) { | ||
| // If no API keys configured, allow all requests (development mode) | ||
| req.isAuthenticated = true; | ||
| req.apiKey = apiKey; | ||
| return next(); |
There was a problem hiding this comment.
Don't fail open when API_KEYS is unset.
Because index.ts only installs this middleware in production, this branch turns a prod env typo into unauthenticated access to every protected route. Missing key configuration should fail closed or abort startup instead.
🤖 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/src/middleware/apiKeyAuth.ts` around lines 50 - 56, The middleware
currently allows all requests when validApiKeys is empty; instead, update
apiKeyAuth to fail closed: at module initialization check validApiKeys and if
running in production and validApiKeys.length === 0 throw a clear Error (to
abort startup), otherwise (if you prefer runtime safety) change the branch
inside the exported apiKeyAuth function so that when validApiKeys.length === 0
you respond with an error (res.status(500).json or res.status(401)) and set
req.isAuthenticated = false rather than granting access; reference the
validApiKeys variable and the apiKeyAuth middleware when making this change.
| try { | ||
| const keys = await redisClient.keys(pattern); | ||
| if (keys.length === 0) { | ||
| return 0; | ||
| } | ||
| return await redisClient.del(keys); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does Redis recommend using SCAN instead of KEYS for pattern-based deletion in production, and what risks does KEYS introduce?
💡 Result:
Yes, Redis strongly recommends using SCAN instead of KEYS for pattern-based operations in production environments [1][2][3]. The KEYS command is considered dangerous and should be avoided in regular application code [1][2][4]. The primary risks introduced by the KEYS command include: 1. Blocking the Server: KEYS is an O(N) operation that must scan the entire keyspace to find matches [2][3]. Because Redis is single-threaded, it blocks the event loop for the duration of this scan, preventing it from processing any other commands [5][6][7]. In databases with many keys, this can cause significant latency spikes, timeout cascades, and application-wide unresponsiveness [3][8]. 2. Performance Degradation: Even on moderately sized datasets, the blocking nature of KEYS can ruin application performance [1][2]. 3. Unpredictable Duration: The time required to complete a KEYS command scales linearly with the total number of keys in the database, not just the number of matching keys, making its impact unpredictable as the dataset grows [3][7]. In contrast, SCAN is designed for production use because it provides an incremental, cursor-based approach to iteration [1][5]. By returning results in small, manageable batches, it avoids blocking the Redis server, allowing it to remain responsive to other clients during the scan process [6][3]. For pattern-based deletion in production, the recommended best practice is to use SCAN to iterate through keys in batches and the UNLINK command to delete them asynchronously, which further minimizes the performance impact by reclaiming memory in the background [9][10][11].
Citations:
- 1: https://redis.io/docs/latest/develop/using-commands/keyspace/
- 2: https://redis.io/docs/latest/commands/keys/
- 3: https://redis.io/tutorials/redis-anti-patterns-every-developer-should-avoid/
- 4: https://medium.com/@ardipurba/do-not-use-keys-on-building-app-using-redis-fd7c74c2e8b3
- 5: https://oneuptime.com/blog/post/2026-03-31-redis-keys-scan-find-by-pattern/view
- 6: https://osvaldo-gonzalez-venegas.medium.com/redis-keys-vs-scan-whats-the-real-difference-e0d72173221c
- 7: https://stackoverflow.com/questions/41542020/why-keys-is-advised-not-to-be-used-in-redis
- 8: https://oneuptime.com/blog/post/2026-03-31-redis-why-not-use-keys-command/view
- 9: https://support.redislabs.com/hc/en-us/articles/32321430231186-Massive-Key-Deletion-in-Redis-Without-Impacting-Performance
- 10: https://support.redislabs.com/hc/en-us/articles/28953806765458-Safe-Key-Deletion-Strategies-in-Large-Redis-Clusters
- 11: https://stackoverflow.com/questions/4006324/how-to-atomically-delete-keys-matching-a-pattern-using-redis
Replace KEYS-based invalidation with SCAN + batched deletion.
backend/src/services/cache.ts (clearCachePattern, lines 126-131) uses redisClient.keys(pattern) followed by del(keys). Redis treats KEYS as dangerous in production because it blocks the server while scanning the full keyspace, causing latency spikes/timeouts. Use cursor-based SCAN and delete matches in batches (prefer UNLINK, otherwise DEL) instead.
🤖 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/src/services/cache.ts` around lines 126 - 131, The clearCachePattern
implementation currently uses redisClient.keys(pattern) which blocks; replace it
with a cursor-based SCAN loop inside clearCachePattern that iterates until
cursor === '0', collects matching keys into a batch (e.g., 100-1000 keys), and
issues batched deletes using redisClient.unlink(...) when available or
redisClient.del(...) as a fallback; accumulate and return the total number of
deleted keys, and ensure awaits are used for SCAN and delete calls and errors
are handled/propagated.
| const pledgePromises = [ | ||
| addPledge(campaignId, { | ||
| contributor: CONTRIBUTOR_1, | ||
| amount: pledgeAmount, | ||
| assetCode: "USDC", | ||
| }), | ||
| addPledge(campaignId, { | ||
| contributor: CONTRIBUTOR_2, | ||
| amount: pledgeAmount, | ||
| assetCode: "USDC", | ||
| }), | ||
| addPledge(campaignId, { | ||
| contributor: CONTRIBUTOR_3, | ||
| amount: pledgeAmount, | ||
| assetCode: "USDC", | ||
| }), | ||
| addPledge(campaignId, { | ||
| contributor: CREATOR, | ||
| amount: pledgeAmount, | ||
| assetCode: "USDC", | ||
| }), | ||
| ]; | ||
|
|
||
| // Execute all pledges concurrently | ||
| const results = await Promise.all(pledgePromises); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== campaignStore function definitions ==="
rg -n -C3 'export\s+(async\s+)?function\s+(addPledge|claimCampaign)\s*\(' backend/src/services/campaignStore.ts
echo
echo "=== db implementation / driver ==="
rg -n -C3 'better-sqlite3|new Database|Database\(' backend/src/services/db.ts backend/package.jsonRepository: ritik4ever/stellar-goal-vault
Length of output: 2171
Fix the concurrency test harness: Promise.all won’t race if addPledge/claimCampaign are synchronous.
backend/src/services/campaignStore.ts exports addPledge and claimCampaign as synchronous functions, so the pledgePromises array literal in campaignStore.concurrent.test.ts executes those calls eagerly—before Promise.all has any effect—meaning this test can pass without exercising concurrent writes. Update the harness to create a real concurrency boundary (e.g., concurrent requests / separate connections / truly async code paths) to reproduce the race reliably.
Also applies to: 98-116, 141-154, 178-195, 234-243, 268-276
🤖 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/src/services/campaignStore.concurrent.test.ts` around lines 45 - 69,
The test builds pledgePromises by calling addPledge/claimCampaign synchronously,
so Promise.all does not create a race; change the harness to defer invocation
and run the functions concurrently (e.g., build an array of thunked async tasks
or wrappers that return Promises and only call addPledge/claimCampaign inside
those async functions) so Promise.all actually runs them in parallel;
alternatively open separate DB connections or make addPledge/claimCampaign real
async functions and invoke them from concurrently-started async wrappers so
functions like addPledge and claimCampaign are executed only when Promise.all
starts them, ensuring a true concurrency boundary for the race tests.
| // All pledges should succeed (no hard cap on total) | ||
| // but campaign should not exceed target in practice | ||
| expect(results).toHaveLength(3); | ||
|
|
||
| const campaign = getCampaign(campaignId); | ||
| expect(campaign).toBeDefined(); | ||
| // Total pledged should be 900 (no hard cap enforced) | ||
| expect(campaign?.pledgedAmount).toBe(900); |
There was a problem hiding this comment.
These assertions codify the bug instead of the invariant.
The over-target case expects 900 on a 500 target, the per-contributor case expects 300 with a 200 cap, and the duplicate case expects every retry to be counted. That makes the tests pass when the protections this PR is supposed to validate are broken. Assert the safety rule instead: excess/duplicate writes should be rejected or ignored, and the final pledgedAmount should stay within the allowed bounds.
Also applies to: 156-163, 278-284
🤖 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/src/services/campaignStore.concurrent.test.ts` around lines 118 -
125, The assertions currently assert failing behavior (allowing
over-target/duplicate pledges) instead of the safety invariant; update the tests
in campaignStore.concurrent.test.ts so they assert that excess or duplicate
pledge attempts are rejected or ignored and that final state remains within
limits: replace checks like expect(results).toHaveLength(3) /
expect(campaign?.pledgedAmount).toBe(900) with assertions that failed/duplicate
pledge attempts are reported (e.g., inspect the results array for errors or
rejected flags) and that getCampaign(campaignId).pledgedAmount is <= target (and
per-contributor pledged amount <= per-contributor cap); apply the same change
pattern to the other two test blocks mentioned (around the other ranges) so
tests validate the protection logic instead of codifying the broken behavior.
2 participants
d API key authentication middleware for production deployments
Protects write operations and sensitive endpoints
Public endpoints (health, config, stats, leaderboard) remain accessible
Configurable via API_KEYS environment variable
Only enabled in production mode
Implement Redis cache layer for API responses
Add comprehensive concurrent pledge race condition tests
Update dependencies
Update configuration
Add comprehensive documentation
closes Add Compact Campaign Card Toggle #81
closes Add request authentication middleware using API keys #209
closes Add Redis cache layer for backend API responses in production #330
closes Add backend test for concurrent pledge race condition #314
Summary by CodeRabbit
New Features
Documentation
Tests