performing a search from the browser search field leaks search suggest results to the disk cache

VERIFIED FIXED in Firefox 18

Status

()

Firefox
Search
VERIFIED FIXED
6 years ago
6 years ago

People

(Reporter: jdm, Assigned: Ehsan)

Tracking

Trunk
Firefox 19
x86_64
All
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(firefox18+ verified)

Details

(Whiteboard: [testday-20121012])

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

6 years ago
STR:
1. enter PB mode
2. clear cache
3. search "open office" from search bar
4. check about:cache
(Reporter)

Updated

6 years ago
status-firefox18: --- → affected
tracking-firefox18: --- → ?
(Reporter)

Updated

6 years ago
OS: Linux → All
(Reporter)

Comment 1

6 years ago
Oh boy, nsNSSCallbacks creates a channel and loadgroup out of thin air in nsHTTPDownloadEvent::Run.
Assignee: nobody → josh
(Reporter)

Updated

6 years ago
Component: General → Security
Seems like this depends on bug 722979 so that PSM knows to set the private browsing flag on the channel.

Also note that AuthCertificate() writes data to disk permanently and unconditionally.
Component: Security → Security: PSM
(Reporter)

Comment 3

6 years ago
I was wrong, this isn't the PSM code. There's an XMLHttpRequest that's created in search.xml.
Component: Security: PSM → General
Product: Core → Firefox
(Reporter)

Updated

6 years ago
Blocks: 723005
(Assignee)

Comment 4

6 years ago
Created attachment 670955 [details] [diff] [review]
Patch (v1)

The test for this patch depends on the patch in bug 723001, so I'll wait until that gets landed.
Assignee: josh → ehsan
Status: NEW → ASSIGNED
Attachment #670955 - Flags: review?(josh)
(Assignee)

Updated

6 years ago
Component: General → Search
Depends on: 723001
(Reporter)

Comment 5

6 years ago
Comment on attachment 670955 [details] [diff] [review]
Patch (v1)

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

I don't think I should review this.
Attachment #670955 - Flags: review?(josh)
Comment on attachment 670955 [details] [diff] [review]
Patch (v1)

You never use split("|")[1] in the front-end code, so the change to autocompletesearchparam in search.xml seems unnecessary.
Attachment #670955 - Flags: review-
(Reporter)

Comment 7

6 years ago
>+            this.setAttribute("autocompletesearchparam",
>+              this.getAttribute("autocompletesearchparam").split("|")[0] +
>+              (PrivateBrowsingUtils.isWindowPrivate(window) ? "private" : "non-private"));

Won't this also give searchbar-historyprivate? I think you want a | in between.
(Assignee)

Comment 8

6 years ago
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #6)
> Comment on attachment 670955 [details] [diff] [review]
> Patch (v1)
> 
> You never use split("|")[1] in the front-end code, so the change to
> autocompletesearchparam in search.xml seems unnecessary.

I did that to make it easier to know that the autocompletesearchparam is carrying two pieces of information by just looking at the code, but I'll take it out.

