Closed Bug 1620131 Opened 5 years ago Closed 5 years ago

Generalize search suggestion restrictions

Categories

(Firefox :: Address Bar, enhancement, P3)

enhancement
Points:
2

Tracking

()

RESOLVED FIXED
Firefox 76
Iteration:
76.1 - Mar 9 - Mar 22
Tracking Status
firefox76 --- fixed

People

(Reporter: mt, Assigned: bugzilla)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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.

Assignee: nobody → htwyford
Status: NEW → ASSIGNED
Iteration: --- → 76.1 - Mar 9 - Mar 22
Points: --- → 2
Priority: -- → P3

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
Flags: needinfo?(mconnor)

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 (?:\/\/)?.

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?

Flags: needinfo?(mconnor) → needinfo?(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.
Flags: needinfo?(mak)
Summary: Avoid search suggestions for file:// URLs → Generalize search suggestion restrictions
Pushed by htwyford@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/29cbb2613acc Generalize search suggestion restrictions. r=mak
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 76
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: