Skip to content

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

Open
Archkon wants to merge 1 commit into
nodejs:mainfrom
Archkon:crypto
Open

crypto: use user-facing error for output encoding changes#64692
Archkon wants to merge 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 6 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
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

6 participants