[Password Manager] Fix to recognise failed login attempt for sites where content server pushes new …

components/password_manager/core/browser/password_manager.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "components/password_manager/core/browser/password_manager.h"
 
 #include "base/command_line.h"
 #include "base/metrics/field_trial.h"
 #include "base/metrics/histogram.h"
 #include "base/prefs/pref_service.h"
@@ skipping 51 matching lines @@
   if (command_line->HasSwitch(switches::kEnableDropSyncCredential))
     return true;
 
   if (command_line->HasSwitch(switches::kDisableDropSyncCredential))
     return false;
 
   // Default to not saving.
   return group_name != "Disabled";
 }
 
+bool CompareURLContentsEqualIgnoreCase(const GURL& src, const GURL& dst) {

[vabr (Chromium), 2014/08/21 14:54:12]

nit: "Compare" seems superfluous in the function name.

[vabr (Chromium), 2014/08/21 14:54:12]

Please spell out concretely that scheme is omitted, the notion of a URLContents
is not known widely enough to indicate that.

[Pritam Nikam, 2014/08/21 16:49:21]

Done.

[Pritam Nikam, 2014/08/21 16:49:21]

Done.

+  return (base::StringToLowerASCII(src.GetContent()) ==
+          base::StringToLowerASCII(dst.GetContent()));
+}
+
 }  // namespace
 
 const char PasswordManager::kOtherPossibleUsernamesExperiment[] =
     "PasswordManagerOtherPossibleUsernames";
 
 // static
 void PasswordManager::RegisterProfilePrefs(
     user_prefs::PrefRegistrySyncable* registry) {
   registry->RegisterBooleanPref(
       prefs::kPasswordManagerSavingEnabled,
@@ skipping 348 matching lines @@
   }
 
   // Record all visible forms from the frame.
   all_visible_forms_.insert(all_visible_forms_.end(),
                             visible_forms.begin(),
                             visible_forms.end());
 
   // If we see the login form again, then the login failed.
   if (did_stop_loading) {
     for (size_t i = 0; i < all_visible_forms_.size(); ++i) {
-      // TODO(vabr): The similarity check is just action equality for now. If it
-      // becomes more complex, it may make sense to consider modifying and using
-      // PasswordFormManager::DoesManage for it.
+      // TODO(vabr): The similarity check is just on case and scheme ignored

[vabr (Chromium), 2014/08/21 14:54:12]

I'm not a native English speaker, but the first sentence seems difficult to
parse to me. I'd suggest rephrasing, but before we do that, we should probably
clarify whether the case sensitivity is included or not.

[Pritam Nikam, 2014/08/21 16:49:21]

Done.

+      // action URL equality for now. If it becomes more complex, it may make
+      // sense to consider modifying and using PasswordFormManager::DoesManage
+      // for it.
       if (all_visible_forms_[i].action.is_valid() &&
-          provisional_save_manager_->pending_credentials().action ==
-              all_visible_forms_[i].action) {
+          CompareURLContentsEqualIgnoreCase(

[vabr (Chromium), 2014/08/21 14:54:12]

Please do not allow mixing of arbitrary schemes, just HTTP and HTTPS at this
point. Let's be conservative, because it is security relevant.

[Pritam Nikam, 2014/08/21 16:49:21]

Done.

[jww, 2014/08/21 19:19:13]

This lgtm from a security perspective but is certainly treading on very
dangerous territory. This seems fine because it leads to an *inaction*... but we
we need to be really, really careful. So if I am misunderstanding the fact that
this is an inaction, please correct me.

+              provisional_save_manager_->pending_credentials().action,
+              all_visible_forms_[i].action)) {
         if (logger) {
           logger->LogPasswordForm(Logger::STRING_PASSWORD_FORM_REAPPEARED,
                                   visible_forms[i]);
           logger->LogMessage(Logger::STRING_DECISION_DROP);
         }
         provisional_save_manager_->SubmitFailed();
         provisional_save_manager_.reset();
         // Clear all_visible_forms_ once we found the match.
         all_visible_forms_.clear();
         return;
@@ skipping 105 matching lines @@
           observers_,
           OnAutofillDataAvailable(preferred_match.username_value,
                                   preferred_match.password_value));
       break;
   }
 
   client_->PasswordWasAutofilled(best_matches);
 }
 
 }  // namespace password_manager