search

astral-sh / ruff

An extremely fast Python linter and code formatter, written in Rust.
https://docs.astral.sh/ruff
MIT License
32.2k stars 1.08k forks source link

Request: adding dlint #4479

Open AbdealiLoKo opened 1 year ago

AbdealiLoKo commented 1 year ago

I use dlint in my project. Supporting the rules from dlint - https://github.com/dlint-py/dlint would be great

qdegraaf commented 1 year ago

Started on this and listed the rules to port. Will give it a go as I find time. Most of the rules seem easy enough, some are no longer relevant in Python 3 and can't be ported. Will see how far I get

qdegraaf commented 1 year ago

The following rules are covered in https://github.com/charliermarsh/ruff/pull/4482 which is ready for final review:

Left TODO are:

I will continue in small batches right after the first PR is merged, but these TODOs can then easily be split among multiple interested contributors.

charliermarsh commented 1 year ago

Hmm, it looks like many of these are implemented in Bandit (and in Ruff's Bandit reimplementation). I think it's worth auditing the list to understand which of these should be considered aliases of existing rules before implementing any further diagnostics.

qdegraaf commented 1 year ago

Good call, will cease further work until then. @AbdealiLoKo do you maybe have a list of rules that you use in DLint that aren't covered by other plugins already in Ruff?

AbdealiLoKo commented 1 year ago

I haven't compared them myself. We do use both bandit and dlint in our application and we seem to just run both (even if there are similar rules)

bastimeyer commented 1 year ago
  • DUO138: BadReCatastrophicUse

https://github.com/dlint-py/dlint/blob/master/docs/linters/DUO138.md

This one is particularly interesting. Does ruff implement similar rules for checking regular expressions which are vulnerable to ReDoS attacks? I couldn't find anything, unless I'm blind. Having such rules is pretty important considering that it's not just about code quality but actually about critical bugs.

DUO138 implementation:

qdegraaf commented 1 year ago

Yeah I can't find anything like that yet either. I'll see if I can find some time to make that the first DLint rule and can audit the rest of the list after.

qdegraaf commented 1 year ago
  • DUO138: BadReCatastrophicUse

https://github.com/dlint-py/dlint/blob/master/docs/linters/DUO138.md

This one is particularly interesting. Does ruff implement similar rules for checking regular expressions which are vulnerable to ReDoS attacks? I couldn't find anything, unless I'm blind. Having such rules is pretty important considering that it's not just about code quality but actually about critical bugs.

DUO138 implementation:

* https://github.com/dlint-py/dlint/blob/307b301cd9e280dcd7a7f9d5edfda3d58e4855f5/dlint/linters/bad_re_catastrophic_use.py#L69

* https://github.com/dlint-py/dlint/tree/307b301cd9e280dcd7a7f9d5edfda3d58e4855f5/dlint/redos

I've opened a draft PR which adds the regex_syntax crate and all the boilerplate. Actually adding the logic/check will be a bit more difficult. dlint relies on sre_parser and builds an OP tree around that. In Rust, if we don't want to build a regex parser from the ground up, we have to rely on regex_syntax as far as I can tell which uses a different representation for parsed regex strings. In order to cover the same ground as dlint I will have to more thoroughly understand when catastrophic backtracking can happen and then build similar checks using the representation provided by the crate. Unsure how far I'll get, PR is open so any feedback/input/help is welcomed.

EDIT: Closed the PR for now. I think we need an overall plan for how to handle Python regex in Ruff before I can make sensible choices on what to do with the particulars of this specific rule.

khanfarhan10 commented 6 months ago

The ReDoS attack is a notorious bug and I've faced this attack myself rather unexplicably.

Can we not add this plugin as an additional and optional dependency to ruff and call dlint via a subprocess equivalent to Ruff.

I understand that this may slow down ruff altogether again, but it shall be a user opted dependency and won't be installed for everyone.

I wonder if we could implement something like this for now and move ahead with migrating the python code to RUST gradually in phases?

khanfarhan10 commented 6 months ago

@charliermarsh @qdegraaf please convey your thoughts if we are going to add this to Ruff.

P. S. - would contribute myself, but my rust knowledgebase is NIL.

zanieb commented 6 months ago

It's not feasible for us to add Python tools as dependencies and call into them — we are very unlikely to do so. Not only is it slow, but each has an entirely different diagnostic system than us. I don't think doing so would meet our bar for user experience.