Accept non-numeric ICE candidate foundations per RFC 8445 (#97)

RFC 8839 Section 5.1 defines foundation as 1*32ice-char where
ice-char = ALPHA / DIGIT / "+" / "/". Implementations like aiortc
generate hex string foundations which caused a crash in unmarshal/1.

- Add parse_foundation/1 that accepts both integer and string foundations
- Preserve parsed foundation from remote candidates instead of recomputing
- Fix else clause in unmarshal/1 to always return {:error, _} tuples

Closes #96

Author: PiotrWodecki

lib/ex_ice/candidate.ex

@@ -12,7 +12,7 @@ defmodule ExICE.Candidate do
           address: :inet.ip_address() | String.t(),
           base_address: :inet.ip_address() | nil,
           base_port: :inet.port_number() | nil,
-          foundation: integer(),
+          foundation: String.t(),
           port: :inet.port_number(),
           priority: integer(),
           transport: :udp | :tcp,
@@ -80,7 +80,7 @@ defmodule ExICE.Candidate do
   def unmarshal(string) do
     with [f_str, c_str, tr_str, pr_str, a_str, po_str, "typ", ty_str | rest] <-
            String.split(string, " "),
-         {foundation, ""} <- Integer.parse(f_str),
+         {:ok, foundation} <- parse_foundation(f_str),
          {_component_id, ""} <- Integer.parse(c_str),
          {:ok, transport} <- parse_transport(String.downcase(tr_str)),
          {priority, ""} <- Integer.parse(pr_str),
@@ -99,7 +99,8 @@ defmodule ExICE.Candidate do
       {:ok, new(type, config ++ extra_config)}
     else
       err when is_list(err) -> {:error, :invalid_candidate}
-      err -> err
+      {:error, _} = err -> err
+      _other -> {:error, :invalid_candidate}
     end
   end
 
@@ -123,7 +124,10 @@ defmodule ExICE.Candidate do
       address: address,
       base_address: config[:base_address],
       base_port: config[:base_port],
-      foundation: ExICE.Priv.Candidate.foundation(type, address, nil, transport),
+      foundation:
+        Keyword.get_lazy(config, :foundation, fn ->
+          ExICE.Priv.Candidate.foundation(type, address, nil, transport)
+        end),
       port: Keyword.fetch!(config, :port),
       priority: Keyword.fetch!(config, :priority),
       transport: transport,
@@ -141,6 +145,16 @@ defmodule ExICE.Candidate do
   defp tcp_type_to_string(nil), do: ""
   defp tcp_type_to_string(type), do: "tcptype #{type}"
 
+  # RFC 8839 Section 5.1: foundation = 1*32ice-char, ice-char = ALPHA / DIGIT / "+" / "/"
+  @foundation_pattern ~r/^[a-zA-Z0-9+\/]+$/
+  defp parse_foundation(str) when byte_size(str) in 1..32 do
+    if Regex.match?(@foundation_pattern, str),
+      do: {:ok, str},
+      else: {:error, :invalid_foundation}
+  end
+
+  defp parse_foundation(_str), do: {:error, :invalid_foundation}
+
   defp parse_transport("udp"), do: {:ok, :udp}
   defp parse_transport("tcp"), do: {:ok, :tcp}
   defp parse_transport(_other), do: {:error, :invalid_transport}

lib/ex_ice/priv/candidate.ex

@@ -13,7 +13,7 @@ defmodule ExICE.Priv.Candidate do
           base_port: :inet.port_number(),
           socket: :inet.socket(),
           priority: integer(),
-          foundation: integer(),
+          foundation: String.t(),
           transport: :udp | :tcp,
           tcp_type: tcp_type()
         ]
@@ -128,8 +128,8 @@ defmodule ExICE.Priv.Candidate do
   end
 
   @spec foundation(type(), :inet.ip_address() | String.t(), :inet.ip_address() | nil, atom()) ::
-          integer()
+          String.t()
   def foundation(type, ip, stun_turn_ip, transport) do
-    {type, ip, stun_turn_ip, transport} |> inspect() |> :erlang.crc32()
+    {type, ip, stun_turn_ip, transport} |> inspect() |> :erlang.crc32() |> Integer.to_string()
   end
 end

