RTC-506 Restrict user assigned room's id to alphanumericals (#171)

* Allow only alphanumeric characters in room_id * Add path_suffix to path_prefix * Update lib/jellyfish_web/controllers/room_controller.ex Co-authored-by: Jakub Pisarek <[email protected]> * Don't allow empty strings as room_id * Change deps * Update deps --------- Co-authored-by: Jakub Pisarek <[email protected]>
fishjam-dev · Apr 8, 2024 · ec13dd3 · ec13dd3
1 parent 87bb5a3
commit ec13dd3
Show file tree

Hide file tree

Showing 6 changed files with 98 additions and 5 deletions.
diff --git a/lib/jellyfish/component.ex b/lib/jellyfish/component.ex
@@ -123,7 +123,8 @@ defmodule Jellyfish.Component do
 
   @spec new(component(), map()) :: {:ok, t()} | {:error, term()}
   def new(type, options) do
-    with {:ok, %{endpoint: endpoint, properties: properties}} <- type.config(options) do
+    with {:ok, %{endpoint: endpoint, properties: properties}} <-
+           type.config(options) do
       {:ok,
        %__MODULE__{
          id: UUID.uuid4(),

diff --git a/lib/jellyfish/component/recording.ex b/lib/jellyfish/component/recording.ex
@@ -25,7 +25,12 @@ defmodule Jellyfish.Component.Recording do
     with {:ok, serialized_opts} <- serialize_options(options, Options.schema()),
          {:ok, credentials} <- get_credentials(serialized_opts, sink_config),
          {:ok, path_prefix} <- get_path_prefix(serialized_opts, sink_config) do
-      output_dir = get_base_path()
+      datetime = DateTime.utc_now() |> to_string()
+      path_suffix = Path.join(options.room_id, "part_#{datetime}")
+
+      path_prefix = Path.join(path_prefix, path_suffix)
+      output_dir = Path.join(get_base_path(), path_suffix)
+
       File.mkdir_p!(output_dir)
 
       file_storage = {Recording.Storage.File, %{output_dir: output_dir}}

diff --git a/lib/jellyfish/room/config.ex b/lib/jellyfish/room/config.ex
@@ -45,7 +45,15 @@ defmodule Jellyfish.Room.Config do
   end
 
   defp parse_room_id(nil), do: {:ok, UUID.uuid4()}
-  defp parse_room_id(room_id) when is_binary(room_id), do: {:ok, room_id}
+
+  defp parse_room_id(room_id) when is_binary(room_id) do
+    if Regex.match?(~r/^[a-zA-Z0-9-]+$/, room_id) do
+      {:ok, room_id}
+    else
+      {:error, :invalid_room_id}
+    end
+  end
+
   defp parse_room_id(_room_id), do: {:error, :invalid_room_id}
 
   defp validate_max_peers(nil), do: :ok

diff --git a/lib/jellyfish_web/controllers/room_controller.ex b/lib/jellyfish_web/controllers/room_controller.ex
@@ -104,6 +104,12 @@ defmodule JellyfishWeb.RoomController do
       {:error, :room_already_exists} ->
         room_id = Map.get(params, "roomId")
         {:error, :bad_request, "Cannot add room with id \"#{room_id}\" - room already exists"}
+
+      {:error, :invalid_room_id} ->
+        room_id = Map.get(params, "roomId")
+
+        {:error, :bad_request,
+         "Cannot add room with id \"#{room_id}\" - roomId may contain only alphanumeric characters and hyphens"}
     end
   end
 

diff --git a/test/jellyfish_web/controllers/component/recording_component_test.exs b/test/jellyfish_web/controllers/component/recording_component_test.exs
@@ -28,14 +28,18 @@ defmodule JellyfishWeb.Component.RecordingComponentTest do
           options: %{credentials: Enum.into(@s3_credentials, %{}), pathPrefix: @path_prefix}
         )
 
+      prefix = "#{@path_prefix}/#{room_id}/part_"
+
       assert %{
                "data" => %{
                  "id" => id,
                  "type" => "recording",
-                 "properties" => %{"pathPrefix" => @path_prefix}
+                 "properties" => %{"pathPrefix" => path_prefix}
                }
              } = model_response(conn, :created, "ComponentDetailsResponse")
 
+      assert String.starts_with?(path_prefix, prefix)
+
       assert_component_created(conn, room_id, id, "recording")
 
       # Try to add another recording component
@@ -60,19 +64,78 @@ defmodule JellyfishWeb.Component.RecordingComponentTest do
           options: %{pathPrefix: @path_prefix}
         )
 
+      prefix = "#{@path_prefix}/#{room_id}/part_"
+
       assert %{
                "data" => %{
                  "id" => id,
                  "type" => "recording",
-                 "properties" => %{"pathPrefix" => @path_prefix}
+                 "properties" => %{"pathPrefix" => path_prefix}
                }
              } = model_response(conn, :created, "ComponentDetailsResponse")
 
+      assert String.starts_with?(path_prefix, prefix)
+
       assert_component_created(conn, room_id, id, "recording")
 
       clean_s3_envs()
     end
 
+    test "path prefix modify when recording is created second time",
+         %{
+           conn: conn,
+           room_id: room_id
+         } do
+      mock_http_request()
+      put_s3_envs(path_prefix: nil, credentials: @s3_credentials)
+
+      conn =
+        post(conn, ~p"/room/#{room_id}/component",
+          type: "recording",
+          options: %{pathPrefix: @path_prefix}
+        )
+
+      prefix = "#{@path_prefix}/#{room_id}/part_"
+
+      assert %{
+               "data" => %{
+                 "id" => id,
+                 "type" => "recording",
+                 "properties" => %{"pathPrefix" => path_prefix1}
+               }
+             } = model_response(conn, :created, "ComponentDetailsResponse")
+
+      assert String.starts_with?(path_prefix1, prefix)
+
+      assert_component_created(conn, room_id, id, "recording")
+
+      conn = delete(conn, ~p"/room/#{room_id}/component/#{id}")
+      assert response(conn, :no_content)
+
+      # Second recording
+      conn =
+        post(conn, ~p"/room/#{room_id}/component",
+          type: "recording",
+          options: %{pathPrefix: @path_prefix}
+        )
+
+      assert %{
+               "data" => %{
+                 "id" => id,
+                 "type" => "recording",
+                 "properties" => %{"pathPrefix" => path_prefix2}
+               }
+             } = model_response(conn, :created, "ComponentDetailsResponse")
+
+      assert_component_created(conn, room_id, id, "recording")
+
+      assert String.starts_with?(path_prefix2, prefix)
+
+      assert path_prefix1 != path_prefix2
+
+      clean_s3_envs()
+    end
+
     test "renders component when path prefix is passed in config", %{
       conn: conn,
       room_id: room_id

diff --git a/test/jellyfish_web/controllers/room_controller_test.exs b/test/jellyfish_web/controllers/room_controller_test.exs
@@ -174,6 +174,16 @@ defmodule JellyfishWeb.RoomControllerTest do
 
       assert json_response(conn, :bad_request)["errors"] ==
                "Expected peerlessPurgeTimeout to be a positive integer, got: nan"
+
+      conn = post(conn, ~p"/room", roomId: "test/path")
+
+      assert json_response(conn, :bad_request)["errors"] ==
+               "Cannot add room with id \"test/path\" - roomId may contain only alphanumeric characters and hyphens"
+
+      conn = post(conn, ~p"/room", roomId: "")
+
+      assert json_response(conn, :bad_request)["errors"] ==
+               "Cannot add room with id \"\" - roomId may contain only alphanumeric characters and hyphens"
     end
   end