Merge pull request #138 from github/erik-krogh/bump-this

bump the severity of `ql/implicit-this`

Author: erik-krogh

ql/src/queries/style/ImplicitThis.ql

@@ -2,8 +2,8 @@
  * @name Using implicit `this`
  * @description Writing member predicate calls with an implicit `this` can be confusing
  * @kind problem
- * @problem.severity recommendation
- * @precision very-high
+ * @problem.severity warning
+ * @precision high
  * @id ql/implicit-this
  * @tags maintainability
  */

repo-tests/codeql-go.txt

@@ -1 +1 @@
-4cae4b23fc1b2b1760e259b660996e9bb5573279
+894102defd0777931a0e261ad66e631e63ec0ad8

repo-tests/codeql-go/ql/lib/semmle/go/Decls.qll

@@ -137,7 +137,8 @@ class FuncDef extends @funcdef, StmtParent, ExprParent {
    */
   DataFlow::CallNode getACall() { result.getACallee() = this }
 
-  predicate isVariadic() { getType().isVariadic() }
+  /** Holds if this function is variadic. */
+  predicate isVariadic() { this.getType().isVariadic() }
 
   override string getAPrimaryQlClass() { result = "FuncDef" }
 }

repo-tests/codeql-go/ql/lib/semmle/go/Scopes.qll

