tls: throw on invalid ALPNProtocols instead of aborting by sankalpsthakur · Pull Request #65076 · nodejs/node

sankalpsthakur · 2026-08-06T09:40:43Z

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 · 2026-08-06T09:40:47Z

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.

Address review feedback on nodejs#65076: move validation closer to call site in convertALPNProtocols so both client and server are covered and fail early. Empty string protocols now throw before reaching OpenSSL; wire-format buffers are validated for truncated/zero-length entries. C++ defense (THROW_ERR_INVALID_ARG_VALUE) remains for safety. Refs: nodejs#65076

Refs: nodejs#65069 PR-URL: nodejs#65076

sankalpsthakur · 2026-08-06T10:48:18Z

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.

Address review feedback on nodejs#65076: move validation closer to call site in convertALPNProtocols so both client and server are covered and fail early. Empty string protocols now throw before reaching OpenSSL; wire-format buffers are validated for truncated/zero-length entries. C++ defense (THROW_ERR_INVALID_ARG_VALUE) remains for safety. Refs: nodejs#65076

Refs: nodejs#65069 PR-URL: nodejs#65076

sankalpsthakur · 2026-08-06T12:43:17Z

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 · 2026-08-07T17:39:26Z

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

Empty ALPN buffer/array means skip ALPN (same as historical behavior for []). Zero-length protocol entries and malformed wire buffers still throw from convertALPNProtocols. Revert the C++ THROW_ERR_INVALID_ARG_VALUE back to CHECK_EQ: after JS validation, a non-zero SSL_set_alpn_protos return is an internal invariant failure, not user-facing input. Refs: nodejs#65076

sankalpsthakur · 2026-08-08T05:02:39Z

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

nodejs-github-bot · 2026-08-09T14:02:15Z

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

sankalpsthakur · 2026-08-09T15:26:45Z

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 · 2026-08-09T22:37:13Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.14%. Comparing base (a48e33f) to head (e074e05).
⚠️ Report is 54 commits behind head on main.

Additional details and impacted files

@@ Coverage Diff @@## main #65076 +/- ##
==========================================
+ Coverage 90.12% 90.14% +0.01% 
==========================================
Files 751 751 Lines 252425 252625 +200 Branches 47471 47542 +71 ==========================================
+ Hits 227504 227724 +220 + Misses 16216 16200 -16 + Partials 8705 8701 -4

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

... and 56 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 · 2026-08-22T03:05:52Z

@sankalpsthakur Can you please rebase?

The merge commits don't work with our tooling.

Address review feedback on nodejs#65076: move validation closer to call site in convertALPNProtocols so both client and server are covered and fail early. Empty string protocols now throw before reaching OpenSSL; wire-format buffers are validated for truncated/zero-length entries. C++ defense (THROW_ERR_INVALID_ARG_VALUE) remains for safety. Refs: nodejs#65076

Refs: nodejs#65069 PR-URL: nodejs#65076

Empty ALPN buffer/array means skip ALPN (same as historical behavior for []). Zero-length protocol entries and malformed wire buffers still throw from convertALPNProtocols. Revert the C++ THROW_ERR_INVALID_ARG_VALUE back to CHECK_EQ: after JS validation, a non-zero SSL_set_alpn_protos return is an internal invariant failure, not user-facing input. Refs: nodejs#65076

sankalpsthakur · 2026-08-22T07:21:44Z

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

sankalpsthakur · 2026-08-22T07:25:58Z

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.

trivikr · 2026-08-22T17:15:41Z

@sankalpsthakur Can you please sign the commit?

It's required as per instructions

Your commit must contain the Signed-off-by line with your name and email address as an acknowledgement that you agree to the Developer Certificate of Origin.

tls.connect() with malformed ALPNProtocols hit CHECK_EQ(0, SSL_set_alpn_protos(...)) in the C++ layer and aborted the process with SIGABRT. Validate in JS instead, in convertALPNProtocols, so both client and server fail early with a recoverable ERR_INVALID_ARG_VALUE: - zero-length string protocols now throw from convertProtocols - wire-format buffers are checked for zero-length and truncated entries - an empty buffer or array is still accepted and means skip ALPN, matching the historical behavior for [] The C++ CHECK_EQ is left unchanged: once JS has validated the input, a non-zero SSL_set_alpn_protos return is an internal invariant failure rather than user-facing input. Fixes: nodejs#65069 Signed-off-by: Sankalp Thakur <sankalphimself@gmail.com>

sankalpsthakur · 2026-08-23T04:59:15Z

Signed-off-by is on e074e05, author Sankalp Thakur sankalphimself@gmail.com.

nodejs-github-bot · 2026-08-23T17:34:07Z

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

nodejs-github-bot · 2026-08-24T11:36:28Z

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

nodejs-github-bot · 2026-08-24T14:31:23Z

Landed in 278ead1

tls.connect() with malformed ALPNProtocols hit CHECK_EQ(0, SSL_set_alpn_protos(...)) in the C++ layer and aborted the process with SIGABRT. Validate in JS instead, in convertALPNProtocols, so both client and server fail early with a recoverable ERR_INVALID_ARG_VALUE: - zero-length string protocols now throw from convertProtocols - wire-format buffers are checked for zero-length and truncated entries - an empty buffer or array is still accepted and means skip ALPN, matching the historical behavior for [] The C++ CHECK_EQ is left unchanged: once JS has validated the input, a non-zero SSL_set_alpn_protos return is an internal invariant failure rather than user-facing input. Fixes: #65069 Signed-off-by: Sankalp Thakur <sankalphimself@gmail.com> PR-URL: #65076 Reviewed-By: Tim Perry <pimterry@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>

sankalpsthakur · 2026-08-06T09:40:43Z

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 · 2026-08-06T09:40:47Z

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.

Address review feedback on nodejs#65076: move validation closer to call site in convertALPNProtocols so both client and server are covered and fail early. Empty string protocols now throw before reaching OpenSSL; wire-format buffers are validated for truncated/zero-length entries. C++ defense (THROW_ERR_INVALID_ARG_VALUE) remains for safety. Refs: nodejs#65076

