Skip to content

Conversation

@Vasco-jofra
Copy link
Contributor

This PR makes these changes:

  1. In FlowSummaryPrivate.qll: Added support for anyProperty content set in flow summaries:
    • Please confirm this is the correct way to support this. I needed this to find the async_.map({a: source()}, call_sink) case. Let me know if there's a better way to do it
    • As a note, I was also unable to use Element, which, according to the documentation, should select "an element of an array, iterator, or set object."
  2. In AsyncPackage.qll: Improve taint tracking through functions from the async package:
    • Improve tracking of the callback arguments
    • Implemented flow summaries for more robust taint tracking
    • Updated tests

@Vasco-jofra Vasco-jofra requested a review from a team as a code owner June 15, 2025 16:06
@github-actions github-actions bot added the JS label Jun 15, 2025
@Vasco-jofra
Copy link
Contributor Author

Also, let me know if there's a way to avoid having two DataFlow::SummarizedCallable when all I want to do is change the argument number. I found no way to get a reference to the call in the propagatesFlow predicate against which I could call getIteratorCallbackIndex.

Copy link
Contributor

@asgerf asgerf left a comment

Choose a reason for hiding this comment

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

Looks great! Thanks for the contribution! I just have a few minor comments while waiting for evaluation results.

* For example, `memberVariant("map")` finds references to `map`, `mapLimit`, and `mapSeries`.
* For example, `memberNameVariant("map")` returns `map`, `mapLimit`, and `mapSeries`.
*/
DataFlow::SourceNode memberVariant(string name) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this is a publicly accessible predicate, we should avoid changing its signature. Instead, deprecate the original predicate and give a different name to the new predicate.

This predicate probably shouldn't have been public in the first place. Feel free to make the new one private.

succ = final.getParameter(1) and
call.getName() = "sortBy"
)
class IterationPreserveTaintStepFlowSummary extends DataFlow::SummarizedCallable {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
class IterationPreserveTaintStepFlowSummary extends DataFlow::SummarizedCallable {
private class IterationPreserveTaintStepFlowSummary extends DataFlow::SummarizedCallable {

@asgerf
Copy link
Contributor

asgerf commented Jun 24, 2025

Also, let me know if there's a way to avoid having two DataFlow::SummarizedCallable when all I want to do is change the argument number. I found no way to get a reference to the call in the propagatesFlow predicate against which I could call getIteratorCallbackIndex.

You can merge the two classes by adding a field to the class, and embedding the value of the field in its this binding. See this example.

@asgerf
Copy link
Contributor

asgerf commented Jun 24, 2025

As a note, I was also unable to use Element, which, according to the documentation, should select "an element of an array, iterator, or set object."

Thanks, I'll make sure to fix the discrepancy.

@asgerf asgerf added the no-change-note-required This PR does not need a change note label Jun 25, 2025
@Vasco-jofra
Copy link
Contributor Author

I fixed what you pointed out in your comments and merged the DataFlow::SummarizedCallable classes into a single one as you suggested.

I don't have time to test now, but in a codebase I was running this on, there was an asyncHelper.each function that called async.each inside. This change was causing asyncHelper.each(array1, cb1); asyncHelper.each(array2, cb2), to incorrectly flow data from array1 to the first parameter of both cb1 and cb2. This caused several false positives. I haven't investigated why this happens, but we should fix it before merging.

@asgerf asgerf merged commit 65102a0 into github:main Sep 16, 2025
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

JS no-change-note-required This PR does not need a change note

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants