fix: connector review follow-ups (shell gating, nav cycles, batch errors, cleanups)

[vaishnavi-kedar-cstk]

Addresses the code issues surfaced during the documentation/code review. No new features; correctness + robustness only. Unit tests added for the three runtime fixes — 165 unit + 8 schematics tests pass, typecheck clean.

Fixes

#4 — Shell content skipped access filtering (security)

Header/footer/nav (the shared shell) bypassed the permission filtering applied to page content, so a _require-login component could render for anonymous visitors.

• getGlobalSlots now accepts EntryAccessOptions: scopes the SSR cache key per permission set and sanitizes restricted nested shell components before the TransferState write.
• 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.

#5 — Navigation cycle / duplicate guard (robustness)

The flat-nav tree builder could recurse forever on a self-referencing parent_id or a node cycle, and duplicate 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 — Batch-fetch error handling (robustness)

getEntriesByUids emitted undefined on API error (no withTransferState fallback) despite its Observable<ContentstackEntry[]> type, so the component adapter threw on .flat()/.map() instead of degrading.

• It now emits []; the adapter also catchErrors each content-type group and filters non-arrays defensively.

#1 — Phantom section2 slot (cleanup)

Removed the section2 field from the landing_page content type (generator + regenerated JSON) and reconciled CONTENT-MODEL.md. LandingPage2Template has no bare Section2 render position.

#2 — Drop componentTypeMapping (cleanup)

Removed from ContentstackConfig (and the README row): it was declared but read nowhere in src/.

Tests

• contentstack-cms-page.adapter.spec.ts — shell gating: permissions + gateRoot:false into the fetch, permissions into buildStructure, unchanged 4-arg call when gating off.
• contentstack-cms-component.adapter.spec.ts — a failed content-type group degrades to a shell without throwing; tolerates a non-array emission.
• contentstack-cms-navigation-component.normalizer.spec.ts — self-parent, 2-node cycle, duplicate id, deeper reachable cycle all terminate correctly.

Deferred

Integration coverage against real Spartacus (navigation / language / login / CMS-outage) is intentionally not in this PR — tracked separately (needs RBSC access).

🤖 Generated with Claude Code

[snyk-io]

✅ Snyk checks have passed. No issues have been found so far.

Status	Scan Engine	Critical	High	Medium	Low	Total (0)
✅	Open Source Security	0	0	0	0	0 issues
✅	Licenses	0	0	0	0	0 issues
✅	Code Security	0	0	0	0	0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

[github-actions]

🔒 Security Scan Results

ℹ️ Note: Only vulnerabilities with available fixes (upgrades or patches) are counted toward thresholds.

Check Type	Count (with fixes)	Without fixes	Threshold	Result
🔴 Critical Severity	0	0	10	✅ Passed
🟠 High Severity	0	0	25	✅ Passed
🟡 Medium Severity	0	2	500	✅ Passed
🔵 Low Severity	0	0	1000	✅ Passed

⏱️ SLA Breach Summary

✅ No SLA breaches detected. All vulnerabilities are within acceptable time thresholds.

Severity	Breaches (with fixes)	Breaches (no fixes)	SLA Threshold (with/no fixes)	Status
🔴 Critical	0	0	15 / 30 days	✅ Passed
🟠 High	0	0	30 / 120 days	✅ Passed
🟡 Medium	0	0	90 / 365 days	✅ Passed
🔵 Low	0	0	180 / 365 days	✅ Passed

ℹ️ Vulnerabilities Without Available Fixes (Informational Only)

The following vulnerabilities were detected but do not have fixes available (no upgrade or patch). These are excluded from failure thresholds:

• Critical without fixes: 0
• High without fixes: 0
• Medium without fixes: 2
• Low without fixes: 0

✅ BUILD PASSED - All security checks passed

[Copilot]

🟡 Changes recommended

A critical shell-cache collision and unresolved moderate navigation and API-compatibility issues block approval.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

This PR hardens Contentstack shell access, navigation normalization, and batch-fetch resilience while removing obsolete schema and configuration fields.

Changes:

• Adds permission-aware shell fetching, caching, and filtering.
• Guards navigation cycles and batch-fetch failures.
• Removes componentTypeMapping and the phantom section2 slot.

File summaries

