From 5ebe3f132050a49c8a2130451fecd63a200c6055 Mon Sep 17 00:00:00 2001 From: vaishnavikedar1511 Date: Tue, 15 Sep 2026 11:25:45 +0530 Subject: [PATCH 1/5] fix: address connector review follow-ups (#4 #5 #3 #1 #2) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-ups surfaced by the documentation/code review. Unit tests added for the three runtime fixes (165 pass, +9); typecheck + schematics green. #4 (security) — shell content skipped access filtering. getGlobalSlots now takes an EntryAccessOptions arg: it scopes the SSR cache key per permission set and sanitizes restricted nested shell components before TransferState. The page adapter threads permissions into the shell fetch (gateRoot:false — the shell root is never hidden) and into toGlobalStructure -> buildStructure so the CSR path filters too. Previously a _require-login header/footer component rendered for anonymous visitors. #5 (robustness) — the flat-nav tree builder could recurse forever on a self-referencing parent_id or a node cycle, and duplicated node_ids spawned duplicate subtrees. buildFromFlat now dedupes by node_id (first wins), reparents a self-referencing node to the root, and carries an ancestor set to break cycles. #3 (robustness) — getEntriesByUids returned undefined on API error (no withTransferState fallback) despite an Observable type, so the component adapter threw on .flat()/.map() instead of falling back. It now emits []; the adapter also catchErrors each content-type group and filters non-arrays defensively. #1 (cleanup) — removed the phantom `section2` slot field from the landing_page content type (generator + regenerated JSON) and reconciled CONTENT-MODEL.md. LandingPage2Template has no bare Section2 render position. #2 (cleanup) — dropped `componentTypeMapping` from ContentstackConfig (and the README row): it was declared but read nowhere in src. Co-Authored-By: Claude Opus 4.8 --- CONTENT-MODEL.md | 4 +- README.md | 1 - .../content_types/landing_page.json | 23 -------- .../starter-pack/generate-content-types.mjs | 4 +- src/client/contentstack-client.service.ts | 22 ++++++-- ...contentstack-cms-component.adapter.spec.ts | 47 +++++++++++++++- .../contentstack-cms-component.adapter.ts | 19 +++++-- .../contentstack-cms-page.adapter.spec.ts | 54 +++++++++++++++++++ .../adapters/contentstack-cms-page.adapter.ts | 22 ++++++-- ...ms-navigation-component.normalizer.spec.ts | 47 ++++++++++++++++ ...ack-cms-navigation-component.normalizer.ts | 43 ++++++++++++--- src/config/contentstack-config.ts | 7 --- 12 files changed, 239 insertions(+), 54 deletions(-) diff --git a/CONTENT-MODEL.md b/CONTENT-MODEL.md index 0a91406..f4b095e 100644 --- a/CONTENT-MODEL.md +++ b/CONTENT-MODEL.md @@ -23,7 +23,7 @@ storefront's `LayoutConfig`. A slot only renders if its template declares it. Yo | SAP template (page type) | SAP slot positions | Contentstack field uid | |---|---|---| | **Shell** (header/footer, every page) | `SiteLogo`, `SearchBox`, `MiniCart`, `NavigationBar`, `SiteContext`, `SiteLinks`, `HeaderLinks`, `Footer` | `site_logo`, `search_box`, `mini_cart`, `navigation_bar`, `site_context`, `site_links`, `header_links`, `footer` | -| **LandingPage2Template** (home/landing) | `Section1`, `Section2`, `Section2A`, `Section2B`, `Section2C`, `Section3`, `Section4`, `Section5` | `section1`, `section2`, `section2_a`, `section2_b`, `section2_c`, `section3`, `section4`, `section5` | +| **LandingPage2Template** (home/landing) | `Section1`, `Section2A`, `Section2B`, `Section2C`, `Section3`, `Section4`, `Section5` | `section1`, `section2_a`, `section2_b`, `section2_c`, `section3`, `section4`, `section5` | | **ContentPage1Template** (FAQ, terms, …) | `Section1`, `Section2A/B/C`, `Section3`, `BodyContent`, `SideContent` | `section1`, `section2_a/b/c`, `section3`, `body_content`, `side_content` | | **ProductDetailsPageTemplate** (PDP) | `Summary`, `UpSelling`, `CrossSelling`, `Tabs`, `PlaceholderContentSlot` | `summary`, `up_selling`, `cross_selling`, `tabs`, `placeholder_content_slot` | | **ProductListPageTemplate** (PLP/category/search) | `ProductLeftRefinements`, `ProductGridSlot`, `ProductListSlot`, `SearchResultsGridSlot` | `product_left_refinements`, `product_grid_slot`, `product_list_slot`, `search_results_grid_slot` | @@ -49,7 +49,7 @@ Each **slot field is a multi-reference** to the component content types below. | Content type (uid) | Template | Slot fields (reference) | |---|---|---| -| `landing_page` | LandingPage2Template | `section1`, `section2`, `section2_a`, `section2_b`, `section2_c`, `section3`, `section4`, `section5` | +| `landing_page` | LandingPage2Template | `section1`, `section2_a`, `section2_b`, `section2_c`, `section3`, `section4`, `section5` | | `content_page` | ContentPage1Template | `section1`, `section2_a`, `section2_b`, `section2_c`, `section3`, `body_content`, `side_content` | | `product_page` | ProductDetailsPageTemplate | `summary`, `up_selling`, `cross_selling`, `tabs`, `placeholder_content_slot` | | `category_page` | ProductListPageTemplate | `product_left_refinements`, `product_grid_slot`, `product_list_slot`, `search_results_grid_slot` | diff --git a/README.md b/README.md index 829bab9..79ed357 100644 --- a/README.md +++ b/README.md @@ -171,7 +171,6 @@ block, with each field's JSDoc on hover. The complete set: | `pageTypeMapping` | per-`PageType` `{ contentTypeUid, slugField?, sharedSlug? }` (shared-layout pages) | | `additionalSlotFields` | extra `{ fieldUid: 'SapSlotPosition' }` beyond the built-in slot map | | `componentContentType` | content type for standalone component lookups (else components ship in pages) | -| `componentTypeMapping` | block uid → SAP typeCode (for author-named blocks without a `type_code`) | | `includeReferences` | reference fields to expand; defaults to all slot + header/footer fields | | `accessControl` | presentation-level gating — see below | | `timeoutMs` | `10000` — Delivery API call timeout | diff --git a/import-export/starter-pack/content_types/landing_page.json b/import-export/starter-pack/content_types/landing_page.json index 856fa09..4cfca3f 100644 --- a/import-export/starter-pack/content_types/landing_page.json +++ b/import-export/starter-pack/content_types/landing_page.json @@ -75,29 +75,6 @@ "instruction": "Maps to SAP slot Section1. Top hero band. Leave empty to keep SAP's." } }, - { - "display_name": "Section 2", - "uid": "section2", - "data_type": "reference", - "reference_to": [ - "simple_responsive_banner_component", - "simple_banner_component", - "product_carousel_component", - "cms_paragraph_component", - "cms_tab_paragraph_component", - "cms_link_component", - "cms_flex_component" - ], - "mandatory": false, - "multiple": true, - "unique": false, - "non_localizable": false, - "field_metadata": { - "ref_multiple": true, - "ref_multiple_content_types": true, - "instruction": "Maps to SAP slot Section2. Leave empty to keep SAP's." - } - }, { "display_name": "Section 2A", "uid": "section2_a", diff --git a/import-export/starter-pack/generate-content-types.mjs b/import-export/starter-pack/generate-content-types.mjs index 229c64e..cc04a09 100644 --- a/import-export/starter-pack/generate-content-types.mjs +++ b/import-export/starter-pack/generate-content-types.mjs @@ -178,7 +178,9 @@ const contentTypes = [ 'CMS page on SAP LandingPage2Template (home / marketing landing).', [...pageMeta('LandingPage2Template'), slot('section1', 'Section 1', 'Section1', EDITORIAL, 'Top hero band. '), - slot('section2', 'Section 2', 'Section2'), + // NB: LandingPage2Template has no bare `Section2` render position (that + // belongs to CategoryPageTemplate), so this page type intentionally omits a + // `section2` slot field — only Section2A/2B/2C exist here. slot('section2_a', 'Section 2A', 'Section2A'), slot('section2_b', 'Section 2B', 'Section2B'), slot('section2_c', 'Section 2C', 'Section2C'), diff --git a/src/client/contentstack-client.service.ts b/src/client/contentstack-client.service.ts index 3c3e0c7..98f8855 100644 --- a/src/client/contentstack-client.service.ts +++ b/src/client/contentstack-client.service.ts @@ -229,10 +229,13 @@ export class ContentstackClientService { title?: string, includeRefs: string[] = [], locale?: string, + access?: EntryAccessOptions, ): Observable { const csLocale = this.resolveLocale(locale); const key = makeStateKey( - `cs-global:${contentTypeUid}:${title ?? '*'}:${csLocale ?? '*'}`, + `cs-global:${contentTypeUid}:${title ?? '*'}:${csLocale ?? '*'}${this.restrictions.cacheKeySuffix( + access?.permissions, + )}`, ); return this.withTransferState(key, () => { // Resolve the shell's component references inline (same as the page path), @@ -252,7 +255,14 @@ export class ContentstackClientService { query.where('title', QueryOperation.EQUALS, title); } return query.find().then((res) => { - const entry = res?.entries?.[0]; + let entry = res?.entries?.[0]; + // Filter gated content (nested restricted shell components) BEFORE it is + // persisted to TransferState — the shell is gated the same as the page. + // gateRoot is false: the shell entry itself is never hidden, only its + // restricted nested components are dropped. + if (entry && access) { + entry = this.restrictions.sanitizeForTransfer(entry, access.permissions, false); + } if (entry && this.config.contentstack?.delivery?.livePreview) { this.tagForLivePreview(entry, contentTypeUid); } @@ -347,7 +357,13 @@ export class ContentstackClientService { ) : list; }); - }); + }, + // Failure fallback: an empty array, never `undefined` — the return type is + // `ContentstackEntry[]` and callers (component adapter) flat-map the result, + // so a failed batch must degrade to "no entries" (→ OCC/shell fallback), + // not to a value that throws on `.map`/`.filter`. + [], + ); } /** diff --git a/src/cms/adapters/contentstack-cms-component.adapter.spec.ts b/src/cms/adapters/contentstack-cms-component.adapter.spec.ts index e403e81..40c2310 100644 --- a/src/cms/adapters/contentstack-cms-component.adapter.spec.ts +++ b/src/cms/adapters/contentstack-cms-component.adapter.spec.ts @@ -1,4 +1,4 @@ -import { Observable, of } from 'rxjs'; +import { Observable, of, throwError } from 'rxjs'; import { ContentstackCmsComponentAdapter } from './contentstack-cms-component.adapter'; import { ContentstackRestrictionsService } from '../access/contentstack-restrictions.service'; import { ContentstackComponentTypeRegistry } from '../model/contentstack-component-type.registry'; @@ -414,4 +414,49 @@ describe('ContentstackCmsComponentAdapter', () => { expect(occComponentAdapter.findComponentsByIds).not.toHaveBeenCalled(); }); }); + + describe('batch-fetch error handling (#3)', () => { + const entry = (uid: string, type: string) => ({ + uid, + _content_type_uid: type, + created_at: '2026-01-01T00:00:00.000Z', + }); + + it('degrades a failed content-type group to empty instead of throwing, keeping healthy groups', () => { + // Two distinct learned types → two forkJoin branches; one fails outright. + const getEntriesByUids = jest.fn((type: string) => + type === 'type_b' + ? throwError(() => new Error('CMS down')) + : of([entry('bltaaaa', 'type_a')]), + ); + const { adapter } = create({ + registry: { bltaaaa: 'type_a', bltbbbb: 'type_b' }, + cs: { componentContentType: undefined }, // rely on learned types only + client: { getEntriesByUids }, + }); + + let res!: Array<{ uid: string }>; + expect(() => { + res = firstValue(adapter.findComponentsByIds(['bltaaaa', 'bltbbbb'], ctx)); + }).not.toThrow(); + const uids = res.map((c) => c.uid); + expect(uids).toContain('bltaaaa'); // healthy group resolved + expect(uids).toContain('bltbbbb'); // failed group degraded to a benign shell + }); + + it('tolerates a client that emits a non-array (undefined) without crashing', () => { + const getEntriesByUids = jest.fn().mockReturnValue(of(undefined as never)); + const { adapter } = create({ + registry: { bltaaaa: 'type_a' }, + cs: { componentContentType: undefined }, + client: { getEntriesByUids }, + }); + + let res!: Array<{ uid: string }>; + expect(() => { + res = firstValue(adapter.findComponentsByIds(['bltaaaa'], ctx)); + }).not.toThrow(); + expect(Array.isArray(res)).toBe(true); + }); + }); }); diff --git a/src/cms/adapters/contentstack-cms-component.adapter.ts b/src/cms/adapters/contentstack-cms-component.adapter.ts index 52e0d24..213b709 100644 --- a/src/cms/adapters/contentstack-cms-component.adapter.ts +++ b/src/cms/adapters/contentstack-cms-component.adapter.ts @@ -1,6 +1,6 @@ import { Inject, Injectable, Optional } from '@angular/core'; import { Observable, combineLatest, forkJoin, of } from 'rxjs'; -import { map, switchMap } from 'rxjs/operators'; +import { catchError, map, switchMap } from 'rxjs/operators'; import { CmsComponent, CmsComponentAdapter, @@ -168,13 +168,24 @@ export class ContentstackCmsComponentAdapter implements CmsComponentAdapter { switchMap(([locale, permissions]) => forkJoin( [...byType.entries()].map(([type, uids]) => - permissions + (permissions ? this.client.getEntriesByUids(type, uids, locale, { permissions, gateRoot: true }) - : this.client.getEntriesByUids(type, uids, locale), + : this.client.getEntriesByUids(type, uids, locale) + ).pipe( + // Defense in depth: one content-type group failing must not fail the + // whole batch (forkJoin errors if any source errors) — degrade that + // group to no entries so its ids fall back to OCC/shells below. + catchError(() => of([] as ContentstackEntry[])), + ), ), ).pipe( switchMap((groups: ContentstackEntry[][]) => { - const entries = groups.flat(); + // Guard against a malformed group (e.g. a client that ever emits a + // non-array) so `.flat()`/`.map()` can't throw on `undefined`. + const entries = groups + .filter((g): g is ContentstackEntry[] => Array.isArray(g)) + .flat() + .filter((e): e is ContentstackEntry => !!e); const accessible = permissions ? entries.filter((entry) => this.restrictions.isEntryAccessible(entry, permissions)) : entries; diff --git a/src/cms/adapters/contentstack-cms-page.adapter.spec.ts b/src/cms/adapters/contentstack-cms-page.adapter.spec.ts index d16a4fd..d44dc40 100644 --- a/src/cms/adapters/contentstack-cms-page.adapter.spec.ts +++ b/src/cms/adapters/contentstack-cms-page.adapter.spec.ts @@ -331,6 +331,60 @@ describe('ContentstackCmsPageAdapter', () => { expect(client.getGlobalSlots).not.toHaveBeenCalled(); }); + describe('gating the shell (#4)', () => { + const GATING = { + accessControl: { enabled: true, accessField: 'access_tags', rolePrefix: '_require-' }, + }; + + it('threads permissions + gateRoot:false into the shell fetch when gating is on', () => { + const getGlobalSlots = jest.fn().mockReturnValue(of(undefined)); + const { adapter } = create({ + cs: { ...GATING, globalSlots: { contentType: 'global_slots', title: 'Global' } }, + user: { roles: ['b2badmingroup'] }, + client: { getPageBySlug: jest.fn().mockReturnValue(of(undefined)), getGlobalSlots }, + }); + + firstValue(adapter.load(ctx('home'))); + const access = getGlobalSlots.mock.calls[0][4]; + expect(access?.permissions).toBeInstanceOf(Set); + expect(access?.permissions.has('_require-login')).toBe(true); + expect(access?.permissions.has('_require-b2badmingroup')).toBe(true); + // The shell root is never hidden — only restricted nested components drop. + expect(access?.gateRoot).toBe(false); + }); + + it('passes the permission set into buildStructure so restricted shell components drop on CSR', () => { + const buildStructure = jest.fn().mockReturnValue({ slots: {}, components: [] }); + const { adapter } = create({ + cs: { ...GATING, globalSlots: { contentType: 'global_slots' } }, + user: { roles: [] }, // logged in, no roles + client: { + getPageBySlug: jest.fn().mockReturnValue(of(undefined)), + getGlobalSlots: jest.fn().mockReturnValue(of({ uid: 'global-entry' })), + }, + normalizer: { buildStructure }, + occ: { load: jest.fn().mockReturnValue(of({ page: { template: 'T', slots: {} }, components: [] })) }, + }); + + firstValue(adapter.load(ctx('home'))); + const permissions = buildStructure.mock.calls[0][1] as Set; + expect(permissions).toBeInstanceOf(Set); + expect(permissions.has('_require-login')).toBe(true); + }); + + it('omits the access arg (unchanged behavior) when gating is off', () => { + const getGlobalSlots = jest.fn().mockReturnValue(of(undefined)); + const { adapter } = create({ + cs: { globalSlots: { contentType: 'global_slots' } }, + client: { getPageBySlug: jest.fn().mockReturnValue(of(undefined)), getGlobalSlots }, + }); + + firstValue(adapter.load(ctx('home'))); + // 4 args only — no 5th access argument when access control is off. + expect(getGlobalSlots.mock.calls[0][4]).toBeUndefined(); + }); + }); + it('always requests the flat-nav include chain for every nav field, independent of depth', () => { const { adapter, client } = create({ cs: { globalSlots: { contentType: 'global_slots' } }, diff --git a/src/cms/adapters/contentstack-cms-page.adapter.ts b/src/cms/adapters/contentstack-cms-page.adapter.ts index 44cf301..fd9827d 100644 --- a/src/cms/adapters/contentstack-cms-page.adapter.ts +++ b/src/cms/adapters/contentstack-cms-page.adapter.ts @@ -130,7 +130,14 @@ export class ContentstackCmsPageAdapter implements CmsPageAdapter { }) : this.client.getPageBySlug(contentType, slugField, slug, includeRefs, locale); const global$ = global - ? this.client.getGlobalSlots(global.contentType, global.title, globalIncludeRefs, locale) + ? permissions + ? this.client.getGlobalSlots(global.contentType, global.title, globalIncludeRefs, locale, { + permissions, + // The shell root is never hidden; only restricted nested shell + // components (e.g. a login-only header link) are filtered out. + gateRoot: false, + }) + : this.client.getGlobalSlots(global.contentType, global.title, globalIncludeRefs, locale) : of(undefined); // Hybrid base: the SAP page for this route. A CMS failure must never // break navigation, so degrade to no-base on error. @@ -154,7 +161,9 @@ export class ContentstackCmsPageAdapter implements CmsPageAdapter { } const csStructure = entry ? this.normalizer.convert(entry, {}, permissions) : undefined; - const globalStructure = globalEntry ? this.toGlobalStructure(globalEntry) : undefined; + const globalStructure = globalEntry + ? this.toGlobalStructure(globalEntry, permissions) + : undefined; // Nothing from Contentstack and no OCC base → not-found (as before). if (!csStructure && !occBase) { @@ -175,8 +184,13 @@ export class ContentstackCmsPageAdapter implements CmsPageAdapter { * base and the page: an authored shell overrides OCC's, and unauthored shell * slots fall through to OCC. */ - protected toGlobalStructure(globalEntry: ContentstackCmsPageEntry): CmsStructureModel { - const { slots, components } = this.normalizer.buildStructure(globalEntry); + protected toGlobalStructure( + globalEntry: ContentstackCmsPageEntry, + permissions?: Set, + ): CmsStructureModel { + // Thread permissions so restricted shell components are dropped on the CSR + // path too (sanitizeForTransfer only guards the SSR TransferState payload). + const { slots, components } = this.normalizer.buildStructure(globalEntry, permissions); return { page: { slots }, components }; } diff --git a/src/cms/converters/components/contentstack-cms-navigation-component.normalizer.spec.ts b/src/cms/converters/components/contentstack-cms-navigation-component.normalizer.spec.ts index a12d3dc..7c7332a 100644 --- a/src/cms/converters/components/contentstack-cms-navigation-component.normalizer.spec.ts +++ b/src/cms/converters/components/contentstack-cms-navigation-component.normalizer.spec.ts @@ -133,4 +133,51 @@ describe('ContentstackCmsNavigationComponentNormalizer', () => { const component = normalizer.convert(flatComponent([withStub])); expect(component.navigationNode?.children?.[0].entries).toBeUndefined(); }); + + describe('malformed-data guards (no infinite recursion)', () => { + it('treats a self-referencing node (parent_id === node_id) as top-level', () => { + const build = () => normalizer.convert(flatComponent([node('Solo', 'Solo', 'Solo', 1)])); + // Must not recurse forever: complete and surface the node once, at the top. + const component = build(); + expect(component.navigationNode?.children?.map((n) => n.uid)).toEqual(['Solo']); + expect(component.navigationNode?.children?.[0].children).toBeUndefined(); + }); + + it('does not hang on a parent/child cycle (A→B→A); a pure cycle yields no roots', () => { + const component = normalizer.convert( + flatComponent([node('A', 'A', 'B', 1), node('B', 'B', 'A', 1)]), + ); + // Neither node is top-level (each names the other as parent), so the menu is + // empty — the point is that it terminates instead of overflowing the stack. + expect(component.navigationNode?.children).toEqual([]); + }); + + it('deduplicates a repeated node_id (first wins) rather than duplicating a subtree', () => { + const component = normalizer.convert( + flatComponent([ + node('Dup', 'First', '', 1, 'blt_first_link'), + node('Dup', 'Second', '', 2, 'blt_second_link'), + node('Child', 'Child', 'Dup', 1), + ]), + ); + const tops = component.navigationNode?.children ?? []; + expect(tops.map((n) => n.uid)).toEqual(['Dup']); // only one 'Dup', not two + expect(tops[0].title).toBe('First'); // first occurrence wins + expect(tops[0].children?.map((n) => n.uid)).toEqual(['Child']); + }); + + it('guards a deeper cycle reachable from a real root without hanging', () => { + // Root → Mid, and a stray pair (X→Y→X) that must not be walked into forever. + const component = normalizer.convert( + flatComponent([ + node('Root', 'Root', '', 1), + node('Mid', 'Mid', 'Root', 1), + node('X', 'X', 'Y', 1), + node('Y', 'Y', 'X', 1), + ]), + ); + expect(component.navigationNode?.children?.map((n) => n.uid)).toEqual(['Root']); + expect(component.navigationNode?.children?.[0].children?.map((n) => n.uid)).toEqual(['Mid']); + }); + }); }); diff --git a/src/cms/converters/components/contentstack-cms-navigation-component.normalizer.ts b/src/cms/converters/components/contentstack-cms-navigation-component.normalizer.ts index c24d4a5..689be11 100644 --- a/src/cms/converters/components/contentstack-cms-navigation-component.normalizer.ts +++ b/src/cms/converters/components/contentstack-cms-navigation-component.normalizer.ts @@ -47,9 +47,26 @@ export class ContentstackCmsNavigationComponentNormalizer implements Converter< * top-level nodes, matching the single-root shape Spartacus expects. */ private buildFromFlat(nodes: ContentstackEntry[], rootUid: string): CmsNavigationNode { - const byParent = new Map(); + // Guard against malformed authoring data: a duplicated `node_id`, a node that + // names itself as its own parent, or a parent/child cycle. Any of these would + // otherwise spawn duplicate subtrees or recurse forever (stack overflow). + + // 1) Dedupe by node_id (first wins), so a duplicated id can't produce two + // conflicting nodes or an ambiguous parent lookup. + const byId = new Map(); for (const n of nodes) { - const key = this.parentKey(n); + const id = this.nodeId(n); + if (!byId.has(id)) { + byId.set(id, n); + } + } + + // 2) Group by parent. A node whose parent_id equals its own node_id is + // self-referencing — treat it as top-level rather than let it parent + // itself (which would recurse infinitely). + const byParent = new Map(); + for (const n of byId.values()) { + const key = this.parentKey(n) === this.nodeId(n) ? '' : this.parentKey(n); const siblings = byParent.get(key) ?? []; siblings.push(n); byParent.set(key, siblings); @@ -58,23 +75,33 @@ export class ContentstackCmsNavigationComponentNormalizer implements Converter< siblings.sort((a, b) => this.sortOrder(a) - this.sortOrder(b)); } - const build = (parentKey: string): CmsNavigationNode[] => + // 3) Build recursively, carrying the set of ancestor ids on the current path. + // A node whose id is already an ancestor closes a cycle — stop descending + // (a fully cyclic pool simply yields no top-level nodes, i.e. an empty menu). + const build = (parentKey: string, ancestors: Set): CmsNavigationNode[] => (byParent.get(parentKey) ?? []).map((n) => { - const nodeId = (n['node_id'] as string) ?? n.uid; + const nodeId = this.nodeId(n); const node: CmsNavigationNode = { uid: nodeId, title: n['title'] as string }; const links = this.resolvedList(n['links']); if (links.length) { node.entries = links.map((linkEntry) => this.toNavigationEntry(linkEntry)); } - const children = build(nodeId); - if (children.length) { - node.children = children; + if (!ancestors.has(nodeId)) { + const children = build(nodeId, new Set(ancestors).add(nodeId)); + if (children.length) { + node.children = children; + } } return node; }); - return { uid: rootUid, children: build('') }; + return { uid: rootUid, children: build('', new Set()) }; + } + + /** A node's identity: its `node_id`, falling back to the entry uid. */ + private nodeId(node: ContentstackEntry): string { + return (node['node_id'] as string) ?? node.uid; } /** The parent grouping key: a node's `parent_id`, normalized to '' for top level. */ diff --git a/src/config/contentstack-config.ts b/src/config/contentstack-config.ts index 8422623..7b24623 100644 --- a/src/config/contentstack-config.ts +++ b/src/config/contentstack-config.ts @@ -208,13 +208,6 @@ export abstract class ContentstackConfig { */ pageTypeMapping?: Partial>; - /** - * Optional map of Contentstack block uid → Spartacus typeCode, consulted by - * the page normalizer when a block has no explicit `type_code` field. Lets an - * app map author-named blocks to stock component types without editing content. - */ - componentTypeMapping?: Record; - /** * Shared/global slots (header, footer, navigation, logo, …) authored once and * merged into every page. When set, the page adapter fetches this entry and From 9c75a4cf11529d5f9f61d241e4b267de32d2758e Mon Sep 17 00:00:00 2001 From: vaishnavikedar1511 Date: Tue, 15 Sep 2026 11:37:39 +0530 Subject: [PATCH 2/5] fix: harden review follow-ups after self-review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - nav (#5): skip descending on an empty node_id so a blank-id node can't adopt every root node as its children (the root sentinel is ''). Adds a regression test. - access (#4): correct misleading comments — sanitizeForTransfer runs on every fetch (SSR write and client re-fetch), not SSR-only; the render-time buildStructure filter mirrors the page path as defense in depth. No behavior change. 166 unit + 8 schematics tests pass; typecheck clean. Co-Authored-By: Claude Opus 4.8 --- src/client/contentstack-client.service.ts | 8 ++++---- src/cms/adapters/contentstack-cms-page.adapter.ts | 6 ++++-- ...ntstack-cms-navigation-component.normalizer.spec.ts | 10 ++++++++++ ...contentstack-cms-navigation-component.normalizer.ts | 5 ++++- 4 files changed, 22 insertions(+), 7 deletions(-) diff --git a/src/client/contentstack-client.service.ts b/src/client/contentstack-client.service.ts index 98f8855..00a177e 100644 --- a/src/client/contentstack-client.service.ts +++ b/src/client/contentstack-client.service.ts @@ -256,10 +256,10 @@ export class ContentstackClientService { } return query.find().then((res) => { let entry = res?.entries?.[0]; - // Filter gated content (nested restricted shell components) BEFORE it is - // persisted to TransferState — the shell is gated the same as the page. - // gateRoot is false: the shell entry itself is never hidden, only its - // restricted nested components are dropped. + // Filter gated content on every fetch (before the SSR TransferState write, + // and again on any client-side re-fetch) — the shell is gated like the + // page. gateRoot is false: the shell entry itself is never hidden, only + // its restricted nested components are stripped. if (entry && access) { entry = this.restrictions.sanitizeForTransfer(entry, access.permissions, false); } diff --git a/src/cms/adapters/contentstack-cms-page.adapter.ts b/src/cms/adapters/contentstack-cms-page.adapter.ts index fd9827d..d4245ef 100644 --- a/src/cms/adapters/contentstack-cms-page.adapter.ts +++ b/src/cms/adapters/contentstack-cms-page.adapter.ts @@ -188,8 +188,10 @@ export class ContentstackCmsPageAdapter implements CmsPageAdapter { globalEntry: ContentstackCmsPageEntry, permissions?: Set, ): CmsStructureModel { - // Thread permissions so restricted shell components are dropped on the CSR - // path too (sanitizeForTransfer only guards the SSR TransferState payload). + // Filter the shell by the viewer's permissions, mirroring the page path + // (normalizer.convert(entry, {}, permissions)). This is the render-time gate; + // the client's sanitizeForTransfer independently strips restricted nested + // entries from every fetch (SSR + CSR) before they reach here — belt and braces. const { slots, components } = this.normalizer.buildStructure(globalEntry, permissions); return { page: { slots }, components }; } diff --git a/src/cms/converters/components/contentstack-cms-navigation-component.normalizer.spec.ts b/src/cms/converters/components/contentstack-cms-navigation-component.normalizer.spec.ts index 7c7332a..95c5cf3 100644 --- a/src/cms/converters/components/contentstack-cms-navigation-component.normalizer.spec.ts +++ b/src/cms/converters/components/contentstack-cms-navigation-component.normalizer.spec.ts @@ -166,6 +166,16 @@ describe('ContentstackCmsNavigationComponentNormalizer', () => { expect(tops[0].children?.map((n) => n.uid)).toEqual(['Child']); }); + it('does not let an empty node_id adopt every root node as its children', () => { + const component = normalizer.convert( + flatComponent([node('', 'Empty', '', 1), node('Real', 'Real', '', 2)]), + ); + const tops = component.navigationNode?.children ?? []; + // Both are top-level; the empty-id node must NOT pull the other roots under it. + expect(tops.map((n) => n.uid)).toEqual(['', 'Real']); + expect(tops.find((n) => n.uid === '')?.children).toBeUndefined(); + }); + it('guards a deeper cycle reachable from a real root without hanging', () => { // Root → Mid, and a stray pair (X→Y→X) that must not be walked into forever. const component = normalizer.convert( diff --git a/src/cms/converters/components/contentstack-cms-navigation-component.normalizer.ts b/src/cms/converters/components/contentstack-cms-navigation-component.normalizer.ts index 689be11..570b54d 100644 --- a/src/cms/converters/components/contentstack-cms-navigation-component.normalizer.ts +++ b/src/cms/converters/components/contentstack-cms-navigation-component.normalizer.ts @@ -87,7 +87,10 @@ export class ContentstackCmsNavigationComponentNormalizer implements Converter< if (links.length) { node.entries = links.map((linkEntry) => this.toNavigationEntry(linkEntry)); } - if (!ancestors.has(nodeId)) { + // Only descend for a real, non-empty id not already on the path. An empty + // id can't be a parent key distinct from the root sentinel (''), so an + // empty-id node must not recurse into build('') and adopt every root node. + if (nodeId && !ancestors.has(nodeId)) { const children = build(nodeId, new Set(ancestors).add(nodeId)); if (children.length) { node.children = children; From fa413e2cf963d2ba60f58c9a7bd64fa6405970eb Mon Sep 17 00:00:00 2001 From: vaishnavikedar1511 Date: Tue, 15 Sep 2026 11:44:28 +0530 Subject: [PATCH 3/5] test: use T[] array syntax to satisfy @typescript-eslint/array-type Fixes the two lint errors failing CI's Build & verify (Array<{uid}> -> {uid}[]). Co-Authored-By: Claude Opus 4.8 --- src/cms/adapters/contentstack-cms-component.adapter.spec.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/cms/adapters/contentstack-cms-component.adapter.spec.ts b/src/cms/adapters/contentstack-cms-component.adapter.spec.ts index 40c2310..5105534 100644 --- a/src/cms/adapters/contentstack-cms-component.adapter.spec.ts +++ b/src/cms/adapters/contentstack-cms-component.adapter.spec.ts @@ -435,7 +435,7 @@ describe('ContentstackCmsComponentAdapter', () => { client: { getEntriesByUids }, }); - let res!: Array<{ uid: string }>; + let res!: { uid: string }[]; expect(() => { res = firstValue(adapter.findComponentsByIds(['bltaaaa', 'bltbbbb'], ctx)); }).not.toThrow(); @@ -452,7 +452,7 @@ describe('ContentstackCmsComponentAdapter', () => { client: { getEntriesByUids }, }); - let res!: Array<{ uid: string }>; + let res!: { uid: string }[]; expect(() => { res = firstValue(adapter.findComponentsByIds(['bltaaaa'], ctx)); }).not.toThrow(); From ba18296d4154f8c0e8c67ddd1b1f84ea49585745 Mon Sep 17 00:00:00 2001 From: vaishnavikedar1511 Date: Tue, 15 Sep 2026 11:48:00 +0530 Subject: [PATCH 4/5] style: apply prettier formatting to changed files Satisfies CI's format:check (prettier --check) for the three files touched by the review follow-ups. Co-Authored-By: Claude Opus 4.8 --- src/client/contentstack-client.service.ts | 46 ++++++++++--------- .../contentstack-cms-page.adapter.spec.ts | 6 ++- .../adapters/contentstack-cms-page.adapter.ts | 25 +++++++--- 3 files changed, 47 insertions(+), 30 deletions(-) diff --git a/src/client/contentstack-client.service.ts b/src/client/contentstack-client.service.ts index 00a177e..83d6056 100644 --- a/src/client/contentstack-client.service.ts +++ b/src/client/contentstack-client.service.ts @@ -334,29 +334,31 @@ export class ContentstackClientService { access?.permissions, )}`, ); - return this.withTransferState(key, () => { - let entries = this.stack.contentType(contentTypeUid).entry(); - if (csLocale) { - entries = entries.locale(csLocale); - if (this.includeFallback) { - entries = entries.includeFallback(); + return this.withTransferState( + key, + () => { + let entries = this.stack.contentType(contentTypeUid).entry(); + if (csLocale) { + entries = entries.locale(csLocale); + if (this.includeFallback) { + entries = entries.includeFallback(); + } } - } - return entries - .query() - .where('uid', QueryOperation.INCLUDES, uids) - .find() - .then((res) => { - const list = res?.entries ?? []; - // Filter gated content BEFORE it is persisted to TransferState: - // each restricted entry is redacted to a tags-only stub so the adapter - // still counts it as "found" (no OCC refetch) without shipping content. - return access - ? list.map((entry) => - this.restrictions.sanitizeForTransfer(entry, access.permissions, access.gateRoot), - ) - : list; - }); + return entries + .query() + .where('uid', QueryOperation.INCLUDES, uids) + .find() + .then((res) => { + const list = res?.entries ?? []; + // Filter gated content BEFORE it is persisted to TransferState: + // each restricted entry is redacted to a tags-only stub so the adapter + // still counts it as "found" (no OCC refetch) without shipping content. + return access + ? list.map((entry) => + this.restrictions.sanitizeForTransfer(entry, access.permissions, access.gateRoot), + ) + : list; + }); }, // Failure fallback: an empty array, never `undefined` — the return type is // `ContentstackEntry[]` and callers (component adapter) flat-map the result, diff --git a/src/cms/adapters/contentstack-cms-page.adapter.spec.ts b/src/cms/adapters/contentstack-cms-page.adapter.spec.ts index d44dc40..3ae20fb 100644 --- a/src/cms/adapters/contentstack-cms-page.adapter.spec.ts +++ b/src/cms/adapters/contentstack-cms-page.adapter.spec.ts @@ -363,7 +363,11 @@ describe('ContentstackCmsPageAdapter', () => { getGlobalSlots: jest.fn().mockReturnValue(of({ uid: 'global-entry' })), }, normalizer: { buildStructure }, - occ: { load: jest.fn().mockReturnValue(of({ page: { template: 'T', slots: {} }, components: [] })) }, + occ: { + load: jest + .fn() + .mockReturnValue(of({ page: { template: 'T', slots: {} }, components: [] })), + }, }); firstValue(adapter.load(ctx('home'))); diff --git a/src/cms/adapters/contentstack-cms-page.adapter.ts b/src/cms/adapters/contentstack-cms-page.adapter.ts index d4245ef..16494bc 100644 --- a/src/cms/adapters/contentstack-cms-page.adapter.ts +++ b/src/cms/adapters/contentstack-cms-page.adapter.ts @@ -131,13 +131,24 @@ export class ContentstackCmsPageAdapter implements CmsPageAdapter { : this.client.getPageBySlug(contentType, slugField, slug, includeRefs, locale); const global$ = global ? permissions - ? this.client.getGlobalSlots(global.contentType, global.title, globalIncludeRefs, locale, { - permissions, - // The shell root is never hidden; only restricted nested shell - // components (e.g. a login-only header link) are filtered out. - gateRoot: false, - }) - : this.client.getGlobalSlots(global.contentType, global.title, globalIncludeRefs, locale) + ? this.client.getGlobalSlots( + global.contentType, + global.title, + globalIncludeRefs, + locale, + { + permissions, + // The shell root is never hidden; only restricted nested shell + // components (e.g. a login-only header link) are filtered out. + gateRoot: false, + }, + ) + : this.client.getGlobalSlots( + global.contentType, + global.title, + globalIncludeRefs, + locale, + ) : of(undefined); // Hybrid base: the SAP page for this route. A CMS failure must never // break navigation, so degrade to no-base on error. From 4f487ac3a2adb79c58d5264d018868845a792ada Mon Sep 17 00:00:00 2001 From: vaishnavikedar1511 Date: Tue, 15 Sep 2026 12:02:34 +0530 Subject: [PATCH 5/5] =?UTF-8?q?fix:=20address=20Copilot=20review=20?= =?UTF-8?q?=E2=80=94=20cache-key=20injectivity=20+=20nav=20simplification?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - access (critical): cacheKeySuffix now percent-encodes each permission token before joining on '|', so the suffix is injective — a token containing the delimiter can no longer collide two distinct permission sets onto the same shell cache key (which would serve one audience another's filtered payload). Adds a collision-resistance test. - nav (moderate): drop the per-node ancestor Set (its copy-per-node was O(depth^2) on deep menus, and the branch was unreachable). Dedup by identity + reparenting self-references + one-parent-per-node already make the root-reachable nodes a forest, so recursion is provably linear and terminating; orphaned parent cycles are never entered. nodeId() now falls a blank/non-string node_id back to the (unique) entry uid so it can't collide with the root sentinel. Tests updated to match. 167 unit + 8 schematics tests pass; lint, format, typecheck, build all green. Co-Authored-By: Claude Opus 4.8 --- .../contentstack-restrictions.service.spec.ts | 8 +++ .../contentstack-restrictions.service.ts | 7 ++- ...ms-navigation-component.normalizer.spec.ts | 14 +++--- ...ack-cms-navigation-component.normalizer.ts | 50 ++++++++++--------- 4 files changed, 49 insertions(+), 30 deletions(-) diff --git a/src/cms/access/contentstack-restrictions.service.spec.ts b/src/cms/access/contentstack-restrictions.service.spec.ts index 415d959..a5d4bfa 100644 --- a/src/cms/access/contentstack-restrictions.service.spec.ts +++ b/src/cms/access/contentstack-restrictions.service.spec.ts @@ -110,6 +110,14 @@ describe('ContentstackRestrictionsService', () => { // Different audiences get different keys (no cross-serving). expect(svc.cacheKeySuffix(new Set(['_require-login']))).not.toBe(a); }); + + it('is injective even when a token contains the join delimiter', () => { + // Without per-token encoding, {'a|b'} and {'a','b'} would collide on `|`, + // letting one audience receive another's permission-filtered cache entry. + const single = svc.cacheKeySuffix(new Set(['a|b'])); + const pair = svc.cacheKeySuffix(new Set(['a', 'b'])); + expect(single).not.toBe(pair); + }); }); describe('redactEntry()', () => { diff --git a/src/cms/access/contentstack-restrictions.service.ts b/src/cms/access/contentstack-restrictions.service.ts index d67d60e..d546a75 100644 --- a/src/cms/access/contentstack-restrictions.service.ts +++ b/src/cms/access/contentstack-restrictions.service.ts @@ -117,7 +117,12 @@ export class ContentstackRestrictionsService { if (!permissions || !permissions.size) { return ''; } - return `:acl=${[...permissions].sort().join('|')}`; + // Encode each token before joining so the `|` delimiter can never collide: + // `encodeURIComponent` percent-escapes any literal `|` (→ `%7C`), so the + // suffix is injective — `{'a|b'}` and `{'a','b'}` map to distinct keys, and a + // token that happens to contain the delimiter can't leak another audience's + // cached, permission-filtered payload. + return `:acl=${[...permissions].sort().map(encodeURIComponent).join('|')}`; } /** Whether a value is a Contentstack entry node (an object carrying a string uid). */ diff --git a/src/cms/converters/components/contentstack-cms-navigation-component.normalizer.spec.ts b/src/cms/converters/components/contentstack-cms-navigation-component.normalizer.spec.ts index 95c5cf3..4ba351a 100644 --- a/src/cms/converters/components/contentstack-cms-navigation-component.normalizer.spec.ts +++ b/src/cms/converters/components/contentstack-cms-navigation-component.normalizer.spec.ts @@ -166,18 +166,20 @@ describe('ContentstackCmsNavigationComponentNormalizer', () => { expect(tops[0].children?.map((n) => n.uid)).toEqual(['Child']); }); - it('does not let an empty node_id adopt every root node as its children', () => { + it('falls a blank node_id back to the entry uid instead of colliding with the root', () => { const component = normalizer.convert( flatComponent([node('', 'Empty', '', 1), node('Real', 'Real', '', 2)]), ); const tops = component.navigationNode?.children ?? []; - // Both are top-level; the empty-id node must NOT pull the other roots under it. - expect(tops.map((n) => n.uid)).toEqual(['', 'Real']); - expect(tops.find((n) => n.uid === '')?.children).toBeUndefined(); + // The blank-id node takes its uid ('blt_') as identity, so it stays a normal + // top-level node and does NOT adopt the other root nodes as children. + expect(tops.map((n) => n.uid)).toEqual(['blt_', 'Real']); + expect(tops.find((n) => n.uid === 'blt_')?.children).toBeUndefined(); }); - it('guards a deeper cycle reachable from a real root without hanging', () => { - // Root → Mid, and a stray pair (X→Y→X) that must not be walked into forever. + it('ignores a disconnected parent cycle while still building the reachable tree', () => { + // Root → Mid is a real tree; the stray pair (X→Y→X) never reaches the root, + // so it is dropped rather than walked into forever. const component = normalizer.convert( flatComponent([ node('Root', 'Root', '', 1), diff --git a/src/cms/converters/components/contentstack-cms-navigation-component.normalizer.ts b/src/cms/converters/components/contentstack-cms-navigation-component.normalizer.ts index 570b54d..7f3b9a7 100644 --- a/src/cms/converters/components/contentstack-cms-navigation-component.normalizer.ts +++ b/src/cms/converters/components/contentstack-cms-navigation-component.normalizer.ts @@ -47,12 +47,19 @@ export class ContentstackCmsNavigationComponentNormalizer implements Converter< * top-level nodes, matching the single-root shape Spartacus expects. */ private buildFromFlat(nodes: ContentstackEntry[], rootUid: string): CmsNavigationNode { - // Guard against malformed authoring data: a duplicated `node_id`, a node that - // names itself as its own parent, or a parent/child cycle. Any of these would - // otherwise spawn duplicate subtrees or recurse forever (stack overflow). + // Guard against malformed authoring data (duplicate ids, self-references, + // parent cycles) with three cheap invariants that make the recursion below + // provably terminating WITHOUT a per-node visited set: + // 1) unique identities — dedupe by node_id (falling back to the always- + // unique entry uid for a blank/non-string node_id); + // 2) no self-parenting — a node naming itself is reparented to the root; + // 3) a single parent per node (the data model gives each node one parent_id). + // Together these mean the root-reachable nodes form a forest: each node has + // exactly one path back to the root, so build() visits every node at most + // once. A parent cycle that never reaches the root is simply never entered + // (build starts at '') — so it terminates and drops the orphaned cycle. - // 1) Dedupe by node_id (first wins), so a duplicated id can't produce two - // conflicting nodes or an ambiguous parent lookup. + // 1) Dedupe by identity (first wins). const byId = new Map(); for (const n of nodes) { const id = this.nodeId(n); @@ -61,9 +68,7 @@ export class ContentstackCmsNavigationComponentNormalizer implements Converter< } } - // 2) Group by parent. A node whose parent_id equals its own node_id is - // self-referencing — treat it as top-level rather than let it parent - // itself (which would recurse infinitely). + // 2) Group by parent; reparent a self-referencing node to the root. const byParent = new Map(); for (const n of byId.values()) { const key = this.parentKey(n) === this.nodeId(n) ? '' : this.parentKey(n); @@ -75,10 +80,8 @@ export class ContentstackCmsNavigationComponentNormalizer implements Converter< siblings.sort((a, b) => this.sortOrder(a) - this.sortOrder(b)); } - // 3) Build recursively, carrying the set of ancestor ids on the current path. - // A node whose id is already an ancestor closes a cycle — stop descending - // (a fully cyclic pool simply yields no top-level nodes, i.e. an empty menu). - const build = (parentKey: string, ancestors: Set): CmsNavigationNode[] => + // 3) Build the tree top-down. Linear in the number of nodes (see invariants). + const build = (parentKey: string): CmsNavigationNode[] => (byParent.get(parentKey) ?? []).map((n) => { const nodeId = this.nodeId(n); const node: CmsNavigationNode = { uid: nodeId, title: n['title'] as string }; @@ -87,24 +90,25 @@ export class ContentstackCmsNavigationComponentNormalizer implements Converter< if (links.length) { node.entries = links.map((linkEntry) => this.toNavigationEntry(linkEntry)); } - // Only descend for a real, non-empty id not already on the path. An empty - // id can't be a parent key distinct from the root sentinel (''), so an - // empty-id node must not recurse into build('') and adopt every root node. - if (nodeId && !ancestors.has(nodeId)) { - const children = build(nodeId, new Set(ancestors).add(nodeId)); - if (children.length) { - node.children = children; - } + const children = build(nodeId); + if (children.length) { + node.children = children; } return node; }); - return { uid: rootUid, children: build('', new Set()) }; + return { uid: rootUid, children: build('') }; } - /** A node's identity: its `node_id`, falling back to the entry uid. */ + /** + * A node's stable identity: its `node_id` when a non-empty string, otherwise + * the entry uid. Using the uid for a blank/non-string node_id keeps every + * identity unique and non-empty, so it can never collide with the root + * sentinel ('') or another node. + */ private nodeId(node: ContentstackEntry): string { - return (node['node_id'] as string) ?? node.uid; + const id = node['node_id']; + return typeof id === 'string' && id.length ? id : node.uid; } /** The parent grouping key: a node's `parent_id`, normalized to '' for top level. */