Skip to content
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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Conversation

RasmusWL
Copy link
Member

Pull Request checklist

All query authors

Internal query authors only

  • Autofixes generated based on these changes are valid, only needed if this PR makes significant changes to .ql, .qll, or .qhelp files. See the documentation (internal access required).
  • Changes are validated at scale (internal access required).
  • Adding a new query? Consider also adding the query to autofix.

@Copilot Copilot bot review requested due to automatic review settings December 18, 2024 15:03
@RasmusWL RasmusWL requested a review from a team as a code owner December 18, 2024 15:03

Choose a reason for hiding this comment

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

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

Files not reviewed (1)
  • python/ql/lib/semmle/python/frameworks/Starlette.qll: Language not supported
Comments suppressed due to low confidence (4)

python/ql/test/library-tests/frameworks/fastapi/taint_test.py:213

  • The taint annotation is missing for the data returned by request.form().getlist("key"). Ensure that this case is covered by the taint analysis.
await request.form().getlist("key"), // $ MISSING: tainted

python/ql/test/library-tests/frameworks/fastapi/taint_test.py:214

  • The taint annotation is missing for the first element of the list returned by request.form().getlist("key"). Ensure that this case is covered by the taint analysis.
await request.form().getlist("key")[0], // $ MISSING: tainted

python/ql/test/library-tests/frameworks/fastapi/taint_test.py:216

  • The taint annotation is missing for the filename of the file returned by request.form()["file"]. Ensure that this case is covered by the taint analysis.
await request.form()["file"].filename, // $ MISSING: tainted

python/ql/test/library-tests/frameworks/fastapi/taint_test.py:217

  • The taint annotation is missing for the filename of the first file in the list returned by request.form().getlist("file"). Ensure that this case is covered by the taint analysis.
await request.form().getlist("file")[0].filename, // $ MISSING: tainted

Tip: If you use Visual Studio Code, you can request a review from Copilot before you push from the "Source Control" tab. Learn more


override DataFlow::Node getInstance() { result = instance() }

override string getAttributeName() { result in ["cookies"] }

Check warning

Code scanning / CodeQL

Singleton set literal Warning

Singleton set literal can be replaced by its member.
Copy link
Collaborator

@adityasharad adityasharad left a comment

Choose a reason for hiding this comment

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

Nice! Quickly read through and it makes sense - good to understand where you add the relevant library modelling. Minor suggestions and questions for my understanding; I'll leave a detailed review of the QL to someone else more familiar.

Comment on lines +213 to +217
await request.form().getlist("key"), # $ MISSING: tainted
await request.form().getlist("key")[0], # $ MISSING: tainted
# data in the form could be an starlette.datastructures.UploadFile
await request.form()["file"].filename, # $ MISSING: tainted
await request.form().getlist("file")[0].filename, # $ MISSING: tainted
Copy link
Collaborator

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?

* Taint propagation for `starlette.requests.Request`.
*/
private class InstanceTaintSteps extends InstanceTaintStepsHelper {
InstanceTaintSteps() { this = "starlette.requests.Request" }
Copy link
Collaborator

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?

---
category: minorAnalysis
---
* Added modeling of `fastapi.Request`, which will improve taint-flow for users of FastAPI.
Copy link
Collaborator

Choose a reason for hiding this comment

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

How about this?

Suggested change
* Added modeling of `fastapi.Request`, which will improve taint-flow for users of FastAPI.
* Added modeling of `fastapi.Request` and `starlette.requests.Request` as sources of untrusted input,
and modeling of tainted data flow out of these request objects.

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.

3 participants