Skip to content

Commit bfddcb1

Browse files
ofirruyadorno
ofir
authored and
ruyadorno
committed
http2: fix pseudo-headers order
Keep pseudo-headers order same as sent order Fixes: #38797 PR-URL: #41735 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
1 parent 145a3b8 commit bfddcb1

File tree

4 files changed

+156
-90
lines changed

4 files changed

+156
-90
lines changed

‎lib/internal/http2/util.js‎

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -472,7 +472,8 @@ const kNeverIndexFlag = StringFromCharCode(NGHTTP2_NV_FLAG_NO_INDEX);
472472
const kNoHeaderFlags = StringFromCharCode(NGHTTP2_NV_FLAG_NONE);
473473
function mapToHeaders(map,
474474
assertValuePseudoHeader = assertValidPseudoHeader) {
475-
let ret = '';
475+
let headers = '';
476+
let pseudoHeaders = '';
476477
let count = 0;
477478
const keys = ObjectKeys(map);
478479
const singles = new SafeSet();
@@ -520,7 +521,7 @@ function mapToHeaders(map,
520521
err = assertValuePseudoHeader(key);
521522
if (err !== undefined)
522523
throw err;
523-
ret = `${key}\0${value}\0${flags}${ret}`;
524+
pseudoHeaders += `${key}\0${value}\0${flags}`;
524525
count++;
525526
continue;
526527
}
@@ -533,16 +534,16 @@ function mapToHeaders(map,
533534
if (isArray) {
534535
for (j = 0; j < value.length; ++j) {
535536
const val = String(value[j]);
536-
ret += `${key}\0${val}\0${flags}`;
537+
headers += `${key}\0${val}\0${flags}`;
537538
}
538539
count += value.length;
539540
continue;
540541
}
541-
ret += `${key}\0${value}\0${flags}`;
542+
headers += `${key}\0${value}\0${flags}`;
542543
count++;
543544
}
544545

545-
return [ret, count];
546+
return [pseudoHeaders + headers, count];
546547
}
547548

