From 8446ca0c8ad26e2a1704a2d8bd11fc306c434f5d Mon Sep 17 00:00:00 2001 From: AminDhouib Date: Thu, 27 Aug 2026 13:56:42 -0400 Subject: [PATCH] fix(core): surface a custom uploadEndpoint's presign error body MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `TokenEndpointCredentials.getPresignedUrl` threw `Presign request failed: ` without ever reading a non-ok response, so the sentence a self-hosted token endpoint wrote for the user — a plan-limit message, an expired-session notice — was discarded before any handler saw it. With `onError` typed `(errorMessage: string) => void` the thrown error's `.status` is not reachable either, which left consumers matching the HTTP status back out of upup's own message text as the only way to recover their own copy. The strategy now reads the body and builds its error through `uploadErrorFromResponse`, the helper direct-PUT, multipart, server credentials and drive transfer already use: the body's message becomes `error.message`, a `code` field lands on `error.code`, and `error.status` still carries the status. `parseErrorBody` selected its message with `error ?? msg`, so a body shaped `{ message, error: true }` took the boolean, failed the string guard, and fell through to the raw-JSON text fallback. It now prefers a *string* `error` and otherwise keeps `message`. Backward compatible by construction: same thrown class (`UpupNetworkError` via `kind: 'network'`), and when the body is empty, whitespace or unreadable the message stays byte-identical to the old wording. No public API change — `onError` keeps its signature and no export surface moves. RED before (vitest, packages/core): FAIL tests/strategies/token-endpoint.test.ts > endpoint error body > throws the endpoint's own message and code instead of the status line Expected: "File exceeds your plan's 4608MB limit. Upgrade for larger uploads." Received: "Presign request failed: 413 Payload Too Large" FAIL ... > lifts a `message` field that sits beside a non-string `error` flag FAIL ... > uses a plain-text error body verbatim FAIL src/__tests__/errors.test.ts > parseErrorBody > keeps `message` when a non-string `error` flag sits beside it Test Files 2 failed (2) Tests 4 failed | 59 passed (63) GREEN after: 63 passed (63); full core suite 1699 passed (142 files); react 644 passed, server 337 passed against a rebuilt core dist. The three pre-existing error-path tests mock a response with no `text()` at all and are left untouched, so they now double as the unreadable-body compatibility pin. --- .changeset/loud-presign-bodies.md | 24 ++++ .../docs/api-reference/error-codes.mdx | 6 + packages/core/src/__tests__/errors.test.ts | 13 ++ packages/core/src/errors.ts | 9 +- .../core/src/strategies/token-endpoint.ts | 41 +++++- .../tests/strategies/token-endpoint.test.ts | 123 +++++++++++++++++- 6 files changed, 208 insertions(+), 8 deletions(-) create mode 100644 .changeset/loud-presign-bodies.md diff --git a/.changeset/loud-presign-bodies.md b/.changeset/loud-presign-bodies.md new file mode 100644 index 000000000..5a1023672 --- /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 f65e080e0..8260a7f2d 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 18ad705a0..6ad6fd551 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 1f5d44efd..48f782de3 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 6726a7bcb..c687cf5f2 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 c6c56ffa2..ffdf2c241 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) + }) +})