tls: throw on invalid ALPNProtocols instead of aborting

[sankalpsthakur]

Fixes #65069

tls.connect() with malformed ALPNProtocols (e.g. [''] or invalid wire format buffers like Buffer.from([0])) aborts the process with SIGABRT. The failure is CHECK_EQ(0, SSL_set_alpn_protos(...)) in TLSWrap::SetALPNProtocols when OpenSSL rejects the malformed list.

This change replaces the hard abort with THROW_ERR_INVALID_ARG_VALUE so invalid input throws a recoverable JavaScript exception. Valid inputs are unchanged; e.g. ['h2','http/1.1'] and wire buffers produced by convertProtocols continue to work.

Repro: node -e "require('node:tls').connect({ host: '127.0.0.1', port: 9999, ALPNProtocols: [''] })"

Before: SIGABRT exit 134
After: throws ERR_INVALID_ARG_VALUE

Assisted-by: Codex

[nodejs-github-bot]

Review requested:

• @nodejs/crypto

[pimterry]

Thanks for looking at this @sankalpsthakur! I agree it'd be good to fix, but I think there's a different approach that would cover the server case too and be a bit cleaner.

We do also need a test covering this, either as a new test or extending any existing validation tests we have here.

[sankalpsthakur]

Updated per review: validation now in lib/tls.js convertALPNProtocols (covers client and server, fails early), C++ defense kept, added test covering [''] and malformed buffers (Buffer.from([0]), [2,0x61], [1,0x61,0x62]). Ready for re-review.

[sankalpsthakur]

Thanks for the feedback. I moved validation into convertALPNProtocols so both client and server fail early before reaching OpenSSL, and kept the C++ check as a defense in depth. Empty string protocols now throw via convertProtocols and wire format buffers are validated for empty, zero length and truncated cases. Added test coverage for the repro cases from #65069. I fixed the lint and formatting issues as well and rebased onto latest main.

[sankalpsthakur]

@pimterry empty [] / empty buffer allowed again (skip ALPN). C++ is CHECK_EQ again, no user throw there. still reject [''] and bad wire buffers in convertALPNProtocols. 6e497bb.

[sankalpsthakur]

@pimterry empty ALPN skip, convertALPNProtocols validation, CHECK_EQ, and tests are on latest head. Ready when you have a moment.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/75694/

[sankalpsthakur]

CI job 75694 ran on 6e497bb before the fixture fix. That commit still used invalid ALPN bytes (abcd) in test-tls-basic-validations.js, so the reds there are expected.

193e77b uses valid length-prefixed fixtures (same copy-isolation check). Could someone re-run CI on current HEAD? Thanks.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.30%. Comparing base (ee64033) to head (193e77b).
⚠️ Report is 184 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff            @@
##             main   #65076    +/-   ##
========================================
  Coverage   90.29%   90.30%            
========================================
  Files         759      759            
  Lines      247754   248355   +601     
  Branches    46721    46857   +136     
========================================
+ Hits       223708   224270   +562     
+ Misses      15521    15514     -7     
- Partials     8525     8571    +46     

Files with missing lines	Coverage Δ
lib/tls.js	93.47% <100.00%> (+0.33%)	⬆️

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

[trivikr]

@sankalpsthakur Can you please rebase?

The merge commits don't work with our tooling.

[sankalpsthakur]

rebased onto main (a48e33f), merge commit dropped, six commits linear now. diff is unchanged: lib/tls.js and the two tls tests.

[sankalpsthakur]

correction to my last note: squashed to a single commit (eaaedf8) rather than the six i mentioned, message rewritten to describe the final js-only validation since the earlier ones still described the reverted c++ change. diff is unchanged.