Skip to content

Commit 9f56ad1

Browse files
joeldrapperjoeldrapper
committed
Fix XSS issues
1 parent 90c6fbc commit 9f56ad1

6 files changed

Lines changed: 200 additions & 6 deletions

File tree

‎browser_tests.rb‎

Lines changed: 93 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -179,6 +179,71 @@ def ignore_warnings
179179
end
180180
end
181181

182+
class EncodedJavaScriptLinks < Phlex::HTML
183+
def view_template
184+
render Layout do
185+
ignore_warnings { a(href: "javascript&#58;alert(1)") { "x" } }
186+
ignore_warnings { a(href: "java&#x73;cript:alert(1)") { "x" } }
187+
ignore_warnings { a(href: "javascript&#58alert(1)") { "x" } }
188+
ignore_warnings { a(href: "java&#x73cript:alert(1)") { "x" } }
189+
ignore_warnings { a(href: "javascript&colon;alert(1)") { "x" } }
190+
end
191+
end
192+
193+
def ignore_warnings
194+
yield
195+
rescue ArgumentError
196+
# ignore
197+
end
198+
end
199+
200+
class InjectedAttributeNames < Phlex::HTML
201+
def view_template
202+
render Layout do
203+
ignore_warnings { div("x onclick": "alert(1)") { "x" } }
204+
ignore_warnings { div("x/onclick": "alert(1)") { "x" } }
205+
end
206+
end
207+
208+
def ignore_warnings
209+
yield
210+
rescue ArgumentError
211+
# ignore
212+
end
213+
end
214+
215+
class UnsafeCustomTagNames < Phlex::HTML
216+
def view_template
217+
render Layout do
218+
ignore_warnings { tag(:"x-widget onclick=alert(1)") { "x" } }
219+
end
220+
end
221+
222+
def ignore_warnings
223+
yield
224+
rescue ArgumentError
225+
# ignore
226+
end
227+
end
228+
229+
class UnsafeSvgXlinkHref < Phlex::HTML
230+
def view_template
231+
render Layout do
232+
svg do |s|
233+
ignore_warnings { s.a("xlink:href": "javascript:alert(1)") { "x" } }
234+
ignore_warnings { s.a("xlink:href": "javascript&colon;alert(1)") { "x" } }
235+
ignore_warnings { s.a("xlink:href": "javascript&#58alert(1)") { "x" } }
236+
end
237+
end
238+
end
239+
240+
def ignore_warnings
241+
yield
242+
rescue ArgumentError
243+
# ignore
244+
end
245+
end
246+
182247
class Browser
183248
MUTEX = { safari: Mutex.new, chrome: Mutex.new, firefox: Mutex.new }
184249

@@ -267,4 +332,32 @@ def quit
267332
if browser.alert
268333
raise "Failed with symbols"
269334
end
335+
336+
browser.load_string(EncodedJavaScriptLinks.new.call)
337+
browser.execute_script("document.querySelectorAll('a').forEach(function(a) { a.click(); });")
338+
browser.each_alert do |alert|
339+
unless alert.text == "Safari cannot open the page because the address is invalid."
340+
raise "Failed with encoded javascript links"
341+
end
342+
343+
alert.accept
344+
end
345+
346+
browser.load_string(InjectedAttributeNames.new.call)
347+
browser.execute_script("document.querySelectorAll('div').forEach(function(div) { div.click(); });")
348+
browser.each_alert do
349+
raise "Failed with injected attribute names"
350+
end
351+
352+
browser.load_string(UnsafeCustomTagNames.new.call)
353+
browser.execute_script("document.querySelectorAll('x-widget').forEach(function(node) { node.click(); });")
354+
browser.each_alert do
355+
raise "Failed with unsafe custom tag names"
356+
end
357+
358+
browser.load_string(UnsafeSvgXlinkHref.new.call)
359+
browser.execute_script("document.querySelectorAll('svg a').forEach(function(node) { node.dispatchEvent(new MouseEvent('click', { bubbles: true, cancelable: true, view: window })); });")
360+
browser.each_alert do
361+
raise "Failed with unsafe SVG xlink href"
362+
end
270363
end

‎lib/phlex/html.rb‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -55,7 +55,7 @@ def tag(name, **attributes, &)
5555
raise Phlex::ArgumentError.new("Expected the tag name to be a Symbol.")
5656
end
5757

