From b16b3c0394e27359053a1f9d0693006160c393de Mon Sep 17 00:00:00 2001
From: erik-krogh <erik-krogh@github.com>
Date: Tue, 4 Oct 2022 15:01:10 +0200
Subject: [PATCH 01/11] move cwe-078 tests into subfolders

---
 .../cwe-078/{ => CommandInjection}/CommandInjection.expected      | 0
 .../cwe-078/{ => CommandInjection}/CommandInjection.qlref         | 0
 .../security/cwe-078/{ => CommandInjection}/CommandInjection.rb   | 0
 .../security/cwe-078/{ => KernelOpen}/KernelOpen.expected         | 0
 .../security/cwe-078/{ => KernelOpen}/KernelOpen.qlref            | 0
 .../query-tests/security/cwe-078/{ => KernelOpen}/KernelOpen.rb   | 0
 6 files changed, 0 insertions(+), 0 deletions(-)
 rename ruby/ql/test/query-tests/security/cwe-078/{ => CommandInjection}/CommandInjection.expected (100%)
 rename ruby/ql/test/query-tests/security/cwe-078/{ => CommandInjection}/CommandInjection.qlref (100%)
 rename ruby/ql/test/query-tests/security/cwe-078/{ => CommandInjection}/CommandInjection.rb (100%)
 rename ruby/ql/test/query-tests/security/cwe-078/{ => KernelOpen}/KernelOpen.expected (100%)
 rename ruby/ql/test/query-tests/security/cwe-078/{ => KernelOpen}/KernelOpen.qlref (100%)
 rename ruby/ql/test/query-tests/security/cwe-078/{ => KernelOpen}/KernelOpen.rb (100%)

diff --git a/ruby/ql/test/query-tests/security/cwe-078/CommandInjection.expected b/ruby/ql/test/query-tests/security/cwe-078/CommandInjection/CommandInjection.expected
similarity index 100%
rename from ruby/ql/test/query-tests/security/cwe-078/CommandInjection.expected
rename to ruby/ql/test/query-tests/security/cwe-078/CommandInjection/CommandInjection.expected
diff --git a/ruby/ql/test/query-tests/security/cwe-078/CommandInjection.qlref b/ruby/ql/test/query-tests/security/cwe-078/CommandInjection/CommandInjection.qlref
similarity index 100%
rename from ruby/ql/test/query-tests/security/cwe-078/CommandInjection.qlref
rename to ruby/ql/test/query-tests/security/cwe-078/CommandInjection/CommandInjection.qlref
diff --git a/ruby/ql/test/query-tests/security/cwe-078/CommandInjection.rb b/ruby/ql/test/query-tests/security/cwe-078/CommandInjection/CommandInjection.rb
similarity index 100%
rename from ruby/ql/test/query-tests/security/cwe-078/CommandInjection.rb
rename to ruby/ql/test/query-tests/security/cwe-078/CommandInjection/CommandInjection.rb
diff --git a/ruby/ql/test/query-tests/security/cwe-078/KernelOpen.expected b/ruby/ql/test/query-tests/security/cwe-078/KernelOpen/KernelOpen.expected
similarity index 100%
rename from ruby/ql/test/query-tests/security/cwe-078/KernelOpen.expected
rename to ruby/ql/test/query-tests/security/cwe-078/KernelOpen/KernelOpen.expected
diff --git a/ruby/ql/test/query-tests/security/cwe-078/KernelOpen.qlref b/ruby/ql/test/query-tests/security/cwe-078/KernelOpen/KernelOpen.qlref
similarity index 100%
rename from ruby/ql/test/query-tests/security/cwe-078/KernelOpen.qlref
rename to ruby/ql/test/query-tests/security/cwe-078/KernelOpen/KernelOpen.qlref
diff --git a/ruby/ql/test/query-tests/security/cwe-078/KernelOpen.rb b/ruby/ql/test/query-tests/security/cwe-078/KernelOpen/KernelOpen.rb
similarity index 100%
rename from ruby/ql/test/query-tests/security/cwe-078/KernelOpen.rb
rename to ruby/ql/test/query-tests/security/cwe-078/KernelOpen/KernelOpen.rb

From 99b90789e5a809b906aa7388b2c050a508980a9e Mon Sep 17 00:00:00 2001
From: erik-krogh <erik-krogh@github.com>
Date: Mon, 10 Oct 2022 14:04:30 +0200
Subject: [PATCH 02/11] add `.shellescape` as a sanitizer for
 `rb/command-injection`

---
 .../ruby/security/CommandInjectionCustomizations.qll   |  3 +++
 .../cwe-078/CommandInjection/CommandInjection.expected |  6 ++++++
 .../cwe-078/CommandInjection/CommandInjection.rb       | 10 ++++++++++
 3 files changed, 19 insertions(+)

diff --git a/ruby/ql/lib/codeql/ruby/security/CommandInjectionCustomizations.qll b/ruby/ql/lib/codeql/ruby/security/CommandInjectionCustomizations.qll
index e67ae044a00..20cc8d5b26b 100644
--- a/ruby/ql/lib/codeql/ruby/security/CommandInjectionCustomizations.qll
+++ b/ruby/ql/lib/codeql/ruby/security/CommandInjectionCustomizations.qll
@@ -49,6 +49,9 @@ module CommandInjection {
   class ShellwordsEscapeAsSanitizer extends Sanitizer {
     ShellwordsEscapeAsSanitizer() {
       this = API::getTopLevelMember("Shellwords").getAMethodCall(["escape", "shellescape"])
+      or
+      // The method is also added as `String#shellescape`.
+      this.(DataFlow::CallNode).getMethodName() = "shellescape"
     }
   }
 }
diff --git a/ruby/ql/test/query-tests/security/cwe-078/CommandInjection/CommandInjection.expected b/ruby/ql/test/query-tests/security/cwe-078/CommandInjection/CommandInjection.expected
index b2e4990daec..1d00f293dfd 100644
--- a/ruby/ql/test/query-tests/security/cwe-078/CommandInjection/CommandInjection.expected
+++ b/ruby/ql/test/query-tests/security/cwe-078/CommandInjection/CommandInjection.expected
@@ -15,6 +15,8 @@ edges
 | CommandInjection.rb:81:20:81:25 | **args :  | CommandInjection.rb:82:22:82:25 | args :  |
 | CommandInjection.rb:82:22:82:25 | args :  | CommandInjection.rb:82:22:82:37 | ...[...] :  |
 | CommandInjection.rb:82:22:82:37 | ...[...] :  | CommandInjection.rb:82:14:82:39 | "echo #{...}" |
+| CommandInjection.rb:94:16:94:21 | call to params :  | CommandInjection.rb:94:16:94:28 | ...[...] :  |
+| CommandInjection.rb:94:16:94:28 | ...[...] :  | CommandInjection.rb:95:16:95:28 | "cat #{...}" |
 nodes
 | CommandInjection.rb:6:15:6:20 | call to params :  | semmle.label | call to params :  |
 | CommandInjection.rb:6:15:6:26 | ...[...] :  | semmle.label | ...[...] :  |
@@ -37,6 +39,9 @@ nodes
 | CommandInjection.rb:82:14:82:39 | "echo #{...}" | semmle.label | "echo #{...}" |
 | CommandInjection.rb:82:22:82:25 | args :  | semmle.label | args :  |
 | CommandInjection.rb:82:22:82:37 | ...[...] :  | semmle.label | ...[...] :  |
