Merge pull request #17880 from hvitved/ruby/symbol-string-key-indiffe…

…rence Ruby: Do not distinguish between symbols and strings in hash keys
github · Nov 1, 2024 · 03ffaac · 03ffaac
2 parents 03aef50 + 6b60865
commit 03ffaac
Show file tree

Hide file tree

Showing 8 changed files with 165 additions and 95 deletions.
diff --git a/ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowPublic.qll b/ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowPublic.qll
@@ -829,7 +829,28 @@ class ContentSet extends TContentSet {
     this.isAny() and
     exists(result)
     or
-    result = this.getAnElementReadContent()
+    exists(Content elementContent | elementContent = this.getAnElementReadContent() |
+      result = elementContent
+      or
+      // Do not distinguish symbol keys from string keys. This allows us to
+      // give more precise summaries for something like `with_indifferent_access`,
+      // and the amount of false-positive flow arising from this should be very
+      // limited.
+      elementContent =
+        any(Content::KnownElementContent known, ConstantValue cv |
+          cv = known.getIndex() and
+          result.(Content::KnownElementContent).getIndex() =
+            any(ConstantValue cv2 |
+              cv2.(ConstantValue::ConstantSymbolValue).getStringlikeValue() =
+                cv.(ConstantValue::ConstantStringValue).getStringlikeValue()
+              or
+              cv2.(ConstantValue::ConstantStringValue).getStringlikeValue() =
+                cv.(ConstantValue::ConstantSymbolValue).getStringlikeValue()
+            )
+        |
+          known
+        )
+    )
   }
 }
 

