reject no-op refreshes, mirror the inner session's pds routing

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

src/state/session/__tests__/bridge-agent-test.ts

@@ -19,113 +19,20 @@ jest.mock('jwt-decode', () => ({
 import {BskyAppAgent, PasswordSessionManager} from '../bridge-agent'
 import {sessionAccountToSessionData} from '../session-data'
 import {type SessionAccount} from '../types'
-
-const DID = 'did:plc:example123'
-const HANDLE = 'alice.test'
-const SERVICE = 'https://bsky.social'
-const PDS_HOST = 'https://shimeji.us-east.host.bsky.network'
-const DIDDOC_PDS_HOST = 'https://morel.us-west.host.bsky.network'
-
-function makeAccount(overrides: Partial<SessionAccount> = {}): SessionAccount {
-  return {
-    service: SERVICE,
-    did: DID,
-    handle: HANDLE,
-    email: 'alice@example.com',
-    emailConfirmed: true,
-    emailAuthFactor: false,
-    refreshJwt: 'refresh-jwt',
-    accessJwt: 'access-jwt',
-    signupQueued: false,
-    active: true,
-    status: undefined,
-    pdsUrl: undefined,
-    isSelfHosted: false,
-    ...overrides,
-  }
-}
-
-/** A minimal valid DID document whose only service entry is a PDS. */
-function makeDidDoc(pdsUrl: string) {
-  return {
-    id: DID,
-    service: [
-      {
-        id: '#atproto_pds',
-        type: 'AtprotoPersonalDataServer',
-        serviceEndpoint: pdsUrl,
-      },
-    ],
-  }
-}
-
-function json(body: unknown, status = 200) {
-  return new Response(JSON.stringify(body), {
-    status,
-    headers: {'content-type': 'application/json'},
-  })
-}
-
-/**
- * Build a mock `fetch` that returns canned XRPC responses keyed by the last
- * path segment (nsid). `refreshSession` returns fresh tokens; `getSession`
- * echoes the account; anything else returns an empty 200.
- */
-function makeMockFetch(
-  overrides: Record<
-    string,
-    (url: string, init: RequestInit) => Response | Promise<Response>
-  > = {},
-) {
-  return jest.fn(
-    /*
-     * PasswordSession calls fetch with a URL object (new URL(path, service));
-     * asFetch() below widens the mock to the full fetch signature it expects.
-     */
-    async (input: URL | string, init: RequestInit = {}): Promise<Response> => {
-      const url = input instanceof URL ? input.href : input
-      const nsid = url.split('/xrpc/')[1]?.split('?')[0]
-      const handler = nsid ? overrides[nsid] : undefined
-      if (handler) {
-        return handler(url, init)
-      }
-      if (nsid === 'com.atproto.server.refreshSession') {
-        return json({
-          accessJwt: 'access-jwt-2',
-          refreshJwt: 'refresh-jwt-2',
-          handle: HANDLE,
-          did: DID,
-          email: 'alice@example.com',
-          /* both emailConfirmed and didDoc present -> no getSession follow-up */
-          emailConfirmed: true,
-          didDoc: makeDidDoc(DIDDOC_PDS_HOST),
-          active: true,
-        })
-      }
-      if (nsid === 'com.atproto.server.getSession') {
-        return json({
-          did: DID,
-          handle: HANDLE,
-          email: 'alice@example.com',
-          emailConfirmed: true,
-          active: true,
-        })
-      }
-      return json({})
-    },
-  )
-}
-
-type MockFetch = ReturnType<typeof makeMockFetch>
-
-/** Cast a jest fetch mock to the `fetch` type PasswordSession options expect. */
-function asFetch(mock: MockFetch): typeof fetch {
-  return mock as unknown as typeof fetch
-}
-
-function urlsOf(mock: MockFetch): string[] {
-  return mock.mock.calls.map(c => (c[0] instanceof URL ? c[0].href : c[0]))
-}
+import {
+  asFetch,
+  DID,
+  DIDDOC_PDS_HOST,
+  HANDLE,
+  json,
+  makeAccount,
+  makeDidDoc,
+  makeMockFetch,
+  type MockFetch,
+  PDS_HOST,
+  SERVICE,
+  urlsOf,
+} from './mock-fetch'
 
 /**
  * Build the manager + agent pair under test.
@@ -223,6 +130,63 @@ describe('PasswordSessionManager getters', () => {
     expect(agent.pdsUrl).toBe(undefined)
     expect(agent.dispatchUrl.toString()).toBe('https://bsky.social/')
   })
+
+  it('matches the inner session on didDocs a strict validator would reject', () => {
+    /*
+     * No `id` on the document and a non-canonical service `type`: enough for
+     * isValidDidDoc/getPdsEndpoint to bail, but PasswordSession still routes
+     * here, so the bridge must agree or dispatchUrl lies about where requests
+     * go (and service-auth aud gets minted for the wrong host).
+     */
+    const {agent} = setup({
+      didDoc: {
+        service: [
+          {
+            id: '#atproto_pds',
+            type: 'SomethingElse',
+            serviceEndpoint: DIDDOC_PDS_HOST,
+          },
+        ],
+      },
+      pdsUrl: PDS_HOST,
+    })
+    expect(agent.pdsUrl?.toString()).toBe(`${DIDDOC_PDS_HOST}/`)
+    expect(agent.dispatchUrl.toString()).toBe(`${DIDDOC_PDS_HOST}/`)
+  })
+
+  it('falls back to the stored pdsUrl when the didDoc has no PDS service', () => {
+    const {agent} = setup({
+      didDoc: {
+        id: DID,
+        service: [
+          {
+            id: '#bsky_notif',
+            type: 'BskyNotificationService',
+            serviceEndpoint: DIDDOC_PDS_HOST,
+          },
+        ],
+      },
+      pdsUrl: PDS_HOST,
+    })
+    expect(agent.pdsUrl?.toString()).toBe(`${PDS_HOST}/`)
+  })
+
+  it('falls back to the stored pdsUrl when the PDS endpoint does not parse', () => {
+    const {agent} = setup({
+      didDoc: {
+        id: DID,
+        service: [
+          {
+            id: '#atproto_pds',
+            type: 'AtprotoPersonalDataServer',
+            serviceEndpoint: 'not a url',
+          },
+        ],
+      },
+      pdsUrl: PDS_HOST,
+    })
+    expect(agent.pdsUrl?.toString()).toBe(`${PDS_HOST}/`)
+  })
 })
 
 describe('PasswordSessionManager.session identity', () => {
@@ -570,9 +534,40 @@ describe('PasswordSession lifecycle over mocked fetch', () => {
       fetchMock,
       sessionOptions: {onDeleted, onUpdateFailure},
     })
-    await manager.refreshSession()
+    /*
+     * PasswordSession.refresh() resolves with the unchanged data here; the
+     * bridge restores the old CredentialSession contract by rejecting.
+     */
+    await expect(manager.refreshSession()).rejects.toThrow(
+      'Failed to refresh session',
+    )
     expect(onUpdateFailure).toHaveBeenCalledTimes(1)
     expect(onDeleted).not.toHaveBeenCalled()
     expect(manager.session?.accessJwt).toBe('access-jwt')
   })
