Closed Bug 1341350 Opened 7 years ago Closed 7 years ago

Visiting an insecure (http://) site that permanently redirects to secure (https://) should only offer the secure version in the visit autofill action

Categories

(Toolkit :: Places, defect, P2)

Product:
Toolkit
Component:
Places
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
relnote-firefox --- 55+
firefox55 --- fixed

People

(Reporter: standard8, Assigned: standard8)

References

(Blocks 1 open bug)

Details

(Whiteboard: [fxsearch])

Attachments

(2 files)

Bug 1341350 - Add more tests to cover updating of host prefixes when new results are added to the places table.
59 bytes, text/x-review-board-request
mak
: review+
Details
Bug 1341350 - Change the hosts prefix algorithm to prefer secure urls, but also to be able to swap back and forth depending on typed entries in history.
59 bytes, text/x-review-board-request
mak
: review+
Details 
STR:

1) Create a fresh profile.
2) Visit a site that has a permanent redirect from http:// to https://, e.g. pinterest.com
3) See that https://pinterest.com and https://www.pinterest.com are the ones redirected to.
4) Open a new tab
5) Enter "pin"

Actual Results

The first entry (autofill action) is displayed with "pinterest.com"
The second entry is "https://www.pinterest.com"

Expected Results

The first entry is displayed as "https://pinterest.com".


