Fix XSS issue

Author: joeldrapper

Gemfile

@@ -7,7 +7,7 @@ gemspec
 
 group :test do
 	gem "sus"
-	gem "quickdraw", github: "joeldrapper/quickdraw"
+	gem "quickdraw", git: "https://github.com/joeldrapper/quickdraw.git", ref: "06615ef2554dabec4fbf6cf2848fb9493842fd05"
 	gem "simplecov", require: false
 	gem "selenium-webdriver"
 end

browser_tests.rb

@@ -184,6 +184,71 @@ def ignore_warnings
 	end
 end
 
+class EncodedJavaScriptLinks < Phlex::HTML
+	def view_template
+		render Layout do
+			ignore_warnings { a(href: "javascript&#58;alert(1)") { "x" } }
+			ignore_warnings { a(href: "java&#x73;cript:alert(1)") { "x" } }
+			ignore_warnings { a(href: "javascript&#58alert(1)") { "x" } }
+			ignore_warnings { a(href: "java&#x73cript:alert(1)") { "x" } }
+			ignore_warnings { a(href: "javascript&colon;alert(1)") { "x" } }
+		end
+	end
+
+	def ignore_warnings
+		yield
+	rescue ArgumentError
+		# ignore
+	end
+end
+
+class InjectedAttributeNames < Phlex::HTML
+	def view_template
+		render Layout do
+			ignore_warnings { div("x onclick": "alert(1)") { "x" } }
+			ignore_warnings { div("x/onclick": "alert(1)") { "x" } }
+		end
+	end
+
+	def ignore_warnings
+		yield
+	rescue ArgumentError
+		# ignore
+	end
+end
+
+class UnsafeCustomTagNames < Phlex::HTML
+	def view_template
+		render Layout do
+			ignore_warnings { tag(:"x-widget onclick=alert(1)") { "x" } }
+		end
+	end
+
+	def ignore_warnings
+		yield
+	rescue ArgumentError
+		# ignore
+	end
+end
+
+class UnsafeSvgXlinkHref < Phlex::HTML
+	def view_template
+		render Layout do
+			svg do |s|
+				ignore_warnings { s.a("xlink:href": "javascript:alert(1)") { "x" } }
+				ignore_warnings { s.a("xlink:href": "javascript&colon;alert(1)") { "x" } }
+				ignore_warnings { s.a("xlink:href": "javascript&#58alert(1)") { "x" } }
+			end
+		end
+	end
+
+	def ignore_warnings
+		yield
+	rescue ArgumentError
+		# ignore
+	end
+end
+
 class Browser
 	MUTEX = { safari: Mutex.new, chrome: Mutex.new, firefox: Mutex.new }
 
@@ -272,4 +337,32 @@ def quit
 	if browser.alert
 		raise "Failed with symbols"
 	end
+
+	browser.load_string(EncodedJavaScriptLinks.new.call)
+	browser.execute_script("document.querySelectorAll('a').forEach(function(a) { a.click(); });")
+	browser.each_alert do |alert|
+		unless alert.text == "Safari cannot open the page because the address is invalid."
+			raise "Failed with encoded javascript links"
+		end
+
+		alert.accept
+	end
+
+	browser.load_string(InjectedAttributeNames.new.call)
+	browser.execute_script("document.querySelectorAll('div').forEach(function(div) { div.click(); });")
+	browser.each_alert do
+		raise "Failed with injected attribute names"
+	end
+
+	browser.load_string(UnsafeCustomTagNames.new.call)
+	browser.execute_script("document.querySelectorAll('x-widget').forEach(function(node) { node.click(); });")
+	browser.each_alert do
+		raise "Failed with unsafe custom tag names"
+	end
+
+	browser.load_string(UnsafeSvgXlinkHref.new.call)
+	browser.execute_script("document.querySelectorAll('svg a').forEach(function(node) { node.dispatchEvent(new MouseEvent('click', { bubbles: true, cancelable: true, view: window })); });")
+	browser.each_alert do
+		raise "Failed with unsafe SVG xlink href"
+	end
 end

lib/phlex/html.rb

@@ -55,7 +55,7 @@ def tag(name, **attributes, &)
 			raise Phlex::ArgumentError.new("Expected the tag name to be a Symbol.")
 		end
 
