Closed Bug 1280441 Opened 5 years ago Closed 4 years ago

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

Categories

(Firefox :: Address Bar, defect, P1)

Product:
Firefox
Component:
Address Bar
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: mak, Assigned: standard8)

References

Details

(Whiteboard: [fxsearch])

Attachments

(3 files)

Bug 1280441 - Improve some of the test documentation for unified complete.
59 bytes, text/x-review-board-request
mak
: review+
Details
Bug 1280441 - Add tests for what happens when existing http/https/ftp prefixes are entered.
59 bytes, text/x-review-board-request
mak
: review+
Details
Bug 1280441 - Avoid suggesting searches from the URL bar when they look like urls (http, https, ftp) or the start of urls.
59 bytes, text/x-review-board-request
mak
: review+
Details 
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.
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.
Priority: P2 → P1
Blocks: 1344928
Assignee: nobody → standard8
Status: NEW → ASSIGNED
Marco/Florian, do you have any more ideas on top of those in comment 0?
Flags: needinfo?(mak77)
Flags: needinfo?(florian)
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.
Flags: needinfo?(mak77)
(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.
Flags: needinfo?(florian)
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 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} ...
         ...
       }
Attachment #8866359 - Flags: review?(mak77) → review-
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).
Attachment #8866361 - Flags: review?(mak77) → review-
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 :)
Attachment #8866359 - Flags: review?(mak77) → review+
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
Attachment #8866360 - Flags: review?(mak77) → review+
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.
Attachment #8866361 - Flags: review?(mak77) → review+
Pushed by mbanner@mozilla.com:
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
Blocks: 1620131
You need to log in before you can comment on or make changes to this bug.