JS: Better type-tracking through Promise.all() #19412
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR improves type-tracking through explicit handling of Promise.all() calls by introducing new classes and refactoring existing steps used in the dataflow analysis. Key changes include the addition of PromiseAllCall and PromiseAllStep classes in Promises.qll, an update to use SharedTypeTrackingStep in ApiGraphs.qll, and a minor documentation update under change-notes.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| javascript/ql/src/change-notes/2025-04-30-promise-all.md | Adds change notes explaining the improved type propagation through Promise.all() calls. |
| javascript/ql/lib/semmle/javascript/internal/flow_summaries/Promises.qll | Introduces PromiseAllCall and PromiseAllStep classes to enable explicit type tracking through Promise.all(). |
| javascript/ql/lib/semmle/javascript/ApiGraphs.qll | Refactors step calls to use SharedTypeTrackingStep for consistency with the new type-tracking implementation. |
|
|
||
| /** Gets the `n`th output */ | ||
| DataFlow::SourceNode getNthOutput(int n) { | ||
| exists(string prop | |
There was a problem hiding this comment.
[nitpick] Consider adding an inline comment clarifying that the property names read from the output array are expected to be numeric strings, which justifies the conversion using toInt().
| exists(string prop | | |
| exists(string prop | | |
| // The property names read from the output array are expected to be numeric strings, | |
| // which justifies the conversion using toInt(). |
Copilot uses AI. Check for mistakes.
| this.flowsToExpr(await.getOperand()) and | ||
| result = await.flow() | ||
| ) | ||
| or |
There was a problem hiding this comment.
[nitpick] Consider adding a comment to explain that this fallback branch captures the Promise resolution callback when an AwaitExpr is not present, which aids future maintainability.
| or | |
| or | |
| // Fallback: Captures the Promise resolution callback when an AwaitExpr is not present. |
Copilot uses AI. Check for mistakes.
Adds explicit type-tracking steps through common uses of
Promise.all:In the following example, we'd generate a read step from
promise1 -> val1reading the promise-content, and likewise forpromise2 -> val2:Evaluation looks good, showing 15 new call edges, 18 new sinks, and 11 new threat model sources.