Fix for ticket 1010: Don't leak valid user names in "forgot password" form.

Includes fixes for user forms as well (edit user / email / password).

2 files changed, 30 insertions, 26 deletions

diff --git a/modules/user/controllers/password.php b/modules/user/controllers/password.php
index 07fdc1ed..c6d7e889 100644
--- a/modules/user/controllers/password.php
+++ b/modules/user/controllers/password.php
@@ -19,12 +19,19 @@
  */
 class Password_Controller extends Controller {
   public function reset() {
+    $form = self::_reset_form();
     if (request::method() == "post") {
       // @todo separate the post from get parts of this function
       access::verify_csrf();
-      $this->_send_reset();
+      // Basic validation (was some user name specified?)
+      if ($form->validate()) {
+        $this->_send_reset($form);
+      } else {
+        print json_encode(array("result" => "error",
+                                "form" => (string) $form));
+      }
     } else {
-      print $this->_reset_form();
+      print $form;
     }
   }
 
@@ -41,19 +48,9 @@ class Password_Controller extends Controller {
     }
   }
 
-  private function _send_reset() {
-    $form = $this->_reset_form();
-
-    $valid = $form->validate();
-    if ($valid) {
-      $user = user::lookup_by_name($form->reset->inputs["name"]->value);
-      if (!$user->loaded() || empty($user->email)) {
-        $form->reset->inputs["name"]->add_error("no_email", 1);
-        $valid = false;
-      }
-    }
-
-    if ($valid) {
+  private function _send_reset($form) {
+    $user = user::lookup_by_name($form->reset->inputs["name"]->value);
+    if ($user && !empty($user->email)) {
       $user->hash = md5(rand());
       $user->save();
       $message = new View("reset_password.html");
@@ -71,22 +68,29 @@ class Password_Controller extends Controller {
       log::success(
         "user",
         t("Password reset email sent for user %name", array("name" => $user->name)));
-    } else {
+    } else if (!$user) {
       // Don't include the username here until you're sure that it's XSS safe
       log::warning(
-        "user", "Password reset email requested for bogus user");
+          "user", t("Password reset email requested for bogus user"));
+    } else  {
+      log::warning(
+          "user", t("Password reset failed for %user_name (has no email address on record).",
+                    array("user_name" => $user->name)));
     }
 
+    // Always pretend that an email has been sent to avoid leaking
+    // information on what user names are actually real.
     message::success(t("Password reset email sent"));
     print json_encode(
       array("result" => "success"));
   }
 
-  private function _reset_form() {
+  private static function _reset_form() {
     $form = new Forge(url::current(true), "", "post", array("id" => "g-reset-form"));
     $group = $form->group("reset")->label(t("Reset Password"));
-    $group->input("name")->label(t("Username"))->id("g-name")->class(null)->rules("required");
-    $group->inputs["name"]->error_messages("no_email", t("No email, unable to reset password"));
+    $group->input("name")->label(t("Username"))->id("g-name")->class(null)
+      ->rules("required")
+      ->error_messages("required", t("You must enter a user name"));
     $group->submit("")->value(t("Reset"));
 
     return $form;
diff --git a/modules/user/controllers/users.php b/modules/user/controllers/users.php
index 0730f391..cd7d271f 100644
--- a/modules/user/controllers/users.php
+++ b/modules/user/controllers/users.php
@@ -20,7 +20,7 @@
 class Users_Controller extends Controller {
   public function update($id) {
     $user = user::lookup($id);
-    if ($user->guest || $user->id != identity::active_user()->id) {
+    if (!$user || $user->guest || $user->id != identity::active_user()->id) {
       access::forbidden();
     }
 
@@ -63,7 +63,7 @@ class Users_Controller extends Controller {
 
   public function change_password($id) {
     $user = user::lookup($id);
-    if ($user->guest || $user->id != identity::active_user()->id) {
+    if (!$user || $user->guest || $user->id != identity::active_user()->id) {
       access::forbidden();
     }
 
@@ -99,7 +99,7 @@ class Users_Controller extends Controller {
 
   public function change_email($id) {
     $user = user::lookup($id);
-    if ($user->guest || $user->id != identity::active_user()->id) {
+    if (!$user || $user->guest || $user->id != identity::active_user()->id) {
       access::forbidden();
     }
 
@@ -134,7 +134,7 @@ class Users_Controller extends Controller {
 
   public function form_edit($id) {
     $user = user::lookup($id);
-    if ($user->guest || $user->id != identity::active_user()->id) {
+    if (!$user || $user->guest || $user->id != identity::active_user()->id) {
       access::forbidden();
     }
 
@@ -143,7 +143,7 @@ class Users_Controller extends Controller {
 
   public function form_change_password($id) {
     $user = user::lookup($id);
-    if ($user->guest || $user->id != identity::active_user()->id) {
+    if (!$user || $user->guest || $user->id != identity::active_user()->id) {
       access::forbidden();
     }
 
@@ -152,7 +152,7 @@ class Users_Controller extends Controller {
 
   public function form_change_email($id) {
     $user = user::lookup($id);
-    if ($user->guest || $user->id != identity::active_user()->id) {
+    if (!$user || $user->guest || $user->id != identity::active_user()->id) {
       access::forbidden();
     }