58-
if (tag = StandardElements.__registered_elements__[name]) || (tag = name.name.tr("_", "-")).include?("-")
58+
if (tag = StandardElements.__registered_elements__[name]) || ((tag = name.name.tr("_", "-")).include?("-") && tag.match?(/\A[a-z0-9-]+\z/))
5959
if attributes.length > 0 # with attributes
6060
if block_given # with content block
6161
buffer << "<#{tag}" << (Phlex::ATTRIBUTE_CACHE[attributes] ||= Phlex::SGML::Attributes.generate_attributes(attributes)) << ">"

‎lib/phlex/sgml/attributes.rb‎

Lines changed: 33 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,13 @@ module Phlex::SGML::Attributes
44
extend self
55

66
UNSAFE_ATTRIBUTES = Set.new(%w[srcdoc sandbox http-equiv]).freeze
7-
REF_ATTRIBUTES = Set.new(%w[href src action formaction lowsrc dynsrc background ping]).freeze
7+
REF_ATTRIBUTES = Set.new(%w[href src action formaction lowsrc dynsrc background ping xlinkhref]).freeze
8+
NAMED_CHARACTER_REFERENCES = {
9+
"colon" => ":",
10+
"tab" => "\t",
11+
"newline" => "\n",
12+
}.freeze
13+
UNSAFE_ATTRIBUTE_NAME_CHARS = %r([<>&"'/=\s\x00])
814

915
def generate_attributes(attributes, buffer = +"")
1016
attributes.each do |k, v|
@@ -68,7 +74,9 @@ def generate_attributes(attributes, buffer = +"")
6874
if value != true && REF_ATTRIBUTES.include?(normalized_name)
6975
case value
7076
when String
71-
if value.downcase.delete("^a-z:").start_with?("javascript:")
77+
decoded_value = decode_html_character_references(value)
78+
79+
if decoded_value.downcase.delete("^a-z:").start_with?("javascript:")
7280
# We just ignore these because they were likely not specified by the developer.
7381
next
7482
end
@@ -86,7 +94,7 @@ def generate_attributes(attributes, buffer = +"")
8694
end
8795
end
8896

89-
if name.match?(/[<>&"']/)
97+
if name.match?(UNSAFE_ATTRIBUTE_NAME_CHARS)
9098
raise Phlex::ArgumentError.new("Unsafe attribute name detected: #{k}.")
9199
end
92100

@@ -122,7 +130,7 @@ def generate_nested_attributes(attributes, base_name, buffer = +"")
122130
else raise Phlex::ArgumentError.new("Attribute keys should be Strings or Symbols")
123131
end
124132

125-
if name.match?(/[<>&"']/)
133+
if name.match?(UNSAFE_ATTRIBUTE_NAME_CHARS)
126134
raise Phlex::ArgumentError.new("Unsafe attribute name detected: #{k}.")
127135
end
128136
end
@@ -160,6 +168,27 @@ def generate_nested_attributes(attributes, base_name, buffer = +"")
160168
end
161169
end
162170

171+
def decode_html_character_references(value)
172+
value
173+
.gsub(/&#x([0-9a-f]+);?/i) {
174+
begin
175+
[$1.to_i(16)].pack("U*")
176+
rescue
177+
""
178+
end
179+
}
180+
.gsub(/&#(\d+);?/) {
181+
begin
182+
[$1.to_i].pack("U*")
183+
rescue
184+
""
185+
end
186+
}
187+
.gsub(/&([a-z][a-z0-9]+);?/i) {
188+
NAMED_CHARACTER_REFERENCES[$1.downcase] || ""
189+
}
190+
end
191+
163192
def generate_nested_tokens(tokens, sep = " ", gsub_from = nil, gsub_to = "")
164193
buffer = +""
165194

‎lib/phlex/svg.rb‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,7 @@ def tag(name, **attributes, &)
4141
raise Phlex::ArgumentError.new("Expected the tag name to be a Symbol.")
4242
end
4343

44-
if (tag = StandardElements.__registered_elements__[name]) || (tag = name.name.tr("_", "-")).include?("-")
44+
if (tag = StandardElements.__registered_elements__[name]) || ((tag = name.name.tr("_", "-")).include?("-") && tag.match?(/\A[a-z0-9-]+\z/))
4545
if attributes.length > 0 # with attributes
4646
if block_given # with content block
4747
buffer << "<#{tag}" << (Phlex::ATTRIBUTE_CACHE[attributes] ||= Phlex::SGML::Attributes.generate_attributes(attributes)) << ">"

‎quickdraw/sgml/attributes.test.rb‎

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -56,11 +56,27 @@ class AttributesTest < Quickdraw::Test
5656
phlex { a("Href" => "javascript:alert('hello')") },
5757
phlex { a("Href" => "javascript:javascript:alert('hello')") },
5858
phlex { a(href: " \t\njavascript:alert('hello')") },
59+
phlex { a(href: "java&#x73;cript:alert(1)") },
60+
phlex { a(href: "javascript&#58;alert(1)") },
61+
phlex { a(href: "java&#115;cript:alert(1)") },
62+
phlex { a(href: "&#106;avascript:alert(1)") },
63+
phlex { a(href: "javascript&#58alert(1)") },
64+
phlex { a(href: "javascript&colon;alert(1)") },
5965
].each do |output|
6066
assert_equal output, %(<a></a>)
6167
end
6268
end
6369

70+
test "unsafe xlink:href attribute" do
71+
[
72+
phlex(Phlex::SVG) { a("xlink:href": "javascript:alert(1)") { "x" } },
73+
phlex(Phlex::SVG) { a("xlink:href": "javascript&colon;alert(1)") { "x" } },
74+
phlex(Phlex::SVG) { a("xlink:href": "javascript&#58alert(1)") { "x" } },
75+
].each do |output|
76+
assert_equal output, %(<a>x</a>)
77+
end
78+
end
79+
6480
test "unsafe attribute name <" do
6581
error = assert_raises(Phlex::ArgumentError) do
6682
phlex { div("<" => true) }
@@ -101,6 +117,38 @@ class AttributesTest < Quickdraw::Test
101117
assert_equal error.message, "Unsafe attribute name detected: \"."
102118
end
103119

120+
test "unsafe attribute name with space (String)" do
121+
error = assert_raises(Phlex::ArgumentError) do
122+
phlex { div("foo bar" => true) }
123+
end
124+
125+
assert_equal error.message, "Unsafe attribute name detected: foo bar."
126+
end
127+
128+
test "unsafe attribute name with space (Symbol)" do
129+
error = assert_raises(Phlex::ArgumentError) do
130+
phlex { div("foo bar": true) }
131+
end
132+
133+
assert_equal error.message, "Unsafe attribute name detected: foo bar."
134+
end
135+
136+
test "unsafe attribute name with slash (String)" do
137+
error = assert_raises(Phlex::ArgumentError) do
138+
phlex { div("foo/bar" => true) }
139+
end
140+
141+
assert_equal error.message, "Unsafe attribute name detected: foo/bar."
142+
end
143+
144+
test "unsafe attribute name with slash (Symbol)" do
145+
error = assert_raises(Phlex::ArgumentError) do
146+
phlex { div("foo/bar": true) }
147+
end
148+
149+
assert_equal error.message, "Unsafe attribute name detected: foo/bar."
150+
end
151+
104152
test "_, nil" do
105153
output = phlex { div(attribute: nil) }
106154
assert_equal output, %(<div></div>)

‎quickdraw/tag.test.rb‎

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -160,4 +160,28 @@ def view_template(&)
160160
<custom-tag></custom-tag>
161161
HTML
162162
end
163+
164+
test "with unsafe custom tag name containing a space" do
165+
error = assert_raises Phlex::ArgumentError do
166+
HTMLComponent.call(:"x-widget onclick=alert(1)")
167+
end
168+
169+
assert_equal error.message, "Invalid HTML tag: x-widget onclick=alert(1)"
170+
end
171+
172+
test "with unsafe custom tag name containing special characters" do
173+
error = assert_raises Phlex::ArgumentError do
174+
HTMLComponent.call(:"x-widget>")
175+
end
176+
177+
assert_equal error.message, "Invalid HTML tag: x-widget>"
178+
end
179+
180+
test "with unsafe SVG custom tag name containing a space" do
181+
error = assert_raises Phlex::ArgumentError do
182+
SVGComponent.call(:"x-widget onclick=alert(1)")
183+
end
184+
185+
assert_equal error.message, "Invalid SVG tag: x-widget onclick=alert(1)"
186+
end
163187
end

0 commit comments

Comments
 (0)