From b89167f07c73b1fd7b2c3ddaad839b4970d500d9 Mon Sep 17 00:00:00 2001 From: Samuel Newman Date: Thu, 13 Aug 2026 22:26:23 +0300 Subject: [PATCH] [SDK] Address stack review feedback (#11389) Co-authored-by: Claude Fable 5 --- src/lib/lexClient.ts | 7 ++++++- src/lib/strings/url-helpers.ts | 9 +++++++++ src/screens/Login/LoginForm.tsx | 4 +++- src/screens/Signup/state.ts | 5 +++-- src/state/queries/profile.ts | 14 +++++++------- src/state/session/index.tsx | 26 +++++++++++++++++++++++++- src/state/session/session-core.ts | 14 +++++++++++--- src/state/session/session-data.ts | 12 +++++------- 8 files changed, 69 insertions(+), 22 deletions(-) diff --git a/src/lib/lexClient.ts b/src/lib/lexClient.ts index 2a865d763c..568f9c142f 100644 --- a/src/lib/lexClient.ts +++ b/src/lib/lexClient.ts @@ -40,7 +40,12 @@ export function createLexClient( * Requests use PLAIN `fetch`, not `networkAwareFetch`: the host is untrusted * input, and a typo'd or dead service must not be reported as the app losing * network reachability. + * + * `appLabelers: null` suppresses the global `Client.appLabelers` static: these + * are `com.atproto.server` calls to a host the user typed, which have no use + * for moderation labels, and the header would disclose the app's configured + * moderation authorities to an arbitrary third-party server. */ export function createServiceClient(service: string): Client { - return createLexClient({service}) + return createLexClient({service}, {appLabelers: null}) } diff --git a/src/lib/strings/url-helpers.ts b/src/lib/strings/url-helpers.ts index 08ecf4b566..6a4e3335f8 100644 --- a/src/lib/strings/url-helpers.ts +++ b/src/lib/strings/url-helpers.ts @@ -27,6 +27,15 @@ const TRUSTED_REGEX = new RegExp( )})|/|#)`, ) +export function canParseUrl(url: string | URL, base?: string | URL): boolean { + try { + new URL(url, base) + return true + } catch { + return false + } +} + export function isValidDomain(str: string): boolean { return !!TLDs.find(tld => { let i = str.lastIndexOf(tld) diff --git a/src/screens/Login/LoginForm.tsx b/src/screens/Login/LoginForm.tsx index dfd6ccc227..4f34d3afaf 100644 --- a/src/screens/Login/LoginForm.tsx +++ b/src/screens/Login/LoginForm.tsx @@ -169,7 +169,9 @@ export const LoginForm = ({ ) } else { logger.warn('Failed to login', {error: errMsg}) - setError(cleanError(errMsg)) + /* the error object, not its stringification: cleanError only + * extracts the clean server message from a live LexError */ + setError(cleanError(err)) } } } diff --git a/src/screens/Signup/state.ts b/src/screens/Signup/state.ts index b012f34427..c2d8684d42 100644 --- a/src/screens/Signup/state.ts +++ b/src/screens/Signup/state.ts @@ -354,7 +354,6 @@ export function useSubmitSignup() { onboardingDispatch({type: 'start'}) } catch (err) { const e = err as Error - let errMsg = e.toString() if ( matchXrpcError(e, com.atproto.server.createAccount) === 'InvalidInviteCode' @@ -368,7 +367,9 @@ export function useSubmitSignup() { return } - const error = cleanError(errMsg) + /* the error object, not its stringification: cleanError only extracts + * the clean server message from a live LexError */ + const error = cleanError(e) const isHandleError = error.toLowerCase().includes('handle') dispatch({type: 'setIsLoading', value: false}) diff --git a/src/state/queries/profile.ts b/src/state/queries/profile.ts index 190f4a3998..fb3f467743 100644 --- a/src/state/queries/profile.ts +++ b/src/state/queries/profile.ts @@ -206,21 +206,21 @@ export function useProfileUpdateMutation() { appviewClient, profile.did, checkCommitted || - (profile => { + (fresh => { if (typeof newUserAvatar !== 'undefined') { - if (newUserAvatar === null && profile.avatar) { + if (newUserAvatar === null && fresh.avatar) { // url hasn't cleared yet return false - } else if (profile.avatar === profile.avatar) { + } else if (fresh.avatar === profile.avatar) { // url hasn't changed yet return false } } if (typeof newUserBanner !== 'undefined') { - if (newUserBanner === null && profile.banner) { + if (newUserBanner === null && fresh.banner) { // url hasn't cleared yet return false - } else if (profile.banner === profile.banner) { + } else if (fresh.banner === profile.banner) { // url hasn't changed yet return false } @@ -229,8 +229,8 @@ export function useProfileUpdateMutation() { return true } return ( - profile.displayName === updates.displayName && - profile.description === updates.description + fresh.displayName === updates.displayName && + fresh.description === updates.description ) }), ) diff --git a/src/state/session/index.tsx b/src/state/session/index.tsx index d3045932d3..7cce22fff7 100644 --- a/src/state/session/index.tsx +++ b/src/state/session/index.tsx @@ -157,7 +157,19 @@ export function Provider({children}: React.PropsWithChildren<{}>) { sessionEvent: AtpSessionEvent, sessionData?: SessionData, ) => { - if (sessionEvent === 'update' && sessionData) { + /* + * Only the live bundle may reset the expiry-rescue bookkeeping: a stale + * bundle's late update would otherwise clear the failed-generation set + * that bounds the rescue loop. (Its dispatch below is separately dropped + * by the reducer's identity guard.) + */ + if ( + sessionEvent === 'update' && + sessionData && + (store.getState().currentBundleState.bundle as unknown as + | SessionBundle + | PublicSessionBundle) === bundle + ) { failedExpiryTokensRef.current.get(accountDid)?.clear() } @@ -505,6 +517,18 @@ export function Provider({children}: React.PropsWithChildren<{}>) { if (after === before) { throw new Error('Failed to refresh session') } + /* + * The user may have logged out or switched accounts while the refresh was + * in flight. Reporting success then would run the caller's success path + * (dialogs closing, success toasts, SignupQueued advancing) against an + * account that is no longer active, so a stale bundle rejects instead. + */ + if ( + (store.getState().currentBundleState.bundle as unknown) !== + (bundle as unknown) + ) { + throw new Error('The session changed while it was being refreshed') + } /* * The session's `onUpdated` hook dispatches the new tokens into the store, * but that lands a render away; this snapshot exposes them immediately. diff --git a/src/state/session/session-core.ts b/src/state/session/session-core.ts index 6269cc7f29..dbed9bfa2f 100644 --- a/src/state/session/session-core.ts +++ b/src/state/session/session-core.ts @@ -6,6 +6,7 @@ import { } from '@atproto/lex-password-session' import {PUBLIC_BSKY_SERVICE} from '#/lib/constants' +import {canParseUrl} from '#/lib/strings/url-helpers' import {logger} from '#/logger' import {prefetchAgeAssuranceServerData} from '#/ageAssurance/data' import {features} from '#/analytics' @@ -95,9 +96,16 @@ export function buildBundle( session: PasswordSession, storedPdsUrl?: string, ): SessionBundle { - const agent = storedPdsUrl - ? routeSessionToPds(session, storedPdsUrl) - : session + /* + * The stored url is persisted data and may be malformed (legacy writes, + * corruption). `routeSessionToPds` feeds it to `new URL()` on every request, + * so an invalid value would throw from every client call; discard it here + * and let the session route against its own service instead. + */ + const agent = + storedPdsUrl && canParseUrl(storedPdsUrl) + ? routeSessionToPds(session, storedPdsUrl) + : session return { session, appviewClient: buildAppviewClient(agent), diff --git a/src/state/session/session-data.ts b/src/state/session/session-data.ts index 711f6ffe9b..bbc81f2875 100644 --- a/src/state/session/session-data.ts +++ b/src/state/session/session-data.ts @@ -1,5 +1,7 @@ -import {getPdsEndpoint, isValidDidDoc} from '@atproto/common-web' -import {type SessionData} from '@atproto/lex-password-session' +import { + extractPdsEndpoint, + type SessionData, +} from '@atproto/lex-password-session' import {jwtDecode} from 'jwt-decode' import {BSKY_SERVICE} from '#/lib/constants' @@ -40,11 +42,7 @@ export function sessionDataToSessionAccount( return undefined } const normalizedService = new URL(service).toString() - const didDocPdsUrl = - session.didDoc && isValidDidDoc(session.didDoc) - ? getPdsEndpoint(session.didDoc) - : undefined - const pdsUrl = didDocPdsUrl ?? storedPdsUrl + const pdsUrl = extractPdsEndpoint(session.didDoc) ?? storedPdsUrl return { service: normalizedService, did: session.did,