Merge pull request mautic#2420 from acquia/MAUT-12024

Maut 12024 - Fixing invalid Page redirects
escopecz · Feb 4, 2025 · 5adeb92 · 5adeb92
1 parent b94713f
commit 5adeb92
Show file tree

Hide file tree

Showing 7 changed files with 87 additions and 52 deletions.
diff --git a/app/bundles/PageBundle/Assets/js/page.js b/app/bundles/PageBundle/Assets/js/page.js
@@ -7,17 +7,6 @@ Mautic.pageOnLoad = function (container, response) {
     if (mQuery(container + ' #page_template').length) {
         Mautic.toggleBuilderButton(mQuery('#page_template').val() == '');
 
-        //Handle autohide of "Redirect URL" field if "Redirect Type" is none
-        if (mQuery(container + ' select[name="page[redirectType]"]').length) {
-            //Auto-hide on page loading
-            Mautic.autoHideRedirectUrl(container);
-
-            //Auto-hide on select changing
-            mQuery(container + ' select[name="page[redirectType]"]').chosen().change(function(){
-                Mautic.autoHideRedirectUrl(container);
-            });
-        }
-
         // Preload tokens for code mode builder
         Mautic.getTokens(Mautic.getBuilderTokensMethod(), function(){});
         Mautic.initSelectTheme(mQuery('#page_template'));
@@ -73,16 +62,3 @@ Mautic.getPageAbTestWinnerForm = function(abKey) {
         }
     });
 };
-
-Mautic.autoHideRedirectUrl = function(container) {
-    var select = mQuery(container + ' select[name="page[redirectType]"]');
-    var input = mQuery(container + ' input[name="page[redirectUrl]"]');
-
-    //If value is none we autohide the "Redirect URL" field and empty it
-    if (select.val() == '') {
-        input.closest('.form-group').hide();
-        input.val('');
-    } else {
-        input.closest('.form-group').show();
-    }
-};
diff --git a/app/bundles/PageBundle/Controller/PublicController.php b/app/bundles/PageBundle/Controller/PublicController.php
@@ -38,6 +38,8 @@
 class PublicController extends AbstractFormController
 {
     /**
+     * @param string $slug
+     *
      * @return Response
      *
      * @throws \Exception
@@ -72,7 +74,11 @@ public function indexAction(
                 if (null != $entity->getRedirectType()) {
                     $model->hitPage($entity, $request, $entity->getRedirectType());
 
-                    return $this->redirect($entity->getRedirectUrl(), (int) $entity->getRedirectType());
+                    if ($entity->getRedirectUrl()) {
+                        return $this->redirect($entity->getRedirectUrl(), (int) $entity->getRedirectType());
+                    } else {
+                        return $this->notFound();
+                    }
                 } else {
                     $model->hitPage($entity, $request, 401);
 

diff --git a/app/bundles/PageBundle/Entity/Page.php b/app/bundles/PageBundle/Entity/Page.php
@@ -236,17 +236,16 @@ public static function loadValidatorMetadata(ClassMetadata $metadata): void
                 $type = $page->getRedirectType();
                 if (!is_null($type)) {
                     $validator  = $context->getValidator();
-                    $violations = $validator->validate($page->getRedirectUrl(), [
-                        new Assert\Url(
-                            [
-                                'message' => 'mautic.core.value.required',
-                            ]
-                        ),
-                    ]);
-
-                    if (count($violations) > 0) {
-                        $string = (string) $violations;
-                        $context->buildViolation($string)
+                    $violations = $validator->validate(
+                        $page->getRedirectUrl(),
+                        [
+                            new Assert\Url(),
+                            new NotBlank(['message' => 'mautic.core.value.required']),
+                        ],
+                    );
+
+                    foreach ($violations as $violation) {
+                        $context->buildViolation($violation->getMessage())
                             ->atPath('redirectUrl')
                             ->addViolation();
                     }
@@ -579,9 +578,7 @@ public function getFooterScript()
     }
 
     /**
-     * Set redirectType.
-     *
-     * @param string $redirectType
+     * @param ?int $redirectType
      *
      * @return Page
      */
@@ -594,9 +591,7 @@ public function setRedirectType($redirectType)
     }
 
     /**
-     * Get redirectType.
-     *
-     * @return string
+     * @return ?int
      */
     public function getRedirectType()
     {

diff --git a/app/bundles/PageBundle/Form/Type/PageType.php b/app/bundles/PageBundle/Form/Type/PageType.php
@@ -271,11 +271,13 @@ function (FormEvent $event) use ($formModifier): void {
                     'class'        => 'form-control',
                     'maxlength'    => 200,
                     'tooltip'      => 'mautic.page.form.redirecturl.help',
+                    'data-hide-on' => '{"page_redirectType":""}',
                     'data-toggle'  => 'field-lookup',
                     'data-action'  => 'page:fieldList',
                     'data-target'  => 'redirectUrl',
                     'data-options' => $redirectUrlDataOptions,
                 ],
+                'default_protocol' => null,
             ]
         );
 

