Visitar URL original
JavaScript: Treat LinkifyIt match calls as non-RegExp operations by theinfosecguy · Pull Request #22577 · github/codeql · GitHub
Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
---
category: minorAnalysis
---
* Calls to `LinkifyIt.match()` are no longer incorrectly identified as regular expression operations.
18 changes: 18 additions & 0 deletions javascript/ql/lib/semmle/javascript/Regexp.qll
Original file line number Diff line number Diff line change
Expand Up @@ -986,6 +986,22 @@ private predicate isMatchObjectProperty(string name) {
name in ["length", "index", "input", "groups"]
}

/** Gets an API node representing a `LinkifyIt` instance. */
private API::Node linkifyItInstance() {
result = API::moduleImport("linkify-it").getMember("exports").getMember("LinkifyIt").getInstance()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this variant with getMember("exports") needed?

or
result = API::moduleImport("linkify-it").getMember("LinkifyIt").getInstance()
or
result = API::moduleImport("linkify-it").getMember("exports").getMember("linkifyit").getReturn()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above, why is this variant needed?

or
result = API::moduleImport("linkify-it").getMember("linkifyit").getReturn()
or
// Before version 6, the module export was the factory function.
result = API::moduleImport("linkify-it").getReturn()
or
result = linkifyItInstance().getMember(["add", "set", "tlds"]).getReturn()
}

/** Holds if `call` is a call to `match` whose result is used in a way that is incompatible with Match objects. */
overlay[global]
private predicate isUsedAsNonMatchObject(DataFlow::MethodCallNode call) {
Expand All @@ -1006,6 +1022,8 @@ private predicate isUsedAsNonMatchObject(DataFlow::MethodCallNode call) {
call.asExpr() = any(ExprStmt stmt).getExpr()
or
call = API::moduleImport("sinon").getMember("match").getACall()
or
call = linkifyItInstance().getMember("match").getACall()
)
}

Expand Down
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
| linkify-it/tst-LinkifyIt.js:21:25:21:48 | ^https://www.example.com | This regular expression has an unescaped '.' before 'example.com', so it might match more hosts than expected. | linkify-it/tst-LinkifyIt.js:21:24:21:49 | "^https ... le.com" | here |
| tst-IncompleteHostnameRegExp.js:3:3:3:28 | ^http:\\/\\/test.example.com | This regular expression has an unescaped '.' before 'example.com', so it might match more hosts than expected. | tst-IncompleteHostnameRegExp.js:3:2:3:29 | /^http: ... le.com/ | here |
| tst-IncompleteHostnameRegExp.js:6:3:6:28 | ^http:\\/\\/test.example.net | This regular expression has an unescaped '.' before 'example.net', so it might match more hosts than expected. | tst-IncompleteHostnameRegExp.js:6:2:6:29 | /^http: ... le.net/ | here |
| tst-IncompleteHostnameRegExp.js:7:3:7:42 | ^http:\\/\\/test.(example-a\|example-b).com | This regular expression has an unescaped '.' before '(example-a\|example-b).com', so it might match more hosts than expected. | tst-IncompleteHostnameRegExp.js:7:2:7:43 | /^http: ... b).com/ | here |
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
{
"type": "module",
"dependencies": {
"linkify-it": "6.1.0"
}
}
Comment on lines +1 to +6

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this file needed?

Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
import { LinkifyIt, linkifyit } from "linkify-it";
import { LinkifyIt as OtherLinkifyIt } from "other-linkify-it";

const scanner = new LinkifyIt({ fuzzyLink: false, fuzzyEmail: false })
.add("ftp:", null)
.add("mailto:", null)
.add("//", null);
const text =
"😀 *literal* (https://www.youtube.com/watch?v=tax4e4hBBZc), then https://store.steampowered.com/app/457140/.";
const matches = scanner.match(text);
if (matches) {
console.log(matches.map((match) => match.raw));
}

if (new LinkifyIt().match("https://www.example.com")) {}
if (new LinkifyIt().set({ fuzzyLink: false }).match("https://www.example.com")) {}
if (new LinkifyIt().tlds("onion", true).match("https://www.example.com")) {}
if (linkifyit().match("https://www.example.com")) {}

const otherScanner = new OtherLinkifyIt().add("ftp:", null);
if (otherScanner.match("^https://www.example.com")) {} // $ Alert
Comment on lines +1 to +21

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this could do with some clean up. There's quite some duplication here and the OtherLinkifyIt seems not really necessary. If it was, then our JS analysis would be quite fundamentally broken.

It's also not clear to me why this ended up in a separate file, which is not a pattern used in this test directory.

Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
const { LinkifyIt } = require("linkify-it");
const legacyLinkifyIt = require("linkify-it");

const scanner = new LinkifyIt().add("ftp:", null).set({ fuzzyLink: false });
const text = "https://a.b.com";
console.log(scanner.match(text));
console.log(legacyLinkifyIt().match(text));
Comment on lines +1 to +7

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's again not clear to me why this is in a separate file.

Original file line number Diff line number Diff line change
Expand Up @@ -9,3 +9,12 @@ app.get('/findKey', function(req, res) {
var safeKey = _.escapeRegExp(key);
var re = new RegExp("\\b" + safeKey + "=(.*)\n");
});

var { LinkifyIt } = require("linkify-it");

app.get('/findLinks', function(req, res) {
var text = req.param("text");
var scanner = new LinkifyIt().set({ fuzzyLink: false });
var matches = scanner.match(text);
res.json(matches);
});
Comment on lines +12 to +20

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure why this test was added, especially since there is no regex here. Could you explain?