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

feat: make lib Web Crypto compliant #50

Closed
wants to merge 1 commit into from

Conversation

MichaelDeBoey
Copy link

@MichaelDeBoey MichaelDeBoey commented Aug 21, 2023

Closes #49


BREAKING CHANGE: Requires Node@>=17.4.0
BREAKING CHANGE: sign & unsign are now async
BREAKING CHANGE: sign & unsign's secret can only be a string

CC/ @natevw

@MichaelDeBoey MichaelDeBoey force-pushed the webcrypto-compliancy branch 2 times, most recently from ad8976d to ba96c2d Compare August 21, 2023 16:23
index.js Outdated Show resolved Hide resolved
BREAKING CHANGE: Requires Node@>=17.4.0
BREAKING CHANGE: `sign` & `unsign` are now async
BREAKING CHANGE: `sign` & `unsign`'s `secret` can only be a string
@natevw
Copy link
Collaborator

natevw commented Aug 21, 2023

Hi, thanks for your interest! I didn't review thoroughly but put one tiny bit of JSDoc definition feedback in case it's helpful to your branch.

Unfortunately this seems like a really huge jump and change for this library which is basically in "maintenance mode". The goals now are compatibility and stability (and of course security) but as you note there are several major breaking changes. (I'm also a bit worried that being compatible with WebCrypto browser-side would only encourage people to use it in insecure ways and/or leak their signing secrets to the client, etc.)

The value of this library at least in my opinion is that it's ± "finished" and can just keep getting used for its initial scope. For changes this major I would probably just recommend starting an alternate library that can progress more ambitiously if there's need for it. I sort of "inherited" (= got left holding the bag 😂 xref #36 but it's fine) this library from its original author, and while I can't speak for TJ my guess is nobody would mind there being more and newer alternatives.

I'll leave this PR open for a little while in case you or others want to chime in, but I just wanted to be up front about unlikely to accept these changes here to this package.

@MichaelDeBoey
Copy link
Author

MichaelDeBoey commented Aug 22, 2023

@natevw My goal was to make this a first step towards full Web Crypto compliance (using the crypto global instead of the Crypto API), so that this package could be used on other runtimes like Bun, Cloudflare and/or Deno as well

I know this PR has some breaking changes, but that's because the Node Crypto API is totally not standardized, whereas the Web Crypto API is 🤷‍♂️
These changes are also acceptable imo as going async is something that's widely happening in the community and most use-cases probably were with a string secret

@natevw
Copy link
Collaborator

natevw commented Oct 23, 2023

Yep, see https://github.com/nexdrew/cookie-signature-subtle via #49 (comment) which looks like it'd give the same functionality without causing the backwards compatibility concerns here. So assuming that's a good path forward for these kinds of use cases, I'll decline this PR here — again nothing inherent wrong with the code and do appreciate the contribution attempt, just a bit more of a maintenance jump than I'm comfortable tackling on this repo.

@natevw natevw closed this Oct 23, 2023
@MichaelDeBoey
Copy link
Author

@natevw If you want I can look at the other lib and keep the API the same

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.

Use crypto.webcrypto.* functions to be compliant with the Web Crypto spec
2 participants