From 5147cd60f141434f82ce95cc9039afddf415f02b Mon Sep 17 00:00:00 2001
From: GitLab Bot <gitlab-bot@gitlab.com>
Date: Thu, 5 Aug 2021 12:09:57 +0000
Subject: [PATCH] Add latest changes from gitlab-org/gitlab@master

---
 GITALY_SERVER_VERSION                         |  2 +-
 .../components/ci_variable_modal.vue          | 23 +++++
 .../javascripts/ci_variable_list/constants.js |  4 +
 .../javascripts/ci_variable_list/index.js     |  2 +
 app/views/ci/variables/_index.html.haml       |  1 +
 doc/ci/variables/index.md                     | 20 +++++
 lib/gitlab/database.rb                        |  5 ++
 locale/gitlab.pot                             |  6 ++
 .../components/ci_variable_modal_spec.js      | 23 +++++
 spec/support/before_all_adapter.rb            | 27 ++++++
 spec/support/database/prevent_cross_joins.rb  | 84 +++++++++++++++++++
 spec/support/db_cleaner.rb                    |  8 +-
 .../database/prevent_cross_joins_spec.rb      | 56 +++++++++++++
 13 files changed, 259 insertions(+), 2 deletions(-)
 create mode 100644 spec/support/before_all_adapter.rb
 create mode 100644 spec/support/database/prevent_cross_joins.rb
 create mode 100644 spec/support_specs/database/prevent_cross_joins_spec.rb

diff --git a/GITALY_SERVER_VERSION b/GITALY_SERVER_VERSION
index 7e30f2c005d..df0556ead12 100644
--- a/GITALY_SERVER_VERSION
+++ b/GITALY_SERVER_VERSION
@@ -1 +1 @@
-c2ad49fbbf325f45bd31e307451cbf2982a4f647
+6430f0f4df82aecc0b282d8fb620d1d9219a6aee
diff --git a/app/assets/javascripts/ci_variable_list/components/ci_variable_modal.vue b/app/assets/javascripts/ci_variable_list/components/ci_variable_modal.vue
index 12def6e7eef..03fd600e493 100644
--- a/app/assets/javascripts/ci_variable_list/components/ci_variable_modal.vue
+++ b/app/assets/javascripts/ci_variable_list/components/ci_variable_modal.vue
@@ -24,6 +24,7 @@ import {
   ADD_CI_VARIABLE_MODAL_ID,
   AWS_TIP_DISMISSED_COOKIE_NAME,
   AWS_TIP_MESSAGE,
+  CONTAINS_VARIABLE_REFERENCE_MESSAGE,
 } from '../constants';
 import CiEnvironmentsDropdown from './ci_environments_dropdown.vue';
 import { awsTokens, awsTokenList } from './ci_variable_autocomplete_tokens';
@@ -33,6 +34,7 @@ export default {
   tokens: awsTokens,
   tokenList: awsTokenList,
   awsTipMessage: AWS_TIP_MESSAGE,
+  containsVariableReferenceMessage: CONTAINS_VARIABLE_REFERENCE_MESSAGE,
   components: {
     CiEnvironmentsDropdown,
     GlAlert,
@@ -70,6 +72,7 @@ export default {
       'awsTipDeployLink',
       'awsTipCommandsLink',
       'awsTipLearnLink',
+      'containsVariableReferenceLink',
       'protectedEnvironmentVariablesLink',
       'maskedEnvironmentVariablesLink',
     ]),
@@ -99,6 +102,10 @@ export default {
       const regex = RegExp(this.maskableRegex);
       return regex.test(this.variable.secret_value);
     },
+    containsVariableReference() {
+      const regex = RegExp(/\$/);
+      return regex.test(this.variable.secret_value);
+    },
     displayMaskedError() {
       return !this.canMask && this.variable.masked;
     },
@@ -328,6 +335,22 @@ export default {
         </div>
       </gl-alert>
     </gl-collapse>
+    <gl-alert
+      v-if="containsVariableReference"
+      :title="__('Value may contain a variable reference')"
+      :dismissible="false"
+      variant="warning"
+      data-testid="contains-variable-reference"
+    >
+      <gl-sprintf :message="$options.containsVariableReferenceMessage">
+        <template #code="{ content }">
+          <code>{{ content }}</code>
+        </template>
+        <template #docsLink="{ content }">
+          <gl-link :href="containsVariableReferenceLink" target="_blank">{{ content }}</gl-link>
+        </template>
+      </gl-sprintf>
+    </gl-alert>
     <template #modal-footer>
       <gl-button @click="hideModal">{{ __('Cancel') }}</gl-button>
       <gl-button
diff --git a/app/assets/javascripts/ci_variable_list/constants.js b/app/assets/javascripts/ci_variable_list/constants.js
index 564e1d01242..f4002537f79 100644
--- a/app/assets/javascripts/ci_variable_list/constants.js
+++ b/app/assets/javascripts/ci_variable_list/constants.js
@@ -24,3 +24,7 @@ export const AWS_ACCESS_KEY_ID = 'AWS_ACCESS_KEY_ID';
 export const AWS_DEFAULT_REGION = 'AWS_DEFAULT_REGION';
 export const AWS_SECRET_ACCESS_KEY = 'AWS_SECRET_ACCESS_KEY';
 export const AWS_TOKEN_CONSTANTS = [AWS_ACCESS_KEY_ID, AWS_DEFAULT_REGION, AWS_SECRET_ACCESS_KEY];
+
+export const CONTAINS_VARIABLE_REFERENCE_MESSAGE = __(
+  'Variable references indicated by %{codeStart}$%{codeEnd} may be expanded. If this is not what you want, consider %{docsLinkStart}using a workaround to prevent expansion%{docsLinkEnd}.',
+);
diff --git a/app/assets/javascripts/ci_variable_list/index.js b/app/assets/javascripts/ci_variable_list/index.js
index 50856ca9533..7c40f8134d4 100644
--- a/app/assets/javascripts/ci_variable_list/index.js
+++ b/app/assets/javascripts/ci_variable_list/index.js
@@ -14,6 +14,7 @@ const mountCiVariableListApp = (containerEl) => {
     awsTipDeployLink,
     awsTipCommandsLink,
     awsTipLearnLink,
+    containsVariableReferenceLink,
     protectedEnvironmentVariablesLink,
     maskedEnvironmentVariablesLink,
   } = containerEl.dataset;
@@ -30,6 +31,7 @@ const mountCiVariableListApp = (containerEl) => {
     awsTipDeployLink,
     awsTipCommandsLink,
     awsTipLearnLink,
+    containsVariableReferenceLink,
     protectedEnvironmentVariablesLink,
     maskedEnvironmentVariablesLink,
   });
diff --git a/app/views/ci/variables/_index.html.haml b/app/views/ci/variables/_index.html.haml
index 9db5ee23c3e..216852b567d 100644
--- a/app/views/ci/variables/_index.html.haml
+++ b/app/views/ci/variables/_index.html.haml
@@ -16,6 +16,7 @@
   aws_tip_deploy_link: help_page_path('ci/cloud_deployment/index.md', anchor: 'deploy-your-application-to-the-aws-elastic-container-service-ecs'),
   aws_tip_commands_link: help_page_path('ci/cloud_deployment/index.md', anchor: 'run-aws-commands-from-gitlab-cicd'),
   aws_tip_learn_link: help_page_path('ci/cloud_deployment/index.md', anchor: 'aws'),
+  contains_variable_reference_link: help_page_path('ci/variables/index', anchor: 'troubleshooting-variables-containing-references'),
   protected_environment_variables_link: help_page_path('ci/variables/index', anchor: 'protect-a-cicd-variable'),
   masked_environment_variables_link: help_page_path('ci/variables/index', anchor: 'mask-a-cicd-variable'),
 } }
diff --git a/doc/ci/variables/index.md b/doc/ci/variables/index.md
index cddff76fcd0..ebc6095c74a 100644
--- a/doc/ci/variables/index.md
+++ b/doc/ci/variables/index.md
@@ -368,6 +368,26 @@ WARNING:
 When you store credentials, there are [security implications](#cicd-variable-security).
 If you use AWS keys for example, follow the [Best practices for managing AWS access keys](https://docs.aws.amazon.com/general/latest/gr/aws-access-keys-best-practices.html).
 
+### Troubleshooting variables containing references
+
+When a variable value contains a reference indicated by `$`, it may be expanded
+which can lead to unexpected values. Use `$$` to ignore a variable name inside
+another variable:
+
+```plaintext
+SOME$$VALUE
+```
+
+Another workaround is to add a new variable set to the one that contains a
+reference:
+
+```yaml
+variables:
+  NEWVAR: $MYVAR
+script:
+  - echo $NEWVAR  # outputs SOME$VALUE
+```
+
 ## Use CI/CD variables in job scripts
 
 All CI/CD variables are set as environment variables in the job's environment.
diff --git a/lib/gitlab/database.rb b/lib/gitlab/database.rb
index 8ae68e2a8a0..70e2af69c56 100644
--- a/lib/gitlab/database.rb
+++ b/lib/gitlab/database.rb
@@ -142,6 +142,11 @@ module Gitlab
       MAX_TIMESTAMP_VALUE > timestamp ? timestamp : MAX_TIMESTAMP_VALUE.dup
     end
 
+    def self.allow_cross_joins_across_databases(url:)
+      # this method is implemented in:
+      # spec/support/database/prevent_cross_joins.rb
+    end
+
     def self.add_post_migrate_path_to_rails(force: false)
       return if ENV['SKIP_POST_DEPLOYMENT_MIGRATIONS'] && !force
 
diff --git a/locale/gitlab.pot b/locale/gitlab.pot
index ec03cf66c79..3307529c3e3 100644
--- a/locale/gitlab.pot
+++ b/locale/gitlab.pot
@@ -36379,6 +36379,9 @@ msgstr ""
 msgid "Value Stream Analytics gives an overview of how much time it takes to go from idea to production in your project."
 msgstr ""
 
+msgid "Value may contain a variable reference"
+msgstr ""
+
 msgid "Value stream"
 msgstr ""
 
@@ -36439,6 +36442,9 @@ msgstr ""
 msgid "Variable"
 msgstr ""
 
+msgid "Variable references indicated by %{codeStart}$%{codeEnd} may be expanded. If this is not what you want, consider %{docsLinkStart}using a workaround to prevent expansion%{docsLinkEnd}."
+msgstr ""
+
 msgid "Variable will be masked in job logs."
 msgstr ""
 
diff --git a/spec/frontend/ci_variable_list/components/ci_variable_modal_spec.js b/spec/frontend/ci_variable_list/components/ci_variable_modal_spec.js
index eb18147fcef..5c7404c1175 100644
--- a/spec/frontend/ci_variable_list/components/ci_variable_modal_spec.js
+++ b/spec/frontend/ci_variable_list/components/ci_variable_modal_spec.js
@@ -123,6 +123,29 @@ describe('Ci variable modal', () => {
     });
   });
 
