WebSocket server doesn't properly handle protocol mistakes.

igor-ramazanov commented 5 years ago

Unfortunately, I don't know how exactly to reproduce this error.

The problem is that when I ran a local WebSocket server it was closing all incoming connections and printing the next stack trace:

scala.MatchError: 7 (of class java.lang.Integer)
    at org.http4s.websocket.package$.makeFrame(package.scala:26)
    at org.http4s.websocket.FrameTranscoder.bufferToFrame(FrameTranscoder.scala:165)
    at org.http4s.server.blaze.WebSocketDecoder.bufferToMessage(WebSocketDecoder.scala:30)
    at org.http4s.blaze.pipeline.stages.ByteToObjectStage.$anonfun$readAndDecodeLoop$1(ByteToObjectStage.scala:85)
    at scala.concurrent.impl.CallbackRunnable.run(Promise.scala:60)
    at org.http4s.blaze.util.Execution$ThreadLocalTrampoline.run(Execution.scala:108)
    at org.http4s.blaze.util.Execution$ThreadLocalTrampoline.execute(Execution.scala:94)
    at org.http4s.blaze.util.Execution$$anon$2.execute(Execution.scala:53)
    at scala.concurrent.impl.CallbackRunnable.executeWithValue(Promise.scala:68)
    at scala.concurrent.impl.Promise$DefaultPromise.dispatchOrAddCallback(Promise.scala:312)
    at scala.concurrent.impl.Promise$DefaultPromise.onComplete(Promise.scala:303)
    at org.http4s.blaze.pipeline.stages.ByteToObjectStage.readAndDecodeLoop(ByteToObjectStage.scala:99)
    at org.http4s.blaze.pipeline.stages.ByteToObjectStage.startReadDecode(ByteToObjectStage.scala:72)
    at org.http4s.blaze.pipeline.stages.ByteToObjectStage.readRequest(ByteToObjectStage.scala:68)
    at org.http4s.blaze.pipeline.stages.ByteToObjectStage.readRequest$(ByteToObjectStage.scala:56)
    at org.http4s.server.blaze.WebSocketDecoder.readRequest(WebSocketDecoder.scala:8)
    at org.http4s.blaze.pipeline.Tail.channelRead(Stages.scala:88)
    at org.http4s.blaze.pipeline.Tail.channelRead$(Stages.scala:85)
    at org.http4s.server.blaze.WSFrameAggregator.channelRead(WSFrameAggregator.scala:16)
    at org.http4s.server.blaze.WSFrameAggregator.readRequest(WSFrameAggregator.scala:24)
    at org.http4s.blaze.pipeline.Tail.channelRead(Stages.scala:88)
    at org.http4s.blaze.pipeline.Tail.channelRead$(Stages.scala:85)
    at org.http4s.blazecore.websocket.Http4sWSStage.channelRead(Http4sWSStage.scala:18)
    at org.http4s.blazecore.websocket.Http4sWSStage.$anonfun$readFrameTrampoline$1(Http4sWSStage.scala:54)
    at org.http4s.blazecore.websocket.Http4sWSStage.$anonfun$readFrameTrampoline$1$adapted(Http4sWSStage.scala:53)
    at monix.eval.internal.TaskCreate$.$anonfun$async$1(TaskCreate.scala:124)
    at monix.eval.internal.TaskCreate$.$anonfun$async$1$adapted(TaskCreate.scala:121)
    at monix.eval.internal.TaskRestartCallback.start(TaskRestartCallback.scala:58)
    at monix.eval.internal.TaskRunLoop$.executeAsyncTask(TaskRunLoop.scala:564)
    at monix.eval.internal.TaskRunLoop$.startFull(TaskRunLoop.scala:115)
    at monix.eval.internal.TaskRunLoop$.$anonfun$restartAsync$1(TaskRunLoop.scala:192)
    at java.util.concurrent.ForkJoinTask$RunnableExecuteAction.exec(ForkJoinTask.java:1402)
    at java.util.concurrent.ForkJoinTask.doExec(ForkJoinTask.java:289)
    at java.util.concurrent.ForkJoinPool$WorkQueue.runTask(ForkJoinPool.java:1056)
    at java.util.concurrent.ForkJoinPool.runWorker(ForkJoinPool.java:1692)
    at java.util.concurrent.ForkJoinWorkerThread.run(ForkJoinWorkerThread.java:157)

Seems that it doesn't properly handle deviations from the protocol and throws an error in case if a received opcode doesn't match to the expected valid set of opcodes.

I've checked the WebSockets RFC and it says that a server must close a connection and return 400 in case of an handshake error - https://tools.ietf.org/html/rfc6455#section-4.2.1

Also, it says that opcodes from 3 to 7 are reserved for the future usage and I didn't find information on how they should be handled exactly. However, there is a sentence: If an unknown opcode is received, the receiving endpoint MUST _Fail the WebSocket Connection_., though not sure if it's applicable to them - https://tools.ietf.org/html/rfc6455#section-5.2

Probably at least server could handle such opcodes and print something less scary and more understandable for users.

igor-ramazanov commented 5 years ago

The problem was solved when I chose a different HTTP port, so it might be just something with my local machine; however, this issue is about inconvenient handling of protocol deviations.

rossabaker commented 5 years ago

I'm not a web socket expert, but I think we should do the following:

When we get one of these frames, send a close with code 1003.
When we get other ProtocolExceptions, send a close with code 1002.
This might be something we can do in a recoverWith in Http4sWSStage#handleRead.

Any volunteers?

http4s / blaze

WebSocket server doesn't properly handle protocol mistakes. #669