+
+  it('rejects on a network error rather than reporting a no-op success', async () => {
+    const fetchMock = makeMockFetch({
+      'com.atproto.server.refreshSession': () => {
+        throw new TypeError('Network request failed')
+      },
+    })
+    const {manager} = setup({fetchMock})
+    await expect(manager.refreshSession()).rejects.toThrow(
+      'Failed to refresh session',
+    )
+    /* the session survives, exactly as the old transient-failure path did */
+    expect(manager.session?.accessJwt).toBe('access-jwt')
+  })
+
+  it('resumeSession rejects on a transient failure too', async () => {
+    const fetchMock = makeMockFetch({
+      'com.atproto.server.refreshSession': () =>
+        json({error: 'InternalServerError'}, 500),
+    })
+    const {agent} = setup({fetchMock})
+    await expect(agent.resumeSession(agent.session!)).rejects.toThrow(
+      'Failed to refresh session',
+    )
+  })
 })

src/state/session/bridge-agent.ts

@@ -7,7 +7,6 @@ import {
   type ComAtprotoServerRefreshSession,
   CredentialSession,
 } from '@atproto/api'
-import {getPdsEndpoint, isValidDidDoc} from '@atproto/common-web'
 import {
   type PasswordSession,
   type SessionData,
@@ -53,6 +52,47 @@ function parseUrl(input: string): URL | undefined {
   }
 }
 
