diff --git a/src/lib/network-error.test.ts b/src/lib/network-error.test.ts index 7e91b14a08..5acbe9d705 100644 --- a/src/lib/network-error.test.ts +++ b/src/lib/network-error.test.ts @@ -1,20 +1,46 @@ +import {getMain, XrpcInternalError} from '@atproto/lex' import {describe, expect, test} from '@jest/globals' +import {AbortError} from '#/lib/async/cancelable' import {isNetworkError} from '#/lib/network-error' +import {com} from '#/lexicons' + +const method = getMain(com.atproto.server.describeServer) describe('isNetworkError', () => { test.each([ 'Network request failed', 'TypeError: Failed to fetch', 'Error: fetch failed: java.net.UnknownHostException', + 'Load failed', + 'Error: Upstream service unreachable', + 'NetworkError when attempting to fetch resource', 'The Internet connection appears to be offline', 'The network connection was lost', + 'Unable to resolve host "bsky.social": No address associated with hostname', 'A server with the specified hostname could not be found', 'TypeError: Network request timed out', + 'ConnectException: Failed to connect to bsky.social/1.2.3.4:443', + 'SSLHandshakeException: Connection closed by peer', ])('detects %s', message => { expect(isNetworkError(new Error(message))).toBe(true) }) + test('detects a cancelled request', () => { + expect(isNetworkError(new AbortError())).toBe(true) + }) + + test('detects a stringified web abort', () => { + expect(isNetworkError('AbortError: The user aborted a request.')).toBe(true) + }) + + test.each([ + 'MultipartUploadError: Multipart upload aborted', + 'TypeError: undefined is not an object (evaluating abortController.abort)', + ])('does not treat a lowercase abort as a network error: %s', message => { + expect(isNetworkError(message)).toBe(false) + }) + test('checks wrapped error causes', () => { const error = new Error('Unable to fulfill XRPC request', { cause: new Error('fetch failed: connection closed'), @@ -23,6 +49,23 @@ describe('isNetworkError', () => { expect(isNetworkError(error)).toBe(true) }) + test('checks the cause of a wrapper that does not embed it', () => { + const error = new XrpcInternalError(method, undefined, { + cause: new Error('fetch failed'), + }) + + expect(error.message).not.toContain('fetch failed') + expect(isNetworkError(error)).toBe(true) + }) + + test('keeps a wrapper whose cause is not a network error', () => { + const error = new XrpcInternalError(method, undefined, { + cause: new TypeError('URL.canParse is not a function'), + }) + + expect(isNetworkError(error)).toBe(false) + }) + test('does not treat arbitrary fetch-handler failures as network errors', () => { expect( isNetworkError( @@ -36,4 +79,28 @@ describe('isNetworkError', () => { test('does not match words ending in load', () => { expect(isNetworkError(new Error('Multipart upload failed'))).toBe(false) }) + + test('terminates on a cycle of causes', () => { + const a = new Error('first') as Error & {cause?: unknown} + const b = new Error('second') as Error & {cause?: unknown} + a.cause = b + b.cause = a + + expect(isNetworkError(a)).toBe(false) + }) + + test.each([ + ['a null-prototype object', () => Object.create(null)], + [ + 'a value whose toString throws', + () => ({ + toString() { + throw new Error('nope') + }, + }), + ], + ])('returns false without throwing for %s', (_label, build) => { + expect(() => isNetworkError(build())).not.toThrow() + expect(isNetworkError(build())).toBe(false) + }) }) diff --git a/src/lib/network-error.ts b/src/lib/network-error.ts index a5647338f4..986de9e3bd 100644 --- a/src/lib/network-error.ts +++ b/src/lib/network-error.ts @@ -1,5 +1,12 @@ const NETWORK_ERROR_PATTERNS = [ - /\babort(?:ed|error)?\b/i, + /* + * Case-sensitive on purpose. `AbortError` (web `DOMException`) and the + * `Aborted` message thrown by `#/lib/async/cancelable` are transport + * cancellations, but the lowercase word shows up in unrelated failures - a + * multipart upload the service marked `aborted`, or a `TypeError` naming + * `abortController.abort` - which must still be reported. + */ + /\bAbort(?:ed|Error)?\b/, /network request failed/i, /failed to fetch/i, /fetch failed/i, @@ -16,24 +23,64 @@ const NETWORK_ERROR_PATTERNS = [ /sslhandshakeexception: connection closed/i, ] +/** + * `String(value)` runs code we do not control - a custom `toString`, a + * `Symbol.toPrimitive`, a `Proxy` trap - and throws outright for a + * null-prototype object, so stringifying an arbitrary thrown value can itself + * throw. Returning `undefined` lets callers treat an unreadable value as "not a + * network error" rather than letting the throw escape `logger.error()` or + * Sentry's `beforeSend`, where it would lose the original report. + */ +export function safeStringify(value: unknown): string | undefined { + try { + // oxlint-disable-next-line typescript/no-base-to-string + return String(value) + } catch { + return undefined + } +} + /** * Detects transport failures across the error strings produced by web, native * fetch, and the XRPC clients. Error causes are checked because the XRPC * clients wrap the platform-specific fetch error. + * + * Never throws: reading `cause` off a hostile value can throw just like + * stringifying it can, and a throw here would escape whatever error path is + * asking the question. */ export function isNetworkError(value: unknown): boolean { - return isNetworkErrorInner(value, new Set()) + try { + return isNetworkErrorInner(value, undefined) + } catch { + return false + } } -function isNetworkErrorInner(value: unknown, seen: Set): boolean { - if (NETWORK_ERROR_PATTERNS.some(pattern => pattern.test(String(value)))) { +/** + * `seen` stays undefined until the first recursion into an object cause, so the + * common case (a string or an error with no cause) allocates nothing. + */ +function isNetworkErrorInner( + value: unknown, + seen: Set | undefined, +): boolean { + const message = safeStringify(value) + if ( + message !== undefined && + NETWORK_ERROR_PATTERNS.some(pattern => pattern.test(message)) + ) { return true } - if (typeof value !== 'object' || value === null || seen.has(value)) { + if (typeof value !== 'object' || value === null || seen?.has(value)) { + return false + } + if (!('cause' in value)) { return false } - seen.add(value) - return 'cause' in value && isNetworkErrorInner(value.cause, seen) + const nextSeen = seen ?? new Set() + nextSeen.add(value) + return isNetworkErrorInner(value.cause, nextSeen) } diff --git a/src/lib/strings/errors.ts b/src/lib/strings/errors.ts index 31cd7a15b8..0d345b433e 100644 --- a/src/lib/strings/errors.ts +++ b/src/lib/strings/errors.ts @@ -39,7 +39,12 @@ export function cleanError(e: unknown): string { */ // oxlint-disable-next-line typescript/no-base-to-string const str = typeof e === 'string' ? e : e.toString() - if (isNetworkError(str)) { + /* + * Passed the original value, not `str`, so the wrapped-cause walk is + * reachable: the XRPC clients report the platform fetch failure as the + * `cause` of a generic message. + */ + if (isNetworkError(e)) { return t`Unable to connect. Please check your internet connection and try again.` } /*