1770224 - imContentSink.jsm should NOT use console.error when it detects undesirable markup and filters it. It is not an error. It should WARN the user.

Status: NEW

(Thunderbird :: Instant Messaging, enhancement)

Description

[ISHIKAWA, Chiaki]

I was testing my local patches for TB to enable buffering output and error handling (more emphasis on the error handling on the latter these days after Ben's cleanup of pop3 download code.) on my Debian GNU/Linux PC.

Locally, I run xpcshell tests using "--sequential --verbose" so that I can take a look at detailed warning, etc. dumped by my patched code.

I noticed the following "error" message coming from xpcshell test of
"comm/chat/modules/test/test_filtering.js", and thought it is strange that
my patch should trigger a bug in instant messaging, etc.

 0:02.63 INFO "CONSOLE_MESSAGE: (error) [JavaScript Error: "removing a script tag from a message before display" {file: "resource:///modules/imContentSink.jsm" line: 379}]
cleanupNode@resource:///modules/imContentSink.jsm:379:14
cleanupImMarkup@resource:///modules/imContentSink.jsm:509:14
test_stripScripts@/NEW-SSD/moz-obj-dir/objdir-tb3/_tests/xpcshell/comm/chat/modules/test/test_filtering.js:67:34
test_allModes@/NEW-SSD/moz-obj-dir/objdir-tb3/_tests/xpcshell/comm/chat/modules/test/test_filtering.js:127:3
test_permissiveMode@/NEW-SSD/moz-obj-dir/objdir-tb3/_tests/xpcshell/comm/chat/modules/test/test_filtering.js:276:3
_run_next_test@/NEW-SSD/NREF-COMM-CENTRAL/mozilla/testing/xpcshell/head.js:1800:11
run@/NEW-SSD/NREF-COMM-CENTRAL/mozilla/testing/xpcshell/head.js:812:9
_do_main@/NEW-SSD/NREF-COMM-CENTRAL/mozilla/testing/xpcshell/head.js:240:6
_execute_test@/NEW-SSD/NREF-COMM-CENTRAL/mozilla/testing/xpcshell/head.js:597:5
@-e:1:1
"

It turns out TB without my buffered output patch also produced it.
So it is an error of C-C TB without my patch.
I thought of pointing at other people's test on try-comm-central that has this problem, but then I realized nobody runs xpcshell tests with "--serialize --verbose" on tryserver (correct me here if I am wrong. I am not sure how I can do that.),
and UNLESS the test fails, tryserver infrastructure does not bother to print the log messages that are recorded during an xpcshell test (mind you, test_filtering.js seems to pass. So UNLESS you run this locally with "--serialize --verbose", you won't notice the block of the error message and stacktrace that I saw.)
I think TB counsel ought to have someone run xpcshell test with "--serialize --verbose" every now and then (maybe every month) on try-comm-central and see if anything strange is going on there. But I digress.

Now, the "error" message looks a bit ominous and so I checked the code.
It turns out it is NOT AN ERROR at all IMHO.

TB proactively removes strange undesirable stuff in the received instant message
by removing construct that might behave dangerously, e.g. SCRIPT (such as JS, I suppose) before display. (I wonder if there was a time when JavaScript in received message was executed without question. That would have been bad!)

Anyway, TB was doing exactly what I described.: removing undesirable elements from the received message and the printed out the action taken.

removing a script tag from a message before display

So it is not an error at all IMHO, but rather TB should report the strange situation where somebody is sending undesirable stuff and it has filtered it out.

The code that prints the message is here.
https://searchfox.org/comm-central/source/chat/modules/imContentSink.jsm#379

       if (nodeName in kForbiddenTags) {
          Cu.reportError(
            "removing a " + nodeName + " tag from a message before display"
          );

Cu.reportError should be avoided here because

• it leaves undesirable log in xpshell test like the one I have seen [this confuses would-be patch developer. I was.], and
• it may not be noticed by the user who is using the chat. The user ought to realize that something amiss is happening, and may warn the other party. The other party may be sending something undesirable without knowing it even.

I am not familiar with how IM works. Maybe there is a separate window pane where
error condition such network outage, login password error, etc. is reported and
I believe this message should be printed there.
But I don't know what the proper function to use for that purpose.

Comment 1

[Patrick Cloke [:clokep]]

TB proactively removes strange undesirable stuff in the received instant message
by removing construct that might behave dangerously, e.g. SCRIPT (such as JS, I suppose) before display. (I wonder if there was a time when JavaScript in received message was executed without question. That would have been bad!)

There was not, please do not postulate that there was.

So it is not an error at all IMHO, but rather TB should report the strange situation where somebody is sending undesirable stuff and it has filtered it out.

It isn't really an "error" but it is more of "something sketchy happened", we could probably reduce it to a warning and that would be fine.

it may not be noticed by the user who is using the chat. The user ought to realize that something amiss is happening, and may warn the other party. The other party may be sending something undesirable without knowing it even.

There isn't really any place to show this to the user and it isn't clear what action a user would even take in such a situation, so I'm not sure how that would be helpful.

Comment 2

[ISHIKAWA, Chiaki]

Attachment: Maybe the message can be shown in the window where the redline is drawn.

Hi,

(In reply to Patrick Cloke [:clokep] from comment #1)

TB proactively removes strange undesirable stuff in the received instant message
by removing construct that might behave dangerously, e.g. SCRIPT (such as JS, I suppose) before display. (I wonder if there was a time when JavaScript in received message was executed without question. That would have been bad!)

There was not, please do not postulate that there was.

Good to know this. （I have not used IM myself a lot. So I am not sure how things have been done there.)

So it is not an error at all IMHO, but rather TB should report the strange situation where somebody is sending undesirable stuff and it has filtered it out.

It isn't really an "error" but it is more of "something sketchy happened", we could probably reduce it to a warning and that would be fine.

I agree completely. Warning/Information style notice would be fine.

it may not be noticed by the user who is using the chat. The user ought to realize that something amiss is happening, and may warn the other party. The other party may be sending something undesirable without knowing it even.

There isn't really any place to show this to the user and it isn't clear what action a user would even take in such a situation, so I'm not sure how that would be helpful.

This is tough.
I feel the action taken by TB is almost akin to running an ad blocker or NoScript in Firefox.
We are filtering out unwanted stuff.
An adblocker usually keeps a tally of how many ads sites are blocked, etc. so far, and in this page. We can see it easily.
Or with NoScript, we really get a proactive warning popup for X-site scripting style of JavaScript for example when the filtering action is to be invoked.

I think someone sending a JavaScript snippet in a chat message is a problem and the receiving user ought to be aware, and preferably that party should be warned. (After all, it is an unwanted behavior and that is why TB is filtering out proactively in the first place. There is already a value judgment done by TB here.)
Warning that party is optional, but the user really ought to be aware of the filtering action somehow.

Well, if there is no window to show, why not show it in the window pane where IM shows logged in status, etc.? I am attaching the image of my hardly used IM window of TB (under Windows).
Right now, I must have registered an IM chat site where I don't own an account and I have a warning there.
If the warning regarding the filtering either precedes (?) the message display
or follows the message display, that is good enough.

My take on the UI side of the situation is that
If someone sends a message with JavaScript often, that sure is a red sign, and
either you may want NOT to chat with the party unless absolutely necessary
or warn the party if the party seems ignorant of what goes on.
If the sending party persists with the behavior, so be it. There is nothing a receiving party can do after that point.
At least, the receiving is now aware that there is a sending party with undesirable content which TB filters out (just like adblocker, or NoScript in FF).

Comment 3

[Magnus Melin [:mkmelin]]

As an aside, looks like that function should be replaced by the now standardized https://developer.mozilla.org/en-US/docs/Web/API/Sanitizer/Sanitizer

Comment 4

[ISHIKAWA, Chiaki]

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

As an aside, looks like that function should be replaced by the now standardized https://developer.mozilla.org/en-US/docs/Web/API/Sanitizer/Sanitizer

Interesting.

I have many questions concerning the TB's use of JS interpreter on the user-supplied content (a message received in IM as in this bugzilla, or
an inside an HTML e-mail received from a stranger.)

I have always assumed that TB's ordinary e-mail handling never invoked JavaScript interpreter on the content of even HTML page. (Am I correct here? Maybe the assumption is wrong now because IM module does the cleansing of script nodes in the user content, presumably inside received IM mesasge. This means JS is applied to user content, too?
Or is disabling of JS interpreter against the user content cannot be done in advance off-band by just flipping a variable or something, so to speak, before starting to interpret an HTML content for display, and is this why TB needs to do the cleansing of script nodes on its own in advance before invoking an HTML rendering engine?

I am asking because if the IM module requires cleansing of user content before display, that means IM uses a purely browser-like HTML rendering function for display INCLUDING JS feature. That is why it needs to remove the unwanted tags.
But if so, doesn't ordinary (HTML) display function of TB need such cleansing, too?

Is it possible that I misunderstood here?
I always assumed that ordinary e-mail display does not invoke JS engine on the user content.
But I am a bit hazy now. If IM needs to remove the unwanted tags, doesn't
ordinary (HTML) e-mail content needs such removal, too before display? This is the question now I have.

But I have not heard such removal operation taking place before.
Such an operation would have been a real mess for PGP signing, for example. (Unless, of course, the removal does not modify the e-mail content at all. Only the temporary data for display only will be modified?)

If such cleansing actually already takes place, that shows my ignorance.
But if so, I have a concern. I have not seen any similar warning/error

"removing a " + nodeName + " tag from a message before display"

coming from xpcshell test/ mochitest. We are lacking proper testing for removal operation inside an ordinary HTML mail in that case.

Also, the use of JS on user content (HTML mail or IM message) may have a complication of a patch I am thinking of.
As part of the cleanup operation of debug log mess, I was looking at the issue of
a warning that is produced more than 1200 times during mochitest, namely

 1213: Main Thread] WARNING: '!ClientIsValidPrincipalInfo(mClientInfo.PrincipalInfo())', file /NEW-SSD/NREF-COMM-CENTRAL/mozilla/dom/clients/manager/ClientSource.cpp:183

(the number at the beginning of the line is added by summarizing script. This is produced a local mochitest log of DEBUG version of TB.)

I am still investigating what caused this warning message and now it looks to me this is caused by the setting of nullptr (many times, 120K times last I counted?),
by |SystemPrincipal::GetURI()| to its output argument, whereas
there are other GetURI functions.:
|ExpandedPrincipal::GetURI(nsIURI** aURI)| sets its argument to 32 times.
|ContentPrincipal::GetURI(nsIURI** aURI) sets *aURI to mURI which is not definitely nullptr.
Since |mClientInfo.PrincipalInfo()| is nullptr [I think so] the warning is produced 1213 times is what I think now.

So, I was about to propose that, maybe in TB, we can create a reasonable virtual URI that is not mappable to any real site and
pretend that the content we are dealing with is from that site, or rather that the TB as a client of rendering has only a limited privilege to deal with that kind of remote site (we need to make sure that the URI is not from local site and/or any friendly sites whitelisted if such a whiltelist exists in terms of security handling within mozilla code), and then
use this as a dummy mClientInfo.PrincipalInfo() place holder. Then I hoped we don't need to modify any M-C code and
don't get this warning (from failing ClientIsValidPrincipalInfo(mClientInfo.PrincipalInfo())) any more.
I am led to believe that this warning was pretty much TB specific and not produced that often in FF. This was pointed out when I mentioned about it about a month ago in the developer mailing list.

Now, though, if the ordinary e-mail display window is like IM window and uses full HTML
display feature (including JS) [I am asking if this were the case.], then this virtual URI idea that points to a some non-existing place to shut up noisy warnings from M-C code and makes life easier for TB developers may require a lot of scrutiny. (This and related warnings have been there since the tightening up of I/O streams due to HTML/HTTP origin restriction and quite a nuisance to TB. That was what I have been thinking since I assumed we didn't use JS on user content at all in TB. But if we do, it is a whole different ball game.

Now come to think of it, C-C TB shows a browser-like window that shows the TB's introductory site image on the first use.
Does that mean somehow full browser feature sneaked into TB sometime ago? (I mean the code has been there all along. We link the whole M-C tree already. But I thought we stayed away from full HTML rendering including JS use. But looking at this warning/error from IM module makes me a bit hazy about my assumption.

I know we wanted to show HTML e-mail and for that purpose HTML browser feature is desirable, but
we need to do lots of blacklisting/whitelisting, etc. IF JS interpreter on the user content is not disabled somehow
If JS use on user content is not easily disabled (IM's removal of unwanted tags suggest that this is the case)
removing an unwanted tag might be also necessary for HTML e-mail, I presume.
However, as I mentioned above I have not noticed any

"removing a " + nodeName + " tag from a message before display"

warning/error message for ordinary HTML e-mail message during mochitest/xpcshell test.
Don't we need such tag removal processing or Is JavaScript on user content disabled by a different mechanism for the ordinary e-mail?
(If so, then why does not such a mechanism get used for IM, too, I wonder.)

There are many questions concerning JS use in TB.
An underlying difference from FF may be related to the notion of the origin and its use in
avoiding cross-scripting vulnerability and such. TB as a mail client does not have such a detailed notion of the origin.
Everything we receive is from a foreign land. The "From:" address should not be relied on at all.
Everything should be treated from a total stranger.
I hate to to handle the subtle distinction of the origins as is done in HTML browser
in TB's e-mail handling and non-use of JS on user content is the surest way to avoid that type of brittle handling.

Anyway, I am a bit surprised to think IM uses a full HTML rendering engine that does not seem to have a global JS off switch, so to speak.
Such a switch would have made the code much cleaner for TB IMHO.

Now, I know this is a mouthful asking questions about JS usage on user content in TB.
Where can I re-post this to get wider discussion on this topic. Maildev mailing list?

Of course, JS scripts have been used to build and execute bulk of TB functions.
I recently notice that we can now show PDF with built-in viewer inside TB's tab.
I am attaching the screen dump. This is very neat.
I think the built-in viewer is written in Java (and is used by FF too for internal viewing).
That is a good use of JavaScript.

But the JS script received in an IM message or ordinary (HTML) e-mail message. That is another story.
I think TB has not really made a clean-cut decision on the handling the notion of the origin, etc. applicable in a web browser.
That is why we have