From cfc3d541229cb572c8fa8aee12dbf4c1bbc0fb9d Mon Sep 17 00:00:00 2001
From: Alejandro Celaya <alejandrocelaya@gmail.com>
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 aca4464e..8e42b1d0 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 32b40033..03091d75 100644
--- a/module/Core/src/Options/UrlShortenerOptions.php
+++ b/module/Core/src/Options/UrlShortenerOptions.php
@@ -10,8 +10,10 @@ use const Shlinkio\Shlink\DEFAULT_SHORT_CODES_LENGTH;
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 00000000..24afc72e
--- /dev/null
+++ b/module/Core/src/ShortUrl/Model/Validation/CustomSlugValidator.php
@@ -0,0 +1,63 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Shlinkio\Shlink\Core\ShortUrl\Model\Validation;
+
+use Laminas\Validator\AbstractValidator;
+use Shlinkio\Shlink\Core\Options\UrlShortenerOptions;
+
+use function is_string;
+use function strpbrk;
+
+class CustomSlugValidator extends AbstractValidator
+{
+ private const NOT_STRING = 'NOT_STRING';
+ private const CONTAINS_URL_CHARACTERS = 'CONTAINS_URL_CHARACTERS';
+
+ protected array $messageTemplates = [
+ self::NOT_STRING => '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 af7e8986..23ac8a2f 100644
--- a/module/Core/src/ShortUrl/Model/Validation/ShortUrlInputFilter.php
+++ b/module/Core/src/ShortUrl/Model/Validation/ShortUrlInputFilter.php
@@ -81,13 +81,15 @@ class ShortUrlInputFilter extends InputFilter
$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 401a3a31..47d4648c 100644
--- a/module/Core/test/ShortUrl/Model/ShortUrlCreationTest.php
+++ b/module/Core/test/ShortUrl/Model/ShortUrlCreationTest.php
@@ -62,6 +62,10 @@ class ShortUrlCreationTest extends TestCase
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 00000000..290fe63d
--- /dev/null
+++ b/module/Core/test/ShortUrl/Model/Validation/CustomSlugValidatorTest.php
@@ -0,0 +1,77 @@
+<?php
+
+declare(strict_types=1);
+
+namespace ShlinkioTest\Shlink\Core\ShortUrl\Model\Validation;
+
+use PHPUnit\Framework\Attributes\DataProvider;
+use PHPUnit\Framework\Attributes\Test;
+use PHPUnit\Framework\TestCase;
+use Shlinkio\Shlink\Core\Options\UrlShortenerOptions;
+use Shlinkio\Shlink\Core\ShortUrl\Model\Validation\CustomSlugValidator;
+use stdClass;
+
+class CustomSlugValidatorTest extends TestCase
+{
+ #[Test]
+ public function nullIsValid(): void
+ {
+ $validator = $this->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 <alejandrocelaya@gmail.com>
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 2510b208..d0f03e72 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