diff options
author | GitLab Bot <gitlab-bot@gitlab.com> | 2021-12-03 10:03:58 +0000 |
---|---|---|
committer | GitLab Bot <gitlab-bot@gitlab.com> | 2021-12-03 10:04:17 +0000 |
commit | a5afc44487c1f49f9e8b2b7749469cefe679f6c6 (patch) | |
tree | db270d5e8125bc2fa99987e7bbc9165d687fa2f8 | |
parent | 960e4b3e3540de808e8e219f5fbec2df24442184 (diff) | |
download | gitlab-ce-a5afc44487c1f49f9e8b2b7749469cefe679f6c6.tar.gz |
Add latest changes from gitlab-org/security/gitlab@14-3-stable-ee
-rw-r--r-- | app/models/concerns/diff_positionable_note.rb | 1 | ||||
-rw-r--r-- | app/validators/json_schema_validator.rb | 4 | ||||
-rw-r--r-- | app/validators/json_schemas/position.json | 151 | ||||
-rw-r--r-- | lib/gitlab/diff/lines_unfolder.rb | 1 | ||||
-rw-r--r-- | locale/gitlab.pot | 3 | ||||
-rw-r--r-- | spec/factories/diff_position.rb | 8 | ||||
-rw-r--r-- | spec/frontend/diffs/store/utils_spec.js | 4 | ||||
-rw-r--r-- | spec/lib/gitlab/diff/formatters/text_formatter_spec.rb | 6 | ||||
-rw-r--r-- | spec/lib/gitlab/diff/lines_unfolder_spec.rb | 10 | ||||
-rw-r--r-- | spec/lib/gitlab/diff/position_tracer/line_strategy_spec.rb | 16 | ||||
-rw-r--r-- | spec/support/shared_examples/models/diff_positionable_note_shared_examples.rb | 33 | ||||
-rw-r--r-- | spec/support/shared_examples/requests/api/diff_discussions_shared_examples.rb | 12 | ||||
-rw-r--r-- | spec/validators/json_schema_validator_spec.rb | 12 |
13 files changed, 245 insertions, 16 deletions
diff --git a/app/models/concerns/diff_positionable_note.rb b/app/models/concerns/diff_positionable_note.rb index cea3c7d119c..b13ca4bf06e 100644 --- a/app/models/concerns/diff_positionable_note.rb +++ b/app/models/concerns/diff_positionable_note.rb @@ -12,6 +12,7 @@ module DiffPositionableNote serialize :change_position, Gitlab::Diff::Position # rubocop:disable Cop/ActiveRecordSerialize validate :diff_refs_match_commit, if: :for_commit? + validates :position, json_schema: { filename: "position", hash_conversion: true } end %i(original_position position change_position).each do |meth| diff --git a/app/validators/json_schema_validator.rb b/app/validators/json_schema_validator.rb index 68f03e8a6a3..4896c2ea2ef 100644 --- a/app/validators/json_schema_validator.rb +++ b/app/validators/json_schema_validator.rb @@ -24,8 +24,10 @@ class JsonSchemaValidator < ActiveModel::EachValidator end def validate_each(record, attribute, value) + value = value.to_h.stringify_keys if options[:hash_conversion] == true + unless valid_schema?(value) - record.errors.add(attribute, "must be a valid json schema") + record.errors.add(attribute, _("must be a valid json schema")) end end diff --git a/app/validators/json_schemas/position.json b/app/validators/json_schemas/position.json new file mode 100644 index 00000000000..d2c83be7639 --- /dev/null +++ b/app/validators/json_schemas/position.json @@ -0,0 +1,151 @@ +{ + "$schema": "http://json-schema.org/draft-07/schema#", + "description": "Gitlab::Diff::Position", + "type": "object", + "additionalProperties": false, + "properties": { + "base_sha": { + "oneOf": [ + { "type": "null" }, + { "type": "string", "maxLength": 40 } + ] + }, + "start_sha": { + "oneOf": [ + { "type": "null" }, + { "type": "string", "maxLength": 40 } + ] + }, + "head_sha": { + "oneOf": [ + { "type": "null" }, + { "type": "string", "maxLength": 40 } + ] + }, + "file_identifier_hash": { + "oneOf": [ + { "type": "null" }, + { "type": "string", "maxLength": 40 } + ] + }, + "old_path": { + "oneOf": [ + { "type": "null" }, + { "type": "string", "maxLength": 1000 } + ] + }, + "new_path": { + "oneOf": [ + { "type": "null" }, + { "type": "string", "maxLength": 1000 } + ] + }, + "position_type": { + "oneOf": [ + { "type": "null" }, + { "type": "string", "maxLength": 10 } + ] + }, + "old_line": { + "oneOf": [ + { "type": "null" }, + { "type": "integer" } + ] + }, + "new_line": { + "oneOf": [ + { "type": "null" }, + { "type": "integer" } + ] + }, + "line_range": { + "oneOf": [ + { "type": "null" }, + { + "type": "object", + "additionalProperties": false, + "properties": { + "start": { + "type": "object", + "additionalProperties": false, + "properties": { + "line_code": { "type": "string", "maxLength": 100 }, + "type": { + "oneOf": [ + { "type": "null" }, + { "type": "string", "maxLength": 100 } + ] + }, + "old_line": { + "oneOf": [ + { "type": "null" }, + { "type": "integer" } + ] + }, + "new_line": { + "oneOf": [ + { "type": "null" }, + { "type": "integer" } + ] + } + } + }, + "end": { + "type": "object", + "additionalProperties": false, + "properties": { + "line_code": { "type": "string", "maxLength": 100 }, + "type": { + "oneOf": [ + { "type": "null" }, + { "type": "string", "maxLength": 100 } + ] + }, + "old_line": { + "oneOf": [ + { "type": "null" }, + { "type": "integer" } + ] + }, + "new_line": { + "oneOf": [ + { "type": "null" }, + { "type": "integer" } + ] + } + } + } + } + } + ] + }, + "width": { + "oneOf": [ + { "type": "null" }, + { "type": "integer" }, + { "type": "string", "maxLength": 10 } + ] + }, + "height": { + "oneOf": [ + { "type": "null" }, + { "type": "integer" }, + { "type": "string", "maxLength": 10 } + ] + }, + "x": { + "oneOf": [ + { "type": "null" }, + { "type": "integer" }, + { "type": "string", "maxLength": 10 } + ] + }, + "y": { + "oneOf": [ + { "type": "null" }, + { "type": "integer" }, + { "type": "string", "maxLength": 10 } + ] + } + } +} diff --git a/lib/gitlab/diff/lines_unfolder.rb b/lib/gitlab/diff/lines_unfolder.rb index 6def3a074a3..04ed5857233 100644 --- a/lib/gitlab/diff/lines_unfolder.rb +++ b/lib/gitlab/diff/lines_unfolder.rb @@ -57,6 +57,7 @@ module Gitlab next false unless @position.unfoldable? next false if @diff_file.new_file? || @diff_file.deleted_file? next false unless @position.old_line + next false unless @position.old_line.is_a?(Integer) # Invalid position (MR import scenario) next false if @position.old_line > @blob.lines.size next false if @diff_file.diff_lines.empty? diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 11feb4d2a41..46340d00d05 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -40515,6 +40515,9 @@ msgstr "" msgid "must be a valid IPv4 or IPv6 address" msgstr "" +msgid "must be a valid json schema" +msgstr "" + msgid "must be after start" msgstr "" diff --git a/spec/factories/diff_position.rb b/spec/factories/diff_position.rb index 41f9a7b574e..bd248452de8 100644 --- a/spec/factories/diff_position.rb +++ b/spec/factories/diff_position.rb @@ -43,8 +43,12 @@ FactoryBot.define do trait :multi_line do line_range do { - start_line_code: Gitlab::Git.diff_line_code(file, 10, 10), - end_line_code: Gitlab::Git.diff_line_code(file, 12, 13) + start: { + line_code: Gitlab::Git.diff_line_code(file, 10, 10) + }, + end: { + line_code: Gitlab::Git.diff_line_code(file, 12, 13) + } } end end diff --git a/spec/frontend/diffs/store/utils_spec.js b/spec/frontend/diffs/store/utils_spec.js index 73de0a6d381..55c0141552d 100644 --- a/spec/frontend/diffs/store/utils_spec.js +++ b/spec/frontend/diffs/store/utils_spec.js @@ -138,7 +138,7 @@ describe('DiffsStoreUtils', () => { old_line: 1, }, linePosition: LINE_POSITION_LEFT, - lineRange: { start_line_code: 'abc_1_1', end_line_code: 'abc_2_2' }, + lineRange: { start: { line_code: 'abc_1_1' }, end: { line_code: 'abc_2_2' } }, }; const position = JSON.stringify({ @@ -608,7 +608,7 @@ describe('DiffsStoreUtils', () => { // When multi line comments are fully implemented `line_code` will be // included in all requests. Until then we need to ensure the logic does // not change when it is included only in the "comparison" argument. - const lineRange = { start_line_code: 'abc_1_1', end_line_code: 'abc_1_2' }; + const lineRange = { start: { line_code: 'abc_1_1' }, end: { line_code: 'abc_1_2' } }; it('returns true when the discussion is up to date', () => { expect( diff --git a/spec/lib/gitlab/diff/formatters/text_formatter_spec.rb b/spec/lib/gitlab/diff/formatters/text_formatter_spec.rb index 41877a16ebf..b6bdc5ff493 100644 --- a/spec/lib/gitlab/diff/formatters/text_formatter_spec.rb +++ b/spec/lib/gitlab/diff/formatters/text_formatter_spec.rb @@ -47,14 +47,14 @@ RSpec.describe Gitlab::Diff::Formatters::TextFormatter do describe "#==" do it "is false when the line_range changes" do - formatter_1 = described_class.new(base.merge(line_range: { start_line_code: "foo", end_line_code: "bar" })) - formatter_2 = described_class.new(base.merge(line_range: { start_line_code: "foo", end_line_code: "baz" })) + formatter_1 = described_class.new(base.merge(line_range: { "start": { "line_code" => "foo" }, "end": { "line_code" => "bar" } })) + formatter_2 = described_class.new(base.merge(line_range: { "start": { "line_code" => "foo" }, "end": { "line_code" => "baz" } })) expect(formatter_1).not_to eq(formatter_2) end it "is true when the line_range doesn't change" do - attrs = base.merge({ line_range: { start_line_code: "foo", end_line_code: "baz" } }) + attrs = base.merge({ line_range: { start: { line_code: "foo" }, end: { line_code: "baz" } } }) formatter_1 = described_class.new(attrs) formatter_2 = described_class.new(attrs) diff --git a/spec/lib/gitlab/diff/lines_unfolder_spec.rb b/spec/lib/gitlab/diff/lines_unfolder_spec.rb index 8385cba3532..f0e710be2e4 100644 --- a/spec/lib/gitlab/diff/lines_unfolder_spec.rb +++ b/spec/lib/gitlab/diff/lines_unfolder_spec.rb @@ -215,6 +215,16 @@ RSpec.describe Gitlab::Diff::LinesUnfolder do build(:text_diff_position, old_line: 43, new_line: 40) end + context 'old_line is an invalid number' do + let(:position) do + build(:text_diff_position, old_line: "foo", new_line: 40) + end + + it 'fails gracefully' do + expect(subject.unfolded_diff_lines).to be_nil + end + end + context 'blob lines' do let(:expected_blob_lines) do [[40, 40, " \"config-opts\": [ \"--disable-introspection\" ],"], diff --git a/spec/lib/gitlab/diff/position_tracer/line_strategy_spec.rb b/spec/lib/gitlab/diff/position_tracer/line_strategy_spec.rb index bdeaabec1f1..752ef7f6b50 100644 --- a/spec/lib/gitlab/diff/position_tracer/line_strategy_spec.rb +++ b/spec/lib/gitlab/diff/position_tracer/line_strategy_spec.rb @@ -295,8 +295,12 @@ RSpec.describe Gitlab::Diff::PositionTracer::LineStrategy, :clean_gitlab_redis_c new_path: file_name, new_line: 2, line_range: { - "start_line_code" => 1, - "end_line_code" => 2 + "start" => { + "line_code" => 1 + }, + "end" => { + "line_code" => 2 + } } ) end @@ -575,8 +579,12 @@ RSpec.describe Gitlab::Diff::PositionTracer::LineStrategy, :clean_gitlab_redis_c new_path: file_name, new_line: 2, line_range: { - "start_line_code" => 1, - "end_line_code" => 2 + "start" => { + "line_code" => 1 + }, + "end" => { + "line_code" => 2 + } } ) end diff --git a/spec/support/shared_examples/models/diff_positionable_note_shared_examples.rb b/spec/support/shared_examples/models/diff_positionable_note_shared_examples.rb index 759b22f794e..eafa589a1d3 100644 --- a/spec/support/shared_examples/models/diff_positionable_note_shared_examples.rb +++ b/spec/support/shared_examples/models/diff_positionable_note_shared_examples.rb @@ -71,5 +71,38 @@ RSpec.shared_examples 'a valid diff positionable note' do |factory_on_commit| end end end + + describe 'schema validation' do + where(:position_attrs) do + [ + { old_path: SecureRandom.alphanumeric(1001) }, + { new_path: SecureRandom.alphanumeric(1001) }, + { old_line: "foo" }, # this should be an integer + { new_line: "foo" }, # this should be an integer + { line_range: { "foo": "bar" } }, + { line_range: { "line_code": SecureRandom.alphanumeric(101) } }, + { line_range: { "type": SecureRandom.alphanumeric(101) } }, + { line_range: { "old_line": "foo" } }, + { line_range: { "new_line": "foo" } } + ] + end + + with_them do + let(:position) do + Gitlab::Diff::Position.new( + { + old_path: "files/ruby/popen.rb", + new_path: "files/ruby/popen.rb", + old_line: nil, + new_line: 14, + line_range: nil, + diff_refs: diff_refs + }.merge(position_attrs) + ) + end + + it { is_expected.to be_invalid } + end + end end end diff --git a/spec/support/shared_examples/requests/api/diff_discussions_shared_examples.rb b/spec/support/shared_examples/requests/api/diff_discussions_shared_examples.rb index 518c5b8dc28..7f2c445e93d 100644 --- a/spec/support/shared_examples/requests/api/diff_discussions_shared_examples.rb +++ b/spec/support/shared_examples/requests/api/diff_discussions_shared_examples.rb @@ -29,10 +29,14 @@ RSpec.shared_examples 'diff discussions API' do |parent_type, noteable_type, id_ describe "POST /#{parent_type}/:id/#{noteable_type}/:noteable_id/discussions" do it "creates a new diff note" do line_range = { - "start_line_code" => Gitlab::Git.diff_line_code(diff_note.position.file_path, 1, 1), - "end_line_code" => Gitlab::Git.diff_line_code(diff_note.position.file_path, 2, 2), - "start_line_type" => diff_note.position.type, - "end_line_type" => diff_note.position.type + "start" => { + "line_code" => Gitlab::Git.diff_line_code(diff_note.position.file_path, 1, 1), + "type" => diff_note.position.type + }, + "end" => { + "line_code" => Gitlab::Git.diff_line_code(diff_note.position.file_path, 2, 2), + "type" => diff_note.position.type + } } position = diff_note.position.to_h.merge({ line_range: line_range }) diff --git a/spec/validators/json_schema_validator_spec.rb b/spec/validators/json_schema_validator_spec.rb index 83eb0e2f3dd..01caf4ab0bd 100644 --- a/spec/validators/json_schema_validator_spec.rb +++ b/spec/validators/json_schema_validator_spec.rb @@ -46,5 +46,17 @@ RSpec.describe JsonSchemaValidator do expect { subject }.to raise_error(described_class::FilenameError) end end + + describe 'hash_conversion option' do + context 'when hash_conversion is enabled' do + let(:validator) { described_class.new(attributes: [:data], filename: "build_report_result_data", hash_conversion: true) } + + it 'returns no errors' do + subject + + expect(build_report_result.errors).to be_empty + end + end + end end end |