Bug 1579512 - Don't include the real password in the markup of about:logins when masked. r=jaws
authorMatthew Noorenberghe <mozilla@noorenberghe.ca>
Mon, 09 Sep 2019 22:42:32 +0000
changeset 551851 f56f3e4f6702f2c3eaff5641459b9beac5e6af31
parent 551850 5e859feee83991b248f6ed7e416d9cd1d72b5ce7
child 551852 f1017bf062e2ad872a08a0a04fac322c6e97491e
push id11975
push userjwein@mozilla.com
push dateTue, 17 Sep 2019 20:23:55 +0000
treeherdermozilla-beta@1a169f1e113d [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjaws
bugs1579512
milestone70.0
Bug 1579512 - Don't include the real password in the markup of about:logins when masked. r=jaws Differential Revision: https://phabricator.services.mozilla.com/D45075
browser/components/aboutlogins/content/aboutLogins.html
browser/components/aboutlogins/content/components/login-item.js
browser/components/aboutlogins/tests/browser/browser_aaa_eventTelemetry_run_first.js
browser/components/aboutlogins/tests/browser/browser_openSite.js
browser/components/aboutlogins/tests/browser/browser_updateLogin.js
browser/components/aboutlogins/tests/browser/head.js
browser/components/aboutlogins/tests/chrome/test_login_item.html
--- a/browser/components/aboutlogins/content/aboutLogins.html
+++ b/browser/components/aboutlogins/content/aboutLogins.html
@@ -193,16 +193,17 @@
         </div>
         <div class="detail-row">
           <div class="detail-row-contents">
             <label class="detail-cell">
               <span class="password-label field-label" data-l10n-id="login-item-password-label"></span>
               <div class="reveal-password-wrapper">
                 <input type="password"
                        name="password"
+                       autocomplete="off"
                        dir="ltr"
                        required
                        readonly/>
                 <input type="checkbox"
                        class="reveal-password-checkbox"
                        data-l10n-id="login-item-password-reveal-checkbox"/>
               </div>
             </label>
--- a/browser/components/aboutlogins/content/components/login-item.js
+++ b/browser/components/aboutlogins/content/components/login-item.js
@@ -141,17 +141,18 @@ export default class LoginItem extends H
       // reset the src and alt attributes if the currently selected favicon doesn't have a favicon
       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 || "";
-    this._passwordInput.defaultValue = this._login.password || "";
+    // The password gets filled in _updatePasswordRevealState
+
     if (this.dataset.editing) {
       this._usernameInput.removeAttribute("data-l10n-id");
       this._usernameInput.placeholder = "";
     } else {
       document.l10n.setAttributes(
         this._usernameInput,
         "about-logins-login-item-username"
       );
@@ -241,17 +242,18 @@ 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")) {
-          if (this._revealCheckbox.checked) {
+          // We prompt for the master password when entering edit mode already.
+          if (this._revealCheckbox.checked && !this.dataset.editing) {
             let masterPasswordAuth = await new Promise(resolve => {
               window.AboutLoginsUtils.promptForMasterPassword(resolve);
             });
             if (!masterPasswordAuth) {
               this._revealCheckbox.checked = false;
               return;
             }
           }
@@ -339,16 +341,23 @@ 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;
@@ -675,12 +684,20 @@ export default class LoginItem extends H
   _updatePasswordRevealState() {
     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.setAttribute("type", inputType);
+    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_aaa_eventTelemetry_run_first.js
+++ b/browser/components/aboutlogins/tests/browser/browser_aaa_eventTelemetry_run_first.js
@@ -74,17 +74,17 @@ add_task(async function test_telemetry_e
       ".copy-password-button"
     );
     copyButton.click();
   });
   await waitForTelemetryEventCount(3);
 
   let promiseNewTab = BrowserTestUtils.waitForNewTab(
     gBrowser,
-    TEST_LOGIN2.origin
+    TEST_LOGIN2.origin + "/"
   );
   await ContentTask.spawn(gBrowser.selectedBrowser, null, async function() {
     let loginItem = content.document.querySelector("login-item");
     let originInput = loginItem.shadowRoot.querySelector(".origin-input");
     originInput.click();
   });
   let newTab = await promiseNewTab;
   ok(true, "New tab opened to " + TEST_LOGIN2.origin);
--- a/browser/components/aboutlogins/tests/browser/browser_openSite.js
+++ b/browser/components/aboutlogins/tests/browser/browser_openSite.js
@@ -11,17 +11,17 @@ add_task(async function setup() {
     BrowserTestUtils.removeTab(gBrowser.selectedTab);
     Services.logins.removeAllLogins();
   });
 });
 
 add_task(async function test_launch_login_item() {
   let promiseNewTab = BrowserTestUtils.waitForNewTab(
     gBrowser,
-    TEST_LOGIN1.origin
+    TEST_LOGIN1.origin + "/"
   );
 
   let browser = gBrowser.selectedBrowser;
   await ContentTask.spawn(
     browser,
     LoginHelper.loginToVanillaObject(TEST_LOGIN1),
     async login => {
       let loginItem = Cu.waiveXrays(
--- a/browser/components/aboutlogins/tests/browser/browser_updateLogin.js
+++ b/browser/components/aboutlogins/tests/browser/browser_updateLogin.js
@@ -58,16 +58,20 @@ add_task(async function test_login_item(
       let passwordInput = loginItem.shadowRoot.querySelector(
         "input[name='password']"
       );
 
       let editButton = loginItem.shadowRoot.querySelector(".edit-button");
 
       async function test_discard_dialog(exitPoint) {
         editButton.click();
+        await ContentTaskUtils.waitForCondition(
+          () => loginItem.dataset.editing,
+          "Entering edit mode"
+        );
         await Promise.resolve();
 
         usernameInput.value += "-undome";
         passwordInput.value += "-undome";
 
         let dialog = content.document.querySelector("confirmation-dialog");
         ok(dialog.hidden, "Confirm dialog should initially be hidden");
 
@@ -97,60 +101,66 @@ add_task(async function test_login_item(
 
         is(
           usernameInput.value,
           login.username,
           "Username change should be reverted"
         );
         is(
           passwordInput.value,
-          login.password,
+          " ".repeat(login.password.length),
           "Password change should be reverted"
         );
         is(
           passwordInput.style.width,
           login.password.length + "ch",
           "Password field width shouldn't have changed"
         );
       }
 
       await test_discard_dialog(loginList._createLoginButton);
 
       let cancelButton = loginItem.shadowRoot.querySelector(".cancel-button");
       await test_discard_dialog(cancelButton);
 
       editButton.click();
+      await ContentTaskUtils.waitForCondition(
+        () => loginItem.dataset.editing,
+        "Entering edit mode"
+      );
       await Promise.resolve();
 
       let revealCheckbox = loginItem.shadowRoot.querySelector(
         ".reveal-password-checkbox"
       );
       revealCheckbox.click();
       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 == passwordInput.value
+          updatedLogin.password == passwordInputValue
         );
       }, "Waiting for corresponding login in login list to update");
 
       ok(
         !revealCheckbox.checked,
         "reveal-checkbox should be unchecked after saving changes"
       );
       ok(
@@ -159,16 +169,20 @@ add_task(async function test_login_item(
       );
       is(
         passwordInput.style.width,
         passwordInput.value.length + "ch",
         "Password field width should be correctly updated"
       );
 
       editButton.click();
+      await ContentTaskUtils.waitForCondition(
+        () => loginItem.dataset.editing,
+        "Entering edit mode"
+      );
       await Promise.resolve();
 
       ok(loginItem.dataset.editing, "LoginItem should be in 'edit' mode");
       let deleteButton = loginItem.shadowRoot.querySelector(".delete-button");
       deleteButton.click();
       let confirmDeleteDialog = Cu.waiveXrays(
         content.document.querySelector("confirmation-dialog")
       );
--- a/browser/components/aboutlogins/tests/browser/head.js
+++ b/browser/components/aboutlogins/tests/browser/head.js
@@ -3,37 +3,37 @@
 
 let nsLoginInfo = new Components.Constructor(
   "@mozilla.org/login-manager/loginInfo;1",
   Ci.nsILoginInfo,
   "init"
 );
 
 let TEST_LOGIN1 = new nsLoginInfo(
-  "https://example.com/",
-  "https://example.com/",
+  "https://example.com",
+  "https://example.com",
   null,
   "user1",
   "pass1",
   "username",
   "password"
 );
 let TEST_LOGIN2 = new nsLoginInfo(
-  "https://2.example.com/",
-  "https://2.example.com/",
+  "https://2.example.com",
+  "https://2.example.com",
   null,
   "user2",
   "pass2",
   "username",
   "password"
 );
 
 let TEST_LOGIN3 = new nsLoginInfo(
-  "https://breached.com/",
-  "https://breached.com/",
+  "https://breached.com",
+  "https://breached.com",
   null,
   "breachedLogin1",
   "pass3",
   "breachedLogin",
   "password"
 );
 
 async function addLogin(login) {
--- a/browser/components/aboutlogins/tests/chrome/test_login_item.html
+++ b/browser/components/aboutlogins/tests/chrome/test_login_item.html
@@ -84,17 +84,17 @@ 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, TEST_LOGIN_1.password, "password should be populated");
+  is(gLoginItem.shadowRoot.querySelector("input[name='password']").value.length, TEST_LOGIN_1.password.length, "password mask text should be populated");
   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 => {
@@ -121,16 +121,17 @@ add_task(async function test_set_login()
   gLoginItem.setLogin(loginNoUsername);
   ok(!gLoginItem.dataset.editing, "loginItem should not be in 'edit' mode");
   is(document.l10n.getAttributes(usernameInput).id, "about-logins-login-item-username", "username field should have default placeholder when username is not present and not editing");
   let copyUsernameButton = gLoginItem.shadowRoot.querySelector(".copy-username-button");
   ok(copyUsernameButton.disabled, "The copy-username-button should be disabled if there is no username");
 
   usernameInput.placeholder = "dummy placeholder";
   gLoginItem.shadowRoot.querySelector(".edit-button").click();
+  await asyncElementRendered();
   is(
     document.l10n.getAttributes(usernameInput).id,
     null,
     "there should be no placeholder id on the username input in edit mode"
   );
   is(usernameInput.placeholder, "", "there should be no placeholder on the username input in edit mode");
 });
 
@@ -155,16 +156,17 @@ add_task(async function test_breach_aler
 });
 
 add_task(async function test_edit_login() {
   gLoginItem.setLogin(TEST_LOGIN_1);
   let usernameInput = gLoginItem.shadowRoot.querySelector("input[name='username']");
   usernameInput.placeholder = "dummy placeholder";
   gLoginItem.shadowRoot.querySelector(".edit-button").click();
   await asyncElementRendered();
+  await asyncElementRendered();
 
   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 not be in 'isNewLogin' mode");
   let deleteButton = gLoginItem.shadowRoot.querySelector(".delete-button");
   ok(!deleteButton.disabled, "Delete button should be enabled when editing a login");
   is(gLoginItem.shadowRoot.querySelector("input[name='origin']").value, TEST_LOGIN_1.origin, "origin should be populated");
   is(usernameInput.value, TEST_LOGIN_1.username, "username should be populated");
@@ -194,16 +196,17 @@ add_task(async function test_edit_login(
   }, {once: true});
   gLoginItem.shadowRoot.querySelector(".save-changes-button").click();
   ok(updateEventDispatched, "Clicking the .save-changes-button should dispatch the AboutLoginsUpdateLogin event");
 });
 
 add_task(async function test_edit_login_cancel() {
   gLoginItem.setLogin(TEST_LOGIN_1);
   gLoginItem.shadowRoot.querySelector(".edit-button").click();
+  await asyncElementRendered();
 
   ok(gLoginItem.dataset.editing, "loginItem should be in 'edit' mode");
   is(!!gLoginItem.dataset.isNewLogin, false,
      "loginItem should not be in 'isNewLogin' mode");
 
   gLoginItem.shadowRoot.querySelector(".cancel-button").click();
   gConfirmationDialog.shadowRoot.querySelector(".confirm-button").click();
 
@@ -226,16 +229,18 @@ add_task(async function test_reveal_pass
   revealCheckbox.click();
   ok(revealCheckbox.checked, "reveal-checkbox should be checked after clicking");
   await SimpleTest.promiseWaitForCondition(() => passwordInput.type == "text",
     "waiting for password input type to change after checking for master password");
   is(passwordInput.type, "text", "Password should be unmasked when checkbox is clicked");
 
   let editButton = gLoginItem.shadowRoot.querySelector(".edit-button");
   editButton.click();
+  await asyncElementRendered();
+
   ok(revealCheckbox.checked, "reveal-checkbox should remain checked when entering 'edit' mode");
   gLoginItem.shadowRoot.querySelector(".cancel-button").click();
   ok(!revealCheckbox.checked, "reveal-checkbox should be unchecked after canceling 'edit' mode");
   revealCheckbox.click();
 
   gLoginItem.setLogin(TEST_LOGIN_2);
   ok(!revealCheckbox.checked, "reveal-checkbox should be unchecked when changing logins");
   is(passwordInput.type, "password", "Password should be masked by default when switching logins");
@@ -302,45 +307,45 @@ 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, TEST_LOGIN_1.password, "password should be unchanged");
+  is(gLoginItem.shadowRoot.querySelector("input[name='password']").value, " ".repeat(TEST_LOGIN_1.password.length), "password length should be unchanged");
   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, TEST_LOGIN_1.password, "password should be unchanged");
+  is(gLoginItem.shadowRoot.querySelector("input[name='password']").value, " ".repeat(TEST_LOGIN_1.password.length), "password length should be unchanged");
   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, modifiedLogin.password, "password should be updated");
+  is(gLoginItem.shadowRoot.querySelector("input[name='password']").value, " ".repeat(modifiedLogin.password.length), "password length should be updated");
   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);