Merge remote-tracking branch 'origin/cells-haml-love' into prepare_0.7.4_release

jonasjabari · jonasjabari · commit e4df711e98b6 · 2020-02-10T19:17:26.000+01:00
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
@@ -94,7 +94,7 @@ def show(component_key=nil, only_page=false)
         begin
           render_child_component component_key
         rescue => e
-          raise "Component '#{component_key}' could not be resolved."
+          raise "Component '#{component_key}' could not be resolved, because of #{e},\n#{e.backtrace.join("\n")}"
         end
       end
     end
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/app/concepts/matestack/ui/core/raw/raw.rb b/app/concepts/matestack/ui/core/raw/raw.rb
@@ -0,0 +1,7 @@
+module Matestack::Ui::Core::Rawhtml
+  class Rawhtml < Matestack::Ui::Core::Component::Static
+    def show
+      @argument
+    end
+  end
+end
diff --git a/docs/components/plain.md b/docs/components/plain.md
@@ -2,7 +2,7 @@
 
 Show [specs](/spec/usage/components/plain_spec.rb)
 
-This element simply renders the value of a variable (or simple a string) wherever you want it.
+This element simply renders the value of a variable (or simple a string) wherever you want it **escaping HTML tags** (`<` becomes `&lt;` etc.).
 
 ## Parameters
 
diff --git a/docs/components/rawhtml.md b/docs/components/rawhtml.md
@@ -0,0 +1,35 @@
+# matestack core component: Rawhtml
+
+Show [specs](/spec/usage/components/rawhtml_spec.rb)
+
+This element simply renders the value of a variable (or simple a string) wherever you want it **without escaping HTML**.
+
+Only use this if you are sure that you have full control over the input to this function/no malicious code can find its way inside.
+
+## Parameters
+
+This component expects one parameter.
+
+## Example 1
+
+Rendering some HTML.
+
+```ruby
+
+def response
+  components {
+    rawhtml <<~HTML
+    <h1>Hello World</h1>
+    <script>alert('Really Hello!')</script>
+    HTML
+}
+end
+
+```
+
+returns
+
+```html
+<h1>Hello World</h1>
+<script>alert('Really Hello!')</script>
+```
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)
+          ERB::Util.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
@@ -41,7 +41,6 @@
 require 'rspec/retry'
 require "rspec/wait"  
 
-
 RSpec.configure do |config|
   # repeat flaky tests
   # show retry status in spec process
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/component_spec.rb b/spec/usage/base/component_spec.rb
@@ -345,7 +345,6 @@ def response
       after_content = element.text
 
       expect(before_content).not_to eq(after_content)
-
     end
 
     it "components can render dynamic content with vue.js involved if inherit from 'Matestack::Ui::DynamicComponent'" do
diff --git a/spec/usage/base/xss_spec.rb b/spec/usage/base/xss_spec.rb
@@ -0,0 +1,75 @@
+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
+
+    # note that `heading do "string" end` doesn't work and you
+    # should rather use `heading do plain "string" end`
+    # Why the hell is this tested then?
+    # Well if that behavior changed I'd like to have a reminder
+    # here to catch it. Call me overly cautious.
+    it "escaping won't be broken in block form (if it worked)" do
+      class ExamplePage < Matestack::Ui::Page
+        def response
+          components {
+            heading do
+              XSS::EVIL_SCRIPT
+            end
+          }
+        end
+      end
+
+      visit "/example"
+
+      static_output = page.html
+      expect(static_output).not_to include("alert(")
+    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/absolute_spec.rb b/spec/usage/components/absolute_spec.rb
@@ -28,4 +28,26 @@ def response
     expect(stripped(static_output)).to include(stripped(expected_static_output))
   end
 
+  describe "XSSing" do
+    it "escaping" do
+      class ExamplePage < Matestack::Ui::Page
+
+        def response
+          components {
+            absolute top: 50, left: '50px;">' + XSS::EVIL_SCRIPT, right: 50, bottom: 100, z: 3 do
+              plain 'I am absolute content'
+            end
+          }
+        end
+
+      end
+
+      visit '/example'
+
+      static_output = page.html
+
+      expect(static_output).not_to include("alert")
+    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/rawhtml_spec.rb b/spec/usage/components/rawhtml_spec.rb
@@ -0,0 +1,25 @@
+describe "Raw Html Component", type: :feature, js: true do
+
+  it "allows the insertion of pure HTML: Example 1" do
+
+    class ExamplePage < Matestack::Ui::Page
+      def response
+        components {
+          rawhtml <<~HTML
+          <h1>Hello World</h1>
+          <script>alert('Really Hello!')</script>
+          HTML
+        }
+      end
+    end
+
+    accept_alert do
+      visit "/example"
+    end
+
+    static_output = page.html
+
+    expect(static_output).to include("<h1>Hello World</h1>")
+  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
