diff --git a/src/EventSubscriber/RedirectRequestSubscriber.php b/src/EventSubscriber/RedirectRequestSubscriber.php index 8127985..3974ba3 100644 --- a/src/EventSubscriber/RedirectRequestSubscriber.php +++ b/src/EventSubscriber/RedirectRequestSubscriber.php @@ -11,6 +11,7 @@ use Drupal\Core\Config\ConfigFactoryInterface; use Drupal\Core\Language\LanguageManagerInterface; use Drupal\Core\Routing\UrlGenerator; +use Drupal\redirect\Exception\RedirectLoopException; use Drupal\redirect\RedirectChecker; use Drupal\redirect\RedirectRepository; use Symfony\Component\HttpFoundation\RedirectResponse; @@ -89,7 +90,18 @@ public function onKernelRequestCheckRedirect(GetResponseEvent $event) { $this->context->fromRequest($request); - $redirect = $this->redirectRepository->findMatchingRedirect($path, $request_query, $this->languageManager->getCurrentLanguage()->getId()); + try { + $redirect = $this->redirectRepository->findMatchingRedirect($path, $request_query, $this->languageManager->getCurrentLanguage() + ->getId()); + } + catch (RedirectLoopException $e) { + \Drupal::logger('redirect')->warning($e->getMessage()); + $response = new Response(); + $response->setStatusCode(503); + $response->setContent('Service unavailable'); + $event->setResponse($response); + return; + } if (!empty($redirect)) { diff --git a/src/Exception/RedirectLoopException.php b/src/Exception/RedirectLoopException.php new file mode 100644 index 0000000..6c47d5c --- /dev/null +++ b/src/Exception/RedirectLoopException.php @@ -0,0 +1,29 @@ + $path, '%rid' => $rid])); + } + +} diff --git a/src/RedirectRepository.php b/src/RedirectRepository.php index 2d44b5a..49342a2 100644 --- a/src/RedirectRepository.php +++ b/src/RedirectRepository.php @@ -11,6 +11,7 @@ use Drupal\Core\Entity\EntityManagerInterface; use Drupal\Core\Language\Language; use Drupal\redirect\Entity\Redirect; +use Drupal\redirect\Exception\RedirectLoopException; class RedirectRepository { @@ -24,6 +25,13 @@ class RedirectRepository { */ protected $connection; + /** + * An array of found redirect IDs to avoid recursion. + * + * @var array + */ + protected $foundRedirects = []; + /** * Constructs a \Drupal\redirect\EventSubscriber\RedirectRequestSubscriber object. * @@ -49,9 +57,10 @@ public function __construct(EntityManagerInterface $manager, Connection $connect * * @return \Drupal\redirect\Entity\Redirect * The matched redirect entity. + * + * @throws \Drupal\redirect\Exception\RedirectLoopException */ public function findMatchingRedirect($source_path, array $query = [], $language = Language::LANGCODE_NOT_SPECIFIED) { - $hashes = [Redirect::generateHash($source_path, $query, $language)]; if ($language != Language::LANGCODE_NOT_SPECIFIED) { $hashes[] = Redirect::generateHash($source_path, $query, Language::LANGCODE_NOT_SPECIFIED); @@ -61,12 +70,42 @@ public function findMatchingRedirect($source_path, array $query = [], $language $rid = $this->connection->query('SELECT rid FROM {redirect} WHERE hash IN (:hashes[])', [':hashes[]' => $hashes])->fetchField(); if (!empty($rid)) { - return $this->load($rid); + // Check if this is a loop. + if (in_array($rid, $this->foundRedirects)) { + throw new RedirectLoopException('/' . $source_path, $rid); + } + $this->foundRedirects[] = $rid; + + $redirect = $this->load($rid); + + // Find chained redirects. + if ($recursive = $this->findByRedirect($redirect, $language)) { + // Reset found redirects. + $this->foundRedirects = []; + return $recursive; + } + + return $redirect; } return NULL; } + /** + * Helper function to find recursive redirects. + * + * @param \Drupal\redirect\Entity\Redirect + * The redirect object. + * @param string $language + * The language to use. + */ + protected function findByRedirect(Redirect $redirect, $language) { + $uri = $redirect->getRedirectUrl(); + $path = ltrim(parse_url($uri->toString(), PHP_URL_PATH), '/'); + $query = $uri->getOption('query') ?: []; + return $this->findMatchingRedirect($path, $query, $language); + } + /** * Finds redirects based on the source path. * diff --git a/src/Tests/RedirectAPITest.php b/src/Tests/RedirectAPITest.php index cd09002..fedeb29 100644 --- a/src/Tests/RedirectAPITest.php +++ b/src/Tests/RedirectAPITest.php @@ -10,6 +10,7 @@ use Drupal\language\Entity\ConfigurableLanguage; use Drupal\redirect\Entity\Redirect; use Drupal\Core\Language\Language; +use Drupal\redirect\Exception\RedirectLoopException; use Drupal\simpletest\KernelTestBase; /** @@ -39,6 +40,7 @@ public function setUp() { $this->installEntitySchema('redirect'); $this->installEntitySchema('user'); + $this->installSchema('system', ['router']); $this->installConfig(array('redirect')); $language = ConfigurableLanguage::createFromLangcode('de'); @@ -55,6 +57,7 @@ public function testRedirectEntity() { /** @var \Drupal\redirect\Entity\Redirect $redirect */ $redirect = $this->controller->create(); $redirect->setSource('some-url', array('key' => 'val')); + $redirect->setRedirect('node'); $redirect->save(); $this->assertEqual(Redirect::generateHash('some-url', array('key' => 'val'), Language::LANGCODE_NOT_SPECIFIED), $redirect->getHash()); @@ -117,6 +120,49 @@ public function testSortRecursive() { } } + /** + * Test loop detection. + */ + public function testLoopDetection() { + // Add a chained redirect that isn't a loop. + /** @var \Drupal\redirect\Entity\Redirect $one */ + $one = $this->controller->create(); + $one->setSource('my-path'); + $one->setRedirect('node'); + $one->save(); + /** @var \Drupal\redirect\Entity\Redirect $two */ + $two = $this->controller->create(); + $two->setSource('second-path'); + $two->setRedirect('my-path'); + $two->save(); + /** @var \Drupal\redirect\Entity\Redirect $three */ + $three = $this->controller->create(); + $three->setSource('third-path'); + $three->setRedirect('second-path'); + $three->save(); + + /** @var \Drupal\redirect\RedirectRepository $repository */ + $repository = \Drupal::service('redirect.repository'); + $found = $repository->findMatchingRedirect('third-path'); + if (!empty($found)) { + $this->assertEqual($found->getRedirectUrl()->toString(), '/node', 'Chained redirects properly resolved in findMatchingRedirect.'); + } + else { + $this->fail('Failed to resolve a chained redirect.'); + } + + // Create a loop. + $one->setRedirect('third-path'); + $one->save(); + try { + $repository->findMatchingRedirect('third-path'); + $this->fail('Failed to detect a redirect loop.'); + } + catch (RedirectLoopException $e) { + $this->pass('Properly detected a redirect loop.'); + } + } + /** * Test redirect_parse_url(). */ diff --git a/src/Tests/RedirectUITest.php b/src/Tests/RedirectUITest.php index c5723b6..cdc0f3b 100644 --- a/src/Tests/RedirectUITest.php +++ b/src/Tests/RedirectUITest.php @@ -306,7 +306,7 @@ function testRedirectLoop() { $log = reset($log); $this->assertEqual($log->severity, RfcLogLevel::WARNING); $this->assertEqual(SafeMarkup::format($log->message, unserialize($log->variables)), - SafeMarkup::format('Redirect loop identified at %path for redirect %id', array('%path' => '/admin', '%id' => $redirect2->id()))); + SafeMarkup::format('Redirect loop identified at %path for redirect %id', array('%path' => '/node', '%id' => $redirect1->id()))); } }