diff options
32 files changed, 372 insertions, 137 deletions
diff --git a/.gitlab/issue_templates/Feature proposal.md b/.gitlab/issue_templates/Feature proposal.md index 9c86dff835f..eef1e877ff2 100644 --- a/.gitlab/issue_templates/Feature proposal.md +++ b/.gitlab/issue_templates/Feature proposal.md @@ -39,7 +39,7 @@ Existing personas are: (copy relevant personas out of this comment, and delete a ### Permissions and Security -What permissions are required to perform the described actions? Are they consistent with the existing permissions as documented for users, groups, and projects as appropriate? Is the proposed behavior consistent between the UI, API, and other access methods (e.g. email replies)? +<!-- What permissions are required to perform the described actions? Are they consistent with the existing permissions as documented for users, groups, and projects as appropriate? Is the proposed behavior consistent between the UI, API, and other access methods (e.g. email replies)? --> ### Documentation diff --git a/app/models/ci/group_variable.rb b/app/models/ci/group_variable.rb index 492d1d0329e..323ff560564 100644 --- a/app/models/ci/group_variable.rb +++ b/app/models/ci/group_variable.rb @@ -5,6 +5,7 @@ module Ci extend Gitlab::Ci::Model include HasVariable include Presentable + include Maskable belongs_to :group, class_name: "::Group" diff --git a/app/models/ci/variable.rb b/app/models/ci/variable.rb index 524d79014f8..64836ea4fa4 100644 --- a/app/models/ci/variable.rb +++ b/app/models/ci/variable.rb @@ -5,6 +5,7 @@ module Ci extend Gitlab::Ci::Model include HasVariable include Presentable + include Maskable belongs_to :project diff --git a/app/models/concerns/has_variable.rb b/app/models/concerns/has_variable.rb index dfbe413a878..2ec42a1029b 100644 --- a/app/models/concerns/has_variable.rb +++ b/app/models/concerns/has_variable.rb @@ -21,9 +21,9 @@ module HasVariable def key=(new_key) super(new_key.to_s.strip) end + end - def to_runner_variable - { key: key, value: value, public: false } - end + def to_runner_variable + { key: key, value: value, public: false } end end diff --git a/app/models/concerns/maskable.rb b/app/models/concerns/maskable.rb new file mode 100644 index 00000000000..8793f0ec965 --- /dev/null +++ b/app/models/concerns/maskable.rb @@ -0,0 +1,22 @@ +# frozen_string_literal: true + +module Maskable + extend ActiveSupport::Concern + + # * Single line + # * No escape characters + # * No variables + # * No spaces + # * Minimal length of 8 characters + # * Absolutely no fun is allowed + REGEX = /\A\w{8,}\z/ + + included do + validates :masked, inclusion: { in: [true, false] } + validates :value, format: { with: REGEX }, if: :masked? + end + + def to_runner_variable + super.merge(masked: masked?) + end +end diff --git a/app/models/diff_note.rb b/app/models/diff_note.rb index 279603496b0..805092e527a 100644 --- a/app/models/diff_note.rb +++ b/app/models/diff_note.rb @@ -41,6 +41,14 @@ class DiffNote < Note create_note_diff_file(creation_params) end + # Returns the diff file from `position` + def latest_diff_file + strong_memoize(:latest_diff_file) do + position.diff_file(project.repository) + end + end + + # Returns the diff file from `original_position` def diff_file strong_memoize(:diff_file) do enqueue_diff_file_creation_job if should_create_diff_file? diff --git a/app/models/suggestion.rb b/app/models/suggestion.rb index 7eee4fbbe5f..09034646bff 100644 --- a/app/models/suggestion.rb +++ b/app/models/suggestion.rb @@ -19,11 +19,6 @@ class Suggestion < ApplicationRecord position.file_path end - def diff_file - repository = project.repository - position.diff_file(repository) - end - # For now, suggestions only serve as a way to send patches that # will change a single line (being able to apply multiple in the same place), # which explains `from_line` and `to_line` being the same line. diff --git a/app/services/suggestions/apply_service.rb b/app/services/suggestions/apply_service.rb index 1f720fc835f..f778c5aa5f5 100644 --- a/app/services/suggestions/apply_service.rb +++ b/app/services/suggestions/apply_service.rb @@ -15,7 +15,13 @@ module Suggestions return error('The file has been changed') end - params = file_update_params(suggestion) + diff_file = suggestion.note.latest_diff_file + + unless diff_file + return error('The file was not found') + end + + params = file_update_params(suggestion, diff_file) result = ::Files::UpdateService.new(suggestion.project, @current_user, params).execute if result[:status] == :success @@ -38,8 +44,8 @@ module Suggestions suggestion.position.head_sha == suggestion.noteable.source_branch_sha end - def file_update_params(suggestion) - blob = suggestion.diff_file.new_blob + def file_update_params(suggestion, diff_file) + blob = diff_file.new_blob file_path = suggestion.file_path branch_name = suggestion.branch file_content = new_file_content(suggestion, blob) diff --git a/app/services/suggestions/create_service.rb b/app/services/suggestions/create_service.rb index 77e958cbe0c..c7ac2452c53 100644 --- a/app/services/suggestions/create_service.rb +++ b/app/services/suggestions/create_service.rb @@ -9,6 +9,10 @@ module Suggestions def execute return unless @note.supports_suggestion? + diff_file = @note.latest_diff_file + + return unless diff_file + suggestions = Banzai::SuggestionsParser.parse(@note.note) # For single line suggestion we're only looking forward to @@ -20,7 +24,7 @@ module Suggestions rows = suggestions.map.with_index do |suggestion, index| - from_content = changing_lines(comment_line, comment_line) + from_content = changing_lines(diff_file, comment_line, comment_line) # The parsed suggestion doesn't have information about the correct # ending characters (we may have a line break, or not), so we take @@ -44,8 +48,8 @@ module Suggestions private - def changing_lines(from_line, to_line) - @note.diff_file.new_blob_lines_between(from_line, to_line).join + def changing_lines(diff_file, from_line, to_line) + diff_file.new_blob_lines_between(from_line, to_line).join end def line_break_chars(line) diff --git a/changelogs/unreleased/13784-simple-masking-of-protected-variables-in-logs.yml b/changelogs/unreleased/13784-simple-masking-of-protected-variables-in-logs.yml new file mode 100644 index 00000000000..5c3b6833235 --- /dev/null +++ b/changelogs/unreleased/13784-simple-masking-of-protected-variables-in-logs.yml @@ -0,0 +1,5 @@ +--- +title: Add support for masking CI variables. +merge_request: 25293 +author: +type: added diff --git a/changelogs/unreleased/osw-fetch-latest-version-when-creating-suggestions.yml b/changelogs/unreleased/osw-fetch-latest-version-when-creating-suggestions.yml new file mode 100644 index 00000000000..4e01a13d781 --- /dev/null +++ b/changelogs/unreleased/osw-fetch-latest-version-when-creating-suggestions.yml @@ -0,0 +1,5 @@ +--- +title: Always fetch MR latest version when creating suggestions +merge_request: 25441 +author: +type: fixed diff --git a/db/migrate/20190218134158_add_masked_to_ci_variables.rb b/db/migrate/20190218134158_add_masked_to_ci_variables.rb new file mode 100644 index 00000000000..b4999d5b4a9 --- /dev/null +++ b/db/migrate/20190218134158_add_masked_to_ci_variables.rb @@ -0,0 +1,21 @@ +# frozen_string_literal: true + +# See http://doc.gitlab.com/ce/development/migration_style_guide.html +# for more information on how to write migrations for GitLab. + +class AddMaskedToCiVariables < ActiveRecord::Migration[5.0] + include Gitlab::Database::MigrationHelpers + + # Set this constant to true if this migration requires downtime. + DOWNTIME = false + + disable_ddl_transaction! + + def up + add_column_with_default :ci_variables, :masked, :boolean, default: false, allow_null: false + end + + def down + remove_column :ci_variables, :masked + end +end diff --git a/db/migrate/20190218134209_add_masked_to_ci_group_variables.rb b/db/migrate/20190218134209_add_masked_to_ci_group_variables.rb new file mode 100644 index 00000000000..8633875b341 --- /dev/null +++ b/db/migrate/20190218134209_add_masked_to_ci_group_variables.rb @@ -0,0 +1,21 @@ +# frozen_string_literal: true + +# See http://doc.gitlab.com/ce/development/migration_style_guide.html +# for more information on how to write migrations for GitLab. + +class AddMaskedToCiGroupVariables < ActiveRecord::Migration[5.0] + include Gitlab::Database::MigrationHelpers + + # Set this constant to true if this migration requires downtime. + DOWNTIME = false + + disable_ddl_transaction! + + def up + add_column_with_default :ci_group_variables, :masked, :boolean, default: false, allow_null: false + end + + def down + remove_column :ci_group_variables, :masked + end +end diff --git a/db/schema.rb b/db/schema.rb index 0e850f412ed..a7a83475679 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -405,6 +405,7 @@ ActiveRecord::Schema.define(version: 20190220150130) do t.boolean "protected", default: false, null: false t.datetime_with_timezone "created_at", null: false t.datetime_with_timezone "updated_at", null: false + t.boolean "masked", default: false, null: false t.index ["group_id", "key"], name: "index_ci_group_variables_on_group_id_and_key", unique: true, using: :btree end @@ -602,6 +603,7 @@ ActiveRecord::Schema.define(version: 20190220150130) do t.integer "project_id", null: false t.boolean "protected", default: false, null: false t.string "environment_scope", default: "*", null: false + t.boolean "masked", default: false, null: false t.index ["project_id", "key", "environment_scope"], name: "index_ci_variables_on_project_id_and_key_and_environment_scope", unique: true, using: :btree end diff --git a/lib/gitlab/ci/variables/collection/item.rb b/lib/gitlab/ci/variables/collection/item.rb index e3e4e62cc02..833aa75adb5 100644 --- a/lib/gitlab/ci/variables/collection/item.rb +++ b/lib/gitlab/ci/variables/collection/item.rb @@ -5,12 +5,12 @@ module Gitlab module Variables class Collection class Item - def initialize(key:, value:, public: true, file: false) + def initialize(key:, value:, public: true, file: false, masked: false) raise ArgumentError, "`#{key}` must be of type String or nil value, while it was: #{value.class}" unless value.is_a?(String) || value.nil? @variable = { - key: key, value: value, public: public, file: file + key: key, value: value, public: public, file: file, masked: masked } end @@ -27,9 +27,13 @@ module Gitlab # don't expose `file` attribute at all (stems from what the runner # expects). # + # If the `variable_masking` feature is enabled we expose the `masked` + # attribute, otherwise it's not exposed. + # def to_runner_variable @variable.reject do |hash_key, hash_value| - hash_key == :file && hash_value == false + (hash_key == :file && hash_value == false) || + (hash_key == :masked && !Feature.enabled?(:variable_masking)) end end diff --git a/spec/factories/ci/group_variables.rb b/spec/factories/ci/group_variables.rb index 64716842b12..9bf520a2c0a 100644 --- a/spec/factories/ci/group_variables.rb +++ b/spec/factories/ci/group_variables.rb @@ -2,6 +2,7 @@ FactoryBot.define do factory :ci_group_variable, class: Ci::GroupVariable do sequence(:key) { |n| "VARIABLE_#{n}" } value 'VARIABLE_VALUE' + masked false trait(:protected) do protected true diff --git a/spec/factories/ci/variables.rb b/spec/factories/ci/variables.rb index 3d014b9b54f..97a7c9ba252 100644 --- a/spec/factories/ci/variables.rb +++ b/spec/factories/ci/variables.rb @@ -2,6 +2,7 @@ FactoryBot.define do factory :ci_variable, class: Ci::Variable do sequence(:key) { |n| "VARIABLE_#{n}" } value 'VARIABLE_VALUE' + masked false trait(:protected) do protected true diff --git a/spec/features/group_variables_spec.rb b/spec/features/group_variables_spec.rb index 57e3ddfb39c..1a53e7c9512 100644 --- a/spec/features/group_variables_spec.rb +++ b/spec/features/group_variables_spec.rb @@ -3,7 +3,7 @@ require 'spec_helper' describe 'Group variables', :js do let(:user) { create(:user) } let(:group) { create(:group) } - let!(:variable) { create(:ci_group_variable, key: 'test_key', value: 'test value', group: group) } + let!(:variable) { create(:ci_group_variable, key: 'test_key', value: 'test_value', group: group) } let(:page_path) { group_settings_ci_cd_path(group) } before do diff --git a/spec/features/project_variables_spec.rb b/spec/features/project_variables_spec.rb index a93df3696d2..6bdf5df1036 100644 --- a/spec/features/project_variables_spec.rb +++ b/spec/features/project_variables_spec.rb @@ -3,7 +3,7 @@ require 'spec_helper' describe 'Project variables', :js do let(:user) { create(:user) } let(:project) { create(:project) } - let(:variable) { create(:ci_variable, key: 'test_key', value: 'test value') } + let(:variable) { create(:ci_variable, key: 'test_key', value: 'test_value') } let(:page_path) { project_settings_ci_cd_path(project) } before do diff --git a/spec/lib/gitlab/ci/variables/collection/item_spec.rb b/spec/lib/gitlab/ci/variables/collection/item_spec.rb index 8bf44acb228..3ff2fe18c15 100644 --- a/spec/lib/gitlab/ci/variables/collection/item_spec.rb +++ b/spec/lib/gitlab/ci/variables/collection/item_spec.rb @@ -6,7 +6,7 @@ describe Gitlab::Ci::Variables::Collection::Item do let(:expected_value) { variable_value } let(:variable) do - { key: variable_key, value: variable_value, public: true } + { key: variable_key, value: variable_value, public: true, masked: false } end describe '.new' do @@ -88,7 +88,7 @@ describe Gitlab::Ci::Variables::Collection::Item do resource = described_class.fabricate(variable) expect(resource).to be_a(described_class) - expect(resource).to eq(key: 'CI_VAR', value: '123', public: false) + expect(resource).to eq(key: 'CI_VAR', value: '123', public: false, masked: false) end it 'supports using another collection item' do @@ -134,7 +134,21 @@ describe Gitlab::Ci::Variables::Collection::Item do .to_runner_variable expect(runner_variable) - .to eq(key: 'VAR', value: 'value', public: true, file: true) + .to eq(key: 'VAR', value: 'value', public: true, file: true, masked: false) + end + end + + context 'when variable masking is disabled' do + before do + stub_feature_flags(variable_masking: false) + end + + it 'does not expose the masked field to the runner' do + runner_variable = described_class + .new(key: 'VAR', value: 'value', masked: true) + .to_runner_variable + + expect(runner_variable).to eq(key: 'VAR', value: 'value', public: true) end end end diff --git a/spec/lib/gitlab/ci/variables/collection_spec.rb b/spec/lib/gitlab/ci/variables/collection_spec.rb index 5c91816a586..edb209c0cf4 100644 --- a/spec/lib/gitlab/ci/variables/collection_spec.rb +++ b/spec/lib/gitlab/ci/variables/collection_spec.rb @@ -3,7 +3,7 @@ require 'spec_helper' describe Gitlab::Ci::Variables::Collection do describe '.new' do it 'can be initialized with an array' do - variable = { key: 'VAR', value: 'value', public: true } + variable = { key: 'VAR', value: 'value', public: true, masked: false } collection = described_class.new([variable]) @@ -93,7 +93,7 @@ describe Gitlab::Ci::Variables::Collection do collection = described_class.new([{ key: 'TEST', value: '1' }]) expect(collection.to_runner_variables) - .to eq [{ key: 'TEST', value: '1', public: true }] + .to eq [{ key: 'TEST', value: '1', public: true, masked: false }] end end diff --git a/spec/models/ci/build_spec.rb b/spec/models/ci/build_spec.rb index 17540443688..81ff727b458 100644 --- a/spec/models/ci/build_spec.rb +++ b/spec/models/ci/build_spec.rb @@ -2114,55 +2114,55 @@ describe Ci::Build do context 'returns variables' do let(:predefined_variables) do [ - { key: 'CI_PIPELINE_ID', value: pipeline.id.to_s, public: true }, - { key: 'CI_PIPELINE_URL', value: project.web_url + "/pipelines/#{pipeline.id}", public: true }, - { key: 'CI_JOB_ID', value: build.id.to_s, public: true }, - { key: 'CI_JOB_URL', value: project.web_url + "/-/jobs/#{build.id}", public: true }, - { key: 'CI_JOB_TOKEN', value: 'my-token', public: false }, - { key: 'CI_BUILD_ID', value: build.id.to_s, public: true }, - { key: 'CI_BUILD_TOKEN', value: 'my-token', public: false }, - { key: 'CI_REGISTRY_USER', value: 'gitlab-ci-token', public: true }, - { key: 'CI_REGISTRY_PASSWORD', value: 'my-token', public: false }, - { key: 'CI_REPOSITORY_URL', value: build.repo_url, public: false }, - { key: 'CI', value: 'true', public: true }, - { key: 'GITLAB_CI', value: 'true', public: true }, - { key: 'GITLAB_FEATURES', value: project.licensed_features.join(','), public: true }, - { key: 'CI_SERVER_NAME', value: 'GitLab', public: true }, - { key: 'CI_SERVER_VERSION', value: Gitlab::VERSION, public: true }, - { key: 'CI_SERVER_VERSION_MAJOR', value: Gitlab.version_info.major.to_s, public: true }, - { key: 'CI_SERVER_VERSION_MINOR', value: Gitlab.version_info.minor.to_s, public: true }, - { key: 'CI_SERVER_VERSION_PATCH', value: Gitlab.version_info.patch.to_s, public: true }, - { key: 'CI_SERVER_REVISION', value: Gitlab.revision, public: true }, - { key: 'CI_JOB_NAME', value: 'test', public: true }, - { key: 'CI_JOB_STAGE', value: 'test', public: true }, - { key: 'CI_COMMIT_SHA', value: build.sha, public: true }, - { key: 'CI_COMMIT_SHORT_SHA', value: build.short_sha, public: true }, - { key: 'CI_COMMIT_BEFORE_SHA', value: build.before_sha, public: true }, - { key: 'CI_COMMIT_REF_NAME', value: build.ref, public: true }, - { key: 'CI_COMMIT_REF_SLUG', value: build.ref_slug, public: true }, - { key: 'CI_NODE_TOTAL', value: '1', public: true }, - { key: 'CI_BUILD_REF', value: build.sha, public: true }, - { key: 'CI_BUILD_BEFORE_SHA', value: build.before_sha, public: true }, - { key: 'CI_BUILD_REF_NAME', value: build.ref, public: true }, - { key: 'CI_BUILD_REF_SLUG', value: build.ref_slug, public: true }, - { key: 'CI_BUILD_NAME', value: 'test', public: true }, - { key: 'CI_BUILD_STAGE', value: 'test', public: true }, - { key: 'CI_PROJECT_ID', value: project.id.to_s, public: true }, - { key: 'CI_PROJECT_NAME', value: project.path, public: true }, - { key: 'CI_PROJECT_PATH', value: project.full_path, public: true }, - { key: 'CI_PROJECT_PATH_SLUG', value: project.full_path_slug, public: true }, - { key: 'CI_PROJECT_NAMESPACE', value: project.namespace.full_path, public: true }, - { key: 'CI_PROJECT_URL', value: project.web_url, public: true }, - { key: 'CI_PROJECT_VISIBILITY', value: 'private', public: true }, - { key: 'CI_PAGES_DOMAIN', value: Gitlab.config.pages.host, public: true }, - { key: 'CI_PAGES_URL', value: project.pages_url, public: true }, - { key: 'CI_API_V4_URL', value: 'http://localhost/api/v4', public: true }, - { key: 'CI_PIPELINE_IID', value: pipeline.iid.to_s, public: true }, - { key: 'CI_CONFIG_PATH', value: pipeline.ci_yaml_file_path, public: true }, - { key: 'CI_PIPELINE_SOURCE', value: pipeline.source, public: true }, - { key: 'CI_COMMIT_MESSAGE', value: pipeline.git_commit_message, public: true }, - { key: 'CI_COMMIT_TITLE', value: pipeline.git_commit_title, public: true }, - { key: 'CI_COMMIT_DESCRIPTION', value: pipeline.git_commit_description, public: true } + { key: 'CI_PIPELINE_ID', value: pipeline.id.to_s, public: true, masked: false }, + { key: 'CI_PIPELINE_URL', value: project.web_url + "/pipelines/#{pipeline.id}", public: true, masked: false }, + { key: 'CI_JOB_ID', value: build.id.to_s, public: true, masked: false }, + { key: 'CI_JOB_URL', value: project.web_url + "/-/jobs/#{build.id}", public: true, masked: false }, + { key: 'CI_JOB_TOKEN', value: 'my-token', public: false, masked: false }, + { key: 'CI_BUILD_ID', value: build.id.to_s, public: true, masked: false }, + { key: 'CI_BUILD_TOKEN', value: 'my-token', public: false, masked: false }, + { key: 'CI_REGISTRY_USER', value: 'gitlab-ci-token', public: true, masked: false }, + { key: 'CI_REGISTRY_PASSWORD', value: 'my-token', public: false, masked: false }, + { key: 'CI_REPOSITORY_URL', value: build.repo_url, public: false, masked: false }, + { key: 'CI', value: 'true', public: true, masked: false }, + { key: 'GITLAB_CI', value: 'true', public: true, masked: false }, + { key: 'GITLAB_FEATURES', value: project.licensed_features.join(','), public: true, masked: false }, + { key: 'CI_SERVER_NAME', value: 'GitLab', public: true, masked: false }, + { key: 'CI_SERVER_VERSION', value: Gitlab::VERSION, public: true, masked: false }, + { key: 'CI_SERVER_VERSION_MAJOR', value: Gitlab.version_info.major.to_s, public: true, masked: false }, + { key: 'CI_SERVER_VERSION_MINOR', value: Gitlab.version_info.minor.to_s, public: true, masked: false }, + { key: 'CI_SERVER_VERSION_PATCH', value: Gitlab.version_info.patch.to_s, public: true, masked: false }, + { key: 'CI_SERVER_REVISION', value: Gitlab.revision, public: true, masked: false }, + { key: 'CI_JOB_NAME', value: 'test', public: true, masked: false }, + { key: 'CI_JOB_STAGE', value: 'test', public: true, masked: false }, + { key: 'CI_COMMIT_SHA', value: build.sha, public: true, masked: false }, + { key: 'CI_COMMIT_SHORT_SHA', value: build.short_sha, public: true, masked: false }, + { key: 'CI_COMMIT_BEFORE_SHA', value: build.before_sha, public: true, masked: false }, + { key: 'CI_COMMIT_REF_NAME', value: build.ref, public: true, masked: false }, + { key: 'CI_COMMIT_REF_SLUG', value: build.ref_slug, public: true, masked: false }, + { key: 'CI_NODE_TOTAL', value: '1', public: true, masked: false }, + { key: 'CI_BUILD_REF', value: build.sha, public: true, masked: false }, + { key: 'CI_BUILD_BEFORE_SHA', value: build.before_sha, public: true, masked: false }, + { key: 'CI_BUILD_REF_NAME', value: build.ref, public: true, masked: false }, + { key: 'CI_BUILD_REF_SLUG', value: build.ref_slug, public: true, masked: false }, + { key: 'CI_BUILD_NAME', value: 'test', public: true, masked: false }, + { key: 'CI_BUILD_STAGE', value: 'test', public: true, masked: false }, + { key: 'CI_PROJECT_ID', value: project.id.to_s, public: true, masked: false }, + { key: 'CI_PROJECT_NAME', value: project.path, public: true, masked: false }, + { key: 'CI_PROJECT_PATH', value: project.full_path, public: true, masked: false }, + { key: 'CI_PROJECT_PATH_SLUG', value: project.full_path_slug, public: true, masked: false }, + { key: 'CI_PROJECT_NAMESPACE', value: project.namespace.full_path, public: true, masked: false }, + { key: 'CI_PROJECT_URL', value: project.web_url, public: true, masked: false }, + { key: 'CI_PROJECT_VISIBILITY', value: 'private', public: true, masked: false }, + { key: 'CI_PAGES_DOMAIN', value: Gitlab.config.pages.host, public: true, masked: false }, + { key: 'CI_PAGES_URL', value: project.pages_url, public: true, masked: false }, + { key: 'CI_API_V4_URL', value: 'http://localhost/api/v4', public: true, masked: false }, + { key: 'CI_PIPELINE_IID', value: pipeline.iid.to_s, public: true, masked: false }, + { key: 'CI_CONFIG_PATH', value: pipeline.ci_yaml_file_path, public: true, masked: false }, + { key: 'CI_PIPELINE_SOURCE', value: pipeline.source, public: true, masked: false }, + { key: 'CI_COMMIT_MESSAGE', value: pipeline.git_commit_message, public: true, masked: false }, + { key: 'CI_COMMIT_TITLE', value: pipeline.git_commit_title, public: true, masked: false }, + { key: 'CI_COMMIT_DESCRIPTION', value: pipeline.git_commit_description, public: true, masked: false } ] end @@ -2175,10 +2175,10 @@ describe Ci::Build do describe 'variables ordering' do context 'when variables hierarchy is stubbed' do - let(:build_pre_var) { { key: 'build', value: 'value', public: true } } - let(:project_pre_var) { { key: 'project', value: 'value', public: true } } - let(:pipeline_pre_var) { { key: 'pipeline', value: 'value', public: true } } - let(:build_yaml_var) { { key: 'yaml', value: 'value', public: true } } + let(:build_pre_var) { { key: 'build', value: 'value', public: true, masked: false } } + let(:project_pre_var) { { key: 'project', value: 'value', public: true, masked: false } } + let(:pipeline_pre_var) { { key: 'pipeline', value: 'value', public: true, masked: false } } + let(:build_yaml_var) { { key: 'yaml', value: 'value', public: true, masked: false } } before do allow(build).to receive(:predefined_variables) { [build_pre_var] } @@ -2200,7 +2200,7 @@ describe Ci::Build do project_pre_var, pipeline_pre_var, build_yaml_var, - { key: 'secret', value: 'value', public: false }]) + { key: 'secret', value: 'value', public: false, masked: false }]) end end @@ -2233,10 +2233,10 @@ describe Ci::Build do context 'when build has user' do let(:user_variables) do [ - { key: 'GITLAB_USER_ID', value: user.id.to_s, public: true }, - { key: 'GITLAB_USER_EMAIL', value: user.email, public: true }, - { key: 'GITLAB_USER_LOGIN', value: user.username, public: true }, - { key: 'GITLAB_USER_NAME', value: user.name, public: true } + { key: 'GITLAB_USER_ID', value: user.id.to_s, public: true, masked: false }, + { key: 'GITLAB_USER_EMAIL', value: user.email, public: true, masked: false }, + { key: 'GITLAB_USER_LOGIN', value: user.username, public: true, masked: false }, + { key: 'GITLAB_USER_NAME', value: user.name, public: true, masked: false } ] end @@ -2250,8 +2250,8 @@ describe Ci::Build do context 'when build has an environment' do let(:environment_variables) do [ - { key: 'CI_ENVIRONMENT_NAME', value: 'production', public: true }, - { key: 'CI_ENVIRONMENT_SLUG', value: 'prod-slug', public: true } + { key: 'CI_ENVIRONMENT_NAME', value: 'production', public: true, masked: false }, + { key: 'CI_ENVIRONMENT_SLUG', value: 'prod-slug', public: true, masked: false } ] end @@ -2286,7 +2286,7 @@ describe Ci::Build do before do environment_variables << - { key: 'CI_ENVIRONMENT_URL', value: url, public: true } + { key: 'CI_ENVIRONMENT_URL', value: url, public: true, masked: false } end context 'when the URL was set from the job' do @@ -2323,7 +2323,7 @@ describe Ci::Build do end let(:manual_variable) do - { key: 'CI_JOB_MANUAL', value: 'true', public: true } + { key: 'CI_JOB_MANUAL', value: 'true', public: true, masked: false } end it { is_expected.to include(manual_variable) } @@ -2331,7 +2331,7 @@ describe Ci::Build do context 'when build is for tag' do let(:tag_variable) do - { key: 'CI_COMMIT_TAG', value: 'master', public: true } + { key: 'CI_COMMIT_TAG', value: 'master', public: true, masked: false } end before do @@ -2343,7 +2343,7 @@ describe Ci::Build do context 'when CI variable is defined' do let(:ci_variable) do - { key: 'SECRET_KEY', value: 'secret_value', public: false } + { key: 'SECRET_KEY', value: 'secret_value', public: false, masked: false } end before do @@ -2358,7 +2358,7 @@ describe Ci::Build do let(:ref) { Gitlab::Git::BRANCH_REF_PREFIX + build.ref } let(:protected_variable) do - { key: 'PROTECTED_KEY', value: 'protected_value', public: false } + { key: 'PROTECTED_KEY', value: 'protected_value', public: false, masked: false } end before do @@ -2390,7 +2390,7 @@ describe Ci::Build do context 'when group CI variable is defined' do let(:ci_variable) do - { key: 'SECRET_KEY', value: 'secret_value', public: false } + { key: 'SECRET_KEY', value: 'secret_value', public: false, masked: false } end before do @@ -2405,7 +2405,7 @@ describe Ci::Build do let(:ref) { Gitlab::Git::BRANCH_REF_PREFIX + build.ref } let(:protected_variable) do - { key: 'PROTECTED_KEY', value: 'protected_value', public: false } + { key: 'PROTECTED_KEY', value: 'protected_value', public: false, masked: false } end before do @@ -2444,11 +2444,11 @@ describe Ci::Build do let(:trigger_request) { create(:ci_trigger_request, pipeline: pipeline, trigger: trigger) } let(:user_trigger_variable) do - { key: 'TRIGGER_KEY_1', value: 'TRIGGER_VALUE_1', public: false } + { key: 'TRIGGER_KEY_1', value: 'TRIGGER_VALUE_1', public: false, masked: false } end let(:predefined_trigger_variable) do - { key: 'CI_PIPELINE_TRIGGERED', value: 'true', public: true } + { key: 'CI_PIPELINE_TRIGGERED', value: 'true', public: true, masked: false } end before do @@ -2480,7 +2480,7 @@ describe Ci::Build do context 'when pipeline has a variable' do let!(:pipeline_variable) { create(:ci_pipeline_variable, pipeline: pipeline) } - it { is_expected.to include(pipeline_variable.to_runner_variable) } + it { is_expected.to include(key: pipeline_variable.key, value: pipeline_variable.value, public: false, masked: false) } end context 'when a job was triggered by a pipeline schedule' do @@ -2497,16 +2497,16 @@ describe Ci::Build do pipeline_schedule.reload end - it { is_expected.to include(pipeline_schedule_variable.to_runner_variable) } + it { is_expected.to include(key: pipeline_schedule_variable.key, value: pipeline_schedule_variable.value, public: false, masked: false) } end context 'when container registry is enabled' do let(:container_registry_enabled) { true } let(:ci_registry) do - { key: 'CI_REGISTRY', value: 'registry.example.com', public: true } + { key: 'CI_REGISTRY', value: 'registry.example.com', public: true, masked: false } end let(:ci_registry_image) do - { key: 'CI_REGISTRY_IMAGE', value: project.container_registry_url, public: true } + { key: 'CI_REGISTRY_IMAGE', value: project.container_registry_url, public: true, masked: false } end context 'and is disabled for project' do @@ -2535,13 +2535,13 @@ describe Ci::Build do build.update(runner: runner) end - it { is_expected.to include({ key: 'CI_RUNNER_ID', value: runner.id.to_s, public: true }) } - it { is_expected.to include({ key: 'CI_RUNNER_DESCRIPTION', value: 'description', public: true }) } - it { is_expected.to include({ key: 'CI_RUNNER_TAGS', value: 'docker, linux', public: true }) } + it { is_expected.to include({ key: 'CI_RUNNER_ID', value: runner.id.to_s, public: true, masked: false }) } + it { is_expected.to include({ key: 'CI_RUNNER_DESCRIPTION', value: 'description', public: true, masked: false }) } + it { is_expected.to include({ key: 'CI_RUNNER_TAGS', value: 'docker, linux', public: true, masked: false }) } end context 'when build is for a deployment' do - let(:deployment_variable) { { key: 'KUBERNETES_TOKEN', value: 'TOKEN', public: false } } + let(:deployment_variable) { { key: 'KUBERNETES_TOKEN', value: 'TOKEN', public: false, masked: false } } before do build.environment = 'production' @@ -2555,7 +2555,7 @@ describe Ci::Build do end context 'when project has custom CI config path' do - let(:ci_config_path) { { key: 'CI_CONFIG_PATH', value: 'custom', public: true } } + let(:ci_config_path) { { key: 'CI_CONFIG_PATH', value: 'custom', public: true, masked: false } } before do project.update(ci_config_path: 'custom') @@ -2572,7 +2572,7 @@ describe Ci::Build do it "includes AUTO_DEVOPS_DOMAIN" do is_expected.to include( - { key: 'AUTO_DEVOPS_DOMAIN', value: 'example.com', public: true }) + { key: 'AUTO_DEVOPS_DOMAIN', value: 'example.com', public: true, masked: false }) end end @@ -2583,7 +2583,7 @@ describe Ci::Build do it "includes AUTO_DEVOPS_DOMAIN" do is_expected.not_to include( - { key: 'AUTO_DEVOPS_DOMAIN', value: 'example.com', public: true }) + { key: 'AUTO_DEVOPS_DOMAIN', value: 'example.com', public: true, masked: false }) end end end @@ -2598,9 +2598,9 @@ describe Ci::Build do variables = subject.reverse.uniq { |variable| variable[:key] }.reverse expect(variables) - .not_to include(key: 'MYVAR', value: 'myvar', public: true) + .not_to include(key: 'MYVAR', value: 'myvar', public: true, masked: false) expect(variables) - .to include(key: 'MYVAR', value: 'pipeline value', public: false) + .to include(key: 'MYVAR', value: 'pipeline value', public: false, masked: false) end end @@ -2616,13 +2616,13 @@ describe Ci::Build do it 'includes CI_NODE_INDEX' do is_expected.to include( - { key: 'CI_NODE_INDEX', value: index.to_s, public: true } + { key: 'CI_NODE_INDEX', value: index.to_s, public: true, masked: false } ) end it 'includes correct CI_NODE_TOTAL' do is_expected.to include( - { key: 'CI_NODE_TOTAL', value: total.to_s, public: true } + { key: 'CI_NODE_TOTAL', value: total.to_s, public: true, masked: false } ) end end @@ -2641,7 +2641,7 @@ describe Ci::Build do it 'returns static predefined variables' do expect(build.variables.size).to be >= 28 expect(build.variables) - .to include(key: 'CI_COMMIT_REF_NAME', value: 'feature', public: true) + .to include(key: 'CI_COMMIT_REF_NAME', value: 'feature', public: true, masked: false) expect(build).not_to be_persisted end end @@ -2651,8 +2651,8 @@ describe Ci::Build do let(:deploy_token_variables) do [ - { key: 'CI_DEPLOY_USER', value: deploy_token.username, public: true }, - { key: 'CI_DEPLOY_PASSWORD', value: deploy_token.token, public: false } + { key: 'CI_DEPLOY_USER', value: deploy_token.username, public: true, masked: false }, + { key: 'CI_DEPLOY_PASSWORD', value: deploy_token.token, public: false, masked: false } ] end @@ -2711,7 +2711,7 @@ describe Ci::Build do end expect(variables) - .to include(key: 'CI_COMMIT_REF_NAME', value: 'feature', public: true) + .to include(key: 'CI_COMMIT_REF_NAME', value: 'feature', public: true, masked: false) end it 'does not return prohibited variables' do diff --git a/spec/models/ci/group_variable_spec.rb b/spec/models/ci/group_variable_spec.rb index 1b10501701c..21d96bf3454 100644 --- a/spec/models/ci/group_variable_spec.rb +++ b/spec/models/ci/group_variable_spec.rb @@ -5,6 +5,7 @@ describe Ci::GroupVariable do it { is_expected.to include_module(HasVariable) } it { is_expected.to include_module(Presentable) } + it { is_expected.to include_module(Maskable) } it { is_expected.to validate_uniqueness_of(:key).scoped_to(:group_id).with_message(/\(\w+\) has already been taken/) } describe '.unprotected' do diff --git a/spec/models/ci/variable_spec.rb b/spec/models/ci/variable_spec.rb index 875e8b2b682..02c07a2bd83 100644 --- a/spec/models/ci/variable_spec.rb +++ b/spec/models/ci/variable_spec.rb @@ -6,6 +6,7 @@ describe Ci::Variable do describe 'validations' do it { is_expected.to include_module(HasVariable) } it { is_expected.to include_module(Presentable) } + it { is_expected.to include_module(Maskable) } it { is_expected.to validate_uniqueness_of(:key).scoped_to(:project_id, :environment_scope).with_message(/\(\w+\) has already been taken/) } end diff --git a/spec/models/concerns/has_variable_spec.rb b/spec/models/concerns/has_variable_spec.rb index 3fbe86c5b56..bff96e12ffa 100644 --- a/spec/models/concerns/has_variable_spec.rb +++ b/spec/models/concerns/has_variable_spec.rb @@ -57,7 +57,7 @@ describe HasVariable do describe '#to_runner_variable' do it 'returns a hash for the runner' do expect(subject.to_runner_variable) - .to eq(key: subject.key, value: subject.value, public: false) + .to include(key: subject.key, value: subject.value, public: false) end end end diff --git a/spec/models/concerns/maskable_spec.rb b/spec/models/concerns/maskable_spec.rb new file mode 100644 index 00000000000..aeba7ad862f --- /dev/null +++ b/spec/models/concerns/maskable_spec.rb @@ -0,0 +1,76 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Maskable do + let(:variable) { build(:ci_variable) } + + describe 'masked value validations' do + subject { variable } + + context 'when variable is masked' do + before do + subject.masked = true + end + + it { is_expected.not_to allow_value('hello').for(:value) } + it { is_expected.not_to allow_value('hello world').for(:value) } + it { is_expected.not_to allow_value('hello$VARIABLEworld').for(:value) } + it { is_expected.not_to allow_value('hello\rworld').for(:value) } + it { is_expected.to allow_value('helloworld').for(:value) } + end + + context 'when variable is not masked' do + before do + subject.masked = false + end + + it { is_expected.to allow_value('hello').for(:value) } + it { is_expected.to allow_value('hello world').for(:value) } + it { is_expected.to allow_value('hello$VARIABLEworld').for(:value) } + it { is_expected.to allow_value('hello\rworld').for(:value) } + it { is_expected.to allow_value('helloworld').for(:value) } + end + end + + describe 'REGEX' do + subject { Maskable::REGEX } + + it 'does not match strings shorter than 8 letters' do + expect(subject.match?('hello')).to eq(false) + end + + it 'does not match strings with spaces' do + expect(subject.match?('hello world')).to eq(false) + end + + it 'does not match strings with shell variables' do + expect(subject.match?('hello$VARIABLEworld')).to eq(false) + end + + it 'does not match strings with escape characters' do + expect(subject.match?('hello\rworld')).to eq(false) + end + + it 'does not match strings that span more than one line' do + string = <<~EOS + hello + world + EOS + + expect(subject.match?(string)).to eq(false) + end + + it 'matches valid strings' do + expect(subject.match?('helloworld')).to eq(true) + end + end + + describe '#to_runner_variable' do + subject { variable.to_runner_variable } + + it 'exposes the masked attribute' do + expect(subject).to include(:masked) + end + end +end diff --git a/spec/requests/api/group_variables_spec.rb b/spec/requests/api/group_variables_spec.rb index e52f4c70407..66b9aae4b58 100644 --- a/spec/requests/api/group_variables_spec.rb +++ b/spec/requests/api/group_variables_spec.rb @@ -87,12 +87,12 @@ describe API::GroupVariables do it 'creates variable' do expect do - post api("/groups/#{group.id}/variables", user), params: { key: 'TEST_VARIABLE_2', value: 'VALUE_2', protected: true } + post api("/groups/#{group.id}/variables", user), params: { key: 'TEST_VARIABLE_2', value: 'PROTECTED_VALUE_2', protected: true } end.to change {group.variables.count}.by(1) expect(response).to have_gitlab_http_status(201) expect(json_response['key']).to eq('TEST_VARIABLE_2') - expect(json_response['value']).to eq('VALUE_2') + expect(json_response['value']).to eq('PROTECTED_VALUE_2') expect(json_response['protected']).to be_truthy end diff --git a/spec/requests/api/runner_spec.rb b/spec/requests/api/runner_spec.rb index e6c235ca26e..43c06f7c973 100644 --- a/spec/requests/api/runner_spec.rb +++ b/spec/requests/api/runner_spec.rb @@ -436,9 +436,9 @@ describe API::Runner, :clean_gitlab_redis_shared_state do end let(:expected_variables) do - [{ 'key' => 'CI_JOB_NAME', 'value' => 'spinach', 'public' => true }, - { 'key' => 'CI_JOB_STAGE', 'value' => 'test', 'public' => true }, - { 'key' => 'DB_NAME', 'value' => 'postgres', 'public' => true }] + [{ 'key' => 'CI_JOB_NAME', 'value' => 'spinach', 'public' => true, 'masked' => false }, + { 'key' => 'CI_JOB_STAGE', 'value' => 'test', 'public' => true, 'masked' => false }, + { 'key' => 'DB_NAME', 'value' => 'postgres', 'public' => true, 'masked' => false }] end let(:expected_artifacts) do @@ -740,12 +740,12 @@ describe API::Runner, :clean_gitlab_redis_shared_state do context 'when triggered job is available' do let(:expected_variables) do - [{ 'key' => 'CI_JOB_NAME', 'value' => 'spinach', 'public' => true }, - { 'key' => 'CI_JOB_STAGE', 'value' => 'test', 'public' => true }, - { 'key' => 'CI_PIPELINE_TRIGGERED', 'value' => 'true', 'public' => true }, - { 'key' => 'DB_NAME', 'value' => 'postgres', 'public' => true }, - { 'key' => 'SECRET_KEY', 'value' => 'secret_value', 'public' => false }, - { 'key' => 'TRIGGER_KEY_1', 'value' => 'TRIGGER_VALUE_1', 'public' => false }] + [{ 'key' => 'CI_JOB_NAME', 'value' => 'spinach', 'public' => true, 'masked' => false }, + { 'key' => 'CI_JOB_STAGE', 'value' => 'test', 'public' => true, 'masked' => false }, + { 'key' => 'CI_PIPELINE_TRIGGERED', 'value' => 'true', 'public' => true, 'masked' => false }, + { 'key' => 'DB_NAME', 'value' => 'postgres', 'public' => true, 'masked' => false }, + { 'key' => 'SECRET_KEY', 'value' => 'secret_value', 'public' => false, 'masked' => false }, + { 'key' => 'TRIGGER_KEY_1', 'value' => 'TRIGGER_VALUE_1', 'public' => false, 'masked' => false }] end let(:trigger) { create(:ci_trigger, project: project) } diff --git a/spec/requests/api/variables_spec.rb b/spec/requests/api/variables_spec.rb index cdac5b2f400..5df6baf0ddf 100644 --- a/spec/requests/api/variables_spec.rb +++ b/spec/requests/api/variables_spec.rb @@ -73,12 +73,12 @@ describe API::Variables do context 'authorized user with proper permissions' do it 'creates variable' do expect do - post api("/projects/#{project.id}/variables", user), params: { key: 'TEST_VARIABLE_2', value: 'VALUE_2', protected: true } + post api("/projects/#{project.id}/variables", user), params: { key: 'TEST_VARIABLE_2', value: 'PROTECTED_VALUE_2', protected: true } end.to change {project.variables.count}.by(1) expect(response).to have_gitlab_http_status(201) expect(json_response['key']).to eq('TEST_VARIABLE_2') - expect(json_response['value']).to eq('VALUE_2') + expect(json_response['value']).to eq('PROTECTED_VALUE_2') expect(json_response['protected']).to be_truthy end diff --git a/spec/services/suggestions/apply_service_spec.rb b/spec/services/suggestions/apply_service_spec.rb index 8e77d582eb4..fe85b5c9065 100644 --- a/spec/services/suggestions/apply_service_spec.rb +++ b/spec/services/suggestions/apply_service_spec.rb @@ -362,6 +362,17 @@ describe Suggestions::ApplyService do project.add_maintainer(user) end + context 'diff file was not found' do + it 'returns error message' do + expect(suggestion.note).to receive(:latest_diff_file) { nil } + + result = subject.execute(suggestion) + + expect(result).to eq(message: 'The file was not found', + status: :error) + end + end + context 'suggestion was already applied' do it 'returns success status' do result = subject.execute(suggestion) diff --git a/spec/services/suggestions/create_service_spec.rb b/spec/services/suggestions/create_service_spec.rb index f1142c88a69..1b4b15b8eaa 100644 --- a/spec/services/suggestions/create_service_spec.rb +++ b/spec/services/suggestions/create_service_spec.rb @@ -9,14 +9,18 @@ describe Suggestions::CreateService do target_project: project_with_repo) end - let(:position) do - Gitlab::Diff::Position.new(old_path: "files/ruby/popen.rb", - new_path: "files/ruby/popen.rb", - old_line: nil, - new_line: 14, - diff_refs: merge_request.diff_refs) + def build_position(args = {}) + default_args = { old_path: "files/ruby/popen.rb", + new_path: "files/ruby/popen.rb", + old_line: nil, + new_line: 14, + diff_refs: merge_request.diff_refs } + + Gitlab::Diff::Position.new(default_args.merge(args)) end + let(:position) { build_position } + let(:markdown) do <<-MARKDOWN.strip_heredoc ```suggestion @@ -74,6 +78,21 @@ describe Suggestions::CreateService do end end + context 'should not create suggestions' do + let(:note) do + create(:diff_note_on_merge_request, project: project_with_repo, + noteable: merge_request, + position: position, + note: markdown) + end + + it 'creates no suggestion when diff file is not found' do + expect(note).to receive(:latest_diff_file) { nil } + + expect { subject.execute }.not_to change(Suggestion, :count) + end + end + context 'should create suggestions' do let(:note) do create(:diff_note_on_merge_request, project: project_with_repo, @@ -104,6 +123,22 @@ describe Suggestions::CreateService do expect(suggestion_2).to have_attributes(from_content: " vars = {\n", to_content: " xpto\n baz\n") end + + context 'outdated position note' do + let!(:outdated_diff) { merge_request.merge_request_diff } + let!(:latest_diff) { merge_request.create_merge_request_diff } + let(:outdated_position) { build_position(diff_refs: outdated_diff.diff_refs) } + let(:position) { build_position(diff_refs: latest_diff.diff_refs) } + + it 'uses the correct position when creating the suggestion' do + expect(note.position) + .to receive(:diff_file) + .with(project_with_repo.repository) + .and_call_original + + subject.execute + end + end end end end diff --git a/spec/support/features/variable_list_shared_examples.rb b/spec/support/features/variable_list_shared_examples.rb index 0a464d77cb7..73156d18c1b 100644 --- a/spec/support/features/variable_list_shared_examples.rb +++ b/spec/support/features/variable_list_shared_examples.rb @@ -8,7 +8,7 @@ shared_examples 'variable list' do it 'adds new CI variable' do page.within('.js-ci-variable-list-section .js-row:last-child') do find('.js-ci-variable-input-key').set('key') - find('.js-ci-variable-input-value').set('key value') + find('.js-ci-variable-input-value').set('key_value') end click_button('Save variables') @@ -19,7 +19,7 @@ shared_examples 'variable list' do # We check the first row because it re-sorts to alphabetical order on refresh page.within('.js-ci-variable-list-section .js-row:nth-child(1)') do expect(find('.js-ci-variable-input-key').value).to eq('key') - expect(find('.js-ci-variable-input-value', visible: false).value).to eq('key value') + expect(find('.js-ci-variable-input-value', visible: false).value).to eq('key_value') end end @@ -44,7 +44,7 @@ shared_examples 'variable list' do it 'adds new protected variable' do page.within('.js-ci-variable-list-section .js-row:last-child') do find('.js-ci-variable-input-key').set('key') - find('.js-ci-variable-input-value').set('key value') + find('.js-ci-variable-input-value').set('key_value') find('.ci-variable-protected-item .js-project-feature-toggle').click expect(find('.js-ci-variable-input-protected', visible: false).value).to eq('true') @@ -58,7 +58,7 @@ shared_examples 'variable list' do # We check the first row because it re-sorts to alphabetical order on refresh page.within('.js-ci-variable-list-section .js-row:nth-child(1)') do expect(find('.js-ci-variable-input-key').value).to eq('key') - expect(find('.js-ci-variable-input-value', visible: false).value).to eq('key value') + expect(find('.js-ci-variable-input-value', visible: false).value).to eq('key_value') expect(find('.js-ci-variable-input-protected', visible: false).value).to eq('true') end end |