From 2e427a052072f9973690e554d0ced4c0e7352dd7 Mon Sep 17 00:00:00 2001
From: Dan Milne <d@nmilne.com>
Date: Sat, 2 May 2026 23:57:22 +1000
Subject: [PATCH] Add SvgScrubber to strip XSS payloads from uploaded app icons
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Application#sanitize_svg_icon already runs a Loofah scrubber on every
icon upload, but the scrubber class itself was never tracked. Land it
along with tests covering the four shapes that matter:

- <script> elements stripped entirely
- on* event handlers (onload, onclick, …) removed but the carrying
  element preserved
- attribute values pointing at javascript:/data: URIs rejected
- benign icons round-trip unchanged

Writing the benign-icon test caught a real bug: the attribute allowlist
holds canonical SVG case (viewBox, preserveAspectRatio, gradientUnits,
…) but safe_attribute? downcases the incoming name before comparing,
so legitimate icons were silently losing those attributes on upload.
Fix by comparing against a precomputed lowercase lookup set; the
constant stays readable as canonical SVG case for documentation.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
---
 app/models/svg_scrubber.rb       | 73 ++++++++++++++++++++++++++++++++
 test/models/svg_scrubber_test.rb | 49 +++++++++++++++++++++
 2 files changed, 122 insertions(+)
 create mode 100644 app/models/svg_scrubber.rb
 create mode 100644 test/models/svg_scrubber_test.rb

diff --git a/app/models/svg_scrubber.rb b/app/models/svg_scrubber.rb
new file mode 100644
index 0000000..e109154
--- /dev/null
+++ b/app/models/svg_scrubber.rb
@@ -0,0 +1,73 @@
+# Loofah scrubber that strips dangerous content from SVG files
+# while preserving safe SVG elements and attributes for icon display.
+class SvgScrubber < Loofah::Scrubber
+  ALLOWED_ELEMENTS = %w[
+    svg g defs use symbol
+    circle ellipse line path polygon polyline rect
+    text tspan textPath
+    clipPath mask pattern
+    linearGradient radialGradient stop
+    filter feBlend feColorMatrix feComponentTransfer feComposite
+    feConvolveMatrix feDiffuseLighting feDisplacementMap feFlood
+    feGaussianBlur feImage feMerge feMergeNode feMorphology
+    feOffset feSpecularLighting feTile feTurbulence
+    title desc metadata
+  ].freeze
+
+  ALLOWED_ATTRIBUTES = %w[
+    id class style
+    x y x1 y1 x2 y2 cx cy r rx ry
+    width height viewBox preserveAspectRatio
+    d points
+    fill stroke stroke-width stroke-linecap stroke-linejoin stroke-dasharray
+    opacity fill-opacity stroke-opacity
+    transform translate rotate scale
+    font-family font-size font-weight text-anchor
+    clip-path mask filter
+    gradientUnits gradientTransform spreadMethod
+    offset stop-color stop-opacity
+    dx dy textLength lengthAdjust
+    xmlns xmlns:xlink
+    color display visibility overflow
+    fill-rule clip-rule
+    marker-start marker-mid marker-end
+  ].freeze
+
+  # Loofah hands attribute names back in their source case (e.g. "viewBox").
+  # Compare against a downcased copy so SVG-spec camelCase attributes aren't
+  # stripped from legitimate icons.
+  ALLOWED_ATTRIBUTES_LOOKUP = ALLOWED_ATTRIBUTES.map(&:downcase).to_set.freeze
+
+  # Event handler attributes that must always be removed
+  EVENT_HANDLER_PATTERN = /\Aon/i
+
+  def initialize
+    @direction = :top_down
+  end
+
+  def scrub(node)
+    return CONTINUE if node.text? || node.cdata?
+
+    if node.element?
+      if ALLOWED_ELEMENTS.include?(node.name)
+        # Remove disallowed and event handler attributes
+        node.attribute_nodes.each do |attr|
+          attr.remove unless safe_attribute?(attr)
+        end
+        return CONTINUE
+      end
+    end
+
+    node.remove
+    STOP
+  end
+
+  private
+
+  def safe_attribute?(attr)
+    name = attr.name.downcase
+    return false if name.match?(EVENT_HANDLER_PATTERN)
+    return false if attr.value&.match?(/javascript:|data:/i)
+    ALLOWED_ATTRIBUTES_LOOKUP.include?(name)
+  end
+end
diff --git a/test/models/svg_scrubber_test.rb b/test/models/svg_scrubber_test.rb
new file mode 100644
index 0000000..1bbf7c0
--- /dev/null
+++ b/test/models/svg_scrubber_test.rb
@@ -0,0 +1,49 @@
+require "test_helper"
+
+class SvgScrubberTest < ActiveSupport::TestCase
+  def scrub(svg)
+    Loofah.xml_document(svg).scrub!(SvgScrubber.new).to_xml
+  end
+
+  test "strips embedded script elements" do
+    svg = %(<svg xmlns="http://www.w3.org/2000/svg"><script>alert(1)</script><path d="M0 0"/></svg>)
+
+    cleaned = scrub(svg)
+
+    refute_match(/<script/i, cleaned)
+    refute_match(/alert/i, cleaned)
+    assert_match(/<path/, cleaned)
+  end
+
+  test "strips on* event handler attributes while preserving the element" do
+    svg = %(<svg xmlns="http://www.w3.org/2000/svg" onload="alert(1)"><circle cx="5" cy="5" r="3" onclick="steal()"/></svg>)
+
+    cleaned = scrub(svg)
+
+    refute_match(/onload/i, cleaned)
+    refute_match(/onclick/i, cleaned)
+    refute_match(/alert|steal/, cleaned)
+    assert_match(/<svg/, cleaned)
+    assert_match(/<circle/, cleaned)
+  end
+
+  test "strips attribute values that point at javascript: or data: URIs" do
+    svg = %(<svg xmlns="http://www.w3.org/2000/svg"><a href="javascript:alert(1)"><path d="M0 0" fill="data:text/html,evil"/></a></svg>)
+
+    cleaned = scrub(svg)
+
+    refute_match(/javascript:/i, cleaned)
+    refute_match(/data:text\/html/i, cleaned)
+  end
+
+  test "preserves a benign icon unchanged in shape" do
+    svg = %(<svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 24 24"><path d="M12 2 L22 22 L2 22 Z" fill="#000"/></svg>)
+
+    cleaned = scrub(svg)
+
+    assert_match(/<svg/, cleaned)
+    assert_match(/<path/, cleaned)
+    assert_match(/M12 2 L22 22 L2 22 Z/, cleaned)
+    assert_match(/viewBox="0 0 24 24"/, cleaned)
+  end
+end