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`. 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 \ 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/apps/landing/content/docs/api-reference/server-http.mdx b/apps/landing/content/docs/api-reference/server-http.mdx index 063c7c599..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 @@ -166,8 +172,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** @@ -179,6 +185,7 @@ three days. | `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 | @@ -282,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` @@ -309,7 +322,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 +566,67 @@ 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. +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 +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/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/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/guides/server-mode-setup.mdx b/apps/landing/content/docs/guides/server-mode-setup.mdx index a179ba9e6..bd6cd77f4 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 @@ -475,6 +573,150 @@ 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** — +`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/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). 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/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 } /** diff --git a/packages/react/src/index.ts b/packages/react/src/index.ts index 871fee2b7..fb28273f3 100644 --- a/packages/react/src/index.ts +++ b/packages/react/src/index.ts @@ -38,6 +38,26 @@ 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, + uploadErrorFromResponse, +} from '@upupjs/core' +export type { RestrictionFailedReason } from '@upupjs/core' + // ── React types ────────────────────────────────────────── export type { ImageEditorOptions, diff --git a/packages/react/src/prop-getters.ts b/packages/react/src/prop-getters.ts index e65d6809d..d6fca3597 100644 --- a/packages/react/src/prop-getters.ts +++ b/packages/react/src/prop-getters.ts @@ -34,6 +34,46 @@ 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: unknown): void { + // Duck-typed rather than `instanceof Promise` — a dep may hand back a + // thenable from another realm, which `instanceof` would silently miss. + // `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(() => {}) + } +} + +/** + * 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, @@ -69,13 +109,21 @@ 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) } 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, @@ -92,10 +140,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, } } @@ -104,11 +148,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, } } @@ -118,21 +162,33 @@ 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 { + 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/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/error-exports.test.ts b/packages/react/tests/error-exports.test.ts new file mode 100644 index 000000000..ac5e2875c --- /dev/null +++ b/packages/react/tests/error-exports.test.ts @@ -0,0 +1,89 @@ +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('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', + 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/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' }) + }) +}) 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() + }) +}) diff --git a/packages/react/tests/public-api.test.ts b/packages/react/tests/public-api.test.ts index b878a114a..18d1d4c2b 100644 --- a/packages/react/tests/public-api.test.ts +++ b/packages/react/tests/public-api.test.ts @@ -19,9 +19,25 @@ 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', + // Completes the documented error toolkit alongside the taxonomy above + // (#339) — promoted to core's public entry in the same change. + 'uploadErrorFromResponse', 'useIsClient', 'useUploaderContext', 'useUploaderEditor', diff --git a/packages/server/src/config.ts b/packages/server/src/config.ts index 4d7ae1915..4b36a7a1c 100644 --- a/packages/server/src/config.ts +++ b/packages/server/src/config.ts @@ -1,6 +1,24 @@ -import type { StorageProvider, UpupCorsConfig } from '@upupjs/core' +import type { + StorageProvider, + UpupCorsConfig, + PresignedUrlResponse, + MultipartInitResponse, + MultipartSignPartResponse, +} 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. */ @@ -8,30 +26,121 @@ 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. */ +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 (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 +} + +/** 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 + +/* 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 { + /** + * 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 +} + +/** 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: { - /** - * 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 - } + /** + * 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 } @@ -63,6 +172,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 @@ -86,12 +204,38 @@ 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?: OnPresignResponse } auth?: (req: Request) => Promise @@ -150,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/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..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,19 +149,32 @@ 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). 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..957b6a00d 100644 --- a/packages/server/src/handler.ts +++ b/packages/server/src/handler.ts @@ -1,11 +1,9 @@ -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 { isStorageResolver } from './resolve-storage' import { handleHealth } from './health' import { createResponder } from './respond' import { @@ -39,17 +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. - 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.', - ) - } + // 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 8c4ae8de2..de18be856 100644 --- a/packages/server/src/index.ts +++ b/packages/server/src/index.ts @@ -2,13 +2,25 @@ export { createUpupHandler } from './handler' export type { RouteHandler } from './handler' export type { UpupServerConfig, + UpupStorageConfig, TokenStore, DriveTokens, OAuthState, FileMetadata, UploadedFile, KeyStrategyContext, + PresignResponsePhase, + PresignResponseContext, + PresignResponseBody, + PresignResponseRewrite, + OnPresignResponse, + UpupClientMetadata, + UpupStorageResolver, + StorageResolverPhase, + StorageResolverContext, } 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..85ab67ed0 100644 --- a/packages/server/src/providers/aws.ts +++ b/packages/server/src/providers/aws.ts @@ -19,11 +19,14 @@ 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 -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). @@ -42,9 +45,9 @@ 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_PUBLIC_URL_EXPIRES_IN, + expiresIn = DEFAULT_DOWNLOAD_URL_EXPIRES_IN, ): Promise { const client = createS3Client(storage) return getSignedUrl( @@ -55,11 +58,12 @@ export async function generateSignedPublicUrl( } export async function generatePresignedUrl( - storage: UpupServerConfig['storage'], + storage: UpupStorageConfig, key: string, 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, @@ -92,7 +100,7 @@ export async function generatePresignedUrl( } export async function initiateMultipartUpload( - storage: UpupServerConfig['storage'], + storage: UpupStorageConfig, key: string, contentType: string, fileSize: number, @@ -123,7 +131,7 @@ export async function initiateMultipartUpload( } export async function generatePresignedPartUrl( - storage: UpupServerConfig['storage'], + storage: UpupStorageConfig, key: string, uploadId: string, partNumber: number, @@ -144,10 +152,11 @@ export async function generatePresignedPartUrl( } export async function completeMultipartUpload( - storage: UpupServerConfig['storage'], + storage: UpupStorageConfig, 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, @@ -173,7 +186,7 @@ export async function completeMultipartUpload( } export async function abortMultipartUpload( - storage: UpupServerConfig['storage'], + storage: UpupStorageConfig, key: string, uploadId: string, ): Promise { @@ -191,7 +204,7 @@ export async function abortMultipartUpload( } export async function listMultipartParts( - storage: UpupServerConfig['storage'], + storage: UpupStorageConfig, key: string, uploadId: string, ): Promise { @@ -236,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 { @@ -275,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 7a8049376..8cfbcb6d6 100644 Binary files a/packages/server/src/providers/s3-client.ts and b/packages/server/src/providers/s3-client.ts differ 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/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..d4f741f9d 100644 --- a/packages/server/src/transfer.ts +++ b/packages/server/src/transfer.ts @@ -6,9 +6,13 @@ 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, generateSignedPublicUrl } from './providers/aws' +import { + MIN_PART_SIZE, + generateSignedPublicUrl, + DEFAULT_DOWNLOAD_URL_EXPIRES_IN, +} from './providers/aws' import { reportServerError, toSafeError, @@ -28,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 @@ -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}` @@ -50,9 +56,10 @@ async function singlePut(opts: { size: number fileName: string mimeType: string - storage: UpupServerConfig['storage'] + storage: UpupStorageConfig 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, @@ -89,11 +100,12 @@ async function streamingMultipart(opts: { size: number fileName: string mimeType: string - storage: UpupServerConfig['storage'] + storage: UpupStorageConfig key: string 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..73e968c5d 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, @@ -29,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 @@ -175,7 +189,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 +211,81 @@ 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 + * 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 @@ -243,16 +345,46 @@ 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, + 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, + file: body, + ...(body.metadata !== undefined ? { metadata: body.metadata } : {}), + userId: owner, + }) + return res.json(payload, 200) } catch (error) { return res.fail( 'presign', @@ -280,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( @@ -305,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, @@ -316,17 +464,39 @@ 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, }) - return res.json({ ...result, token }, 200) + const payload = await applyPresignResponseHook( + config, + { ...result, token }, + { + req, + phase: 'multipart-init', + key: result.key, + file: body, + ...(body.metadata !== undefined + ? { metadata: body.metadata } + : {}), + userId: owner, + }, + ) + return res.json(payload, 200) } catch (error) { return res.fail( 'multipart/init', @@ -365,13 +535,34 @@ 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, ) - 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', @@ -414,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, @@ -433,10 +634,11 @@ export async function handleMultipartComplete( } const result = await completeMultipartUpload( - config.storage, + storage, payload.k, payload.u, body.parts, + config.downloadUrlExpiresIn, ) const uploaded: UploadedFile = { @@ -501,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/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/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 new file mode 100644 index 000000000..736420290 --- /dev/null +++ b/packages/server/tests/presign-response-hook.test.ts @@ -0,0 +1,300 @@ +// 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, + file: ctx.file, + 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', + 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', + file: meta, + metadata: undefined, + userId: 'user-7', + isRequest: true, + }, + { + // 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, + }, + ]) + }) + + 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') + }) +}) 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/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) + }) +}) 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') + }) +}) diff --git a/scripts/docs/api-docs-map.json b/scripts/docs/api-docs-map.json index 7d560b0ae..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", @@ -89,7 +99,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",