HTTP passwords should be used on the HTTPS version of the same domain

RESOLVED FIXED in Firefox 49

Status

()

--
enhancement
RESOLVED FIXED
8 years ago
2 months ago

People

(Reporter: steevo, Assigned: MattN)

Tracking

(Blocks: 3 bugs, {sec-want})

Trunk
mozilla49
sec-want
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox44- disabled, firefox45- wontfix, firefox46 wontfix, firefox47 affected, firefox48 affected, firefox49 fixed)

Details

(Whiteboard: security:passwords, [fxprivacy][adv-main49-], URL)

Attachments

(3 attachments, 9 obsolete attachments)

58 bytes, text/x-review-board-request
Dolske
: review+
Details
58 bytes, text/x-review-board-request
Dolske
: review+
Details
58 bytes, text/x-review-board-request
Dolske
: review+
Details
(Reporter)

Description

8 years ago
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2.16) Gecko/20110319 Firefox/3.6.16 (.NET CLR 3.5.30729)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2.16) Gecko/20110319 Firefox/3.6.16 (.NET CLR 3.5.30729)

This site has apparently changed some of their site to HTTPS. I had used it and have saved username/pw as http://www.jobster.com/ in pw manager. 

Now when logging in I am taken to an HTTPS page https://www.jobster.com/
and pw is not inserted.  There is no reason why pw manager would have two entries for one site, one with and one without SSL. 


Reproducible: Always

Steps to Reproduce:
1. Go to site http://www.jobster.com/
2. Click login
3. You get https://www.jobster.com/
4. pw manager has credentials for http://www.jobster.com/ but does not insert them in https site, which is the same. 


Actual Results:  
I have to go to pw manager, determine what username and pw are valid for this site and type them.  They are now saved both with and without https. 

I'm glad the mozilla pw manager is so insecure, if this were IE we'd have no idea what the pw was.  

Expected Results:  
This site is the same, and because it has been changed to ssl the pw manager should be smart enough to offer to insert my credentials. 

This is not nearly the only instance of this.  I have many others and I will be posted test cases on this bug.  

The password manager is broken. It worked great a year or so ago. It's getting much worse as time goes on. 

I have many instances of pw manager entries that are the same but with some load balancing.  PW manager should offer to insert pw when the underlying site is the same.
(Reporter)

Updated

8 years ago
Hardware: x86 → All
Version: unspecified → Trunk
Fwiw, how do other (non Mozilla) browsers handle this case?

