fix: forward auth token only via account-scoped cookie#3381
Conversation
lemagnetic
requested review from
hellofanny and
lucasfp13
and removed request for
a team
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
💤 Files with no reviewable changes (2)
WalkthroughThis PR refactors VTEX authentication cookie handling by removing header-based auth extraction and shifting to explicit cookie-forwarding. The credential validation now derives tokens from merged cookie sets, ensuring request body tokens align with storage updates, while authentication flows exclusively through the ChangesVTEX Auth Cookie Forwarding
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/api/test/integration/vtex.cookies.test.ts (1)
104-165: 💤 Low valueConsider moving unit tests to a unit test location.
These tests verify individual utility functions in isolation (no network calls, no API integration). Per coding guidelines,
@faststore/apishould separate unit tests (*.test.ts) from integration tests (*.int.test.ts). This file is named*.test.tsbut lives intest/integration/.Either move to a unit test directory or rename to
*.int.test.tsif the intent is to run with integration test suites.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/api/test/integration/vtex.cookies.test.ts` around lines 104 - 165, Tests in test/integration cover a pure utility (getWithAutCookie) and should be classified consistently: either move this file to the unit-test suite or mark it as an integration test. Fix by either (A) relocating the file to the unit tests folder and updating any import-relative paths so getWithAutCookie imports still resolve, or (B) renaming the test file from *.test.ts to *.int.test.ts so the integration runner picks it up; ensure the test name continues to reference getWithAutCookie and that CI/test config (if any) does not require additional changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/api/test/integration/vtex.cookies.test.ts`:
- Around line 104-165: Tests in test/integration cover a pure utility
(getWithAutCookie) and should be classified consistently: either move this file
to the unit-test suite or mark it as an integration test. Fix by either (A)
relocating the file to the unit tests folder and updating any import-relative
paths so getWithAutCookie imports still resolve, or (B) renaming the test file
from *.test.ts to *.int.test.ts so the integration runner picks it up; ensure
the test name continues to reference getWithAutCookie and that CI/test config
(if any) does not require additional changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4fef2328-3d26-4fb4-a1ab-e290d6e9c2c4
📒 Files selected for processing (3)
packages/api/src/platforms/vtex/clients/commerce/index.tspackages/api/src/platforms/vtex/utils/cookies.tspackages/api/test/integration/vtex.cookies.test.ts
renatomaurovtex
left a comment
renatomaurovtex
left a comment
There was a problem hiding this comment.
Reviewed the auth-forwarding change. The root cause (duplicated token: account-scoped cookie + top-level VtexIdclientAutCookie header, with the platform 401-ing on the duplicate) is well explained, and the fix matches the established cookie-only pattern (getAuthCookie, getJWTAutCookie, getIsRepresentative). Tests are solid and the regression case for the unscoped header is a nice touch. A few things to confirm before merge.
🔴 [security] Auth path change — needs a human sign-off
This touches the authentication path (vtexid.validate backing the @auth directive). Per our review limits I won't solo-approve an auth change; flagging for a maintainer to confirm the platform contract (cookie-only forwarding for credential/validate) and to green-light the merge.
🟠 [bug] Blast radius is wider than the PR body — withAutCookie feeds ~10 other endpoints
getWithAutCookie previously attached VtexIdclientAutCookie to every caller, not just vtexid.validate. After this change none of these get it anymore:
profile.addresses→profile-system/pvt/profiles/{userId}/addressesoms.userOrder→oms/user/orders/{orderId}oms.getCommercialAuthorizationsByOrderId/processOrderAuthorizationunits.*→units/v1/...licenseManager.*→license-manager/...
The PR description only validates the My Account /pvt/account/* flow. Did you verify these resolvers (especially license-manager and commercial-authorizations) still authenticate via the account-scoped cookie alone? Reassuringly, oms.listUserOrders already uses withCookie (no VtexIdclientAutCookie header) against the same OMS API, so the precedent looks right — but a quick confirmation (or a test step covering one B2B/orders path) would de-risk the wider change.
💬 [nit] _account param is now dead weight
withAutCookie(forwardedHost, _account?) keeps the param only for call-site compatibility. Fine to leave it, but since all 11 call sites are in this same file you could drop the second arg entirely and remove the account passthrough — slightly cleaner. Non-blocking, your call.
Verdict: Approved with comments
Blocking (🔴/🟠):
- 🔴 Auth-path change — requires a maintainer sign-off (can't be auto-approved).
- 🟠 Confirm the now-unscoped endpoints (
license-manager,units,commercial-authorizations,profile,oms.userOrder) still authenticate via the account-scoped cookie.listUserOrdersprecedent suggests yes; a test step or note would close it out.
Non-blocking (🟡/💬):
- 💬 Drop the unused
_accountparam if you want the small cleanup.
Checks to confirm before merge: pnpm lint · build · pnpm --filter @faststore/api test
hellofanny
left a comment
hellofanny
left a comment
There was a problem hiding this comment.
- pvt/account/profile ✅
- pvt/account/orders ✅
Those seems to be working as expected.
But I not sure if pvt/account/security should also be working here. It returns 403. Do you know if It was pre-existing issue (I think probably was..)? Or licenseManager endpoint (ServerSecurity query calls userDetails → licenseManager.getUserRoles, which uses withAutCookie) may require the explicit VtexIdclientAutCookie header.
eduardoformiga
left a comment
eduardoformiga
left a comment
There was a problem hiding this comment.
Please, take a look at this
| * Headers for authenticated VtexID requests. The auth token is carried by | ||
| * the forwarded `cookie` header — not duplicated as a separate header. | ||
| */ | ||
| export const getWithAutCookie = (ctx: ContextForCookies) => { |
There was a problem hiding this comment.
I think this function no longer makes sense.
since getWithAutCookie didn't return the AutCookie (VtexIdclientAutCookie) anymore.
We can use instead, directly the:
const headers: HeadersInit = withCookie({
'content-type': 'application/json',
'X-FORWARDED-HOST': forwardedHost,
})
@faststore/api
@faststore/cli
@faststore/components
@faststore/core
@faststore/diagnostics
@faststore/lighthouse
@faststore/sdk
@faststore/ui
commit: |
|
0 New Issues
0 Fixed Issues
0 Accepted Issues
## What's the purpose of this pull request? fix(api): use account-scoped cookie in orderEntry headers The orderEntry methods (file upload via OES, #3334) still referenced the withAutCookie helper that #3381 removed, since that PR was not rebased on the latest dev and did not see the newly merged orderEntry block. This left an undefined withAutCookie reference, breaking the build and tests on dev. Replace the 5 withAutCookie(forwardedHost, account) calls with the withCookie({ 'content-type', 'X-FORWARDED-HOST' }) pattern adopted by #3381 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved authentication/header handling for order entry endpoints to ensure requests consistently include the correct `Content-Type` and forwarded host information. * Ensures file upload continues to use `multipart/form-data` with the proper boundary while retaining the updated base header behavior. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
## Summary Graduate the `4.3.0-dev.8` prerelease cycle to the stable `4.3.0` release on `latest`. Merging this PR triggers CD on `main`, which runs `lerna version --conventional-graduate` and publishes all `@faststore/*` packages to the `latest` dist-tag. ### Features - Password Protection (v4) (#3276) - File upload via Order Entry Service (OES) (#3334) - Add sitemap to CMS Landing Page content-type (#3386) ### Bug Fixes - **core:** use authenticator route for setpassword (#3380) - migrate partytown `@builder.io` → `@qwik.dev@0.14.0` (#3394) - **api:** use account-scoped cookie in orderEntry headers (#3395) - forward auth token only via account-scoped cookie (#3381) - **core:** propagate upstream error status instead of always 500 (#3379) ## Pre-flight (faststore-release skill) - Working tree clean; `release.yml` has `fetch-depth: 0` - All 8 packages have `repository.url` - No breaking changes since `v4.2.0` - `dev → main` merges cleanly (no CHANGELOG/codegen conflicts) - No unmerged hotfixes on `main` (accidental `4.2.1` was fully reverted in #3375) ## Test plan - [ ] CI green on this PR - [ ] After merge, confirm CD publishes all 8 packages at `4.3.0` under `latest` Made with [Cursor](https://cursor.com) --------- Co-authored-by: Matheus P. Silva <cout.matheusps@gmail.com> Co-authored-by: vtexgithubbot <vtexgithubbot@github.com> Co-authored-by: Lucas Feijó <lucas.portela@vtex.com> Co-authored-by: Luiz Falcão <39093175+llfalcao@users.noreply.github.com> Co-authored-by: Artur Santiago <artur.santiago@cubos.io> Co-authored-by: Larícia Mota <laricia.mota@vtex.com.br> Co-authored-by: Sahan Jayawardana <sahan@clouda.io> Co-authored-by: Mateus Pontes <mateuspo10@gmail.com> Co-authored-by: Matheus Martins <mathews_2010@outlook.com> Co-authored-by: renatomaurovtex <167437775+renatomaurovtex@users.noreply.github.com> Co-authored-by: Leandro Rodrigues <leandro.rodrigues@vtex.com> Co-authored-by: Fanny Chien <fanny.chien@vtex.com> Co-authored-by: Arthur Andrade <arthurfelandrade@gmail.com> Co-authored-by: Leandro Rodrigues <leandro.swf@gmail.com> Co-authored-by: Cursor <cursoragent@cursor.com> Co-authored-by: Thiago Pereira <thiago.pereira@vtex.com> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: Giuliana Rigaud <62848434+giurigaud@users.noreply.github.com> Co-authored-by: renato <renato.neto@cubos.io> Co-authored-by: Bruna Santos <brunassdev@gmail.com> Co-authored-by: BrunaCubos <104789782+BrunaCubos@users.noreply.github.com> Co-authored-by: Ícaro Oliveira <icarovinici@gmail.com> Co-authored-by: Bruna Santos <bruna.santos@cubos.io> Co-authored-by: Everton Ataide <everton.ataide@vtex.com> Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> Co-authored-by: Otavio Moreira Meirelles <otavio.meirelles@vtex.com> Co-authored-by: Marco Cardoso <marcopaulo@outlook.com> Co-authored-by: Gabriel Paladino <gabpaladino@users.noreply.github.com> Co-authored-by: CodeRabbit <noreply@coderabbit.ai> Co-authored-by: dk-portal[bot] <134092483+dk-portal[bot]@users.noreply.github.com> Co-authored-by: Rodrigo Tavares <rodrigo.tavares@vtex.com> Co-authored-by: vitorflg <vitor.gomes@vtex.com> Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com> Co-authored-by: Válber Laux <valber.laux@vtex.com>
4 participants
What's the purpose of this pull request?
Logged-in shoppers were unable to access the FastStore My Account area: any navigation to
/pvt/account/*resulted in an immediate redirect to/pvt/account/403?from=..., where the page entered an infinite refresh-token loop.The root cause was on the API side. Every
@auth-protected GraphQL query relies onvtexid.validate, and the request that backs that call was forwarding the auth token in two ways at once: once through the standard account-scoped cookie and again under a duplicated, top-level header. The duplicated header was being rejected by the platform with401, even though the user's session was perfectly valid.How it works?
This PR removes the duplication. The auth token is now forwarded exclusively through the account-scoped cookie (
VtexIdclientAutCookie_{account}) — exactly like every other authenticated path in the codebase already does (seegetAuthCookie,getJWTAutCookie,getIsRepresentative).How to test it?
Pre-conditions: a logged-in shopper on a store that has FastStore My Account enabled (
experimental.enableFaststoreMyAccount = true).Navigate to
/pvt/account/profile— should render the profile page directly, without redirecting to/pvt/account/403.Run the package tests:
pnpm --filter @faststore/api testStarters Deploy Preview
Before
Gravacao.de.Tela.2026-05-05.as.19.26.38.mov
References
@faststore/apipackages/core/src/pages/pvt/account/*@authdirective (packages/api/src/directives/auth.ts) →validateUserAuthentication→vtexid.validateSummary by CodeRabbit
Bug Fixes
Tests