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>
Mon, 30 Jan 2017 15:53:27 -0800
changeset 468332 129802a1c2095109afd4d1bbed6ba8b10746ad20
parent 467095 1e0e193b0812f68a12fbd69198552af62347af1e
child 543914 5787aa9065637f033e0cecc606e4660a7d12ef45
push id43427
push usermozilla@noorenberghe.ca
push dateTue, 31 Jan 2017 01:36:37 +0000
reviewersmconley
bugs1334026
milestone54.0a1
Bug 1334026 - Show the the insecure field warning on insecure password fields even if they're not marked. r=mconley MozReview-Commit-ID: JwEYWQmexj
toolkit/components/satchel/nsFormFillController.cpp
toolkit/components/satchel/test/mochitest.ini
toolkit/components/satchel/test/test_password_autocomplete.html
--- a/toolkit/components/satchel/nsFormFillController.cpp
+++ b/toolkit/components/satchel/nsFormFillController.cpp
@@ -689,20 +689,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);
@@ -973,39 +982,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/mochitest.ini
+++ b/toolkit/components/satchel/test/mochitest.ini
@@ -10,10 +10,11 @@ support-files =
 [test_bug_787624.html]
 [test_datalist_with_caching.html]
 [test_form_autocomplete.html]
 [test_form_autocomplete_with_list.html]
 [test_form_submission.html]
 [test_form_submission_cap.html]
 [test_form_submission_cap2.html]
 [test_password_autocomplete.html]
+scheme = https
 [test_popup_direction.html]
 [test_popup_enter_event.html]
--- a/toolkit/components/satchel/test/test_password_autocomplete.html
+++ b/toolkit/components/satchel/test/test_password_autocomplete.html
@@ -16,17 +16,24 @@
 <!-- we presumably can't hide the content for this test. -->
 <div id="content">
   <datalist id="datalist1">
     <option>value10</option>
     <option>value11</option>
     <option>value12</option>
   </datalist>
   <form id="form1" onsubmit="return false;">
-    <input  type="password" name="field1" list="datalist1">
+    <!-- Don't set the type to password until rememberSignons is false since we
+         want to test when rememberSignons is false. -->
+    <input  type="to-be-password" name="field1" list="datalist1">
+    <button type="submit">Submit</button>
+  </form>
+  <!-- Same as form1 but with an insecure HTTP action -->
+  <form id="form2" onsubmit="return false;" action="http://mochi.test/">
+    <input  type="to-be-password" name="field1" list="datalist1">
     <button type="submit">Submit</button>
   </form>
 </div>
 
 <pre id="test">
 <script class="testbody" type="text/javascript">
 /* import-globals-from satchel_common.js */
 
@@ -61,36 +68,42 @@ 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() {
+  yield SpecialPowers.pushPrefEnv({set: [["signon.rememberSignons", false]]});
 
-add_task(function* test_initialize() {
-  yield SpecialPowers.pushPrefEnv("set", [["signon.rememberSignons", false]]);
+  is(window.location.protocol, "https:", "This test must run on HTTPS");
+
+  // Now that rememberSignons is false, create the password fields.
+  $_(1, "field1").type = "password";
+  $_(2, "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() {
+add_task(function* test_secure_noFormHistoryOrWarning() {
+  let input = $_(1, "field1");
+
   // 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"),
@@ -102,12 +115,26 @@ add_task(function* test_no_form_history(
     // 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);
   }
 
   // Close the popup.
   input.blur();
 });
+
+add_task(function* test_insecure_focusWarning() {
+  // Form 2 has an insecure action so should show the warning even if password manager is disabled.
+  let input = $_(2, "field1");
+  let shownPromise = waitForNextPopup();
+  input.focus();
+  yield shownPromise;
+
+  ok(getMenuEntries()[0].includes("Logins entered here could be compromised"),
+     "Check warning is first")
+
+  // Close the popup
+  input.blur();
+});
 </script>
 </pre>
 </body>
 </html>