Skip to content

https: disallow boolean types for key and cert options #14807

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
jimmycann wants to merge 4 commits into from
Closed

https: disallow boolean types for key and cert options #14807

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
262d4b2
https: disallow boolean types for `key` and `cert` options
jimmycann Aug 13, 2017
e92e64a
tls: disallow boolean types for `key`, `cert` and `ca` options
jimmycann Aug 14, 2017
b132c96
tls: type checking for `key`, `cert` and `ca` options
jimmycann Aug 16, 2017
c46fdd4
tls: type checking for `key`, `cert` and `ca` options
jimmycann Aug 16, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions lib/https.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,12 @@ function Server(opts, requestListener) {
}
opts = util._extend({}, opts);

if (opts && typeof opts.key === 'boolean')
Copy link
Copy Markdown
Member

@Trott Trott Aug 13, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Line 41 guarantee that opts is an object so there is no need to check opts here or in line 46.

Copy link
Copy Markdown
Member

@Trott Trott Aug 13, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think checking against all invalid values is the way to go. I think it should confirm it is a valid value and reject everything else.

throw new Error('"options.key" must not be a boolean');
Copy link
Copy Markdown
Member

@Trott Trott Aug 13, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This and the other Error should be a TypeError.


if (opts && typeof opts.cert === 'boolean')
throw new Error('"options.cert" must not be a boolean');
Copy link
Copy Markdown
Member

@addaleax addaleax Aug 13, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By the way, we try to migrate to a new error system that allows for more flexibility in the future, it would be cool if you think you could take a look at using that.


if (process.features.tls_npn && !opts.NPNProtocols) {
opts.NPNProtocols = ['http/1.1', 'http/1.0'];
}
Expand Down
84 changes: 84 additions & 0 deletions test/parallel/test-https-options-boolean-check.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,84 @@
// Copyright Joyent, Inc. and other Node contributors.
//
// Permission is hereby granted, free of charge, to any person obtaining a
// copy of this software and associated documentation files (the
// "Software"), to deal in the Software without restriction, including
// without limitation the rights to use, copy, modify, merge, publish,
// distribute, sublicense, and/or sell copies of the Software, and to permit
// persons to whom the Software is furnished to do so, subject to the
// following conditions:
//
// The above copyright notice and this permission notice shall be included
// in all copies or substantial portions of the Software.
//
// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS
// OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
// MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN
// NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM,
// DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
// OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE
// USE OR OTHER DEALINGS IN THE SOFTWARE.
Copy link
Copy Markdown
Member

@addaleax addaleax Aug 13, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don’t need the copyright header for new files.


'use strict';
const common = require('../common');

if (!common.hasCrypto)
common.skip('missing crypto');

const assert = require('assert');
const https = require('https');
const fs = require('fs');

assert.doesNotThrow(() =>
https.createServer({
key: fs.readFileSync(`${common.fixturesDir}/keys/agent1-key.pem`),
cert: fs.readFileSync(`${common.fixturesDir}/keys/agent1-cert.pem`)
}));

assert.throws(() =>
Copy link
Copy Markdown
Contributor

@cjihrig cjihrig Aug 13, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can reduce code duplication a lot here. All of these assert.throws() calls are essentially the same, with only the value of key, cert, and the regular expression changing. You can do something like this instead:

const key = fs.readFileSync(`${common.fixturesDir}/keys/agent1-key.pem`);
const cert = fs.readFileSync(`${common.fixturesDir}/keys/agent1-cert.pem`);
const invalidKeyRE = /^"options\.key" must not be a boolean$/;
const invalidCertRE = /^"options\.cert" must not be a boolean$/;

[
  [true, cert, invalidKeyRE],
  // Fill in other cases here
].forEach((params) => {
  assert.throws(() => {
    https.createServer({
      key: params[0],
      cert: params[1]
    });
  }, params[2]);
});

https.createServer({
key: true,
cert: fs.readFileSync(`${common.fixturesDir}/keys/agent1-cert.pem`)
}), /"options\.key" must not be a boolean/);
Copy link
Copy Markdown
Member

@Trott Trott Aug 13, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These regular expressions would be better if they matched the entire message (that is, if they started with ^ and ended with $).


assert.throws(() =>
https.createServer({
key: false,
cert: fs.readFileSync(`${common.fixturesDir}/keys/agent1-cert.pem`)
}), /"options\.key" must not be a boolean/);

assert.throws(() =>
https.createServer({
key: fs.readFileSync(`${common.fixturesDir}/keys/agent1-key.pem`),
cert: true
}), /"options\.cert" must not be a boolean/);

assert.throws(() =>
https.createServer({
key: fs.readFileSync(`${common.fixturesDir}/keys/agent1-key.pem`),
cert: false
}), /"options\.cert" must not be a boolean/);

assert.throws(() =>
https.createServer({
key: false,
cert: false
}), /"options\.key" must not be a boolean/);

assert.throws(() =>
https.createServer({
key: true,
cert: true
}), /"options\.key" must not be a boolean/);

assert.throws(() =>
https.createServer({
key: true,
cert: false
}), /"options\.key" must not be a boolean/);

assert.throws(() =>
https.createServer({
key: false,
cert: true
}), /"options\.key" must not be a boolean/);