diff --git a/app/controllers/projects_controller.rb b/app/controllers/projects_controller.rb index e8ae7b8db19..2c4cb55844c 100644 --- a/app/controllers/projects_controller.rb +++ b/app/controllers/projects_controller.rb @@ -616,13 +616,16 @@ class ProjectsController < Projects::ApplicationController def redirect_git_extension return unless params[:format] == 'git' + git_extension_regex = %r{\.git/?\Z} + return unless request.path.match?(git_extension_regex) + # `project` calls `find_routable!`, so this will trigger the usual not-found # behaviour when the user isn't authorized to see the project return if project.nil? || performed? uri = URI(request.original_url) # Strip the '.git' part from the path - uri.path = uri.path.sub(%r{\.git/?\Z}, '') + uri.path = uri.path.sub(git_extension_regex, '') redirect_to(uri.to_s) end diff --git a/app/models/board.rb b/app/models/board.rb index e407ffe582f..d98b43164fb 100644 --- a/app/models/board.rb +++ b/app/models/board.rb @@ -11,7 +11,7 @@ class Board < ApplicationRecord has_many :lists, -> { ordered }, dependent: :delete_all, inverse_of: :board # rubocop:disable Cop/ActiveRecordDependent has_many :destroyable_lists, -> { destroyable.ordered }, class_name: "List", inverse_of: :board - validates :name, presence: true + validates :name, presence: true, length: { maximum: 255, if: :name_changed? } validates :project, presence: true, if: :project_needed? validates :group, presence: true, unless: :project validates :group, absence: { diff --git a/app/models/concerns/web_hooks/hook.rb b/app/models/concerns/web_hooks/hook.rb index fae6efbd756..0a8df4c662c 100644 --- a/app/models/concerns/web_hooks/hook.rb +++ b/app/models/concerns/web_hooks/hook.rb @@ -7,6 +7,7 @@ module WebHooks InterpolationError = Class.new(StandardError) SECRET_MASK = '************' + MAX_PARAM_LENGTH = 8192 # See app/validators/json_schemas/web_hooks_url_variables.json VARIABLE_REFERENCE_RE = /\{([A-Za-z]+[0-9]*(?:[._-][A-Za-z0-9]+)*)\}/ @@ -45,9 +46,14 @@ module WebHooks encode_iv: false validates :url, presence: true - validates :url, public_url: true, if: ->(hook) { hook.validate_public_url? && !hook.url_variables? } + validates :url, length: { maximum: MAX_PARAM_LENGTH } + validates :url, public_url: true, if: ->(hook) { + # Apply the validation up to the point where the length validation above would make the record invalid. + # See https://gitlab.com/gitlab-org/gitlab/-/issues/524020#note_2529307579 + (hook.url&.length&.<= MAX_PARAM_LENGTH) && hook.validate_public_url? && !hook.url_variables? + } - validates :token, format: { without: /\n/ } + validates :token, length: { maximum: MAX_PARAM_LENGTH }, format: { without: /\n/ } after_initialize :initialize_url_variables after_initialize :initialize_custom_headers diff --git a/lib/gitlab/git_access.rb b/lib/gitlab/git_access.rb index e37d71e51c3..977deb56ccf 100644 --- a/lib/gitlab/git_access.rb +++ b/lib/gitlab/git_access.rb @@ -83,8 +83,7 @@ module Gitlab check_command_disabled! check_command_existence! - custom_action = check_custom_action - return custom_action if custom_action + check_custom_ssh_action! check_db_accessibility! check_container! @@ -185,7 +184,7 @@ module Gitlab check_project_accessibility! end - def check_custom_action + def check_custom_ssh_action! # no-op: Overridden in EE end diff --git a/lib/gitlab/git_access_snippet.rb b/lib/gitlab/git_access_snippet.rb index 8c291dd56ba..9eda2099eb7 100644 --- a/lib/gitlab/git_access_snippet.rb +++ b/lib/gitlab/git_access_snippet.rb @@ -46,8 +46,8 @@ module Gitlab private # TODO: Implement EE/Geo https://gitlab.com/gitlab-org/gitlab/issues/205629 - override :check_custom_action - def check_custom_action + override :check_custom_ssh_action! + def check_custom_ssh_action! # snippets never return custom actions, such as geo replication. end diff --git a/locale/gitlab.pot b/locale/gitlab.pot index e05a8518509..92a853527ff 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -47221,6 +47221,9 @@ msgstr "" msgid "Project %{code_open}%{source_project}%{code_close} has more restricted access settings than %{code_open}%{target_project}%{code_close}. To avoid exposing private changes, make sure you're submitting changes to the correct project." msgstr "" +msgid "Project %{project_name} and framework are not from same namespace." +msgstr "" + msgid "Project %{project_repo} could not be found" msgstr "" @@ -47413,6 +47416,9 @@ msgstr "" msgid "Project was not found or you do not have permission to add this project to Security Dashboards." msgstr "" +msgid "Project with ID %{project_id} not found" +msgstr "" + msgid "Project:Branches: %{source_project_path}:%{source_branch} to %{target_project_path}:%{target_branch}" msgstr "" diff --git a/spec/controllers/projects_controller_spec.rb b/spec/controllers/projects_controller_spec.rb index ceb09bbaa9f..e9f98453feb 100644 --- a/spec/controllers/projects_controller_spec.rb +++ b/spec/controllers/projects_controller_spec.rb @@ -459,6 +459,39 @@ RSpec.describe ProjectsController, feature_category: :groups_and_projects do end end + context 'redirection from http://someproject?format=git' do + let_it_be_with_reload(:public_project) { create(:project, :public) } + + context 'with git in project path' do + before do + public_project.update!(path: 'my.gitlab') + + request.env['PATH_INFO'] = "/#{public_project.namespace.path}/#{public_project.path}" + request.env['QUERY_STRING'] = 'format=git' + end + + it 'does not trigger a redirect' do + get :show, + params: { namespace_id: public_project.namespace.path, id: public_project.path }, + format: :git + + expect(response).to have_gitlab_http_status(:not_found) + expect(response).not_to redirect_to(project_path(public_project)) + end + end + + context 'with git not in project path' do + it 'does trigger a redirect' do + get :show, + params: { namespace_id: public_project.namespace.path, id: public_project.path }, + format: :git + + expect(response).to have_gitlab_http_status(:found) + expect(response).to redirect_to(project_path(public_project)) + end + end + end + context 'when project is moved and git format is requested' do let(:old_path) { project.path + 'old' } diff --git a/spec/models/board_spec.rb b/spec/models/board_spec.rb index ddf1170d908..c9eb372bf4b 100644 --- a/spec/models/board_spec.rb +++ b/spec/models/board_spec.rb @@ -18,9 +18,22 @@ RSpec.describe Board do end describe 'validations' do + let_it_be(:board) { build(:board, project: project) } + it { is_expected.to validate_presence_of(:name) } it { is_expected.to validate_presence_of(:project) } + it 'validates name length is at most 255 characters when name is changed' do + board.name = 'a' * 256 + expect(board).not_to be_valid + expect(board.errors[:name]).to include('is too long (maximum is 255 characters)') + end + + it 'allows names up to 255 characters' do + board.name = 'a' * 255 + expect(board).to be_valid + end + describe 'group and project mutually exclusive' do context 'when project is present' do subject { described_class.new(project: project) } diff --git a/spec/support/shared_examples/models/concerns/web_hooks/web_hook_shared_examples.rb b/spec/support/shared_examples/models/concerns/web_hooks/web_hook_shared_examples.rb index 80bc6703e2a..bbb68f36a8f 100644 --- a/spec/support/shared_examples/models/concerns/web_hooks/web_hook_shared_examples.rb +++ b/spec/support/shared_examples/models/concerns/web_hooks/web_hook_shared_examples.rb @@ -20,6 +20,8 @@ RSpec.shared_examples 'a webhook' do |factory:| it { is_expected.to validate_length_of(:custom_webhook_template).is_at_most(4096) } it { is_expected.to validate_length_of(:name).is_at_most(255) } it { is_expected.to validate_length_of(:description).is_at_most(2048) } + it { is_expected.to validate_length_of(:url).is_at_most(described_class::MAX_PARAM_LENGTH) } + it { is_expected.to validate_length_of(:token).is_at_most(described_class::MAX_PARAM_LENGTH) } describe 'url_variables' do it { is_expected.to allow_value({}).for(:url_variables) } @@ -110,6 +112,33 @@ RSpec.shared_examples 'a webhook' do |factory:| end end + context 'when testing PublicUrlValidator execution around MAX_PARAM_LENGTH threshold' do + # Ensures invalid URLs are rejected at all lengths around MAX_PARAM_LENGTH boundary. + # PublicUrlValidator runs only when length <= MAX_PARAM_LENGTH, so we verify + # the webhook stays invalid whether rejected by URL format or by length. + + let(:invalid_url_base) { 'ftp://example.com/' } + let(:padding_to_max_length) { described_class::MAX_PARAM_LENGTH - invalid_url_base.length } + + context 'with URL length 1 less than MAX_PARAM_LENGTH' do + let(:url) { invalid_url_base + ('x' * (padding_to_max_length - 1)) } + + it { is_expected.not_to allow_value(url).for(:url).with_message(including('allowed schemes are http')) } + end + + context 'with URL length exactly MAX_PARAM_LENGTH' do + let(:url) { invalid_url_base + ('x' * padding_to_max_length) } + + it { is_expected.not_to allow_value(url).for(:url).with_message(including('allowed schemes are http')) } + end + + context 'with URL length 1 more than MAX_PARAM_LENGTH' do + let(:url) { invalid_url_base + ('x' * (padding_to_max_length + 1)) } + + it { is_expected.not_to allow_value(url).for(:url).with_message(including('is too long')) } + end + end + it 'strips :url before saving it' do hook.url = ' https://example.com ' hook.save!