Merge pull request #3630 from erik-krogh/DevServer

Approved by asgerf
2026-05-01 03:35:13 +02:00 · 2020-06-22 12:59:13 +01:00
parent 287bc40264 e46bd709c4
commit 7a5aae7432
9 changed files with 100 additions and 2 deletions
--- a/change-notes/1.25/analysis-javascript.md
+++ b/change-notes/1.25/analysis-javascript.md
@@ -20,6 +20,7 @@
  - [sqlite](https://www.npmjs.com/package/sqlite)
  - [ssh2-streams](https://www.npmjs.com/package/ssh2-streams)
  - [ssh2](https://www.npmjs.com/package/ssh2)
+  - [webpack-dev-server](https://www.npmjs.com/package/webpack-dev-server)

 * TypeScript 3.9 is now supported.

--- a/javascript/ql/src/semmle/javascript/Modules.qll
+++ b/javascript/ql/src/semmle/javascript/Modules.qll
@@ -174,7 +174,8 @@ abstract class Import extends ASTNode {
      result = resolveAsProvidedModule() or
      result = resolveImportedPath() or
      result = resolveFromTypeRoot() or
-      result = resolveFromTypeScriptSymbol()
+      result = resolveFromTypeScriptSymbol() or
+      result = resolveNeighbourPackage(this.getImportedPath().getValue())
    )
  }

@@ -196,3 +197,28 @@ abstract deprecated class PathExprInModule extends PathExpr {
    this.(Comment).getTopLevel() instanceof Module
  }
 }
+
+/**
+ * Gets a module imported from another package in the same repository.
+ *
+ * No support for importing from folders inside the other package.
+ */
+private Module resolveNeighbourPackage(PathString importPath) {
+  exists(PackageJSON json | importPath = json.getPackageName() and result = json.getMainModule())
+  or
+  exists(string package |
+    result.getFile().getParentContainer() = getPackageFolder(package) and
+    importPath = package + "/" + [result.getFile().getBaseName(), result.getFile().getStem()]
+  )
+}
+
+/**
+ * Gets the folder for a package that has name `package` according to a package.json file in the resulting folder.
+ */
+pragma[noinline]
+private Folder getPackageFolder(string package) {
+  exists(PackageJSON json |
+    json.getPackageName() = package and
+    result = json.getFile().getParentContainer()
+  )
+}
--- a/javascript/ql/src/semmle/javascript/frameworks/Express.qll
+++ b/javascript/ql/src/semmle/javascript/frameworks/Express.qll
@@ -44,6 +44,9 @@ module Express {
    isRouter(e, _)
    or
    e.getType().hasUnderlyingType("express", "Router")
+    or
+    // created by `webpack-dev-server`
+    WebpackDevServer::webpackDevServerApp().flowsToExpr(e)
  }

  /**
@@ -903,4 +906,32 @@ module Express {

    override DataFlow::ValueNode getARouteHandlerArg() { result = routeHandlerArg }
  }
+
+  private module WebpackDevServer {
+    /**
+     * Gets a source for the options given to an instantiation of `webpack-dev-server`.
+     */
+    private DataFlow::SourceNode devServerOptions(DataFlow::TypeBackTracker t) {
+      t.start() and
+      result =
+        DataFlow::moduleImport("webpack-dev-server")
+            .getAnInstantiation()
+            .getArgument(1)
+            .getALocalSource()
+      or
+      exists(DataFlow::TypeBackTracker t2 | result = devServerOptions(t2).backtrack(t2, t))
+    }
+
+    /**
+     * Gets an instance of the `express` app created by `webpack-dev-server`.
+     */
+    DataFlow::ParameterNode webpackDevServerApp() {
+      result =
+        devServerOptions(DataFlow::TypeBackTracker::end())
+            .getAPropertyWrite(["after", "before", "setup"])
+            .getRhs()
+            .getAFunctionValue()
+            .getParameter(0)
+    }
+  }
 }
