fix(storefront): Fix checkout blocked by censored profile cached on browser - #808
Conversation
…rowser Profiles identified by e-mail and document only used to be returned with censored fields: "***" surname, "000XX" phone and address holding just zip and province. Sessions persisted at that time are still cached on customer browsers and get resubmitted on every checkout, where they overwrite the saved profile and produce an incomplete shipping address that payment gateways reject. Detect those fields when loading the persisted session and drop the cached profile, so it is fetched again once authenticated, and filter them out on fetchCustomer so a profile already stored censored is not reused as valid. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
RevisãoAnalisei a mudança e confirmei os padrões de detecção contra a origem da censura em
Detecção conservadora, sem risco realista de falso-positivo. 👍 Observação (a única que vale mexer)Os dois caminhos tratam a censura de formas diferentes:
O comentário justifica o "drop entirely" para forçar o refetch removendo Dá pra ter os dois: preservar os campos válidos e ainda forçar o refetch, aplicando Fora de escopo (ok deixar como follow-up, já mapeado no corpo do PR)
Corpo do PR está excelente — problema, impacto medido e follow-ups explícitos. Aprovo com a observação acima. |
…ched session Previously the whole cached profile was dropped when a censored field was detected, forcing the customer to re-enter name, phone and address even when those fields were still valid. That extra friction hit exactly the customers whose sessions were already affected. Reuse withoutCensoredFields on load so only the censored fields are removed while valid ones are kept, and drop doc_number so fetchCustomer still runs to refresh from the server once the customer is authenticated. Both the load and fetch paths now handle censored data the same way. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
leomp12
left a comment
There was a problem hiding this comment.
O diagnóstico está certo e a investigação que sustenta esta PR é o melhor pedaço dela. Levantar 16 checkouts com dado censurado em 30 dias, cruzar com o estado atual do banco e separar os 14 que são cadastro já corrompido ecoando de volta dos 2 que partiram de cadastro limpo é o que transforma "cliente reclamou" em "sei qual é o vetor". E a conclusão que sai daí — que o navegador ainda contamina perfil saudável — é o que justifica mexer no cliente e não só limpar dado.
Confirmei também o que você levantou no seu comentário: os três padrões de detecção batem com o maskCustomerFields (serve-passport-api.ts:19-46). Fui além e olhei o histórico, porque a máscara já teve outro formato: a variante '***' + lastTwo do 5360ed1bb nunca foi liberada isolada (ela e o 3203eedbe saíram juntas na v2.10.0), então não existe cache de produção com telefone '***12'. A detecção cobre o que de fato foi gravado.
O momento escolhido para a limpeza também está certo, e por um motivo que vale registrar porque não é óbvio: como o vbeta-app importa o customer-session, o bloco do topo é avaliado antes do watch(customer, …, { immediate: true }) de vbeta-app.ts:276-287. Naquele instante window.ecomPassport ainda não existe, então o watcher cai no else do :282 — localStorage.setItem, overwrite total — e a limpeza alcança também o ecomPassportClient, que é o cache que o app de checkout lê (vbeta-app.ts:129). Se a purga tivesse ido para dentro do onAuthStateChanged, chegaria depois daquele store já semeado, e aí teria que brigar com o setCustomer, que no bundle da CDN é Object.assign(e.customer, o) — merge raso, incapaz de apagar chave. Adiantar foi a decisão certa.
O que me trava são duas consequências do saneamento, as duas em cima da mesma população que a PR quer resgatar.
🔴 Bloqueante — apagar o name mata um auto-reparo que hoje funciona, e arrisca derrubar o checkout
customer-session.ts:54 só preserva o name quando ele está limpo:
if (name && name.family_name !== CENSORED) cleanCustomer.name = name;Sobrenome censurado, então, remove o objeto inteiro — inclusive o given_name, que a máscara preserva de propósito (serve-passport-api.ts:19-22). Três consequências:
1. O servidor já conserta esse caso sozinho, e o marcador é a chave. checkout.ts:95-97:
if (customer.name.family_name?.includes('***') && savedCustomer.name) {
customer.name = savedCustomer.name;
}Hoje o body chega com { given_name: 'Maria', family_name: '***' } e o sobrenome real é restaurado a partir do cadastro salvo. Sem o marcador, esse reparo nunca dispara — o cliente passa a ter que redigitar o que antes voltava sozinho. O mesmo vale para o telefone em :98.
2. name é obrigatório no contrato. packages/modules/schemas/@checkout.cjs:861,864 — required: ['main_email', 'name'], com additionalProperties: false. A validação ajv roda em checkout.ts:43, antes do backfill de :89-94, então campo ausente não é recuperado: é 400 @checkout.
3. E se passar da validação, dois desreferenciamentos sem guarda. checkout.ts:74 faz Object.values(customer.name).some(testXss) e read-or-save-customer.ts:41 faz customer.name.given_name || 'visitor'. Ambos estouram em TypeError com name indefinido. A forma "customer com display_name mas sem name" nunca existiu nesse pipeline antes desta PR.
Não consegui fechar se o app.js reconstrói o name a partir do display_name antes de montar o body — o bundle é CDN (@ecomplus/storefront-app@2.0.0-beta.228) e não está no repo. Se não reconstrói, a PR troca "endereço recusado pelo gateway" por "checkout recusado na validação", para a mesma população. Vale confirmar antes do merge, mas mesmo no melhor cenário a perda do given_name válido e do auto-reparo já não se pagam.
Direção: zerar só o family_name em vez de deletar o objeto — manter name: { given_name } preserva o dado bom, mantém o contrato satisfeito e deixa o reparo do servidor funcionando. phones e addresses não têm esse problema: phones é opcional e o read-or-save-customer faz backfill, e addresses nem entra no customer do checkout (vai em shipping.to).
🔴 Bloqueante — o fetchCustomer() destravado pode prender o isAuthReady em false
O mecanismo da PR é apagar o doc_number (:65) justamente para destravar o :170. Só que o destino desse await é um callback sem try/catch:
onAuthStateChanged(firebaseAuth, async (user) => {
...
isAuthReady.value = false; // :158
if (user.emailVerified) {
if (isEmailChanged || !session.customer.doc_number) {
await fetchCustomer(); // :171
}
}
isAuthReady.value = true; // :175 — não alcançado se :171 rejeitar
});:171 é a única chamada de fetchCustomer() e não há try/catch em nenhum ponto da cadeia. Os outros dois pontos que setam isAuthReady = true (:181 e :188) rodam sincronamente na inicialização, dentro do branch de isSignInWithEmailLink — antes do callback assíncrono sequer disparar. Depois que :158 zera, o :175 é o único caminho de volta.
E fetchCustomer() rejeita por caminhos banais: authenticate() chama throwNoAuth em :99 fora do try/catch de :103-114; se o fetch do /passport/token falha, o catch engole e :121 dispara throwNoAuth(); e se o resAuth.json() traz corpo de erro, isAuthenticated vira NaN > 10000 → false, o accessToken sai undefined e o api.get responde 401.
Com isAuthReady preso em false:
| Consumidor | Efeito |
|---|---|
use-login-form.ts:50-52 |
isSubmitReady nunca fica true — botão de login travado |
vbeta-app.ts:339-354 (rota #account) |
initializingAuth nunca settla; o .then não roda e o .catch também não, porque a promise não rejeita, só pendura. loadAppScript() nunca é chamado — a área de conta não carrega |
vbeta-app.ts:355-372 (token velho) |
Salvo pelo Promise.race com timeout de 10s do :371 — o checkout carrega, 10s atrasado |
A rota #account é a única sem rede de proteção. E é regressão de verdade: antes da PR essa população tinha doc_number em cache, o :170 dava falso e o fetchCustomer() nunca rodava — o travamento era inalcançável. A purga é exatamente o que o torna alcançável.
Direção: try/catch em volta do :171, ou isAuthReady.value = true num finally. É uma linha e fecha os três consumidores.
🟠 A detecção é a terceira cópia no repo, e é a mais estreita das três
customer-session.ts:35-45 reimplementa predicados que já existem no servidor, em checkout.ts, com regras que não batem:
| campo | checkout.ts |
esta PR |
|---|---|---|
name.family_name |
.includes('***') (:95) |
=== '***' |
phones[].number |
/^0{3,}\d{1,4}$/ (:98) |
/^0{3}\d{2}$/ |
addresses[].name |
.includes('***') (:114) |
=== '***' |
Para a saída atual do maskCustomerFields as duas versões casam, então isso não é bug hoje — por isso não é bloqueante. O custo é de manutenção: '***' vira literal mágico em três pacotes com três dialetos, e o próximo ajuste vai ser feito na cópia que o dev achar primeiro. Como a PR já lista checkout.ts como território de follow-up, vale pelo menos alinhar no .includes agora, e considerar um predicado compartilhado quando os follow-ups forem feitos.
🟠 A limpeza não sobrevive a duas abas abertas
O bloco roda uma única vez, na avaliação do módulo. Mas o useStorage sincroniza abas por BroadcastChannel, e o merge é o deepMergeState de use-storage.ts:5-23, que itera Object.keys(newState) e nunca remove chave:
- Aba A limpa, grava o
ecomSessionsemname/phones/addresses, posta'set'. - Aba B (aberta antes, estado censurado em memória) recebe e faz
deepMergeState(limpo, censurado)— as chaves ausentes no payload continuam lá. - O
watchDebouncedda aba B regrava oecomSessioncensurado e posta'set'. - Aba A re-mergeia e volta a ficar contaminada.
Como a premissa da PR é que o snapshot persiste no navegador, uma aba velha desfaz o fix. Não bloqueia porque cura no reload da aba velha, mas vale saber que existe.
🟠 doc_number como sentinela matricula essa população num refetch por page load
O loop não é novo — quem já estava sem doc_number sempre caiu nele pelo :170. O que a PR faz é mover a população censurada para dentro desse balde, e doc_number é opcional no registro (customers.d.ts:209) e nunca é copiado pelo maskCustomerFields (quem injeta é o identify, a partir do corpo do request — serve-passport-api.ts:79). Cliente cujo cadastro no servidor não tem doc_number passa a fazer authenticate() + GET customers/{id} a cada navegação, permanentemente — e sorteia o travamento do bloqueante anterior a cada vez. Uma marca explícita de "já saneei" seria mais honesta que reaproveitar doc_number como sentinela.
🟢 Minors
customer-session.ts:61— semif (!import.meta.env.SSR), enquanto os três vizinhos que fazem efeito no boot têm (modules-info.ts:64,ab-experiment.ts:96,shopping-cart.ts:201). Inofensivo hoje, mas nesse caminho osessioné proxy sobre oemptySessionmodule-level.customer-session.ts:131— o comentário diz "Profiles already persisted", mas alidataé resposta fresca da API autenticada (:128), não perfil persistido. É o único dos comentários novos que não se paga; os outros dois registram história e acoplamento que o código não carrega, e esses eu manteria.customer-session.ts:41-45e:46-60—hasCensoredFieldsewithoutCensoredFieldsrepetem a lista de campos sem vínculo. Um quarto campo mascarado adicionado só ao filtro desativa o saneamento em silêncio.customer-session.ts:66-67vs:132— o invariante dedisplay_name/main_emailsempre presentes é remendado num call site e não no outro.fetchCustomerpode gravar umsession.customersem essas chaves, que é o shape quecustomerEmail(:77-84),logout(:206) evbeta-app.ts:277assumem.customer-session.ts:36-60— 25 linhas de helper puro inline, enquanto a convenção do próprio diretório é extrair (shopping-cart.ts:8-10importa destate/shopping-cart/).
🔭 Fora do diff, mas é o outro caminho de limpeza do mesmo arquivo
emptySession (:12-18) é objeto mutável compartilhado entre o initialValue do useStorage e o reset do logout() (:206-207). Como use-storage.ts:47 faz reactive(persistedValue || initialValue), num page load que começou sem ecomSession o state é o proxy do emptySession — e o logout() vira auto-atribuição, no-op completo: o token sobrevive, isAuthenticated segue true e a UI continua logada. Depois do primeiro logout() no caso com valor persistido, escritas seguintes poluem a constante, e um logout → login (outro cliente) → logout na mesma página restaura um "session vazio" com display_name/main_email do cliente anterior.
É pré-existente e não é escopo desta PR, mas é literalmente o outro mecanismo de limpar sessão no arquivo que você está mexendo, e vale abrir issue — não achei nenhuma no backlog que cubra isso, nem os seis follow-ups de packages/modules/src/firebase/ que você listou no corpo da PR.
Pra entrar antes do merge
- Preservar
name: { given_name }em vez de deletar o objeto — é o que mantém o contrato do@checkout, o auto-reparo decheckout.ts:95-97e ogiven_nameválido. try/catch(oufinally) em volta dofetchCustomer()do:171— destrava login e#accountquando o refresh falha.- Confirmar se o
app.jsmonta onamedo body a partir dodisplay_name. Se não montar, o item 1 deixa de ser prudência e vira a diferença entre checkout funcionando e400.
Os dois 🟠 de meio (abas e sentinela) dão para tratar em follow-up, mas queria tua leitura antes de abrir issue.
Uma ressalva de método: não rodei nada além de leitura e rastreamento de fluxo. packages/storefront não tem suíte de teste, e o test-apps.yml exclui esse path explicitamente, então nem lint nem typecheck rodaram no CI desta PR — só o CodeFactor. QA manual é o único gate real aqui, e o cenário que mais precisa dele é o cliente com perfil censurado chegando no checkout.
…ng censored session Address review points from PR #808: - `name` is no longer dropped when clearing a censored cached session: it's required on `@checkout` contract, `given_name` is preserved valid by the mask, and the '***' marker is what triggers server side restore from the saved profile on checkout.ts; - `authenticate()`/`fetchCustomer()` failures on auth state change no longer keep `isAuthReady` stuck false, which was locking login submit and the account page load; - Censored fields predicates aligned with server side checks (`.includes('***')` and `/^0{3,}\d{1,4}$/` as checkout.ts); - Session purge on module boot guarded with `!import.meta.env.SSR` as sibling states do. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…er keys consistent Remaining review points from PR #808: - `emptySession` shared mutable constant replaced by a factory: `useStorage` may keep the initial value as the live reactive state, so the `logout()` reset was a self-assignment no-op (token surviving and UI still logged) and later writes polluted the "empty" constant with the previous customer's data; - `fetchCustomer` now keeps the `display_name`/`main_email` always-set invariant that consumers assume, as the boot purge already did; - Censored session helpers extracted to `state/customer-session/` per directory convention (as `state/shopping-cart/`). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Os dois bloqueantes estão resolvidos, e conferi cada um contra o que motivou o apontamento: O O Além do pedido, entraram a guarda de SSR, o comentário do Um efeito novo, criado pela própria correçãoPreservar o O gatilho continua sendo
Isso não afeta quem tem o cadastro limpo no servidor: ali o O custo é um Direção: o gatilho precisa deixar de ser "existe marcador?" e passar a ser "a limpeza mudou alguma coisa?". Comparar o resultado de Continuam em aberto, e para mim seguem valendo follow-up
Os dois se resolvem juntos com o ajuste do gatilho, então talvez valha tratar na mesma passada em vez de abrir issue. Fora isso, da minha parte está pronto para mergear assim que o gatilho parar de rodar em toda navegação. E vale lembrar que aqui o único gate real é QA manual — |
`hasCensoredFields` was testing `name.family_name`, but `withoutCensoredFields`
deliberately keeps `name` so the `@checkout` contract stays satisfied and the
'***' marker still triggers the server side restore. The marker therefore never
left the session, keeping the check true forever: every page load dropped
`doc_number` and made `onAuthStateChanged` refetch `customers/{id}`.
It only settled for profiles clean on the server, so it stuck exactly on the
corrupted ones the fix targets, and silently, since the new try/catch swallows
the failure.
The check now covers only what the cleanup actually removes, censored phones
and addresses, so it runs once per contaminated session and turns false
afterwards. A profile censored only on the surname stops being purged at all,
which is correct: there is nothing to remove and checkout.ts restores it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013J3PpxE1ScfqPwmG9rEBUZ
|
Uma correção no que escrevi acima: eu disse que a ressurreição entre abas e a sentinela E, olhando melhor, o problema das abas é mais estreito do que registrei. Depois do deploy toda aba roda a purga ao carregar, então duas abas pós-deploy nunca se recontaminam — o merge de estado limpo com estado limpo é limpo. O cenário exige uma aba aberta de antes do deploy, que não tem o código da purga, sobrescrevendo por Corrigir de verdade exigiria mudar o |
leomp12
left a comment
There was a problem hiding this comment.
Aprovado.
Os dois bloqueantes que eu tinha levantado estão resolvidos, e conferi cada um contra o que motivou o apontamento: o name preservado mantém o contrato do @checkout e o restore de checkout.ts:95-97, e o try/catch garante que o isAuthReady seja sempre alcançado. Entraram junto a guarda de SSR, os predicados alinhados com o servidor, os helpers extraídos, e o emptySession virando factory — que eu tinha registrado como fora de escopo e fecha um vazamento de PII entre clientes em dispositivo compartilhado.
Empurrei um commit no branch, f93d478, e a decisão é discutível, então reverta sem dó se discordar. Preservar o name — que é a correção certa — tinha removido a condição de parada do saneamento: como o hasCensoredFields testava name.family_name e o marcador agora nunca sai da sessão, o gatilho ficava permanentemente verdadeiro, e cliente com cadastro corrompido no servidor passava a dropar doc_number e refazer customers/{id} em toda navegação, em silêncio. O commit restringe o gatilho ao que a limpeza de fato remove — telefone e endereço —, então ele roda uma vez por sessão contaminada e depois fica falso. Cadastro censurado só no sobrenome deixa de ser purgado, o que é o comportamento correto: não há o que remover e o servidor já conserta no checkout.
Verificação: ESLint limpo nos dois arquivos. tsc --noEmit acusa apenas os dois import.meta.env conhecidos — um é o que já existe em main, o outro veio da guarda de SSR desta PR. Como packages/storefront não tem suíte e o test-apps.yml exclui esse path, validei a lógica numa reprodução isolada dos dois predicados, com cinco cenários: sobrenome censurado, telefone censurado, endereço censurado, sobrenome + endereço, e perfil limpo. Todos terminam no primeiro page load e o name é preservado em todos.
Segurança: o try/catch não abre bypass — isAuthReady só governa prontidão de UI, e a autorização continua vindo de session.auth.expires e de emailVerified. A purga só reduz PII em cache local, e nada novo é gravado ou transmitido.
Duas ressalvas explícitas: o commit que resolve o ponto que eu mesmo levantei é meu, então quem valida o test plan é o @vitorrgg; e o único gate real aqui é QA manual, em especial um cliente com sessão censurada chegando ao checkout. A ressurreição entre abas fica registrada no comentário acima como aresta conhecida, com recomendação de não mexer.
Problem
Customer profiles identified by e-mail and document only — without verified login — used to be returned censored by the passport:
***surname,000XXphone, and an address holding onlyzip,province_codeand a truncatedline_address.That behaviour is off today (
PASSPORT_UNVERIFIED_AUTH), but the censored snapshots persisted back then are still cached in customer browsers underecomSession, andfetchCustomer()only runs again when the e-mail changes ordoc_numberis missing — so they never refresh.Every checkout by those customers resubmits the censored profile. Two consequences:
readOrSaveCustomerwrites the censored values over any field the saved profile is missing, corrupting a previously clean record — permanently, since the existing guards restore name and phone from the saved profile.line_1: "undefined,undefined,undefined"and a422from the gateway. The checkout aborts withCKT704and the order is left with no financial status.The customer sees an error that explains nothing, retries, and fails identically every time.
Observed impact
Investigated on two stores over a 30-day log window:
A full sweep of barradoce's 56.612 customer records found 132 corrupted: 89 with censored surname, 6 with the phone reduced to five digits, 5 with the address destroyed — those 5 cannot complete a purchase at all — and 47 with truncated order history (this last group still needs confirmation, part of it may have another cause).
Cross-checking the 16 checkouts against the current database state: 14 are records already corrupted echoing back, and 2 came from a clean record — meaning the browser cache is what still contaminates healthy profiles.
Change
In
packages/storefront/src/lib/state/customer-session.ts:display_nameandmain_email. Removingdoc_numbermakesfetchCustomer()run again as soon as the customer is authenticated.fetchCustomer()response, so a profile already stored censored server-side is not reused as if valid. The customer is then asked for the address instead of submitting an unusable one.Detection is deliberately narrow, matching only what the censoring produced:
family_name === '***', phone matching/^0{3}\d{2}$/, and address withname === '***'or***insideline_address.Scope
This stops the propagation vector. It does not repair the already corrupted records — that needs a separate data cleanup — and it does not fix the checkout module, where the same censored payload is still accepted and persisted. Follow-ups worth doing in
packages/modules/src/firebase/:checkout.ts:79—readOrSaveCustomerreceives the payload before the uncensoring at:88.checkout.ts:88— the uncensoring is gated bycustomerId === customer._id, so it is skipped exactly on the record-creation path.checkout.ts:94-99— restoring fromsavedCustomerreapplies the censored values when the saved record is itself corrupted, so no profile ever recovers.checkout.ts:81— the address guard tests onlyline_address.read-or-save-customer.ts:26-33— any non-404 read failure falls through toapi.post('customers'), creating a duplicate with the censored values and no log.orders[]is accepted from the browser and has no reason to be.Verification
eslint -c ../eslint/storefront.staged.eslintrc.cjs src/lib/state/customer-session.ts— clean.tsc --noEmitreports only the pre-existingimport.meta.enverror, present onmainat the same line.🤖 Generated with Claude Code