Refs: nodejs#65069 PR-URL: nodejs#65076

sankalpsthakur · 2026-08-06T10:48:18Z

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.

Address review feedback on nodejs#65076: move validation closer to call site in convertALPNProtocols so both client and server are covered and fail early. Empty string protocols now throw before reaching OpenSSL; wire-format buffers are validated for truncated/zero-length entries. C++ defense (THROW_ERR_INVALID_ARG_VALUE) remains for safety. Refs: nodejs#65076

Refs: nodejs#65069 PR-URL: nodejs#65076

sankalpsthakur · 2026-08-06T12:43:17Z

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 · 2026-08-07T17:39:26Z

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

Empty ALPN buffer/array means skip ALPN (same as historical behavior for []). Zero-length protocol entries and malformed wire buffers still throw from convertALPNProtocols. Revert the C++ THROW_ERR_INVALID_ARG_VALUE back to CHECK_EQ: after JS validation, a non-zero SSL_set_alpn_protos return is an internal invariant failure, not user-facing input. Refs: nodejs#65076

sankalpsthakur · 2026-08-08T05:02:39Z

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

nodejs-github-bot · 2026-08-09T14:02:15Z

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

sankalpsthakur · 2026-08-09T15:26:45Z

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 · 2026-08-09T22:37:13Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.14%. Comparing base (a48e33f) to head (e074e05).
⚠️ Report is 54 commits behind head on main.

Additional details and impacted files

@@ Coverage Diff @@## main #65076 +/- ##
==========================================
+ Coverage 90.12% 90.14% +0.01% 
==========================================
Files 751 751 Lines 252425 252625 +200 Branches 47471 47542 +71 ==========================================
+ Hits 227504 227724 +220 + Misses 16216 16200 -16 + Partials 8705 8701 -4

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

... and 56 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 · 2026-08-22T03:05:52Z

@sankalpsthakur Can you please rebase?

The merge commits don't work with our tooling.

Address review feedback on nodejs#65076: move validation closer to call site in convertALPNProtocols so both client and server are covered and fail early. Empty string protocols now throw before reaching OpenSSL; wire-format buffers are validated for truncated/zero-length entries. C++ defense (THROW_ERR_INVALID_ARG_VALUE) remains for safety. Refs: nodejs#65076

Refs: nodejs#65069 PR-URL: nodejs#65076

Empty ALPN buffer/array means skip ALPN (same as historical behavior for []). Zero-length protocol entries and malformed wire buffers still throw from convertALPNProtocols. Revert the C++ THROW_ERR_INVALID_ARG_VALUE back to CHECK_EQ: after JS validation, a non-zero SSL_set_alpn_protos return is an internal invariant failure, not user-facing input. Refs: nodejs#65076

sankalpsthakur · 2026-08-22T07:21:44Z

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

sankalpsthakur · 2026-08-22T07:25:58Z

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.

trivikr · 2026-08-22T17:15:41Z

@sankalpsthakur Can you please sign the commit?

It's required as per instructions

Your commit must contain the Signed-off-by line with your name and email address as an acknowledgement that you agree to the Developer Certificate of Origin.

tls.connect() with malformed ALPNProtocols hit CHECK_EQ(0, SSL_set_alpn_protos(...)) in the C++ layer and aborted the process with SIGABRT. Validate in JS instead, in convertALPNProtocols, so both client and server fail early with a recoverable ERR_INVALID_ARG_VALUE: - zero-length string protocols now throw from convertProtocols - wire-format buffers are checked for zero-length and truncated entries - an empty buffer or array is still accepted and means skip ALPN, matching the historical behavior for [] The C++ CHECK_EQ is left unchanged: once JS has validated the input, a non-zero SSL_set_alpn_protos return is an internal invariant failure rather than user-facing input. Fixes: nodejs#65069 Signed-off-by: Sankalp Thakur <sankalphimself@gmail.com>

sankalpsthakur · 2026-08-23T04:59:15Z

Signed-off-by is on e074e05, author Sankalp Thakur sankalphimself@gmail.com.

nodejs-github-bot · 2026-08-23T17:34:07Z

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

nodejs-github-bot · 2026-08-24T11:36:28Z

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

nodejs-github-bot · 2026-08-24T14:31:23Z

Landed in 278ead1

tls.connect() with malformed ALPNProtocols hit CHECK_EQ(0, SSL_set_alpn_protos(...)) in the C++ layer and aborted the process with SIGABRT. Validate in JS instead, in convertALPNProtocols, so both client and server fail early with a recoverable ERR_INVALID_ARG_VALUE: - zero-length string protocols now throw from convertProtocols - wire-format buffers are checked for zero-length and truncated entries - an empty buffer or array is still accepted and means skip ALPN, matching the historical behavior for [] The C++ CHECK_EQ is left unchanged: once JS has validated the input, a non-zero SSL_set_alpn_protos return is an internal invariant failure rather than user-facing input. Fixes: #65069 Signed-off-by: Sankalp Thakur <sankalphimself@gmail.com> PR-URL: #65076 Reviewed-By: Tim Perry <pimterry@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>

sankalpsthakur · 2026-08-06T09:40:43Z

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 · 2026-08-06T09:40:47Z

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.

Address review feedback on nodejs#65076: move validation closer to call site in convertALPNProtocols so both client and server are covered and fail early. Empty string protocols now throw before reaching OpenSSL; wire-format buffers are validated for truncated/zero-length entries. C++ defense (THROW_ERR_INVALID_ARG_VALUE) remains for safety. Refs: nodejs#65076

Refs: nodejs#65069 PR-URL: nodejs#65076

sankalpsthakur · 2026-08-06T10:48:18Z

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.

Address review feedback on nodejs#65076: move validation closer to call site in convertALPNProtocols so both client and server are covered and fail early. Empty string protocols now throw before reaching OpenSSL; wire-format buffers are validated for truncated/zero-length entries. C++ defense (THROW_ERR_INVALID_ARG_VALUE) remains for safety. Refs: nodejs#65076

Refs: nodejs#65069 PR-URL: nodejs#65076

sankalpsthakur · 2026-08-06T12:43:17Z

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 · 2026-08-07T17:39:26Z

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

Empty ALPN buffer/array means skip ALPN (same as historical behavior for []). Zero-length protocol entries and malformed wire buffers still throw from convertALPNProtocols. Revert the C++ THROW_ERR_INVALID_ARG_VALUE back to CHECK_EQ: after JS validation, a non-zero SSL_set_alpn_protos return is an internal invariant failure, not user-facing input. Refs: nodejs#65076

sankalpsthakur · 2026-08-08T05:02:39Z

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

nodejs-github-bot · 2026-08-09T14:02:15Z

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

sankalpsthakur · 2026-08-09T15:26:45Z

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 · 2026-08-09T22:37:13Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.14%. Comparing base (a48e33f) to head (e074e05).
⚠️ Report is 54 commits behind head on main.

Additional details and impacted files

@@ Coverage Diff @@## main #65076 +/- ##
==========================================
+ Coverage 90.12% 90.14% +0.01% 
==========================================
Files 751 751 Lines 252425 252625 +200 Branches 47471 47542 +71 ==========================================
+ Hits 227504 227724 +220 + Misses 16216 16200 -16 + Partials 8705 8701 -4

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

... and 56 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 · 2026-08-22T03:05:52Z

@sankalpsthakur Can you please rebase?

The merge commits don't work with our tooling.

Address review feedback on nodejs#65076: move validation closer to call site in convertALPNProtocols so both client and server are covered and fail early. Empty string protocols now throw before reaching OpenSSL; wire-format buffers are validated for truncated/zero-length entries. C++ defense (THROW_ERR_INVALID_ARG_VALUE) remains for safety. Refs: nodejs#65076

Refs: nodejs#65069 PR-URL: nodejs#65076

Empty ALPN buffer/array means skip ALPN (same as historical behavior for []). Zero-length protocol entries and malformed wire buffers still throw from convertALPNProtocols. Revert the C++ THROW_ERR_INVALID_ARG_VALUE back to CHECK_EQ: after JS validation, a non-zero SSL_set_alpn_protos return is an internal invariant failure, not user-facing input. Refs: nodejs#65076

sankalpsthakur · 2026-08-22T07:21:44Z

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

sankalpsthakur · 2026-08-22T07:25:58Z

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.

trivikr · 2026-08-22T17:15:41Z

@sankalpsthakur Can you please sign the commit?

It's required as per instructions

Your commit must contain the Signed-off-by line with your name and email address as an acknowledgement that you agree to the Developer Certificate of Origin.

tls.connect() with malformed ALPNProtocols hit CHECK_EQ(0, SSL_set_alpn_protos(...)) in the C++ layer and aborted the process with SIGABRT. Validate in JS instead, in convertALPNProtocols, so both client and server fail early with a recoverable ERR_INVALID_ARG_VALUE: - zero-length string protocols now throw from convertProtocols - wire-format buffers are checked for zero-length and truncated entries - an empty buffer or array is still accepted and means skip ALPN, matching the historical behavior for [] The C++ CHECK_EQ is left unchanged: once JS has validated the input, a non-zero SSL_set_alpn_protos return is an internal invariant failure rather than user-facing input. Fixes: nodejs#65069 Signed-off-by: Sankalp Thakur <sankalphimself@gmail.com>

sankalpsthakur · 2026-08-23T04:59:15Z

Signed-off-by is on e074e05, author Sankalp Thakur sankalphimself@gmail.com.

nodejs-github-bot · 2026-08-23T17:34:07Z

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

nodejs-github-bot · 2026-08-24T11:36:28Z

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

nodejs-github-bot · 2026-08-24T14:31:23Z

Landed in 278ead1

tls.connect() with malformed ALPNProtocols hit CHECK_EQ(0, SSL_set_alpn_protos(...)) in the C++ layer and aborted the process with SIGABRT. Validate in JS instead, in convertALPNProtocols, so both client and server fail early with a recoverable ERR_INVALID_ARG_VALUE: - zero-length string protocols now throw from convertProtocols - wire-format buffers are checked for zero-length and truncated entries - an empty buffer or array is still accepted and means skip ALPN, matching the historical behavior for [] The C++ CHECK_EQ is left unchanged: once JS has validated the input, a non-zero SSL_set_alpn_protos return is an internal invariant failure rather than user-facing input. Fixes: #65069 Signed-off-by: Sankalp Thakur <sankalphimself@gmail.com> PR-URL: #65076 Reviewed-By: Tim Perry <pimterry@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>

sankalpsthakur · 2026-08-06T09:40:43Z

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 · 2026-08-06T09:40:47Z

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.

Address review feedback on nodejs#65076: move validation closer to call site in convertALPNProtocols so both client and server are covered and fail early. Empty string protocols now throw before reaching OpenSSL; wire-format buffers are validated for truncated/zero-length entries. C++ defense (THROW_ERR_INVALID_ARG_VALUE) remains for safety. Refs: nodejs#65076

Refs: nodejs#65069 PR-URL: nodejs#65076

sankalpsthakur · 2026-08-06T10:48:18Z

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.

Address review feedback on nodejs#65076: move validation closer to call site in convertALPNProtocols so both client and server are covered and fail early. Empty string protocols now throw before reaching OpenSSL; wire-format buffers are validated for truncated/zero-length entries. C++ defense (THROW_ERR_INVALID_ARG_VALUE) remains for safety. Refs: nodejs#65076

Refs: nodejs#65069 PR-URL: nodejs#65076

sankalpsthakur · 2026-08-06T12:43:17Z

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 · 2026-08-07T17:39:26Z

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

