From 837710e25b665fee1fb1f32809db52b210d5b59b Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 8 Sep 2026 13:24:51 +0000 Subject: [PATCH] fix(lint): the flow-template rules read an `http` node's request payload MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `flow-double-brace-interpolation` and `flow-bare-dollar-reference` scan a node's config recursively, over a region-STRIPPED view so a container is not also credited with its descendants' findings. That view was built from the FLAT UNION of every config key that holds a region on ANY node type (`body`, `try`, `catch`, `branches`) instead of the slots the node in hand owns, because this call site took `stripRegions`' default argument. `body` is `loop`'s region slot AND the canonical request-payload key on an `http` node, so `config.body` was deleted from every node's view before the scan read it. That is the one key where an uninterpolated token has an outbound consequence: `http-nodes.ts` interpolates the raw config wholesale, so a double-brace `{{ }}` or a bare `$ref.field` written in a payload ships to the endpoint as literal text — the exact failure both rules exist to catch, in the exact place they could not see. The remedy was already written in `stripRegions`' own docblock ("Pass the OWNING node's slots, not the flat union") and the sibling call site in `flow-walk.ts` already followed it. This one now does: `stripRegions(node.config, ownRegionKeys(node.type))`. `regionKeys` also becomes REQUIRED, and the flat-union default is deleted. The union survived as a default only to bound the earlier change that introduced the per-type argument, and this defect is what that cost: the shorter call compiled and quietly asked a different question than its caller meant. With no default, a caller that has not decided which set it means fails to compile. `flow-walk.test.ts` pins that with a `@ts-expect-error` — evaluated, since `tsconfig.test.json` compiles the test layer. Both directions are pinned. Measured on the parent commit, every new case returned zero findings for its rule: a `{{ }}` and a bare `$ref.field` in an `http` payload, at top level and nested in a `try_catch` region, for both rule ids. The over-correction direction — a repair that stripped nothing — is pinned too: a payload token inside a `loop` body is still reported exactly ONCE, against the node carrying it and not also against the container. Blast radius measured across `examples/app-showcase`, `app-crm` and `app-todo`: 34 flows, `http` payloads inside a `parallel` branch and a `try_catch` try among them, zero findings before and zero after — those payloads use correct single-brace tokens. A positive control, one `{{ }}` injected into a real showcase `http` payload, reads 0 before and 1 after. Fixes #16405 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_012GKcPZbMoGq7WPzKLfRBTU --- ...-flow-template-rules-reach-http-payload.md | 15 ++ packages/lint/src/flow-walk.test.ts | 33 +++- packages/lint/src/flow-walk.ts | 21 +- packages/lint/src/lint-flow-patterns.test.ts | 183 +++++++++++++++++- packages/lint/src/lint-flow-patterns.ts | 29 ++- 5 files changed, 259 insertions(+), 22 deletions(-) create mode 100644 .changeset/lint-flow-template-rules-reach-http-payload.md diff --git a/.changeset/lint-flow-template-rules-reach-http-payload.md b/.changeset/lint-flow-template-rules-reach-http-payload.md new file mode 100644 index 0000000000..64b5dbdf3c --- /dev/null +++ b/.changeset/lint-flow-template-rules-reach-http-payload.md @@ -0,0 +1,15 @@ +--- +"@objectstack/lint": patch +--- + +`flow-double-brace-interpolation` and `flow-bare-dollar-reference` now read an `http` node's request payload. Both rules were blind to the whole of `config.body` on every node type — the one key where an uninterpolated token has an outbound consequence. + +The recursive template scan in `lint-flow-patterns.ts` read a region-stripped view of each node's config, and it built that view from the FLAT UNION of every config key that holds a region on *any* node type (`body`, `try`, `catch`, `branches`) rather than from the slots the node in hand actually owns. `body` is `loop`'s region slot **and** the canonical request-payload key on an `http` node, so `config.body` was deleted from every node's view before the scan ever read it. + +That made the two rules silent exactly where they matter most: `http-nodes.ts` interpolates the raw config wholesale, so a double-brace `{{ record.title }}` or a bare `$source.id` written in a payload is never interpolated and ships to the endpoint as literal text. Measured before this change, an `http` node whose `body` carried either token shape — at the top level or nested inside a `try_catch` region — produced zero findings from either rule. + +- **The call site passes its own slots.** `stripRegions(node.config, ownRegionKeys(node.type))`. The remedy was already written in `stripRegions`' own docblock ("Pass the OWNING node's slots, not the flat union") and the sibling call site in `flow-walk.ts` already followed it; this one did not. +- **The trapping default is gone.** `stripRegions`' `regionKeys` parameter is now REQUIRED. The flat union survived as a default only to bound an earlier change, and the cost of leaving it was this defect: the shorter call compiled and quietly asked a different question. A caller that has not decided which set it means now fails to compile instead. +- **The double-count direction is unchanged and pinned.** A token inside a `loop` body is still reported exactly ONCE, against the node that carries it and not also against the container — the reason the strip exists, and the direction that breaks if a repair over-corrects to stripping nothing. + +Both rules keep their existing severity. New findings appear only where a `{{ }}` or bare `$ref.field` sits in a previously-hidden key; measured across `examples/app-showcase`, `app-crm` and `app-todo` (34 flows, `http` payloads inside a `parallel` branch and a `try_catch` try among them), the count is unchanged at zero — those payloads use correct single-brace tokens. diff --git a/packages/lint/src/flow-walk.test.ts b/packages/lint/src/flow-walk.test.ts index 13e9e6c8c9..015274f421 100644 --- a/packages/lint/src/flow-walk.test.ts +++ b/packages/lint/src/flow-walk.test.ts @@ -194,11 +194,38 @@ describe('walkFlowNodes', () => { expect(ownRegionKeys('constructor')).toEqual([]); }); - it('treats an empty key list as a real answer, distinct from omitting it', () => { + it('treats an empty key list as a real answer: strip nothing, same reference', () => { const config = { body: 'payload', try: 'kept' }; expect(stripRegions(config, [])).toBe(config); - // Omitted: the flat-union view its remaining caller was written against. - expect(Object.keys(stripRegions(config) ?? {})).toEqual([]); + }); + + /** + * #16405 — `regionKeys` is REQUIRED, and this is the pin that keeps it so. + * + * It carried the flat union as a DEFAULT until #16405, which meant the + * shorter call compiled and answered a different question than the caller + * was asking: "every key that holds a region on SOME node type" rather than + * "this node's own slots". `lint-flow-patterns.ts` wrote that shorter call + * and was silently blind to an `http` node's `body` — its request payload — + * for as long as it existed. With no default, the omission does not compile. + * + * A `@ts-expect-error` rather than a runtime assertion because the trap was + * only ever visible to the type checker; `tsconfig.test.json` compiles this + * file, so the directive is evaluated (`pnpm --filter @objectstack/lint + * check:test-typecheck`) and re-adding a default makes it unused — TS2578. + */ + it('does not compile when the key list is omitted', () => { + const config = { body: 'payload', try: 'kept' }; + // @ts-expect-error — `regionKeys` is required: the flat-union default is gone. + expect(stripRegions(config)).toBeDefined(); + }); + + it('keeps a non-region key a node type owns as ordinary config', () => { + // `body` on an `http` node is its request payload, not a region. + const config = { url: 'https://x.test', body: { text: 'hi' } }; + expect(stripRegions(config, ownRegionKeys('http'))).toBe(config); + // …and is still stripped on the type that owns it as a region. + expect(Object.keys(stripRegions(config, ownRegionKeys('loop')) ?? {})).toEqual(['url']); }); it('returns undefined for a non-record config, whatever the key list', () => { diff --git a/packages/lint/src/flow-walk.ts b/packages/lint/src/flow-walk.ts index 034991774f..170f7cdd65 100644 --- a/packages/lint/src/flow-walk.ts +++ b/packages/lint/src/flow-walk.ts @@ -151,17 +151,18 @@ export function ownRegionKeys(nodeType: unknown): readonly string[] { * is the reverse of the double-count this view exists to prevent and strictly * worse: a double-count is visible in the output. * - * The union stays the DEFAULT to bound this change to the two rules #16111 - * names — ⛔ NOT because it is the right argument for the caller still taking - * it. `lint-flow-patterns.ts` reads the union view for its own recursive - * template scan, so it is blind to an `http` node's `body` for exactly the - * reason above: the same defect, one call site over, tracked on #16405. Once - * that caller passes its own slots this default has no callers left and - * `regionKeys` must become REQUIRED, so no later caller inherits the trap by - * writing the shorter call. + * `regionKeys` is REQUIRED — there is deliberately no default (#16405). The + * union was the default until #16111's remaining caller was fixed, to bound + * that change to the two rules it named, and the cost of leaving it was exactly + * what this parameter documents: `lint-flow-patterns.ts` inherited the trap by + * writing the shorter call and was blind to an `http` node's `body` for the + * reason above, silently, for as long as the default existed. With the + * parameter required, the next caller that has not decided which question it is + * asking does not compile instead of quietly asking the wrong one. * * `regionKeys: []` is a real answer (strip nothing) and is distinct from - * omitting the parameter. + * {@link ownRegionKeys}' answer for a non-container type, which happens to be + * the same empty list arrived at by asking. * * Exported since #5383 because {@link WalkedFlowNode.localConfig} is not the only * consumer that needs this view. `lint-flow-patterns.ts` walks graphs rather than @@ -171,7 +172,7 @@ export function ownRegionKeys(nodeType: unknown): readonly string[] { */ export function stripRegions( config: unknown, - regionKeys: Iterable = REGION_CONFIG_KEYS, + regionKeys: Iterable, ): AnyRec | undefined { if (!isRec(config)) return undefined; const strip = regionKeys instanceof Set ? regionKeys : new Set(regionKeys); diff --git a/packages/lint/src/lint-flow-patterns.test.ts b/packages/lint/src/lint-flow-patterns.test.ts index 171c712412..fc1094102e 100644 --- a/packages/lint/src/lint-flow-patterns.test.ts +++ b/packages/lint/src/lint-flow-patterns.test.ts @@ -1,7 +1,7 @@ // Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. import { describe, it, expect } from 'vitest'; -import { TimeRelativeTriggerSchema, LoopConfigSchema, ParallelConfigSchema, TryCatchConfigSchema, FlowSchema } from '@objectstack/spec/automation'; +import { TimeRelativeTriggerSchema, LoopConfigSchema, ParallelConfigSchema, TryCatchConfigSchema, HttpConfigSchema, FlowSchema } from '@objectstack/spec/automation'; // [#5659] The shared identity reduction, asserted beside the rule that consumes // it — the rule's verdict and the drivers' verdict are one object now. import { reduceFilterVerdict } from '@objectstack/spec/data'; @@ -2273,3 +2273,184 @@ describe('per-iteration containment (#13681 / #14394)', () => { }); }); }); + +/** + * #16405 — the two #1315 template rules reach an `http` node's REQUEST PAYLOAD. + * + * The recursive template scan read a region-stripped view of each node's config + * built from the FLAT UNION of every region key on ANY node type (`body`, + * `try`, `catch`, `branches`), rather than from the slots the node in hand + * actually owns. `body` is `loop`'s region slot AND the canonical request-payload + * key on an `http` node (`HttpConfigSchema.body`), so `config.body` was deleted + * from EVERY node's view before the scan read it — and the payload is the one + * place an uninterpolated token reaches a real outbound request, because + * `http-nodes.ts` interpolates the raw config wholesale. + * + * Measured on the parent commit, both directions: every case in this block + * returned ZERO findings for its rule before the call site passed + * `ownRegionKeys(node.type)`. + * + * The over-correction direction is pinned too, and it is the one that breaks if + * a repair strips NOTHING: the last case here keeps a payload token inside a + * `loop` body reported exactly ONCE, on the node carrying it. + */ + +/** An `http` push node's config: a real URL, a real method, and the payload under test. */ +const httpPushConfig = (body: unknown) => ({ + url: 'https://api.example.com/v1/incidents', + method: 'POST', + body, +}); + +/** A scheduled flow whose one `http` node carries the payload under test. */ +function httpFlow(body: unknown) { + return { + flows: [{ + name: 'incident_push', + runAs: 'system', + nodes: [ + { id: 'start', type: 'start', config: { triggerType: 'schedule', schedule: 'cron:0 3 * * *' } }, + { id: 'push', type: 'http', label: 'POST incident', config: httpPushConfig(body) }, + ], + edges: [{ id: 'e1', source: 'start', target: 'push' }], + }], + }; +} + +/** The same `http` node, moved inside a `try_catch`'s `try` region. */ +function guardedHttpFlow(body: unknown) { + return { + flows: [{ + name: 'incident_push', + runAs: 'system', + nodes: [ + { id: 'start', type: 'start', config: { triggerType: 'schedule', schedule: 'cron:0 3 * * *' } }, + { + id: 'guard', type: 'try_catch', label: 'Guard', + config: { + try: { + nodes: [{ id: 'push', type: 'http', label: 'POST incident', config: httpPushConfig(body) }], + edges: [], + }, + catch: { + nodes: [{ id: 'log_failure', type: 'create_record', label: 'Log failure', config: { objectName: 'sync_error' } }], + edges: [], + }, + }, + }, + ], + edges: [{ id: 'e1', source: 'start', target: 'guard' }], + }], + }; +} + +describe('#16405 — an `http` node payload is not a region, and both #1315 rules read it', () => { + /** + * #5700's bar, applied here: a payload these rules judge has to be one an + * author can really write, or the pins prove a rule against metadata the + * schema refuses. `HttpConfigSchema` is a `strictObject`, so a misspelled key + * would surface as an `unrecognized_key` rather than being dropped. + */ + it('pins the fixture payload as an authorable `http` config', () => { + const parsed = HttpConfigSchema.safeParse(httpPushConfig({ text: 'Incident {{record.title}}' })); + expect(parsed.success ? [] : parsed.error.issues.map((i) => `${i.path.join('.')}: ${i.message}`)).toEqual([]); + expect(parsed.success).toBe(true); + }); + + it('pins the guarded fixture as an authorable `try_catch` config', () => { + const cfg = (guardedHttpFlow({ text: '{{record.title}}' }).flows[0].nodes[1] as { config: unknown }).config; + const parsed = TryCatchConfigSchema.safeParse(cfg); + expect(parsed.success ? [] : parsed.error.issues.map((i) => `${i.path.join('.')}: ${i.message}`)).toEqual([]); + expect(parsed.success).toBe(true); + }); + + describe('flow-double-brace-interpolation', () => { + it('flags a `{{ }}` token in a top-level `http` node payload', () => { + const fnds = lintFlowPatterns(httpFlow({ text: 'Incident {{record.title}}' })) + .filter((f) => f.rule === FLOW_DOUBLE_BRACE_INTERP); + expect(fnds).toHaveLength(1); + expect(fnds[0].where).toBe("flow 'incident_push' · node 'push' (http)"); + expect(fnds[0].message).toContain('{{record.title}}'); + }); + + it('flags the same token when the `http` node sits inside a region', () => { + const fnds = lintFlowPatterns(guardedHttpFlow({ text: 'Incident {{record.title}}' })) + .filter((f) => f.rule === FLOW_DOUBLE_BRACE_INTERP); + expect(fnds).toHaveLength(1); + expect(fnds[0].where).toBe("flow 'incident_push' · try_catch 'guard' try · node 'push' (http)"); + }); + + it('reaches a token nested deep inside the payload, not only its top level', () => { + const fnds = lintFlowPatterns(httpFlow({ fields: [{ value: '{{record.amount}}' }] })) + .filter((f) => f.rule === FLOW_DOUBLE_BRACE_INTERP); + expect(fnds).toHaveLength(1); + }); + }); + + describe('flow-bare-dollar-reference', () => { + it('flags a bare `$ref.field` in a top-level `http` node payload', () => { + const fnds = lintFlowPatterns(httpFlow({ ticket: '$source.id' })) + .filter((f) => f.rule === FLOW_BARE_DOLLAR_REF); + expect(fnds).toHaveLength(1); + expect(fnds[0].where).toBe("flow 'incident_push' · node 'push' (http)"); + expect(fnds[0].message).toContain('$source.id'); + }); + + it('flags the same reference when the `http` node sits inside a region', () => { + const fnds = lintFlowPatterns(guardedHttpFlow({ ticket: '$source.id' })) + .filter((f) => f.rule === FLOW_BARE_DOLLAR_REF); + expect(fnds).toHaveLength(1); + expect(fnds[0].where).toBe("flow 'incident_push' · try_catch 'guard' try · node 'push' (http)"); + }); + }); + + it('still raises nothing for a correct single-brace payload', () => { + const fnds = lintFlowPatterns(httpFlow({ id: '{record.id}', owner: '{$User.Id}', note: 'Total $5' })) + .filter((f) => f.rule === FLOW_DOUBLE_BRACE_INTERP || f.rule === FLOW_BARE_DOLLAR_REF); + expect(fnds).toEqual([]); + }); + + /** + * The over-correction guard, and the reason the second argument must be the + * node's OWN slots rather than nothing at all: a `loop`'s config physically + * CONTAINS its body, so a repair that stopped stripping would report this + * payload token twice — once on the `http` node that carries it, once on the + * `loop` that merely wraps it. + */ + it('reports a payload token inside a `loop` body ONCE, on the node carrying it', () => { + const fnds = lintFlowPatterns(loopBodyFlow({ + nodes: [{ + id: 'push', type: 'http', label: 'POST incident', + config: httpPushConfig({ text: 'Lead {{lead.name}}' }), + }], + edges: [], + // Scoped to this rule: an `http` node in a bare loop body also trips the + // #14394 containment warning, a different finding about a different defect. + })).filter((f) => f.rule === FLOW_DOUBLE_BRACE_INTERP); + expect(fnds).toHaveLength(1); + expect(fnds[0].where).toBe( + "flow 'campaign_enrollment' · loop 'loop_leads' body · node 'push' (http)", + ); + expect(fnds[0].where).not.toContain("node 'loop_leads'"); + }); + + /** + * `try` / `catch` / `branches` — the rest of the flat union — for the same + * reason, on a node type that does not own them. No node type in the protocol + * declares these as ordinary config today (only `try_catch` and `parallel` + * own them, as regions), but `FlowNodeSchema.config` is an open `z.record`, so + * an authored key by any of those names on any other node type is metadata a + * rule must still read rather than silently delete. + */ + it('reads a `try` / `catch` / `branches` key authored on a node that owns no region', () => { + const fnds = lintFlowPatterns(nodeFlow({ + objectName: 'm', + fields: { try: '{{a}}', catch: '{{b}}', branches: '{{c}}' }, + })).filter((f) => f.rule === FLOW_DOUBLE_BRACE_INTERP); + expect(fnds).toHaveLength(3); + // Top level too, not only nested under a declared key. + const top = lintFlowPatterns(nodeFlow({ objectName: 'm', try: '{{a}}', catch: '{{b}}', branches: '{{c}}' })) + .filter((f) => f.rule === FLOW_DOUBLE_BRACE_INTERP); + expect(top).toHaveLength(3); + }); +}); diff --git a/packages/lint/src/lint-flow-patterns.ts b/packages/lint/src/lint-flow-patterns.ts index 03215ed9fd..f73cc7ef94 100644 --- a/packages/lint/src/lint-flow-patterns.ts +++ b/packages/lint/src/lint-flow-patterns.ts @@ -163,7 +163,7 @@ import type { FlowNodeParsed, FlowEdgeParsed } from '@objectstack/spec/automatio // driver-sql, driver-mongodb and driver-memory execute. This linter asks it // rather than hand-writing a fourth copy; see {@link filterCarriesNoCondition}. import { reduceFilterVerdict } from '@objectstack/spec/data'; -import { stripRegions, REGION_SLOTS, MAX_REGION_DEPTH } from './flow-walk.js'; +import { stripRegions, ownRegionKeys, REGION_SLOTS, MAX_REGION_DEPTH } from './flow-walk.js'; import { recordsOf } from './object-graph.js'; export interface FlowLintFinding { @@ -1552,14 +1552,27 @@ export function lintFlowPatterns(stack: AnyRec): FlowLintFinding[] { } } - // Region-STRIPPED: this scan is recursive and a container's config - // physically contains every descendant's, which the walk above already - // visits in its own right. Without the strip a `{{ }}` in a loop body - // would be reported twice — once here against the `loop`, once against the - // node that carries it. With it, the count stays 1 and the finding lands - // on the right node (before #5383 it landed only on the container). + // Region-STRIPPED, by THIS node type's own slots (#16405). The scan is + // recursive and a container's config physically contains every + // descendant's, which the walk above already visits in its own right: + // without the strip a `{{ }}` in a loop body would be reported twice — + // once here against the `loop`, once against the node that carries it. + // With it, the count stays 1 and the finding lands on the right node + // (before #5383 it landed only on the container). + // + // `ownRegionKeys(node.type)` rather than the flat union of every region + // key on ANY node type, which is what this call site passed until #16405 + // by taking `stripRegions`' default. That union deleted `body` from every + // node's view — and `body` is `loop`'s region slot AND the canonical + // request payload on an `http` node, so the whole of an `http` node's + // payload was invisible to both rules below. That is the one key where an + // uninterpolated token has an outbound consequence: `http-nodes.ts` + // interpolates the raw config wholesale, so a `{{ }}` or a bare `$ref.x` + // there ships to the endpoint as literal text. Remove fewer than the + // node's own slots and the double-count returns; remove more and a key + // that was never a region is deleted unread. const strings: string[] = []; - collectTemplateStrings(stripRegions(node.config), undefined, strings); + collectTemplateStrings(stripRegions(node.config, ownRegionKeys(node.type)), undefined, strings); for (const str of strings) { if (DOUBLE_BRACE.test(str)) { findings.push({