Python: Fix URL redirection validation sanitizers

python/ql/lib/change-notes/2026-10-22-url-redirect-validation.md

@@ -0,0 +1,4 @@
+---
+category: minorAnalysis
+---
+* URL redirection sanitizers based on `urllib.parse.urlparse` and `urllib.parse.urlsplit` now require checks of the scheme, leading slashes, and browser-ignored characters when the network location may be empty. Membership allowlists must resolve locally to collection literals, and arbitrary attributes are no longer assumed to exclude an empty network location.

python/ql/lib/semmle/python/frameworks/Stdlib/Urllib.qll

@@ -81,47 +81,235 @@ private module Urllib {
    */
   module Parse {
     /**
-     * A call to `urllib.parse.urlparse`.
+     * A call to `urllib.parse.urlparse` or `urllib.parse.urlsplit`.
      */
     private DataFlow::CallCfgNode getUrlParseCall() {
-      result = API::moduleImport("urllib").getMember("parse").getMember("urlparse").getACall()
+      result =
+        API::moduleImport("urllib")
+            .getMember("parse")
+            .getMember(["urlparse", "urlsplit"])
+            .getACall()
+    }
+
+    /** Holds if `e` is a parsed URL hostname, which is either non-empty or `None`. */
+    private predicate isParsedUrlHostname(Expr e) {
+      exists(DataFlow::AttrRead read |
+        read = DataFlow::exprNode(e) and
+        read.getAttributeName() = "hostname" and
+        read.getObject().getALocalSource() = getUrlParseCall()
+      )
+    }
+
+    /** Holds if `e` itself is known not to be `""`. */
+    private predicate sourceExcludesEmptyString(Expr e) {
+      e.(StringLiteral).getText() != ""
+      or
+      isParsedUrlHostname(e)
+    }
+
+    /** Holds if every local source of `e` excludes `""`. */
+    private predicate excludesEmptyString(Expr e) {
+      forex(DataFlow::LocalSourceNode source | source = DataFlow::exprNode(e).getALocalSource() |
+        sourceExcludesEmptyString(source.asExpr())
+      )
+    }
+
+    /** Holds if every local source of `e` is a collection literal, rather than a string. */
+    private predicate membershipChecksCollection(Expr e) {
+      forex(DataFlow::LocalSourceNode source | source = DataFlow::exprNode(e).getALocalSource() |
+        source.asCfgNode() instanceof IterableNode
+        or
+        source.asCfgNode() instanceof DictNode
+      )
+    }
+
+    /** Holds if every local source of `e` is a collection literal whose elements all exclude `""`. */
+    private predicate membershipExcludesEmptyString(Expr e) {
+      forex(DataFlow::LocalSourceNode source | source = DataFlow::exprNode(e).getALocalSource() |
+        exists(Expr collection |
+          collection = source.asExpr() and
+          (collection instanceof List or collection instanceof Tuple or collection instanceof Set) and
+          forall(Expr elt |
+            elt =
+              [
+                collection.(List).getAnElt(), collection.(Tuple).getAnElt(),
+                collection.(Set).getAnElt()
+              ]
+          |
+            excludesEmptyString(elt)
+          )
+        )
+      )
+    }
+
+    /** Holds if every local source of `e` is `""`. */
+    private predicate isEmptyString(Expr e) {
+      forex(DataFlow::LocalSourceNode source | source = DataFlow::exprNode(e).getALocalSource() |
+        source.asExpr().(StringLiteral).getText() = ""
+      )
+    }
+
+    /** Holds if comparing against `e` using `op` cannot accept `""`. */
+    private predicate comparisonExcludesEmptyString(Cmpop op, Expr e) {
+      op.getSymbol() in ["in", "not in"] and membershipExcludesEmptyString(e)
+      or
+      op.getSymbol() in ["==", "is", "!=", "is not"] and excludesEmptyString(e)
     }
 
     /**
-     * A read of the `netloc` attribute of a parsed URL as returned by `urllib.parse.urlparse`,
-     * which is being checked in a way that is relevant for URL redirection vulnerabilities.
+     * Holds if `g` is a falsiness check `not parsed.<attribute>` on a parsed URL `node`.
      */
-    private predicate netlocCheck(DataFlow::GuardNode g, ControlFlowNode node, boolean branch) {
-      exists(DataFlow::CallCfgNode urlParseCall, DataFlow::AttrRead netlocRead |
+    private predicate parsedUrlAttributeFalsiness(
+      DataFlow::GuardNode g, ControlFlowNode node, boolean branch, string attribute
+    ) {
+      exists(DataFlow::CallCfgNode urlParseCall |
         urlParseCall = getUrlParseCall() and
-        netlocRead = urlParseCall.getAnAttributeRead("netloc") and
-        node = urlParseCall.getArg(0).asCfgNode()
-      |
-        // either a simple check of the netloc attribute
-        g = netlocRead.asCfgNode() and
+        g = urlParseCall.getAnAttributeRead(attribute).asCfgNode() and
+        node = [urlParseCall.getArg(0), urlParseCall.getArgByName(["url", "urlstring"])].asCfgNode() and
         branch = false
+      )
+    }
+
+    /**
+     * Holds if `g` compares `attribute` (`"netloc"` or `"scheme"`) of a parsed URL `node`
+     * against `other` using `op`, and the equality or collection membership holds on `branch`.
+     */
+    private predicate parsedUrlAttributeComparison(
+      DataFlow::GuardNode g, ControlFlowNode node, boolean branch, string attribute, Cmpop op,
+      Expr other
+    ) {
+      exists(
+        DataFlow::CallCfgNode urlParseCall, DataFlow::AttrRead attributeRead, CompareNode cmp,
+        ControlFlowNode otherNode
+      |
+        urlParseCall = getUrlParseCall() and
+        attributeRead = urlParseCall.getAnAttributeRead(attribute) and
+        node = [urlParseCall.getArg(0), urlParseCall.getArgByName(["url", "urlstring"])].asCfgNode() and
+        cmp = g and
+        op = unique(Cmpop opp | opp = cmp.getNode().getAnOp()) and
+        other = otherNode.getNode()
+      |
+        op.getSymbol() in ["==", "is"] and
+        branch = true and
+        (
+          cmp.operands(attributeRead.asCfgNode(), op, otherNode) or
+          cmp.operands(otherNode, op, attributeRead.asCfgNode())
+        )
         or
-        // or a comparison (we don't care against what)
-        exists(Compare cmp, string op |
-          cmp = g.getNode() and
-          op = unique(Cmpop opp | opp = cmp.getAnOp()).getSymbol() and
-          cmp.getASubExpression() = netlocRead.asExpr()
-        |
-          op in ["==", "is", "in"] and branch = true
-          or
-          op in ["!=", "is not", "not in"] and branch = false
+        op.getSymbol() in ["!=", "is not"] and
+        branch = false and
+        (
+          cmp.operands(attributeRead.asCfgNode(), op, otherNode) or
+          cmp.operands(otherNode, op, attributeRead.asCfgNode())
         )
+        or
+        op.getSymbol() = "in" and
+        membershipChecksCollection(other) and
+        branch = true and
+        cmp.operands(attributeRead.asCfgNode(), op, otherNode)
+        or
+        op.getSymbol() = "not in" and
+        membershipChecksCollection(other) and
+        branch = false and
+        cmp.operands(attributeRead.asCfgNode(), op, otherNode)
+      )
+    }
+
+    /** A `netloc` check that does not allow `""` (e.g. `parsed.netloc == request.host`). */
+    private predicate nonEmptyNetlocCheck(
+      DataFlow::GuardNode g, ControlFlowNode node, boolean branch
+    ) {
+      exists(Cmpop op, Expr other |
+        parsedUrlAttributeComparison(g, node, branch, "netloc", op, other) and
+        comparisonExcludesEmptyString(op, other)
+      )
+    }
+
+    /** A `netloc` check that allows `""` (e.g. `not parsed.netloc` or `parsed.netloc in ["", host]`). */
+    private predicate possiblyEmptyNetlocCheck(
+      DataFlow::GuardNode g, ControlFlowNode node, boolean branch
+    ) {
+      parsedUrlAttributeFalsiness(g, node, branch, "netloc")
+      or
+      exists(Cmpop op, Expr other |
+        parsedUrlAttributeComparison(g, node, branch, "netloc", op, other) and
+        not comparisonExcludesEmptyString(op, other)
+      )
+    }
+
+    /** A `scheme` check that ensures `scheme` is `""`. */
+    private predicate emptySchemeCheck(DataFlow::GuardNode g, ControlFlowNode node, boolean branch) {
+      parsedUrlAttributeFalsiness(g, node, branch, "scheme")
+      or
+      exists(Cmpop op, Expr other |
+        parsedUrlAttributeComparison(g, node, branch, "scheme", op, other) and
+        op.getSymbol() in ["==", "is", "!=", "is not"] and
+        isEmptyString(other)
+      )
+    }
+
+    private predicate leadingSlashesCheck(
+      DataFlow::GuardNode g, ControlFlowNode node, boolean branch
+    ) {
+      exists(DataFlow::MethodCallNode call, DataFlow::Node url, Expr prefix |
+        call.calls(url, "startswith") and
+        (
+          prefix = call.getArg(0).asExpr() or
+          prefix = call.getArg(0).asExpr().(Tuple).getAnElt()
+        ) and
+        prefix.(StringLiteral).getText() in ["/", "//", "///"] and
+        not exists(call.getArg(1)) and
+        g = call.asCfgNode() and
+        node = url.asCfgNode() and
+        branch = false
+      )
+    }
+
+    /** Holds if every local source of `node` is a no-argument `strip` or `lstrip` call. */
+    private predicate isStripped(DataFlow::Node node) {
+      forex(DataFlow::LocalSourceNode source | source = node.getALocalSource() |
+        source.(DataFlow::MethodCallNode).getMethodName() in ["strip", "lstrip"] and
+        not exists(source.asExpr().(Call).getAPositionalArg()) and
+        not exists(source.asExpr().(Call).getANamedArg())
+      )
+    }
+
+    /** An `isprintable` check on a stripped URL. */
+    private predicate normalizedPrintableCheck(
+      DataFlow::GuardNode g, ControlFlowNode node, boolean branch
+    ) {
+      exists(DataFlow::MethodCallNode call, DataFlow::Node url |
+        call.calls(url, "isprintable") and
+        not exists(call.asExpr().(Call).getAPositionalArg()) and
+        not exists(call.asExpr().(Call).getANamedArg()) and
+        isStripped(url) and
+        g = call.asCfgNode() and
+        node = url.asCfgNode() and
+        branch = true
       )
     }
 
     /**
-     * A check of `urllib.parse.urlparse().netloc`, considered as a sanitizer-guard for URL redirection.
+     * A check of `urllib.parse.urlparse` or `urllib.parse.urlsplit` considered as a
+     * sanitizer-guard for URL redirection.
+     *
+     * Either `netloc` is restricted to a non-empty host, or `netloc` may be empty
+     * and `scheme`, leading slashes (`///`), and browser-ignored characters are also checked.
      */
     private class NetlocCheck extends UrlRedirect::Sanitizer {
-      NetlocCheck() { this = DataFlow::BarrierGuard<netlocCheck/3>::getABarrierNode() }
+      NetlocCheck() {
+        this = DataFlow::BarrierGuard<nonEmptyNetlocCheck/3>::getABarrierNode()
+        or
+        this = DataFlow::BarrierGuard<possiblyEmptyNetlocCheck/3>::getABarrierNode() and
+        this = DataFlow::BarrierGuard<emptySchemeCheck/3>::getABarrierNode() and
+        // Browsers interpret three or more leading slashes as an authority, unlike `urlparse`.
+        this = DataFlow::BarrierGuard<leadingSlashesCheck/3>::getABarrierNode() and
+        // Browsers ignore leading whitespace and control characters within URLs.
+        this = DataFlow::BarrierGuard<normalizedPrintableCheck/3>::getABarrierNode()
+      }
 
       override predicate sanitizes(UrlRedirect::FlowState state) {
-        // `urlparse` does not handle backslashes
+        // Neither parser handles backslashes as browsers do.
         state instanceof UrlRedirect::NoBackslashes
       }
     }

python/ql/src/Security/CWE-601/UrlRedirect.qhelp

@@ -18,7 +18,7 @@ redirects on the server; then choose from that list based on the user input prov
 </p>
 <p>
 If this is not possible, then the user input should be validated in some other way,
-for example, by verifying that the target URL does not include an explicit host name.
+using a maintained framework helper that accounts for how browsers interpret URLs.
 </p>
 </recommendation>
 
@@ -38,40 +38,41 @@ and check that the user input is in that list:
 <sample src="examples/redirect_good.py"/>
 
 <p>
-Often this is not possible, so an alternative is to check that the target URL does not
-specify an explicit host name. For example, you can use the <code>urlparse</code> function
-from the Python standard library to parse the URL and check that the <code>netloc</code>
-attribute is empty.
+If an allowlist is not practical, use a maintained framework helper to validate the
+redirect target. For Django applications, <code>url_has_allowed_host_and_scheme</code>
+checks whether a URL is safe to redirect to:
 </p>
 
+<sample src="examples/redirect_good3.py"/>
+
 <p>
-Note, however, that some cases are not handled as we desire out-of-the-box by <code>urlparse</code>, so we need to adjust two things, as shown in the example below:
+Note that <code>url_has_allowed_host_and_scheme</code> handles backslashes correctly, so no
+additional processing is required.
+</p>
+
+<p>
+Do not rely on <code>urlparse</code> or <code>urlsplit</code> alone to validate redirect
+targets. These functions do not interpret all URLs in the same way as browsers:
 </p>
 
 <ul>
   <li>
     Many browsers accept backslash characters (<code>\</code>) as equivalent
-    to forward slash characters (<code>/</code>) in URLs, but the <code>urlparse</code> function
-    does not.
+    to forward slash characters (<code>/</code>) in URLs, but these parsers do not.
   </li>
   <li>
     Mistyped URLs such as <code>https:/example.com</code> or <code>https:///example.com</code> are parsed as having an empty <code>netloc</code> attribute, while browsers will still redirect to the correct site.
   </li>
+  <li>
+    URLs such as <code>///example.com</code> and <code>////example.com</code> are parsed as
+    having both an empty <code>netloc</code> and an empty <code>scheme</code>, but browsers
+    interpret them as URLs with a host name.
+  </li>
 </ul>
 
-
-<sample src="examples/redirect_good2.py"/>
-
 <p>
-For Django application, you can use the function <code>url_has_allowed_host_and_scheme</code>
-to check that a URL is safe to redirect to, as shown in the following example:
-</p>
-
-<sample src="examples/redirect_good3.py"/>
-
-<p>
-Note that <code>url_has_allowed_host_and_scheme</code> handles backslashes correctly, so no
-additional processing is required.
+Hand-written validation must also account for whitespace and control characters that
+browsers may ignore. Validate the same normalized value that is used in the redirect.
 </p>
 
 </example>

python/ql/src/change-notes/2026-10-22-url-redirect-validation.md

@@ -0,0 +1,4 @@
+---
+category: minorAnalysis
+---
+* The `py/url-redirection` query now reports redirects guarded only by incomplete `urllib.parse` checks, including checks that accept URLs beginning with three or more slashes. The query help no longer recommends an empty network location and scheme as sufficient validation.