Merge branch 'security-337193' into 'master'

Highlight usage of unicode bidi characters

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

app/assets/javascripts/pages/projects/shared/permissions/components/settings_panel.vue

@@ -37,6 +37,10 @@ export default {
     securityAndComplianceLabel: s__('ProjectSettings|Security & Compliance'),
     snippetsLabel: s__('ProjectSettings|Snippets'),
     wikiLabel: s__('ProjectSettings|Wiki'),
+    pucWarningLabel: s__('ProjectSettings|Warn about Potentially Unwanted Characters'),
+    pucWarningHelpText: s__(
+      'ProjectSettings|Highlight the usage of hidden unicode characters. These have innocent uses for right-to-left languages, but can also be used in potential exploits.',
+    ),
   },
 
   components: {
@@ -178,6 +182,7 @@ export default {
       securityAndComplianceAccessLevel: featureAccessLevel.PROJECT_MEMBERS,
       operationsAccessLevel: featureAccessLevel.EVERYONE,
       containerRegistryAccessLevel: featureAccessLevel.EVERYONE,
+      warnAboutPotentiallyUnwantedCharacters: true,
       lfsEnabled: true,
       requestAccessEnabled: true,
       highlightChangesClass: false,
@@ -752,5 +757,19 @@ export default {
         }}</template>
       </gl-form-checkbox>
     </project-setting-row>
+    <project-setting-row class="gl-mb-5">
+      <input
+        :value="warnAboutPotentiallyUnwantedCharacters"
+        type="hidden"
+        name="project[project_setting_attributes][warn_about_potentially_unwanted_characters]"
+      />
+      <gl-form-checkbox
+        v-model="warnAboutPotentiallyUnwantedCharacters"
+        name="project[project_setting_attributes][warn_about_potentially_unwanted_characters]"
+      >
+        {{ $options.i18n.pucWarningLabel }}
+        <template #help>{{ $options.i18n.pucWarningHelpText }}</template>
+      </gl-form-checkbox>
+    </project-setting-row>
   </div>
 </template>

app/assets/javascripts/snippets/components/show.vue

@@ -66,7 +66,13 @@ export default {
           data-qa-selector="clone_button"
         />
       </div>
-      <snippet-blob v-for="blob in blobs" :key="blob.path" :snippet="snippet" :blob="blob" />
+      <snippet-blob
+        v-for="blob in blobs"
+        :key="blob.path"
+        :snippet="snippet"
+        :blob="blob"
+        class="project-highlight-puc"
+      />
     </template>
   </div>
 </template>

app/assets/stylesheets/framework/highlight.scss

@@ -85,3 +85,9 @@
 td.line-numbers {
   line-height: 1;
 }
+
+.project-highlight-puc .unicode-bidi::before {
+  content: '�';
+  cursor: pointer;
+  text-decoration: underline wavy $red-500;
+}

app/controllers/projects_controller.rb

@@ -409,6 +409,7 @@ class ProjectsController < Projects::ApplicationController
       show_default_award_emojis
       squash_option
       mr_default_target_self
+      warn_about_potentially_unwanted_characters
     ]
   end
 

app/helpers/projects_helper.rb

@@ -376,6 +376,12 @@ module ProjectsHelper
     }
   end
 
+  def project_classes(project)
+    return "project-highlight-puc" if project.warn_about_potentially_unwanted_characters?
+
+    ""
+  end
+
   private
 
   def tab_ability_map
@@ -532,6 +538,7 @@ module ProjectsHelper
       metricsDashboardAccessLevel: feature.metrics_dashboard_access_level,
       operationsAccessLevel: feature.operations_access_level,
       showDefaultAwardEmojis: project.show_default_award_emojis?,
+      warnAboutPotentiallyUnwantedCharacters: project.warn_about_potentially_unwanted_characters?,
       securityAndComplianceAccessLevel: project.security_and_compliance_access_level,
       containerRegistryAccessLevel: feature.container_registry_access_level
     }

app/models/project.rb

@@ -423,8 +423,8 @@ class Project < ApplicationRecord
     :container_registry_access_level, :container_registry_enabled?,
     to: :project_feature, allow_nil: true
   alias_method :container_registry_enabled, :container_registry_enabled?
