GeezerVison

Copy link

Copy Markdown

Contributor

pimterry requested changes Aug 6, 2026

View reviewed changes

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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

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 force-pushed the fix/65069-alpn-sigabrt branch from dab0602 to af6b21f Compare August 6, 2026 12:42

pimterry requested changes Aug 6, 2026

View reviewed changes

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

jasnell approved these changes Aug 8, 2026

View reviewed changes

pimterry approved these changes Aug 9, 2026

View reviewed changes

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

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

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

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 force-pushed the fix/65069-alpn-sigabrt branch from 1e49721 to eaaedf8 Compare August 22, 2026 07:25

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

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

trivikr approved these changes Aug 23, 2026

View reviewed changes

pimterry approved these changes Aug 24, 2026

View reviewed changes

72 of 73 checks passed

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 <[email protected]>
PR-URL: #65076
Reviewed-By: Tim Perry <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>

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 <[email protected]>
PR-URL: #65076
Reviewed-By: Tim Perry <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>

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 <[email protected]>
PR-URL: #65076
Reviewed-By: Tim Perry <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>

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 <[email protected]>
PR-URL: #65076
Reviewed-By: Tim Perry <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>

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 <[email protected]>
PR-URL: #65076
Reviewed-By: Tim Perry <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>