From c90ef25f7909513f4cf5fe6c88a4cd9781cd3672 Mon Sep 17 00:00:00 2001 From: AminDhouib Date: Wed, 12 Aug 2026 09:34:08 -0400 Subject: [PATCH 01/14] docs(migration): warn on dead v1 server validation and new default capture sources (#344, #340) --- .../content/docs/guides/server-auth.mdx | 10 +- .../content/docs/migration/v1-to-v3.mdx | 99 +++++++++++++++++++ 2 files changed, 108 insertions(+), 1 deletion(-) diff --git a/apps/landing/content/docs/guides/server-auth.mdx b/apps/landing/content/docs/guides/server-auth.mdx index c974065ca..0d38b10a9 100644 --- a/apps/landing/content/docs/guides/server-auth.mdx +++ b/apps/landing/content/docs/guides/server-auth.mdx @@ -203,7 +203,15 @@ paths before any storage call: An `onBeforeUpload` hook — configured under `hooks`, i.e. `config.hooks.onBeforeUpload`, not top-level — can reject a specific upload -(**403** `Upload rejected`) with your own logic. +(**403** `Upload rejected`) with your own logic. It receives the same +`{ name, size, type }` metadata plus the raw `Request`, which is where per-user +rules (plan caps, quotas) belong. + +These are the only size and type limits on the server. The `` +props (`maxFiles`, `maxFileSize`, `allowedFileTypes`) are client-side UX and +never reach this handler — if you are porting a v1 route that read its limits +out of the request body, see +[Re-check upload limits on the server](/docs/migration/v1-to-v3/#re-check-upload-limits-on-the-server). ## What forged and unsigned requests get diff --git a/apps/landing/content/docs/migration/v1-to-v3.mdx b/apps/landing/content/docs/migration/v1-to-v3.mdx index 1cf2f1cbd..a92e8a1e7 100644 --- a/apps/landing/content/docs/migration/v1-to-v3.mdx +++ b/apps/landing/content/docs/migration/v1-to-v3.mdx @@ -138,6 +138,26 @@ Adapter → source id mapping: | `CAMERA` | `'camera'` | | _(new)_ | `'box'`, `'screen'`, `'microphone'` | + + v1's default surface was effectively local files plus link imports. v3 + defaults `sources` to `['local', 'url', 'camera', 'microphone', 'screen']`, + so an uploader migrated **without** a `sources` prop silently grows three + capture panels. Opening one asks the user for camera or microphone + permission, and what it produces is a recording: `video/webm` for screen, + `audio/webm` for microphone, `image/jpeg` for a camera photo. The exact + recording type comes from the browser's `MediaRecorder` and may carry a + `;codecs=…` suffix, so match it with a `video/*`-style wildcard rather than + an exact string. If those types are not in your `allowedFileTypes` (or your + server's `allowedTypes`), a user can record a clip and then watch it be + rejected — a dead end with no way forward. + + +Pass `sources` explicitly to keep the v1 surface: + +```tsx + +``` + `driveConfigs` becomes `cloudDrives`, and the snake_case keys become camelCase: ```diff @@ -387,6 +407,76 @@ See [Server Mode — Setup](/docs/guides/server-mode-setup/) for the Next.js, Express, Fastify, and Hono adapters, and [Upload modes](/docs/guides/modes/) for choosing between them. +### Re-check upload limits on the server + + + v1 clients sent the uploader's configured constraints along with the presign + request, so a v1 route could read them back and enforce them. v3 does not: + the presign body is `{ name, type, size, metadata }` and carries **no + constraints at all**. A v1 route ported field-for-field keeps answering + `200` while validating nothing — the limits it reads off the body are now + `undefined`, and every guard they feed passes. Nothing fails loudly, so the + gap usually surfaces only when a billing or abuse cap turns out never to + have been enforced. + + +`maxFiles`, `maxFileSize`, and `allowedFileTypes` are **client-side UX props**: +they keep the picker honest, and that is all they do in either version. Anyone +can POST to your presign route directly, so every limit you actually rely on has +to be re-checked where the URL is signed. + +With `@upupjs/server`, flat policy is declarative and per-user policy goes in the +`onBeforeUpload` hook. Both run on `/presign` and `/multipart/init` before any +storage call: + +```ts +import { createUpupHandler } from '@upupjs/server' + +const handler = createUpupHandler({ + storage: { + type: 'aws', + bucket: process.env.S3_BUCKET!, + region: process.env.S3_REGION!, + }, + uploadTokenSecret: process.env.UPUP_UPLOAD_TOKEN_SECRET!, + getUserId: async req => resolveUser(req), + + // Flat policy: 413 over the size cap, 415 on a disallowed type. + maxFileSize: 25 * 1024 * 1024, // bytes — not the { size, unit } prop shape + allowedTypes: ['image/*', 'application/pdf'], + + hooks: { + // Per-user policy. `file` is { name, size, type }; `req` is the raw + // Request. Returning false rejects with 403 "Upload rejected". + onBeforeUpload: async (file, req) => { + const plan = await resolvePlan(req) + if (file.size > plan.perFileByteCap) return false + if (plan.tier === 'free' && !file.type.startsWith('image/')) + return false + return await withinMonthlyQuota(plan, file.size) + }, + }, +}) +``` + +If you kept a hand-rolled client-mode route instead, do the same work there: read +`name`, `type`, and `size` off the request body, reject before signing, and sign +for exactly the size you approved. The presigned `PUT` signature covers +`content-length`, so a client that later sends a bigger body is rejected by +storage rather than quietly landing an oversized object. + + + The server's `maxFileSize` is a **number of bytes**, not the + `{ size, unit }` object the `` prop takes. They are separate + settings that happen to share a name — one gates the picker, the other gates + the signature. + + +See [Server Auth & Trust Model](/docs/guides/server-auth/#metadata-policy-size-and-type) +for the full policy surface, and +[`POST /presign`](/docs/api-reference/server-http/#post-presign) for the exact +request shape and status codes. + ## Full example: before and after **v1** (`upup-react-file-uploader`): @@ -468,6 +558,15 @@ export default function Uploader() { Behavioral differences to check after the mechanical rename: +- **Your v1 server-side size/type validation silently stops applying.** v3's + presign body carries no constraints, so a ported v1 route keeps returning + `200` while checking nothing. Re-enforce the limits where the URL is signed — + see [Re-check upload limits on the server](#re-check-upload-limits-on-the-server). +- **`sources` defaults to five sources, including camera, microphone, and + screen recording.** Omit the prop and your app gains capture panels it never + had in v1 — and recordings your `allowedFileTypes` may reject. Pass + `sources={['local', 'url']}` for the v1 surface; see + [Sources & cloud drives](#sources--cloud-drives). - **`maxFiles` defaults to 10, not 1.** v1's `limit` defaulted to `1`. If your app relied on single-file selection, set `maxFiles={1}` (or use `mini`, which forces a single file). From 7ffe28918cb19e8ae54b9ada21ed54dece59b5ff Mon Sep 17 00:00:00 2001 From: AminDhouib Date: Wed, 12 Aug 2026 09:35:20 -0400 Subject: [PATCH 02/14] fix(ci): set GH_REPO so the nightly tracking-issue arms work without a checkout --- .github/workflows/nightly.yml | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/.github/workflows/nightly.yml b/.github/workflows/nightly.yml index 78f733b26..28a7f14e1 100644 --- a/.github/workflows/nightly.yml +++ b/.github/workflows/nightly.yml @@ -517,6 +517,9 @@ jobs: if: github.event_name == 'schedule' && steps.status.outputs.failed == 'true' env: GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} + # No checkout in this job — without GH_REPO, gh tries to infer + # the repo from a git remote and dies ("not a git repository"). + GH_REPO: ${{ github.repository }} RUN_URL: ${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }} FAILED_JOBS: ${{ steps.status.outputs.jobs }} run: | @@ -555,6 +558,7 @@ jobs: if: github.event_name == 'schedule' && steps.status.outputs.failed == 'false' env: GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} + GH_REPO: ${{ github.repository }} RUN_URL: ${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }} run: | existing=$(gh issue list \ From 34e132da801044cf7b625322840091a464bfda9d Mon Sep 17 00:00:00 2001 From: AminDhouib Date: Wed, 12 Aug 2026 09:43:51 -0400 Subject: [PATCH 03/14] fix(react): stop restriction failures leaking as unhandled rejections (#342) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit core.addFiles() emits `restriction-failed` and then rethrows. The rethrow is deliberate — a direct `await core.addFiles()` caller narrows on it — but the react headless surface reaches addFiles from three fire-and-forget DOM callbacks that have nobody to rethrow to: - getInputProps().onChange -> `void addFiles(...)` - getDropzoneProps().onDrop -> `void dragDrop.handleDrop(...)` - useUpupUpload's DragDropDeps.setFiles -> bare `core.addFiles(files)`, which the controller invokes un-awaited for BOTH drop and paste So every ordinary user-facing restriction failure (wrong type, too large, over the limit) surfaced a second time as an unhandled promise rejection and polluted error reporting. Fix: swallow the rejection on those three paths only. Nothing is lost — core emits `restriction-failed` carrying the identical error BEFORE it throws, so the event bus is the surviving channel. `await addFiles()` still rejects. Census: the visual panels in all six frameworks were never affected — their setFiles routes through createUploaderController's handleSetSelectedFiles, which try/catches and never rejects (vue's useUploaderController already documents this as "the void contract"). This headless dep was the sole call site reaching core.addFiles bare, so core's DragDropController is untouched. RED before the fix (tests/prop-getters-unhandled-rejection.test.ts): Test Files 1 failed (1) Tests 4 failed | 1 passed (5) AssertionError: expected [ ...(1) ] to deeply equal [] - [] + [ + UpupValidationError { + "message": "File type \"text/plain\" is not accepted", + "code": "TYPE_MISMATCH", + "reason": "TYPE_MISMATCH", + }, + ] The one test that passed RED is "leaves a DIRECT await core.addFiles() rejection intact" — pinning that the rethrow itself is preserved. GREEN after: 5 passed (5); full @upupjs/react suite 611 passed (69 files); `pnpm --filter @upupjs/react typecheck` exit 0. --- packages/react/src/prop-getters.ts | 22 ++- packages/react/src/use-upup-upload.ts | 11 +- .../prop-getters-unhandled-rejection.test.ts | 179 ++++++++++++++++++ 3 files changed, 209 insertions(+), 3 deletions(-) create mode 100644 packages/react/tests/prop-getters-unhandled-rejection.test.ts diff --git a/packages/react/src/prop-getters.ts b/packages/react/src/prop-getters.ts index e65d6809d..8792064ca 100644 --- a/packages/react/src/prop-getters.ts +++ b/packages/react/src/prop-getters.ts @@ -34,6 +34,24 @@ function composeEventHandlers( } } +/** + * Detach a fire-and-forget promise from the unhandled-rejection channel (#342). + * + * `core.addFiles()` emits `restriction-failed` and THEN rethrows — the rethrow + * is deliberate, so a direct `await core.addFiles()` caller can narrow on the + * error. But a DOM callback has nobody to rethrow to, so an ordinary restriction + * failure (wrong type, too large, over the limit) used to surface a second time + * as an unhandled rejection and pollute error reporting. Dropping it here loses + * nothing: the event bus already carried the identical error before the throw. + */ +function ignoreRejection(result: Promise | void): void { + // Duck-typed rather than `instanceof Promise` — a dep may hand back a + // thenable from another realm, which `instanceof` would silently miss. + if (result && typeof result.catch === 'function') { + result.catch(() => {}) + } +} + export interface PropGetters { getDropzoneProps: ( overrides?: HTMLAttributes, @@ -69,7 +87,7 @@ export function createPropGetters(deps: PropGetterDeps): PropGetters { dragDrop?.handleDragLeave(e as unknown as DragEvent) } const onDrop = (e: React.DragEvent): void => { - void dragDrop?.handleDrop(e as unknown as DragEvent) + ignoreRejection(dragDrop?.handleDrop(e as unknown as DragEvent)) } const onPaste = (e: React.ClipboardEvent): void => { dragDrop?.handlePaste(e as unknown as ClipboardEvent) @@ -118,7 +136,7 @@ export function createPropGetters(deps: PropGetterDeps): PropGetters { const onChange: ChangeEventHandler = e => { const fileList = e.target.files if (fileList) { - void addFiles(Array.from(fileList)) + ignoreRejection(addFiles(Array.from(fileList))) } } return { diff --git a/packages/react/src/use-upup-upload.ts b/packages/react/src/use-upup-upload.ts index 10c37a27f..c760babb7 100644 --- a/packages/react/src/use-upup-upload.ts +++ b/packages/react/src/use-upup-upload.ts @@ -132,7 +132,16 @@ export function useUpupUpload( // the headless hook's existing drop/paste semantics; only the // GATING (enablePaste/isProcessing/folder-drop/filename/events) // was the bug, not the append-vs-replace choice. - setFiles: files => core.addFiles(files), + // + // The .catch honors DragDropDeps.setFiles' void contract (#342): + // the controller fires this drop/paste path without awaiting, and + // core.addFiles rethrows restriction failures AFTER emitting + // `restriction-failed`. Every other framework satisfies the same + // contract via handleSetSelectedFiles, which try/catches; this + // headless dep is the one that reached core.addFiles bare. + setFiles: files => { + core.addFiles(files).catch(() => {}) + }, filesSize: () => orchestrator.getSnapshot().files.size, options: () => ({ ...(optionsRef.current.enablePaste !== undefined diff --git a/packages/react/tests/prop-getters-unhandled-rejection.test.ts b/packages/react/tests/prop-getters-unhandled-rejection.test.ts new file mode 100644 index 000000000..3bfe27ce0 --- /dev/null +++ b/packages/react/tests/prop-getters-unhandled-rejection.test.ts @@ -0,0 +1,179 @@ +import { describe, it, expect, afterEach } from 'vitest' +import { renderHook, act } from '@testing-library/react' +import { useUpupUpload } from '../src/use-upup-upload' + +// Issue #342 — core's addFiles() emits `restriction-failed` and then RETHROWS. +// The rethrow is deliberate (direct `await core.addFiles()` callers narrow on it), +// but the headless surface reaches addFiles from three FIRE-AND-FORGET DOM +// callbacks — the hidden input's onChange, and DragDropController's drop/paste, +// whose setFiles dep in useUpupUpload is core.addFiles itself. Nothing awaits +// those promises, so every ordinary user-facing restriction failure ALSO +// surfaced as an unhandled promise rejection (Sentry noise for a handled event). +// +// The contract these tests pin: on a fire-and-forget path the rejection is +// swallowed, because `restriction-failed` has ALREADY carried the same error to +// the event bus — core emits it before it rethrows, so nothing is lost. The +// visual panels never had this bug: their setFiles routes through +// createUploaderController's handleSetSelectedFiles, which try/catches. + +type RejectionListener = (reason: unknown) => void + +/** + * Vitest runs on Node even under the jsdom environment, but @types/node is not + * in this package's test tsconfig — so reach the process object through a local + * structural type instead of widening the package's global types for one file. + */ +interface NodeProcessLike { + on(event: 'unhandledRejection', listener: RejectionListener): void + off(event: 'unhandledRejection', listener: RejectionListener): void +} +const nodeProcess = (globalThis as unknown as { process: NodeProcessLike }) + .process + +/** + * Node's default unhandled-rejection mode is `throw`, which is bypassed as soon + * as an 'unhandledRejection' listener exists — so registering one both captures + * the event and keeps a RED run from taking the whole worker down. + */ +function captureUnhandledRejections() { + const seen: unknown[] = [] + const listener: RejectionListener = reason => { + seen.push(reason) + } + nodeProcess.on('unhandledRejection', listener) + return { + seen, + /** Node decides a rejection is unhandled only after the microtask queue + * drains; two macrotask turns is the window it needs to emit. */ + async settle() { + await new Promise(resolve => setTimeout(resolve, 0)) + await new Promise(resolve => setTimeout(resolve, 0)) + }, + stop() { + nodeProcess.off('unhandledRejection', listener) + }, + } +} + +let capture: ReturnType | null = null + +afterEach(() => { + capture?.stop() + capture = null +}) + +/** An options bag whose only restriction is a type filter the test files fail. */ +const IMAGES_ONLY = { + provider: 'S3' as const, + allowedFileTypes: 'image/*', + enablePaste: true, +} + +const REJECTED_FILE = () => new File(['x'], 'notes.txt', { type: 'text/plain' }) + +function makeChangeEvent(files: File[]) { + return { + target: { files }, + } as unknown as React.ChangeEvent +} + +function makeDragEvent(files: File[]) { + return { + preventDefault: () => {}, + dataTransfer: { + dropEffect: '', + files, + items: files.map(f => ({ + kind: 'file', + webkitGetAsEntry: () => null, + getAsFile: () => f, + })), + }, + } as unknown as React.DragEvent +} + +function makeClipboardEvent(files: File[]) { + return { + preventDefault: () => {}, + clipboardData: { + items: files.map(f => ({ kind: 'file', getAsFile: () => f })), + }, + } as unknown as React.ClipboardEvent +} + +describe('useUpupUpload — restriction failures on fire-and-forget paths (#342)', () => { + it('getInputProps().onChange does not raise an unhandled rejection', async () => { + capture = captureUnhandledRejections() + const { result } = renderHook(() => useUpupUpload(IMAGES_ONLY)) + + await act(async () => { + result.current.getInputProps().onChange!( + makeChangeEvent([REJECTED_FILE()]), + ) + }) + await capture.settle() + + expect(capture.seen).toEqual([]) + expect(result.current.files.length).toBe(0) + }) + + it('getDropzoneProps().onDrop does not raise an unhandled rejection', async () => { + capture = captureUnhandledRejections() + const { result } = renderHook(() => useUpupUpload(IMAGES_ONLY)) + + await act(async () => { + result.current.getDropzoneProps().onDrop!( + makeDragEvent([REJECTED_FILE()]), + ) + }) + await capture.settle() + + expect(capture.seen).toEqual([]) + expect(result.current.files.length).toBe(0) + }) + + it('getDropzoneProps().onPaste does not raise an unhandled rejection', async () => { + capture = captureUnhandledRejections() + const { result } = renderHook(() => useUpupUpload(IMAGES_ONLY)) + + await act(async () => { + result.current.getDropzoneProps().onPaste!( + makeClipboardEvent([REJECTED_FILE()]), + ) + }) + await capture.settle() + + expect(capture.seen).toEqual([]) + expect(result.current.files.length).toBe(0) + }) + + it('still emits restriction-failed carrying the error the rejection would have carried', async () => { + capture = captureUnhandledRejections() + const seenErrors: unknown[] = [] + const { result } = renderHook(() => useUpupUpload(IMAGES_ONLY)) + + act(() => { + result.current.on('restriction-failed', payload => { + seenErrors.push(payload.error) + }) + }) + await act(async () => { + result.current.getInputProps().onChange!( + makeChangeEvent([REJECTED_FILE()]), + ) + }) + await capture.settle() + + expect(capture.seen).toEqual([]) + expect(seenErrors.length).toBe(1) + expect(seenErrors[0]).toBeInstanceOf(Error) + }) + + it('leaves a DIRECT await core.addFiles() rejection intact (the rethrow is deliberate)', async () => { + const { result } = renderHook(() => useUpupUpload(IMAGES_ONLY)) + + await expect( + result.current.addFiles([REJECTED_FILE()]), + ).rejects.toThrow() + }) +}) From 6f6b395d30f668b3ca369bcb5c745cceb196c06f Mon Sep 17 00:00:00 2001 From: AminDhouib Date: Wed, 12 Aug 2026 09:47:13 -0400 Subject: [PATCH 04/14] fix(react): give all three prop getters one override contract (#341) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The three getters each merged `overrides` differently, so what happened to a key depended on which getter you passed it to: - getDropzoneProps composed the four drag handlers and DROPPED every other override key — className/style/id silently vanished - getRootProps spread overrides, then wrote its own keys over the top, including a literal `aria-describedby: undefined` that deleted a caller's value - getInputProps spread overrides, then replaced `style` wholesale with `{ display: 'none' }` All three now follow ONE rule: 1. `...overrides` is spread FIRST — anything you pass survives. 2. Getter-OWNED keys are applied after and win, limited to values derived from live core state plus the ones without which the element stops being an uploader element: root -> aria-busy dropzone -> aria-dropeffect input -> type, multiple, accept, style.display 3. Event handlers are COMPOSED, never replaced: the getter's handler runs first, then the caller's (the order composeEventHandlers already used, so no behavior change for existing handler overrides). 4. `style` is MERGED, not replaced. 5. role / aria-label / tabIndex / aria-hidden become overridable DEFAULTS. `accept` is claimed only when core actually declares `allowedFileTypes` — writing `undefined` unconditionally would delete a caller's own accept, which is the same silent-drop bug this contract exists to end. Headless-surface only: createPropGetters has exactly two call sites (itself and useUpupUpload), the visual components render their own input, so no DOM contract string or parity fixture is touched. RED before the fix (tests/prop-getters-override-contract.test.ts): Test Files 1 failed (1) Tests 7 failed | 14 passed (21) x getDropzoneProps keeps non-handler override keys expected undefined to be 'my-zone' x getRootProps does not clobber an aria-describedby override with undefined expected undefined to be 'help-text' x an accept override survives when core declares no file-type filter expected undefined to be '.csv' x getInputProps keeps override style keys and still hides the input expected { display: 'none' } to deeply equal { Object (position, width, ...) } x root role and aria-label are overridable expected 'application' to be 'group' x dropzone role, aria-label and tabIndex are overridable expected 'region' to be 'button' x input tabIndex and aria-hidden are overridable expected -1 to be +0 GREEN after: 21 passed (21); full @upupjs/react suite 632 passed (70 files) — the pre-existing prop-getters tests still pass unchanged, since every default is preserved when no override is supplied. typecheck exit 0. Docs: apps/landing/content/docs/guides/headless.mdx gains a "Prop getters and overrides" section stating the five rules and tabulating the owned keys. --- apps/landing/content/docs/guides/headless.mdx | 43 ++- packages/react/src/prop-getters.ts | 55 +++- .../prop-getters-override-contract.test.ts | 294 ++++++++++++++++++ 3 files changed, 381 insertions(+), 11 deletions(-) create mode 100644 packages/react/tests/prop-getters-override-contract.test.ts diff --git a/apps/landing/content/docs/guides/headless.mdx b/apps/landing/content/docs/guides/headless.mdx index fab0b162e..4f6318da8 100644 --- a/apps/landing/content/docs/guides/headless.mdx +++ b/apps/landing/content/docs/guides/headless.mdx @@ -83,7 +83,8 @@ function MyUploader() { typed; namespaced `':'` names pass through. - **Prop getters** — `getRootProps()`, `getInputProps()`, `getDropzoneProps()` spread onto your own elements to wire drag/drop, click-to-browse, and the - hidden file input. + hidden file input. Each accepts an overrides object — see + [Prop getters and overrides](#prop-getters-and-overrides). - **Escape hatch** — `core: UpupCore`, the underlying engine instance, plus `ext` for plugin-contributed methods. @@ -92,6 +93,46 @@ A `UploadFile` extends the native `File`, so `file.name`, `file.size`, and `file.source`, `file.key`, and `file.metadata`. `UploadStatus` is the enum `IDLE | PROCESSING | READY | UPLOADING | PAUSED | SUCCESSFUL | FAILED`. +### Prop getters and overrides + +Every getter takes an optional overrides object and merges it by one rule: + +```tsx +
+ +
+
+``` + +1. **Your overrides are spread first**, so anything you pass survives — + `className`, `id`, `style`, `data-*`, and ARIA attributes all reach the + element. +2. **A short list of getter-owned keys is applied after and wins.** These are + the values derived from live uploader state, plus the ones without which the + element stops working as an uploader element: + +| Getter | Getter-owned keys | +| -------------------- | -------------------------------------------------------------------------------- | +| `getRootProps()` | `aria-busy` (tracks upload status) | +| `getDropzoneProps()` | `aria-dropeffect` (tracks drag state) | +| `getInputProps()` | `type`, `multiple`, `accept`, `style.display` | + +3. **Event handlers are composed, never replaced.** The getter's own handler + runs first, then yours — pass `onDrop`, `onDragOver`, `onDragLeave`, + `onPaste`, or `onChange` and the uploader keeps working alongside it. There + is no way to accidentally unwire drag/drop by passing your own handler. +4. **`style` is merged, not replaced.** `getInputProps()` owns only + `display: 'none'`; every other style key you pass survives, so you can + position or size the visually-hidden input. +5. **Everything else is a default you may override** — `role`, `aria-label`, + `tabIndex`, and `aria-hidden`. Override `aria-label` to localize it. + +> **Why `multiple` and `accept` are owned.** They mirror the hook's own `limit` +> and `allowedFileTypes` options, so the file picker cannot advertise a +> selection the engine would then reject. `accept` is only claimed when you +> actually set `allowedFileTypes`; with no filter configured, your own `accept` +> passes straight through. + ### Options The hook's options are the engine's `CoreOptions` plus a few convenience diff --git a/packages/react/src/prop-getters.ts b/packages/react/src/prop-getters.ts index 8792064ca..4fa578e27 100644 --- a/packages/react/src/prop-getters.ts +++ b/packages/react/src/prop-getters.ts @@ -52,6 +52,25 @@ function ignoreRejection(result: Promise | void): void { } } +/** + * All three getters share ONE override contract (#341): + * + * 1. `...overrides` is spread first — anything you pass survives by default. + * 2. Getter-OWNED keys are applied after the spread and win. Only two kinds + * qualify: values derived from live core state, and the handful without + * which the element stops being an uploader element. + * - `getRootProps` → `aria-busy` + * - `getDropzoneProps` → `aria-dropeffect` + * - `getInputProps` → `type`, `multiple`, `accept` (only when core + * declares a filter), `style.display` + * 3. Event handlers are COMPOSED, never replaced: the getter's own handler + * runs first, then yours. + * 4. `style` is MERGED, not replaced. + * 5. Everything else the getters set — `role`, `aria-label`, `tabIndex`, + * `aria-hidden` — is a DEFAULT you may override. + * + * Pinned by tests/prop-getters-override-contract.test.ts. + */ export interface PropGetters { getDropzoneProps: ( overrides?: HTMLAttributes, @@ -94,6 +113,14 @@ export function createPropGetters(deps: PropGetterDeps): PropGetters { } return { + // Defaults first so a caller can replace the descriptive ones. + role: 'region' as const, + 'aria-label': 'Drop files here or click to browse', + tabIndex: 0, + ...overrides, + // Owned: derived from live drag state. + 'aria-dropeffect': isDragging ? 'copy' : 'none', + // Owned: composed, so the delegation can never be replaced away. onDragOver: composeEventHandlers>( onDragOver, overrides.onDragOver, @@ -110,10 +137,6 @@ export function createPropGetters(deps: PropGetterDeps): PropGetters { onPaste, overrides.onPaste, ), - role: 'region' as const, - 'aria-label': 'Drop files here or click to browse', - 'aria-dropeffect': isDragging ? 'copy' : 'none', - tabIndex: 0, } } @@ -122,11 +145,11 @@ export function createPropGetters(deps: PropGetterDeps): PropGetters { ): HTMLAttributes { const isUploading = status === 'uploading' return { - ...overrides, role: 'application' as const, 'aria-label': 'File uploader', + ...overrides, + // Owned: derived from live upload status. 'aria-busy': isUploading, - 'aria-describedby': undefined as string | undefined, } } @@ -140,17 +163,29 @@ export function createPropGetters(deps: PropGetterDeps): PropGetters { } } return { + tabIndex: -1, + 'aria-hidden': true as const, ...overrides, + // Owned: without type=file the element is not a file picker at all. type: 'file' as const, + // Owned: mirrors the uploader's own `limit` / `allowedFileTypes`, so + // the picker can't advertise a selection core would then reject. + // `accept` is only claimed when core actually declares a filter — + // writing `undefined` unconditionally would delete a caller's own + // accept, which is the silent-drop bug this contract exists to end. multiple, - accept: allowedFileTypes, + ...(allowedFileTypes !== undefined + ? { accept: allowedFileTypes } + : {}), + // Owned: composed, never replaced — an override that swapped this + // out would leave a file input that adds no files. onChange: composeEventHandlers>( onChange, overrides.onChange, ), - style: { display: 'none' as const }, - tabIndex: -1, - 'aria-hidden': true as const, + // Owned: `display` only. Every other style key a caller passes + // survives, so positioning/sizing the visually-hidden input works. + style: { ...overrides.style, display: 'none' as const }, } } diff --git a/packages/react/tests/prop-getters-override-contract.test.ts b/packages/react/tests/prop-getters-override-contract.test.ts new file mode 100644 index 000000000..fb0ad15a1 --- /dev/null +++ b/packages/react/tests/prop-getters-override-contract.test.ts @@ -0,0 +1,294 @@ +import { describe, it, expect, vi } from 'vitest' +import type React from 'react' +import type { DragDropController } from '@upupjs/core/internal' +import { createPropGetters } from '../src/prop-getters' + +// Issue #341 — the three prop getters each handled `overrides` differently: +// getRootProps spread them but then wrote getter keys (including a literal +// `aria-describedby: undefined`) over the top; getDropzoneProps composed the four +// drag handlers and DROPPED every other override key, so className/style/id +// silently vanished; getInputProps spread them but replaced `style` wholesale +// with `{ display: 'none' }`. +// +// This file pins the ONE contract all three now share: +// +// 1. `...overrides` is spread FIRST — anything passed survives by default. +// 2. Getter-OWNED keys are re-applied after the spread and win, but only the +// ones derived from core state or required for the element to function: +// root -> aria-busy +// dropzone -> aria-dropeffect +// input -> type, multiple, accept (when core has a filter), +// style.display +// 3. Event handlers are COMPOSED, never replaced: the getter's own handler +// runs first, then the caller's. +// 4. `style` is MERGED, not replaced. +// 5. Everything else the getter sets (role, aria-label, tabIndex, +// aria-hidden) is a DEFAULT the caller may override. + +function makeFakeDragDrop() { + return { + handleDragOver: vi.fn(), + handleDragLeave: vi.fn(), + handleDrop: vi.fn(), + handlePaste: vi.fn(), + } as unknown as DragDropController +} + +function makeDeps( + overrides: Partial[0]> = {}, +) { + return { + addFiles: vi.fn(), + status: 'idle', + allowedFileTypes: undefined as string | undefined, + multiple: true, + isDragging: false, + dragDrop: makeFakeDragDrop(), + ...overrides, + } +} + +const anyEvent = () => + ({ preventDefault: vi.fn() }) as unknown as React.DragEvent + +describe('prop-getter override contract (#341) — rule 1: overrides survive', () => { + it('getDropzoneProps keeps non-handler override keys', () => { + const { getDropzoneProps } = createPropGetters(makeDeps()) + const props = getDropzoneProps({ + className: 'my-zone', + id: 'zone', + style: { padding: 8 }, + 'data-testid': 'custom', + } as React.HTMLAttributes) + + expect(props.className).toBe('my-zone') + expect(props.id).toBe('zone') + expect(props.style).toEqual({ padding: 8 }) + expect((props as Record)['data-testid']).toBe('custom') + }) + + it('getRootProps keeps non-handler override keys', () => { + const { getRootProps } = createPropGetters(makeDeps()) + const props = getRootProps({ + className: 'my-root', + style: { display: 'grid' }, + } as React.HTMLAttributes) + + expect(props.className).toBe('my-root') + expect(props.style).toEqual({ display: 'grid' }) + }) + + it('getRootProps does not clobber an aria-describedby override with undefined', () => { + const { getRootProps } = createPropGetters(makeDeps()) + const props = getRootProps({ + 'aria-describedby': 'help-text', + } as React.HTMLAttributes) + + expect(props['aria-describedby']).toBe('help-text') + }) + + it('getInputProps keeps non-handler override keys', () => { + const { getInputProps } = createPropGetters(makeDeps()) + const props = getInputProps({ + name: 'upload', + className: 'sr-only', + } as React.InputHTMLAttributes) + + expect(props.name).toBe('upload') + expect(props.className).toBe('sr-only') + }) +}) + +describe('prop-getter override contract (#341) — rule 2: getter-owned keys win', () => { + it('root aria-busy is derived from status, not overridable', () => { + const { getRootProps } = createPropGetters( + makeDeps({ status: 'uploading' }), + ) + const props = getRootProps({ + 'aria-busy': false, + } as React.HTMLAttributes) + + expect(props['aria-busy']).toBe(true) + }) + + it('dropzone aria-dropeffect is derived from drag state, not overridable', () => { + const { getDropzoneProps } = createPropGetters( + makeDeps({ isDragging: true }), + ) + const props = getDropzoneProps({ + 'aria-dropeffect': 'none', + } as React.HTMLAttributes) + + expect(props['aria-dropeffect']).toBe('copy') + }) + + it('input type stays "file"', () => { + const { getInputProps } = createPropGetters(makeDeps()) + const props = getInputProps({ + type: 'text', + } as React.InputHTMLAttributes) + + expect(props.type).toBe('file') + }) + + it('input multiple/accept follow core options when core has a filter', () => { + const { getInputProps } = createPropGetters( + makeDeps({ allowedFileTypes: 'image/*', multiple: false }), + ) + const props = getInputProps({ + accept: '*/*', + multiple: true, + } as React.InputHTMLAttributes) + + expect(props.accept).toBe('image/*') + expect(props.multiple).toBe(false) + }) + + it('an accept override survives when core declares no file-type filter', () => { + const { getInputProps } = createPropGetters( + makeDeps({ allowedFileTypes: undefined }), + ) + const props = getInputProps({ + accept: '.csv', + } as React.InputHTMLAttributes) + + expect(props.accept).toBe('.csv') + }) +}) + +describe('prop-getter override contract (#341) — rule 3: handlers compose', () => { + it.each([ + ['onDragOver', 'handleDragOver'], + ['onDragLeave', 'handleDragLeave'], + ['onDrop', 'handleDrop'], + ['onPaste', 'handlePaste'], + ] as const)( + 'dropzone %s runs the getter delegation AND the override', + (propName, method) => { + const dragDrop = makeFakeDragDrop() + const override = vi.fn() + const { getDropzoneProps } = createPropGetters( + makeDeps({ dragDrop }), + ) + const e = anyEvent() + + const handler = getDropzoneProps({ + [propName]: override, + } as unknown as React.HTMLAttributes)[propName] as ( + event: unknown, + ) => void + handler(e) + + expect(dragDrop[method]).toHaveBeenCalledWith(e) + expect(override).toHaveBeenCalledWith(e) + }, + ) + + it('the getter handler runs BEFORE the override', () => { + const order: string[] = [] + const dragDrop = { + handleDragOver: vi.fn(() => order.push('getter')), + handleDragLeave: vi.fn(), + handleDrop: vi.fn(), + handlePaste: vi.fn(), + } as unknown as DragDropController + const { getDropzoneProps } = createPropGetters(makeDeps({ dragDrop })) + + getDropzoneProps({ + onDragOver: () => order.push('override'), + } as React.HTMLAttributes).onDragOver!(anyEvent()) + + expect(order).toEqual(['getter', 'override']) + }) + + it('input onChange runs addFiles AND the override', () => { + const deps = makeDeps() + const override = vi.fn() + const { getInputProps } = createPropGetters(deps) + const file = new File(['x'], 'a.txt', { type: 'text/plain' }) + const e = { + target: { files: [file] }, + } as unknown as React.ChangeEvent + + getInputProps({ + onChange: override, + } as React.InputHTMLAttributes).onChange!(e) + + expect(deps.addFiles).toHaveBeenCalledWith([file]) + expect(override).toHaveBeenCalledWith(e) + }) +}) + +describe('prop-getter override contract (#341) — rule 4: style merges', () => { + it('getInputProps keeps override style keys and still hides the input', () => { + const { getInputProps } = createPropGetters(makeDeps()) + const props = getInputProps({ + style: { position: 'absolute', width: 1 }, + } as React.InputHTMLAttributes) + + expect(props.style).toEqual({ + position: 'absolute', + width: 1, + display: 'none', + }) + }) + + it('an override cannot un-hide the input', () => { + const { getInputProps } = createPropGetters(makeDeps()) + const props = getInputProps({ + style: { display: 'block' }, + } as React.InputHTMLAttributes) + + expect(props.style?.display).toBe('none') + }) +}) + +describe('prop-getter override contract (#341) — rule 5: the rest are defaults', () => { + it('root role and aria-label are overridable', () => { + const { getRootProps } = createPropGetters(makeDeps()) + const props = getRootProps({ + role: 'group', + 'aria-label': 'Attach receipts', + } as React.HTMLAttributes) + + expect(props.role).toBe('group') + expect(props['aria-label']).toBe('Attach receipts') + }) + + it('dropzone role, aria-label and tabIndex are overridable', () => { + const { getDropzoneProps } = createPropGetters(makeDeps()) + const props = getDropzoneProps({ + role: 'button', + 'aria-label': 'Drop receipts', + tabIndex: -1, + } as React.HTMLAttributes) + + expect(props.role).toBe('button') + expect(props['aria-label']).toBe('Drop receipts') + expect(props.tabIndex).toBe(-1) + }) + + it('input tabIndex and aria-hidden are overridable', () => { + const { getInputProps } = createPropGetters(makeDeps()) + const props = getInputProps({ + tabIndex: 0, + 'aria-hidden': false, + } as React.InputHTMLAttributes) + + expect(props.tabIndex).toBe(0) + expect(props['aria-hidden']).toBe(false) + }) + + it('defaults still apply when no override is passed', () => { + const { getRootProps, getDropzoneProps, getInputProps } = + createPropGetters(makeDeps()) + + expect(getRootProps().role).toBe('application') + expect(getRootProps()['aria-label']).toBe('File uploader') + expect(getDropzoneProps().role).toBe('region') + expect(getDropzoneProps().tabIndex).toBe(0) + expect(getInputProps().tabIndex).toBe(-1) + expect(getInputProps()['aria-hidden']).toBe(true) + expect(getInputProps().style).toEqual({ display: 'none' }) + }) +}) From b336ed58f9510d4a3e2f5d0f298b0bac411e91fc Mon Sep 17 00:00:00 2001 From: AminDhouib Date: Wed, 12 Aug 2026 09:50:01 -0400 Subject: [PATCH 05/14] feat(react): re-export the core error surface (#339) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The error-handling docs tell you to narrow failures with `instanceof UpupError` and `UpupErrorCode`, and `useUpupUpload().error` is already typed `UpupError | null` — but neither name was reachable from @upupjs/react, so a react-only consumer had to add a direct @upupjs/core dependency purely to type a catch block. @upupjs/react's entry now re-exports, verbatim from @upupjs/core, 8 runtime names: UpupErrorCode UpupError UpupAuthError UpupNetworkError UpupValidationError UpupQuotaError UpupStorageError UpupConfigError plus the type-only `RestrictionFailedReason`. React's pinned public list goes 27 -> 35 runtime names; the added names are exactly the 8 above. @upupjs/preact and @upupjs/next inherit them: both are a one-line `export * from '@upupjs/react'`, and both pins assert equality WITH react's list rather than duplicating it, so they follow automatically. Verified at the runtime level rather than trusting the equality pin — probing preact's BUILT bundle after rebuilding core -> react -> preact: preact dist total exports: 35 missing: [] instanceof UpupError from preact dist: true New tests/error-exports.test.ts pins IDENTITY, not just presence: a re-export that produced a second class object would satisfy the name pin while silently breaking every `instanceof` across the package boundary. It asserts each class is the same object core exports, and that an error raised by the engine itself narrows through react-only imports. RED before the change: Test Files 2 failed (2) Tests 11 failed (11) AssertionError: expected undefined to be type of 'function' AssertionError: expected undefined to be { AUTH_EXPIRED: 'AUTH_EXPIRED', ...(22) } AssertionError: The instanceof assertion needs a constructor but undefined was given. AssertionError: expected [ Array(27) ] to deeply equal [ Array(35) ] GREEN after: react 642 passed (71 files), preact 22 passed, next 23 passed; typecheck exit 0 for react, preact and next. Deliberately NOT included: `uploadErrorFromResponse`. Despite the docs showing `import { uploadErrorFromResponse } from '@upupjs/core'`, it is exported only from @upupjs/core/internal, not core's public entry — promoting an internal to the public allow-list is a separate API decision. The stale docs import is reported to the issue batch rather than fixed here. --- packages/react/src/index.ts | 19 ++++++ packages/react/tests/error-exports.test.ts | 70 ++++++++++++++++++++++ packages/react/tests/public-api.test.ts | 13 ++++ 3 files changed, 102 insertions(+) create mode 100644 packages/react/tests/error-exports.test.ts diff --git a/packages/react/src/index.ts b/packages/react/src/index.ts index 871fee2b7..081395452 100644 --- a/packages/react/src/index.ts +++ b/packages/react/src/index.ts @@ -38,6 +38,25 @@ export type { // ── Canonical shared enums/types ───────────────────────── export { FileSource, StorageProvider } from '@upupjs/core' +// ── Error taxonomy (#339) ──────────────────────────────── +// Re-exported verbatim from @upupjs/core so catching a typed upload error +// doesn't force a react-only consumer to add a direct @upupjs/core dependency — +// the error-handling docs point at these names, and `error` on the hook's +// return value is already an `UpupError | null`. They are the SAME class +// objects core exports, so `instanceof` narrows identically whichever package +// you import from (pinned by tests/error-exports.test.ts). +export { + UpupErrorCode, + UpupError, + UpupAuthError, + UpupNetworkError, + UpupValidationError, + UpupQuotaError, + UpupStorageError, + UpupConfigError, +} from '@upupjs/core' +export type { RestrictionFailedReason } from '@upupjs/core' + // ── React types ────────────────────────────────────────── export type { ImageEditorOptions, diff --git a/packages/react/tests/error-exports.test.ts b/packages/react/tests/error-exports.test.ts new file mode 100644 index 000000000..d6dd2aecd --- /dev/null +++ b/packages/react/tests/error-exports.test.ts @@ -0,0 +1,70 @@ +import { describe, it, expect } from 'vitest' +import * as ReactPackage from '../src/index' +import * as CorePackage from '@upupjs/core' + +// Issue #339 — the error-handling docs tell you to narrow failures with +// `instanceof UpupError` / `UpupErrorCode`, but neither name was reachable from +// @upupjs/react, so a react-only consumer had to add a direct @upupjs/core +// dependency purely to type a catch block. React now re-exports the taxonomy. +// +// Identity is the load-bearing part: a re-export that produced a SECOND class +// object would still satisfy the export-name pin while silently breaking every +// `instanceof` check across the package boundary. These tests assert the +// classes are the same objects core exports, not merely present. + +const ERROR_CLASSES = [ + 'UpupError', + 'UpupAuthError', + 'UpupNetworkError', + 'UpupValidationError', + 'UpupQuotaError', + 'UpupStorageError', + 'UpupConfigError', +] as const + +describe('@upupjs/react error surface (#339)', () => { + it.each(ERROR_CLASSES)( + 're-exports %s as the same class core exports', + name => { + const fromReact = (ReactPackage as Record)[name] + const fromCore = (CorePackage as Record)[name] + + expect(fromReact).toBeTypeOf('function') + expect(fromReact).toBe(fromCore) + }, + ) + + it('re-exports UpupErrorCode as the same enum object core exports', () => { + expect(ReactPackage.UpupErrorCode).toBe(CorePackage.UpupErrorCode) + expect(ReactPackage.UpupErrorCode.TYPE_MISMATCH).toBe('TYPE_MISMATCH') + }) + + it('every subclass still narrows to UpupError through react-only imports', () => { + const error = new ReactPackage.UpupValidationError( + 'File type "text/plain" is not accepted', + ReactPackage.UpupErrorCode.TYPE_MISMATCH, + new File(['x'], 'notes.txt', { type: 'text/plain' }), + ) + + expect(error).toBeInstanceOf(ReactPackage.UpupError) + expect(error).toBeInstanceOf(CorePackage.UpupError) + expect(error.code).toBe(ReactPackage.UpupErrorCode.TYPE_MISMATCH) + }) + + it('narrows an error raised by the engine itself', async () => { + const core = new CorePackage.UpupCore({ + provider: 'S3' as const, + allowedFileTypes: 'image/*', + }) + const caught = await core + .addFiles([new File(['x'], 'notes.txt', { type: 'text/plain' })]) + .then(() => null) + .catch((e: unknown) => e) + + expect(caught).toBeInstanceOf(ReactPackage.UpupError) + expect( + (caught as InstanceType).code, + ).toBe(ReactPackage.UpupErrorCode.TYPE_MISMATCH) + core.destroy() + }) +}) diff --git a/packages/react/tests/public-api.test.ts b/packages/react/tests/public-api.test.ts index b878a114a..582a2879a 100644 --- a/packages/react/tests/public-api.test.ts +++ b/packages/react/tests/public-api.test.ts @@ -19,8 +19,21 @@ const EXPECTED_PUBLIC_VALUE_EXPORTS = [ 'OneDriveIcon', 'ScreenCaptureIcon', 'StorageProvider', + // The UpupError taxonomy, re-exported from @upupjs/core (#339) so catching + // a typed upload error does not force a direct @upupjs/core dependency. + // Same class identities as core's, so `instanceof` narrowing works no + // matter which package the consumer imported from — pinned by + // tests/error-exports.test.ts. + 'UpupAuthError', + 'UpupConfigError', + 'UpupError', + 'UpupErrorCode', + 'UpupNetworkError', + 'UpupQuotaError', + 'UpupStorageError', 'UpupThemeProvider', 'UpupUploader', + 'UpupValidationError', 'resolveAccept', 'useIsClient', 'useUploaderContext', From f014a694c84777bcca79a08a7397213b946cff26 Mon Sep 17 00:00:00 2001 From: AminDhouib Date: Wed, 12 Aug 2026 09:53:34 -0400 Subject: [PATCH 06/14] fix(react): type ignoreRejection's parameter as unknown (#342) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `Promise | void` tripped @typescript-eslint/no-invalid-void-type ("void is not valid as a constituent in a union type"). The callers' return types are themselves void-unions (`Promise | void` from PropGetterDeps.addFiles), so no narrower parameter type accepts them — take `unknown` and keep the existing duck-typed thenable check. Caught by `pnpm --filter @upupjs/react lint`, which the pre-commit hook does not run (it runs oxlint + prettier + unit suites; eslint is a pre-push gate). react: lint exit 0, typecheck exit 0, 642 passed (71 files). --- packages/react/src/prop-getters.ts | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/packages/react/src/prop-getters.ts b/packages/react/src/prop-getters.ts index 4fa578e27..d6fca3597 100644 --- a/packages/react/src/prop-getters.ts +++ b/packages/react/src/prop-getters.ts @@ -44,11 +44,14 @@ function composeEventHandlers( * as an unhandled rejection and pollute error reporting. Dropping it here loses * nothing: the event bus already carried the identical error before the throw. */ -function ignoreRejection(result: Promise | void): void { +function ignoreRejection(result: unknown): void { // Duck-typed rather than `instanceof Promise` — a dep may hand back a // thenable from another realm, which `instanceof` would silently miss. - if (result && typeof result.catch === 'function') { - result.catch(() => {}) + // `unknown` rather than `Promise | void`, because the callers' + // return types are void-unions that no narrower parameter type accepts. + const thenable = result as { catch?: (cb: () => void) => unknown } | null + if (typeof thenable?.catch === 'function') { + thenable.catch(() => {}) } } From fec4fb29daaf34bceab5474582cee5b6af2266d0 Mon Sep 17 00:00:00 2001 From: AminDhouib Date: Wed, 12 Aug 2026 10:01:05 -0400 Subject: [PATCH 07/14] feat(core): promote uploadErrorFromResponse to the public entry (#339) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CLAUDE.md names `UpupError + subclasses + uploadErrorFromResponse` as the one public error taxonomy, and apps/landing/content/docs/api-reference/ error-codes.mdx:240 already instructs users to `import { uploadErrorFromResponse } from '@upupjs/core'` — but the curated allow-list never caught up with that documented intent, so the import did not resolve. CORRECTION to the earlier report on this issue: it was NOT "exported only from ./internal". It was exported from NEITHER entry. Verified by probing the built artifacts directly rather than trusting the pins: public entry has uploadErrorFromResponse: false internal entry has uploadErrorFromResponse: false Core's own pin carried a comment asserting "(via ./internal)", which was simply wrong; that comment is corrected here. All four in-tree call sites (direct-upload, multipart-upload, server-credentials, server-transfer) reach it by relative import inside core's own src, so nothing consumed it through the internal subpath and this promotion is purely additive — ./internal and its pin are untouched, and no alias or second binding is introduced. Surfaces: - @upupjs/core `.` : 51 -> 52 runtime names (+uploadErrorFromResponse) - @upupjs/react : 35 -> 36, re-exported alongside the taxonomy so framework-only consumers get the full documented error toolkit without a direct core dependency - @upupjs/preact / @upupjs/next inherit (both are `export * from '@upupjs/react'`; both pins assert equality with react's list) Verified in preact's BUILT bundle after rebuilding core -> react -> preact, not merely via the equality pin: preact dist total exports: 36 missing: [] uploadErrorFromResponse identity === core: true built err instanceof preact UpupError: true built err instanceof preact UpupStorageError: true built err.code: STORAGE_ERROR RED before the change: core — expected [ 'ACCEPT_PRESETS', ...(50) ] to deeply equal [ ...(51) ] react — expected [ Array(35) ] to deeply equal [ Array(36) ] expected undefined to be type of 'function' Tests 1 failed | 1 passed (core), 3 failed | 10 passed (react) GREEN after: core 1585 passed (134 files), react 644 (71), preact 22, next 23; typecheck exit 0 for all four; lint 20/20; size exit 0; knip exit 0. NOT included, deliberately: the `UploadErrorFromResponseArgs` parameter interface stays unexported. Inline object-literal call sites type-check structurally, so the ergonomic gap only affects someone building the args object separately — widening core's public TYPE surface is its own decision. --- packages/core/src/index.ts | 6 ++++++ packages/core/tests/public-api.test.ts | 9 +++++++-- packages/react/src/index.ts | 1 + packages/react/tests/error-exports.test.ts | 19 +++++++++++++++++++ packages/react/tests/public-api.test.ts | 3 +++ 5 files changed, 36 insertions(+), 2 deletions(-) diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index 0363118a6..11f7b0cd7 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -51,6 +51,12 @@ export { UpupQuotaError, UpupStorageError, UpupConfigError, + // The taxonomy's response parser: turns a failed fetch/XHR response into + // the right typed subclass. Public since #339 — CLAUDE.md names it part of + // the one error surface and api-reference/error-codes.mdx already documents + // `import { uploadErrorFromResponse } from '@upupjs/core'`, but the + // allow-list never caught up, so that documented import did not resolve. + uploadErrorFromResponse, } from './errors' export type { RestrictionFailedReason } from './errors' diff --git a/packages/core/tests/public-api.test.ts b/packages/core/tests/public-api.test.ts index ec097df1b..c7cc3576d 100644 --- a/packages/core/tests/public-api.test.ts +++ b/packages/core/tests/public-api.test.ts @@ -128,8 +128,12 @@ describe('@upupjs/core public API surface (pin test)', () => { it('runtime value export list matches the curated, checked-in list', () => { // The curated public surface (D2), updated in pass 2: the legacy // parallel UploadError/UploadErrorType family was deleted (F-724) — - // the UpupError taxonomy + uploadErrorFromResponse (via ./internal) - // are the one error surface. 51 entries. + // the UpupError taxonomy + uploadErrorFromResponse are the one error + // surface. `uploadErrorFromResponse` joined this entry in #339: the + // previous "(via ./internal)" note here was wrong — it was exported + // from NEITHER entry, so the documented + // `import { uploadErrorFromResponse } from '@upupjs/core'` in + // api-reference/error-codes.mdx did not resolve. 52 entries. const EXPECTED_PUBLIC_VALUE_EXPORTS: string[] = [ 'ACCEPT_PRESETS', 'BOX_DESCRIPTOR', @@ -180,6 +184,7 @@ describe('@upupjs/core public API surface (pin test)', () => { 'resolveAccept', 'resolveTheme', 'tokensToVars', + 'uploadErrorFromResponse', 'zhCN', 'zhTW', ] diff --git a/packages/react/src/index.ts b/packages/react/src/index.ts index 081395452..fb28273f3 100644 --- a/packages/react/src/index.ts +++ b/packages/react/src/index.ts @@ -54,6 +54,7 @@ export { UpupQuotaError, UpupStorageError, UpupConfigError, + uploadErrorFromResponse, } from '@upupjs/core' export type { RestrictionFailedReason } from '@upupjs/core' diff --git a/packages/react/tests/error-exports.test.ts b/packages/react/tests/error-exports.test.ts index d6dd2aecd..ac5e2875c 100644 --- a/packages/react/tests/error-exports.test.ts +++ b/packages/react/tests/error-exports.test.ts @@ -39,6 +39,25 @@ describe('@upupjs/react error surface (#339)', () => { expect(ReactPackage.UpupErrorCode.TYPE_MISMATCH).toBe('TYPE_MISMATCH') }) + it('re-exports uploadErrorFromResponse as the same function core exports', () => { + expect(ReactPackage.uploadErrorFromResponse).toBeTypeOf('function') + expect(ReactPackage.uploadErrorFromResponse).toBe( + CorePackage.uploadErrorFromResponse, + ) + }) + + it('uploadErrorFromResponse builds an error that narrows to the re-exported classes', () => { + const err = ReactPackage.uploadErrorFromResponse({ + kind: 'storage', + status: 500, + statusText: 'Internal Server Error', + }) + + expect(err).toBeInstanceOf(ReactPackage.UpupError) + expect(err).toBeInstanceOf(ReactPackage.UpupStorageError) + expect(err.code).toBe(ReactPackage.UpupErrorCode.STORAGE_ERROR) + }) + it('every subclass still narrows to UpupError through react-only imports', () => { const error = new ReactPackage.UpupValidationError( 'File type "text/plain" is not accepted', diff --git a/packages/react/tests/public-api.test.ts b/packages/react/tests/public-api.test.ts index 582a2879a..18d1d4c2b 100644 --- a/packages/react/tests/public-api.test.ts +++ b/packages/react/tests/public-api.test.ts @@ -35,6 +35,9 @@ const EXPECTED_PUBLIC_VALUE_EXPORTS = [ 'UpupUploader', 'UpupValidationError', 'resolveAccept', + // Completes the documented error toolkit alongside the taxonomy above + // (#339) — promoted to core's public entry in the same change. + 'uploadErrorFromResponse', 'useIsClient', 'useUploaderContext', 'useUploaderEditor', From 0bf5105307a06064df6f9e75bac3020aa73041e1 Mon Sep 17 00:00:00 2001 From: AminDhouib Date: Wed, 12 Aug 2026 10:09:01 -0400 Subject: [PATCH 08/14] fix(next): make pages-handler body BodyInit-compatible with newer @types/node MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `readBody` returned the assembled Node `Buffer` straight into `toWebRequest({ body })`. Under @types/node >=22 a Buffer types as `Buffer`, which is not assignable to `BodyInit`, so the dependabot lockfile regen in PR #351 turned both `@upupjs/next:typecheck` and `@upupjs/next:build` red: src/pages-handler.ts(59,17): error TS2322: Type 'Buffer | undefined' is not assignable to type 'BodyInit | null | undefined'. Type 'Buffer' is not assignable to type 'BodyInit | null | undefined'. Copy into a fresh `Uint8Array` (same bytes, backed by a real ArrayBuffer) and annotate the helper as `RequestInit['body']` — a bare `Uint8Array` annotation means `Uint8Array` and reproduces the identical error: packages/next/src/pages-handler.ts(66,17): error TS2322: Type 'Uint8Array | undefined' is not assignable to type 'BodyInit | null | undefined'. RED before the fix (packages/next/src/__tests__/pages-handler.spec.ts): FAIL src/__tests__/pages-handler.spec.ts > createUpupPagesHandler > hands the body to the bridge as a plain Uint8Array, not a Node Buffer AssertionError: expected Buffer[ 123, 34, 110, 97, 109, ... ] to be an instance of Uint8Array > 135| expect(bridged.body).toBeInstanceOf(Uint8Array) Verified both ways: `pnpm --filter @upupjs/next typecheck` (TS 5.3.3 / @types/node 20) exit 0, and tsc 5.9.3 with typeRoots pinned to @types/node 26.1.2 over the real source exit 0. --- .../next/src/__tests__/pages-handler.spec.ts | 37 +++++++++++++++++++ packages/next/src/pages-handler.ts | 15 +++++++- 2 files changed, 50 insertions(+), 2 deletions(-) diff --git a/packages/next/src/__tests__/pages-handler.spec.ts b/packages/next/src/__tests__/pages-handler.spec.ts index 783413a98..18c3f2f80 100644 --- a/packages/next/src/__tests__/pages-handler.spec.ts +++ b/packages/next/src/__tests__/pages-handler.spec.ts @@ -14,6 +14,21 @@ vi.mock('@upupjs/server', () => ({ }, })) +// Capture the raw body value handed to the bridge, before Request swallows it. +const bridged: { body?: unknown } = {} + +vi.mock('@upupjs/server/node-bridge', async importOriginal => { + const actual = + await importOriginal() + return { + ...actual, + toWebRequest: (input: Parameters[0]) => { + bridged.body = input.body + return actual.toWebRequest(input) + }, + } +}) + import { createUpupPagesHandler } from '../pages-handler' function mockReq(opts: { @@ -66,6 +81,7 @@ function mockRes(): MockRes & NextApiResponse { beforeEach(() => { delete received.req + delete bridged.body respond = async () => new Response('{}', { status: 200 }) }) @@ -102,6 +118,27 @@ describe('createUpupPagesHandler', () => { }) }) + // A Node Buffer types as `Buffer` under @types/node >=22, + // which is not assignable to BodyInit and breaks the dts build. A plain + // Uint8Array over a real ArrayBuffer satisfies every @types/node version. + it('hands the body to the bridge as a plain Uint8Array, not a Node Buffer', async () => { + const handler = createUpupPagesHandler({} as UpupServerConfig) + await handler( + mockReq({ + method: 'POST', + url: '/api/upup-pages/presign', + headers: { 'content-type': 'application/json' }, + body: '{"name":"a.png"}', + }), + mockRes(), + ) + expect(bridged.body).toBeInstanceOf(Uint8Array) + expect(Buffer.isBuffer(bridged.body)).toBe(false) + expect(new TextDecoder().decode(bridged.body as Uint8Array)).toBe( + '{"name":"a.png"}', + ) + }) + it('does not read a body for GET', async () => { const handler = createUpupPagesHandler({} as UpupServerConfig) const req = mockReq({ diff --git a/packages/next/src/pages-handler.ts b/packages/next/src/pages-handler.ts index 43b984951..f8cf70b2f 100644 --- a/packages/next/src/pages-handler.ts +++ b/packages/next/src/pages-handler.ts @@ -24,7 +24,18 @@ function resolveBase(req: NextApiRequest, opts?: UpupNextOptions): string { return `${proto}://${host}` } -async function readBody(req: NextApiRequest): Promise { +/** + * Yields the raw body as a plain `Uint8Array`, never the `Buffer` we assemble. + * Under @types/node >=22 a `Buffer` types as `Buffer`, which is + * not assignable to `BodyInit` — copying into a fresh `Uint8Array` yields + * `Uint8Array`, which every @types/node version accepts. The return + * type is `RequestInit['body']` (the bridge's own parameter type) rather than a + * bare `Uint8Array`, because bare `Uint8Array` means `Uint8Array` + * and would reintroduce the same mismatch at the annotation. + */ +async function readBody( + req: NextApiRequest, +): Promise { const method = (req.method ?? 'GET').toUpperCase() if (method === 'GET' || method === 'HEAD') return undefined const chunks: Buffer[] = [] @@ -33,7 +44,7 @@ async function readBody(req: NextApiRequest): Promise { typeof chunk === 'string' ? Buffer.from(chunk) : (chunk as Buffer), ) } - return chunks.length ? Buffer.concat(chunks) : undefined + return chunks.length ? new Uint8Array(Buffer.concat(chunks)) : undefined } /** From 38f1d73279ee3922067b06d25bc9bf7520c53aba Mon Sep 17 00:00:00 2001 From: AminDhouib Date: Wed, 12 Aug 2026 10:15:47 -0400 Subject: [PATCH 09/14] feat(server): add getDownloadUrl + downloadUrlExpiresIn (#343) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two gaps around serving stored objects, both hit while migrating an app that serves gated downloads. 1. There was no way to sign a GET for an EXISTING key. The only signed-GET producer lived inside the upload flow, so "give me a fresh URL for a key I stored last month" meant standing up a second handler with an identity keyStrategy and using half of it. `getDownloadUrl(config, key, opts?)` is that operation on its own — no handler, no route, no token. It takes the storage slice of UpupServerConfig (pass the whole config, or `{ storage }`), and throws UpupConfigError for a storage.type with no S3 API. 2. The download-URL TTL was hardcoded at 3 days, so an app that issued 15-minute links for gated content silently got 3-day links. The new `downloadUrlExpiresIn` (seconds) on UpupServerConfig sets it for every signed GET the server hands out: `downloadUrl` on /presign and /multipart/complete, and `url` on /files/:provider/transfer. Expiry resolves per-call `expiresIn` -> `config.downloadUrlExpiresIn` -> 3 days. The upload URL's own 1-hour expiry is untouched. Public API pin (packages/server/tests/public-api.test.ts) gains ONE runtime name: `getDownloadUrl`. New exported types: `UpupStorageConfig` (the storage object, extracted from the inline UpupServerConfig shape), `DownloadUrlConfig`, `GetDownloadUrlOptions`. The non-S3 provider guard moved out of handler.ts into src/storage.ts (`assertS3Storage`) so the handler's construct-time check and getDownloadUrl's per-call check are the same code and the same message, not two copies. RED before the implementation (packages/server/tests/download-url.test.ts): FAIL tests/download-url.test.ts [ tests/download-url.test.ts ] Error: Cannot find module '/src/download-url' imported from .../packages/server/tests/download-url.test.ts > 40| import { getDownloadUrl } from '../src/download-url' Gates: server unit suite 276 passed | 35 skipped, typecheck (src + test tree) exit 0, docs:links:check / docs:api-sync:check (my entry) / docs:snippets:coverage / docs:snippets:check all OK. --- .../docs/api-reference/server-http.mdx | 36 +++++- .../content/docs/guides/server-mode-setup.mdx | 55 +++++++++ packages/server/src/config.ts | 54 +++++---- packages/server/src/download-url.ts | 48 ++++++++ packages/server/src/drive-routes.ts | 1 + packages/server/src/handler.ts | 19 +--- packages/server/src/index.ts | 3 + packages/server/src/providers/aws.ts | 21 +++- packages/server/src/storage.ts | 28 +++++ packages/server/src/transfer.ts | 22 +++- packages/server/src/upload-routes.ts | 3 + packages/server/tests/download-url.test.ts | 107 ++++++++++++++++++ packages/server/tests/public-api.test.ts | 1 + scripts/docs/api-docs-map.json | 3 +- 14 files changed, 353 insertions(+), 48 deletions(-) create mode 100644 packages/server/src/download-url.ts create mode 100644 packages/server/src/storage.ts create mode 100644 packages/server/tests/download-url.test.ts diff --git a/apps/landing/content/docs/api-reference/server-http.mdx b/apps/landing/content/docs/api-reference/server-http.mdx index 063c7c599..7dd018b88 100644 --- a/apps/landing/content/docs/api-reference/server-http.mdx +++ b/apps/landing/content/docs/api-reference/server-http.mdx @@ -166,8 +166,8 @@ type PresignedUrlResponse = { This handler returns `key`, `uploadUrl`, `downloadUrl`, `uploadHeaders`, and `expiresIn: 3600`. The PUT signature covers both `content-type` and `content-length`, so a body larger than the approved size fails at S3 rather -than silently landing. `downloadUrl` is a separately signed `GET` valid for -three days. +than silently landing. `downloadUrl` is a separately signed `GET`, valid for +three days unless `downloadUrlExpiresIn` says otherwise. **Status codes** @@ -309,7 +309,8 @@ type MultipartCompleteResponse = { } ``` -This handler returns `key`, a three-day signed `downloadUrl`, and `etag` when S3 +This handler returns `key`, a signed `downloadUrl` (three days by default, see +`downloadUrlExpiresIn`), and `etag` when S3 supplied one. **The envelope check.** Because `sign-part` and the browser's direct PUTs never @@ -552,6 +553,35 @@ safety is not a knob that can be raised away. `summary.uploadTokenTtlSeconds`. A multipart session that outlives it must be restarted from `init`. +**Download-URL TTL.** Three days by default, for every signed `GET` the handler +returns — `downloadUrl` on `/presign` and `/multipart/complete`, `url` on +`/files/:provider/transfer`. Set `downloadUrlExpiresIn` (seconds) to change it. +This is independent of the one-hour upload-URL expiry. + +## Signing a download URL outside the handler + +`getDownloadUrl` signs a `GET` for a key that already exists, without going +through any route: + +```ts +import { getDownloadUrl } from '@upupjs/server' + +const url = await getDownloadUrl(config, 'user-42/9f3a/invoice.pdf', { + expiresIn: 300, +}) +``` + +| Argument | Type | Notes | +| ---------------- | --------------------------------- | ------------------------------------------------------------ | +| `config` | `{ storage, downloadUrlExpiresIn?}` | Your `UpupServerConfig`, or any object with a `storage` slice | +| `key` | `string` | The stored object key. Signed as given — not validated | +| `opts.expiresIn` | `number` | Seconds, for this URL only | + +Expiry resolves `opts.expiresIn` → `config.downloadUrlExpiresIn` → three days. +Throws `UpupConfigError` when `storage.type` has no S3 API, matching +`createUpupHandler`'s construct-time guard. It performs **no authorization** — +decide whether the caller may read that key before you sign it. + ## Adapters Express, Fastify, Hono, `@upupjs/next` (App and Pages routers), and any custom diff --git a/apps/landing/content/docs/guides/server-mode-setup.mdx b/apps/landing/content/docs/guides/server-mode-setup.mdx index a179ba9e6..da3597e4e 100644 --- a/apps/landing/content/docs/guides/server-mode-setup.mdx +++ b/apps/landing/content/docs/guides/server-mode-setup.mdx @@ -475,6 +475,61 @@ Full request/response shapes for every route: [Server HTTP API](/docs/api-reference/server-http/). For client-side error wiring, see [Error monitoring](/docs/guides/error-monitoring/). +## Download URLs + +Every route that reports a stored object hands back a **presigned GET** — +`downloadUrl` on `/presign` and `/multipart/complete`, `url` on +`/files/:provider/transfer`. Those links expire after **3 days** by default. + +### `downloadUrlExpiresIn` + +One knob sets the TTL, in seconds, for every signed GET the server issues: + +```ts +createUpupHandler({ + // ...storage, uploadTokenSecret + downloadUrlExpiresIn: 900, // 15 minutes +}) +``` + +It covers the download half only. The **upload** URL's own 1-hour expiry is +separate and unaffected, so shortening download links never shortens the +window a large upload has to finish in. + +### Gated downloads: `getDownloadUrl` + +For content you serve later — a link on a dashboard, an email attachment, a +paywalled asset — you need a fresh URL for a key stored weeks ago, with no +upload in flight. `getDownloadUrl` is that operation on its own, with no +handler and no HTTP route involved: + +```ts +import { getDownloadUrl } from '@upupjs/server' +import { upupConfig } from './lib/upup-config' + +const url = await getDownloadUrl(upupConfig, invoice.storageKey, { + expiresIn: 300, // 5 minutes, for this link only +}) +``` + +The first argument is your server config, or any object with a `storage` +slice — it reuses the same credentials and endpoint the handler uses. Expiry +resolves in this order: + +1. the per-call `expiresIn`, +2. `config.downloadUrlExpiresIn`, +3. the 3-day default. + +Two things to keep in mind: + +- **It authorizes nothing.** It signs whatever key you hand it. Check that the + current user may see that object *before* you call it — this is the same + trust position as `S3Client.getSignedUrl`, not a replacement for your access + control. +- **S3-compatible providers only**, like the rest of the package. A + `storage.type` with no S3 API (`azure`) throws `UpupConfigError`, the same + error `createUpupHandler` throws at construct time. + ## Limits ### `maxFileSize` diff --git a/packages/server/src/config.ts b/packages/server/src/config.ts index 4d7ae1915..195992245 100644 --- a/packages/server/src/config.ts +++ b/packages/server/src/config.ts @@ -10,28 +10,31 @@ export interface KeyStrategyContext { size: number } +/** One bucket's worth of S3 / S3-compatible connection settings. */ +export interface UpupStorageConfig { + /** + * An S3 / S3-compatible provider label. @upupjs/server only speaks the S3 + * API (buildS3ClientConfig always builds an @aws-sdk/client-s3 client) — + * set `endpoint` for any non-AWS backend (MinIO/R2/DO Spaces/etc). A + * provider with no S3-compatible surface (currently `StorageProvider.Azure` + * — see @upupjs/core's NON_S3_STORAGE_PROVIDERS) is rejected by + * createUpupHandler at construct time. + */ + type: StorageProvider | string + bucket: string + region: string + accessKeyId?: string + secretAccessKey?: string + /** S3-compatible endpoint (MinIO / Cloudflare R2 / DO Spaces / on-prem). Omit for AWS S3. */ + endpoint?: string + /** Path-style addressing. Defaults to true when `endpoint` is set (required by MinIO). + * Only applies when `endpoint` is set; ignored for native AWS S3. */ + forcePathStyle?: boolean + [key: string]: unknown +} + export type UpupServerConfig = { - storage: { - /** - * An S3 / S3-compatible provider label. @upupjs/server only speaks the S3 - * API (buildS3ClientConfig always builds an @aws-sdk/client-s3 client) — - * set `endpoint` for any non-AWS backend (MinIO/R2/DO Spaces/etc). A - * provider with no S3-compatible surface (currently `StorageProvider.Azure` - * — see @upupjs/core's NON_S3_STORAGE_PROVIDERS) is rejected by - * createUpupHandler at construct time. - */ - type: StorageProvider | string - bucket: string - region: string - accessKeyId?: string - secretAccessKey?: string - /** S3-compatible endpoint (MinIO / Cloudflare R2 / DO Spaces / on-prem). Omit for AWS S3. */ - endpoint?: string - /** Path-style addressing. Defaults to true when `endpoint` is set (required by MinIO). - * Only applies when `endpoint` is set; ignored for native AWS S3. */ - forcePathStyle?: boolean - [key: string]: unknown - } + storage: UpupStorageConfig providers?: { googleDrive?: { clientId: string; clientSecret: string } @@ -63,6 +66,15 @@ export type UpupServerConfig = { */ keyStrategy?: (ctx: KeyStrategyContext) => string + /** + * TTL, in SECONDS, for the signed GET download URLs this server hands back + * (`downloadUrl` on the presign / multipart-complete / drive-transfer + * responses, and `getDownloadUrl`'s result). Defaults to 3 days. Lower it + * for gated content — a 15-minute link is `900`. This is the download half + * only; the upload URL's own 1-hour expiry is unaffected. + */ + downloadUrlExpiresIn?: number + /** * Permit drive providers / tokenStore WITHOUT a getUserId resolver, collapsing * every caller into one shared anonymous namespace. Demos only — never in diff --git a/packages/server/src/download-url.ts b/packages/server/src/download-url.ts new file mode 100644 index 000000000..4f137616c --- /dev/null +++ b/packages/server/src/download-url.ts @@ -0,0 +1,48 @@ +// packages/server/src/download-url.ts +// +// Sign a GET for an object that ALREADY exists (#343). Before this, the only +// signed-GET producer was buried in the upload flow, so "give me a fresh +// download URL for a key I stored last month" meant standing up a second +// handler with an identity keyStrategy and using half of it. This is that +// operation on its own, with no handler, no routing, and no token involved. +// +// It is read-only and does NOT authorize anything: callers are responsible for +// deciding whether the current user may see the key they pass in. + +import type { UpupServerConfig, UpupStorageConfig } from './config' +import { assertS3Storage } from './storage' +import { + generateSignedPublicUrl, + DEFAULT_DOWNLOAD_URL_EXPIRES_IN, +} from './providers/aws' + +/** The slice of UpupServerConfig getDownloadUrl needs — pass your whole server + * config, or just `{ storage }`. */ +export type DownloadUrlConfig = { + storage: UpupStorageConfig + downloadUrlExpiresIn?: UpupServerConfig['downloadUrlExpiresIn'] +} + +export interface GetDownloadUrlOptions { + /** TTL in seconds for this URL only. Wins over `config.downloadUrlExpiresIn`. */ + expiresIn?: number +} + +/** + * A presigned GET URL for `key`. Expiry resolves per-call `expiresIn` -> + * `config.downloadUrlExpiresIn` -> 3 days. Throws UpupConfigError when + * `storage.type` has no S3-compatible API, matching createUpupHandler's + * construct-time guard. + */ +export async function getDownloadUrl( + config: DownloadUrlConfig, + key: string, + opts?: GetDownloadUrlOptions, +): Promise { + assertS3Storage(config.storage) + const expiresIn = + opts?.expiresIn ?? + config.downloadUrlExpiresIn ?? + DEFAULT_DOWNLOAD_URL_EXPIRES_IN + return generateSignedPublicUrl(config.storage, key, expiresIn) +} diff --git a/packages/server/src/drive-routes.ts b/packages/server/src/drive-routes.ts index c918b5f6b..fe673d344 100644 --- a/packages/server/src/drive-routes.ts +++ b/packages/server/src/drive-routes.ts @@ -159,6 +159,7 @@ export async function handleFileTransfer( maxBytes: config.maxFileSize, onError: config.onError, requestId: res.requestId, + downloadUrlExpiresIn: config.downloadUrlExpiresIn, }) // Post-commit: object durably in S3. A throwing onFileUploaded hook is // logged + swallowed by runPostCompletionHooks, never bubbling to the diff --git a/packages/server/src/handler.ts b/packages/server/src/handler.ts index a69a2a0fc..97b75569f 100644 --- a/packages/server/src/handler.ts +++ b/packages/server/src/handler.ts @@ -1,11 +1,8 @@ -import { - UpupErrorCode, - UpupConfigError, - NON_S3_STORAGE_PROVIDERS, -} from '@upupjs/core' +import { UpupErrorCode, UpupConfigError } from '@upupjs/core' import type { UpupServerConfig } from './config' import { assertUploadTokenSecret } from './uploadToken' import { validateServerConfig } from './validate-config' +import { assertS3Storage } from './storage' import { handleHealth } from './health' import { createResponder } from './respond' import { @@ -39,17 +36,7 @@ export function createUpupHandler(config: UpupServerConfig): RouteHandler { // credentials/region. A provider with no S3-compatible surface (currently // just Azure) could never function, with zero compile- or startup-time // signal until now. - const storageType = config.storage.type - if ( - typeof storageType === 'string' && - (NON_S3_STORAGE_PROVIDERS as ReadonlySet).has(storageType) - ) { - throw new UpupConfigError( - `[@upupjs/server] storage.type "${storageType}" has no S3-compatible API and cannot be served. ` + - 'upup uploads via the S3 API — use an S3-compatible provider ' + - '(aws, minio, r2, wasabi, …) and set storage.endpoint for non-AWS backends.', - ) - } + assertS3Storage(config.storage) if ( (config.providers || config.tokenStore) && !config.getUserId && diff --git a/packages/server/src/index.ts b/packages/server/src/index.ts index 8c4ae8de2..0dd2609d8 100644 --- a/packages/server/src/index.ts +++ b/packages/server/src/index.ts @@ -2,6 +2,7 @@ export { createUpupHandler } from './handler' export type { RouteHandler } from './handler' export type { UpupServerConfig, + UpupStorageConfig, TokenStore, DriveTokens, OAuthState, @@ -9,6 +10,8 @@ export type { UploadedFile, KeyStrategyContext, } from './config' +export { getDownloadUrl } from './download-url' +export type { DownloadUrlConfig, GetDownloadUrlOptions } from './download-url' export { InMemoryTokenStore, getTokens, diff --git a/packages/server/src/providers/aws.ts b/packages/server/src/providers/aws.ts index 534e67b0f..40a0e2bf8 100644 --- a/packages/server/src/providers/aws.ts +++ b/packages/server/src/providers/aws.ts @@ -23,7 +23,10 @@ import type { UpupServerConfig } from '../config' import { createS3Client } from './s3-client' const DEFAULT_EXPIRES_IN = 3600 -const DEFAULT_PUBLIC_URL_EXPIRES_IN = 3600 * 24 * 3 // 3 days +// Exported: the fallback for `config.downloadUrlExpiresIn` (#343). Every signed +// GET this package hands out resolves through here, so the 3-day policy has one +// definition rather than one per call site. +export const DEFAULT_DOWNLOAD_URL_EXPIRES_IN = 3600 * 24 * 3 // 3 days // Exported: this is the one canonical home for the 5 MiB S3 part-size floor — // also the fixed memory-safety cap `transfer.ts` uses for its singlePut/ // multipart routing decision (F-501, F-653). @@ -44,7 +47,7 @@ function computePartSize(fileSize: number, chunkSizeBytes?: number): number { export async function generateSignedPublicUrl( storage: UpupServerConfig['storage'], key: string, - expiresIn = DEFAULT_PUBLIC_URL_EXPIRES_IN, + expiresIn = DEFAULT_DOWNLOAD_URL_EXPIRES_IN, ): Promise { const client = createS3Client(storage) return getSignedUrl( @@ -60,6 +63,7 @@ export async function generatePresignedUrl( contentType: string, contentLength: number, expiresIn = DEFAULT_EXPIRES_IN, + downloadUrlExpiresIn?: number, ): Promise { const client = createS3Client(storage) @@ -78,7 +82,11 @@ export async function generatePresignedUrl( signableHeaders: new Set(['content-type', 'content-length']), }) - const downloadUrl = await generateSignedPublicUrl(storage, key) + const downloadUrl = await generateSignedPublicUrl( + storage, + key, + downloadUrlExpiresIn ?? DEFAULT_DOWNLOAD_URL_EXPIRES_IN, + ) return { key, @@ -148,6 +156,7 @@ export async function completeMultipartUpload( key: string, uploadId: string, parts: MultipartPart[], + downloadUrlExpiresIn?: number, ): Promise { const client = createS3Client(storage) @@ -163,7 +172,11 @@ export async function completeMultipartUpload( }) const result = await client.send(command) - const downloadUrl = await generateSignedPublicUrl(storage, key) + const downloadUrl = await generateSignedPublicUrl( + storage, + key, + downloadUrlExpiresIn ?? DEFAULT_DOWNLOAD_URL_EXPIRES_IN, + ) return { key, diff --git a/packages/server/src/storage.ts b/packages/server/src/storage.ts new file mode 100644 index 000000000..631fa14f0 --- /dev/null +++ b/packages/server/src/storage.ts @@ -0,0 +1,28 @@ +// packages/server/src/storage.ts +// +// The one home for "is this storage config something @upupjs/server can serve?". +// createUpupHandler asserts it at construct time and getDownloadUrl asserts it +// per call, so the check and its wording live here rather than being duplicated +// (and drifting) at each entry point. + +import { UpupConfigError, NON_S3_STORAGE_PROVIDERS } from '@upupjs/core' +import type { UpupStorageConfig } from './config' + +/** + * Reject a provider with no S3-compatible API (F-657). The S3 upload path + * (buildS3ClientConfig) always builds an @aws-sdk/client-s3 client, so such a + * provider could never function — fail loudly instead of 500ing at request time. + */ +export function assertS3Storage(storage: UpupStorageConfig): void { + const storageType = storage.type + if ( + typeof storageType === 'string' && + (NON_S3_STORAGE_PROVIDERS as ReadonlySet).has(storageType) + ) { + throw new UpupConfigError( + `[@upupjs/server] storage.type "${storageType}" has no S3-compatible API and cannot be served. ` + + 'upup uploads via the S3 API — use an S3-compatible provider ' + + '(aws, minio, r2, wasabi, …) and set storage.endpoint for non-AWS backends.', + ) + } +} diff --git a/packages/server/src/transfer.ts b/packages/server/src/transfer.ts index 3e548b5ec..119d34509 100644 --- a/packages/server/src/transfer.ts +++ b/packages/server/src/transfer.ts @@ -8,7 +8,11 @@ import { import { UpupStorageError, UpupErrorCode } from '@upupjs/core' import type { UpupServerConfig, UploadedFile } from './config' import { createS3Client } from './providers/s3-client' -import { MIN_PART_SIZE, generateSignedPublicUrl } from './providers/aws' +import { + MIN_PART_SIZE, + generateSignedPublicUrl, + DEFAULT_DOWNLOAD_URL_EXPIRES_IN, +} from './providers/aws' import { reportServerError, toSafeError, @@ -36,6 +40,8 @@ export async function transferDriveFileToS3(opts: { * swallowed (F-744). */ onError?: UpupServerLogger | undefined requestId?: string | undefined + /** TTL for the signed GET returned as `url`; defaults to 3 days (#343). */ + downloadUrlExpiresIn?: number | undefined }): Promise { const key = `${crypto.randomUUID()}-${opts.fileName}` @@ -53,6 +59,7 @@ async function singlePut(opts: { storage: UpupServerConfig['storage'] key: string maxBytes?: number | undefined + downloadUrlExpiresIn?: number | undefined }): Promise { const buffer = await streamToUint8Array(opts.stream) // Enforce maxFileSize against the bytes we actually received before writing @@ -74,7 +81,11 @@ async function singlePut(opts: { Body: buffer, }), ) - const url = await generateSignedPublicUrl(opts.storage, opts.key) + const url = await generateSignedPublicUrl( + opts.storage, + opts.key, + opts.downloadUrlExpiresIn ?? DEFAULT_DOWNLOAD_URL_EXPIRES_IN, + ) return { key: opts.key, name: opts.fileName, @@ -94,6 +105,7 @@ async function streamingMultipart(opts: { maxBytes?: number | undefined onError?: UpupServerLogger | undefined requestId?: string | undefined + downloadUrlExpiresIn?: number | undefined }): Promise { const client = createS3Client(opts.storage) @@ -195,7 +207,11 @@ async function streamingMultipart(opts: { throw err } - const url = await generateSignedPublicUrl(opts.storage, opts.key) + const url = await generateSignedPublicUrl( + opts.storage, + opts.key, + opts.downloadUrlExpiresIn ?? DEFAULT_DOWNLOAD_URL_EXPIRES_IN, + ) return { key: opts.key, name: opts.fileName, diff --git a/packages/server/src/upload-routes.ts b/packages/server/src/upload-routes.ts index d6b8a2acc..cc91f5f62 100644 --- a/packages/server/src/upload-routes.ts +++ b/packages/server/src/upload-routes.ts @@ -251,6 +251,8 @@ export async function handlePresign( key, body.type, body.size, + undefined, + config.downloadUrlExpiresIn, ) return res.json(result, 200) } catch (error) { @@ -437,6 +439,7 @@ export async function handleMultipartComplete( payload.k, payload.u, body.parts, + config.downloadUrlExpiresIn, ) const uploaded: UploadedFile = { diff --git a/packages/server/tests/download-url.test.ts b/packages/server/tests/download-url.test.ts new file mode 100644 index 000000000..7363b29c7 --- /dev/null +++ b/packages/server/tests/download-url.test.ts @@ -0,0 +1,107 @@ +// Issue #343: a first-class primitive for signing a GET against an EXISTING +// key, plus a config knob for the download-URL TTL that was hardcoded to 3 days. +// +// Mocks at the provider boundary (the presigner + the S3 client factory) the +// same way transfer.test.ts does, so the real providers/aws.ts signing path +// runs and the expiry actually threads through it. +import { describe, it, expect, vi, beforeEach } from 'vitest' +import { UpupConfigError } from '@upupjs/core' + +const signed: Array<{ + expiresIn: number | undefined + command: string + bucket: unknown + key: unknown +}> = [] + +vi.mock('@aws-sdk/s3-request-presigner', () => ({ + getSignedUrl: vi.fn( + async ( + _client: unknown, + cmd: { + constructor: { name: string } + input: Record + }, + opts?: { expiresIn?: number }, + ) => { + signed.push({ + expiresIn: opts?.expiresIn, + command: cmd.constructor.name, + bucket: cmd.input.Bucket, + key: cmd.input.Key, + }) + return `https://signed.example/${String(cmd.input.Key)}` + }, + ), +})) + +vi.mock('../src/providers/s3-client', () => ({ + createS3Client: () => ({ send: vi.fn(async () => ({})) }), + buildS3ClientConfig: () => ({ region: 'us-east-1' }), +})) + +import { getDownloadUrl } from '../src/download-url' +import { generatePresignedUrl } from '../src/providers/aws' + +const THREE_DAYS = 3600 * 24 * 3 + +const storage = { + type: 'aws', + bucket: 'gated-files', + region: 'us-east-1', + accessKeyId: 'AK', + secretAccessKey: 'SK', +} + +beforeEach(() => { + signed.length = 0 +}) + +describe('getDownloadUrl (#343)', () => { + it('signs a GET for an existing key against the configured bucket', async () => { + const url = await getDownloadUrl({ storage }, 'user-1/old/report.pdf') + expect(url).toBe('https://signed.example/user-1/old/report.pdf') + expect(signed).toHaveLength(1) + expect(signed[0]?.command).toBe('GetObjectCommand') + expect(signed[0]?.bucket).toBe('gated-files') + expect(signed[0]?.key).toBe('user-1/old/report.pdf') + }) + + it('defaults the expiry to three days when nothing overrides it', async () => { + await getDownloadUrl({ storage }, 'k') + expect(signed[0]?.expiresIn).toBe(THREE_DAYS) + }) + + it('honours config.downloadUrlExpiresIn over the default', async () => { + await getDownloadUrl({ storage, downloadUrlExpiresIn: 900 }, 'k') + expect(signed[0]?.expiresIn).toBe(900) + }) + + it('lets a per-call expiresIn win over the config knob', async () => { + await getDownloadUrl({ storage, downloadUrlExpiresIn: 900 }, 'k', { + expiresIn: 60, + }) + expect(signed[0]?.expiresIn).toBe(60) + }) + + it('rejects a storage provider with no S3 surface', async () => { + await expect( + getDownloadUrl({ storage: { ...storage, type: 'azure' } }, 'k'), + ).rejects.toBeInstanceOf(UpupConfigError) + expect(signed).toHaveLength(0) + }) +}) + +describe('downloadUrlExpiresIn threads into the presign response (#343)', () => { + it('signs the returned downloadUrl with the configured TTL', async () => { + await generatePresignedUrl(storage, 'k.png', 'image/png', 10, 3600, 900) + const get = signed.find(s => s.command === 'GetObjectCommand') + expect(get?.expiresIn).toBe(900) + }) + + it('still signs the downloadUrl for three days when unset', async () => { + await generatePresignedUrl(storage, 'k.png', 'image/png', 10) + const get = signed.find(s => s.command === 'GetObjectCommand') + expect(get?.expiresIn).toBe(THREE_DAYS) + }) +}) diff --git a/packages/server/tests/public-api.test.ts b/packages/server/tests/public-api.test.ts index 003903f17..14b7d9b76 100644 --- a/packages/server/tests/public-api.test.ts +++ b/packages/server/tests/public-api.test.ts @@ -9,6 +9,7 @@ const EXPECTED_PUBLIC_VALUE_EXPORTS = [ 'InMemoryTokenStore', 'createUpupHandler', 'deleteTokens', + 'getDownloadUrl', 'getTokens', 'setTokens', ].sort() diff --git a/scripts/docs/api-docs-map.json b/scripts/docs/api-docs-map.json index 7d560b0ae..d193dfac2 100644 --- a/scripts/docs/api-docs-map.json +++ b/scripts/docs/api-docs-map.json @@ -89,7 +89,8 @@ }, "@upupjs/server": { "InMemoryTokenStore": "apps/landing/content/docs/guides/server-auth.mdx", - "createUpupHandler": "apps/landing/content/docs/api-reference/server-http.mdx" + "createUpupHandler": "apps/landing/content/docs/api-reference/server-http.mdx", + "getDownloadUrl": "apps/landing/content/docs/api-reference/server-http.mdx" }, "@upupjs/next/server": { "InMemoryTokenStore": "apps/landing/content/docs/guides/server-auth.mdx", From d177cf1954cf8132067b94d014314f243bdeef70 Mon Sep 17 00:00:00 2001 From: AminDhouib Date: Wed, 12 Aug 2026 10:21:00 -0400 Subject: [PATCH 10/14] feat(server): add hooks.onPresignResponse; surface UpupError from onBeforeUpload (#338) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A deployment whose storage endpoint is not browser-reachable — a private MinIO behind a same-origin proxy route, a docker-internal hostname in local dev, a VPC-only endpoint — could not use the shipped handlers at all: `hooks` had no response-side seam, so there was no way to rewrite `uploadUrl` before it left the server. One hook, `onPresignResponse`, closes that. It fires on the three presign-side responses, discriminated by `ctx.phase`: presign POST /presign PresignedUrlResponse multipart-init POST /multipart/init MultipartInitResponse + token multipart-sign-part POST /multipart/sign-part MultipartSignPartResponse One hook name rather than three, because the rewrite is the same operation each time and covering only /presign would leave multipart uploads pointed at the unreachable host. Returning an object replaces the payload; returning nothing keeps it. ctx is { req, phase, key, metadata?, userId } — `key` is always the key that is IN the payload (on sign-part it comes from the VERIFIED token, never the client), and `metadata` is absent on sign-part, which sees only a token and a part number. Trust model unchanged. The hook runs after every auth, policy, and token check and after the upload token is issued; it cannot alter a status code or turn a rejection into a success, and a request that would 401/403 never reaches it. Two tests pin that directly ("never runs for a request rejected before the route", "never runs for a request the auth gate rejected"). Responses still go out through the Responder, so response-contract.test.ts is untouched. Also: `onBeforeUpload` throwing an UpupError now serializes that error's message and code into the 403, so a quota check can say "Storage limit exceeded — upgrade to keep uploading" instead of the opaque "Upload rejected". Returning `false` keeps the generic body byte-for-byte. Any NON-UpupError throw is re-thrown and stays a generic 500 with the cause going only to onError — pinned by a test asserting the thrown message does not appear in the response body. New exported types (types only — the runtime public-API pin is unchanged): PresignResponsePhase, PresignResponseContext, PresignResponseBody, PresignResponseRewrite. RED before the implementation (6 of 11 cases in packages/server/tests/presign-response-hook.test.ts): × replaces the /presign payload when the hook returns an object expected 'https://internal-minio:9000/bucket/u1…' to be 'https://app.example.com/api/s3/bucket…' × replaces the /multipart/init payload, token included expected undefined to be 'eu-west-1' × replaces the /multipart/sign-part payload × reports phase, key, metadata and userId per response × surfaces an UpupError thrown by the hook with its message and code × surfaces it on /multipart/init too The two UpupError cases were 500s before, with the hook's message reaching only the logs: [upup:server] {"route":"presign",…,"status":500,"code":"STORAGE_ERROR", "message":"Internal error","error":{"name":"UpupQuotaError","message": "Storage limit exceeded — upgrade to keep uploading",…}} Gates: server unit suite 287 passed | 35 skipped (handler-extended and trust-model unmodified), typecheck (src + test tree) exit 0, docs:links:check OK. --- .../docs/api-reference/server-http.mdx | 23 ++ .../content/docs/guides/server-mode-setup.mdx | 114 ++++++- packages/server/src/config.ts | 69 +++- packages/server/src/index.ts | 4 + packages/server/src/upload-routes.ts | 72 ++++- .../tests/presign-response-hook.test.ts | 295 ++++++++++++++++++ 6 files changed, 562 insertions(+), 15 deletions(-) create mode 100644 packages/server/tests/presign-response-hook.test.ts diff --git a/apps/landing/content/docs/api-reference/server-http.mdx b/apps/landing/content/docs/api-reference/server-http.mdx index 7dd018b88..a820446dc 100644 --- a/apps/landing/content/docs/api-reference/server-http.mdx +++ b/apps/landing/content/docs/api-reference/server-http.mdx @@ -179,6 +179,7 @@ three days unless `downloadUrlExpiresIn` says otherwise. | `401` | `{ "error": "Unauthenticated" }` | `getUserId` returned `null` | | `403` | `{ "error": "…", "code": "AUTH_REQUIRED" }` | no auth path configured | | `403` | `{ "error": "Upload rejected" }` | your `hooks.onBeforeUpload` returned `false` | +| `403` | `{ "error": "…", "code": "…" }` | your `hooks.onBeforeUpload` threw an `UpupError` — its own message and code | | `413` | `{ "error": "File too large" }` | `size` exceeds `maxFileSize` | | `415` | `{ "error": "File type not allowed" }` | `type` does not match `allowedTypes` | | `500` | `{ "error": "Presign failed", "code": "PRESIGN_FAILED" }` | the storage call threw | @@ -558,6 +559,28 @@ returns — `downloadUrl` on `/presign` and `/multipart/complete`, `url` on `/files/:provider/transfer`. Set `downloadUrlExpiresIn` (seconds) to change it. This is independent of the one-hour upload-URL expiry. +## Rewriting a response body + +`hooks.onPresignResponse` is the one seam that can change what a route sends. +It fires on three responses only, discriminated by `ctx.phase`: + +| `ctx.phase` | Route | Body it receives | +| --------------------- | --------------------------- | ----------------------------------------- | +| `presign` | `POST /presign` | `PresignedUrlResponse` | +| `multipart-init` | `POST /multipart/init` | `MultipartInitResponse` plus `token` | +| `multipart-sign-part` | `POST /multipart/sign-part` | `MultipartSignPartResponse` | + +Returning an object replaces the payload; returning nothing keeps it. The +context is `{ req, phase, key, metadata?, userId }` — `key` is always the key +in the payload, and `metadata` is absent on `multipart-sign-part`, which sees +only a verified token. + +The hook runs after auth, policy, and token verification, and after the upload +token is issued. It cannot change a status code, cannot turn a rejection into a +success, and never runs on a request that answered `401`/`403`. It is a body +rewriter for deployments whose storage endpoint the browser cannot reach — see +[Rewriting presign responses](/docs/guides/server-mode-setup/#rewriting-presign-responses). + ## Signing a download URL outside the handler `getDownloadUrl` signs a `GET` for a key that already exists, without going diff --git a/apps/landing/content/docs/guides/server-mode-setup.mdx b/apps/landing/content/docs/guides/server-mode-setup.mdx index da3597e4e..c27dec6a6 100644 --- a/apps/landing/content/docs/guides/server-mode-setup.mdx +++ b/apps/landing/content/docs/guides/server-mode-setup.mdx @@ -322,13 +322,17 @@ rename. ## Lifecycle hooks -Three optional hooks let you gate uploads and react to completions: +Four optional hooks let you gate uploads, rewrite what the server hands back, +and react to completions: ```ts createUpupHandler({ // ...storage, uploadTokenSecret hooks: { onBeforeUpload: async (file, req) => true, // false rejects with 403 + onPresignResponse: (response, ctx) => { + // return an object to replace the payload; nothing to keep it + }, onFileUploaded: async (file, req) => { // one file finished — file.key, .name, .size, .type, .url }, @@ -342,17 +346,23 @@ createUpupHandler({ **Which hook fires on which path.** Read this before wiring alerting, billing, or webhooks on top of them — the gaps are structural, not bugs: -| Route | `onBeforeUpload` | `onFileUploaded` | `onUploadComplete` | -| -------------------------------- | ---------------- | ---------------- | ------------------ | -| `POST /presign` | yes | no | no | -| `POST /multipart/init` | yes | no | no | -| `POST /multipart/complete` | no | yes | yes | -| `POST /files/:provider/transfer` | no | yes | no | +| Route | `onBeforeUpload` | `onPresignResponse` | `onFileUploaded` | `onUploadComplete` | +| -------------------------------- | ---------------- | ------------------- | ---------------- | ------------------ | +| `POST /presign` | yes | yes | no | no | +| `POST /multipart/init` | yes | yes | no | no | +| `POST /multipart/sign-part` | no | yes | no | no | +| `POST /multipart/complete` | no | no | yes | yes | +| `POST /files/:provider/transfer` | no | no | yes | no | - **`onBeforeUpload` is an admission gate, not a completion signal.** It runs during metadata validation on `/presign` and `/multipart/init`, after the `maxFileSize` and `allowedTypes` checks. Returning `false` responds - `403 Upload rejected` and nothing is presigned. + `403 Upload rejected` and nothing is presigned. To explain the rejection, + **throw an `UpupError`** instead — see + [Explaining a rejection](#explaining-a-rejection) below. +- **`onPresignResponse` is the only hook that can change a response body.** It + sees the three presign-side payloads and nothing else — see + [Rewriting presign responses](#rewriting-presign-responses). - **`onFileUploaded` fires once per file on the two server-side-completion paths only:** `/multipart/complete` (the server just finished the S3 multipart upload) and `/files/:provider/transfer` (the server just finished @@ -376,6 +386,94 @@ A hook that throws **after** a successful upload is reported through durably in S3, so a 500 would only tell the client to retry something that already succeeded. +### Rewriting presign responses + +If your bucket is not reachable from the browser — a private MinIO behind a +same-origin proxy route, a docker-internal hostname in local dev, a VPC-only +endpoint — the signed URL the server produces is not the URL the browser can +use. `onPresignResponse` gets the last look at the payload and can replace it: + +```ts +createUpupHandler({ + // ...storage, uploadTokenSecret + hooks: { + onPresignResponse: (response, ctx) => { + if (!('uploadUrl' in response)) return + return { + ...response, + uploadUrl: response.uploadUrl.replace( + 'https://minio.internal:9000', + 'https://app.example.com/api/s3', + ), + } + }, + }, +}) +``` + +Return an object to replace the payload; return nothing to leave it alone. + +It fires on exactly three responses, told apart by `ctx.phase`: + +| `ctx.phase` | Route | Payload | +| ---------------------- | --------------------------- | ------------------------------------------- | +| `presign` | `POST /presign` | `PresignedUrlResponse` | +| `multipart-init` | `POST /multipart/init` | `MultipartInitResponse` plus the `token` | +| `multipart-sign-part` | `POST /multipart/sign-part` | `MultipartSignPartResponse` | + +`ctx` also carries `req`, the server-chosen `key` (always the key in the +payload), the resolved `userId`, and `metadata` — the client-declared file +metadata, absent on `multipart-sign-part`, which sees only a verified token and +a part number. + + + The hook runs **after** every auth, policy, and token check, so it cannot + widen what a caller was allowed to do — a request that would answer 401 or + 403 never reaches it. But rewriting `uploadUrl` does change where the + browser sends bytes: the URL you substitute must land at the same object, + and whatever proxy it points at is now part of your upload path. + + +Cover all three phases if you use multipart. Rewriting only `/presign` leaves +multipart uploads pointed at the unreachable host. + +### Explaining a rejection + +`onBeforeUpload` returning `false` answers a deliberately generic +`403 { "error": "Upload rejected" }` — it says nothing about why. When the +reason is something the user should see, throw an `UpupError` instead and its +message and code are serialized into the 403 body: + +```ts +import { UpupQuotaError } from '@upupjs/core' + +hooks: { + onBeforeUpload: async (file, req) => { + const { used, limit } = await getQuota(req) + if (used + file.size > limit) { + throw new UpupQuotaError( + 'Storage limit exceeded — upgrade to keep uploading', + limit, + used, + ) + } + return true + } +} +``` + +```json +{ + "error": "Storage limit exceeded — upgrade to keep uploading", + "code": "QUOTA_EXCEEDED" +} +``` + +Only `UpupError` and its subclasses are serialized, and only from this hook. +Any other throw stays a generic `500` with the real cause going to `onError` +alone — an accidental `Error` carrying a connection string or a stack trace +never reaches the client. + ## Observability ### The `onError` seam diff --git a/packages/server/src/config.ts b/packages/server/src/config.ts index 195992245..e2469c561 100644 --- a/packages/server/src/config.ts +++ b/packages/server/src/config.ts @@ -1,4 +1,10 @@ -import type { StorageProvider, UpupCorsConfig } from '@upupjs/core' +import type { + StorageProvider, + UpupCorsConfig, + PresignedUrlResponse, + MultipartInitResponse, + MultipartSignPartResponse, +} from '@upupjs/core' import type { UpupServerLogger } from './observability' /** Context passed to a custom keyStrategy. */ @@ -10,6 +16,35 @@ export interface KeyStrategyContext { size: number } +/** Which of the three presign-side responses `onPresignResponse` is rewriting. */ +export type PresignResponsePhase = + 'presign' | 'multipart-init' | 'multipart-sign-part' + +export interface PresignResponseContext { + /** The originating request, already past every auth and policy check. */ + req: Request + phase: PresignResponsePhase + /** The server-chosen object key. Present on all three phases — on + * `multipart-sign-part` it comes from the VERIFIED token, not the client. */ + key: string + /** The client-declared file metadata. Absent on `multipart-sign-part`, + * which sees only a token and a part number. */ + metadata?: FileMetadata + /** Resolved userId, or null for an anonymous (server-namespaced) upload. */ + userId: string | null +} + +/** What the hook receives — narrow it on `ctx.phase`, or with `'uploadUrl' in response`. */ +export type PresignResponseBody = + | PresignedUrlResponse + | (MultipartInitResponse & { token: string }) + | MultipartSignPartResponse + +/** What the hook may return: the same shapes, plus any extra fields you want + * to add for your client. */ +export type PresignResponseRewrite = PresignResponseBody & + Record + /** One bucket's worth of S3 / S3-compatible connection settings. */ export interface UpupStorageConfig { /** @@ -98,12 +133,44 @@ export type UpupServerConfig = { * README's "Lifecycle hooks" section for the full per-path breakdown. */ hooks?: { + /** + * Admission gate. Return `false` to reject with a generic + * `403 Upload rejected`; THROW an `UpupError` to reject with that + * error's own message and code in the 403 body (a quota check can say + * "Storage limit exceeded — upgrade to keep uploading"). Any other + * throw stays a generic 500 — internal error text never reaches the + * client. + */ onBeforeUpload?: (file: FileMetadata, req: Request) => Promise onFileUploaded?: (file: UploadedFile, req: Request) => Promise onUploadComplete?: ( files: UploadedFile[], req: Request, ) => Promise + /** + * Last look at a presign-side response body before it is sent, for + * deployments where the storage endpoint is not browser-reachable + * (a same-origin proxy route, a docker-internal MinIO hostname, a + * VPC-only endpoint). Return an object to REPLACE the payload; return + * nothing to leave it as-is. + * + * Fires on exactly three responses, identified by `ctx.phase`: + * `POST /presign` (`presign`), `POST /multipart/init` + * (`multipart-init`, token already issued), and + * `POST /multipart/sign-part` (`multipart-sign-part`). + * + * It runs AFTER every auth, policy, and token check and cannot bypass + * any of them — a request that would 401/403 never reaches the hook. + * Rewriting `uploadUrl` changes where the browser sends bytes, so the + * URL you substitute must land at the same object. + */ + onPresignResponse?: ( + response: PresignResponseBody, + ctx: PresignResponseContext, + ) => + | PresignResponseRewrite + | void + | Promise } auth?: (req: Request) => Promise diff --git a/packages/server/src/index.ts b/packages/server/src/index.ts index 0dd2609d8..c4e20c7cf 100644 --- a/packages/server/src/index.ts +++ b/packages/server/src/index.ts @@ -9,6 +9,10 @@ export type { FileMetadata, UploadedFile, KeyStrategyContext, + PresignResponsePhase, + PresignResponseContext, + PresignResponseBody, + PresignResponseRewrite, } from './config' export { getDownloadUrl } from './download-url' export type { DownloadUrlConfig, GetDownloadUrlOptions } from './download-url' diff --git a/packages/server/src/upload-routes.ts b/packages/server/src/upload-routes.ts index cc91f5f62..5e9b81b4e 100644 --- a/packages/server/src/upload-routes.ts +++ b/packages/server/src/upload-routes.ts @@ -7,8 +7,14 @@ // can read the trust core without wading through OAuth or drive-provider code. // The HMAC/token/envelope logic is UNCHANGED — this is a move, not a rewrite. -import { UpupErrorCode } from '@upupjs/core' -import type { UpupServerConfig, FileMetadata, UploadedFile } from './config' +import { UpupErrorCode, UpupError } from '@upupjs/core' +import type { + UpupServerConfig, + FileMetadata, + UploadedFile, + PresignResponseBody, + PresignResponseContext, +} from './config' import { generatePresignedUrl, initiateMultipartUpload, @@ -175,7 +181,20 @@ async function validateUploadMetadata( } if (config.hooks?.onBeforeUpload) { - const allowed = await config.hooks.onBeforeUpload(body, req) + let allowed: boolean + try { + allowed = await config.hooks.onBeforeUpload(body, req) + } catch (error) { + // upup-catch: an UpupError is the integrator DELIBERATELY speaking to + // the client — serialize its message + code into the 403 so a quota + // check can explain itself (#338). Anything else is an unexpected + // failure and is re-thrown, so it surfaces as a generic 500 with the + // real cause going only to onError. Never leak internal error text. + if (error instanceof UpupError) { + return res.json({ error: error.message, code: error.code }, 403) + } + throw error + } if (!allowed) { return res.json({ error: 'Upload rejected' }, 403) } @@ -184,6 +203,20 @@ async function validateUploadMetadata( return null } +/** Give `hooks.onPresignResponse` the last look at a presign-side payload + * (#338). Runs after every auth/policy/token check, so it can rewrite where + * the browser sends bytes but can never widen what the caller was allowed to + * do. Returning nothing keeps the payload untouched. */ +async function applyPresignResponseHook( + config: UpupServerConfig, + response: PresignResponseBody, + ctx: PresignResponseContext, +): Promise { + const hook = config.hooks?.onPresignResponse + if (!hook) return response + return (await hook(response, ctx)) ?? response +} + /** Run integrator post-completion hooks (onFileUploaded/onUploadComplete) AFTER * the object is durably written, in their OWN try so a throwing hook is logged * via onError and swallowed — never re-coded as a 500 that would tell the @@ -254,7 +287,15 @@ export async function handlePresign( undefined, config.downloadUrlExpiresIn, ) - return res.json(result, 200) + const payload = await applyPresignResponseHook(config, result, { + req, + phase: 'presign', + // Always the key that is IN the payload, on every phase. + key: result.key, + metadata: body, + userId: owner, + }) + return res.json(payload, 200) } catch (error) { return res.fail( 'presign', @@ -328,7 +369,18 @@ export async function handleMultipartInit( Math.floor(Date.now() / 1000) + DEFAULT_UPLOAD_TOKEN_TTL_SECONDS, }) - return res.json({ ...result, token }, 200) + const payload = await applyPresignResponseHook( + config, + { ...result, token }, + { + req, + phase: 'multipart-init', + key: result.key, + metadata: body, + userId: owner, + }, + ) + return res.json(payload, 200) } catch (error) { return res.fail( 'multipart/init', @@ -373,7 +425,15 @@ export async function handleMultipartSignPart( payload.u, body.partNumber, ) - return res.json(result, 200) + const rewritten = await applyPresignResponseHook(config, result, { + req, + phase: 'multipart-sign-part', + // From the VERIFIED token — sign-part never sees a client-asserted + // key, and has no file metadata to report. + key: payload.k, + userId: payload.uid, + }) + return res.json(rewritten, 200) } catch (error) { return res.fail( 'multipart/sign-part', diff --git a/packages/server/tests/presign-response-hook.test.ts b/packages/server/tests/presign-response-hook.test.ts new file mode 100644 index 000000000..db9340086 --- /dev/null +++ b/packages/server/tests/presign-response-hook.test.ts @@ -0,0 +1,295 @@ +// Issue #338: a hook to rewrite the presign-side responses, for deployments +// where the storage endpoint is not browser-reachable (same-origin proxy route, +// docker-internal MinIO hostname, VPC-only endpoints), plus surfacing an +// UpupError thrown by onBeforeUpload instead of the generic "Upload rejected". +// +// Drives the REAL handler with only the AWS provider mocked, so the hook runs +// where it actually runs: after validation and token issuance, immediately +// before the route responds. +import { describe, it, expect, vi, beforeEach } from 'vitest' +import { UpupQuotaError, UpupError, UpupErrorCode } from '@upupjs/core' + +vi.mock('../src/providers/aws', () => ({ + generatePresignedUrl: vi.fn().mockResolvedValue({ + key: 'u1/uuid/photo.png', + uploadUrl: 'https://internal-minio:9000/bucket/u1/uuid/photo.png?sig', + downloadUrl: 'https://internal-minio:9000/bucket/u1/uuid/photo.png?get', + uploadHeaders: { 'Content-Type': 'image/png' }, + expiresIn: 3600, + }), + initiateMultipartUpload: vi.fn().mockResolvedValue({ + key: 'u1/uuid/big.zip', + uploadId: 'mp-1', + partSize: 5 * 1024 * 1024, + expiresIn: 3600, + }), + generatePresignedPartUrl: vi.fn().mockResolvedValue({ + uploadUrl: 'https://internal-minio:9000/bucket/part?sig', + expiresIn: 3600, + }), + completeMultipartUpload: vi.fn().mockResolvedValue({ key: 'k' }), + abortMultipartUpload: vi.fn().mockResolvedValue({ ok: true }), + getMultipartUploadedSize: vi.fn().mockResolvedValue(0), + checkStorageReachable: vi.fn().mockResolvedValue({ ok: true }), + generateSignedPublicUrl: vi.fn().mockResolvedValue('https://signed'), + DEFAULT_DOWNLOAD_URL_EXPIRES_IN: 3600 * 24 * 3, + MIN_PART_SIZE: 5 * 1024 * 1024, +})) + +import { createUpupHandler } from '../src/handler' +import type { UpupServerConfig } from '../src/config' + +const base: UpupServerConfig = { + storage: { type: 'minio', bucket: 'b', region: 'us-east-1' }, + uploadTokenSecret: 'a-stable-secret-at-least-16', + allowAnonymousUploads: true, +} + +function post(path: string, body: unknown): Request { + return new Request(`https://app.example.com/api/upup${path}`, { + method: 'POST', + headers: { 'content-type': 'application/json' }, + body: JSON.stringify(body), + }) +} + +const meta = { name: 'photo.png', type: 'image/png', size: 1024 } + +/** Swap the docker-internal host for the browser-reachable proxy route. */ +const proxied = (url: string) => + url.replace('https://internal-minio:9000', 'https://app.example.com/api/s3') + +beforeEach(() => { + vi.clearAllMocks() +}) + +describe('hooks.onPresignResponse (#338)', () => { + it('replaces the /presign payload when the hook returns an object', async () => { + const handler = createUpupHandler({ + ...base, + hooks: { + onPresignResponse: res => + 'uploadUrl' in res + ? { ...res, uploadUrl: proxied(res.uploadUrl) } + : undefined, + }, + }) + const body = (await ( + await handler(post('/presign', meta)) + ).json()) as Record + expect(body.uploadUrl).toBe( + 'https://app.example.com/api/s3/bucket/u1/uuid/photo.png?sig', + ) + // Untouched fields survive the rewrite. + expect(body.key).toBe('u1/uuid/photo.png') + expect(body.expiresIn).toBe(3600) + }) + + it('replaces the /multipart/init payload, token included', async () => { + const handler = createUpupHandler({ + ...base, + hooks: { + onPresignResponse: res => ({ ...res, region: 'eu-west-1' }), + }, + }) + const body = (await ( + await handler(post('/multipart/init', meta)) + ).json()) as Record + expect(body.region).toBe('eu-west-1') + expect(body.uploadId).toBe('mp-1') + expect(typeof body.token).toBe('string') + }) + + it('replaces the /multipart/sign-part payload', async () => { + const issue = createUpupHandler(base) + const init = (await ( + await issue(post('/multipart/init', meta)) + ).json()) as { token: string } + + const handler = createUpupHandler({ + ...base, + hooks: { + onPresignResponse: res => + 'uploadUrl' in res + ? { ...res, uploadUrl: proxied(res.uploadUrl) } + : undefined, + }, + }) + const body = (await ( + await handler( + post('/multipart/sign-part', { + token: init.token, + partNumber: 1, + }), + ) + ).json()) as Record + expect(body.uploadUrl).toBe( + 'https://app.example.com/api/s3/bucket/part?sig', + ) + }) + + it('leaves the payload untouched when the hook returns void', async () => { + const handler = createUpupHandler({ + ...base, + hooks: { + onPresignResponse: () => { + /* inspect only */ + }, + }, + }) + const body = (await ( + await handler(post('/presign', meta)) + ).json()) as Record + expect(body.uploadUrl).toBe( + 'https://internal-minio:9000/bucket/u1/uuid/photo.png?sig', + ) + }) + + it('reports phase, key, metadata and userId per response', async () => { + const seen: Array> = [] + const config: UpupServerConfig = { + ...base, + allowAnonymousUploads: false, + getUserId: async () => 'user-7', + hooks: { + onPresignResponse: (_res, ctx) => { + seen.push({ + phase: ctx.phase, + key: ctx.key, + metadata: ctx.metadata, + userId: ctx.userId, + isRequest: ctx.req instanceof Request, + }) + }, + }, + } + const handler = createUpupHandler(config) + await handler(post('/presign', meta)) + const init = (await ( + await handler(post('/multipart/init', meta)) + ).json()) as { token: string } + await handler( + post('/multipart/sign-part', { token: init.token, partNumber: 2 }), + ) + + expect(seen).toEqual([ + { + phase: 'presign', + key: 'u1/uuid/photo.png', + metadata: meta, + userId: 'user-7', + isRequest: true, + }, + { + phase: 'multipart-init', + key: 'u1/uuid/big.zip', + metadata: meta, + userId: 'user-7', + isRequest: true, + }, + { + // sign-part sees only a verified token, never file metadata. + phase: 'multipart-sign-part', + key: 'u1/uuid/big.zip', + metadata: undefined, + userId: 'user-7', + isRequest: true, + }, + ]) + }) + + it('never runs for a request rejected before the route (403 AUTH_REQUIRED)', async () => { + const onPresignResponse = vi.fn() + const handler = createUpupHandler({ + storage: base.storage, + uploadTokenSecret: 'a-stable-secret-at-least-16', + hooks: { onPresignResponse }, + }) + const res = await handler(post('/presign', meta)) + expect(res.status).toBe(403) + const body = (await res.json()) as { code: string } + expect(body.code).toBe(UpupErrorCode.AUTH_REQUIRED) + expect(onPresignResponse).not.toHaveBeenCalled() + }) + + it('never runs for a request the auth gate rejected (401)', async () => { + const onPresignResponse = vi.fn() + const onBeforeUpload = vi.fn().mockResolvedValue(true) + const handler = createUpupHandler({ + ...base, + auth: async () => false, + hooks: { onPresignResponse, onBeforeUpload }, + }) + const res = await handler(post('/presign', meta)) + expect(res.status).toBe(401) + expect(onBeforeUpload).not.toHaveBeenCalled() + expect(onPresignResponse).not.toHaveBeenCalled() + }) +}) + +describe('onBeforeUpload rejection messages (#338)', () => { + it('surfaces an UpupError thrown by the hook with its message and code', async () => { + const handler = createUpupHandler({ + ...base, + hooks: { + onBeforeUpload: async () => { + throw new UpupQuotaError( + 'Storage limit exceeded — upgrade to keep uploading', + 100, + 120, + ) + }, + }, + }) + const res = await handler(post('/presign', meta)) + expect(res.status).toBe(403) + expect(await res.json()).toEqual({ + error: 'Storage limit exceeded — upgrade to keep uploading', + code: UpupErrorCode.QUOTA_EXCEEDED, + }) + }) + + it('surfaces it on /multipart/init too', async () => { + const handler = createUpupHandler({ + ...base, + hooks: { + onBeforeUpload: async () => { + throw new UpupError('Docs are read-only today', 'READ_ONLY') + }, + }, + }) + const res = await handler(post('/multipart/init', meta)) + expect(res.status).toBe(403) + expect(await res.json()).toEqual({ + error: 'Docs are read-only today', + code: 'READ_ONLY', + }) + }) + + it('keeps the generic rejection when the hook returns false', async () => { + const handler = createUpupHandler({ + ...base, + hooks: { onBeforeUpload: async () => false }, + }) + const res = await handler(post('/presign', meta)) + expect(res.status).toBe(403) + expect(await res.json()).toEqual({ error: 'Upload rejected' }) + }) + + it('does not leak a non-UpupError throw into the response body', async () => { + const handler = createUpupHandler({ + ...base, + onError: () => {}, + hooks: { + onBeforeUpload: async () => { + throw new Error('db password is hunter2') + }, + }, + }) + const res = await handler(post('/presign', meta)) + expect(res.status).toBe(500) + const body = (await res.json()) as { error: string } + expect(body.error).not.toContain('hunter2') + expect(body.error).toBe('Internal error') + }) +}) From 7bf5e8d409c91c4a60d0cd3d2de0a1a53bf637bc Mon Sep 17 00:00:00 2001 From: AminDhouib Date: Wed, 12 Aug 2026 10:33:30 -0400 Subject: [PATCH 11/14] feat(server): resolve storage per request; keyStrategy sees req + metadata (#337) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `storage` was a single static object, so `createUpupHandler` only fit single-bucket apps — an app routing images / quarantined-but-unscanned documents / general documents to three buckets with their own credentials and endpoints had to keep a hand-rolled presign route. It now also accepts a resolver: storage: ctx => ctx.storageId ? byIdentity(ctx.storageId) : BUCKETS[classify(ctx.metadata)] ctx is { req, phase, userId, metadata?, fileName?, contentType?, size?, storageId? }, and every route resolves through it: presign, multipart init, the three multipart continuations, and drive-transfer (which resolves AFTER the drive reports the real name/type/size, not from the client's claim). The token carrier (the hard part of this change) ------------------------------------------------ A multipart upload picks its bucket at init, but sign-part/complete/abort arrive later with only a token — none of the metadata the decision came from. Re-running the resolver blind would send them elsewhere; accepting a client-supplied hint would let anyone redirect a continuation into a bucket of their choosing. So init stamps an opaque STORAGE IDENTITY into the HMAC-signed upload token (new optional `sid`), and each continuation hands it back to the resolver as `ctx.storageId`. The server then re-derives the identity of whatever the resolver returned and answers 403 AUTH_DENIED on a mismatch — a resolver that ignores storageId fails closed instead of writing parts into the wrong bucket. No unsigned storage hint is ever accepted. The identity is SHA-256(bucket, endpoint, region), truncated — deterministic (so a token issued by one worker verifies on any other) and free of credentials, so rotating an access key does not strand in-flight uploads and nothing secret sits in a token the client can read. A token with no `sid` while storage is a resolver is REJECTED, not guessed at; tokens live an hour, so the client restarts from init. Static configs are unaffected ----------------------------- A static object emits NO `sid` (there is one destination to bind to) and skips every resolver path, so its tokens and behavior are byte-identical to before — pinned by two tests asserting the token has no `sid` and that the old flow still 200s. Also in this change ------------------- - keyStrategy ctx gains `metadata` and `req` (the issue's second ask). The existing { userId, fileName, contentType, size } fields are untouched, so existing strategies keep working. - FileMetadata gains an optional free-form `metadata` object, the wire field the client uses to say which class of upload this is. It is UNTRUSTED and documented as such in the type, the guide, and the API reference. - S3 clients are now cached per destination (endpoint|region|bucket| accessKeyId|forcePathStyle) instead of constructed per call, so multi-bucket traffic does not rebuild a connection pool on every presign. The secret key is never part of the cache key and is never logged. - Construct-time guards apply to static configs only; a resolver's result is validated per request (bucket/region present, S3-capable provider) and a bad one fails that request with 500 "Storage configuration error" through the Responder, with the real cause going to onError alone. There is deliberately no fallback bucket — a misrouted write is worse than a failed one. - /health reports checks.storage "skipped" and summary.storageType "dynamic" for a resolver: there is no single destination to probe, and calling an integrator's resolver from an unauthenticated liveness route would reach a real backend. - RENAME carried over from #338 in this same branch: PresignResponseContext's `metadata` is now `file` (it was always the FileMetadata), freeing `metadata` to mean the client's free-form hints consistently across PresignResponseContext, KeyStrategyContext and StorageResolverContext. One name, one meaning, per the repo's naming rule. - hooks.onPresignResponse's inline signature became the exported `OnPresignResponse` type so the `| void` idiom could carry a scoped, described eslint exemption rather than being degraded to `| undefined` (which would force every inspect-only hook to end in `return undefined`). New exported types (types only — the runtime public-API pin is unchanged): UpupStorageResolver, StorageResolverPhase, StorageResolverContext, UpupClientMetadata, OnPresignResponse. RED before the implementation (packages/server/tests/storage-routing.test.ts): FAIL tests/storage-routing.test.ts [ tests/storage-routing.test.ts ] Error: Cannot find module '/src/resolve-storage' imported from .../packages/server/tests/storage-routing.test.ts > 63| import { storageIdentity } from '../src/resolve-storage' Gates: server unit suite 300 passed | 35 skipped (handler-extended, response-contract and trust-model unmodified and green), typecheck (src + test tree) exit 0, eslint --max-warnings 0 clean, test:quality OK (374 test files, 0 exceptions), @upupjs/next typecheck exit 0 against the rebuilt server dist, docs:links:check / docs:snippets:check OK. --- .../docs/api-reference/server-http.mdx | 22 + .../content/docs/guides/server-mode-setup.mdx | 89 ++++ packages/server/src/config.ts | 90 +++- packages/server/src/drive-routes.ts | 17 +- packages/server/src/handler.ts | 5 +- packages/server/src/health.ts | 33 +- packages/server/src/index.ts | 5 + packages/server/src/providers/aws.ts | 20 +- packages/server/src/providers/s3-client.ts | Bin 1062 -> 2171 bytes packages/server/src/resolve-storage.ts | 144 ++++++ packages/server/src/transfer.ts | 8 +- packages/server/src/upload-routes.ts | 180 +++++++- packages/server/src/uploadToken.ts | 10 +- packages/server/src/validate-config.ts | 3 + .../integration/minio.integration.test.ts | 4 +- packages/server/tests/key.test.ts | 9 + .../tests/presign-response-hook.test.ts | 11 +- packages/server/tests/storage-routing.test.ts | 433 ++++++++++++++++++ 18 files changed, 1022 insertions(+), 61 deletions(-) create mode 100644 packages/server/src/resolve-storage.ts create mode 100644 packages/server/tests/storage-routing.test.ts diff --git a/apps/landing/content/docs/api-reference/server-http.mdx b/apps/landing/content/docs/api-reference/server-http.mdx index a820446dc..41434adc1 100644 --- a/apps/landing/content/docs/api-reference/server-http.mdx +++ b/apps/landing/content/docs/api-reference/server-http.mdx @@ -147,9 +147,15 @@ type FileMetadata = { name: string // non-empty type: string // MIME type; may be empty, but then it must pass allowedTypes size: number // bytes, finite, >= 0 + metadata?: Record // free-form; see below } ``` +`metadata` is optional, opaque routing input: the handler neither reads nor +validates it, and passes it through to `keyStrategy` and a `storage` resolver. +It is client-controlled — a server that routes on it must treat it like a query +parameter. `/multipart/init` accepts the same field. + **Response `200`** — the `PresignedUrlResponse` shape from `@upupjs/core`: ```ts @@ -283,6 +289,12 @@ different authenticated user cannot be replayed: Without `getUserId` there was no identity to bind at `init`, `uid` is `null`, and the check is skipped — possession of the token is then the model by design. +**Storage binding.** With a `storage` resolver, the token also carries the +identity of the bucket `init` resolved, and this route answers `403 +AUTH_DENIED` if the resolver returns anything else — a continuation cannot be +steered into a different bucket. A static `storage` binds nothing, since there +is only one destination. + `500` is `{ "error": "Multipart sign failed", "code": "STORAGE_ERROR" }`. ## `POST /multipart/complete` @@ -554,6 +566,16 @@ safety is not a knob that can be raised away. `summary.uploadTokenTtlSeconds`. A multipart session that outlives it must be restarted from `init`. +**Per-request storage.** When `config.storage` is a resolver rather than a +static object, every route resolves its bucket per request, and the multipart +continuation routes are pinned to the one `init` chose: the token carries a +signed storage identity, the resolver is handed it back as `ctx.storageId`, and +a resolved bucket that does not match answers `403 AUTH_DENIED`. A resolver that +returns an unusable config fails that request with `500 Storage configuration +error` (the real cause goes to `onError` only), and `/health` reports +`checks.storage: "skipped"` with `summary.storageType: "dynamic"`. See +[Multi-bucket routing](/docs/guides/server-mode-setup/#multi-bucket-routing). + **Download-URL TTL.** Three days by default, for every signed `GET` the handler returns — `downloadUrl` on `/presign` and `/multipart/complete`, `url` on `/files/:provider/transfer`. Set `downloadUrlExpiresIn` (seconds) to change it. diff --git a/apps/landing/content/docs/guides/server-mode-setup.mdx b/apps/landing/content/docs/guides/server-mode-setup.mdx index c27dec6a6..bd6cd77f4 100644 --- a/apps/landing/content/docs/guides/server-mode-setup.mdx +++ b/apps/landing/content/docs/guides/server-mode-setup.mdx @@ -573,6 +573,95 @@ Full request/response shapes for every route: [Server HTTP API](/docs/api-reference/server-http/). For client-side error wiring, see [Error monitoring](/docs/guides/error-monitoring/). +## Multi-bucket routing + +`storage` takes either one static object or a **resolver** called per request, +for apps that split uploads across buckets — images in one, unscanned documents +in a quarantine bucket, each tenant in their own: + +```ts +import { defineUpupConfig } from '@upupjs/next/server' + +const BUCKETS = { + images: { type: 'aws', bucket: 'app-images', region: 'us-east-1' }, + quarantine: { type: 'aws', bucket: 'app-quarantine', region: 'us-east-1' }, + documents: { type: 'aws', bucket: 'app-docs', region: 'eu-west-1' }, +} as const + +export default defineUpupConfig({ + uploadTokenSecret: process.env.UPUP_UPLOAD_TOKEN_SECRET, + storage: ctx => { + // A continuation of a multipart upload: return the bucket it started in. + if (ctx.storageId) return byIdentity(ctx.storageId) + + const requested = ctx.metadata?.uploadClass + if (requested === 'images') return BUCKETS.images + if (requested === 'unscanned') return BUCKETS.quarantine + return BUCKETS.documents + }, +}) +``` + +The client picks a class by sending a `metadata` object alongside the file's +name, type, and size in the presign body. `ctx` carries `req`, `phase`, +`userId`, `metadata`, `fileName`, `contentType`, `size` — and `storageId` on +multipart continuations. + + + `ctx.metadata` is **whatever the client sent**. Switch on it against a fixed + map like the one above; never build a bucket name, a path prefix, or a + credential out of it. A resolver that does `bucket: ctx.metadata.bucket` + hands every caller the run of your account. + + +### How multipart stays in one bucket + +A multipart upload resolves its bucket once, at `init` — but `sign-part`, +`complete`, and `abort` arrive later carrying only a token, with none of the +metadata that decision was made from. upup closes that gap by stamping an +opaque **storage identity** into the HMAC-signed upload token at `init` and +handing it back to your resolver as `ctx.storageId`. + +Return the storage matching that id and the upload proceeds. The server then +re-derives the identity of whatever you returned and answers +`403 AUTH_DENIED` on a mismatch, so a resolver that ignores `storageId` fails +loudly instead of writing parts into the wrong bucket. The client never supplies +the identity unsigned, and it is a hash of bucket, endpoint, and region only — +never of your credentials, so rotating an access key does not strand uploads in +flight. + +A token issued while `storage` was still a static object carries no identity; +if you then switch to a resolver, uploads already in flight answer `403` and +restart from `init`. Upload tokens live one hour. + +### What changes with a resolver + +- **Validation moves to request time.** A static config is checked when + `createUpupHandler` is constructed; a resolver has nothing to check until a + request arrives, so a bad result fails that one request with a `500` and the + cause goes to `onError`. There is no fallback bucket — a misrouted write is + worse than a failed one. +- **`/health` reports `storage: "skipped"`** and `storageType: "dynamic"`. + There is no single destination to probe, and calling your resolver from an + unauthenticated liveness route would reach a real backend. +- **S3 clients are cached per destination**, keyed on endpoint, region, bucket, + and access key ID, so a multi-bucket deployment does not rebuild a client and + its connection pool on every presign. + +### `keyStrategy` sees it too + +`keyStrategy` now receives `metadata` and `req` alongside the existing +`userId` / `fileName` / `contentType` / `size` — existing strategies keep +working unchanged: + +```ts +keyStrategy: ctx => + `${ctx.metadata?.tenant ?? 'shared'}/${ctx.userId ?? 'anon'}/${crypto.randomUUID()}` +``` + +The same warning applies: `ctx.metadata` is untrusted, and here it lands in an +object key. Sanitize anything you interpolate. + ## Download URLs Every route that reports a stored object hands back a **presigned GET** — diff --git a/packages/server/src/config.ts b/packages/server/src/config.ts index e2469c561..4b36a7a1c 100644 --- a/packages/server/src/config.ts +++ b/packages/server/src/config.ts @@ -7,6 +7,18 @@ import type { } from '@upupjs/core' import type { UpupServerLogger } from './observability' +/** + * Free-form routing hints the CLIENT sends alongside a file's name/type/size, + * as the `metadata` field of a `/presign`, `/multipart/init`, or drive-transfer + * body. upup neither interprets nor validates it — it is carried through to + * `keyStrategy` and the storage resolver verbatim. + * + * It is ATTACKER-CONTROLLED. Treat it as you would a query parameter: switch on + * it against a fixed allow-list, never let it name a bucket, a path prefix, or + * a credential directly. + */ +export type UpupClientMetadata = Record + /** Context passed to a custom keyStrategy. */ export interface KeyStrategyContext { /** Resolved userId, or null when anonymous. */ @@ -14,6 +26,11 @@ export interface KeyStrategyContext { fileName: string contentType: string size: number + /** The client's `metadata` field, if it sent one. Untrusted — see + * {@link UpupClientMetadata}. */ + metadata?: UpupClientMetadata + /** The originating request, past every auth and policy check. */ + req: Request } /** Which of the three presign-side responses `onPresignResponse` is rewriting. */ @@ -27,9 +44,12 @@ export interface PresignResponseContext { /** The server-chosen object key. Present on all three phases — on * `multipart-sign-part` it comes from the VERIFIED token, not the client. */ key: string - /** The client-declared file metadata. Absent on `multipart-sign-part`, - * which sees only a token and a part number. */ - metadata?: FileMetadata + /** The client-declared file (name/type/size). Absent on + * `multipart-sign-part`, which sees only a token and a part number. */ + file?: FileMetadata + /** The client's `metadata` field, if it sent one. Untrusted — see + * {@link UpupClientMetadata}. Absent on `multipart-sign-part`. */ + metadata?: UpupClientMetadata /** Resolved userId, or null for an anonymous (server-namespaced) upload. */ userId: string | null } @@ -45,6 +65,13 @@ export type PresignResponseBody = export type PresignResponseRewrite = PresignResponseBody & Record +/* eslint-disable @typescript-eslint/no-invalid-void-type -- `| void` is the deliberate "rewrite it, or just look at it" idiom: it is what lets an inspect-only hook be written with no return statement at all. `| undefined` would not — TypeScript rejects a void-returning function there, forcing every hook to end in `return undefined`. */ +export type OnPresignResponse = ( + response: PresignResponseBody, + ctx: PresignResponseContext, +) => PresignResponseRewrite | void | Promise +/* eslint-enable @typescript-eslint/no-invalid-void-type -- scope of the exemption above ends here; the rest of this file is held to the rule. */ + /** One bucket's worth of S3 / S3-compatible connection settings. */ export interface UpupStorageConfig { /** @@ -68,8 +95,52 @@ export interface UpupStorageConfig { [key: string]: unknown } +/** Which operation is asking for a storage config. */ +export type StorageResolverPhase = + | 'presign' + | 'multipart-init' + | 'multipart-sign-part' + | 'multipart-complete' + | 'multipart-abort' + | 'drive-transfer' + +export interface StorageResolverContext { + /** The originating request, past every auth and policy check. */ + req: Request + phase: StorageResolverPhase + /** Resolved userId, or null for an anonymous (server-namespaced) upload. */ + userId: string | null + /** The client's `metadata` field, if it sent one. Untrusted — see + * {@link UpupClientMetadata}. Absent on the multipart continuation + * phases, which carry only a token. */ + metadata?: UpupClientMetadata + fileName?: string + contentType?: string + size?: number + /** + * Set on `multipart-sign-part` / `-complete` / `-abort` ONLY: the opaque + * identity of the storage this upload's `init` resolved, carried inside the + * HMAC-signed upload token. Return the SAME storage for it — the server + * re-derives the identity of whatever you return and answers `403 + * AUTH_DENIED` if it does not match, so a continuation can never be + * steered to a different bucket than the one it started in. + */ + storageId?: string +} + +export type UpupStorageResolver = ( + ctx: StorageResolverContext, +) => UpupStorageConfig | Promise + export type UpupServerConfig = { - storage: UpupStorageConfig + /** + * One static bucket, or a resolver called per request to pick one — three + * buckets by upload class, a tenant's own bucket, a quarantine bucket for + * unscanned files. A resolver is validated at RESOLVE time (a bad config + * fails that request with a 500), not at construct time like the static + * form. + */ + storage: UpupStorageConfig | UpupStorageResolver providers?: { googleDrive?: { clientId: string; clientSecret: string } @@ -164,13 +235,7 @@ export type UpupServerConfig = { * Rewriting `uploadUrl` changes where the browser sends bytes, so the * URL you substitute must land at the same object. */ - onPresignResponse?: ( - response: PresignResponseBody, - ctx: PresignResponseContext, - ) => - | PresignResponseRewrite - | void - | Promise + onPresignResponse?: OnPresignResponse } auth?: (req: Request) => Promise @@ -229,6 +294,9 @@ export interface FileMetadata { name: string size: number type: string + /** Free-form routing hints from the client. Untrusted — see + * {@link UpupClientMetadata}. */ + metadata?: UpupClientMetadata } export interface UploadedFile { diff --git a/packages/server/src/drive-routes.ts b/packages/server/src/drive-routes.ts index fe673d344..fccc29255 100644 --- a/packages/server/src/drive-routes.ts +++ b/packages/server/src/drive-routes.ts @@ -13,6 +13,8 @@ import { isValidProvider, refreshAccessToken } from './oauth' import { getDriveClient } from './drive-clients' import { matchesAllowedType, runPostCompletionHooks } from './upload-routes' import { type Responder } from './respond' +import { resolveStorage } from './resolve-storage' +import type { UpupClientMetadata } from './config' export async function handleListFiles( req: Request, @@ -115,6 +117,7 @@ export async function handleFileTransfer( fileName?: string size?: number mimeType?: string + metadata?: UpupClientMetadata } try { body = (await req.json()) as typeof body @@ -146,13 +149,25 @@ export async function handleFileTransfer( const { stream, size, fileName, mimeType } = await getDriveClient( provider, ).fetchFile(tokens.accessToken, body) + // Resolved AFTER the drive reports the real name/type/size, so a + // resolver can route on what is actually being transferred rather than + // on the client's claim (#337). + const storage = await resolveStorage(config, { + req, + phase: 'drive-transfer', + userId, + ...(body.metadata !== undefined ? { metadata: body.metadata } : {}), + fileName, + contentType: mimeType, + size, + }) const { transferDriveFileToS3 } = await import('./transfer') const result = await transferDriveFileToS3({ stream, size, fileName, mimeType, - storage: config.storage, + storage, // Enforce maxFileSize against the ACTUAL streamed bytes, and route // an abort-cleanup failure through onError instead of swallowing it // (F-743 / F-744). diff --git a/packages/server/src/handler.ts b/packages/server/src/handler.ts index 97b75569f..957b6a00d 100644 --- a/packages/server/src/handler.ts +++ b/packages/server/src/handler.ts @@ -3,6 +3,7 @@ import type { UpupServerConfig } from './config' import { assertUploadTokenSecret } from './uploadToken' import { validateServerConfig } from './validate-config' import { assertS3Storage } from './storage' +import { isStorageResolver } from './resolve-storage' import { handleHealth } from './health' import { createResponder } from './respond' import { @@ -36,7 +37,9 @@ export function createUpupHandler(config: UpupServerConfig): RouteHandler { // credentials/region. A provider with no S3-compatible surface (currently // just Azure) could never function, with zero compile- or startup-time // signal until now. - assertS3Storage(config.storage) + // A resolver has no type to check yet — the same guard runs on whatever it + // returns, per request, in resolve-storage.ts (#337). + if (!isStorageResolver(config.storage)) assertS3Storage(config.storage) if ( (config.providers || config.tokenStore) && !config.getUserId && diff --git a/packages/server/src/health.ts b/packages/server/src/health.ts index 2a8c92e6c..50d6e6419 100644 --- a/packages/server/src/health.ts +++ b/packages/server/src/health.ts @@ -13,6 +13,7 @@ import type { Responder } from './respond' import { checkStorageReachable } from './providers/aws' import { reportServerError, toSafeError } from './observability' import { DEFAULT_UPLOAD_TOKEN_TTL_SECONDS } from './uploadToken' +import { isStorageResolver } from './resolve-storage' type StorageCheckCache = { ok: true } | { ok: false } | undefined let cachedStorageCheck: StorageCheckCache @@ -28,12 +29,13 @@ export function _resetStorageCheckCacheForTests(): void { } function isConfigComplete(config: UpupServerConfig): boolean { - return Boolean( - config.storage.bucket && - config.storage.region && - config.uploadTokenSecret && - config.uploadTokenSecret.length >= 16, + const secretOk = Boolean( + config.uploadTokenSecret && config.uploadTokenSecret.length >= 16, ) + // A resolver has no fields to check until a request supplies a context; + // its result is validated per request instead (#337). + if (isStorageResolver(config.storage)) return secretOk + return Boolean(config.storage.bucket && config.storage.region && secretOk) } async function sha256Hex(input: string): Promise { @@ -50,9 +52,19 @@ export async function handleHealth( ): Promise { const configOk = isConfigComplete(config) + // With a storage RESOLVER there is no single destination to probe, and + // inventing a synthetic request to feed the integrator's resolver would + // reach a real backend on an unauthenticated route. Report the probe as + // skipped rather than guessing (#337). + const staticStorage = isStorageResolver(config.storage) + ? null + : config.storage const now = Date.now() - if (!cachedStorageCheck || now - cachedAt > STORAGE_CHECK_TTL_MS) { - const result = await checkStorageReachable(config.storage) + if ( + staticStorage && + (!cachedStorageCheck || now - cachedAt > STORAGE_CHECK_TTL_MS) + ) { + const result = await checkStorageReachable(staticStorage) if (!result.ok) { reportServerError(config.onError, { route: 'health', @@ -68,11 +80,12 @@ export async function handleHealth( cachedAt = now } + const storageOk = staticStorage ? Boolean(cachedStorageCheck?.ok) : true const body: Record = { - status: configOk && cachedStorageCheck.ok ? 'ok' : 'degraded', + status: configOk && storageOk ? 'ok' : 'degraded', checks: { config: configOk ? 'ok' : 'incomplete', - storage: cachedStorageCheck.ok ? 'ok' : 'error', + storage: !staticStorage ? 'skipped' : storageOk ? 'ok' : 'error', }, // Non-secret operational summary — labels/flags/counts only, never any // secret VALUE. Lets an operator eyeball how an instance is configured @@ -80,7 +93,7 @@ export async function handleHealth( // drive providers are wired, the upload-token lifetime) from the same // unauthenticated probe. summary: { - storageType: config.storage.type, + storageType: staticStorage ? staticStorage.type : 'dynamic', anonymousUploads: Boolean(config.allowAnonymousUploads), anonymousDrives: Boolean(config.allowAnonymous), driveProviders: config.providers diff --git a/packages/server/src/index.ts b/packages/server/src/index.ts index c4e20c7cf..de18be856 100644 --- a/packages/server/src/index.ts +++ b/packages/server/src/index.ts @@ -13,6 +13,11 @@ export type { PresignResponseContext, PresignResponseBody, PresignResponseRewrite, + OnPresignResponse, + UpupClientMetadata, + UpupStorageResolver, + StorageResolverPhase, + StorageResolverContext, } from './config' export { getDownloadUrl } from './download-url' export type { DownloadUrlConfig, GetDownloadUrlOptions } from './download-url' diff --git a/packages/server/src/providers/aws.ts b/packages/server/src/providers/aws.ts index 40a0e2bf8..85ab67ed0 100644 --- a/packages/server/src/providers/aws.ts +++ b/packages/server/src/providers/aws.ts @@ -19,7 +19,7 @@ import type { MultipartListPartsResponse, MultipartPart, } from '@upupjs/core' -import type { UpupServerConfig } from '../config' +import type { UpupStorageConfig } from '../config' import { createS3Client } from './s3-client' const DEFAULT_EXPIRES_IN = 3600 @@ -45,7 +45,7 @@ function computePartSize(fileSize: number, chunkSizeBytes?: number): number { // producer as the direct-presign path below, so the 3-day TTL + signing body // are single-sourced instead of duplicated (F-653). export async function generateSignedPublicUrl( - storage: UpupServerConfig['storage'], + storage: UpupStorageConfig, key: string, expiresIn = DEFAULT_DOWNLOAD_URL_EXPIRES_IN, ): Promise { @@ -58,7 +58,7 @@ export async function generateSignedPublicUrl( } export async function generatePresignedUrl( - storage: UpupServerConfig['storage'], + storage: UpupStorageConfig, key: string, contentType: string, contentLength: number, @@ -100,7 +100,7 @@ export async function generatePresignedUrl( } export async function initiateMultipartUpload( - storage: UpupServerConfig['storage'], + storage: UpupStorageConfig, key: string, contentType: string, fileSize: number, @@ -131,7 +131,7 @@ export async function initiateMultipartUpload( } export async function generatePresignedPartUrl( - storage: UpupServerConfig['storage'], + storage: UpupStorageConfig, key: string, uploadId: string, partNumber: number, @@ -152,7 +152,7 @@ export async function generatePresignedPartUrl( } export async function completeMultipartUpload( - storage: UpupServerConfig['storage'], + storage: UpupStorageConfig, key: string, uploadId: string, parts: MultipartPart[], @@ -186,7 +186,7 @@ export async function completeMultipartUpload( } export async function abortMultipartUpload( - storage: UpupServerConfig['storage'], + storage: UpupStorageConfig, key: string, uploadId: string, ): Promise { @@ -204,7 +204,7 @@ export async function abortMultipartUpload( } export async function listMultipartParts( - storage: UpupServerConfig['storage'], + storage: UpupStorageConfig, key: string, uploadId: string, ): Promise { @@ -249,7 +249,7 @@ export async function listMultipartParts( * Paginates on `IsTruncated`, mirroring `listMultipartParts`. */ export async function getMultipartUploadedSize( - storage: UpupServerConfig['storage'], + storage: UpupStorageConfig, key: string, uploadId: string, ): Promise { @@ -288,7 +288,7 @@ export async function getMultipartUploadedSize( * TTL-cached there so a probing client can't hammer the real provider. */ export async function checkStorageReachable( - storage: UpupServerConfig['storage'], + storage: UpupStorageConfig, ): Promise<{ ok: true } | { ok: false; error: unknown }> { try { const client = createS3Client(storage) diff --git a/packages/server/src/providers/s3-client.ts b/packages/server/src/providers/s3-client.ts index 7a8049376a3fc8b3d1e5403d2540c8c26e5234cb..8cfbcb6d654e7ed6aa719399fcd4a7d64d47d742 100644 GIT binary patch literal 2171 zcmZuy+iu%N5bZO+Vt@h>sY(=3KNN7Arm5`$Mv%scT==0V>NUBNwx)O4-DMoYFwl?a z7xqhfW-qcLi4TUwnK^SiXQ&=}V?905^5m>jQu`VCv6tJQXGX8onx1K8%>zw-5|3`~ z+WSQloagRjQm6Fv#umgnz~ zmm)Mp-U4j7Pa!6>p5}Vs@){hla*d>?8N80a={ZEJ6_qJ#EgDR~ zy_4f*ORrvS{he&A^l!7VqL`QKjoYkYZ$UC8I!DMDfZ5E?4(Lk>J1=Knc<<$UiP$r| zM8!4^o5>qLQf@Y(*4%o2wJj!fwlcPnS0KWYd9V|(XL$35KtB)#Ppkza8>~(fDxM*Z z1zkxyZ$Vt8Mc_f!glM3Z^6Qh6@2Z+^Kxt6edH6Ib9%2INHl}Mqz0acY-N<-GOEA&f zk%Vrs=e`RZX65xr)rhX6TP+{A#3}2#u5P~%vOx}T)O4Su1yLF9UdrC#^#kav=66GL z523U`HRA))M9~KBMwP7)zsbTbxdi1M17UbtMymJ5bQKCEi991gSx&8WO<)=1gZEtQ zj=u^-@*~TMq4Hv8iN-UChwGY!mN__b3B`|edCr#t^5622tl>Et%~+$7JU~IeUfkre zEq03m_ziB0l|#@skCm(BF8RQO8Zmp$QPcHCGT8Fs?Bm6CNQ>2IZVwj&N7t-^Ki3mR zl8v2NuVq^oXx8Cig-gqF;r+Z9{SUZbNI2W>>c_GwitWj0m?Ick7VD7{fddgSk^&P0 zf8TO?pX>zIP9`r>%Jz654&SHAAK>k%c^`5an#x7jhpL%Os+j1H`Y#;la`M%rVzum_ zOsrIRnQvwWJU)f&mF5r!&iiJtU@zsC2VWiDek6TOJ5=^`tYF)(QO5yL~VzF zU4#&rcABdwk6rsT>MG?=3Grd#gtJsZH0ET%CmfRLl&*0_=07=kA>~rxy D-YL&+ delta 102 zcmew@u#96u7;9=#S!&V7m@GyyFv~eVFD)}YTD`a=zbG+1RXuj|CB{RHlMk`!>S34H hv{DE*cFxI6%_~u;=28HHqSTVoqP)rV>^e-fTmZS3Bs%~A diff --git a/packages/server/src/resolve-storage.ts b/packages/server/src/resolve-storage.ts new file mode 100644 index 000000000..862f45706 --- /dev/null +++ b/packages/server/src/resolve-storage.ts @@ -0,0 +1,144 @@ +// packages/server/src/resolve-storage.ts +// +// Per-request storage resolution (#337). `config.storage` is either one static +// object — in which case every function here is a passthrough and behavior is +// byte-identical to before — or a resolver invoked per request. +// +// The hard part is the multipart lifecycle. `init` resolves a bucket, but +// `sign-part` / `complete` / `abort` arrive later carrying only a token, with +// none of the metadata the routing decision was made from. Re-running the +// resolver blind would send them somewhere else. Accepting a client-supplied +// hint would let anyone redirect a continuation into a bucket of their +// choosing. +// +// So `init` stamps a STORAGE IDENTITY into the HMAC-signed upload token, and +// each continuation hands it back to the resolver as `ctx.storageId`. The +// resolver returns the matching storage; the server then re-derives the +// identity of what came back and rejects a mismatch. The client never supplies +// the identity unsigned, and a resolver that ignores `storageId` fails closed +// rather than writing to the wrong bucket. +// +// The identity is a hash of the DESTINATION (bucket + endpoint + region), never +// of credentials: rotating an access key must not strand in-flight uploads, and +// nothing secret may sit in a token the client can read. + +import { UpupConfigError } from '@upupjs/core' +import type { + UpupServerConfig, + UpupStorageConfig, + UpupStorageResolver, + StorageResolverContext, +} from './config' +import { assertS3Storage } from './storage' + +export function isStorageResolver( + storage: UpupServerConfig['storage'], +): storage is UpupStorageResolver { + return typeof storage === 'function' +} + +/** + * A stable, opaque, non-secret id for a storage DESTINATION. Deterministic + * across instances and restarts (a plain SHA-256), so a token issued by one + * worker verifies on any other. + */ +export async function storageIdentity( + storage: UpupStorageConfig, +): Promise { + const material = [ + storage.bucket, + storage.endpoint ?? '', + storage.region, + ].join('\n') + const digest = await crypto.subtle.digest( + 'SHA-256', + new TextEncoder().encode(material), + ) + return Array.from(new Uint8Array(digest)) + .map(b => b.toString(16).padStart(2, '0')) + .join('') + .slice(0, 32) +} + +/** The same required-field rules validateServerConfig applies at construct + * time, applied to what a resolver just returned. */ +function assertResolvedStorage( + storage: unknown, +): asserts storage is UpupStorageConfig { + const s = storage as Partial | null | undefined + const missing: string[] = [] + if (!s || typeof s !== 'object') { + throw new UpupConfigError( + '[@upupjs/server] the storage resolver did not return a storage config object.', + ) + } + if (typeof s.bucket !== 'string' || s.bucket.trim() === '') + missing.push('bucket') + if (typeof s.region !== 'string' || s.region.trim() === '') + missing.push('region') + if (missing.length > 0) { + throw new UpupConfigError( + '[@upupjs/server] the storage resolver returned a config missing required field(s): ' + + missing.map(m => `storage.${m}`).join(', '), + ) + } + assertS3Storage(s as UpupStorageConfig) +} + +/** + * The storage for one request. Static configs are returned as-is (already + * validated at construct time); a resolver's result is validated here, because + * this is the first moment it exists. + * + * Throws UpupConfigError on a bad result — callers turn that into a 500 through + * the Responder rather than letting a misrouted upload proceed. + */ +export async function resolveStorage( + config: UpupServerConfig, + ctx: StorageResolverContext, +): Promise { + if (!isStorageResolver(config.storage)) return config.storage + const resolved = await config.storage(ctx) + assertResolvedStorage(resolved) + return resolved +} + +/** + * Resolve for a multipart CONTINUATION and prove it landed where `init` did. + * + * A token with no `sid` predates the resolver (or was issued while `storage` + * was still static) and is rejected rather than routed by guesswork — upload + * tokens live an hour, so the client simply restarts from `init`. + */ +export async function resolveBoundStorage( + config: UpupServerConfig, + ctx: StorageResolverContext, + boundId: string | undefined, +): Promise { + if (!isStorageResolver(config.storage)) return config.storage + if (!boundId) { + throw new StorageBindingError( + 'Upload token carries no storage binding; restart the upload from /multipart/init', + ) + } + const resolved = await resolveStorage(config, { + ...ctx, + storageId: boundId, + }) + if ((await storageIdentity(resolved)) !== boundId) { + throw new StorageBindingError( + 'Upload token is bound to different storage than the resolver returned', + ) + } + return resolved +} + +/** A continuation that cannot be proven to reach the storage its init chose. + * Distinct from UpupConfigError because it is a 403, not a 500 — the request + * is not authorized for that storage, the server is not misconfigured. */ +export class StorageBindingError extends Error { + constructor(message: string) { + super(message) + this.name = 'StorageBindingError' + } +} diff --git a/packages/server/src/transfer.ts b/packages/server/src/transfer.ts index 119d34509..d4f741f9d 100644 --- a/packages/server/src/transfer.ts +++ b/packages/server/src/transfer.ts @@ -6,7 +6,7 @@ import { AbortMultipartUploadCommand, } from '@aws-sdk/client-s3' import { UpupStorageError, UpupErrorCode } from '@upupjs/core' -import type { UpupServerConfig, UploadedFile } from './config' +import type { UpupStorageConfig, UploadedFile } from './config' import { createS3Client } from './providers/s3-client' import { MIN_PART_SIZE, @@ -32,7 +32,7 @@ export async function transferDriveFileToS3(opts: { size: number fileName: string mimeType: string - storage: UpupServerConfig['storage'] + storage: UpupStorageConfig /** Authoritative cap enforced against the ACTUAL streamed bytes (not the * client/drive-declared size). Exceeding it aborts + rejects (F-743). */ maxBytes?: number | undefined @@ -56,7 +56,7 @@ async function singlePut(opts: { size: number fileName: string mimeType: string - storage: UpupServerConfig['storage'] + storage: UpupStorageConfig key: string maxBytes?: number | undefined downloadUrlExpiresIn?: number | undefined @@ -100,7 +100,7 @@ async function streamingMultipart(opts: { size: number fileName: string mimeType: string - storage: UpupServerConfig['storage'] + storage: UpupStorageConfig key: string maxBytes?: number | undefined onError?: UpupServerLogger | undefined diff --git a/packages/server/src/upload-routes.ts b/packages/server/src/upload-routes.ts index 5e9b81b4e..73e968c5d 100644 --- a/packages/server/src/upload-routes.ts +++ b/packages/server/src/upload-routes.ts @@ -35,6 +35,14 @@ import { resolveUserId, DEFAULT_USER_ID } from './tokenStore' import { defaultKeyStrategy } from './key' import { reportServerError, toSafeError } from './observability' import { parseJsonBody, type Responder } from './respond' +import { + isStorageResolver, + resolveStorage, + resolveBoundStorage, + storageIdentity, + StorageBindingError, +} from './resolve-storage' +import type { UpupStorageConfig, StorageResolverContext } from './config' /** Secure-by-default gate for the capability-granting upload routes (/presign, * /multipart/init): reject an unauthenticated, unidentified caller unless the @@ -203,6 +211,67 @@ async function validateUploadMetadata( return null } +/** Resolve the storage for a fresh upload (#337). A resolver that returns + * something unusable is a server misconfiguration, so it fails the request as + * a logged 500 — the real cause goes to onError, the client gets a fixed + * message. Never fall back to a default bucket: a misrouted write is worse + * than a failed one. */ +async function resolveStorageOrFail( + config: UpupServerConfig, + res: Responder, + route: string, + method: string, + ctx: StorageResolverContext, +): Promise { + try { + return await resolveStorage(config, ctx) + } catch (error) { + return res.fail( + route, + method, + 500, + UpupErrorCode.STORAGE_ERROR, + 'Storage configuration error', + error, + ) + } +} + +/** Resolve the storage for a multipart CONTINUATION, pinned to the identity the + * init bound into the token. A binding that cannot be honoured is a 403, not a + * 500 — the request is not authorized for that storage. */ +async function resolveBoundStorageOrFail( + config: UpupServerConfig, + res: Responder, + route: string, + method: string, + ctx: StorageResolverContext, + boundId: string | undefined, +): Promise { + try { + return await resolveBoundStorage(config, ctx, boundId) + } catch (error) { + if (error instanceof StorageBindingError) { + return res.fail( + route, + method, + 403, + UpupErrorCode.AUTH_DENIED, + 'Upload token is not valid for the resolved storage', + error, + ) + } + return res.fail( + route, + method, + 500, + UpupErrorCode.STORAGE_ERROR, + 'Storage configuration error', + error, + ) + } +} + /** Give `hooks.onPresignResponse` the last look at a presign-side payload * (#338). Runs after every auth/policy/token check, so it can rewrite where * the browser sends bytes but can never widen what the caller was allowed to @@ -276,11 +345,30 @@ export async function handlePresign( fileName: body.name, contentType: body.type, size: body.size, + ...(body.metadata !== undefined ? { metadata: body.metadata } : {}), + req, }) + const storage = await resolveStorageOrFail( + config, + res, + 'presign', + req.method, + { + req, + phase: 'presign', + userId: owner, + ...(body.metadata !== undefined ? { metadata: body.metadata } : {}), + fileName: body.name, + contentType: body.type, + size: body.size, + }, + ) + if (storage instanceof Response) return storage + try { const result = await generatePresignedUrl( - config.storage, + storage, key, body.type, body.size, @@ -292,7 +380,8 @@ export async function handlePresign( phase: 'presign', // Always the key that is IN the payload, on every phase. key: result.key, - metadata: body, + file: body, + ...(body.metadata !== undefined ? { metadata: body.metadata } : {}), userId: owner, }) return res.json(payload, 200) @@ -323,12 +412,7 @@ export async function handleMultipartInit( const parsed = await parseJsonBody(req, res) if (!parsed.ok) return parsed.response - const body = parsed.value as { - name: string - type: string - size: number - chunkSizeBytes?: number - } + const body = parsed.value as FileMetadata & { chunkSizeBytes?: number } try { const validationError = await validateUploadMetadata( @@ -348,10 +432,31 @@ export async function handleMultipartInit( fileName: body.name, contentType: body.type, size: body.size, + ...(body.metadata !== undefined ? { metadata: body.metadata } : {}), + req, }) + const storage = await resolveStorageOrFail( + config, + res, + 'multipart/init', + req.method, + { + req, + phase: 'multipart-init', + userId: owner, + ...(body.metadata !== undefined + ? { metadata: body.metadata } + : {}), + fileName: body.name, + contentType: body.type, + size: body.size, + }, + ) + if (storage instanceof Response) return storage + const result = await initiateMultipartUpload( - config.storage, + storage, key, body.type, body.size, @@ -359,12 +464,20 @@ export async function handleMultipartInit( body.chunkSizeBytes, ) assertUploadTokenSecret(config.uploadTokenSecret) + // Bind the resolved destination into the SIGNED token so every + // continuation provably lands in this bucket (#337). Omitted entirely + // for a static config — there is only one destination, and omitting it + // keeps the static token byte-identical to before. + const sid = isStorageResolver(config.storage) + ? await storageIdentity(storage) + : undefined const token = await signUploadToken(config.uploadTokenSecret, { k: result.key, u: result.uploadId, uid: owner, smin: 0, smax: body.size, + ...(sid !== undefined ? { sid } : {}), exp: Math.floor(Date.now() / 1000) + DEFAULT_UPLOAD_TOKEN_TTL_SECONDS, @@ -376,7 +489,10 @@ export async function handleMultipartInit( req, phase: 'multipart-init', key: result.key, - metadata: body, + file: body, + ...(body.metadata !== undefined + ? { metadata: body.metadata } + : {}), userId: owner, }, ) @@ -419,8 +535,21 @@ export async function handleMultipartSignPart( req.method, ) if (owned) return owned + const storage = await resolveBoundStorageOrFail( + config, + res, + 'multipart/sign-part', + req.method, + { + req, + phase: 'multipart-sign-part', + userId: payload.uid, + }, + payload.sid, + ) + if (storage instanceof Response) return storage const result = await generatePresignedPartUrl( - config.storage, + storage, payload.k, payload.u, body.partNumber, @@ -476,18 +605,28 @@ export async function handleMultipartComplete( ) if (owned) return owned + const storage = await resolveBoundStorageOrFail( + config, + res, + 'multipart/complete', + req.method, + { req, phase: 'multipart-complete', userId: payload.uid }, + payload.sid, + ) + if (storage instanceof Response) return storage + // S1 (multipart): smin/smax are SIGNED at init but must be ENFORCED here — // otherwise a client can init with a tiny declared size (tiny smax) and // upload arbitrarily large real parts, since sign-part/PUT never sees the // client-declared size. Sum the bytes S3 actually received (ListParts) and // reject + abort if outside the signed envelope. const uploadedSize = await getMultipartUploadedSize( - config.storage, + storage, payload.k, payload.u, ) if (uploadedSize < payload.smin || uploadedSize > payload.smax) { - await abortMultipartUpload(config.storage, payload.k, payload.u) + await abortMultipartUpload(storage, payload.k, payload.u) return res.json( { error: 'Upload size outside signed envelope' }, 403, @@ -495,7 +634,7 @@ export async function handleMultipartComplete( } const result = await completeMultipartUpload( - config.storage, + storage, payload.k, payload.u, body.parts, @@ -564,11 +703,16 @@ export async function handleMultipartAbort( req.method, ) if (owned) return owned - const result = await abortMultipartUpload( - config.storage, - payload.k, - payload.u, + const storage = await resolveBoundStorageOrFail( + config, + res, + 'multipart/abort', + req.method, + { req, phase: 'multipart-abort', userId: payload.uid }, + payload.sid, ) + if (storage instanceof Response) return storage + const result = await abortMultipartUpload(storage, payload.k, payload.u) return res.json(result, 200) } catch (error) { return res.fail( diff --git a/packages/server/src/uploadToken.ts b/packages/server/src/uploadToken.ts index 8b6745301..d1c53ade2 100644 --- a/packages/server/src/uploadToken.ts +++ b/packages/server/src/uploadToken.ts @@ -20,6 +20,13 @@ export interface UploadTokenPayload { smin: number /** Maximum allowed total size, bytes. */ smax: number + /** + * Opaque identity of the storage this upload's `init` resolved (#337). + * Present only when `config.storage` is a resolver; a static config binds + * nothing because there is only ever one destination. Signed, so the client + * cannot steer a continuation into another bucket. + */ + sid?: string /** Expiry, epoch SECONDS. */ exp: number } @@ -135,7 +142,8 @@ export async function verifyUploadToken( typeof payload.u !== 'string' || typeof payload.exp !== 'number' || typeof payload.smin !== 'number' || - typeof payload.smax !== 'number' + typeof payload.smax !== 'number' || + (payload.sid !== undefined && typeof payload.sid !== 'string') ) { throw new UploadTokenError( 'malformed', diff --git a/packages/server/src/validate-config.ts b/packages/server/src/validate-config.ts index 27f807096..d8599fc57 100644 --- a/packages/server/src/validate-config.ts +++ b/packages/server/src/validate-config.ts @@ -22,6 +22,9 @@ export function validateServerConfig(config: UpupServerConfig): void { // Runtime guard: callers may pass partial/invalid objects at boot time. if (!(config as Partial).storage) { missing.push('storage') + } else if (typeof config.storage === 'function') { + // A storage RESOLVER has no fields yet — its result is validated per + // request in resolve-storage.ts, which is the first moment it exists. } else { if (!isNonEmpty(config.storage.bucket)) missing.push('storage.bucket') if (!isNonEmpty(config.storage.region)) missing.push('storage.region') diff --git a/packages/server/tests/integration/minio.integration.test.ts b/packages/server/tests/integration/minio.integration.test.ts index 53146e631..a7ce203e3 100644 --- a/packages/server/tests/integration/minio.integration.test.ts +++ b/packages/server/tests/integration/minio.integration.test.ts @@ -23,11 +23,11 @@ import { completeMultipartUpload, } from '../../src/providers/aws' import { transferDriveFileToS3 } from '../../src/transfer' -import type { UpupServerConfig } from '../../src/config' +import type { UpupStorageConfig } from '../../src/config' const RUN = process.env.UPUP_E2E_MINIO === '1' -const storage: UpupServerConfig['storage'] = { +const storage: UpupStorageConfig = { type: 'aws', bucket: process.env.UPUP_E2E_BUCKET ?? 'upup-e2e', region: process.env.UPUP_E2E_REGION ?? 'us-east-1', diff --git a/packages/server/tests/key.test.ts b/packages/server/tests/key.test.ts index b26c719af..d42ccbdc5 100644 --- a/packages/server/tests/key.test.ts +++ b/packages/server/tests/key.test.ts @@ -35,6 +35,12 @@ describe('sanitizeFilename', () => { }) }) +// KeyStrategyContext carries the originating request (#337); these cases are +// about key SHAPE, so any request stands in. +const req = new Request('https://app.example.com/api/upup/presign', { + method: 'POST', +}) + describe('defaultKeyStrategy', () => { it('namespaces by userId', () => { const key = defaultKeyStrategy({ @@ -42,6 +48,7 @@ describe('defaultKeyStrategy', () => { fileName: 'f.bin', contentType: 't', size: 1, + req, }) expect(key.startsWith('alice/')).toBe(true) expect(key.endsWith('/f.bin')).toBe(true) @@ -52,6 +59,7 @@ describe('defaultKeyStrategy', () => { fileName: 'f.bin', contentType: 't', size: 1, + req, }) expect(key.startsWith('anon/')).toBe(true) }) @@ -61,6 +69,7 @@ describe('defaultKeyStrategy', () => { fileName: 'f.bin', contentType: 't', size: 1, + req, } expect(defaultKeyStrategy(ctx)).not.toBe(defaultKeyStrategy(ctx)) }) diff --git a/packages/server/tests/presign-response-hook.test.ts b/packages/server/tests/presign-response-hook.test.ts index db9340086..736420290 100644 --- a/packages/server/tests/presign-response-hook.test.ts +++ b/packages/server/tests/presign-response-hook.test.ts @@ -156,6 +156,7 @@ describe('hooks.onPresignResponse (#338)', () => { seen.push({ phase: ctx.phase, key: ctx.key, + file: ctx.file, metadata: ctx.metadata, userId: ctx.userId, isRequest: ctx.req instanceof Request, @@ -176,21 +177,25 @@ describe('hooks.onPresignResponse (#338)', () => { { phase: 'presign', key: 'u1/uuid/photo.png', - metadata: meta, + file: meta, + // This client sent no free-form routing metadata. + metadata: undefined, userId: 'user-7', isRequest: true, }, { phase: 'multipart-init', key: 'u1/uuid/big.zip', - metadata: meta, + file: meta, + metadata: undefined, userId: 'user-7', isRequest: true, }, { - // sign-part sees only a verified token, never file metadata. + // sign-part sees only a verified token, never the file. phase: 'multipart-sign-part', key: 'u1/uuid/big.zip', + file: undefined, metadata: undefined, userId: 'user-7', isRequest: true, diff --git a/packages/server/tests/storage-routing.test.ts b/packages/server/tests/storage-routing.test.ts new file mode 100644 index 000000000..9484c4307 --- /dev/null +++ b/packages/server/tests/storage-routing.test.ts @@ -0,0 +1,433 @@ +// Issue #337: route each upload to a different bucket per request. `storage` +// accepts a resolver function, `keyStrategy` sees the request and the client's +// metadata, and the multipart continuation routes are pinned to the storage the +// init resolved via a signed identity in the upload token. +// +// Mocks providers/aws so every call records the storage it was handed — the +// assertion is always "which bucket did this route actually reach". +import { describe, it, expect, vi, beforeEach } from 'vitest' +import { UpupErrorCode } from '@upupjs/core' +import type { UpupStorageConfig } from '../src/config' + +const reached: Array<{ op: string; bucket: string }> = [] + +function record(op: string) { + return (storage: UpupStorageConfig, ...rest: unknown[]) => { + reached.push({ op, bucket: storage.bucket }) + return rest + } +} + +vi.mock('../src/providers/aws', () => ({ + generatePresignedUrl: vi.fn((storage: UpupStorageConfig, key: string) => { + record('presign')(storage) + return Promise.resolve({ + key, + uploadUrl: `https://${storage.bucket}/put`, + downloadUrl: `https://${storage.bucket}/get`, + expiresIn: 3600, + }) + }), + initiateMultipartUpload: vi.fn( + (storage: UpupStorageConfig, key: string) => { + record('multipart-init')(storage) + return Promise.resolve({ + key, + uploadId: 'mp-1', + partSize: 5 * 1024 * 1024, + expiresIn: 3600, + }) + }, + ), + generatePresignedPartUrl: vi.fn((storage: UpupStorageConfig) => { + record('sign-part')(storage) + return Promise.resolve({ + uploadUrl: `https://${storage.bucket}/part`, + expiresIn: 3600, + }) + }), + completeMultipartUpload: vi.fn( + (storage: UpupStorageConfig, key: string) => { + record('complete')(storage) + return Promise.resolve({ + key, + downloadUrl: `https://${storage.bucket}/get`, + }) + }, + ), + abortMultipartUpload: vi.fn((storage: UpupStorageConfig) => { + record('abort')(storage) + return Promise.resolve({ ok: true }) + }), + getMultipartUploadedSize: vi.fn(() => Promise.resolve(0)), + checkStorageReachable: vi.fn(() => Promise.resolve({ ok: true as const })), + generateSignedPublicUrl: vi.fn(() => Promise.resolve('https://signed')), + DEFAULT_DOWNLOAD_URL_EXPIRES_IN: 3600 * 24 * 3, + MIN_PART_SIZE: 5 * 1024 * 1024, +})) + +// Drive-transfer path: a stub provider that yields a small known file, and a +// transfer that records only which bucket it was pointed at. +const transferred: string[] = [] + +vi.mock('../src/drive-clients', () => ({ + getDriveClient: () => ({ + listFiles: () => Promise.resolve([]), + fetchFile: () => + Promise.resolve({ + stream: new Blob(['hello world!']).stream(), + size: 12, + fileName: 'from-drive.pdf', + mimeType: 'application/pdf', + }), + }), +})) + +vi.mock('../src/transfer', () => ({ + transferDriveFileToS3: (opts: { + storage: UpupStorageConfig + fileName: string + mimeType: string + size: number + }) => { + transferred.push(opts.storage.bucket) + return Promise.resolve({ + key: `k/${opts.fileName}`, + name: opts.fileName, + size: opts.size, + type: opts.mimeType, + url: `https://${opts.storage.bucket}/get`, + }) + }, +})) + +import { createUpupHandler } from '../src/handler' +import { storageIdentity } from '../src/resolve-storage' +import { signUploadToken } from '../src/uploadToken' +import { + InMemoryTokenStore, + setTokens, + DEFAULT_USER_ID, +} from '../src/tokenStore' +import type { UpupServerConfig, StorageResolverContext } from '../src/config' + +const SECRET = 'a-stable-secret-at-least-16' + +const BUCKETS: Record = { + images: { type: 'aws', bucket: 'app-images', region: 'us-east-1' }, + quarantine: { type: 'aws', bucket: 'app-quarantine', region: 'us-east-1' }, + documents: { type: 'aws', bucket: 'app-documents', region: 'eu-west-1' }, +} + +/** The issue's real shape: three buckets, chosen from a client-supplied class, + * and pinned by storageId on the multipart continuation routes. */ +const routeByClass = (ctx: StorageResolverContext): UpupStorageConfig => { + if (ctx.storageId) { + const bound = Object.values(BUCKETS).find( + b => identityOf(b) === ctx.storageId, + ) + if (bound) return bound + } + const cls = String(ctx.metadata?.uploadClass ?? 'documents') + return BUCKETS[cls] ?? BUCKETS.documents! +} + +// Populated in beforeAll-ish fashion below; storageIdentity is async. +const identities = new Map() +const identityOf = (s: UpupStorageConfig) => identities.get(s.bucket) + +function post(path: string, body: unknown): Request { + return new Request(`https://app.example.com/api/upup${path}`, { + method: 'POST', + headers: { 'content-type': 'application/json' }, + body: JSON.stringify(body), + }) +} + +const meta = (uploadClass?: string) => ({ + name: 'scan.pdf', + type: 'application/pdf', + size: 2048, + ...(uploadClass ? { metadata: { uploadClass } } : {}), +}) + +beforeEach(async () => { + reached.length = 0 + transferred.length = 0 + for (const s of Object.values(BUCKETS)) { + identities.set(s.bucket, await storageIdentity(s)) + } +}) + +const dynamic: UpupServerConfig = { + storage: routeByClass, + uploadTokenSecret: SECRET, + allowAnonymousUploads: true, +} + +describe('per-request storage routing (#337)', () => { + it('sends /presign to the bucket the resolver picks from metadata', async () => { + const handler = createUpupHandler(dynamic) + await handler(post('/presign', meta('images'))) + await handler(post('/presign', meta('quarantine'))) + await handler(post('/presign', meta())) + expect(reached.map(r => r.bucket)).toEqual([ + 'app-images', + 'app-quarantine', + 'app-documents', + ]) + }) + + it('gives the resolver the request, phase, identity and declared file', async () => { + const seen: StorageResolverContext[] = [] + const handler = createUpupHandler({ + ...dynamic, + allowAnonymousUploads: false, + getUserId: async () => 'user-3', + storage: ctx => { + seen.push(ctx) + return routeByClass(ctx) + }, + }) + await handler(post('/presign', meta('images'))) + expect(seen).toHaveLength(1) + expect(seen[0]).toMatchObject({ + phase: 'presign', + userId: 'user-3', + metadata: { uploadClass: 'images' }, + fileName: 'scan.pdf', + contentType: 'application/pdf', + size: 2048, + }) + expect(seen[0]?.req).toBeInstanceOf(Request) + }) + + it('routes the drive-transfer path through the resolver too', async () => { + const seen: StorageResolverContext[] = [] + const tokenStore = new InMemoryTokenStore() + const config: UpupServerConfig = { + ...dynamic, + allowAnonymous: true, + tokenStore, + providers: { + googleDrive: { clientId: 'id', clientSecret: 'secret' }, + }, + storage: ctx => { + seen.push(ctx) + return routeByClass(ctx) + }, + } + await setTokens(tokenStore, DEFAULT_USER_ID, 'google-drive', { + accessToken: 'at', + }) + const handler = createUpupHandler(config) + + const res = await handler( + post('/files/google-drive/transfer', { + fileId: 'f1', + metadata: { uploadClass: 'quarantine' }, + }), + ) + + expect(res.status).toBe(200) + expect(seen).toHaveLength(1) + expect(seen[0]).toMatchObject({ + phase: 'drive-transfer', + metadata: { uploadClass: 'quarantine' }, + // The drive's REAL name/type/size, not the client's claim. + fileName: 'from-drive.pdf', + contentType: 'application/pdf', + size: 12, + }) + expect(transferred).toEqual(['app-quarantine']) + }) +}) + +describe('multipart continuations stay bound to the init storage (#337)', () => { + it('reaches the same bucket on sign-part, complete and abort', async () => { + const handler = createUpupHandler(dynamic) + const init = (await ( + await handler(post('/multipart/init', meta('quarantine'))) + ).json()) as { token: string } + + await handler( + post('/multipart/sign-part', { token: init.token, partNumber: 1 }), + ) + await handler( + post('/multipart/complete', { token: init.token, parts: [] }), + ) + await handler(post('/multipart/abort', { token: init.token })) + + expect(reached.map(r => `${r.op}:${r.bucket}`)).toEqual([ + 'multipart-init:app-quarantine', + 'sign-part:app-quarantine', + 'complete:app-quarantine', + 'abort:app-quarantine', + ]) + }) + + it('binds the storage identity into the signed token', async () => { + const handler = createUpupHandler(dynamic) + const init = (await ( + await handler(post('/multipart/init', meta('images'))) + ).json()) as { token: string } + const payload = JSON.parse( + Buffer.from(init.token.split('.')[0]!, 'base64url').toString(), + ) as { sid?: string } + expect(payload.sid).toBe(identityOf(BUCKETS.images!)) + }) + + it('403s when a validly-signed token names storage the resolver will not produce', async () => { + const handler = createUpupHandler({ + ...dynamic, + // Ignores storageId entirely — the resolved bucket cannot match the + // token's bound identity, which must be caught, not silently used. + storage: () => BUCKETS.images!, + }) + const forged = await signUploadToken(SECRET, { + k: 'app-quarantine/uuid/scan.pdf', + u: 'mp-1', + uid: null, + smin: 0, + smax: 2048, + sid: identityOf(BUCKETS.quarantine!)!, + exp: Math.floor(Date.now() / 1000) + 600, + }) + const res = await handler( + post('/multipart/sign-part', { token: forged, partNumber: 1 }), + ) + expect(res.status).toBe(403) + expect((await res.json()) as { code: string }).toMatchObject({ + code: UpupErrorCode.AUTH_DENIED, + }) + expect(reached).toEqual([]) + }) + + it('403s a token carrying no storage identity when storage is a resolver', async () => { + const handler = createUpupHandler(dynamic) + const legacy = await signUploadToken(SECRET, { + k: 'k', + u: 'mp-1', + uid: null, + smin: 0, + smax: 2048, + exp: Math.floor(Date.now() / 1000) + 600, + }) + const res = await handler( + post('/multipart/sign-part', { token: legacy, partNumber: 1 }), + ) + expect(res.status).toBe(403) + expect(reached).toEqual([]) + }) +}) + +describe('static storage config is unchanged (#337)', () => { + const staticConfig: UpupServerConfig = { + storage: BUCKETS.documents!, + uploadTokenSecret: SECRET, + allowAnonymousUploads: true, + } + + it('still reaches the one configured bucket', async () => { + const handler = createUpupHandler(staticConfig) + await handler(post('/presign', meta('images'))) + expect(reached).toEqual([{ op: 'presign', bucket: 'app-documents' }]) + }) + + it('issues a token with NO storage identity, and accepts it', async () => { + const handler = createUpupHandler(staticConfig) + const init = (await ( + await handler(post('/multipart/init', meta())) + ).json()) as { token: string } + const payload = JSON.parse( + Buffer.from(init.token.split('.')[0]!, 'base64url').toString(), + ) as Record + expect('sid' in payload).toBe(false) + + const res = await handler( + post('/multipart/sign-part', { token: init.token, partNumber: 1 }), + ) + expect(res.status).toBe(200) + }) +}) + +describe('resolver failures are config errors, not silent 200s (#337)', () => { + it('500s when the resolver returns a provider with no S3 API', async () => { + const handler = createUpupHandler({ + ...dynamic, + onError: () => {}, + storage: () => ({ ...BUCKETS.images!, type: 'azure' }), + }) + const res = await handler(post('/presign', meta())) + expect(res.status).toBe(500) + const body = (await res.json()) as { error: string; code: string } + expect(body.code).toBe(UpupErrorCode.STORAGE_ERROR) + expect(body.error).toBe('Storage configuration error') + expect(reached).toEqual([]) + }) + + it('500s when the resolver returns a config missing a bucket', async () => { + const handler = createUpupHandler({ + ...dynamic, + onError: () => {}, + storage: () => ({ type: 'aws', bucket: '', region: 'us-east-1' }), + }) + const res = await handler(post('/presign', meta())) + expect(res.status).toBe(500) + expect(reached).toEqual([]) + }) + + it('does not construct-throw for a resolver, unlike a static azure config', () => { + expect(() => + createUpupHandler({ + storage: () => BUCKETS.images!, + uploadTokenSecret: SECRET, + allowAnonymousUploads: true, + }), + ).not.toThrow() + expect(() => + createUpupHandler({ + storage: { ...BUCKETS.images!, type: 'azure' }, + uploadTokenSecret: SECRET, + allowAnonymousUploads: true, + }), + ).toThrow(/no S3-compatible API/) + }) +}) + +describe('keyStrategy sees the request and metadata (#337)', () => { + it('passes metadata and req alongside the existing fields', async () => { + const seen: Array> = [] + const handler = createUpupHandler({ + storage: BUCKETS.images!, + uploadTokenSecret: SECRET, + allowAnonymousUploads: true, + keyStrategy: ctx => { + seen.push({ + userId: ctx.userId, + fileName: ctx.fileName, + contentType: ctx.contentType, + size: ctx.size, + metadata: ctx.metadata, + isRequest: ctx.req instanceof Request, + }) + return `${String(ctx.metadata?.uploadClass)}/${ctx.fileName}` + }, + }) + const body = (await ( + await handler(post('/presign', meta('quarantine'))) + ).json()) as { key: string } + + expect(seen).toEqual([ + { + userId: null, + fileName: 'scan.pdf', + contentType: 'application/pdf', + size: 2048, + metadata: { uploadClass: 'quarantine' }, + isRequest: true, + }, + ]) + expect(body.key).toBe('quarantine/scan.pdf') + }) +}) From 99393b749e50cf7d9168f4f6d99ec15a4f295ac3 Mon Sep 17 00:00:00 2001 From: AminDhouib Date: Wed, 12 Aug 2026 10:36:08 -0400 Subject: [PATCH 12/14] test(server): pin the per-destination S3 client cache (#337) The previous commit made createS3Client memoize, and added `_resetS3ClientCacheForTests` with nothing calling it. Exercise the behavior instead of leaving a dead export: one client is reused for a repeated destination, bucket/region/endpoint each key separately, and a rotated accessKeyId yields a NEW client rather than one still signing with the old key. Gates: server typecheck exit 0, eslint --max-warnings 0 exit 0, test:quality exit 0 (374 test files), s3-client.test.ts 8 passed. --- packages/server/tests/s3-client.test.ts | 35 +++++++++++++++++++++++-- 1 file changed, 33 insertions(+), 2 deletions(-) diff --git a/packages/server/tests/s3-client.test.ts b/packages/server/tests/s3-client.test.ts index 1bfd70477..3517a75fb 100644 --- a/packages/server/tests/s3-client.test.ts +++ b/packages/server/tests/s3-client.test.ts @@ -1,5 +1,9 @@ -import { describe, it, expect } from 'vitest' -import { buildS3ClientConfig } from '../src/providers/s3-client' +import { describe, it, expect, beforeEach } from 'vitest' +import { + buildS3ClientConfig, + createS3Client, + _resetS3ClientCacheForTests, +} from '../src/providers/s3-client' const base = { type: 'aws', bucket: 'b', region: 'us-east-1' } as const @@ -48,3 +52,30 @@ describe('buildS3ClientConfig', () => { expect(cfg.forcePathStyle).toBe(false) }) }) + +// Per-destination client cache (#337): multi-bucket routing must not rebuild a +// client and its connection pool on every presign, and must not silently reuse +// one across destinations or across a credential rotation. +describe('createS3Client caching', () => { + beforeEach(() => { + _resetS3ClientCacheForTests() + }) + + it('reuses one client for repeated calls with the same destination', () => { + expect(createS3Client({ ...base })).toBe(createS3Client({ ...base })) + }) + + it('builds a separate client per bucket, region and endpoint', () => { + const first = createS3Client({ ...base }) + expect(createS3Client({ ...base, bucket: 'other' })).not.toBe(first) + expect(createS3Client({ ...base, region: 'eu-west-1' })).not.toBe(first) + expect( + createS3Client({ ...base, endpoint: 'http://localhost:9100' }), + ).not.toBe(first) + }) + + it('builds a new client when the access key rotates', () => { + const old = createS3Client({ ...base, accessKeyId: 'AK1' }) + expect(createS3Client({ ...base, accessKeyId: 'AK2' })).not.toBe(old) + }) +}) From 92ea13dfb7e5d99a317b090c54267bcf1688ef4d Mon Sep 17 00:00:00 2001 From: AminDhouib Date: Wed, 12 Aug 2026 10:42:56 -0400 Subject: [PATCH 13/14] docs(api): map the re-exported error surface + uploadErrorFromResponse (#339) --- .../landing/content/docs/api-reference/error-codes.mdx | 8 ++++++++ scripts/docs/api-docs-map.json | 10 ++++++++++ 2 files changed, 18 insertions(+) diff --git a/apps/landing/content/docs/api-reference/error-codes.mdx b/apps/landing/content/docs/api-reference/error-codes.mdx index bc66eb4b2..4602ec7b7 100644 --- a/apps/landing/content/docs/api-reference/error-codes.mdx +++ b/apps/landing/content/docs/api-reference/error-codes.mdx @@ -10,6 +10,14 @@ to switch on, log, and tag in an error tracker. This page is the complete list; Monitoring](/docs/guides/error-monitoring/) covers wiring failures into Sentry or your own reporter. +Everything on this page — `UpupError`, `UpupValidationError`, `UpupNetworkError`, +`UpupStorageError`, `UpupAuthError`, `UpupConfigError`, `UpupQuotaError`, +`UpupErrorCode`, and `uploadErrorFromResponse` — is exported from `@upupjs/core` +and re-exported by `@upupjs/react` (and therefore `@upupjs/next` and +`@upupjs/preact`), so an app that only installed its framework package can +import the whole surface from there without adding a direct `@upupjs/core` +dependency. + The one important exception is the rejection from `upload()` itself — see [Batch failures](/docs/api-reference/error-codes/#batch-failures-are-not-upuperrors) before you write a handler, because it is **not** an `UpupError` and carries no diff --git a/scripts/docs/api-docs-map.json b/scripts/docs/api-docs-map.json index d193dfac2..be1f44f74 100644 --- a/scripts/docs/api-docs-map.json +++ b/scripts/docs/api-docs-map.json @@ -32,6 +32,7 @@ "frFR": "apps/landing/content/docs/api-reference/upupuploader/optional-props.mdx", "jaJP": "apps/landing/content/docs/localization.mdx", "koKR": "apps/landing/content/docs/localization.mdx", + "uploadErrorFromResponse": "apps/landing/content/docs/api-reference/error-codes.mdx", "zhCN": "apps/landing/content/docs/localization.mdx", "zhTW": "apps/landing/content/docs/localization.mdx" }, @@ -44,8 +45,17 @@ "GoogleDriveIcon": "apps/landing/content/docs/quickstarts/react.mdx", "OneDriveIcon": "apps/landing/content/docs/quickstarts/react.mdx", "StorageProvider": "apps/landing/content/docs/guides/storage-providers.mdx", + "UpupAuthError": "apps/landing/content/docs/api-reference/error-codes.mdx", + "UpupConfigError": "apps/landing/content/docs/api-reference/error-codes.mdx", + "UpupError": "apps/landing/content/docs/api-reference/error-codes.mdx", + "UpupErrorCode": "apps/landing/content/docs/api-reference/error-codes.mdx", + "UpupNetworkError": "apps/landing/content/docs/api-reference/error-codes.mdx", + "UpupQuotaError": "apps/landing/content/docs/api-reference/error-codes.mdx", + "UpupStorageError": "apps/landing/content/docs/api-reference/error-codes.mdx", "UpupThemeProvider": "apps/landing/content/docs/guides/theming.mdx", "UpupUploader": "apps/landing/content/docs/api-reference/upupuploader/optional-props.mdx", + "UpupValidationError": "apps/landing/content/docs/api-reference/error-codes.mdx", + "uploadErrorFromResponse": "apps/landing/content/docs/api-reference/error-codes.mdx", "useUploaderContext": "apps/landing/content/docs/quickstarts/preact.mdx", "useUploaderFiles": "apps/landing/content/docs/guides/headless.mdx", "useUploaderUploadControls": "apps/landing/content/docs/guides/headless.mdx", From 5fbd2c671a1834cd8e884bda455eb5602480f829 Mon Sep 17 00:00:00 2001 From: AminDhouib Date: Wed, 12 Aug 2026 10:44:33 -0400 Subject: [PATCH 14/14] chore: changeset for the 2026-08 issue batch (minor) --- .changeset/issue-batch-server-react.md | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) create mode 100644 .changeset/issue-batch-server-react.md diff --git a/.changeset/issue-batch-server-react.md b/.changeset/issue-batch-server-react.md new file mode 100644 index 000000000..164dee4f5 --- /dev/null +++ b/.changeset/issue-batch-server-react.md @@ -0,0 +1,16 @@ +--- +'@upupjs/core': minor +'@upupjs/react': minor +'@upupjs/server': minor +'@upupjs/next': minor +--- + +Issue-batch release: headless and server API gaps reported by v1→v3 migrators. + +- `@upupjs/react` re-exports the full core error surface — `UpupError` and its six subclasses, `UpupErrorCode`, and `uploadErrorFromResponse` — so framework-only apps no longer need a direct `@upupjs/core` dependency for typed error handling (#339). `uploadErrorFromResponse` is now on core's public entry, making the documented import real. +- Headless prop getters (`getRootProps` / `getDropzoneProps` / `getInputProps`) now share one override contract: overrides are spread first, getter-owned functional keys are set after, event handlers are composed instead of dropped, and `getInputProps` merges `style` rather than clobbering it (#341). +- Restriction failures raised through the file input, dropzone drop, or paste no longer surface as unhandled promise rejections — the `restriction-failed` event remains the reporting channel (#342). +- `@upupjs/server`: new `getDownloadUrl(config, key, opts?)` primitive signs a GET for an existing key without a handler, and `downloadUrlExpiresIn` makes the download-URL expiry configurable (#343). +- `@upupjs/server`: new `hooks.onPresignResponse` rewrites the presign, multipart-init, and sign-part responses (for proxied or non-browser-reachable storage endpoints), and an `UpupError` thrown from `onBeforeUpload` now surfaces its message and code in the 403 instead of a generic rejection (#338). +- `@upupjs/server`: `storage` accepts a per-request resolver `(ctx) => StorageConfig` for multi-bucket routing; multipart continuations are bound to the resolved destination through the HMAC-signed upload token, and `keyStrategy` now receives `metadata` and `req` (#337). +- `@upupjs/next`: the Pages Router handler body is `BodyInit`-compatible with newer `@types/node`.