From c5b8c7c469fa9dd68107b363c3603af2253a98f4 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 12 Aug 2026 00:39:07 +0000 Subject: [PATCH] fix(objectql): strip the hidden `__search` companion from every record body (#7642) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The `__search` search-normalization companion (#2486) is declared invisible to clients — `hidden` + `readonly` + `system` + `searchable: false` — and every one of those flags does something real: the column stays out of auto-views, out of the `$search` auto-default, and a `$searchFields` override naming it is refused with a 400 ("is hidden"). None of them is a PROJECTION rule. A query that names no `fields` reaches the driver with `ast.fields` undefined, drivers answer that with `SELECT *`, and the column rode back in the four record bodies QA measured (#7629): query results, GET by id, `/search` hits, and the 201 create body. The strip runs at the engine, the producer all four surfaces share — `/search` hits are `engine.find` rows verbatim, the create body is `engine.insert`'s return verbatim, so fixing consumers one at a time would have left three of the four broken. Covered: `find`, `findOne`, the nested records `expand` produces, the create response and the update response. The update response is not one of the four reported surfaces but is the same column in the same response shape; leaving it out would make POST and PATCH on one object disagree about whether a client-invisible column is visible. A predicate update resolves to a count and is unaffected. Two shaping details, both from the report: - Not gated on the schema declaring the column. The symptom survived a restart with `OS_SEARCH_PINYIN_ENABLED=false`: with the switch off the registry stops DECLARING the field, but the physical column and its values remain (ADR-0045 migrations are additive) and `SELECT *` keeps returning them. A strip that asked `schema.fields.__search` first would be silent in exactly the deployment that filed the bug. - One caller keeps its read. `plugin-pinyin-search`'s backfill projects `['id', ...sources, '__search']` under a system context and compares the stored blob against a recomputed one; stripping it unconditionally would make the walk rewrite every row of every object on every pass. A SYSTEM caller that names the column still gets it — a non-system caller does not, even by name, since `select` only gates on whether a field is KNOWN and `?select=__search` would otherwise be a documented way straight through the strip. Scope is this one column. Hidden system columns do come back generally (`organization_id` and its siblings), but they are load-bearing in client payloads today; removing them is a contract decision, not a defect fix. The new suite is a MATRIX over every record-returning door rather than a test for the door that was fixed, in both provisioning states — one contract, five places that can break it independently is the shape that rots one door at a time. Reverse-checked: 17 of its 24 cases fail on the unfixed engine. --- ...arch-companion-default-projection-strip.md | 49 +++ packages/objectql/src/core.ts | 2 + packages/objectql/src/engine.ts | 83 ++++ packages/objectql/src/index.ts | 2 + ...panion-read-projection-conformance.test.ts | 410 ++++++++++++++++++ packages/objectql/src/search-companion.ts | 57 +++ 6 files changed, 603 insertions(+) create mode 100644 .changeset/search-companion-default-projection-strip.md create mode 100644 packages/objectql/src/search-companion-read-projection-conformance.test.ts diff --git a/.changeset/search-companion-default-projection-strip.md b/.changeset/search-companion-default-projection-strip.md new file mode 100644 index 0000000000..368c94d72d --- /dev/null +++ b/.changeset/search-companion-default-projection-strip.md @@ -0,0 +1,49 @@ +--- +"@objectstack/objectql": patch +--- + +fix(objectql): strip the hidden `__search` companion column from every record body (#7642) + +The `__search` search-normalization companion (#2486) is declared invisible to +clients — `hidden` + `readonly` + `system` + `searchable: false` — and every one +of those flags does something real: the column stays out of auto-views, out of +the `$search` auto-default, and a `$searchFields` override naming it is refused +with a 400 ("is hidden"). None of them is a **projection** rule. A query that +names no `fields` reaches the driver with `ast.fields` undefined, drivers answer +that with `SELECT *`, and the column rode back in the four record bodies a QA +run measured (#7629): list/query results, `GET /data/:object/:id`, +`GET /api/v1/search` hits, and the 201 create body. + +The strip now runs at the engine, which is the producer all four surfaces share +(`/search` hits are `engine.find` rows verbatim; the create body is +`engine.insert`'s return verbatim). Fixing them one consumer at a time is how +three of the four would have stayed broken. `find`, `findOne`, the nested +records `expand` produces, the create response and the **update** response are +all covered; the update response is not one of the four reported surfaces but is +the same column in the same response shape, and leaving it out would have made +POST and PATCH on one object disagree about whether a client-invisible column is +visible. A predicate update resolves to an affected-row count and is unaffected. + +Two details the fix is shaped around, both from the report: + +- **It is not gated on the schema declaring the column.** The symptom survived a + restart with `OS_SEARCH_PINYIN_ENABLED=false`, and that is not a stale process: + with the switch off the registry stops declaring the field, but the physical + column and its values remain (ADR-0045 migrations are additive) and `SELECT *` + keeps returning them. A strip that asked `schema.fields.__search` first would + be silent in exactly the deployment that reported the bug, so the key on the + row is the signal. +- **One caller keeps its read.** `plugin-pinyin-search`'s backfill/reconcile walk + projects `['id', …sources, '__search']` under a system context and compares the + stored blob against a recomputed one; stripping that unconditionally would make + it rewrite every row of every object on every pass. A **system** caller that + names the column in `fields` still gets it. A non-system caller does not, even + by name — `select` only gates on whether a field is *known*, so `?select=__search` + would otherwise be a documented way straight through the strip. + +Scope is this one column. Hidden system columns do come back generally +(`organization_id` and its siblings), but they are load-bearing in client +payloads today; removing them is a contract decision, not a defect fix. + +New exports from `@objectstack/objectql`: `stripSearchCompanion` and +`isSearchCompanionRequested`. diff --git a/packages/objectql/src/core.ts b/packages/objectql/src/core.ts index cef65bd443..96d8450cec 100644 --- a/packages/objectql/src/core.ts +++ b/packages/objectql/src/core.ts @@ -31,6 +31,8 @@ export { resolveSearchCompanionSources, isCompanionSourceEligible, isCompanionMatchableTerm, + isSearchCompanionRequested, + stripSearchCompanion, containsCJK, } from './search-companion.js'; export type { CompanionFieldMeta, CompanionObjectMeta } from './search-companion.js'; diff --git a/packages/objectql/src/engine.ts b/packages/objectql/src/engine.ts index d70eeb41d9..9cd8bdbe88 100644 --- a/packages/objectql/src/engine.ts +++ b/packages/objectql/src/engine.ts @@ -131,6 +131,7 @@ import { import { pluralToSingular, ExternalWriteForbiddenError } from '@objectstack/spec/shared'; import { SchemaRegistry, computeFQN } from './registry.js'; import { expandSearchToFilter } from './search-filter.js'; +import { isSearchCompanionRequested, stripSearchCompanion } from './search-companion.js'; import { ExpressionEngine } from '@objectstack/formula'; import type { Expression } from '@objectstack/spec'; import { isAggregatedViewContainer, expandViewContainer } from '@objectstack/spec'; @@ -4755,6 +4756,52 @@ export class ObjectQL implements IObjectQLEngine { } } + /** + * [#7642] Strip the hidden `__search` companion column from what a read + * hands back, unless a SYSTEM caller named it in its projection. + * + * The column is declared client-invisible (`hidden` + `readonly` + `system` + * + `searchable: false`) and the enforcement that exists is real: it is kept + * out of auto-views, out of the `$search` auto-default, and a `$searchFields` + * override naming it is refused with a 400 ("is hidden"). What was missing is + * the PROJECTION half — a query that names no `fields` reaches the driver + * with `ast.fields` undefined, every driver answers that with `SELECT *`, and + * the companion rode back in every record body: list results, GET by id, + * `/search` hits (which are `engine.find` rows verbatim) and the 201 create + * body. The rule is applied HERE, at the engine, because the engine is the + * PRODUCER those four surfaces share; fixing them one consumer at a time is + * how three of the four would stay broken. + * + * Two carve-outs, both measured rather than defensive: + * + * - **A system caller that asks for it by name keeps it.** The companion has + * exactly one such reader: `plugin-pinyin-search`'s backfill/reconcile + * walk, which projects `['id', ...sources, '__search']` under + * `{ isSystem: true }` and compares the stored blob against the recomputed + * one. Strip it unconditionally and that comparison reads `undefined` + * every pass — the backfill would rewrite every row of every object on + * every run, which is worse than the disclosure it was fixing. + * - **A non-system caller does NOT keep it, even by name.** `select` only + * gates on whether a field is KNOWN (`assertProjectionFieldsExist`), and + * the companion is known once provisioned — so `?select=__search` would + * otherwise be an open door straight through this strip, and a + * client-invisibility rule with a documented spelling that bypasses it is + * not one. `isSystem` is server-derived (never client input), the same + * trust the read-only strips on the write path already place in it. + * + * ⚠️ `requestedFields` must be the CALLER's `fields`, captured before + * `planFormulaProjection` — that pass rewrites the projection to every stored + * column when a formula is in play, companion included. + */ + private stripSearchCompanionFromRead( + rows: unknown, + requestedFields: readonly string[] | undefined, + context: ExecutionContext | undefined, + ): void { + if (context?.isSystem && isSearchCompanionRequested(requestedFields)) return; + stripSearchCompanion(rows); + } + /** * Dereference a stored secret ref back to its plaintext. Intended for * privileged, server-side consumers (e.g. a datasource connection-pool @@ -6867,6 +6914,10 @@ export class ObjectQL implements IObjectQLEngine { const _findSchema = this._registry.getObject(object); this.expandSearchOnAst(ast, _findSchema); + // [#7642] The caller's OWN projection, captured before any planning pass + // rewrites it — the only thing that can answer "did this caller ask for + // `__search`?". See `stripSearchCompanionFromRead`. + const _findRequestedFields = Array.isArray(ast.fields) ? [...ast.fields] : undefined; // [#7095] Before the projection is planned and before anything is handed to // a driver: an ORDER BY this engine cannot materialise is refused, not // dropped. `fillQueryAstDefaults` has already normalised `orderBy` into @@ -6953,6 +7004,12 @@ export class ObjectQL implements IObjectQLEngine { // resolveSecret() against the stored ref instead. this.maskSecretFields(object, hookContext.result); + // [#7642] …and never let the hidden `__search` companion column out + // through the default projection either. After the hooks, for the + // same reason the mask is: a server-side `afterFind` handler is not + // the client this column is hidden from. + this.stripSearchCompanionFromRead(hookContext.result, _findRequestedFields, opCtx.context); + return hookContext.result; } catch (e) { this.logger.error('Find operation failed', e as Error, { object }); @@ -7023,6 +7080,8 @@ export class ObjectQL implements IObjectQLEngine { // dropped sort does not merely reorder the answer, it returns a DIFFERENT // record, and the one it returns looks exactly as legitimate. assertOrderByIsMaterializable(objectName, 'findOne', _findOneSchema, ast.orderBy); + // [#7642] Caller's own projection, before planning rewrites it — see `find`. + const _findOneRequestedFields = Array.isArray(ast.fields) ? [...ast.fields] : undefined; const _findOneFormula = planFormulaProjection(_findOneSchema, ast.fields); if (_findOneFormula.projected) ast.fields = _findOneFormula.projected; @@ -7089,6 +7148,10 @@ export class ObjectQL implements IObjectQLEngine { // Mask secret fields — plaintext never leaves through the read path. this.maskSecretFields(objectName, hookContext.result); + // [#7642] Hidden `__search` companion — same door, same rule as `find`. + // This is the `GET /data/:object/:id` surface (`getData` reads through + // findOne), one of the four the issue measured. + this.stripSearchCompanionFromRead(hookContext.result, _findOneRequestedFields, opCtx.context); return hookContext.result; }); @@ -7655,6 +7718,15 @@ export class ObjectQL implements IObjectQLEngine { rowCtx.event = 'afterInsert'; rowCtx.result = coerceBooleanFields(schemaForValidation as any, resultRows[k] as any); await this.triggerHooks('afterInsert', rowCtx); + // [#7642] The 201 create body is the surface most likely to be missed + // on this card, and the one no read-path fix reaches: `createData` + // returns `engine.insert`'s value verbatim as `record`, so the + // companion the `beforeInsert` stamp just wrote came straight back to + // the client. A write has no projection to consult, so there is no + // "asked for it by name" case to honour — the strip is unconditional. + // AFTER the hook dispatch, matching the read path: `afterInsert` + // handlers still observe the whole stored row. + stripSearchCompanion(rowCtx.result); } // Roll-up: recompute parent summary fields that aggregate this object. @@ -8581,6 +8653,17 @@ export class ObjectQL implements IObjectQLEngine { } } + // [#7642] Same strip the create body gets, for the same reason: a + // by-id update resolves to a RECORD, `updateData` returns it as + // `record`, and the `beforeUpdate` companion stamp had just written + // `__search` into the row it echoes. The issue measured four + // surfaces and this is not one of them — it is the same column, the + // same contract and the same response shape, and leaving it out + // would mean POST and PATCH on one object disagreed about whether a + // client-invisible column is visible. A predicate update resolves to + // an affected-row COUNT (#4639), which the strip skips as a + // non-object. + stripSearchCompanion(hookContext.result); // The record IS updated; a summary that could not recompute after // retries must surface, not stay silent (framework#3147). if (summaryFailures.length > 0) throw new SummaryRecomputeError(summaryFailures, hookContext.result); diff --git a/packages/objectql/src/index.ts b/packages/objectql/src/index.ts index 340b22f410..6a6a27f72f 100644 --- a/packages/objectql/src/index.ts +++ b/packages/objectql/src/index.ts @@ -37,6 +37,8 @@ export { resolveSearchCompanionSources, isCompanionSourceEligible, isCompanionMatchableTerm, + isSearchCompanionRequested, + stripSearchCompanion, containsCJK, } from './search-companion.js'; export type { CompanionFieldMeta, CompanionObjectMeta } from './search-companion.js'; diff --git a/packages/objectql/src/search-companion-read-projection-conformance.test.ts b/packages/objectql/src/search-companion-read-projection-conformance.test.ts new file mode 100644 index 0000000000..dd592d4c61 --- /dev/null +++ b/packages/objectql/src/search-companion-read-projection-conformance.test.ts @@ -0,0 +1,410 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#7642] The hidden `__search` companion column never leaves the engine + * through the default projection — on ANY door. + * + * ## The defect + * + * `provisionSearchCompanion` declares the column `hidden` + `readonly` + + * `system` + `searchable: false`, and every one of those flags does something + * real: the column stays out of auto-views, out of the `$search` auto-default, + * and a `$searchFields` override that names it is refused with a 400 ("is + * hidden"). None of them is a PROJECTION rule. A query naming no `fields` + * reaches the driver with `ast.fields` undefined, every driver answers that + * with `SELECT *`, and so `__search` came back in every record body the QA run + * looked at: list results, GET by id, `/search` hits and the 201 create body. + * + * ## Why this suite is a MATRIX and not a test per door + * + * There is one contract ("clients never see this column") and at least five + * places that can honour or break it independently — `find`, `findOne`, the + * nested records `expand` produces, the create response and the update + * response. That is exactly the shape that rots one door at a time: the read + * path gets fixed, the write path keeps echoing, and nothing fails. So every + * door runs the SAME assertion from the same table below, and a door added + * later is one row. + * + * `GET /api/v1/search` is not directly reachable from this package — it is + * served by `MetadataProtocol.searchAll` (@objectstack/metadata-protocol), + * which builds a `where`/`orderBy`/`limit` query with NO `fields` and returns + * `engine.find`'s rows verbatim as `hit.record`. The `search-shaped read` row + * below is that call, spelled the way `searchAll` spells it. + * + * ## The two provisioning states, both of which must strip + * + * The QA report's sharpest detail is that the symptom SURVIVED a restart with + * `OS_SEARCH_PINYIN_ENABLED=false`. That is not a stale-process artifact: with + * the switch off the registry stops DECLARING the field, but the physical + * column and its stored values remain (ADR-0045 migrations are additive) and + * `SELECT *` keeps returning them. A strip gated on `schema.fields.__search` + * would therefore be silent in precisely the deployment that filed the bug — + * so both states are pinned here. + */ + +import { describe, it, expect, beforeEach } from 'vitest'; +import type { EngineQueryOptions, ServiceObject } from '@objectstack/spec/data'; +import type { ExecutionContext } from '@objectstack/spec/kernel'; + +import { ObjectQL } from './engine.js'; +import { SEARCH_COMPANION_FIELD, provisionSearchCompanion } from './search-companion.js'; + +// --------------------------------------------------------------------------- +// A driver that stores whole rows and honours an explicit projection, so a +// `fields`-less read really is `SELECT *` and the companion really does ride +// back — the condition the defect needs. Rows handed out are shallow copies +// (the `IDataDriver` contract; see `MemoryDriver.find`), so an engine-side +// in-place strip cannot corrupt the store — asserted at the bottom. +// --------------------------------------------------------------------------- + +type Row = Record; + +interface DriverAst { + where?: Record; + fields?: string[]; + limit?: number; + offset?: number; + orderBy?: Array<{ field: string; order?: string }>; +} + +interface StoreDriver { + rows: Map>; + /** Write a row straight into the backing table, bypassing the engine. */ + seed(object: string, row: Row): void; + /** Read a row straight out of the backing table, bypassing the engine. */ + stored(object: string, id: string): Row | undefined; +} + +function makeStoreDriver(): { driver: unknown } & StoreDriver { + const rows = new Map>(); + const tableFor = (o: string): Map => { + let t = rows.get(o); + if (!t) { t = new Map(); rows.set(o, t); } + return t; + }; + let seq = 0; + + const matches = (row: Row, where: Record | undefined): boolean => { + if (!where) return true; + for (const [k, v] of Object.entries(where)) { + if (k === '$and') return (v as Array>).every((w) => matches(row, w)); + if (k === '$or') return (v as Array>).some((w) => matches(row, w)); + if (k.startsWith('$')) continue; + if (v !== null && typeof v === 'object') { + const cmp = v as Record; + if ('$contains' in cmp) { + const needle = String(cmp.$contains).toLowerCase(); + if (!String(row[k] ?? '').toLowerCase().includes(needle)) return false; + continue; + } + if ('$in' in cmp) { + if (!(cmp.$in as unknown[]).some((x) => x === row[k])) return false; + continue; + } + if ('$eq' in cmp) { + if (row[k] !== cmp.$eq) return false; + continue; + } + continue; + } + if ((row[k] ?? null) !== (v ?? null)) return false; + } + return true; + }; + + const run = (object: string, ast: DriverAst | undefined): Row[] => { + let out = Array.from(tableFor(object).values()).filter((r) => matches(r, ast?.where)); + if (typeof ast?.offset === 'number' && ast.offset > 0) out = out.slice(ast.offset); + if (typeof ast?.limit === 'number' && ast.limit >= 0) out = out.slice(0, ast.limit); + // Projection honoured exactly (SqlDriver semantics); absent ⇒ SELECT *, + // which is the whole point of this suite. Shallow copies either way. + return Array.isArray(ast?.fields) && ast.fields.length > 0 + ? out.map((r) => Object.fromEntries(ast.fields!.map((f) => [f, r[f]]))) + : out.map((r) => ({ ...r })); + }; + + const driver = { + name: 'store', version: '0.0.0', supports: {}, + async connect(): Promise {}, + async disconnect(): Promise {}, + async checkHealth(): Promise { return true; }, + async execute(): Promise { return null; }, + async find(object: string, ast?: DriverAst): Promise { return run(object, ast); }, + async findOne(object: string, ast?: DriverAst): Promise { return run(object, ast)[0] ?? null; }, + async create(object: string, data: Row): Promise { + seq += 1; + const id = (data.id as string | undefined) ?? `r_${seq}`; + const row: Row = { ...data, id }; + tableFor(object).set(id, row); + return { ...row }; + }, + async update(object: string, id: string, data: Row): Promise { + const table = tableFor(object); + const current = table.get(id); + if (!current) throw new Error(`not found: ${object}/${id}`); + const next: Row = { ...current, ...data, id }; + table.set(id, next); + return { ...next }; + }, + async delete(object: string, id: string): Promise { return tableFor(object).delete(id); }, + async count(object: string, ast?: DriverAst): Promise { return run(object, ast).length; }, + async bulkCreate(object: string, batch: Row[]): Promise { + const out: Row[] = []; + for (const r of batch) out.push(await driver.create(object, r)); + return out; + }, + async beginTransaction(): Promise<{ commit: () => Promise; rollback: () => Promise }> { + return { commit: async () => {}, rollback: async () => {} }; + }, + async commit(): Promise {}, + async rollback(): Promise {}, + }; + + return { + driver, + rows, + seed: (object, row) => { tableFor(object).set(String(row.id), { ...row }); }, + stored: (object, id) => tableFor(object).get(id), + }; +} + +// --------------------------------------------------------------------------- +// Objects +// --------------------------------------------------------------------------- + +const CONTACT = 'showcase_contact'; +const ACCOUNT = 'showcase_account'; + +/** The QA repro's own value: full pinyin + initials for 张伟. */ +const BLOB = 'zhangwei zw'; + +const contactBase: ServiceObject = { + name: CONTACT, + label: 'Contact', + fields: { + id: { type: 'text' }, + name: { type: 'text' }, + account: { type: 'lookup', reference: ACCOUNT }, + // A formula field is present ON PURPOSE: `planFormulaProjection` widens an + // explicit projection to every stored column — companion included — so a + // strip that read the POST-planning `ast.fields` would conclude the caller + // had asked for `__search` and hand it back. Pinned below. + shout: { type: 'formula', expression: { dialect: 'cel', source: 'record.name' } }, + }, +}; + +const accountBase: ServiceObject = { + name: ACCOUNT, + label: 'Account', + fields: { + id: { type: 'text' }, + name: { type: 'text' }, + }, +}; + +/** `OS_SEARCH_PINYIN_ENABLED=true`: the registry declares the column. */ +const contactProvisioned = provisionSearchCompanion(contactBase); +const accountProvisioned = provisionSearchCompanion(accountBase); + +const SYSTEM_CTX: ExecutionContext = { isSystem: true, positions: [], permissions: [] } as ExecutionContext; + +interface Harness { + engine: ObjectQL; + store: StoreDriver; +} + +/** + * @param declared whether the registry declares `__search` on the object — + * i.e. whether `OS_SEARCH_PINYIN_ENABLED` was on at boot. + */ +async function makeEngine(declared: boolean): Promise { + const engine = new ObjectQL(); + const store = makeStoreDriver(); + engine.registerDriver(store.driver as never, true); + await engine.init(); + engine.registry.registerObject(declared ? contactProvisioned : contactBase, 'test'); + engine.registry.registerObject(declared ? accountProvisioned : accountBase, 'test'); + return { engine, store }; +} + +/** The `plugin-pinyin-search` write hook, in miniature. */ +function bindCompanionStamp(engine: ObjectQL): void { + const stamp = (ctx: { input?: { data?: unknown } }): void => { + const data = ctx?.input?.data; + if (!data || typeof data !== 'object' || Array.isArray(data)) return; + const row = data as Row; + if (!('name' in row)) return; + row[SEARCH_COMPANION_FIELD] = BLOB; + }; + engine.registerHook('beforeInsert', stamp, { packageId: 'test:companion' }); + engine.registerHook('beforeUpdate', stamp, { packageId: 'test:companion' }); +} + +/** Every record body a door can hand back, flattened for one assertion. */ +function recordsOf(value: unknown): Row[] { + if (value == null || typeof value !== 'object') return []; + if (Array.isArray(value)) return value.flatMap((v) => recordsOf(v)); + return [value as Row]; +} + +function expectNoCompanion(value: unknown): void { + for (const row of recordsOf(value)) { + expect(Object.keys(row)).not.toContain(SEARCH_COMPANION_FIELD); + } +} + +// --------------------------------------------------------------------------- +// The matrix +// --------------------------------------------------------------------------- + +describe.each([ + ['column declared by the registry (OS_SEARCH_PINYIN_ENABLED=true)', true], + ['column NOT declared — stored rows still carry it (restart with OS_SEARCH_PINYIN_ENABLED=false)', false], +])('[#7642] `__search` never reaches a caller — %s', (_label, declared) => { + let engine: ObjectQL; + let store: StoreDriver; + let contactId: string; + + beforeEach(async () => { + const harness = await makeEngine(declared); + engine = harness.engine; + store = harness.store; + + // The row is SEEDED through the driver in both states: with the column + // undeclared the engine would refuse (or strip) an unknown key on insert, + // and "the column exists in the table but not in the metadata" is exactly + // the state the flag-off restart leaves behind. The create-body door + // below writes through the engine instead, where the stamp applies. + store.seed(ACCOUNT, { id: 'acc_1', name: '华宁科技', [SEARCH_COMPANION_FIELD]: 'huaningkeji hnkj' }); + contactId = 'con_1'; + store.seed(CONTACT, { id: contactId, name: '张伟', account: 'acc_1', [SEARCH_COMPANION_FIELD]: BLOB }); + }); + + it('the fixture is real: the stored row DOES carry the column', () => { + // Guards the suite against passing vacuously — every assertion below is + // "the engine removed it", which is worthless if it was never there. + expect(store.stored(CONTACT, contactId)).toHaveProperty(SEARCH_COMPANION_FIELD, BLOB); + }); + + it('door: find — POST /data/:object/query', async () => { + const rows = await engine.find(CONTACT, { where: { id: contactId } }); + expect(rows).toHaveLength(1); + expect(rows[0]?.name).toBe('张伟'); + expectNoCompanion(rows); + }); + + it('door: find with no query at all — the bare list read', async () => { + expectNoCompanion(await engine.find(CONTACT)); + }); + + it('door: findOne — GET /data/:object/:id', async () => { + const row = await engine.findOne(CONTACT, { where: { id: contactId } }); + expect(row?.name).toBe('张伟'); + expectNoCompanion(row); + }); + + it('door: the search-shaped read — GET /api/v1/search hits', async () => { + // `MetadataProtocol.searchAll`'s query, verbatim in shape: a `$contains` + // predicate, a limit, an `updated_at` sort — and no `fields`. Its rows + // become `hit.record` untouched. + const query: EngineQueryOptions = { + where: { name: { $contains: '张' } }, + limit: 5, + orderBy: [{ field: 'name', order: 'desc' }], + }; + const hits = await engine.find(CONTACT, query); + expect(hits).toHaveLength(1); + expectNoCompanion(hits); + }); + + it('door: the 201 create body', async () => { + bindCompanionStamp(engine); + const created = await engine.insert(CONTACT, { name: '李娜' }); + expect(created?.id).toBeTruthy(); + expectNoCompanion(created); + }); + + it('door: the update response body', async () => { + bindCompanionStamp(engine); + const updated = await engine.update(CONTACT, { id: contactId, name: '张伟明' }); + expect(updated?.name).toBe('张伟明'); + expectNoCompanion(updated); + }); + + it('door: records nested by expand', async () => { + const rows = await engine.find(CONTACT, { + where: { id: contactId }, + expand: { account: { object: ACCOUNT } }, + }); + // The expansion is what is under test — if it did not happen there is no + // nested body to have stripped, and the assertion would pass vacuously. + expect(rows[0]?.account).toBeTypeOf('object'); + expectNoCompanion(rows[0]?.account); + }); + + it('an explicit projection that widens through a formula still strips it', async () => { + // `shout` is a formula, so `planFormulaProjection` rewrites `ast.fields` + // to every stored column — `__search` among them. The strip must read the + // CALLER's projection, not the planned one. + const rows = await engine.find(CONTACT, { where: { id: contactId }, fields: ['id', 'shout'] }); + expect(rows[0]?.shout).toBe('张伟'); + expectNoCompanion(rows); + }); + + it('a non-system caller cannot opt back in by naming the column', async () => { + // `select` only gates on whether a field is KNOWN, so once the column is + // provisioned `?select=__search` sails through `assertProjectionFieldsExist`. + // If that spelling still returned the value, the strip would be advisory. + const rows = await engine.find(CONTACT, { + where: { id: contactId }, + fields: ['id', SEARCH_COMPANION_FIELD], + }); + expect(rows).toHaveLength(1); + expectNoCompanion(rows); + }); + + it('the strip does not write through to the stored row', async () => { + await engine.find(CONTACT, { where: { id: contactId } }); + await engine.findOne(CONTACT, { where: { id: contactId } }); + // The engine strips in place (as secret-masking does) and relies on the + // driver contract that read rows are copies. A driver-side regression here + // would silently destroy the index instead of hiding it. + expect(store.stored(CONTACT, contactId)).toHaveProperty(SEARCH_COMPANION_FIELD, BLOB); + }); +}); + +// --------------------------------------------------------------------------- +// The one caller that MUST keep reading the column. +// --------------------------------------------------------------------------- + +describe('[#7642] the system backfill keeps its read of `__search`', () => { + it('a system caller that names the column in `fields` still gets it', async () => { + // `plugin-pinyin-search`'s backfill/reconcile walk projects + // `['id', ...sources, '__search']` under `{ isSystem: true }` and compares + // the stored blob against a freshly computed one. Strip that and the + // comparison reads `undefined` every pass, so the walk rewrites every row + // of every object on every run — write amplification traded for a + // disclosure fix, on a column whose disclosure risk the issue rates low. + const { engine, store } = await makeEngine(true); + store.seed(CONTACT, { id: 'con_1', name: '张伟', [SEARCH_COMPANION_FIELD]: BLOB }); + + const query: EngineQueryOptions = { + fields: ['id', 'name', SEARCH_COMPANION_FIELD], + context: SYSTEM_CTX, + }; + const rows = await engine.find(CONTACT, query); + + expect(rows).toHaveLength(1); + expect(rows[0]?.[SEARCH_COMPANION_FIELD]).toBe(BLOB); + }); + + it('…but a system caller that did NOT name it does not get it either', async () => { + // The exemption is "asked for it", not "is privileged" — a system-context + // read with a default projection is still a default projection. + const { engine, store } = await makeEngine(true); + store.seed(CONTACT, { id: 'con_1', name: '张伟', [SEARCH_COMPANION_FIELD]: BLOB }); + + const query: EngineQueryOptions = { context: SYSTEM_CTX }; + expectNoCompanion(await engine.find(CONTACT, query)); + }); +}); diff --git a/packages/objectql/src/search-companion.ts b/packages/objectql/src/search-companion.ts index 7da81b3961..35ed8801a0 100644 --- a/packages/objectql/src/search-companion.ts +++ b/packages/objectql/src/search-companion.ts @@ -199,3 +199,60 @@ export function containsCJK(value: unknown): boolean { export function isCompanionMatchableTerm(term: string): boolean { return /[a-z]/i.test(term) && !CJK_RE.test(term); } + +/** + * Did the caller NAME the companion column in its projection? (#7642) + * + * The strip below is a DEFAULT-projection rule, so it has to be able to tell + * "the caller asked for everything" from "the caller asked for this column". + * Read the caller's ORIGINAL `fields`, never the planned one: when a formula + * field is in play `planFormulaProjection` rewrites `ast.fields` to every + * stored column on the schema — companion included — so a post-planning read + * would report "explicitly requested" for a query that named only `name`. + */ +export function isSearchCompanionRequested(fields: unknown): boolean { + return Array.isArray(fields) && fields.includes(SEARCH_COMPANION_FIELD); +} + +/** + * Remove the companion column from records on their way OUT of the engine + * (#7642). + * + * The column is declared `hidden` + `readonly` + `system` + `searchable: + * false`, and those flags are all real — they keep it out of auto-views, out + * of the `$search` auto-default and out of `$searchFields` overrides (which + * refuse it with a 400 "is hidden"). None of them is a PROJECTION rule, + * though: no read path narrowed the default projection, the drivers answer an + * absent `fields` with `SELECT *`, and so every record body — query results, + * GET by id, `/search` hits, the 201 create body — carried `__search`. + * + * ## Why this is not gated on the schema declaring the column + * + * It cannot be. The symptom outlives the switch: `OS_SEARCH_PINYIN_ENABLED=false` + * stops {@link provisionSearchCompanion} from DECLARING the field, but the + * physical column stays in the table (ADR-0045 migrations are additive), the + * stored values stay in it, and `SELECT *` keeps returning them. A strip that + * asked `schema.fields.__search` first would therefore go silent in exactly + * the deployment that reported the bug. The key on the ROW is the only + * reliable signal, and deleting an absent key costs nothing. + * + * ## Scope — `__search` only + * + * Deliberately this one column, not "hidden system columns" as a class. + * Hidden system columns DO come back generally (`organization_id` is the + * obvious one), but they are load-bearing in client payloads today; removing + * them is a contract decision, not a defect fix. See #7642. + * + * Mutates in place, like {@link ObjectQL.maskSecretFields} — the driver + * contract already forbids handing back live references into the backing + * store (`MemoryDriver.find` spells this out), so the row being mutated is + * the caller's copy. + */ +export function stripSearchCompanion(records: unknown): void { + if (records == null) return; + const rows: unknown[] = Array.isArray(records) ? records : [records]; + for (const row of rows) { + if (!row || typeof row !== 'object') continue; + delete (row as Record)[SEARCH_COMPANION_FIELD]; + } +}