Closed Bug 1675589 Opened 4 years ago Closed 1 year ago

Creating websocket throws in reply compose windows

Categories

(Thunderbird :: Add-Ons: Extensions API, defect)

Product:
Thunderbird
Component:
Add-Ons: Extensions API
Type:
defect

Tracking

(Not tracked)

Status:
RESOLVED WONTFIX

People

(Reporter: code, Assigned: code)

References

(
URL
)

Details

Attachments

(2 files)

thunderbird_reproducer.tar
10.00 KB, application/x-tar
Details
fix-extension-csp.patch
828 bytes, patch
Details | Diff | Splinter Review

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:82.0) Gecko/20100101 Firefox/82.0

Steps to reproduce:

Loaded my reproducer and tried to reply to an email.

Actual results:

An extension was thrown and the message "after websocket" was not printed in the addon debugging console.

Expected results:

The message "after websocket" should have been printed, like it happens when creating a new message instead of replying to an existing message.

Woops, just noticed a typo: s/An extension was thrown/An exception was thrown/.

Here are the logs I get when running a debug build from the console and just clicking on "write":

[70772, Main Thread] WARNING: HTMLEditor::GetAbsolutelyPositionedSelectionContainer() reached <html> element: file /home/me/prog/mozilla-central/editor/libeditor/HTMLAbsPositionEditor.cpp:94
[70772, Main Thread] WARNING: HTMLEditor::GetAbsolutelyPositionedSelectionContainer() reached <html> element: file /home/me/prog/mozilla-central/editor/libeditor/HTMLAbsPositionEditor.cpp:94
[70772, Main Thread] WARNING: HTMLEditor::GetAbsolutelyPositionedSelectionContainer() reached <html> element: file /home/me/prog/mozilla-central/editor/libeditor/HTMLAbsPositionEditor.cpp:94
[70772, Main Thread] WARNING: '!aNode', file /home/me/prog/mozilla-central/editor/libeditor/EditorBase.cpp:3030
[70772, Main Thread] WARNING: NS_ENSURE_TRUE(name) failed: file /home/me/prog/mozilla-central/dom/base/nsDOMAttributeMap.cpp:306
[70772, Main Thread] WARNING: HTMLEditor::GetAbsolutelyPositionedSelectionContainer() reached <html> element: file /home/me/prog/mozilla-central/editor/libeditor/HTMLAbsPositionEditor.cpp:94
[70772, Main Thread] WARNING: HTMLEditor::GetAbsolutelyPositionedSelectionContainer() reached <html> element: file /home/me/prog/mozilla-central/editor/libeditor/HTMLAbsPositionEditor.cpp:94
[70772, Main Thread] WARNING: HTMLEditor::GetAbsolutelyPositionedSelectionContainer() reached <html> element: file /home/me/prog/mozilla-central/editor/libeditor/HTMLAbsPositionEditor.cpp:94
[70772, Main Thread] WARNING: NS_ENSURE_TRUE(frame) failed: file /home/me/prog/mozilla-central/dom/events/ContentEventHandler.cpp:1300
[70772, Main Thread] WARNING: NS_ENSURE_SUCCESS_VOID(rv) failed with result 0x80004001 (NS_ERROR_NOT_IMPLEMENTED): file /home/me/prog/mozilla-central/caps/OriginAttributes.cpp:114
[70772, Main Thread] WARNING: Failed to retarget HTML data delivery to the parser thread.: file /home/me/prog/mozilla-central/parser/html/nsHtml5StreamParser.cpp:1132
JavaScript error: , line 0: uncaught exception: Object
[70772, Main Thread] WARNING: NS_ENSURE_SUCCESS_VOID(rv) failed with result 0x80004001 (NS_ERROR_NOT_IMPLEMENTED): file /home/me/prog/mozilla-central/caps/OriginAttributes.cpp:114
[70772, Main Thread] WARNING: NS_ENSURE_SUCCESS_VOID(rv) failed with result 0x80004001 (NS_ERROR_NOT_IMPLEMENTED): file /home/me/prog/mozilla-central/caps/OriginAttributes.cpp:114
[70772, Main Thread] WARNING: NS_ENSURE_SUCCESS_VOID(rv) failed with result 0x80004001 (NS_ERROR_NOT_IMPLEMENTED): file /home/me/prog/mozilla-central/caps/OriginAttributes.cpp:114
[70772, Main Thread] WARNING: NS_ENSURE_SUCCESS_VOID(rv) failed with result 0x80004001 (NS_ERROR_NOT_IMPLEMENTED): file /home/me/prog/mozilla-central/caps/OriginAttributes.cpp:114
[70772, Main Thread] WARNING: NS_ENSURE_SUCCESS_VOID(rv) failed with result 0x80004001 (NS_ERROR_NOT_IMPLEMENTED): file /home/me/prog/mozilla-central/caps/OriginAttributes.cpp:114

