Skip to content

Commit

Permalink
http2: adjust error types, test coverage
Browse files Browse the repository at this point in the history
Change error types emitted from request and validatePriorityOptions
to be TypeError rather than the incorrect RangeError. Add tests
to confirm that all errors are thrown as expected (weight, parent,
exclusive, silent, endStream and getTrailers). Add test for method
CONNECT throwing error on missing :authority or superfluous :scheme
and :path.

PR-URL: #15109
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
  • Loading branch information
apapirovski authored and MylesBorins committed Sep 12, 2017
1 parent e3e9e50 commit fd51cb8
Show file tree
Hide file tree
Showing 3 changed files with 114 additions and 15 deletions.
30 changes: 15 additions & 15 deletions lib/internal/http2/core.js
Original file line number Diff line number Diff line change
Expand Up @@ -485,39 +485,39 @@ function validatePriorityOptions(options) {
if (options.weight === undefined) {
options.weight = NGHTTP2_DEFAULT_WEIGHT;
} else if (typeof options.weight !== 'number') {
const err = new errors.RangeError('ERR_INVALID_OPT_VALUE',
'weight',
options.weight);
const err = new errors.TypeError('ERR_INVALID_OPT_VALUE',
'weight',
options.weight);
Error.captureStackTrace(err, validatePriorityOptions);
throw err;
}

if (options.parent === undefined) {
options.parent = 0;
} else if (typeof options.parent !== 'number' || options.parent < 0) {
const err = new errors.RangeError('ERR_INVALID_OPT_VALUE',
'parent',
options.parent);
const err = new errors.TypeError('ERR_INVALID_OPT_VALUE',
'parent',
options.parent);
Error.captureStackTrace(err, validatePriorityOptions);
throw err;
}

if (options.exclusive === undefined) {
options.exclusive = false;
} else if (typeof options.exclusive !== 'boolean') {
const err = new errors.RangeError('ERR_INVALID_OPT_VALUE',
'exclusive',
options.exclusive);
const err = new errors.TypeError('ERR_INVALID_OPT_VALUE',
'exclusive',
options.exclusive);
Error.captureStackTrace(err, validatePriorityOptions);
throw err;
}

if (options.silent === undefined) {
options.silent = false;
} else if (typeof options.silent !== 'boolean') {
const err = new errors.RangeError('ERR_INVALID_OPT_VALUE',
'silent',
options.silent);
const err = new errors.TypeError('ERR_INVALID_OPT_VALUE',
'silent',
options.silent);
Error.captureStackTrace(err, validatePriorityOptions);
throw err;
}
Expand Down Expand Up @@ -1119,9 +1119,9 @@ class ClientHttp2Session extends Http2Session {
// preference.
options.endStream = isPayloadMeaningless(headers[HTTP2_HEADER_METHOD]);
} else if (typeof options.endStream !== 'boolean') {
throw new errors.RangeError('ERR_INVALID_OPT_VALUE',
'endStream',
options.endStream);
throw new errors.TypeError('ERR_INVALID_OPT_VALUE',
'endStream',
options.endStream);
}

if (options.getTrailers !== undefined &&
Expand Down
63 changes: 63 additions & 0 deletions test/parallel/test-http2-client-request-options-errors.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
// Flags: --expose-http2
'use strict';

const common = require('../common');
if (!common.hasCrypto)
common.skip('missing crypto');
const assert = require('assert');
const http2 = require('http2');

// Check if correct errors are emitted when wrong type of data is passed
// to certain options of ClientHttp2Session request method

const optionsToTest = {
endStream: 'boolean',
getTrailers: 'function',
weight: 'number',
parent: 'number',
exclusive: 'boolean',
silent: 'boolean'
};

const types = {
boolean: true,
function: () => {},
number: 1,
object: {},
array: [],
null: null,
symbol: Symbol('test')
};

const server = http2.createServer(common.mustNotCall());

server.listen(0, common.mustCall(() => {
const port = server.address().port;
const client = http2.connect(`http://localhost:${port}`);

Object.keys(optionsToTest).forEach((option) => {
Object.keys(types).forEach((type) => {
if (type === optionsToTest[option]) {
return;
}

assert.throws(
() => client.request({
':method': 'CONNECT',
':authority': `localhost:${port}`
}, {
[option]: types[type]
}),
common.expectsError({
type: TypeError,
code: 'ERR_INVALID_OPT_VALUE',
message: `The value "${String(types[type])}" is invalid ` +
`for option "${option}"`
})
);
});
});

server.close();
client.destroy();
}));
36 changes: 36 additions & 0 deletions test/parallel/test-http2-connect-method.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@ const { URL } = require('url');
const {
HTTP2_HEADER_METHOD,
HTTP2_HEADER_AUTHORITY,
HTTP2_HEADER_SCHEME,
HTTP2_HEADER_PATH,
NGHTTP2_CONNECT_ERROR
} = http2.constants;

Expand Down Expand Up @@ -53,6 +55,40 @@ server.listen(0, common.mustCall(() => {
proxy.listen(0, () => {
const client = http2.connect(`http://localhost:${proxy.address().port}`);

// confirm that :authority is required and :scheme & :path are forbidden
assert.throws(
() => client.request({
[HTTP2_HEADER_METHOD]: 'CONNECT'
}),
common.expectsError({
code: 'ERR_HTTP2_CONNECT_AUTHORITY',
message: ':authority header is required for CONNECT requests'
})
);
assert.throws(
() => client.request({
[HTTP2_HEADER_METHOD]: 'CONNECT',
[HTTP2_HEADER_AUTHORITY]: `localhost:${port}`,
[HTTP2_HEADER_SCHEME]: 'http'
}),
common.expectsError({
code: 'ERR_HTTP2_CONNECT_SCHEME',
message: 'The :scheme header is forbidden for CONNECT requests'
})
);
assert.throws(
() => client.request({
[HTTP2_HEADER_METHOD]: 'CONNECT',
[HTTP2_HEADER_AUTHORITY]: `localhost:${port}`,
[HTTP2_HEADER_PATH]: '/'
}),
common.expectsError({
code: 'ERR_HTTP2_CONNECT_PATH',
message: 'The :path header is forbidden for CONNECT requests'
})
);

// valid CONNECT request
const req = client.request({
[HTTP2_HEADER_METHOD]: 'CONNECT',
[HTTP2_HEADER_AUTHORITY]: `localhost:${port}`,
Expand Down

0 comments on commit fd51cb8

Please sign in to comment.