diff --git a/src/main/orca-profiles/profile-cloud-client.ts b/src/main/orca-profiles/profile-cloud-client.ts index 9ab8a57a41c..d8bf581b720 100644 --- a/src/main/orca-profiles/profile-cloud-client.ts +++ b/src/main/orca-profiles/profile-cloud-client.ts @@ -160,13 +160,8 @@ function normalizeSessionResponse(value: unknown): OrcaCloudSessionExchangeRespo const CLOUD_REQUEST_TIMEOUT_MS = 30_000 -async function postJson( - operation: string, - url: string, - body: unknown, - accessToken?: string -): Promise { - const response = await orcaCloudFetch(operation, url, { +async function postJson(url: string, body: unknown, accessToken?: string): Promise { + const response = await orcaCloudFetch(url, { method: 'POST', headers: { 'content-type': 'application/json', @@ -191,7 +186,7 @@ export async function exchangeOrcaCloudAuthCode( args: ExchangeCodeArgs ): Promise { 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 { return normalizeSessionResponse( - await postJson('session-refresh', config.refreshEndpoint, { + await postJson(config.refreshEndpoint, { refreshToken: session.refreshToken }) ) @@ -236,7 +231,6 @@ export async function createOrcaCloudProfile( ): Promise { 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 { - await postJson( - 'session-revoke', - config.logoutEndpoint, - { refreshToken: session.refreshToken }, - session.accessToken - ) + await postJson(config.logoutEndpoint, { refreshToken: session.refreshToken }, session.accessToken) } diff --git a/src/main/orca-profiles/profile-cloud-org-members-client.ts b/src/main/orca-profiles/profile-cloud-org-members-client.ts index 1b1e6c25069..341c586ae0d 100644 --- a/src/main/orca-profiles/profile-cloud-org-members-client.ts +++ b/src/main/orca-profiles/profile-cloud-org-members-client.ts @@ -138,12 +138,11 @@ function requestInit(method: 'GET' | 'POST', accessToken: string, body?: unknown } async function requestOrgMembers( - operation: string, url: string, init: RequestInit, parse: (value: unknown) => T ): Promise { - 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 { 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 { 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 { 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 { 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 { await requestOrgMembers( - 'org-member-remove', orgMembersUrl(config, args.orgId, '/members/remove'), requestInit('POST', session.accessToken, { userId: args.userId }), () => undefined diff --git a/src/main/orca-profiles/profile-cloud-transport.test.ts b/src/main/orca-profiles/profile-cloud-transport.test.ts index b8e1129d69a..dca0b9dcaf4 100644 --- a/src/main/orca-profiles/profile-cloud-transport.test.ts +++ b/src/main/orca-profiles/profile-cloud-transport.test.ts @@ -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 }) + ) }) }) diff --git a/src/main/orca-profiles/profile-cloud-transport.ts b/src/main/orca-profiles/profile-cloud-transport.ts index 67a7aa627b6..7cff8e36ed8 100644 --- a/src/main/orca-profiles/profile-cloud-transport.ts +++ b/src/main/orca-profiles/profile-cloud-transport.ts @@ -32,7 +32,6 @@ const FAILURE_REASON: Record = { 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 { +export async function orcaCloudFetch(url: string, init: RequestInit): Promise { 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' }