diff --git a/danger/html_escape_once/Dangerfile b/danger/html_escape_once/Dangerfile new file mode 100644 index 0000000000000000000000000000000000000000..2724507521a98fa76bd4515ea1d3d248d31e1c12 --- /dev/null +++ b/danger/html_escape_once/Dangerfile @@ -0,0 +1,3 @@ +# frozen_string_literal: true + +html_escape_once.check_html_escape_once_calls diff --git a/danger/plugins/html_escape_once.rb b/danger/plugins/html_escape_once.rb new file mode 100644 index 0000000000000000000000000000000000000000..bc1044fea71af61cf3fa9b797260bc65d1e21330 --- /dev/null +++ b/danger/plugins/html_escape_once.rb @@ -0,0 +1,9 @@ +# frozen_string_literal: true + +require_relative '../../tooling/danger/html_escape_once' + +module Danger + class HtmlEscapeOnce < ::Danger::Plugin + include Tooling::Danger::HtmlEscapeOnce + end +end diff --git a/spec/tooling/danger/html_escape_once_spec.rb b/spec/tooling/danger/html_escape_once_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..412f03e89950f7cbc367b35b3254d1c96f4d1142 --- /dev/null +++ b/spec/tooling/danger/html_escape_once_spec.rb @@ -0,0 +1,108 @@ +# frozen_string_literal: true + +require 'fast_spec_helper' +require 'gitlab/dangerfiles/spec_helper' +require_relative '../../../tooling/danger/html_escape_once' +require_relative '../../../tooling/danger/project_helper' + +RSpec.describe Tooling::Danger::HtmlEscapeOnce, feature_category: :markdown do + include_context "with dangerfile" + + subject(:html_escape_once) { fake_danger.new(helper: fake_helper) } + + let(:fake_danger) { DangerSpecHelper.fake_danger.include(described_class) } + let(:all_changed_files) { ['lib/banzai/filter/include_filter.rb'] } + let(:changed_lines) { [] } + + before do + allow(fake_helper).to receive_messages(all_changed_files: all_changed_files, changed_lines: changed_lines) + end + + describe '#check_html_escape_once_calls' do + subject(:check_html_escape_once_calls) { html_escape_once.check_html_escape_once_calls } + + context 'when there are no added calls' do + let(:changed_lines) { ['+def some_method', '+ end'] } + + it 'does not fail' do + expect(html_escape_once).not_to receive(:fail) + check_html_escape_once_calls + end + end + + context 'when there is a changed call' do + let(:changed_lines) { ['-ERB::Util.html_escape_once("hello")', '+ERB::Util.html_escape_once("hola")'] } + + it 'does not fail' do + expect(html_escape_once).not_to receive(:fail) + check_html_escape_once_calls + end + end + + context 'when there is a removed call' do + let(:changed_lines) { ['-ERB::Util.html_escape_once("hello")'] } + + it 'does not fail' do + expect(html_escape_once).not_to receive(:fail) + check_html_escape_once_calls + end + end + + context 'when there is an added call' do + let(:changed_lines) { ['+ERB::Util.html_escape_once("hello")'] } + + it 'fails' do + expect(html_escape_once).to receive(:fail).with( + %r{^Added calls found in:\n\n- `lib/banzai/filter/include_filter\.rb`\n}) + check_html_escape_once_calls + end + end + + context 'when there is a call added to a line with one already in it' do + let(:changed_lines) do + [ + '-ERB::Util.html_escape_once("hello")', + '+ERB::Util.html_escape_once("hello") + escape_once("hi")' + ] + end + + it 'fails' do + expect(html_escape_once).to receive(:fail).with( + %r{^Added calls found in:\n\n- `lib/banzai/filter/include_filter\.rb`\n}) + check_html_escape_once_calls + end + end + + context 'when there is a call removed from a line that had two' do + let(:changed_lines) do + [ + '-ERB::Util.html_escape_once("hello") + escape_once("hi")', + '+ERB::Util.html_escape_once("hello")' + ] + end + + it 'does not fail' do + expect(html_escape_once).not_to receive(:fail) + check_html_escape_once_calls + end + end + + context 'when there is an added call to something similar' do + let(:changed_lines) { ['+not_actually_html_escape_once("hello")'] } + + it 'does not fail' do + expect(html_escape_once).not_to receive(:fail) + check_html_escape_once_calls + end + end + + context 'when unrelated or ignored files are changed' do + let(:all_changed_files) { ['irrelevant.js', 'danger/ignored.rb'] } + + it 'does not check their contents' do + expect(fake_helper).not_to receive(:changed_lines) + check_html_escape_once_calls + end + end + end +end diff --git a/tooling/danger/html_escape_once.rb b/tooling/danger/html_escape_once.rb new file mode 100644 index 0000000000000000000000000000000000000000..d05539af961620ddb745beff315ca242913c1245 --- /dev/null +++ b/tooling/danger/html_escape_once.rb @@ -0,0 +1,70 @@ +# frozen_string_literal: true + +module Tooling + module Danger + module HtmlEscapeOnce + def check_html_escape_once_calls + added_calls = added_call_paths + fail message(added_calls) if added_calls.any? + end + + private + + TARGET_CALL = /\b(?:html_escape_once|escape_once)\b/ + + def added_call_paths + helper.all_changed_files.select do |file| + next unless file.end_with?('.rb', '.haml', '.erb') + next if file.start_with?('danger/', 'tooling/', 'spec/tooling/', 'ee/spec/tooling/') + + # Block MRs that (overall) add uses of `html_escape_once` and `escape_once`. + # + # Don't block MRs that only _change_ lines containing these. + # We can't perfectly know if a given + and - pair up, but better to fail safe + # here than block MRs incorrectly. + net = 0 + helper.changed_lines(file).grep(TARGET_CALL).each do |line| + if line.start_with?('+') + net += line.scan(TARGET_CALL).count + elsif line.start_with?('-') + net -= line.scan(TARGET_CALL).count + end + end + + net > 0 + end + end + + def format_added_calls(added_calls) + added_calls.map { |c| "- `#{c}`" }.join("\n") + end + + def message(added_calls) + format(message_template, { added_calls: format_added_calls(added_calls) }) + end + + def message_template + <<~MSG + Adding calls to `html_escape_once` or `escape_once` to the codebase is forbidden! + We are in the process of removing all calls to these methods --- they mix unescaped + and escaped content in a way which cannot be unmixed, and often leads to XSS. + + We should always know whether any string content is considered text (and _must_ be escaped + when included in HTML) or HTML (and can be included safely) --- any in-between means we have + lost track of what can be trusted, and need to revisit the data flow. + + Please remove the added calls to `html_escape_once` or `escape_once`. + Use `html_escape` if the input is text, and ask for help if you need it! + + Thank you for helping keep GitLab XSS-free! + + Added calls found in: + + %s + + Tracking issue: https://gitlab.com/gitlab-org/gitlab/-/issues/581104 + MSG + end + end + end +end