Skip to content

JS: Generate legacy flow steps for all flow summaries #20169

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.

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
merged 1 commit into from
Aug 6, 2025

Conversation

hvitved
Copy link
Contributor

@hvitved hvitved commented Aug 5, 2025

#19445 follow up: The legacy data flow/taint tracking libraries do not use SummarizedCallables, so we still need to generate local flow steps for all summaries.

@github-actions github-actions bot added the JS label Aug 5, 2025
@hvitved hvitved force-pushed the javascript/legacy-summary-steps branch from 4547d9f to eb3c054 Compare August 6, 2025 07:39
@hvitved hvitved added the no-change-note-required This PR does not need a change note label Aug 6, 2025
@hvitved hvitved marked this pull request as ready for review August 6, 2025 07:56
@hvitved hvitved requested a review from a team as a code owner August 6, 2025 07:56
@Copilot Copilot AI review requested due to automatic review settings August 6, 2025 07:56
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR ensures that legacy data flow/taint tracking libraries receive flow steps for all flow summaries, not just those that are unsupported by the new flow summary system. Previously, legacy flow steps were only generated for summaries that couldn't be handled as proper flow summaries, but legacy libraries don't use SummarizedCallables at all and need steps for all summaries.

Key Changes:

  • Modified the logic to generate legacy flow steps for all flow summaries instead of only unsupported ones
  • Added new legacy flow step classes that handle all summary-based steps
  • Updated the test expectations to reflect that legacy and new data flow libraries now have consistent behavior

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
javascript/ql/lib/semmle/javascript/frameworks/data/ModelsAsData.qll Added logic to generate legacy flow steps for all summaries and created separate legacy step classes
javascript/ql/test/library-tests/frameworks/data/test.expected Updated test expectations to remove differences between legacy and new data flow libraries

Copy link
Contributor

@Napalys Napalys left a comment

Choose a reason for hiding this comment

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

Thank you very much for tackling this! ❤️ I had tried previously looking into it, but wasn't able to find a solution. This looks good to me!

@hvitved hvitved merged commit dfe4401 into github:main Aug 6, 2025
17 checks passed
@hvitved hvitved deleted the javascript/legacy-summary-steps branch August 6, 2025 16:52
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