JS: Fix regression from global declare vars

asgerf · asgerf · commit ae0aeb9aaa83 · 2025-05-19T12:00:11.000+02:00
diff --git a/javascript/ql/lib/semmle/javascript/Variables.qll b/javascript/ql/lib/semmle/javascript/Variables.qll
@@ -134,8 +134,26 @@ class Variable extends @variable, LexicalName {
   /** Gets the scope this variable is declared in. */
   override Scope getScope() { variables(this, _, result) }
 
+  /**
+   * Holds if this variable is declared in the top-level of a module using a `declare` statement.
+   *
+   * For example:
+   * ```js
+   * declare var $: any;
+   * ```
+   *
+   * Such variables are generally treated as a global variables, except for type-checking related purposes.
+   */
+  pragma[nomagic]
+  predicate isTopLevelWithAmbientDeclaration() {
+    this.getScope() instanceof ModuleScope and
+    forex(VarDecl decl | decl = this.getADeclaration() | decl.isAmbient())
+  }
+
   /** Holds if this is a global variable. */
-  predicate isGlobal() { this.getScope() instanceof GlobalScope }
+  predicate isGlobal() {
+    this.getScope() instanceof GlobalScope or this.isTopLevelWithAmbientDeclaration()
+  }
 
   /**
    * Holds if this is a variable exported from a TypeScript namespace.
diff --git a/javascript/ql/lib/semmle/javascript/internal/NameResolution.qll b/javascript/ql/lib/semmle/javascript/internal/NameResolution.qll
@@ -28,7 +28,7 @@ module NameResolution {
     Location getLocation() {
       result = this.(AstNode).getLocation()
       or
-      result = this.(LocalVariable).getLocation()
+      result = this.(LocalVariableLike).getLocation()
       or
       result = this.(JSDocTypeExpr).getLocation()
     }
@@ -47,6 +47,22 @@ module NameResolution {
     }
   }
 
+  /**
+   * A local variable, or a top-level variable that acts as a global variable due to an ambient declaration.
+   */
+  class LocalVariableLike extends Variable {
+    LocalVariableLike() { this.isLocal() or this.isTopLevelWithAmbientDeclaration() }
+
+    Location getLocation() {
+      result =
+        min(Location loc |
+          loc = this.getADeclaration().getLocation()
+        |
+          loc order by loc.getStartLine(), loc.getStartColumn()
+        )
+    }
+  }
+
   /**
    * Holds if values/namespaces/types in `node1` can flow to values/namespaces/types in `node2`.
    */
@@ -224,7 +240,7 @@ module NameResolution {
   /**
    * A local variable with exactly one definition, not counting implicit initialization.
    */
-  private class EffectivelyConstantVariable extends LocalVariable {
+  private class EffectivelyConstantVariable extends LocalVariableLike {
     EffectivelyConstantVariable() {
       count(SsaExplicitDefinition ssa | ssa.getSourceVariable() = this) <= 1 // count may be zero if ambient
     }
@@ -294,7 +310,7 @@ module NameResolution {
      * Holds if `value` is stored in `target.prop`. Only needs to recognise assignments
      * that are also recognised by JSDoc tooling such as the Closure compiler.
      */
-    private predicate storeToVariable(Expr value, string prop, LocalVariable target) {
+    private predicate storeToVariable(Expr value, string prop, LocalVariableLike target) {
       exists(AssignExpr assign |
         // exports.name = value
         assign.getLhs().(PropAccess).accesses(target.getAnAccess(), prop) and
diff --git a/javascript/ql/lib/semmle/javascript/internal/TypeResolution.qll b/javascript/ql/lib/semmle/javascript/internal/TypeResolution.qll
@@ -190,7 +190,7 @@ module TypeResolution {
   }
 
   predicate contextualType(Node value, Node type) {
-    exists(LocalVariable v |
+    exists(LocalVariableLike v |
       type = v.getADeclaration().getTypeAnnotation() and
       value = v.getAnAssignedExpr()
     )
@@ -239,7 +239,7 @@ module TypeResolution {
       // ValueFlow::step is restricted to variables with at most one assignment. Allow the type annotation
       // of a variable to propagate to its uses, even if the variable has multiple assignments.
       type = decl.getTypeAnnotation() and
-      value = decl.getVariable().(LocalVariable).getAnAccess()
+      value = decl.getVariable().(LocalVariableLike).getAnAccess()
     )
     or
     exists(MemberDeclaration member |
diff --git a/javascript/ql/test/query-tests/Security/CWE-079/DomBasedXss/Xss.expected b/javascript/ql/test/query-tests/Security/CWE-079/DomBasedXss/Xss.expected
@@ -62,6 +62,8 @@
 | dragAndDrop.ts:73:29:73:39 | droppedHtml | dragAndDrop.ts:71:27:71:61 | e.dataT ... /html') | dragAndDrop.ts:73:29:73:39 | droppedHtml | Cross-site scripting vulnerability due to $@. | dragAndDrop.ts:71:27:71:61 | e.dataT ... /html') | user-provided value |
 | event-handler-receiver.js:2:31:2:83 | '<h2><a ... ></h2>' | event-handler-receiver.js:2:49:2:61 | location.href | event-handler-receiver.js:2:31:2:83 | '<h2><a ... ></h2>' | Cross-site scripting vulnerability due to $@. | event-handler-receiver.js:2:49:2:61 | location.href | user-provided value |
 | express.js:6:15:6:33 | req.param("wobble") | express.js:6:15:6:33 | req.param("wobble") | express.js:6:15:6:33 | req.param("wobble") | Cross-site scripting vulnerability due to $@. | express.js:6:15:6:33 | req.param("wobble") | user-provided value |
+| jquery-declare-any.ts:6:7:6:17 | window.name | jquery-declare-any.ts:6:7:6:17 | window.name | jquery-declare-any.ts:6:7:6:17 | window.name | Cross-site scripting vulnerability due to $@. | jquery-declare-any.ts:6:7:6:17 | window.name | user-provided value |
+| jquery-declare-type.ts:6:7:6:17 | window.name | jquery-declare-type.ts:6:7:6:17 | window.name | jquery-declare-type.ts:6:7:6:17 | window.name | Cross-site scripting vulnerability due to $@. | jquery-declare-type.ts:6:7:6:17 | window.name | user-provided value |
 | jquery.js:7:5:7:34 | "<div i ... + "\\">" | jquery.js:2:17:2:40 | documen ... .search | jquery.js:7:5:7:34 | "<div i ... + "\\">" | Cross-site scripting vulnerability due to $@. | jquery.js:2:17:2:40 | documen ... .search | user-provided value |
 | jquery.js:8:18:8:34 | "XSS: " + tainted | jquery.js:2:17:2:40 | documen ... .search | jquery.js:8:18:8:34 | "XSS: " + tainted | Cross-site scripting vulnerability due to $@. | jquery.js:2:17:2:40 | documen ... .search | user-provided value |
 | jquery.js:10:5:10:40 | "<b>" + ...  "</b>" | jquery.js:10:13:10:20 | location | jquery.js:10:5:10:40 | "<b>" + ...  "</b>" | Cross-site scripting vulnerability due to $@. | jquery.js:10:13:10:20 | location | user-provided value |
@@ -954,6 +956,8 @@ nodes
 | event-handler-receiver.js:2:31:2:83 | '<h2><a ... ></h2>' | semmle.label | '<h2><a ... ></h2>' |
 | event-handler-receiver.js:2:49:2:61 | location.href | semmle.label | location.href |
 | express.js:6:15:6:33 | req.param("wobble") | semmle.label | req.param("wobble") |
+| jquery-declare-any.ts:6:7:6:17 | window.name | semmle.label | window.name |
+| jquery-declare-type.ts:6:7:6:17 | window.name | semmle.label | window.name |
 | jquery.js:2:7:2:40 | tainted | semmle.label | tainted |
 | jquery.js:2:17:2:40 | documen ... .search | semmle.label | documen ... .search |
 | jquery.js:4:5:4:11 | tainted | semmle.label | tainted |
diff --git a/javascript/ql/test/query-tests/Security/CWE-079/DomBasedXss/XssWithAdditionalSources.expected b/javascript/ql/test/query-tests/Security/CWE-079/DomBasedXss/XssWithAdditionalSources.expected
@@ -182,6 +182,8 @@ nodes
 | hana.js:85:35:85:54 | tableRows[0].comment | semmle.label | tableRows[0].comment |
 | hana.js:90:33:90:34 | rs | semmle.label | rs |
 | hana.js:90:33:90:45 | rs[0].comment | semmle.label | rs[0].comment |
+| jquery-declare-any.ts:6:7:6:17 | window.name | semmle.label | window.name |
+| jquery-declare-type.ts:6:7:6:17 | window.name | semmle.label | window.name |
 | jquery.js:2:7:2:40 | tainted | semmle.label | tainted |
 | jquery.js:2:17:2:40 | documen ... .search | semmle.label | documen ... .search |
 | jquery.js:4:5:4:11 | tainted | semmle.label | tainted |
diff --git a/javascript/ql/test/query-tests/Security/CWE-079/DomBasedXss/jquery-declare-any.ts b/javascript/ql/test/query-tests/Security/CWE-079/DomBasedXss/jquery-declare-any.ts
@@ -3,5 +3,5 @@ import 'dummy';
 declare var $: any;
 
 function t() {
-    $(window.name); // $ MISSING: Alert
+    $(window.name); // $ Alert
 }
diff --git a/javascript/ql/test/query-tests/Security/CWE-079/DomBasedXss/jquery-declare-type.ts b/javascript/ql/test/query-tests/Security/CWE-079/DomBasedXss/jquery-declare-type.ts
@@ -3,5 +3,5 @@ import 'dummy';
 declare var $: JQueryStatic;
 
 function t() {
-    $(window.name); // $ MISSING: Alert
+    $(window.name); // $ Alert
 }

Original file line number	Diff line number	Diff line change
`@@ -190,7 +190,7 @@ module TypeResolution {`
`190`	`190`	`}`
`191`	`191`
`192`	`192`	`predicate contextualType(Node value, Node type) {`
`193`		`- exists(LocalVariable v \|`
	`193`	`+ exists(LocalVariableLike v \|`
`194`	`194`	`type = v.getADeclaration().getTypeAnnotation() and`
`195`	`195`	`value = v.getAnAssignedExpr()`
`196`	`196`	`)`
`@@ -239,7 +239,7 @@ module TypeResolution {`
`239`	`239`	`// ValueFlow::step is restricted to variables with at most one assignment. Allow the type annotation`
`240`	`240`	`// of a variable to propagate to its uses, even if the variable has multiple assignments.`
`241`	`241`	`type = decl.getTypeAnnotation() and`
`242`		`- value = decl.getVariable().(LocalVariable).getAnAccess()`
	`242`	`+ value = decl.getVariable().(LocalVariableLike).getAnAccess()`
`243`	`243`	`)`
`244`	`244`	`or`
`245`	`245`	`exists(MemberDeclaration member \|`
Original file line number	Diff line number	Diff line change
`@@ -3,5 +3,5 @@ import 'dummy';`
`3`	`3`	`declare var $: any;`
`4`	`4`
`5`	`5`	`function t() {`
`6`		`- $(window.name); // $ MISSING: Alert`
	`6`	`+ $(window.name); // $ Alert`
`7`	`7`	`}`
Original file line number	Diff line number	Diff line change
`@@ -3,5 +3,5 @@ import 'dummy';`
`3`	`3`	`declare var $: JQueryStatic;`
`4`	`4`
`5`	`5`	`function t() {`
`6`		`- $(window.name); // $ MISSING: Alert`
	`6`	`+ $(window.name); // $ Alert`
`7`	`7`	`}`