diff options
author | GitLab Release Tools Bot <robert+release-tools@gitlab.com> | 2019-08-30 00:34:15 +0300 |
---|---|---|
committer | GitLab Release Tools Bot <robert+release-tools@gitlab.com> | 2019-08-30 00:34:15 +0300 |
commit | b01c7ad291a81bc23d2c3fe7266eaf05de6cb434 (patch) | |
tree | 87f9ce5d469b330aa336f675194d1ce11b4b38c2 | |
parent | a5b2a3786056ddf99de06c8315e9a42c3bf86cd5 (diff) | |
parent | 5af535d919c50951513f5859730afd924a01c29b (diff) |
Merge branch 'security-61974-limit-issue-comment-size' into 'master'
Limit the size of issuable description and comments
See merge request gitlab/gitlabhq!3267
-rw-r--r-- | app/models/concerns/issuable.rb | 1 | ||||
-rw-r--r-- | app/models/note.rb | 1 | ||||
-rw-r--r-- | changelogs/unreleased/security-61974-limit-issue-comment-size-2.yml | 5 | ||||
-rw-r--r-- | changelogs/unreleased/security-61974-limit-issue-comment-size.yml | 5 | ||||
-rw-r--r-- | doc/api/epics.md | 4 | ||||
-rw-r--r-- | doc/api/issues.md | 4 | ||||
-rw-r--r-- | doc/api/merge_requests.md | 4 | ||||
-rw-r--r-- | doc/api/notes.md | 16 | ||||
-rw-r--r-- | lib/gitlab/database.rb | 4 | ||||
-rw-r--r-- | lib/gitlab/path_regex.rb | 2 | ||||
-rw-r--r-- | spec/lib/banzai/filter/project_reference_filter_spec.rb | 16 | ||||
-rw-r--r-- | spec/models/concerns/issuable_spec.rb | 26 | ||||
-rw-r--r-- | spec/models/note_spec.rb | 1 | ||||
-rw-r--r-- | spec/support/shared_examples/models/concern/issuable_shared_examples.rb | 8 |
14 files changed, 78 insertions, 19 deletions
diff --git a/app/models/concerns/issuable.rb b/app/models/concerns/issuable.rb index db46d7afbb9..eefe9f00836 100644 --- a/app/models/concerns/issuable.rb +++ b/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) } diff --git a/app/models/note.rb b/app/models/note.rb index a12d1eb7243..79aad5cbff9 100644 --- a/app/models/note.rb +++ b/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 diff --git a/changelogs/unreleased/security-61974-limit-issue-comment-size-2.yml b/changelogs/unreleased/security-61974-limit-issue-comment-size-2.yml new file mode 100644 index 00000000000..962171dc6f8 --- /dev/null +++ b/changelogs/unreleased/security-61974-limit-issue-comment-size-2.yml @@ -0,0 +1,5 @@ +--- +title: Speed up regexp in namespace format by failing fast after reaching maximum namespace depth +merge_request: +author: +type: security diff --git a/changelogs/unreleased/security-61974-limit-issue-comment-size.yml b/changelogs/unreleased/security-61974-limit-issue-comment-size.yml new file mode 100644 index 00000000000..6d5ef057d83 --- /dev/null +++ b/changelogs/unreleased/security-61974-limit-issue-comment-size.yml @@ -0,0 +1,5 @@ +--- +title: Limit the size of issuable description and comments +merge_request: +author: +type: security diff --git a/doc/api/epics.md b/doc/api/epics.md index 87ae0c48199..08eb84bfb63 100644 --- a/doc/api/epics.md +++ b/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) | diff --git a/doc/api/issues.md b/doc/api/issues.md index 8313dd2c3bd..1b0279154c0 100644 --- a/doc/api/issues.md +++ b/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.| diff --git a/doc/api/merge_requests.md b/doc/api/merge_requests.md index 49ed4968b0d..0d030ef30c8 100644 --- a/doc/api/merge_requests.md +++ b/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 | diff --git a/doc/api/notes.md b/doc/api/notes.md index acbf0334563..d7183df1387 100644 --- a/doc/api/notes.md +++ b/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 diff --git a/lib/gitlab/database.rb b/lib/gitlab/database.rb index cbdff0ab060..707466426db 100644 --- a/lib/gitlab/database.rb +++ b/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 diff --git a/lib/gitlab/path_regex.rb b/lib/gitlab/path_regex.rb index f96466b2b00..d9c28ff1181 100644 --- a/lib/gitlab/path_regex.rb +++ b/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 diff --git a/spec/lib/banzai/filter/project_reference_filter_spec.rb b/spec/lib/banzai/filter/project_reference_filter_spec.rb index 69f9c1ae829..927d226c400 100644 --- a/spec/lib/banzai/filter/project_reference_filter_spec.rb +++ b/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 diff --git a/spec/models/concerns/issuable_spec.rb b/spec/models/concerns/issuable_spec.rb index 39680c0e51a..65d41edc035 100644 --- a/spec/models/concerns/issuable_spec.rb +++ b/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 diff --git a/spec/models/note_spec.rb b/spec/models/note_spec.rb index bfd0e5f0558..927fbdb93d8 100644 --- a/spec/models/note_spec.rb +++ b/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) } diff --git a/spec/support/shared_examples/models/concern/issuable_shared_examples.rb b/spec/support/shared_examples/models/concern/issuable_shared_examples.rb new file mode 100644 index 00000000000..9604555c57d --- /dev/null +++ b/spec/support/shared_examples/models/concern/issuable_shared_examples.rb @@ -0,0 +1,8 @@ +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 |