1563452 - When serialising with nsIDocumentEncoder::OutputFormatted, attributes, like href, can be broken across lines, which alters their value (broken links etc.)

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P2)

Description

[Mario Rossi]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:67.0) Gecko/20100101 Firefox/67.0

Steps to reproduce:

1. Open a new html message
2. Add this text "http://www.test.com/pdf/Title of this document.pdf"
3. Select this text
4. Click on "add hyperlink" button
5. Paste the same text ("http://www.test.com/pdf/Title of this document.pdf")
6. Click on OK
7. Send message

Actual results:

Lokk at the message in "sent messages", when you hover the mouse on the link, it was transformed to:
http://www.test.com/pdf/Title%20%20%20%20%20 of this document.pdf
and the link will not work.

Expected results:

Hyperlink should be transformed to:
http://www.test.com/pdf/Title%20of%20this%20document.pdf

Comment 1

[Jorg K (CEST = GMT+2)]

Spaces in links are a constant source of trouble.

The HTML stored is in fact:

    <p><a moz-do-not-send="true" href="http://www.test.com/pdf/Title of
        this document.pdf">XXX<br>
      </a></p>

So the link gets broken over two lines. I don't see any multiple spaces when hovering, but copy link location results in
http://www.test.com/pdf/Title%20of%20%20%20%20%20%20%20%20this%20document.pdf

Masayuki-san, what's your opinion here? Looks like a serialiser bug to me. A href containing spaces should EDIT: NOT be serialised out like this. Or is this a TB editor bug? When we store the href, we should replace the spaces so this can't happen?

Also note bug 1371010 where percent-encoding was remove for hrefs and bug 1371559.

Comment 2

[Jorg K (CEST = GMT+2)]

Attachment: 1563452-spaces-in-links.patch

Something like this?

Comment 3

[Jorg K (CEST = GMT+2)]

Attachment: 1563452-spaces-in-links.patch

Typo :-(

Comment 4

[Jorg K (CEST = GMT+2)]

Masatoshi-san, any comment here since you worked on bug 1371010?

Comment 5

[Masatoshi Kimura [:emk]]

Attachment: Test case

How other browsers behave with this test case? If they percent-encode spaces, this is a web-compatiblity problem and we should fix the serializer. If they do not, we should deal with the problem in our editor.

Comment 6

[Masatoshi Kimura [:emk]]

We should also make sure that the change does not affect contenteditable elements.

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 9076065 [details] [diff] [review]
1563452-spaces-in-links.patch

Yeah, sounds like that solving in the UI code must be the best unless this causes some web-compat issues of Gecko. But perhaps, using XPCOM API must be better to fix this if you add this hack. But be careful, user may input %-encoded UI. So, your hack should run only when input URL is invalid.

Comment 8

[:aceman]

Comment on attachment 9076126 [details]
Test case

On this test case, Chrome does show the link with all the spaces when hovering the link with mouse.
Copying the link copies http://www.test.com/pdf/Title%20of%20%20%20%20%20%20%20%20this%20document.pdf .

Comment 9

[:aceman]

Is it actually legal to break HTML attributes inside the value like this? Could we prevent it for total safety?
I know contents of elements (text) can be line-wrapped freely (expcept <pre>) but then the renderer ignores surplus spaces (except one). Also tag attributes can be line-wrapped between attributes. But inside of attribute value is really strange.

Comment 10

[Jorg K (CEST = GMT+2)]

I agree, that's why I wrote in comment #1: Looks like a serialiser bug to me. A href containing spaces should NOT be serialised out like this. But given that bug 1371010 stopped percent encoding, all we can do it make sure that the link doesn't get broken across lines.

Comment 11

[Masatoshi Kimura [:emk]]

(In reply to :aceman from comment #8)

On this test case, Chrome does show the link with all the spaces when
hovering the link with mouse.
Copying the link copies
http://www.test.com/pdf/Title%20of%20%20%20%20%20%20%20%20this%20document.pdf .

It is exactly the same behavior as us.

(In reply to :aceman from comment #9)

Is it actually legal to break HTML attributes inside the value like this? Could we prevent it for total safety?
I know contents of elements (text) can be line-wrapped freely (expcept <pre>) but then the renderer ignores surplus spaces (except one). Also tag attributes can be line-wrapped between attributes. But inside of attribute value is really strange.

Even if it is not legal, The HTML spec defines the error handling algorithm almost step-by-step, and all browsers follow that. We can't break the spec, interop with other browsers, and web-compat just for Thunderbird, sorry.

(In reply to Jorg K (GMT+2) from comment #10)

I agree, that's why I wrote in comment #1: Looks like a serialiser bug to me. A href containing spaces should NOT be serialised out like this. But given that bug 1371010 stopped percent encoding, all we can do it make sure that the link doesn't get broken across lines.

Could you quote the spec text that we failed to follow (that is, our bug)? I could not even find the word "escape" or "percent-encoding" in the spec.

Comment 12

[:aceman]

(In reply to Masatoshi Kimura [:emk] from comment #11)

Even if it is not legal, The HTML spec defines the error handling algorithm almost step-by-step, and all browsers follow that. We can't break the spec, interop with other browsers, and web-compat just for Thunderbird, sorry.

We don't mean to change the reading of the attr value if it is already stored like that.
But can we somehow achieve that Thunderbird does not save the attr value wrapped like that? Replacing the spaces as in the patch is a way, but would there be a cleaner solution using some m-c API?

Comment 13

[Jorg K (CEST = GMT+2)]

(In reply to Masatoshi Kimura [:emk] from comment #11)

(In reply to Jorg K (GMT+2) from comment #10)

I agree, that's why I wrote in comment #1: Looks like a serialiser bug to me. A href containing spaces should NOT be serialised out like this. But given that bug 1371010 stopped percent encoding, all we can do it make sure that the link doesn't get broken across lines.

Could you quote the spec text that we failed to follow (that is, our bug)? I could not even find the word "escape" or "percent-encoding" in the spec.

Is there a misunderstanding here? I'm not saying that serialising should escape or percent-encode. I am saying that if you pass DOM with a href of "x y z", it is not expected that the serialiser function returns a pretty-printed/formatted result where there is a line break inserted into the tag. That is a bug. If it wants to format, it should break before the tag. We can work around it by not producing tags with spaces, but that would be a work-around.

Note that TB does the serialising around here:
https://searchfox.org/comm-central/search?q=nsIDocumentEncoder%3A%3AOutputFormatted&case=false&regexp=false&path=mailnews%2F

If we didn't requests "formatted" resulting in illegible spaghetti HTML in the e-mail body, then there wouldn't be a problem.

What am I missing?

Comment 14

[Masatoshi Kimura [:emk]]

Ah, thanks for the clarification. I was misunderstanding because bug 1371010 was solely about percent-encoding.

Since nsIDocumentEncoder::OutputFormatted is not exposed to the web, this is indeed our (m-c) bug. nsXHTMLContentSerializer should consider the context on formatting.

Comment 15

[Jorg K (CEST = GMT+2)]

Just as a matter of interest, why doesn't that go to Core::Serialisers? Sadly still unowned, https://wiki.mozilla.org/Modules/Core, very bottom of page.

Comment 16

[Jorg K (CEST = GMT+2)]

Comment on attachment 9076126 [details]
Test case

If I'm not mistaken, this is not a text case but an experiment. A test case would be some addition to TestPlainTextSerializer.cpp that recreated the case were the serialiser inserts a line break into the tag.

Comment 17

[Mirko Brodesser (:mbrodesser-Igalia), offline until 10th of Feb]

@Jork K: Core::Serializers is presumably the correct component. However, I still haven't understood how to reproduce this in Firefox (not Thunderbird). I've seen comment 13 but couldn't produce an example where this fails. A concrete case/STR leading to a failure in Firefox would be helpful.

Comment 18

[Jorg K (CEST = GMT+2)]

It's a problem of the underlying backend libraries. You could write a page which contains a link (anchor) with a href attribute with spaces in it. Then serialise that using some JS using nsIDocumentEncoder with nsIDocumentEncoder::OutputFormatted which is triggered on load. Tweak the HTML of the page until the href is broken at one of the spaces and something like is shown in comment #1 comes out as a result.

I suggested to add some tests to TestPlainTextSerializer.cpp, or looking at this again, maybe that isn't the right test bed for it. But I'm sure you have tests for serialisation somewhere, like
https://searchfox.org/mozilla-central/search?q=nsIDocumentEncoder&case=false&regexp=false&path=.html

Comment 20

[Magnus Melin [:mkmelin]]

Maybe around here: https://searchfox.org/mozilla-central/rev/f16b7b648f2b294225edd0192f0fbb543327bce9/dom/serializers/nsXMLContentSerializer.cpp#671,673
I can't think it's ever allowed to do any line wrapping inside an attribute value.

Comment 21

[José M. Muñoz]

Attachment: 1563452.patch

With the change Magnus suggested in comment #20 and with the suggestion for a gTest from comment #18 I took a copy of
https://searchfox.org/mozilla-central/source/dom/base/test/gtest/TestParser.cpp
and modified it slightly to show the problem. Without the changes in nsXMLContentSerializer.cpp, the gTest fails, with the changes it succeeds.

To run the gTest, simply type this into a command prompt:
mach gtest TestSerializerNoBreakLink*
(with the * at the end).

Magnus, can you take it from here? This would need a try run on M-C, agreement that this is the right way to go, review, etc.

Comment 22

[Magnus Melin [:mkmelin]]

Successful try: https://treeherder.mozilla.org/jobs?repo=try&revision=b938afdbc51b318e7ca7b916c3fd1197826424b3&selectedTaskRun=QtMm3e3oRBuOxMcWYUgt1g.0

Comment 23

[José M. Muñoz]

Comment on attachment 9222976 [details] [diff] [review]
1563452.patch

Sadly Magnus hasn't moved this forward.

Is this the correct way forward? I can't f?hsivonen, hence the NI?

Comment 24

[Henri Sivonen (:hsivonen)]

Comment on attachment 9222976 [details] [diff] [review]
1563452.patch

Review of attachment 9222976 [details] [diff] [review]:
-----------------------------------------------------------------

The fix looks correct to me, but I'll let Mirko confirm.

Comment 25

[Mirko Brodesser (:mbrodesser-Igalia), offline until 10th of Feb]

I'm not very familiar with percent-encoding and the specific code which is touched in this review. However, I'm surprised Firefox allows one space character, and doesn't percent-encode that one. Only the surplus space characters are percent-encoded as "%20". According to https://en.wikipedia.org/wiki/Percent-encoding#Types_of_URI_characters no space characters should be allowed.

Hence, the expectation of the reporter isn't met with the patch of #c23.

Chrome percent-encodes all space characters.

Firefox' behavior might be intentional, because URLs stay more human-readable. That's to be clarified. Does someone know? Only after answering that, it makes sense to continue evaluating the correctness of that patch.

Comment 26

[Henri Sivonen (:hsivonen)]

(In reply to Mirko Brodesser (:mbrodesser) from comment #25)

Firefox' behavior might be intentional, because URLs stay more human-readable. That's to be clarified. Does someone know? Only after answering that, it makes sense to continue evaluating the correctness of that patch.

I believe this in an artifact of the serializer by default retaining the in-DOM attribute value string unless rewriting relative URLs is requested. I'm not sure if absolute URLs go through URL-level reserialization even when rewriting of relative URLs is requested.

Comment 27

[Mirko Brodesser (:mbrodesser-Igalia), offline until 10th of Feb]

Is it an option for Thunderbird to additonally use OutputRaw? That would avoid wrapping, too.

Comment 28

[José M. Muñoz]

(In reply to Mirko Brodesser (:mbrodesser) from comment #27)

Is it an option for Thunderbird to additonally use OutputRaw? That would avoid wrapping, too.

No. The idea is to send HTML in human-readable formatted form, see bug 1707360 comment #6. The more general question is whether it is permissible to introduce extra white-space and even line breaks when serializing attribute values, see Magnus' comment #20.

Comment 29

[Mirko Brodesser (:mbrodesser-Igalia), offline until 10th of Feb]

One has to be careful to not break existing behavior. OutputFormatted is used at https://searchfox.org/mozilla-central/search?q=OutputFormatted&case=true&regexp=true&path=, which contains two relevant cases:

• https://searchfox.org/mozilla-central/rev/36904ac58d2528fc59f640db57cc9429103368d3/editor/libeditor/HTMLEditorDataTransfer.cpp#2978, which is only used by Thunderbird.

• https://searchfox.org/mozilla-central/rev/36904ac58d2528fc59f640db57cc9429103368d3/dom/webbrowserpersist/WebBrowserPersistLocalDocument.cpp#1204, to which ENCODE_FLAGS_FORMATTED maps, see https://searchfox.org/mozilla-central/search?q=ENCODE_FLAGS_FORMATTED&redirect=false. It's only used by https://searchfox.org/mozilla-central/rev/36904ac58d2528fc59f640db57cc9429103368d3/toolkit/content/contentAreaUtils.js#480, for which one would have to check all callers.

The patch proposed in #c23 would affect all attributes and I'm wondering if there could be cases, where readability for humans matters, but additional space characters not. For instance, if an attribute is used to store a long list of values "value1 value2 ... value100" that line would become long.

Comment 30

[Magnus Melin [:mkmelin]]

I'd assume there are such, rare, cases where readability is "worse". However, those cases are likely mostly about debugging, and then it's essential for people to actually have the technically correct value of the attribute. You can't debug if you can't trust what's on the screen.

Hence, the expectation of the reporter isn't met with the patch of #c23.

I would assume the reporter doesn't care about the actual percent encoding. He just want the link not to turn invalid due to the wrong handling of the attribute, breaking it up and adding spaces where it should not. In that regard the patch would fix the use case.

Comment 31

[Mirko Brodesser (:mbrodesser-Igalia), offline until 10th of Feb]

(In reply to Magnus Melin [:mkmelin] from comment #30)

I'd assume there are such, rare, cases where readability is "worse". However, those cases are likely mostly about debugging, and then it's essential for people to actually have the technically correct value of the attribute. You can't debug if you can't trust what's on the screen.

Hence, the expectation of the reporter isn't met with the patch of #c23.

I would assume the reporter doesn't care about the actual percent encoding. He just want the link not to turn invalid due to the wrong handling of the attribute, breaking it up and adding spaces where it should not. In that regard the patch would fix the use case.

Agree with the latter.

I'm considering to have the patch adapted to only affect "href" attributes.

Comment 32

[Magnus Melin [:mkmelin]]

There are also many other attributes that can have urls. Most notably "src".

Comment 33

[Mirko Brodesser (:mbrodesser-Igalia), offline until 10th of Feb]

(In reply to Magnus Melin [:mkmelin] from comment #32)

There are also many other attributes that can have urls. Most notably "src".

Thanks for pointing this out. Some of them can be found from the references of https://html.spec.whatwg.org/multipage/urls-and-fetching.html#valid-non-empty-url-potentially-surrounded-by-spaces.

Comment 34

[Henri Sivonen (:hsivonen)]

Also, introducing line feeds in the title attribute changes rendering. I think it would be good not to add breaks inside any attribute values.

Comment 35

[Mirko Brodesser (:mbrodesser-Igalia), offline until 10th of Feb]

(In reply to Henri Sivonen (:hsivonen) from comment #34)

Also, introducing line feeds in the title attribute changes rendering.

Thanks for pointing this out.

I think it would be good not to add breaks inside any attribute values.

I'm not sure. With the current patch, the semantics of OutputWrap would change, too. It's used at multiple places, see https://searchfox.org/mozilla-central/search?q=%5CbOutputWrap%5Cb&path=&case=true&regexp=true. If we accept that patch, OutputWrap should be renamed to reflect attribute values are not wrapped, e.g. OutputWrapNonAttributeValues.

Comment 36

[Mirko Brodesser (:mbrodesser-Igalia), offline until 10th of Feb]

Comment on attachment 9222976 [details] [diff] [review]
1563452.patch

Review of attachment 9222976 [details] [diff] [review]:
-----------------------------------------------------------------

José: if possible, could you please incorporate the desired changes and create a Phabricator review for this?

::: dom/base/test/gtest/TestSerializerNoBreakLink.cpp
@@ +1,1 @@
> +/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */

Please apply Mozilla's C++ coding style, https://firefox-source-docs.mozilla.org/code-quality/coding-style/coding_style_cpp.html#c-coding-style, to this file, `./mach clang-format` could help.

@@ +1,5 @@
> +/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +

Please check the order of the includes.

@@ +12,5 @@
> +#include "mozilla/ErrorResult.h"
> +
> +// This is a test for serialising some DOM with a link
> +// whose href contains a space.
> +

Please remove the line below.

@@ +20,5 @@
> +  bool allTestsPassed = false;
> +  constexpr auto htmlInput =
> +      u"<html><head>"
> +      "<meta http-equiv=\"content-type\" content=\"text/html; charset=\">"
> +      "</head><body>Hello Thunderbird! <a href=\"http:www.example.com/"

Please make the test more robust by, checking a really long line. E.g. "link with space 1 2 ... 125".

@@ +24,5 @@
> +      "</head><body>Hello Thunderbird! <a href=\"http:www.example.com/"
> +      "link with space.txt\">Link</a></body></html>"_ns;
> +
> +  do {
> +    // Parse the HTML source.

`using namespace mozilla;`
`using namepsace mozilla::dom;`

would be OK for this .cpp file.

Please add a separate scope for `rv2` and define `nsCOMPtr<mozilla::dom::Document> document` above it, so that it can be used outside of `rv2`'s scope below.

@@ +34,5 @@
> +        htmlInput, mozilla::dom::SupportedType::Text_html, rv2);
> +    if (rv2.Failed()) break;
> +
> +    // Serialize it back to HTML source again.
> +    nsCOMPtr<nsIDocumentEncoder> encoder =

Here we only test the "text/html" serializer, even though the patch affects other serializers too.
Please either test all affected serializers, or rename the test file accordingly, e.g. "TestHtmlSerializer...".

@@ +36,5 @@
> +
> +    // Serialize it back to HTML source again.
> +    nsCOMPtr<nsIDocumentEncoder> encoder =
> +        do_createDocumentEncoder("text/html");
> +    if (!encoder) break;

Could "text/html"_ns be bound to a variable and be reused above?

@@ +37,5 @@
> +    // Serialize it back to HTML source again.
> +    nsCOMPtr<nsIDocumentEncoder> encoder =
> +        do_createDocumentEncoder("text/html");
> +    if (!encoder) break;
> +    nsresult rv = encoder->Init(document, u"text/html"_ns,

Please also test this for `OutputWrap`.

@@ +43,5 @@
> +    if (NS_FAILED(rv)) break;
> +    nsString parsed;
> +    rv = encoder->EncodeToString(parsed);
> +    if (NS_FAILED(rv)) break;
> +

`EXPECT_NE(...)` would be more concise.

::: dom/serializers/nsXMLContentSerializer.cpp
@@ +664,5 @@
>      }
>      NS_ENSURE_TRUE(attrString.Append(sValue, mozilla::fallible), false);
>      NS_ENSURE_TRUE(attrString.Append(cDelimiter, mozilla::fallible), false);
>    }
> +  NS_ENSURE_TRUE(AppendToStringConvertLF(attrString, aStr), false);

Please update the documentation of `OutputWrap` and perhaps `OutputFormatted` in the .idl file, too. See https://bugzilla.mozilla.org/show_bug.cgi?id=1563452#c35.

Comment 37

[Mirko Brodesser (:mbrodesser-Igalia), offline until 10th of Feb]

In general I agree with the fix, it just needs some polishing as requested in the review.

Comment 38

[José M. Muñoz]

Thanks for the comments, Mirko. I don't intend to work on this any further. Hopefully someone from the TB team can pick this up. It was after all Magnus's idea in the first place.

Comment 39

[Mirko Brodesser (:mbrodesser-Igalia), offline until 10th of Feb]

(In reply to José M. Muñoz from comment #38)

Thanks for the comments, Mirko. I don't intend to work on this any further. Hopefully someone from the TB team can pick this up. It was after all Magnus's idea in the first place.

Thanks for your help so far, much appreciated :-)

Comment 41

[Thomas D. (:thomas8)]

For Thunderbird, broken links in messages are a serious fallout.
I've sent a ping to tb-devs on Matrix today if someone can finish this off.

Comment 42

[Chris Graham]

Isn't it expected that as messages are bumped around between SMTP servers that they may be wordwrapped in different ways? So unless we are base64-encoding the HTML, we have to expect that line breaks could be placed at any whitespace position. HTML parsers are robust to tolerate this, and URLs are defined to not allow white-space for this kind of reason.

It therefore seems to me that the correct fix is to percent-encode URLs that are entered.
Or at least, that should be done as well as any other fix.

From a user point of view, they won't know anymore that they need to percent-encode URLs for them to be valid as web browser address bars remove the percent-encoding when viewing the URL in the address bars to make the URLs look nicer,

Comment 43

[Thomas D. (:thomas8)]

Ryan, for users of Thunderbird, this bug can make plain vanilla links like http://www.test.com/pdf/Title of this document.pdf get broken in outgoing messages (depending on the link length or position in the text). This is caused by HML source wrapping/indenting adding extra spaces inside the href attribute (link URL), which are then considered part of the URL, making the link disfunctional (mailnews bug 1707360).

See also my bug 1707360 comment 1 with alternative STR and analysis.
Broken links sounds like something which we should try to avoid also wrt enterprise usage.

The fix isn't exactly trivial, but I understand from comment 36 that the WIP patch in this bug has been thoroughly discussed and would just need a couple of changes to be accepted and landed (comment 36 and some of the previous comments iiuc). What do you think?

Comment 44

[Magnus Melin [:mkmelin]]

Ben, any cycles to get this over the finishing line?

Comment 45

[Ben Campbell]

Attachment: Bug 1563452 - Don't break attributes when serializing with nsXMLContentSerializer. r=mbrodesser

Comment 46

[Ben Campbell]

I can't pretend that I really grasp this area, so I'm just going by the patch itself and Mirko's feedback in comment #36.
Mirko seems to be away for another week or so - I've left it with no reviewer for now, but maybe someone else would like to jump in? (or even just point out the obvious bits I screwed up ;- )

Comment 47

[Ryan Lee Sipes]

Gotcha, eager to see this addressed. Thanks for pushing this forward Magnus and Ben.

Comment 48

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last months and this bug has priority 'P2'/severity 'S2'.
:hsinyi, could you have a look please?
For more information, please visit auto_nag documentation.

Comment 49

[Suhaib Mujahid [:suhaib]]

Sorry, there was a problem with the detection of inactive users. I'm reverting the change.

Comment 50

[Ben Campbell]

Thanks! I got sidetracked with other stuff, but I'm still working on this one. Just a unit test hiccup or two to nail down and we're done.

Comment 51

[BugBot [:suhaib / :marco/ :calixte]]

There's a r+ patch which didn't land and no activity in this bug for 2 weeks.
:benc, could you have a look please?
If you still have some work to do, you can add an action "Plan Changes" in Phabricator.
For more information, please visit auto_nag documentation.

Comment 52

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/1bfcdfd1502b