Skip to content

Commit 87712ab

Browse files
fix(metadata-protocol): global search skips objects and fields the caller cannot read (#21879)
Fixes #21836 Clause-②: no ## What changed `searchAll` (`packages/metadata-protocol/src/protocol.ts`) no longer answers a member's whole global search with `403 PERMISSION_DENIED` because one object in scope is unreadable. - **Object level.** Before an object is queried, the sweep asks the `security` service's `canReadObject` with the caller's context. That method is the engine middleware's own read gate, arm for arm (`ISecurityService.canReadObject`). An object it refuses is skipped. - **Field level (found on the real boot).** With only the object-level skip, a restricted member's UNSCOPED search was still 403. The cause: `sys_user`'s server-resolved search fields include admin-only columns (`role`, `ban_reason`, `last_login_ip`), and the engine's predicate guard refuses a search that matches on a field the caller may not query. Each object is now searched only on the fields the caller may query (`getQueryableFields`), handed to the engine as `searchFields`. ADR-0061 says that key only ever narrows. An object left with no queryable search field is skipped. When the caller may query every search field, no `searchFields` is sent, so the request is the same as before. - **No leak from a skipped object.** It is never queried, never named and never counted in `totalObjects`. The decision is made before any row is read, so no hit, count or timing depends on what it holds. No `objectsSkipped` field was added, because that count would itself describe objects the caller cannot see. An explicit `objects=` naming an unreadable object gets the same answer as a name that matches no object. - **Still fails closed.** A read error on a readable object propagates, envelope intact. If `canReadObject` or `getQueryableFields` throws, the search fails instead of shrinking. Without a security service, or with one that lacks these methods, there is no pre-filter, and `find` still enforces. Calls that carry no context are not pre-filtered. - **The misleading comment is fixed.** The per-object `catch` said object authorization is enforced at the REST door (`enforceAuth`) first. That door checks authentication only. The comment now says where admission is decided. ### Route chosen: the pre-filter, not catching the typed denial The triage comment chose to catch the engine's typed denial. The dispatch preferred the pre-filter. I took the pre-filter for two reasons: 1. The middleware throws the same `PermissionDeniedError` for very different causes: "permission subsystem unavailable", an unresolved object posture, a missing delegator, a field-level filter-oracle refusal. Catching the class would swallow every one of them, including a field-level refusal on an object the caller may read (the 403 this card's dogfood hit). With the shipped `plugin-security`, a permission outage is not distinguished by either route: `canReadObject` answers `false` on a resolution failure, so the pre-filter skips objects during an outage too (see the Acceptance note below). The deciding reasons are item 2 and the field-level fix, not outage handling. 2. The single-authority concern is met by contract: `canReadObject` is specified to compute the middleware's verdict from the same resolution, never a re-derivation. ## Tests Unit tests are in `packages/metadata-protocol/src/protocol.search-skip-unreadable.test.ts`, 12 cases: - a control that reproduces the 403 when no security service is wired; - a mixed scope: only readable hits, and the unreadable objects are not queried, named or counted; - the answer is the same whether or not a skipped object's rows would match; - an explicit `objects=` with an unreadable object answers the same as a nonexistent name; - an all-access caller gets the same answer as with no service; - a read error on a readable object still fails the search; - an admission check that throws fails the search; - a call without a context is not pre-filtered; - a partial queryable set is sent as `searchFields`; - an object with no queryable search field is skipped; - a full queryable set, or no answer from `getQueryableFields`, sends no `searchFields`; - a field check that throws fails the search. Dogfood: `packages/qa/dogfood/test/search-skip-unreadable.dogfood.test.ts` boots a real kernel with the real `SecurityPlugin` over HTTP. Its two app objects hold rows matching one term, and the member can read one of them. It covers: - control: the walled object is 403 at its own `/data` door; - the member's unscoped search is 200 with the readable hit and never names the walled object; - `objects=open,walled` returns the readable hit only; - `objects=walled` gives the same answer as a nonexistent name; - the admin still gets both hits. ### Ablation (dogfood, one-off, nothing left in the tree) - **Mutation.** `node scripts/ablation-replace.mjs` replaced the `canReadObject` skip line with a constant-false guard carrying the marker `ABLATED_21836` (anchor hits 1 to 0, blob 66cc5f6 to 5d37739011c9). - **Build and check.** Ran `OS_SKIP_DTS=1` for the metadata-protocol build, then `ablation-dist-preflight` confirmed the marker is present in `dist/index.js` and `dist/index.cjs`. - **Result.** Dogfood went **2 failed / 2 passed**. Both member cases got `403 PERMISSION_DENIED` ("You do not have permission to perform this action."). - **Restore.** The tool restored the file, with blob equal to HEAD and an empty `git diff HEAD`. The rebuilt closure then passed `ablation-dist-preflight --absent`, with the marker absent from all 24 built files and the tree clean. ### Local verification (at the final head) - `pnpm --filter @objectstack/metadata-protocol exec vitest run` over the 7 search test files: 68 passed. - Dogfood file: 4 passed. - `typecheck` for metadata-protocol and dogfood: exit 0. Both programs include the new test files (`--listFilesOnly`). - The derived gate families from `dispatch-gates.mjs --commands` are green, plus `check:type-check-debt`, which ran under the verify lock. The exception is `check:dual-build-cjs-loads`, which printed PREREQUISITE NOT MET because unrelated packages in this worktree have no `dist/`. That gate was NOT MEASURED and is declared to CI. - `check:engine-double-contract` asked for the new double to be pinned, and that pin is committed. - Lint, narrowed to the 3 changed TS files with `eslint --no-inline-config --format json`: 3 files, 0 errors, 0 warnings. Type-aware linting is not enabled in `eslint.config.mjs` (no `parserOptions.project`), so this diff cannot change the verdict on any untouched file. ## Acceptance notes - **Out of scope here, and remains open as a separate finding: the same field-level refusal at `GET /api/v1/data/sys_user?search=admin`.** Measured on a fresh boot as a plain member: `403 PERMISSION_DENIED` ("query on 'sys_user' references field(s) not readable by the caller: role, ban_reason, last_login_ip"). The same member gets `200` from `GET /api/v1/data/sys_user`. The caller named no field. The server picked the search fields and then refused the caller for them. The upstream fix belongs in the engine's search expansion (`expandSearchOnAst`), which could narrow to the caller's queryable fields. If that lands, the field narrowing in `searchAll` becomes redundant and can be deleted. It is reported for filing by the seat. - `canReadObject` in `plugin-security` returns `false`, logged at `error`, when permission resolution throws. It does not throw. During a permission outage the search therefore skips objects instead of failing. That is the method's documented fail-closed contract, and nothing leaks, but it is not the propagate-on-outage behaviour of the rest of this sweep. No defect is claimed; noted only. --- _Generated by [Claude Code](https://claude.ai/code/session_018zT8d8NpiQ1ExhuNd5TxY6)_ --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 607463d commit 87712ab

5 files changed

Lines changed: 616 additions & 15 deletions

File tree

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
---
2+
'@objectstack/metadata-protocol': patch
3+
---
4+
5+
Global search (`GET /api/v1/search`) skips the objects a caller cannot read instead of failing the whole request with `403 PERMISSION_DENIED` (#21836).
6+
7+
Clause-②: no
8+
9+
- **Object level.** Before an object is queried, `searchAll` asks the `security` service's `canReadObject` with the caller's context, the same read gate the engine middleware enforces. An object it refuses is skipped. A member whose scope included any object they hold no read grant on used to get 403 for every query. The console palette then showed "No results found." with no error.
10+
- **Field level.** Each object is searched only on the fields the caller may query (`getQueryableFields`). The engine refuses a search that would match on a hidden field, and `sys_user`'s searchable fields include admin-only columns, so a member's unscoped search hit that refusal as well. An object left with no queryable search field is skipped.
11+
- **Nothing about a skipped object reaches the response.** It is not queried, named or counted in `totalObjects`, and the decision is made before any row is read. An explicit `objects=` naming an unreadable object gets the same answer as a name that matches no object.
12+
- **Other failures still fail the search.** A read error on a readable object, or an admission check that throws, propagates as before. Callers that can read every object, and calls without a context, get the same answer as before.
Lines changed: 325 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,325 @@
1+
// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
2+
3+
/**
4+
* `searchAll` skips an object the caller may not READ, instead of failing the
5+
* whole search.
6+
*
7+
* The REST search door checks authentication only. Each swept object reached
8+
* `engine.find`, whose security middleware refuses an object the caller holds
9+
* no read grant on — and that refusal propagated out of the sweep, so a member
10+
* whose scope included ANY unreadable object got `403 PERMISSION_DENIED` for
11+
* the whole request, whatever the query (the console palette then showed
12+
* "No results found.").
13+
*
14+
* The sweep now asks the `security` service's `canReadObject` — the
15+
* middleware's own read gate — before it queries an object, and skips one it
16+
* refuses. What these pins hold:
17+
*
18+
* - a mixed scope answers the readable objects' hits, and the unreadable
19+
* object is never queried, never named, never counted;
20+
* - an explicit `objects=` naming an unreadable object answers exactly as
21+
* one naming an object that does not exist;
22+
* - nothing about the skipped object's rows can shape the answer — it is
23+
* identical whether or not its rows would have matched;
24+
* - an all-access caller's answer is unchanged;
25+
* - every other failure still fails the request: a read error on a readable
26+
* object, and an admission check that itself throws;
27+
* - a readable object is searched only on the fields the caller may QUERY
28+
* (`getQueryableFields`), and one left with none is skipped — the engine's
29+
* predicate guard refuses a search over a hidden field with the same 403;
30+
* - no context means no pre-filter (the reads pose no principal).
31+
*
32+
* The engine double here stands in for the middleware: it THROWS the typed
33+
* denial for an object the fake security service refuses, so a sweep that
34+
* stopped consulting `canReadObject` turns these pins red with that 403.
35+
*/
36+
37+
import { describe, it, expect, vi } from 'vitest';
38+
import { ObjectStackProtocolImplementation } from './protocol.js';
39+
import { assertEngineFindOnePredicate, type EngineFindOneQueryInput } from '@objectstack/metadata-core';
40+
41+
interface FixtureObject {
42+
name: string;
43+
label: string;
44+
fields: Record<string, { name: string; label: string; type: string }>;
45+
searchableFields?: string[];
46+
}
47+
48+
const objectFixture = (name: string): FixtureObject => ({
49+
name,
50+
label: name,
51+
fields: { name: { name: 'name', label: 'Name', type: 'text' } },
52+
});
53+
54+
function fixtureRegistry(objects: FixtureObject[]) {
55+
return {
56+
getObject: (n: string) => objects.find((o) => o.name === n),
57+
getAllObjects: () => objects,
58+
getItem: () => undefined,
59+
listItems: () => [],
60+
applyNavContributions: (x: unknown) => x,
61+
isPackageDisabled: () => false,
62+
getObjectOwner: () => undefined,
63+
getPackage: () => undefined,
64+
};
65+
}
66+
67+
/** The engine middleware's typed object-level denial, as `PermissionDeniedError` carries it. */
68+
const objectReadDenied = () =>
69+
Object.assign(new Error('You do not have permission to perform this action.'), {
70+
name: 'PermissionDeniedError',
71+
code: 'PERMISSION_DENIED',
72+
status: 403,
73+
statusCode: 403,
74+
});
75+
76+
const acct = objectFixture('acct');
77+
const lead = objectFixture('lead');
78+
const secret = objectFixture('secret_ledger');
79+
80+
const ROWS: Record<string, Array<Record<string, unknown>>> = {
81+
acct: [{ id: 'a1', name: 'Acme' }],
82+
lead: [{ id: 'l1', name: 'Acme Lead' }],
83+
secret_ledger: [{ id: 's1', name: 'Acme Secret' }],
84+
};
85+
86+
const MEMBER = { userId: 'usr_member', tenantId: 'org_1' };
87+
88+
/**
89+
* An engine whose `find` enforces `readable` the way the middleware does
90+
* (typed throw for anything else), and a security service answering the same
91+
* set. `rows` lets a case vary what the unreadable object WOULD hold.
92+
*/
93+
function harness(opts: {
94+
readable: Set<string> | 'all';
95+
withSecurity?: boolean;
96+
rows?: Record<string, Array<Record<string, unknown>>>;
97+
canReadObject?: (object: string, context?: unknown) => Promise<boolean>;
98+
getQueryableFields?: (object: string, context?: unknown) => Promise<string[] | undefined>;
99+
objects?: FixtureObject[];
100+
failRead?: { object: string; error: unknown };
101+
}) {
102+
const rows = opts.rows ?? ROWS;
103+
const readCalls: string[] = [];
104+
const findOptions: Array<[string, Record<string, unknown>]> = [];
105+
const admits = (o: string) => opts.readable === 'all' || opts.readable.has(o);
106+
const engine = {
107+
registry: fixtureRegistry(opts.objects ?? [acct, lead, secret]),
108+
find: vi.fn(async (object: string, options: Record<string, unknown>) => {
109+
if (object === 'sys_metadata') return [];
110+
readCalls.push(object);
111+
findOptions.push([object, options]);
112+
if (!admits(object)) throw objectReadDenied();
113+
if (opts.failRead && opts.failRead.object === object) throw opts.failRead.error;
114+
return rows[object] ?? [];
115+
}),
116+
findOne: vi.fn(async (object: string, query?: EngineFindOneQueryInput) => {
117+
assertEngineFindOnePredicate(object, query);
118+
return null;
119+
}),
120+
};
121+
const canReadObject = vi.fn(opts.canReadObject ?? (async (o: string) => admits(o)));
122+
const services = new Map<string, unknown>();
123+
const security: Record<string, unknown> = { canReadObject };
124+
if (opts.getQueryableFields) security.getQueryableFields = vi.fn(opts.getQueryableFields);
125+
if (opts.withSecurity !== false) services.set('security', security);
126+
const protocol = new ObjectStackProtocolImplementation(engine as never, () => services as Map<string, any>);
127+
return { protocol, readCalls, findOptions, canReadObject };
128+
}
129+
130+
async function rejection(run: () => Promise<unknown>): Promise<Record<string, unknown>> {
131+
let caught: unknown;
132+
let didResolve = false;
133+
try {
134+
await run();
135+
didResolve = true;
136+
} catch (e) {
137+
caught = e;
138+
}
139+
expect(didResolve, 'expected a rejection, but the call resolved').toBe(false);
140+
return caught as Record<string, unknown>;
141+
}
142+
143+
describe('searchAll — an object the caller may not read is skipped, not fatal', () => {
144+
it('control: without the pre-filter the middleware denial fails the whole search with 403', async () => {
145+
// The defect, reproduced on this harness: no security service to ask,
146+
// so the sweep reaches `find` on `lead` and the typed denial escapes.
147+
const { protocol } = harness({ readable: new Set(['acct']), withSecurity: false });
148+
const caught = await rejection(() => protocol.searchAll({ q: 'Acme', context: MEMBER }));
149+
expect(caught.code).toBe('PERMISSION_DENIED');
150+
expect(caught.status).toBe(403);
151+
});
152+
153+
it('a mixed scope answers the readable objects and never queries, names or counts the rest', async () => {
154+
const { protocol, readCalls, canReadObject } = harness({ readable: new Set(['acct']) });
155+
156+
const result = await protocol.searchAll({ q: 'Acme', context: MEMBER });
157+
158+
expect(result.hits.map((h) => [h.object, h.id])).toEqual([['acct', 'a1']]);
159+
expect(result.totalObjects).toBe(1);
160+
expect(result.totalHits).toBe(1);
161+
expect(result.truncated).toBe(false);
162+
// Never queried — so nothing about its rows can reach the answer.
163+
expect(readCalls).toEqual(['acct']);
164+
// Asked with the caller's own context, for every swept object.
165+
expect(canReadObject.mock.calls).toEqual([
166+
['acct', MEMBER],
167+
['lead', MEMBER],
168+
['secret_ledger', MEMBER],
169+
]);
170+
// Never named, anywhere in the response.
171+
const wire = JSON.stringify(result);
172+
expect(wire).not.toContain('lead');
173+
expect(wire).not.toContain('secret_ledger');
174+
expect(Object.keys(result).sort()).toEqual(
175+
['hits', 'pages', 'query', 'totalHits', 'totalObjects', 'truncated'],
176+
);
177+
});
178+
179+
it('the answer does not depend on what a skipped object holds', async () => {
180+
const matching = harness({ readable: new Set(['acct']) });
181+
const empty = harness({
182+
readable: new Set(['acct']),
183+
rows: { acct: ROWS.acct, lead: [], secret_ledger: [] },
184+
});
185+
186+
const a = await matching.protocol.searchAll({ q: 'Acme', context: MEMBER });
187+
const b = await empty.protocol.searchAll({ q: 'Acme', context: MEMBER });
188+
189+
expect(a).toEqual(b);
190+
});
191+
192+
it('an explicit objects= naming an unreadable object answers like one naming no such object', async () => {
193+
const { protocol, readCalls } = harness({ readable: new Set(['acct']) });
194+
195+
const mixed = await protocol.searchAll({ q: 'Acme', objects: ['acct', 'secret_ledger'], context: MEMBER });
196+
expect(mixed.hits.map((h) => h.object)).toEqual(['acct']);
197+
expect(mixed.totalObjects).toBe(1);
198+
expect(JSON.stringify(mixed)).not.toContain('secret_ledger');
199+
200+
const unreadable = await protocol.searchAll({ q: 'Acme', objects: ['secret_ledger'], context: MEMBER });
201+
const nonexistent = await protocol.searchAll({ q: 'Acme', objects: ['no_such_object'], context: MEMBER });
202+
expect(unreadable).toEqual(nonexistent);
203+
expect(unreadable).toEqual({
204+
query: 'Acme', hits: [], pages: [], totalObjects: 0, totalHits: 0, truncated: false,
205+
});
206+
expect(readCalls).toEqual(['acct']);
207+
});
208+
209+
it('an all-access caller gets exactly what the sweep answered without a security service', async () => {
210+
const admin = harness({ readable: 'all' });
211+
const bare = harness({ readable: 'all', withSecurity: false });
212+
213+
const withService = await admin.protocol.searchAll({ q: 'Acme', context: { userId: 'usr_admin' } });
214+
const without = await bare.protocol.searchAll({ q: 'Acme', context: { userId: 'usr_admin' } });
215+
216+
expect(withService).toEqual(without);
217+
expect(withService.hits.map((h) => h.object)).toEqual(['acct', 'lead', 'secret_ledger']);
218+
expect(withService.totalObjects).toBe(3);
219+
expect(admin.readCalls).toEqual(['acct', 'lead', 'secret_ledger']);
220+
});
221+
222+
it('a read failure on a READABLE object still fails the search, envelope intact', async () => {
223+
const injected = Object.assign(new Error('connection terminated unexpectedly'), { code: 'ECONNRESET' });
224+
const { protocol } = harness({
225+
readable: new Set(['acct', 'lead']),
226+
failRead: { object: 'lead', error: injected },
227+
});
228+
229+
const caught = await rejection(() => protocol.searchAll({ q: 'Acme', context: MEMBER }));
230+
expect(caught).toBe(injected);
231+
});
232+
233+
it('an admission check that THROWS fails the search instead of shrinking it', async () => {
234+
const injected = Object.assign(new Error('permission store unreachable'), { code: 'ECONNREFUSED' });
235+
const { protocol, readCalls } = harness({
236+
readable: new Set(['acct']),
237+
canReadObject: async (o) => {
238+
if (o === 'lead') throw injected;
239+
return o === 'acct';
240+
},
241+
});
242+
243+
const caught = await rejection(() => protocol.searchAll({ q: 'Acme', context: MEMBER }));
244+
expect(caught).toBe(injected);
245+
expect(readCalls).not.toContain('lead');
246+
});
247+
248+
it('a context-less call is not pre-filtered: its reads carry no principal to ask about', async () => {
249+
const { protocol, canReadObject } = harness({ readable: 'all' });
250+
251+
const result = await protocol.searchAll({ q: 'Acme' });
252+
253+
expect(canReadObject).not.toHaveBeenCalled();
254+
expect(result.totalObjects).toBe(3);
255+
});
256+
});
257+
258+
describe('searchAll — a readable object is searched only on the fields the caller may query', () => {
259+
const memo: FixtureObject = {
260+
name: 'memo',
261+
label: 'memo',
262+
fields: {
263+
name: { name: 'name', label: 'Name', type: 'text' },
264+
hidden_note: { name: 'hidden_note', label: 'Hidden note', type: 'text' },
265+
},
266+
searchableFields: ['name', 'hidden_note'],
267+
};
268+
const rows = { memo: [{ id: 'm1', name: 'Acme memo' }] };
269+
270+
it('a partial queryable set is handed to the engine as searchFields', async () => {
271+
const { protocol, findOptions } = harness({
272+
readable: 'all', objects: [memo], rows,
273+
getQueryableFields: async () => ['id', 'name'],
274+
});
275+
276+
const result = await protocol.searchAll({ q: 'Acme', context: MEMBER });
277+
278+
expect(findOptions).toHaveLength(1);
279+
expect(findOptions[0][1].searchFields).toEqual(['name']);
280+
expect(findOptions[0][1].search).toBe('Acme');
281+
expect(result.hits.map((h) => [h.object, h.id])).toEqual([['memo', 'm1']]);
282+
expect(result.totalObjects).toBe(1);
283+
});
284+
285+
it('an object with NO queryable search field is skipped, unqueried and uncounted', async () => {
286+
const { protocol, readCalls } = harness({
287+
readable: 'all', objects: [acct, memo], rows: { ...ROWS, ...rows },
288+
getQueryableFields: async (o) => (o === 'memo' ? ['id'] : ['id', 'name']),
289+
});
290+
291+
const result = await protocol.searchAll({ q: 'Acme', context: MEMBER });
292+
293+
expect(readCalls).toEqual(['acct']);
294+
expect(result.totalObjects).toBe(1);
295+
expect(JSON.stringify(result)).not.toContain('memo');
296+
});
297+
298+
it('a full queryable set, or no answer, leaves the request exactly as before (no searchFields)', async () => {
299+
const full = harness({
300+
readable: 'all', objects: [memo], rows,
301+
getQueryableFields: async () => ['id', 'name', 'hidden_note'],
302+
});
303+
await full.protocol.searchAll({ q: 'Acme', context: MEMBER });
304+
expect('searchFields' in full.findOptions[0][1]).toBe(false);
305+
306+
const noAnswer = harness({
307+
readable: 'all', objects: [memo], rows,
308+
getQueryableFields: async () => undefined,
309+
});
310+
await noAnswer.protocol.searchAll({ q: 'Acme', context: MEMBER });
311+
expect('searchFields' in noAnswer.findOptions[0][1]).toBe(false);
312+
});
313+
314+
it('a queryable-fields check that THROWS fails the search', async () => {
315+
const injected = new Error('field resolution unavailable');
316+
const { protocol, readCalls } = harness({
317+
readable: 'all', objects: [memo], rows,
318+
getQueryableFields: async () => { throw injected; },
319+
});
320+
321+
const caught = await rejection(() => protocol.searchAll({ q: 'Acme', context: MEMBER }));
322+
expect(caught).toBe(injected);
323+
expect(readCalls).toEqual([]);
324+
});
325+
});

0 commit comments

Comments
 (0)