Merge pull request #505 from metabase/user_leak

Remove user existence leak in login and password reset endpoints [WIP]

src/metabase/api/session.clj

@@ -20,7 +20,8 @@
    password [Required NonEmptyString]}
   (let [user (sel :one :fields [User :id :password_salt :password] :email email (korma/where {:is_active true}))]
     (checkp (not (nil? user))
-      (symbol "email") "no account found for the given email")
+    ; Don't leak whether the account doesn't exist or the password was incorrect
+      (symbol "password") "did not match stored password")
     (checkp (pass/verify-password password (:password_salt user) (:password user))
       (symbol "password") "did not match stored password")
     (let [session-id (str (java.util.UUID/randomUUID))]
@@ -51,11 +52,12 @@
   (let [{user-id :id} (sel :one User :email email)
         reset-token (java.util.UUID/randomUUID)
         password-reset-url (str origin "/auth/reset_password/" reset-token)]
-    (checkp (not (nil? user-id))
-      (symbol "email") "no account found for the given email")
-    (upd User user-id :reset_token reset-token :reset_triggered (System/currentTimeMillis))
-    (email/send-password-reset-email email server-name password-reset-url)
-    (log/info password-reset-url)))
+    ; Don't leak whether the account doesn't exist, just pretend everything is ok
+    (if (not (nil? user-id))
+      (do
+        (upd User user-id :reset_token reset-token :reset_triggered (System/currentTimeMillis))
+        (email/send-password-reset-email email server-name password-reset-url)
+        (log/info password-reset-url)))))
 
 
 (defendpoint POST "/reset_password"

test/metabase/api/session_test.clj

@@ -25,7 +25,8 @@
   (client :post 400 "session" {:email "anything@metabase.com"}))
 
 ;; Test for inactive user (user shouldn't be able to login if :is_active = false)
-(expect {:errors {:email "no account found for the given email"}}
+;; Return same error as incorrect password to avoid leaking existence of user
+(expect {:errors {:password "did not match stored password"}}
   (client :post 400 "session" (user->credentials :trashbird)))
 
 ;; Test for password checking
@@ -62,9 +63,9 @@
 (expect {:errors {:email "field is a required param."}}
   (client :post 400 "session/forgot_password" {}))
 
-;; Test that email not found gives 404
-(expect {:errors {:email "no account found for the given email"}}
-  (client :post 400 "session/forgot_password" {:email "not-found@metabase.com"}))
+;; Test that email not found also gives 200 as to not leak existence of user
+(expect nil
+  (client :post 200 "session/forgot_password" {:email "not-found@metabase.com"}))
 
 
 ;; POST /api/session/reset_password