-
Notifications
You must be signed in to change notification settings - Fork 789
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
Add validation for server opening handshake response in Ember WebSocket client #7138
Add validation for server opening handshake response in Ember WebSocket client #7138
Conversation
Thanks! I retargeted to a new |
/** Validate the opening handshake response from the server | ||
* https://datatracker.ietf.org/doc/html/rfc6455#page-6 | ||
*/ | ||
def validateServerHandshake[F[_]]( | ||
response: Response[F], | ||
secWebSocketKey: String, | ||
)(implicit F: MonadThrow[F]): F[Boolean] = |
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 looks great! Can we add a WebSocketHelpersSuite
that validates a successful response, and also one response for each of the different failure kinds?
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.
I think this all looks great! Just a couple optimization ideas.
|
||
private[client] object WebSocketHelpers { | ||
|
||
private[internal] val supportedWebSocketVersion = 13L |
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.
Adding final
makes it a constant that will be inlined, because the right-hand-side is known at compile-time.
private[internal] val supportedWebSocketVersion = 13L | |
private[internal] final val supportedWebSocketVersion = 13L |
private def serverHandshake[F[_]](res: Response[F]): Either[ServerHandshakeError, ByteVector] = { | ||
val status = res.status match { | ||
case Status.SwitchingProtocols => Either.unit | ||
case _ => Left(InvalidStatus) | ||
} | ||
|
||
val connection = res.headers.get[Connection] match { | ||
case Some(header) if header.hasUpgrade => Either.unit | ||
case _ => Left(UpgradeRequired) | ||
} | ||
|
||
val upgrade = res.headers.get[Upgrade] match { | ||
case Some(header) if header.values.contains_(webSocketProtocol) => Either.unit | ||
case _ => Left(UpgradeRequired) | ||
} | ||
|
||
val secWebSocketAcceptKey = res.headers.get[`Sec-WebSocket-Accept`] match { | ||
case Some(header) => Right(header.hashedKey) | ||
case None => Left(SecWebSocketAcceptNotFound) | ||
} | ||
|
||
(status, connection, upgrade, secWebSocketAcceptKey).mapN { | ||
case (_, _, _, secWebSocketAcceptKey) => | ||
secWebSocketAcceptKey | ||
} |
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 way each check evaluates lazily and will short-circuit immediately if there is an error.
private def serverHandshake[F[_]](res: Response[F]): Either[ServerHandshakeError, ByteVector] = { | |
val status = res.status match { | |
case Status.SwitchingProtocols => Either.unit | |
case _ => Left(InvalidStatus) | |
} | |
val connection = res.headers.get[Connection] match { | |
case Some(header) if header.hasUpgrade => Either.unit | |
case _ => Left(UpgradeRequired) | |
} | |
val upgrade = res.headers.get[Upgrade] match { | |
case Some(header) if header.values.contains_(webSocketProtocol) => Either.unit | |
case _ => Left(UpgradeRequired) | |
} | |
val secWebSocketAcceptKey = res.headers.get[`Sec-WebSocket-Accept`] match { | |
case Some(header) => Right(header.hashedKey) | |
case None => Left(SecWebSocketAcceptNotFound) | |
} | |
(status, connection, upgrade, secWebSocketAcceptKey).mapN { | |
case (_, _, _, secWebSocketAcceptKey) => | |
secWebSocketAcceptKey | |
} | |
private def serverHandshake[F[_]](res: Response[F]): Either[ServerHandshakeError, ByteVector] = { | |
def status = res.status match { | |
case Status.SwitchingProtocols => Either.unit | |
case _ => Left(InvalidStatus) | |
} | |
def connection = res.headers.get[Connection] match { | |
case Some(header) if header.hasUpgrade => Either.unit | |
case _ => Left(UpgradeRequired) | |
} | |
def upgrade = res.headers.get[Upgrade] match { | |
case Some(header) if header.values.contains_(webSocketProtocol) => Either.unit | |
case _ => Left(UpgradeRequired) | |
} | |
def secWebSocketAcceptKey = res.headers.get[`Sec-WebSocket-Accept`] match { | |
case Some(header) => Right(header.hashedKey) | |
case None => Left(SecWebSocketAcceptNotFound) | |
} | |
status >> connection >> upgrade >> secWebSocketAcceptKey |
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 is fantastic! Glad to see the effort you are putting in!
I'll merge, we can do optimizations later 😁 correctness first! |
This is part of the Ember WebSocket client implementation. This validates the opening handshake response from the server.
http://rfc-editor.org/rfc/rfc6455#page-6