From c1c828ac7f7b3c2e51d81921bbef9d474cd4d0a4 Mon Sep 17 00:00:00 2001 From: GitLab Bot Date: Wed, 27 Oct 2021 12:41:41 +0000 Subject: Add latest changes from gitlab-org/security/gitlab@14-4-stable-ee --- .../permissions/components/settings_panel.vue | 19 +++++++++++++ .../javascripts/snippets/components/show.vue | 8 +++++- app/assets/stylesheets/framework/highlight.scss | 6 ++++ app/controllers/projects_controller.rb | 1 + app/helpers/projects_helper.rb | 7 +++++ app/models/project.rb | 4 +-- app/policies/project_policy.rb | 1 + app/views/layouts/project.html.haml | 1 + ...ally_unwanted_characters_to_project_settings.rb | 16 +++++++++++ db/schema_migrations/20210929144453 | 1 + db/structure.sql | 1 + lib/api/helpers/projects_helpers.rb | 1 + lib/gitlab/import_export/project/import_export.yml | 1 + lib/gitlab/unicode.rb | 19 +++++++++++++ lib/rouge/formatters/html_gitlab.rb | 14 ++++++++- locale/gitlab.pot | 9 ++++++ spec/controllers/projects_controller_spec.rb | 28 ++++++++++++++++++ .../permissions/components/settings_panel_spec.js | 13 +++++++++ spec/helpers/projects_helper_spec.rb | 22 +++++++++++++++ spec/lib/gitlab/unicode_spec.rb | 33 ++++++++++++++++++++++ spec/lib/rouge/formatters/html_gitlab_spec.rb | 21 ++++++++++++++ spec/models/project_spec.rb | 13 +++++++++ spec/requests/api/project_attributes.yml | 1 + 23 files changed, 236 insertions(+), 4 deletions(-) create mode 100644 db/migrate/20210929144453_add_warn_about_potentially_unwanted_characters_to_project_settings.rb create mode 100644 db/schema_migrations/20210929144453 create mode 100644 lib/gitlab/unicode.rb create mode 100644 spec/lib/gitlab/unicode_spec.rb diff --git a/app/assets/javascripts/pages/projects/shared/permissions/components/settings_panel.vue b/app/assets/javascripts/pages/projects/shared/permissions/components/settings_panel.vue index 261f7af7ef1..c53d367ed71 100644 --- a/app/assets/javascripts/pages/projects/shared/permissions/components/settings_panel.vue +++ b/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 { }} + + + + {{ $options.i18n.pucWarningLabel }} + + + diff --git a/app/assets/javascripts/snippets/components/show.vue b/app/assets/javascripts/snippets/components/show.vue index 46629a569ec..35d88d5ec8e 100644 --- a/app/assets/javascripts/snippets/components/show.vue +++ b/app/assets/javascripts/snippets/components/show.vue @@ -66,7 +66,13 @@ export default { data-qa-selector="clone_button" /> - + diff --git a/app/assets/stylesheets/framework/highlight.scss b/app/assets/stylesheets/framework/highlight.scss index b4a1d9f9977..122c605e603 100644 --- a/app/assets/stylesheets/framework/highlight.scss +++ b/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; +} diff --git a/app/controllers/projects_controller.rb b/app/controllers/projects_controller.rb index 26da0436dd8..0760f97d7c1 100644 --- a/app/controllers/projects_controller.rb +++ b/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 diff --git a/app/helpers/projects_helper.rb b/app/helpers/projects_helper.rb index 03e7fb5ffc4..e3b63d122d2 100644 --- a/app/helpers/projects_helper.rb +++ b/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 } diff --git a/app/models/project.rb b/app/models/project.rb index 00a572b775d..2ceba10e86e 100644 --- a/app/models/project.rb +++ b/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 diff --git a/app/policies/project_policy.rb b/app/policies/project_policy.rb index 59aa47beff9..87573c9ad13 100644 --- a/app/policies/project_policy.rb +++ b/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 diff --git a/app/views/layouts/project.html.haml b/app/views/layouts/project.html.haml index 2df502d2899..a54e0351d2f 100644 --- a/app/views/layouts/project.html.haml +++ b/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 diff --git a/db/migrate/20210929144453_add_warn_about_potentially_unwanted_characters_to_project_settings.rb b/db/migrate/20210929144453_add_warn_about_potentially_unwanted_characters_to_project_settings.rb new file mode 100644 index 00000000000..166afa13371 --- /dev/null +++ b/db/migrate/20210929144453_add_warn_about_potentially_unwanted_characters_to_project_settings.rb @@ -0,0 +1,16 @@ +# 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 diff --git a/db/schema_migrations/20210929144453 b/db/schema_migrations/20210929144453 new file mode 100644 index 00000000000..753ea50c272 --- /dev/null +++ b/db/schema_migrations/20210929144453 @@ -0,0 +1 @@ +0f808c27d19e6a38d4aa31f2dd820fe226681af84e05c4af47213409b2043e5a \ No newline at end of file diff --git a/db/structure.sql b/db/structure.sql index d3882446f57..c73ade92f80 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -18156,6 +18156,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)) ); diff --git a/lib/api/helpers/projects_helpers.rb b/lib/api/helpers/projects_helpers.rb index becd25595a6..30edbe91125 100644 --- a/lib/api/helpers/projects_helpers.rb +++ b/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' diff --git a/lib/gitlab/import_export/project/import_export.yml b/lib/gitlab/import_export/project/import_export.yml index c962e0f677b..618ef9a4f43 100644 --- a/lib/gitlab/import_export/project/import_export.yml +++ b/lib/gitlab/import_export/project/import_export.yml @@ -325,6 +325,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 diff --git a/lib/gitlab/unicode.rb b/lib/gitlab/unicode.rb new file mode 100644 index 00000000000..b49c5647dab --- /dev/null +++ b/lib/gitlab/unicode.rb @@ -0,0 +1,19 @@ +# 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 diff --git a/lib/rouge/formatters/html_gitlab.rb b/lib/rouge/formatters/html_gitlab.rb index e0e9677fac7..9e76225fc54 100644 --- a/lib/rouge/formatters/html_gitlab.rb +++ b/lib/rouge/formatters/html_gitlab.rb @@ -21,12 +21,24 @@ module Rouge is_first = false yield %() - 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 %() @line_number += 1 end end + + private + + def highlight_unicode_control_characters(text) + text.gsub(Gitlab::Unicode::BIDI_REGEXP) do |char| + %(#{char}) + end + end end end end diff --git a/locale/gitlab.pot b/locale/gitlab.pot index bdb3c99969b..56e7413c530 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -25736,6 +25736,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 "" @@ -26849,6 +26852,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 "" @@ -27038,6 +27044,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 "" diff --git a/spec/controllers/projects_controller_spec.rb b/spec/controllers/projects_controller_spec.rb index 3d966848c5b..b34cfedb767 100644 --- a/spec/controllers/projects_controller_spec.rb +++ b/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 diff --git a/spec/frontend/pages/projects/shared/permissions/components/settings_panel_spec.js b/spec/frontend/pages/projects/shared/permissions/components/settings_panel_spec.js index 1e562419f32..0020269e4e7 100644 --- a/spec/frontend/pages/projects/shared/permissions/components/settings_panel_spec.js +++ b/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(); diff --git a/spec/helpers/projects_helper_spec.rb b/spec/helpers/projects_helper_spec.rb index 1100f4a3ad5..5d52c9178cb 100644 --- a/spec/helpers/projects_helper_spec.rb +++ b/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 diff --git a/spec/lib/gitlab/unicode_spec.rb b/spec/lib/gitlab/unicode_spec.rb new file mode 100644 index 00000000000..68f3266ecc7 --- /dev/null +++ b/spec/lib/gitlab/unicode_spec.rb @@ -0,0 +1,33 @@ +# 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 diff --git a/spec/lib/rouge/formatters/html_gitlab_spec.rb b/spec/lib/rouge/formatters/html_gitlab_spec.rb index 4bc9b256dce..7c92c62e30b 100644 --- a/spec/lib/rouge/formatters/html_gitlab_spec.rb +++ b/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(%{}).exactly(4).times + end + end end end diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index 10220448936..2e5c5af4eb0 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -667,6 +667,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 diff --git a/spec/requests/api/project_attributes.yml b/spec/requests/api/project_attributes.yml index 9174356f123..dd00d413664 100644 --- a/spec/requests/api/project_attributes.yml +++ b/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 -- cgit v1.2.3