Fix a false positive for Security/YamlLoad

This PR fixes a false positive for `Security/YamlLoad` when using Ruby 3.1+ (Psych 4). And it also makes the examples a bit more specific. Ruby 3.1+ (Psych 4) uses `Psych.load` as `Psych.safe_load` by default. ruby/psych#487 We may consider removing this cop in the future, but not yet.
rubocop · Feb 17, 2022 · 0a8ad1b · 0a8ad1b
1 parent 9f82e0f
commit 0a8ad1b
Show file tree

Hide file tree

Showing 4 changed files with 29 additions and 6 deletions.
diff --git a/changelog/fix_a_false_positive_for_security_yaml_load.md b/changelog/fix_a_false_positive_for_security_yaml_load.md
@@ -0,0 +1 @@
+* [#10424](https://github.com/rubocop/rubocop/pull/10424): Fix a false positive for `Security/YamlLoad` when using Ruby 3.1+ (Psych 4). ([@koic][])
diff --git a/lib/rubocop/cop/security/yaml_load.rb b/lib/rubocop/cop/security/yaml_load.rb
@@ -7,17 +7,21 @@ module Security
       # potential security issues leading to remote code execution when
       # loading from an untrusted source.
       #
+      # NOTE: Ruby 3.1+ (Psych 4) uses `Psych.load` as `Psych.safe_load` by default.
+      #
       # @safety
       #   The behaviour of the code might change depending on what was
       #   in the YAML payload, since `YAML.safe_load` is more restrictive.
       #
       # @example
       #   # bad
-      #   YAML.load("--- foo")
+      #   YAML.load("--- !ruby/object:Foo {}") # Psych 3 is unsafe by default
       #
       #   # good
-      #   YAML.safe_load("--- foo")
-      #   YAML.dump("foo")
+      #   YAML.safe_load("--- !ruby/object:Foo {}", [Foo])                    # Ruby 2.5  (Psych 3)
+      #   YAML.safe_load("--- !ruby/object:Foo {}", permitted_classes: [Foo]) # Ruby 3.0- (Psych 3)
+      #   YAML.load("--- !ruby/object:Foo {}", permitted_classes: [Foo])      # Ruby 3.1+ (Psych 4)
+      #   YAML.dump(foo)
       #
       class YAMLLoad < Base
         extend AutoCorrector
@@ -31,6 +35,8 @@ class YAMLLoad < Base
         PATTERN
 
         def on_send(node)
+          return if target_ruby_version >= 3.1
+
           yaml_load(node) do
             add_offense(node.loc.selector) do |corrector|
               corrector.replace(node.loc.selector, 'safe_load')

diff --git a/lib/rubocop/cop/variable_force.rb b/lib/rubocop/cop/variable_force.rb
@@ -190,7 +190,7 @@ def regexp_captured_names(node)
           child.children.first
         end.join || ''
 
-        regexp = Regexp.new(regexp_string)
+        regexp = Regexp.new(regexp_string) # FIXME: Need to be handle `Regexp.new(/\x82/n)`.
 
         regexp.named_captures.keys
       end

diff --git a/spec/rubocop/cop/security/yaml_load_spec.rb b/spec/rubocop/cop/security/yaml_load_spec.rb
@@ -15,12 +15,12 @@
 
   it 'registers an offense and corrects load with a literal string' do
     expect_offense(<<~RUBY)
-      YAML.load("--- foo")
+      YAML.load("--- !ruby/object:Foo {}")
            ^^^^ Prefer using `YAML.safe_load` over `YAML.load`.
     RUBY
 
     expect_correction(<<~RUBY)
-      YAML.safe_load("--- foo")
+      YAML.safe_load("--- !ruby/object:Foo {}")
     RUBY
   end
 
@@ -34,4 +34,20 @@
       ::YAML.safe_load("--- foo")
     RUBY
   end
+
+  # Ruby 3.1+ (Psych 4) uses `Psych.load` as `Psych.safe_load` by default.
+  # https://github.com/ruby/psych/pull/487
+  context 'Ruby >= 3.1', :ruby31 do
+    it 'does not register an offense and corrects load with a literal string' do
+      expect_no_offenses(<<~RUBY)
+        YAML.load("--- !ruby/object:Foo {}", permitted_classes: [Foo])
+      RUBY
+    end
+
+    it 'does not register an offense and corrects a fully qualified `::YAML.load`' do
+      expect_no_offenses(<<~RUBY)
+        ::YAML.load("--- !ruby/object:Foo {}", permitted_classes: [Foo])
+      RUBY
+    end
+  end
 end