Skip to content

Commit 0343ece

Browse files
apapirovskiMylesBorins
apapirovski
authored and
MylesBorins
committed
http2: fix refs to status 205, add tests
Fix references within http2 core to HTTP_STATUS_CONTENT_RESET to point to the correct HTTP_STATUS_RESET_CONTENT. Add tests for status 204, 205 & 304 in respond, respondWithFD & respondWithFile. Add general error tests for respondWithFD & respondWithFile. PR-URL: #15153 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Claudio Rodriguez <cjrodr@yahoo.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
1 parent d8ff550 commit 0343ece

4 files changed

Lines changed: 270 additions & 4 deletions

File tree

‎lib/internal/http2/core.js‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -121,7 +121,7 @@ const {
121121
HTTP2_METHOD_CONNECT,
122122

123123
HTTP_STATUS_CONTINUE,
124-
HTTP_STATUS_CONTENT_RESET,
124+
HTTP_STATUS_RESET_CONTENT,
125125
HTTP_STATUS_OK,
126126
HTTP_STATUS_NO_CONTENT,
127127
HTTP_STATUS_NOT_MODIFIED,
@@ -1879,7 +1879,7 @@ class ServerHttp2Stream extends Http2Stream {
18791879
// the options.endStream option to true so that the underlying
18801880
// bits do not attempt to send any.
18811881
if (statusCode === HTTP_STATUS_NO_CONTENT ||
1882-
statusCode === HTTP_STATUS_CONTENT_RESET ||
1882+
statusCode === HTTP_STATUS_RESET_CONTENT ||
18831883
statusCode === HTTP_STATUS_NOT_MODIFIED ||
18841884
state.headRequest === true) {
18851885
options.endStream = true;
@@ -1973,7 +1973,7 @@ class ServerHttp2Stream extends Http2Stream {
19731973
const statusCode = headers[HTTP2_HEADER_STATUS] |= 0;
19741974
// Payload/DATA frames are not permitted in these cases
19751975
if (statusCode === HTTP_STATUS_NO_CONTENT ||
1976-
statusCode === HTTP_STATUS_CONTENT_RESET ||
1976+
statusCode === HTTP_STATUS_RESET_CONTENT ||
19771977
statusCode === HTTP_STATUS_NOT_MODIFIED) {
19781978
throw new errors.Error('ERR_HTTP2_PAYLOAD_FORBIDDEN', statusCode);
19791979
}
@@ -2050,7 +2050,7 @@ class ServerHttp2Stream extends Http2Stream {
20502050
const statusCode = headers[HTTP2_HEADER_STATUS] |= 0;
20512051
// Payload/DATA frames are not permitted in these cases
20522052
if (statusCode === HTTP_STATUS_NO_CONTENT ||
2053-
statusCode === HTTP_STATUS_CONTENT_RESET ||
2053+
statusCode === HTTP_STATUS_RESET_CONTENT ||
20542054
statusCode === HTTP_STATUS_NOT_MODIFIED) {
20552055
throw new errors.Error('ERR_HTTP2_PAYLOAD_FORBIDDEN', statusCode);
20562056
}

‎test/parallel/test-http2-respond-file-errors.js‎

Lines changed: 103 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,103 @@
1+
// Flags: --expose-http2
2+
'use strict';
3+
4+
const common = require('../common');
5+
if (!common.hasCrypto)
6+
common.skip('missing crypto');
7+
const http2 = require('http2');
8+
const path = require('path');
9+
10+
const optionsWithTypeError = {
11+
offset: 'number',
12+
length: 'number',
13+
statCheck: 'function',
14+
getTrailers: 'function'
15+
};
16+
17+
const types = {
18+
boolean: true,
19+
function: () => {},
20+
number: 1,
21+
object: {},
22+
array: [],
23+
null: null,
24+
symbol: Symbol('test')
25+
};
26+
27+
const fname = path.resolve(common.fixturesDir, 'elipses.txt');
28+
29+
const server = http2.createServer();
30+
31+
server.on('stream', common.mustCall((stream) => {
32+
// Check for all possible TypeError triggers on options
33+
Object.keys(optionsWithTypeError).forEach((option) => {
34+
Object.keys(types).forEach((type) => {
35+
if (type === optionsWithTypeError[option]) {
36+
return;
37+
}
38+
39+
common.expectsError(
40+
() => stream.respondWithFile(fname, {
41+
[http2.constants.HTTP2_HEADER_CONTENT_TYPE]: 'text/plain'
42+
}, {
43+
[option]: types[type]
44+
}),
45+
{
46+
type: TypeError,
47+
code: 'ERR_INVALID_OPT_VALUE',
48+
message: `The value "${String(types[type])}" is invalid ` +
49+
`for option "${option}"`
50+
}
51+
);
52+
});
53+
});
54+
55+
// Should throw if :status 204, 205 or 304
56+
[204, 205, 304].forEach((status) => common.expectsError(
57+
() => stream.respondWithFile(fname, {
58+
[http2.constants.HTTP2_HEADER_CONTENT_TYPE]: 'text/plain',
59+
':status': status,
60+
}),
61+
{
62+
code: 'ERR_HTTP2_PAYLOAD_FORBIDDEN',
63+
message: `Responses with ${status} status must not have a payload`
64+
}
65+
));
66+
67+
// Should throw if headers already sent
68+
stream.respond({
69+
':status': 200,
70+
});
71+
common.expectsError(
72+
() => stream.respondWithFile(fname, {
73+
[http2.constants.HTTP2_HEADER_CONTENT_TYPE]: 'text/plain'
74+
}),
75+
{
76+
code: 'ERR_HTTP2_HEADERS_SENT',
77+
message: 'Response has already been initiated.'
78+
}
79+
);
80+
81+
// Should throw if stream already destroyed
82+
stream.destroy();
83+
common.expectsError(
84+
() => stream.respondWithFile(fname, {
85+
[http2.constants.HTTP2_HEADER_CONTENT_TYPE]: 'text/plain'
86+
}),
87+
{
88+
code: 'ERR_HTTP2_INVALID_STREAM',
89+
message: 'The stream has been destroyed'
90+
}
91+
);
92+
}));
93+
94+
server.listen(0, common.mustCall(() => {
95+
const client = http2.connect(`http://localhost:${server.address().port}`);
96+
const req = client.request();
97+
98+
req.on('streamClosed', common.mustCall(() => {
99+
client.destroy();
100+
server.close();
101+
}));
102+
req.end();
103+
}));

‎test/parallel/test-http2-respond-file-fd-errors.js‎

Lines changed: 123 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,123 @@
1+
// Flags: --expose-http2
2+
'use strict';
3+
4+
const common = require('../common');
5+
if (!common.hasCrypto)
6+
common.skip('missing crypto');
7+
const http2 = require('http2');
8+
const path = require('path');
9+
const fs = require('fs');
10+
11+
const optionsWithTypeError = {
12+
offset: 'number',
13+
length: 'number',
14+
statCheck: 'function',
15+
getTrailers: 'function'
16+
};
17+
18+
const types = {
19+
boolean: true,
20+
function: () => {},
21+
number: 1,
22+
object: {},
23+
array: [],
24+
null: null,
25+
symbol: Symbol('test')
26+
};
27+
28+
const fname = path.resolve(common.fixturesDir, 'elipses.txt');
29+
const fd = fs.openSync(fname, 'r');
30+
31+
const server = http2.createServer();
32+
33+
server.on('stream', common.mustCall((stream) => {
34+
// should throw if fd isn't a number
35+
Object.keys(types).forEach((type) => {
36+
if (type === 'number') {
37+
return;
38+
}
39+
40+
common.expectsError(
41+
() => stream.respondWithFD(types[type], {
42+
[http2.constants.HTTP2_HEADER_CONTENT_TYPE]: 'text/plain'
43+
}),
44+
{
45+
type: TypeError,
46+
code: 'ERR_INVALID_ARG_TYPE',
47+
message: 'The "fd" argument must be of type number'
48+
}
49+
);
50+
});
51+
52+
// Check for all possible TypeError triggers on options
53+
Object.keys(optionsWithTypeError).forEach((option) => {
54+
Object.keys(types).forEach((type) => {
55+
if (type === optionsWithTypeError[option]) {
56+
return;
57+
}
58+
59+
common.expectsError(
60+
() => stream.respondWithFD(fd, {
61+
[http2.constants.HTTP2_HEADER_CONTENT_TYPE]: 'text/plain'
62+
}, {
63+
[option]: types[type]
64+
}),
65+
{
66+
type: TypeError,
67+
code: 'ERR_INVALID_OPT_VALUE',
68+
message: `The value "${String(types[type])}" is invalid ` +
69+
`for option "${option}"`
70+
}
71+
);
72+
});
73+
});
74+
75+
// Should throw if :status 204, 205 or 304
76+
[204, 205, 304].forEach((status) => common.expectsError(
77+
() => stream.respondWithFD(fd, {
78+
[http2.constants.HTTP2_HEADER_CONTENT_TYPE]: 'text/plain',
79+
':status': status,
80+
}),
81+
{
82+
code: 'ERR_HTTP2_PAYLOAD_FORBIDDEN',
83+
message: `Responses with ${status} status must not have a payload`
84+
}
85+
));
86+
87+
// Should throw if headers already sent
88+
stream.respond({
89+
':status': 200,
90+
});
91+
common.expectsError(
92+
() => stream.respondWithFD(fd, {
93+
[http2.constants.HTTP2_HEADER_CONTENT_TYPE]: 'text/plain'
94+
}),
95+
{
96+
code: 'ERR_HTTP2_HEADERS_SENT',
97+
message: 'Response has already been initiated.'
98+
}
99+
);
100+
101+
// Should throw if stream already destroyed
102+
stream.destroy();
103+
common.expectsError(
104+
() => stream.respondWithFD(fd, {
105+
[http2.constants.HTTP2_HEADER_CONTENT_TYPE]: 'text/plain'
106+
}),
107+
{
108+
code: 'ERR_HTTP2_INVALID_STREAM',
109+
message: 'The stream has been destroyed'
110+
}
111+
);
112+
}));
113+
114+
server.listen(0, common.mustCall(() => {
115+
const client = http2.connect(`http://localhost:${server.address().port}`);
116+
const req = client.request();
117+
118+
req.on('streamClosed', common.mustCall(() => {
119+
client.destroy();
120+
server.close();
121+
}));
122+
req.end();
123+
}));

‎test/parallel/test-http2-respond-no-data.js‎

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,40 @@
1+
// Flags: --expose-http2
2+
'use strict';
3+
4+
const common = require('../common');
5+
if (!common.hasCrypto)
6+
common.skip('missing crypto');
7+
const http2 = require('http2');
8+
const assert = require('assert');
9+
10+
const server = http2.createServer();
11+
12+
// Check that stream ends immediately after respond on :status 204, 205 & 304
13+
14+
const status = [204, 205, 304];
15+
16+
server.on('stream', common.mustCall((stream) => {
17+
stream.on('streamClosed', common.mustCall(() => {
18+
assert.strictEqual(stream.destroyed, true);
19+
}));
20+
stream.respond({ ':status': status.shift() });
21+
}, 3));
22+
23+
server.listen(0, common.mustCall(makeRequest));
24+
25+
function makeRequest() {
26+
const client = http2.connect(`http://localhost:${server.address().port}`);
27+
const req = client.request();
28+
req.resume();
29+
30+
req.on('end', common.mustCall(() => {
31+
client.destroy();
32+
33+
if (!status.length) {
34+
server.close();
35+
} else {
36+
makeRequest();
37+
}
38+
}));
39+
req.end();
40+
}

0 commit comments

Comments
 (0)