Skip to content

W-21432256: Few follow up fixes to address Basket V2-V1 gaps and content security policy headers#3728

Merged
amittapalli merged 10 commits intodevelopfrom
amittapalli/sfp-followup-fixes
Mar 6, 2026
Merged

W-21432256: Few follow up fixes to address Basket V2-V1 gaps and content security policy headers#3728
amittapalli merged 10 commits intodevelopfrom
amittapalli/sfp-followup-fixes

Conversation

@amittapalli
Copy link
Contributor

W-21432256: Few follow up fixes to address Basket V2-V1 gaps and content security policy headers

Description

  • Update the code to use v2 cache key for basket v2
  • Fix comments in the code to indicate v2
  • Fix content security policy headers for google pay related sources

Types of Changes

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Documentation update
  • Breaking change (could cause existing functionality to not work as expected)
  • Other changes (non-breaking changes that does not fit any of the above)

Breaking changes include:

  • Removing a public function or component or prop
  • Adding a required argument to a function
  • Changing the data type of a function parameter or return value
  • Adding a new peer dependency to package.json

Changes

  • (change1)

How to Test-Drive This PR

  • (step1)

Checklists

General

  • Changes are covered by test cases
  • CHANGELOG.md updated with a short description of changes (not required for documentation updates)

Accessibility Compliance

You must check off all items in one of the follow two lists:

  • There are no changes to UI

or...

Localization

  • Changes include a UI text update in the Retail React App (which requires translation)

@amittapalli amittapalli requested a review from a team as a code owner March 6, 2026 15:02
@cc-prodsec
Copy link
Collaborator

cc-prodsec commented Mar 6, 2026

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

Status Scanner 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.

jeffraab-sfdc
jeffraab-sfdc previously approved these changes Mar 6, 2026
'google.com/pay',
'google.com/pay/',
'www.google.com/pay',
'www.google.com/pay/'

Choose a reason for hiding this comment

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

I guess same question. Do we need www. and google.com? Just want to confirm.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added them based on what I saw in the browser for CSP errors. It would help if someone else can run the checks during our testing phase or locally. We initially had *.google.com which is not safe but google pay seems to be depending on several url patterns

'google.com',
'www.google.com'
'google.com/pay',
'google.com/pay/',

Choose a reason for hiding this comment

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

are these both needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added them based on what I saw in the browser for CSP errors

Copy link
Contributor

@vmarta vmarta Mar 6, 2026

Choose a reason for hiding this comment

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

When specifying a path for the CSP header, a trailing slash is significant. It means like matching the any requests that start with this path.

https://developer.mozilla.org/en-US/docs/Web/HTTP/Reference/Headers/Content-Security-Policy?authuser=0#host-source

Paths that end in / match any path they are a prefix of. For example:
example.com/api/ will permit resources from example.com/api/users/new.

Paths that do not end in / are matched exactly. For example:
https://example.com/file.js permits resources from https://example.com/file.js but not https://example.com/file.js/file2.js.

So I'm guessing google.com/pay/ is sufficient, unless there's a request specifically for exactly google.com/pay.

Copy link
Contributor

@joeluong-sfcc joeluong-sfcc left a comment

Choose a reason for hiding this comment

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

Thanks for updating the Shopper Baskets V2 hooks, the changes LGTM, did basic smoke testing and confirmed the following:

  1. new baskets/v2 cache key is being used instead of old cache keys
  2. guest checkout works
  3. registered checkout works

Copy link
Contributor

@vcua-mobify vcua-mobify left a comment

Choose a reason for hiding this comment

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

Thanks for updating the comments in default.js around using a vanity domain / ecom domain.

The changes in ssr.js seem fine to me also aside from what @sf-mkosak called out with with the google.com/pay csp entries.

'/organizations/',
string | undefined,
'/baskets/',
'/baskets/v2',
Copy link
Contributor

Choose a reason for hiding this comment

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

@joeluong-sfcc do we need the slash '/' at the end or not? Wondering why previously it had the slash at the end

Copy link
Contributor

Choose a reason for hiding this comment

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

@amittapalli please add the trailing slash to all the query keys for consistency sake. Yes, functionally, it doesn't matter because query key is just a string (not url). But we have an existing pattern/convention, which only this file does not follow. It may be confusing later in the future for us and the end users.

The Convention in This Codebase

  Looking at both files, there's a clear and deliberate pattern for trailing slashes:

  - Trailing slash → a dynamic ID segment follows immediately after
  - No trailing slash → it's a terminal static segment
                                                                                                           In ShopperBaskets:
  '/baskets/',   <-- trailing slash: basketId follows
  basketId,
  '/shipments/', <-- trailing slash: shipmentId follows
  shipmentId,
  '/payment-methods'  <-- no slash: terminal

  In ShopperBasketsV2:
  '/baskets/v2',  <-- NO trailing slash, but basketId still follows  ← inconsistent
  basketId,

  So yes, /baskets/v2 should be /baskets/v2/ to be consistent with the established convention.

  TanStack Query Best Practices (from tkdodo.eu)

  TanStack Query docs don't prescribe a specific string format — keys are opaque array tuples. The
  community best practices focus on:
  1. Hierarchy from generic → specific (which this codebase does well)
  2. Consistency within a project (which is the issue here)

  The URL-path-like format in this codebase is a local convention, not a TanStack recommendation. Since
  the strings are purely used as cache key identifiers (not as actual URLs), trailing slash consistency
  is entirely a project-style concern — but it matters because path() returns a prefix used for cache
  invalidation. An inconsistent format could cause confusion about what's intentional.

  Recommendation

  Fix the inconsistency in ShopperBasketsV2/queryKeyHelpers.ts: change /baskets/v2 → /baskets/v2/ (in
  both the QueryKeys type and all path() implementations). This aligns with the codebase's own convention
   that trailing slashes signal "a dynamic ID follows."

Copy link
Collaborator

Choose a reason for hiding this comment

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

Might be good to consider a lint rule for this, if that's possible. Not for this case but for future development.

'google.com',
'www.google.com'
'google.com/pay',
'google.com/pay/',
Copy link
Contributor

@vmarta vmarta Mar 6, 2026

Choose a reason for hiding this comment

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

When specifying a path for the CSP header, a trailing slash is significant. It means like matching the any requests that start with this path.

https://developer.mozilla.org/en-US/docs/Web/HTTP/Reference/Headers/Content-Security-Policy?authuser=0#host-source

Paths that end in / match any path they are a prefix of. For example:
example.com/api/ will permit resources from example.com/api/users/new.

Paths that do not end in / are matched exactly. For example:
https://example.com/file.js permits resources from https://example.com/file.js but not https://example.com/file.js/file2.js.

So I'm guessing google.com/pay/ is sufficient, unless there's a request specifically for exactly google.com/pay.

@amittapalli amittapalli added the skip changelog Skip the "Changelog Check" GitHub Actions step even if the Changelog.md files are not updated label Mar 6, 2026
@amittapalli amittapalli merged commit 64b0c1b into develop Mar 6, 2026
42 checks passed
@amittapalli amittapalli deleted the amittapalli/sfp-followup-fixes branch March 6, 2026 20:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

skip changelog Skip the "Changelog Check" GitHub Actions step even if the Changelog.md files are not updated

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants