Closed Bug 1048444 Opened 5 years ago Closed 5 years ago

Search activity displays private browsing searches from browser

Categories

(Firefox for Android Graveyard :: Search Activity, defect, P1)

34 Branch
All
Android
defect

Tracking

(firefox33 fixed, firefox34 verified, fennec33+)

VERIFIED FIXED
Firefox 34
Tracking Status
firefox33 --- fixed
firefox34 --- verified
fennec 33+ ---

People

(Reporter: suirtemedb, Assigned: Margaret)

References

Details

(Keywords: privacy)

Attachments

(1 file, 1 obsolete file)

User Agent: Mozilla/5.0 (Android; Mobile; rv:34.0) Gecko/34.0 Firefox/34.0 (Nightly/Aurora)
Build ID: 20140803030203

Steps to reproduce:

Opened a private tab and searched for something


Actual results:

Opened the nightly search app and found my private searches in the history


Expected results:

Private tab searches shouldn't be found in the history of the nightly search apo
Hardware: Other → ARM
Version: unspecified → Firefox 34
tracking-fennec: --- → ?
Component: General → Search Activity
Keywords: privacy
Summary: Firefox search (nightly search) has private tab searches in the historu → Search activity displays private searches from browser
Status: UNCONFIRMED → NEW
Ever confirmed: true
Blocks: search
Summary: Search activity displays private searches from browser → Search activity displays private/guest browsing searches from browser
This is no good! This was introduced by bug 1038227.

We won't be showing these search terms anywhere in Firefox 33, but we should still uplift a fix.
Assignee: nobody → eric.edens
Blocks: 1038227
Status: NEW → ASSIGNED
tracking-fennec: ? → 33+
Hardware: ARM → All
This is important, let's get a fix sooner rather than later.
Priority: -- → P1
I can take this.
Assignee: eric.edens → margaret.leibovic
Guest browsing shouldn't be an issue here because searches would be stored to the guest profile, and we only read search terms from the default profile in the search activity.
Summary: Search activity displays private/guest browsing searches from browser → Search activity displays private browsing searches from browser
Attachment #8472553 - Flags: review?(rnewman)
Comment on attachment 8472553 [details] [diff] [review]
Don't store searches that happen in private browsing mode

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

::: mobile/android/base/BrowserApp.java
@@ +1826,5 @@
>       *        a search query to store. We won't store empty queries.
>       */
>      private void storeSearchQuery(String query) {
> +        // Don't store searches that happen in private tabs. This assumes the user can only
> +        // perform a search inside the currently selected tab.

This assumption is technically wrong, and it might or might not matter in practice.

There are three sources of calls for this method:

* BrowserApp.onSearch, from SearchEngineRow; this is safe.
* BrowserApp.handleMessage, via EventDispatcher; this is not quite safe. It's possible for the user to change tabs before the event gets processed.
* BrowserApp.commitEditingMode, which spawns a Runnable on the background thread to record a barkeyword search. This is definitely not safe; the background thread can block for arbitrary amounts of time.

In practice I doubt this is a concern; people don't use keyword searches much. The right solution is for BrowserApp.recordSearch/commitEditingMode, and JS-side message senders, to not send (undecorated) Search: messages -- not only will that avoid this kind of race, but it'll also stop us storing these searches in FHR, if we're doing that.

Take a look, see if it's easy to be safer. If not, then r+.
Attachment #8472553 - Flags: review?(rnewman) → review+
This aims to address the risky points that rnewman raised in his last comment by dropping the query term in JS.

This still makes an assumption that you aren't loading the search in a background tab. We're already making that assumption for the userSearch property, so I'm hoping that's reasonable, but I don't always trust our past selves :)
Attachment #8472647 - Flags: review?(bnicholson)
Comment on attachment 8472647 [details] [diff] [review]
(alternate)  Don't store searches that happen in private browsing mode (

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

Hopefully nsDefaultURIFixup dispatches the keyword-search event synchronously (it looks like it does); otherwise, the "change tabs before the event gets processed" race would still apply.
Attachment #8472647 - Flags: review?(bnicholson) → review+
Attachment #8472553 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/7aef335d6627
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 34
Flags: qe-verify+
QA Contact: aaron.train
Comment on attachment 8472647 [details] [diff] [review]
(alternate)  Don't store searches that happen in private browsing mode (

Approval Request Comment
[Feature/regressing bug #]: bug 1038227
[User impact if declined]: private browsing search terms are stored and can appear in search activity
[Describe test coverage new/current, TBPL]: no automated tests, tested locally and landed on Nightly
[Risks and why]: low-risk, adds checks around calls to store search terms to make sure we're not in private browsing mode
[String/UUID change made/needed]: n/a
Attachment #8472647 - Flags: approval-mozilla-aurora?
Attachment #8472647 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.