Skip to content

@W-22476773@ Storefront Preview / HttpOnly - foundations for dynamic SameSite#3850

Merged
vcua-mobify merged 14 commits into
developfrom
vc/http-only-storefront-preview
May 27, 2026
Merged

@W-22476773@ Storefront Preview / HttpOnly - foundations for dynamic SameSite#3850
vcua-mobify merged 14 commits into
developfrom
vc/http-only-storefront-preview

Conversation

@vcua-mobify
Copy link
Copy Markdown
Contributor

@vcua-mobify vcua-mobify commented May 22, 2026

Summary

Foundations for a 2-part fix that lets the SLAS proxy issue session cookies as SameSite=None; Secure; Partitioned (CHIPS) when the storefront is loaded inside a trusted Storefront Preview iframe, while keeping SameSite=Lax for top-level traffic.

This PR is plumbing only — no behavior change for any existing flow. The marker cookie is written under qualifying conditions but is not yet read on SLAS responses, so all session cookies still ship SameSite=Lax. PR 2/2 adds the read path and flips the attribute.

The second part of this change is #3851

  • Adds Partitioned (CHIPS) attribute support to cookieAsString.
  • Adds two new constants: STOREFRONT_PREVIEW_CTX_COOKIE (the marker name) and STOREFRONT_PREVIEW_PARENT_ALLOW_LIST (server mirror of IFRAME_HOST_ALLOW_LIST from commerce-sdk-react).
  • Extracts getValidatedCookieDomain + INVALID_COOKIE_DOMAIN_PATTERN into cookie-domain.js so both process-token-response.js and the new preview-context.js can share the validator without circular imports.
  • Adds preview-context.js exporting tryWriteStorefrontPreviewMarker(req, res, options) and readStorefrontPreviewMarker(req).
  • Mounts the marker-writer middleware in _setupCommonMiddleware, gated on MRT_ENABLE_HTTPONLY_SESSION_COOKIES === 'true'.

Why

With MRT_ENABLE_HTTPONLY_SESSION_COOKIES=true, the BFF currently writes cc-at, cc-nx-g, cc-nx, idp_access_token, idp_refresh_token (and the indicator cookies) with SameSite=Lax hardcoded in httponly-cookie-config.js. Storefront Preview iframes the storefront under a cross-site Runtime Admin parent (e.g. runtime-admin-preview.mobify-storefront.com). On the iframe's subresource fetches the browser strips Lax cookies, so SLAS refresh fails, every SCAPI proxy call returns 400 {"message":"access_token_cookie_missing"}, getUsidForPreview times out, and Runtime Admin reports "Failed to receive a response to 'requestUsid' within the 10000ms timeout." The same storefront URL opened in a top-level tab works fine — the bug is specific to the cross-site iframe boundary.

The fix is to relax SameSite only when the BFF can prove the storefront is inside a trusted preview iframe. We do this in two steps, entirely on the server:

  1. Trust capture (this PR) — on the iframe's initial top-level navigation the browser sends Sec-Fetch-Dest: iframe, Sec-Fetch-Site: cross-site, and Referer: https://<parent>/... (preserved by Runtime Admin's referrerpolicy="strict-origin-when-cross-origin"). None of these are settable from cross-origin JS, so an attacker page cannot trigger the gate. When all three line up and the Referer origin is on the allow-list, we set a server-only marker cookie (__pwakit_preview_ctx=<parent>; Path=/; Secure; HttpOnly; SameSite=None; Partitioned).
  2. Trust use (PR 2/2)setHttpOnlySessionCookies reads the marker, re-validates the value, and switches per-request to SameSite=None; Partitioned.

CHIPS partitions the marker by the legitimate parent's top-level site, so even if the cookie value was somehow influenced (it isn't — it's set under server-attested conditions only), partition isolation bounds the blast radius. CHIPS is unsupported in Firefox/Safari today; those browsers ignore Partitioned and the marker degrades gracefully to plain SameSite=None; Secure.

