From e747ef5e45f229ede0ee7fcfde777be021b14a52 Mon Sep 17 00:00:00 2001 From: ofir Date: Fri, 28 Jan 2022 23:13:54 +0200 Subject: [PATCH] http2: fix pseudo-headers order Keep pseudo-headers order same as sent order Fixes: https://github.com/nodejs/node/issues/38797 PR-URL: https://github.com/nodejs/node/pull/41735 Reviewed-By: James M Snell Reviewed-By: Matteo Collina --- lib/internal/http2/util.js | 11 +- ...test-http2-compat-serverrequest-headers.js | 209 ++++++++++++------ test/parallel/test-http2-multiheaders-raw.js | 12 +- test/parallel/test-http2-util-headers-list.js | 14 +- 4 files changed, 156 insertions(+), 90 deletions(-) diff --git a/lib/internal/http2/util.js b/lib/internal/http2/util.js index 78ff2937c3a317..38578d2a151c52 100644 --- a/lib/internal/http2/util.js +++ b/lib/internal/http2/util.js @@ -472,7 +472,8 @@ const kNeverIndexFlag = StringFromCharCode(NGHTTP2_NV_FLAG_NO_INDEX); const kNoHeaderFlags = StringFromCharCode(NGHTTP2_NV_FLAG_NONE); function mapToHeaders(map, assertValuePseudoHeader = assertValidPseudoHeader) { - let ret = ''; + let headers = ''; + let pseudoHeaders = ''; let count = 0; const keys = ObjectKeys(map); const singles = new SafeSet(); @@ -520,7 +521,7 @@ function mapToHeaders(map, err = assertValuePseudoHeader(key); if (err !== undefined) throw err; - ret = `${key}\0${value}\0${flags}${ret}`; + pseudoHeaders += `${key}\0${value}\0${flags}`; count++; continue; } @@ -533,16 +534,16 @@ function mapToHeaders(map, if (isArray) { for (j = 0; j < value.length; ++j) { const val = String(value[j]); - ret += `${key}\0${val}\0${flags}`; + headers += `${key}\0${val}\0${flags}`; } count += value.length; continue; } - ret += `${key}\0${value}\0${flags}`; + headers += `${key}\0${value}\0${flags}`; count++; } - return [ret, count]; + return [pseudoHeaders + headers, count]; } class NghttpError extends Error { diff --git a/test/parallel/test-http2-compat-serverrequest-headers.js b/test/parallel/test-http2-compat-serverrequest-headers.js index 06f37c8a47ce19..2028e672a7f173 100644 --- a/test/parallel/test-http2-compat-serverrequest-headers.js +++ b/test/parallel/test-http2-compat-serverrequest-headers.js @@ -6,83 +6,148 @@ if (!common.hasCrypto) const assert = require('assert'); const h2 = require('http2'); -// Http2ServerRequest should have header helpers - -const server = h2.createServer(); -server.listen(0, common.mustCall(function() { - const port = server.address().port; - server.once('request', common.mustCall(function(request, response) { - const expected = { - ':path': '/foobar', - ':method': 'GET', - ':scheme': 'http', - ':authority': `localhost:${port}`, - 'foo-bar': 'abc123' - }; - - assert.strictEqual(request.path, undefined); - assert.strictEqual(request.method, expected[':method']); - assert.strictEqual(request.scheme, expected[':scheme']); - assert.strictEqual(request.url, expected[':path']); - assert.strictEqual(request.authority, expected[':authority']); - - const headers = request.headers; - for (const [name, value] of Object.entries(expected)) { - assert.strictEqual(headers[name], value); - } - - const rawHeaders = request.rawHeaders; - for (const [name, value] of Object.entries(expected)) { - const position = rawHeaders.indexOf(name); - assert.notStrictEqual(position, -1); - assert.strictEqual(rawHeaders[position + 1], value); - } - - request.url = '/one'; - assert.strictEqual(request.url, '/one'); - - // Third-party plugins for packages like express use query params to - // change the request method - request.method = 'POST'; - assert.strictEqual(request.method, 'POST'); - assert.throws( - () => request.method = ' ', - { - code: 'ERR_INVALID_ARG_VALUE', - name: 'TypeError', - message: "The argument 'method' is invalid. Received ' '" +{ + // Http2ServerRequest should have header helpers + + const server = h2.createServer(); + server.listen(0, common.mustCall(function() { + const port = server.address().port; + server.once('request', common.mustCall(function(request, response) { + const expected = { + ':path': '/foobar', + ':method': 'GET', + ':scheme': 'http', + ':authority': `localhost:${port}`, + 'foo-bar': 'abc123' + }; + + assert.strictEqual(request.path, undefined); + assert.strictEqual(request.method, expected[':method']); + assert.strictEqual(request.scheme, expected[':scheme']); + assert.strictEqual(request.url, expected[':path']); + assert.strictEqual(request.authority, expected[':authority']); + + const headers = request.headers; + for (const [name, value] of Object.entries(expected)) { + assert.strictEqual(headers[name], value); } - ); - assert.throws( - () => request.method = true, - { - code: 'ERR_INVALID_ARG_TYPE', - name: 'TypeError', - message: 'The "method" argument must be of type string. ' + - 'Received type boolean (true)' + + const rawHeaders = request.rawHeaders; + for (const [name, value] of Object.entries(expected)) { + const position = rawHeaders.indexOf(name); + assert.notStrictEqual(position, -1); + assert.strictEqual(rawHeaders[position + 1], value); } - ); - response.on('finish', common.mustCall(function() { - server.close(); + request.url = '/one'; + assert.strictEqual(request.url, '/one'); + + // Third-party plugins for packages like express use query params to + // change the request method + request.method = 'POST'; + assert.strictEqual(request.method, 'POST'); + assert.throws( + () => request.method = ' ', + { + code: 'ERR_INVALID_ARG_VALUE', + name: 'TypeError', + message: "The argument 'method' is invalid. Received ' '" + } + ); + assert.throws( + () => request.method = true, + { + code: 'ERR_INVALID_ARG_TYPE', + name: 'TypeError', + message: 'The "method" argument must be of type string. ' + + 'Received type boolean (true)' + } + ); + + response.on('finish', common.mustCall(function() { + server.close(); + })); + response.end(); + })); + + const url = `http://localhost:${port}`; + const client = h2.connect(url, common.mustCall(function() { + const headers = { + ':path': '/foobar', + ':method': 'GET', + ':scheme': 'http', + ':authority': `localhost:${port}`, + 'foo-bar': 'abc123' + }; + const request = client.request(headers); + request.on('end', common.mustCall(function() { + client.close(); + })); + request.end(); + request.resume(); })); - response.end(); })); +} + +{ + // Http2ServerRequest should keep pseudo-headers order and after them, + // in the same order, the others headers + + const server = h2.createServer(); + server.listen(0, common.mustCall(function() { + const port = server.address().port; + server.once('request', common.mustCall(function(request, response) { + const expected = { + ':path': '/foobar', + ':method': 'GET', + ':scheme': 'http', + ':authority': `localhost:${port}`, + 'foo1': 'abc1', + 'foo2': 'abc2' + }; + + assert.strictEqual(request.path, undefined); + assert.strictEqual(request.method, expected[':method']); + assert.strictEqual(request.scheme, expected[':scheme']); + assert.strictEqual(request.url, expected[':path']); + assert.strictEqual(request.authority, expected[':authority']); + + const headers = request.headers; + for (const [name, value] of Object.entries(expected)) { + assert.strictEqual(headers[name], value); + } + + const rawHeaders = request.rawHeaders; + let expectedPosition = 0; + for (const [name, value] of Object.entries(expected)) { + const position = rawHeaders.indexOf(name); + assert.strictEqual(position / 2, expectedPosition); + assert.strictEqual(rawHeaders[position + 1], value); + expectedPosition++; + } + + response.on('finish', common.mustCall(function() { + server.close(); + })); + response.end(); + })); - const url = `http://localhost:${port}`; - const client = h2.connect(url, common.mustCall(function() { - const headers = { - ':path': '/foobar', - ':method': 'GET', - ':scheme': 'http', - ':authority': `localhost:${port}`, - 'foo-bar': 'abc123' - }; - const request = client.request(headers); - request.on('end', common.mustCall(function() { - client.close(); + const url = `http://localhost:${port}`; + const client = h2.connect(url, common.mustCall(function() { + const headers = { + ':path': '/foobar', + ':method': 'GET', + 'foo1': 'abc1', + ':scheme': 'http', + 'foo2': 'abc2', + ':authority': `localhost:${port}` + }; + const request = client.request(headers); + request.on('end', common.mustCall(function() { + client.close(); + })); + request.end(); + request.resume(); })); - request.end(); - request.resume(); })); -})); +} diff --git a/test/parallel/test-http2-multiheaders-raw.js b/test/parallel/test-http2-multiheaders-raw.js index 8e80a1c6312a91..9bd48a9528eab3 100644 --- a/test/parallel/test-http2-multiheaders-raw.js +++ b/test/parallel/test-http2-multiheaders-raw.js @@ -16,14 +16,14 @@ src.test = 'foo, bar, baz'; server.on('stream', common.mustCall((stream, headers, flags, rawHeaders) => { const expected = [ - ':path', - '/', - ':scheme', - 'http', - ':authority', - `localhost:${server.address().port}`, ':method', 'GET', + ':authority', + `localhost:${server.address().port}`, + ':scheme', + 'http', + ':path', + '/', 'www-authenticate', 'foo', 'www-authenticate', diff --git a/test/parallel/test-http2-util-headers-list.js b/test/parallel/test-http2-util-headers-list.js index a25336ee7a8473..322323960e545d 100644 --- a/test/parallel/test-http2-util-headers-list.js +++ b/test/parallel/test-http2-util-headers-list.js @@ -98,8 +98,8 @@ const { { const headers = { 'abc': 1, - ':status': 200, ':path': 'abc', + ':status': 200, 'xyz': [1, '2', { toString() { return '3'; } }, 4], 'foo': [], 'BAR': [1] @@ -116,8 +116,8 @@ const { { const headers = { 'abc': 1, - ':path': 'abc', ':status': [200], + ':path': 'abc', ':authority': [], 'xyz': [1, 2, 3, 4] }; @@ -132,10 +132,10 @@ const { { const headers = { 'abc': 1, - ':path': 'abc', + ':status': 200, 'xyz': [1, 2, 3, 4], '': 1, - ':status': 200, + ':path': 'abc', [Symbol('test')]: 1 // Symbol keys are ignored }; @@ -150,10 +150,10 @@ const { // Only own properties are used const base = { 'abc': 1 }; const headers = Object.create(base); - headers[':path'] = 'abc'; + headers[':status'] = 200; headers.xyz = [1, 2, 3, 4]; headers.foo = []; - headers[':status'] = 200; + headers[':path'] = 'abc'; assert.deepStrictEqual( mapToHeaders(headers), @@ -191,8 +191,8 @@ const { { const headers = { 'abc': 1, - ':path': 'abc', ':status': [200], + ':path': 'abc', ':authority': [], 'xyz': [1, 2, 3, 4], [sensitiveHeaders]: ['xyz']