lib/ex_ice/priv/candidate_base.ex

@@ -7,7 +7,7 @@ defmodule ExICE.Priv.CandidateBase do
           address: :inet.ip_address() | String.t(),
           base_address: :inet.ip_address() | nil,
           base_port: :inet.port_number() | nil,
-          foundation: integer(),
+          foundation: String.t(),
           port: :inet.port_number(),
           priority: integer(),
           transport: :udp | :tcp,

test/candidate_test.exs

@@ -83,4 +83,76 @@ defmodule ExICE.CandidateTest do
     c = %Candidate{c | id: expected_c.id}
     assert c == expected_c
   end
+
+  test "unmarshal/1 with string foundation" do
+    m_c = "834be7808a5c955b681935b0c6ad99df 1 UDP 2130706431 192.168.1.74 55474 typ host"
+
+    assert {:ok, c} = Candidate.unmarshal(m_c)
+    assert c.foundation == "834be7808a5c955b681935b0c6ad99df"
+    assert c.address == {192, 168, 1, 74}
+    assert c.port == 55_474
+    assert c.priority == 2_130_706_431
+    assert c.type == :host
+    assert c.transport == :udp
+  end
+
+  test "unmarshal/1 preserves numeric foundation as string" do
+    m_c = "936255739 1 UDP 1234 192.168.1.1 12345 typ host"
+
+    assert {:ok, c} = Candidate.unmarshal(m_c)
+    assert c.foundation == "936255739"
+    assert is_binary(c.foundation)
+  end
+
+  test "unmarshal/1 rejects candidate with leading space (empty foundation)" do
+    m_c = " 1 UDP 1234 192.168.1.1 12345 typ host"
+    assert {:error, :invalid_foundation} = Candidate.unmarshal(m_c)
+  end
+
+  test "unmarshal/1 accepts foundation of exactly 32 characters" do
+    foundation = String.duplicate("a", 32)
+    m_c = "#{foundation} 1 UDP 1234 192.168.1.1 12345 typ host"
+    assert {:ok, c} = Candidate.unmarshal(m_c)
+    assert c.foundation == foundation
+  end
+
+  test "unmarshal/1 accepts single character foundation" do
+    m_c = "a 1 UDP 1234 192.168.1.1 12345 typ host"
+    assert {:ok, c} = Candidate.unmarshal(m_c)
+    assert c.foundation == "a"
+  end
+
+  test "unmarshal/1 rejects foundation longer than 32 characters" do
+    long_foundation = String.duplicate("a", 33)
+    m_c = "#{long_foundation} 1 UDP 1234 192.168.1.1 12345 typ host"
+    assert {:error, :invalid_foundation} = Candidate.unmarshal(m_c)
+  end
+
+  test "unmarshal/1 rejects foundation with invalid characters" do
+    m_c = "abc!@#def 1 UDP 1234 192.168.1.1 12345 typ host"
+    assert {:error, :invalid_foundation} = Candidate.unmarshal(m_c)
+  end
+
+  test "unmarshal/1 preserves foundation with leading zeros" do
+    m_c = "001 1 UDP 1234 192.168.1.1 12345 typ host"
+
+    assert {:ok, c} = Candidate.unmarshal(m_c)
+    assert c.foundation == "001"
+    assert Candidate.marshal(c) == m_c
+  end
+
+  test "unmarshal/1 preserves foundation with plus and slash" do
+    m_c = "abc+/def 1 UDP 1234 192.168.1.1 12345 typ host"
+
+    assert {:ok, c} = Candidate.unmarshal(m_c)
+    assert c.foundation == "abc+/def"
+    assert Candidate.marshal(c) == m_c
+  end
+
+  test "marshal/1 roundtrips with string foundation" do
+    m_c = "834be7808a5c955b681935b0c6ad99df 1 UDP 2130706431 192.168.1.74 55474 typ host"
+
+    assert {:ok, c} = Candidate.unmarshal(m_c)
+    assert Candidate.marshal(c) == m_c
+  end
 end