Codes B301-B306 conflict with openstack/bandit (via. flake8-bandit)

[myii]

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 while bugbear is deactivated
• If I uninstall bandit, bugbear is activated and works as expected

[myii]

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

1. Search and replace in bugbear.py to modify B30 => 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, ...) ...

1. Have only just put this workaround together so very little testing done -- but I am seeing the B950 I was expecting...

[myii]

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)

[ambv]

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.

[myii]

@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:

1. https://github.com/openstack/bandit

   1. Project started on 16 July 2014
   2. This is the main project, where the conflicts are arising

2. https://github.com/tylerwince/flake8-bandit

   1. Project started on 29 Oct 2017
   2. However, this is just a wrapper around bandit
   3. 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:

1. Bandit: openstack-archive/bandit@c364408

   1. This commit was made on 22 Jan 2016

2. Bugbear: 0fb7d8d

   1. 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.

[tylerwince]

@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

[myii]

@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.

[myii]

@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

[tylerwince]

@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.

[myii]

@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
Owner

Are 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-letter RST 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.

[ambv]

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:

1. either flake8-bandit renames bandit Bxxx codes into Sxxx; or
2. flake8-bugbear renames its codes into BBxxx; or
3. 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?

[myii]

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.

1. 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.

2. flake8-bugbear renames its codes into BBxxx

Grossly inappropriate, as you've gone on to mention.

3. 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 range FI10-FI90
  • Is there any advantage to providing the violation codes in this implicit fashion?

• In any case, Sxxx looks like it could work, since the lowest number that would be used (after conversion) is S101

  • 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

[tylerwince]

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/flake8-bandit@2cc5ae5

[myii]

@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.

[sigmavirus24]

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?

[tylerwince]

Thanks @sigmavirus24, I submitted a PR here: https://gitlab.com/pycqa/flake8/merge_requests/229

Open to any changes or suggestions. Thanks!

[myii]

@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:

$$1-letter + 3-digit: 26,000 combinations 2-letter + 3-digit: 676,000 combinations 3-letter + 3-digit: 17,576,000 combinations = A lot less conflicts = Note: last method actually includes the previous two to give almost 18.28M combos$$

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.
• 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

[tylerwince]

@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.

[myii]

@tylerwince You're welcome. Thanks for incorporating my suggestions -- I hope the changes are appropriate.

[myii]

@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.