diff options
| -rw-r--r-- | rubocop/cop/security/json_serialization.rb (renamed from rubocop/cop/security/to_json.rb) | 66 | ||||
| -rw-r--r-- | rubocop/rubocop.rb | 2 | ||||
| -rw-r--r-- | spec/rubocop/cop/security/json_serialization_spec.rb | 128 | ||||
| -rw-r--r-- | spec/rubocop/cop/security/to_json_spec.rb | 128 |
4 files changed, 178 insertions, 146 deletions
diff --git a/rubocop/cop/security/to_json.rb b/rubocop/cop/security/json_serialization.rb index afd3da91136..50f2d73eee8 100644 --- a/rubocop/cop/security/to_json.rb +++ b/rubocop/cop/security/json_serialization.rb @@ -1,13 +1,37 @@ module RuboCop module Cop module Security - class ToJson < RuboCop::Cop::Cop - MSG = "Don't use `to_json` without specifying `only`".freeze + # This cop checks for `to_json`/`as_json` without `:only` options + # + # Either method called on an instance of a `Serializer` class will be + # ignored. Associations included via `include` are subject to the same + # rules. + # + # @example + # + # # bad + # render json: @user.to_json + # render json: @user.to_json(except: %i[password]) + # render json: @user.to_json( + # only: %i[username], + # include: { :identities } + # ) + # + # # acceptable + # render json: UserSerializer.new.to_json + # + # # good + # render json: @user.to_json(only: %i[name username]) + # render json: @user.to_json(include: { + # identities: { only: %i[provider] } + # }) + class JsonSerialization < RuboCop::Cop::Cop + MSG = "Don't use `%s` without specifying `only`".freeze # Check for `to_json` sent to any object that's not a Hash literal or # Serializer instance def_node_matcher :to_json?, <<~PATTERN - (send !{nil hash #serializer?} :to_json $...) + (send !{nil hash #serializer?} ${:to_json :as_json} $...) PATTERN # Check if node is a `only: ...` pair @@ -25,17 +49,23 @@ module RuboCop (pair (sym :only) (array ...)) PATTERN + # Check for `SomeConstant.new` + def_node_search :constant_init, <<~PATTERN + (send (const nil $_) :new) + PATTERN + def on_send(node) matched = to_json?(node) return unless matched @_has_top_level_only = false + @method = matched.first - if matched[0].nil? + if matched.last.nil? || matched.last.empty? # Empty `to_json` call - add_offense(node, :expression) + add_offense(node, :expression, format_message) else - options = matched.first + options = matched.last.first # If `to_json` was given an argument that isn't a Hash, we don't # know what to do here, so just move along @@ -48,26 +78,22 @@ module RuboCop # Add a top-level offense for the entire argument list, but only if # we haven't yet added any offenses to the child Hash values (such # as `include`) - add_offense(node.children.last, :expression) if requires_only? + if requires_only? + add_offense(node.children.last, :expression, format_message) + end end end private - def_node_search :constant_init, <<~PATTERN - (send (const nil $_) :new) - PATTERN + def format_message + format(MSG, @method) + end def serializer?(node) constant_init(node).any? { |name| name.to_s.end_with?('Serializer') } end - def requires_only? - return false if @_has_top_level_only - - offenses.count.zero? - end - def check_pair(pair) if only_pair?(pair) @_has_top_level_only = true @@ -77,10 +103,16 @@ module RuboCop includes.each_child_node do |child_node| next if contains_only?(child_node) - add_offense(child_node, :expression) + add_offense(child_node, :expression, format_message) end end end + + def requires_only? + return false if @_has_top_level_only + + offenses.count.zero? + end end end end diff --git a/rubocop/rubocop.rb b/rubocop/rubocop.rb index 18a18440bfc..1c26ca9820a 100644 --- a/rubocop/rubocop.rb +++ b/rubocop/rubocop.rb @@ -5,4 +5,4 @@ require_relative 'cop/migration/add_column_with_default' require_relative 'cop/migration/add_concurrent_foreign_key' require_relative 'cop/migration/add_concurrent_index' require_relative 'cop/migration/add_index' -require_relative 'cop/security/to_json' +require_relative 'cop/security/json_serialization' diff --git a/spec/rubocop/cop/security/json_serialization_spec.rb b/spec/rubocop/cop/security/json_serialization_spec.rb new file mode 100644 index 00000000000..77914710a2e --- /dev/null +++ b/spec/rubocop/cop/security/json_serialization_spec.rb @@ -0,0 +1,128 @@ +require 'spec_helper' + +require 'rubocop' +require 'rubocop/rspec/support' + +require_relative '../../../../rubocop/cop/security/json_serialization' + +describe RuboCop::Cop::Security::JsonSerialization do + include CopHelper + + subject(:cop) { described_class.new } + + shared_examples 'an upstanding constable' do |method| + it "ignores calls except `#{method}`" do + inspect_source(cop, 'render json: foo') + + expect(cop.offenses).to be_empty + end + + context "`#{method}` without options" do + it 'does nothing when sent to nil receiver' do + inspect_source(cop, method.to_s) + + expect(cop.offenses).to be_empty + end + + it 'does nothing when sent to a Hash' do + inspect_source(cop, "{}.#{method}") + + expect(cop.offenses).to be_empty + end + + it 'does nothing when sent to a Serializer instance' do + inspect_source(cop, "MergeRequestSerializer.new.represent(issuable).#{method}") + + expect(cop.offenses).to be_empty + end + + it 'adds an offense when sent to any other receiver' do + inspect_source(cop, "foo.#{method}") + + expect(cop.offenses.size).to eq(1) + expect(cop.highlights).to contain_exactly("foo.#{method}") + expect(cop.messages.first).to start_with("Don't use `#{method}`") + end + end + + context "`#{method}` with options" do + it 'does nothing when provided `only`' do + inspect_source(cop, <<~EOS) + render json: @issue.#{method}(only: [:name, :username]) + EOS + + expect(cop.offenses).to be_empty + end + + it 'does nothing when provided `only` and `methods`' do + inspect_source(cop, <<~EOS) + render json: @issue.#{method}( + only: [:name, :username], + methods: [:avatar_url] + ) + EOS + + expect(cop.offenses).to be_empty + end + + it 'adds an offense to `include`d attributes without `only` option' do + inspect_source(cop, <<~EOS) + render json: @issue.#{method}( + include: { + milestone: {}, + assignee: { methods: :avatar_url }, + author: { only: %i[foo bar] }, + } + ) + EOS + + expect(cop.offenses.size).to eq(2) + expect(cop.highlights).to contain_exactly( + 'milestone: {}', + 'assignee: { methods: :avatar_url }' + ) + expect(cop.messages.first).to start_with("Don't use `#{method}`") + end + + it 'handles a top-level `only` with child `include`s' do + inspect_source(cop, <<~EOS) + render json: @issue.#{method}( + only: [:foo, :bar], + include: { + assignee: { methods: :avatar_url }, + author: { only: %i[foo bar] } + } + ) + EOS + + expect(cop.offenses.size).to eq(1) + expect(cop.highlights) + .to contain_exactly('assignee: { methods: :avatar_url }') + expect(cop.messages.first).to start_with("Don't use `#{method}`") + end + + it 'adds an offense for `include: [...]`' do + inspect_source(cop, <<~EOS) + render json: @issue.#{method}(include: %i[foo bar baz]) + EOS + + expect(cop.offenses.size).to eq(1) + expect(cop.highlights).to contain_exactly('include: %i[foo bar baz]') + expect(cop.messages.first).to start_with("Don't use `#{method}`") + end + + it 'adds an offense for `except`' do + inspect_source(cop, <<~EOS) + render json: @issue.#{method}(except: [:private_token]) + EOS + + expect(cop.offenses.size).to eq(1) + expect(cop.highlights).to contain_exactly('except: [:private_token]') + expect(cop.messages.first).to start_with("Don't use `#{method}`") + end + end + end + + it_behaves_like 'an upstanding constable', :to_json + it_behaves_like 'an upstanding constable', :as_json +end diff --git a/spec/rubocop/cop/security/to_json_spec.rb b/spec/rubocop/cop/security/to_json_spec.rb deleted file mode 100644 index c81d41c0a55..00000000000 --- a/spec/rubocop/cop/security/to_json_spec.rb +++ /dev/null @@ -1,128 +0,0 @@ -require 'spec_helper' - -require 'rubocop' -require 'rubocop/rspec/support' - -require_relative '../../../../rubocop/cop/security/to_json' - -describe RuboCop::Cop::Security::ToJson do - include CopHelper - - subject(:cop) { described_class.new } - - it 'ignores calls except `to_json`' do - inspect_source(cop, 'render json: foo') - - expect(cop.offenses).to be_empty - end - - context '`to_json` without options' do - it 'does nothing when sent to nil receiver' do - inspect_source(cop, 'to_json') - - expect(cop.offenses).to be_empty - end - - it 'does nothing when sent to a Hash' do - inspect_source(cop, '{}.to_json') - - expect(cop.offenses).to be_empty - end - - it 'does nothing when sent to a Serializer instance' do - inspect_source(cop, 'MergeRequestSerializer.new.represent(issuable).to_json') - - expect(cop.offenses).to be_empty - end - - it 'adds an offense when sent to any other receiver' do - inspect_source(cop, 'foo.to_json') - - aggregate_failures do - expect(cop.offenses.size).to eq(1) - expect(cop.highlights).to contain_exactly('foo.to_json') - end - end - end - - context '`to_json` with options' do - it 'does nothing when provided `only`' do - inspect_source(cop, <<~EOS) - render json: @issue.to_json(only: [:name, :username]) - EOS - - expect(cop.offenses).to be_empty - end - - it 'does nothing when provided `only` and `methods`' do - inspect_source(cop, <<~EOS) - render json: @issue.to_json( - only: [:name, :username], - methods: [:avatar_url] - ) - EOS - - expect(cop.offenses).to be_empty - end - - it 'adds an offense to `include`d attributes without `only` option' do - inspect_source(cop, <<~EOS) - render json: @issue.to_json( - include: { - milestone: {}, - assignee: { methods: :avatar_url }, - author: { only: %i[foo bar] }, - } - ) - EOS - - aggregate_failures do - expect(cop.offenses.size).to eq(2) - expect(cop.highlights).to contain_exactly( - 'milestone: {}', - 'assignee: { methods: :avatar_url }' - ) - end - end - - it 'handles a top-level `only` with child `include`s' do - inspect_source(cop, <<~EOS) - render json: @issue.to_json( - only: [:foo, :bar], - include: { - assignee: { methods: :avatar_url }, - author: { only: %i[foo bar] } - } - ) - EOS - - aggregate_failures do - expect(cop.offenses.size).to eq(1) - expect(cop.highlights) - .to contain_exactly('assignee: { methods: :avatar_url }') - end - end - - it 'adds an offense for `include: [...]`' do - inspect_source(cop, <<~EOS) - render json: @issue.to_json(include: %i[foo bar baz]) - EOS - - aggregate_failures do - expect(cop.offenses.size).to eq(1) - expect(cop.highlights).to contain_exactly('include: %i[foo bar baz]') - end - end - - it 'adds an offense for `except`' do - inspect_source(cop, <<~EOS) - render json: @issue.to_json(except: [:private_token]) - EOS - - aggregate_failures do - expect(cop.offenses.size).to eq(1) - expect(cop.highlights).to contain_exactly('except: [:private_token]') - end - end - end -end |