Merge branch 'security-61974-limit-issue-comment-size-ee' into 'master'

Limit the size of issuable description and comments

See merge request gitlab/gitlab-ee!1119

app/models/concerns/issuable.rb

@@ -73,6 +73,7 @@ module Issuable
 
     validates :author, presence: true
     validates :title, presence: true, length: { maximum: 255 }
+    validates :description, length: { maximum: Gitlab::Database::MAX_TEXT_SIZE_LIMIT }, allow_blank: true
     validate :milestone_is_valid
 
     scope :authored, ->(user) { where(author_id: user) }

app/models/note.rb

@@ -89,6 +89,7 @@ class Note < ApplicationRecord
   delegate :title, to: :noteable, allow_nil: true
 
   validates :note, presence: true
+  validates :note, length: { maximum: Gitlab::Database::MAX_TEXT_SIZE_LIMIT }
   validates :project, presence: true, if: :for_project_noteable?
 
   # Attachments are deprecated and are handled by Markdown uploader

changelogs/unreleased/security-61974-limit-issue-comment-size-2.yml

+---
+title: Speed up regexp in namespace format by failing fast after reaching maximum namespace depth
+merge_request:
+author:
+type: security

changelogs/unreleased/security-61974-limit-issue-comment-size.yml

+---
+title: Limit the size of issuable description and comments
+merge_request:
+author:
+type: security

doc/api/epics.md

@@ -165,7 +165,7 @@ POST /groups/:id/epics
 | `id`                | integer/string   | yes        | The ID or [URL-encoded path of the group](README.md#namespaced-path-encoding) owned by the authenticated user                |
 | `title`             | string           | yes        | The title of the epic |
 | `labels`            | string           | no         | The comma separated list of labels |
-| `description`       | string           | no         | The description of the epic  |
+| `description`       | string           | no         | The description of the epic. Limited to 1 000 000 characters.  |
 | `start_date_is_fixed` | boolean        | no         | Whether start date should be sourced from `start_date_fixed` or from milestones (since 11.3) |
 | `start_date_fixed`  | string           | no         | The fixed start date of an epic (since 11.3) |
 | `due_date_is_fixed` | boolean          | no         | Whether due date should be sourced from `due_date_fixed` or from milestones (since 11.3) |
@@ -231,7 +231,7 @@ PUT /groups/:id/epics/:epic_iid
 | `id`                | integer/string   | yes        | The ID or [URL-encoded path of the group](README.md#namespaced-path-encoding) owned by the authenticated user                |
 | `epic_iid`          | integer/string   | yes        | The internal ID  of the epic  |
 | `title`             | string           | no         | The title of an epic |
-| `description`       | string           | no         | The description of an epic  |
+| `description`       | string           | no         | The description of an epic. Limited to 1 000 000 characters.  |
 | `labels`            | string           | no         | The comma separated list of labels |
 | `start_date_is_fixed` | boolean        | no         | Whether start date should be sourced from `start_date_fixed` or from milestones (since 11.3) |
 | `start_date_fixed`  | string           | no         | The fixed start date of an epic (since 11.3) |

doc/api/issues.md

@@ -591,7 +591,7 @@ POST /projects/:id/issues
 | `id`                                      | integer/string | yes      | The ID or [URL-encoded path of the project](README.md#namespaced-path-encoding) owned by the authenticated user |
 | `iid`                                     | integer/string | no       | The internal ID of the project's issue (requires admin or project owner rights) |
 | `title`                                   | string         | yes      | The title of an issue |
-| `description`                             | string         | no       | The description of an issue  |
+| `description`                             | string         | no       | The description of an issue. Limited to 1 000 000 characters. |
 | `confidential`                            | boolean        | no       | Set an issue to be confidential. Default is `false`.  |
 | `assignee_ids`                            | integer array  | no       | The ID of a user to assign issue |
 | `milestone_id`                            | integer        | no       | The global ID of a milestone to assign issue  |
@@ -692,7 +692,7 @@ PUT /projects/:id/issues/:issue_iid
 | `id`           | integer/string | yes | The ID or [URL-encoded path of the project](README.md#namespaced-path-encoding) owned by the authenticated user |
 | `issue_iid`    | integer | yes      | The internal ID of a project's issue                                                                       |
 | `title`        | string  | no       | The title of an issue                                                                                      |
-| `description`  | string  | no       | The description of an issue                                                                                |
+| `description`  | string  | no       | The description of an issue. Limited to 1 000 000 characters.        |
 | `confidential` | boolean | no       | Updates an issue to be confidential                                                                        |
 | `assignee_ids` | integer array | no | The ID of the user(s) to assign the issue to. Set to `0` or provide an empty value to unassign all assignees. |
 | `milestone_id` | integer | no       | The global ID of a milestone to assign the issue to. Set to `0` or provide an empty value to unassign a milestone.|

doc/api/merge_requests.md

@@ -837,7 +837,7 @@ POST /projects/:id/merge_requests
 | `title`                    | string  | yes      | Title of MR                                                                     |
 | `assignee_id`              | integer | no       | Assignee user ID                                                                |
 | `assignee_ids`             | integer array | no | The ID of the user(s) to assign the MR to. Set to `0` or provide an empty value to unassign all assignees.  |
-| `description`              | string  | no       | Description of MR                                                               |
+| `description`              | string  | no       | Description of MR. Limited to 1 000 000 characters. |
 | `target_project_id`        | integer | no       | The target project (numeric id)                                                 |
 | `labels`                   | string  | no       | Labels for MR as a comma-separated list                                         |
 | `milestone_id`             | integer | no       | The global ID of a milestone                                                           |
@@ -990,7 +990,7 @@ PUT /projects/:id/merge_requests/:merge_request_iid
 | `assignee_ids`             | integer array | no | The ID of the user(s) to assign the MR to. Set to `0` or provide an empty value to unassign all assignees.  |
 | `milestone_id`             | integer | no       | The global ID of a milestone to assign the merge request to. Set to `0` or provide an empty value to unassign a milestone.|
 | `labels`                   | string  | no       | Comma-separated label names for a merge request. Set to an empty string to unassign all labels.                    |
-| `description`              | string  | no       | Description of MR                                                               |
+| `description`              | string  | no       | Description of MR. Limited to 1 000 000 characters. |
 | `state_event`              | string  | no       | New state (close/reopen)                                                        |
 | `remove_source_branch`     | boolean | no       | Flag indicating if a merge request should remove the source branch when merging |
 | `squash`                   | boolean | no       | Squash commits into a single commit when merging |

doc/api/notes.md

@@ -113,7 +113,7 @@ Parameters:
 
 - `id` (required) - The ID or [URL-encoded path of the project](README.md#namespaced-path-encoding)
 - `issue_iid` (required) - The IID of an issue
-- `body` (required) - The content of a note
+- `body` (required) - The content of a note. Limited to 1 000 000 characters.
 - `created_at` (optional) - Date time string, ISO 8601 formatted, e.g. 2016-03-11T03:45:40Z (requires admin or project/group owner rights)
 
 ```bash
@@ -133,7 +133,7 @@ Parameters:
 - `id` (required) - The ID or [URL-encoded path of the project](README.md#namespaced-path-encoding)
 - `issue_iid` (required) - The IID of an issue
 - `note_id` (required) - The ID of a note
-- `body` (required) - The content of a note
+- `body` (required) - The content of a note. Limited to 1 000 000 characters.
 
 ```bash
 curl --request PUT --header "PRIVATE-TOKEN: <your_access_token>" https://gitlab.example.com/api/v4/projects/5/issues/11/notes?body=note
@@ -231,7 +231,7 @@ Parameters:
 
 - `id` (required) - The ID or [URL-encoded path of the project](README.md#namespaced-path-encoding)
 - `snippet_id` (required) - The ID of a snippet
-- `body` (required) - The content of a note
+- `body` (required) - The content of a note. Limited to 1 000 000 characters.
 - `created_at` (optional) - Date time string, ISO 8601 formatted, e.g. 2016-03-11T03:45:40Z
 
 ```bash
@@ -251,7 +251,7 @@ Parameters:
 - `id` (required) - The ID or [URL-encoded path of the project](README.md#namespaced-path-encoding)
 - `snippet_id` (required) - The ID of a snippet
 - `note_id` (required) - The ID of a note
-- `body` (required) - The content of a note
+- `body` (required) - The content of a note. Limited to 1 000 000 characters.
 
 ```bash
 curl --request PUT --header "PRIVATE-TOKEN: <your_access_token>" https://gitlab.example.com/api/v4/projects/5/snippets/11/notes?body=note
@@ -354,7 +354,7 @@ Parameters:
 
 - `id` (required) - The ID or [URL-encoded path of the project](README.md#namespaced-path-encoding)
 - `merge_request_iid` (required) - The IID of a merge request
-- `body` (required) - The content of a note
+- `body` (required) - The content of a note. Limited to 1 000 000 characters.
 - `created_at` (optional) - Date time string, ISO 8601 formatted, e.g. 2016-03-11T03:45:40Z
 
 ### Modify existing merge request note
@@ -370,7 +370,7 @@ Parameters:
 - `id` (required) - The ID or [URL-encoded path of the project](README.md#namespaced-path-encoding)
 - `merge_request_iid` (required) - The IID of a merge request
 - `note_id` (required) - The ID of a note
-- `body` (required) - The content of a note
+- `body` (required) - The content of a note. Limited to 1 000 000 characters.
 
 ```bash
 curl --request PUT --header "PRIVATE-TOKEN: <your_access_token>" https://gitlab.example.com/api/v4/projects/5/merge_requests/11/notes?body=note
@@ -472,7 +472,7 @@ Parameters:
 | --------- | -------------- | -------- | ----------- |
 | `id`      | integer/string | yes | The ID or [URL-encoded path of the group](README.md#namespaced-path-encoding) |
 | `epic_id` | integer | yes  | The ID of an epic |
-| `body`    | string  | yes  | The content of a note |
+| `body`    | string  | yes  | The content of a note. Limited to 1 000 000 characters. |
 
 ```bash
 curl --request POST --header "PRIVATE-TOKEN: <your_access_token>" https://gitlab.example.com/api/v4/projects/5/snippet/11/notes?body=note
@@ -493,7 +493,7 @@ Parameters:
 | `id`      | integer/string | yes | The ID or [URL-encoded path of the group](README.md#namespaced-path-encoding) |
 | `epic_id` | integer | yes  | The ID of an epic |
 | `note_id` | integer | yes  | The ID of a note |
-| `body`    | string  | yes  | The content of a note |
+| `body`    | string  | yes  | The content of a note. Limited to 1 000 000 characters. |
 
 ```bash
 curl --request PUT --header "PRIVATE-TOKEN: <your_access_token>" https://gitlab.example.com/api/v4/projects/5/snippet/11/notes?body=note

ee/spec/models/concerns/ee/issuable_spec.rb

@@ -3,6 +3,22 @@
 require 'spec_helper'
 
 describe EE::Issuable do
+  describe "Validation" do
+    context 'general validations' do
+      subject { build(:epic) }
+
+      before do
+        allow(InternalId).to receive(:generate_next).and_return(nil)
+      end
+
+      it { is_expected.to validate_presence_of(:iid) }
+      it { is_expected.to validate_presence_of(:author) }
+      it { is_expected.to validate_presence_of(:title) }
+      it { is_expected.to validate_length_of(:title).is_at_most(255) }
+      it { is_expected.to validate_length_of(:description).is_at_most(1_000_000) }
+    end
+  end
+
   describe '.labels_hash' do
     let(:feature_label) { create(:label, title: 'Feature') }
     let(:second_label) { create(:label, title: 'Second Label') }
@@ -50,4 +66,12 @@ describe EE::Issuable do
       end
     end
   end
+
+  describe '#matches_cross_reference_regex?' do
+    context "epic description with long path string" do
+      let(:mentionable) { build(:epic, description: "/a" * 50000) }
+
+      it_behaves_like 'matches_cross_reference_regex? fails fast'
+    end
+  end
 end

lib/gitlab/database.rb

@@ -13,6 +13,10 @@ module Gitlab
     # FIXME: this should just be the max value of timestampz
     MAX_TIMESTAMP_VALUE = Time.at((1 << 31) - 1).freeze
 
+    # The maximum number of characters for text fields, to avoid DoS attacks via parsing huge text fields
+    # https://gitlab.com/gitlab-org/gitlab-ce/issues/61974
+    MAX_TEXT_SIZE_LIMIT = 1_000_000
+
     # Minimum schema version from which migrations are supported
     # Migrations before this version may have been removed
     MIN_SCHEMA_VERSION = 20190506135400

lib/gitlab/path_regex.rb

@@ -132,7 +132,7 @@ module Gitlab
     NO_SUFFIX_REGEX = /(?<!\.git|\.atom)/.freeze
     NAMESPACE_FORMAT_REGEX = /(?:#{NAMESPACE_FORMAT_REGEX_JS})#{NO_SUFFIX_REGEX}/.freeze
     PROJECT_PATH_FORMAT_REGEX = /(?:#{PATH_REGEX_STR})#{NO_SUFFIX_REGEX}/.freeze
-    FULL_NAMESPACE_FORMAT_REGEX = %r{(#{NAMESPACE_FORMAT_REGEX}/)*#{NAMESPACE_FORMAT_REGEX}}.freeze
+    FULL_NAMESPACE_FORMAT_REGEX = %r{(#{NAMESPACE_FORMAT_REGEX}/){,#{Namespace::NUMBER_OF_ANCESTORS_ALLOWED}}#{NAMESPACE_FORMAT_REGEX}}.freeze
 
     def root_namespace_route_regex
       @root_namespace_route_regex ||= begin

spec/lib/banzai/filter/project_reference_filter_spec.rb

@@ -26,10 +26,18 @@ describe Banzai::Filter::ProjectReferenceFilter do
     expect(reference_filter(act).to_html).to eq(CGI.escapeHTML(exp))
   end
 
-  it 'fails fast for long invalid string' do
-    expect do
-      Timeout.timeout(5.seconds) { reference_filter("A" * 50000).to_html }
-    end.not_to raise_error
+  context 'when invalid reference strings are very long' do
+    shared_examples_for 'fails fast' do |ref_string|
+      it 'fails fast for long strings' do
+        # took well under 1 second in CI https://dev.gitlab.org/gitlab/gitlabhq/merge_requests/3267#note_172824
+        expect do
+          Timeout.timeout(3.seconds) { reference_filter(ref_string).to_html }
+        end.not_to raise_error
+      end
+    end
+
+    it_behaves_like 'fails fast', 'A' * 50000
+    it_behaves_like 'fails fast', '/a' * 50000
   end
 
   it 'allows references with text after the > character' do

spec/models/concerns/issuable_spec.rb

@@ -46,6 +46,7 @@ describe Issuable do
       it { is_expected.to validate_presence_of(:author) }
       it { is_expected.to validate_presence_of(:title) }
       it { is_expected.to validate_length_of(:title).is_at_most(255) }
+      it { is_expected.to validate_length_of(:description).is_at_most(1_000_000) }
     end
 
     describe 'milestone' do
@@ -795,4 +796,29 @@ describe Issuable do
       end
     end
   end
+
+  describe '#matches_cross_reference_regex?' do
+    context "issue description with long path string" do
+      let(:mentionable) { build(:issue, description: "/a" * 50000) }
+
+      it_behaves_like 'matches_cross_reference_regex? fails fast'
+    end
+
+    context "note with long path string" do
+      let(:mentionable) { build(:note, note: "/a" * 50000) }
+
+      it_behaves_like 'matches_cross_reference_regex? fails fast'
+    end
+
+    context "note with long path string" do
+      let(:project) { create(:project, :public, :repository) }
+      let(:mentionable) { project.commit }
+
+      before do
+        expect(mentionable.raw).to receive(:message).and_return("/a" * 50000)
+      end
+
+      it_behaves_like 'matches_cross_reference_regex? fails fast'
+    end
+  end
 end

spec/models/note_spec.rb

@@ -22,6 +22,7 @@ describe Note do
   end
 
   describe 'validation' do
+    it { is_expected.to validate_length_of(:note).is_at_most(1_000_000) }
     it { is_expected.to validate_presence_of(:note) }
     it { is_expected.to validate_presence_of(:project) }
 

spec/support/shared_examples/models/concern/issuable_shared_examples.rb

+shared_examples_for 'matches_cross_reference_regex? fails fast' do
+  it 'fails fast for long strings' do
+    # took well under 1 second in CI https://dev.gitlab.org/gitlab/gitlabhq/merge_requests/3267#note_172823
+    expect do
+      Timeout.timeout(3.seconds) { mentionable.matches_cross_reference_regex? }
+    end.not_to raise_error
+  end
+end