fix various nits based on feedback

Author: erik-krogh

python/ql/lib/semmle/python/security/dataflow/UnsafeShellCommandConstructionCustomizations.qll

@@ -68,11 +68,11 @@ module UnsafeShellCommandConstruction {
     Fstring fstring;
 
     StringInterpolationAsSink() {
-      isUsedAsShellCommand(any(DataFlow::Node n | n.asExpr() = fstring), s) and
+      isUsedAsShellCommand(DataFlow::exprNode(fstring), s) and
       this.asExpr() = fstring.getASubExpression()
     }
 
-    override string describe() { result = "string construction" }
+    override string describe() { result = "f-string" }
 
     override DataFlow::Node getCommandExecution() { result = s }
 
@@ -101,7 +101,7 @@ module UnsafeShellCommandConstruction {
   }
 
   /**
-   * A string constructed using a `.join(" ")` call, where the resulting string ends up being executed as a shell command.
+   * A string constructed using a `" ".join(...)` call, where the resulting string ends up being executed as a shell command.
    */
   class ArrayJoin extends Sink {
     Concepts::SystemCommandExecution s;

python/ql/lib/semmle/python/security/dataflow/UnsafeShellCommandConstructionQuery.qll

@@ -24,8 +24,7 @@ class Configuration extends TaintTracking::Configuration {
   override predicate isSink(DataFlow::Node sink) { sink instanceof Sink }
 
   override predicate isSanitizer(DataFlow::Node node) {
-    node instanceof CommandInjection::Sanitizer or // using all sanitizers from `rb/command-injection`
-    node instanceof StringConstCompareBarrier
+    node instanceof CommandInjection::Sanitizer // using all sanitizers from `rb/command-injection`
   }
 
   // override to require the path doesn't have unmatched return steps

python/ql/src/Security/CWE-078/UnsafeShellCommandConstruction.qhelp

@@ -4,7 +4,7 @@
 <qhelp>
 <overview>
 	<p>
-		Dynamically constructing a shell command with inputs from exported 
+		Dynamically constructing a shell command with inputs from library 
 		functions may inadvertently change the meaning of the shell command.
 
 		Clients using the exported function may use inputs containing
@@ -21,7 +21,7 @@
 
 	<p>
 		If possible, provide the dynamic arguments to the shell as an array 
-		to APIs such as <code>system(..)</code> to avoid interpretation by the shell.
+		to APIs such as <code>subprocess.run</code> to avoid interpretation by the shell.
 	</p>
 
 	<p>
@@ -55,7 +55,7 @@
 
 	<p>
 		To avoid such potentially catastrophic behaviors, provide the
-		input from exported functions as an argument that does not
+		input from library functions as an argument that does not
 		get interpreted by a shell:
 	</p>
 

python/ql/src/Security/CWE-078/examples/unsafe-shell-command-construction.py

@@ -1,4 +1,4 @@
 import os
 
-def download (path): 
+def download(path): 
     os.system("wget " + path) # NOT OK

python/ql/src/Security/CWE-078/examples/unsafe-shell-command-construction_fixed.py

@@ -1,4 +1,4 @@
 import subprocess
 
-def download (path): 
+def download(path): 
     subprocess.run(["wget", path]) # OK

python/ql/test/query-tests/Security/CWE-078-UnsafeShellCommandConstruction/UnsafeShellCommandConstruction.expected

@@ -30,7 +30,7 @@ nodes
 subpaths
 #select
 | src/unsafe_shell_test.py:5:15:5:28 | ControlFlowNode for BinaryExpr | src/unsafe_shell_test.py:4:22:4:25 | ControlFlowNode for name | src/unsafe_shell_test.py:5:25:5:28 | ControlFlowNode for name | This string concatenation which depends on $@ is later used in a $@. | src/unsafe_shell_test.py:4:22:4:25 | ControlFlowNode for name | library input | src/unsafe_shell_test.py:5:5:5:29 | ControlFlowNode for Attribute() | shell command |
-| src/unsafe_shell_test.py:8:15:8:28 | ControlFlowNode for Fstring | src/unsafe_shell_test.py:4:22:4:25 | ControlFlowNode for name | src/unsafe_shell_test.py:8:23:8:26 | ControlFlowNode for name | This string construction which depends on $@ is later used in a $@. | src/unsafe_shell_test.py:4:22:4:25 | ControlFlowNode for name | library input | src/unsafe_shell_test.py:8:5:8:29 | ControlFlowNode for Attribute() | shell command |
+| src/unsafe_shell_test.py:8:15:8:28 | ControlFlowNode for Fstring | src/unsafe_shell_test.py:4:22:4:25 | ControlFlowNode for name | src/unsafe_shell_test.py:8:23:8:26 | ControlFlowNode for name | This f-string which depends on $@ is later used in a $@. | src/unsafe_shell_test.py:4:22:4:25 | ControlFlowNode for name | library input | src/unsafe_shell_test.py:8:5:8:29 | ControlFlowNode for Attribute() | shell command |
 | src/unsafe_shell_test.py:11:15:11:38 | ControlFlowNode for BinaryExpr | src/unsafe_shell_test.py:4:22:4:25 | ControlFlowNode for name | src/unsafe_shell_test.py:11:25:11:38 | ControlFlowNode for Attribute() | This string concatenation which depends on $@ is later used in a $@. | src/unsafe_shell_test.py:4:22:4:25 | ControlFlowNode for name | library input | src/unsafe_shell_test.py:11:5:11:39 | ControlFlowNode for Attribute() | shell command |
 | src/unsafe_shell_test.py:14:15:14:40 | ControlFlowNode for BinaryExpr | src/unsafe_shell_test.py:4:22:4:25 | ControlFlowNode for name | src/unsafe_shell_test.py:14:25:14:40 | ControlFlowNode for Attribute() | This string concatenation which depends on $@ is later used in a $@. | src/unsafe_shell_test.py:4:22:4:25 | ControlFlowNode for name | library input | src/unsafe_shell_test.py:14:5:14:41 | ControlFlowNode for Attribute() | shell command |
 | src/unsafe_shell_test.py:17:15:17:36 | ControlFlowNode for Attribute() | src/unsafe_shell_test.py:4:22:4:25 | ControlFlowNode for name | src/unsafe_shell_test.py:17:32:17:35 | ControlFlowNode for name | This formatted string which depends on $@ is later used in a $@. | src/unsafe_shell_test.py:4:22:4:25 | ControlFlowNode for name | library input | src/unsafe_shell_test.py:17:5:17:37 | ControlFlowNode for Attribute() | shell command |