The default bug view has changed. See this FAQ.

hide Browser Console JS input field if devtools.chrome.enabled is false

VERIFIED FIXED in Firefox 30

Status

()

Firefox
Developer Tools: Console
P3
normal
VERIFIED FIXED
4 years ago
3 years ago

People

(Reporter: Gavin, Assigned: Henri Kinnunen)

Tracking

(Blocks: 1 bug, {dev-doc-complete, sec-want})

Trunk
Firefox 30
dev-doc-complete, sec-want
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first bug][lang=js][mentor=msucan])

Attachments

(2 attachments, 2 obsolete attachments)

To protect against bug 664589-style attacks, disabling the browser console's chrome-privileged JS evaluator by default seems like the best tradeoff.

I propose just having its visibility controlled by the existing devtools.chrome.enabled pref.
Duplicate of this bug: 894380
Priority: -- → P3
Whiteboard: [good first bug][lang=js][mentor=msucan]
(Assignee)

Comment 2

3 years ago
I would like to take this one.
Henri: thank you for your interest to help us with this bug!


Overview of the workflow:

1. Get the firefox source:

https://developer.mozilla.org/en-US/docs/Developer_Guide/Source_Code/Mercurial

2. Build firefox:

https://developer.mozilla.org/en-US/docs/Simple_Firefox_build

3. Make a patch:

https://developer.mozilla.org/en-US/docs/Creating_a_patch

4. Submit the patch:

https://developer.mozilla.org/en-US/docs/Developer_Guide/How_to_Submit_a_Patch

Please make sure you ask for feedback from me. Set the patch flag feedback? and write my nick, :msucan.

If you read through the source code about something you do not know about, you may find documentation here:

1. Mozilla Developer Network - has a ton of info about XUL elements, HTML, JS, DOM, Gecko-specific APIs and more.

http://developer.mozilla.org/

2. MXR (Mozilla Cross-Reference) is a source code search engine - search for symbols you want to learn about, eg. nsIDocument.

http://mxr.mozilla.org/mozilla-central/

3. DXR is a smart source code search tool, similar to MXR but sometimes better.

http://dxr.mozilla.org


Here's an overview of how you can fix this bug:

Edit browser/devtools/webconsole/webconsole.js. Find the JST_init() function and hide the |inputNode| when devtools.chrome.enabled is false. To read the preference use Services.prefs.getBoolPref(). You can find examples in the same JS file which use Services.prefs. Also check if this.hud.owner._browserConsole is true - we only want to hide the input in the browser console. Last, but not least, when you hide the inputNode, make sure you change the code to not add any event listeners to the input. You might need to hide the completeNode as well, or just hide the parent container. To see the markup, look at webconsole.xul file, in the same folder.


If you have any questions, please ask me here. Use the needinfo flag and write my nick, :msucan.

I will explain how you can write a test once you have a working patch.

Thanks and good luck!
Assignee: nobody → hequmania
Status: NEW → ASSIGNED
(Assignee)

Comment 4

3 years ago
Created attachment 8349022 [details] [diff] [review]
disable browser console if devtools.chrome.enabled == false
Attachment #8349022 - Flags: feedback?(mihai.sucan)
(Assignee)

Comment 5

