Skip to content

tls: make Server.getTicketKeys throw instead of assert#58365

Open
nektro wants to merge 1 commit into
nodejs:mainfrom
nektro:nektro-patch-40171
Open

tls: make Server.getTicketKeys throw instead of assert#58365
nektro wants to merge 1 commit into
nodejs:mainfrom
nektro:nektro-patch-40171

Conversation

@nektro

@nektro nektro commented May 16, 2025

Copy link
Copy Markdown
Contributor

assert is for invariants and prints a message about the condition being a bug in Node.js if false.
throw ERR_INVALID_ARG_VALUE for publicly facing validation.
this brings it into sync with the validation done in configSecureContext.

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/crypto
  • @nodejs/net

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. tls Issues and PRs related to the tls subsystem. labels May 16, 2025
@codecov

codecov Bot commented May 16, 2025

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.22%. Comparing base (34c28d5) to head (43a4f4e).

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #58365      +/-   ##
==========================================
- Coverage   90.23%   90.22%   -0.01%     
==========================================
  Files         739      739              
  Lines      241639   241640       +1     
  Branches    45547    45535      -12     
==========================================
- Hits       218031   218030       -1     
- Misses      15152    15153       +1     
- Partials     8456     8457       +1     
Files with missing lines Coverage Δ
lib/internal/tls/wrap.js 95.10% <100.00%> (+<0.01%) ⬆️

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

@mertcanaltin

Copy link
Copy Markdown
Member

The linter pipeline section seems to be broken.

@nektro

nektro commented May 21, 2025

Copy link
Copy Markdown
Contributor Author

anything i need to do on my end?

@nektro nektro force-pushed the nektro-patch-40171 branch 5 times, most recently from 19a54a8 to b2dd7ec Compare August 19, 2025 07:19

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

The PR is updatingServer.setTicketKeys but the commit message talks about Server.getTicketKeys instead. Can you update the commit message?

Comment thread lib/internal/tls/wrap.js
assert(keys.byteLength === 48,
'Session ticket keys must be a 48-byte buffer');
if (keys.byteLength !== 48) {
throw new ERR_INVALID_ARG_VALUE('keys', keys.byteLength, 'must be exactly 48 bytes');

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.

Suggested change
throw new ERR_INVALID_ARG_VALUE('keys', keys.byteLength, 'must be exactly 48 bytes');
throw new ERR_INVALID_ARG_VALUE('keys.byteLength', keys.byteLength, 'must be 48 bytes');

The original message looks a bit ambiguous to me by saying "received N" (but N=buffer.byteLength isn't the argument. The buffer is).

Comment thread lib/internal/tls/wrap.js


Server.prototype.setTicketKeys = function setTicketKeys(keys) {
validateBuffer(keys);

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.

Suggested change
validateBuffer(keys);
validateBuffer(keys, 'keys');

This predated the PR but since it's adding a test for the message, let's align the two names.

{
name: 'TypeError',
code: 'ERR_INVALID_ARG_TYPE',
message: 'The "buffer" argument must be an instance of Buffer, TypedArray, or DataView.' +

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.

Suggested change
message: 'The "buffer" argument must be an instance of Buffer, TypedArray, or DataView.' +
message: 'The "keys" argument must be an instance of Buffer, TypedArray, or DataView.' +

{
name: 'TypeError',
code: 'ERR_INVALID_ARG_VALUE',
message: `The argument 'keys' must be exactly 48 bytes. Received ${arg.byteLength}`,

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.

Suggested change
message: `The argument 'keys' must be exactly 48 bytes. Received ${arg.byteLength}`,
message: `The property 'keys.byteLength' must be exactly 48 bytes. Received ${arg.byteLength}`,

@github-actions

Copy link
Copy Markdown
Contributor

This pull request has been marked as stale due to 210 days of inactivity.
It will be automatically closed in 30 days if no further activity occurs. If this is still relevant, please leave a comment or update it to keep it open.

@github-actions github-actions Bot added stale and removed stale labels Apr 19, 2026
@Ethan-Arrowood

Copy link
Copy Markdown
Contributor

Hello, I'm doing some backlog hygiene on all PRs related to networking and streaming modules. There hasn't been any activity here for quite a while so I'm going to mark it stalled. It will auto-close in ~30 days. This is not a rejection; if you've like to pick it back up, rebase onto latest main and give me a ping.

Thank you!

@Ethan-Arrowood Ethan-Arrowood added the stalled Issues and PRs that are stalled. label Jul 15, 2026
@github-actions

Copy link
Copy Markdown
Contributor

This issue/PR was marked as stalled, it will be automatically closed in 30 days. If it should remain open, please leave a comment explaining why it should remain open.

@nektro nektro force-pushed the nektro-patch-40171 branch from b2dd7ec to 48174e6 Compare July 15, 2026 19:13
assert is for invariants and print a message about the condition being a
bug in Node.js if false. throw ERR_INVALID_ARG_VALUE for publicly facing
validation.

Signed-off-by: Meghan Denny <hello@nektro.net>
@nektro nektro force-pushed the nektro-patch-40171 branch from 48174e6 to 43a4f4e Compare July 15, 2026 19:16
@github-actions github-actions Bot removed the stalled Issues and PRs that are stalled. label Jul 16, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-ci PRs that need a full CI run. tls Issues and PRs related to the tls subsystem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants