From 879b696f220a2d562cbc7d695c7c821257fb697c Mon Sep 17 00:00:00 2001 From: rugk Date: Wed, 3 Sep 2025 13:43:57 +0000 Subject: [PATCH 01/11] wipfix: correct contatenation of options --- lib/Proxy/AbstractProxy.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/Proxy/AbstractProxy.php b/lib/Proxy/AbstractProxy.php index bcf0a188..ebf67fe7 100644 --- a/lib/Proxy/AbstractProxy.php +++ b/lib/Proxy/AbstractProxy.php @@ -49,11 +49,11 @@ abstract class AbstractProxy */ public function __construct(Configuration $conf, string $link) { - if (!filter_var($link, FILTER_VALIDATE_URL, FILTER_FLAG_PATH_REQUIRED & FILTER_FLAG_QUERY_REQUIRED)) { + 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) ) { From dbaa70ec11fd5cc0880a3a82d3fbb534715d6f0e Mon Sep 17 00:00:00 2001 From: rugk Date: Wed, 3 Sep 2025 13:45:30 +0000 Subject: [PATCH 02/11] test: move ftp example to rejected because of foreign URL --- tst/YourlsProxyTest.php | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/tst/YourlsProxyTest.php b/tst/YourlsProxyTest.php index ecffda6e..197b04c8 100644 --- a/tst/YourlsProxyTest.php +++ b/tst/YourlsProxyTest.php @@ -50,9 +50,9 @@ class YourlsProxyTest extends TestCase /** * @dataProvider providerInvalidUrl */ - public function testImvalidUrl($uri) + public function testImvalidUrl($url) { - $yourls = new YourlsProxy($this->_conf, $uri); + $yourls = new YourlsProxy($this->_conf, $url); $this->assertTrue($yourls->isError()); $this->assertEquals($yourls->getError(), 'Invalid URL given.'); } @@ -63,7 +63,6 @@ class YourlsProxyTest extends TestCase array(' '), array('foo'), array('https://'), - array('ftp://example.com/?n=np'), 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 @@ -82,13 +81,21 @@ class YourlsProxyTest extends TestCase $this->assertTrue($yourls->isError()); $this->assertEquals($yourls->getError(), 'Trying to shorten a URL that isn\'t pointing at our instance.'); } - - public function testForeignUrl() +/** + * @dataProvider providerForeignUrl + */ + public function testForeignUrl($url) { - $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(), 'Trying to shorten a URL that isn\'t pointing at our instance.'); } + public function providerForeignUrl() { + return array( + ['ftp://example.com/?n=np'], // wrong protocol + ['https://other.example.com/?foo#bar'] // wrong domain + ); + } public function testSneakyForeignUrl() { From f76704a88cc82a4b15f0690526d8c396f3f00944 Mon Sep 17 00:00:00 2001 From: rugk Date: Wed, 3 Sep 2025 13:48:28 +0000 Subject: [PATCH 03/11] refactor: simplify tests --- tst/YourlsProxyTest.php | 38 +++++++++++++++++--------------------- 1 file changed, 17 insertions(+), 21 deletions(-) diff --git a/tst/YourlsProxyTest.php b/tst/YourlsProxyTest.php index 197b04c8..1b29c9d0 100644 --- a/tst/YourlsProxyTest.php +++ b/tst/YourlsProxyTest.php @@ -58,15 +58,15 @@ class YourlsProxyTest extends TestCase } public function providerInvalidUrl() { - 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 - ); + return [ + [''], + [' '], + ['foo'], + ['https://'], + ['https://example.com'], // missing path and query parameter, + ['https://example.com/'], // missing query parameter + ['https://example.com?paste=something'] // missing path parameter + ]; } /** @@ -81,7 +81,8 @@ class YourlsProxyTest extends TestCase $this->assertTrue($yourls->isError()); $this->assertEquals($yourls->getError(), 'Trying to shorten a URL that isn\'t pointing at our instance.'); } -/** + + /** * @dataProvider providerForeignUrl */ public function testForeignUrl($url) @@ -90,18 +91,13 @@ class YourlsProxyTest extends TestCase $this->assertTrue($yourls->isError()); $this->assertEquals($yourls->getError(), 'Trying to shorten a URL that isn\'t pointing at our instance.'); } - public function providerForeignUrl() { - return array( - ['ftp://example.com/?n=np'], // wrong protocol - ['https://other.example.com/?foo#bar'] // wrong domain - ); - } - public function testSneakyForeignUrl() - { - $yourls = new YourlsProxy($this->_conf, 'https://other.example.com/?q=https://example.com/?foo#bar'); - $this->assertTrue($yourls->isError()); - $this->assertEquals($yourls->getError(), 'Trying to shorten a URL that isn\'t pointing at our instance.'); + public function providerForeignUrl() { + return [ + ['ftp://example.com/?n=np'], // wrong protocol + ['https://other.example.com/?foo#bar'], // wrong domain + ['https://other.example.com/?q=https://example.com/?foo#bar'] // domain included inside string + ]; } public function testYourlsError() From 4f13d93af2371eef7798cfe72a417cceec514d4b Mon Sep 17 00:00:00 2001 From: rugk Date: Wed, 3 Sep 2025 13:53:51 +0000 Subject: [PATCH 04/11] style: use explicit types --- tst/YourlsProxyTest.php | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/tst/YourlsProxyTest.php b/tst/YourlsProxyTest.php index 1b29c9d0..3ed503b1 100644 --- a/tst/YourlsProxyTest.php +++ b/tst/YourlsProxyTest.php @@ -50,14 +50,14 @@ class YourlsProxyTest extends TestCase /** * @dataProvider providerInvalidUrl */ - public function testImvalidUrl($url) + public function testImvalidUrl($url): void { $yourls = new YourlsProxy($this->_conf, $url); $this->assertTrue($yourls->isError()); $this->assertEquals($yourls->getError(), 'Invalid URL given.'); } - public function providerInvalidUrl() { + public function providerInvalidUrl(): array { return [ [''], [' '], @@ -75,7 +75,7 @@ class YourlsProxyTest extends TestCase * * @return void */ - public function testForeignUrlUsingUsernameTrick() + public function testForeignUrlUsingUsernameTrick(): void { $yourls = new YourlsProxy($this->_conf, 'https://example.com/@foreign.malicious.example?foo#bar'); $this->assertTrue($yourls->isError()); @@ -85,14 +85,14 @@ class YourlsProxyTest extends TestCase /** * @dataProvider providerForeignUrl */ - public function testForeignUrl($url) + 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() { + public function providerForeignUrl(): array { return [ ['ftp://example.com/?n=np'], // wrong protocol ['https://other.example.com/?foo#bar'], // wrong domain From 168fed64b9ee55f161214720c853c1781d67ace5 Mon Sep 17 00:00:00 2001 From: rugk Date: Wed, 3 Sep 2025 14:11:35 +0000 Subject: [PATCH 05/11] chore: apply Scruintizer diff --- tst/YourlsProxyTest.php | 34 ++++++++++++++++++---------------- 1 file changed, 18 insertions(+), 16 deletions(-) diff --git a/tst/YourlsProxyTest.php b/tst/YourlsProxyTest.php index 3ed503b1..60c360eb 100644 --- a/tst/YourlsProxyTest.php +++ b/tst/YourlsProxyTest.php @@ -57,16 +57,17 @@ class YourlsProxyTest extends TestCase $this->assertEquals($yourls->getError(), 'Invalid URL given.'); } - public function providerInvalidUrl(): array { - return [ - [''], - [' '], - ['foo'], - ['https://'], - ['https://example.com'], // missing path and query parameter, - ['https://example.com/'], // missing query parameter - ['https://example.com?paste=something'] // missing path parameter - ]; + 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 + ); } /** @@ -92,12 +93,13 @@ class YourlsProxyTest extends TestCase $this->assertEquals($yourls->getError(), 'Trying to shorten a URL that isn\'t pointing at our instance.'); } - public function providerForeignUrl(): array { - return [ - ['ftp://example.com/?n=np'], // wrong protocol - ['https://other.example.com/?foo#bar'], // wrong domain - ['https://other.example.com/?q=https://example.com/?foo#bar'] // domain included inside string - ]; + 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() From cfc687d62be257d5cae34ccd5d000319a8cfdf49 Mon Sep 17 00:00:00 2001 From: rugk Date: Wed, 3 Sep 2025 14:12:12 +0000 Subject: [PATCH 06/11] style: fix indentation --- lib/Proxy/AbstractProxy.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/Proxy/AbstractProxy.php b/lib/Proxy/AbstractProxy.php index ebf67fe7..e45f5377 100644 --- a/lib/Proxy/AbstractProxy.php +++ b/lib/Proxy/AbstractProxy.php @@ -56,7 +56,7 @@ abstract class AbstractProxy 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; } From 25dca0838ec8b5f6bb756837340709aad1ab78f6 Mon Sep 17 00:00:00 2001 From: rugk Date: Wed, 3 Sep 2025 14:14:08 +0000 Subject: [PATCH 07/11] style(codespaces): comment PHP unit testing setup for now --- .devcontainer/postCreateCommand.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.devcontainer/postCreateCommand.sh b/.devcontainer/postCreateCommand.sh index 309dc239..a59fbfbf 100755 --- a/.devcontainer/postCreateCommand.sh +++ b/.devcontainer/postCreateCommand.sh @@ -8,8 +8,8 @@ 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 +# 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 From e4f2383dd84d3290a1a2ad47ac2d16e73fec6d57 Mon Sep 17 00:00:00 2001 From: rugk Date: Wed, 3 Sep 2025 14:20:03 +0000 Subject: [PATCH 08/11] test: more test cases for testForeignUrlUsingUsernameTrick --- tst/YourlsProxyTest.php | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/tst/YourlsProxyTest.php b/tst/YourlsProxyTest.php index 60c360eb..42bc17fd 100644 --- a/tst/YourlsProxyTest.php +++ b/tst/YourlsProxyTest.php @@ -74,15 +74,24 @@ class YourlsProxyTest extends TestCase * 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} * - * @return void + * @dataProvider providerForeignUrlUsernameTrick */ - public function testForeignUrlUsingUsernameTrick(): void + public function testForeignUrlUsingUsernameTrick($url): void { - $yourls = new YourlsProxy($this->_conf, 'https://example.com/@foreign.malicious.example?foo#bar'); + $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 providerForeignUrlUsernameTrick(): array + { + return array( + array('https://example.com@foreign.malicious.example?foo#bar'), + array('https://example.com/@foreign.malicious.example?foo#bar'), + array('https://example.com/?@foreign.malicious.example?foo#bar') + ); + } + /** * @dataProvider providerForeignUrl */ From 616635c66cab6fbc3a1c5bf7de0cec7d0099f8ec Mon Sep 17 00:00:00 2001 From: rugk Date: Wed, 3 Sep 2025 14:21:00 +0000 Subject: [PATCH 09/11] style: scruintizer wants some trailing comma --- tst/YourlsProxyTest.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tst/YourlsProxyTest.php b/tst/YourlsProxyTest.php index 42bc17fd..5b0c5f30 100644 --- a/tst/YourlsProxyTest.php +++ b/tst/YourlsProxyTest.php @@ -88,7 +88,7 @@ class YourlsProxyTest extends TestCase return array( array('https://example.com@foreign.malicious.example?foo#bar'), array('https://example.com/@foreign.malicious.example?foo#bar'), - array('https://example.com/?@foreign.malicious.example?foo#bar') + array('https://example.com/?@foreign.malicious.example?foo#bar'), ); } From a6034ace1bf051c2b927fc5245a051c0afe2c6a8 Mon Sep 17 00:00:00 2001 From: rugk Date: Wed, 3 Sep 2025 14:25:04 +0000 Subject: [PATCH 10/11] test: PHP considers this invalid --- tst/YourlsProxyTest.php | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tst/YourlsProxyTest.php b/tst/YourlsProxyTest.php index 5b0c5f30..11c07a86 100644 --- a/tst/YourlsProxyTest.php +++ b/tst/YourlsProxyTest.php @@ -67,6 +67,7 @@ class YourlsProxyTest extends TestCase 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'), // shall belong to providerForeignUrlUsernameTrick, but for some reason PHP considers this an invalid URL ); } @@ -86,7 +87,7 @@ class YourlsProxyTest extends TestCase public function providerForeignUrlUsernameTrick(): array { return array( - array('https://example.com@foreign.malicious.example?foo#bar'), + // array('https://example.com@foreign.malicious.example?foo#bar'), array('https://example.com/@foreign.malicious.example?foo#bar'), array('https://example.com/?@foreign.malicious.example?foo#bar'), ); From bd61a3d0216fd16020f9f3675c07464463e2c385 Mon Sep 17 00:00:00 2001 From: El RIDO Date: Wed, 3 Sep 2025 20:13:33 +0200 Subject: [PATCH 11/11] 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. --- tst/YourlsProxyTest.php | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/tst/YourlsProxyTest.php b/tst/YourlsProxyTest.php index 11c07a86..a4a87bd9 100644 --- a/tst/YourlsProxyTest.php +++ b/tst/YourlsProxyTest.php @@ -45,6 +45,10 @@ 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'); } /** @@ -67,7 +71,7 @@ class YourlsProxyTest extends TestCase 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'), // shall belong to providerForeignUrlUsernameTrick, but for some reason PHP considers this an invalid URL + array('https://example.com@foreign.malicious.example?foo#bar'), // missing path parameter ); } @@ -87,9 +91,8 @@ class YourlsProxyTest extends TestCase public function providerForeignUrlUsernameTrick(): array { return array( - // array('https://example.com@foreign.malicious.example?foo#bar'), + array('https://example.com@foreign.malicious.example/?foo#bar'), array('https://example.com/@foreign.malicious.example?foo#bar'), - array('https://example.com/?@foreign.malicious.example?foo#bar'), ); }