Skip to content

Add RuboCop lint rules for module best practices - #21772

Open
dwelch-r7 wants to merge 2 commits into
rapid7:masterfrom
dwelch-r7:updates-rubocop-linters
Open

Add RuboCop lint rules for module best practices#21772
dwelch-r7 wants to merge 2 commits into
rapid7:masterfrom
dwelch-r7:updates-rubocop-linters

Conversation

@dwelch-r7

Copy link
Copy Markdown
Contributor

I had kiro look at our modules/PR comments etc and had it come up with some new linting rules to prevent us from needing to leave similar comments in the future this is what it came up with

How these rules were identified
These three cops were selected from a quantitative analysis of 5,058 modules in the repository, cross-referenced against patterns actively enforced in PR reviews by maintainers:

ModuleMissingAutocheck — 847 exploit/auxiliary modules define a check method but don't prepend AutoCheck. Every recent PR review where this is missing gets a "please add AutoCheck" comment. The pattern is already documented in CONTRIBUTING.md under "Modernizing Existing Modules" but was only enforced by human memory.

ModuleDefaultPayload — Hardcoded 'PAYLOAD' => '...' in DefaultOptions appears in ~60 modules. Reviewers consistently request removal because it limits the framework's automatic payload selection. The escape hatch (# rubocop:disable with a comment) exists because a small number of modules genuinely need it (auto-selection picks an incompatible payload), and the comment makes these searchable for future framework improvements.

ModuleHttpFingerprint — HttpFingerprint = { ... } appears in ~180 legacy modules. No new module has used it since approximately 2014. It was never formally deprecated in code (no warn call or removal timeline), but reviewers reject it on sight in favour of a check method + AutoCheck. This cop codifies that de facto standard.

The selection criteria were: (a) the pattern is already documented as a convention, (b) reviewers actively comment on it in PRs today, and (c) it can be detected reliably via AST without false positives. Patterns that are subjective, context-dependent, or already covered by existing cops (like Lint/ModuleEnforceNotes) were excluded.

Description

Adds three new custom RuboCop cops that enforce module best practices documented in CONTRIBUTING.md:

  1. Lint/ModuleMissingAutocheck — flags exploit/auxiliary modules that define a check method but don't prepend Msf::Exploit::Remote::AutoCheck. Severity: info.
  2. Lint/ModuleDefaultPayload — flags modules that hardcode 'PAYLOAD' => '...' inside DefaultOptions. Severity: warning. Contributors can suppress with an inline # rubocop:disable Lint/ModuleDefaultPayload and a comment explaining why — making the exception searchable for future cleanup.
  3. Lint/ModuleHttpFingerprint — flags usage of the legacy HttpFingerprint constant assignment. Severity: info.

All three cops are registered in .rubocop.yml and scoped to modules/ paths. They fire on newly-added modules via msftidy's existing CI pattern (which only lints files added after the RuboCop epoch commit 3a046f01). New modules must pass these checks to merge — the severity levels affect display classification only, not whether CI fails.

These cops codify conventions already documented in the "Modernizing Existing Modules" section of CONTRIBUTING.md, making them machine-enforceable for new module submissions rather than relying on reviewer memory.

Related Issue: None

Breaking Changes

None. Only newly-added module files are linted by msftidy in CI. Existing modules are unaffected unless a contributor explicitly runs rubocop on them.

Reviewer Notes

  • The cops reference "Modernizing Existing Modules in CONTRIBUTING.md" in their messages — this section already exists and documents AutoCheck, HttpFingerprint, and DefaultOptions patterns.
  • ModuleDefaultPayload intentionally supports # rubocop:disable suppression with a justification comment for the rare case where auto-selection genuinely picks an incompatible payload. This makes exceptions explicit and searchable rather than silently accepted.
  • All three cops use standard RuboCop AST node visitors (on_def, on_pair, on_casgn) with no external dependencies.
  • Existing modules are never flagged because msftidy only runs RuboCop on files added after the epoch commit — modified existing modules skip the RuboCop pass entirely.

