Runtime-deprecate calling digest() on HMAC more than once

[ChALkeR]

(Upd: detected by the scanner behind @deepview-autofix)

Hash behavior is reasonable:

> hash = require('crypto').createHash('sha256').update('data')
> hash.digest()
<Buffer 3a 6e b0 79 0f 39 ac 87 c9 4f 38 56 b2 dd 2c 5d 11 0e 68 11 60 22 61 a9 a9 23 d3 bb 23 ad c8 b7>
> hash.digest()
Uncaught Error [ERR_CRYPTO_HASH_FINALIZED]: Digest already called
 at Hash.digest (node:internal/crypto/hash:155:11) {
 code: 'ERR_CRYPTO_HASH_FINALIZED'
}

But HMAC, on the other hand, returns empty buffers on further .digest() calls, likely for compat reasons:

> hmac = require('crypto').createHmac('sha256', 'key').update('data')
> hmac.digest()
<Buffer 50 31 fe 3d 98 9c 6d 15 37 a0 13 fa 6e 73 9d a2 34 63 fd ae c3 b7 01 37 d8 28 e3 6a ce 22 1b d0>
> hmac.digest()
<Buffer >

This is a footgun with potential security risks, and should be first runtime-deprecated, then removed if no breakage is detected.

---

[panva]

Given the lack of usefulness of the second digest() call's return (and in so the likelyhood of it being depended on), shouldn't we treat this as a bug fix?

[panva]

The behaviour looks entirely deliberate

node/lib/internal/crypto/hash.js

Lines 185 to 190 in c3dd52a

	if (state[kFinalized]) {
	const buf = Buffer.from('');
	if (outputEncoding && outputEncoding !== 'buffer')
	return buf.toString(outputEncoding);
	return buf;
	}

but looking at the blame its introduction traces back to #15231 by @jasnell, its parent was just throwing the same as Hash

node/lib/crypto.js

Line 121 in 8fa5fcc

Hmac.prototype.update = Hash.prototype.update;

[jasnell]

Oh, we reaching way back. I don't remember what the reasoning here was. I'm +1 on deprecation of the inconsistent behavior

[panva]

Test Wayback machine: #14122, a3b9f4b

Add a test that ensures the second call to .digest() returns an empty
HMAC, like it did before. No comment on whether that is the right
behavior or not.

[panva]

Yeah, +1 deprecate it.

[ChALkeR]

@panva this was also auto-detected by the scanner behind @deepview-autofix btw.

But this in fact looks intentional so I would recommend removing this in a major as a safeguard, following a deprecation cycle.

Upd: github glitched, I replied to #62838 (comment), all the other comments (end the edit on that one) just loaded. I agree with those.

[panva]

Enough time to land a semver-major PRs that contain breaking changes and should be released in the next major version. runtime-deprecation for v27.x and then soonest with v28.x EOL the deprecation.

[mizoz]

Hi! I'd love to contribute. Could you assign this issue to me?

[Anshikakalpana]

Hi! I’d like to work on this issue. I’m preparing a PR for it.