@W-17386338 - Secure SSR Endpoints by Verifying SLAS Callback Requests by yunakim714 · Pull Request #2180 · SalesforceCommerceCloud/pwa-kit

yunakim714 · 2025-01-02T16:16:46Z

Description

To ensure the security of the POST endpoints written in ssr.js, we need to validate that incoming requests originate from SLAS. This will involve validating the x-slas-callback-token header, which is a JWT provided by SLAS during passwordless login and reset callbacks. Unauthorized requests should be rejected with a 401 Unauthorized response.

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

Add jwt-utils module that will take the x-slas-callback-token header and verify it against the jwks fetched for that SLAS instance
In ssr, fetch the JWKS for the SLAS instance with caching
Add new jose dependency

How to Test-Drive This PR

Go to https://wasatch-mrt-reset-password-poc.mrt-storefront-staging.com/
Start tail logs by running the following command: npx @salesforce/pwa-kit-dev@latest tail-logs --project wasatch-mrt --environment reset-password-poc --cloud-origin https://cloud-staging.mrt-staging.com --credentialsFile <pathToFileHere>
Click through the reset password flow
Verify that you see successful calls to both /jwks and /reset-password-callback

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

Changes were tested with a Screen Reader (iOS VoiceOver or Android Talkback) and had no issues
Changes comply with WCAG 2.0 guidelines levels A and AA
Changes to common UI patterns and interactions comply with WAI-ARIA best practices

Localization

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

packages/template-retail-react-app/app/ssr.js

hajinsuha1 · 2025-01-17T20:44:56Z

packages/template-retail-react-app/app/ssr.js