-  delegate :show_default_award_emojis, :show_default_award_emojis=,
-    :show_default_award_emojis?,
+  delegate :show_default_award_emojis, :show_default_award_emojis=, :show_default_award_emojis?,
+    :warn_about_potentially_unwanted_characters, :warn_about_potentially_unwanted_characters=, :warn_about_potentially_unwanted_characters?,
     to: :project_setting, allow_nil: true
   delegate :scheduled?, :started?, :in_progress?, :failed?, :finished?,
     prefix: :import, to: :import_state, allow_nil: true

app/policies/project_policy.rb

@@ -221,6 +221,7 @@ class ProjectPolicy < BasePolicy
     enable :set_note_created_at
     enable :set_emails_disabled
     enable :set_show_default_award_emojis
+    enable :set_warn_about_potentially_unwanted_characters
   end
 
   rule { can?(:guest_access) }.policy do

app/views/layouts/project.html.haml

@@ -6,6 +6,7 @@
 - display_subscription_banner!
 - display_namespace_storage_limit_alert!
 - @left_sidebar = true
+- @content_class = [@content_class, project_classes(@project)].compact.join(" ")
 
 - content_for :project_javascripts do
   - project = @target_project || @project

db/migrate/20210929144453_add_warn_about_potentially_unwanted_characters_to_project_settings.rb

+# frozen_string_literal: true
+
+# See https://docs.gitlab.com/ee/development/migration_style_guide.html
+# for more information on how to write migrations for GitLab.
+
+class AddWarnAboutPotentiallyUnwantedCharactersToProjectSettings < Gitlab::Database::Migration[1.0]
+  enable_lock_retries!
+
+  def up
+    add_column :project_settings, :warn_about_potentially_unwanted_characters, :boolean, null: false, default: true
+  end
+
+  def down
+    remove_column :project_settings, :warn_about_potentially_unwanted_characters
+  end
+end

db/schema_migrations/20210929144453

+0f808c27d19e6a38d4aa31f2dd820fe226681af84e05c4af47213409b2043e5a
\ No newline at end of file

db/structure.sql

@@ -18186,6 +18186,7 @@ CREATE TABLE project_settings (
     cve_id_request_enabled boolean DEFAULT true NOT NULL,
     mr_default_target_self boolean DEFAULT false NOT NULL,
     previous_default_branch text,
+    warn_about_potentially_unwanted_characters boolean DEFAULT true NOT NULL,
     CONSTRAINT check_3a03e7557a CHECK ((char_length(previous_default_branch) <= 4096)),
     CONSTRAINT check_bde223416c CHECK ((show_default_award_emojis IS NOT NULL))
 );

lib/api/helpers/projects_helpers.rb

@@ -39,6 +39,7 @@ module API
 
         optional :emails_disabled, type: Boolean, desc: 'Disable email notifications'
         optional :show_default_award_emojis, type: Boolean, desc: 'Show default award emojis'
+        optional :warn_about_potentially_unwanted_characters, type: Boolean, desc: 'Warn about Potentially Unwanted Characters'
         optional :shared_runners_enabled, type: Boolean, desc: 'Flag indication if shared runners are enabled for that project'
         optional :resolve_outdated_diff_discussions, type: Boolean, desc: 'Automatically resolve merge request diffs discussions on lines changed with a push'
         optional :remove_source_branch_after_merge, type: Boolean, desc: 'Remove the source branch by default after merge'

lib/gitlab/import_export/project/import_export.yml

@@ -368,6 +368,7 @@ excluded_attributes:
     - :marked_for_deletion_by_user_id
     - :compliance_framework_setting
     - :show_default_award_emojis
+    - :warn_about_potentially_unwanted_characters
     - :services
     - :exported_protected_branches
     - :repository_size_limit

lib/gitlab/unicode.rb

+# frozen_string_literal: true
+
+module Gitlab
+  class Unicode
+    # Regular expression for identifying bidirectional control
+    # characters in UTF-8 strings
+    #
+    # Documentation on how this works:
+    # https://idiosyncratic-ruby.com/41-proper-unicoding.html
+    BIDI_REGEXP = /\p{Bidi Control}/.freeze
+
+    class << self
+      # Warning message used to highlight bidi characters in the GUI
+      def bidi_warning
+        _("Potentially unwanted character detected: Unicode BiDi Control")
+      end
+    end
+  end
+end