Empty ALPN buffer/array means skip ALPN (same as historical behavior for []). Zero-length protocol entries and malformed wire buffers still throw from convertALPNProtocols. Revert the C++ THROW_ERR_INVALID_ARG_VALUE back to CHECK_EQ: after JS validation, a non-zero SSL_set_alpn_protos return is an internal invariant failure, not user-facing input. Refs: nodejs#65076

sankalpsthakur · 2026-08-08T05:02:39Z

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

nodejs-github-bot · 2026-08-09T14:02:15Z

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

sankalpsthakur · 2026-08-09T15:26:45Z

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 · 2026-08-09T22:37:13Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.14%. Comparing base (a48e33f) to head (e074e05).
⚠️ Report is 54 commits behind head on main.

Additional details and impacted files

@@ Coverage Diff @@## main #65076 +/- ##
==========================================
+ Coverage 90.12% 90.14% +0.01% 
==========================================
Files 751 751 Lines 252425 252625 +200 Branches 47471 47542 +71 ==========================================
+ Hits 227504 227724 +220 + Misses 16216 16200 -16 + Partials 8705 8701 -4

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

... and 56 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 · 2026-08-22T03:05:52Z

@sankalpsthakur Can you please rebase?

The merge commits don't work with our tooling.

Address review feedback on nodejs#65076: move validation closer to call site in convertALPNProtocols so both client and server are covered and fail early. Empty string protocols now throw before reaching OpenSSL; wire-format buffers are validated for truncated/zero-length entries. C++ defense (THROW_ERR_INVALID_ARG_VALUE) remains for safety. Refs: nodejs#65076

Refs: nodejs#65069 PR-URL: nodejs#65076

Empty ALPN buffer/array means skip ALPN (same as historical behavior for []). Zero-length protocol entries and malformed wire buffers still throw from convertALPNProtocols. Revert the C++ THROW_ERR_INVALID_ARG_VALUE back to CHECK_EQ: after JS validation, a non-zero SSL_set_alpn_protos return is an internal invariant failure, not user-facing input. Refs: nodejs#65076

sankalpsthakur · 2026-08-22T07:21:44Z

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

sankalpsthakur · 2026-08-22T07:25:58Z

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.

trivikr · 2026-08-22T17:15:41Z

@sankalpsthakur Can you please sign the commit?

It's required as per instructions

Your commit must contain the Signed-off-by line with your name and email address as an acknowledgement that you agree to the Developer Certificate of Origin.

tls.connect() with malformed ALPNProtocols hit CHECK_EQ(0, SSL_set_alpn_protos(...)) in the C++ layer and aborted the process with SIGABRT. Validate in JS instead, in convertALPNProtocols, so both client and server fail early with a recoverable ERR_INVALID_ARG_VALUE: - zero-length string protocols now throw from convertProtocols - wire-format buffers are checked for zero-length and truncated entries - an empty buffer or array is still accepted and means skip ALPN, matching the historical behavior for [] The C++ CHECK_EQ is left unchanged: once JS has validated the input, a non-zero SSL_set_alpn_protos return is an internal invariant failure rather than user-facing input. Fixes: nodejs#65069 Signed-off-by: Sankalp Thakur <sankalphimself@gmail.com>

sankalpsthakur · 2026-08-23T04:59:15Z

Signed-off-by is on e074e05, author Sankalp Thakur sankalphimself@gmail.com.

nodejs-github-bot · 2026-08-23T17:34:07Z

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

nodejs-github-bot · 2026-08-24T11:36:28Z

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

nodejs-github-bot · 2026-08-24T14:31:23Z

Landed in 278ead1

tls.connect() with malformed ALPNProtocols hit CHECK_EQ(0, SSL_set_alpn_protos(...)) in the C++ layer and aborted the process with SIGABRT. Validate in JS instead, in convertALPNProtocols, so both client and server fail early with a recoverable ERR_INVALID_ARG_VALUE: - zero-length string protocols now throw from convertProtocols - wire-format buffers are checked for zero-length and truncated entries - an empty buffer or array is still accepted and means skip ALPN, matching the historical behavior for [] The C++ CHECK_EQ is left unchanged: once JS has validated the input, a non-zero SSL_set_alpn_protos return is an internal invariant failure rather than user-facing input. Fixes: #65069 Signed-off-by: Sankalp Thakur <sankalphimself@gmail.com> PR-URL: #65076 Reviewed-By: Tim Perry <pimterry@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>

sankalpsthakur · 2026-08-06T09:40:43Z

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 · 2026-08-06T09:40:47Z

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.

Address review feedback on nodejs#65076: move validation closer to call site in convertALPNProtocols so both client and server are covered and fail early. Empty string protocols now throw before reaching OpenSSL; wire-format buffers are validated for truncated/zero-length entries. C++ defense (THROW_ERR_INVALID_ARG_VALUE) remains for safety. Refs: nodejs#65076

Refs: nodejs#65069 PR-URL: nodejs#65076

sankalpsthakur · 2026-08-06T10:48:18Z

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.

Address review feedback on nodejs#65076: move validation closer to call site in convertALPNProtocols so both client and server are covered and fail early. Empty string protocols now throw before reaching OpenSSL; wire-format buffers are validated for truncated/zero-length entries. C++ defense (THROW_ERR_INVALID_ARG_VALUE) remains for safety. Refs: nodejs#65076

Refs: nodejs#65069 PR-URL: nodejs#65076

sankalpsthakur · 2026-08-06T12:43:17Z

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 · 2026-08-07T17:39:26Z

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

Empty ALPN buffer/array means skip ALPN (same as historical behavior for []). Zero-length protocol entries and malformed wire buffers still throw from convertALPNProtocols. Revert the C++ THROW_ERR_INVALID_ARG_VALUE back to CHECK_EQ: after JS validation, a non-zero SSL_set_alpn_protos return is an internal invariant failure, not user-facing input. Refs: nodejs#65076

sankalpsthakur · 2026-08-08T05:02:39Z

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

nodejs-github-bot · 2026-08-09T14:02:15Z

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

sankalpsthakur · 2026-08-09T15:26:45Z

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 · 2026-08-09T22:37:13Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.14%. Comparing base (a48e33f) to head (e074e05).
⚠️ Report is 54 commits behind head on main.

Additional details and impacted files

@@ Coverage Diff @@## main #65076 +/- ##
==========================================
+ Coverage 90.12% 90.14% +0.01% 
==========================================
Files 751 751 Lines 252425 252625 +200 Branches 47471 47542 +71 ==========================================
+ Hits 227504 227724 +220 + Misses 16216 16200 -16 + Partials 8705 8701 -4

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

... and 56 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 · 2026-08-22T03:05:52Z

@sankalpsthakur Can you please rebase?

The merge commits don't work with our tooling.

Address review feedback on nodejs#65076: move validation closer to call site in convertALPNProtocols so both client and server are covered and fail early. Empty string protocols now throw before reaching OpenSSL; wire-format buffers are validated for truncated/zero-length entries. C++ defense (THROW_ERR_INVALID_ARG_VALUE) remains for safety. Refs: nodejs#65076

Refs: nodejs#65069 PR-URL: nodejs#65076

Empty ALPN buffer/array means skip ALPN (same as historical behavior for []). Zero-length protocol entries and malformed wire buffers still throw from convertALPNProtocols. Revert the C++ THROW_ERR_INVALID_ARG_VALUE back to CHECK_EQ: after JS validation, a non-zero SSL_set_alpn_protos return is an internal invariant failure, not user-facing input. Refs: nodejs#65076

sankalpsthakur · 2026-08-22T07:21:44Z

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

sankalpsthakur · 2026-08-22T07:25:58Z

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.

trivikr · 2026-08-22T17:15:41Z

@sankalpsthakur Can you please sign the commit?

It's required as per instructions

Your commit must contain the Signed-off-by line with your name and email address as an acknowledgement that you agree to the Developer Certificate of Origin.

tls.connect() with malformed ALPNProtocols hit CHECK_EQ(0, SSL_set_alpn_protos(...)) in the C++ layer and aborted the process with SIGABRT. Validate in JS instead, in convertALPNProtocols, so both client and server fail early with a recoverable ERR_INVALID_ARG_VALUE: - zero-length string protocols now throw from convertProtocols - wire-format buffers are checked for zero-length and truncated entries - an empty buffer or array is still accepted and means skip ALPN, matching the historical behavior for [] The C++ CHECK_EQ is left unchanged: once JS has validated the input, a non-zero SSL_set_alpn_protos return is an internal invariant failure rather than user-facing input. Fixes: nodejs#65069 Signed-off-by: Sankalp Thakur <sankalphimself@gmail.com>

sankalpsthakur · 2026-08-23T04:59:15Z

Signed-off-by is on e074e05, author Sankalp Thakur sankalphimself@gmail.com.

nodejs-github-bot · 2026-08-23T17:34:07Z

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

nodejs-github-bot · 2026-08-24T11:36:28Z

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

nodejs-github-bot · 2026-08-24T14:31:23Z

Landed in 278ead1

tls.connect() with malformed ALPNProtocols hit CHECK_EQ(0, SSL_set_alpn_protos(...)) in the C++ layer and aborted the process with SIGABRT. Validate in JS instead, in convertALPNProtocols, so both client and server fail early with a recoverable ERR_INVALID_ARG_VALUE: - zero-length string protocols now throw from convertProtocols - wire-format buffers are checked for zero-length and truncated entries - an empty buffer or array is still accepted and means skip ALPN, matching the historical behavior for [] The C++ CHECK_EQ is left unchanged: once JS has validated the input, a non-zero SSL_set_alpn_protos return is an internal invariant failure rather than user-facing input. Fixes: #65069 Signed-off-by: Sankalp Thakur <sankalphimself@gmail.com> PR-URL: #65076 Reviewed-By: Tim Perry <pimterry@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>

sankalpsthakur · 2026-08-06T09:40:43Z

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 · 2026-08-06T09:40:47Z

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.

Address review feedback on nodejs#65076: move validation closer to call site in convertALPNProtocols so both client and server are covered and fail early. Empty string protocols now throw before reaching OpenSSL; wire-format buffers are validated for truncated/zero-length entries. C++ defense (THROW_ERR_INVALID_ARG_VALUE) remains for safety. Refs: nodejs#65076

Refs: nodejs#65069 PR-URL: nodejs#65076

sankalpsthakur · 2026-08-06T10:48:18Z

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.

Address review feedback on nodejs#65076: move validation closer to call site in convertALPNProtocols so both client and server are covered and fail early. Empty string protocols now throw before reaching OpenSSL; wire-format buffers are validated for truncated/zero-length entries. C++ defense (THROW_ERR_INVALID_ARG_VALUE) remains for safety. Refs: nodejs#65076

Refs: nodejs#65069 PR-URL: nodejs#65076

sankalpsthakur · 2026-08-06T12:43:17Z

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 · 2026-08-07T17:39:26Z

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

Empty ALPN buffer/array means skip ALPN (same as historical behavior for []). Zero-length protocol entries and malformed wire buffers still throw from convertALPNProtocols. Revert the C++ THROW_ERR_INVALID_ARG_VALUE back to CHECK_EQ: after JS validation, a non-zero SSL_set_alpn_protos return is an internal invariant failure, not user-facing input. Refs: nodejs#65076

sankalpsthakur · 2026-08-08T05:02:39Z

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

nodejs-github-bot · 2026-08-09T14:02:15Z

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

sankalpsthakur · 2026-08-09T15:26:45Z

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 · 2026-08-09T22:37:13Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.14%. Comparing base (a48e33f) to head (e074e05).
⚠️ Report is 54 commits behind head on main.

Additional details and impacted files

