fix(vtex): sanitize query params to prevent NaN crashes#1546
Open
fix(vtex): sanitize query params to prevent NaN crashes#1546
Conversation
Add middleware-level validation for page and PS params (301 redirect on invalid values like SSRF URLs), and defensive NaN guards in both IS and legacy PLP loaders for count, page, sort, simulationBehavior, and fq. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
Tagging OptionsShould a new tag be published when this PR is merged?
|
Contributor
📝 WalkthroughWalkthroughThis PR adds parameter validation and sanitization across the VTEX product listing infrastructure. URL query parameters (simulationBehavior, count, page, sort, and fq) are now validated against whitelists and constraints, with fallback defaults applied when invalid values are detected. The middleware performs early sanitization with 301 redirects for invalid numeric parameters. Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant Middleware
participant Loader
participant Backend
Client->>Middleware: Request with query params<br/>(page, PS, simulationBehavior, etc.)
alt Invalid numeric parameters detected
Middleware->>Middleware: Remove invalid params
Middleware->>Client: 301 Redirect to cleaned URL
Client->>Middleware: Request with cleaned params
end
Middleware->>Loader: Pass validated params
Loader->>Loader: Parse & validate each param<br/>(whitelist check, bounds check,<br/>apply fallbacks)
Loader->>Backend: Request with sanitized params
Backend->>Loader: Response
Loader->>Client: Response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 3 files
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/intelligentSearch/productListingPage.ts">
<violation number="1" location="vtex/loaders/intelligentSearch/productListingPage.ts:180">
P2: `page` validation still accepts fractional values. Restrict to integer pages to avoid propagating non-integer pagination state.</violation>
</file>
<file name="vtex/loaders/legacy/productListingPage.ts">
<violation number="1" location="vtex/loaders/legacy/productListingPage.ts:217">
P2: `page` validation still accepts fractional numbers, which can generate invalid pagination offsets (`_from`/`_to`). Restrict `page` to integers before computing offsets.</violation>
<violation number="2" location="vtex/loaders/legacy/productListingPage.ts:230">
P2: `fq` is sanitized for the backend request but not in `cacheKey`, causing cache-key fragmentation for equivalent sanitized queries.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Comment on lines
+180
to
+182
| const parsedPage = Number.isFinite(rawPage) && rawPage >= currentPageoffset | ||
| ? rawPage - currentPageoffset | ||
| : 0; |
There was a problem hiding this comment.
P2: page validation still accepts fractional values. Restrict to integer pages to avoid propagating non-integer pagination state.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At vtex/loaders/intelligentSearch/productListingPage.ts, line 180:
<comment>`page` validation still accepts fractional values. Restrict to integer pages to avoid propagating non-integer pagination state.</comment>
<file context>
@@ -161,20 +161,27 @@ export interface Props {
+ const rawPage = url.searchParams.get("page")
+ ? Number(url.searchParams.get("page"))
+ : NaN;
+ const parsedPage = Number.isFinite(rawPage) && rawPage >= currentPageoffset
+ ? rawPage - currentPageoffset
+ : 0;
</file context>
Suggested change
| const parsedPage = Number.isFinite(rawPage) && rawPage >= currentPageoffset | |
| ? rawPage - currentPageoffset | |
| : 0; | |
| const parsedPage = Number.isInteger(rawPage) && rawPage >= currentPageoffset | |
| ? rawPage - currentPageoffset | |
| : 0; |
| ...new Set([ | ||
| ...(props.fq ? [props.fq] : []), | ||
| ...url.searchParams.getAll("fq"), | ||
| ...url.searchParams.getAll("fq").map((v) => |
There was a problem hiding this comment.
P2: fq is sanitized for the backend request but not in cacheKey, causing cache-key fragmentation for equivalent sanitized queries.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At vtex/loaders/legacy/productListingPage.ts, line 230:
<comment>`fq` is sanitized for the backend request but not in `cacheKey`, causing cache-key fragmentation for equivalent sanitized queries.</comment>
<file context>
@@ -204,18 +211,25 @@ const loader = async (
...new Set([
...(props.fq ? [props.fq] : []),
- ...url.searchParams.getAll("fq"),
+ ...url.searchParams.getAll("fq").map((v) =>
+ removeScriptChars(removeNonLatin1Chars(v))
+ ),
</file context>
| const rawPage = url.searchParams.get("page") | ||
| ? Number(url.searchParams.get("page")) | ||
| : NaN; | ||
| const pageParam = Number.isFinite(rawPage) && rawPage >= currentPageoffset |
There was a problem hiding this comment.
P2: page validation still accepts fractional numbers, which can generate invalid pagination offsets (_from/_to). Restrict page to integers before computing offsets.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At vtex/loaders/legacy/productListingPage.ts, line 217:
<comment>`page` validation still accepts fractional numbers, which can generate invalid pagination offsets (`_from`/`_to`). Restrict `page` to integers before computing offsets.</comment>
<file context>
@@ -204,18 +211,25 @@ const loader = async (
+ const rawPage = url.searchParams.get("page")
+ ? Number(url.searchParams.get("page"))
+ : NaN;
+ const pageParam = Number.isFinite(rawPage) && rawPage >= currentPageoffset
+ ? rawPage - currentPageoffset
: 0;
</file context>
![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: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@vtex/loaders/intelligentSearch/productListingPage.ts`:
- Around line 177-184: The current parsing allows fractional page values
(rawPage) because it only checks Number.isFinite; update the validation to
require an integer: when computing parsedPage use Number.isInteger(rawPage) (or
equivalent integer check) instead of Number.isFinite so values like ?page=1.5
are rejected; keep the existing comparisons to currentPageoffset and
VTEX_MAX_PAGES and ensure page still falls back to 0 or props.page as before
(refer to rawPage, parsedPage, page, currentPageoffset, VTEX_MAX_PAGES).
- Around line 164-169: The cacheKey currently uses the raw URL query value while
the loader normalizes simulationBehavior using VALID_SIM_BEHAVIORS and the
simulationBehavior variable, causing cache fragmentation; update the cacheKey
construction to reference the normalized simulationBehavior variable (the result
of the VALID_SIM_BEHAVIORS check / props fallback) instead of
url.searchParams.get("simulationBehavior") so equivalent requests produce the
same key and ensure the value is cast/typed as SimulationBehavior where needed.
In `@vtex/loaders/legacy/productListingPage.ts`:
- Around line 214-220: The loader currently allows fractional pages because it
only checks Number.isFinite(rawPage); change the validation to require integers:
parse the query string into rawPage (keep the variable) then verify
Number.isInteger(rawPage) (or compare parseInt(pageStr,10) to the original
pageStr) and rawPage >= currentPageoffset before computing pageParam and page;
update the conditional that sets pageParam (uses rawPage and currentPageoffset)
so only integer rawPage values are accepted, and still clamp the result with
Math.min(..., MAX_ALLOWED_PAGES).
- Around line 230-232: The current fq sanitization chain uses
removeNonLatin1Chars which aggressively strips valid VTEX filter characters and
can break filters; update the map over url.searchParams.getAll("fq") to only
remove script/control characters by calling removeScriptChars (and optional
trim) and remove the removeNonLatin1Chars call so separators/ranges/accents
remain intact; locate the code that maps getAll("fq") and replace the chained
call removeScriptChars(removeNonLatin1Chars(v)) with a narrower sanitizer such
as removeScriptChars(v).trim() (or an explicit control-char strip) so valid
filter syntax is preserved.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4691508f-c66f-4235-a2ba-68d7b85758b1
📒 Files selected for processing (3)
vtex/loaders/intelligentSearch/productListingPage.tsvtex/loaders/legacy/productListingPage.tsvtex/middleware.ts
Comment on lines
+164
to
+169
| const VALID_SIM_BEHAVIORS = new Set(["default", "skip", "only1P"]); | ||
| const rawSim = url.searchParams.get("simulationBehavior"); | ||
| const simulationBehavior = | ||
| url.searchParams.get("simulationBehavior") as SimulationBehavior || | ||
| props.simulationBehavior || "default"; | ||
| (rawSim && VALID_SIM_BEHAVIORS.has(rawSim) | ||
| ? rawSim | ||
| : props.simulationBehavior || "default") as SimulationBehavior; |
Contributor
There was a problem hiding this comment.
Normalize simulationBehavior consistently in loader and cache key.
Line 166–169 correctly normalizes invalid simulationBehavior to default, but cacheKey still includes raw query values (Line 475–478). Equivalent requests can generate different cache keys, enabling cache fragmentation with junk params.
Proposed fix (outside this hunk: cacheKey section)
+ const rawSimulationBehavior = url.searchParams.get("simulationBehavior");
+ const simulationBehavior = rawSimulationBehavior &&
+ VALID_SIM_BEHAVIORS.has(rawSimulationBehavior)
+ ? rawSimulationBehavior
+ : props.simulationBehavior || "default";
+
const params = new URLSearchParams([
@@
- [
- "simulationBehavior",
- url.searchParams.get("simulationBehavior") || props.simulationBehavior ||
- "default",
- ],
+ ["simulationBehavior", simulationBehavior],
]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@vtex/loaders/intelligentSearch/productListingPage.ts` around lines 164 - 169,
The cacheKey currently uses the raw URL query value while the loader normalizes
simulationBehavior using VALID_SIM_BEHAVIORS and the simulationBehavior
variable, causing cache fragmentation; update the cacheKey construction to
reference the normalized simulationBehavior variable (the result of the
VALID_SIM_BEHAVIORS check / props fallback) instead of
url.searchParams.get("simulationBehavior") so equivalent requests produce the
same key and ensure the value is cast/typed as SimulationBehavior where needed.
Comment on lines
+177
to
+184
| const rawPage = url.searchParams.get("page") | ||
| ? Number(url.searchParams.get("page")) | ||
| : NaN; | ||
| const parsedPage = Number.isFinite(rawPage) && rawPage >= currentPageoffset | ||
| ? rawPage - currentPageoffset | ||
| : 0; | ||
| const page = props.page ?? | ||
| Math.min( | ||
| url.searchParams.get("page") | ||
| ? Number(url.searchParams.get("page")) - currentPageoffset | ||
| : 0, | ||
| VTEX_MAX_PAGES - currentPageoffset, | ||
| ); | ||
| Math.min(parsedPage, VTEX_MAX_PAGES - currentPageoffset); |
Contributor
There was a problem hiding this comment.
page defensive parsing should enforce integer values.
Line 180 uses Number.isFinite, so ?page=1.5 is still accepted and converted into a fractional internal page index. Validate rawPage as an integer here as well.
Proposed fix
- const parsedPage = Number.isFinite(rawPage) && rawPage >= currentPageoffset
+ const parsedPage = Number.isInteger(rawPage) && rawPage >= currentPageoffset
? rawPage - currentPageoffset
: 0;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@vtex/loaders/intelligentSearch/productListingPage.ts` around lines 177 - 184,
The current parsing allows fractional page values (rawPage) because it only
checks Number.isFinite; update the validation to require an integer: when
computing parsedPage use Number.isInteger(rawPage) (or equivalent integer check)
instead of Number.isFinite so values like ?page=1.5 are rejected; keep the
existing comparisons to currentPageoffset and VTEX_MAX_PAGES and ensure page
still falls back to 0 or props.page as before (refer to rawPage, parsedPage,
page, currentPageoffset, VTEX_MAX_PAGES).
Comment on lines
+214
to
+220
| const rawPage = url.searchParams.get("page") | ||
| ? Number(url.searchParams.get("page")) | ||
| : NaN; | ||
| const pageParam = Number.isFinite(rawPage) && rawPage >= currentPageoffset | ||
| ? rawPage - currentPageoffset | ||
| : 0; | ||
| const page = props.page || pageParam; | ||
| const O: LegacySort = (url.searchParams.get("O") as LegacySort) ?? | ||
| const page = props.page || Math.min(pageParam, MAX_ALLOWED_PAGES); |
Contributor
There was a problem hiding this comment.
page parsing still accepts non-integer values.
Line 217 validates only Number.isFinite, so inputs like ?page=1.5 pass and produce fractional _from/_to offsets later (Line 235–236). This should be integer-only in the loader too.
Proposed fix
- const pageParam = Number.isFinite(rawPage) && rawPage >= currentPageoffset
+ const pageParam = Number.isInteger(rawPage) && rawPage >= currentPageoffset
? rawPage - currentPageoffset
: 0;📝 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
| const rawPage = url.searchParams.get("page") | |
| ? Number(url.searchParams.get("page")) | |
| : NaN; | |
| const pageParam = Number.isFinite(rawPage) && rawPage >= currentPageoffset | |
| ? rawPage - currentPageoffset | |
| : 0; | |
| const page = props.page || pageParam; | |
| const O: LegacySort = (url.searchParams.get("O") as LegacySort) ?? | |
| const page = props.page || Math.min(pageParam, MAX_ALLOWED_PAGES); | |
| const rawPage = url.searchParams.get("page") | |
| ? Number(url.searchParams.get("page")) | |
| : NaN; | |
| const pageParam = Number.isInteger(rawPage) && rawPage >= currentPageoffset | |
| ? rawPage - currentPageoffset | |
| : 0; | |
| const page = props.page || Math.min(pageParam, MAX_ALLOWED_PAGES); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@vtex/loaders/legacy/productListingPage.ts` around lines 214 - 220, The loader
currently allows fractional pages because it only checks
Number.isFinite(rawPage); change the validation to require integers: parse the
query string into rawPage (keep the variable) then verify
Number.isInteger(rawPage) (or compare parseInt(pageStr,10) to the original
pageStr) and rawPage >= currentPageoffset before computing pageParam and page;
update the conditional that sets pageParam (uses rawPage and currentPageoffset)
so only integer rawPage values are accepted, and still clamp the result with
Math.min(..., MAX_ALLOWED_PAGES).
Comment on lines
+230
to
+232
| ...url.searchParams.getAll("fq").map((v) => | ||
| removeScriptChars(removeNonLatin1Chars(v)) | ||
| ), |
Contributor
There was a problem hiding this comment.
fq sanitization is too destructive and can break valid filters.
Line 230–232 strips characters commonly used by legitimate VTEX filter syntax (e.g. separators/ranges/accents), which can silently alter queries and return wrong or empty PLPs. Use narrower sanitization (script tags/control chars) instead of broad character removal.
Safer direction
- ...url.searchParams.getAll("fq").map((v) =>
- removeScriptChars(removeNonLatin1Chars(v))
- ),
+ ...url.searchParams.getAll("fq")
+ .map((v) =>
+ v
+ .replace(/<\/?script\b[^>]*>/gi, "")
+ .replace(/[\u0000-\u001F\u007F]/g, "")
+ .trim()
+ )
+ .filter(Boolean),📝 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
| ...url.searchParams.getAll("fq").map((v) => | |
| removeScriptChars(removeNonLatin1Chars(v)) | |
| ), | |
| ...url.searchParams.getAll("fq") | |
| .map((v) => | |
| v | |
| .replace(/<\/?script\b[^>]*>/gi, "") | |
| .replace(/[\u0000-\u001F\u007F]/g, "") | |
| .trim() | |
| ) | |
| .filter(Boolean), |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@vtex/loaders/legacy/productListingPage.ts` around lines 230 - 232, The
current fq sanitization chain uses removeNonLatin1Chars which aggressively
strips valid VTEX filter characters and can break filters; update the map over
url.searchParams.getAll("fq") to only remove script/control characters by
calling removeScriptChars (and optional trim) and remove the
removeNonLatin1Chars call so separators/ranges/accents remain intact; locate the
code that maps getAll("fq") and replace the chained call
removeScriptChars(removeNonLatin1Chars(v)) with a narrower sanitizer such as
removeScriptChars(v).trim() (or an explicit control-char strip) so valid filter
syntax is preserved.
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
1 participant
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.
Summary
pageandPSquery params — invalid values (SSRF URLs, garbage strings) now get stripped with a 301 redirect instead of propagating NaN through PLP loaderscount,page, andsimulationBehavior(validated against known values instead of blindascast)countandpage, sort validation againstsortOptions(matching whatcacheKeyalready does), andfqsanitization withremoveScriptChars/removeNonLatin1CharsContext
A production site was hit by bots passing URLs as
?page=values, causingNumber("http://...")→NaN→ crash in product listing page loaders. The fix was initially a site-level middleware; this PR moves it into the VTEX app as a default safeguard for all sites, plus hardens other unsanitized params.Test plan
?page=1works normally (no redirect)?page=http://evil.comgets 301 redirected to URL withoutpageparam?page=0,?page=-1,?page=1.5,?page=NaNall get redirected?PS=12works normally,?PS=garbagegets redirected?sort=invalidfalls back to default sort in both IS and legacy loaders?fq=<script>alert(1)</script>gets sanitized before reaching VTEX API🤖 Generated with Claude Code
Summary by cubic
Sanitizes VTEX query params to stop PLP crashes caused by NaN. Invalid page and PS values are removed via 301 redirect; IS and legacy loaders now validate and clean incoming params.
Written for commit 33044ed. Summary will update on new commits.
Summary by CodeRabbit
Release Notes