JS: Promisification library modeling and enhance flow #20435
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR enhances the JavaScript analysis by adding comprehensive modeling for promisification libraries and improving data flow through promisified functions.
- Added support for seven new promisification libraries including
@gar/promisify,es6-promisify,util.promisify,thenify-all,call-me-maybe,@google-cloud/promisify, andutil-promisify - Enhanced data flow tracking to handle promisified user-defined functions
- Added API graph support for promisified object member access
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
javascript/ql/test/query-tests/Security/CWE-078/CommandInjection/promisification.js |
Test file demonstrating command injection vulnerabilities through various promisification libraries |
javascript/ql/test/query-tests/Security/CWE-078/CommandInjection/CommandInjection.expected |
Expected test results for the new promisification test cases |
javascript/ql/lib/semmle/javascript/dataflow/PromisifyFlow.qll |
New module providing data flow steps for promisified user-defined function calls |
javascript/ql/lib/semmle/javascript/dataflow/DataFlow.qll |
Imports the new PromisifyFlow module |
javascript/ql/lib/semmle/javascript/Promises.qll |
Extended PromisifyCall and PromisifyAllCall classes to support new libraries |
javascript/ql/lib/semmle/javascript/ApiGraphs.qll |
Added handling for promisified object member access in API graphs |
javascript/ql/lib/ext/call-me-maybe.model.yml |
New model file for the call-me-maybe library |
javascript/ql/lib/change-notes/2025-09-15-promisifications.md |
Release notes documenting the new promisification features |
| const promisify2 = require('util.promisify-all'); | ||
| const promisifiedCp = promisify2(cp); |
There was a problem hiding this comment.
[nitpick] The variable name 'promisify2' is inconsistent with the naming pattern used elsewhere in the file. Consider using 'promisifyAll' to match the library's functionality and improve clarity.
| const promisify2 = require('util.promisify-all'); | |
| const promisifiedCp = promisify2(cp); | |
| const promisifyAll = require('util.promisify-all'); | |
| const promisifiedCp = promisifyAll(cp); |
| 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 |
There was a problem hiding this comment.
The comment contains a grammatical error. 'but it should fine to flag it' should be 'but it should be fine to flag it'.
| 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 |
| app.post('/eval', async (req, res) => { | ||
| const maybe = require('call-me-maybe'); | ||
| const code = req.body; // $ Source | ||
|
|
There was a problem hiding this comment.
There is unnecessary trailing whitespace on line 115. This should be removed for consistency with the rest of the codebase.
asgerf
left a comment
asgerf
left a comment
There was a problem hiding this comment.
One minor comments otherwise looks good to merge
| exists( | ||
| DataFlow::SourceNode promisifiedObj, DataFlow::SourceNode originalObj, string member | ||
| | | ||
| promisifiedObj instanceof Promisify::PromisifyAllCall and |
There was a problem hiding this comment.
Why not just set the type of promisifiedObj to Promisify::PromifiyAllCall?
…d of `DataFlow::SourceNode`
2 participants
Added modeling for the following promisification related packages: