1608762 - Username not saved or filled on https://www.modern-imm.fr

Status: RESOLVED FIXED

(Toolkit :: Password Manager: Site Compatibility, defect, P3)

Description

[Julien Wajsberg [:julienw] (PTO -> Mar 16)]

Hey,

There's this website that I often use (https://www.modern-imm.fr/) where the login information aren't saved nor autofilled. The reason is that the action is for a different website. Moreover there's a target='_blank' that opens the result in a new tab.

I could force autofill by rightclicking and chosing the right option, and this "opt in" solution would work for me. But for this to work, I'd first need to be able to save the login information, and as far as I know this doesn't work in this case.

Therefore this feature request: it would be a good idea to be able to save the login information also in this case.

Comment 1

[Matthew N. [:MattN]]

If you delete the existing login I think we should offer to save the cross-origin one. I think we just see that the login is already saved (ignoring the origin) and don't prompt.

Can you attach debug logs: https://wiki.mozilla.org/Firefox:Password_Manager_Debugging

Thanks

Comment 2

[Julien Wajsberg [:julienw] (PTO -> Mar 16)]

One correction first: in the description I wrote I could force autofill and you understood that this was the past tense, but it was the conditional tense. When I wrote this, I didn't do it but I was saying I would do it. So when I wrote this comment initially I didn't have any saved login yet. Sorry for the confusion.

But now it seems like something changed in previous months. I could (past tense :D) save my login information because I see it's recalled now. (honestly I don't remember now).
From about:logins I see both the login and the password are saved, however the login isn't autofilled in the page, only the password is.

Note: the wiki page says:

The <form>'s action attribute is for a different origin than the one that the login was saved for.

which doesn't seem true anymore?

Comment 3

[Julien Wajsberg [:julienw] (PTO -> Mar 16)]

Attachment: logs when navigating to the page

Comment 4

[Matthew N. [:MattN]]

Thanks for the logs.

I'm still not sure I'm understanding what you expect since I think the summary of the bug and parts of comment 0 and comment 2 have incorrect assumptions. Does it boil down to you wanting to both save and fill on the homepage of https://www.modern-imm.fr/?

This is a very interesting login process… I see multiple issues:

• <input type="login"> is why we log (form -- no username field found)… that's the first time I've seen that invalid @type…
  • We intentionally didn't use input.type and prefer input.getAttribute("type") so that we can support input types that are standardized but which Gecko doesn't yet support: https://searchfox.org/mozilla-central/rev/4e228dc5f594340d35da7453829ad9f3c3cb8b58/toolkit/components/passwordmgr/LoginHelper.jsm#864-877 – We can maybe change/improve this at this point.
• At one point when I submitted the login form the doorhanger opened with an empty password field… this should never happen and I've never seen it before.

Are those the remaining issues to fix at this point?

(In reply to Julien Wajsberg [:julienw] from comment #2)

Note: the wiki page says:

The <form>'s action attribute is for a different origin than the one that the login was saved for.

which doesn't seem true anymore?

That is still true so I think there is some confusion. I have now clarified the meaning of "one" in that sentence. Does that make sense now? To make it more concrete, we should save the login with:

origin: https://www.modern-imm.fr
formActionOrigin: https://www.orchestrav2.egiweb.net

and if the latter doesn't match the origin of form.action then we won't auto-fill without user interaction, we will still autocomplete it from the dropdown.

Comment 5

[Julien Wajsberg [:julienw] (PTO -> Mar 16)]

I have now clarified the meaning of "one" in that sentence. Does that make sense now?

Ah, I understand now, thanks to your added explanations! I think the confusion (for me) was that you save both the page's origin and the form action's origin. I understood origin in the form's action attribute is for a different origin as the origin of the page where the form is, instead of he origin of the action.

Does it boil down to you wanting to both save and fill on the homepage of https://www.modern-imm.fr/?

Yes!

Are those the remaining issues to fix at this point?

I'll test again the full process with a clean profile, so that I see what the current state is. I believe something changed since I first created the bug.
Keeping the NI then.

Comment 6

[Matthew N. [:MattN]]

(In reply to Matthew N. [:MattN] (PM me if request are blocking you) from comment #4)

This is a very interesting login process… I see multiple issues:

• <input type="login"> is why we log (form -- no username field found)… that's the first time I've seen that invalid @type…
  • We intentionally didn't use input.type and prefer input.getAttribute("type") so that we can support input types that are standardized but which Gecko doesn't yet support: https://searchfox.org/mozilla-central/rev/4e228dc5f594340d35da7453829ad9f3c3cb8b58/toolkit/components/passwordmgr/LoginHelper.jsm#864-877 – We can maybe change/improve this at this point.

Since this is the only site that has this specific problem, I think we can track that issue here.

• At one point when I submitted the login form the doorhanger opened with an empty password field… this should never happen and I've never seen it before.

I'm fixing this in bug 1629174.

I think there is a third issue:

• The doorhanger to save appears in the original tab, not in the target _blank one… this makes sense since the login form is in that tab but the user can easily miss it. I think this is a general popup notification (doorhanger) problem… if a doorhanger gets dismissed by a tab change just as it's opening it get partially opened: "showing" => "dismissed" with no "shown" state in the middle. In that case IMO the doorhanger should automatically re-open (like we do in similar cases) when the tab is re-selected. I'll look into this

Comment 7

[Julien Wajsberg [:julienw] (PTO -> Mar 16)]

Yeah, I think that's accurate.
These are indeed the problems I see in the current version of Firefox:

1. The "login" input isn't seen, neither to save nor to retrieve.
2. The doorhanger is in the origin tab, which makes it easy to miss.

For the first error, isn't it something we could use toolkit/components/passwordmgr/content/recipes.json for ? Especially if this happens only on this website so far. If that's an OK fix I could work on it.

Comment 8

[Matthew N. [:MattN]]

(In reply to Julien Wajsberg [:julienw] from comment #7)

1. The "login" input isn't seen, neither to save nor to retrieve.
2. The doorhanger is in the origin tab, which makes it easy to miss.

I will file a generic PopupNotifications bug on this since I don't think it's specific to password manager.

For the first error, isn't it something we could use toolkit/components/passwordmgr/content/recipes.json for ? Especially if this happens only on this website so far. If that's an OK fix I could work on it.

I'm not 100% sure if it would work but you're welcome to try it. You can use this bug for that.

Comment 9

[Julien Wajsberg [:julienw] (PTO -> Mar 16)]

Thanks for needinfo me, I do a bad job managing my bugmail these days. I'll look at this soon!

Comment 10

[Timea Cernea [:tbabos][inactive]]

Checked this once more and the "Show Password" toggle does not appear in the dismissed doorhanger.
Could this also be addressed in this bug or we need a new one?

Comment 11

[Matthew N. [:MattN]]

(In reply to Timea Cernea [:tbabos] from comment #10)

Checked this once more and the "Show Password" toggle does not appear in the dismissed doorhanger.
Could this also be addressed in this bug or we need a new one?

I guess you mean that's something I didn't address in bug 1629174? That kinda makes sense since we remove the option when a doorhanger is dismissed and in this case it is immediately dismissed upon tab change. I think that's probably what we want as the point of that behaviour is to not have the password in plaintext some minutes or hours after the user submitted the form. In this case the user may submit the form and then an hour later go back to the original tab… the password shouldn't be in plaintext in the doorhanger then. i.e. the checkbox should only appear for doorhangers that weren't dismissed.

Comment 12

[Julien Wajsberg [:julienw] (PTO -> Mar 16)]

I added this bit to services/settings/dumps/main/password-recipes.json and it worked for me:

    {
      "hosts": [
        "www.modern-imm.fr"
      ],
      "description": "The Modern'Imm website uses a field type of 'login' that's not recognized by the internal logic.",
      "usernameSelector": "input[name='login']"
    },

I understand I can't add it myself anymore directly to the file, maybe Tim you could help add it on Remote Settings?

Thanks!

Comment 13

[Tim Giles [:tgiles]]

:julienw, I added your snippet from Comment 12 into my local password-recipes.json but I'm only seeing the password manager pick up the password field and not the username field when I have a saved login for https://www.modern-imm.fr/. Are you saying this snippet should fix both the password identification and the username identification for this case (where we have a saved login for the site) or some other case? It also looks like the portal that opens if you fail to log in does not correctly save the username, not sure if that's accounted for on this bug or not.

Comment 14

[Julien Wajsberg [:julienw] (PTO -> Mar 16)]

(In reply to Tim Giles [:tgiles] from comment #13)

:julienw, I added your snippet from Comment 12 into my local password-recipes.json but I'm only seeing the password manager pick up the password field and not the username field when I have a saved login for https://www.modern-imm.fr/. Are you saying this snippet should fix both the password identification and the username identification for this case (where we have a saved login for the site) or some other case? It also looks like the portal that opens if you fail to log in does not correctly save the username, not sure if that's accounted for on this bug or not.

Well, I was puzzled at first, because I couldn't make it work when I tried again this morning.

Then I realized that I was going to https://modern-imm.fr instead of https://www.modern-imm.fr. Indeed the website doesn't redirect between each of these. So it's possible you used the former instead of the latter when you tried it yesterday too?

The fix is easy, we should also include "modern-imm.fr" in the hosts section. With this addition now both the login and the password are saved properly in both domain names for me. Actually I don't even need to press the "connect" button so that Firefox' logic finds there's something to save, the little "key" button appears as soon as I type there. So this looks good on my side!

I hope you see the same thing on your end!

Comment 15

[Tim Giles [:tgiles]]

(In reply to Julien Wajsberg [:julienw] from comment #14)

Well, I was puzzled at first, because I couldn't make it work when I tried again this morning.

Then I realized that I was going to https://modern-imm.fr instead of https://www.modern-imm.fr. Indeed the website doesn't redirect between each of these. So it's possible you used the former instead of the latter when you tried it yesterday too?

The fix is easy, we should also include "modern-imm.fr" in the hosts section. With this addition now both the login and the password are saved properly in both domain names for me. Actually I don't even need to press the "connect" button so that Firefox' logic finds there's something to save, the little "key" button appears as soon as I type there. So this looks good on my side!

I hope you see the same thing on your end!

I added the additional "modern.imm.fr" entry to my local password-recipes.json so that I have both "www.modern-imm.fr" and "modern-imm.fr" for the hosts of that particular rule...but on the main page (www.modern-imm.fr) when I add a username and password to the "Votre accès privilège", Firefox is still only picking up the password field and not saving the username.

Okay, I think I figured out the issue. I had to set signon.recipes.remoteRecipes.enabled to false so that the newly added recipe wouldn't get wiped by the initial "password-recipes" remote settings sync. I'm not sure how you were able to get the recipe added locally without it being wiped by the sync, maybe I have something else going on with my config. Anyway, after updating that pref to test, and having the updated recipe, the username is successfully identified on both modern-imm.fr and www.modern-imm.fr. I'll see to getting this recipe added to remote settings.

Thanks for helping me work through this!

Comment 16

[Julien Wajsberg [:julienw] (PTO -> Mar 16)]

ah that's weird, I'm not sure how this worked for me then :) is it possible that when I ran Firefox this just reused a previous profile and so the sync didn't happen?

Anyway thanks a lot for looking at this and helping with that!

Comment 17

[Tim Giles [:tgiles]]

:julienw, the recipes list should be updated now. Can you verify that the issue is fixed on the modern-imm sites?

Comment 18

[Julien Wajsberg [:julienw] (PTO -> Mar 16)]

Thanks Tim, I just had a look.

Saving and retrieving seem to work on both www.modern-imm.fr and modern-imm.fr

However I found what could be another bug, albeit more minor.

STR is:

1. fill in the right credentials on https://modern-imm.fr.
2. save them.
3. go to https://www.modern-imm.fr.
4. click on the login => I expect that the saved credentials for modern-imm.fr are proposed. But instead I get this error:

JavaScript error: resource://gre/modules/LoginFormFactory.jsm, line 101: Error: createFromField requires a password or username field in a document

Is it possible that this logic doesn't use the recipes?

Note that when clicking on the password field, then as expected I get the choice to use the saved password for the other website. But the username isn't filled in that case.

Comment 19

[Tim Giles [:tgiles]]

(In reply to Julien Wajsberg [:julienw] from comment #18)

However I found what could be another bug, albeit more minor.

STR is:

1. fill in the right credentials on https://modern-imm.fr.
2. save them.
3. go to https://www.modern-imm.fr.
4. click on the login => I expect that the saved credentials for modern-imm.fr are proposed. But instead I get this error:

JavaScript error: resource://gre/modules/LoginFormFactory.jsm, line 101: Error: createFromField requires a password or username field in a document

Is it possible that this logic doesn't use the recipes?
Note that when clicking on the password field, then as expected I get the choice to use the saved password for the other website. But the username isn't filled in that case.

This is because the username field is determined to not be a username field by our password manager logic. Because the username field isn't recognized as a username, we throw that error when we try to call createFromField. If we look at inputTypeIsCompatibleWithUsername, we can see that "login" is not in this fieldType list (nor should it be since it is not an available type for input fields). This is why the username is not filled correctly and why you can't autocomplete from the username input. The most ideal solution would be for them to change their markup so that the username field type is "text" instead of "login".

This raises a new point though, should we be using the recipes to help determine what the username and/or password fields are in the LoginHelper logic? If so, it should definitely be a new bug. :dimi, what do you think about this? Should this bug be closed because the recipe has been added, but doesn't completely solve the issue, or should this bug stay open and we figure out a way to use recipes to solve the isUsernameFieldType issue?

Comment 20

[Julien Wajsberg [:julienw] (PTO -> Mar 16)]

I just contacted the website directly, with precise things they could change. Let's see how it goes...

While writing my mail I was wondering why, because the browser internally handles the login type as text since the value login is incorrect, we don't handle type=login as type=text for the password manager as well. What do you think?

Comment 21

[Dimi Lee [:dimi]]

Right now password manager only handles cases we accept. Let's say if we want to support this type=login case, the implementation might be changing our code to only reject certain valid input types (we shouldn't accept non-standard input type in our code). Doing this means we'll then run password manager code on a lot of places where we don't run now. I think the change might introduce performance issue (maybe not very serious) and increase the chance we identified a non username field as a username field (false-positive).

Given that "login" is not a real input type, i would prefer not changing our implementation and instead asking site to fix like what we've already done for this issue.

Comment 22

[Julien Wajsberg [:julienw] (PTO -> Mar 16)]

Hey Dimi, thanks for your answer. I feel like you answered my question in comment 20. I understand that running the password manager in a lot more websites might lead to more faulty behaviors that we may not have the resource to handle now. Stability is good too.

However my first question in comment 18 was more general than this: we already have this recipes.json file to somehow manually fix websites where the main logic doesn't work. I'm surprised these recipes are not used in all the cases where Firefox try to identify if a field is a username or a password. The consequences of doing this should be a lot more contained because there are only a few entries in this file, they are constrained with specific hosts.

That said most of the hosts properties in the recipes file contain only one host, so effectively this change would be just for modern-imm.fr currently. It's probably not worth the effort.

Comment 23

[Tim Giles [:tgiles]]

:julienw, can we resolve this bug or is there more work to be done for this?

Comment 24

[Julien Wajsberg [:julienw] (PTO -> Mar 16)]

I'd say this is your call :-) Is using recipes.json for finding the username and password fields something you'd like to implement? Should we file a separate bug for this specific task?
On my side I don't see this as fully fixed, but I'll understand if you decide this isn't worth the effort.

Comment 25

[Tim Giles [:tgiles]]

I'm going to say this bug is fixed then, in that case 🙂 It sounds like we'll need another bug for the specific task of using recipes.json for finding the username and password in a particular site though.