Skip to content
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

Jump to bottom

tls: remove util and calls to util.format #3456

Closed
wants to merge 1 commit into from
Closed

tls: remove util and calls to util.format #3456

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
18 changes: 6 additions & 12 deletions lib/tls.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@

const net = require('net');
const url = require('url');
const util = require('util');
const binding = process.binding('crypto');
const Buffer = require('buffer').Buffer;
const constants = require('constants');
Expand Down Expand Up @@ -141,9 +140,7 @@ exports.checkServerIdentity = function checkServerIdentity(host, cert) {
return ip === host;
});
if (!valid) {
reason = util.format('IP: %s is not in the cert\'s list: %s',
host,
ips.join(', '));
reason = `IP: ${host} is not in the cert's list: ${ips.join(', ')}`;
}
} else if (cert.subject) {
// Transform hostname to canonical form
Expand Down Expand Up @@ -189,13 +186,11 @@ exports.checkServerIdentity = function checkServerIdentity(host, cert) {

if (!valid) {
if (cert.subjectaltname) {
reason = util.format('Host: %s is not in the cert\'s altnames: %s',
host,
cert.subjectaltname);
reason =
`Host: ${host} is not in the cert's altnames: ` +
`${cert.subjectaltname}`;
} else {
reason = util.format('Host: %s is not cert\'s CN: %s',
host,
cert.subject.CN);
reason = `Host: ${host} is not cert's CN: ${cert.subject.CN}`;
}
}
} else {
Expand All @@ -204,8 +199,7 @@ exports.checkServerIdentity = function checkServerIdentity(host, cert) {

if (!valid) {
var err = new Error(
util.format('Hostname/IP doesn\'t match certificate\'s altnames: %j',
reason));
`Hostname/IP doesn't match certificate's altnames: "${reason}"`);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why use quotes around the code it at the end here?

Copy link
Member

Choose a reason for hiding this comment

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

It's because of the %j (JSON.stringify) it replaces. reason is always a string so putting it in quotes looks right to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

JSON.stringify does offer protection for circular references... I don't think reason would cause that, but would appreciate a +1 that we don't have to worry about that.

As an aside, how do backticks handle circular refs?

Copy link
Member

Choose a reason for hiding this comment

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

It's always a string, right? It can't have circular references.

As an aside, how do backticks handle circular refs?

They don't, interpolated values are simply stringified with .toString().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was assuming it was always a string but didn't dig through the code (best to dummy check :D)

Copy link
Member

Choose a reason for hiding this comment

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

Not well.

var m = {};
m.n = m;
console.log(`${m}`);

Outputs: "[object Object]"

Looking this over, I think it's safer to go ahead and do a variation on the JSON.stringify performed by the original version:

function stringify(val) {
  try {
    return JSON.stringify(val);
  } catch(_) {
    return '[Circular]';
  }
};
// ...
// ...
`Hostname/IP doesn't match certificate's altnames: ${stringify(reason)}`;

Copy link
Member

Choose a reason for hiding this comment

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

Actually, yeah, looking at it, reason is always a string in this case so nevermind 👍

err.reason = reason;
err.host = host;
err.cert = cert;
Expand Down