+| CommandInjection.rb:94:16:94:21 | call to params :  | semmle.label | call to params :  |
+| CommandInjection.rb:94:16:94:28 | ...[...] :  | semmle.label | ...[...] :  |
+| CommandInjection.rb:95:16:95:28 | "cat #{...}" | semmle.label | "cat #{...}" |
 subpaths
 #select
 | CommandInjection.rb:7:10:7:15 | #{...} | CommandInjection.rb:6:15:6:20 | call to params :  | CommandInjection.rb:7:10:7:15 | #{...} | This command depends on a $@. | CommandInjection.rb:6:15:6:20 | call to params | user-provided value |
@@ -51,3 +56,4 @@ subpaths
 | CommandInjection.rb:65:14:65:29 | "echo #{...}" | CommandInjection.rb:64:18:64:23 | number :  | CommandInjection.rb:65:14:65:29 | "echo #{...}" | This command depends on a $@. | CommandInjection.rb:64:18:64:23 | number | user-provided value |
 | CommandInjection.rb:73:14:73:34 | "echo #{...}" | CommandInjection.rb:72:23:72:33 | blah_number :  | CommandInjection.rb:73:14:73:34 | "echo #{...}" | This command depends on a $@. | CommandInjection.rb:72:23:72:33 | blah_number | user-provided value |
 | CommandInjection.rb:82:14:82:39 | "echo #{...}" | CommandInjection.rb:81:20:81:25 | **args :  | CommandInjection.rb:82:14:82:39 | "echo #{...}" | This command depends on a $@. | CommandInjection.rb:81:20:81:25 | **args | user-provided value |
+| CommandInjection.rb:95:16:95:28 | "cat #{...}" | CommandInjection.rb:94:16:94:21 | call to params :  | CommandInjection.rb:95:16:95:28 | "cat #{...}" | This command depends on a $@. | CommandInjection.rb:94:16:94:21 | call to params | user-provided value |
diff --git a/ruby/ql/test/query-tests/security/cwe-078/CommandInjection/CommandInjection.rb b/ruby/ql/test/query-tests/security/cwe-078/CommandInjection/CommandInjection.rb
index ed9750128cc..75219e2131c 100644
--- a/ruby/ql/test/query-tests/security/cwe-078/CommandInjection/CommandInjection.rb
+++ b/ruby/ql/test/query-tests/security/cwe-078/CommandInjection/CommandInjection.rb
@@ -88,3 +88,13 @@ module Types
     end
   end
 end
+
+class Foo < ActionController::Base
+    def create
+        file = params[:file]
+        system("cat #{file}")
+        # .shellescape
+        system("cat #{file.shellescape}") # OK, because file is shell escaped
+        
+    end
+end
\ No newline at end of file

From 75422dfa72238e49bef88353f58724ba8fe8c4e0 Mon Sep 17 00:00:00 2001
From: erik-krogh <erik-krogh@github.com>
Date: Mon, 10 Oct 2022 15:02:18 +0200
Subject: [PATCH 03/11] add library for reasoning about gems and .gemspec files

---
 ruby/ql/lib/codeql/ruby/frameworks/Core.qll   |  1 +
 .../lib/codeql/ruby/frameworks/core/Gem.qll   | 92 +++++++++++++++++++
 2 files changed, 93 insertions(+)
 create mode 100644 ruby/ql/lib/codeql/ruby/frameworks/core/Gem.qll

diff --git a/ruby/ql/lib/codeql/ruby/frameworks/Core.qll b/ruby/ql/lib/codeql/ruby/frameworks/Core.qll
index 73824b123ff..3e61b50dc04 100644
--- a/ruby/ql/lib/codeql/ruby/frameworks/Core.qll
+++ b/ruby/ql/lib/codeql/ruby/frameworks/Core.qll
@@ -7,6 +7,7 @@ private import codeql.ruby.DataFlow
 private import codeql.ruby.dataflow.FlowSummary
 import core.BasicObject::BasicObject
 import core.Object::Object
+import core.Gem::Gem
 import core.Kernel::Kernel
 import core.Module
 import core.Array
diff --git a/ruby/ql/lib/codeql/ruby/frameworks/core/Gem.qll b/ruby/ql/lib/codeql/ruby/frameworks/core/Gem.qll
new file mode 100644
index 00000000000..41daf6ac3b8
--- /dev/null
+++ b/ruby/ql/lib/codeql/ruby/frameworks/core/Gem.qll
@@ -0,0 +1,92 @@
+/**
+ * Provides modeling for the `Gem` module and `.gemspec` files.
+ */
+
+private import ruby
+private import Ast
+private import codeql.ruby.ApiGraphs
+
+/** Provides modeling for the `Gem` module and `.gemspec` files. */
+module Gem {
+  /** A .gemspec file that lists properties of a Ruby gem. */
+  class GemSpec instanceof File {
+    API::Node specCall;
+
+    GemSpec() {
+      this.getExtension() = "gemspec" and
+      specCall = API::root().getMember("Gem").getMember("Specification").getMethod("new") and
+      specCall.getLocation().getFile() = this
+    }
+
+    /** Gets the name of this .gemspec file. */
+    string toString() { result = File.super.getBaseName() }
+
+    /**
+     * Gets a value of the `name` property of this .gemspec file.
+     * These properties are set using the `Gem::Specification.new` method.
+     */
+    private Expr getSpecProperty(string key) {
+      exists(Expr rhs |
+        rhs =
+          specCall
+              .getBlock()
+              .getParameter(0)
+              .getMethod(key + "=")
+              .getParameter(0)
+              .asSink()
+              .asExpr()
+              .getExpr()
+              .(Ast::AssignExpr)
+              .getRightOperand()
+      |
+        result = rhs
+        or
+        // some properties are arrays, we just unfold them
+        result = rhs.(ArrayLiteral).getAnElement()
+      )
+    }
+
+    /** Gets the name of the gem */
+    string getName() { result = getSpecProperty("name").getConstantValue().getString() }
+
+    /** Gets a path that is loaded when the gem is required */
+    private string getARequirePath() {
+      result = getSpecProperty(["require_paths", "require_path"]).getConstantValue().getString()
+      or
+      not exists(getSpecProperty(["require_paths", "require_path"]).getConstantValue().getString()) and
+      result = "lib" // the default is "lib"
+    }
+
+    /** Gets a file that is loaded when the gem is required. */
+    private File getAnRequiredFile() {
+      result = File.super.getParentContainer().getFolder(getARequirePath()).getAChildContainer*()
+    }
+
+    /** Gets a class/module that is exported by this gem. */
+    private ModuleBase getAPublicModule() {
+      result.(Toplevel).getLocation().getFile() = getAnRequiredFile()
+      or
+      result = getAPublicModule().getAModule()
+      or
+      result = getAPublicModule().getAClass()
+      or
+      result = getAPublicModule().getStmt(_).(SingletonClass)
+    }
+
+    /** Gets a parameter from an exported method, which is an input to this gem. */
+    DataFlow::ParameterNode getAnInputParameter() {
+      exists(MethodBase method | method = getAPublicModule().getAMethod() |
+        result.getParameter() = method.getAParameter() and
+        method.isPublic()
+      )
+    }
+  }
+
+  /** Gets a parameter that is an input to a named gem. */
+  DataFlow::ParameterNode getALibraryInput() {
+    exists(GemSpec spec |
+      exists(spec.getName()) and // we only consider `.gemspec` files that have a name
+      result = spec.getAnInputParameter()
+    )
+  }
+}

