Skip to content

Commit

Permalink
tools: add eslint rule for hasCrypto checking
Browse files Browse the repository at this point in the history
The motivation for this commit is to pick up early on missing checks for
crypto support (when Node is built --without-ssl).

There are currently usages of common.hasCrypto which are not just for
detecting if crypto support is available and then skip the test in
question. For these case we still want to have a lint error generated
which can then be disabled using an ESLint comment.

PR-URL: #13813
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Teddy Katz <teddy.katz@gmail.com>
  • Loading branch information
danbev authored and MylesBorins committed Sep 12, 2017
1 parent 959b270 commit 1d97ff4
Show file tree
Hide file tree
Showing 8 changed files with 152 additions and 6 deletions.
1 change: 1 addition & 0 deletions test/.eslintrc.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -10,5 +10,6 @@ rules:
prefer-assert-iferror: error
prefer-assert-methods: error
prefer-common-mustnotcall: error
crypto-check: error
## common module is mandatory in tests
required-modules: [error, common]
2 changes: 1 addition & 1 deletion test/common/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
// OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE
// USE OR OTHER DEALINGS IN THE SOFTWARE.

/* eslint-disable required-modules */
/* eslint-disable required-modules, crypto-check */
'use strict';
const path = require('path');
const fs = require('fs');
Expand Down
6 changes: 3 additions & 3 deletions test/parallel/test-async-wrap-getasyncid.js
Original file line number Diff line number Diff line change
Expand Up @@ -81,14 +81,14 @@ function testInitialized(req, ctor_name) {
}


if (common.hasCrypto) {
if (common.hasCrypto) { // eslint-disable-line crypto-check
const tls = require('tls');
// SecurePair
testInitialized(tls.createSecurePair().ssl, 'Connection');
}


if (common.hasCrypto) {
if (common.hasCrypto) { // eslint-disable-line crypto-check
const crypto = require('crypto');

// The handle for PBKDF2 and RandomBytes isn't returned by the function call,
Expand Down Expand Up @@ -215,7 +215,7 @@ if (common.hasCrypto) {
}


if (common.hasCrypto) {
if (common.hasCrypto) { // eslint-disable-line crypto-check
const TCP = process.binding('tcp_wrap').TCP;
const tcp = new TCP();

Expand Down
2 changes: 1 addition & 1 deletion test/parallel/test-buffer-alloc.js
Original file line number Diff line number Diff line change
Expand Up @@ -905,7 +905,7 @@ assert.throws(() => Buffer.from('', 'buffer'),
}
}

if (common.hasCrypto) {
if (common.hasCrypto) { // eslint-disable-line crypto-check
// Test truncation after decode
const crypto = require('crypto');

Expand Down
1 change: 1 addition & 0 deletions test/parallel/test-buffer-concat.js
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ function assertWrongList(value) {
});
}

// eslint-disable-next-line crypto-check
const random10 = common.hasCrypto ?
require('crypto').randomBytes(10) :
Buffer.alloc(10, 1);
Expand Down
2 changes: 1 addition & 1 deletion test/parallel/test-http2-noflag.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,5 +4,5 @@
require('../common');
const assert = require('assert');

assert.throws(() => require('http2'),
assert.throws(() => require('http2'), // eslint-disable-line crypto-check
/^Error: Cannot find module 'http2'$/);
83 changes: 83 additions & 0 deletions tools/eslint-rules/crypto-check.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,83 @@
/**
* @fileoverview Check that common.hasCrypto is used if crypto, tls,
* https, or http2 modules are required.
*
* This rule can be ignored using // eslint-disable-line crypto-check
*
* @author Daniel Bevenius <daniel.bevenius@gmail.com>
*/
'use strict';

const utils = require('./rules-utils.js');

//------------------------------------------------------------------------------
// Rule Definition
//------------------------------------------------------------------------------
const msg = 'Please add a hasCrypto check to allow this test to be skipped ' +
'when Node is built "--without-ssl".';

module.exports = function(context) {
const missingCheckNodes = [];
const requireNodes = [];
var hasSkipCall = false;

function testCryptoUsage(node) {
if (utils.isRequired(node, ['crypto', 'tls', 'https', 'http2'])) {
requireNodes.push(node);
}
}

function testIfStatement(node) {
if (node.test.argument === undefined) {
return;
}
if (isCryptoCheck(node.test.argument)) {
checkCryptoCall(node);
}
}

function isCryptoCheck(node) {
return utils.usesCommonProperty(node, ['hasCrypto', 'hasFipsCrypto']);
}

function checkCryptoCall(node) {
if (utils.inSkipBlock(node)) {
hasSkipCall = true;
} else {
missingCheckNodes.push(node);
}
}

function testMemberExpression(node) {
if (isCryptoCheck(node)) {
checkCryptoCall(node);
}
}

function reportIfMissingCheck(node) {
if (hasSkipCall) {
return;
}

if (requireNodes.length > 0) {
if (missingCheckNodes.length > 0) {
report(missingCheckNodes);
} else {
report(requireNodes);
}
}
}

function report(nodes) {
nodes.forEach((node) => {
context.report(node, msg);
});
}

return {
'CallExpression': (node) => testCryptoUsage(node),
'IfStatement:exit': (node) => testIfStatement(node),
'MemberExpression:exit': (node) => testMemberExpression(node),
'Program:exit': (node) => reportIfMissingCheck(node)
};
};
61 changes: 61 additions & 0 deletions tools/eslint-rules/rules-utils.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
/**
* Utility functions common to ESLint rules.
*/
'use strict';

/**
* Returns true if any of the passed in modules are used in
* require calls.
*/
module.exports.isRequired = function(node, modules) {
return node.callee.name === 'require' &&
modules.includes(node.arguments[0].value);
};

/**
* Returns true is the node accesses any property in the properties
* array on the 'common' object.
*/
module.exports.usesCommonProperty = function(node, properties) {
if (node.name) {
return properties.includes(node.name);
}
if (node.property) {
return properties.includes(node.property.name);
}
return false;
};

/**
* Returns true if the passed in node is inside an if statement block,
* and the block also has a call to skip.
*/
module.exports.inSkipBlock = function(node) {
var hasSkipBlock = false;
if (node.test &&
node.test.type === 'UnaryExpression' &&
node.test.operator === '!') {
const consequent = node.consequent;
if (consequent.body) {
consequent.body.some(function(expressionStatement) {
if (hasSkip(expressionStatement.expression)) {
return hasSkipBlock = true;
}
return false;
});
} else {
if (hasSkip(consequent.expression)) {
hasSkipBlock = true;
}
}
}
return hasSkipBlock;
};

function hasSkip(expression) {
return expression &&
expression.callee &&
(expression.callee.name === 'skip' ||
expression.callee.property &&
expression.callee.property.name === 'skip');
}

0 comments on commit 1d97ff4

Please sign in to comment.