From 381b28e5285aa1b2d79e9ce19dae8b62ada0085e Mon Sep 17 00:00:00 2001 From: vilchik-elena Date: Tue, 11 Aug 2020 11:32:36 +0200 Subject: [PATCH 1/2] Rule S4507: debug features activated in production (deprecate S1442 and S1525) --- eslint-bridge/src/rules/main.ts | 2 + eslint-bridge/src/rules/production-debug.ts | 107 +++++++++++++++ .../tests/rules/production-debug.test.ts | 124 ++++++++++++++++++ .../javascript-S4507.json | 5 + .../expected/js/jshint/javascript-S4507.json | 12 ++ .../expected/js/p5.js/javascript-S4507.json | 13 ++ .../js/prototype/javascript-S4507.json | 5 + .../ts/TypeScript/typescript-S4507.json | 5 + .../expected/ts/console/typescript-S4507.json | 12 ++ .../sonar/javascript/checks/CheckList.java | 1 + .../checks/ProductionDebugCheck.java | 36 +++++ .../javascript/rules/javascript/S1442.html | 3 +- .../javascript/rules/javascript/S1442.json | 2 +- .../javascript/rules/javascript/S1525.html | 3 +- .../javascript/rules/javascript/S1525.json | 2 +- .../javascript/rules/javascript/S4507.html | 61 +++++++++ .../javascript/rules/javascript/S4507.json | 33 +++++ .../rules/javascript/Sonar_way_profile.json | 1 + .../Sonar_way_recommended_profile.json | 1 + 19 files changed, 424 insertions(+), 4 deletions(-) create mode 100644 eslint-bridge/src/rules/production-debug.ts create mode 100644 eslint-bridge/tests/rules/production-debug.test.ts create mode 100644 its/ruling/src/test/expected/js/javascript-test-sources/javascript-S4507.json create mode 100644 its/ruling/src/test/expected/js/jshint/javascript-S4507.json create mode 100644 its/ruling/src/test/expected/js/p5.js/javascript-S4507.json create mode 100644 its/ruling/src/test/expected/js/prototype/javascript-S4507.json create mode 100644 its/ruling/src/test/expected/ts/TypeScript/typescript-S4507.json create mode 100644 its/ruling/src/test/expected/ts/console/typescript-S4507.json create mode 100644 javascript-checks/src/main/java/org/sonar/javascript/checks/ProductionDebugCheck.java create mode 100644 javascript-checks/src/main/resources/org/sonar/l10n/javascript/rules/javascript/S4507.html create mode 100644 javascript-checks/src/main/resources/org/sonar/l10n/javascript/rules/javascript/S4507.json diff --git a/eslint-bridge/src/rules/main.ts b/eslint-bridge/src/rules/main.ts index b77028dfa0..79393dc9b2 100644 --- a/eslint-bridge/src/rules/main.ts +++ b/eslint-bridge/src/rules/main.ts @@ -98,6 +98,7 @@ import { rule as preferDefaultLast } from './prefer-default-last'; import { rule as preferPromiseShorthand } from './prefer-promise-shorthand'; import { rule as preferTypeGuard } from './prefer-type-guard'; import { rule as processArgv } from './process-argv'; +import { rule as productionDebug } from './production-debug'; import { rule as pseudoRandom } from './pseudo-random'; import { rule as regularExpr } from './regular-expr'; import { rule as shorthandPropertyGrouping } from './shorthand-property-grouping'; @@ -198,6 +199,7 @@ ruleModules['prefer-default-last'] = preferDefaultLast; ruleModules['prefer-promise-shorthand'] = preferPromiseShorthand; ruleModules['prefer-type-guard'] = preferTypeGuard; ruleModules['process-argv'] = processArgv; +ruleModules['production-debug'] = productionDebug; ruleModules['pseudo-random'] = pseudoRandom; ruleModules['regular-expr'] = regularExpr; ruleModules['shorthand-property-grouping'] = shorthandPropertyGrouping; diff --git a/eslint-bridge/src/rules/production-debug.ts b/eslint-bridge/src/rules/production-debug.ts new file mode 100644 index 0000000000..48fed1732a --- /dev/null +++ b/eslint-bridge/src/rules/production-debug.ts @@ -0,0 +1,107 @@ +/* + * SonarQube JavaScript Plugin + * Copyright (C) 2011-2020 SonarSource SA + * mailto:info AT sonarsource DOT com + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 3 of the License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public License + * along with this program; if not, write to the Free Software Foundation, + * Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. + */ +// https://jira.sonarsource.com/browse/RSPEC-4507 + +import { Rule } from 'eslint'; +import * as estree from 'estree'; +import { getModuleNameOfIdentifier, getUniqueWriteUsage, getVariableFromName } from './utils'; + +const ERRORHANDLER_MODULE = 'errorhandler'; +const message = + 'Make sure this debug feature is deactivated before delivering the code in production.'; + +export const rule: Rule.RuleModule = { + create(context: Rule.RuleContext) { + return { + DebuggerStatement(node: estree.Node) { + context.report({ + node, + message, + }); + }, + CallExpression(node: estree.Node) { + const callExpression = node as estree.CallExpression; + + // alert(...) + checkOpenDialogFunction(context, callExpression); + + // app.use(...) + checkErrorHandlerMiddleware(context, callExpression); + }, + }; + }, +}; + +function checkOpenDialogFunction(context: Rule.RuleContext, callExpression: estree.CallExpression) { + const { callee } = callExpression; + if (callee.type === 'Identifier') { + const { name } = callee; + + if (name === 'alert' || name === 'prompt' || name === 'confirm') { + const variable = getVariableFromName(context, name); + if (variable) { + // we don't report on custom function + return; + } + context.report({ + node: callExpression, + message, + }); + } + } +} + +function checkErrorHandlerMiddleware( + context: Rule.RuleContext, + callExpression: estree.CallExpression, +) { + const { callee, arguments: args } = callExpression; + if ( + callee.type === 'MemberExpression' && + callee.property.type === 'Identifier' && + callee.property.name === 'use' && + args.length > 0 && + !conditional(context) + ) { + let middleware: estree.Node | undefined = args[0]; + if (middleware.type === 'Identifier') { + middleware = getUniqueWriteUsage(context, middleware.name); + } + + if ( + middleware && + middleware.type === 'CallExpression' && + middleware.callee.type === 'Identifier' + ) { + const module = getModuleNameOfIdentifier(middleware.callee, context); + if (module?.value === ERRORHANDLER_MODULE) { + context.report({ + node: callExpression, + message, + }); + } + } + } +} + +function conditional(context: Rule.RuleContext) { + const ancestors = context.getAncestors(); + return !!ancestors.find(ancestor => ancestor.type === 'IfStatement'); +} diff --git a/eslint-bridge/tests/rules/production-debug.test.ts b/eslint-bridge/tests/rules/production-debug.test.ts new file mode 100644 index 0000000000..d81d9022fd --- /dev/null +++ b/eslint-bridge/tests/rules/production-debug.test.ts @@ -0,0 +1,124 @@ +/* + * SonarQube JavaScript Plugin + * Copyright (C) 2011-2020 SonarSource SA + * mailto:info AT sonarsource DOT com + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 3 of the License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public License + * along with this program; if not, write to the Free Software Foundation, + * Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. + */ +import { RuleTester } from 'eslint'; +import { rule } from 'rules/production-debug'; + +const ruleTester = new RuleTester({ parserOptions: { ecmaVersion: 2018, sourceType: 'module' } }); + +const message = + 'Make sure this debug feature is deactivated before delivering the code in production.'; + +ruleTester.run( + 'Delivering code in production with debug features activated is security-sensitive', + rule, + { + valid: [ + { + code: ` + Debug.write("hello, world"); + + // we report only on trivial (and mostly used) usages without object access + this.alert("here!"); + window.alert("here!"); + + // custom is ok + function alert() {} + alert("here!"); + + import { confirm } from './confirm'; + confirm("Are you sure?"); + `, + }, + ], + invalid: [ + { + code: `debugger;`, + errors: [ + { + message, + line: 1, + endLine: 1, + column: 1, + endColumn: 10, + }, + ], + }, + { + code: ` + alert("here!"); + confirm("Are you sure?"); + prompt("What's your name?", "John Doe"); + `, + errors: [ + { + message, + line: 2, + }, + { + message, + line: 3, + }, + { + message, + line: 4, + }, + ], + }, + { + code: ` + const errorhandler = require('errorhandler'); + if (process.env.NODE_ENV === 'development') { + app1.use(errorhandler()); // Compliant + } + app2.use(errorhandler()); // Noncompliant + `, + errors: [ + { + message, + line: 6, + column: 9, + endColumn: 33, + }, + ], + }, + { + code: ` + import * as errorhandler from 'errorhandler'; + const handler = errorhandler(); + app1.use(handler); // Noncompliant + if (process.env.NODE_ENV === 'development') { + app2.use(handler); // Compliant + } else { + app3.use(handler); // Compliant + } + app4.use(); + `, + errors: [ + { + message, + line: 4, + column: 9, + endColumn: 26, + }, + ], + }, + ], + }, +); diff --git a/its/ruling/src/test/expected/js/javascript-test-sources/javascript-S4507.json b/its/ruling/src/test/expected/js/javascript-test-sources/javascript-S4507.json new file mode 100644 index 0000000000..e97b7bbca4 --- /dev/null +++ b/its/ruling/src/test/expected/js/javascript-test-sources/javascript-S4507.json @@ -0,0 +1,5 @@ +{ +'javascript-test-sources:src/ecmascript6/sonar-web/src/main/js/components/issue/issue-view.js':[ +133, +], +} diff --git a/its/ruling/src/test/expected/js/jshint/javascript-S4507.json b/its/ruling/src/test/expected/js/jshint/javascript-S4507.json new file mode 100644 index 0000000000..8984f62c35 --- /dev/null +++ b/its/ruling/src/test/expected/js/jshint/javascript-S4507.json @@ -0,0 +1,12 @@ +{ +'jshint:tests/unit/fixtures/gh247.js':[ +9, +10, +11, +17, +22, +34, +35, +36, +], +} diff --git a/its/ruling/src/test/expected/js/p5.js/javascript-S4507.json b/its/ruling/src/test/expected/js/p5.js/javascript-S4507.json new file mode 100644 index 0000000000..2731f01063 --- /dev/null +++ b/its/ruling/src/test/expected/js/p5.js/javascript-S4507.json @@ -0,0 +1,13 @@ +{ +'p5.js:lib/addons/p5.sound.js':[ +10042, +10176, +10193, +], +'p5.js:src/io/files.js':[ +1838, +], +'p5.js:test/manual-test-examples/dom/audioelt_onended/sketch.js':[ +8, +], +} diff --git a/its/ruling/src/test/expected/js/prototype/javascript-S4507.json b/its/ruling/src/test/expected/js/prototype/javascript-S4507.json new file mode 100644 index 0000000000..13b2033728 --- /dev/null +++ b/its/ruling/src/test/expected/js/prototype/javascript-S4507.json @@ -0,0 +1,5 @@ +{ +'prototype:test/unit/tests/element_mixins.test.js':[ +27, +], +} diff --git a/its/ruling/src/test/expected/ts/TypeScript/typescript-S4507.json b/its/ruling/src/test/expected/ts/TypeScript/typescript-S4507.json new file mode 100644 index 0000000000..a218e146b0 --- /dev/null +++ b/its/ruling/src/test/expected/ts/TypeScript/typescript-S4507.json @@ -0,0 +1,5 @@ +{ +'TypeScript:src/compiler/core.ts':[ +2254, +], +} diff --git a/its/ruling/src/test/expected/ts/console/typescript-S4507.json b/its/ruling/src/test/expected/ts/console/typescript-S4507.json new file mode 100644 index 0000000000..7be3e2b089 --- /dev/null +++ b/its/ruling/src/test/expected/ts/console/typescript-S4507.json @@ -0,0 +1,12 @@ +{ +'console:src/main.tsx':[ +55, +], +'console:src/views/Integrations/AlgoliaPopup/AlgoliaIndexPopup/AlgoliaIndexPopup.tsx':[ +143, +148, +], +'console:src/views/account/ResetPasswordView/ResetPasswordView.tsx':[ +45, +], +} diff --git a/javascript-checks/src/main/java/org/sonar/javascript/checks/CheckList.java b/javascript-checks/src/main/java/org/sonar/javascript/checks/CheckList.java index 01bc574e1b..633c9e0240 100644 --- a/javascript-checks/src/main/java/org/sonar/javascript/checks/CheckList.java +++ b/javascript-checks/src/main/java/org/sonar/javascript/checks/CheckList.java @@ -238,6 +238,7 @@ public static List getAllChecks() { PrimitiveWrappersCheck.class, ProcessArgvCheck.class, PreferDefaultLastCheck.class, + ProductionDebugCheck.class, PseudoRandomCheck.class, ReassignedParameterCheck.class, RedeclaredSymbolCheck.class, diff --git a/javascript-checks/src/main/java/org/sonar/javascript/checks/ProductionDebugCheck.java b/javascript-checks/src/main/java/org/sonar/javascript/checks/ProductionDebugCheck.java new file mode 100644 index 0000000000..f2154644c0 --- /dev/null +++ b/javascript-checks/src/main/java/org/sonar/javascript/checks/ProductionDebugCheck.java @@ -0,0 +1,36 @@ +/* + * SonarQube JavaScript Plugin + * Copyright (C) 2011-2020 SonarSource SA + * mailto:info AT sonarsource DOT com + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 3 of the License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public License + * along with this program; if not, write to the Free Software Foundation, + * Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. + */ +package org.sonar.javascript.checks; + +import org.sonar.check.Rule; +import org.sonar.javascript.checks.annotations.JavaScriptRule; +import org.sonar.javascript.checks.annotations.TypeScriptRule; + +@JavaScriptRule +@TypeScriptRule +@Rule(key = "S4507") +public class ProductionDebugCheck extends EslintBasedCheck { + + @Override + public String eslintKey() { + return "production-debug"; + } +} + diff --git a/javascript-checks/src/main/resources/org/sonar/l10n/javascript/rules/javascript/S1442.html b/javascript-checks/src/main/resources/org/sonar/l10n/javascript/rules/javascript/S1442.html index b6308d6fd1..d1479e43a3 100644 --- a/javascript-checks/src/main/resources/org/sonar/l10n/javascript/rules/javascript/S1442.html +++ b/javascript-checks/src/main/resources/org/sonar/l10n/javascript/rules/javascript/S1442.html @@ -12,4 +12,5 @@

