Skip to content
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-15248536@ - [v2] Google Search Console fix createCodeVerifier #1765

Merged
merged 11 commits into from
May 2, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,7 @@ const runGenerator = () => {

const extension = process.platform === 'win32' ? '.cmd' : ''
const npm = `npm${extension}`
const foundNpm = cp.spawnSync(npm, ['-v']).stdout.toString().trim()
const foundNpm = cp.spawnSync(npm, ['-v'], {shell: true}).stdout.toString().trim()
Copy link
Collaborator

Choose a reason for hiding this comment

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

This change doesn't look related.. what and why did we make this change?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need to mention this in a change log anywhere fixing a bug I suspect?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Correct. It's an unrelated change. I added details to the PR description.
After the latest v2 release, the Windows GA builds were failing on v2, and it seems like we decided not to fix it at that time. So I included the fix in this PR.

https://github.com/SalesforceCommerceCloud/pwa-kit/tree/release-2.8.x
https://github.com/SalesforceCommerceCloud/pwa-kit/actions/runs/8731228868/job/23956306359

Extra: Fix failing GitHub Actions Windows builds
Node.js issue: nodejs/node#52554

The latest security fix on Node.js on Windows blocks the spawSync .cmd unless { shell: true } is used.

Good call on adding the change to the change log.

const flags = semver.satisfies(foundNpm, '>=7') ? '-y' : ''

const pathToNpxCache = p.join(sh.exec('npm config get cache', {silent: true}).trim(), '_npx')
Expand Down
5 changes: 5 additions & 0 deletions packages/template-retail-react-app/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,8 @@
## v2.8.4 (May 2, 2024)

- Fixed createCodeVerifier adding entropy to be successfully indexed by Google Search Console. [#1765](https://github.com/SalesforceCommerceCloud/pwa-kit/pull/1765)
- Fixed Node.js on Windows blocking spawSync .cmd unless { shell: true } is used with trusted code. [#1765](https://github.com/SalesforceCommerceCloud/pwa-kit/pull/1765)

## v2.8.3 (Apr 9, 2024)

- Storefront Preview: avoid stale cached Commerce API responses, whenever the Shopper Context is set [#1740](https://github.com/SalesforceCommerceCloud/pwa-kit/pull/1740)
Expand Down
14 changes: 12 additions & 2 deletions packages/template-retail-react-app/app/commerce-api/pkce.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,22 +4,32 @@
* 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 {nanoid} from 'nanoid'
import {customRandom, urlAlphabet} from 'nanoid'
import {encode as base64encode} from 'base64-arraybuffer'
import seedrandom from 'seedrandom'

// Server Side
const randomstring = require('randomstring')

// Globals
const isServer = typeof window === 'undefined'

/**
* Adds entropy to nanoid() using seedrandom to ensure that the code_challenge sent to SCAPI by Google's crawler browser is unique.
* Solves the issue with Google's crawler getting the same result from nanoid() in two different runs, which results in the same PKCE code_challenge being used twice.
*/
const nanoid = () => {
const rng = seedrandom(+new Date(), {entropy: true})
return customRandom(urlAlphabet, 128, (size) => new Uint8Array(size).map(() => 256 * rng()))()
}

/**
* Creates Code Verifier use for PKCE auth flow.
*
* @returns {String} The 128 character length code verifier.
*/
export const createCodeVerifier = () => {
return isServer ? randomstring.generate(128) : nanoid(128)
return isServer ? randomstring.generate(128) : nanoid()
}

/**
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
/*
* Copyright (c) 2024, 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 {createCodeVerifier, generateCodeChallenge} from './pkce'
import seedrandom from 'seedrandom'

describe('PKCE Utility Functions', () => {
describe('createCodeVerifier', () => {
test('should generate unique code verifiers', () => {
const verifiers = new Set()
const numVerifiers = 100

for (let i = 0; i < numVerifiers; i++) {
const verifier = createCodeVerifier()
verifiers.add(verifier)
}
expect(verifiers.size).toBe(numVerifiers)
})

test('should provide sufficient entropy', () => {
const seed1 = 123456789
const seed2 = 987654321
seedrandom(seed1, {global: true})
const verifier1 = createCodeVerifier()
seedrandom(seed2, {global: true})
const verifier2 = createCodeVerifier()
// the generated verifiers should be different because we're using different seeds
expect(verifier1).not.toBe(verifier2)
})
})

describe('generateCodeChallenge', () => {
test('should generate a code challenge based on the provided code verifier', async () => {
const codeVerifier = createCodeVerifier()
const codeChallenge = await generateCodeChallenge(codeVerifier)

expect(codeChallenge).toBeDefined()
})
})
})
15 changes: 14 additions & 1 deletion packages/template-retail-react-app/package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 3 additions & 2 deletions packages/template-retail-react-app/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,8 @@
"react-helmet": "^6.1.0",
"react-hook-form": "^6.15.8",
"react-intl": "^5.25.1",
"react-router-dom": "^5.3.4"
"react-router-dom": "^5.3.4",
"seedrandom": "^3.0.5"
Copy link
Collaborator

Choose a reason for hiding this comment

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

don't forget to file 3pp

Copy link
Collaborator Author

@adamraya adamraya Apr 30, 2024

Choose a reason for hiding this comment

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

The seedrandom 3PP was already filed and approved via the new Intake UI.

},
"peerDependencies": {
"@chakra-ui/system": "^1.12.1"
Expand Down Expand Up @@ -80,7 +81,7 @@
"bundlesize": [
{
"path": "build/main.js",
"maxSize": "55 kB"
"maxSize": "56 kB"
},
{
"path": "build/vendor.js",
Expand Down
5 changes: 3 additions & 2 deletions scripts/check-version.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,10 @@ const extension = isWin ? '.cmd' : ''
const npm = `npm${extension}`

const spawnSync = (...args) => {
const proc = childProc.spawnSync(...args)
const proc = childProc.spawnSync(...args, { shell: true })
if (proc.status !== 0) {
throw proc.stderr.toString()
console.error(proc)
throw proc.stderr ? proc.stderr.toString() : 'Unknown error'
}
return proc
}
Expand Down
Loading