From ac2309c182fc026da7df82c2c03009b51a1698db Mon Sep 17 00:00:00 2001 From: Samuel Newman Date: Fri, 17 Jul 2026 16:48:12 +0300 Subject: [PATCH] snapshot session after prep, skip no-op cross-tab rebuilds, restore chat proxy env Review fixes (PR #11182 round 3): - The session factories snapshotted the account BEFORE the prep awaits (moderation config, age-assurance prefetch) with hooks still disarmed. A 401 during prep triggers PasswordSession's internal auto-refresh, rotating both tokens; the dropped onUpdated meant the stale refreshJwt got persisted (dead on next cold start). The returned account is now re-snapshotted after prep, right before arm() - matching the old BskyAppAgent.prepare ordering. Tests pin the mid-prep rotation case. - Cross-tab sync rebuilt and killed the active bundle on ANY saved- account change; now bails out when the current account's tokens are unchanged. - buildChatClient hard-coded the SDK's prod chat DID, making the env- configurable CHAT_PROXY_DID dead code and routing staging DMs to prod chat; new CHAT_PROXY_SERVICE constant restores the override. - The cross-tab labeler reapply was fire-and-forget; the bundle swap now defers until the labeler config resolves, matching the factories. Note: the reviewer's URL.canParse/ReadableStream claim was verified INCORRECT - Expo 54's WinterCG runtime defines URL.canParse and Metro injects a ReadableStream polyfill (expo/virtual/streams.js) into dev and release bundles. Co-Authored-By: Claude Fable 5 --- src/lib/constants.ts | 10 +- .../session/__tests__/session-core-test.ts | 122 +++++++++++++++++- src/state/session/clients.ts | 20 ++- src/state/session/index.tsx | 69 ++++++---- src/state/session/session-core.ts | 66 ++++++++-- 5 files changed, 244 insertions(+), 43 deletions(-) diff --git a/src/lib/constants.ts b/src/lib/constants.ts index ad1d70ff01..330cac7a70 100644 --- a/src/lib/constants.ts +++ b/src/lib/constants.ts @@ -2,7 +2,7 @@ import {type Insets, Platform} from 'react-native' import {type Service} from '@atproto/lex' import {api} from '@bsky.app/sdk' -import {BLUESKY_PROXY_DID, IS_DEV} from '#/env' +import {BLUESKY_PROXY_DID, CHAT_PROXY_DID, IS_DEV} from '#/env' import {type app} from '#/lexicons' /** @@ -265,6 +265,14 @@ export const BLUESKY_MOD_SERVICE_HEADERS = { */ export const NOTIF_SERVICE = `${BLUESKY_PROXY_DID}#bsky_notif` as Service +/** + * Service proxy identifier for the chat service. Passed as the `service` option + * on the chat client so lex-client emits the `atproto-proxy` header (replaces + * the old per-call `DM_SERVICE_HEADERS`). Env-configurable via + * `EXPO_PUBLIC_CHAT_PROXY_DID` (see {@link CHAT_PROXY_DID}). + */ +export const CHAT_PROXY_SERVICE = `${CHAT_PROXY_DID}#bsky_chat` as Service + export const webLinks = { tos: `https://bsky.social/about/support/tos`, privacy: `https://bsky.social/about/support/privacy-policy`, diff --git a/src/state/session/__tests__/session-core-test.ts b/src/state/session/__tests__/session-core-test.ts index 20f693611c..cad8efa37a 100644 --- a/src/state/session/__tests__/session-core-test.ts +++ b/src/state/session/__tests__/session-core-test.ts @@ -3,7 +3,7 @@ import { type PasswordSessionOptions, type SessionData, } from '@atproto/lex-password-session' -import {describe, expect, it, jest} from '@jest/globals' +import {beforeEach, describe, expect, it, jest} from '@jest/globals' import {type SessionAccount} from '../types' @@ -27,11 +27,45 @@ jest.mock('#/state/queries/messages/restrictChatSettings', () => ({ restrictChatSettings: () => Promise.resolve(), })) +/* + * The factory tail awaits `features.refresh(...)`; stub the analytics module so + * the factory does not pull GrowthBook (and its native deps) into this + * lightweight suite. + */ +jest.mock('#/analytics', () => ({ + features: {refresh: () => Promise.resolve()}, +})) + +/* + * `configureModerationForAccount` is one of the prep awaits in the resume/login + * factories. We replace it with a hook that runs a REAL `session.refresh()`, so + * a token rotation happens DURING prep (before arm()) - exactly the 401 + * auto-refresh scenario the re-snapshot fix guards against. The default is a + * no-op so other tests are unaffected; individual tests install the refreshing + * behavior via `mockImplementationOnce`. (jest requires out-of-scope factory + * references to be `mock`-prefixed.) + */ +const mockConfigureModerationForAccount = + jest.fn<(bundle: unknown, account: unknown) => Promise>() +jest.mock('../moderation', () => ({ + configureModerationForAccount: (bundle: unknown, account: unknown) => + mockConfigureModerationForAccount(bundle, account), + configureModerationForGuest: () => {}, +})) + jest.mock('jwt-decode', () => ({ jwtDecode(token: string) { if (token === 'queued-access-jwt') { return {scope: 'com.atproto.signupQueued'} } + /* + * A far-future exp so isSessionExpired() reads this stored token as still + * valid, which routes resume() through the sync (no-network) fast path. + * That isolates the prep-time refresh as the ONLY token rotation. + */ + if (token === 'valid-access-jwt') { + return {scope: 'com.atproto.access', exp: 4102444800} + } return {scope: 'com.atproto.access'} }, })) @@ -785,3 +819,89 @@ describe('refreshSession semantics', () => { await expect(session.refresh()).rejects.toBeDefined() }) }) + +/* + * Fix 1: the resume/login factories must snapshot the RETURNED account AFTER + * the prep awaits, not before. A 401 during prep triggers PasswordSession's + * internal auto-refresh (rotating BOTH tokens and firing an onUpdated the + * disarmed latch drops); an early snapshot would persist the stale refreshJwt, + * which is dead on the next cold start. + * + * We simulate the mid-prep rotation by making the mocked + * `configureModerationForAccount` (a genuine prep await in each factory) run a + * real `session.refresh()`. The factory itself is re-required inside + * `jest.isolateModulesAsync` AFTER overriding `globalThis.fetch`, because + * session-core captures `globalThis.fetch` into `networkAwareFetch` at module + * load - and that captured fetch is what PasswordSession's auto-refresh routes + * through. + */ +describe('factory account snapshot is taken AFTER prep (fix 1)', () => { + /** Load a fresh session-core whose networkAwareFetch captures `fetch`. */ + async function withFreshFactory( + fetch: typeof globalThis.fetch, + run: (core: typeof import('../session-core')) => Promise, + ) { + const realFetch = globalThis.fetch + globalThis.fetch = fetch + try { + await jest.isolateModulesAsync(async () => { + const core = + require('../session-core') as typeof import('../session-core') + await run(core) + }) + } finally { + globalThis.fetch = realFetch + } + } + + beforeEach(() => { + mockConfigureModerationForAccount.mockReset() + }) + + it('resume: returned account carries the tokens rotated DURING prep', async () => { + /* + * A refresh mid-prep rotates the session to access-jwt-2/refresh-jwt-2. + * `valid-access-jwt` decodes as non-expired, so resume() takes the sync + * fast path and the only refresh is the one prep triggers. + */ + mockConfigureModerationForAccount.mockImplementationOnce( + async (bundle: unknown) => { + await (bundle as SessionBundle).session.refresh() + }, + ) + const fetchMock = makeMockFetch() + + await withFreshFactory(asFetch(fetchMock), async core => { + const {account, bundle} = await core.createSessionBundleAndResume( + makeAccount({accessJwt: 'valid-access-jwt'}), + jest.fn(), + ) + /* the moderation prep step ran the refresh */ + expect(mockConfigureModerationForAccount).toHaveBeenCalledTimes(1) + /* the RETURNED account carries the POST-prep (rotated) tokens */ + expect(account.accessJwt).toBe('access-jwt-2') + expect(account.refreshJwt).toBe('refresh-jwt-2') + /* and it matches the session's committed state */ + expect(bundle.session.session.accessJwt).toBe('access-jwt-2') + }) + }) + + it('resume: returned account falls back to the stored account when the fast path yields no live token change', async () => { + /* + * Control: no mid-prep refresh. The re-snapshot still reflects the (still + * valid) stored tokens, confirming the moved snapshot did not regress the + * happy path. + */ + mockConfigureModerationForAccount.mockResolvedValueOnce(undefined) + const fetchMock = makeMockFetch() + + await withFreshFactory(asFetch(fetchMock), async core => { + const {account} = await core.createSessionBundleAndResume( + makeAccount({accessJwt: 'valid-access-jwt'}), + jest.fn(), + ) + expect(account.accessJwt).toBe('valid-access-jwt') + expect(account.refreshJwt).toBe('refresh-jwt') + }) + }) +}) diff --git a/src/state/session/clients.ts b/src/state/session/clients.ts index 242631f38e..1004a98488 100644 --- a/src/state/session/clients.ts +++ b/src/state/session/clients.ts @@ -2,7 +2,11 @@ import {Client} from '@atproto/lex' import {type PasswordSession} from '@atproto/lex-password-session' import {api} from '@bsky.app/sdk' -import {BLUESKY_PROXY_HEADER, PUBLIC_BSKY_SERVICE} from '#/lib/constants' +import { + BLUESKY_PROXY_HEADER, + CHAT_PROXY_SERVICE, + PUBLIC_BSKY_SERVICE, +} from '#/lib/constants' import {networkAwareFetch} from './session-core' /** @@ -41,14 +45,16 @@ export function buildAccountClient(session: PasswordSession): Client { /** * Build the chat client over a {@link PasswordSession}. * - * `api.chat.service` (`did:web:api.bsky.chat#bsky_chat`) is passed as the - * client's `service`, so lex-client sets `atproto-proxy: did:web:api.bsky.chat#bsky_chat` - * on every request. This is exactly what the old per-call `DM_SERVICE_HEADERS` - * did, once and centrally, so `chat.bsky.*` calls are proxied to the chat - * service. + * {@link CHAT_PROXY_SERVICE} (`${CHAT_PROXY_DID}#bsky_chat`, default + * `did:web:api.bsky.chat#bsky_chat`) is passed as the client's `service`, so + * lex-client sets `atproto-proxy: ` on every request. This is + * exactly what the old per-call `DM_SERVICE_HEADERS` did, once and centrally, so + * `chat.bsky.*` calls are proxied to the chat service. It is read from the + * env-configurable `CHAT_PROXY_DID` (via `EXPO_PUBLIC_CHAT_PROXY_DID`) rather + * than the hard-coded SDK constant, restoring the old routing override. */ export function buildChatClient(session: PasswordSession): Client { - return new Client(session, {service: api.chat.service}) + return new Client(session, {service: CHAT_PROXY_SERVICE}) } /** Thrown when a write/auth-only client is used with no active session. */ diff --git a/src/state/session/index.tsx b/src/state/session/index.tsx index 148f5cef92..cc037af90f 100644 --- a/src/state/session/index.tsx +++ b/src/state/session/index.tsx @@ -420,6 +420,26 @@ export function Provider({children}: React.PropsWithChildren<{}>) { const prevBundle = state.currentAgentState.agent as unknown as | SessionBundle | PublicSessionBundle + /* + * Any change to ANY saved account fires persisted.onUpdate. The + * 'synced-accounts' dispatch above already keeps the accounts list + * fresh, so if the CURRENT account's tokens are unchanged there is + * nothing to rebuild - a change to a non-current account landed here. + * Bail out before rebuilding: rebuild+swap would kill the live bundle + * (client-identity churn, in-flight request kills) for no reason. + * Fall through to rebuild only when we have no usable live session. + */ + const live = + prevBundle.session && !prevBundle.session.destroyed + ? prevBundle.session.session + : undefined + if ( + live && + live.accessJwt === syncedAccount.accessJwt && + live.refreshJwt === syncedAccount.refreshJwt + ) { + return + } let newBundle!: SessionBundle const hooks = makeSessionHooks( onSessionChange, @@ -435,30 +455,35 @@ export function Provider({children}: React.PropsWithChildren<{}>) { hooks.arm() /* * Reapply this account's subscribed labelers to the freshly built - * appview client. buildBundle starts with an empty per-instance - * labeler set, and unlike login/resume/createAccount this rebuild path - * never ran configureModerationForAccount - so without this, subscribed - * labelers would silently drop from appview requests in follower tabs - * until the next full resume. readLabelers is a local storage read (no - * network), preserving the branch's no-network intent; setLabelers - * mutates the client's Set in place so labels reappear when it - * resolves. + * appview client - buildBundle starts with an empty per-instance + * labeler set, and unlike login/resume/createAccount this rebuild + * path never runs configureModerationForAccount on its own. The + * bundle swap is deferred until the labeler config resolves so the + * new bundle enters the reducer with its labelers already applied + * (matching the factories, which await moderation before returning). + * readLabelers is a local-storage read (microtask-scale, no network, + * preserving this branch's no-network intent), and the OLD bundle's + * access token stays valid throughout the deferral, so nothing + * regresses by waiting. */ void configureModerationForAccount(newBundle, syncedAccount) - addSessionDebugLog({ - type: 'agent:patch', - agent: newBundle, - prevSession: - prevBundle.session && !prevBundle.session.destroyed - ? prevBundle.session.session - : undefined, - nextSession: newBundle.session.session, - }) - store.dispatch({ - type: 'replaced-current-bundle', - newAgent: newBundle, - newAccount: syncedAccount, - }) + .catch(() => {}) + .finally(() => { + addSessionDebugLog({ + type: 'agent:patch', + agent: newBundle, + prevSession: + prevBundle.session && !prevBundle.session.destroyed + ? prevBundle.session.session + : undefined, + nextSession: newBundle.session.session, + }) + store.dispatch({ + type: 'replaced-current-bundle', + newAgent: newBundle, + newAccount: syncedAccount, + }) + }) } } }) diff --git a/src/state/session/session-core.ts b/src/state/session/session-core.ts index 8f9a80e65e..14ce81a2d6 100644 --- a/src/state/session/session-core.ts +++ b/src/state/session/session-core.ts @@ -534,16 +534,32 @@ export async function createSessionBundleAndResume( bundle = buildBundle(session) registerBundleKillSwitch(bundle, hooks.kill) - const account = + /* + * Early snapshot: only used to configure moderation below (its handle/did are + * refresh-stable). The RETURNED account is re-snapshotted after the prep + * awaits (see below). + */ + const earlyAccount = sessionDataToSessionAccount(session.session, session.session.service) ?? storedAccount - const moderation = configureModerationForAccount(bundle, account) + const moderation = configureModerationForAccount(bundle, earlyAccount) const aa = prefetchAgeAssuranceServerData({ appviewClient: bundle.appviewClient, accountClient: bundle.accountClient, }) await Promise.all([gates, moderation, aa]) + /* + * Re-snapshot AFTER prep, right before arm(). A 401 during a prep request + * (e.g. the AA prefetch) triggers PasswordSession's internal auto-refresh, + * which rotates both tokens; its onUpdated is dropped by the still-disarmed + * latch. Snapshotting the returned account here (not before prep) ensures we + * persist the fresh refreshJwt rather than a stale one that is dead on the + * next cold start. + */ + const account = + sessionDataToSessionAccount(session.session, session.session.service) ?? + storedAccount hooks.arm() return {account, bundle} } @@ -587,16 +603,28 @@ export async function createSessionBundleAndLogin( bundle = buildBundle(session) registerBundleKillSwitch(bundle, hooks.kill) - const account = sessionDataToSessionAccountOrThrow(session) - accountDid = account.did + /* + * Early snapshot: needed now to seed `accountDid` (the getDid closure the + * hooks read). The RETURNED account is re-snapshotted after the prep awaits. + */ + const earlyAccount = sessionDataToSessionAccountOrThrow(session) + accountDid = earlyAccount.did const gates = features.refresh({strategy: 'prefer-fresh-gates'}) - const moderation = configureModerationForAccount(bundle, account) + const moderation = configureModerationForAccount(bundle, earlyAccount) const aa = prefetchAgeAssuranceServerData({ appviewClient: bundle.appviewClient, accountClient: bundle.accountClient, }) await Promise.all([gates, moderation, aa]) + /* + * Re-snapshot AFTER prep, right before arm(). A 401 during a prep request + * triggers PasswordSession's internal auto-refresh, which rotates both tokens + * and fires an onUpdated the disarmed latch drops; snapshotting here persists + * the fresh refreshJwt. If the session was destroyed mid-prep, OrThrow throws + * (login effectively failed). + */ + const account = sessionDataToSessionAccountOrThrow(session) hooks.arm() return {account, bundle} } @@ -655,11 +683,17 @@ export async function createSessionBundleAndCreateAccount( bundle = buildBundle(session) registerBundleKillSwitch(bundle, hooks.kill) - const account = sessionDataToSessionAccountOrThrow(session) - accountDid = account.did + /* + * Early snapshot: needed now to seed `accountDid` and for the DID/handle used + * across the local writes and deferred server writes below (all + * refresh-stable). The RETURNED account is re-snapshotted after the prep + * awaits. + */ + const earlyAccount = sessionDataToSessionAccountOrThrow(session) + accountDid = earlyAccount.did const gates = features.refresh({strategy: 'prefer-fresh-gates'}) - const moderation = configureModerationForAccount(bundle, account) + const moderation = configureModerationForAccount(bundle, earlyAccount) const createdAt = toDatetimeString(new Date()) const birthdate = birthDate.toISOString() @@ -670,9 +704,9 @@ export async function createSessionBundleAndCreateAccount( * to the server in the next step, so on subsequent reloads, the server will * be the source of truth. */ - setCreatedAtForDid({did: account.did, createdAt}) - setBirthdateForDid({did: account.did, birthdate}) - snoozeBirthdateUpdateAllowedForDid(account.did) + setCreatedAtForDid({did: earlyAccount.did, createdAt}) + setBirthdateForDid({did: earlyAccount.did, birthdate}) + snoozeBirthdateUpdateAllowedForDid(earlyAccount.did) // do this last const aa = prefetchAgeAssuranceServerData({ appviewClient: bundle.appviewClient, @@ -725,7 +759,7 @@ export async function createSessionBundleAndCreateAccount( }), // wait for AA data to load first, then check state aa.then(() => { - const {flags} = unsafeGetAndComputeAgeAssurance({did: account.did}) + const {flags} = unsafeGetAndComputeAgeAssurance({did: earlyAccount.did}) if (flags?.chatDisabled || flags?.groupChatDisabled) { void restrictChatSettings({ client: bundle.accountClient, @@ -786,6 +820,14 @@ export async function createSessionBundleAndCreateAccount( } await Promise.all([gates, moderation, aa]) + /* + * Re-snapshot AFTER prep, right before arm(). A 401 during a prep request + * triggers PasswordSession's internal auto-refresh, which rotates both tokens + * and fires an onUpdated the disarmed latch drops; snapshotting here persists + * the fresh refreshJwt rather than a stale one. If the session was destroyed + * mid-prep, OrThrow throws. + */ + const account = sessionDataToSessionAccountOrThrow(session) hooks.arm() return {account, bundle} }