From 0d5da42ddd95c62d80ebbaaea437b65cb5a3d87a Mon Sep 17 00:00:00 2001
From: erik-krogh <erik-krogh@github.com>
Date: Mon, 10 Oct 2022 15:04:50 +0200
Subject: [PATCH 04/11] add a `getName()` utility to `DataFlow::ParameterNode`

---
 ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowPublic.qll | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowPublic.qll b/ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowPublic.qll
index f6ec8c4ab1b..a89e4964b8b 100644
--- a/ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowPublic.qll
+++ b/ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowPublic.qll
@@ -147,6 +147,9 @@ class ExprNode extends Node, TExprNode {
 class ParameterNode extends Node, TParameterNode instanceof ParameterNodeImpl {
   /** Gets the parameter corresponding to this node, if any. */
   final Parameter getParameter() { result = super.getParameter() }
+
+  /** Gets the name of the parameter, if any. */
+  final string getName() { result = this.getParameter().(NamedParameter).getName() }
 }
 
 /**

From 557dd108964cf9e04c8436a0935dc40820661e84 Mon Sep 17 00:00:00 2001
From: erik-krogh <erik-krogh@github.com>
Date: Mon, 10 Oct 2022 15:07:07 +0200
Subject: [PATCH 05/11] add a `rb/unsafe-shell-command-construction` query

---
 ...ShellCommandConstructionCustomizations.qll | 108 ++++++++++++++++++
 .../UnsafeShellCommandConstructionQuery.qll   |  35 ++++++
 .../cwe-078/UnsafeShellCommandConstruction.ql |  26 +++++
 .../UnsafeShellCommandConstruction.expected   |  40 +++++++
 .../UnsafeShellCommandConstruction.qlref      |   1 +
 .../impl/sub/notImported.rb                   |   6 +
 .../impl/sub/other.rb                         |   7 ++
 .../impl/sub/other2.rb                        |   5 +
 .../impl/unsafeShell.rb                       |  45 ++++++++
 .../unsafe-shell.gemspec                      |   5 +
 10 files changed, 278 insertions(+)
 create mode 100644 ruby/ql/lib/codeql/ruby/security/UnsafeShellCommandConstructionCustomizations.qll
 create mode 100644 ruby/ql/lib/codeql/ruby/security/UnsafeShellCommandConstructionQuery.qll
 create mode 100644 ruby/ql/src/queries/security/cwe-078/UnsafeShellCommandConstruction.ql
 create mode 100644 ruby/ql/test/query-tests/security/cwe-078/UnsafeShellCommandConstruction/UnsafeShellCommandConstruction.expected
 create mode 100644 ruby/ql/test/query-tests/security/cwe-078/UnsafeShellCommandConstruction/UnsafeShellCommandConstruction.qlref
 create mode 100644 ruby/ql/test/query-tests/security/cwe-078/UnsafeShellCommandConstruction/impl/sub/notImported.rb
 create mode 100644 ruby/ql/test/query-tests/security/cwe-078/UnsafeShellCommandConstruction/impl/sub/other.rb
 create mode 100644 ruby/ql/test/query-tests/security/cwe-078/UnsafeShellCommandConstruction/impl/sub/other2.rb
 create mode 100644 ruby/ql/test/query-tests/security/cwe-078/UnsafeShellCommandConstruction/impl/unsafeShell.rb
 create mode 100644 ruby/ql/test/query-tests/security/cwe-078/UnsafeShellCommandConstruction/unsafe-shell.gemspec

diff --git a/ruby/ql/lib/codeql/ruby/security/UnsafeShellCommandConstructionCustomizations.qll b/ruby/ql/lib/codeql/ruby/security/UnsafeShellCommandConstructionCustomizations.qll
new file mode 100644
index 00000000000..a235c53462f
--- /dev/null
+++ b/ruby/ql/lib/codeql/ruby/security/UnsafeShellCommandConstructionCustomizations.qll
@@ -0,0 +1,108 @@
+/**
+ * Provides default sources, sinks and sanitizers for reasoning about
+ * shell command constructed from library input vulnerabilities, as
+ * well as extension points for adding your own.
+ */
+
+private import codeql.ruby.DataFlow
+private import codeql.ruby.DataFlow2
+private import codeql.ruby.ApiGraphs
+private import codeql.ruby.frameworks.core.Gem::Gem as Gem
+private import codeql.ruby.AST as Ast
+private import codeql.ruby.Concepts as Concepts
+
+module UnsafeShellCommandConstruction {
+  /** A source for shell command constructed from library input vulnerabilities. */
+  abstract class Source extends DataFlow::Node { }
+
+  /** An input parameter to a gem seen as a source. */
+  private class LibraryInputAsSource extends Source instanceof DataFlow::ParameterNode {
+    LibraryInputAsSource() {
+      this = Gem::getALibraryInput() and
+      // we exclude arguments named `cmd` or similar, as they seem to execute commands on purpose
+      not exists(string name | name = super.getName() |
+        name = ["cmd", "command"]
+        or
+        name.regexpMatch(".*(Cmd|Command)$")
+      )
+    }
+  }
+
+  /** A sink for shell command constructed from library input vulnerabilities. */
+  abstract class Sink extends DataFlow::Node {
+    /** Gets a description of how the string in this sink was constructed. */
+    abstract string describe();
+
+    /** Gets the dataflow node where the string is constructed. */
+    DataFlow::Node getStringConstruction() { result = this }
+
+    /** Gets the dataflow node that executed the string as a shell command. */
+    abstract DataFlow::Node getCommandExecution();
+  }
+
+  /** A dataflow-configuration for tracking flow from various string constructions to places where those strings are executed as shell commands. */
+  class TrackSystemCommand extends DataFlow2::Configuration {
+    TrackSystemCommand() { this = "StringConcatAsSink::TrackSystemCommand" }
+
+    override predicate isSource(DataFlow::Node source) {
+      source instanceof TaintedFormat::PrintfStyleCall
+      or
+      source.asExpr().getExpr() =
+        any(Ast::StringLiteral lit |
+          lit.getComponent(_) instanceof Ast::StringInterpolationComponent
+        )
+    }
+
+    override predicate isSink(DataFlow::Node sink) {
+      exists(Concepts::SystemCommandExecution s | s.isShellInterpreted(sink))
+    }
+  }
+
+  /** Holds if the string constructed at `source` is executed at `shellExec` */
+  predicate isUsedAsShellCommand(DataFlow::Node source, Concepts::SystemCommandExecution shellExec) {
+    any(TrackSystemCommand conf)
+        .hasFlow(source, any(DataFlow::Node arg | shellExec.isShellInterpreted(arg)))
+  }
+
+  /**
+   * A string constructed from a string-literal (e.g. `"foo #{sink}"`),
+   * where the resulting string ends up being executed as a shell command.
+   */
+  class StringFormatAsSink extends Sink {
+    Concepts::SystemCommandExecution s;
+    Ast::StringLiteral lit;
+
+    StringFormatAsSink() {
+      isUsedAsShellCommand(any(DataFlow::Node n | n.asExpr().getExpr() = lit), s) and
+      this.asExpr().getExpr() = lit.getComponent(_)
+    }
+
+    override string describe() { result = "string construction" }
+
+    override DataFlow::Node getCommandExecution() { result = s }
+
+    override DataFlow::Node getStringConstruction() { result.asExpr().getExpr() = lit }
+  }
+
+  import codeql.ruby.security.TaintedFormatStringSpecific as TaintedFormat
+
+  /**
+   * A string constructed from a printf-style call,
+   * where the resulting string ends up being executed as a shell command.
+   */
+  class TaintedFormatStringAsSink extends Sink {
+    Concepts::SystemCommandExecution s;
+    TaintedFormat::PrintfStyleCall call;
+
+    TaintedFormatStringAsSink() {
+      isUsedAsShellCommand(call, s) and
+      this = [call.getFormatArgument(_), call.getFormatString()]
+    }
+
+    override string describe() { result = "formatted string" }
+
+    override DataFlow::Node getCommandExecution() { result = s }
+
+    override DataFlow::Node getStringConstruction() { result = call }
+  }
+}
diff --git a/ruby/ql/lib/codeql/ruby/security/UnsafeShellCommandConstructionQuery.qll b/ruby/ql/lib/codeql/ruby/security/UnsafeShellCommandConstructionQuery.qll
new file mode 100644
index 00000000000..87b602911ae
--- /dev/null
+++ b/ruby/ql/lib/codeql/ruby/security/UnsafeShellCommandConstructionQuery.qll
@@ -0,0 +1,35 @@
+/**
+ * Provides a taint tracking configuration for reasoning about shell command
+ * constructed from library input vulnerabilities
+ *
+ * Note, for performance reasons: only import this file if `Configuration` is needed,
+ * otherwise `UnsafeShellCommandConstructionCustomizations` should be imported instead.
+ */
+
+import codeql.ruby.DataFlow
+import UnsafeShellCommandConstructionCustomizations::UnsafeShellCommandConstruction
+private import codeql.ruby.TaintTracking
+private import CommandInjectionCustomizations::CommandInjection as CommandInjection
+private import codeql.ruby.dataflow.BarrierGuards
+
+/**
+ * A taint-tracking configuration for detecting shell command constructed from library input vulnerabilities.
+ */
+class Configuration extends TaintTracking::Configuration {
+  Configuration() { this = "UnsafeShellCommandConstruction" }
+
+  override predicate isSource(DataFlow::Node source) { source instanceof Source }
+
+  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 or
+    node instanceof StringConstArrayInclusionCallBarrier
+  }
+
+  // override to require the path doesn't have unmatched return steps
+  override DataFlow::FlowFeature getAFeature() {
+    result instanceof DataFlow::FeatureHasSourceCallContext
+  }
+}
diff --git a/ruby/ql/src/queries/security/cwe-078/UnsafeShellCommandConstruction.ql b/ruby/ql/src/queries/security/cwe-078/UnsafeShellCommandConstruction.ql
new file mode 100644
index 00000000000..53c71cfc314
--- /dev/null
+++ b/ruby/ql/src/queries/security/cwe-078/UnsafeShellCommandConstruction.ql
@@ -0,0 +1,26 @@
+/**
+ * @name Unsafe shell command constructed from library input
+ * @description Using externally controlled strings in a command line may allow a malicious
+ *              user to change the meaning of the command.
+ * @kind path-problem
+ * @problem.severity error
+ * @security-severity 6.3
+ * @precision high
+ * @id rb/shell-command-constructed-from-input
+ * @tags correctness
+ *       security
+ *       external/cwe/cwe-078
+ *       external/cwe/cwe-088
+ *       external/cwe/cwe-073
+ */
+
+import codeql.ruby.security.UnsafeShellCommandConstructionQuery
+import DataFlow::PathGraph
+
+from Configuration config, DataFlow::PathNode source, DataFlow::PathNode sink, Sink sinkNode
+where
+  config.hasFlowPath(source, sink) and
+  sinkNode = sink.getNode()
+select sinkNode.getStringConstruction(), source, sink,
+  "This " + sinkNode.describe() + " which depends on $@ is later used in a $@.", source.getNode(),
+  "library input", sinkNode.getCommandExecution(), "shell command"
diff --git a/ruby/ql/test/query-tests/security/cwe-078/UnsafeShellCommandConstruction/UnsafeShellCommandConstruction.expected b/ruby/ql/test/query-tests/security/cwe-078/UnsafeShellCommandConstruction/UnsafeShellCommandConstruction.expected
new file mode 100644
index 00000000000..3cc91bebdd9
--- /dev/null
+++ b/ruby/ql/test/query-tests/security/cwe-078/UnsafeShellCommandConstruction/UnsafeShellCommandConstruction.expected
@@ -0,0 +1,40 @@
+edges
+| impl/sub/notImported.rb:2:12:2:17 | target :  | impl/sub/notImported.rb:3:19:3:27 | #{...} |
+| impl/sub/other2.rb:2:12:2:17 | target :  | impl/sub/other2.rb:3:19:3:27 | #{...} |
+| impl/sub/other.rb:2:12:2:17 | target :  | impl/sub/other.rb:3:19:3:27 | #{...} |
+| impl/unsafeShell.rb:2:12:2:17 | target :  | impl/unsafeShell.rb:3:19:3:27 | #{...} |
+| impl/unsafeShell.rb:6:12:6:12 | x :  | impl/unsafeShell.rb:7:32:7:32 | x |
+| impl/unsafeShell.rb:15:47:15:64 | innocent_file_path :  | impl/unsafeShell.rb:20:21:20:41 | #{...} |
+| impl/unsafeShell.rb:23:15:23:23 | file_path :  | impl/unsafeShell.rb:26:19:26:30 | #{...} |
+| impl/unsafeShell.rb:33:12:33:17 | target :  | impl/unsafeShell.rb:34:19:34:27 | #{...} |
+| impl/unsafeShell.rb:37:10:37:10 | x :  | impl/unsafeShell.rb:38:19:38:22 | #{...} |
+nodes
+| impl/sub/notImported.rb:2:12:2:17 | target :  | semmle.label | target :  |
+| impl/sub/notImported.rb:3:19:3:27 | #{...} | semmle.label | #{...} |
+| impl/sub/other2.rb:2:12:2:17 | target :  | semmle.label | target :  |
+| impl/sub/other2.rb:3:19:3:27 | #{...} | semmle.label | #{...} |
+| impl/sub/other.rb:2:12:2:17 | target :  | semmle.label | target :  |
+| impl/sub/other.rb:3:19:3:27 | #{...} | semmle.label | #{...} |
+| impl/unsafeShell.rb:2:12:2:17 | target :  | semmle.label | target :  |
+| impl/unsafeShell.rb:3:19:3:27 | #{...} | semmle.label | #{...} |
+| impl/unsafeShell.rb:6:12:6:12 | x :  | semmle.label | x :  |
+| impl/unsafeShell.rb:7:32:7:32 | x | semmle.label | x |
+| impl/unsafeShell.rb:15:47:15:64 | innocent_file_path :  | semmle.label | innocent_file_path :  |
+| impl/unsafeShell.rb:20:21:20:41 | #{...} | semmle.label | #{...} |
+| impl/unsafeShell.rb:23:15:23:23 | file_path :  | semmle.label | file_path :  |
+| impl/unsafeShell.rb:26:19:26:30 | #{...} | semmle.label | #{...} |
+| impl/unsafeShell.rb:33:12:33:17 | target :  | semmle.label | target :  |
+| impl/unsafeShell.rb:34:19:34:27 | #{...} | semmle.label | #{...} |
+| impl/unsafeShell.rb:37:10:37:10 | x :  | semmle.label | x :  |
+| impl/unsafeShell.rb:38:19:38:22 | #{...} | semmle.label | #{...} |
+subpaths
+#select
+| impl/sub/notImported.rb:3:14:3:28 | "cat #{...}" | impl/sub/notImported.rb:2:12:2:17 | target :  | impl/sub/notImported.rb:3:19:3:27 | #{...} | This string construction which depends on $@ is later used in a $@. | impl/sub/notImported.rb:2:12:2:17 | target | library input | impl/sub/notImported.rb:3:5:3:34 | call to popen | shell command |
+| impl/sub/other2.rb:3:14:3:28 | "cat #{...}" | impl/sub/other2.rb:2:12:2:17 | target :  | impl/sub/other2.rb:3:19:3:27 | #{...} | This string construction which depends on $@ is later used in a $@. | impl/sub/other2.rb:2:12:2:17 | target | library input | impl/sub/other2.rb:3:5:3:34 | call to popen | shell command |
+| impl/sub/other.rb:3:14:3:28 | "cat #{...}" | impl/sub/other.rb:2:12:2:17 | target :  | impl/sub/other.rb:3:19:3:27 | #{...} | This string construction which depends on $@ is later used in a $@. | impl/sub/other.rb:2:12:2:17 | target | library input | impl/sub/other.rb:3:5:3:34 | call to popen | shell command |
+| impl/unsafeShell.rb:3:14:3:28 | "cat #{...}" | impl/unsafeShell.rb:2:12:2:17 | target :  | impl/unsafeShell.rb:3:19:3:27 | #{...} | This string construction which depends on $@ is later used in a $@. | impl/unsafeShell.rb:2:12:2:17 | target | library input | impl/unsafeShell.rb:3:5:3:34 | call to popen | shell command |
+| impl/unsafeShell.rb:7:14:7:33 | call to sprintf | impl/unsafeShell.rb:6:12:6:12 | x :  | impl/unsafeShell.rb:7:32:7:32 | x | This formatted string which depends on $@ is later used in a $@. | impl/unsafeShell.rb:6:12:6:12 | x | library input | impl/unsafeShell.rb:8:5:8:25 | call to popen | shell command |
+| impl/unsafeShell.rb:20:14:20:42 | "which #{...}" | impl/unsafeShell.rb:15:47:15:64 | innocent_file_path :  | impl/unsafeShell.rb:20:21:20:41 | #{...} | This string construction which depends on $@ is later used in a $@. | impl/unsafeShell.rb:15:47:15:64 | innocent_file_path | library input | impl/unsafeShell.rb:20:5:20:48 | call to popen | shell command |
+| impl/unsafeShell.rb:26:14:26:31 | "cat #{...}" | impl/unsafeShell.rb:23:15:23:23 | file_path :  | impl/unsafeShell.rb:26:19:26:30 | #{...} | This string construction which depends on $@ is later used in a $@. | impl/unsafeShell.rb:23:15:23:23 | file_path | library input | impl/unsafeShell.rb:26:5:26:37 | call to popen | shell command |
+| impl/unsafeShell.rb:34:14:34:28 | "cat #{...}" | impl/unsafeShell.rb:33:12:33:17 | target :  | impl/unsafeShell.rb:34:19:34:27 | #{...} | This string construction which depends on $@ is later used in a $@. | impl/unsafeShell.rb:33:12:33:17 | target | library input | impl/unsafeShell.rb:34:5:34:34 | call to popen | shell command |
+| impl/unsafeShell.rb:38:14:38:23 | "cat #{...}" | impl/unsafeShell.rb:37:10:37:10 | x :  | impl/unsafeShell.rb:38:19:38:22 | #{...} | This string construction which depends on $@ is later used in a $@. | impl/unsafeShell.rb:37:10:37:10 | x | library input | impl/unsafeShell.rb:38:5:38:29 | call to popen | shell command |
diff --git a/ruby/ql/test/query-tests/security/cwe-078/UnsafeShellCommandConstruction/UnsafeShellCommandConstruction.qlref b/ruby/ql/test/query-tests/security/cwe-078/UnsafeShellCommandConstruction/UnsafeShellCommandConstruction.qlref
new file mode 100644
index 00000000000..99292da7663
--- /dev/null
+++ b/ruby/ql/test/query-tests/security/cwe-078/UnsafeShellCommandConstruction/UnsafeShellCommandConstruction.qlref
@@ -0,0 +1 @@
+queries/security/cwe-078/UnsafeShellCommandConstruction.ql
\ No newline at end of file
diff --git a/ruby/ql/test/query-tests/security/cwe-078/UnsafeShellCommandConstruction/impl/sub/notImported.rb b/ruby/ql/test/query-tests/security/cwe-078/UnsafeShellCommandConstruction/impl/sub/notImported.rb
new file mode 100644
index 00000000000..0a385f5f6bc
--- /dev/null
+++ b/ruby/ql/test/query-tests/security/cwe-078/UnsafeShellCommandConstruction/impl/sub/notImported.rb
@@ -0,0 +1,6 @@
+class Foobar
+  def foo1(target)
+    IO.popen("cat #{target}", "w") # NOT OK - everything assumed to be imported...
+  end
+end
+  
\ No newline at end of file
diff --git a/ruby/ql/test/query-tests/security/cwe-078/UnsafeShellCommandConstruction/impl/sub/other.rb b/ruby/ql/test/query-tests/security/cwe-078/UnsafeShellCommandConstruction/impl/sub/other.rb
new file mode 100644
index 00000000000..22eaa13bcc0
--- /dev/null
+++ b/ruby/ql/test/query-tests/security/cwe-078/UnsafeShellCommandConstruction/impl/sub/other.rb
@@ -0,0 +1,7 @@
+class Foobar
+  def foo1(target)
+    IO.popen("cat #{target}", "w") # NOT OK
+  end
+end
+
+require 'sub/other2'
\ No newline at end of file
diff --git a/ruby/ql/test/query-tests/security/cwe-078/UnsafeShellCommandConstruction/impl/sub/other2.rb b/ruby/ql/test/query-tests/security/cwe-078/UnsafeShellCommandConstruction/impl/sub/other2.rb
new file mode 100644
index 00000000000..007dae343ff
--- /dev/null
+++ b/ruby/ql/test/query-tests/security/cwe-078/UnsafeShellCommandConstruction/impl/sub/other2.rb
@@ -0,0 +1,5 @@
+class Foobar
+  def foo1(target)
+    IO.popen("cat #{target}", "w") # NOT OK
+  end
+end
\ No newline at end of file
diff --git a/ruby/ql/test/query-tests/security/cwe-078/UnsafeShellCommandConstruction/impl/unsafeShell.rb b/ruby/ql/test/query-tests/security/cwe-078/UnsafeShellCommandConstruction/impl/unsafeShell.rb
new file mode 100644
index 00000000000..52e3016ac91
--- /dev/null
+++ b/ruby/ql/test/query-tests/security/cwe-078/UnsafeShellCommandConstruction/impl/unsafeShell.rb
@@ -0,0 +1,45 @@
+class Foobar
+  def foo1(target)
+    IO.popen("cat #{target}", "w") # NOT OK
+  end
+
+  def foo2(x) 
+    format = sprintf("cat %s", x) # NOT OK
+    IO.popen(format, "w")
+  end
+
+  def fileRead1(path) 
+    File.read(path) # OK
+  end
+
+  def my_exec(cmd, command, myCmd, myCommand, innocent_file_path) 
+    IO.popen("which #{cmd}", "w") # OK - the parameter is named `cmd`, so it's meant to be a command
+    IO.popen("which #{command}", "w") # OK - the parameter is named `command`, so it's meant to be a command
+    IO.popen("which #{myCmd}", "w") # OK - the parameter is named `myCmd`, so it's meant to be a command
+    IO.popen("which #{myCommand}", "w") # OK - the parameter is named `myCommand`, so it's meant to be a command
+    IO.popen("which #{innocent_file_path}", "w") # NOT OK - the parameter is named `innocent_file_path`, so it's not meant to be a command
+  end
+
+  def escaped(file_path)
+    IO.popen("cat #{file_path.shellescape}", "w") # OK - the parameter is escaped
+
+    IO.popen("cat #{file_path}", "w") # NOT OK - the parameter is not escaped
+  end
+end
+
+require File.join(File.dirname(__FILE__), 'sub', 'other')
+
+class Foobar2
+  def foo1(target)
+    IO.popen("cat #{target}", "w") # NOT OK
+  end
+
+  def id(x) 
+    IO.popen("cat #{x}", "w") # NOT OK - the parameter is not a constant.
+    return x
+  end
+    
+  def thisIsSafe()
+    IO.popen("echo #{id('foo')}", "w") # OK - only using constants.
+  end    
+end
diff --git a/ruby/ql/test/query-tests/security/cwe-078/UnsafeShellCommandConstruction/unsafe-shell.gemspec b/ruby/ql/test/query-tests/security/cwe-078/UnsafeShellCommandConstruction/unsafe-shell.gemspec
new file mode 100644
index 00000000000..545bc14a7a6
--- /dev/null
+++ b/ruby/ql/test/query-tests/security/cwe-078/UnsafeShellCommandConstruction/unsafe-shell.gemspec
@@ -0,0 +1,5 @@
+Gem::Specification.new do |s|
+    s.name = 'unsafe-shell'
+    s.require_path = "impl"
+  end
+  
\ No newline at end of file

