From cb074d5da6a1251de5137329bb7bba37ff1cf93c Mon Sep 17 00:00:00 2001 From: Noel Light-Hilary Date: Thu, 4 Feb 2021 11:14:19 +0000 Subject: [PATCH] WPK-8, #339 - tweak IP rate limit checks * use a more Symfony-standard way to determine forwarded IPs * move request table check/update logic to a repository helper * use QB + ORM there as appropriate for cleaner code & fewer DB vendor hacks --- src/Controller/MainController.php | 68 +++------------------------- src/Entity/Request.php | 20 ++++++++- src/Entity/RequestRepository.php | 75 +++++++++++++++++++++++++++++++ web/index.php | 9 ++++ 4 files changed, 107 insertions(+), 65 deletions(-) create mode 100644 src/Entity/RequestRepository.php diff --git a/src/Controller/MainController.php b/src/Controller/MainController.php index b1effd4..f9fcff8 100644 --- a/src/Controller/MainController.php +++ b/src/Controller/MainController.php @@ -12,7 +12,6 @@ use Outlandish\Wpackagist\Service; use Pagerfanta\Doctrine\ORM\QueryAdapter; use Pagerfanta\Pagerfanta; -use PDO; use Psr\Log\LoggerInterface; use Symfony\Bundle\FrameworkBundle\Controller\AbstractController; use Symfony\Component\Form\Extension\Core\Type\ChoiceType; @@ -27,6 +26,7 @@ use Symfony\Component\HttpFoundation\Response; use Symfony\Component\HttpKernel\Exception\NotFoundHttpException; use Symfony\Component\Routing\Annotation\Route; +use Outlandish\Wpackagist\Entity\Request as RequestLog; use Outlandish\Wpackagist\Storage; class MainController extends AbstractController @@ -204,20 +204,13 @@ public function update( return new Response('Not Found',404); } - $safeName = $package->getName(); - - if (isset($_SERVER['HTTP_X_FORWARDED_FOR']) && $_SERVER['HTTP_X_FORWARDED_FOR']) { - $splitIp = explode(',', $_SERVER['HTTP_X_FORWARDED_FOR']); - $userIp = trim($splitIp[0]); - } else { - $userIp = $_SERVER['REMOTE_ADDR']; - } - - $count = $this->getRequestCountByIp($userIp, $connection); - if ($count > 5) { + $requestCount = $entityManager->getRepository(RequestLog::class) + ->getRequestCountByIp($request->getClientIp()); + if ($requestCount > 5) { return new Response('Too many requests. Try again in an hour.', 403); } + $safeName = $package->getName(); $package = $updateService->updateOne($logger, $safeName); if ($package && !empty($package->getVersions()) && $package->isActive()) { // update just the package @@ -260,55 +253,4 @@ private function getForm(): FormInterface return $this->form; } - - /** - * @param string $ip - * @param Connection $db - * @return int The number of requests within the past 24 hours - * @throws \Doctrine\DBAL\DBALException - * @todo move to a Repository helper? - */ - private function getRequestCountByIp(string $ip, Connection $db): int - { - $query = $db->prepare( - "SELECT * FROM requests WHERE ip_address = :ip AND last_request > :cutoff" - ); - $query->execute([ - 'cutoff' => (new \DateTime())->sub(new \DateInterval('PT1H'))->format($db->getDatabasePlatform()->getDateTimeFormatString()), - 'ip' => $ip, - ]); - - $requestHistory = $query->fetch(PDO::FETCH_ASSOC); - if (!$requestHistory) { - $this->resetRequestCount($ip, $db); - return 1; - } - - $update = $db->prepare( - 'UPDATE requests SET request_count = request_count + 1, last_request = CURRENT_TIMESTAMP WHERE ip_address = :ip' - ); - - $update->execute([ $ip ]); - return $requestHistory['request_count'] + 1; - } - - /** - * Add an entry to the requests table for the provided IP address. - * Has the side effect of removing all expired entries. - * - * @param string $ip - * @param Connection $db - * @throws \Doctrine\DBAL\DBALException - * @todo move to a Repository helper? - */ - private function resetRequestCount(string $ip, Connection $db) - { - $prune = $db->prepare('DELETE FROM requests WHERE last_request < :cutoff'); - $prune->bindValue('cutoff', (new \DateTime())->sub(new \DateInterval('PT1H'))->format($db->getDatabasePlatform()->getDateTimeFormatString())); - $prune->execute(); - $insert = $db->prepare( - 'INSERT INTO requests (ip_address, last_request, request_count) VALUES (:ip_address, CURRENT_TIMESTAMP, 1)' - ); - $insert->execute([ $ip ]); - } } diff --git a/src/Entity/Request.php b/src/Entity/Request.php index 764bffc..5de1b74 100644 --- a/src/Entity/Request.php +++ b/src/Entity/Request.php @@ -6,7 +6,7 @@ use Doctrine\ORM\Mapping as ORM; /** - * @ORM\Entity() + * @ORM\Entity(repositoryClass=RequestRepository::class) * @ORM\Table(name="requests") */ class Request @@ -35,5 +35,21 @@ class Request * @ORM\Column(type="integer") * @var int */ - protected $requestCount; + protected $requestCount = 0; + + public function addRequest(): void + { + $this->requestCount++; + $this->lastRequest = new \DateTime(); + } + + public function getRequestCount(): int + { + return $this->requestCount; + } + + public function setIpAddress(string $ipAddress): void + { + $this->ipAddress = $ipAddress; + } } diff --git a/src/Entity/RequestRepository.php b/src/Entity/RequestRepository.php new file mode 100644 index 0000000..eaac6d2 --- /dev/null +++ b/src/Entity/RequestRepository.php @@ -0,0 +1,75 @@ +getEntityManager(); + $qb = new QueryBuilder($em); + + $oneHourAgo = (new \DateTime())->sub(new \DateInterval('PT1H')); + + $qb->select('r') + ->from(Request::class, 'r') + ->where('r.ipAddress = :ip') + ->andWhere('r.lastRequest >= :cutoff') + ->setParameter('ip', $ip) + ->setParameter('cutoff', $oneHourAgo); + $requestHistory = $qb->getQuery()->getResult(); + + if (empty($requestHistory)) { + $this->resetRequestCount($ip, $oneHourAgo); + return 1; + } + + /** @var Request $requestItem */ + $requestItem = $requestHistory[0]; + $requestItem->addRequest(); + $em->persist($requestItem); + + return $requestItem->getRequestCount(); + } + + /** + * Add an entry to the requests table for the provided IP address. + * Has the side effect of removing all expired entries. + * + * @param string $ip + * @param \DateTime $cutoff + */ + private function resetRequestCount(string $ip, \DateTime $cutoff) + { + // Prune any old records. + $em = $this->getEntityManager(); + $qb = new QueryBuilder($em); + $qb->delete(Request::class, 'r') + ->where('r.ipAddress = :ip') + ->andWhere('r.lastRequest < :cutoff') + ->setParameter('ip', $ip) + ->setParameter('cutoff', $cutoff) + ->getQuery() + ->execute(); + + // Ensure the old record is deleted at DB level before the insert coming up, so we don't + // fall foul of the IP uniqueness constraint. + $em->flush(); + + // Add a new Request record and set it up with `requestCount` 1 and `lastRequest` now. + $requestItem = new Request(); + $requestItem->setIpAddress($ip); + $requestItem->addRequest(); + $em->persist($requestItem); + } +} diff --git a/web/index.php b/web/index.php index aa207f5..d3ab760 100644 --- a/web/index.php +++ b/web/index.php @@ -14,6 +14,15 @@ if ($trustedProxies = $_SERVER['TRUSTED_PROXIES'] ?? false) { Request::setTrustedProxies(explode(',', $trustedProxies), Request::HEADER_X_FORWARDED_ALL ^ Request::HEADER_X_FORWARDED_HOST); +} else { + /** + * If no env override, get the correct request context behind an AWS load balancer. + * @link https://symfony.com/doc/5.2/deployment/proxies.html + */ + Request::setTrustedProxies( + ['127.0.0.1', 'REMOTE_ADDR'], // REMOTE_ADDR string replaced at runtime. + Request::HEADER_X_FORWARDED_AWS_ELB + ); } if ($trustedHosts = $_SERVER['TRUSTED_HOSTS'] ?? false) {