Disallow invalid values to be set in XML::Node #5200

RX14 · 2017-10-28T15:16:08Z

Previously, invalid content and name values could be set for XML nodes, including values that had no valid encoding in XML (such as \0).

straight-shoota · 2017-10-28T15:44:43Z

src/xml/node.cr

+      raise XML::Error.new("Cannot escape string containing NUL character", 0)
+    end
+
+    string.gsub(SUBSTITUTIONS)


Why not use HTML.escape?

Because it's not HTML?

But it does the same thing, there is no need to implement this again. That's why XML.escape was removed in favour of HTML.escape.

Okay, it's not exactly the same: HTML.escape encodes ' as &x39; instead of ' because the named entity is not defined for HTML4. But it's still valid XML and no big deal. We could even consider changing the behaviour for HTML.escape because in other places (like HTML.unescape) we support only HTML5 anyway.

I disagree, I really don't want to use a module called HTML in a module for XML. It's simply wrong. They're different standards and people working on the HTML escape method won't think about XML when they're modifying it. Besides, it makes very little difference either way because both methods are unlikely to change for a long long time.

No, XML.escape was used because you shouldn't need it, because we have facilities to build XML built in. We don't have a HTML::Builder so HTML.escape has a purpose.

I wouldn't be against adding XML.escape back again though.

If the code is put in anyway (as suggested in this PR), it might as well be exposed in the public API.

I don't think that necessarily follows, personally.

Not necessarily, true. But if it's there - why not?

Because there's no point in adding a method as a public API when it's not needed. It means that if later we find some other way to implement this we might need to remove XML.escape and introduce a breaking change. Do you need XML.escape? Does someone else requested it? No. So please let's stick to this PRs goal, without adding more and more APIs. We can't keep adding API, we need stabilization for once.

asterite · 2017-10-30T17:18:08Z

@RX14 Are you sure this fixes the issue? XML::Builder doesn't build up nodes, it's an entirely different API. I think escape should be used in both places. Basically, this fixes something, but not exactly the issue in #4809

RX14 · 2017-10-30T18:13:53Z

@asterite I don't understand. XML::Builder is entirely unrelated to XML::Node and uses an entirely different libxml2 API. This PR simply uses the libxml2 API correctly by escaping manually the arguments to the right methods. I only looked at XML::Builder a little bit but it appears that XML api is designed differently and does escaping internally. Please correct me if i'm wrong.

asterite · 2017-10-30T18:44:20Z

@RX14 The snippet in the issue is:

# example.cr
require "xml"

xml = XML.build(indent: 2) do |xml|
  xml.element("example", number: "1") do
    xml.text "foo\0bar"
  end
  xml.element("example", number: "2") do
    xml.text "foo\u{08}bar"
  end
end

puts xml

Does it work well with this PR?

RX14 · 2017-10-30T18:49:33Z

Yeah, looks like I didn't read my own issue properly. This does solve a valid issue though, so i'll just update it to fix them all.

asterite · 2017-10-30T19:01:15Z

@RX14 To be honest, I wanted to solve this too and that's when I replied this. But that's also for nodes, not for XML::Builder... so we kinda did the same thing :-)

(well, except you actually fixed on of the issues)

RX14 · 2017-10-30T19:57:58Z

@asterite we essentially just need to raise on NUL for the builder, I think.

asterite · 2017-10-31T14:17:37Z

src/xml/node.cr

+
+    name = name.to_s
+
+    if name.includes?('\0') || LibXML.xmlValidateNameValue(name) == 0


Maybe we can call name.check_no_null_byte before this if

I'd rather that we raised a single exception type for the case in which the name is invalid, instead of different ones based on how it's invalid.

Sounds good. But for \0 a message like "contains a null byte" might be more helpful.

RX14 · 2017-11-04T02:25:36Z

This pr is finished and ready to be reviewed again btw

ysbaddaden · 2017-11-04T08:40:12Z

src/xml/builder.cr

@@ -275,8 +275,17 @@ 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?)


Just def string_to_unsafe(string : Nil) since you have the String overload below.

I was going to comment exactly the same thing

straight-shoota reviewed Oct 28, 2017

View reviewed changes

RX14 added kind:bug A bug in the code. Does not apply to documentation, specs, etc. topic:stdlib labels Oct 28, 2017

RX14 added the pr:wip label Oct 30, 2017

asterite reviewed Oct 31, 2017

View reviewed changes

Properly escape in XML::Node#content=

5f1d3e3

RX14 force-pushed the bugfix/xml-special-entities branch 2 times, most recently from 42dd787 to c2152fb Compare November 2, 2017 23:49

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

5eb8146

RX14 force-pushed the bugfix/xml-special-entities branch from c2152fb to 62a1929 Compare November 2, 2017 23:52

RX14 removed the pr:wip label Nov 4, 2017

ysbaddaden reviewed Nov 4, 2017

View reviewed changes

Error on null byte in XML::Builder

412ca48

RX14 force-pushed the bugfix/xml-special-entities branch from 62a1929 to 412ca48 Compare November 4, 2017 15:11

asterite approved these changes Nov 4, 2017

View reviewed changes

RX14 added this to the Next milestone Nov 4, 2017

RX14 merged commit 39b8e88 into crystal-lang:master Nov 4, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Disallow invalid values to be set in XML::Node #5200

Disallow invalid values to be set in XML::Node #5200

RX14 commented Oct 28, 2017

straight-shoota Oct 28, 2017

RX14 Oct 28, 2017

straight-shoota Oct 28, 2017

straight-shoota Oct 28, 2017

RX14 Oct 28, 2017

RX14 Oct 28, 2017 •

edited

Loading

straight-shoota Oct 28, 2017 •

edited

Loading

RX14 Oct 28, 2017

straight-shoota Oct 28, 2017

asterite Oct 29, 2017

asterite commented Oct 30, 2017

RX14 commented Oct 30, 2017 •

edited

Loading

asterite commented Oct 30, 2017

RX14 commented Oct 30, 2017

asterite commented Oct 30, 2017

RX14 commented Oct 30, 2017

asterite Oct 31, 2017

RX14 Oct 31, 2017

asterite Oct 31, 2017

RX14 commented Nov 4, 2017

ysbaddaden Nov 4, 2017

asterite Nov 4, 2017

RX14 Nov 4, 2017


		name = name.to_s

		if name.includes?('\0') \|\| LibXML.xmlValidateNameValue(name) == 0

Disallow invalid values to be set in XML::Node #5200

Disallow invalid values to be set in XML::Node #5200

Conversation

RX14 commented Oct 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RX14 Oct 28, 2017 • edited Loading

Choose a reason for hiding this comment

straight-shoota Oct 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asterite commented Oct 30, 2017

RX14 commented Oct 30, 2017 • edited Loading

asterite commented Oct 30, 2017

RX14 commented Oct 30, 2017

asterite commented Oct 30, 2017

RX14 commented Oct 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RX14 commented Nov 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RX14 Oct 28, 2017 •

edited

Loading

straight-shoota Oct 28, 2017 •

edited

Loading

RX14 commented Oct 30, 2017 •

edited

Loading