994134 - Warn first time users on pasting code into the console

Status: RESOLVED FIXED

(DevTools :: Console, defect, P3)

Description

[Manish Goregaokar [:manishearth]]

From a suggestion on the Chromium counterpart of our dev-self-xss bug: https://code.google.com/p/chromium/issues/detail?id=345205#c18

As an addition to the CSP solution to the dev-self-xss issue (bug 971597), why not disallow first time users of the console from *pasting* code into the console, or at least show a specific warning?

Not sure how we determine if a user is a first time user, though.

Comment 1

[Manish Goregaokar [:manishearth]]

Jesse, any thoughts on this?

Comment 2

[Jesse Ruderman]

Sounds reasonable. Probably good to give drags the same treatment as pastes.

Comment 3

[Manish Goregaokar [:manishearth]]

This seems to be getting implemented in Chrome (Patch: http://src.chromium.org/viewvc/blink?view=revision&revision=171275)

The heuristic is that if the user has less than 10 entries in their console history, warn them.

Comment 4

[Manish Goregaokar [:manishearth]]

Jeff Griffiths proposes that this be done, but limited only to Release and Beta (bug 664589 comment 47). Sounds reasonable to me.

Comment 5

[Manish Goregaokar [:manishearth]]

I can try working on this after May begins, if that's okay with you. I have an endterm going on right now.

Comment 6

[Manish Goregaokar [:manishearth]]

Also, I guess that if the user bypasses the dialog "I know what I'm doing", this should set a pref to not bother them again.

However, scammers may tell the user to then just fix this pref from about:config. One way to get around this is to add an observer to about:config that emits another warning. Chrome has an internal pref, but Chrome's preferences system isn't easy to tamper with as a user.


Should we have this pref? If so, should we have an observer for it as well?

Comment 7

[Manish Goregaokar [:manishearth]]

Attachment: Basic functionality

This is a rather basic patch for the Web Console. Unfortunately the history is not persistent by default, so I made a pref that is flipped when the history crosses 10.

Some issues:

 - Should we do this for the Scratchpad too?
 - We need better strings, I'm terrible at writing warning messages
 - Should there be any warning if the value of the pref is changed directly from about:config? Or should the default about:config warning suffice?
 - This requires the history to cross 10 in a particular window. Instead, we can make it persistent by storing the number of commands entered (until it reaches 10, no need to keep counting after that) in the pref and block pasting accordingly.

Comment 8

[Manish Goregaokar [:manishearth]]

Oh, also I'll have to get it working for dragdrop. I'll wait for some feedback on the implementation before going ahead.

Comment 9

[Joe Walker [:jwalker]]

(In reply to Manish Goregaokar [:manishearth] from comment #7)
> Created attachment 8411484 [details] [diff] [review]
> Basic functionality
> 
> This is a rather basic patch for the Web Console. Unfortunately the history
> is not persistent by default, so I made a pref that is flipped when the
> history crosses 10.
> 
> Some issues:
> 
>  - Should we do this for the Scratchpad too?

Scratchpad is available from a single key press, although the key sequences involved are all more complex. If we're going to include scratchpad, then we need to include uses of scratchpad in the "use count"

>  - We need better strings, I'm terrible at writing warning messages
>  - Should there be any warning if the value of the pref is changed directly
> from about:config? Or should the default about:config warning suffice?

The warning should suffice. There are many other ways to make a victim vulnerable if you can coerce them to alter about:config, and there is no evidence that I've seen of that being an attack vector.

This goes for comment 6 too.

>  - This requires the history to cross 10 in a particular window. Instead, we
> can make it persistent by storing the number of commands entered (until it
> reaches 10, no need to keep counting after that) in the pref and block
> pasting accordingly.

I think we need this. It's easy to imagine someone using the web console for quite a while without crossing the 10 history items in a single window.

(In reply to Manish Goregaokar [:manishearth] from comment #8)
> Oh, also I'll have to get it working for dragdrop. I'll wait for some
> feedback on the implementation before going ahead.

I'm trying to imagine an attack script that uses drag and drop, and it feels like it's going to be too complex to be viable. (I blogged about the data that I have here: http://incompleteness.me/blog/2014/04/24/combatting-self-xss-part-2/)

But that said, the cost of adding it to the user could be zero, so I don't think I'm against it.

We have another issue in terms of testing. Jeff is happy with this being on for Release and Beta users, but we have to go through Nightly and Aurora to get there. Perhaps we should have a several phase landing:
* get the history thing in place first
* deploy the actual protection to nightly and aurora
* when it's reached release, turn off the protection in nightly and aurora

We might also challenge the requirement that it's for release and beta only.

Comment 10

[Joe Walker [:jwalker]]

Mihai - do you have thoughts?

Comment 11

[Mihai Sucan [:msucan]]

(In reply to Joe Walker [:jwalker] from comment #9)
> (In reply to Manish Goregaokar [:manishearth] from comment #7)
> > Created attachment 8411484 [details] [diff] [review]
> > Basic functionality
> > 
> > This is a rather basic patch for the Web Console. Unfortunately the history
> > is not persistent by default, so I made a pref that is flipped when the
> > history crosses 10.
> > 
> > Some issues:
> > 
> >  - Should we do this for the Scratchpad too?
> 
> Scratchpad is available from a single key press, although the key sequences
> involved are all more complex. If we're going to include scratchpad, then we
> need to include uses of scratchpad in the "use count"

If we are adding this kind of warning to the console, we should do it for scratchpad as well. Any attacker can simply change instructions from opening the console to opening the scratchpad.


> >  - This requires the history to cross 10 in a particular window. Instead, we
> > can make it persistent by storing the number of commands entered (until it
> > reaches 10, no need to keep counting after that) in the pref and block
> > pasting accordingly.
> 
> I think we need this. It's easy to imagine someone using the web console for
> quite a while without crossing the 10 history items in a single window.

Agreed. I believe this is important.


> (In reply to Manish Goregaokar [:manishearth] from comment #8)
> > Oh, also I'll have to get it working for dragdrop. I'll wait for some
> > feedback on the implementation before going ahead.
> 
> I'm trying to imagine an attack script that uses drag and drop, and it feels
> like it's going to be too complex to be viable. (I blogged about the data
> that I have here:
> http://incompleteness.me/blog/2014/04/24/combatting-self-xss-part-2/)
> 
> But that said, the cost of adding it to the user could be zero, so I don't
> think I'm against it.

Instead of saying 'copy/paste this code' the attacker can say 'drag and drop this code'.


> We have another issue in terms of testing. Jeff is happy with this being on
> for Release and Beta users, but we have to go through Nightly and Aurora to
> get there. Perhaps we should have a several phase landing:
> * get the history thing in place first
> * deploy the actual protection to nightly and aurora
> * when it's reached release, turn off the protection in nightly and aurora
> 
> We might also challenge the requirement that it's for release and beta only.

I like the proposed phases of landing.

Comment 12

[Mihai Sucan [:msucan]]

(In reply to Manish Goregaokar [:manishearth] from comment #7)
> Created attachment 8411484 [details] [diff] [review]
> Basic functionality
> 
> ...

Manish, thanks for the patch. I do have one concern: please avoid using modal prompts.

Comment 13

[Manish Goregaokar [:manishearth]]

Attachment: Both scratchpad and webconsole

Alright, got it working for the Scratchpad as well.

I'm unable to get the localization to work properly, somehow. I'd defined an L10N getter similar to that in css-logic.js, but it was throwing errors.

Also, the getter for WebConsoleUtils in scratchpad.js wasn't working (I guess the module was moved out of resources:// ?), so I replaced it with a require()


@msucan: this is supposed to block further usage of the web console until it is dismissed, shouldn't it be a modal dialog?

(The warning dialog for opening the browser toolbox is also modal)

Comment 14

[Joe Walker [:jwalker]]

Comment on attachment 8411806 [details] [diff] [review]
Both scratchpad and webconsole

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

This looks good.
I think you should get r+ from msucan rather than me.

::: browser/app/profile/firefox.js
@@ +1259,5 @@
>  
>  // Text size in the Web Console. Use 0 for the system default size.
>  pref("devtools.webconsole.fontSize", 0);
>  
> +// Number of usages of the web console or scratchpad. 

Nit: trailing whitespace

::: browser/devtools/scratchpad/scratchpad.js
@@ +1616,5 @@
>      this.editor.appendTo(document.querySelector("#scratchpad-editor")).then(() => {
>        var lines = initialText.split("\n");
>  
>        this.editor.on("change", this._onChanged);
> +      document.querySelector("#scratchpad-editor").addEventListener("paste", WebConsoleUtils.pasteHandler)

We probably need to remove this handler somewhere?
Also, nits: Could you wrap to 80 lines and use ';' to end a statement.

::: browser/devtools/webconsole/webconsole.js
@@ +3842,5 @@
>    {
>      if (this.autocompletePopup) {
>        this.clearCompletion();
>      }
>    },

We don't need to delete this line do we?

::: browser/locales/en-US/chrome/browser/devtools/webconsole.properties
@@ +222,5 @@
>  # LOCALIZATION NOTE (cdFunctionInvalidArgument): the text that is displayed when
>  # cd() is invoked with an invalid argument.
>  cdFunctionInvalidArgument=Cannot cd() to the given window. Invalid argument.
> +
> +selfxss.title=Pasted code detected

I suggest "Pasted code warning"
(we're not actually detecting anything).

@@ +223,5 @@
>  # cd() is invoked with an invalid argument.
>  cdFunctionInvalidArgument=Cannot cd() to the given window. Invalid argument.
> +
> +selfxss.title=Pasted code detected
> +selfxss.msg=Please be careful when pasting code into the console, untrusted code can hijack your account (etc etc)

I suggest "Scam Warning: Take care when pasting things you don't understand. This could allow attackers to steal your identity or take control of your computer".

I'm not sure this is the best we can do, but things I was trying:

* Get the word 'scam' in as soon as possible for the TL;DR crowd.
* Not talking about 'code' or anything technical which might not have meaning to a potential victim
* To as simply as possible describe the problem.

I'm not keen on the chromium solution for those reasons:
https://src.chromium.org/viewvc/blink/trunk/Source/devtools/front_end/ConsoleView.js?r1=171275&r2=171274&pathrev=171275

@@ +224,5 @@
>  cdFunctionInvalidArgument=Cannot cd() to the given window. Invalid argument.
> +
> +selfxss.title=Pasted code detected
> +selfxss.msg=Please be careful when pasting code into the console, untrusted code can hijack your account (etc etc)
> +selfxss.okbutton=I know what I'm doing
\ No newline at end of file

Chrome does a nice thing of asking the user to type a string to enable rather than just pressing a button.

Very annoying, but proves you've read it.

Perhaps we should add this to the string above:

  "Please type 'always allow' below to allow pasting".

Comment 15

[Manish Goregaokar [:manishearth]]

Attachment: With text prompt

Added a text prompt, as per jwalker's comment.

I used the code for a tab-modal dialog, but it's not working. Since the Scratchpad is a separate window, it doesn't matter much, but for the Web Console, this blocks the entire browser window.

The reason this issue occurs is that the tab modal prompt works on the topmost available Window[1], and  getTabModalPromptBox[2] is only defined for <tabbrowser>s, but the developer panel is not a <tabbrowser>

We can:
 - Make the modal dialog block the tab (instead of the webconsole), but not other tabs
 - File a separate bug to make this work for general <window> objects
 - Make it work for webconsole.xul only
 - Roll our own implementation for webconsole

I like the first option. Comments?
 [1]: http://mxr.mozilla.org/mozilla-central/source/toolkit/components/prompts/src/nsPrompter.js#300
 [2]: http://mxr.mozilla.org/mozilla-central/source/browser/base/content/tabbrowser.xml#413

Comment 16

[Manish Goregaokar [:manishearth]]

Attachment: Patch which uses tab modal

Got it to work for a tab modal.

In this case, the patch blocks the *entire* tab (not just the webconsole)

Are we fine with this?

Todo: Add l10n notes.

Comment 17

[Brian Grinstead [:bgrins]]

Attachment: modal-warnings.png

Screenshots of modal in scratchpad and webconsole.  IMO this works perfectly with scratchpad.

For the webconsole it is a bit weird since this blocks interaction with the page, not with devtools.  This means you can do fun things like paste again and opening up a second prompt.

Comment 18

[Brian Grinstead [:bgrins]]

Comment on attachment 8412079 [details] [diff] [review]
Patch which uses tab modal

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

::: browser/devtools/scratchpad/scratchpad.js
@@ +1618,5 @@
>        var lines = initialText.split("\n");
>  
>        this.editor.on("change", this._onChanged);
> +      document.querySelector("#scratchpad-editor").addEventListener("paste",
> +                                                                     this._paste);

We should also listen for text being drag/dropped into the editor here and in the web console ("drop" event should do the trick, I think).

Comment 19

[Manish Goregaokar [:manishearth]]

Attachment: xss.patch

We (robcee, jwalker, bgrins, and I) discussed the modal dialogs in chat, and came up with a solution where a slider notification is shown to the user, and the user has to type something in the console/scratchpad to be able to paste stuff into it.

This patch seems to do it. I also added L10N notes and a drop handler.

Comment 20

[Manish Goregaokar [:manishearth]]

Attachment: slider-warnings.png

Screenshot of the slider warnings. 

(Usually the sliders won't appear simultaneously in both the scratchpad and the console, I've just done this for the sake of the screenshot)

Comment 21

[Manish Goregaokar [:manishearth]]

Attachment: Extra patch for branding changes

This is the patch that handles the branding. I will qfold it before getting it checked in. (I kept the patches separate since we want to be able to test this in Nightly for now)

Comment 22

[Joe Walker [:jwalker]]

Comment on attachment 8412212 [details] [diff] [review]
xss.patch

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

I've switched around f? and r?. I'm on holiday today, and Mihai doesn't have much spare time, so I think Brian should r? with Mihai's input if he's got time.
Thanks for this Manish. Top job.

Comment 23

[Brian Grinstead [:bgrins]]

Comment on attachment 8412212 [details] [diff] [review]
xss.patch

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

You are going to have to modify two tests that I know of that are expecting paste to work:

 1) browser_webconsole_bug_642615_autocomplete.js
 2) browser/devtools/scratchpad/test/browser_scratchpad_edit_ui_updates.js

I don't think we necessarily need to test the whole 'allow pasting' workflow directly.  But, for the webconsole test you could check to make sure the initial paste fails (which it does now), then run 10 commands and try the paste again to make sure it works. For the scratchpad test you could check to make sure the initial paste fails (which it does now), set the pref directly to 11 and try the paste again to make sure it works.

::: browser/devtools/scratchpad/scratchpad.js
@@ +491,5 @@
>     *         The promise for the script evaluation result.
>     */
>    execute: function SP_execute()
>    {
> +    if (WebConsoleUtils.usageCount <= 10) {

Increase usage count unconditionally here, and move the conditional into the setter.

::: browser/devtools/webconsole/webconsole.js
@@ +3271,5 @@
>      // recent entry. The most recent entry may contain the last edited input
>      // value that was not evaluated yet.
>      this.history[this.historyIndex++] = aExecuteString;
>      this.historyPlaceHolder = this.history.length;
> +    if (WebConsoleUtils.usageCount <= 10) {

Increase usage count unconditionally here, and move the conditional into the setter.

::: browser/devtools/webconsole/webconsole.xul
@@ +81,5 @@
>  
>    <tooltip id="aHTMLTooltip" page="true"/>
>  
>    <box class="hud-outer-wrapper devtools-responsive-container theme-body" flex="1">
> +    <notificationbox id="webconsole-notificationbox" flex="1">

If you move this element to surround the 'jsterm-input-container' and remove flex="1" on it, then it will show up just above the input, which is closer to where the action is happening

::: toolkit/devtools/webconsole/utils.js
@@ +534,5 @@
> +   * Value of devtools.selfxss.count preference
> +   *
> +   * @type int
> +   */
> +  _usageCount: 0,

please make the number 10 a const in this file

@@ +552,5 @@
> +   *
> +   * @param nsIDOMElement inputField
> +   * @param nsIDOMElement notificationBox
> +   */
> +  pasteHandlerGen: function WCU_pasteHandlerGen(inputField, notificationBox){

I actually prefer the prompt as far as the UI goes - and it would simplify this paste handler quite a bit.  But I understand this has already been discussed, and I wouldn't stop it from landing because of that.

@@ +579,5 @@
> +        });
> +
> +      let typePosition = 0;
> +      function pasteKeyHandler(aEvent2) {
> +        if (aEvent2.charCode == okstring.charCodeAt(typePosition)){

If I include the single quotes when typing 'allow pasting', it doesn't work in the console.  This part seems fragile - what if instead of tracking typePosition with each keypress we just did:

inputField.value.contains(okstring)

This would allow variations on the input string and would simplify the code.  This would also let us check the field oninput instead of onkeypress

@@ +598,5 @@
> +
> +      aEvent.preventDefault();
> +      aEvent.stopPropagation();
> +      return false;
> +      }

Whitespace: these lines are not indented consistently

Comment 24

[Brian Grinstead [:bgrins]]

Attachment: slider-warning-console-above-input.png

What the slider looks like right above the input

Comment 25

[Manish Goregaokar [:manishearth]]

(In reply to Brian Grinstead [:bgrins] from comment #23)
> > +    if (WebConsoleUtils.usageCount <= 10) {
> 
> Increase usage count unconditionally here, and move the conditional into the
> setter.

The idea was to avoid the call to System.prefs every time something is entered into the console. This pref is not expected to change it reaches 11, so I let it cache itself.

The alternative method is to use an observer to monitor pref change (a bit less ineffective)

Thoughts?

I'll make the other changes in the meantime. I might take a bit of time for the tests, I'm a bit busy.

Comment 26

[Brian Grinstead [:bgrins]]

(In reply to Manish Goregaokar [:manishearth] from comment #25)
> (In reply to Brian Grinstead [:bgrins] from comment #23)
> > > +    if (WebConsoleUtils.usageCount <= 10) {
> > 
> > Increase usage count unconditionally here, and move the conditional into the
> > setter.
> 
> The idea was to avoid the call to System.prefs every time something is
> entered into the console. This pref is not expected to change it reaches 11,
> so I let it cache itself.
> 
> The alternative method is to use an observer to monitor pref change (a bit
> less ineffective)
> 
> Thoughts?

I meant that you can check for <= 10 inside of the set() call.  This way all of that logic is in one place.  So this should be equivalent logic, right?

set usageCount(newUC) {
  if (newUC <= 11) {
    WebConsoleUtils._usageCount = newUC;
    Services.prefs.setIntPref("devtools.selfxss.count", newUC);
  }
},

Comment 27

[Mihai Sucan [:msucan]]

Comment on attachment 8412212 [details] [diff] [review]
xss.patch

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

Patch looks good. Thanks for your work on this.

Please add a webconsole test for this functionality. You may also want a scratchpad test. Also fix coding style as suggested by Brian.

::: browser/devtools/webconsole/webconsole.js
@@ +3129,5 @@
>          !Services.prefs.getBoolPref("devtools.chrome.enabled")) {
>        inputContainer.style.display = "none";
>      }
>      else {
> +      this._paste = WebConsoleUtils.pasteHandlerGen(this.inputNode, doc.getElementById("webconsole-notificationbox"));

nit: this._onPaste or this._pasteHandler.

@@ +4528,5 @@
>      }
>  
> +    if (this._paste) {
> +      this.inputNode.removeEventListener("paste", this._paste, false);
> +      this.inputNode.removeEventListener("drop", this._paste, false);

nit: also add this._paste = null;

::: toolkit/devtools/webconsole/utils.js
@@ +532,5 @@
>    },
> +  /**
> +   * Value of devtools.selfxss.count preference
> +   *
> +   * @type int

nit: @type number (for consistency)
also @private

@@ +564,5 @@
> +        aEvent.preventDefault();
> +        aEvent.stopPropagation();
> +        return false;
> +      }
> +      let l10n = Services.strings.createBundle("chrome://browser/locale/devtools/webconsole.properties");

Please dont do this here. Use WebConsoleUtils.l10n, like webconsole.js uses it.

Comment 28

[Mihai Sucan [:msucan]]

Comment on attachment 8412398 [details] [diff] [review]
Extra patch for branding changes

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

I dont have experience with the branding-related changes and what is the process. Patch looks good to me but please ask for review from Joe maybe.

Comment 29

[Brian Grinstead [:bgrins]]

Comment on attachment 8412398 [details] [diff] [review]
Extra patch for branding changes

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

::: browser/app/profile/firefox.js
@@ -1259,5 @@
>  
>  // Text size in the Web Console. Use 0 for the system default size.
>  pref("devtools.webconsole.fontSize", 0);
>  
> -// Number of usages of the web console or scratchpad.

I wonder if we should do this differently - start with 11 in fireefox.js, then set to 0 on release / beta?  This way, if you've pulled down the code and built yourself then you don't have the warning.  Then again, I don't know much about this branding either, and don't see any beta firefox-branding.

Comment 30

[Manish Goregaokar [:manishearth]]

(In reply to Brian Grinstead [:bgrins] from comment #29)

> I wonder if we should do this differently - start with 11 in fireefox.js,
> then set to 0 on release / beta?  This way, if you've pulled down the code
> and built yourself then you don't have the warning.  Then again, I don't
> know much about this branding either, and don't see any beta
> firefox-branding.

As far as I can tell, the Firefox beta build is supposed to behave exactly like a release build, except with more bugs, and some string changes. So the branding is shared.

I'm not entirely certain if prefs declared in branding/ override prefs from firefox.js -- it depends on which order the file is called in. I don't see any prefs that are both in branding/ and firefox.js (haven't looked thoroughly though)

Besides, if you've pulled/built the code, it will use the Nightly branding, which has the pref at 11.

Comment 31

[Manish Goregaokar [:manishearth]]

Attachment: Fixed patch, no tests

Here's the new patch.

One issue is that while the `input` event works perfectly on the console, one needs to type an extra character to get it to work for the scratchpad. Basically, the input event is fired before the textContent updates, so you have to type `always allow<return>` or `always allow<space>` or something for this to work. Is this okay (should still be quite intuitive)? Or is there some other event type we should use?

Comment 32

[Manish Goregaokar [:manishearth]]

Attachment: Fixed patch, no tests

Moved a comment.

Comment 33

[Brian Grinstead [:bgrins]]

Comment on attachment 8412716 [details] [diff] [review]
Fixed patch, no tests

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

f+, awaiting test changes

::: browser/devtools/scratchpad/scratchpad.js
@@ +1616,4 @@
>        var lines = initialText.split("\n");
>  
>        this.editor.on("change", this._onChanged);
> +      this._onPaste = WebConsoleUtils.pasteHandlerGen(this.editor.container.contentDocument.body, 

remove trailing whitespace

::: toolkit/devtools/webconsole/utils.js
@@ +556,5 @@
> +  /**
> +   * The inputNode "paste" event handler generator. Helps prevent self-xss attacks
> +   *
> +   * @param nsIDOMElement inputField
> +   * @param nsIDOMElement notificationBox

Add a comment like `@returns A function to be added as a handler to 'paste' and 'drop' events on the input field`.

@@ +584,5 @@
> +          }
> +        });
> +
> +      function pasteInputHandler(aEvent2) {
> +        let value = inputField.value || inputField.textContent; 

remove trailing whitespace

@@ +587,5 @@
> +      function pasteInputHandler(aEvent2) {
> +        let value = inputField.value || inputField.textContent; 
> +        if (value.contains(okstring)) {
> +          notificationBox.removeNotification(notification);
> +          inputField.removeEventListener("input", pasteInputHandler);

Not sure, but will this get handler get removed during the cleanup function in appendNotification above?

Comment 34

[Manish Goregaokar [:manishearth]]

The event handler should get removed already, but I'm not sure. I'll look into it and remove the duplicate removeEventListener call if it's already gone.

I've got an exam coming up, I'll get back to this (the tests) in a couple of days.

Comment 35

[Brian Grinstead [:bgrins]]

> One issue is that while the `input` event works perfectly on the console,
> one needs to type an extra character to get it to work for the scratchpad.
> Basically, the input event is fired before the textContent updates, so you
> have to type `always allow<return>` or `always allow<space>` or something
> for this to work. Is this okay (should still be quite intuitive)? Or is
> there some other event type we should use?

OK, I've thought about the abstraction here a bit with regards to the difference between the scratchpad editor and webconsole input.  I think that the differences are big enough that instead of sharing a paste handler generator function, we should share only the notification appending functionality.  The reason is that there are too many differences between the two input modes, and I can't think of any clean way to resolve them without breaking the functionality up like this.  There may be a way to do this that I'm not thinking of, but I don't like making the user enter an extra character in the scratchpad (and having extra instructions for this).

The code below isn't necessarily working code, just where I'm thinking that it makes sense for the abstractions to live.  I really don't like how much code this takes to accomplish, but I think this is the tradeoff with not using a prompt() - in which case the pasteHandlerGen function would be fine.

## In webconsole.js / scratchpad.js

Sometime during initialization:

this.notificationBox = doc.getElementById("webconsole-notificationbox" /* or scratchpad-notificationbox */);
this._onSelfXSSPaste = this._onSelfXSSPaste.bind(this);
inputField.addEventListener("paste", this._onSelfXSSPaste);
inputField.addEventListener("drop", this._onSelfXSSPaste);


_onSelfXSSPaste: function(aEvent) {
  // For scratchpad this would swap the inputField out with the editor
  let inputField = this.inputNode;
  let notificationBox = this.notificationBox;

  // Pastes are allowed - unbind this event and return.
  if (WebConsoleUtils.isSelfXSSAllowed()) {
    inputField.removeEventListener("paste", this._onSelfXSSPaste);
    inputField.removeEventListener("drop", this._onSelfXSSPaste);
    return true;
  }

  // Paste happened when notification was already opened.  Prevent it.
  if (notificationBox.getNotificationWithValue("selfxss-notification")) {
    aEvent.preventDefault();
    aEvent.stopPropagation();
    return false;
  }

  // Show a notification and wait until user has confirmed before allowing paste.
  // In case of scratchpad, this would be listening to the editor change event.
  inputField.addEventListener("input", onSelfXSSInput);
  let notification = WebConsoleUtils.appendSelfXSSNotification(this.notificationBox, function() {
    inputField.removeEventListener("input", onSelfXSSInput);
  });

  function onSelfXSSInput() {
    // User has confirmed, allow future paste events to go through
    // In case of scratchpad, this would be using editor.getValue() instead of inputField.value
    if (inputField.value.contains(WebConsoleUtils.okstring)) {
      WebConsoleUtils.allowSelfXSS();
      notificationBox.removeNotification(notification);
    }
  }
}

## In utils.js

isSelfXSSAllowed: function() {
  return this.usageCount >= CONSOLE_ENTRY_THRESHOLD
},
allowSelfXSS: function() {
  this.usageCount = CONSOLE_ENTRY_THRESHOLD
},
appendSelfXSSNotification: function(notificationBox, onremoved) {
  let l10n = new WebConsoleUtils.l10n("chrome://browser/locale/devtools/webconsole.properties");
  let okstring = l10n.getStr("selfxss.okstring");
  let msg = l10n.getFormatStr("selfxss.msg", [okstring]);
  let notification = notificationBox.appendNotification(msg,
    "selfxss-notification",
    null,
    notificationBox.PRIORITY_WARNING_HIGH,
    null,
    function(eventType) {
      if (eventType == "removed") {
        onremoved();
      }
    }
  );

  return notification;
}

Comment 36

[Brian Grinstead [:bgrins]]

Comment on attachment 8412716 [details] [diff] [review]
Fixed patch, no tests

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

::: toolkit/devtools/webconsole/utils.js
@@ +589,5 @@
> +        if (value.contains(okstring)) {
> +          notificationBox.removeNotification(notification);
> +          inputField.removeEventListener("input", pasteInputHandler);
> +          WebConsoleUtils.usageCount = CONSOLE_ENTRY_THRESHOLD;
> +          inputField.removeEventListener("paste", handler);

The paste/drop event listeners shouldn't need to be removed here - they will be removed in the first condition of the handler the next time a paste happens.

Comment 37

[Manish Goregaokar [:manishearth]]

As discussed in IRC, on solution to the oninput problem is to use keyup instead.

As for the paste listeners, I didn't like having them hang around for longer, but since this is a one time thing it doesn't matter much. I'll remove 'em.

Comment 38

[Manish Goregaokar [:manishearth]]

> Not sure, but will this get handler get removed during the cleanup function in appendNotification above?

They won't. That cleanup function just removes the handlers when the notification is dismissed -- these are supposed to be around only when there's a notification box, otherwise there will be dupes.

I'm looking in to the tests now.

Comment 39

[Manish Goregaokar [:manishearth]]

What are we going to do about the branding changes? If we put this into the tree with the branding changes, the feature won't get tested at all till it reaches beta. Should we keep a month's delay between checking in the main patch and the branding changes to allow for feedback from the Nightly users?

Alternatively, we can uplift it to beta (since the patch is impotent in Nightly/Aurora). This also gets the feature out sooner -- many people seem to want this.

Thoughts?

Comment 40

[Joe Walker [:jwalker]]

(In reply to Manish Goregaokar [:manishearth] from comment #39)
> What are we going to do about the branding changes? If we put this into the
> tree with the branding changes, the feature won't get tested at all till it
> reaches beta. Should we keep a month's delay between checking in the main
> patch and the branding changes to allow for feedback from the Nightly users?
> 
> Alternatively, we can uplift it to beta (since the patch is impotent in
> Nightly/Aurora). This also gets the feature out sooner -- many people seem
> to want this.
> 
> Thoughts?

Jeff - you were in favor of not having this turned on in Aurora/Nightly. I think this needs testing before Beta, and I suspect that "Has history of using the JS console" is a better indication of an understanding of the risks than "Has installed Aurora".

So personally, I'd remove the "not on Aurora/Nightly" thing altogether.

Comment 41

[Joe Walker [:jwalker]]

There is indication that the chrome solution is being backed out because of l10n. We don't have that problem.

There is good and bad in doing the same as Chrome. Specifically we should not have the same string to type (if we both end up with that solution), because this will make cross-browsers scripts a more likely possibility again.

If the Chrome team does something radically different, then perhaps we should think about the reasoning.

Comment 42

[Manish Goregaokar [:manishearth]]

(In reply to Joe Walker [:jwalker] from comment #41)
> There is indication that the chrome solution is being backed out because of
> l10n. We don't have that problem.

They're not backing it out, apparently. They're leaving it as-is and might improve it later. Apparently l10n is not done for the devtools, and the concept of Developer Mode has been vetoed before (I *think* the same has been done for Firefox)

Yeah, we should be different from them at least as far as the string goes. If they do something different, that's even better, though this solution seems pretty balanced as far as annoyance-to-devs vs effectiveness-in-preventing-selfxss goes.

Comment 43

[Manish Goregaokar [:manishearth]]

Attachment: Part 2 (test changes)

Here's the patch for the tests, to be applied on top of the other one (I'll qfold it later)

Perhaps we should rename the browser_webconsole_bug_642615_autocomplete test to something more generic?

Comment 44

[Manish Goregaokar [:manishearth]]

Attachment: Part 1 (main patch)

Nits fixed

Comment 46

[Jeff Griffiths (:canuckistani) (:⚡︎)]

(In reply to Joe Walker [:jwalker] from comment #40)
> (In reply to Manish Goregaokar [:manishearth] from comment #39)
> > What are we going to do about the branding changes? If we put this into the
> > tree with the branding changes, the feature won't get tested at all till it
> > reaches beta. Should we keep a month's delay between checking in the main
> > patch and the branding changes to allow for feedback from the Nightly users?
> > 
> > Alternatively, we can uplift it to beta (since the patch is impotent in
> > Nightly/Aurora). This also gets the feature out sooner -- many people seem
> > to want this.
> > 
> > Thoughts?
> 
> Jeff - you were in favor of not having this turned on in Aurora/Nightly. I
> think this needs testing before Beta, and I suspect that "Has history of
> using the JS console" is a better indication of an understanding of the
> risks than "Has installed Aurora".
> 
> So personally, I'd remove the "not on Aurora/Nightly" thing altogether.

I would *eventually* like to turn this off for Aurora / Nightly, agree that testing in the near term is more important. Once this feature is vetted and shipped I don't see why it would need to be turned on for relatively small user pools such as Aurora  / Nightly, and if we market Aurora aggressively to developers, this will just cause friction.

Comment 47

[Brian Grinstead [:bgrins]]

(In reply to Manish Goregaokar [:manishearth] from comment #43)
> Created attachment 8416902 [details] [diff] [review]
> Part 2 (test changes)
> 
> Here's the patch for the tests, to be applied on top of the other one (I'll
> qfold it later)
> 
> Perhaps we should rename the browser_webconsole_bug_642615_autocomplete test
> to something more generic?

I've pushed the two latest patches to try, and am getting test failures on browser_webconsole_bug_642615_autocomplete.js and browser_scratchpad_edit_ui_updates.js: https://tbpl.mozilla.org/?tree=Try&rev=8893653ff90e

Comment 48

[Manish Goregaokar [:manishearth]]

Pushed to try, with some tweaks and assertions.

https://tbpl.mozilla.org/?tree=Try&rev=bfdbf6cfac15

Comment 49

[Manish Goregaokar [:manishearth]]

Attachment: Part 1 (main patch)

Fixed. When multiple tests are being run the pref has a chance of hitting the threshold, after which the value is cached. The usageCount setter was not designed for resetting the value. I tweaked it so that when doing usageCount++ on a usageCount=10 it will still be a no-op, but when doing usageCount=0, it will work.

Tests pass: https://tbpl.mozilla.org/?tree=Try&rev=34e45426eff5

Comment 50

[Manish Goregaokar [:manishearth]]

Attachment: Part 2 (test changes)

Comment 51

[Brian Grinstead [:bgrins]]

Comment on attachment 8418437 [details] [diff] [review]
Part 2 (test changes)

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

The webconsole changes look good, the scratchpad changes could use a bit of reorganizing I think (see note below)

::: browser/devtools/scratchpad/test/browser_scratchpad_edit_ui_updates.js
@@ +137,5 @@
>      closeMenu(hideAfterCut);
>    };
>  
>    let hideAfterCut = function() {
> +    waitForFocus(function () {

This is confusing - why is it doing different things based on 'pass', and why is selfXSS stuff happening in a function called hideAfterCut?

Could you maybe move all of the selfXSS stuff to the beginning of this test to get it out of the way so it doesn't need to interfere with the rest of the test?

Comment 52

[Brian Grinstead [:bgrins]]

Comment on attachment 8418437 [details] [diff] [review]
Part 2 (test changes)

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

::: browser/devtools/webconsole/test/browser_webconsole_bug_642615_autocomplete.js
@@ +49,5 @@
> +    ok(notification,  "Self-xss notification shown");
> +    is(oldVal, jsterm.inputNode.value, "Paste blocked by self-xss prevention");
> +    notificationbox.removeNotification(notification);
> +    // Input some commands to disable the self-xss prevention
> +    for(let i = 0; i <= 10; i++){

One more thing: I like that it is testing running 10 commands to make sure this allows pasting, but we should also test entering the 'allow paste' string into the console to make sure this has the same effect.

Comment 53

[Manish Goregaokar [:manishearth]]

Attachment: Part 2 (test changes)

I improved the Scratchpad test.

The web console test has two issues: Firstly, I'm not able to spoof the events correctly, dispatchEvent with both keypress and keyup are not updating the input. I can, however, set the input first

Secondly, the enter-10-times test sets the pref to 10, and setting it back won't work since the paste handler has since been removed. What we can do here is to just test that the usage count increases on execution, but not hit the threshhold.  Then we do the selfxss tests with the allow pasting bit. I'll upload an alternate patch which accomplishes this.

Comment 54

[Manish Goregaokar [:manishearth]]

Attachment: Part 2 (test changes) [With modified webconsole tests]

This is the alternate patch with improvements to the webconsole tests. Let me know which one we should use.

Comment 55

[Brian Grinstead [:bgrins]]

Comment on attachment 8420488 [details] [diff] [review]
Part 2 (test changes) [With modified webconsole tests]

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

I like this version more - it separates the xss testing and covers both ways of bypassing the self xss protection

::: browser/devtools/webconsole/test/browser_webconsole_bug_642615_autocomplete.js
@@ +71,5 @@
> +    jsterm.inputNode.value = "allow pasting";
> +    var evt = document.createEvent("KeyboardEvent");
> +    evt.initKeyEvent ("keyup", true, true, window,
> +                      0, 0, 0, 0,
> +                      0, " ".charCodeAt(0)) 

remove whitespace and add semicolon at end of line

@@ +76,5 @@
> +    jsterm.inputNode.dispatchEvent(evt);
> +    jsterm.inputNode.value = "";
> +    oldVal = "";
> +    goDoCommand("cmd_paste");
> +    isnot(oldVal, jsterm.inputNode.value, "Paste works");

I wouldn't bother setting oldVal here, just check "" in the assertion

Comment 56

[Manish Goregaokar [:manishearth]]

Attachment: Part 2 (test changes) [With modified webconsole tests]

Nits fixed.

Comment 57

[Brian Grinstead [:bgrins]]

Comment on attachment 8412398 [details] [diff] [review]
Extra patch for branding changes

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

So according to Comment 40 and Comment 46, should we hold off on making any branding changes in this bug?

Comment 58

[Manish Goregaokar [:manishearth]]

I think so (and so do the other comments, afaict) 

We can file a followup bug to tack on the branding changes, and discuss the need for it separately.

Comment 59

[Brian Grinstead [:bgrins]]

Comment on attachment 8418436 [details] [diff] [review]
Part 1 (main patch)

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

Asking jwalker to review

::: toolkit/devtools/webconsole/utils.js
@@ +539,5 @@
> +   * @private
> +   */
> +  _usageCount: 0,
> +  get usageCount() {
> +    // Cache the value, for efficiency

This comment isn't very helpful - you could probably remove it

@@ +544,5 @@
> +    if (WebConsoleUtils._usageCount < CONSOLE_ENTRY_THRESHOLD) {
> +      WebConsoleUtils._usageCount = Services.prefs.getIntPref("devtools.selfxss.count")
> +      return WebConsoleUtils._usageCount;
> +    }
> +    return CONSOLE_ENTRY_THRESHOLD;

It may be easier to read if you remove the early return and just return  WebConsoleUtils._usageCount at the bottom here (pretty sure that is equiv. logic)

Comment 60

[Manish Goregaokar [:manishearth]]

Attachment: Part 1 (main patch)

Nits fixed.

Should I qfold the two patches now? We probably shouldn't commit these separately as the tests break in between, which can get at someone running a bisect.

Comment 61

[Brian Grinstead [:bgrins]]

(In reply to Manish Goregaokar [:manishearth] from comment #60)
> Created attachment 8421220 [details] [diff] [review]
> Part 1 (main patch)
> 
> Nits fixed.
> 
> Should I qfold the two patches now? We probably shouldn't commit these
> separately as the tests break in between, which can get at someone running a
> bisect.

Yeah, I think they can be folded at this point.

Comment 62

[Manish Goregaokar [:manishearth]]

Attachment: Folded patch

Folded.

Comment 63

[Manish Goregaokar [:manishearth]]

Try push, in case: https://tbpl.mozilla.org/?tree=Try&rev=eaa8089cb16a

Comment 64

[Joe Walker [:jwalker]]

Comment on attachment 8418436 [details] [diff] [review]
Part 1 (main patch)

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

Looks good so far. A question about lazy-loading, and a minor nit. Next I'm going to have a play. Sorry for the delay.

::: browser/devtools/scratchpad/scratchpad.js
@@ +87,5 @@
>  // to do so here.
>  let telemetry = new Telemetry();
>  telemetry.toolOpened("scratchpad");
>  
> +let WebConsoleUtils = require("devtools/toolkit/webconsole/utils").Utils;

Did we need to un-lazily load this?

@@ +1691,5 @@
>  
>      PreferenceObserver.uninit();
>      CloseObserver.uninit();
> +    if (this._onPaste) {
> +      let ed = document.querySelector("#scratchpad-editor");

If we're making changes, then maybe you could rename ed to editorElement for symmetry with the code above? Probably not worth creating a new patch for.

Comment 65

[Joe Walker [:jwalker]]

Comment on attachment 8418436 [details] [diff] [review]
Part 1 (main patch)

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

Sorry I had the page open for a while and ended up resetting the flag.

Comment 66

[Joe Walker [:jwalker]]

Comment on attachment 8418436 [details] [diff] [review]
Part 1 (main patch)

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

::: browser/locales/en-US/chrome/browser/devtools/webconsole.properties
@@ +225,5 @@
>  
> +# LOCALIZATION NOTE (selfxss.msg): the text that is displayed when
> +# a new user of the developer tools pastes code into the console
> +# %1 is the text of selfxss.okstring
> +selfxss.msg=Scam Warning: Take care when pasting things you don't understand. This could allow attackers to steal your identity or take control of your computer. Please type '%S' below to allow pasting.

Please could you change the last sentence to read
  "Please type '%S' (no need to press enter) below to allow pasting."

I think this is important because otherwise someone might type 'allow pasting' and then press return quickly (without noticing, the notification going away) get an error, and assume that it didn't work.

Comment 67

[Manish Goregaokar [:manishearth]]

Attachment: Final patch +nits

Nits fixed.

I wasn't able to get the lazy loading to work. Using `XPCOMUtils.defineLazyModuleGetter(this, 
                     "WebConsoleUtils", 
"resource://gre/modules/devtools/toolkit/webconsole/utils.js", "Utils");` instead of the `require()` gives me a `missing ; before statement` error on scratchpad.js:1637 (this is just after the first instance of WebConsoleUtils). I'm not sure how to debug this -- this error usually comes out of a missing semicolon after a `= function(){}` type statement, but I can't find any missing semicolons in utils.js or scratchpad.js.

Comment 68

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8425007 [details] [diff] [review]
Final patch +nits

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

::: browser/devtools/scratchpad/scratchpad.js
@@ +87,5 @@
>  // to do so here.
>  let telemetry = new Telemetry();
>  telemetry.toolOpened("scratchpad");
>  
> +let WebConsoleUtils = require("devtools/toolkit/webconsole/utils").Utils;

You could use lazyRequireGetter instead of require:

http://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/Loader.jsm#290

Comment 69

[Manish Goregaokar [:manishearth]]

I tried that, it seems broken (`TypeError: this is undefined    Loader.jsm:292`). Perhaps there should be a .bind() for Loader.jsm in the constructor above?

Comment 70

[Manish Goregaokar [:manishearth]]

(In reply to Panos Astithas [:past] from comment #68)
> You could use lazyRequireGetter instead of require:
> 
> http://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/Loader.jsm#290

lazyRequireGetter is currently broken (bug 1013905). We can use it as a part of bug 1004485 once it lands.

Comment 71

[Manish Goregaokar [:manishearth]]

Attachment: Final patch

Comment 72

[Manish Goregaokar [:manishearth]]

The above patch with minor differences has already been pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=eaa8089cb16a, and relevant tests pass locally -- marking as checkin-needed.

Comment 73

[Ryan VanderMeulen [:RyanVM]]

This is bitrotted :(. Please rebase.

Comment 74

[Manish Goregaokar [:manishearth]]

Attachment: Rebased patch

r=jwalker

Tests pass locally, I also have a try push here (in case): https://tbpl.mozilla.org/?tree=Try&rev=7a99ad0fc621

Comment 75

[Carsten Book [:Tomcat]]

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

Comment 76

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

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

Comment 77

[Manish Goregaokar [:manishearth]]

Bug for restricting this to Release/beta: bug 1015314

Comment 78

[Manish Goregaokar [:manishearth]]

Some feedback I got from John Oliver:

- For the scratch pad it's impossible to tell *where* you should type 'allow pasting'.
- The warning text is completely monotone, and thus makes me not read it at all. You should highlight "Scam Warning" (at the very least by making it bold).

Thoughts?

Comment 79

[Joe Walker [:jwalker]]

(In reply to Manish Goregaokar [:manishearth] from comment #78)
> Some feedback I got from John Oliver:
> 
> - For the scratch pad it's impossible to tell *where* you should type 'allow
> pasting'.

Makes sense. I guess it would be easy to add "anywhere in this Scratchpad" to the message?

> - The warning text is completely monotone, and thus makes me not read it at
> all. You should highlight "Scam Warning" (at the very least by making it
> bold).

It is monotone, but it happens just when you hit the paste key, so I hope that makes it more active, and more likely to be read for most people. I'm not against highlighting parts of the warning in bold though.

Comment 80

[Joe Walker [:jwalker]]

Raised bug 1028903.

Comment 81

[tom.liao]

Can we please add an option to turn this off, I do alot of automated web testing via Selenium webdrive, and since webdriver creates a new profile every single time, I have to type in 'allow pasting' every single time just to try out some javascript code.

This is extremely annoying

Comment 82

[Girish Sharma [:Optimizer]]

Since you are automating things, you can get rid of this feature by setting the setting devtools.selfxss.count to a high number like 100.

Comment 83

[tom.liao]

(In reply to Girish Sharma [:Optimizer] from comment #82)
> Since you are automating things, you can get rid of this feature by setting
> the setting devtools.selfxss.count to a high number like 100.

Thank you very much, this worked

Comment 84

[Manish Goregaokar [:manishearth]]

These patches landed in release, btw (I see it in 33.0, probably is there in some earlier versions too). It's silent enough (for devs) that I didn't notice it at all on my own Firefox.

(I'm surprised that there was no activity indicating its progress through aurora/beta/release, other bugs I've worked on have had special flags set as this happens)

I wonder if we can get some stats from Facebook and other websites who had a self-xss problem on a reduction (if any) in successful self-xss-es across Firefox versions (since these attacks generate support tickets, there may be a way of tracking them). Hopefully Joe's predictions[1] on completion vs complexity are correct! 


 [1]: http://incompleteness.me/blog/2014/04/24/combatting-self-xss-part-2/

Comment 85

[Manish Goregaokar [:manishearth]]

Seems to have been around since 32.0 (September)

http://goo.gl/tZT8ML

I haven't seen any angry dev posts on this after two months, so it seems like the dev detection heuristic is working :)

Comment 86

[Joe Walker [:jwalker]]

Manish wrote:
> I wonder if we can get some stats from Facebook and other websites who had a self-xss problem
> on a reduction (if any) in successful self-xss-es across Firefox versions (since these attacks
> generate support tickets, there may be a way of tracking them)

Jessie - do you have any contacts at Facebook that could help?

Comment 87

[Brad Hill]

We are happy to see this, but the nature of self-xss is that it's difficult to detect accurately in the general case and very bursty (and we quickly blacklist it) in specific cases.  The end result is that even at Facebook we don't have statistically significant data distinguishing self-xss rates in Firefox before and after this change to be able to offer much insight on its effectiveness.

Comment 88

[sworddragon2]

(In reply to Girish Sharma [:Optimizer] from comment #82)
> Since you are automating things, you can get rid of this feature by setting
> the setting devtools.selfxss.count to a high number like 100.

What does this option exactly do and does setting it to 100 completely disable this feature or making the message just less likely to appear?

Comment 89

[Manish Goregaokar [:manishearth]]

> What does this option exactly do and does setting it to 100 completely
> disable this feature or making the message just less likely to appear?


The selfxss prevention works by counting the number of times you have used the console, and turning off paste protection when you cross a certain number, which is at the moment 5 (the number also stops incrementing after that). Alternatively, when you type "allow pasting" into the console when the prompt comes up, this pref is set to a number >5.

Setting it to any number more than 5 should fix this. 100 is just a high enough number that it will be resilient to all change.

Comment 90

[sworddragon2]

Wouldn't setting it to 0 also allow pasting then? If yes this would maybe be a better value to disable this feature as it would be more reliable for potential future changes.

Comment 91

[Manish Goregaokar [:manishearth]]

0 is the default value, which means that no action has been taken by the user to enable pasting

Comment 92

[sworddragon2]

Ah, now I see it, thanks for the explanation.

Comment 93

[Daniel]

This feature is INCREDIBLY annoying. I use Firefox for running Selenium test cases, and each time Selenium starts Firefox it of course does so with a new, blank user profile. When tests fail, I need to copy/paste CSS selectors to debug the error in the console, figuring out what went wrong. EVERY F***ING TIME it fails and I need to type in "allow pasting" in the console. This amounts to upwards of a hundred times a day. Because it's a new profile each time, there's nothing I can do to prevent it from showing up.

PLEASE PLEASE for the love of god, give us an option to disable this "feature" for good. I'm going INSANE!!!

Comment 94

[Manish Goregaokar [:manishearth]]

> PLEASE PLEASE for the love of god, give us an option to disable this "feature" for good. I'm going INSANE!!!

There is one. Set devtools.selfxss.count to something like 20

Comment 95

[Daniel]

As stated, Firefox starts each time with a new, blank user profile, so this is reset on each run.

Comment 96

[Manish Goregaokar [:manishearth]]

Sure, but you can set that flag each time in the test (you asked for an option).

You can also provide a template profile for selenium iirc.

Comment 97

[Daniel]

By golly, I had no idea you could provide a template — that's awesome! Thanks for the tip :)

Comment 98

[roselan]

the moron that came up with this idea shall be burned at the stake

Comment 99

[Brian Grinstead [:bgrins]]

roselan - please read https://bugzilla.mozilla.org/page.cgi?id=etiquette.html before commenting on any more bugs