3 years ago
Sould we somehow tell the user that the console input is not available unless the user checks the devtools.chrome.enabled checkbox? I think it's not very clear at the moment that why the console input field is missing but the console itself is visible.
(In reply to Henri Kinnunen from comment #5)
> Sould we somehow tell the user that the console input is not available
> unless the user checks the devtools.chrome.enabled checkbox? I think it's
> not very clear at the moment that why the console input field is missing but
> the console itself is visible.

Good question. I would say yes. However, I am not sure if we should do this - it might defeat the purpose of this bug.

Gavin, should we add a message letting users know the JS input is disabled? What kind of wording would be acceptable? "The JavaScript input field is disabled. To enable it go to about:config and set devtools.chrome.enabled to true." ? Should we make the message a link with a callback that toggles the pref on click? Thank you!
Flags: needinfo?(gavin.sharp)
Comment on attachment 8349022 [details] [diff] [review]
disable browser console if devtools.chrome.enabled == false

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

Henri, thank you for the patch!

With this patch, the Browser Console stops working if I set devtools.chrome.enabled to false. I get exceptions related to the inputNode.

This patch includes trailing whitespace. Please remove it.

::: browser/devtools/webconsole/webconsole.js
@@ +3054,3 @@
>      this.completeNode = doc.querySelector(".jsterm-complete-node");
> +
> +    this.jsTermInputContainer = doc.querySelector("hbox.jsterm-input-container")

The jsterm input container is not needed outside this function. Just make it a variable, let inputContainer = ...

@@ +3058,5 @@
> +    if (!Services.prefs.getBoolPref("devtools.chrome.enabled") && this.hud.owner._browserConsole === true) {
> +      this.jsTermInputContainer.style.display = "none"
> +    } else {
> +      this.jsTermInputContainer.style.display = "block"
> +      this.inputNode = doc.querySelector(".jsterm-input-node");

You should have this.inputNode set, irrespective of the preference. Just dont add the event listeners.
Attachment #8349022 - Flags: feedback?(mihai.sucan)
(In reply to Mihai Sucan [:msucan] from comment #6)
> Gavin, should we add a message letting users know the JS input is disabled?
> What kind of wording would be acceptable? "The JavaScript input field is
> disabled. To enable it go to about:config and set devtools.chrome.enabled to
> true." ? Should we make the message a link with a callback that toggles the
> pref on click? Thank you!

I don't see why we'd do that. The idea is that it shouldn't be easy to get a privileged console, and I don't really see understand why you think users would have an expectation of there being a console in the "Browser Console". Is the concern that they'd expect it because the similar-looking Web Console has one? The two are pretty clearly separately labeled, so I wouldn't really worry about that.
Flags: needinfo?(gavin.sharp)
(In reply to :Gavin Sharp (email gavin@gavinsharp.com) from comment #8)
> The idea is that it shouldn't be easy to get a privileged console...

Just a side-note: It's still easy to get one, if you pick a privileged chrome page, like about:about and open the devtools console
(In reply to :Gavin Sharp (email gavin@gavinsharp.com) from comment #8)
> (In reply to Mihai Sucan [:msucan] from comment #6)
> > Gavin, should we add a message letting users know the JS input is disabled?
> > What kind of wording would be acceptable? "The JavaScript input field is
> > disabled. To enable it go to about:config and set devtools.chrome.enabled to
> > true." ? Should we make the message a link with a callback that toggles the
> > pref on click? Thank you!
> 
> I don't see why we'd do that. The idea is that it shouldn't be easy to get a
> privileged console, and I don't really see understand why you think users
> would have an expectation of there being a console in the "Browser Console".
> Is the concern that they'd expect it because the similar-looking Web Console
> has one? The two are pretty clearly separately labeled, so I wouldn't really
> worry about that.

The question came because, yes, there's strong resemblance between the web console and the browser console, and we have releases of firefox that include the input - hiding it without notice might surprise users. Also, the error console had a js input, by default, which will make the browser console less useful than the error console was.

Simply hiding the js input in the browser console does not make it read-only either. We still have the object inspector, which shows up for any console.foo() method. Users can inspect and edit properties in objects. It seems we should also disallow editing of properties when devtools.chrome.enabled = false.

As Frederik points out, it's easy to bring back chrome privileges with the web console on a chrome privileged page. It is equally simple to just open the devtools, go to options and pick 'enable chrome debugging' (which turns devtools.chrome.enabled to true).
(In reply to Henri Kinnunen from comment #5)
> Sould we somehow tell the user that the console input is not available
> unless the user checks the devtools.chrome.enabled checkbox? I think it's
> not very clear at the moment that why the console input field is missing but
> the console itself is visible.

Henri, please also disallow the editing/renaming/deleting of properties in the object inspector. Please update JST__updateVariablesView(). You will see we check if the variables view displays an object actor (aOptions.objectActor) and we set the |eval|, |switch| and |delete| functions, otherwise we set those to null so no changes can be made. Add a check for devtools.chrome.enabled, such that we get a read-only object inspector if the pref is false.

Thank you!
(In reply to Frederik Braun [:freddyb] from comment #9)
> Just a side-note: It's still easy to get one, if you pick a privileged
> chrome page, like about:about and open the devtools console

That's harder than "Click Tools->Browser Console".
(In reply to :Gavin Sharp (email gavin@gavinsharp.com) from comment #12)
> (In reply to Frederik Braun [:freddyb] from comment #9)
> > Just a side-note: It's still easy to get one, if you pick a privileged
> > chrome page, like about:about and open the devtools console
> 
> That's harder than "Click Tools->Browser Console".

Or even using the keyboard shortcut to open the browser console. IMHO that is the only attack scenario worthy of extra countermeasures:

"Pres Ctrl-Shift-J and then copy & paste the following s3cr3t 1337 code to get a million more chicken for your farm!"
Fixing this bug as summarized addresses that case.
Henri, sorry to bother you, do you have any progress with the patch? Please let us know if you need any help. Thanks!
Flags: needinfo?(hequmania)
(Assignee)

Comment 16

3 years ago
No that's fine, I just haven't had time to work on this. I've had such a busy holidays but I hope that I can continue on this today.. :) 

I think that I have enough information to implement this feature but if not, then I'll ask more help. Thanks for asking. :)
Flags: needinfo?(hequmania)
(Assignee)

Comment 17

3 years ago
Created attachment 8364575 [details] [diff] [review]
disable browser console and object inspector edit mode if devtools.chrome.enabled == false
Attachment #8364575 - Flags: feedback?(mihai.sucan)
Comment on attachment 8364575 [details] [diff] [review]
disable browser console and object inspector edit mode if devtools.chrome.enabled == false

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

Thanks for the updated patch.

Do you have time to write a test? To add a test, please learn about browser chrome tests:

https://developer.mozilla.org/en-US/docs/Browser_chrome_tests

Go to browser/devtools/webconsole/test and copy one of the "browser_" JS test files. Name the new file as you see fit, add the new file name to the list of tests in browser.ini. Update the test to test the bug you are fixing. Run mach/make to update the local build, then run the test as explained in the provided link.

::: browser/devtools/webconsole/webconsole.js
@@ +3053,5 @@
> +    let inputContainer = doc.querySelector("hbox.jsterm-input-container");
> +
> +    this.inputNode = doc.querySelector(".jsterm-input-node");
> +
> +    if (!Services.prefs.getBoolPref("devtools.chrome.enabled")) {

This is hiding the input in the web console as well. We need to only hide the input in the browser console.

Please also check if this.owner._browserConsole is true.

(update both location where you read the pref)
Attachment #8364575 - Flags: feedback?(mihai.sucan) → feedback+
(Assignee)

Comment 19

3 years ago
Should I also disable the edit mode of object inspector only in the browser console or in both web and browser consoles?
Flags: needinfo?(mihai.sucan)
(In reply to Henri Kinnunen from comment #19)
> Should I also disable the edit mode of object inspector only in the browser
> console or in both web and browser consoles?

Yes, also disable edit mode of object inspector, only in the browser console. Thanks!
Flags: needinfo?(mihai.sucan)

Updated

3 years ago
Blocks: 971597

Comment 21

3 years ago
Please do the same for the browser console (⌘⌥K) when it is pointed at a privileged page such as about:home. Keeping in mind that you can navigate from an untrusted page to a privileged page or vice versa.

We should also do something for unprivileged pages to prevent account hijacking (bug 971613), but that will probably need a different solution and different pref(s).
(In reply to Jesse Ruderman from comment #21)
> Please do the same for the browser console (⌘⌥K) when it is pointed at a
> privileged page such as about:home. Keeping in mind that you can navigate
> from an untrusted page to a privileged page or vice versa.

The browser console is ⌘⌥J, and it points to the browser chrome window rather than a web page.

Assuming this is in relation to the Facebook self-xss problems, please could you explain what the self-xss instructions would look like which used about:home to attack a user? (Perhaps a separate bug would be best)
Keywords: sec-want

Comment 23

3 years ago
Self-XSS using about:home would look like:

1. ⌘T
2. ⌘⌥K
3. Components
(Assignee)

Comment 24

3 years ago
Created attachment 8376999 [details] [diff] [review]
disables jsterm input and does not set object inspector properties when devtools.chrome.enabled == false  tests included.
Attachment #8349022 - Attachment is obsolete: true
Attachment #8364575 - Attachment is obsolete: true
Attachment #8376999 - Flags: feedback?(mihai.sucan)
Comment on attachment 8376999 [details] [diff] [review]
disables jsterm input and does not set object inspector properties when devtools.chrome.enabled == false  tests included.

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

The test for browser_console_dead_objects.js fails with this patch applied because the input field is hidden by default. I added a setBoolPref() call to enable the input and the test passes.

I sent the patch to the try servers: https://tbpl.mozilla.org/?tree=Try&rev=27a2014545aa

If this is green, I will land the patch. Thanks for your work!

::: browser/devtools/webconsole/test/browser_console_hide_jsterm_when_devtools_chrome_enabled_false.js
@@ +12,5 @@
> +*    -webconsole object inspector properties should be set.
> +*/
> +
> +function testObjectInspectorPropertiesAreNotSet(variablesView) {
> +  is(variablesView.eval, null);

For future reference: every is() call needs to take 3 arguments. the third gives the check a name that we can read in the logs after TEST-PASS.
Attachment #8376999 - Flags: feedback?(mihai.sucan) → review+
Created attachment 8378222 [details] [diff] [review]
bug922161-1.diff

Try push was green. This is the patch with minor changes (nits).

Landed: https://hg.mozilla.org/integration/fx-team/rev/23e724fda922

Thank you Henri!
Attachment #8378222 - Flags: checkin+

Updated

3 years ago
Whiteboard: [good first bug][lang=js][mentor=msucan] → [good first bug][lang=js][mentor=msucan][fixed-in-fx-team]
(Assignee)

Comment 27

3 years ago
(In reply to Mihai Sucan [:msucan] from comment #26)
> Created attachment 8378222 [details] [diff] [review]
> bug922161-1.diff
> 
> Try push was green. This is the patch with minor changes (nits).
> 
> Landed: https://hg.mozilla.org/integration/fx-team/rev/23e724fda922
> 
> Thank you Henri!

Woohoo! \o/ Thank you! :)
Will: we need MDN docs for the browser console to mention to users that the devtools.chrome.enabled pref needs to be set to true, if they want the JS input. Thanks!
Keywords: dev-doc-needed
https://hg.mozilla.org/mozilla-central/rev/23e724fda922
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [good first bug][lang=js][mentor=msucan][fixed-in-fx-team] → [good first bug][lang=js][mentor=msucan]
Target Milestone: --- → Firefox 30
Now the input field is displayed in the browser console only if devtools.chrome.enabled=TRUE.
Verified fixed FF 30.0a1 (2014-02-21), Win 7, Mac OS X 10.8.5, Ubuntu 13.04.
Status: RESOLVED → VERIFIED
documented -> https://developer.mozilla.org/en-US/docs/Tools/Browser_Console#Browser_Console_command_line
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.