From 81b49f55bfcec2095c938ab981ef2230f42eaece Mon Sep 17 00:00:00 2001 From: Kieran Date: Sat, 6 Apr 2024 11:48:28 -0700 Subject: [PATCH] [Bugfix] Properly escape NFO files (#168) * Properly escaped NFO file contents * Added an NFO backfill worker * Added a try-catch to the backfill since I _really_ don't want failures to halt app boot --- lib/pinchflat/boot/nfo_backfill_worker.ex | 70 ++++++++++++++++++++ lib/pinchflat/boot/post_job_startup_tasks.ex | 6 +- lib/pinchflat/metadata/nfo_builder.ex | 20 +++--- lib/pinchflat/podcasts/rss_feed_builder.ex | 10 +-- lib/pinchflat/utils/xml_utils.ex | 17 +++++ test/pinchflat/metadata/nfo_builder_test.exs | 28 ++++++++ test/pinchflat/utils/xml_utils_test.exs | 16 +++++ 7 files changed, 149 insertions(+), 18 deletions(-) create mode 100644 lib/pinchflat/boot/nfo_backfill_worker.ex create mode 100644 lib/pinchflat/utils/xml_utils.ex create mode 100644 test/pinchflat/utils/xml_utils_test.exs diff --git a/lib/pinchflat/boot/nfo_backfill_worker.ex b/lib/pinchflat/boot/nfo_backfill_worker.ex new file mode 100644 index 0000000..917d2f1 --- /dev/null +++ b/lib/pinchflat/boot/nfo_backfill_worker.ex @@ -0,0 +1,70 @@ +defmodule Pinchflat.Boot.NfoBackfillWorker do + @moduledoc false + + use Oban.Worker, + queue: :local_metadata, + # This should have it running once _ever_ (until the job is pruned, anyway) + # NOTE: remove within the next month + unique: [period: :infinity, states: Oban.Job.states()], + tags: ["media_item", "media_metadata", "local_metadata", "data_backfill"] + + import Ecto.Query, warn: false + require Logger + + alias Pinchflat.Repo + alias Pinchflat.Media + alias Pinchflat.Media.MediaItem + alias Pinchflat.Metadata.NfoBuilder + alias Pinchflat.Metadata.MetadataFileHelpers + + @doc """ + Runs a one-off backfill job to regenerate NFO files for media items that have + both an NFO file and a metadata file. This is needed because NFO files weren't + escaping characters properly so we need to regenerate them. + + This job will only run once as long as I remove it before the jobs are pruned in a month. + + Returns :ok + """ + @impl Oban.Worker + def perform(%Oban.Job{}) do + Logger.info("Running NFO backfill worker") + + media_items = get_media_items_to_backfill() + + Enum.each(media_items, fn media_item -> + nfo_exists = File.exists?(media_item.nfo_filepath) + metadata_exists = File.exists?(media_item.metadata.metadata_filepath) + + if nfo_exists && metadata_exists do + Logger.info("NFO and metadata exist for media item #{media_item.id} - proceeding") + + regenerate_nfo_for_media_item(media_item) + end + end) + + :ok + end + + defp get_media_items_to_backfill do + from(m in MediaItem, where: not is_nil(m.nfo_filepath)) + |> Repo.all() + |> Repo.preload([:metadata, source: :media_profile]) + end + + defp regenerate_nfo_for_media_item(media_item) do + try do + case MetadataFileHelpers.read_compressed_metadata(media_item.metadata.metadata_filepath) do + {:ok, metadata} -> + Media.update_media_item(media_item, %{ + nfo_filepath: NfoBuilder.build_and_store_for_media_item(media_item.nfo_filepath, metadata) + }) + + _err -> + Logger.error("Failed to read metadata for media item #{media_item.id}") + end + rescue + e -> Logger.error("Unknown error regenerating NFO file for MI ##{media_item.id}: #{inspect(e)}") + end + end +end diff --git a/lib/pinchflat/boot/post_job_startup_tasks.ex b/lib/pinchflat/boot/post_job_startup_tasks.ex index 867c300..ab69da7 100644 --- a/lib/pinchflat/boot/post_job_startup_tasks.ex +++ b/lib/pinchflat/boot/post_job_startup_tasks.ex @@ -7,6 +7,9 @@ defmodule Pinchflat.Boot.PostJobStartupTasks do Phoenix supervision tree. """ + alias Pinchflat.Repo + alias Pinchflat.Boot.NfoBackfillWorker + # restart: :temporary means that this process will never be restarted (ie: will run once and then die) use GenServer, restart: :temporary import Ecto.Query, warn: false @@ -26,7 +29,8 @@ defmodule Pinchflat.Boot.PostJobStartupTasks do """ @impl true def init(state) do - # Empty for now, keeping because tasks _will_ be added in future + Repo.insert_unique_job(NfoBackfillWorker.new(%{})) + {:ok, state} end end diff --git a/lib/pinchflat/metadata/nfo_builder.ex b/lib/pinchflat/metadata/nfo_builder.ex index fb5ba7b..ad42331 100644 --- a/lib/pinchflat/metadata/nfo_builder.ex +++ b/lib/pinchflat/metadata/nfo_builder.ex @@ -4,6 +4,8 @@ defmodule Pinchflat.Metadata.NfoBuilder do use by Kodi/Jellyfin and other media center software. """ + import Pinchflat.Utils.XmlUtils, only: [safe: 1] + alias Pinchflat.Metadata.MetadataFileHelpers alias Pinchflat.Filesystem.FilesystemHelpers @@ -42,12 +44,12 @@ defmodule Pinchflat.Metadata.NfoBuilder do """ - #{metadata["title"]} - #{metadata["uploader"]} - #{metadata["id"]} - #{metadata["description"]} - #{upload_date} - #{upload_date.year} + #{safe(metadata["title"])} + #{safe(metadata["uploader"])} + #{safe(metadata["id"])} + #{safe(metadata["description"])} + #{safe(upload_date)} + #{safe(upload_date.year)} #{Calendar.strftime(upload_date, "%m%d")} YouTube @@ -58,9 +60,9 @@ defmodule Pinchflat.Metadata.NfoBuilder do """ - #{metadata["title"]} - #{metadata["description"]} - #{metadata["id"]} + #{safe(metadata["title"])} + #{safe(metadata["description"])} + #{safe(metadata["id"])} YouTube """ diff --git a/lib/pinchflat/podcasts/rss_feed_builder.ex b/lib/pinchflat/podcasts/rss_feed_builder.ex index 6a1eb37..5e23edd 100644 --- a/lib/pinchflat/podcasts/rss_feed_builder.ex +++ b/lib/pinchflat/podcasts/rss_feed_builder.ex @@ -5,6 +5,8 @@ defmodule Pinchflat.Podcasts.RssFeedBuilder do @datetime_format "%a, %d %b %Y %H:%M:%S %z" + import Pinchflat.Utils.XmlUtils, only: [safe: 1] + alias Pinchflat.Utils.DatetimeUtils alias Pinchflat.Podcasts.PodcastHelpers alias PinchflatWeb.Router.Helpers, as: Routes @@ -94,14 +96,6 @@ defmodule Pinchflat.Podcasts.RssFeedBuilder do """ end - defp safe(nil), do: "" - - defp safe(value) do - value - |> Phoenix.HTML.html_escape() - |> Phoenix.HTML.safe_to_string() - end - defp generate_self_link(url_base, source) do Path.join(url_base, "#{podcast_route(:rss_feed, source.uuid)}.xml") end diff --git a/lib/pinchflat/utils/xml_utils.ex b/lib/pinchflat/utils/xml_utils.ex new file mode 100644 index 0000000..6cbabff --- /dev/null +++ b/lib/pinchflat/utils/xml_utils.ex @@ -0,0 +1,17 @@ +defmodule Pinchflat.Utils.XmlUtils do + @moduledoc """ + Utility methods for working with XML documents + """ + + @doc """ + Escapes invalid XML characters in a string + + Returns binary() + """ + def safe(value) do + value + |> to_string() + |> Phoenix.HTML.html_escape() + |> Phoenix.HTML.safe_to_string() + end +end diff --git a/test/pinchflat/metadata/nfo_builder_test.exs b/test/pinchflat/metadata/nfo_builder_test.exs index c6119a4..14ea1fa 100644 --- a/test/pinchflat/metadata/nfo_builder_test.exs +++ b/test/pinchflat/metadata/nfo_builder_test.exs @@ -30,6 +30,21 @@ defmodule Pinchflat.Metadata.NfoBuilderTest do assert String.contains?(nfo, ~S()) assert String.contains?(nfo, "#{metadata["title"]}") end + + test "escapes invalid characters", %{filepath: filepath} do + metadata = %{ + "title" => "hello' & ", + "uploader" => "uploader", + "id" => "id", + "description" => "description", + "upload_date" => "20210101" + } + + result = NfoBuilder.build_and_store_for_media_item(filepath, metadata) + nfo = File.read!(result) + + assert String.contains?(nfo, "hello' & <world>") + end end describe "build_and_store_for_source/2" do @@ -46,5 +61,18 @@ defmodule Pinchflat.Metadata.NfoBuilderTest do assert String.contains?(nfo, ~S()) assert String.contains?(nfo, "#{metadata["title"]}") end + + test "escapes invalid characters", %{filepath: filepath} do + metadata = %{ + "title" => "hello' & ", + "description" => "description", + "id" => "id" + } + + result = NfoBuilder.build_and_store_for_source(filepath, metadata) + nfo = File.read!(result) + + assert String.contains?(nfo, "hello' & <world>") + end end end diff --git a/test/pinchflat/utils/xml_utils_test.exs b/test/pinchflat/utils/xml_utils_test.exs new file mode 100644 index 0000000..883cfc2 --- /dev/null +++ b/test/pinchflat/utils/xml_utils_test.exs @@ -0,0 +1,16 @@ +defmodule Pinchflat.Utils.XmlUtilsTest do + use ExUnit.Case, async: true + + alias Pinchflat.Utils.XmlUtils + + describe "safe/1" do + test "escapes invalid characters" do + assert XmlUtils.safe("hello' & ") == "hello' & <world>" + end + + test "converts input to string" do + assert XmlUtils.safe(42) == "42" + assert XmlUtils.safe(nil) == "" + end + end +end