diff --git a/CREDITS.md b/CREDITS.md index 37026898..aabc732e 100644 --- a/CREDITS.md +++ b/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 diff --git a/lib/AbstractProxy.php b/lib/AbstractProxy.php new file mode 100644 index 00000000..f02229fd --- /dev/null +++ b/lib/AbstractProxy.php @@ -0,0 +1,139 @@ +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; +} diff --git a/lib/Controller.php b/lib/Controller.php index 104985b0..31e2dbfe 100644 --- a/lib/Controller.php +++ b/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(); } } diff --git a/lib/ShlinkProxy.php b/lib/ShlinkProxy.php index d33f2c7f..56915609 100644 --- a/lib/ShlinkProxy.php +++ b/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; - } + } + /** + * Extracts the short URL from the shlink 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 shlink 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; } } diff --git a/lib/YourlsProxy.php b/lib/YourlsProxy.php index a7c6ad6d..4f60ba78 100644 --- a/lib/YourlsProxy.php +++ b/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; } } diff --git a/tpl/shlinkproxy.php b/tpl/shortenerproxy.php similarity index 100% rename from tpl/shlinkproxy.php rename to tpl/shortenerproxy.php diff --git a/tpl/yourlsproxy.php b/tpl/yourlsproxy.php deleted file mode 100644 index bbd454e9..00000000 --- a/tpl/yourlsproxy.php +++ /dev/null @@ -1,27 +0,0 @@ - -> - - - - - - <?php echo I18n::_($NAME); ?> - - - -

%s (Hit Ctrl+c to copy)', $SHORTURL, $SHORTURL); ?>

- -
-

-
- - - diff --git a/tst/JsonApiTest.php b/tst/JsonApiTest.php index 3bbd2114..bbb1cc62 100644 --- a/tst/JsonApiTest.php +++ b/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'); } } diff --git a/tst/ViewTest.php b/tst/ViewTest.php index c1e7aad3..fcb0bdac 100644 --- a/tst/ViewTest.php +++ b/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( diff --git a/tst/YourlsProxyTest.php b/tst/YourlsProxyTest.php index 389f510d..11eb6708 100644 --- a/tst/YourlsProxyTest.php +++ b/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.'); } } diff --git a/vendor/composer/autoload_classmap.php b/vendor/composer/autoload_classmap.php index 2ef68746..a89cb900 100644 --- a/vendor/composer/autoload_classmap.php +++ b/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', diff --git a/vendor/composer/autoload_static.php b/vendor/composer/autoload_static.php index a4b6c6b2..1d86d957 100644 --- a/vendor/composer/autoload_static.php +++ b/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',