diff --git a/GITALY_SERVER_VERSION b/GITALY_SERVER_VERSION index 7d176d17e34..8462e19de92 100644 --- a/GITALY_SERVER_VERSION +++ b/GITALY_SERVER_VERSION @@ -1 +1 @@ -a26694e9a954f7ccca30ce08ab2cad00f1a185fa +189f3dd25520d70c2d35e65d2d1b575b1843cfe6 diff --git a/app/models/concerns/ci/partitionable/organizer.rb b/app/models/concerns/ci/partitionable/organizer.rb index 3a608a8381d..06ed8599743 100644 --- a/app/models/concerns/ci/partitionable/organizer.rb +++ b/app/models/concerns/ci/partitionable/organizer.rb @@ -11,7 +11,7 @@ module Ci def new_partition_required?(latest_partition_id) insert_first_partitions if Feature.enabled?(:ci_partitioning_first_records) - create_partitions_102? && Ci::Pipeline::NEXT_PARTITION_VALUE > latest_partition_id + Ci::Pipeline::NEXT_PARTITION_VALUE > latest_partition_id end private @@ -27,12 +27,6 @@ module Ci ) end strong_memoize_attr :insert_first_partitions - - # This method is evaluated before the stubs are set in place for the test environment - # so we need to return true to create the partitions. - def create_partitions_102? - ::Gitlab.dev_or_test_env? || Feature.enabled?(:ci_create_partitions_102, :instance) - end end end end diff --git a/app/models/import/source_user.rb b/app/models/import/source_user.rb index 49a80f17af1..d87886a9228 100644 --- a/app/models/import/source_user.rb +++ b/app/models/import/source_user.rb @@ -6,6 +6,7 @@ module Import belongs_to :placeholder_user, class_name: 'User', optional: true belongs_to :reassign_to_user, class_name: 'User', optional: true + belongs_to :reassigned_by_user, class_name: 'User', optional: true belongs_to :namespace validates :namespace_id, :import_type, :source_hostname, :source_user_identifier, :status, presence: true diff --git a/app/services/ci/partitions/create_service.rb b/app/services/ci/partitions/create_service.rb new file mode 100644 index 00000000000..4628813fa50 --- /dev/null +++ b/app/services/ci/partitions/create_service.rb @@ -0,0 +1,37 @@ +# frozen_string_literal: true + +module Ci + module Partitions + class CreateService + MAX_PARTITION_SIZE = 100.gigabytes + HEADROOM_PARTITIONS = 3 + + def initialize(partition) + @partition = partition + end + + def execute + return unless Feature.enabled?(:ci_partitioning_automation, :instance) + return unless partition + + Ci::Partition.create_next! if should_create_next? + end + + private + + attr_reader :partition + + def should_create_next? + above_threshold? && headroom_available? + end + + def above_threshold? + partition.above_threshold?(MAX_PARTITION_SIZE) + end + + def headroom_available? + Ci::Partition.id_after(partition.id).count < HEADROOM_PARTITIONS + end + end + end +end diff --git a/app/services/projects/protect_default_branch_service.rb b/app/services/projects/protect_default_branch_service.rb index f9273a74dbe..f265b0feb51 100644 --- a/app/services/projects/protect_default_branch_service.rb +++ b/app/services/projects/protect_default_branch_service.rb @@ -34,7 +34,9 @@ module Projects params = { name: default_branch, push_access_levels_attributes: [{ access_level: push_access_level }], - merge_access_levels_attributes: [{ access_level: merge_access_level }] + merge_access_levels_attributes: [{ access_level: merge_access_level }], + code_owner_approval_required: code_owner_approval_required?, + allow_force_push: allow_force_push? } # The creator of the project is always allowed to create protected @@ -44,6 +46,17 @@ module Projects .execute(skip_authorization: true) end + # overriden in EE + def code_owner_approval_required? + false + end + + def allow_force_push? + return false unless Feature.enabled?(:default_branch_protection_defaults, project) + + default_branch_protection.allow_force_push? + end + def protect_branch? default_branch_protection.any? && !ProtectedBranch.protected?(project, default_branch) diff --git a/app/views/admin/application_settings/_visibility_and_access.html.haml b/app/views/admin/application_settings/_visibility_and_access.html.haml index 624f5a48c3a..8d0b8c17bd2 100644 --- a/app/views/admin/application_settings/_visibility_and_access.html.haml +++ b/app/views/admin/application_settings/_visibility_and_access.html.haml @@ -17,7 +17,7 @@ %fieldset.form-group.gl-form-group{ data: { testid: 'restricted-visibility-levels' } } %legend.col-form-label = s_('AdminSettings|Restricted visibility levels') - %small.form-text.text-gl-muted + %small.form-text.gl-text-secondary = s_('AdminSettings|Prevent non-administrators from using the selected visibility levels for groups, projects and snippets.') = s_('AdminSettings|The selected level must be different from the selected default group and project visibility.') = link_to _('Learn more.'), help_page_path('administration/settings/visibility_and_access_controls', anchor: 'restrict-visibility-levels'), target: '_blank', rel: 'noopener noreferrer' diff --git a/app/views/groups/_group_admin_settings.html.haml b/app/views/groups/_group_admin_settings.html.haml index 33cbec9be61..6c0c7e02657 100644 --- a/app/views/groups/_group_admin_settings.html.haml +++ b/app/views/groups/_group_admin_settings.html.haml @@ -26,7 +26,7 @@ .form-group.gl-form-group{ role: 'group' } = f.label :two_factor_grace_period, _('Two-factor authentication grace period'), class: 'gl-display-block col-form-label' = f.text_field :two_factor_grace_period, class: 'form-control gl-form-input gl-form-input-sm' - %small.form-text.text-gl-muted + %small.form-text.gl-text-secondary = _("Time (in hours) that users are allowed to skip forced configuration of two-factor authentication.") - if @group.namespace_settings.present? diff --git a/app/views/groups/_import_group_from_another_instance_panel.html.haml b/app/views/groups/_import_group_from_another_instance_panel.html.haml index 3ce9caf27f9..2e5bceaf8dd 100644 --- a/app/views/groups/_import_group_from_another_instance_panel.html.haml +++ b/app/views/groups/_import_group_from_another_instance_panel.html.haml @@ -37,7 +37,7 @@ title: s_('GroupsNew|Enter the URL for the source instance.'), id: 'import_gitlab_url', data: { testid: 'import-gitlab-url' } - %small.form-text.text-gl-muted + %small.form-text.gl-text-secondary = s_('Import|Must only contain the base URL of the source GitLab instance.') .form-group.gl-form-group.gl-display-flex.gl-flex-direction-column = f.label :bulk_import_gitlab_access_token, s_('GroupsNew|Personal access token'), for: 'import_gitlab_token', class: 'col-form-label' diff --git a/app/views/projects/_new_project_fields.html.haml b/app/views/projects/_new_project_fields.html.haml index 302e7e9bcd5..78b0454c397 100644 --- a/app/views/projects/_new_project_fields.html.haml +++ b/app/views/projects/_new_project_fields.html.haml @@ -11,7 +11,7 @@ = f.label :name, class: 'label-bold' do %span= _("Project name") = f.text_field :name, placeholder: "My awesome project", class: "form-control gl-form-input input-lg", data: { testid: 'project-name', track_label: "#{track_label}", track_action: "activate_form_input", track_property: "project_name", track_value: "" }, required: true, aria: { required: true } - %small#js-project-name-description.form-text.text-gl-muted + %small#js-project-name-description.form-text.gl-text-secondary = s_("ProjectsNew|Must start with a lowercase or uppercase letter, digit, emoji, or underscore. Can also contain dots, pluses, dashes, or spaces.") #js-project-name-error.gl-field-error.gl-mt-2.gl-display-none .form-group.gl-form-group.gl-w-full.gl-display-flex.gl-flex-wrap diff --git a/app/views/user_settings/profiles/_email_settings.html.haml b/app/views/user_settings/profiles/_email_settings.html.haml index 6c50be5b702..c11a03a2728 100644 --- a/app/views/user_settings/profiles/_email_settings.html.haml +++ b/app/views/user_settings/profiles/_email_settings.html.haml @@ -8,7 +8,7 @@ .form-group.gl-form-group = form.label :email, _('Email') = form.text_field :email, required: true, class: 'gl-form-input form-control gl-md-form-input-lg', value: (@user.email unless @user.temp_oauth_email?), readonly: readonly || email_change_disabled - %small.form-text.text-gl-muted + %small.form-text.gl-text-secondary = help_text.html_safe - unless password_automatically_set @@ -21,7 +21,7 @@ options_for_select(@user.public_verified_emails, selected: @user.public_email), { include_blank: s_("Profiles|Do not show on profile") }, { class: 'gl-form-select custom-select', disabled: email_change_disabled } - %small.form-text.text-gl-muted + %small.form-text.gl-text-secondary = s_("Profiles|This email will be displayed on your public profile.") .form-group.gl-form-group @@ -34,5 +34,5 @@ options_for_select(commit_email_select_options(@user), selected: @user.commit_email), {}, { class: 'gl-form-select custom-select', disabled: email_change_disabled } - %small.form-text.text-gl-muted + %small.form-text.gl-text-secondary = commit_email_docs_link diff --git a/app/views/user_settings/profiles/_name.html.haml b/app/views/user_settings/profiles/_name.html.haml index 76d370e6ec9..c7389427b6d 100644 --- a/app/views/user_settings/profiles/_name.html.haml +++ b/app/views/user_settings/profiles/_name.html.haml @@ -1,10 +1,10 @@ = form.label :name, s_('Profiles|Full name') - if user.read_only_attribute?(:name) = form.text_field :name, class: 'gl-form-input form-control', required: true, readonly: true - %small.form-text.text-gl-muted + %small.form-text.gl-text-secondary = s_("Profiles|Your name was automatically set based on your %{provider_label} account, so people you know can recognize you.") % { provider_label: attribute_provider_label(:name) } - else = form.text_field :name, class: 'gl-form-input form-control', required: true, title: s_("Profiles|Using emoji in names seems fun, but please try to set a status message instead") - %small.form-text.text-gl-muted + %small.form-text.gl-text-secondary = s_("Profiles|Enter your name, so people you know can recognize you.") = safe_format(_("Profiles|No \"<\" or \">\" characters, please.")) diff --git a/app/views/user_settings/profiles/show.html.haml b/app/views/user_settings/profiles/show.html.haml index 66977d814b9..d477f6277aa 100644 --- a/app/views/user_settings/profiles/show.html.haml +++ b/app/views/user_settings/profiles/show.html.haml @@ -87,12 +87,12 @@ .form-group.gl-form-group = f.label :pronouns, s_('Profiles|Pronouns') = f.text_field :pronouns, class: 'gl-form-input form-control gl-md-form-input-lg' - %small.form-text.text-gl-muted + %small.form-text.gl-text-secondary = s_("Profiles|Enter your pronouns to let people know how to refer to you.") .form-group.gl-form-group = f.label :pronunciation, s_('Profiles|Pronunciation') = f.text_field :pronunciation, class: 'gl-form-input form-control gl-md-form-input-lg' - %small.form-text.text-gl-muted + %small.form-text.gl-text-secondary = s_("Profiles|Enter how your name is pronounced to help people address you correctly.") = render_if_exists 'profiles/extra_settings', form: f = render_if_exists 'user_settings/profiles/email_settings', form: f @@ -104,7 +104,7 @@ = f.text_field :linkedin, class: 'gl-form-input form-control gl-md-form-input-lg', placeholder: "profilename" - %small.form-text.text-gl-muted + %small.form-text.gl-text-secondary = s_("Profiles|Your LinkedIn profile name from linkedin.com/in/profilename") .form-group.gl-form-group = f.label :twitter, _('X (formerly Twitter)') @@ -124,7 +124,7 @@ max_length: max_discord_length, max_length_message: s_('Profiles|Discord ID is too long (maximum is %{max_length} characters).') % { max_length: max_discord_length }, allow_empty: true} - %small.form-text.text-gl-muted + %small.form-text.gl-text-secondary = external_accounts_docs_link .form-group.gl-form-group = f.label :mastodon @@ -137,7 +137,7 @@ = f.label :location, s_('Profiles|Location') - if @user.read_only_attribute?(:location) = f.text_field :location, class: 'gl-form-input form-control gl-md-form-input-lg', readonly: true - %small.form-text.text-gl-muted + %small.form-text.gl-text-secondary = s_("Profiles|Your location was automatically set based on your %{provider_label} account") % { provider_label: attribute_provider_label(:location) } - else = f.text_field :location, class: 'gl-form-input form-control gl-md-form-input-lg', placeholder: s_("Profiles|City, country") @@ -147,12 +147,12 @@ .form-group.gl-form-group = f.label :organization, s_('Profiles|Organization') = f.text_field :organization, class: 'gl-form-input form-control gl-md-form-input-lg' - %small.form-text.text-gl-muted + %small.form-text.gl-text-secondary = s_("Profiles|Who you represent or work for.") .form-group.gl-form-group.gl-mb-6.gl-max-w-80 = f.label :bio, s_('Profiles|Bio') = f.text_area :bio, class: 'gl-form-input gl-form-textarea form-control', rows: 4, maxlength: 250 - %small.form-text.text-gl-muted + %small.form-text.gl-text-secondary = s_("Profiles|Tell us about yourself in fewer than 250 characters.") .gl-border-t.gl-pt-6 %fieldset.form-group.gl-form-group diff --git a/config/feature_flags/gitlab_com_derisk/ci_create_partitions_102.yml b/config/feature_flags/gitlab_com_derisk/ci_partitioning_automation.yml similarity index 50% rename from config/feature_flags/gitlab_com_derisk/ci_create_partitions_102.yml rename to config/feature_flags/gitlab_com_derisk/ci_partitioning_automation.yml index 9383a806081..bcfa1e90bc8 100644 --- a/config/feature_flags/gitlab_com_derisk/ci_create_partitions_102.yml +++ b/config/feature_flags/gitlab_com_derisk/ci_partitioning_automation.yml @@ -1,9 +1,9 @@ --- -name: ci_create_partitions_102 -feature_issue_url: -introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/148376 -rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/454519 -milestone: '16.11' -group: group::pipeline execution +name: ci_partitioning_automation +feature_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/454978 +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/152047 +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/457517 +milestone: '17.0' +group: group::ci platform type: gitlab_com_derisk default_enabled: false diff --git a/db/migrate/20240502131137_add_reassigned_by_to_import_source_user.rb b/db/migrate/20240502131137_add_reassigned_by_to_import_source_user.rb new file mode 100644 index 00000000000..9b3c2e76db9 --- /dev/null +++ b/db/migrate/20240502131137_add_reassigned_by_to_import_source_user.rb @@ -0,0 +1,21 @@ +# frozen_string_literal: true + +class AddReassignedByToImportSourceUser < Gitlab::Database::Migration[2.2] + disable_ddl_transaction! + + milestone '17.0' + + TABLE_NAME = :import_source_users + COLUMN_NAME = :reassigned_by_user_id + + def up + add_column TABLE_NAME, COLUMN_NAME, :bigint, if_not_exists: true + + add_concurrent_index TABLE_NAME, COLUMN_NAME, name: "index_#{TABLE_NAME}_on_#{COLUMN_NAME}" + add_concurrent_foreign_key TABLE_NAME, :users, column: COLUMN_NAME, on_delete: :nullify + end + + def down + remove_column TABLE_NAME, COLUMN_NAME + end +end diff --git a/db/schema_migrations/20240502131137 b/db/schema_migrations/20240502131137 new file mode 100644 index 00000000000..ce081ccfcb9 --- /dev/null +++ b/db/schema_migrations/20240502131137 @@ -0,0 +1 @@ +70f0dce772dde6cfe21d25b78456fab6d9b5fc6714be6e906b1407a7cb19779c \ No newline at end of file diff --git a/db/structure.sql b/db/structure.sql index 43961ca0c73..e85a37b75e2 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -9861,6 +9861,7 @@ CREATE TABLE import_source_users ( source_user_identifier text NOT NULL, source_hostname text NOT NULL, import_type text NOT NULL, + reassigned_by_user_id bigint, CONSTRAINT check_0d7295a307 CHECK ((char_length(import_type) <= 255)), CONSTRAINT check_199c28ec54 CHECK ((char_length(source_username) <= 255)), CONSTRAINT check_562655155f CHECK ((char_length(source_name) <= 255)), @@ -25705,6 +25706,8 @@ CREATE INDEX index_import_source_users_on_placeholder_user_id ON import_source_u CREATE INDEX index_import_source_users_on_reassign_to_user_id ON import_source_users USING btree (reassign_to_user_id); +CREATE INDEX index_import_source_users_on_reassigned_by_user_id ON import_source_users USING btree (reassigned_by_user_id); + CREATE INDEX index_imported_projects_on_import_type_creator_id_created_at ON projects USING btree (import_type, creator_id, created_at) WHERE (import_type IS NOT NULL); CREATE INDEX index_imported_projects_on_import_type_id ON projects USING btree (import_type, id) WHERE (import_type IS NOT NULL); @@ -30288,6 +30291,9 @@ ALTER TABLE ONLY deploy_tokens ALTER TABLE ONLY protected_branch_push_access_levels ADD CONSTRAINT fk_7111b68cdb FOREIGN KEY (group_id) REFERENCES namespaces(id) ON DELETE CASCADE; +ALTER TABLE ONLY import_source_users + ADD CONSTRAINT fk_719b74231d FOREIGN KEY (reassigned_by_user_id) REFERENCES users(id) ON DELETE SET NULL; + ALTER TABLE ONLY integrations ADD CONSTRAINT fk_71cce407f9 FOREIGN KEY (project_id) REFERENCES projects(id) ON DELETE CASCADE; diff --git a/doc/administration/backup_restore/troubleshooting_backup_gitlab.md b/doc/administration/backup_restore/troubleshooting_backup_gitlab.md index 3add449d9ed..c5da475317c 100644 --- a/doc/administration/backup_restore/troubleshooting_backup_gitlab.md +++ b/doc/administration/backup_restore/troubleshooting_backup_gitlab.md @@ -25,6 +25,7 @@ decrypt those columns, preventing access to the following items: - [Project mirroring](../../user/project/repository/mirror/index.md) - [Integrations](../../user/project/integrations/index.md) - [Web hooks](../../user/project/integrations/webhooks.md) +- [Deploy tokens](../../user/project/deploy_tokens/index.md) In cases like CI/CD variables and runner authentication, you can experience unexpected behaviors, such as: diff --git a/doc/administration/pages/index.md b/doc/administration/pages/index.md index 6ea197d4358..d85682f3580 100644 --- a/doc/administration/pages/index.md +++ b/doc/administration/pages/index.md @@ -395,7 +395,7 @@ control over how the Pages daemon runs and serves content in your environment. | `artifacts_server` | Enable viewing [artifacts](../job_artifacts.md) in GitLab Pages. | | `artifacts_server_timeout` | Timeout (in seconds) for a proxied request to the artifacts server. | | `artifacts_server_url` | API URL to proxy artifact requests to. Defaults to GitLab `external URL` + `/api/v4`, for example `https://gitlab.com/api/v4`. When running a [separate Pages server](#running-gitlab-pages-on-a-separate-server), this URL must point to the main GitLab server's API. | -| `auth_redirect_uri` | Callback URL for authenticating with GitLab. Defaults to project's subdomain of `pages_external_url` + `/auth`. | +| `auth_redirect_uri` | Callback URL for authenticating with GitLab. Defaults to project's subdomain of `pages_external_url` + `/auth`, for example `https://projects.example.io/auth`. When `namespace_in_path` is enabled, defaults to `pages_external_url` + `/projects/auth`, for example `https://example.io/projects/auth`. | | `auth_secret` | Secret key for signing authentication requests. Leave blank to pull automatically from GitLab during OAuth registration. | | `dir` | Working directory for configuration and secrets files. | | `enable` | Enable or disable GitLab Pages on the current system. | @@ -414,7 +414,7 @@ control over how the Pages daemon runs and serves content in your environment. | `gitlab_id` | The OAuth application public ID. Leave blank to automatically fill when Pages authenticates with GitLab. | | `gitlab_secret` | The OAuth application secret. Leave blank to automatically fill when Pages authenticates with GitLab. | | `auth_scope` | The OAuth application scope to use for authentication. Must match GitLab Pages OAuth application settings. Leave blank to use `api` scope by default. | -| `auth_timeout` | GitLab application client timeout for authentication in seconds (default: `5s`). A value of `0` means no timeout. | +| `auth_timeout` | GitLab application client timeout for authentication in seconds (default: `5s`). A value of `0` means no timeout. | | `auth_cookie_session_timeout` | Authentication cookie session timeout in seconds (default: `10m`). A value of `0` means the cookie is deleted after the browser session ends. | | `gitlab_server` | Server to use for authentication when access control is enabled; defaults to GitLab `external_url`. | | `headers` | Specify any additional http headers that should be sent to the client with each response. Multiple headers can be given as an array, header and value as one string, for example `['my-header: myvalue', 'my-other-header: my-other-value']` | @@ -425,10 +425,10 @@ control over how the Pages daemon runs and serves content in your environment. | `log_directory` | Absolute path to a log directory. | | `log_format` | The log output format: `text` or `json`. | | `log_verbose` | Verbose logging, true/false. | -| `namespace_in_path` | (Beta) Enable or disable namespace in the URL path. This requires `pages_nginx[enable] = true`. Sets `rewrite` configuration in NGINX to support [without wildcard DNS setup](#for-namespace-in-url-path-without-wildcard-dns). Default: `false` | +| `namespace_in_path` | (Beta) Enable or disable namespace in the URL path. This requires `pages_nginx[enable] = true`. Sets `rewrite` configuration in NGINX to support [without wildcard DNS setup](#for-namespace-in-url-path-without-wildcard-dns). Default: `false`. | | `propagate_correlation_id` | Set to true (false by default) to re-use existing Correlation ID from the incoming request header `X-Request-ID` if present. If a reverse proxy sets this header, the value is propagated in the request chain. | | `max_connections` | Limit on the number of concurrent connections to the HTTP, HTTPS or proxy listeners. | -| `max_uri_length` | The maximum length of URIs accepted by GitLab Pages. Set to 0 for unlimited length. | +| `max_uri_length` | The maximum length of URIs accepted by GitLab Pages. Set to 0 for unlimited length. | | `metrics_address` | The address to listen on for metrics requests. | | `redirect_http` | Redirect pages from HTTP to HTTPS, true/false. | | `redirects_max_config_size` | The maximum size of the `_redirects` file, in bytes (default: 65536). | diff --git a/doc/ci/runners/hosted_runners/linux.md b/doc/ci/runners/hosted_runners/linux.md index cf377ed3db6..20556aedeea 100644 --- a/doc/ci/runners/hosted_runners/linux.md +++ b/doc/ci/runners/hosted_runners/linux.md @@ -24,7 +24,7 @@ GitLab offers the following machine types for hosted runners on Linux x86-64. | Runner Tag | vCPUs | Memory | Storage | |--------------------------------------------------------|-------|--------|---------| -| `saas-linux-small-amd64` (default) | 2 | 8 GB | 25 GB | +| `saas-linux-small-amd64` (default) | 2 | 8 GB | 30 GB | | `saas-linux-medium-amd64` | 4 | 16 GB | 50 GB | | `saas-linux-large-amd64` (Premium and Ultimate only) | 8 | 32 GB | 100 GB | | `saas-linux-xlarge-amd64` (Premium and Ultimate only) | 16 | 64 GB | 200 GB | diff --git a/doc/user/project/repository/code_suggestions/supported_extensions.md b/doc/user/project/repository/code_suggestions/supported_extensions.md index 33ed3392888..af67e5b29fc 100644 --- a/doc/user/project/repository/code_suggestions/supported_extensions.md +++ b/doc/user/project/repository/code_suggestions/supported_extensions.md @@ -49,6 +49,7 @@ The following languages are supported: | Java | **{check-circle}** Yes | **{check-circle}** Yes | **{check-circle}** Yes | **{check-circle}** Yes | | JavaScript | **{check-circle}** Yes | **{check-circle}** Yes | **{check-circle}** Yes | **{check-circle}** Yes | | Kotlin | **{check-circle}** Yes