+  describe.each`
+    value           | secret            | rendered
+    ${'value'}      | ${'secret_value'} | ${false}
+    ${'dollar$ign'} | ${'dollar$ign'}   | ${true}
+  `('Adding a new variable', ({ value, secret, rendered }) => {
+    beforeEach(() => {
+      const [variable] = mockData.mockVariables;
+      const invalidKeyVariable = {
+        ...variable,
+        key: 'key',
+        value,
+        secret_value: secret,
+      };
+      createComponent(mount);
+      store.state.variable = invalidKeyVariable;
+    });
+
+    it(`${rendered ? 'renders' : 'does not render'} the variable reference warning`, () => {
+      const warning = wrapper.find(`[data-testid='contains-variable-reference']`);
+      expect(warning.exists()).toBe(rendered);
+    });
+  });
+
   describe('Editing a variable', () => {
     beforeEach(() => {
       const [variable] = mockData.mockVariables;
diff --git a/spec/support/before_all_adapter.rb b/spec/support/before_all_adapter.rb
new file mode 100644
index 00000000000..f48e0f46e80
--- /dev/null
+++ b/spec/support/before_all_adapter.rb
@@ -0,0 +1,27 @@
+# frozen_string_literal: true
+
+class BeforeAllAdapter # rubocop:disable Gitlab/NamespacedClass
+  def self.all_connection_pools
+    ::ActiveRecord::Base.connection_handler.all_connection_pools
+  end
+
+  def self.begin_transaction
+    self.all_connection_pools.each do |connection_pool|
+      connection_pool.connection.begin_transaction(joinable: false)
+    end
+  end
+
+  def self.rollback_transaction
+    self.all_connection_pools.each do |connection_pool|
+      if connection_pool.connection.open_transactions.zero?
+        warn "!!! before_all transaction has been already rollbacked and " \
+              "could work incorrectly"
+        next
+      end
+
+      connection_pool.connection.rollback_transaction
+    end
+  end
+end
+
+TestProf::BeforeAll.adapter = ::BeforeAllAdapter
diff --git a/spec/support/database/prevent_cross_joins.rb b/spec/support/database/prevent_cross_joins.rb
new file mode 100644
index 00000000000..9fb697da722
--- /dev/null
+++ b/spec/support/database/prevent_cross_joins.rb
@@ -0,0 +1,84 @@
+# frozen_string_literal: true
+
+# This module tries to discover and prevent cross-joins across tables
+# This will forbid usage of tables between CI and main database
+# on a same query unless explicitly allowed by. This will change execution
+# from a given point to allow cross-joins. The state will be cleared
+# on a next test run.
+#
+# This method should be used to mark METHOD introducing cross-join
+# not a test using the cross-join.
+#
+# class User
+#   def ci_owned_runners
+#     ::Gitlab::Database.allow_cross_joins_across_databases!(url: link-to-issue-url)
+#
+#     ...
+#   end
+# end
+
+module Database
+  module PreventCrossJoins
+    CrossJoinAcrossUnsupportedTablesError = Class.new(StandardError)
+
+    def self.validate_cross_joins!(sql)
+      return if Thread.current[:allow_cross_joins_across_databases]
+
+      # PgQuery might fail in some cases due to limited nesting:
+      # https://github.com/pganalyze/pg_query/issues/209
+      tables = PgQuery.parse(sql).tables
+
+      unless only_ci_or_only_main?(tables)
+        raise CrossJoinAcrossUnsupportedTablesError,
+          "Unsupported cross-join across '#{tables.join(", ")}' discovered " \
+          "when executing query '#{sql}'"
+      end
+    end
+
+    # Returns true if a set includes only CI tables, or includes only non-CI tables
+    def self.only_ci_or_only_main?(tables)
+      tables.all? { |table| ci_table_name?(table) } ||
+        tables.none? { |table| ci_table_name?(table) }
+    end
+
+    def self.ci_table_name?(name)
+      ci_tables.include?(name)
+    end
+
+    def self.ci_tables
+      @@ci_tables ||= Set.new.tap do |tables| # rubocop:disable Style/ClassVars
+        tables.merge(Ci::ApplicationRecord.descendants.map(&:table_name).compact)
+
+        # It was decided that taggings/tags are best placed with CI
+        # https://gitlab.com/gitlab-org/gitlab/-/issues/333413
+        tables.add('taggings')
+        tables.add('tags')
+      end
+    end
+
+    module GitlabDatabaseMixin
+      def allow_cross_joins_across_databases(url:)
+        Thread.current[:allow_cross_joins_across_databases] = true
+        super
+      end
+    end
+  end
+end
+
+Gitlab::Database.singleton_class.prepend(
+  Database::PreventCrossJoins::GitlabDatabaseMixin)
+
+RSpec.configure do |config|
+  # TODO: remove `:prevent_cross_joins` to enable the check by default
+  config.around(:each, :prevent_cross_joins) do |example|
+    subscriber = ActiveSupport::Notifications.subscribe('sql.active_record') do |event|
+      ::Database::PreventCrossJoins.validate_cross_joins!(event.payload[:sql])
+    end
+
+    Thread.current[:allow_cross_joins_across_databases] = false
+
+    example.run
+  ensure
+    ActiveSupport::Notifications.unsubscribe(subscriber) if subscriber
+  end
+end
diff --git a/spec/support/db_cleaner.rb b/spec/support/db_cleaner.rb
index ff913ebf22b..155dc3c17d9 100644
--- a/spec/support/db_cleaner.rb
+++ b/spec/support/db_cleaner.rb
@@ -1,6 +1,10 @@
 # frozen_string_literal: true
 
 module DbCleaner
+  def all_connection_classes
+    ::ActiveRecord::Base.connection_handler.connection_pool_names.map(&:constantize)
+  end
+
   def delete_from_all_tables!(except: [])
     except << 'ar_internal_metadata'
 
@@ -12,7 +16,9 @@ module DbCleaner
   end
 
   def setup_database_cleaner
-    DatabaseCleaner[:active_record, { connection: ActiveRecord::Base }]
+    all_connection_classes.each do |connection_class|
+      DatabaseCleaner[:active_record, { connection: connection_class }]
+    end
   end
 end
 
diff --git a/spec/support_specs/database/prevent_cross_joins_spec.rb b/spec/support_specs/database/prevent_cross_joins_spec.rb
new file mode 100644
index 00000000000..dd4ed9c40b8
--- /dev/null
+++ b/spec/support_specs/database/prevent_cross_joins_spec.rb
@@ -0,0 +1,56 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Database::PreventCrossJoins do
+  context 'when running in :prevent_cross_joins scope', :prevent_cross_joins do
+    context 'when only non-CI tables are used' do
+      it 'does not raise exception' do
+        expect { main_only_query }.not_to raise_error
+      end
+    end
+
+    context 'when only CI tables are used' do
+      it 'does not raise exception' do
+        expect { ci_only_query }.not_to raise_error
+      end
+    end
+
+    context 'when CI and non-CI tables are used' do
+      it 'raises exception' do
+        expect { main_and_ci_query }.to raise_error(
+          described_class::CrossJoinAcrossUnsupportedTablesError)
+      end
+
+      context 'when allow_cross_joins_across_databases is used' do
+        it 'does not raise exception' do
+          Gitlab::Database.allow_cross_joins_across_databases(url: 'http://issue-url')
+
+          expect { main_and_ci_query }.not_to raise_error
+        end
+      end
+    end
+  end
+
+  context 'when running in a default scope' do
+    context 'when CI and non-CI tables are used' do
+      it 'does not raise exception' do
+        expect { main_and_ci_query }.not_to raise_error
+      end
+    end
+  end
+
+  private
+
+  def main_only_query
+    Issue.joins(:project).last
+  end
+
+  def ci_only_query
+    Ci::Build.joins(:pipeline).last
+  end
+
+  def main_and_ci_query
+    Ci::Build.joins(:project).last
+  end
+end