From b6f9cd3e9e23d2b3bf584111b492ef18fa373265 Mon Sep 17 00:00:00 2001 From: Justin Mclean Date: Mon, 12 Oct 2015 00:07:03 +1100 Subject: [PATCH 1/7] add boolean expression checker rule --- src/main/resources/cflint.definition.xml | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/main/resources/cflint.definition.xml b/src/main/resources/cflint.definition.xml index befa1ab8a..856997a92 100644 --- a/src/main/resources/cflint.definition.xml +++ b/src/main/resources/cflint.definition.xml @@ -179,4 +179,9 @@ INFO + + + INFO + + From d1670ea6f898d1515bcf048bfc5d6f4fe5d4ce1e Mon Sep 17 00:00:00 2001 From: Justin Mclean Date: Mon, 12 Oct 2015 00:08:11 +1100 Subject: [PATCH 2/7] Add rule to check boolean expression checker for explicit comparison against true and false --- .../core/BooleanExpressionChecker.java | 68 +++++++++++++++++++ 1 file changed, 68 insertions(+) create mode 100644 src/main/java/com/cflint/plugins/core/BooleanExpressionChecker.java diff --git a/src/main/java/com/cflint/plugins/core/BooleanExpressionChecker.java b/src/main/java/com/cflint/plugins/core/BooleanExpressionChecker.java new file mode 100644 index 000000000..dffc1d677 --- /dev/null +++ b/src/main/java/com/cflint/plugins/core/BooleanExpressionChecker.java @@ -0,0 +1,68 @@ +package com.cflint.plugins.core; + +import net.htmlparser.jericho.Element; +import cfml.parsing.cfscript.CFExpression; +import cfml.parsing.cfscript.CFBinaryExpression; +import cfml.parsing.cfscript.script.CFScriptStatement; +import cfml.parsing.cfscript.script.CFIfStatement; +import cfml.parsing.cfscript.script.CFExpressionStatement; + +import com.cflint.BugInfo; +import com.cflint.BugList; +import com.cflint.plugins.CFLintScannerAdapter; +import com.cflint.plugins.Context; + +public class BooleanExpressionChecker extends CFLintScannerAdapter { + final String severity = "INFO"; + + protected int lastLineNo = -1; + + @Override + public void expression(final CFExpression expression, final Context context, final BugList bugs) { + if (expression instanceof CFBinaryExpression) { + String code = expression.Decompile(0).toLowerCase(); + + if (hasExplicitBooleanCheck(code)) { + int lineNo = ((CFBinaryExpression) expression).getLine() + context.startLine() - 1; + + // Only report issue once per line + if (lastLineNo != lineNo) { + booleanExpression(lineNo, context, bugs); + lastLineNo = lineNo; + } + } + } + } + + @Override + public void element(final Element element, final Context context, final BugList bugs) { + String tag = element.getName(); + + if (tag.equals("cfif")) { + String content = element.getStartTag().getTagContent().toString(); + + if (hasExplicitBooleanCheck(content)) { + int lineNo = element.getSource().getRow(element.getBegin()); + + // Only report issue once per line + if (lastLineNo != lineNo) { + booleanExpression(lineNo, context, bugs); + lastLineNo = lineNo; + } + } + } + } + + protected boolean hasExplicitBooleanCheck(final String code) { + return code.contains("== true") || code.contains("eq true") || code.contains("is true") || code.contains("!= true") + || code.contains("== false") || code.contains("eq false") || code.contains("is false") || code.contains("!= false"); + } + + public void booleanExpression(final int lineNo, final Context context, final BugList bugs) { + bugs.add(new BugInfo.BugInfoBuilder().setLine(lineNo).setMessageCode("EXPLICIT_BOOLEAN_CHECK") + .setSeverity(severity).setFilename(context.getFilename()) + .setMessage("Explict check of boolean expession at " + lineNo + " is not needed.") + .build()); + } + +} From db1b92ef34644ae4f73400e64e71d25188cf25bf Mon Sep 17 00:00:00 2001 From: Justin Mclean Date: Mon, 12 Oct 2015 00:08:45 +1100 Subject: [PATCH 3/7] Tests for boolean expression checker --- .../cflint/TestBooleanExpressionChecker.java | 65 +++++++++++++++++++ 1 file changed, 65 insertions(+) create mode 100644 src/test/java/com/cflint/TestBooleanExpressionChecker.java diff --git a/src/test/java/com/cflint/TestBooleanExpressionChecker.java b/src/test/java/com/cflint/TestBooleanExpressionChecker.java new file mode 100644 index 000000000..1f5def938 --- /dev/null +++ b/src/test/java/com/cflint/TestBooleanExpressionChecker.java @@ -0,0 +1,65 @@ +package com.cflint; + +import static org.junit.Assert.assertEquals; + +import java.io.IOException; +import java.util.List; + +import org.junit.Before; +import org.junit.Test; + +import cfml.parsing.reporting.ParseException; + +import com.cflint.config.CFLintPluginInfo.PluginInfoRule; +import com.cflint.config.CFLintPluginInfo.PluginInfoRule.PluginMessage; +import com.cflint.config.ConfigRuntime; +import com.cflint.plugins.core.BooleanExpressionChecker; + +public class TestBooleanExpressionChecker { + + private CFLint cfBugs; + + @Before + public void setUp() { + final ConfigRuntime conf = new ConfigRuntime(); + final PluginInfoRule pluginRule = new PluginInfoRule(); + pluginRule.setName("BooleanExpressionChecker"); + conf.getRules().add(pluginRule); + final PluginMessage pluginMessage = new PluginMessage("EXPLICIT_BOOLEAN_CHECK"); + pluginMessage.setSeverity("INFO"); + cfBugs = new CFLint(conf, new BooleanExpressionChecker()); + } + + @Test + public void testBooleanExpressionInScript() throws ParseException, IOException { + final String scriptSrc = "\r\n" + + "if (a && b == true) {\r\n" + + " c = 1;\r\n" + + "}\r\n" + + "else if (a or b is false) {\r\n" + + " c = 1;\r\n" + + "}\r\n" + + ""; + + cfBugs.process(scriptSrc, "test"); + final List result = cfBugs.getBugs().getBugList().values().iterator().next(); + assertEquals(2, result.size()); + assertEquals("EXPLICIT_BOOLEAN_CHECK", result.get(0).getMessageCode()); + assertEquals(2, result.get(0).getLine()); + assertEquals("EXPLICIT_BOOLEAN_CHECK", result.get(1).getMessageCode()); + assertEquals(5, result.get(1).getLine()); + } + + @Test + public void testBooleanExpressionInTag() throws ParseException, IOException { + final String tagSrc = "\r\n" + + ""; + + cfBugs.process(tagSrc, "test"); + final List result = cfBugs.getBugs().getBugList().values().iterator().next(); + assertEquals(1, result.size()); + assertEquals("EXPLICIT_BOOLEAN_CHECK", result.get(0).getMessageCode()); + assertEquals(2, result.get(0).getLine()); + } + +} From e62530f4afdefd94aa30469312285ae2763fbced Mon Sep 17 00:00:00 2001 From: Justin Mclean Date: Mon, 12 Oct 2015 00:21:07 +1100 Subject: [PATCH 4/7] Added rule to check for complex boolean expressions --- .../core/ComplexBooleanExpressionChecker.java | 82 +++++++++++++++++++ 1 file changed, 82 insertions(+) create mode 100644 src/main/java/com/cflint/plugins/core/ComplexBooleanExpressionChecker.java diff --git a/src/main/java/com/cflint/plugins/core/ComplexBooleanExpressionChecker.java b/src/main/java/com/cflint/plugins/core/ComplexBooleanExpressionChecker.java new file mode 100644 index 000000000..92b932c09 --- /dev/null +++ b/src/main/java/com/cflint/plugins/core/ComplexBooleanExpressionChecker.java @@ -0,0 +1,82 @@ +package com.cflint.plugins.core; + +import net.htmlparser.jericho.Element; +import cfml.parsing.cfscript.CFExpression; +import cfml.parsing.cfscript.CFBinaryExpression; +import cfml.parsing.cfscript.script.CFScriptStatement; +import cfml.parsing.cfscript.script.CFIfStatement; +import cfml.parsing.cfscript.script.CFExpressionStatement; + +import com.cflint.BugInfo; +import com.cflint.BugList; +import com.cflint.plugins.CFLintScannerAdapter; +import com.cflint.plugins.Context; + +public class ComplexBooleanExpressionChecker extends CFLintScannerAdapter { + final String severity = "WARNING"; + + protected int complexThreshold = 10; + + @Override + public void expression(final CFExpression expression, final Context context, final BugList bugs) { + if (expression instanceof CFBinaryExpression) { + String code = expression.Decompile(0).toLowerCase(); + + if (isComplex(code, complexThreshold)) { + int lineNo = ((CFBinaryExpression) expression).getLine() + context.startLine() - 1; + + complexBooleanExpression(lineNo, context, bugs); + } + } + } + + @Override + public void element(final Element element, final Context context, final BugList bugs) { + String tag = element.getName(); + + if (tag.equals("cfif")) { + String content = element.getStartTag().getTagContent().toString(); + + if (isComplex(content, complexThreshold)) { + int lineNo = element.getSource().getRow(element.getBegin()); + + complexBooleanExpression(lineNo, context, bugs); + } + } + } + + protected boolean isComplex(final String code, final int complexThreshold) { + int noAnds = noSubstrings(code, " && ") + noSubstrings(code, " and "); + int noOrs = noSubstrings(code, " || ") + noSubstrings(code, " or "); + int noNots = noSubstrings(code, " ! ") + noSubstrings(code, " not "); + + // This is just a rough heuristic + // lots of and's or or's on their own is usually easy to understand + // but combine them together and it gets hard to understand very quickly + return (noAnds * noOrs + noNots + noAnds + noOrs) > complexThreshold; + } + + protected int noSubstrings(String string, String substring) { + int lastIndex = 0; + int count = 0; + + while (lastIndex != -1) { + lastIndex = string.indexOf(substring, lastIndex); + + if (lastIndex != -1) { + count ++; + lastIndex += substring.length(); + } + } + + return count; + } + + public void complexBooleanExpression(final int lineNo, final Context context, final BugList bugs) { + bugs.add(new BugInfo.BugInfoBuilder().setLine(lineNo).setMessageCode("COMPLEX_BOOLEAN_CHECK") + .setSeverity(severity).setFilename(context.getFilename()) + .setMessage("Boolean expession at " + lineNo + " is too complex. Consider simplifying or moving to a named method.") + .build()); + } + +} From 5cc1d80adbdd8b481331e4b4dd21a23592a128ea Mon Sep 17 00:00:00 2001 From: Justin Mclean Date: Mon, 12 Oct 2015 00:21:56 +1100 Subject: [PATCH 5/7] tests for complex boolean expression rule --- .../TestComplexBooleanExpressionChecker.java | 65 +++++++++++++++++++ 1 file changed, 65 insertions(+) create mode 100644 src/test/java/com/cflint/TestComplexBooleanExpressionChecker.java diff --git a/src/test/java/com/cflint/TestComplexBooleanExpressionChecker.java b/src/test/java/com/cflint/TestComplexBooleanExpressionChecker.java new file mode 100644 index 000000000..3c2b49fe0 --- /dev/null +++ b/src/test/java/com/cflint/TestComplexBooleanExpressionChecker.java @@ -0,0 +1,65 @@ +package com.cflint; + +import static org.junit.Assert.assertEquals; + +import java.io.IOException; +import java.util.List; + +import org.junit.Before; +import org.junit.Test; + +import cfml.parsing.reporting.ParseException; + +import com.cflint.config.CFLintPluginInfo.PluginInfoRule; +import com.cflint.config.CFLintPluginInfo.PluginInfoRule.PluginMessage; +import com.cflint.config.ConfigRuntime; +import com.cflint.plugins.core.ComplexBooleanExpressionChecker; + +public class TestComplexBooleanExpressionChecker { + + private CFLint cfBugs; + + @Before + public void setUp() { + final ConfigRuntime conf = new ConfigRuntime(); + final PluginInfoRule pluginRule = new PluginInfoRule(); + pluginRule.setName("ComplexBooleanExpressionChecker"); + conf.getRules().add(pluginRule); + final PluginMessage pluginMessage = new PluginMessage("COMPLEX_BOOLEAN_CHECK"); + pluginMessage.setSeverity("WARNING"); + cfBugs = new CFLint(conf, new ComplexBooleanExpressionChecker()); + } + + @Test + public void testBooleanExpressionInScript() throws ParseException, IOException { + final String scriptSrc = "\r\n" + + "if (a && b || c && d || e && f) {\r\n" + + " c = 1;\r\n" + + "}\r\n" + + "else if (a or not b and not a or b and not c) {\r\n" + + " c = 1;\r\n" + + "}\r\n" + + ""; + + cfBugs.process(scriptSrc, "test"); + final List result = cfBugs.getBugs().getBugList().values().iterator().next(); + assertEquals(2, result.size()); + assertEquals("COMPLEX_BOOLEAN_CHECK", result.get(0).getMessageCode()); + assertEquals(2, result.get(0).getLine()); + assertEquals("COMPLEX_BOOLEAN_CHECK", result.get(1).getMessageCode()); + assertEquals(5, result.get(1).getLine()); + } + + @Test + public void testBooleanExpressionInTag() throws ParseException, IOException { + final String tagSrc = "\r\n" + + ""; + + cfBugs.process(tagSrc, "test"); + final List result = cfBugs.getBugs().getBugList().values().iterator().next(); + assertEquals(1, result.size()); + assertEquals("COMPLEX_BOOLEAN_CHECK", result.get(0).getMessageCode()); + assertEquals(2, result.get(0).getLine()); + } + +} From 00b6ed49fb7fc40031cc2bfdb77c2dd23d770e93 Mon Sep 17 00:00:00 2001 From: Justin Mclean Date: Mon, 12 Oct 2015 00:22:37 +1100 Subject: [PATCH 6/7] Added rule for complex boolean expressions --- src/main/resources/cflint.definition.xml | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/main/resources/cflint.definition.xml b/src/main/resources/cflint.definition.xml index befa1ab8a..a627c5049 100644 --- a/src/main/resources/cflint.definition.xml +++ b/src/main/resources/cflint.definition.xml @@ -179,4 +179,9 @@ INFO + + + WARNING + + From 1bd2c162821018fbad6fd31a6c5bee4687049160 Mon Sep 17 00:00:00 2001 From: ryaneberly Date: Sun, 11 Oct 2015 15:12:30 -0400 Subject: [PATCH 7/7] Update BooleanExpressionChecker.java fix spelling --- .../java/com/cflint/plugins/core/BooleanExpressionChecker.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/com/cflint/plugins/core/BooleanExpressionChecker.java b/src/main/java/com/cflint/plugins/core/BooleanExpressionChecker.java index dffc1d677..96b0de35f 100644 --- a/src/main/java/com/cflint/plugins/core/BooleanExpressionChecker.java +++ b/src/main/java/com/cflint/plugins/core/BooleanExpressionChecker.java @@ -61,7 +61,7 @@ protected boolean hasExplicitBooleanCheck(final String code) { public void booleanExpression(final int lineNo, final Context context, final BugList bugs) { bugs.add(new BugInfo.BugInfoBuilder().setLine(lineNo).setMessageCode("EXPLICIT_BOOLEAN_CHECK") .setSeverity(severity).setFilename(context.getFilename()) - .setMessage("Explict check of boolean expession at " + lineNo + " is not needed.") + .setMessage("Explicit check of boolean expession at " + lineNo + " is not needed.") .build()); }