(Requires third-party extension providing Kotlin support) | **{check-circle}** Yes | **{check-circle}** Yes | **{check-circle}** Yes | +| Markdown | **{check-circle}** Yes | **{check-circle}** Yes | **{dotted-circle}** No | **{check-circle}** Yes | | PHP | **{check-circle}** Yes | **{check-circle}** Yes | **{check-circle}** Yes | **{check-circle}** Yes | | Python | **{check-circle}** Yes | **{check-circle}** Yes | **{check-circle}** Yes | **{check-circle}** Yes | | Ruby | **{check-circle}** Yes | **{check-circle}** Yes | **{check-circle}** Yes | **{check-circle}** Yes | diff --git a/lib/api/helpers/groups_helpers.rb b/lib/api/helpers/groups_helpers.rb index ab7df044c73..e7712a2e20c 100644 --- a/lib/api/helpers/groups_helpers.rb +++ b/lib/api/helpers/groups_helpers.rb @@ -34,6 +34,7 @@ module API optional :allowed_to_merge, type: Array, desc: 'An array of access levels allowed to merge' do requires :access_level, type: Integer, values: [::Gitlab::Access::DEVELOPER, ::Gitlab::Access::MAINTAINER], desc: 'A valid access level' end + optional :code_owner_approval_required, type: Boolean, desc: "Require approval from code owners" optional :developer_can_initial_push, type: Boolean, desc: 'Allow developers to initial push' end optional :shared_runners_setting, type: String, values: ::Namespace::SHARED_RUNNERS_SETTINGS, desc: 'Enable/disable shared runners for the group and its subgroups and projects' diff --git a/lib/api/settings.rb b/lib/api/settings.rb index 0cd694f8c46..31f5cfcc0d2 100644 --- a/lib/api/settings.rb +++ b/lib/api/settings.rb @@ -58,6 +58,7 @@ module API optional :allowed_to_merge, type: Array, desc: 'An array of access levels allowed to merge' do requires :access_level, type: Integer, values: [::Gitlab::Access::DEVELOPER, ::Gitlab::Access::MAINTAINER], desc: 'A valid access level' end + optional :code_owner_approval_required, type: Boolean, desc: "Require approval from code owners" optional :developer_can_initial_push, type: Boolean, desc: 'Allow developers to initial push' end optional :default_group_visibility, type: String, values: Gitlab::VisibilityLevel.string_values, desc: 'The default group visibility' diff --git a/lib/gitlab/access/default_branch_protection.rb b/lib/gitlab/access/default_branch_protection.rb index 6648f078341..2a2b6d8e84d 100644 --- a/lib/gitlab/access/default_branch_protection.rb +++ b/lib/gitlab/access/default_branch_protection.rb @@ -9,6 +9,14 @@ module Gitlab @settings = settings.deep_symbolize_keys end + def code_owner_approval_required? + !!settings[:code_owner_approval_required] + end + + def allow_force_push? + !!settings[:allow_force_push] + end + def any? return true unless settings[:allow_force_push] diff --git a/spec/factories/import_source_users.rb b/spec/factories/import_source_users.rb index f0ed58d8489..3ffa3cc97a4 100644 --- a/spec/factories/import_source_users.rb +++ b/spec/factories/import_source_users.rb @@ -14,5 +14,9 @@ FactoryBot.define do trait :with_reassign_to_user do reassign_to_user factory: :user end + + trait :with_reassigned_by_user do + reassigned_by_user factory: :user + end end end diff --git a/spec/lib/gitlab/access/default_branch_protection_spec.rb b/spec/lib/gitlab/access/default_branch_protection_spec.rb index 12770d99296..714950b227d 100644 --- a/spec/lib/gitlab/access/default_branch_protection_spec.rb +++ b/spec/lib/gitlab/access/default_branch_protection_spec.rb @@ -19,6 +19,30 @@ RSpec.describe Gitlab::Access::DefaultBranchProtection, feature_category: :sourc end end + describe '#code_owner_approval_required?' do + where(:setting, :result) do + { code_owner_approval_required: true } | true + { code_owner_approval_required: false } | false + { code_owner_approval_required: nil } | false + end + + with_them do + it { expect(described_class.new(setting).code_owner_approval_required?).to eq(result) } + end + end + + describe '#allow_force_push?' do + where(:setting, :result) do + { allow_force_push: true } | true + { allow_force_push: false } | false + { allow_force_push: nil } | false + end + + with_them do + it { expect(described_class.new(setting).allow_force_push?).to eq(result) } + end + end + describe '#developer_can_push?' do it 'when developer can push' do expect( diff --git a/spec/requests/api/groups_spec.rb b/spec/requests/api/groups_spec.rb index 431f8717497..72905d5e001 100644 --- a/spec/requests/api/groups_spec.rb +++ b/spec/requests/api/groups_spec.rb @@ -984,6 +984,7 @@ RSpec.describe API::Groups, feature_category: :groups_and_projects do project_creation_level: "noone", subgroup_creation_level: "maintainer", default_branch_protection: ::Gitlab::Access::MAINTAINER_PROJECT_ACCESS, + default_branch_protection_defaults: ::Gitlab::Access::BranchProtection.protected_after_initial_push.stringify_keys, prevent_sharing_groups_outside_hierarchy: true, avatar: fixture_file_upload(file_path), math_rendering_limits_enabled: false, @@ -1012,6 +1013,7 @@ RSpec.describe API::Groups, feature_category: :groups_and_projects do expect(json_response['shared_projects']).to be_an Array expect(json_response['shared_projects'].length).to eq(0) expect(json_response['default_branch_protection']).to eq(::Gitlab::Access::MAINTAINER_PROJECT_ACCESS) + expect(json_response['default_branch_protection_defaults']).to eq(::Gitlab::Access::BranchProtection.protected_after_initial_push.stringify_keys) expect(json_response['avatar_url']).to end_with('dk.png') expect(json_response['math_rendering_limits_enabled']).to eq(false) expect(json_response['lock_math_rendering_limits_enabled']).to eq(true) diff --git a/spec/requests/api/settings_spec.rb b/spec/requests/api/settings_spec.rb index ea8907766e0..9f1966b3ea6 100644 --- a/spec/requests/api/settings_spec.rb +++ b/spec/requests/api/settings_spec.rb @@ -173,7 +173,7 @@ RSpec.describe API::Settings, 'Settings', :do_not_mock_admin_mode_setting, featu diff_max_files: 2000, diff_max_lines: 50000, default_branch_protection: ::Gitlab::Access::PROTECTION_DEV_CAN_MERGE, - default_branch_protection_defaults: ::Gitlab::Access::BranchProtection.protected_against_developer_pushes.stringify_keys, + default_branch_protection_defaults: ::Gitlab::Access::BranchProtection.protected_after_initial_push.stringify_keys, local_markdown_version: 3, allow_local_requests_from_web_hooks_and_services: true, allow_local_requests_from_system_hooks: false, @@ -262,7 +262,7 @@ RSpec.describe API::Settings, 'Settings', :do_not_mock_admin_mode_setting, featu expect(json_response['diff_max_files']).to eq(2000) expect(json_response['diff_max_lines']).to eq(50000) expect(json_response['default_branch_protection']).to eq(Gitlab::Access::PROTECTION_DEV_CAN_MERGE) - expect(json_response['default_branch_protection_defaults']).to eq(::Gitlab::Access::BranchProtection.protected_against_developer_pushes.stringify_keys) + expect(json_response['default_branch_protection_defaults']).to eq(::Gitlab::Access::BranchProtection.protected_after_initial_push.stringify_keys) expect(json_response['local_markdown_version']).to eq(3) expect(json_response['allow_local_requests_from_web_hooks_and_services']).to eq(true) expect(json_response['allow_local_requests_from_system_hooks']).to eq(false) diff --git a/spec/services/ci/partitions/create_service_spec.rb b/spec/services/ci/partitions/create_service_spec.rb new file mode 100644 index 00000000000..a6630d50e42 --- /dev/null +++ b/spec/services/ci/partitions/create_service_spec.rb @@ -0,0 +1,63 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Ci::Partitions::CreateService, feature_category: :continuous_integration do + let_it_be(:ci_partition) { create(:ci_partition, :current) } + let(:service) { described_class.new(ci_partition) } + + describe '.execute' do + subject(:execute_service) { service.execute } + + shared_examples 'ci_partition not created' do + it 'does not create the next ci_partition', :aggregate_failures do + expect(Ci::Partition).not_to receive(:create_next!) + + expect { execute_service }.not_to change { Ci::Partition.count } + end + end + + context 'when ci_partitioning_automation is disabled' do + before do + stub_feature_flags(ci_partitioning_automation: false) + end + + it_behaves_like 'ci_partition not created' + end + + context 'when ci_partition is nil' do + let(:ci_partition) { nil } + + it_behaves_like 'ci_partition not created' + end + + context 'when all conditions are satistied' do + before do + stub_const("#{described_class}::MAX_PARTITION_SIZE", 1.byte) + end + + it 'creates the next ci_partition' do + expect { execute_service }.to change { Ci::Partition.count }.by(1) + end + end + + context 'when database_partition sizes are not above the threshold' do + it_behaves_like 'ci_partition not created' + end + + context 'when database_partition sizes are above the threshold' do + before do + stub_const("#{described_class}::MAX_PARTITION_SIZE", 1.byte) + end + + context 'when no more headroom available' do + before do + stub_const("#{described_class}::HEADROOM_PARTITIONS", 1) + create(:ci_partition) + end + + it_behaves_like 'ci_partition not created' + end + end + end +end diff --git a/spec/services/projects/protect_default_branch_service_spec.rb b/spec/services/projects/protect_default_branch_service_spec.rb index 4615d89ad3b..ed9d2ede386 100644 --- a/spec/services/projects/protect_default_branch_service_spec.rb +++ b/spec/services/projects/protect_default_branch_service_spec.rb @@ -161,7 +161,9 @@ RSpec.describe Projects::ProtectDefaultBranchService, feature_category: :source_ params = { name: 'master', push_access_levels_attributes: [{ access_level: access_level }], - merge_access_levels_attributes: [{ access_level: access_level }] + merge_access_levels_attributes: [{ access_level: access_level }], + code_owner_approval_required: false, + allow_force_push: false } allow(project) @@ -181,6 +183,14 @@ RSpec.describe Projects::ProtectDefaultBranchService, feature_category: :source_ .to receive(:merge_access_level) .and_return(access_level) + allow(service) + .to receive(:code_owner_approval_required?) + .and_return(false) + + allow(service) + .to receive(:allow_force_push?) + .and_return(false) + allow(service) .to receive(:default_branch) .and_return('master') @@ -325,6 +335,12 @@ RSpec.describe Projects::ProtectDefaultBranchService, feature_category: :source_ end end end + + describe '#allow_force_push?' do + it 'is falsey' do + expect(service.allow_force_push?).to be_falsey + end + end end context 'when feature flag `default_branch_protection_defaults` is enabled' do @@ -482,7 +498,9 @@ RSpec.describe Projects::ProtectDefaultBranchService, feature_category: :source_ params = { name: 'master', push_access_levels_attributes: [{ access_level: access_level }], - merge_access_levels_attributes: [{ access_level: access_level }] + merge_access_levels_attributes: [{ access_level: access_level }], + code_owner_approval_required: false, + allow_force_push: false } allow(project) @@ -506,6 +524,14 @@ RSpec.describe Projects::ProtectDefaultBranchService, feature_category: :source_ .to receive(:default_branch) .and_return('master') + allow(service) + .to receive(:code_owner_approval_required?) + .and_return(false) + + allow(service) + .to receive(:allow_force_push?) + .and_return(false) + allow(create_service) .to receive(:execute) .with(skip_authorization: true) @@ -646,5 +672,28 @@ RSpec.describe Projects::ProtectDefaultBranchService, feature_category: :source_ end end end + + describe '#allow_force_push?' do + before do + allow(project.namespace) + .to receive(:default_branch_protection_settings) + .and_return(Gitlab::Access::BranchProtection.protected_against_developer_pushes) + end + + it 'calls allow_force_push? method of Gitlab::Access::DefaultBranchProtection and returns correct value', + :aggregate_failures do + expect_next_instance_of(Gitlab::Access::DefaultBranchProtection) do |instance| + expect(instance).to receive(:allow_force_push?) + end + + expect(service.allow_force_push?).to be_falsey + end + end + end + + describe '#code_owner_approval_required?' do + it 'is falsey' do + expect(service.code_owner_approval_required?).to be_falsey + end end end diff --git a/workhorse/internal/dependencyproxy/dependencyproxy.go b/workhorse/internal/dependencyproxy/dependencyproxy.go index a28d108b431..a53d21262de 100644 --- a/workhorse/internal/dependencyproxy/dependencyproxy.go +++ b/workhorse/internal/dependencyproxy/dependencyproxy.go @@ -1,3 +1,4 @@ +// Package dependencyproxy provides functionality for handling dependency proxy operations package dependencyproxy import ( @@ -24,13 +25,14 @@ var httpClient = &http.Client{ Transport: httpTransport, } +// Injector provides functionality for injecting dependencies type Injector struct { senddata.Prefix uploadHandler http.Handler } type entryParams struct { - Url string + URL string Headers http.Header UploadConfig uploadConfig } @@ -38,7 +40,7 @@ type entryParams struct { type uploadConfig struct { Headers http.Header Method string - Url string + URL string } type nullResponseWriter struct { @@ -60,14 +62,17 @@ func (w *nullResponseWriter) WriteHeader(status int) { } } +// NewInjector creates a new instance of Injector func NewInjector() *Injector { return &Injector{Prefix: "send-dependency:"} } +// SetUploadHandler sets the upload handler for the Injector func (p *Injector) SetUploadHandler(uploadHandler http.Handler) { p.uploadHandler = uploadHandler } +// Inject performs the injection of dependencies func (p *Injector) Inject(w http.ResponseWriter, r *http.Request, sendData string) { params, err := p.unpackParams(sendData) if err != nil { @@ -75,7 +80,7 @@ func (p *Injector) Inject(w http.ResponseWriter, r *http.Request, sendData strin return } - dependencyResponse, err := p.fetchUrl(r.Context(), params) + dependencyResponse, err := p.fetchURL(r.Context(), params) if err != nil { status := http.StatusBadGateway @@ -86,10 +91,12 @@ func (p *Injector) Inject(w http.ResponseWriter, r *http.Request, sendData strin fail.Request(w, r, err, fail.WithStatus(status)) return } - defer dependencyResponse.Body.Close() + defer func() { _ = dependencyResponse.Body.Close() }() if dependencyResponse.StatusCode >= 400 { w.WriteHeader(dependencyResponse.StatusCode) - io.Copy(w, dependencyResponse.Body) + // We swallow errors for now as we need to investigate further, see + // https://gitlab.com/gitlab-org/gitlab/-/issues/459952. + _, _ = io.Copy(w, dependencyResponse.Body) return } @@ -125,8 +132,8 @@ func (p *Injector) Inject(w http.ResponseWriter, r *http.Request, sendData strin } } -func (p *Injector) fetchUrl(ctx context.Context, params *entryParams) (*http.Response, error) { - r, err := http.NewRequestWithContext(ctx, "GET", params.Url, nil) +func (p *Injector) fetchURL(ctx context.Context, params *entryParams) (*http.Response, error) { + r, err := http.NewRequestWithContext(ctx, "GET", params.URL, nil) if err != nil { return nil, fmt.Errorf("dependency proxy: failed to fetch dependency: %w", err) } @@ -137,8 +144,8 @@ func (p *Injector) fetchUrl(ctx context.Context, params *entryParams) (*http.Res func (p *Injector) newUploadRequest(ctx context.Context, params *entryParams, originalRequest *http.Request, body io.Reader) (*http.Request, error) { method := p.uploadMethodFrom(params) - uploadUrl := p.uploadUrlFrom(params, originalRequest) - request, err := http.NewRequestWithContext(ctx, method, uploadUrl, body) + uploadURL := p.uploadURLFrom(params, originalRequest) + request, err := http.NewRequestWithContext(ctx, method, uploadURL, body) if err != nil { return nil, err } @@ -174,9 +181,9 @@ func (p *Injector) validateParams(params entryParams) error { return fmt.Errorf("invalid upload method %s", uploadMethod) } - var uploadUrl = params.UploadConfig.Url - if uploadUrl != "" { - if _, err := url.ParseRequestURI(uploadUrl); err != nil { + var uploadURL = params.UploadConfig.URL + if uploadURL != "" { + if _, err := url.ParseRequestURI(uploadURL); err != nil { return fmt.Errorf("invalid upload url %w", err) } } @@ -191,9 +198,9 @@ func (p *Injector) uploadMethodFrom(params *entryParams) string { return http.MethodPost } -func (p *Injector) uploadUrlFrom(params *entryParams, originalRequest *http.Request) string { - if params.UploadConfig.Url != "" { - return params.UploadConfig.Url +func (p *Injector) uploadURLFrom(params *entryParams, originalRequest *http.Request) string { + if params.UploadConfig.URL != "" { + return params.UploadConfig.URL } return originalRequest.URL.String() + "/upload" diff --git a/workhorse/internal/dependencyproxy/dependencyproxy_test.go b/workhorse/internal/dependencyproxy/dependencyproxy_test.go index b028fbcf355..865213bc382 100644 --- a/workhorse/internal/dependencyproxy/dependencyproxy_test.go +++ b/workhorse/internal/dependencyproxy/dependencyproxy_test.go @@ -26,6 +26,11 @@ type fakeUploadHandler struct { handler func(w http.ResponseWriter, r *http.Request) } +const ( + tokenJSON = `{"Token": "token", "Url": "` + urlJSON = `/url"}` +) + func (f *fakeUploadHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { f.request = r @@ -37,7 +42,7 @@ func (f *fakeUploadHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { type errWriter struct{ writes int } func (w *errWriter) Header() http.Header { return make(http.Header) } -func (w *errWriter) WriteHeader(h int) {} +func (w *errWriter) WriteHeader(_ int) {} // First call of Write function succeeds while all the subsequent ones fail func (w *errWriter) Write(p []byte) (int, error) { @@ -105,7 +110,7 @@ func TestInject(t *testing.T) { injector.SetUploadHandler(bodyUploader) r := httptest.NewRequest("GET", "/target", nil) - sendData := base64.StdEncoding.EncodeToString([]byte(`{"Token": "token", "Url": "` + originResourceServer.URL + `/url"}`)) + sendData := base64.StdEncoding.EncodeToString([]byte(tokenJSON + originResourceServer.URL + urlJSON)) injector.Inject(tc.responseWriter, r, sendData) @@ -137,7 +142,7 @@ func TestSuccessfullRequest(t *testing.T) { injector := NewInjector() injector.SetUploadHandler(uploadHandler) - response := makeRequest(injector, `{"Token": "token", "Url": "`+originResourceServer.URL+`/url"}`) + response := makeRequest(injector, tokenJSON+originResourceServer.URL+urlJSON) require.Equal(t, "/target/upload", uploadHandler.request.URL.Path) require.Equal(t, int64(6), uploadHandler.request.ContentLength) @@ -175,35 +180,35 @@ func TestValidUploadConfiguration(t *testing.T) { desc: "with the default values", expectedConfig: uploadConfig{ Method: http.MethodPost, - Url: "/target/upload", + URL: "/target/upload", }, }, { - desc: "with overriden method", + desc: "with overridden method", uploadConfig: &uploadConfig{ Method: http.MethodPut, }, expectedConfig: uploadConfig{ Method: http.MethodPut, - Url: "/target/upload", + URL: "/target/upload", }, }, { - desc: "with overriden url", + desc: "with overridden url", uploadConfig: &uploadConfig{ - Url: "http://test.org/overriden/upload", + URL: "http://test.org/overriden/upload", }, expectedConfig: uploadConfig{ Method: http.MethodPost, - Url: "http://test.org/overriden/upload", + URL: "http://test.org/overriden/upload", }, }, { - desc: "with overriden headers", + desc: "with overridden headers", uploadConfig: &uploadConfig{ Headers: map[string][]string{"Private-Token": {"123456789"}}, }, expectedConfig: uploadConfig{ Headers: map[string][]string{"Private-Token": {"123456789"}}, Method: http.MethodPost, - Url: "/target/upload", + URL: "/target/upload", }, }, } @@ -212,7 +217,7 @@ func TestValidUploadConfiguration(t *testing.T) { t.Run(tc.desc, func(t *testing.T) { uploadHandler := &fakeUploadHandler{ handler: func(w http.ResponseWriter, r *http.Request) { - require.Equal(t, tc.expectedConfig.Url, r.URL.String()) + require.Equal(t, tc.expectedConfig.URL, r.URL.String()) require.Equal(t, tc.expectedConfig.Method, r.Method) if tc.expectedConfig.Headers != nil { @@ -237,12 +242,12 @@ func TestValidUploadConfiguration(t *testing.T) { sendData["UploadConfig"] = tc.uploadConfig } - sendDataJsonString, err := json.Marshal(sendData) + sendDataJSONString, err := json.Marshal(sendData) require.NoError(t, err) - response := makeRequest(injector, string(sendDataJsonString)) + response := makeRequest(injector, string(sendDataJSONString)) - //checking the response + // checking the response require.Equal(t, 200, response.Code) require.Equal(t, string(content), response.Body.String()) // checking remote file request @@ -261,7 +266,7 @@ func TestInvalidUploadConfiguration(t *testing.T) { sendData map[string]interface{} }{ { - desc: "with an invalid overriden method", + desc: "with an invalid overridden method", sendData: mergeMap(baseSendData, map[string]interface{}{ "UploadConfig": map[string]string{ "Method": "TEAPOT", @@ -288,10 +293,10 @@ func TestInvalidUploadConfiguration(t *testing.T) { for _, tc := range testCases { t.Run(tc.desc, func(t *testing.T) { - sendDataJsonString, err := json.Marshal(tc.sendData) + sendDataJSONString, err := json.Marshal(tc.sendData) require.NoError(t, err) - response := makeRequest(NewInjector(), string(sendDataJsonString)) + response := makeRequest(NewInjector(), string(sendDataJSONString)) require.Equal(t, 500, response.Code) require.Equal(t, "Internal Server Error\n", response.Body.String()) @@ -307,24 +312,26 @@ func TestTimeoutConfiguration(t *testing.T) { injector := NewInjector() - var oldHttpClient = httpClient + var oldHTTPClient = httpClient httpClient = &http.Client{ Transport: transport.NewRestrictedTransport(transport.WithResponseHeaderTimeout(10 * time.Millisecond)), } t.Cleanup(func() { - httpClient = oldHttpClient + httpClient = oldHTTPClient }) sendData := map[string]string{ "Url": originResourceServer.URL + "/file", } - sendDataJsonString, err := json.Marshal(sendData) + sendDataJSONString, err := json.Marshal(sendData) require.NoError(t, err) - response := makeRequest(injector, string(sendDataJsonString)) - require.Equal(t, http.StatusGatewayTimeout, response.Result().StatusCode) + response := makeRequest(injector, string(sendDataJSONString)) + responseResult := response.Result() + defer responseResult.Body.Close() + require.Equal(t, http.StatusGatewayTimeout, responseResult.StatusCode) } func mergeMap(from map[string]interface{}, into map[string]interface{}) map[string]interface{} { @@ -363,7 +370,7 @@ func TestFailedOriginServer(t *testing.T) { injector := NewInjector() injector.SetUploadHandler(uploadHandler) - response := makeRequest(injector, `{"Token": "token", "Url": "`+originResourceServer.URL+`/url"}`) + response := makeRequest(injector, tokenJSON+originResourceServer.URL+urlJSON) require.Equal(t, 404, response.Code) require.Equal(t, "Not found", response.Body.String()) diff --git a/workhorse/internal/helper/exception/exception_test.go b/workhorse/internal/helper/exception/exception_test.go new file mode 100644 index 00000000000..60490c28151 --- /dev/null +++ b/workhorse/internal/helper/exception/exception_test.go @@ -0,0 +1,82 @@ +package exception + +import ( + "net/http" + "testing" + + "github.com/stretchr/testify/require" +) + +const ( + URL = "http://example.com" + method = "GET" + authorization = "Authorization" + token = "token" + secret = "secret" + privateToken = "Private-Token" + redacted = "[redacted]" +) + +func TestCleanHeaders(t *testing.T) { + type args struct { + createNewRequest bool + key string + value string + expectedValue string + } + tests := []struct { + name string + args args + }{ + { + name: "no request", + args: args{ + createNewRequest: false, + }, + }, + { + name: "JSON header", + args: args{ + createNewRequest: false, + key: "Accept", + value: "application/json", + expectedValue: "application/json", + }, + }, + { + name: "Authorization header", + args: args{ + createNewRequest: true, + key: authorization, + value: secret, + expectedValue: "[redacted]", + }, + }, + { + name: "Private-Token header", + args: args{ + createNewRequest: true, + key: privateToken, + value: secret, + expectedValue: "[redacted]", + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + var req *http.Request + + if tt.args.createNewRequest { + req, _ = http.NewRequest(method, URL, nil) + req.Header.Set(tt.args.key, tt.args.value) + } + + CleanHeaders(req) + + if tt.args.createNewRequest { + require.Equal(t, tt.args.expectedValue, req.Header.Get(tt.args.key)) + } + }) + } +} diff --git a/workhorse/internal/staticpages/error_pages_test.go b/workhorse/internal/staticpages/error_pages_test.go index 1406cd7f800..afbe70fa084 100644 --- a/workhorse/internal/staticpages/error_pages_test.go +++ b/workhorse/internal/staticpages/error_pages_test.go @@ -169,7 +169,7 @@ func TestIfErrorPageIsPresentedText(t *testing.T) { func TestErrorPageResponseWriterFlushable(t *testing.T) { rw := httptest.NewRecorder() eprw := errorPageResponseWriter{rw: rw} - rc := http.NewResponseController(&eprw) + rc := http.NewResponseController(&eprw) //nolint:bodyclose // false-positive https://github.com/timakin/bodyclose/issues/52 err := rc.Flush() require.NoError(t, err, "the underlying response writer is not flushable") diff --git a/workhorse/internal/staticpages/servefile.go b/workhorse/internal/staticpages/servefile.go index 70163356a68..98307556199 100644 --- a/workhorse/internal/staticpages/servefile.go +++ b/workhorse/internal/staticpages/servefile.go @@ -1,6 +1,7 @@ package staticpages import ( + "context" "errors" "fmt" "net/http" @@ -26,7 +27,8 @@ const ( CacheExpireMax ) -// BUG/QUIRK: If a client requests 'foo%2Fbar' and 'foo/bar' exists, +// ServeExisting serves static assets +// QUIRK: If a client requests 'foo%2Fbar' and 'foo/bar' exists, // handleServeFile will serve foo/bar instead of passing the request // upstream. func (s *Static) ServeExisting(prefix urlprefix.Prefix, cache CacheMode, notFoundHandler http.Handler) http.Handler { @@ -37,18 +39,15 @@ func (s *Static) ServeExisting(prefix urlprefix.Prefix, cache CacheMode, notFoun // We intentionally use r.URL.Path instead of r.URL.EscaptedPath() below. // This is to make it possible to serve static files with e.g. a space %20 in their name. - relativePath, err := s.validatePath(prefix.Strip(r.URL.Path)) + file, err := s.getFile(prefix, r.URL.Path) if err != nil { + if errors.Is(err, errPathTraversal) { + log.WithRequest(r).WithError(err).Error() + } notFoundHandler.ServeHTTP(w, r) return } - file := filepath.Join(s.DocumentRoot, relativePath) - if !strings.HasPrefix(file, s.DocumentRoot) { - log.WithRequest(r).WithError(errPathTraversal).Error() - notFoundHandler.ServeHTTP(w, r) - return - } var content *os.File var fi os.FileInfo @@ -76,19 +75,8 @@ func (s *Static) ServeExisting(prefix urlprefix.Prefix, cache CacheMode, notFoun } }() - if cache == CacheExpireMax { - // Cache statically served files for 1 year - cacheUntil := time.Now().AddDate(1, 0, 0).Format(http.TimeFormat) - w.Header().Set("Cache-Control", "public") - w.Header().Set("Expires", cacheUntil) - } - - log.WithContextFields(r.Context(), log.Fields{ - "file": file, - "encoding": w.Header().Get("Content-Encoding"), - "method": r.Method, - "uri": mask.URL(r.RequestURI), - }).Info("Send static file") + s.setCacheHeaders(w, cache) + s.logFileServed(r.Context(), file, w.Header().Get("Content-Encoding"), r.Method, r.RequestURI) http.ServeContent(w, r, filepath.Base(file), fi.ModTime(), content) }) @@ -96,6 +84,38 @@ func (s *Static) ServeExisting(prefix urlprefix.Prefix, cache CacheMode, notFoun var errPathTraversal = errors.New("path traversal") +func (s *Static) getFile(prefix urlprefix.Prefix, path string) (string, error) { + relativePath, err := s.validatePath(prefix.Strip(path)) + if err != nil { + return "", err + } + + file := filepath.Join(s.DocumentRoot, relativePath) + if !strings.HasPrefix(file, s.DocumentRoot) { + return "", errPathTraversal + } + + return file, nil +} + +func (s *Static) setCacheHeaders(w http.ResponseWriter, cache CacheMode) { + if cache == CacheExpireMax { + // Cache statically served files for 1 year + cacheUntil := time.Now().AddDate(1, 0, 0).Format(http.TimeFormat) + w.Header().Set("Cache-Control", "public") + w.Header().Set("Expires", cacheUntil) + } +} + +func (s *Static) logFileServed(ctx context.Context, file, encoding, method, uri string) { + log.WithContextFields(ctx, log.Fields{ + "file": file, + "encoding": encoding, + "method": method, + "uri": mask.URL(uri), + }).Info("Send static file") +} + func (s *Static) validatePath(filename string) (string, error) { filename = filepath.Clean(filename)