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@ - [v3] Google Search Console fix createCodeVerifier #150

Merged
merged 5 commits into from
May 2, 2024

Conversation

adamraya
Copy link
Contributor

@adamraya adamraya commented Apr 29, 2024

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.

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.

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

yarn run check:size results:

current main branch:

 PASS  lib/index.cjs.js: 44.84KB < maxSize 45KB (gzip) 

 PASS  lib/index.esm.js: 44.73KB < maxSize 45KB (gzip) 

 PASS  commerce-sdk-isomorphic-with-deps.tgz: 338.08KB < maxSize 350KB (gzip) 

google-search-console-fix-v3 branch:

 PASS  lib/index.cjs.js: 44.96KB < maxSize 46KB (gzip) 

 PASS  lib/index.esm.js: 44.84KB < maxSize 46KB (gzip) 

 PASS  commerce-sdk-isomorphic-with-deps.tgz: 390.52KB < maxSize 400KB (gzip)

We go from 338.08KB to 390.52KB which is a decent jump for commerce-sdk-isomorphic-with-deps.tgz. We should try to make this package should be as lightweight as possible so just wanted to ask a few things:

  1. Might be a silly question, but is it possible for us to add entropy without the use of a third party package?
  2. Is there a specific reason why we use seedrandom versus an alternative package that provides similar functionality? Mostly asking if there's an alternative package we could use that is smaller in size

This PR looks good otherwise, just wanted to check for the reasoning behind the implementation

@adamraya
Copy link
Contributor Author

adamraya commented May 1, 2024

We go from 338.08KB to 390.52KB which is a decent jump for commerce-sdk-isomorphic-with-deps.tgz. We should try to make this package should be as lightweight as possible so just wanted to ask a few things:

  1. Might be a silly question, but is it possible for us to add entropy without the use of a third party package?
  2. Is there a specific reason why we use seedrandom versus an alternative package that provides similar functionality? Mostly asking if there's an alternative package we could use that is smaller in size

This PR looks good otherwise, just wanted to check for the reasoning behind the implementation

No silly question at all. I did notice the same. I think it's a good convo to have before making a decision.

The reasons to use seedrandom.

  1. It's documented on the nanoid docs. https://github.com/ai/nanoid?tab=readme-ov-file#custom-random-bytes-generator
  2. It's a popular NPM package with a high number of downloads. https://www.npmjs.com/package/seedrandom

We need to find a solution that doesn't rely on Math.random() to add entropy which is the source of the issue. I didn't find an alternative to seedrandom.

The Googlebot doesn't use a deterministic value when using seedrandom. That's why we get a new number from one run to the other.

joeluong-sfcc
joeluong-sfcc previously approved these changes May 2, 2024
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.

Thought about this some more, I think the size change should be ok since the minified bundle size is the size that we care more about, and that has only increased slightly.

},
{
"path": "commerce-sdk-isomorphic-with-deps.tgz",
"maxSize": "350 kB"
"maxSize": "400 kB"
Copy link

Choose a reason for hiding this comment

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

Is this because that lib has a lot of docs or large source that is being added to the gzip?

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 assume that's the reason as you can see in the previous Joel's comment comparing the before and after file sizes. We added two libraries seedrandom as a dependency and @types/seedrandom as a devDependency.

But as later Joel also mentions the minimized bundle size is the size we care about and that one only increased slightly, 1KB.

@adamraya adamraya merged commit d336d21 into main May 2, 2024
10 checks passed
@adamraya adamraya deleted the google-search-console-fix-v3 branch May 2, 2024 23:34
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.

4 participants