Bug 1334026 - Show the the insecure field warning on insecure password fields even if they're not marked. r=mconley draft
authorMatthew Noorenberghe <mozilla@noorenberghe.ca>
Fri, 03 Feb 2017 01:37:21 -0800
changeset 470205 fd7ae2794ba5471b6c83ceb4e95ca8d5bdda9806
parent 470204 9759594e223f25075dde334fe96af323bf33d144
child 544410 2148e4e70d4806596684bc732324efd7ce6a41d7
push id43956
push usermozilla@noorenberghe.ca
push dateFri, 03 Feb 2017 09:40:43 +0000
reviewersmconley
bugs1334026
milestone52.0
Bug 1334026 - Show the the insecure field warning on insecure password fields even if they're not marked. r=mconley MozReview-Commit-ID: LxvPzD3SOnc
toolkit/components/satchel/nsFormFillController.cpp
toolkit/components/satchel/test/test_password_autocomplete.html
--- a/toolkit/components/satchel/nsFormFillController.cpp
+++ b/toolkit/components/satchel/nsFormFillController.cpp
@@ -662,20 +662,29 @@ nsFormFillController::GetUserContextId(u
 ////////////////////////////////////////////////////////////////////////
 //// nsIAutoCompleteSearch
 
 NS_IMETHODIMP
 nsFormFillController::StartSearch(const nsAString &aSearchString, const nsAString &aSearchParam,
                                   nsIAutoCompleteResult *aPreviousResult, nsIAutoCompleteObserver *aListener)
 {
   nsresult rv;
+  nsCOMPtr<nsIFormControl> formControl = do_QueryInterface(mFocusedInputNode);
 
   // If the login manager has indicated it's responsible for this field, let it
   // handle the autocomplete. Otherwise, handle with form history.
-  if (mPwmgrInputs.Get(mFocusedInputNode)) {
+  if (mFocusedInputNode && (mPwmgrInputs.Get(mFocusedInputNode) ||
+                            formControl->GetType() == NS_FORM_INPUT_PASSWORD)) {
+
+    // Handle the case where a password field is focused but
+    // MarkAsLoginManagerField wasn't called because password manager is disabled.
+    if (!mLoginManager) {
+      mLoginManager = do_GetService("@mozilla.org/login-manager;1");
+    }
+
     // XXX aPreviousResult shouldn't ever be a historyResult type, since we're not letting
     // satchel manage the field?
     mLastListener = aListener;
     rv = mLoginManager->AutoCompleteSearchAsync(aSearchString,
                                                 aPreviousResult,
                                                 mFocusedInput,
                                                 this);
     NS_ENSURE_SUCCESS(rv, rv);
@@ -934,39 +943,45 @@ nsFormFillController::MaybeStartControll
   nsCOMPtr<nsIDOMHTMLElement> datalist;
   aInput->GetList(getter_AddRefs(datalist));
   bool hasList = datalist != nullptr;
 
   bool isPwmgrInput = false;
   if (mPwmgrInputs.Get(inputNode))
       isPwmgrInput = true;
 
-  // Don't show autocomplete on password fields regardless of datalist or
-  // autocomplete being enabled as we don't want to show form history on them.
-  // The GetType() check was more readable than !formControl->IsSingleLineTextControl(true)
-  // but this logic should be kept in-sync with that.
-  if (formControl->GetType() == NS_FORM_INPUT_PASSWORD && !isPwmgrInput) {
-    return;
-  }
-
   if (isPwmgrInput || hasList || autocomplete) {
     StartControllingInput(aInput);
   }
 }
 
 nsresult
 nsFormFillController::Focus(nsIDOMEvent* aEvent)
 {
   nsCOMPtr<nsIDOMHTMLInputElement> input = do_QueryInterface(
     aEvent->InternalDOMEvent()->GetTarget());
   MaybeStartControllingInput(input);
 
+  // Bail if we didn't start controlling the input.
+  if (!mFocusedInputNode) {
+    mContextMenuFiredBeforeFocus = false;
+    return NS_OK;
+  }
+
+  nsCOMPtr<nsIFormControl> formControl = do_QueryInterface(mFocusedInputNode);
+  MOZ_ASSERT(formControl);
+
   // If this focus doesn't immediately follow a contextmenu event then show
   // the autocomplete popup
-  if (!mContextMenuFiredBeforeFocus && mPwmgrInputs.Get(mFocusedInputNode)) {
+  if (!mContextMenuFiredBeforeFocus &&
+      (mPwmgrInputs.Get(mFocusedInputNode)
+#ifndef ANDROID
+       || formControl->GetType() == NS_FORM_INPUT_PASSWORD
+#endif
+       )) {
     ShowPopup();
   }
 
   mContextMenuFiredBeforeFocus = false;
   return NS_OK;
 }
 
 nsresult
--- a/toolkit/components/satchel/test/test_password_autocomplete.html
+++ b/toolkit/components/satchel/test/test_password_autocomplete.html
@@ -63,58 +63,45 @@ function expectPopupDoesNotOpen(triggerF
   let popupShown = waitForNextPopup();
   triggerFn();
   return Promise.race([
     popupShown.then(() => Promise.reject("Popup opened unexpectedly.")),
     new Promise(resolve => setTimeout(resolve, POPUP_RESPONSE_WAIT_TIME_MS)),
   ]);
 }
 
-let input = $_(1, "field1");
-
 add_task(function* test_initialize() {
-  info("remember: " + SpecialPowers.getBoolPref("signon.rememberSignons"));
   yield SpecialPowers.pushPrefEnv({"set": [["signon.rememberSignons", false]]});
-  info("remember: " + SpecialPowers.getBoolPref("signon.rememberSignons"));
 
   // Now that rememberSignons is false, create the password field
-  input.type = "password";
+  $_(1, "field1").type = "password";
 
   yield new Promise(resolve => updateFormHistory([
     { op : "remove" },
     { op : "add", fieldname : "field1", value : "value1" },
     { op : "add", fieldname : "field1", value : "value2" },
     { op : "add", fieldname : "field1", value : "value3" },
     { op : "add", fieldname : "field1", value : "value4" },
     { op : "add", fieldname : "field1", value : "value5" },
     { op : "add", fieldname : "field1", value : "value6" },
     { op : "add", fieldname : "field1", value : "value7" },
     { op : "add", fieldname : "field1", value : "value8" },
     { op : "add", fieldname : "field1", value : "value9" },
   ], resolve));
 });
 
-add_task(function* test_no_form_history() {
-  // The autocomplete popup should not open under any circumstances on
-  // type=password with password manager disabled.
-  for (let triggerFn of [
-    () => input.focus(),
-    () => input.click(),
-    () => doKey("down"),
-    () => doKey("page_down"),
-    () => doKey("return"),
-    () => doKey("v"),
-    () => doKey(" "),
-    () => doKey("back_space"),
-  ]) {
-    ok(true, "Testing: " + triggerFn.toString())
-    // We must wait for the entire timeout for each individual test, because the
-    // next event in the list might prevent the popup from opening.
-    yield expectPopupDoesNotOpen(triggerFn);
-  }
+add_task(function* test_insecure_focusWarning() {
+  // The form is insecure so should show the warning even if password manager is disabled.
+  let input = $_(1, "field1");
+  let shownPromise = waitForNextPopup();
+  input.focus();
+  yield shownPromise;
 
-  // Close the popup.
+  ok(getMenuEntries()[0].includes("Logins entered here could be compromised"),
+     "Check warning is first")
+
+  // Close the popup
   input.blur();
 });
 </script>
 </pre>
 </body>
 </html>