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

Hostname validation is a breaking change in 1.18.11 #354

Closed
kring opened this issue Aug 16, 2017 · 5 comments
Closed

Hostname validation is a breaking change in 1.18.11 #354

kring opened this issue Aug 16, 2017 · 5 comments

Comments

@kring
Copy link

kring commented Aug 16, 2017

We're using URI.js to, among other things, manipulate URLs that contain template placeholders to be replaced later. For example, http://{s}.example.com/{z}/{x}/{y}.png. In 1.18.11, this started failing because { is not a valid hostname character. While it's true that this is not a valid hostname character, I'd argue that it's a breaking change to start enforcing a strict whitelist of valid hostname characters (not even including all the characters allowed by RFC 3986) when previously the parsing was quite permissive. If this change cannot be reverted, perhaps it can be released as v2.0.0 instead?

@rodneyrehm
Copy link
Member

yeah, this change was handled rather poorly by me. sorry for that. can I offer the middle ground between revert and re-release? We could add an option (e.g. URI.allowInvalidHostname) to not run ensureValidHostname(). That would allow you to opt out.

That said, are you experiencing any problems with the templates themselves, or are you throwing a template at URI?

@konstantinblaesi
Copy link
Contributor

konstantinblaesi commented Aug 18, 2017

@kring Well ... I wonder why you aren't splitting your tasks into manipulating a templated string and validating that with URI.js as soon as you're done replacing all your placeholders? The thing is, URI.js did validation before that change, just not in all cases, which allowed these nonsensical characters. I agree the change breaks other code and that can be painful, but isn't the scope of URI.js to be a URL validation/manipulation library rather than a templating engine?

@kring
Copy link
Author

kring commented Aug 18, 2017

We could add an option (e.g. URI.allowInvalidHostname) to not run ensureValidHostname(). That would allow you to opt out.

@rodneyrehm that's fine by me! I'd still argue that should be a major version change, though, unless you're going to make the default consistent with the previous behavior.

I wonder why you aren't splitting your tasks into manipulating a templated string and validating that with URI.js as soon as you're done replacing all your placeholders?

@konstantinblaesi I am using URI.js to build up a templated URL which I am then passing to another library as a string. The library does the template substitution.

I agree the change breaks other code and that can be painful, but isn't the scope of URI.js to be a URL validation/manipulation library rather than a templating engine?

I'm not asking it to be a templating engine, only to avoid being overly opinionated about what constitutes a "valid" URL. In any case, if the maintainers of URI.js think this is the right direction for the library, I won't argue. But I would strongly encourage a semver major version bump, since the change definitely broke my code and likely has the potential to break others. This change was released as only a patch version bump, which is meant to only be for bug fixes and not new features or changes in behavior.

I guess I'm doing a lot of complaining here, so I just want to say: I've used URI.js in a lot of projects, and it's great! So thanks for all your work on it!

@ankon
Copy link
Contributor

ankon commented Aug 22, 2017

I have a similar issue: We use URI.js to "split" URL-ish things, in our case we allow placeholders (https://*.example.com). With 1.8.11 things started failing.

A compatibility/dont-be-strict switch would be nice, and a semver change would definitely be appreciated.

As @kring said though, we have also used URI.js in many projects, and found it awesome to work with!

@rodneyrehm
Copy link
Member

As the hostname validation caused some kerfuffle I've decided to make this feature opt-in, thereby returning to the previous default behavior of v1.18.10. As of v1.19.0 you can activate hostname validation globally by setting URI.preventInvalidHostname = true, or on an instance by calling .preventInvalidHostname(true).

I'm sorry for the confusion, inconvenience caused by the original change and the delayed rectification.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants