Achieve 100% test coverage

Author: thecodeboss

coveralls.json

@@ -2,6 +2,7 @@
     "skip_files": [
         "lib/minecraft/packet/client/handshake.ex",
         "lib/minecraft/packet/client/status.ex",
-        "lib/minecraft/packet/server/status.ex"
+        "lib/minecraft/packet/server/status.ex",
+        "test/support"
     ]
 }

lib/minecraft/packet.ex

@@ -15,7 +15,7 @@ defmodule Minecraft.Packet do
   Given a raw binary packet, deserializes it into a `Packet` struct.
   """
   @spec deserialize(binary, state :: atom, type :: :client | :server) ::
-          {packet :: term, new_state :: atom, rest :: binary}
+          {packet :: term, new_state :: atom, rest :: binary} | {:error, :invalid_packet}
   def deserialize(data, state, type \\ :client)
 
   def deserialize(data, :handshaking, type) when is_binary(data) do

lib/minecraft/packet/handshake.ex

@@ -5,8 +5,6 @@ defmodule Minecraft.Packet.Handshake do
   alias Minecraft.Packet.Client
   alias Minecraft.Protocol
   import Minecraft.Packet
-  @protocol_1_12_2 340
-  @protocol_1_12_2_v Minecraft.Packet.encode_varint(@protocol_1_12_2)
 
   @type packet_id :: 0
 
@@ -15,9 +13,9 @@ defmodule Minecraft.Packet.Handshake do
   """
   @spec deserialize(packet_id, binary, type :: :client | :server) ::
           {packet :: term, new_state :: Protocol.state(), rest :: binary}
-  def deserialize(packet_id, data, type \\ :client)
-
-  def deserialize(0, <<@protocol_1_12_2_v::binary, rest::binary>>, :client) do
+          | {:error, :invalid_packet}
+  def deserialize(0 = _packet_id, data, :client = _type) do
+    {protocol_version, rest} = decode_varint(data)
     {server_addr, rest} = decode_string(rest)
     <<server_port::size(16), next_state::size(8), rest::binary>> = rest
 
