Skip to content

crypto: add cipher update/final methods encoding validation - #45990

Merged
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
vitpavlenko:bugfix/45189-encoding-validation
Jan 17, 2023
Merged

crypto: add cipher update/final methods encoding validation#45990
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
vitpavlenko:bugfix/45189-encoding-validation

Conversation

@vitpavlenko

@vitpavlenkovitpavlenko commented Dec 27, 2022

Copy link
Copy Markdown
Contributor

Adds encoding validation to update and final cipher methods.

Refs: #45189

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/crypto

@nodejs-github-botnodejs-github-bot added crypto Issues and PRs related to the crypto subsystem. doc Issues and PRs related to the documentations. errors Issues and PRs related to JavaScript errors originated in Node.js core. needs-ci PRs that need a full CI run. labels Dec 27, 2022
VoltrexKeyva
VoltrexKeyva previously requested changes Dec 28, 2022
Comment threadREADME.md Outdated
Comment threadlib/internal/crypto/cipher.js Outdated
Comment threadlib/internal/validators.js Outdated
@vitpavlenko
vitpavlenkoforce-pushed the bugfix/45189-encoding-validation branch from eba9755 to 9bbecffCompareDecember 28, 2022 12:16
@VoltrexKeyva
VoltrexKeyva dismissed their stale reviewDecember 28, 2022 14:40

Changes addressed

@vitpavlenko
vitpavlenkoforce-pushed the bugfix/45189-encoding-validation branch from 9bbecff to 449cc81CompareDecember 28, 2022 16:34
@vitpavlenko

vitpavlenko commented Dec 28, 2022

Copy link
Copy Markdown
ContributorAuthor

Sorry, some-how missed js lint error in test. Fixed it, also updated first commit message. Please approve run @VoltrexKeyva

@vitpavlenko

Copy link
Copy Markdown
ContributorAuthor

@VoltrexKeyva any chance to approve?)

@panvapanva added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 1, 2023
Comment threadtest/parallel/test-crypto-encoding-validation-error.js Outdated
Comment threadtest/parallel/test-crypto-encoding-validation-error.js Outdated
Comment threadtest/parallel/test-crypto-encoding-validation-error.js Outdated
@panvapanva removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 1, 2023
@panva
panva requested a review from targosJanuary 1, 2023 16:43
@panvapanva changed the title cipher: add cipher update/final methods encoding validationcrypto: add cipher update/final methods encoding validationJan 1, 2023
@panvapanva added the commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. label Jan 1, 2023
@vitpavlenko
vitpavlenkoforce-pushed the bugfix/45189-encoding-validation branch from 449cc81 to 1853424CompareJanuary 1, 2023 17:20
@panvapanva added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 1, 2023
@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 1, 2023
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

panva
panva previously requested changes Jan 6, 2023

@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.

We can do perfectly fine without a new error. Here's a combined diff of my suggested changes.

@panvapanva removed the commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. label Jan 12, 2023
@panva
panva dismissed their stale reviewJanuary 12, 2023 17:55

feedback applied

@panva
panvaforce-pushed the bugfix/45189-encoding-validation branch from 47f8bc3 to ca20150CompareJanuary 12, 2023 20:32
@panvapanva added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. and removed doc Issues and PRs related to the documentations. labels Jan 13, 2023
@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 13, 2023
@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

if (normalizedEncoding === undefined) {
throw new ERR_UNKNOWN_ENCODING(encoding);
}
assert(false, 'Cannot change encoding');

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

why throw error by assert? it will be some internal error, no?

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.

We can replace the assert with a coded error in a follow up semver-majorPRs that contain breaking changes and should be released in the next major version. PR.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I think it is better to have a coded error, can I make the change? @panva

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.

Sure, best wait until this change lands.

@panvapanva added the commit-queue Add this label to land a pull request using GitHub Actions. label Jan 17, 2023
@nodejs-github-botnodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jan 17, 2023
@nodejs-github-bot
nodejs-github-bot merged commit 5a7d4a7 into nodejs:mainJan 17, 2023
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Landed in 5a7d4a7

RafaelGSS pushed a commit that referenced this pull request Jan 20, 2023
Refs #45189
PR-URL: #45990
Refs: #45189
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
@RafaelGSSRafaelGSS mentioned this pull request Jan 20, 2023
juanarbol pushed a commit that referenced this pull request Jan 26, 2023
Refs #45189
PR-URL: #45990
Refs: #45189
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
@juanarboljuanarbol mentioned this pull request Jan 28, 2023
juanarbol pushed a commit that referenced this pull request Jan 31, 2023
Refs #45189
PR-URL: #45990
Refs: #45189
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.cryptoIssues and PRs related to the crypto subsystem.errorsIssues and PRs related to JavaScript errors originated in Node.js core.needs-ciPRs that need a full CI run.review wantedPRs that need reviews.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@vitpavlenko@nodejs-github-bot@panva@jasnell@VoltrexKeyva