The internal caching of the redirects does mean that we will only generally visit the secure version, but we should investigate if we can change that to update the display to give the user reassurance that we will only visit the secure version (and that's where they will end up).
This is a bit like bug 902338, but I'm thinking for the specific case of 301 http -> https only.
See Also: → 902338
Depends on: 1322747
See Also: → 1060546
Moving to frecency, as I suggested in bug 1239708, would help us now that redirects are handled properly by frecency.
Ok, that seems reasonable, let make this depend on that bug for now.
Assignee: standard8 → nobody
Depends on: 1239708
Priority: -- → P3
While working on something else my eyes dropped on the trigger doing this, I think I know what's the problem.
This is our current algorithm:

 * Given a host, examine associated pages and:
 *  - if all of the typed pages start with https://www. return https://www.
 *  - if all of the typed pages start with https:// return https://
 *  - if all of the typed pages start with ftp: return ftp://
 *  - if all of the typed pages start with www. return www.
 *  - otherwise don't use any prefix

It's clear that my assumption about "typing https even just once will fix it" is false, since we check that all the typed pages start with a given prefix.
We could, as a temporary solution until bug 1239708, change the code to use average instead of all, that would mean changing min() with cast(round(avg(substr(url,1,12) = 'https://www.')) AS integer) (And so on for all the other cases) in HOSTS_PREFIX_PRIORITY_FRAGMENT.

We could even go the complete opposite side and tell "if at least one typed page...", but then we'd be stuck with the opposite problem where it would be hard to go to the unsecure version. The avg seems to go the same direction that bug 1239708 is headed, where it would basically use frequency instead of frecency.
bumping priority for the security benefit, we are all-in for https.
Priority: P3 → P2
Blocks: 1352847
Assignee: nobody → standard8
(In reply to Marco Bonardo [::mak] from comment #5)
> We could, as a temporary solution until bug 1239708, change the code to use
> average instead of all, that would mean changing min() with
> cast(round(avg(substr(url,1,12) = 'https://www.')) AS integer) (And so on
> for all the other cases) in HOSTS_PREFIX_PRIORITY_FRAGMENT.

So if I make this change, then it seems to work reasonably well for the simple cases of http://site and https://site.

What doesn't work so well, is for example visit these:

- www.stackoverflow.com (redirects to stackoverflow.com)
- stackoverflow.com
- https://stackoverflow.com

You get stuck in a place where you'll always get non-secure (the avg is 0.33), until one of the non-secure expires (or is deleted).

This is because we're looking for the rev_host, which returns just three results. I don't think we want to start searching within the actual hosts as that'll probably cause other issues and will likely flip the suggested value too soon.

Marco, any thoughts here?
Flags: needinfo?(mak77)
(In reply to Mark Banner (:standard8) from comment #7)
> - www.stackoverflow.com (redirects to stackoverflow.com)
> - stackoverflow.com
> - https://stackoverflow.com
> 
> You get stuck in a place where you'll always get non-secure (the avg is
> 0.33), until one of the non-secure expires (or is deleted).

Well, that's not correct, you are stuck until you add more pages for one of those prefixes, not forever, while the current situation is that we are stuck forever until a page is expired (because adding more pages won't change the fact ALL the pages should have the same prefix). Isn't it?

> This is because we're looking for the rev_host, which returns just three
> results.

We look at all the pages having the given rev_host, if a page has the given prefix it gets a 1, otherwise it gets a 0, and then we average the result, it's 3 results in this particular situations, but will fetch more results when pages to that host are added to history.
Far from being perfect, but it's an improvement, because the situation CAN evolve.

Note, we could even go full speed towards implementing the suggestion in bug 1239708, depending on time availability. This bug is intended as a very quick stop gap, if it ends up taking more time than expected, we should rather look into the long term solution.
Flags: needinfo?(mak77)
even if, actually, bug 1239708 will still need some way to select a prefix, so this prior work may be needed regardless.
I discussed this with Marco over irc and realised that my tests had been assuming that we would be running the average on any url in moz_places, not just the typed ones. The typed ones are the correct choice, so there's no issues here.
I've added lots of tests here, I may have overdone it a bit, but since they run fast, it didn't seem to matter too much, and having more is possibly better.
Comment on attachment 8864135 [details]
Bug 1341350 - Add more tests to cover updating of host prefixes when new results are added to the places table.

https://reviewboard.mozilla.org/r/135824/#review139114

::: toolkit/components/places/tests/unit/test_hosts_triggers.js:36
(Diff revision 1)
>    return result;
>  }
>  
> -function isHostInMozHosts(aURI, aTyped, aPrefix) {
> +function checkHostInMozHosts(aURI, aTyped, aPrefix, aShouldBePresent = true) {
> +  if (typeof aURI == "string") {
> +    aURI = NetUtil.newURI(aURI);

please, use new URL() (should already be in the context since it's imported by head_common.js).
Also, in new code let's prefer Services.io.newURI to NetUtil.newURI, unless we really don't know if it's an uri or an nsIFile. So one day we could be able to remove most of the NetUtil imports.

::: toolkit/components/places/tests/unit/test_hosts_triggers.js:56
(Diff revision 1)
> +  if (aShouldBePresent) {
> +    do_check_true(result, {});
> +    do_check_eq(result.typed, aTyped);
> +    do_check_eq(result.prefix, aPrefix);
> +  } else {
> +    do_check_eq(result, undefined);

Assert.strictEqual is likely more precise when comparing with null or undefined (I may misremember but I think do_check_eq(null, "") passes).

In general new code should use Assert, rather than the old do_check_ helpers, but that's a nit.
Attachment #8864135 - Flags: review?(mak77) → review+
Comment on attachment 8864136 [details]
Bug 1341350 - Change the hosts prefix algorithm to prefer secure urls, but also to be able to swap back and forth depending on typed entries in history.

https://reviewboard.mozilla.org/r/135826/#review139118

please test on Try, other tests may rely on the autofill behavior.

::: toolkit/components/places/nsPlacesTriggers.h:77
(Diff revision 1)
>   *  - otherwise don't use any prefix
>   */
>  #define HOSTS_PREFIX_PRIORITY_FRAGMENT \
>    "SELECT CASE " \
>      "WHEN 1 = ( " \
> -      "SELECT min(substr(url,1,12) = 'https://www.') FROM moz_places h " \
> +      "SELECT cast(round(avg(substr(url,1,12) = 'https://www.')) AS integer) FROM moz_places h " \

I think that we don't need the CAST, since:
SELECT round(0.5) = 1 => 1

and probably we also don't need the "1 =", since 
SELECT CASE WHEN round(0.4) THEN "true" ELSE "false" END (try with 0.5 to get "true").

::: toolkit/components/places/tests/unit/test_hosts_triggers.js:222
(Diff revision 1)
>    yield PlacesTestUtils.addVisits(places);
>  
>    checkHostInMozHosts(TEST_URI_1, true, "https://www.");
>  });
>  
> -function getTestSection(baseURL1, baseURL2, extra) {
> +function getTestSection(baseURL1, baseURL2, baseURL2Prefix, extra, ) {

extra comma at the end?
Attachment #8864136 - Flags: review?(mak77) → review+
Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4a0e9b01d69d
Add more tests to cover updating of host prefixes when new results are added to the places table. r=mak
https://hg.mozilla.org/integration/autoland/rev/7e9c97247ba0
Change the hosts prefix algorithm to prefer secure urls, but also to be able to swap back and forth depending on typed entries in history. r=mak
https://hg.mozilla.org/mozilla-central/rev/4a0e9b01d69d
https://hg.mozilla.org/mozilla-central/rev/7e9c97247ba0
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Release Note Request (optional, but appreciated)
[Why is this notable]: We've been working on improving suggestions for autofill entries to favour https more frequently. 
[Affects Firefox for Android]: No
[Suggested wording]: When entering a host name in the URL bar, Firefox is now more likely to automatically suggest visiting a secure host.
[Links (documentation, blog post, etc)]:
relnote-firefox: --- → ?
While this sounds like a nice improvement, I'm not completely clear on why this is release notes worthy?
It plays into our security and privacy strengths, but it's OK if you think it's not relnote-worthy.
As a side note, this was a pretty common bug report from our users, the fact the location bar looked like going to the insecure version when they had visited the secure version before. Now we both select https more often AND we show that in the location bar.
"When entering a hostname (like pinterest.com) in the URL bar, Firefox resolves to the secure version of the site (https://www.pinterest.com) instead of the insecure version (http://www.pinterest.com) when possible" is part of https://www.mozilla.org/en-US/firefox/55.0/releasenotes/
relnote-firefox: ? → 55+
See Also: 1060546 →
See Also: → 1812192
Blocks: https-everything
No longer blocks: 1352847
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: