-
Notifications
You must be signed in to change notification settings - Fork 0
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
[ECO-4767] Fix server certificate verification #2
Conversation
62afd51
to
80b79cf
Compare
(This is based on top of #1, which forks the library for Ably. That one is still a WIP but I think that this PR can be reviewed.) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - great use of comments btw 👏
ee345b4
to
81e9f75
Compare
80b79cf
to
39dbd71
Compare
81e9f75
to
8e3ecd4
Compare
39dbd71
to
2862fbf
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
2862fbf
to
9996b7b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This LGTM. Verifying the certificate before the handshake is complete is the right thing to do.
See ably-forks/em-http-request#2. This fixes the internet-up check and hence the fallback hosts mechanism. Resolves #396.
For ably/ably-ruby#396. See code comment for details.