Merge pull request #18670 from asgerf/js/test-suite

JS: Update test suite to use post-processed inline expectations

Author: asgerf

javascript/ql/lib/semmle/javascript/frameworks/SQL.qll

@@ -221,7 +221,10 @@ private module Postgres {
 
     /** Gets a value that is plugged into a raw placeholder variable, making it a sink for SQL injection. */
     private DataFlow::Node getARawValue() {
-      result = this.getValues() and this.getARawParameterName() = "1" // Special case: if the argument is not an array or object, it's just plugged into $1
+      result = this.getValues() and
+      this.getARawParameterName() = "1" and // Special case: if the argument is not an array or object, it's just plugged into $1
+      not result instanceof DataFlow::ArrayCreationNode and
+      not result instanceof DataFlow::ObjectLiteralNode
       or
       exists(DataFlow::SourceNode values | values = this.getValues().getALocalSource() |
         result = values.getAPropertyWrite(this.getARawParameterName()).getRhs()

javascript/ql/lib/semmle/javascript/frameworks/UriLibraries.qll

@@ -421,3 +421,22 @@ private module ClosureLibraryUri {
     }
   }
 }
+
+private class QueryStringStringification extends DataFlow::SummarizedCallable {
+  QueryStringStringification() { this = "query-string stringification" }
+
+  override DataFlow::InvokeNode getACall() {
+    result =
+      API::moduleImport(["querystring", "query-string", "querystringify", "qs"])
+          .getMember("stringify")
+          .getACall() or
+    result = API::moduleImport("url-parse").getMember("qs").getMember("stringify").getACall() or
+    result = API::moduleImport("parseqs").getMember("encode").getACall()
+  }
+
+  override predicate propagatesFlow(string input, string output, boolean preservesValue) {
+    preservesValue = false and
+    input = ["Argument[0]", "Argument[0].AnyMemberDeep"] and
+    output = "ReturnValue"
+  }
+}

javascript/ql/lib/semmle/javascript/security/dataflow/ServerSideUrlRedirectQuery.qll

@@ -20,7 +20,11 @@ module ServerSideUrlRedirectConfig implements DataFlow::ConfigSig {
 
   predicate isSink(DataFlow::Node sink) { sink instanceof Sink }
 
-  predicate isBarrier(DataFlow::Node node) { node instanceof Sanitizer }
+  predicate isBarrier(DataFlow::Node node) {
+    node instanceof Sanitizer
+    or
+    node = HostnameSanitizerGuard::getABarrierNode()
+  }
 
   predicate isBarrierOut(DataFlow::Node node) { hostnameSanitizingPrefixEdge(node, _) }
 
@@ -69,10 +73,12 @@ deprecated class Configuration extends TaintTracking::Configuration {
 }
 
 /**
+ * DEPRECATED. This is no longer used as a sanitizer guard.
+ *
  * A call to a function called `isLocalUrl` or similar, which is
  * considered to sanitize a variable for purposes of URL redirection.
  */
-class LocalUrlSanitizingGuard extends DataFlow::CallNode {
+deprecated class LocalUrlSanitizingGuard extends DataFlow::CallNode {
   LocalUrlSanitizingGuard() { this.getCalleeName().regexpMatch("(?i)(is_?)?local_?url") }
 
   /** DEPRECATED. Use `blocksExpr` instead. */

javascript/ql/lib/utils/test/internal/InlineExpectationsTestImpl.qll

@@ -4,14 +4,22 @@ private import codeql.util.test.InlineExpectationsTest
 module Impl implements InlineExpectationsTestSig {
   private import javascript
 
-  final private class LineCommentFinal = LineComment;
+  final class ExpectationComment = ExpectationCommentImpl;
 
-  class ExpectationComment extends LineCommentFinal {
-    string getContents() { result = this.getText() }
+  class Location = JS::Location;
+
+  abstract private class ExpectationCommentImpl extends Locatable {
+    abstract string getContents();
 
     /** Gets this element's location. */
     Location getLocation() { result = super.getLocation() }
   }
 
-  class Location = JS::Location;
+  private class JSComment extends ExpectationCommentImpl instanceof Comment {
+    override string getContents() { result = super.getText() }
+  }
+
+  private class HtmlComment extends ExpectationCommentImpl instanceof HTML::CommentNode {
+    override string getContents() { result = super.getText() }
+  }
 }

javascript/ql/src/change-notes/2025-02-21-test-suite.md

@@ -0,0 +1,5 @@
+---
+category: fix
+---
+* Fixed a recently-introduced bug that caused `js/server-side-unvalidated-url-redirection` to ignore
+  valid hostname checks and report spurious alerts after such a check. The original behaviour has been restored.

javascript/ql/test/query-tests/AngularJS/DeadAngularJSEventListener/DeadAngularJSEventListener.qlref

@@ -1 +1,2 @@
-AngularJS/DeadAngularJSEventListener.ql
+query: AngularJS/DeadAngularJSEventListener.ql
+postprocess: utils/test/InlineExpectationsTestQuery.ql

javascript/ql/test/query-tests/AngularJS/DeadAngularJSEventListener/tst.js

@@ -1,50 +1,50 @@
 angular.module('myModule', [])
     .controller('MyController', function($scope) {
-        $scope.$on('destroy', cleanup); // BAD
+        $scope.$on('destroy', cleanup); // $ Alert
     })
     .controller('MyController', ["$scope", function(s) {
-        s.$on('destroy', cleanup); // BAD
+        s.$on('destroy', cleanup); // $ Alert
     }])
     .controller('MyController', function($scope) {
         var destroy = 'destroy';
-        $scope.$on(destroy, cleanup); // BAD
+        $scope.$on(destroy, cleanup); // $ Alert
     })
     .controller('MyController', function($scope) {
-        $scope.$on('$destroy', cleanup); // GOOD
+        $scope.$on('$destroy', cleanup);
     })
     .controller('MyController', function($scope) {
         $scope.$emit('foo');
-        $scope.$on('foo', cleanup); // GOOD
+        $scope.$on('foo', cleanup);
     })
     .controller('MyController', function($scope) {
-        $scope.$on('bar', cleanup); // BAD
+        $scope.$on('bar', cleanup); // $ Alert
     })
     .controller('MyController', function($scope) {
-        $scope.$on('$locationChangeStart', cleanup); // OK
+        $scope.$on('$locationChangeStart', cleanup);
     })
     .controller('MyController', function($scope) {
-        $scope.$on('lib1.foo', cleanup); // OK
+        $scope.$on('lib1.foo', cleanup);
     })
     .controller('MyController', function($scope) {
-        $scope.$on('lib2:foo', cleanup); // OK
+        $scope.$on('lib2:foo', cleanup);
     })
     .controller('MyController', function($scope) {
-        $scope.$on('onClick', cleanup); // OK
+        $scope.$on('onClick', cleanup);
     })
     .controller('MyController', function($scope) {
         function f($scope){
             $scope.$emit('probablyFromUserCode1')
         }
-        $scope.$on('probablyFromUserCode1', cleanup); // OK
+        $scope.$on('probablyFromUserCode1', cleanup);
     })
     .controller('MyController', function($scope) {
         function f($scope){
             var scope = $scope;
             scope.$emit('probablyFromUserCode2')
         }
-        $scope.$on('probablyFromUserCode2', cleanup); // OK
+        $scope.$on('probablyFromUserCode2', cleanup);
     })
     .controller('MyController', function($scope) {
-        $scope.$on('event-from-AngularJS-expression', cleanup); // GOOD
+        $scope.$on('event-from-AngularJS-expression', cleanup);
     })
 ;

javascript/ql/test/query-tests/AngularJS/DependencyMismatch/DependencyMismatch.qlref

@@ -1 +1,2 @@
-AngularJS/DependencyMismatch.ql
+query: AngularJS/DependencyMismatch.ql
+postprocess: utils/test/InlineExpectationsTestQuery.ql

javascript/ql/test/query-tests/AngularJS/DependencyMismatch/tst.js

@@ -1,36 +1,36 @@
 angular.module('app1', [])
-       .run(['dep1', 'dep2', 'dep3', function(dep1, dep3, dep2) {}]);  // NOT OK
+       .run(['dep1', 'dep2', 'dep3', function(dep1, dep3, dep2) {}]);  // $ Alert
 
 angular.module('app2')
-       .directive('mydirective', [ '$compile', function($compile, $http) { // NOT OK
+       .directive('mydirective', [ '$compile', function($compile, $http) { // $ Alert
            // ...
        }]);
 
 angular.module('app1', [])
-       .run(['dep1', 'dep2', 'dep3', function(dep1, dep2, dep3) {}]);  // OK
+       .run(['dep1', 'dep2', 'dep3', function(dep1, dep2, dep3) {}]);
 
 angular.module('app2')
-       .directive('mydirective', [ '$compile', '$http', function($compile, $http) { // OK
+       .directive('mydirective', [ '$compile', '$http', function($compile, $http) {
            // ...
        }]);
 
 angular.module('app3', [])
-       .run(function(dep1, dep3) {});  // OK
+       .run(function(dep1, dep3) {});
 
 angular.module('app4')
-       .directive('mydirective', function($compile, $http) {  // OK
+       .directive('mydirective', function($compile, $http) {
            // ...
        });
 
 angular.module('app5')
-       .directive('mydirective', [ 'fully.qualified.name', function(name) { // OK
+       .directive('mydirective', [ 'fully.qualified.name', function(name) {
            // ...
        }])
 
 angular.module('app6')
     .directive('mydirective', function() {
         return {
-            link: function (scope, element, attrs) { // OK
+            link: function (scope, element, attrs) {
             }
         };
     });

javascript/ql/test/query-tests/AngularJS/DisablingSce/DisablingSce.js

@@ -1,17 +1,17 @@
 angular.module('app', [])
     .config(function($sceProvider) {
-        $sceProvider.enabled(false); // BAD
+        $sceProvider.enabled(false); // $ Alert
     })
     .config(['otherProvider', function($sceProvider) {
-        $sceProvider.enabled(false); // OK
+        $sceProvider.enabled(false);
     }])
     .config(['$sceProvider', function(x) {
-        x.enabled(false); // BAD
+        x.enabled(false); // $ Alert
     }])
     .config(function($sceProvider) {
-        $sceProvider.enabled(true); // OK
+        $sceProvider.enabled(true);
     })
     .config(function($sceProvider) {
         var x = false;
-        $sceProvider.enabled(x); // BAD
+        $sceProvider.enabled(x); // $ Alert
     });