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

Add support for nosec comments for bandit rules #12743

Open
Daverball opened this issue Aug 8, 2024 · 2 comments
Open

Add support for nosec comments for bandit rules #12743

Daverball opened this issue Aug 8, 2024 · 2 comments
Labels
needs-decision Awaiting a decision from a maintainer suppression Related to supression of violations e.g. noqa

Comments

@Daverball
Copy link

I already brought this up a while back in the issue for the flake8-bandit rules, which has recently been completed.

I personally think it would be a good idea to support them, because they provide better visibility for potential security flaws in the code than their equivalent noqa comment, which I find easy to gloss over by comparison. It also means improved interoperability between bandit and ruff without the need for flake8-bandit as a compatibility shim.

There was some push back against the idea last time, but I wanted to try again, since ruff has has undergone some big changes in relevant portions like the parser in the meantime, from what I can tell ruff already recognizes nosec as a pragma so the cost of adding support should be overall a bit lower now.

I'd be happy to give the implementation a try myself, if there is no significant push back against the idea.

The most difficult thing to support will probably be a bare nosec without a code. Since that should only silence bandit rules, so it can't be equivalent to a bare noqa. For the rest of the rules it should be possible to treat it as an alias to an equivalent noqa rule.

Although there's also a broader question of the interaction with flake8-noqa. Should nosec comments be able to trigger NQA10X rules? Or do we add some ruff specific rules to tidy up nosec rules? bandit itself does emit warnings for nosec comments, where the supplied rule doesn't apply (although there are some false positives).

@charliermarsh charliermarsh added the suppression Related to supression of violations e.g. noqa label Aug 9, 2024
@charliermarsh
Copy link
Member

I'm not totally opposed if the implementation doesn't add much complexity. What do you think @MichaReiser?

@charliermarsh charliermarsh added the needs-decision Awaiting a decision from a maintainer label Aug 9, 2024
@MichaReiser
Copy link
Member

I feel hesitant about adding new suppression pragmas without knowing the outcome of #1774. The bandit group will likely be gone afterward, and it then becomes unclear why some rules can be suppressed by nosec while others can't. Unless we make noses an alias of no,, but that misses the author's point of having something more explicit around security-related suppressions.

Rule categorization might also solve this if we allow noqa with rule names. E.g. noqa: security/some-rule would make for an easy grepable string.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-decision Awaiting a decision from a maintainer suppression Related to supression of violations e.g. noqa
Projects
None yet
Development

No branches or pull requests

3 participants