diff --git a/lang/en/validation.php b/lang/en/validation.php index 02a87bf05ca..087f0c3adb4 100644 --- a/lang/en/validation.php +++ b/lang/en/validation.php @@ -205,6 +205,7 @@ 'parent_exceeds_max_depth' => 'This would exceed the maximum depth.', 'reserved' => 'This is a reserved word.', 'reserved_field_handle' => 'Field with a handle of :handle is a reserved word.', + 'link_cannot_reference_itself' => 'An entry cannot link to itself.', 'unique_entry_value' => 'This value has already been taken.', 'unique_form_handle' => 'This value has already been taken.', 'unique_term_value' => 'This value has already been taken.', diff --git a/src/Fieldtypes/Link.php b/src/Fieldtypes/Link.php index 65fb1c3a1aa..760a0903b5c 100644 --- a/src/Fieldtypes/Link.php +++ b/src/Fieldtypes/Link.php @@ -77,6 +77,20 @@ protected function configFieldItems(): array ]; } + public function rules(): array + { + return [ + ...parent::rules(), + function ($attribute, $value, $fail) { + $parent = $this->field->parent(); + + if ($this->field->handle() === 'redirect' && $parent instanceof Entry && $value === 'entry::'.$parent->id()) { + $fail(__('statamic::validation.link_cannot_reference_itself')); + } + }, + ]; + } + public function augment($value) { $localize = ! $this->canSelectAcrossSites(); diff --git a/src/Http/Responses/DataResponse.php b/src/Http/Responses/DataResponse.php index 7ae6885df3e..e0d7c769596 100644 --- a/src/Http/Responses/DataResponse.php +++ b/src/Http/Responses/DataResponse.php @@ -82,7 +82,8 @@ protected function getRedirect() // they might have set one of the magic values like @child or entry::some-id. $redirect = ResolveRedirect::resolve($redirect, $this->data); - if ($redirect === 404) { + // Missing target (404) or a redirect loop (null). + if ($redirect === 404 || $redirect === null) { throw new NotFoundHttpException; } diff --git a/src/Routing/ResolveRedirect.php b/src/Routing/ResolveRedirect.php index 4f2e02f215a..710f29d9ccb 100644 --- a/src/Routing/ResolveRedirect.php +++ b/src/Routing/ResolveRedirect.php @@ -11,6 +11,13 @@ class ResolveRedirect { + // Redirects followed in a row before we assume a loop (self-redirect, A -> B -> A). + private const MAX_DEPTH = 10; + + private static int $depth = 0; + + private static bool $cycleDetected = false; + public function __invoke($redirect, $parent = null, $localize = false) { return $this->resolve($redirect, $parent, $localize); @@ -22,6 +29,29 @@ public function resolve($redirect, $parent = null, $localize = false) return null; } + // Resolving an entry's url() resolves its own redirect, so a loop never ends. + // Flag it so every level unwinds to null and entries fall back to their own URL. + if (self::$depth >= self::MAX_DEPTH) { + self::$cycleDetected = true; + + return null; + } + + self::$depth++; + + try { + $result = $this->resolveItem($redirect, $parent, $localize); + + return self::$cycleDetected ? null : $result; + } finally { + if (--self::$depth === 0) { + self::$cycleDetected = false; + } + } + } + + private function resolveItem($redirect, $parent, $localize) + { if (! $item = $this->item($redirect, $parent, $localize)) { return 404; } diff --git a/tests/Fieldtypes/LinkTest.php b/tests/Fieldtypes/LinkTest.php index a2607cf04df..ac389cbd393 100644 --- a/tests/Fieldtypes/LinkTest.php +++ b/tests/Fieldtypes/LinkTest.php @@ -3,6 +3,7 @@ namespace Tests\Fieldtypes; use Facades\Statamic\Routing\ResolveRedirect; +use Illuminate\Support\Facades\Validator; use Mockery; use PHPUnit\Framework\Attributes\DataProvider; use PHPUnit\Framework\Attributes\Test; @@ -46,6 +47,39 @@ public function it_augments_string_to_string() $this->assertEquals(['url' => '/foo'], $augmented->toArray()); } + #[Test] + #[DataProvider('selfReferenceProvider')] + public function it_doesnt_allow_an_entry_to_link_to_itself($value, $passes) + { + $field = new Field('redirect', ['type' => 'link']); + $field->setParent((new Entry)->id('abc')); + + $validator = Validator::make(['redirect' => $value], $field->rules()); + + $this->assertSame($passes, $validator->passes()); + } + + public static function selfReferenceProvider() + { + return [ + 'itself' => ['entry::abc', false], + 'another entry' => ['entry::def', true], + 'url' => ['/abc', true], + 'child' => ['@child', true], + ]; + } + + #[Test] + public function it_allows_an_entry_to_link_to_itself_in_a_non_redirect_field() + { + $field = new Field('cta', ['type' => 'link']); + $field->setParent((new Entry)->id('abc')); + + $validator = Validator::make(['cta' => 'entry::abc'], $field->rules()); + + $this->assertTrue($validator->passes()); + } + #[Test] public function it_augments_reference_to_object() { diff --git a/tests/FrontendTest.php b/tests/FrontendTest.php index ca02226c615..208c4c2b0e4 100644 --- a/tests/FrontendTest.php +++ b/tests/FrontendTest.php @@ -235,6 +235,15 @@ public function drafts_are_not_visible() $this->get('/about')->assertStatus(404); } + #[Test] + public function an_entry_redirecting_to_itself_is_not_found() + { + $this->withStandardFakeErrorViews(); + $this->createPage('about', ['with' => ['redirect' => 'entry::about']]); + + $this->get('/about')->assertStatus(404); + } + #[Test] public function drafts_are_visible_if_using_live_preview() { diff --git a/tests/Routing/ResolveRedirectTest.php b/tests/Routing/ResolveRedirectTest.php index 185bd410860..18b37525db3 100644 --- a/tests/Routing/ResolveRedirectTest.php +++ b/tests/Routing/ResolveRedirectTest.php @@ -14,10 +14,15 @@ use Statamic\Routing\ResolveRedirect; use Statamic\Structures\Page; use Statamic\Structures\Pages; +use Tests\FakesContent; +use Tests\PreventSavingStacheItemsToDisk; use Tests\TestCase; class ResolveRedirectTest extends TestCase { + use FakesContent; + use PreventSavingStacheItemsToDisk; + #[Test] public function it_resolves_standard_redirects() { @@ -280,4 +285,68 @@ public function it_can_resolve_an_arrayable_link() $this->assertEquals('/test', $resolver($arrayableLink)); $this->assertEquals(404, $resolver($arrayableLink)); } + + #[Test] + public function an_entry_redirecting_to_itself_has_no_redirect() + { + $entry = $this->createPage('about', ['with' => ['redirect' => 'entry::about']]); + + $this->assertNull($entry->redirectUrl()); + $this->assertFalse($entry->isRedirect()); + $this->assertEquals('/about', $entry->url()); + } + + #[Test] + public function entries_redirecting_to_each_other_have_no_redirect() + { + $a = $this->createPage('a', ['with' => ['redirect' => 'entry::b']]); + $b = $this->createPage('b', ['with' => ['redirect' => 'entry::a']]); + + $this->assertFalse($a->isRedirect()); + $this->assertFalse($b->isRedirect()); + $this->assertEquals('/a', $a->url()); + $this->assertEquals('/b', $b->url()); + } + + #[Test] + public function a_chain_of_redirects_up_to_the_max_depth_is_followed() + { + $first = $this->createRedirectChain(10); + + $this->assertTrue($first->isRedirect()); + $this->assertEquals('/final', $first->url()); + } + + #[Test] + public function a_chain_of_redirects_beyond_the_max_depth_is_treated_as_a_loop() + { + $first = $this->createRedirectChain(11); + + $this->assertFalse($first->isRedirect()); + $this->assertEquals('/chain-1', $first->url()); + } + + #[Test] + public function a_detected_loop_does_not_affect_later_redirects() + { + $loop = $this->createPage('loop', ['with' => ['redirect' => 'entry::loop']]); + $other = $this->createPage('other', ['with' => ['redirect' => '/final']]); + + $this->assertFalse($loop->isRedirect()); + $this->assertTrue($other->isRedirect()); + $this->assertEquals('/final', $other->url()); + } + + private function createRedirectChain(int $length) + { + foreach (range(1, $length) as $i) { + $redirect = $i === $length ? '/final' : 'entry::chain-'.($i + 1); + + $entry = $this->createPage('chain-'.$i, ['with' => ['redirect' => $redirect]]); + + $first ??= $entry; + } + + return $first; + } }