Merge pull request #8894 from koic/fix_a_false_negative_for_security_…

…open

Make `Security/Open` aware of `URI.open`

CHANGELOG.md

@@ -16,6 +16,7 @@
 * [#7966](https://github.com/rubocop-hq/rubocop/issues/7966): **(Breaking)** Enable all pending cops for RuboCop 1.0. ([@koic][])
 * [#8490](https://github.com/rubocop-hq/rubocop/pull/8490): **(Breaking)** Change logic for cop department name computation. Cops inside deep namespaces (5 or more levels deep) now belong to departments with names that are calculated by joining module names starting from the third one with slashes as separators. For example, cop `Rubocop::Cop::Foo::Bar::Baz` now belongs to `Foo/Bar` department (previously it was `Bar`). ([@dsavochkin][])
 * [#8692](https://github.com/rubocop-hq/rubocop/pull/8692): Default changed to disallow `Layout/TrailingWhitespace` in heredoc. ([@marcandre][])
+* [#8894](https://github.com/rubocop-hq/rubocop/issues/8894): Make `Security/Open` aware of `URI.open`. ([@koic][])
 
 ## 0.93.1 (2020-10-12)
 

config/default.yml

@@ -2415,9 +2415,10 @@ Security/MarshalLoad:
   VersionAdded: '0.47'
 
 Security/Open:
-  Description: 'The use of Kernel#open represents a serious security risk.'
+  Description: 'The use of `Kernel#open` and `URI.open` represent a serious security risk.'
   Enabled: true
   VersionAdded: '0.53'
+  VersionChanged: '0.94'
   Safe: false
 
 Security/YAMLLoad:

docs/modules/ROOT/pages/cops_security.adoc

@@ -117,23 +117,24 @@ Marshal.load(Marshal.dump({}))
 | No
 | No
 | 0.53
-| -
+| 0.94
 |===
 
-This cop checks for the use of `Kernel#open`.
+This cop checks for the use of `Kernel#open` and `URI.open`.
 
-`Kernel#open` enables not only file access but also process invocation
-by prefixing a pipe symbol (e.g., `open("| ls")`). So, it may lead to
-a serious security risk by using variable input to the argument of
-`Kernel#open`. It would be better to use `File.open`, `IO.popen` or
-`URI#open` explicitly.
+`Kernel#open` and `URI.open` enable not only file access but also process
+invocation by prefixing a pipe symbol (e.g., `open("| ls")`).
+So, it may lead to a serious security risk by using variable input to
+the argument of `Kernel#open` and `URI.open`. It would be better to use
+`File.open`, `IO.popen` or `URI.parse#open` explicitly.
 
 === Examples
 
 [source,ruby]
 ----
 # bad
 open(something)
+URI.open(something)
 
 # good
 File.open(something)

lib/rubocop/cop/security/open.rb

@@ -3,35 +3,37 @@
 module RuboCop
   module Cop
     module Security
-      # This cop checks for the use of `Kernel#open`.
+      # This cop checks for the use of `Kernel#open` and `URI.open`.
       #
-      # `Kernel#open` enables not only file access but also process invocation
-      # by prefixing a pipe symbol (e.g., `open("| ls")`). So, it may lead to
-      # a serious security risk by using variable input to the argument of
-      # `Kernel#open`. It would be better to use `File.open`, `IO.popen` or
-      # `URI#open` explicitly.
+      # `Kernel#open` and `URI.open` enable not only file access but also process
+      # invocation by prefixing a pipe symbol (e.g., `open("| ls")`).
+      # So, it may lead to a serious security risk by using variable input to
+      # the argument of `Kernel#open` and `URI.open`. It would be better to use
+      # `File.open`, `IO.popen` or `URI.parse#open` explicitly.
       #
       # @example
       #   # bad
       #   open(something)
+      #   URI.open(something)
       #
       #   # good
       #   File.open(something)
       #   IO.popen(something)
       #   URI.parse(something).open
       class Open < Base
-        MSG = 'The use of `Kernel#open` is a serious security risk.'
+        MSG = 'The use of `%<receiver>sopen` is a serious security risk.'
         RESTRICT_ON_SEND = %i[open].freeze
 
         def_node_matcher :open?, <<~PATTERN
-          (send nil? :open $!str ...)
+          (send ${nil? (const {nil? cbase} :URI)} :open $!str ...)
         PATTERN
 
         def on_send(node)
-          open?(node) do |code|
+          open?(node) do |receiver, code|
             return if safe?(code)
 
-            add_offense(node.loc.selector)
+            message = format(MSG, receiver: receiver ? "#{receiver.source}." : 'Kernel#')
+            add_offense(node.loc.selector, message: message)
           end
         end
 

spec/rubocop/cop/security/open_spec.rb

@@ -31,6 +31,20 @@
     RUBY
   end
 
+  it 'registers an offense for `URI.open` with string that starts with a pipe' do
+    expect_offense(<<~'RUBY')
+      URI.open("| #{foo}")
+          ^^^^ The use of `URI.open` is a serious security risk.
+    RUBY
+  end
+
+  it 'registers an offense for `::URI.open` with string that starts with a pipe' do
+    expect_offense(<<~'RUBY')
+      ::URI.open("| #{foo}")
+            ^^^^ The use of `::URI.open` is a serious security risk.
+    RUBY
+  end
+
   it 'accepts open as variable' do
     expect_no_offenses('open = something')
   end