Fix basic XSS vector, check with more of them

PragTob · PragTob · commit e96915cf20c4 · 2020-02-04T15:48:35.000+01:00
diff --git a/Gemfile b/Gemfile
@@ -26,7 +26,7 @@ group :development, :test do
   gem 'puma'
   gem 'simplecov', require: false, group: :test
   gem 'byebug'
-  # gem 'pry-byebug'
+  gem 'pry-byebug'
   gem 'webmock'
   gem 'webdrivers', '~> 4.1'
 end
diff --git a/Gemfile.lock b/Gemfile.lock
@@ -80,6 +80,7 @@ GEM
       cells (>= 4.1.6, < 5.0.0)
     childprocess (0.9.0)
       ffi (~> 1.0, >= 1.0.11)
+    coderay (1.1.2)
     concurrent-ruby (1.1.5)
     crack (0.4.3)
       safe_yaml (~> 1.0.0)
@@ -125,6 +126,12 @@ GEM
     nokogiri (1.10.5)
       mini_portile2 (~> 2.4.0)
     pipetree (0.1.1)
+    pry (0.12.2)
+      coderay (~> 1.1.0)
+      method_source (~> 0.9.0)
+    pry-byebug (3.8.0)
+      byebug (~> 11.0)
+      pry (~> 0.10)
     public_suffix (3.0.3)
     puma (4.3.1)
       nio4r (~> 2.0)
@@ -253,6 +260,7 @@ DEPENDENCIES
   cells-rails
   generator_spec
   matestack-ui-core!
+  pry-byebug
   puma
   rspec-rails (~> 3.8)
   selenium-webdriver
diff --git a/app/concepts/matestack/ui/core/app/app.rb b/app/concepts/matestack/ui/core/app/app.rb
@@ -1,7 +1,7 @@
 module Matestack::Ui::Core::App
   class App < Trailblazer::Cell
 
-    include ::Cell::Haml
+    include Matestack::Ui::Core::Cell
     include Matestack::Ui::Core::ApplicationHelper
     include Matestack::Ui::Core::ToCell
 
diff --git a/app/concepts/matestack/ui/core/component/dynamic.rb b/app/concepts/matestack/ui/core/component/dynamic.rb
@@ -1,7 +1,6 @@
 module Matestack::Ui::Core::Component
   class Dynamic < Trailblazer::Cell
-
-    include ::Cell::Haml
+    include Matestack::Ui::Core::Cell
     include Matestack::Ui::Core::ApplicationHelper
     include Matestack::Ui::Core::ToCell
     include Matestack::Ui::Core::HasViewContext
diff --git a/app/concepts/matestack/ui/core/page/page.rb b/app/concepts/matestack/ui/core/page/page.rb
@@ -2,7 +2,7 @@ module Matestack::Ui::Core::Page
   class Page < Trailblazer::Cell
 
     include ActionView::Helpers::TranslationHelper
-    include ::Cell::Haml
+    include Matestack::Ui::Core::Cell
     include Matestack::Ui::Core::ApplicationHelper
     include Matestack::Ui::Core::ToCell
     include Matestack::Ui::Core::HasViewContext
diff --git a/app/concepts/matestack/ui/core/plain/plain.rb b/app/concepts/matestack/ui/core/plain/plain.rb
@@ -2,7 +2,7 @@ module Matestack::Ui::Core::Plain
   class Plain < Matestack::Ui::Core::Component::Static
 
     def show
-      @argument
+      html_escape @argument
     end
 
   end
diff --git a/lib/matestack/ui/core.rb b/lib/matestack/ui/core.rb
@@ -4,6 +4,7 @@
 require 'cell/rails'
 require 'cell/haml'
 
+require "matestack/ui/core/cell"
 require "matestack/ui/core/engine"
 
 module Matestack
diff --git a/lib/matestack/ui/core/cell.rb b/lib/matestack/ui/core/cell.rb
@@ -0,0 +1,33 @@
+module Matestack
+  module Ui
+    module Core
+      # Custom Cell options/handling based on a Cell from the cells gem.
+      #
+      # Needed to redefine some options and gives us more control over
+      # the cells.
+      module Cell
+        include ::Cell::Haml
+
+        # based on https://github.com/trailblazer/cells-haml/blob/master/lib/cell/haml.rb
+        # be aware that as of February 2020 this differs from the released version though.
+        def template_options_for(_options)
+          # Note, cells uses Hash#delete which mutates the hash,
+          # hence we can't use a constant here as on the first
+          # invocation it'd lose it's suffix key
+          {
+            template_class: ::Tilt::HamlTemplate,
+            escape_html:    true,
+            escape_attrs:   true,
+            suffix:         "haml"
+          }
+        end
+
+        # def html_escape(string)
+
+        # end
+      end
+    end
+  end
+end
+
+# Matestack::Ui::Core::Cell = ::Cell::Haml
diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb
@@ -36,6 +36,8 @@
 
 Dir[File.join File.dirname(__FILE__), 'support', '**', '*.rb'].each { |f| require f }
 
