Merge branch 'rs-issue-36098' into 'security-9-5'

[9.5] Limit `style` attribute on `th` and `td` elements to specific properties

See merge request gitlab/gitlabhq!2155

changelogs/unreleased/rs-issue-36098.yml

+---
+title: Disallow arbitrary properties in `th` and `td` `style` attributes
+merge_request:
+author:

lib/banzai/filter/sanitization_filter.rb

@@ -5,6 +5,7 @@ module Banzai
     # Extends HTML::Pipeline::SanitizationFilter with a custom whitelist.
     class SanitizationFilter < HTML::Pipeline::SanitizationFilter
       UNSAFE_PROTOCOLS = %w(data javascript vbscript).freeze
+      TABLE_ALIGNMENT_PATTERN = /text-align: (?<alignment>center|left|right)/
 
       def whitelist
         whitelist = super
@@ -24,7 +25,8 @@ module Banzai
         # Only push these customizations once
         return if customized?(whitelist[:transformers])
 
-        # Allow table alignment
+        # Allow table alignment; we whitelist specific style properties in a
+        # transformer below
         whitelist[:attributes]['th'] = %w(style)
         whitelist[:attributes]['td'] = %w(style)
 
@@ -55,6 +57,9 @@ module Banzai
         # Remove `rel` attribute from `a` elements
         whitelist[:transformers].push(self.class.remove_rel)
 
+        # Remove any `style` properties not required for table alignment
+        whitelist[:transformers].push(self.class.remove_unsafe_table_style)
+
         whitelist
       end
 
@@ -84,6 +89,21 @@ module Banzai
             end
           end
         end
+
+        def remove_unsafe_table_style
+          lambda do |env|
+            node = env[:node]
+
+            return unless node.name == 'th' || node.name == 'td'
+            return unless node.has_attribute?('style')
+
+            if node['style'] =~ TABLE_ALIGNMENT_PATTERN
+              node['style'] = "text-align: #{$~[:alignment]}"
+            else
+              node.remove_attribute('style')
+            end
+          end
+        end
       end
     end
   end

spec/lib/banzai/filter/sanitization_filter_spec.rb

@@ -49,7 +49,7 @@ describe Banzai::Filter::SanitizationFilter do
       instance = described_class.new('Foo')
       3.times { instance.whitelist }
 
-      expect(instance.whitelist[:transformers].size).to eq 4
+      expect(instance.whitelist[:transformers].size).to eq 5
     end
 
     it 'sanitizes `class` attribute from all elements' do
@@ -63,8 +63,8 @@ describe Banzai::Filter::SanitizationFilter do
       expect(filter(act).to_html).to eq %q{<span>def</span>}
     end
 
-    it 'allows `style` attribute on table elements' do
-      html = <<-HTML.strip_heredoc
+    it 'allows `text-align` property in `style` attribute on table elements' do
+      html = <<~HTML
       <table>
         <tr><th style="text-align: center">Head</th></tr>
         <tr><td style="text-align: right">Body</th></tr>
@@ -77,6 +77,20 @@ describe Banzai::Filter::SanitizationFilter do
       expect(doc.at_css('td')['style']).to eq 'text-align: right'
     end
 
+    it 'disallows other properties in `style` attribute on table elements' do
+      html = <<~HTML
+        <table>
+          <tr><th style="text-align: foo">Head</th></tr>
+          <tr><td style="position: fixed; height: 50px; width: 50px; background: red; z-index: 999; font-size: 36px; text-align: center">Body</th></tr>
+        </table>
+      HTML
+
+      doc = filter(html)
+
+      expect(doc.at_css('th')['style']).to be_nil
+      expect(doc.at_css('td')['style']).to eq 'text-align: center'
+    end
+
     it 'allows `span` elements' do
       exp = act = %q{<span>Hello</span>}
       expect(filter(act).to_html).to eq exp