Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Adding URL reserved characters to custom slug creates a non-editable non-working shorturl #1901

Closed
matthiasharrer opened this issue Nov 2, 2023 · 8 comments · Fixed by #1911
Labels
Milestone

Comments

@matthiasharrer
Copy link

Shlink version

3.6.4

PHP version

The one included in the docker image

How do you serve Shlink

RoadRunner Docker image

Database engine

PostgreSQL

Database version

14.7

Current behavior

Using the app.shlink.io frontend I was able to create two short urls containing a ? in their slug.
shlink

When I try to edit them it shows the error Oops! We could not find requested route..
When visiting the short url by clicking it in app.shlink.io I am forwarded to the fallback base_url long url.

Expected behavior

I would have expected that either I can add short urls containing query params and that I will be forwarded depending on the query param.
Also I would expect the be able to edit / remove those entries then.

OR

that I cannot even create a short url containing a query.

How to reproduce

Create a new short url using a custom slug abcd?env=prod

@acelaya
Copy link
Member

acelaya commented Nov 2, 2023

that I cannot even create a short url containing a query.

This should be the case. Let me elaborate.

Since the question mark is a reserved character in URLs, it cannot be used as part of the path.

The custom slug is not designed to predefine a query string. Shlink will not treat it like that, because when that URL is parsed, everything after the question mark will not be treated as part of the slug, and Shlink won't be able to find it.

Those URLs probably don't work in any case after creation. You won't be able to edit, delete or "visit" them.

That said, I remember thinking about this problem some time ago. I considered URL-encoding custom slugs, in order to avoid this, but ended up discarding to avoid custom slugs that look too different from what was initially sent.

The only manipulation Shlink does is replace spaces with dashes.

What I guess should happen here is that an error should be returned on creation, if the slug contains invalid characters.

@matthiasharrer
Copy link
Author

Thanks for the quick response!

What I guess should happen here is that an error should be returned on creation, if the slug contains invalid characters.

I absolutely agree, that this would fix the bug (that's why I put it in one option of expected behavior).

But let me elaborate a bit more on why I was trying this in the first place (although this would be more of a feature request and not part of this bug report I guess):

I have some embedded devices, that will provide QR-Codes with links to tutorial videos about certain functions of the device. To be able to change the actual video URLs I am pointing the QR-Codes to a shlink instance and configuring the video URL there. So I am using it not so much for the shorter URLs but more for the indirection.

Now the devices can have different configurations like for example different language.
I saw that shlink offers device-specific redirects based on the User-Agent and I thought it would be super cool if I could have a similar thing based on a query param (or subpath). Ideally so that I can have a fallback / default long URL and customize it based on incoming query parameters.

But I didn't open it as a feature request yet because I am not sure if that's just my esoteric use case or something useful to others as well.

@acelaya
Copy link
Member

acelaya commented Nov 2, 2023

I think you might be overthinking this a bit.

If you are already trying to create different short urls, just create regular ones and forget about the query parameters.

Use abcd-env-test and abcd-env-prod and you'll get the same result.

@matthiasharrer
Copy link
Author

Yeah I'll probably be doing this .. the only missing thing or let's say a bit annoying thing is that I cannot have a default value (Like let's say I only have abcd-env currently which should always be served but in the future I might want to add a specific option for abcd-env-test).
I would have to create all the possibilities in the first place.

But as I said it's probably quite specific to my use case.

@acelaya
Copy link
Member

acelaya commented Nov 2, 2023

That's because you are still thinking in terms of query params.

In a url there's no such thing as default. There are valid URLs, and invalid ones. Invalid ones would result in a 404.

What you should do is create a URL per environment and use those.

@matthiasharrer
Copy link
Author

Yes I do understand that.

It's just if you consider the example of different languages:
I would maybe start out with one version which is english and no matter which of let's say 20 languages is configured on the device it would show the english version. One after another I might choose to add a translated version.

Now of course when I eventually have the 20 different translations I would need 20 different URLs .. but it's a bit annoying if I have to create them already if I just got the english version. The query params came to my mind even taking this further where I could just put all kinds of context into the query variables and later on decide which ones to use.

But I think all this comes from my use case of shlink being an indirection where the source (the embedded device) might be hard or slow to change instead of the typical use case of shortening the urls and tracking them for mainly marketing purposes (i suppose).

@acelaya
Copy link
Member

acelaya commented Nov 2, 2023

Ok, so I think you are actually mixing different use cases here.

When we are talking about information that cannot be known beforehand and can only be inferred at runtime, it makes sense to have dynamic redirections, where the same short URL can redirect to different places.

This includes device types, languages and things like that. You can't provide a URL specifically to Android users and one to iOS users. They will all discover the same short URL, and Shlink will be able to redirect to a different app store to each one of them.

But the use case you are describing is different (or at least what I'm understanding). Query parameters are known beforehand by definition, so you would effectively end up with different URLs, no matter if the difference is in the custom slug or the query params.

The URLs /abcd-env-prod and /abcd-env-test are as different from each other as /abcd?env=prod and /abcd?env=test.

If Shlink supported the feature you are describing, you would be able to create a URL, lets say /abcd, which is what you call the default URL.

This URL would redirect to some place. Later, you want to be able to edit it and tell Shlink "hey, if the env param is present with value test, redirect to this other place instead". And you would need to edit the URL in future with as many values you would want to support for the query param, making it behave as if no query param was provided for any unknown values.

But the thing is that is effectively the same as creating your /abcd URL at first, and in future creating as many new URLs as needed with a variation of the custom slug (/abcd-env-test, /abcd-env-staging, /abcd-env-you-name-it, etc). It would take the same effort to create new short URLs or to edit an existing one.

And any of your embedded devices which are pointing to the default URL would need to be "manipulated" to point to one of the variations, no matter if the variation is appending query params, or changing the URL itself.

Considerations of this approach:

  • You would not have combined visits stats in one single URL (which can be good or bad depending on your needs), but 1) as you mentioned, this doesn't seem to be important to your case, and 2) you would still be able to add the same tag to all the short URLs, and then check the visits stats for that tag.
  • You would not be able to create just /abcd and pre-configure your devices to consume /abcd?env=prod from day one, with the ability to change the "prod" URL later on.

The last point is perhaps the only thing that cannot be done with Shlink as is, but it's very niche. Feel free to open a feature request if this is actually wan't you were trying to do, and we can see how much attention it gets.

@matthiasharrer
Copy link
Author

Thanks a lot for the discussion and timely responses!

The last point is perhaps the only thing that cannot be done with Shlink as is, but it's very niche. Feel free to open a feature request if this is actually wan't you were trying to do, and we can see how much attention it gets.

I think this exactly what I was trying to do ..

I'll try to write it up as a feature request .. although I see that as described the approach has at least the one problem that it will be hard to determine which url to use if there are multiple competing urls.

@acelaya acelaya moved this to Todo in Shlink Nov 4, 2023
@acelaya acelaya added this to the 3.7.0 milestone Nov 4, 2023
@acelaya acelaya changed the title Adding questionmark to custom slug creates a non-editable non-working shorturl Adding URLreserved characters to custom slug creates a non-editable non-working shorturl Nov 4, 2023
@acelaya acelaya changed the title Adding URLreserved characters to custom slug creates a non-editable non-working shorturl Adding URL reserved characters to custom slug creates a non-editable non-working shorturl Nov 4, 2023
@acelaya acelaya moved this from Todo to In Progress in Shlink Nov 5, 2023
@acelaya acelaya moved this from In Progress to In review in Shlink Nov 5, 2023
@github-project-automation github-project-automation bot moved this from In review to Done in Shlink Nov 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants