Add RuboCop lint rules for module best practices

.rubocop.yml

@@ -29,6 +29,9 @@ require:
   - ./lib/rubocop/cop/lint/bare_check_code_in_non_exploit.rb
   - ./lib/rubocop/cop/lint/check_code_missing_reason.rb
   - ./lib/rubocop/cop/lint/module_redundant_arch_platform.rb
+  - ./lib/rubocop/cop/lint/module_missing_autocheck.rb
+  - ./lib/rubocop/cop/lint/module_default_payload.rb
+  - ./lib/rubocop/cop/lint/module_http_fingerprint.rb
 
 Lint/CheckCodeMissingReason:
   Enabled: true
@@ -707,3 +710,31 @@ Lint/BareCheckCodeInNonExploit:
     - 'modules/auxiliary/**/*'
     - 'modules/post/**/*'
     - 'modules/evasion/**/*'
+
+Lint/ModuleMissingAutocheck:
+  Description: >-
+    Modules with a check method should prepend Msf::Exploit::Remote::AutoCheck
+    so users can verify vulnerability before exploitation.
+  Enabled: true
+  Severity: info
+  Include:
+    - 'modules/exploits/**/*'
+    - 'modules/auxiliary/**/*'
+
+Lint/ModuleDefaultPayload:
+  Description: >-
+    Do not hardcode a default PAYLOAD in DefaultOptions.
+    Let the framework choose the most appropriate payload automatically.
+  Enabled: true
+  Severity: warning
+  Include:
+    - 'modules/**/*'
+
+Lint/ModuleHttpFingerprint:
+  Description: >-
+    HttpFingerprint is a legacy passive fingerprinting mechanism.
+    Use a check method with AutoCheck instead.
+  Enabled: true
+  Severity: info
+  Include:
+    - 'modules/**/*'

lib/rubocop/cop/lint/module_default_payload.rb

@@ -0,0 +1,70 @@
+# frozen_string_literal: true
+
+module RuboCop
+  module Cop
+    module Lint
+      # Detects modules that hardcode a PAYLOAD in DefaultOptions.
+      #
+      # The framework can automatically select the most appropriate payload based
+      # on the target and available session types. Hardcoding a default payload
+      # limits flexibility and may not work in all environments.
+      #
+      # If a module genuinely requires a specific payload (e.g. the framework's
+      # auto-selection picks an incompatible one), suppress with an inline
+      # `# rubocop:disable Lint/ModuleDefaultPayload` and add a comment explaining
+      # why. This makes the workaround searchable so the underlying auto-selection
+      # issue can be fixed later without blocking the PR.
+      #
+      # @example
+      #   # bad - hardcoded default payload without justification
+      #   'DefaultOptions' => {
+      #     'PAYLOAD' => 'cmd/unix/reverse_bash'
+      #   }
+      #
+      #   # good - let the framework choose
+      #   'DefaultOptions' => {
+      #     'SSL' => true,
+      #     'WfsDelay' => 5
+      #   }
+      #
+      #   # acceptable - justified workaround (searchable for future fix)
+      #   'DefaultOptions' => {
+      #     # Auto-selection picks generic/shell_reverse_tcp which lacks job support
+      #     'PAYLOAD' => 'cmd/unix/reverse_bash' # rubocop:disable Lint/ModuleDefaultPayload
+      #   }
+      #
+      class ModuleDefaultPayload < Base
+        MSG = 'Do not hardcode a default PAYLOAD in DefaultOptions — ' \
+              'let the framework choose automatically. ' \
+              'If a specific payload is genuinely required, add a comment explaining why ' \
+              'and suppress with `# rubocop:disable Lint/ModuleDefaultPayload`. ' \
+              'See Modernizing Existing Modules in CONTRIBUTING.md.'
+
+        def on_pair(node)
+          return unless payload_in_default_options?(node)
+
+          add_offense(node, message: MSG)
+        end
+
+        private
+
+        # Check if this pair node is 'PAYLOAD' => ... inside a 'DefaultOptions' hash
+        def payload_in_default_options?(node)
+          # Node must be a pair with key 'PAYLOAD'
+          return false unless node.key.str_type? && node.key.value == 'PAYLOAD'
+
+          # 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'
+
+          true
+        end
+      end
+    end
+  end
+end