From d427e555077158829997df0a83ad7b1f0dabbe8f Mon Sep 17 00:00:00 2001
From: erik-krogh <erik-krogh@github.com>
Date: Mon, 10 Oct 2022 15:07:14 +0200
Subject: [PATCH 06/11] add qhelp

---
 .../UnsafeShellCommandConstruction.qhelp      | 73 +++++++++++++++++++
 .../unsafe-shell-command-construction.rb      |  5 ++
 ...unsafe-shell-command-construction_fixed.rb |  6 ++
 3 files changed, 84 insertions(+)
 create mode 100644 ruby/ql/src/queries/security/cwe-078/UnsafeShellCommandConstruction.qhelp
 create mode 100644 ruby/ql/src/queries/security/cwe-078/examples/unsafe-shell-command-construction.rb
 create mode 100644 ruby/ql/src/queries/security/cwe-078/examples/unsafe-shell-command-construction_fixed.rb

diff --git a/ruby/ql/src/queries/security/cwe-078/UnsafeShellCommandConstruction.qhelp b/ruby/ql/src/queries/security/cwe-078/UnsafeShellCommandConstruction.qhelp
new file mode 100644
index 00000000000..88cea1d80d3
--- /dev/null
+++ b/ruby/ql/src/queries/security/cwe-078/UnsafeShellCommandConstruction.qhelp
@@ -0,0 +1,73 @@
+<!DOCTYPE qhelp PUBLIC
+"-//Semmle//qhelp//EN"
+"qhelp.dtd">
+<qhelp>
+<overview>
+	<p>
+		Dynamically constructing a shell command with inputs from exported 
+		functions may inadvertently change the meaning of the shell command.
+		
+		Clients using the exported function may use inputs containing
+		characters that the shell interprets in a special way, for instance
+		quotes and spaces.
+
+		This can result in the shell command misbehaving, or even
+		allowing a malicious user to execute arbitrary commands on the system.
+	</p>
+
+
+</overview>
+<recommendation>
+
+	<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.
+	</p>
+
+	<p>
+		Alternatively, if the shell command must be constructed
+		dynamically, then add code to ensure that special characters 
+		do not alter the shell command unexpectedly.
+	</p>
+
+</recommendation>
+<example>
+
+	<p>
+		The following example shows a dynamically constructed shell
+		command that downloads a file from a remote URL.
+	</p>
+
+	<sample src="examples/unsafe-shell-command-construction.rb" />
+
+	<p>
+		The shell command will, however, fail to work as intended if the
+		input contains spaces or other special characters interpreted in a 
+		special way by the shell. 
+	</p>
+
+	<p>
+		Even worse, a client might pass in user-controlled
+		data, not knowing that the input is interpreted as a shell command. 
+		This could allow a malicious user to provide the input <code>http://example.org; cat /etc/passwd</code>
+		in order to execute the command <code>cat /etc/passwd</code>.
+	</p>
+
+	<p>
+		To avoid such potentially catastrophic behaviors, provide the
+		inputs from exported functions as an argument that does not
+		get interpreted by a shell:
+	</p>
+
+	<sample src="examples/unsafe-shell-command-construction_fixed.rb" />
+
+</example>
+<references>
+
+	<li>
+		OWASP:
+		<a href="https://www.owasp.org/index.php/Command_Injection">Command Injection</a>.
+	</li>
+
+</references>
+</qhelp>
diff --git a/ruby/ql/src/queries/security/cwe-078/examples/unsafe-shell-command-construction.rb b/ruby/ql/src/queries/security/cwe-078/examples/unsafe-shell-command-construction.rb
new file mode 100644
index 00000000000..500bd49e890
--- /dev/null
+++ b/ruby/ql/src/queries/security/cwe-078/examples/unsafe-shell-command-construction.rb
@@ -0,0 +1,5 @@
+module Utils 
+    def download(path)
+        system("wget #{path}") # NOT OK
+    end
+end
\ No newline at end of file
diff --git a/ruby/ql/src/queries/security/cwe-078/examples/unsafe-shell-command-construction_fixed.rb b/ruby/ql/src/queries/security/cwe-078/examples/unsafe-shell-command-construction_fixed.rb
new file mode 100644
index 00000000000..cb8730eee09
--- /dev/null
+++ b/ruby/ql/src/queries/security/cwe-078/examples/unsafe-shell-command-construction_fixed.rb
@@ -0,0 +1,6 @@
+module Utils 
+    def download(path)
+        # using an array to call `system` is safe
+        system("wget", path) # OK
+    end
+end
\ No newline at end of file

