Codes B301-B306 conflict with openstack/bandit (via. flake8-bandit)
myii opened this issue ยท 19 comments
Similar situation to #20, there are conflicts across codes B301
-B306
.
https://github.com/openstack/bandit:
The following tests were discovered and loaded: ... B301 pickle B302 marshal B303 md5 B304 ciphers B305 cipher_modes B306 mktemp_q
- https://github.com/tylerwince/flake8-bandit is "a flake8 wrapper around [bandit]"
In my situation:
- When both are installed,
bandit
is still available whilebugbear
is deactivated - If I uninstall
bandit
,bugbear
is activated and works as expected
I've tested a workaround, which allows both to be activated at the same time. I'm only mentioning it here for informational purposes -- in no way am I expecting this to be a proper solution.
Modified B301
-B306
to use B351
-B356
instead
- Search and replace in
bugbear.py
to modifyB30
=>B35
Modified the entry points to differ from flake8-bandit
bandit
:
B = flake8_bandit:BanditTester
bugbear
-- before modification:
B = bugbear:BugBearChecker
bugbear
-- after modification:
B00 = bugbear:BugBearChecker
B35 = bugbear:BugBearChecker
B9 = bugbear:BugBearChecker
Result
$ flake8 --version
3.5.0 (..., flake8-bandit: v1.0.1, flake8-bugbear: 18.2.0, ...) ...
- Have only just put this workaround together so very little testing done -- but I am seeing the
B950
I was expecting...
Asides from the fact that I'm seeing each error in triplicate, everything's just peachy...!
./.../settings.py:99:92: B950 line too long (91 > 79 characters)
./.../settings.py:99:92: B950 line too long (91 > 79 characters)
./.../settings.py:99:92: B950 line too long (91 > 79 characters)
./.../settings.py:113:91: B950 line too long (90 > 79 characters)
./.../settings.py:113:91: B950 line too long (90 > 79 characters)
./.../settings.py:113:91: B950 line too long (90 > 79 characters)
Every plugin should register only once, hence you're seeing triplicates if you register multiple times.
Bugbear had been around for a few years now.
flake8-bandit is a new plugin, it should change its prefix to something that doesn't conflict.
@ambv Thanks for the response. Unfortunately, I haven't been clear in my explanation above.
The conflict is not with flake8-bandit
but rather with openstack/bandit
There are two projects that are involved here:
-
https://github.com/openstack/bandit
- Project started on 16 July 2014
- This is the main project, where the conflicts are arising
-
https://github.com/tylerwince/flake8-bandit
- Project started on 29 Oct 2017
- However, this is just a wrapper around
bandit
- They have no control over the error codes that are supplied by
bandit
The OpenStack Bandit project has been using B30x
codes for a few years as well
Taking B301
as the earliest example in both projects:
-
Bandit: openstack-archive/bandit@c364408
- This commit was made on 22 Jan 2016
-
Bugbear: 0fb7d8d
- This commit was made on 8 Jun 2016
The triplicate issue is just extra noise from my personal workaround
Please ignore my workaround above; I was just sharing a method that allowed me to get both bugbear
and bandit
working at the same time on my system. I fully understand that I'm getting triplicate because of the extra registrations but I'm happy to have that rather than choose between both of these excellent plugins.
@myii @ambv Thanks for pointing this out.
We could always handle this internally in flake8-bandit
. Definitely not a long term solution but a workaround until we can figure out which codes to be used by each project.
What are the thoughts around flake8-bandit
changing the openstack/bandit
code to be S30x
for the time being? (quick look and it doesn't appear any other plugins are using S30x and S makes sense for "security")
Is anyone using flake8-bandit
and comparing those results to the openstack/bandit
cli output? That is the only time I could see this causing an issue as the codes won't match up
I've opened up an issue with openstack/bandit
to see if we can pull them into the discussion here: https://bugs.launchpad.net/bandit/+bug/1759643
@tylerwince Appreciate the prompt response.
This is incredibly naive, since I'm unfamiliar with the internals of Flake8 (see my wonderful workaround above) but is there any mileage in simply using something like BAN
as the entry point for all bandit
codes? Something like:
BAN = flake8_bandit:BanditTester
That way, nothing would have to be done for either of the well-established bugbear
and bandit
projects. The coercion would be done within flake8-bandit
, with a simple conversion of B***
to BAN***
.
I've actually had a quick go at the above and it seems to hold together. However, I don't know how that works for users who want the exact bandit
codes.
@tylerwince Something like the following:
setup.py
:
@@ -78,7 +78,7 @@
license="MIT",
entry_points={
"flake8.extension": [
- "B=flake8_bandit:BanditTester",
+ "BAN=flake8_bandit:BanditTester",
],
},
classifiers=[
flake8_bandit.py
:
@@ -42,8 +42,8 @@
issues = []
for item in b_mgr.get_issue_list():
i = {}
- i["test_id"] = item.test_id
- i["issue_text"] = item.text
+ i["test_id"] = item.test_id.replace('B', 'BAN')
+ i["issue_text"] = 'Bandit [{0}]: {1}'.format(item.test_id, item.text)
i["line_number"] = item.lineno
issues.append(i)
try:
Sample output:
./.../setup.py:14:1: BAN110 Bandit [B110]: Try, Except, Pass detected.
- This way, at least the exact
bandit
code is displayed in text of the error
@myii I am not sure we want the flake8 code to be longer than 4 characters since that seems to be the standard -- this way the output is easier to visually scan.
I am good with the idea of throwing the actual error code in the issue_text
, but let's just change the test_id
to be 'S' instead of 'B' until we hear back from the folks at openstack/bandit
. This will enable people to continue using both flake8-bugbear
and flake8-bandit
in harmony for the time being.
I can throw a PR together later today or if you want to take a stab at it I will probably be able to accept it and push it to PyPI sooner.
@tylerwince OK, one thing I am certain about is that Flake8 violation codes can be longer than 4 characters -- it is no longer a fixed standard since v3.0+. The following discussion involves the author of Flake8 itself.
https://gitlab.com/pycqa/flake8/issues/337
Ian Stapleton Cordasco @sigmavirus24 commented 9 months ago
OwnerAre there conventions on plugin naming I should be aware of?
None whatsoever. There are many plugins named flake8- but also plenty that don't have that prefix, e.g., pycodestyle, mccabe, pyflakes, radon.
Are there conventions on the error code naming I should be aware of?
Try to find an unused prefix.
Could an RST validator use the prefix R (e.g. R101, R102, ...) or is that taken?
Neither Flake8 nor I maintain a registry of prefixes. I don't have the time for that, nor do I have time to arbitrate arguments over who used it first or what to do if one plugin is no longer maintained.
I believe the R prefix is used by radon if memory serves me well. Perhaps use RST as the prefix. Flake8 3.0+ doesn't enforce 4 character long violation codes.
Peter Cock @pjacock commented 9 months ago
I've opened #339 looking at the prefix conventions and improving the plugin development documentation about this.
Given since v3.0 Flake8 isn't restricted to 4 character long violation codes, I like your suggestion of using RST as the prefix here.
-
The emphasis above is mine
-
I'm using the
flake8-rst-docstrings
plugin mentioned above and it uses the 3-letterRST
prefix -
At some point, I came across another discussion, that was very much in favour of using longer prefixes, since there aren't enough to go around all of the plugins that are now available -- unfortunately, I don't have time to track that down right now
As for the modifications, I'm a little surprised but on my system, it seems sufficient to simply modify the entry point without having to change flake8_bandit.py
at all. In fact, I'm actually having trouble displaying the Flake8 errors in Vim (Syntastic) when I make the changes mentioned in my previous comment, even though running flake8
from the command line displays the errors. Yet, if I limit the change to the entry point only, it works from the command line and in Vim.
Whoa, quite a discussion here, folks!
We're using Flake8 extensively where I work so I have some experience with this:
- changing the entry point seems to work because both entry points get registered; but then your user configs can't for example ignore Bugbear's B3xx warnings while keeping Bandit's B3xx warnings. As far as Flake8 is concerned, both are the same warning now.
- changing codes to use more than one character might not work with your editor integrations (like Syntastic) because the authors often use a very limited regex that was only tested with the builtin warnings. It would be helpful if Flake8 itself shipped with multi-letter warnings, this way educating integrators that matching a single uppercase letter and three digits is not enough.
Given the two problems I outlined above, I see two possibilities:
- either flake8-bandit renames bandit Bxxx codes into Sxxx; or
- flake8-bugbear renames its codes into BBxxx; or
- we merge the two plugins? Security problems is an interesting class that I think could fit in Bugbear, too.
If we choose the first solution, that solves the error code clash forever.
If we choose to rename Bugbear codes, the .flake8 configuration of existing projects in the wild will become invalid. Plus, I will have to deal with non-working integrations like outlined above.
If we choose to merge, renaming B30x in Bugbear to B350 is only going to help in the short term because as soon as Bandit reaches B351 then we're conflicting again.
In general, the age of Bandit itself doesn't matter in this issue since this is a separate tool unrelated to Flake8 and can use whatever output it desires. It doesn't have to compose.
I think 1. is easiest, but 3. is probably wisest long-term. What do you think?
Whoa, quite a discussion here, folks!
@ambv Yeah, sorry for spamming your repo!
- changing the entry point seems to work because both entry points get registered; but then your user configs can't for example ignore Bugbear's B3xx warnings while keeping Bandit's B3xx warnings. As far as Flake8 is concerned, both are the same warning now.
Yes, I knew it was too good to be true -- that explanation helps clarify things.
- changing codes to use more than one character might not work with your editor integrations (like Syntastic) because the authors often use a very limited regex that was only tested with the builtin warnings.
The thing is that it works fine with other plugins, such as displaying the RSTxxx
violations, etc. So there must be a way to do it.
- either flake8-bandit renames bandit Bxxx codes into Sxxx
Since the discussion above, I've tested this and it works fine for me, CLI & Vim. My only concern is that with so many plugins, are we sure that Sxxx
won't clash with something else?
UPDATE: See below for a table showing entry points for violation codes in some of the existing plugins.
- flake8-bugbear renames its codes into BBxxx
Grossly inappropriate, as you've gone on to mention.
- we merge the two plugins? Security problems is an interesting class that I think could fit in Bugbear, too.
Only @tylerwince is in a position to respond to that.
I think 1. is easiest, but 3. is probably wisest long-term. What do you think?
Solution 1 is the obvious starting point for now, so that both can be used at the same time. After that, the affected parties can discuss possibility 3.
List of entry points for a selection of existing plugins
Extracted entry points from some of the plugins out there:
Entry point | Checker |
---|---|
A00 | flake8_builtins:BuiltinsChecker |
A40 | flake8_author:Checker |
B | bugbear:BugBearChecker |
B | flake8_bandit:BanditTester |
C001 | flake8_confusables:IdentifierChecker |
C10 | flake8_coding:CodingChecker |
C4 | flake8_comprehensions:ComprehensionChecker |
C81 | flake8_commas:CommaChecker |
C90 | mccabe:McCabeChecker |
CNL100 | flake8_class_newline:new_line_checker |
D | flake8_docstrings:pep257Checker |
D001 | flake8_deprecated:Flake8Deprecated |
F | flake8.plugins.pyflakes:FlakesChecker |
I | flake8_import_order.flake8_linter:Linter |
I00 | flake8_isort:Flake8Isort |
I20 | flake8_tidy_imports:ImportChecker |
IES | flake8_invalid_escape_sequences:plugin |
M90 | mutable_defaults:MutableDefaultChecker |
N8 | pep8ext_naming:NamingChecker |
N999 | flake8_module_name:ModuleNameChecker |
P | flake8_string_format:StringFormatChecker |
Q0 | flake8_quotes:QuoteChecker |
R70 | radon.complexity:Flake8Checker |
RST | flake8_rst_docstrings:reStructuredTextChecker |
S00 | flake8_sorted_keys:SortedKeysChecker |
S001 | flake8_pep3101:Flake8Pep3101 |
T00 | flake8_print:PrintChecker |
T000 | flake8_todo:check_todo_notes |
T100 | flake8_debugger:DebuggerChecker |
T4 | flake8_mypy:MypyChecker |
T80 | flake8_tuple:TupleChecker |
flake8-future-import | flake8_future_import:FutureImportChecker |
-
Note, there are numerous plugins that don't list the specific violation code as an entry point, such as the last entry in the table
- For
flake8-future-import
, the violation codes are in the rangeFI10
-FI90
- Is there any advantage to providing the violation codes in this implicit fashion?
- For
-
In any case,
Sxxx
looks like it could work, since the lowest number that would be used (after conversion) isS101
- With such a broad range, bound to get conflicts at some point -- probably OK for short-term use
- Conflicts can be greatly minimised by selecting a longer prefix as other plugins have done, e.g.
CNL
,IES
&RST
Just issued a quick fix to flake8-bandit
that changes the letter prefix to Sxxx
. Looks like everything is working using both plugins side-by-side at this point.
@tylerwince Thanks for that. Upgraded to 1.0.2
and all working well.
@ambv Since we've got a resolution, I'm closing this issue. Thanks for the feedback above.
I'm sorry I missed this discussion but it seems like you all hit upon the right notes throughout the conversation. Well done.
It seems like we need to update http://flake8.pycqa.org/en/latest/plugin-development/registering-plugins.html to represent this advice to plugin developers. Would someone here be willing to send an update with a note/caveat about picking an entry-point/plugin code name?
Thanks @sigmavirus24, I submitted a PR here: https://gitlab.com/pycqa/flake8/merge_requests/229
Open to any changes or suggestions. Thanks!
@tylerwince Thanks for rolling with that. Couldn't have been much fun extracting the project names for those error codes! More about that below...
To all, apologies for continuing the discussion here. I could have commented on the GitLab MR but I have some feedback that includes questions/assumptions, that would be better to confirm from the combined experience here. Most of the points are in the discussion above, so easier to quote as well.
I've offered my suggestions as either Include
to or Exclude
from the MR. With my Flake8 inexperience, please take with a pinch of salt.
Include: Entry-point conflicts can cause plugins to be deactivated
https://gitlab.com/pycqa/flake8/merge_requests/229/diffs
|Flake8| requires each entry point to be unique amongst all plugins installed in the users environment. Before defining your set of error codes, please check the list below and select a unique entry point for your plugin.
- To highlight the importance of avoiding conflicts, useful to add that these can cause plugin deactivations
Exclude: Tabularised list of entry points
https://gitlab.com/pycqa/flake8/merge_requests/229/diffs
... Before defining your set of error codes, please check the list below and select a unique entry point for your plugin.
[Tabularised list of entry points]
- Checking the list will only be worthwhile if it is both exhaustive and maintained
- With this method, that would depend on numerous MRs, as changes are continuously made
Consider from above:
Ian Stapleton Cordasco @sigmavirus24 commented 9 months ago
Owner
...
Neither Flake8 nor I maintain a registry of prefixes. I don't have the time for that, nor do I have time to arbitrate arguments over who used it first or what to do if one plugin is no longer maintained.
- So there's no official mechanism for this
- However, it would be incredibly useful if there was somewhere that this list could be collected and maintained
- @sigmavirus24 If someone was ready to take responsibility for this, would that be supported, such as linking to that resource from the docs?
- Furthermore, would there be any stipulations as to where such a repository was located, such as a specific group-based organisation?
- My take on it is that it should only be used for informational purposes, warts and all (i.e. conflicts, unmaintained plugins, etc.)
- That way, developers/users can make informed decisions about which plugins may be affected by other plugins, even if they appear unmaintained
- In fact, it will much easier for new plugins to steer clear of code ranges that have been used by any plugins, historically
Include: Flake8 3.0+ doesn't enforce 4 character long violation codes
From above:
Ian Stapleton Cordasco @sigmavirus24 commented 9 months ago
Owner
...
Flake8 3.0+ doesn't enforce 4 character long violation codes
And @ambv above:
...
It would be helpful if Flake8 itself shipped with multi-letter warnings, this way educating integrators that matching a single uppercase letter and three digits is not enough.
And @tylerwince above:
I am not sure we want the flake8 code to be longer than 4 characters since that seems to be the standard
- I also believed that the 4-characters were the standard -- until I came across the
RST
plugin and read a few of the discussions about this - IMHO, the MR should include a strong recommendation for new plugins to use three-letter prefixes
- At the very least, this information (can use more than 4-chars) must be included in some way, to minimise future conflicts
- Using this GitHub issue as an example, it is so easy for a new plugin to inadvertently deactivate a well-established plugin
- To be honest, I only stumbled upon this after a period of time, when finally realising that
bugbear
wasn't giving me any violations
- My maths isn't what it used to be but let me give it a go:
Include (unrelated): Fix 404
hyperlink
https://gitlab.com/pycqa/flake8/merge_requests/229/diffs
.. _Entry Points: https://pythonhosted.org/setuptools/pkg_resources.html#entry-points
- As I was looking at the diff, I noticed this URL directly below the modified section
- Tried the URL and it ended up in a
404
- Did a search and it appears to have moved to: http://setuptools.readthedocs.io/en/latest/pkg_resources.html#entry-points
- @tylerwince I could always prepare a separate MR but would you mind adding this fix to yours?
Tangential issue: List of Flake8 plugins to complement the list of entry points above
From above:
Ian Stapleton Cordasco @sigmavirus24 commented 9 months ago
Owner
...
There are many plugins named flake8- but also plenty that don't have that prefix, e.g., pycodestyle, mccabe, pyflakes, radon.
- No disagreement with this at all
- But when looking for Flake8 plugins, the only useful list I could get was by using the search string
flake8-
in PyPI- That meant I initially missed various plugins, including
ebb-lint
,pep8-naming
,radon
, etc.
- That meant I initially missed various plugins, including
- It would be useful to have a single place to find plugins
- If there is a positive response to
Tabularised list of entry points
above, then the same repo could be used to capture (PyPI) hyperlinks for any Flake8 plugins - In fact, could easily be integrated into collecting the list of entry points -- using the PyPI API
- If there is a positive response to
@myii Thanks for the suggestions.
I have updated the MR and added language about selecting a 3letter/3digit combination for an entry point. I will look to the maintainers (@sigmavirus24) to decide if they want to make it a strong recommendation or not by changing how it is presented.
@tylerwince You're welcome. Thanks for incorporating my suggestions -- I hope the changes are appropriate.
@ambv You hit the nail on the head about Vim Syntastic:
- changing codes to use more than one character might not work with your editor integrations (like Syntastic) because the authors often use a very limited regex that was only tested with the builtin warnings.
And I was mistaken when I said:
The thing is that it works fine with other plugins, such as displaying the
RSTxxx
violations, etc. So there must be a way to do it.
I've since tried to fix the Flake8 syntax checker used by Syntastic and I've filed a bug on their issue tracker.
No need to respond; just giving credit where it's due.