+require 'pry'
+
 RSpec.configure do |config|
   # config.include Capybara::DSL
   # rspec-expectations config goes here. You can use an alternate
diff --git a/spec/support/xss.rb b/spec/support/xss.rb
@@ -0,0 +1,4 @@
+module XSS
+  EVIL_SCRIPT = "<script>alert('hello');</script>"
+  ESCAPED_EVIL_SCRIPT = "&lt;script&gt;alert('hello');&lt;/script&gt;"
+end
diff --git a/spec/usage/base/xss_spec.rb b/spec/usage/base/xss_spec.rb
@@ -0,0 +1,53 @@
+describe "XSS behavior", type: :feature, js: true do
+
+  describe "injection in heading as an example" do
+    it "escapes the evil script" do
+      class ExamplePage < Matestack::Ui::Page
+        def response
+          components {
+            heading text: XSS::EVIL_SCRIPT
+          }
+        end
+      end
+
+      visit "/example"
+
+      static_output = page.html
+      expect(static_output).to include("<h1>\n#{XSS::ESCAPED_EVIL_SCRIPT}\n</h1>")
+    end
+
+    it "does not escape when we specifically say #html_safe" do
+      class ExamplePage < Matestack::Ui::Page
+        def response
+          components {
+            heading text: XSS::EVIL_SCRIPT.html_safe
+          }
+        end
+      end
+
+      # gotta accept our injected alert
+      accept_alert do
+        visit "/example"
+      end
+
+      # for reasons beyond me Chrome seems to remove our injected script tag,
+      # but since we accepted an alert to get here this test should be fine
+    end
+
+    it "escapes the evil when injecting into attributes" do
+      class ExamplePage < Matestack::Ui::Page
+
+        def response
+          components {
+            heading text: "Be Safe!", id: "something-\"#{XSS::EVIL_SCRIPT}"
+          }
+        end
+
+      end
+
+      visit "/example"
+
+      expect(page.html).to include("id=\"something-&quot;<script>alert('hello');</script>")
+    end
+  end
+end
diff --git a/spec/usage/components/plain_spec.rb b/spec/usage/components/plain_spec.rb
@@ -26,4 +26,36 @@ def response
     expect(stripped(static_output)).to include(stripped(expected_static_output))
   end
 
+  describe "XSSing" do
+    it "doesn't allow script injection" do
+      class ExamplePage < Matestack::Ui::Page
+        def response
+          components {
+            plain XSS::EVIL_SCRIPT
+          }
+        end
+      end
+
+      visit "/example"
+
+      expect(page.html).to include(XSS::ESCAPED_EVIL_SCRIPT)
+    end
+
+    it "allows injection when you say #html_safe" do
+      class ExamplePage < Matestack::Ui::Page
+        def response
+          components {
+            plain XSS::EVIL_SCRIPT.html_safe
+          }
+        end
+      end
+
+      # gotta accept our injected alert
+      accept_alert do
+        visit "/example"
+      end
+
+      # the script tag seems removed afterwards so we can't check against it here
+    end
+  end
 end
diff --git a/spec/usage/components/youtube_spec.rb b/spec/usage/components/youtube_spec.rb
@@ -25,7 +25,7 @@ def response
     static_output = page.html
 
     expect(page).to have_selector("iframe[src='https://www.youtube.com/embed/OY5AeGhgK7I'][class='iframe']")
-    expect(page).to have_selector("iframe[src='https://www.youtube.com/embed/OY5AeGhgK7I?controls=0&start=30']")
+    expect(page).to have_selector("iframe[src='https://www.youtube.com/embed/OY5AeGhgK7I?controls=0&amp;start=30']")
     expect(page).to have_selector("iframe[src='https://www.youtube-nocookie.com/embed/OY5AeGhgK7I?start=30']")
 
   end
