From 8d1adc7e4d9b3e0735d81006b8926262d32b7ffb Mon Sep 17 00:00:00 2001 From: Pig Fang Date: Sun, 5 Nov 2017 10:25:20 +0800 Subject: [PATCH] Add security check before resetting password --- app/Http/Controllers/AuthController.php | 24 +++++++- .../assets/src/js/__tests__/auth.test.js | 6 +- resources/assets/src/js/auth/reset.js | 3 +- tests/AuthControllerTest.php | 57 ++++++++++++++++++- 4 files changed, 82 insertions(+), 8 deletions(-) diff --git a/app/Http/Controllers/AuthController.php b/app/Http/Controllers/AuthController.php index e90b928a..e8e13566 100644 --- a/app/Http/Controllers/AuthController.php +++ b/app/Http/Controllers/AuthController.php @@ -214,9 +214,9 @@ class AuthController extends Controller return redirect('auth/forgot')->with('msg', trans('auth.reset.invalid')); // unpack to get user token & timestamp - $encrypted = base64_decode($request->input('token')); - $token = substr($encrypted, 0, -22); - $timestamp = substr($encrypted, strlen($token), 6); + $decoded = base64_decode($request->input('token')); + $token = substr($decoded, 0, -22); + $timestamp = substr($decoded, strlen($token), 6); if ($user->getToken() != $token) { return redirect('auth/forgot')->with('msg', trans('auth.reset.invalid')); @@ -238,8 +238,26 @@ class AuthController extends Controller $this->validate($request, [ 'uid' => 'required|integer', 'password' => 'required|min:8|max:16', + 'token' => 'required', ]); + $decoded = base64_decode($request->input('token')); + $token = substr($decoded, 0, -22); + $timestamp = intval(substr($decoded, strlen($token), 6)); + + $user = $users->get($request->input('uid')); + if (!$user) + return json(trans('auth.reset.invalid'), 1); + + if ($user->getToken() != $token) { + return json(trans('auth.reset.invalid'), 1); + } + + // more than 1 hour + if ((intval(substr(time(), 4, 6)) - $timestamp) > 3600) { + return json(trans('auth.reset.expired'), 1); + } + $users->get($request->input('uid'))->changePasswd($request->input('password')); Log::info("[Password Reset] Password of user [{$request->input('uid')}] has been changed"); diff --git a/resources/assets/src/js/__tests__/auth.test.js b/resources/assets/src/js/__tests__/auth.test.js index f383f974..70e5aeed 100644 --- a/resources/assets/src/js/__tests__/auth.test.js +++ b/resources/assets/src/js/__tests__/auth.test.js @@ -302,12 +302,14 @@ describe('tests for "reset" module', () => { const url = jest.fn(path => path); const swal = jest.fn().mockReturnValue(Promise.resolve()); const showMsg = jest.fn(); + const getQueryString = jest.fn().mockReturnValue('token'); window.fetch = fetch; window.trans = trans; window.url = url; window.swal = swal; window.showMsg = showMsg; window.refreshCaptcha = jest.fn(); + window.getQueryString = getQueryString; document.body.innerHTML = ` @@ -348,13 +350,15 @@ describe('tests for "reset" module', () => { $('#confirm-pwd').val('password'); await $('button').click(); + expect(getQueryString).toBeCalledWith('token'); expect(fetch).toBeCalledWith(expect.objectContaining({ type: 'POST', url: 'auth/reset', dataType: 'json', data: { uid: '1', - password: 'password' + password: 'password', + token: 'token' } })); expect($('button').html()).toBe( diff --git a/resources/assets/src/js/auth/reset.js b/resources/assets/src/js/auth/reset.js index bc209f16..f66254d3 100644 --- a/resources/assets/src/js/auth/reset.js +++ b/resources/assets/src/js/auth/reset.js @@ -5,7 +5,8 @@ $('#reset-button').click(e => { let data = { uid: $('#uid').val(), - password: $('#password').val() + password: $('#password').val(), + token: getQueryString('token') }; (function validate({ password }, callback) { diff --git a/tests/AuthControllerTest.php b/tests/AuthControllerTest.php index 0c6100c5..f36e4a11 100644 --- a/tests/AuthControllerTest.php +++ b/tests/AuthControllerTest.php @@ -589,11 +589,64 @@ class AuthControllerTest extends TestCase 'msg' => trans('validation.max.string', ['attribute' => 'password', 'max' => 16]) ]); - // Success + // Should be forbidden if `token` is missing $this->post( '/auth/reset', [ 'uid' => $user->uid, 'password' => '12345678' + ], ['X-Requested-With' => 'XMLHttpRequest'])->seeJson([ + 'errno' => 1, + 'msg' => trans('validation.required', ['attribute' => 'token']) + ]); + + // Should be forbidden if expired + $token = base64_encode( + $user->getToken().substr(time() - 60 * 60 * 2, 4, 6).str_random(16) + ); + $this->post( + '/auth/reset', [ + 'uid' => $user->uid, + 'password' => '12345678', + 'token' => $token + ])->seeJson([ + 'errno' => 1, + 'msg' => trans('auth.reset.expired') + ]); + + // Should return a warning if the user is not existed + $token = base64_encode( + $user->getToken().substr(time(), 4, 6).str_random(16) + ); + $this->post( + '/auth/reset', [ + 'uid' => -1, + 'password' => '12345678', + 'token' => $token + ])->seeJson([ + 'errno' => 1, + 'msg' => trans('auth.reset.invalid') + ]); + + // Should be forbidden if `token` is invalid + $this->post( + '/auth/reset', [ + 'uid' => $user->uid, + 'password' => '12345678', + 'token' => 'invalid' + ])->seeJson([ + 'errno' => 1, + 'msg' => trans('auth.reset.invalid') + ]); + + // Success + $token = base64_encode( + $user->getToken().substr(time(), 4, 6).str_random(16) + ); + $this->post( + '/auth/reset', [ + 'uid' => $user->uid, + 'password' => '12345678', + 'token' => $token ])->seeJson([ 'errno' => 0, 'msg' => trans('auth.reset.success') @@ -603,8 +656,6 @@ class AuthControllerTest extends TestCase // after resetting password. $user = User::find($user->uid); $this->assertTrue($user->verifyPassword('12345678')); - - // TODO: Security check before resetting password!!! } public function testCaptcha()