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.

Sign up for GitHub

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

Jump to bottom

bandit check seems to link arguments not function call #9994

Open
Borda opened this issue Feb 15, 2024 · 5 comments · May be fixed by #10667
Open

bandit check seems to link arguments not function call #9994

Borda opened this issue Feb 15, 2024 · 5 comments · May be fixed by #10667
Assignees
@charliermarsh
Labels
bug Something isn't working

Comments

@Borda
Copy link

Borda commented Feb 15, 2024

It seems that Ruff reports wrong lines for this bandit issue

out = subprocess.check_output(  # <-- yesqa expected
    ["git", "rev-parse", "HEAD"],  # <-- ruff reported
    stderr=DEVNULL,
)

in fact, it creates deadlock as yesqa removes noqas from line 2 as it expects them with line 1, but then ruff reports line 2

2: S603 `subprocess` call: check for execution of untrusted input
2: S607 Starting a process with a partial executable path

adding a dummy line to the argument, and it seems to be baked with the first argument as this

out = subprocess.check_output(  # <-- yesqa expected
	print(), # <-- ruff reported
    ["git", "rev-parse", "HEAD"],
    stderr=DEVNULL,
)

still reports line 2 but the next one

out = subprocess.check_output(  # <-- yesqa expected
	# any line
    ["git", "rev-parse", "HEAD"],  # <-- ruff reported
    stderr=DEVNULL,
)

reports line 3

--

using pre-commit with

  - repo: https://github.com/asottile/yesqa
    rev: v1.5.0
    hooks:
      - id: yesqa
        additional_dependencies:
          - flake8-bandit

  - repo: https://github.com/astral-sh/ruff-pre-commit
    rev: v0.2.1
    hooks:
      - id: ruff
        args: ["--fix"]
This was referenced Feb 15, 2024
Merged
Merged
@charliermarsh charliermarsh added the bug Something isn't working label Feb 16, 2024
@charliermarsh
Copy link
Member

Interesting, will take a look.

@dhruvmanila
Copy link
Member

It seems like bandit highlights the subprocess.check_output call in the diagnostic and we highlight the argument. And, yesqa uses the location bandit provides to check if NoQA comment is valid or not.

$ bat src/S603.py 
         File: src/S603.py
     1   import subprocess
     2   
     3   out = subprocess.check_output(
     4       ["git", "rev-parse", "HEAD"],
     5   )

$ flake8 src/S603.py
src/S603.py:1:1: S404 Consider possible security implications associated with the subprocess module.
src/S603.py:3:1: S607 Starting a process with a partial executable path
src/S603.py:3:1: S603 subprocess call - check for execution of untrusted input.

A simple fix would be to use the same location.

@charliermarsh
Copy link
Member

Note that in general, using yesqa alongside Ruff isn't necessarily a supported workflow (for this reason: we don't guarantee that our rules trigger on the exact same locations, since we sometimes refine the ranges).

@dhruvmanila
Copy link
Member

You can use our own implementation of unused-noqa if yesqa isn't strictly required in your workflow.

@charliermarsh
Copy link
Member

@dhruvmanila - I'm not totally sure which (if any) of these rules should have their ranges adjusted -- would need to look...

@charliermarsh charliermarsh linked a pull request Mar 30, 2024 that will close this issue
Open
@charliermarsh charliermarsh self-assigned this Mar 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@charliermarsh charliermarsh

Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Modify diagnostic ranges for shell-related bandit rules astral-sh/ruff
3 participants
@charliermarsh @Borda @dhruvmanila