Detect redirect loops before redirecting.

- Redirects are searched for recursively to avoid loops.
- Addresses issue md-systems#16.

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)) {
 

src/Exception/RedirectLoopException.php

@@ -0,0 +1,29 @@
+<?php
+
+/**
+ * @file
+ * Contains \Drupal\redirect\Exception\RedirectLoopException.
+ */
+
+namespace Drupal\redirect\Exception;
+
+use Drupal\Component\Utility\SafeMarkup;
+
+/**
+ * Exception for when a redirect loop is detected.
+ */
+class RedirectLoopException extends \RuntimeException {
+
+  /**
+   * Formats a redirect loop exception message.
+   *
+   * @param string $path
+   *   The path that results in a redirect loop.
+   * @param int $rid
+   *   The redirect ID that is involved in a loop.
+   */
+  public function __construct($path, $rid) {
+    parent::__construct(SafeMarkup::format('Redirect loop identified at %path for redirect %rid', ['%path' => $path, '%rid' => $rid]));
+  }
+
+}

src/RedirectRepository.php

@@ -12,6 +12,7 @@
 use Drupal\Core\Entity\EntityManagerInterface;
 use Drupal\Core\Language\Language;
 use Drupal\redirect\Entity\Redirect;
+use Drupal\redirect\Exception\RedirectLoopException;
 
 class RedirectRepository {
 
@@ -30,6 +31,13 @@ class RedirectRepository {
    */
   protected $config;
 
+  /**
+   * An array of found redirect IDs to avoid recursion.
+   *
+   * @var array
+   */
+  protected $foundRedirects = [];
+
   /**
    * Constructs a \Drupal\redirect\EventSubscriber\RedirectRequestSubscriber object.
    *
@@ -56,9 +64,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);
@@ -76,12 +85,42 @@ public function findMatchingRedirect($source_path, array $query = [], $language
     $rid = $this->connection->query('SELECT rid FROM {redirect} WHERE hash IN (:hashes[]) ORDER BY LENGTH(redirect_source__query) DESC', [':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.
    *

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());
@@ -164,6 +167,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().
    */

src/Tests/RedirectUITest.php

@@ -307,7 +307,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' => Url::fromUri('base:admin')->toString(), '%id' => $redirect2->id())));
+        SafeMarkup::format('Redirect loop identified at %path for redirect %id', array('%path' => '/node', '%id' => $redirect1->id())));
     }
   }