Merge pull request #18964 from Napalys/js/mark_down_table

Napalys · web-flow · commit 7c9edff33cd7 · 2025-03-11T09:02:56.000+01:00
JS: Refactor `markdown-table` library modeling
diff --git a/javascript/ql/lib/change-notes/2025-03-10-js-refactor-markdown-table.md b/javascript/ql/lib/change-notes/2025-03-10-js-refactor-markdown-table.md
@@ -0,0 +1,4 @@
+---
+category: minorAnalysis
+---
+* Improved the modeling of the `markdown-table` package to ensure it handles nested arrays properly.
diff --git a/javascript/ql/lib/ext/markdown-table.model.yml b/javascript/ql/lib/ext/markdown-table.model.yml
@@ -0,0 +1,6 @@
+extensions:
+  - addsTo:
+      pack: codeql/javascript-all
+      extensible: summaryModel
+    data:
+      - ["markdown-table", "", "Argument[0].ArrayElement.ArrayElement", "ReturnValue", "taint"]
diff --git a/javascript/ql/lib/semmle/javascript/frameworks/Markdown.qll b/javascript/ql/lib/semmle/javascript/frameworks/Markdown.qll
@@ -46,19 +46,6 @@ module Markdown {
     }
   }
 
-  /**
-   * A taint step for the `markdown-table` library.
-   */
-  private class MarkdownTableStep extends MarkdownStep {
-    override predicate step(DataFlow::Node pred, DataFlow::Node succ) {
-      exists(DataFlow::CallNode call | call = DataFlow::moduleImport("markdown-table").getACall() |
-        // TODO: needs a flow summary to ensure ArrayElement content is unfolded
-        succ = call and
-        pred = call.getArgument(0)
-      )
-    }
-  }
-
   /**
    * A taint step for the `showdown` library.
    */
diff --git a/javascript/ql/test/query-tests/Security/CWE-079/ReflectedXss/ReflectedXss.expected b/javascript/ql/test/query-tests/Security/CWE-079/ReflectedXss/ReflectedXss.expected
@@ -2,6 +2,9 @@ edges
 | ReflectedXss.js:8:33:8:45 | req.params.id | ReflectedXss.js:8:14:8:45 | "Unknow ... rams.id | provenance |  |
 | ReflectedXss.js:17:31:17:39 | params.id | ReflectedXss.js:17:12:17:39 | "Unknow ... rams.id | provenance |  |
 | ReflectedXss.js:23:19:23:26 | req.body | ReflectedXss.js:23:12:23:27 | marked(req.body) | provenance |  |
+| ReflectedXss.js:30:7:33:4 | mytable | ReflectedXss.js:34:12:34:18 | mytable | provenance |  |
+| ReflectedXss.js:30:17:33:4 | table([ ... y]\\n  ]) | ReflectedXss.js:30:7:33:4 | mytable | provenance |  |
+| ReflectedXss.js:32:14:32:21 | req.body | ReflectedXss.js:30:17:33:4 | table([ ... y]\\n  ]) | provenance |  |
 | ReflectedXss.js:42:31:42:38 | req.body | ReflectedXss.js:42:12:42:39 | convert ... q.body) | provenance |  |
 | ReflectedXss.js:64:14:64:21 | req.body | ReflectedXss.js:64:39:64:42 | file | provenance |  |
 | ReflectedXss.js:64:39:64:42 | file | ReflectedXss.js:65:16:65:19 | file | provenance |  |
@@ -152,6 +155,10 @@ nodes
 | ReflectedXss.js:23:12:23:27 | marked(req.body) | semmle.label | marked(req.body) |
 | ReflectedXss.js:23:19:23:26 | req.body | semmle.label | req.body |
 | ReflectedXss.js:29:12:29:19 | req.body | semmle.label | req.body |
+| ReflectedXss.js:30:7:33:4 | mytable | semmle.label | mytable |
+| ReflectedXss.js:30:17:33:4 | table([ ... y]\\n  ]) | semmle.label | table([ ... y]\\n  ]) |
+| ReflectedXss.js:32:14:32:21 | req.body | semmle.label | req.body |
+| ReflectedXss.js:34:12:34:18 | mytable | semmle.label | mytable |
 | ReflectedXss.js:41:12:41:19 | req.body | semmle.label | req.body |
 | ReflectedXss.js:42:12:42:39 | convert ... q.body) | semmle.label | convert ... q.body) |
 | ReflectedXss.js:42:31:42:38 | req.body | semmle.label | req.body |
