fix: preserve client IP and VTEX route priority when proxying - #1655
fix: preserve client IP and VTEX route priority when proxying#1655nicacioliveira wants to merge 4 commits into
Conversation
removeCFHeaders drops every cf-* header, including cf-connecting-ip, so proxied origins saw only the pod's IP. Capture it before the strip and forward it as x-forwarded-for/x-real-ip. Affects every site using website/handlers/proxy.ts, including the VTEX proxy routes and A/B testing via the abTesting prop, where the origin otherwise loses geo, rate limiting, analytics and fraud signals. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Tagging OptionsShould a new tag be published when this PR is merged?
|
|
Warning Review limit reached
Next review available in: 37 seconds You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe proxy forwards the client IP from ChangesProxy client IP forwarding
VTEX route priority
Estimated code review effort: 2 (Simple) | ~15 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Measured on a live pod: x-forwarded-for already reaches the handler with the client IP as its first entry, so unconditionally prepending it produced a duplicate. Only seed the header when absent, and always set x-real-ip, which was the header actually missing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
nicacioliveira
changed the title
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review completed against the latest diff
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
There was a problem hiding this comment.
All reported issues were addressed across 1 file (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
Validated end to end on a live deco site ( Before — After — The client IP now reaches the origin via Site was restored to |
PATHS_TO_PROXY covers checkout, account, login, /api/*, /_v/*, /arquivos/* and friends, but the routes were registered without highPriority. Route rank is (highPriority ? 1000 : 0) + rankRoute(path), so an A/B audience registering `/*` with highPriority scores 1003 and outranks `/checkout` at 6 — the catch-all swallows every VTEX system path, in both arms. Concretely on a FastStore A/B: /checkout proxies to the FastStore, whose checkout route only does `window.location.href = checkoutUrl`, pointing back at the same origin. Infinite redirect. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@vtex/loaders/proxy.ts`:
- Around line 84-88: Update the route construction around routeFromPath so
highPriority: true applies only to entries from PATHS_TO_PROXY, not configured
extraPaths. Ensure extraPaths use normal priority unless they are explicitly
classified as VTEX system paths requiring the same bypass behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e0e77c17-a282-4d22-8474-90fc8ad914d9
📒 Files selected for processing (2)
vtex/loaders/proxy.tswebsite/handlers/proxy.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- website/handlers/proxy.ts
| // These are VTEX system paths — checkout, account, login, /api, /_v. | ||
| // Without the priority bump a catch-all `/*` route from an A/B test | ||
| // audience outranks them (1000 + rank("/*") = 1003 beats rank | ||
| // ("/checkout") = 6) and swallows the whole platform surface. | ||
| highPriority: true, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify how extraPaths are defined and whether callers expect normal priority.
rg -n -C 8 '\bextraPaths\b|\bPATHS_TO_PROXY\b|\bhighPriority\b' --glob '*.ts' .Repository: deco-cx/apps
Length of output: 24295
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'vtex proxy relevant section:\n'
sed -n '1,120p' vtex/loaders/proxy.ts | cat -n
printf '\nwebsite router relevant section:\n'
sed -n '1,180p' website/handlers/router.ts | cat -n
printf '\nwebsite audience/mod relevant section:\n'
sed -n '1,260p' website/mod.ts | cat -n
printf '\nreferences to extraPathsToProxy and extraPaths in vtex:\n'
rg -n -C 6 'extraPaths(ToProxy)?|PathsToProxy' vtex --glob '*.ts'Repository: deco-cx/apps
Length of output: 23663
Keep extraPaths out of the high-priority block.
routeFromPath is closed over highPriority: true and invoked for both PATHS_TO_PROXY and every configured extraPaths. Use normal priority for extraPaths, or explicitly document these routes as VTEX system paths that must bypass A/B test catch-all routes.
🤖 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 `@vtex/loaders/proxy.ts` around lines 84 - 88, Update the route construction
around routeFromPath so highPriority: true applies only to entries from
PATHS_TO_PROXY, not configured extraPaths. Ensure extraPaths use normal priority
unless they are explicitly classified as VTEX system paths requiring the same
bypass behavior.
The guard compared raw strings, so an IPv6 client whose casing differs between hops, or an x-forwarded-for entry carrying a port, would slip past it and get its IP prepended a second time. Compare canonical forms instead; the forwarded value is untouched. Also documents the trust boundary: x-forwarded-for is already forwarded untouched, so deriving x-real-ip from cf-connecting-ip adds no new spoofing surface. Authenticating the edge belongs at the ingress. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Addressed both cubic findings in P3 (dedup comparison) — valid, fixed. The guard compared raw strings, so IPv6 hex casing differing between hops, or an P2 (trust boundary) — real, but not introduced here. Validating against Cloudflare IP ranges is also not enforceable at this layer: the handler runs behind the service mesh, so the peer address it would check is an internal hop, not the edge. Authenticating the edge belongs at the ingress — that is CDN-to-origin auth (mTLS, a shared secret header, or an allowlist), not a header check inside a proxy loader. Documented the boundary in the code comment as suggested, rather than implementing a check that would look like a guarantee without being one. |
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="website/handlers/proxy.ts">
<violation number="1" location="website/handlers/proxy.ts:26">
P2: Equivalent IPv6 spellings can still be prepended as duplicate client entries because `normalizeIp` is not actually canonical for IPv6. Canonicalize parsed IPv6 (including mapped forms) before the deduplication comparison while preserving the original forwarded value.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| const normalizeIp = (value: string): string => { | ||
| const ip = value.trim().toLowerCase(); | ||
| const bracketed = ip.match(/^\[(.+)\](?::\d+)?$/); | ||
| if (bracketed) return bracketed[1]; | ||
| const ipv4WithPort = ip.match(/^([\d.]+):\d+$/); | ||
| if (ipv4WithPort) return ipv4WithPort[1]; | ||
| return ip; | ||
| }; |
There was a problem hiding this comment.
P2: Equivalent IPv6 spellings can still be prepended as duplicate client entries because normalizeIp is not actually canonical for IPv6. Canonicalize parsed IPv6 (including mapped forms) before the deduplication comparison while preserving the original forwarded value.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At website/handlers/proxy.ts, line 26:
<comment>Equivalent IPv6 spellings can still be prepended as duplicate client entries because `normalizeIp` is not actually canonical for IPv6. Canonicalize parsed IPv6 (including mapped forms) before the deduplication comparison while preserving the original forwarded value.</comment>
<file context>
@@ -17,6 +17,20 @@ const HOP_BY_HOP = [
+ * 1.2.3.4:56789) and IPv6 hex casing varies between hops; cf-connecting-ip
+ * is always a bare address.
+ */
+const normalizeIp = (value: string): string => {
+ const ip = value.trim().toLowerCase();
+ const bracketed = ip.match(/^\[(.+)\](?::\d+)?$/);
</file context>
| const normalizeIp = (value: string): string => { | |
| const ip = value.trim().toLowerCase(); | |
| const bracketed = ip.match(/^\[(.+)\](?::\d+)?$/); | |
| if (bracketed) return bracketed[1]; | |
| const ipv4WithPort = ip.match(/^([\d.]+):\d+$/); | |
| if (ipv4WithPort) return ipv4WithPort[1]; | |
| return ip; | |
| }; | |
| const normalizeIp = (value: string): string => { | |
| const ip = value.trim().toLowerCase(); | |
| const bracketed = ip.match(/^\[(.+)\](?::\d+)?$/); | |
| const host = bracketed?.[1] ?? | |
| ip.match(/^([\d.]+):\d+$/)?.[1] ?? | |
| ip; | |
| if (!host.includes(":")) return host; | |
| try { | |
| return new URL(`http://[${host}]`).hostname.slice(1, -1); | |
| } catch { | |
| return host; | |
| } | |
| }; |
|
Field note for anyone using this alongside an A/B test against a FastStore target: Reproduced live: with Not a defect in this PR: sending {
"pathTemplate": "/api/graphql",
"highPriority": true,
"handler": { "value": {
"__resolveType": "website/handlers/proxy.ts",
"url": "https://<faststore-host>"
}}
}Worth calling out in the A/B docs — without it the symptom is a working checkout and empty PLPs, which reads like a data problem rather than a routing one. |
1 participant
Two defects in the proxy path, both surfaced while wiring an A/B test between a deco storefront and a VTEX FastStore. Neither is specific to that store — they affect any deco site using
website/handlers/proxy.tsor the VTEX app.1.
x-real-ipnever reaches the originremoveCFHeadersdrops everycf-*header, includingcf-connecting-ip, and nothing replaces it.Measured on a live pod, proxying 100% of traffic to a header echo and toggling only the
apps/import:appsx-real-ipat the origin0.153.0187.61.224.250x-forwarded-forwas already fine — it is not inHOP_BY_HOPand arrives with the client IP first. Hence the second commit: the first version prepended unconditionally and would have produced a duplicate entry.2. A catch-all
/*outranks every VTEX system pathRoute rank is
(highPriority ? 1000 : 0) + rankRoute(path), androuteFromPathinvtex/loaders/proxy.tsregistered withouthighPriority:So an A/B audience swallows
/checkout,/account,/login,/api/*,/_v/*,/arquivos/*and the rest ofPATHS_TO_PROXY— in both arms, since routes are ranked before the matcher runs.The concrete failure:
/checkoutproxies to the FastStore, whose checkout route is onlywith
checkoutUrlhardcoded to the store origin. That redirects straight back into the catch-all. Infinite loop.Marking those paths
highPriorityputs them at1006, ahead of any catch-all.Notes
removeCFHeadersis left untouched; it is exported and used elsewhere (linx/utils/headers.ts).x-forwarded-for, so origins should read the first entry.deno fmtanddeno lintpass.deno checkreports 2 pre-existing errors inwebsite/utils/crypto.ts, identical on a cleanmain.🤖 Generated with Claude Code