refactor: added AbstractProxy base class for shortener proxies

CREDITS.md

@@ -6,6 +6,7 @@
 * rugk - security review, doc improvment, JS refactoring & various other stuff
 * R4SAS - python client, compression, blob URI to support larger attachments
 * Mikhail Romanov - UI improvements, theme switching, clipboard support, multi-file upload, bugfixes, code refactoring
+* karthikkasturi - shlink proxy and url shortening bugfixes
 
 ## Past contributions
 

lib/AbstractProxy.php

@@ -0,0 +1,139 @@
+<?php declare(strict_types=1);
+/**
+ * PrivateBin
+ *
+ * a zero-knowledge paste bin
+ *
+ * @link      https://github.com/PrivateBin/PrivateBin
+ * @copyright 2012 Sébastien SAUVAGE (sebsauvage.net)
+ * @license   https://www.opensource.org/licenses/zlib-license.php The zlib/libpng License
+ */
+
+namespace PrivateBin;
+
+use Exception;
+
+/**
+ * AbstractProxy
+ *
+ * Forwards a URL for shortening to shlink and stores the result.
+ */
+abstract class AbstractProxy
+{
+    /**
+     * error message
+     *
+     * @access private
+     * @var    string
+     */
+    private $_error = '';
+
+    /**
+     * shortened URL
+     *
+     * @access private
+     * @var    string
+     */
+    private $_url = '';
+
+    /**
+     * constructor
+     *
+     * initializes and runs the proxy class
+     *
+     * @access public
+     * @param string $link
+     */
+    public function __construct(Configuration $conf, $link)
+    {
+        if (!str_starts_with($link, $conf->getKey('basepath') . '?')) {
+            $this->_error = 'Trying to shorten a URL that isn\'t pointing at our instance.';
+            return;
+        }
+
+        $data = $this->_getcontents($conf, $link);
+
+        if ($data == null) {
+            $this->_error = 'Error calling proxy. Probably a configuration issue';
+            error_log('Error calling proxy: ' . $this->_error);
+            return;
+        }
+
+        if ($data === false) {
+            $http_response_header = $http_response_header ?? array();
+            $statusCode           = '';
+            if (!empty($http_response_header) && preg_match('/HTTP\/\d+\.\d+\s+(\d+)/', $http_response_header[0], $matches)) {
+                $statusCode = $matches[1];
+            }
+            $this->_error = 'Error calling proxy. HTTP request failed. Status code: ' . $statusCode;
+            error_log('Error calling proxy: ' . $this->_error);
+            return;
+        }
+
+        try {
+            $data = Json::decode($data);
+        } catch (Exception $e) {
+            $this->_error = 'Error calling proxy. Probably a configuration issue, like wrong or missing config keys.';
+            error_log('Error calling proxy: ' . $e->getMessage());
+            return;
+        }
+
+        $url = $this->_extractShortUrl($data);
+
+        if ($url === null) {
+            $this->_error = 'Error calling proxy. Probably a configuration issue, like wrong or missing config keys.';
+        } else {
+            $this->_url = $url;
+        }
+    }
+
+    /**
+     * Returns the (untranslated) error message
+     *
+     * @access public
+     * @return string
+     */
+    public function getError()
+    {
+        return $this->_error;
+    }
+
+    /**
+     * Returns the shortened URL
+     *
+     * @access public
+     * @return string
+     */
+    public function getUrl()
+    {
+        return $this->_url;
+    }
+
+    /**
+     * Returns true if any error has occurred
+     *
+     * @access public
+     * @return bool
+     */
+    public function isError()
+    {
+        return !empty($this->_error);
+    }
+
+    /**
+     * Abstract method to get contents from a URL.
+     *
+     * @param Configuration $conf
+     * @param string $link
+     * @return mixed
+     */
+    abstract protected function _getcontents(Configuration $conf, string $link);
+
+    /**
+     * Abstract method to extract the shortUrl from the response
+     *
+     * @param array $data
+     * @return ?string
+     */
+    abstract protected function _extractShortUrl(array $data): ?string;
+}

lib/Controller.php

@@ -149,10 +149,10 @@ class Controller
                 $this->_jsonld($this->_request->getParam('jsonld'));
                 return;
             case 'yourlsproxy':
-                $this->_yourlsproxy($this->_request->getParam('link'));
+                $this->_shortenerproxy($this->_request->getParam('link'), YourlsProxy::class);
                 break;
             case 'shlinkproxy':
-                $this->_shlinkproxy($this->_request->getParam('link'));
+                $this->_shortenerproxy($this->_request->getParam('link'), ShlinkProxy::class);
                 break;
         }
 
@@ -454,12 +454,12 @@ class Controller
         $page->assign('NAME', $this->_conf->getKey('name'));
         if ($this->_request->getOperation() === 'yourlsproxy') {
             $page->assign('SHORTURL', $this->_status);
-            $page->draw('yourlsproxy');
+            $page->draw('shortenerproxy');
             return;
         }
         if ($this->_request->getOperation() === 'shlinkproxy') {
             $page->assign('SHORTURL', $this->_status);
-            $page->draw('shlinkproxy');
+            $page->draw('shortenerproxy');
             return;
         }
         $page->assign('BASEPATH', I18n::_($this->_conf->getKey('basepath')));
@@ -536,34 +536,23 @@ class Controller
     }
 
     /**
-     * proxies link to YOURLS, updates status or error with response
+     * Proxies a link using the specified proxy class, and updates the status or error with the response.
      *
      * @access private
-     * @param string $link
+     * @param string $link The link to be proxied.
+     * @param string $proxyClass The fully qualified class name of the proxy to use.
      */
-    private function _yourlsproxy($link)
+    private function _shortenerproxy($link, $proxyClass)
     {
-        $yourls = new YourlsProxy($this->_conf, $link);
-        if ($yourls->isError()) {
-            $this->_error = $yourls->getError();
-        } else {
-            $this->_status = $yourls->getUrl();
+        if (!is_subclass_of($proxyClass, AbstractProxy::class)) {
+            $this->_error = 'Invalid proxy class.';
+            return;
         }
-    }
-
-    /**
-     * proxies link to SHLINK, updates status or error with response
-     *
-     * @access private
-     * @param string $link
-     */
-    private function _shlinkproxy($link)
-    {
-        $shlink = new ShlinkProxy($this->_conf, $link);
-        if ($shlink->isError()) {
-            $this->_error = $shlink->getError();
+        $proxy = new $proxyClass($this->_conf, $link);
+        if ($proxy->isError()) {
+            $this->_error = $proxy->getError();
         } else {
-            $this->_status = $shlink->getUrl();
+            $this->_status = $proxy->getUrl();
         }
     }
 

lib/ShlinkProxy.php

@@ -11,130 +11,74 @@
 
 namespace PrivateBin;
 
-use Exception;
-
 /**
  * ShlinkProxy
  *
  * Forwards a URL for shortening to shlink and stores the result.
  */
-class ShlinkProxy
+class ShlinkProxy extends AbstractProxy
 {
-    /**
-     * error message
-     *
-     * @access private
-     * @var    string
-     */
-    private $_error = '';
-
-    /**
-     * shortened URL
-     *
-     * @access private
-     * @var    string
-     */
-    private $_url = '';
-
     /**
      * constructor
      *
-     * initializes and runs PrivateBin
+     * initializes and runs ShlinkProxy
      *
      * @access public
      * @param string $link
      */
     public function __construct(Configuration $conf, $link)
     {
-        if (!str_starts_with($link, $conf->getKey('basepath') . '?')) {
-            $this->_error = 'Trying to shorten a URL that isn\'t pointing at our instance.';
-            return;
-        }
+        parent::__construct($conf, $link);
+    }
 
+    /**
+     * Overrides the abstract parent function to get contents from Shlink API.
+     *
+     * @access protected
+     * @return string
+     */
+    protected function _getcontents(Configuration $conf, string $link)
+    {
         $shlink_api_url = $conf->getKey('apiurl', 'shlink');
         $shlink_api_key = $conf->getKey('apikey', 'shlink');
 
         if (empty($shlink_api_url) || empty($shlink_api_key)) {
-            $this->_error = 'Error calling Shlink. Probably a configuration issue, like wrong or missing "apiurl" or "apikey".';
             return;
         }
 
-        $data = file_get_contents(
+        $body = array(
+            'longUrl' => $link,
+        );
+
+        return file_get_contents(
             $shlink_api_url, false, stream_context_create(
                 array(
                     'http' => array(
                         'method'  => 'POST',
                         'header'  => "Content-Type: application/json\r\n" .
                                      'X-Api-Key: ' . $shlink_api_key . "\r\n",
-                        'content' => json_encode(
-                            array(
-                                'longUrl'       => $link,
-                            )
-                        ),
+                        'content' => Json::encode($body),
                     ),
                 )
             )
         );
-        if ($data === false) {
-            $http_response_header = $http_response_header ?? array();
-            $statusCode           = '';
-            if (!empty($http_response_header) && preg_match('/HTTP\/\d+\.\d+\s+(\d+)/', $http_response_header[0], $matches)) {
-                $statusCode = $matches[1];
-            }
-            $this->_error = 'Error calling shlink. HTTP request failed for URL ' . $shlink_api_url . '. Status code: ' . $statusCode;
-            error_log('Error calling shlink: HTTP request failed for URL ' . $shlink_api_url . '. Status code: ' . $statusCode);
-            return;
-        }
-        try {
-            $data = Json::decode($data);
-        } catch (Exception $e) {
-            $this->_error = 'Error calling shlink. Probably a configuration issue, like wrong or missing "apiurl" or "apikey".';
-            error_log('Error calling shlink: ' . $e->getMessage());
-            return;
-        }
-
-        if (
-            !is_null($data) &&
-            // array_key_exists('statusCode', $data) &&
-            // $data['statusCode'] == 200 &&
-            array_key_exists('shortUrl', $data)
-        ) {
-            $this->_url = $data['shortUrl'];
-        } else {
-            $this->_error = 'Error parsing shlink response.';
-        }
     }
 
     /**
-     * Returns the (untranslated) error message
+     * Extracts the short URL from the shlink API response.
      *
-     * @access public
-     * @return string
-     */
-    public function getError()
-    {
-        return $this->_error;
-    }
-
-    /**
-     * Returns the shortened URL
-     *
-     * @access public
-     * @return string
+     * @access protected
+     * @param array $data
+     * @return ?string
      */
-    public function getUrl()
+    protected function _extractShortUrl(array $data): ?string
     {
-        return $this->_url;
-    }
-
-    /**
-     * Returns true if any error has occurred
-     *
-     * @access public
-     * @return bool
-     */
-    public function isError()
-    {
-        return !empty($this->_error);
+        if (
+            !is_null($data) &&
+            array_key_exists('shortUrl', $data)
+        ) {
+            return $data['shortUrl'];
+        }
+        return null;
     }
 }

lib/YourlsProxy.php

@@ -11,54 +11,42 @@
 
 namespace PrivateBin;
 
-use Exception;
-
 /**
  * YourlsProxy
  *
  * Forwards a URL for shortening to YOURLS (your own URL shortener) and stores
  * the result.
  */
-class YourlsProxy
+class YourlsProxy extends AbstractProxy
 {
-    /**
-     * error message
-     *
-     * @access private
-     * @var    string
-     */
-    private $_error = '';
-
-    /**
-     * shortened URL
-     *
-     * @access private
-     * @var    string
-     */
-    private $_url = '';
-
     /**
      * constructor
      *
-     * initializes and runs PrivateBin
+     * initializes and runs YourlsProxy
      *
      * @access public
      * @param string $link
      */
     public function __construct(Configuration $conf, $link)
     {
-        if (!str_starts_with($link, $conf->getKey('basepath') . '?')) {
-            $this->_error = 'Trying to shorten a URL that isn\'t pointing at our instance.';
-            return;
-        }
+        parent::__construct($conf, $link);
+    }
 
+    /**
+     * Overrides the abstract parent function to get contents from YOURLS API.
+     *
+     * @access protected
+     * @return string
+     */
+    protected function _getcontents(Configuration $conf, string $link)
+    {
         $yourls_api_url = $conf->getKey('apiurl', 'yourls');
+
         if (empty($yourls_api_url)) {
-            $this->_error = 'Error calling YOURLS. Probably a configuration issue, like wrong or missing "apiurl" or "signature".';
-            return;
+            return null;
         }
 
-        $data = file_get_contents(
+        return file_get_contents(
             $yourls_api_url, false, stream_context_create(
                 array(
                     'http' => array(
@@ -76,56 +64,25 @@ class YourlsProxy
                 )
             )
         );
-        try {
-            $data = Json::decode($data);
-        } catch (Exception $e) {
-            $this->_error = 'Error calling YOURLS. Probably a configuration issue, like wrong or missing "apiurl" or "signature".';
-            error_log('Error calling YOURLS: ' . $e->getMessage());
-            return;
-        }
+    }
 
+    /**
+     * Extracts the short URL from the YOURLS API response.
+     *
+     * @access protected
+     * @param array $data
+     * @return ?string
+     */
+    protected function _extractShortUrl(array $data): ?string
+    {
         if (
             !is_null($data) &&
             array_key_exists('statusCode', $data) &&
             $data['statusCode'] == 200 &&
             array_key_exists('shorturl', $data)
         ) {
-            $this->_url = $data['shorturl'];
-        } else {
-            $this->_error = 'Error parsing YOURLS response.';
+            return $data['shorturl'];
         }
-    }
-
-    /**
-     * Returns the (untranslated) error message
-     *
-     * @access public
-     * @return string
-     */
-    public function getError()
-    {
-        return $this->_error;
-    }
-
-    /**
-     * Returns the shortened URL
-     *
-     * @access public
-     * @return string
-     */
-    public function getUrl()
-    {
-        return $this->_url;
-    }
-
-    /**
-     * Returns true if any error has occurred
-     *
-     * @access public
-     * @return bool
-     */
-    public function isError()
-    {
-        return !empty($this->_error);
+        return null;
     }
 }

tpl/yourlsproxy.php

@@ -1,27 +0,0 @@
-<?php declare(strict_types=1);
-use PrivateBin\I18n;
-?><!DOCTYPE html>
-<html lang="<?php echo I18n::getLanguage(); ?>"<?php echo I18n::isRtl() ? ' dir="rtl"' : ''; ?>>
-	<head>
-		<meta charset="utf-8" />
-		<meta http-equiv="Content-Security-Policy" content="<?php echo I18n::encode($CSPHEADER); ?>">
-		<meta name="robots" content="noindex" />
-		<meta name="google" content="notranslate">
-		<title><?php echo I18n::_($NAME); ?></title>
-	</head>
-	<body>
-<?php
-if (empty($ERROR)) :
-?>
-		<p><?php echo I18n::_('Your document is <a id="pasteurl" href="%s">%s</a> <span id="copyhint">(Hit <kbd>Ctrl</kbd>+<kbd>c</kbd> to copy)</span>', $SHORTURL, $SHORTURL); ?></p>
-<?php
-else:
-?>
-		<div id="errormessage">
-			<p><?php echo I18n::_('Could not create document: %s', $ERROR); ?></p>
-		</div>
-<?php
-endif;
-?>
-	</body>
-</html>

tst/JsonApiTest.php

@@ -336,6 +336,6 @@ class JsonApiTest extends TestCase
         new Controller;
         $content = ob_get_contents();
         ob_end_clean();
-        $this->assertStringContainsString('Error calling YOURLS.', $content, 'outputs error correctly');
+        $this->assertStringContainsString('Error calling proxy.', $content, 'outputs error correctly');
     }
 }

tst/ViewTest.php

@@ -106,8 +106,8 @@ class ViewTest extends TestCase
                 $content,
                 $template . ': outputs error correctly'
             );
-            if ($template === 'yourlsproxy' || $template === 'shlinkproxy') {
-                // yourlsproxy and shlinkproxy templates only display error message
+            if ($template === 'shortenerproxy') {
+                // shortenerproxy template only displays error message
                 continue;
             }
             $this->assertMatchesRegularExpression(

tst/YourlsProxyTest.php

@@ -68,7 +68,7 @@ class YourlsProxyTest extends TestCase
 
         $yourls = new YourlsProxy($this->_conf, 'https://example.com/?foo#bar');
         $this->assertTrue($yourls->isError());
-        $this->assertEquals($yourls->getError(), 'Error parsing YOURLS response.');
+        $this->assertEquals($yourls->getError(), 'Error calling proxy. Probably a configuration issue, like wrong or missing config keys.');
     }
 
     public function testServerError()
@@ -78,6 +78,6 @@ class YourlsProxyTest extends TestCase
 
         $yourls = new YourlsProxy($this->_conf, 'https://example.com/?foo#bar');
         $this->assertTrue($yourls->isError());
-        $this->assertEquals($yourls->getError(), 'Error calling YOURLS. Probably a configuration issue, like wrong or missing "apiurl" or "signature".');
+        $this->assertEquals($yourls->getError(), 'Error calling proxy. Probably a configuration issue, like wrong or missing config keys.');
     }
 }

vendor/composer/autoload_classmap.php

@@ -66,6 +66,7 @@ return array(
     'Jdenticon\\Shapes\\ShapeCategory' => $vendorDir . '/jdenticon/jdenticon/src/Shapes/ShapeCategory.php',
     'Jdenticon\\Shapes\\ShapeDefinitions' => $vendorDir . '/jdenticon/jdenticon/src/Shapes/ShapeDefinitions.php',
     'PhpToken' => $vendorDir . '/symfony/polyfill-php80/Resources/stubs/PhpToken.php',
+    'PrivateBin\\AbstractProxy' => $baseDir . '/lib/AbstractProxy.php',
     'PrivateBin\\Configuration' => $baseDir . '/lib/Configuration.php',
     'PrivateBin\\Controller' => $baseDir . '/lib/Controller.php',
     'PrivateBin\\Data\\AbstractData' => $baseDir . '/lib/Data/AbstractData.php',
@@ -86,11 +87,11 @@ return array(
     'PrivateBin\\Persistence\\ServerSalt' => $baseDir . '/lib/Persistence/ServerSalt.php',
     'PrivateBin\\Persistence\\TrafficLimiter' => $baseDir . '/lib/Persistence/TrafficLimiter.php',
     'PrivateBin\\Request' => $baseDir . '/lib/Request.php',
+    'PrivateBin\\ShlinkProxy' => $baseDir . '/lib/ShlinkProxy.php',
     'PrivateBin\\TemplateSwitcher' => $baseDir . '/lib/TemplateSwitcher.php',
     'PrivateBin\\View' => $baseDir . '/lib/View.php',
     'PrivateBin\\Vizhash16x16' => $baseDir . '/lib/Vizhash16x16.php',
     'PrivateBin\\YourlsProxy' => $baseDir . '/lib/YourlsProxy.php',
-    'PrivateBin\\ShlinkProxy' => $baseDir . '/lib/ShlinkProxy.php',
     'Stringable' => $vendorDir . '/symfony/polyfill-php80/Resources/stubs/Stringable.php',
     'Symfony\\Polyfill\\Php80\\Php80' => $vendorDir . '/symfony/polyfill-php80/Php80.php',
     'Symfony\\Polyfill\\Php80\\PhpToken' => $vendorDir . '/symfony/polyfill-php80/PhpToken.php',

vendor/composer/autoload_static.php

@@ -114,6 +114,7 @@ class ComposerStaticInitDontChange
         'Jdenticon\\Shapes\\ShapeCategory' => __DIR__ . '/..' . '/jdenticon/jdenticon/src/Shapes/ShapeCategory.php',
         'Jdenticon\\Shapes\\ShapeDefinitions' => __DIR__ . '/..' . '/jdenticon/jdenticon/src/Shapes/ShapeDefinitions.php',
         'PhpToken' => __DIR__ . '/..' . '/symfony/polyfill-php80/Resources/stubs/PhpToken.php',
+        'PrivateBin\\AbstractProxy' => __DIR__ . '/../..' . '/lib/AbstractProxy.php',
         'PrivateBin\\Configuration' => __DIR__ . '/../..' . '/lib/Configuration.php',
         'PrivateBin\\Controller' => __DIR__ . '/../..' . '/lib/Controller.php',
         'PrivateBin\\Data\\AbstractData' => __DIR__ . '/../..' . '/lib/Data/AbstractData.php',
@@ -134,11 +135,11 @@ class ComposerStaticInitDontChange
         'PrivateBin\\Persistence\\ServerSalt' => __DIR__ . '/../..' . '/lib/Persistence/ServerSalt.php',
         'PrivateBin\\Persistence\\TrafficLimiter' => __DIR__ . '/../..' . '/lib/Persistence/TrafficLimiter.php',
         'PrivateBin\\Request' => __DIR__ . '/../..' . '/lib/Request.php',
+        'PrivateBin\\ShlinkProxy' => __DIR__ . '/../..' . '/lib/ShlinkProxy.php',
         'PrivateBin\\TemplateSwitcher' => __DIR__ . '/../..' . '/lib/TemplateSwitcher.php',
         'PrivateBin\\View' => __DIR__ . '/../..' . '/lib/View.php',
         'PrivateBin\\Vizhash16x16' => __DIR__ . '/../..' . '/lib/Vizhash16x16.php',
         'PrivateBin\\YourlsProxy' => __DIR__ . '/../..' . '/lib/YourlsProxy.php',
-        'PrivateBin\\ShlinkProxy' => __DIR__ . '/../..' . '/lib/ShlinkProxy.php',
         'Stringable' => __DIR__ . '/..' . '/symfony/polyfill-php80/Resources/stubs/Stringable.php',
         'Symfony\\Polyfill\\Php80\\Php80' => __DIR__ . '/..' . '/symfony/polyfill-php80/Php80.php',
         'Symfony\\Polyfill\\Php80\\PhpToken' => __DIR__ . '/..' . '/symfony/polyfill-php80/PhpToken.php',