Improved input validation for URLs. (vufind-org#3540)

subhh · Mar 22, 2024 · 0d95174 · 0d95174
1 parent 15faf60
commit 0d95174
Show file tree

Hide file tree

Showing 5 changed files with 30 additions and 23 deletions.
diff --git a/module/VuFind/src/VuFind/Controller/AbstractBase.php b/module/VuFind/src/VuFind/Controller/AbstractBase.php
@@ -705,23 +705,16 @@ protected function setFollowupUrlToReferer(bool $allowCurrentUrl = true, array $
             'lbreferer',
             $this->getRequest()->getServer()->get('HTTP_REFERER', null)
         );
-        // Get the referer -- if it's empty, there's nothing to store!
-        if (empty($referer)) {
-            return;
-        }
-        $refererNorm = $this->normalizeUrlForComparison($referer);
-
-        // If the referer lives outside of VuFind, don't store it! We only
+        // Get the referer -- if it's empty, there's nothing to store! Also,
+        // if the referer lives outside of VuFind, don't store it! We only
         // want internal post-login redirects.
-        $baseUrl = $this->getServerUrl('home');
-        $baseUrlNorm = $this->normalizeUrlForComparison($baseUrl);
-        if (!str_starts_with($refererNorm, $baseUrlNorm)) {
+        if (empty($referer) || !$this->isLocalUrl($referer)) {
             return;
         }
-
         // If the referer is the MyResearch/Home action, it probably means
         // that the user is repeatedly mistyping their password. We should
         // ignore this and instead rely on any previously stored referer.
+        $refererNorm = $this->normalizeUrlForComparison($referer);
         $myResearchHomeUrl = $this->getServerUrl('myresearch-home');
         $mrhuNorm = $this->normalizeUrlForComparison($myResearchHomeUrl);
         if ($mrhuNorm === $refererNorm) {
@@ -893,4 +886,17 @@ protected function getRefreshResponse()
         $response->setStatusCode(205);
         return $response;
     }
+
+    /**
+     * Is the provided URL local to this instance?
+     *
+     * @param string $url URL to check
+     *
+     * @return bool
+     */
+    protected function isLocalUrl(string $url): bool
+    {
+        $baseUrlNorm = $this->normalizeUrlForComparison($this->getServerUrl('home'));
+        return str_starts_with($this->normalizeUrlForComparison($url), $baseUrlNorm);
+    }
 }
diff --git a/module/VuFind/src/VuFind/Controller/AbstractRecord.php b/module/VuFind/src/VuFind/Controller/AbstractRecord.php
@@ -480,6 +480,7 @@ public function saveAction()
         if (
             !str_ends_with($referer, '/Save')
             && stripos($referer, 'MyResearch/EditList/NEW') === false
+            && $this->isLocalUrl($referer)
         ) {
             $this->setFollowupUrlToReferer();
         } else {

diff --git a/module/VuFind/src/VuFind/Controller/CartController.php b/module/VuFind/src/VuFind/Controller/CartController.php
@@ -139,9 +139,8 @@ public function searchresultsbulkAction()
         // of this action (for example, because of a login screen), or if we
         // have an external site in the referer, we should ignore that!
         $referer = $this->getRequest()->getServer()->get('HTTP_REFERER');
-        $baseUrl = $this->getServerUrl('home');
         $bulk = $this->url()->fromRoute('cart-searchresultsbulk');
-        if (str_starts_with($referer, $baseUrl) && !str_ends_with($referer, $bulk)) {
+        if ($this->isLocalUrl($referer) && !str_ends_with($referer, $bulk)) {
             $this->session->url = $referer;
         }
 

diff --git a/module/VuFind/src/VuFind/Controller/MyResearchController.php b/module/VuFind/src/VuFind/Controller/MyResearchController.php
@@ -394,7 +394,7 @@ public function logoutAction()
             $logoutTarget = $this->getServerUrl($config->Site->logOutRoute);
         } else {
             $logoutTarget = $this->getRequest()->getServer()->get('HTTP_REFERER');
-            if (empty($logoutTarget)) {
+            if (empty($logoutTarget) || !$this->isLocalUrl($logoutTarget)) {
                 $logoutTarget = $this->getServerUrl('home');
             }
 

diff --git a/module/VuFind/src/VuFind/Controller/SearchController.php b/module/VuFind/src/VuFind/Controller/SearchController.php
@@ -67,8 +67,10 @@ public function editmemoryAction()
     {
         // Get the user's referer, with the home page as a fallback; we'll
         // redirect here after the work is done.
-        $from = $this->getRequest()->getServer()->get('HTTP_REFERER')
-            ?? $this->url()->fromRoute('home');
+        $from = $this->getRequest()->getServer()->get('HTTP_REFERER') ?? null;
+        if (empty($from) || !$this->isLocalUrl($from)) {
+            $from = $this->url()->fromRoute('home');
+        }
 
         // Get parameters:
         $searchClassId = $this->params()
@@ -127,13 +129,12 @@ public function emailAction()
         $mailer->setMaxRecipients($view->maxRecipients);
         // Set up Captcha
         $view->useCaptcha = $this->captcha()->active('email');
-        $view->url = $this->params()->fromPost(
-            'url',
-            $this->params()->fromQuery(
-                'url',
-                $this->getRequest()->getServer()->get('HTTP_REFERER')
-            )
-        );
+        $view->url = $this->params()->fromPost('url')
+            ?? $this->params()->fromQuery('url')
+            ?? $this->getRequest()->getServer()->get('HTTP_REFERER');
+        if (!$this->isLocalUrl($view->url)) {
+            throw new \Exception('Unexpected value passed to emailAction: ' . $view->url);
+        }
 
         // Force login if necessary:
         $config = $this->getConfig();