Apply code review suggestions

python/ql/src/experimental/Security/CWE-347/JWTEmptyKeyOrAlgorithm.ql

@@ -14,11 +14,9 @@ import experimental.semmle.python.frameworks.JWT
 
 from JWTEncoding jwtEncoding, string affectedComponent
 where
-  exists( |
-    affectedComponent = "algorithm" and
-    isEmptyOrNone(jwtEncoding.getAlgorithm())
-    or
-    affectedComponent = "key" and
-    isEmptyOrNone(jwtEncoding.getKey())
-  )
-select jwtEncoding, affectedComponent, "is empty."
+  affectedComponent = "algorithm" and
+  isEmptyOrNone(jwtEncoding.getAlgorithm())
+  or
+  affectedComponent = "key" and
+  isEmptyOrNone(jwtEncoding.getKey())
+select jwtEncoding, "This JWT encoding has an empty " + affectedComponent + "."

python/ql/src/experimental/semmle/python/Concepts.qll

@@ -334,30 +334,26 @@ module JWTEncoding {
  * Extend this class to refine existing API models. If you want to model new APIs,
  * extend `JWTEncoding::Range` instead.
  */
-class JWTEncoding extends DataFlow::Node {
-  JWTEncoding::Range range;
-
-  JWTEncoding() { this = range }
-
+class JWTEncoding extends DataFlow::Node instanceof JWTEncoding::Range {
   /**
    * Gets the argument containing the payload.
    */
-  DataFlow::Node getPayload() { result = range.getPayload() }
+  DataFlow::Node getPayload() { result = super.getPayload() }
 
   /**
    * Gets the argument containing the encoding key.
    */
-  DataFlow::Node getKey() { result = range.getKey() }
+  DataFlow::Node getKey() { result = super.getKey() }
 
   /**
    * Gets the algorithm Node used in the encoding.
    */
-  DataFlow::Node getAlgorithm() { result = range.getAlgorithm() }
+  DataFlow::Node getAlgorithm() { result = super.getAlgorithm() }
 
   /**
    * Tries to get the algorithm used in the encoding.
    */
-  string getAlgorithmString() { result = range.getAlgorithmString() }
+  string getAlgorithmString() { result = super.getAlgorithmString() }
 }
 
 /** Provides classes for modeling JWT decoding-related APIs. */
@@ -407,38 +403,34 @@ module JWTDecoding {
  * Extend this class to refine existing API models. If you want to model new APIs,
  * extend `JWTDecoding::Range` instead.
  */
-class JWTDecoding extends DataFlow::Node {
-  JWTDecoding::Range range;
-
-  JWTDecoding() { this = range }
-
+class JWTDecoding extends DataFlow::Node instanceof JWTDecoding::Range {
   /**
    * Gets the argument containing the payload.
    */
-  DataFlow::Node getPayload() { result = range.getPayload() }
+  DataFlow::Node getPayload() { result = super.getPayload() }
 
   /**
    * Gets the argument containing the encoding key.
    */
-  DataFlow::Node getKey() { result = range.getKey() }
+  DataFlow::Node getKey() { result = super.getKey() }
 
   /**
    * Gets the algorithm Node used in the encoding.
    */
-  DataFlow::Node getAlgorithm() { result = range.getAlgorithm() }
+  DataFlow::Node getAlgorithm() { result = super.getAlgorithm() }
 
   /**
    * Tries to get the algorithm used in the encoding.
    */
-  string getAlgorithmString() { result = range.getAlgorithmString() }
+  string getAlgorithmString() { result = super.getAlgorithmString() }
 
   /**
    * Gets the options Node used in the encoding.
    */
-  DataFlow::Node getOptions() { result = range.getOptions() }
+  DataFlow::Node getOptions() { result = super.getOptions() }
 
   /**
    * Checks if the signature gets verified while decoding.
    */
-  predicate verifiesSignature() { range.verifiesSignature() }
+  predicate verifiesSignature() { super.verifiesSignature() }
 }

python/ql/src/experimental/semmle/python/frameworks/JWT.qll

@@ -14,10 +14,10 @@ predicate isEmpty(DataFlow::Node arg) {
 
 /** Checks if `None` flows to `arg` */
 predicate isNone(DataFlow::Node arg) {
-  exists( | DataFlow::exprNode(any(None no)).(DataFlow::LocalSourceNode).flowsTo(arg))
+  DataFlow::exprNode(any(None no)).(DataFlow::LocalSourceNode).flowsTo(arg)
 }
 
 /** Checks if `False` flows to `arg` */
 predicate isFalse(DataFlow::Node arg) {
-  exists( | DataFlow::exprNode(any(False falseExpr)).(DataFlow::LocalSourceNode).flowsTo(arg))
+  DataFlow::exprNode(any(False falseExpr)).(DataFlow::LocalSourceNode).flowsTo(arg)
 }

python/ql/src/experimental/semmle/python/libraries/Authlib.qll

@@ -34,7 +34,6 @@ private module Authlib {
    * * `getAlgorithmstring()`'s result would be `HS256`.
    */
   private class AuthlibJWTEncodeCall extends DataFlow::CallCfgNode, JWTEncoding::Range {
-    // def encode(self, header, payload, key, check=True):
     AuthlibJWTEncodeCall() { this = authlibJWTEncode().getACall() }
 
     override DataFlow::Node getPayload() { result = this.getArg(1) }
@@ -71,7 +70,6 @@ private module Authlib {
    * * `getKey()`'s result would be `key`.
    */
   private class AuthlibJWTDecodeCall extends DataFlow::CallCfgNode, JWTDecoding::Range {
-    // def decode(self, s, key, claims_cls=None, claims_options=None, claims_params=None):
     AuthlibJWTDecodeCall() { this = authlibJWTDecode().getACall() }
 
     override DataFlow::Node getPayload() { result = this.getArg(0) }

python/ql/src/experimental/semmle/python/libraries/PyJWT.qll

@@ -26,7 +26,6 @@ private module PyJWT {
    * * `getAlgorithmstring()`'s result would be `HS256`.
    */
   private class PyJWTEncodeCall extends DataFlow::CallCfgNode, JWTEncoding::Range {
-    // def encode(self, payload, key, algorithm="HS256", headers=None, json_encoder=None)
     PyJWTEncodeCall() { this = pyjwtEncode().getACall() }
 
     override DataFlow::Node getPayload() {
@@ -65,7 +64,6 @@ private module PyJWT {
    * * `verifiesSignature()` predicate would succeed.
    */
   private class PyJWTDecodeCall extends DataFlow::CallCfgNode, JWTDecoding::Range {
-    // def decode(self, jwt, key="", algorithms=None, options=None)
     PyJWTDecodeCall() { this = pyjwtDecode().getACall() }
 
     override DataFlow::Node getPayload() { result in [this.getArg(0), this.getArgByName("jwt")] }
@@ -88,13 +86,20 @@ private module PyJWT {
     }
 
     override predicate verifiesSignature() {
-      // jwt.decode(token, "key", "HS256")
-      not exists(this.getArgByName("verify")) and not exists(this.getOptions())
+      this.hasNoVerifyArgumentOrOptions()
       or
-      // jwt.decode(token, verify=False)
-      not isFalse(this.getArgByName("verify")) and
-      // jwt.decode(token, key, options={"verify_signature": False})
-      not exists(KeyValuePair optionsDict, NameConstant falseName |
+      not this.hasVerifySetToFalse() and
+      not this.hasVerifySignatureSetToFalse()
+    }
+
+    predicate hasNoVerifyArgumentOrOptions() {
+      not exists(this.getArgByName("verify")) and not exists(this.getOptions())
+    }
+
+    predicate hasVerifySetToFalse() { isFalse(this.getArgByName("verify")) }
+
+    predicate hasVerifySignatureSetToFalse() {
+      exists(KeyValuePair optionsDict, NameConstant falseName |
         falseName.getId() = "False" and
         optionsDict = this.getOptions().asExpr().(Dict).getItems().getAnItem() and
         optionsDict.getKey().(Str_).getS().matches("%verify%") and

python/ql/src/experimental/semmle/python/libraries/PythonJose.qll

@@ -29,7 +29,6 @@ private module PythonJose {
    * * `getAlgorithmstring()`'s result would be `HS256`.
    */
   private class JoseJWTEncodeCall extends DataFlow::CallCfgNode, JWTEncoding::Range {
-    // def encode(claims, key, algorithm=ALGORITHMS.HS256, headers=None, access_token=None):
     JoseJWTEncodeCall() { this = joseJWTEncode().getACall() }
 
     override DataFlow::Node getPayload() { result = this.getArg(0) }
@@ -66,7 +65,6 @@ private module PythonJose {
    * * `verifiesSignature()` predicate would succeed.
    */
   private class JoseJWTDecodeCall extends DataFlow::CallCfgNode, JWTDecoding::Range {
-    // def decode(token, key, algorithms=None, options=None, audience=None, issuer=None, subject=None, access_token=None):
     JoseJWTDecodeCall() { this = joseJWTDecode().getACall() }
 
     override DataFlow::Node getPayload() { result = this.getArg(0) }