#11804 Remove > character replacement in escaped comments

[ghost]

Fixes #11804

Email templates for MS Outlook require conditional CSS. The closing tag for those is a > character, and _flatten.escapedComment was replacing that character for its HTML counterpart. That was in turn preventing the conditional CSS blocks from closing causing the ghost code to appear superimposed in the body of Outlook emails.

Emails sent to MSO users are displaying the code for the conditional CSS:

If you look closely you’ll see that the conditional if block looks like this: <!--[if (gte mso 9)|(IE)]&gt;, and that the closing bracket, >, has been converted to >.
As a result, the transformation of > into it's HTML counterpart, > is not allowing the if block to adequately close.

The source of that transformation was introduced in this PR

In the docstring of test_commentEscaping this statement was made: "Comments start with and never contain -- or >." That is not correct given that we close if statements with a >
i.e. <!--[if (gte mso 9)|(IE)]>

Outlook conditional CSS overview

---

To solve this issue, this PR adds a helper function that detects if the code being parsed contains Outlook conditional CSS, and then uses that to determine whether the > character should be escaped or not when parsing comments. It also adds tests to make sure the behavior is correct and does not break/alter anything else.

[adiroiban]

Many thanks for the PR. Don't forget to remote the "Draft marker" once this is ready for review.

Also, this needs a release notes fragment, in the form of a file like src/twisted/newsfragmetns/11804.bugfix that will describe the change library's to end users.

I am not sure that this change is ok.

From that I see in the previous PR, the comments were transformed into normal content in order to keep the XML structure valid.

I checked this SO question https://stackoverflow.com/questions/2640453/single-line-comment-in-html ... but the focus there is mostly on HTML, while the first answer is generic SGML

and reviewed the initial issue report #5275

---

The change might be ok, but it need better testing.

It needs at least one new test that explains the new behaviour

---

My undestanding of this issue is this.

The current twisted code tried to make "invalid SGML" comments into valid "SGML content"
But from what I see, it converts any comment (valid or invalid) into SGML content.

So. One way to fix this is to recognizie invalid comments and convert them into content.
While, anything that looks like valid SGML comment should be kept in place.

I am not an expert into XML/HTML markup, so we might need to wait for another team member to take a look

Thanks again

[felipemelendez]

Thank you for your prompt response, I will include the release notes fragment. Also, we are parsing XHTML and solved this issue by monkey patching _flatten.escapedComment, but following your arguments, it seems that I might have disregarded the fact that this method is also used to escape HTML comments? If that is the case the method needs to be further developed to take this into consideration, and conditionally escape the > depending on whether the content is HTML or XML, and a test needs to follow suit.

[adiroiban]

Based on latest conversation this needs changes.

Let me know when this is ready again for a new revew.

Thanks again

[felipemelendez]

After careful consideration, the problem to address was the detection and proper handling of MSO (Microsoft Office) conditional CSS, and not XML code as a whole.
To that end I have created a helper function that detects if the data parsed by escapedComment contains MSO tags so that it may correctly escape the appropriate characters.
I read several docs and this one summarizes the requirements: Outlook Conditional CSS rules of engagement
In addition, I revamped the test for escapedComment
I also tested my work with production.

[felipemelendez]

please review

[felipemelendez]

because c was used as a variable name, whenever I tested the output with a breakpoint and pressed c to check the content of the variable "continue" was activated. Perhaps that is why I mishandled my revision of the test cases? Not sure, but for some reason I woke up with that realization, checked, and found an issue with the test - this now works as expected.

[adiroiban]

why not rename it to "comment" or "targetedComment" or "expectedComment" or something that will explain what this variable is all about.

I am not a mathematician, and I don't like single letter variables :)

[felipemelendez]

#11809 will fix the isort errors that are causing the CI checks to break

[felipemelendez]

well @adiroiban, I have addressed the changes requested in your previous comments:

• I have written a docstring that explains the purpose of _hasMSOComments
• I added to the docstring of escapedComment to explain the presence of _hasMSOComments
• I fixed the issue with the handling of the call to _hasMSOComments, particularly in that because data is converted to bytes before _hasMSOComments the type of its argument is always bytes.
• I created a test function that specifically validates the new functionality added by _hasMSOComments
• the new test function has a proper docstring that will help developers understand its intent
• with these changes we are now at 100%

