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 new file mode 100644 index 000000000000..8dd3c17404c7 --- /dev/null +++ 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 new file mode 100644 index 000000000000..4dcf36164c99 --- /dev/null +++ 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 index 04f3c9f7db78..344bdc2a40fc 100644 --- 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 index 9e842ab516ed..675403f5ab9d 100644 --- 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 index c3b1cbc2da8a..dee88dd177bc 100644 --- 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; } -}); \ No newline at end of file +}); 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 index d29b35203b80..a367f07307a5 100644 --- 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 |