Merge pull request #8396 from alexrford/ruby/charpred-only-field

alexrford · web-flow · commit 808cc9cf3565 · 2022-03-11T15:48:05.000Z
Ruby: resolve `ql/field-only-used-in-charpred` alerts
diff --git a/javascript/ql/lib/semmle/javascript/security/performance/ReDoSUtil.qll b/javascript/ql/lib/semmle/javascript/security/performance/ReDoSUtil.qll
@@ -119,18 +119,18 @@ class EmptyPositiveSubPatttern extends RegExpSubPattern {
  * whose root node is not a disjunction.
  */
 class RegExpRoot extends RegExpTerm {
-  RegExpParent parent;
-
   RegExpRoot() {
-    exists(RegExpAlt alt |
-      alt.isRootTerm() and
-      this = alt.getAChild() and
-      parent = alt.getParent()
+    exists(RegExpParent parent |
+      exists(RegExpAlt alt |
+        alt.isRootTerm() and
+        this = alt.getAChild() and
+        parent = alt.getParent()
+      )
+      or
+      this.isRootTerm() and
+      not this instanceof RegExpAlt and
+      parent = this.getParent()
     )
-    or
-    this.isRootTerm() and
-    not this instanceof RegExpAlt and
-    parent = this.getParent()
   }
 
   /**
@@ -466,13 +466,14 @@ private module CharacterClasses {
    * An implementation of `CharacterClass` for \d, \s, and \w.
    */
   private class PositiveCharacterClassEscape extends CharacterClass {
-    RegExpTerm cc;
     string charClass;
 
     PositiveCharacterClassEscape() {
-      isEscapeClass(cc, charClass) and
-      this = getCanonicalCharClass(cc) and
-      charClass = ["d", "s", "w"]
+      exists(RegExpTerm cc |
+        isEscapeClass(cc, charClass) and
+        this = getCanonicalCharClass(cc) and
+        charClass = ["d", "s", "w"]
+      )
     }
 
     override string getARelevantChar() {
@@ -504,13 +505,14 @@ private module CharacterClasses {
    * An implementation of `CharacterClass` for \D, \S, and \W.
    */
   private class NegativeCharacterClassEscape extends CharacterClass {
-    RegExpTerm cc;
     string charClass;
 
     NegativeCharacterClassEscape() {
-      isEscapeClass(cc, charClass) and
-      this = getCanonicalCharClass(cc) and
-      charClass = ["D", "S", "W"]
+      exists(RegExpTerm cc |
+        isEscapeClass(cc, charClass) and
+        this = getCanonicalCharClass(cc) and
+        charClass = ["D", "S", "W"]
+      )
     }
 
     override string getARelevantChar() {
diff --git a/python/ql/lib/semmle/python/security/performance/ReDoSUtil.qll b/python/ql/lib/semmle/python/security/performance/ReDoSUtil.qll
@@ -119,18 +119,18 @@ class EmptyPositiveSubPatttern extends RegExpSubPattern {
  * whose root node is not a disjunction.
  */
 class RegExpRoot extends RegExpTerm {
-  RegExpParent parent;
-
   RegExpRoot() {
-    exists(RegExpAlt alt |
-      alt.isRootTerm() and
-      this = alt.getAChild() and
-      parent = alt.getParent()
+    exists(RegExpParent parent |
+      exists(RegExpAlt alt |
+        alt.isRootTerm() and
+        this = alt.getAChild() and
+        parent = alt.getParent()
+      )
+      or
+      this.isRootTerm() and
+      not this instanceof RegExpAlt and
+      parent = this.getParent()
     )
-    or
-    this.isRootTerm() and
-    not this instanceof RegExpAlt and
-    parent = this.getParent()
   }
 
   /**
@@ -466,13 +466,14 @@ private module CharacterClasses {
    * An implementation of `CharacterClass` for \d, \s, and \w.
    */
   private class PositiveCharacterClassEscape extends CharacterClass {
-    RegExpTerm cc;
     string charClass;
 
     PositiveCharacterClassEscape() {
-      isEscapeClass(cc, charClass) and
-      this = getCanonicalCharClass(cc) and
-      charClass = ["d", "s", "w"]
+      exists(RegExpTerm cc |
+        isEscapeClass(cc, charClass) and
+        this = getCanonicalCharClass(cc) and
+        charClass = ["d", "s", "w"]
+      )
     }
 
     override string getARelevantChar() {
@@ -504,13 +505,14 @@ private module CharacterClasses {
    * An implementation of `CharacterClass` for \D, \S, and \W.
    */
   private class NegativeCharacterClassEscape extends CharacterClass {
-    RegExpTerm cc;
     string charClass;
 
     NegativeCharacterClassEscape() {
-      isEscapeClass(cc, charClass) and
-      this = getCanonicalCharClass(cc) and
-      charClass = ["D", "S", "W"]
+      exists(RegExpTerm cc |
+        isEscapeClass(cc, charClass) and
+        this = getCanonicalCharClass(cc) and
+        charClass = ["D", "S", "W"]
+      )
     }
 
     override string getARelevantChar() {
diff --git a/ruby/ql/lib/codeql/ruby/ast/internal/Scope.qll b/ruby/ql/lib/codeql/ruby/ast/internal/Scope.qll
@@ -184,9 +184,7 @@ abstract class ScopeImpl extends AstNode, TScopeType {
 }
 
 private class ScopeRealImpl extends ScopeImpl, TScopeReal {
-  private Scope::Range range;
-
-  ScopeRealImpl() { range = toGenerated(this) }
+  ScopeRealImpl() { toGenerated(this) instanceof Scope::Range }
 
   override Variable getAVariableImpl() { result.getDeclaringScope() = this }
 }
diff --git a/ruby/ql/lib/codeql/ruby/ast/internal/Variable.qll b/ruby/ql/lib/codeql/ruby/ast/internal/Variable.qll
@@ -675,10 +675,11 @@ private class ClassVariableAccessSynth extends ClassVariableAccessRealImpl,
 abstract class SelfVariableAccessImpl extends LocalVariableAccessImpl, TSelfVariableAccess { }
 
 private class SelfVariableAccessReal extends SelfVariableAccessImpl, TSelfReal {
-  private Ruby::Self self;
   private SelfVariable var;
 
-  SelfVariableAccessReal() { this = TSelfReal(self) and var = TSelfVariable(scopeOf(self)) }
+  SelfVariableAccessReal() {
+    exists(Ruby::Self self | this = TSelfReal(self) and var = TSelfVariable(scopeOf(self)))
+  }
 
   final override SelfVariable getVariableImpl() { result = var }
 
diff --git a/ruby/ql/lib/codeql/ruby/frameworks/ActionController.qll b/ruby/ql/lib/codeql/ruby/frameworks/ActionController.qll
@@ -127,9 +127,7 @@ abstract class ParamsCall extends MethodCall {
  * ActionController parameters available via the `params` method.
  */
 class ParamsSource extends RemoteFlowSource::Range {
-  ParamsCall call;
-
-  ParamsSource() { this.asExpr().getExpr() = call }
+  ParamsSource() { this.asExpr().getExpr() instanceof ParamsCall }
 
   override string getSourceType() { result = "ActionController::Metal#params" }
 }
@@ -146,9 +144,7 @@ abstract class CookiesCall extends MethodCall {
  * ActionController parameters available via the `cookies` method.
  */
 class CookiesSource extends RemoteFlowSource::Range {
-  CookiesCall call;
-
-  CookiesSource() { this.asExpr().getExpr() = call }
+  CookiesSource() { this.asExpr().getExpr() instanceof CookiesCall }
 
   override string getSourceType() { result = "ActionController::Metal#cookies" }
 }
diff --git a/ruby/ql/lib/codeql/ruby/frameworks/ActionDispatch.qll b/ruby/ql/lib/codeql/ruby/frameworks/ActionDispatch.qll
@@ -573,15 +573,16 @@ module ActionDispatch {
    */
   private class ResourcesRoute extends RouteImpl, TResourcesRoute {
     RouteBlock parent;
-    string resource;
     string action;
     string httpMethod;
     string pathComponent;
 
     ResourcesRoute() {
-      this = TResourcesRoute(parent, method, action) and
-      method.getArgument(0).getConstantValue().isStringOrSymbol(resource) and
-      isDefaultResourceRoute(resource, httpMethod, pathComponent, action)
+      exists(string resource |
+        this = TResourcesRoute(parent, method, action) and
+        method.getArgument(0).getConstantValue().isStringOrSymbol(resource) and
+        isDefaultResourceRoute(resource, httpMethod, pathComponent, action)
+      )
     }
 
     override string getAPrimaryQlClass() { result = "ResourcesRoute" }
@@ -610,15 +611,16 @@ module ActionDispatch {
    */
   private class SingularResourceRoute extends RouteImpl, TResourceRoute {
     RouteBlock parent;
-    string resource;
     string action;
     string httpMethod;
     string pathComponent;
 
     SingularResourceRoute() {
-      this = TResourceRoute(parent, method, action) and
-      method.getArgument(0).getConstantValue().isStringOrSymbol(resource) and
-      isDefaultSingularResourceRoute(resource, httpMethod, pathComponent, action)
+      exists(string resource |
+        this = TResourceRoute(parent, method, action) and
+        method.getArgument(0).getConstantValue().isStringOrSymbol(resource) and
+        isDefaultSingularResourceRoute(resource, httpMethod, pathComponent, action)
+      )
     }
 
     override string getAPrimaryQlClass() { result = "SingularResourceRoute" }
diff --git a/ruby/ql/lib/codeql/ruby/frameworks/ActiveRecord.qll b/ruby/ql/lib/codeql/ruby/frameworks/ActiveRecord.qll
@@ -269,15 +269,15 @@ private Expr getUltimateReceiver(MethodCall call) {
 
 // A call to `find`, `where`, etc. that may return active record model object(s)
 private class ActiveRecordModelFinderCall extends ActiveRecordModelInstantiation, DataFlow::CallNode {
-  private MethodCall call;
   private ActiveRecordModelClass cls;
-  private Expr recv;
 
   ActiveRecordModelFinderCall() {
-    call = this.asExpr().getExpr() and
-    recv = getUltimateReceiver(call) and
-    resolveConstant(recv) = cls.getAQualifiedName() and
-    call.getMethodName() = finderMethodName()
+    exists(MethodCall call, Expr recv |
+      call = this.asExpr().getExpr() and
+      recv = getUltimateReceiver(call) and
+      resolveConstant(recv) = cls.getAQualifiedName() and
+      call.getMethodName() = finderMethodName()
+    )
   }
 
   final override ActiveRecordModelClass getClass() { result = cls }
@@ -347,10 +347,8 @@ private module Persistence {
 
   /** A call to e.g. `User.create(name: "foo")` */
   private class CreateLikeCall extends DataFlow::CallNode, PersistentWriteAccess::Range {
-    private ActiveRecordModelClass modelCls;
-
     CreateLikeCall() {
-      modelCls = this.asExpr().getExpr().(ActiveRecordModelClassMethodCall).getReceiverClass() and
+      exists(this.asExpr().getExpr().(ActiveRecordModelClassMethodCall).getReceiverClass()) and
       this.getMethodName() =
         [
           "create", "create!", "create_or_find_by", "create_or_find_by!", "find_or_create_by",
@@ -367,10 +365,8 @@ private module Persistence {
 
   /** A call to e.g. `User.update(1, name: "foo")` */
   private class UpdateLikeClassMethodCall extends DataFlow::CallNode, PersistentWriteAccess::Range {
-    private ActiveRecordModelClass modelCls;
-
     UpdateLikeClassMethodCall() {
-      modelCls = this.asExpr().getExpr().(ActiveRecordModelClassMethodCall).getReceiverClass() and
+      exists(this.asExpr().getExpr().(ActiveRecordModelClassMethodCall).getReceiverClass()) and
       this.getMethodName() = ["update", "update!", "upsert"]
     }
 
@@ -397,10 +393,9 @@ private module Persistence {
   /** A call to e.g. `User.insert_all([{name: "foo"}, {name: "bar"}])` */
   private class InsertAllLikeCall extends DataFlow::CallNode, PersistentWriteAccess::Range {
     private ExprNodes::ArrayLiteralCfgNode arr;
-    private ActiveRecordModelClass modelCls;
 
     InsertAllLikeCall() {
-      modelCls = this.asExpr().getExpr().(ActiveRecordModelClassMethodCall).getReceiverClass() and
+      exists(this.asExpr().getExpr().(ActiveRecordModelClassMethodCall).getReceiverClass()) and
       this.getMethodName() = ["insert_all", "insert_all!", "upsert_all"] and
       arr = this.getArgument(0).asExpr()
     }
diff --git a/ruby/ql/lib/codeql/ruby/frameworks/core/Kernel.qll b/ruby/ql/lib/codeql/ruby/frameworks/core/Kernel.qll
@@ -18,20 +18,15 @@ module Kernel {
    * providing a specific receiver as in `Kernel.exit`.
    */
   class KernelMethodCall extends DataFlow::CallNode {
-    private MethodCall methodCall;
-
     KernelMethodCall() {
-      methodCall = this.asExpr().getExpr() and
+      this = API::getTopLevelMember("Kernel").getAMethodCall(_)
+      or
+      this.asExpr().getExpr() instanceof UnknownMethodCall and
       (
-        this = API::getTopLevelMember("Kernel").getAMethodCall(_)
+        this.getReceiver().asExpr().getExpr() instanceof SelfVariableAccess and
+        isPrivateKernelMethod(this.getMethodName())
         or
-        methodCall instanceof UnknownMethodCall and
-        (
-          this.getReceiver().asExpr().getExpr() instanceof SelfVariableAccess and
-          isPrivateKernelMethod(methodCall.getMethodName())
-          or
-          isPublicKernelMethod(methodCall.getMethodName())
-        )
+        isPublicKernelMethod(this.getMethodName())
       )
     }
   }
diff --git a/ruby/ql/lib/codeql/ruby/security/performance/ReDoSUtil.qll b/ruby/ql/lib/codeql/ruby/security/performance/ReDoSUtil.qll
@@ -119,18 +119,18 @@ class EmptyPositiveSubPatttern extends RegExpSubPattern {
  * whose root node is not a disjunction.
  */
 class RegExpRoot extends RegExpTerm {
-  RegExpParent parent;
-
   RegExpRoot() {
-    exists(RegExpAlt alt |
-      alt.isRootTerm() and
-      this = alt.getAChild() and
-      parent = alt.getParent()
+    exists(RegExpParent parent |
+      exists(RegExpAlt alt |
+        alt.isRootTerm() and
+        this = alt.getAChild() and
+        parent = alt.getParent()
+      )
+      or
+      this.isRootTerm() and
+      not this instanceof RegExpAlt and
+      parent = this.getParent()
     )
-    or
-    this.isRootTerm() and
-    not this instanceof RegExpAlt and
-    parent = this.getParent()
   }
 
   /**
@@ -466,13 +466,14 @@ private module CharacterClasses {
    * An implementation of `CharacterClass` for \d, \s, and \w.
    */
   private class PositiveCharacterClassEscape extends CharacterClass {
-    RegExpTerm cc;
     string charClass;
 
     PositiveCharacterClassEscape() {
-      isEscapeClass(cc, charClass) and
-      this = getCanonicalCharClass(cc) and
-      charClass = ["d", "s", "w"]
+      exists(RegExpTerm cc |
+        isEscapeClass(cc, charClass) and
+        this = getCanonicalCharClass(cc) and
+        charClass = ["d", "s", "w"]
+      )
     }
 
     override string getARelevantChar() {
@@ -504,13 +505,14 @@ private module CharacterClasses {
    * An implementation of `CharacterClass` for \D, \S, and \W.
    */
   private class NegativeCharacterClassEscape extends CharacterClass {
-    RegExpTerm cc;
     string charClass;
 
     NegativeCharacterClassEscape() {
-      isEscapeClass(cc, charClass) and
-      this = getCanonicalCharClass(cc) and
-      charClass = ["D", "S", "W"]
+      exists(RegExpTerm cc |
+        isEscapeClass(cc, charClass) and
+        this = getCanonicalCharClass(cc) and
+        charClass = ["D", "S", "W"]
+      )
     }
 
     override string getARelevantChar() {
diff --git a/ruby/ql/lib/codeql/ruby/security/performance/RegExpTreeView.qll b/ruby/ql/lib/codeql/ruby/security/performance/RegExpTreeView.qll
@@ -241,12 +241,11 @@ newtype TRegExpParent =
 
 class RegExpQuantifier extends RegExpTerm, TRegExpQuantifier {
   int part_end;
-  boolean maybe_empty;
   boolean may_repeat_forever;
 
   RegExpQuantifier() {
     this = TRegExpQuantifier(re, start, end) and
-    re.qualifiedPart(start, part_end, end, maybe_empty, may_repeat_forever)
+    re.qualifiedPart(start, part_end, end, _, may_repeat_forever)
   }
 
   override RegExpTerm getChild(int i) {

Original file line number	Diff line number	Diff line change
`@@ -184,9 +184,7 @@ abstract class ScopeImpl extends AstNode, TScopeType {`
`184`	`184`	`}`
`185`	`185`
`186`	`186`	`private class ScopeRealImpl extends ScopeImpl, TScopeReal {`
`187`		`- private Scope::Range range;`
`188`		`-`
`189`		`- ScopeRealImpl() { range = toGenerated(this) }`
	`187`	`+ ScopeRealImpl() { toGenerated(this) instanceof Scope::Range }`
`190`	`188`
`191`	`189`	`override Variable getAVariableImpl() { result.getDeclaringScope() = this }`
`192`	`190`	`}`