From b27e63ba8334aaef1f62d539653072bfdd5a7726 Mon Sep 17 00:00:00 2001
From: Sauyon Lee <sauyon@github.com>
Date: Thu, 5 Mar 2020 02:42:13 -0800
Subject: [PATCH] Address review comments

Co-authored-by: Max Schaefer <max-schaefer@github.com>
---
 ql/src/semmle/go/GoModExpr.qll                | 104 +++++++++++-------
 .../semmle/go/dependencies/Dependencies.qll   |  15 ++-
 ql/src/semmle/go/dependencies/SemVer.qll      |  10 +-
 .../semmle/go/GoModExpr/ExcludeLines.expected |   6 +-
 .../semmle/go/GoModExpr/ExcludeLines.ql       |   5 +-
 .../semmle/go/GoModExpr/ReplaceLines.expected |   1 +
 .../semmle/go/GoModExpr/ReplaceLines.ql       |  19 +++-
 .../semmle/go/GoModExpr/RequireLines.expected |  10 +-
 .../semmle/go/GoModExpr/RequireLines.ql       |   5 +-
 .../semmle/go/dependencies/SemVer.ql          |   8 +-
 .../semmle/go/dependencies/ShaVer.ql          |   2 +-
 .../go/dependencies/fabric-snaps/README.md    |   3 +
 .../dependencies/hrm-profile-tool/README.md   |   3 +
 .../semmle/go/dependencies/sweb/README.md     |   3 +
 14 files changed, 119 insertions(+), 75 deletions(-)
 create mode 100644 ql/test/library-tests/semmle/go/dependencies/fabric-snaps/README.md
 create mode 100644 ql/test/library-tests/semmle/go/dependencies/hrm-profile-tool/README.md
 create mode 100644 ql/test/library-tests/semmle/go/dependencies/sweb/README.md

diff --git a/ql/src/semmle/go/GoModExpr.qll b/ql/src/semmle/go/GoModExpr.qll
index fced69266f7..959fd1a62fa 100644
--- a/ql/src/semmle/go/GoModExpr.qll
+++ b/ql/src/semmle/go/GoModExpr.qll
@@ -5,7 +5,7 @@
 import go
 
 /**
- * A go.mod expression.
+ * An expression in a go.mod file, which is used to declare dependencies.
  */
 class GoModExpr extends @modexpr, GoModExprParent {
   /**
@@ -22,6 +22,11 @@ class GoModExpr extends @modexpr, GoModExprParent {
    */
   DocComment getComments() { result.getDocumentedElement() = this }
 
+  /** Gets path of the module of this go.mod expression. */
+  string getModulePath() {
+    exists(GoModModuleLine mod | this.getFile() = mod.getFile() | result = mod.getPath())
+  }
+
   override string toString() { result = "go.mod expression" }
 }
 
@@ -43,27 +48,13 @@ class GoModLine extends @modline, GoModExpr {
 }
 
 /**
- * A line that contains the module information
- */
-class GoModModuleLine extends GoModLine {
-  GoModModuleLine() {
-    this.getParent().(GoModLineBlock).getToken(0) = "module"
-    or
-    not this.getParent() instanceof GoModLineBlock and
-    this.getToken(0) = "module"
-  }
-
-  string getPath() {
-    if this.getParent() instanceof GoModLineBlock
-    then result = this.getToken(1)
-    else result = this.getToken(0)
-  }
-
-  override string toString() { result = "go.mod module line" }
-}
-
-/**
- * A factored block of lines, for example `require ( "path" )`.
+ * A factored block of lines, for example:
+ * ```
+ * require (
+ *   "github.com/github/codeql-go" v1.2.3
+ *   "golang.org/x/tools" v3.2.1
+ * )
+ * ```
  */
 class GoModLineBlock extends @modlineblock, GoModExpr {
   /**
@@ -75,17 +66,9 @@ class GoModLineBlock extends @modlineblock, GoModExpr {
 }
 
 /**
- * A line that declares the Go version to be used, for example `go 1.14`.
+ * Gets the `i`th token of `line`, including the token in the line block declaration, if it there is
+ * one.
  */
-class GoModGoLine extends GoModLine {
-  GoModGoLine() { this.getToken(0) = "go" }
-
-  /** Gets the Go version declared. */
-  string getVer() { result = this.getToken(1) }
-
-  override string toString() { result = "go.mod go line" }
-}
-
 private string getOffsetToken(GoModLine line, int i) {
   if line.getParent() instanceof GoModLineBlock
   then result = line.getToken(i - 1)
@@ -93,7 +76,38 @@ private string getOffsetToken(GoModLine line, int i) {
 }
 
 /**
- * A line that declares a requirement, for example `require "path"`.
+ * A line that contains the module information
+ */
+class GoModModuleLine extends GoModLine {
+  GoModModuleLine() {
+    this.getParent().(GoModLineBlock).getToken(0) = "module"
+    or
+    not this.getParent() instanceof GoModLineBlock and
+    this.getToken(0) = "module"
+  }
+
+  /**
+   * Get the path of the module being declared.
+   */
+  string getPath() { result = getOffsetToken(this, 1) }
+
+  override string toString() { result = "go.mod module line" }
+}
+
+/**
+ * A line that declares the Go version to be used, for example `go 1.14`.
+ */
+class GoModGoLine extends GoModLine {
+  GoModGoLine() { this.getToken(0) = "go" }
+
+  /** Gets the Go version declared. */
+  string getVersion() { result = this.getToken(1) }
+
+  override string toString() { result = "go.mod go line" }
+}
+
+/**
+ * A line that declares a requirement, for example `require "github.com/github/codeql-go" v1.2.3`.
  */
 class GoModRequireLine extends GoModLine {
   GoModRequireLine() {
@@ -107,13 +121,14 @@ class GoModRequireLine extends GoModLine {
   string getPath() { result = getOffsetToken(this, 1) }
 
   /** Gets the version of the dependency. */
-  string getVer() { result = getOffsetToken(this, 2) }
+  string getVersion() { result = getOffsetToken(this, 2) }
 
   override string toString() { result = "go.mod require line" }
 }
 
 /**
- * A line that declares a dependency version to exclude, for example `exclude "ver"`.
+ * A line that declares a dependency version to exclude, for example
+ * `exclude "github.com/github/codeql-go" v1.2.3`.
  */
 class GoModExcludeLine extends GoModLine {
   GoModExcludeLine() {
@@ -127,14 +142,14 @@ class GoModExcludeLine extends GoModLine {
   string getPath() { result = getOffsetToken(this, 1) }
 
   /** Gets the excluded version. */
-  string getVer() { result = getOffsetToken(this, 2) }
+  string getVersion() { result = getOffsetToken(this, 2) }
 
   override string toString() { result = "go.mod exclude line" }
 }
 
 /**
  * A line that specifies a dependency to use instead of another one, for example
- * `replace "a" => "b" ver`.
+ * `replace "golang.org/x/tools" => "github.com/golang/tools" v1.2.3`.
  */
 class GoModReplaceLine extends GoModLine {
   GoModReplaceLine() {
@@ -147,11 +162,22 @@ class GoModReplaceLine extends GoModLine {
   /** Gets the path of the dependency to be replaced. */
   string getOriginalPath() { result = getOffsetToken(this, 1) }
 
+  /** Gets the path of the dependency to be replaced, if any. */
+  string getOriginalVersion() { result = getOffsetToken(this, 2) and not result = "=>" }
+
   /** Gets the path of the replacement dependency. */
-  string getReplacementPath() { result = getOffsetToken(this, 3) }
+  string getReplacementPath() {
+    if exists(this.getOriginalVersion())
+    then result = getOffsetToken(this, 4)
+    else result = getOffsetToken(this, 3)
+  }
 
   /** Gets the version of the replacement dependency. */
-  string getReplacementVer() { result = getOffsetToken(this, 4) }
+  string getReplacementVersion() {
+    if exists(this.getOriginalVersion())
+    then result = getOffsetToken(this, 5)
+    else result = getOffsetToken(this, 4)
+  }
 
   override string toString() { result = "go.mod replace line" }
 }
diff --git a/ql/src/semmle/go/dependencies/Dependencies.qll b/ql/src/semmle/go/dependencies/Dependencies.qll
index 4972b0682e2..ad09b62e0e5 100644
--- a/ql/src/semmle/go/dependencies/Dependencies.qll
+++ b/ql/src/semmle/go/dependencies/Dependencies.qll
@@ -21,7 +21,7 @@ abstract class Dependency extends Locatable {
   string getDepPath() { this.info(result, _) }
 
   /** Gets the version of this dependency. */
-  string getDepVer() { this.info(_, result) }
+  string getDepVersion() { this.info(_, result) }
 
   /**
    * An import of this dependency.
@@ -51,9 +51,9 @@ class GoModDependency extends Dependency, GoModRequireLine {
     |
       path = replace.getReplacementPath() and
       (
-        v = replace.getReplacementVer()
+        v = replace.getReplacementVersion()
         or
-        not exists(replace.getReplacementVer()) and
+        not exists(replace.getReplacementVersion()) and
         v = "unknown"
       )
     )
@@ -62,17 +62,20 @@ class GoModDependency extends Dependency, GoModRequireLine {
   /**
    * Get a version that was excluded for this dependency.
    */
-  string getAnExcludedVer() {
+  string getAnExcludedVersion() {
     exists(GoModExcludeLine exclude |
       exclude.getFile() = this.getFile() and
       exclude.getPath() = this.getPath()
     |
-      result = exclude.getVer()
+      result = exclude.getVersion()
     )
   }
 
   /**
    * Holds if this require line originally states dependency `path` had version `ver`.
+   *
+   * The actual info of this dependency can change based on `replace` directives in the same go.mod
+   * file, which replace a dependency with another one.
    */
-  predicate originalInfo(string path, string v) { path = this.getPath() and v = this.getVer() }
+  predicate originalInfo(string path, string v) { path = this.getPath() and v = this.getVersion() }
 }
diff --git a/ql/src/semmle/go/dependencies/SemVer.qll b/ql/src/semmle/go/dependencies/SemVer.qll
index a9c6161dd22..7dfb97e0a5c 100644
--- a/ql/src/semmle/go/dependencies/SemVer.qll
+++ b/ql/src/semmle/go/dependencies/SemVer.qll
@@ -10,7 +10,7 @@ class DependencySemVer extends string {
   string normalized;
 
   DependencySemVer() {
-    this = dep.getDepVer() and
+    this = dep.getDepVersion() and
     normalized = normalizeSemver(this)
   }
 
@@ -76,10 +76,10 @@ private string normalizeSemver(string orig) {
  *
  * Pre-release information and build metadata is not yet supported.
  */
-class DependencySemShaVer extends DependencySemVer {
+class DependencySemShaVersion extends DependencySemVer {
   string sha;
 
-  DependencySemShaVer() { sha = this.regexpCapture(".*-([0-9a-f]+)", 1) }
+  DependencySemShaVersion() { sha = this.regexpCapture(".*-([0-9a-f]+)", 1) }
 
   /**
    * Gets the commit SHA associated with this version.
@@ -88,9 +88,9 @@ class DependencySemShaVer extends DependencySemVer {
 
   bindingset[other]
   override predicate is(string other) {
-    this.getSha() = other.(DependencySemShaVer).getSha()
+    this.getSha() = other.(DependencySemShaVersion).getSha()
     or
-    not other instanceof DependencySemShaVer and
+    not other instanceof DependencySemShaVersion and
     super.is(other)
   }
 }
diff --git a/ql/test/library-tests/semmle/go/GoModExpr/ExcludeLines.expected b/ql/test/library-tests/semmle/go/GoModExpr/ExcludeLines.expected
index d23727f1726..b4025cd96ec 100644
--- a/ql/test/library-tests/semmle/go/GoModExpr/ExcludeLines.expected
+++ b/ql/test/library-tests/semmle/go/GoModExpr/ExcludeLines.expected
@@ -1,3 +1,3 @@
-| pkg1/go.mod:10:1:10:44 | go.mod exclude line | module | github.com/github/codeql-go | v1.23.1 |
-| pkg2/go.mod:14:2:14:35 | go.mod exclude line | module | github.com/sirupsen/logrus | v1.4.2 |
-| pkg2/go.mod:15:2:15:37 | go.mod exclude line | module | github.com/github/codeql-go | v1.23.1 |
+| pkg1/go.mod:10:1:10:44 | go.mod exclude line | codeql-go-tests/gomod/dep1 | github.com/github/codeql-go | v1.23.1 |
+| pkg2/go.mod:14:2:14:35 | go.mod exclude line | codeql-go-tests/gomod/dep2 | github.com/sirupsen/logrus | v1.4.2 |
+| pkg2/go.mod:15:2:15:37 | go.mod exclude line | codeql-go-tests/gomod/dep2 | github.com/github/codeql-go | v1.23.1 |
diff --git a/ql/test/library-tests/semmle/go/GoModExpr/ExcludeLines.ql b/ql/test/library-tests/semmle/go/GoModExpr/ExcludeLines.ql
index dfd2262b9b7..f469a5d5b21 100644
--- a/ql/test/library-tests/semmle/go/GoModExpr/ExcludeLines.ql
+++ b/ql/test/library-tests/semmle/go/GoModExpr/ExcludeLines.ql
@@ -1,5 +1,4 @@
 import go
 
-from GoModExcludeLine excl, GoModModuleLine mod
-where excl.getFile() = mod.getFile()
-select excl, mod.getPath(), excl.getPath(), excl.getVer()
+from GoModExcludeLine excl
+select excl, excl.getModulePath(), excl.getPath(), excl.getVersion()
diff --git a/ql/test/library-tests/semmle/go/GoModExpr/ReplaceLines.expected b/ql/test/library-tests/semmle/go/GoModExpr/ReplaceLines.expected
index e69de29bb2d..8aec48809a2 100644
--- a/ql/test/library-tests/semmle/go/GoModExpr/ReplaceLines.expected
+++ b/ql/test/library-tests/semmle/go/GoModExpr/ReplaceLines.expected
@@ -0,0 +1 @@
+| pkg2/go.mod:5:1:5:55 | go.mod replace line | codeql-go-tests/gomod/dep2 | github.com/Masterminds/squirrel | no version | ../squirrel | no version |
diff --git a/ql/test/library-tests/semmle/go/GoModExpr/ReplaceLines.ql b/ql/test/library-tests/semmle/go/GoModExpr/ReplaceLines.ql
index 0e82e2eea9d..485a7c69c44 100644
--- a/ql/test/library-tests/semmle/go/GoModExpr/ReplaceLines.ql
+++ b/ql/test/library-tests/semmle/go/GoModExpr/ReplaceLines.ql
@@ -1,11 +1,18 @@
 import go
 
-from GoModReplaceLine repl, GoModModuleLine mod, string repVer
+from GoModReplaceLine repl, string origVersion, string repVersion
 where
-  repl.getFile() = mod.getFile() and
   (
-    repVer = repl.getReplacementVer() or
-    repVer = "no version"
+    repVersion = repl.getReplacementVersion()
+    or
+    not exists(repl.getReplacementVersion()) and
+    repVersion = "no version"
+  ) and
+  (
+    origVersion = repl.getOriginalVersion()
+    or
+    not exists(repl.getOriginalVersion()) and
+    origVersion = "no version"
   )
-select repl, mod.getPath(), repl.getOriginalPath(), repl.getReplacementPath(),
-  repl.getReplacementVer()
+select repl, repl.getModulePath(), repl.getOriginalPath(), origVersion, repl.getReplacementPath(),
+  repVersion
diff --git a/ql/test/library-tests/semmle/go/GoModExpr/RequireLines.expected b/ql/test/library-tests/semmle/go/GoModExpr/RequireLines.expected
index df5da59bc87..49ec4d743be 100644
--- a/ql/test/library-tests/semmle/go/GoModExpr/RequireLines.expected
+++ b/ql/test/library-tests/semmle/go/GoModExpr/RequireLines.expected
@@ -1,5 +1,5 @@
-| pkg1/go.mod:6:2:6:67 | go.mod require line | module | github.com/github/codeql-go | v1.23.2-0.20200302182241-5e71a04fdf30 |
-| pkg1/go.mod:7:2:7:55 | go.mod require line | module | golang.org/x/tools | v0.0.0-20200109174759-ac4f524c1612 |
-| pkg2/go.mod:7:1:7:38 | go.mod require line | module | github.com/gorilla/mux | v1.7.4 |
-| pkg2/go.mod:9:2:9:35 | go.mod require line | module | github.com/sirupsen/logrus | v1.4.1 |
-| pkg2/go.mod:10:2:10:40 | go.mod require line | module | github.com/Masterminds/squirrel | v1.2.0 |
+| pkg1/go.mod:6:2:6:67 | go.mod require line | codeql-go-tests/gomod/dep1 | github.com/github/codeql-go | v1.23.2-0.20200302182241-5e71a04fdf30 |
+| pkg1/go.mod:7:2:7:55 | go.mod require line | codeql-go-tests/gomod/dep1 | golang.org/x/tools | v0.0.0-20200109174759-ac4f524c1612 |
+| pkg2/go.mod:7:1:7:38 | go.mod require line | codeql-go-tests/gomod/dep2 | github.com/gorilla/mux | v1.7.4 |
+| pkg2/go.mod:9:2:9:35 | go.mod require line | codeql-go-tests/gomod/dep2 | github.com/sirupsen/logrus | v1.4.1 |
+| pkg2/go.mod:10:2:10:40 | go.mod require line | codeql-go-tests/gomod/dep2 | github.com/Masterminds/squirrel | v1.2.0 |
diff --git a/ql/test/library-tests/semmle/go/GoModExpr/RequireLines.ql b/ql/test/library-tests/semmle/go/GoModExpr/RequireLines.ql
index 84854191b65..d3c1ebf3e75 100644
--- a/ql/test/library-tests/semmle/go/GoModExpr/RequireLines.ql
+++ b/ql/test/library-tests/semmle/go/GoModExpr/RequireLines.ql
@@ -1,5 +1,4 @@
 import go
 
-from GoModRequireLine req, GoModModuleLine mod
-where req.getFile() = mod.getFile()
-select req, mod.getPath(), req.getPath(), req.getVer()
+from GoModRequireLine req
+select req, req.getModulePath(), req.getPath(), req.getVersion()
diff --git a/ql/test/library-tests/semmle/go/dependencies/SemVer.ql b/ql/test/library-tests/semmle/go/dependencies/SemVer.ql
index 0c6b7374b94..15a5701673c 100644
--- a/ql/test/library-tests/semmle/go/dependencies/SemVer.ql
+++ b/ql/test/library-tests/semmle/go/dependencies/SemVer.ql
@@ -1,11 +1,11 @@
 import semmle.go.dependencies.SemVer
 
-from DependencySemVer ver, string normVer
+from DependencySemVer ver, string normVersion
 where
   exists(int major, int minor, int patch |
     major = [0 .. 20] and minor = [0 .. 20] and patch = [0 .. 20]
   |
-    normVer = major + "." + minor + "." + patch
+    normVersion = major + "." + minor + "." + patch
   ) and
-  ver.is(normVer)
-select ver, normVer
+  ver.is(normVersion)
+select ver, normVersion
diff --git a/ql/test/library-tests/semmle/go/dependencies/ShaVer.ql b/ql/test/library-tests/semmle/go/dependencies/ShaVer.ql
index 19b9b319bdc..b85b5cbbfab 100644
--- a/ql/test/library-tests/semmle/go/dependencies/ShaVer.ql
+++ b/ql/test/library-tests/semmle/go/dependencies/ShaVer.ql
@@ -1,4 +1,4 @@
 import semmle.go.dependencies.SemVer
 
-from DependencySemShaVer ver
+from DependencySemShaVersion ver
 select ver, ver.getSha()
diff --git a/ql/test/library-tests/semmle/go/dependencies/fabric-snaps/README.md b/ql/test/library-tests/semmle/go/dependencies/fabric-snaps/README.md
new file mode 100644
index 00000000000..343b9e55123
--- /dev/null
+++ b/ql/test/library-tests/semmle/go/dependencies/fabric-snaps/README.md
@@ -0,0 +1,3 @@
+This is the go.mod and go.sum files from  https://github.com/securekey/fabric-snaps, strictly for use in query testing.
+
+See the LICENSE file in this folder for information about the licensing of the original code.
diff --git a/ql/test/library-tests/semmle/go/dependencies/hrm-profile-tool/README.md b/ql/test/library-tests/semmle/go/dependencies/hrm-profile-tool/README.md
new file mode 100644
index 00000000000..98ad865d147
--- /dev/null
+++ b/ql/test/library-tests/semmle/go/dependencies/hrm-profile-tool/README.md
@@ -0,0 +1,3 @@
+This is the go.mod and go.sum files from  https://github.com/clj/hrm-profile-tool, strictly for use in query testing.
+
+See the LICENSE file in this folder for information about the licensing of the original code.
diff --git a/ql/test/library-tests/semmle/go/dependencies/sweb/README.md b/ql/test/library-tests/semmle/go/dependencies/sweb/README.md
new file mode 100644
index 00000000000..549bdee8bf8
--- /dev/null
+++ b/ql/test/library-tests/semmle/go/dependencies/sweb/README.md
@@ -0,0 +1,3 @@
+This is the go.mod and go.sum files from  https://github.com/xuender/sweb, strictly for use in query testing.
+
+See the LICENSE file in this folder for information about the licensing of the original code.