Please let me know if there's anything else that needs to be addressed before we can merge this pull request, I would be happy to address it. But first let's enjoy the rest of our Sunday 😎

[exarkun]

Why isn't the whole change:

     data = data.replace(b"-->", b"-->")

[felipemelendez]

Fair question, this is done so that we may precisely follow the recommendations set by w3 regarding the Extensible Markup Language (XML). There is a lot to this, and that's why we find ourselves fine-tuning from time to time, so that we may adhere to these rules and thereby ensure language compatibility.

• The part about the > can be found in the Character Data and Markup section. There we find this rule:
  "The right angle bracket (>) may be represented using the string >, and must, for compatibility, be escaped using either > or a character reference when it appears in the string ]]> in content, when that string is not marking the end of a CDATA section."

• In regards to the --, the rules for that can be found in the Comments section. There we find this:
  "For compatibility, the string -- (double-hyphen) must not occur within comments"

So, if we were to just replace --> we would miss cases where we encounter standalone instances of -- or >

[exarkun]

Okay. Indeed, there is a complex and partially contradictory set of rules that apply - the rules of XML, the rules of SGML, and whatever rules MSIE is following. Additionally, twisted.web.template doesn't even clearly state what its output is meant to be (in some places it talks about HTML, in some places it talks about XML, in other places it implies HTML5).

Still, the actual problem for MSIE is that ">" gets escaped inside comments. The change I suggested would make MSIE happy but it would leave the output potentially in violation of the SGML comment rule that comments may not include "--".

This would make SGML and MSIE happy:

    data = data.replace(b"--", b"- - ")

That is, drop the ">" quoting from the current behavior but leave the other part alone. This fixes the MSIE behavior by leaving ">" alone. I don't think it breaks XML. XML doesn't actually dictate that you quote this character inside a comment. Instead, it says that the contents of comments don't even have to be parsed by an XML parser (https://www.w3.org/TR/REC-xml/#dt-comment) - and the associate example even includes unquoted "<", ">", and "&".

I don't know what SGML makes of this behavior but I also wonder if it actually matters. In practice twisted.web.template is not for emitting SGML except for the special case of HTML (which may or may not sometimes be an SGML subset). The only reference to SGML in all of Twisted is, in fact, the test for this existing behavior. It at least avoids the most serious issue SGML has - it gets rid of any "--" inside comments.

[felipemelendez]

Point well taken, in fact, here is the snippet that we used to monkey patch around this issue:

def escapedComment(data: bytes | str) -> bytes:
    """
    Same as _flatten.escapedComment but with .replace(b">", b">") removed
    Twisted issue #11804: https://github.com/twisted/twisted/issues/11804
    """
    if isinstance(data, str):
        data = data.encode("utf-8")
    data = data.replace(b"--", b"- - ")
    if data and data[-1:] == b"-":
        data += b" "
    return data

But, to be fair, our use case is, in the grand scheme of things, relatively narrow. The problem here is that in XML, the > character has a special meaning as the closing bracket for elements. If the > character is used inside an XML element as text, it could be misinterpreted by an XML parser as the end of the element, causing the parser to generate an error. By using character references, we can avoid any potential issues with the parser misinterpreting the characters.

The HTML specification document summarizes these rules in a way that is a little more succinct: https://html.spec.whatwg.org/multipage/syntax.html#comments

There, you'll find that:
text must not start with the string >, nor start with the string ->, nor contain the strings <!--, -->, or --!>, nor end with the string <!-.

I reference the HTML specification document because XML has a stricter syntax that requires well-formedness, while HTML has a more relaxed syntax, and still the HTML doc makes this point. While there are some differences between HTML and XML, both use similar rules for certain elements such as comments, and the use of the > character is one such example. The HTML specification document provides a concise and clear summary of the rules for comments in HTML, which is helpful in understanding the potential issues with using the > character in comments.

[exarkun]

There, you'll find that: text must not start with the string >, nor start with the string ->, nor contain the strings , or --!>, nor end with the string <!-.

Perhaps this is an important rule to implement? I notice that at least one these cases ("nor end with the string <!-") is handled by neither the existing code nor the proposed change.

