Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add Security/Open rubocop rule #3855

Merged
merged 3 commits into from
Nov 16, 2021
Merged

Add Security/Open rubocop rule #3855

merged 3 commits into from
Nov 16, 2021

Conversation

javierm
Copy link
Member

@javierm javierm commented Nov 13, 2019 •
edited
Loading

References

Objectives

  • Make sure we get a warning if we use Kernel#open in the future, since it might be dangerous if done with user input
  • Make sure we parse cached attachment URLs when using remote storages with Paperclip

Notes

We had a false positive for a "dangerous" usage which wasn't actually dangerous because it used a value extracted from the secrets.yml file; that is, the argument was under our control.

@javierm javierm added the Linters Rubocop, ERB Lint, ESLint, SCSS-Lint, ... label Nov 13, 2019
@javierm javierm self-assigned this Nov 13, 2019
@javierm javierm removed the post 1.1 label Mar 11, 2020
github-actions[bot]
github-actions bot reviewed Nov 8, 2021
View reviewed changes
lib/sms_api.rb Outdated Show resolved Hide resolved
@javierm javierm added the 1.4 label Nov 10, 2021
@javierm javierm marked this pull request as draft November 10, 2021 20:58
@javierm javierm added the security Pull requests that address a security vulnerability label Nov 10, 2021
@javierm javierm force-pushed the rubocop_open branch 2 times, most recently from b7e0740 to 61c9979 Compare November 11, 2021 17:26
@javierm javierm marked this pull request as ready for review November 11, 2021 17:37
@taitus taitus self-assigned this Nov 16, 2021
@javierm
Extract method to check for a filesystem storage
e5fbd34 
We'll use this method to write a test dealing with remote storages.
@javierm
Parse cached attachment URLs with remote storages
5519518 
In commit 5a4921a we replaced `URI.parse` with `URI.open` due to some
issues during our tests with S3.

However, there are some security issues with `URI.open` [1], since it
might allow some users to execute code on the server.

So we're using `URI.parse#open` instead.

[1] https://docs.rubocop.org/rubocop/cops_security.html#securityopen
@javierm
Add Security/Open rubocop rule
dc87f9d 
The `open` method can be used to open files or URLs and it's deprecated
in Ruby 2.7. In this case, it's clear we're dealing with a URL, so we
can use `URI.parse`.

The code was a bit strange, since it returned a value and had a side
effect: opening the URL. I'm not sure about the intention of the code;
my best guess is we wanted to test the URL exists and was accessible
before returning it (and, if that's the case, IMHO the code should be a
bit more explicit in order to show the intention behind it), but it
could also be an unintended side effect which was there by accident.

Now the URL is no longer opened; if the URL isn't accessible, we'll find
out when trying to connect to it with the Savon client.
@javierm javierm merged commit 6eddf5d into master Nov 16, 2021
@javierm javierm deleted the rubocop_open branch November 16, 2021 11:55
@javierm javierm removed the 1.4 label Nov 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@github-actions GitHub Actions GitHub Actions left review comments

@taitus taitus taitus approved these changes

Assignees

@taitus taitus

@javierm javierm

Labels
Linters Rubocop, ERB Lint, ESLint, SCSS-Lint, ... security Pull requests that address a security vulnerability
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Enable Rubocop's Security/Open cop & fix issue
2 participants
@javierm @taitus