diff --git a/.changeset/rest-production-exceljs-typing.md b/.changeset/rest-production-exceljs-typing.md new file mode 100644 index 0000000000..6be54c46a8 --- /dev/null +++ b/.changeset/rest-production-exceljs-typing.md @@ -0,0 +1,32 @@ +--- +"@objectstack/rest": patch +--- + +fix(rest): type the two production exceljs dynamic imports behind one named accessor + +Both places `@objectstack/rest` production source reached exceljs bound the +module as `const ExcelJS: any`, so `parseXlsxToRows` (the whole .xlsx import +path) and `createXlsxStream` (the streaming .xlsx export path) built workbooks, +read worksheets, iterated rows and read cells through a value tsc knew nothing +about. A misspelled method, a wrong argument arity or a property exceljs +renamed was not a compile error — it surfaced, if at all, as a runtime fault in +a deployed import or export. + +`src/xlsx-module.ts` is now the single binding site both paths share, and it +keeps the load lazy: everything in it is either a type (erased at emit) or +inside the async accessor, so a CSV or JSON import still never pays to load +exceljs. + +**Typing-only — no runtime behaviour change, and no change to the published API +surface.** `parseXlsxToRows` keeps its exact signature +(`(buffer: Buffer | ArrayBuffer, mapping?, sheet?)`), the package's exports are +unchanged, and the accessor is internal. The interop expression is the one the +call sites already ran (awaited once rather than twice — the second +`await import('exceljs')` resolved from the module cache to the identical +record), and the non-array shape of `Row.values` iterates zero times before and +after. `@objectstack/rest`'s 160 test files / 2698 tests pass unchanged. + +This is a `patch` rather than a `skip-changeset` because it was measured to +reach the published artifact: `loadExcelJs` greps 3 in `dist/index.js` and 3 in +`dist/index.cjs` (`asXlsxLoadInput` 2 and 2), against a positive control of +`RestServer` = 36. diff --git a/content/docs/permissions/system-context.mdx b/content/docs/permissions/system-context.mdx index 02f78b850d..7bacc53996 100644 --- a/content/docs/permissions/system-context.mdx +++ b/content/docs/permissions/system-context.mdx @@ -64,7 +64,7 @@ not on any flag. ## How the flag is set `isSystem` is **server-constructed and never client-supplied**. Inbound HTTP -cannot set it (`packages/rest/src/rest-server.ts:1234`, `:1263`), and neither +cannot set it (`packages/rest/src/rest-server.ts:1235`, `:1264`), and neither can an action body (`packages/runtime/src/domains/actions.ts:404`). It is written by internal callers only, as an option on the engine call: @@ -103,7 +103,7 @@ that silently does not happen. | 14 | MCP stdio bridge skips the object API-exposure gate | mcp | Get: the bridge reaches objects whose `apiEnabled` / `apiMethods` would refuse an external caller | `stdio-data-bridge.ts:246` | | 15 | **Read-audit rows are not written** | plugin-audit | Lose: the "a person opened this record" trail. `sudo()` keeps the caller's `userId`, so this flag is the only thing separating a human read from a platform one | `read-audit.ts:556` | | 16 | Approval snapshot payload redaction skipped | plugin-approvals | Get: the whole snapshot on `find` / `findOne` — the audit/replay channel. Lose: field-visibility redaction over approval payloads | `payload-redaction-middleware.ts:115` | -| 17 | REST anonymous-deny seam satisfied | rest | Get: `enforceAuth` passes with no `userId`. Not reachable from the wire — `isSystem` is never set on an inbound request | `rest-server.ts:1266` | +| 17 | REST anonymous-deny seam satisfied | rest | Get: `enforceAuth` passes with no `userId`. Not reachable from the wire — `isSystem` is never set on an inbound request | `rest-server.ts:1267` | ### 2. Write pipeline and data integrity @@ -158,7 +158,7 @@ The largest single consumer — **20 of the 109 sites**. |:--|:---|:---|:---|:---| | 48 | Object API-exposure gate bypassed (`apiEnabled` / `apiMethods`) | runtime | Get: internal self-writes ignore exposure declarations — these govern **external** exposure, not engine self-writes | `action-execution.ts:136` | | 49 | Action `requiredPermissions` bypassed | runtime | Get: engine self-invocation runs any action | `action-execution.ts:399` | -| 50 | `manage_metadata` bypassed on metadata writes | runtime, rest | Get: schema writes without the capability | `domains/meta.ts:471`, `:874`, `rest-server.ts:4270`, `:5633`, `:5865`, `:6210`, `:6403` | +| 50 | `manage_metadata` bypassed on metadata writes | runtime, rest | Get: schema writes without the capability | `domains/meta.ts:471`, `:874`, `rest-server.ts:4271`, `:5634`, `:5866`, `:6211`, `:6404` | | 51 | The shared metadata-write verdict itself returns `allowed` | metadata-core | Get: the one function all of row 50's doors consult answers yes before any capability is examined | `meta-write-capability.ts:134` | | 52 | Anonymous-deny seam satisfied on the domain dispatchers and the package/federation routes | runtime, rest | Get: passes with no `userId` | `domains/actions.ts:411`, `domains/ai.ts:60`, `domains/automation.ts:982`, `domains/meta.ts:232`, `domains/security.ts:78`, `domains/packages.ts:246`, `external-datasource-routes.ts:302`, `package-routes.ts:92` | | 53 | MCP principal check satisfied | runtime | Get: MCP surface reachable with no user | `domains/mcp.ts:61` | @@ -199,7 +199,7 @@ assuming `isSystem` covers it is a documented source of bugs. | "It preserves a supplied `updated_at` / `updated_by`" | **No.** That is `preserveAudit`, a separate opt-in — and an UPDATE-path exemption only | `field.zod.ts:1451` (#3493 / #6640) | | "It stamps `created_by`" | **No.** Audit stamping reads `userId` from the context. A user-less system write stamps nothing — that is today's behaviour, not an error | `runtime-identity.ts:280`–`281` | | "It bypasses every guard" | **No.** The last-admin guard applies to **every** context, `isSystem` included — the deprovision path that actually locks an org out is the system one | `last-admin-guard.ts:273` | -| "A client can request it" | **No.** Never settable from inbound HTTP or from an action body | `rest-server.ts:1234`, `:1263`; `domains/actions.ts:404` | +| "A client can request it" | **No.** Never settable from inbound HTTP or from an action body | `rest-server.ts:1235`, `:1264`; `domains/actions.ts:404` | --- diff --git a/packages/rest/src/import-prepare.ts b/packages/rest/src/import-prepare.ts index d336c25e5e..99a5651692 100644 --- a/packages/rest/src/import-prepare.ts +++ b/packages/rest/src/import-prepare.ts @@ -13,6 +13,7 @@ import { type ExportFieldMeta, } from './export-format.js'; import { resolveNamedMapping, applyMappingToRows, type MappingArtifactLike } from './import-mapping.js'; +import { asXlsxLoadInput, loadExcelJs } from './xlsx-module.js'; /** * Minimal RFC-4180-style CSV parser used by the bulk-import endpoint @@ -138,15 +139,23 @@ export async function parseXlsxToRows( mapping: Record = {}, sheet?: string | number, ): Promise>> { - const ExcelJS: any = (await import('exceljs')).default ?? (await import('exceljs')); + const ExcelJS = await loadExcelJs(); const wb = new ExcelJS.Workbook(); - await wb.xlsx.load(buffer); - const ws = sheet !== undefined ? wb.getWorksheet(sheet as any) : wb.worksheets[0]; + // The assertion is on the Node `Buffer` arm ONLY. `ArrayBuffer` is already + // assignable to the module-local shim exceljs declares for this parameter, + // so that arm reaches `load` unasserted and stays checked; asserting the + // whole union would switch that check off. See `xlsx-module.ts`. + await wb.xlsx.load(buffer instanceof ArrayBuffer ? buffer : asXlsxLoadInput(buffer)); + const ws = sheet !== undefined ? wb.getWorksheet(sheet) : wb.worksheets[0]; if (!ws) return []; const cells: string[][] = []; - ws.eachRow({ includeEmpty: false }, (row: any) => { - const values = row.values as any[]; // 1-based; index 0 is unused + ws.eachRow({ includeEmpty: false }, (row) => { + // exceljs declares `Row.values` as the 1-based array form (index 0 + // unused) OR a keyed object; a workbook parsed from bytes yields the + // array. The keyed shape iterates zero times either way — its `.length` + // was `undefined` before, so the loop below never ran for it. + const values = Array.isArray(row.values) ? row.values : []; const line: string[] = []; for (let c = 1; c < values.length; c++) line.push(xlsxCellToString(values[c])); cells.push(line); diff --git a/packages/rest/src/package-door-declared-code.test.ts b/packages/rest/src/package-door-declared-code.test.ts index 88517682bf..1b776bc867 100644 --- a/packages/rest/src/package-door-declared-code.test.ts +++ b/packages/rest/src/package-door-declared-code.test.ts @@ -100,6 +100,38 @@ * itself and the handler's `try` catches it) — and the production wrapper * above has no statement that can make one. * + * ⭐ WHICH `.catch` is load-bearing: NEITHER of the two named above. A + * production fault never arrives at either one AS A REJECTION. + * `computeExecCtx` — the private body behind `resolveExecCtx` — wraps its + * WHOLE body in `try { … } catch { return undefined; }`, so a faulting + * resolve FULFILS with `undefined` rather than rejecting, and the + * fault-to-anonymous conversion has already happened one level BELOW by the + * time either `.catch(() => undefined)` is reached. ⇒ removing either of them + * would not by itself surface a production fault: a reader who deletes one + * expecting a 5xx to appear measures no change and concludes the wrong thing + * about where the conversion lives. That is the whole reason this paragraph + * exists. + * + * ⚠️ And that first net is NOT per-door, unlike the two above it. + * `resolveExecCtx(environmentId, req).catch(() => undefined)` is written + * per-consumer — 16 times in `rest-server.ts` as this file is written — while + * `computeExecCtx`'s catch is ONE site whose conversion every one of those + * consumers inherits. So what this door reads is not a property OF this door, + * and a census of the other consumers is a separate file rather than a + * section here. + * + * ⛔ Both facts are MEASURED ELSEWHERE and cited here rather than restated — + * a second copy is a second thing to drift, and this file's own convention is + * to state a reason once and point at it: + * - `package-door-execctx-fault-reading.test.ts` — that the production + * supplier really does fulfil rather than reject, read per class. + * - `package-door-execctx-fault-reachability.test.ts` — that no degraded + * class reaches the wrapper as a rejection, against a control showing the + * witness CAN report one. + * - `execctx-consumer-census.test.ts` — the per-site census over every + * `resolveExecCtx` consumer, each row DRIVEN rather than read off the + * shape. + * * ⚠️ Nor can an embedder reach it: `registerPackageRoutes` and * `PackageRoutesOptions` are NOT exported from `packages/rest/src/index.ts` * (the package publishes a single `.` entry, and `direct-mount-composition.ts` @@ -116,8 +148,11 @@ * ⛔ Whether that double swallow SHOULD exist at all is a different * question — it would change what a public door emits — and it is open * at #12537, deliberately not answered here. Note for whoever takes it: the - * swallow is documented at NEITHER site, so "deliberate" is not established by - * the code as it stands. + * swallow is now documented at ONE of the two sites, not neither — + * `rest-server.ts`'s `resolvePackageRouteExecutionContext` carries the + * reading, the second-net correction above included — while the + * `package-routes.ts` site still carries none. So "deliberate" is established + * by the code for the first site and still is not for the second. * * Section 5 is the second fact stated as a test: a REAL `ObjectQL`, a REAL * `ObjectStackProtocolImplementation` and a failing driver, driven through the diff --git a/packages/rest/src/rest-server.ts b/packages/rest/src/rest-server.ts index b9809d0e52..2f63997669 100644 --- a/packages/rest/src/rest-server.ts +++ b/packages/rest/src/rest-server.ts @@ -194,6 +194,7 @@ import { } from './export-format.js'; import { runImport } from './import-runner.js'; import { prepareImportRequest } from './import-prepare.js'; +import { loadExcelJs, type Worksheet } from './xlsx-module.js'; import { enrichOpenApiWithEndpoints } from './openapi-endpoints.js'; import { buildBuiltinPaths } from './openapi-builtin-paths.js'; import { @@ -605,11 +606,11 @@ function rowsToCsv( * module's static graph. */ async function createXlsxStream(res: any, useStyles = false): Promise<{ - ws: any; + ws: Worksheet; finalize: () => Promise; }> { const { PassThrough } = await import('node:stream'); - const ExcelJS: any = (await import('exceljs')).default ?? (await import('exceljs')); + const ExcelJS = await loadExcelJs(); const passthrough = new PassThrough(); const done = new Promise((resolve, reject) => { diff --git a/packages/rest/src/xlsx-module.test.ts b/packages/rest/src/xlsx-module.test.ts new file mode 100644 index 0000000000..161fdac770 --- /dev/null +++ b/packages/rest/src/xlsx-module.test.ts @@ -0,0 +1,85 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * Runtime pins for `xlsx-module.ts` — the single typed, lazily-loaded exceljs + * binding both production xlsx paths share. + * + * The TYPE half of that module (workbooks, worksheets, rows and cells are now + * inside tsc's reach) is asserted by tsc itself and cannot be asserted from + * here. What CAN rot at runtime is the part that is not a type: + * + * - the CommonJS interop (`.default ?? namespace`) the accessor performs. A + * "simplification" that drops either half returns a namespace whose + * `Workbook` is `undefined`, and the failure surfaces only in a deployed + * import or export. + * - both arms of `parseXlsxToRows(buffer: Buffer | ArrayBuffer, …)`. Only the + * Node `Buffer` arm carries the type assertion; the `ArrayBuffer` arm is + * type-checked as it stands. Neither is allowed to change behaviour, so + * both are driven here against bytes the accessor itself produced. + */ + +import { describe, it, expect } from 'vitest'; +import { loadExcelJs } from './xlsx-module.js'; +import { parseXlsxToRows } from './import-prepare.js'; + +/** A two-row sheet, written through the accessor's own namespace. */ +async function writeFixture(): Promise { + const ExcelJS = await loadExcelJs(); + const wb = new ExcelJS.Workbook(); + const ws = wb.addWorksheet('Sheet1'); + ws.addRow(['id', 'title', 'score']); + ws.addRow(['a1', 'first', 7]); + ws.addRow(['a2', 'second', 9]); + return Buffer.from(await wb.xlsx.writeBuffer()); +} + +describe('loadExcelJs', () => { + it('resolves the exceljs namespace with the interop the call sites used to do inline', async () => { + const ExcelJS = await loadExcelJs(); + // `.default ?? namespace`: drop either half and one of these is undefined. + expect(typeof ExcelJS.Workbook).toBe('function'); + expect(typeof ExcelJS.stream.xlsx.WorkbookWriter).toBe('function'); + expect(new ExcelJS.Workbook().worksheets).toEqual([]); + }); + + it('returns the same module record on repeat calls — the load stays cached, not re-fetched', async () => { + expect(await loadExcelJs()).toBe(await loadExcelJs()); + }); +}); + +describe('parseXlsxToRows keeps both arms of its Buffer | ArrayBuffer parameter', () => { + it('reads the Node `Buffer` arm — the one arm the exceljs shim forces an assertion on', async () => { + const rows = await parseXlsxToRows(await writeFixture()); + expect(rows).toEqual([ + { id: 'a1', title: 'first', score: '7' }, + { id: 'a2', title: 'second', score: '9' }, + ]); + }); + + it('reads the `ArrayBuffer` arm — the arm that stays type-checked, unasserted', async () => { + const bytes = await writeFixture(); + const arrayBuffer = bytes.buffer.slice( + bytes.byteOffset, + bytes.byteOffset + bytes.byteLength, + ) as ArrayBuffer; + expect(arrayBuffer).toBeInstanceOf(ArrayBuffer); + const rows = await parseXlsxToRows(arrayBuffer); + expect(rows).toEqual([ + { id: 'a1', title: 'first', score: '7' }, + { id: 'a2', title: 'second', score: '9' }, + ]); + }); + + it('honours the sheet selector through the typed `getWorksheet` (its `as any` is gone)', async () => { + const ExcelJS = await loadExcelJs(); + const wb = new ExcelJS.Workbook(); + wb.addWorksheet('Empty'); // decoy first sheet + const ws = wb.addWorksheet('Data'); + ws.addRow(['id', 'title']); + ws.addRow(['x1', 'from-named-sheet']); + const bytes = Buffer.from(await wb.xlsx.writeBuffer()); + expect(await parseXlsxToRows(bytes, {}, 'Data')).toEqual([ + { id: 'x1', title: 'from-named-sheet' }, + ]); + }); +}); diff --git a/packages/rest/src/xlsx-module.ts b/packages/rest/src/xlsx-module.ts new file mode 100644 index 0000000000..13090a5dac --- /dev/null +++ b/packages/rest/src/xlsx-module.ts @@ -0,0 +1,108 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * The one place PRODUCTION source in this package reaches `exceljs`. + * + * Two production paths load the module lazily — `parseXlsxToRows` (the whole + * .xlsx **import** path, `import-prepare.ts`) and `createXlsxStream` (the + * streaming .xlsx **export** path, `rest-server.ts`). Both used to bind it as + * `const ExcelJS: any = (await import('exceljs')).default ?? (await import('exceljs'))`, + * so every workbook, worksheet, row and cell downstream of that binding sat + * outside the type system: a misspelled method, a wrong argument arity or a + * property exceljs renamed was not a compile error, only a runtime fault in a + * deployed import or export. Stating the binding once, here, and typing it is + * what puts those two paths back inside tsc's reach. + * + * ## The lazy load is deliberate, and it is preserved + * + * `exceljs` is reached through `await import('exceljs')` ON PURPOSE, so CSV and + * JSON imports/exports never pay to load it. Everything in this module is + * either a TYPE (erased at emit) or lives inside the async accessor, so the + * module stays out of both callers' static graph exactly as before. + * ⛔ Do not convert this to a static import. + * + * ## The trade this file accepts, stated once because it is real + * + * Typing `await import('exceljs')` pulls exceljs's DECLARATIONS into production + * modules that previously kept them out — including the module-local `Buffer` + * shim the test layer already documents. `exceljs@4.4.0/index.d.ts` opens, at + * line 1, with: + * + * ```ts + * declare interface Buffer extends ArrayBuffer { } + * ``` + * + * That file carries 106 top-level `export` declarations, so it IS a module — + * which makes this `Buffer` module-local, and it therefore SHADOWS Node's + * global `Buffer` inside every exceljs signature, `Xlsx.load` (index.d.ts:1490) + * included: + * + * ```ts + * load(buffer: Buffer, options?: Partial): Promise; + * ``` + * + * ⇒ there is no Node `Buffer` value that satisfies that parameter, so the + * production import path inherits the same assertion problem the test layer + * pays. The defect is in the published declaration, not at any call site. + * + * ⭐ That trade is accepted deliberately, and it is a trade DOWN. Before this + * module the cost was *the whole path is unchecked*; after it the cost is *one + * named assertion with its reason written next to it* — + * {@link asXlsxLoadInput}, applied to the Node `Buffer` arm and to nothing + * else. The first hides the problem; the second puts it where a reader can see + * it, and where a future exceljs release can retire it in one edit. + * + * ## Runtime is untouched + * + * exceljs has always accepted the values these two paths hand it — that is what + * the `any` bindings were doing, and passing. This module changes only what tsc + * is told; it does not convert, copy or reshape any bytes. + */ + +import type { Xlsx } from 'exceljs'; + +/** + * exceljs's own types, re-exported so this stays the only production file in + * the package that names the dependency. + */ +export type { Workbook, Worksheet, Row, CellValue } from 'exceljs'; + +/** The exceljs module namespace, as exceljs's own declarations describe it. */ +export type ExcelJsModule = typeof import('exceljs'); + +/** + * The parameter type `Xlsx.load` actually declares, read off the dependency's + * own signature rather than spelled by hand. Naming it this way means that if + * exceljs ever drops the shim, this alias resolves to Node's `Buffer` and + * {@link asXlsxLoadInput} quietly becomes a no-op rather than a lie. + */ +export type XlsxLoadInput = Parameters[0]; + +/** + * Assert ONE arm of `parseXlsxToRows`'s `Buffer | ArrayBuffer` parameter — the + * Node `Buffer` one — into what `Xlsx.load` declares. + * + * ⛔ The parameter below is `Buffer`, NOT `Buffer | ArrayBuffer`, and that is the + * whole point. `ArrayBuffer` is already assignable to exceljs's module-local + * shim, so the `ArrayBuffer` arm must keep reaching `load` unasserted and stay + * genuinely checked. A blanket assertion over the union would switch off type + * checking that works today — patching a small hole with a bigger one. + */ +export function asXlsxLoadInput(bytes: Buffer): XlsxLoadInput { + return bytes as unknown as XlsxLoadInput; +} + +/** + * Load `exceljs` lazily and hand back its namespace TYPED. + * + * The single binding site the two production paths share. `.default ?? + * namespace` is the CommonJS interop the callers already performed: exceljs is + * a CJS package, so an ESM `import()` puts the real namespace on `.default` + * while a bundled/transpiled consumer receives it directly. The expression is + * awaited once instead of twice — the second `await import('exceljs')` in the + * code this replaces resolved from the module cache to the identical record. + */ +export async function loadExcelJs(): Promise { + const mod: ExcelJsModule & { default?: ExcelJsModule } = await import('exceljs'); + return mod.default ?? mod; +}