From b8de551514d3f82eeb2f14f276141edd3c2198bc Mon Sep 17 00:00:00 2001
From: Tom Udding <tomudding@users.noreply.github.com>
Date: Sun, 21 Jul 2024 21:44:32 +0200
Subject: [PATCH] fix: referrer for login not being GET parameter

Quite bad for SEO, however, Google has not indexed the affected pages (ignored).

Also fixes a flaw in the logic that could lead to a "recursion" due to
consecutive failed login attempts.
---
 .../view/activity/activity/view.phtml         | 12 ++--
 module/Activity/view/partial/signupForm.phtml |  6 +-
 module/Application/view/error/403.phtml       |  4 +-
 module/Application/view/partial/admin.phtml   |  6 +-
 .../Application/view/partial/main-nav.phtml   |  5 +-
 .../view/education/education/course.phtml     |  6 +-
 .../view/frontpage/organ/organ.phtml          |  3 +-
 module/User/config/module.config.php          |  3 +-
 module/User/src/Controller/UserController.php | 61 ++++++-------------
 module/User/src/Form/CompanyUserLogin.php     |  8 ---
 module/User/src/Form/UserLogin.php            |  8 ---
 module/User/view/partial/login/company.phtml  |  9 +--
 module/User/view/partial/login/member.phtml   | 10 +--
 module/User/view/user/user/login.phtml        | 17 +++++-
 14 files changed, 60 insertions(+), 98 deletions(-)

diff --git a/module/Activity/view/activity/activity/view.phtml b/module/Activity/view/activity/activity/view.phtml
index aa90264154..857968877e 100644
--- a/module/Activity/view/activity/activity/view.phtml
+++ b/module/Activity/view/activity/activity/view.phtml
@@ -272,10 +272,8 @@ $this->headTitle($this->translate('Activities'));
                         <?php if (!$isAllowedToSubscribe && $signupList->getOnlyGEWIS()): ?>
                             <a href="<?= $this->url(
                                 'user/login',
-                                [
-                                    'user_type' => 'member',
-                                    'redirect_to' => base64_encode($this->serverUrl(true)),
-                                ],
+                                ['user_type' => 'member'],
+                                ['query' => ['redirect_to' => base64_encode($this->serverUrl(true))]],
                             ) ?>"
                                class="btn btn-primary btn-lg">
                                 <span class="fas fa-user-plus"></span> <?= $this->translate('Log in to subscribe') ?>
@@ -341,10 +339,8 @@ $this->headTitle($this->translate('Activities'));
                                             <?php endif; ?>
                                             <a href="<?= $this->url(
                                                 'user/login',
-                                                [
-                                                    'user_type' => 'member',
-                                                    'redirect_to' => base64_encode($this->serverUrl(true)),
-                                                ],
+                                                ['user_type' => 'member'],
+                                                ['query' => ['redirect_to' => base64_encode($this->serverUrl(true))]],
                                             ) ?>">
                                                 <?= $this->translate('Login to view the subscribed members.') ?>
                                             </a>
diff --git a/module/Activity/view/partial/signupForm.phtml b/module/Activity/view/partial/signupForm.phtml
index 8f665a4ad1..d18ed22e68 100644
--- a/module/Activity/view/partial/signupForm.phtml
+++ b/module/Activity/view/partial/signupForm.phtml
@@ -129,10 +129,8 @@ function formElementRender(
                                     $this->translate('Do you have a GEWIS account? <a href="%s">Log in to subscribe.</a>'),
                                     $this->url(
                                         'user/login',
-                                        [
-                                            'user_type' => 'member',
-                                            'redirect_to' => base64_encode($this->serverUrl(true)),
-                                        ],
+                                        ['user_type' => 'member'],
+                                        ['query' => ['redirect_to' => base64_encode($this->serverUrl(true))]],
                                     ),
                                 ) ?>
                             </strong> <?= $this->translate('Or subscribe without a GEWIS membership: ') ?>
diff --git a/module/Application/view/error/403.phtml b/module/Application/view/error/403.phtml
index e3dc88c6fc..7e322eb05c 100644
--- a/module/Application/view/error/403.phtml
+++ b/module/Application/view/error/403.phtml
@@ -21,8 +21,8 @@ use Laminas\View\Renderer\PhpRenderer;
         if ($this->identity() === null): ?>
             <p><?= $this->translate('You might be able to view this page by logging in') ?></p>
             <a href="<?= $this->url(
-                'user/login',
-                ['redirect_to' => base64_encode($this->serverUrl(true))],
+                name: 'user/login',
+                options: ['query' => ['redirect_to' => base64_encode($this->serverUrl(true))]],
             ) ?>">
                 <span class="fas fa-user"></span>
                 <?= $this->translate('Login') ?>
diff --git a/module/Application/view/partial/admin.phtml b/module/Application/view/partial/admin.phtml
index 7e5881cbda..3694f5718e 100644
--- a/module/Application/view/partial/admin.phtml
+++ b/module/Application/view/partial/admin.phtml
@@ -26,10 +26,8 @@ use Laminas\View\Renderer\PhpRenderer;
                         <li>
                             <a href="<?= $this->url(
                                 'user/login',
-                                [
-                                    'user_type' => 'member',
-                                    'redirect_to' => base64_encode($this->serverUrl(true)),
-                                ],
+                                ['user_type' => 'member'],
+                                ['query' => ['redirect_to' => base64_encode($this->serverUrl(true))]],
                             ) ?>">
                                 <?= $this->translate('Login') ?>
                             </a>
diff --git a/module/Application/view/partial/main-nav.phtml b/module/Application/view/partial/main-nav.phtml
index e584837e7a..cc702be844 100644
--- a/module/Application/view/partial/main-nav.phtml
+++ b/module/Application/view/partial/main-nav.phtml
@@ -102,7 +102,10 @@ endif; ?>
                         </a>
                         <ul class="dropdown-menu">
                             <li>
-                                <a href="<?= $this->url('user/login', ['redirect_to' => base64_encode($this->serverUrl(true))]) ?>">
+                                <a href="<?= $this->url(
+                                    name: 'user/login',
+                                    options: ['query' => ['redirect_to' => base64_encode($this->serverUrl(true))]],
+                                ) ?>">
                                     <?= $this->translate('Login') ?>
                                 </a>
                             </li>
diff --git a/module/Education/view/education/education/course.phtml b/module/Education/view/education/education/course.phtml
index b7f5652073..3a699a766d 100644
--- a/module/Education/view/education/education/course.phtml
+++ b/module/Education/view/education/education/course.phtml
@@ -48,10 +48,8 @@ foreach ($documents as $document) {
                         $this->translate('If you want to view these documents, you can either <a href="%s">login</a> or access these documents on the campus.'),
                         $this->url(
                             'user/login',
-                            [
-                                'user_type' => 'member',
-                                'redirect_to' => base64_encode($this->serverUrl(true)),
-                            ],
+                            ['user_type' => 'member'],
+                            ['query' => ['redirect_to' => base64_encode($this->serverUrl(true))]],
                         ),
                     ) ?>
                 </p>
diff --git a/module/Frontpage/view/frontpage/organ/organ.phtml b/module/Frontpage/view/frontpage/organ/organ.phtml
index d844973ff3..7d8dbff026 100644
--- a/module/Frontpage/view/frontpage/organ/organ.phtml
+++ b/module/Frontpage/view/frontpage/organ/organ.phtml
@@ -227,7 +227,8 @@ function getOrganDescription($organInformation, $lang)
                             <p>
                                 <a href="<?= $this->url(
                                     'user/login',
-                                    ['redirect_to' => base64_encode($this->serverUrl(true))],
+                                    ['user_type' => 'member'],
+                                    ['query' => ['redirect_to' => base64_encode($this->serverUrl(true))]],
                                 ) ?>">
                                     <?= $this->translate('Login') ?>
                                 </a>