+ * @param {string} options.tenantId - The Tenant ID for the ECOM instance.
+ * @returns {Promise<*>} Promise with the JWKS data.
+ */
+async function jwksCaching(req, res, options) {


nit: would we consider moving this method, tenantIdRegExp, and shortCodeRegExp into jwt-utils to reduce the code in ssr.js?

hajinsuha1 · 2025-01-17T21:25:00Z

Verified that /jwks endpoint is only called for the first pasword reset or passwordless call
And verfiied both reset password and passwordless still work and send the email!

…fy-slas-post-callbacks Signed-off-by: Yuna Kim <84923642+yunakim714@users.noreply.github.com>

yunakim714 · 2025-01-21T18:10:47Z

packages/template-retail-react-app/app/utils/jwt-utils.js

+            headers: {
+                'User-Agent': 'OctoperfMercuryPerfTest'
+            }


TODO: Remove before merging into feature branch.

@adamraya When testing with staging, this header was required to bypass some Cloudflare restrictions. Was this something you saw when testing with Storefront Preview as well?

No. At the time we implemented this feature in the private BFF layer, we did not need to add any header.

Confirmed with Pratik that customer ECOM instances should hit production CDN so there should be no Cloudflare WAFs blocking this API call!

adamraya · 2025-01-22T02:13:00Z

packages/template-retail-react-app/app/utils/jwt-utils.js

+    const appOrigin = getAppOrigin()
+    const {app: appConfig} = getConfig()
+    const shortCode = appConfig.commerceAPI.parameters.shortCode
+    const tenantId = appConfig.commerceAPI.parameters.organizationId.replace(/^f_ecom_/, '')


The code assumes the values shortCode and tenantId from the static PWA Kit config instead of extracting and validating them directly from the SLAS token.

Probably the assumption is correct in most cases, but it’s not the safest approach. My understanding is that tokens should be validated based on the data they carry to ensure they match their intended context.

@adamraya I am using the values from the PWA Kit config because I want to make sure that we are retrieving the JWKs from the tenant that the customer has configured and make sure that the JWT from the callback matches that.

Would you consider doing both things? i.e.

Extract the shortCode and tenantId from the token.

Validate the values match the PWA Kit config values.

adamraya · 2025-01-22T02:21:28Z

packages/template-retail-react-app/app/utils/jwt-utils.js

+export const validateSlasCallbackToken = async (token) => {
+    try {
+        const jwks = createRemoteJWKSet()
+        const {payload} = await jwtVerify(token, jwks, {})


Are we only relying on jwtVerify() to validate the token? Should we also check headers, such as kid, alg, or jku?

I'm not an expert in this area, but I recommend reaching out to a SLAS expert who can guide you on what is the CC standard to consider a token valid.

Another question for the SLAS expert is whether it is safe to share these validation logic in a public repo.

I will ask Blair for his input on this!

@adamraya I believe it is sufficient to call jwtVerify and have the validation logic in this repo. Unless the PWA team has preferences on where this logic is placed, security wise I believe it is not a risk.

From Blair: "There is no need to validate the header portion of the JWT. The kid is the most important header claim as that is the KeyID of the public key to validate the body signature. Calling jwtVerify should be fine.
I think it would be okay to add the validation of the JWT signature to the repo. After all it is using the Public keys and not exposing the private key or how the JWT was signed."

I see. I mention this because we usually have an additional layer of validation that checks those claims to ensure the token context is valid, which jwtVerify does not validate. But let's continue with what the experts say.

adamraya · 2025-01-22T02:24:30Z

packages/template-retail-react-app/app/utils/jwt-utils.js

+            headers: {
+                'User-Agent': 'OctoperfMercuryPerfTest'
+            }


No. At the time we implemented this feature in the private BFF layer, we did not need to add any header.

…fy-slas-post-callbacks

yunakim714 · 2025-01-22T20:31:18Z

packages/template-retail-react-app/app/utils/jwt-utils.js

+    const payload = decodeJwt(token)
+    const subClaim = payload[CLAIM.ISSUER]
+    const tokens = subClaim.split(DELIMITER.ISSUER)
+    const tenantId = tokens[2]


Extracting the tenantId from the JWT here by reading the iss claim

yunakim714 · 2025-01-22T20:31:54Z

packages/template-retail-react-app/app/utils/jwt-utils.js

+    if (tenantId !== configTenantId) {
+        throw new Error(`The tenant ID in your PWA Kit configuration ("${configTenantId}") does not match the tenant ID in the SLAS callback token ("${tenantId}").`)
+    }


If the configured tenantId and the JWT tenantId do not match, an error is thrown.

The short code is not provided as part of the JWT, so we can only rely on the configured value in the config file.

yunakim714 added 9 commits January 2, 2025 11:15

changes from previous branch

58e5039

verify callbacktoken in post callbacks

32e0d42

add package lock

0ab4052

cleanup

33be681

regex check tenant id

adbccc9

add jest tests

55cefa6

revert changes

1817222

revert changes

7ffd098

cleanup tests

692e937

yunakim714 commented Jan 14, 2025

View reviewed changes

packages/template-retail-react-app/app/ssr.js Outdated Show resolved Hide resolved

yunakim714 requested a review from hajinsuha1 January 14, 2025 16:25

yunakim714 assigned jeremy-jung1 Jan 14, 2025

yunakim714 marked this pull request as ready for review January 14, 2025 16:26

yunakim714 requested a review from a team as a code owner January 14, 2025 16:26

yunakim714 requested a review from jeremy-jung1 January 14, 2025 16:27

yunakim714 unassigned jeremy-jung1 Jan 14, 2025

hajinsuha1 reviewed Jan 17, 2025

View reviewed changes

move jwkscaching function to jwtutils

9e38e2d

yunakim714 requested a review from adamraya January 21, 2025 15:57

Merge branch 'feature-passwordless-social-login' into W-17386338-veri…

374cb46

…fy-slas-post-callbacks Signed-off-by: Yuna Kim <84923642+yunakim714@users.noreply.github.com>

hajinsuha1 approved these changes Jan 21, 2025

View reviewed changes

yunakim714 commented Jan 21, 2025

View reviewed changes

adamraya reviewed Jan 22, 2025

View reviewed changes

yunakim714 and others added 4 commits January 22, 2025 10:15

Merge branch 'feature-passwordless-social-login' into W-17386338-veri…

03bcc67

…fy-slas-post-callbacks

remove octoperf header

18a8199

grab tenant id from jwt

590f420

add test for tenant id check

a6b1dc0

yunakim714 commented Jan 22, 2025

View reviewed changes

yunakim714 added the skip changelog Skip the "Changelog Check" GitHub Actions step even if the Changelog.md files are not updated label Jan 22, 2025

hajinsuha1 approved these changes Jan 22, 2025

View reviewed changes

remove octoperf header

82891a1

adamraya approved these changes Jan 22, 2025

View reviewed changes

yunakim714 merged commit b70a97c into feature-passwordless-social-login Jan 23, 2025
1 of 26 checks passed

Conversation

yunakim714 commented Jan 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Types of Changes

Changes

How to Test-Drive This PR

Checklists

General

Accessibility Compliance

Localization

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hajinsuha1 commented Jan 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adamraya Jan 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yunakim714 Jan 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

yunakim714 commented Jan 2, 2025 •

edited

Loading

hajinsuha1 commented Jan 17, 2025 •

edited

Loading

adamraya Jan 22, 2025 •

edited

Loading

yunakim714 Jan 22, 2025 •

edited

Loading