From a4ea853c52c9e5f54240d9b0d9cbdf68c2348fdd Mon Sep 17 00:00:00 2001 From: Daniel Vandersluis Date: Fri, 12 Nov 2021 13:48:24 -0500 Subject: [PATCH 1/2] Refactor shared gemspec matcher into a mixin. --- lib/rubocop.rb | 1 + lib/rubocop/cop/gemspec/date_assignment.rb | 12 ++------ .../cop/gemspec/duplicated_assignment.rb | 11 +------ .../cop/gemspec/ruby_version_globals_usage.rb | 13 ++------ lib/rubocop/cop/mixin/gemspec_help.rb | 30 +++++++++++++++++++ 5 files changed, 37 insertions(+), 30 deletions(-) create mode 100644 lib/rubocop/cop/mixin/gemspec_help.rb diff --git a/lib/rubocop.rb b/lib/rubocop.rb index 54363c1a8f2..d353cca8478 100644 --- a/lib/rubocop.rb +++ b/lib/rubocop.rb @@ -83,6 +83,7 @@ require_relative 'rubocop/cop/mixin/first_element_line_break' require_relative 'rubocop/cop/mixin/frozen_string_literal' require_relative 'rubocop/cop/mixin/gem_declaration' +require_relative 'rubocop/cop/mixin/gemspec_help' require_relative 'rubocop/cop/mixin/hash_alignment_styles' require_relative 'rubocop/cop/mixin/hash_transform_method' require_relative 'rubocop/cop/mixin/ignored_pattern' diff --git a/lib/rubocop/cop/gemspec/date_assignment.rb b/lib/rubocop/cop/gemspec/date_assignment.rb index 857a99aa3b8..b8a0645bbc6 100644 --- a/lib/rubocop/cop/gemspec/date_assignment.rb +++ b/lib/rubocop/cop/gemspec/date_assignment.rb @@ -21,21 +21,13 @@ module Gemspec # class DateAssignment < Base include RangeHelp + include GemspecHelp extend AutoCorrector MSG = 'Do not use `date =` in gemspec, it is set automatically when the gem is packaged.' - # @!method gem_specification(node) - def_node_matcher :gem_specification, <<~PATTERN - (block - (send - (const - (const {cbase nil?} :Gem) :Specification) :new) - ...) - PATTERN - def on_block(block_node) - return unless gem_specification(block_node) + return unless gem_specification?(block_node) block_parameter = block_node.arguments.first.source diff --git a/lib/rubocop/cop/gemspec/duplicated_assignment.rb b/lib/rubocop/cop/gemspec/duplicated_assignment.rb index b832db4f476..8bd10fe0157 100644 --- a/lib/rubocop/cop/gemspec/duplicated_assignment.rb +++ b/lib/rubocop/cop/gemspec/duplicated_assignment.rb @@ -36,20 +36,11 @@ module Gemspec # end class DuplicatedAssignment < Base include RangeHelp + include GemspecHelp MSG = '`%s` method calls already given on line '\ '%d of the gemspec.' - # @!method gem_specification(node) - def_node_search :gem_specification, <<~PATTERN - (block - (send - (const - (const {cbase nil?} :Gem) :Specification) :new) - (args - (arg $_)) ...) - PATTERN - # @!method assignment_method_declarations(node) def_node_search :assignment_method_declarations, <<~PATTERN (send diff --git a/lib/rubocop/cop/gemspec/ruby_version_globals_usage.rb b/lib/rubocop/cop/gemspec/ruby_version_globals_usage.rb index 0f9a867d51f..73ccb290d90 100644 --- a/lib/rubocop/cop/gemspec/ruby_version_globals_usage.rb +++ b/lib/rubocop/cop/gemspec/ruby_version_globals_usage.rb @@ -26,20 +26,13 @@ module Gemspec # end # class RubyVersionGlobalsUsage < Base + include GemspecHelp + MSG = 'Do not use `RUBY_VERSION` in gemspec file.' # @!method ruby_version?(node) def_node_matcher :ruby_version?, '(const {cbase nil?} :RUBY_VERSION)' - # @!method gem_specification?(node) - def_node_search :gem_specification?, <<~PATTERN - (block - (send - (const - (const {cbase nil?} :Gem) :Specification) :new) - ...) - PATTERN - def on_const(node) return unless gem_spec_with_ruby_version?(node) @@ -49,7 +42,7 @@ def on_const(node) private def gem_spec_with_ruby_version?(node) - gem_specification?(processed_source.ast) && ruby_version?(node) + gem_specification(processed_source.ast) && ruby_version?(node) end end end diff --git a/lib/rubocop/cop/mixin/gemspec_help.rb b/lib/rubocop/cop/mixin/gemspec_help.rb new file mode 100644 index 00000000000..19d58d51ffc --- /dev/null +++ b/lib/rubocop/cop/mixin/gemspec_help.rb @@ -0,0 +1,30 @@ +# frozen_string_literal: true + +module RuboCop + module Cop + # Common functionality for checking gem declarations. + module GemspecHelp + extend NodePattern::Macros + + # @!method gem_specification?(node) + def_node_matcher :gem_specification?, <<~PATTERN + (block + (send + (const + (const {cbase nil?} :Gem) :Specification) :new) + (args + (arg $_)) ...) + PATTERN + + # @!method gem_specification(node) + def_node_search :gem_specification, <<~PATTERN + (block + (send + (const + (const {cbase nil?} :Gem) :Specification) :new) + (args + (arg $_)) ...) + PATTERN + end + end +end From f7b8db678ce5a001f5de4e7f4aa3f4e6a688c31d Mon Sep 17 00:00:00 2001 From: Daniel Vandersluis Date: Fri, 12 Nov 2021 13:49:05 -0500 Subject: [PATCH 2/2] Add new `Gemspec/RequireMFA` cop. --- .../new_add_new_gemspecrequiremfa_cop.md | 1 + config/default.yml | 9 + lib/rubocop.rb | 1 + lib/rubocop/cop/gemspec/require_mfa.rb | 146 +++++++++++++++ spec/rubocop/cop/gemspec/require_mfa_spec.rb | 171 ++++++++++++++++++ 5 files changed, 328 insertions(+) create mode 100644 changelog/new_add_new_gemspecrequiremfa_cop.md create mode 100644 lib/rubocop/cop/gemspec/require_mfa.rb create mode 100644 spec/rubocop/cop/gemspec/require_mfa_spec.rb diff --git a/changelog/new_add_new_gemspecrequiremfa_cop.md b/changelog/new_add_new_gemspecrequiremfa_cop.md new file mode 100644 index 00000000000..d5dbd222d52 --- /dev/null +++ b/changelog/new_add_new_gemspecrequiremfa_cop.md @@ -0,0 +1 @@ +* [#10243](https://github.com/rubocop/rubocop/pull/10243): Add new `Gemspec/RequireMFA` cop. ([@dvandersluis][]) diff --git a/config/default.yml b/config/default.yml index e99b7987ff7..5bf8df8ef4d 100644 --- a/config/default.yml +++ b/config/default.yml @@ -259,6 +259,15 @@ Gemspec/OrderedDependencies: Include: - '**/*.gemspec' +Gemspec/RequireMFA: + Description: 'Checks that the gemspec has metadata to require MFA from RubyGems.' + Enabled: pending + VersionAdded: '<>' + Reference: + - https://guides.rubygems.org/mfa-requirement-opt-in/ + Include: + - '**/*.gemspec' + Gemspec/RequiredRubyVersion: Description: 'Checks that `required_ruby_version` of gemspec is specified and equal to `TargetRubyVersion` of .rubocop.yml.' Enabled: true diff --git a/lib/rubocop.rb b/lib/rubocop.rb index d353cca8478..1cd9d5110a8 100644 --- a/lib/rubocop.rb +++ b/lib/rubocop.rb @@ -161,6 +161,7 @@ require_relative 'rubocop/cop/gemspec/date_assignment' require_relative 'rubocop/cop/gemspec/duplicated_assignment' require_relative 'rubocop/cop/gemspec/ordered_dependencies' +require_relative 'rubocop/cop/gemspec/require_mfa' require_relative 'rubocop/cop/gemspec/required_ruby_version' require_relative 'rubocop/cop/gemspec/ruby_version_globals_usage' diff --git a/lib/rubocop/cop/gemspec/require_mfa.rb b/lib/rubocop/cop/gemspec/require_mfa.rb new file mode 100644 index 00000000000..994135300e1 --- /dev/null +++ b/lib/rubocop/cop/gemspec/require_mfa.rb @@ -0,0 +1,146 @@ +# frozen_string_literal: true + +module RuboCop + module Cop + module Gemspec + # Requires a gemspec to have `rubygems_mfa_required` metadata set. + # + # This setting tells RubyGems that MFA is required for accounts to + # be able perform any of these privileged operations: + # + # * gem push + # * gem yank + # * gem owner --add/remove + # * adding or removing owners using gem ownership page + # + # This helps make your gem more secure, as users can be more + # confident that gem updates were pushed by maintainers. + # + # @example + # + # # bad + # Gem::Specification.new do |spec| + # # no `rubygems_mfa_required` metadata specified + # end + # + # # good + # Gem::Specification.new do |spec| + # spec.metadata = { + # 'rubygems_mfa_required' => 'true' + # } + # end + # + # # good + # Gem::Specification.new do |spec| + # spec.metadata['rubygems_mfa_required'] = 'true' + # end + # + # # bad + # Gem::Specification.new do |spec| + # spec.metadata = { + # 'rubygems_mfa_required' => 'false' + # } + # end + # + # # good + # Gem::Specification.new do |spec| + # spec.metadata = { + # 'rubygems_mfa_required' => 'true' + # } + # end + # + # # bad + # Gem::Specification.new do |spec| + # spec.metadata['rubygems_mfa_required'] = 'false' + # end + # + # # good + # Gem::Specification.new do |spec| + # spec.metadata['rubygems_mfa_required'] = 'true' + # end + # + class RequireMFA < Base + include GemspecHelp + extend AutoCorrector + + MSG = "`metadata['rubygems_mfa_required']` must be set to `'true'`." + + # @!method metadata(node) + def_node_matcher :metadata, <<~PATTERN + `{ + (send _ :metadata= $_) + (send (send _ :metadata) :[]= (str "rubygems_mfa_required") $_) + } + PATTERN + + # @!method rubygems_mfa_required(node) + def_node_search :rubygems_mfa_required, <<~PATTERN + (pair (str "rubygems_mfa_required") $_) + PATTERN + + # @!method true_string?(node) + def_node_matcher :true_string?, <<~PATTERN + (str "true") + PATTERN + + def on_block(node) # rubocop:disable Metrics/MethodLength + gem_specification(node) do |block_var| + metadata_value = metadata(node) + mfa_value = mfa_value(metadata_value) + + if mfa_value + unless true_string?(mfa_value) + add_offense(mfa_value) do |corrector| + change_value(corrector, mfa_value) + end + end + else + add_offense(node) do |corrector| + autocorrect(corrector, node, block_var, metadata_value) + end + end + end + end + + private + + def mfa_value(metadata_value) + return unless metadata_value + return metadata_value if metadata_value.str_type? + + rubygems_mfa_required(metadata_value).first + end + + def autocorrect(corrector, node, block_var, metadata) + if metadata + return unless metadata.hash_type? + + correct_metadata(corrector, metadata) + else + correct_missing_metadata(corrector, node, block_var) + end + end + + def correct_metadata(corrector, metadata) + if metadata.pairs.any? + corrector.insert_after(metadata.pairs.last, ",\n'rubygems_mfa_required' => 'true'") + else + corrector.insert_before(metadata.loc.end, "'rubygems_mfa_required' => 'true'") + end + end + + def correct_missing_metadata(corrector, node, block_var) + corrector.insert_before(node.loc.end, <<~RUBY) + #{block_var}.metadata = { + 'rubygems_mfa_required' => 'true' + } + RUBY + end + + def change_value(corrector, value) + corrector.replace(value, "'true'") + end + end + end + end +end diff --git a/spec/rubocop/cop/gemspec/require_mfa_spec.rb b/spec/rubocop/cop/gemspec/require_mfa_spec.rb new file mode 100644 index 00000000000..05171002a40 --- /dev/null +++ b/spec/rubocop/cop/gemspec/require_mfa_spec.rb @@ -0,0 +1,171 @@ +# frozen_string_literal: true + +RSpec.describe RuboCop::Cop::Gemspec::RequireMFA, :config do + context 'when the gemspec is blank' do + it 'does not register an offense' do + expect_no_offenses('', 'my.gemspec') + end + end + + context 'when the specification is blank' do + it 'registers an offense and corrects' do + expect_offense(<<~RUBY, 'my.gemspec') + Gem::Specification.new do |spec| + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ `metadata['rubygems_mfa_required']` must be set to `'true'`. + end + RUBY + + expect_correction(<<~RUBY) + Gem::Specification.new do |spec| + spec.metadata = { + 'rubygems_mfa_required' => 'true' + } + end + RUBY + end + end + + context 'when the specification has a metadata hash but no rubygems_mfa_required key' do + it 'registers an offense and corrects' do + expect_offense(<<~RUBY, 'my.gemspec') + Gem::Specification.new do |spec| + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ `metadata['rubygems_mfa_required']` must be set to `'true'`. + spec.metadata = { + } + end + RUBY + + expect_correction(<<~RUBY) + Gem::Specification.new do |spec| + spec.metadata = { + 'rubygems_mfa_required' => 'true'} + end + RUBY + end + end + + context 'when the specification has an non-hash metadata' do + it 'registers an offense but does not correct' do + expect_offense(<<~RUBY, 'my.gemspec') + Gem::Specification.new do |spec| + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ `metadata['rubygems_mfa_required']` must be set to `'true'`. + spec.metadata = Metadata.new + end + RUBY + end + end + + context 'when there are other metadata keys' do + context 'and `rubygems_mfa_required` is included' do + it 'does not register an offense' do + expect_no_offenses(<<~RUBY, 'my.gemspec') + Gem::Specification.new do |spec| + spec.metadata = { + 'foo' => 'bar', + 'rubygems_mfa_required' => 'true', + 'baz' => 'quux' + } + end + RUBY + end + end + + context 'and `rubygems_mfa_required` is not included' do + it 'registers an offense and corrects' do + expect_offense(<<~RUBY, 'my.gemspec') + Gem::Specification.new do |spec| + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ `metadata['rubygems_mfa_required']` must be set to `'true'`. + spec.metadata = { + 'foo' => 'bar', + 'baz' => 'quux' + } + end + RUBY + + expect_correction(<<~RUBY) + Gem::Specification.new do |spec| + spec.metadata = { + 'foo' => 'bar', + 'baz' => 'quux', + 'rubygems_mfa_required' => 'true' + } + end + RUBY + end + end + end + + context 'when metadata is set by key assignment' do + context 'and `rubygems_mfa_required` is included' do + it 'does not register an offense' do + expect_no_offenses(<<~RUBY, 'my.gemspec') + Gem::Specification.new do |spec| + spec.metadata['foo'] = 'bar' + spec.metadata['rubygems_mfa_required'] = 'true' + end + RUBY + end + end + + context 'and `rubygems_mfa_required` is not included' do + it 'registers an offense' do + expect_offense(<<~RUBY, 'my.gemspec') + Gem::Specification.new do |spec| + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ `metadata['rubygems_mfa_required']` must be set to `'true'`. + spec.metadata['foo'] = 'bar' + end + RUBY + end + end + end + + context 'with rubygems_mfa_required: true' do + it 'does not register an offense' do + expect_no_offenses(<<~RUBY, 'my.gemspec') + Gem::Specification.new do |spec| + spec.metadata = { + 'rubygems_mfa_required' => 'true' + } + end + RUBY + end + end + + context 'with rubygems_mfa_required: false' do + it 'registers an offense and corrects' do + expect_offense(<<~RUBY, 'my.gemspec') + Gem::Specification.new do |spec| + spec.metadata = { + 'rubygems_mfa_required' => 'false' + ^^^^^^^ `metadata['rubygems_mfa_required']` must be set to `'true'`. + } + end + RUBY + + expect_correction(<<~RUBY) + Gem::Specification.new do |spec| + spec.metadata = { + 'rubygems_mfa_required' => 'true' + } + end + RUBY + end + end + + context 'with rubygems_mfa_required: false by key access' do + it 'registers an offense and corrects' do + expect_offense(<<~RUBY, 'my.gemspec') + Gem::Specification.new do |spec| + spec.metadata['rubygems_mfa_required'] = 'false' + ^^^^^^^ `metadata['rubygems_mfa_required']` must be set to `'true'`. + end + RUBY + + expect_correction(<<~RUBY) + Gem::Specification.new do |spec| + spec.metadata['rubygems_mfa_required'] = 'true' + end + RUBY + end + end +end