x/net: regression in x/net/html package: The <!--[if mso]--> email HTML comments are now being escaped

[VojtechVitek]

Hi,

We're rendering HTML emails in Go. We're using github.com/aymerick/douceur to do CSS inlining / HTML sanitization, which in turn uses golang.org/x/net/html package under the hood.

Recently, after upgrading x/net package, we have noticed that our HTML emails broke in certain email clients, ie. Windows / Microsoft Outlook, and rendered as a blank page.

We're using some Outlook-specific conditional comments in the HTML of our emails:

Example:

<!--[if gte mso 12]>
  <xml>
      <o:OfficeDocumentSettings>
      <o:AllowPNG/>
      <o:PixelsPerInch>96</o:PixelsPerInch>
      </o:OfficeDocumentSettings>
    </xml>
<![endif]-->

However, after this commit (#48237), the content of the HTML comments changed and is now being escaped:

<!--[if gte mso 12]&gt;
  &lt;xml&gt;
      &lt;o:OfficeDocumentSettings&gt;
      &lt;o:AllowPNG/&gt;
      &lt;o:PixelsPerInch&gt;96&lt;/o:PixelsPerInch&gt;
      &lt;/o:OfficeDocumentSettings&gt;
    &lt;/xml&gt;
&lt;![endif]-->

Emails are hard and it's quite normal to rely on these special <!--[if mso]> HTML comments to support Microsoft Outlook properly. See Targeting specific Outlook versions.

Questions:

1. Is there any chance we could revert this change?
2. Is there any chance we could provide a setting to turn off HTML comment escaping?
3. Or should we consider this a breaking change "by design" and try to work around the issue (ie. by forking x/net/html)?

Thank you for your input.

Best,
Vojtech

[mknyszek]

CC @nigeltao

[mknyszek]

CC @namusyaka

[antichris]

It seems logical that only the --> sequence should be escaped in the Data of a CommentToken. But escape() does not look at sequences, it just escapes every rune that's in escapedChars. Maybe a separate escapeCommentData() could be implemented?

[nigeltao]

I think it's unlikely that we'll (1) revert or (2) provide a setting. The behavior was changed because it was a bug that an "html.Parse and html.Render" round trip could change the semantics of the HTML. There was a potential "comment injection" security issue where a parse+render of malformed comment could inject non-comment content.

If you need to inject meaningful comments, and want control over escaping, perhaps an html.RawNode would help.

However, I will quote myself from another issue:

I'm not keen on using golang.org/x/net/html for server-side rendering. Use the standard library's html/template instead. Yes, the former has nodes and the latter is text based, but the latter is explicitly designed to render HTML securely.

[nigeltao]

Having said that...

Maybe a separate escapeCommentData() could be implemented?

This could possibly work. We'd have to think carefully about any security implications, though. For example, there are non-standard ways to create a CommentToken that stops at the first ">", not the first "-->".

[nigeltao]

Here's what the HTML spec says about parsing comments:

• https://html.spec.whatwg.org/#comments (section "13.1.6 Comments")
• https://html.spec.whatwg.org/#comment-start-state (sections "13.2.5.43 Comment start state" through to "13.2.5.52 Comment end bang state")

Given that second link... I don't know about you, but I'm going to need to think for a bit about when it's safe to not-escape > characters.

[antichris]

It does look ominous at the first glance, but I'm pretty sure that the gist of it is that

• when you're in the <!-- state (13.2.5.43), reads that turn it into <!--> or <!---> (transitioning from 13.2.5.43 and 13.2.5.44, respectively), end the comment with abrupt-closing-of-empty-comment parse error.
• When in comment state (13.2.5.45), reading
  • <!--> simply ends the comment (after 13.2.5.49); the <! content is retained in comment token's data.
  • --!--> also ends the comment (after transitioning from 13.2.5.52 through 13.2.5.50 and 13.2.5.51); --! is retained in comment token's data.
  • --!> ends the comment with incorrectly-closed-comment parse error (13.2.5.52).
    This is probably another instance where > in the data of a comment token should be escaped.

Anything else between <!-- and --> gets appended to comment token's data.

That looks about it. Or am I missing some fine point?

[gopherbot]

Change https://go.dev/cl/466776 mentions this issue: html: parse comments per HTML spec

[gopherbot]

Change https://go.dev/cl/469055 mentions this issue: html: add "Microsoft Outlook comment" tests

[gopherbot]

Change https://go.dev/cl/469056 mentions this issue: html: have Render escape comments less often