diff --git a/.changeset/meta-read-path-credential-redaction.md b/.changeset/meta-read-path-credential-redaction.md new file mode 100644 index 0000000000..7e667ff6ed --- /dev/null +++ b/.changeset/meta-read-path-credential-redaction.md @@ -0,0 +1,50 @@ +--- +"@objectstack/metadata-protocol": patch +--- + +fix(metadata-protocol): the metadata read path no longer serves stored cleartext credentials (#8154) + +`decorateMetadataItem` returned the whole stored body, so a `datasource` row +written before #8078 closed the write door came back with `config.password` in +cleartext — and the password embedded in `config.url` alongside it — from +`GET /api/v1/meta/datasources`, from the single-item read, and from the layered +read in **both** its `overlay` and `effective` layers. PR #8126 closed the +datasource-admin door (`GET /api/v1/datasources/:name`); this closes the +platform door one over. Meta read permission is granted at a far lower bar than +"may see the production database password", which is what made this reachable. + +The fix consumes the per-type redactor registry #8300 landed in +`@objectstack/spec/kernel` (`getMetadataTypeRedactor`) rather than redacting +`datasource` specifically: `datasource` is that registry's first consumer, and a +type-shaped patch here would be the narrow fix that leaves the next +secret-bearing type exposed. A plugin whose metadata type stores secrets gets +the same protection by calling `registerMetadataTypeRedactor` — no change here. + +Three properties worth knowing, each measured rather than assumed: + +- **`_diagnostics` are still computed on the RAW stored body, before + redaction.** The redacted body is exactly the shape the post-#8078 schema + accepts, so computing them afterwards flips `valid:false` to `valid:true` on + precisely the rows that hold a stored credential — which would delete the + operator's only inventory of what still needs migrating (#8081 item 3). The + two steps are composed inside one function so no call site can invert an + ordering it cannot see. +- **The stored record is never mutated, and the connect path is untouched.** + Redaction is a serving act; datasource connection and boot-time restore read + `sys_metadata` directly through the data engine, not through these exits. +- **The write path carries the credential forward**, and this half is not + optional: `saveMetaItem` accepts a redacted body and persists the credential + away, so a read scrub shipped alone would convert today's loud `422` into + **silent credential deletion** on an ordinary GET → edit → PUT round trip. + `config.url` makes it unavoidable rather than a masking choice — a + URL-embedded password is schema-accepted, so dropping it round-trips to + deletion and masking it round-trips to storing the mask as the literal + password. Stored material is re-applied only where the incoming body is + indistinguishable from what the read served; anything the author actually + wrote wins and is still judged by #8078's write gate on its own merits. This + also restores the #4326 byte-identical round-trip invariant, which + read-redaction alone would have broken. + +It preserves cleartext already at rest and creates none; moving stored +credentials into `sys_secret` is #8081 item 3's migration and is deliberately +not attempted on a write door an author drove. diff --git a/packages/metadata-protocol/src/index.ts b/packages/metadata-protocol/src/index.ts index 4b758b8d14..172eacf16d 100644 --- a/packages/metadata-protocol/src/index.ts +++ b/packages/metadata-protocol/src/index.ts @@ -79,6 +79,18 @@ export { } from './metadata-diagnostics.js'; export type { MetadataDiagnostics } from './metadata-diagnostics.js'; +// [#8154] The metadata read path's per-type credential redaction (consuming +// #8300's `@objectstack/spec/kernel` registry) and its write-path inverse. +// `decorateMetadataItem` above already composes the read half — these are +// exported for the exits decoration does not reach, and so the invariant is +// testable from the package surface rather than only through a live protocol. +export { + carryForwardRedactedValues, + hasMetadataRedactor, + redactMetadataItem, + redactMetadataItems, +} from './metadata-redaction.js'; + export type { MetadataHostEngine } from './host-engine.js'; // [#7560] ADR-0070's read-only-package rule. The authoring path (`saveMetaItem` diff --git a/packages/metadata-protocol/src/metadata-diagnostics.ts b/packages/metadata-protocol/src/metadata-diagnostics.ts index cb507c8098..36b7a9be20 100644 --- a/packages/metadata-protocol/src/metadata-diagnostics.ts +++ b/packages/metadata-protocol/src/metadata-diagnostics.ts @@ -32,6 +32,10 @@ import { PLURAL_TO_SINGULAR } from '@objectstack/spec/shared'; // function, so a document's verdict cannot depend on whether it was being saved // or being opened. See the note above the `.safeParse()` below. import { zodIssuesToMetadataIssues } from './protocol.js'; +// [#8154] The per-type credential redactor seam (`@objectstack/spec/kernel`, +// landed by #8300). Composed into `decorateMetadataItem` below — see the +// ordering note there for why it is not applied per read exit. +import { redactMetadataItem } from './metadata-redaction.js'; /** * Re-export the canonical validation-result type so callers in this @@ -119,18 +123,40 @@ export function computeMetadataDiagnostics( } /** - * Attach `_diagnostics` to a single metadata item. Returns the item - * unchanged when no diagnostics could be computed (unknown type) or + * Attach `_diagnostics` to a single metadata item, and apply the type's + * read-path redactor. Returns the item unchanged when neither applies, or * when the input is not an object. * * The returned reference is always a shallow copy when decoration * occurs — callers must not assume identity equality with the input. + * + * [#8154] ⛔ THE ORDER OF THE TWO STATEMENTS BELOW IS LOAD-BEARING, and it is + * why redaction is composed HERE rather than applied at each read exit beside + * `governServedObject` (whose own docblock in `protocol.ts` explains why + * governance and injection went the other way). + * + * Diagnostics MUST be computed on the RAW stored body. Measured, in the + * predicted direction: computing them on the redacted body flips + * `valid:false` → `valid:true` for exactly the rows holding a stored cleartext + * credential — because the redacted body is the one the post-#8078 schema + * ACCEPTS — which destroys the `#8081` item-3 migration inventory. That badge + * is the operator's only enumeration of which rows still need migrating, so + * inverting these two lines silently removes the remedy while the leak it was + * tracking looks fixed. Composed into one function so no call site can invert + * an ordering it cannot see. + * + * Redaction runs even when diagnostics are `undefined`. A type with a + * registered redactor and no registered Zod schema is exactly the shape a + * plugin's secret-bearing type arrives in, and an early `return item` on the + * diagnostics miss would serve its credentials in cleartext — a fail-open + * keyed on an unrelated registration. */ export function decorateMetadataItem(type: string, item: T): T { if (!item || typeof item !== 'object') return item; const diagnostics = computeMetadataDiagnostics(type, item); - if (!diagnostics) return item; - return { ...(item as Record), _diagnostics: diagnostics } as T; + const served = redactMetadataItem(type, item); + if (!diagnostics) return served; + return { ...(served as Record), _diagnostics: diagnostics } as T; } /** diff --git a/packages/metadata-protocol/src/metadata-redaction.ts b/packages/metadata-protocol/src/metadata-redaction.ts new file mode 100644 index 0000000000..484ab53f3c --- /dev/null +++ b/packages/metadata-protocol/src/metadata-redaction.ts @@ -0,0 +1,258 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#8154] The metadata READ path's per-type credential redaction — and the + * WRITE path's inverse, without which the redaction is a data-loss bug. + * + * ## What this module is, and what it deliberately is not + * + * It is the CONSUMER of the `@objectstack/spec/kernel` redactor registry + * (`registerMetadataTypeRedactor` / `getMetadataTypeRedactor`, #8300). It holds + * no opinion about what a credential is: `datasource` is that registry's first + * entry, SSO is expected to be its second, and this module never names either. + * A datasource-shaped patch here would be the narrow fix that leaves the next + * type exposed, which is the thing #8154 was filed to prevent. + * + * ## Ordering: `_diagnostics` BEFORE redaction — load-bearing, measured + * + * Diagnostics MUST be computed on the RAW stored body. Computing them on the + * redacted body flips `valid:false` to `valid:true` for exactly the rows that + * hold a stored cleartext credential, which destroys the `#8081` item-3 + * operator inventory — the `valid:false` badge is the only enumeration path an + * operator has for "which rows still need migrating". That is why the + * composition lives inside {@link decorateMetadataItem} (see + * `metadata-diagnostics.ts`) rather than at each read exit: an ordering a call + * site can invert is an ordering that gets inverted. + * + * ## The write-path inverse ({@link carryForwardRedactedValues}) + * + * A read scrub with no inverse is a data-loss bug, not a fix. Measured on + * `origin/main` before this change: `saveMetaItem` ACCEPTS a redacted + * datasource body and persists the credential away, so read-redaction alone + * converts today's loud `422` into SILENT credential deletion on an ordinary + * `/meta` GET → edit → PUT round trip. + * + * `config.url` is what makes the inverse unavoidable rather than a masking + * choice: a URL-embedded password is schema-ACCEPTED (#8078 pinned that + * boundary as fact), so dropping it round-trips to deletion and masking it + * round-trips to storing the mask as the literal password. Neither is a + * survivable read shape; carrying the stored value forward is. + * + * This is the generic form of the carry-forward PR #8126 added to + * `DatasourceAdminService.updateDatasource`, and it mirrors that function's + * rule exactly: **stored material is carried forward ONLY where the incoming + * body is indistinguishable from what the read path served.** Anything the + * author actually wrote wins, and is judged on its own merits by the schema + * gate — a caller that types `password` into a config still gets #8078's + * refusal. + * + * ⛔ It does NOT create cleartext, and does not migrate it out either: it + * preserves what is already at rest. Getting stored cleartext OUT of the store + * is #8081 item 3's migration and is deliberately not attempted here. + * + * ## Why nothing is stamped on the wire + * + * {@link MetadataRedactionResult} carries `redactedKeys`, and the registry's + * own docblock argues for serving it beside the item so a caller knows a + * credential is being withheld rather than inferring it from an absence. This + * module deliberately does NOT stamp it. A new served key is a READ DECORATION, + * and the list of those lives in `spec/kernel/metadata-read-decorations.ts` + * (`METADATA_READ_DECORATIONS`) — which `stripReadDecorations` uses to take + * them back off on write. Stamping a key that is not on that list would push it + * through the ordinary GET → edit → PUT round trip and into a CLOSED schema + * (#4001), so every legacy datasource save would fail with + * `unrecognized_keys` naming a key the author never wrote — the "error the + * author cannot act on" shape PR #8126 already had to repair once. The key + * belongs on that list first; that file is `packages/spec`'s to change. + */ + +import { getMetadataTypeRedactor } from '@objectstack/spec/kernel'; +import type { MetadataTypeRedactor } from '@objectstack/spec/kernel'; +import { PLURAL_TO_SINGULAR } from '@objectstack/spec/shared'; + +/** + * Resolve the redactor for a request-shaped type name. + * + * The read exits are reached with either spelling (`GET /api/v1/meta/datasources` + * arrives as `datasources`), while the registry is keyed by the SINGULAR + * metadata type name (Prime Directive #3). Normalised here through the same + * `PLURAL_TO_SINGULAR` map `computeMetadataDiagnostics` uses, so a plural read + * and a singular read cannot disagree about whether a credential is withheld. + */ +function redactorFor(type: string): MetadataTypeRedactor | undefined { + return getMetadataTypeRedactor(PLURAL_TO_SINGULAR[type] ?? type); +} + +/** + * Whether any redactor is registered for `type`. + * + * Lets a caller skip work — notably the extra stored-row read the write-path + * carry-forward needs — for the overwhelming majority of types that hold no + * secret, without having to know which types those are. + */ +export function hasMetadataRedactor(type: string): boolean { + return redactorFor(type) !== undefined; +} + +/** + * Apply the type's read-path redactor to one served metadata body. + * + * Returns the input BY REFERENCE when no redactor is registered, when the input + * is not a plain object, or when the redactor found nothing to hide — so the + * common path allocates nothing and the stored record a caller may still be + * holding is never mutated. + * + * ⛔ A throwing redactor is NOT swallowed. A redactor is contractually pure and + * must not throw; if one does, the choice is between a loud failed read and + * serving the cleartext this function exists to withhold. Failing closed is the + * only defensible answer for a security control ("Absence must be loud" — + * AGENTS.md Route & surface ownership §3), and a `catch` here would produce + * precisely the outcome #8300's own header calls the worst available one: a + * redaction that looks installed but is not applied. + */ +export function redactMetadataItem(type: string, item: T): T { + if (!item || typeof item !== 'object' || Array.isArray(item)) return item; + const redactor = redactorFor(type); + if (!redactor) return item; + const result = redactor(item as Record); + if (!result || result.redactedKeys.length === 0) return item; + return (result.item ?? item) as T; +} + +/** + * {@link redactMetadataItem} over a list. Non-array inputs and non-object + * elements pass through unchanged, matching the defensive "items may be a + * wrapped or naked array" contract the read exits already document. + */ +export function redactMetadataItems(type: string, items: T[]): T[] { + if (!Array.isArray(items)) return items; + const redactor = redactorFor(type); + if (!redactor) return items; + return items.map((item) => redactMetadataItem(type, item)); +} + +/** + * Walk to the plain object that OWNS the last segment of `segments`. + * + * `undefined` when any hop along the way is absent or is not a plain object — + * which the caller must read as "this body does not speak to that path at all", + * never as "the value is absent". The distinction is the whole guard: a PUT + * body carrying no `config` key is an author removing the container, and + * grafting `config.password` back onto it would MINT a config that holds + * nothing but a credential. + */ +function containerAt(root: unknown, segments: string[]): Record | undefined { + let node: unknown = root; + for (let i = 0; i < segments.length - 1; i += 1) { + if (!node || typeof node !== 'object' || Array.isArray(node)) return undefined; + node = (node as Record)[segments[i] as string]; + } + if (!node || typeof node !== 'object' || Array.isArray(node)) return undefined; + return node as Record; +} + +/** + * Copy-on-write set of `value` at `segments`, returning a new root and copying + * only the containers along the path. + * + * The incoming request body belongs to the caller (`saveMetaItem` hands the + * same object to the audit trail and the registry write-through), so the + * carry-forward must not mutate it in place. + */ +function withValueAt( + root: Record, + segments: string[], + value: unknown, +): Record { + const [head, ...rest] = segments as [string, ...string[]]; + const next: Record = { ...root }; + if (rest.length === 0) { + next[head] = value; + return next; + } + next[head] = withValueAt(root[head] as Record, rest, value); + return next; +} + +/** Structural equality for the values a redactor hides (scalars in practice; general by construction). */ +function sameValue(a: unknown, b: unknown): boolean { + if (a === b) return true; + if (a === null || b === null || typeof a !== 'object' || typeof b !== 'object') { + // NaN is the one primitive `===` disagrees with itself about. + return Number.isNaN(a as number) && Number.isNaN(b as number); + } + if (Array.isArray(a) !== Array.isArray(b)) return false; + if (Array.isArray(a) && Array.isArray(b)) { + return a.length === b.length && a.every((v, i) => sameValue(v, b[i])); + } + const ao = a as Record; + const bo = b as Record; + const ak = Object.keys(ao); + const bk = Object.keys(bo); + if (ak.length !== bk.length) return false; + return ak.every((k) => Object.prototype.hasOwnProperty.call(bo, k) && sameValue(ao[k], bo[k])); +} + +/** + * The write-path inverse of {@link redactMetadataItem}: re-apply the material + * the read path withheld, wherever the incoming body is indistinguishable from + * what was served. + * + * The decision is made per redacted PATH, and only three things can happen: + * + * - the incoming value at that path equals what the read served ⇒ the author + * is round-tripping something they were never shown, so the stored value is + * carried forward; + * - the incoming value differs ⇒ the author spoke, and their word wins + * verbatim (a typed-in `password` is then refused by #8078's write gate on + * its own merits — this function never launders one past it); + * - the incoming body has no container for that path at all ⇒ nothing is + * grafted, because a removed container is also the author's word. + * + * ⚠️ The first case is genuinely INDISTINGUISHABLE, not merely treated as + * equal: an author who hand-deletes `:password` from a URL sends exactly the + * bytes the redaction served, and this function restores the stored password. + * The wire carries nothing that separates the two intents, so this is a + * deliberate choice of the safe side — preserving a credential an operator may + * still depend on, over silently destroying one. The same ambiguity exists in + * `restoreRedactedConfig`, and clearing a credential on purpose has an + * unambiguous door: change it, or delete the row. + * + * @param type request-shaped metadata type (plural or singular). + * @param incoming the body about to be persisted. + * @param stored the body currently at rest, RAW (never a served copy). + */ +export function carryForwardRedactedValues(type: string, incoming: T, stored: unknown): T { + if (!incoming || typeof incoming !== 'object' || Array.isArray(incoming)) return incoming; + if (!stored || typeof stored !== 'object' || Array.isArray(stored)) return incoming; + const redactor = redactorFor(type); + if (!redactor) return incoming; + + // What a read exit WOULD have served for the row at rest. Computed from the + // stored body rather than remembered from a response, so the comparison + // holds for any caller — Studio, the CLI, a raw `curl` — and needs no + // session state. + const served = redactor(stored as Record); + if (served.redactedKeys.length === 0) return incoming; + + let out = incoming as unknown as Record; + for (const path of served.redactedKeys) { + // Dotted, item-relative — the registry's documented contract for + // `redactedKeys` (`config.password`). + const segments = path.split('.'); + const key = segments[segments.length - 1] as string; + + const storedParent = containerAt(stored, segments); + const storedValue = storedParent?.[key]; + if (storedValue === undefined) continue; + + const incomingParent = containerAt(out, segments); + if (!incomingParent) continue; + + const servedParent = containerAt(served.item, segments); + if (!sameValue(incomingParent[key], servedParent?.[key])) continue; + + out = withValueAt(out, segments, storedValue); + } + return out as unknown as T; +} diff --git a/packages/metadata-protocol/src/protocol.metadata-redaction.test.ts b/packages/metadata-protocol/src/protocol.metadata-redaction.test.ts new file mode 100644 index 0000000000..37be24c50e --- /dev/null +++ b/packages/metadata-protocol/src/protocol.metadata-redaction.test.ts @@ -0,0 +1,469 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #8154 — the metadata READ path must not serve stored cleartext credentials, + * and the WRITE path must not turn that scrub into silent credential deletion. + * + * Both halves are pinned here because shipping either alone is a defect: + * + * - read alone ⇒ an ordinary `/meta` GET → edit → PUT round trip converts + * today's loud `422` into SILENT credential DELETION (measured on + * `origin/main`; it is the reason this card is one PR); + * - write alone ⇒ nothing changes. + * + * Every row here is seeded DIRECTLY into the stub engine, never through + * `saveMetaItem`. That is not a shortcut — it is the population under test: + * #8078 closed the write door on inline credentials, so a legacy row holding + * `config.password` can no longer be created through any authoring path. The + * rows that leak are the ones written BEFORE that door closed, and only a + * direct seed reproduces them. + * + * ## Anti-vacuity + * + * Two arms, both wired: + * + * 1. **The redaction assertions go red when the redactor is removed.** The + * `ablate the redactor` block re-registers `datasource` with an IDENTITY + * redactor through the public `registerMetadataTypeRedactor` overlay and + * asserts the cleartext comes back — so the green in the blocks above is a + * statement about the redactor running, not about the fixture being + * credential-free. + * 2. **The badge assertions go red when the `_diagnostics` ordering is + * inverted.** `computeMetadataDiagnostics` on the REDACTED body returns + * `valid:true` (pinned directly below), because the redacted body is + * exactly what the post-#8078 schema accepts. So the `valid:false` + * assertions on the read exits fail the moment diagnostics are computed + * after redaction instead of before — which is the ordering that would + * destroy the #8081 item-3 operator inventory. + */ +import { afterEach, describe, expect, it } from 'vitest'; +import { assertEngineDeleteDispatch, assertEngineUpdateDispatch, hashSpec } from '@objectstack/metadata-core'; +import { + getMetadataTypeRedactor, + getMetadataTypeSchema, + registerMetadataTypeRedactor, +} from '@objectstack/spec/kernel'; +import { + ObjectStackProtocolImplementation, + carryForwardRedactedValues, + computeMetadataDiagnostics, + hasMetadataRedactor, + redactMetadataItem, +} from './index.js'; + +interface Row { + id: string; + type: string; + name: string; + organization_id: string | null; + package_id: string | null; + state: string; + metadata: string; + checksum?: string; + version?: number; +} + +function matches(r: Row, where: Record): boolean { + for (const [k, v] of Object.entries(where)) { + if (v === undefined) continue; + if ((r as any)[k] !== v) return false; + } + return true; +} + +function keyOf(w: Record) { + return `${w.type}|${w.name}|${w.organization_id ?? '__env__'}|${w.state ?? 'active'}|${w.package_id ?? '__nopkg__'}`; +} + +function makeStubEngine() { + const rows = new Map(); + let nextId = 0; + const findRow = (w: Record) => { + for (const [k, r] of rows) if (matches(r, w)) return { key: k, row: r }; + return null; + }; + const engine: any = { + async findOne(_t: string, opts: { where: Record }) { + return findRow(opts.where)?.row ?? null; + }, + async find(_t: string, opts: { where: Record }) { + return Array.from(rows.values()).filter((r) => matches(r, opts.where)); + }, + async insert(table: string, data: Record) { + if (table !== 'sys_metadata') return { id: 'side_table' }; + const row = { id: `r_${++nextId}`, ...(data as any) } as Row; + rows.set(keyOf(data), row); + return { id: row.id }; + }, + async update(table: string, data: Record, opts: { where: Record }) { + assertEngineUpdateDispatch(data, opts); + if (table !== 'sys_metadata') return { id: null }; + const found = findRow(opts.where); + if (!found) return { id: null }; + const merged = { ...found.row, ...(data as any) }; + rows.delete(found.key); + rows.set(keyOf(merged), merged); + return { id: found.row.id }; + }, + async delete(_t: string, opts?: Record) { + assertEngineDeleteDispatch(opts); + return { deleted: 0 }; + }, + async transaction(cb: (ctx: any, info: { owned: boolean }) => Promise): Promise { + return cb(undefined, { owned: true }); + }, + async syncObjectSchema() { /* no DDL in this stub */ }, + registry: { + listItems: () => [], + isPackageDisabled: () => false, + getItem: () => undefined, + registerItem: () => {}, + registerObject: () => {}, + getPackage: () => undefined, + }, + }; + return { engine, rows }; +} + +/** The password the fixture stores inline, and the one embedded in its URL. */ +const INLINE_PASSWORD = 'hunter2'; +const URL_PASSWORD = 's3cr3t'; +const STORED_URL = `postgresql://reporting:${URL_PASSWORD}@db.internal:5432/warehouse`; +/** What the read path serves in the URL's place — username kept, password gone. */ +const SERVED_URL = 'postgresql://reporting@db.internal:5432/warehouse'; + +/** + * A `datasource` row exactly as it exists at rest from before #8078: an inline + * `config.password` AND a password embedded in `config.url`. Both spellings + * matter — dropping the inline key while serving the identical credential one + * key over would be a scrub in name only. + */ +function legacyDatasourceBody() { + return { + name: 'warehouse', + label: 'Warehouse', + driver: 'postgres', + config: { + host: 'db.internal', + port: 5432, + database: 'warehouse', + username: 'reporting', + password: INLINE_PASSWORD, + url: STORED_URL, + }, + }; +} + +function seedLegacyRow(rows: Map, state: 'active' | 'draft' = 'active') { + const where = { + type: 'datasource', + name: 'warehouse', + organization_id: null, + package_id: null, + state, + }; + const body = legacyDatasourceBody(); + rows.set(keyOf(where), { + id: 'r_seed', + ...where, + metadata: JSON.stringify(body), + // A real row carries its own checksum. Omitting it makes the + // optimistic-lock parent (`repo.get()` → `row.checksum ?? hashSpec`) + // disagree with the conflict check (which reads the column), and every + // save 409s for a reason that has nothing to do with this card. + checksum: hashSpec(body), + version: 1, + } as Row); +} + +const storedRow = (rows: Map, state: 'active' | 'draft' = 'active') => + Array.from(rows.values()).find((r) => r.name === 'warehouse' && r.state === state)!; + +const storedBody = (rows: Map, state: 'active' | 'draft' = 'active') => + JSON.parse(storedRow(rows, state).metadata); + +/** Every string anywhere in a served payload, so a leak cannot hide in a nested key. */ +function allStrings(value: unknown, out: string[] = []): string[] { + if (typeof value === 'string') out.push(value); + else if (Array.isArray(value)) value.forEach((v) => allStrings(v, out)); + else if (value && typeof value === 'object') Object.values(value).forEach((v) => allStrings(v, out)); + return out; +} + +function expectNoCredential(payload: unknown) { + const strings = allStrings(payload); + expect(strings).not.toContain(INLINE_PASSWORD); + expect(strings.some((s) => s.includes(URL_PASSWORD))).toBe(false); +} + +// Restore the built-in after any block that overlays it (the registry overlay +// is process-global, and a leaked identity redactor would silently vacuum every +// later assertion in the run). +const BUILTIN_DATASOURCE_REDACTOR = getMetadataTypeRedactor('datasource')!; +afterEach(() => { + registerMetadataTypeRedactor('datasource', BUILTIN_DATASOURCE_REDACTOR); +}); + +describe('#8154 — the seam this card consumes', () => { + it('resolves a redactor for `datasource`, in both the plural and singular spellings', () => { + // `GET /api/v1/meta/datasources` arrives as the plural; the registry is + // keyed singular. A read that normalised one way and not the other + // would serve cleartext on exactly one of the two URLs. + expect(hasMetadataRedactor('datasource')).toBe(true); + expect(hasMetadataRedactor('datasources')).toBe(true); + }); + + it('registers no redactor for an ordinary type, and leaves its body by reference', () => { + expect(hasMetadataRedactor('view')).toBe(false); + const body = { name: 'v', label: 'V' }; + expect(redactMetadataItem('view', body)).toBe(body); + }); +}); + +describe('#8154 — the `_diagnostics` ordering is load-bearing (ruling 4)', () => { + it('the RAW stored body is `valid:false` and the REDACTED body is `valid:true`', () => { + // This is the whole reason the ordering is pinned rather than assumed, + // and it is what makes every `valid:false` assertion below an + // anti-vacuity arm: computing diagnostics AFTER redaction flips the + // verdict, because the redacted body is precisely the shape #8078's + // schema accepts. The #8081 item-3 operator inventory — "which rows + // still hold a stored credential" — is that `valid:false` badge, so an + // inverted ordering deletes the remedy while the leak looks fixed. + const raw = legacyDatasourceBody(); + expect(computeMetadataDiagnostics('datasource', raw)?.valid).toBe(false); + + const redacted = redactMetadataItem('datasource', raw); + expect(computeMetadataDiagnostics('datasource', redacted)?.valid).toBe(true); + + // …and the schema agrees directly, so the pin does not depend on the + // diagnostics wrapper keeping its current shape. + const schema = getMetadataTypeSchema('datasource')!; + expect(schema.safeParse(raw).success).toBe(false); + expect(schema.safeParse(redacted).success).toBe(true); + }); + + it('does not mutate the stored body — the connect path still reads cleartext', () => { + const raw = legacyDatasourceBody(); + const redacted = redactMetadataItem('datasource', raw) as any; + expect(redacted).not.toBe(raw); + expect(raw.config.password).toBe(INLINE_PASSWORD); + expect(raw.config.url).toBe(STORED_URL); + expect(redacted.config.password).toBeUndefined(); + expect(redacted.config.url).toBe(SERVED_URL); + }); +}); + +describe('#8154 — the read exits withhold the stored credential', () => { + it('getMetaItems (GET /api/v1/meta/datasources — the card`s named door)', async () => { + const { engine, rows } = makeStubEngine(); + seedLegacyRow(rows); + const protocol = new ObjectStackProtocolImplementation(engine); + + const res: any = await protocol.getMetaItems({ type: 'datasource' }); + const item = res.items.find((i: any) => i.name === 'warehouse'); + expect(item).toBeDefined(); + expect(item.config.password).toBeUndefined(); + expect(item.config.url).toBe(SERVED_URL); + expectNoCredential(res); + + // The migration-inventory badge survives — computed on the RAW body. + expect(item._diagnostics.valid).toBe(false); + + // …and the stored row is untouched: redaction is a SERVING act. + expect(storedBody(rows).config.password).toBe(INLINE_PASSWORD); + expect(storedBody(rows).config.url).toBe(STORED_URL); + }); + + it('getMetaItem (single-item read)', async () => { + const { engine, rows } = makeStubEngine(); + seedLegacyRow(rows); + const protocol = new ObjectStackProtocolImplementation(engine); + + const res: any = await protocol.getMetaItem({ type: 'datasource', name: 'warehouse' }); + expect(res.item.config.password).toBeUndefined(); + expect(res.item.config.url).toBe(SERVED_URL); + expect(res.item._diagnostics.valid).toBe(false); + expectNoCredential(res); + }); + + it('getMetaItemLayered — ALL THREE layers, `code` and `overlay` included', async () => { + // The exit `decorateMetadataItem` does not reach: this method computes + // `_diagnostics` itself and serves its layers raw. Before this change + // it returned `hunter2` in BOTH `overlay` and `effective`. + // + // Redacting `code`/`overlay` is a reading of #7556's deliberate + // rawness, stated in the PR body for its lane to contest: those layers + // are raw so a Studio diff shows what the tenant customised, and + // redacting the key on BOTH sides leaves that diff unchanged. + const { engine, rows } = makeStubEngine(); + seedLegacyRow(rows); + const protocol = new ObjectStackProtocolImplementation(engine); + + const res: any = await protocol.getMetaItemLayered({ type: 'datasource', name: 'warehouse' }); + expect(res.overlay).not.toBeNull(); + expect(res.overlay.config.password).toBeUndefined(); + expect(res.overlay.config.url).toBe(SERVED_URL); + expect(res.effective.config.password).toBeUndefined(); + expect(res.effective.config.url).toBe(SERVED_URL); + expectNoCredential(res); + + // The badge on this exit is computed from the raw effective body too. + expect(res._diagnostics.valid).toBe(false); + }); + + it('ABLATION — every assertion above goes red when the redactor is removed', async () => { + // Anti-vacuity arm 1. Overlay the registry entry with an identity + // redactor (the public extension seam), re-run the three exits, and + // watch the cleartext come back. If this block ever goes green, the + // blocks above are asserting nothing. + registerMetadataTypeRedactor('datasource', (item) => ({ item, redactedKeys: [] })); + + const { engine, rows } = makeStubEngine(); + seedLegacyRow(rows); + const protocol = new ObjectStackProtocolImplementation(engine); + + const list: any = await protocol.getMetaItems({ type: 'datasource' }); + expect(list.items[0].config.password).toBe(INLINE_PASSWORD); + + const one: any = await protocol.getMetaItem({ type: 'datasource', name: 'warehouse' }); + expect(one.item.config.url).toBe(STORED_URL); + + const layered: any = await protocol.getMetaItemLayered({ type: 'datasource', name: 'warehouse' }); + expect(layered.overlay.config.password).toBe(INLINE_PASSWORD); + expect(layered.effective.config.password).toBe(INLINE_PASSWORD); + }); +}); + +describe('#8154 — the write-path inverse (the read scrub may NOT ship alone)', () => { + it('the GET → edit → PUT round trip KEEPS the stored credential', async () => { + const { engine, rows } = makeStubEngine(); + seedLegacyRow(rows); + const protocol = new ObjectStackProtocolImplementation(engine); + + // Exactly what Studio holds: the SERVED (redacted) document. + const served: any = (await protocol.getMetaItem({ type: 'datasource', name: 'warehouse' })).item; + expect(served.config.password).toBeUndefined(); + + // The author edits one label and PUTs the whole body back. + await protocol.saveMetaItem({ + type: 'datasource', + name: 'warehouse', + item: { ...served, label: 'Warehouse (edited)' }, + }); + + const stored = storedBody(rows); + expect(stored.label).toBe('Warehouse (edited)'); + // Without the carry-forward, BOTH of these are gone — that is the + // silent credential deletion this card refuses to ship. + expect(stored.config.password).toBe(INLINE_PASSWORD); + expect(stored.config.url).toBe(STORED_URL); + }); + + it('an UNTOUCHED save persists a byte-identical body (#4326 invariant preserved)', async () => { + const { engine, rows } = makeStubEngine(); + seedLegacyRow(rows); + const protocol = new ObjectStackProtocolImplementation(engine); + + const before = storedBody(rows); + const served: any = (await protocol.getMetaItem({ type: 'datasource', name: 'warehouse' })).item; + await protocol.saveMetaItem({ type: 'datasource', name: 'warehouse', item: served }); + + expect(storedBody(rows)).toEqual(before); + }); + + it('the AUTHOR`s word wins — a changed URL is persisted verbatim, not carried over', async () => { + const { engine, rows } = makeStubEngine(); + seedLegacyRow(rows); + const protocol = new ObjectStackProtocolImplementation(engine); + + const served: any = (await protocol.getMetaItem({ type: 'datasource', name: 'warehouse' })).item; + await protocol.saveMetaItem({ + type: 'datasource', + name: 'warehouse', + item: { ...served, config: { ...served.config, url: 'postgresql://reporting@db2.internal:5432/warehouse' } }, + }); + + const stored = storedBody(rows); + expect(stored.config.url).toBe('postgresql://reporting@db2.internal:5432/warehouse'); + // The inline password was still untouched by the author, so it stays. + expect(stored.config.password).toBe(INLINE_PASSWORD); + }); + + it('does NOT launder a typed-in credential past #8078`s write gate', async () => { + const { engine, rows } = makeStubEngine(); + seedLegacyRow(rows); + const protocol = new ObjectStackProtocolImplementation(engine); + + const served: any = (await protocol.getMetaItem({ type: 'datasource', name: 'warehouse' })).item; + await expect(protocol.saveMetaItem({ + type: 'datasource', + name: 'warehouse', + item: { ...served, config: { ...served.config, password: 'typed-by-hand' } }, + })).rejects.toMatchObject({ code: 'INVALID_METADATA', status: 422 }); + + // Refused ⇒ the stored row is unchanged. + expect(storedBody(rows).config.password).toBe(INLINE_PASSWORD); + }); + + it('a DRAFT save of a legacy row carries the credential forward from the ACTIVE row', async () => { + // The load-bearing fallback: the first `?mode=draft` save has no draft + // row of its own to compare against, while the body the author edited + // came from the active row. Without it the draft drops the credential + // and `promoteDraft` later publishes that loss. + const { engine, rows } = makeStubEngine(); + seedLegacyRow(rows); + const protocol = new ObjectStackProtocolImplementation(engine); + + const served: any = (await protocol.getMetaItem({ type: 'datasource', name: 'warehouse' })).item; + await protocol.saveMetaItem({ + type: 'datasource', + name: 'warehouse', + mode: 'draft', + item: { ...served, label: 'Warehouse (draft)' }, + }); + + const draft = storedBody(rows, 'draft'); + expect(draft.label).toBe('Warehouse (draft)'); + expect(draft.config.password).toBe(INLINE_PASSWORD); + expect(draft.config.url).toBe(STORED_URL); + }); +}); + +describe('#8154 — carryForwardRedactedValues, the three outcomes', () => { + const stored = legacyDatasourceBody(); + + it('carries forward where the incoming body is INDISTINGUISHABLE from what was served', () => { + const served = redactMetadataItem('datasource', stored) as any; + const out: any = carryForwardRedactedValues('datasource', served, stored); + expect(out.config.password).toBe(INLINE_PASSWORD); + expect(out.config.url).toBe(STORED_URL); + }); + + it('leaves the author`s own value alone', () => { + const served = redactMetadataItem('datasource', stored) as any; + const incoming = { ...served, config: { ...served.config, password: 'new-one', url: 'postgresql://u@h:5432/w' } }; + const out: any = carryForwardRedactedValues('datasource', incoming, stored); + expect(out.config.password).toBe('new-one'); + expect(out.config.url).toBe('postgresql://u@h:5432/w'); + }); + + it('⛔ never MINTS a container the author removed', () => { + // A body with no `config` at all is an author deleting the container. + // Grafting `config.password` back would create a config holding + // nothing but a credential. + const incoming = { name: 'warehouse', label: 'Warehouse', driver: 'postgres' }; + const out: any = carryForwardRedactedValues('datasource', incoming, stored); + expect(out.config).toBeUndefined(); + }); + + it('does not mutate the incoming body, and is a no-op without a redactor', () => { + const served = redactMetadataItem('datasource', stored) as any; + const incoming = { ...served, config: { ...served.config } }; + const out = carryForwardRedactedValues('datasource', incoming, stored); + expect(out).not.toBe(incoming); + expect((incoming as any).config.password).toBeUndefined(); + + const view = { name: 'v', label: 'V' }; + expect(carryForwardRedactedValues('view', view, { name: 'v', secret: 'x' })).toBe(view); + }); +}); diff --git a/packages/metadata-protocol/src/protocol.ts b/packages/metadata-protocol/src/protocol.ts index 7df8947136..783f7f7f66 100644 --- a/packages/metadata-protocol/src/protocol.ts +++ b/packages/metadata-protocol/src/protocol.ts @@ -93,6 +93,16 @@ import { decorateMetadataItems, type MetadataDiagnostics, } from './metadata-diagnostics.js'; +// [#8154] The per-type read-path redaction seam. `decorateMetadataItem` already +// composes `redactMetadataItem` (see its ordering note), which covers the +// `getMetaItems` / `getMetaItem` exits; the two named here are the exits that +// decoration does not reach — `getMetaItemLayered`, which serves three RAW +// layers, and `saveMetaItem`, which owes the redaction its write-path inverse. +import { + carryForwardRedactedValues, + hasMetadataRedactor, + redactMetadataItem, +} from './metadata-redaction.js'; import type { StoredFlowCanonicalization, StoredMigrationNotice, @@ -4860,6 +4870,58 @@ export class ObjectStackProtocolImplementation implements return this.stripMaterializedFromRegistry(type, stripServedSystemColumns(type, item)); } + /** + * [#8154] The write-path counterpart the read exits' credential redaction + * owes, in the same sense {@link stripServedObjectColumns} is owed to + * {@link governServedObject}: a body this service's READ subtracted + * something from must not persist that subtraction when it comes straight + * back. Here the stakes are higher than a phantom customization — the + * subtracted material is a credential nobody can retype from the wire. + * + * Reads the row at rest through the overlay repository, whose `get()` is + * documented VERBATIM (no ADR-0087 conversion): the comparison must be + * against the bytes that were written, because those are the bytes the read + * exit redacted. {@link carryForwardRedactedValues} then decides per + * redacted path — see its docblock for the three outcomes. + * + * ⚠️ THE DRAFT FALLBACK IS LOAD-BEARING, not defensive. A `?mode=draft` + * save of an item with no draft row yet has nothing at its own state to + * compare against, while the body the author edited came from the ACTIVE + * row — so without the fallback the first draft save drops the credential, + * and `promoteDraft` later publishes that loss into the active row. The + * leak's write-side twin, one lifecycle over. + * + * ⛔ No `try`/`catch`. A failed stored-row read must fail the SAVE: the + * alternative is persisting a body whose credential we could not confirm + * was carried, which is the silent deletion this method exists to prevent. + * The read is skipped entirely — no extra query, for any save — when the + * type has no registered redactor, which is every type but `datasource` + * today. + */ + private async carryForwardRedactedCredentials(args: { + type: string; + repo: SysMetadataRepository; + ref: Parameters[0]; + state: 'draft' | 'active'; + packageId: string | null; + item: any; + }): Promise { + if (!hasMetadataRedactor(args.type)) return args.item; + let stored = await args.repo.get(args.ref, { + state: args.state, + packageId: args.packageId, + }); + if (!stored && args.state === 'draft') { + stored = await args.repo.get(args.ref, { + state: 'active', + packageId: args.packageId, + }); + } + const body = stored?.body; + if (!body) return args.item; + return carryForwardRedactedValues(args.type, args.item, body); + } + /** * [#5840] Read ONE item from the `metadata` service, keeping the ADR-0110 * D3 verdict instead of flattening it into `undefined`. @@ -5929,13 +5991,43 @@ export class ObjectStackProtocolImplementation implements const lockSource: any = code ?? overlay ?? {}; const lockState = resolveLockState(lockSource, artifactBacked); + // [#8154] The per-type credential redaction, on the ONE read exit + // `decorateMetadataItem` does not reach — this method never calls it + // (it computes `_diagnostics` directly, above) and serves its three + // layers raw. Measured on `origin/main` before this change: a legacy + // `datasource` row came back with `config.password` in cleartext in + // BOTH `overlay` and `effective`, so a hook that covered only the + // decorated exits would have left this door open while the report said + // "the read path is closed" — the exact shape this card exists to end. + // + // ⚠️ ALL THREE LAYERS, `code` and `overlay` included, and that is a + // reading of ANOTHER lane's ruling rather than an inference from this + // one. #7556 keeps `code` / `overlay` deliberately RAW — ungoverned, + // uninjected, unfolded — so that a Studio diff shows what the tenant + // actually customised and nothing else. That reason does not extend to + // credentials, and redacting the key on BOTH sides leaves the diff + // itself unchanged: the key is absent from both layers, so "what was + // customised" reads exactly as before. Stated here, and in the PR body, + // so `domain:engine-core` / `domain:spec` reviewers can contest it + // rather than discover it. ⛔ It is NOT a licence to fold, govern or + // inject on these layers — redaction subtracts, and only a credential. + // + // Placed AFTER `_diagnostics` and AFTER `resolveLockState`, both of + // which must read the raw bodies: the diagnostics ordering is the + // migration-inventory badge (see `decorateMetadataItem`), and a lock + // resolved from a redacted body would be a lock resolved from a + // document this method is not serving. + const servedCode = redactMetadataItem(request.type, code); + const servedOverlay = redactMetadataItem(request.type, overlay); + const servedEffective = redactMetadataItem(request.type, effective); + return { type: request.type, name: request.name, - code, - overlay, + code: servedCode, + overlay: servedOverlay, overlayScope, - effective, + effective: servedEffective, ...(_diagnostics ? { _diagnostics } : {}), lock: lockState.lock, ...(lockState.lockReason !== undefined ? { lockReason: lockState.lockReason } : {}), @@ -11802,6 +11894,46 @@ export class ObjectStackProtocolImplementation implements }); parentVersion = current?.hash ?? null; } + // [#8154] THE WRITE-PATH INVERSE of the read exits' credential + // redaction — the half without which this card's fix is a DATA-LOSS + // bug rather than a security fix. + // + // Measured on `origin/main` before this change: `saveMetaItem` ACCEPTS + // a redacted datasource body and persists the credential away. So the + // read scrub alone converts today's loud `422` (the stored cleartext + // `config.password` is refused by #8078's write gate) into SILENT + // credential DELETION on the ordinary `/meta` GET → edit → PUT round + // trip. `config.url` is what makes this unavoidable rather than a + // masking choice: a URL-embedded password is schema-ACCEPTED, so + // dropping it round-trips to deletion and masking it round-trips to + // storing the mask as the literal password. + // + // The generic form of the carry-forward PR #8126 added to + // `DatasourceAdminService.updateDatasource`, and placed for the same + // reason that one is: AFTER every gate, immediately before the put. + // The gates judge what the AUTHOR wrote, and #8078's refusal of an + // inline credential is aimed at exactly that; this restores material + // the author never saw, was never offered the chance to write, and is + // not asking to change. Running the schema gate over it would refuse a + // legacy row for the contents of its own stored config — i.e. keep + // today's 422 — and make an ordinary label edit unreachable on exactly + // the rows that need one. + // + // It also RESTORES the #4326 byte-identical round-trip invariant that + // read-redaction alone would have broken: an untouched GET → PUT of a + // legacy datasource now persists a body identical to the one at rest. + // + // ⛔ Preserves cleartext already at rest; creates none. Getting stored + // cleartext OUT of the store is #8081 item 3's migration, deliberately + // not attempted on a write door the author drove. + request.item = await this.carryForwardRedactedCredentials({ + type: singularTypeForRepo, + repo, + ref, + state: mode === 'draft' ? 'draft' : 'active', + packageId: request.packageId ?? null, + item: request.item, + }); try { const result = await repo.put(ref, request.item, { parentVersion,