MRT impact

None. Verified — the fix uses only browser-set request metadata (Sec-Fetch-*, Referer) and a new Set-Cookie response header. Both already traverse MRT/CloudFront unchanged for Set-Cookies emitted by the SSR app, and Sec-Fetch-*/Referer are standard request headers that aren't filtered. No WAF allow-list change required (no new request header introduced).

Changes

pwa-kit-runtime

Tests

  • packages/pwa-kit-runtime/src/utils/ssr-proxying.test.js — two new cookieAsString cases asserting Partitioned emit on/off.
  • New packages/pwa-kit-runtime/src/ssr/server/preview-context.test.js — 14 cases:
    • Writer: emits the marker on the happy path; test.each over every entry in STOREFRONT_PREVIEW_PARENT_ALLOW_LIST; negative cases for non-GET, wrong Sec-Fetch-Dest, wrong Sec-Fetch-Site, untrusted Referer origin, missing Referer, unparseable Referer, Sec-Fetch-* absent (older browser — fail closed); Domain= is emitted when cookieDomain is set and omitted when malformed.
    • Reader: returns undefined when absent, returns the validated origin on a happy match, returns undefined for a non-allow-listed value (re-validation), returns undefined when the cookie name isn't present.
  • packages/pwa-kit-runtime/src/ssr/server/build-remote-server.test.js — two cases inside the existing _setupCommonMiddleware describe block:
    • MRT_ENABLE_HTTPONLY_SESSION_COOKIES=true → marker middleware is registered immediately after prepNonProxyRequest, and forwarding it an iframe-shaped request appends a Set-Cookie: __pwakit_preview_ctx=....
    • env var unset → middleware is not registered (mockApp.use called exactly 3 times).

Behavioral notes

  • Top-level traffic, regular iframe parents, all browsers without MRT_ENABLE_HTTPONLY_SESSION_COOKIES set: completely unchanged. The marker is gated on the env var.
  • No session-cookie attribute change yet. This PR only writes the marker cookie; PR 2/2 reads it and flips SameSite. You can deploy this PR alone and verify in DevTools that __pwakit_preview_ctx shows up on iframe document loads from the trusted parents — without affecting the existing broken-iframe behavior.
  • Fail-closed on missing browser headers. When Sec-Fetch-* is absent (very old browsers), the marker is not written and traffic stays on the existing Lax path.
  • Marker cookie lifetime — session cookie (no Expires/Max-Age). Re-issued on every qualifying request; clears when the preview window closes.

