Merge pull request #1639 from PrivateBin/urlshortener/url-vadility

Strengthen validation of URL in proxy services
Merge pull request #1645 from karthikkasturi/master
2025-10-03 09:49:14 +02:00 · 2025-09-12 00:05:02 +02:00 · 2025-09-11 20:55:34 +02:00 · 2025-09-11 18:17:30 +01:00 · 2025-09-10 16:02:06 +01:00 · 2025-09-03 20:13:33 +02:00
7 changed files with 75 additions and 7 deletions
--- a/.devcontainer/postCreateCommand.sh
+++ b/.devcontainer/postCreateCommand.sh
@ -1,9 +1,16 @@
 #!/bin/sh

 export PATH="$PATH:$HOME/.composer/vendor/bin"
+export PATH="$PATH:$PWD/vendor/bin"
+echo 'export PATH="$PATH:$HOME/.composer/vendor/bin"' >> ~/.bashrc
+echo 'export PATH="$PATH:$PWD/vendor/bin"' >> ~/.bashrc
 ln -s ./conf.sample.php cfg/conf.php
 composer install --no-dev --optimize-autoloader

+# for PHP unit testing
+# composer require google/cloud-storage
+# composer install --optimize-autoloader
+
 sudo chmod a+x "$(pwd)" && sudo rm -rf /var/www/html && sudo ln -s "$(pwd)" /var/www/html

 npm install --global nyc
--- a/.gitignore
+++ b/.gitignore
@ -25,6 +25,7 @@ vendor/**/tst
 vendor/**/tests
 vendor/**/build_phar.php
 !vendor/**/*.php