lib/rubocop/cop/lint/module_http_fingerprint.rb

@@ -0,0 +1,38 @@
+# frozen_string_literal: true
+
+module RuboCop
+  module Cop
+    module Lint
+      # Detects usage of the legacy `HttpFingerprint` constant assignment.
+      #
+      # `HttpFingerprint` was a passive fingerprinting mechanism that predates
+      # the modern `check` method API. Modules should implement a `check` method
+      # and use `prepend Msf::Exploit::Remote::AutoCheck` instead.
+      #
+      # @example
+      #   # bad - legacy passive fingerprinting
+      #   HttpFingerprint = { :pattern => [/Apache/] }
+      #
+      #   # good - active check method
+      #   prepend Msf::Exploit::Remote::AutoCheck
+      #
+      #   def check
+      #     # version detection logic
+      #     CheckCode::Appears('Target appears vulnerable')
+      #   end
+      #
+      class ModuleHttpFingerprint < Base
+        MSG = 'HttpFingerprint is a legacy passive fingerprinting mechanism. ' \
+              'Implement a check method and prepend AutoCheck instead. ' \
+              'See Modernizing Existing Modules in CONTRIBUTING.md.'
+
+        def on_casgn(node)
+          _scope, name, _value = *node
+          return unless name == :HttpFingerprint
+
+          add_offense(node, message: MSG)
+        end
+      end
+    end
+  end
+end

lib/rubocop/cop/lint/module_missing_autocheck.rb

@@ -0,0 +1,83 @@
+# frozen_string_literal: true
+
+module RuboCop
+  module Cop
+    module Lint
+      # Detects exploit and auxiliary modules that define a `check` method but do not
+      # `prepend Msf::Exploit::Remote::AutoCheck`.
+      #
+      # AutoCheck wraps the `exploit`/`run` method to automatically call `check` before
+      # exploitation, giving users the ability to verify vulnerability first.
+      #
+      # @example
+      #   # bad - check method without AutoCheck prepend
+      #   class MetasploitModule < Msf::Exploit::Remote
+      #     include Msf::Exploit::Remote::HttpClient
+      #
+      #     def check
+      #       CheckCode::Safe('Not vulnerable')
+      #     end
+      #
+      #     def exploit
+      #     end
+      #   end
+      #
+      #   # good - AutoCheck prepended after includes
+      #   class MetasploitModule < Msf::Exploit::Remote
+      #     include Msf::Exploit::Remote::HttpClient
+      #     prepend Msf::Exploit::Remote::AutoCheck
+      #
+      #     def check
+      #       CheckCode::Safe('Not vulnerable')
+      #     end
+      #
+      #     def exploit
+      #     end
+      #   end
+      #
+      class ModuleMissingAutocheck < Base
+        MSG = 'Module has a check method but does not prepend Msf::Exploit::Remote::AutoCheck. ' \
+              'Add it after your includes — see Modernizing Existing Modules in CONTRIBUTING.md.'
+
+        def on_def(node)
+          return unless node.method_name == :check
+
+          class_node = node.each_ancestor(:class).first
+          return unless class_node
+
+          # Only flag the MetasploitModule class, not nested helper/utility classes
+          return unless metasploit_module_class?(class_node)
+
+          return if has_autocheck_prepend?(class_node)
+
+          add_offense(node, message: MSG)
+        end
+
+        private
+
+        # The framework loader requires the primary module class be named MetasploitModule
+        def metasploit_module_class?(class_node)
+          class_node.identifier.short_name == :MetasploitModule
+        end
+
+        # Search the class body for a `prepend` call whose argument ends in ::AutoCheck
+        def has_autocheck_prepend?(class_node)
+          class_node.each_descendant(:send).any? do |send_node|
+            next unless send_node.method_name == :prepend
+            next if send_node.arguments.empty?
+
+            arg = send_node.first_argument
+            const_ends_with_autocheck?(arg)
+          end
+        end
+
+        # Check if a const node's name chain ends with :AutoCheck
+        def const_ends_with_autocheck?(node)
+          return false unless node&.const_type?
+
+          node.short_name == :AutoCheck
+        end
+      end
+    end
+  end
+end

