1386283 - After removed <input type="password"> from the document, history.replaceState() would still cause save login notification being prompted

Status: RESOLVED FIXED

(Toolkit :: Password Manager, defect, P1)

Description

[Damian Bradicich]

User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/59.0.3071.115 Safari/537.36

Steps to reproduce:

I had my company's application running and I logged in as admin user using typical form in the app, then after form was gone, used the dev console to call the following

window.history.replaceState({},null,"http://localhost:8081/#browse/browse:maven-releases:com")

Where the previous url was "http://localhost:8081/#browse/browse:maven-releases:com/sonatype"


Actual results:

Each time I call replaceState() I am given the 'Store your credentials' dialog


Expected results:

The state changes with no 'Store your credentials' dialog

Comment 1

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Thoughts, Samael?

Comment 2

[Samael Wang [:freesamael] (away for now)]

Is it Firefox? The posted user agent is Chrome 59. And without an accessible public website to reproduce the issue, I really couldn't get clues on what happened.

Comment 3

[Damian Bradicich]

Yes it's firefox, i just happen to use chrome to open the issue ;)

I'll try to give more detail.  

I've got a tree displayed in my app, anytime you click on a node in this tree, we call window.history.replaceState() to update the url with the current tree coordinates.  This all works great, until i authenticate a user.  At this point, anytime i call window.history.replaceState() (whether from our code, or from the console) I get a prompt to save my credentials for the site.  Otherwise everything functions properly (i.e. the replaceState call updates the url as expected)

Comment 4

[Samael Wang [:freesamael] (away for now)]

Do you mean calling replaceState from the page where a login form redirects to? I'm trying to make a minimal dummy test case for it but I still don't quite get it. Does this somewhat look like you're case?

http://freesamael.github.io/gecko/shistory/replaceState/login.html

Could you help me to produce a test case for the issue?

Comment 5

[Damian Bradicich]

After some more investigation here with other applications, we only have one app that shows the functionality I am noting here.  Which deters me from thinking this is any kind of bug in FF.  I will post an answer here once a resolution was found, but in the mean time, I don't think you guys should spend any more cycles on this

Comment 6

[Damian Bradicich]

(In reply to Damian Bradicich from comment #5)
> After some more investigation here with other applications, we only have one
> app that shows the functionality I am noting here.  Which deters me from
> thinking this is any kind of bug in FF.  I will post an answer here once a
> resolution was found, but in the mean time, I don't think you guys should
> spend any more cycles on this

Just to note, I don't want to close the ticket yet, just do a bit more investigation on my end :)

Comment 7

[Damian Bradicich]

I have opened a port on my router that has our app running, you can access it here http://66.31.253.206:8089/?debug

You will find that prior to signing in, if you do this in the firefox console

window.history.replaceState({}, null, "http://66.31.253.206:8089/#browse:maven-releases:com")

You will see the state change exactly as expected and nothing else happens.

But once you sign in (user/pass -> admin/admin123) and run the same command in the console, you see a store credential dialog popup

Comment 8

[Damian Bradicich]

Is someone able to take a peek at this ?  I'll leave the server up and open over the weekend.

Thanks!

http://66.31.253.206:8089/?debug

Comment 9

[Samael Wang [:freesamael] (away for now)]

(In reply to Damian Bradicich from comment #8)
> Is someone able to take a peek at this ?  I'll leave the server up and open
> over the weekend.
> 
> Thanks!
> 
> http://66.31.253.206:8089/?debug

Thanks Damian. I was stuck at the initializing page last time so I thought you were still working on it. Just realized it might be caused by uBlock origin today.

I'm able to produce the issue with the site you provided, and have a short discussion with a frontend engineer. It seems the sign-in was done by an XHR directly, but not with a <form>. I believe this somehow confuses gecko's LoginManager, that it thinks the replaceState() is where the location changes and should prompt for remembering password. 

I'm trying to make a minimal test with XHR to hopefully make this easier to test.

Comment 10

[Samael Wang [:freesamael] (away for now)]

cc'ing Luke who's more familiar with form autofilling stuff. Damian, could you leave the site alive for a few days more so that people who have better sense with LoginManager have a chance to understand the scenario?

Comment 11

[Samael Wang [:freesamael] (away for now)]

Minimal test case:

    <input type="text" placeholder="Enter Username"><br>
    <input type="password" placeholder="Enter Password"><br>
    <button onclick="history.replaceState({}, 'replaceState causes password prompt', location.href);">replaceState</button>

Comment 12

[Damian Bradicich]

Great, glad you were able to reproduce, yes sure, i'll do my best to leave the site up for the foreseeable future.  

Note that you can simply do http://66.31.253.206:8089/ for faster load times, you just have the ?debug param in case debugging any of our (or extjs code) proves handy.

Thanks!

Comment 13

[Samael Wang [:freesamael] (away for now)]

This should be a better example than comment 11: http://freesamael.github.io/gecko/password-notification/replaceState/login.html

(and BTW Safari has the same symptom)

Comment 14

[Matthew N. [:MattN]]

(In reply to Samael Wang [:freesamael] from comment #11)
> Minimal test case:

This minimal test case is demonstrating expected behaviour. replaceState is supposed to cause a login prompt if there are login fields present.

What is the actual problem you are encountering since running replaceState from the console when login fields are present is expected to show the prompt? Are you leaving hidden login fields (e.g. type=password) in the document even after login?

Comment 15

[Samael Wang [:freesamael] (away for now)]

(In reply to Matthew N. [:MattN] (huge backlog; PM if requests are blocking you) from comment #14)
> (In reply to Samael Wang [:freesamael] from comment #11)
> > Minimal test case:
> 
> This minimal test case is demonstrating expected behaviour. replaceState is
> supposed to cause a login prompt if there are login fields present.
> 
> What is the actual problem you are encountering since running replaceState
> from the console when login fields are present is expected to show the
> prompt? Are you leaving hidden login fields (e.g. type=password) in the
> document even after login?

Yes. I later realized that, so I provided another test case in comment 13 which should more resemble what Damian is encountering:

<html lang="en">
  <head>
    <meta charset="utf-8">
    <title>Test LoginManager</title>
    <script type="application/javascript">
    function doFakeSignIn() {
      // (assume there're XHR operations here to post credentials...)
      // ...
      document.querySelector("#fake-login-form").remove();
      document.querySelector("#text").textContent = "You're signed in";
    }
    </script>
  </head>
  <body>
    <div id="fake-login-form">
      <input type="text" placeholder="Enter Username"><br>
      <input type="password" placeholder="Enter Password"><br>
      <button onclick="doFakeSignIn();">SignIn</button>
    </div>
    <p id="text"></p>
    <button onclick="history.replaceState({}, 'replaceState causes password prompt', location.href);">replaceState</button>
  </body>
</html>

It's that even if the script completely remove input fields from the document, history.replaceState() would still cause a prompt. I should have updated the summary. My bad.

Comment 16

[Damian Bradicich]

Are you still expecting anything from me ?  (I see issue is still marked as 'needs info'

Comment 18

[Firefox Bug Husbandry Bot]

https://wiki.mozilla.org/Bug_Triage/Projects/Bug_Handling/Bug_Husbandry#Release_Cycle_Queries_and_Activities

Move optional bugs to next release

Comment 19

[Sylvestre Ledru [:Sylvestre]]

Moving to p3 because no activity for at least 24 weeks.

Comment 20

[Damian Bradicich]

just to note, i've just been waiting for the issue to be picked up, still just as high priority to me as ever, figured pinging for updates would get annoying :)

Comment 21

[Matthew N. [:MattN]]

Yeah, this is still high-priority and shouln't have been changed.

Comment 22

[Matthew N. [:MattN]]

Attachment: Comment 13 testcase

Comment 23

[Matthew N. [:MattN]]

(In reply to Damian Bradicich from comment #20)

just to note, i've just been waiting for the issue to be picked up, still
just as high priority to me as ever, figured pinging for updates would get
annoying :)

Hi Damian,

It seems like bug 1456843 (released in Firefox 63 on 2018-10-23) has fixed the testcase so the problem no longer occurs. Are you still seeing the issue?

If so, could you provide more details, maybe making the test page available again or attaching a reduced testcase like comment 22 that causes the problem.

Comment 24

[Matthew N. [:MattN]]

Attachment: Bug 1386283 - Only consider connected <input>s for username/password fields. r=mconley

Comment 25

[Damian Bradicich]

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

(In reply to Damian Bradicich from comment #20)

just to note, i've just been waiting for the issue to be picked up, still
just as high priority to me as ever, figured pinging for updates would get
annoying :)

Hi Damian,

It seems like bug 1456843 (released in Firefox 63 on 2018-10-23) has fixed the testcase so the problem no longer occurs. Are you still seeing the issue?

If so, could you provide more details, maybe making the test page available again or attaching a reduced testcase like comment 22 that causes the problem.

Hi Matthew, so I went ahead and tried to reproduce, and it looks like all is well now :) Thanks for the heads up!

Comment 26

[Matthew N. [:MattN]]

Great, thanks for confirming! I'm still going to do a more proper fix (and add a test) since bug 1456843 didn't really intend to fix this.

Comment 27

[Matthew N. [:MattN]]

Attachment: Bug 1386283 - Test: Only consider connected <input>s for username/password fields. r=mconley

Depends on D15887

Comment 28

[Matthew N. [:MattN]]

Attachment: Bug 1386283 - Enable eslint 'indent' rule on passwordmgr with --fix. r=mconley

ignore-this-changeset for blame

Depends on D15997

Comment 29

[Matthew N. [:MattN]]

Attachment: Bug 1386283 - Enable eslint 'brace-style' and 'curly' rules on passwordmgr with --fix. r=mconley

ignore-this-changeset for blame

Depends on D16109

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d5357c40725c
Only consider connected <input>s for username/password fields. r=mconley
https://hg.mozilla.org/integration/autoland/rev/c39b7fb8bbdd
Test: Only consider connected <input>s for username/password fields. r=mconley
https://hg.mozilla.org/integration/autoland/rev/06cb65fac0f3
Enable eslint 'indent' rule on passwordmgr with --fix. r=mconley
https://hg.mozilla.org/integration/autoland/rev/16ce9a71ff04
Enable eslint 'brace-style' and 'curly' rules on passwordmgr with --fix. r=mconley

Comment 31

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/d5357c40725c
https://hg.mozilla.org/mozilla-central/rev/c39b7fb8bbdd
https://hg.mozilla.org/mozilla-central/rev/06cb65fac0f3
https://hg.mozilla.org/mozilla-central/rev/16ce9a71ff04