816998 - Clipboard buffer contains copied link from a Private Tab outside of Private Browsing

Status: RESOLVED WONTFIX

(Firefox for Android Graveyard :: General, defect)

Description

[Aaron Train [:aaronmt]]

Currently on desktop if one copies the URL and tries to paste it as a new link outside of Private Browsing in a new window, it is not permitted. Currently one can use the context menu-item, 'Copy Link' from a Private Tab, and paste as a new URL in a regular tab. Should we deny that capability like desktop?

Comment 1

[Mark Finkle (:mfinkle) (use needinfo?)]

I think it should be allowed. The user is explicitly doing the action (copying). It's not something we are doing behind the scenes. I feel it's explicit, like bookmarking, so we should allow it.

Comment 2

[Brian Nicholson (:bnicholson)]

For reference, the desktop bug and discussion is bug 462106.

Comment 3

[(no longer active)]

No, we should disallow it, probably similarly to what I did in bug 462106.

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Ehsan Akhgari [:ehsan] from comment #3)
> No, we should disallow it, probably similarly to what I did in bug 462106.

I disagree. As long as one PB tab is open, we would allow copying data to the clipboard. Then when the last PB tab is closed we would clear the clipboard? Sounds almost silly to me.

Comment 5

[(no longer active)]

(In reply to comment #4)
> (In reply to Ehsan Akhgari [:ehsan] from comment #3)
> > No, we should disallow it, probably similarly to what I did in bug 462106.
> 
> I disagree. As long as one PB tab is open, we would allow copying data to the
> clipboard. Then when the last PB tab is closed we would clear the clipboard?
> Sounds almost silly to me.

Because it is not clear to the user that they're leaving the copied data around in memory, and its fairly easy for someone to see what was on the clipboard after the user is done using their private tab.  Note that we do not attempt to protect the user from this while they have a private tab open, since, obviously, the private tabs themselves give out information about the user's browsing.

Comment 6

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

I think I'm with Mark on this one. I think it is an explicit action like making a bookmark.

What is the concern?

Comment 7

[(no longer active)]

(In reply to comment #6)
> I think I'm with Mark on this one. I think it is an explicit action like making
> a bookmark.

It is not.  When you make a bookmark, you know what that means.  Most users have no idea how clipboards work, and cannot be expected to know that the data that is invisible to them for all intents and purposes can later on be retrieved using the software which ships on all Android phones supporting copy and paste.

Comment 8

[Mark Finkle (:mfinkle) (use needinfo?)]

Ehsan - I guess my biggest issue is why it's OK to copy data while a private tab is open, but not OK after we close the last private tab.

Comment 9

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Mark Finkle (:mfinkle) from comment #8)
> Ehsan - I guess my biggest issue is why it's OK to copy data while a private
> tab is open, but not OK after we close the last private tab.

Keeping in mind that Fennec uses per-tab private browsing. We are not closing an entire window. There is always a mix of private and non-private tabs at play.

Comment 10

[(no longer active)]

(In reply to comment #9)
> (In reply to Mark Finkle (:mfinkle) from comment #8)
> > Ehsan - I guess my biggest issue is why it's OK to copy data while a private
> > tab is open, but not OK after we close the last private tab.
> 
> Keeping in mind that Fennec uses per-tab private browsing. We are not closing
> an entire window. There is always a mix of private and non-private tabs at
> play.

So we have a loose concept of a private session which is defined from the time the first private tab is opened to the time when the last private tab is closed.  We clear things like the clipboard (and a whole bunch of other things) when the last private tab is closed, by listening for the last-pb-context-exited notification.

The reason why it's OK to paste the data that you've copied in a private tab is that if you hand off your phone to somebody else while you have a private tab open, your open private tab already tells them about what you're doing in that tab.  But once you close all private tabs, you're implicitly trusting your browser to not remember anything from those tabs any more.

Note that I don't think that the tab/window distinction between mobile/desktop is really relevant to this bug, since we don't have a concept of windows on mobile at all.

Comment 11

[Mark Finkle (:mfinkle) (use needinfo?)]

OK. It's the "loose concept of a private session" that is causing my resistance. IMO, it's too loose to care, but I don't think this bug will fix a massive, complicated fix, so I will not push back on it.

I'm OK with adding the listener. Just know that there are ways Android can kill Firefox that will not allow us to remove clipboard data. If we get the gecko notification, we can clear it though.

Comment 12

[Aaron Train [:aaronmt]]

Noting in here that Chrome on Android permits clipboard use through their incognito to regular tabs and vice versa.

Comment 13

[(no longer active)]

(In reply to comment #12)
> Noting in here that Chrome on Android permits clipboard use through their
> incognito to regular tabs and vice versa.

Im a bit confused here.  I'm not proposing to disallow clipboard use.

Comment 14

[Brian Nicholson (:bnicholson)]

Attachment: Add privacy handler to Android clipboard

I have little platform dev experience, so please review with scrutiny.

Comment 15

[Brian Nicholson (:bnicholson)]

Filed bug 831461 for inheriting from nsBaseClipboard.

Comment 16

[(no longer active)]

Comment on attachment 702985 [details] [diff] [review]
Add privacy handler to Android clipboard

As discussed on IRC, we need to make sure that we only delete data copies from private tabs.  We also need to handle stuff copied from the urlbar somehow.

Comment 17

[Brian Nicholson (:bnicholson)]

Attachment: Part 1: Add privacy handler to Android clipboard

Handles clearing all text copied from Gecko by doing a text comparison between the logical clipboard and Android's clipboard.

Comment 18

[Brian Nicholson (:bnicholson)]

Attachment: Part 2: Use Gecko clipboard for Android widgets

Allows clearing private clipboard data copied from Android widgets. Also saves Gecko clipboard data to the bundle so it can be restored between Fennec sessions.

Having to iterate through all open tabs to find a private document is kind of a hack, but we can't always use the selected tab (since we might be restoring the private clipboard data when we're restoring from the Bundle, but the selected tab won't necessarily be private).

Comment 19

[(no longer active)]

Comment on attachment 703680 [details] [diff] [review]
Part 1: Add privacy handler to Android clipboard

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

::: widget/android/nsClipboard.cpp
@@ +12,5 @@
>  
>  using namespace mozilla;
>  using mozilla::dom::ContentChild;
>  
> +#define NS_MOZ_DATA_FROM_PRIVATEBROWSING "application/x-moz-private-browsing"

Please move this to nsClipboardPrivacyHandler.h instead of defining it twice.

@@ +44,3 @@
>    NS_ENSURE_SUCCESS(rv, rv);
> +
> +  NS_IF_RELEASE(mTransferable);

When you make mTransferable an nsCOMPtr, this will no longer be necessary.

@@ +44,5 @@
>    NS_ENSURE_SUCCESS(rv, rv);
> +
> +  NS_IF_RELEASE(mTransferable);
> +  mTransferable = aTransferable;
> +  NS_ADDREF(mTransferable);

Neither will this.

@@ +115,5 @@
>        AndroidBridge::Bridge()->EmptyClipboard();
>    } else {
>      ContentChild::GetSingleton()->SendEmptyClipboard();
>    }
> +  NS_IF_RELEASE(mTransferable);

Just say |mTransferable = nullptr;|.

@@ +158,5 @@
> +              nsAutoString androidBuffer;
> +              rv = GetClipboardBuffer(transferable, androidBuffer);
> +              NS_ENSURE_SUCCESS(rv, rv);
> +
> +              *aHasText = NS_strcmp(geckoBuffer.get(), androidBuffer.get()) == 0;

Nit: please use strcmp.

@@ +182,5 @@
>  }
>  
> +nsresult
> +nsClipboard::GetClipboardBuffer(nsITransferable *aTransferable,
> +                                nsAutoString &aBuffer) {

Nit: open brace on new line please.

Nit#2: Please use nsAString&.

@@ +193,5 @@
> +  // No support for non-text data
> +  NS_ENSURE_TRUE(supportsString, NS_ERROR_NOT_IMPLEMENTED);
> +
> +  rv = supportsString->GetData(aBuffer);
> +  NS_ENSURE_SUCCESS(rv, rv);

You don't need this since you're just going to return rv in the next statement.

::: widget/android/nsClipboard.h
@@ +21,5 @@
> +  ~nsClipboard();
> +
> +protected:
> +  nsRefPtr<nsClipboardPrivacyHandler> mPrivacyHandler;
> +  nsITransferable *mTransferable;

Please make this an nsCOMPtr and get rid of the dtor.

Comment 20

[Aaron Train [:aaronmt]]

Follow-up ping?

Comment 21

[costinel]

Mozilla, thank you for transforming into the new Microsoft "this features works as intended"
#462106
#816998
#827260
#849799
#410669

Comment 22

[Brian Nicholson (:bnicholson)]

After becoming more familiar with Android's lifecycle, this feature simply isn't reasonably possible to implement. Swiping Firefox from the recent apps list (which, after removing the Quit button, we promote as the primary way to manually kill the browser) immediately kills the application process, meaning we can't listen for any destroy callbacks; also see https://bugzilla.mozilla.org/show_bug.cgi?id=823285#c21. At most, we could clear the clipboard only when the user manually closes the last private tab, but that would be incomplete and inconsistent.

The only way to force the onDestroy() callback to be fired would be to do the same thing we did in bug 823285, which would be to use a foreground service. That would not play well with other apps since it would keep the Firefox process alive during private browsing until the user explicitly closes the browser or the last private tab, consuming valuable resources on the device. It would also require creating a persistent notification icon in Android's notification bar during private browsing, and that would just be silly.

Comment 23

[(no longer active)]

Sigh, but fair enough!  Thanks a lot for all of the effort you put into researching this, much appreciated!