1280441 - Avoid search suggestions when they could worry the user about his privacy

Status: RESOLVED FIXED

(Firefox :: Address Bar, defect, P1)

Description

[Marco Bonardo [:mak]]

The current algorithm avoids search suggestions if:
1. the string is shorter than 2 chars
2. the first "token" is a whitelisted domain (like localhost)
3. if the string includes any of "/", "@", ":", "."
4. we are in a private window

There are other cases we would like to handle, for example if the user typed a commonly used scheme like http, https, ftp.

Any ideas for further cases we should handle are welcome.

Comment 1

[Marco Bonardo [:mak]]

as a side note, the "string" in 3. is actually the typed string minus "http://www.", since that's what the awesomebar usually looks for, and that is likely part of the problem here.

Comment 2

[Mark Banner (:standard8)]

Marco/Florian, do you have any more ideas on top of those in comment 0?

Comment 3

[Marco Bonardo [:mak]]

No further ideas from me.

What I meant in comment 1 is that the original code we added to avoid entries that may scare the user, is examining a filtered search string, that doesn't include http and similar prefixes (_searchString), while it should probably examine _trimmedOriginalSearchString.

The reason I didn't originally do that, is that _trimmedOriginalSearchString may contain restriction chars, and if a restriction char overlaps with one of the chars that make the string look like a url (for example @ can be both part of a url or the restriction char for match.url) we may do the wrong thing.
In practice the only restriction char that matters here is the one for searches (currently "$", even if I'd like to change it to "?"), since for any other restriction the suggestions would not be provided at all, so the problem is minor than I thought.

http://searchfox.org/mozilla-central/rev/ae8c2e2354db652950fe0ec16983360c21857f2a/toolkit/components/places/UnifiedComplete.js#1270

Basically it's likely this bug would be partially fixed by testing the original typed string, rather than the stripped one, since then typing "http://test" would end up examinining *that* string, not "test".

That's clearly not enough, considered we also want to exclude common schemes typed at the beginning of the string, but it would be a good start.

Comment 4

[Florian Quèze [:florian]]

(In reply to Mark Banner (:standard8) from comment #2)
> Marco/Florian, do you have any more ideas on top of those in comment 0?

No additional idea; from my point of view http/https are the most important ones.

Comment 5

[Mark Banner (:standard8)]

Attachment: Bug 1280441 - Improve some of the test documentation for unified complete.

Review commit: https://reviewboard.mozilla.org/r/138000/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/138000/

Comment 6

[Mark Banner (:standard8)]

Attachment: Bug 1280441 - Add tests for what happens when existing http/https/ftp prefixes are entered.

Review commit: https://reviewboard.mozilla.org/r/138002/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/138002/

Comment 7

[Mark Banner (:standard8)]

Attachment: Bug 1280441 - Avoid suggesting searches from the URL bar when they look like urls (http, https, ftp) or the start of urls.

Review commit: https://reviewboard.mozilla.org/r/138004/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/138004/

Comment 8

[Mark Banner (:standard8)]

I've put something together based on matching the start of the url. I added lots of tests to make sure that we were covering everything, and we could see the before/after effects.

Please test it out and see if it works as expected.

Comment 9

[Marco Bonardo [:mak]]

Comment on attachment 8866359 [details]
Bug 1280441 - Improve some of the test documentation for unified complete.

https://reviewboard.mozilla.org/r/138000/#review141540

::: toolkit/components/places/tests/unifiedcomplete/head_autocomplete.js:116
(Diff revision 1)
> + * @param {Object} match The expected match for the result.
> + * @param {nsIURI} match.uri The expected uri.
> + * @param {String} match.title The title of the entry.
> + * @param {String} match.tags The tags for the entry.
> + * @param {String} match.style The style of the entry.
> + * @param {Object} result The result to compare the result against with the same

See the comment for check_autocomplete.

::: toolkit/components/places/tests/unifiedcomplete/head_autocomplete.js:152
(Diff revision 1)
>  }
>  
> +/**
> + * Helper function to test an autocomplete entry and check the resultant matches.
> + *
> + * @param {Object} test The details of the test to run, wih the following items:

s/items/properties/?

::: toolkit/components/places/tests/unifiedcomplete/head_autocomplete.js:153
(Diff revision 1)
>  
> +/**
> + * Helper function to test an autocomplete entry and check the resultant matches.
> + *
> + * @param {Object} test The details of the test to run, wih the following items:
> + * @param {String} test.search The string to enter for autocompleting.

Not a great fan of this notation, since it looks like this takes multiple params.
I'd prefer something like:

@param {Object} test
       An object representing the test to run, in the following form:
       {
         search: {String} The string to....
         param: {String} ...
         ...
       }

Comment 10

[Marco Bonardo [:mak]]

Comment on attachment 8866361 [details]
Bug 1280441 - Avoid suggesting searches from the URL bar when they look like urls (http, https, ftp) or the start of urls.

https://reviewboard.mozilla.org/r/138004/#review141542

::: toolkit/components/places/UnifiedComplete.js:1322
(Diff revision 1)
> +    // like urls.
> +    if ((this._trimmedOriginalSearchString.length <= DISALLOWED_URL_PREFIXES_EXACT_MATCH_MAX_LENGTH &&
> +         DISALLOWED_URL_PREFIXES_EXACT_MATCH.some(prefix => this._trimmedOriginalSearchString == prefix)) ||
> +        DISALLOWED_URL_PREFIXES_START_MATCH.some(prefix => this._trimmedOriginalSearchString.startsWith(prefix))) {
> +      return true;
> +    }

As per the brief IRC discussion, let's make this simpler, just exclude perfect matches for "http", "https", "ftp", and startsWith(same_as_before + ":")

This will simplify the code and in the end it's what we care about.

I don't think "ht", "htt" or "ft" cause any harm, they can be about totally different stuff than urls (as you suggested on IRC I may be willing to search for httpd).

Comment 11

[Mark Banner (:standard8)]

Comment on attachment 8866359 [details]
Bug 1280441 - Improve some of the test documentation for unified complete.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/138000/diff/1-2/

Comment 12

[Mark Banner (:standard8)]

Comment on attachment 8866360 [details]
Bug 1280441 - Add tests for what happens when existing http/https/ftp prefixes are entered.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/138002/diff/1-2/

Comment 13

[Mark Banner (:standard8)]

Comment on attachment 8866361 [details]
Bug 1280441 - Avoid suggesting searches from the URL bar when they look like urls (http, https, ftp) or the start of urls.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/138004/diff/1-2/

Comment 14

[Marco Bonardo [:mak]]

Comment on attachment 8866359 [details]
Bug 1280441 - Improve some of the test documentation for unified complete.

https://reviewboard.mozilla.org/r/138000/#review141660

::: toolkit/components/places/tests/unifiedcomplete/head_autocomplete.js:112
(Diff revision 2)
> -// A helper for check_autocomplete to check a specific match against data from
> -// the controller.
> +/**
> + * A helper for check_autocomplete to check a specific match against data from
> + * the controller.
> + *
> + * @param {Object} match The expected match for the result, in the following
> + *                       form:

nit: let this flow in the previous line, even if it goes slightly over 80 chars, we're not so strict :)

Comment 15

[Marco Bonardo [:mak]]

Comment on attachment 8866360 [details]
Bug 1280441 - Add tests for what happens when existing http/https/ftp prefixes are entered.

https://reviewboard.mozilla.org/r/138002/#review141662

Comment 16

[Marco Bonardo [:mak]]

Comment on attachment 8866361 [details]
Bug 1280441 - Avoid suggesting searches from the URL bar when they look like urls (http, https, ftp) or the start of urls.

https://reviewboard.mozilla.org/r/138004/#review141666

::: toolkit/components/places/UnifiedComplete.js:106
(Diff revision 2)
>  const QUERYINDEX_TYPED         = 7;
>  const QUERYINDEX_PLACEID       = 8;
>  const QUERYINDEX_SWITCHTAB     = 9;
>  const QUERYINDEX_FRECENCY      = 10;
>  
> +const DISALLOWED_URL_PREFIXES = [

this could probably be named better, but I have no fantasy! DISALLOWED_URLLIKE_PREFIXES would be (imo) a first trivial improvement.
Also a comment above it explaining the expected behavior may help future archeology.

::: toolkit/components/places/tests/unifiedcomplete/test_search_suggestions.js
(Diff revision 2)
> -
> -  yield check_autocomplete({
> -    search: "htt",
> -    searchParam: "enable-actions",
> -    matches: [
> -      makeSearchMatch("htt", { engineName: ENGINE_NAME, heuristic: true }),

why removing the "htt" test, it would be good to check we return suggestions for it.

Comment 17

[Mark Banner (:standard8)]

Comment on attachment 8866359 [details]
Bug 1280441 - Improve some of the test documentation for unified complete.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/138000/diff/2-3/

Comment 18

[Mark Banner (:standard8)]

Comment on attachment 8866360 [details]
Bug 1280441 - Add tests for what happens when existing http/https/ftp prefixes are entered.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/138002/diff/2-3/

Comment 19

[Mark Banner (:standard8)]

Comment on attachment 8866361 [details]
Bug 1280441 - Avoid suggesting searches from the URL bar when they look like urls (http, https, ftp) or the start of urls.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/138004/diff/2-3/

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/dca652d1fce1
Improve some of the test documentation for unified complete. r=mak
https://hg.mozilla.org/integration/autoland/rev/27a97108e1a4
Add tests for what happens when existing http/https/ftp prefixes are entered. r=mak
https://hg.mozilla.org/integration/autoland/rev/24bb92425086
Avoid suggesting searches from the URL bar when they look like urls (http, https, ftp) or the start of urls. r=mak

Comment 21

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/dca652d1fce1
https://hg.mozilla.org/mozilla-central/rev/27a97108e1a4
https://hg.mozilla.org/mozilla-central/rev/24bb92425086