Skip to content

Comments

Improving error handling in digest copy#6166

Merged
jasnell merged 1 commit intomainfrom
jasnell/tightening-hash-copy
Feb 25, 2026
Merged

Improving error handling in digest copy#6166
jasnell merged 1 commit intomainfrom
jasnell/tightening-hash-copy

Conversation

@jasnell
Copy link
Collaborator

@jasnell jasnell commented Feb 24, 2026

Matching node.js behavior

@jasnell jasnell requested a review from anonrig February 24, 2026 23:12
@jasnell jasnell requested review from a team as code owners February 24, 2026 23:12
@jasnell jasnell enabled auto-merge February 24, 2026 23:12
@ask-bonk
Copy link
Contributor

ask-bonk bot commented Feb 24, 2026

Now I understand the full flow:

  1. hash.copy() at the TS layer (line 118-123) checks kFinalized first (line 120), then calls new Hash(this[kHandle], options) (line 122)
  2. The Hash constructor when receiving a HashHandle calls algorithm.copy(xofLen) (line 102)
  3. The test bypasses the TS layer's digest() (which sets kFinalized = true) by calling handle.digest() directly on the C++ handle
  4. Then hash.copy() goes through TS which checks kFinalized (still false), passes through, and calls this[kHandle].copy() which hits the C++ clone() method

So the C++ clone() method is the right place for this defense-in-depth check. The old behavior was to silently copy the digest result, creating a new HashContext with the finalized state — which is wrong behavior (Node.js throws).

Now let me check the error type. The PR uses DOMOperationError which is consistent with the other JSG_FAIL_REQUIRE in the same file. Let me check what other "finalized" errors use:

Looking at digest.c++:185 and digest.c++:363, the other finalized checks already use DOMOperationError. Meanwhile, the Node.js layer in crypto.c++ uses plain Error. The DOMOperationError is used for the HashContext/HmacContext methods (which are lower-level crypto primitives), while the Error type is used in the higher-level Node.js crypto module. This is consistent.

Let me also verify that the test checks for the right error pattern. The test asserts message: /already been finalized/ which matches the C++ error "Hash context has already been finalized.".

This is a clean, well-scoped change. The fix is correct -- Node.js throws when copying after digest, and the old behavior of silently cloning the digest was incorrect. The test properly validates the new behavior by reaching into the internal handle to trigger the C++ path.

LGTM

github run

@jasnell jasnell merged commit abea619 into main Feb 25, 2026
25 of 26 checks passed
@jasnell jasnell deleted the jasnell/tightening-hash-copy branch February 25, 2026 00:21
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.

2 participants