Rubocop rollout

[adfoster-r7]

Problem

A lot of time is currently spent during code review pointing out obvious code quality mistakes with newly contributed Metasploit Modules. These problems can easily be automated with Rubocop.

Goal

Allow Metasploit maintainers to spend their time on more important matters.

Reduce the time spent reviewing easily fixed mistakes on pull requests.

Solution

Rely on Rubocop to enforce the consistency of Ruby code. Metasploit maintainers can simply request that Rubocop be ran on new modules, instead of commenting on each individual code quality rule that has been violated.

Considerations

Although it's possible to run Rubocop wholesale on all of Metasploit, it comes with some downsides:

• We would like to enforce whitespace consistency. However - in some contexts breaking the whitespace rules is beneficial - for instance around bindata classes
• Adding linting commits can impact Git Blame functionality, and thus waste the time of Metasploit maintainers:
  • Applying a single big bang ‘fix all the things’ PR would have less impact on the codebase, but is a higher risk of introducing issues.
  • Adding individual commits to fix things gradually will be painful, both for reviewers/landers
  • Example of prior pain, retabbing Metasploit
• If some of the code hasn’t been touched in 15+ years, why bother spending cycles on it?
• Some of Rubocop's ‘safe’ fixes aren’t always safe - or generate invalid code
• Rubocop continually pushes new rules which may impact a large number of files. These changes may not be necessary to have a high quality codebase, and would just add additional unneeded overhead
• Bike shedding new rules takes is always fun - but also takes up maintainer's time. It's best to go with the path of least resistance, to avoid losing any time that we would have saved during module reviews Add rubocop method call with args parenthesis rule #13398

Some of the above downsides with Rubocop directly conflict with the goal of allowing Metasploit maintainers and community members to spend their time on more important matters.

Outcome

WIth the above considerations, a good first step for the Rubocop initiative is to focus on solving the immediate problem of reducing the time spent on reviewing new modules. This means that core library files and old modules would be ignored for now.

Project mile stones

• Add custom Rubocop rules for formatting modules consistently: Add rubocop rules to consistently format modules #12990
• Ensure that new modules lint as expected, and we are happy with their consistency
• Add a custom Rubocop rule to enforce timestamp consistency in new modules: Add disclosure date rubocop linting rule - enforce iso8601 disclosure dates #14213
• Introduce Rubocop as part of the CI process. Specifically new modules for now, or potentially new files as a whole. This may need to be integrated as part of msftidy, or Rubocop should trigger msftidy, or msftidy will need to be rewritten.
• Re-evaluate if we wish to run Rubocop on the existing files, and create the new project mile stones as a result of that meeting

[bcoles]

Some of Rubocop's ‘safe’ fixes aren’t always safe - or generate invalid code

These can/should be overridden in the Rubocop config file or using custom rules. Discovering which rules are problematic is the hard part. Generally the Rubocop team try to avoid rules which will generate bugs.

Rubocop continually pushes new rules which may impact a large number of files. These changes may not be necessary to have a high quality codebase, and would just add additional unneeded overhead

Locking the version of Rubocop in the Gemfile.lock file will resolve this. Let the wider community act as guinea pigs and bicker among themselves.

This means that core library files and old modules would be ignored for now.

I agree in the short term. In the long term, ensuring all code is consistent will make future reviews easier.

Once the majority of the code base is brought up to date with consistent style then it will be much easier to manage and review new style changes as the changes will be much smaller.

If some of the code hasn’t been touched in 15+ years, why bother spending cycles on it?

As above.

Applying a single big bang ‘fix all the things’ PR would have less impact on the codebase, but is a higher risk of introducing issues.

Performing rubocop -a on a per-directory basis can make manual review more manageable. If bugs are encountered, the rules can be updated to prevent the same issues for the next directory.

[bcoles]

If some of the code hasn’t been touched in 15+ years, why bother spending cycles on it?

Because the code is probably buggy and is impossible to maintain (and may contain security vulnerabilities) and unnecessarily increases the effort required to update/modify to the point where no one bothers adding new features because the existing code is impossible to read/understand so we end up writing terrible new code that leverages the existing terrible code which only makes the problem worse.

[bcoles]

We would like to enforce whitespace consistency. However - in some contexts breaking the whitespace rules is beneficial - for instance around bindata classes

One additional issue not discussed above relates to indentation. A lot of the library code uses this format for modules:

module Msf
class Post
module Linux
module BusyBox

Rather than:

module Msf
  class Post
    module Linux
      module BusyBox

Thus running rubocop -a will result in re-indentation of the entire file, making changes harder to read. We should decide what we want to do here.

Edit: A quick look through available Rubocop rules shows no rules which allow keeping the existing behavior. Presuming that we want to use indentation, perhaps it would make sense to update the indentation for all libraries (by automatically applying only the indentation rule) before applying additional changes.

[adfoster-r7]

Github supporting ignore-revs-file would be great, we could benefit having a list of ignored commits for Rubocop's whitespace/layout rules and a bunch of known safe rules too. Not sure when/if that would be supported:

https://github.community/t/support-ignore-revs-file-in-githubs-blame-view/3256/13