fix: secure SSR cache key generation against untrusted headers by l3tchupkt · Pull Request #21383 · SAP/spartacus

l3tchupkt · 2026-04-20T13:57:15Z

This PR addresses a security issue in the Spartacus SSR engine where the cache key could be influenced by untrusted request headers (notably X-Forwarded-Host). This could allow cache poisoning and cache fragmentation, which may contribute to SSR resource exhaustion.

Key Changes

Secure Default
The SSR cache key now uses only req.originalUrl by default, ignoring all host-related headers. This removes host-based attack surface unless explicitly enabled.
Configurable Host-Based Keys
Added useHostInCacheKey and allowedHosts options to SsrOptimizationOptions to support multi-domain setups in a controlled way.
Robust Host Validation (getSafeHost)
- Normalization: lowercasing, port stripping, trailing dot removal
- Validation: strict hostname pattern enforcement (labels start/end with alphanumeric characters)
- Allowlist enforcement: only explicitly allowed hosts are accepted
- Subdomain support: allows valid subdomains of allowlisted domains while preventing bypasses
- Length limits: enforces a 255-character maximum
- Header sanitization: safely handles multi-value headers (e.g. a.com,b.com)
Safe Fallback
If no trusted host is identified, the system falls back to a host-independent cache key (originalUrl), avoiding use of untrusted input.

Security Impact

This change mitigates:

SSR cache poisoning via host header manipulation
Cache fragmentation caused by attacker-controlled host values
Cache key manipulation through proxy headers

Verification

Expanded test suite with security-focused coverage:
- Prevention of cache fragmentation from attacker-controlled host values
- Normalization bypass checks (case, ports, trailing dots)
- Strict allowlist enforcement for both X-Forwarded-Host and Host headers
- Rejection of malformed or invalid hostname inputs
Updated brittle initialization snapshots in the existing test suite

- Use path-only cache key by default to prevent poisoning via X-Forwarded-Host. - Add SsrOptimizationOptions: useHostInCacheKey and allowedHosts. - Introduce getSafeHost utility with RFC-compliant validation and normalization. - Support safe subdomain matching and enforce hostname length limits. - Update tests to verify protection against cache fragmentation and hijacking.

Copilot

Pull request overview

Hardens SSR cache key generation to reduce cache poisoning / fragmentation risk by moving away from untrusted host-related headers, while introducing an allowlisted opt-in for host-based keys.

Changes:

Added useHostInCacheKey and allowedHosts options for controlled host-based cache keys.
Updated OptimizedSsrEngine.getRenderingKey() to prefer a host-independent key by default, with optional allowlisted host prefixing.
Added getSafeHost() utility and expanded SSR engine tests around cache key behavior.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.

File	Description
core-libs/setup/ssr/optimized-engine/ssr-optimization-options.ts	Adds new SSR options for host-based cache keys and default `useHostInCacheKey: false`.
core-libs/setup/ssr/optimized-engine/optimized-ssr-engine.ts	Updates cache key generation logic and integrates `getSafeHost()`.
core-libs/setup/ssr/optimized-engine/optimized-ssr-engine.spec.ts	Updates logging assertions and adds security-focused cache key tests.
core-libs/setup/ssr/express-utils/express-request-safe-host.ts	Introduces allowlist-based host extraction/validation helper.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-20T15:01:17Z

+      expect(consoleLogSpy.mock.lastCall[0]).toContain(
+        '[spartacus] SSR optimization engine initialized'
+      );


These assertions now only check the first console.log argument, but OptimizedSsrEngine.logOptions() also logs a context object with the resolved options. Keeping an assertion on the second argument shape (e.g. that it contains an options object) would prevent regressions in initialization logging behavior.

Suggested change

expect(consoleLogSpy.mock.lastCall[0]).toContain(

'[spartacus] SSR optimization engine initialized'

);

expect(consoleLogSpy.mock.lastCall).toEqual([

expect.stringContaining(

'[spartacus] SSR optimization engine initialized'

),

expect.objectContaining({

options: expect.any(Object),

}),

]);

- Fix unreachable secure logic in getRenderingKey by removing default resolver. - Correct JSDocs for host-based cache keys and getSafeHost fallback behavior. - Fix case-sensitivity in TestEngineRunner request mock. - Resolve type errors in default optimization options.

cla-assistant · 2026-04-20T15:19:42Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

cla-assistant · 2026-04-20T15:28:51Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

cla-assistant · 2026-04-20T15:29:12Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

Platonn · 2026-04-22T07:59:47Z

+    if (this.ssrOptions?.renderKeyResolver) {
+      return this.ssrOptions.renderKeyResolver(request);
+    }
+
+    // SECURITY: Do not use X-Forwarded-Host directly as it is user-controlled.
+    if (this.ssrOptions?.useHostInCacheKey) {
+      const host = getSafeHost(request, this.ssrOptions.allowedHosts);
+      if (host) {
+        return `${host}:${request.originalUrl}`;
+      }
+    }
+
+    // Secure default: ignore host entirely
+    return request.originalUrl;


