From 731529455554f7e27cc78e46dc8fa84a89494345 Mon Sep 17 00:00:00 2001 From: Chirag Aggarwal Date: Mon, 20 Jan 2025 05:44:19 +0000 Subject: [PATCH 1/5] fix: phone number parsing exception handling --- app/controllers/api/account.php | 22 +++++++++++++++++++--- 1 file changed, 19 insertions(+), 3 deletions(-) diff --git a/app/controllers/api/account.php b/app/controllers/api/account.php index 6935029450..592b72e83a 100644 --- a/app/controllers/api/account.php +++ b/app/controllers/api/account.php @@ -28,6 +28,7 @@ use Appwrite\Utopia\Database\Validator\CustomId; use Appwrite\Utopia\Database\Validator\Queries\Identities; use Appwrite\Utopia\Request; use Appwrite\Utopia\Response; +use libphonenumber\NumberParseException; use libphonenumber\PhoneNumberUtil; use MaxMind\Db\Reader; use Utopia\Abuse\Abuse; @@ -2467,7 +2468,12 @@ App::post('/v1/account/tokens/phone') $abuse = new Abuse($timelimit); if ($abuse->check() && System::getEnv('_APP_OPTIONS_ABUSE', 'enabled') === 'enabled') { $helper = PhoneNumberUtil::getInstance(); - $countryCode = $helper->parse($phone)->getCountryCode(); + + try { + $countryCode = $helper->parse($phone)->getCountryCode(); + } catch (NumberParseException $e) { + throw new Exception(Exception::GENERAL_ARGUMENT_INVALID, 'Invalid phone number'); + } if (!empty($countryCode)) { $queueForUsage @@ -3587,7 +3593,12 @@ App::post('/v1/account/verification/phone') $abuse = new Abuse($timelimit); if ($abuse->check() && System::getEnv('_APP_OPTIONS_ABUSE', 'enabled') === 'enabled') { $helper = PhoneNumberUtil::getInstance(); - $countryCode = $helper->parse($phone)->getCountryCode(); + + try { + $countryCode = $helper->parse($phone)->getCountryCode(); + } catch (NumberParseException $e) { + throw new Exception(Exception::GENERAL_ARGUMENT_INVALID, 'Invalid phone number'); + } if (!empty($countryCode)) { $queueForUsage @@ -4148,7 +4159,12 @@ App::post('/v1/account/mfa/challenge') $abuse = new Abuse($timelimit); if ($abuse->check() && System::getEnv('_APP_OPTIONS_ABUSE', 'enabled') === 'enabled') { $helper = PhoneNumberUtil::getInstance(); - $countryCode = $helper->parse($phone)->getCountryCode(); + + try { + $countryCode = $helper->parse($phone)->getCountryCode(); + } catch (NumberParseException $e) { + throw new Exception(Exception::GENERAL_ARGUMENT_INVALID, 'Invalid phone number'); + } if (!empty($countryCode)) { $queueForUsage From 8fb772340f2f6a5623e9689646808b1d159bded7 Mon Sep 17 00:00:00 2001 From: Chirag Aggarwal Date: Mon, 20 Jan 2025 08:12:42 +0000 Subject: [PATCH 2/5] chore: shifted validation --- app/controllers/api/account.php | 22 +++------------------- src/Appwrite/Auth/Validator/Phone.php | 13 +++++++++++++ tests/unit/Auth/Validator/PhoneTest.php | 1 + 3 files changed, 17 insertions(+), 19 deletions(-) diff --git a/app/controllers/api/account.php b/app/controllers/api/account.php index 592b72e83a..6935029450 100644 --- a/app/controllers/api/account.php +++ b/app/controllers/api/account.php @@ -28,7 +28,6 @@ use Appwrite\Utopia\Database\Validator\CustomId; use Appwrite\Utopia\Database\Validator\Queries\Identities; use Appwrite\Utopia\Request; use Appwrite\Utopia\Response; -use libphonenumber\NumberParseException; use libphonenumber\PhoneNumberUtil; use MaxMind\Db\Reader; use Utopia\Abuse\Abuse; @@ -2468,12 +2467,7 @@ App::post('/v1/account/tokens/phone') $abuse = new Abuse($timelimit); if ($abuse->check() && System::getEnv('_APP_OPTIONS_ABUSE', 'enabled') === 'enabled') { $helper = PhoneNumberUtil::getInstance(); - - try { - $countryCode = $helper->parse($phone)->getCountryCode(); - } catch (NumberParseException $e) { - throw new Exception(Exception::GENERAL_ARGUMENT_INVALID, 'Invalid phone number'); - } + $countryCode = $helper->parse($phone)->getCountryCode(); if (!empty($countryCode)) { $queueForUsage @@ -3593,12 +3587,7 @@ App::post('/v1/account/verification/phone') $abuse = new Abuse($timelimit); if ($abuse->check() && System::getEnv('_APP_OPTIONS_ABUSE', 'enabled') === 'enabled') { $helper = PhoneNumberUtil::getInstance(); - - try { - $countryCode = $helper->parse($phone)->getCountryCode(); - } catch (NumberParseException $e) { - throw new Exception(Exception::GENERAL_ARGUMENT_INVALID, 'Invalid phone number'); - } + $countryCode = $helper->parse($phone)->getCountryCode(); if (!empty($countryCode)) { $queueForUsage @@ -4159,12 +4148,7 @@ App::post('/v1/account/mfa/challenge') $abuse = new Abuse($timelimit); if ($abuse->check() && System::getEnv('_APP_OPTIONS_ABUSE', 'enabled') === 'enabled') { $helper = PhoneNumberUtil::getInstance(); - - try { - $countryCode = $helper->parse($phone)->getCountryCode(); - } catch (NumberParseException $e) { - throw new Exception(Exception::GENERAL_ARGUMENT_INVALID, 'Invalid phone number'); - } + $countryCode = $helper->parse($phone)->getCountryCode(); if (!empty($countryCode)) { $queueForUsage diff --git a/src/Appwrite/Auth/Validator/Phone.php b/src/Appwrite/Auth/Validator/Phone.php index 26aa687278..2ad4d4fad7 100644 --- a/src/Appwrite/Auth/Validator/Phone.php +++ b/src/Appwrite/Auth/Validator/Phone.php @@ -2,6 +2,8 @@ namespace Appwrite\Auth\Validator; +use libphonenumber\NumberParseException; +use libphonenumber\PhoneNumberUtil; use Utopia\Validator; /** @@ -12,10 +14,12 @@ use Utopia\Validator; class Phone extends Validator { protected bool $allowEmpty; + protected PhoneNumberUtil $helper; public function __construct(bool $allowEmpty = false) { $this->allowEmpty = $allowEmpty; + $this->helper = PhoneNumberUtil::getInstance(); } /** @@ -47,6 +51,15 @@ class Phone extends Validator return true; } + try { + $parsedValue = $this->helper->parse($value); + if (!$this->helper->isValidNumber($parsedValue)) { + return false; + } + } catch (NumberParseException $e) { + return false; + } + return !!\preg_match('/^\+[1-9]\d{6,14}$/', $value); } diff --git a/tests/unit/Auth/Validator/PhoneTest.php b/tests/unit/Auth/Validator/PhoneTest.php index d5a4e7f826..e9103e608a 100644 --- a/tests/unit/Auth/Validator/PhoneTest.php +++ b/tests/unit/Auth/Validator/PhoneTest.php @@ -31,6 +31,7 @@ class PhoneTest extends TestCase $this->assertEquals($this->object->isValid('+0415553452342'), false); $this->assertEquals($this->object->isValid('+14 155 5524564'), false); $this->assertEquals($this->object->isValid('+1415555245634543'), false); + $this->assertEquals($this->object->isValid('+8027547935'), false); $this->assertEquals($this->object->isValid(+14155552456), false); $this->assertEquals($this->object->isValid('+1415555'), true); From ca0459046d21b04c9445c5a4e4c6c0f46d7c18b1 Mon Sep 17 00:00:00 2001 From: Chirag Aggarwal Date: Mon, 20 Jan 2025 08:33:49 +0000 Subject: [PATCH 3/5] chore: remove isValidNumber check --- src/Appwrite/Auth/Validator/Phone.php | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/Appwrite/Auth/Validator/Phone.php b/src/Appwrite/Auth/Validator/Phone.php index 2ad4d4fad7..e74a78d265 100644 --- a/src/Appwrite/Auth/Validator/Phone.php +++ b/src/Appwrite/Auth/Validator/Phone.php @@ -52,10 +52,7 @@ class Phone extends Validator } try { - $parsedValue = $this->helper->parse($value); - if (!$this->helper->isValidNumber($parsedValue)) { - return false; - } + $this->helper->parse($value); } catch (NumberParseException $e) { return false; } From 07630c272defa75a938d37fd16cafb05a9f46225 Mon Sep 17 00:00:00 2001 From: Chirag Aggarwal Date: Mon, 20 Jan 2025 12:41:57 +0000 Subject: [PATCH 4/5] chore: update test --- tests/unit/Auth/Validator/PhoneTest.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/unit/Auth/Validator/PhoneTest.php b/tests/unit/Auth/Validator/PhoneTest.php index e9103e608a..b5308ac8a9 100644 --- a/tests/unit/Auth/Validator/PhoneTest.php +++ b/tests/unit/Auth/Validator/PhoneTest.php @@ -31,7 +31,7 @@ class PhoneTest extends TestCase $this->assertEquals($this->object->isValid('+0415553452342'), false); $this->assertEquals($this->object->isValid('+14 155 5524564'), false); $this->assertEquals($this->object->isValid('+1415555245634543'), false); - $this->assertEquals($this->object->isValid('+8027547935'), false); + $this->assertEquals($this->object->isValid('+8020000000'), false); $this->assertEquals($this->object->isValid(+14155552456), false); $this->assertEquals($this->object->isValid('+1415555'), true); From 27459cc2c0497ecadcd284076dd6de14eedceb3d Mon Sep 17 00:00:00 2001 From: Chirag Aggarwal Date: Tue, 21 Jan 2025 07:04:16 +0000 Subject: [PATCH 5/5] chore: added test comment --- tests/unit/Auth/Validator/PhoneTest.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/unit/Auth/Validator/PhoneTest.php b/tests/unit/Auth/Validator/PhoneTest.php index b5308ac8a9..b7730641c3 100644 --- a/tests/unit/Auth/Validator/PhoneTest.php +++ b/tests/unit/Auth/Validator/PhoneTest.php @@ -31,7 +31,7 @@ class PhoneTest extends TestCase $this->assertEquals($this->object->isValid('+0415553452342'), false); $this->assertEquals($this->object->isValid('+14 155 5524564'), false); $this->assertEquals($this->object->isValid('+1415555245634543'), false); - $this->assertEquals($this->object->isValid('+8020000000'), false); + $this->assertEquals($this->object->isValid('+8020000000'), false); // when country code is not present $this->assertEquals($this->object->isValid(+14155552456), false); $this->assertEquals($this->object->isValid('+1415555'), true);