And here are the logs when replying and the exception is thrown:

[70772, Main Thread] WARNING: HTMLEditor::GetAbsolutelyPositionedSelectionContainer() reached <html> element: file /home/me/prog/mozilla-central/editor/libeditor/HTMLAbsPositionEditor.cpp:94
[70772, Main Thread] WARNING: HTMLEditor::GetAbsolutelyPositionedSelectionContainer() reached <html> element: file /home/me/prog/mozilla-central/editor/libeditor/HTMLAbsPositionEditor.cpp:94
[70772, Main Thread] WARNING: HTMLEditor::GetAbsolutelyPositionedSelectionContainer() reached <html> element: file /home/me/prog/mozilla-central/editor/libeditor/HTMLAbsPositionEditor.cpp:94
[70772, Main Thread] WARNING: NS_ENSURE_TRUE(!doc->IsResourceDoc() && ((!doc->IsLoadedAsData() && aOwner-IsInComposedDoc()) || doc->IsStaticDocument())) failed: file /home/me/prog/mozilla-central/dom/base/nsFrameLoader.cpp:412
[70772, Main Thread] WARNING: NS_ENSURE_TRUE(!doc->IsResourceDoc() && ((!doc->IsLoadedAsData() && aOwner-IsInComposedDoc()) || doc->IsStaticDocument())) failed: file /home/me/prog/mozilla-central/dom/base/nsFrameLoader.cpp:412
[70772, Main Thread] WARNING: HTMLEditor::GetAbsolutelyPositionedSelectionContainer() reached <html> element: file /home/me/prog/mozilla-central/editor/libeditor/HTMLAbsPositionEditor.cpp:94
[70772, Main Thread] WARNING: HTMLEditor::GetAbsolutelyPositionedSelectionContainer() reached <html> element: file /home/me/prog/mozilla-central/editor/libeditor/HTMLAbsPositionEditor.cpp:94
[70772, Main Thread] WARNING: HTMLEditor::GetAbsolutelyPositionedSelectionContainer() reached <html> element: file /home/me/prog/mozilla-central/editor/libeditor/HTMLAbsPositionEditor.cpp:94
[70772, Main Thread] WARNING: NS_ENSURE_TRUE(frame) failed: file /home/me/prog/mozilla-central/dom/events/ContentEventHandler.cpp:1300
[70772, Main Thread] WARNING: NS_ENSURE_SUCCESS_VOID(rv) failed with result 0x80004001 (NS_ERROR_NOT_IMPLEMENTED): file /home/me/prog/mozilla-central/caps/OriginAttributes.cpp:114
[70772, Main Thread] WARNING: Failed to retarget HTML data delivery to the parser thread.: file /home/me/prog/mozilla-central/parser/html/nsHtml5StreamParser.cpp:1132
JavaScript error: , line 0: uncaught exception: Object
[70772, Main Thread] WARNING: NS_ENSURE_SUCCESS_VOID(rv) failed with result 0x80004001 (NS_ERROR_NOT_IMPLEMENTED): file /home/me/prog/mozilla-central/caps/OriginAttributes.cpp:114
[70772, Main Thread] WARNING: Failed to retarget HTML data delivery to the parser thread.: file /home/me/prog/mozilla-central/parser/html/nsHtml5StreamParser.cpp:1132
[70772, Main Thread] WARNING: 'aRv.Failed()', file /home/me/prog/mozilla-central/dom/websocket/WebSocket.cpp:1245
JavaScript error: moz-extension://ef27a09b-7b25-4ab3-b0c9-9d59979cf46e/frame.js, line 2: :
[70772, Main Thread] WARNING: 'aRv.Failed()', file /home/me/prog/mozilla-central/dom/websocket/WebSocket.cpp:1245
JavaScript error: moz-extension://ef27a09b-7b25-4ab3-b0c9-9d59979cf46e/frame.js, line 2: :

