Skip to content

Commit 75e66fc

Browse files
hotlongclaude
andauthored
fix(metadata-protocol): stop the meta diff endpoint serving credential values (#8671) (#8797)
* fix(metadata-protocol): stop the meta diff endpoint serving credential values (#8671) `diffMetaItem` read two stored bodies verbatim and emitted the raw values, so a `datasource` row whose credential rotated between versions returned both the old and the new password in cleartext through the live GA endpoint `GET /api/v1/meta/:type/:name/diff`. Implements the maintainer ruling (issue comment 5299845282, Option B): diff the RAW bodies so a rotation still reports its path as changed, then take the emitted value/from/to from the type's redacted projection of those same bodies — both sides, all three buckets. Substitution rather than path-matching against `redactedKeys` is deliberate: `diffShallow` is top-level while `redactedKeys` is nested and dotted, so matching `redactedKeys` against an entry path would match nothing on the real leak. Reading each emitted value out of the already-redacted body keeps the redactor as the single source of what a credential is. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XeQRiAa7vYRVX5Fog7Zby8 * test(metadata-protocol): make the diff-redaction stub refuse WHERE combinators (#8671) `check:where-matcher` caught the new test double's `matches` as silently wrong: it walked the filter treating every key as a field name, so a `$or` / `$and` key was compared as if it were a column — `r.$or` is undefined, the row is silently excluded, and a suite can go green while asserting on a query nobody wrote. Fixed by REFUSING rather than implementing, the path 142 of 236 matchers in this repo take and the one the gate's header recommends for a double that only ever sees scalar equality. This double serves two readers that both issue flat `{ organization_id, type, name }` filters, so no combinator can legitimately arrive; if one ever does, the throw makes it loud instead of silent. Gate now reports 236/236 conforming, 0 silently-wrong, 0 unjudged, baseline not grown. The seven pins are unaffected. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XeQRiAa7vYRVX5Fog7Zby8 --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent bb96297 commit 75e66fc

3 files changed

Lines changed: 387 additions & 2 deletions

File tree

‎.changeset/tough-jars-invite.md‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,16 @@
1+
---
2+
'@objectstack/metadata-protocol': patch
3+
---
4+
5+
Stop `GET /api/v1/meta/:type/:name/diff` serving stored credential values.
6+
7+
`diffMetaItem` compared two stored metadata bodies and emitted the raw values it
8+
found, so a `datasource` row whose credential rotated between versions returned
9+
both the old and the new password in cleartext (inline `config.password` and the
10+
password component of `config.url` alike).
11+
12+
The diff is still computed on the RAW bodies — a credential rotation continues to
13+
report its path as changed — but the emitted `value` / `from` / `to` are now taken
14+
from the type's redacted projection of those same bodies, on both sides. Types
15+
with no registered redactor are unaffected and keep serving their values by
16+
reference.
Lines changed: 332 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,332 @@
1+
// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
2+
3+
/**
4+
* #8671 — `GET /api/v1/meta/:type/:name/diff` must not serve stored credential
5+
* VALUES, while still reporting that the credential CHANGED.
6+
*
7+
* The endpoint is routed and live (`rest-server.ts` answers `res.json(result)`
8+
* unmodified), and `diffMetaItem` reads `sys_metadata_history` bodies plus the
9+
* active row through `repo.get()` — both VERBATIM. A pre-#8078 `datasource` row
10+
* whose credential rotated therefore emitted both the old and the new password
11+
* in cleartext.
12+
*
13+
* ## Why this is not #8154's item redactor one line over
14+
*
15+
* A diff is not an item body: it is a list of `(path, value)` triples, so there
16+
* is no body for the item-level redactor to take. This is the same "different
17+
* plane, same stored bytes" split that made #7990 its own card.
18+
*
19+
* ## The ruled shape (maintainer ruling, issue comment 5299845282 — Option B)
20+
*
21+
* > Ruled: Option B — diff raw, then redact emitted values at redactor-named
22+
* > paths, keeping the path and the "changed" signal.
23+
*
24+
* ⛔ Option A (redact BOTH bodies before diffing) is ruled out and these pins
25+
* are written to catch a drift back to it: two redacted bodies are EQUAL at a
26+
* redacted path, so the rotation would vanish into a no-diff. `pins the path as
27+
* changed` below fails under Option A, which is the point of asserting the
28+
* path's presence and its values' absence as two separate facts rather than
29+
* just sweeping for the secret.
30+
*
31+
* ## Anti-vacuity
32+
*
33+
* The `ablate the redactor` block re-registers `datasource` with an IDENTITY
34+
* redactor through the public `registerMetadataTypeRedactor` overlay and
35+
* asserts the cleartext comes back. So the green above is a statement about the
36+
* redaction running — not about the fixture happening to be credential-free.
37+
* The `label` control is the second arm: it proves the redaction is
38+
* PATH-SELECTIVE rather than a blanket value scrub, which a sweep-only test
39+
* cannot distinguish (a diff that emitted no values at all would pass it).
40+
*/
41+
import { afterEach, describe, expect, it } from 'vitest';
42+
import { assertEngineDeleteDispatch, assertEngineUpdateDispatch, hashSpec } from '@objectstack/metadata-core';
43+
import { getMetadataTypeRedactor, registerMetadataTypeRedactor } from '@objectstack/spec/kernel';
44+
import { ObjectStackProtocolImplementation } from './index.js';
45+
46+
/** The rotating inline credential, one spelling per version. */
47+
const PW_V2 = 'hunter2-old';
48+
const PW_V3 = 'hunter3-new';
49+
/** …and the same secret embedded in a URL, which #8078 leaves schema-ACCEPTED. */
50+
const URL_PW_V2 = 's3cr3t-old';
51+
const URL_PW_V3 = 's3cr3t-new';
52+
const URL_V2 = `postgresql://reporting:${URL_PW_V2}@db.internal:5432/warehouse`;
53+
const URL_V3 = `postgresql://reporting:${URL_PW_V3}@db.internal:5432/warehouse`;
54+
/** What the read path serves in the URL's place — username kept, password gone. */
55+
const SERVED_URL = 'postgresql://reporting@db.internal:5432/warehouse';
56+
57+
/** The ordinary, non-credential field whose values MUST still be served. */
58+
const LABEL_V2 = 'Warehouse';
59+
const LABEL_V3 = 'Warehouse Reporting';
60+
61+
/** v1: before the datasource had a `config` at all — the `added` bucket's fixture. */
62+
const BODY_V1 = { name: 'warehouse', label: LABEL_V2, driver: 'postgres' };
63+
/** v2: a legacy row at rest from before #8078 — inline password AND URL password. */
64+
const BODY_V2 = {
65+
name: 'warehouse',
66+
label: LABEL_V2,
67+
driver: 'postgres',
68+
config: { host: 'db.internal', username: 'reporting', password: PW_V2, url: URL_V2 },
69+
};
70+
/** v3: the ROTATION — both credential spellings change, and so does `label`. */
71+
const BODY_V3 = {
72+
name: 'warehouse',
73+
label: LABEL_V3,
74+
driver: 'postgres',
75+
config: { host: 'db.internal', username: 'reporting', password: PW_V3, url: URL_V3 },
76+
};
77+
78+
/**
79+
* Scalar equality only — and it REFUSES anything else rather than guessing.
80+
*
81+
* The two readers this double serves issue flat filters: `diffMetaItem` queries
82+
* `sys_metadata_history` by `{ organization_id, type, name }`, and
83+
* `SysMetadataRepository.history` by the same three. No combinator ever arrives.
84+
*
85+
* The `throw` is the point (`check:where-matcher`). Treating a `$or` / `$and`
86+
* key as an ordinary column name is the gate's shape (b): `r.$or` is
87+
* `undefined`, the comparison fails, the row is silently excluded, and the
88+
* suite goes green while asserting on a query nobody wrote. Refusing makes this
89+
* file RED the moment a combinator arrives, which is the honest failure — this
90+
* double is not a query engine and must not pretend to be one.
91+
*/
92+
function matches(r: Record<string, unknown>, where: Record<string, unknown>): boolean {
93+
for (const [k, v] of Object.entries(where)) {
94+
if (k.startsWith('$')) {
95+
throw new Error(
96+
`stub engine: WHERE combinator '${k}' is not implemented by this double — `
97+
+ 'it matches scalar equality only. Implement it here rather than letting it '
98+
+ 'be read as a field name.',
99+
);
100+
}
101+
if (v === undefined) continue;
102+
if (r[k] !== v) return false;
103+
}
104+
return true;
105+
}
106+
107+
/**
108+
* Table-aware stub: `diffMetaItem` reads `sys_metadata_history` directly while
109+
* `historyMetaItem` and `repo.get()` read it and `sys_metadata`. A single-map
110+
* stub would answer the history query with the active row and quietly make the
111+
* version selection meaningless.
112+
*/
113+
function makeStubEngine() {
114+
const tables: Record<string, Array<Record<string, unknown>>> = {
115+
sys_metadata: [],
116+
sys_metadata_history: [],
117+
};
118+
const engine: any = {
119+
async find(table: string, opts: { where: Record<string, unknown> }) {
120+
return (tables[table] ?? []).filter((r) => matches(r, opts.where));
121+
},
122+
async findOne(table: string, opts: { where: Record<string, unknown> }) {
123+
return (tables[table] ?? []).find((r) => matches(r, opts.where)) ?? null;
124+
},
125+
async insert() { return { id: 'stub' }; },
126+
async update(_t: string, data: Record<string, unknown>, opts: { where: Record<string, unknown> }) {
127+
assertEngineUpdateDispatch(data, opts);
128+
return { id: null };
129+
},
130+
async delete(_t: string, opts?: Record<string, unknown>) {
131+
assertEngineDeleteDispatch(opts);
132+
return { deleted: 0 };
133+
},
134+
async transaction<T>(cb: (ctx: any, info: { owned: boolean }) => Promise<T>): Promise<T> {
135+
return cb(undefined, { owned: true });
136+
},
137+
async syncObjectSchema() { /* no DDL in this stub */ },
138+
registry: {
139+
listItems: () => [],
140+
isPackageDisabled: () => false,
141+
getItem: () => undefined,
142+
registerItem: () => {},
143+
registerObject: () => {},
144+
getPackage: () => undefined,
145+
},
146+
};
147+
return { engine, tables };
148+
}
149+
150+
/**
151+
* Seed the three history versions plus the active row.
152+
*
153+
* Seeded DIRECTLY, never through `saveMetaItem`: #8078 closed the write door on
154+
* inline credentials, so the rows that leak are precisely the ones no authoring
155+
* path can create any more, and only a direct seed reproduces them.
156+
*/
157+
function seedVersions(tables: Record<string, Array<Record<string, unknown>>>) {
158+
const base = { organization_id: null, type: 'datasource', name: 'warehouse' };
159+
const bodies = [BODY_V1, BODY_V2, BODY_V3];
160+
bodies.forEach((body, i) => {
161+
tables.sys_metadata_history!.push({
162+
...base,
163+
id: `h_${i + 1}`,
164+
version: i + 1,
165+
event_seq: i + 1,
166+
operation_type: i === 0 ? 'create' : 'update',
167+
metadata: JSON.stringify(body),
168+
checksum: hashSpec(body),
169+
recorded_at: new Date(i + 1).toISOString(),
170+
});
171+
});
172+
tables.sys_metadata!.push({
173+
...base,
174+
id: 'r_active',
175+
package_id: null,
176+
state: 'active',
177+
metadata: JSON.stringify(BODY_V3),
178+
checksum: hashSpec(BODY_V3),
179+
version: 3,
180+
});
181+
}
182+
183+
/** Every string anywhere in a payload, so a leak cannot hide in a nested key. */
184+
function allStrings(value: unknown, out: string[] = []): string[] {
185+
if (typeof value === 'string') out.push(value);
186+
else if (Array.isArray(value)) value.forEach((v) => allStrings(v, out));
187+
else if (value && typeof value === 'object') Object.values(value).forEach((v) => allStrings(v, out));
188+
return out;
189+
}
190+
191+
function expectNoCredential(payload: unknown) {
192+
const strings = allStrings(payload);
193+
for (const secret of [PW_V2, PW_V3, URL_PW_V2, URL_PW_V3]) {
194+
expect(strings.some((s) => s.includes(secret))).toBe(false);
195+
}
196+
}
197+
198+
const entry = (list: Array<{ path: string }>, path: string) => list.find((e) => e.path === path);
199+
200+
// The registry overlay is process-global; a leaked identity redactor would
201+
// silently vacuum every later assertion in the run.
202+
const BUILTIN_DATASOURCE_REDACTOR = getMetadataTypeRedactor('datasource')!;
203+
afterEach(() => {
204+
registerMetadataTypeRedactor('datasource', BUILTIN_DATASOURCE_REDACTOR);
205+
});
206+
207+
async function diff(from: number, to: number) {
208+
const { engine, tables } = makeStubEngine();
209+
seedVersions(tables);
210+
const protocol = new ObjectStackProtocolImplementation(engine);
211+
return await protocol.diffMetaItem({
212+
type: 'datasource',
213+
name: 'warehouse',
214+
fromVersion: from,
215+
toVersion: to,
216+
}) as any;
217+
}
218+
219+
describe('#8671 — a credential ROTATION diffs as changed, with neither value served', () => {
220+
it('pins the path as changed and withholds BOTH sides (the ruling`s named pin)', async () => {
221+
const res = await diff(2, 3);
222+
223+
// ① The audit fact SURVIVES: the path is reported as changed. This is
224+
// the assertion Option A cannot pass — redacting before the compare
225+
// makes the two bodies equal at `config`, and the entry disappears.
226+
const changed = entry(res.changed, 'config');
227+
expect(changed).toBeDefined();
228+
229+
// ② Neither the old nor the new value is served, in either spelling.
230+
expect((changed as any).from.password).toBeUndefined();
231+
expect((changed as any).to.password).toBeUndefined();
232+
expect((changed as any).from.url).toBe(SERVED_URL);
233+
expect((changed as any).to.url).toBe(SERVED_URL);
234+
expectNoCredential(res);
235+
});
236+
237+
it('is PATH-SELECTIVE — an ordinary field still diffs with both values visible', async () => {
238+
// The discriminating control. Without it these pins cannot tell
239+
// "correct" from "redacts everything": a diff that emitted no values at
240+
// all would satisfy the sweep above while destroying the endpoint.
241+
const res = await diff(2, 3);
242+
243+
const label = entry(res.changed, 'label') as any;
244+
expect(label).toBeDefined();
245+
expect(label.from).toBe(LABEL_V2);
246+
expect(label.to).toBe(LABEL_V3);
247+
});
248+
249+
it('keeps the non-credential keys INSIDE the redacted value', async () => {
250+
// Redaction is per credential key, not per entry: an operator still
251+
// sees what else moved in `config`. `diffShallow` is top-level, so the
252+
// emitted value is the whole sub-object — and it is the redactor's
253+
// nested projection of it, not a hole.
254+
const res = await diff(2, 3);
255+
256+
const changed = entry(res.changed, 'config') as any;
257+
expect(changed.from.host).toBe('db.internal');
258+
expect(changed.from.username).toBe('reporting');
259+
expect(changed.to.host).toBe('db.internal');
260+
expect(changed.to.username).toBe('reporting');
261+
});
262+
});
263+
264+
describe('#8671 — the other two emission buckets', () => {
265+
it('`added` — a credential appearing in a later version is not served', async () => {
266+
const res = await diff(1, 2);
267+
268+
const added = entry(res.added, 'config') as any;
269+
expect(added).toBeDefined();
270+
expect(added.value.password).toBeUndefined();
271+
expect(added.value.url).toBe(SERVED_URL);
272+
expect(added.value.username).toBe('reporting');
273+
expectNoCredential(res);
274+
});
275+
276+
it('`removed` — a credential dropped in a later version is not served', async () => {
277+
const res = await diff(2, 1);
278+
279+
const removed = entry(res.removed, 'config') as any;
280+
expect(removed).toBeDefined();
281+
expect(removed.value.password).toBeUndefined();
282+
expect(removed.value.url).toBe(SERVED_URL);
283+
expectNoCredential(res);
284+
});
285+
});
286+
287+
describe('#8671 — anti-vacuity: ablate the redactor', () => {
288+
it('the cleartext comes back when `datasource` carries an identity redactor', async () => {
289+
// Proves the green above is the redaction RUNNING, not a fixture that
290+
// never held a credential. Ablated through the public registry overlay
291+
// — the same door a type owner registers through — so nothing about the
292+
// production wiring is reached around.
293+
registerMetadataTypeRedactor('datasource', (item) => ({ item, redactedKeys: [] }));
294+
295+
const res = await diff(2, 3);
296+
const changed = entry(res.changed, 'config') as any;
297+
expect(changed.from.password).toBe(PW_V2);
298+
expect(changed.to.password).toBe(PW_V3);
299+
expect(changed.from.url).toBe(URL_V2);
300+
expect(changed.to.url).toBe(URL_V3);
301+
});
302+
303+
it('an ordinary type with no redactor keeps serving its values untouched', async () => {
304+
// The passthrough branch: `hasMetadataRedactor` is false for `view`, so
305+
// the emitted values are the raw diff's, by reference.
306+
const { engine, tables } = makeStubEngine();
307+
const base = { organization_id: null, type: 'view', name: 'grid' };
308+
[{ name: 'grid', label: 'A' }, { name: 'grid', label: 'B' }].forEach((body, i) => {
309+
tables.sys_metadata_history!.push({
310+
...base,
311+
id: `h_${i + 1}`,
312+
version: i + 1,
313+
event_seq: i + 1,
314+
operation_type: i === 0 ? 'create' : 'update',
315+
metadata: JSON.stringify(body),
316+
checksum: hashSpec(body),
317+
recorded_at: new Date(i + 1).toISOString(),
318+
});
319+
});
320+
const protocol = new ObjectStackProtocolImplementation(engine);
321+
322+
const res: any = await protocol.diffMetaItem({
323+
type: 'view',
324+
name: 'grid',
325+
fromVersion: 1,
326+
toVersion: 2,
327+
});
328+
const label = entry(res.changed, 'label') as any;
329+
expect(label.from).toBe('A');
330+
expect(label.to).toBe('B');
331+
});
332+
});

0 commit comments

Comments
 (0)