File	Review summary
src/config/contentstack-config.ts	Removes the unused mapping. Moderate (1 vote): this is a source-breaking API change.
src/cms/converters/components/contentstack-cms-navigation-component.normalizer.ts	Adds deduplication and cycle guards. Moderate (1 vote): ancestor-set copying can be quadratic; empty or non-string IDs can create self-child behavior.
src/cms/converters/components/contentstack-cms-navigation-component.normalizer.spec.ts	Adds cycle fixtures. Nit (1 vote): the disconnected cycle does not exercise the ancestor-set guard.
src/cms/adapters/contentstack-cms-page.adapter.ts	Threads permissions through shell loading and normalization.
src/cms/adapters/contentstack-cms-page.adapter.spec.ts	Tests shell access propagation.
src/cms/adapters/contentstack-cms-component.adapter.ts	Adds resilient batch-fetch fallbacks.
src/cms/adapters/contentstack-cms-component.adapter.spec.ts	Tests batch failure and malformed emissions.
src/client/contentstack-client.service.ts	Scopes shell cache entries and sanitizes restricted content. Critical (2 votes): delimiter-joined permission keys can collide across permission sets.
README.md	Removes obsolete configuration documentation.
import-export/starter-pack/generate-content-types.mjs	Stops generating section2.
import-export/starter-pack/content_types/landing_page.json	Removes the phantom schema field.
CONTENT-MODEL.md	Reconciles documented landing-page slots.

Review details

Suppressed comments (4)

src/cms/converters/components/contentstack-cms-navigation-component.normalizer.spec.ts:173

• This fixture does not exercise the new ancestor-set guard: X and Y form a disconnected component, so build('', ...) never visits them. The test would also pass with the pre-change recursion and therefore cannot catch a regression in the cycle-breaking branch; either add a fixture that actually reaches that branch or simplify the implementation/test to reflect the constraints of this flat parent model.

    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),

src/cms/converters/components/contentstack-cms-navigation-component.normalizer.ts:94

