Merge pull request #1911 from acelaya-forks/feature/slug-url-chars

Feature/slug url chars

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

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": {

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,

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;
+    }
+}

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));

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 => '   ',

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),
+        );
+    }
+}