Skip to content

crypto: use user-facing error for output encoding changes#64692

Merged
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
Archkon:crypto
Jul 25, 2026
Merged

crypto: use user-facing error for output encoding changes#64692
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
Archkon:crypto

Conversation

@Archkon

@Archkon Archkon commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

Fixes: #64689

Replace the internal assertion with ERR_INVALID_ARG_VALUE when the
output encoding changes between Cipher or Decipher calls. Document the
encoding requirement and update the corresponding tests.

Signed-off-by: Archkon <180910180+Archkon@users.noreply.github.com>
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/crypto

@nodejs-github-bot nodejs-github-bot added crypto Issues and PRs related to the crypto subsystem. needs-ci PRs that need a full CI run. labels Jul 23, 2026
@panva

panva commented Jul 23, 2026

Copy link
Copy Markdown
Member

Technically a semver-major PRs that contain breaking changes and should be released in the next major version. but i'm inclined to say it's acceptable.

@Renegade334

Copy link
Copy Markdown
Member

Intermittent GHA outages, it's going to be a fun day...

@Renegade334 Renegade334 reopened this Jul 23, 2026
@Renegade334

Copy link
Copy Markdown
Member

Looks like this will need a push of some description to (hopefully) trigger the workflows.

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

LGTM

@codecov

codecov Bot commented Jul 23, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.11%. Comparing base (0992f6b) to head (b13ea18).
⚠️ Report is 38 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #64692      +/-   ##
==========================================
- Coverage   90.14%   90.11%   -0.03%     
==========================================
  Files         741      741              
  Lines      242112   242114       +2     
  Branches    45605    45601       -4     
==========================================
- Hits       218244   218190      -54     
- Misses      15360    15430      +70     
+ Partials     8508     8494      -14     
Files with missing lines Coverage Δ
lib/internal/crypto/cipher.js 98.00% <100.00%> (+0.01%) ⬆️

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

@pimterry pimterry added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 23, 2026
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 23, 2026
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@jasnell

jasnell commented Jul 23, 2026

Copy link
Copy Markdown
Member

I'm also fine with this not being semver-major.

@Renegade334 Renegade334 added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jul 23, 2026
@Archkon

Archkon commented Jul 24, 2026

Copy link
Copy Markdown
Contributor Author

Could some collaborator review what happend for these failed tests ?

@avivkeller avivkeller added the commit-queue Add this label to land a pull request using GitHub Actions. label Jul 24, 2026
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@Archkon

Archkon commented Jul 25, 2026

Copy link
Copy Markdown
Contributor Author

@lpinca @Renegade334 @avivkeller Could review this failed test again ? Would this prevent this pr from landing ?

@avivkeller

Copy link
Copy Markdown
Member

It's flaky, see #64718

@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jul 25, 2026
@nodejs-github-bot nodejs-github-bot added the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Jul 25, 2026
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator
Commit Queue failed
- Loading data for nodejs/node/pull/64692
✔  Done loading data for nodejs/node/pull/64692
----------------------------------- PR info ------------------------------------
Title      crypto: use user-facing error for output encoding changes (#64692)
   ⚠  Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch     Archkon:crypto -> nodejs:main
Labels     crypto, author ready, needs-ci
Commits    1
 - crypto: use user-facing error for output encoding changes
Committers 1
 - Archkon <180910180+Archkon@users.noreply.github.com>
PR-URL: https://github.com/nodejs/node/pull/64692
Fixes: https://github.com/nodejs/node/issues/64689
Reviewed-By: René <contact.9a5d6388@renegade334.me.uk>
Reviewed-By: Tim Perry <pimterry@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Aviv Keller <me@aviv.sh>
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/64692
Fixes: https://github.com/nodejs/node/issues/64689
Reviewed-By: René <contact.9a5d6388@renegade334.me.uk>
Reviewed-By: Tim Perry <pimterry@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Aviv Keller <me@aviv.sh>
--------------------------------------------------------------------------------
   ℹ  This PR was created on Thu, 23 Jul 2026 07:27:29 GMT
   ✔  Approvals: 6
   ✔  - René (@Renegade334): https://github.com/nodejs/node/pull/64692#pullrequestreview-4763043896
   ✔  - Tim Perry (@pimterry): https://github.com/nodejs/node/pull/64692#pullrequestreview-4763710773
   ✔  - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/64692#pullrequestreview-4763984060
   ✔  - Filip Skokan (@panva) (TSC): https://github.com/nodejs/node/pull/64692#pullrequestreview-4764391687
   ✔  - Luigi Pinca (@lpinca): https://github.com/nodejs/node/pull/64692#pullrequestreview-4767882556
   ✔  - Aviv Keller (@avivkeller): https://github.com/nodejs/node/pull/64692#pullrequestreview-4770448677
   ✘  GitHub CI failed with status: FAILURE
   ℹ  Last Full PR CI on 2026-07-24T11:38:33Z: https://ci.nodejs.org/job/node-test-pull-request/75091/
- Querying data for job/node-test-pull-request/75091/
✔  Build data downloaded
- Querying failures of job/node-test-commit/89767/
✔  Data downloaded
   ✘  4 failure(s) on the last Jenkins CI run
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu
https://github.com/nodejs/node/actions/runs/30149409066

@Archkon

Archkon commented Jul 25, 2026

Copy link
Copy Markdown
Contributor Author

It's flaky, see #64718

@avivkeller I know but this would prevent this pr form being landed by nodesj github bot would you like to rerun jenkins ci again ?

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@Renegade334 Renegade334 added commit-queue Add this label to land a pull request using GitHub Actions. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Jul 25, 2026
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jul 25, 2026
@nodejs-github-bot
nodejs-github-bot merged commit 6af3dda into nodejs:main Jul 25, 2026
82 checks passed
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Landed in 6af3dda

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author ready PRs that have at least one approval, no pending requests for changes, and a CI started. crypto Issues and PRs related to the crypto subsystem. needs-ci PRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Passing different encodings to Cipheriv updates results in ERR_INTERNAL_ASSERTION

8 participants