More generally, the idea I'm wondering about is why we don't just implement the XML and HTML rules for comments instead of adding a couple hundred lines of code specific to MSIE.

Correct me if I'm wrong here: these MSIE comments aren't illegal according to any of the relevant specs.

If that's true then the problem here is that the comment mangling code Comment uses to try to avoid the illegal cases incorrectly mangles some legal content. The solution that makes sense to me is to stop mangling any legal content and only mangle illegal content. The solution in this PR seems like it is still mangling a mix of legal and illegal cases (for example, x> is a legal comment but before and after this PR, Comment mangles it to x>).

Thanks for reading this far. The code in question is not great and I appreciate your efforts on it. All of the rules that apply here are complex and I haven't worked directly with XML or HTML encoding in a long time so I don't have ready-made answers - hence the questions. I do have a hunch that this code could be both simpler and closer correct and my reading of the current PR hasn't shown me that this hunch is wrong. It could be wrong and I don't mind if that's the outcome, but I'd like the reasons it is wrong to show up clearly somewhere - either in the PR or in these comments.

[felipemelendez]

I know there's a life lesson in here somewhere.
I think that your hunch is well founded and I am hard pressed to think of reasons why you would be wrong, particularly because the snippet that I shared in my previous comment is working fine. See my first commit that was the path that I intended to take...
What is more, before I made the suggestion to my team on how to solve the MSIE issue I did a lot of research and found that data = data.replace(b"--", b"- - ") was enough, and it has been.
But then I decided to jump in here and try to help address this issue but perhaps got carried away with trying too hard or too much? In retrospect I think that I have been walking on egg shells making sure that we keep in mind every potential user and use case, and weary that I can't possibly test for all of them.

By the way, in regards to the <!-, I can't think of another instance where that would appear within a comment besides this: <!--<![endif]-->, and that line has not been a problem. We have tested our email templates using Litmus (and send them in production to a large number of users), which tests email templates across a wide range of email clients, browsers, and devices. All together it supports over 90 email clients, including popular ones such as Gmail, Yahoo Mail, Outlook, Apple Mail, and more., so I can confidently say that there is not need to touch that. And what is more, that range of coverage is so wide that it serves as a good indicator that we no longer need to escape the > character.

[felipemelendez]

So I shifted my research criteria to history and here is what I found.
The XML 1.0 specification, which was released in February 1998, originally allowed the use of the ">" character within comments, as long as it was not part of the "-->" sequence. However, security concerns were later raised about the potential for malicious code to be injected into an XML document through comments. I believe this type of attack is known as cross-site scripting (XSS).
In the early days of HTML and XML, some web browsers and other software used to interpret them may not have handled the ">" character correctly, causing parsing errors or incorrect interpretation of the comment boundaries. This could potentially lead to security vulnerabilities or other issues if an attacker was able to inject malicious code or unexpected input into a comment.
However, modern web browsers and other software are generally designed to follow the HTML and XML specifications closely and fully support all standard features, including comments with the ">" character. So, while older systems or outdated software may have had issues with this, it is less likely to be a problem with current parsers and more recent technology.

While doing this research I learned that one example of a tool that did not support the ">" character within comments in the past was Internet Explorer 6 and earlier versions – that's the Microsoft's MSXML parser (specifically versions 3.0 and earlier). These older versions of Microsoft's web browser had a bug where they would not properly handle comments that contained the ">" character. This meant that any XML document that contained this character within a comment would not be rendered correctly in Internet Explorer. However, this bug was fixed in later versions of Internet Explorer, and most modern web browsers and XML tools should have no problem handling the ">" character within comments in XML documents. Similarly, and I add this to show that my research has been exhaustive, some older versions of the Apache Xerces C++ parser had a bug where they would not properly handle comments that contained the ">" character, which could cause parsing errors.

Some examples of software that had issues with the > character within comments include:

• Internet Explorer 6 and older versions of the browser
• Netscape Navigator 4 and older versions of the browser
• Some older versions of the libxml2 library, particularly versions prior to 2.7.8

And these are all old versions of these parsers that have since been updated and support the use of > within comments

[felipemelendez]

• The part about the > can be found in the Character Data and Markup section.

