diff --git a/.changeset/loud-presign-bodies.md b/.changeset/loud-presign-bodies.md new file mode 100644 index 00000000..5a102367 --- /dev/null +++ b/.changeset/loud-presign-bodies.md @@ -0,0 +1,24 @@ +--- +'@upupjs/core': patch +--- + +A custom `uploadEndpoint`'s presign failures now carry the endpoint's own error +body. `TokenEndpointCredentials.getPresignedUrl` threw +`Presign request failed: ` without ever reading a non-ok +response, so the sentence the endpoint wrote for the user — a plan-limit +message, an expired-session notice — was discarded before any handler saw it, +and the only way to recover it was to match the HTTP status out of upup's own +message text. The strategy now reads the body and builds the error through +`uploadErrorFromResponse`, the same helper the direct-PUT, multipart, server +credentials and drive-transfer strategies already use: the body's message +becomes `error.message` (what `onError` receives), a `code` field lands on +`error.code`, and `error.status` still carries the HTTP status. + +Backward compatible: the thrown class is still `UpupNetworkError`, and when the +body is empty or unreadable the message is byte-identical to before, so a +consumer matching on the old wording is unaffected. Nothing in the public +`onError` signature changes. + +`parseErrorBody` also stops discarding a valid `message` when a non-string +`error` field sits beside it — a `{ message, error: true }` body used to fall +all the way through to the raw-JSON text fallback. diff --git a/apps/landing/content/docs/api-reference/error-codes.mdx b/apps/landing/content/docs/api-reference/error-codes.mdx index f65e080e..8260a7f2 100644 --- a/apps/landing/content/docs/api-reference/error-codes.mdx +++ b/apps/landing/content/docs/api-reference/error-codes.mdx @@ -245,6 +245,12 @@ The exception is the tus strategy, which rejects with whatever error no `code`, so a handler that reads `error.code` will come up empty on tus uploads. +The presign call a custom `uploadEndpoint` makes routes through it too, so the +sentence your endpoint writes into a non-2xx body is what `onError` receives — +you do not have to recover it from an HTTP status. When the body is empty or +unreadable the message stays `Presign request failed: `, +the wording that path has always thrown. + ```typescript import { uploadErrorFromResponse } from '@upupjs/core' diff --git a/packages/core/src/__tests__/errors.test.ts b/packages/core/src/__tests__/errors.test.ts index 18ad705a..6ad6fd55 100644 --- a/packages/core/src/__tests__/errors.test.ts +++ b/packages/core/src/__tests__/errors.test.ts @@ -295,6 +295,19 @@ describe('parseErrorBody', () => { }) }) + it('keeps `message` when a non-string `error` flag sits beside it', () => { + const parsed = parseErrorBody( + JSON.stringify({ + message: "File exceeds your plan's 4608MB limit.", + error: true, + }), + ) + expect(parsed).toEqual({ + code: undefined, + message: "File exceeds your plan's 4608MB limit.", + }) + }) + it('parses an S3 XML error body', () => { const xml = '\nSignatureDoesNotMatchThe request signature we calculated does not match the signature you provided.' diff --git a/packages/core/src/errors.ts b/packages/core/src/errors.ts index 1f5d44ef..48f782de 100644 --- a/packages/core/src/errors.ts +++ b/packages/core/src/errors.ts @@ -152,10 +152,15 @@ export function parseErrorBody(body: string | undefined): { message: msg, } = parsed as { code?: string - error?: string + error?: unknown message?: string } - const message = error ?? msg + // Prefer a string `error` — the shape @upupjs/server emits — but + // never let a non-string one discard the `message` beside it: a + // `{ message, error: true }` body is common in hand-rolled + // endpoints, and reading `error` blindly turned it into a raw + // JSON dump. + const message = typeof error === 'string' ? error : msg if (typeof message === 'string' || typeof code === 'string') { return { message: message ?? '', diff --git a/packages/core/src/strategies/token-endpoint.ts b/packages/core/src/strategies/token-endpoint.ts index 6726a7bc..c687cf5f 100644 --- a/packages/core/src/strategies/token-endpoint.ts +++ b/packages/core/src/strategies/token-endpoint.ts @@ -1,10 +1,26 @@ import { - UpupNetworkError, + parseErrorBody, + uploadErrorFromResponse, type CredentialStrategy, type FileMetadata, type PresignedUrlResponse, } from '../contracts' +/** + * Read a failed presign response's body without letting the read itself become + * the failure: `text()` can reject on a torn connection, and a hand-rolled + * Response stand-in may not implement it at all. Either way the status alone + * still classifies the error. + */ +async function readErrorBody(response: Response): Promise { + try { + return await response.text() + } catch { + // upup-catch: body unreadable — fall back to the status-only message + return undefined + } +} + export class TokenEndpointCredentials implements CredentialStrategy { private url: string private headers: Record @@ -30,10 +46,25 @@ export class TokenEndpointCredentials implements CredentialStrategy { }) if (!response.ok) { - throw new UpupNetworkError( - `Presign request failed: ${response.status} ${response.statusText}`, - response.status, - ) + // A self-hosted token endpoint puts its actionable copy in the body + // ("File exceeds your plan's limit", "your sign-in expired"). This + // strategy used to drop it and throw the status line alone, which + // left consumers matching HTTP statuses out of upup's own message + // text to recover what their server had already said. + const body = await readErrorBody(response) + const error = uploadErrorFromResponse({ + status: response.status, + statusText: response.statusText, + ...(body !== undefined ? { body } : {}), + kind: 'network', + }) + if (!parseErrorBody(body).message.trim()) { + // Nothing usable in the body — keep the exact wording this + // strategy has always thrown, so a consumer matching on it + // sees no change. + error.message = `Presign request failed: ${response.status} ${response.statusText}` + } + throw error } return response.json() as Promise diff --git a/packages/core/tests/strategies/token-endpoint.test.ts b/packages/core/tests/strategies/token-endpoint.test.ts index c6c56ffa..ffdf2c24 100644 --- a/packages/core/tests/strategies/token-endpoint.test.ts +++ b/packages/core/tests/strategies/token-endpoint.test.ts @@ -1,6 +1,6 @@ import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest' import { TokenEndpointCredentials } from '../../src/strategies/token-endpoint' -import { UpupNetworkError } from '@upupjs/core' +import { UpupNetworkError, type UpupError } from '@upupjs/core' const FILE_META = { name: 'photo.jpg', size: 1024, type: 'image/jpeg' } @@ -211,3 +211,124 @@ describe('TokenEndpointCredentials — getPresignedUrl errors', () => { ) }) }) + +// ───────────────────────────────────────────── +// getPresignedUrl — the endpoint's own error body +// +// A self-hosted token endpoint is where the host app enforces its own rules +// (plan limits, auth), and it writes the sentence it wants the user to read +// into the response body. This strategy used to throw the HTTP status line and +// nothing else, so that sentence was unreachable and consumers resorted to +// matching statuses out of upup's message text. +// ───────────────────────────────────────────── +describe('TokenEndpointCredentials — endpoint error body', () => { + afterEach(() => { + vi.unstubAllGlobals() + }) + + function failWith(init: { + status: number + statusText: string + body?: string + }): TokenEndpointCredentials { + vi.stubGlobal( + 'fetch', + vi.fn().mockResolvedValue({ + ok: false, + status: init.status, + statusText: init.statusText, + text: () => Promise.resolve(init.body ?? ''), + }), + ) + return new TokenEndpointCredentials({ + url: 'https://example.com/presign', + }) + } + + async function presignError( + creds: TokenEndpointCredentials, + ): Promise { + return (await creds + .getPresignedUrl(FILE_META) + .catch((e: unknown) => e)) as UpupError + } + + it("throws the endpoint's own message and code instead of the status line", async () => { + const creds = failWith({ + status: 413, + statusText: 'Payload Too Large', + body: JSON.stringify({ + error: "File exceeds your plan's 4608MB limit. Upgrade for larger uploads.", + code: 'PLAN_FILE_SIZE_EXCEEDED', + }), + }) + const err = await presignError(creds) + expect(err.message).toBe( + "File exceeds your plan's 4608MB limit. Upgrade for larger uploads.", + ) + expect(err.code).toBe('PLAN_FILE_SIZE_EXCEEDED') + expect(err.status).toBe(413) + }) + + it('lifts a `message` field that sits beside a non-string `error` flag', async () => { + const creds = failWith({ + status: 413, + statusText: 'Payload Too Large', + body: JSON.stringify({ + message: "File exceeds your plan's 4608MB limit.", + error: true, + }), + }) + const err = await presignError(creds) + expect(err.message).toBe("File exceeds your plan's 4608MB limit.") + }) + + it('uses a plain-text error body verbatim', async () => { + const creds = failWith({ + status: 403, + statusText: 'Forbidden', + body: 'Your sign-in expired before the upload started.', + }) + const err = await presignError(creds) + expect(err.message).toBe( + 'Your sign-in expired before the upload started.', + ) + }) + + it('still throws UpupNetworkError carrying the status', async () => { + const creds = failWith({ + status: 500, + statusText: 'Internal Server Error', + body: JSON.stringify({ error: 'presign threw' }), + }) + const err = await presignError(creds) + expect(err).toBeInstanceOf(UpupNetworkError) + expect(err.status).toBe(500) + }) + + it('keeps the legacy wording when the body is empty', async () => { + const creds = failWith({ status: 413, statusText: 'Payload Too Large' }) + const err = await presignError(creds) + expect(err.message).toBe( + 'Presign request failed: 413 Payload Too Large', + ) + }) + + it('keeps the legacy wording when the body cannot be read', async () => { + vi.stubGlobal( + 'fetch', + vi.fn().mockResolvedValue({ + ok: false, + status: 502, + statusText: 'Bad Gateway', + text: () => Promise.reject(new Error('body stream errored')), + }), + ) + const creds = new TokenEndpointCredentials({ + url: 'https://example.com/presign', + }) + const err = await presignError(creds) + expect(err.message).toBe('Presign request failed: 502 Bad Gateway') + expect(err.status).toBe(502) + }) +})