Test plan

  • npm test in packages/pwa-kit-runtime669/669 passing.
  • npm run lint in packages/pwa-kit-runtime0 errors (7 pre-existing warnings in files this PR doesn't touch).
  • Local smoke (trusted parent) — run a storefront with MRT_ENABLE_HTTPONLY_SESSION_COOKIES=true, load it inside https://runtime-admin-preview.mobify-storefront.com, and confirm in DevTools →
    Application → Cookies that __pwakit_preview_ctx appears with SameSite=None, Partitioned, Secure, HttpOnly, Path=/, value = parent origin.
  • Negative — top-level — open the storefront URL directly in a new tab. Confirm __pwakit_preview_ctx is not set.
  • Negative — untrusted parent — iframe the storefront under https://example.com. Confirm __pwakit_preview_ctx is not set.
  • Negative — env var off — with MRT_ENABLE_HTTPONLY_SESSION_COOKIES unset, repeat the trusted-iframe load. Confirm __pwakit_preview_ctx is not set.
  • Existing iframe behavior unchanged — confirm SLAS still 400s with access_token_cookie_missing (this PR does not yet fix the iframe; that's PR 2/2).
  • Existing top-level behavior unchanged — log in on a top-level tab, confirm session cookies still ship SameSite=Lax.
  • cookieDomain interop — set commerceAPI.cookieDomain, repeat the trusted-iframe load, confirm __pwakit_preview_ctx includes Domain=....

@git2gus
Copy link
Copy Markdown

git2gus Bot commented May 22, 2026

Git2Gus App is installed but the .git2gus/config.json doesn't have right values. You should add the required configuration.

@cc-prodsec
Copy link
Copy Markdown
Collaborator

cc-prodsec commented May 22, 2026

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

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

@vcua-mobify vcua-mobify marked this pull request as ready for review May 22, 2026 20:53
@vcua-mobify vcua-mobify requested a review from a team as a code owner May 22, 2026 20:53
Signed-off-by: vcua-mobify <47404250+vcua-mobify@users.noreply.github.com>
* SPDX-License-Identifier: BSD-3-Clause
* For full license text, see the LICENSE file in the repo root or https://opensource.org/licenses/BSD-3-Clause
*/
import cookie from 'cookie'
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: We are importing the cookie package but we didn't add it as a dependecy.

Should we import the dependency or replace it with an inline parser e.g. req.headers.cookie?.split('; ') ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm now using an inline parser

function detectTrustedPreviewParent(req) {
if (req.method !== 'GET') return undefined
if (req.headers?.[SEC_FETCH_DEST] !== 'iframe') return undefined
if (req.headers?.[SEC_FETCH_SITE] !== 'cross-site') return undefined
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We may want to widening to also accept same-site for scenarios when we are testing Preview with non production domains and both RA and the storefront end up sharing the same domain e.g. *.mobify-storefront.com, in those scenarios the browser sends same-site, not cross-site.

Suggested change
if (req.headers?.[SEC_FETCH_SITE] !== 'cross-site') return undefined
const fetchSite = req.headers?.[SEC_FETCH_SITE]
if (fetchSite !== 'cross-site' && fetchSite !== 'same-site') return undefined

// secure context. We always set Secure on this cookie, so the prefix adds
// defense in depth at zero cost. (`__Host-` won't work because the cookie
// can carry Domain= when commerceAPI.cookieDomain is configured.)
export const STOREFRONT_PREVIEW_CTX_COOKIE = '__Secure-pwakit_preview_ctx'
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we use __Host- here instead of __Secure-? it only needs to be readable on the host that wrote it

per MDN docs on CHIPS pattern:
https://developer.mozilla.org/en-US/docs/Web/Privacy/Guides/Privacy_sandbox/Partitioned_cookies#:~:text=you%20can%20use%20the%20__Host%20prefix%20when%20setting%20partitioned%20cookies%20to%20bind%20them%20only%20to%20the%20current%20domain%20or%20subdomain%2C%20and%20this%20is%20recommended%20if%20you%20don%27t%20need%20to%20share%20cookies%20between%20subdomains
__Host- is browser-enforced to require Path=/ and forbid Domain,
which lets you drop the cookieDomain && {domain} conditional

@vcua-mobify vcua-mobify requested a review from adamraya May 25, 2026 22:58
adamraya
adamraya previously approved these changes May 26, 2026
unandyala
unandyala previously approved these changes May 26, 2026
// Mirrors IFRAME_HOST_ALLOW_LIST in commerce-sdk-react/src/constant.ts.
// Kept in sync by a parity test in preview-context.test.js — drift will
// fail the test.
export const STOREFRONT_PREVIEW_PARENT_ALLOW_LIST = Object.freeze([
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we include soak environment as well?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, I've added both the soak and test environments just now

@vcua-mobify vcua-mobify dismissed stale reviews from unandyala and adamraya via 580b9e0 May 27, 2026 00:15
@vcua-mobify vcua-mobify enabled auto-merge (squash) May 27, 2026 16:30
@vcua-mobify vcua-mobify merged commit 0cdb15b into develop May 27, 2026
40 of 42 checks passed
@vcua-mobify vcua-mobify deleted the vc/http-only-storefront-preview branch May 27, 2026 16:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants