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 5 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?
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe proxy normalizes and forwards client IP values through request headers. Generated VTEX system routes now use high priority, while configured extra paths retain default priority. 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
nicacioliveira
changed the title
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. |
Marking every generated route highPriority also promoted extraPathsToProxy, which is site configuration and routinely holds a `/*` fallback for pages the storefront does not implement. At rank 1003 that fallback outranked the storefront's own page routes and proxied the entire site to the platform. Caught on a preview environment: every path returned the legacy VTEX store instead of the deco storefront. Only PATHS_TO_PROXY is promoted now. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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="vtex/loaders/proxy.ts">
<violation number="1" location="vtex/loaders/proxy.ts:103">
P1: A configured duplicate of a VTEX system path loses that path’s priority, allowing the A/B catch-all to swallow checkout/login again. Filter paths already in `PATHS_TO_PROXY` from `extraPaths` so built-ins remain authoritative.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| // catch-alls (`/*`, `/section/*`) used as a fallback for pages the | ||
| // storefront does not implement. Promoting those would let the fallback | ||
| // outrank the storefront's own pages and swallow the entire site. | ||
| ...extraPaths.map((path) => routeFromPath(path)), |
There was a problem hiding this comment.
P1: A configured duplicate of a VTEX system path loses that path’s priority, allowing the A/B catch-all to swallow checkout/login again. Filter paths already in PATHS_TO_PROXY from extraPaths so built-ins remain authoritative.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At vtex/loaders/proxy.ts, line 103:
<comment>A configured duplicate of a VTEX system path loses that path’s priority, allowing the A/B catch-all to swallow checkout/login again. Filter paths already in `PATHS_TO_PROXY` from `extraPaths` so built-ins remain authoritative.</comment>
<file context>
@@ -81,19 +84,24 @@ const buildProxyRoutes = (
+ // catch-alls (`/*`, `/section/*`) used as a fallback for pages the
+ // storefront does not implement. Promoting those would let the fallback
+ // outrank the storefront's own pages and swallow the entire site.
+ ...extraPaths.map((path) => routeFromPath(path)),
+ ];
</file context>
| ...extraPaths.map((path) => routeFromPath(path)), | |
| ...extraPaths.filter((path) => !PATHS_TO_PROXY.includes(path)).map((path) => | |
| routeFromPath(path) | |
| ), |
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
Summary by CodeRabbit