Add SvgScrubber to strip XSS payloads from uploaded app icons
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>
This commit is contained in:
73
app/models/svg_scrubber.rb
Normal file
73
app/models/svg_scrubber.rb
Normal file
@@ -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
|
||||||
49
test/models/svg_scrubber_test.rb
Normal file
49
test/models/svg_scrubber_test.rb
Normal file
@@ -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
|
||||||
Reference in New Issue
Block a user