See

  • MITRE, CWE-489 - Leftover Debug Code
    • - +

    Deprecated

    +

    This rule is deprecated; use {rule:javascript:S4507} instead.

    diff --git a/javascript-checks/src/main/resources/org/sonar/l10n/javascript/rules/javascript/S1442.json b/javascript-checks/src/main/resources/org/sonar/l10n/javascript/rules/javascript/S1442.json index 3e63710526..4f70dc83a3 100644 --- a/javascript-checks/src/main/resources/org/sonar/l10n/javascript/rules/javascript/S1442.json +++ b/javascript-checks/src/main/resources/org/sonar/l10n/javascript/rules/javascript/S1442.json @@ -1,7 +1,7 @@ { "title": "\"alert(...)\" should not be used", "type": "VULNERABILITY", - "status": "ready", + "status": "deprecated", "remediation": { "func": "Constant\/Issue", "constantCost": "10min" diff --git a/javascript-checks/src/main/resources/org/sonar/l10n/javascript/rules/javascript/S1525.html b/javascript-checks/src/main/resources/org/sonar/l10n/javascript/rules/javascript/S1525.html index ae608922e2..ea47215c78 100644 --- a/javascript-checks/src/main/resources/org/sonar/l10n/javascript/rules/javascript/S1525.html +++ b/javascript-checks/src/main/resources/org/sonar/l10n/javascript/rules/javascript/S1525.html @@ -23,4 +23,5 @@

    See

  • MITRE, CWE-489 - Leftover Debug Code
    • - +

    Deprecated

    +

    This rule is deprecated; use {rule:javascript:S4507} instead.

    diff --git a/javascript-checks/src/main/resources/org/sonar/l10n/javascript/rules/javascript/S1525.json b/javascript-checks/src/main/resources/org/sonar/l10n/javascript/rules/javascript/S1525.json index eb387aef84..cece5c464e 100644 --- a/javascript-checks/src/main/resources/org/sonar/l10n/javascript/rules/javascript/S1525.json +++ b/javascript-checks/src/main/resources/org/sonar/l10n/javascript/rules/javascript/S1525.json @@ -1,7 +1,7 @@ { "title": "Debugger statements should not be used", "type": "VULNERABILITY", - "status": "ready", + "status": "deprecated", "remediation": { "func": "Constant\/Issue", "constantCost": "5min" diff --git a/javascript-checks/src/main/resources/org/sonar/l10n/javascript/rules/javascript/S4507.html b/javascript-checks/src/main/resources/org/sonar/l10n/javascript/rules/javascript/S4507.html new file mode 100644 index 0000000000..3342406a80 --- /dev/null +++ b/javascript-checks/src/main/resources/org/sonar/l10n/javascript/rules/javascript/S4507.html @@ -0,0 +1,61 @@ +

    Delivering code in production with debug features activated is security-sensitive. It has led in the past to the following vulnerabilities:

    + +

    An application's debug features enable developers to find bugs more easily and thus facilitate also the work of attackers. It often gives access to +detailed information on both the system running the application and users.

    +

    Ask Yourself Whether

    + +

    There is a risk if you answered yes to any of those questions.

    +

    Recommended Secure Coding Practices

    +

    Do not enable debug features on production servers.

    +

    Sensitive Code Example

    +

    The debugger statement should be removed in +production:

    +
    +for (i = 1; i<5; i++) {
+  // Print i to the Output window.
+  Debug.write("loop index is " + i);
+  // Wait for user to resume.
+  debugger; // Sensitive
+}
+
    +

    alert(), confirm() and prompt() instructions should be removed in production:

    +
    +if(unexpectedCondition) {
+  alert("Unexpected Condition");  // Sensitive
+}
+
    +

    errorhandler Express.js middleware should not be used in production:

    +
    +const express = require('express');
+const errorhandler = require('errorhandler');
+
+let app = express();
+app.use(errorhandler()); // Sensitive
+
    +

    Compliant Solution

    +

    errorhandler Express.js middleware used only in development mode:

    +
    +const express = require('express');
+const errorhandler = require('errorhandler');
+
+let app = express();
+
+if (process.env.NODE_ENV === 'development') {  // Compliant
+  app.use(errorhandler());  // Compliant
+}
+
    +

    See

    + + diff --git a/javascript-checks/src/main/resources/org/sonar/l10n/javascript/rules/javascript/S4507.json b/javascript-checks/src/main/resources/org/sonar/l10n/javascript/rules/javascript/S4507.json new file mode 100644 index 0000000000..b892014dc8 --- /dev/null +++ b/javascript-checks/src/main/resources/org/sonar/l10n/javascript/rules/javascript/S4507.json @@ -0,0 +1,33 @@ +{ + "title": "Delivering code in production with debug features activated is security-sensitive", + "type": "SECURITY_HOTSPOT", + "status": "ready", + "remediation": { + "func": "Constant\/Issue", + "constantCost": "1min" + }, + "tags": [ + "cwe", + "error-handling", + "user-experience", + "express.js", + "owasp-a3" + ], + "defaultSeverity": "Minor", + "ruleSpecification": "RSPEC-4507", + "sqKey": "S4507", + "compatibleLanguages": [ + "JAVASCRIPT", + "TYPESCRIPT" + ], + "scope": "Main", + "securityStandards": { + "CWE": [ + 489, + 215 + ], + "OWASP": [ + "A3" + ] + } +} diff --git a/javascript-checks/src/main/resources/org/sonar/l10n/javascript/rules/javascript/Sonar_way_profile.json b/javascript-checks/src/main/resources/org/sonar/l10n/javascript/rules/javascript/Sonar_way_profile.json index 74f6343de5..c0bda8931c 100644 --- a/javascript-checks/src/main/resources/org/sonar/l10n/javascript/rules/javascript/Sonar_way_profile.json +++ b/javascript-checks/src/main/resources/org/sonar/l10n/javascript/rules/javascript/Sonar_way_profile.json @@ -98,6 +98,7 @@ "S4275", "S4325", "S4335", + "S4507", "S4524", "S4619", "S4621", diff --git a/javascript-checks/src/main/resources/org/sonar/l10n/javascript/rules/javascript/Sonar_way_recommended_profile.json b/javascript-checks/src/main/resources/org/sonar/l10n/javascript/rules/javascript/Sonar_way_recommended_profile.json index cd4d110eb5..42f52ec02a 100644 --- a/javascript-checks/src/main/resources/org/sonar/l10n/javascript/rules/javascript/Sonar_way_recommended_profile.json +++ b/javascript-checks/src/main/resources/org/sonar/l10n/javascript/rules/javascript/Sonar_way_recommended_profile.json @@ -165,6 +165,7 @@ "S4327", "S4328", "S4335", + "S4507", "S4524", "S4619", "S4621", From ca1ae493fcc34639ff616dda54c52898e3a66157 Mon Sep 17 00:00:00 2001 From: vilchik-elena Date: Thu, 13 Aug 2020 09:33:08 +0200 Subject: [PATCH 2/2] Fixes after review --- eslint-bridge/src/rules/production-debug.ts | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/eslint-bridge/src/rules/production-debug.ts b/eslint-bridge/src/rules/production-debug.ts index 48fed1732a..3557e3f655 100644 --- a/eslint-bridge/src/rules/production-debug.ts +++ b/eslint-bridge/src/rules/production-debug.ts @@ -21,7 +21,12 @@ import { Rule } from 'eslint'; import * as estree from 'estree'; -import { getModuleNameOfIdentifier, getUniqueWriteUsage, getVariableFromName } from './utils'; +import { + getModuleNameOfIdentifier, + getUniqueWriteUsage, + getVariableFromName, + isIdentifier, +} from './utils'; const ERRORHANDLER_MODULE = 'errorhandler'; const message = @@ -75,10 +80,9 @@ function checkErrorHandlerMiddleware( const { callee, arguments: args } = callExpression; if ( callee.type === 'MemberExpression' && - callee.property.type === 'Identifier' && - callee.property.name === 'use' && + isIdentifier(callee.property, 'use') && args.length > 0 && - !conditional(context) + !isInsideConditional(context) ) { let middleware: estree.Node | undefined = args[0]; if (middleware.type === 'Identifier') { @@ -101,7 +105,7 @@ function checkErrorHandlerMiddleware( } } -function conditional(context: Rule.RuleContext) { +function isInsideConditional(context: Rule.RuleContext) { const ancestors = context.getAncestors(); - return !!ancestors.find(ancestor => ancestor.type === 'IfStatement'); + return ancestors.some(ancestor => ancestor.type === 'IfStatement'); }