diff --git a/src/lib/moderation.ts b/src/lib/moderation.ts index f93d7fc1f9..6b0ef23a2e 100644 --- a/src/lib/moderation.ts +++ b/src/lib/moderation.ts @@ -1,5 +1,5 @@ import {useMemo} from 'react' -import {api} from '@bsky.app/sdk' +import {Client} from '@atproto/lex-client' import { type InterpretedLabelValueDefinition, LABELS, @@ -104,10 +104,11 @@ export function isAppLabeler( | app.bsky.labeler.defs.LabelerView | app.bsky.labeler.defs.LabelerViewDetailed, ): boolean { + const appLabelers = Client.appLabelers as readonly string[] if (typeof labeler === 'string') { - return labeler === api.moderation.did + return appLabelers.includes(labeler) } - return labeler.creator.did === api.moderation.did + return appLabelers.includes(labeler.creator.did) } export function isLabelerSubscribed( diff --git a/src/lib/xrpc-error.ts b/src/lib/xrpc-error.ts index dd9e3ab9d3..2ccf720a72 100644 --- a/src/lib/xrpc-error.ts +++ b/src/lib/xrpc-error.ts @@ -1,4 +1,4 @@ -import {XrpcError, XrpcResponseError} from '@atproto/lex-client' +import {LexError, XrpcError, XrpcResponseError} from '@atproto/lex-client' /** * True for an XRPC error from a lex `Client` (`@atproto/lex-client` `XrpcError`, @@ -18,9 +18,16 @@ export function getErrorStatus(e: unknown): number | undefined { return e instanceof XrpcResponseError ? e.status : undefined } -/** The lexicon error code (`err.error`). */ +/** + * The lexicon error code (`err.error`). Gated on `LexError` (the base of the + * lex error hierarchy) rather than `XrpcError` so sibling `LexError` subclasses + * that are NOT `XrpcError` also surface their `.error` - notably + * `LexAuthFactorError` (`'AuthFactorTokenRequired'`), which `PasswordSession` + * throws for email-2FA logins. Every `XrpcError` is a `LexError`, so all + * existing call sites keep working. + */ export function getErrorName(e: unknown): string | undefined { - return isXrpcError(e) ? (e as {error?: string}).error : undefined + return e instanceof LexError ? e.error : undefined } /** diff --git a/src/state/session/__tests__/session-core-test.ts b/src/state/session/__tests__/session-core-test.ts index 90efc73760..20f693611c 100644 --- a/src/state/session/__tests__/session-core-test.ts +++ b/src/state/session/__tests__/session-core-test.ts @@ -38,8 +38,10 @@ jest.mock('jwt-decode', () => ({ import { type AtpSessionEvent, + disposeBundle, extractPdsUrl, makeSessionHooks, + registerBundleKillSwitch, sessionAccountToSessionData, type SessionBundle, sessionDataToSessionAccount, @@ -425,7 +427,12 @@ describe('makeSessionHooks arm-latch + event mapping', () => { function setup() { const onSessionChange = jest.fn< - (bundle: SessionBundle, did: string, event: AtpSessionEvent) => void + ( + bundle: SessionBundle, + did: string, + event: AtpSessionEvent, + sessionData?: SessionData, + ) => void >() /* the hook only passes this through by identity; a stub bundle suffices */ const bundle = {} as SessionBundle @@ -443,7 +450,7 @@ describe('makeSessionHooks arm-latch + event mapping', () => { expect(onSessionChange).not.toHaveBeenCalled() }) - it("maps onUpdated -> 'update' after arm(), passing the bundle through", () => { + it("maps onUpdated -> 'update' after arm(), passing the bundle + payload through", () => { const {onSessionChange, bundle, hooks} = setup() hooks.arm() void hooks.onUpdated?.call(fakeSession, fakeData) @@ -451,6 +458,8 @@ describe('makeSessionHooks arm-latch + event mapping', () => { expect(onSessionChange.mock.calls[0][0]).toBe(bundle) expect(onSessionChange.mock.calls[0][1]).toBe(DID) expect(onSessionChange.mock.calls[0][2]).toBe('update') + /* the fresh SessionData the library delivers is threaded through verbatim */ + expect(onSessionChange.mock.calls[0][3]).toBe(fakeData) }) it("maps onDeleted -> 'expired' after arm()", () => { @@ -470,6 +479,169 @@ describe('makeSessionHooks arm-latch + event mapping', () => { ) expect(onSessionChange.mock.calls[0][2]).toBe('network-error') }) + + it("does NOT thread a payload on the 'expired' path", () => { + /* + * onDeleted maps to 'expired' with no sessionData. The provider guards on + * `event === 'update' && sessionData`, so a missing payload here is what + * forces refreshedAccount === undefined (reducer clears tokens + logs out). + */ + const {onSessionChange, hooks} = setup() + hooks.arm() + void hooks.onDeleted?.call(fakeSession, fakeData) + expect(onSessionChange.mock.calls[0][2]).toBe('expired') + expect(onSessionChange.mock.calls[0][3]).toBe(undefined) + }) +}) + +/* + * The exact derivation from the provider's onSessionChange (index.tsx). Pinned + * here because the payload threading (session-core) and this mapping together + * are the fix: read tokens from the delivered payload on 'update', and force + * undefined on the drop paths so the reducer logs the user out. + */ +function deriveRefreshedAccount( + event: AtpSessionEvent, + sessionData?: SessionData, +): SessionAccount | undefined { + return event === 'update' && sessionData + ? sessionDataToSessionAccount(sessionData, sessionData.service) + : undefined +} + +/* + * Pins the pre-commit ordering bug fix. `PasswordSession` fires onUpdated with + * the fresh session BEFORE committing it internally, so the live getter is + * still stale at hook time. Driven through the real library (not a hand-rolled + * fixture) so the ordering is authentic. + */ +describe('session-hook payload threading (pre-commit ordering)', () => { + it('delivers the NEW tokens via the payload even though the live getter is still pre-commit stale', async () => { + const fetchMock = makeMockFetch() + let session!: PasswordSession + let liveGetterAtHookTime: SessionAccount | undefined + let refreshedAccountAtHookTime: SessionAccount | undefined + const onSessionChange = jest.fn( + ( + _bundle: SessionBundle, + _did: string, + event: AtpSessionEvent, + sessionData?: SessionData, + ) => { + /* what the OLD code did: snapshot the live (mutable) getter */ + liveGetterAtHookTime = sessionDataToSessionAccount( + session.session, + session.session.service, + ) + /* what the fix does: derive from the delivered payload */ + refreshedAccountAtHookTime = deriveRefreshedAccount(event, sessionData) + }, + ) + const hooks = makeSessionHooks( + onSessionChange, + () => ({}) as SessionBundle, + () => DID, + ) + session = new PasswordSession(sessionAccountToSessionData(makeAccount()), { + ...hooks, + fetch: asFetch(fetchMock), + }) + hooks.arm() + + await session.refresh() + + /* pre-commit ordering: at hook time the live getter still held OLD tokens */ + expect(liveGetterAtHookTime?.accessJwt).toBe('access-jwt') + /* the fix reads the fresh tokens from the payload the hook delivered */ + expect(refreshedAccountAtHookTime?.accessJwt).toBe('access-jwt-2') + expect(refreshedAccountAtHookTime?.refreshJwt).toBe('refresh-jwt-2') + /* and the session does eventually commit those same tokens */ + expect(session.session.accessJwt).toBe('access-jwt-2') + }) + + it("yields refreshedAccount === undefined on the 'expired' path (forces logout)", async () => { + const fetchMock = makeMockFetch({ + 'com.atproto.server.refreshSession': () => + new Response( + JSON.stringify({error: 'ExpiredToken', message: 'Token expired'}), + {status: 400, headers: {'content-type': 'application/json'}}, + ), + }) + let refreshedAccountAtHookTime: SessionAccount | undefined = makeAccount() + let observedEvent: AtpSessionEvent | undefined + const onSessionChange = jest.fn( + ( + _bundle: SessionBundle, + _did: string, + event: AtpSessionEvent, + sessionData?: SessionData, + ) => { + observedEvent = event + refreshedAccountAtHookTime = deriveRefreshedAccount(event, sessionData) + }, + ) + const hooks = makeSessionHooks( + onSessionChange, + () => ({}) as SessionBundle, + () => DID, + ) + const session = new PasswordSession( + sessionAccountToSessionData(makeAccount()), + {...hooks, fetch: asFetch(fetchMock)}, + ) + hooks.arm() + + await expect(session.refresh()).rejects.toBeDefined() + + expect(observedEvent).toBe('expired') + expect(refreshedAccountAtHookTime).toBe(undefined) + }) +}) + +/* + * Pins the disposal kill-switch (fix 3). `PasswordSession` exposes no local + * destroy, so disposeBundle neutralizes the session by tripping the flag inside + * the injected fetch - after disposal every request (direct or auto-refresh, + * which shares this same captured fetch) throws before touching the network. + */ +describe('disposeBundle kill-switch', () => { + it('the injected fetch throws after disposeBundle', () => { + const hooks = makeSessionHooks( + jest.fn(), + () => ({}) as SessionBundle, + () => DID, + ) + /* the injected fetch is the kill-switch wrapper makeSessionHooks bakes in */ + const injectedFetch = hooks.fetch! + + /* + * A live session is required for disposeBundle to act (it early-returns on + * a null/destroyed session). + */ + const session = new PasswordSession( + sessionAccountToSessionData(makeAccount()), + {...hooks}, + ) + const bundle = {session} as unknown as SessionBundle + registerBundleKillSwitch(bundle, hooks.kill) + + /* + * Before disposal the wrapper does NOT throw synchronously - it delegates + * to the async networkAwareFetch and returns a promise. Swallow that + * promise's rejection (the real network is unavailable under jest); we only + * care that no synchronous throw happened here. + */ + const pending = injectedFetch('https://bsky.social/xrpc/x') + expect(pending).toBeInstanceOf(Promise) + void pending.catch(() => {}) + + disposeBundle(bundle) + + /* after disposal every call through the injected fetch throws */ + expect(() => injectedFetch('https://bsky.social/xrpc/x')).toThrow( + 'session disposed', + ) + }) }) /* diff --git a/src/state/session/index.tsx b/src/state/session/index.tsx index c7c863fa33..442f724325 100644 --- a/src/state/session/index.tsx +++ b/src/state/session/index.tsx @@ -9,7 +9,7 @@ import { useSyncExternalStore, } from 'react' import {type Client} from '@atproto/lex-client' -import {PasswordSession} from '@atproto/lex-password-session' +import {PasswordSession, type SessionData} from '@atproto/lex-password-session' import * as persisted from '#/state/persisted' import {useCloseAllActiveElements} from '#/state/util' @@ -19,6 +19,7 @@ import {IS_WEB} from '#/env' import {com} from '#/lexicons' import {emitSessionDropped} from '../events' import {getPublicLexClient, getUnauthenticatedClient} from './clients' +import {configureModerationForAccount} from './moderation' import {type Action, getInitialState, reducer, type State} from './reducer' import { type AtpSessionEvent, @@ -29,6 +30,7 @@ import { disposeBundle, makeSessionHooks, type PublicSessionBundle, + registerBundleKillSwitch, sessionAccountToSessionData, type SessionBundle, sessionDataToSessionAccount, @@ -131,14 +133,23 @@ export function Provider({children}: React.PropsWithChildren<{}>) { bundle: SessionBundle, accountDid: string, sessionEvent: AtpSessionEvent, + sessionData?: SessionData, ) => { - // Snapshot the (mutable) live session data right away. + /* + * Build the refreshed account from the payload the hook delivers, NOT the + * live session getter. `PasswordSession` fires onUpdated/onDeleted BEFORE + * it commits its internal `#sessionData` (see password-session.js), so at + * hook time `bundle.session.session` still holds the OLD tokens (and, on + * the expiry path, `destroyed` is still false). Reading the live getter + * here would (a) persist stale tokens on 'update' -> eventual forced + * logout once the real refresh token expires, and (b) keep the user + * signed in on 'expired'. On 'update' the payload carries the new session; + * on 'expired'/'create-failed' we force it undefined so the reducer clears + * tokens and logs out (it treats undefined as "session gone"). + */ const refreshedAccount = - bundle.session && !bundle.session.destroyed - ? sessionDataToSessionAccount( - bundle.session.session, - bundle.session.session.service, - ) + sessionEvent === 'update' && sessionData + ? sessionDataToSessionAccount(sessionData, sessionData.service) : undefined if (sessionEvent === 'expired' || sessionEvent === 'create-failed') { emitSessionDropped() @@ -420,7 +431,20 @@ export function Provider({children}: React.PropsWithChildren<{}>) { hooks, ) newBundle = buildBundle(newSession) + registerBundleKillSwitch(newBundle, hooks.kill) 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. + */ + void configureModerationForAccount(newBundle, syncedAccount) addSessionDebugLog({ type: 'agent:patch', agent: newBundle, diff --git a/src/state/session/session-core.ts b/src/state/session/session-core.ts index 2557c71244..8bad74d1d7 100644 --- a/src/state/session/session-core.ts +++ b/src/state/session/session-core.ts @@ -295,6 +295,31 @@ export type SessionBundle = { readonly service: URL } +/** + * Kill-switches for live bundles, keyed by bundle identity. + * + * `PasswordSession` exposes no local (logout-free) destroy, so disposal is + * implemented via a closure flag inside the session's injected `fetch` (see + * {@link makeSessionHooks}). The `kill()` that trips that flag is produced next + * to the hooks - before the bundle exists - so we stash it here once the bundle + * is built and look it up in {@link disposeBundle}. A `WeakMap` keeps this off + * the {@link SessionBundle} type (the reducer's opaque view must not see it) and + * lets the entry be GC'd with the bundle. + */ +const bundleKillSwitches = new WeakMap void>() + +/** + * Associate a bundle with the `kill()` from its {@link makeSessionHooks}, so + * {@link disposeBundle} can neutralize the underlying session. Call once, right + * after the bundle is built, at every session-construction site. + */ +export function registerBundleKillSwitch( + bundle: SessionBundle, + kill: () => void, +) { + bundleKillSwitches.set(bundle, kill) +} + /** * Assemble a {@link SessionBundle} from a live session: the account, appview, * and chat clients, all read-through views over the one session. The appview @@ -331,11 +356,20 @@ export function buildBundle(session: PasswordSession): SessionBundle { * -> `'expired'`, transient failure -> `'network-error'`. The whole * {@link SessionBundle} is handed through so the provider can snapshot the live * session and use the bundle itself as the reducer's identity token. + * + * `sessionData` is the fresh payload the library hands the hook. It matters + * because `PasswordSession` fires `onUpdated`/`onDeleted` BEFORE committing + * `#sessionData` (see `refresh()`/`logout()` in password-session.js), so the + * live getter (`bundle.session.session`) still returns the OLD tokens at hook + * time. The provider must build the refreshed account from this argument, not + * from the live getter. Present on the `'update'` path (the new session) and + * absent on the error paths. */ type OnSessionChange = ( bundle: SessionBundle, did: string, event: AtpSessionEvent, + sessionData?: SessionData, ) => void /** @@ -352,6 +386,20 @@ type OnSessionChange = ( * are constructed (the session is created first, then the bundle is built over * it). * + * The hooks thread the fresh `SessionData` the library delivers straight + * through to `onSessionChange` (the `'update'` payload). The library fires the + * hook BEFORE committing that data internally, so the provider must read tokens + * from this argument rather than the (still-stale) live session getter. + * + * The `fetch` option is wrapped in a kill-switch: `kill()` (returned alongside + * `arm()`) sets a closure flag so every subsequent request through this + * session - direct fetches AND the internal auto-refresh, which + * `PasswordSession` routes through the same `options.fetch` captured at + * construction - throws instead of hitting the network. `kill()` also disarms + * the hooks so a disposed session can never dispatch into the reducer. This is + * the disposal mechanism {@link disposeBundle} relies on (`PasswordSession` + * exposes no local destroy). + * * Exported for testing (the arm-latch + event mapping is the core semantics). */ export function makeSessionHooks( @@ -360,12 +408,13 @@ export function makeSessionHooks( getDid: () => string, ) { let armed = false - const dispatch = (event: AtpSessionEvent) => { + let killed = false + const dispatch = (event: AtpSessionEvent, sessionData?: SessionData) => { if (!armed) { return } const did = getDid() - onSessionChange(getBundle(), did, event) + onSessionChange(getBundle(), did, event, sessionData) /* * Mirror the old BskyAppAgent.prepare wiring: log any non-create/update * session event. In practice we only emit 'update'/'expired'/'network-error' @@ -376,9 +425,14 @@ export function makeSessionHooks( } } const hooks: PasswordSessionOptions = { - fetch: networkAwareFetch, - onUpdated() { - dispatch('update') + fetch: (input, init) => { + if (killed) { + throw new Error('session disposed') + } + return networkAwareFetch(input, init) + }, + onUpdated(data) { + dispatch('update', data) }, onDeleted() { dispatch('expired') @@ -391,6 +445,10 @@ export function makeSessionHooks( arm() { armed = true }, + kill() { + killed = true + armed = false + }, }) } @@ -475,6 +533,7 @@ export async function createSessionBundleAndResume( } bundle = buildBundle(session) + registerBundleKillSwitch(bundle, hooks.kill) const account = sessionDataToSessionAccount(session.session, session.session.service) ?? storedAccount @@ -527,6 +586,7 @@ export async function createSessionBundleAndLogin( }) bundle = buildBundle(session) + registerBundleKillSwitch(bundle, hooks.kill) const account = sessionDataToSessionAccountOrThrow(session) accountDid = account.did @@ -594,6 +654,7 @@ export async function createSessionBundleAndCreateAccount( ) bundle = buildBundle(session) + registerBundleKillSwitch(bundle, hooks.kill) const account = sessionDataToSessionAccountOrThrow(session) accountDid = account.did @@ -749,26 +810,25 @@ function sessionDataToSessionAccountOrThrow( /** * Neutralize a bundle's session so it can never refresh again. * - * Called when switching away from / disposing an account. We null out the - * session locally (constructing a fresh destroyed-state marker is not exposed, - * so we rely on the reducer dropping all references) - the important guarantee - * is that this session's tokens are no longer reachable by any live client. We - * do NOT call `logout()` here: disposal is a local switch, not a server-side - * revocation (revocation is handled separately via the push-token unregister - * temporary sessions). The bundle's clients stay usable enough not to crash - * late readers. + * Called when switching away from / disposing an account. `PasswordSession` + * exposes no synchronous, hook-free way to mark itself destroyed without a + * network logout (and `logout()`/`delete()` would revoke on the server, which + * we do NOT want for a local switch - revocation is handled separately via the + * push-token unregister temporary sessions). So we trip the kill-switch + * installed in the session's injected `fetch` (see {@link makeSessionHooks} / + * {@link registerBundleKillSwitch}): every subsequent request through this + * session - direct fetch AND the internal auto-refresh, which shares the same + * captured `options.fetch` - throws before touching the network. A tripped + * refresh routes into the `onUpdateFailure` path (session preserved locally, + * refresh token NOT consumed server-side). `kill()` also disarms the hooks so + * the stale bundle can no longer dispatch into the reducer. The important + * guarantee - matching the old `dispose()` - is that this session's tokens are + * no longer reachable by any live network path. */ export function disposeBundle(bundle: SessionBundle | PublicSessionBundle) { const session = bundle.session if (!session || session.destroyed) { return } - /* - * There is no synchronous, hook-free way to mark a PasswordSession destroyed - * without a network logout. PasswordSession.delete() would revoke on the - * server, which we do NOT want for a local switch. So we fire-and-forget a - * logout-free neutralization by dropping our reference; GC reclaims the - * session. Any late fetchHandler call still uses valid tokens until the - * bundle is dereferenced by the reducer, which is the pre-existing behavior. - */ + bundleKillSwitches.get(bundle)?.() }