+/**
+ * Read a property off an unknown value the way JS optional chaining would,
+ * without narrowing assumptions about the shape of a `LexMap`.
+ */
+function prop(value: unknown, key: string): unknown {
+  return typeof value === 'object' && value !== null
+    ? (value as Record<string, unknown>)[key]
+    : undefined
+}
+
+/**
+ * The PDS endpoint declared by a DID document, or `undefined`.
+ *
+ * This deliberately mirrors `extractPdsUrl` in `@atproto/lex-password-session`,
+ * which is the predicate `PasswordSession` uses to route its own requests: the
+ * first service entry whose `id` ends with `#atproto_pds`, taking its
+ * `serviceEndpoint` if it parses as a URL. It is looser than the
+ * `isValidDidDoc` + `getPdsEndpoint` pair from `@atproto/common-web` (no doc
+ * schema validation, no `type` check), and that is the point - a stricter
+ * predicate here would let `dispatchUrl` disagree with the host requests
+ * actually go to, which in turn mints service-auth tokens (video upload) for
+ * the wrong audience.
+ */
+function extractPdsUrl(didDoc: SessionData['didDoc']): URL | undefined {
+  const services = prop(didDoc, 'service')
+  if (!Array.isArray(services)) {
+    return undefined
+  }
+  /*
+   * `find`, not a scan: the inner session stops at the first `#atproto_pds`
+   * entry and gives up if its endpoint does not parse, rather than falling
+   * through to a later entry.
+   */
+  const pds = services.find(service => {
+    const id = prop(service, 'id')
+    return typeof id === 'string' && id.endsWith('#atproto_pds')
+  })
+  const endpoint = prop(pds, 'serviceEndpoint')
+  return typeof endpoint === 'string' ? parseUrl(endpoint) : undefined
+}
+
 /**
  * A `CredentialSession` whose auth core is a `PasswordSession`.
  *
@@ -155,9 +195,11 @@ export class PasswordSessionManager extends CredentialSession {
    *
    * Identity-cached on the source `SessionData`: consecutive reads with no
    * intervening token rotation return the same object, and a rotation produces
-   * a new one. Consumers depend on this - `useAccountEmailState` has a
-   * `useMemo` keyed on `agent.session`, which would recompute on every render
-   * if we allocated a fresh object per read.
+   * a new one. `CredentialSession` declares `session` as a plain field, so
+   * consumers are entitled to treat it as a value whose identity changes only
+   * when the session does; this class is read from render paths, and returning
+   * a freshly allocated object on every read would break that expectation for
+   * any memo, dependency array or reference comparison built on top of it.
    */
   #readSession(): AtpSessionData | undefined {
     const live = this.#liveData()
