From 5a54ae8d0a2b29cbd3e7d4292645bc03476cbd85 Mon Sep 17 00:00:00 2001 From: Bradley Schofield Date: Mon, 8 Nov 2021 14:54:28 +0000 Subject: [PATCH 1/8] Update abuse-key for password recovery to add IP Update abuse-key for password recovery to add IP --- app/controllers/api/account.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/controllers/api/account.php b/app/controllers/api/account.php index c236509dd6..cef31772bd 100644 --- a/app/controllers/api/account.php +++ b/app/controllers/api/account.php @@ -1733,7 +1733,7 @@ App::post('/v1/account/recovery') ->label('sdk.response.type', Response::CONTENT_TYPE_JSON) ->label('sdk.response.model', Response::MODEL_TOKEN) ->label('abuse-limit', 10) - ->label('abuse-key', 'url:{url},email:{param-email}') + ->label('abuse-key', 'url:{url},email:{param-email},ip:{ip}') ->param('email', '', new Email(), 'User email.') ->param('url', '', function ($clients) { return new Host($clients); }, 'URL to redirect the user back to your app from the recovery email. Only URLs from hostnames in your project platform list are allowed. This requirement helps to prevent an [open redirect](https://cheatsheetseries.owasp.org/cheatsheets/Unvalidated_Redirects_and_Forwards_Cheat_Sheet.html) attack against your project API.', false, ['clients']) ->inject('request') From 24b747453f4d086fcde3869c72c18b5ae67828d4 Mon Sep 17 00:00:00 2001 From: Bradley Schofield Date: Tue, 9 Nov 2021 10:21:24 +0000 Subject: [PATCH 2/8] Add the ability for multiple abuse-keys --- app/controllers/api/account.php | 2 +- app/controllers/shared/api.php | 75 +++++++++++++++++++++------------ 2 files changed, 48 insertions(+), 29 deletions(-) diff --git a/app/controllers/api/account.php b/app/controllers/api/account.php index cef31772bd..16a6df19c4 100644 --- a/app/controllers/api/account.php +++ b/app/controllers/api/account.php @@ -1733,7 +1733,7 @@ App::post('/v1/account/recovery') ->label('sdk.response.type', Response::CONTENT_TYPE_JSON) ->label('sdk.response.model', Response::MODEL_TOKEN) ->label('abuse-limit', 10) - ->label('abuse-key', 'url:{url},email:{param-email},ip:{ip}') + ->label('abuse-key', ['url:{url},email:{param-email}', 'ip:{ip}']) ->param('email', '', new Email(), 'User email.') ->param('url', '', function ($clients) { return new Host($clients); }, 'URL to redirect the user back to your app from the recovery email. Only URLs from hostnames in your project platform list are allowed. This requirement helps to prevent an [open redirect](https://cheatsheetseries.owasp.org/cheatsheets/Unvalidated_Redirects_and_Forwards_Cheat_Sheet.html) attack against your project API.', false, ['clients']) ->inject('request') diff --git a/app/controllers/shared/api.php b/app/controllers/shared/api.php index 5c76b4d807..d219083363 100644 --- a/app/controllers/shared/api.php +++ b/app/controllers/shared/api.php @@ -37,41 +37,60 @@ App::init(function ($utopia, $request, $response, $project, $user, $register, $e /* * Abuse Check */ - $timeLimit = new TimeLimit($route->getLabel('abuse-key', 'url:{url},ip:{ip}'), $route->getLabel('abuse-limit', 0), $route->getLabel('abuse-time', 3600), $db); - $timeLimit->setNamespace('app_'.$project->getId()); - $timeLimit - ->setParam('{userId}', $user->getId()) - ->setParam('{userAgent}', $request->getUserAgent('')) - ->setParam('{ip}', $request->getIP()) - ->setParam('{url}', $request->getHostname().$route->getPath()) - ; + $abuseKeyLabel = $route->getLabel('abuse-key', 'url:{url},ip:{ip}'); + $timeLimitArray = []; + if (is_array($abuseKeyLabel)) { + for ($i = 0; $i < count($abuseKeyLabel); $i++) { + $timeLimit = new TimeLimit($abuseKeyLabel[$i], $route->getLabel('abuse-limit', 0), $route->getLabel('abuse-time', 3600), $db); + $timeLimit->setNamespace('app_'.$project->getId()); + $timeLimit + ->setParam('{userId}', $user->getId()) + ->setParam('{userAgent}', $request->getUserAgent('')) + ->setParam('{ip}', $request->getIP()) + ->setParam('{url}', $request->getHostname().$route->getPath()) + ; + $timeLimitArray[] = $timeLimit; + } + } else { + $timeLimit = new TimeLimit($abuseKeyLabel, $route->getLabel('abuse-limit', 0), $route->getLabel('abuse-time', 3600), $db); + $timeLimit->setNamespace('app_'.$project->getId()); + $timeLimit + ->setParam('{userId}', $user->getId()) + ->setParam('{userAgent}', $request->getUserAgent('')) + ->setParam('{ip}', $request->getIP()) + ->setParam('{url}', $request->getHostname().$route->getPath()) + ; + $timeLimitArray[] = $timeLimit; + } //TODO make sure we get array here - foreach ($request->getParams() as $key => $value) { // Set request params as potential abuse keys - if(!empty($value)) { - $timeLimit->setParam('{param-'.$key.'}', (\is_array($value)) ? \json_encode($value) : $value); + foreach ($timeLimitArray as $timeLimit) { + foreach ($request->getParams() as $key => $value) { // Set request params as potential abuse keys + if(!empty($value)) { + $timeLimit->setParam('{param-'.$key.'}', (\is_array($value)) ? \json_encode($value) : $value); + } } - } - $abuse = new Abuse($timeLimit); + $abuse = new Abuse($timeLimit); - if ($timeLimit->limit()) { - $response - ->addHeader('X-RateLimit-Limit', $timeLimit->limit()) - ->addHeader('X-RateLimit-Remaining', $timeLimit->remaining()) - ->addHeader('X-RateLimit-Reset', $timeLimit->time() + $route->getLabel('abuse-time', 3600)) - ; - } + if ($timeLimit->limit()) { + $response + ->addHeader('X-RateLimit-Limit', $timeLimit->limit()) + ->addHeader('X-RateLimit-Remaining', $timeLimit->remaining()) + ->addHeader('X-RateLimit-Reset', $timeLimit->time() + $route->getLabel('abuse-time', 3600)) + ; + } - $isPrivilegedUser = Auth::isPrivilegedUser(Authorization::$roles); - $isAppUser = Auth::isAppUser(Authorization::$roles); - - if (($abuse->check() // Route is rate-limited - && App::getEnv('_APP_OPTIONS_ABUSE', 'enabled') !== 'disabled') // Abuse is not diabled - && (!$isAppUser && !$isPrivilegedUser)) // User is not an admin or API key - { - throw new Exception('Too many requests', 429); + $isPrivilegedUser = Auth::isPrivilegedUser(Authorization::$roles); + $isAppUser = Auth::isAppUser(Authorization::$roles); + + if (($abuse->check() // Route is rate-limited + && App::getEnv('_APP_OPTIONS_ABUSE', 'enabled') !== 'disabled') // Abuse is not diabled + && (!$isAppUser && !$isPrivilegedUser)) // User is not an admin or API key + { + throw new Exception('Too many requests', 429); + } } /* From b78715f353c7af669c34c79aa69c8e7c5cfb5bc3 Mon Sep 17 00:00:00 2001 From: Bradley Schofield Date: Tue, 9 Nov 2021 14:07:10 +0000 Subject: [PATCH 3/8] Update api.php --- app/controllers/shared/api.php | 29 ++++++++++++----------------- 1 file changed, 12 insertions(+), 17 deletions(-) diff --git a/app/controllers/shared/api.php b/app/controllers/shared/api.php index d219083363..53e0a60741 100644 --- a/app/controllers/shared/api.php +++ b/app/controllers/shared/api.php @@ -39,32 +39,26 @@ App::init(function ($utopia, $request, $response, $project, $user, $register, $e */ $abuseKeyLabel = $route->getLabel('abuse-key', 'url:{url},ip:{ip}'); $timeLimitArray = []; - if (is_array($abuseKeyLabel)) { - for ($i = 0; $i < count($abuseKeyLabel); $i++) { - $timeLimit = new TimeLimit($abuseKeyLabel[$i], $route->getLabel('abuse-limit', 0), $route->getLabel('abuse-time', 3600), $db); - $timeLimit->setNamespace('app_'.$project->getId()); - $timeLimit - ->setParam('{userId}', $user->getId()) - ->setParam('{userAgent}', $request->getUserAgent('')) - ->setParam('{ip}', $request->getIP()) - ->setParam('{url}', $request->getHostname().$route->getPath()) - ; - $timeLimitArray[] = $timeLimit; - } - } else { - $timeLimit = new TimeLimit($abuseKeyLabel, $route->getLabel('abuse-limit', 0), $route->getLabel('abuse-time', 3600), $db); + + if (!is_array($abuseKeyLabel)) { + $abuseKeyLabel = [$abuseKeyLabel]; + } + + foreach ($abuseKeyLabel as $abuseKey) { + $timeLimit = new TimeLimit($abuseKey, $route->getLabel('abuse-limit', 0), $route->getLabel('abuse-time', 3600), $db); $timeLimit->setNamespace('app_'.$project->getId()); $timeLimit ->setParam('{userId}', $user->getId()) ->setParam('{userAgent}', $request->getUserAgent('')) ->setParam('{ip}', $request->getIP()) - ->setParam('{url}', $request->getHostname().$route->getPath()) - ; + ->setParam('{url}', $request->getHostname().$route->getPath()); $timeLimitArray[] = $timeLimit; } //TODO make sure we get array here + $closestLimit = 999; + foreach ($timeLimitArray as $timeLimit) { foreach ($request->getParams() as $key => $value) { // Set request params as potential abuse keys if(!empty($value)) { @@ -74,7 +68,8 @@ App::init(function ($utopia, $request, $response, $project, $user, $register, $e $abuse = new Abuse($timeLimit); - if ($timeLimit->limit()) { + if ($timeLimit->limit() && $timeLimit->remaining() < $closestLimit) { + $closestLimit = $timeLimit->remaining(); $response ->addHeader('X-RateLimit-Limit', $timeLimit->limit()) ->addHeader('X-RateLimit-Remaining', $timeLimit->remaining()) From eb73bee0935b34b0f98a221d0f926da07fc58dfb Mon Sep 17 00:00:00 2001 From: Bradley Schofield Date: Tue, 9 Nov 2021 14:45:11 +0000 Subject: [PATCH 4/8] Update api.php --- app/controllers/shared/api.php | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/app/controllers/shared/api.php b/app/controllers/shared/api.php index 53e0a60741..7221288b66 100644 --- a/app/controllers/shared/api.php +++ b/app/controllers/shared/api.php @@ -58,6 +58,8 @@ App::init(function ($utopia, $request, $response, $project, $user, $register, $e //TODO make sure we get array here $closestLimit = 999; + $isPrivilegedUser = Auth::isPrivilegedUser(Authorization::$roles); + $isAppUser = Auth::isAppUser(Authorization::$roles); foreach ($timeLimitArray as $timeLimit) { foreach ($request->getParams() as $key => $value) { // Set request params as potential abuse keys @@ -76,9 +78,6 @@ App::init(function ($utopia, $request, $response, $project, $user, $register, $e ->addHeader('X-RateLimit-Reset', $timeLimit->time() + $route->getLabel('abuse-time', 3600)) ; } - - $isPrivilegedUser = Auth::isPrivilegedUser(Authorization::$roles); - $isAppUser = Auth::isAppUser(Authorization::$roles); if (($abuse->check() // Route is rate-limited && App::getEnv('_APP_OPTIONS_ABUSE', 'enabled') !== 'disabled') // Abuse is not diabled From a101c06c62298a679467132bc69780f49ce2c887 Mon Sep 17 00:00:00 2001 From: Bradley Schofield Date: Tue, 9 Nov 2021 14:52:32 +0000 Subject: [PATCH 5/8] Update api.php --- app/controllers/shared/api.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/controllers/shared/api.php b/app/controllers/shared/api.php index 7221288b66..5229390f2c 100644 --- a/app/controllers/shared/api.php +++ b/app/controllers/shared/api.php @@ -57,7 +57,7 @@ App::init(function ($utopia, $request, $response, $project, $user, $register, $e //TODO make sure we get array here - $closestLimit = 999; + $closestLimit = null; $isPrivilegedUser = Auth::isPrivilegedUser(Authorization::$roles); $isAppUser = Auth::isAppUser(Authorization::$roles); @@ -70,7 +70,7 @@ App::init(function ($utopia, $request, $response, $project, $user, $register, $e $abuse = new Abuse($timeLimit); - if ($timeLimit->limit() && $timeLimit->remaining() < $closestLimit) { + if ($timeLimit->limit() && $timeLimit->remaining() < $closestLimit || is_null($closestLimit)) { $closestLimit = $timeLimit->remaining(); $response ->addHeader('X-RateLimit-Limit', $timeLimit->limit()) From 89cc417e3fd0c9bf82ec49529ba70734758222e2 Mon Sep 17 00:00:00 2001 From: Bradley Schofield Date: Tue, 9 Nov 2021 14:56:25 +0000 Subject: [PATCH 6/8] Update api.php --- app/controllers/shared/api.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/controllers/shared/api.php b/app/controllers/shared/api.php index 5229390f2c..968fdfc9fd 100644 --- a/app/controllers/shared/api.php +++ b/app/controllers/shared/api.php @@ -70,7 +70,7 @@ App::init(function ($utopia, $request, $response, $project, $user, $register, $e $abuse = new Abuse($timeLimit); - if ($timeLimit->limit() && $timeLimit->remaining() < $closestLimit || is_null($closestLimit)) { + if ($timeLimit->limit() && ($timeLimit->remaining() < $closestLimit || is_null($closestLimit))) { $closestLimit = $timeLimit->remaining(); $response ->addHeader('X-RateLimit-Limit', $timeLimit->limit()) From 6184c4c93bf5fbc48f618f169fb2a3b56d325a0e Mon Sep 17 00:00:00 2001 From: Bradley Schofield Date: Wed, 24 Nov 2021 09:57:25 +0000 Subject: [PATCH 7/8] Add Eldad's Suggestion --- app/controllers/shared/api.php | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/app/controllers/shared/api.php b/app/controllers/shared/api.php index 968fdfc9fd..3e72389b55 100644 --- a/app/controllers/shared/api.php +++ b/app/controllers/shared/api.php @@ -40,9 +40,7 @@ App::init(function ($utopia, $request, $response, $project, $user, $register, $e $abuseKeyLabel = $route->getLabel('abuse-key', 'url:{url},ip:{ip}'); $timeLimitArray = []; - if (!is_array($abuseKeyLabel)) { - $abuseKeyLabel = [$abuseKeyLabel]; - } + $abuseKeyLabel = (!is_array($abuseKeyLabel)) ? [$abuseKeyLabel] : $abuseKeyLabel; foreach ($abuseKeyLabel as $abuseKey) { $timeLimit = new TimeLimit($abuseKey, $route->getLabel('abuse-limit', 0), $route->getLabel('abuse-time', 3600), $db); From 879bdf1a748c4d1f59c752242111782b0f5d912c Mon Sep 17 00:00:00 2001 From: Bradley Schofield Date: Mon, 29 Nov 2021 09:52:12 +0000 Subject: [PATCH 8/8] Update api.php --- app/controllers/shared/api.php | 2 -- 1 file changed, 2 deletions(-) diff --git a/app/controllers/shared/api.php b/app/controllers/shared/api.php index 8ecd61c0ba..304a265303 100644 --- a/app/controllers/shared/api.php +++ b/app/controllers/shared/api.php @@ -53,8 +53,6 @@ App::init(function ($utopia, $request, $response, $project, $user, $register, $e $timeLimitArray[] = $timeLimit; } - //TODO make sure we get array here - $closestLimit = null; $isPrivilegedUser = Auth::isPrivilegedUser(Authorization::$roles); $isAppUser = Auth::isAppUser(Authorization::$roles);