1089428 - Allow script injection into source editor component

Status: RESOLVED FIXED

(DevTools :: Source Editor, defect)

Description

[Tim Nguyen :ntim]

I would suggest a new field in the editor config called externalScripts, it's value would be an array of scripts.

Comment 1

[Tim Nguyen :ntim]

Attachment: Patch

This works for me

Comment 2

[Tim Nguyen :ntim]

Attachment: Patch v1.1

Better comment + replaced let with var for consistency.

Comment 3

[Tim Nguyen :ntim]

Attachment: Patch v2

Fixed issue where scripts were added globally across all editors.

Comment 4

[Brian Grinstead [:bgrins]]

What is the use case for this and sample usage?

Comment 5

[Tim Nguyen :ntim]

(In reply to Brian Grinstead [:bgrins] from comment #4)
> What is the use case for this and sample usage?

Letting an editor instance in an add-on use a plugin such as Emmet, or any other codemirror plugin.

Comment 6

[Brian Grinstead [:bgrins]]

(In reply to Tim Nguyen [:ntim] from comment #5)
> (In reply to Brian Grinstead [:bgrins] from comment #4)
> > What is the use case for this and sample usage?
> 
> Letting an editor instance in an add-on use a plugin such as Emmet, or any
> other codemirror plugin.

So what would sample code for this be, both for including the script and for initializing the plugin?

Comment 7

[Tim Nguyen :ntim]

(In reply to Brian Grinstead [:bgrins] from comment #6)
> (In reply to Tim Nguyen [:ntim] from comment #5)
> > (In reply to Brian Grinstead [:bgrins] from comment #4)
> > > What is the use case for this and sample usage?
> > 
> > Letting an editor instance in an add-on use a plugin such as Emmet, or any
> > other codemirror plugin.
> 
> So what would sample code for this be, both for including the script and for
> initializing the plugin?
See this commit : https://github.com/nt1m/devtools-prototyper/commit/fa9e59c1a9e250808d563cd29edf053909658910
This code works with the patch applied.

So basically this is a new field in the config object called externalScripts, with an array of urls as value.

Comment 8

[Brian Grinstead [:bgrins]]

Mark, is there a risk to allowing scripts to be injected inside of the editor component by an extension in devtools?  The editor component is the widget wrapping CodeMirror, loaded inside of an iframe inside of a devtools panel.

It seems like including a script that's already bundled within the extension would be fine, since they are already running a script in the first place.  I'm worried in particular about an extension loading remotely hosted scripts.  Could/should this be prevented?

Comment 9

[Mark Goodwin [:mgoodwin]]

(In reply to Brian Grinstead [:bgrins] from comment #8)
> Mark, is there a risk to allowing scripts to be injected inside of the
> editor component by an extension in devtools?  The editor component is the
> widget wrapping CodeMirror, loaded inside of an iframe inside of a devtools
> panel.

Yes, if the script comes from an untrusted source (e.g. the Internet). We may be able to manage this risk, though.

> It seems like including a script that's already bundled within the extension
> would be fine, since they are already running a script in the first place. 
> I'm worried in particular about an extension loading remotely hosted
> scripts.  Could/should this be prevented?

There are a couple of things that could be done, depending on how the scripts are injected:

A technical solution could be to whitelist allowed schemes (if you're loading them that way).

Another solution (these aren't mutually exclusive, of course) is to let the AMO review people know about new and dangerous APIs so they know to look for them.

Comment 10

[Brian Grinstead [:bgrins]]

(In reply to Mark Goodwin [:mgoodwin] from comment #9)
> (In reply to Brian Grinstead [:bgrins] from comment #8)
> > Mark, is there a risk to allowing scripts to be injected inside of the
> > editor component by an extension in devtools?  The editor component is the
> > widget wrapping CodeMirror, loaded inside of an iframe inside of a devtools
> > panel.
> 
> Yes, if the script comes from an untrusted source (e.g. the Internet). We
> may be able to manage this risk, though.
> 

Correct my if I'm wrong - if someone is already running code in an extension, nothing would technically stop them from injecting a script from the Internet in a some other manner (like injecting a script directly into browser.xul).  Except that the AMO reviewers would know to look for things like this.  So if I'm understanding correctly, we basically don't want to open up new ways (unknown to reviewers) to inject untrusted scripts.

> > It seems like including a script that's already bundled within the extension
> > would be fine, since they are already running a script in the first place. 
> > I'm worried in particular about an extension loading remotely hosted
> > scripts.  Could/should this be prevented?
> 
> There are a couple of things that could be done, depending on how the
> scripts are injected:
> 
> A technical solution could be to whitelist allowed schemes (if you're
> loading them that way).
> 

Seems like a good idea - I don't see a use case where we would want someone to be able to directly load a script from the Internet.  Mark, what's the best way to do this?  Were you thinking of parsing out the URL, putting a CSP on the iframe, or something else?  FWIW we are currently using Services.scriptloader.loadSubScript to load the script: http://dxr.mozilla.org/mozilla-central/source/browser/devtools/sourceeditor/editor.js#262

Comment 11

[Mark Goodwin [:mgoodwin]]

(In reply to Brian Grinstead [:bgrins] from comment #10)
> Correct my if I'm wrong - if someone is already running code in an
> extension, nothing would technically stop them from injecting a script from
> the Internet in a some other manner (like injecting a script directly into
> browser.xul).  Except that the AMO reviewers would know to look for things
> like this.  So if I'm understanding correctly, we basically don't want to
> open up new ways (unknown to reviewers) to inject untrusted scripts.

That's exactly right, technical measures only get us so far; we need to be explicit about what's still dangerous.

> Seems like a good idea - I don't see a use case where we would want someone
> to be able to directly load a script from the Internet.  Mark, what's the
> best way to do this?  Were you thinking of parsing out the URL, putting a
> CSP on the iframe, or something else?  FWIW we are currently using
> Services.scriptloader.loadSubScript to load the script:
> http://dxr.mozilla.org/mozilla-central/source/browser/devtools/sourceeditor/
> editor.js#262

I'm not sure the relevant CSP bits are ready for this yet (I'll check up on this separately).

You could parse out the URL and check the protocol, but takes care; in particular, you want to ensure you whitelist (check for allowed rather than denying disallowed), since other addons can make their own protocol handlers. Also, you need to ensure that you're looking at the inner URI - e.g. if you're allowing jar: then jar:http://some.example.com probably isn't what you want. Finally, be careful with protocols where addons can register stuff dynamically at run-time (e.g. about:)

But whatever you choose to do here, you should also contact the the AMO reviewers to let them know that there's a vector for loading script in this context. I expect they'll be able to let you know any requirements that make things easier for them, and contacting them gives them opportunity to modify tooling.

Comment 12

[Brian Grinstead [:bgrins]]

Comment on attachment 8511730 [details] [diff] [review]
Patch v2

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

Will you please update this as per Comment 11, either by applying a CSP or adding a whitelist of allowed protocols (maybe just chrome://)?

::: browser/devtools/sourceeditor/editor.js
@@ +220,5 @@
>      cm.replaceSelection(" ".repeat(num), "end", "+input");
>    };
>  
> +  // Allow add-ons to inject scripts for their editor instances
> +  if (this.config.externalScripts) {

I wouldn't bother setting this on the object here.  Instead just do what gutters does:

  if (!this.config.externalScripts) {
    this.config.externalScripts = [];
  }

Then concat with this.config.externalScripts later

Comment 13

[Tim Nguyen :ntim]

Attachment: Patch v3

Now verifies from chrome:// prefix on each url.

Comment 14

[Tim Nguyen :ntim]

(In reply to Tim Nguyen [:ntim] from comment #13)
> Created attachment 8532709 [details] [diff] [review]
> Patch v3
> 
> Now verifies from chrome:// prefix on each url.

*for

Comment 15

[Brian Grinstead [:bgrins]]

Comment on attachment 8532709 [details] [diff] [review]
Patch v3

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

We need a new test for this.  All of the current source editor tests are using a standard setup() method that puts it in a new window.  That could work, but it will add complexity since we need to change the options.  In this case we could use something simpler that just does a new Editor() with the relevant options and checks to make sure it works.  Let me know if you need some help getting the test set up

Comment 16

[Tim Nguyen :ntim]

Attachment: Patch v3.1 with tests (WIP)

So the test fails when I run it, I'm not sure why though, can you take a look ? Thanks :)

Comment 17

[Brian Grinstead [:bgrins]]

Comment on attachment 8564617 [details] [diff] [review]
Patch v3.1 with tests (WIP)

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

::: browser/devtools/sourceeditor/editor.js
@@ +264,5 @@
>          win.document.documentElement.setAttribute("force-theme", "light");
>  
> +      let scriptsToInject = CM_SCRIPTS.concat(this.config.externalScripts);
> +      scriptsToInject.forEach((url) => {
> +        if(url.startsWith("chrome://"))

Nit: add a space between the if and (

::: browser/devtools/sourceeditor/test/browser_editor_script_injection.js
@@ +10,5 @@
> +  const url = "data:text/xml;charset=UTF-8,<?xml version='1.0'?>" +
> +    "<?xml-stylesheet href='chrome://global/skin/global.css'?>" +
> +    "<window xmlns='http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul'" +
> +    " title='Editor' width='600' height='500'><box flex='1'/></window>";
> +  const injectedScriptURL = "chrome://mochitests/content/browser/browser/devtools/test/cm_script_injection_test.js";

Needs to be chrome://mochitests/content/browser/browser/devtools/sourceeditor/test/cm_script_injection_test.js

@@ +36,5 @@
> +        })
> +        .then(null, (err) => ok(false, err.message));
> +    }, win);
> +  }, false);
> +

Nit: remove extra empty lines

::: browser/devtools/sourceeditor/test/cm_script_injection_test.js
@@ +1,2 @@
> +window.addEventListener("load", () => {
> +	CodeMirror.setText("Script successfully injected !")

This doesn't work - there is no instance of the editor attached to the window.  You will need to say `win.editor = this;` inside the onLoad call in editor.js and then say `editor.setText` here

Comment 18

[Tim Nguyen :ntim]

Attachment: Patch v3.2

Brain, there seems to be an assertion, I can't figure out why with the logging. Can you take a look please ?

Comment 19

[Brian Grinstead [:bgrins]]

Comment on attachment 8588224 [details] [diff] [review]
Patch v3.2

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

::: browser/devtools/sourceeditor/test/browser_editor_script_injection.js
@@ +10,5 @@
> +  const injectedText = "Script successfully injected !";
> +
> +  setup((ed, win) => {
> +    win.editor = this;
> +    ok(ed.getText() == injectedText);

It's complaining because you haven't passed in a name to the call to ok().  In this case you should use `is` anyway. So this line can be replaced with:

    is(ed.getText(), injectedText, "The text has been injected");

Comment 20

[Tim Nguyen :ntim]

Attachment: Patch v4

Comment 21

[Brian Grinstead [:bgrins]]

Comment on attachment 8593098 [details] [diff] [review]
Patch v4

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

Code looks good, just want to see a couple updates to the test case

::: browser/devtools/sourceeditor/editor.js
@@ +341,5 @@
>        this._prefObserver.on(DETECT_INDENT, this.reloadPreferences);
>        this._prefObserver.on(ENABLE_CODE_FOLDING, this.reloadPreferences);
>  
>        this.reloadPreferences();
> +      

Nit trailing whitespace

::: browser/devtools/sourceeditor/test/browser_editor_script_injection.js
@@ +8,5 @@
> +
> +  const injectedScriptURL = "chrome://mochitests/content/browser/browser/devtools/sourceeditor/test/cm_script_injection_test.js";
> +  const injectedText = "Script successfully injected !";
> +
> +  setup((ed, win) => {

Please switch this test add_task so you can just yield on setup (kind of like https://dxr.mozilla.org/mozilla-central/source/browser/devtools/sourceeditor/test/browser_editor_autocomplete_events.js#21).

After that, can you add a second assertion to this test that injects / uses a custom mode (maybe something like [0])?  Since this is the most clear use case for injected scripts I'd like to make sure that works.

[0]: https://github.com/codemirror/CodeMirror/blob/master/mode/haml/haml.js

Comment 22

[Tim Nguyen :ntim]

Attachment: Patch v5

Comment 23

[Brian Grinstead [:bgrins]]

Comment on attachment 8594364 [details] [diff] [review]
Patch v5

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

r=me with the added comment and a try push

::: browser/devtools/sourceeditor/test/browser_editor_script_injection.js
@@ +1,5 @@
> +/* Any copyright is dedicated to the Public Domain.
> +   http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +"use strict";
> +

Please add a basic comment here explaining what the test does.  Could be as simple as:

// Test the externalScripts option, which allows custom language modes or
// other scripts to be injected into the editor window.  See Bug 1089428.

Comment 24

[Tim Nguyen :ntim]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fc8d1da6507a

Comment 25

[Tim Nguyen :ntim]

Attachment: Patch v5.1

Try push in previous (auto-generated !) previous comment.

Comment 26

[Tim Nguyen :ntim]

Try in comment 24 green !

Comment 27

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/integration/fx-team/rev/c06221ff1d8e

Comment 28

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c06221ff1d8e