Modify diagnostic ranges for shell-related bandit rules
#10667
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
| code | total | + violation | - violation | + fix | - fix |
|---|---|---|---|---|---|
| ANN101 | 35382 | 0 | 35382 | 0 | 0 |
| ANN102 | 674 | 0 | 674 | 0 | 0 |
| S603 | 454 | 227 | 227 | 0 | 0 |
| S602 | 21 | 11 | 10 | 0 | 0 |
| S604 | 16 | 8 | 8 | 0 | 0 |
| S605 | 8 | 4 | 4 | 0 | 0 |
| D107 | 2 | 1 | 1 | 0 | 0 |
| RUF100 | 1 | 1 | 0 | 0 | 0 |
Linter (preview)
ℹ️ ecosystem check detected linter changes. (+251 -249 violations, +0 -0 fixes in 5 projects; 45 projects unchanged)
apache/airflow (+171 -171 violations, +0 -0 fixes)
ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview --select ALL
+ airflow/cli/commands/dag_command.py:309:14: S603 `subprocess` call: check for execution of untrusted input - airflow/cli/commands/dag_command.py:309:31: S603 `subprocess` call: check for execution of untrusted input + airflow/cli/commands/info_command.py:199:18: S603 `subprocess` call: check for execution of untrusted input - airflow/cli/commands/info_command.py:199:35: S603 `subprocess` call: check for execution of untrusted input + airflow/cli/commands/internal_api_command.py:166:17: S603 `subprocess` call: check for execution of untrusted input - airflow/cli/commands/internal_api_command.py:166:34: S603 `subprocess` call: check for execution of untrusted input + airflow/cli/commands/internal_api_command.py:179:22: S603 `subprocess` call: check for execution of untrusted input - airflow/cli/commands/internal_api_command.py:179:39: S603 `subprocess` call: check for execution of untrusted input + airflow/cli/commands/standalone_command.py:267:24: S603 `subprocess` call: check for execution of untrusted input ... 294 additional changes omitted for rule S603 + airflow/example_dags/example_kubernetes_executor.py:132:35: S605 Starting a process with a shell: seems safe, but may be changed in the future; consider rewriting without `shell` - airflow/example_dags/example_kubernetes_executor.py:132:45: S605 Starting a process with a shell: seems safe, but may be changed in the future; consider rewriting without `shell` + airflow/example_dags/example_kubernetes_executor.py:94:27: S605 Starting a process with a shell: seems safe, but may be changed in the future; consider rewriting without `shell` - airflow/example_dags/example_kubernetes_executor.py:94:37: S605 Starting a process with a shell: seems safe, but may be changed in the future; consider rewriting without `shell` + airflow/providers/apache/beam/hooks/beam.py:575:25: S604 Function call with `shell=True` parameter identified, security issue - airflow/providers/apache/beam/hooks/beam.py:577:13: S604 Function call with `shell=True` parameter identified, security issue + dev/breeze/src/airflow_breeze/commands/kubernetes_commands.py:1082:13: S604 Function call with `shell=True` parameter identified, security issue - dev/breeze/src/airflow_breeze/commands/kubernetes_commands.py:1091:17: S604 Function call with `shell=True` parameter identified, security issue + dev/breeze/src/airflow_breeze/commands/kubernetes_commands.py:1094:13: S604 Function call with `shell=True` parameter identified, security issue - dev/breeze/src/airflow_breeze/commands/kubernetes_commands.py:1103:17: S604 Function call with `shell=True` parameter identified, security issue + dev/breeze/src/airflow_breeze/commands/kubernetes_commands.py:1106:13: S604 Function call with `shell=True` parameter identified, security issue - dev/breeze/src/airflow_breeze/commands/kubernetes_commands.py:1115:17: S604 Function call with `shell=True` parameter identified, security issue - dev/breeze/src/airflow_breeze/commands/release_candidate_command.py:160:87: S604 Function call with `shell=True` parameter identified, security issue ... 8 additional changes omitted for rule S604 + hatch_build.py:660:13: S602 `subprocess` call with `shell=True` identified, security issue - hatch_build.py:660:59: S602 `subprocess` call with `shell=True` identified, security issue + hatch_build.py:673:13: S602 `subprocess` call with `shell=True` identified, security issue - hatch_build.py:673:59: S602 `subprocess` call with `shell=True` identified, security issue + scripts/ci/pre_commit/ruff_format.py:26:1: S602 `subprocess` call with `shell=True` identified, security issue - scripts/ci/pre_commit/ruff_format.py:26:33: S602 `subprocess` call with `shell=True` identified, security issue + tests/dags/test_on_kill.py:44:13: S605 Starting a process with a shell: seems safe, but may be changed in the future; consider rewriting without `shell` - tests/dags/test_on_kill.py:44:23: S605 Starting a process with a shell: seems safe, but may be changed in the future; consider rewriting without `shell` + tests/system/providers/amazon/aws/example_emr_eks.py:102:13: S602 `subprocess` call with `shell=True` identified, security issue - tests/system/providers/amazon/aws/example_emr_eks.py:104:9: S602 `subprocess` call with `shell=True` identified, security issue + tests/system/providers/amazon/aws/example_emr_eks.py:120:13: S602 `subprocess` call with `shell=True` identified, security issue ... 8 additional changes omitted for rule S602 - tests/task/task_runner/test_standard_task_runner.py:340:19: S605 Starting a process with a shell, possible injection detected ... 308 additional changes omitted for project
bokeh/bokeh (+38 -38 violations, +0 -0 fixes)
ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview --select ALL
+ examples/output/apis/server_document/flask_server.py:45:17: S603 `subprocess` call: check for execution of untrusted input - examples/output/apis/server_document/flask_server.py:46:5: S603 `subprocess` call: check for execution of untrusted input + release/system.py:43:18: S602 `subprocess` call with `shell=True` identified, security issue - release/system.py:43:34: S602 `subprocess` call with `shell=True` identified, security issue - scripts/hooks/install.py:5:20: S603 `subprocess` call: check for execution of untrusted input + scripts/hooks/install.py:5:5: S603 `subprocess` call: check for execution of untrusted input + scripts/hooks/protect_branches.py:10:22: S603 `subprocess` call: check for execution of untrusted input - scripts/hooks/protect_branches.py:10:26: S603 `subprocess` call: check for execution of untrusted input ... 69 additional changes omitted for rule S603 ... 68 additional changes omitted for project
freedomofpress/securedrop (+2 -0 violations, +0 -0 fixes)
ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview
+ devops/scripts/verify-mo.py:116:16: S602 `subprocess` call with `shell=True` identified, security issue + devops/scripts/verify-mo.py:120:26: RUF100 [*] Unused `noqa` directive (unused: `S602`)
rotki/rotki (+3 -3 violations, +0 -0 fixes)
ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview
+ packaging/docker/entrypoint.py:144:11: S603 `subprocess` call: check for execution of untrusted input - packaging/docker/entrypoint.py:144:28: S603 `subprocess` call: check for execution of untrusted input - packaging/docker/entrypoint.py:166:26: S603 `subprocess` call: check for execution of untrusted input + packaging/docker/entrypoint.py:166:9: S603 `subprocess` call: check for execution of untrusted input - packaging/docker/entrypoint.py:174:52: S602 `subprocess` call with `shell=True` seems safe, but may be changed in the future; consider rewriting without `shell` + packaging/docker/entrypoint.py:174:9: S602 `subprocess` call with `shell=True` seems safe, but may be changed in the future; consider rewriting without `shell`
zulip/zulip (+37 -37 violations, +0 -0 fixes)
ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview --select ALL
+ scripts/lib/check_rabbitmq_queue.py:143:26: S603 `subprocess` call: check for execution of untrusted input - scripts/lib/check_rabbitmq_queue.py:144:9: S603 `subprocess` call: check for execution of untrusted input + scripts/lib/check_rabbitmq_queue.py:160:23: S603 `subprocess` call: check for execution of untrusted input - scripts/lib/check_rabbitmq_queue.py:161:9: S603 `subprocess` call: check for execution of untrusted input + scripts/lib/hash_reqs.py:38:12: S603 `subprocess` call: check for execution of untrusted input - scripts/lib/hash_reqs.py:38:36: S603 `subprocess` call: check for execution of untrusted input + scripts/lib/puppet_cache.py:25:30: S603 `subprocess` call: check for execution of untrusted input - scripts/lib/puppet_cache.py:27:9: S603 `subprocess` call: check for execution of untrusted input + scripts/lib/setup_venv.py:179:31: S603 `subprocess` call: check for execution of untrusted input - scripts/lib/setup_venv.py:179:55: S603 `subprocess` call: check for execution of untrusted input ... 64 additional changes omitted for project
Changes by rule (5 rules affected)
| code | total | + violation | - violation | + fix | - fix |
|---|---|---|---|---|---|
| S603 | 454 | 227 | 227 | 0 | 0 |
| S602 | 21 | 11 | 10 | 0 | 0 |
| S604 | 16 | 8 | 8 | 0 | 0 |
| S605 | 8 | 4 | 4 | 0 | 0 |
| RUF100 | 1 | 1 | 0 | 0 | 0 |
Formatter (stable)
✅ ecosystem check detected no format changes.
Formatter (preview)
✅ ecosystem check detected no format changes.
charliermarsh
force-pushed
the
charlie/bandit
branch
from
ef6415c to
1de2e90
Compare
dhruvmanila
reviewed
Apr 1, 2024
There was a problem hiding this comment.
This has the side effect that some of the noqa comments would need to be moved like in the case highlighted by the ecosystem checks: https://github.com/freedomofpress/securedrop/blob/da8ac79a2017001e017b2a742fcb8c81b2f3e7b0/devops/scripts/verify-mo.py#L120:
return subprocess.run( # <- The below `noqa` comment should be here now
cmd,
capture_output=True,
env=os.environ,
shell=True, # noqa: S602 # <- This `noqa` command needs to be moved up
)This now raises RUF100 [*] Unused noqa directive (unused: S602).
I think this change should go either under preview or in the next minor release.
MichaReiser
approved these changes
Apr 2, 2024
Comment on lines
463
to
459
| arguments: &'a Arguments, | ||
| semantic: &SemanticModel, | ||
| ) -> Option<ShellKeyword<'a>> { | ||
| fn find_shell_keyword(arguments: &Arguments, semantic: &SemanticModel) -> Option<ShellKeyword> { | ||
| arguments.find_keyword("shell").map(|keyword| ShellKeyword { | ||
| truthiness: Truthiness::from_expr(&keyword.value, |id| semantic.is_builtin(id)), | ||
| keyword, | ||
| }) | ||
| } |
There was a problem hiding this comment.
Nit: Rename to evaluate_shell_keywrod and return a Option<Truthiness> directly. The ShellKeyword type seems superfluous now that it only stores Truthiness
| 20 | subprocess.getoutput("true") | ||
| 21 | subprocess.getstatusoutput("true") | ||
| | | ||
|
|
||
| S605.py:20:22: S605 Starting a process with a shell: seems safe, but may be changed in the future; consider rewriting without `shell` | ||
| S605.py:20:1: S605 Starting a process with a shell: seems safe, but may be changed in the future; consider rewriting without `shell` |
There was a problem hiding this comment.
To me it's now unclear what shell is referring to. What I assumed from before is that the first argument ("true") is the shell argument. Now, that context is lost. I'm not saying that we shouldn't make this change but it might require fine-tuning the diagnostic message as well.
|
To be clear, the motivation here isn't achieving complete compatibility with another tool. Some of our ranges are still different. The motivation is that the current ranges don't really make sense (why highlight (We definitely can't ship these without a minor bump though.) |
MichaReiser
force-pushed
the
charlie/bandit
branch
from
1de2e90 to
d701d1c
Compare
|
This makes sense to me. I'll pick this for 0.5 |
Merged
MichaReiser
pushed a commit
that referenced
this pull request
Jun 27, 2024
MichaReiser
pushed a commit
that referenced
this pull request
Jun 27, 2024
This was referenced Jun 28, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary
The rules that flag shell calls now center on the function name, rather than the shell argument (which isn't really relevant -- especially it it's falsy). The rules that center on specific arguments (e.g., a wildcard in a string) now highlight the argument.
Closes #9994.