lib/rouge/formatters/html_gitlab.rb

@@ -21,12 +21,24 @@ module Rouge
           is_first = false
 
           yield %(<span id="LC#{@line_number}" class="line" lang="#{@tag}">)
-          line.each { |token, value| yield span(token, value.chomp! || value) }
+
+          line.each do |token, value|
+            yield highlight_unicode_control_characters(span(token, value.chomp! || value))
+          end
+
           yield %(</span>)
 
           @line_number += 1
         end
       end
+
+      private
+
+      def highlight_unicode_control_characters(text)
+        text.gsub(Gitlab::Unicode::BIDI_REGEXP) do |char|
+          %(<span class="unicode-bidi has-tooltip" data-toggle="tooltip" title="#{Gitlab::Unicode.bidi_warning}">#{char}</span>)
+        end
+      end
     end
   end
 end

locale/gitlab.pot

@@ -25798,6 +25798,9 @@ msgstr ""
 msgid "Postman collection file path or URL"
 msgstr ""
 
+msgid "Potentially unwanted character detected: Unicode BiDi Control"
+msgstr ""
+
 msgid "Pre-defined push rules."
 msgstr ""
 
@@ -26923,6 +26926,9 @@ msgstr ""
 msgid "ProjectSettings|Global"
 msgstr ""
 
+msgid "ProjectSettings|Highlight the usage of hidden unicode characters. These have innocent uses for right-to-left languages, but can also be used in potential exploits."
+msgstr ""
+
 msgid "ProjectSettings|Internal"
 msgstr ""
 
@@ -27112,6 +27118,9 @@ msgstr ""
 msgid "ProjectSettings|Visualize the project's performance metrics."
 msgstr ""
 
+msgid "ProjectSettings|Warn about Potentially Unwanted Characters"
+msgstr ""
+
 msgid "ProjectSettings|What are badges?"
 msgstr ""
 

spec/controllers/projects_controller_spec.rb

@@ -323,6 +323,34 @@ RSpec.describe ProjectsController do
         expect(response).to render_template('_files')
         expect(response.body).to have_content('LICENSE') # would be 'MIT license' if stub not works
       end
+
+      describe "PUC highlighting" do
+        render_views
+
+        before do
+          expect(controller).to receive(:find_routable!).and_return(public_project)
+        end
+
+        context "option is enabled" do
+          it "adds the highlighting class" do
+            expect(public_project).to receive(:warn_about_potentially_unwanted_characters?).and_return(true)
+
+            get_show
+
+            expect(response.body).to have_css(".project-highlight-puc")
+          end
+        end
+
+        context "option is disabled" do
+          it "doesn't add the highlighting class" do
+            expect(public_project).to receive(:warn_about_potentially_unwanted_characters?).and_return(false)
+
+            get_show
+
+            expect(response.body).not_to have_css(".project-highlight-puc")
+          end
+        end
+      end
     end
 
     context "when the url contains .atom" do

spec/frontend/pages/projects/shared/permissions/components/settings_panel_spec.js

@@ -27,6 +27,7 @@ const defaultProps = {
     emailsDisabled: false,
     packagesEnabled: true,
     showDefaultAwardEmojis: true,
+    warnAboutPotentiallyUnwantedCharacters: true,
   },
   isGitlabCom: true,
   canDisableEmails: true,
@@ -97,6 +98,10 @@ describe('Settings Panel', () => {
   const findEmailSettings = () => wrapper.find({ ref: 'email-settings' });
   const findShowDefaultAwardEmojis = () =>
     wrapper.find('input[name="project[project_setting_attributes][show_default_award_emojis]"]');
+  const findWarnAboutPuc = () =>
+    wrapper.find(
+      'input[name="project[project_setting_attributes][warn_about_potentially_unwanted_characters]"]',
+    );
   const findMetricsVisibilitySettings = () => wrapper.find({ ref: 'metrics-visibility-settings' });
   const findOperationsSettings = () => wrapper.find({ ref: 'operations-settings' });
 
@@ -539,6 +544,14 @@ describe('Settings Panel', () => {
     });
   });
 
+  describe('Warn about potentially unwanted characters', () => {
+    it('should have a "Warn about Potentially Unwanted Characters" input', () => {
+      wrapper = mountComponent();
+
+      expect(findWarnAboutPuc().exists()).toBe(true);
+    });
+  });
+
   describe('Metrics dashboard', () => {
     it('should show the metrics dashboard access toggle', () => {
       wrapper = mountComponent();

spec/helpers/projects_helper_spec.rb

@@ -961,4 +961,26 @@ RSpec.describe ProjectsHelper do
       )
     end
   end
+
+  describe '#project_classes' do
+    subject { helper.project_classes(project) }
+
+    it { is_expected.to be_a(String) }
+
+    context 'PUC highlighting enabled' do
+      before do
+        project.warn_about_potentially_unwanted_characters = true
+      end
+
+      it { is_expected.to include('project-highlight-puc') }
+    end
+
+    context 'PUC highlighting disabled' do
+      before do
+        project.warn_about_potentially_unwanted_characters = false
+      end
+
+      it { is_expected.not_to include('project-highlight-puc') }
+    end
+  end
 end

spec/lib/gitlab/unicode_spec.rb

+# frozen_string_literal: true
+
+require "spec_helper"
+
+RSpec.describe Gitlab::Unicode do
+  describe described_class::BIDI_REGEXP do
+    using RSpec::Parameterized::TableSyntax
+
+    where(:bidi_string, :match) do
+      "\u2066"       | true # left-to-right isolate
+      "\u2067"       | true # right-to-left isolate
+      "\u2068"       | true # first strong isolate
+      "\u2069"       | true # pop directional isolate
+      "\u202a"       | true # left-to-right embedding
+      "\u202b"       | true # right-to-left embedding
+      "\u202c"       | true # pop directional formatting
+      "\u202d"       | true # left-to-right override
+      "\u202e"       | true # right-to-left override
+      "\u2066foobar" | true
+      ""             | false
+      "foo"          | false
+      "\u2713"       | false # checkmark
+    end
+
+    with_them do
+      let(:utf8_string) { bidi_string.encode("utf-8") }
+
+      it "matches only the bidi characters" do
+        expect(utf8_string.match?(subject)).to eq(match)
+      end
+    end
+  end
+end

spec/lib/rouge/formatters/html_gitlab_spec.rb

@@ -36,5 +36,26 @@ RSpec.describe Rouge::Formatters::HTMLGitlab do
         is_expected.to eq(code)
       end
     end
+
+    context 'when unicode control characters are used' do
+      let(:lang) { 'javascript' }
+      let(:tokens) { lexer.lex(code, continue: false) }
+      let(:code) do
+        <<~JS
+          #!/usr/bin/env node
+
+          var accessLevel = "user";
+          if (accessLevel != "user‮ ⁦// Check if admin⁩ ⁦") {
+              console.log("You are an admin.");
+          }
+        JS
+      end
+
+      it 'highlights the control characters' do
+        message = "Potentially unwanted character detected: Unicode BiDi Control"
+
+        is_expected.to include(%{<span class="unicode-bidi has-tooltip" data-toggle="tooltip" title="#{message}">}).exactly(4).times
+      end
+    end
   end
 end

spec/models/project_spec.rb

@@ -681,6 +681,19 @@ RSpec.describe Project, factory_default: :keep do
     it { is_expected.to delegate_method(:container_registry_enabled?).to(:project_feature) }
     it { is_expected.to delegate_method(:container_registry_access_level).to(:project_feature) }
 
+    describe 'project settings' do
+      %i(
+        show_default_award_emojis
+        show_default_award_emojis=
+        show_default_award_emojis?
+        warn_about_potentially_unwanted_characters
+        warn_about_potentially_unwanted_characters=
+        warn_about_potentially_unwanted_characters?
+      ).each do |method|
+        it { is_expected.to delegate_method(method).to(:project_setting).with_arguments(allow_nil: true) }
+      end
+    end
+
     include_examples 'ci_cd_settings delegation' do
       # Skip attributes defined in EE code
       let(:exclude_attributes) do

spec/requests/api/project_attributes.yml

@@ -139,6 +139,7 @@ project_setting:
     - has_confluence
     - has_vulnerabilities
     - prevent_merge_without_jira_issue
+    - warn_about_potentially_unwanted_characters
     - previous_default_branch
     - project_id
     - push_rule_id