The following diff is enough to make the problem disappear:

diff -r 0e95e169ef40 dom/websocket/WebSocket.cpp
--- a/dom/websocket/WebSocket.cpp       Fri Nov 06 13:50:52 2020 +0000
+++ b/dom/websocket/WebSocket.cpp       Sat Nov 07 12:54:04 2020 +0100
@@ -1545,10 +1545,6 @@
                                    nsContentUtils::GetContentPolicy());
     NS_ENSURE_SUCCESS(rv, rv);

-    if (NS_CP_REJECTED(shouldLoad)) {
-      // Disallowed by content policy
-      return NS_ERROR_CONTENT_BLOCKED;
-    }
   }

   // If the HTTPS-Only mode is enabled, we need to upgrade the websocket

So I guess the problem is that the websocket has a different CSP between a "new message" window and a "reply" window? This doesn't make much sense as in both cases the websocket is created within an iframe that belongs to the webextension…

This patches fixes this bug. @mkmelin I see that you've worked on code adjacent to what I'm adding. Are you able to review and merge my patch? If not, do you know who I should ask?

Flags: needinfo?(mkmelin+mozilla)

Why is it only a problem in the compose window?

Flags: needinfo?(mkmelin+mozilla)
Assignee: nobody → code
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true

For review, please set the "review?" flag on the attachment to my email address.

Attachment #9186470 - Flags: review?(mkmelin+mozilla)

It's only a problem in a compose window created for replying to an email ("new email" windows are fine), so I imagine this is because the document created from the reply is considered untrusted and its CSP incorrectly "bleeds" into the web-extension's.

The relevant code for compose is here: https://searchfox.org/comm-central/rev/93bfded213518bb71f465d6fa17baaafafb9a846/mailnews/base/src/nsMsgContentPolicy.cpp#714

But what are you trying to do? Why would a compose window use websocket?

The relevant code for compose is here: https://searchfox.org/comm-central/rev/93bfded213518bb71f465d6fa17baaafafb9a846/mailnews/base/src/nsMsgContentPolicy.cpp#714

Do you mean that you think my patch is fixing this issue in the wrong place? My patch just adds the "moz-extension" scheme to the list of safe locations that nsMsgContentPolicy::IsSafeRequestingLocation knows about. It is my understanding that moz-extension locations are treated as safe in Firefox (because they act on behalf of the user).

But what are you trying to do? Why would a compose window use websocket?

In my opinion this doesn't really matter, WebSockets should just work. But if you're curious, I've ported Firenvim ( https://addons.thunderbird.net/en-US/thunderbird/addon/firenvim/ ), an extension that communicates with a Neovim server running on the user's computer.

Well, it seems at least it would do unexpected things. If I understand it correctly, the compose area is replaced by the add-on? And any loads to anywhere (including file system) would therefore be allowed from it? Seems pretty bad...

If I understand it correctly, the compose area is replaced by the add-on?

Here is what my extension does:

  • The background script launches a neovim server.
  • When a compose window is opened, an iframe whose source document is in extension storage is wrapped inside a shadow dom element and inserted in the compose window's document.
  • This iframe uses a websocket to connect to the neovim server, possibly reading files and starting programs.
  • At some point, the user decides to sync the Neovim server's content. The iframe is removed from the compose window's document and text the user wrote is added to the compose window's document.

And any loads to anywhere (including file system) would therefore be allowed from it? Seems pretty bad...

Yes, Firenvim can read anything the Neovim server can read. Note that WebSockets have nothing to do with this, reading files from the file system is already possible thanks to the native messaging API. Also note that Firenvim is used by at least two firefox devs, so connecting to a Neovim server with websockets is considered by at least multiple firefox devs.

is considered by at least multiple firefox devs.

I meant "is considered ok by".

Hmm, maybe file system access is still blocked elsewhere. The point is the quoted content is arbitrary, and could link to e.g. something in the file system to trick, and maybe steal it by including it in the reply. So giving that content a free pass to do anything seems like a problem.

And even for http links, it would let spammers be notified you read the mail you may be just forwarding to spam filtering (like some people do).

Hmm, maybe file system access is still blocked elsewhere.

I don't understand what you mean. Could you expand on that?

The point is the quoted content is arbitrary, and could link to e.g. something in the file system to trick, and maybe steal it by including it in the reply.

So is the content of web pages in web browsers. Yet multiple Firefox devs think Firenvim for Firefox is fine.

So giving that content a free pass to do anything seems like a problem.

You've already given that content a free pass to do anything when you allowed webextensions to have both compose and native permissions.

And even for http links, it would let spammers be notified you read the mail you may be just forwarding to spam filtering (like some people do).

I don't understand what you mean. The moz-extension storage is strictly controlled by a webextension. Allowing webextensions to open WebSockets does not let the original document open WebSockets as the original document does not have a moz-extension url scheme.

The (well, one) reason it's blocked for replies is that if you had say a spam with linked images in it, and you didn't load those when viewing the message, those should not get automatically loaded just because you forward the message. What I'm saying is if your add-on gives free pass to all such images (etc.) even that is bad.

Specific case of Firenvim: My add-on doesn't load any resources from the email's content. It just takes the text and puts it in an iframe, so there's no risk here.

More general case of all add-ons: I believe add-ons should be free to load any remote resources, even the ones belonging to spammers. This is because one of the fundamental assumptions behind webextensions is that the webextension is on the user's side. I'm not inventing this, see this quote:

We usually assume that the add-on is acting on the user's behalf (the user chose to install it) so it'd be nice if add-ons could bypass CSP.

Taken right from this comment, from a Firefox developer: https://bugzilla.mozilla.org/show_bug.cgi?id=1267027#c3 . I understand that Thunderbird is not Firefox anymore but given the large shared codebases and philosophies, I think it doesn't make sense to diverge from Firefox here.

