tylerwince / flake8-bandit

Automated security testing using bandit and flake8.
MIT License
111 stars 23 forks source link

Bandit 1.7.3 addition of new positional argument ``fdata`` causes ``TypeError`` #21

Closed RemyLau closed 2 years ago

RemyLau commented 2 years ago

I've been using the flake8-bandit plugin. But recently, a new positional argument fdata was recently added to the BanditNodeVisitor function in version 1.7.3, causing a TypeError as follows

multiprocessing.pool.RemoteTraceback: 
"""
Traceback (most recent call last):
  File "/mnt/home/liurenmi/software/anaconda3/envs/geneplexus/lib/python3.8/multiprocessing/pool.py", line 125, in worker
    result = (True, func(*args, **kwds))
  File "/mnt/ufs18/home-026/liurenmi/repo/GeneplexusPublic/.tox/flake8/lib/python3.8/site-packages/flake8/checker.py", line 687, in _run_checks
    return checker.run_checks()
  File "/mnt/ufs18/home-026/liurenmi/repo/GeneplexusPublic/.tox/flake8/lib/python3.8/site-packages/flake8/checker.py", line 597, in run_checks
    self.run_ast_checks()
  File "/mnt/ufs18/home-026/liurenmi/repo/GeneplexusPublic/.tox/flake8/lib/python3.8/site-packages/flake8/checker.py", line 500, in run_ast_checks
    for (line_number, offset, text, _) in runner:
  File "/mnt/ufs18/home-026/liurenmi/repo/GeneplexusPublic/.tox/flake8/lib/python3.8/site-packages/flake8_bandit.py", line 85, in run
    for warn in self._check_source():
  File "/mnt/ufs18/home-026/liurenmi/repo/GeneplexusPublic/.tox/flake8/lib/python3.8/site-packages/flake8_bandit.py", line 59, in _check_source
    bnv = BanditNodeVisitor(
TypeError: __init__() missing 1 required positional argument: 'metrics'
"""

Would it be possible to make a patch for this?

kjbergman commented 2 years ago

I’m also experiencing this issue

mschoettle commented 2 years ago

It would probably be good to pin the exact bandit version in the requirements of this plugin to avoid a similar situation in the future. Not sure how exactly right now but happy to provide a PR if @tylerwince agrees.

Dreamsorcerer commented 2 years ago

This looks like a very low-activity project, so I'd suspect that will only work if someone sets up dependabot to automatically update the dependency and then automatically deploy a new release at the same time, if the tests pass successfully.

dolfinus commented 2 years ago

It would probably be good to pin the exact bandit version

This is not a good idea for a library. In such a case user will not be able to install another library which requires some other bandit version, as well as just upgrade bandit because some issue was fixed or a new feature was introduced.

If flake8-bandit requires some changes were added only to bandit==1.7.3, requirements.txt should look like bandit>=1.7.3. This allows user to install any other new version of bandit if this is required.

There is a way to protect from issues with future bandit releases - set up upper limit for bandit version, like bandit>=1.7.3,<2.0 or even bandit>=1.7.3,<1.8. But this requires flake8-bandit to release more often.

Also there is no guarantee that 1.7.4 release will not break backward compatibility. Actually, instead of 1.7.3 version https://github.com/PyCQA/bandit/pull/496 commit should be released as 1.8.0 because it caused backward compatibility break in the first place.

pawamoy commented 2 years ago

There is a way to protect from issues with future bandit releases - set up upper limit for bandit version, like bandit>=1.7.3,<2.0 or even bandit>=1.7.3,<1.8.

Disagree as well, for the reasons you mentioned:

But this requires flake8-bandit to release more often. Also there is no guarantee that 1.7.4 release will not break backward compatibility.

Compatibility can be broken at any time indeed. Upper bounds do not protect your library. And they prevent downstream users to get upgrades. Without upper bounds, sure, things can break more often, but users can exclude the problematic version themselves. Then upstream can either fix the compatibility issue or exclude the version as well.

Upper bounds can still be used of course, but only if you know the excluded range broke or is going to break compatiblity.

dhuckins commented 2 years ago

is @tylerwince still watching this repo? doesn't seem like its been updated in a few years does anyone else have access to merge a change and deploy?

tylerwince commented 2 years ago

Hey all! I'm happy to update and add a dependabot and accept PRs on this.

Let me take a look at the PR that was opened this morning and I'll try to work on it later today.

dolfinus commented 2 years ago

Dependabot updates only dependencies with fixed version, like ==1.7.3. It is completely useless for this package

Dreamsorcerer commented 2 years ago

It can do more than that, but we just saw breakage in a patch release, so it would need to pin to exact versions to avoid this situation anyway. Unless you pinned it to a <=1.7.3 until it is tested on newer versions (which dependabot can still update).

JayWelborn commented 2 years ago

I found this while investigating why pre-commit broke in CI for all our repos. We don't use flake8-bandit directly, but it's used by one of our hooks. For anyone using the flake8 pre-commit hook who finds this and needs a workaround, my team temporarily added a

bandit < 1.7.3 in additional_dependencies in pre-commit config like:

    additional_dependencies:
          # ... other plugins
          - bandit <1.7.3

to unblock our CI until there's an update.

Natureshadow commented 2 years ago

Has anyone asked the bandit people why they break the API in a patch release? That's the real issue here.

alimony commented 2 years ago

Has anyone asked the bandit people why they break the API in a patch release? That's the real issue here.

It's not a public API: https://github.com/PyCQA/bandit/issues/837#issuecomment-1054746340

Natureshadow commented 2 years ago

It's not a public API: PyCQA/bandit#837 (comment)[https://github.com/PyCQA/bandit/issues/837#issuecomment-1054746340]

Maybe the "Code Quality Authority" should learn about semantic versioning and naming conventions in Python, then.

pawamoy commented 2 years ago

We all make mistakes. And even when we don't, we can't know in advance that a specific change will break someone's use case, or not. About naming conventions: sure, they communicate that something is public or private. But to be fair, Bandit's README and documentation pages never speak about programmatic use, so we could guess that none of its API is public.

Natureshadow commented 2 years ago

We all do mistakes.

That's why we all don't give ourselves pretentious names like "Code Quality Authority", as in assuming to have the last word in code quality ;).

Either way, I hope this will somehow be sorted out soon…

cjolowicz commented 2 years ago

@Natureshadow I understand that you find it frustrating if a change in an open-source tool breaks your workflow. But remember that these tools are maintained by volunteers in their free time, for you and everybody else. They deserve our kindness and respect.

Here's some great reading on the topic:

As for the name PyCQA, see https://meta.pycqa.org/introduction.html

tylerwince commented 2 years ago

Sorry all! Been crazy the last week at work but this should be resolved! Let me know if you see anything that isn't working right! Cheers and thanks for being patient.

pawamoy commented 2 years ago

Thank you for your time @tylerwince :slightly_smiling_face:

radomirbosak commented 2 years ago

Thank you very much for fixing this! :)

Would it be also possible do a new flake8-bandit release to pypi?

RemyLau commented 2 years ago

Thanks a lot!

Zethson commented 2 years ago

@tylerwince thank you! I think that for this to propagate properly we need a new release on PyPI. Would appreciate it!