JS: Migrate to shared data flow library (targeting main!) 🚀 #18467
+28,632
−35,398
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
TaintedUrlSuffix is currently only used in TaintTracking configs meaning it is already propagated by taint steps. The inclusion of these taint steps here however meant that implicit reads could appear prior to any of these steps. This was is problematic for PropRead steps as an expression like x[0] could spuriously read from array element 1 via the path: x [element 1] x [empty access path] (after implicit read) x[0] (taint step through PropRead)
A URL of form https://example.com?evil#bar will contain '?evil' after splitting out the '#' suffix, and vice versa.
Preserving tainted-url-suffix into array element 0 seemed like a good idea, but didn't work out so well.
JS: Fix handling of constant array index reads, and fix the fallout
Previously only reflected XSS used shared barrier guards.
JS: Fix bug causing re-evaluation of cached barriers
- Paths are now relative to the test case, not the qlpack - Paths going through an implicit reads have changed slightly
We need these to return the dominator instead of declaring it in the parameter list, so that we can use it directly to fulfill part of the signature for the SSA library. We can't rewrite it with an inline predicate since the SSA module calls with a transitive closure '*', which does not permit inline predicates.
JS: Remove with statement comment
…public This exposes the predicates publicly, but will be hidden again in the next commit.
Indentation will be fixed in next commit
Only formatting changes
JS: Deprecate some .qll files
Co-authored-by: Erik Krogh Kristensen <[email protected]>
…ow-queries.rst Co-authored-by: Erik Krogh Kristensen <[email protected]>
JS: Add migration guide and change note
This isn't going to become a taint step, the workaround is the permanent solution
These are tracked in github/codeql-javascript-team#456
The test case actually has the correct result now
There is an issue for tracking this. It's not a small fix.
This requires changes to the shared data flow library, not something we should track with a TODO in the JS codebase
This is useful info, but not something that can be fixed locally in this query, so a TODO comment isn't helping
We have an issue for fixing the underlying problem
The RHS of an assignment actually has a post-update node now
It is not subsumed by the other case, both cases are needed
JS: Clean up some TODO comments
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
CodeQL found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
erik-krogh
approved these changes
Jan 15, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This merges the feature branch
js/shared-dataflow-branchintomain, replacing JavaScript's data flow library with the shared data flow library (instantiated for JavaScript).Data flow queries are now written against the same interface as other languages (
ConfigSigstyle). All queries and documentation has been updated accordingly. There's a migration guide included in this PR to assist external users in migrating their queries.In terms of analysis quality, there are some general benefits from performing the switch:
It also comes with a lot of other semi-related changes, which perhaps seem orthogonal, but were necessary to enable a smooth transition. There's too much to mention in full, so just a few highlights:
Function.prototype.apply, and uses of theargumentsarray. These "dynamic" argument-passing steps are now much better at preserving argument indices to avoid mixing up different arguments and parameters. Super-calls through default constructors are also handled more precisely, since they desugar down to rest parameters and spread arguments.See the list of pull requests and their descriptions for more details.