Apply review comments

ql/src/Security/CWE-601/OpenUrlRedirect.ql

@@ -14,7 +14,12 @@ import go
 import semmle.go.security.OpenUrlRedirect::OpenUrlRedirect
 import DataFlow::PathGraph
 
-from Configuration cfg, DataFlow::PathNode source, DataFlow::PathNode sink
-where cfg.hasFlowPath(source, sink)
+from
+  Configuration cfg, SafeUrlConfiguration scfg, DataFlow::PathNode source, DataFlow::PathNode sink
+where
+  cfg.hasFlowPath(source, sink) and
+  // this excludes flow from safe parts of request URLs, for example the full URL when the
+  // doing a redirect from `http://<path>` to `https://<path>`
+  not scfg.hasFlow(_, sink.getNode())
 select sink.getNode(), source, sink, "Untrusted URL redirection due to $@.", source.getNode(),
   "user-provided value"

ql/src/semmle/go/security/OpenUrlRedirect.qll

@@ -14,17 +14,14 @@ module OpenUrlRedirect {
   import OpenUrlRedirectCustomizations::OpenUrlRedirect
 
   /**
-   * A taint-tracking configuration for reasoning about unvalidated URL redirections.
+   * A data-flow configuration for reasoning about unvalidated URL redirections.
    */
   class Configuration extends DataFlow::Configuration {
     Configuration() { this = "OpenUrlRedirect" }
 
     override predicate isSource(DataFlow::Node source) { source instanceof Source }
 
-    override predicate isSink(DataFlow::Node sink) {
-      sink instanceof Sink and
-      not sink instanceof NotSink
-    }
+    override predicate isSink(DataFlow::Node sink) { sink instanceof Sink }
 
     override predicate isBarrier(DataFlow::Node node) { node instanceof Barrier }
 
@@ -48,4 +45,25 @@ module OpenUrlRedirect {
       guard instanceof BarrierGuard
     }
   }
+
+  /**
+   * A data-flow configuration for reasoning about safe URLs for unvalidated URL redirections.
+   */
+  class SafeUrlConfiguration extends DataFlow::Configuration {
+    SafeUrlConfiguration() { this = "SafeUrlFlow" }
+
+    override predicate isSource(DataFlow::Node source) {
+      source.(DataFlow::FieldReadNode).getField().hasQualifiedName("net/http", "Request", "URL")
+    }
+
+    override predicate isSink(DataFlow::Node sink) { sink instanceof Sink }
+
+    override predicate isAdditionalFlowStep(DataFlow::Node pred, DataFlow::Node succ) {
+      TaintTracking::functionModelStep(any(SafeUrlMethod m), pred, succ)
+      or
+      exists(DataFlow::FieldReadNode frn | succ = frn |
+        frn.getBase() = pred and frn.getFieldName() = "Host"
+      )
+    }
+  }
 }

ql/src/semmle/go/security/OpenUrlRedirectCustomizations.qll

@@ -18,14 +18,6 @@ module OpenUrlRedirect {
    */
   abstract class Sink extends DataFlow::Node { }
 
-  /**
-   * A data flow node that should not be considered a sink for unvalidated URL redirect
-   * vulnerabilities.
-   *
-   * This takes precedence over `Sink`, and so can be used to remove sinks that are safe.
-   */
-  abstract class NotSink extends DataFlow::Node { }
-
   /**
    * A barrier for unvalidated URL redirect vulnerabilities.
    */
@@ -37,17 +29,12 @@ module OpenUrlRedirect {
   abstract class BarrierGuard extends DataFlow::BarrierGuard { }
 
   /**
-   * A source of third-party user input, considered as a flow source for URL redirects.
-   *
-   * Excludes the URL field, as it is handled more precisely in `UntrustedUrlField`.
+   * A method on a `net/url.URL` that is considered safe to redirect to.
    */
-  class UntrustedFlowAsSource extends Source, UntrustedFlowSource { }
-
   class SafeUrlMethod extends TaintTracking::FunctionModel, Method {
     SafeUrlMethod() {
       this instanceof StringMethod
       or
-      this instanceof URL::UrlGetter and
       exists(string m | this.hasQualifiedName("net/url", "URL", m) | m = "Hostname" or m = "Port")
     }
 
@@ -56,33 +43,10 @@ module OpenUrlRedirect {
     }
   }
 
-  private class SafeUrlConfiguration extends DataFlow2::Configuration {
-    SafeUrlConfiguration() { this = "FullUrlFlow" }
-
-    override predicate isSource(DataFlow::Node source) {
-      exists(Type req | req.hasQualifiedName("net/http", "Request") |
-        source.(DataFlow::FieldReadNode).getField() = req.getField("URL")
-      )
-    }
-
-    override predicate isSink(DataFlow::Node sink) { sink instanceof Sink }
-
-    override predicate isAdditionalFlowStep(DataFlow::Node pred, DataFlow::Node succ) {
-      TaintTracking::functionModelStep(any(SafeUrlMethod m), pred, succ)
-      or
-      exists(DataFlow::FieldReadNode frn | succ = frn |
-        frn.getBase() = pred and frn.getFieldName() = "Host"
-      )
-    }
-  }
-
   /**
-   * A safe part of a request URL to redirect to. This includes values such as the hostname and port
-   * which should not be considered as unsafe when redirected to.
+   * A source of third-party user input, considered as a flow source for URL redirects.
    */
-  class SafeUrl extends NotSink, DataFlow::Node {
-    SafeUrl() { exists(SafeUrlConfiguration conf | conf.hasFlow(_, this)) }
-  }
+  class UntrustedFlowAsSource extends Source, UntrustedFlowSource { }
 
   /**
    * An HTTP redirect, considered as a sink for `Configuration`.

ql/test/query-tests/Security/CWE-601/OpenUrlRedirect/OpenUrlRedirect.expected

@@ -6,6 +6,8 @@ edges
 | stdlib.go:43:13:43:18 | selection of Form : Values | stdlib.go:45:23:45:28 | target |
 | stdlib.go:63:13:63:18 | selection of Form : Values | stdlib.go:66:23:66:40 | ...+... |
 | stdlib.go:88:13:88:18 | selection of Form : Values | stdlib.go:91:23:91:28 | target |
+| stdlib.go:112:24:112:28 | selection of URL : pointer type | stdlib.go:112:24:112:37 | call to String |
+| stdlib.go:112:24:112:28 | selection of URL : pointer type | stdlib.go:112:24:112:37 | call to String |
 | stdlib.go:133:13:133:18 | selection of Form : Values | stdlib.go:139:23:139:28 | target |
 nodes
 | OpenUrlRedirect.go:10:23:10:28 | selection of Form : Values | semmle.label | selection of Form : Values |
@@ -22,6 +24,10 @@ nodes
 | stdlib.go:66:23:66:40 | ...+... | semmle.label | ...+... |
 | stdlib.go:88:13:88:18 | selection of Form : Values | semmle.label | selection of Form : Values |
 | stdlib.go:91:23:91:28 | target | semmle.label | target |
+| stdlib.go:112:24:112:28 | selection of URL : pointer type | semmle.label | selection of URL : pointer type |
+| stdlib.go:112:24:112:28 | selection of URL : pointer type | semmle.label | selection of URL : pointer type |
+| stdlib.go:112:24:112:37 | call to String | semmle.label | call to String |
+| stdlib.go:112:24:112:37 | call to String | semmle.label | call to String |
 | stdlib.go:133:13:133:18 | selection of Form : Values | semmle.label | selection of Form : Values |
 | stdlib.go:139:23:139:28 | target | semmle.label | target |
 #select