(In reply to Josh Matthews [:jdm] from comment #7)
> >+            this.setAttribute("autocompletesearchparam",
> >+              this.getAttribute("autocompletesearchparam").split("|")[0] +
> >+              (PrivateBrowsingUtils.isWindowPrivate(window) ? "private" : "non-private"));
> 
> Won't this also give searchbar-historyprivate? I think you want a | in
> between.

Good catch.  I did that locally in a different commit and then forgot to squash it. :/
(Assignee)

Comment 9

6 years ago
Created attachment 670992 [details] [diff] [review]
Patch (v2)
Attachment #670955 - Attachment is obsolete: true
Attachment #670992 - Flags: review?(gavin.sharp)
Comment on attachment 670992 [details] [diff] [review]
Patch (v2)

Rather than actually setting the autocompletesearchparam attribute in openPopup, I think you should override the searchparam getter (http://hg.mozilla.org/mozilla-central/annotate/90857937b601/toolkit/content/widgets/autocomplete.xml#l159), and have it return the hacked value. Then change all the other users of the attribute in search.xml to just use this.searchParam rather than the attribute.

Then add a bunch of comments explaining what the deal is with this magic string argument munging, because this is incredibly gross :(
Attachment #670992 - Flags: review?(gavin.sharp) → review-
Whiteboard: [testday-20121012]
(Assignee)

Comment 11

6 years ago
(In reply to comment #10)
> Comment on attachment 670992 [details] [diff] [review]
>   --> https://bugzilla.mozilla.org/attachment.cgi?id=670992
> Patch (v2)
> 
> Rather than actually setting the autocompletesearchparam attribute in
> openPopup, I think you should override the searchparam getter
> (http://hg.mozilla.org/mozilla-central/annotate/90857937b601/toolkit/content/widgets/autocomplete.xml#l159),
> and have it return the hacked value. Then change all the other users of the
> attribute in search.xml to just use this.searchParam rather than the attribute.
> 
> Then add a bunch of comments explaining what the deal is with this magic string
> argument munging, because this is incredibly gross :(

Hmm, do you really think that is the best solution?  I thought about that, but this is something that only the search box autocomplete code will ever need as far as I can tell, and baking this logic into autocomplete.xml looks like a mistake to me.  We can keep the magic string local to the search box autocomplete, so why spread this plague across all autocomplete fields?

(I totally agree that this is gross, but the alternative would be to modify the autocomplete interfaces, which would be even more gross, as it spreads the sickness across the code base.  I _really_ would like to keep this horribleness local to the search box implementation.  :/)
(In reply to Ehsan Akhgari [:ehsan] from comment #11)
> Hmm, do you really think that is the best solution?  I thought about that,
> but this is something that only the search box autocomplete code will ever
> need as far as I can tell, and baking this logic into autocomplete.xml looks
> like a mistake to me.  We can keep the magic string local to the search box
> autocomplete, so why spread this plague across all autocomplete fields?

You don't need to touch anything in autocomplete.xml to implement my suggestion. I'm suggesting that you override the searchParam getter in search.xml.

Updated

6 years ago
tracking-firefox18: ? → +
(Assignee)

Comment 13

6 years ago
Created attachment 671540 [details] [diff] [review]
Patch (v3)
Attachment #670992 - Attachment is obsolete: true
Attachment #671540 - Flags: review?(gavin.sharp)
Comment on attachment 671540 [details] [diff] [review]
Patch (v3)

>diff --git a/browser/components/search/content/search.xml b/browser/components/search/content/search.xml

>+              textBox._formHistSvc.addEntry(this.searchParams, textValue);

typo: searchParam (no "s")

I think this looks OK, but it needs comments (both in search.xml and nsSearchSuggestions) about the hacks (i.e. how you're abusing searchParam), and ideally a test (do we really not have one that would have caught this typo? :/).
Attachment #671540 - Flags: review?(gavin.sharp) → review-
(Assignee)

Comment 15

6 years ago
I'll write a test for it once bug 723001 relands.
Flags: in-testsuite?
Summary: Performing a search from the browser search field leaks request URIs to the disk cache → performing a search from the browser search field leaks search suggest results to the disk cache
(Assignee)

Comment 16

6 years ago
Created attachment 671617 [details] [diff] [review]
Patch (v4)

So I'm pretty sure that we don't want to change the getAttribute calls in search.xml to use the property, since those should all return "searchbar-history".  I guess we could use .searchParams.split("|")[0] there but that wouldn't make a lot of sense to me.  Please let me know if you disagree.
Attachment #671540 - Attachment is obsolete: true
Attachment #671617 - Flags: review?(gavin.sharp)
Comment on attachment 671617 [details] [diff] [review]
Patch (v4)

You're right about the getAttribute calls in search.xml - those don't want the modified value, only the GetSearchParam caller in nsAutocompleteController does.

I don't think you need to modify set the searchParam setter - in practice no one really calls that on the search bar, and if they somehow did I don't think they'd want this weird behavior.

You could only pass "|private" in private browsing windows and leave searchParam unmodified otherwise (and change the check in nsSearchSuggestions accordingly). That might reduce the likelihood of this hack causing trouble outside of PB windows.

Still need tests!
Attachment #671617 - Flags: review?(gavin.sharp) → review+
(Assignee)

Comment 19

6 years ago
Comment on attachment 671617 [details] [diff] [review]
Patch (v4)

This is not the most beautiful patch ever, but it gets the job done.  I think it has a medium risk level, but we do need it for Firefox 18, and it's best to land it earlier in the cycle than later so that we can get testing and deal with any possible regressions.
Attachment #671617 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/b39bc1f5e0f8
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 19
(In reply to Josh Matthews [:jdm] from comment #0)
> STR:
> 1. enter PB mode
> 2. clear cache
> 3. search "open office" from search bar
> 4. check about:cache

In PB mode, leave google.com tab opened, and in another tab open about:cache. After couples of minutes, there will be a disk cache entry: https://sb-ssl.google.com/safebrowsing/newkey?client=navclient-auto-ffox&appver=19.0a1&pver=2.2 and that will not disappear after exiting PB.
Same behavior on yahoo.com, amazon.com. Tested on Nightly 19.0a1 (2012-10-16)
(Reporter)

Comment 22

6 years ago
That's not an entry we need to care about. It doesn't reveal anything about where the user has visited, so it's fine.
(Assignee)

Comment 23

6 years ago
(In reply to comment #22)
> That's not an entry we need to care about. It doesn't reveal anything about
> where the user has visited, so it's fine.

Precisely.

Updated

6 years ago
Attachment #671617 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(Assignee)

Comment 24

6 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/1372975ca418
status-firefox18: affected → fixed
No search suggest results to the disk cache. Verified fixed Mozilla/5.0 (Windows NT 6.1; WOW64; rv:18.0) Gecko/20100101 Firefox/18.0b3
Status: RESOLVED → VERIFIED
status-firefox18: fixed → verified
You need to log in before you can comment on or make changes to this bug.