diff --git a/module/User/config/module.config.php b/module/User/config/module.config.php
index c614955c03..82f295ab15 100644
--- a/module/User/config/module.config.php
+++ b/module/User/config/module.config.php
@@ -45,10 +45,9 @@
                     'login' => [
                         'type' => Segment::class,
                         'options' => [
-                            'route' => '/login[/:user_type][/to/:redirect_to]',
+                            'route' => '/login[/:user_type]',
                             'constraints' => [
                                 'user_type' => '(company|member)',
-                                'redirect_to' => '[a-zA-Z0-9\+\=]+',
                             ],
                             'defaults' => [
                                 'action' => 'login',
diff --git a/module/User/src/Controller/UserController.php b/module/User/src/Controller/UserController.php
index 9498119ca4..936208cb60 100644
--- a/module/User/src/Controller/UserController.php
+++ b/module/User/src/Controller/UserController.php
@@ -12,8 +12,6 @@
 use Laminas\Mvc\I18n\Translator;
 use Laminas\Mvc\Plugin\FlashMessenger\FlashMessenger;
 use Laminas\View\Model\ViewModel;
-use User\Form\CompanyUserLogin as CompanyLoginForm;
-use User\Form\UserLogin as UserLoginForm;
 use User\Permissions\NotAllowedException;
 use User\Service\AclService;
 use User\Service\User as UserService;
@@ -44,18 +42,19 @@ public function loginAction(): Response|ViewModel
         }
 
         $userType = $this->params()->fromRoute('user_type');
-        $redirectTo = $this->params()->fromRoute('redirect_to');
+        $redirectTo = $this->params()->fromQuery('redirect_to');
+
+        if ('company' === $userType) {
+            $form = $this->userService->getCompanyUserLoginForm();
+        } else {
+            $form = $this->userService->getUserLoginForm();
+        }
 
         /** @var Request $request */
         $request = $this->getRequest();
         if ($request->isPost()) {
-            if ('company' === $userType) {
-                $form = $this->userService->getCompanyUserLoginForm();
-            } else {
-                $form = $this->userService->getUserLoginForm();
-            }
-
             $form->setData($request->getPost()->toArray());
+
             if ($form->isValid()) {
                 $data = $form->getData();
 
@@ -67,50 +66,30 @@ public function loginAction(): Response|ViewModel
 
                 if (null !== $login) {
                     return $this->redirect()->toUrl(
-                        $this->decodeRedirect(empty($data['redirect']) ? $redirectTo : $data['redirect']),
+                        $this->decodeRedirect($redirectTo),
                     );
                 }
             }
         }
 
+        if (null === $redirectTo) {
+            $redirectTo = base64_encode(
+                $this->url()->fromRoute(
+                    route: 'home',
+                    options: ['force_canonical' => true],
+                ),
+            );
+        }
+
         return new ViewModel(
             [
-                'form' => $this->handleRedirect($userType, $redirectTo),
+                'form' => $form,
+                'redirectTo' => $redirectTo,
                 'userType' => $userType,
             ],
         );
     }
 
-    private function handleRedirect(
-        string $userType,
-        ?string $referer,
-    ): CompanyLoginForm|UserLoginForm {
-        if ('company' === $userType) {
-            $form = $this->userService->getCompanyUserLoginForm();
-        } else {
-            $form = $this->userService->getUserLoginForm();
-        }
-
-        if (null === $form->get('redirect')->getValue()) {
-            if (null !== $referer) {
-                $form->get('redirect')->setValue($referer);
-
-                return $form;
-            }
-
-            $form->get('redirect')->setValue(
-                base64_encode(
-                    $this->url()->fromRoute(
-                        route: 'home',
-                        options: ['force_canonical' => true],
-                    ),
-                ),
-            );
-        }
-
-        return $form;
-    }
-
     /**
      * Decode the base64 encoded referer, if it is not valid always return the home page.
      */
diff --git a/module/User/src/Form/CompanyUserLogin.php b/module/User/src/Form/CompanyUserLogin.php
index dcab1a300e..91b4f34265 100644
--- a/module/User/src/Form/CompanyUserLogin.php
+++ b/module/User/src/Form/CompanyUserLogin.php
@@ -8,7 +8,6 @@
 use Laminas\Filter\StringTrim;
 use Laminas\Form\Element\Csrf;
 use Laminas\Form\Element\Email;
-use Laminas\Form\Element\Hidden;
 use Laminas\Form\Element\Password;
 use Laminas\Form\Element\Submit;
 use Laminas\Form\Form;
@@ -56,13 +55,6 @@ public function __construct(
             ],
         );
 