@@ -176,11 +218,13 @@ export class PasswordSessionManager extends CredentialSession {
   /**
    * The `pdsUrl` accessor's implementation.
    *
-   * The DID document's PDS endpoint wins when there is one. Before the first
-   * refresh delivers a didDoc (the non-expired resume fast path, which makes no
-   * network call) we fall back to the `pdsUrl` persisted on the account, so the
-   * very first requests still reach the right host - entryway accounts have
-   * `service: bsky.social` but live on a different PDS.
+   * The DID document's PDS endpoint wins when there is one, derived with
+   * {@link extractPdsUrl} so this agrees exactly with the inner session's own
+   * routing. Before the first refresh delivers a didDoc (the non-expired resume
+   * fast path, which makes no network call) we fall back to the `pdsUrl`
+   * persisted on the account, so the very first requests still reach the right
+   * host - entryway accounts have `service: bsky.social` but live on a
+   * different PDS.
    *
    * Identity-cached on the didDoc for the same reason as `session`.
    */
@@ -193,10 +237,7 @@ export class PasswordSessionManager extends CredentialSession {
     }
     if (live !== this.#pdsSource) {
       this.#pdsSource = live
-      const endpoint = isValidDidDoc(live.didDoc)
-        ? getPdsEndpoint(live.didDoc)
-        : undefined
-      this.#pdsValue = endpoint ? parseUrl(endpoint) : this.#storedPdsUrl
+      this.#pdsValue = extractPdsUrl(live.didDoc) ?? this.#storedPdsUrl
     }
     return this.#pdsValue
   }
@@ -225,6 +266,12 @@ export class PasswordSessionManager extends CredentialSession {
      * session entirely. This is mandatory: `PasswordSession.fetchHandler`
      * throws `TypeError` on a pre-set authorization header rather than
      * deferring to it.
+     *
+     * Bypassing also means these requests get no refresh-on-401 retry, since
+     * that lives in `PasswordSession.fetchHandler`. That is intentional: the
+     * caller supplied its own credential (a service-auth token, say), so
+     * rotating the session's tokens would not make the request any more likely
+     * to succeed on a retry.
      */
     if (
       !inner ||
@@ -241,12 +288,38 @@ export class PasswordSessionManager extends CredentialSession {
     return inner.fetchHandler(target.href, init ?? {})
   }
 
+  /**
+   * Refresh the session, rejecting if nothing was refreshed.
+   *
+   * This restores the contract of the `CredentialSession.refreshSession` this
+   * class replaces, which rejected on any refresh failure.
+   * `PasswordSession.refresh()` does not: on a transient failure (a 500, a
+   * network error) it reports through `onUpdateFailure` and then *resolves*
+   * with the unchanged session data, reserving rejection for the cases where
+   * the session is definitively gone. Callers here read resolution as "tokens
+   * rotated" - `SignupQueued` refreshes and then re-checks the token scope, and
+   * the various verification dialogs refresh and then close - so a resolved
+   * no-op would silently loop or report success.
+   *
+   * The signal is the identity of the returned `SessionData`, not a field
+   * comparison: `PasswordSession` builds a brand new object on every successful
+   * rotation and returns the existing one untouched on a transient failure, so
+   * identity separates the two exactly. Comparing against the data captured
+   * immediately before the call also gets concurrent refreshes right - if
+   * another caller's refresh rotated the tokens while ours was queued behind it
+   * (`PasswordSession` serializes refreshes), the data we get back still
+   * differs from what we captured, which is a success for our caller.
+   */
   override async refreshSession(): Promise<ComAtprotoServerRefreshSession.Response> {
     const inner = this.#disposed ? null : this.#inner
     if (!inner || inner.destroyed) {
       throw new Error('No session to refresh')
     }
+    const before = this.#liveData()
     const data = await inner.refresh()
+    if (data === before) {
+      throw new Error('Failed to refresh session')
+    }
     /*
      * Re-shape the lex payload into the `@atproto/api` XRPC response envelope.
      * `headers` is empty because the inner session does not surface response
@@ -278,6 +351,9 @@ export class PasswordSessionManager extends CredentialSession {
    * now". The returned envelope is the refresh one, which is structurally a
    * superset of `ComAtprotoServerGetSession.Response` (the shape `AtpAgent`
    * advertises), so both layers stay type-correct.
+   *
+   * It inherits {@link PasswordSessionManager.refreshSession}'s contract, so it
+   * rejects rather than resolving when no tokens were rotated.
    */
   override resumeSession(
     _session: AtpSessionData,