1620131 - Generalize search suggestion restrictions

Status: RESOLVED FIXED

(Firefox :: Address Bar, enhancement, P3)

Description

[Martin Thomson [:mt:]]

https://searchfox.org/mozilla-central/rev/d5b34cc8872177d3ee071e06f787c2a14268595b/toolkit/components/places/UnifiedComplete.jsm#62-64 disables search suggestions for things that look like URLs. That is a great privacy feature, but it is limited (currently) to "http", "https", and "ftp".

file:/// is potentially even more sensitive. Adding that to the list would help avoid leaking information about what is on the local filesystem to a search provider.

Separately, I wonder whether it would make sense to drop everything that looks like a real URL from this. I know that people probably aren't typing urn: or one of the local URL-like things that operating systems use (Android seems to like wxyz: for some reasons, Steam uses steam:, etc...), but maybe it is time to cast a wider net.

As before, this wouldn't disable searching, just suggestions.

Comment 1

[Harry Twyford]

Mike, what do you think of disabling search suggestions on any query that begins with some a-z characters and then ://? This is to include file:// and URIs like the examples in comment 0.

To be more precise, queries matching this regex:

/^[a-z]+:(?:\/){0,2}/i

Comment 2

[Martin Thomson [:mt:]]

Hey Harry, if you want to catch all potential URI schemes, you need to include a couple more characters:

      scheme      = ALPHA *( ALPHA / DIGIT / "+" / "-" / "." )

-- https://tools.ietf.org/html/rfc3986#section-3.1

I've seen both '-' and '.' in the wild (not '+', but I've seen proposals to use it). So that's:

/^[a-z][a-z+\.-]*:(?:\/){0,2}/i

Note that your pattern matches one slash, you might be better off with (?:\/\/)?.

Comment 3

[Harry Twyford]

As far as I can tell, we do disable search suggestions for file:// URLs. In fact, we disable suggestions for any queries containing "/", "@", ":", or ".", per bug 1280441 comment 0 which covers the cases in this bug's comment 0. This appears to still be the case in my testing.

The array linked in comment 0 is used to disable restrictions for the strings "http", "http", and "ftp" (without the colon) since those are unlikely to be the start of a search query. I wouldn't say the same of disabling queries starting with "file", since a user could very well start a query with "file"; for example, "file my taxes".

Marco, can you provide any additional context here? Is there a case I'm missing where file:// – or any similar prefix such as steam:// – might be sent to the search engine?

Comment 4

[Marco Bonardo [:mak]]

I suspect the bug was filed by code inspection, but not taking into account all of the additional checks and misunderstanding the scope of that list of schemes (the comment is very misleading!). I also read the bug report too quickly.
Additionally we only recently started using the tokenizer here, so it's possible earlier we needed this additional scheme check.

I think we should check if there's a recognizable protocol at the beginning of the string, and bailout if so, but that check should not be limited to a handful of protocols like it was here.

I think we should do a few simple things here:

1. verify that we have tests in place to check we don't fetch search suggestions for common urls (file, http, https, ftp, about...), even if those urls are incomplete or invalid. I think the tokenizer code is doing a decent job, but we must validate that with tests for all the common cases coming to our mind.
2. remove the explicit check for these protocols. I think it should be perfectly valid for a Web Developer to search for "http something", for example to check an RFC or a code reference.

Comment 5

[Harry Twyford]

Attachment: Bug 1620131 - Generalize search suggestion restrictions. r?mak

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/29cbb2613acc
Generalize search suggestion restrictions. r=mak

Comment 7

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/29cbb2613acc