Merge branch 'main' into rb/summarize-more

2026-04-28 10:15:14 +02:00 · 2022-10-05 09:55:23 +02:00
parent 8b7ec20573 1496c4f0e2
commit 6f74a52542
6 changed files with 24 additions and 113 deletions
--- a/ruby/ql/lib/codeql/ruby/dataflow/FlowSummary.qll
+++ b/ruby/ql/lib/codeql/ruby/dataflow/FlowSummary.qll
@@ -84,16 +84,6 @@ module SummaryComponent {
    result = SC::content(TElementLowerBoundContent(lower, true))
  }

-  /** Gets a summary component that represents a value in a pair at an unknown key. */
-  SummaryComponent pairValueUnknown() {
-    result = SC::content(TSingletonContent(TUnknownPairValueContent()))
-  }
-
-  /** Gets a summary component that represents a value in a pair at a known key. */
-  SummaryComponent pairValueKnown(ConstantValue key) {
-    result = SC::content(TSingletonContent(DataFlow::Content::getPairValueContent(key)))
-  }
-
  /** Gets a summary component that represents the return value of a call. */
  SummaryComponent return() { result = SC::return(any(NormalReturnKind rk)) }
 }
--- a/ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowPrivate.qll
+++ b/ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowPrivate.qll
@@ -277,6 +277,8 @@ private module Cached {
    } or
    TSynthHashSplatArgumentNode(CfgNodes::ExprNodes::CallCfgNode c) {
      exists(Argument arg | arg.isArgumentOf(c, any(ArgumentPosition pos | pos.isKeyword(_))))
+      or
+      c.getAnArgument() instanceof CfgNodes::ExprNodes::PairCfgNode
    }

  class TParameterNode =