@@ -377,10 +377,11 @@ class Function extends ValueEntity, @functionobject {
   /** Gets the declaration of this function, if any. */
   FuncDecl getFuncDecl() { none() }
 
+  /** Holds if this function is variadic. */
   predicate isVariadic() {
     this.(BuiltinFunction).getName() = ["append", "make", "print", "println"]
     or
-    this.(DeclaredFunction).getFuncDecl().isVariadic()
+    this.(DeclaredFunction).getType().(SignatureType).isVariadic()
   }
 
   /** Holds if this function has no observable side effects. */

repo-tests/codeql-go/ql/lib/semmle/go/dataflow/barrierguardutil/RegexpCheck.qll

@@ -8,18 +8,26 @@ import go
  * A call to a regexp match function, considered as a barrier guard for sanitizing untrusted URLs.
  *
  * This is overapproximate: we do not attempt to reason about the correctness of the regexp.
+ *
+ * Use this if you want to define a derived `DataFlow::BarrierGuard` without
+ * make the type recursive. Otherwise use `RegexpCheck`.
  */
-class RegexpCheck extends DataFlow::BarrierGuard {
-  RegexpMatchFunction matchfn;
-  DataFlow::CallNode call;
-
-  RegexpCheck() {
+predicate regexpFunctionChecksExpr(DataFlow::Node resultNode, Expr checked, boolean branch) {
+  exists(RegexpMatchFunction matchfn, DataFlow::CallNode call |
     matchfn.getACall() = call and
-    this = matchfn.getResult().getNode(call).getASuccessor*()
-  }
-
-  override predicate checks(Expr e, boolean branch) {
-    e = matchfn.getValue().getNode(call).asExpr() and
+    resultNode = matchfn.getResult().getNode(call).getASuccessor*() and
+    checked = matchfn.getValue().getNode(call).asExpr() and
     (branch = false or branch = true)
-  }
+  )
+}
+
+/**
+ * A call to a regexp match function, considered as a barrier guard for sanitizing untrusted URLs.
+ *
+ * This is overapproximate: we do not attempt to reason about the correctness of the regexp.
+ */
+class RegexpCheck extends DataFlow::BarrierGuard {
+  RegexpCheck() { regexpFunctionChecksExpr(this, _, _) }
+
+  override predicate checks(Expr e, boolean branch) { regexpFunctionChecksExpr(this, e, branch) }
 }

repo-tests/codeql-go/ql/lib/semmle/go/frameworks/Glog.qll

@@ -11,10 +11,17 @@ import go
  */
 module Glog {
   private class GlogCall extends LoggerCall::Range, DataFlow::CallNode {
+    int firstPrintedArg;
+
     GlogCall() {
-      exists(string pkg, Function f, string fn |
+      exists(string pkg, Function f, string fn, string level |
         pkg = package(["github.com/golang/glog", "gopkg.in/glog", "k8s.io/klog"], "") and
-        fn.regexpMatch("(Error|Exit|Fatal|Info|Warning)(|f|ln)") and
+        level = ["Error", "Exit", "Fatal", "Info", "Warning"] and
+        (
+          fn = level + ["", "f", "ln"] and firstPrintedArg = 0
+          or
+          fn = level + "Depth" and firstPrintedArg = 1
+        ) and
         this = f.getACall()
       |
         f.hasQualifiedName(pkg, fn)
@@ -23,6 +30,8 @@ module Glog {
       )
     }
 
-    override DataFlow::Node getAMessageComponent() { result = this.getAnArgument() }
+    override DataFlow::Node getAMessageComponent() {
+      result = this.getArgument(any(int i | i >= firstPrintedArg))
+    }
   }
 }

repo-tests/codeql-go/ql/lib/semmle/go/frameworks/Logrus.qll

@@ -11,7 +11,10 @@ module Logrus {
 
   bindingset[result]
   private string getALogResultName() {
-    result.matches(["Debug%", "Error%", "Fatal%", "Info%", "Panic%", "Print%", "Trace%", "Warn%"])
+    result
+        .matches([
+            "Debug%", "Error%", "Fatal%", "Info%", "Log%", "Panic%", "Print%", "Trace%", "Warn%"
+          ])
   }
 
   bindingset[result]
@@ -23,7 +26,7 @@ module Logrus {
     LogCall() {
       exists(string name | name = getALogResultName() or name = getAnEntryUpdatingMethodName() |
         this.getTarget().hasQualifiedName(packagePath(), name) or
-        this.getTarget().(Method).hasQualifiedName(packagePath(), "Entry", name)
+        this.getTarget().(Method).hasQualifiedName(packagePath(), ["Entry", "Logger"], name)
       )
     }
 

repo-tests/codeql-go/ql/lib/semmle/go/frameworks/SQL.qll

@@ -88,10 +88,11 @@ module SQL {
             // first argument to `squirrel.Expr`
             fn.hasQualifiedName(sq, "Expr")
             or
-            // first argument to the `Prefix` or `Suffix` method of one of the `*Builder` classes
+            // first argument to the `Prefix`, `Suffix` or `Where` method of one of the `*Builder` classes
             exists(string builder | builder.matches("%Builder") |
               fn.(Method).hasQualifiedName(sq, builder, "Prefix") or
-              fn.(Method).hasQualifiedName(sq, builder, "Suffix")
+              fn.(Method).hasQualifiedName(sq, builder, "Suffix") or
+              fn.(Method).hasQualifiedName(sq, builder, "Where")
             )
           ) and
           this = fn.getACall().getArgument(0) and

repo-tests/codeql-go/ql/lib/semmle/go/frameworks/stdlib/Log.qll

@@ -8,13 +8,7 @@ import go
 module Log {
   private class LogCall extends LoggerCall::Range, DataFlow::CallNode {
     LogCall() {
-      exists(string fn |
-        fn.matches("Fatal%")
-        or
-        fn.matches("Panic%")
-        or
-        fn.matches("Print%")
-      |
+      exists(string fn | fn.matches(["Fatal%", "Panic%", "Print%"]) |
         this.getTarget().hasQualifiedName("log", fn)
         or
         this.getTarget().(Method).hasQualifiedName("log", "Logger", fn)

repo-tests/codeql-go/ql/lib/semmle/go/security/IncorrectIntegerConversionLib.qll

@@ -124,7 +124,7 @@ class ConversionWithoutBoundsCheckConfig extends TaintTracking::Configuration {
     )
   }
 
-  override predicate isSink(DataFlow::Node sink) { isSink(sink, sinkBitSize) }
+  override predicate isSink(DataFlow::Node sink) { this.isSink(sink, sinkBitSize) }
 
   override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) {
     // To catch flows that only happen on 32-bit architectures we
@@ -136,7 +136,7 @@ class ConversionWithoutBoundsCheckConfig extends TaintTracking::Configuration {
 
   override predicate isSanitizerOut(DataFlow::Node node) {
     exists(int bitSize | isIncorrectIntegerConversion(sourceBitSize, bitSize) |
-      isSink(node, bitSize)
+      this.isSink(node, bitSize)
     )
   }
 }