548549
class NghttpError extends Error {

‎test/parallel/test-http2-compat-serverrequest-headers.js‎

Lines changed: 137 additions & 72 deletions
Original file line numberDiff line numberDiff line change
@@ -6,83 +6,148 @@ if (!common.hasCrypto)
66
const assert = require('assert');
77
const h2 = require('http2');
88

9-
// Http2ServerRequest should have header helpers
10-
11-
const server = h2.createServer();
12-
server.listen(0, common.mustCall(function() {
13-
const port = server.address().port;
14-
server.once('request', common.mustCall(function(request, response) {
15-
const expected = {
16-
':path': '/foobar',
17-
':method': 'GET',
18-
':scheme': 'http',
19-
':authority': `localhost:${port}`,
20-
'foo-bar': 'abc123'
21-
};
22-
23-
assert.strictEqual(request.path, undefined);
24-
assert.strictEqual(request.method, expected[':method']);
25-
assert.strictEqual(request.scheme, expected[':scheme']);
26-
assert.strictEqual(request.url, expected[':path']);
27-
assert.strictEqual(request.authority, expected[':authority']);
28-
29-
const headers = request.headers;
30-
for (const [name, value] of Object.entries(expected)) {
31-
assert.strictEqual(headers[name], value);
32-
}
33-
34-
const rawHeaders = request.rawHeaders;
35-
for (const [name, value] of Object.entries(expected)) {
36-
const position = rawHeaders.indexOf(name);
37-
assert.notStrictEqual(position, -1);
38-
assert.strictEqual(rawHeaders[position + 1], value);
39-
}
40-
41-
request.url = '/one';
42-
assert.strictEqual(request.url, '/one');
43-
44-
// Third-party plugins for packages like express use query params to
45-
// change the request method
46-
request.method = 'POST';
47-
assert.strictEqual(request.method, 'POST');
48-
assert.throws(
49-
() => request.method = ' ',
50-
{
51-
code: 'ERR_INVALID_ARG_VALUE',
52-
name: 'TypeError',
53-
message: "The argument 'method' is invalid. Received ' '"
9+
{
10+
// Http2ServerRequest should have header helpers
11+
12+
const server = h2.createServer();
13+
server.listen(0, common.mustCall(function() {
14+
const port = server.address().port;
15+
server.once('request', common.mustCall(function(request, response) {
16+
const expected = {
17+
':path': '/foobar',
18+
':method': 'GET',
19+
':scheme': 'http',
20+
':authority': `localhost:${port}`,
21+
'foo-bar': 'abc123'
22+
};
23+
24+
assert.strictEqual(request.path, undefined);
25+
assert.strictEqual(request.method, expected[':method']);
26+
assert.strictEqual(request.scheme, expected[':scheme']);
27+
assert.strictEqual(request.url, expected[':path']);
28+
assert.strictEqual(request.authority, expected[':authority']);
29+
30+
const headers = request.headers;
31+
for (const [name, value] of Object.entries(expected)) {
32+
assert.strictEqual(headers[name], value);
5433
}
55-
);
56-
assert.throws(
57-
() => request.method = true,
58-
{
59-
code: 'ERR_INVALID_ARG_TYPE',
60-
name: 'TypeError',
61-
message: 'The "method" argument must be of type string. ' +
62-
'Received type boolean (true)'
34+
35+
const rawHeaders = request.rawHeaders;
36+
for (const [name, value] of Object.entries(expected)) {
37+
const position = rawHeaders.indexOf(name);
38+
assert.notStrictEqual(position, -1);
39+
assert.strictEqual(rawHeaders[position + 1], value);
6340
}
64-
);
6541

66-
response.on('finish', common.mustCall(function() {
67-
server.close();
42+
request.url = '/one';
43+
assert.strictEqual(request.url, '/one');
44+
45+
// Third-party plugins for packages like express use query params to
46+
// change the request method
47+
request.method = 'POST';
48+
assert.strictEqual(request.method, 'POST');
49+
assert.throws(
50+
() => request.method = ' ',
51+
{
52+
code: 'ERR_INVALID_ARG_VALUE',
53+
name: 'TypeError',
54+
message: "The argument 'method' is invalid. Received ' '"
55+
}
56+
);
57+
assert.throws(
58+
() => request.method = true,
59+
{
60+
code: 'ERR_INVALID_ARG_TYPE',
61+
name: 'TypeError',
62+
message: 'The "method" argument must be of type string. ' +
63+
'Received type boolean (true)'
64+
}
65+
);
66+
67+
response.on('finish', common.mustCall(function() {
68+
server.close();
69+
}));
70+
response.end();
71+
}));
72+
73+
const url = `http://localhost:${port}`;
74+
const client = h2.connect(url, common.mustCall(function() {
75+
const headers = {
76+
':path': '/foobar',
77+
':method': 'GET',
78+
':scheme': 'http',
79+
':authority': `localhost:${port}`,
80+
'foo-bar': 'abc123'
81+
};
82+
const request = client.request(headers);
83+
request.on('end', common.mustCall(function() {
84+
client.close();
85+
}));
86+
request.end();
87+
request.resume();
6888
}));
69-
response.end();
7089
}));
90+
}
91+
92+
{
93+
// Http2ServerRequest should keep pseudo-headers order and after them,
94+
// in the same order, the others headers
95+
96+
const server = h2.createServer();
97+
server.listen(0, common.mustCall(function() {
98+
const port = server.address().port;
99+
server.once('request', common.mustCall(function(request, response) {
100+
const expected = {
101+
':path': '/foobar',
102+
':method': 'GET',
103+
':scheme': 'http',
104+
':authority': `localhost:${port}`,
105+
'foo1': 'abc1',
106+
'foo2': 'abc2'
107+
};
108+
109+
assert.strictEqual(request.path, undefined);
110+
assert.strictEqual(request.method, expected[':method']);
111+
assert.strictEqual(request.scheme, expected[':scheme']);
112+
assert.strictEqual(request.url, expected[':path']);
113+
assert.strictEqual(request.authority, expected[':authority']);
114+
115+
const headers = request.headers;
116+
for (const [name, value] of Object.entries(expected)) {
117+
assert.strictEqual(headers[name], value);
118+
}
119+
120+
const rawHeaders = request.rawHeaders;
121+
let expectedPosition = 0;
122+
for (const [name, value] of Object.entries(expected)) {
123+
const position = rawHeaders.indexOf(name);
124+
assert.strictEqual(position / 2, expectedPosition);
125+
assert.strictEqual(rawHeaders[position + 1], value);
126+
expectedPosition++;
127+
}
128+
129+
response.on('finish', common.mustCall(function() {
130+
server.close();
131+
}));
132+
response.end();
133+
}));
71134

72-
const url = `http://localhost:${port}`;
73-
const client = h2.connect(url, common.mustCall(function() {
74-
const headers = {
75-
':path': '/foobar',
76-
':method': 'GET',
77-
':scheme': 'http',
78-
':authority': `localhost:${port}`,
79-
'foo-bar': 'abc123'
80-
};
81-
const request = client.request(headers);
82-
request.on('end', common.mustCall(function() {
83-
client.close();
135+
const url = `http://localhost:${port}`;
136+
const client = h2.connect(url, common.mustCall(function() {
137+
const headers = {
138+
':path': '/foobar',
139+
':method': 'GET',
140+
'foo1': 'abc1',
141+
':scheme': 'http',
142+
'foo2': 'abc2',
143+
':authority': `localhost:${port}`
144+
};
145+
const request = client.request(headers);
146+
request.on('end', common.mustCall(function() {
147+
client.close();
148+
}));
149+
request.end();
150+
request.resume();
84151
}));
85-
request.end();
86-
request.resume();
87152
}));
88-
}));
153+
}

‎test/parallel/test-http2-multiheaders-raw.js‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -16,14 +16,14 @@ src.test = 'foo, bar, baz';
1616

1717
server.on('stream', common.mustCall((stream, headers, flags, rawHeaders) => {
1818
const expected = [
19-
':path',
20-
'/',
21-
':scheme',
22-
'http',
23-
':authority',
24-
`localhost:${server.address().port}`,
2519
':method',
2620
'GET',
21+
':authority',
22+
`localhost:${server.address().port}`,
23+
':scheme',
24+
'http',
25+
':path',
26+
'/',
2727
'www-authenticate',
2828
'foo',
2929
'www-authenticate',

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

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -98,8 +98,8 @@ const {
9898
{
9999
const headers = {
100100
'abc': 1,
101-
':status': 200,
102101
':path': 'abc',
102+
':status': 200,
103103
'xyz': [1, '2', { toString() { return '3'; } }, 4],
104104
'foo': [],
105105
'BAR': [1]
@@ -116,8 +116,8 @@ const {
116116
{
117117
const headers = {
118118
'abc': 1,
119-
':path': 'abc',
120119
':status': [200],
120+
':path': 'abc',
121121
':authority': [],
122122
'xyz': [1, 2, 3, 4]
123123
};
@@ -132,10 +132,10 @@ const {
132132
{
133133
const headers = {
134134
'abc': 1,
135-
':path': 'abc',
135+
':status': 200,
136136
'xyz': [1, 2, 3, 4],
137137
'': 1,
138-
':status': 200,
138+
':path': 'abc',
139139
[Symbol('test')]: 1 // Symbol keys are ignored
140140
};
141141

@@ -150,10 +150,10 @@ const {
150150
// Only own properties are used
151151
const base = { 'abc': 1 };
152152
const headers = Object.create(base);
153-
headers[':path'] = 'abc';
153+
headers[':status'] = 200;
154154
headers.xyz = [1, 2, 3, 4];
155155
headers.foo = [];
156-
headers[':status'] = 200;
156+
headers[':path'] = 'abc';
157157

158158
assert.deepStrictEqual(
159159
mapToHeaders(headers),
@@ -191,8 +191,8 @@ const {
191191
{
192192
const headers = {
193193
'abc': 1,
194-
':path': 'abc',
195194
':status': [200],
195+
':path': 'abc',
196196
':authority': [],
197197
'xyz': [1, 2, 3, 4],
198198
[sensitiveHeaders]: ['xyz']

0 commit comments

Comments
 (0)