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

Corrected documentation on configuration #868

Merged
merged 4 commits into from
Apr 24, 2022
Merged

Conversation

a-takahashi223
Copy link
Contributor

As mentioned in #606 (comment) , there are some errors in the configuration documentation.
This has been corrected.
Note that only the documentation has been corrected, not the man page or help messages.

"Profile" is deprecated, legacy and undocumented.
Note: decriptions about profile are still in man pages or help messages.
@a-takahashi223
Copy link
Contributor Author

Hello.

Is there any problem with this PR?
I am not a native English speaker, so English corrections are welcome.

@a-takahashi223
Copy link
Contributor Author

Hi @ericwb @lukehinds @ghugo @sigmavirus24 ,

Am I violating the contribute guideline?

I read https://bandit.readthedocs.io/en/latest/config.html trying to get Bandit to ignore B101 and was very annoyed that it didn't work.
Others may have the same problem.

If such problems are reduced, Bandit, which is a great tool, will be easier to use.
Let me help you with that.

@sigmavirus24
Copy link
Member

Am I violating the contribute guideline?

I don't see a violation here but I'm also carrying for my newborn child and not paying close attention. Did someone tell you that you were?

@a-takahashi223
Copy link
Contributor Author

I'm also carrying for my newborn child

Congratulations on the birth of your baby. That must be very busy.

Did someone tell you that you were?

No, but this PR seems to be ignored, and I was concerned that the contribution guideline violation might be the reason.

Copy link
Member

@ericwb ericwb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks mostly good, just open issue on the removal of the -p example.

@@ -40,11 +40,6 @@ context and only reporting on the high-severity issues::

bandit examples/*.py -n 3 -lll

Bandit can be run with profiles. To run Bandit against the examples directory
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bandit does still have an option to specify profiles to load via the -p or --profile argument.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I cloned PyCQA/bandit:main, ran python setup.py install and pip -r requirements.txt, then ran bandit examples/*.py -p ShellInjection. The output is below:

[main]  ERROR   Unable to find profile (ShellInjection) in config file: None

This result suggests that profile is not built-in, but something we define ourselves. So how do we define it?
Where is it written in the documentation?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, creating profiles is still possible via the bandit.yaml file. They are not built-in. And you can create a config yaml file bandit-config-generator

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you. I ran bandit-config-generator -o bandit.yaml then bandit examples/*.py -p ShellInjection -c bandit.yaml. The output is:

[main]  ERROR   Unable to find profile (ShellInjection) in config file: bandit.yaml

Again, how can I define profile in a configuration file?

After some trial and error, a configuration file as below finally worked.

profiles:
  ShellInjection:
    # ...

But I got the message below.

[config]        WARNING Config file 'bandit.yaml' contains deprecated legacy config data. Please consider upgrading to the new config format. The tool 'bandit-config-generator' can help you with this. Support for legacy configs will be removed in a future bandit version.

This is probably the best evidence that profile is deprecated and should not be used.
Therefore, all descriptions about profile should be removed from the documentation.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even if it's "deprecated" (which it isn't) it shouldn't be removed in this pull request

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. Reverted in 2d6cd55

Copy link
Member

@ericwb ericwb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

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

Successfully merging this pull request may close these issues.

3 participants