changes based on review

Author: erik-krogh

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

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(_)
     }

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 |

ruby/ql/test/query-tests/security/cwe-078/UnsafeShellCommandConstruction/impl/unsafeShell.rb

@@ -42,4 +42,9 @@ def id(x)
   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