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

The secret can be any CipherKey, not just a String. #44

Merged
merged 3 commits into from
Feb 27, 2023

Conversation

jyasskin
Copy link
Contributor

@jyasskin jyasskin commented Dec 9, 2022

No description provided.

@natevw
Copy link
Collaborator

natevw commented Dec 9, 2022

Thanks, looks like this is a necessary comment-based documentation update for the work done on #33.

I'm a bit unclear on using crypto.CipherKey to describe the field though. Is that your own shorthand? The current docs for https://nodejs.org/api/crypto.html#cryptocreatehmacalgorithm-key-options don't use that terminology.

(They do mention something called CryptoKey but that's just one of the options and afaict is a specific type of object in the Web Crypto API.)

Afaict there's no official type definitions for node.js, but my IDE relies on @types/node as I suspect many others do, and their current definition for createHmac uses BinaryLike | KeyObject. Maybe we can borrow those names as being close enough human-readable descriptions too?

index.js Outdated
@@ -8,7 +8,7 @@ var crypto = require('crypto');
* Sign the given `val` with `secret`.
*
* @param {String} val
* @param {String} secret
* @param {crypto.CipherKey} secret
Copy link
Collaborator

Choose a reason for hiding this comment

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

see background in the PR comments, but maybe @param {String|BinaryLike|KeyObject} secret would be most clear here?

[technically "String" is already part of "BinaryLike" in the typescript defs that we're borrowing from, but I'd rather err on the side of people knowing a string is still fine here]

index.js Outdated
@@ -28,7 +28,7 @@ exports.sign = function(val, secret){
* returning `false` if the signature is invalid.
*
* @param {String} input
* @param {String} secret
* @param {crypto.CipherKey} secret
Copy link
Collaborator

Choose a reason for hiding this comment

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

[same here as whatever settled on above]

@jyasskin
Copy link
Contributor Author

jyasskin commented Dec 9, 2022

Ah, sorry for not explaining that. CipherKey is defined at https://github.com/DefinitelyTyped/DefinitelyTyped/blob/0039ca2aaaf3b372e9664fd2e765a1eba3ca5a46/types/node/crypto.d.ts#L675 as

type CipherKey = BinaryLike | KeyObject;

so just a shorthand for the argument type of createHmac. And in fact, tsc *.js --checkJs --noEmit passes with this change.

+1 that including "String" is probably clearer for JS folks trying to refer to the nodejs docs. It might also be clearest to expand out BinaryLike to string | NodeJS.ArrayBufferView. I've done that, but let me know if you'd prefer to use BinaryLike instead.

@natevw
Copy link
Collaborator

natevw commented Feb 27, 2023

Whoops, sorry I missed your updates here! Looks good to me, I'll merge and bump a patch release for this.

@natevw natevw merged commit 29d44c3 into tj:master Feb 27, 2023
@natevw
Copy link
Collaborator

natevw commented Feb 27, 2023

Published as cookie-signature@1.2.1 — thanks again for getting this caught up!

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.

None yet

2 participants