From 754bfdd13686d50e2df08c495e52aa653236598c Mon Sep 17 00:00:00 2001
From: Arthur Baars <aibaars@github.com>
Date: Tue, 13 Apr 2021 11:30:41 +0200
Subject: [PATCH] Ignore include/prepend statements in blocks

Include and prepend statements are rarely used in block in normal code and when
used in normal code they tend to be in blocks that are passed to methods like
`module_eval` which is a builtin method that evaluates a block in the context
of some other module (typically created with Module.new). We currently don't attempt
to track such "dynamically" constructed modules, and ignoring such modules
 and the `module_eval` calls on them seems fine for now.

Another, much more frequent use of include/prepend statements in blocks is in Rspec.describe and
Rspec.context method calls in tests. Rspec also evaluates those blocks in the context of some
special Rspec class. Precisely tracking such calls during the initial construction of the module/class
hierarchy would be really hard and there would be little benefit because the interesting modules and classes of
an application are not defined in test files.
---
 ql/src/codeql_ruby/ast/internal/Module.qll    | 22 ++++++++++++++++++-
 .../ast/modules/modules.expected              |  2 +-
 2 files changed, 22 insertions(+), 2 deletions(-)

diff --git a/ql/src/codeql_ruby/ast/internal/Module.qll b/ql/src/codeql_ruby/ast/internal/Module.qll
index 8a0f98182af..86d10b23c61 100644
--- a/ql/src/codeql_ruby/ast/internal/Module.qll
+++ b/ql/src/codeql_ruby/ast/internal/Module.qll
@@ -1,4 +1,5 @@
 private import codeql.Locations
+private import codeql_ruby.AST
 private import codeql_ruby.ast.Call
 private import codeql_ruby.ast.Constant
 private import codeql_ruby.ast.Expr
@@ -169,7 +170,7 @@ private class IncludeOrPrependCall extends MethodCall {
   string getTarget() {
     result = resolveScopeExpr(this.getReceiver(), _)
     or
-    result = qualifiedModuleName(this.getEnclosingModule()) and
+    result = qualifiedModuleName(enclosingModule(this)) and
     (
       this.getReceiver() instanceof Self
       or
@@ -178,6 +179,25 @@ private class IncludeOrPrependCall extends MethodCall {
   }
 }
 
+/**
+ * A variant of AstNode::getEnclosingModule that excludes
+ * results that are enclosed in a block. This is a bit wrong because
+ * it could lead to false negatives. However, `include` statements in
+ * blocks are very rare in normal code. The majority of cases are in calls
+ * to methods like `module_eval` and `Rspec.describe` / `Rspec.context`. These
+ * methods evaluate the block in the context of some other module/class instead of
+ * the enclosing one.
+ */
+private ModuleBase enclosingModule(AstNode node) {
+  exists(AstNode parent | parent = node.getParent() |
+    result = parent
+    or
+    not parent instanceof ModuleBase and
+    not parent instanceof Block and
+    result = enclosingModule(parent)
+  )
+}
+
 private string prepends(string qname) {
   exists(IncludeOrPrependCall m |
     m.getMethodName() = "prepend" and
diff --git a/ql/test/library-tests/ast/modules/modules.expected b/ql/test/library-tests/ast/modules/modules.expected
index 70145c6da6a..b395f09e63d 100644
--- a/ql/test/library-tests/ast/modules/modules.expected
+++ b/ql/test/library-tests/ast/modules/modules.expected
@@ -125,7 +125,7 @@ moduleTypes
 | modules.rb:83:1:86:3 | Other | modules.rb:83:1:86:3 | Other |
 | modules.rb:84:3:85:5 | Foo1 | modules.rb:84:3:85:5 | Other::Foo1 |
 | modules.rb:88:1:93:3 | IncludeTest | modules.rb:88:1:93:3 | IncludeTest |
-| modules.rb:91:3:92:5 | Y | modules.rb:91:3:92:5 | Other::Foo1::Y |
+| modules.rb:91:3:92:5 | Y | modules.rb:91:3:92:5 | Test::Foo1::Y |
 | modules.rb:95:1:99:3 | IncludeTest2 | modules.rb:95:1:99:3 | IncludeTest2 |
 | modules.rb:97:3:98:5 | Z | modules.rb:97:3:98:5 | Test::Foo1::Z |
 | modules.rb:101:1:105:3 | PrependTest | modules.rb:101:1:105:3 | PrependTest |