Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Query params replacing space (%20) with underscore (_) #2213

Closed
DAVe3283 opened this issue Oct 9, 2024 · 6 comments · Fixed by #2214
Closed

Query params replacing space (%20) with underscore (_) #2213

DAVe3283 opened this issue Oct 9, 2024 · 6 comments · Fixed by #2214
Labels
Milestone

Comments

@DAVe3283
Copy link

DAVe3283 commented Oct 9, 2024

Shlink version

4.2.1

PHP version

8.3.12

How do you serve Shlink

Docker image

Database engine

PostgreSQL

Database version

13.9

Current behavior

Spaces in query parameters are replaced by underscores.

For example, https://internal.fqdn/F4UVN?spaces are valid=true redirects to http://www.example.com/?spaces_are_valid=true
(note the underscores instead of spaces)

Expected behavior

Spaces need to be preserved in the query parameters.

For example, in Tableau, query parameters can be used to set filters on the linked dashboard, but most filters contain spaces in their names. When the spaces are changed to underscores, it breaks the filtering.

Minimum steps to reproduce

  1. Create a short URL to any domain (http://www.example.com is good since it doesn't do anything with query parameters).
  2. Ensure Forward query params on redirect is enabled.
  3. Go to the short URL and add some query parameters with spaces (e.g. ?Some Filter=1)
  4. Observe they have been replaced with underscores (e.g. ?Some_Filter=1)
@acelaya
Copy link
Member

acelaya commented Oct 9, 2024

Yeah, I can reproduce that. That's weird.

I'll check why this is happening.

@Fibonacci78
Copy link

Fibonacci78 commented Oct 9, 2024

Some additional information on this bug. A subset of percent encoded characters in parameters are being replaced with the non-percent encoded versions. This replacement does not happen if the percent encoded character is in the URL that is being shortened.

Here is a test sample:

  • %20 -> _
  • %22 -> "
  • %25 -> %25
  • %2D -> -
  • %2E -> _
  • %3C -> <
  • %3E -> >
  • %5C -> %5C
  • %5E -> %5E
  • %5F -> _
  • %60 -> %60
  • %7B -> %7B
  • %7C -> %7C

Space and period are placed with underscore, but others seem to be replaced with the correct equivalent character. I don't see any particulate logic to what is or is not being replaced.

@acelaya acelaya added this to the 4.2.2 milestone Oct 10, 2024
@acelaya acelaya moved this to Todo in Shlink Oct 10, 2024
@acelaya acelaya moved this from Todo to In Progress in Shlink Oct 10, 2024
@acelaya
Copy link
Member

acelaya commented Oct 10, 2024

So, I have been investigating this, and at first I didn't realize you were setting spaces in the parameter names. If you set them in the values, it works as expected.

The reason this is happening is because, for historical reasons, PHP ensures query parameter names are valid variable names, because they get promoted as variables. That implies spaces are converted to underscores (see the first example here https://www.php.net/manual/en/function.parse-str.php), but there are other conversions as you found.

And the reason the query string needs to be parsed is because Shlink needs to merge the query params to be forwarded, with any potential query param in the long URL.

So for example, if the short URL is https://s.test/abc and it has been configured to redirect to https://example.com?foo=bar&baz=something, when you visit https://s.test/abc?foo=overwritten&new=param, Shlink will redirect to https://example.com?foo=overwritten&baz=something&new=param.

Additionally, there are some special query params that are stripped, etc. So Shlink needs to parse the raw query strings, apply some logic, and serialize the query string back.

However, I have access to the raw query string as well, so I could try to apply some custom deserialization logic that prevents this behavior and preserves query param names verbatim.

@acelaya acelaya moved this from In Progress to In review in Shlink Oct 10, 2024
@github-project-automation github-project-automation bot moved this from In review to Done in Shlink Oct 10, 2024
@acelaya
Copy link
Member

acelaya commented Oct 10, 2024

This should now be fixed. I'll release v4.2.2 soon.

@acelaya
Copy link
Member

acelaya commented Oct 14, 2024

I have just released Shlink 4.2.2 which includes the fix for this.

@Fibonacci78
Copy link

This update is working as expected. Thank you!

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants