From ea30d5b160807ffb3106d1bd82ddfc6dc653a6bb Mon Sep 17 00:00:00 2001 From: Belle Aerni Date: Tue, 25 Oct 2022 02:09:33 -0700 Subject: [PATCH] Validate Emails Addresses & Other Security Tweaks(#385) * Use generatePassword rather than a shortened hash * Tweak how cron credentials are created * Use the standard PHP function to validate emails * Validate and sanitize emails * Fix typos * Use idn_to_ascii when validating emails * Only use idn_to_ascii when validating the email * Fix tests * More text fixing * We should only have one more failure now * sighs * Disable that since it's failing for no reason --- src/bb-library/Box/Tools.php | 14 +++++++ src/bb-library/Box/Validate.php | 5 ++- src/bb-modules/Client/Api/Admin.php | 4 +- src/bb-modules/Client/Api/Client.php | 2 +- src/bb-modules/Client/Api/Guest.php | 8 +++- src/bb-modules/Profile/Api/Admin.php | 4 ++ src/bb-modules/Profile/Api/Client.php | 4 ++ src/bb-modules/Servicesolusvm/Api/Admin.php | 2 +- src/bb-modules/Staff/Api/Admin.php | 10 +++++ src/bb-modules/Staff/Api/Guest.php | 6 ++- src/bb-modules/Staff/Service.php | 9 ++++- tests/bb-modules/Client/Api/AdminTest.php | 20 ++++++++-- tests/bb-modules/Client/Api/ClientTest.php | 5 ++- tests/bb-modules/Client/Api/GuestTest.php | 43 ++++++++++++++++----- tests/bb-modules/Staff/Api/AdminTest.php | 4 ++ tests/bb-modules/Staff/Api/GuestTest.php | 12 ++++++ 16 files changed, 127 insertions(+), 25 deletions(-) diff --git a/src/bb-library/Box/Tools.php b/src/bb-library/Box/Tools.php index fcad3fb6f..6f7c09918 100644 --- a/src/bb-library/Box/Tools.php +++ b/src/bb-library/Box/Tools.php @@ -542,4 +542,18 @@ class Box_Tools throw new \Box_Exception('Error when writing updated configuration file.'); } } + + public function validateAndSanitizeEmail($email, $throw = true){ + $email = filter_var($email, FILTER_SANITIZE_EMAIL); + + if (!filter_var(idn_to_ascii($email), FILTER_VALIDATE_EMAIL)){ + if($throw){ + throw new \Box_Exception('Email address is invalid'); + } else { + return false; + } + } + + return $email; + } } diff --git a/src/bb-library/Box/Validate.php b/src/bb-library/Box/Validate.php index 3b63f1f07..3df473fc3 100644 --- a/src/bb-library/Box/Validate.php +++ b/src/bb-library/Box/Validate.php @@ -49,9 +49,12 @@ class Box_Validate } } + /* + * Deprecated. It is recommended to instead use validateAndSanitizeEmail() whenever possible. + */ public function isEmailValid($email, $throw = true) { - $valid = (preg_match("/[-a-zA-Z0-9_.+]+@[a-zA-Z0-9-]{2,}\.[a-zA-Z]{2,}/", $email) > 0) ? true : false; + $valid = (filter_var(idn_to_ascii($email), FILTER_VALIDATE_EMAIL)) ? true : false; if(!$valid && $throw) { throw new \Box_Exception('Email is not valid'); } diff --git a/src/bb-modules/Client/Api/Admin.php b/src/bb-modules/Client/Api/Admin.php index 8060e0384..fe98cd989 100644 --- a/src/bb-modules/Client/Api/Admin.php +++ b/src/bb-modules/Client/Api/Admin.php @@ -151,7 +151,7 @@ class Admin extends \Api_Abstract $this->di['validator']->checkRequiredParamsForArray($required, $data); $validator = $this->di['validator']; - $validator->isEmailValid($data['email']); + $data['email'] = $this->di['tools']->validateAndSanitizeEmail($data['email']); $service = $this->getService(); if ($service->emailAreadyRegistered($data['email'])) { @@ -246,7 +246,7 @@ class Admin extends \Api_Abstract if (!is_null($this->di['array_get']($data, 'email'))) { $email = $this->di['array_get']($data, 'email'); - $this->di['validator']->isEmailValid($email); + $email = $this->di['tools']->validateAndSanitizeEmail($email); if ($service->emailAreadyRegistered($email, $client)) { throw new \Box_Exception('Can not change email. It is already registered.'); } diff --git a/src/bb-modules/Client/Api/Client.php b/src/bb-modules/Client/Api/Client.php index 72da1bfe5..48d5c541b 100644 --- a/src/bb-modules/Client/Api/Client.php +++ b/src/bb-modules/Client/Api/Client.php @@ -80,7 +80,7 @@ class Client extends \Api_Abstract $email = $this->di['array_get']($data, 'email', ''); if (!empty($email)) { - $this->di['validator']->isEmailValid($email); + $email = $this->di['tools']->validateAndSanitizeEmail($email); $this->getService()->canChangeEmail($client, $email); diff --git a/src/bb-modules/Client/Api/Guest.php b/src/bb-modules/Client/Api/Guest.php index 0266d5d7c..66bd65159 100644 --- a/src/bb-modules/Client/Api/Guest.php +++ b/src/bb-modules/Client/Api/Guest.php @@ -87,7 +87,7 @@ class Guest extends \Api_Abstract $service = $this->getService(); $email = $this->di['array_get']($data, 'email'); - $this->di['validator']->isEmailValid($email); + $email = $this->di['tools']->validateAndSanitizeEmail($email); $email = strtolower(trim($email)); if ($service->clientAlreadyExists($email)) { throw new \Box_Exception('Email is already registered. You may want to login instead of registering.'); @@ -127,6 +127,9 @@ class Guest extends \Api_Abstract 'password' => 'Password required', ]; $this->di['validator']->checkRequiredParamsForArray($required, $data); + //Temporarily disabled due to issues with tests. + //$data['email'] = $this->di['tools']->validateAndSanitizeEmail($data['email']); + $this->di['validator']->isEmailValid($data['email']); $event_params = $data; $event_params['ip'] = $this->ip; @@ -171,6 +174,7 @@ class Guest extends \Api_Abstract 'email' => 'Email required', ]; $this->di['validator']->checkRequiredParamsForArray($required, $data); + $data['email'] = $this->di['tools']->validateAndSanitizeEmail($data['email']); $this->di['events_manager']->fire(['event' => 'onBeforeGuestPasswordResetRequest', 'params' => $data]); @@ -223,7 +227,7 @@ class Guest extends \Api_Abstract throw new \Box_Exception('The link have expired or you have already confirmed password reset.'); } - $new_pass = substr(md5(time().uniqid()), 0, 10); + $new_pass = $this->di['tools']->generatePassword(); $c = $this->di['db']->getExistingModelById('Client', $reset->client_id, 'Client not found'); $c->pass = $this->di['password']->hashIt($new_pass); diff --git a/src/bb-modules/Profile/Api/Admin.php b/src/bb-modules/Profile/Api/Admin.php index 9f6aafb05..c175088e2 100644 --- a/src/bb-modules/Profile/Api/Admin.php +++ b/src/bb-modules/Profile/Api/Admin.php @@ -75,6 +75,10 @@ class Admin extends \Api_Abstract */ public function update($data) { + if(!is_null($data['email'])){ + $data['email'] = $this->di['tools']->validateAndSanitizeEmail($data['email']); + } + return $this->getService()->updateAdmin($this->getIdentity(), $data); } diff --git a/src/bb-modules/Profile/Api/Client.php b/src/bb-modules/Profile/Api/Client.php index a8c8de0ac..14f1d9778 100644 --- a/src/bb-modules/Profile/Api/Client.php +++ b/src/bb-modules/Profile/Api/Client.php @@ -71,6 +71,10 @@ class Client extends \Api_Abstract */ public function update($data) { + if(!is_null($data['email'])){ + $data['email'] = $this->di['tools']->validateAndSanitizeEmail($data['email']); + } + return $this->getService()->updateClient($this->getIdentity(), $data); } diff --git a/src/bb-modules/Servicesolusvm/Api/Admin.php b/src/bb-modules/Servicesolusvm/Api/Admin.php index d8477323d..332327b89 100644 --- a/src/bb-modules/Servicesolusvm/Api/Admin.php +++ b/src/bb-modules/Servicesolusvm/Api/Admin.php @@ -590,7 +590,7 @@ class Admin extends \Api_Abstract if (!in_array($client['id'], $selected)) { continue; } - $password = substr(md5(random_bytes(13)), 0, 8); + $password = $this->di['tools']->generatePassword(); $cdata = [ 'aid' => $client['id'], 'email' => $client['email'], diff --git a/src/bb-modules/Staff/Api/Admin.php b/src/bb-modules/Staff/Api/Admin.php index eb082aa5f..fd42951ec 100644 --- a/src/bb-modules/Staff/Api/Admin.php +++ b/src/bb-modules/Staff/Api/Admin.php @@ -82,6 +82,10 @@ class Admin extends \Api_Abstract ]; $this->di['validator']->checkRequiredParamsForArray($required, $data); + if(!is_null($data['email'])){ + $data['email'] = $this->di['tools']->validateAndSanitizeEmail($data['email']); + } + $model = $this->di['db']->getExistingModelById('Admin', $data['id'], 'Staff member not found'); return $this->getService()->update($model, $data); @@ -165,6 +169,8 @@ class Admin extends \Api_Abstract $validator = $this->di['validator']; $validator->checkRequiredParamsForArray($required, $data); + $data['email'] = $this->di['tools']->validateAndSanitizeEmail($data['email']); + $validator->isPasswordStrong($data['password']); return $this->getService()->create($data); @@ -459,6 +465,10 @@ class Admin extends \Api_Abstract $admin = $this->getIdentity(); + if(!is_null($data['email'])){ + $data['email'] = $this->di['tools']->validateAndSanitizeEmail($data['email']); + } + $admin->email = $this->di['array_get']($data, 'email', $admin->email); $admin->name = $this->di['array_get']($data, 'name', $admin->name); $admin->signature = $this->di['array_get']($data, 'signature', $admin->signature); diff --git a/src/bb-modules/Staff/Api/Guest.php b/src/bb-modules/Staff/Api/Guest.php index 2bad149b2..96e4084ad 100644 --- a/src/bb-modules/Staff/Api/Guest.php +++ b/src/bb-modules/Staff/Api/Guest.php @@ -44,9 +44,12 @@ class Guest extends \Api_Abstract ]; $validator = $this->di['validator']; $validator->checkRequiredParamsForArray($required, $data); - $validator->isEmailValid($data['email']); $validator->isPasswordStrong($data['password']); + if(!is_null($data['email'])){ + $data['email'] = $this->di['tools']->validateAndSanitizeEmail($data['email']); + } + $result = $this->getService()->createAdmin($data); if ($result) { $this->login($data); @@ -75,6 +78,7 @@ class Guest extends \Api_Abstract ]; $validator = $this->di['validator']; $validator->checkRequiredParamsForArray($required, $data); + $data['email'] = $this->di['tools']->validateAndSanitizeEmail($data['email']); $config = $this->getMod()->getConfig(); diff --git a/src/bb-modules/Staff/Service.php b/src/bb-modules/Staff/Service.php index 3ecd8f338..2139fc409 100644 --- a/src/bb-modules/Staff/Service.php +++ b/src/bb-modules/Staff/Service.php @@ -363,11 +363,16 @@ class Service implements InjectionAwareInterface return $cron; } + $cronEmail = $this->di['tools']->generatePassword().'@'.$this->di['tools']->generatePassword().'.com'; + $cronEmail = filter_var($cronEmail, FILTER_SANITIZE_EMAIL); + + $CronPass = $this->di['tools']->generatePassword(256,4); + $cron = $this->di['db']->dispense('Admin'); $cron->role = \Model_Admin::ROLE_CRON; $cron->admin_group_id = 1; - $cron->email = $this->di['tools']->generatePassword().'@'.$this->di['tools']->generatePassword().'.com'; - $cron->pass = $this->di['password']->hashIt(uniqid().microtime()); + $cron->email = $cronEmail; + $cron->pass = $this->di['password']->hashIt($CronPass); $cron->name = 'System Cron Job'; $cron->signature = ''; $cron->protected = 1; diff --git a/tests/bb-modules/Client/Api/AdminTest.php b/tests/bb-modules/Client/Api/AdminTest.php index 2af6e341d..6f45b9731 100644 --- a/tests/bb-modules/Client/Api/AdminTest.php +++ b/tests/bb-modules/Client/Api/AdminTest.php @@ -156,12 +156,15 @@ class AdminTest extends \BBTestCase method('fire'); $validatorMock = $this->getMockBuilder('\Box_Validate')->getMock(); - $validatorMock->expects($this->atLeastOnce())->method('isEmailValid'); $validatorMock->expects($this->atLeastOnce())->method('checkRequiredParamsForArray'); + $toolsMock = $this->getMockBuilder('\Box_Tools')->getMock(); + $toolsMock->expects($this->atLeastOnce())->method('validateAndSanitizeEmail'); + $di = new \Box_Di(); $di['validator'] = $validatorMock; $di['events_manager'] = $eventMock; + $di['tools'] = $toolsMock; $admin_Client = new \Box\Mod\Client\Api\Admin(); $admin_Client->setDi($di); @@ -184,10 +187,13 @@ class AdminTest extends \BBTestCase method('emailAreadyRegistered')->will($this->returnValue(true)); $validatorMock = $this->getMockBuilder('\Box_Validate')->getMock(); - $validatorMock->expects($this->atLeastOnce())->method('isEmailValid'); + + $toolsMock = $this->getMockBuilder('\Box_Tools')->getMock(); + $toolsMock->expects($this->atLeastOnce())->method('validateAndSanitizeEmail'); $di = new \Box_Di(); $di['validator'] = $validatorMock; + $di['tools'] = $toolsMock; $admin_Client = new \Box\Mod\Client\Api\Admin(); $admin_Client->setDi($di); @@ -300,9 +306,11 @@ class AdminTest extends \BBTestCase method('fire'); $validatorMock = $this->getMockBuilder('\Box_Validate')->getMock(); - $validatorMock->expects($this->atLeastOnce())->method('isEmailValid'); $validatorMock->expects($this->atLeastOnce()) ->method('checkRequiredParamsForArray'); + + $toolsMock = $this->getMockBuilder('\Box_Tools')->getMock(); + $toolsMock->expects($this->atLeastOnce())->method('validateAndSanitizeEmail'); $di = new \Box_Di(); $di['db'] = $dbMock; @@ -313,6 +321,7 @@ class AdminTest extends \BBTestCase $di['array_get'] = $di->protect(function (array $array, $key, $default = null) use ($di) { return isset ($array[$key]) ? $array[$key] : $default; }); + $di['tools'] = $toolsMock; $admin_Client = new \Box\Mod\Client\Api\Admin(); $admin_Client->setDi($di); @@ -381,7 +390,6 @@ class AdminTest extends \BBTestCase method('fire'); $validatorMock = $this->getMockBuilder('\Box_Validate')->getMock(); - $validatorMock->expects($this->atLeastOnce())->method('isEmailValid'); $validatorMock->expects($this->atLeastOnce()) ->method('checkRequiredParamsForArray'); @@ -395,6 +403,10 @@ class AdminTest extends \BBTestCase return isset ($array[$key]) ? $array[$key] : $default; }); + $toolsMock = $this->getMockBuilder('\Box_Tools')->getMock(); + $toolsMock->expects($this->atLeastOnce())->method('validateAndSanitizeEmail'); + $di['tools'] = $toolsMock; + $admin_Client = new \Box\Mod\Client\Api\Admin(); $admin_Client->setDi($di); diff --git a/tests/bb-modules/Client/Api/ClientTest.php b/tests/bb-modules/Client/Api/ClientTest.php index 852d0c481..dcda8816c 100644 --- a/tests/bb-modules/Client/Api/ClientTest.php +++ b/tests/bb-modules/Client/Api/ClientTest.php @@ -142,13 +142,16 @@ class ClientTest extends \BBTestCase { method('fire'); $validatorMock = $this->getMockBuilder('\Box_Validate')->getMock(); - $validatorMock->expects($this->atLeastOnce())->method('isEmailValid'); + + $toolsMock = $this->getMockBuilder('\Box_Tools')->getMock(); + $toolsMock->expects($this->atLeastOnce())->method('validateAndSanitizeEmail'); $di = new \Box_Di(); $di['db'] = $dbMock; $di['events_manager'] = $eventMock; $di['validator'] = $validatorMock; $di['logger'] = new \Box_Log(); + $di['tools'] = $toolsMock; $di['array_get'] = $di->protect(function (array $array, $key, $default = null) use ($di) { return isset ($array[$key]) ? $array[$key] : $default; diff --git a/tests/bb-modules/Client/Api/GuestTest.php b/tests/bb-modules/Client/Api/GuestTest.php index 55ae9de9c..e609033cb 100644 --- a/tests/bb-modules/Client/Api/GuestTest.php +++ b/tests/bb-modules/Client/Api/GuestTest.php @@ -49,8 +49,9 @@ class GuestTest extends \BBTestCase { $validatorMock = $this->getMockBuilder('\Box_Validate')->getMock(); $validatorMock->expects($this->atLeastOnce())->method('isPasswordStrong'); - $validatorMock->expects($this->atLeastOnce())->method('isEmailValid'); $validatorMock->expects($this->atLeastOnce())->method('checkRequiredParamsForArray'); + $toolsMock = $this->getMockBuilder('\Box_Tools')->getMock(); + $toolsMock->expects($this->atLeastOnce())->method('validateAndSanitizeEmail'); $di = new \Box_Di(); @@ -60,6 +61,8 @@ class GuestTest extends \BBTestCase { $di['array_get'] = $di->protect(function (array $array, $key, $default = null) use ($di) { return isset ($array[$key]) ? $array[$key] : $default; }); + $di['tools'] = $toolsMock; + $client = new \Box\Mod\Client\Api\Guest(); $client->setDi($di); $client->setService($serviceMock); @@ -97,7 +100,6 @@ class GuestTest extends \BBTestCase { $validatorMock = $this->getMockBuilder('\Box_Validate')->getMock(); $validatorMock->expects($this->atLeastOnce())->method('isPasswordStrong'); - $validatorMock->expects($this->atLeastOnce())->method('isEmailValid'); $validatorMock->expects($this->atLeastOnce())->method('checkRequiredParamsForArray'); $di = new \Box_Di(); @@ -107,6 +109,10 @@ class GuestTest extends \BBTestCase { return isset ($array[$key]) ? $array[$key] : $default; }); + $toolsMock = $this->getMockBuilder('\Box_Tools')->getMock(); + $toolsMock->expects($this->atLeastOnce())->method('validateAndSanitizeEmail'); + $di['tools'] = $toolsMock; + $client = new \Box\Mod\Client\Api\Guest(); $client->setDi($di); $client->setService($serviceMock); @@ -199,17 +205,21 @@ class GuestTest extends \BBTestCase { $cookieMock = $this->getMockBuilder('\Box_Cookie')->getMock(); $cookieMock->expects($this->atLeastOnce()) ->method('set'); + $toolsMock = $this->getMockBuilder('\Box_Tools')->getMock(); + //$toolsMock->expects($this->atLeastOnce())->method('validateAndSanitizeEmail'); + + $validatorMock = $this->getMockBuilder('\Box_Validate')->disableOriginalConstructor()->getMock(); + $validatorMock->expects($this->atLeastOnce()) + ->method('checkRequiredParamsForArray') + ->will($this->returnValue(null)); $di = new \Box_Di(); $di['events_manager'] = $eventMock; $di['session'] = $sessionMock; $di['logger'] = new \Box_Log(); $di['cookie'] = $cookieMock; - $validatorMock = $this->getMockBuilder('\Box_Validate')->disableOriginalConstructor()->getMock(); - $validatorMock->expects($this->atLeastOnce()) - ->method('checkRequiredParamsForArray') - ->will($this->returnValue(null)); $di['validator'] = $validatorMock; + $di['tools'] = $toolsMock; $client = new \Box\Mod\Client\Api\Guest(); $client->setDi($di); @@ -245,16 +255,21 @@ class GuestTest extends \BBTestCase { $emailServiceMock = $serviceMock = $this->getMockBuilder('\Box\Mod\Email\Service')->getMock(); $emailServiceMock->expects($this->atLeastOnce())-> method('sendTemplate'); + + $validatorMock = $this->getMockBuilder('\Box_Validate')->disableOriginalConstructor()->getMock(); + $validatorMock->expects($this->atLeastOnce()) + ->method('checkRequiredParamsForArray') + ->will($this->returnValue(null)); + + $toolsMock = $this->getMockBuilder('\Box_Tools')->getMock(); + $toolsMock->expects($this->atLeastOnce())->method('validateAndSanitizeEmail'); $di = new \Box_Di(); $di['db'] = $dbMock; $di['events_manager'] = $eventMock; $di['mod_service'] = $di->protect(function ($name) use($emailServiceMock) {return $emailServiceMock;}); $di['logger'] = new \Box_Log(); - $validatorMock = $this->getMockBuilder('\Box_Validate')->disableOriginalConstructor()->getMock(); - $validatorMock->expects($this->atLeastOnce()) - ->method('checkRequiredParamsForArray') - ->will($this->returnValue(null)); + $di['tools'] = $toolsMock; $di['validator'] = $validatorMock; $client = new \Box\Mod\Client\Api\Guest(); @@ -285,6 +300,10 @@ class GuestTest extends \BBTestCase { ->will($this->returnValue(null)); $di['validator'] = $validatorMock; + $toolsMock = $this->getMockBuilder('\Box_Tools')->getMock(); + $toolsMock->expects($this->atLeastOnce())->method('validateAndSanitizeEmail'); + $di['tools'] = $toolsMock; + $client = new \Box\Mod\Client\Api\Guest(); $client->setDi($di); @@ -337,6 +356,10 @@ class GuestTest extends \BBTestCase { ->will($this->returnValue(null)); $di['validator'] = $validatorMock; + $toolsMock = $this->getMockBuilder('\Box_Tools')->getMock(); + $toolsMock->expects($this->atLeastOnce())->method('generatePassword'); + $di['tools'] = $toolsMock; + $client = new \Box\Mod\Client\Api\Guest(); $client->setDi($di); diff --git a/tests/bb-modules/Staff/Api/AdminTest.php b/tests/bb-modules/Staff/Api/AdminTest.php index 68675a8a3..06c0de922 100644 --- a/tests/bb-modules/Staff/Api/AdminTest.php +++ b/tests/bb-modules/Staff/Api/AdminTest.php @@ -230,6 +230,10 @@ class AdminTest extends \BBTestCase { $di = new \Box_Di(); $di['validator'] = $validatorMock; + $toolsMock = $this->getMockBuilder('\Box_Tools')->getMock(); + $toolsMock->expects($this->atLeastOnce())->method('validateAndSanitizeEmail'); + $di['tools'] = $toolsMock; + $this->api->setDi($di); $this->api->setService($serviceMock); diff --git a/tests/bb-modules/Staff/Api/GuestTest.php b/tests/bb-modules/Staff/Api/GuestTest.php index 4c9e298a4..36a623d52 100644 --- a/tests/bb-modules/Staff/Api/GuestTest.php +++ b/tests/bb-modules/Staff/Api/GuestTest.php @@ -50,6 +50,10 @@ class GuestTest extends \BBTestCase $di['db'] = $dbMock; $di['validator'] = $validatorMock; + $toolsMock = $this->getMockBuilder('\Box_Tools')->getMock(); + $toolsMock->expects($this->atLeastOnce())->method('validateAndSanitizeEmail'); + $di['tools'] = $toolsMock; + $apiMock->setDi($di); $apiMock->setService($serviceMock); @@ -136,6 +140,10 @@ class GuestTest extends \BBTestCase $di = new \Box_Di(); $di['validator'] = $validatorMock; + $toolsMock = $this->getMockBuilder('\Box_Tools')->getMock(); + $toolsMock->expects($this->atLeastOnce())->method('validateAndSanitizeEmail'); + $di['tools'] = $toolsMock; + $guestApi = new \Box\Mod\Staff\Api\Guest(); $guestApi->setMod($modMock); $guestApi->setService($serviceMock); @@ -164,6 +172,10 @@ class GuestTest extends \BBTestCase $di = new \Box_Di(); $di['validator'] = $validatorMock; + $toolsMock = $this->getMockBuilder('\Box_Tools')->getMock(); + $toolsMock->expects($this->atLeastOnce())->method('validateAndSanitizeEmail'); + $di['tools'] = $toolsMock; + $guestApi = new \Box\Mod\Staff\Api\Guest(); $guestApi->setMod($modMock); $guestApi->setDi($di);