diff --git a/.changeset/page-walk-cycle-guard.md b/.changeset/page-walk-cycle-guard.md new file mode 100644 index 0000000000..2f25ac0136 --- /dev/null +++ b/.changeset/page-walk-cycle-guard.md @@ -0,0 +1,48 @@ +--- +"@objectstack/lint": patch +--- + +fix(lint): guard `walkPageComponents` against component cycles (#13217) + +`walkPageComponents` — the one shared page-component traversal under every +page-shaped lint rule and the CLI's i18n object-sections pass — descended the +untyped composition slots inside `properties` with no cycle guard. Every one of +those slots is `z.array(z.unknown())` authored data, so a component whose +`properties.children` contains itself is **legal input**, and feeding one in +recursed until the stack died with `RangeError: Maximum call stack size +exceeded`. Because the walk is shared rather than copied, that crash was not +scoped to one rule: it took every rule standing on the walk down in the same +process. + +The descent now carries an **ancestor set** — the node is added before +descending and removed on the way out — so a node that is its own ancestor +stops the descent. Measured on the shapes that matter: a direct self-reference, +an indirect cycle (`A -> B -> A`) and a longer chain (`A -> B -> C -> A`) all +terminate, through every descended slot (`properties.children`, +`properties.items[].children`, `properties.body`, `properties.footer`). + +Two deliberate non-changes, both pinned: + +- **An ancestor set, not a visited set.** A component object placed twice as a + *sibling*, or reached down two different branches, is legitimate re-use at two + distinct config paths, and every rule built on this walk must see both + placements. A visited set would yield the first and silently drop the rest — + trading a loud crash for missing lint coverage. This matches the predicate the + sibling resolver `translatePage` already settled on. +- **No depth cap.** A cap and a cycle guard are different instruments. On a + resolver a cap leaves copy untranslated; on a lint walk it would drop real + components from the walk output and every rule would go quiet about them — a + silent truncation that reads exactly like a clean page. With the cycle guard + the descent is bounded by the document's own finite nesting, so a cap could + only ever fire on acyclic input, which is the input it must not truncate. + +The guard is silent: a cycle stops the descent and yields nothing extra, and no +finding or warning is produced. Deciding that a self-referential page is itself +an authoring error would be new reject behaviour on authored input, which is a +contract call and not this walk's to make. Measured on a cyclic-but-otherwise +valid page, all six rules that route through the walk report exactly what they +report for the equivalent acyclic document (zero findings either way). + +No authored page in this repo carries such a cycle — swept across 57 +page-shaped objects with a positive control, zero hits — so this fixes a +reachable crash, not an active incident. diff --git a/packages/lint/src/page-walk.test.ts b/packages/lint/src/page-walk.test.ts index 3bccaad8c3..498b7a22fd 100644 --- a/packages/lint/src/page-walk.test.ts +++ b/packages/lint/src/page-walk.test.ts @@ -140,6 +140,106 @@ describe('walkPageComponents — object binding precedence', () => { }); }); +describe('walkPageComponents — cycle guard', () => { + // `properties.children` is `z.array(z.unknown())`, so a component that + // contains itself is LEGAL input. Before the guard each of these died with + // `RangeError: Maximum call stack size exceeded`, taking down every rule + // built on this walk in the same process. + + it('terminates on an INDIRECT cycle (A -> B -> A), yielding each node once', () => { + // The load-bearing case: a guard that only compares a node against its + // immediate parent still recurses forever here, so this is the fixture a + // direct-only half-fix fails. + const a: Record = { type: 'a', properties: {} }; + const b: Record = { type: 'b', properties: {} }; + (a.properties as Record).children = [b]; + (b.properties as Record).children = [a]; + + expect(paths({ regions: [{ name: 'main', components: [a] }] })).toEqual([ + 'pages[0].regions[0].components[0]', + 'pages[0].regions[0].components[0].properties.children[0]', + ]); + }); + + it('terminates on a DIRECT self-reference', () => { + const self: Record = { type: 'a', properties: {} }; + (self.properties as Record).children = [self]; + + expect(paths({ regions: [{ name: 'main', components: [self] }] })).toEqual([ + 'pages[0].regions[0].components[0]', + ]); + }); + + it('terminates on a cycle through a longer chain (A -> B -> C -> A)', () => { + const a: Record = { type: 'a', properties: {} }; + const b: Record = { type: 'b', properties: {} }; + const c: Record = { type: 'c', properties: {} }; + (a.properties as Record).children = [b]; + (b.properties as Record).children = [c]; + (c.properties as Record).children = [a]; + + expect(paths({ regions: [{ name: 'main', components: [a] }] })).toHaveLength(3); + }); + + it('guards every descended slot, not just `properties.children`', () => { + // items[].children (`page:tabs`), body and footer (`page:card`) all recurse + // through the same `visit`, so each needs the guard to hold. + const viaItems: Record = { type: 'tabs', properties: {} }; + (viaItems.properties as Record).items = [{ children: [viaItems] }]; + + const viaBody: Record = { type: 'card', properties: {} }; + (viaBody.properties as Record).body = [viaBody]; + + const viaFooter: Record = { type: 'card', properties: {} }; + (viaFooter.properties as Record).footer = [viaFooter]; + + for (const node of [viaItems, viaBody, viaFooter]) { + expect(paths({ regions: [{ name: 'main', components: [node] }] })).toHaveLength(1); + } + }); + + it('is an ANCESTOR guard, not a visited set — a re-used sibling is yielded twice', () => { + // The same component object placed twice under one parent is two + // legitimate placements at two distinct config paths, and every rule built + // on this walk must see both. A visited-set guard would yield the first + // and silently drop the second, trading the crash for missing coverage. + const leaf: Record = { type: 'leaf' }; + const parent = { type: 'flex', properties: { children: [leaf, leaf] } }; + + expect(paths({ regions: [{ name: 'main', components: [parent] }] })).toEqual([ + 'pages[0].regions[0].components[0]', + 'pages[0].regions[0].components[0].properties.children[0]', + 'pages[0].regions[0].components[0].properties.children[1]', + ]); + }); + + it('re-uses the same node on a SEPARATE branch — it is not an ancestor there', () => { + // A shared sub-tree reached down two different branches is legal re-use. + // The ancestor set must be popped on the way out, or the second branch + // would be silently truncated. + const shared: Record = { type: 'shared' }; + const left = { type: 'flex', properties: { children: [shared] } }; + const right = { type: 'flex', properties: { children: [shared] } }; + + expect(paths({ regions: [{ name: 'main', components: [left, right] }] })).toEqual([ + 'pages[0].regions[0].components[0]', + 'pages[0].regions[0].components[0].properties.children[0]', + 'pages[0].regions[0].components[1]', + 'pages[0].regions[0].components[1].properties.children[0]', + ]); + }); + + it('does NOT truncate a legal deep tree — this is a cycle guard, not a depth cap', () => { + // A cap would bound a legal-but-deep document by dropping components from + // the walk, and every rule would go quiet about them. Nesting well past + // the sibling resolver's cap of 32 stays fully walked. + let node: Record = { type: 'leaf' }; + for (let i = 0; i < 64; i++) node = { type: 'flex', properties: { children: [node] } }; + + expect(paths({ regions: [{ name: 'main', components: [node] }] })).toHaveLength(65); + }); +}); + describe('isSourceAuthoredPage', () => { it('treats html/react/jsx as source-authored and skips their regions', () => { for (const kind of ['html', 'react', 'jsx']) { diff --git a/packages/lint/src/page-walk.ts b/packages/lint/src/page-walk.ts index 58dd0ab5f8..ee1d709699 100644 --- a/packages/lint/src/page-walk.ts +++ b/packages/lint/src/page-walk.ts @@ -63,6 +63,36 @@ export function isSourceAuthoredPage(page: AnyRec): boolean { * path and resolved object binding. Source-authored pages yield nothing. * * `pagePath` is the caller's path prefix for the page (e.g. `pages[3]`). + * + * The descent is cycle-safe. Every composition slot below is `z.array(z.unknown())` + * authored data, so a component that contains itself — directly, or through a + * chain of containers — is LEGAL input, and an unguarded walk recurses until the + * stack dies. That crash is not scoped to one rule: this is the one shared + * traversal under every page-shaped lint rule and the CLI's i18n object-sections + * pass, so it takes all of them down in the same process. + * + * The guard is an ANCESTOR set, not a visited set, and the difference is + * load-bearing rather than stylistic — it is the same predicate + * `translatePage` (`packages/spec/src/system/i18n-resolver.ts`) settled on. A + * component object reused twice as a SIBLING is two legitimate placements at two + * distinct config paths, and every rule built on this walk must see both; a + * visited set would yield the first and silently drop the second, converting a + * crash into missing lint coverage. Only a node that is its own ancestor is a + * cycle. + * + * A cycle stops the descent SILENTLY — the repeated node is not yielded a second + * time and no finding is produced. The guard is a safety property of the walk, + * not a verdict about the document: deciding that a self-referential page is + * itself an authoring error would be new reject behaviour on authored input, and + * that is a contract call, not this walk's to make. + * + * Deliberately NO depth cap, which is a different instrument (`translatePage` + * carries both). A cap bounds a legal-but-absurd document; on a resolver it + * leaves copy untranslated, but on a LINT walk it would drop real components + * from the walk output and every rule would go quiet about them — a silent loss + * of coverage that looks exactly like a clean page. With the cycle guard the + * descent is bounded by the document's own finite nesting, so the cap would only + * ever fire on acyclic input, which is precisely the input it must not truncate. */ export function walkPageComponents(page: AnyRec, pagePath: string): WalkedComponent[] { const out: WalkedComponent[] = []; @@ -70,8 +100,13 @@ export function walkPageComponents(page: AnyRec, pagePath: string): WalkedCompon const pageObject = strName(page.object); + // The current descent path — ancestors only, removed again on the way out. + const ancestors = new Set(); + const visit = (node: unknown, path: string, inheritedObject?: string) => { if (!isRec(node)) return; + // Cycle guard: this node is already an ancestor of itself. + if (ancestors.has(node)) return; // Per-element `dataSource` overrides the page object so one page can bind // several objects; an inline `properties.object` does the same for the @@ -85,32 +120,37 @@ export function walkPageComponents(page: AnyRec, pagePath: string): WalkedCompon if (!props) return; - // `page:tabs` / `page:accordion` — items[].children[] - if (Array.isArray(props.items)) { - for (let i = 0; i < props.items.length; i++) { - const item = props.items[i]; - if (!isRec(item) || !Array.isArray(item.children)) continue; - for (let c = 0; c < item.children.length; c++) { - visit(item.children[c], `${path}.properties.items[${i}].children[${c}]`, objectName); + ancestors.add(node); + try { + // `page:tabs` / `page:accordion` — items[].children[] + if (Array.isArray(props.items)) { + for (let i = 0; i < props.items.length; i++) { + const item = props.items[i]; + if (!isRec(item) || !Array.isArray(item.children)) continue; + for (let c = 0; c < item.children.length; c++) { + visit(item.children[c], `${path}.properties.items[${i}].children[${c}]`, objectName); + } } } - } - // Generic layout nesting — `properties.children[]`. Not in any props - // schema, but it is how real pages compose layout containers (`type: - // 'flex'` grids in the showcase command-center wrap every chart this way). - // Omitting it hides whole sub-trees from every rule built on this walk. - if (Array.isArray(props.children)) { - for (let i = 0; i < props.children.length; i++) { - visit(props.children[i], `${path}.properties.children[${i}]`, objectName); + // Generic layout nesting — `properties.children[]`. Not in any props + // schema, but it is how real pages compose layout containers (`type: + // 'flex'` grids in the showcase command-center wrap every chart this way). + // Omitting it hides whole sub-trees from every rule built on this walk. + if (Array.isArray(props.children)) { + for (let i = 0; i < props.children.length; i++) { + visit(props.children[i], `${path}.properties.children[${i}]`, objectName); + } } - } - // `page:card` — body[] / footer[] - for (const key of ['body', 'footer'] as const) { - const slotList = props[key]; - if (!Array.isArray(slotList)) continue; - for (let i = 0; i < slotList.length; i++) { - visit(slotList[i], `${path}.properties.${key}[${i}]`, objectName); + // `page:card` — body[] / footer[] + for (const key of ['body', 'footer'] as const) { + const slotList = props[key]; + if (!Array.isArray(slotList)) continue; + for (let i = 0; i < slotList.length; i++) { + visit(slotList[i], `${path}.properties.${key}[${i}]`, objectName); + } } + } finally { + ancestors.delete(node); } };