@@ -411,8 +413,6 @@ private module Cached {
  newtype TContent =
    TKnownElementContent(ConstantValue cv) { trackKnownValue(cv) } or
    TUnknownElementContent() or
-    TKnownPairValueContent(ConstantValue cv) { trackKnownValue(cv) } or
-    TUnknownPairValueContent() or
    TFieldContent(string name) {
      name = any(InstanceVariable v).getName()
      or
@@ -449,8 +449,6 @@ private module Cached {

 class TElementContent = TKnownElementContent or TUnknownElementContent;

-class TPairValueContent = TKnownPairValueContent or TUnknownPairValueContent;
-
 import Cached

 /** Holds if `n` should be hidden from path explanations. */
@@ -1042,25 +1040,24 @@ predicate storeStep(Node node1, ContentSet c, Node node2) {
  or
  FlowSummaryImpl::Private::Steps::summaryStoreStep(node1, c, node2)
  or
-  // Needed for pairs passed into method calls where the key is not a symbol,
-  // that is, where it is not a keyword argument.
-  node2.asExpr() =
-    any(CfgNodes::ExprNodes::PairCfgNode pair |
-      exists(CfgNodes::ExprCfgNode key, ConstantValue cv |
-        key = pair.getKey() and
-        pair.getValue() = node1.asExpr() and
-        cv = key.getConstantValue() and
-        not cv.isSymbol(_) and // handled as a keyword argument
-        c.isSingleton(Content::getPairValueContent(cv))
-      )
+  // Wrap all key-value arguments in a synthesized hash-splat argument node
+  exists(CfgNodes::ExprNodes::CallCfgNode call | node2 = TSynthHashSplatArgumentNode(call) |
+    // symbol key
+    exists(ArgumentPosition keywordPos, string name |
+      node1.asExpr().(Argument).isArgumentOf(call, keywordPos) and
+      keywordPos.isKeyword(name) and
+      c = getKeywordContent(name)
+    )
+    or
+    // non-symbol key
+    exists(CfgNodes::ExprNodes::PairCfgNode pair, CfgNodes::ExprCfgNode key, ConstantValue cv |
+      node1.asExpr() = pair.getValue() and
+      pair = call.getAnArgument() and
+      key = pair.getKey() and
+      cv = key.getConstantValue() and
+      not cv.isSymbol(_) and
+      c.isSingleton(TKnownElementContent(cv))
    )
-  or
-  // Wrap all keyword arguments in a synthesized hash-splat argument node
-  exists(CfgNodes::ExprNodes::CallCfgNode call, ArgumentPosition keywordPos, string name |
-    node2 = TSynthHashSplatArgumentNode(call) and
-    node1.asExpr().(Argument).isArgumentOf(call, keywordPos) and
-    keywordPos.isKeyword(name) and
-    c = getKeywordContent(name)
  )
 }

--- a/ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowPublic.qll
+++ b/ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowPublic.qll
@@ -295,34 +295,6 @@ module Content {
    result = getElementContent(e.getConstantValue()).(KnownElementContent).getIndex()
  }

-  /** A value in a pair with a known or unknown key. */
-  class PairValueContent extends Content, TPairValueContent { }
-
-  /** A value in a pair with a known key. */
-  class KnownPairValueContent extends PairValueContent, TKnownPairValueContent {
-    private ConstantValue key;
-
-    KnownPairValueContent() { this = TKnownPairValueContent(key) }
-
-    /** Gets the index in the collection. */
-    ConstantValue getIndex() { result = key }
-
-    override string toString() { result = "pair " + key }
-  }
-
-  /** A value in a pair with an unknown key. */
-  class UnknownPairValueContent extends PairValueContent, TUnknownPairValueContent {
-    override string toString() { result = "pair" }
-  }
-
-  /** Gets the content representing a value in a pair corresponding to constant value `cv`. */
-  PairValueContent getPairValueContent(ConstantValue cv) {
-    result = TKnownPairValueContent(cv)
-    or
-    not exists(TKnownPairValueContent(cv)) and
-    result = TUnknownPairValueContent()
-  }
-
  /**
   * A value stored behind a getter/setter pair.
   *
--- a/ruby/ql/lib/codeql/ruby/dataflow/internal/FlowSummaryImplSpecific.qll
+++ b/ruby/ql/lib/codeql/ruby/dataflow/internal/FlowSummaryImplSpecific.qll
@@ -135,16 +135,6 @@ SummaryComponent interpretComponentSpecific(AccessPathToken c) {
      interpretElementArg(c.getAnArgument("WithoutElement")) and
    result = FlowSummary::SummaryComponent::withoutContent(cs)
  )
-  or
-  exists(string arg | arg = c.getAnArgument("PairValue") |
-    arg = "?" and
-    result = FlowSummary::SummaryComponent::pairValueUnknown()
-    or
-    exists(ConstantValue cv |
-      result = FlowSummary::SummaryComponent::pairValueKnown(cv) and
-      cv.serialize() = arg
-    )
-  )
 }

 /** Gets the textual representation of a summary component in the format used for flow summaries. */
--- a/ruby/ql/lib/codeql/ruby/frameworks/core/Hash.qll
+++ b/ruby/ql/lib/codeql/ruby/frameworks/core/Hash.qll
@@ -19,25 +19,6 @@ private import codeql.ruby.typetracking.TypeTrackerSpecific
 * in `Array.qll`.
 */
 module Hash {
-  /**
-   * Holds if `key` is used as the non-symbol key in a hash literal. For example
-   *
-   * ```rb
-   * {
-   *   :a => 1, # symbol
-   *   "b" => 2 # non-symbol, "b" is the key
-   * }
-   * ```
-   */
-  private predicate isHashLiteralNonSymbolKey(ConstantValue key) {
-    exists(Pair pair |
-      key = DataFlow::Content::getKnownElementIndex(pair.getKey()) and
-      // cannot use API graphs due to negative recursion
-      pair = any(MethodCall mc | mc.getMethodName() = "[]").getAnArgument() and
-      not key.isSymbol(_)
-    )
-  }
-
  /**
   * Gets a call to the method `name` invoked on the `Hash` object
   * (not on a hash instance).
@@ -53,15 +34,6 @@ module Hash {
    final override MethodCall getACallSimple() { result = getAStaticHashCall("[]") }

    override predicate propagatesFlowExt(string input, string output, boolean preservesValue) {
-      // { 'nonsymbol' => x }
-      exists(ConstantValue key |
-        isHashLiteralNonSymbolKey(key) and
-        input = "Argument[0..].PairValue[" + key.serialize() + "]" and
-        output = "ReturnValue.Element[" + key.serialize() + "]" and
-        preservesValue = true
-      )
-      or
-      // { symbol: x }
      // we make use of the special `hash-splat` argument kind, which contains all keyword
      // arguments wrapped in an implicit hash, as well as explicit hash splat arguments
      input = "Argument[hash-splat].WithElement[any]" and
--- a/ruby/ql/test/library-tests/dataflow/hash-flow/hash-flow.expected
+++ b/ruby/ql/test/library-tests/dataflow/hash-flow/hash-flow.expected
@@ -2,12 +2,9 @@ failures
 edges
 | hash_flow.rb:11:15:11:24 | call to taint :  | hash_flow.rb:22:10:22:13 | hash [element :a] :  |
 | hash_flow.rb:13:12:13:21 | call to taint :  | hash_flow.rb:24:10:24:13 | hash [element :c] :  |
-| hash_flow.rb:15:9:15:23 | Pair [pair e] :  | hash_flow.rb:26:10:26:13 | hash [element e] :  |
-| hash_flow.rb:15:14:15:23 | call to taint :  | hash_flow.rb:15:9:15:23 | Pair [pair e] :  |
-| hash_flow.rb:17:9:17:25 | Pair [pair g] :  | hash_flow.rb:28:10:28:13 | hash [element g] :  |
-| hash_flow.rb:17:16:17:25 | call to taint :  | hash_flow.rb:17:9:17:25 | Pair [pair g] :  |
-| hash_flow.rb:19:9:19:23 | Pair [pair 0] :  | hash_flow.rb:30:10:30:13 | hash [element 0] :  |
-| hash_flow.rb:19:14:19:23 | call to taint :  | hash_flow.rb:19:9:19:23 | Pair [pair 0] :  |
+| hash_flow.rb:15:14:15:23 | call to taint :  | hash_flow.rb:26:10:26:13 | hash [element e] :  |
+| hash_flow.rb:17:16:17:25 | call to taint :  | hash_flow.rb:28:10:28:13 | hash [element g] :  |
+| hash_flow.rb:19:14:19:23 | call to taint :  | hash_flow.rb:30:10:30:13 | hash [element 0] :  |
 | hash_flow.rb:22:10:22:13 | hash [element :a] :  | hash_flow.rb:22:10:22:17 | ...[...] |
 | hash_flow.rb:24:10:24:13 | hash [element :c] :  | hash_flow.rb:24:10:24:17 | ...[...] |
 | hash_flow.rb:26:10:26:13 | hash [element e] :  | hash_flow.rb:26:10:26:18 | ...[...] |
@@ -56,12 +53,10 @@ edges
 | hash_flow.rb:68:22:68:31 | call to taint :  | hash_flow.rb:68:13:68:39 | ...[...] [element :a] :  |
 | hash_flow.rb:69:10:69:14 | hash4 [element :a] :  | hash_flow.rb:69:10:69:18 | ...[...] |
 | hash_flow.rb:72:13:72:45 | ...[...] [element a] :  | hash_flow.rb:73:10:73:14 | hash5 [element a] :  |
-| hash_flow.rb:72:18:72:34 | Pair [pair a] :  | hash_flow.rb:72:13:72:45 | ...[...] [element a] :  |
-| hash_flow.rb:72:25:72:34 | call to taint :  | hash_flow.rb:72:18:72:34 | Pair [pair a] :  |
+| hash_flow.rb:72:25:72:34 | call to taint :  | hash_flow.rb:72:13:72:45 | ...[...] [element a] :  |
 | hash_flow.rb:73:10:73:14 | hash5 [element a] :  | hash_flow.rb:73:10:73:19 | ...[...] |
 | hash_flow.rb:76:13:76:47 | ...[...] [element a] :  | hash_flow.rb:77:10:77:14 | hash6 [element a] :  |
-| hash_flow.rb:76:19:76:35 | Pair [pair a] :  | hash_flow.rb:76:13:76:47 | ...[...] [element a] :  |
-| hash_flow.rb:76:26:76:35 | call to taint :  | hash_flow.rb:76:19:76:35 | Pair [pair a] :  |
+| hash_flow.rb:76:26:76:35 | call to taint :  | hash_flow.rb:76:13:76:47 | ...[...] [element a] :  |
 | hash_flow.rb:77:10:77:14 | hash6 [element a] :  | hash_flow.rb:77:10:77:19 | ...[...] |
 | hash_flow.rb:84:13:84:42 | call to [] [element :a] :  | hash_flow.rb:85:10:85:14 | hash1 [element :a] :  |
 | hash_flow.rb:84:26:84:35 | call to taint :  | hash_flow.rb:84:13:84:42 | call to [] [element :a] :  |
@@ -533,11 +528,8 @@ edges
 nodes
 | hash_flow.rb:11:15:11:24 | call to taint :  | semmle.label | call to taint :  |
 | hash_flow.rb:13:12:13:21 | call to taint :  | semmle.label | call to taint :  |
-| hash_flow.rb:15:9:15:23 | Pair [pair e] :  | semmle.label | Pair [pair e] :  |
 | hash_flow.rb:15:14:15:23 | call to taint :  | semmle.label | call to taint :  |
-| hash_flow.rb:17:9:17:25 | Pair [pair g] :  | semmle.label | Pair [pair g] :  |
 | hash_flow.rb:17:16:17:25 | call to taint :  | semmle.label | call to taint :  |
-| hash_flow.rb:19:9:19:23 | Pair [pair 0] :  | semmle.label | Pair [pair 0] :  |
 | hash_flow.rb:19:14:19:23 | call to taint :  | semmle.label | call to taint :  |
 | hash_flow.rb:22:10:22:13 | hash [element :a] :  | semmle.label | hash [element :a] :  |
 | hash_flow.rb:22:10:22:17 | ...[...] | semmle.label | ...[...] |
@@ -599,12 +591,10 @@ nodes
 | hash_flow.rb:69:10:69:14 | hash4 [element :a] :  | semmle.label | hash4 [element :a] :  |
 | hash_flow.rb:69:10:69:18 | ...[...] | semmle.label | ...[...] |
 | hash_flow.rb:72:13:72:45 | ...[...] [element a] :  | semmle.label | ...[...] [element a] :  |
-| hash_flow.rb:72:18:72:34 | Pair [pair a] :  | semmle.label | Pair [pair a] :  |
 | hash_flow.rb:72:25:72:34 | call to taint :  | semmle.label | call to taint :  |
 | hash_flow.rb:73:10:73:14 | hash5 [element a] :  | semmle.label | hash5 [element a] :  |
 | hash_flow.rb:73:10:73:19 | ...[...] | semmle.label | ...[...] |
 | hash_flow.rb:76:13:76:47 | ...[...] [element a] :  | semmle.label | ...[...] [element a] :  |
-| hash_flow.rb:76:19:76:35 | Pair [pair a] :  | semmle.label | Pair [pair a] :  |
 | hash_flow.rb:76:26:76:35 | call to taint :  | semmle.label | call to taint :  |
 | hash_flow.rb:77:10:77:14 | hash6 [element a] :  | semmle.label | hash6 [element a] :  |
 | hash_flow.rb:77:10:77:19 | ...[...] | semmle.label | ...[...] |