Refactoring: Migrate ticket_xss_test to RSpec

spec/models/channel/email_parser_spec.rb

@@ -406,6 +406,38 @@ RSpec.describe Channel::EmailParser, type: :model do
       end
     end
 
+    describe 'XSS protection' do
+      let(:article) { described_class.new.process({}, raw_mail).second }
+
+      let(:raw_mail) { <<~RAW.chomp }
+        From: ME Bob <me@example.com>
+        To: customer@example.com
+        Subject: some subject
+        Content-Type: #{content_type}
+        MIME-Version: 1.0
+
+        no HTML <script type="text/javascript">alert(\'XSS\')</script>
+      RAW
+
+      context 'for Content-Type: text/html' do
+        let(:content_type) { 'text/html' }
+
+        it 'removes injected <script> tags from body' do
+          expect(article.body).to eq("no HTML alert('XSS')")
+        end
+      end
+
+      context 'for Content-Type: text/plain' do
+        let(:content_type) { 'text/plain' }
+
+        it 'leaves body as-is' do
+          expect(article.body).to eq(<<~SANITIZED.chomp)
+            no HTML <script type="text/javascript">alert(\'XSS\')</script>
+          SANITIZED
+        end
+      end
+    end
+
     context 'for “delivery failed” notifications (a.k.a. bounce messages)' do
       let(:ticket) { article.ticket }
       let(:article) { create(:ticket_article, sender_name: 'Agent', message_id: message_id) }

spec/models/ticket/article_spec.rb

@@ -57,6 +57,189 @@ RSpec.describe Ticket::Article, type: :model do
       end
     end
 
+    describe 'XSS protection:' do
+      subject(:article) { create(:ticket_article, body: body, content_type: 'text/html') }
+
+      context 'when body contains only injected JS' do
+        let(:body) { <<~RAW.chomp }
+          <script type="text/javascript">alert("XSS!");</script>
+        RAW
+
+        it 'removes <script> tags' do
+          expect(article.body).to eq('alert("XSS!");')
+        end
+      end
+
+      context 'when body contains injected JS amidst other text' do
+        let(:body) { <<~RAW.chomp }
+          please tell me this doesn't work: <script type="text/javascript">alert("XSS!");</script>
+        RAW
+
+        it 'removes <script> tags' do
+          expect(article.body).to eq(<<~SANITIZED.chomp)
+            please tell me this doesn't work: alert("XSS!");
+          SANITIZED
+        end
+      end
+
+      context 'when body contains invalid HTML tags' do
+        let(:body) { '<some_not_existing>ABC</some_not_existing>' }
+
+        it 'removes invalid tags' do
+          expect(article.body).to eq('ABC')
+        end
+      end
+
+      context 'when body contains restricted HTML attributes' do
+        let(:body) { '<div class="adasd" id="123" data-abc="123"></div>' }
+
+        it 'removes restricted attributes' do
+          expect(article.body).to eq('<div></div>')
+        end
+      end
+
+      context 'when body contains JS injected into href attribute' do
+        let(:body) { '<a href="javascript:someFunction()">LINK</a>' }
+
+        it 'removes <a> tags' do
+          expect(article.body).to eq('LINK')
+        end
+      end
+
+      context 'when body contains an unclosed <div> element' do
+        let(:body) { '<div>foo' }
+
+        it 'closes it' do
+          expect(article.body).to eq('<div>foo</div>')
+        end
+      end
+
+      context 'when body contains a plain link (<a> element)' do
+        let(:body) { '<a href="https://example.com">foo</a>' }
+
+        it 'adds sanitization attributes' do
+          expect(article.body).to eq(<<~SANITIZED.chomp)
+            <a href="https://example.com" rel="nofollow noreferrer noopener" target="_blank" title="https://example.com">foo</a>
+          SANITIZED
+        end
+      end
+
+      context 'for all cases above, combined' do
+        let(:body) { <<~RAW.chomp }
+          please tell me this doesn't work: <table>ada<tr></tr></table>
+          <div class="adasd" id="123" data-abc="123"></div>
+          <div>
+          <a href="javascript:someFunction()">LINK</a>
+          <a href="http://lalal.de">aa</a>
+          <some_not_existing>ABC</some_not_existing>
+        RAW
+
+        it 'performs all sanitizations' do
+          expect(article.body).to eq(<<~SANITIZED.chomp)
+            please tell me this doesn't work: <table>ada<tr></tr>
+            </table>
+            <div></div>
+            <div>
+            LINK
+            <a href="http://lalal.de" rel="nofollow noreferrer noopener" target="_blank" title="http://lalal.de">aa</a>
+            ABC</div>
+          SANITIZED
+        end
+      end
+
+      context 'for content_type: "text/plain"' do
+        subject(:article) { create(:ticket_article, body: body, content_type: 'text/plain') }
+
+        let(:body) { <<~RAW.chomp }
+          please tell me this doesn't work: <table>ada<tr></tr></table>
+          <div class="adasd" id="123" data-abc="123"></div>
+          <div>
+          <a href="javascript:someFunction()">LINK</a>
+          <a href="http://lalal.de">aa</a>
+          <some_not_existing>ABC</some_not_existing>
+        RAW
+
+        it 'performs no sanitizations' do
+          expect(article.body).to eq(<<~SANITIZED.chomp)
+            please tell me this doesn't work: <table>ada<tr></tr></table>
+            <div class="adasd" id="123" data-abc="123"></div>
+            <div>
+            <a href="javascript:someFunction()">LINK</a>
+            <a href="http://lalal.de">aa</a>
+            <some_not_existing>ABC</some_not_existing>
+          SANITIZED
+        end
+      end
+
+      context 'when body contains <video> element' do
+        let(:body) { <<~RAW.chomp }
+          please tell me this doesn't work: <video>some video</video><foo>alal</foo>
+        RAW
+
+        it 'leaves it as-is' do
+          expect(article.body).to eq(<<~SANITIZED.chomp)
+            please tell me this doesn't work: <video>some video</video>alal
+          SANITIZED
+        end
+      end
+
+      context 'when body contains CSS in style attribute' do
+        context 'for cid-style email attachment' do
+          let(:body) { <<~RAW.chomp }
+            <img style="width: 85.5px; height: 49.5px" src="cid:15.274327094.140938@zammad.example.com">
+            asdasd
+            <img src="cid:15.274327094.140939@zammad.example.com">
+          RAW
+
+          it 'adds terminal semicolons to style rules' do
+            expect(article.body).to eq(<<~SANITIZED.chomp)
+              <img style="width: 85.5px; height: 49.5px;" src="cid:15.274327094.140938@zammad.example.com">
+              asdasd
+              <img src="cid:15.274327094.140939@zammad.example.com">
+            SANITIZED
+          end
+        end
+
+        context 'for relative-path-style email attachment' do
+          let(:body) { <<~RAW.chomp }
+            <img style="width: 85.5px; height: 49.5px" src="api/v1/ticket_attachment/123/123/123">
+            asdasd
+            <img src="api/v1/ticket_attachment/123/123/123">
+          RAW
+
+          it 'adds terminal semicolons to style rules' do
+            expect(article.body).to eq(<<~SANITIZED.chomp)
+              <img style="width: 85.5px; height: 49.5px;" src="api/v1/ticket_attachment/123/123/123">
+              asdasd
+              <img src="api/v1/ticket_attachment/123/123/123">
+            SANITIZED
+          end
+        end
+      end
+
+      context 'when body contains <body> elements' do
+        let(:body) { '<body>123</body>' }
+
+        it 'removes <body> tags' do
+          expect(article.body).to eq('123')
+        end
+      end
+
+      context 'when body contains onclick attributes in <a> elements' do
+        let(:body) { <<~RAW.chomp }
+          <a href="#" onclick="some_function();">abc</a>
+          <a href="https://example.com" oNclIck="some_function();">123</a>
+        RAW
+
+        it 'removes onclick attributes' do
+          expect(article.body).to eq(<<~SANITIZED.chomp)
+            <a href="#">abc</a>
+            <a href="https://example.com" rel="nofollow noreferrer noopener" target="_blank" title="https://example.com">123</a>
+          SANITIZED
+        end
+      end
+    end
+
     describe 'DoS protection:' do
       context 'when #body exceeds 1.5MB' do
         subject(:article) { create(:ticket_article, body: body) }

spec/models/ticket_spec.rb

@@ -585,6 +585,16 @@ RSpec.describe Ticket, type: :model do
       end
     end
 
+    describe 'XSS protection:' do
+      subject(:ticket) { create(:ticket, title: title) }
+
+      let(:title) { 'test 123 <script type="text/javascript">alert("XSS!");</script>' }
+
+      it 'does not sanitize title' do
+        expect(ticket.title).to eq(title)
+      end
+    end
+
     describe 'Cti::CallerId syncing:' do
       subject(:ticket) { build(:ticket) }
       before { allow(Cti::CallerId).to receive(:build) }

test/unit/ticket_xss_test.rb

@@ -1,184 +0,0 @@
-require 'test_helper'
-
-class TicketXssTest < ActiveSupport::TestCase
-  test 'xss via model' do
-    ticket = Ticket.create(
-      title:         'test 123 <script type="text/javascript">alert("XSS!");</script>',
-      group:         Group.lookup(name: 'Users'),
-      customer_id:   2,
-      state:         Ticket::State.lookup(name: 'new'),
-      priority:      Ticket::Priority.lookup(name: '2 normal'),
-      updated_by_id: 1,
-      created_by_id: 1,
-    )
-    assert(ticket, 'ticket created')
-
-    assert_equal('test 123 <script type="text/javascript">alert("XSS!");</script>', ticket.title, 'ticket.title verify')
-    assert_equal('Users', ticket.group.name, 'ticket.group verify')
-    assert_equal('new', ticket.state.name, 'ticket.state verify')
-
-    article1 = Ticket::Article.create(
-      ticket_id:     ticket.id,
-      from:          'some_sender@example.com',
-      to:            'some_recipient@example.com',
-      subject:       'some subject <script type="text/javascript">alert("XSS!");</script>',
-      message_id:    'some@id',
-      content_type:  'text/html',
-      body:          '<script type="text/javascript">alert("XSS!");</script>',
-      internal:      false,
-      sender:        Ticket::Article::Sender.find_by(name: 'Customer'),
-      type:          Ticket::Article::Type.find_by(name: 'email'),
-      updated_by_id: 1,
-      created_by_id: 1,
-    )
-    assert_equal('alert("XSS!");', article1.body, 'article1.body verify - inbound')
-
-    article2 = Ticket::Article.create(
-      ticket_id:     ticket.id,
-      from:          'some_sender@example.com',
-      to:            'some_recipient@example.com',
-      subject:       'some subject <script type="text/javascript">alert("XSS!");</script>',
-      message_id:    'some@id',
-      content_type:  'text/html',
-      body:          'please tell me this doesn\'t work: <script type="text/javascript">alert("XSS!");</script>',
-      internal:      false,
-      sender:        Ticket::Article::Sender.find_by(name: 'Customer'),
-      type:          Ticket::Article::Type.find_by(name: 'email'),
-      updated_by_id: 1,
-      created_by_id: 1,
-    )
-    assert_equal('please tell me this doesn\'t work: alert("XSS!");', article2.body, 'article2.body verify - inbound')
-
-    article3 = Ticket::Article.create(
-      ticket_id:     ticket.id,
-      from:          'some_sender@example.com',
-      to:            'some_recipient@example.com',
-      subject:       'some subject <script type="text/javascript">alert("XSS!");</script>',
-      message_id:    'some@id',
-      content_type:  'text/html',
-      body:          'please tell me this doesn\'t work: <table>ada<tr></tr></table><div class="adasd" id="123" data-abc="123"></div><div><a href="javascript:someFunction()">LINK</a><a href="http://lalal.de">aa</a><some_not_existing>ABC</some_not_existing>',
-      internal:      false,
-      sender:        Ticket::Article::Sender.find_by(name: 'Customer'),
-      type:          Ticket::Article::Type.find_by(name: 'email'),
-      updated_by_id: 1,
-      created_by_id: 1,
-    )
-    assert_equal("please tell me this doesn't work: <table>ada<tr></tr>
-</table><div></div><div>
-LINK<a href=\"http://lalal.de\" rel=\"nofollow noreferrer noopener\" target=\"_blank\" title=\"http://lalal.de\">aa</a>ABC</div>", article3.body, 'article3.body verify - inbound')
-
-    article4 = Ticket::Article.create(
-      ticket_id:     ticket.id,
-      from:          'some_sender@example.com',
-      to:            'some_recipient@example.com',
-      subject:       'some subject <script type="text/javascript">alert("XSS!");</script>',
-      message_id:    'some@id',
-      content_type:  'text/html',
-      body:          'please tell me this doesn\'t work: <video>some video</video><foo>alal</foo>',
-      internal:      false,
-      sender:        Ticket::Article::Sender.find_by(name: 'Customer'),
-      type:          Ticket::Article::Type.find_by(name: 'email'),
-      updated_by_id: 1,
-      created_by_id: 1,
-    )
-    assert_equal("please tell me this doesn't work: <video>some video</video>alal", article4.body, 'article4.body verify - inbound')
-
-    article5 = Ticket::Article.create(
-      ticket_id:     ticket.id,
-      from:          'some_sender@example.com',
-      to:            'some_recipient@example.com',
-      subject:       'some subject <script type="text/javascript">alert("XSS!");</script>',
-      message_id:    'some@id',
-      content_type:  'text/plain',
-      body:          'please tell me this doesn\'t work: <table>ada<tr></tr></table><div class="adasd" id="123" data-signature-id="123"></div><div><a href="javascript:someFunction()">LINK</a><a href="http://lalal.de">aa</a><some_not_existing>ABC</some_not_existing>',
-      internal:      false,
-      sender:        Ticket::Article::Sender.find_by(name: 'Customer'),
-      type:          Ticket::Article::Type.find_by(name: 'email'),
-      updated_by_id: 1,
-      created_by_id: 1,
-    )
-    assert_equal('please tell me this doesn\'t work: <table>ada<tr></tr></table><div class="adasd" id="123" data-signature-id="123"></div><div><a href="javascript:someFunction()">LINK</a><a href="http://lalal.de">aa</a><some_not_existing>ABC</some_not_existing>', article5.body, 'article5.body verify - inbound')
-
-    article6 = Ticket::Article.create(
-      ticket_id:     ticket.id,
-      from:          'some_sender@example.com',
-      to:            'some_recipient@example.com',
-      subject:       'some subject <script type="text/javascript">alert("XSS!");</script>',
-      message_id:    'some@id',
-      content_type:  'text/html',
-      body:          'some message article helper test1 <div><img style="width: 85.5px; height: 49.5px" src="cid:15.274327094.140938@zammad.example.com">asdasd<img src="cid:15.274327094.140939@zammad.example.com"><br>',
-      internal:      false,
-      sender:        Ticket::Article::Sender.find_by(name: 'Customer'),
-      type:          Ticket::Article::Type.find_by(name: 'email'),
-      updated_by_id: 1,
-      created_by_id: 1,
-    )
-    assert_equal('some message article helper test1 <div>
-<img style="width: 85.5px; height: 49.5px;" src="cid:15.274327094.140938@zammad.example.com">asdasd<img src="cid:15.274327094.140939@zammad.example.com"><br>
-</div>', article6.body, 'article6.body verify - inbound')
-
-    article7 = Ticket::Article.create(
-      ticket_id:     ticket.id,
-      from:          'some_sender@example.com',
-      to:            'some_recipient@example.com',
-      subject:       'some subject <script type="text/javascript">alert("XSS!");</script>',
-      message_id:    'some@id',
-      content_type:  'text/html',
-      body:          'some message article helper test1 <div><img style="width: 85.5px; height: 49.5px" src="api/v1/ticket_attachment/123/123/123">asdasd<img src="api/v1/ticket_attachment/123/123/123"><br>',
-      internal:      false,
-      sender:        Ticket::Article::Sender.find_by(name: 'Customer'),
-      type:          Ticket::Article::Type.find_by(name: 'email'),
-      updated_by_id: 1,
-      created_by_id: 1,
-    )
-    assert_equal('some message article helper test1 <div>
-<img style="width: 85.5px; height: 49.5px;" src="api/v1/ticket_attachment/123/123/123">asdasd<img src="api/v1/ticket_attachment/123/123/123"><br>
-</div>', article7.body, 'article7.body verify - inbound')
-
-    article8 = Ticket::Article.create(
-      ticket_id:     ticket.id,
-      from:          'some_sender@example.com',
-      to:            'some_recipient@example.com',
-      subject:       'some subject <script type="text/javascript">alert("XSS!");</script>',
-      message_id:    'some@id',
-      content_type:  'text/html',
-      body:          'some message article helper test1 <a href="#" onclick="some_function();">abc</a> <a href="https://example.com" oNclIck="some_function();">123</a><body>123</body>',
-      internal:      false,
-      sender:        Ticket::Article::Sender.find_by(name: 'Customer'),
-      type:          Ticket::Article::Type.find_by(name: 'email'),
-      updated_by_id: 1,
-      created_by_id: 1,
-    )
-    assert_equal('some message article helper test1 <a href="#">abc</a> <a href="https://example.com" rel="nofollow noreferrer noopener" target="_blank" title="https://example.com">123</a>123', article8.body, 'article8.body verify - inbound')
-
-  end
-
-  test 'xss via mail' do
-    data = 'From: ME Bob <me@example.com>
-To: customer@example.com
-Subject: some subject
-Content-Type: text/html
-MIME-Version: 1.0
-
-no HTML <script type="text/javascript">alert(\'XSS\')</script>'
-
-    parser = Channel::EmailParser.new
-    ticket, article, user = parser.process({}, data)
-    assert_equal('text/html', ticket.articles.first.content_type)
-    assert_equal('no HTML alert(\'XSS\')', ticket.articles.first.body)
-
-    data = 'From: ME Bob <me@example.com>
-To: customer@example.com
-Subject: some subject
-Content-Type: text/plain
-MIME-Version: 1.0
-
-no HTML <script type="text/javascript">alert(\'XSS\')</script>'
-
-    parser = Channel::EmailParser.new
-    ticket, article, user = parser.process({}, data)
-    assert_equal('text/plain', ticket.articles.first.content_type)
-    assert_equal('no HTML <script type="text/javascript">alert(\'XSS\')</script>', ticket.articles.first.body)
-
-  end
-end