@@ -28,6 +26,7 @@ defmodule Minecraft.Packet.Handshake do
       end
 
     packet = %Client.Handshake{
+      protocol_version: protocol_version,
       server_addr: server_addr,
       server_port: server_port,
       next_state: next_state
@@ -36,6 +35,10 @@ defmodule Minecraft.Packet.Handshake do
     {packet, next_state, rest}
   end
 
+  def deserialize(_, _, _) do
+    {:error, :invalid_packet}
+  end
+
   @doc """
   Serializes a handshake packet.
   """

lib/minecraft/packet/status.ex

@@ -14,10 +14,9 @@ defmodule Minecraft.Packet.Status do
   """
   @spec deserialize(packet_id, binary, type :: :client | :server) ::
           {packet :: term, new_state :: Protocol.state(), rest :: binary}
-  def deserialize(packet_id, data, type \\ :client)
-
-  def deserialize(0 = _packet_id, rest, :client) do
-    {%Client.Status.Request{}, :status, rest}
+          | {:error, :invalid_packet}
+  def deserialize(0 = _packet_id, data, :client = _type) do
+    {%Client.Status.Request{}, :status, data}
   end
 
   def deserialize(1 = _packet_id, data, :client) do
@@ -35,6 +34,10 @@ defmodule Minecraft.Packet.Status do
     {%Server.Status.Pong{payload: payload}, :status, rest}
   end
 
+  def deserialize(_, _, _) do
+    {:error, :invalid_packet}
+  end
+
   @doc """
   Serializes a status packet.
   """

lib/minecraft/protocol.ex

@@ -26,6 +26,10 @@ defmodule Minecraft.Protocol do
     {:ok, pid}
   end
 
+  #
+  # Server Callbacks
+  #
+
   @impl true
   def init({ref, socket, transport, _protocol_opts}) do
     :ok = :ranch.accept_ack(ref)
@@ -46,13 +50,33 @@ defmodule Minecraft.Protocol do
 
   @impl true
   def handle_info({:tcp, socket, packet}, state) do
-    {packet, current, rest} = Packet.deserialize(packet, state.current)
-    Logger.debug(fn -> "REQUEST: #{inspect(packet)}" end)
+    case Packet.deserialize(packet, state.current) do
+      {packet, current, rest} when is_binary(rest) ->
+        Logger.debug(fn -> "REQUEST: #{inspect(packet)}" end)
+
+        if byte_size(rest) > 0 do
+          send(self(), {:tcp, socket, rest})
+        end
+
+        handle_packet(packet, socket, current, state)
 
-    if byte_size(rest) > 0 do
-      send(self(), {:tcp, socket, rest})
+      {:error, :invalid_packet} ->
+        Logger.error(fn -> "Received an invalid packet from client, closing connection." end)
+        {:stop, :normal, state}
     end
+  end
+
+  def handle_info({:tcp_closed, socket}, state) do
+    Logger.info(fn -> "Client #{state.client_ip} disconnected." end)
+    :ok = state.transport.close(socket)
+    {:stop, :normal, state}
+  end
+
+  #
+  # Helpers
+  #
 
+  defp handle_packet(packet, socket, current, state) do
     case Handler.handle(packet) do
       {:ok, :noreply} ->
         :ok = state.transport.setopts(socket, active: :once)
@@ -71,10 +95,4 @@ defmodule Minecraft.Protocol do
         {:stop, :normal, state}
     end
   end
-
-  def handle_info({:tcp_closed, socket}, state) do
-    Logger.info(fn -> "Client #{state.client_ip} disconnected." end)
-    :ok = state.transport.close(socket)
-    {:stop, :normal, state}
-  end
 end

lib/minecraft/protocol/handler.ex

@@ -11,10 +11,14 @@ defmodule Minecraft.Protocol.Handler do
   @spec handle(packet :: struct) :: {:ok, :noreply | struct} | {:error, :unknown_packet}
   def handle(packet)
 
-  def handle(%Client.Handshake{}) do
+  def handle(%Client.Handshake{protocol_version: 340}) do
     {:ok, :noreply}
   end
 
+  def handle(%Client.Handshake{protocol_version: _}) do
+    {:error, :unsupported_protocol}
+  end
+
   def handle(%Client.Status.Request{}) do
     {:ok, json} =
       Poison.encode(%{

test/minecraft/integration/handshake_test.exs

@@ -26,4 +26,19 @@ defmodule Minecraft.HandshakeTest do
     assert {:ok, %Server.Status.Pong{payload: 12_345_678}} =
              TestClient.send(client, %Client.Status.Ping{payload: 12_345_678})
   end
+
+  test "invalid protocol", %{client: client} do
+    packet = %Client.Handshake{
+      protocol_version: 123,
+      server_addr: "localhost",
+      server_port: 25565,
+      next_state: :status
+    }
+
+    assert {:error, :closed} = TestClient.send(client, packet)
+  end
+
+  test "invalid packet results in socket closure", %{client: client} do
+    assert {:error, :closed} = TestClient.send_raw(client, <<1, 2, 3>>)
+  end
 end

test/support/test_client.ex

@@ -17,13 +17,21 @@ defmodule Minecraft.TestClient do
   end
 
   @doc """
-  Sends a message to the server.
+  Sends a packet to the server.
   """
-  @spec send(pid, struct) :: {:ok, response :: term} | {:error, term}
+  @spec send(pid, packet :: struct) :: {:ok, response :: term} | {:error, term}
   def send(pid, packet) do
     GenServer.call(pid, {:send, packet})
   end
 
+  @doc """
+  Sends raw data to the server.
+  """
+  @spec send_raw(pid, data :: binary) :: {:ok, response :: term} | {:error, term}
+  def send_raw(pid, data) do
+    GenServer.call(pid, {:send_raw, data})
+  end
+
   @doc """
   Sets the client's connection state.
   """
@@ -52,9 +60,29 @@ defmodule Minecraft.TestClient do
   def handle_call({:send, packet}, _from, {socket, state}) do
     {:ok, request} = Minecraft.Packet.serialize(packet)
     :ok = :gen_tcp.send(socket, request)
-    {:ok, response} = :gen_tcp.recv(socket, 0)
-    {response_packet, _, ""} = Minecraft.Packet.deserialize(response, state, :server)
-    {:reply, {:ok, response_packet}, {socket, state}}
+
+    case :gen_tcp.recv(socket, 0) do
+      {:ok, response} ->
+        {response_packet, _, ""} = Minecraft.Packet.deserialize(response, state, :server)
+        {:reply, {:ok, response_packet}, {socket, state}}
+
+      {:error, _} = err ->
+        {:stop, :normal, err, {socket, state}}
+    end
+  end
+
+  @impl true
+  def handle_call({:send_raw, data}, _from, {socket, state}) do
+    :ok = :gen_tcp.send(socket, data)
+
+    case :gen_tcp.recv(socket, 0) do
+      {:ok, response} ->
+        {response_packet, _, ""} = Minecraft.Packet.deserialize(response, state, :server)
+        {:reply, {:ok, response_packet}, {socket, state}}
+
+      {:error, _} = err ->
+        {:stop, :normal, err, {socket, state}}
+    end
   end
 
   def handle_call({:set_state, state}, _from, {socket, _old_state}) do