Merge pull request #10405 from erik-krogh/styleGuide

update the style guide on alert-messages

Author: erik-krogh

docs/query-metadata-style-guide.md

@@ -179,7 +179,15 @@ The select clause of each alert query defines the alert message that is displaye
 * The message should factually describe the problem that is being highlighted–it should not contain recommendations about how to fix the problem or value judgements.
 * Program element references should be in 'single quotes' to distinguish them from ordinary words. Quotes are not needed around substitutions (`$@`).
 * Avoid constant alert message strings and include some context, if possible. For example, `The class 'Foo' is duplicated as 'Bar'.` is preferable to `This class is duplicated here.`
+* If a reference to the current location can't be avoided use "this location" instead of "here". For example, `Bad thing at this location.` is preferable to `Bad thing here.`. This avoids the "click here" anti-pattern.
+* For path queries, if possible, try to follow the template: `This path depends on a [user-provided value].`, or alternatively (if the first option doesn't work) `[User-provided value] flows to this location and is used in a path.`. 
+* Taint tracking queries generally have a sink that "depends on" the source, and dataflow queries generally have a source that "flows to" the sink.
+
+### Links in alert messages
+
 * Where you reference another program element, link to it if possible using a substitution (`$@`). Links should be used inline in the sentence, rather than as parenthesised lists or appositions.
+* Avoid using link texts that don't describe what they link to. For example, rewrite `This sensitive data is written to a logfile unescaped [here]` to `This sensitive data is [written to a logfile unescaped]`. 
+* Make link text as concise and precise as possible. For example, avoid starting a link text with an indefinite article (a, an). `Path construction depends on a [user-provided value]` is preferable to `Path construction depends on [a user-provided value]`. (Where the square brackets indicate a link.) See [the W3C guide on link texts](https://www.w3.org/WAI/WCAG22/Understanding/link-purpose-in-context.html) for further information. 
 * When a message contains multiple links, construct a sentence that has the most variable link (that is, the link with most targets) last. For further information, see [Defining the results of a query](https://codeql.github.com/docs/writing-codeql-queries/defining-the-results-of-a-query/).
 
 For examples of select clauses and alert messages, see the query source files at the following pages:

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

@@ -0,0 +1,198 @@
+/**
+ * @name Alert message style violation
+ * @description An alert message that doesn't follow the style guide is harder for end users to digest.
+ *              See the style guide here: https://github.com/github/codeql/blob/main/docs/query-metadata-style-guide.md#alert-messages
+ * @kind problem
+ * @problem.severity warning
+ * @id ql/alert-message-style-violation
+ * @precision high
+ */
+
+import ql
+
+/** 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()
+        or
+        // the strings are behind a predicate call.
+        exists(Call c, Predicate target |
+          c.getParent*() = sel.getExpr(_) and loc = c.getLocation()
+        |
+          c.getTarget() = target and
+          (
+            target.getBody().(ComparisonFormula).getAnOperand() = n
+            or
+            exists(ClassPredicate sub | sub.overrides(target) |
+              sub.getBody().(ComparisonFormula).getAnOperand() = n
+            )
+          )
+        )
+      )
+    |
+      n
+      order by
+        loc.getStartLine(), loc.getStartColumn(), loc.getEndLine(), loc.getEndColumn(),
+        loc.getFile().getRelativePath()
+    )
+}
+
+/**
+ * Gets a string element that is the last part of the message, that doesn't end with a period.
+ *
+ * For example:
+ * ```CodeQL
+ * select foo(), "This is a description" // <- bad
+ *
+ * select foo(), "This is a description." // <- good
+ * ```
+ */
+String shouldHaveFullStop(Select sel) {
+  result =
+    max(AstNode str, int i |
+      str.getParent+() = sel.getExpr(1) and str = getSelectPart(sel, i)
+    |
+      str order by i
+    ) and
+  not result.getValue().matches("%.") and
+  not result.getValue().matches("%?")
+}
+
+/**
+ * Gets a string element that is the first part of the message, that starts with a lower case letter.
+ *
+ * For example:
+ * ```CodeQL
+ * select foo(), "this is a description." // <- bad
+ *
+ * select foo(), "This is a description." // <- good
+ * ```
+ */
+String shouldStartCapital(Select sel) {
+  result =
+    min(AstNode str, int i |
+      str.getParent+() = sel.getExpr(1) and str = getSelectPart(sel, i)
+    |
+      str order by i
+    ) and
+  result.getValue().regexpMatch("^[a-z].*")
+}
+
+/**
+ * Gets a string element that is used in a message that contains "here" or "this location".
+ *
+ * For example:
+ * ```CodeQL
+ * select foo(), "XSS happens here from using a unsafe value." // <- bad
+ *
+ * select foo(), "XSS from using a unsafe value." // <- good
+ * ```
+ */
+String avoidHere(string part) {
+  part = ["here", "this location"] and
+  (
+    result.getValue().regexpMatch(".*\\b" + part + "\\b.*") and
+    result = getSelectPart(_, _)
+  )
+}
+
+/**
+ * Avoid using an indefinite article ("a" or "an") in a link text.
+ *
+ * For example:
+ * ```CodeQL
+ * select foo(), "XSS from $@", val, "an unsafe value." // <- bad
+ *
+ * select foo(), "XSS from a $@", val, "unsafe value." // <- good
+ * ```
+ *
+ * See https://www.w3.org/WAI/WCAG22/Understanding/link-purpose-in-context.html for the W3C guideline on link text. a
+ */
+String avoidArticleInLinkText(Select sel) {
+  result = sel.getExpr((any(int i | i > 1))) and
+  result = getSelectPart(sel, _) and
+  result.getValue().regexpMatch("a|an .*")
+}
+
+/**
+ * Don't quote substitutions in a message.
+ *
+ * For example:
+ * ```CodeQL
+ * select foo(), "XSS from '$@'", val, "an unsafe value." // <- bad
+ *
+ * select foo(), "XSS from $@", val, "an unsafe value." // <- good
+ * ```
+ */
+String dontQuoteSubstitutions(Select sel) {
+  result = getSelectPart(sel, _) and
+  result.getValue().matches(["%'$@'%", "%\"$@\"%"])
+}
+
+/**
+ * Gets the kind of the path-query represented by `sel`.
+ * Either "data" for a dataflow query or "taint" for a taint-tracking query.
+ */
+private string getQueryKind(Select sel) {
+  exists(TypeExpr sup |
+    sup = sel.getVarDecl(_).getType().(ClassType).getDeclaration().getASuperType() and
+    sup.getResolvedType().(ClassType).getName() = "Configuration"
+  |
+    result = "data" and
+    sup.getModule().getName() = "DataFlow"
+    or
+    result = "taint" and
+    sup.getModule().getName() = "TaintTracking"
+  )
+}
+
+/**
+ * Gets a string element from a message that uses the wrong phrase for a path query.
+ * A dataflow query should use "flows to" and a taint-tracking query should use "depends on".
+ */
+String wrongFlowsPhrase(Select sel, string kind) {
+  result = getSelectPart(sel, _) and
+  kind = getQueryKind(sel) and
+  (
+    kind = "data" and
+    result.getValue().matches(["% depends %", "% depend %"])
+    or
+    kind = "taint" and
+    result.getValue().matches(["% flows to %", "% flow to %"])
+  )
+}
+
+from AstNode node, string msg
+where
+  not node.getLocation().getFile().getAbsolutePath().matches("%/test/%") and
+  (
+    node = shouldHaveFullStop(_) and
+    msg = "Alert message should end with a full stop."
+    or
+    node = shouldStartCapital(_) and
+    msg = "Alert message should start with a capital letter."
+    or
+    exists(string part | node = avoidHere(part) |
+      part = "here" and
+      msg =
+        "Try to use a descriptive phrase instead of \"here\". Use \"this location\" if you can't get around mentioning the current location."
+      or
+      part = "this location" and
+      msg = "Try to more descriptive phrase instead of \"this location\" if possible."
+    )
+    or
+    node = avoidArticleInLinkText(_) and
+    msg = "Avoid starting a link text with an indefinite article."
+    or
+    node = dontQuoteSubstitutions(_) and
+    msg = "Don't quote substitutions in alert messages."
+    or
+    node = wrongFlowsPhrase(_, "data") and
+    msg = "Use \"flows to\" instead of \"depends on\" in data flow queries."
+    or
+    node = wrongFlowsPhrase(_, "taint") and
+    msg = "Use \"depends on\" instead of \"flows to\" in taint tracking queries."
+  )
+select node, msg