diff --git a/CHANGELOG.md b/CHANGELOG.md index c5b52d82be5..d1d7b20e30b 100644 --- a/CHANGELOG.md +++ b/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) diff --git a/config/default.yml b/config/default.yml index 05104802b7c..674ffd88c67 100644 --- a/config/default.yml +++ b/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: diff --git a/docs/modules/ROOT/pages/cops_security.adoc b/docs/modules/ROOT/pages/cops_security.adoc index 8f6c3bd51a3..b06a3358829 100644 --- a/docs/modules/ROOT/pages/cops_security.adoc +++ b/docs/modules/ROOT/pages/cops_security.adoc @@ -117,16 +117,16 @@ 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 @@ -134,6 +134,7 @@ a serious security risk by using variable input to the argument of ---- # bad open(something) +URI.open(something) # good File.open(something) diff --git a/lib/rubocop/cop/security/open.rb b/lib/rubocop/cop/security/open.rb index bfb153b09c4..9e6637129c1 100644 --- a/lib/rubocop/cop/security/open.rb +++ b/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 `%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 diff --git a/spec/rubocop/cop/security/open_spec.rb b/spec/rubocop/cop/security/open_spec.rb index 67a3ed0f65e..6d270dffe70 100644 --- a/spec/rubocop/cop/security/open_spec.rb +++ b/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