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.
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
Python: Model FastAPI requests #18318
Python: Model FastAPI requests #18318
Changes from 3 commits
79dfbf7
34631a8
2b3fc9b
a9704d8
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For my understanding: This would also apply to
fastapi.Request
but you're just choosing one qualified name to represent the class right?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, this class is like the old string-based dataflow configurations, so it just needs a unique string to not have cross-talk.
Check warning
Code scanning / CodeQL
Singleton set literal Warning
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this assert tell the analysis (type tracking?) that these types are equal?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no, it's purely for humans reading the test-file to highlight the fact that they are indeed the same class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(we just adjusted a copy of the websocket modeling, which did the same)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Non-blocking question: If we accidentally dropped either fastapi or starlette from the union in
classRef
, this test would still pass right? Would we want separate test code paths for the two ways of obtaining theRequest
type?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep. that
asssert
has no impact on the analysis, and could have been a comment.If we wanted 100% test-coverage of our modeling, then yes. My personal opinion is that we're getting into the realm of diminishing returns of doing so, so I didn't bother.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For my understanding: These are missing because we don't track flow from the result of
form()
through to its own properties?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we don't have taint-steps to attribute reads by default 👍 We would need to model the specific attribute-flow we wanted for multidict and uploaded files, which we haven't done yet.