1170911 - Add pref which allows user to disable document.execCommand("cut"/"copy")

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Description

[Xidorn Quan [:xidorn] UTC+11]

Letting websites use those commands could silently destroy the existing data in clipboard. It could make some users feel uncomfortable.

Although currently content can use Flash for the these operations, Flash itself can be blocked in Firefox. However, with bug 1012662, users have no way to block the content from silently destroying their clipboard data anymore.

Hence we should provide a method for users to protect their clipboard data. Though, it doesn't seem that websites tend to abuse this function. So at present, simply adding a pref should be enough.

Comment 1

[Xidorn Quan [:xidorn] UTC+11]

Attachment: patch

Comment 2

[(no longer active)]

Comment on attachment 8614506 [details] [diff] [review]
patch

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

You would need to add tests that examine the behavior of execCommand/queryCommandEnabled/queryCommandSupported with this pref set to false as well.

::: modules/libpref/init/all.js
@@ +4306,5 @@
>  pref("full-screen-api.allow-trusted-requests-only", true);
>  pref("full-screen-api.pointer-lock.enabled", true);
>  
> +// DOM execCommand("cut"/"copy")
> +pref("dom.allow_cut_copy", true);

This needs to be a hidden pref.  I have no interest in people toggling this by accident and potentially breaking their browser.  Please remove it from all.js.

Comment 3

[Xidorn Quan [:xidorn] UTC+11]

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #2)
> Comment on attachment 8614506 [details] [diff] [review]
> patch
> 
> Review of attachment 8614506 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> You would need to add tests that examine the behavior of
> execCommand/queryCommandEnabled/queryCommandSupported with this pref set to
> false as well.

Not quite sure what should be repoted for queryCommandSupported. Should we return false if they are disabled?

> ::: modules/libpref/init/all.js
> @@ +4306,5 @@
> >  pref("full-screen-api.allow-trusted-requests-only", true);
> >  pref("full-screen-api.pointer-lock.enabled", true);
> >  
> > +// DOM execCommand("cut"/"copy")
> > +pref("dom.allow_cut_copy", true);
> 
> This needs to be a hidden pref.  I have no interest in people toggling this
> by accident and potentially breaking their browser.  Please remove it from
> all.js.

Hmm. It seems to me there have already been a bunch of prefs which can break the browser by accident, if user opens about:config. e.g. full-screen-api.enabled, browser.history.allow*State, and so on.

I feel it is probably better to make it not hidden, so that user can easily find it. It would also allow us to ask people to turn off it as soon as possible, once we find some websites actually start abusing this function.

But I'm fine with either way.

Comment 4

[Xidorn Quan [:xidorn] UTC+11]

Attachment: patch

Comment 5

[(no longer active)]

