tls: throw on invalid ALPNProtocols instead of aborting

Project: nodejs/node | Language: JavaScript | Concepts: error-handling, api-design, testing-strategy | Difficulty: mid
Link: https://github.com/nodejs/node/pull/65076

The problem

Passing malformed ALPNProtocols to tls.connect() (such as [''] or invalid buffers like Buffer.from([0])) would cause Node.js to abort the process with a SIGABRT (hard crash), rather than throwing a recoverable JavaScript exception. This made it impossible for user code to handle these errors gracefully.

Why it's non-trivial

A naive fix might only validate input in some code paths (e.g., just for the client or just for arrays), or might allow some malformed wire-format buffers through. The fix needs to:

• Validate both array and buffer forms of ALPNProtocols.
• Fail early in JavaScript, not deep in C++ or OpenSSL.
• Allow existing valid uses (like empty arrays/buffers to skip ALPN).
• Ensure consistent error handling for both client and server.

The approach

1. Validate protocol arrays:

In convertProtocols, the code now checks for empty strings (which are invalid protocol entries):

+    if (len === 0) {
+      throw new ERR_INVALID_ARG_VALUE(`protocols[${i}]`, c,
+                                      'must be a non-empty string');
+    }

This ensures that [''] or arrays containing empty strings throw a recoverable error immediately, rather than crashing later.

2. Validate wire-format buffers:

A new function validateALPNBuffer checks that buffers are well-formed according to the ALPN wire format (no zero-length entries, no truncated protocols):

+function validateALPNBuffer(buffer) {
+  // Wire format: sequence of <len><proto> where len is 1 byte (1-255) and
+  // exactly len bytes follow, no trailing bytes, no zero-length entries.
+  // Empty buffer is allowed and means skip ALPN (same as []).
+  let offset = 0;
+  while (offset < buffer.length) {
+    const len = buffer[offset];
+    if (len === 0) {
+      throw new ERR_INVALID_ARG_VALUE('ALPNProtocols', buffer,
+                                      'must not contain zero-length protocol');
+    }
+    if (offset + 1 + len > buffer.length) {
+      throw new ERR_INVALID_ARG_VALUE('ALPNProtocols', buffer,
+                                      'contains truncated protocol');
+    }
+    offset += 1 + len;
+  }
+}

This prevents malformed buffers like Buffer.from([0]) or truncated entries from being accepted.

3. Apply validation in all code paths:

The exported convertALPNProtocols function now validates both arrays and buffers, for all entry points (client and server):

-    out.ALPNProtocols = Buffer.from(protocols);
+    const buf = Buffer.from(protocols);
+    validateALPNBuffer(buf);
+    out.ALPNProtocols = buf;
...
-    out.ALPNProtocols = Buffer.from(protocols.buffer.slice(
+    const buf = Buffer.from(protocols.buffer.slice(
      protocols.byteOffset,
      protocols.byteOffset + protocols.byteLength,
    ));
+    validateALPNBuffer(buf);
+    out.ALPNProtocols = buf;

This ensures all forms of input are checked before reaching native code.

4. Add comprehensive tests:

A new test file covers all error and valid cases:

+// Array with empty string should throw (zero-length protocol entry)
+assert.throws(() => {
+  const out = {};
+  tls.convertALPNProtocols([''], out);
+}, {
+  code: 'ERR_INVALID_ARG_VALUE',
+});
...
+// Buffer wire format with leading zero length
+assert.throws(() => {
+  const out = {};
+  tls.convertALPNProtocols(Buffer.from([0]), out);
+}, {
+  code: 'ERR_INVALID_ARG_VALUE',
+});
...
+// Empty array means skip ALPN (allowed)
+{
+  const out = {};
+  tls.convertALPNProtocols([], out);
+  assert.ok(Buffer.isBuffer(out.ALPNProtocols));
+  assert.strictEqual(out.ALPNProtocols.length, 0);
+}

Tests ensure both error and valid cases are handled as intended.

From the review thread

pimterry:
This only covers the client case. The server side seems to have the same issue.

We'd also normally validate on the JS side, closer to call site and failing early before doing the extra work it takes to get here.

I think we could do validation inside convertALPNProtocols instead, which is run by both client & server, and already checks for too-long values - it's just missing the empty string case. Can you take a look at fixing this there instead?

sankalpsthakur:
Thanks @pimterry, good points. Updated to validate in convertALPNProtocols so both client and server are covered and we fail early before reaching OpenSSL. Empty string protocols now throw via convertProtocols, and wire-format buffers are validated for truncated/zero-length cases. Kept the C++ defense as well. Added test/parallel/test-tls-alpn-protocols-validation.js covering the repro cases from #65069. Let me know if you'd like anything else.

Takeaways

• Validate user input as early as possible and as close to the API boundary as feasible, to catch errors before they reach lower-level code or external libraries.
• Defensive checks should be applied to all supported input types and code paths, not just the most common or obvious ones.
• When changing error handling, ensure that valid legacy behaviors (such as empty arrays meaning "skip") are preserved unless there's a strong reason to break compatibility.
• Comprehensive tests for both error and valid cases are essential to prevent regressions and to document intended behaviors.