crypto: fix unsigned conversion of 4-byte RSA publicExponent

[deepview-autofix]

bigIntArrayToUnsignedInt used the signed << operator, so when the most significant byte of a 4-byte input had its top bit set (e.g. [0x80, 0x00, 0x00, 0x01]) the result was a negative Int32 instead of the intended unsigned 32-bit value. This caused any RSA publicExponent exactly 4 bytes long with the top bit set to be parsed incorrectly. Coerce the final value with >>> 0 and add a unit test.

[nodejs-github-bot]

Review requested:

• @nodejs/crypto

[ChALkeR]

This is mine (scanner + autofixer)

bigIntArrayToUnsignedInt was, in fact, signed prior to the fix, and returned negative values for half of the accepted input range.

That happens on 4 significant byte input and is currently only reachable through RSA-OAEP publicExponent which is usually at most 3 significant bytes (Chrome even only allows two specific values), so real impact should be non-existent.

It should still be fixed to return unsigned values (especially if something new starts using it as this is an per-standard input parsing helper).

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 89.61%. Comparing base (14e16db) to head (b1f8d08).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #62839      +/-   ##
==========================================
- Coverage   89.62%   89.61%   -0.02%     
==========================================
  Files         706      706              
  Lines      219136   219136              
  Branches    41987    41985       -2     
==========================================
- Hits       196404   196373      -31     
- Misses      14611    14656      +45     
+ Partials     8121     8107      -14     

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

... and 32 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.

[panva]

@ChALkeR please align the first commit message with that of the PR

[ChALkeR]

@panva done, also squashed.

[panva]

Please note that i'm not sure having an agent author your PRs is valid per #62577

[ChALkeR]

@panva The commit is signed off by me, not by the agent.
This is not just a text label, it had to go trough me manually approving that to be filed.
I do not auto-add signed-off-by without looking.

(Also technically it wasn't PRd by an agent, it was PRd by a simple script after approval, the agent is isolated in a VM and only produces a patch and does not have credentials to commit it, it was long terminated together with the VM at the commit time).

As for an agent - it is the main co-author, and I think that labelling that explicitly instead of hiding it is valuable.

[nodejs-github-bot]

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

[panva]

@ChALkeR

Refs: [deepview/758ca4a73c], {deepview-autofix/deepview/758ca4a73c}, v3.0.0-34631-gb1f8d08a1c1
Author:     DeepView Autofix <276251120+deepview-autofix@users.noreply.github.com>

The name and email used in the Signed-off-by trailer must match the name and email used in the commit author metadata.

[panva]

As for an agent - it is the main co-author, and I think that labelling that explicitly instead of hiding it is valuable.

Assisted-by would be a way to do so.

[ChALkeR]

The name and email used in the Signed-off-by trailer must match the name and email used in the commit author metadata.

... that's not how signed-off-by is supposed to work.
It should be authoritative even if it mismatches the commit author.

Compare for example with DCO: https://developercertificate.org/

Commented: #62577 (comment)

[ChALkeR]

@panva Assisted-by is a threshold, I think? This is more like Generated-by. Having a policy would be nice before using non-standard labels.

This is the setup:

1. The scanner that detected this and the non-ai script that commited this (after manual approval) is one project
2. The fix itself was done in a VM by Claude (terminated before the commit)
3. I have overseen both the scanner results and the commit (and made adjustments in this case, but not in some others)
4. Sign-off and PR steps were separately approved manually

I do not think that (3) makes me the main author.
This is mostly generated, and I want to make that visible.

[panva]

@ChALkeR that's for a discussion elsewhere I believe. But note that this discussion applies to all the PRs opened by @deepview-autofix today.

[ChALkeR]

@panva Yes, they all shared this same setup. Thanks for raising, let's continue in #62577

[panva]

@ChALkeR I am also uncertain how this would appear in a changelog, I belive it could result in contributions by DeepView Autofix instead of you. @aduh95 can you confirm?

[ChALkeR]

I belive it could result in contributions by DeepView Autofix instead of you

That is my intention.

It is my pipeline that auto-detects logic mistakes and then (upon manual confirmation) feeds them to the autofixer-in-vm.

But the original code issue is auto-detected.
The main contribution here is detecting the code bug.

I think it should be explicit in the changelog and that preserving that in the record has value.

[panva]

That is my intention.

That may be the case but the guidelines that may (or may not?) end up being adopted for AI contribution explicitly don't align with this.

Pull requests must not be opened by automated tooling not specifically approved in advance by the project.

#62105

[ChALkeR]

Pull requests must not be opened by automated tooling not specifically approved in advance by the project.

The pull request tooling is manual in that sense.

(This also happens after the patch review step, which is separate and is prior to this.)

[panva]

The project has no way to discern or confirm that, neither do the reviewers. I think for now it's better if the PRs and their commits come authored by you and attribute your tooling with Assisted-by trailers.

[ChALkeR]

The project has no way to discern or confirm that,

I can confirm that (and Signed-off-by explicitly means that).
It, by spirit (and DCO), can't be added automatically without manual human review.

Without trusting contributors the project has no way to confirm anything at all.

These lines are more relevant though: https://github.com/nodejs/node/pull/62105/files#diff-042367f42be04fc3f4bcf7442f7a24183e207dc1ab552492f8a528904dd5d770R19-R21

Hm.
Upd: left a comment: https://github.com/nodejs/node/pull/62105/files#r3110451824

[ChALkeR]

Hm.

The OpenJSF policy (at https://openjsf.cdn.prismic.io/openjsf/aca4d5GXnQHGZDiZ_OpenJS_AI_Coding_Assistants_Policy.pdf) specifies Assisted-by for AI assistants indeed.

So, per policy, at least the Claude line should be Assisted-by: Claude

Unsure about the deepview-autofix line.
It is not an AI agent.

It's a pipeline: detection -> confirmation -> agent in vm (but that's the Claude line) generating a fix -> confirmation -> commit -> confirmation -> PR.

I'll try to think of a better way to signal that...

[ChALkeR]

@panva For now, until the policies are finalized, I'll manually switch to this format:

Author: Human
Assisted-by: Claude
Co-Authored-By: Claude
Co-Authored-By: Bot for detection/pipeline
Co-Authored-By: Human
Signed-off-by: Human

This is indeed reasonable as the whole pipeline is manually-reviewed at many points.
In future, I hope moving towards more visibility on the fact that the fix is mostly auto-detected and auto-generated

Wdyt?

[panva]

Wouldn't that at odds with this?

If a commit has multiple authors, there should be one Signed-off-by for each author. One of those must match the author metadata for the commit.

And non-humans can't have a signed-off-by, that would mean all your assistants should be assisted-by. Which leaves:

Author: Human
Assisted-by: Claude
Assisted-by: Bot for detection/pipeline
Signed-off-by: Human

I see the points behind your motivation (or the motivation behind your points) for attributing the tooling but from the POV of the project we accept/look for contributions from a Human, not AI. Or the human babysitting the AI.

[ChALkeR]

@panva good catch. #62577 (comment)

(I don't think that this should create an incentive to obscure code origin and remove co-authored-by from agents).

[ChALkeR]

for attributing the tooling

This is more about transparency than attribution.
I don't care about attributing Claude.
I care about making it visible that code was generated.

Assisted-by: is not parsed by GitHub and is not immediately visible in the UI unlike co-authored-by

Only assisted-by:

Both assisted-by and co-authored-by:

The OpenJSF policy does not prohibit using both.