Add latest changes from gitlab-org/security/gitlab@14-4-stable-ee

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 56e7413c530..bb182d96bc4 100644
--- a/locale/gitlab.pot
+++ b/locale/gitlab.pot
@@ -40994,6 +40994,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