diff --git a/app/bundles/PageBundle/Form/Type/RedirectListType.php b/app/bundles/PageBundle/Form/Type/RedirectListType.php
@@ -4,6 +4,7 @@
 
 use Symfony\Component\Form\AbstractType;
 use Symfony\Component\Form\Extension\Core\Type\ChoiceType;
+use Symfony\Component\HttpFoundation\Response;
 use Symfony\Component\OptionsResolver\OptionsResolver;
 
 /**
@@ -14,11 +15,11 @@ class RedirectListType extends AbstractType
     public function configureOptions(OptionsResolver $resolver): void
     {
         $choices = [
-            'mautic.page.form.redirecttype.permanent'     => 301,
-            'mautic.page.form.redirecttype.temporary'     => 302,
-            'mautic.page.form.redirecttype.303_temporary' => 303,
-            'mautic.page.form.redirecttype.307_temporary' => 307,
-            'mautic.page.form.redirecttype.308_permanent' => 308,
+            'mautic.page.form.redirecttype.permanent'     => Response::HTTP_MOVED_PERMANENTLY,
+            'mautic.page.form.redirecttype.temporary'     => Response::HTTP_FOUND,
+            'mautic.page.form.redirecttype.303_temporary' => Response::HTTP_SEE_OTHER,
+            'mautic.page.form.redirecttype.307_temporary' => Response::HTTP_TEMPORARY_REDIRECT,
+            'mautic.page.form.redirecttype.308_permanent' => Response::HTTP_PERMANENTLY_REDIRECT,
         ];
 
         $resolver->setDefaults([
@@ -29,10 +30,8 @@ public function configureOptions(OptionsResolver $resolver): void
             'label_attr'  => ['class' => 'control-label'],
             'placeholder' => false,
             'required'    => false,
-            'attr'        => [
-                'class' => 'form-control',
-            ],
-            'feature'           => 'all',
+            'attr'        => ['class' => 'form-control'],
+            'feature'     => 'all',
         ]);
 
         $resolver->setDefined(['feature']);

diff --git a/app/bundles/PageBundle/Tests/Controller/PublicControllerRedirectTest.php b/app/bundles/PageBundle/Tests/Controller/PublicControllerRedirectTest.php
@@ -0,0 +1,57 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Mautic\PageBundle\Tests\Controller;
+
+use Mautic\CoreBundle\Test\MauticMysqlTestCase;
+use Mautic\PageBundle\Entity\Page;
+use PHPUnit\Framework\Assert;
+use Symfony\Component\HttpFoundation\Request;
+use Symfony\Component\HttpFoundation\Response;
+
+class PublicControllerRedirectTest extends MauticMysqlTestCase
+{
+    /**
+     * @dataProvider redirectTypeOptions
+     */
+    public function testValidationRedirectWithoutUrl(string $redirectUrl, string $expectedMessage): void
+    {
+        $crawler    = $this->client->request(Request::METHOD_GET, '/s/pages/new');
+        $saveButton = $crawler->selectButton('Save');
+        $form       = $saveButton->form();
+        $form['page[title]']->setValue('Redirect test');
+        $form['page[redirectType]']->setValue((string) Response::HTTP_MOVED_PERMANENTLY);
+        $form['page[redirectUrl]']->setValue($redirectUrl);
+        $form['page[template]']->setValue('mautic_code_mode');
+
+        $this->client->submit($form);
+
+        Assert::assertStringContainsString($expectedMessage, $this->client->getResponse()->getContent());
+    }
+
+    /**
+     * @return iterable<string, array{string, string}>
+     */
+    public function redirectTypeOptions(): iterable
+    {
+        yield 'redirect set, empty redirect URL' => ['', 'A value is required.'];
+        yield 'redirect set, invalid redirect URL' => ['invalid.url', 'This value is not a valid URL.'];
+        yield 'redirect set, valid redirect URL' => ['https://valid.url', 'Edit Page - Redirect test'];
+    }
+
+    public function testCreateRedirectWithNoUrlForExistingPages(): void
+    {
+        $page = new Page();
+        $page->setTitle('Page A');
+        $page->setAlias('page-a');
+        $page->setIsPublished(false);
+        $page->setRedirectType(Response::HTTP_MOVED_PERMANENTLY);
+        $this->em->persist($page);
+        $this->em->flush();
+
+        $this->client->request(Request::METHOD_GET, '/page-a');
+
+        Assert::assertSame(Response::HTTP_NOT_FOUND, $this->client->getResponse()->getStatusCode());
+    }
+}
diff --git a/app/bundles/PageBundle/Tests/Form/Type/RedirectListTypeTest.php b/app/bundles/PageBundle/Tests/Form/Type/RedirectListTypeTest.php
@@ -11,7 +11,7 @@ class RedirectListTypeTest extends TestCase
 {
     private RedirectListType $form;
 
-    public function setUp(): void
+    protected function setUp(): void
     {
         $this->form = new RedirectListType();
     }