Verification Steps

    • cd /path/to/metasploit-framework
    • Run specs: bundle exec rspec spec/rubocop/cop/lint/module_missing_autocheck_spec.rb spec/rubocop/cop/lint/module_default_payload_spec.rb spec/rubocop/cop/lint/module_http_fingerprint_spec.rb — expect 19 examples, 0 failures
    • Verify cops fire on a real module: bundle exec rubocop --only Lint/ModuleMissingAutocheck modules/exploits/linux/http/cacti_pollers_sqli_rce.rb (should report offense if module has check but no AutoCheck prepend)
    • Verify cops don't false-positive: bundle exec rubocop --only Lint/ModuleDefaultPayload modules/exploits/linux/http/metabase_setup_token_rce.rb (should pass clean — no DefaultOptions PAYLOAD)
    • Confirm msftidy integration: bundle exec ruby tools/dev/msftidy.rb modules/exploits/linux/http/cacti_pollers_sqli_rce.rb — rubocop section fires and reports offenses (non-zero exit)

AI Usage Disclosure

Kiro

Pre-Submission Checklist

…sage, prevent hardcoded payloads, and deprecate HttpFingerprint

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds three custom RuboCop cops for Metasploit module best practices.

Changes:

  • Detects missing AutoCheck, default payloads, and legacy HTTP fingerprints.
  • Registers the cops for module paths.
  • Adds focused RSpec coverage.

Impact Analysis:

  • Blast radius: Module linting and CI for new and post-epoch modules; medium.
  • Data and contract effects: No runtime schema or API changes.
  • Rollback and test focus: Validate CI severity, AST scoping, and false-positive cases.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
.rubocop.yml Registers and configures the cops.
lib/rubocop/cop/lint/module_missing_autocheck.rb Detects missing AutoCheck prepends.
lib/rubocop/cop/lint/module_default_payload.rb Detects hardcoded default payloads.
lib/rubocop/cop/lint/module_http_fingerprint.rb Detects legacy HTTP fingerprints.
spec/rubocop/cop/lint/module_missing_autocheck_spec.rb Tests AutoCheck detection.
spec/rubocop/cop/lint/module_default_payload_spec.rb Tests default-payload detection.
spec/rubocop/cop/lint/module_http_fingerprint_spec.rb Tests fingerprint detection.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread .rubocop.yml
Modules with a check method should prepend Msf::Exploit::Remote::AutoCheck
so users can verify vulnerability before exploitation.
Enabled: true
Severity: info

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

False positive. msftidy invokes RuboCop without --fail-level (L63-L67), so any offense — including info — returns exit code 1. That non-zero is promoted to ERROR (L941). All three cops block CI as intended; severity only affects the output letter prefix (I vs W).

Comment on lines +45 to +48
class_node = node.each_ancestor(:class).first
return unless class_node

return if has_autocheck_prepend?(class_node)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will add

Comment on lines +56 to +63
# Parent must be a hash
parent_hash = node.parent
return false unless parent_hash&.hash_type?

# Grandparent must be a pair with key 'DefaultOptions'
grandparent_pair = parent_hash.parent
return false unless grandparent_pair&.pair_type?
return false unless grandparent_pair.key.str_type? && grandparent_pair.key.value == 'DefaultOptions'

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Anchoring to update_info would introduce false negatives — 8 modules use super() directly with this pattern (e.g. vmware_vcenter_log4shell.rb#L56-L57). The structural match (L52-L65) — string 'PAYLOAD' inside string 'DefaultOptions' — is already unique to module metadata across 5000+ modules with zero non-metadata occurrences.

Comment thread .rubocop.yml
Comment on lines +718 to +722
Enabled: true
Severity: info
Include:
- 'modules/exploits/**/*'
- 'modules/auxiliary/**/*'

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Incorrect — msftidy only lints files with git status A (Added): next unless summary[:status] == 'A'. Modified files (M) skip RuboCop entirely (L56-L60 — returns STATUS_SUCCESS immediately). Editing any existing module never invokes these cops.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Todo

Development

Successfully merging this pull request may close these issues.

2 participants