http2: reject non-latin1 client request values

Author: mcollina

doc/api/http2.md

@@ -959,6 +959,10 @@ For HTTP/2 Client `Http2Session` instances only, the `http2session.request()`
 creates and returns an `Http2Stream` instance that can be used to send an
 HTTP/2 request to the connected server.
 
+When sending a request, header values must not contain characters outside the
+`latin1` encoding. The `:path` pseudo-header must not contain unescaped
+characters.
+
 When a `ClientHttp2Session` is first created, the socket may not yet be
 connected. if `clienthttp2session.request()` is called during this time, the
 actual request will be deferred until the socket is ready to go.

lib/internal/http2/util.js

@@ -18,16 +18,23 @@ const {
   Symbol,
 } = primordials;
 
+const {
+  _checkInvalidHeaderChar: checkInvalidHeaderChar,
+  _checkIsHttpToken: checkIsHttpToken,
+} = require('_http_common');
+
 const binding = internalBinding('http2');
 const {
   codes: {
     ERR_HTTP2_HEADER_SINGLE_VALUE,
     ERR_HTTP2_INVALID_CONNECTION_HEADERS,
+    ERR_HTTP2_INVALID_HEADER_VALUE,
     ERR_HTTP2_INVALID_PSEUDOHEADER: { HideStackFramesError: ERR_HTTP2_INVALID_PSEUDOHEADER },
     ERR_HTTP2_INVALID_SETTING_VALUE,
     ERR_HTTP2_TOO_MANY_CUSTOM_SETTINGS,
     ERR_INVALID_ARG_TYPE,
     ERR_INVALID_HTTP_TOKEN,
+    ERR_UNESCAPED_CHARACTERS,
   },
   getMessage,
   hideStackFrames,
@@ -113,6 +120,18 @@ const kValidPseudoHeaders = new SafeSet([
   HTTP2_HEADER_PROTOCOL,
 ]);
 
+const INVALID_PATH_REGEX = /[^\u0021-\u00ff]/;
+
+function assertValidHeaderValue(name, value) {
+  if (name === HTTP2_HEADER_PATH && INVALID_PATH_REGEX.test(value)) {
+    throw new ERR_UNESCAPED_CHARACTERS('Request path');
+  }
+
+  if (checkInvalidHeaderChar(value)) {
+    throw new ERR_HTTP2_INVALID_HEADER_VALUE(value, name);
+  }
+}
+
 // This set contains headers that are permitted to have only a single
 // value. Multiple instances must not be specified.
 const kSingleValueHeaders = new SafeSet([
@@ -595,6 +614,8 @@ function mapToHeaders(map,
   let pseudoHeaders = '';
   let count = 0;
   const keys = ObjectKeys(map);
+  const shouldValidateHeaderValue =
+    assertValuePseudoHeader === assertValidPseudoHeader;
   const singles = new SafeSet();
   let i, j;
   let isArray;
@@ -640,6 +661,9 @@ function mapToHeaders(map,
       err = assertValuePseudoHeader(key);
       if (err !== undefined)
         throw err;
+      if (shouldValidateHeaderValue) {
+        assertValidHeaderValue(key, value);
+      }
       pseudoHeaders += `${key}\0${value}\0${flags}`;
       count++;
       continue;
@@ -653,11 +677,17 @@ function mapToHeaders(map,
     if (isArray) {
       for (j = 0; j < value.length; ++j) {
         const val = String(value[j]);
+        if (shouldValidateHeaderValue) {
+          assertValidHeaderValue(key, val);
+        }
         headers += `${key}\0${val}\0${flags}`;
       }
       count += value.length;
       continue;
     }
+    if (shouldValidateHeaderValue) {
+      assertValidHeaderValue(key, value);
+    }
     headers += `${key}\0${value}\0${flags}`;
     count++;
   }

test/parallel/test-http2-client-unescaped-path.js

@@ -3,8 +3,8 @@
 const common = require('../common');
 if (!common.hasCrypto)
   common.skip('missing crypto');
+const assert = require('assert');
 const http2 = require('http2');
-const Countdown = require('../common/countdown');
 
 const server = http2.createServer();
 
@@ -14,24 +14,22 @@ const count = 32;
 
 server.listen(0, common.mustCall(() => {
   const client = http2.connect(`http://localhost:${server.address().port}`);
-  client.setMaxListeners(33);
 
-  const countdown = new Countdown(count + 1, () => {
-    server.close();
-    client.close();
-  });
-
-  // nghttp2 will catch the bad header value for us.
-  function doTest(i) {
-    const req = client.request({ ':path': `bad${String.fromCharCode(i)}path` });
-    req.on('error', common.expectsError({
-      code: 'ERR_HTTP2_STREAM_ERROR',
-      name: 'Error',
-      message: 'Stream closed with error code NGHTTP2_PROTOCOL_ERROR'
-    }));
-    req.on('close', common.mustCall(() => countdown.dec()));
+  for (let i = 0; i <= count; i += 1) {
+    const path = `bad${String.fromCharCode(i)}path`;
+    assert.throws(() => client.request({ ':path': path }), {
+      code: 'ERR_UNESCAPED_CHARACTERS',
+      name: 'TypeError',
+      message: 'Request path contains unescaped characters'
+    });
   }
 
-  for (let i = 0; i <= count; i += 1)
-    doTest(i);
+  assert.throws(() => client.request({ ':path': 'bad\u0100path' }), {
+    code: 'ERR_UNESCAPED_CHARACTERS',
+    name: 'TypeError',
+    message: 'Request path contains unescaped characters'
+  });
+
+  client.close();
+  server.close();
 }));

test/parallel/test-http2-invalidheaderfields-client.js

@@ -14,8 +14,14 @@ server1.listen(0, common.mustCall(() => {
   }, {
     code: 'ERR_INVALID_HTTP_TOKEN'
   });
+  assert.throws(() => {
+    session.request({ 'x-bad-char': 'oʊmɪɡə' });
+  }, {
+    code: 'ERR_HTTP2_INVALID_HEADER_VALUE'
+  });
   session.on('error', common.mustCall((e) => {
     assert.strictEqual(e.code, 'ERR_INVALID_HTTP_TOKEN');
+    session.close();
     server1.close();
   }));
 }));

test/parallel/test-http2-util-headers-list.js

@@ -336,6 +336,26 @@ mapToHeaders({ te: ['trailers'] });
 // Refs: https://github.com/nodejs/node/issues/29858
 mapToHeaders({ [HTTP2_HEADER_HOST]: 'abc' });
 
+assert.throws(() => buildNgHeaderString(
+  { ':path': 'bad\u0100path' },
+  assertValidPseudoHeader,
+  true
+), {
+  code: 'ERR_UNESCAPED_CHARACTERS',
+  name: 'TypeError',
+  message: 'Request path contains unescaped characters'
+});
+
+assert.throws(() => buildNgHeaderString(
+  { 'x-bad-char': 'oʊmɪɡə' },
+  assertValidPseudoHeader,
+  true
+), {
+  code: 'ERR_HTTP2_INVALID_HEADER_VALUE',
+  name: 'TypeError',
+  message: 'Invalid value "oʊmɪɡə" for header "x-bad-char"'
+});
+
 // If both are present, the latter has priority
 assert.strictEqual(getAuthority({
   [HTTP2_HEADER_AUTHORITY]: 'abc',