refactor(orca-cloud): tighten transport classification and drop the operation label

Review follow-ups on the cloud transport wrapper: - Bare CERT_* codes (CERT_HAS_EXPIRED, CERT_REVOKED, CERT_UNTRUSTED) are how Node reports certificate problems, and none of them carry an err_ prefix, so they fell through to 'unknown' — including the expiry case a skewed clock produces. Match on `cert` instead. DNS is still checked first so a hostname containing "cert" cannot be mistaken for a certificate failure. - net::ERR_ABORTED is how Chromium surfaces the 30s timeout signal, and ERR_ADDRESS_UNREACHABLE / ERR_SOCKET_NOT_CONNECTED had no home either. - Drop the `operation` parameter. It was threaded through five functions and eleven call sites, and OrcaCloudTransportError.operation was never read; the span now derives the label from the URL's origin and path, which is the same information. Both clients are back to a one-line change each. - Report the deepest chain level's own `code: message` rather than pairing an outer code with an inner message, and cap the detail length since it reaches a toast and the trace file. Verified the net.fetch contract against real Electron rather than assuming it: redirect:'error' refuses with "Attempted to redirect, but redirect policy was 'error'", a dead socket reports net::ERR_EMPTY_RESPONSE, AbortSignal.timeout raises TimeoutError, and — importantly — net.fetch attaches default-session cookies unless credentials is 'omit'. That option is load-bearing: without it this switch would have started sending app cookies to token endpoints, which undici never did cross-origin. Added a test so it cannot be dropped silently, and corrected two fixtures to the strings Electron actually throws.
2026-10-29 00:03:37 +00:00 · 2026-07-27 12:45:03 -07:00
parent ddf57276bc
commit 35e3feacdb
4 changed files with 132 additions and 79 deletions
@@ -160,13 +160,8 @@ function normalizeSessionResponse(value: unknown): OrcaCloudSessionExchangeRespo

 const CLOUD_REQUEST_TIMEOUT_MS = 30_000