On a final note, I now realize that I made a mistake when I referenced this. I figure that I was trying to align with the statements made within test_commentEscaping. Where in fact:
Char ::= #x9 | #xA | #xD | [#x20-#xD7FF] | [#xE000-#xFFFD] | [#x10000-#x10FFFF] Comment ::= '<!--' ((Char - '-') | ('-' (Char - '-')))* '-->'
The definition for Char includes all characters in the range from #x1 to #xD7FF, the range from #xE000 to #xFFFD, and the range from #x10000 to #x10FFFF.
The range [#x1-#xD7FF] includes all Unicode code points from #x0001 to #xD7FF, which means that #x3E (which is a hexadecimal representation of the greater-than character '>') falls within this range.

Sadly, I made this statement when I drafted the issue

[felipemelendez]

Esteemed @exarkun, thank you very much for providing your valuable input and insight on this PR. I have a lot of respect for your work and was hesitant to make changes that could compromise it. Instead of following my initial approach, I decided to work around it. I have now come full circle and made the appropriate changes. I also conducted additional research to support the reasoning behind this change, and I hope it will help inform your final decision. Whatever verdict you reach, I am fully supportive. Best regards.

[glyph]

Just a suggestion, I think it would be helpful to present the production rule as code, rather than flowed in paragraph text:

Suggested change

	BNF (Backus-Naur Form) production rule for comments:
	Comment ::= '<!--' ((Char - '-') | ('-' (Char - '-')))* '-->'
	BNF (Backus-Naur Form) production rule for comments::
	Comment ::= '<!--' ((Char - '-') | ('-' (Char - '-')))* '-->'

[glyph]

This is still mangling, not quoting (it looks like quoting is not possible here) but it is a less legible form of quoting.

What do you think about this, instead?

Suggested change

	data = data.replace(b"--", b"- - ")
	data = data.replace(b"-->", b"-->")

I parsed a -- in a comment with html5lib 1.1 and despite the spec's prohibition on --, this document:

<!DOCTYPE html>
<html>
 <head>
  <title>hi</title>
 </head>
 <body>
  <!-- -- -->
 </body>
</html>

responds to this program:

from html5lib import parse
from xml.etree.ElementTree import tostring

with open("comment.html") as f:
    d = parse(f, namespaceHTMLElements=False)
    c = list(d.iter())[-1]
    print(c.text)

by printing out --, and can happily re-serialize it without any warnings I can detect.

Given that we need to mangle it, it would seem to me that we should mangle as minimally as possible, which appears to be what browsers and reference specs do. For example, that HTML file in a browser looks like this as well:

> document.body.childNodes[1].textContent
< " -- "

Most of all, adding in a space to mangle like this seems really arbitrary.

[felipemelendez]

Awesome @glyph, thank you for your explanation, it was incredibly helpful and improved my understanding. I also appreciate the invocation of Greek mythology to help break the impasse. Alexander would be proud.

Also, although it's a small matter, I followed your lead and conducted similar tests as you and noticed that ending a comment with a dash does not appear to have any effect. Following the ideal to perturb as little as possible, I would be tempted to remove the following code block, but I don’t want to be petty and make any unwarranted changes, so I won't touch it unless directed to do so.

if data and data[-1:] == b"-": 
    data += b" " 

I'll jump on this as soon as I get a chance. Once again I appreciate your input on this.

[glyph]

Oh, that's a great catch. Thank you for pointing it out :-).

[glyph]

(And yes, I agree that adding whitespace to break the connection of the ending - with the closing --> is probably the least intrusive thing I can think of.)

[glyph]

Hopefully I can do a little bit of gordian knot-cutting here by suggesting what the "right" behavior for twisted.web.template is, in general:

First and foremost, any parsed document should round-trip as closely as possible. We do not need to commit to perfect byte-for-byte fidelity; there are some cases (i.e. dealing with certain subtle encoding errors) where we might do something different, but we should perturb as little as possible. The current behavior here violates that constraint and that's the most important thing to fix. Our current behavior of quoting > is wrong, but quoting -- would be equally wrong, and is unnecessary.

Then there's the question of what to do with potentially invalid synthetic documents; i.e. what should we do if we didn't parse something but a user constructed it. We don't want to suddenly start raising exceptions on people constructing nodes with strings that would previously have serialized something correctly, so constructing a comment with a -- in it shouldn't raise an error. The question about what to do here is what specs do we look to in order to figure out how any quoting behavior should be exposed to applications. Is it an HTML processor, an SGML processor, an XML processor or what? If these specs disagree, what's the tiebreaker?

The history of SGML, HTML (3, 4), and XML is interesting, but in practice twisted.web.template is used to serialize HTML documents, usually in an HTML5 context. At the time of its creation, compatibility with XHTML was really pressing, and thus we should try to maintain compatibility with XML (and XHTML in particular) as much as possible, but HTML5 is so dominant, and what we are serializing is HTML here, that when we need a tie broken on what behavior to have, the hierarchy of applicable specs is:

1. if browsers & the reference implementation (html5lib) agree on something, do that (as long as it doesn't result in output that results in quoting bugs that will corrupt the document or potentially create escaping problems with untrusted data (more on that in a moment))
2. if the implementation behavior doesn't agree, try to follow the HTML5 spec
3. if possible, follow any relevant XML specs as well, but HTML5 wins if there's a hard conflict where something just won't work

So, applying those principles in this case:

The current implementation will mangle valid documents it does not need to. That is a bug. If you read > in a comment, which is allowed per most of these specs, then you'll get back > if you reserialize it, which is unnecessary. We should fix that.

Mangling -- instead will mangle invalid documents according to the spec, but the spec is more of a tiebreaker in the case that the implementations don't agree, so what do the implementations do? As I explained above, they actually allow -- just fine.

But then there's that caveat about quoting / injection attacks; i.e. if a document can be made to fail to round-trip through other implementations, we should do something that is at least safe against that. And here, we can see that html5lib, at least, is not safe, when we try to create a synthetic comment that contains --> by doing this to the aforementioned document:

with open("comment.html") as f:
    d = parse(f, namespaceHTMLElements=False)
    c = list(d.iter())[-1]
    c.text = "-->"
    print(c.text)
    print(tostring(d))

and you get this:

b'<html><head>\n  <title>hi</title>\n </head>\n <body>\n  <!---->-->\n \n\n</body></html>'

which is just wrong, because that will produce a document that will be parsed completely differently than the document model that serialized to it. Here's where we still need to do some kind of mangling, because bailing entirely is not acceptable (that leads to an attack where serializing some untrusted data in a comment can blow up your rendering) and quoting is not possible in this context. This will produce something which is still XML, still HTML, and unfortunately is slightly mangled but only in the case that a document contains text that cannot be parsed, thus cannot break the round-trip rule.

[adiroiban]

I think the changes looks much better.

I still have a question regarding docstring, before this can be merged.

Thanks again for all your work on this.

[adiroiban]

Is space still added?

[felipemelendez]

yeah:

if data and data[-1:] == b"-":
        data += b" "

I also questioned this to be certain that it should remain and got the green light.
In this regard Glyph said: "Yes, I agree that adding whitespace to break the connection of the ending - with the closing --> is probably the least intrusive thing I can think of."

[adiroiban]

Suggested change

	To prevent parsing errors and ensure validity when serializing HTML comments, twisted.web.template.flattenString has been updated to escape the --> sequence within comments
	To prevent parsing errors and ensure validity when serializing HTML comments, twisted.web.template.flattenString has been updated to escape the --> sequence within comments.

Just a full top at the end, to make sure that this was not left unfinished.

[felipemelendez]

b754b4a

[felipemelendez]

Gentlemen, I think the fact that my last commit was to add a period is pretty symbolic, if that is the final touch needed to complete this PR that would be metaphorically awesome. But if there is anything else that needs to be done to make sure that this is up to par please let me know. I am thrilled to have the opportunity to contribute to this fantastic project. I realize that my induction has been a little chaotic, but in retrospect the best adventures are those that come with a little drama.

@adiroiban, thank you for making sure that my work was complete and done right.

@exarkun, you jumped into this PR with the right idea and I veered you off course, my apologies. I owe you a cold one for that.

@glyph, I follow your blueprint at work daily, so I was thrilled to see you peek into this PR. I unfortunately missed the chance to work with you at Pilot, but this is a pretty cool consolation prize.

[adiroiban]

All good then. Sorry for the trouble.