diff --git a/ruby/ql/lib/codeql/ruby/frameworks/ActiveSupport.qll b/ruby/ql/lib/codeql/ruby/frameworks/ActiveSupport.qll
@@ -121,16 +121,6 @@ module ActiveSupport {
      * Extensions to the `Hash` class.
      */
     module Hash {
-      private class WithIndifferentAccessSummary extends SimpleSummarizedCallable {
-        WithIndifferentAccessSummary() { this = "with_indifferent_access" }
-
-        override predicate propagatesFlow(string input, string output, boolean preservesValue) {
-          input = "Argument[self].Element[any]" and
-          output = "ReturnValue.Element[any]" and
-          preservesValue = true
-        }
-      }
-
       /**
        * Flow summary for `reverse_merge`, and its alias `with_defaults`.
        */
@@ -167,8 +157,9 @@ module ActiveSupport {
         }
 
         override predicate propagatesFlow(string input, string output, boolean preservesValue) {
-          input = "Argument[self].Element[any]" and
-          output = "ReturnValue.Element[?]" and
+          // keys are considered equal modulo string/symbol in our implementation
+          input = "Argument[self].WithElement[any]" and
+          output = "ReturnValue" and
           preservesValue = true
         }
       }

diff --git a/ruby/ql/test/library-tests/dataflow/flow-summaries/semantics.expected b/ruby/ql/test/library-tests/dataflow/flow-summaries/semantics.expected
@@ -458,22 +458,40 @@ edges
 | semantics.rb:263:14:263:14 | h [element :foo] | semantics.rb:263:10:263:15 | call to s31 | provenance |  |
 | semantics.rb:263:14:263:14 | h [element] | semantics.rb:263:10:263:15 | call to s31 | provenance |  |
 | semantics.rb:263:14:263:14 | h [element] | semantics.rb:263:10:263:15 | call to s31 | provenance |  |
+| semantics.rb:267:5:267:5 | [post] h [element :foo] | semantics.rb:268:5:268:5 | h [element :foo] | provenance |  |
+| semantics.rb:267:5:267:5 | [post] h [element :foo] | semantics.rb:268:5:268:5 | h [element :foo] | provenance |  |
+| semantics.rb:267:15:267:25 | call to source | semantics.rb:267:5:267:5 | [post] h [element :foo] | provenance |  |
+| semantics.rb:267:15:267:25 | call to source | semantics.rb:267:5:267:5 | [post] h [element :foo] | provenance |  |
+| semantics.rb:268:5:268:5 | [post] h [element :foo] | semantics.rb:269:5:269:5 | h [element :foo] | provenance |  |
+| semantics.rb:268:5:268:5 | [post] h [element :foo] | semantics.rb:269:5:269:5 | h [element :foo] | provenance |  |
 | semantics.rb:268:5:268:5 | [post] h [element foo] | semantics.rb:269:5:269:5 | h [element foo] | provenance |  |
 | semantics.rb:268:5:268:5 | [post] h [element foo] | semantics.rb:269:5:269:5 | h [element foo] | provenance |  |
+| semantics.rb:268:5:268:5 | h [element :foo] | semantics.rb:268:5:268:5 | [post] h [element :foo] | provenance |  |
+| semantics.rb:268:5:268:5 | h [element :foo] | semantics.rb:268:5:268:5 | [post] h [element :foo] | provenance |  |
 | semantics.rb:268:16:268:26 | call to source | semantics.rb:268:5:268:5 | [post] h [element foo] | provenance |  |
 | semantics.rb:268:16:268:26 | call to source | semantics.rb:268:5:268:5 | [post] h [element foo] | provenance |  |
+| semantics.rb:269:5:269:5 | [post] h [element :foo] | semantics.rb:270:5:270:5 | h [element :foo] | provenance |  |
+| semantics.rb:269:5:269:5 | [post] h [element :foo] | semantics.rb:270:5:270:5 | h [element :foo] | provenance |  |
 | semantics.rb:269:5:269:5 | [post] h [element foo] | semantics.rb:270:5:270:5 | h [element foo] | provenance |  |
 | semantics.rb:269:5:269:5 | [post] h [element foo] | semantics.rb:270:5:270:5 | h [element foo] | provenance |  |
+| semantics.rb:269:5:269:5 | h [element :foo] | semantics.rb:269:5:269:5 | [post] h [element :foo] | provenance |  |
+| semantics.rb:269:5:269:5 | h [element :foo] | semantics.rb:269:5:269:5 | [post] h [element :foo] | provenance |  |
 | semantics.rb:269:5:269:5 | h [element foo] | semantics.rb:269:5:269:5 | [post] h [element foo] | provenance |  |
 | semantics.rb:269:5:269:5 | h [element foo] | semantics.rb:269:5:269:5 | [post] h [element foo] | provenance |  |
+| semantics.rb:270:5:270:5 | [post] h [element :foo] | semantics.rb:273:14:273:14 | h [element :foo] | provenance |  |
+| semantics.rb:270:5:270:5 | [post] h [element :foo] | semantics.rb:273:14:273:14 | h [element :foo] | provenance |  |
 | semantics.rb:270:5:270:5 | [post] h [element foo] | semantics.rb:273:14:273:14 | h [element foo] | provenance |  |
 | semantics.rb:270:5:270:5 | [post] h [element foo] | semantics.rb:273:14:273:14 | h [element foo] | provenance |  |
+| semantics.rb:270:5:270:5 | h [element :foo] | semantics.rb:270:5:270:5 | [post] h [element :foo] | provenance |  |
+| semantics.rb:270:5:270:5 | h [element :foo] | semantics.rb:270:5:270:5 | [post] h [element :foo] | provenance |  |
 | semantics.rb:270:5:270:5 | h [element foo] | semantics.rb:270:5:270:5 | [post] h [element foo] | provenance |  |
 | semantics.rb:270:5:270:5 | h [element foo] | semantics.rb:270:5:270:5 | [post] h [element foo] | provenance |  |
 | semantics.rb:271:5:271:5 | [post] h [element] | semantics.rb:273:14:273:14 | h [element] | provenance |  |
 | semantics.rb:271:5:271:5 | [post] h [element] | semantics.rb:273:14:273:14 | h [element] | provenance |  |
 | semantics.rb:271:12:271:22 | call to source | semantics.rb:271:5:271:5 | [post] h [element] | provenance |  |
 | semantics.rb:271:12:271:22 | call to source | semantics.rb:271:5:271:5 | [post] h [element] | provenance |  |
+| semantics.rb:273:14:273:14 | h [element :foo] | semantics.rb:273:10:273:15 | call to s32 | provenance |  |
+| semantics.rb:273:14:273:14 | h [element :foo] | semantics.rb:273:10:273:15 | call to s32 | provenance |  |
 | semantics.rb:273:14:273:14 | h [element foo] | semantics.rb:273:10:273:15 | call to s32 | provenance |  |
 | semantics.rb:273:14:273:14 | h [element foo] | semantics.rb:273:10:273:15 | call to s32 | provenance |  |
 | semantics.rb:273:14:273:14 | h [element] | semantics.rb:273:10:273:15 | call to s32 | provenance |  |
@@ -538,6 +556,8 @@ edges
 | semantics.rb:291:10:291:10 | x [element :foo] | semantics.rb:291:10:291:16 | ...[...] | provenance |  |
 | semantics.rb:293:10:293:10 | x [element :foo] | semantics.rb:293:10:293:13 | ...[...] | provenance |  |
 | semantics.rb:293:10:293:10 | x [element :foo] | semantics.rb:293:10:293:13 | ...[...] | provenance |  |
+| semantics.rb:297:5:297:5 | x [element foo] | semantics.rb:298:10:298:10 | x [element foo] | provenance |  |
+| semantics.rb:297:5:297:5 | x [element foo] | semantics.rb:298:10:298:10 | x [element foo] | provenance |  |
 | semantics.rb:297:5:297:5 | x [element foo] | semantics.rb:299:10:299:10 | x [element foo] | provenance |  |
 | semantics.rb:297:5:297:5 | x [element foo] | semantics.rb:299:10:299:10 | x [element foo] | provenance |  |
 | semantics.rb:297:5:297:5 | x [element foo] | semantics.rb:301:10:301:10 | x [element foo] | provenance |  |
@@ -546,6 +566,8 @@ edges
 | semantics.rb:297:9:297:24 | call to s36 [element foo] | semantics.rb:297:5:297:5 | x [element foo] | provenance |  |
 | semantics.rb:297:13:297:23 | call to source | semantics.rb:297:9:297:24 | call to s36 [element foo] | provenance |  |
 | semantics.rb:297:13:297:23 | call to source | semantics.rb:297:9:297:24 | call to s36 [element foo] | provenance |  |
+| semantics.rb:298:10:298:10 | x [element foo] | semantics.rb:298:10:298:16 | ...[...] | provenance |  |
+| semantics.rb:298:10:298:10 | x [element foo] | semantics.rb:298:10:298:16 | ...[...] | provenance |  |
 | semantics.rb:299:10:299:10 | x [element foo] | semantics.rb:299:10:299:17 | ...[...] | provenance |  |
 | semantics.rb:299:10:299:10 | x [element foo] | semantics.rb:299:10:299:17 | ...[...] | provenance |  |
 | semantics.rb:301:10:301:10 | x [element foo] | semantics.rb:301:10:301:13 | ...[...] | provenance |  |
@@ -1616,16 +1638,32 @@ nodes
 | semantics.rb:263:14:263:14 | h [element :foo] | semmle.label | h [element :foo] |
 | semantics.rb:263:14:263:14 | h [element] | semmle.label | h [element] |
 | semantics.rb:263:14:263:14 | h [element] | semmle.label | h [element] |
+| semantics.rb:267:5:267:5 | [post] h [element :foo] | semmle.label | [post] h [element :foo] |
+| semantics.rb:267:5:267:5 | [post] h [element :foo] | semmle.label | [post] h [element :foo] |
+| semantics.rb:267:15:267:25 | call to source | semmle.label | call to source |
+| semantics.rb:267:15:267:25 | call to source | semmle.label | call to source |
+| semantics.rb:268:5:268:5 | [post] h [element :foo] | semmle.label | [post] h [element :foo] |
+| semantics.rb:268:5:268:5 | [post] h [element :foo] | semmle.label | [post] h [element :foo] |
 | semantics.rb:268:5:268:5 | [post] h [element foo] | semmle.label | [post] h [element foo] |
 | semantics.rb:268:5:268:5 | [post] h [element foo] | semmle.label | [post] h [element foo] |
+| semantics.rb:268:5:268:5 | h [element :foo] | semmle.label | h [element :foo] |
+| semantics.rb:268:5:268:5 | h [element :foo] | semmle.label | h [element :foo] |
 | semantics.rb:268:16:268:26 | call to source | semmle.label | call to source |
 | semantics.rb:268:16:268:26 | call to source | semmle.label | call to source |
+| semantics.rb:269:5:269:5 | [post] h [element :foo] | semmle.label | [post] h [element :foo] |
+| semantics.rb:269:5:269:5 | [post] h [element :foo] | semmle.label | [post] h [element :foo] |
 | semantics.rb:269:5:269:5 | [post] h [element foo] | semmle.label | [post] h [element foo] |
 | semantics.rb:269:5:269:5 | [post] h [element foo] | semmle.label | [post] h [element foo] |
+| semantics.rb:269:5:269:5 | h [element :foo] | semmle.label | h [element :foo] |
+| semantics.rb:269:5:269:5 | h [element :foo] | semmle.label | h [element :foo] |
 | semantics.rb:269:5:269:5 | h [element foo] | semmle.label | h [element foo] |
 | semantics.rb:269:5:269:5 | h [element foo] | semmle.label | h [element foo] |
+| semantics.rb:270:5:270:5 | [post] h [element :foo] | semmle.label | [post] h [element :foo] |
+| semantics.rb:270:5:270:5 | [post] h [element :foo] | semmle.label | [post] h [element :foo] |
 | semantics.rb:270:5:270:5 | [post] h [element foo] | semmle.label | [post] h [element foo] |
 | semantics.rb:270:5:270:5 | [post] h [element foo] | semmle.label | [post] h [element foo] |
+| semantics.rb:270:5:270:5 | h [element :foo] | semmle.label | h [element :foo] |
+| semantics.rb:270:5:270:5 | h [element :foo] | semmle.label | h [element :foo] |
 | semantics.rb:270:5:270:5 | h [element foo] | semmle.label | h [element foo] |
 | semantics.rb:270:5:270:5 | h [element foo] | semmle.label | h [element foo] |
 | semantics.rb:271:5:271:5 | [post] h [element] | semmle.label | [post] h [element] |
@@ -1634,6 +1672,8 @@ nodes
 | semantics.rb:271:12:271:22 | call to source | semmle.label | call to source |
 | semantics.rb:273:10:273:15 | call to s32 | semmle.label | call to s32 |
 | semantics.rb:273:10:273:15 | call to s32 | semmle.label | call to s32 |
+| semantics.rb:273:14:273:14 | h [element :foo] | semmle.label | h [element :foo] |
+| semantics.rb:273:14:273:14 | h [element :foo] | semmle.label | h [element :foo] |
 | semantics.rb:273:14:273:14 | h [element foo] | semmle.label | h [element foo] |
 | semantics.rb:273:14:273:14 | h [element foo] | semmle.label | h [element foo] |
 | semantics.rb:273:14:273:14 | h [element] | semmle.label | h [element] |
@@ -1708,6 +1748,10 @@ nodes
 | semantics.rb:297:9:297:24 | call to s36 [element foo] | semmle.label | call to s36 [element foo] |
 | semantics.rb:297:13:297:23 | call to source | semmle.label | call to source |
 | semantics.rb:297:13:297:23 | call to source | semmle.label | call to source |
+| semantics.rb:298:10:298:10 | x [element foo] | semmle.label | x [element foo] |
+| semantics.rb:298:10:298:10 | x [element foo] | semmle.label | x [element foo] |
+| semantics.rb:298:10:298:16 | ...[...] | semmle.label | ...[...] |
+| semantics.rb:298:10:298:16 | ...[...] | semmle.label | ...[...] |
 | semantics.rb:299:10:299:10 | x [element foo] | semmle.label | x [element foo] |
 | semantics.rb:299:10:299:10 | x [element foo] | semmle.label | x [element foo] |
 | semantics.rb:299:10:299:17 | ...[...] | semmle.label | ...[...] |

diff --git a/ruby/ql/test/library-tests/dataflow/flow-summaries/semantics.rb b/ruby/ql/test/library-tests/dataflow/flow-summaries/semantics.rb
@@ -270,7 +270,7 @@ def m32(h, i)
     h[1] = source("d")
     h[i] = source("e")
 
-    sink s32(h) # $ hasValueFlow=b hasValueFlow=e
+    sink s32(h) # $ hasValueFlow=b $ hasValueFlow=e $ SPURIOUS: hasValueFlow=a
 end
 
 def m33(h, i)
@@ -295,7 +295,7 @@ def m35(h, i)
 
 def m36(h, i)
     x = s36(source("a"))
-    sink x[:foo]
+    sink x[:foo] # $ SPURIOUS: hasValueFlow=a
     sink x["foo"] # $ hasValueFlow=a
     sink x[:bar]
     sink x[i] # $ hasValueFlow=a