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

Adds missing hostname to ssl handle #213

Merged
merged 4 commits into from
Jun 15, 2017
Merged

Adds missing hostname to ssl handle #213

merged 4 commits into from
Jun 15, 2017

Conversation

SinisterRectus
Copy link
Member

Copy link
Member

@creationix creationix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great. My only suggestion is to have .isServer for the boolean flag instead of just .server. It confused me for a bit thinking it held some table or uv struct.

@SinisterRectus
Copy link
Member Author

It was originally server, but it makes more sense as isServer. I made the change. As for setting the hostname, I'm assuming that it's been done right. Nothing broke on my end, so that's a plus.

@SinisterRectus
Copy link
Member Author

I implemented this fix thinking that I was not touching any part of the public API; however, it seems that this PR would make secure-socket incompatible with older versions of coro-net. In other words, you would need to update to the latest coro-net to use the latest secure-socket. We'll have to carefully select version numbers this time, or I can adjust the fix to something non-breaking; something more like that which is proposed in #212.

@creationix
Copy link
Member

We should probably just bump the major version on coro-net and secure-socket to be safe.

Looks good!

@creationix
Copy link
Member

I'm also fine with the suggestion in #212, it's a less invasive change and should work as well right?

@SinisterRectus
Copy link
Member Author

As discussed in IRC, I've restored backwards compatibility. Note that servername should technically be hostname, not host. Luckily, coro-http passes hostname to coro-net as host. This would only be a problem if anyone is using coro-net directly, and if they pass it an options table where host is not the same as hostname. We could add a fix for this with ssl:set('hostname', servername:match("^([^:/]+)")), but it seems like an edge case.

@creationix creationix merged commit 73be212 into luvit:master Jun 15, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants