Skip to content

Python: Add Tornado source modeling #4869

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 10 commits into from
Jan 14, 2021
Merged

Conversation

RasmusWL
Copy link
Member

A draft PR since it builds upon the changes from #4864.

Locally confirmed to bring back the missing result for https://lgtm.com/projects/g/ArchiveTeam/wpull?mode=tree.

Comment on lines 477 to 480
TornadoRouteRegex() {
this instanceof StrConst and
DataFlow::localFlow(DataFlow::exprNode(this), setup.getUrlPatternArg())
}
Copy link
Contributor

Choose a reason for hiding this comment

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

We might be able to ask for a LocalSourceNode here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, just rewrote it to match DjangoRouteRegex

@yoff
Copy link
Contributor

yoff commented Jan 6, 2021

Preliminary review, since it is only a draft. But it basically looks great 👍 (nice, clean history too)

@RasmusWL RasmusWL marked this pull request as draft January 11, 2021 12:13
Now you can run them, and the examples have been adjusted so they actually work!
I should probably have split this up into 2 commits, so sorry that didn't happen :|
The reason this becomes valueable right now, is that we can mark routed params
as taint-sources. Longer down the line, we can (hopefully) detect that a routed
param will only accept digits, and mark it safe for some of our taint-tracking
queries.
@RasmusWL RasmusWL force-pushed the tornado-source-modeling branch from c8ddc8f to f9a29cb Compare January 14, 2021 12:38
@RasmusWL
Copy link
Member Author

Basically nothing changed since your last "preliminary" review, now it's just rebased on top of main (meaning the UI is only listing the commits that are part of this PR).

Only new thing is using LocalSourceNode as you suggested 😉

@RasmusWL RasmusWL marked this pull request as ready for review January 14, 2021 12:41
@RasmusWL RasmusWL requested a review from yoff January 14, 2021 12:41
Copy link
Contributor

@yoff yoff left a comment

Choose a reason for hiding this comment

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

Looks good to merge now. Should the remaining failing tests be tracked in an issue or is the (awesome) fail column sufficient?

@yoff yoff merged commit de8ac6c into github:main Jan 14, 2021
@RasmusWL RasmusWL deleted the tornado-source-modeling branch January 14, 2021 17:23
@RasmusWL
Copy link
Member Author

Looks good to merge now. Should the remaining failing tests be tracked in an issue or is the (awesome) fail column sufficient?

Good point. We probably should be tracking all those missing models in some issue 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants