Tim Nguyen :ntim Assignee
	Description • 10 years ago

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

	Tim Nguyen :ntim Assignee
	Comment 1 • 10 years ago

This works for me

	Tim Nguyen :ntim Assignee
	Comment 2 • 10 years ago

Better comment + replaced let with var for consistency.

	Tim Nguyen :ntim Assignee
	Comment 3 • 10 years ago

Fixed issue where scripts were added globally across all editors.

	Brian Grinstead [:bgrins]
	Comment 4 • 10 years ago

What is the use case for this and sample usage?

	Tim Nguyen :ntim Assignee
	Comment 5 • 10 years ago

(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.

	Brian Grinstead [:bgrins]
	Comment 6 • 10 years ago

(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?

	Tim Nguyen :ntim Assignee
	Comment 7 • 10 years ago

(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.

	Brian Grinstead [:bgrins]
	Comment 8 • 10 years ago

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?

	Mark Goodwin [:mgoodwin]
	Comment 9 • 10 years ago

(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.

	Brian Grinstead [:bgrins]
	Comment 10 • 10 years ago

(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

	Mark Goodwin [:mgoodwin]
	Comment 11 • 10 years ago

(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.

	Brian Grinstead [:bgrins]
	Comment 12 • 10 years ago

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

	Tim Nguyen :ntim Assignee
	Comment 13 • 10 years ago

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

	Tim Nguyen :ntim Assignee
	Comment 14 • 10 years ago

(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

	Brian Grinstead [:bgrins]
	Comment 15 • 10 years ago

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

	Tim Nguyen :ntim Assignee
	Comment 16 • 10 years ago

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

	Brian Grinstead [:bgrins]
	Comment 17 • 10 years ago

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

	Tim Nguyen :ntim Assignee
	Comment 18 • 10 years ago

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

	Brian Grinstead [:bgrins]
	Comment 19 • 10 years ago

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");

	Brian Grinstead [:bgrins]
	Comment 21 • 10 years ago

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

	Brian Grinstead [:bgrins]
	Comment 23 • 10 years ago

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.

	Tim Nguyen :ntim Assignee
	Comment 24 • 10 years ago

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

	Tim Nguyen :ntim Assignee
	Comment 25 • 10 years ago

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

	Tim Nguyen :ntim Assignee
	Comment 26 • 10 years ago

Try in comment 24 green !

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 27 • 10 years ago

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

	Carsten Book [:Tomcat]
	Comment 28 • 10 years ago

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