Merge pull request #10513 from erik-krogh/more-alert-style

QL: improve the `ql/alert-message-style-violation` query.

ql/ql/src/codeql_ql/ast/Ast.qll

@@ -186,7 +186,9 @@ class QueryDoc extends QLDoc {
   override string getAPrimaryQlClass() { result = "QueryDoc" }
 
   /** Gets the @kind for the query */
-  string getQueryKind() { result = this.getContents().regexpCapture("(?s).*@kind (\\w+)\\s.*", 1) }
+  string getQueryKind() {
+    result = this.getContents().regexpCapture("(?s).*@kind ([\\w-]+)\\s.*", 1)
+  }
 
   /** Gets the id part (without language) of the @id */
   string getQueryId() {
@@ -242,6 +244,12 @@ class Select extends TSelect, AstNode {
    */
   Expr getExpr(int i) { toQL(result) = sel.getChild(_).(QL::AsExprs).getChild(i) }
 
+  Expr getMessage() {
+    if this.getQueryDoc().getQueryKind() = "path-problem"
+    then result = this.getExpr(3)
+    else result = this.getExpr(1)
+  }
+
   // TODO: This gets the `i`th order-by, but some expressions might not have an order-by.
   Expr getOrderBy(int i) { toQL(result) = sel.getChild(_).(QL::OrderBys).getChild(i).getChild(0) }
 
@@ -1403,6 +1411,14 @@ class ComparisonFormula extends TComparisonFormula, Formula {
     or
     pred = directMember("getRightOperand") and result = this.getRightOperand()
   }
+
+  /** Holds if this comparison has the operands `a` and `b` (in any order). */
+  pragma[noinline]
+  predicate hasOperands(Expr a, Expr b) {
+    this.getLeftOperand() = a and this.getRightOperand() = b
+    or
+    this.getLeftOperand() = b and this.getRightOperand() = a
+  }
 }
 
 /** A quantifier formula, such as `exists` or `forall`. */

ql/ql/src/queries/style/AlertMessage.ql

@@ -10,17 +10,21 @@
 
 import ql
 
+AstNode getASubExpression(Select sel) {
+  result = sel.getExpr(_)
+  or
+  result = getASubExpression(sel).getAChild()
+}
+
 /** Gets the `index`th part of the select statement. */
 private AstNode getSelectPart(Select sel, int index) {
   result =
     rank[index](AstNode n, Location loc |
       (
-        n.getParent*() = sel.getExpr(_) and loc = n.getLocation()
+        n = getASubExpression(sel) and loc = n.getLocation()
         or
         // the strings are behind a predicate call.
-        exists(Call c, Predicate target |
-          c.getParent*() = sel.getExpr(_) and loc = c.getLocation()
-        |
+        exists(Call c, Predicate target | c = getASubExpression(sel) and loc = c.getLocation() |
           c.getTarget() = target and
           (
             target.getBody().(ComparisonFormula).getAnOperand() = n
@@ -30,6 +34,14 @@ private AstNode getSelectPart(Select sel, int index) {
             )
           )
         )
+        or
+        // the string is a variable that is assigned in the `where` clause.
+        exists(VarAccess v, ComparisonFormula comp, String str |
+          v = getASubExpression(sel) and
+          loc = v.getLocation() and
+          comp.hasOperands(v.getDeclaration().getAnAccess(), str) and
+          n = str
+        )
       )
     |
       n
@@ -52,7 +64,7 @@ private AstNode getSelectPart(Select sel, int index) {
 String shouldHaveFullStop(Select sel) {
   result =
     max(AstNode str, int i |
-      str.getParent+() = sel.getExpr(1) and str = getSelectPart(sel, i)
+      str.getParent*() = sel.getMessage() and str = getSelectPart(sel, i)
     |
       str order by i
     ) and
@@ -73,7 +85,7 @@ String shouldHaveFullStop(Select sel) {
 String shouldStartCapital(Select sel) {
   result =
     min(AstNode str, int i |
-      str.getParent+() = sel.getExpr(1) and str = getSelectPart(sel, i)
+      str.getParent*() = sel.getMessage() and str = getSelectPart(sel, i)
     |
       str order by i
     ) and
@@ -164,6 +176,14 @@ String wrongFlowsPhrase(Select sel, string kind) {
   )
 }
 
+/**
+ * Gets a string element that contains double whitespace.
+ */
+String doubleWhitespace(Select sel) {
+  result = getSelectPart(sel, _) and
+  result.getValue().regexpMatch(".*\\s\\s.*")
+}
+
 from AstNode node, string msg
 where
   not node.getLocation().getFile().getAbsolutePath().matches("%/test/%") and
@@ -194,5 +214,8 @@ where
     or
     node = wrongFlowsPhrase(_, "taint") and
     msg = "Use \"depends on\" instead of \"flows to\" in taint tracking queries."
+    or
+    node = doubleWhitespace(_) and
+    msg = "Avoid using double whitespace in alert messages."
   )
 select node, msg