+vendor/bin/**

 # Ignore local node modules, unit testing logs, api docs and IDE project files
 js/node_modules/
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@ -6,6 +6,7 @@
 * FIXED: Allow copying the shortened link after using a URL shortener (#1624)
 * ADDED: Auto shorten URLs with config option `shortenbydefault` (#1627)
 * ADDED: Added `shortenviashlink` endpoint with an `shlink` configuration section 
+* FIXED: Check for quotes and conical braces when extracting short url (#1644)

 ## 2.0.0 (2025-07-28)
 * ADDED: Error logging in database and filesystem backend (#1554)
--- a/js/test/PasteStatus.js
+++ b/js/test/PasteStatus.js
@ -149,7 +149,6 @@ describe('PasteStatus', function () {
                        '<html lang="en">\n' +
                        '\t<head>\n' +
                        '\t\t<meta charset="utf-8" />\n' +
-                        '\t\t<meta http-equiv="Content-Security-Policy" content="default-src \'none\'; base-uri \'self\'; form-action \'none\'; manifest-src \'self\'; connect-src * blob:; script-src \'self\' \'unsafe-eval\'; style-src \'self\'; font-src \'self\'; frame-ancestors \'none\'; img-src \'self\' data: blob:; media-src blob:; object-src blob:; sandbox allow-same-origin allow-scripts allow-forms allow-popups allow-modals allow-downloads">\n' +
                        '\t\t<meta name="robots" content="noindex" />\n' +
                        '\t\t<meta name="google" content="notranslate">\n' +
                        '\t\t<title>PrivateBin</title>\n' +
--- a/lib/Proxy/AbstractProxy.php
+++ b/lib/Proxy/AbstractProxy.php
@ -49,7 +49,14 @@ abstract class AbstractProxy
     */
    public function __construct(Configuration $conf, string $link)
    {
-        if (!str_starts_with($link, $conf->getKey('basepath') . '?')) {
+        if (!filter_var($link, FILTER_VALIDATE_URL, FILTER_FLAG_PATH_REQUIRED | FILTER_FLAG_QUERY_REQUIRED)) {
+            $this->_error = 'Invalid URL given.';
+            return;
+        }
+
+        if (!str_starts_with($link, $conf->getKey('basepath') . '?') ||
+            parse_url($link, PHP_URL_HOST) != parse_url($conf->getKey('basepath'), PHP_URL_HOST)
+        ) {
            $this->_error = 'Trying to shorten a URL that isn\'t pointing at our instance.';
            return;
        }
--- a/tpl/shortenerproxy.php
+++ b/tpl/shortenerproxy.php
@ -4,7 +4,6 @@ use PrivateBin\I18n;
 <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>
--- a/tst/YourlsProxyTest.php
+++ b/tst/YourlsProxyTest.php
@ -45,22 +45,76 @@ class YourlsProxyTest extends TestCase
        $yourls = new YourlsProxy($this->_conf, 'https://example.com/?foo#bar');
        $this->assertFalse($yourls->isError());
        $this->assertEquals($yourls->getUrl(), 'https://example.com/1');
+
+        $yourls = new YourlsProxy($this->_conf, 'https://example.com/?@foreign.malicious.example?foo#bar');
+        $this->assertFalse($yourls->isError());
+        $this->assertEquals($yourls->getUrl(), 'https://example.com/1');
    }

-    public function testForeignUrl()
+    /**
+     * @dataProvider providerInvalidUrl
+     */
+    public function testImvalidUrl($url): void
    {
-        $yourls = new YourlsProxy($this->_conf, 'https://other.example.com/?foo#bar');
+        $yourls = new YourlsProxy($this->_conf, $url);
+        $this->assertTrue($yourls->isError());
+        $this->assertEquals($yourls->getError(), 'Invalid URL given.');
+    }
+
+    public function providerInvalidUrl(): array
+    {
+        return array(
+            array(''),
+            array(' '),
+            array('foo'),
+            array('https://'),
+            array('https://example.com'), // missing path and query parameter,
+            array('https://example.com/'), // missing query parameter
+            array('https://example.com?paste=something'), // missing path parameter
+            array('https://example.com@foreign.malicious.example?foo#bar'), // missing path parameter
+        );
+    }
+
+    /**
+     * This tests for a trick using username of an URI, see:
+     * {@see https://cloud.google.com/blog/topics/threat-intelligence/url-obfuscation-schema-abuse/?hl=en}
+     *
+     * @dataProvider providerForeignUrlUsernameTrick
+     */
+    public function testForeignUrlUsingUsernameTrick($url): void
+    {
+        $yourls = new YourlsProxy($this->_conf, $url);
        $this->assertTrue($yourls->isError());
        $this->assertEquals($yourls->getError(), 'Trying to shorten a URL that isn\'t pointing at our instance.');
    }

-    public function testSneakyForeignUrl()
+    public function providerForeignUrlUsernameTrick(): array
    {
-        $yourls = new YourlsProxy($this->_conf, 'https://other.example.com/?q=https://example.com/?foo#bar');
+        return array(
+            array('https://example.com@foreign.malicious.example/?foo#bar'),
+            array('https://example.com/@foreign.malicious.example?foo#bar'),
+        );
+    }
+
+    /**
+     * @dataProvider providerForeignUrl
+     */
+    public function testForeignUrl($url): void
+    {
+        $yourls = new YourlsProxy($this->_conf, $url);
        $this->assertTrue($yourls->isError());
        $this->assertEquals($yourls->getError(), 'Trying to shorten a URL that isn\'t pointing at our instance.');
    }

+    public function providerForeignUrl(): array
+    {
+        return array(
+            array('ftp://example.com/?n=np'), // wrong protocol
+            array('https://other.example.com/?foo#bar'), // wrong domain
+            array('https://other.example.com/?q=https://example.com/?foo#bar'), // domain included inside string
+        );
+    }
+
    public function testYourlsError()
    {
        // when statusCode is not 200, shorturl may not have been set
Author	SHA1	Message	Date
rugk	394b4cb33d	Merge pull request #1639 from PrivateBin/urlshortener/url-vadility Strengthen validation of URL in proxy services	2025-09-12 00:05:02 +02:00
El RIDO	2b8b5d71d2	Merge pull request #1645 from karthikkasturi/master fix regex check for short url in response	2025-09-11 20:55:34 +02:00
Karthik Kasturi	24afa5a1d8	removed CSP tag from shortenerproxy.php	2025-09-11 18:17:30 +01:00
Karthik Kasturi	191ed63b04	fix regex check for short url in response	2025-09-10 16:02:06 +01:00
El RIDO	bd61a3d021	enable tests to pass The path is only optional when it is / and the very last element, otherwise it is required. As soon as it is in the middle of a URL it helps the parser to identify which part is the username and domain and what is path and GET parameters. The @ sign is legitimate, if unusual, in the latter two.	2025-09-03 20:13:33 +02:00
rugk	a6034ace1b	test: PHP considers this invalid	2025-09-03 14:25:04 +00:00
rugk	616635c66c	style: scruintizer wants some trailing comma	2025-09-03 14:21:00 +00:00
rugk	e4f2383dd8	test: more test cases for testForeignUrlUsingUsernameTrick	2025-09-03 14:20:03 +00:00
rugk	25dca0838e	style(codespaces): comment PHP unit testing setup for now	2025-09-03 14:14:08 +00:00
rugk	cfc687d62b	style: fix indentation	2025-09-03 14:12:12 +00:00
rugk	168fed64b9	chore: apply Scruintizer diff	2025-09-03 14:11:35 +00:00
rugk	4f13d93af2	style: use explicit types	2025-09-03 13:53:51 +00:00
rugk	f76704a88c	refactor: simplify tests	2025-09-03 13:48:28 +00:00
rugk	dbaa70ec11	test: move ftp example to rejected because of foreign URL	2025-09-03 13:45:30 +00:00
rugk	879b696f22	wipfix: correct contatenation of options	2025-09-03 13:43:57 +00:00
rugk	0a398d73f0	chore(codespace): install stuff for PHP unit testing	2025-09-03 12:57:55 +00:00
rugk	fae7e233f3	test: write some tests for testing proxy ensurance	2025-09-03 12:38:44 +00:00
rugk	64165d9928	chore: always ignore composer PHP bin dir	2025-09-03 12:38:13 +00:00
rugk	bdfe74c077	chore: fix Codespace COmposer/PHPUnit PATH	2025-09-03 12:37:55 +00:00
rugk	2c1a17a07f	Strengthen validation of URL in proxy services This should definitively rule out any circumstances, where invalid URLs could cause problems. Both URL validity is checked before it is forwarded to the URL shortener proxy _and_ the host part is explicitly compared to make sure the domain is really the same one. TOOD: * [ ] some tests may be needed here (hmpff…)	2025-09-02 22:40:22 +02:00