-		if (tag = StandardElements.__registered_elements__[name]) || (tag = name.name.tr("_", "-")).include?("-")
+		if (tag = StandardElements.__registered_elements__[name]) || ((tag = name.name.tr("_", "-")).include?("-") && tag.match?(/\A[a-z0-9-]+\z/))
 			if attributes.length > 0 # with attributes
 				if block_given # with content block
 					buffer << "<#{tag}" << (Phlex::ATTRIBUTE_CACHE[attributes] ||= __attributes__(attributes)) << ">"

lib/phlex/sgml.rb

@@ -3,7 +3,13 @@
 # **Standard Generalized Markup Language** for behaviour common to {HTML} and {SVG}.
 class Phlex::SGML
 	UNSAFE_ATTRIBUTES = Set.new(%w[srcdoc sandbox http-equiv]).freeze
-	REF_ATTRIBUTES = Set.new(%w[href src action formaction lowsrc dynsrc background ping]).freeze
+	REF_ATTRIBUTES = Set.new(%w[href src action formaction lowsrc dynsrc background ping xlinkhref]).freeze
+	NAMED_CHARACTER_REFERENCES = {
+		"colon" => ":",
+		"tab" => "\t",
+		"newline" => "\n",
+	}.freeze
+	UNSAFE_ATTRIBUTE_NAME_CHARS = %r([<>&"'/=\s\x00])
 
 	ERBCompiler = ERB::Compiler.new("<>").tap do |compiler|
 		compiler.pre_cmd    = [""]
@@ -518,7 +524,9 @@ def __attributes__(attributes, buffer = +"")
 				if value != true && REF_ATTRIBUTES.include?(normalized_name)
 					case value
 					when String
-						if value.downcase.delete("^a-z:").start_with?("javascript:")
+						decoded_value = decode_html_character_references(value)
+
+						if decoded_value.downcase.delete("^a-z:").start_with?("javascript:")
 							# We just ignore these because they were likely not specified by the developer.
 							next
 						end
@@ -536,7 +544,7 @@ def __attributes__(attributes, buffer = +"")
 				end
 			end
 
-			if name.match?(/[<>&"']/)
+			if name.match?(UNSAFE_ATTRIBUTE_NAME_CHARS)
 				raise Phlex::ArgumentError.new("Unsafe attribute name detected: #{k}.")
 			end
 
@@ -572,7 +580,7 @@ def __nested_attributes__(attributes, base_name, buffer = +"")
 					else raise Phlex::ArgumentError.new("Attribute keys should be Strings or Symbols")
 				end
 
-				if name.match?(/[<>&"']/)
+				if name.match?(UNSAFE_ATTRIBUTE_NAME_CHARS)
 					raise Phlex::ArgumentError.new("Unsafe attribute name detected: #{k}.")
 				end
 			end
@@ -653,6 +661,27 @@ def __nested_tokens__(tokens)
 		buffer.gsub('"', "&quot;")
 	end
 
+	private def decode_html_character_references(value)
+		value
+			.gsub(/&#x([0-9a-f]+);?/i) {
+				begin
+					[$1.to_i(16)].pack("U*")
+				rescue
+					""
+				end
+			}
+			.gsub(/&#(\d+);?/) {
+				begin
+					[$1.to_i].pack("U*")
+				rescue
+					""
+				end
+			}
+			.gsub(/&([a-z][a-z0-9]+);?/i) {
+				NAMED_CHARACTER_REFERENCES[$1.downcase] || ""
+			}
+	end
+
 	# Result is **unsafe**, so it should be escaped!
 	def __styles__(styles)
 		case styles

lib/phlex/svg.rb

@@ -41,7 +41,7 @@ def tag(name, **attributes, &)
 			raise Phlex::ArgumentError.new("Expected the tag name to be a Symbol.")
 		end
 
-		if (tag = StandardElements.__registered_elements__[name]) || (tag = name.name.tr("_", "-")).include?("-")
+		if (tag = StandardElements.__registered_elements__[name]) || ((tag = name.name.tr("_", "-")).include?("-") && tag.match?(/\A[a-z0-9-]+\z/))
 			if attributes.length > 0 # with attributes
 				if block_given # with content block
 					buffer << "<#{tag}" << (Phlex::ATTRIBUTE_CACHE[attributes] ||= __attributes__(attributes)) << ">"

mise.toml

@@ -1,3 +1,6 @@
+[settings]
+idiomatic_version_file_enable_tools = ["ruby"]
+
 [tasks.setup]
 description = "Install all project dependencies."
 run = ["mise install", "bundle install --jobs 4"]

quickdraw/sgml/attributes.test.rb

@@ -45,11 +45,27 @@
 		phlex { a("Href" => "javascript:alert('hello')") },
 		phlex { a("Href" => "javascript:javascript:alert('hello')") },
 		phlex { a(href: " \t\njavascript:alert('hello')") },
+		phlex { a(href: "javascript:alert(1)") },
+		phlex { a(href: "javascript:alert(1)") },
+		phlex { a(href: "javascript:alert(1)") },
+		phlex { a(href: "javascript:alert(1)") },
+		phlex { a(href: "javascript&#58alert(1)") },
+		phlex { a(href: "javascript:alert(1)") },
 	].each do |output|
 		assert_equal_html output, %(<a></a>)
 	end
 end
 
+test "unsafe xlink:href attribute" do
+	[
+		phlex(Phlex::SVG) { a("xlink:href": "javascript:alert(1)") { "x" } },
+		phlex(Phlex::SVG) { a("xlink:href": "javascript&colon;alert(1)") { "x" } },
+		phlex(Phlex::SVG) { a("xlink:href": "javascript&#58alert(1)") { "x" } },
+	].each do |output|
+		assert_equal_html output, %(<a>x</a>)
+	end
+end
+
 test "unsafe attribute name <" do
 	error = assert_raises(Phlex::ArgumentError) do
 		phlex { div("<" => true) }
@@ -90,6 +106,38 @@
 	assert_equal error.message, "Unsafe attribute name detected: \"."
 end
 
+test "unsafe attribute name with space (String)" do
+	error = assert_raises(Phlex::ArgumentError) do
+		phlex { div("foo bar" => true) }
+	end
+
+	assert_equal error.message, "Unsafe attribute name detected: foo bar."
+end
+
+test "unsafe attribute name with space (Symbol)" do
+	error = assert_raises(Phlex::ArgumentError) do
+		phlex { div("foo bar": true) }
+	end
+
+	assert_equal error.message, "Unsafe attribute name detected: foo bar."
+end
+
+test "unsafe attribute name with slash (String)" do
+	error = assert_raises(Phlex::ArgumentError) do
+		phlex { div("foo/bar" => true) }
+	end
+
+	assert_equal error.message, "Unsafe attribute name detected: foo/bar."
+end
+
+test "unsafe attribute name with slash (Symbol)" do
+	error = assert_raises(Phlex::ArgumentError) do
+		phlex { div("foo/bar": true) }
+	end
+
+	assert_equal error.message, "Unsafe attribute name detected: foo/bar."
+end
+
 test "_, nil" do
 	output = phlex { div(attribute: nil) }
 	assert_equal_html output, %(<div></div>)

quickdraw/tag.test.rb

@@ -159,3 +159,27 @@ def view_template(&)
 		<custom-tag></custom-tag>
 	HTML
 end
+
+test "with unsafe custom tag name containing a space" do
+	error = assert_raises Phlex::ArgumentError do
+		HTMLComponent.call(:"x-widget onclick=alert(1)")
+	end
+
+	assert_equal error.message, "Invalid HTML tag: x-widget onclick=alert(1)"
+end
+
+test "with unsafe custom tag name containing special characters" do
+	error = assert_raises Phlex::ArgumentError do
+		HTMLComponent.call(:"x-widget>")
+	end
+
+	assert_equal error.message, "Invalid HTML tag: x-widget>"
+end
+
+test "with unsafe SVG custom tag name containing a space" do
+	error = assert_raises Phlex::ArgumentError do
+		SVGComponent.call(:"x-widget onclick=alert(1)")
+	end
+
+	assert_equal error.message, "Invalid SVG tag: x-widget onclick=alert(1)"
+end