From f28cd627f89bf27d79764f0d81bd06176c415eeb Mon Sep 17 00:00:00 2001 From: Ryan Lue Date: Tue, 12 Feb 2019 07:46:04 +0100 Subject: [PATCH 1/9] Refactoring: Migrate notification_factory_template_test.rb to RSpec --- lib/notification_factory/template.rb | 38 ++++---- .../lib/notification_factory/template_spec.rb | 91 +++++++++++++++++++ .../notification_factory_template_test.rb | 79 ---------------- 3 files changed, 108 insertions(+), 100 deletions(-) create mode 100644 spec/lib/notification_factory/template_spec.rb delete mode 100644 test/unit/notification_factory_template_test.rb diff --git a/lib/notification_factory/template.rb b/lib/notification_factory/template.rb index 98ce92347..945eba14d 100644 --- a/lib/notification_factory/template.rb +++ b/lib/notification_factory/template.rb @@ -17,35 +17,31 @@ examples how to use end def to_s - strip_html - end + @template.gsub(/\#{\s*(.*?)\s*}/m) do + # some browsers start adding HTML tags + # fixes https://github.com/zammad/zammad/issues/385 + input_template = $1.gsub(/\A<.+?>\s*|\s*<.+?>\z/, '') - def strip_html - # some browsers start adding HTML tags - # fixes https://github.com/zammad/zammad/issues/385 - @template.gsub(/\#\{\s*t\((.+?)\)\s*\}/m) do - content = $1 - if content =~ /^'(.+?)'$/ - %(<%= t "#{strip_content($1)}", #{@escape} %>) + case input_template + when /\At\('(.+?)'\)\z/m + %(<%= t "#{sanitize_text($1)}", #{@escape} %>) + when /\At\((.+?)\)\z/m + %(<%= t d"#{sanitize_object_name($1)}", #{@escape} %>) + when /\Aconfig\.(.+?)\z/m + %(<%= c "#{sanitize_object_name($1)}", #{@escape} %>) else - %(<%= t d"#{strip_variable(content)}", #{@escape} %>) + %(<%= d "#{sanitize_object_name(input_template)}", #{@escape} %>) end - end.gsub(/\#\{\s*config\.(.+?)\s*\}/m) do - %(<%= c "#{strip_variable($1)}", #{@escape} %>) - end.gsub(/\#\{(.*?)\}/m) do - %(<%= d "#{strip_variable($1)}", #{@escape} %>) end end - def strip_content(string) - string&.gsub(/\t|\r|\n/, '') - &.gsub(/"/, '\"') + def sanitize_text(string) + string&.tr("\t\r\n", '') + &.gsub(/(?/, '') + def sanitize_object_name(string) + string&.tr("\t\r\n\f \"'§;", '') end end diff --git a/spec/lib/notification_factory/template_spec.rb b/spec/lib/notification_factory/template_spec.rb new file mode 100644 index 000000000..094f5ca46 --- /dev/null +++ b/spec/lib/notification_factory/template_spec.rb @@ -0,0 +1,91 @@ +require 'rails_helper' + +RSpec.describe NotificationFactory::Template do + subject(:template) do + NotificationFactory::Template.new(template_string, escape) + end + + describe '#to_s' do + context 'for empty input template (incl. whitespace-only)' do + let(:template_string) { "\#{ }" } + + context 'with escape = true' do + let(:escape) { true } + + it 'returns an ERB template with the #d helper, and passes escape arg as string' do + expect(template.to_s).to eq('<%= d "", true %>') + end + end + + context 'with escape = false' do + let(:escape) { false } + + it 'returns an ERB template with the #d helper, and passes escape arg as string' do + expect(template.to_s).to eq('<%= d "", false %>') + end + end + end + + context 'for input template using #t helper' do + let(:template_string) { "\#{t('some text')}" } + let(:escape) { false } + + it 'returns an ERB template with the #t helper, and passes escape arg as string' do + expect(template.to_s).to eq('<%= t "some text", false %>') + end + + context 'with double-quotes in argument' do + let(:template_string) { "\#{t('some \"text\"')}" } + + it 'adds backslash-escaping' do + expect(template.to_s).to eq('<%= t "some \"text\"", false %>') + end + end + end + + # Regression test for https://github.com/zammad/zammad/issues/385 + context 'with HTML auto-injected by browser' do + let(:escape) { true } + + context 'for tags wrapped around "ticket.id"' do + let(:template_string) { <<~'TEMPLATE'.chomp } + #{ticket.id} + TEMPLATE + + it 'strips tag from resulting ERB template' do + expect(template.to_s).to eq('<%= d "ticket.id", true %>') + end + end + + context 'for tags wrapped around "config.fqdn"' do + let(:template_string) { <<~'TEMPLATE'.chomp } + #{config.fqdn} + TEMPLATE + + it 'strips tag from resulting ERB template' do + expect(template.to_s).to eq('<%= c "fqdn", true %>') + end + end + + context 'for tags surrounded by whitespace' do + let(:template_string) { <<~'TEMPLATE'.chomp } + #{ ticket.id } + TEMPLATE + + it 'strips tag and spaces from template' do + expect(template.to_s).to eq('<%= d "ticket.id", true %>') + end + end + + context 'for unpaired tag and trailing whitespace' do + let(:template_string) { <<~'TEMPLATE'.chomp } + #{ticket.id } + TEMPLATE + + it 'strips tag and spaces from template' do + expect(template.to_s).to eq('<%= d "ticket.id", true %>') + end + end + end + end +end diff --git a/test/unit/notification_factory_template_test.rb b/test/unit/notification_factory_template_test.rb deleted file mode 100644 index 38ca7367c..000000000 --- a/test/unit/notification_factory_template_test.rb +++ /dev/null @@ -1,79 +0,0 @@ -require 'test_helper' - -class NotificationFactoryTemplateTest < ActiveSupport::TestCase - - # RSpec incoming! - def described_class - NotificationFactory::Template - end - - test 'regular browser html' do - - # ensures https://github.com/zammad/zammad/issues/385 - template_before = '#{ticket.id}' - template_after = '<%= d "ticket.id", true %>' - - result = described_class.new(template_before, true).to_s - assert_equal(template_after, result) - - template_before = '#{config.fqdn}' - template_after = '<%= d "config.fqdn", true %>' - - result = described_class.new(template_before, true).to_s - assert_equal(template_after, result) - end - - test 'spaced browser html' do - - # ensures https://github.com/zammad/zammad/issues/385 - template_before = '#{ ticket.id }' - template_after = '<%= d "ticket.id", true %>' - - result = described_class.new(template_before, true).to_s - assert_equal(template_after, result) - end - - test 'broken browser html' do - - # ensures https://github.com/zammad/zammad/issues/385 - template_before = '#{ticket.id }' - template_after = '<%= d "ticket.id", true %>' - - result = described_class.new(template_before, true).to_s - assert_equal(template_after, result) - end - - test 'empty tag' do - - template_before = '#{}' - template_after = '<%= d "", true %>' - - result = described_class.new(template_before, true).to_s - assert_equal(template_after, result) - end - - test 'empty tag with space' do - - template_before = '#{ }' - template_after = '<%= d "", false %>' - - result = described_class.new(template_before, false).to_s - assert_equal(template_after, result) - end - - test 'translation' do - - template_before = "\#{t('some text')}" - template_after = '<%= t "some text", false %>' - - result = described_class.new(template_before, false).to_s - assert_equal(template_after, result) - - template_before = "\#{t('some \"text\"')}" - template_after = '<%= t "some \"text\"", false %>' - - result = described_class.new(template_before, false).to_s - assert_equal(template_after, result) - end - -end From dabce01ec2aba82aaafdb08deba755b7b222734c Mon Sep 17 00:00:00 2001 From: Martin Edenhofer Date: Tue, 12 Feb 2019 08:38:59 +0100 Subject: [PATCH 2/9] Sanitize html for note fields of tickets, users and organizations. --- app/models/organization.rb | 3 +++ app/models/ticket.rb | 3 +++ app/models/user.rb | 3 +++ .../models/concerns/has_xss_sanitized_note_examples.rb | 10 ++++++++++ spec/models/organization_spec.rb | 3 +++ spec/models/ticket_spec.rb | 3 +++ spec/models/user_spec.rb | 3 +++ 7 files changed, 28 insertions(+) create mode 100644 spec/models/concerns/has_xss_sanitized_note_examples.rb diff --git a/app/models/organization.rb b/app/models/organization.rb index d772fea1b..0b35e24af 100644 --- a/app/models/organization.rb +++ b/app/models/organization.rb @@ -7,6 +7,7 @@ class Organization < ApplicationModel include HasHistory include HasSearchIndexBackend include CanCsvImport + include ChecksHtmlSanitized include Organization::ChecksAccess include Organization::Assets @@ -22,6 +23,8 @@ class Organization < ApplicationModel activity_stream_permission 'admin.role' + sanitized_html :note + private def domain_cleanup diff --git a/app/models/ticket.rb b/app/models/ticket.rb index 740120b55..a5a81c1df 100644 --- a/app/models/ticket.rb +++ b/app/models/ticket.rb @@ -6,6 +6,7 @@ class Ticket < ApplicationModel include ChecksClientNotification include ChecksLatestChangeObserved include CanCsvImport + include ChecksHtmlSanitized include HasHistory include HasTags include HasSearchIndexBackend @@ -56,6 +57,8 @@ class Ticket < ApplicationModel history_relation_object 'Ticket::Article' + sanitized_html :note + belongs_to :group belongs_to :organization has_many :articles, class_name: 'Ticket::Article', after_add: :cache_update, after_remove: :cache_update, dependent: :destroy, inverse_of: :ticket diff --git a/app/models/user.rb b/app/models/user.rb index 5d564f088..280431476 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -6,6 +6,7 @@ class User < ApplicationModel include HasHistory include HasSearchIndexBackend include CanCsvImport + include ChecksHtmlSanitized include HasGroups include HasRoles @@ -66,6 +67,8 @@ class User < ApplicationModel :groups, :user_groups + sanitized_html :note + def ignore_search_indexing?(_action) # ignore internal user return true if id == 1 diff --git a/spec/models/concerns/has_xss_sanitized_note_examples.rb b/spec/models/concerns/has_xss_sanitized_note_examples.rb new file mode 100644 index 000000000..8af57b840 --- /dev/null +++ b/spec/models/concerns/has_xss_sanitized_note_examples.rb @@ -0,0 +1,10 @@ +RSpec.shared_examples 'HasXssSanitizedNote' do |model_factory:| + describe 'XSS prevention' do + context 'with injected JS' do + subject { create(model_factory, note: 'test 123 some text') } + it 'strips out