--- a/javascript/ql/test/library-tests/Portals/PortalEntry.expected
+++ b/javascript/ql/test/library-tests/Portals/PortalEntry.expected
@@ -711,12 +711,14 @@
 | (parameter 0 (member default (root https://www.npmjs.com/package/m2))) | src/m3/tst3.js:5:7:5:10 | "me" | false |
 | (parameter 0 (member foo (root https://www.npmjs.com/package/m2))) | src/m3/tst2.js:5:5:5:12 | { x: o } | false |
 | (parameter 0 (member m (instance (member default (root https://www.npmjs.com/package/m2))))) | src/m3/tst3.js:4:15:4:18 | "hi" | false |
+| (parameter 0 (member m (instance (member default (root https://www.npmjs.com/package/m2))))) | src/m3/tst3.js:4:15:4:18 | "hi" | true |
 | (parameter 0 (member m (instance (root https://www.npmjs.com/package/m2)))) | src/m3/tst3.js:4:15:4:18 | "hi" | false |
 | (parameter 0 (member m (member default (root https://www.npmjs.com/package/m2)))) | src/m3/tst3.js:2:5:2:8 | "hi" | false |
 | (parameter 0 (member m (return (member default (root https://www.npmjs.com/package/m2))))) | src/m3/tst3.js:4:15:4:18 | "hi" | false |
 | (parameter 0 (member m (return (root https://www.npmjs.com/package/m2)))) | src/m3/tst3.js:4:15:4:18 | "hi" | false |
 | (parameter 0 (member m (root https://www.npmjs.com/package/m2))) | src/m3/tst3.js:2:5:2:8 | "hi" | false |
 | (parameter 0 (member s (instance (member default (root https://www.npmjs.com/package/m2))))) | src/m3/tst3.js:5:15:5:21 | "there" | false |
+| (parameter 0 (member s (instance (member default (root https://www.npmjs.com/package/m2))))) | src/m3/tst3.js:5:15:5:21 | "there" | true |
 | (parameter 0 (member s (instance (root https://www.npmjs.com/package/m2)))) | src/m3/tst3.js:5:15:5:21 | "there" | false |
 | (parameter 0 (member s (member default (root https://www.npmjs.com/package/m2)))) | src/m3/tst3.js:3:5:3:11 | "there" | false |
 | (parameter 0 (member s (return (member default (root https://www.npmjs.com/package/m2))))) | src/m3/tst3.js:5:15:5:21 | "there" | false |
--- a/javascript/ql/test/library-tests/Portals/PortalExit.expected
+++ b/javascript/ql/test/library-tests/Portals/PortalExit.expected
@@ -6,12 +6,15 @@
 | (instance (member default (root https://www.npmjs.com/package/m2))) | src/m2/main.js:11:4:11:3 | this | true |
 | (instance (member default (root https://www.npmjs.com/package/m2))) | src/m2/main.js:15:11:15:10 | this | true |
 | (instance (member default (root https://www.npmjs.com/package/m2))) | src/m3/tst3.js:4:1:4:11 | new A("me") | false |
+| (instance (member default (root https://www.npmjs.com/package/m2))) | src/m3/tst3.js:4:1:4:11 | new A("me") | true |
 | (instance (member default (root https://www.npmjs.com/package/m2))) | src/m3/tst3.js:5:1:5:11 | new A("me") | false |
+| (instance (member default (root https://www.npmjs.com/package/m2))) | src/m3/tst3.js:5:1:5:11 | new A("me") | true |
 | (instance (root https://www.npmjs.com/package/m2)) | src/m3/tst3.js:4:1:4:11 | new A("me") | false |
 | (instance (root https://www.npmjs.com/package/m2)) | src/m3/tst3.js:5:1:5:11 | new A("me") | false |
 | (member default (root https://www.npmjs.com/package/m2)) | src/m3/tst3.js:1:8:1:8 | A | false |
 | (member foo (root https://www.npmjs.com/package/m2)) | src/m3/tst2.js:1:10:1:12 | foo | false |
 | (member m (instance (member default (root https://www.npmjs.com/package/m2)))) | src/m3/tst3.js:4:1:4:13 | new A("me").m | false |
+| (member m (instance (member default (root https://www.npmjs.com/package/m2)))) | src/m3/tst3.js:4:1:4:13 | new A("me").m | true |
 | (member m (instance (root https://www.npmjs.com/package/m2))) | src/m3/tst3.js:4:1:4:13 | new A("me").m | false |
 | (member m (member default (root https://www.npmjs.com/package/m2))) | src/m3/tst3.js:2:1:2:3 | A.m | false |
 | (member m (return (member default (root https://www.npmjs.com/package/m2)))) | src/m3/tst3.js:4:1:4:13 | new A("me").m | false |
@@ -19,6 +22,7 @@
 | (member m (root https://www.npmjs.com/package/m2)) | src/m3/tst3.js:2:1:2:3 | A.m | false |
 | (member name (instance (member default (root https://www.npmjs.com/package/m2)))) | src/m2/main.js:12:27:12:35 | this.name | true |
 | (member s (instance (member default (root https://www.npmjs.com/package/m2)))) | src/m3/tst3.js:5:1:5:13 | new A("me").s | false |
+| (member s (instance (member default (root https://www.npmjs.com/package/m2)))) | src/m3/tst3.js:5:1:5:13 | new A("me").s | true |
 | (member s (instance (root https://www.npmjs.com/package/m2))) | src/m3/tst3.js:5:1:5:13 | new A("me").s | false |
 | (member s (member default (root https://www.npmjs.com/package/m2))) | src/m3/tst3.js:3:1:3:3 | A.s | false |
 | (member s (return (member default (root https://www.npmjs.com/package/m2)))) | src/m3/tst3.js:5:1:5:13 | new A("me").s | false |
@@ -734,12 +738,14 @@
 | (return (member default (root https://www.npmjs.com/package/m2))) | src/m3/tst3.js:5:1:5:11 | new A("me") | false |
 | (return (member foo (root https://www.npmjs.com/package/m2))) | src/m3/tst2.js:5:1:5:13 | foo({ x: o }) | false |
 | (return (member m (instance (member default (root https://www.npmjs.com/package/m2))))) | src/m3/tst3.js:4:1:4:19 | new A("me").m("hi") | false |
+| (return (member m (instance (member default (root https://www.npmjs.com/package/m2))))) | src/m3/tst3.js:4:1:4:19 | new A("me").m("hi") | true |
 | (return (member m (instance (root https://www.npmjs.com/package/m2)))) | src/m3/tst3.js:4:1:4:19 | new A("me").m("hi") | false |
 | (return (member m (member default (root https://www.npmjs.com/package/m2)))) | src/m3/tst3.js:2:1:2:9 | A.m("hi") | false |
 | (return (member m (return (member default (root https://www.npmjs.com/package/m2))))) | src/m3/tst3.js:4:1:4:19 | new A("me").m("hi") | false |
 | (return (member m (return (root https://www.npmjs.com/package/m2)))) | src/m3/tst3.js:4:1:4:19 | new A("me").m("hi") | false |
 | (return (member m (root https://www.npmjs.com/package/m2))) | src/m3/tst3.js:2:1:2:9 | A.m("hi") | false |
 | (return (member s (instance (member default (root https://www.npmjs.com/package/m2))))) | src/m3/tst3.js:5:1:5:22 | new A(" ... there") | false |
+| (return (member s (instance (member default (root https://www.npmjs.com/package/m2))))) | src/m3/tst3.js:5:1:5:22 | new A(" ... there") | true |
 | (return (member s (instance (root https://www.npmjs.com/package/m2)))) | src/m3/tst3.js:5:1:5:22 | new A(" ... there") | false |
 | (return (member s (member default (root https://www.npmjs.com/package/m2)))) | src/m3/tst3.js:3:1:3:12 | A.s("there") | false |
 | (return (member s (return (member default (root https://www.npmjs.com/package/m2))))) | src/m3/tst3.js:5:1:5:22 | new A(" ... there") | false |
--- a/javascript/ql/test/query-tests/Security/CWE-078/CommandInjection.expected
+++ b/javascript/ql/test/query-tests/Security/CWE-078/CommandInjection.expected
@@ -48,6 +48,11 @@ nodes
 | child_process-test.js:70:25:70:31 | req.url |
 | child_process-test.js:72:29:72:31 | cmd |
 | child_process-test.js:72:29:72:31 | cmd |
+| child_process-test.js:80:19:80:36 | req.query.fileName |
+| child_process-test.js:80:19:80:36 | req.query.fileName |
+| child_process-test.js:80:19:80:36 | req.query.fileName |
+| child_process-test.js:82:37:82:54 | req.query.fileName |
+| child_process-test.js:82:37:82:54 | req.query.fileName |
 | execSeries.js:3:20:3:22 | arr |
 | execSeries.js:6:14:6:16 | arr |
 | execSeries.js:6:14:6:21 | arr[i++] |
@@ -64,6 +69,10 @@ nodes
 | execSeries.js:18:34:18:40 | req.url |
 | execSeries.js:19:12:19:16 | [cmd] |
 | execSeries.js:19:13:19:15 | cmd |
+| lib/subLib/index.js:7:32:7:35 | name |
+| lib/subLib/index.js:8:10:8:25 | "rm -rf " + name |
+| lib/subLib/index.js:8:10:8:25 | "rm -rf " + name |
+| lib/subLib/index.js:8:22:8:25 | name |
 | other.js:5:9:5:49 | cmd |
 | other.js:5:15:5:38 | url.par ... , true) |
 | other.js:5:15:5:44 | url.par ... ).query |
@@ -152,6 +161,9 @@ edges
 | child_process-test.js:70:15:70:49 | url.par ... ry.path | child_process-test.js:70:9:70:49 | cmd |
 | child_process-test.js:70:25:70:31 | req.url | child_process-test.js:70:15:70:38 | url.par ... , true) |
 | child_process-test.js:70:25:70:31 | req.url | child_process-test.js:70:15:70:38 | url.par ... , true) |
+| child_process-test.js:80:19:80:36 | req.query.fileName | child_process-test.js:80:19:80:36 | req.query.fileName |
+| child_process-test.js:82:37:82:54 | req.query.fileName | lib/subLib/index.js:7:32:7:35 | name |
+| child_process-test.js:82:37:82:54 | req.query.fileName | lib/subLib/index.js:7:32:7:35 | name |
 | execSeries.js:3:20:3:22 | arr | execSeries.js:6:14:6:16 | arr |
 | execSeries.js:6:14:6:16 | arr | execSeries.js:6:14:6:21 | arr[i++] |
 | execSeries.js:6:14:6:21 | arr[i++] | execSeries.js:14:24:14:30 | command |
@@ -168,6 +180,9 @@ edges
 | execSeries.js:18:34:18:40 | req.url | execSeries.js:18:13:18:47 | require ... , true) |
 | execSeries.js:19:12:19:16 | [cmd] | execSeries.js:13:19:13:26 | commands |
 | execSeries.js:19:13:19:15 | cmd | execSeries.js:19:12:19:16 | [cmd] |
+| lib/subLib/index.js:7:32:7:35 | name | lib/subLib/index.js:8:22:8:25 | name |
+| lib/subLib/index.js:8:22:8:25 | name | lib/subLib/index.js:8:10:8:25 | "rm -rf " + name |
+| lib/subLib/index.js:8:22:8:25 | name | lib/subLib/index.js:8:10:8:25 | "rm -rf " + name |
 | other.js:5:9:5:49 | cmd | other.js:7:33:7:35 | cmd |
 | other.js:5:9:5:49 | cmd | other.js:7:33:7:35 | cmd |
 | other.js:5:9:5:49 | cmd | other.js:8:28:8:30 | cmd |
@@ -228,7 +243,9 @@ edges
 | child_process-test.js:59:5:59:39 | cp.exec ... , args) | child_process-test.js:6:25:6:31 | req.url | child_process-test.js:50:15:50:17 | cmd | This command depends on $@. | child_process-test.js:6:25:6:31 | req.url | a user-provided value |
 | child_process-test.js:64:3:64:21 | cp.spawn(cmd, args) | child_process-test.js:6:25:6:31 | req.url | child_process-test.js:43:15:43:17 | cmd | This command depends on $@. | child_process-test.js:6:25:6:31 | req.url | a user-provided value |
 | child_process-test.js:72:29:72:31 | cmd | child_process-test.js:70:25:70:31 | req.url | child_process-test.js:72:29:72:31 | cmd | This command depends on $@. | child_process-test.js:70:25:70:31 | req.url | a user-provided value |
+| child_process-test.js:80:19:80:36 | req.query.fileName | child_process-test.js:80:19:80:36 | req.query.fileName | child_process-test.js:80:19:80:36 | req.query.fileName | This command depends on $@. | child_process-test.js:80:19:80:36 | req.query.fileName | a user-provided value |
 | execSeries.js:14:41:14:47 | command | execSeries.js:18:34:18:40 | req.url | execSeries.js:14:41:14:47 | command | This command depends on $@. | execSeries.js:18:34:18:40 | req.url | a user-provided value |
+| lib/subLib/index.js:8:10:8:25 | "rm -rf " + name | child_process-test.js:82:37:82:54 | req.query.fileName | lib/subLib/index.js:8:10:8:25 | "rm -rf " + name | This command depends on $@. | child_process-test.js:82:37:82:54 | req.query.fileName | a user-provided value |
 | other.js:7:33:7:35 | cmd | other.js:5:25:5:31 | req.url | other.js:7:33:7:35 | cmd | This command depends on $@. | other.js:5:25:5:31 | req.url | a user-provided value |
 | other.js:8:28:8:30 | cmd | other.js:5:25:5:31 | req.url | other.js:8:28:8:30 | cmd | This command depends on $@. | other.js:5:25:5:31 | req.url | a user-provided value |
 | other.js:9:32:9:34 | cmd | other.js:5:25:5:31 | req.url | other.js:9:32:9:34 | cmd | This command depends on $@. | other.js:5:25:5:31 | req.url | a user-provided value |
--- a/javascript/ql/test/query-tests/Security/CWE-078/child_process-test.js
+++ b/javascript/ql/test/query-tests/Security/CWE-078/child_process-test.js
@@ -72,3 +72,14 @@ http.createServer(function(req, res) {
    util.promisify(cp.exec)(cmd); // NOT OK
 });

+
+const webpackDevServer = require('webpack-dev-server');
+new webpackDevServer(compiler, {
+    before: function (app) {
+        app.use(function (req, res, next) {
+          cp.exec(req.query.fileName); // NOT OK
+
+          require("my-sub-lib").foo(req.query.fileName); // calls lib/subLib/index.js#foo
+        });
+    }
+});
--- a/javascript/ql/test/query-tests/Security/CWE-078/lib/subLib/index.js
+++ b/javascript/ql/test/query-tests/Security/CWE-078/lib/subLib/index.js
@@ -2,4 +2,8 @@ var cp = require("child_process")

 module.exports = function (name) {
 	cp.exec("rm -rf " + name); // OK - this file belongs in a sub-"module", and is not the primary exported module.
+};
+
+module.exports.foo = function (name) {
+	cp.exec("rm -rf " + name); // NOT OK - this is being called explicitly from child_process-test.js
 };
--- a/javascript/ql/test/query-tests/Security/CWE-078/lib/subLib/package.json
+++ b/javascript/ql/test/query-tests/Security/CWE-078/lib/subLib/package.json
@@ -1,5 +1,5 @@
 {
-  "name": "mySubLib",
+  "name": "my-sub-lib",
  "version": "0.0.7",
  "main": "./index.js"
 }