Repository: shlinkio/shlink (language: php)

Issue:
Allow "&" and "+" characters in custom slugs again.
### Summary

Hi there!

I'm a big fan of shlink and have been using it for over a year now. It's been a great tool for me, super efficient and convenient.

I do have a small request though - I used to be able to use characters like "&" and "+" in my custom slugs, which was really handy for readability. It would be awesome if I could use them again.

I receive this error:
```
Provided data is not valid

Invalid elements: [customSlug]
```

Thanks so much for considering this!

### Use case

/T+B-lunch
/T&C_car


Discussion / hints:
> I used to be able to use characters like "&" and "+" in my custom slugs

In what version did you stop being able to use those? I don't remember making a conscious decision to "not allow" them, so it might be a side effect of some other change.

In any case, it might be a desirable side effect, as those are invalid URL characters that would need to be encoded, but with proper url-encoding, I don't see an immediate reason.

Also, can you elaborate what are you experiencing that you are interpreting as "not being able to use them"? Do you get an error? If so, which one? Have you checked the logs?
Closing until requested information is provided.
Thanks for your answer.

I am away From home for quite a while so I'm sorry I can't answer properly for the moment as I don't have access to the logs.

I can't remember what version it started not working.

As for the error message, it is mentioned in my original post.
> As for the error message, it is mentioned in my original post.

Thanks! I somehow missed that, perhaps I read the message in the email notification from GitHub and didn't realize it had been updated right after.
Ok, so I found when was this introduced. It was the result of this bug report https://github.com/shlinkio/shlink/issues/1901 (you can ignore everything after my first comment there, because the conversation deviated into a feature request not related to the bug itself).

To fix that, [this PR](https://github.com/shlinkio/shlink/pull/1911/files#diff-1590198407073217b1d39ab886681639464e3477b6d21e1d9988431884a75c47R49-R50) was provided, which validates custom slugs to disallow URL-reserved characters as per [this RFC](https://datatracker.ietf.org/doc/html/rfc3986#section-2.2).

I need to do a bit of investigation and think on potential side effects of changing this, or if that decision was wrong in the first place.
I'm really glad you're considering looking into this and I'm looking forward to reading you on this matter :)
I've been investigating this, and I think the right solution here which would bring back both `+` and `&` without breaking the original bugfix, is to be less restrictive about what characters are disallowed in custom slugs, allowing sub-delimiters while still disallowing gen-delimiters, which are the ones used to split parts of a URI (host from path, path from query string, credentials from authority, etc)

More info in the RFC https://datatracker.ietf.org/doc/html/rfc3986#section-2.2
I'm really happy you're considering re-implementing this again :)

Fix patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index fb43ec7ee..241b734ed 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -14,6 +14,10 @@ The format is based on [Keep a Changelog](https://keepachangelog.com), and this
 
     This option effectively replaces the old `REDIRECT_APPEND_EXTRA_PATH` option, which is now deprecated and will be removed in Shlink 5.0.0
 
+* [#2156](https://github.com/shlinkio/shlink/issues/2156) Be less restrictive on what characters are disallowed in custom slugs.
+
+    All [URI-reserved characters](https://datatracker.ietf.org/doc/html/rfc3986#section-2.2) were disallowed up until now, but from now on, only the gen-delimiters are.
+
 ### Changed
 * [#2281](https://github.com/shlinkio/shlink/issues/2281) Update docker image to PHP 8.4
 * [#2124](https://github.com/shlinkio/shlink/issues/2124) Improve how Shlink decides if a GeoLite db file needs to be downloaded, and reduces the chances for API limits to be reached.
diff --git a/module/Core/src/ShortUrl/Model/Validation/CustomSlugValidator.php b/module/Core/src/ShortUrl/Model/Validation/CustomSlugValidator.php
index f33416983..3d3e7792f 100644
--- a/module/Core/src/ShortUrl/Model/Validation/CustomSlugValidator.php
+++ b/module/Core/src/ShortUrl/Model/Validation/CustomSlugValidator.php
@@ -46,10 +46,10 @@ public function isValid(mixed $value): bool
             return false;
         }
 
-        // URL reserved characters: https://datatracker.ietf.org/doc/html/rfc3986#section-2.2
-        $reservedChars = "!*'();:@&=+$,?%#[]";
+        // URL gen-delimiter reserved characters, except `/`: https://datatracker.ietf.org/doc/html/rfc3986#section-2.2
+        $reservedChars = ':?#[]@';
         if (! $this->options->multiSegmentSlugsEnabled) {
-            // Slashes should be allowed for multi-segment slugs
+            // Slashes should only be allowed if multi-segment slugs are enabled
             $reservedChars .= '/';
         }
 


Test patch:
diff --git a/module/Core/test/ShortUrl/Model/Validation/CustomSlugValidatorTest.php b/module/Core/test/ShortUrl/Model/Validation/CustomSlugValidatorTest.php
index 86f695c7d..f763b44e2 100644
--- a/module/Core/test/ShortUrl/Model/Validation/CustomSlugValidatorTest.php
+++ b/module/Core/test/ShortUrl/Model/Validation/CustomSlugValidatorTest.php
@@ -59,13 +59,11 @@ public function valuesWithReservedCharsAreInvalid(string $value): void
 
     public static function provideInvalidValues(): iterable
     {
+        yield ['port:8080'];
         yield ['foo?bar=baz'];
         yield ['some-thing#foo'];
-        yield ['call()'];
-        yield ['array[]'];
+        yield ['brackets[]'];
         yield ['email@example.com'];
-        yield ['wildcard*'];
-        yield ['$500'];
     }
 
     public function createValidator(bool $multiSegmentSlugsEnabled = false): CustomSlugValidator