Merge pull request brave#7653 from brave/fix/7649

do not accidentally show password as username
NejcZdovc · Mar 12, 2017 · 99efb5a · 99efb5a
2 parents 870fd32 + 51d2d58
commit 99efb5a
Show file tree

Hide file tree

Showing 8 changed files with 89 additions and 26 deletions.
diff --git a/app/extensions/brave/content/scripts/passwordManager.js b/app/extensions/brave/content/scripts/passwordManager.js
@@ -198,6 +198,16 @@ if (chrome.contentSettings.passwordManager == 'allow') {
     // Last resort: find the first text input in the form
     username = username || form.querySelector('input[type=text i]')
 
+    // If the username turns out to be a password field, just ignore it so
+    // we don't show the password in plaintext.
+    if (username) {
+      let autocomplete = username.getAttribute('autocomplete')
+      if (username.getAttribute('type') === 'password' ||
+        (autocomplete && autocomplete.includes('password'))) {
+        username = null
+      }
+    }
+
     // If not a submission, autofill the first password field and ignore the rest
     if (!isSubmission || passwords.length === 1) {
       return [username instanceof HTMLInputElement ? username : null, passwords[0], null]

diff --git a/test/components/notificationBarTest.js b/test/components/notificationBarTest.js
@@ -3,7 +3,7 @@
 const Brave = require('../lib/brave')
 const {notificationBar, titleBar, urlInput} = require('../lib/selectors')
 
-describe('notificationBar', function () {
+describe('notificationBar permissions', function () {
   function * setup (client) {
     yield client
       .waitForBrowserWindow()
@@ -14,11 +14,6 @@ describe('notificationBar', function () {
   Brave.beforeAll(this)
   before(function * () {
     this.notificationUrl = Brave.server.url('notification.html')
-    this.loginUrl1 = Brave.server.url('login1.html')
-    this.loginUrl2 = Brave.server.url('login2.html')
-    this.loginUrl3 = Brave.server.url('login3.html')
-    this.loginUrl4 = Brave.server.url('login4.html')
-    this.loginUrl5 = Brave.server.url('login5.html')
     yield setup(this.app.client)
   })
 
@@ -93,22 +88,62 @@ describe('notificationBar', function () {
         return this.getText(titleBar).then((val) => val.includes('granted'))
       })
   })
+})
+
+describe('notificationBar passwords', function () {
+  function * setup (client) {
+    yield client
+      .waitForBrowserWindow()
+      .waitForVisible(urlInput)
+  }
+
+  Brave.beforeAll(this)
+
+  beforeEach(function * () {
+    yield this.app.client
+      .waitForElementCount('.notificationItem', 0)
+  })
+
+  before(function * () {
+    this.loginUrl1 = Brave.server.url('login1.html')
+    this.loginUrl2 = Brave.server.url('login2.html')
+    this.loginUrl3 = Brave.server.url('login3.html')
+    this.loginUrl4 = Brave.server.url('login4.html')
+    this.loginUrl5 = Brave.server.url('login5.html')
+    this.loginUrl6 = Brave.server.url('login6.html')
+    yield setup(this.app.client)
+  })
 
   it('shows notification for login form', function * () {
     yield this.app.client
       .tabByIndex(0)
-      .loadUrl(this.loginUrl1)
+      .url(this.loginUrl1)
       .windowByUrl(Brave.browserWindowUrl)
       .waitForExist(notificationBar)
       .waitUntil(function () {
-        return this.getText(notificationBar).then((val) => val.includes('localhost') && val.includes('brave_user'))
+        return this.getText(notificationBar).then((val) => {
+          return val.includes('localhost') && val.includes('brave_user')
+        })
+      }).click('button=No')
+  })
+
+  it('does not include a password in the notification bar', function * () {
+    yield this.app.client
+      .tabByIndex(0)
+      .url(this.loginUrl6)
+      .windowByUrl(Brave.browserWindowUrl)
+      .waitForExist(notificationBar)
+      .waitUntil(function () {
+        return this.getText(notificationBar).then((val) => {
+          return val.includes('your password') && !val.includes('secret')
+        })
       }).click('button=No')
   })
 
   it('autofills remembered password on login form', function * () {
     yield this.app.client
       .tabByIndex(0)
-      .loadUrl(this.loginUrl1)
+      .url(this.loginUrl1)
       .windowByUrl(Brave.browserWindowUrl)
       .waitForExist(notificationBar)
       .waitUntil(function () {
@@ -119,7 +154,7 @@ describe('notificationBar', function () {
       .waitForExist('[data-test-id="passwordItem"]')
       .windowByUrl(Brave.browserWindowUrl)
       .tabByIndex(0)
-      .loadUrl(this.loginUrl4)
+      .url(this.loginUrl4)
       .waitUntil(function () {
         return this.getValue('#user').then((val) => val === 'brave_user') &&
           this.getValue('#password').then((val) => val === 'testing') &&
@@ -131,10 +166,7 @@ describe('notificationBar', function () {
   it('autofills remembered password on login page with multiple forms', function * () {
     yield this.app.client
       .tabByIndex(0)
-      .loadUrl(this.loginUrl1)
-      .windowByUrl(Brave.browserWindowUrl)
-      .tabByIndex(0)
-      .loadUrl(this.loginUrl5)
+      .url(this.loginUrl5)
       .waitUntil(function () {
         return this.getValue('#user').then((val) => val === 'brave_user') &&
           this.getValue('#password').then((val) => val === 'testing') &&
@@ -146,15 +178,15 @@ describe('notificationBar', function () {
   it('does not show login form notification if user turns it off for the site', function * () {
     yield this.app.client
       .tabByIndex(0)
-      .loadUrl(this.loginUrl3)
+      .url(this.loginUrl3)
       .windowByUrl(Brave.browserWindowUrl)
       .waitForExist(notificationBar)
       .waitUntil(function () {
         return this.getText(notificationBar).then((val) => val.includes('localhost') && val.includes('brave_user'))
       })
       .click('button=Never for this site')
       .tabByIndex(0)
-      .loadUrl(this.loginUrl2)
+      .url(this.loginUrl2)
       .windowByUrl(Brave.browserWindowUrl)
       .isExisting(notificationBar).should.eventually.be.false
   })

diff --git a/test/fixtures/login1.html b/test/fixtures/login1.html
@@ -9,13 +9,13 @@
               document.querySelector('#password').value = 'testing'
               document.querySelector('#submit').click()
           }
-      }, 1000)
+      }, 200)
   </script>
 </head>
 <body>
   <div id="content" class="login">
     <h1>Login</h1>
-    <form method="post" id="acctmgr_loginform" action="/"><div><input type="hidden" name="__FORM_TOKEN" value="foo" /></div>
+    <form method="post" id="acctmgr_loginform" action="login4.html"><div><input type="hidden" name="__FORM_TOKEN" value="foo" /></div>
       <div>
         <input type="hidden" name="referer" value="bar" />
       </div>

diff --git a/test/fixtures/login2.html b/test/fixtures/login2.html
@@ -9,13 +9,13 @@
               document.querySelector('#password').value = 'testing'
               document.querySelector('#submit').click()
           }
-      }, 1000)
+      }, 200)
   </script>
 </head>
 <body>
   <div id="content" class="login">
     <h1>Login</h1>
-    <form method="post" id="acctmgr_loginform" action=""><div><input type="hidden" name="__FORM_TOKEN" value="foo" /></div>
+    <form method="post" id="acctmgr_loginform" action="login4.html"><div><input type="hidden" name="__FORM_TOKEN" value="foo" /></div>
       <div>
         <input type="hidden" name="referer" value="bar" />
       </div>

diff --git a/test/fixtures/login3.html b/test/fixtures/login3.html
@@ -9,13 +9,13 @@
               document.querySelector('#password').value = 'testing'
               document.querySelector('#submit').click()
           }
-      }, 1000)
+      }, 200)
   </script>
 </head>
 <body>
   <div id="content" class="login">
     <h1>Login</h1>
-    <form method="post" id="acctmgr_loginform" action=""><div><input type="hidden" name="__FORM_TOKEN" value="foo" /></div>
+    <form method="post" id="acctmgr_loginform" action="login5.html"><div><input type="hidden" name="__FORM_TOKEN" value="foo" /></div>
       <div>
         <input type="hidden" name="referer" value="bar" />
       </div>

diff --git a/test/fixtures/login4.html b/test/fixtures/login4.html
@@ -6,7 +6,7 @@
 <body>
   <div id="content" class="login">
     <h1>Login</h1>
-    <form method="post" id="acctmgr_loginform" action="/"><div><input type="hidden" name="__FORM_TOKEN" value="foo" /></div>
+    <form method="post" id="acctmgr_loginform" action="login4.html"><div><input type="hidden" name="__FORM_TOKEN" value="foo" /></div>
       <div>
         <input type="hidden" name="referer" value="bar" />
       </div>
@@ -25,7 +25,7 @@ <h1>Login</h1>
   </div>
   <div id="content2" class="login">
     <h1>Login 2</h1>
-    <form method="post" action="/blah"><div><input type="hidden" name="__FORM_TOKEN" value="foo" /></div>
+    <form method="post" action="https://brave.com/"><div><input type="hidden" name="__FORM_TOKEN" value="foo" /></div>
       <div class="textbox">
         <label for="user">Username:</label><br />
         <input type="text" name="user" id='user2' class="textwidget" />

diff --git a/test/fixtures/login5.html b/test/fixtures/login5.html
@@ -6,7 +6,7 @@
 <body>
   <div id="content1" class="login">
     <h1>Login 1</h1>
-    <form method="post" action="/"><div><input type="hidden" name="__FORM_TOKEN" value="foo" /></div>
+    <form method="post" action="login4.html"><div><input type="hidden" name="__FORM_TOKEN" value="foo" /></div>
       <div class="textbox">
         <label for="user">Username:</label><br />
         <input type="text" id="user" name="user" class="textwidget" />
@@ -22,7 +22,7 @@ <h1>Login 1</h1>
   </div>
   <div id="content2" class="login">
     <h1>Login 2</h1>
-    <form method="post" action="/"><div><input type="hidden" name="__FORM_TOKEN" value="foo" /></div>
+    <form method="post" action="login4.html"><div><input type="hidden" name="__FORM_TOKEN" value="foo" /></div>
       <div class="textbox">
         <label for="user">Username:</label><br />
         <input type="text" name="user" id='user2' class="textwidget" />

diff --git a/test/fixtures/login6.html b/test/fixtures/login6.html
@@ -0,0 +1,21 @@
+<body>
+  <script>
+      window.onload = window.setTimeout(function () {
+          if (!document.querySelector('#password').value) {
+              document.querySelector('#password').value = 'secret'
+              document.querySelector('#old-password').value = 'secret'
+              document.querySelector('#new-password').value = 'secret2'
+              document.querySelector('#submit').click()
+          }
+      }, 200)
+  </script>
+<form action="/login4.html" name="ChangePassForm" id="ChangePassForm">
+    <input type="hidden" id="countryId" name="countryId" value="1" />
+    <input type="hidden" name="MfcISAPICommand" value="HandleNewPassword">
+    <input type="hidden" name="srt" value="01"><div class="rclSir"></div>
+    <input id='password' type="password" name="user" />
+    <input id='old-password' type="password" autocomplete="current-password" />
+    <input id='new-password' type="password" autocomplete="new-password" />
+    <input id="submit" type="submit" value="Change password" />
+</form>
+</body>