-        $this->add(
-            [
-                'name' => 'redirect',
-                'type' => Hidden::class,
-            ],
-        );
-
         $this->add(
             [
                 'name' => 'security',
diff --git a/module/User/src/Form/UserLogin.php b/module/User/src/Form/UserLogin.php
index c78549d785..4781ff8327 100644
--- a/module/User/src/Form/UserLogin.php
+++ b/module/User/src/Form/UserLogin.php
@@ -7,7 +7,6 @@
 use Laminas\Authentication\Result;
 use Laminas\Form\Element\Checkbox;
 use Laminas\Form\Element\Csrf;
-use Laminas\Form\Element\Hidden;
 use Laminas\Form\Element\Password;
 use Laminas\Form\Element\Submit;
 use Laminas\Form\Element\Text;
@@ -68,13 +67,6 @@ public function __construct(
             ],
         );
 
-        $this->add(
-            [
-                'name' => 'redirect',
-                'type' => Hidden::class,
-            ],
-        );
-
         $this->add(
             [
                 'name' => 'security',
diff --git a/module/User/view/partial/login/company.phtml b/module/User/view/partial/login/company.phtml
index cb38778665..47f1215f79 100644
--- a/module/User/view/partial/login/company.phtml
+++ b/module/User/view/partial/login/company.phtml
@@ -9,6 +9,7 @@ use User\Form\CompanyUserLogin as CompanyUserLoginForm;
 /**
  * @var PhpRenderer|HelperTrait $this
  * @var CompanyUserLoginForm $form
+ * @var string $redirectTo
  */
 
 $form->prepare();
@@ -17,10 +18,8 @@ $form->setAttribute(
     'action',
     $this->url(
         'user/login',
-        [
-            'user_type' => 'company',
-            'redirect_to' => base64_encode($this->serverUrl(true)),
-        ],
+        ['user_type' => 'company'],
+        ['query' => ['redirect_to' => $redirectTo]],
     ),
 );
 $form->setAttribute('method', 'post');
@@ -63,8 +62,6 @@ $form->setAttribute('class', 'form-horizontal');
         </div>
     </div>
 
-    <?= $this->formInput($form->get('redirect')) ?>
-
     <div class="form-group">
         <div class="col-sm-offset-5 col-sm-7">
             <?php
diff --git a/module/User/view/partial/login/member.phtml b/module/User/view/partial/login/member.phtml
index 3dd17279c5..3470752573 100644
--- a/module/User/view/partial/login/member.phtml
+++ b/module/User/view/partial/login/member.phtml
@@ -9,18 +9,16 @@ use User\Form\UserLogin as UserLoginForm;
 /**
  * @var PhpRenderer|HelperTrait $this
  * @var UserLoginForm $form
+ * @var string $redirectTo
  */
 
 $form->prepare();
-
 $form->setAttribute(
     'action',
     $this->url(
         'user/login',
-        [
-            'user_type' => 'member',
-            'redirect_to' => base64_encode($this->serverUrl(true)),
-        ],
+        ['user_type' => 'member'],
+        ['query' => ['redirect_to' => $redirectTo]],
     ),
 );
 $form->setAttribute('method', 'post');
@@ -71,8 +69,6 @@ $form->setAttribute('class', 'form-horizontal');
         </div>
     </div>
 
-    <?= $this->formInput($form->get('redirect')) ?>
-
     <div class="form-group">
         <div class="col-sm-offset-5 col-sm-7">
             <?php
diff --git a/module/User/view/user/user/login.phtml b/module/User/view/user/user/login.phtml
index 3e91b6dcbf..baefdbf263 100644
--- a/module/User/view/user/user/login.phtml
+++ b/module/User/view/user/user/login.phtml
@@ -10,6 +10,7 @@ use User\Form\UserLogin as UserLoginForm;
 /**
  * @var PhpRenderer|HelperTrait $this
  * @var CompanyUserLoginForm|UserLoginForm $form
+ * @var string $redirectTo
  * @var string $userType
  */
 
@@ -39,9 +40,21 @@ $isMember = ('member' === $userType);
                         <h3><?= $this->translate('Login') ?></h3>
                     </div>
                     <?php if ($isMember): ?>
-                        <?= $this->partial('partial/login/member.phtml', ['form' => $form]) ?>
+                        <?= $this->partial(
+                            'partial/login/member.phtml',
+                            [
+                                'form' => $form,
+                                'redirectTo' => $redirectTo,
+                            ],
+                        ) ?>
                     <?php else: ?>
-                        <?= $this->partial('partial/login/company.phtml', ['form' => $form]) ?>
+                        <?= $this->partial(
+                            'partial/login/company.phtml',
+                            [
+                                'form' => $form,
+                                'redirectTo' => $redirectTo,
+                            ],
+                        ) ?>
                     <?php endif; ?>
                 </div>
             </div>