diff --git a/.changeset/service-error-envelope-conformance.md b/.changeset/service-error-envelope-conformance.md new file mode 100644 index 0000000000..c7a050d892 --- /dev/null +++ b/.changeset/service-error-envelope-conformance.md @@ -0,0 +1,50 @@ +--- +"@objectstack/service-storage": patch +"@objectstack/service-i18n": patch +--- + +fix(service-storage,service-i18n): emit the declared error envelope, not a bare `{ error }` (#3675) + +#3636 aligned the **success** bodies of the autonomously-mounted service +routes because those were the ones breaking `ObjectStackClient.unwrapResponse`. +The error bodies were left alone and stayed a bare `{ error: '' }` — +with the code, where one existed at all, as a *sibling* of `error` rather than +a field of it — against a contract (`BaseResponseSchema` + `ApiErrorSchema`) +that declares `{ success: false, error: { code, message } }`. + +So the same SDK method returned two different error shapes depending on which +provider mounted the route: a caller reading `body.error.message` got the real +message from the dispatcher and `undefined` from these services. All 32 sites +(27 in `storage-routes.ts`, 5 in `i18n-service-plugin.ts`) now go through a +single `sendError` helper per module — the nested-`error` shape the sibling +services already use (`settings-routes.ts`, `share-link-routes.ts`), plus the +`success` flag those two still omit and the contract requires. + +**Codes moved, and that is the breaking part.** `AUTH_REQUIRED`, +`ATTACHMENT_DOWNLOAD_DENIED` and `FILE_DOWNLOAD_DENIED` used to sit at +`body.code`; they now sit at `body.error.code`. The SDK is unaffected — it +already reads `errorBody?.code || errorBody?.error?.code`, one of the four +shapes its error path sniffs for, which is the consumer-side shim Prime +Directive #12 says to cure at the producer. The console's attachment panel +was NOT: it read the top level only, so every gated download would have +degraded from "You don't have access to download this attachment." to +"Download failed (403)". Fixed in objectui to read both dialects, since a +console build ships independently of the server it talks to. + +**Guarded both ways.** New `error-envelope.conformance.test.ts` in each +service drives every distinct error branch through the real registrar and +parses the body against the real `BaseResponseSchema` imported from +`packages/spec` — not a local restatement of it — and scans the module source +so a new route cannot quietly reintroduce the bare shape. The route ledgers +(#3563 → #3656) could never have caught this: they audit which routes exist +and whether the SDK can address them, not what comes back. + +Measured and left alone: the dispatcher does not conform either — it puts the +HTTP status in `error.code`, where the contract declares a semantic string, +and parks the real code in `details` to work around its own occupied field. +That deviation is now pinned to exactly one field by a test in +`http-dispatcher.test.ts` rather than described in prose. Also unchanged: +service-storage's success bodies are still three shapes of their own +(`{ data }`, bare `{ url }`, `{ ok, key }`, none with `success: true`) — a +non-additive change that needs its own issue, not a quiet ride along with this +one. diff --git a/content/docs/api/wire-format.mdx b/content/docs/api/wire-format.mdx index c64d36f46a..55db92e101 100644 --- a/content/docs/api/wire-format.mdx +++ b/content/docs/api/wire-format.mdx @@ -384,6 +384,27 @@ Returned when an `If-Match` / `expectedVersion` token no longer matches the stor **Error Codes:** See the [Error Catalog](/docs/api/error-catalog) for the complete list of error codes and their meanings. +### Service-mounted routes use the declared envelope + +The flat envelope above is what the kernel REST server emits for `/api/v1/data/*`. +Routes mounted directly by a service plugin — `/api/v1/storage/*` and +`/api/v1/i18n/*` — instead return the `BaseResponse` shape their contract +declares, with the code **inside** the error object: + +```json +{ + "success": false, + "error": { + "code": "ATTACHMENT_DOWNLOAD_DENIED", + "message": "You do not have access to a record this file is attached to" + } +} +``` + +Read `body.error.code`, not `body.code`, on these routes. `ObjectStackClient` +normalizes both — `error.code` and `error.message` are populated whichever +envelope the server used — so SDK callers do not need to branch. + --- ## 8. Batch Operations diff --git a/docs/audits/2026-07-dispatcher-client-route-coverage.md b/docs/audits/2026-07-dispatcher-client-route-coverage.md index e4c67b687b..6cee8a9412 100644 --- a/docs/audits/2026-07-dispatcher-client-route-coverage.md +++ b/docs/audits/2026-07-dispatcher-client-route-coverage.md @@ -271,6 +271,37 @@ capstone also had to prefer exact rows over wildcard families when matching — otherwise every `/auth/*` URL would still have been absorbed by `* /auth/**` and the new ledger would have changed nothing. +## 12. What the ledgers cannot see: response bodies (#3675) + +Worth recording as a **boundary of this audit family**, not an oversight in it. +Every guard from §1 to §11 answers one question — does this route exist, and +can the SDK address it? None of them looks at what comes back. That is how +service-i18n and service-storage carried green `sdk` rows for surfaces that +emitted a bare `{ error: '' }` against a contract declaring +`{ success: false, error: { code, message } }`. + +Four error dialects were live in-repo at the time: + +| Producer | Shape | +|---|---| +| `contract.zod.ts` (declared) | `{ success, error: { code: string, message, … } }` | +| `http-dispatcher.ts` | `{ success: false, error: { message, code: , details } }` | +| `rest-server.ts` | `{ error: , code: }` | +| `settings-routes.ts`, `share-link-routes.ts` | `{ error: { code, message } }` — no `success` | +| service-i18n, service-storage | `{ error: }`, sometimes `+ code` at the top level | + +#3675 moved the last row to the contract. The rest are unchanged, and the +dispatcher's deviation is now pinned to exactly one field (`error.code` carries +the HTTP status where a semantic string is declared) by a test rather than by +prose — it parks the real code in `details` to work around its own occupied +field, which is the tell. + +The generalisable lesson matches §11's: a guard only covers the question it +asks. "The route exists" and "the route answers in the declared shape" are two +questions, and the second one needs the contract imported into the assertion — +`BaseResponseSchema.safeParse(body)`, not a hand-copied restatement that drifts +from the schema it claims to check. + ## Follow-up slicing (proposed) 1. **`client.actions.invoke(...)`** — closes the largest hole (3 routes). @@ -286,6 +317,9 @@ and the new ledger would have changed nothing. 11. **Control-plane surface** (§10) — #3655, needs a ledger in the `cloud` repo. 12. **Enumerate `/auth/**`** (§11) — done in #3656; wildcard ratchet 60 → 3. 13. **Enumerate `/ai/**`** — the last dynamic family, 3 SDK methods. +14. **Response-shape conformance** (§12) — error path done in #3675 for both + services; the storage **success** bodies (three shapes, none carrying + `success: true`) and the dispatcher's numeric `error.code` remain. Each gap closed must flip its ledger row to `sdk` and lower the ratchet bound in the conformance test — the guard enforces both directions from PR-1 onward. diff --git a/packages/qa/dogfood/test/attachments-permission-matrix.dogfood.test.ts b/packages/qa/dogfood/test/attachments-permission-matrix.dogfood.test.ts index a12a71b2b8..b3c8dc252b 100644 --- a/packages/qa/dogfood/test/attachments-permission-matrix.dogfood.test.ts +++ b/packages/qa/dogfood/test/attachments-permission-matrix.dogfood.test.ts @@ -157,7 +157,7 @@ describe('attachments permission matrix (#2755)', () => { body: JSON.stringify({ filename: 'x.txt', mimeType: 'text/plain', size: 1, scope: 'attachments' }), }); expect(res.status).toBe(401); - expect(((await res.json()) as any).code).toBe('AUTH_REQUIRED'); + expect(((await res.json()) as any).error?.code).toBe('AUTH_REQUIRED'); }); it('(e) authenticated upload succeeds and sys_file.owner_id is server-stamped', async () => { @@ -319,13 +319,13 @@ describe('attachments permission matrix (#2755)', () => { // Anonymous → 401 (was a 200 capability URL before #2970). const anon = await stack.api(`/storage/files/${adminFile}/url`); expect(anon.status).toBe(401); - expect(((await anon.json()) as any).code).toBe('AUTH_REQUIRED'); + expect(((await anon.json()) as any).error?.code).toBe('AUTH_REQUIRED'); // memberB is authenticated but cannot read att_secret and is not the // owner → 403. const denied = await stack.apiAs(memberBTok, 'GET', `/storage/files/${adminFile}/url`); expect(denied.status).toBe(403); - expect(((await denied.json()) as any).code).toBe('ATTACHMENT_DOWNLOAD_DENIED'); + expect(((await denied.json()) as any).error?.code).toBe('ATTACHMENT_DOWNLOAD_DENIED'); // The owner (admin) → 200 with a signed URL. const owner = await stack.apiAs(adminTok, 'GET', `/storage/files/${adminFile}/url`); diff --git a/packages/runtime/src/http-dispatcher.test.ts b/packages/runtime/src/http-dispatcher.test.ts index 6634547ad4..3b5c5b9678 100644 --- a/packages/runtime/src/http-dispatcher.test.ts +++ b/packages/runtime/src/http-dispatcher.test.ts @@ -2,6 +2,7 @@ import { describe, it, expect, vi, beforeEach } from 'vitest'; import { HttpDispatcher } from './http-dispatcher.js'; import { ObjectKernel } from '@objectstack/core'; +import { ApiErrorSchema } from '@objectstack/spec/api'; describe('HttpDispatcher', () => { let kernel: ObjectKernel; @@ -1837,6 +1838,35 @@ describe('HttpDispatcher', () => { expect(result.response?.body?.error?.message).toBe('Missing locale parameter'); }); + /** + * The dispatcher's error body is the SHAPE the autonomously-mounted + * i18n/storage services were aligned to in #3675 — nested `error`, with + * the `success` flag. It is not yet the CONTRACT: `ApiErrorSchema` + * declares `code` as a semantic string ('validation_error'), and this + * emits the HTTP status as a number. The dispatcher already works + * around its own field being occupied — `this.error(msg, 403, { code: + * 'PERMISSION_DENIED' })` parks the real code in `details` — which is + * the tell that the number is in the wrong place. + * + * Pinned rather than fixed: `error.code` is read across the SDK, the + * console and the dogfood suite, so moving it is its own change. + * `toEqual(['code'])` is deliberately exact — if a second field starts + * deviating this fails, and if the dispatcher is fixed it also fails + * and this pin should be deleted. + */ + it('pins the ONE field where the dispatcher deviates from ApiErrorSchema (#3675)', async () => { + const result = await dispatcher.handleI18n('/translations', 'GET', {}, { request: {} }); + const body = result.response?.body as { success?: boolean; error?: unknown }; + + expect(body.success).toBe(false); + expect(typeof body.error).toBe('object'); + + const parsed = ApiErrorSchema.safeParse(body.error); + expect(parsed.success).toBe(false); + expect(parsed.error!.issues.map((i) => i.path.join('.'))).toEqual(['code']); + expect((body.error as { code: unknown }).code).toBe(400); + }); + it('should fallback to deriving labels from translations when getFieldLabels is missing', async () => { delete mockI18nService.getFieldLabels; mockI18nService.getTranslations.mockReturnValue({ diff --git a/packages/services/service-i18n/src/error-envelope.conformance.test.ts b/packages/services/service-i18n/src/error-envelope.conformance.test.ts new file mode 100644 index 0000000000..833653c58c --- /dev/null +++ b/packages/services/service-i18n/src/error-envelope.conformance.test.ts @@ -0,0 +1,192 @@ +// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * Error-envelope conformance for the autonomously-mounted `/api/v1/i18n/*` + * routes (#3675) — the error-path twin of the success-path fix in #3636. + * + * #3636 aligned the SUCCESS bodies because those were the ones breaking + * `ObjectStackClient.unwrapResponse`. The error bodies stayed a bare + * `{ error: '' }` while the dispatcher's `/i18n` domain — the OTHER + * provider of these same three shapes — emitted + * `{ success: false, error: { … } }` for the identical condition. Same SDK + * method, two error shapes, decided by which plugin mounted the route. + * + * Both directions are guarded: every error branch is driven and parsed against + * the real `BaseResponseSchema`, and the module source is scanned so a new + * route cannot quietly reintroduce the bare shape. + */ + +import { describe, it, expect, vi } from 'vitest'; +import { readFileSync } from 'node:fs'; +import { BaseResponseSchema } from '@objectstack/spec/api'; +import type { IHttpRequest, IHttpResponse, RouteHandler } from '@objectstack/spec/contracts'; +import { I18nServicePlugin } from './i18n-service-plugin'; + +const BASE = '/api/v1/i18n'; + +/** + * Mount the plugin's routes through its REAL lifecycle and hand back the + * handlers, so these assertions run against the routes the plugin actually + * registers rather than a hand-copied table. + */ +async function mount(i18nOverrides: Record = {}) { + const routes = new Map(); + const server = { + get: (p: string, h: RouteHandler) => { routes.set(`GET:${p}`, h); }, + post: vi.fn(), put: vi.fn(), delete: vi.fn(), patch: vi.fn(), use: vi.fn(), + listen: vi.fn().mockResolvedValue(undefined), + close: vi.fn().mockResolvedValue(undefined), + }; + const hooks = new Map Promise>>(); + const services = new Map(); + const ctx = { + registerService: vi.fn((name: string, svc: unknown) => { services.set(name, svc); }), + replaceService: vi.fn(), + getService: vi.fn((name: string) => { + if (name === 'http-server') return server; + if (services.has(name)) return services.get(name); + throw new Error(`Service '${name}' not found`); + }), + getServices: vi.fn(() => new Map()), + getKernel: vi.fn(), + hook: vi.fn((name: string, h: (...a: unknown[]) => Promise) => { + if (!hooks.has(name)) hooks.set(name, []); + hooks.get(name)!.push(h); + }), + trigger: vi.fn(async (name: string, ...a: unknown[]) => { + for (const h of hooks.get(name) ?? []) await h(...a); + }), + logger: { info: vi.fn(), warn: vi.fn(), error: vi.fn(), debug: vi.fn() }, + }; + + const plugin = new I18nServicePlugin(); + await plugin.init!(ctx as never); + // Sabotage the registered service where a test needs the `catch` arm — the + // routes close over whatever `init` registered. + const svc = services.get('i18n') as Record | undefined; + if (svc) Object.assign(svc, i18nOverrides); + await plugin.start!(ctx as never); + await ctx.trigger('kernel:ready'); + return routes; +} + +async function drive( + routes: Map, + path: string, + req: Partial = {}, +): Promise<{ status: number; body: any }> { + const handler = routes.get(`GET:${path}`); + if (!handler) throw new Error(`no handler for GET ${path} (have: ${[...routes.keys()].join(', ')})`); + const captured = { status: 200, body: undefined as any }; + const res: any = { + json(data: any) { captured.body = data; }, + send() {}, + status(code: number) { captured.status = code; return res; }, + header() { return res; }, + }; + await handler( + { params: {}, query: {}, body: undefined, headers: {}, method: 'GET', path, ...req } as IHttpRequest, + res as IHttpResponse, + ); + return captured; +} + +describe('i18n error envelope (#3675)', () => { + const CASES: Array<{ + name: string; + status: number; + code: string; + run: () => Promise<{ status: number; body: any }>; + }> = [ + { + name: 'translations without a locale', + status: 400, + code: 'INVALID_REQUEST', + run: async () => drive(await mount(), `${BASE}/translations/:locale`, { params: {} }), + }, + { + name: 'labels without an object or locale', + status: 400, + code: 'INVALID_REQUEST', + run: async () => drive(await mount(), `${BASE}/labels/:object/:locale`, { params: {} }), + }, + { + name: 'a throwing i18n service on /locales', + status: 500, + code: 'INTERNAL', + run: async () => + drive( + await mount({ getLocales: () => { throw new Error('adapter exploded'); } }), + `${BASE}/locales`, + ), + }, + { + name: 'a throwing i18n service on /translations/:locale', + status: 500, + code: 'INTERNAL', + run: async () => + drive( + await mount({ getTranslations: () => { throw new Error('adapter exploded'); } }), + `${BASE}/translations/:locale`, + { params: { locale: 'en' } }, + ), + }, + { + name: 'a throwing i18n service on /labels/:object/:locale', + status: 500, + code: 'INTERNAL', + run: async () => + drive( + await mount({ + getFieldLabels: () => { throw new Error('adapter exploded'); }, + getTranslations: () => { throw new Error('adapter exploded'); }, + }), + `${BASE}/labels/:object/:locale`, + { params: { object: 'customer', locale: 'en' } }, + ), + }, + ]; + + for (const c of CASES) { + it(`${c.name} → ${c.status} ${c.code}, in the declared envelope`, async () => { + const { status, body } = await c.run(); + expect(status).toBe(c.status); + + const parsed = BaseResponseSchema.safeParse(body); + expect(parsed.success, `body is not a BaseResponse: ${JSON.stringify(body)}`).toBe(true); + + expect(body.success).toBe(false); + expect(body.error.code).toBe(c.code); + expect(typeof body.error.message).toBe('string'); + expect(body.error.message.length).toBeGreaterThan(0); + + // The pre-#3675 shape, explicitly dead. + expect(typeof body.error).not.toBe('string'); + }); + } + + it('a 500 still carries a message when the thrown value has none', async () => { + // `error.message` is REQUIRED by ApiErrorSchema, so passing a raw thrown + // value straight through would emit an invalid body for anything that is + // not an Error. + const routes = await mount({ getLocales: () => { throw 'a bare string'; } }); + const { status, body } = await drive(routes, `${BASE}/locales`); + expect(status).toBe(500); + expect(BaseResponseSchema.safeParse(body).success).toBe(true); + expect(body.error.message).toBe('Internal error'); + }); + + it('routes every error through `sendError` — no route may reintroduce the bare shape', () => { + // Comments stripped first: this module's prose quotes both shapes, and a + // doc comment is not a code path. + const source = readFileSync(new URL('./i18n-service-plugin.ts', import.meta.url), 'utf8') + .replace(/\/\*[\s\S]*?\*\//g, '') + .replace(/\/\/[^\n]*/g, ''); + + const bare = [...source.matchAll(/res\s*\.\s*status\([^)]*\)\s*\.\s*json\(\s*\{\s*error/g)]; + expect(bare, `bare error bodies found: ${bare.map((m) => m[0]).join(', ')}`).toHaveLength(0); + + // The envelope is built in exactly one place. + expect([...source.matchAll(/success:\s*false/g)]).toHaveLength(1); + }); +}); diff --git a/packages/services/service-i18n/src/i18n-service-plugin.test.ts b/packages/services/service-i18n/src/i18n-service-plugin.test.ts index a3069c9891..2c7a0f2206 100644 --- a/packages/services/service-i18n/src/i18n-service-plugin.test.ts +++ b/packages/services/service-i18n/src/i18n-service-plugin.test.ts @@ -234,7 +234,10 @@ describe('I18nServicePlugin', () => { await handler(req, res); expect(res._status).toBe(400); - expect(res._data).toEqual({ error: 'Missing locale parameter' }); + expect(res._data).toEqual({ + success: false, + error: { code: 'INVALID_REQUEST', message: 'Missing locale parameter' }, + }); }); it('GET /labels/:object/:locale should derive field labels from translation bundle', async () => { @@ -266,7 +269,10 @@ describe('I18nServicePlugin', () => { await handler(req, res); expect(res._status).toBe(400); - expect(res._data).toEqual({ error: 'Missing object or locale parameter' }); + expect(res._data).toEqual({ + success: false, + error: { code: 'INVALID_REQUEST', message: 'Missing object or locale parameter' }, + }); }); }); diff --git a/packages/services/service-i18n/src/i18n-service-plugin.ts b/packages/services/service-i18n/src/i18n-service-plugin.ts index ae69b64475..36c7ac4892 100644 --- a/packages/services/service-i18n/src/i18n-service-plugin.ts +++ b/packages/services/service-i18n/src/i18n-service-plugin.ts @@ -7,6 +7,27 @@ import type { II18nService } from '@objectstack/spec/contracts'; import { FileI18nAdapter } from './file-i18n-adapter.js'; import type { FileI18nAdapterOptions } from './file-i18n-adapter.js'; +/** + * Emit an error in the DECLARED envelope — `BaseResponseSchema` + + * `ApiErrorSchema` (`packages/spec/src/api/contract.zod.ts`), i.e. + * `{ success: false, error: { code, message } }` with `code` a semantic + * STRING. + * + * These routes used to emit a bare `{ error: '' }`, so the same SDK + * method handed callers a string against this provider and an object against + * the dispatcher's `/i18n` domain — the error-path twin of the success-path + * asymmetry #3636 fixed (#3675). `ObjectStackClient` tolerates both shapes by + * sniffing, which is the consumer-side shim Prime Directive #12 says to + * delete by fixing the producer. + * + * Shape borrowed from the sibling services that already nest the error object + * (`settings-routes.ts`, `share-link-routes.ts`); the `success` flag is what + * those two still omit and the contract requires. + */ +function sendError(res: IHttpResponse, status: number, code: string, message: string): void { + res.status(status).json({ success: false, error: { code, message } }); +} + /** * Configuration options for the I18nServicePlugin. */ @@ -144,6 +165,11 @@ export class I18nServicePlugin implements Plugin { * returning the declared unwrapped shape against the dispatcher. Same * method, two shapes, decided by which plugin happened to mount the route * (#3636). `data` did not move, so direct body readers are unaffected. + * + * Error bodies now carry the matching `{ success: false, error: { code, + * message } }` half via `sendError` (#3675). That was left out of #3636 + * because only the success bodies broke `unwrapResponse`, and unlike the + * success fix it is NOT additive — `error` goes from string to object. */ private registerI18nRoutes(httpServer: IHttpServer, ctx: PluginContext): void { if (!this.i18n) return; @@ -167,7 +193,7 @@ export class I18nServicePlugin implements Plugin { }, }); } catch (error: any) { - res.status(500).json({ error: error.message }); + sendError(res, 500, 'INTERNAL', error?.message ?? 'Internal error'); } }); @@ -176,13 +202,13 @@ export class I18nServicePlugin implements Plugin { try { const locale = req.params.locale; if (!locale) { - res.status(400).json({ error: 'Missing locale parameter' }); + sendError(res, 400, 'INVALID_REQUEST', 'Missing locale parameter'); return; } const translations = i18n.getTranslations(locale); res.json({ success: true, data: { locale, translations } }); } catch (error: any) { - res.status(500).json({ error: error.message }); + sendError(res, 500, 'INTERNAL', error?.message ?? 'Internal error'); } }); @@ -192,7 +218,7 @@ export class I18nServicePlugin implements Plugin { const objectName = req.params.object; const locale = req.params.locale; if (!objectName || !locale) { - res.status(400).json({ error: 'Missing object or locale parameter' }); + sendError(res, 400, 'INVALID_REQUEST', 'Missing object or locale parameter'); return; } // Some implementations may provide a dedicated getFieldLabels method @@ -215,7 +241,7 @@ export class I18nServicePlugin implements Plugin { res.json({ success: true, data: { object: objectName, locale, labels } }); } } catch (error: any) { - res.status(500).json({ error: error.message }); + sendError(res, 500, 'INTERNAL', error?.message ?? 'Internal error'); } }); diff --git a/packages/services/service-storage/src/error-envelope.conformance.test.ts b/packages/services/service-storage/src/error-envelope.conformance.test.ts new file mode 100644 index 0000000000..a6411f9723 --- /dev/null +++ b/packages/services/service-storage/src/error-envelope.conformance.test.ts @@ -0,0 +1,283 @@ +// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * Error-envelope conformance for the autonomously-mounted `/api/v1/storage/*` + * routes (#3675). + * + * The route ledgers (#3563 → #3656) audit which routes EXIST and whether the + * SDK can address them. They say nothing about what comes back, which is how + * these routes carried green `sdk` rows while emitting a bare + * `{ error: '' }` — with the code, where one existed at all, as a + * SIBLING of `error` rather than a field of it — against a contract that + * declares `{ success: false, error: { code, message } }`. + * + * This file closes that gap for the error path, in two directions: + * 1. every distinct error branch is DRIVEN and its body parsed against the + * real `BaseResponseSchema` from `packages/spec` — not a local copy, so + * the assertion tracks the contract if the contract moves; + * 2. the module source is scanned so a NEW route cannot quietly reintroduce + * the bare shape. Without (2) this suite only ever covers the branches + * that existed the day it was written. + */ + +import { describe, it, expect } from 'vitest'; +import { readFileSync } from 'node:fs'; +import { promises as fs } from 'node:fs'; +import { join } from 'node:path'; +import { tmpdir } from 'node:os'; +import { BaseResponseSchema } from '@objectstack/spec/api'; +import type { IHttpRequest, IHttpResponse, RouteHandler } from '@objectstack/spec/contracts'; +import { LocalStorageAdapter } from './local-storage-adapter'; +import { StorageMetadataStore } from './metadata-store'; +import { registerStorageRoutes } from './storage-routes'; + +const BASE = '/api/v1/storage'; + +interface Captured { + status: number; + body: any; +} + +function mount(storage: any, store: any, routeOpts: any = {}) { + const routes = new Map(); + const http = { + get: (p: string, h: RouteHandler) => { routes.set(`GET:${p}`, h); }, + post: (p: string, h: RouteHandler) => { routes.set(`POST:${p}`, h); }, + put: (p: string, h: RouteHandler) => { routes.set(`PUT:${p}`, h); }, + delete: () => {}, + patch: () => {}, + use: () => {}, + listen: async () => {}, + close: async () => {}, + }; + registerStorageRoutes(http as any, storage, store, { basePath: BASE, ...routeOpts }); + return routes; +} + +async function drive( + routes: Map, + method: string, + path: string, + req: Partial = {}, +): Promise { + const handler = routes.get(`${method}:${path}`); + if (!handler) throw new Error(`no handler for ${method} ${path}`); + const captured: Captured = { status: 200, body: undefined }; + const res: any = { + json(data: any) { captured.body = data; }, + send() {}, + status(code: number) { captured.status = code; return res; }, + header() { return res; }, + }; + await handler( + { params: {}, query: {}, body: undefined, headers: {}, method, path, ...req } as IHttpRequest, + res as IHttpResponse, + ); + return captured; +} + +/** A store whose every read throws — drives the `catch` arms. */ +const explodingStore = { + getFile: async () => { throw new Error('boom'); }, + getSession: async () => { throw new Error('boom'); }, + createFile: async () => { throw new Error('boom'); }, +} as any; + +async function tmpAdapter(): Promise { + const rootDir = join(tmpdir(), `os-err-env-${Date.now()}-${Math.random().toString(36).slice(2)}`); + await fs.mkdir(rootDir, { recursive: true }); + return new LocalStorageAdapter({ rootDir, signingSecret: 'test-secret' }); +} + +async function committedAttachment(store: StorageMetadataStore, id: string, extra: any = {}) { + await store.createFile({ + id, + key: `attachments/${id}.bin`, + name: 'x.bin', + status: 'committed', + acl: 'private', + scope: 'attachments', + ...extra, + } as any); +} + +describe('storage error envelope (#3675)', () => { + /** + * One entry per distinct `code` the routes can emit. `expected` is asserted + * exactly, so retiring a code without retiring its case fails here rather + * than silently shrinking coverage. + */ + const CASES: Array<{ name: string; status: number; code: string; run: () => Promise }> = [ + { + name: 'missing required upload fields', + status: 400, + code: 'INVALID_REQUEST', + run: async () => { + const routes = mount(await tmpAdapter(), new StorageMetadataStore(null)); + return drive(routes, 'POST', `${BASE}/upload/presigned`, { body: {} }); + }, + }, + { + name: 'completing an upload for an unknown file', + status: 404, + code: 'FILE_NOT_FOUND', + run: async () => { + const routes = mount(await tmpAdapter(), new StorageMetadataStore(null)); + return drive(routes, 'POST', `${BASE}/upload/complete`, { body: { fileId: 'nope' } }); + }, + }, + { + name: 'completing an unknown chunked session', + status: 404, + code: 'UPLOAD_SESSION_NOT_FOUND', + run: async () => { + const routes = mount(await tmpAdapter(), new StorageMetadataStore(null)); + return drive(routes, 'POST', `${BASE}/upload/chunked/:uploadId/complete`, { + params: { uploadId: 'nope' }, + }); + }, + }, + { + name: 'chunk upload with the wrong resume token', + status: 403, + code: 'INVALID_RESUME_TOKEN', + run: async () => { + const store = new StorageMetadataStore(null); + await store.createSession({ + id: 'sess-1', + file_id: 'f-1', + key: 'user/f-1.bin', + filename: 'f.bin', + mime_type: 'application/octet-stream', + total_size: 10, + chunk_size: 5, + total_chunks: 2, + resume_token: 'the-real-token', + status: 'in_progress', + } as any); + const routes = mount(await tmpAdapter(), store); + return drive(routes, 'PUT', `${BASE}/upload/chunked/:uploadId/chunk/:chunkIndex`, { + params: { uploadId: 'sess-1', chunkIndex: '0' }, + headers: { 'x-resume-token': 'wrong' }, + }); + }, + }, + { + name: 'anonymous upload when a session resolver is wired', + status: 401, + code: 'AUTH_REQUIRED', + run: async () => { + const routes = mount(await tmpAdapter(), new StorageMetadataStore(null), { + resolveSession: async () => null, + }); + return drive(routes, 'POST', `${BASE}/upload/presigned`, { body: {} }); + }, + }, + { + name: 'denied download of an attachments-scope file', + status: 403, + code: 'ATTACHMENT_DOWNLOAD_DENIED', + run: async () => { + const store = new StorageMetadataStore(null); + await committedAttachment(store, 'a1'); + const routes = mount(await tmpAdapter(), store, { authorizeFileRead: async () => 'deny' }); + return drive(routes, 'GET', `${BASE}/files/:fileId/url`, { params: { fileId: 'a1' } }); + }, + }, + { + name: 'denied download of a field-owned file', + status: 403, + code: 'FILE_DOWNLOAD_DENIED', + run: async () => { + const store = new StorageMetadataStore(null); + await committedAttachment(store, 'fo1', { + scope: 'user', + key: 'user/fo1.png', + ref_object: 'product', + ref_id: 'p1', + }); + const routes = mount(await tmpAdapter(), store, { authorizeFileRead: async () => 'deny' }); + return drive(routes, 'GET', `${BASE}/files/:fileId/url`, { params: { fileId: 'fo1' } }); + }, + }, + { + name: 'unauthenticated download of an attachments-scope file', + status: 401, + code: 'AUTH_REQUIRED', + run: async () => { + const store = new StorageMetadataStore(null); + await committedAttachment(store, 'a2'); + const routes = mount(await tmpAdapter(), store, { + authorizeFileRead: async () => 'unauthenticated', + }); + return drive(routes, 'GET', `${BASE}/files/:fileId/url`, { params: { fileId: 'a2' } }); + }, + }, + { + name: 'raw upload against an adapter with no token support', + status: 501, + code: 'NOT_IMPLEMENTED', + run: async () => { + const routes = mount({} as any, new StorageMetadataStore(null)); + return drive(routes, 'PUT', `${BASE}/_local/raw/:token`, { params: { token: 'x' } }); + }, + }, + { + name: 'raw download with a forged token signature', + status: 403, + code: 'INVALID_TOKEN', + run: async () => { + const routes = mount(await tmpAdapter(), new StorageMetadataStore(null)); + return drive(routes, 'GET', `${BASE}/_local/raw/:token`, { params: { token: 'YWJj.ZGVm' } }); + }, + }, + { + name: 'an unexpected throw from the metadata store', + status: 500, + code: 'INTERNAL', + run: async () => { + const routes = mount(await tmpAdapter(), explodingStore); + return drive(routes, 'GET', `${BASE}/files/:fileId/url`, { params: { fileId: 'whatever' } }); + }, + }, + ]; + + for (const c of CASES) { + it(`${c.name} → ${c.status} ${c.code}, in the declared envelope`, async () => { + const { status, body } = await c.run(); + expect(status).toBe(c.status); + + // The contract itself, imported — not a restatement of it. + const parsed = BaseResponseSchema.safeParse(body); + expect(parsed.success, `body is not a BaseResponse: ${JSON.stringify(body)}`).toBe(true); + + expect(body.success).toBe(false); + expect(body.error.code).toBe(c.code); + expect(typeof body.error.message).toBe('string'); + expect(body.error.message.length).toBeGreaterThan(0); + + // The pre-#3675 shapes, explicitly dead: `error` is no longer a bare + // string, and the code is no longer a SIBLING of `error`. + expect(typeof body.error).not.toBe('string'); + expect(body.code).toBeUndefined(); + }); + } + + it('routes every error through `sendError` — no route may reintroduce the bare shape', () => { + // Comments stripped first: this file's own prose quotes both the old and + // the new shape, and a doc comment is not a code path. + const source = readFileSync(new URL('./storage-routes.ts', import.meta.url), 'utf8') + .replace(/\/\*[\s\S]*?\*\//g, '') + .replace(/\/\/[^\n]*/g, ''); + + // `res.status(…).json({ error…` was the bare-shape idiom. Any reappearance + // means a new route bypassed the helper. + const bare = [...source.matchAll(/res\s*\.\s*status\([^)]*\)\s*\.\s*json\(\s*\{\s*error/g)]; + expect(bare, `bare error bodies found: ${bare.map((m) => m[0]).join(', ')}`).toHaveLength(0); + + // And the helper is the only place that builds one, so the shape lives in + // exactly one line of this package. + const builders = [...source.matchAll(/success:\s*false/g)]; + expect(builders).toHaveLength(1); + }); +}); diff --git a/packages/services/service-storage/src/storage-routes.test.ts b/packages/services/service-storage/src/storage-routes.test.ts index e63ddf923e..408ca9ce66 100644 --- a/packages/services/service-storage/src/storage-routes.test.ts +++ b/packages/services/service-storage/src/storage-routes.test.ts @@ -291,7 +291,7 @@ describe('Storage REST Routes', () => { for (const p of ['/api/v1/storage/files/:fileId/url', '/api/v1/storage/files/:fileId']) { const res = await hit(server, p, 'a1'); expect(res._status, p).toBe(401); - expect(res._json?.code, p).toBe('AUTH_REQUIRED'); + expect(res._json?.error?.code, p).toBe('AUTH_REQUIRED'); } }); @@ -300,7 +300,7 @@ describe('Storage REST Routes', () => { await commit(s, { id: 'a2' }); const res = await hit(server, '/api/v1/storage/files/:fileId/url', 'a2'); expect(res._status).toBe(403); - expect(res._json?.code).toBe('ATTACHMENT_DOWNLOAD_DENIED'); + expect(res._json?.error?.code).toBe('ATTACHMENT_DOWNLOAD_DENIED'); }); it('allows the download when authorized, minting a short-lived signed URL', async () => { @@ -354,7 +354,7 @@ describe('Storage REST Routes', () => { expect(res._status).toBe(403); // Distinct from the attachments code — this file BELONGS to one // record, it is not attached to several. - expect(res._json?.code).toBe('FILE_DOWNLOAD_DENIED'); + expect(res._json?.error?.code).toBe('FILE_DOWNLOAD_DENIED'); expect(authorizeFileRead).toHaveBeenCalledOnce(); }); @@ -363,7 +363,7 @@ describe('Storage REST Routes', () => { await commit(s, owned('fo2')); const res = await hit(server, '/api/v1/storage/files/:fileId', 'fo2'); expect(res._status).toBe(401); - expect(res._json?.code).toBe('AUTH_REQUIRED'); + expect(res._json?.error?.code).toBe('AUTH_REQUIRED'); }); it('allows an authorized field-owned download', async () => { @@ -466,7 +466,7 @@ describe('Storage REST Routes', () => { const res = createMockRes(); await server._getHandler(method, path)!(createMockReq(reqOpts as any), res); expect(res._status, `${method} ${path}`).toBe(401); - expect(res._json?.code, `${method} ${path}`).toBe('AUTH_REQUIRED'); + expect(res._json?.error?.code, `${method} ${path}`).toBe('AUTH_REQUIRED'); } }); diff --git a/packages/services/service-storage/src/storage-routes.ts b/packages/services/service-storage/src/storage-routes.ts index a57db99528..4ca9ce7912 100644 --- a/packages/services/service-storage/src/storage-routes.ts +++ b/packages/services/service-storage/src/storage-routes.ts @@ -9,6 +9,28 @@ import { contentDispositionValue } from './content-disposition.js'; /** Authorization verdict for an attachments-scope download (#2970 item 2). */ export type FileReadVerdict = 'allow' | 'deny' | 'unauthenticated'; +/** + * Emit an error in the DECLARED envelope — `BaseResponseSchema` + + * `ApiErrorSchema` (`packages/spec/src/api/contract.zod.ts`), i.e. + * `{ success: false, error: { code, message } }` with `code` a semantic + * STRING. + * + * These routes used to emit a bare `{ error: '' }`, with the code — + * where one existed at all — as a SIBLING of `error` rather than a field of + * it. So a caller reading `body.error.message` got `undefined` here and the + * real message from the dispatcher (#3675). `ObjectStackClient` papers over + * the difference by sniffing four shapes; that shim is the consumer-side + * symptom Prime Directive #12 says to cure at the producer. + * + * Only the ERROR path is normalized here. The success bodies are still three + * shapes of their own (`{ data }`, bare `{ url }`, `{ ok, key }`, none with + * `success: true`) — a separate, non-additive change, tracked as its own + * issue rather than smuggled into this one. + */ +function sendError(res: IHttpResponse, status: number, code: string, message: string): void { + res.status(status).json({ success: false, error: { code, message } }); +} + /** * Options for the storage route registration helper. */ @@ -117,21 +139,15 @@ export function registerStorageRoutes( verdict = 'deny'; // a failed authz check must never fall open } if (verdict === 'unauthenticated') { - res.status(401).json({ error: 'Authentication required to download this file', code: 'AUTH_REQUIRED' }); + sendError(res, 401, 'AUTH_REQUIRED', 'Authentication required to download this file'); return false; } if (verdict === 'deny') { - res.status(403).json( - fieldOwned - ? { - error: 'You do not have access to the record this file belongs to', - code: 'FILE_DOWNLOAD_DENIED', - } - : { - error: 'You do not have access to a record this file is attached to', - code: 'ATTACHMENT_DOWNLOAD_DENIED', - }, - ); + if (fieldOwned) { + sendError(res, 403, 'FILE_DOWNLOAD_DENIED', 'You do not have access to the record this file belongs to'); + } else { + sendError(res, 403, 'ATTACHMENT_DOWNLOAD_DENIED', 'You do not have access to a record this file is attached to'); + } return false; } return downloadTtl; @@ -161,7 +177,7 @@ export function registerStorageRoutes( session = null; } if (!session?.userId) { - res.status(401).json({ error: 'Authentication required to upload files', code: 'AUTH_REQUIRED' }); + sendError(res, 401, 'AUTH_REQUIRED', 'Authentication required to upload files'); return false; } return session; @@ -176,7 +192,7 @@ export function registerStorageRoutes( if (session === false) return; const { filename, mimeType, size, scope, bucket } = req.body ?? {}; if (!filename || !mimeType || size == null) { - res.status(400).json({ error: 'filename, mimeType, and size are required' }); + sendError(res, 400, 'INVALID_REQUEST', 'filename, mimeType, and size are required'); return; } @@ -225,7 +241,7 @@ export function registerStorageRoutes( }, }); } catch (err: any) { - res.status(500).json({ error: err.message ?? 'Internal error' }); + sendError(res, 500, 'INTERNAL', err?.message ?? 'Internal error'); } }); @@ -237,13 +253,13 @@ export function registerStorageRoutes( if ((await requireUploadSession(req, res)) === false) return; const { fileId, eTag } = req.body ?? {}; if (!fileId) { - res.status(400).json({ error: 'fileId is required' }); + sendError(res, 400, 'INVALID_REQUEST', 'fileId is required'); return; } const file = await store.getFile(fileId); if (!file) { - res.status(404).json({ error: 'File not found' }); + sendError(res, 404, 'FILE_NOT_FOUND', 'File not found'); return; } @@ -268,7 +284,7 @@ export function registerStorageRoutes( }, }); } catch (err: any) { - res.status(500).json({ error: err.message ?? 'Internal error' }); + sendError(res, 500, 'INTERNAL', err?.message ?? 'Internal error'); } }); @@ -281,7 +297,7 @@ export function registerStorageRoutes( if (session === false) return; const { filename, mimeType, totalSize, chunkSize: reqChunkSize, scope, bucket, metadata } = req.body ?? {}; if (!filename || !mimeType || !totalSize) { - res.status(400).json({ error: 'filename, mimeType, and totalSize are required' }); + sendError(res, 400, 'INVALID_REQUEST', 'filename, mimeType, and totalSize are required'); return; } @@ -349,7 +365,7 @@ export function registerStorageRoutes( }, }); } catch (err: any) { - res.status(500).json({ error: err.message ?? 'Internal error' }); + sendError(res, 500, 'INTERNAL', err?.message ?? 'Internal error'); } }); @@ -362,20 +378,20 @@ export function registerStorageRoutes( const { uploadId, chunkIndex: chunkIndexStr } = req.params; const chunkIndex = parseInt(chunkIndexStr, 10); if (!uploadId || isNaN(chunkIndex)) { - res.status(400).json({ error: 'uploadId and chunkIndex are required' }); + sendError(res, 400, 'INVALID_REQUEST', 'uploadId and chunkIndex are required'); return; } const session = await store.getSession(uploadId); if (!session) { - res.status(404).json({ error: 'Upload session not found' }); + sendError(res, 404, 'UPLOAD_SESSION_NOT_FOUND', 'Upload session not found'); return; } // Verify resume token const token = (req.headers['x-resume-token'] ?? '') as string; if (session.resume_token && token !== session.resume_token) { - res.status(403).json({ error: 'Invalid resume token' }); + sendError(res, 403, 'INVALID_RESUME_TOKEN', 'Invalid resume token'); return; } @@ -388,7 +404,7 @@ export function registerStorageRoutes( } else if (req.body instanceof ArrayBuffer) { data = Buffer.from(req.body); } else { - res.status(400).json({ error: 'Binary body required' }); + sendError(res, 400, 'INVALID_REQUEST', 'Binary body required'); return; } @@ -417,7 +433,7 @@ export function registerStorageRoutes( }, }); } catch (err: any) { - res.status(500).json({ error: err.message ?? 'Internal error' }); + sendError(res, 500, 'INTERNAL', err?.message ?? 'Internal error'); } }); @@ -430,7 +446,7 @@ export function registerStorageRoutes( const { uploadId } = req.params; const session = await store.getSession(uploadId); if (!session) { - res.status(404).json({ error: 'Upload session not found' }); + sendError(res, 404, 'UPLOAD_SESSION_NOT_FOUND', 'Upload session not found'); return; } @@ -461,7 +477,7 @@ export function registerStorageRoutes( }, }); } catch (err: any) { - res.status(500).json({ error: err.message ?? 'Internal error' }); + sendError(res, 500, 'INTERNAL', err?.message ?? 'Internal error'); } }); @@ -474,7 +490,7 @@ export function registerStorageRoutes( const { uploadId } = req.params; const session = await store.getSession(uploadId); if (!session) { - res.status(404).json({ error: 'Upload session not found' }); + sendError(res, 404, 'UPLOAD_SESSION_NOT_FOUND', 'Upload session not found'); return; } @@ -500,7 +516,7 @@ export function registerStorageRoutes( }, }); } catch (err: any) { - res.status(500).json({ error: err.message ?? 'Internal error' }); + sendError(res, 500, 'INTERNAL', err?.message ?? 'Internal error'); } }); @@ -512,7 +528,7 @@ export function registerStorageRoutes( const { fileId } = req.params; const file = await store.getFile(fileId); if (!file || file.status !== 'committed') { - res.status(404).json({ error: 'File not found or not committed' }); + sendError(res, 404, 'FILE_NOT_FOUND', 'File not found or not committed'); return; } @@ -532,7 +548,7 @@ export function registerStorageRoutes( res.json({ url }); } catch (err: any) { - res.status(500).json({ error: err.message ?? 'Internal error' }); + sendError(res, 500, 'INTERNAL', err?.message ?? 'Internal error'); } }); @@ -552,7 +568,7 @@ export function registerStorageRoutes( const { fileId } = req.params; const file = await store.getFile(fileId); if (!file || file.status !== 'committed') { - res.status(404).json({ error: 'File not found or not committed' }); + sendError(res, 404, 'FILE_NOT_FOUND', 'File not found or not committed'); return; } @@ -572,7 +588,7 @@ export function registerStorageRoutes( res.status(302).header('Location', url).send(''); } catch (err: any) { - res.status(500).json({ error: err.message ?? 'Internal error' }); + sendError(res, 500, 'INTERNAL', err?.message ?? 'Internal error'); } }); @@ -584,7 +600,7 @@ export function registerStorageRoutes( const { token } = req.params; const localAdapter = storage as LocalStorageAdapter; if (!localAdapter.verifyToken) { - res.status(501).json({ error: 'Presigned raw upload not supported by this adapter' }); + sendError(res, 501, 'NOT_IMPLEMENTED', 'Presigned raw upload not supported by this adapter'); return; } @@ -595,15 +611,20 @@ export function registerStorageRoutes( } else if (Buffer.isBuffer(req.body)) { data = req.body; } else { - res.status(400).json({ error: 'Binary body required' }); + sendError(res, 400, 'INVALID_REQUEST', 'Binary body required'); return; } await storage.upload(payload.k, data, { contentType: payload.ct }); res.json({ ok: true, key: payload.k }); } catch (err: any) { - const statusCode = err.message?.includes('expired') || err.message?.includes('signature') ? 403 : 500; - res.status(statusCode).json({ error: err.message ?? 'Upload failed' }); + const invalidToken = err?.message?.includes('expired') || err?.message?.includes('signature'); + sendError( + res, + invalidToken ? 403 : 500, + invalidToken ? 'INVALID_TOKEN' : 'INTERNAL', + err?.message ?? 'Upload failed', + ); } }); @@ -615,7 +636,7 @@ export function registerStorageRoutes( const { token } = req.params; const localAdapter = storage as LocalStorageAdapter; if (!localAdapter.verifyToken) { - res.status(501).json({ error: 'Presigned download not supported by this adapter' }); + sendError(res, 501, 'NOT_IMPLEMENTED', 'Presigned download not supported by this adapter'); return; } @@ -631,8 +652,13 @@ export function registerStorageRoutes( } res.send(data); } catch (err: any) { - const statusCode = err.message?.includes('expired') || err.message?.includes('signature') ? 403 : 500; - res.status(statusCode).json({ error: err.message ?? 'Download failed' }); + const invalidToken = err?.message?.includes('expired') || err?.message?.includes('signature'); + sendError( + res, + invalidToken ? 403 : 500, + invalidToken ? 'INVALID_TOKEN' : 'INTERNAL', + err?.message ?? 'Download failed', + ); } }); }