-async function postJson<T>(
-  operation: string,
-  url: string,
-  body: unknown,
-  accessToken?: string
-): Promise<T> {
-  const response = await orcaCloudFetch(operation, url, {
+async function postJson<T>(url: string, body: unknown, accessToken?: string): Promise<T> {
+  const response = await orcaCloudFetch(url, {
    method: 'POST',
    headers: {
      'content-type': 'application/json',
@@ -191,7 +186,7 @@ export async function exchangeOrcaCloudAuthCode(
  args: ExchangeCodeArgs
 ): Promise<OrcaCloudSessionExchangeResponse> {
  return normalizeSessionResponse(
-    await postJson('session-exchange', config.sessionEndpoint, {
+    await postJson(config.sessionEndpoint, {
      code: args.code,
      codeVerifier: args.codeVerifier,
      nonce: args.nonce,
@@ -210,7 +205,7 @@ export async function refreshOrcaCloudCapabilities(
    cloud?: unknown
    organizations?: unknown
    capabilities: unknown
-  }>('capabilities-refresh', config.capabilitiesEndpoint, {}, session.accessToken)
+  }>(config.capabilitiesEndpoint, {}, session.accessToken)
  return {
    cloud: response.cloud === undefined ? undefined : normalizeCloudSummary(response.cloud),
    organizations: normalizeOrganizations(response.organizations),
@@ -223,7 +218,7 @@ export async function refreshOrcaCloudSession(
  session: OrcaCloudSession
 ): Promise<OrcaCloudSessionExchangeResponse> {
  return normalizeSessionResponse(
-    await postJson('session-refresh', config.refreshEndpoint, {
+    await postJson(config.refreshEndpoint, {
      refreshToken: session.refreshToken
    })
  )
@@ -236,7 +231,6 @@ export async function createOrcaCloudProfile(
 ): Promise<OrcaCloudSessionExchangeResponse> {
  return normalizeSessionResponse(
    await postJson(
-      'profile-create',
      config.profileEndpoint,
      {
        orgId: args.orgId,
@@ -256,7 +250,7 @@ export async function selectOrcaCloudOrg(
    cloud: unknown
    organizations?: unknown
    capabilities: unknown
-  }>('org-select', config.orgEndpoint, { orgId }, session.accessToken)
+  }>(config.orgEndpoint, { orgId }, session.accessToken)
  return {
    cloud: normalizeCloudSummary(response.cloud),
    organizations: normalizeOrganizations(response.organizations),
@@ -268,10 +262,5 @@ export async function revokeOrcaCloudSession(
  config: OrcaCloudAuthConfig,
  session: OrcaCloudSession
 ): Promise<void> {
-  await postJson(
-    'session-revoke',
-    config.logoutEndpoint,
-    { refreshToken: session.refreshToken },
-    session.accessToken
-  )
+  await postJson(config.logoutEndpoint, { refreshToken: session.refreshToken }, session.accessToken)
 }
@@ -138,12 +138,11 @@ function requestInit(method: 'GET' | 'POST', accessToken: string, body?: unknown
 }

 async function requestOrgMembers<T>(
-  operation: string,
  url: string,
  init: RequestInit,
  parse: (value: unknown) => T
 ): Promise<T> {
-  const response = await orcaCloudFetch(operation, url, init)
+  const response = await orcaCloudFetch(url, init)
  if (!response.ok) {
    throw new OrcaCloudRequestError(response.status, await extractErrorCode(response))
  }
@@ -156,7 +155,6 @@ export async function listOrcaCloudOrgMembers(
  orgId: string
 ): Promise<OrcaOrgMembersRoster> {
  return requestOrgMembers(
-    'org-members-list',
    orgMembersUrl(config, orgId, '/members'),
    requestInit('GET', session.accessToken),
    normalizeRoster
@@ -169,7 +167,6 @@ export async function inviteOrcaCloudOrgMember(
  args: { orgId: string; email: string; role: OrcaOrgRole }
 ): Promise<void> {
  await requestOrgMembers(
-    'org-member-invite',
    orgMembersUrl(config, args.orgId, '/invites'),
    requestInit('POST', session.accessToken, { email: args.email, role: args.role }),
    () => undefined
@@ -182,7 +179,6 @@ export async function revokeOrcaCloudOrgInvite(
  args: { orgId: string; email: string }
 ): Promise<void> {
  await requestOrgMembers(
-    'org-invite-revoke',
    orgMembersUrl(config, args.orgId, '/invites/revoke'),
    requestInit('POST', session.accessToken, { email: args.email }),
    () => undefined
@@ -195,7 +191,6 @@ export async function changeOrcaCloudOrgMemberRole(
  args: { orgId: string; userId: string; role: OrcaOrgRole }
 ): Promise<void> {
  await requestOrgMembers(
-    'org-member-role',
    orgMembersUrl(config, args.orgId, '/members/role'),
    requestInit('POST', session.accessToken, { userId: args.userId, role: args.role }),
    () => undefined
@@ -208,7 +203,6 @@ export async function removeOrcaCloudOrgMember(
  args: { orgId: string; userId: string }
 ): Promise<void> {
  await requestOrgMembers(
-    'org-member-remove',
    orgMembersUrl(config, args.orgId, '/members/remove'),
    requestInit('POST', session.accessToken, { userId: args.userId }),
    () => undefined
@@ -49,6 +49,12 @@ describe('Orca cloud transport failure reporting', () => {
    fetchMock.mockReset()
  })

+  // Why afterEach and not inline: a failed assertion would otherwise leave the
+  // global fetch stub installed for the live-socket suite below.
+  afterEach(() => {
+    vi.unstubAllGlobals()
+  })
+
  const cases: { label: string; error: unknown; failure: OrcaCloudTransportFailure }[] = [
    {
      label: 'undici DNS lookup failure',
@@ -66,10 +72,17 @@ describe('Orca cloud transport failure reporting', () => {
      failure: 'redirect'
    },
    {
+      // Verbatim from Electron's net.fetch with redirect: 'error'.
      label: 'Chromium redirect refusal',
-      error: new TypeError("Attempted to redirect, but redirect policy was 'error'"),
+      error: new Error("Attempted to redirect, but redirect policy was 'error'"),
      failure: 'redirect'
    },
+    {
+      // What Electron actually reports when the peer kills the socket.
+      label: 'Chromium empty response after socket death',
+      error: new Error('net::ERR_EMPTY_RESPONSE'),
+      failure: 'connection-lost'
+    },
    {
      label: 'Chromium name resolution failure',
      error: new Error('net::ERR_NAME_NOT_RESOLVED'),
@@ -88,6 +101,18 @@ describe('Orca cloud transport failure reporting', () => {
      ),
      failure: 'tls'
    },
+    {
+      // Node reports these without an err_ prefix; a skewed clock makes the
+      // expiry case common, and it used to fall through to 'unknown'.
+      label: 'expired certificate from a skewed clock',
+      error: undiciError('certificate has expired', 'CERT_HAS_EXPIRED'),
+      failure: 'tls'
+    },
+    {
+      label: 'revoked certificate',
+      error: undiciError('certificate revoked', 'CERT_REVOKED'),
+      failure: 'tls'
+    },
    {
      label: 'proxy tunnel refusal',
      error: new Error('net::ERR_TUNNEL_CONNECTION_FAILED'),
@@ -108,6 +133,17 @@ describe('Orca cloud transport failure reporting', () => {
      error: new DOMException('The operation was aborted due to timeout', 'TimeoutError'),
      failure: 'timeout'
    },
+    {
+      // The only abort these requests carry is the 30s timeout signal.
+      label: 'Chromium abort from the timeout signal',
+      error: new Error('net::ERR_ABORTED'),
+      failure: 'timeout'
+    },
+    {
+      label: 'unreachable address',
+      error: new Error('net::ERR_ADDRESS_UNREACHABLE'),
+      failure: 'offline'
+    },
    {
      label: 'unrecognized transport failure',
      error: new Error('net::ERR_FAILED'),
@@ -119,11 +155,9 @@ describe('Orca cloud transport failure reporting', () => {
    it(`classifies ${label} as ${failure}`, async () => {
      fetchMock.mockRejectedValue(error)

-      const thrown = await orcaCloudFetch(
-        'session-exchange',
-        'https://login.onorca.dev/v1/desktop/auth/session',
-        { method: 'POST' }
-      ).catch((caught: unknown) => caught)
+      const thrown = await orcaCloudFetch('https://login.onorca.dev/v1/desktop/auth/session', {
+        method: 'POST'
+      }).catch((caught: unknown) => caught)

      expect(thrown).toBeInstanceOf(OrcaCloudTransportError)
      expect((thrown as OrcaCloudTransportError).failure).toBe(failure)
@@ -137,13 +171,40 @@ describe('Orca cloud transport failure reporting', () => {
    vi.stubGlobal('fetch', globalFetch)
    fetchMock.mockResolvedValue({ ok: true, status: 200, json: async () => ({}) })

-    await orcaCloudFetch('capabilities-refresh', 'https://login.onorca.dev/v1/desktop/auth/x', {
-      method: 'POST'
-    })
+    await orcaCloudFetch('https://login.onorca.dev/v1/desktop/auth/x', { method: 'POST' })

    expect(fetchMock).toHaveBeenCalledTimes(1)
    expect(globalFetch).not.toHaveBeenCalled()
-    vi.unstubAllGlobals()
+  })
+
+  // Verified against real Electron: net.fetch attaches default-session cookies
+  // unless credentials is 'omit'. Dropping this would silently start sending
+  // app cookies to token endpoints, which the previous undici path never did.
+  it('keeps ambient session cookies and cached responses off token endpoints', async () => {
+    fetchMock.mockResolvedValue({ ok: true, status: 200, json: async () => ({}) })
+
+    await orcaCloudFetch('https://login.onorca.dev/v1/desktop/auth/session', { method: 'POST' })
+
+    expect(fetchMock).toHaveBeenCalledWith(
+      'https://login.onorca.dev/v1/desktop/auth/session',
+      expect.objectContaining({ credentials: 'omit', cache: 'no-store' })
+    )
+  })
+
+  it('lets callers keep redirect refusal and the request timeout', async () => {
+    fetchMock.mockResolvedValue({ ok: true, status: 200, json: async () => ({}) })
+    const signal = AbortSignal.timeout(30_000)
+
+    await orcaCloudFetch('https://login.onorca.dev/v1/desktop/auth/session', {
+      method: 'POST',
+      redirect: 'error',
+      signal
+    })
+
+    expect(fetchMock).toHaveBeenCalledWith(
+      expect.any(String),
+      expect.objectContaining({ redirect: 'error', signal })
+    )
  })
 })

@@ -32,7 +32,6 @@ const FAILURE_REASON: Record<OrcaCloudTransportFailure, string> = {
 export class OrcaCloudTransportError extends Error {
  constructor(
    public readonly failure: OrcaCloudTransportFailure,
-    public readonly operation: string,
    public readonly detail: string,
    options?: { cause?: unknown }
  ) {
@@ -41,7 +40,9 @@ export class OrcaCloudTransportError extends Error {
  }
 }

-type ErrorFacts = { names: string[]; codes: string[]; messages: string[] }
+// `details` holds one `code: message` string per chain level, so the reported
+// detail never pairs an outer error's code with an inner error's message.
+type ErrorFacts = { names: string[]; details: string[]; haystack: string }

 type ErrorLike = { name?: unknown; message?: unknown; code?: unknown; cause?: unknown }

@@ -55,51 +56,66 @@ function isErrorLike(value: unknown): value is ErrorLike {
 // `net::ERR_*` in the message. Both chains are walked so classification does
 // not depend on which transport produced the failure.
 function collectErrorFacts(error: unknown): ErrorFacts {
-  const facts: ErrorFacts = { names: [], codes: [], messages: [] }
+  const names: string[] = []
+  const details: string[] = []
  let current: unknown = error
  for (let depth = 0; depth < 5 && isErrorLike(current); depth += 1) {
-    if (typeof current.name === 'string') {
-      facts.names.push(current.name)
+    const name = typeof current.name === 'string' ? current.name : ''
+    const message = typeof current.message === 'string' ? current.message : ''
+    const code = typeof current.code === 'string' ? current.code : ''
+    if (name) {
+      names.push(name)
    }
-    if (typeof current.message === 'string') {
-      facts.messages.push(current.message)
-    }
-    if (typeof current.code === 'string') {
-      facts.codes.push(current.code)
+    const detail = [code, message].filter(Boolean).join(': ')
+    if (detail) {
+      details.push(detail)
    }
    current = current.cause
  }
-  return facts
+  return { names, details, haystack: details.join(' | ').toLowerCase() }
 }

 function classifyTransportFailure(facts: ErrorFacts): OrcaCloudTransportFailure {
-  const haystack = [...facts.codes, ...facts.messages].join(' | ').toLowerCase()
-  const matches = (pattern: RegExp): boolean => pattern.test(haystack)
+  const matches = (pattern: RegExp): boolean => pattern.test(facts.haystack)

+  // Why err_aborted counts as a timeout: the only abort signal these requests
+  // carry is the 30s AbortSignal.timeout, which Chromium surfaces that way.
  if (facts.names.includes('TimeoutError') || facts.names.includes('AbortError')) {
    return 'timeout'
  }
-  if (matches(/etimedout|und_err_(connect_|headers_|body_)?timeout|err_(connection_)?timed_out/)) {
+  if (
+    matches(
+      /etimedout|und_err_(connect_|headers_|body_)?timeout|err_(connection_)?timed_out|err_aborted/
+    )
+  ) {
    return 'timeout'
  }
  if (matches(/unexpected redirect|redirect (mode|policy|count)|err_(unsafe_|too_many_)redirect/)) {
    return 'redirect'
  }
+  // DNS is checked before TLS so a hostname containing "cert" cannot be
+  // mistaken for a certificate failure.
  if (matches(/enotfound|eai_again|err_name_not_resolved|err_name_resolution_failed/)) {
    return 'dns'
  }
-  if (matches(/err_cert|err_ssl|err_tls|_cert_|certificate|self.signed|unable_to_verify|eproto/)) {
+  // Bare `cert`: Node reports CERT_HAS_EXPIRED/CERT_REVOKED/CERT_UNTRUSTED
+  // without any err_ prefix, and a skewed clock makes the expiry case common.
+  if (matches(/cert|err_ssl|err_tls|self.signed|unable_to_verify|eproto/)) {
    return 'tls'
  }
  if (matches(/err_proxy|err_tunnel_connection_failed|proxy_auth/)) {
    return 'proxy'
  }
-  if (matches(/err_internet_disconnected|err_network_changed|enetunreach|enetdown|ehostunreach/)) {
+  if (
+    matches(
+      /err_internet_disconnected|err_network_changed|err_address_unreachable|enetunreach|enetdown|ehostunreach/
+    )
+  ) {
    return 'offline'
  }
  if (
    matches(
-      /econnreset|econnrefused|econnaborted|epipe|und_err_socket|err_connection_|err_empty_response/
+      /econnreset|econnrefused|econnaborted|epipe|und_err_socket|err_connection_|err_empty_response|err_socket_not_connected/
    )
  ) {
    return 'connection-lost'
@@ -107,26 +123,18 @@ function classifyTransportFailure(facts: ErrorFacts): OrcaCloudTransportFailure
  return 'unknown'
 }

-// Transport-level text only (errno, net:: code, host) — never request bodies,
-// so authorization codes, verifiers, nonces, and tokens cannot leak here.
-function describeErrorChain(facts: ErrorFacts): string {
-  const deepest = facts.messages.at(-1) ?? 'unknown error'
-  const code = facts.codes.at(-1)
-  return code ? `${code}: ${deepest}` : deepest
-}
-
-function recordTransportFailure(
-  span: ActiveSpan,
-  operation: string,
-  error: unknown
-): OrcaCloudTransportError {
+function recordTransportFailure(span: ActiveSpan, error: unknown): OrcaCloudTransportError {
  const facts = collectErrorFacts(error)
  const failure = classifyTransportFailure(facts)
-  const detail = describeErrorChain(facts)
+  // Transport-level text only (errno, net:: code, host) — never request bodies,
+  // so authorization codes, verifiers, nonces, and tokens cannot leak here.
+  // Capped because this reaches a toast and the trace file, and nothing
+  // upstream promises a short message.
+  const detail = (facts.details.at(-1) || String(error)).slice(0, 200)
  span.setAttribute('orcaCloud.transportFailure', failure)
  span.setAttribute('orcaCloud.transportErrorName', facts.names.join(' <- ') || typeof error)
  span.setAttribute('orcaCloud.transportErrorDetail', detail)
-  return new OrcaCloudTransportError(failure, operation, detail, { cause: error })
+  return new OrcaCloudTransportError(failure, detail, { cause: error })
 }

 /**
@@ -135,16 +143,15 @@ function recordTransportFailure(
 * call survives Windows TLS interception and undici's stale keep-alive sockets
 * that global fetch could only report as `fetch failed` (orca#10758).
 */
-export async function orcaCloudFetch(
-  operation: string,
-  url: string,
-  init: RequestInit
-): Promise<Response> {
+export async function orcaCloudFetch(url: string, init: RequestInit): Promise<Response> {
  return withSpan(
    'orcaCloud.request',
    async (span) => {
-      span.setAttribute('orcaCloud.operation', operation)
-      span.setAttribute('orcaCloud.origin', new URL(url).origin)
+      const endpoint = new URL(url)
+      // Origin + path identify which call failed. The query string is dropped
+      // because it is the one part of a URL that could ever carry a secret;
+      // these endpoints put codes and tokens in the body or Bearer header.
+      span.setAttribute('orcaCloud.endpoint', `${endpoint.origin}${endpoint.pathname}`)
      await ensureElectronProxyFromEnvironment({
        proxySession: session.defaultSession,
        probeUrl: url
@@ -154,10 +161,12 @@ export async function orcaCloudFetch(
        })
      })
      try {
-        // Why the explicit defaults: Chromium would otherwise attach default-
-        // session cookies and consult the HTTP cache, neither of which the
-        // previous undici path did. Token endpoints authenticate from the body
-        // or the Bearer header, so ambient session state stays out of it.
+        // credentials:'omit' is load-bearing, not cosmetic — verified against
+        // Electron: net.fetch attaches default-session cookies unless told not
+        // to, which undici never did cross-origin. Token endpoints authenticate
+        // from the body or the Bearer header, so no ambient session state
+        // belongs on them. cache:'no-store' likewise keeps Chromium from
+        // serving a stale roster on the one GET route.
        const response = await net.fetch(url, {
          credentials: 'omit',
          cache: 'no-store',
@@ -166,7 +175,7 @@ export async function orcaCloudFetch(
        span.setAttribute('orcaCloud.status', response.status)
        return response
      } catch (error) {
-        throw recordTransportFailure(span, operation, error)
+        throw recordTransportFailure(span, error)
      }
    },
    { kind: 'client' }