@@ Coverage Diff @@## main #65076 +/- ##
==========================================
+ Coverage 90.12% 90.14% +0.01% 
==========================================
Files 751 751 Lines 252425 252625 +200 Branches 47471 47542 +71 ==========================================
+ Hits 227504 227724 +220 + Misses 16216 16200 -16 + Partials 8705 8701 -4

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

... and 56 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 · 2026-08-22T03:05:52Z

@sankalpsthakur Can you please rebase?

The merge commits don't work with our tooling.

Address review feedback on nodejs#65076: move validation closer to call site in convertALPNProtocols so both client and server are covered and fail early. Empty string protocols now throw before reaching OpenSSL; wire-format buffers are validated for truncated/zero-length entries. C++ defense (THROW_ERR_INVALID_ARG_VALUE) remains for safety. Refs: nodejs#65076

Refs: nodejs#65069 PR-URL: nodejs#65076

Empty ALPN buffer/array means skip ALPN (same as historical behavior for []). Zero-length protocol entries and malformed wire buffers still throw from convertALPNProtocols. Revert the C++ THROW_ERR_INVALID_ARG_VALUE back to CHECK_EQ: after JS validation, a non-zero SSL_set_alpn_protos return is an internal invariant failure, not user-facing input. Refs: nodejs#65076

sankalpsthakur · 2026-08-22T07:21:44Z

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

sankalpsthakur · 2026-08-22T07:25:58Z

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.

trivikr · 2026-08-22T17:15:41Z

@sankalpsthakur Can you please sign the commit?

It's required as per instructions

Your commit must contain the Signed-off-by line with your name and email address as an acknowledgement that you agree to the Developer Certificate of Origin.

tls.connect() with malformed ALPNProtocols hit CHECK_EQ(0, SSL_set_alpn_protos(...)) in the C++ layer and aborted the process with SIGABRT. Validate in JS instead, in convertALPNProtocols, so both client and server fail early with a recoverable ERR_INVALID_ARG_VALUE: - zero-length string protocols now throw from convertProtocols - wire-format buffers are checked for zero-length and truncated entries - an empty buffer or array is still accepted and means skip ALPN, matching the historical behavior for [] The C++ CHECK_EQ is left unchanged: once JS has validated the input, a non-zero SSL_set_alpn_protos return is an internal invariant failure rather than user-facing input. Fixes: nodejs#65069 Signed-off-by: Sankalp Thakur <sankalphimself@gmail.com>

sankalpsthakur · 2026-08-23T04:59:15Z

Signed-off-by is on e074e05, author Sankalp Thakur sankalphimself@gmail.com.

nodejs-github-bot · 2026-08-23T17:34:07Z

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

nodejs-github-bot · 2026-08-24T11:36:28Z

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

nodejs-github-bot · 2026-08-24T14:31:23Z

Landed in 278ead1

tls.connect() with malformed ALPNProtocols hit CHECK_EQ(0, SSL_set_alpn_protos(...)) in the C++ layer and aborted the process with SIGABRT. Validate in JS instead, in convertALPNProtocols, so both client and server fail early with a recoverable ERR_INVALID_ARG_VALUE: - zero-length string protocols now throw from convertProtocols - wire-format buffers are checked for zero-length and truncated entries - an empty buffer or array is still accepted and means skip ALPN, matching the historical behavior for [] The C++ CHECK_EQ is left unchanged: once JS has validated the input, a non-zero SSL_set_alpn_protos return is an internal invariant failure rather than user-facing input. Fixes: #65069 Signed-off-by: Sankalp Thakur <sankalphimself@gmail.com> PR-URL: #65076 Reviewed-By: Tim Perry <pimterry@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>

sankalpsthakur · 2026-08-06T09:40:43Z

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 · 2026-08-06T09:40:47Z

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.

Address review feedback on nodejs#65076: move validation closer to call site in convertALPNProtocols so both client and server are covered and fail early. Empty string protocols now throw before reaching OpenSSL; wire-format buffers are validated for truncated/zero-length entries. C++ defense (THROW_ERR_INVALID_ARG_VALUE) remains for safety. Refs: nodejs#65076

Refs: nodejs#65069 PR-URL: nodejs#65076

sankalpsthakur · 2026-08-06T10:48:18Z

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.

Address review feedback on nodejs#65076: move validation closer to call site in convertALPNProtocols so both client and server are covered and fail early. Empty string protocols now throw before reaching OpenSSL; wire-format buffers are validated for truncated/zero-length entries. C++ defense (THROW_ERR_INVALID_ARG_VALUE) remains for safety. Refs: nodejs#65076

Refs: nodejs#65069 PR-URL: nodejs#65076

sankalpsthakur · 2026-08-06T12:43:17Z

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 · 2026-08-07T17:39:26Z

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

Empty ALPN buffer/array means skip ALPN (same as historical behavior for []). Zero-length protocol entries and malformed wire buffers still throw from convertALPNProtocols. Revert the C++ THROW_ERR_INVALID_ARG_VALUE back to CHECK_EQ: after JS validation, a non-zero SSL_set_alpn_protos return is an internal invariant failure, not user-facing input. Refs: nodejs#65076

sankalpsthakur · 2026-08-08T05:02:39Z

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

nodejs-github-bot · 2026-08-09T14:02:15Z

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

sankalpsthakur · 2026-08-09T15:26:45Z

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 · 2026-08-09T22:37:13Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.14%. Comparing base (a48e33f) to head (e074e05).
⚠️ Report is 54 commits behind head on main.

Additional details and impacted files

@@ Coverage Diff @@## main #65076 +/- ##
==========================================
+ Coverage 90.12% 90.14% +0.01% 
==========================================
Files 751 751 Lines 252425 252625 +200 Branches 47471 47542 +71 ==========================================
+ Hits 227504 227724 +220 + Misses 16216 16200 -16 + Partials 8705 8701 -4

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

... and 56 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 · 2026-08-22T03:05:52Z

@sankalpsthakur Can you please rebase?

The merge commits don't work with our tooling.

