-
Notifications
You must be signed in to change notification settings - Fork 144
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
@W-17386338 - Secure SSR Endpoints by Verifying SLAS Callback Requests #2180
Changes from 11 commits
58e5039
32e0d42
0ab4052
33be681
adbccc9
55cefa6
1817222
7ffd098
692e937
9e38e2d
374cb46
03bcc67
18a8199
590f420
a6b1dc0
82891a1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,75 @@ | ||
/* | ||
* Copyright (c) 2021, salesforce.com, inc. | ||
* All rights reserved. | ||
* 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 {createRemoteJWKSet as joseCreateRemoteJWKSet, jwtVerify} from 'jose' | ||
import {getAppOrigin} from '@salesforce/pwa-kit-react-sdk/utils/url' | ||
import {getConfig} from '@salesforce/pwa-kit-runtime/utils/ssr-config' | ||
|
||
const throwSlasTokenValidationError = (message, code) => { | ||
throw new Error(`SLAS Token Validation Error: ${message}`, code) | ||
} | ||
|
||
export const createRemoteJWKSet = () => { | ||
const appOrigin = getAppOrigin() | ||
const {app: appConfig} = getConfig() | ||
const shortCode = appConfig.commerceAPI.parameters.shortCode | ||
const tenantId = appConfig.commerceAPI.parameters.organizationId.replace(/^f_ecom_/, '') | ||
const JWKS_URI = `${appOrigin}/${shortCode}/${tenantId}/oauth2/jwks` | ||
return joseCreateRemoteJWKSet(new URL(JWKS_URI)) | ||
} | ||
|
||
export const validateSlasCallbackToken = async (token) => { | ||
try { | ||
const jwks = createRemoteJWKSet() | ||
const {payload} = await jwtVerify(token, jwks, {}) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are we only relying on 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I will ask Blair for his input on this! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @adamraya I believe it is sufficient to call From Blair: "There is no need to validate the header portion of the JWT. The There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
return payload | ||
} catch (error) { | ||
throwSlasTokenValidationError(error.message, 401) | ||
} | ||
} | ||
|
||
const tenantIdRegExp = /^[a-zA-Z]{4}_([0-9]{3}|s[0-9]{2}|stg|dev|prd)$/ | ||
const shortCodeRegExp = /^[a-zA-Z0-9-]+$/ | ||
|
||
/** | ||
* Handles JWKS (JSON Web Key Set) caching the JWKS response for 2 weeks. | ||
* | ||
* @param {object} req Express request object. | ||
* @param {object} res Express response object. | ||
* @param {object} options Options for fetching B2C Commerce API JWKS. | ||
* @param {string} options.shortCode - The Short Code assigned to the realm. | ||
* @param {string} options.tenantId - The Tenant ID for the ECOM instance. | ||
* @returns {Promise<*>} Promise with the JWKS data. | ||
*/ | ||
export async function jwksCaching(req, res, options) { | ||
const {shortCode, tenantId} = options | ||
|
||
const isValidRequest = tenantIdRegExp.test(tenantId) && shortCodeRegExp.test(shortCode) | ||
if (!isValidRequest) | ||
return res | ||
.status(400) | ||
.json({error: 'Bad request parameters: Tenant ID or short code is invalid.'}) | ||
try { | ||
const JWKS_URI = `https://${shortCode}.api.commercecloud.salesforce.com/shopper/auth/v1/organizations/f_ecom_${tenantId}/oauth2/jwks` | ||
const response = await fetch(JWKS_URI, { | ||
headers: { | ||
'User-Agent': 'OctoperfMercuryPerfTest' | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No. At the time we implemented this feature in the private BFF layer, we did not need to add any header. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Confirmed with Pratik that customer ECOM instances should hit production CDN so there should be no Cloudflare WAFs blocking this API call! |
||
}) | ||
|
||
if (!response.ok) { | ||
throw new Error('Request failed with status: ' + response.status) | ||
} | ||
|
||
// JWKS rotate every 30 days. For now, cache response for 14 days so that | ||
// fetches only need to happen twice a month | ||
res.set('Cache-Control', 'public, max-age=1209600') | ||
|
||
return res.json(await response.json()) | ||
} catch (error) { | ||
res.status(400).json({error: `Error while fetching data: ${error.message}`}) | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,125 @@ | ||
/* | ||
* Copyright (c) 2025, Salesforce, Inc. | ||
* All rights reserved. | ||
* 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 {createRemoteJWKSet as joseCreateRemoteJWKSet, jwtVerify} from 'jose' | ||
import { | ||
createRemoteJWKSet, | ||
validateSlasCallbackToken | ||
} from '@salesforce/retail-react-app/../../app/utils/jwt-utils' | ||
import {getAppOrigin} from '@salesforce/pwa-kit-react-sdk/utils/url' | ||
import {getConfig} from '@salesforce/pwa-kit-runtime/utils/ssr-config' | ||
|
||
const MOCK_JWKS = { | ||
keys: [ | ||
{ | ||
kty: 'EC', | ||
crv: 'P-256', | ||
use: 'sig', | ||
kid: '8edb82b1-f6d5-49c1-bab2-c0d152ee3d0b', | ||
x: 'i8e53csluQiqwP6Af8KsKgnUceXUE8_goFcvLuSzG3I', | ||
y: 'yIH500tLKJtPhIl7MlMBOGvxQ_3U-VcrrXusr8bVr_0' | ||
}, | ||
{ | ||
kty: 'EC', | ||
crv: 'P-256', | ||
use: 'sig', | ||
kid: 'da9effc5-58cb-4a9c-9c9c-2919fb7d5e5e', | ||
x: '_tAU1QSvcEkslcrbNBwx5V20-sN87z0zR7gcSdBETDQ', | ||
y: 'ZJ7bgy7WrwJUGUtzcqm3MNyIfawI8F7fVawu5UwsN8E' | ||
}, | ||
{ | ||
kty: 'EC', | ||
crv: 'P-256', | ||
use: 'sig', | ||
kid: '5ccbbc6e-b234-4508-90f3-3b9b17efec16', | ||
x: '9ULO2Atj5XToeWWAT6e6OhSHQftta4A3-djgOzcg4-Q', | ||
y: 'JNuQSLMhakhLWN-c6Qi99tA5w-D7IFKf_apxVbVsK-g' | ||
} | ||
] | ||
} | ||
|
||
jest.mock('@salesforce/pwa-kit-react-sdk/utils/url', () => ({ | ||
getAppOrigin: jest.fn() | ||
})) | ||
|
||
jest.mock('@salesforce/pwa-kit-runtime/utils/ssr-config', () => ({ | ||
getConfig: jest.fn() | ||
})) | ||
|
||
jest.mock('jose', () => ({ | ||
createRemoteJWKSet: jest.fn(), | ||
jwtVerify: jest.fn() | ||
})) | ||
|
||
describe('createRemoteJWKSet', () => { | ||
afterEach(() => { | ||
jest.restoreAllMocks() | ||
}) | ||
|
||
it('constructs the correct JWKS URI and call joseCreateRemoteJWKSet', () => { | ||
const mockAppOrigin = 'https://test-storefront.com' | ||
getAppOrigin.mockReturnValue(mockAppOrigin) | ||
getConfig.mockReturnValue({ | ||
app: { | ||
commerceAPI: { | ||
parameters: { | ||
shortCode: 'abc123', | ||
organizationId: 'f_ecom_aaaa_001' | ||
} | ||
} | ||
} | ||
}) | ||
joseCreateRemoteJWKSet.mockReturnValue('mockJWKSet') | ||
|
||
const expectedJWKS_URI = new URL(`${mockAppOrigin}/abc123/aaaa_001/oauth2/jwks`) | ||
|
||
const res = createRemoteJWKSet() | ||
|
||
expect(getAppOrigin).toHaveBeenCalled() | ||
expect(getConfig).toHaveBeenCalled() | ||
expect(joseCreateRemoteJWKSet).toHaveBeenCalledWith(expectedJWKS_URI) | ||
expect(res).toBe('mockJWKSet') | ||
}) | ||
}) | ||
|
||
describe('validateSlasCallbackToken', () => { | ||
beforeEach(() => { | ||
jest.resetAllMocks() | ||
const mockAppOrigin = 'https://test-storefront.com' | ||
getAppOrigin.mockReturnValue(mockAppOrigin) | ||
getConfig.mockReturnValue({ | ||
app: { | ||
commerceAPI: { | ||
parameters: { | ||
shortCode: 'abc123', | ||
organizationId: 'f_ecom_aaaa_001' | ||
} | ||
} | ||
} | ||
}) | ||
joseCreateRemoteJWKSet.mockReturnValue(MOCK_JWKS) | ||
}) | ||
|
||
it('returns payload when callback token is valid', async () => { | ||
const mockPayload = {sub: '123', role: 'admin'} | ||
jwtVerify.mockResolvedValue({payload: mockPayload}) | ||
|
||
const res = await validateSlasCallbackToken('mock.slas.token') | ||
|
||
expect(jwtVerify).toHaveBeenCalledWith('mock.slas.token', MOCK_JWKS, {}) | ||
expect(res).toEqual(mockPayload) | ||
}) | ||
|
||
it('throws validation error when the token is invalid', async () => { | ||
const mockError = new Error('Invalid token') | ||
jwtVerify.mockRejectedValue(mockError) | ||
|
||
await expect(validateSlasCallbackToken('mock.slas.token')).rejects.toThrow( | ||
mockError.message | ||
) | ||
expect(jwtVerify).toHaveBeenCalledWith('mock.slas.token', MOCK_JWKS, {}) | ||
}) | ||
}) |
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code assumes the values
shortCode
andtenantId
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you consider doing both things? i.e.
shortCode
andtenantId
from the token.