Skip to content

Fix crypto-rsa code - #57572

Closed
AmitPrajapati-1 wants to merge 4 commits into
nodejs:mainfrom
AmitPrajapati-1:fix-crypto-code-amit
Closed

Fix crypto-rsa code #57572
AmitPrajapati-1 wants to merge 4 commits into
nodejs:mainfrom
AmitPrajapati-1:fix-crypto-code-amit

Conversation

@AmitPrajapati-1

@AmitPrajapati-1AmitPrajapati-1 commented Mar 21, 2025

Copy link
Copy Markdown

No description provided.

@nodejs-github-bot

nodejs-github-bot commented Mar 21, 2025

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/crypto

@nodejs-github-botnodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem. needs-ci PRs that need a full CI run. labels Mar 21, 2025
This was referenced Mar 21, 2025
Closed
Closed
panva
panva requested changes Mar 21, 2025
View reviewed changes

@panvapanva left a comment •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does not fix the underlying issue introduced somewhere in the ncrypto refactors. Checking that all bytes are 0 and then returning a zero-byte buffer is not a fix to the issue and it introduces more edge cases than it solves. i.e. an n-byte empty buffer will now always return 0-byte buffer.

#57553 (comment)

aduh95
aduh95 reviewed Mar 21, 2025
View reviewed changes
Comment threadpackage-lock.json

@aduh95aduh95Mar 21, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please remove this?

aduh95
aduh95 reviewed Mar 21, 2025
View reviewed changes
Comment threadtest/parallel/package-lock.json

@aduh95aduh95Mar 21, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please remove this?

@AmitPrajapati-1

AmitPrajapati-1 commented Mar 21, 2025

Copy link
Copy Markdown
Author

rather than converting empty string. we can Throwing appropriate errors with meaningful messages instead of trying to handle empty strings by returning zero-byte buffers.

panva
panva requested changes Mar 21, 2025
View reviewed changes

@panvapanva left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My original review stands.

panva added a commit to panva/node that referenced this pull request Mar 21, 2025
@panva
crypto: fix output of privateDecrypt with zero-length data
057f4c5
closesnodejs#57553closesnodejs#57572closesnodejs#57558
@panvapanva mentioned this pull request Mar 21, 2025
Merged
@panva

panva commented Mar 21, 2025

Copy link
Copy Markdown
Member

Fix in #57575

@panvapanva closed this Mar 31, 2025
nodejs-github-bot pushed a commit that referenced this pull request Apr 1, 2025
@panva
crypto: fix output of privateDecrypt with zero-length data
12b81df
closes#57553closes#57572closes#57558
PR-URL: #57575
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
aduh95 pushed a commit that referenced this pull request Apr 1, 2025
@panva@aduh95
crypto: fix output of privateDecrypt with zero-length data
7611fc1
closes#57553closes#57572closes#57558
PR-URL: #57575
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
RafaelGSS pushed a commit that referenced this pull request May 1, 2025
@panva@RafaelGSS
crypto: fix output of privateDecrypt with zero-length data
33e4380
closes#57553closes#57572closes#57558
PR-URL: #57575
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
RafaelGSS pushed a commit that referenced this pull request May 2, 2025
@panva@RafaelGSS
crypto: fix output of privateDecrypt with zero-length data
913a283
closes#57553closes#57572closes#57558
PR-URL: #57575
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@aduh95aduh95aduh95 left review comments
@panvapanvapanva requested changes

Assignees

No one assigned

Labels

c++Issues and PRs that require attention from people who are familiar with C++.cryptoIssues and PRs related to the crypto subsystem.needs-ciPRs that need a full CI run.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@AmitPrajapati-1@nodejs-github-bot@panva@aduh95