(SP: 3)[Security] Harden rate-limit inputs, normalize webhook subjects, and isolate test env#157
Merged
Merged
Conversation
…nalytics-friendly keys
…IPv6-safe, UA fallback + tests)
… fix CSRF env leak; unify test IP helper
liudmylasovetovs
requested review from
AM1007 and
ViktorSvertoka
as code owners
✅ Deploy Preview for develop-devlovers ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Contributor
📝 WalkthroughWalkthroughIntroduces a subject-based rate limiting system for checkout and Stripe webhook endpoints with normalized subjects, configurable per-endpoint limits, forwarded header trust controls, and dynamic per-reason Stripe webhook rate limits via environment configuration with fallback chains. Changes
Sequence DiagramsequenceDiagram
participant Request
participant getRateLimitSubject
participant normalizeRateLimitSubject
participant enforceRateLimit
participant normalizeRateLimitKey
participant Database
Request->>getRateLimitSubject: extract subject from IP or UA fingerprint
getRateLimitSubject->>normalizeRateLimitSubject: derive stable subject (hash IPv6, sanitize input)
normalizeRateLimitSubject-->>getRateLimitSubject: normalized subject
getRateLimitSubject-->>Request: rate limit subject ready
Request->>enforceRateLimit: check/increment counter with subject as key
enforceRateLimit->>normalizeRateLimitKey: normalize legacy key format
normalizeRateLimitKey-->>enforceRateLimit: { legacyKey, normalizedKey }
alt Key normalization occurred
enforceRateLimit->>Database: migrate legacy key to normalized key (async)
Database-->>enforceRateLimit: key migration complete
end
enforceRateLimit-->>Request: rate limit decision (allow/deny)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@frontend/lib/security/rate-limit.ts`:
- Around line 110-129: The function getClientIpFromHeaders currently accepts the
cf-connecting-ip header unconditionally; add a new env flag
TRUST_CF_CONNECTING_IP (default false) and gate the cf header check behind it
similar to how TRUST_FORWARDED_HEADERS is used so that cf-connecting-ip is only
trusted when TRUST_CF_CONNECTING_IP is true; update any envBool usage to read
TRUST_CF_CONNECTING_IP, adjust the early-return logic accordingly, and add
documentation/example entry in .env.example describing the flag and its default.
🧹 Nitpick comments (1)
frontend/lib/tests/stripe-webhook-rate-limit-env.test.ts (1)
76-88: Consider adding edge case tests for zero and negative values.The current test validates whitespace and non-numeric values fall back correctly. You might also want to explicitly test that
"0"and"-1"are rejected (falling back to defaults), since these are valid integers but invalid for rate limiting.Optional test case addition
it('ignores empty/whitespace and non-numeric env values (falls back safely)', () => { vi.stubEnv('STRIPE_WEBHOOK_RL_MAX', ' '); vi.stubEnv('STRIPE_WEBHOOK_RL_WINDOW_SECONDS', 'nope'); expect(resolveStripeWebhookRateLimit('missing_sig')).toEqual({ max: 30, windowSeconds: 60, }); expect(resolveStripeWebhookRateLimit('invalid_sig')).toEqual({ max: 30, windowSeconds: 60, }); }); + + it('rejects zero and falls back to defaults', () => { + vi.stubEnv('STRIPE_WEBHOOK_RL_MAX', '0'); + vi.stubEnv('STRIPE_WEBHOOK_RL_WINDOW_SECONDS', '0'); + + expect(resolveStripeWebhookRateLimit('missing_sig')).toEqual({ + max: 30, + windowSeconds: 60, + }); + });
Comment on lines
+110
to
+129
| export function getClientIpFromHeaders(headers: Headers): string | null { | ||
| // Always allow Cloudflare canonical header (highest priority). | ||
| const cf = (headers.get('cf-connecting-ip') ?? '').trim(); | ||
| if (cf && isIP(cf)) return cf; | ||
|
|
||
| const xr = (h.get('x-real-ip') ?? '').trim(); | ||
| if (xr) return xr; | ||
| const trustForwarded = envBool('TRUST_FORWARDED_HEADERS', false); | ||
|
|
||
| const xff = (h.get('x-forwarded-for') ?? '').trim(); | ||
| // Trusted boundary: if we don't trust forwarded headers and CF is missing, | ||
| // do NOT fall back to spoofable headers. | ||
| if (!trustForwarded) return null; | ||
|
|
||
| const xr = (headers.get('x-real-ip') ?? '').trim(); | ||
| if (xr && isIP(xr)) return xr; | ||
|
|
||
| const xff = (headers.get('x-forwarded-for') ?? '').trim(); | ||
| if (xff) { | ||
| const first = xff.split(',')[0]?.trim(); | ||
| return first?.length ? first : null; | ||
| for (const part of xff.split(',')) { | ||
| const candidate = part.trim(); | ||
| if (candidate && isIP(candidate)) return candidate; | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's locate the file and understand the repository structure
find . -name "rate-limit.ts" -o -name ".env.example" -o -name ".env*" | head -20Repository: DevLoversTeam/devlovers.net
Length of output: 132
🏁 Script executed:
# Get an overview of the repo structure
git ls-files | head -30Repository: DevLoversTeam/devlovers.net
Length of output: 903
🏁 Script executed:
# Look at the full rate-limit.ts file to understand the context
wc -l frontend/lib/security/rate-limit.tsRepository: DevLoversTeam/devlovers.net
Length of output: 110
🏁 Script executed:
# Read the full rate-limit.ts file to understand context and usage
cat -n frontend/lib/security/rate-limit.tsRepository: DevLoversTeam/devlovers.net
Length of output: 10136
🏁 Script executed:
# Check .env.example to see what trust flags are documented
cat -n frontend/.env.exampleRepository: DevLoversTeam/devlovers.net
Length of output: 2977
🏁 Script executed:
# Search for usages of TRUST_FORWARDED_HEADERS in the codebase
rg "TRUST_FORWARDED_HEADERS" -A 3 -B 3Repository: DevLoversTeam/devlovers.net
Length of output: 5727
🏁 Script executed:
# Search for any deployment or infrastructure documentation
find . -type f \( -name "*.md" -o -name "*.txt" \) -exec grep -l "Cloudflare\|cloudflare\|cf-connecting-ip" {} \; 2>/dev/nullRepository: DevLoversTeam/devlovers.net
Length of output: 53
🏁 Script executed:
# Look at the test file to see expectations around cf-connecting-ip
cat -n frontend/lib/tests/rate-limit-subject.test.ts | head -80Repository: DevLoversTeam/devlovers.net
Length of output: 3003
🏁 Script executed:
# Search for any additional documentation about Cloudflare assumptions
find . -type f \( -name "*.md" -o -name "DEPLOYMENT*" -o -name "INFRASTRUCTURE*" \) -exec grep -l "Cloudflare\|cloudflare\|deployment\|infrastructure" {} \; 2>/dev/null | head -10Repository: DevLoversTeam/devlovers.net
Length of output: 91
🏁 Script executed:
# Check SECURITY.md or README for any deployment assumptions
cat frontend/README.md 2>/dev/null || echo "frontend/README.md not found"Repository: DevLoversTeam/devlovers.net
Length of output: 1522
🏁 Script executed:
# Let's check if there are any comments or documentation in the codebase about why cf-connecting-ip is trusted unconditionally
rg "cf-connecting-ip\|cf_connecting_ip\|Cloudflare.*canonical\|canonical.*Cloudflare" -B 5 -A 5Repository: DevLoversTeam/devlovers.net
Length of output: 53
🏁 Script executed:
# Check if there are any other rate limiting files or usage patterns
fd "rate" --type f | grep -E "\.(ts|js)$"Repository: DevLoversTeam/devlovers.net
Length of output: 428
🏁 Script executed:
# Look at how getClientIp is used in the actual application
rg "getClientIp\|getRateLimitSubject" --type ts -B 2 -A 2 | head -100Repository: DevLoversTeam/devlovers.net
Length of output: 53
🏁 Script executed:
# Try a broader search for how rate-limit functions are used
rg "getClientIp|getRateLimitSubject" -t ts | head -50Repository: DevLoversTeam/devlovers.net
Length of output: 2156
🏁 Script executed:
# Check stripe-webhook-rate-limit.ts to see if it has similar patterns
cat -n frontend/lib/security/stripe-webhook-rate-limit.ts | head -50Repository: DevLoversTeam/devlovers.net
Length of output: 1883
🏁 Script executed:
# Search for where getClientIpFromHeaders is imported/used
rg "import.*getClientIp|from.*rate-limit" -t tsRepository: DevLoversTeam/devlovers.net
Length of output: 595
🏁 Script executed:
# Let's check the checkout and stripe webhook routes to see how rate limiting is used in practice
cat -n frontend/app/api/shop/checkout/route.ts | grep -A 10 -B 10 "getRateLimitSubject\|rate.*limit" | head -40Repository: DevLoversTeam/devlovers.net
Length of output: 1531
🏁 Script executed:
# Check if there's any configuration or deployment info about Cloudflare integration
find . -name "*.config.*" -o -name "*.env*" -o -name "vercel.json" -o -name "next.config.*" | head -10Repository: DevLoversTeam/devlovers.net
Length of output: 289
🏁 Script executed:
# Let's verify the exact behavior: does cf-connecting-ip bypass trust checks?
cat -n frontend/lib/security/rate-limit.ts | sed -n '110,133p'Repository: DevLoversTeam/devlovers.net
Length of output: 1027
Gate cf-connecting-ip behind an explicit trust boundary.
If any deployment isn't guaranteed to sit behind Cloudflare (or doesn't sanitize this header at the edge), clients can spoof it and evade rate limiting. The current code unconditionally accepts this header for rate-limit subjects, while x-real-ip and x-forwarded-for are properly guarded by TRUST_FORWARDED_HEADERS. Add an explicit TRUST_CF_CONNECTING_IP flag (defaulting to false for safety) and document it in .env.example.
🔒 Suggested guard for trusted CF header
- // Always allow Cloudflare canonical header (highest priority).
- const cf = (headers.get('cf-connecting-ip') ?? '').trim();
- if (cf && isIP(cf)) return cf;
-
const trustForwarded = envBool('TRUST_FORWARDED_HEADERS', false);
+ const trustCf = envBool('TRUST_CF_CONNECTING_IP', false);
+
+ // Allow Cloudflare canonical header (highest priority) when trusted.
+ if (trustCf) {
+ const cf = (headers.get('cf-connecting-ip') ?? '').trim();
+ if (cf && isIP(cf)) return cf;
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| export function getClientIpFromHeaders(headers: Headers): string | null { | |
| // Always allow Cloudflare canonical header (highest priority). | |
| const cf = (headers.get('cf-connecting-ip') ?? '').trim(); | |
| if (cf && isIP(cf)) return cf; | |
| const xr = (h.get('x-real-ip') ?? '').trim(); | |
| if (xr) return xr; | |
| const trustForwarded = envBool('TRUST_FORWARDED_HEADERS', false); | |
| const xff = (h.get('x-forwarded-for') ?? '').trim(); | |
| // Trusted boundary: if we don't trust forwarded headers and CF is missing, | |
| // do NOT fall back to spoofable headers. | |
| if (!trustForwarded) return null; | |
| const xr = (headers.get('x-real-ip') ?? '').trim(); | |
| if (xr && isIP(xr)) return xr; | |
| const xff = (headers.get('x-forwarded-for') ?? '').trim(); | |
| if (xff) { | |
| const first = xff.split(',')[0]?.trim(); | |
| return first?.length ? first : null; | |
| for (const part of xff.split(',')) { | |
| const candidate = part.trim(); | |
| if (candidate && isIP(candidate)) return candidate; | |
| } | |
| export function getClientIpFromHeaders(headers: Headers): string | null { | |
| const trustForwarded = envBool('TRUST_FORWARDED_HEADERS', false); | |
| const trustCf = envBool('TRUST_CF_CONNECTING_IP', false); | |
| // Allow Cloudflare canonical header (highest priority) when trusted. | |
| if (trustCf) { | |
| const cf = (headers.get('cf-connecting-ip') ?? '').trim(); | |
| if (cf && isIP(cf)) return cf; | |
| } | |
| // Trusted boundary: if we don't trust forwarded headers and CF is missing, | |
| // do NOT fall back to spoofable headers. | |
| if (!trustForwarded) return null; | |
| const xr = (headers.get('x-real-ip') ?? '').trim(); | |
| if (xr && isIP(xr)) return xr; | |
| const xff = (headers.get('x-forwarded-for') ?? '').trim(); | |
| if (xff) { | |
| for (const part of xff.split(',')) { | |
| const candidate = part.trim(); | |
| if (candidate && isIP(candidate)) return candidate; | |
| } |
🤖 Prompt for AI Agents
In `@frontend/lib/security/rate-limit.ts` around lines 110 - 129, The function
getClientIpFromHeaders currently accepts the cf-connecting-ip header
unconditionally; add a new env flag TRUST_CF_CONNECTING_IP (default false) and
gate the cf header check behind it similar to how TRUST_FORWARDED_HEADERS is
used so that cf-connecting-ip is only trusted when TRUST_CF_CONNECTING_IP is
true; update any envBool usage to read TRUST_CF_CONNECTING_IP, adjust the
early-return logic accordingly, and add documentation/example entry in
.env.example describing the flag and its default.
ViktorSvertoka
approved these changes
Jan 18, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR hardens rate limiting and webhook abuse controls by (1) making env-driven limits resilient to invalid/empty values, (2) enforcing a trusted-boundary model for client IP extraction to prevent header spoofing, and (3) normalizing rate-limit subjects (including IPv6) to keep keys analytics/grep-friendly. It also improves test stability by isolating env mutations and deduplicating test helpers.
Related Issue
Issue: #<issue_number>
Changes
CHECKOUT_RATE_LIMIT_MAXandCHECKOUT_RATE_LIMIT_WINDOW_SECONDSas positive integers with safe fallbacks before callingenforceRateLimit(preventsNaN/0from disabling or corrupting RL).Number(process.env...)with a strict positive-int resolver; supportSTRIPE_WEBHOOK_MISSING_SIG_RL_*+ genericSTRIPE_WEBHOOK_RL_*, with legacySTRIPE_WEBHOOK_INVALID_SIG_RL_*preserved as fallback for backward compatibility.getClientIpFromHeadersalways preferscf-connecting-ip, and readsx-real-ip/x-forwarded-foronly whenTRUST_FORWARDED_HEADERS=true; IPv6 subjects are canonicalized (no:) via normalization/hashing. Added.env.exampleguidance forTRUST_FORWARDED_HEADERS.admin-csrf-contract.test.tsby saving/restoringprocess.env.CSRF_SECRETin atry/finally.deriveTestIpFromIdemKeyinto a shared helper and replace local copies with imports (prevents drift across test files).Database Changes (if applicable)
How Has This Been Tested?
Commands (PowerShell):
npx vitest run .\lib\tests\rate-limit-subject.test.tsnpx vitest run .\lib\tests\admin-csrf-contract.test.tsnpx vitest run .\lib\tests\checkout-no-payments.test.tsScreenshots (if applicable)
N/A (no UI changes)
Checklist
Before submitting
Reviewers
Summary by CodeRabbit
Release Notes
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.