Prevent code injection in Product Analytics funnels YAML

Merge branch 'security-prevent-code-injection-in-pa-funnels-17-1-17-1' into '17-1-stable-ee'

See merge request gitlab-org/security/gitlab!4429

Changelog: security

ee/app/models/product_analytics/funnel.rb

@@ -8,8 +8,11 @@ class Funnel
     FUNNELS_ROOT_LOCATION = '.gitlab/analytics/funnels'
+    # This model is not used as a true ActiveRecord
+    # You must run .valid? wherever this model is used for these validations to be run
     validates :name, presence: true
     validates :seconds_to_convert, numericality: { only_integer: true, greater_than: 0 }
+    validate :check_steps_validity
     def self.from_diff(diff, project:, sha: nil, commit: nil)
       config_project = project.analytics_dashboards_configuration_project || project
@@ -98,7 +101,13 @@ def initialize(name:, project:, seconds_to_convert:, config_path:, config_projec
       @previous_name = previous_name
     end
+    def check_steps_validity
+      errors.add(:base, "Invalid steps") unless steps.all?(&:valid?)
+    end
+
     def to_h
+      return unless valid?
+
       {
         name: name,
         schema: to_sql,
@@ -126,6 +135,8 @@ def steps
     end
     def to_sql
+      return unless valid?
+
       <<-SQL
         SELECT
           (SELECT max(derived_tstamp) FROM gitlab_project_#{project.id}.snowplow_events) as x,

ee/app/models/product_analytics/funnel_step.rb

@@ -4,9 +4,13 @@ module ProductAnalytics
   class FunnelStep
     include ActiveModel::Validations
-    attr_reader :name, :target, :action
-    validates! :action, inclusion: { in: %w[pageview] }
+    attr_accessor :name, :target, :action
+    # This model is not used as a true ActiveRecord
+    # You must run .valid? wherever this model is used for these validations to be run
+    validates :action, inclusion: { in: %w[pageview] }
+    validates :name, format: { with: /\A[\w\-]+\z/ }
+    validates :target, format: { with: %r{\A[\w\-./]+\z} }
     def initialize(name:, target:, action:, funnel:)
       @name = name

ee/app/workers/product_analytics/move_funnels_worker.rb

@@ -56,6 +56,8 @@ def funnels
       end
       funnels_to_create.each do |funnel|
+        next unless funnel.valid?
+
         funnels_to_send << {
           state: 'created',
           name: funnel.name,

ee/app/workers/product_analytics/sync_funnels_worker.rb

@@ -39,8 +39,10 @@ def funnel_files
     end
     def new_funnels
-      funnel_files.select(&:new_file).map do |file|
+      funnel_files.select(&:new_file).filter_map do |file|
         funnel = ProductAnalytics::Funnel.from_diff(file, project: @project)
+        next unless funnel.valid?
+
         {
           state: 'created',
           name: funnel.name,
@@ -51,8 +53,10 @@ def new_funnels
     def updated_funnels
       # if a file is not new or deleted, but is in a diff, we assume it is changed.
-      funnel_files.select { |f| !f.new_file && !f.deleted_file }.map do |file|
+      funnel_files.select { |f| !f.new_file && !f.deleted_file }.filter_map do |file|
         funnel = ProductAnalytics::Funnel.from_diff(file, project: @project, commit: @commit)
+        next unless funnel.valid?
+
         o = {
           state: 'updated',
           name: funnel.name,

ee/spec/factories/projects.rb

@@ -207,6 +207,48 @@
       end
     end
+    trait :with_invalid_seconds_product_analytics_funnel do
+      repository
+
+      after(:create) do |project|
+        project.repository.create_file(
+          project.creator,
+          '.gitlab/analytics/funnels/funnel_example_invalid_seconds.yaml',
+          File.open(Rails.root.join('ee/spec/fixtures/product_analytics/funnel_example_invalid_seconds.yaml')).read,
+          message: 'Add invalid seconds funnel definition',
+          branch_name: 'master'
+        )
+      end
+    end
+
+    trait :with_invalid_step_name_product_analytics_funnel do
+      repository
+
+      after(:create) do |project|
+        project.repository.create_file(
+          project.creator,
+          '.gitlab/analytics/funnels/funnel_example_invalid_step_name.yaml',
+          File.open(Rails.root.join('ee/spec/fixtures/product_analytics/funnel_example_invalid_step_name.yaml')).read,
+          message: 'Add invalid step name funnel definition',
+          branch_name: 'master'
+        )
+      end
+    end
+
+    trait :with_invalid_step_target_product_analytics_funnel do
+      repository
+
+      after(:create) do |project|
+        project.repository.create_file(
+          project.creator,
+          '.gitlab/analytics/funnels/funnel_example_invalid_step_target.yaml',
+          File.open(Rails.root.join('ee/spec/fixtures/product_analytics/funnel_example_invalid_step_target.yaml')).read,
+          message: 'Add invalid step target funnel definition',
+          branch_name: 'master'
+        )
+      end
+    end
+
     trait(:allow_pipeline_trigger_approve_deployment) { allow_pipeline_trigger_approve_deployment { true } }
     trait :verification_succeeded do

ee/spec/fixtures/product_analytics/funnel_example_invalid_seconds.yaml

+seconds_to_convert: 'invalid string'
+steps:
+  - name: view_page_1
+    target: '/page1.html'
+    action: 'pageview'
+  - name: view_page_2
+    target: '/page2.html'
+    action: 'pageview'

ee/spec/fixtures/product_analytics/funnel_example_invalid_step_name.yaml

+seconds_to_convert: 3600
+steps:
+  - name: '$step_name{}'
+    target: '/page1.html'
+    action: 'pageview'
+  - name: view_page_2
+    target: '/page2.html'
+    action: 'pageview'

ee/spec/fixtures/product_analytics/funnel_example_invalid_step_target.yaml

+seconds_to_convert: 3600
+steps:
+  - name: view_page_1
+    target: '$step_name{}'
+    action: 'pageview'
+  - name: view_page_2
+    target: '/page2.html'
+    action: 'pageview'

ee/spec/models/product_analytics/funnel_spec.rb

@@ -5,6 +5,42 @@
 RSpec.describe ProductAnalytics::Funnel, feature_category: :product_analytics_data_management do
   let_it_be(:group) { create(:group) }
   let_it_be(:project) { create(:project, :with_product_analytics_funnel, group: group) }
+  let_it_be(:project_invalid_seconds) { create(:project, :with_invalid_seconds_product_analytics_funnel, group: group) }
+
+  let_it_be(:project_invalid_step_name) do
+    create(:project, :with_invalid_step_name_product_analytics_funnel, group: group)
+  end
+
+  let_it_be(:project_invalid_step_target) do
+    create(:project, :with_invalid_step_target_product_analytics_funnel, group: group)
+  end
+
+  let(:query) do
+    <<-SQL
+        SELECT
+          (SELECT max(derived_tstamp) FROM gitlab_project_#{project.id}.snowplow_events) as x,
+          arrayJoin(range(1, 3)) AS level,
+          sumIf(c, user_level >= level) AS count
+        FROM
+          (SELECT
+             level AS user_level,
+             count(*) AS c
+           FROM (
+               SELECT
+                 user_id,
+                 windowFunnel(3600, 'strict_order')(toDateTime(derived_tstamp),
+                    page_urlpath = '/page1.html', page_urlpath = '/page2.html'
+                 ) AS level
+               FROM gitlab_project_#{project.id}.snowplow_events
+               WHERE ${FILTER_PARAMS.funnel_example_1.date.filter('derived_tstamp')}
+               GROUP BY user_id
+               )
+           GROUP BY level
+          )
+          GROUP BY level
+	        ORDER BY level ASC
+    SQL
+  end
   before do
     allow(Gitlab::CurrentSettings).to receive(:product_analytics_enabled?).and_return(true)
@@ -16,6 +52,24 @@
   it { is_expected.to validate_numericality_of(:seconds_to_convert) }
+  context 'when the funnel has invalid seconds' do
+    subject(:funnel) { project_invalid_seconds.product_analytics_funnels.first }
+
+    it { is_expected.to be_invalid }
+  end
+
+  context 'when the funnel has invalid step name' do
+    subject(:funnel) { project_invalid_step_name.product_analytics_funnels.first }
+
+    it { is_expected.to be_invalid }
+  end
+
+  context 'when the funnel has invalid step target' do
+    subject(:funnel) { project_invalid_step_target.product_analytics_funnels.first }
+
+    it { is_expected.to be_invalid }
+  end
+
   describe '.for_project' do
     subject(:funnels) { described_class.for_project(project) }
@@ -139,37 +193,92 @@
     end
   end
+  describe '#to_h' do
+    subject { project.product_analytics_funnels.first.to_h }
+
+    let(:object) do
+      {
+        name: 'funnel_example_1',
+        schema: query,
+        steps: ["page_urlpath = '/page1.html'", "page_urlpath = '/page2.html'"]
+      }
+    end
+
+    it { is_expected.to eq(object) }
+
+    context 'when the funnel has invalid seconds' do
+      subject { project_invalid_seconds.product_analytics_funnels.first.to_h }
+
+      it { is_expected.to eq(nil) }
+    end
+
+    context 'when the funnel has invalid step name' do
+      subject { project_invalid_step_name.product_analytics_funnels.first.to_h }
+
+      it { is_expected.to eq(nil) }
+    end
+
+    context 'when the funnel has invalid step target' do
+      subject { project_invalid_step_target.product_analytics_funnels.first.to_h }
+
+      it { is_expected.to eq(nil) }
+    end
+  end
+
+  describe '#to_json' do
+    subject { project.product_analytics_funnels.first.to_json }
+
+    let(:object) do
+      {
+        name: 'funnel_example_1',
+        schema: query,
+        steps: ["page_urlpath = '/page1.html'", "page_urlpath = '/page2.html'"]
+      }.to_json
+    end
+
+    it { is_expected.to eq(object) }
+
+    context 'when the funnel has invalid seconds' do
+      subject { project_invalid_seconds.product_analytics_funnels.first.to_json }
+
+      it { is_expected.to eq('null') }
+    end
+
+    context 'when the funnel has invalid step name' do
+      subject { project_invalid_step_name.product_analytics_funnels.first.to_json }
+
+      it { is_expected.to eq('null') }
+    end
+
+    context 'when the funnel has invalid step target' do
+      subject { project_invalid_step_target.product_analytics_funnels.first.to_json }
+
+      it { is_expected.to eq('null') }
+    end
+  end
+
   describe '#to_sql' do
     subject { project.product_analytics_funnels.first.to_sql }
-    let(:query) do
-      <<-SQL
-        SELECT
-          (SELECT max(derived_tstamp) FROM gitlab_project_#{project.id}.snowplow_events) as x,
-          arrayJoin(range(1, 3)) AS level,
-          sumIf(c, user_level >= level) AS count
-        FROM
-          (SELECT
-             level AS user_level,
-             count(*) AS c
-           FROM (
-               SELECT
-                 user_id,
-                 windowFunnel(3600, 'strict_order')(toDateTime(derived_tstamp),
-                    page_urlpath = '/page1.html', page_urlpath = '/page2.html'
-                 ) AS level
-               FROM gitlab_project_#{project.id}.snowplow_events
-               WHERE ${FILTER_PARAMS.funnel_example_1.date.filter('derived_tstamp')}
-               GROUP BY user_id
-               )
-           GROUP BY level
-          )
-          GROUP BY level
-	        ORDER BY level ASC
-      SQL
+    it { is_expected.to eq(query) }
+
+    context 'when the funnel has invalid seconds' do
+      subject { project_invalid_seconds.product_analytics_funnels.first.to_sql }
+
+      it { is_expected.to eq(nil) }
     end
-    it { is_expected.to eq(query) }
+    context 'when the funnel has invalid step name' do
+      subject { project_invalid_step_name.product_analytics_funnels.first.to_sql }
+
+      it { is_expected.to eq(nil) }
+    end
+
+    context 'when the funnel has invalid step target' do
+      subject { project_invalid_step_target.product_analytics_funnels.first.to_sql }
+
+      it { is_expected.to eq(nil) }
+    end
   end
   private

ee/spec/models/product_analytics/funnel_step_spec.rb

@@ -13,7 +13,14 @@
     )
   end
-  let(:funnel_step) { described_class.new(name: 'test', target: '/page1.html', action: 'pageview', funnel: funnel) }
+  subject(:funnel_step) { described_class.new(name: 'test', target: '/page1.html', action: 'pageview', funnel: funnel) }
+
+  it { is_expected.to validate_inclusion_of(:action).in_array(%w[pageview]) }
+  it { is_expected.not_to allow_value('test').for(:action) }
+  it { is_expected.to allow_value('Test_name-format01').for(:name) }
+  it { is_expected.not_to allow_value('${(test(){})}').for(:name) }
+  it { is_expected.to allow_value('section/subsection/page.html').for(:target) }
+  it { is_expected.not_to allow_value('${(test(){})}').for(:target) }
   describe '#initialize' do
     it 'has a name' do
@@ -27,16 +34,6 @@
     it 'has an action' do
       expect(funnel_step.action).to eq('pageview')
     end
-
-    context 'when action is not a valid type' do
-      let(:funnel_step) do
-        described_class.new(name: 'test', target: '/page1.html', action: 'invalid', funnel: funnel).valid?
-      end
-
-      it 'raises an error' do
-        expect { funnel_step }.to raise_error(ActiveModel::StrictValidationFailed)
-      end
-    end
   end
   describe '#to_h' do

ee/spec/workers/product_analytics/move_funnels_worker_spec.rb

@@ -21,6 +21,20 @@
       message: 'Add funnel',
       branch_name: 'master'
     )
+    previous_custom_dashboard_project.repository.create_file(
+      project.creator,
+      '.gitlab/analytics/funnels/funnel_example_invalid_seconds.yml',
+      File.open(Rails.root.join('ee/spec/fixtures/product_analytics/funnel_example_invalid_seconds.yaml')),
+      message: 'Add invalid seconds funnel definition',
+      branch_name: 'master'
+    )
+    previous_custom_dashboard_project.repository.create_file(
+      project.creator,
+      '.gitlab/analytics/funnels/funnel_example_invalid_step.yml',
+      File.open(Rails.root.join('ee/spec/fixtures/product_analytics/funnel_example_invalid_step_name.yaml')),
+      message: 'Add invalid step name funnel definition',
+      branch_name: 'master'
+    )
     new_custom_dashboard_project.repository.create_file(
       project.creator,
       '.gitlab/analytics/funnels/example1.yml',
@@ -28,6 +42,20 @@
       message: 'Add funnel',
       branch_name: 'master'
     )
+    new_custom_dashboard_project.repository.create_file(
+      project.creator,
+      '.gitlab/analytics/funnels/funnel_example_invalid_seconds.yml',
+      File.open(Rails.root.join('ee/spec/fixtures/product_analytics/funnel_example_invalid_seconds.yaml')),
+      message: 'Add invalid seconds funnel definition',
+      branch_name: 'master'
+    )
+    new_custom_dashboard_project.repository.create_file(
+      project.creator,
+      '.gitlab/analytics/funnels/funnel_example_invalid_step.yml',
+      File.open(Rails.root.join('ee/spec/fixtures/product_analytics/funnel_example_invalid_step_name.yaml')),
+      message: 'Add invalid step name funnel definition',
+      branch_name: 'master'
+    )
   end
   describe "perform" do
@@ -66,9 +94,11 @@
           expect(payload).to match(
             a_hash_including(
               project_ids: ["gitlab_project_#{project.id}"],
-              funnels: [a_hash_including(
-                state: "deleted"
-              )]
+              funnels: [
+                { name: "example1", state: "deleted" },
+                { name: "funnel_example_invalid_seconds", state: "deleted" },
+                { name: "funnel_example_invalid_step", state: "deleted" }
+              ]
             )
           )
         end
@@ -90,11 +120,15 @@
           expect(payload).to match(
             a_hash_including(
               project_ids: ["gitlab_project_#{project.id}"],
-              funnels: [a_hash_including(
-                state: "deleted"
-              ), a_hash_including(
-                state: "created"
-              )]
+              funnels: [
+                { name: "example1", state: "deleted" },
+                { name: "funnel_example_invalid_seconds", state: "deleted" },
+                { name: "funnel_example_invalid_step", state: "deleted" },
+                a_hash_including(
+                  name: "example1",
+                  state: "created"
+                )
+              ]
             )
           )
         end

ee/spec/workers/product_analytics/sync_funnels_worker_spec.rb

@@ -11,7 +11,11 @@
         create_valid_funnel
       end
-      it 'is successful', quarantine: 'https://gitlab.com/gitlab-org/gitlab/-/issues/469177' do
+      after do
+        delete_funnel("example1.yml")
+      end
+
+      it 'is successful' do
         url_to_projects_regex.each do |url, projects_regex|
           expect(Gitlab::HTTP).to receive(:post)
                                     .with(URI.parse(url.to_s), {
@@ -23,14 +27,35 @@
         worker
       end
+
+      context 'when the new funnel is invalid' do
+        before do
+          create_invalid_funnel
+        end
+
+        after do
+          delete_funnel("funnel_example_invalid_step.yml")
+        end
+
+        it 'does not attempt to post to the API' do
+          expect(Gitlab::HTTP).not_to receive(:post)
+
+          worker
+        end
+      end
     end
     context 'when an updated funnel is in the commit' do
       before do
+        create_valid_funnel
         update_funnel
       end
-      it 'is successful', quarantine: 'https://gitlab.com/gitlab-org/gitlab/-/issues/469178' do
+      after do
+        delete_funnel("example1.yml")
+      end
+
+      it 'is successful' do
         url_to_projects_regex.each do |url, projects_regex|
           expect(Gitlab::HTTP).to receive(:post)
                                     .with(URI.parse(url.to_s), {
@@ -42,14 +67,36 @@
         worker
       end
+
+      context 'when the updated funnel is invalid' do
+        before do
+          create_invalid_funnel
+          update_invalid_funnel
+        end
+
+        after do
+          delete_funnel("funnel_example_invalid_step.yml")
+        end
+
+        it 'does not attempt to post to the API' do
+          expect(Gitlab::HTTP).not_to receive(:post)
+
+          worker
+        end
+      end
     end
     context 'when an renamed funnel is in the commit' do
       before do
-        rename_funnel
+        create_valid_funnel
+        rename_valid_funnel
+      end
+
+      after do
+        delete_funnel("example2.yml")
       end
-      it 'is successful', quarantine: 'https://gitlab.com/gitlab-org/gitlab/-/issues/469456' do
+      it 'is successful' do
         url_to_projects_regex.each do |url, _projects_regex|
           expect(Gitlab::HTTP).to receive(:post)
                                     .with(URI.parse(url.to_s), {
@@ -61,14 +108,32 @@
         worker
       end
+
+      context 'when the renamed funnel is invalid' do
+        before do
+          create_invalid_funnel
+          rename_invalid_funnel
+        end
+
+        after do
+          delete_funnel("funnel_example_invalid_seconds.yml")
+        end
+
+        it 'does not attempt to post to the API' do
+          expect(Gitlab::HTTP).not_to receive(:post)
+
+          worker
+        end
+      end
     end
     context 'when a deleted funnel is in the commit' do
       before do
-        delete_funnel
+        create_valid_funnel
+        delete_funnel("example1.yml")
       end
-      it 'is successful', quarantine: 'https://gitlab.com/gitlab-org/gitlab/-/issues/469176' do
+      it 'is successful' do
         url_to_projects_regex.each do |url, projects_regex|
           expect(Gitlab::HTTP).to receive(:post)
                                     .with(URI.parse(url.to_s), {
@@ -80,6 +145,26 @@
         worker
       end
+
+      context 'when the deleted funnel is invalid' do
+        before do
+          create_invalid_funnel
+          delete_funnel("funnel_example_invalid_step.yml")
+        end
+
+        it 'is successful' do
+          url_to_projects_regex.each do |url, projects_regex|
+            expect(Gitlab::HTTP).to receive(:post)
+                                      .with(URI.parse(url.to_s), {
+                                        allow_local_requests: true,
+                                        body: Regexp.new(projects_regex.source + /.*\"state\":\"deleted\"/.source)
+                                      }).once
+                                      .and_return(instance_double("HTTParty::Response", body: { result: 'success' }))
+          end
+
+          worker
+        end
+      end
     end
     context 'when no new or updated funnels are in the commit' do
@@ -258,7 +343,17 @@ def create_valid_funnel
     )
   end
-  def rename_funnel
+  def create_invalid_funnel
+    project.repository.create_file(
+      project.creator,
+      '.gitlab/analytics/funnels/funnel_example_invalid_step.yml',
+      File.read(Rails.root.join('ee/spec/fixtures/product_analytics/funnel_example_invalid_step_name.yaml')),
+      message: 'Add invalid funnel',
+      branch_name: 'master'
+    )
+  end
+
+  def rename_valid_funnel
     project.repository.update_file(
       project.creator,
       '.gitlab/analytics/funnels/example2.yml',
@@ -269,6 +364,17 @@ def rename_funnel
     )
   end
+  def rename_invalid_funnel
+    project.repository.update_file(
+      project.creator,
+      '.gitlab/analytics/funnels/funnel_example_invalid_seconds.yml',
+      File.read(Rails.root.join('ee/spec/fixtures/product_analytics/funnel_example_invalid_seconds.yaml')),
+      message: 'Rename invalid funnel',
+      branch_name: 'master',
+      previous_path: '.gitlab/analytics/funnels/funnel_example_invalid_step.yml'
+    )
+  end
+
   def update_funnel
     project.repository.update_file(
       project.creator,
@@ -279,10 +385,20 @@ def update_funnel
     )
   end
-  def delete_funnel
+  def update_invalid_funnel
+    project.repository.update_file(
+      project.creator,
+      '.gitlab/analytics/funnels/funnel_example_invalid_step.yml',
+      File.read(Rails.root.join('ee/spec/fixtures/product_analytics/funnel_example_invalid_step_name.yaml')),
+      message: 'Update invalid funnel',
+      branch_name: 'master'
+    )
+  end
+
+  def delete_funnel(filename)
     project.repository.delete_file(
       project.creator,
-      '.gitlab/analytics/funnels/example2.yml',
+      ".gitlab/analytics/funnels/#{filename}",
       message: 'delete funnel',
       branch_name: 'master'
     )