Allow script injection into source editor component

RESOLVED FIXED in Firefox 40

Status

RESOLVED FIXED
4 years ago
2 months ago

People

(Reporter: ntim, Assigned: ntim)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 40

Firefox Tracking Flags

(firefox40 fixed)

Details

Attachments

(1 attachment, 8 obsolete attachments)

(Assignee)

Description

4 years ago
I would suggest a new field in the editor config called externalScripts, it's value would be an array of scripts.
(Assignee)

Comment 1

4 years ago
Created attachment 8511717 [details] [diff] [review]
Patch

This works for me
Assignee: nobody → ntim007
Status: NEW → ASSIGNED
Attachment #8511717 - Flags: review?(bgrinstead)
(Assignee)

Comment 2

4 years ago
Created attachment 8511722 [details] [diff] [review]
Patch v1.1

Better comment + replaced let with var for consistency.
Attachment #8511717 - Attachment is obsolete: true
Attachment #8511717 - Flags: review?(bgrinstead)
Attachment #8511722 - Flags: review?(bgrinstead)
(Assignee)

Comment 3

4 years ago
Created attachment 8511730 [details] [diff] [review]
Patch v2

Fixed issue where scripts were added globally across all editors.
Attachment #8511722 - Attachment is obsolete: true
Attachment #8511722 - Flags: review?(bgrinstead)
Attachment #8511730 - Flags: review?(bgrinstead)
What is the use case for this and sample usage?
(Assignee)

Comment 5

4 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.
(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?
(Assignee)

Comment 7

4 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.
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?
Flags: needinfo?(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.
Flags: needinfo?(mgoodwin)
(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
Flags: needinfo?(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.
Flags: needinfo?(mgoodwin)
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
Attachment #8511730 - Flags: review?(bgrinstead)
(Assignee)

Comment 13

4 years ago
Created attachment 8532709 [details] [diff] [review]
Patch v3

Now verifies from chrome:// prefix on each url.
Attachment #8511730 - Attachment is obsolete: true
Attachment #8532709 - Flags: review?(bgrinstead)
(Assignee)

Comment 14

4 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
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
Attachment #8532709 - Flags: review?(bgrinstead)
(Assignee)

Updated

4 years ago
Blocks: 1088996
(Assignee)

Comment 16

4 years ago
Created attachment 8564617 [details] [diff] [review]
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 :)
Attachment #8532709 - Attachment is obsolete: true
Attachment #8564617 - Flags: review?(bgrinstead)
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
Attachment #8564617 - Flags: review?(bgrinstead) → review-
(Assignee)

Comment 18

3 years ago
Created attachment 8588224 [details] [diff] [review]
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 ?
Attachment #8564617 - Attachment is obsolete: true
Attachment #8588224 - Flags: review?(bgrinstead)
(Assignee)

Updated

3 years ago
Attachment #8588224 - Attachment description: Patch v2.2 → Patch v3.2
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");
Attachment #8588224 - Flags: review?(bgrinstead)
(Assignee)

Comment 20

3 years ago
Created attachment 8593098 [details] [diff] [review]
Patch v4
Attachment #8588224 - Attachment is obsolete: true
Attachment #8593098 - Flags: review?(bgrinstead)
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
Attachment #8593098 - Flags: review?(bgrinstead) → feedback+
(Assignee)

Comment 22

3 years ago
Created attachment 8594364 [details] [diff] [review]
Patch v5
Attachment #8593098 - Attachment is obsolete: true
Attachment #8594364 - Flags: review?(bgrinstead)
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.
Attachment #8594364 - Flags: review?(bgrinstead) → review+
(Assignee)

Comment 25

3 years ago
Created attachment 8595238 [details] [diff] [review]
Patch v5.1

Try push in previous (auto-generated !) previous comment.
Attachment #8594364 - Attachment is obsolete: true
Attachment #8595238 - Flags: review+
(Assignee)

Comment 26

3 years ago
Try in comment 24 green !
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/c06221ff1d8e
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/c06221ff1d8e
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox40: --- → fixed
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 40

Updated

2 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.