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

Throw error if long URLs are provided without schema #1715

Closed
codaamok opened this issue Mar 5, 2023 · 2 comments · Fixed by #1730
Closed

Throw error if long URLs are provided without schema #1715

codaamok opened this issue Mar 5, 2023 · 2 comments · Fixed by #1730
Labels
Milestone

Comments

@codaamok
Copy link

codaamok commented Mar 5, 2023

Summary

When creating short codes with the REST API, e.g. createShortUrl, long URLs without a schema are allowed and Shlink redirects them to the default domain where the longUrl is appended to the URI. Same is true for the new deviceLongUrl.[android|ios|desktop] request properties too.

For example, If the value bbc.co.uk is given for longUrl, Shlink redirects to http://<default_domain>/bbc.co.uk instead of http(s)://bbc.co.uk.

Example request:

curl -X 'POST' \
  'https://domain/rest/v3/short-urls' \
  -H 'accept: application/json' \
  -H 'X-Api-Key: x' \
  -H 'Content-Type: application/json' \
  -d '{
  "longUrl": "bbc.co.uk",
  "deviceLongUrls": {
    "android": "bbc.co.uk",
    "ios": "bbc.co.uk",
    "desktop": "bbc.co.uk"
  },
  "customSlug": "bbc"
}'

There are two solutions to this request:

  • Throw an error if longUrl is given without a schema. Shlink web UI has validation in the form so this implementation would match that behaviour
  • Assume the user means http:// in this situation and use this in the response

If it were me, I'd probably go with option 1 on the basis if you're creating short-url's via REST API then you should a) know what a schema is and why it's important to specify and b) can handle errors in code. Plus it'll follow the functionality of the web UI.

Although option 2 allows for an easier user experience.

@acelaya
Copy link
Member

acelaya commented Mar 5, 2023

This is a bug. Option one is how it should be working.

I even remember having to tweak this to support deeplinks, some years ago. Perhaps that was what broke this.

@acelaya acelaya added bug and removed feature labels Mar 5, 2023
@acelaya acelaya added this to the 3.5.3 milestone Mar 5, 2023
@acelaya acelaya added this to Shlink Mar 5, 2023
@acelaya acelaya moved this to Todo in Shlink Mar 5, 2023
@acelaya acelaya changed the title Assume http for schema if Long URLs are provided without schema Throw error if long URLs are provided without schema Mar 24, 2023
@acelaya acelaya moved this from Todo to In Progress in Shlink Mar 25, 2023
@acelaya
Copy link
Member

acelaya commented Mar 25, 2023

I have been re-considering this for a couple reasons.

  • First, I seemed to be relying on this on tests a lot, when building dummy data, which has forced me to refactor a lot of stuff 😅
  • Second, I realized that changing this behavior is technically speaking, a breaking change.

The first problem was caused by me. I have had to rewrite many tests, but such is life. It's done now.

Regarding the second problem, I have decided to still go with it because a) most of the people use shlink-web-client, which already validates URLs, and b) the resulting short URLs when the schema is not provided are completely broken, so I very much doubt anyone was actually relying on this behavior.

That said, since Shlink has to support different types of URIs (including deeplinks and such), I have decided to go with a simplistic approach, which is just validating that provided URL has a schema of any type. So values like foo or bbc.co.uk are no longer considered valid, but http://foo, https://foo, http://bbc.co.uk, ftp://foo or market://foo will be considered valid.

# 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.

2 participants