(In reply to glacambre from comment #17)

Specific case of Firenvim: My add-on doesn't load any resources from the email's content. It just takes the text and puts it in an iframe, so there's no risk here.

"Just putting it in an iframe", and with your patch give all resources in there the rights to load, means exactly that those spam images would load, no?

"Just putting it in an iframe", and with your patch give all resources in there the rights to load, means exactly that those spam images would load, no?

No, sorry, I wasn't clear enough. Putting it in an iframe actually means taking the .textContent of the compose window, sending it to a neovim server and then having the server send back an individual view of each character, which are then each inserted in their own span element (there can only be a single character per span). Note that the use of HTML here will soon stop, I've written a new renderer which draws each of the characters individually on a canvas instead.

But as I've said, what Firenvim does should not matter. Webextensions should be free to load any content because they act on the behalf of the user.

Posted yesterday on matrix:

glacambre:

Basically the problem is that you can't create a websocket in a webextension iframe in a (reply) compose window. AFAIU mkmelin thinks you shouldn't be able to do that and I disagree.
mkmelin:
I don't think that is what I'm saying, but there's a reason loading is blocked there, and your patch would seem to break that protection

Mkmelin: Oh, sorry for the misunderstanding then. So the problem is only with the implementation itself and not allowing webextensions to open websockets? Could you tell me what technical solutions you envision to allow moz-extension frames to create arbitrary HTTP and WebSocket connections in a reply compose window? I'm willing to (attempt) to implement it :).

Comment on attachment 9186470 [details] [diff] [review]
fix-extension-csp.patch

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

Per previous comments, I don't think this is completely right.

I think we'd need to check the permissions the specific add-on has obtained, at least. We should also see how we can add a test for this.
(About the patch, please always use a commit message as "Bug 1675589 - why the change was done"

John might be able to give some input on what should happen here.
Attachment #9186470 - Flags: review?(mkmelin+mozilla) → feedback?(john)

I think we'd need to check the permissions the specific add-on has obtained

Which permissions would you like to check? I'm not seeing anything particularily relevant in https://thunderbird-webextensions.readthedocs.io/en/68/index.html, are there other "hidden" permissions that Thunderbird could use in order to figure out whether an extension is allowed to make a web request?

We should also see how we can add a test for this.

I'll try to figure out how to do this.

Thanks a lot for taking the time to review my patch/answer my questions! :)

I think Magnus is afraid of blocked content being made available again. But this is not the case. If the email contains content that is blocked by Thunderbird (for example remote tracking images), the WebExtension methods in the compose window (like getComposeDetails) also do not see that content, because it is not there. So whatever a WebExtension might be doing, if the user has not unblocked that content, the WebExtension is not able to see the problematic content in the compose window and thus cannot manually cause tracking or alike.

The only difference between "new" and "reply" is that the WebExtension gains (partially) access to the content of a users email. Usually this is controlled by the messagesRead permission, but for me it is ok to not be required here.

The compose permission is giving compose scripts host permission. The add-on could load whatever it wants into the composer, for example remote kitten pictures:

  await browser.tabs.executeScript(tab.id, {
    code: `
      document.execCommand("insertText", false, "Text to insert into message");
      document.execCommand("InsertImage", false, "http://placekitten.com/200/300");
      `
  })

The websocket should work as well. There is no fundamental difference.

The only thing one can think about is to separate the permissions as such as that "compose" is just giving access to the compose area, but remote content would still need a dedicated host permission. But I believe Geoff did not want to go down that route (impl. imap:// url support for host permission and adapt <all_urls> to work in compose scripts).

Sorry for taking so long.

The patch is changing code in mozilla-central (Firefox code which is also used by Thunderbird) and not in comm-central (Thunderbird specific code). You say this works in Firefox? Then this is not the correct way to fix it. I think we need to "fix" this on our side. I will have a closer look.

Attachment #9186470 - Flags: feedback?(john)

mailnews is comm-central...

Oh, I was looking at the changes in WebSocket.cpp. I keep investigating.

tbsync, mkmelin, thank you both for still interacting with this bug. I'm still hoping I'll find the time to write a test. Here's what I got so far:

diff -r 8819c8ab92f6 mailnews/base/test/unit/test_bug1675589.js
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/mailnews/base/test/unit/test_bug1675589.js	Wed Dec 02 04:07:11 2020 +0100
@@ -0,0 +1,32 @@
+/* -*- mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
+const { NetUtil } = ChromeUtils.import("resource://gre/modules/NetUtil.jsm");
+/*
+ * Test for bug 1675589.
+ */
+function run_test() {
+  const policy = Cc["@mozilla.org/messenger/content-policy;1"].getService(
+    Ci.nsIMsgContentPolicy
+  );
+
+  const context_uri = Services.io.newURI("moz-extension://00000000-0000-0000-0000-000000000000/index.html");
+  const principal = Services.scriptSecurityManager.createContentPrincipal(context_uri, {});
+
+  const resource_uri = Services.io.newURI("ws://127.0.0.1:12345");
+  const channel = NetUtil.newChannel({
+    uri: resource_uri,
+    loadingPrincipal: principal,
+    securityFlags: Ci.nsILoadInfo.SEC_ONLY_FOR_EXPLICIT_CONTENTSEC_CHECK,
+    contentPolicyType: Ci.nsIContentPolicy.TYPE_WEBSOCKET,
+  });
+
+  const decision = content_policy.shouldLoad(
+    resource_url,
+    channel.loadInfo
+  );
+
+  Assert.equal(
+    decision,
+    Ci.nsIContentPolicy.ACCEPT,
+    "moz-extensions should load webextensions"
+  );
+}
diff -r 8819c8ab92f6 mailnews/base/test/unit/xpcshell.ini
--- a/mailnews/base/test/unit/xpcshell.ini	Sat Nov 07 16:42:00 2020 +0100
+++ b/mailnews/base/test/unit/xpcshell.ini	Wed Dec 02 04:07:11 2020 +0100
@@ -9,6 +9,7 @@
 [test_accountMigration.js]
 [test_acctRepair.js]
 [test_bccInDatabase.js]
+[test_bug1675589.js]
 [test_bug428427.js]
 [test_bug434810.js]
 [test_bug471682.js]

Unfortunately this fails with:

Unexpected Results
------------------
FAIL comm/mailnews/base/test/unit/test_bug1675589.js - xpcshell return code: 0
ERROR NS_ERROR_NOT_IMPLEMENTED: Component returned failure code: 0x80004001 (NS_ERROR_NOT_IMPLEMENTED) [nsIIOService.newChannelFromURI]
NetUtil_newChannel@resource://gre/modules/NetUtil.jsm:311:31
run_test@/home/me/prog/thunderbird/obj-x86_64-pc-linux-gnu/_tests/xpcshell/comm/mailnews/base/test/unit/test_bug1675589.js:15:27
_execute_test@/home/me/prog/thunderbird/testing/xpcshell/head.js:562:7
@-e:1:1

I'm in way over my head so figuring out the correct sequence of characters will take me some time :)

Now I've got this, which looks correct to my unexperienced eyes, but which doesn't fail when my patch isn't present (so it's not correct :( ):

/* -*- mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */

/*
 * Test for bug 1675589.
 */
function run_test() {
  const content_policy = Cc["@mozilla.org/messenger/content-policy;1"].getService(
    Ci.nsIContentPolicy
  );

  const msg_content_policy = content_policy.QueryInterface(
    Ci.nsIMsgContentPolicy
  );

  const context_uri = Services.io.newURI("moz-extension://00000000-0000-0000-0000-000000000000/index.html");
  const principal = Services.scriptSecurityManager.createContentPrincipal(context_uri, {});

  const resource_uri = Services.io.newURI("ws://127.0.0.1:12345");

  const channel = Cc["@mozilla.org/network/protocol;1?name=ws"].createInstance(
    Ci.nsIWebSocketChannel
  );

  channel.initLoadInfo(
    null, // aLoadingNode
    principal,
    null, // aTriggeringPrincipal
    Ci.nsILoadInfo.SEC_ONLY_FOR_EXPLICIT_CONTENTSEC_CHECK,
    Ci.nsIContentPolicy.TYPE_WEBSOCKET,
  );

  const decision = msg_content_policy.shouldLoad(
    resource_uri,
    channel.loadInfo,
    null,
    null
  );

  Assert.equal(
    decision,
    Ci.nsIContentPolicy.ACCEPT,
    "moz-extensions should load webextensions"
  );
}
URL: https://bugzilla.mozilla.org/show_bug...
Depends on: 1707449

Hi glacambre,

I am going through old bugs which went stale. This and Bug 1675589 are probably not fixable, they both fail due to the iframe.

What I tried instead is the following compose script:

// I do not have a local websocket server, so I used this free test tool at
// https://www.piesocket.com/websocket-tester
const socket = new WebSocket("wss://demo.piesocket.com/v3/channel_123?api_key=<redacted>&notify_self");

// Connection opened
socket.addEventListener('open', (event) => {
    socket.send('Hello Server!');
});

// Listen for messages
socket.addEventListener('message', (event) => {
    console.log('Message from server ', event.data);
	const newContent = document.createTextNode(event.data);
	document.body.appendChild(newContent);
});

And this works in "new" composers and "reply" composers: it fills the composer with responses from the server. This means websocket connections work in compose scripts, but not within an iframe.

What you could do is to add a div at the top of the composer and then update the content of that div (or replace the entire body).

Does that work for you?

The iframe in the compose window is not something we want to support at the moment, so I will close this as wontfix.

Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → WONTFIX

Hi TbSync, thanks a lot for coming back to this bug after all this time.

What you could do is to add a div at the top of the composer and then update the content of that div (or replace the entire body). Does that work for you?

I had to give up on the iframe a long time ago due to https://bugzilla.mozilla.org/show_bug.cgi?id=1707449 , and IIRC indeed managed to do without for a while :).

Please feel free to mark RESOLVED+WONTFIX any/all of the thunderbird bugs/requests I made before 2023, I don't need these improvements anymore due to being blocked by https://bugzilla.mozilla.org/show_bug.cgi?id=1746468 .

You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: