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

Usage needlessly complicated #606

Open
lietu opened this issue Apr 28, 2020 · 6 comments
Open

Usage needlessly complicated #606

lietu opened this issue Apr 28, 2020 · 6 comments
Labels
enhancement New feature or request

Comments

@lietu
Copy link

lietu commented Apr 28, 2020

Is your feature request related to a problem? Please describe.
When setting up Bandit the usage is needlessly complicated. This is made even worse when I want to use things like pre-commit or otherwise get colleagues to use the same tools, as the configuration files are a mess.

  1. There is no example that I can find for the -c config file. I tried to use the only format documented, the INI format, and it complained about some document start not being found, which sounds like it expects XML. I see no examples of XML configuration.

  2. The INI format config is supposed to be loaded by default from .bandit, it's not. I need to add --ini .bandit or I get the completely false error message No targets found in CLI or ini files, exiting. .. considering it says path to a .bandit file that supplies command line arguments, AND the error claims to "not find" ini files, this should clearly be loaded by default.

  3. The INI format configuration is not well documented, just some handwavy "comma separated list of blah" instead of showing actual examples for most of the usage, and it fails to specify that it does not support comments at the end of lines.

E.g. this causes an error:

[bandit]
skips: B101  # Assertion used
  1. The INI format configuration does not support proper glob expressions nor does it support the -r argument, causing really stupid looking configuration and significant astonishment at the difficulties to get it to work.

This causes an error

[bandit]
targets: **/*.py
Files skipped (1):
        **/*.py (Invalid argument)

This does nothing:

[bandit]
exclude: **/tests

This also fails to work, because directories cannot be excluded:

[bandit]
exclude: tests,*/tests,*/*/tests,*/*/*/tests,... for as many levels as you guess your app will need

This also fails because the spaces are not trimmed around the ,

[bandit]
exclude: tests/*, */tests/*, */*/tests/*, */*/*/tests/*, ... for as many levels as you guess your app will need

This seems to work though, if your code is e.g. in app directory and IF used with bandit --ini .bandit -r:

[bandit]
include: app
exclude: app/tests/*,app/*/tests/*,app/*/*/tests/*,app/*/*/*/tests/*,... for as many levels as you guess your app will need
  1. Configuration is not handled well when used with other tools. I want to automatically run this with pre-commit, but Bandit ignores the exclude from the INI file even with the explicitly set --ini .bandit arg, I imagine because pre-commit gives it a list of files as arguments, so I have to re-do the exclude in .pre-commit-config.yaml.
  - repo: https://github.com/PyCQA/bandit
    rev: 'master'
    hooks:
      - id: bandit
        args: ['--ini', '.bandit', '-r']
        exclude: >
          (?x)^(
            .*/tests/.*
          )$

You should be able to tell by now why this just feels wrong on so many levels and is at the very least needlessly complicated.

Describe the solution you'd like

  1. bandit . -r should be the default operation, with something like --no-recurse to override the recursion
  2. .bandit configuration (and I really hope soon pyproject.toml configuration) should be read automatically
  3. -c configuration example file should be in the repo's README, and the --ini should be properly documented as well
  4. .bandit and other configuration should be respected regardless of if bandit gets a list of filenames as arguments or not, though of course it should be possible to override configuration when necessary .. the expectation is that targets: app with -r and app/file.py as argument have the same effect with the same configuration
  5. .bandit and other configuration formats should be fully capable of making the tool run properly by default without requiring constant manual edits, i.e. they should support the recursion (if 1. is implemented less important), and they should support proper globs both for targets and exclude
  6. Configuration parsing should support common usage, such as spaces around separators, and comments at end of lines
@lietu
Copy link
Author

lietu commented Apr 28, 2020

Actually one more thing just popped in my mind, that would make this better: the only reason I skip tests at all is B101, which probably should be enabled for most of the project, but I don't want every test to cause warnings, nor am I going to #nosec every assert.

It would probably be nice if we could set B101 to be skipped on **/tests, or do a file level # banditskip B101 or something to validate test code without unwanted warnings

Even better would be automatically skipping B101 on all unittest code, but automatically detecting that would likely not be easy.

@ericwb ericwb added the enhancement New feature or request label Apr 29, 2020
@cc-stjm
Copy link

cc-stjm commented Apr 30, 2020

Actually one more thing just popped in my mind, that would make this better: the only reason I skip tests at all is B101, which probably should be enabled for most of the project, but I don't want every test to cause warnings, nor am I going to #nosec every assert.

It would probably be nice if we could set B101 to be skipped on **/tests, or do a file level # banditskip B101 or something to validate test code without unwanted warnings

Even better would be automatically skipping B101 on all unittest code, but automatically detecting that would likely not be easy.

Agree with all of the above, but I think that just dealing with #346 nicely would solve your need for configuration, and mine too...

@diegovalenzuelaiturra
Copy link

diegovalenzuelaiturra commented Nov 16, 2021

Hi, the following may be helpful to configure bandit to avoid raising B101 assert_used warnings on python tests.

@a-takahashi223
Copy link
Contributor

The documentation should cover such a topic.
But the "Configuration" page of the documentation has many mistakes.

Projects may include a YAML file named .bandit

No. .bandit is a INI file. The example below that sentence won't work.
Also, a "project" is recognized only bandit is invoked with -r option. Without -r, bandit doesn't look for .bandit.

In Scanning Behavior section, example YAML configuration is shown. And bandit-config-generator introduced in Generating a Config section generates an YAML file.
The reader of the documentation would think these YAMLs are .bandit. No.
YAML configuration file is loaded with -c option. No such thing is mentioned anywhere.

In Usage section,

Bandit can be run with profiles.

What is a profile? How can I define it? Not explained anywhere.
YAML loaded with -c seems to be the only nameless profile.

@hofrob
Copy link

hofrob commented Jun 1, 2022

I just want to add, that with more and more tools allowing the whole config to be in a pyproject.toml, I would like to see this to be the default. Running bandit . should look for a .bandit for those who want to keep it. Next look for a [tool.bandit] section in pyproject.toml.

The major reason behind this is, I think it makes life easier for those who work with CI jobs. Having one simple command (like <tool_name> .) and all the config in one file makes a lot of difference for us. Also I don't want to bother other devs and explain all the different cli args 😀.

@CTimmerman
Copy link

I also suggested config file detection order in #317 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

7 participants