diff options
| -rw-r--r-- | app/models/merge_request.rb | 18 | ||||
| -rw-r--r-- | db/post_migrate/20221006083240_prepare_partial_trigram_indexes_for_issues_attempt_2.rb | 25 | ||||
| -rw-r--r-- | db/schema_migrations/20221006083240 | 1 | ||||
| -rw-r--r-- | lib/api/environments.rb | 4 | ||||
| -rw-r--r-- | lib/gitlab/middleware/go.rb | 35 | ||||
| -rw-r--r-- | spec/lib/gitlab/middleware/go_spec.rb | 22 | ||||
| -rw-r--r-- | spec/models/merge_request_spec.rb | 78 |
7 files changed, 57 insertions, 126 deletions
diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index a3c1ef8bd79..fb20d91fa20 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -71,23 +71,9 @@ class MergeRequest < ApplicationRecord belongs_to :latest_merge_request_diff, class_name: 'MergeRequestDiff' manual_inverse_association :latest_merge_request_diff, :merge_request + # method overriden in EE def suggested_reviewer_users - return User.none unless predictions && predictions.suggested_reviewers.is_a?(Hash) - - usernames = Array.wrap(suggested_reviewers["reviewers"]) - return User.none if usernames.empty? - - # Preserve the orginal order of suggested usernames - join_sql = MergeRequest.sanitize_sql_array( - [ - 'JOIN UNNEST(ARRAY[?]::varchar[]) WITH ORDINALITY AS t(username, ord) USING(username)', - usernames - ] - ) - - project.authorized_users.active - .joins(Arel.sql(join_sql)) - .order('t.ord') + User.none end # This is the same as latest_merge_request_diff unless: diff --git a/db/post_migrate/20221006083240_prepare_partial_trigram_indexes_for_issues_attempt_2.rb b/db/post_migrate/20221006083240_prepare_partial_trigram_indexes_for_issues_attempt_2.rb new file mode 100644 index 00000000000..6ca2ba222ae --- /dev/null +++ b/db/post_migrate/20221006083240_prepare_partial_trigram_indexes_for_issues_attempt_2.rb @@ -0,0 +1,25 @@ +# frozen_string_literal: true + +class PreparePartialTrigramIndexesForIssuesAttempt2 < Gitlab::Database::Migration[2.0] + TITLE_INDEX_NAME = 'index_issues_on_title_trigram_non_latin' + DESCRIPTION_INDEX_NAME = 'index_issues_on_description_trigram_non_latin' + + def up + prepare_async_index :issues, :title, + name: TITLE_INDEX_NAME, + using: :gin, opclass: { description: :gin_trgm_ops }, + where: "title NOT SIMILAR TO '[\\u0000-\\u02FF\\u1E00-\\u1EFF\\u2070-\\u218F]*' " \ + "OR description NOT SIMILAR TO '[\\u0000-\\u02FF\\u1E00-\\u1EFF\\u2070-\\u218F]*'" + + prepare_async_index :issues, :description, + name: DESCRIPTION_INDEX_NAME, + using: :gin, opclass: { description: :gin_trgm_ops }, + where: "title NOT SIMILAR TO '[\\u0000-\\u02FF\\u1E00-\\u1EFF\\u2070-\\u218F]*' " \ + "OR description NOT SIMILAR TO '[\\u0000-\\u02FF\\u1E00-\\u1EFF\\u2070-\\u218F]*'" + end + + def down + unprepare_async_index_by_name :issues, DESCRIPTION_INDEX_NAME + unprepare_async_index_by_name :issues, TITLE_INDEX_NAME + end +end diff --git a/db/schema_migrations/20221006083240 b/db/schema_migrations/20221006083240 new file mode 100644 index 00000000000..51c509f471c --- /dev/null +++ b/db/schema_migrations/20221006083240 @@ -0,0 +1 @@ +0ad92f76e14b2e9286b2f77f32c00dba8ae29b64035f79641451edfdc725c92a
\ No newline at end of file diff --git a/lib/api/environments.rb b/lib/api/environments.rb index c4b67f83941..42d5e6a73b3 100644 --- a/lib/api/environments.rb +++ b/lib/api/environments.rb @@ -40,7 +40,7 @@ module API params do requires :name, type: String, desc: 'The name of the environment to be created' optional :external_url, type: String, desc: 'URL on which this deployment is viewable' - optional :slug, absence: { message: "is automatically generated and cannot be changed" } + optional :slug, absence: { message: "is automatically generated and cannot be changed" }, documentation: { hidden: true } optional :tier, type: String, values: Environment.tiers.keys, desc: 'The tier of the environment to be created' end post ':id/environments' do @@ -64,7 +64,7 @@ module API # TODO: disallow renaming via the API https://gitlab.com/gitlab-org/gitlab/-/issues/338897 optional :name, type: String, desc: 'DEPRECATED: Renaming environment can lead to errors, this will be removed in 15.0' optional :external_url, type: String, desc: 'The new URL on which this deployment is viewable' - optional :slug, absence: { message: "is automatically generated and cannot be changed" } + optional :slug, absence: { message: "is automatically generated and cannot be changed" }, documentation: { hidden: true } optional :tier, type: String, values: Environment.tiers.keys, desc: 'The tier of the environment to be created' end put ':id/environments/:environment_id' do diff --git a/lib/gitlab/middleware/go.rb b/lib/gitlab/middleware/go.rb index e7b4084d153..dcbb4557377 100644 --- a/lib/gitlab/middleware/go.rb +++ b/lib/gitlab/middleware/go.rb @@ -2,9 +2,6 @@ # A dumb middleware that returns a Go HTML document if the go-get=1 query string # is used irrespective if the namespace/project exists -# -# In order to prevent the project from being exposed, -# auth failure (401 & 403) is regarded as not found (404) module Gitlab module Middleware class Go @@ -24,16 +21,15 @@ module Gitlab rescue Gitlab::Auth::IpBlacklisted Gitlab::AuthLogger.error( message: 'Rack_Attack', + status: 403, env: :blocklist, remote_ip: request.ip, request_method: request.request_method, path: request.fullpath ) - Rack::Response.new('', 404).finish + Rack::Response.new('', 403).finish rescue Gitlab::Auth::MissingPersonalAccessTokenError - Rack::Response.new('', 404).finish - rescue ActiveRecord::RecordNotFound - Rack::Response.new('', 404).finish + Rack::Response.new('', 401).finish end private @@ -104,6 +100,7 @@ module Gitlab # We find all potential project paths out of the path segments path_segments = path.split('/') + simple_project_path = path_segments.first(2).join('/') project_paths = [] begin @@ -113,18 +110,28 @@ module Gitlab # We see if a project exists with any of these potential paths project = project_for_paths(project_paths, request) - # If a project is found and the user has access, we return the full project path - [project.full_path, project.default_branch] + + if project + # If a project is found and the user has access, we return the full project path + [project.full_path, project.default_branch] + else + # If not, we return the first two components as if it were a simple `namespace/project` path, + # so that we don't reveal the existence of a nested project the user doesn't have access to. + # This means that for an unauthenticated request to `group/subgroup/project/subpackage` + # for a private `group/subgroup/project` with subpackage path `subpackage`, GitLab will respond + # as if the user is looking for project `group/subgroup`, with subpackage path `project/subpackage`. + # Since `go get` doesn't authenticate by default, this means that + # `go get gitlab.com/group/subgroup/project/subpackage` will not work for private projects. + # `go get gitlab.com/group/subgroup/project.git/subpackage` will work, since Go is smart enough + # to figure that out. `import 'gitlab.com/...'` behaves the same as `go get`. + [simple_project_path, 'master'] + end end def project_for_paths(paths, request) project = Project.where_full_path_in(paths).first - raise ActiveRecord::RecordNotFound unless project - - unless authentication_result(request, project).can_perform_action_on_project?(:read_project, project) - raise Gitlab::Auth::MissingPersonalAccessTokenError - end + return unless authentication_result(request, project).can_perform_action_on_project?(:read_project, project) project end diff --git a/spec/lib/gitlab/middleware/go_spec.rb b/spec/lib/gitlab/middleware/go_spec.rb index e795577d203..bc1d53b2ccb 100644 --- a/spec/lib/gitlab/middleware/go_spec.rb +++ b/spec/lib/gitlab/middleware/go_spec.rb @@ -32,7 +32,7 @@ RSpec.describe Gitlab::Middleware::Go do shared_examples 'go-get=1' do |enabled_protocol:| context 'with simple 2-segment project path' do - let!(:project) { create(:project, :public, :repository) } + let!(:project) { create(:project, :private, :repository) } context 'with subpackages' do let(:path) { "#{project.full_path}/subpackage" } @@ -51,16 +51,6 @@ RSpec.describe Gitlab::Middleware::Go do end end - context 'with nonexistent path' do - let(:path) { 'nonexistent-group/nonexistent-project' } - - it 'responses not found' do - status_code, _headers, body = go - expect(status_code).to eq(404) - expect(body).to match_array(['']) - end - end - context 'with a nested project path' do let(:group) { create(:group, :nested) } let!(:project) { create(:project, :public, :repository, namespace: group) } @@ -78,10 +68,8 @@ RSpec.describe Gitlab::Middleware::Go do end shared_examples 'unauthorized' do - it 'returns unauthorized' do - status_code, _headers, body = go - expect(status_code).to eq(404) - expect(body).to match_array(['']) + it 'returns the 2-segment group path' do + expect_response_with_path(go, enabled_protocol, group.full_path, project.default_branch) end end @@ -153,7 +141,7 @@ RSpec.describe Gitlab::Middleware::Go do expect(Gitlab::Auth).to receive(:find_for_git_client).and_raise(Gitlab::Auth::IpBlacklisted) response = go - expect(response[0]).to eq(404) + expect(response[0]).to eq(403) expect(response[1]['Content-Length']).to be_nil expect(response[2]).to eq(['']) end @@ -170,7 +158,7 @@ RSpec.describe Gitlab::Middleware::Go do expect(Gitlab::Auth).to receive(:find_for_git_client).and_raise(Gitlab::Auth::MissingPersonalAccessTokenError) response = go - expect(response[0]).to eq(404) + expect(response[0]).to eq(401) expect(response[1]['Content-Length']).to be_nil expect(response[2]).to eq(['']) end diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index b16fa4df6c1..32518b867cb 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -5416,82 +5416,6 @@ RSpec.describe MergeRequest, factory_default: :keep do subject(:suggested_reviewer_users) { merge_request.suggested_reviewer_users } - shared_examples 'blank suggestions' do - it 'returns an empty relation' do - expect(suggested_reviewer_users).to be_empty - end - end - - context 'when predictions is nil' do - it_behaves_like 'blank suggestions' - end - - context 'when predictions is not nil' do - before do - merge_request.build_predictions - end - - context 'when predictions is a non hash' do - before do - merge_request.build_predictions - merge_request.predictions.suggested_reviewers = 1 - end - - it_behaves_like 'blank suggestions' - end - - context 'when predictions is an empty hash' do - before do - merge_request.predictions.suggested_reviewers = {} - end - - it_behaves_like 'blank suggestions' - end - - context 'when suggests a user who is not a member' do - let_it_be(:non_member) { create(:user) } - - before do - merge_request.predictions.suggested_reviewers = { 'reviewers' => [non_member.username] } - end - - it_behaves_like 'blank suggestions' - end - - context 'when suggests users who are members' do - let_it_be(:first_member) { create(:user) } - let_it_be(:second_member) { create(:user) } - - before_all do - project.add_developer(first_member) - project.add_developer(second_member) - end - - before do - merge_request.predictions.suggested_reviewers = { - 'reviewers' => [ - second_member.username, - first_member.username - ] - } - end - - context 'when a user is inactive' do - before do - second_member.deactivate - end - - it 'returns only active users' do - expect(suggested_reviewer_users).to eq([first_member]) - end - end - - context 'when all users are active' do - it 'returns users in correct suggested order' do - expect(suggested_reviewer_users).to eq([second_member, first_member]) - end - end - end - end + it { is_expected.to be_empty } end end |