WIP: fix Logout and session VTEX #1366
Conversation
…shtoken/webstore” in refreshToken and setLoginCookies.
WalkthroughCentralizes cookie handling into a cookie-jar/proxy model, updates authentication and session flows to propagate cookies via headers, adds session validation and logout wiring, expands session types/utilities, and exposes cache policies on multiple loaders. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Browser
participant AuthAction as Auth Action
participant Backend as VTEX Id API
participant SessionVal as session.validateSession
Browser->>AuthAction: Auth request (with Cookie header)
AuthAction->>AuthAction: startSetCookies = getSetCookies(ctx.response.headers)
AuthAction->>AuthAction: jar = buildCookieJar(req.headers, startSetCookies)
AuthAction->>Backend: POST /... with Header: Cookie: jar.header
Backend-->>AuthAction: 200 + Set-Cookie*
Note over AuthAction: proxySetCookie(from Backend → ctx.response.headers)
Note over AuthAction: rewrite REFRESH_TOKEN_COOKIE path if present
AuthAction->>SessionVal: validateSession()
SessionVal-->>AuthAction: Session
AuthAction-->>Browser: Final response (cookies proxied + redirect/JSON)
sequenceDiagram
autonumber
participant Browser
participant LogoutSection as Logout Section
participant LogoutAction as auth.logout
participant VTEX as VTEX Id API
participant SessionVal as session.validateSession
Browser->>LogoutSection: GET /logout?returnUrl=...
LogoutSection->>LogoutAction: logout({ returnUrl })
LogoutAction->>VTEX: GET /api/vtexid/pub/logout?scope=account&returnUrl=...
VTEX-->>LogoutAction: 200 + Set-Cookie*
Note over LogoutAction: proxySetCookie(from VTEX → ctx.response.headers)
LogoutAction->>SessionVal: validateSession()
SessionVal-->>LogoutAction: Session
LogoutAction-->>LogoutSection: LogoutResponse
LogoutSection-->>Browser: Redirect to returnUrl
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
💤 Files with no reviewable changes (1)
🔇 Additional comments (1)
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 |
Tagging OptionsShould a new tag be published when this PR is merged?
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
vtex/utils/login/setLoginCookies.ts (1)
29-36: Add deletion ofVID_RT_COOKIE_NAMEcookie with matching path in logout/clear flows. NodeleteCookieorsetCookie(..., maxAge: 0)calls forVID_RT_COOKIE_NAMEwere found; implement deletion using the samepath: "/api/vtexid/refreshtoken/webstore".
🧹 Nitpick comments (2)
vtex/actions/authentication/refreshToken.ts (1)
70-70: Extract and reuseVID_RT_COOKIE_PATHfor refresh-token cookie
- In
vtex/utils/login/setLoginCookies.ts, add:export const VID_RT_COOKIE_NAME = "vid_rt";
- export const VID_RT_COOKIE_PATH = "/api/vtexid/refreshtoken/webstore";
…
setCookie(ctx.response.headers, {
name: VID_RT_COOKIE_NAME,
value: vidRtCookie.value,
httpOnly: true,
maxAge,
path: "/api/vtexid/refreshtoken/webstore",
path: VID_RT_COOKIE_PATH, secure: true,
- In
vtex/actions/authentication/refreshToken.ts, import and use it:- import { VID_RT_COOKIE_NAME } from "../../utils/login/setLoginCookies.ts"; + import { + VID_RT_COOKIE_NAME, + VID_RT_COOKIE_PATH, + } from "../../utils/login/setLoginCookies.ts"; … setCookie(ctx.response.headers, { name: VID_RT_COOKIE_NAME, value: vidRtCookie.value, httpOnly: true, maxAge,path: "/api/vtexid/refreshtoken/webstore",
path: VID_RT_COOKIE_PATH, secure: true,
- Confirmed route mapping
"POST /api/vtexid/refreshtoken/webstore"inmanifest.gen.tsmatches this path.vtex/utils/login/setLoginCookies.ts (1)
34-34: Extract a shared path constant and (optionally) set SameSite
- Create
VID_RT_COOKIE_PATHand reuse it across files.- Consider
sameSite: "Lax"for the refresh token cookie unless cross-site POSTs to that path are required.Apply:
export const VID_RT_COOKIE_NAME = "vid_rt"; +export const VID_RT_COOKIE_PATH = "/api/vtexid/refreshtoken/webstore"; @@ setCookie(ctx.response.headers, { name: VID_RT_COOKIE_NAME, value: vidRtCookie.value, httpOnly: true, maxAge: maxAge, - path: "/api/vtexid/refreshtoken/webstore", + path: VID_RT_COOKIE_PATH, secure: true, + // sameSite can be adjusted if needed: + // sameSite: "Lax", });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
vtex/actions/authentication/refreshToken.ts(1 hunks)vtex/utils/login/setLoginCookies.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Bundle & Check Apps (ubuntu-latest)
🔇 Additional comments (2)
vtex/actions/authentication/refreshToken.ts (1)
70-70: Cookie path alignment looks goodRestricting
vid_rtto/api/vtexid/refreshtoken/webstorematches VTEX defaults and avoids cross-route conflicts (e.g., logout).vtex/utils/login/setLoginCookies.ts (1)
34-34: Cookie path change aligns with refresh flowUsing
/api/vtexid/refreshtoken/webstoreforvid_rtis consistent with VTEX and the change inrefreshToken.ts.
marcoferreiradev
changed the title
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
vtex/utils/cookies.ts (1)
13-30: Support cookie path overrides when proxying (align VID_RT to VTEX refresh-token path).To meet the PR goal (VID_RT refresh token under
/api/vtexid/refreshtoken/webstore) and avoid conflicts with logout, allow per-cookie path overrides during proxy.Apply:
-export const proxySetCookie = ( +export const proxySetCookie = ( from: Headers, to: Headers, toDomain?: URL | string, -) => { + overrides?: { pathByName?: Record<string, string> }, +) => { const newDomain = toDomain && new URL(toDomain); for (const cookie of getSetCookies(from)) { - const newCookie = newDomain - ? { - ...cookie, - domain: newDomain.hostname, - } - : cookie; + const newCookie = { + ...cookie, + ...(newDomain ? { domain: newDomain.hostname } : {}), + ...(overrides?.pathByName?.[cookie.name] + ? { path: overrides.pathByName[cookie.name] } + : {}), + }; setCookie(to, newCookie); } };Usage example in callers when dealing with VTEX ID cookies:
proxySetCookie(upstream.headers, ctx.response.headers, req.url, { pathByName: { vid_rt: "/api/vtexid/refreshtoken/webstore" }, });
🧹 Nitpick comments (7)
vtex/utils/session.ts (1)
1-36: Solid centralization of session fields; consider freezing for typo-safetyLooks good. Consider
as constto lock literals and help tooling catch typos when composing requests.-export const items = [ +export const items = [ "account.id", "account.accountName", "store.channel", "store.countryCode", "store.cultureInfo", "store.currencyCode", "store.currencySymbol", "store.admin_cultureInfo", "creditControl.creditAccounts", "creditControl.deadlines", "creditControl.minimumInstallmentValue", "authentication.storeUserId", "authentication.storeUserEmail", "authentication.customerId", "authentication.unitId", "authentication.unitName", "profile.firstName", "profile.document", "profile.email", "profile.id", "profile.isAuthenticated", "profile.lastName", "profile.phone", "shopper.firstName", "shopper.lastName", "checkout.regionId", "public.favoritePickup", "public.utm_source", "public.utm_medium", "public.utm_campaign", "public.utmi_cp", "public.utmi_p", "public.utmi_pc", "public.postalCode", -]; +] as const;vtex/middleware.ts (1)
43-61: Initialize cookies/segment bags once; comment noiseLogic looks fine. Please drop commented logging before merge to keep middleware lean.
vtex/actions/session/createSession.ts (1)
36-38: Consider proxying Set-Cookie even on non-2xx responses.Some upstream errors clear auth cookies. You currently throw before proxying. Optionally proxy first, then throw.
Example:
- if (!response.ok) { - throw new Error(`Failed to create session: ${response.status}`); - } - - proxySetCookie(response.headers, ctx.response.headers, req.url); + proxySetCookie(response.headers, ctx.response.headers, req.url); + if (!response.ok) { + throw new Error(`Failed to create session: ${response.status}`); + }vtex/utils/cookies.ts (1)
42-70: Nit: determinism and minor ergonomics inbuildCookieJar.
- Consider sorting cookie names when building the header for deterministic outputs (debugging, tests).
- Tiny guard: skip empty names after normalization.
Suggested tweak:
- Object.entries(incoming).forEach(([name, value]) => { - jar.set(name, { name, value }); - }); + Object.entries(incoming).forEach(([name, value]) => { + if (!name) return; + jar.set(name, { name, value }); + }); @@ - const cookies = Array.from(jar.values()); + const cookies = Array.from(jar.values()).sort((a, b) => + a.name.localeCompare(b.name) + );vtex/actions/session/editSession.ts (1)
39-39: If fixing VID_RT path, pass overrides toproxySetCookiehere too.Keeps behavior consistent with the refresh-token path requirement.
Example:
-proxySetCookie(response.headers, ctx.response.headers, req.url); +proxySetCookie(response.headers, ctx.response.headers, req.url, { + pathByName: { vid_rt: "/api/vtexid/refreshtoken/webstore" }, +});vtex/actions/authentication/startAuthentication.ts (1)
39-40: Optional: unify cookie forwarding and apply path override when proxying.
- Consider
buildCookieJaracross actions for consistency (mergectx.responseSet-Cookies).- If you adopt path overrides, pass them here as well when applicable.
Example:
- headers: { cookie: req.headers.get("cookie") || "" }, + headers: { cookie: req.headers.get("cookie") || "" }, @@ - proxySetCookie(response.headers, ctx.response.headers, req.url); + proxySetCookie(response.headers, ctx.response.headers, req.url, { + pathByName: { vid_rt: "/api/vtexid/refreshtoken/webstore" }, + });Also applies to: 53-53
vtex/actions/authentication/classicSignIn.ts (1)
49-58: Send the Cookie header only when non-empty.Avoid sending an empty "cookie" header; some backends treat an empty header differently from absence.
- const response = await vcsDeprecated + const baseHeaders = { + "Content-Type": "application/x-www-form-urlencoded", + "Accept": "application/json", + } as Record<string, string>; + if (cookieHeader) baseHeaders.cookie = cookieHeader; + const response = await vcsDeprecated ["POST /api/vtexid/pub/authentication/classic/validate"]( {}, { body: urlencoded, - headers: { - "Content-Type": "application/x-www-form-urlencoded", - "Accept": "application/json", - "cookie": cookieHeader, - }, + headers: baseHeaders, }, );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
vtex/actions/authentication/accessKeySignIn.ts(2 hunks)vtex/actions/authentication/classicSignIn.ts(4 hunks)vtex/actions/authentication/logout.ts(1 hunks)vtex/actions/authentication/sendEmailVerification.ts(4 hunks)vtex/actions/authentication/startAuthentication.ts(4 hunks)vtex/actions/session/createSession.ts(2 hunks)vtex/actions/session/editSession.ts(2 hunks)vtex/actions/session/validateSession.ts(1 hunks)vtex/loaders/intelligentSearch/productDetailsPage.ts(1 hunks)vtex/loaders/legacy/productDetailsPage.ts(1 hunks)vtex/loaders/legacy/relatedProductsLoader.ts(1 hunks)vtex/manifest.gen.ts(4 hunks)vtex/middleware.ts(1 hunks)vtex/sections/Authentication/Logout.tsx(1 hunks)vtex/utils/cookies.ts(2 hunks)vtex/utils/segment.ts(4 hunks)vtex/utils/session.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- vtex/loaders/legacy/productDetailsPage.ts
🧰 Additional context used
🧬 Code graph analysis (9)
vtex/actions/session/validateSession.ts (2)
vtex/actions/authentication/classicSignIn.ts (1)
action(16-86)vtex/actions/authentication/logout.ts (1)
action(19-71)
vtex/actions/authentication/logout.ts (2)
vtex/actions/session/validateSession.ts (1)
action(4-16)vtex/utils/cookies.ts (1)
proxySetCookie(13-30)
vtex/utils/cookies.ts (1)
vtex/utils/openapi/orders.openapi.gen.ts (1)
Headers(6147-6149)
vtex/actions/session/createSession.ts (1)
vtex/utils/session.ts (1)
items(1-36)
vtex/actions/authentication/classicSignIn.ts (2)
vtex/utils/types.ts (1)
AuthResponse(1740-1756)vtex/utils/cookies.ts (2)
buildCookieJar(42-70)proxySetCookie(13-30)
vtex/actions/session/editSession.ts (2)
vtex/utils/cookies.ts (2)
buildCookieJar(42-70)proxySetCookie(13-30)vtex/utils/session.ts (1)
items(1-36)
vtex/middleware.ts (3)
vtex/mod.ts (1)
AppMiddlewareContext(27-27)vtex/utils/segment.ts (2)
getSegmentFromBag(67-69)parse(117-117)vtex/utils/intelligentSearch.ts (2)
getISCookiesFromBag(80-92)setISCookiesBag(94-134)
vtex/utils/segment.ts (1)
vtex/utils/vtexId.ts (1)
parseCookie(16-36)
vtex/actions/authentication/startAuthentication.ts (3)
vtex/utils/types.ts (1)
StartAuthentication(1764-1774)vtex/utils/segment.ts (1)
getSegmentFromBag(67-69)vtex/utils/cookies.ts (1)
proxySetCookie(13-30)
🔇 Additional comments (4)
vtex/actions/authentication/logout.ts (1)
38-39: Confirm redirect merges cookie headers or set explicit redirect Response
- Verify whether
@deco/deco’sredirect(returnUrl)carries over the currentctx.response.headers(including allSet-Cookieentries). If it does, replace the two-step side-effect call and return with a singlereturn redirect(returnUrl).- If it does not, explicitly proxy cookies from both
logoutResponseand—ifvalidateSession()returns aResponse—its headers, then setin place of lines 69–71 inctx.response.headers.set("Location", returnUrl); ctx.response.status = 302; return ctx.response;vtex/actions/authentication/logout.ts.vtex/utils/cookies.ts (1)
13-30: Refresh-token cookie path handling confirmed in setLoginCookies.ts (line 34) and refreshToken.ts (line 70); no changes needed in proxySetCookie.vtex/actions/authentication/classicSignIn.ts (1)
72-75: Validate session with merged cookies (double-check contract).Assuming
validateSessionbuilds its cookie jar fromreq.headersplusctx.response.headersSet-Cookie (as done here), this is fine. If it only readsreq.headers, it will miss the newly set auth cookies.I can scan the repo to confirm how
validateSessioncomposes cookies and, if needed, wire the samebuildCookieJar(req.headers, getSetCookies(ctx.response.headers))pattern there. Want me to run that check?vtex/manifest.gen.ts (1)
45-48: Generated manifest updates look consistent.Imports and action/section mappings include
session/validateSession.tsandsections/Authentication/Logout.tsx, with renumbered aliases. No issues spotted.Since this file is generated, ensure it’s rebuilt locally after the last change (
dev.ts) so alias ordering matches the build you deploy.Also applies to: 162-164, 206-209
| const data: AuthResponse = await response.json(); | ||
| const setCookies = getSetCookies(response.headers); | ||
| await setLoginCookies(data, ctx, setCookies); | ||
| proxySetCookie(response.headers, ctx.response.headers, req.url); | ||
| await ctx.invoke.vtex.actions.session.validateSession(); | ||
| // const setCookies = getSetCookies(response.headers); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure VTEX auth cookies use the refresh-token path when proxying.
PR objective mentions normalizing auth cookies to Path=/api/vtexid/refreshtoken/webstore. proxySetCookie only rewrites Domain; it won’t fix Path. Set the Path explicitly for auth cookies when proxying.
Apply this diff (keeps domain rewrite and enforces path for relevant cookies):
-import { getCookies, getSetCookies } from "std/http/cookie.ts";
+import { getCookies, getSetCookies, setCookie } from "std/http/cookie.ts";- proxySetCookie(response.headers, ctx.response.headers, req.url);
+ const VTEX_REFRESH_PATH = "/api/vtexid/refreshtoken/webstore";
+ const targetHost = new URL(req.url).hostname;
+ for (const c of getSetCookies(response.headers)) {
+ const needsAuthPath =
+ c.name === "vid_rt" || c.name.startsWith("VtexIdclientAutCookie");
+ setCookie(ctx.response.headers, {
+ ...c,
+ domain: targetHost,
+ path: needsAuthPath ? VTEX_REFRESH_PATH : c.path,
+ });
+ }
await ctx.invoke.vtex.actions.session.validateSession();Follow-up: If you prefer a reusable approach, extend proxySetCookie to accept an optional toPath for specific cookie names; I can draft that change in vtex/utils/cookies.ts.
📝 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.
| const data: AuthResponse = await response.json(); | |
| const setCookies = getSetCookies(response.headers); | |
| await setLoginCookies(data, ctx, setCookies); | |
| proxySetCookie(response.headers, ctx.response.headers, req.url); | |
| await ctx.invoke.vtex.actions.session.validateSession(); | |
| // const setCookies = getSetCookies(response.headers); | |
| ++ b/vtex/actions/authentication/classicSignIn.ts | |
| @@ -1,7 +1,7 @@ | |
| import { getCookies, getSetCookies, setCookie } from "std/http/cookie.ts"; | |
| import { proxySetCookie } from "../../utils/cookies.ts"; | |
| export async function classicSignIn(ctx: Context, req: Request) { | |
| // … earlier code … | |
| @@ -70,7 +70,16 @@ export async function classicSignIn(ctx: Context, req: Request) { | |
| const data: AuthResponse = await response.json(); | |
| // Rewrite domain and explicitly set the refresh‐token path for VTEX auth cookies | |
| const VTEX_REFRESH_PATH = "/api/vtexid/refreshtoken/webstore"; | |
| const targetHost = new URL(req.url).hostname; | |
| for (const c of getSetCookies(response.headers)) { | |
| const needsAuthPath = | |
| c.name === "vid_rt" || c.name.startsWith("VtexIdclientAutCookie"); | |
| setCookie(ctx.response.headers, { | |
| ...c, | |
| domain: targetHost, | |
| path: needsAuthPath ? VTEX_REFRESH_PATH : c.path, | |
| }); | |
| } | |
| await ctx.invoke.vtex.actions.session.validateSession(); | |
| // const setCookies = getSetCookies(response.headers); | |
| return data; |
🤖 Prompt for AI Agents
In vtex/actions/authentication/classicSignIn.ts around lines 72–75,
proxySetCookie currently only rewrites Domain but not Path, so auth cookies must
be proxied with Path=/api/vtexid/refreshtoken/webstore; update this call to
preserve the existing domain rewrite and also enforce the Path for auth-related
cookies (e.g., the refresh-token cookie) by either passing a toPath argument if
proxySetCookie supports it or by retrieving Set-Cookie headers, rewriting/adding
Path=/api/vtexid/refreshtoken/webstore for the relevant cookie names, and
writing the modified Set-Cookie headers back to ctx.response.headers before
invoking session.validateSession.
| @@ -1,5 +1,6 @@ | |||
| import { setCookie } from "std/http/mod.ts"; | |||
| import type { AppContext } from "../../mod.ts"; | |||
| import { getCookies } from "std/http/cookie.ts"; | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Drop cookie-logging; revert param/import now unused
Logging cookies/response cookies leaks tokens. After removal, getCookies and req become unused; revert.
-import { getCookies } from "std/http/cookie.ts";
+// no cookie logging; import removed
@@
-export default async function action(
- props: Props,
- req: Request,
- ctx: AppContext,
+export default async function action(
+ props: Props,
+ _req: Request,
+ ctx: AppContext,
): Promise<SendEmailVerificationResult> {
@@
- const cookies = getCookies(req.headers);
- console.log("sendEmailVerification cookies", cookies);
+ // do not log raw cookies
@@
- const responseCookies = getCookies(response.headers);
- console.log("sendEmailVerification responseCookies", responseCookies);
+ // do not log Set-Cookie contentsAlso applies to: 19-19, 28-30, 70-72
🤖 Prompt for AI Agents
In vtex/actions/authentication/sendEmailVerification.ts around lines 3, 19,
28-30, and 70-72: remove the cookie-logging and revert the now-unused getCookies
import and any unused req parameter; specifically delete the import line for
getCookies, remove any console/processLogger calls that log request or response
cookies or tokens, restore any function signatures that included req if it's no
longer used, and then run a quick lint to ensure no unused imports/vars remain.
| console.log( | ||
| "cacheKey segment productDetailsPage", | ||
| JSON.parse(atob(segment))?.priceTables, | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Guard segment decoding in cacheKey; avoid logging sensitive details
atob(segment) will throw on undefined/invalid values. Wrap in try/catch and avoid verbose logs in hot paths.
- console.log(
- "cacheKey segment productDetailsPage",
- JSON.parse(atob(segment))?.priceTables,
- );
+ if (segment) {
+ try {
+ const payload = JSON.parse(atob(segment));
+ console.debug(
+ "cacheKey segment productDetailsPage",
+ payload?.priceTables,
+ );
+ } catch {
+ // ignore malformed segment cookie
+ }
+ }📝 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.
| console.log( | |
| "cacheKey segment productDetailsPage", | |
| JSON.parse(atob(segment))?.priceTables, | |
| ); | |
| if (segment) { | |
| try { | |
| const payload = JSON.parse(atob(segment)); | |
| console.debug( | |
| "cacheKey segment productDetailsPage", | |
| payload?.priceTables, | |
| ); | |
| } catch { | |
| // ignore malformed segment cookie | |
| } | |
| } |
🤖 Prompt for AI Agents
In vtex/loaders/intelligentSearch/productDetailsPage.ts around lines 167 to 170,
the code calls atob(segment) and JSON.parse without guarding for
undefined/invalid values and prints potentially sensitive data; wrap the
decode+parse in a try/catch (or validate segment before decoding), bail out or
use a safe fallback when decode/parse fails, and replace the verbose console.log
with a minimal, non-sensitive log (e.g., log that segment decoding failed or log
only presence/length) to avoid throwing in hot paths and exposing data.
| function getReturnUrl(url: string, defaultUrl: string = "/") { | ||
| const returnUrl = url?.split("returnUrl=")?.[1] || defaultUrl; | ||
| return decodeURIComponent(returnUrl); | ||
| } |
There was a problem hiding this comment.
Fix open-redirect risk and fragile returnUrl parsing.
Current split/decoding accepts arbitrary URLs (e.g., https://evil.com) and can redirect off-site. Parse with URLSearchParams and allow only safe, same-origin-relative paths.
Apply:
-function getReturnUrl(url: string, defaultUrl: string = "/") {
- const returnUrl = url?.split("returnUrl=")?.[1] || defaultUrl;
- return decodeURIComponent(returnUrl);
-}
+function getReturnUrl(url: string, defaultUrl = "/") {
+ try {
+ const u = new URL(url);
+ const r = u.searchParams.get("returnUrl");
+ if (!r) return defaultUrl;
+ // allow only relative paths like "/foo" (disallow "//host" and absolute URLs)
+ if (/^\/(?!\/)/.test(r)) return r;
+ return defaultUrl;
+ } catch {
+ return defaultUrl;
+ }
+}
@@
- console.log("SECTION LOGOUT returnUrl", returnUrl);
- const teste = await ctx.invoke.vtex.actions.authentication.logout({
+ const result = await ctx.invoke.vtex.actions.authentication.logout({
returnUrl,
});
- console.log("SECTION LOGOUT teste", teste);
- return teste;
+ return result;Also applies to: 8-16
🤖 Prompt for AI Agents
In vtex/sections/Authentication/Logout.tsx around lines 3-6 (and similarly for
lines 8-16), replace the fragile split/decode logic with URLSearchParams to read
the returnUrl param, then validate it is a safe same-origin relative path before
using it: parse the current URL's search with new
URLSearchParams(location.search) to get returnUrl, reject any value that doesn't
start with a single '/' (disallow values starting with '//' or containing '://'
or other schemes), and fall back to defaultUrl if validation fails; ensure you
do not allow full origins or protocol-prefixed strings and always return the
validated relative path or the default.
| }; | ||
|
|
||
| const parse = (cookie: string) => JSON.parse(atob(cookie)); | ||
| export const parse = (cookie: string) => JSON.parse(atob(cookie)); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Harden segment parsing; tame verbose logs
- Make
parsetolerant to tampering to avoid 500s when clients send invalidvtex_segment. - Reduce/guard logs that may dump segment internals.
-export const parse = (cookie: string) => JSON.parse(atob(cookie));
+export const parse = (cookie: string) => {
+ try {
+ return JSON.parse(atob(cookie));
+ } catch {
+ return null;
+ }
+};- const segmentFromCookie = vtex_segment ? parse(vtex_segment) : null;
- console.log("setSegmentBag priceTables", segmentFromCookie?.priceTables);
+ const segmentFromCookie = vtex_segment ? parse(vtex_segment) : null;
+ if (segmentFromCookie) {
+ console.debug("setSegmentBag priceTables", segmentFromCookie?.priceTables);
+ }
@@
- const { payload: userPayload } = parseCookie(req.headers, ctx.account);
-
- const isAuthenticated = !!userPayload;
- // console.log("setSegmentBag vtex_segment", !!vtex_segment);
- console.log("setSegmentBag isAuthenticated", isAuthenticated);
+ const { payload: userPayload } = parseCookie(req.headers, ctx.account);
+ const isAuthenticated = !!userPayload;
+ console.debug("setSegmentBag isAuthenticated", isAuthenticated);- console.log("setSegmentBag vtex_segment !== token");
- console.log("setSegmentBag segment", segment);
+ console.debug("setSegmentBag vtex_segment !== token");
+ console.debug("setSegmentBag segment", segment);Also applies to: 169-176, 219-221
🤖 Prompt for AI Agents
In vtex/utils/segment.ts (lines 117, and related logic at 169-176 and 219-221):
the one-liner JSON.parse(atob(cookie)) must be hardened to avoid throwing on
tampered/invalid vtex_segment and to stop dumping segment internals to logs;
replace it with a tolerant parser that first checks the cookie is a non-empty
string, wraps atob + JSON.parse in a try/catch, returns a safe null/undefined
(or empty object) on failure instead of throwing, and ensure any logs only
record minimal/masked context (e.g., presence/length or an error code) rather
than the full decoded or parsed segment; update the code at 169-176 and 219-221
to use this safe parse result and guard logging accordingly so errors are
handled gracefully and sensitive internals are never printed.
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (10)
vtex/utils/segment.ts (2)
117-117: Harden segment parse; don’t throw on tampered cookies
JSON.parse(atob(cookie))will 500 on malformed/attacker-controlled cookies. Return null on failure.-export const parse = (cookie: string) => JSON.parse(atob(cookie)); +export const parse = (cookie: unknown) => { + if (typeof cookie !== "string" || !cookie) return null; + try { + return JSON.parse(atob(cookie)); + } catch { + return null; + } +};
219-221: Stop dumping segment internals to logsAvoid logging the whole segment; keep a concise debug note.
- console.log("setSegmentBag vtex_segment !== token"); - console.log("setSegmentBag segment", segment); + console.debug("setSegmentBag cookie will be refreshed");vtex/actions/authentication/startAuthentication.ts (2)
27-30: Remove unused cookies var and sensitive debugAvoid collecting/logging request cookies; they can contain secrets.
- const cookies = getCookies(req.headers); - const segment = getSegmentFromBag(ctx); - // ("startAuthentication cookies", cookies); + const segment = getSegmentFromBag(ctx);
48-52: Don’t log cookies/Set-Cookie (secrets leak)These logs expose credentials.
- const responseCookies = getCookies(response.headers); - console.log("startAuthentication responseCookies", responseCookies); - const cookiesSet = getSetCookies(response.headers); - console.log("startAuthentication getSetCookies", cookiesSet);vtex/actions/authentication/classicSignIn.ts (6)
4-5: Adjust imports: remove unused, add setCookie for path rewriteStop importing unused helpers; we’ll rewrite cookies explicitly.
-import { getCookies, getSetCookies } from "std/http/cookie.ts"; -import { buildCookieJar, proxySetCookie } from "../../utils/cookies.ts"; +import { getSetCookies, setCookie } from "std/http/cookie.ts"; +import { buildCookieJar } from "../../utils/cookies.ts";
33-36: Don’t log the composed Cookie headerThis header can contain credentials.
- const startSetCookies = getSetCookies(ctx.response.headers); - const { header: cookieHeader } = buildCookieJar(req.headers, startSetCookies); - console.log("classicSignIn cookieHeader", cookieHeader); + const startSetCookies = getSetCookies(ctx.response.headers); + const { header: cookieHeader } = buildCookieJar(req.headers, startSetCookies);
61-65: Remove response cookie debugAvoid logging secrets and fix lint unused vars.
- const responseCookies = getCookies(response.headers); - // console.log("classicSignIn responseCookies", responseCookies); - const cookiesSet = getSetCookies(response.headers); - // console.log("classicSignIn getSetCookies", cookiesSet);
73-75: Proxy Set-Cookie with enforced refresh-token pathNormalize auth cookies to VTEX refresh path and keep domain rewrite.
- proxySetCookie(response.headers, ctx.response.headers, req.url); - await ctx.invoke.vtex.actions.session.validateSession(); + const VTEX_REFRESH_PATH = "/api/vtexid/refreshtoken/webstore"; + const targetHost = new URL(req.url).hostname; + for (const c of getSetCookies(response.headers)) { + const needsAuthPath = + c.name === "vid_rt" || c.name.startsWith("VtexIdclientAutCookie"); + setCookie(ctx.response.headers, { + ...c, + domain: targetHost, + path: needsAuthPath ? VTEX_REFRESH_PATH : (c.path ?? "/"), + }); + } + await ctx.invoke.vtex.actions.session.validateSession();
22-24: Remove unused cookies variableAvoid collecting request cookies unless strictly needed.
- const cookies = getCookies(req.headers); - // console.log("classicSignIn cookies antes", cookies);
82-83: Remove unused classicToClient debugFix lint and avoid leaking cookie metadata.
- const classicToClient = getSetCookies(ctx.response.headers); - // console.log("classicSignIn classicToClient setCookies", classicToClient);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.github/workflows/ci.yaml(1 hunks)vtex/actions/authentication/accessKeySignIn.ts(2 hunks)vtex/actions/authentication/classicSignIn.ts(4 hunks)vtex/actions/authentication/startAuthentication.ts(4 hunks)vtex/middleware.ts(1 hunks)vtex/utils/segment.ts(4 hunks)
✅ Files skipped from review due to trivial changes (1)
- .github/workflows/ci.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- vtex/actions/authentication/accessKeySignIn.ts
🧰 Additional context used
🧬 Code graph analysis (4)
vtex/actions/authentication/classicSignIn.ts (3)
vtex/mod.ts (1)
AppContext(25-25)vtex/utils/types.ts (1)
AuthResponse(1740-1756)vtex/utils/cookies.ts (2)
buildCookieJar(42-70)proxySetCookie(13-30)
vtex/actions/authentication/startAuthentication.ts (3)
vtex/utils/types.ts (1)
StartAuthentication(1764-1774)vtex/utils/segment.ts (1)
getSegmentFromBag(67-69)vtex/utils/cookies.ts (1)
proxySetCookie(13-30)
vtex/middleware.ts (3)
vtex/mod.ts (1)
AppMiddlewareContext(27-27)vtex/utils/segment.ts (2)
getSegmentFromBag(67-69)parse(117-117)vtex/utils/intelligentSearch.ts (2)
getISCookiesFromBag(80-92)setISCookiesBag(94-134)
vtex/utils/segment.ts (1)
vtex/utils/vtexId.ts (1)
parseCookie(16-36)
🪛 GitHub Check: Bundle & Check Apps (ubuntu-latest)
vtex/actions/authentication/classicSignIn.ts
[failure] 22-22:
�[0m�[1mcookies is never used
[failure] 63-63:
�[0m�[1mcookiesSet is never used
[failure] 61-61:
�[0m�[1mresponseCookies is never used
[failure] 82-82:
�[0m�[1mclassicToClient is never used
vtex/actions/authentication/startAuthentication.ts
[failure] 27-27:
�[0m�[1mcookies is never used
vtex/middleware.ts
[failure] 20-20:
�[0m�[1msegmentFromCookie is never used
vtex/utils/segment.ts
[failure] 178-178:
�[0m�[1misAuthenticated is never used
🪛 GitHub Actions: ci
vtex/middleware.ts
[error] 20-20: deno lint: no-unused-vars - 'segmentFromCookie' is never used. (Step: deno lint)
vtex/utils/segment.ts
[error] 1-1: deno lint: no-unused-vars - 'segmentFromCookie' is never used. (Step: deno lint)
🔇 Additional comments (1)
vtex/actions/authentication/startAuthentication.ts (1)
1-1: Trim imports; avoid unused and prep for path enforcementRemove unused
getCookies; addsetCookiefor controlled proxying.-import { getCookies, getSetCookies } from "std/http/cookie.ts"; +import { getSetCookies, setCookie } from "std/http/cookie.ts";Likely an incorrect or invalid review comment.
| import { AppContext } from "../../mod.ts"; | ||
| import { getSegmentFromBag } from "../../utils/segment.ts"; | ||
| import { StartAuthentication } from "../../utils/types.ts"; | ||
| import { proxySetCookie } from "../../utils/cookies.ts"; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unused helper
proxySetCookie won’t be needed after explicit Set-Cookie rewrite.
-import { proxySetCookie } from "../../utils/cookies.ts";📝 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.
| import { proxySetCookie } from "../../utils/cookies.ts"; | |
| // (line removed: import { proxySetCookie } from "../../utils/cookies.ts";) |
🤖 Prompt for AI Agents
In vtex/actions/authentication/startAuthentication.ts around line 5, the
imported helper proxySetCookie is no longer used after switching to explicit
Set-Cookie rewriting; remove the unused import statement and any related
references/usages (if present) to avoid lint errors and dead code, then run the
build/linter to confirm no remaining references.
| proxySetCookie(response.headers, ctx.response.headers, req.url); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enforce VTEX auth cookie Path=/api/vtexid/refreshtoken/webstore
PR objective is to normalize refresh-token path. Replace generic proxy with explicit rewrite of Domain and Path for auth cookies.
- proxySetCookie(response.headers, ctx.response.headers, req.url);
+ const VTEX_REFRESH_PATH = "/api/vtexid/refreshtoken/webstore";
+ const targetHost = new URL(req.url).hostname;
+ for (const c of getSetCookies(response.headers)) {
+ const needsAuthPath =
+ c.name === "vid_rt" || c.name.startsWith("VtexIdclientAutCookie");
+ setCookie(ctx.response.headers, {
+ ...c,
+ domain: targetHost,
+ path: needsAuthPath ? VTEX_REFRESH_PATH : (c.path ?? "/"),
+ });
+ }📝 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.
| proxySetCookie(response.headers, ctx.response.headers, req.url); | |
| const VTEX_REFRESH_PATH = "/api/vtexid/refreshtoken/webstore"; | |
| const targetHost = new URL(req.url).hostname; | |
| for (const c of getSetCookies(response.headers)) { | |
| const needsAuthPath = | |
| c.name === "vid_rt" || c.name.startsWith("VtexIdclientAutCookie"); | |
| setCookie(ctx.response.headers, { | |
| ...c, | |
| domain: targetHost, | |
| path: needsAuthPath ? VTEX_REFRESH_PATH : (c.path ?? "/"), | |
| }); | |
| } |
🤖 Prompt for AI Agents
In vtex/actions/authentication/startAuthentication.ts around lines 53-54,
replace the generic proxySetCookie(response.headers, ctx.response.headers,
req.url) call with logic that iterates response 'set-cookie' headers, identifies
VTEX auth cookies (e.g., refresh/auth cookie names used by VTEX), and for those
cookies explicitly rewrite their Path to "/api/vtexid/refreshtoken/webstore" and
set the Domain to the correct target domain (derived from req.url or
ctx.request.hostname) before adding them to ctx.response.headers; leave non-auth
cookies unchanged and ensure multiple Set-Cookie headers are preserved.
vtex/utils/segment.ts
Outdated
| const segmentFromCookie = vtex_segment ? parse(vtex_segment) : null; | ||
| console.log("setSegmentBag current priceTables", segmentFromCookie?.priceTables); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid spreading null; default to safe object and quiet logs
...segmentFromCookie will throw if null/undefined. Also reduce log noise.
- const segmentFromCookie = vtex_segment ? parse(vtex_segment) : null;
- console.log("setSegmentBag current priceTables", segmentFromCookie?.priceTables);
+ const parsed = vtex_segment ? (parse(vtex_segment) as Partial<Segment> | null) : null;
+ const segmentFromCookie = parsed ?? {};
+ if (parsed) console.debug("setSegmentBag priceTables", parsed.priceTables);📝 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.
| const segmentFromCookie = vtex_segment ? parse(vtex_segment) : null; | |
| console.log("setSegmentBag current priceTables", segmentFromCookie?.priceTables); | |
| const parsed = vtex_segment | |
| ? (parse(vtex_segment) as Partial<Segment> | null) | |
| : null; | |
| const segmentFromCookie = parsed ?? {}; | |
| if (parsed) console.debug("setSegmentBag priceTables", parsed.priceTables); |
🤖 Prompt for AI Agents
In vtex/utils/segment.ts around lines 169-171, the code currently allows
segmentFromCookie to be null which causes spreading it to throw and also noisy
console logging; change the default to a safe empty object (e.g. use
parse(vtex_segment) : {}), update any spread usage to use that safe object (or
use conditional spread only if truthy), and remove or replace the console.log
with a quieter logger.debug or no-op; also access priceTables via optional
chaining (segmentFromCookie?.priceTables) to avoid runtime errors.
… actions; streamline session management
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
vtex/actions/authentication/accessKeySignIn.ts (1)
12-14: Typo in JSDoc title ("AcessKey")Fix the user-facing title to “AccessKey”.
- * @title VTEX Integration - Authenticate with Email and AcessKey + * @title VTEX Integration - Authenticate with Email and AccessKey
♻️ Duplicate comments (5)
vtex/actions/session/editSession.ts (1)
20-22: Default publicProperties to {} to avoid runtime crash when props is missing.This action may be invoked without props; spreading
props.publicPropertieswould throw.const setCookiesSoFar = getSetCookies(ctx.response.headers); const { header: cookie } = buildCookieJar(req.headers, setCookiesSoFar); + const publicProperties = props?.publicProperties ?? {}; @@ body: { public: { - ...props.publicProperties, + ...publicProperties, }, },Also applies to: 31-33
vtex/actions/session/createSession.ts (1)
17-18: Good fix: forwarding upstream Set-Cookies within the same request.Merging ctx.response Set-Cookie with the incoming cookies via buildCookieJar resolves the earlier gap. Matches editSession pattern.
vtex/actions/authentication/accessKeySignIn.ts (1)
64-66: Remove sensitive debug logging of Set-CookieThese logs can leak credentials/PII. They were previously flagged.
- // TODO: REMOVE THIS AFTER TESTING - const setCookies = getSetCookies(ctx.response.headers); - console.log("accessKeySignIn ctx response headers", setCookies); + // debug logging of Set-Cookie removedvtex/actions/authentication/classicSignIn.ts (2)
69-71: Stop logging cookies (secrets leak).These logs dump Set-Cookie contents to stdout. Remove before merge.
- // TODO: REMOVE THIS AFTER TESTING - const setCookies = getSetCookies(ctx.response.headers); - console.log("classicSignIn ctx response headers", setCookies);
66-67: Enforce VTEX auth cookie Path when proxying Set-Cookie.Without forcing Path=/api/vtexid/refreshtoken/webstore for auth cookies, you can still hit the path-mismatch issue described in the PR. Override the auth cookies’ Path after proxying.
Apply within this hunk:
- proxySetCookie(response.headers, ctx.response.headers, req.url); - await ctx.invoke.vtex.actions.session.validateSession(); + // Proxy all cookies (domain rewrite) + proxySetCookie(response.headers, ctx.response.headers, req.url); + // Normalize Path for VTEX auth cookies to avoid route/path conflicts + const targetHost = new URL(req.url).hostname; + const VTEX_REFRESH_PATH = "/api/vtexid/refreshtoken/webstore"; + for (const c of getSetCookies(response.headers)) { + const needsAuthPath = + c.name === "vid_rt" || c.name.startsWith("VtexIdclientAutCookie"); + if (needsAuthPath) { + setCookie(ctx.response.headers, { + ...c, + domain: targetHost, + path: VTEX_REFRESH_PATH, + }); + } + } + await ctx.invoke.vtex.actions.session.validateSession();And add the missing import at the top:
-import { getSetCookies } from "std/http/cookie.ts"; +import { getSetCookies, setCookie } from "std/http/cookie.ts";
🧹 Nitpick comments (22)
vtex/middleware.ts (2)
8-8: Remove commented-out importThis stray commented import adds noise and can mislead future edits. Delete it.
-// import { parseCookie } from "./utils/vtexId.ts";
18-28: Avoid relying on request cookies for path-scoped VTEX tokensReminder: getCookies(req.headers) only includes cookies valid for the current request path. VTEX vid_rt often has path /api/vtexid/refreshtoken/webstore and won’t be present on routes like /api/vtexid/pub/logout. Keep this middleware scoped to IS/segment, and use the new cookie-jar/proxySetCookie helpers for refresh-token flows.
vtex/actions/session/editSession.ts (6)
24-29: Avoid forwarding arbitrary query params to VTEX; whitelist what you need.Prevents leaking unrelated client query params upstream.
- const searchParams = new URLSearchParams(url.search); - searchParams.set("items", items.join(",")); + const searchParams = new URLSearchParams(); + searchParams.set("items", items.join(","));
35-36: Only send Cookie header when non-empty.Prevents emitting
Cookie:with an empty value.- headers: { cookie }, + headers: cookie ? { cookie } : undefined,
39-41: Include response body in error message for faster triage.Speeds up debugging without changing behavior.
- if (!response.ok) { - throw new Error(`Failed to edit session: ${response.status}`); - } + if (!response.ok) { + const body = await response.text().catch(() => ""); + throw new Error(`Failed to edit session: ${response.status} ${response.statusText} — ${body?.slice(0, 512)}`); + }
43-43: Consider normalizing Set-Cookie path for VTEX refresh token (vid_rt).Logout path mismatches happen when
vid_rtis scoped to/api/vtexid/refreshtoken/webstore. If appropriate for your flows, extendproxySetCookieto optionally override cookie paths (only forvid_rt) to a broader path (e.g.,/) or a consistent VTEX path.Apply in utils (example):
- export const proxySetCookie = ( - from: Headers, - to: Headers, - toDomain?: URL | string, - ) => { + export const proxySetCookie = ( + from: Headers, + to: Headers, + toDomain?: URL | string, + opts?: { pathOverrides?: Record<string, string> }, + ) => { const newDomain = toDomain && new URL(toDomain); for (const cookie of getSetCookies(from)) { - const newCookie = newDomain + const overriddenPath = opts?.pathOverrides?.[cookie.name]; + const newCookie = newDomain ? { ...cookie, domain: newDomain.hostname, + ...(overriddenPath ? { path: overriddenPath } : {}), } : cookie; setCookie(to, newCookie); } };Then here:
- proxySetCookie(response.headers, ctx.response.headers, req.url); + proxySetCookie(response.headers, ctx.response.headers, req.url, { + pathOverrides: { vid_rt: "/" }, + });Please verify with VTEX docs/env that changing the
vid_rtpath does not break refresh flows.
5-9: Remove dead commented code.Keeps the file clean.
-// import { parseCookie } from "../../utils/vtexId.ts"; -// import { SEGMENT_COOKIE_NAME } from "../../utils/segment.ts"; -// import { setSegmentBag } from "../../utils/segment.ts"; @@ -// TODO: REMOVE THIS AFTER TESTING -// setSegmentBag(record, req, ctx);Also applies to: 44-46
47-47: Nit: drop unnecessary await.Minor readability improvement.
- return await response.json() as GetSessionResponse; + return response.json() as Promise<GetSessionResponse>;vtex/actions/session/createSession.ts (4)
20-29: Add timeout/abort to the upstream POST to avoid hanging requests.Guard the VCS call with AbortController and pass signal; clear the timer afterward.
@@ - const response = await vcs["POST /api/sessions"]({ - items: items.join(","), - }, { + const ac = new AbortController(); + const timer = setTimeout(() => ac.abort("createSession timeout (8s)"), 8000); + const response = await vcs["POST /api/sessions"]({ + items: items.join(","), + }, { body: { public: { - ...props.publicProperties, + ...props.publicProperties, }, }, - headers: { cookie }, + headers: { cookie }, + signal: ac.signal, }); + clearTimeout(timer);
31-33: Return richer errors (statusText + body snippet) to aid debugging.Include response text when !ok; keeps success path unchanged.
- if (!response.ok) { - throw new Error(`Failed to create session: ${response.status}`); - } + if (!response.ok) { + const errText = await response.text().catch(() => ""); + throw new Error( + `Failed to create session: ${response.status} ${response.statusText}${errText ? ` - ${errText.slice(0, 500)}` : ""}`, + ); + }
11-16: Harden against undefined props at runtime.Even if TS types guarantee presence, a defensive default prevents runtime crashes when called dynamically.
async function action( props: Props, req: Request, ctx: AppContext, ): Promise<GetSessionResponse> { const { vcs } = ctx; + const publicProperties = props?.publicProperties ?? {}; @@ body: { public: { - ...props.publicProperties, + ...publicProperties, }, },Also applies to: 23-26
18-18: Scope cookies sent upstream (allowlist VTEX auth/session cookies).Forwarding every browser cookie risks leaking unrelated app cookies to VTEX. Prefer an allowlist (e.g., vid_rt, VtexIdclientAutCookie, VtexSession, VtexIdclientAutCookie_${account}). If convenient, extend buildCookieJar to accept a filter.
Example utility change (in utils/cookies.ts):
export function buildCookieJar( reqHeaders: Headers, upstreamSetCookies: Cookie[], allow?: (name: string) => boolean, ): CookieJarResult { const incoming = getCookies(reqHeaders); const jar = new Map<string, Cookie>(); const accept = allow ?? (() => true); Object.entries(incoming).forEach(([name, value]) => { if (accept(name)) jar.set(name, { name, value }); }); upstreamSetCookies .filter((c) => c?.name && c.value !== undefined && accept(c.name)) .forEach((cookie) => jar.set(cookie.name, cookie)); ... }And here:
const { header: cookie } = buildCookieJar( req.headers, setCookiesSoFar, (n) => /^(vid_rt|Vtex(IdclientAutCookie|Session)(_|$))/i.test(n), );Also applies to: 28-28
vtex/actions/authentication/recoveryPassword.ts (2)
30-32: Name the upstream cookies source clearly.Minor:
startSetCookiesmay be confusing here (no startAuthentication call in this action). Rename toupstreamSetCookiesfor clarity.- const startSetCookies = getSetCookies(ctx.response.headers); - const { header: cookie } = buildCookieJar(req.headers, startSetCookies); + const upstreamSetCookies = getSetCookies(ctx.response.headers); + const { header: cookie } = buildCookieJar(req.headers, upstreamSetCookies);
55-56: Set Cookie header only when present; prefer canonical casing.Avoid sending an empty cookie header and use standard "Cookie" casing.
- cookie, + ...(cookie && { Cookie: cookie }),vtex/actions/authentication/resetPassword.ts (3)
32-34: Minor naming nit.For readability, mirror the terminology and rename to
upstreamSetCookies.- const startSetCookies = getSetCookies(ctx.response.headers); - const { header: cookie } = buildCookieJar(req.headers, startSetCookies); + const upstreamSetCookies = getSetCookies(ctx.response.headers); + const { header: cookie } = buildCookieJar(req.headers, upstreamSetCookies);
60-61: Guard and canonically case the Cookie header.Same rationale as in recoveryPassword.
- cookie, + ...(cookie && { Cookie: cookie }),
73-75: Also normalize refresh-token Path here (via cookies.ts).Once
proxySetCookieapplies the Path fix forvid_rt, this action will benefit automatically. No changes needed in this file after that.vtex/actions/authentication/sendEmailVerification.ts (1)
84-91: Set SameSite on VtexSessionToken cookieConsider explicitly setting sameSite: "Lax" to align with secure, httpOnly defaults and reduce CSRF risk.
setCookie(ctx.response.headers, { name: "VtexSessionToken", value: authenticationToken, httpOnly: true, maxAge: SESSION_TOKEN_EXPIRES, path: "/", secure: true, + sameSite: "Lax", });vtex/actions/authentication/refreshToken.ts (2)
1-1: Drop unused import after removing debug codegetSetCookies becomes unused once debug logs are removed.
-import { getCookies, getSetCookies } from "std/http/cookie.ts"; +import { getCookies } from "std/http/cookie.ts";
33-34: Prefer cookie-jar composition for consistencyUsing buildCookieJar keeps behavior uniform and allows merging any upstream Set-Cookie already proxied in this request (if any).
- headers: { - cookie: req.headers.get("cookie") || "", - }, + headers: (() => { + // merge incoming cookies with any Set-Cookie already proxied this request + const { getSetCookies } = await import("std/http/cookie.ts"); + const { buildCookieJar } = await import("../../utils/cookies.ts"); + const startSetCookies = getSetCookies(ctx.response.headers); + const { header } = buildCookieJar(req.headers, startSetCookies); + return { cookie: header }; + })(),vtex/actions/authentication/logout.ts (2)
27-33: Forwarding cookies is correct; consider passing returnUrl upstream (optional).Not required since you handle redirect locally, but passing returnUrl to VTEX won’t hurt and may keep audit trails consistent.
- const response = await vcsDeprecated["GET /api/vtexid/pub/logout"]({ - scope: account, - }, { + const response = await vcsDeprecated["GET /api/vtexid/pub/logout"]({ + scope: account, + // returnUrl, // optional: uncomment if you want VTEX-side awareness + }, {
35-39: Guard against non-JSON/204 responses (optional).Some accounts return 204; the change above handles this. If you prefer, wrap JSON parse in try/catch instead.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
vtex/actions/authentication/accessKeySignIn.ts(4 hunks)vtex/actions/authentication/classicSignIn.ts(5 hunks)vtex/actions/authentication/logout.ts(1 hunks)vtex/actions/authentication/recoveryPassword.ts(4 hunks)vtex/actions/authentication/refreshToken.ts(4 hunks)vtex/actions/authentication/resetPassword.ts(5 hunks)vtex/actions/authentication/sendEmailVerification.ts(5 hunks)vtex/actions/authentication/startAuthentication.ts(4 hunks)vtex/actions/session/createSession.ts(3 hunks)vtex/actions/session/editSession.ts(2 hunks)vtex/actions/session/validateSession.ts(1 hunks)vtex/middleware.ts(1 hunks)vtex/sections/Authentication/Logout.tsx(1 hunks)vtex/utils/client.ts(2 hunks)vtex/utils/cookies.ts(2 hunks)vtex/utils/login/getLoginCookies.ts(1 hunks)vtex/utils/login/setLoginCookies.ts(2 hunks)vtex/utils/segment.ts(3 hunks)vtex/utils/types.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- vtex/utils/login/getLoginCookies.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- vtex/actions/authentication/startAuthentication.ts
- vtex/actions/session/validateSession.ts
- vtex/utils/login/setLoginCookies.ts
- vtex/sections/Authentication/Logout.tsx
- vtex/utils/cookies.ts
- vtex/utils/segment.ts
🧰 Additional context used
🧬 Code graph analysis (10)
vtex/utils/client.ts (1)
vtex/utils/types.ts (1)
LogoutResponse(1878-1881)
vtex/actions/authentication/refreshToken.ts (1)
vtex/utils/cookies.ts (2)
REFRESH_TOKEN_COOKIE(74-74)proxySetCookie(13-30)
vtex/actions/session/editSession.ts (3)
vtex/utils/openapi/vcs.openapi.gen.ts (1)
GetSessionResponse(22176-22300)vtex/utils/cookies.ts (2)
buildCookieJar(42-70)proxySetCookie(13-30)vtex/utils/session.ts (1)
items(1-36)
vtex/actions/authentication/classicSignIn.ts (1)
vtex/utils/cookies.ts (2)
buildCookieJar(42-70)proxySetCookie(13-30)
vtex/actions/session/createSession.ts (3)
vtex/utils/openapi/vcs.openapi.gen.ts (1)
GetSessionResponse(22176-22300)vtex/utils/cookies.ts (1)
buildCookieJar(42-70)vtex/utils/session.ts (1)
items(1-36)
vtex/actions/authentication/logout.ts (3)
vtex/actions/session/validateSession.ts (1)
action(5-17)vtex/utils/types.ts (1)
LogoutResponse(1878-1881)vtex/utils/cookies.ts (1)
proxySetCookie(13-30)
vtex/actions/authentication/recoveryPassword.ts (1)
vtex/utils/cookies.ts (2)
buildCookieJar(42-70)proxySetCookie(13-30)
vtex/actions/authentication/accessKeySignIn.ts (1)
vtex/utils/cookies.ts (2)
buildCookieJar(42-70)proxySetCookie(13-30)
vtex/actions/authentication/sendEmailVerification.ts (1)
vtex/utils/cookies.ts (2)
buildCookieJar(42-70)proxySetCookie(13-30)
vtex/actions/authentication/resetPassword.ts (1)
vtex/utils/cookies.ts (2)
buildCookieJar(42-70)proxySetCookie(13-30)
🔇 Additional comments (13)
vtex/actions/session/createSession.ts (2)
35-35: LGTM: proper Set-Cookie proxying back to the client.Ensures session continuity on the browser side.
1-41: Sanity-check VTEX path‑scoped refresh-token cookie handlingVTEX refresh-token cookie is path-scoped at /api/vtexid/refreshtoken/webstore — ensure any action that depends on it either (a) merges upstream Set-Cookie into the outgoing request (use getSetCookies(ctx.response.headers) + buildCookieJar(req.headers, setCookiesSoFar) and proxySetCookie for responses) or (b) does not rely on the browser sending it for non-matching paths. Do not forward raw req.headers.get('cookie') to upstream without merging Set-Cookie.
Start with vtex/actions/session/createSession.ts (lines 1–41) and then search the repo for other VTEX auth/session calls (/api/sessions, /api/vtexid, /pub/logout). Automated repo search returned no files (ripgrep skipped files); re-run a repo-wide search for raw cookie forwarding and buildCookieJar usage and attach the output so I can re-verify.
vtex/actions/authentication/recoveryPassword.ts (1)
5-5: Good move centralizing cookie handling.Switch to
buildCookieJar/proxySetCookieimproves consistency and reduces duplication.vtex/actions/authentication/resetPassword.ts (2)
5-5: Cookie-jar adoption here looks good.Consistent with other auth flows and aligns with the PR direction.
19-19: Param rename is appropriate.Using
reqenables domain rewriting inproxySetCookie.vtex/actions/authentication/sendEmailVerification.ts (2)
32-34: Cookie jar build looks correctMerges upstream Set-Cookie with incoming request cookies to compose the Cookie header. Matches the new pattern across the PR.
80-81: Domain rewrite on Set-Cookie proxyingPassing req.url to proxySetCookie to rewrite the domain is appropriate here; path is preserved as sent by VTEX.
vtex/actions/authentication/accessKeySignIn.ts (2)
27-29: Good adoption of cookie-jar patternMerging upstream Set-Cookie with incoming cookies and sending as Cookie header is correct and consistent.
Also applies to: 47-50
61-63: Post-auth cookie proxy + session validationProxying Set-Cookie and then validating the session is a solid sequence.
vtex/actions/authentication/refreshToken.ts (1)
23-25: No change required — refresh endpoint already matches vid_rt cookie pathvtex/utils/login/setLoginCookies.ts sets the cookie path to "/api/vtexid/refreshtoken/webstore" and the refresh action is bound to POST /api/vtexid/refreshtoken/webstore (see vtex/utils/client.ts and vtex/actions/authentication/refreshToken.ts), so the browser will send vid_rt to this route.
vtex/actions/authentication/classicSignIn.ts (2)
45-57: Request construction looks solid.Headers/body are correct; cookie jar is forwarded properly.
30-32: Jar source OK — startAuthentication proxies Set-Cookie into ctx.response.headers.
proxySetCookie(response.headers, ctx.response.headers, req.url) present in vtex/actions/authentication/startAuthentication.ts:45; cookieHeader will receive upstream Set-Cookie.vtex/actions/authentication/logout.ts (1)
21-24: Action shape looks good.Signature and types align with utils/client.ts.
| const data = await response.json(); | ||
| proxySetCookie(response.headers, ctx.response.headers, req.url); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Normalize auth cookie Path on logout as well.
Logout relies on Set-Cookie expirations; if Path isn’t the refresh-token path, cookies may linger for that scope. Mirror the sign-in fix here.
Apply within this hunk:
- const data = await response.json();
- proxySetCookie(response.headers, ctx.response.headers, req.url);
+ const contentType = response.headers.get("content-type") ?? "";
+ const data: LogoutResponse =
+ contentType.includes("application/json")
+ ? await response.json()
+ : { cookieName: null, accountCookieName: null };
+ // Proxy all cookies (domain rewrite)
+ proxySetCookie(response.headers, ctx.response.headers, req.url);
+ // Normalize Path for VTEX auth cookies
+ const targetHost = new URL(req.url).hostname;
+ const VTEX_REFRESH_PATH = "/api/vtexid/refreshtoken/webstore";
+ for (const c of getSetCookies(response.headers)) {
+ const needsAuthPath =
+ c.name === "vid_rt" || c.name.startsWith("VtexIdclientAutCookie");
+ if (needsAuthPath) {
+ setCookie(ctx.response.headers, {
+ ...c,
+ domain: targetHost,
+ path: VTEX_REFRESH_PATH,
+ });
+ }
+ }Add imports:
-// import { getCookies, getSetCookies } from "std/http/cookie.ts";
+import { getSetCookies, setCookie } from "std/http/cookie.ts";📝 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.
| const data = await response.json(); | |
| proxySetCookie(response.headers, ctx.response.headers, req.url); | |
| import { getSetCookies, setCookie } from "std/http/cookie.ts"; | |
| const contentType = response.headers.get("content-type") ?? ""; | |
| const data: LogoutResponse = | |
| contentType.includes("application/json") | |
| ? await response.json() | |
| : { cookieName: null, accountCookieName: null }; | |
| // Proxy all cookies (domain rewrite) | |
| proxySetCookie(response.headers, ctx.response.headers, req.url); | |
| // Normalize Path for VTEX auth cookies | |
| const targetHost = new URL(req.url).hostname; | |
| const VTEX_REFRESH_PATH = "/api/vtexid/refreshtoken/webstore"; | |
| for (const c of getSetCookies(response.headers)) { | |
| const needsAuthPath = | |
| c.name === "vid_rt" || c.name.startsWith("VtexIdclientAutCookie"); | |
| if (needsAuthPath) { | |
| setCookie(ctx.response.headers, { | |
| ...c, | |
| domain: targetHost, | |
| path: VTEX_REFRESH_PATH, | |
| }); | |
| } | |
| } |
🤖 Prompt for AI Agents
In vtex/actions/authentication/logout.ts around lines 41 to 43, the logout flow
currently calls proxySetCookie(response.headers, ctx.response.headers, req.url)
but does not normalize the cookie Path, so cookies may persist for the wrong
scope; mirror the sign-in fix by updating the proxySetCookie call to include the
refresh-token path (e.g. proxySetCookie(response.headers, ctx.response.headers,
req.url, getRefreshTokenPath(req.url)) or by normalizing the Set-Cookie Path
after proxying using normalizeCookiePath on ctx.response.headers['set-cookie']),
and add the necessary imports at the top of the file (import getRefreshTokenPath
and/or normalizeCookiePath from the auth/cookie utilities used in sign-in).
| return data; | ||
| } |
There was a problem hiding this comment.
Prevent open redirects in returnUrl.
returnUrl appears user-controlled (derived from the request in the section). Restrict to same-origin paths to avoid phishing/open-redirect.
- redirect(returnUrl);
- return data;
+ const safeLocation = (() => {
+ try {
+ if (!returnUrl) return "/";
+ if (returnUrl.startsWith("/")) return returnUrl.startsWith("//") ? "/" : returnUrl;
+ const dest = new URL(returnUrl, req.url);
+ const origin = new URL(req.url).origin;
+ return dest.origin === origin ? (dest.pathname + dest.search + dest.hash) : "/";
+ } catch {
+ return "/";
+ }
+ })();
+ redirect(safeLocation);
+ return data;📝 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.
| redirect(returnUrl); | |
| return data; | |
| const safeLocation = (() => { | |
| try { | |
| if (!returnUrl) return "/"; | |
| if (returnUrl.startsWith("/")) return returnUrl.startsWith("//") ? "/" : returnUrl; | |
| const dest = new URL(returnUrl, req.url); | |
| const origin = new URL(req.url).origin; | |
| return dest.origin === origin ? (dest.pathname + dest.search + dest.hash) : "/"; | |
| } catch { | |
| return "/"; | |
| } | |
| })(); | |
| redirect(safeLocation); | |
| return data; |
🤖 Prompt for AI Agents
In vtex/actions/authentication/logout.ts around lines 47-48, the redirect uses a
user-controlled returnUrl which enables open-redirects; ensure returnUrl is
validated and restricted to same-origin paths only by treating only relative
paths as safe (e.g., require it to start with a single '/' and not with '//' and
disallow any ':' or protocol components), or parse it with URL and compare
origin to the current origin; if validation fails, replace with a safe default
path before calling redirect.
| @@ -1,5 +1,7 @@ | |||
| import { setCookie } from "std/http/mod.ts"; | |||
There was a problem hiding this comment.
Fix wrong std import for setCookie
setCookie is provided by std/http/cookie.ts, not std/http/mod.ts. Importing from mod.ts will fail at runtime/build.
Apply:
-import { setCookie } from "std/http/mod.ts";
+import { setCookie, getSetCookies } from "std/http/cookie.ts";
-import { getSetCookies } from "std/http/cookie.ts";Also applies to: 3-4
🤖 Prompt for AI Agents
In vtex/actions/authentication/sendEmailVerification.ts at line 1 (and similarly
lines 3-4), the import path for setCookie is incorrect (std/http/mod.ts); change
the import to pull setCookie from std/http/cookie.ts instead. Update the import
statement to import { setCookie } from "std/http/cookie.ts" and verify any other
std/http imports reference the correct submodule paths.
| "GET /api/vtexid/pub/logout": { | ||
| searchParams: { | ||
| scope?: string; | ||
| returnUrl?: string; | ||
| }; | ||
| response: LogoutResponse; | ||
| }; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Confirm VTEX logout contract (query params + JSON shape).
Scope/returnUrl params and the LogoutResponse {cookieName, accountCookieName} look plausible but can vary across accounts. Verify against upstream docs or a live store.
🌐 Web query:
Does VTEX endpoint GET /api/vtexid/pub/logout accept scope and returnUrl query parameters, and does it return a JSON body with cookieName and accountCookieName or 204 No Content?
💡 Result:
Short answer:
- Yes — the VTEX logout URL /api/vtexid/pub/logout is commonly used with scope and returnUrl query parameters (e.g. ?scope=...&returnUrl=...). [1][2]
- The endpoint does not document returning a JSON body with cookieName/accountCookieName; logout is handled by clearing VTEX ID cookies and typically results in a redirect (to returnUrl) or no response body (no content) when invoked as an API. VTEX docs/forums show use of returnUrl for redirection and cookie-based clearing, not a JSON payload. [1][2][3]
Sources
- VTEX Login app docs (returnUrl behavior). [developers.vtex.com]. [1]
- VTEX Community examples calling /api/vtexid/pub/logout?scope=...&returnUrl=... (community thread). [2]
- VTEX guides on auth cookies / authenticated endpoints (describes cookie handling; no JSON logout body documented). [3]
If you need, I can call the endpoint against a store (or inspect a specific VTEX account) to show the exact HTTP status/response behavior for that environment.
VTEX logout: accepts scope & returnUrl; remove JSON LogoutResponse typing
Docs/community show GET /api/vtexid/pub/logout is used with ?scope=...&returnUrl=... and clears VTEX ID cookies, redirecting to returnUrl or returning 204 No Content — no documented JSON body with cookieName/accountCookieName.
File: vtex/utils/client.ts (lines 311–317)
- Replace response: LogoutResponse with a void/204 response (or a type modeling redirect behavior).
- Update callers that expect cookieName/accountCookieName to instead handle cookie clearing/redirect flow.
🤖 Prompt for AI Agents
In vtex/utils/client.ts around lines 311–317 the OpenAPI mapping incorrectly
types "GET /api/vtexid/pub/logout" as returning LogoutResponse; change the
response type to reflect no JSON body (void or a 204 response / redirect
semantics) and remove any properties like cookieName/accountCookieName from that
response type. Update any call sites that consume cookieName/accountCookieName
to instead treat logout as a side-effect that clears VTEX ID cookies and either
follows a redirect (returnUrl) or handles a 204 status — remove assumptions
about a JSON body and handle HTTP status/Location header accordingly.
| export interface LogoutResponse { | ||
| cookieName: string | null; | ||
| accountCookieName: string | null; | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Confirm VTEX logout response shape before committing to LogoutResponse.
Many VTEX auth endpoints respond with Set-Cookie side-effects and little/no JSON body. Please verify that /api/vtexid/pub/logout returns a JSON payload with cookieName and accountCookieName; otherwise, model this as void (and rely solely on Set-Cookie), or make fields optional.
You can verify via docs or a quick cURL against a test account.
🌐 Web query:
Does VTEX endpoint GET /api/vtexid/pub/logout (with scope=store) return a JSON body? If yes, what is its exact schema?
💡 Result:
Short answer: No — the GET /api/vtexid/pub/logout?scope=store does not return a JSON body. It performs a browser redirect (clears VTEXID cookies and redirects to the provided returnUrl or to VTEX ID UI), so you should expect an HTTP redirect/HTML response flow rather than a JSON payload. See VTEX community examples showing usage via window.location.assign and the VTEX ID authentication docs describing the cookie-based logout/redirect behavior. [1][2]
Sources:
- VTEX Community example using /api/vtexid/pub/logout with returnUrl (redirect from frontend). [1]
- VTEX docs on VTEX ID / authentication and user token (describes cookie-based auth/logout behavior). [2]
Logout returns a redirect/no JSON — update the type.
GET /api/vtexid/pub/logout?scope=store performs a redirect and clears VTEXID cookies; it does not return a JSON body. Update vtex/utils/types.ts (lines 1878–1881) to either use void for the response or make the fields optional (cookieName?: string | null; accountCookieName?: string | null).
🤖 Prompt for AI Agents
In vtex/utils/types.ts around lines 1878 to 1881, the LogoutResponse type
assumes a JSON body but the logout endpoint performs a redirect and returns no
JSON; change the type to reflect that by either making fields optional
(cookieName?: string | null; accountCookieName?: string | null) or replacing the
type with void/undefined as appropriate for callers. Update any code sites that
consume LogoutResponse to handle undefined/void responses (guard optional fields
or treat the call as not returning JSON).
…ate cookie management for sign-in and logout
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
vtex/actions/authentication/classicSignIn.ts (2)
70-72: Enforce vid_rt Path=/api/vtexid/refreshtoken/webstore when proxying (don’t rely on a follow-up override).Set Path at proxy time so the browser stores the cookie scoped to the refresh endpoint as intended.
- proxySetCookie(response.headers, ctx.response.headers, req.url); + const targetHost = new URL(req.url).hostname; + const VTEX_REFRESH_PATH = "/api/vtexid/refreshtoken/webstore"; + for (const c of getSetCookies(response.headers)) { + setCookie(ctx.response.headers, { + ...c, + domain: targetHost, + path: c.name === REFRESH_TOKEN_COOKIE ? VTEX_REFRESH_PATH : c.path, + }); + }
73-82: Do not widen the refresh token to Path="/". This contradicts the PR goal and weakens cookie scope.Overriding vid_rt to “/” increases exposure, can cause duplicate-name conflicts, and undermines logout/session behaviors tied to the refresh path.
Apply this diff to remove the override:
- // TODO: REMOVE THIS AFTER TESTING AND VALIDATE IF NEEDED - const setCookies = getSetCookies(ctx.response.headers); - for (const cookie of setCookies) { - if (cookie.name === REFRESH_TOKEN_COOKIE) { - setCookie(ctx.response.headers, { - ...cookie, - path: "/", // default path is /api/vtexid/refreshtoken/webstore, but browser dont send to backend headers - }); - } - }If you must adjust paths, set it explicitly to “/api/vtexid/refreshtoken/webstore” at proxy time as shown above.
vtex/actions/authentication/recoveryPassword.ts (1)
74-83: Fix refresh-token Path at the source; avoid duplicate Set-Cookie for vid_rt.Re-setting vid_rt with Path="/" after proxying creates two cookies with the same name but different paths. Browsers can store both and send them in different orders, causing flaky auth/logout flows. Move the Path normalization into proxySetCookie (in vtex/utils/cookies.ts) and remove this local override.
Apply this diff to remove the local override:
- // TODO: REMOVE THIS AFTER TESTING AND VALIDATE IF NEEDED - const setCookies = getSetCookies(ctx.response.headers); - for (const cookie of setCookies) { - if (cookie.name === REFRESH_TOKEN_COOKIE) { - setCookie(ctx.response.headers, { - ...cookie, - path: "/", // default path is /api/vtexid/refreshtoken/webstore, but browser dont send to backend headers - }); - } - }Then centralize the fix in vtex/utils/cookies.ts:
// vtex/utils/cookies.ts import { getSetCookies, setCookie, Cookie } from "std/http/cookie.ts"; export const REFRESH_TOKEN_COOKIE = "vid_rt"; export const proxySetCookie = ( from: Headers, to: Headers, toDomain?: URL | string, ) => { const newDomain = toDomain && new URL(toDomain); for (const cookie of getSetCookies(from)) { const patched: Cookie = newDomain ? { ...cookie, domain: newDomain.hostname } : { ...cookie }; if (patched.name === REFRESH_TOKEN_COOKIE) { patched.path = "/"; // normalize refresh token path } setCookie(to, patched); } };
🧹 Nitpick comments (7)
vtex/actions/authentication/classicSignIn.ts (1)
49-61: Optionally avoid sending an empty Cookie header.Minor: omit the Cookie header when cookieHeader is empty to reduce noise.
- const response = await vcsDeprecated[ + const headers: HeadersInit = { + "Content-Type": "application/x-www-form-urlencoded", + Accept: "application/json", + }; + if (cookieHeader) { + (headers as Record<string, string>).cookie = cookieHeader; + } + const response = await vcsDeprecated[ "POST /api/vtexid/pub/authentication/classic/validate" ]( {}, { - body: urlencoded, - headers: { - "Content-Type": "application/x-www-form-urlencoded", - Accept: "application/json", - cookie: cookieHeader, - }, + body: urlencoded, + headers, }, );vtex/actions/authentication/recoveryPassword.ts (3)
1-1: Clean up unused imports after centralizing the Path fix.Once the local re-set is removed, setCookie and REFRESH_TOKEN_COOKIE are no longer used here.
-import { getCookies, getSetCookies, setCookie } from "std/http/cookie.ts"; +import { getCookies, getSetCookies } from "std/http/cookie.ts";import { buildCookieJar, proxySetCookie, - REFRESH_TOKEN_COOKIE, } from "../../utils/cookies.ts";Also applies to: 5-9
34-36: Scope the outbound Cookie header to VTEX cookies only.buildCookieJar merges all incoming cookies plus any upstream Set-Cookie without domain/path filtering. To reduce leakage and shrink headers, consider whitelisting known VTEX cookies when composing the outbound header (apply in vtex/utils/cookies.ts).
Example:
const ALLOWED = new Set([ "VtexSessionToken", "VtexIdclientAutCookie", REFRESH_TOKEN_COOKIE, // add others you truly need ]); const cookies = Array.from(jar.values()).filter((c) => ALLOWED.has(c.name));
59-59: Minor: use conventional header casing.Not required, but using "Cookie" improves readability vs shorthand property.
- cookie, + "Cookie": cookie,vtex/actions/authentication/resetPassword.ts (1)
61-66: Use explicit "Cookie" header key (nit)Being explicit avoids confusion with the cookie variable name.
Apply:
- cookie, + "Cookie": cookie,vtex/actions/authentication/accessKeySignIn.ts (2)
1-1: DropsetCookieimport once the temp block is removed
setCookieis only used by the testing block below. After replacing that logic (see comments on Lines 68-77), remove this import to avoid unused symbols.-import { getCookies, getSetCookies, setCookie } from "std/http/cookie.ts"; +import { getCookies, getSetCookies } from "std/http/cookie.ts";
51-54: Use canonicalCookieheader casingHeader names are case-insensitive, but prefer
Cookiefor consistency with other calls and to avoid surprises in custom clients.headers: { "Content-Type": "application/x-www-form-urlencoded", "Accept": "application/json", - cookie, + Cookie: cookie, },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
vtex/actions/authentication/accessKeySignIn.ts(4 hunks)vtex/actions/authentication/classicSignIn.ts(5 hunks)vtex/actions/authentication/logout.ts(1 hunks)vtex/actions/authentication/recoveryPassword.ts(4 hunks)vtex/actions/authentication/resetPassword.ts(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- vtex/actions/authentication/logout.ts
🧰 Additional context used
🧬 Code graph analysis (4)
vtex/actions/authentication/resetPassword.ts (1)
vtex/utils/cookies.ts (3)
buildCookieJar(42-70)proxySetCookie(13-30)REFRESH_TOKEN_COOKIE(74-74)
vtex/actions/authentication/recoveryPassword.ts (1)
vtex/utils/cookies.ts (3)
buildCookieJar(42-70)proxySetCookie(13-30)REFRESH_TOKEN_COOKIE(74-74)
vtex/actions/authentication/accessKeySignIn.ts (1)
vtex/utils/cookies.ts (3)
buildCookieJar(42-70)proxySetCookie(13-30)REFRESH_TOKEN_COOKIE(74-74)
vtex/actions/authentication/classicSignIn.ts (1)
vtex/utils/cookies.ts (3)
buildCookieJar(42-70)proxySetCookie(13-30)REFRESH_TOKEN_COOKIE(74-74)
🔇 Additional comments (11)
vtex/actions/authentication/classicSignIn.ts (3)
3-9: Imports look correct and necessary.The added cookie helpers and constants match their usage below.
22-22: Param rename to req is appropriate.Used later for domain rewrite; no issues spotted.
34-36: Cookie jar construction is correct.Merges upstream Set-Cookie with incoming request cookies to produce a valid Cookie header.
vtex/actions/authentication/recoveryPassword.ts (1)
71-73: Verify domain rewrite target used by proxySetCookie.Confirm that req.url always reflects the shopper-facing origin (e.g., https://www.loja.com). If actions can run under preview/staging hosts, consider passing a canonical public URL instead to ensure the cookie Domain matches the storefront.
vtex/actions/authentication/resetPassword.ts (4)
4-9: Cookie utilities import looks goodBrings in exactly what’s used and centralizes around utils. LGTM.
23-23: Param rename to req is correctMatches downstream usage (req.headers, req.url). No issues.
77-79: Verify domain override targetproxySetCookie(..., req.url) forces Domain to req.url’s hostname. On preview/localhost or multi-domain setups, this may differ from the store domain that originally set vid_rt. Confirm this is the intended target domain for clients.
36-38: Confirmed: upstream Set-Cookie is proxied into ctx.response.headers
startAuthentication.ts calls proxySetCookie(response.headers, ctx.response.headers, req.url) (line 45), so ctx.response.headers will contain the upstream Set-Cookie.vtex/actions/authentication/accessKeySignIn.ts (3)
4-8: Good shift to cookie utilitiesUsing
buildCookieJar,proxySetCookie, and theREFRESH_TOKEN_COOKIEconstant aligns this flow with the shared cookie handling. No concerns.
65-67: LGTM: Propagate Set-Cookie then validate the sessionOrder is right: proxy cookies first so
validateSession()can see them viactx.response.headers. 👍
31-33: Verify vid_rt is proxied into ctx.response.headers for all entry pathsJar composition is correct; vtex/actions/authentication/accessKeySignIn.ts (lines 31–33) merges ctx.response.headers into the cookie jar but assumes vid_rt was proxied earlier — confirm earlier middleware/proxy injects vid_rt for every path invoking this action.
Run (from repo root):
rg -nC2 --hidden -S 'accessKeySignIn' -g '!**/node_modules/**' rg -nC2 --hidden -S -F 'proxySetCookie(response.headers, ctx.response.headers' -g '!**/node_modules/**' rg -nC2 --hidden -S 'vid_rt' -g '!**/node_modules/**' rg -nC2 --hidden -S 'buildCookieJar' -g '!**/node_modules/**' rg -nC2 --hidden -S 'getSetCookies' -g '!**/node_modules/**' rg -nC2 --hidden -S -i 'set-cookie' -g '!**/node_modules/**' # if you see "No files were searched", run: rg --debug 'accessKeySignIn'
| // TODO: REMOVE THIS AFTER TESTING AND VALIDATE IF NEEDED | ||
| const setCookies = getSetCookies(ctx.response.headers); | ||
| for (const cookie of setCookies) { | ||
| if (cookie.name === REFRESH_TOKEN_COOKIE) { | ||
| setCookie(ctx.response.headers, { | ||
| ...cookie, | ||
| path: "/", // default path is /api/vtexid/refreshtoken/webstore, but browser dont send to backend headers | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid duplicate vid_rt cookies with different paths
Re-issuing vid_rt at "/" without expiring the legacy path cookie can yield two vid_rt cookies. Depending on server parsing order, this can break logout/session flows. Expire the old-path cookie first, then set the root-scoped one.
Apply:
- // TODO: REMOVE THIS AFTER TESTING AND VALIDATE IF NEEDED
- const setCookies = getSetCookies(ctx.response.headers);
- for (const cookie of setCookies) {
- if (cookie.name === REFRESH_TOKEN_COOKIE) {
- setCookie(ctx.response.headers, {
- ...cookie,
- path: "/", // default path is /api/vtexid/refreshtoken/webstore, but browser dont send to backend headers
- });
- }
- }
+ // TODO: remove once stabilized. Ensure we don't keep duplicate vid_rt cookies with different paths.
+ const setCookies = getSetCookies(ctx.response.headers);
+ const rt = setCookies.find((c) => c.name === REFRESH_TOKEN_COOKIE);
+ if (rt) {
+ // Expire the legacy path-scoped cookie to avoid duplicates (e.g., /api/vtexid/refreshtoken/webstore)
+ if (rt.path && rt.path !== "/") {
+ setCookie(ctx.response.headers, {
+ name: rt.name,
+ value: "",
+ domain: rt.domain,
+ path: rt.path,
+ secure: rt.secure,
+ httpOnly: rt.httpOnly,
+ sameSite: rt.sameSite,
+ maxAge: 0,
+ expires: new Date(0),
+ });
+ }
+ // Re-issue on the root path so the browser sends it broadly
+ setCookie(ctx.response.headers, {
+ ...rt,
+ path: "/",
+ });
+ }📝 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.
| // TODO: REMOVE THIS AFTER TESTING AND VALIDATE IF NEEDED | |
| const setCookies = getSetCookies(ctx.response.headers); | |
| for (const cookie of setCookies) { | |
| if (cookie.name === REFRESH_TOKEN_COOKIE) { | |
| setCookie(ctx.response.headers, { | |
| ...cookie, | |
| path: "/", // default path is /api/vtexid/refreshtoken/webstore, but browser dont send to backend headers | |
| }); | |
| } | |
| } | |
| // TODO: remove once stabilized. Ensure we don't keep duplicate vid_rt cookies with different paths. | |
| const setCookies = getSetCookies(ctx.response.headers); | |
| const rt = setCookies.find((c) => c.name === REFRESH_TOKEN_COOKIE); | |
| if (rt) { | |
| // Expire the legacy path-scoped cookie to avoid duplicates (e.g., /api/vtexid/refreshtoken/webstore) | |
| if (rt.path && rt.path !== "/") { | |
| setCookie(ctx.response.headers, { | |
| name: rt.name, | |
| value: "", | |
| domain: rt.domain, | |
| path: rt.path, | |
| secure: rt.secure, | |
| httpOnly: rt.httpOnly, | |
| sameSite: rt.sameSite, | |
| maxAge: 0, | |
| expires: new Date(0), | |
| }); | |
| } | |
| // Re-issue on the root path so the browser sends it broadly | |
| setCookie(ctx.response.headers, { | |
| ...rt, | |
| path: "/", | |
| }); | |
| } |
🤖 Prompt for AI Agents
In vtex/actions/authentication/resetPassword.ts around lines 80 to 89, the code
unconditionally sets a new REFRESH_TOKEN_COOKIE at path "/" which can create two
vid_rt cookies (legacy path and "/") and break session flows; update the
sequence to first expire the legacy-path cookie by issuing a Set-Cookie for the
same cookie name with the legacy path (e.g.,
"/api/vtexid/refreshtoken/webstore") and an immediate past expires/max-age=0,
then set the root-scoped cookie at path "/" as currently done, ensuring both
operations use setCookie on ctx.response.headers so the old cookie is removed by
browsers before the new root-scoped cookie is accepted.
…icated due to the deco's native segment
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
vtex/utils/cookies.ts (1)
14-31: Add a transform hook to proxySetCookie to safely normalize cookies (Path/domain)Centralize Path normalization (e.g., vid_rt → /api/vtexid) to avoid per-callsite rewrites and duplicates.
Apply this diff:
+export type CookieTransform = (cookie: Cookie) => Cookie | null; + export const proxySetCookie = ( from: Headers, to: Headers, toDomain?: URL | string, -) => { + transform?: CookieTransform, +) => { const newDomain = toDomain && new URL(toDomain); for (const cookie of getSetCookies(from)) { - const newCookie = newDomain - ? { - ...cookie, - domain: newDomain.hostname, - } - : cookie; - - setCookie(to, newCookie); + let newCookie = newDomain + ? { ...cookie, domain: newDomain.hostname } + : { ...cookie }; + + if (transform) { + const maybe = transform(newCookie); + if (!maybe) continue; + newCookie = maybe; + } + + setCookie(to, newCookie); } };vtex/actions/session/editSession.ts (1)
15-20: Harden against missing props to avoid runtime crashvalidateSession may invoke editSession without props. Accessing props.publicProperties will throw. Default to {}.
Apply:
async function action( - props: Props, + props: Props, req: Request, ctx: AppContext, ): Promise<GetSessionResponse> { const { vcs } = ctx; const setCookiesSoFar = getSetCookies(ctx.response.headers); const { header: cookie } = buildCookieJar(req.headers, setCookiesSoFar); + const publicProperties = props?.publicProperties ?? {}; @@ body: { public: { - ...props.publicProperties, + ...publicProperties, }, },Also applies to: 31-34
🧹 Nitpick comments (8)
vtex/actions/authentication/accessKeySignIn.ts (1)
31-33: Be cautious merging upstream Set-Cookie into outbound Cookie headerbuildCookieJar merges upstream Set-Cookie into the Cookie header ignoring Path/Domain, which can send cookies the browser wouldn’t normally send. Prefer an allowlist (e.g., VTEX auth/session cookies) to reduce risk.
Apply this minimal pattern:
-const { header: cookie } = buildCookieJar(req.headers, startSetCookies); +const jar = buildCookieJar(req.headers, startSetCookies); +const allow = new Set([REFRESH_TOKEN_COOKIE, "VtexSessionToken", "VtexIdclientAutCookie"]); +const cookie = jar.detailed + .filter((c) => c.name && allow.has(c.name)) + .map((c) => `${c.name}=${c.value}`) + .join("; ");vtex/utils/cookies.ts (2)
33-56: Preserve attributes when forwarding segment cookieFor
SEGMENT_COOKIE_NAME, attributes like sameSite/expires/maxAge should be preserved; only override what’s required.Apply:
- for (const cookie of getSetCookies(from)) { - const newCookie = cookie.name === SEGMENT_COOKIE_NAME - ? { - value: cookie.value, - name: cookie.name, - path: "/", - secure: true, - httpOnly: true, - } - : { - ...cookie, - domain: newDomain.hostname, - }; - setCookie(to, newCookie); - } + for (const cookie of getSetCookies(from)) { + const newCookie = cookie.name === SEGMENT_COOKIE_NAME + ? { ...cookie, domain: newDomain.hostname, path: "/", secure: true, httpOnly: true } + : { ...cookie, domain: newDomain.hostname }; + setCookie(to, newCookie); + }
68-96: Optionally restrict buildCookieJar to an allowlistMerging all cookies can leak or misapply unrelated cookies. Consider allowing only VTEX auth/session cookies.
vtex/actions/authentication/recoveryPassword.ts (1)
34-36: Limit cookie merge to relevant namesSame note as accessKeySignIn: prefer an allowlist to avoid propagating cookies the browser wouldn’t send.
Apply:
-const { header: cookie } = buildCookieJar(req.headers, startSetCookies); +const jar = buildCookieJar(req.headers, startSetCookies); +const allow = new Set([REFRESH_TOKEN_COOKIE, "VtexSessionToken", "VtexIdclientAutCookie"]); +const cookie = jar.detailed + .filter((c) => c.name && allow.has(c.name)) + .map((c) => `${c.name}=${c.value}`) + .join("; ");vtex/actions/authentication/startAuthentication.ts (1)
4-4: Import the refresh token constant for transform usagePrepare to normalize the refresh-token Path centrally.
Apply:
-import { proxySetCookie } from "../../utils/cookies.ts"; +import { proxySetCookie, REFRESH_TOKEN_COOKIE } from "../../utils/cookies.ts";vtex/actions/authentication/refreshToken.ts (2)
30-33: Unify header building with cookie jar (consistency + within-request Set-Cookie)Forward upstream Set-Cookies emitted earlier in the same request; align with other actions.
Apply:
-import { deleteCookie, getCookies } from "std/http/cookie.ts"; +import { deleteCookie, getCookies, getSetCookies } from "std/http/cookie.ts"; @@ -import { REFRESH_TOKEN_COOKIE } from "../../utils/cookies.ts"; -import { proxySetCookie } from "../../utils/cookies.ts"; +import { buildCookieJar, REFRESH_TOKEN_COOKIE, proxySetCookie } from "../../utils/cookies.ts"; @@ - const cookies = getCookies(req.headers); + const cookies = getCookies(req.headers); + const startSetCookies = getSetCookies(ctx.response.headers); + const { header: cookie } = buildCookieJar(req.headers, startSetCookies); @@ - headers: { - cookie: req.headers.get("cookie") || "", - }, + headers: { cookie },Also applies to: 41-43, 1-1, 3-4
10-17: Consider enum for status or use backend values directlyOptional: define STATUS as a const enum or use backend literals to avoid drift.
vtex/actions/authentication/resetPassword.ts (1)
77-79: Centralize refresh-token path normalizationThis pattern repeats across actions. Extract a helper (e.g., normalizeRefreshTokenCookiePath(toHeaders, targetPath)) to reduce drift.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
vtex/actions/authentication/accessKeySignIn.ts(4 hunks)vtex/actions/authentication/classicSignIn.ts(5 hunks)vtex/actions/authentication/logout.ts(1 hunks)vtex/actions/authentication/recoveryPassword.ts(4 hunks)vtex/actions/authentication/refreshToken.ts(4 hunks)vtex/actions/authentication/resetPassword.ts(5 hunks)vtex/actions/authentication/sendEmailVerification.ts(5 hunks)vtex/actions/authentication/startAuthentication.ts(4 hunks)vtex/actions/session/createSession.ts(2 hunks)vtex/actions/session/editSession.ts(2 hunks)vtex/utils/cookies.ts(2 hunks)vtex/utils/types.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (11)
vtex/actions/authentication/classicSignIn.ts (1)
vtex/utils/cookies.ts (3)
buildCookieJar(68-96)proxySetCookie(14-31)REFRESH_TOKEN_COOKIE(100-100)
vtex/actions/authentication/resetPassword.ts (1)
vtex/utils/cookies.ts (3)
buildCookieJar(68-96)proxySetCookie(14-31)REFRESH_TOKEN_COOKIE(100-100)
vtex/actions/session/createSession.ts (3)
vtex/utils/openapi/vcs.openapi.gen.ts (1)
GetSessionResponse(22176-22300)vtex/utils/cookies.ts (2)
buildCookieJar(68-96)setCookiesFromSession(33-56)vtex/utils/session.ts (1)
items(1-36)
vtex/actions/authentication/startAuthentication.ts (1)
vtex/utils/cookies.ts (1)
proxySetCookie(14-31)
vtex/actions/authentication/sendEmailVerification.ts (1)
vtex/utils/cookies.ts (2)
buildCookieJar(68-96)proxySetCookie(14-31)
vtex/actions/session/editSession.ts (3)
vtex/utils/openapi/vcs.openapi.gen.ts (1)
GetSessionResponse(22176-22300)vtex/utils/cookies.ts (2)
buildCookieJar(68-96)setCookiesFromSession(33-56)vtex/utils/session.ts (1)
items(1-36)
vtex/actions/authentication/logout.ts (3)
vtex/actions/session/validateSession.ts (1)
action(5-17)vtex/utils/types.ts (1)
LogoutResponse(1886-1889)vtex/utils/cookies.ts (2)
REFRESH_TOKEN_COOKIE(100-100)proxySetCookie(14-31)
vtex/utils/cookies.ts (1)
vtex/utils/segment.ts (1)
SEGMENT_COOKIE_NAME(7-7)
vtex/actions/authentication/recoveryPassword.ts (1)
vtex/utils/cookies.ts (3)
buildCookieJar(68-96)proxySetCookie(14-31)REFRESH_TOKEN_COOKIE(100-100)
vtex/actions/authentication/accessKeySignIn.ts (1)
vtex/utils/cookies.ts (3)
buildCookieJar(68-96)proxySetCookie(14-31)REFRESH_TOKEN_COOKIE(100-100)
vtex/actions/authentication/refreshToken.ts (1)
vtex/utils/cookies.ts (2)
REFRESH_TOKEN_COOKIE(100-100)proxySetCookie(14-31)
🔇 Additional comments (18)
vtex/utils/types.ts (1)
1886-1889: Logout endpoint returns no JSON — make fields optional or drop the typeVTEX
/api/vtexid/pub/logouttypically redirects and does not return a JSON body. Model this asvoidor make fields optional to avoid misleading consumers.Apply this diff:
-export interface LogoutResponse { - cookieName: string | null; - accountCookieName: string | null; -} +export interface LogoutResponse { + cookieName?: string | null; + accountCookieName?: string | null; +}vtex/actions/authentication/accessKeySignIn.ts (2)
4-8: Good move centralizing cookie utilsImporting buildCookieJar/proxySetCookie/REFRESH_TOKEN_COOKIE improves consistency across auth flows.
65-77: Normalize REFRESH_TOKEN_COOKIE path to/api/vtexidat proxy time
Remove the manual Set-Cookie loop and normalize the cookie path when proxying. If you refactorproxySetCookieto accept a transform callback, you must update its signature across all ~30 existing call sites to avoid breakage; otherwise, apply inline normalization here for just the refresh-token.vtex/utils/cookies.ts (1)
100-101: LGTM: REFRESH_TOKEN_COOKIE constantNaming the refresh-token cookie avoids magic strings and improves consistency.
vtex/actions/authentication/startAuthentication.ts (2)
35-36: LGTM: Forwarding the Cookie headerPassing through request cookies is appropriate for VTEX start flow.
45-46: Normalize vid_rt Path to /api/vtexid at proxy timeKeeps scope tight, fixes logout/refresh flows, and avoids later rewrites.
Apply:
- proxySetCookie(response.headers, ctx.response.headers, req.url); + proxySetCookie( + response.headers, + ctx.response.headers, + req.url, + (c) => c.name === REFRESH_TOKEN_COOKIE ? { ...c, path: "/api/vtexid" } : c, + );vtex/actions/session/createSession.ts (2)
21-33: Solid: forwards cookies via jar and normalizes Set-Cookie from sessionGood adoption of buildCookieJar + setCookiesFromSession. This should fix within-request cookie propagation and session cookie domain/path handling.
19-19: Session actions and downstream callers consistently use GetSessionResponse. No CreateEditSessionResponse references remain.vtex/actions/authentication/sendEmailVerification.ts (2)
32-34: LGTM: cookie jar + proxying is correctCorrectly merges cookies and proxies Set-Cookie to client. Matches the new pattern.
Also applies to: 60-66, 80-81
1-4: Wrong std import for setCookie; will fail at runtimesetCookie is in std/http/cookie.ts, not std/http/mod.ts.
Apply:
-import { setCookie } from "std/http/mod.ts"; +import { setCookie, getSetCookies } from "std/http/cookie.ts"; -import { getSetCookies } from "std/http/cookie.ts";vtex/actions/authentication/classicSignIn.ts (1)
69-72: Good: proxy upstream cookies and validate sessionPattern aligns with other actions and should keep client state in sync.
vtex/actions/authentication/resetPassword.ts (1)
80-89: Avoid duplicate vid_rt cookies; expire legacy path before rewritingReissuing vid_rt at "/" without expiring the default VTEX path creates two cookies and breaks flows.
Apply:
- // TODO: REMOVE THIS AFTER TESTING AND VALIDATE IF NEEDED REWRITE REFRESH_TOKEN_COOKIE - const setCookies = getSetCookies(ctx.response.headers); - for (const cookie of setCookies) { - if (cookie.name === REFRESH_TOKEN_COOKIE) { - setCookie(ctx.response.headers, { - ...cookie, - path: "/", // default path is /api/vtexid/refreshtoken/webstore, but browser dont send to backend headers - }); - } - } + // If you must rewrite, first expire the legacy path to avoid duplicates + const setCookies = getSetCookies(ctx.response.headers); + const rt = setCookies.find((c) => c.name === REFRESH_TOKEN_COOKIE); + if (rt) { + if (rt.path && rt.path !== "/") { + setCookie(ctx.response.headers, { + name: rt.name, + value: "", + domain: rt.domain, + path: rt.path, + secure: rt.secure, + httpOnly: rt.httpOnly, + sameSite: rt.sameSite, + maxAge: 0, + expires: new Date(0), + }); + } + setCookie(ctx.response.headers, { ...rt, path: "/" }); + }vtex/actions/authentication/logout.ts (4)
28-34: Upstream logout call and cookie forwarding look correctForwarding the incoming cookie header and scoping by account via scope is aligned with VTEX’s logout endpoint usage.
Please verify with a real logout against VTEX that Set-Cookie from upstream contains expirations for auth cookies; our added Path normalization should handle refresh-token scope regardless.
49-49: Normalize Set-Cookie Path for VTEX auth cookies after proxyingUpstream may expire cookies at a different Path. Mirror sign-in behavior and normalize the Path to the refresh-token scope so cookies don’t linger.
Apply:
- proxySetCookie(response.headers, ctx.response.headers, req.url); + proxySetCookie(response.headers, ctx.response.headers, req.url); + // Ensure auth cookies also use the VTEX refresh-token path + const VTEX_REFRESH_PATH = "/api/vtexid/refreshtoken/webstore"; + const targetHost = new URL(req.url).hostname; + for (const c of getSetCookies(response.headers)) { + const needsAuthPath = + c.name === REFRESH_TOKEN_COOKIE || c.name.startsWith("VtexIdclientAutCookie"); + if (needsAuthPath) { + setCookie(ctx.response.headers, { + ...c, + domain: targetHost, + path: VTEX_REFRESH_PATH, + }); + } + }And add missing imports:
-import { deleteCookie } from "std/http/cookie.ts"; +import { deleteCookie, getSetCookies, setCookie } from "std/http/cookie.ts";
54-55: Prevent open redirects in returnUrlreturnUrl is user-controlled; restrict to same-origin path.
Apply:
- redirect(returnUrl); - return data; + const safeLocation = (() => { + try { + if (!returnUrl) return "/"; + if (returnUrl.startsWith("/")) return returnUrl.startsWith("//") ? "/" : returnUrl; + const dest = new URL(returnUrl, req.url); + const origin = new URL(req.url).origin; + return dest.origin === origin ? (dest.pathname + dest.search + dest.hash) : "/"; + } catch { + return "/"; + } + })(); + redirect(safeLocation); + return data;
42-43: Guard JSON parse; upstream may not return JSONSome VTEX logout responses may be empty or non-JSON. Avoid runtime errors by checking content-type and providing a typed fallback.
Apply:
- const data = await response.json(); + const contentType = response.headers.get("content-type") ?? ""; + const data: LogoutResponse = contentType.includes("application/json") + ? await response.json() + : { cookieName: null, accountCookieName: null };vtex/actions/session/editSession.ts (2)
21-23: Cookie jar composition LGTMMerging upstream Set-Cookie already staged on the response with incoming request cookies before PATCH /api/sessions is correct.
44-46: Session cookie propagation LGTMUsing setCookiesFromSession with domain rewrite to req.url ensures correct scoping.
| import { AppContext } from "../../mod.ts"; | ||
| import { AuthResponse } from "../../utils/types.ts"; | ||
| import setLoginCookies from "../../utils/login/setLoginCookies.ts"; | ||
| import { getSetCookies } from "std/http/cookie.ts"; | ||
| // import setLoginCookies from "../../utils/login/setLoginCookies.ts"; | ||
| import { getSetCookies, setCookie } from "std/http/cookie.ts"; | ||
| import { | ||
| buildCookieJar, | ||
| proxySetCookie, | ||
| REFRESH_TOKEN_COOKIE, | ||
| } from "../../utils/cookies.ts"; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Unify strategy for vid_rt path across the codebase
PR intent highlights path mismatch issues. Decide one policy (keep VTEX default path vs. rewrite to “/”) and apply consistently (proxy, resetPassword, accessKeySignIn, login utils). I can help extract a utility and update call sites.
🤖 Prompt for AI Agents
In vtex/actions/authentication/classicSignIn.ts lines 1-9: the PR notes
inconsistent handling of the vid_rt cookie path across the codebase; pick one
policy (either preserve VTEX default path or always rewrite to "/") and apply it
consistently by extracting a small cookie-path utility (e.g.,
normalizeVidRtPath(cookie: Cookie | SetCookieOptions) -> SetCookieOptions) and
using it wherever vid_rt is set or proxied. Update this file and the other call
sites (proxy logic, resetPassword, accessKeySignIn, and the login cookie
utilities) to call the new utility so vid_rt path behavior is unified across
proxy, resetPassword, accessKeySignIn, and login utils.
| // TODO: REMOVE THIS AFTER TESTING AND VALIDATE IF NEEDED REWRITE REFRESH_TOKEN_COOKIE | ||
| const setCookies = getSetCookies(ctx.response.headers); | ||
| for (const cookie of setCookies) { | ||
| if (cookie.name === REFRESH_TOKEN_COOKIE) { | ||
| setCookie(ctx.response.headers, { | ||
| ...cookie, | ||
| path: "/", // default path is /api/vtexid/refreshtoken/webstore, but browser dont send to backend headers | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
Stop reissuing vid_rt at "/" without expiring the default path
This can leave two vid_rt cookies ("/" and "/api/.../webstore"), causing nondeterministic behavior on logout/session.
Apply:
- // TODO: REMOVE THIS AFTER TESTING AND VALIDATE IF NEEDED REWRITE REFRESH_TOKEN_COOKIE
- const setCookies = getSetCookies(ctx.response.headers);
- for (const cookie of setCookies) {
- if (cookie.name === REFRESH_TOKEN_COOKIE) {
- setCookie(ctx.response.headers, {
- ...cookie,
- path: "/", // default path is /api/vtexid/refreshtoken/webstore, but browser dont send to backend headers
- });
- }
- }
+ // If rewriting is required, expire the legacy path first to avoid duplicates
+ const setCookies = getSetCookies(ctx.response.headers);
+ const rt = setCookies.find((c) => c.name === REFRESH_TOKEN_COOKIE);
+ if (rt) {
+ if (rt.path && rt.path !== "/") {
+ setCookie(ctx.response.headers, {
+ name: rt.name,
+ value: "",
+ domain: rt.domain,
+ path: rt.path,
+ secure: rt.secure,
+ httpOnly: rt.httpOnly,
+ sameSite: rt.sameSite,
+ maxAge: 0,
+ expires: new Date(0),
+ });
+ }
+ setCookie(ctx.response.headers, { ...rt, path: "/" });
+ }📝 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.
| // TODO: REMOVE THIS AFTER TESTING AND VALIDATE IF NEEDED REWRITE REFRESH_TOKEN_COOKIE | |
| const setCookies = getSetCookies(ctx.response.headers); | |
| for (const cookie of setCookies) { | |
| if (cookie.name === REFRESH_TOKEN_COOKIE) { | |
| setCookie(ctx.response.headers, { | |
| ...cookie, | |
| path: "/", // default path is /api/vtexid/refreshtoken/webstore, but browser dont send to backend headers | |
| }); | |
| } | |
| } | |
| // If rewriting is required, expire the legacy path first to avoid duplicates | |
| const setCookies = getSetCookies(ctx.response.headers); | |
| const rt = setCookies.find((c) => c.name === REFRESH_TOKEN_COOKIE); | |
| if (rt) { | |
| if (rt.path && rt.path !== "/") { | |
| setCookie(ctx.response.headers, { | |
| name: rt.name, | |
| value: "", | |
| domain: rt.domain, | |
| path: rt.path, | |
| secure: rt.secure, | |
| httpOnly: rt.httpOnly, | |
| sameSite: rt.sameSite, | |
| maxAge: 0, | |
| expires: new Date(0), | |
| }); | |
| } | |
| setCookie(ctx.response.headers, { ...rt, path: "/" }); | |
| } |
🤖 Prompt for AI Agents
In vtex/actions/authentication/classicSignIn.ts around lines 73 to 82, the code
forces REFRESH_TOKEN_COOKIE to path "/" which can create a second vid_rt cookie
("/" plus the original "/api/.../webstore") and cause nondeterministic
logout/session behavior; remove the hardcoded path override and instead preserve
the cookie's original path when calling setCookie (or, if you must promote it to
"/", explicitly expire the original cookie at its original path before setting
the "/" cookie) and delete the temporary TODO comment.
| deleteCookie(ctx.response.headers, REFRESH_TOKEN_COOKIE, { | ||
| path: "/", | ||
| domain: new URL(req.url).hostname, | ||
| }); |
There was a problem hiding this comment.
Fix: delete refresh-token cookie using VTEX path, not "/"
The refresh token cookie (vid_rt) is scoped to /api/vtexid/refreshtoken/webstore. Deleting it at path "/" won’t clear the scoped cookie.
Apply:
- deleteCookie(ctx.response.headers, REFRESH_TOKEN_COOKIE, {
- path: "/",
- domain: new URL(req.url).hostname,
- });
+ const VTEX_REFRESH_PATH = "/api/vtexid/refreshtoken/webstore";
+ const targetHost = new URL(req.url).hostname;
+ deleteCookie(ctx.response.headers, REFRESH_TOKEN_COOKIE, {
+ path: VTEX_REFRESH_PATH,
+ domain: targetHost,
+ });📝 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.
| deleteCookie(ctx.response.headers, REFRESH_TOKEN_COOKIE, { | |
| path: "/", | |
| domain: new URL(req.url).hostname, | |
| }); | |
| // Ensure we delete the refresh-token cookie at its scoped VTEX path | |
| const VTEX_REFRESH_PATH = "/api/vtexid/refreshtoken/webstore"; | |
| const targetHost = new URL(req.url).hostname; | |
| deleteCookie(ctx.response.headers, REFRESH_TOKEN_COOKIE, { | |
| path: VTEX_REFRESH_PATH, | |
| domain: targetHost, | |
| }); |
🤖 Prompt for AI Agents
In vtex/actions/authentication/logout.ts around lines 44 to 47, the code deletes
the refresh-token cookie using path "/" which won’t remove the cookie scoped to
/api/vtexid/refreshtoken/webstore; update the deleteCookie call to use path:
"/api/vtexid/refreshtoken/webstore" (keeping domain: new URL(req.url).hostname
and any other existing options) so the scoped vid_rt cookie is properly cleared.
| proxySetCookie(response.headers, ctx.response.headers, req.url); | ||
| await ctx.invoke.vtex.actions.session.validateSession(); | ||
|
|
||
| // TODO: REMOVE THIS AFTER TESTING AND VALIDATE IF NEEDED | ||
| const setCookies = getSetCookies(ctx.response.headers); | ||
| for (const cookie of setCookies) { | ||
| if (cookie.name === REFRESH_TOKEN_COOKIE) { | ||
| setCookie(ctx.response.headers, { | ||
| ...cookie, | ||
| path: "/", // default path is /api/vtexid/refreshtoken/webstore, but browser dont send to backend headers | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
Remove duplicate Set-Cookie and avoid Path="/" for vid_rt
Normalize the refresh-token cookie once via proxy transform to Path=/api/vtexid.
Apply:
- proxySetCookie(response.headers, ctx.response.headers, req.url);
- await ctx.invoke.vtex.actions.session.validateSession();
-
- // TODO: REMOVE THIS AFTER TESTING AND VALIDATE IF NEEDED
- const setCookies = getSetCookies(ctx.response.headers);
- for (const cookie of setCookies) {
- if (cookie.name === REFRESH_TOKEN_COOKIE) {
- setCookie(ctx.response.headers, {
- ...cookie,
- path: "/", // default path is /api/vtexid/refreshtoken/webstore, but browser dont send to backend headers
- });
- }
- }
+ proxySetCookie(
+ response.headers,
+ ctx.response.headers,
+ req.url,
+ (c) => c.name === REFRESH_TOKEN_COOKIE ? { ...c, path: "/api/vtexid" } : c,
+ );
+ await ctx.invoke.vtex.actions.session.validateSession();Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In vtex/actions/authentication/recoveryPassword.ts around lines 71 to 83, remove
the manual loop that re-sets Set-Cookie to Path="/" (which creates duplicate
Set-Cookie headers and wrongly exposes vid_rt to root); instead, delete that
block and update the proxySetCookie call (or the proxy transform that sets
cookies) to normalize the refresh-token cookie by ensuring its Path is set to
"/api/vtexid" and deduplicate any Set-Cookie headers for REFRESH_TOKEN_COOKIE so
only the proxy-normalized cookie is sent.
| if (data?.status !== STATUS.SUCCESS) { | ||
| deleteCookie(ctx.response.headers, REFRESH_TOKEN_COOKIE, { | ||
| path: "/", | ||
| secure: true, | ||
| domain: new URL(req.url).hostname, | ||
| }); |
There was a problem hiding this comment.
Deleting only Path=/ misses the default VTEX path; expire both
VTEX sets vid_rt at /api/vtexid/refreshtoken/webstore. Deleting only Path=/ can leave a valid cookie behind.
Apply:
- if (data?.status !== STATUS.SUCCESS) {
- deleteCookie(ctx.response.headers, REFRESH_TOKEN_COOKIE, {
- path: "/",
- domain: new URL(req.url).hostname,
- });
- }
+ if (data?.status !== STATUS.SUCCESS) {
+ const domain = new URL(req.url).hostname;
+ // Expire both potential paths to avoid duplicates
+ deleteCookie(ctx.response.headers, REFRESH_TOKEN_COOKIE, {
+ path: "/",
+ domain,
+ });
+ deleteCookie(ctx.response.headers, REFRESH_TOKEN_COOKIE, {
+ path: "/api/vtexid/refreshtoken/webstore",
+ domain,
+ });
+ }📝 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.
| if (data?.status !== STATUS.SUCCESS) { | |
| deleteCookie(ctx.response.headers, REFRESH_TOKEN_COOKIE, { | |
| path: "/", | |
| secure: true, | |
| domain: new URL(req.url).hostname, | |
| }); | |
| if (data?.status !== STATUS.SUCCESS) { | |
| const domain = new URL(req.url).hostname; | |
| // Expire both potential paths to avoid duplicates | |
| deleteCookie(ctx.response.headers, REFRESH_TOKEN_COOKIE, { | |
| path: "/", | |
| domain, | |
| }); | |
| deleteCookie(ctx.response.headers, REFRESH_TOKEN_COOKIE, { | |
| path: "/api/vtexid/refreshtoken/webstore", | |
| domain, | |
| }); | |
| } |
🤖 Prompt for AI Agents
In vtex/actions/authentication/refreshToken.ts around lines 54 to 58, the cookie
deletion only targets Path=/ which can leave VTEX's default vid_rt cookie at
/api/vtexid/refreshtoken/webstore intact; update the logic to expire the refresh
cookie for both paths by issuing two deleteCookie calls (or equivalent) with the
same cookie name and domain (new URL(req.url).hostname), once with path "/" and
once with path "/api/vtexid/refreshtoken/webstore", ensuring both cookies are
removed.
| const response = await vcs["POST /api/sessions"]({ | ||
| items: items.join(","), | ||
| }, { | ||
| body: { | ||
| public: { | ||
| ...props.publicProperties, | ||
| }, | ||
| }, | ||
| headers: { cookie }, | ||
| }); |
There was a problem hiding this comment.
Prevent runtime crash when publicProperties is omitted
Props come from user input; spreading undefined throws. Default the object before use.
Apply:
- const response = await vcs["POST /api/sessions"]({
+ const publicProperties = props?.publicProperties ?? {};
+ const response = await vcs["POST /api/sessions"]({
items: items.join(","),
}, {
body: {
public: {
- ...props.publicProperties,
+ ...publicProperties,
},
},
headers: { cookie },
});Also applies to: 29-31
🤖 Prompt for AI Agents
In vtex/actions/session/createSession.ts around lines 24–33, spreading
props.publicProperties may throw if undefined; default it to an empty object
before spreading (e.g., const publicProps = props.publicProperties ?? {} and use
...publicProps in the request body) and apply the same defensive default for the
other spread at lines 29–31 so no runtime crash occurs when callers omit
publicProperties.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
vtex/actions/session/createSession.ts (1)
31-34: DefaultpublicPropertiesbefore spreadingLine 33 spreads
props.publicPropertiesdirectly; if callers omit that field (common for session validation), the spread hitsundefinedand crashes the request. We need to default it before use.Apply:
-interface Props { - publicProperties: Record<string, { value: string }>; -} +interface Props { + publicProperties?: Record<string, { value: string }>; +} @@ - const response = await vcs["POST /api/sessions"]( + const publicProperties = props?.publicProperties ?? {}; + const response = await vcs["POST /api/sessions"]( @@ - public: { - ...props.publicProperties, - }, + public: { + ...publicProperties, + },This prevents the action from throwing when optional props are omitted.
vtex/actions/session/editSession.ts (1)
31-34: Guard against missing public propertiesLine 33 spreads
props.publicProperties, but when the action is invoked without that field (or withpropsundefined),...props.publicPropertiesthrows. This happens in flows likevalidateSession, which can call the action with partial props, causing a runtime 500 instead of editing the session.Apply:
-interface Props { - publicProperties: Record<string, { value: string }>; -} +interface Props { + publicProperties?: Record<string, { value: string }>; +} @@ - const response = await vcs["PATCH /api/sessions"]( + const publicProperties = props?.publicProperties ?? {}; + const response = await vcs["PATCH /api/sessions"]( @@ - public: { - ...props.publicProperties, - }, + public: { + ...publicProperties, + },This keeps the action resilient when optional data is omitted.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
vtex/actions/session/createSession.ts(2 hunks)vtex/actions/session/editSession.ts(2 hunks)vtex/mod.ts(1 hunks)vtex/utils/cookies.ts(2 hunks)vtex/utils/segment.ts(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
vtex/utils/cookies.ts (2)
vtex/mod.ts (1)
AppContext(26-26)vtex/utils/segment.ts (2)
SEGMENT_COOKIE_NAME(7-7)setSegmentBag(162-206)
vtex/actions/session/editSession.ts (3)
vtex/utils/openapi/vcs.openapi.gen.ts (1)
GetSessionResponse(22176-22300)vtex/utils/cookies.ts (2)
buildCookieJar(70-94)setCookiesFromSession(34-58)vtex/utils/session.ts (1)
items(1-36)
vtex/utils/segment.ts (2)
vtex/utils/types.ts (1)
Segment(1077-1098)utils/normalize.ts (1)
removeNonLatin1Chars(13-17)
vtex/actions/session/createSession.ts (3)
vtex/utils/openapi/vcs.openapi.gen.ts (1)
GetSessionResponse(22176-22300)vtex/utils/cookies.ts (2)
buildCookieJar(70-94)setCookiesFromSession(34-58)vtex/utils/session.ts (1)
items(1-36)
🔇 Additional comments (1)
vtex/mod.ts (1)
79-83: Addition looks goodOptional
preserveUtmCharskeeps the API backward-compatible; no issues spotted.
| for (const cookie of getSetCookies(from)) { | ||
| if (cookie.name === SEGMENT_COOKIE_NAME) { | ||
| const { cookies } = buildCookieJar(from, getSetCookies(from)); | ||
| setSegmentBag(cookies, req, ctx); | ||
| continue; | ||
| } | ||
| const newCookie = { | ||
| ...cookie, | ||
| domain: newDomain.hostname, | ||
| }; | ||
|
|
||
| setCookie(ctx.response.headers, newCookie); | ||
| } |
There was a problem hiding this comment.
Do not swallow the upstream vtex_segment Set-Cookie
Right now the SEGMENT_COOKIE branch does two harmful things:
- It short-circuits before calling
setCookie, so any upstreamSet-Cookie: vtex_segment=...is dropped on the floor. On a fresh session/login the browser never receives the segment token, which breaks follow-up requests. - It builds the jar with
buildCookieJar(from, …), butfromare response headers. That loses the request’s existing cookies (e.g.VTEXSC), sosetSegmentBagcan no longer recover the sales channel and falls back to defaults.
Together this regresses both session propagation and segmentation. Please merge the request cookies into the jar and still emit the Set-Cookie to the client.
export const setCookiesFromSession = ({
from,
req,
ctx,
}: {
from: Headers;
req: Request;
ctx: AppContext;
}) => {
const newDomain = new URL(req.url);
+ const upstreamSetCookies = getSetCookies(from);
+ const { cookies } = buildCookieJar(req.headers, upstreamSetCookies);
- for (const cookie of getSetCookies(from)) {
+ for (const cookie of upstreamSetCookies) {
if (cookie.name === SEGMENT_COOKIE_NAME) {
- const { cookies } = buildCookieJar(from, getSetCookies(from));
setSegmentBag(cookies, req, ctx);
- continue;
}
const newCookie = {
...cookie,
domain: newDomain.hostname,
};
setCookie(ctx.response.headers, newCookie);
}
};📝 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.
| for (const cookie of getSetCookies(from)) { | |
| if (cookie.name === SEGMENT_COOKIE_NAME) { | |
| const { cookies } = buildCookieJar(from, getSetCookies(from)); | |
| setSegmentBag(cookies, req, ctx); | |
| continue; | |
| } | |
| const newCookie = { | |
| ...cookie, | |
| domain: newDomain.hostname, | |
| }; | |
| setCookie(ctx.response.headers, newCookie); | |
| } | |
| export const setCookiesFromSession = ({ | |
| from, | |
| req, | |
| ctx, | |
| }: { | |
| from: Headers; | |
| req: Request; | |
| ctx: AppContext; | |
| }) => { | |
| const newDomain = new URL(req.url); | |
| const upstreamSetCookies = getSetCookies(from); | |
| const { cookies } = buildCookieJar(req.headers, upstreamSetCookies); | |
| for (const cookie of upstreamSetCookies) { | |
| if (cookie.name === SEGMENT_COOKIE_NAME) { | |
| setSegmentBag(cookies, req, ctx); | |
| } | |
| const newCookie = { | |
| ...cookie, | |
| domain: newDomain.hostname, | |
| }; | |
| setCookie(ctx.response.headers, newCookie); | |
| } | |
| }; |
🤖 Prompt for AI Agents
In vtex/utils/cookies.ts around lines 45-57, the SEGMENT_COOKIE branch currently
short-circuits before forwarding the upstream Set-Cookie and builds a jar only
from the response, losing request cookies; to fix, merge the request's cookies
into the cookie jar (parse req.headers.cookie or include req cookies) together
with getSetCookies(from) when calling buildCookieJar so setSegmentBag sees both
request and response cookies, call setSegmentBag(cookies, req, ctx) as before,
and then still call setCookie(ctx.response.headers, { ...cookie, domain:
newDomain.hostname }) for the SEGMENT_COOKIE instead of continue so the upstream
vtex_segment Set-Cookie is emitted to the client.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
verified-reviews/loaders/productDetailsPage.ts (1)
56-56: Document the caching strategy for verified-reviews loaders.This adds the same
cache = "no-cache"export seen in productList.ts and productListingPage.ts, indicating a deliberate pattern across all verified-reviews loaders.While the consistency is good, the connection to this PR's VTEX session/logout objectives remains unclear, and the rationale for disabling caching on these loaders is undocumented.
Consider adding a shared constant or module-level documentation explaining the caching policy. For example, create a shared constant:
// verified-reviews/utils/cache.ts /** * Verified reviews loaders use no-cache to ensure fresh data * after user authentication state changes. */ export const VERIFIED_REVIEWS_CACHE_POLICY = "no-cache";Then import and export it from each loader:
export { VERIFIED_REVIEWS_CACHE_POLICY as cache } from "../utils/cache.ts";This centralizes the policy and makes the reasoning explicit.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
verified-reviews/loaders/productDetailsPage.ts(1 hunks)verified-reviews/loaders/productList.ts(1 hunks)verified-reviews/loaders/productListingPage.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- verified-reviews/loaders/productListingPage.ts
🧰 Additional context used
🧬 Code graph analysis (2)
verified-reviews/loaders/productList.ts (2)
verified-reviews/loaders/productDetailsPage.ts (1)
cache(56-56)verified-reviews/loaders/productListingPage.ts (1)
cache(49-49)
verified-reviews/loaders/productDetailsPage.ts (2)
verified-reviews/loaders/productList.ts (1)
cache(41-41)verified-reviews/loaders/productListingPage.ts (1)
cache(49-49)
🔇 Additional comments (1)
verified-reviews/loaders/productList.ts (1)
41-41: Clarify the relationship to PR objectives and add documentation.This PR is titled "fix Logout and session VTEX" and describes issues with refresh token paths and logout routes. However, this file is a product ratings loader in the verified-reviews module, which appears unrelated to authentication or session management.
Additionally, while the cache export is straightforward, there's no explanation of why "no-cache" is needed here. This disables caching for product ratings, which could impact performance.
Please confirm:
- Is this change intentional and related to the VTEX logout/session fix?
- Why is caching disabled for product ratings loaders?
- Could you add a brief comment explaining the reasoning?
Example:
// Disable caching to ensure fresh ratings data after session changes export const cache = "no-cache";
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
commerce/loaders/product/products.ts (1)
16-17: Add blank line for consistency and clarify cache strategy.Add a blank line before the export statement to maintain consistent formatting across the codebase (run
deno fmt).Since this loader is deprecated and the change follows a broader pattern, verify that disabling cache is the appropriate solution rather than addressing the underlying session/authentication requirements more selectively.
commerce/loaders/product/productListingPage.ts (1)
16-17: Re-run formatter and reconsider cache change on deprecated loader. A blank line already precedesexport const cache, so runningdeno fmtshould clear the CI error. Since this loader is marked deprecated, disable caching here only if strictly necessary—otherwise prioritize migrating to the new loader.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
commerce/loaders/product/extensions/list.ts(1 hunks)commerce/loaders/product/extensions/suggestions.ts(1 hunks)commerce/loaders/product/productListingPage.ts(1 hunks)commerce/loaders/product/products.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
commerce/loaders/product/extensions/list.ts (10)
commerce/loaders/product/extensions/suggestions.ts (1)
cache(18-18)commerce/loaders/product/productListingPage.ts (1)
cache(17-17)commerce/loaders/product/products.ts (1)
cache(17-17)vtex/loaders/product/extensions/list.ts (1)
cache(23-23)vtex/loaders/product/extensions/suggestions.ts (1)
cache(31-31)vtex/loaders/product/extend.ts (1)
cache(169-169)commerce/loaders/product/extensions/detailsPage.ts (1)
cache(16-16)vtex/loaders/product/extensions/detailsPage.ts (1)
cache(31-31)commerce/loaders/product/extensions/listingPage.ts (1)
cache(16-16)vtex/loaders/product/extensions/listingPage.ts (1)
cache(31-31)
commerce/loaders/product/extensions/suggestions.ts (4)
commerce/loaders/product/extensions/list.ts (1)
cache(16-16)commerce/loaders/product/productListingPage.ts (1)
cache(17-17)commerce/loaders/product/products.ts (1)
cache(17-17)vtex/loaders/product/extensions/suggestions.ts (1)
cache(31-31)
commerce/loaders/product/productListingPage.ts (5)
commerce/loaders/product/extensions/list.ts (1)
cache(16-16)commerce/loaders/product/extensions/suggestions.ts (1)
cache(18-18)commerce/loaders/product/products.ts (1)
cache(17-17)commerce/loaders/product/extensions/detailsPage.ts (1)
cache(16-16)commerce/loaders/product/extensions/listingPage.ts (1)
cache(16-16)
commerce/loaders/product/products.ts (5)
commerce/loaders/product/extensions/list.ts (1)
cache(16-16)commerce/loaders/product/extensions/suggestions.ts (1)
cache(18-18)commerce/loaders/product/productListingPage.ts (1)
cache(17-17)commerce/loaders/product/extensions/detailsPage.ts (1)
cache(16-16)commerce/loaders/product/extensions/listingPage.ts (1)
cache(16-16)
🪛 GitHub Actions: ci
commerce/loaders/product/extensions/list.ts
[error] 16-16: Deno fmt check failed: not formatted. Run 'deno fmt' to fix formatting issues in this file.
commerce/loaders/product/extensions/suggestions.ts
[error] 18-18: Deno fmt check failed: not formatted. Run 'deno fmt' to fix formatting issues in this file.
commerce/loaders/product/productListingPage.ts
[error] 17-17: Deno fmt check failed: not formatted. Run 'deno fmt' to fix formatting issues in this file.
🔇 Additional comments (2)
commerce/loaders/product/extensions/list.ts (1)
15-16: Fix formatting and justify disabling cacheRun
deno fmtto insert a blank line before the export.Disabling cache globally (
Cache-Control: no-cache) forces CDN revalidation on every request—adding origin round-trips, higher latency, and increased origin load. Please clarify:
- Why is a global
no-cachedirective required for this product loader?- Is this tied to ensuring fresh data post-authentication changes?
- Could you use targeted invalidation (e.g. cache-busting headers) or a
max-age/s-maxagewithstale-while-revalidateinstead?commerce/loaders/product/extensions/suggestions.ts (1)
17-18: Verify necessity of broad cache disabling. Disabling cache (export const cache = "no-cache") in the suggestions loader may degrade performance; confirm this is required for the session/logout fix and consider more granular cache invalidation on authentication state changes.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
website/components/Analytics.tsx (1)
122-132: Consider adding format constraints for the key field.While the interface is well-defined, consider adding validation hints in the JSDoc or using string patterns to guide users toward URL-safe keys (e.g., alphanumeric with hyphens/underscores). This prevents potential issues with special characters in URL parameters.
Example JSDoc enhancement:
/** - * @description key for extra param. + * @description Key for extra param (use alphanumeric characters, hyphens, or underscores). */ key: string;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
website/components/Analytics.tsx(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
website/components/Analytics.tsx (1)
compat/std/sections/Analytics.tsx (1)
Props(34-49)
🔇 Additional comments (3)
website/components/Analytics.tsx (3)
1-203: PR scope mismatch with file changes.The PR title indicates "fix Logout and session VTEX" and the AI summary mentions "authentication/session cookie handling," but this file implements GTM extra parameters functionality unrelated to authentication or VTEX sessions. Please verify this file was intended for this PR.
36-44: Verify handling of empty values.The code filters out empty keys (line 41) but allows empty values. This means
{key: "test", value: ""}will append?test=to the URL. Confirm whether empty values should be filtered or if this behavior is intentional for flag-style parameters.If empty values should be filtered, apply this diff:
- if (key) { + if (key && value) { src.searchParams.append(key, value); noscript.searchParams.append(key, value); }
16-16: LGTM! Clean integration of extra params.The implementation correctly:
- Types extraParams as optional throughout
- Uses null coalescing to handle undefined cases
- Filters reserved keys case-insensitively
- Threads parameters consistently to all GoogleTagManager instances
- Maintains parity between iframe and script URLs
Also applies to: 34-45, 149-152, 178-182, 189-193
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
verified-reviews/loaders/productDetailsPage.ts (1)
22-25: Remove redundant client creation.The client is created on line 22 but is no longer used for data fetching (line 39 now uses
ctx.invoke). The client is only checked for null on line 25, which is wasteful. Since the loader now delegates tofullReviewviactx.invoke, the client check may be unnecessary.Consider removing the client creation and null check:
- const client = createClient({ ...ctx }); - return async (productDetailsPage: ProductDetailsPage | null) => { - if (!productDetailsPage || !client) { + if (!productDetailsPage) { return null; }Alternatively, if you need to verify configuration is present, check
ctxproperties directly rather than creating a client instance.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
verified-reviews/loaders/fullReview.ts(1 hunks)verified-reviews/loaders/productDetailsPage.ts(2 hunks)verified-reviews/manifest.gen.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
verified-reviews/loaders/fullReview.ts (2)
verified-reviews/utils/client.ts (2)
PaginationOptions(12-27)createClient(47-209)verified-reviews/utils/types.ts (1)
VerifiedReviewsFullReview(52-55)
verified-reviews/loaders/productDetailsPage.ts (3)
verified-reviews/loaders/fullReview.ts (1)
cache(36-36)verified-reviews/loaders/productList.ts (1)
cache(41-41)verified-reviews/loaders/productListingPage.ts (1)
cache(49-49)
🔇 Additional comments (5)
verified-reviews/loaders/fullReview.ts (2)
10-34: LGTM: Solid error handling and fallback strategy.The loader implementation properly handles missing client and errors by returning a safe fallback structure with
aggregateRating: undefinedand emptyreviewarray. The error logging provides useful debugging information.
36-36: Appropriate cache policy for review data.The
stale-while-revalidatecache policy is well-suited for review data, allowing stale content to be served while fetching fresh data in the background.verified-reviews/loaders/productDetailsPage.ts (2)
56-56: Verify cache policy consistency.This loader exports
cache = "no-cache"while the invokedfullReviewloader usescache = "stale-while-revalidate". Please verify this is intentional. IfproductDetailsPageis an extension loader that should always refetch, this is correct. Otherwise, consider aligning the cache policies.Based on the relevant code snippets, other loaders in this module (
productList.ts,productListingPage.ts) also exportcache = "no-cache", suggesting this might be a consistent pattern for these extension loaders.
39-44: LGTM: Clean invocation of the new fullReview loader.The switch to using
ctx.invoke["verified-reviews"].loaders.fullReviewproperly delegates review fetching to the dedicated loader, improving modularity and reusability.verified-reviews/manifest.gen.ts (1)
1-27: LGTM: Manifest properly updated for new loader.The auto-generated manifest correctly integrates the new
fullReview.tsloader and exports theManifesttype and default export. The loader mappings are properly ordered and aliased.
| ctx: AppContext | ||
| ): Promise<VerifiedReviewsFullReview> { | ||
| try { | ||
| const client = createClient({ ...ctx }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Verify the context spreading pattern.
Spreading the entire AppContext into createClient may pass unnecessary properties. According to the relevant code snippet, createClient expects ConfigVerifiedReviews | undefined. Please verify that the AppContext structure is compatible with this expectation, or extract only the necessary properties.
🏁 Script executed:
#!/bin/bash
# Description: Verify the createClient function signature and AppContext structure
# Check createClient function signature
ast-grep --pattern $'export $_ createClient($PARAM) {
$$$
}'
# Check AppContext type definition
rg -nP "export\s+(type|interface)\s+AppContext" verified-reviews/Length of output: 79
🏁 Script executed:
#!/bin/bash
# Locate createClient definition and usage
rg -nP "createClient" -R verified-reviews/
# Show import context in fullReview.ts
sed -n '1,10p' verified-reviews/loaders/fullReview.ts
# Extract createClient signature from mod.ts
rg -nP "export .*createClient" -C3 verified-reviews/mod.tsLength of output: 736
🏁 Script executed:
#!/bin/bash
# Extract createClient definition and its parameter type
rg -nP "export\s+(async\s+)?(function|const)\s+createClient" -C5 verified-reviews/utils/client.tsLength of output: 542
Avoid passing entire AppContext into createClient.
createClient accepts only a ConfigVerifiedReviews object (it only uses idWebsite), so replace
createClient({ ...ctx });with something like
createClient({ idWebsite: ctx.idWebsite });🤖 Prompt for AI Agents
In verified-reviews/loaders/fullReview.ts around line 16, createClient is being
called with the entire AppContext ({ ...ctx }) even though it only expects a
ConfigVerifiedReviews object (it only uses idWebsite); change the call to pass
only the required shape by extracting idWebsite from ctx and calling
createClient with that single property (e.g., createClient({ idWebsite:
ctx.idWebsite })), ensuring the rest of ctx is not spread into the client
creation.
| export const cacheKey = (props: Props) => { | ||
| const normalized = { | ||
| productId: Array.isArray(props.productId) | ||
| ? [...props.productId].sort().join(",") | ||
| : props.productId, | ||
| count: props.count ?? 10, | ||
| offset: props.offset ?? 0, | ||
| order: props.order ?? "date_desc", | ||
| customizeOrder: props.customizeOrder ?? false, | ||
| }; | ||
|
|
||
| return `${normalized.productId}-${normalized.count}-${normalized.offset}-${normalized.order}-${normalized.customizeOrder}`; | ||
| }; |
There was a problem hiding this comment.
Inconsistent default values between cacheKey and client implementation.
The cacheKey function uses count: 10 as the default (line 43), but according to the relevant code snippet from client.ts, client.fullReview uses count = 5 as its default. This mismatch could cause cache keys to differ from actual request parameters, leading to unnecessary cache misses.
Apply this diff to align the default with the client implementation:
- count: props.count ?? 10,
+ count: props.count ?? 5,📝 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.
| export const cacheKey = (props: Props) => { | |
| const normalized = { | |
| productId: Array.isArray(props.productId) | |
| ? [...props.productId].sort().join(",") | |
| : props.productId, | |
| count: props.count ?? 10, | |
| offset: props.offset ?? 0, | |
| order: props.order ?? "date_desc", | |
| customizeOrder: props.customizeOrder ?? false, | |
| }; | |
| return `${normalized.productId}-${normalized.count}-${normalized.offset}-${normalized.order}-${normalized.customizeOrder}`; | |
| }; | |
| export const cacheKey = (props: Props) => { | |
| const normalized = { | |
| productId: Array.isArray(props.productId) | |
| ? [...props.productId].sort().join(",") | |
| : props.productId, | |
| count: props.count ?? 5, | |
| offset: props.offset ?? 0, | |
| order: props.order ?? "date_desc", | |
| customizeOrder: props.customizeOrder ?? false, | |
| }; | |
| return `${normalized.productId}-${normalized.count}-${normalized.offset}-${normalized.order}-${normalized.customizeOrder}`; | |
| }; |
🤖 Prompt for AI Agents
In verified-reviews/loaders/fullReview.ts around lines 38 to 50, the cacheKey
function sets a default count of 10 which is inconsistent with
client.fullReview's default count of 5; update the normalized.count default from
10 to 5 so the cache key reflects the actual request default and prevents
unnecessary cache misses.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
vtex/utils/types.ts (1)
1886-1889: Logout returns no JSON body—update type or callers.
/api/vtexid/pub/logoutredirects and emits Set-Cookie only; it never returns{ cookieName, accountCookieName }. Keeping these as required fields misrepresents the response and encourages code to dereference properties that will never exist, leading to runtime failures whenresponse.json()throws. Please model the endpoint asvoid(or make the fields optional and guard at call sites).vtex/actions/authentication/refreshToken.ts (1)
57-61: Still expiring only Path=/; remove/api/vtexidcookie too.When the refresh fails we have to wipe every
vid_rt. VTEX sets the token at/api/vtexid/refreshtoken/webstore; deleting only the Path=/ variant leaves the original cookie untouched, so the browser keeps resending the stale token and we loop on failures. Issue already flagged—please expire both paths (or iterate over known paths).vtex/actions/authentication/accessKeySignIn.ts (1)
74-83: Drop the temporary Path=/ rewrite forvid_rt.This loop replays every
Set-Cookieand appends a secondvid_rtwithPath=/. The response now emits two refresh-token cookies (original/api/vtexid/refreshtoken/webstoreplus/), which confuses browsers and upstream parsers and widens the token’s scope unnecessarily. Please remove the block and handle path normalization in the proxy (e.g., single transformed cookie), per the earlier guidance.vtex/actions/authentication/classicSignIn.ts (1)
80-88: Do not emit vid_rt twice—expire the legacy path before rewriting.Line 82 grabs the proxied vid_rt and Line 86 overwrites its path to “/”. Because the original Set-Cookie (still at
/api/vtexid/refreshtoken/webstore) remains, the browser ends up with two vid_rt cookies whose precedence depends on path ordering, leading to random logout/session failures. Make sure you either preserve the original path or explicitly expire the legacy cookie before minting the “/” version.- const setCookies = getSetCookies(ctx.response.headers); - for (const cookie of setCookies) { - if (cookie.name === REFRESH_TOKEN_COOKIE) { - setCookie(ctx.response.headers, { - ...cookie, - path: "/", // default path is /api/vtexid/refreshtoken/webstore, but browser dont send to backend headers - }); - } - } + const setCookies = getSetCookies(ctx.response.headers); + const rt = setCookies.find((cookie) => cookie.name === REFRESH_TOKEN_COOKIE); + if (rt && rt.path && rt.path !== "/") { + setCookie(ctx.response.headers, { + ...rt, + value: "", + maxAge: 0, + expires: new Date(0), + }); + setCookie(ctx.response.headers, { ...rt, path: "/" }); + }
🧹 Nitpick comments (1)
website/components/Analytics.tsx (1)
16-16: Consider adding validation for whitespace-only keys.The implementation correctly filters reserved keys (case-insensitive) and appends extra params to both GTM URLs. However, the check
if (key)on line 41 filters empty strings but allows whitespace-only strings like" ", which could create malformed URL parameters.Apply this diff to trim keys and filter whitespace-only values:
- for (const { key, value } of (props.extraParams ?? [])) { + for (let { key, value } of (props.extraParams ?? [])) { + key = key.trim(); + if (reservedKeys.has(key.toLowerCase())) { continue; } if (key) { src.searchParams.append(key, value); noscript.searchParams.append(key, value); } }Also applies to: 34-45, 122-132, 149-152
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
commerce/loaders/product/extensions/list.ts(1 hunks)commerce/loaders/product/extensions/suggestions.ts(1 hunks)commerce/loaders/product/productListingPage.ts(1 hunks)verified-reviews/loaders/fullReview.ts(1 hunks)verified-reviews/loaders/productList.ts(1 hunks)verified-reviews/loaders/productListingPage.ts(1 hunks)vtex/actions/authentication/accessKeySignIn.ts(4 hunks)vtex/actions/authentication/classicSignIn.ts(5 hunks)vtex/actions/authentication/refreshToken.ts(4 hunks)vtex/actions/session/createSession.ts(1 hunks)vtex/actions/session/editSession.ts(1 hunks)vtex/actions/session/validateSession.ts(1 hunks)vtex/loaders/product/extensions/list.ts(1 hunks)vtex/utils/segment.ts(4 hunks)vtex/utils/session.ts(1 hunks)vtex/utils/types.ts(1 hunks)website/components/Analytics.tsx(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- verified-reviews/loaders/productList.ts
- commerce/loaders/product/productListingPage.ts
- commerce/loaders/product/extensions/list.ts
- verified-reviews/loaders/productListingPage.ts
- verified-reviews/loaders/fullReview.ts
- vtex/loaders/product/extensions/list.ts
- commerce/loaders/product/extensions/suggestions.ts
- vtex/utils/segment.ts
🧰 Additional context used
🧬 Code graph analysis (7)
vtex/actions/authentication/classicSignIn.ts (1)
vtex/utils/cookies.ts (3)
buildCookieJar(70-94)proxySetCookie(15-32)REFRESH_TOKEN_COOKIE(98-98)
vtex/actions/authentication/accessKeySignIn.ts (1)
vtex/utils/cookies.ts (3)
buildCookieJar(70-94)proxySetCookie(15-32)REFRESH_TOKEN_COOKIE(98-98)
website/components/Analytics.tsx (1)
compat/std/sections/Analytics.tsx (1)
Props(34-49)
vtex/actions/session/createSession.ts (5)
vtex/actions/session/validateSession.ts (1)
action(6-18)vtex/utils/types.ts (1)
SessionProps(1891-1899)vtex/utils/openapi/vcs.openapi.gen.ts (1)
GetSessionResponse(22176-22300)vtex/utils/cookies.ts (2)
buildCookieJar(70-94)setCookiesFromSession(34-58)vtex/utils/session.ts (1)
defaultItems(1-39)
vtex/actions/session/validateSession.ts (2)
vtex/utils/types.ts (1)
SessionProps(1891-1899)vtex/utils/openapi/vcs.openapi.gen.ts (1)
GetSessionResponse(22176-22300)
vtex/actions/authentication/refreshToken.ts (1)
vtex/utils/cookies.ts (2)
buildCookieJar(70-94)REFRESH_TOKEN_COOKIE(98-98)
vtex/actions/session/editSession.ts (3)
vtex/utils/types.ts (1)
SessionProps(1891-1899)vtex/utils/cookies.ts (2)
buildCookieJar(70-94)setCookiesFromSession(34-58)vtex/utils/session.ts (1)
defaultItems(1-39)
🔇 Additional comments (1)
website/components/Analytics.tsx (1)
159-165: LGTM! Proper integration of extraParams.The
extraParamsprop is correctly threaded through both GTM render paths (mappedtrackingIdsat lines 178-182 and fallbacksrcpath at lines 189-193). The destructuring and propagation are consistent.Also applies to: 178-182, 189-193
| { | ||
| publicProperties, | ||
| items, | ||
| }: SessionProps, | ||
| req: Request, | ||
| ctx: AppContext, | ||
| ): Promise<CreateEditSessionResponse> { | ||
| ): Promise<GetSessionResponse> { | ||
| const { vcs } = ctx; | ||
| const { cookie } = parseCookie(req.headers, ctx.account); | ||
| const setCookiesSoFar = getSetCookies(ctx.response.headers); | ||
| const { header: cookie } = buildCookieJar(req.headers, setCookiesSoFar); | ||
|
|
||
| const url = new URL(req.url); | ||
| const searchParams = new URLSearchParams(url.search); | ||
| searchParams.set("items", items?.join(",") || defaultItems); | ||
|
|
||
| const response = await vcs["PATCH /api/sessions"]({}, { | ||
| body: { | ||
| public: { | ||
| ...props.publicProperties, | ||
| const response = await vcs["PATCH /api/sessions"]( | ||
| Object.fromEntries(searchParams.entries()), | ||
| { | ||
| body: { | ||
| public: { | ||
| ...publicProperties, | ||
| }, | ||
| }, | ||
| headers: { cookie }, | ||
| }, |
There was a problem hiding this comment.
Guard against missing props before destructuring/spreading.
Directly destructuring SessionProps and spreading publicProperties assumes callers always pass a full payload. At runtime a missing body (common when invoked through templates or entrypoints that don’t send props) will throw before you even reach the API call. Default the param and each field before use.
-async function action(
- {
- publicProperties,
- items,
- }: SessionProps,
+async function action(
+ props: SessionProps | undefined,
req: Request,
ctx: AppContext,
): Promise<GetSessionResponse> {
+ const { publicProperties = {}, items } = props ?? {};
@@
- searchParams.set("items", items?.join(",") || defaultItems);
+ searchParams.set("items", items?.join(",") || defaultItems);
@@
- public: {
- ...publicProperties,
- },
+ public: {
+ ...publicProperties,
+ },📝 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.
| { | |
| publicProperties, | |
| items, | |
| }: SessionProps, | |
| req: Request, | |
| ctx: AppContext, | |
| ): Promise<CreateEditSessionResponse> { | |
| ): Promise<GetSessionResponse> { | |
| const { vcs } = ctx; | |
| const { cookie } = parseCookie(req.headers, ctx.account); | |
| const setCookiesSoFar = getSetCookies(ctx.response.headers); | |
| const { header: cookie } = buildCookieJar(req.headers, setCookiesSoFar); | |
| const url = new URL(req.url); | |
| const searchParams = new URLSearchParams(url.search); | |
| searchParams.set("items", items?.join(",") || defaultItems); | |
| const response = await vcs["PATCH /api/sessions"]({}, { | |
| body: { | |
| public: { | |
| ...props.publicProperties, | |
| const response = await vcs["PATCH /api/sessions"]( | |
| Object.fromEntries(searchParams.entries()), | |
| { | |
| body: { | |
| public: { | |
| ...publicProperties, | |
| }, | |
| }, | |
| headers: { cookie }, | |
| }, | |
| async function action( | |
| props: SessionProps | undefined, | |
| req: Request, | |
| ctx: AppContext, | |
| ): Promise<GetSessionResponse> { | |
| const { publicProperties = {}, items } = props ?? {}; | |
| const { vcs } = ctx; | |
| const setCookiesSoFar = getSetCookies(ctx.response.headers); | |
| const { header: cookie } = buildCookieJar(req.headers, setCookiesSoFar); | |
| const url = new URL(req.url); | |
| const searchParams = new URLSearchParams(url.search); | |
| searchParams.set("items", items?.join(",") || defaultItems); | |
| const response = await vcs["PATCH /api/sessions"]( | |
| Object.fromEntries(searchParams.entries()), | |
| { | |
| body: { | |
| public: { | |
| ...publicProperties, | |
| }, | |
| }, | |
| headers: { cookie }, | |
| }, | |
| ); | |
| // …rest of implementation… | |
| } |
🤖 Prompt for AI Agents
In vtex/actions/session/editSession.ts around lines 13 to 37, the function
destructures SessionProps and spreads publicProperties without guarding for a
missing or undefined payload, which will throw at runtime; fix it by defaulting
the SessionProps parameter to an empty object and defaulting its fields when
destructuring (e.g., items = undefined or [] and publicProperties = {}) so the
spread is safe, ensure searchParams.set uses defaultItems when items is falsy,
and propagate these safe defaults into the vcs PATCH call and headers
construction.
…es' to defaultItems
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
vtex/utils/session.ts (1)
1-41: Consider adding documentation for the constant.The
defaultItemsconstant would benefit from a JSDoc comment explaining:
- Its purpose (default session keys to fetch from VTEX session API)
- When it's used (as fallback in
createSessionandeditSessionactions)- The meaning of the namespaces (account., store., etc.)
Apply this diff to add documentation:
+/** + * Default session keys to fetch from VTEX session API. + * Used as fallback when no specific items are provided in session actions. + * Covers account info, store settings, authentication, profile, checkout, and public data. + */ export const defaultItems = [ "account.id",
| "priceTables", | ||
| "profilePriceTables", | ||
| "profile.priceTables", | ||
| "profile.profilePriceTables", |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Verify potential duplicate price table entries.
Lines 25-28 contain multiple price table related entries that may be duplicates:
"priceTables"(line 25)"profilePriceTables"(line 26)"profile.priceTables"(line 27)"profile.profilePriceTables"(line 28)
In particular, "profilePriceTables" and "profile.profilePriceTables" appear redundant. Please confirm whether all four entries are necessary or if some can be removed to avoid unnecessary data transfer.
Run the following script to check how these keys are used in the VTEX session API and related code:
🏁 Script executed:
#!/bin/bash
# Description: Check usage of price table related session keys in the codebase
# Search for references to these specific price table keys
rg -n -C3 'priceTables|profilePriceTables' --type=tsLength of output: 68468
Remove unused session keys in defaultItems.
In vtex/utils/session.ts (lines 25–28), delete "profilePriceTables", "profile.priceTables" and "profile.profilePriceTables"—only "priceTables" is supported by the session API and types.
🤖 Prompt for AI Agents
In vtex/utils/session.ts around lines 25 to 28, remove the unsupported session
keys by deleting "profilePriceTables", "profile.priceTables", and
"profile.profilePriceTables" from the defaultItems array so that only
"priceTables" remains (update any trailing commas or formatting as needed to
keep the array valid).
…ession type instead of GetSessionResponse
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
vtex/actions/session/editSession.ts (1)
12-36: Default the incoming props before destructuringJust like
createSession, this action will throw when invoked without props: destructuringitems/publicPropertiesfromundefinedcrashes, and spreadingpublicPropertiesdoes too. Normalize the payload before use so templated or bare invocations don't break.-async function action( - { - publicProperties, - items, - }: SessionProps, +async function action( + incomingProps: SessionProps | undefined, req: Request, ctx: AppContext, ): Promise<Session> { const { vcs } = ctx; const setCookiesSoFar = getSetCookies(ctx.response.headers); const { header: cookie } = buildCookieJar(req.headers, setCookiesSoFar); const url = new URL(req.url); const searchParams = new URLSearchParams(url.search); - searchParams.set("items", items?.join(",") || defaultItems); + const { publicProperties = {}, items } = incomingProps ?? {}; + const resolvedItems = items?.length ? items.join(",") : defaultItems; + searchParams.set("items", resolvedItems); const response = await vcs["PATCH /api/sessions"]( Object.fromEntries(searchParams.entries()), { body: { public: { ...publicProperties, }, }, headers: { cookie }, }, );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
vtex/actions/session/createSession.ts(1 hunks)vtex/actions/session/editSession.ts(1 hunks)vtex/actions/session/validateSession.ts(1 hunks)vtex/loaders/session/getSession.ts(3 hunks)vtex/utils/types.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- vtex/actions/session/validateSession.ts
🧰 Additional context used
🧬 Code graph analysis (3)
vtex/loaders/session/getSession.ts (1)
vtex/utils/types.ts (1)
Session(1864-2266)
vtex/actions/session/createSession.ts (4)
vtex/actions/session/validateSession.ts (1)
action(5-17)vtex/utils/types.ts (2)
SessionProps(1854-1862)Session(1864-2266)vtex/utils/cookies.ts (2)
buildCookieJar(70-94)setCookiesFromSession(34-58)vtex/utils/session.ts (1)
defaultItems(1-41)
vtex/actions/session/editSession.ts (4)
vtex/actions/session/validateSession.ts (1)
action(5-17)vtex/utils/types.ts (2)
SessionProps(1854-1862)Session(1864-2266)vtex/utils/cookies.ts (2)
buildCookieJar(70-94)setCookiesFromSession(34-58)vtex/utils/session.ts (1)
defaultItems(1-41)
| { | ||
| publicProperties, | ||
| items, | ||
| }: SessionProps, | ||
| req: Request, | ||
| ctx: AppContext, | ||
| ): Promise<CreateEditSessionResponse> { | ||
| ): Promise<Session> { | ||
| const { vcs } = ctx; | ||
| const setCookiesSoFar = getSetCookies(ctx.response.headers); | ||
| const { header: cookie } = buildCookieJar(req.headers, setCookiesSoFar); | ||
|
|
||
| const url = new URL(req.url); | ||
| const searchParams = new URLSearchParams(url.search); | ||
| searchParams.set("items", items?.join(",") || defaultItems); | ||
|
|
||
| const response = await vcs["POST /api/sessions"]({}, { | ||
| body: { | ||
| public: { | ||
| ...props.publicProperties, | ||
| const response = await vcs["POST /api/sessions"]( | ||
| Object.fromEntries(searchParams.entries()), | ||
| { | ||
| body: { | ||
| public: { | ||
| ...publicProperties, | ||
| }, | ||
| }, | ||
| headers: { cookie }, | ||
| }, |
There was a problem hiding this comment.
Guard against missing props to avoid crashes
If this action is invoked without a body (common for session actions), props arrives as undefined. The parameter destructuring and ...publicProperties spread will throw TypeError: Cannot destructure property 'publicProperties' of 'undefined'. Capture the incoming payload and default its fields before using them.
-async function action(
- {
- publicProperties,
- items,
- }: SessionProps,
+async function action(
+ incomingProps: SessionProps | undefined,
req: Request,
ctx: AppContext,
): Promise<Session> {
const { vcs } = ctx;
const setCookiesSoFar = getSetCookies(ctx.response.headers);
const { header: cookie } = buildCookieJar(req.headers, setCookiesSoFar);
const url = new URL(req.url);
const searchParams = new URLSearchParams(url.search);
- searchParams.set("items", items?.join(",") || defaultItems);
+ const { publicProperties = {}, items } = incomingProps ?? {};
+ const resolvedItems = items?.length ? items.join(",") : defaultItems;
+ searchParams.set("items", resolvedItems);
const response = await vcs["POST /api/sessions"](
Object.fromEntries(searchParams.entries()),
{
body: {
public: {
...publicProperties,
},
},
headers: { cookie },
},
);📝 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.
| { | |
| publicProperties, | |
| items, | |
| }: SessionProps, | |
| req: Request, | |
| ctx: AppContext, | |
| ): Promise<CreateEditSessionResponse> { | |
| ): Promise<Session> { | |
| const { vcs } = ctx; | |
| const setCookiesSoFar = getSetCookies(ctx.response.headers); | |
| const { header: cookie } = buildCookieJar(req.headers, setCookiesSoFar); | |
| const url = new URL(req.url); | |
| const searchParams = new URLSearchParams(url.search); | |
| searchParams.set("items", items?.join(",") || defaultItems); | |
| const response = await vcs["POST /api/sessions"]({}, { | |
| body: { | |
| public: { | |
| ...props.publicProperties, | |
| const response = await vcs["POST /api/sessions"]( | |
| Object.fromEntries(searchParams.entries()), | |
| { | |
| body: { | |
| public: { | |
| ...publicProperties, | |
| }, | |
| }, | |
| headers: { cookie }, | |
| }, | |
| async function action( | |
| incomingProps: SessionProps | undefined, | |
| req: Request, | |
| ctx: AppContext, | |
| ): Promise<Session> { | |
| const { vcs } = ctx; | |
| const setCookiesSoFar = getSetCookies(ctx.response.headers); | |
| const { header: cookie } = buildCookieJar(req.headers, setCookiesSoFar); | |
| const url = new URL(req.url); | |
| const searchParams = new URLSearchParams(url.search); | |
| const { publicProperties = {}, items } = incomingProps ?? {}; | |
| const resolvedItems = items?.length ? items.join(",") : defaultItems; | |
| searchParams.set("items", resolvedItems); | |
| const response = await vcs["POST /api/sessions"]( | |
| Object.fromEntries(searchParams.entries()), | |
| { | |
| body: { | |
| public: { | |
| ...publicProperties, | |
| }, | |
| }, | |
| headers: { cookie }, | |
| }, | |
| ); | |
| // ... | |
| } |
🤖 Prompt for AI Agents
In vtex/actions/session/createSession.ts around lines 12 to 36, the function
destructures props directly which throws if props is undefined; update the code
to safely default the incoming payload before destructuring (e.g., accept props
parameter and use const { publicProperties = {}, items = undefined } = props ||
{} or set a default param of {}), then use items ?? defaultItems when building
searchParams and spread ...publicProperties safely; ensure headers/cookie logic
remains unchanged.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
vtex/utils/client.ts (2)
15-15: LogoutResponse import is used incorrectly for the logout endpoint.While the import statement itself is valid, the
LogoutResponsetype is applied incorrectly to the logout endpoint at line 316. Per previous review findings with documentation references, the VTEX logout endpoint does not return a JSON body withcookieNameandaccountCookieNamefields.
311-317: Critical: Incorrect response type for logout endpoint—returns 204/redirect, not JSON.The
LogoutResponsetype is incorrect for this endpoint. As documented in the previous review with citations to VTEX docs and community examples, the/api/vtexid/pub/logoutendpoint:
- ✓ Accepts
scopeandreturnUrlquery parameters (correct)- ✗ Does NOT return a JSON body with
cookieNameandaccountCookieName- Returns 204 No Content or performs a redirect to
returnUrlApply this diff to correct the response type:
"GET /api/vtexid/pub/logout": { searchParams: { scope?: string; returnUrl?: string; }; - response: LogoutResponse; + response: void; };Additionally, update any call sites consuming this endpoint to handle it as a side-effect operation (cookie clearing + optional redirect) rather than expecting a JSON response body.
vtex/actions/authentication/logout.ts (3)
44-47: Critical: Refresh token cookie not cleared due to wrong path.The refresh token cookie (
vid_rt) is scoped to path/api/vtexid/refreshtoken/webstore, but the code attempts to delete it using path"/". This will fail to clear the scoped cookie, leaving the user's session partially intact.Apply this diff to fix:
- deleteCookie(ctx.response.headers, REFRESH_TOKEN_COOKIE, { - path: "/", - domain: new URL(req.url).hostname, - }); + const VTEX_REFRESH_PATH = "/api/vtexid/refreshtoken/webstore"; + deleteCookie(ctx.response.headers, REFRESH_TOKEN_COOKIE, { + path: VTEX_REFRESH_PATH, + domain: new URL(req.url).hostname, + });
49-49: Normalize cookie Path for VTEX auth cookies.After proxying Set-Cookie headers, the Path attribute for VTEX authentication cookies (like
vid_rtandVtexIdclientAutCookie*) should be normalized to/api/vtexid/refreshtoken/webstoreto match the refresh-token scope. Without this, cookies may be set with incorrect paths.Add cookie path normalization after line 49:
proxySetCookie(response.headers, ctx.response.headers, req.url); + +// Normalize Path for VTEX auth cookies +const targetHost = new URL(req.url).hostname; +const VTEX_REFRESH_PATH = "/api/vtexid/refreshtoken/webstore"; +for (const cookie of getSetCookies(response.headers)) { + const needsAuthPath = + cookie.name === "vid_rt" || cookie.name.startsWith("VtexIdclientAutCookie"); + if (needsAuthPath) { + setCookie(ctx.response.headers, { + ...cookie, + domain: targetHost, + path: VTEX_REFRESH_PATH, + }); + } +}Add the required import at the top:
-// import { getCookies, getSetCookies } from "std/http/cookie.ts"; +import { getSetCookies, setCookie, deleteCookie } from "std/http/cookie.ts"; -import { deleteCookie } from "std/http/cookie.ts";
55-56: Major: Open redirect vulnerability in returnUrl.The
returnUrlparameter is user-controlled and passed directly toredirect()without validation. An attacker could supply an external URL (e.g.,https://evil.com) to redirect users to a phishing site after logout.Apply this diff to validate and restrict redirects to same-origin paths:
- redirect(returnUrl); + // Validate returnUrl to prevent open redirects + const safeLocation = (() => { + try { + if (!returnUrl) return "/"; + // Only allow relative paths starting with single "/" + if (returnUrl.startsWith("/") && !returnUrl.startsWith("//")) { + return returnUrl; + } + // Validate same-origin URLs + const dest = new URL(returnUrl, req.url); + const origin = new URL(req.url).origin; + return dest.origin === origin ? (dest.pathname + dest.search + dest.hash) : "/"; + } catch { + return "/"; + } + })(); + redirect(safeLocation);
🧹 Nitpick comments (1)
vtex/actions/authentication/logout.ts (1)
1-1: Remove or uncomment dead import statements.Commented-out imports suggest incomplete refactoring. Either remove them if unnecessary or uncomment if they're needed for the suggested fixes in past review comments.
Also applies to: 4-4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
vtex/actions/authentication/logout.ts(1 hunks)vtex/actions/cart/updateUser.ts(2 hunks)vtex/utils/client.ts(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
vtex/actions/cart/updateUser.ts (2)
vtex/utils/vtexId.ts (1)
parseCookie(16-36)vtex/utils/segment.ts (1)
getSegmentFromBag(67-69)
vtex/actions/authentication/logout.ts (3)
vtex/actions/session/validateSession.ts (1)
action(5-17)vtex/utils/types.ts (1)
LogoutResponse(1849-1852)vtex/utils/cookies.ts (2)
REFRESH_TOKEN_COOKIE(98-98)proxySetCookie(15-32)
vtex/utils/client.ts (1)
vtex/utils/types.ts (1)
LogoutResponse(1849-1852)
🔇 Additional comments (6)
vtex/utils/client.ts (1)
174-177: Let me search for the original path change to confirm the PR diff shows the removal of/api:Endpoint path is correct; no changes needed.
The VTEX API documentation shows the changeToAnonymousUser endpoint uses
/checkout/changeToAnonymousUser/without the/apiprefix. The code change aligns with the official API contract, and all usage in the codebase is consistent.vtex/actions/cart/updateUser.ts (5)
44-48: Good practice: Response body consumption for resource management.Properly consuming the response body before proxying cookies prevents resource leaks and ensures the response stream is fully read.
49-55: Robust error handling with appropriate logging.The catch block properly handles errors with descriptive logging and returns a safe boolean value.
5-5: Logger import is correct and follows established codebase conventions.The import of
loggerfrom@deco/deco/o11yis consistent with usage across 20+ files throughout the codebase (website, vtex, wake, blog, shopify, hubspot, konfidency, and other integrations), confirming this is the standard source for logging in the Deco framework.
22-36: Endpoint path change verified as correct.The VTEX Checkout API endpoint is
/checkout/changeToAnonymousUser/{orderFormId}, confirming the removal of the/apiprefix in the code is correct. Theredirect: "manual"header is a standard HTTP control mechanism and does not conflict with the API specification.
12-16: Verify the impact of this breaking change and consider API consistency.The return type change from
Promise<OrderForm>toPromise<boolean>is confirmed. The underlying VTEX API endpoint returnsOrderForm, and all similar cart actions (updateProfile,updateItems,updateGifts,updateCoupons, etc.) returnPromise<OrderForm>. The current implementation discards the API response and returns only a success/failure boolean. The only internal invocation inlogout.tsdoesn't capture the return value, so immediate codebase impact is minimal. However, this is a public API (registered inmanifest.gen.ts), and external consumers expectingOrderFormwill break. Confirm whether this change is intentional and whetherOrderFormshould be returned instead for API consistency.
| await ctx.invoke.vtex.actions.cart.updateUser(); | ||
| await ctx.invoke.vtex.actions.session | ||
| .validateSession(); |
There was a problem hiding this comment.
🧩 Analysis chain
Verify the invocation order and error handling.
The code invokes updateUser and validateSession sequentially without awaiting error handling. If either fails, the user is still redirected. Consider whether partial failure should prevent the redirect or be logged.
🏁 Script executed:
#!/bin/bash
# Description: Check error handling patterns in other authentication flows
# Search for similar invoke patterns to understand error handling conventions
rg -nP --type=ts -C5 'invoke\.vtex\.actions\.(cart|session)\.' | rg -A3 -B3 'await.*invoke'Length of output: 3960
---
🏁 Script executed:
```shell
#!/bin/bash
# Search for try-catch blocks in authentication flows to see if errors are handled
rg -nP --type=ts 'try\s*\{' vtex/actions/authentication/ -A10 | head -50
Length of output: 870
🏁 Script executed:
#!/bin/bash
# Search for error handling patterns around invoke calls specifically
rg -nP --type=ts 'catch.*invoke|\.catch\(' vtex/actions/authentication/ -B5 -A5Length of output: 0
🏁 Script executed:
#!/bin/bash
# Check if there's a higher-level error handler or middleware for actions
rg -nP --type=ts 'export.*default|handler|middleware' vtex/actions/authentication/logout.ts -B2 -A5Length of output: 262
---
🏁 Script executed:
```shell
#!/bin/bash
# View the full logout.ts file to see complete error handling context
cat vtex/actions/authentication/logout.ts
Length of output: 1482
Add error handling for invoke calls or document the intended failure behavior.
The code inconsistently handles errors: the initial logout API call (line 30) checks if (!response.ok), but the invoke calls at lines 51–53 lack error handling. If updateUser() or validateSession() fails, the function still redirects, leaving session state potentially inconsistent. Either wrap these invokes in a try-catch to handle partial failures explicitly, or document that framework-level error handling catches these failures.
🤖 Prompt for AI Agents
In vtex/actions/authentication/logout.ts around lines 51–53, the calls to
ctx.invoke.vtex.actions.cart.updateUser() and
ctx.invoke.vtex.actions.session.validateSession() lack error handling which can
leave session state inconsistent; wrap these invoke calls in a try-catch (cover
both calls together), on error log the exception with context and decide
behavior: either propagate the error (rethrow) to prevent the redirect or record
a recoverable warning and continue; ensure the chosen path is consistent with
the earlier response.ok check (i.e., if partial failure should abort logout,
rethrow after logging; if it can be tolerated, log a warning and proceed).
…ateSession on successful auth status
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
vtex/actions/authentication/classicSignIn.ts (1)
80-89: Stop reissuing vid_rt at "/" without expiring the default path.This creates two
vid_rtcookies (one at "/" and one at "/api/vtexid/refreshtoken/webstore"), causing nondeterministic behavior on logout and session validation. This issue was flagged in previous reviews but remains unaddressed.Apply this diff to expire the legacy path before setting the new cookie:
- // TODO: REMOVE THIS AFTER TESTING AND VALIDATE IF NEEDED REWRITE REFRESH_TOKEN_COOKIE - const setCookies = getSetCookies(ctx.response.headers); - for (const cookie of setCookies) { - if (cookie.name === REFRESH_TOKEN_COOKIE) { - setCookie(ctx.response.headers, { - ...cookie, - path: "/", // default path is /api/vtexid/refreshtoken/webstore, but browser dont send to backend headers - }); - } - } + // If rewriting is required, expire the legacy path first to avoid duplicates + const setCookies = getSetCookies(ctx.response.headers); + const rt = setCookies.find((c) => c.name === REFRESH_TOKEN_COOKIE); + if (rt && rt.path && rt.path !== "/") { + // Expire the original cookie at its original path + setCookie(ctx.response.headers, { + name: rt.name, + value: "", + domain: rt.domain, + path: rt.path, + secure: rt.secure, + httpOnly: rt.httpOnly, + sameSite: rt.sameSite, + maxAge: 0, + expires: new Date(0), + }); + // Set the new cookie at "/" + setCookie(ctx.response.headers, { ...rt, path: "/" }); + }Note: Consider whether path rewriting is truly necessary. The comment "browser dont send to backend headers" suggests a misunderstanding—browsers send cookies when the request path matches the cookie's path. Verify the actual requirement before implementing this workaround.
🧹 Nitpick comments (1)
vtex/actions/authentication/classicSignIn.ts (1)
3-3: Remove the commented-out import.This commented import indicates work in progress. Clean it up before merging.
-// import setLoginCookies from "../../utils/login/setLoginCookies.ts";
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
vtex/actions/authentication/accessKeySignIn.ts(4 hunks)vtex/actions/authentication/classicSignIn.ts(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- vtex/actions/authentication/accessKeySignIn.ts
🧰 Additional context used
🧬 Code graph analysis (1)
vtex/actions/authentication/classicSignIn.ts (1)
vtex/utils/cookies.ts (3)
buildCookieJar(70-94)proxySetCookie(15-32)REFRESH_TOKEN_COOKIE(98-98)
🔇 Additional comments (3)
vtex/actions/authentication/classicSignIn.ts (3)
34-35: LGTM!The cookie jar correctly merges incoming request cookies with Set-Cookie headers from
startAuthentication, ensuring both client and server state are propagated to the validation endpoint.
49-61: LGTM!The validation request correctly includes the cookie header, ensuring VTEX receives all necessary cookies for authentication.
70-79: The cookie proxying and session validation flow is correct, but path handling is problematic.The sequence of proxying cookies, calling
refreshToken, and validating the session is sound. However, theproxySetCookiecall doesn't enforce the correct path for auth cookies, and the subsequent path rewrite at lines 80-89 introduces duplicate cookies (see separate comment).Consider implementing the path normalization directly in the
proxySetCookiecall as suggested in previous reviews, rather than rewriting afterwards.
1 participant
T
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores