GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — crypto: remove default encoding from Hash/Hmac by tniessen · Pull Request #49167 · nodejs/node

Conversation

tniessen commented Aug 14, 2023 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member

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
	function getDefaultEncoding() {
	return 'buffer';
	}

Refs: #47182

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          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 commented Aug 14, 2023

Copy link

Copy Markdown

Collaborator

Review requested:

@nodejs/crypto

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

nodejs-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 dont-land-on-v16.x labels

Aug 14, 2023

anonrig approved these changes

Aug 14, 2023

View reviewed changes

tniessen mentioned this pull request

Aug 14, 2023

crypto: remove getDefaultEncoding() #49170

Closed

tniessen added author ready

PRs with CI started, the required approvals, and no outstanding review comments.

request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. labels

Aug 14, 2023

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label

Aug 14, 2023

This comment was marked as outdated.

panva approved these changes

Aug 14, 2023

View reviewed changes

nodejs-github-bot commented Aug 14, 2023

Copy link

Copy Markdown

Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

github-actions Bot mentioned this pull request

Aug 15, 2023

CI Reliability 2023-08-15 nodejs/reliability#637

Open

22 tasks

tniessen added the commit-queue PRs queued for automated landing through the Commit Queue. label

Aug 15, 2023

github-actions Bot mentioned this pull request

Aug 16, 2023

CI Reliability 2023-08-16 nodejs/reliability#638

Open

24 tasks

nodejs-github-bot removed the commit-queue PRs queued for automated landing through the Commit Queue. label

Aug 16, 2023

nodejs-github-bot merged commit 8ed4397 into nodejs:main

Aug 16, 2023

nodejs-github-bot commented Aug 16, 2023

Copy link

Copy Markdown

Collaborator

Landed in 8ed4397

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

tniessen added a commit to tniessen/node that referenced this pull request

Aug 16, 2023


          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

CI Reliability 2023-08-17 nodejs/reliability#639

Open

CI Reliability 2023-08-18 nodejs/reliability#640

Open

debadree25 pushed a commit that referenced this pull request

Aug 19, 2023


          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


          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


          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]>

UlisesGascon mentioned this pull request

Sep 10, 2023

v20.7.0 proposal #49592

Merged

Reviewers

panva

panva approved these changes

anonrig

anonrig approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

author ready

PRs with CI started, the required approvals, and no outstanding review comments.

crypto

Issues and PRs related to the crypto subsystem.

needs-ci

PRs that need a full CI run.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

GeezerVison

Conversation

tniessen commented Aug 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Aug 14, 2023

Uh oh!

This comment was marked as outdated.

nodejs-github-bot commented Aug 14, 2023

Uh oh!

nodejs-github-bot commented Aug 16, 2023

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

tniessen commented Aug 14, 2023 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.