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

Use browser crypto for SCRAM #988

Draft
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

scotttrinh
Copy link
Collaborator

Since all supported platforms support browser-compatible crypto, remove
the extraneous adapters and standardize on the browser API. This
requires a flag for Node v18 to enable the browser global
--experimental-global-webcrypto, but this is already required by a few
other libraries in our ecosystem.

scotttrinh added 4 commits May 1, 2024 10:58
Since all supported platforms support browser-compatible crypto, remove
the extraneous adapters and standardize on the browser API. This
requires a flag for Node v18 to enable the browser global
`--experimental-global-webcrypto`, but this is already required by a few
other libraries in our ecosystem.
Node 18 hides the `crypto` global behind a flag, so make note of that in
the main README.
@scotttrinh scotttrinh requested a review from jaclarke May 1, 2024 15:11
scotttrinh added 4 commits May 1, 2024 11:22
Since our CI still runs on Node 18, ensure we run with this flag on
which is a no-op for newer versions
There is some security reason we are specifically not allowed to set
NODE_OPTIONS in GITHUB_ENV.
@scotttrinh scotttrinh force-pushed the use-browser-crypto-everywhere branch from 6e07128 to 627eeef Compare May 1, 2024 15:40
README.md Outdated
@@ -39,6 +39,9 @@ writing some simple queries.
### Requirements

- Node.js 18+
- The driver requires the Browser API `crypto` to be available as a global.
Therefore, for Node 18, you _must_ run `node` with the special command line
Copy link
Member

Choose a reason for hiding this comment

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

Since you mention 18+ above, I would rephrase this to clarify that only 18 needs the flag, as the feature stabilized in 20+

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Instead of "Therefore, for Node 18," maybe "Therefore, for Node 18 only,"?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Or maybe "Therefore, for Node versions before 19,"?

@scotttrinh
Copy link
Collaborator Author

I'm going to sit on this for a while. Feels like something that we'd want to require a major version bump 😰

@scotttrinh scotttrinh marked this pull request as draft May 1, 2024 16:28
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.

2 participants