[flake8-bandit]: Implement S610 rule
#10316
Conversation
|
| code | total | + violation | - violation | + fix | - fix |
|---|---|---|---|---|---|
| S610 | 22 | 22 | 0 | 0 | 0 |
Formatter (stable)
✅ ecosystem check detected no format changes.
Formatter (preview)
✅ ecosystem check detected no format changes.
Upstream references: - Implementation: https://github.com/PyCQA/bandit/blob/1.7.8/bandit/plugins/django_sql_injection.py#L20-L97 - Test cases: https://github.com/PyCQA/bandit/blob/1.7.8/examples/django_sql_injection_extra.py - Test assertion: https://github.com/PyCQA/bandit/blob/1.7.8/tests/functional/test_functional.py#L517-L524
| if !checker.semantic().seen_module(Modules::DJANGO) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
This is also done for django_raw_sql, but I'm wondering if this should also be done for this rule.
In django_raw_sql, RawSQL import comes from django module, so this is reliable, but in our case, extra can be used even if no django imports occurs, if for instance a model defined in another file in a codebase is used, so this could skip the check for several valid cases.
On the other hand, the entire rule has the potential of having several false positives, so this could be seen as a way to know if a file belongs to a project using Django. But if I'm not mistaken, seen_module only applies to the current file, not the whole codebase, and in this specific case the latter would probably be preferable.
There was a problem hiding this comment.
Case in point, the ecosystem checks detects 18 errors while bandit detects 22. That's because there are 4 things that would have been flagged in https://github.com/zulip/zulip/blob/29ca4ba6620f16598e1171be7495356f56b84328/zerver/tests/test_message_move_topic.py, but are skipped because there is no import from django in this file.
There was a problem hiding this comment.
I think I'm okay with leaving this in for now.
There was a problem hiding this comment.
I could go either way though. extra isn't a common method name.
There was a problem hiding this comment.
Yeah, I think that the combination of extra and select/where/tables arguments should not be that common outside of Django. Worst case, for the few times it would get triggered, it's always possible for users to ignore the false positives, while skipping the check entirely if Ruff does not find a django import would leave the user with no clue that the check was not run because of that. So I think it might be best in the end to not check if Django was seen.
There was a problem hiding this comment.
I went ahead and removed the check in 4e88142. Also updated the error message so that in case of false positives, the user at least knows that this is related to Django.
| match argument_name { | ||
| "select" => match argument { | ||
| Expr::Dict(ExprDict { keys, values, .. }) => { | ||
| if !keys.iter().flatten().all(Expr::is_string_literal_expr) { |
There was a problem hiding this comment.
flatten() is used to remove optional keys here. Not sure that this is really a clean way to do this, let me know if not.
There was a problem hiding this comment.
Do we want to flag on select(**kwargs)? It seems like this wouldn't, but it probably should, right?
There was a problem hiding this comment.
We might want to consider allowing: select(**{"foo": "bar"})
Part of #1646.
Summary
Implement
S610rule fromflake8-bandit.Upstream references:
The implementation in
bandittargets additional arguments (params,order_byandselect_params) but doesn't seem to do anything with them in the end, so I did not include them in the implementation.Note that this rule could be prone to false positives, as ideally we would want to check if
extra()is tied to a Django queryset, but AFAIK Ruff is not able to resolve classes outside of the current module.Test Plan
Snapshot tests