diff --git a/CODESMELL.md b/CODESMELL.md index 3491290..a9a7296 100644 --- a/CODESMELL.md +++ b/CODESMELL.md @@ -403,10 +403,14 @@ --- -### CSM-026 — TOCTOU Race Condition in Template File System -- **File:** `lib/bds/rendering/file_system.ex:28-37` -- **What:** `Enum.find(&File.regular?/1)` checks existence, then the file is read later (in the `Liquex.FileSystem` impl, Z. 43-49). Between check and read the file can vanish. -- **Fix:** Just try to read and handle `{:error, :enoent}`. Remove the `Enum.find` existence check and attempt reads directly. +### ~~CSM-026 — TOCTOU Race Condition in Template File System~~ ✅ FIXED +- **Fixed:** 2026-05-11 +- **What was done:** + - Extracted `candidate_paths/2` — validates the template path and returns all candidate file paths without checking existence. + - Added `try_read/2` — attempts `File.read` on each candidate path sequentially, returning `{:ok, contents}` on first success or `{:error, :enoent}` when all fail. No separate existence check. + - Simplified `full_path/2` to delegate to `candidate_paths/2` (returns first candidate for backward compatibility with tests). + - Rewrote `Liquex.FileSystem` protocol impl to use `try_read/2` directly, eliminating the TOCTOU window between `File.regular?` and `File.read`. + - Added 10 tests in `test/bds/csm026_toctou_file_system_test.exs`: atomic read, missing template, multi-root fallthrough, first-root-wins priority, file-deleted-between-calls safety, protocol read, protocol raise on missing, and path validation (empty, absolute, traversal). --- diff --git a/lib/bds/rendering/file_system.ex b/lib/bds/rendering/file_system.ex index 3a688be..071418a 100644 --- a/lib/bds/rendering/file_system.ex +++ b/lib/bds/rendering/file_system.ex @@ -13,8 +13,8 @@ defmodule BDS.Rendering.FileSystem do new([root_path]) end - @spec full_path(t(), String.t()) :: String.t() - def full_path(%__MODULE__{root_paths: root_paths}, template_path) do + @spec candidate_paths(t(), String.t()) :: [String.t()] + def candidate_paths(%__MODULE__{root_paths: root_paths}, template_path) do normalized_path = to_string(template_path) cond do @@ -29,17 +29,29 @@ defmodule BDS.Rendering.FileSystem do true -> filename = ensure_liquid_ext(normalized_path) + Enum.map(root_paths, &Path.expand(Path.join(&1, filename))) + end + end - root_paths - |> Enum.map(&Path.expand(Path.join(&1, filename))) - |> Enum.find(&File.regular?/1) - |> case do - nil -> - Path.expand(Path.join(List.first(root_paths) || ".", filename)) + @spec full_path(t(), String.t()) :: String.t() + def full_path(%__MODULE__{} = fs, template_path) do + List.first(candidate_paths(fs, template_path)) + end - path -> - path - end + @spec try_read(t(), String.t()) :: {:ok, String.t()} | {:error, :enoent} + def try_read(%__MODULE__{} = fs, template_path) do + fs + |> candidate_paths(template_path) + |> try_read_from_paths() + end + + defp try_read_from_paths([]), do: {:error, :enoent} + + defp try_read_from_paths([path | rest]) do + case File.read(path) do + {:ok, contents} -> {:ok, contents} + {:error, :enoent} -> try_read_from_paths(rest) + {:error, _reason} -> try_read_from_paths(rest) end end @@ -50,12 +62,9 @@ end defimpl Liquex.FileSystem, for: BDS.Rendering.FileSystem do def read_template_file(file_system, template_path) do - file_system - |> BDS.Rendering.FileSystem.full_path(template_path) - |> File.read() - |> case do + case BDS.Rendering.FileSystem.try_read(file_system, template_path) do {:ok, contents} -> contents - _error -> raise Liquex.Error, message: "No such template '#{template_path}'" + {:error, :enoent} -> raise Liquex.Error, message: "No such template '#{template_path}'" end end end diff --git a/test/bds/csm026_toctou_file_system_test.exs b/test/bds/csm026_toctou_file_system_test.exs new file mode 100644 index 0000000..10c619c --- /dev/null +++ b/test/bds/csm026_toctou_file_system_test.exs @@ -0,0 +1,106 @@ +defmodule BDS.TOCTOU.FileSystemTest do + use ExUnit.Case, async: true + + alias BDS.Rendering.FileSystem, as: TemplateFileSystem + + describe "try_read/2 eliminates TOCTOU race" do + @tag :tmp_dir + test "reads file atomically without separate existence check", %{tmp_dir: tmp_dir} do + File.write!(Path.join(tmp_dir, "header.liquid"), "HEADER CONTENT") + fs = TemplateFileSystem.new(tmp_dir) + + assert {:ok, "HEADER CONTENT"} = TemplateFileSystem.try_read(fs, "header") + end + + @tag :tmp_dir + test "returns {:error, :enoent} for missing templates", %{tmp_dir: tmp_dir} do + fs = TemplateFileSystem.new(tmp_dir) + + assert {:error, :enoent} = TemplateFileSystem.try_read(fs, "nonexistent") + end + + @tag :tmp_dir + test "falls through to next root path when first is missing", %{tmp_dir: tmp_dir} do + root_a = Path.join(tmp_dir, "a") + root_b = Path.join(tmp_dir, "b") + File.mkdir_p!(root_a) + File.mkdir_p!(root_b) + File.write!(Path.join(root_b, "partial.liquid"), "FROM B") + + fs = TemplateFileSystem.new([root_a, root_b]) + + assert {:ok, "FROM B"} = TemplateFileSystem.try_read(fs, "partial") + end + + @tag :tmp_dir + test "first root path wins when both have the template", %{tmp_dir: tmp_dir} do + root_a = Path.join(tmp_dir, "a") + root_b = Path.join(tmp_dir, "b") + File.mkdir_p!(root_a) + File.mkdir_p!(root_b) + File.write!(Path.join(root_a, "shared.liquid"), "FROM A") + File.write!(Path.join(root_b, "shared.liquid"), "FROM B") + + fs = TemplateFileSystem.new([root_a, root_b]) + + assert {:ok, "FROM A"} = TemplateFileSystem.try_read(fs, "shared") + end + + @tag :tmp_dir + test "file deleted between candidate_paths and try_read does not crash", %{tmp_dir: tmp_dir} do + path = Path.join(tmp_dir, "ephemeral.liquid") + File.write!(path, "TEMPORARY") + fs = TemplateFileSystem.new(tmp_dir) + + _candidates = TemplateFileSystem.candidate_paths(fs, "ephemeral") + File.rm!(path) + + assert {:error, :enoent} = TemplateFileSystem.try_read(fs, "ephemeral") + end + end + + describe "read_template_file/2 protocol uses atomic read" do + @tag :tmp_dir + test "reads existing template", %{tmp_dir: tmp_dir} do + File.write!(Path.join(tmp_dir, "footer.liquid"), "FOOTER") + fs = TemplateFileSystem.new(tmp_dir) + + assert "FOOTER" = Liquex.FileSystem.read_template_file(fs, "footer") + end + + @tag :tmp_dir + test "raises on missing template", %{tmp_dir: tmp_dir} do + fs = TemplateFileSystem.new(tmp_dir) + + assert_raise Liquex.Error, ~r/No such template/, fn -> + Liquex.FileSystem.read_template_file(fs, "missing") + end + end + end + + describe "candidate_paths/2 validation" do + test "raises on empty path" do + fs = TemplateFileSystem.new("/tmp") + + assert_raise Liquex.Error, ~r/Illegal template path/, fn -> + TemplateFileSystem.candidate_paths(fs, "") + end + end + + test "raises on absolute path" do + fs = TemplateFileSystem.new("/tmp") + + assert_raise Liquex.Error, ~r/Illegal template path/, fn -> + TemplateFileSystem.candidate_paths(fs, "/etc/passwd") + end + end + + test "raises on path traversal" do + fs = TemplateFileSystem.new("/tmp") + + assert_raise Liquex.Error, ~r/Illegal template path/, fn -> + TemplateFileSystem.candidate_paths(fs, "../secret") + end + end + end +end