crypto: allow inspecting a crypto key prototype

[BridgeAR]

This would fail so far due to accessing a undefined property.

[nodejs-github-bot]

Review requested:

• @nodejs/crypto

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.21%. Comparing base (f89baf2) to head (7215b4a).
Report is 369 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #57890      +/-   ##
==========================================
- Coverage   90.24%   90.21%   -0.04%     
==========================================
  Files         630      630              
  Lines      185670   186445     +775     
  Branches    36401    36621     +220     
==========================================
+ Hits       167567   168196     +629     
- Misses      10992    11052      +60     
- Partials     7111     7197      +86     

Files with missing lines	Coverage Δ
lib/internal/crypto/keys.js	95.40% <100.00%> (ø)

... and 95 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[tniessen]

If this is only necessary for inspect(), perhaps the fix could be inside [kInspect]() { ... }? In any other case, get type() should throw if this[kKeyObject] is undefined.

[BridgeAR]

@tniessen I thought about that as well initially but discarded the thought, when thinking about directly accessing the property. I think it should just return undefined in that case, since it would otherwise be a bad error message for the user.

dismissing my approval, @tniessen is correct in that if the property is not defined a throw is appropriate behaviour for regular instances of CryptoKey

[BridgeAR]

@panva @jasnell @tniessen would one of you mind picking this up? I just stumbled upon it while working on something else and I did not expect a lot of bikeshedding in this case. I don't really feel strongly enough about either implementation.
I do think this does improve the current status quo.

The current suggestion would require all of our instanceof checks to be extended to the accessed property being accessible in all getters to have a consistent behavior (accessing .type would otherwise throw while accessing the other properties would not, even though they are also undefined). So changing all is the only consistent way I can think about, while I don't feel strongly enough about this to implement this and would otherwise close the PR.

[panva]

@BridgeAR what are the use cases for inspecting the prototype in the first place? (genuinely just curious here)

[BridgeAR]

what are the use cases for inspecting the prototype

@panva I have none. I just wrote some tests elsewhere and this failed. The current error message is quite bad in that case (accessing a property of undefined). I also wondered about it meant to fail as @tniessen pointed out, but doing that only for .type would be highly inconsistent with all other properties, so I decided to just be lenient and show undefined (which I find is actually fine for this specific case).

[tniessen]

I am a bit confused as to what the desired output is (other than not throwing an error). I assumed inspecting the prototype should behave like inspecting many other prototypes in Node.js, e.g., type: [Getter].

I think the root problem is that this instanceof CryptoKey evaluates to true for the prototype, probably because the key is actually an instance of InternalCryptoKey and the prototype of InternalCryptoKey.prototype is CryptoKey.prototype?

[tniessen]

The current suggestion would require all of our instanceof checks to be extended to the accessed property being accessible in all getters to have a consistent behavior

I also wondered about it meant to fail as @tniessen pointed out, but doing that only for .type would be highly inconsistent with all other properties

To clarify, I was not suggesting that we should explicitly check for this case and throw in the getters. If we can somehow fix the prototype chain such that Object.getProtoypeOf(key) instanceof CryptoKey becomes false, that'd be great just to prevent this sort of scenario (and for consistency with other runtimes).

I decided to just be lenient and show undefined (which I find is actually fine for this specific case).

I think that, if this patch ends up explicitly allowing this[kKeyObject] to be undefined, then there should at least be a comment inside the getter explaining the edge case that makes it necessary, since I'd be very surprised to come across this as a reader.

In the end, explicitly allowing this might not be a particularly elegant solution, but since we are talking about JavaScript, I can live with that.

[BridgeAR]

I'll go ahead and close this for now. I don't think it's important enough for now.