Importance: major -> minor, fttb.
(It's not like you can not ("easily") access the (new) site.)
Severity: major → minor
OS: Windows XP → All
Comment hidden (advocacy)
(In reply to comment #2)
> I completely disagree about minor or major.  PW manager is a major feature. 
> It's broken.  It's been getting worse and worse for a year until now I have
> many instances where it fails.

The workaround is to re-enter your password, once only, on the HTTPS site, and tell the PW manager to remember it. It will then be filled-in again in the future. This fulfils the second criterion for a "minor" bug: "other problem where easy workaround is present".
Severity: major → minor
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Reporter)

Comment 4

8 years ago
(In reply to comment #3)
> (In reply to comment #2)
> > I completely disagree about minor or major.  PW manager is a major feature. 
> > It's broken.  It's been getting worse and worse for a year until now I have
> > many instances where it fails.
> 
> The workaround is to re-enter your password, once only, on the HTTPS site,
> and tell the PW manager to remember it. It will then be filled-in again in
> the future. This fulfils the second criterion for a "minor" bug: "other
> problem where easy workaround is present".
Tony, 
I have many instances of pw manager saving passwords that are not later filled in automatically, that was said to be an autocomplete bug. But it does refer to passwords.  

So what you say is not true in my experience.  Even using the save password bookmarklet from squarefree.com though it does cause the pw to be saved in some instances it is not filled in later.  

I suspect this is a ploy being used by some web developers to avoid saving passwords and break the password manager in a false attempt to increase security.  With all the spoof and phishing pages I have had taken down and the 10s of thousands of emails about those that I have analyzed typing passwords repeatedly is a fools errand.
(Reporter)

Comment 5

8 years ago
Right now, I go to Jobster.com by typing that in the url box. 
I end up at 
http://jobster.com/
I click login. 
I get a login box at 
http://jobster.com/login

I click the box that says Email Address
A dropdown appears with 11 entries. 
I look in my password manager, and there are two entries. 
http://jobster.com/
and 
https://jobster.com/
Each has the same email address and pw. 

The one email that is valid for Jobster is among the 11 presented. 
I select the correct email. 
The pw is not filled in. 

Clearly this data for this login is not coming from the pw manager. 

Where is it coming from and why isn't it coming from where it's designed to come from? 

Why does the pw manager not contain the data that is presented when I login? 

This feature is broken and I have a hundred test cases. The autocomplete bug I also posted on has been languishing since 2008.  It's getting worse.  Much worse.
Severity: minor → major
(Reporter)

Comment 6

8 years ago
Another test case. 

https://twlax.convergentcare.com/
I click login

https://twlax.convergentcare.com/twlax/validateSelfCareCustomer.do?action=initialize

I have an entry in password manager that reads
https://twlax.convergentcare.com/

With saved username and pw. 

But on the site it is not filled in. I have to go to pw manager, read what the pw is and type it in on the page. 

I used the remember password bookmarklet from squarefree.com. 
It said removed autocomplete from 4 form elements and offered to save. 
I said yes. 

Now, I have still only the one entry in password manager. 
This might be a different issue since it is supposed to save and the bar at the top asked, but there is nothing there.
(Reporter)

Comment 7

8 years ago
I just saw another test case like in comment 5. 

https://www.terapeak.com appears in pw manager. 
When I clicked login, I found myself at 
https:/data.terapeak.com

PW is not filled in.  
I click the login box and I get 11 or 12 entries, none of which are the correct pw for https://www.terapeak.com or for https://data.terapeak.com. The correct credentials are in pw manager but are not filled in.  I have no idea where the 11 or 12 invalid, unrelated entries in the pulldown came from, but not from password manager. 

I type the correct credentials and they are offered to save. Now I have two entries for this site in pw manager

https://www.terapeak.com
https://data.terapeak.com

I click logout. 
I am at 
http://www.terapeak.com/
I click login
I now find myself at 
https://data.terapeak.com/verify/
THe credentials that I typed today are filled in but clicking the box results in the pulldown with a whole bunch of invalid usernames for this site. 

I click back. 
I click login. 
I am back at 
https://data.terapeak.com/verify/
and the username and pw are filled in correctly with the data from the pw manager but now clicking does not result in any pulldown.  

What changed?
(Reporter)

Updated

8 years ago
Summary: Passwords are saved in password manager, but site now uses HTTPS and pw is not inserted → Passwords are saved in password manager, but site address is slightly different and pw is not inserted
(Reporter)

Comment 8

8 years ago
Bug 675834 may be more exact for this, the problem is likely worse than I thought.
Steve, do you still see this?  And bug 675834 and bug 667251?
Flags: needinfo?(steevo)
(Reporter)

Comment 10

5 years ago
I'm going to have to test, I have been using Lastpass, which works terribly, as a workaround for the broken firefox pw manager.
Flags: needinfo?(steevo)
(Quoting Jesse Ruderman from bug 986609 comment #1)
> Passwords saved for http should also work on https. Not doing this severely
> punishes sites that switch to https. Ideally we'd change the password to
> https-only at some point (when the form is submitted? when the site enables
> HSTS, a la bug 1119555?).
> 
> Passwords saved for https should NOT also work on http. That would enable
> really dangerous MITM attacks.

e.g. This recently affected all Reddit users.

When the domain isn't the same, that will be handled by bug 494593.
Flags: firefox-backlog+
Summary: Passwords are saved in password manager, but site address is slightly different and pw is not inserted → HTTP passwords should be used on the HTTPS version of the same domain
Duplicate of this bug: 986609
Keywords: sec-want

Updated

4 years ago
Blocks: 1118400

Comment 13

4 years ago
I think implementing a fix to this could create an unacceptable security vulnerability.  When a Web site is HTTPS, transactions are encrypted.  When a site is HTTP, there is no encryption.  Thus, the user ID and password under the prior HTTP might have been compromised by a third party, leaving HTTPS access open to that third party.  I strongly recommend (1) that a NEW password be used when a site changes from HTTP to HTTPS and (2) this bug report be marked WontFix.
(In reply to David E. Ross from comment #13)
> I think implementing a fix to this could create an unacceptable security
> vulnerability.  When a Web site is HTTPS, transactions are encrypted.  When
> a site is HTTP, there is no encryption.  Thus, the user ID and password
> under the prior HTTP might have been compromised by a third party, leaving
> HTTPS access open to that third party.
Yes, if you login to a site over HTTP your username and password are open for compromise.  But if https://a.com/login.html and http://a.com/login.html accepts the same username/password (which is pretty much always the case), then the attacker can take the HTTP credentials and use them on the HTTPS version of the site.  Likely they will have access to the same data/cookies either way.

Now assume a website migrates from HTTP to HTTPS.  The Password Manager has the user credentials stored for the HTTP version of the site.  The user goes to the website and their username and password isn't autofilled.  They are confused, why isn't it getting filled?  They navigate around and make their way to the HTTP version of the login page (which still exists but isn't the default anymore).  They login that way, leaving there credentials open to compromise.  If the Password Manager had instead made the credentials available to HTTPS, they would have logged in over the HTTPS version of the site.

Note that we are only talking about upgrading here!  If you have a saved HTTPS username/password for a given domain, we will not populate those credentials on the HTTP version of the same domain.

> I strongly recommend (1) that a NEW
> password be used when a site changes from HTTP to HTTPS
That would be great.  But that is up to the user and the website.  How can password manager control that?
(Reporter)

Comment 15

4 years ago
Tanvi is correct, David Ross is incorrect. 

David, it's up to the user to manage his security. 

The PW manager is fatally broken due to this type of thinking.  It's a somewhat technically correct but puritanical approach.  The same as some sites that expect you to change your PW often and enforce complexity, which forces users to either write it down (lower security) or do what I did, start using Lastpass, which works lousy. 

The PW manager should offer to insert credentials where appropriate, and if the user doesn't like that he can turn it off.  

Mozilla is about user empowerment. Not making everyone wear seatbelts, bicycle helmets and condoms all the time in the name of safety.
Priority: -- → P1

Comment 16

4 years ago
(In reply to Tanvi Vyas [:tanvi] from comment #14)
> Yes, if you login to a site over HTTP your username and password are open
> for compromise.  But if https://a.com/login.html and http://a.com/login.html
> accepts the same username/password (which is pretty much always the case),
> then the attacker can take the HTTP credentials and use them on the HTTPS
> version of the site.  Likely they will have access to the same data/cookies
> either way.

Tanvi Vyas describes a security vulnerability created by the Web site, not by the user or the user's Mozilla application.  To auto-fill when the URI changes from HTTP to HTTPS without the user updating the user ID and password in Password Manager for the changed URI would be a security vulnerability created by the user's Mozilla application.
(Reporter)

Comment 17

4 years ago
Maybe there should be a checkbox, allow pw manager on HTTP sites, HTTPS sites, or both. 
Then the user can migrate to HTTPS and disable pw manager for HTTP. 

That fits the Mozilla mission of putting the user in control. 

Ah, it's nonsensical, the pw manager is broken, has been for ages and it will be broken going forward.  I wish Lastpass worked better, it works horribly.
If we are still honouring formSubmitURL at the time this is implemented (see bug 1121119), I think we should also support an upgrade in the formSubmitURL scheme.
(In reply to Matthew N. [:MattN] from comment #18)
> If we are still honouring formSubmitURL at the time this is implemented (see
> bug 1121119), I think we should also support an upgrade in the formSubmitURL
> scheme.

+1.  Yes, submitting passwords over HTTPS is far better then submitting in cleartext over HTTP.  If a user has a saved password on http://a.com with a form submit url of http://login.a.com, then we should allow the user to fill in the password if the form submit url changes to https://login.a.com.

The opposite is NOT true.  If the user has been submitting over https://login.a.com, we should not fill in the password for http://login.a.com.
> if https://a.com/login.html and http://a.com/login.html accepts the same username/password
> (which is pretty much always the case)

I'm a little uncomfortable with that since they are technically different origins. How often is it NOT the case? I don't suppose we have any data on whether users have different passwords saved for the http and https versions of a site--plus "last used" times because the difference might simply indicate a site that upgraded and then the user changed passwords sometime later. The data would be nearly impossible to interpret; I'm not saying we should actually set up telemetry probes for it.

We should at least be explicit that we will NEVER do this if either the saved http PW or the current https site have a non-default port. And as stated in previous comments, NEVER use a httpS pw on a matching insecure http site. A one-way ratchet.

If we do use the saved insecure password do we upgrade its origin to https? If the site has really switched then having the password still saved for http could leave those users vulnerable to an sslstrip-type attack That's especially true if we continue to autofill without user action, but even if user interaction is required the fact that we remember the password would lend believability to the phish. Do we create a new https entry? Do we simply leave the data as-is and take the upgrade path forever (and if we do, would we handle future password change forms correctly?)
One hint to know that it's probably safer to do an upgrade of the saved signon is if the page has HSTS setup.
(In reply to Daniel Veditz [:dveditz] from comment #20)
> > if https://a.com/login.html and http://a.com/login.html accepts the same username/password
> > (which is pretty much always the case)
> 
> I'm a little uncomfortable with that since they are technically different
> origins. How often is it NOT the case?

I think this is a technicality that is overwhelmed by this being such a common (and otherwise harmless) cause of broken password management. If a site is relying on users to treat the SSL version of the site as a different origin, that's already exceedingly fragile and poor design. It's unreasonable to expect users to know not to type in their http://a.com password in on https://a.com (the reverse is legit but already hard!), and I think we should just treat supporting that use-case as an explicit non-goal.

> We should at least be explicit that we will NEVER do this if either the
> saved http PW or the current https site have a non-default port.

I think that's reasonable. Especially because it seems fairly uncommon. If we find a need for this, I suspect we'd want to address it with the site-specific recipe mechanism we're adding.

> And as
> stated in previous comments, NEVER use a httpS pw on a matching insecure
> http site. A one-way ratchet.

Yes, worth restating. Although I'd note that we're specifically talking about autofill in this bug -- I think we should have UI that makes it possible for the user manually fetch an SSL login for a non-SSL page, if they're determined to.

> If we do use the saved insecure password do we upgrade its origin to https?

No. The scope of this bug is simply to allow using an existing http:// login on a https:// page.
(Reporter)

Comment 23

4 years ago
>The scope of this bug is simply to allow using an existing http:// login on a https:// page.

Exactly. 
IMHO if there is a login box on the screen and credentials exist in pw manager for that domain in any form at all the user should be able to insert them.  If the user cannot insert them, that is a very serious defect. 

This is to login, not to figure out who should do what, or whether something is safe or even good design.  

When I posted this bug I noticed that even though the proper credentials were actually stored in PW manager they were not being inserted reliably. So the user cannot login without having to troubleshoot what the problem is. I have the ability to do that troubleshooting but most people do not.  They should not have to learn that stuff. It's too much to ask an ordinary user. 

So the PW manager was and still is broken.  Since I reported this three and a half years ago more and more sites have changed to SSL for their login pages.  And guess what?  I have the username and pw, but it does not come out where expected. 

Broken. It just needs to be fixed, and just forget what other aspects of security might exist.  If the login box matches the domain, just put in the username/pw.  No more hassles.

Comment 24

4 years ago
> To auto-fill when the URI changes from HTTP to HTTPS without the user updating the user ID and
> password in Password Manager for the changed URI would be a security vulnerability created by the
> user's Mozilla application.

What would be the security vulnerability?


> I'm a little uncomfortable with that since they are technically different origins. How often is it
> NOT the case?

So http://a.com/login.html and https://a.com/login.html could be 2 totally different sites at the same time owned by different people?


> We should at least be explicit that we will NEVER do this if either the saved http PW or the current
> https site have a non-default port.

To my above: Does that mean there could be 2 different sites running for example one on port 80 for http://a.com/login.html and one on port 8080 for https://a.com/login.html ?
Summary: HTTP passwords should be used on the HTTPS version of the same domain → Breakdown: HTTP passwords should be used on the HTTPS version of the same domain
(In reply to Daniel Veditz [:dveditz] from comment #20)
> If we do use the saved insecure password do we upgrade its origin to https?
> If the site has really switched then having the password still saved for
> http could leave those users vulnerable to an sslstrip-type attack That's
> especially true if we continue to autofill without user action, but even if
> user interaction is required the fact that we remember the password would
> lend believability to the phish. Do we create a new https entry? Do we
> simply leave the data as-is and take the upgrade path forever (and if we do,
> would we handle future password change forms correctly?)

(In reply to Matthew N. [:MattN] from comment #21)
> One hint to know that it's probably safer to do an upgrade of the saved
> signon is if the page has HSTS setup.

See bug https://bugzilla.mozilla.org/show_bug.cgi?id=1119555.  We should probably move the discussion of what to do with the HTTP entry there.

Updated

4 years ago
Depends on: 1127418
Proposed approach:

In LoginManagerParent.findLogins (http://mxr.mozilla.org/mozilla-central/source/toolkit/components/passwordmgr/LoginManagerParent.jsm#102), if the provided formOrigin has an HTTPS scheme and uses the default port for HTTPS, first collect the union of stored logins for the formOrigin and the HTTP version of the formOrigin (with default port).    Then de-dup the list based on username. If we have an entry for both HTTP and HTTPS origins, and the same username with different passwords, we could:
  - choose the one most recently used (this gets wacky with Sync, since it doesn't sync the lastUsed timestamps), or
  - choose the HTTPS one

Return the result.
See Also: → bug 1127567

Comment 27

4 years ago
(In reply to Tanvi Vyas [:tanvi] from comment #14)
> 
> Now assume a website migrates from HTTP to HTTPS.  The Password Manager has
> the user credentials stored for the HTTP version of the site.  The user goes
> to the website and their username and password isn't autofilled.  They are
> confused, why isn't it getting filled?  They navigate around and make their
> way to the HTTP version of the login page (which still exists but isn't the
> default anymore).  They login that way, leaving there credentials open to
> compromise.  If the Password Manager had instead made the credentials
> available to HTTPS, they would have logged in over the HTTPS version of the
> site.
> 
> Note that we are only talking about upgrading here!  If you have a saved
> HTTPS username/password for a given domain, we will not populate those
> credentials on the HTTP version of the same domain.

I have encountered several situations where a Web site login was upgraded from HTTP to HTTPS.  Not once did my user ID or password for the HTTP login work for the HTTPS login.  This RFE -- and this is indeed a request for enhancement and not a discprepancy -- would encourage a bad and unsecure pratice.
Severity: major → enhancement
(Reporter)

Comment 28

4 years ago
I have encountered hundreds of situations where the login was changed slightly, like from HTTP to HTTPS and the pw is not inserted by the PW Manager.  

Or a slight subdomain change, sometimes from ham handed load balancing.

I was able to insert the credential manually in every case. But that is a PW manager failure, which is why I reported this bug.  

IMHO 
http://w.domain.com 
http://a.domain.com 
and 
https://w.domain.com 
should be equivalent if a login box is being presented to prompt user login. 

And don't tell me that "load balancing URL changes like that should not be exposed to the user".  
I admit that, but we must use the internet as it is.  We can huff and puff, but ultimately we are not making the rules. 

We just want things to work.  To ALWAYS work.

Comment 29

4 years ago
> IMHO 
> http://w.domain.com 
> http://a.domain.com 
> and 
> https://w.domain.com 
> should be equivalent if a login box is being presented to prompt user login.

In my opinion this should not be the case with http://w.domain.com and http://a.domain.com as it is not uncommon that the owner of domain.com does give away subdomains to different users.
This bug is specifically about the http://site.com --> https://site.com case.

In other bugs (eg 494593, 589628) we'll be looking at how to support using logins from another subdomain/domain on a site. That's offtopic for this bug, but I expect the union of the two will be to make it easier to use http://a.site.com on https://b.site.com while still respecting the various security issues involved.
Summary: Breakdown: HTTP passwords should be used on the HTTPS version of the same domain → HTTP passwords should be used on the HTTPS version of the same domain
Removing "Breakdown". Since this is a historical bug with many watching, we'll just land the fix here.
> If we have an entry for both HTTP and HTTPS origins, and the same username with different passwords, we could:
>  - choose the one most recently used (this gets wacky with Sync, since it doesn't sync the lastUsed timestamps), or
>  - choose the HTTPS one

We decided to use the most recently updated one.
Assignee: nobody → ally
Blocks: 1134941
No longer blocks: 1118955
Assignee: ally → tanvi
Proposed policy for how to handle fill and capture for HTTP and HTTPS records with the same username and domain:
https://etherpad.mozilla.org/same-domain-http-https-passwords4
(In reply to Tanvi Vyas [:tanvi] from comment #33)
> Proposed policy for how to handle fill and capture for HTTP and HTTPS
> records with the same username and domain:
> https://etherpad.mozilla.org/same-domain-http-https-passwords4

The policy covers fill and capture for a wide variety of HTTP/HTTPS combinations.

Since this bug title specifically states 'HTTP passwords should be used on the HTTPS version of the same domain", adding a patch that does exactly that and no more.

I will file separate bugs to implement other parts of the policy.

Note that this bug may be dependent on capture on an HTTPS page when an HTTP login exists with the same password.  Since the HTTP login was likely autofilled because of this patch, we'd like to save a new HTTPS entry without prompting.  If we land this patch before that one is implemented, then we will still prompt when we'd rather not.  (See line 62 of the etherpad for more details; I'll likely create a wiki for this soon instead of using etherpad).

Who should review this?
Comment on attachment 8604870 [details] [diff] [review]
autofill-HTTP-on-HTTPS-05-12-15.patch

Review of attachment 8604870 [details] [diff] [review]:
-----------------------------------------------------------------

Could you include more lines of context in your patch or ideally use mozreview? 8 lines of context is recommended for patch files: https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

I think the PWMGR_FORM_ACTION_EFFECT code at the bottom of this method will need to be updated or it will be inaccurate after this change.

It seems like we would want similar logic in the autocomplete search code so some of the logic should be moved to a helper.

::: toolkit/components/passwordmgr/LoginManagerParent.jsm
@@ +273,3 @@
>      if (!allLoginsCount) {
> +      // if we are on an HTTPS page and didn't find an HTTPS login, see if
> +      // there is an HTTP login that we can return instead.

Why don't we consider the HTTP passwords even if there are some HTTPS ones found? This would be for a consistent experience in that we don't autofill when we have > 1 logins returned from the parent. Currently if I get autofill it usually means that I only have one saved login but with this change it could mean only 1 of my logins for the host are for HTTPS which may lead me to have a harder time accessing the other login for this site which hasn't been upgraded to HTTPS yet.

It doesn't seem like this is covered in https://etherpad.mozilla.org/same-domain-http-https-passwords4

@@ +276,5 @@
> +      let allHttpLoginsCount = 0;
> +      let httpFormOrigin;
> +      if (formScheme == "https:" && formPort == "") {
> +        // we want to use the default port 80 for http so we don't need to specify it.
> +        httpFormOrigin =  new URL("http://" + formHost).origin;

Nit: extra space after `=`

@@ +285,5 @@
> +        */
> +
> +        allHttpLoginsCount = Services.logins.countLogins(httpFormOrigin, "", null);
> +      }
> +      if (allHttpLoginsCount) {

If I understand the indentation properly, allHttpLoginsCount will always be undefined since its defined in the scope of the `if (!allLoginsCount) {`.

Nit: newline before the `if`

@@ +286,5 @@
> +
> +        allHttpLoginsCount = Services.logins.countLogins(httpFormOrigin, "", null);
> +      }
> +      if (allHttpLoginsCount) {
> +        log("There are no HTTPS logins for this domain, but there are",allHttpLoginsCount,"HTTP logins.");

Nit: include a space after both commas
Attachment #8604870 - Flags: feedback+
(In reply to Matthew N. [:MattN] from comment #35)
> Comment on attachment 8604870 [details] [diff] [review]
> autofill-HTTP-on-HTTPS-05-12-15.patch

Thanks Matt for your feedback.  I will work on an updated patch.

> ::: toolkit/components/passwordmgr/LoginManagerParent.jsm
> @@ +273,3 @@
> >      if (!allLoginsCount) {
> > +      // if we are on an HTTPS page and didn't find an HTTPS login, see if
> > +      // there is an HTTP login that we can return instead.
> 
> Why don't we consider the HTTP passwords even if there are some HTTPS ones
> found? This would be for a consistent experience in that we don't autofill
> when we have > 1 logins returned from the parent. Currently if I get
> autofill it usually means that I only have one saved login but with this
> change it could mean only 1 of my logins for the host are for HTTPS which
> may lead me to have a harder time accessing the other login for this site
> which hasn't been upgraded to HTTPS yet.
> 
> It doesn't seem like this is covered in
> https://etherpad.mozilla.org/same-domain-http-https-passwords4
> 
If we return both the HTTP and HTTPS logins, then if there is one HTTPS login and one or more HTTP logins, the user will get the autocomplete experience instead of the autofill one.  Moreover, the same username may show up twice in the autocomplete list, so we'd have to either correct for that or add UI to help users understand the difference.

We could define more heuristics:
* If there is one HTTPS login and one HTTP login with the same username, just return the HTTPS login.
* If there is more than one HTTPS login, then return both the HTTPS logins and the HTTP logins because the user is going to go through the autocomplete experience anyway.

But that only makes the code more complicated and confusing.  Instead, I propose we start by only offering HTTP logins if no HTTPS logins exist.  Then once all parts of https://etherpad.mozilla.org/same-domain-http-https-passwords4 are implemented, we can take a second pass and further optimize.

Thoughts?
Updated patch with a comment and question about how to handle the PWMGR_FORM_ACTION_EFFECT telemetry.

(In reply to Matthew N. [:MattN] from comment #35)
> @@ +285,5 @@
> > +        */
> > +
> > +        allHttpLoginsCount = Services.logins.countLogins(httpFormOrigin, "", null);
> > +      }
> > +      if (allHttpLoginsCount) {
> 
> If I understand the indentation properly, allHttpLoginsCount will always be
> undefined since its defined in the scope of the `if (!allLoginsCount) {`.

allHttpLoginsCount is defined inside 'if (!allLoginsCount {'.  'if (allHttpLoginsCount) {' is within that if block so it will still be defined here.

Other nits addressed.
Attachment #8604870 - Attachment is obsolete: true
Attachment #8605951 - Flags: review?(MattN+bmo)
(In reply to Tanvi Vyas [:tanvi] from comment #36)
> (In reply to Matthew N. [:MattN] from comment #35)
> > ::: toolkit/components/passwordmgr/LoginManagerParent.jsm
> > @@ +273,3 @@
> > >      if (!allLoginsCount) {
> > > +      // if we are on an HTTPS page and didn't find an HTTPS login, see if
> > > +      // there is an HTTP login that we can return instead.
> > 
> > Why don't we consider the HTTP passwords even if there are some HTTPS ones
> > found? This would be for a consistent experience in that we don't autofill
> > when we have > 1 logins returned from the parent. Currently if I get
> > autofill it usually means that I only have one saved login but with this
> > change it could mean only 1 of my logins for the host are for HTTPS which
> > may lead me to have a harder time accessing the other login for this site
> > which hasn't been upgraded to HTTPS yet.
> > 
> > It doesn't seem like this is covered in
> > https://etherpad.mozilla.org/same-domain-http-https-passwords4
> > 
> If we return both the HTTP and HTTPS logins, then if there is one HTTPS
> login and one or more HTTP logins, the user will get the autocomplete
> experience instead of the autofill one.  Moreover, the same username may
> show up twice in the autocomplete list, so we'd have to either correct for
> that or add UI to help users understand the difference.

Right.

> We could define more heuristics:
> * If there is one HTTPS login and one HTTP login with the same username,
> just return the HTTPS login.

This is what I had in mind.

> * If there is more than one HTTPS login, then return both the HTTPS logins
> and the HTTP logins because the user is going to go through the autocomplete
> experience anyway.

I think this adds to the confusion of when the user gets autofill vs. autocomplete.

> But that only makes the code more complicated and confusing.  Instead, I
> propose we start by only offering HTTP logins if no HTTPS logins exist. 
> Then once all parts of
> https://etherpad.mozilla.org/same-domain-http-https-passwords4 are
> implemented, we can take a second pass and further optimize.

I think it makes it more consistent (and therefore less confusing) with the deduping and I don't think the middle ground of confusing users who have multiple logins on a site (17% of hostnames with passwords over all users have more than one login) is a good one to expose to users. I don't think it's that much more work and I thought we were wanting the deduping anyways. If you want to land this behind a temporary pref or follow-up in the same release to address the deduping it's fine but I wouldn't ship without handling it.
Status: NEW → ASSIGNED
(In reply to Matthew N. [:MattN] from comment #38) 
> > * If there is more than one HTTPS login, then return both the HTTPS logins
> > and the HTTP logins because the user is going to go through the autocomplete
> > experience anyway.
> 
> I think this adds to the confusion of when the user gets autofill vs.
> autocomplete.
> 
What do you mean?  If there is more than one HTTPS login, the user will go through autocomplete.  So we might as well add in the HTTP logins (deduped).

Here are the cases to consider if we want to return both HTTP and HTTPS logins:

1) Only HTTPS logins exist
Easy, return all HTTPS logins (current patch does this)
2) Only HTTP logins exist
Easy, return all HTTP logins (current patch does this)

3) One HTTPS login exists and one HTTP login exists with the same username
Return the HTTPS login

4) One HTTPS login exists and one (or more) HTTP logins exist with a different username
Undecided.  If we return the HTTP logins along with the one HTTPS login, the user will experience autocomplete instead of autofill.  So either
i) Just return the HTTPS and user gets autofill
ii) Return the HTTP and HTTPS logins deduped and user gets autocomplete

5) More than one HTTPS login exists and one or more HTTP logins exist.
Return the HTTP and HTTPS logins deduped and user gets autocomplete

I still think it makes more sense to handle the full policy and then dive deeper to further optimize.
Added more context.
Attachment #8605951 - Attachment is obsolete: true
Attachment #8605951 - Flags: review?(MattN+bmo)
Attachment #8606046 - Flags: review?(MattN+bmo)
We may need to add more to this patch.  This works well when their is one login for the HTTP site.  But if there is more than one HTTP login and no HTTPS logins, the user doesn't get the autocomplete drop down for the username field.  If they manually fill the username, the HTTP password will be filled though, so that is good.

We need to add some logic to doAutocompleteSearch.

Comment 42

4 years ago
The HTTP Web sites that I visit generally do not involve anything more than a user-specific configuration and setup.  They do not involve financial transactions, personal data, or anything else that I want to keep secure.  The HTTPS Web sites that I visit generally do indeed involve financial transactions, personal data, and other items that I most definitely do want to keep secure.  Thus, my passwords for HTTP Web sites are often shorter and less secure than my passwords for HTTPS Web sites.  If an HTTP site changes to HTTPS because it has evolved to where security becomes important, I certainly do not want my less secure HTTP password used for HTTPS.

Updated

4 years ago
Blocks: 1166111
Comment on attachment 8606046 [details] [diff] [review]
autofill-HTTP-on-HTTPS-05-14-15B.patch

Clearing flag due to issues in comment 41.
Attachment #8606046 - Flags: review?(MattN+bmo)

Updated

4 years ago
Whiteboard: security:passwords
Blocks: 1167657
Rank: 15
Posted patch Bug667233-06-05-15.patch (obsolete) — Splinter Review
Attachment #8606046 - Attachment is obsolete: true
Posted patch Bug667233-06-05-15B.patch (obsolete) — Splinter Review
Attachment #8616282 - Attachment is obsolete: true
Comment on attachment 8616284 [details] [diff] [review]
Bug667233-06-05-15B.patch

A little verbose with comments and logging.  I can clean that up and remove those, but before I do I wanted a first pass review/feedback cycle.  This incorporates code for both this bug and bug 1127567, because implementation wise it did not make sense to separate these.  I'll mark that bug as a dup of this one.  I've added a few open questions inline.

This still needs tests.

Thanks!
Attachment #8616284 - Flags: review?(MattN+bmo)
To be clear, this bug is to implement the following:

Autofill HTTP on HTTPS if there is no HTTPS record:
If no HTTPS logins exist for an HTTPS page, see if any HTTP logins exist and return those.  If there is one login, it will be autofilled. If there are multiple logins the user can select one from the autocomplete drop down and then the password for that login will be filled.

Autocomplete HTTPS on HTTP if there is no HTTP record:
If no HTTP logins exist for an HTTP page, see if any HTTPS logins exist.  If one or more HTTPS login does exist, allow the user to select one from the autocomplete drop down and then the password for that login will be filled.

Updated

4 years ago
Duplicate of this bug: 1127567

Comment 49

4 years ago
Could the latter maybe be a problem? Comment #1 of bug #986609 stated this.
Comment on attachment 8616284 [details] [diff] [review]
Bug667233-06-05-15B.patch

Review of attachment 8616284 [details] [diff] [review]:
-----------------------------------------------------------------

Tanvi, if you want me to help with a rebase, I'm willing to do so with a 3-way merge tool. I can finish a review pass on this version tomorrow but it probably makes the most sense to review a version rebased over changes Bernardo and I have made to these files.

Btw., reviews are easier to do in MozReview as there is syntax highlighting and code movement markers. https://mozilla-version-control-tools.readthedocs.org/en/latest/mozreview-user.html

::: toolkit/components/passwordmgr/LoginManagerContent.jsm
@@ +19,5 @@
>  XPCOMUtils.defineLazyModuleGetter(this, "LoginRecipesContent",
>                                    "resource://gre/modules/LoginRecipes.jsm");
>  
>  // These mirror signon.* prefs.
> +var gEnabled, gDebug, gAutofillForms, gStoreWhenAutocompleteOff, gApplyProtocolPolicies;

FYI: this change will conflict with bug 1171348.

@@ +690,5 @@
> +  /**
> +   * Returns the username and password fields found in the form if they are
> +   * the right sizes.
> +   *
> +   * @param {HTMLFormElement} form

This should probably be a {FormLike} now. I think this patch is based on an older revision before my FormLike changes for formless logins.

@@ +710,5 @@
> +
> +    // Need a valid password field to do anything.
> +    if (passwordField == null) {
> +      log("not filling form, no password field found");
> +      recordAutofillResult(AUTOFILL_RESULT.NO_PASSWORD_FIELD);

I think AUTOFILL_RESULT is undefined here and I think that would mean that this code doesn't work as this would throw. Do tests pass or do you mostly want feedback on the approach?

Comment 51

4 years ago
Allowing dropdown-fill of HTTPS passwords on HTTP seems really dangerous to me. I don't engage my anti-phishing defenses unless I'm actually typing my password. And it might even be clickjackable.
Comment on attachment 8616284 [details] [diff] [review]
Bug667233-06-05-15B.patch

Review of attachment 8616284 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/passwordmgr/LoginManagerContent.jsm
@@ +318,5 @@
> +              } */
> +
> +              if (usernameField && logins.length) {
> +                log("Mark usernamefield");
> +                this._formFillService.markAsLoginManagerField(usernameField);

Add a comment about why you're marking here since someone may think it's a mistake that we're marking in 2 places.

::: toolkit/components/passwordmgr/LoginManagerParent.jsm
@@ +283,5 @@
> +   *
> +   * On an HTTP page: If an HTTP login isn't found, this method will return
> +   * any HTTPS logins that may exist for the same domain and same actionOrigin.
> +   * On an HTTPS page: If an HTTPS login isn't found, this method will return
> +   * any HTTP logins that may exist for the same domain and same actionOrigin.

I still think all HTTPS and HTTP logins should be returned so users aren't stuck not being able to see some of their accounts on a site where some are HTTP and others are HTTPS. What is your plan for addressing that?

@@ +319,5 @@
> +      // * if we are on an HTTPS page and didn't find an HTTPS login, see if
> +      // there is an HTTP login that we can return instead.
> +      // * if we are on an HTTP page and didn't find an HTTP login, see if
> +      // there is an HTTPS login that we can return instead.
> +      let allHttpLoginsCount = 0, allHttpsLoginsCount =0;

Nit: missing space after `=`

@@ +330,5 @@
> +        httpFormOrigin.replace(/^https:/, "http:");
> +        */
> +
> +        allHttpLoginsCount = Services.logins.countLogins(httpFormOrigin, "", null);
> +      } else if (formScheme = "http:" && formPort == "") {

I think you meant to use an equality operator instead of assignment.
Attachment #8616284 - Flags: review?(MattN+bmo) → feedback+

Updated

4 years ago
Blocks: 1174361
(In reply to Matthew N. [:MattN] from comment #52)
> I still think all HTTPS and HTTP logins should be returned so users aren't
> stuck not being able to see some of their accounts on a site where some are
> HTTP and others are HTTPS. What is your plan for addressing that?
> 
I filed bug https://bugzilla.mozilla.org/show_bug.cgi?id=1174361 for this.
Blocks: 1175279
Posted patch Bug667233-06-18-15B.patch (obsolete) — Splinter Review
Feedback comments addressed and patch cleaned up.  Matt, I have 3 questions for you inline.
Attachment #8616284 - Attachment is obsolete: true
Attachment #8624430 - Flags: review?(MattN+bmo)
(In reply to Jesse Ruderman from comment #51)
> Allowing dropdown-fill of HTTPS passwords on HTTP seems really dangerous to
> me. I don't engage my anti-phishing defenses unless I'm actually typing my
> password. And it might even be clickjackable.

If I understand the comments in this bug correctly, then I agree with Jesse.

I'd like to assume that if Firefox is willing to fill in my password, it's not going
to make my security worse, but taking HTTPS passwords and putting them in HTTP origins
seems to do that
(Reporter)

Comment 56

4 years ago
Eric, 
The pw manager has been broken for *years*.  
If you don't like it you can certainly turn it off and type passwords yourself.  

But think about this: Jesse mentions phishing.  

The danger in phishing is in typing passwords.  
For you, ebay.com.dddjkfjdskjsdfaasdfoijsdfaijasdfsdfioaosdiaf.com might look the same because of formatting, but firefox knows the difference.  
I have had users type their pw in just that type url. 

So even though people might type their pw, firefox will not.  It's not worse, not worse at all.
(In reply to Steve from comment #56)
> Eric, 
> The pw manager has been broken for *years*.  
> If you don't like it you can certainly turn it off and type passwords
> yourself.  

I'm not really understanding what you are saying here. Is your claim that
this behavior is the existing behavior?


> But think about this: Jesse mentions phishing.  
> 
> The danger in phishing is in typing passwords.  
> For you, ebay.com.dddjkfjdskjsdfaasdfoijsdfaijasdfsdfioaosdiaf.com might
> look the same because of formatting, but firefox knows the difference.  
> I have had users type their pw in just that type url. 
> 
> So even though people might type their pw, firefox will not.  It's not
> worse, not worse at all.
(Reporter)

Comment 58

4 years ago
No, this will probably work, and it will insert pw if the user wants ff to do so. 
As it's been, the passwords are not reliably inserted.  

Read the OP.
I have read the OP. I think the current behavior is largely correct. It might be OK to upgrade from HTTP->HTTPS but going the other way is a bad idea.
(Reporter)

Comment 60

4 years ago
That is an idealistic view, similar to the view that broke the android logging system. 
"Lets increase security so nothing works, even if the user wants it to work this way".

Would it be better if everything were encrypted?  Yes.  Is that the case today? No. 
Do you want the system to work, or to just fail inexplicably?  

Well, I am for making things work, as best we can.
And guess what, the Mozilla project is about empowering the user.

If you want no control over anything, just use IE.  Microsoft.com.
(In reply to Steve from comment #60)
> That is an idealistic view, similar to the view that broke the android
> logging system. 
> "Lets increase security so nothing works, even if the user wants it to work
> this way".
> 
> Would it be better if everything were encrypted?  Yes.  Is that the case
> today? No. 
> Do you want the system to work, or to just fail inexplicably?  
> 
> Well, I am for making things work, as best we can.
> And guess what, the Mozilla project is about empowering the user.
> 
> If you want no control over anything, just use IE.  Microsoft.com.

Obviously, I disagree with this characterization of the situation.

Comment 62

4 years ago
There is an easy solution: Implementing an option in about:config that controls this behavior.
Jesse, ekr, and others have expressed concerns about the HTTPS->HTTP downgrade.  They are worried that even though it requires user interaction, a user could easily be tricked into leaking passwords.

Example:
attacker gets user to visit http version of mail.com, where mail.com is not HSTS[1].  They can do this in various ways - intercepting ANY http connection, phishing emails, navigating a window.  Then one of two things can happen:

a) User doesn't notice that they aren't on the https version of mail.com.  They focus on the username field and click the down arrow twice and enter.  Their HTTPS password is filled in for them and that attacker reads it.  

b) The attacker changes the content of the HTTP page returned to the user (and perhaps focuses on the username field).  They convince the user to click the down arrow twice and then enter (can this happen when the password field is off screen?).

Access to the users credentials on mail.com then offers the attacker access to many other logins for that user.

One way to get around this threat is to add an option to the autocomplete drop down during a downgrade that says "login on the secure version of the website instead" (or something like that).  Clicking that will redirect the user to the https version of the domain.  We know the https version exists because the user has a password stored for it.  Any form data that the user may have set on the page will be gone, any ajax'y state changes that happened on the page will be lost.

These are our options:

1) Change the patch to only do the upgrade

2) Change the patch to only do the upgrade.  BUT, keep the downgrade code for reporting purposes.  This will give us data on what percentage of users/sites would have benefited from a downgrade.  If the benefit is high, then we revisit offering a downgrade.  (I'm actually not to keen on this option, because I'm not sure data that tells us this will help user experience will be strong enough to outweigh the security risks)

3) Add an autocomplete entry for "use the secure version instead".  This helps deter example b) but doesn't make it impossible.

Note that https://bugzilla.mozilla.org/show_bug.cgi?id=1179961 will be happening sometime in the next year (maybe this quarter, maybe not, but its not too far off).  Offering HTTPS logins on HTTP pages is kind of the opposite of what that bug is trying to achieve.

Also note that my time is quickly getting consumed by other things, so I'd like to make a decision on this and wrap it up as soon as possible.

[1] Most sites haven't adopted HSTS yet, https://www.veracode.com/blog/2014/10/security-headers-top-1000000-websites-october-2014-report
Comment on attachment 8624430 [details] [diff] [review]
Bug667233-06-18-15B.patch

Removing the r?.  Per comment 63 options 1-3, this patch needs to be tweaked.  I'm just not sure yet which option to tweak it to.

rbarnes, ekr, MattN, jesse, ckarlof - comments welcome!
Attachment #8624430 - Flags: review?(MattN+bmo)

Comment 65

4 years ago
I'd prefer #1 plus a variant of #3. As soon as the user focuses a username or password field, show an autocomplete dropdown with an entry: "[more] passwords saved for _secure site_".

For passive attacks, this will be better than the status quo, because users will have a clear alternative to remembering and typing their password on the insecure site. Nothing will "fail inexplicably" except during attacks or on extremely broken sites.
(In reply to Jesse Ruderman from comment #65)
> I'd prefer #1 plus a variant of #3. As soon as the user focuses a username
> or password field, show an autocomplete dropdown with an entry: "[more]
> passwords saved for _secure site_".

Is this meant to be an offer to reload the page as HTTPS?  I don't think that wording conveys that.

If we do something like that, it should notice when the page has redirected from HTTPS to HTTP and change the wording / make the offer vanish.  Would need user testing.
Somewhat relevant to some of the attacks mentioned in this bug - 
https://thehackerblog.com/stealing-lastpass-passwords-with-clickjacking/

Firefox's Password Manager UI can't be clickjacked; it is chrome UI.  But the attacker could deploy techniques to trick the user into selecting a username.
Given the feedback and our plan to deprecate HTTP (https://blog.mozilla.org/security/2015/04/30/deprecating-non-secure-http/), I don't think we should do the downgrade.  I'm going to change this patch to only do the upgrade.

If folks are still interested in a downgrade, please file another bug for it and we can continue the debate there.

I will file another bug to add a drop down autocomplete option to "Use secure version instead" when the Password Manager has an HTTPS entry for an HTTP hostname.
Updated patch so that it just does the upgrade (HTTP logins can autofill or autocomplete on HTTPS pages when no HTTPS logins exist) and rebased.

This needs a couple tests that perhaps someone on the password manager team can pick up:
1 HTTP login exists, 0 HTTPS logins exist - HTTP login is autofilled on HTTPS
2 HTTP login exists, 0 HTTPS logins exist - HTTP logins can be autocompleted on HTTPS
1 HTTP login exists, 1 HTTPS login exists - HTTPS login is autofilled on HTTPS
1 HTTP login exists, 2 HTTPS login exists - autocomplete UI just has the two HTTPS logins in autocomplete UI.


If you want to provide both the HTTP and HTTPS logins on an HTTPS page and deduplicate, that needs to happen in another bug.
Attachment #8648270 - Flags: review?(MattN+bmo)

Updated

4 years ago
Attachment #8624430 - Attachment is obsolete: true
Comment on attachment 8648270 [details] [diff] [review]
autofill-HTTP-on-HTTPS-08-14-15.patch

Review of attachment 8648270 [details] [diff] [review]:
-----------------------------------------------------------------

Thank you Tanvi. I had a look at the patch and thought about giving some feedback.

::: toolkit/components/passwordmgr/LoginManagerParent.jsm
@@ +301,5 @@
> +    let allHttpLoginsCount = 0;
> +    if (!allLoginsCount && Services.prefs.getBoolPref("signon.applyProtocolPolicies")) {
> +      // Upgrade scheme - If we are on an HTTPS page and didn't find an HTTPS
> +      // login, see if there is an HTTP login that we can return instead.
> +      if (formScheme == "https:" && formPort == "") {

Do we really want to check for formPort here? I don't see why using a non-standard port should not allow us to fallback to http.

@@ +382,5 @@
> +    // HTTP origin (or vice versa), we shouldn't report the telemetry. (MattN - is that okay?)
> +    // Because the form action may be different on the HTTP version of the
> +    // page. Reporting that the number of logins reduced after we downgraded
> +    // the scheme will make it appear as if more logins failed because of
> +    // non-matching form origins.

I think that as we only do fallback upgrades for now, it would make sense to report this telemetry anyways. 
Filtering by form action would still be limiting the amount of useful results the user may get.
Measurements may go a bit up, but we don't want to hide these new results. This could end hiding useful information for us.

If that's not the case and we really don't want to report upgrades, we should think about passing the original formOrigin when we defer because of the master password. (line 356)
Attachment #8648270 - Flags: feedback+
Thanks for the feedback Bernardo!

(In reply to Bernardo Rittmeyer [:rittme] from comment #70)
> ::: toolkit/components/passwordmgr/LoginManagerParent.jsm
> @@ +301,5 @@
> > +    let allHttpLoginsCount = 0;
> > +    if (!allLoginsCount && Services.prefs.getBoolPref("signon.applyProtocolPolicies")) {
> > +      // Upgrade scheme - If we are on an HTTPS page and didn't find an HTTPS
> > +      // login, see if there is an HTTP login that we can return instead.
> > +      if (formScheme == "https:" && formPort == "") {
> 
> Do we really want to check for formPort here? I don't see why using a
> non-standard port should not allow us to fallback to http.
> 
See line 223 here - https://etherpad.mozilla.org/same-domain-http-https-passwords4

> @@ +382,5 @@
> > +    // HTTP origin (or vice versa), we shouldn't report the telemetry. (MattN - is that okay?)
> > +    // Because the form action may be different on the HTTP version of the
> > +    // page. Reporting that the number of logins reduced after we downgraded
> > +    // the scheme will make it appear as if more logins failed because of
> > +    // non-matching form origins.
> 
> I think that as we only do fallback upgrades for now, it would make sense to
> report this telemetry anyways. 
> Filtering by form action would still be limiting the amount of useful
> results the user may get.
> Measurements may go a bit up, but we don't want to hide these new results.
> This could end hiding useful information for us.
> 
> If that's not the case and we really don't want to report upgrades, we
> should think about passing the original formOrigin when we defer because of
> the master password. (line 356)

I was worried that the http and https version of the site would have different form actions.  If that is the case, the http login wouldn't match and make the telemetry show that more logins were not filled because formAction didn't match for the page.  When that is not completely true.  On the other hand, the case I was thinking about most was http formAction vs https formAction, and you have taken care of that in https://bugzilla.mozilla.org/show_bug.cgi?id=1147561.   So if we are okay with this change skewing our metric, that is fine and hopefully it won't make a huge difference.  I'll wait for Matt's review before changing this though, to make sure he's okay with it too.
Comment on attachment 8648270 [details] [diff] [review]
autofill-HTTP-on-HTTPS-08-14-15.patch

Review of attachment 8648270 [details] [diff] [review]:
-----------------------------------------------------------------

As I said before, I don't think that duplicating the fallback logic for each consumer of the pwmgr storage is a maintainable solution. I think we should do this logic in the storage code by providing an argument to the various methods to indicate that inexact matches are allowed. This would apply to findLogins, searchLogins, and countLogins.

Example: ```
findLogins({}, "https://example.com", "https://example.com", null, {
  schemeUpgrades: true, // Indicate that logins for HTTP (origin and action) should be included for HTTPS forms
  relatedOrigins: true, // For bug 1200472
});
```

Also, I don't think we should only use the looser search as a fallback for most UI and instead always retrieve applicable logins then use the dedupe function that rittme wrote before using the result.
Attachment #8648270 - Flags: review?(MattN+bmo) → review-

Updated

3 years ago
Assignee: tanvi → nobody
Status: ASSIGNED → NEW

Updated

3 years ago
Whiteboard: security:passwords → security:passwords, [fxprivacy]
Rank: 15
Flags: firefox-backlog+
Priority: P1 → --
Whiteboard: security:passwords, [fxprivacy] → security:passwords, [fxprivacy] [triage]
Priority: -- → P3
Whiteboard: security:passwords, [fxprivacy] [triage] → security:passwords, [fxprivacy]
Given that Firefox will soon mark password fields on http websites as insecure, big websites are migrating over to https. A big webmail provider did test runs and got increased hotline calls (in that particular target group) why the Firefox password manager doesn't work anymore. This is bad enough that they are looking for a solution.

The fact that Firefox doesn't upgrade http passwords to https actually acts as a deterrent for big websites to deploy https. Even if it doesn't stop them, it causes a problem for their and our users.

Thus, this bug is important to be fixed NOW. The websites need to migrate to https only before Firefox ships, and this bug here needs to be implemented before these websites migrate to https. There should be a few months in between this fix here ships and before Firefox marks password forms on http insecure.
Comment 63:
> attacker gets user to visit http version of mail.com

Coincidentally, this is exactly the company I work for and that has the concrete problem mentioned in my last comment.

I concur with comment 63 that we should not downgrade from http to https. But we need to upgrade from http to https. This is a notable problem for mail.com and other, even larger websites in the migration to https.

Comment 75

3 years ago
[Tracking Requested - why for this release]:

BenB is right. Bug 1179961 seems to be targeted at Firefox 45 so this should be too.
tracking-firefox45: --- → ?

Comment 76

3 years ago
(In reply to Ben Bucksch (:BenB) from comment #73)
> The fact that Firefox doesn't upgrade http passwords to https actually acts
> as a deterrent for big websites to deploy https. Even if it doesn't stop
> them, it causes a problem for their and our users.

I agree that supporting the HTTP to HTTPS migration of saved passwords is very important, and it should land way in advance of when the insecure passwords warning goes to Beta and Release.

This does not block the activation of the warning in the Developer Edition only, which is our immediate target.

Updated

3 years ago
Blocks: 1217142
[Tracking Requested - why for this release]:

Jesse wrote:
> BenB is right. Bug 1179961 seems to be targeted at Firefox 45 so this should be too.

Thank you. Actually, I was saying that this here must be implemented at least one Firefox release before the "password is insecure" check, for this reason:

I wrote:
> The websites need to migrate to https-only *before* Firefox [with insecure password warning]
> ships, and this bug here needs to be implemented before these websites migrate to https.
> There should be a few months [or at least weeks] in-between this fix here ships and before
> Firefox marks password forms on http insecure.

In other words, if bug 1179961 targets FF45, this here needs to target FF44.
(Or if this here targets FF45, then bug 1179961 should be delayed until FF46 or later. There's no concrete time plan mentioned for bug 1179961, so I don't know.)
tracking-firefox44: --- → ?

Updated

3 years ago
Assignee: nobody → neil
Posted patch First attempt (obsolete) — Splinter Review
To get the ball rolling, this is a simple patch that makes passwords saved with HTTP sites or form actions appear when queried against the equivalent HTTPS site or action.

Sites with port numbers are ignored and no fallback is attempted in this case.

If somehow you've previously saved passwords for both versions of the site then you'll get duplicates. I can't do anything about that, because this function is so low-level it doesn't actually know the username you used.

If I've read the code correctly we don't try to eagerly decrypt the username unless we have exactly one, so as to avoid unnecessarily prompting the user for the master password. There is therefore a chicken-and-egg problem where we don't know whether the http and https saved logins are in fact duplicate; we would have to prompt for the master password to find out that we shouldn't...
Attachment #8697310 - Flags: feedback?(MattN+bmo)
Comment on attachment 8697310 [details] [diff] [review]
First attempt

Review of attachment 8697310 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the patch Neil and sorry for the delay. I believe there are some consumers (which deal with editing and deciding whether to offer a remember or update) that need strict matching and that's why I suggested adding an argument in comment 72.


Also please put this new functionality behind a pref since bug 1127579 will need to be addressed before this is enabled by default.

::: toolkit/components/passwordmgr/storage-json.js
@@ +283,4 @@
>            case "formSubmitURL":
> +            if (value != null && loginValue != null && loginValue != value &&
> +                !/^https:\/\/[^\/]+:/.test(value) &&
> +                loginValue == value.replace(/^https:/, "http:")) {

We tried to avoid using regexes since we have nsIURI which already knows about schemes.

Does this properly handle "" (wildcard) and "javascript:"?

I think keeping this as multiple conditions makes it easier to read and comment.
Attachment #8697310 - Flags: feedback?(MattN+bmo) → feedback+
[Tracking Requested - why for this release]:
In bug 1221206, it is said that we might do something in fx 46. Until then, not tracking.
status-firefox44: --- → disabled
status-firefox45: --- → wontfix
status-firefox46: --- → affected
tracking-firefox44: ? → -
tracking-firefox45: ? → -
tracking-firefox46: --- → ?

Comment 81

3 years ago
For web site owners trying to switch from a non-SSL/SSL setup to SSL only, this bug still is mission critical to do that switch as at least 50% of our users use firefox. As mentioned by Ben Bucksch this Bug should be live to the users one version earlier as https://bugzilla.mozilla.org/show_bug.cgi?id=1221206.
Tracking this for 46, looks like it may slip to 47 though. We need to evaluate and make a decision before the end of next week for the Jan. 25 merge date for 46 to go to aurora.
tracking-firefox46: ? → +
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #82)
> Tracking this for 46, looks like it may slip to 47 though. We need to
> evaluate and make a decision before the end of next week for the Jan. 25
> merge date for 46 to go to aurora.

This likely won't make Firefox 46.
Blocks: 1193404
Neil, are you fine with me taking this bug? I can flag you for feedback/review.
Flags: needinfo?(neil)
Sorry, I never saw your feedback+ for some reason, so I thought it was still outstanding.

(In reply to Matthew N. from comment #79)
> (From update of attachment 8697310 [details] [diff] [review])
> Thanks for the patch Neil and sorry for the delay. I believe there are some
> consumers (which deal with editing and deciding whether to offer a remember
> or update) that need strict matching and that's why I suggested adding an
> argument in comment 72.
Sorry, I don't understand what the problem is here; if you are editing then you already have an existing login object.

> > +            if (value != null && loginValue != null && loginValue != value &&
> > +                !/^https:\/\/[^\/]+:/.test(value) &&
> > +                loginValue == value.replace(/^https:/, "http:")) {
> 
> We tried to avoid using regexes since we have nsIURI which already knows
> about schemes.

Except that login manager doesn't really deal in URIs. (Except for the weird code that does fuzzy matching of form submit URLs.)

> Does this properly handle "" (wildcard) and "javascript:"?
Well, it ignores them, since it's only concerned with URLs that differ only in HTTP <=> HTTPS scheme with default ports.

(In reply to Matthew N. from comment #84)
> Neil, are you fine with me taking this bug? I can flag you for feedback/review.
Sure, whatever gets the job done the fastest.
Flags: needinfo?(neil)
Assignee: neil → MattN+bmo
Status: NEW → ASSIGNED
(In reply to Ben Bucksch (:BenB) from comment #77)
> [Tracking Requested - why for this release]:
> 
> Jesse wrote:
> > BenB is right. Bug 1179961 seems to be targeted at Firefox 45 so this should be too.
> 
> Thank you. Actually, I was saying that this here must be implemented at
> least one Firefox release before the "password is insecure" check, for this
> reason:
> 
> I wrote:
> > The websites need to migrate to https-only *before* Firefox [with insecure password warning]
> > ships, and this bug here needs to be implemented before these websites migrate to https.
> > There should be a few months [or at least weeks] in-between this fix here ships and before
> > Firefox marks password forms on http insecure.
> 
> In other words, if bug 1179961 targets FF45, this here needs to target FF44.
> (Or if this here targets FF45, then bug 1179961 should be delayed until FF46
> or later. There's no concrete time plan mentioned for bug 1179961, so I
> don't know.)

Now, bug 1179961 has been fixed for FF44, while this one here still doesn't have a patch even committed.

I just spoke to the Product Manager, and 3 of the biggest German websites are very concretely holding back on the SSL migration because of this bug here. They feel they *cannot* make the switch until this is fixed. It would incur massive calls to the hotline. They know, because they made a test with a test group.

It's ironic that Firefox itself is stopping the migration to SSL, while Firefox tries to punish websites for nor migrating in bug 1179961. That doesn't make sense.

Please:
* Get this here fixed. I paid Neil to create a patch, still no further action here.
* Please back out bug 1179961 out of FF44, FF45 and until at least one release *after* this bug here is fixed, so that the websites have at least 6 weeks to make the transition.

Comment 87

3 years ago
I've commented in bug 1179961 to clarify that the warning there applies to the Developer Edition only.
Posted patch Second attempt (obsolete) — Splinter Review
Attachment #8716874 - Flags: feedback?(MattN+bmo)
Comment on attachment 8716874 [details] [diff] [review]
Second attempt

Thanks Neil, for the new patch. We're seeking review, so marking r?
(And thanks Paolo, for clearing up my misunderstanding.)
Attachment #8716874 - Flags: review?(MattN+bmo)
Comment on attachment 8716874 [details] [diff] [review]
Second attempt

I'll give feedback soon. I already had a WIP patch addressing the comments so now I'm going to need to reconcile the differences.
Attachment #8716874 - Flags: review?(MattN+bmo)
Hey Matt, feel free to just use your patch, if it works just as well as Neil's. We merely wanted to get this moving.

I agree with your review. Just one point:
> please put this new functionality behind a pref since bug 1127579 will need to be addressed
> before this is enabled by default.

The feature here must be enabled by default, otherwise it's useless for us. If it's off by default, it's essentially not fixed for our use case (website wanting to make sure their users' stored passwords are migrated before switching to https).

If there is a https password stored, that should be used and http passwords could be ignored. Bug 1127579 can solve the writing/capture time, independently of this bug here.
MattN: ping
Comment on attachment 8724312 [details]
MozReview Request: Bug 667233 - Support scheme upgrades for searching logins. r=dolske

I think this is basically ready for review other than adding functional tests and updating comments.
Attachment #8724312 - Flags: feedback?(neil)
Attachment #8724312 - Flags: feedback?(dolske)
Comment on attachment 8724313 [details]
MozReview Request: Bug 667233 - Support login scheme upgrades for autocomplete/fill/contextmenu. r=dolske

Ditto
Attachment #8724313 - Flags: feedback?(neil)
Attachment #8724313 - Flags: feedback?(dolske)
Attachment #8648270 - Attachment is obsolete: true
Attachment #8697310 - Attachment is obsolete: true
(In reply to Ben Bucksch (:BenB) from comment #91)
> I agree with your review. Just one point:
> > please put this new functionality behind a pref since bug 1127579 will need to be addressed
> > before this is enabled by default.
> 
> The feature here must be enabled by default, otherwise it's useless for us.
> If it's off by default, it's essentially not fixed for our use case (website
> wanting to make sure their users' stored passwords are migrated before
> switching to https).
> 
> If there is a https password stored, that should be used and http passwords
> could be ignored. Bug 1127579 can solve the writing/capture time,
> independently of this bug here.

The problem is that it's confusing to users if we autofill their HTTP password on HTTPS but then upon submission ask them if they want us to remember the passwords. To the user it looks like we're asking a nonsensical question as it appears that we already have and used the saved passwords for that "site".
Attachment #8716874 - Flags: feedback?(MattN+bmo)
Justin, note that another reason why I left the logic in storage and used an options argument is because one of the obvious things ckarlof and I would like to do is include related origins (subdomains of the eTLD+1) in fallback UI (e.g context menu and autocomplete). I was thinking of adding a `relatedOrigins` boolean attribute on nsILoginFindOptions for this. We would likely need some type of storage support for this (at least for mozstorage) and wouldn't be able to make storage dumb as there isn't a way to query based on an eTLD or a suffix of a hostname. Without a storage change we would have to reset to getAllLogins + post-processing which probably isn't a good idea with mozStorage + mobile for users like me with 600+ logins.
Priority: P3 → --
Depends on: 1255253
https://reviewboard.mozilla.org/r/36969/#review35707

Talked about this in person...

Suggested we leave the old countLogins() and findLogins() unchanged, and instead add a new flag to the searchData argument in searchLogins(). This does mean existing findLogins() callsites wanting the new behavour will need to change to using searchLogins() instead. Matt's going to look separately at seeing if we can make searchLogins() not require using a property bag to make this a bit easier to use from JS. Also suggested we make searchLogins() generate a console warning if you call it without specifing either a formSubmitURL or a httpRealm, just to make it slightly harder to accidentally misuse the API. (Arguably we could make it a hard-fail.)

::: toolkit/components/passwordmgr/nsILoginManagerStorage.idl:18
(Diff revision 1)
> +};

Sadface, we should deCOM this stuff at some point...

::: toolkit/components/passwordmgr/storage-json.js:286
(Diff revision 1)
> +                continue;

I sorta feel like the null-and-empty-string stuff is confusing enough already, and it would be better to just duplicate the isOriginMatching call instead of making it a special-case fallthru.
Comment on attachment 8724311 [details]
MozReview Request: Bug 667233 - Backout bug 1147561 to only support formSubmit upgrades so we can use the same logic. r=dolske

https://reviewboard.mozilla.org/r/10507/#review35753

rs=me
Attachment #8724311 - Flags: review?(dolske) → review+
Comment on attachment 8724311 [details]
MozReview Request: Bug 667233 - Backout bug 1147561 to only support formSubmit upgrades so we can use the same logic. r=dolske

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/10507/diff/1-2/
Attachment #8724312 - Attachment description: MozReview Request: Bug 667233 - Support scheme upgrades for counting and finding logins. r=dolske → MozReview Request: Bug 667233 - Support scheme upgrades for searching logins. r=dolske
Attachment #8724312 - Flags: review?(dolske)
Attachment #8724312 - Flags: feedback?(neil)
Attachment #8724312 - Flags: feedback?(dolske)
Comment on attachment 8724312 [details]
MozReview Request: Bug 667233 - Support scheme upgrades for searching logins. r=dolske

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36969/diff/1-2/
Comment on attachment 8724313 [details]
MozReview Request: Bug 667233 - Support login scheme upgrades for autocomplete/fill/contextmenu. r=dolske

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36971/diff/1-2/
Attachment #8724313 - Attachment description: MozReview Request: Bug 667233 - Support login scheme upgrades for autocomplete/fill/contextmenu. r=dolske → MozReview Request: Bug 667233 - Support login scheme upgrades for autocomplete/fill/contextmenu/prompts. r=dolske
Attachment #8724313 - Flags: feedback?(neil)
Attachment #8724313 - Flags: feedback?(dolske)
Comment on attachment 8724313 [details]
MozReview Request: Bug 667233 - Support login scheme upgrades for autocomplete/fill/contextmenu. r=dolske

I still need tests for the consumers of this new functionality and to implement this for HTTP auth but the other consumers are converted to use searchLogins with the new option.
Attachment #8724313 - Flags: feedback?(dolske)
https://reviewboard.mozilla.org/r/36969/#review35707

I considered creating a new interface with all of the properties that matchData could support to replace the property bag but I realized this won't work since `null` needs to be treated differently than "not specified" for searchLogins and once the object went through XPConnect, I don't think we would be able to differentiate between a property not specified vs. explicitly set to `null`.

I instead created a helper that wraps searchLogins which also nicely hides the `count` argument from consumers and doesn't break compatibility.

I think the warning may be too spammy (if it's output by default) since our own context menu consumer only wants to filter based on `hostname` and so will trigger this warning whenever it's opened. I can't just pass `null` to silence the warning as that changes the meaning (see above). If we want to warn but allow silencing then we could have another specially named property to silence the warning or have some other magic values (yuck e.g. "-1") for the omitted properties for silencing. I'm leaning towards not having the warning output by default or a new property name.

What do you think about having the helper also de-dupe? It seems like every call site is going to want that.

> I sorta feel like the null-and-empty-string stuff is confusing enough already, and it would be better to just duplicate the isOriginMatching call instead of making it a special-case fallthru.

In case you didn't notice, I still need a fall-through for the null case. I added a comment in the second fall-through to make this explicit.
Comment on attachment 8724312 [details]
MozReview Request: Bug 667233 - Support scheme upgrades for searching logins. r=dolske

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36969/diff/2-3/
Comment on attachment 8724313 [details]
MozReview Request: Bug 667233 - Support login scheme upgrades for autocomplete/fill/contextmenu. r=dolske

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36971/diff/2-3/
Attachment #8724313 - Flags: feedback?(dolske)
Comment on attachment 8724313 [details]
MozReview Request: Bug 667233 - Support login scheme upgrades for autocomplete/fill/contextmenu. r=dolske

See comment 105.

Not sure why mozreview is clearing the feedback flags. I guess it just clears all even though it doesn't support feedback.
Attachment #8724313 - Flags: feedback?(dolske)
Attachment #8716874 - Attachment is obsolete: true
https://reviewboard.mozilla.org/r/36971/#review36009

Looking good. No r+ since I think you're planning to finish the TODOs?

::: toolkit/components/passwordmgr/LoginManagerParent.jsm:33
(Diff revision 3)
> +  _schemeUpgrades: false,

bikeshed: _doSchemeUpgrades, so it's clearer as a boolean?

::: toolkit/components/passwordmgr/LoginManagerParent.jsm:207
(Diff revision 3)
> +      httpRealm: null,

Do we actually need to specify httpReam:null with the switch to searchLogins?

I'd have expected that to not be needed (which makes it nicer than the calls to findLogins because you only need to specify one of formSubmitURL / httpRealm).
Comment on attachment 8724312 [details]
MozReview Request: Bug 667233 - Support scheme upgrades for searching logins. r=dolske

https://reviewboard.mozilla.org/r/36969/#review35993

::: toolkit/components/passwordmgr/storage-json.js:293
(Diff revision 3)
>            // Historical compatibility requires this special case

The "historical compat" comment should go next to the empty-string check (already kinda out of place, but with your changes it more clearly goes here now).

::: toolkit/components/passwordmgr/storage-json.js:297
(Diff revision 3)
> +                continue;

s/continue/break/? This isn't a common pattern, and risks being too clever if we ever added code after the switch and failed to notice this.

Although if we ever add more logic here, this entire switch should probably go away. It was fine as an old-school "check for valid properties" + one special case (formSubmitURL), but gets strained beyond that. Maybe just a simple loop with a separate step to validate that all the keys are known.

::: toolkit/components/passwordmgr/storage-mozStorage.js:477
(Diff revision 3)
>          // Historical compatibility requires this special case

Ditto, this move next to the empty-string thing now.
Attachment #8724312 - Flags: review?(dolske) → review+
Thanks for the review
status-firefox46: affected → wontfix
status-firefox47: --- → affected
status-firefox48: --- → affected
tracking-firefox46: + → ---
Comment on attachment 8724313 [details]
MozReview Request: Bug 667233 - Support login scheme upgrades for autocomplete/fill/contextmenu. r=dolske

(Clearing review, I don't think there was anything you wanted me to look at in this half-finished patch?)
Attachment #8724313 - Flags: feedback?(dolske)
Where are we on this bug?
I'll get back to it when I finish fixing e10s tests

Comment 116

3 years ago
As FF47 is getting closer and this was meant for being released in FF47: How far have you come with that?

1&1 (as provider of GMX, WEB.DE and Mail.com) already started to switch there login pages to ssl-only for Safari users (gmx.at/gmx.ch last week, mail.com, gmx.com, gmx.fr, gmx.es, gmx.co.uk, gmx.net today and web.de next week) and waiting for doing the same for Firefox (the majority of their users) until this one is launched.
https://reviewboard.mozilla.org/r/36971/#review36009

> Do we actually need to specify httpReam:null with the switch to searchLogins?
> 
> I'd have expected that to not be needed (which makes it nicer than the calls to findLogins because you only need to specify one of formSubmitURL / httpRealm).

I see what you mean. I agree it's safe to rely on not returning a login with a non-null httpRealm when we're searching for a formSubmitURL.
Comment on attachment 8724311 [details]
MozReview Request: Bug 667233 - Backout bug 1147561 to only support formSubmit upgrades so we can use the same logic. r=dolske

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/10507/diff/2-3/
Attachment #8724313 - Attachment description: MozReview Request: Bug 667233 - Support login scheme upgrades for autocomplete/fill/contextmenu/prompts. r=dolske → MozReview Request: Bug 667233 - Support login scheme upgrades for autocomplete/fill/contextmenu. r=dolske
Attachment #8724313 - Flags: review?(dolske)
Comment on attachment 8724312 [details]
MozReview Request: Bug 667233 - Support scheme upgrades for searching logins. r=dolske

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36969/diff/3-4/
Comment on attachment 8724313 [details]
MozReview Request: Bug 667233 - Support login scheme upgrades for autocomplete/fill/contextmenu. r=dolske

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36971/diff/3-4/
Comment on attachment 8724313 [details]
MozReview Request: Bug 667233 - Support login scheme upgrades for autocomplete/fill/contextmenu. r=dolske

https://reviewboard.mozilla.org/r/36971/#review52286

::: toolkit/components/passwordmgr/LoginHelper.jsm:382
(Diff revision 4)
> +      for (let [attr, preferred] of Object.entries(preferences)) {
> +        switch (attr) {
> +          case "scheme": {
> +            try {
> +              // Only `hostname` is currently considered
> +              let existingLoginURI = Services.io.newURI(existingLogin.hostname, null, null);

Simlarly to below comment -- can we just use login.hostname.startsWith("https://")?

::: toolkit/components/passwordmgr/LoginHelper.jsm:397
(Diff revision 4)
> +            break;
> +          }
> +          case "timeCreated":
> +          case "timeLastUsed":
> +          case "timeLastChanged": {
> +            if (preferred != "newest") {

Do we use all 3 types in the code?

::: toolkit/components/passwordmgr/LoginHelper.jsm:418
(Diff revision 4)
> +
>      for (let login of logins) {
>        let key = getKey(login, uniqueKeys);
> -      // If we find a more recently used login for the same key, replace the existing one.
> +
>        if (loginsByKeys.has(key)) {
> -        let loginDate = login.QueryInterface(Ci.nsILoginMetaInfo).timeLastUsed;
> +        if (!isLoginPreferred(loginsByKeys.get(key), login)) {

This is (already) implicitly picking the first of two otherwise-equally preferred logins. Expected?

::: toolkit/components/passwordmgr/LoginManagerContextMenu.jsm:100
(Diff revision 4)
>     * @returns {nsILoginInfo[]} a login list
>     */
>    _findLogins(documentURI) {
> -    let logins = Services.logins.findLogins({}, documentURI.prePath, "", "");
> +    let searchParams = {
> +      hostname: documentURI.prePath,
> +      schemeUpgrades: LoginHelper.schemeUpgrades,

Maybe have schemeUpgrades always be true here, and have the callee check to pref to clear it? (ie, is it actually allowed).

::: toolkit/components/passwordmgr/LoginManagerParent.jsm:262
(Diff revision 4)
> -      logins = Services.logins.findLogins({}, formOrigin, actionOrigin, null);
> +      logins = LoginHelper.searchLoginsWithObject({
> +        formSubmitURL: actionOrigin,
> +        hostname: formOrigin,
> +        schemeUpgrades: LoginHelper.schemeUpgrades,
> +      });
> +      let dedupePreferences = [

Bikeshed: s/dedupePreferences/resolveBy/ or something like that?

::: toolkit/components/passwordmgr/LoginManagerParent.jsm:272
(Diff revision 4)
> +        dedupePreferences.unshift({
> +          scheme,
> +        });
> +      } catch (ex) {
> +        // Handle origins that can't be made into nsIURI.
> +      }

Can this try block just be simplified to:

  if (formOrigin.startsWith("https://")) {
    dedupePreferences.preferHTTPS = true;
  }
  
Seems this is the only case we actually care about?

Or maybe just pass in formOrigin directly, and let dedupeLogins() sweat the details?

It might also be simpler to have dedupePreferences just be a simple array, eg ["timeLastChanged", "preferHTTPS"]

::: toolkit/components/passwordmgr/LoginManagerParent.jsm:513
(Diff revision 4)
> -                    Services.logins.countLogins(loginFormOrigin, "", null) > 0;
> +                    Services.logins.countLogins(loginFormOrigin, "", null, {
> +                      schemeUpgrades: LoginHelper.schemeUpgrades,

Err, should this be searchLoginsWithObject?
Attachment #8724313 - Flags: review?(dolske)
Comment on attachment 8724311 [details]
MozReview Request: Bug 667233 - Backout bug 1147561 to only support formSubmit upgrades so we can use the same logic. r=dolske

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/10507/diff/3-4/
Attachment #8724313 - Flags: review?(dolske)
Comment on attachment 8724312 [details]
MozReview Request: Bug 667233 - Support scheme upgrades for searching logins. r=dolske

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36969/diff/4-5/
Comment on attachment 8724313 [details]
MozReview Request: Bug 667233 - Support login scheme upgrades for autocomplete/fill/contextmenu. r=dolske

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36971/diff/4-5/
https://reviewboard.mozilla.org/r/36971/#review52286

I didn't add unit tests for deduping yet (I will do that before landing but that shouldn't block reviewing this). I did improve the existing tests by folding the bug 1275113 change into this one.

> This is (already) implicitly picking the first of two otherwise-equally preferred logins. Expected?

Yes, I've documented this now. Previously I only mentioned that the sort is preserved which is kinda related.

> Maybe have schemeUpgrades always be true here, and have the callee check to pref to clear it? (ie, is it actually allowed).

I looked into this and it wasn't clear which function I would want to ignore the argument in. At the lowest level (isOriginMatching) doesn't really seem like the right place since it's a generic helper and having behaviour changes based on a pref doesn't seem ideal. I think have the function always do what is asked of it by the caller is clearer and makes for easier to test code.
Attachment #8724313 - Flags: review?(dolske) → review+
Comment on attachment 8724313 [details]
MozReview Request: Bug 667233 - Support login scheme upgrades for autocomplete/fill/contextmenu. r=dolske

https://reviewboard.mozilla.org/r/36971/#review52746
Comment on attachment 8724313 [details]
MozReview Request: Bug 667233 - Support login scheme upgrades for autocomplete/fill/contextmenu. r=dolske

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36971/diff/5-6/
Duplicate of this bug: 1275113

Comment 133

3 years ago
backoutbugherder
https://hg.mozilla.org/mozilla-central/rev/ba10f978fe39
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Depends on: 1127579
See Also: bug 1127579
Duplicate of this bug: 1166111
Depends on: 1292340
Duplicate of this bug: 1296216
Whiteboard: security:passwords, [fxprivacy] → security:passwords, [fxprivacy][adv-main49-]

Updated

3 years ago
Depends on: 1305652

Comment 137

3 years ago
I am very thankful that this has been changed and fixed with the launch of FF 49. We are currently switching our servers to https only for all FF >= 49 (and already have done that for all other browsers).

This works fine, but still leaves me with one open question:

username/pw was originally stored on http version of the page
FF49 is using the stored username/pw now also on https version of the page, but not altering (or duplicating for the https version) the stored username/pw, so in the credential store they are still marked as "only" belonging to the http version

Doesn't this imply the possibility, that in the future, we will maybe see an additional change to be "again more secure" which would then limit the use to https stored username/pw again and we would be back to the same trouble, we now have prevented?

A user now saving his username/pw on the https version gets his credentials marked for https and also a password change leads to a https marked save of the new credential - so this two cases are better off now.
(In reply to Thomas Schäfer from comment #137)
> This works fine, but still leaves me with one open question:
> 
> username/pw was originally stored on http version of the page
> FF49 is using the stored username/pw now also on https version of the page,
> but not altering (or duplicating for the https version) the stored
> username/pw, so in the credential store they are still marked as "only"
> belonging to the http version
> 
> Doesn't this imply the possibility, that in the future, we will maybe see an
> additional change to be "again more secure" which would then limit the use
> to https stored username/pw again and we would be back to the same trouble,
> we now have prevented?

Yes, that's a possibility but I don't think we would do that without helping to migrate users. i.e. I don't think we'd suddenly prevent access to saved HTTP logins on HTTPS without an automated migration so I don't think you should worry about that now.

Updated

2 years ago
Depends on: 1312048

Updated

2 years ago
Depends on: 1322673
You need to log in before you can comment on or make changes to this bug.