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: Bottle Framework Support #17370

Merged
merged 12 commits into from
Nov 19, 2024
Merged

Conversation

Kwstubbs
Copy link
Contributor

@Kwstubbs Kwstubbs commented Sep 3, 2024

Add basic Bottle support

@Kwstubbs Kwstubbs requested a review from a team as a code owner September 3, 2024 21:01
@Kwstubbs Kwstubbs changed the title Python: Bottle Header support Python: Bottle Framework Header Support Sep 3, 2024
Copy link
Member

@RasmusWL RasmusWL left a comment

Choose a reason for hiding this comment

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

Thanks 🎉

Overall the code looks OK to me, but to accept this PR I need you to write some tests 😊 (good start by copying over the ConceptsTest, but we also need some code using bottle to show that the modeling works as intended) See flask tests as an inspiration.

python/ql/lib/semmle/python/frameworks/Bottle.qll Outdated Show resolved Hide resolved
@Kwstubbs Kwstubbs changed the title Python: Bottle Framework Header Support Python: Bottle Framework Support Sep 23, 2024
@Kwstubbs
Copy link
Contributor Author

@RasmusWL I've added more support for Bottle routes and I've added tests.

@yoff
Copy link
Contributor

yoff commented Oct 14, 2024

@RasmusWL is travelling at the moment, so I will take this over for now.
I am curious about the switch away from API nodes in 5d12f7b, was it necessary to get the tests pass?
It looks like you based this off of other models. This is absolutely fine, but let us double check that we are actually modelling the bottle framework.

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.

The structure of both the code and the tests are good. I am a little worried that the modelling is not completely aligned with the framework; all these frameworks have subtle differences. A good way to align with the framework could be to write several examples, taken from tutorials and the like, in the test files and then align the modelling to those (padding out the set of function names and argument names by looking at the documentation).

python/ql/lib/semmle/python/frameworks/Bottle.qll Outdated Show resolved Hide resolved
python/ql/lib/semmle/python/frameworks/Bottle.qll Outdated Show resolved Hide resolved
python/ql/lib/semmle/python/frameworks/Bottle.qll Outdated Show resolved Hide resolved
python/ql/lib/semmle/python/frameworks/Bottle.qll Outdated Show resolved Hide resolved
@yoff yoff self-assigned this Oct 14, 2024
@Kwstubbs
Copy link
Contributor Author

@yoff Hi, please let me know what you think of the changes. I'm trying to model every possible usage of bottle, so even though the documentation might not mention something, if I find it is used a lot in Github I have included them. Obviously it must run on the latest, currently v12.25 as LTS. Also, I'm not sure where I moved away from API nodes in 5d12f7bd3. Everything I have modeled here I have created a codebase/codeql database to ensure validity.

@Kwstubbs Kwstubbs requested a review from yoff October 29, 2024 22:45
@yoff
Copy link
Contributor

yoff commented Oct 30, 2024

Also, I'm not sure where I moved away from API nodes in 5d12f7bd3.

Sorry, that was a somewhat vague reference to a large diff. I meant the fields name and value inside the class HeaderWriteSubscript in the Headers module (see here).

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.

I am requesting a small simplification, but I am feeling much better about this now; thanks for explaining your testing :-)

python/ql/lib/semmle/python/frameworks/Bottle.qll Outdated Show resolved Hide resolved
python/ql/lib/semmle/python/frameworks/Bottle.qll Outdated Show resolved Hide resolved
python/ql/lib/semmle/python/frameworks/Bottle.qll Outdated Show resolved Hide resolved
@Kwstubbs
Copy link
Contributor Author

@yoff I was just copying from Werkzeug.qll as recommended by Rasmus on his first pass. The API nodes was working, as I was using these models in my research before PRing. ✅

@Kwstubbs Kwstubbs requested a review from yoff October 30, 2024 20:59
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.

LGTM

@yoff
Copy link
Contributor

yoff commented Oct 31, 2024

@yoff I was just copying from Werkzeug.qll as recommended by Rasmus on his first pass. The API nodes was working, as I was using these models in my research before PRing. ✅

In that case, it would be nicer to use the API nodes. They are cleaner and might match in a few more cases. I will not require it, though, it can be a clean-up of both situations later.

@Kwstubbs
Copy link
Contributor Author

@yoff just curious about the status of this PR. Are we just waiting on approval from @RasmusWL?

@yoff
Copy link
Contributor

yoff commented Nov 19, 2024

@yoff just curious about the status of this PR. Are we just waiting on approval from @RasmusWL?

Sorry, this must have fallen through the cracks.

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.

LGTM

@yoff yoff dismissed RasmusWL’s stale review November 19, 2024 14:34

Changes have been made satisfactorily

@yoff yoff merged commit 22287be into github:main Nov 19, 2024
14 checks passed
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