Address review feedback on nodejs#65076: move validation closer to call site in convertALPNProtocols so both client and server are covered and fail early. Empty string protocols now throw before reaching OpenSSL; wire-format buffers are validated for truncated/zero-length entries. C++ defense (THROW_ERR_INVALID_ARG_VALUE) remains for safety. Refs: nodejs#65076

Refs: nodejs#65069 PR-URL: nodejs#65076

Empty ALPN buffer/array means skip ALPN (same as historical behavior for []). Zero-length protocol entries and malformed wire buffers still throw from convertALPNProtocols. Revert the C++ THROW_ERR_INVALID_ARG_VALUE back to CHECK_EQ: after JS validation, a non-zero SSL_set_alpn_protos return is an internal invariant failure, not user-facing input. Refs: nodejs#65076

sankalpsthakur · 2026-08-22T07:21:44Z

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

sankalpsthakur · 2026-08-22T07:25:58Z

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.

trivikr · 2026-08-22T17:15:41Z

@sankalpsthakur Can you please sign the commit?

It's required as per instructions

Your commit must contain the Signed-off-by line with your name and email address as an acknowledgement that you agree to the Developer Certificate of Origin.

tls.connect() with malformed ALPNProtocols hit CHECK_EQ(0, SSL_set_alpn_protos(...)) in the C++ layer and aborted the process with SIGABRT. Validate in JS instead, in convertALPNProtocols, so both client and server fail early with a recoverable ERR_INVALID_ARG_VALUE: - zero-length string protocols now throw from convertProtocols - wire-format buffers are checked for zero-length and truncated entries - an empty buffer or array is still accepted and means skip ALPN, matching the historical behavior for [] The C++ CHECK_EQ is left unchanged: once JS has validated the input, a non-zero SSL_set_alpn_protos return is an internal invariant failure rather than user-facing input. Fixes: nodejs#65069 Signed-off-by: Sankalp Thakur <sankalphimself@gmail.com>

sankalpsthakur · 2026-08-23T04:59:15Z

Signed-off-by is on e074e05, author Sankalp Thakur sankalphimself@gmail.com.

nodejs-github-bot · 2026-08-23T17:34:07Z

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

nodejs-github-bot · 2026-08-24T11:36:28Z

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

nodejs-github-bot · 2026-08-24T14:31:23Z

Landed in 278ead1

tls.connect() with malformed ALPNProtocols hit CHECK_EQ(0, SSL_set_alpn_protos(...)) in the C++ layer and aborted the process with SIGABRT. Validate in JS instead, in convertALPNProtocols, so both client and server fail early with a recoverable ERR_INVALID_ARG_VALUE: - zero-length string protocols now throw from convertProtocols - wire-format buffers are checked for zero-length and truncated entries - an empty buffer or array is still accepted and means skip ALPN, matching the historical behavior for [] The C++ CHECK_EQ is left unchanged: once JS has validated the input, a non-zero SSL_set_alpn_protos return is an internal invariant failure rather than user-facing input. Fixes: #65069 Signed-off-by: Sankalp Thakur <sankalphimself@gmail.com> PR-URL: #65076 Reviewed-By: Tim Perry <pimterry@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>

sankalpsthakur · 2026-08-06T09:40:43Z

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 · 2026-08-06T09:40:47Z

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.

Address review feedback on nodejs#65076: move validation closer to call site in convertALPNProtocols so both client and server are covered and fail early. Empty string protocols now throw before reaching OpenSSL; wire-format buffers are validated for truncated/zero-length entries. C++ defense (THROW_ERR_INVALID_ARG_VALUE) remains for safety. Refs: nodejs#65076

Refs: nodejs#65069 PR-URL: nodejs#65076

sankalpsthakur · 2026-08-06T10:48:18Z

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.

Address review feedback on nodejs#65076: move validation closer to call site in convertALPNProtocols so both client and server are covered and fail early. Empty string protocols now throw before reaching OpenSSL; wire-format buffers are validated for truncated/zero-length entries. C++ defense (THROW_ERR_INVALID_ARG_VALUE) remains for safety. Refs: nodejs#65076

Refs: nodejs#65069 PR-URL: nodejs#65076

sankalpsthakur · 2026-08-06T12:43:17Z

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 · 2026-08-07T17:39:26Z

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

Empty ALPN buffer/array means skip ALPN (same as historical behavior for []). Zero-length protocol entries and malformed wire buffers still throw from convertALPNProtocols. Revert the C++ THROW_ERR_INVALID_ARG_VALUE back to CHECK_EQ: after JS validation, a non-zero SSL_set_alpn_protos return is an internal invariant failure, not user-facing input. Refs: nodejs#65076

sankalpsthakur · 2026-08-08T05:02:39Z

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

nodejs-github-bot · 2026-08-09T14:02:15Z

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

sankalpsthakur · 2026-08-09T15:26:45Z

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 · 2026-08-09T22:37:13Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.14%. Comparing base (a48e33f) to head (e074e05).
⚠️ Report is 54 commits behind head on main.

Additional details and impacted files

@@ Coverage Diff @@## main #65076 +/- ##
==========================================
+ Coverage 90.12% 90.14% +0.01% 
==========================================
Files 751 751 Lines 252425 252625 +200 Branches 47471 47542 +71 ==========================================
+ Hits 227504 227724 +220 + Misses 16216 16200 -16 + Partials 8705 8701 -4

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

... and 56 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 · 2026-08-22T03:05:52Z

@sankalpsthakur Can you please rebase?

The merge commits don't work with our tooling.

Address review feedback on nodejs#65076: move validation closer to call site in convertALPNProtocols so both client and server are covered and fail early. Empty string protocols now throw before reaching OpenSSL; wire-format buffers are validated for truncated/zero-length entries. C++ defense (THROW_ERR_INVALID_ARG_VALUE) remains for safety. Refs: nodejs#65076

Refs: nodejs#65069 PR-URL: nodejs#65076

Empty ALPN buffer/array means skip ALPN (same as historical behavior for []). Zero-length protocol entries and malformed wire buffers still throw from convertALPNProtocols. Revert the C++ THROW_ERR_INVALID_ARG_VALUE back to CHECK_EQ: after JS validation, a non-zero SSL_set_alpn_protos return is an internal invariant failure, not user-facing input. Refs: nodejs#65076

sankalpsthakur · 2026-08-22T07:21:44Z

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

sankalpsthakur · 2026-08-22T07:25:58Z

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.

trivikr · 2026-08-22T17:15:41Z

@sankalpsthakur Can you please sign the commit?

It's required as per instructions

Your commit must contain the Signed-off-by line with your name and email address as an acknowledgement that you agree to the Developer Certificate of Origin.

tls.connect() with malformed ALPNProtocols hit CHECK_EQ(0, SSL_set_alpn_protos(...)) in the C++ layer and aborted the process with SIGABRT. Validate in JS instead, in convertALPNProtocols, so both client and server fail early with a recoverable ERR_INVALID_ARG_VALUE: - zero-length string protocols now throw from convertProtocols - wire-format buffers are checked for zero-length and truncated entries - an empty buffer or array is still accepted and means skip ALPN, matching the historical behavior for [] The C++ CHECK_EQ is left unchanged: once JS has validated the input, a non-zero SSL_set_alpn_protos return is an internal invariant failure rather than user-facing input. Fixes: nodejs#65069 Signed-off-by: Sankalp Thakur <sankalphimself@gmail.com>

sankalpsthakur · 2026-08-23T04:59:15Z

Signed-off-by is on e074e05, author Sankalp Thakur sankalphimself@gmail.com.

nodejs-github-bot · 2026-08-23T17:34:07Z

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

nodejs-github-bot · 2026-08-24T11:36:28Z

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

nodejs-github-bot · 2026-08-24T14:31:23Z

Landed in 278ead1

tls.connect() with malformed ALPNProtocols hit CHECK_EQ(0, SSL_set_alpn_protos(...)) in the C++ layer and aborted the process with SIGABRT. Validate in JS instead, in convertALPNProtocols, so both client and server fail early with a recoverable ERR_INVALID_ARG_VALUE: - zero-length string protocols now throw from convertProtocols - wire-format buffers are checked for zero-length and truncated entries - an empty buffer or array is still accepted and means skip ALPN, matching the historical behavior for [] The C++ CHECK_EQ is left unchanged: once JS has validated the input, a non-zero SSL_set_alpn_protos return is an internal invariant failure rather than user-facing input. Fixes: #65069 Signed-off-by: Sankalp Thakur <sankalphimself@gmail.com> PR-URL: #65076 Reviewed-By: Tim Perry <pimterry@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem. needs-ci PRs that need a full CI run. labels Aug 6, 2026

pimterry requested changes Aug 6, 2026
View reviewed changes

Comment threadsrc/crypto/crypto_tls.cc Outdated

sankalpsthakur pushed a commit to sankalpsthakur/node that referenced this pull request Aug 6, 2026
test: add validation for invalid ALPNProtocols
dab0602
Refs: nodejs#65069 PR-URL: nodejs#65076

sankalpsthakur pushed a commit to sankalpsthakur/node that referenced this pull request Aug 6, 2026
test: add validation for invalid ALPNProtocols
aac5713
Refs: nodejs#65069 PR-URL: nodejs#65076

sankalpsthakur force-pushed the fix/65069-alpn-sigabrt branch from dab0602 to af6b21fCompare August 6, 2026 12:42

pimterry requested changes Aug 6, 2026
View reviewed changes

Comment threadlib/tls.js Outdated
Comment threadsrc/crypto/crypto_tls.cc Outdated

jasnell approved these changes Aug 8, 2026
View reviewed changes

pimterry approved these changes Aug 9, 2026
View reviewed changes

pimterry added author ready PRs with CI started, the required approvals, and no outstanding review comments. request-ci Add this label to start a Jenkins CI on a PR. labels Aug 9, 2026

github-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 9, 2026

pimterry added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 9, 2026

sankalpsthakur force-pushed the fix/65069-alpn-sigabrt branch from 82aed18 to 193e77bCompare August 18, 2026 04:45

trivikr removed the author ready PRs with CI started, the required approvals, and no outstanding review comments. label Aug 22, 2026

sankalpsthakur force-pushed the fix/65069-alpn-sigabrt branch from 82aed18 to 1e49721Compare August 22, 2026 07:21

sankalpsthakur pushed a commit to sankalpsthakur/node that referenced this pull request Aug 22, 2026
test: add validation for invalid ALPNProtocols
6f951ae
Refs: nodejs#65069 PR-URL: nodejs#65076

sankalpsthakur force-pushed the fix/65069-alpn-sigabrt branch from 1e49721 to eaaedf8Compare August 22, 2026 07:25

sankalpsthakur force-pushed the fix/65069-alpn-sigabrt branch 3 times, most recently from 73e91cb to 4e001a7Compare August 23, 2026 04:58

sankalpsthakur force-pushed the fix/65069-alpn-sigabrt branch from 4e001a7 to e074e05Compare August 23, 2026 04:59

trivikr approved these changes Aug 23, 2026
View reviewed changes

trivikr added the author ready PRs with CI started, the required approvals, and no outstanding review comments. label Aug 23, 2026

github-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 23, 2026

pimterry approved these changes Aug 24, 2026
View reviewed changes

trivikr added the commit-queue PRs queued for automated landing through the Commit Queue. label Aug 24, 2026

nodejs-github-bot merged commit 278ead1 into nodejs:mainAug 24, 2026
72 of 73 checks passed

nodejs-github-bot removed the commit-queue PRs queued for automated landing through the Commit Queue. label Aug 24, 2026

github-actionsBot mentioned this pull request Aug 25, 2026
2026-08-26, Version 26.8.0 (Current) #65551
Merged