Closed Bug 861691 Opened 7 years ago Closed 7 years ago

Search bar should not save search history in private windows

Categories

(SeaMonkey :: Search, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
seamonkey2.20

People

(Reporter: neil, Assigned: neil)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

4.25 KB, patch
philip.chee
: review+
iann_bugzilla
: feedback+
philip.chee
: feedback+
iann_bugzilla
: approval-comm-aurora+
iann_bugzilla
: approval-comm-beta+
Details | Diff | Splinter Review
Steps to reproduce problem:
1. Open a private window
2. Customise the toolbar to show the search bar
3. Perform a search
4. Press down arrow

Actual results: Search just performed appears in search history

Expected results: Search history unaffected by searches in private windows
Attached patch Possible patchSplinter Review
While I was looking into this I noticed that the search backend (nsSearchSuggestions.js) expects the autocomplete search param to have a |private suffix when the window is private, so that it can use private channels. However the problem then appears to arise that the code to clear search suggestions (which I presume should continue to function in private windows) does not want the |private suffix. (Firefox doesn't do this, so I don't know whether it gets this wrong here, or whether I have misread the code.) To work around this I have simply hardcoded the search param when dealing with the search history.
Assignee: nobody → neil
Status: NEW → ASSIGNED
Attachment #737311 - Flags: feedback?(philip.chee)
Attachment #737311 - Flags: feedback?(iann_bugzilla)
Summary: Search bar should not save suggestions in private windows → Search bar should not save search history in private windows
(In reply to neil@parkwaycc.co.uk from comment #1)
> (Firefox doesn't do this, so I don't know whether it gets this wrong here, or
> whether I have misread the code.)

In the Firefox version of search.xml, the "private" suffix is only added by the JS getter (.searchParam, read by the autocomplete controller), the DOM attribute itself is left as-is.
(In reply to Gavin Sharp from comment #2)
> (In reply to comment #1)
> > (Firefox doesn't do this, so I don't know whether it gets this wrong here, or
> > whether I have misread the code.)
> 
> In the Firefox version of search.xml, the "private" suffix is only added by
> the JS getter (.searchParam, read by the autocomplete controller), the DOM
> attribute itself is left as-is.

Subtle! Turns out it was my idea (bug 640427 comment 2, but only one version got changed) to switch SeaMonkey's "clear search autocomplete" to use the getter instead of the attribute...
Comment on attachment 737311 [details] [diff] [review]
Possible patch

> +        if (this.usePrivateBrowsing)
> +          this._textbox.searchParam += "|private";

> -              textBox._formHistSvc.addEntry(textBox.getAttribute("autocompletesearchparam"),
> -                                            textValue);
> +              textBox._formHistSvc.addEntry("searchbar-history", textValue);

> -              return this._formHistSvc.nameExists(this.searchParam);
> +              return this._formHistSvc.nameExists("searchbar-history");

> -              var param = this.getAttribute("autocompletesearchparam");
> -              this._formHistSvc.removeEntriesForName(param);
> +              this._formHistSvc.removeEntriesForName("searchbar-history");

Instead of hardcoding for "searchbar-history" can we (in the constructor) cache the original autocompletesearchparam and reuse this?
Comment on attachment 737311 [details] [diff] [review]
Possible patch

f=me
Attachment #737311 - Flags: feedback?(philip.chee) → feedback+
Comment on attachment 737311 [details] [diff] [review]
Possible patch

Should switching to a different search engine within the private window be remembered?
Attachment #737311 - Flags: feedback?(iann_bugzilla) → feedback+
> Should switching to a different search engine within the private window be remembered?
What does Firefox do in this case?
>> Should switching to a different search engine within the private window be remembered?
> What does Firefox do in this case?
As far as I can tell, Firefox doesn't check the private browsing status when switching to a different search engine.
Attachment #737311 - Flags: review?(philip.chee)
Comment on attachment 737311 [details] [diff] [review]
Possible patch

Discussed this over IRC.
r=me
Attachment #737311 - Flags: review?(philip.chee) → review+
Pushed comm-central changeset 3658f802883b.
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.20
Comment on attachment 737311 [details] [diff] [review]
Possible patch

[Approval Request Comment]
Regression caused by (bug #): N/A
User impact if declined: Private search information leak
Testing completed (on m-c, etc.): Landed on c-c a few days ago
Risk to taking this patch (and alternatives if risky): Low
String changes made by this patch: None
Attachment #737311 - Flags: approval-comm-release?
Attachment #737311 - Flags: approval-comm-beta?
Attachment #737311 - Flags: approval-comm-aurora?
Attachment #737311 - Flags: approval-comm-beta?
Attachment #737311 - Flags: approval-comm-beta+
Attachment #737311 - Flags: approval-comm-aurora?
Attachment #737311 - Flags: approval-comm-aurora+
Attachment #737311 - Flags: approval-comm-release?
You need to log in before you can comment on or make changes to this bug.