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

Conversation

adamraya
Copy link
Collaborator

@adamraya adamraya commented Apr 27, 2024

Description

Problem
When Google crawler calls createCodeVerifier() to create the code verifier we use for the PKCE authentication flow, the result from nanoid() is the same in two different runs. This causes the same generated code_challenge to be used twice thus Google crawler gets the error stating that the PKCE code_challenge must be unique error.

The root of this issue is the fact that Googlebot’s implementation of random() is deterministic, meaning that it won’t produce unique random values from one string of runs to the next.

The result is that Google crawler indexes the error page instead of the proper page.

Solution
Add entropy to nanoid() using the library seedrandom to avoid duplicated code_challenge sent to SCAPI by Google crawler browser.

Nanoid docs on Custom Random Bytes Generator.


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.

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 entropy to nanoid by using a custom random bytes generator.
  • Fix failing GitHub Actions Windows builds.

How to Test-Drive This PR

  1. Follow the steps to reproduce in the quip doc: https://salesforce.quip.com/U1k5A2fXzpym#temp:C:DXJd70645fbe1a44e999dff07b74
  2. Verify Google Search Console can successfully index the indicated URLs.

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)

@adamraya adamraya marked this pull request as ready for review April 30, 2024 16:22
@adamraya adamraya requested a review from a team as a code owner April 30, 2024 16:22
@@ -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.

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

@adamraya adamraya merged commit 5af2acf into release-2.8.x May 2, 2024
29 checks passed
@adamraya adamraya deleted the google-search-console-fix-v2 branch May 2, 2024 23:33
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.

3 participants