From cfc3d541229cb572c8fa8aee12dbf4c1bbc0fb9d Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sun, 5 Nov 2023 10:30:40 +0100 Subject: [PATCH 1/2] Do not allow URL reserved characters in custom slugs --- composer.json | 2 +- .../Core/src/Options/UrlShortenerOptions.php | 4 +- .../Model/Validation/CustomSlugValidator.php | 63 +++++++++++++++ .../Model/Validation/ShortUrlInputFilter.php | 12 +-- .../ShortUrl/Model/ShortUrlCreationTest.php | 4 + .../Validation/CustomSlugValidatorTest.php | 77 +++++++++++++++++++ 6 files changed, 155 insertions(+), 7 deletions(-) create mode 100644 module/Core/src/ShortUrl/Model/Validation/CustomSlugValidator.php create mode 100644 module/Core/test/ShortUrl/Model/Validation/CustomSlugValidatorTest.php diff --git a/composer.json b/composer.json index aca4464e7..8e42b1d02 100644 --- a/composer.json +++ b/composer.json @@ -77,7 +77,7 @@ "shlinkio/php-coding-standard": "~2.3.0", "shlinkio/shlink-test-utils": "dev-main#cbbb64e as 3.8.0", "symfony/var-dumper": "^6.3", - "veewee/composer-run-parallel": "^1.2" + "veewee/composer-run-parallel": "^1.3" }, "autoload": { "psr-4": { diff --git a/module/Core/src/Options/UrlShortenerOptions.php b/module/Core/src/Options/UrlShortenerOptions.php index 32b40033e..03091d75a 100644 --- a/module/Core/src/Options/UrlShortenerOptions.php +++ b/module/Core/src/Options/UrlShortenerOptions.php @@ -10,8 +10,10 @@ final class UrlShortenerOptions { + /** + * @param array{schema: ?string, hostname: ?string} $domain + */ public function __construct( - /** @var array{schema: ?string, hostname: ?string} */ public readonly array $domain = ['schema' => null, 'hostname' => null], public readonly int $defaultShortCodesLength = DEFAULT_SHORT_CODES_LENGTH, public readonly bool $autoResolveTitles = false, diff --git a/module/Core/src/ShortUrl/Model/Validation/CustomSlugValidator.php b/module/Core/src/ShortUrl/Model/Validation/CustomSlugValidator.php new file mode 100644 index 000000000..24afc72e4 --- /dev/null +++ b/module/Core/src/ShortUrl/Model/Validation/CustomSlugValidator.php @@ -0,0 +1,63 @@ + 'Provided value is not a string.', + self::CONTAINS_URL_CHARACTERS => 'URL-reserved characters cannot be used in a custom slug.', + ]; + + private UrlShortenerOptions $options; + + private function __construct() + { + parent::__construct(); + } + + public static function forUrlShortenerOptions(UrlShortenerOptions $options): self + { + $instance = new self(); + $instance->options = $options; + + return $instance; + } + + public function isValid(mixed $value): bool + { + if ($value === null) { + return true; + } + + if (! is_string($value)) { + $this->error(self::NOT_STRING); + return false; + } + + // URL reserved characters: https://datatracker.ietf.org/doc/html/rfc3986#section-2.2 + $reservedChars = "!*'();:@&=+$,?%#[]"; + if (! $this->options->multiSegmentSlugsEnabled) { + // Slashes should be allowed for multi-segment slugs + $reservedChars .= '/'; + } + + if (strpbrk($value, $reservedChars) !== false) { + $this->error(self::CONTAINS_URL_CHARACTERS); + return false; + } + + return true; + } +} diff --git a/module/Core/src/ShortUrl/Model/Validation/ShortUrlInputFilter.php b/module/Core/src/ShortUrl/Model/Validation/ShortUrlInputFilter.php index af7e8986a..23ac8a2fb 100644 --- a/module/Core/src/ShortUrl/Model/Validation/ShortUrlInputFilter.php +++ b/module/Core/src/ShortUrl/Model/Validation/ShortUrlInputFilter.php @@ -81,13 +81,15 @@ private function initialize(bool $requireLongUrl, UrlShortenerOptions $options): $this->add($validUntil); // The only way to enforce the NotEmpty validator to be evaluated when the key is present with an empty value - // is by using the deprecated setContinueIfEmpty + // is with setContinueIfEmpty $customSlug = $this->createInput(self::CUSTOM_SLUG, false)->setContinueIfEmpty(true); $customSlug->getFilterChain()->attach(new CustomSlugFilter($options)); - $customSlug->getValidatorChain()->attach(new Validator\NotEmpty([ - Validator\NotEmpty::STRING, - Validator\NotEmpty::SPACE, - ])); + $customSlug->getValidatorChain() + ->attach(new Validator\NotEmpty([ + Validator\NotEmpty::STRING, + Validator\NotEmpty::SPACE, + ])) + ->attach(CustomSlugValidator::forUrlShortenerOptions($options)); $this->add($customSlug); $this->add($this->createNumericInput(self::MAX_VISITS, false)); diff --git a/module/Core/test/ShortUrl/Model/ShortUrlCreationTest.php b/module/Core/test/ShortUrl/Model/ShortUrlCreationTest.php index 401a3a314..47d4648ce 100644 --- a/module/Core/test/ShortUrl/Model/ShortUrlCreationTest.php +++ b/module/Core/test/ShortUrl/Model/ShortUrlCreationTest.php @@ -62,6 +62,10 @@ public static function provideInvalidData(): iterable ShortUrlInputFilter::LONG_URL => 'https://foo', ShortUrlInputFilter::CUSTOM_SLUG => '', ]]; + yield [[ + ShortUrlInputFilter::LONG_URL => 'https://foo', + ShortUrlInputFilter::CUSTOM_SLUG => 'foo?some=param', + ]]; yield [[ ShortUrlInputFilter::LONG_URL => 'https://foo', ShortUrlInputFilter::CUSTOM_SLUG => ' ', diff --git a/module/Core/test/ShortUrl/Model/Validation/CustomSlugValidatorTest.php b/module/Core/test/ShortUrl/Model/Validation/CustomSlugValidatorTest.php new file mode 100644 index 000000000..290fe63d1 --- /dev/null +++ b/module/Core/test/ShortUrl/Model/Validation/CustomSlugValidatorTest.php @@ -0,0 +1,77 @@ +createValidator(); + self::assertTrue($validator->isValid(null)); + } + + #[Test, DataProvider('provideNonStringValues')] + public function nonStringValuesAreInvalid(mixed $value): void + { + $validator = $this->createValidator(); + + self::assertFalse($validator->isValid($value)); + self::assertEquals(['NOT_STRING' => 'Provided value is not a string.'], $validator->getMessages()); + } + + public static function provideNonStringValues(): iterable + { + yield [123]; + yield [new stdClass()]; + yield [true]; + } + + #[Test] + public function slashesAreAllowedWhenMultiSegmentSlugsAreEnabled(): void + { + $slugWithSlashes = 'foo/bar/baz'; + + self::assertTrue($this->createValidator(multiSegmentSlugsEnabled: true)->isValid($slugWithSlashes)); + self::assertFalse($this->createValidator(multiSegmentSlugsEnabled: false)->isValid($slugWithSlashes)); + } + + #[Test, DataProvider('provideInvalidValues')] + public function valuesWithReservedCharsAreInvalid(string $value): void + { + $validator = $this->createValidator(); + + self::assertFalse($validator->isValid($value)); + self::assertEquals( + ['CONTAINS_URL_CHARACTERS' => 'URL-reserved characters cannot be used in a custom slug.'], + $validator->getMessages(), + ); + } + + public static function provideInvalidValues(): iterable + { + yield ['foo?bar=baz']; + yield ['some-thing#foo']; + yield ['call()']; + yield ['array[]']; + yield ['email@example.com']; + yield ['wildcard*']; + yield ['$500']; + } + + public function createValidator(bool $multiSegmentSlugsEnabled = false): CustomSlugValidator + { + return CustomSlugValidator::forUrlShortenerOptions( + new UrlShortenerOptions(multiSegmentSlugsEnabled: $multiSegmentSlugsEnabled), + ); + } +} From e431395a12dac4738b36787ada8ddc8d6d37b9f5 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sun, 5 Nov 2023 10:31:51 +0100 Subject: [PATCH 2/2] Update changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2510b208b..d0f03e727 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -36,6 +36,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com), and this ### Fixed * [#1819](https://github.com/shlinkio/shlink/issues/1819) Fix incorrect timeout when running DB commands during Shlink start-up. +* [#1901](https://github.com/shlinkio/shlink/issues/1901) Do not allow short URLs with custom slugs containing URL-reserved characters, as they will not work at all afterward. ## [3.6.4] - 2023-09-23