diff options
| -rw-r--r-- | app/models/environment.rb | 26 | ||||
| -rw-r--r-- | config/feature_flags/development/soft_validation_on_external_url.yml | 8 | ||||
| -rw-r--r-- | doc/ci/environments/index.md | 2 | ||||
| -rw-r--r-- | spec/controllers/projects/environments_controller_spec.rb | 2 | ||||
| -rw-r--r-- | spec/models/environment_spec.rb | 84 | ||||
| -rw-r--r-- | spec/services/deployments/update_environment_service_spec.rb | 4 |
6 files changed, 121 insertions, 5 deletions
diff --git a/app/models/environment.rb b/app/models/environment.rb index 68540ce0f5c..f6af5d7e6a2 100644 --- a/app/models/environment.rb +++ b/app/models/environment.rb @@ -56,8 +56,10 @@ class Environment < ApplicationRecord validates :external_url, length: { maximum: 255 }, - allow_nil: true, - addressable_url: true + allow_nil: true + + validates :external_url, addressable_url: true, unless: :soft_validation_on_external_url_enabled? + validate :safe_external_url, if: :soft_validation_on_external_url_enabled? delegate :manual_actions, :other_manual_actions, to: :last_deployment, allow_nil: true delegate :auto_rollback_enabled?, to: :project @@ -493,6 +495,26 @@ class Environment < ApplicationRecord private + def soft_validation_on_external_url_enabled? + ::Feature.enabled?(:soft_validation_on_external_url, project) + end + + # We deliberately avoid using AddressableUrlValidator to allow users to update their environments even if they have + # misconfigured `environment:url` keyword. The external URL is presented as a clickable link on UI and not consumed + # in GitLab internally, thus we sanitize the URL before the persistence to make sure the rendered link is XSS safe. + # See https://gitlab.com/gitlab-org/gitlab/-/issues/337417 + def safe_external_url + return unless self.external_url.present? + + new_external_url = Addressable::URI.parse(self.external_url) + + if Gitlab::Utils::SanitizeNodeLink::UNSAFE_PROTOCOLS.include?(new_external_url.normalized_scheme) + errors.add(:external_url, "#{new_external_url.normalized_scheme} scheme is not allowed") + end + rescue Addressable::URI::InvalidURIError + errors.add(:external_url, 'URI is invalid') + end + def rollout_status_available? has_terminals? end diff --git a/config/feature_flags/development/soft_validation_on_external_url.yml b/config/feature_flags/development/soft_validation_on_external_url.yml new file mode 100644 index 00000000000..3df63a7bb96 --- /dev/null +++ b/config/feature_flags/development/soft_validation_on_external_url.yml @@ -0,0 +1,8 @@ +--- +name: soft_validation_on_external_url +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/91970 +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/367206 +milestone: '15.2' +type: development +group: group::release +default_enabled: false diff --git a/doc/ci/environments/index.md b/doc/ci/environments/index.md index 6ffa68e4873..6d8f655724d 100644 --- a/doc/ci/environments/index.md +++ b/doc/ci/environments/index.md @@ -374,6 +374,8 @@ To retry or rollback a deployment: ### Environment URL +> [Fixed](https://gitlab.com/gitlab-org/gitlab/-/issues/337417) to persist arbitrary URLs in GitLab 15.2 [with a flag](../../administration/feature_flags.md) named `soft_validation_on_external_url`. Disabled by default. + The [environment URL](../yaml/index.md#environmenturl) is displayed in a few places in GitLab: diff --git a/spec/controllers/projects/environments_controller_spec.rb b/spec/controllers/projects/environments_controller_spec.rb index f4cad5790a3..1a6edab795d 100644 --- a/spec/controllers/projects/environments_controller_spec.rb +++ b/spec/controllers/projects/environments_controller_spec.rb @@ -233,7 +233,7 @@ RSpec.describe Projects::EnvironmentsController do end context "when environment params are invalid" do - let(:params) { environment_params.merge(environment: { name: '/foo/', external_url: '/git.gitlab.com' }) } + let(:params) { environment_params.merge(environment: { external_url: 'javascript:alert("hello")' }) } it 'returns bad request' do subject diff --git a/spec/models/environment_spec.rb b/spec/models/environment_spec.rb index 4064f24cff0..c9462acbf69 100644 --- a/spec/models/environment_spec.rb +++ b/spec/models/environment_spec.rb @@ -34,6 +34,90 @@ RSpec.describe Environment, :use_clean_rails_memory_store_caching do it { is_expected.to validate_length_of(:external_url).is_at_most(255) } + describe 'validate and sanitize external url' do + let_it_be_with_refind(:environment) { create(:environment) } + + where(:source_external_url, :expected_error_message) do + 'http://example.com' | nil + 'example.com' | nil + 'www.example.io' | nil + 'http://$URL' | nil + 'http://$(URL)' | nil + 'custom://example.com' | nil + '1.1.1.1' | nil + '$BASE_URL/${CI_COMMIT_REF_NAME}' | nil + '$ENVIRONMENT_URL' | nil + 'https://$SUB.$MAIN' | nil + 'https://$SUB-$REGION.$MAIN' | nil + 'https://example.com?param={()}' | nil + 'http://XSS?x=<script>alert(1)</script>' | nil + 'https://user:${VARIABLE}@example.io' | nil + 'https://example.com/test?param={data}' | nil + 'http://${URL}' | 'URI is invalid' + 'https://${URL}.example/test' | 'URI is invalid' + 'http://test${CI_MERGE_REQUEST_IID}.example.com' | 'URI is invalid' + 'javascript:alert("hello")' | 'javascript scheme is not allowed' + end + with_them do + it 'sets an external URL or an error' do + environment.external_url = source_external_url + + environment.valid? + + if expected_error_message + expect(environment.errors[:external_url].first).to eq(expected_error_message) + else + expect(environment.errors[:external_url]).to be_empty, + "There were unexpected errors: #{environment.errors.full_messages}" + expect(environment.external_url).to eq(source_external_url) + end + end + end + + context 'when soft_validation_on_external_url feature flag is disabled' do + before do + stub_feature_flags(soft_validation_on_external_url: false) + end + + where(:source_external_url, :expected_error_message) do + 'http://example.com' | nil + 'example.com' | 'is blocked: Only allowed schemes are http, https' + 'www.example.io' | 'is blocked: Only allowed schemes are http, https' + 'http://$URL' | 'is blocked: Hostname or IP address invalid' + 'http://$(URL)' | 'is blocked: Hostname or IP address invalid' + 'custom://example.com' | 'is blocked: Only allowed schemes are http, https' + '1.1.1.1' | 'is blocked: Only allowed schemes are http, https' + '$BASE_URL/${CI_COMMIT_REF_NAME}' | 'is blocked: Only allowed schemes are http, https' + '$ENVIRONMENT_URL' | 'is blocked: Only allowed schemes are http, https' + 'https://$SUB.$MAIN' | 'is blocked: Hostname or IP address invalid' + 'https://$SUB-$REGION.$MAIN' | 'is blocked: Hostname or IP address invalid' + 'https://example.com?param={()}' | nil + 'http://XSS?x=<script>alert(1)</script>' | nil + 'https://user:${VARIABLE}@example.io' | nil + 'https://example.com/test?param={data}' | nil + 'http://${URL}' | 'is blocked: URI is invalid' + 'https://${URL}.example/test' | 'is blocked: URI is invalid' + 'http://test${CI_MERGE_REQUEST_IID}.example.com' | 'is blocked: URI is invalid' + 'javascript:alert("hello")' | 'is blocked: Only allowed schemes are http, https' + end + with_them do + it 'sets an external URL or an error' do + environment.external_url = source_external_url + + environment.valid? + + if expected_error_message + expect(environment.errors[:external_url].first).to eq(expected_error_message) + else + expect(environment.errors[:external_url]).to be_empty, + "There were unexpected errors: #{environment.errors.full_messages}" + expect(environment.external_url).to eq(source_external_url) + end + end + end + end + end + describe '.before_save' do it 'ensures environment tier when a new object is created' do environment = build(:environment, name: 'gprd', tier: nil) diff --git a/spec/services/deployments/update_environment_service_spec.rb b/spec/services/deployments/update_environment_service_spec.rb index 8ab53a37a33..d3e5920baaf 100644 --- a/spec/services/deployments/update_environment_service_spec.rb +++ b/spec/services/deployments/update_environment_service_spec.rb @@ -112,7 +112,7 @@ RSpec.describe Deployments::UpdateEnvironmentService do end context 'when external URL is invalid' do - let(:external_url) { 'google.com' } + let(:external_url) { 'javascript:alert("hello")' } it 'fails to update the tier due to validation error' do expect { subject.execute }.not_to change { environment.tier } @@ -123,7 +123,7 @@ RSpec.describe Deployments::UpdateEnvironmentService do .with(an_instance_of(described_class::EnvironmentUpdateFailure), project_id: project.id, environment_id: environment.id, - reason: %q{External url is blocked: Only allowed schemes are http, https}) + reason: %q{External url javascript scheme is not allowed}) .once subject.execute |