@@ -340,6 +347,7 @@ subpaths
 | ReflectedXss.js:22:12:22:19 | req.body | ReflectedXss.js:22:12:22:19 | req.body | ReflectedXss.js:22:12:22:19 | req.body | Cross-site scripting vulnerability due to a $@. | ReflectedXss.js:22:12:22:19 | req.body | user-provided value |
 | ReflectedXss.js:23:12:23:27 | marked(req.body) | ReflectedXss.js:23:19:23:26 | req.body | ReflectedXss.js:23:12:23:27 | marked(req.body) | Cross-site scripting vulnerability due to a $@. | ReflectedXss.js:23:19:23:26 | req.body | user-provided value |
 | ReflectedXss.js:29:12:29:19 | req.body | ReflectedXss.js:29:12:29:19 | req.body | ReflectedXss.js:29:12:29:19 | req.body | Cross-site scripting vulnerability due to a $@. | ReflectedXss.js:29:12:29:19 | req.body | user-provided value |
+| ReflectedXss.js:34:12:34:18 | mytable | ReflectedXss.js:32:14:32:21 | req.body | ReflectedXss.js:34:12:34:18 | mytable | Cross-site scripting vulnerability due to a $@. | ReflectedXss.js:32:14:32:21 | req.body | user-provided value |
 | ReflectedXss.js:41:12:41:19 | req.body | ReflectedXss.js:41:12:41:19 | req.body | ReflectedXss.js:41:12:41:19 | req.body | Cross-site scripting vulnerability due to a $@. | ReflectedXss.js:41:12:41:19 | req.body | user-provided value |
 | ReflectedXss.js:42:12:42:39 | convert ... q.body) | ReflectedXss.js:42:31:42:38 | req.body | ReflectedXss.js:42:12:42:39 | convert ... q.body) | Cross-site scripting vulnerability due to a $@. | ReflectedXss.js:42:31:42:38 | req.body | user-provided value |
 | ReflectedXss.js:56:12:56:19 | req.body | ReflectedXss.js:56:12:56:19 | req.body | ReflectedXss.js:56:12:56:19 | req.body | Cross-site scripting vulnerability due to a $@. | ReflectedXss.js:56:12:56:19 | req.body | user-provided value |
diff --git a/javascript/ql/test/query-tests/Security/CWE-079/ReflectedXss/ReflectedXss.js b/javascript/ql/test/query-tests/Security/CWE-079/ReflectedXss/ReflectedXss.js
@@ -31,7 +31,7 @@ app.get('/user/:id', function(req, res) {
     ['Name', 'Content'],
     ['body', req.body]
   ]);
-  res.send(mytable); // NOT OK - FIXME: only works in OLD dataflow, add implicit reads before library-contributed taint steps
+  res.send(mytable); // NOT OK
 });
 
 var showdown  = require('showdown');
@@ -122,4 +122,4 @@ app.get("invalid/keys/:id", async (req, res) => {
         res.status(400).send(`${invalidKeys.join(', ')} not in whitelist`);
         return;
     }
-});
+});
diff --git a/javascript/ql/test/query-tests/Security/CWE-079/ReflectedXss/ReflectedXssWithCustomSanitizer.expected b/javascript/ql/test/query-tests/Security/CWE-079/ReflectedXss/ReflectedXssWithCustomSanitizer.expected
@@ -3,6 +3,7 @@
 | ReflectedXss.js:22:12:22:19 | req.body | Cross-site scripting vulnerability due to $@. | ReflectedXss.js:22:12:22:19 | req.body | user-provided value |
 | ReflectedXss.js:23:12:23:27 | marked(req.body) | Cross-site scripting vulnerability due to $@. | ReflectedXss.js:23:19:23:26 | req.body | user-provided value |
 | ReflectedXss.js:29:12:29:19 | req.body | Cross-site scripting vulnerability due to $@. | ReflectedXss.js:29:12:29:19 | req.body | user-provided value |
+| ReflectedXss.js:34:12:34:18 | mytable | Cross-site scripting vulnerability due to $@. | ReflectedXss.js:32:14:32:21 | req.body | user-provided value |
 | ReflectedXss.js:41:12:41:19 | req.body | Cross-site scripting vulnerability due to $@. | ReflectedXss.js:41:12:41:19 | req.body | user-provided value |
 | ReflectedXss.js:42:12:42:39 | convert ... q.body) | Cross-site scripting vulnerability due to $@. | ReflectedXss.js:42:31:42:38 | req.body | user-provided value |
 | ReflectedXss.js:56:12:56:19 | req.body | Cross-site scripting vulnerability due to $@. | ReflectedXss.js:56:12:56:19 | req.body | user-provided value |
