JavaScript: Add new query flagging identity replacements. #222
Conversation
ghost
left a comment
ghost
left a comment
There was a problem hiding this comment.
LGTM. Modulo support for some additional regex features.
Should this query be added to a query suite?
| */ | ||
| predicate matchesString(Expr e, string s) { | ||
| exists (RegExpConstant c | | ||
| matchesConstant(e.(RegExpLiteral).getRoot(), c) and |
There was a problem hiding this comment.
We should exclude the i-flag for cases like: x.replace(/foo/i, 'foo').
| exists (RegExpCharacterClass recc | recc = t and not recc.isInverted() | | ||
| recc.getNumChild() = 1 and | ||
| matchesConstant(recc.getChild(0), c) | ||
| ) |
There was a problem hiding this comment.
We should handle anchors for cases like /^x$/
There was a problem hiding this comment.
Good idea; and also other zero-width assertions like \b.
| * @kind problem | ||
| * @problem.severity warning | ||
| * @id js/identity-replacement | ||
| * @tags correctness |
There was a problem hiding this comment.
It's a bit of a stretch, but I suppose it does cover CWE-116, so might as well claim it.
xiemaisi
force-pushed
the
js/identity-replacement
branch
from
9ca1ebd to
9002d7f
Compare
|
@esben-semmle: Updated as suggested. I've amended the original commit to include the changes to the query metadata and added it to a suite, the other commits roughly correspond to your other suggestions. This query is now a bit more complex. I could run a full performance evaluation if desired, but it still looks fairly snappy on the query console, and takes about 20 seconds on |
| repl.getArgument(1).getStringValue() = s | ||
| select repl.getArgument(0), "This replaces '" + s + "' with itself." | ||
| repl.getArgument(1).getStringValue() = s and | ||
| (if s = "" then friendly = "the empty string" else friendly = "'" + s + "'") |
There was a problem hiding this comment.
How about a friendly formatting of the two quote characters: ' and "?
There was a problem hiding this comment.
Could do, but once we start going down that route there are many other friendliness improvements we could also consider (such as special handling of newlines and whatnot). This one is very simple and straightforward, the other ones not so much.
| |-----------------------------------------------|------------------------------------------------------|-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------| | ||
| | Enabling Node.js integration for Electron web content renderers (`js/enabling-electron-renderer-node-integration`) | security, frameworks/electron, external/cwe/cwe-094 | Highlights Electron web content renderer preferences with Node.js integration enabled, indicating a violation of [CWE-94](https://cwe.mitre.org/data/definitions/94.html). Results are not shown on LGTM by default. | | ||
| | Stored cross-site scripting (`js/stored-xss`) | security, external/cwe/cwe-079, external/cwe/cwe-116 | Highlights uncontrolled stored values flowing into HTML content, indicating a violation of [CWE-079](https://cwe.mitre.org/data/definitions/79.html). Results shown on LGTM by default. | | ||
| | Replacement of a substring with itself (`js/identity-replacement`) | correctness | Highlights string replacements that replace a string with itself, which usually indicates a mistake. Results shown on LGTM by default. | |
There was a problem hiding this comment.
This line is now missing some Tags.
Optionally, the Purpose could also be the standard "... indicating a violation a violation of CWE-116..."
There was a problem hiding this comment.
Ah, of course; fixed. I decided not to go for the violation-of-CWE phrasing, though, since I don't want to claim that that's the main purpose of the query.
xiemaisi
force-pushed
the
js/identity-replacement
branch
from
9002d7f to
0e63ea1
Compare
|
Nits addressed/responded to. |
|
Ping @mc-semmle for doc-review. |
|
@xiemaisi - editorial review completed, LGTM. |
ghost
merged commit 
Add support for LGTM_INDEX_FILTERS environment variable
Kotlin: Remove some comments
…-stub-models Revert "Update the C# stub models"
2 participants
This is a surprisingly common mistake.