Skip to content

Commit

Permalink
http2: fix pseudo-headers order
Browse files Browse the repository at this point in the history 
Keep pseudo-headers order same as sent order

Fixes: #38797

PR-URL: #41735
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
  • Loading branch information
@danielleadams
ofir authored and danielleadams committed Mar 14, 2022
1 parent bf99ef8 commit e747ef5
Show file tree
Hide file tree
Showing 4 changed files with 156 additions and 90 deletions.
11 changes: 6 additions & 5 deletions lib/internal/http2/util.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down Expand Up @@ -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;
}
Expand All @@ -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 {
Expand Down
209 changes: 137 additions & 72 deletions test/parallel/test-http2-compat-serverrequest-headers.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}));
}));
}
12 changes: 6 additions & 6 deletions test/parallel/test-http2-multiheaders-raw.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand Down
14 changes: 7 additions & 7 deletions test/parallel/test-http2-util-headers-list.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Expand All @@ -116,8 +116,8 @@ const {
{
const headers = {
'abc': 1,
':path': 'abc',
':status': [200],
':path': 'abc',
':authority': [],
'xyz': [1, 2, 3, 4]
};
Expand All @@ -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
};

Expand All @@ -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),
Expand Down Expand Up @@ -191,8 +191,8 @@ const {
{
const headers = {
'abc': 1,
':path': 'abc',
':status': [200],
':path': 'abc',
':authority': [],
'xyz': [1, 2, 3, 4],
[sensitiveHeaders]: ['xyz']
Expand Down

0 comments on commit e747ef5

Please sign in to comment.