Skip to content

crypto: remove default encoding from Hash/Hmac#49167

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.

Sign up for GitHub

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

Jump to bottom
Merged
nodejs-github-bot merged 1 commit into from
Aug 16, 2023
Merged

crypto: remove default encoding from Hash/Hmac #49167

nodejs-github-bot merged 1 commit into from
Aug 16, 2023

Conversation

@tniessen
Copy link
Member

@tniessentniessen commented Aug 14, 2023•
edited
Loading

getDefaultEncoding() always returns 'buffer' in Node.js 20. It requires some careful justification but the default encoding can be eliminated from hash.js entirely. The reasoning is almost identical with that in #49145 so I won't repeat it here. The main thing to note here is that digest() conveniently defaults to BUFFER for falsy encodings.

This partially addresses:

node/lib/internal/crypto/util.js

Lines 77 to 80 in af9b48a

// TODO(tniessen): remove all call sites and this function
functiongetDefaultEncoding(){
return'buffer';
}

Refs: #47182

@tniessen
crypto: remove default encoding from Hash/Hmac
c8d479a
getDefaultEncoding() always returns 'buffer' in Node.js 20. It requires some careful justification but the default encoding can be eliminated from hash.js entirely. The reasoning is almost identical with that in nodejs#49145 so I won't repeat it here. Refs: nodejs#47182
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Aug 14, 2023

Review requested:

  • @nodejs/crypto

@nodejs-github-botnodejs-github-bot added crypto Issues and PRs related to the crypto subsystem. needs-ci PRs that need a full CI run. labels Aug 14, 2023
tniessen added a commit to tniessen/node that referenced this pull request Aug 14, 2023
@tniessen
crypto: remove getDefaultEncoding()
d66373c
Refs: nodejs#47182 Refs: nodejs#47869 Refs: nodejs#47943 Refs: nodejs#47998 Refs: nodejs#49140 Refs: nodejs#49145 Refs: nodejs#49167 Refs: nodejs#49169
@tniessentniessen mentioned this pull request Aug 14, 2023
Closed
@tniessentniessen added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Aug 14, 2023
@github-actionsgithub-actionsbot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 14, 2023
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Aug 14, 2023

CI: https://ci.nodejs.org/job/node-test-pull-request/53329/

@github-actionsgithub-actionsbot mentioned this pull request Aug 15, 2023
Open
22 tasks
@tniessentniessen added the commit-queue Add this label to land a pull request using GitHub Actions. label Aug 15, 2023
@github-actionsgithub-actionsbot mentioned this pull request Aug 16, 2023
Open
24 tasks
@nodejs-github-botnodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Aug 16, 2023
@nodejs-github-botnodejs-github-bot merged commit 8ed4397 into nodejs:mainAug 16, 2023
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Aug 16, 2023

Landed in 8ed4397

tniessen added a commit to tniessen/node that referenced this pull request Aug 16, 2023
@tniessen
crypto: remove getDefaultEncoding()
d4a91a8
Refs: nodejs#47182 Refs: nodejs#47869 Refs: nodejs#47943 Refs: nodejs#47998 Refs: nodejs#49140 Refs: nodejs#49145 Refs: nodejs#49167 Refs: nodejs#49169
This was referenced Aug 17, 2023
Open
Open
debadree25 pushed a commit that referenced this pull request Aug 19, 2023
@tniessen@debadree25
crypto: remove getDefaultEncoding()
78842cf
Refs: #47182 Refs: #47869 Refs: #47943 Refs: #47998 Refs: #49140 Refs: #49145 Refs: #49167 Refs: #49169 PR-URL: #49170 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Filip Skokan <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
UlisesGascon pushed a commit that referenced this pull request Sep 10, 2023
@tniessen@UlisesGascon
crypto: remove default encoding from Hash/Hmac
c795083
getDefaultEncoding() always returns 'buffer' in Node.js 20. It requires some careful justification but the default encoding can be eliminated from hash.js entirely. The reasoning is almost identical with that in #49145 so I won't repeat it here. Refs: #47182 PR-URL: #49167 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Filip Skokan <[email protected]>
UlisesGascon pushed a commit that referenced this pull request Sep 10, 2023
@tniessen@UlisesGascon
crypto: remove getDefaultEncoding()
7eb10a3
Refs: #47182 Refs: #47869 Refs: #47943 Refs: #47998 Refs: #49140 Refs: #49145 Refs: #49167 Refs: #49169 PR-URL: #49170 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Filip Skokan <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
@UlisesGasconUlisesGascon mentioned this pull request Sep 10, 2023
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@panvapanvapanva approved these changes

@anonriganonriganonrig approved these changes

Assignees

No one assigned

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.cryptoIssues and PRs related to the crypto subsystem.needs-ciPRs that need a full CI run.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@tniessen@nodejs-github-bot@panva@anonrig