Bug 1582534 - about:logins: Always set the password value with .value so it's not easily visible in the Inspector. r=jaws a=lizzard
authorMatthew Noorenberghe <mozilla@noorenberghe.ca>
Sat, 21 Sep 2019 04:06:57 +0000
changeset 555227 d855eaf4028bdd96b0a74600ede3e7509ac0b663
parent 555226 861f24a4c2f8056563855fefa841f6c6651f6387
child 555228 ff9d611b5c723c071d39362b9693dda0089bb264
push id2165
push userffxbld-merge
push dateMon, 14 Oct 2019 16:30:58 +0000
treeherdermozilla-release@0eae18af659f [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjaws, lizzard
bugs1582534
milestone70.0
Bug 1582534 - about:logins: Always set the password value with .value so it's not easily visible in the Inspector. r=jaws a=lizzard The previous approach of using space characters would sometimes end up causing spaces to get saved in storage and cause data loss. Differential Revision: https://phabricator.services.mozilla.com/D46663
browser/components/aboutlogins/content/components/login-item.js
browser/components/aboutlogins/tests/browser/browser_masterPassword.js
browser/components/aboutlogins/tests/browser/browser_updateLogin.js
browser/components/aboutlogins/tests/chrome/test_login_item.html
--- a/browser/components/aboutlogins/content/components/login-item.js
+++ b/browser/components/aboutlogins/content/components/login-item.js
@@ -147,17 +147,25 @@ export default class LoginItem extends H
       this._favicon.src = "";
       this._favicon.hidden = true;
       this._faviconWrapper.classList.remove("hide-default-favicon");
     }
 
     this._title.textContent = this._login.title;
     this._originInput.defaultValue = this._login.origin || "";
     this._usernameInput.defaultValue = this._login.username || "";
-    // The password gets filled in _updatePasswordRevealState
+    if (this._login.password) {
+      // We use .value instead of .defaultValue since the latter updates the
+      // content attribute making the password easily viewable with Inspect
+      // Element even when Master Password is enabled. This is only run when
+      // the password is non-empty since setting the field to an empty value
+      // would mark the field as 'dirty' for form validation and thus trigger
+      // the error styling since the password field is 'required'.
+      this._passwordInput.value = this._login.password;
+    }
 
     if (this.dataset.editing) {
       this._usernameInput.removeAttribute("data-l10n-id");
       this._usernameInput.placeholder = "";
     } else {
       document.l10n.setAttributes(
         this._usernameInput,
         "about-logins-login-item-username"
@@ -248,18 +256,17 @@ export default class LoginItem extends H
         if (!originValue.match(/:\/\//)) {
           this._originInput.value = "https://" + originValue;
         }
         break;
       }
       case "click": {
         let classList = event.currentTarget.classList;
         if (classList.contains("reveal-password-checkbox")) {
-          // We prompt for the master password when entering edit mode already.
-          if (this._revealCheckbox.checked && !this.dataset.editing) {
+          if (this._revealCheckbox.checked && !this.dataset.isNewLogin) {
             let masterPasswordAuth = await new Promise(resolve => {
               window.AboutLoginsUtils.promptForMasterPassword(resolve);
             });
             if (!masterPasswordAuth) {
               this._revealCheckbox.checked = false;
               return;
             }
           }
@@ -355,23 +362,16 @@ export default class LoginItem extends H
           });
           return;
         }
         if (classList.contains("dismiss-breach-alert")) {
           this.dismissBreachAlert();
           return;
         }
         if (classList.contains("edit-button")) {
-          let masterPasswordAuth = await new Promise(resolve => {
-            window.AboutLoginsUtils.promptForMasterPassword(resolve);
-          });
-          if (!masterPasswordAuth) {
-            return;
-          }
-
           this._toggleEditing();
           this.render();
 
           this._recordTelemetryEvent({
             object: "existing_login",
             method: "edit",
           });
           return;
@@ -602,19 +602,18 @@ export default class LoginItem extends H
    * @param {login} login The login that was removed from storage. The login object is
    *                      a plain JS object representation of nsILoginInfo/nsILoginMetaInfo.
    */
   loginRemoved(login) {
     if (login.guid != this._login.guid) {
       return;
     }
 
-    this._login = {};
+    this.setLogin({}, { skipFocusChange: true });
     this._toggleEditing(false);
-    this.render();
   }
 
   _handleOriginClick() {
     document.dispatchEvent(
       new CustomEvent("AboutLoginsOpenSite", {
         bubbles: true,
         detail: this._login,
       })
@@ -714,19 +713,11 @@ export default class LoginItem extends H
     let titleId = this._revealCheckbox.checked
       ? "login-item-password-reveal-checkbox-hide"
       : "login-item-password-reveal-checkbox-show";
     document.l10n.setAttributes(this._revealCheckbox, titleId);
 
     let { checked } = this._revealCheckbox;
     let inputType = checked ? "text" : "password";
     this._passwordInput.type = inputType;
-    // Don't include the password value in the attribute when it's supposed to be
-    // masked so that it's not trivial to bypass the Master Password prompt with
-    // the inspector in devtools.
-    let password = this._login.password || "";
-    // We prompt for the master password before entering edit mode so we can use
-    // the password in the markup then.
-    this._passwordInput.defaultValue =
-      checked || this.dataset.editing ? password : " ".repeat(password.length);
   }
 }
 customElements.define("login-item", LoginItem);
--- a/browser/components/aboutlogins/tests/browser/browser_masterPassword.js
+++ b/browser/components/aboutlogins/tests/browser/browser_masterPassword.js
@@ -122,17 +122,17 @@ add_task(async function test() {
       ".copy-password-button"
     );
     await ContentTaskUtils.waitForCondition(() => {
       return copyButton.disabled;
     }, "Waiting for copy button to be disabled");
     info("Password was copied to clipboard");
   });
 
-  // Show MP dialog when Reveal Password checkbox is checked
+  // Show MP dialog when Reveal Password checkbox is checked if not on a new login
   mpDialogShown = waitForMPDialog("cancel");
   await ContentTask.spawn(gBrowser.selectedBrowser, null, async function() {
     let loginItem = content.document.querySelector("login-item");
     let revealCheckbox = loginItem.shadowRoot.querySelector(
       ".reveal-password-checkbox"
     );
     revealCheckbox.click();
   });
@@ -164,16 +164,44 @@ add_task(async function test() {
       ".reveal-password-checkbox"
     );
     ok(
       revealCheckbox.checked,
       "reveal checkbox should be checked if MP dialog authenticated"
     );
   });
 
+  info("Test toggling the password visibility on a new login");
+  await ContentTask.spawn(browser, null, async function createNewToggle() {
+    let createButton = content.document
+      .querySelector("login-list")
+      .shadowRoot.querySelector(".create-login-button");
+    createButton.click();
+
+    let loginItem = Cu.waiveXrays(content.document.querySelector("login-item"));
+    let passwordField = loginItem.shadowRoot.querySelector(
+      "input[name='password']"
+    );
+    let revealCheckbox = loginItem.shadowRoot.querySelector(
+      ".reveal-password-checkbox"
+    );
+    ok(ContentTaskUtils.is_visible(revealCheckbox), "Toggle visible");
+    ok(!revealCheckbox.checked, "Not revealed initially");
+    is(passwordField.type, "password", "type is password");
+    revealCheckbox.click();
+
+    await ContentTaskUtils.waitForCondition(() => {
+      return passwordField.type == "text";
+    }, "Waiting for type='text'");
+    ok(revealCheckbox.checked, "Not revealed after click");
+
+    let cancelButton = loginItem.shadowRoot.querySelector(".cancel-button");
+    cancelButton.click();
+  });
+
   await ContentTask.spawn(gBrowser.selectedBrowser, null, async function() {
     let loginFilter = Cu.waiveXrays(
       content.document.querySelector("login-filter")
     );
     loginFilter.value = "pass1";
     let loginList = Cu.waiveXrays(content.document.querySelector("login-list"));
     is(
       loginList._list.querySelectorAll(
--- a/browser/components/aboutlogins/tests/browser/browser_updateLogin.js
+++ b/browser/components/aboutlogins/tests/browser/browser_updateLogin.js
@@ -101,19 +101,23 @@ add_task(async function test_login_item(
 
         is(
           usernameInput.value,
           login.username,
           "Username change should be reverted"
         );
         is(
           passwordInput.value,
-          " ".repeat(login.password.length),
+          login.password,
           "Password change should be reverted"
         );
+        ok(
+          !passwordInput.hasAttribute("value"),
+          "Password shouldn't be exposed in @value"
+        );
         is(
           passwordInput.style.width,
           login.password.length + "ch",
           "Password field width shouldn't have changed"
         );
       }
 
       await test_discard_dialog(loginList._createLoginButton);
@@ -135,32 +139,30 @@ add_task(async function test_login_item(
       ok(
         revealCheckbox.checked,
         "reveal-checkbox should be checked after clicking"
       );
 
       usernameInput.value += "-saveme";
       passwordInput.value += "-saveme";
 
-      // Cache the value since it will change upon leaving edit mode.
-      let passwordInputValue = passwordInput.value;
       ok(loginItem.dataset.editing, "LoginItem should be in 'edit' mode");
 
       let saveChangesButton = loginItem.shadowRoot.querySelector(
         ".save-changes-button"
       );
       saveChangesButton.click();
 
       await ContentTaskUtils.waitForCondition(() => {
         let guid = loginList._loginGuidsSortedOrder[0];
         let updatedLogin = loginList._logins[guid].login;
         return (
           updatedLogin &&
           updatedLogin.username == usernameInput.value &&
-          updatedLogin.password == passwordInputValue
+          updatedLogin.password == passwordInput.value
         );
       }, "Waiting for corresponding login in login list to update");
 
       ok(
         !revealCheckbox.checked,
         "reveal-checkbox should be unchecked after saving changes"
       );
       ok(
--- a/browser/components/aboutlogins/tests/chrome/test_login_item.html
+++ b/browser/components/aboutlogins/tests/chrome/test_login_item.html
@@ -84,17 +84,19 @@ add_task(async function test_set_login()
 
   ok(!gLoginItem.dataset.editing, "loginItem should not be in 'edit' mode");
   ok(!gLoginItem.dataset.isNewLogin, "loginItem should not be in 'isNewLogin' mode");
   let originInput = gLoginItem.shadowRoot.querySelector("input[name='origin']");
   is(originInput.value, TEST_LOGIN_1.origin, "origin should be populated");
   let usernameInput = gLoginItem.shadowRoot.querySelector("input[name='username']");
   is(usernameInput.value, TEST_LOGIN_1.username, "username should be populated");
   is(document.l10n.getAttributes(usernameInput).id, "about-logins-login-item-username", "username field should have default placeholder when not editing");
-  is(gLoginItem.shadowRoot.querySelector("input[name='password']").value.length, TEST_LOGIN_1.password.length, "password mask text should be populated");
+  let passwordInput = gLoginItem.shadowRoot.querySelector("input[name='password']");
+  is(passwordInput.value, TEST_LOGIN_1.password, "password should be populated");
+  ok(!passwordInput.hasAttribute("value"), "Password shouldn't be exposed in @value");
   is(document.l10n.getAttributes(gLoginItem.shadowRoot.querySelector(".time-created")).args.timeCreated, TEST_LOGIN_1.timeCreated, "time-created should be populated");
   is(document.l10n.getAttributes(gLoginItem.shadowRoot.querySelector(".time-changed")).args.timeChanged, TEST_LOGIN_1.timePasswordChanged, "time-changed should be populated");
   is(document.l10n.getAttributes(gLoginItem.shadowRoot.querySelector(".time-used")).args.timeUsed, TEST_LOGIN_1.timeLastUsed, "time-used should be populated");
   let copyButtons = [...gLoginItem.shadowRoot.querySelectorAll(".copy-button")];
   ok(copyButtons.every(button => !isHidden(button)), "The copy buttons should be visible when viewing a login");
 
   let openSiteEventDispatched = false;
   document.addEventListener("AboutLoginsOpenSite", event => {
@@ -253,16 +255,17 @@ add_task(async function test_set_login_e
   ok(gLoginItem.dataset.editing, "loginItem should be in 'edit' mode");
   ok(isHidden(gLoginItem.shadowRoot.querySelector(".edit-button")), "edit button should be hidden in 'edit' mode");
   ok(gLoginItem.dataset.isNewLogin, "loginItem should be in 'isNewLogin' mode");
   let deleteButton = gLoginItem.shadowRoot.querySelector(".delete-button");
   ok(deleteButton.disabled, "Delete button should be disabled when creating a login");
   is(gLoginItem.shadowRoot.querySelector("input[name='origin']").value, "", "origin should be empty");
   is(gLoginItem.shadowRoot.querySelector("input[name='username']").value, "", "username should be empty");
   is(gLoginItem.shadowRoot.querySelector("input[name='password']").value, "", "password should be empty");
+  ok(!gLoginItem.shadowRoot.querySelector("input[name='password']").hasAttribute("value"), "Password shouldn't be exposed in @value");
   is(document.l10n.getAttributes(gLoginItem.shadowRoot.querySelector(".time-created")).args.timeCreated, "", "time-created should be blank when undefined");
   is(document.l10n.getAttributes(gLoginItem.shadowRoot.querySelector(".time-changed")).args.timeChanged, "", "time-changed should be blank when undefined");
   is(document.l10n.getAttributes(gLoginItem.shadowRoot.querySelector(".time-used")).args.timeUsed, "", "time-used should be blank when undefined");
   let copyButtons = [...gLoginItem.shadowRoot.querySelectorAll(".copy-button")];
   ok(copyButtons.every(button => isHidden(button)), "The copy buttons should be hidden when creating a login");
 
   let createEventDispatched = false;
   document.addEventListener("AboutLoginsCreateLogin", event => {
@@ -307,58 +310,62 @@ add_task(async function test_set_login_e
 add_task(async function test_different_login_modified() {
   gLoginItem.setLogin(TEST_LOGIN_1);
   let otherLogin = Object.assign({}, TEST_LOGIN_1, {username: "fakeuser", guid: "fakeguid"});
   gLoginItem.loginModified(otherLogin);
   await asyncElementRendered();
 
   is(gLoginItem.shadowRoot.querySelector("input[name='origin']").value, TEST_LOGIN_1.origin, "origin should be unchanged");
   is(gLoginItem.shadowRoot.querySelector("input[name='username']").value, TEST_LOGIN_1.username, "username should be unchanged");
-  is(gLoginItem.shadowRoot.querySelector("input[name='password']").value, " ".repeat(TEST_LOGIN_1.password.length), "password length should be unchanged");
+  is(gLoginItem.shadowRoot.querySelector("input[name='password']").value, TEST_LOGIN_1.password, "password should be unchanged");
+  ok(!gLoginItem.shadowRoot.querySelector("input[name='password']").hasAttribute("value"), "Password shouldn't be exposed in @value");
   is(document.l10n.getAttributes(gLoginItem.shadowRoot.querySelector(".time-created")).args.timeCreated, TEST_LOGIN_1.timeCreated, "time-created should be unchanged");
   is(document.l10n.getAttributes(gLoginItem.shadowRoot.querySelector(".time-changed")).args.timeChanged, TEST_LOGIN_1.timePasswordChanged, "time-changed should be unchanged");
   is(document.l10n.getAttributes(gLoginItem.shadowRoot.querySelector(".time-used")).args.timeUsed, TEST_LOGIN_1.timeLastUsed, "time-used should be unchanged");
 });
 
 add_task(async function test_different_login_removed() {
   gLoginItem.setLogin(TEST_LOGIN_1);
   let otherLogin = Object.assign({}, TEST_LOGIN_1, {username: "fakeuser", guid: "fakeguid"});
   gLoginItem.loginRemoved(otherLogin);
   await asyncElementRendered();
 
   is(gLoginItem.shadowRoot.querySelector("input[name='origin']").value, TEST_LOGIN_1.origin, "origin should be unchanged");
   is(gLoginItem.shadowRoot.querySelector("input[name='username']").value, TEST_LOGIN_1.username, "username should be unchanged");
-  is(gLoginItem.shadowRoot.querySelector("input[name='password']").value, " ".repeat(TEST_LOGIN_1.password.length), "password length should be unchanged");
+  is(gLoginItem.shadowRoot.querySelector("input[name='password']").value, TEST_LOGIN_1.password, "password should be unchanged");
+  ok(!gLoginItem.shadowRoot.querySelector("input[name='password']").hasAttribute("value"), "Password shouldn't be exposed in @value");
   is(document.l10n.getAttributes(gLoginItem.shadowRoot.querySelector(".time-created")).args.timeCreated, TEST_LOGIN_1.timeCreated, "time-created should be unchanged");
   is(document.l10n.getAttributes(gLoginItem.shadowRoot.querySelector(".time-changed")).args.timeChanged, TEST_LOGIN_1.timePasswordChanged, "time-changed should be unchanged");
   is(document.l10n.getAttributes(gLoginItem.shadowRoot.querySelector(".time-used")).args.timeUsed, TEST_LOGIN_1.timeLastUsed, "time-used should be unchanged");
 });
 
 add_task(async function test_login_modified() {
   gLoginItem.setLogin(TEST_LOGIN_1);
   let modifiedLogin = Object.assign({}, TEST_LOGIN_1, {username: "updateduser"});
   gLoginItem.loginModified(modifiedLogin);
   await asyncElementRendered();
 
   is(gLoginItem.shadowRoot.querySelector("input[name='origin']").value, modifiedLogin.origin, "origin should be updated");
   is(gLoginItem.shadowRoot.querySelector("input[name='username']").value, modifiedLogin.username, "username should be updated");
-  is(gLoginItem.shadowRoot.querySelector("input[name='password']").value, " ".repeat(modifiedLogin.password.length), "password length should be updated");
+  is(gLoginItem.shadowRoot.querySelector("input[name='password']").value, modifiedLogin.password, "password should be updated");
+  ok(!gLoginItem.shadowRoot.querySelector("input[name='password']").hasAttribute("value"), "Password shouldn't be exposed in @value");
   is(document.l10n.getAttributes(gLoginItem.shadowRoot.querySelector(".time-created")).args.timeCreated, modifiedLogin.timeCreated, "time-created should be updated");
   is(document.l10n.getAttributes(gLoginItem.shadowRoot.querySelector(".time-changed")).args.timeChanged, modifiedLogin.timePasswordChanged, "time-changed should be updated");
   is(document.l10n.getAttributes(gLoginItem.shadowRoot.querySelector(".time-used")).args.timeUsed, modifiedLogin.timeLastUsed, "time-used should be updated");
 });
 
 add_task(async function test_login_removed() {
   gLoginItem.setLogin(TEST_LOGIN_1);
   gLoginItem.loginRemoved(TEST_LOGIN_1);
   await asyncElementRendered();
 
   is(gLoginItem.shadowRoot.querySelector("input[name='origin']").value, "", "origin should be cleared");
   is(gLoginItem.shadowRoot.querySelector("input[name='username']").value, "", "username should be cleared");
   is(gLoginItem.shadowRoot.querySelector("input[name='password']").value, "", "password should be cleared");
+  ok(!gLoginItem.shadowRoot.querySelector("input[name='password']").hasAttribute("value"), "Password shouldn't be exposed in @value");
   is(document.l10n.getAttributes(gLoginItem.shadowRoot.querySelector(".time-created")).args.timeCreated, "", "time-created should be blank when undefined");
   is(document.l10n.getAttributes(gLoginItem.shadowRoot.querySelector(".time-changed")).args.timeChanged, "", "time-changed should be blank when undefined");
   is(document.l10n.getAttributes(gLoginItem.shadowRoot.querySelector(".time-used")).args.timeUsed, "", "time-used should be blank when undefined");
 });
 
 </script>
 
 </body>