Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
Disallow invalid values to be set in XML::Node (#5200)
* Properly escape in XML::Node#content=

* Don't allow invalid node names in XML::Node#name=

* Error on null byte in XML::Builder
  • Loading branch information
RX14 committed Nov 4, 2017
1 parent 94480d9 commit 39b8e88
Show file tree
Hide file tree
Showing 5 changed files with 109 additions and 19 deletions.
18 changes: 18 additions & 0 deletions spec/std/xml/builder_spec.cr
Expand Up @@ -195,4 +195,22 @@ describe XML::Builder do
io.rewind
io.to_s.should eq("<?xml version=\"1.0\"?>\n<foo id=\"1\">hello</foo>\n")
end

it "errors on null byte" do
expect_raises(XML::Error, "String cannot contain null character") do
XML.build do |xml|
xml.element("example", number: "1") do
xml.text "foo\0bar"
end
end
end

expect_raises(XML::Error, "String cannot contain null character") do
XML.build do |xml|
xml.element("exam\0ple", number: "1") do
xml.text "foobar"
end
end
end
end
end
41 changes: 39 additions & 2 deletions spec/std/xml/xml_spec.cr
Expand Up @@ -225,8 +225,20 @@ describe XML do
root.text = "Peter"
root.text.should eq("Peter")

root.content = "Foo"
root.content.should eq("Foo")
root.content = "Foo 👌"
root.content.should eq("Foo 👌")
end

it "doesn't set invalid node content" do
doc = XML.parse(<<-XML
<?xml version='1.0' encoding='UTF-8'?>
<name>John</name>
XML
)
root = doc.root.not_nil!
expect_raises(Exception, "Cannot escape") do
root.content = "\0"
end
end

it "gets empty content" do
Expand All @@ -245,6 +257,31 @@ describe XML do
root.name.should eq("last-name")
end

it "doesn't set invalid node name" do
doc = XML.parse(<<-XML
<?xml version='1.0' encoding='UTF-8'?>
<name>John</name>
XML
)
root = doc.root.not_nil!

expect_raises(XML::Error, "Invalid node name") do
root.name = " foo bar"
end

expect_raises(XML::Error, "Invalid node name") do
root.name = "foo bar"
end

expect_raises(XML::Error, "Invalid node name") do
root.name = "1foo"
end

expect_raises(XML::Error, "Invalid node name") do
root.name = "\0foo"
end
end

it "gets encoding" do
doc = XML.parse(<<-XML
<?xml version='1.0' encoding='UTF-8'?>
Expand Down
35 changes: 20 additions & 15 deletions src/xml/builder.cr
Expand Up @@ -27,7 +27,7 @@ struct XML::Builder

# Emits the start of the document.
def start_document(version = nil, encoding = nil) : Nil
call StartDocument, version, encoding, nil
call StartDocument, string_to_unsafe(version), string_to_unsafe(encoding), nil
end

# Emits the end of a document.
Expand All @@ -44,12 +44,12 @@ struct XML::Builder

# Emits the start of an element.
def start_element(name : String) : Nil
call StartElement, name
call StartElement, string_to_unsafe(name)
end

# Emits the start of an element with namespace info.
def start_element(prefix : String?, name : String, namespace_uri : String?) : Nil
call StartElementNS, string_to_unsafe(prefix), name, string_to_unsafe(namespace_uri)
call StartElementNS, string_to_unsafe(prefix), string_to_unsafe(name), string_to_unsafe(namespace_uri)
end

# Emits the end of an element.
Expand Down Expand Up @@ -111,12 +111,12 @@ struct XML::Builder

# Emits the start of an attribute.
def start_attribute(name : String) : Nil
call StartAttribute, name
call StartAttribute, string_to_unsafe(name)
end

# Emits the start of an attribute with namespace info.
def start_attribute(prefix : String?, name : String, namespace_uri : String?)
call StartAttributeNS, string_to_unsafe(prefix), name, string_to_unsafe(namespace_uri)
call StartAttributeNS, string_to_unsafe(prefix), string_to_unsafe(name), string_to_unsafe(namespace_uri)
end

# Emits the end of an attribute.
Expand All @@ -133,12 +133,12 @@ struct XML::Builder

# Emits an attribute with a *value*.
def attribute(name : String, value) : Nil
call WriteAttribute, name, value.to_s
call WriteAttribute, string_to_unsafe(name), string_to_unsafe(value.to_s)
end

# Emits an attribute with namespace info and a *value*.
def attribute(prefix : String?, name : String, namespace_uri : String?, value) : Nil
call WriteAttributeNS, string_to_unsafe(prefix), name, string_to_unsafe(namespace_uri), value.to_s
call WriteAttributeNS, string_to_unsafe(prefix), string_to_unsafe(name), string_to_unsafe(namespace_uri), string_to_unsafe(value.to_s)
end

# Emits the given *attributes* with their values.
Expand All @@ -157,7 +157,7 @@ struct XML::Builder
#
# Text content can happen inside of an `element`, `attribute` value, `cdata`, `dtd`, etc.
def text(content : String) : Nil
call WriteString, content
call WriteString, string_to_unsafe(content)
end

# Emits the start of a `CDATA` section.
Expand All @@ -179,7 +179,7 @@ struct XML::Builder

# Emits a `CDATA` section.
def cdata(text : String) : Nil
call WriteCDATA, text
call WriteCDATA, string_to_unsafe(text)
end

# Emits the start of a comment.
Expand All @@ -201,12 +201,12 @@ struct XML::Builder

# Emits a comment.
def comment(text : String) : Nil
call WriteComment, text
call WriteComment, string_to_unsafe(text)
end

# Emits the start of a `DTD`.
def start_dtd(name : String, pubid : String, sysid : String) : Nil
call StartDTD, name, pubid, sysid
call StartDTD, string_to_unsafe(name), string_to_unsafe(pubid), string_to_unsafe(sysid)
end

# Emits the end of a `DTD`.
Expand All @@ -223,7 +223,7 @@ struct XML::Builder

# Emits a `DTD`.
def dtd(name : String, pubid : String, sysid : String, subset : String? = nil) : Nil
call WriteDTD, name, pubid, sysid, string_to_unsafe(subset)
call WriteDTD, string_to_unsafe(name), string_to_unsafe(pubid), string_to_unsafe(sysid), string_to_unsafe(subset)
end

# Emits a namespace.
Expand All @@ -243,7 +243,7 @@ struct XML::Builder
call SetIndent, 0
else
call SetIndent, 1
call SetIndentString, str
call SetIndentString, string_to_unsafe(str)
end
end

Expand Down Expand Up @@ -275,8 +275,13 @@ struct XML::Builder
raise XML::Error.new("Error in #{msg}", 0) if ret < 0
end

private def string_to_unsafe(string)
string ? string.to_unsafe : Pointer(UInt8).null
private def string_to_unsafe(string : String)
raise XML::Error.new("String cannot contain null character", 0) if string.includes? '\0'
string.to_unsafe
end

private def string_to_unsafe(string : Nil)
Pointer(UInt8).null
end
end

Expand Down
2 changes: 2 additions & 0 deletions src/xml/libxml2.cr
Expand Up @@ -294,6 +294,8 @@ lib LibXML
fun xmlGetNsList(doc : Doc*, node : Node*) : NS**

fun xmlSetProp(node : Node*, name : UInt8*, value : UInt8*) : Attr*

fun xmlValidateNameValue(value : UInt8*) : Int
end

LibXML.xmlGcMemSetup(
Expand Down
32 changes: 30 additions & 2 deletions src/xml/node.cr
Expand Up @@ -97,7 +97,8 @@ struct XML::Node
# Sets the Node's content to a Text node containing string.
# The string gets XML escaped, not interpreted as markup.
def content=(content)
LibXML.xmlNodeSetContent(self, content.to_s)
content = escape(content.to_s)
LibXML.xmlNodeSetContent(self, content)
end

# Gets the document for this Node as a `XML::Node`.
Expand Down Expand Up @@ -273,7 +274,18 @@ struct XML::Node
if document? || text? || cdata? || fragment?
raise XML::Error.new("Can't set name of XML #{type}", 0)
end
LibXML.xmlNodeSetName(self, name.to_s)

name = name.to_s

if name.includes? '\0'
raise XML::Error.new("Invalid node name: #{name.inspect} (contains null character)", 0)
end

if LibXML.xmlValidateNameValue(name) == 0
raise XML::Error.new("Invalid node name: #{name.inspect}", 0)
end

LibXML.xmlNodeSetName(self, name)
end

# Returns the namespace for this node or `nil` if not found.
Expand Down Expand Up @@ -544,4 +556,20 @@ struct XML::Node
ptr = @node.value._private
ptr ? (ptr.as(Array(XML::Error))) : nil
end

private SUBSTITUTIONS = {
'>' => "&gt;",
'<' => "&lt;",
'"' => "&quot;",
'\'' => "&apos;",
'&' => "&amp;",
}

private def escape(string)
if string.includes? '\0'
raise XML::Error.new("Cannot escape string containing null character", 0)
end

string.gsub(SUBSTITUTIONS)
end
end

0 comments on commit 39b8e88

Please sign in to comment.