search

rapid7 / metasploit-framework

Metasploit Framework
https://www.metasploit.com/
Other
34k stars 13.95k forks source link

Create rubocop rule for preferring File.binread over IO.read #16441

Open bcoles opened 2 years ago

bcoles commented 2 years ago

As per:

May also need to check for File.read as per:

adfoster-r7 commented 2 years ago

For context, we didn't originally add automation for this to the original PR as there are scenarios where IO.read and File.read work just fine, i.e. for wordlists etc, but not for executable binary files.

There are other scenarios where the user's intentions are ambiguous, i.e. if a user uses an ftp upload module - should the bytes be copied verbatim, or should new lines be normalized between the client/target hosts. Other tools solve this by letting the user specify ascii/binary modes, i.e. in the case of bsd's ftp client, or just always uploading in binary mode, i.e. samba's smbclient, but it feels like we'd have to introduce a convention to our modules for that.

It'd be cool to automate this with Rubocop, but I'm on the fence about it solving the underlying issue with above ambiguities. I think the nuances of specifying mode rb or r, or using binread vs readlines vs readlines(chomp: true) may be lost to drive-by contributors, who will just copy/paste from existing modules until rubocop is happy. Although a rule would at least bring an awareness to the issue either way, which is definitely better.

adfoster-r7 commented 2 years ago

The above aside; msftidy does some checks for specifying binary mode with File.open over here:

https://github.com/rapid7/metasploit-framework/blob/45dcfda49aec712ae20ed772a4cfe07fca706e4d/tools/dev/msftidy.rb#L680-L685

A dedicated rubocop rule will definitely be smarter than this regex :+1:

github-actions[bot] commented 2 years ago

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.