From cadb948d576e938368313d53e1ca35dea6f9bc0a Mon Sep 17 00:00:00 2001
From: erik-krogh <erik-krogh@github.com>
Date: Mon, 10 Oct 2022 15:07:20 +0200
Subject: [PATCH 07/11] add change-note

---
 .../2022-10-10-unsafe-shell-command-construction.md           | 4 ++++
 1 file changed, 4 insertions(+)
 create mode 100644 ruby/ql/src/change-notes/2022-10-10-unsafe-shell-command-construction.md

diff --git a/ruby/ql/src/change-notes/2022-10-10-unsafe-shell-command-construction.md b/ruby/ql/src/change-notes/2022-10-10-unsafe-shell-command-construction.md
new file mode 100644
index 00000000000..d61d32dcc5f
--- /dev/null
+++ b/ruby/ql/src/change-notes/2022-10-10-unsafe-shell-command-construction.md
@@ -0,0 +1,4 @@
+---
+category: newQuery
+---
+* Added a new query, `rb/shell-command-constructed-from-input`, to detect libraries that unsafely constructs shell commands from their inputs.

From b64a1b7c42f2e7ed3214f5911bd8563c6cebaebc Mon Sep 17 00:00:00 2001
From: erik-krogh <erik-krogh@github.com>
Date: Mon, 10 Oct 2022 15:14:49 +0200
Subject: [PATCH 08/11] add a missing qldoc