spec/rubocop/cop/lint/module_default_payload_spec.rb

@@ -0,0 +1,82 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+require 'rubocop/cop/lint/module_default_payload'
+
+RSpec.describe RuboCop::Cop::Lint::ModuleDefaultPayload do
+  subject(:cop) { described_class.new(config) }
+  let(:empty_rubocop_config) { {} }
+  let(:config) { RuboCop::Config.new(empty_rubocop_config) }
+
+  it 'flags DefaultOptions containing PAYLOAD key' do
+    expect_offense(<<~RUBY)
+      class MetasploitModule < Msf::Exploit::Remote
+        def initialize(info = {})
+          super(
+            update_info(
+              info,
+              'Name' => 'Test Module',
+              'DefaultOptions' => {
+                'PAYLOAD' => 'cmd/unix/reverse_bash'
+                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Do not hardcode a default PAYLOAD in DefaultOptions [...]
+              }
+            )
+          )
+        end
+      end
+    RUBY
+  end
+
+  it 'does not flag DefaultOptions without PAYLOAD key' do
+    expect_no_offenses(<<~RUBY)
+      class MetasploitModule < Msf::Exploit::Remote
+        def initialize(info = {})
+          super(
+            update_info(
+              info,
+              'Name' => 'Test Module',
+              'DefaultOptions' => {
+                'SSL' => true,
+                'WfsDelay' => 5
+              }
+            )
+          )
+        end
+      end
+    RUBY
+  end
+
+  it 'does not flag modules without DefaultOptions' do
+    expect_no_offenses(<<~RUBY)
+      class MetasploitModule < Msf::Exploit::Remote
+        def initialize(info = {})
+          super(
+            update_info(
+              info,
+              'Name' => 'Test Module',
+              'Author' => ['Test']
+            )
+          )
+        end
+      end
+    RUBY
+  end
+
+  it 'does not flag PAYLOAD string used outside DefaultOptions context' do
+    expect_no_offenses(<<~RUBY)
+      class MetasploitModule < Msf::Exploit::Remote
+        def initialize(info = {})
+          super(
+            update_info(
+              info,
+              'Name' => 'Test Module',
+              'Notes' => {
+                'PAYLOAD' => 'this is not DefaultOptions'
+              }
+            )
+          )
+        end
+      end
+    RUBY
+  end
+end

spec/rubocop/cop/lint/module_http_fingerprint_spec.rb

@@ -0,0 +1,46 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+require 'rubocop/cop/lint/module_http_fingerprint'
+
+RSpec.describe RuboCop::Cop::Lint::ModuleHttpFingerprint do
+  subject(:cop) { described_class.new(config) }
+  let(:empty_rubocop_config) { {} }
+  let(:config) { RuboCop::Config.new(empty_rubocop_config) }
+
+  it 'flags HttpFingerprint constant assignment' do
+    expect_offense(<<~RUBY)
+      class MetasploitModule < Msf::Exploit::Remote
+        HttpFingerprint = { :pattern => [/Apache/] }
+        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ HttpFingerprint is a legacy passive fingerprinting mechanism. [...]
+      end
+    RUBY
+  end
+
+  it 'does not flag other constant assignments' do
+    expect_no_offenses(<<~RUBY)
+      class MetasploitModule < Msf::Exploit::Remote
+        Rank = ExcellentRanking
+      end
+    RUBY
+  end
+
+  it 'does not flag local variable named http_fingerprint' do
+    expect_no_offenses(<<~RUBY)
+      class MetasploitModule < Msf::Exploit::Remote
+        def check
+          http_fingerprint = {}
+        end
+      end
+    RUBY
+  end
+
+  it 'flags HttpFingerprint with different value types' do
+    expect_offense(<<~RUBY)
+      class MetasploitModule < Msf::Exploit::Remote
+        HttpFingerprint = { :uri => '/index.html', :pattern => [] }
+        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ HttpFingerprint is a legacy passive fingerprinting mechanism. [...]
+      end
+    RUBY
+  end
+end