Add missing Notes for modules (Lint/ModuleEnforceNotes)

[bcoles]

Many modules are missing Notes.

$ rubocop --only Lint/ModuleEnforceNotes modules/exploits/
[...]
2306 files inspected, 1937 offenses detected

This causes rubocop to fail, which causes these modules to be non-compliant with msftidy, whenever these modules are updated.

As a stopgap, using empty arrays makes rubocop happy.

      'Notes' => {
        'Reliability' => [],
        'Stability' => [],
        'SideEffects' => []
      }

There are over 1,900 modules missing Notes. Good luck.

[gwillcox-r7]

This was a fairly recent addition to our requirements. A concern here is that we would need to manually go back and review those 1900 modules individually and then set up all the associated targets to gather this information accurately. This is part of the reason we haven't typically backported a lot of these new requirements, such as requiring msftidy requirements on modules that haven't been updated since 2010.

[gwillcox-r7]

Also my understanding is that we already have this check in place for newer modules to ensure that they have Note fields? Or was your intention to backport this to older modules as well?

[bcoles]

A concern here is that we would need to manually go back and review those 1900 modules individually and then set up all the associated targets to gather this information accurately.

Empty arrays can be used as a stopgap.

This is part of the reason we haven't typically backported a lot of these new requirements, such as requiring msftidy requirements on modules that haven't been updated since 2010.

These requirements are backported to all modules. msftidy is required for all modules. Developers cannot make a change to an old module without also complying with msftidy as non-compliance causes the tests to fail.

When a developer updates a module, they should not be forced to perform unrelated busy work to make an unrelated tangential problem go away.

Since msftidy was introduced, all modules have been compliant. This has been the case for about a decade, until the requirement to add notes was introduced almost 2 years ago. Now, the vast majority of all modules are not compliant with msftidy due to missing notes. All modules currently in Framework should be compliant with msftidy.

Also my understanding is that we already have this check in place for newer modules to ensure that they have Note fields?

Yes, this check is enforced by msftidy. All new modules must be compliant with msftidy else the tests fail.

Or was your intention to backport this to older modules as well?

This check is already applicable to old modules as per above.

[adfoster-r7]

I'll have a look into this :+1:

---

Edit: For CI we only require msftidy and Rubocop to pass for any modules added after this epoch:

https://github.com/rapid7/metasploit-framework/blob/6d62362b821bc392648ce01cc690e45bb9c7800f/tools/dev/msftidy.rb#L95

Or new files that are being added to the PR:

https://github.com/rapid7/metasploit-framework/blob/6d62362b821bc392648ce01cc690e45bb9c7800f/tools/dev/msftidy.rb#L96

Which means there's 108 files that, if modified, would fail CI based on missing notes:

$ git diff -b --name-status -l0 --summary  3a046f01dae340c124dd3895e670983aef5fe0c5..HEAD | grep '\tmodules' | grep 'A\t' | awk '{ print $2 }' | xargs rubocop --only Lint/ModuleEnforceNotes

...

535 files inspected, 108 offenses detected

However - this epoch logic isn't baked into the Rubocop rule if it's run locally.

Will keep looking at a middleground solution

[bcoles]

I'm also not a huge fan of using the empty notes as a stopgap;

Me neither, but we already use empty arrays for SideEffect notes for more than 100 modules:

# grep -rn SideEffect modules/ | grep '\[\]' | wc -l
116
# grep -rn SideEffect modules/exploits/ | grep '\[\]' | wc -l
30

it makes it look like there are no side effects rather than the side effects are unknown.

This seems like a design flaw.

What of the existing 1,900+ modules which have no SideEffects listed?

Missing SideEffects notes should fail-unknown, not fail-safe. For comparison, with Stability notes we use a CRASH_SAFE value. Absence of a Stability value implies unknown stability.

We are also bad at validating these values. Often the values are blank or wrong. (Here's 11 examples: #17091 #16722 #17597)

Adding validation for these values to Rubocop/msftidy would be an improvement but doesn't resolve the underlying issue: enforcing busy work encourages inaccuracy.

[github-actions[bot]]

Hi!

This issue has been left open with no activity for a while now.

We get a lot of issues, so we currently close issues after 60 days of inactivity. It’s been at least 30 days since the last update here. If we missed this issue or if you want to keep it open, please reply here. You can also add the label "not stale" to keep this issue open!

As a friendly reminder: the best way to see this issue, or any other, fixed is to open a Pull Request.

[tactipus]

Hi! While I understand there are higher-level design issues here, it seems like we need someone to go thru all these modules to insert the empty arrays. may I volunteer?