From 99699c3f172cc5d4ed1f3d9db07d7ad8183dba67 Mon Sep 17 00:00:00 2001 From: Daniel Bevenius Date: Tue, 20 Jun 2017 08:30:47 +0200 Subject: [PATCH 1/3] tools: add eslint rule for hasCrypto checking The motivation for this commit is to pick up early on missing checks for crypto support (when Node is built --without-ssl). --- test/.eslintrc.yaml | 1 + tools/eslint-rules/crypto-check.js | 83 ++++++++++++++++++++++++++++++ tools/eslint-rules/rules-utils.js | 61 ++++++++++++++++++++++ 3 files changed, 145 insertions(+) create mode 100644 tools/eslint-rules/crypto-check.js create mode 100644 tools/eslint-rules/rules-utils.js diff --git a/test/.eslintrc.yaml b/test/.eslintrc.yaml index aeaf09fb0ff732..b571d3a1b6e0a9 100644 --- a/test/.eslintrc.yaml +++ b/test/.eslintrc.yaml @@ -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] diff --git a/tools/eslint-rules/crypto-check.js b/tools/eslint-rules/crypto-check.js new file mode 100644 index 00000000000000..b1b2a03f50e3b6 --- /dev/null +++ b/tools/eslint-rules/crypto-check.js @@ -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 + */ +'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) + }; +}; diff --git a/tools/eslint-rules/rules-utils.js b/tools/eslint-rules/rules-utils.js new file mode 100644 index 00000000000000..e3e5e6e5ef9718 --- /dev/null +++ b/tools/eslint-rules/rules-utils.js @@ -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'); +} From b4924a30587865d0adc323aebc1c7955919b054f Mon Sep 17 00:00:00 2001 From: Daniel Bevenius Date: Tue, 27 Jun 2017 08:15:07 +0200 Subject: [PATCH 2/3] test: add eslint-disable crypto-check 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. --- test/common/index.js | 2 +- test/parallel/test-async-wrap-getasyncid.js | 6 +++--- test/parallel/test-buffer-alloc.js | 2 +- test/parallel/test-buffer-concat.js | 1 + test/parallel/test-http2-noflag.js | 2 +- 5 files changed, 7 insertions(+), 6 deletions(-) diff --git a/test/common/index.js b/test/common/index.js index 8175474818b9dc..b79c1a57559e1a 100644 --- a/test/common/index.js +++ b/test/common/index.js @@ -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'); diff --git a/test/parallel/test-async-wrap-getasyncid.js b/test/parallel/test-async-wrap-getasyncid.js index 4c1ea8e212e7ef..95317f711c9907 100644 --- a/test/parallel/test-async-wrap-getasyncid.js +++ b/test/parallel/test-async-wrap-getasyncid.js @@ -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, @@ -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(); diff --git a/test/parallel/test-buffer-alloc.js b/test/parallel/test-buffer-alloc.js index b62a192b126380..2843e9c9db727a 100644 --- a/test/parallel/test-buffer-alloc.js +++ b/test/parallel/test-buffer-alloc.js @@ -909,7 +909,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'); diff --git a/test/parallel/test-buffer-concat.js b/test/parallel/test-buffer-concat.js index 9f3acbc599c3eb..87ccb720b7497a 100644 --- a/test/parallel/test-buffer-concat.js +++ b/test/parallel/test-buffer-concat.js @@ -62,6 +62,7 @@ function assertWrongList(value) { })); } +// eslint-disable-next-line crypto-check const random10 = common.hasCrypto ? require('crypto').randomBytes(10) : Buffer.alloc(10, 1); diff --git a/test/parallel/test-http2-noflag.js b/test/parallel/test-http2-noflag.js index a1e0e8b72c79e9..903fd649c5e458 100644 --- a/test/parallel/test-http2-noflag.js +++ b/test/parallel/test-http2-noflag.js @@ -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'$/); From 11dc3d09cc6b7dab6b2cb28f9c21e33f2c55a2e0 Mon Sep 17 00:00:00 2001 From: Daniel Bevenius Date: Tue, 20 Jun 2017 08:30:47 +0200 Subject: [PATCH 3/3] tools: add eslint rule for inspector checking The motivation for this commit is to pick up early on missing checks for inspector support (when Node is built --without-inspector). --- test/.eslintrc.yaml | 1 + tools/eslint-rules/inspector-check.js | 43 +++++++++++++++++++++++++++ 2 files changed, 44 insertions(+) create mode 100644 tools/eslint-rules/inspector-check.js diff --git a/test/.eslintrc.yaml b/test/.eslintrc.yaml index b571d3a1b6e0a9..58e072c1951e27 100644 --- a/test/.eslintrc.yaml +++ b/test/.eslintrc.yaml @@ -11,5 +11,6 @@ rules: prefer-assert-methods: error prefer-common-mustnotcall: error crypto-check: error + inspector-check: error ## common module is mandatory in tests required-modules: [error, common] diff --git a/tools/eslint-rules/inspector-check.js b/tools/eslint-rules/inspector-check.js new file mode 100644 index 00000000000000..f225b34cb6b0ca --- /dev/null +++ b/tools/eslint-rules/inspector-check.js @@ -0,0 +1,43 @@ +/** + * @fileoverview Check that common.skipIfInspectorDisabled is used if + * the inspector module is required. + * @author Daniel Bevenius + */ +'use strict'; + +const utils = require('./rules-utils.js'); + +//------------------------------------------------------------------------------ +// Rule Definition +//------------------------------------------------------------------------------ +const msg = 'Please add a skipIfInspectorDisabled() call to allow this ' + + 'test to be skippped when Node is built \'--without-inspector\'.'; + +module.exports = function(context) { + var usesInspector = false; + var hasInspectorCheck = false; + + function testInspectorUsage(context, node) { + if (utils.isRequired(node, ['inspector'])) { + usesInspector = true; + } + } + + function checkMemberExpression(context, node) { + if (utils.usesCommonProperty(node, ['skipIfInspectorDisabled'])) { + hasInspectorCheck = true; + } + } + + function reportIfMissing(context, node) { + if (usesInspector && !hasInspectorCheck) { + context.report(node, msg); + } + } + + return { + 'CallExpression': (node) => testInspectorUsage(context, node), + 'MemberExpression': (node) => checkMemberExpression(context, node), + 'Program:exit': (node) => reportIfMissing(context, node) + }; +};