The Wayback Machine - https://web.archive.org/web/20260101024414/https://github.com/github/codeql/pull/4564
Skip to content

JS: Improve handling of React, lodash, function composition, and dynamic imports #4564

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.

Sign up for GitHub

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

Jump to bottom
Merged
codeql-ci merged 15 commits into from
Oct 30, 2020
Merged

JS: Improve handling of React, lodash, function composition, and dynamic imports #4564

codeql-ci merged 15 commits into from
Oct 30, 2020

Conversation

@asgerf
Copy link
Contributor

@asgerf asgerf commented Oct 28, 2020 •
edited
Loading

Adds models for React hooks and react-router (react-router-dom), and tracks components through component transformers and lazy loading.

Some drive-by fixes from testing on some React applications:

  • The react props taint steps no longer extends ValueNode as this prevented it from propagating into a property pattern (object patterns are value nodes, but not property patterns).
  • A bunch of lodash taint steps (almost the entire "string" category)
  • A template injection sink for lodash.template (experimental query). Added to justify not having it as a taint step.
  • The function composition model now has a public API and distinguishes left-to-right from right-to-left composition.
  • Type-tracking steps through dynamic imports (needed for lazy-loaded React components).
    • Historically we did not handle this well due to the module being wrapped in a promise. We have store/load steps for promises now, which worked like a charm 👍
  • The TaintSteps metric now includes AdditionalFlowStep steps. They also affect taint-tracking so this seemed easier than having a separate metric for it. (Both sides of the evaluations contain this commit)

Evaluations:

  • react_webapps shows a good number of new taint steps and a few new sources.
  • Nightly shows a few thousand new taint steps and a slowdown of about 1%, but the slowdown did not reproduce on a double re-run of the slowest slugs.
@asgerf asgerf added JS Awaiting evaluation Do not merge yet, this PR is waiting for an evaluation to finish labels Oct 28, 2020
@asgerf asgerf requested a review from a team as a code owner October 28, 2020 12:01
erik-krogh
erik-krogh reviewed Oct 28, 2020
View reviewed changes
Copy link
Contributor

@erik-krogh erik-krogh left a comment

Choose a reason for hiding this comment

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

I like it.

It took a while to chew through, but it looks great.
I just got a few minor comments.
asgerf and others added 3 commits October 28, 2020 16:23
@asgerf
JS: Add test and fix for contextType
f99db23
@asgerf @erik-krogh
Update change-notes/1.26/analysis-javascript.md
43336ff 
Co-authored-by: Erik Krogh Kristensen <erik-krogh@github.com>
@asgerf @erik-krogh
Update javascript/ql/src/semmle/javascript/frameworks/React.qll
581441d 
Co-authored-by: Erik Krogh Kristensen <erik-krogh@github.com>
@asgerf
Copy link
Contributor Author

asgerf commented Oct 28, 2020

Evaluation on nightly came back with a median slowdown of about 1%. Could have been worse, but could also be better. Will investigate.
@asgerf
Copy link
Contributor Author

asgerf commented Oct 30, 2020

Double re-run of the 3 slowest slugs shows that the 1% was most likely a fluke. Looking at tuple counts I didn't see anything out of the ordinary either, although I did notice that getAContextInput should probably be restricted a bit.
@asgerf asgerf removed the Awaiting evaluation Do not merge yet, this PR is waiting for an evaluation to finish label Oct 30, 2020
esbena
esbena reviewed Oct 30, 2020
View reviewed changes
Copy link
Contributor

@esbena esbena left a comment

Choose a reason for hiding this comment

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

LGTM. Two optional nits.

I have raised https://github.com/github/codeql-javascript-team/issues/248, which isn't a blocker for this PR.
javascript/ql/src/semmle/javascript/frameworks/ComposedFunctions.qll

override predicate step(DataFlow::Node pred, DataFlow::Node succ) {
exists(int fnIndex, DataFlow::FunctionNode fn | fn = composed.getFunction(fnIndex) |
exists(int fnIndex, DataFlow::FunctionNode fn | fn = composed.getOperandFunction(fnIndex) |
Copy link
Contributor

@esbena esbena Oct 30, 2020

Choose a reason for hiding this comment

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

Nit: this would read more natural if the order of the three disjuncts was in between out, instead of the current out in between.
javascript/ql/src/semmle/javascript/frameworks/React.qll Outdated
}

override predicate step(DataFlow::Node pred, DataFlow::Node succ) {
// `memo(f)` returns a function behaves as `f` but caches results
Copy link
Contributor

@esbena esbena Oct 30, 2020

Choose a reason for hiding this comment

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

Suggested change
// `memo(f)` returns a function behaves as `f` but caches results
// `memo(f)` returns a function that behaves as `f` but caches results
Copy link
Contributor Author

@asgerf asgerf Oct 30, 2020

Choose a reason for hiding this comment

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

Fixed, and also moved the comment closer to where it belongs
@codeql-ci codeql-ci merged commit 4a59e69 into github:main Oct 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation JS

4 participants

@asgerf @esbena @erik-krogh @codeql-ci