diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 28c9d9b..8dbf530 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -15,7 +15,7 @@ jobs: matrix: include: - ruby: 2.5.9 - gemfile: Gemfile.rails-3.2 + gemfile: Gemfile.rails-3.2.haml-4 - ruby: 2.5.9 gemfile: Gemfile.rails-4.2.haml-4 - ruby: 2.5.9 @@ -42,6 +42,10 @@ jobs: gemfile: Gemfile.rails-6.1.haml-5 - ruby: 3.2.3 gemfile: Gemfile.rails-7.0.haml-5 + - ruby: 3.2.3 + gemfile: Gemfile.rails-7.1.haml-5 + - ruby: 3.2.3 + gemfile: Gemfile.rails-7.1.haml-6 env: BUNDLE_GEMFILE: "${{ matrix.gemfile }}" steps: diff --git a/CHANGELOG.md b/CHANGELOG.md index 0d22edc..e1738a9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,14 @@ This project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.html ## Unreleased ### Compatible changes +* Add compatibility with Rails 7.1 +* Add compatibility with HAML 6 + * NOTE: Don't use HAML 6.0.0. AngularXSS relies on a patch [introduced in 6.0.1](https://github.com/haml/haml/blob/main/CHANGELOG.md#601). Anything newer should be fine - the gem is currently tested against HAML 6.3 +* Refactor our patches to use `Module#prepend` instead of `Module#module_eval` +* Refactor gem version comparisons to use `Gem::Version` instances +* Refactor specs to use the `expect` syntax +* Add missing unit tests for patched methods +* Improve test coverage for more interpolation scenarios in ERB and HAML ### Breaking changes diff --git a/Gemfile.rails-3.2 b/Gemfile.rails-3.2.haml-4 similarity index 100% rename from Gemfile.rails-3.2 rename to Gemfile.rails-3.2.haml-4 diff --git a/Gemfile.rails-3.2.lock b/Gemfile.rails-3.2.haml-4.lock similarity index 100% rename from Gemfile.rails-3.2.lock rename to Gemfile.rails-3.2.haml-4.lock diff --git a/Gemfile.rails-5.1.haml-5.lock b/Gemfile.rails-5.1.haml-5.lock index bfd31fa..ebd368e 100644 --- a/Gemfile.rails-5.1.haml-5.lock +++ b/Gemfile.rails-5.1.haml-5.lock @@ -54,7 +54,7 @@ GEM nokogiri (>= 1.6) rails-html-sanitizer (1.0.3) loofah (~> 2.0) - rake (12.3.0) + rake (13.2.1) rspec (3.10.0) rspec-core (~> 3.10.0) rspec-expectations (~> 3.10.0) diff --git a/Gemfile.rails-7.1.haml-5 b/Gemfile.rails-7.1.haml-5 new file mode 100644 index 0000000..3fb6175 --- /dev/null +++ b/Gemfile.rails-7.1.haml-5 @@ -0,0 +1,9 @@ +source 'http://rubygems.org' + +gem 'actionpack', '~>7.1' +gem 'rspec' +gem 'haml', '~> 5' +gem 'angular_xss', :path => '.' +gem 'gemika', '>= 0.8.3' +gem 'rake' +gem 'byebug' diff --git a/Gemfile.rails-7.1.haml-5.lock b/Gemfile.rails-7.1.haml-5.lock new file mode 100644 index 0000000..6aa1b54 --- /dev/null +++ b/Gemfile.rails-7.1.haml-5.lock @@ -0,0 +1,105 @@ +PATH + remote: . + specs: + angular_xss (0.4.1) + activesupport + haml (>= 3.1.5) + +GEM + remote: http://rubygems.org/ + specs: + actionpack (7.1.3.4) + actionview (= 7.1.3.4) + activesupport (= 7.1.3.4) + nokogiri (>= 1.8.5) + racc + rack (>= 2.2.4) + rack-session (>= 1.0.1) + rack-test (>= 0.6.3) + rails-dom-testing (~> 2.2) + rails-html-sanitizer (~> 1.6) + actionview (7.1.3.4) + activesupport (= 7.1.3.4) + builder (~> 3.1) + erubi (~> 1.11) + rails-dom-testing (~> 2.2) + rails-html-sanitizer (~> 1.6) + activesupport (7.1.3.4) + base64 + bigdecimal + concurrent-ruby (~> 1.0, >= 1.0.2) + connection_pool (>= 2.2.5) + drb + i18n (>= 1.6, < 2) + minitest (>= 5.1) + mutex_m + tzinfo (~> 2.0) + base64 (0.2.0) + bigdecimal (3.1.8) + builder (3.3.0) + byebug (11.1.3) + concurrent-ruby (1.3.3) + connection_pool (2.4.1) + crass (1.0.6) + diff-lcs (1.5.1) + drb (2.2.1) + erubi (1.13.0) + gemika (0.8.3) + haml (5.2.2) + temple (>= 0.8.0) + tilt + i18n (1.14.5) + concurrent-ruby (~> 1.0) + loofah (2.22.0) + crass (~> 1.0.2) + nokogiri (>= 1.12.0) + minitest (5.23.1) + mutex_m (0.2.0) + nokogiri (1.16.6-x86_64-linux) + racc (~> 1.4) + racc (1.8.0) + rack (3.1.3) + rack-session (2.0.0) + rack (>= 3.0.0) + rack-test (2.1.0) + rack (>= 1.3) + rails-dom-testing (2.2.0) + activesupport (>= 5.0.0) + minitest + nokogiri (>= 1.6) + rails-html-sanitizer (1.6.0) + loofah (~> 2.21) + nokogiri (~> 1.14) + rake (13.2.1) + rspec (3.13.0) + rspec-core (~> 3.13.0) + rspec-expectations (~> 3.13.0) + rspec-mocks (~> 3.13.0) + rspec-core (3.13.0) + rspec-support (~> 3.13.0) + rspec-expectations (3.13.1) + diff-lcs (>= 1.2.0, < 2.0) + rspec-support (~> 3.13.0) + rspec-mocks (3.13.1) + diff-lcs (>= 1.2.0, < 2.0) + rspec-support (~> 3.13.0) + rspec-support (3.13.1) + temple (0.10.3) + tilt (2.3.0) + tzinfo (2.0.6) + concurrent-ruby (~> 1.0) + +PLATFORMS + x86_64-linux + +DEPENDENCIES + actionpack (~> 7.1) + angular_xss! + byebug + gemika (>= 0.8.3) + haml (~> 5) + rake + rspec + +BUNDLED WITH + 2.5.13 diff --git a/Gemfile.rails-7.1.haml-6 b/Gemfile.rails-7.1.haml-6 new file mode 100644 index 0000000..1581997 --- /dev/null +++ b/Gemfile.rails-7.1.haml-6 @@ -0,0 +1,9 @@ +source 'http://rubygems.org' + +gem 'actionpack', '~>7.1' +gem 'rspec' +gem 'haml', '~> 6' +gem 'angular_xss', :path => '.' +gem 'gemika', '>= 0.8.3' +gem 'rake' +gem 'byebug' diff --git a/Gemfile.rails-7.1.haml-6.lock b/Gemfile.rails-7.1.haml-6.lock new file mode 100644 index 0000000..a73902f --- /dev/null +++ b/Gemfile.rails-7.1.haml-6.lock @@ -0,0 +1,122 @@ +PATH + remote: . + specs: + angular_xss (0.4.1) + activesupport + haml (>= 3.1.5) + +GEM + remote: http://rubygems.org/ + specs: + actionpack (7.1.3.4) + actionview (= 7.1.3.4) + activesupport (= 7.1.3.4) + nokogiri (>= 1.8.5) + racc + rack (>= 2.2.4) + rack-session (>= 1.0.1) + rack-test (>= 0.6.3) + rails-dom-testing (~> 2.2) + rails-html-sanitizer (~> 1.6) + actionview (7.1.3.4) + activesupport (= 7.1.3.4) + builder (~> 3.1) + erubi (~> 1.11) + rails-dom-testing (~> 2.2) + rails-html-sanitizer (~> 1.6) + activesupport (7.1.3.4) + base64 + bigdecimal + concurrent-ruby (~> 1.0, >= 1.0.2) + connection_pool (>= 2.2.5) + drb + i18n (>= 1.6, < 2) + minitest (>= 5.1) + mutex_m + tzinfo (~> 2.0) + base64 (0.2.0) + bigdecimal (3.1.8) + builder (3.3.0) + byebug (11.1.3) + concurrent-ruby (1.3.3) + connection_pool (2.4.1) + crass (1.0.6) + diff-lcs (1.5.1) + drb (2.2.1) + erubi (1.13.0) + gemika (0.8.3) + haml (6.3.0) + temple (>= 0.8.2) + thor + tilt + i18n (1.14.5) + concurrent-ruby (~> 1.0) + loofah (2.22.0) + crass (~> 1.0.2) + nokogiri (>= 1.12.0) + minitest (5.24.0) + mutex_m (0.2.0) + nokogiri (1.16.6-aarch64-linux) + racc (~> 1.4) + nokogiri (1.16.6-arm-linux) + racc (~> 1.4) + nokogiri (1.16.6-arm64-darwin) + racc (~> 1.4) + nokogiri (1.16.6-x86-linux) + racc (~> 1.4) + nokogiri (1.16.6-x86_64-darwin) + racc (~> 1.4) + nokogiri (1.16.6-x86_64-linux) + racc (~> 1.4) + racc (1.8.0) + rack (3.1.3) + rack-session (2.0.0) + rack (>= 3.0.0) + rack-test (2.1.0) + rack (>= 1.3) + rails-dom-testing (2.2.0) + activesupport (>= 5.0.0) + minitest + nokogiri (>= 1.6) + rails-html-sanitizer (1.6.0) + loofah (~> 2.21) + nokogiri (~> 1.14) + rake (13.2.1) + rspec (3.13.0) + rspec-core (~> 3.13.0) + rspec-expectations (~> 3.13.0) + rspec-mocks (~> 3.13.0) + rspec-core (3.13.0) + rspec-support (~> 3.13.0) + rspec-expectations (3.13.1) + diff-lcs (>= 1.2.0, < 2.0) + rspec-support (~> 3.13.0) + rspec-mocks (3.13.1) + diff-lcs (>= 1.2.0, < 2.0) + rspec-support (~> 3.13.0) + rspec-support (3.13.1) + temple (0.10.3) + thor (1.3.1) + tilt (2.3.0) + tzinfo (2.0.6) + concurrent-ruby (~> 1.0) + +PLATFORMS + aarch64-linux + arm-linux + arm64-darwin + x86-linux + x86_64-darwin + x86_64-linux + +DEPENDENCIES + actionpack (~> 7.1) + angular_xss! + byebug + gemika (>= 0.8.3) + haml (~> 6) + rake + rspec + +BUNDLED WITH + 2.5.13 diff --git a/README.md b/README.md index 8f3f76e..244ab5b 100644 --- a/README.md +++ b/README.md @@ -56,11 +56,13 @@ Development ----------- - Fork the repository. -- Push your changes with specs. There is a Rails 3 test application in `spec/app_root` if you need to test integration with a live Rails app. -- You may run single tests with a specified Rails version via `BUNDLE_GEMFILE=Gemfile.rails-7.0.haml-5 bundle exec rspec ./spec/angular_xss` +- Prepare your changes, and ensure existing and new test are green: + - `bundle exec rake matrix:install` installs all dependencies for all Gemfiles + - `bundle exec rake matrix:spec` runs all specs in all configurations + - You may run single tests with a specified Rails version via `BUNDLE_GEMFILE=Gemfile.rails-7.0.haml-5 bundle exec rspec ./spec/angular_xss` +- Push your changes with specs. There is a test application in `spec/app_root` if you need to test integration with a live Rails app. - Send a pull request. - Credits ------- diff --git a/lib/angular_xss.rb b/lib/angular_xss.rb index fe76527..3a986ec 100644 --- a/lib/angular_xss.rb +++ b/lib/angular_xss.rb @@ -1,6 +1,7 @@ #"string".respond_to?(:html_safe?) or raise "No rails_xss implementation present" require 'angular_xss/escaper' +require 'angular_xss/output_buffer' require 'angular_xss/safe_buffer' require 'angular_xss/erb' require 'angular_xss/haml' diff --git a/lib/angular_xss/erb.rb b/lib/angular_xss/erb.rb index 89e6440..76bd00a 100644 --- a/lib/angular_xss/erb.rb +++ b/lib/angular_xss/erb.rb @@ -1,33 +1,25 @@ -# Use module_eval so we crash when ERB::Util has not yet been loaded. -ERB::Util.module_eval do - - if private_method_defined? :unwrapped_html_escape # Rails 4.2+ - - def unwrapped_html_escape_with_escaping_angular_expressions(s) - s = s.to_s - if s.html_safe? - s - else - unwrapped_html_escape_without_escaping_angular_expressions(AngularXss::Escaper.escape(s)) - end +if ERB::Util.private_method_defined? :unwrapped_html_escape + # Rails 4.2+ + # https://github.com/rails/rails/blob/main/activesupport/lib/active_support/core_ext/erb/util.rb + module ERBUtilExt + def html_escape_once(s) + super(AngularXss::Escaper.escape_if_unsafe(s)) end - alias_method :unwrapped_html_escape_without_escaping_angular_expressions, :unwrapped_html_escape - alias_method :unwrapped_html_escape, :unwrapped_html_escape_with_escaping_angular_expressions - - singleton_class.send(:remove_method, :unwrapped_html_escape) - module_function :unwrapped_html_escape - module_function :unwrapped_html_escape_without_escaping_angular_expressions + def unwrapped_html_escape(s) + super(AngularXss::Escaper.escape_if_unsafe(s)) + end + # Note that html_escape() and h() are passively fixed as they are calling the two methods above + end + ERB::Util.prepend ERBUtilExt + ERB::Util.singleton_class.prepend ERBUtilExt - else # Rails < 4.2 +else + ERB::Util.module_eval do + # Rails < 4.2 def html_escape_with_escaping_angular_expressions(s) - s = s.to_s - if s.html_safe? - s - else - html_escape_without_escaping_angular_expressions(AngularXss::Escaper.escape(s)) - end + html_escape_without_escaping_angular_expressions(AngularXss::Escaper.escape_if_unsafe(s)) end alias_method_chain :html_escape, :escaping_angular_expressions @@ -41,7 +33,5 @@ def html_escape_with_escaping_angular_expressions(s) singleton_class.send(:remove_method, :html_escape) module_function :html_escape module_function :html_escape_without_escaping_angular_expressions - end - end diff --git a/lib/angular_xss/escaper.rb b/lib/angular_xss/escaper.rb index 9dbfe04..f453f1e 100644 --- a/lib/angular_xss/escaper.rb +++ b/lib/angular_xss/escaper.rb @@ -27,6 +27,14 @@ def self.escape(string) end end + def self.escape_if_unsafe(string) + if string.nil? || string.to_s.html_safe? + string + else + escape(string.to_s) + end + end + def self.disabled? !!Thread.current[XSS_DISABLED_KEY] end diff --git a/lib/angular_xss/haml.rb b/lib/angular_xss/haml.rb index 5904bd5..0dff7cb 100644 --- a/lib/angular_xss/haml.rb +++ b/lib/angular_xss/haml.rb @@ -1,32 +1,38 @@ -# Haml 5.0 and 5.1 fall back to erb -if Haml::VERSION < '5' +haml_version = Gem::Version.new(Haml::VERSION) + +if haml_version < Gem::Version.new(5) # Use module_eval so we crash when Haml::Helpers has not yet been loaded. Haml::Helpers.module_eval do - def html_escape_with_escaping_angular_expressions(s) - s = s.to_s - if s.html_safe? - s - else - html_escape_without_escaping_angular_expressions(AngularXss::Escaper.escape(s)) - end + html_escape_without_escaping_angular_expressions(AngularXss::Escaper.escape_if_unsafe(s)) end alias_method :html_escape_without_escaping_angular_expressions, :html_escape alias_method :html_escape, :html_escape_with_escaping_angular_expressions end +elsif haml_version < Gem::Version.new('5.2') + # Haml 5.0 and 5.1 fall back to erb +elsif haml_version < Gem::Version.new(6) + # HAML 5.2+ + module HTMLEscapeWithoutHAMLWithAngularXSS + def html_escape_without_haml_xss(html) + super(AngularXss::Escaper.escape_if_unsafe(html)) + end + end -elsif Haml::VERSION >= '5.2' - Haml::Helpers.module_eval do - - def html_escape_without_haml_xss_with_escaping_angular_expressions(s) - s = s.to_s - return s if s.html_safe? + Haml::Helpers.singleton_class.prepend HTMLEscapeWithoutHAMLWithAngularXSS +else + # Haml 6+ + # It ditched most of is own helpers in favor of Haml::Util.escape_html + # https://github.com/haml/haml/blob/main/CHANGELOG.md#600 + # https://github.com/haml/haml/compare/v5.2.2...v6.3.0 + # https://github.com/haml/haml/blob/v6.3.0/lib/haml/util.rb - html_escape_without_haml_xss_without_escaping_angular_expressions(AngularXss::Escaper.escape(s)) + module EscapeHTMLWithAngularXSS + def escape_html(html) + super(AngularXss::Escaper.escape_if_unsafe(html)) end - - alias_method :html_escape_without_haml_xss_without_escaping_angular_expressions, :html_escape_without_haml_xss - alias_method :html_escape_without_haml_xss, :html_escape_without_haml_xss_with_escaping_angular_expressions end + + Haml::Util.singleton_class.prepend EscapeHTMLWithAngularXSS end diff --git a/lib/angular_xss/output_buffer.rb b/lib/angular_xss/output_buffer.rb new file mode 100644 index 0000000..76ee082 --- /dev/null +++ b/lib/angular_xss/output_buffer.rb @@ -0,0 +1,25 @@ +## +# Monkey patch ActionView::OutputBuffer to escape double braces from Angular +# +# Link to the original implementation without Angular XSS escaping: +# https://github.com/rails/rails/blob/v7.1.3.4/actionview/lib/action_view/buffers.rb + + +if defined?(ActionView::VERSION) && Gem::Version.new(ActionView::VERSION::STRING) >= Gem::Version.new('7.1') + # ActionView < 7.1 used our patched ERB::Util.h to escape, 7.1 switched to CGI.escapeHTML + module OutputBufferWithEscapedAngularXSS + def <<(value) + super(AngularXss::Escaper.escape_if_unsafe(value)) + end + + def concat(value) + super(AngularXss::Escaper.escape_if_unsafe(value)) + end + + def append=(value) + super(AngularXss::Escaper.escape_if_unsafe(value)) + end + end + + ActionView::OutputBuffer.prepend OutputBufferWithEscapedAngularXSS +end diff --git a/spec/angular_xss/erb_spec.rb b/spec/angular_xss/erb_spec.rb index b165f22..d191742 100644 --- a/spec/angular_xss/erb_spec.rb +++ b/spec/angular_xss/erb_spec.rb @@ -1,7 +1,50 @@ -require 'spec_helper' - describe 'Angular XSS prevention in ERB', :type => :view do - it_should_behave_like 'engine preventing Angular XSS', :partial => 'test_erb' +end + +describe ERB::Util do + describe '#html_escape' do + it 'escapes angular braces' do + expect(described_class.html_escape("{{unsafe}}")).to eq("{{ $root.DOUBLE_LEFT_CURLY_BRACE }}unsafe}}") + end + + it 'does not modify already HTML safe strings' do + expect(described_class.html_escape("{{safe}}".html_safe)).to eq("{{safe}}") + end + end + + describe '#h' do + it 'escapes angular braces' do + expect(described_class.h("{{unsafe}}")).to eq("{{ $root.DOUBLE_LEFT_CURLY_BRACE }}unsafe}}") + end + + it 'does not modify already HTML safe strings' do + expect(described_class.h("{{safe}}".html_safe)).to eq("{{safe}}") + end + end + + # Rails < 4 does not implement unwrapped_html_escape and html_escape_once + if described_class.method_defined? :unwrapped_html_escape + describe '#unwrapped_html_escape' do + it 'escapes angular braces' do + expect(described_class.unwrapped_html_escape("{{unsafe}}")).to eq("{{ $root.DOUBLE_LEFT_CURLY_BRACE }}unsafe}}") + end + + it 'does not modify already HTML safe strings' do + expect(described_class.unwrapped_html_escape("{{safe}}".html_safe)).to eq("{{safe}}") + end + end + end + + if described_class.method_defined? :html_escape_once + describe '#html_escape_once' do + it 'escapes angular braces' do + expect(described_class.html_escape_once("{{unsafe}}")).to eq("{{ $root.DOUBLE_LEFT_CURLY_BRACE }}unsafe}}") + end + it 'does not modify already HTML safe strings' do + expect(described_class.html_escape_once("{{safe}}".html_safe)).to eq("{{safe}}") + end + end + end end diff --git a/spec/angular_xss/escaper_spec.rb b/spec/angular_xss/escaper_spec.rb new file mode 100644 index 0000000..8a891a7 --- /dev/null +++ b/spec/angular_xss/escaper_spec.rb @@ -0,0 +1,21 @@ +describe AngularXss::Escaper do + describe '.escape' do + it 'replaces double braces with a closed variant' do + expect(described_class.escape('{{')).to eq('{{ $root.DOUBLE_LEFT_CURLY_BRACE }}') + end + + it 'does not handle HTML safe strings differently' do + expect(described_class.escape('{{'.html_safe)).to eq('{{ $root.DOUBLE_LEFT_CURLY_BRACE }}') + end + end + + describe '.escape_if_unsafe' do + it 'replaces double braces with a closed variant' do + expect(described_class.escape_if_unsafe('{{')).to eq('{{ $root.DOUBLE_LEFT_CURLY_BRACE }}') + end + + it 'does not modify HTML safe strings' do + expect(described_class.escape_if_unsafe('{{'.html_safe)).to eq('{{') + end + end +end diff --git a/spec/angular_xss/haml_spec.rb b/spec/angular_xss/haml_spec.rb index 844419f..46928ed 100644 --- a/spec/angular_xss/haml_spec.rb +++ b/spec/angular_xss/haml_spec.rb @@ -1,5 +1,3 @@ -require 'spec_helper' - describe 'Angular XSS prevention in Haml', :type => :view do it_should_behave_like 'engine preventing Angular XSS', :partial => 'test_haml' diff --git a/spec/angular_xss/output_buffer_spec.rb b/spec/angular_xss/output_buffer_spec.rb new file mode 100644 index 0000000..a193852 --- /dev/null +++ b/spec/angular_xss/output_buffer_spec.rb @@ -0,0 +1,45 @@ +describe ActionView::OutputBuffer do + describe '#<<' do + it 'escapes angular braces' do + expect((subject << "{{unsafe}}").to_s).to eq("{{ $root.DOUBLE_LEFT_CURLY_BRACE }}unsafe}}") + end + + it 'does not change behavior for already HTML safe strings' do + expect((subject << "{{safe}}".html_safe).to_s).to eq("{{safe}}") + end + + it 'allows concatting nil' do + expect { subject << nil }.to_not raise_error + end + end + + describe '#concat' do + it 'escapes angular braces' do + expect((subject.concat "{{unsafe}}").to_s).to eq("{{ $root.DOUBLE_LEFT_CURLY_BRACE }}unsafe}}") + end + + it 'does not change behavior for already HTML safe strings' do + expect((subject.concat "{{safe}}".html_safe).to_s).to eq("{{safe}}") + end + + it 'allows concatting nil' do + expect { subject.concat nil }.to_not raise_error + end + end + + describe '#append=' do + it 'escapes angular braces' do + subject.append = "{{unsafe}}" + expect(subject.to_s).to eq("{{ $root.DOUBLE_LEFT_CURLY_BRACE }}unsafe}}") + end + + it 'does not change behavior for already HTML safe strings' do + subject.append = "{{safe}}".html_safe + expect(subject.to_s).to eq("{{safe}}") + end + + it 'allows concatting nil' do + expect { subject.append = nil }.to_not raise_error + end + end +end diff --git a/spec/angular_xss/safe_buffer_spec.rb b/spec/angular_xss/safe_buffer_spec.rb index d8cc40f..e959821 100644 --- a/spec/angular_xss/safe_buffer_spec.rb +++ b/spec/angular_xss/safe_buffer_spec.rb @@ -1,9 +1,21 @@ -require 'spec_helper' - describe ActiveSupport::SafeBuffer do - it 'still allows concatting nil' do - expect { subject << nil }.to_not raise_error + describe '#<<' do + it 'escapes angular braces' do + subject << "{{unsafe}}" + expect(subject.to_s).to eq("{{ $root.DOUBLE_LEFT_CURLY_BRACE }}unsafe}}") + end + + it 'allows concatting nil' do + expect { subject << nil }.to_not raise_error + end + end + + describe '#+' do + it 'escapes angular braces' do + combined_string = subject + "{{unsafe}}" + expect(combined_string.to_s).to eq("{{ $root.DOUBLE_LEFT_CURLY_BRACE }}unsafe}}") + end end end diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index f891d5a..b2d9bc1 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -17,7 +17,11 @@ def self.env end require 'haml' -require 'haml/template' +if Gem::Version.new(Haml::VERSION) < Gem::Version.new(6) + require 'haml/template' +else + require 'haml/rails_template' +end require 'angular_xss' @@ -25,13 +29,3 @@ def self.env Dir["#{File.dirname(__FILE__)}/support/**/*.rb"].each {|f| require f} TEMPLATE_ROOT = Pathname.new(__dir__).join('templates') - - -RSpec.configure do |config| - config.mock_with :rspec do |c| - c.syntax = [:should, :expect] - end - config.expect_with :rspec do |c| - c.syntax = [:should, :expect] - end -end diff --git a/spec/support/engine_preventing_angular_xss.rb b/spec/support/engine_preventing_angular_xss.rb index a47422e..025f4bc 100644 --- a/spec/support/engine_preventing_angular_xss.rb +++ b/spec/support/engine_preventing_angular_xss.rb @@ -2,7 +2,7 @@ let(:path_set) { ActionView::LookupContext.new([TEMPLATE_ROOT]) } - if defined?(ActionView::VERSION) && ActionView::VERSION::MAJOR >= 6 + if defined?(ActionView::VERSION) && Gem::Version.new(ActionView::VERSION::MAJOR) >= Gem::Version.new(6) let(:engine) { ActionView::Base.with_empty_template_cache.new(path_set, {}, nil) } else let(:engine) { ActionView::Base.new(path_set) } @@ -11,14 +11,18 @@ let(:html) { engine.render(partial) } it 'escapes Angular interpolation marks in unsafe strings' do - html.should_not include('{{unsafe}}') - html.should include('{{ $root.DOUBLE_LEFT_CURLY_BRACE }}unsafe}}') + expect(html).not_to include('{{unsafe}}') + expect(html).to include('{{ $root.DOUBLE_LEFT_CURLY_BRACE }}unsafe}}') end it 'recognizes the many ways to express an opening curly brace in HTML' do + # Only unsafe strings are escaped + expect(html).to include("{{ $root.DOUBLE_LEFT_CURLY_BRACE }}unsafe}}") + expect(html).not_to include("{{ $root.DOUBLE_LEFT_CURLY_BRACE }}safe}}") - html.should include("{{ $root.DOUBLE_LEFT_CURLY_BRACE }}unsafe}}") - html.should_not include("{{unsafe}}") + # Only safe strings with braces are left untouched + expect(html).to include("{{safe}}") + expect(html).not_to include("{{unsafe}}") braces = [ '{', @@ -35,15 +39,15 @@ braces.each do |brace1| braces.each do |brace2| - html.should_not include("#{brace1}#{brace2}unsafe}}") + expect(html).not_to include("#{brace1}#{brace2}unsafe}}") end end end it 'does not escape Angular interpolation marks in safe strings' do - html.should include("{{safe}}") - html.should_not include("{{ $root.DOUBLE_LEFT_CURLY_BRACE }}safe}}") + expect(html).to include("{{safe}}") + expect(html).not_to include("{{ $root.DOUBLE_LEFT_CURLY_BRACE }}safe}}") end it 'does not escape Angular interpolation marks in a block where AngularXSS is disabled' do @@ -52,8 +56,8 @@ result = html end - result.should include('{{unsafe}}') - result.should_not include('{{ $root.DOUBLE_LEFT_CURLY_BRACE }}unsafe}}') + expect(result).to include('{{unsafe}}') + expect(result).not_to include('{{ $root.DOUBLE_LEFT_CURLY_BRACE }}unsafe}}') end it 'does escape Angular interpolation marks after the block where AngularXSS is disabled' do @@ -61,27 +65,27 @@ end result = html - result.should include('{{ $root.DOUBLE_LEFT_CURLY_BRACE }}unsafe}}') - result.should_not include('{{unsafe}}') + expect(result).to include('{{ $root.DOUBLE_LEFT_CURLY_BRACE }}unsafe}}') + expect(result).not_to include('{{unsafe}}') end it 'is not confused by exceptions in disable blocks' do class SomeException < StandardError; end - proc { + expect do AngularXss.disable do raise SomeException end - }.should raise_error(SomeException) + end.to raise_error(SomeException) - html.should include('{{ $root.DOUBLE_LEFT_CURLY_BRACE }}unsafe}}') - html.should_not include('{{unsafe}}') + expect(html).to include('{{ $root.DOUBLE_LEFT_CURLY_BRACE }}unsafe}}') + expect(html).not_to include('{{unsafe}}') end it 'does not escape twice' do escaped = AngularXss::Escaper.escape('{{') double_escaped = AngularXss::Escaper.escape(escaped) - html.should_not include(double_escaped) + expect(html).not_to include(double_escaped) end end diff --git a/spec/templates/_test_erb.erb b/spec/templates/_test_erb.erb index 6def798..4352ee8 100644 --- a/spec/templates/_test_erb.erb +++ b/spec/templates/_test_erb.erb @@ -1,14 +1,23 @@ -<%= "{{unsafe}}" %> -<%= "{{safe}}".html_safe %> +<%- unsafe_string = '{{unsafe}}' %> +<%- safe_string = '{{safe}}'.html_safe %> + +<%= unsafe_string %> +<%= safe_string %> + +<%= ''.html_safe + unsafe_string %> +<%= ''.html_safe + safe_string %> + +<%= ''.html_safe << unsafe_string %> +<%= ''.html_safe << safe_string %> {{safe}} -
+
{{safe}}
-<%= content_tag(:span, '{{unsafe}}') %> -<%= content_tag(:span, '{{safe}}'.html_safe) %> +<%= content_tag(:span, unsafe_string) %> +<%= content_tag(:span, safe_string) %> <%= '{{unsafe}}' %> <%= '{{unsafe}}' %> diff --git a/spec/templates/_test_haml.haml b/spec/templates/_test_haml.haml index c6ad216..4b96b2d 100644 --- a/spec/templates/_test_haml.haml +++ b/spec/templates/_test_haml.haml @@ -1,11 +1,47 @@ -= "{{unsafe}}" -#{'{{unsafe}}'} -= "{{safe}}".html_safe +-# HTML attributes and static string interpolation in Haml work in different ways: +-# 1. Under certain conditions, attributes are precompiled. +-# We never have to escape those because they can not contain user input. +-# 2. Whenever there is a Ruby call on attributes, Haml will have to evaluate +-# them at runtime. Since they can contain user input, XSS logic applies. + +-# precompiled (static) + +- if Gem::Version.new(Haml::VERSION) >= Gem::Version.new(6) + -# HAML 6 is smart enough to recognize static strings and will not + -# escape it - so neither do we + #{'{{safe}}'} + = "{{safe}}" +- else + #{'{{unsafe}}'} + = "{{unsafe}}" {{safe}} +%div(foo='{{safe}}') +%div{:class => '{{safe}}', :id => '{{safe}}'} + +-# Compiled at runtime: +- unsafe_evaluated_variable = '{{unsafe}}' +- safe_evaluated_variable = '{{safe}}'.html_safe + += unsafe_evaluated_variable += safe_evaluated_variable + +#{unsafe_evaluated_variable} +#{safe_evaluated_variable} + += ''.html_safe + unsafe_evaluated_variable += ''.html_safe + safe_evaluated_variable + += ''.html_safe << unsafe_evaluated_variable += ''.html_safe << safe_evaluated_variable -= content_tag(:span, '{{unsafe}}') -= content_tag(:span, '{{safe}}'.html_safe) += content_tag(:span, unsafe_evaluated_variable) += content_tag(:span, safe_evaluated_variable) + +%div{:class => unsafe_evaluated_variable, :id => unsafe_evaluated_variable} +%div(bar="#{unsafe_evaluated_variable}") + %div{:foo => safe_evaluated_variable, :bar => unsafe_evaluated_variable} + {{safe}} = '{{unsafe}}' = '{{unsafe}}' @@ -17,21 +53,3 @@ = '{{unsafe}}' = '{{unsafe}}' = '{{unsafe}}' - --# HTML attributes in Haml work in different ways: --# 1. Under certain conditions, attributes are precompiled. --# We never have to escape those because they can not contain user input. --# 2. Whenever there is a Ruby call on attributes, Haml will have to evaluate --# them at runtime. Since they can contain user input, XSS logic applies. - --# Precompiled: -%div(foo='{{safe}}') -%div{:class => '{{safe}}', :id => '{{safe}}'} - --# Compiled at runtime: -- unsafe = '{{unsafe}}' -- safe = '{{safe}}'.html_safe -%div{:class => unsafe, :id => unsafe} -%div(bar="#{unsafe}") - %div{:foo => safe, :bar => unsafe} - {{safe}}