search

luvit / lit

Toolkit for developing, sharing, and running luvit/lua programs and libraries.
http://lit.luvit.io/
Apache License 2.0
245 stars 58 forks source link

Adds missing hostname to ssl handle #213

Closed SinisterRectus closed 7 years ago

SinisterRectus commented 7 years ago

luvit/lit#212 luvit/luvit#986 luvit/luvit#987

SinisterRectus commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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

Looks good!

creationix commented 7 years ago

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

SinisterRectus commented 7 years ago

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.