Skip to content

JS: Promisification library modeling and enhance flow #20435

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Napalys merged 11 commits into from
Sep 16, 2025
Merged

JS: Promisification library modeling and enhance flow #20435

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
d6a14e6
JS: Add test cases for promisification libraries.
Napalys Sep 15, 2025
22b6185
JS: Add modeling for `thenify-all`
Napalys Sep 15, 2025
d37425a
JS: Treat promisify(obj).member as obj.member
Napalys Sep 15, 2025
312471e
JS: Add modeling for `@google-cloud/promisify`
Napalys Sep 15, 2025
35c75c0
JS: Add modeling for `@gar/promisify`
Napalys Sep 15, 2025
e002f20
JS: Add modeling for `es6-promisify`
Napalys Sep 15, 2025
2c6db00
JS: Add modeling for util promisify*
Napalys Sep 15, 2025
0d23ab0
JS: Add data flow modeling for promisified user-defined functions
Napalys Sep 15, 2025
3a75500
JS: Add modeling for `call-me-maybe`
Napalys Sep 15, 2025
278a1ef
JS: Add change note
Napalys Sep 15, 2025
49ccb8c
JS: Simplify exist clause to use `Promisify::PromisifyAllCall` instea…
Napalys Sep 16, 2025
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
11 changes: 11 additions & 0 deletions javascript/ql/lib/semmle/javascript/ApiGraphs.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1115,6 +1115,17 @@ module API {
ref = awaited(call)
)
or
// Handle promisified object member access: promisify(obj).member should be treated as obj.member (promisified)
exists(
DataFlow::SourceNode promisifiedObj, DataFlow::SourceNode originalObj, string member
|
promisifiedObj instanceof Promisify::PromisifyAllCall and
Copy link
Copy Markdown
Contributor

@asgerf asgerf Sep 16, 2025

Choose a reason for hiding this comment

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

Why not just set the type of promisifiedObj to Promisify::PromifiyAllCall?

Copy link
Copy Markdown
Contributor Author

@Napalys Napalys Sep 16, 2025

Choose a reason for hiding this comment

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

Fair 😄 Fixed 49ccb8c

originalObj.flowsTo(promisifiedObj.(Promisify::PromisifyAllCall).getArgument(0)) and
use(base, originalObj) and
lbl = Label::member(member) and
ref = promisifiedObj.getAPropertyRead(member)
)
or
decoratorDualEdge(base, lbl, ref)
or
decoratorUseEdge(base, lbl, ref)
Expand Down
18 changes: 18 additions & 0 deletions javascript/ql/test/query-tests/Security/CWE-078/CommandInjection/CommandInjection.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,11 @@
| promisification.js:24:22:24:25 | code | promisification.js:21:18:21:25 | req.body | promisification.js:24:22:24:25 | code | This command line depends on a $@. | promisification.js:21:18:21:25 | req.body | user-provided value |
| promisification.js:52:21:52:24 | code | promisification.js:49:18:49:25 | req.body | promisification.js:52:21:52:24 | code | This command line depends on a $@. | promisification.js:49:18:49:25 | req.body | user-provided value |
| promisification.js:55:15:55:18 | code | promisification.js:49:18:49:25 | req.body | promisification.js:55:15:55:18 | code | This command line depends on a $@. | promisification.js:49:18:49:25 | req.body | user-provided value |
| promisification.js:100:23:100:26 | code | promisification.js:99:18:99:25 | req.body | promisification.js:100:23:100:26 | code | This command line depends on a $@. | promisification.js:99:18:99:25 | req.body | user-provided value |
| promisification.js:101:27:101:30 | code | promisification.js:99:18:99:25 | req.body | promisification.js:101:27:101:30 | code | This command line depends on a $@. | promisification.js:99:18:99:25 | req.body | user-provided value |
| promisification.js:102:27:102:30 | code | promisification.js:99:18:99:25 | req.body | promisification.js:102:27:102:30 | code | This command line depends on a $@. | promisification.js:99:18:99:25 | req.body | user-provided value |
| promisification.js:106:24:106:27 | code | promisification.js:99:18:99:25 | req.body | promisification.js:106:24:106:27 | code | This command line depends on a $@. | promisification.js:99:18:99:25 | req.body | user-provided value |
| promisification.js:109:24:109:27 | code | promisification.js:99:18:99:25 | req.body | promisification.js:109:24:109:27 | code | This command line depends on a $@. | promisification.js:99:18:99:25 | req.body | user-provided value |
| third-party-command-injection.js:6:21:6:27 | command | third-party-command-injection.js:5:20:5:26 | command | third-party-command-injection.js:6:21:6:27 | command | This command line depends on a $@. | third-party-command-injection.js:5:20:5:26 | command | user-provided value |
edges
| actions.js:8:9:8:13 | title | actions.js:9:16:9:20 | title | provenance | |
Expand Down Expand Up @@ -267,6 +272,12 @@ edges
| promisification.js:49:11:49:14 | code | promisification.js:52:21:52:24 | code | provenance | |
| promisification.js:49:11:49:14 | code | promisification.js:55:15:55:18 | code | provenance | |
| promisification.js:49:18:49:25 | req.body | promisification.js:49:11:49:14 | code | provenance | |
| promisification.js:99:11:99:14 | code | promisification.js:100:23:100:26 | code | provenance | |
| promisification.js:99:11:99:14 | code | promisification.js:101:27:101:30 | code | provenance | |
| promisification.js:99:11:99:14 | code | promisification.js:102:27:102:30 | code | provenance | |
| promisification.js:99:11:99:14 | code | promisification.js:106:24:106:27 | code | provenance | |
| promisification.js:99:11:99:14 | code | promisification.js:109:24:109:27 | code | provenance | |
| promisification.js:99:18:99:25 | req.body | promisification.js:99:11:99:14 | code | provenance | |
| third-party-command-injection.js:5:20:5:26 | command | third-party-command-injection.js:6:21:6:27 | command | provenance | |
nodes
| actions.js:8:9:8:13 | title | semmle.label | title |
Expand Down Expand Up @@ -461,6 +472,13 @@ nodes
| promisification.js:49:18:49:25 | req.body | semmle.label | req.body |
| promisification.js:52:21:52:24 | code | semmle.label | code |
| promisification.js:55:15:55:18 | code | semmle.label | code |
| promisification.js:99:11:99:14 | code | semmle.label | code |
| promisification.js:99:18:99:25 | req.body | semmle.label | req.body |
| promisification.js:100:23:100:26 | code | semmle.label | code |
| promisification.js:101:27:101:30 | code | semmle.label | code |
| promisification.js:102:27:102:30 | code | semmle.label | code |
| promisification.js:106:24:106:27 | code | semmle.label | code |
| promisification.js:109:24:109:27 | code | semmle.label | code |
| third-party-command-injection.js:5:20:5:26 | command | semmle.label | command |
| third-party-command-injection.js:6:21:6:27 | command | semmle.label | command |
subpaths
12 changes: 6 additions & 6 deletions javascript/ql/test/query-tests/Security/CWE-078/CommandInjection/promisification.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,17 +96,17 @@ app.post('/eval', async (req, res) => {
'exec',
'execSync',
]);
const code = req.body; // $ MISSING: Source
cpThenifyAll.exec(code); // $ MISSING: Alert
cpThenifyAll.execSync(code); // $ MISSING: Alert
cpThenifyAll.execFile(code); // $ MISSING: Alert - not promisified, as it is not listed in `thenifyAll`
const code = req.body; // $ Source
cpThenifyAll.exec(code); // $ Alert
cpThenifyAll.execSync(code); // $ Alert
cpThenifyAll.execFile(code); // $ SPURIOUS: Alert - not promisified, as it is not listed in `thenifyAll`, but it should fine to flag it
Copy link

Copilot AI Sep 16, 2025

Choose a reason for hiding this comment

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

The comment contains a grammatical error. 'but it should fine to flag it' should be 'but it should be fine to flag it'.

Suggested change
cpThenifyAll.execFile(code); // $ SPURIOUS: Alert - not promisified, as it is not listed in `thenifyAll`, but it should fine to flag it
cpThenifyAll.execFile(code); // $ SPURIOUS: Alert - not promisified, as it is not listed in `thenifyAll`, but it should be fine to flag it

Copilot uses AI. Check for mistakes.


var cpThenifyAll1 = thenifyAll.withCallback(require('child_process'), {}, ['exec']);
cpThenifyAll1.exec(code, function (err, string) {}); // $ MISSING: Alert
cpThenifyAll1.exec(code, function (err, string) {}); // $ Alert

var cpThenifyAll2 = thenifyAll(require('child_process'));
cpThenifyAll2.exec(code); // $ MISSING: Alert
cpThenifyAll2.exec(code); // $ Alert
});

app.post('/eval', async (req, res) => {
Expand Down