1646611 - Stop using raw HTML strings in chat (chat broken)

Status: VERIFIED FIXED

(Chat Core :: General, defect, P1)

Description

[Geoff Lankow (:darktrojan)]

I'm not sure if it's because of bug 1646483 or it just turned up at the same time, but code such as this which modifies HTML strings and adds them to documents is no longer working ("the operation is insecure"). I did a quick replacement of this function using DOM methods, which worked, but I realised there's much more, so I didn't land that.

Chat is currently broken by this.

Comment 1

[Patrick Cloke [:clokep]]

Geoff -- just to confirm, this affects > 79, correct?

Comment 2

[Geoff Lankow (:darktrojan)]

Correct.

Comment 4

[Patrick Cloke [:clokep]]

(In reply to Geoff Lankow (:darktrojan) from comment #0)

I'm not sure if it's because of bug 1646483 or it just turned up at the same time, but code such as this which modifies HTML strings and adds them to documents is no longer working ("the operation is insecure"). I did a quick replacement of this function using DOM methods, which worked, but I realised there's much more, so I didn't land that.

Would it be possible to get this patch or do we know if something can be backed out to get this to work again? I wanted to work on some chat stuff and this is interfering unfortunately! I might just throw some log statements in that code and read it on the console too...

Comment 5

[Patrick Cloke [:clokep]]

Florian -- curious if you've seen anything that might be causing this? Bug 1646483 doesn't quite feel right to me, but it might be the cause!

Comment 6

[Geoff Lankow (:darktrojan)]

Attachment: 1646611-raw-html-1.diff

Here's what I did at the time. As you can see I didn't get far, and I don't recall what the last piece is about.

Comment 7

[Wayne Mery (:wsmwk)]

Florian, Lasana,
This is now a blocker for 79

Comment 8

[Lasana Murray]

I'm looking into it but I'm still learning what does what (using 68 to figure some of it out). If anyone more experienced has a quick fix I'll be happy to submit a patch. I don't get the "the operation is insecure" message and I see some of the initial HTML in the document so I have been digging further to see what's going on.

Comment 9

[Florian Quèze [:florian]]

I'm unlikely to have time to fix this soon, but if you have questions about what the existing code does or was trying to do, I can likely answer them.

Comment 10

[Lasana Murray]

Ok this is what I have learned about this so far:

I was able to see the error message after some try catch fiddling and a debug build. There was an if clause in a progress listener in conversations-browser as well that bails out before we run into the failure.

The source of the failure seems to be here:
https://searchfox.org/comm-central/source/mozilla/dom/security/DOMSecurityMonitor.cpp#101

This might be a duplicate of bug 1636388.

Basically APIs like innerHTML etc are not allowed in privileged (chrome) code with a few exceptions for specific files. It would take me a while to figure out all the various functions operating on raw HTML and convert them to DOM (not to mention possibly break things) so I opted instead for a simpler path of using iframes like here:
https://developer.mozilla.org/en-US/docs/Archive/Add-ons/Displaying_web_content_in_an_extension_without_security_issues

The only thing with this approach is that I can't access the iframe body content until a load event is fired, possibly because of bug 543435. So I had to make a few functions async. Currently I don't have any catch at their call sites, advice on handling potential errors in a user friendly manner is welcome!

I'm attaching a patch for feedback before I go any further.

Comment 11

[Lasana Murray]

Attachment: bug1646611-04-preview.patch

With this irc seems to work again.

Comment 12

[Lasana Murray]

Attachment: bug1646611-05-part1.patch

There is another use of Range#createContextualFragment to update and possibly other uses of innerHTML. I'll do other patches for them. This one at least makes chat usable again.

The functions I made async mostly get called in event handler style code or as a side effect as with MozConversationBrowser#isActive so they should not cause any immediate problems.

Comment 13

[Wayne Mery (:wsmwk)]

We are proceeding to make 79.0b1 live with chat broken. So it will be urgent to quickly pick up the fixing patch in b2.

Comment 14

[Patrick Cloke [:clokep]]

A couple of comments / questions I see:

• This seems to add an iframe per message, I would be surprised if this worked properly with all of themes. Have you tested the bubbles theme, for example? Are we concerned about the performance impact of all the additional nodes?
• I think Florian also asked a reasonable question which was not answered:

  what's the cause of the problem exactly? I was wondering if there might be a way to create a document with lower privileges so that the security code doesn't care if we are using 'unsafe' methods there.

Link to previous conversation about this: https://matrix.to/#/!fubIsJzeAcCcjYTQvm:mozilla.org/$SzRWsaXQvaV8xQhpxGZvX-PBTzgi6ylYsFCJy2BLhd8?via=mozilla.org&via=matrix.org&via=privacytools.io

I think the general approach that this uses DOM methods is OK, it is really the iframe piece I'm unsure about.

Comment 15

[Florian Quèze [:florian]]

Comment on attachment 9163314 [details] [diff] [review]
bug1646611-05-part1.patch

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

::: chat/content/conversation-browser.js
@@ +68,1 @@
>            if (this.currentURI.spec != "chrome://chat/content/conv.html") {

Loading the empty conv.html file as chrome:// was a hack for something, but I don't remember what. Maybe it was so that loading other chrome:// files (eg. the css files of the message theme) was allowed.

This might be what you need to revisit. You may need to create a custom protocol handler that's not privileged, but accesses the files in the folder of the message theme. We already have a custom protocol handler for emoticons at https://searchfox.org/comm-central/source/chat/components/src/smileProtocolHandler.jsm, it may help you as an example to get started if we decide this is the way to go.

::: chat/modules/imThemes.jsm
@@ +1198,5 @@
> + * @param {string} html  - The target HTML to be parsed.
> + *
> + * @returns {DocumentFragment}
> + */
> +async function getDocumentFragmentFromHTML(doc, html) {

This feels like a hack, and I don't know what the performance of importing nodes from a document to another will be, but if we really want to do it this way, the iframe should be created when creating the conversation document, and it should be reused for all message insertions, so that you don't need to make this function async.

Comment 17

[Lasana Murray]

Attachment: bug1646611-05-part2.patch

Here is an additional but broken patch.
I reverted insertHTMLMessage to be sync and added a custom protocol handler to load the conv.html called conv://. This still runs into the innerHTML() in privileged code error.

I assume it's because I still use a chrome URL to actually load the file but I'm unsure. Also not sure how else the file can be loaded, maybe via a file URL?

Comment 19

[Patrick Cloke [:clokep]]

Were you able to test whether MozChatConversation class should have a type="content" assigned (as https://developer.mozilla.org/en-US/docs/Archive/Add-ons/Displaying_web_content_in_an_extension_without_security_issues suggests)?

It might be worth reaching out to someone on the DOM team to see if they can help us solve this?

Comment 20

[Wayne Mery (:wsmwk)]

So perhaps visiting bug 1644943

Comment 21

[Lasana Murray]

Were you able to test whether MozChatConversation class should have a type="content"

That does not appear to make a difference. The MozConversationBrowser where the messages gets displayed has a type="content" attribute in any case and does not appear to resolve this.

I spent some hours looking at the custom protocol handler approach and I don't think it would work here. In order to load the conv.html I need to set some security flags and I could not figure out a way to do that from nsIWebnavigation.loadURI.

Comment 22

[Lasana Murray]

Attachment: use-nsIParserUtils.patch

In this patch, I kept the getDocumentFragmentFromHTML function but I'm providing it without using iframes. I use the nsIParserUtils api so no need to make anything async here.

Comment 24

[Magnus Melin [:mkmelin]]

Comment on attachment 9166615 [details] [diff] [review]
use-nsIParserUtils.patch

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

Looks pretty good. But can you provide an unbitrotted version as this doesn't apply? Include all you need in the same patch (Geoff's patch is obsolete?)

Comment 25

[Lasana Murray]

Attachment: use-nsIParserUtils-updated.patch

Updated after some changes landed that affect imThemes.jsm.
Yes Geoff's patch can be obsoleted now.

Comment 26

[Magnus Melin [:mkmelin]]

Comment on attachment 9167743 [details] [diff] [review]
use-nsIParserUtils-updated.patch

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

Looks good, r=mkmelin

Comment 27

[Patrick Cloke [:clokep]]

Comment on attachment 9167743 [details] [diff] [review]
use-nsIParserUtils-updated.patch

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

::: chat/modules/imThemes.jsm
@@ +732,3 @@
>  
>    // We insert the whole content of body: chat div, footer
> +  html += '<div id="Chat" aria-live="polite"></div>';

Minor comment, but can we declare html here instead of expanding it? I don't think it used above this point anymore?

Comment 28

[Patrick Cloke [:clokep]]

Also, thanks for figuring this out! :)

Comment 29

[Lasana Murray]

Attachment: move-html-var.patch

Additional patch for html variable.

Comment 30

[Patrick Cloke [:clokep]]

Comment on attachment 9167960 [details] [diff] [review]
move-html-var.patch

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

Looks good. Thank you so much!

We should probably squash these when they get landed.

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/c9d5f6ed6aaa
Use nsIParserUtils instead of innerHTML for chat messages. r=mkmelin

Comment 32

[Patrick Cloke [:clokep]]

Comment on attachment 9167743 [details] [diff] [review]
use-nsIParserUtils-updated.patch

[Approval Request Comment]
Regression caused by (bug #): We're not 100%, maybe an m-c change?
User impact if declined: chat won't work
Testing completed (on c-c, etc.): I've tested it on local builds.
Risk to taking this patch (and alternatives if risky): I don't think we can make things worse than they are?

Comment 33

[Wayne Mery (:wsmwk)]

Comment on attachment 9167743 [details] [diff] [review]
use-nsIParserUtils-updated.patch

[Triage Comment]
Approved for beta
Yay

Comment 34

[Rob Lemley [:rjl]]

Thunderbird 80.0b2:
https://hg.mozilla.org/releases/comm-beta/rev/97990ee27996

Comment 35

[WaltS48 [:walts48]]

Chat (IRC) is working in my testing of the 80.0b2 release candidate on Ubuntu 18.04.4.

Comment 36

[WaltS48 [:walts48]]

However after a few days of using it I went to check something in a previous conversation and the chat area was blank.
Caused by the fix to this bug?

Comment 37

[Lasana Murray]

Not sure. All this patch really does is replace direct innerHTML and createContextualFragment use with calls to the nsIParserUtils api. Maybe somewhere else we are running into the security exception? I was using a debug build to track them down.

Comment 38

[Patrick Cloke [:clokep]]

(In reply to WaltS48 [:walts48] from comment #36)

However after a few days of using it I went to check something in a previous conversation and the chat area was blank.
Caused by the fix to this bug?

It looks like logs aren't showing up properly. Can you (WaltS48 or Lasana) file a follow-up issue to this? Thank you!

Comment 39

[Lasana Murray]

Regarding the issue of what caused the problem in the first place, as far as I could tell anything loaded via a chrome:// scheme is going to be treated as privileged code by the security mechanism and thus not allowed to use innerHTML etc. That is to say; as far as I can tell we are not allowed to load chrome code and treat it as content. That's the impression I got by looking at some of the nsISecurityPrincipal and nsILoadInfo code. I could not find a way to properly change the loadingPrincipal when I tried making the custom protocol.