Thank you @l3tchupkt for your feedback and for providing the PR. Much appreciated!

Do you really think it should be a responsibility of Spartacus to filter X-Forwarded-Host headers? What other alternatives did you considered and what pros,cons,risks of them can you see. In particular, did you consider filtering X-Forwarded-Host it in CCV2 reverse-proxy? AFAIK it's already a responsibility of CCV2 reverse proxy to set X-Forwarded-Host by default. Only in case of using CDN, CCV2 should trust the X-Forwarded-Host set by the CDN.

This PR introduces a breaking change new behavior: the origin is no longer a part of the cache key (i.e. ssrOptions.useHostInCacheKey is false by default). It's a breaking change behavior for existing multi-site storefronts, which handle traffic from multiple domains. Is it justified to change the default behavior?

If we'd stay with the old behavior (keeping useHostInCacheKey: true by default, the next breaking change is requiring from storefront owners to hardcode all their supported domains in allowedHosts in the source code (config) of Spartacus. For storefronts having multiple domains (multi-site), and multiple environments (development,staging,production) it would mean hardcoding them all (for all envs) in the built code, right? Or I'm missing something. Do you think it's an optimal approach? What alternative options can you see, their pros,cons,risks?

PS. I'm just curious: what is your main usecase in your projects: multi-site (multi-domain) or single domain? And are you using CCV2 hosting or a custom one?

l3tchupkt · 2026-04-22T08:10:54Z

Hi Krzysztof, Thanks for the detailed feedback—I really appreciate the perspective. My main intention with this PR was to address the underlying vulnerability regarding cache poisoning via untrusted host headers. The proposed change is one possible way to mitigate this risk while maintaining flexibility for multi-site setups. I am not strongly opinionated on enforcing this specific approach. My goal was to surface the issue and provide a fix that is secure by default. If there is a preferred direction, such as relying on CCV2/proxy guarantees or adjusting the default behavior, I am happy to adapt the implementation accordingly. I look forward to your thoughts on the best way to proceed. Best regards, letchupkt

l3tchupkt requested a review from a team as a code owner April 20, 2026 13:57

Copilot AI review requested due to automatic review settings April 20, 2026 13:57

Copilot started reviewing on behalf of l3tchupkt April 20, 2026 13:57 View session

Copilot AI reviewed Apr 20, 2026

View reviewed changes

l3tchupkt force-pushed the fix/secure-ssr-cache-key branch from aecb077 to 0bc1d3c Compare April 20, 2026 14:08

l3tchupkt requested a review from Copilot April 20, 2026 14:38

Copilot started reviewing on behalf of l3tchupkt April 20, 2026 14:39 View session

Copilot AI reviewed Apr 20, 2026

View reviewed changes

Comment thread core-libs/setup/ssr/optimized-engine/optimized-ssr-engine.spec.ts

Comment thread core-libs/setup/ssr/optimized-engine/ssr-optimization-options.ts

Comment thread core-libs/setup/ssr/express-utils/express-request-safe-host.ts Outdated

l3tchupkt force-pushed the fix/secure-ssr-cache-key branch from 0bc1d3c to 4921c08 Compare April 20, 2026 14:54

l3tchupkt requested a review from Copilot April 20, 2026 14:55

Copilot started reviewing on behalf of l3tchupkt April 20, 2026 14:55 View session

Copilot AI reviewed Apr 20, 2026

View reviewed changes

l3tchupkt force-pushed the fix/secure-ssr-cache-key branch from 4921c08 to cd81d57 Compare April 20, 2026 15:07

l3tchupkt force-pushed the fix/secure-ssr-cache-key branch from cd81d57 to ac3599e Compare April 20, 2026 15:11

Platonn reviewed Apr 22, 2026

View reviewed changes

-      expect(consoleLogSpy.mock.lastCall[0]).toContain(
-        '[spartacus] SSR optimization engine initialized'
-      );
+      expect(consoleLogSpy.mock.lastCall).toEqual([
+        expect.stringContaining(
+          '[spartacus] SSR optimization engine initialized'
+        ),
+        expect.objectContaining({
+          options: expect.any(Object),
+        }),
+      ]);

Conversation

l3tchupkt commented Apr 20, 2026

Key Changes

Security Impact

Verification

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Apr 20, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cla-assistant Bot commented Apr 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cla-assistant Bot commented Apr 20, 2026

Uh oh!

cla-assistant Bot commented Apr 20, 2026

Uh oh!

Platonn Apr 22, 2026

Choose a reason for hiding this comment

Uh oh!

l3tchupkt commented Apr 22, 2026 via email

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

cla-assistant Bot commented Apr 20, 2026 •

edited

Loading