Simplify unsafe attributes detection (#797)

lib/phlex/html.rb

@@ -4,9 +4,6 @@ class Phlex::HTML < Phlex::SGML
 	autoload :StandardElements, "phlex/html/standard_elements"
 	autoload :VoidElements, "phlex/html/void_elements"
 
-	# A list of HTML attributes that have the potential to execute unsafe JavaScript.
-	UNSAFE_ATTRIBUTES = Set.new(%w[onabort onafterprint onbeforeprint onbeforeunload onblur oncanplay oncanplaythrough onchange onclick oncontextmenu oncopy oncuechange oncut ondblclick ondrag ondragend ondragenter ondragleave ondragover ondragstart ondrop ondurationchange onemptied onended onerror onfocus onhashchange oninput oninvalid onkeydown onkeypress onkeyup onload onloadeddata onloadedmetadata onloadstart onmessage onmousedown onmousemove onmouseout onmouseover onmouseup onmousewheel onoffline ononline onpagehide onpageshow onpaste onpause onplay onplaying onpopstate onprogress onratechange onreset onresize onscroll onsearch onseeked onseeking onselect onstalled onstorage onsubmit onsuspend ontimeupdate ontoggle onunload onvolumechange onwaiting onwheel srcdoc]).freeze
-
 	extend Phlex::SGML::Elements
 	include VoidElements, StandardElements
 

lib/phlex/sgml.rb

@@ -2,6 +2,9 @@
 
 # **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
+
 	autoload :Elements, "phlex/sgml/elements"
 	autoload :SafeObject, "phlex/sgml/safe_object"
 	autoload :SafeValue, "phlex/sgml/safe_value"
@@ -357,62 +360,75 @@ def __attributes__(attributes, buffer = +"")
 				else raise Phlex::ArgumentError.new("Attribute keys should be Strings or Symbols.")
 			end
 
-			lower_name = name.downcase
-
-			unless Phlex::SGML::SafeObject === v
-				if lower_name == "href" && v.to_s.downcase.delete("^a-z:").start_with?("javascript:")
-					next
-				end
-
-				# Detect unsafe attribute names. Attribute names are considered unsafe if they match an event attribute or include unsafe characters.
-				if Phlex::HTML::UNSAFE_ATTRIBUTES.include?(lower_name.delete("^a-z-"))
-					raise Phlex::ArgumentError.new("Unsafe attribute name detected: #{k}.")
-				end
-			end
-
-			if name.match?(/[<>&"']/)
-				raise Phlex::ArgumentError.new("Unsafe attribute name detected: #{k}.")
-			end
-
-			if lower_name.to_sym == :id && k != :id
-				raise Phlex::ArgumentError.new(":id attribute should only be passed as a lowercase symbol.")
-			end
-
-			case v
+			value = case v
 			when true
-				buffer << " " << name
+				true
 			when String
-				buffer << " " << name << '="' << v.gsub('"', "&quot;") << '"'
+				v.gsub('"', "&quot;")
 			when Symbol
-				buffer << " " << name << '="' << v.name.tr("_", "-").gsub('"', "&quot;") << '"'
+				v.name.tr("_", "-").gsub('"', "&quot;")
 			when Integer, Float
-				buffer << " " << name << '="' << v.to_s << '"'
+				v.to_s
 			when Hash
 				case k
 				when :style
-					buffer << " " << name << '="' << __styles__(v).gsub('"', "&quot;") << '"'
+					__styles__(v).gsub('"', "&quot;")
 				else
 					__nested_attributes__(v, "#{name}-", buffer)
 				end
 			when Array
 				case k
 				when :style
-					buffer << " " << name << '="' << __styles__(v).gsub('"', "&quot;") << '"'
+					__styles__(v).gsub('"', "&quot;")
 				else
-					buffer << " " << name << '="' << __nested_tokens__(v) << '"'
+					__nested_tokens__(v)
 				end
 			when Set
 				case k
 				when :style
-					buffer << " " << name << '="' << __styles__(v).gsub('"', "&quot;") << '"'
+					__styles__(v).gsub('"', "&quot;")
 				else
-					buffer << " " << name << '="' << __nested_tokens__(v.to_a) << '"'
+					__nested_tokens__(v.to_a)
 				end
 			when Phlex::SGML::SafeObject
-				buffer << " " << name << '="' << v.to_s.gsub('"', "&quot;") << '"'
+				v.to_s.gsub('"', "&quot;")
 			else
 				raise Phlex::ArgumentError.new("Invalid attribute value for #{k}: #{v.inspect}.")
 			end
+
+			lower_name = name.downcase
+
+			unless Phlex::SGML::SafeObject === v
+				normalized_name = lower_name.delete("^a-z-")
+
+				if value != true && REF_ATTRIBUTES.include?(normalized_name) && value.downcase.delete("^a-z:").start_with?("javascript:")
+					# We just ignore these because they were likely not specified by the developer.
+					next
+				end
+
+				if normalized_name.bytesize > 2 && normalized_name.start_with?("on") && !normalized_name.include?("-")
+					raise Phlex::ArgumentError.new("Unsafe attribute name detected: #{k}.")
+				end
+
+				if UNSAFE_ATTRIBUTES.include?(normalized_name)
+					raise Phlex::ArgumentError.new("Unsafe attribute name detected: #{k}.")
+				end
+			end
+
+			if name.match?(/[<>&"']/)
+				raise Phlex::ArgumentError.new("Unsafe attribute name detected: #{k}.")
+			end
+
+			if lower_name.to_sym == :id && k != :id
+				raise Phlex::ArgumentError.new(":id attribute should only be passed as a lowercase symbol.")
+			end
+
+			case value
+			when true
+				buffer << " " << name
+			when String
+				buffer << " " << name << '="' << value << '"'
+			end
 		end
 
 		buffer

test/phlex/view/naughty_business.rb

@@ -157,7 +157,7 @@ def view_template
 		end
 	end
 
-	Phlex::HTML::UNSAFE_ATTRIBUTES.each do |event_attribute|
+	Phlex::SGML::UNSAFE_ATTRIBUTES.each do |event_attribute|
 		with "with naughty #{event_attribute} attribute" do
 			naughty_attributes = { event_attribute => "alert(1);" }