1652686 - Content scripts in message composition editor are broken

Status: RESOLVED FIXED

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

Description

[Geoff Lankow (:darktrojan)]

In Thunderbird 77 I hacked together a way to run content scripts in the message composition window. It worked by overriding ExtensionContent.handleExtensionExecute to short-circuit the mechanism for checking the extension had permission, where appropriate. Although it's a hack it worked fine and will ship with Thunderbird 78.

Now bug 1587541 has come along and replaced that function the hack no longer works. I'm no longer under time pressure to get something ready for release so I can think about a proper solution.

At the least, something would have to change about this block of code, which controls if a script runs or not. I can't just override as I did before without making a copy of vast amounts of code (the contentScripts map and Script/UserScript classes which are inaccessible because they're in a JSM) and I'd rather not.

I think replacing Script.matchesWindow in some subclass would be feasible. I've just realised I can replace WebExtensionContentScript.prototype.matchesWindow but that really feels like a bad idea.

Comment 1

[Geoff Lankow (:darktrojan)]

Would you be open to exporting Script from ExtensionContent.jsm? That should be enough to reinstate our hack. Unless you have a better idea?

Comment 2

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/43b2408d2475
Temporarily disable browser_ext_composeScripts.js. rs=bustage-fix DONTBUILD

Comment 3

[Tomislav Jovanovic :zombie]

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

Would you be open to exporting Script from ExtensionContent.jsm? That should be enough to reinstate our hack. Unless you have a better idea?

Sure thing (we're not precious about exports), and sorry for the inconvenience.

Comment 4

[Rob Wu [:robwu]]

Some feedback below. Feel free to ignore if you believe that you have better options available.

I think replacing Script.matchesWindow in some subclass would be feasible.

Note that matchesWindow has been replaced with matchesWindowGlobal in https://hg.mozilla.org/mozilla-central/rev/98f8b96a4aee

What's the reason for the "compose" permission? If it is because you aren't able to define chrome:// as a permission / match pattern, then note that you can do so by extending the MatchPattern type in the schema with $extend, and ensuring that extension.restrictSchemes is true (by ensuring that isPrivileged is true and that the mozillaAddons permission is appended to the permissions). The need for mozillaAddons permission is because restrictSchemes is currently recomputed in the child. If you want to be able to override restrictSchemes without modifying the permissions set, I am willing to accept a patch to pass restrictSchemes in extension.serialize, so that the child can rely on that.
If you did that, then it could be possible to run a content script via content_scripts in manifest.json instead of with tabs.executeScript.

An alternative, to support the "compose" permission without directly exposing the chrome:-match pattern to extensions, is to skip the above part where MatchPattern is defined in the schema, but do the rest to support the chrome://... pattern internally, and monkey-patching ExtensionData's parseManifest method to map the "compose" permission to the chrome://-URL/pattern. Something like this:

• Call and await original parseManifest method.
• If this instanceof Extension and the permissions set has "compose", continue (otherwise just return the original result).
• Use Object.defineProperty to let isPrivileged return true (this is needed to unlock the next step - "mozillaAddons")
• Add mozillaAddons to the set of permissions (to unlock the next step - unrestricted schemes)
• Add COMPOSE_WINDOW_URI to the set of allowedOrigins.

This monkey-patching is a bit gross, but it would run only once per extension instead of every time when a content script is about to run.

Comment 5

[Geoff Lankow (:darktrojan)]

Thanks Rob, that looks like some useful information for when I get back to this bug. We require the compose permission for any modifications an extension can make to an email being composed, so it needs to stay.

Comment 6

[Intermittent Failures Robot]

36 failures in 4014 pushes (0.009 failures/push) were associated with this bug in the last 7 days.

This is the #37 most frequent failure this week.

** This failure happened more than 30 times this week! Resolving this bug is a high priority. **

** Try to resolve this bug as soon as possible. If unresolved for 2 weeks, the affected test(s) may be disabled. **

Repository breakdown:

• comm-central: 36

Platform breakdown:

• windows7-32: 6
• macosx1014-64: 6
• windows10-64: 6
• linux64: 6
• linux64-shippable: 3
• windows7-32-shippable: 3
• windows10-64-shippable: 3
• macosx1014-64-shippable: 3

For more details, see:
https://treeherder.mozilla.org/intermittent-failures.html#/bugdetails?bug=1652686&startday=2020-07-13&endday=2020-07-19&tree=all

Comment 7

[Geoff Lankow (:darktrojan)]

Rob, when you're doing the 80b1 release notes, please mention this in the unresolved section.

(Rob L, that is, not Rob W. This is confusing.)

Comment 8

[Geoff Lankow (:darktrojan)]

I've realised there's an easy solution to make this work again – since the document in question is actually about:blank, not messengercompose.xhtml, all I need is the matchAboutBlank property and everything is good. Except that the matchAboutBlank property works too well, and I can't (easily) prevent it being used without the permission. This is a problem already, but I'm not too concerned about it as long as it gets fixed.

I'll deal with the later problem in another bug, so far I've not been able to fix it in a satisfactory way, and I want to get this bug moving again.

Comment 9

[Geoff Lankow (:darktrojan)]

Attachment: Bug 1652686 - Fix content scripts in message composition editor. r?mkmelin!,robwu

Comment 10

[Rob Wu [:robwu]]

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

I've realised there's an easy solution to make this work again – since the document in question is actually about:blank, not messengercompose.xhtml, all I need is the matchAboutBlank property and everything is good.

This is really concerning. A content script should only be permitted in about:blank if it the extension is either allowed access to the principal that created the document, or if it's a top-level about:blank document with an opaque (null) origin.
The code responsible for enforcing that is at https://searchfox.org/mozilla-central/rev/8df04ff073d0fa70f692935c5dcddc0e23cb0117/toolkit/components/extensions/WebExtensionPolicy.cpp#637-644

Geoff, do you know why the content script is allowed to be injected?
The checks in your patch only enforce the compose permission for tabs.executeScript, but not for content scripts in manifest.json nor contentScripts.register.

Comment 11

[Geoff Lankow (:darktrojan)]

(In reply to Rob Wu [:robwu] from comment #10)

This is really concerning. A content script should only be permitted in about:blank if it the extension is either allowed access to the principal that created the document, or if it's a top-level about:blank document with an opaque (null) origin.
The code responsible for enforcing that is at https://searchfox.org/mozilla-central/rev/8df04ff073d0fa70f692935c5dcddc0e23cb0117/toolkit/components/extensions/WebExtensionPolicy.cpp#637-644

It is a top-level about:blank document with an null origin.

The checks in your patch only enforce the compose permission for tabs.executeScript, but not for content scripts in manifest.json nor contentScripts.register.

I know. If you've got any good ideas I'd like to hear them because I've come up with nothing sensible so far.

Comment 12

[Geoff Lankow (:darktrojan)]

(The outer document is messengercompose.xhtml, which is why there's a bunch of references to that in the code. Our webextension Tab objects are weird on account of Thunderbird not being a browser.)

Comment 13

[Rob Wu [:robwu]]

Does that page (or any other similar kinds of pages) have any special privileges? If not, then it will probably be fine.

Have you also checked whether a content script for <all_urls> with "match_about_blank": true would run in that page? This could potentially be more problematic, because unaware extensions could potentially inject junk in composed emails. If you haven't done so yet, consider adding a unit test for this scenario.

Comment 14

[Geoff Lankow (:darktrojan)]

(In reply to Rob Wu [:robwu] from comment #13)

Have you also checked whether a content script for <all_urls> with "match_about_blank": true would run in that page? This could potentially be more problematic, because unaware extensions could potentially inject junk in composed emails. If you haven't done so yet, consider adding a unit test for this scenario.

It does run there, and that bothers me. I'd prefer that scripts not run on top-level about:blank at all (even though that would kill off my proposed solution to this bug). Is there some way I can do that? What is the use case for it anyway?

Comment 15

[Rob Wu [:robwu]]

bug 1272890 shows a use case for top-level about:-blank.

Without any changes to toolkit/, I suppose that you can try to load about:blank? (basically a URL equivalent to about:blank but not the exact string, to bypass the logic that I linked in comment 10).
You can do that here: https://searchfox.org/comm-central/rev/b3767e49a6acf1f66abd0a276bd71579eada4787/mail/components/compose/content/messengercompose.xhtml#2531

Comment 16

[Geoff Lankow (:darktrojan)]

Changing to about:blank? does work, now that I've figured out it needs changing in two places. I had previously tried it in one place as suggested and not succeeded.

I think this is a reasonable approach and apart from a few small timing issues it doesn't appear to have any side effects. It means I need to go back to another solution to get the scripts that are supposed to work running, but that's not a big deal.

Comment 17

[Geoff Lankow (:darktrojan)]

Attachment: Bug 1652686 - Export `Script` from ExtensionContent.jsm. r?robwu

Comment 18

[Geoff Lankow (:darktrojan)]

Thanks for all your help, Rob.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/93d19e3eba91
Fix content scripts in message composition editor. r=mkmelin,robwu

Comment 20

[Rob Lemley [:rjl]]

Comment on attachment 9167627 [details]
Bug 1652686 - Fix content scripts in message composition editor. r?mkmelin!,robwu

[Approval Request Comment]
Geoff, this was in the bundle you sent me for 78.4 mailextension uplifts. I'm guessing it's needed for bug 1504475?

Comment 21

[Rob Lemley [:rjl]]

Comment on attachment 9167627 [details]
Bug 1652686 - Fix content scripts in message composition editor. r?mkmelin!,robwu

[Triage Comment]
Tests are present. Approved by wsmwk via Matrix

Comment 22

[Rob Lemley [:rjl]]

Thunderbird 78.4.0:
https://hg.mozilla.org/releases/comm-esr78/rev/b73b456b1194