Skip to content

Commit 837710e

Browse files
committed
fix(lint): the flow-template rules read an http node's request payload
`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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012GKcPZbMoGq7WPzKLfRBTU
1 parent b37f0b1 commit 837710e

5 files changed

Lines changed: 259 additions & 22 deletions

File tree

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
---
2+
"@objectstack/lint": patch
3+
---
4+
5+
`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.
6+
7+
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.
8+
9+
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.
10+
11+
- **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.
12+
- **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.
13+
- **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.
14+
15+
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.

packages/lint/src/flow-walk.test.ts

Lines changed: 30 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -194,11 +194,38 @@ describe('walkFlowNodes', () => {
194194
expect(ownRegionKeys('constructor')).toEqual([]);
195195
});
196196

197-
it('treats an empty key list as a real answer, distinct from omitting it', () => {
197+
it('treats an empty key list as a real answer: strip nothing, same reference', () => {
198198
const config = { body: 'payload', try: 'kept' };
199199
expect(stripRegions(config, [])).toBe(config);
200-
// Omitted: the flat-union view its remaining caller was written against.
201-
expect(Object.keys(stripRegions(config) ?? {})).toEqual([]);
200+
});
201+
202+
/**
203+
* #16405 — `regionKeys` is REQUIRED, and this is the pin that keeps it so.
204+
*
205+
* It carried the flat union as a DEFAULT until #16405, which meant the
206+
* shorter call compiled and answered a different question than the caller
207+
* was asking: "every key that holds a region on SOME node type" rather than
208+
* "this node's own slots". `lint-flow-patterns.ts` wrote that shorter call
209+
* and was silently blind to an `http` node's `body` — its request payload —
210+
* for as long as it existed. With no default, the omission does not compile.
211+
*
212+
* A `@ts-expect-error` rather than a runtime assertion because the trap was
213+
* only ever visible to the type checker; `tsconfig.test.json` compiles this
214+
* file, so the directive is evaluated (`pnpm --filter @objectstack/lint
215+
* check:test-typecheck`) and re-adding a default makes it unused — TS2578.
216+
*/
217+
it('does not compile when the key list is omitted', () => {
218+
const config = { body: 'payload', try: 'kept' };
219+
// @ts-expect-error — `regionKeys` is required: the flat-union default is gone.
220+
expect(stripRegions(config)).toBeDefined();
221+
});
222+
223+
it('keeps a non-region key a node type owns as ordinary config', () => {
224+
// `body` on an `http` node is its request payload, not a region.
225+
const config = { url: 'https://x.test', body: { text: 'hi' } };
226+
expect(stripRegions(config, ownRegionKeys('http'))).toBe(config);
227+
// …and is still stripped on the type that owns it as a region.
228+
expect(Object.keys(stripRegions(config, ownRegionKeys('loop')) ?? {})).toEqual(['url']);
202229
});
203230

204231
it('returns undefined for a non-record config, whatever the key list', () => {

packages/lint/src/flow-walk.ts

Lines changed: 11 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -151,17 +151,18 @@ export function ownRegionKeys(nodeType: unknown): readonly string[] {
151151
* is the reverse of the double-count this view exists to prevent and strictly
152152
* worse: a double-count is visible in the output.
153153
*
154-
* The union stays the DEFAULT to bound this change to the two rules #16111
155-
* names — ⛔ NOT because it is the right argument for the caller still taking
156-
* it. `lint-flow-patterns.ts` reads the union view for its own recursive
157-
* template scan, so it is blind to an `http` node's `body` for exactly the
158-
* reason above: the same defect, one call site over, tracked on #16405. Once
159-
* that caller passes its own slots this default has no callers left and
160-
* `regionKeys` must become REQUIRED, so no later caller inherits the trap by
161-
* writing the shorter call.
154+
* `regionKeys` is REQUIRED — there is deliberately no default (#16405). The
155+
* union was the default until #16111's remaining caller was fixed, to bound
156+
* that change to the two rules it named, and the cost of leaving it was exactly
157+
* what this parameter documents: `lint-flow-patterns.ts` inherited the trap by
158+
* writing the shorter call and was blind to an `http` node's `body` for the
159+
* reason above, silently, for as long as the default existed. With the
160+
* parameter required, the next caller that has not decided which question it is
161+
* asking does not compile instead of quietly asking the wrong one.
162162
*
163163
* `regionKeys: []` is a real answer (strip nothing) and is distinct from
164-
* omitting the parameter.
164+
* {@link ownRegionKeys}' answer for a non-container type, which happens to be
165+
* the same empty list arrived at by asking.
165166
*
166167
* Exported since #5383 because {@link WalkedFlowNode.localConfig} is not the only
167168
* consumer that needs this view. `lint-flow-patterns.ts` walks graphs rather than
@@ -171,7 +172,7 @@ export function ownRegionKeys(nodeType: unknown): readonly string[] {
171172
*/
172173
export function stripRegions(
173174
config: unknown,
174-
regionKeys: Iterable<string> = REGION_CONFIG_KEYS,
175+
regionKeys: Iterable<string>,
175176
): AnyRec | undefined {
176177
if (!isRec(config)) return undefined;
177178
const strip = regionKeys instanceof Set ? regionKeys : new Set(regionKeys);

packages/lint/src/lint-flow-patterns.test.ts

Lines changed: 182 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license.
22

33
import { describe, it, expect } from 'vitest';
4-
import { TimeRelativeTriggerSchema, LoopConfigSchema, ParallelConfigSchema, TryCatchConfigSchema, FlowSchema } from '@objectstack/spec/automation';
4+
import { TimeRelativeTriggerSchema, LoopConfigSchema, ParallelConfigSchema, TryCatchConfigSchema, HttpConfigSchema, FlowSchema } from '@objectstack/spec/automation';
55
// [#5659] The shared identity reduction, asserted beside the rule that consumes
66
// it — the rule's verdict and the drivers' verdict are one object now.
77
import { reduceFilterVerdict } from '@objectstack/spec/data';
@@ -2273,3 +2273,184 @@ describe('per-iteration containment (#13681 / #14394)', () => {
22732273
});
22742274
});
22752275
});
2276+
2277+
/**
2278+
* #16405 — the two #1315 template rules reach an `http` node's REQUEST PAYLOAD.
2279+
*
2280+
* The recursive template scan read a region-stripped view of each node's config
2281+
* built from the FLAT UNION of every region key on ANY node type (`body`,
2282+
* `try`, `catch`, `branches`), rather than from the slots the node in hand
2283+
* actually owns. `body` is `loop`'s region slot AND the canonical request-payload
2284+
* key on an `http` node (`HttpConfigSchema.body`), so `config.body` was deleted
2285+
* from EVERY node's view before the scan read it — and the payload is the one
2286+
* place an uninterpolated token reaches a real outbound request, because
2287+
* `http-nodes.ts` interpolates the raw config wholesale.
2288+
*
2289+
* Measured on the parent commit, both directions: every case in this block
2290+
* returned ZERO findings for its rule before the call site passed
2291+
* `ownRegionKeys(node.type)`.
2292+
*
2293+
* The over-correction direction is pinned too, and it is the one that breaks if
2294+
* a repair strips NOTHING: the last case here keeps a payload token inside a
2295+
* `loop` body reported exactly ONCE, on the node carrying it.
2296+
*/
2297+
2298+
/** An `http` push node's config: a real URL, a real method, and the payload under test. */
2299+
const httpPushConfig = (body: unknown) => ({
2300+
url: 'https://api.example.com/v1/incidents',
2301+
method: 'POST',
2302+
body,
2303+
});
2304+
2305+
/** A scheduled flow whose one `http` node carries the payload under test. */
2306+
function httpFlow(body: unknown) {
2307+
return {
2308+
flows: [{
2309+
name: 'incident_push',
2310+
runAs: 'system',
2311+
nodes: [
2312+
{ id: 'start', type: 'start', config: { triggerType: 'schedule', schedule: 'cron:0 3 * * *' } },
2313+
{ id: 'push', type: 'http', label: 'POST incident', config: httpPushConfig(body) },
2314+
],
2315+
edges: [{ id: 'e1', source: 'start', target: 'push' }],
2316+
}],
2317+
};
2318+
}
2319+
2320+
/** The same `http` node, moved inside a `try_catch`'s `try` region. */
2321+
function guardedHttpFlow(body: unknown) {
2322+
return {
2323+
flows: [{
2324+
name: 'incident_push',
2325+
runAs: 'system',
2326+
nodes: [
2327+
{ id: 'start', type: 'start', config: { triggerType: 'schedule', schedule: 'cron:0 3 * * *' } },
2328+
{
2329+
id: 'guard', type: 'try_catch', label: 'Guard',
2330+
config: {
2331+
try: {
2332+
nodes: [{ id: 'push', type: 'http', label: 'POST incident', config: httpPushConfig(body) }],
2333+
edges: [],
2334+
},
2335+
catch: {
2336+
nodes: [{ id: 'log_failure', type: 'create_record', label: 'Log failure', config: { objectName: 'sync_error' } }],
2337+
edges: [],
2338+
},
2339+
},
2340+
},
2341+
],
2342+
edges: [{ id: 'e1', source: 'start', target: 'guard' }],
2343+
}],
2344+
};
2345+
}
2346+
2347+
describe('#16405 — an `http` node payload is not a region, and both #1315 rules read it', () => {
2348+
/**
2349+
* #5700's bar, applied here: a payload these rules judge has to be one an
2350+
* author can really write, or the pins prove a rule against metadata the
2351+
* schema refuses. `HttpConfigSchema` is a `strictObject`, so a misspelled key
2352+
* would surface as an `unrecognized_key` rather than being dropped.
2353+
*/
2354+
it('pins the fixture payload as an authorable `http` config', () => {
2355+
const parsed = HttpConfigSchema.safeParse(httpPushConfig({ text: 'Incident {{record.title}}' }));
2356+
expect(parsed.success ? [] : parsed.error.issues.map((i) => `${i.path.join('.')}: ${i.message}`)).toEqual([]);
2357+
expect(parsed.success).toBe(true);
2358+
});
2359+
2360+
it('pins the guarded fixture as an authorable `try_catch` config', () => {
2361+
const cfg = (guardedHttpFlow({ text: '{{record.title}}' }).flows[0].nodes[1] as { config: unknown }).config;
2362+
const parsed = TryCatchConfigSchema.safeParse(cfg);
2363+
expect(parsed.success ? [] : parsed.error.issues.map((i) => `${i.path.join('.')}: ${i.message}`)).toEqual([]);
2364+
expect(parsed.success).toBe(true);
2365+
});
2366+
2367+
describe('flow-double-brace-interpolation', () => {
2368+
it('flags a `{{ }}` token in a top-level `http` node payload', () => {
2369+
const fnds = lintFlowPatterns(httpFlow({ text: 'Incident {{record.title}}' }))
2370+
.filter((f) => f.rule === FLOW_DOUBLE_BRACE_INTERP);
2371+
expect(fnds).toHaveLength(1);
2372+
expect(fnds[0].where).toBe("flow 'incident_push' · node 'push' (http)");
2373+
expect(fnds[0].message).toContain('{{record.title}}');
2374+
});
2375+
2376+
it('flags the same token when the `http` node sits inside a region', () => {
2377+
const fnds = lintFlowPatterns(guardedHttpFlow({ text: 'Incident {{record.title}}' }))
2378+
.filter((f) => f.rule === FLOW_DOUBLE_BRACE_INTERP);
2379+
expect(fnds).toHaveLength(1);
2380+
expect(fnds[0].where).toBe("flow 'incident_push' · try_catch 'guard' try · node 'push' (http)");
2381+
});
2382+
2383+
it('reaches a token nested deep inside the payload, not only its top level', () => {
2384+
const fnds = lintFlowPatterns(httpFlow({ fields: [{ value: '{{record.amount}}' }] }))
2385+
.filter((f) => f.rule === FLOW_DOUBLE_BRACE_INTERP);
2386+
expect(fnds).toHaveLength(1);
2387+
});
2388+
});
2389+
2390+
describe('flow-bare-dollar-reference', () => {
2391+
it('flags a bare `$ref.field` in a top-level `http` node payload', () => {
2392+
const fnds = lintFlowPatterns(httpFlow({ ticket: '$source.id' }))
2393+
.filter((f) => f.rule === FLOW_BARE_DOLLAR_REF);
2394+
expect(fnds).toHaveLength(1);
2395+
expect(fnds[0].where).toBe("flow 'incident_push' · node 'push' (http)");
2396+
expect(fnds[0].message).toContain('$source.id');
2397+
});
2398+
2399+
it('flags the same reference when the `http` node sits inside a region', () => {
2400+
const fnds = lintFlowPatterns(guardedHttpFlow({ ticket: '$source.id' }))
2401+
.filter((f) => f.rule === FLOW_BARE_DOLLAR_REF);
2402+
expect(fnds).toHaveLength(1);
2403+
expect(fnds[0].where).toBe("flow 'incident_push' · try_catch 'guard' try · node 'push' (http)");
2404+
});
2405+
});
2406+
2407+
it('still raises nothing for a correct single-brace payload', () => {
2408+
const fnds = lintFlowPatterns(httpFlow({ id: '{record.id}', owner: '{$User.Id}', note: 'Total $5' }))
2409+
.filter((f) => f.rule === FLOW_DOUBLE_BRACE_INTERP || f.rule === FLOW_BARE_DOLLAR_REF);
2410+
expect(fnds).toEqual([]);
2411+
});
2412+
2413+
/**
2414+
* The over-correction guard, and the reason the second argument must be the
2415+
* node's OWN slots rather than nothing at all: a `loop`'s config physically
2416+
* CONTAINS its body, so a repair that stopped stripping would report this
2417+
* payload token twice — once on the `http` node that carries it, once on the
2418+
* `loop` that merely wraps it.
2419+
*/
2420+
it('reports a payload token inside a `loop` body ONCE, on the node carrying it', () => {
2421+
const fnds = lintFlowPatterns(loopBodyFlow({
2422+
nodes: [{
2423+
id: 'push', type: 'http', label: 'POST incident',
2424+
config: httpPushConfig({ text: 'Lead {{lead.name}}' }),
2425+
}],
2426+
edges: [],
2427+
// Scoped to this rule: an `http` node in a bare loop body also trips the
2428+
// #14394 containment warning, a different finding about a different defect.
2429+
})).filter((f) => f.rule === FLOW_DOUBLE_BRACE_INTERP);
2430+
expect(fnds).toHaveLength(1);
2431+
expect(fnds[0].where).toBe(
2432+
"flow 'campaign_enrollment' · loop 'loop_leads' body · node 'push' (http)",
2433+
);
2434+
expect(fnds[0].where).not.toContain("node 'loop_leads'");
2435+
});
2436+
2437+
/**
2438+
* `try` / `catch` / `branches` — the rest of the flat union — for the same
2439+
* reason, on a node type that does not own them. No node type in the protocol
2440+
* declares these as ordinary config today (only `try_catch` and `parallel`
2441+
* own them, as regions), but `FlowNodeSchema.config` is an open `z.record`, so
2442+
* an authored key by any of those names on any other node type is metadata a
2443+
* rule must still read rather than silently delete.
2444+
*/
2445+
it('reads a `try` / `catch` / `branches` key authored on a node that owns no region', () => {
2446+
const fnds = lintFlowPatterns(nodeFlow({
2447+
objectName: 'm',
2448+
fields: { try: '{{a}}', catch: '{{b}}', branches: '{{c}}' },
2449+
})).filter((f) => f.rule === FLOW_DOUBLE_BRACE_INTERP);
2450+
expect(fnds).toHaveLength(3);
2451+
// Top level too, not only nested under a declared key.
2452+
const top = lintFlowPatterns(nodeFlow({ objectName: 'm', try: '{{a}}', catch: '{{b}}', branches: '{{c}}' }))
2453+
.filter((f) => f.rule === FLOW_DOUBLE_BRACE_INTERP);
2454+
expect(top).toHaveLength(3);
2455+
});
2456+
});

packages/lint/src/lint-flow-patterns.ts

Lines changed: 21 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -163,7 +163,7 @@ import type { FlowNodeParsed, FlowEdgeParsed } from '@objectstack/spec/automatio
163163
// driver-sql, driver-mongodb and driver-memory execute. This linter asks it
164164
// rather than hand-writing a fourth copy; see {@link filterCarriesNoCondition}.
165165
import { reduceFilterVerdict } from '@objectstack/spec/data';
166-
import { stripRegions, REGION_SLOTS, MAX_REGION_DEPTH } from './flow-walk.js';
166+
import { stripRegions, ownRegionKeys, REGION_SLOTS, MAX_REGION_DEPTH } from './flow-walk.js';
167167
import { recordsOf } from './object-graph.js';
168168

169169
export interface FlowLintFinding {
@@ -1552,14 +1552,27 @@ export function lintFlowPatterns(stack: AnyRec): FlowLintFinding[] {
15521552
}
15531553
}
15541554

1555-
// Region-STRIPPED: this scan is recursive and a container's config
1556-
// physically contains every descendant's, which the walk above already
1557-
// visits in its own right. Without the strip a `{{ }}` in a loop body
1558-
// would be reported twice — once here against the `loop`, once against the
1559-
// node that carries it. With it, the count stays 1 and the finding lands
1560-
// on the right node (before #5383 it landed only on the container).
1555+
// Region-STRIPPED, by THIS node type's own slots (#16405). The scan is
1556+
// recursive and a container's config physically contains every
1557+
// descendant's, which the walk above already visits in its own right:
1558+
// without the strip a `{{ }}` in a loop body would be reported twice —
1559+
// once here against the `loop`, once against the node that carries it.
1560+
// With it, the count stays 1 and the finding lands on the right node
1561+
// (before #5383 it landed only on the container).
1562+
//
1563+
// `ownRegionKeys(node.type)` rather than the flat union of every region
1564+
// key on ANY node type, which is what this call site passed until #16405
1565+
// by taking `stripRegions`' default. That union deleted `body` from every
1566+
// node's view — and `body` is `loop`'s region slot AND the canonical
1567+
// request payload on an `http` node, so the whole of an `http` node's
1568+
// payload was invisible to both rules below. That is the one key where an
1569+
// uninterpolated token has an outbound consequence: `http-nodes.ts`
1570+
// interpolates the raw config wholesale, so a `{{ }}` or a bare `$ref.x`
1571+
// there ships to the endpoint as literal text. Remove fewer than the
1572+
// node's own slots and the double-count returns; remove more and a key
1573+
// that was never a region is deleted unread.
15611574
const strings: string[] = [];
1562-
collectTemplateStrings(stripRegions(node.config), undefined, strings);
1575+
collectTemplateStrings(stripRegions(node.config, ownRegionKeys(node.type)), undefined, strings);
15631576
for (const str of strings) {
15641577
if (DOUBLE_BRACE.test(str)) {
15651578
findings.push({

0 commit comments

Comments
 (0)