(SP: 3) [Security] Harden admin CSRF gate + add DB-backed rate limiting for checkout and Stripe webhooks#154
Conversation
… product form; split Security Gates follow-ups (rate limit, CORS/origin)
…pe webhooks (policy, migration, contracts)
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. |
📝 WalkthroughWalkthroughThis PR implements CSRF protection for admin product/order operations and adds DB-backed rate limiting for checkout and Stripe webhook processing. It introduces rate limit configuration environment variables, a database table for tracking request counts, server-side utilities for CSRF validation and rate enforcement, and propagates CSRF tokens through admin UI components and API routes. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant AdminPage as Admin Page
participant ProductForm
participant API as Admin API Route
participant CSRF as CSRF Validator
participant DB as Database
Client->>AdminPage: View product edit form
AdminPage->>AdminPage: Call issueCsrfToken('admin:products:update')
AdminPage->>ProductForm: Render with csrfToken prop
Client->>ProductForm: Submit form with changes
ProductForm->>ProductForm: Validate csrfToken present
ProductForm->>API: PATCH with x-csrf-token header
API->>CSRF: requireAdminCsrf(request, 'admin:products:update', formData)
CSRF->>CSRF: Extract token from header or form field
alt Token Missing
CSRF->>API: Return 403 {code: CSRF_MISSING}
API->>Client: 403 CSRF_MISSING
Client->>ProductForm: Display error: "Security token expired"
else Token Invalid
CSRF->>DB: Call verifyCsrfToken(token, purpose)
DB->>CSRF: Verification failed
CSRF->>API: Return 403 {code: CSRF_INVALID}
API->>Client: 403 CSRF_INVALID
Client->>ProductForm: Display error: "Security token expired"
else Token Valid
CSRF->>API: Return null (continue processing)
API->>DB: Update product
DB->>API: Success
API->>Client: 200 {product}
Client->>ProductForm: Display success
end
sequenceDiagram
participant Client
participant CheckoutAPI as Checkout API Route
participant RateLimit as Rate Limiter
participant DB as Database
participant Response
Client->>CheckoutAPI: POST /api/shop/checkout (user_id or IP: 192.168.1.1)
CheckoutAPI->>CheckoutAPI: Extract subject (user_id or client IP)
CheckoutAPI->>CheckoutAPI: Compute key = "checkout:192.168.1.1"
CheckoutAPI->>RateLimit: enforceRateLimit({key, limit: 10, windowSeconds: 300})
RateLimit->>DB: Upsert api_rate_limits (atomic)
alt Window Expired
DB->>DB: Reset count to 1, window_started_at = now()
DB->>RateLimit: {ok: true, remaining: 9}
else Within Window and Under Limit
DB->>DB: Increment count, count = 5
DB->>RateLimit: {ok: true, remaining: 5}
else Limit Exceeded
DB->>DB: count = 11 (already exceeded)
DB->>RateLimit: {ok: false, retryAfterSeconds: 250}
end
alt Rate Limit Exceeded
RateLimit->>Response: 429 {code: 'RATE_LIMIT', retryAfterSeconds: 250}
Response->>Client: 429 Too Many Requests
else Rate Limit OK
RateLimit->>CheckoutAPI: {ok: true}
CheckoutAPI->>CheckoutAPI: Process checkout logic
CheckoutAPI->>Response: 200 or error response
Response->>Client: Response
end
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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d31920853c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| await requireAdminApi(request); | ||
| const csrfRes = requireAdminCsrf(request, 'admin:orders:refund'); | ||
| if (csrfRes) return csrfRes; |
There was a problem hiding this comment.
The refund endpoint now hard-requires requireAdminCsrf, but the only caller (frontend/app/[locale]/shop/admin/orders/[id]/RefundButton.tsx) still posts without x-csrf-token or csrfToken form field. That means every admin refund will now return 403 CSRF_MISSING even for valid admins, effectively breaking refunds in the UI. Either pass an issued token into RefundButton and set the header, or allow this specific endpoint to accept the existing request format.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@frontend/app/api/shop/checkout/route.ts`:
- Around line 234-251: The env parsing for CHECKOUT_RATE_LIMIT_MAX and
CHECKOUT_RATE_LIMIT_WINDOW_SECONDS can produce NaN/0; update the code that
builds the enforceRateLimit call (the variables passed for limit and
windowSeconds) to parse and validate the env values (use parseInt/Number and
check Number.isFinite and >0), and fall back to sane defaults (e.g., 10 and 300)
when the env is missing, non-numeric, or <=0; keep the checkoutSubject logic
as-is and pass the validated numeric values into enforceRateLimit.
In `@frontend/app/api/shop/webhooks/stripe/route.ts`:
- Around line 323-326: The environment-driven numeric settings
(STRIPE_WEBHOOK_INVALID_SIG_RL_MAX and
STRIPE_WEBHOOK_INVALID_SIG_RL_WINDOW_SECONDS) are converted with Number(...)
which can yield NaN; update the code that builds the rate-limit config (the
object with properties limit and windowSeconds) to robustly parse those env
values using a safe integer parser and fallback to the default (e.g., use
parseInt and Number.isFinite or Number.isInteger checks, or coerce with
Math.floor and isFinite) so if the env is invalid it falls back to 30 and 60
respectively; apply the same defensive parsing to the analogous block that sets
those env-driven limits later in the file (the second occurrence around the
STRIPE_WEBHOOK_INVALID_SIG_* variables).
- Around line 320-322: The code uses getClientIp() (used when building the
rate-limit key for enforceRateLimit and the `stripe_webhook:missing_sig:${ip}`
logic) which falls back to spoofable headers; ensure the webhook only trusts
proxy-provided addresses by adding a deployment-configured trusted-proxy/headers
check (e.g., require Cloudflare/CF-Connecting-IP or a configured TRUSTED_PROXY
flag) or reject requests that only provide x-forwarded-for; update getClientIp
usage (and the related logic around enforceRateLimit and the other occurrence at
lines ~356-358) to consult that trusted-proxy config before accepting
X-Forwarded-For, and document the requirement (“must be deployed behind
Cloudflare or reverse proxy that overwrites X-Forwarded-For”) in deployment
docs.
In `@frontend/lib/security/rate-limit.ts`:
- Around line 25-40: getClientIp currently trusts x-real-ip and x-forwarded-for
which can be spoofed; modify getClientIp to only use those headers when a
configured trust mechanism is enabled: add an environment-driven flag (e.g.
TRUST_FORWARDED_HEADERS, default false) and/or an allowlist of proxy IP ranges,
then change getClientIp to return cf-connecting-ip if present, and only parse
x-real-ip/x-forwarded-for when TRUST_FORWARDED_HEADERS is true or the request
originates from a known proxy IP; update references to the getClientIp function
to rely on the new behavior.
In `@frontend/lib/tests/admin-csrf-contract.test.ts`:
- Around line 28-42: The test sets process.env.CSRF_SECRET directly which can
leak into other tests; capture the original value of process.env.CSRF_SECRET
before mutating it and restore it after the test (either in an afterEach hook or
a finally block around the test) so subsequent tests are unaffected; locate the
mutation in admin-csrf-contract.test.ts where process.env.CSRF_SECRET =
'test_csrf_secret' and ensure restoration of the original value around the call
to patchStatus and assertions.
🧹 Nitpick comments (1)
frontend/lib/tests/checkout-no-payments.test.ts (1)
159-191: Consider reusing a shared test IP helper to avoid drift.
deriveTestIpFromIdemKeynow exists in multiple test helpers; exporting it from a single helper would keep rate‑limit test IP generation consistent.
| // P1: rate limit checkout (cross-instance, DB-backed) | ||
| // Policy: allow reasonable retries; block abusive burst. | ||
| const checkoutSubject = sessionUserId ?? getClientIp(request) ?? 'anon'; | ||
|
|
||
| const decision = await enforceRateLimit({ | ||
| key: `checkout:${checkoutSubject}`, | ||
| limit: Number(process.env.CHECKOUT_RATE_LIMIT_MAX ?? 10), | ||
| windowSeconds: Number( | ||
| process.env.CHECKOUT_RATE_LIMIT_WINDOW_SECONDS ?? 300 | ||
| ), | ||
| }); | ||
|
|
||
| if (!decision.ok) { | ||
| return rateLimitResponse({ | ||
| retryAfterSeconds: decision.retryAfterSeconds, | ||
| details: { scope: 'checkout' }, | ||
| }); | ||
| } |
There was a problem hiding this comment.
Guard rate-limit env parsing to avoid NaN/zero defaults.
If CHECKOUT_RATE_LIMIT_MAX or CHECKOUT_RATE_LIMIT_WINDOW_SECONDS is unset or non-numeric, Number(...) can yield NaN or 0, which may block all requests or disable limits unexpectedly (Line 240–243). Consider validating and falling back to sane defaults.
🔧 Suggested fix
- const decision = await enforceRateLimit({
- key: `checkout:${checkoutSubject}`,
- limit: Number(process.env.CHECKOUT_RATE_LIMIT_MAX ?? 10),
- windowSeconds: Number(
- process.env.CHECKOUT_RATE_LIMIT_WINDOW_SECONDS ?? 300
- ),
- });
+ const limitRaw = Number(process.env.CHECKOUT_RATE_LIMIT_MAX);
+ const windowRaw = Number(process.env.CHECKOUT_RATE_LIMIT_WINDOW_SECONDS);
+ const limit = Number.isFinite(limitRaw) && limitRaw > 0 ? limitRaw : 10;
+ const windowSeconds =
+ Number.isFinite(windowRaw) && windowRaw > 0 ? windowRaw : 300;
+
+ const decision = await enforceRateLimit({
+ key: `checkout:${checkoutSubject}`,
+ limit,
+ windowSeconds,
+ });📝 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.
| // P1: rate limit checkout (cross-instance, DB-backed) | |
| // Policy: allow reasonable retries; block abusive burst. | |
| const checkoutSubject = sessionUserId ?? getClientIp(request) ?? 'anon'; | |
| const decision = await enforceRateLimit({ | |
| key: `checkout:${checkoutSubject}`, | |
| limit: Number(process.env.CHECKOUT_RATE_LIMIT_MAX ?? 10), | |
| windowSeconds: Number( | |
| process.env.CHECKOUT_RATE_LIMIT_WINDOW_SECONDS ?? 300 | |
| ), | |
| }); | |
| if (!decision.ok) { | |
| return rateLimitResponse({ | |
| retryAfterSeconds: decision.retryAfterSeconds, | |
| details: { scope: 'checkout' }, | |
| }); | |
| } | |
| // P1: rate limit checkout (cross-instance, DB-backed) | |
| // Policy: allow reasonable retries; block abusive burst. | |
| const checkoutSubject = sessionUserId ?? getClientIp(request) ?? 'anon'; | |
| const limitRaw = Number(process.env.CHECKOUT_RATE_LIMIT_MAX); | |
| const windowRaw = Number(process.env.CHECKOUT_RATE_LIMIT_WINDOW_SECONDS); | |
| const limit = Number.isFinite(limitRaw) && limitRaw > 0 ? limitRaw : 10; | |
| const windowSeconds = | |
| Number.isFinite(windowRaw) && windowRaw > 0 ? windowRaw : 300; | |
| const decision = await enforceRateLimit({ | |
| key: `checkout:${checkoutSubject}`, | |
| limit, | |
| windowSeconds, | |
| }); | |
| if (!decision.ok) { | |
| return rateLimitResponse({ | |
| retryAfterSeconds: decision.retryAfterSeconds, | |
| details: { scope: 'checkout' }, | |
| }); | |
| } |
🤖 Prompt for AI Agents
In `@frontend/app/api/shop/checkout/route.ts` around lines 234 - 251, The env
parsing for CHECKOUT_RATE_LIMIT_MAX and CHECKOUT_RATE_LIMIT_WINDOW_SECONDS can
produce NaN/0; update the code that builds the enforceRateLimit call (the
variables passed for limit and windowSeconds) to parse and validate the env
values (use parseInt/Number and check Number.isFinite and >0), and fall back to
sane defaults (e.g., 10 and 300) when the env is missing, non-numeric, or <=0;
keep the checkoutSubject logic as-is and pass the validated numeric values into
enforceRateLimit.
| const ip = getClientIp(request) ?? 'anon'; | ||
| const decision = await enforceRateLimit({ | ||
| key: `stripe_webhook:missing_sig:${ip}`, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find the implementation of getClientIp
find . -type f -name "*.ts" -o -name "*.tsx" -o -name "*.js" | head -20Repository: DevLoversTeam/devlovers.net
Length of output: 934
🏁 Script executed:
# Search for getClientIp implementation
rg "function getClientIp|const getClientIp|export.*getClientIp" --type ts --type tsx --type jsRepository: DevLoversTeam/devlovers.net
Length of output: 98
🏁 Script executed:
# Look at the Stripe webhook route file
wc -l frontend/app/api/shop/webhooks/stripe/route.tsRepository: DevLoversTeam/devlovers.net
Length of output: 122
🏁 Script executed:
# Read the specific lines mentioned in the review
sed -n '310,330p' frontend/app/api/shop/webhooks/stripe/route.tsRepository: DevLoversTeam/devlovers.net
Length of output: 746
🏁 Script executed:
# Also check lines 356-358
sed -n '350,365p' frontend/app/api/shop/webhooks/stripe/route.tsRepository: DevLoversTeam/devlovers.net
Length of output: 551
🏁 Script executed:
# Search for where enforceRateLimit and getClientIp are imported from
head -50 frontend/app/api/shop/webhooks/stripe/route.tsRepository: DevLoversTeam/devlovers.net
Length of output: 1748
🏁 Script executed:
# Find the rate-limit utility file
find . -path "*/lib/security/rate-limit*" -type fRepository: DevLoversTeam/devlovers.net
Length of output: 108
🏁 Script executed:
# Search for the rate-limit file more broadly
fd "rate-limit" --type fRepository: DevLoversTeam/devlovers.net
Length of output: 106
🏁 Script executed:
# Search for the lib/security directory
ls -la frontend/lib/security/ 2>/dev/null || echo "Path not found, searching..."Repository: DevLoversTeam/devlovers.net
Length of output: 370
🏁 Script executed:
# Search more broadly for getClientIp implementation
rg "getClientIp" --type ts --type js -A 5 -B 2Repository: DevLoversTeam/devlovers.net
Length of output: 3896
🏁 Script executed:
# Read the full rate-limit.ts file to see the complete getClientIp implementation
cat -n frontend/lib/security/rate-limit.tsRepository: DevLoversTeam/devlovers.net
Length of output: 5200
IP-spoofing risk requires deployment boundary verification.
getClientIp checks cf-connecting-ip first (good), but falls back to x-forwarded-for which clients can spoof if not behind a trusted proxy. If your deployment is behind Cloudflare or a reverse proxy that overwrites these headers, this is safe. If the webhook endpoint is exposed directly to untrusted clients relying on x-forwarded-for, rate limits can be bypassed by rotating the header.
Action: Document your deployment boundary (e.g., "deployed behind Cloudflare" or "reverse proxy at X-Real-IP-trusted boundary") or add explicit configuration to specify trusted proxy headers. Consider adding IP validation or rejecting requests from untrusted sources.
Also applies to: 356-358
🤖 Prompt for AI Agents
In `@frontend/app/api/shop/webhooks/stripe/route.ts` around lines 320 - 322, The
code uses getClientIp() (used when building the rate-limit key for
enforceRateLimit and the `stripe_webhook:missing_sig:${ip}` logic) which falls
back to spoofable headers; ensure the webhook only trusts proxy-provided
addresses by adding a deployment-configured trusted-proxy/headers check (e.g.,
require Cloudflare/CF-Connecting-IP or a configured TRUSTED_PROXY flag) or
reject requests that only provide x-forwarded-for; update getClientIp usage (and
the related logic around enforceRateLimit and the other occurrence at lines
~356-358) to consult that trusted-proxy config before accepting X-Forwarded-For,
and document the requirement (“must be deployed behind Cloudflare or reverse
proxy that overwrites X-Forwarded-For”) in deployment docs.
| limit: Number(process.env.STRIPE_WEBHOOK_INVALID_SIG_RL_MAX ?? 30), | ||
| windowSeconds: Number( | ||
| process.env.STRIPE_WEBHOOK_INVALID_SIG_RL_WINDOW_SECONDS ?? 60 | ||
| ), |
There was a problem hiding this comment.
Guard against NaN in env‑driven limits.
Number(process.env...) can produce NaN (e.g., misconfigured env), which then propagates into SQL and can error or effectively disable limits. Prefer a safe integer parser with a fallback.
🛠️ Proposed fix
+function readIntEnv(name: string, fallback: number) {
+ const raw = process.env[name];
+ const parsed = Number.parseInt(raw ?? '', 10);
+ return Number.isFinite(parsed) ? parsed : fallback;
+}
@@
- const decision = await enforceRateLimit({
- key: `stripe_webhook:missing_sig:${ip}`,
- limit: Number(process.env.STRIPE_WEBHOOK_INVALID_SIG_RL_MAX ?? 30),
- windowSeconds: Number(
- process.env.STRIPE_WEBHOOK_INVALID_SIG_RL_WINDOW_SECONDS ?? 60
- ),
- });
+ const decision = await enforceRateLimit({
+ key: `stripe_webhook:missing_sig:${ip}`,
+ limit: readIntEnv('STRIPE_WEBHOOK_INVALID_SIG_RL_MAX', 30),
+ windowSeconds: readIntEnv(
+ 'STRIPE_WEBHOOK_INVALID_SIG_RL_WINDOW_SECONDS',
+ 60
+ ),
+ });
@@
- const decision = await enforceRateLimit({
- key: `stripe_webhook:invalid_sig:${ip}`,
- limit: Number(process.env.STRIPE_WEBHOOK_INVALID_SIG_RL_MAX ?? 30),
- windowSeconds: Number(
- process.env.STRIPE_WEBHOOK_INVALID_SIG_RL_WINDOW_SECONDS ?? 60
- ),
- });
+ const decision = await enforceRateLimit({
+ key: `stripe_webhook:invalid_sig:${ip}`,
+ limit: readIntEnv('STRIPE_WEBHOOK_INVALID_SIG_RL_MAX', 30),
+ windowSeconds: readIntEnv(
+ 'STRIPE_WEBHOOK_INVALID_SIG_RL_WINDOW_SECONDS',
+ 60
+ ),
+ });Also applies to: 359-362
🤖 Prompt for AI Agents
In `@frontend/app/api/shop/webhooks/stripe/route.ts` around lines 323 - 326, The
environment-driven numeric settings (STRIPE_WEBHOOK_INVALID_SIG_RL_MAX and
STRIPE_WEBHOOK_INVALID_SIG_RL_WINDOW_SECONDS) are converted with Number(...)
which can yield NaN; update the code that builds the rate-limit config (the
object with properties limit and windowSeconds) to robustly parse those env
values using a safe integer parser and fallback to the default (e.g., use
parseInt and Number.isFinite or Number.isInteger checks, or coerce with
Math.floor and isFinite) so if the env is invalid it falls back to 30 and 60
respectively; apply the same defensive parsing to the analogous block that sets
those env-driven limits later in the file (the second occurrence around the
STRIPE_WEBHOOK_INVALID_SIG_* variables).
| export function getClientIp(request: NextRequest): string | null { | ||
| const h = request.headers; | ||
|
|
||
| const cf = (h.get('cf-connecting-ip') ?? '').trim(); | ||
| if (cf) return cf; | ||
|
|
||
| const xr = (h.get('x-real-ip') ?? '').trim(); | ||
| if (xr) return xr; | ||
|
|
||
| const xff = (h.get('x-forwarded-for') ?? '').trim(); | ||
| if (xff) { | ||
| const first = xff.split(',')[0]?.trim(); | ||
| return first?.length ? first : null; | ||
| } | ||
|
|
||
| return null; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and read the file
find . -name "rate-limit.ts" -type fRepository: DevLoversTeam/devlovers.net
Length of output: 108
🏁 Script executed:
# Check the file content around lines 25-40
cat -n frontend/lib/security/rate-limit.ts | head -50Repository: DevLoversTeam/devlovers.net
Length of output: 1904
🏁 Script executed:
# Search for how NextRequest is imported and used
rg "NextRequest" frontend/lib/security/rate-limit.ts -A 3 -B 3Repository: DevLoversTeam/devlovers.net
Length of output: 435
🏁 Script executed:
# Check for TRUST_PROXY or proxy-related configurations
rg "TRUST_PROXY|trust.?proxy|proxy" --type ts --type js frontend/ -iRepository: DevLoversTeam/devlovers.net
Length of output: 130
🏁 Script executed:
# Look for how getClientIp is used in rate limiting
rg "getClientIp" --type ts --type js -A 5 -B 2Repository: DevLoversTeam/devlovers.net
Length of output: 3896
🏁 Script executed:
# Check the proxy.ts file to understand deployment setup
cat -n frontend/proxy.tsRepository: DevLoversTeam/devlovers.net
Length of output: 3475
🏁 Script executed:
# Search for Next.js version and configuration
cat -n next.config.ts next.config.js | head -50Repository: DevLoversTeam/devlovers.net
Length of output: 164
🏁 Script executed:
# Check for deployment/middleware configuration
find . -name "middleware.ts" -o -name "vercel.json" -o -name ".env*" | head -20Repository: DevLoversTeam/devlovers.net
Length of output: 94
🏁 Script executed:
# Search for any documentation or comments about IP handling
rg "ip|IP|forwarded|proxy" frontend/lib/security/ -i -B 2 -A 2Repository: DevLoversTeam/devlovers.net
Length of output: 1263
🏁 Script executed:
# Check if there's any request IP handling in middleware or auth
cat -n frontend/middleware.ts 2>/dev/null || echo "No middleware.ts found"Repository: DevLoversTeam/devlovers.net
Length of output: 93
🏁 Script executed:
# Check .env.example for any proxy or Cloudflare config hints
cat -n frontend/.env.exampleRepository: DevLoversTeam/devlovers.net
Length of output: 2139
🏁 Script executed:
# Check for vercel.json or any deployment config
find . -name "vercel.json" -o -name "netlify.toml" -o -name "dockerfile*" 2>/dev/nullRepository: DevLoversTeam/devlovers.net
Length of output: 85
🏁 Script executed:
# Search for any Cloudflare-specific configuration or comments
rg "cloudflare|cf-" --type ts --type js -iRepository: DevLoversTeam/devlovers.net
Length of output: 161
🏁 Script executed:
# Check if there's any documentation about deployment setup
find . -name "README*" -o -name "DEPLOY*" -o -name "docs" -type d 2>/dev/null | head -5Repository: DevLoversTeam/devlovers.net
Length of output: 149
🏁 Script executed:
# Check Netlify deployment config
cat -n ./netlify.tomlRepository: DevLoversTeam/devlovers.net
Length of output: 218
🏁 Script executed:
# Check README for deployment context
cat -n ./README.md | head -100Repository: DevLoversTeam/devlovers.net
Length of output: 725
🏁 Script executed:
# Look for any configuration about trusted proxies or environment-specific settings
rg "netlify|cloudflare|TRUST|proxy" frontend/ -i -lRepository: DevLoversTeam/devlovers.net
Length of output: 293
🏁 Script executed:
# Check Next.js version to understand IP handling capabilities
cat -n frontend/package.json | grep -A 2 -B 2 "next"Repository: DevLoversTeam/devlovers.net
Length of output: 900
🏁 Script executed:
# Search for any documentation or comments about IP extraction in the codebase
rg "\.ip|request\.ip" frontend/ -B 3 -A 3Repository: DevLoversTeam/devlovers.net
Length of output: 53
🏁 Script executed:
# Verify the actual HTTP flow: check if there's any middleware that might already extract or validate IP
rg "request.headers|NextRequest" frontend/app/api -A 5 | head -100Repository: DevLoversTeam/devlovers.net
Length of output: 7449
🏁 Script executed:
# Search for Netlify documentation or discussions about request.ip and header forwarding
rg "netlify|x-forwarded-for|x-real-ip" frontend/ -lRepository: DevLoversTeam/devlovers.net
Length of output: 249
🏁 Script executed:
# Check if there's any error handling or logging around IP extraction
rg "getClientIp" frontend/ -B 5 -A 10Repository: DevLoversTeam/devlovers.net
Length of output: 7166
🏁 Script executed:
# Verify the actual checkout endpoint to understand the risk impact
cat -n frontend/app/api/shop/checkout/route.ts | head -100Repository: DevLoversTeam/devlovers.net
Length of output: 3501
🏁 Script executed:
# Check if there are any comments or documentation about Cloudflare being mandatory
rg "cloudflare|cf-connecting" frontend/ README.md -i -B 3 -A 3Repository: DevLoversTeam/devlovers.net
Length of output: 2334
🏁 Script executed:
# Verify the exact Netlify setup and whether direct origin access is possible
cat -n ./netlify.tomlRepository: DevLoversTeam/devlovers.net
Length of output: 218
Validate trust boundary for forwarded IP headers to prevent spoofing.
The x-real-ip and x-forwarded-for fallbacks can be spoofed by direct clients to bypass rate limits on checkout and webhook endpoints. While cf-connecting-ip (Cloudflare) is trustworthy and checked first, the subsequent fallbacks lack validation. On Netlify, these headers are not automatically stripped or sanitized.
Consider one of:
- Only trust
cf-connecting-ipwhen available — restrict fallbacks to Cloudflare-originated traffic by returningnullif neither Cloudflare nor authenticated context provides the IP. - Validate against known proxy IPs — if Netlify or another trusted proxy is the only legitimate source, maintain a allowlist of their IP ranges and reject other
x-forwarded-forclaims. - Add environment-based trust control — introduce a
TRUST_FORWARDED_HEADERSflag (default off) to skip spoofable headers unless explicitly enabled in trusted environments.
The rate limiting impact is moderate since unauthenticated checkout is already gated by signature/auth checks downstream, but this removes a potential abuse vector.
🤖 Prompt for AI Agents
In `@frontend/lib/security/rate-limit.ts` around lines 25 - 40, getClientIp
currently trusts x-real-ip and x-forwarded-for which can be spoofed; modify
getClientIp to only use those headers when a configured trust mechanism is
enabled: add an environment-driven flag (e.g. TRUST_FORWARDED_HEADERS, default
false) and/or an allowlist of proxy IP ranges, then change getClientIp to return
cf-connecting-ip if present, and only parse x-real-ip/x-forwarded-for when
TRUST_FORWARDED_HEADERS is true or the request originates from a known proxy IP;
update references to the getClientIp function to rely on the new behavior.
| process.env.CSRF_SECRET = 'test_csrf_secret'; | ||
|
|
||
| const req = new NextRequest( | ||
| new Request('http://localhost/api/shop/admin/products/x/status', { | ||
| method: 'PATCH', | ||
| }) | ||
| ); | ||
|
|
||
| const res = await patchStatus(req, { | ||
| params: Promise.resolve({ id: '11111111-1111-1111-1111-111111111111' }), | ||
| }); | ||
|
|
||
| expect(res.status).toBe(403); | ||
| const body = await res.json(); | ||
| expect(body.code).toBe('CSRF_MISSING'); |
There was a problem hiding this comment.
Restore CSRF_SECRET after the test to avoid cross-test leakage.
Line 28 mutates process.env without cleanup, which can taint other tests in the same worker.
🧹 Proposed fix to restore process.env.CSRF_SECRET
- process.env.CSRF_SECRET = 'test_csrf_secret';
-
- const req = new NextRequest(
- new Request('http://localhost/api/shop/admin/products/x/status', {
- method: 'PATCH',
- })
- );
-
- const res = await patchStatus(req, {
- params: Promise.resolve({ id: '11111111-1111-1111-1111-111111111111' }),
- });
-
- expect(res.status).toBe(403);
- const body = await res.json();
- expect(body.code).toBe('CSRF_MISSING');
+ const prevSecret = process.env.CSRF_SECRET;
+ process.env.CSRF_SECRET = 'test_csrf_secret';
+
+ try {
+ const req = new NextRequest(
+ new Request('http://localhost/api/shop/admin/products/x/status', {
+ method: 'PATCH',
+ })
+ );
+
+ const res = await patchStatus(req, {
+ params: Promise.resolve({ id: '11111111-1111-1111-1111-111111111111' }),
+ });
+
+ expect(res.status).toBe(403);
+ const body = await res.json();
+ expect(body.code).toBe('CSRF_MISSING');
+ } finally {
+ if (prevSecret === undefined) {
+ delete process.env.CSRF_SECRET;
+ } else {
+ process.env.CSRF_SECRET = prevSecret;
+ }
+ }📝 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.
| process.env.CSRF_SECRET = 'test_csrf_secret'; | |
| const req = new NextRequest( | |
| new Request('http://localhost/api/shop/admin/products/x/status', { | |
| method: 'PATCH', | |
| }) | |
| ); | |
| const res = await patchStatus(req, { | |
| params: Promise.resolve({ id: '11111111-1111-1111-1111-111111111111' }), | |
| }); | |
| expect(res.status).toBe(403); | |
| const body = await res.json(); | |
| expect(body.code).toBe('CSRF_MISSING'); | |
| const prevSecret = process.env.CSRF_SECRET; | |
| process.env.CSRF_SECRET = 'test_csrf_secret'; | |
| try { | |
| const req = new NextRequest( | |
| new Request('http://localhost/api/shop/admin/products/x/status', { | |
| method: 'PATCH', | |
| }) | |
| ); | |
| const res = await patchStatus(req, { | |
| params: Promise.resolve({ id: '11111111-1111-1111-1111-111111111111' }), | |
| }); | |
| expect(res.status).toBe(403); | |
| const body = await res.json(); | |
| expect(body.code).toBe('CSRF_MISSING'); | |
| } finally { | |
| if (prevSecret === undefined) { | |
| delete process.env.CSRF_SECRET; | |
| } else { | |
| process.env.CSRF_SECRET = prevSecret; | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@frontend/lib/tests/admin-csrf-contract.test.ts` around lines 28 - 42, The
test sets process.env.CSRF_SECRET directly which can leak into other tests;
capture the original value of process.env.CSRF_SECRET before mutating it and
restore it after the test (either in an afterEach hook or a finally block around
the test) so subsequent tests are unaffected; locate the mutation in
admin-csrf-contract.test.ts where process.env.CSRF_SECRET = 'test_csrf_secret'
and ensure restoration of the original value around the call to patchStatus and
assertions.
2 participants
-## Description
This PR hardens critical security gates in the shop module by tightening admin CSRF verification semantics and adding a durable, DB-backed rate limit layer for high-risk public endpoints (checkout and Stripe webhooks). The goal is to ensure fail-closed behavior and predictable abuse resistance without relying on in-memory limits.
Related Issue
Issue: #<issue_number>
Changes
api_rate_limitstable (durable counters with window tracking) for:/api/shop/checkout(abuse protection)/api/shop/webhooks/stripeinvalid/missing signature paths (brute-force noise control)Database Changes (if applicable)
How Has This Been Tested?
Commands / checks
npx drizzle-kit migratenpx vitest run .\lib\tests\admin-product-sale-contract.test.tsnpx vitest run .\lib\tests\admin-product-patch-price-config-error-contract.test.ts429withRetry-After429after threshold and writesapi_rate_limitsrowsScreenshots (if applicable)
N/A (backend/security changes)
Checklist
Before submitting
Reviewers
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.