From 99a75023d47be43f2d15e7ca31ba94d7fe4604c8 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 29 Sep 2026 07:22:33 +0000 Subject: [PATCH 1/7] test(service-automation,metadata-protocol,runtime): measuring pins for the remaining flow credential positions, red on the unfixed code Three measurements, committed before any fix so each one is shown red first: - service-automation: an enumeration pin reads every declared node config contract and asks the registered flow projection to withhold each credential-named key at every depth a node can sit (top level, loop body, parallel branch, try and catch regions). - runtime: the dispatcher's /meta list read, for a member-level caller, when the protocol's list read faults. - metadata-protocol and runtime: a round trip that keeps a node's id and changes its kind, read back afterwards. Claude-Session: https://claude.ai/code/session_01XY5uCwTjZj7884yYtyur4H Co-authored-by: Claude --- .../src/protocol.metadata-redaction.test.ts | 58 +++++ ...omation-flow-credential-projection.test.ts | 26 +++ .../domains/meta-list-protocol-fault.test.ts | 143 +++++++++++++ .../src/flow-credential-positions.test.ts | 202 ++++++++++++++++++ 4 files changed, 429 insertions(+) create mode 100644 packages/runtime/src/domains/meta-list-protocol-fault.test.ts create mode 100644 packages/services/service-automation/src/flow-credential-positions.test.ts diff --git a/packages/metadata-protocol/src/protocol.metadata-redaction.test.ts b/packages/metadata-protocol/src/protocol.metadata-redaction.test.ts index 41f55eee5ea..dc87741901c 100644 --- a/packages/metadata-protocol/src/protocol.metadata-redaction.test.ts +++ b/packages/metadata-protocol/src/protocol.metadata-redaction.test.ts @@ -726,3 +726,61 @@ describe('#20552 — first save of a registry-only (code-authored) flow keeps it expect(startNodeOf(JSON.parse(overlayRow(rows, 'active')!.metadata)).config.secret).toBe(ROTATED); }); }); + +// --------------------------------------------------------------------------- +// #20590 position 3 — the carry-forward never lands a credential where the +// projection does not withhold it +// --------------------------------------------------------------------------- +// +// The two halves pick positions by different rules. The read side withholds by +// the element's KIND (a flow's start node); the write side follows an array hop +// by the stored element's IDENTITY (`id`). An ordinary round trip that keeps a +// node's id and changes its kind therefore used to graft the stored credential +// onto a node the next read serves whole. The save door runs the carry-forward +// after every authoring gate, so no node-config check stood in the way there. + +/** The served body of the seeded flow, relocated: the old start node's kind changed, a new start node added. */ +function relocate(served: any) { + const [finish, begin] = served.nodes; + return { + ...served, + nodes: [ + finish, + { ...begin, type: 'assignment', label: 'Was the start node', config: {} }, + { id: 'begin_v2', type: 'start', label: 'On Webhook', config: { triggerType: 'api', hookId: 'intake' } }, + ], + edges: [{ id: 'e1', source: 'begin_v2', target: 'finish' }], + }; +} + +describe('#20590 — a relocating round trip never carries a credential into a served position', () => { + beforeEach(() => registerMetadataTypeRedactor('flow', flowStandInRedactor)); + + it('carryForwardRedactedValues drops a carried value the projection would not withhold at its new position', () => { + const stored = storedInboundFlow(); + const incoming = relocate(redactMetadataItem('flow', stored)); + const out: any = carryForwardRedactedValues('flow', incoming, stored); + // What the next read serves from the persisted body. + expect(allStrings(redactMetadataItem('flow', out))).not.toContain(FLOW_SECRET); + expect(allStrings(out)).not.toContain(FLOW_SECRET); + }); + + it('the save door: the next served read of a relocated flow carries no credential', async () => { + const { engine, rows } = makeStubEngine(); + seedFlowRow(rows); + const protocol = new ObjectStackProtocolImplementation(engine); + + const served: any = (await protocol.getMetaItem({ type: 'flow', name: 'inbound_hook' })).item; + const { _diagnostics: _d, ...editable } = served; + void _d; + await protocol.saveMetaItem({ type: 'flow', name: 'inbound_hook', item: relocate(editable) }); + + const next: any = await protocol.getMetaItem({ type: 'flow', name: 'inbound_hook' }); + expect(next.item.nodes.map((n: any) => n.id)).toEqual(['finish', 'begin', 'begin_v2']); + expect(allStrings(next)).not.toContain(FLOW_SECRET); + const list: any = await protocol.getMetaItems({ type: 'flow' }); + expect(allStrings(list)).not.toContain(FLOW_SECRET); + // Nothing was grafted at rest either: the relocated node holds no credential. + expect(allStrings(storedFlowBody(rows))).not.toContain(FLOW_SECRET); + }); +}); diff --git a/packages/runtime/src/domains/automation-flow-credential-projection.test.ts b/packages/runtime/src/domains/automation-flow-credential-projection.test.ts index 18ef6668a3f..ae3086b2c88 100644 --- a/packages/runtime/src/domains/automation-flow-credential-projection.test.ts +++ b/packages/runtime/src/domains/automation-flow-credential-projection.test.ts @@ -164,3 +164,29 @@ describe('#20552 — anti-vacuity: the door serves exactly what the registry ent expect(JSON.stringify(result.response?.body)).toContain(SECRET); }); }); + +describe('#20590 — a relocating PUT never lands the secret where the next read serves it', () => { + it('the start node’s kind changed and a new start node added: the member’s next read carries no credential', async () => { + const { dispatcher, spies } = makeDispatcher(); + const served = dataOf(await dispatcher.handleAutomation('/inbound_hook', 'GET', undefined, MEMBER)); + const [finish, begin] = served.nodes; + const relocated = { + ...served, + nodes: [ + finish, + { ...begin, type: 'assignment', label: 'Was the start node', config: {} }, + { id: 'begin_v2', type: 'start', label: 'On Webhook', config: { triggerType: 'api', hookId: 'intake' } }, + ], + edges: [{ id: 'e1', source: 'begin_v2', target: 'finish' }], + }; + + const put = await dispatcher.handleAutomation('/inbound_hook', 'PUT', relocated, AUTHOR); + expect(put.response?.status).toBe(200); + const next = await dispatcher.handleAutomation('/inbound_hook', 'GET', undefined, MEMBER); + expect(next.response?.status).toBe(200); + expect(dataOf(next).nodes.map((n: any) => n.id)).toEqual(['finish', 'begin', 'begin_v2']); + expect(JSON.stringify(next.response?.body)).not.toContain(SECRET); + // …and nothing was grafted into what the engine was handed. + expect(JSON.stringify(spies.registerFlow.mock.calls.at(-1)![1])).not.toContain(SECRET); + }); +}); diff --git a/packages/runtime/src/domains/meta-list-protocol-fault.test.ts b/packages/runtime/src/domains/meta-list-protocol-fault.test.ts new file mode 100644 index 00000000000..4c64485d587 --- /dev/null +++ b/packages/runtime/src/domains/meta-list-protocol-fault.test.ts @@ -0,0 +1,143 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #20590 position 2 — the dispatcher's `/meta/:type` list answers a protocol + * FAULT as that fault. It never falls through to the metadata service's raw + * list, which applies no per-type read-path redaction. + * + * ## What the protocol's throw means, measured + * + * `ObjectStackProtocolImplementation.getMetaItems` (the one implementation in + * this repository) answers a type it holds nothing for with `{ items: [] }`: + * it merges the metadata service's runtime-registered items (agents, tools) + * into its own answer, so no type is "unknown" to it in a way it signals by + * throwing. What it throws is a fault or a refusal: the store read failed + * (`503 SERVICE_UNAVAILABLE`, or a metadata app's marked refusal), a + * registered redactor threw (fail-closed by design, `redactMetadataItem`), or + * the segment is an unrecognised spelling of a declared type + * (`400 INVALID_REQUEST`). The branch used to swallow all of them and serve + * `metadataService.list(type)` instead — the stored bodies, so a flow's hook + * secret and a datasource's password went out to a member-level caller on + * any protocol fault. `RestServer`'s list route has no such fallback and + * answers the same throw as itself. + * + * The fallback itself stays, for the host shape it exists for: a protocol + * slot with no list verb at all. + */ + +import { describe, it, expect, vi } from 'vitest'; +import { HttpDispatcher } from '../http-dispatcher.js'; + +const HOOK_SECRET = 'stored-hook-secret-20590'; +const DB_PASSWORD = 'stored-db-password-20590'; + +const STORED: Record = { + flow: [{ + name: 'inbound_hook', + label: 'Inbound hook', + type: 'api', + nodes: [{ id: 'begin', type: 'start', label: 'Start', config: { triggerType: 'api', secret: HOOK_SECRET } }], + edges: [], + }], + datasource: [{ name: 'warehouse', label: 'Warehouse', driver: 'postgres', config: { host: 'db', password: DB_PASSWORD } }], + agent: [{ name: 'triage_agent', label: 'Triage agent' }], +}; + +const clone = (v: T): T => JSON.parse(JSON.stringify(v)); +const singular = (type: unknown): string => String(type ?? '').replace(/s$/, ''); + +/** A member-level caller: authenticated, and nothing else. */ +const MEMBER = { userId: 'u_member', isSystem: false, systemPermissions: [] as string[] }; + +/** The store fault the protocol raises when its `sys_metadata` read fails (`metadataStoreUnavailableError`). */ +function storeFault(): Error { + return Object.assign(new Error('The metadata store could not be read, so whether this item exists is unknown.'), { + code: 'SERVICE_UNAVAILABLE', + status: 503, + }); +} + +function boot(protocol: Record) { + const metadata = { list: vi.fn(async (type: string) => clone(STORED[singular(type)] ?? [])) }; + const services: Record = { + protocol, + metadata, + security: { resolvePermissionSetNames: async () => [], getMetadataReadableFields: async () => [] }, + }; + const get = (n: string) => services[n] ?? null; + const kernel: any = { context: { getService: get }, getService: get, getServiceAsync: async (n: string) => get(n) }; + const dispatcher = new HttpDispatcher(kernel); + (dispatcher as any).timedResolveExecutionContext = async () => clone(MEMBER); + const list = async (type: string) => { + const res = await dispatcher.dispatch('GET', `/meta/${type}`, undefined, {}, { request: { headers: {} } } as any); + return { status: res.response?.status ?? 0, body: res.response?.body }; + }; + return { list, metadata }; +} + +const text = (v: unknown) => JSON.stringify(v ?? null); + +describe('#20590 — a protocol fault on the list read is answered as itself', () => { + for (const [type, credential] of [['flow', HOOK_SECRET], ['datasource', DB_PASSWORD]] as const) { + it(`${type}: a store fault answers 503 SERVICE_UNAVAILABLE and never the stored bodies`, async () => { + const protocol = { + getMetaTypes: vi.fn(async () => ({ types: Object.keys(STORED) })), + getMetaItems: vi.fn(async () => { throw storeFault(); }), + }; + const { list, metadata } = boot(protocol); + const res = await list(type); + expect(text(res.body)).not.toContain(credential); + expect({ status: res.status, code: res.body?.error?.code }).toEqual({ status: 503, code: 'SERVICE_UNAVAILABLE' }); + expect(metadata.list).not.toHaveBeenCalled(); + }); + } + + it('a protocol refusal keeps its own status and code (400 INVALID_REQUEST)', async () => { + const protocol = { + getMetaTypes: vi.fn(async () => ({ types: Object.keys(STORED) })), + getMetaItems: vi.fn(async () => { + throw Object.assign(new Error('not a recognised spelling of a declared metadata type'), { code: 'INVALID_REQUEST', status: 400 }); + }), + }; + const { list, metadata } = boot(protocol); + const res = await list('flow'); + expect({ status: res.status, code: res.body?.error?.code }).toEqual({ status: 400, code: 'INVALID_REQUEST' }); + expect(text(res.body)).not.toContain(HOOK_SECRET); + expect(metadata.list).not.toHaveBeenCalled(); + }); + + it('an undeclared throw (a redactor failing closed) is a 500, never the unredacted list', async () => { + const protocol = { + getMetaTypes: vi.fn(async () => ({ types: Object.keys(STORED) })), + getMetaItems: vi.fn(async () => { throw new Error('redactor for flow threw'); }), + }; + const { list, metadata } = boot(protocol); + const res = await list('flow'); + expect(res.status).toBe(500); + expect(text(res.body)).not.toContain(HOOK_SECRET); + expect(metadata.list).not.toHaveBeenCalled(); + }); +}); + +describe('#20590 — what still reaches the metadata service fallback', () => { + it('a type the protocol holds nothing for is answered with its empty list — the protocol’s own "unknown" answer', async () => { + const protocol = { + getMetaTypes: vi.fn(async () => ({ types: Object.keys(STORED) })), + getMetaItems: vi.fn(async () => ({ items: [] })), + }; + const { list, metadata } = boot(protocol); + const res = await list('agent'); + expect(res.status).toBe(200); + expect(res.body?.data?.items ?? res.body?.data).toEqual([]); + expect(metadata.list).not.toHaveBeenCalled(); + }); + + it('a protocol slot with no list verb still lists the metadata service’s runtime-registered items', async () => { + const protocol = { getMetaTypes: vi.fn(async () => ({ types: Object.keys(STORED) })) }; + const { list, metadata } = boot(protocol); + const res = await list('agent'); + expect(res.status).toBe(200); + expect(metadata.list).toHaveBeenCalledWith('agent'); + expect(text(res.body)).toContain('triage_agent'); + }); +}); diff --git a/packages/services/service-automation/src/flow-credential-positions.test.ts b/packages/services/service-automation/src/flow-credential-positions.test.ts new file mode 100644 index 00000000000..64bc984e8d9 --- /dev/null +++ b/packages/services/service-automation/src/flow-credential-positions.test.ts @@ -0,0 +1,202 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #20590 — every credential position a flow definition can hold is withheld by + * the registered `flow` redactor, at every depth a node can sit. + * + * #20552 withheld one position: the start node's inbound-hook secret. This + * file is the family's enumeration pin. It does not list the positions by + * hand. It READS them from the node config contracts the platform declares — + * every builtin executor's descriptor `configSchema` (the vocabulary + * `registerFlow` checks a node's `config` against), the schemaless builtins' + * spec Zod contracts, and the approval node's — and asks the one projection to + * withhold each credential-named key it finds. So a node kind that declares a + * new credential key turns this file red until the projection covers it, or + * until the key is reviewed into {@link NOT_A_CREDENTIAL} with a reason. + * + * "Every depth": an ADR-0031 container (`loop`, `parallel`, `try_catch`) holds + * whole sub-graphs inside its own `config` (`FLOW_REGION_SLOTS`), so an `http` + * callout inside a loop body is a node whose definition is served with the + * flow like any other. A projection that walked only the top-level `nodes` + * would serve every nested one. + */ + +import { describe, it, expect } from 'vitest'; +import { + FLOW_REGION_SLOTS_BY_TYPE, + getApprovalNodeConfigJsonSchema, + getSchemalessNodeConfigJsonSchemas, +} from '@objectstack/spec/automation'; +import { AutomationEngine } from './engine.js'; +import { installBuiltinNodes } from './builtin/index.js'; +import { redactFlowCredentials } from './flow-credential-projection.js'; + +function silentLogger(): any { + return { info() {}, warn() {}, error() {}, debug() {}, child() { return silentLogger(); } }; +} + +/** The node config contracts the platform declares, keyed by `node.type`. */ +function declaredNodeConfigSchemas(): Map { + const engine = new AutomationEngine(silentLogger()); + installBuiltinNodes(engine, { logger: silentLogger(), getService() { throw new Error('none'); } } as any); + const out = new Map(); + for (const descriptor of engine.getActionDescriptors()) { + if (descriptor.configSchema) out.set(descriptor.type, descriptor.configSchema); + } + for (const [type, schema] of Object.entries(getSchemalessNodeConfigJsonSchemas())) { + if (!out.has(type)) out.set(type, schema); + } + out.set('approval', getApprovalNodeConfigJsonSchema()); + return out; +} + +function isRecord(value: unknown): value is Record { + return !!value && typeof value === 'object' && !Array.isArray(value); +} + +/** + * Every declared property path under one node type's `config`, as dotted + * segments (`*` for an array item or an open map's value). A container's + * region slots are not descended into: what they hold is nodes, each with its + * own type and its own contract, which this enumeration already reads. + */ +function declaredConfigPaths(nodeType: string, root: unknown): string[][] { + const regionKeys = new Set((FLOW_REGION_SLOTS_BY_TYPE.get(nodeType) ?? []).map((slot) => slot.key)); + const defs = isRecord(root) && isRecord(root.$defs) ? root.$defs : {}; + const out: string[][] = []; + const seen = new Set(); + const walk = (schema: unknown, prefix: string[]): void => { + if (!isRecord(schema)) return; + if (typeof schema.$ref === 'string' && schema.$ref.startsWith('#/$defs/')) { + const target = defs[schema.$ref.slice('#/$defs/'.length)]; + if (seen.has(target)) return; + seen.add(target); + walk(target, prefix); + return; + } + for (const branch of ['anyOf', 'oneOf', 'allOf'] as const) { + const list = schema[branch]; + if (Array.isArray(list)) for (const sub of list) walk(sub, prefix); + } + if (isRecord(schema.properties)) { + for (const [key, sub] of Object.entries(schema.properties)) { + const path = [...prefix, key]; + out.push(path); + if (prefix.length === 0 && regionKeys.has(key)) continue; + walk(sub, path); + } + } + if (isRecord(schema.items)) walk(schema.items, [...prefix, '*']); + if (isRecord(schema.additionalProperties)) walk(schema.additionalProperties, [...prefix, '*']); + }; + walk(root, []); + return out; +} + +/** + * The credential-name vocabulary. Deliberately broad: under-redacting is the + * dangerous direction, so a key this matches must be covered by the + * projection or reviewed out below — a false positive costs one line with a + * reason, a false negative serves a secret. + */ +const CREDENTIAL_NAME = /secret|passw(?:or)?d|pwd|token|api[-_]?key|access[-_]?key|private[-_]?key|credential|bearer|signing/i; + +/** + * Declared keys the vocabulary matches that are NOT credentials, each with the + * reason it was reviewed out. Empty today; an entry needs its reason. + */ +const NOT_A_CREDENTIAL: ReadonlyMap = new Map([]); + +/** + * The positions no schema declares, with their authority. The start node's + * `config` is an open record (FlowNodeSchema), so its inbound-hook secret is + * declared by ADR-0041 and enforced by the engine's `validateApiTriggerSecret` + * rather than read off a contract. + */ +const SCHEMALESS_POSITIONS: ReadonlyArray = [['start', 'secret']]; + +function credentialPositions(): Array { + const found: Array = [...SCHEMALESS_POSITIONS]; + for (const [nodeType, schema] of declaredNodeConfigSchemas()) { + for (const path of declaredConfigPaths(nodeType, schema)) { + const key = path[path.length - 1]!; + if (key === '*' || !CREDENTIAL_NAME.test(key)) continue; + const id = `${nodeType}.${path.join('.')}`; + if (NOT_A_CREDENTIAL.has(id)) continue; + found.push([nodeType, path.join('.')] as const); + } + } + return found; +} + +const SENTINEL = 'credential-position-sentinel-20590'; + +/** A node of `nodeType` carrying the sentinel at `configKey`, with a unique id. */ +function nodeWith(id: string, nodeType: string, configKey: string): Record { + return { id, type: nodeType, label: id, config: { [configKey]: SENTINEL } }; +} + +/** The same credential-bearing node, placed at each depth a node can sit. */ +function placements(nodeType: string, configKey: string): Array<{ where: string; flow: Record }> { + const inner = () => nodeWith('inner', nodeType, configKey); + const tail = { id: 'tail', type: 'assignment', label: 'Tail', config: {} }; + const shell = (node: Record) => ({ + name: 'positions_20590', + label: 'Positions', + type: 'autolaunched', + nodes: [{ id: 'begin', type: 'start', label: 'Start', config: {} }, node], + edges: [], + }); + return [ + { where: 'top level', flow: shell(inner()) }, + { + where: 'a loop body', + flow: shell({ id: 'box', type: 'loop', label: 'Loop', config: { collection: '{items}', body: { nodes: [inner()], edges: [] } } }), + }, + { + where: 'a parallel branch', + flow: shell({ + id: 'box', type: 'parallel', label: 'Parallel', + config: { branches: [{ name: 'a', nodes: [tail] }, { name: 'b', nodes: [inner()] }] }, + }), + }, + { + where: 'a try region', + flow: shell({ id: 'box', type: 'try_catch', label: 'Try', config: { try: { nodes: [inner()], edges: [] } } }), + }, + { + where: 'a catch region', + flow: shell({ + id: 'box', type: 'try_catch', label: 'Try', + config: { try: { nodes: [tail], edges: [] }, catch: { nodes: [inner()], edges: [] } }, + }), + }, + ]; +} + +describe('#20590 — the enumeration: every declared credential position is withheld', () => { + const positions = credentialPositions(); + + it('reads a real population — the walk is not vacuous', () => { + // Anti-vacuity: the declared universe is large, and the `http` + // node's outbound HMAC secret is in it (HttpConfigSchema, + // `signingSecret`). A walk that stopped reading schemas would find + // only the hand-listed start position and pass everything below. + const universe = [...declaredNodeConfigSchemas()].flatMap(([t, s]) => declaredConfigPaths(t, s).map((p) => `${t}.${p.join('.')}`)); + expect(universe.length).toBeGreaterThan(40); + expect(positions.map(([t, k]) => `${t}.${k}`)).toContain('http.signingSecret'); + }); + + for (const [nodeType, configKey] of credentialPositions()) { + for (const { where, flow } of placements(nodeType, configKey)) { + it(`withholds ${nodeType}.config.${configKey} at ${where}`, () => { + const before = structuredClone(flow); + const { item, redactedKeys } = redactFlowCredentials(flow); + expect(JSON.stringify(item)).not.toContain(SENTINEL); + expect(redactedKeys.length).toBeGreaterThan(0); + // Pure: the stored body the engine executes keeps it. + expect(flow).toEqual(before); + }); + } + } +}); From 1f17293b2ae2ac30910e1c61aa8d2b176d8e0df0 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 29 Sep 2026 07:27:52 +0000 Subject: [PATCH 2/7] fix(service-automation,metadata-protocol,runtime): withhold every flow credential position at every depth, keep a carried value off served positions, and answer a protocol list fault as itself - service-automation: the flow projection withholds an http node's config.signingSecret beside the start node's config.secret, through every loop, parallel and try/catch region. The positions are one table by node kind; the empty string is the explicit clearing value and is served as written, so an optional secret can be removed across a round trip whose absent key means "unchanged". - metadata-protocol: the write-path inverse re-runs the type's redactor over what it grafted and drops a carried value whose new position the read would serve. An array element with no id (a parallel branch) is walked by the identified node beneath it on the same path. - runtime: the /meta list read no longer swallows a protocol throw and serves the metadata service's unredacted list; the throw is answered as itself. Claude-Session: https://claude.ai/code/session_01XY5uCwTjZj7884yYtyur4H Co-authored-by: Claude --- .../src/metadata-redaction.ts | 169 +++++++++++--- .../src/protocol.metadata-redaction.test.ts | 191 ++++++++++++++++ .../meta-list-read-gate-parity.test.ts | 17 +- packages/runtime/src/domains/meta.ts | 18 +- .../src/flow-credential-positions.test.ts | 83 ++++++- .../src/flow-credential-projection.ts | 212 +++++++++++++----- 6 files changed, 592 insertions(+), 98 deletions(-) diff --git a/packages/metadata-protocol/src/metadata-redaction.ts b/packages/metadata-protocol/src/metadata-redaction.ts index 0bff2b3f871..08c3ac98fca 100644 --- a/packages/metadata-protocol/src/metadata-redaction.ts +++ b/packages/metadata-protocol/src/metadata-redaction.ts @@ -171,68 +171,135 @@ function elementWithIdentity( * reorders a flow's `nodes` sends the same start node back at another index, * and grafting by position would put its credential on whichever node now sits * where it used to. So an array hop is resolved ONCE, against the stored body, - * into the element's `id`, and every body — served and incoming — is then - * walked by that identity, never by the index. + * into an identity, and every body — served and incoming — is then walked by + * that identity, never by the index. Two kinds of identity: * - * An array hop whose stored element carries no string `id`, or shares it with - * a sibling, resolves to nothing: the path is skipped, exactly as every array - * hop was skipped before identity resolution existed. + * - **the element's own `id`** (`{ elementId }`) — a flow node; + * - **for an element with no `id`, the identified element below it on the + * same path** (`{ anchor }`, #20590). A `parallel` block's branch carries no + * `id`, and a credential inside one sits at + * `nodes..config.branches..nodes..config.signingSecret`. The branch + * is the one element of `branches` whose own walk down the rest of the + * path reaches node ``'s `id` — and a flow's node ids are one space + * across every region (`FlowSchema`), so at most one branch does. `anchor` + * is that walk: the hops from inside the element down to, and including, + * the first identified element beneath it. + * + * An array hop that reaches neither — an element with no `id` and no + * identified element below it on the path, or an `id` shared with a sibling — + * resolves to nothing, and the path is skipped. */ -type PathHop = { readonly key: string } | { readonly elementId: string }; +type PathHop = + | { readonly key: string } + | { readonly elementId: string } + | { readonly anchor: readonly PathHop[] }; /** * Resolve every CONTAINER hop of `segments` (all but the last, which names the * redacted key itself) against `stored`. `undefined` when the stored body does - * not reach that far — which the caller reads as "nothing at rest to carry". + * not reach that far, or an array hop has no identity — which the caller reads + * as "nothing at rest to carry". */ function resolveHops(stored: unknown, segments: readonly string[]): PathHop[] | undefined { - const hops: PathHop[] = []; + // First pass: key hops and `id` hops; `null` marks an element with no `id`. + const found: Array = []; let node: unknown = stored; for (let i = 0; i < segments.length - 1; i += 1) { const segment = segments[i] as string; if (Array.isArray(node)) { if (!/^(0|[1-9][0-9]*)$/.test(segment)) return undefined; const element = node[Number(segment)]; + if (!isPlainRecord(element)) return undefined; const elementId = identityOf(element); - if (elementId === undefined || !elementWithIdentity(node, elementId)) return undefined; - hops.push({ elementId }); + if (elementId !== undefined && !elementWithIdentity(node, elementId)) return undefined; + found.push(elementId === undefined ? null : { elementId }); node = element; continue; } if (!isPlainRecord(node)) return undefined; - hops.push({ key: segment }); + found.push({ key: segment }); node = node[segment]; } + // Second pass, from the end: anchor each id-less element on the first + // identified element below it. The anchor may itself cross an id-less + // element (a branch inside a branch), whose own anchor is already built. + const hops: PathHop[] = new Array(found.length); + let nextIdentified = -1; + for (let i = found.length - 1; i >= 0; i -= 1) { + const hop = found[i]; + if (hop === null) { + if (nextIdentified < 0) return undefined; + hops[i] = { anchor: hops.slice(i + 1, nextIdentified + 1) }; + continue; + } + hops[i] = hop as PathHop; + if ('elementId' in (hop as PathHop)) nextIdentified = i; + } return hops; } +/** + * Resolve ONE hop against `node`: the segment it stands for in THIS body (a + * key, or the element's index here) and the value it leads to. `undefined` + * when this body does not speak to the hop — the wrong kind of container, or + * an array hop no single element answers (none does, or two do and cannot be + * told apart: guessing would graft a credential onto whichever came first). + */ +function stepInto(node: unknown, hop: PathHop): { segment: string; next: unknown } | undefined { + if ('key' in hop) { + return isPlainRecord(node) ? { segment: hop.key, next: node[hop.key] } : undefined; + } + if (!Array.isArray(node)) return undefined; + let index: number | undefined; + for (let i = 0; i < node.length; i += 1) { + const hit = 'elementId' in hop + ? identityOf(node[i]) === hop.elementId + : isPlainRecord(node[i]) && containerAt(node[i], hop.anchor) !== undefined; + if (!hit) continue; + if (index !== undefined) return undefined; + index = i; + } + return index === undefined ? undefined : { segment: String(index), next: node[index] }; +} + /** * Walk to the plain object that OWNS the redacted key, hop by hop. * * `undefined` when any hop along the way is absent, is the wrong kind of - * container, or (for an array hop) holds no single element with that identity - * — 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 + * container, or (for an array hop) is answered by no single element — 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, hops: readonly PathHop[]): Record | undefined { let node: unknown = root; for (const hop of hops) { - if ('key' in hop) { - if (!isPlainRecord(node)) return undefined; - node = node[hop.key]; - continue; - } - if (!Array.isArray(node)) return undefined; - const found = elementWithIdentity(node, hop.elementId); - if (!found) return undefined; - node = found.element; + const step = stepInto(node, hop); + if (!step) return undefined; + node = step.next; } return isPlainRecord(node) ? node : undefined; } +/** + * The dotted, index-addressed path `hops` + `key` names in `root` — the + * spelling a redactor's `redactedKeys` uses for that same body. Called only + * where {@link containerAt} found the container, so every hop resolves. + */ +function pathAt(root: unknown, hops: readonly PathHop[], key: string): string { + const segments: string[] = []; + let node: unknown = root; + for (const hop of hops) { + const step = stepInto(node, hop) as { segment: string; next: unknown }; + segments.push(step.segment); + node = step.next; + } + segments.push(key); + return segments.join('.'); +} + /** * Copy-on-write set of `value` under `key` at the end of `hops`, returning a * new root and copying only the containers along the path — an array hop @@ -246,14 +313,12 @@ function containerAt(root: unknown, hops: readonly PathHop[]): Record), [key]: value }; const [hop, ...rest] = hops as [PathHop, ...PathHop[]]; + const step = stepInto(root, hop) as { segment: string; next: unknown }; if ('key' in hop) { - const record = root as Record; - return { ...record, [hop.key]: withValueAt(record[hop.key], rest, key, value) }; + return { ...(root as Record), [hop.key]: withValueAt(step.next, rest, key, value) }; } - const array = root as unknown[]; - const found = elementWithIdentity(array, hop.elementId) as { index: number; element: Record }; - const next = array.slice(); - next[found.index] = withValueAt(found.element, rest, key, value); + const next = (root as unknown[]).slice(); + next[Number(step.segment)] = withValueAt(step.next, rest, key, value); return next; } @@ -295,8 +360,21 @@ function sameValue(a: unknown, b: unknown): boolean { * A path through an ARRAY (a flow's start node, `nodes..config.secret`, * #20552) is walked by the stored element's `id`, not by its index, so a body * that reorders the array still carries the value onto the element it came - * from — see {@link resolveHops}. An element with no `id`, or an `id` shared - * with a sibling, is never carried into. + * from — see {@link resolveHops}. An element with no `id` is walked by the + * identified element below it on the same path (a `parallel` branch, by the + * node inside it that holds the credential, #20590); one with neither, or an + * `id` shared with a sibling, is never carried into. + * + * ⛔ A carried value never lands where the read would SERVE it (#20590). The + * array hop follows an identity, while a redactor chooses what to withhold by + * whatever rule it has — a flow's by the node's KIND — and the two can + * disagree: an edit that keeps a node's `id` and changes its kind would have + * the stored credential grafted onto a node the next read serves whole. So + * the type's redactor is run over the grafted body, and a carried value whose + * path it no longer withholds is dropped rather than persisted. Changing the + * kind of the node that held a credential is the author's word about that + * credential: it is gone, and a kind that needs one asks for it again (an + * `api` flow's start node without a secret is refused at registration). * * ⚠️ The first case is genuinely INDISTINGUISHABLE, not merely treated as * equal: an author who hand-deletes `:password` from a URL sends exactly the @@ -305,7 +383,10 @@ function sameValue(a: unknown, b: unknown): boolean { * 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. + * unambiguous door: change it, or delete the row — or, where the type's + * redactor serves one value as written because it holds no credential, send + * that value (a flow's empty string, #20590): it differs from the absent key + * that was served, so it is the author's word and it wins. * * @param type request-shaped metadata type (plural or singular). * @param incoming the body about to be persisted. @@ -324,7 +405,7 @@ export function carryForwardRedactedValues(type: string, incoming: T, stored: const served = redactor(stored as Record); if (served.redactedKeys.length === 0) return incoming; - let out = incoming as unknown as Record; + const grafts: Array<{ hops: PathHop[]; key: string; value: unknown }> = []; for (const path of served.redactedKeys) { // Dotted, item-relative — the registry's documented contract for // `redactedKeys` (`config.password`; an array hop is an index into the @@ -339,13 +420,27 @@ export function carryForwardRedactedValues(type: string, incoming: T, stored: const storedValue = storedParent?.[key]; if (storedValue === undefined) continue; - const incomingParent = containerAt(out, hops); + const incomingParent = containerAt(incoming, hops); if (!incomingParent) continue; const servedParent = containerAt(served.item, hops); if (!sameValue(incomingParent[key], servedParent?.[key])) continue; - out = withValueAt(out, hops, key, storedValue) as Record; + grafts.push({ hops, key, value: storedValue }); + } + + // [#20590] Keep only what the read would withhold where it now lands. A + // graft sets a leaf and moves no container, so each pass re-grafts the + // survivors onto the untouched incoming body; the set only shrinks, so + // this settles in at most one pass per graft. + let kept = grafts; + for (;;) { + let out: unknown = incoming; + for (const graft of kept) out = withValueAt(out, graft.hops, graft.key, graft.value); + if (kept.length === 0) return out as T; + const withheld = new Set(redactor(out as Record).redactedKeys); + const next = kept.filter((graft) => withheld.has(pathAt(out, graft.hops, graft.key))); + if (next.length === kept.length) return out as T; + kept = next; } - 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 index dc87741901c..2f9311f89e6 100644 --- a/packages/metadata-protocol/src/protocol.metadata-redaction.test.ts +++ b/packages/metadata-protocol/src/protocol.metadata-redaction.test.ts @@ -784,3 +784,194 @@ describe('#20590 — a relocating round trip never carries a credential into a s expect(allStrings(storedFlowBody(rows))).not.toContain(FLOW_SECRET); }); }); + +// --------------------------------------------------------------------------- +// #20590 position 1 — a credential inside a REGION, and the removal door +// --------------------------------------------------------------------------- +// +// The real `flow` projection (service-automation `flow-credential-projection.ts`) +// withholds an `http` node's `config.signingSecret` and a start node's +// `config.secret` at every depth, and serves the cleared form (`''`) as +// written. This stand-in emits the SAME path shapes that projection is pinned to +// emit in `flow-credential-positions.test.ts` +// (`nodes..config.body.nodes..config.signingSecret`, +// `nodes..config.branches..nodes..config.signingSecret`), so what is +// exercised here is this package's half: the inverse walks those paths. + +const SIGNING = 'stored-signing-secret-20590'; +const ROTATED_SIGNING = 'rotated-signing-secret-20590'; +const STAND_IN_KEYS: Record = { start: 'secret', http: 'signingSecret' }; +const STAND_IN_REGIONS: Record> = { + loop: [['body', 'one']], + parallel: [['branches', 'many']], + try_catch: [['try', 'one'], ['catch', 'one']], +}; + +const regionFlowStandIn = (item: Record) => { + const redactedKeys: string[] = []; + const walkNodes = (nodes: any[], path: string): any[] => nodes.map((node, i) => { + if (!node || typeof node !== 'object' || !node.config || typeof node.config !== 'object') return node; + const at = `${path}.${i}`; + const config = { ...node.config }; + const key = STAND_IN_KEYS[node.type]; + if (key && key in config && config[key] !== '') { + delete config[key]; + redactedKeys.push(`${at}.config.${key}`); + } + for (const [slot, arity] of STAND_IN_REGIONS[node.type] ?? []) { + const value = config[slot]; + if (arity === 'one' && value?.nodes) config[slot] = { ...value, nodes: walkNodes(value.nodes, `${at}.config.${slot}.nodes`) }; + if (arity === 'many' && Array.isArray(value)) { + config[slot] = value.map((r: any, b: number) => (r?.nodes ? { ...r, nodes: walkNodes(r.nodes, `${at}.config.${slot}.${b}.nodes`) } : r)); + } + } + return { ...node, config }; + }); + if (!Array.isArray(item.nodes)) return { item, redactedKeys }; + const nodes = walkNodes(item.nodes as any[], 'nodes'); + return redactedKeys.length === 0 ? { item, redactedKeys } : { item: { ...item, nodes }, redactedKeys: redactedKeys.sort() }; +}; + +/** A flow whose outbound callouts sit inside a loop body and a parallel branch — the branch deliberately not first. */ +function storedCalloutFlow() { + return { + name: 'callouts', + label: 'Callouts', + type: 'autolaunched', + nodes: [ + { id: 'begin', type: 'start', label: 'Start', config: {} }, + { + id: 'each', type: 'loop', label: 'Each', + config: { + collection: '{rows}', + body: { nodes: [{ id: 'per_row', type: 'http', label: 'Per row', config: { url: 'https://a', durable: true, signingSecret: SIGNING } }], edges: [] }, + }, + }, + { + id: 'fan', type: 'parallel', label: 'Fan', + config: { + branches: [ + { name: 'quiet', nodes: [{ id: 'note', type: 'assignment', label: 'Note', config: {} }] }, + { name: 'loud', nodes: [{ id: 'push', type: 'http', label: 'Push', config: { url: 'https://b', durable: true, signingSecret: SIGNING } }] }, + ], + }, + }, + ], + edges: [], + }; +} + +const calloutOf = (flow: any, id: string): any => { + let hit: any; + const visit = (nodes: any[]) => nodes.forEach((n) => { + if (n?.id === id) hit = n; + const c = n?.config ?? {}; + if (c.body?.nodes) visit(c.body.nodes); + if (Array.isArray(c.branches)) c.branches.forEach((b: any) => visit(b?.nodes ?? [])); + for (const k of ['try', 'catch']) if (c[k]?.nodes) visit(c[k].nodes); + }); + visit(flow.nodes); + return hit; +}; + +describe('#20590 — the inverse carries a credential back into a region', () => { + beforeEach(() => registerMetadataTypeRedactor('flow', regionFlowStandIn)); + afterEach(() => registerMetadataTypeRedactor('flow', flowStandInRedactor)); + + it('keeps both nested signing secrets on a round trip that also REORDERS the parallel branches', () => { + const stored = storedCalloutFlow(); + const served: any = redactMetadataItem('flow', stored); + expect(allStrings(served)).not.toContain(SIGNING); + + const fan = served.nodes[2]; + const incoming = { + ...served, + label: 'Edited', + nodes: [served.nodes[0], served.nodes[1], { ...fan, config: { ...fan.config, branches: [fan.config.branches[1], fan.config.branches[0]] } }], + }; + const out: any = carryForwardRedactedValues('flow', incoming, stored); + expect(calloutOf(out, 'per_row').config.signingSecret).toBe(SIGNING); + expect(calloutOf(out, 'push').config.signingSecret).toBe(SIGNING); + // The branch the credential came from, not the one now at its old index. + expect(out.nodes[2].config.branches[0].name).toBe('loud'); + expect(calloutOf(out, 'note').config).toEqual({}); + expect(out.label).toBe('Edited'); + // Copy-on-write: the caller's body is untouched. + expect(allStrings(incoming)).not.toContain(SIGNING); + }); + + it('carries nothing into a branch that cannot be told apart from another', () => { + const stored = storedCalloutFlow(); + const served: any = redactMetadataItem('flow', stored); + const fan = served.nodes[2]; + const twinBranches = { ...fan, config: { ...fan.config, branches: [fan.config.branches[1], structuredClone(fan.config.branches[1])] } }; + const out: any = carryForwardRedactedValues('flow', { ...served, nodes: [served.nodes[0], served.nodes[1], twinBranches] }, stored); + expect(out.nodes[2].config.branches.every((b: any) => b.nodes[0].config.signingSecret === undefined)).toBe(true); + expect(calloutOf(out, 'per_row').config.signingSecret).toBe(SIGNING); + }); + + it('the removal door: the empty string clears a signing secret, an absent key keeps it, a value replaces it', () => { + const stored = storedCalloutFlow(); + const served: any = redactMetadataItem('flow', stored); + + const cleared = structuredClone(served); + calloutOf(cleared, 'push').config.signingSecret = ''; + const clearedOut: any = carryForwardRedactedValues('flow', cleared, stored); + expect(calloutOf(clearedOut, 'push').config.signingSecret).toBe(''); + expect(calloutOf(clearedOut, 'per_row').config.signingSecret).toBe(SIGNING); + // …and the cleared form is served as written, so the next round trip keeps it cleared. + const reread: any = redactMetadataItem('flow', clearedOut); + expect(calloutOf(reread, 'push').config.signingSecret).toBe(''); + expect(calloutOf(carryForwardRedactedValues('flow', reread, clearedOut), 'push').config.signingSecret).toBe(''); + + const rotated = structuredClone(served); + calloutOf(rotated, 'push').config.signingSecret = ROTATED_SIGNING; + expect(calloutOf(carryForwardRedactedValues('flow', rotated, stored), 'push').config.signingSecret).toBe(ROTATED_SIGNING); + + // Absent is never "remove": the served form saved straight back keeps both. + const kept: any = carryForwardRedactedValues('flow', served, stored); + expect(calloutOf(kept, 'push').config.signingSecret).toBe(SIGNING); + expect(calloutOf(kept, 'per_row').config.signingSecret).toBe(SIGNING); + }); + + it('a callout moved out of its region keeps its secret; one whose kind changed does not carry it', () => { + const stored = storedCalloutFlow(); + const served: any = redactMetadataItem('flow', stored); + const perRow = served.nodes[1].config.body.nodes[0]; + // `push` becomes an assignment in place; `per_row` stays an http node. + const fan = served.nodes[2]; + const incoming = { + ...served, + nodes: [ + served.nodes[0], + { ...served.nodes[1], config: { ...served.nodes[1].config, body: { nodes: [perRow], edges: [] } } }, + { ...fan, config: { ...fan.config, branches: [fan.config.branches[0], { ...fan.config.branches[1], nodes: [{ ...fan.config.branches[1].nodes[0], type: 'assignment', config: {} }] }] } }, + ], + }; + const out: any = carryForwardRedactedValues('flow', incoming, stored); + expect(calloutOf(out, 'per_row').config.signingSecret).toBe(SIGNING); + expect(calloutOf(out, 'push').type).toBe('assignment'); + expect(calloutOf(out, 'push').config.signingSecret).toBeUndefined(); + expect(allStrings(redactMetadataItem('flow', out))).not.toContain(SIGNING); + }); + + it('the save door round trip: the stored row keeps both nested secrets, and no served read carries them', async () => { + const { engine, rows } = makeStubEngine(); + const where = { type: 'flow', name: 'callouts', organization_id: null, package_id: null, state: 'active' }; + const body = storedCalloutFlow(); + rows.set(keyOf(where), { id: 'r_callouts', ...where, metadata: JSON.stringify(body), checksum: hashSpec(body), version: 1 } as Row); + const protocol = new ObjectStackProtocolImplementation(engine); + + const served: any = (await protocol.getMetaItem({ type: 'flow', name: 'callouts' })).item; + expect(allStrings(served)).not.toContain(SIGNING); + const { _diagnostics: _d, ...editable } = served; + void _d; + await protocol.saveMetaItem({ type: 'flow', name: 'callouts', item: { ...editable, label: 'Edited' } }); + + const at = JSON.parse(Array.from(rows.values()).find((r) => r.name === 'callouts' && r.state === 'active')!.metadata); + expect(at.label).toBe('Edited'); + expect(calloutOf(at, 'per_row').config.signingSecret).toBe(SIGNING); + expect(calloutOf(at, 'push').config.signingSecret).toBe(SIGNING); + expect(allStrings(await protocol.getMetaItems({ type: 'flow' }))).not.toContain(SIGNING); + }); +}); diff --git a/packages/runtime/src/domains/meta-list-read-gate-parity.test.ts b/packages/runtime/src/domains/meta-list-read-gate-parity.test.ts index 97923bcbf44..01c0346d040 100644 --- a/packages/runtime/src/domains/meta-list-read-gate-parity.test.ts +++ b/packages/runtime/src/domains/meta-list-read-gate-parity.test.ts @@ -139,16 +139,21 @@ const CALLERS: Record<'holder' | 'non-holder' | 'anonymous', Caller> = { type CallerName = keyof typeof CALLERS; /** - * One protocol double, the same shape both transports read. `unknownTypes` - * makes `getMetaItems` throw for those types — the "protocol doesn't know this - * type" answer that sends the dispatcher on to its fallback stores — while it - * still answers every other type, the books the doc audience reads included. + * One protocol double, the same shape both transports read. `unansweredTypes` + * makes `getMetaItems` answer NO list for those types — the one protocol + * answer that sends the dispatcher on to its fallback stores — while it still + * answers every other type, the books the doc audience reads included. + * + * [#20590] Not a throw. A protocol throw is a fault and is answered as itself + * (`meta-list-protocol-fault.test.ts`); it used to be read as "the protocol + * does not know this type", which the one real protocol never signals that + * way — it answers such a type with an empty list. */ -function protocolDouble(unknownTypes: string[] = []) { +function protocolDouble(unansweredTypes: string[] = []) { return { getMetaTypes: vi.fn(async () => ({ types: Object.keys(STORE) })), getMetaItems: vi.fn(async ({ type }: any) => { - if (unknownTypes.includes(singular(type))) throw new Error(`unknown metadata type '${type}'`); + if (unansweredTypes.includes(singular(type))) return undefined; return clone(STORE[singular(type)] ?? []); }), }; diff --git a/packages/runtime/src/domains/meta.ts b/packages/runtime/src/domains/meta.ts index b4ddb362cc2..0857fe07d18 100644 --- a/packages/runtime/src/domains/meta.ts +++ b/packages/runtime/src/domains/meta.ts @@ -1874,8 +1874,20 @@ export async function handleMetadataRequest(deps: DomainHandlerDeps, path: strin const data = await protocol.getMetaItems({ type: typeOrName, packageId, organizationId, previewDrafts }); // Return any valid response from protocol (including empty items arrays) if (data && (data.items !== undefined || Array.isArray(data))) listed = data; - } catch { - // Protocol doesn't know this type, fall through + } catch (e: any) { + // [#20590] A throw here is a FAULT, answered as itself — never a + // cue to serve the metadata service's list below, which holds + // the stored bodies and applies no per-type read-path redaction + // (a flow's hook secret, a datasource's password). The protocol + // answers a type it holds nothing for with an empty list — it + // merges the metadata service's runtime-registered items (agents, + // tools) into its own answer — so it never signals "unknown + // type" by throwing. What it throws is a failed store read + // (503), a metadata app's marked refusal, a redactor failing + // closed, or a refused spelling (400). `RestServer`'s list route + // answers the same throw the same way ("prefer failing to + // falling back", AGENTS.md). + return { handled: true, response: deps.errorFromThrown(e, 500) }; } } // [ADR-0106 D5(2)] The dispatcher's list read is the same outlet as @@ -1883,6 +1895,8 @@ export async function handleMetadataRequest(deps: DomainHandlerDeps, path: strin if (listed !== undefined) return answerList(listed); // Try MetadataService directly for runtime-registered metadata (agents, tools, etc.) + // — reached only by a host whose protocol slot has no list verb, or + // whose protocol answered no list at all; never on a protocol fault. const metadataService = await deps.getService(_context, CoreServiceName.enum.metadata); if (metadataService && typeof (metadataService as any).list === 'function') { let items: any; diff --git a/packages/services/service-automation/src/flow-credential-positions.test.ts b/packages/services/service-automation/src/flow-credential-positions.test.ts index 64bc984e8d9..03733905e2f 100644 --- a/packages/services/service-automation/src/flow-credential-positions.test.ts +++ b/packages/services/service-automation/src/flow-credential-positions.test.ts @@ -29,7 +29,11 @@ import { } from '@objectstack/spec/automation'; import { AutomationEngine } from './engine.js'; import { installBuiltinNodes } from './builtin/index.js'; -import { redactFlowCredentials } from './flow-credential-projection.js'; +import { + FLOW_CREDENTIAL_CLEARED, + FLOW_NODE_CREDENTIAL_KEYS, + redactFlowCredentials, +} from './flow-credential-projection.js'; function silentLogger(): any { return { info() {}, warn() {}, error() {}, debug() {}, child() { return silentLogger(); } }; @@ -187,6 +191,12 @@ describe('#20590 — the enumeration: every declared credential position is with expect(positions.map(([t, k]) => `${t}.${k}`)).toContain('http.signingSecret'); }); + it('the projection covers exactly the declared positions — no fewer, and none that is no longer declared', () => { + const declared = positions.map(([t, k]) => `${t}.${k}`).sort(); + const covered = [...FLOW_NODE_CREDENTIAL_KEYS].flatMap(([t, keys]) => keys.map((k) => `${t}.${k}`)).sort(); + expect(covered).toEqual(declared); + }); + for (const [nodeType, configKey] of credentialPositions()) { for (const { where, flow } of placements(nodeType, configKey)) { it(`withholds ${nodeType}.config.${configKey} at ${where}`, () => { @@ -200,3 +210,74 @@ describe('#20590 — the enumeration: every declared credential position is with } } }); + +describe('#20590 — what the projection answers, position by position', () => { + it('names each withheld path, through every region kind, by the index in the body it was handed', () => { + const flow = { + name: 'nested', + label: 'Nested', + nodes: [ + { id: 'begin', type: 'start', label: 'Start', config: { secret: 'hook' } }, + { + id: 'each', type: 'loop', label: 'Each', + config: { + collection: '{rows}', + body: { nodes: [{ id: 'call', type: 'http', label: 'Call', config: { url: 'https://x', signingSecret: 'sign-1' } }], edges: [] }, + }, + }, + { + id: 'fan', type: 'parallel', label: 'Fan', + config: { + branches: [ + { name: 'a', nodes: [{ id: 'noop', type: 'assignment', label: 'Noop', config: {} }] }, + { name: 'b', nodes: [{ id: 'push', type: 'http', label: 'Push', config: { url: 'https://y', signingSecret: 'sign-2' } }] }, + ], + }, + }, + ], + edges: [], + }; + const { item, redactedKeys } = redactFlowCredentials(flow); + expect(redactedKeys).toEqual([ + 'nodes.0.config.secret', + 'nodes.1.config.body.nodes.0.config.signingSecret', + 'nodes.2.config.branches.1.nodes.0.config.signingSecret', + ]); + const text = JSON.stringify(item); + for (const s of ['hook', 'sign-1', 'sign-2']) expect(text).not.toContain(`"${s}"`); + // Everything that is not a credential survives, and untouched subtrees are shared. + const nodes = item.nodes as any[]; + expect(nodes[1].config.body.nodes[0].config).toEqual({ url: 'https://x' }); + expect(nodes[2].config.branches[0]).toBe((flow.nodes[2] as any).config.branches[0]); + }); + + it('serves the cleared form as written — the empty string holds no credential', () => { + const flow = { + name: 'cleared', + label: 'Cleared', + nodes: [ + { id: 'begin', type: 'start', label: 'Start', config: { secret: FLOW_CREDENTIAL_CLEARED } }, + { id: 'call', type: 'http', label: 'Call', config: { url: 'https://x', signingSecret: FLOW_CREDENTIAL_CLEARED } }, + ], + edges: [], + }; + const result = redactFlowCredentials(flow); + expect(result.redactedKeys).toEqual([]); + expect(result.item).toBe(flow); + // A blank that is not empty is still a value somebody typed: withheld. + const spaced = { ...flow, nodes: [flow.nodes[0], { ...flow.nodes[1], config: { url: 'https://x', signingSecret: ' ' } }] }; + expect(redactFlowCredentials(spaced).redactedKeys).toEqual(['nodes.1.config.signingSecret']); + }); + + it('withholds nothing from a key of the same name on a kind that does not hold that credential', () => { + const flow = { + name: 'lookalike', + label: 'Lookalike', + nodes: [{ id: 'n', type: 'assignment', label: 'N', config: { signingSecret: 'not-a-position', secret: 'nor-this' } }], + edges: [], + }; + const result = redactFlowCredentials(flow); + expect(result.redactedKeys).toEqual([]); + expect(result.item).toBe(flow); + }); +}); diff --git a/packages/services/service-automation/src/flow-credential-projection.ts b/packages/services/service-automation/src/flow-credential-projection.ts index 339c0e8b4da..b616a937d89 100644 --- a/packages/services/service-automation/src/flow-credential-projection.ts +++ b/packages/services/service-automation/src/flow-credential-projection.ts @@ -2,22 +2,44 @@ /** * # The flow credential projection — the ONE definition of what a served flow - * withholds (#20552) - * - * ADR-0041's `trigger-api` acceptance criteria give an inbound hook exactly one - * credential: "a per-flow secret; HMAC signature verification". That secret is - * authored as a literal on the flow's START node, `config.secret` — the object - * `AutomationEngine.deriveTriggerBinding` hands `trigger-api`'s - * `start()` as `binding.config`, and the key the engine's - * `validateApiTriggerSecret` refuses a flow without. Whoever holds it can sign - * posts to the hook, and the documented inbound pattern runs `runAs: 'system'`. - * - * Until this module every definition read served it verbatim: the automation - * domain's `GET /:name`, and on the metadata plane the item read, the list - * read, the layered read, the draft preview, the package export. A + * withholds (#20552, #20590) + * + * A flow definition holds two kinds of credential, each a literal in one node + * kind's `config`: + * + * - **the inbound hook's secret** — the start node's `config.secret`. ADR-0041's + * `trigger-api` acceptance criteria give an inbound hook exactly one + * credential: "a per-flow secret; HMAC signature verification". It is the + * object `AutomationEngine.deriveTriggerBinding` hands `trigger-api`'s + * `start()` as `binding.config`, and the key the engine's + * `validateApiTriggerSecret` refuses a flow without. Whoever holds it can + * sign posts to the hook, and the documented inbound pattern runs + * `runAs: 'system'`. + * - **an outbound callout's signing secret** — the `http` node's + * `config.signingSecret` (`HttpConfigSchema`). The durable arm hands it to + * the messaging outbox, which signs every delivery with it; whoever holds it + * can forge a delivery the receiving endpoint accepts as this platform's. + * + * Until this module every definition read served both verbatim: the + * automation domain's `GET /:name`, and on the metadata plane the item read, + * the list read, the layered read, the draft preview, the package export. A * credential is never readable back, so the projection below removes it from * what is SERVED — and nothing else. * + * {@link FLOW_NODE_CREDENTIAL_KEYS} is the table of those positions, by node + * kind. `flow-credential-positions.test.ts` holds it to the node config + * contracts the platform declares: it reads every declared key, and a + * credential-named one this table does not cover turns it red. + * + * ## At every depth a node can sit + * + * An ADR-0031 container (`loop`, `parallel`, `try_catch`) holds whole + * sub-graphs inside its own `config` — the slots `FLOW_REGION_SLOTS` declares + * — and an `http` callout inside a loop body is served with the flow like any + * top-level node. So the walk follows those slots, through every region, at + * every depth. A start node inside a region is not a valid flow, and its + * secret is withheld there too: under-redacting is the dangerous direction. + * * ## Where it applies, and where it deliberately does not * * It is registered as the `flow` metadata-type redactor (the @@ -29,7 +51,7 @@ * One helper, applied where each surface's definition leaves the process. * * It is NOT applied to anything the engine EXECUTES. The flow map the engine - * arms triggers from keeps the stored secret, and so does the in-process + * arms triggers from keeps the stored secrets, and so does the in-process * `getFlow` (the clone door copies a whole definition through it, ADR-0126 * §7.1): redaction is a serving act, and a raw-record consumer keeps reading * the stored body (`spec/kernel/metadata-type-redaction.ts`). That is also why @@ -39,36 +61,51 @@ * * ## Dropped, not masked * - * The key is REMOVED, the datasource redactor's posture: a mask would be a - * non-blank string, `validateApiTriggerSecret` would accept it, and any write - * path that missed the carry-forward would store the mask as the literal HMAC - * secret — silently. An absent key is what every registration door already - * refuses loudly, so a round trip that misses the carry-forward fails at the - * door instead of arming a hook nobody can sign for. + * A withheld key is REMOVED, the datasource redactor's posture: a mask would be + * a non-blank string, `validateApiTriggerSecret` would accept it, the outbox + * would sign with it, and any write path that missed the carry-forward would + * store the mask as the literal secret — silently. An absent start-node secret + * is what every registration door already refuses loudly, so a round trip that + * misses the carry-forward fails at the door instead of arming a hook nobody + * can sign for. * - * ## The write-path inverse + * ## The write-path inverse, and the one door that removes a credential * * `@objectstack/metadata-protocol`'s `carryForwardRedactedValues` is the one - * inverse, on both planes: a body that carries the projected form — no - * `secret` on the start node, exactly what was served — keeps the stored - * secret; an explicit value replaces it. The `redactedKeys` below address the - * node by its index in the stored body, and the inverse resolves that index to - * the node's `id`, so an edit that reorders `nodes` still carries the secret - * back onto the start node it came from. + * inverse, on both planes: a body that carries the projected form — the key + * absent, exactly what was served — keeps the stored value; an explicit value + * replaces it. The `redactedKeys` below address each node by its index in the + * stored body, and the inverse resolves that index to the node's `id` (a + * flow's node ids are one space across every region), so an edit that + * reorders `nodes`, or a region's `nodes`, or a `parallel` block's branches, + * still carries each value back onto the node it came from. And it re-runs + * this projection over what it carried, so a value never lands on a node whose + * kind no longer holds that credential (#20590 position 3). + * + * Absent therefore means "unchanged", never "removed" — so `signingSecret`, + * which is optional, needs a door of its own: the empty string + * ({@link FLOW_CREDENTIAL_CLEARED}). It is what the contract already accepts + * (`z.string().optional()`), it holds no secret, and the messaging outbox + * signs only with a non-empty one, so a callout whose secret is cleared is + * delivered unsigned. It is not withheld: the cleared form is served as written, so a + * reader can tell "cleared" from "withheld", and a round trip of it keeps it + * cleared. The start node's secret takes the same value and meets the engine's + * non-blank refusal, which is the answer an `api` flow with no secret is owed. * * ## Why registered by this plugin, not built into `@objectstack/spec` * * The registry's own note prefers a built-in for a type whose rows exist * without the owning plugin, because the credential stays live there. This - * one does not: the secret is a credential only to an armed hook, and a hook - * is armed only through this engine — `trigger-api` registers against the - * `automation` service and has no other source of flows. The knowledge of - * which key is the credential lives here too (`validateApiTriggerSecret`), and - * `packages/spec` declares no start-node config shape to derive it from, so a - * spec-side copy would be this package's business rule restated in the - * contract package (Prime Directive #2). + * one does not: the hook secret is a credential only to an armed hook, a hook + * is armed only through this engine, and an `http` node signs only when this + * engine runs it. The knowledge of which key is the credential lives here too + * (`validateApiTriggerSecret`, the `http` executor), and `packages/spec` + * declares no start-node config shape to derive it from, so a spec-side copy + * would be this package's business rule restated in the contract package + * (Prime Directive #2). */ +import { FLOW_REGION_SLOTS_BY_TYPE } from '@objectstack/spec/automation'; import { registerMetadataTypeRedactor } from '@objectstack/spec/kernel'; import type { MetadataRedactionResult, MetadataTypeRedactor } from '@objectstack/spec/kernel'; @@ -78,17 +115,99 @@ export const FLOW_METADATA_TYPE = 'flow'; /** The start-node `config` key that holds the inbound hook's HMAC secret (ADR-0041). */ export const FLOW_HOOK_SECRET_KEY = 'secret'; +/** The `http`-node `config` key that holds the outbound delivery's HMAC signing secret (`HttpConfigSchema`). */ +export const HTTP_SIGNING_SECRET_KEY = 'signingSecret'; + +/** + * Every credential position a flow node holds, by `node.type` → `config` keys. + * + * A `Map`, not an object literal: `node.type` is author-controlled and an open + * namespace (ADR-0018), and an object lookup would resolve `'constructor'` + * through `Object`'s prototype — the reason `FLOW_REGION_SLOTS_BY_TYPE` is one. + */ +export const FLOW_NODE_CREDENTIAL_KEYS: ReadonlyMap = new Map([ + ['start', [FLOW_HOOK_SECRET_KEY]], + ['http', [HTTP_SIGNING_SECRET_KEY]], +]); + +/** + * The explicit clearing value: a credential key set to it holds no credential, + * so it is served as written rather than withheld — the one unambiguous way to + * remove an optional credential across a round trip whose absent key means + * "unchanged". + */ +export const FLOW_CREDENTIAL_CLEARED = ''; + function isPlainRecord(value: unknown): value is Record { return !!value && typeof value === 'object' && !Array.isArray(value); } +/** Project a list of nodes; `undefined` when nothing in it was withheld. */ +function projectNodes(nodes: readonly unknown[], path: string, redactedKeys: string[]): unknown[] | undefined { + let out: unknown[] | undefined; + nodes.forEach((node, index) => { + const projected = projectNode(node, `${path}.${index}`, redactedKeys); + if (projected === node) return; + out ??= nodes.slice(); + out[index] = projected; + }); + return out; +} + +/** One region (`{ nodes, edges }`), projected; the input by reference when nothing was withheld. */ +function projectRegion(region: unknown, path: string, redactedKeys: string[]): unknown { + if (!isPlainRecord(region) || !Array.isArray(region.nodes)) return region; + const nodes = projectNodes(region.nodes, `${path}.nodes`, redactedKeys); + return nodes ? { ...region, nodes } : region; +} + +/** One node: its own credential keys, then every region its kind holds. */ +function projectNode(node: unknown, path: string, redactedKeys: string[]): unknown { + if (!isPlainRecord(node) || typeof node.type !== 'string') return node; + const config = node.config; + if (!isPlainRecord(config)) return node; + + let next: Record | undefined; + for (const key of FLOW_NODE_CREDENTIAL_KEYS.get(node.type) ?? []) { + if (!Object.prototype.hasOwnProperty.call(config, key)) continue; + if (config[key] === FLOW_CREDENTIAL_CLEARED) continue; + next ??= { ...config }; + delete next[key]; + redactedKeys.push(`${path}.config.${key}`); + } + + for (const slot of FLOW_REGION_SLOTS_BY_TYPE.get(node.type) ?? []) { + const value = config[slot.key]; + const slotPath = `${path}.config.${slot.key}`; + let projected: unknown = value; + if (slot.arity === 'one') { + projected = projectRegion(value, slotPath, redactedKeys); + } else if (Array.isArray(value)) { + let regions: unknown[] | undefined; + value.forEach((region, index) => { + const p = projectRegion(region, `${slotPath}.${index}`, redactedKeys); + if (p === region) return; + regions ??= value.slice(); + regions[index] = p; + }); + projected = regions ?? value; + } + if (projected === value) continue; + next ??= { ...config }; + next[slot.key] = projected; + } + + return next ? { ...node, config: next } : node; +} + /** - * Project the inbound-hook secret out of a flow definition. + * Project every credential in {@link FLOW_NODE_CREDENTIAL_KEYS} out of a flow + * definition, at every depth. * - * Every `start` node's `config.secret` is removed — not only the first start - * node's, and not only on a flow whose trigger resolves to `api`. The engine - * reads the first start node of an `api` flow, but a secret authored anywhere - * else is still a secret, and under-redacting is the dangerous direction. + * Every node of a listed kind is projected — not only the first start node, + * and not only on a flow whose trigger resolves to `api`. The engine reads the + * first start node of an `api` flow, but a secret authored anywhere else is + * still a secret, and under-redacting is the dangerous direction. * * Pure: the input is never mutated, and when there is nothing to withhold the * input is returned by reference with `redactedKeys: []`. @@ -98,19 +217,8 @@ export const redactFlowCredentials: MetadataTypeRedactor = (item): MetadataRedac if (!Array.isArray(nodes)) return { item, redactedKeys: [] }; const redactedKeys: string[] = []; - const projected = nodes.map((node, index) => { - if (!isPlainRecord(node) || node.type !== 'start') return node; - const config = node.config; - if (!isPlainRecord(config) || !Object.prototype.hasOwnProperty.call(config, FLOW_HOOK_SECRET_KEY)) { - return node; - } - const { [FLOW_HOOK_SECRET_KEY]: _withheld, ...rest } = config; - void _withheld; - redactedKeys.push(`nodes.${index}.config.${FLOW_HOOK_SECRET_KEY}`); - return { ...node, config: rest }; - }); - - if (redactedKeys.length === 0) return { item, redactedKeys: [] }; + const projected = projectNodes(nodes, 'nodes', redactedKeys); + if (!projected) return { item, redactedKeys: [] }; return { item: { ...item, nodes: projected }, redactedKeys: redactedKeys.sort() }; }; From fee1d6b9bd465208ab551ceeb68095e4210443d3 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 29 Sep 2026 07:46:05 +0000 Subject: [PATCH 3/7] chore(changeset): patch service-automation, metadata-protocol and runtime for the remaining flow credential positions Claude-Session: https://claude.ai/code/session_01XY5uCwTjZj7884yYtyur4H Co-authored-by: Claude --- .changeset/20590-flow-credential-positions.md | 31 +++++++++++++++++++ 1 file changed, 31 insertions(+) create mode 100644 .changeset/20590-flow-credential-positions.md diff --git a/.changeset/20590-flow-credential-positions.md b/.changeset/20590-flow-credential-positions.md new file mode 100644 index 00000000000..e0f5e16287e --- /dev/null +++ b/.changeset/20590-flow-credential-positions.md @@ -0,0 +1,31 @@ +--- +'@objectstack/service-automation': patch +'@objectstack/metadata-protocol': patch +'@objectstack/runtime': patch +--- + +fix(security): every credential a flow definition holds is withheld from what is served, at every depth, and an edit round trip keeps each one where it belongs (#20590) + +Clause-②: no + +**What is now withheld.** Beside an `api` flow's inbound-hook secret (the start node's +`config.secret`), every served flow definition now also withholds an `http` node's +outbound signing secret (`config.signingSecret`), and both are withheld wherever the +node sits: at the top level, or inside a `loop` body, a `parallel` branch, or a +`try_catch` region. The engine still executes the stored values. + +**Removing a signing secret.** A definition saved back without the key keeps the +stored secret, because an absent key is what every read serves. To remove it, save +the key as the empty string (`signingSecret: ''`): the durable callout is then +delivered unsigned, and the empty value is served as written, so the next round trip +keeps it cleared. + +**Changing a node's kind.** An edit that keeps a node's `id` and changes its kind no +longer carries that node's stored credential onto it. The credential belonged to the +old kind; a start node that needs a secret asks for one again at registration. + +**The `/meta` list read on a dispatcher host.** When the metadata protocol's list read +fails, the list answers that failure (`503 SERVICE_UNAVAILABLE` for a store outage, or +the protocol's own refusal) instead of serving the metadata service's stored list, +which applies no credential redaction. A host whose protocol has no list verb keeps +its metadata-service fallback. From 0661a9a04bc8177f5f44d98e1c8f82cf61607641 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 29 Sep 2026 08:45:34 +0000 Subject: [PATCH 4/7] test(metadata-protocol): pins for a flow node moved across regions, red on fee1d6b9 where the moved node's credential is dropped at save (a) an http node moved out of a loop body to the top level, (b) one moved from the top level into a parallel branch, and (a) again through the save door followed by the served reads: each loses the stored signing secret today. (c) a node moved into a region and changed in kind, and (d) an id duplicated across two regions, are the guards that must keep holding. Retitles the old region test to what it asserts: the node stays in place. Claude-Session: https://claude.ai/code/session_01XY5uCwTjZj7884yYtyur4H Co-authored-by: Claude --- .../src/protocol.metadata-redaction.test.ts | 131 +++++++++++++++++- 1 file changed, 130 insertions(+), 1 deletion(-) diff --git a/packages/metadata-protocol/src/protocol.metadata-redaction.test.ts b/packages/metadata-protocol/src/protocol.metadata-redaction.test.ts index 2f9311f89e6..e9d97e99da7 100644 --- a/packages/metadata-protocol/src/protocol.metadata-redaction.test.ts +++ b/packages/metadata-protocol/src/protocol.metadata-redaction.test.ts @@ -934,7 +934,7 @@ describe('#20590 — the inverse carries a credential back into a region', () => expect(calloutOf(kept, 'per_row').config.signingSecret).toBe(SIGNING); }); - it('a callout moved out of its region keeps its secret; one whose kind changed does not carry it', () => { + it('a callout left in place inside a rebuilt region keeps its secret; one whose kind changed in place does not carry it', () => { const stored = storedCalloutFlow(); const served: any = redactMetadataItem('flow', stored); const perRow = served.nodes[1].config.body.nodes[0]; @@ -975,3 +975,132 @@ describe('#20590 — the inverse carries a credential back into a region', () => expect(allStrings(await protocol.getMetaItems({ type: 'flow' }))).not.toContain(SIGNING); }); }); + +// --------------------------------------------------------------------------- +// #20590 patch round 1 — a node MOVED across regions keeps its credential +// --------------------------------------------------------------------------- +// +// The stored path names the credential's container through the regions the +// node sat in at rest. An author who moves that node — out of a loop body, into +// a parallel branch — keeping its id, its kind and the withheld form, sends a +// body in which that path no longer resolves. Skipping the graft there dropped +// the stored secret at save, silently, and the next durable callout went out +// unsigned. The node is found by its id across the whole incoming body instead +// (exactly one match), and the position check still decides whether it lands. + +/** {@link storedCalloutFlow} plus a top-level callout, so a node can move INTO a region too. */ +function storedMovesFlow() { + const flow: any = storedCalloutFlow(); + flow.name = 'moves'; + flow.nodes.push({ id: 'call', type: 'http', label: 'Call', config: { url: 'https://c', durable: true, signingSecret: SIGNING } }); + return flow; +} + +const NOOP = (id: string) => ({ id, type: 'assignment', label: id, config: {} }); + +/** The served body with `per_row` moved from the loop body to the top level (the body keeps a stand-in node). */ +function movedOutOfLoop(served: any) { + const [begin, each, fan, call] = served.nodes; + const perRow = each.config.body.nodes[0]; + return { + ...served, + nodes: [begin, { ...each, config: { ...each.config, body: { nodes: [NOOP('body_noop')], edges: [] } } }, fan, call, perRow], + }; +} + +describe('#20590 round 1 — a node moved across regions keeps its credential', () => { + beforeEach(() => registerMetadataTypeRedactor('flow', regionFlowStandIn)); + afterEach(() => registerMetadataTypeRedactor('flow', flowStandInRedactor)); + + it('(a) an http node moved out of a loop body to the top level: the secret is kept at rest and not served', () => { + const stored = storedMovesFlow(); + const served: any = redactMetadataItem('flow', stored); + const incoming = movedOutOfLoop(served); + const out: any = carryForwardRedactedValues('flow', incoming, stored); + expect(out.nodes[4].id).toBe('per_row'); + expect(out.nodes[4].config.signingSecret).toBe(SIGNING); + expect(calloutOf(out, 'push').config.signingSecret).toBe(SIGNING); + expect(calloutOf(out, 'call').config.signingSecret).toBe(SIGNING); + expect(allStrings(redactMetadataItem('flow', out))).not.toContain(SIGNING); + expect(allStrings(incoming)).not.toContain(SIGNING); + }); + + it('(b) an http node moved from the top level into a parallel branch: the secret is kept at rest and not served', () => { + const stored = storedMovesFlow(); + const served: any = redactMetadataItem('flow', stored); + const [begin, each, fan, call] = served.nodes; + const quiet = fan.config.branches[0]; + const incoming = { + ...served, + nodes: [begin, each, { ...fan, config: { ...fan.config, branches: [{ ...quiet, nodes: [...quiet.nodes, call] }, fan.config.branches[1]] } }], + }; + const out: any = carryForwardRedactedValues('flow', incoming, stored); + expect(out.nodes[2].config.branches[0].nodes[1].id).toBe('call'); + expect(out.nodes[2].config.branches[0].nodes[1].config.signingSecret).toBe(SIGNING); + expect(calloutOf(out, 'per_row').config.signingSecret).toBe(SIGNING); + expect(allStrings(redactMetadataItem('flow', out))).not.toContain(SIGNING); + }); + + it('(c) a node moved into a region AND changed in kind: the value is dropped — the position check still wins', () => { + const stored = storedMovesFlow(); + const served: any = redactMetadataItem('flow', stored); + const [begin, each, fan, call] = served.nodes; + const incoming = { + ...served, + nodes: [ + begin, + { ...each, config: { ...each.config, body: { nodes: [...each.config.body.nodes, { ...call, type: 'assignment', config: {} }], edges: [] } } }, + fan, + ], + }; + const out: any = carryForwardRedactedValues('flow', incoming, stored); + expect(calloutOf(out, 'call').type).toBe('assignment'); + expect(calloutOf(out, 'call').config.signingSecret).toBeUndefined(); + expect(calloutOf(out, 'per_row').config.signingSecret).toBe(SIGNING); + expect(allStrings(redactMetadataItem('flow', out))).not.toContain(SIGNING); + }); + + it('(d) an id duplicated across two regions: nothing is grafted onto either copy', () => { + const stored = storedMovesFlow(); + const served: any = redactMetadataItem('flow', stored); + const [begin, each, fan, call] = served.nodes; + const perRow = each.config.body.nodes[0]; + const [quiet, loud] = fan.config.branches; + const incoming = { + ...served, + nodes: [ + begin, + { ...each, config: { ...each.config, body: { nodes: [NOOP('body_noop')], edges: [] } } }, + { ...fan, config: { ...fan.config, branches: [{ ...quiet, nodes: [...quiet.nodes, perRow] }, { ...loud, nodes: [...loud.nodes, structuredClone(perRow)] }] } }, + call, + ], + }; + const out: any = carryForwardRedactedValues('flow', incoming, stored); + const copies = out.nodes[2].config.branches.flatMap((b: any) => b.nodes.filter((n: any) => n.id === 'per_row')); + expect(copies).toHaveLength(2); + expect(copies.every((n: any) => n.config.signingSecret === undefined)).toBe(true); + // The unambiguous ones are unaffected. + expect(calloutOf(out, 'call').config.signingSecret).toBe(SIGNING); + }); + + it('(a) through the save door: the row at rest keeps the moved node\'s secret, and no served read carries it', async () => { + const { engine, rows } = makeStubEngine(); + const where = { type: 'flow', name: 'moves', organization_id: null, package_id: null, state: 'active' }; + const body = storedMovesFlow(); + rows.set(keyOf(where), { id: 'r_moves', ...where, metadata: JSON.stringify(body), checksum: hashSpec(body), version: 1 } as Row); + const protocol = new ObjectStackProtocolImplementation(engine); + + const served: any = (await protocol.getMetaItem({ type: 'flow', name: 'moves' })).item; + expect(allStrings(served)).not.toContain(SIGNING); + const { _diagnostics: _d, ...editable } = served; + void _d; + await protocol.saveMetaItem({ type: 'flow', name: 'moves', item: movedOutOfLoop(editable) }); + + const at = JSON.parse(Array.from(rows.values()).find((r) => r.name === 'moves' && r.state === 'active')!.metadata); + expect(at.nodes.map((n: any) => n.id)).toEqual(['begin', 'each', 'fan', 'call', 'per_row']); + expect(at.nodes[4].config.signingSecret).toBe(SIGNING); + expect(calloutOf(at, 'body_noop').config).toEqual({}); + expect(allStrings(await protocol.getMetaItem({ type: 'flow', name: 'moves' }))).not.toContain(SIGNING); + expect(allStrings(await protocol.getMetaItems({ type: 'flow' }))).not.toContain(SIGNING); + }); +}); From 5116194e04c5029362e428bb86f0b9ea6a54a722 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 29 Sep 2026 09:59:06 +0000 Subject: [PATCH 5/7] fix(metadata-protocol): a flow node moved across regions keeps its credential on a round trip When the stored path to a redacted key no longer resolves in the incoming body, because the element that owns it moved (out of a loop body, into a parallel branch) keeping its id, the carry-forward now finds that element by its id across the whole incoming body, using it only when exactly one object carries that id. The existing position check still decides whether the value lands, so a node moved and changed in kind still does not carry it. Two stored paths landing on one incoming position carry neither. A path with no identified element (every datasource path) is unaffected. Claude-Session: https://claude.ai/code/session_01XY5uCwTjZj7884yYtyur4H Co-authored-by: Claude --- .changeset/20590-flow-credential-positions.md | 4 + .../src/metadata-redaction.ts | 144 +++++++++++++----- 2 files changed, 109 insertions(+), 39 deletions(-) diff --git a/.changeset/20590-flow-credential-positions.md b/.changeset/20590-flow-credential-positions.md index e0f5e16287e..520686591e1 100644 --- a/.changeset/20590-flow-credential-positions.md +++ b/.changeset/20590-flow-credential-positions.md @@ -24,6 +24,10 @@ keeps it cleared. longer carries that node's stored credential onto it. The credential belonged to the old kind; a start node that needs a secret asks for one again at registration. +**Moving a node.** A node moved into or out of a `loop` body, a `parallel` branch or a +`try_catch` region keeps its stored credential across the round trip, as long as its +`id` and kind are unchanged and no other node in the definition carries the same `id`. + **The `/meta` list read on a dispatcher host.** When the metadata protocol's list read fails, the list answers that failure (`503 SERVICE_UNAVAILABLE` for a store outage, or the protocol's own refusal) instead of serving the metadata service's stored list, diff --git a/packages/metadata-protocol/src/metadata-redaction.ts b/packages/metadata-protocol/src/metadata-redaction.ts index 08c3ac98fca..784c18b56ee 100644 --- a/packages/metadata-protocol/src/metadata-redaction.ts +++ b/packages/metadata-protocol/src/metadata-redaction.ts @@ -263,7 +263,8 @@ function stepInto(node: unknown, hop: PathHop): { segment: string; next: unknown } /** - * Walk to the plain object that OWNS the redacted key, hop by hop. + * Walk `hops` in `root`: the plain object that OWNS the redacted key, and the + * concrete segments (a key, or an array index in THIS body) that reach it. * * `undefined` when any hop along the way is absent, is the wrong kind of * container, or (for an array hop) is answered by no single element — which @@ -273,53 +274,101 @@ function stepInto(node: unknown, hop: PathHop): { segment: string; next: unknown * grafting `config.password` back onto it would MINT a config that holds * nothing but a credential. */ -function containerAt(root: unknown, hops: readonly PathHop[]): Record | undefined { +function locate(root: unknown, hops: readonly PathHop[]): { at: string[]; container: Record } | undefined { + const at: string[] = []; let node: unknown = root; for (const hop of hops) { const step = stepInto(node, hop); if (!step) return undefined; + at.push(step.segment); node = step.next; } - return isPlainRecord(node) ? node : undefined; + return isPlainRecord(node) ? { at, container: node } : undefined; +} + +/** {@link locate}, container only. */ +function containerAt(root: unknown, hops: readonly PathHop[]): Record | undefined { + return locate(root, hops)?.container; +} + +/** The value `at` names in `root`; every segment resolves (it came from a walk of `root`). */ +function valueAt(root: unknown, at: readonly string[]): unknown { + let node: unknown = root; + for (const segment of at) node = Array.isArray(node) ? node[Number(segment)] : (node as Record)[segment]; + return node; } /** - * The dotted, index-addressed path `hops` + `key` names in `root` — the - * spelling a redactor's `redactedKeys` uses for that same body. Called only - * where {@link containerAt} found the container, so every hop resolves. + * [#20590 round 1] Where the redacted key's container sits in `root` when the + * STORED path to it no longer resolves there — because the element that owns + * it MOVED, keeping its identity: a flow node taken out of a `loop` body, or + * put into a `parallel` branch, keeps its `id`, its kind and the withheld form, + * and only the regions around it change. Skipping the graft there would drop + * the stored credential at save, silently. + * + * The owner is the path's deepest identified element (its last `elementId` + * hop). It is looked for by that `id` across the WHOLE body, and used only + * when exactly ONE plain object anywhere in it carries that `id` — none, and + * the author removed it; two or more, and they cannot be told apart, so + * nothing is chosen. Whole-body uniqueness is what keeps this safe for a type + * whose ids are not one space the way a flow's node ids are. From the owner, + * the rest of the path is walked as usual (the key hops down to the container, + * so an owner whose `config` the author removed still grafts nothing). + * + * A path with no identified element — every datasource path, whose redactor + * never crosses an array — has no owner to find, and is unaffected. */ -function pathAt(root: unknown, hops: readonly PathHop[], key: string): string { - const segments: string[] = []; - let node: unknown = root; - for (const hop of hops) { - const step = stepInto(node, hop) as { segment: string; next: unknown }; - segments.push(step.segment); - node = step.next; +function relocateById(root: unknown, hops: readonly PathHop[]): { at: string[]; container: Record } | undefined { + let owner = -1; + for (let i = hops.length - 1; i >= 0; i -= 1) { + if ('elementId' in (hops[i] as PathHop)) { + owner = i; + break; + } } - segments.push(key); - return segments.join('.'); + if (owner < 0) return undefined; + const id = (hops[owner] as { elementId: string }).elementId; + + const matches: string[][] = []; + const visit = (node: unknown, at: string[]): void => { + if (Array.isArray(node)) { + node.forEach((value, index) => visit(value, [...at, String(index)])); + return; + } + if (!isPlainRecord(node)) return; + if (node.id === id) matches.push(at); + for (const [key, value] of Object.entries(node)) { + if (value && typeof value === 'object') visit(value, [...at, key]); + } + }; + visit(root, []); + if (matches.length !== 1) return undefined; + + const ownerAt = matches[0] as string[]; + const rest = locate(valueAt(root, ownerAt), hops.slice(owner + 1)); + return rest ? { at: [...ownerAt, ...rest.at], container: rest.container } : undefined; } /** - * Copy-on-write set of `value` under `key` at the end of `hops`, returning a - * new root and copying only the containers along the path — an array hop - * copies the array and replaces the one element it names. + * Copy-on-write set of `value` under `key` in the container `at` names, + * returning a new root and copying only the containers along the way — an + * array segment copies the array and replaces the one element it names. * - * Called only after {@link containerAt} found the container in `root`, so - * every hop resolves. 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. + * `at` came from a walk of `root`, so every segment resolves. 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: unknown, hops: readonly PathHop[], key: string, value: unknown): unknown { - if (hops.length === 0) return { ...(root as Record), [key]: value }; - const [hop, ...rest] = hops as [PathHop, ...PathHop[]]; - const step = stepInto(root, hop) as { segment: string; next: unknown }; - if ('key' in hop) { - return { ...(root as Record), [hop.key]: withValueAt(step.next, rest, key, value) }; +function withValueAt(root: unknown, at: readonly string[], key: string, value: unknown): unknown { + if (at.length === 0) return { ...(root as Record), [key]: value }; + const [head, ...rest] = at as [string, ...string[]]; + if (Array.isArray(root)) { + const next = root.slice(); + next[Number(head)] = withValueAt(root[Number(head)], rest, key, value); + return next; } - const next = (root as unknown[]).slice(); - next[Number(step.segment)] = withValueAt(step.next, rest, key, value); - return next; + const record = root as Record; + return { ...record, [head]: withValueAt(record[head], rest, key, value) }; } /** Structural equality for the values a redactor hides (scalars in practice; general by construction). */ @@ -376,6 +425,14 @@ function sameValue(a: unknown, b: unknown): boolean { * credential: it is gone, and a kind that needs one asks for it again (an * `api` flow's start node without a secret is refused at registration). * + * A node MOVED across regions — out of a loop body, into a parallel branch — + * keeps its identity while the stored path around it stops resolving. Its + * container is then found by the owning element's `id` across the whole + * incoming body, exactly one match or none ({@link relocateById}), and the + * same position check decides whether the value lands: a moved node whose + * kind still holds the credential keeps it; one moved AND changed in kind + * does not. + * * ⚠️ 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. @@ -405,7 +462,7 @@ export function carryForwardRedactedValues(type: string, incoming: T, stored: const served = redactor(stored as Record); if (served.redactedKeys.length === 0) return incoming; - const grafts: Array<{ hops: PathHop[]; key: string; value: unknown }> = []; + const grafts: Array<{ at: string[]; key: string; value: unknown }> = []; for (const path of served.redactedKeys) { // Dotted, item-relative — the registry's documented contract for // `redactedKeys` (`config.password`; an array hop is an index into the @@ -420,26 +477,35 @@ export function carryForwardRedactedValues(type: string, incoming: T, stored: const storedValue = storedParent?.[key]; if (storedValue === undefined) continue; - const incomingParent = containerAt(incoming, hops); - if (!incomingParent) continue; + // Where the container sits in the incoming body: along the stored + // path, or — the node that owns it having MOVED — at the one element + // anywhere in the body that carries the owner's id (#20590 round 1). + const target = locate(incoming, hops) ?? relocateById(incoming, hops); + if (!target) continue; const servedParent = containerAt(served.item, hops); - if (!sameValue(incomingParent[key], servedParent?.[key])) continue; + if (!sameValue(target.container[key], servedParent?.[key])) continue; - grafts.push({ hops, key, value: storedValue }); + grafts.push({ at: target.at, key, value: storedValue }); } + // Two stored paths landing on ONE incoming position cannot be told apart; + // neither is carried rather than one silently overwriting the other. + const landing = (graft: { at: readonly string[]; key: string }) => [...graft.at, graft.key].join('.'); + const counts = new Map(); + for (const graft of grafts) counts.set(landing(graft), (counts.get(landing(graft)) ?? 0) + 1); + // [#20590] Keep only what the read would withhold where it now lands. A // graft sets a leaf and moves no container, so each pass re-grafts the // survivors onto the untouched incoming body; the set only shrinks, so // this settles in at most one pass per graft. - let kept = grafts; + let kept = grafts.filter((graft) => counts.get(landing(graft)) === 1); for (;;) { let out: unknown = incoming; - for (const graft of kept) out = withValueAt(out, graft.hops, graft.key, graft.value); + for (const graft of kept) out = withValueAt(out, graft.at, graft.key, graft.value); if (kept.length === 0) return out as T; const withheld = new Set(redactor(out as Record).redactedKeys); - const next = kept.filter((graft) => withheld.has(pathAt(out, graft.hops, graft.key))); + const next = kept.filter((graft) => withheld.has(landing(graft))); if (next.length === kept.length) return out as T; kept = next; } From 2b7e04d3dfeae657878f0be1c618ca3af9244b6c Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 29 Sep 2026 10:44:00 +0000 Subject: [PATCH 6/7] test(metadata-protocol): pins for a moved flow node whose id an edge shares, red on 5116194e where the edge blocks the relocation (e) a moved node plus an edge sharing its id, as a pure inverse and through the save door (a schema-valid flow): the moved node's signing secret is dropped today, because the by-id lookup counts the edge as a second match. (f) two nodes sharing the moved node's id in different regions is the guard that must keep grafting nothing. Claude-Session: https://claude.ai/code/session_01XY5uCwTjZj7884yYtyur4H Co-authored-by: Claude --- .../src/protocol.metadata-redaction.test.ts | 71 +++++++++++++++++++ 1 file changed, 71 insertions(+) diff --git a/packages/metadata-protocol/src/protocol.metadata-redaction.test.ts b/packages/metadata-protocol/src/protocol.metadata-redaction.test.ts index e9d97e99da7..8a40f599541 100644 --- a/packages/metadata-protocol/src/protocol.metadata-redaction.test.ts +++ b/packages/metadata-protocol/src/protocol.metadata-redaction.test.ts @@ -1104,3 +1104,74 @@ describe('#20590 round 1 — a node moved across regions keeps its credential', expect(allStrings(await protocol.getMetaItems({ type: 'flow' }))).not.toContain(SIGNING); }); }); + +// --------------------------------------------------------------------------- +// #20590 patch round 2 — only a NODE can stand where the moved node stood +// --------------------------------------------------------------------------- +// +// The by-id lookup must count the elements that stand where the owner stood — +// the elements of an array under the same key as the owner's container — and +// nothing else. A flow keeps node ids and edge ids in separate spaces, so an +// edge may share a moved node's id and still be a valid flow; counted as a +// second match, it blocked the relocation and dropped the credential at save. + +describe('#20590 round 2 — the relocation counts nodes, not every object carrying the id', () => { + beforeEach(() => registerMetadataTypeRedactor('flow', regionFlowStandIn)); + afterEach(() => registerMetadataTypeRedactor('flow', flowStandInRedactor)); + + /** {@link movedOutOfLoop} plus a top-level EDGE whose id is the moved node's. */ + function movedWithEdgeTwin(served: any) { + const moved: any = movedOutOfLoop(served); + return { ...moved, edges: [...moved.edges, { id: 'per_row', source: 'begin', target: 'per_row' }] }; + } + + it('(e) a moved node plus an edge sharing its id: the credential is kept', () => { + const stored = storedMovesFlow(); + const incoming = movedWithEdgeTwin(redactMetadataItem('flow', stored)); + const out: any = carryForwardRedactedValues('flow', incoming, stored); + expect(out.edges.map((e: any) => e.id)).toEqual(['per_row']); + expect(out.nodes[4].id).toBe('per_row'); + expect(out.nodes[4].config.signingSecret).toBe(SIGNING); + expect(allStrings(redactMetadataItem('flow', out))).not.toContain(SIGNING); + }); + + it('(e) through the save door: the edge twin is a valid flow, and the row at rest keeps the moved node\'s secret', async () => { + const { engine, rows } = makeStubEngine(); + const where = { type: 'flow', name: 'moves', organization_id: null, package_id: null, state: 'active' }; + const body = storedMovesFlow(); + rows.set(keyOf(where), { id: 'r_moves', ...where, metadata: JSON.stringify(body), checksum: hashSpec(body), version: 1 } as Row); + const protocol = new ObjectStackProtocolImplementation(engine); + + const served: any = (await protocol.getMetaItem({ type: 'flow', name: 'moves' })).item; + const { _diagnostics: _d, ...editable } = served; + void _d; + await protocol.saveMetaItem({ type: 'flow', name: 'moves', item: movedWithEdgeTwin(editable) }); + + const at = JSON.parse(Array.from(rows.values()).find((r) => r.name === 'moves' && r.state === 'active')!.metadata); + expect(at.edges.map((e: any) => e.id)).toEqual(['per_row']); + expect(at.nodes[4].config.signingSecret).toBe(SIGNING); + expect(allStrings(await protocol.getMetaItem({ type: 'flow', name: 'moves' }))).not.toContain(SIGNING); + }); + + it('(f) two NODES sharing the moved node\'s id in different regions: nothing is grafted', () => { + const stored = storedMovesFlow(); + const served: any = redactMetadataItem('flow', stored); + const [begin, each, fan, call] = served.nodes; + const [quiet, loud] = fan.config.branches; + const incoming = { + ...served, + nodes: [ + begin, + { ...each, config: { ...each.config, body: { nodes: [...each.config.body.nodes, call], edges: [] } } }, + { ...fan, config: { ...fan.config, branches: [{ ...quiet, nodes: [...quiet.nodes, { ...NOOP('call') }] }, loud] } }, + ], + }; + const out: any = carryForwardRedactedValues('flow', incoming, stored); + const moved = out.nodes[1].config.body.nodes.find((n: any) => n.id === 'call'); + expect(moved.type).toBe('http'); + expect(moved.config.signingSecret).toBeUndefined(); + expect(out.nodes[2].config.branches[0].nodes.find((n: any) => n.id === 'call').config).toEqual({}); + // The unambiguous ones are unaffected. + expect(calloutOf(out, 'per_row').config.signingSecret).toBe(SIGNING); + }); +}); From 60a5f765af845f23a85d01f73669e0a78a488d6f Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 29 Sep 2026 10:44:49 +0000 Subject: [PATCH 7/7] fix(metadata-protocol): relocate a moved flow node by the elements that stand where it stood, not every object carrying its id The by-id lookup counted every plain object in the body whose id equals the moved node's, edges and nested config values included. A flow keeps node and edge ids in separate spaces, so an edge sharing a moved node's id is valid, counted as a second match, blocked the relocation and dropped the stored credential at save. The match set is now the elements of arrays held under the same key as the owner's array in the stored path (read from the stored hops, not named), and must still be unique across the body. The changeset's "Moving a node" sentence now states that condition. Claude-Session: https://claude.ai/code/session_01XY5uCwTjZj7884yYtyur4H Co-authored-by: Claude --- .changeset/20590-flow-credential-positions.md | 3 +- .../src/metadata-redaction.ts | 32 +++++++++++++++---- 2 files changed, 27 insertions(+), 8 deletions(-) diff --git a/.changeset/20590-flow-credential-positions.md b/.changeset/20590-flow-credential-positions.md index 520686591e1..53076603308 100644 --- a/.changeset/20590-flow-credential-positions.md +++ b/.changeset/20590-flow-credential-positions.md @@ -26,7 +26,8 @@ old kind; a start node that needs a secret asks for one again at registration. **Moving a node.** A node moved into or out of a `loop` body, a `parallel` branch or a `try_catch` region keeps its stored credential across the round trip, as long as its -`id` and kind are unchanged and no other node in the definition carries the same `id`. +`id` and kind are unchanged and it is the only node, at the top level or in any region, +that carries that `id`. An edge or a config value with the same `id` does not count. **The `/meta` list read on a dispatcher host.** When the metadata protocol's list read fails, the list answers that failure (`503 SERVICE_UNAVAILABLE` for a store outage, or diff --git a/packages/metadata-protocol/src/metadata-redaction.ts b/packages/metadata-protocol/src/metadata-redaction.ts index 784c18b56ee..b5269351a3d 100644 --- a/packages/metadata-protocol/src/metadata-redaction.ts +++ b/packages/metadata-protocol/src/metadata-redaction.ts @@ -307,14 +307,23 @@ function valueAt(root: unknown, at: readonly string[]): unknown { * the stored credential at save, silently. * * The owner is the path's deepest identified element (its last `elementId` - * hop). It is looked for by that `id` across the WHOLE body, and used only - * when exactly ONE plain object anywhere in it carries that `id` — none, and - * the author removed it; two or more, and they cannot be told apart, so - * nothing is chosen. Whole-body uniqueness is what keeps this safe for a type + * hop). It is looked for by that `id` across the WHOLE body — but only among + * the elements that stand where the owner stood: the elements of an array + * held under the SAME key as the owner's own array in the stored path (for a + * flow node that key is `nodes`, so a node at the top level or in any region + * counts, and an edge or a config value that happens to carry the same `id` + * does not — a flow keeps node ids and edge ids in separate spaces, #20590 + * round 2). The key is read from the stored hops, never named here. It is used + * only when exactly ONE such element carries that `id` — none, and the author + * removed it; two or more, and they cannot be told apart, so nothing is + * chosen. Uniqueness across the whole body is what keeps this safe for a type * whose ids are not one space the way a flow's node ids are. From the owner, * the rest of the path is walked as usual (the key hops down to the container, * so an owner whose `config` the author removed still grafts nothing). * + * An owner whose array is not held under a key — an array directly inside + * another array — has no key to scope by, and is not relocated. + * * A path with no identified element — every datasource path, whose redactor * never crosses an array — has no owner to find, and is unaffected. */ @@ -326,8 +335,12 @@ function relocateById(root: unknown, hops: readonly PathHop[]): { at: string[]; break; } } - if (owner < 0) return undefined; + if (owner < 1) return undefined; const id = (hops[owner] as { elementId: string }).elementId; + // The key the owner's array is held under in the stored path. + const parent = hops[owner - 1] as PathHop; + if (!('key' in parent)) return undefined; + const arrayKey = parent.key; const matches: string[][] = []; const visit = (node: unknown, at: string[]): void => { @@ -336,9 +349,14 @@ function relocateById(root: unknown, hops: readonly PathHop[]): { at: string[]; return; } if (!isPlainRecord(node)) return; - if (node.id === id) matches.push(at); for (const [key, value] of Object.entries(node)) { - if (value && typeof value === 'object') visit(value, [...at, key]); + if (!value || typeof value !== 'object') continue; + if (key === arrayKey && Array.isArray(value)) { + value.forEach((element, index) => { + if (isPlainRecord(element) && element.id === id) matches.push([...at, key, String(index)]); + }); + } + visit(value, [...at, key]); } }; visit(root, []);