---
 .../security/UnsafeShellCommandConstructionCustomizations.qll  | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/ruby/ql/lib/codeql/ruby/security/UnsafeShellCommandConstructionCustomizations.qll b/ruby/ql/lib/codeql/ruby/security/UnsafeShellCommandConstructionCustomizations.qll
index a235c53462f..94b16935a55 100644
--- a/ruby/ql/lib/codeql/ruby/security/UnsafeShellCommandConstructionCustomizations.qll
+++ b/ruby/ql/lib/codeql/ruby/security/UnsafeShellCommandConstructionCustomizations.qll
@@ -11,6 +11,9 @@ private import codeql.ruby.frameworks.core.Gem::Gem as Gem
 private import codeql.ruby.AST as Ast
 private import codeql.ruby.Concepts as Concepts
 
+/**
+ * Module containing sources, sinks, and sanitizers for shell command constructed from library input.
+ */
 module UnsafeShellCommandConstruction {
   /** A source for shell command constructed from library input vulnerabilities. */
   abstract class Source extends DataFlow::Node { }

From 0220f0aa5cfb566a579dd10181875429ea78ab5e Mon Sep 17 00:00:00 2001
From: erik-krogh <erik-krogh@github.com>
Date: Tue, 11 Oct 2022 13:37:01 +0200
Subject: [PATCH 09/11] use type-tracking instead

---
 ...ShellCommandConstructionCustomizations.qll | 34 ++++++++-----------
 1 file changed, 14 insertions(+), 20 deletions(-)

diff --git a/ruby/ql/lib/codeql/ruby/security/UnsafeShellCommandConstructionCustomizations.qll b/ruby/ql/lib/codeql/ruby/security/UnsafeShellCommandConstructionCustomizations.qll
index 94b16935a55..b89743fe68e 100644
--- a/ruby/ql/lib/codeql/ruby/security/UnsafeShellCommandConstructionCustomizations.qll
+++ b/ruby/ql/lib/codeql/ruby/security/UnsafeShellCommandConstructionCustomizations.qll
@@ -43,28 +43,22 @@ module UnsafeShellCommandConstruction {
     abstract DataFlow::Node getCommandExecution();
   }
 
-  /** A dataflow-configuration for tracking flow from various string constructions to places where those strings are executed as shell commands. */
-  class TrackSystemCommand extends DataFlow2::Configuration {
-    TrackSystemCommand() { this = "StringConcatAsSink::TrackSystemCommand" }
-
-    override predicate isSource(DataFlow::Node source) {
-      source instanceof TaintedFormat::PrintfStyleCall
-      or
-      source.asExpr().getExpr() =
-        any(Ast::StringLiteral lit |
-          lit.getComponent(_) instanceof Ast::StringInterpolationComponent
-        )
-    }
-
-    override predicate isSink(DataFlow::Node sink) {
-      exists(Concepts::SystemCommandExecution s | s.isShellInterpreted(sink))
-    }
-  }
-
   /** Holds if the string constructed at `source` is executed at `shellExec` */
   predicate isUsedAsShellCommand(DataFlow::Node source, Concepts::SystemCommandExecution shellExec) {
-    any(TrackSystemCommand conf)
-        .hasFlow(source, any(DataFlow::Node arg | shellExec.isShellInterpreted(arg)))
+    source = backtrackShellExec(TypeTracker::TypeBackTracker::end(), shellExec)
+  }
+
+  import codeql.ruby.typetracking.TypeTracker as TypeTracker
+
+  private DataFlow::LocalSourceNode backtrackShellExec(
+    TypeTracker::TypeBackTracker t, Concepts::SystemCommandExecution shellExec
+  ) {
+    t.start() and
+    result = any(DataFlow::Node n | shellExec.isShellInterpreted(n)).getALocalSource()
+    or
+    exists(TypeTracker::TypeBackTracker t2 |
+      result = backtrackShellExec(t2, shellExec).backtrack(t2, t)
+    )
   }
 
   /**

From f3741ff1e44745ab18fa92b09cedfd860f9f6d09 Mon Sep 17 00:00:00 2001
From: erik-krogh <erik-krogh@github.com>
Date: Thu, 3 Nov 2022 09:41:05 +0100
Subject: [PATCH 10/11] changes based on review

---
 .../lib/codeql/ruby/frameworks/core/Gem.qll   | 22 ++++++++++++++-----
 ...ShellCommandConstructionCustomizations.qll |  4 ++--
 .../UnsafeShellCommandConstruction.expected   |  4 ++++
 .../impl/unsafeShell.rb                       |  5 +++++
 4 files changed, 27 insertions(+), 8 deletions(-)

diff --git a/ruby/ql/lib/codeql/ruby/frameworks/core/Gem.qll b/ruby/ql/lib/codeql/ruby/frameworks/core/Gem.qll
index 41daf6ac3b8..9e417ad371a 100644
--- a/ruby/ql/lib/codeql/ruby/frameworks/core/Gem.qll
+++ b/ruby/ql/lib/codeql/ruby/frameworks/core/Gem.qll
@@ -8,7 +8,17 @@ private import codeql.ruby.ApiGraphs
 
 /** Provides modeling for the `Gem` module and `.gemspec` files. */
 module Gem {
-  /** A .gemspec file that lists properties of a Ruby gem. */
+  /**
+   * A .gemspec file that lists properties of a Ruby gem.
+   * The contents of a .gemspec file generally look like:
+   * ```Ruby
+   * Gem::Specification.new do |s|
+   *   s.name = 'library-name'
+   *   s.require_path = "lib"
+   *   # more properties
+   * end
+   * ```
+   */
   class GemSpec instanceof File {
     API::Node specCall;
 
@@ -25,13 +35,13 @@ module Gem {
      * Gets a value of the `name` property of this .gemspec file.
      * These properties are set using the `Gem::Specification.new` method.
      */
-    private Expr getSpecProperty(string key) {
+    private Expr getSpecProperty(string name) {
       exists(Expr rhs |
         rhs =
           specCall
               .getBlock()
               .getParameter(0)
-              .getMethod(key + "=")
+              .getMethod(name + "=")
               .getParameter(0)
               .asSink()
               .asExpr()
@@ -57,14 +67,14 @@ module Gem {
       result = "lib" // the default is "lib"
     }
 
-    /** Gets a file that is loaded when the gem is required. */
-    private File getAnRequiredFile() {
+    /** Gets a file that could be loaded when the gem is required. */
+    private File getAPossiblyRequiredFile() {
       result = File.super.getParentContainer().getFolder(getARequirePath()).getAChildContainer*()
     }
 
     /** Gets a class/module that is exported by this gem. */
     private ModuleBase getAPublicModule() {
-      result.(Toplevel).getLocation().getFile() = getAnRequiredFile()
+      result.(Toplevel).getLocation().getFile() = getAPossiblyRequiredFile()
       or
       result = getAPublicModule().getAModule()
       or
diff --git a/ruby/ql/lib/codeql/ruby/security/UnsafeShellCommandConstructionCustomizations.qll b/ruby/ql/lib/codeql/ruby/security/UnsafeShellCommandConstructionCustomizations.qll
index b89743fe68e..515b563c1e1 100644
--- a/ruby/ql/lib/codeql/ruby/security/UnsafeShellCommandConstructionCustomizations.qll
+++ b/ruby/ql/lib/codeql/ruby/security/UnsafeShellCommandConstructionCustomizations.qll
@@ -65,11 +65,11 @@ module UnsafeShellCommandConstruction {
    * A string constructed from a string-literal (e.g. `"foo #{sink}"`),
    * where the resulting string ends up being executed as a shell command.
    */
-  class StringFormatAsSink extends Sink {
+  class StringInterpolationAsSink extends Sink {
     Concepts::SystemCommandExecution s;
     Ast::StringLiteral lit;
 
-    StringFormatAsSink() {
+    StringInterpolationAsSink() {
       isUsedAsShellCommand(any(DataFlow::Node n | n.asExpr().getExpr() = lit), s) and
       this.asExpr().getExpr() = lit.getComponent(_)
     }
diff --git a/ruby/ql/test/query-tests/security/cwe-078/UnsafeShellCommandConstruction/UnsafeShellCommandConstruction.expected b/ruby/ql/test/query-tests/security/cwe-078/UnsafeShellCommandConstruction/UnsafeShellCommandConstruction.expected
index 3cc91bebdd9..964fb4433ce 100644
--- a/ruby/ql/test/query-tests/security/cwe-078/UnsafeShellCommandConstruction/UnsafeShellCommandConstruction.expected
+++ b/ruby/ql/test/query-tests/security/cwe-078/UnsafeShellCommandConstruction/UnsafeShellCommandConstruction.expected
@@ -8,6 +8,7 @@ edges
 | impl/unsafeShell.rb:23:15:23:23 | file_path :  | impl/unsafeShell.rb:26:19:26:30 | #{...} |
 | impl/unsafeShell.rb:33:12:33:17 | target :  | impl/unsafeShell.rb:34:19:34:27 | #{...} |
 | impl/unsafeShell.rb:37:10:37:10 | x :  | impl/unsafeShell.rb:38:19:38:22 | #{...} |
+| impl/unsafeShell.rb:47:16:47:21 | target :  | impl/unsafeShell.rb:48:19:48:27 | #{...} |
 nodes
 | impl/sub/notImported.rb:2:12:2:17 | target :  | semmle.label | target :  |
 | impl/sub/notImported.rb:3:19:3:27 | #{...} | semmle.label | #{...} |
@@ -27,6 +28,8 @@ nodes
 | impl/unsafeShell.rb:34:19:34:27 | #{...} | semmle.label | #{...} |
 | impl/unsafeShell.rb:37:10:37:10 | x :  | semmle.label | x :  |
 | impl/unsafeShell.rb:38:19:38:22 | #{...} | semmle.label | #{...} |
+| impl/unsafeShell.rb:47:16:47:21 | target :  | semmle.label | target :  |
+| impl/unsafeShell.rb:48:19:48:27 | #{...} | semmle.label | #{...} |
 subpaths
 #select
 | impl/sub/notImported.rb:3:14:3:28 | "cat #{...}" | impl/sub/notImported.rb:2:12:2:17 | target :  | impl/sub/notImported.rb:3:19:3:27 | #{...} | This string construction which depends on $@ is later used in a $@. | impl/sub/notImported.rb:2:12:2:17 | target | library input | impl/sub/notImported.rb:3:5:3:34 | call to popen | shell command |
@@ -38,3 +41,4 @@ subpaths
 | impl/unsafeShell.rb:26:14:26:31 | "cat #{...}" | impl/unsafeShell.rb:23:15:23:23 | file_path :  | impl/unsafeShell.rb:26:19:26:30 | #{...} | This string construction which depends on $@ is later used in a $@. | impl/unsafeShell.rb:23:15:23:23 | file_path | library input | impl/unsafeShell.rb:26:5:26:37 | call to popen | shell command |
 | impl/unsafeShell.rb:34:14:34:28 | "cat #{...}" | impl/unsafeShell.rb:33:12:33:17 | target :  | impl/unsafeShell.rb:34:19:34:27 | #{...} | This string construction which depends on $@ is later used in a $@. | impl/unsafeShell.rb:33:12:33:17 | target | library input | impl/unsafeShell.rb:34:5:34:34 | call to popen | shell command |
 | impl/unsafeShell.rb:38:14:38:23 | "cat #{...}" | impl/unsafeShell.rb:37:10:37:10 | x :  | impl/unsafeShell.rb:38:19:38:22 | #{...} | This string construction which depends on $@ is later used in a $@. | impl/unsafeShell.rb:37:10:37:10 | x | library input | impl/unsafeShell.rb:38:5:38:29 | call to popen | shell command |
+| impl/unsafeShell.rb:48:14:48:28 | "cat #{...}" | impl/unsafeShell.rb:47:16:47:21 | target :  | impl/unsafeShell.rb:48:19:48:27 | #{...} | This string construction which depends on $@ is later used in a $@. | impl/unsafeShell.rb:47:16:47:21 | target | library input | impl/unsafeShell.rb:48:5:48:34 | call to popen | shell command |
diff --git a/ruby/ql/test/query-tests/security/cwe-078/UnsafeShellCommandConstruction/impl/unsafeShell.rb b/ruby/ql/test/query-tests/security/cwe-078/UnsafeShellCommandConstruction/impl/unsafeShell.rb
index 52e3016ac91..f1fd9f685a8 100644
--- a/ruby/ql/test/query-tests/security/cwe-078/UnsafeShellCommandConstruction/impl/unsafeShell.rb
+++ b/ruby/ql/test/query-tests/security/cwe-078/UnsafeShellCommandConstruction/impl/unsafeShell.rb
@@ -42,4 +42,9 @@ class Foobar2
   def thisIsSafe()
     IO.popen("echo #{id('foo')}", "w") # OK - only using constants.
   end    
+
+  # class methods
+  def self.foo(target)
+    IO.popen("cat #{target}", "w") # NOT OK
+  end
 end

From 3f871a08e2fa2ab7116e05d29b8a7a2bf4f49e9f Mon Sep 17 00:00:00 2001
From: Erik Krogh Kristensen <erik-krogh@github.com>
Date: Mon, 7 Nov 2022 16:29:10 +0100
Subject: [PATCH 11/11] apply suggestions from doc review

Co-authored-by: mc <42146119+mchammer01@users.noreply.github.com>
---
 .../2022-10-10-unsafe-shell-command-construction.md             | 2 +-
 .../security/cwe-078/UnsafeShellCommandConstruction.qhelp       | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/ruby/ql/src/change-notes/2022-10-10-unsafe-shell-command-construction.md b/ruby/ql/src/change-notes/2022-10-10-unsafe-shell-command-construction.md
index d61d32dcc5f..fba6a9304cf 100644
--- a/ruby/ql/src/change-notes/2022-10-10-unsafe-shell-command-construction.md
+++ b/ruby/ql/src/change-notes/2022-10-10-unsafe-shell-command-construction.md
@@ -1,4 +1,4 @@
 ---
 category: newQuery
 ---
-* Added a new query, `rb/shell-command-constructed-from-input`, to detect libraries that unsafely constructs shell commands from their inputs.
+* Added a new query, `rb/shell-command-constructed-from-input`, to detect libraries that unsafely construct shell commands from their inputs.
diff --git a/ruby/ql/src/queries/security/cwe-078/UnsafeShellCommandConstruction.qhelp b/ruby/ql/src/queries/security/cwe-078/UnsafeShellCommandConstruction.qhelp
index 88cea1d80d3..4231f7cb0b4 100644
--- a/ruby/ql/src/queries/security/cwe-078/UnsafeShellCommandConstruction.qhelp
+++ b/ruby/ql/src/queries/security/cwe-078/UnsafeShellCommandConstruction.qhelp
@@ -55,7 +55,7 @@
 
 	<p>
 		To avoid such potentially catastrophic behaviors, provide the
-		inputs from exported functions as an argument that does not
+		input from exported functions as an argument that does not
 		get interpreted by a shell:
 	</p>