(In reply to Xidorn Quan [:xidorn] (UTC+12) from comment #3)
> (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from
> comment #2)
> > Comment on attachment 8614506 [details] [diff] [review]
> > patch
> > 
> > Review of attachment 8614506 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > You would need to add tests that examine the behavior of
> > execCommand/queryCommandEnabled/queryCommandSupported with this pref set to
> > false as well.
> 
> Not quite sure what should be repoted for queryCommandSupported. Should we
> return false if they are disabled?

It should return false, I think, since setting that pref effectively removes support for this command, as far as the Web content is concerned.  You may want to add an IsCutCopySupported() function that checks that condition.

Your current test in the patch doesn't exercise this function.

Also, I think that queryCommandEnabled() must return false for these commands always, which your patch does.

> > ::: modules/libpref/init/all.js
> > @@ +4306,5 @@
> > >  pref("full-screen-api.allow-trusted-requests-only", true);
> > >  pref("full-screen-api.pointer-lock.enabled", true);
> > >  
> > > +// DOM execCommand("cut"/"copy")
> > > +pref("dom.allow_cut_copy", true);
> > 
> > This needs to be a hidden pref.  I have no interest in people toggling this
> > by accident and potentially breaking their browser.  Please remove it from
> > all.js.
> 
> Hmm. It seems to me there have already been a bunch of prefs which can break
> the browser by accident, if user opens about:config. e.g.
> full-screen-api.enabled, browser.history.allow*State, and so on.

Sure, but it doesn't mean that it's OK to add more knobs that can break the browser.

> I feel it is probably better to make it not hidden, so that user can easily
> find it.

That is the point.  I do not want to make it easy for people to find this.  I am only agreeing to add this pref to stop this debate (and also because that I think your argument in that it's possible to disable Flash makes sense), but because I have little faith in Web pages properly using queryCommandSupported/Enabled to feature detect this, I am worried that toggling this pref will make the various copy buttons on websites mysteriously stopping to work.

Therefore, this really needs to be a hidden pref.

> It would also allow us to ask people to turn off it as soon as
> possible, once we find some websites actually start abusing this function.

If that ever happens, we probably want to take a stronger action than just asking users to toggle this pref.  Besides, toggling a hidden pref is almost as easy as toggling a non-hidden one, it's the toggling by accident without knowing what you're doing which is more difficult.  :-)

Comment 6

[(no longer active)]

Comment on attachment 8615044 [details] [diff] [review]
patch

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

Minusing mostly because of the queryCommandSupported issue  (sorry, I should have been more clear on what I was asking there before!)

Also, Splinter warns me that this patch is adding Windows line endings, do you mind fixing that please?  Thanks!

::: dom/tests/mochitest/general/test_bug1170911.html
@@ +24,5 @@
> +const TEXTAREA = document.querySelector('textarea');
> +const TEXTAREA_VALUE = TEXTAREA.value;
> +
> +function doTest() {
> +  SpecialPowers.setBoolPref("dom.allow_cut_copy", false);

Nit: please use pushPrefEnv().  That will take care of resetting the pref at the end for you.

@@ +25,5 @@
> +const TEXTAREA_VALUE = TEXTAREA.value;
> +
> +function doTest() {
> +  SpecialPowers.setBoolPref("dom.allow_cut_copy", false);
> +  SpecialPowers.clipboardCopyString(CLIPBOARD_DATA, document);

The document argument has been removed.

@@ +27,5 @@
> +function doTest() {
> +  SpecialPowers.setBoolPref("dom.allow_cut_copy", false);
> +  SpecialPowers.clipboardCopyString(CLIPBOARD_DATA, document);
> +  is(SpecialPowers.getClipboardData("text/unicode"), CLIPBOARD_DATA,
> +     "Current clipboard should have predefined data stored");

Not sure if you want to keep these two statements once you switch to waitForClipboard, since that function does this for you under the hoods.

@@ +65,5 @@
> +  is(TEXTAREA.value, TEXTAREA_VALUE,
> +     "Content in the textarea shouldn't be changed");
> +
> +  is(SpecialPowers.getClipboardData("text/unicode"), CLIPBOARD_DATA,
> +     "Clipboard data shouldn't be changed");

On some platforms, the clipboard may be asynchronous, so this actually doesn't test what you want.  You need to use waitForClipboard.  You can pass true for aExpectFailure <https://dxr.mozilla.org/mozilla-central/source/testing/mochitest/tests/SimpleTest/SimpleTest.js#902> which should help you test that the contents of the clipboard really doesn't change.

Comment 7

[Xidorn Quan [:xidorn] UTC+11]

Attachment: patch

Comment 8

[(no longer active)]

Comment on attachment 8615692 [details] [diff] [review]
patch

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

Note: the patch still contains Windows line endings.  Please fix that before landing.  Thanks!

Comment 9

[Xidorn Quan [:xidorn] UTC+11]

Could you tell me where are the Windows line endings you found? I don't see any...

Comment 10

[Xidorn Quan [:xidorn] UTC+11]

Oh, I see the notification in splinter. Probably there is still some problem with my export extension, but the commits in my repo shouldn't have any problem. I'll check later, anyway.

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2f9f8ea4b9c3

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2f9f8ea4b9c3