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

URI.withinString finds invalid hostnames #352

Closed
xPaw opened this issue Aug 13, 2017 · 8 comments
Closed

URI.withinString finds invalid hostnames #352

xPaw opened this issue Aug 13, 2017 · 8 comments

Comments

@xPaw
Copy link

@xPaw xPaw commented Aug 13, 2017

This appears to be a new issue in one of the latest versions.

We have code like this:

	URI.withinString(text, function(url, start, end) {
		// Extract the scheme of the URL detected, if there is one
		const parsedScheme = URI(url).scheme().toLowerCase();
	});

and now it throws Hostname "-oProxyCommand=whois" contains characters other than [A-Z0-9.-:_] on URI(url) line, which did not happen before.

@rodneyrehm
Copy link
Member

@rodneyrehm rodneyrehm commented Aug 13, 2017

Hostnames are validated since v1.18.11. What is the actual input you're dealing with?

@xPaw
Copy link
Author

@xPaw xPaw commented Aug 13, 2017

That would be a string like you mean ssh://-oProxyCommand=whois. I would expect withinString to not find it if URI is going to throw on same url.

@rodneyrehm
Copy link
Member

@rodneyrehm rodneyrehm commented Aug 14, 2017

So you'd like to parse the matched string ("slice") before handing it to the callback? Do you want to send a PR for that?

@xPaw
Copy link
Author

@xPaw xPaw commented Aug 14, 2017

We are already parsing the matched string to fix some url matching with known schemas, and the recent validation change basically breaks this. I am not that interested in figuring out how I would validate it to send a PR as I don't see a tryParse mechanism somewhere. Easiest fix I can do is just wrap URI(url) in a try/catch in our code, but that seems to defeat the purposes of using withinString in the first place.

@rodneyrehm
Copy link
Member

@rodneyrehm rodneyrehm commented Aug 14, 2017

We are already parsing the matched string to fix some url matching with known schemas

which are?

Easiest fix I can do is just wrap URI(url) in a try/catch in our code, but that seems to defeat the purposes of using withinString in the first place.

a) that's basically what I would have done
b) not sure it does since the callback is not invoked for obviously invalid input

@xPaw
Copy link
Author

@xPaw xPaw commented Aug 14, 2017

which are?

https://github.com/thelounge/lounge/blob/28e32dc5585d2401dc8431f6069a413177acb21d/client/js/libs/handlebars/ircmessageparser/findLinks.js#L25

not sure it does since the callback is not invoked for obviously invalid input

except it does and that's the entire problem here. It's finding an url with invalid hostname which fails in URI function.

@rodneyrehm
Copy link
Member

@rodneyrehm rodneyrehm commented Oct 1, 2017

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.

@astorije
Copy link

@astorije astorije commented Oct 1, 2017

Thank you @rodneyrehm! ❤️

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

3 participants