From 75256bdee832e45650ea3f4bacac9e3a47f0a7b6 Mon Sep 17 00:00:00 2001 From: Vasilii Iakliushin Date: Fri, 29 Jul 2022 17:02:37 +0200 Subject: [PATCH 1/3] Don't allow negative values for pagination Sentry error: https://sentry.gitlab.net/gitlab/gitlabcom/issues/3326721 **Problem** GitLab API does not support negaative values for pagination and result can be unexpected. **Solution** Return 400 error if negative value is provided to the pagination. Changelog: fixed --- lib/api/pagination_params.rb | 6 ++++-- spec/requests/api/commits_spec.rb | 22 +++++++++++++++++++--- 2 files changed, 23 insertions(+), 5 deletions(-) diff --git a/lib/api/pagination_params.rb b/lib/api/pagination_params.rb index 85ac50d5becb8b..bc68ecc0afe936 100644 --- a/lib/api/pagination_params.rb +++ b/lib/api/pagination_params.rb @@ -17,8 +17,10 @@ module PaginationParams included do helpers do params :pagination do - optional :page, type: Integer, default: 1, desc: 'Current page number' - optional :per_page, type: Integer, default: 20, desc: 'Number of items per page', except_values: [0] + with(type: Integer, values: ->(v) { !v.is_a?(Integer) || v > 0 }) do + optional :page, default: 1, desc: 'Current page number' + optional :per_page, default: 20, desc: 'Number of items per page' + end end end end diff --git a/spec/requests/api/commits_spec.rb b/spec/requests/api/commits_spec.rb index 9ef845f06bf75b..49d4f1731b1fd4 100644 --- a/spec/requests/api/commits_spec.rb +++ b/spec/requests/api/commits_spec.rb @@ -238,10 +238,26 @@ end end - context 'when per_page is 0' do - let(:per_page) { 0 } + context 'when pagination params are invalid' do + let_it_be(:project) { create(:project, :empty_repo) } + + using RSpec::Parameterized::TableSyntax + + where(:page, :per_page, :error_message) do + 0 | 1 | 'page does not have a valid value' + -1 | 1 | 'page does not have a valid value' + 'a' | 1 | 'page is invalid' + 1 | 0 | 'per_page does not have a valid value' + 1 | -1 | 'per_page does not have a valid value' + 1 | 'a' | 'per_page is invalid' + end - it_behaves_like '400 response' + with_them do + it 'returns 400 response' do + expect(response).to have_gitlab_http_status(:bad_request) + expect(json_response['error']).to eq(error_message) + end + end end end -- GitLab From 39243ea3fd8df1faea6218b0b38aeb73dd3dbe56 Mon Sep 17 00:00:00 2001 From: Vasilii Iakliushin Date: Mon, 1 Aug 2022 10:38:43 +0200 Subject: [PATCH 2/3] Add feature flag to pagination params --- .../development/only_positive_pagination_values.yml | 8 ++++++++ lib/api/pagination_params.rb | 4 +++- 2 files changed, 11 insertions(+), 1 deletion(-) create mode 100644 config/feature_flags/development/only_positive_pagination_values.yml diff --git a/config/feature_flags/development/only_positive_pagination_values.yml b/config/feature_flags/development/only_positive_pagination_values.yml new file mode 100644 index 00000000000000..9347c628e65be5 --- /dev/null +++ b/config/feature_flags/development/only_positive_pagination_values.yml @@ -0,0 +1,8 @@ +--- +name: only_positive_pagination_values +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/93571 +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/369225 +milestone: '15.3' +type: development +group: group::source code +default_enabled: false diff --git a/lib/api/pagination_params.rb b/lib/api/pagination_params.rb index bc68ecc0afe936..5d5b5b8fb63cf4 100644 --- a/lib/api/pagination_params.rb +++ b/lib/api/pagination_params.rb @@ -17,7 +17,9 @@ module PaginationParams included do helpers do params :pagination do - with(type: Integer, values: ->(v) { !v.is_a?(Integer) || v > 0 }) do + with(type: Integer, values: ->(v) do + !v.is_a?(Integer) || Feature.disabled?(:only_positive_pagination_values) || v > 0 + end) do optional :page, default: 1, desc: 'Current page number' optional :per_page, default: 20, desc: 'Number of items per page' end -- GitLab From 9b8270fba7161ff5e29e39d7c1855f1939abb136 Mon Sep 17 00:00:00 2001 From: Vasilii Iakliushin Date: Wed, 3 Aug 2022 17:45:45 +0200 Subject: [PATCH 3/3] Dynamically verify pagination params --- lib/api/commits.rb | 2 ++ lib/api/pagination_params.rb | 26 +++++++++++++--- spec/requests/api/commits_spec.rb | 51 ++++++++++++++++++++++++++----- 3 files changed, 66 insertions(+), 13 deletions(-) diff --git a/lib/api/commits.rb b/lib/api/commits.rb index 5fd9a8e32789f0..221e044b24d0c8 100644 --- a/lib/api/commits.rb +++ b/lib/api/commits.rb @@ -10,6 +10,8 @@ class Commits < ::API::Base before do require_repository_enabled! authorize! :download_code, user_project + + verify_pagination_params! end helpers do diff --git a/lib/api/pagination_params.rb b/lib/api/pagination_params.rb index 5d5b5b8fb63cf4..bdb69d0ba44ad0 100644 --- a/lib/api/pagination_params.rb +++ b/lib/api/pagination_params.rb @@ -17,12 +17,28 @@ module PaginationParams included do helpers do params :pagination do - with(type: Integer, values: ->(v) do - !v.is_a?(Integer) || Feature.disabled?(:only_positive_pagination_values) || v > 0 - end) do - optional :page, default: 1, desc: 'Current page number' - optional :per_page, default: 20, desc: 'Number of items per page' + optional :page, type: Integer, default: 1, desc: 'Current page number' + optional :per_page, type: Integer, default: 20, desc: 'Number of items per page', except_values: [0] + end + + def verify_pagination_params! + return if Feature.disabled?(:only_positive_pagination_values) + + page = begin + Integer(params[:page]) + rescue ArgumentError, TypeError + nil end + + return render_structured_api_error!({ error: 'page does not have a valid value' }, 400) if page&.< 1 + + per_page = begin + Integer(params[:per_page]) + rescue ArgumentError, TypeError + nil + end + + return render_structured_api_error!({ error: 'per_page does not have a valid value' }, 400) if per_page&.< 1 end end end diff --git a/spec/requests/api/commits_spec.rb b/spec/requests/api/commits_spec.rb index 49d4f1731b1fd4..7c7a167d71df7d 100644 --- a/spec/requests/api/commits_spec.rb +++ b/spec/requests/api/commits_spec.rb @@ -206,11 +206,13 @@ let(:page) { 1 } let(:per_page) { 5 } let(:ref_name) { 'master' } - let!(:request) do + let(:request) do get api("/projects/#{project_id}/repository/commits?page=#{page}&per_page=#{per_page}&ref_name=#{ref_name}", user) end it 'returns correct headers' do + request + expect(response).to include_limited_pagination_headers expect(response.headers['Link']).to match(/page=1&per_page=5/) expect(response.headers['Link']).to match(/page=2&per_page=5/) @@ -218,6 +220,8 @@ context 'viewing the first page' do it 'returns the first 5 commits' do + request + commit = project.repository.commit expect(json_response.size).to eq(per_page) @@ -230,6 +234,8 @@ let(:page) { 3 } it 'returns the third 5 commits' do + request + commit = project.repository.commits('HEAD', limit: per_page, offset: (page - 1) * per_page).first expect(json_response.size).to eq(per_page) @@ -239,25 +245,54 @@ end context 'when pagination params are invalid' do - let_it_be(:project) { create(:project, :empty_repo) } + let_it_be(:project) { create(:project, :repository) } using RSpec::Parameterized::TableSyntax where(:page, :per_page, :error_message) do - 0 | 1 | 'page does not have a valid value' - -1 | 1 | 'page does not have a valid value' - 'a' | 1 | 'page is invalid' - 1 | 0 | 'per_page does not have a valid value' - 1 | -1 | 'per_page does not have a valid value' - 1 | 'a' | 'per_page is invalid' + 0 | nil | 'page does not have a valid value' + -1 | nil | 'page does not have a valid value' + 'a' | nil | 'page is invalid' + nil | 0 | 'per_page does not have a valid value' + nil | -1 | 'per_page does not have a valid value' + nil | 'a' | 'per_page is invalid' end with_them do it 'returns 400 response' do + request + expect(response).to have_gitlab_http_status(:bad_request) expect(json_response['error']).to eq(error_message) end end + + context 'when FF is off' do + before do + stub_feature_flags(only_positive_pagination_values: false) + end + + where(:page, :per_page, :error_message, :status) do + 0 | nil | nil | :success + -10 | nil | nil | :internal_server_error + 'a' | nil | 'page is invalid' | :bad_request + nil | 0 | 'per_page has a value not allowed' | :bad_request + nil | -1 | nil | :success + nil | 'a' | 'per_page is invalid' | :bad_request + end + + with_them do + it 'returns a response' do + request + + expect(response).to have_gitlab_http_status(status) + + if error_message + expect(json_response['error']).to eq(error_message) + end + end + end + end end end -- GitLab