• Copying the entire ancestors set for every node makes a deep flat navigation chain quadratic in both set-copy work and live memory, while this normalizer explicitly supports arbitrary depth. Mutate/backtrack the single path set (or use another path-local marker) instead so building a chain remains linear.

        if (!ancestors.has(nodeId)) {
          const children = build(nodeId, new Set(ancestors).add(nodeId));
          if (children.length) {
            node.children = children;
          }

src/cms/converters/components/contentstack-cms-navigation-component.normalizer.ts:104

• nodeId returns an empty or non-string node_id verbatim. For malformed data with node_id: '' and an empty/missing parent, parentKey is also '', so build('') re-enters the same bucket and emits the node again as its own child instead of using the UID fallback. Validate that node_id is a non-empty string before using it as the identity (or discard the malformed node).

  private nodeId(node: ContentstackEntry): string {
    return (node['node_id'] as string) ?? node.uid;

src/config/contentstack-config.ts:211

• ContentstackConfig is an exported, fully typed configuration API and this field was also documented in the README. Removing componentTypeMapping is therefore a source-breaking change for consumers that set it, even if the implementation currently ignores it; retain the optional property as deprecated until a major release, or explicitly treat this as a breaking API change and document the migration.

    pageTypeMapping?: Partial<Record<PageType, ContentstackPageTypeMapping>>;

    /**

• Files reviewed: 12/12 changed files
• Comments generated: 1
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[github-actions]

🔒 Security Scan Results

ℹ️ Note: Only vulnerabilities with available fixes (upgrades or patches) are counted toward thresholds.

Check Type	Count (with fixes)	Without fixes	Threshold	Result
🔴 Critical Severity	0	0	10	✅ Passed
🟠 High Severity	0	0	25	✅ Passed
🟡 Medium Severity	0	2	500	✅ Passed
🔵 Low Severity	0	0	1000	✅ Passed

⏱️ SLA Breach Summary

✅ No SLA breaches detected. All vulnerabilities are within acceptable time thresholds.

Severity	Breaches (with fixes)	Breaches (no fixes)	SLA Threshold (with/no fixes)	Status
🔴 Critical	0	0	15 / 30 days	✅ Passed
🟠 High	0	0	30 / 120 days	✅ Passed
🟡 Medium	0	0	90 / 365 days	✅ Passed
🔵 Low	0	0	180 / 365 days	✅ Passed

ℹ️ Vulnerabilities Without Available Fixes (Informational Only)

The following vulnerabilities were detected but do not have fixes available (no upgrade or patch). These are excluded from failure thresholds:

• Critical without fixes: 0
• High without fixes: 0
• Medium without fixes: 2
• Low without fixes: 0

✅ BUILD PASSED - All security checks passed

[Copilot]

🟡 Changes recommended

The critical removal of an exported configuration property is source-breaking, and the reachable-cycle test fixture does not exercise the claimed guard.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (1)

src/cms/converters/components/contentstack-cms-navigation-component.normalizer.spec.ts:187

• This fixture does not exercise the claimed reachable-cycle guard: X and Y form a disconnected X↔Y component, while build('') only visits Root and Mid. The pre-change implementation would pass this test too, so the new ancestor-set behavior remains untested; please rework the test/fixture or remove the misleading reachability claim.

          node('Root', 'Root', '', 1),
          node('Mid', 'Mid', 'Root', 1),
          node('X', 'X', 'Y', 1),
          node('Y', 'Y', 'X', 1),
        ]),

• Files reviewed: 12/12 changed files
• Comments generated: 1
• Review effort level: Lite

[github-actions]

🔒 Security Scan Results

ℹ️ Note: Only vulnerabilities with available fixes (upgrades or patches) are counted toward thresholds.

Check Type	Count (with fixes)	Without fixes	Threshold	Result
🔴 Critical Severity	0	0	10	✅ Passed
🟠 High Severity	0	0	25	✅ Passed
🟡 Medium Severity	0	2	500	✅ Passed
🔵 Low Severity	0	0	1000	✅ Passed

⏱️ SLA Breach Summary

✅ No SLA breaches detected. All vulnerabilities are within acceptable time thresholds.

Severity	Breaches (with fixes)	Breaches (no fixes)	SLA Threshold (with/no fixes)	Status
🔴 Critical	0	0	15 / 30 days	✅ Passed
🟠 High	0	0	30 / 120 days	✅ Passed
🟡 Medium	0	0	90 / 365 days	✅ Passed
🔵 Low	0	0	180 / 365 days	✅ Passed

ℹ️ Vulnerabilities Without Available Fixes (Informational Only)

The following vulnerabilities were detected but do not have fixes available (no upgrade or patch). These are excluded from failure thresholds:

• Critical without fixes: 0
• High without fixes: 0
• Medium without fixes: 2
• Low without fixes: 0

✅ BUILD PASSED - All security checks passed

[github-actions]

🔒 Security Scan Results

ℹ️ Note: Only vulnerabilities with available fixes (upgrades or patches) are counted toward thresholds.

Check Type	Count (with fixes)	Without fixes	Threshold	Result
🔴 Critical Severity	0	0	10	✅ Passed
🟠 High Severity	0	0	25	✅ Passed
🟡 Medium Severity	0	2	500	✅ Passed
🔵 Low Severity	0	0	1000	✅ Passed

⏱️ SLA Breach Summary

✅ No SLA breaches detected. All vulnerabilities are within acceptable time thresholds.

Severity	Breaches (with fixes)	Breaches (no fixes)	SLA Threshold (with/no fixes)	Status
🔴 Critical	0	0	15 / 30 days	✅ Passed
🟠 High	0	0	30 / 120 days	✅ Passed
🟡 Medium	0	0	90 / 365 days	✅ Passed
🔵 Low	0	0	180 / 365 days	✅ Passed

ℹ️ Vulnerabilities Without Available Fixes (Informational Only)

The following vulnerabilities were detected but do not have fixes available (no upgrade or patch). These are excluded from failure thresholds:

• Critical without fixes: 0
• High without fixes: 0
• Medium without fixes: 2
• Low without fixes: 0

✅ BUILD PASSED - All security checks passed

[Copilot]

🔵 Needs a closer look

Two moderate issues and one nit remain unresolved, including public API compatibility and incomplete cycle-test coverage.

Review details

Suppressed comments (3)

Previously missed (1) — in code that hasn't changed since the last review.

src/cms/converters/components/contentstack-cms-navigation-component.normalizer.spec.ts:152

• These cycle tests do not exercise the new ancestor-set guard: with A.parent_id = B and B.parent_id = A, neither node is reachable from the synthetic build('') root, so the pre-change implementation also returns [] without recursing; the X↔Y pair in the later test is disconnected for the same reason. Please add a fixture that actually reaches the ancestors.has(nodeId) path, or adjust the test/description so it does not claim coverage of cycle termination.

import-export/starter-pack/generate-content-types.mjs:183

• The note says bare Section2 belongs to CategoryPageTemplate, but the shipped category_page schema has no such slot; it defines only ProductLeftRefinements, ProductGridSlot, ProductListSlot, and SearchResultsGridSlot. This comment should describe Section2 as simply absent rather than documenting a nonexistent mapping.

     // 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.

src/config/contentstack-config.ts:211

• ContentstackConfig is exported through public-api.ts and supplied via Spartacus provideConfig, so removing this public optional field is source-breaking for existing typed app configurations even though the connector never consumed it. Keep it as a deprecated no-op for a compatibility window, or make the removal an explicitly versioned breaking change.

    pageTypeMapping?: Partial<Record<PageType, ContentstackPageTypeMapping>>;

    /**

• Files reviewed: 12/12 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

Resolve the transfer-state key collision, add security-path regression coverage, and preserve compatibility for the removed configuration field.

Review details

Suppressed comments (3)

src/client/contentstack-client.service.ts:238

• This new shell TransferState key relies on cacheKeySuffix, which serializes permission tokens with a raw | join. That encoding is not injective: { '_require-login', '_require-a|_require-b' } and { '_require-login', '_require-a', '_require-b' } produce the same suffix, so a sanitized shell for one audience can be replayed to the other. Encode/length-prefix each token (or hash a canonical JSON representation) and add a collision regression test.

      `cs-global:${contentTypeUid}:${title ?? '*'}:${csLocale ?? '*'}${this.restrictions.cacheKeySuffix(
        access?.permissions,
      )}`,

src/client/contentstack-client.service.ts:264

• The added adapter tests mock getGlobalSlots, so they verify only argument threading and never exercise this security path that sanitizes the shell before TransferState persistence. Add a client-service test that asserts a restricted nested shell entry is removed (and that permission sets produce distinct cache keys), otherwise the SSR leak fix can regress while the current tests remain green.

        if (entry && access) {
          entry = this.restrictions.sanitizeForTransfer(entry, access.permissions, false);

src/config/contentstack-config.ts:210

• Because ContentstackConfig is exported through src/public-api.ts and augments Spartacus's Config, removing this documented optional field is a source-breaking change for consumers that still include it in provideConfig objects, even though the field is currently unused. Keep it as a deprecated no-op (or defer removal to a major release) so existing configurations continue to type-check.

• Files reviewed: 12/12 changed files
• Comments generated: 0 new
• Review effort level: Lite

[github-actions]

🔒 Security Scan Results

ℹ️ Note: Only vulnerabilities with available fixes (upgrades or patches) are counted toward thresholds.

Check Type	Count (with fixes)	Without fixes	Threshold	Result
🔴 Critical Severity	0	0	10	✅ Passed
🟠 High Severity	0	0	25	✅ Passed
🟡 Medium Severity	0	2	500	✅ Passed
🔵 Low Severity	0	0	1000	✅ Passed

⏱️ SLA Breach Summary

✅ No SLA breaches detected. All vulnerabilities are within acceptable time thresholds.

Severity	Breaches (with fixes)	Breaches (no fixes)	SLA Threshold (with/no fixes)	Status
🔴 Critical	0	0	15 / 30 days	✅ Passed
🟠 High	0	0	30 / 120 days	✅ Passed
🟡 Medium	0	0	90 / 365 days	✅ Passed
🔵 Low	0	0	180 / 365 days	✅ Passed

ℹ️ Vulnerabilities Without Available Fixes (Informational Only)

The following vulnerabilities were detected but do not have fixes available (no upgrade or patch). These are excluded from failure thresholds:

• Critical without fixes: 0
• High without fixes: 0
• Medium without fixes: 2
• Low without fixes: 0

✅ BUILD PASSED - All security checks passed

[vaishnavi-kedar-cstk]

Thanks for the review — addressed all findings in 4f487ac.

🔴 Critical — shell cache-key collision (client service): cacheKeySuffix now percent-encodes each permission token before joining on |, so the suffix is injective and a token containing the delimiter can't map two different permission sets onto the same key. Added a collision test.

🟠 Moderate — nav: quadratic ancestor-set copy + unreachable branch (navigation normalizer): you're right on both counts, so I removed the ancestor Set entirely rather than optimize it. Deduping by identity + reparenting self-references + the one-parent-per-node data model already make the root-reachable nodes a forest, so build() visits each node at most once (linear, no per-node set copy), and an orphaned parent cycle is simply never entered because traversal starts at the root. This also resolves the nit that the disconnected-cycle fixture didn't exercise the old guard — with the guard gone, those fixtures now correctly assert that orphaned cycles are dropped and the reachable tree still builds. Comments document the forest invariant.

🟠 Moderate — empty/non-string node ids (navigation normalizer): nodeId() now falls a blank or non-string node_id back to the (always-unique, non-empty) entry uid, so an id can never collide with the root sentinel ('') or another node.

🟠 Moderate — removing componentTypeMapping is source-breaking (config): intentional — it was declared but read nowhere in src/. Replied inline with the semver rationale (pre-1.0) and offered a @deprecated no-op alternative if preferred.

All green locally: lint, format:check, typecheck, test:all (167 unit + 8 schematics), build.

[Copilot]

🔵 Needs a closer look

Cross-cutting security and runtime changes with deferred integration coverage require final human review.

Review details

• Files reviewed: 14/14 changed files
• Comments generated: 0 new
• Review effort level: Lite