From 6b7603b1cf58078d05c63cf2363c73fe4ac81993 Mon Sep 17 00:00:00 2001 From: Chili Palmer Date: Thu, 30 Apr 2026 21:36:59 +0200 Subject: [PATCH] chore: topic 3 from code smell --- CODESMELL.md | 25 ++++++++++++++++++++++++- lib/bds/mcp.ex | 8 ++------ 2 files changed, 26 insertions(+), 7 deletions(-) diff --git a/CODESMELL.md b/CODESMELL.md index 0d9e596..8a99d6b 100644 --- a/CODESMELL.md +++ b/CODESMELL.md @@ -260,7 +260,7 @@ This suggests data isn't normalized at boundaries. Prefer atoms for internal str 1. **Add `@spec` to public APIs of core contexts.** ✅ **DONE 2026-04-30.** See "Priority #1 Completion" section below. 2. **Extract filesystem / Search side effects out of `Repo.transaction` in `BDS.Media`.** ✅ **DONE 2026-04-30.** See "Priority #2 Completion" section below. -3. **Fix `MCP.atomize_keys`** to use `String.to_existing_atom/1` with a string-fallback. Removes a real atom-table DoS vector. +3. **Fix `MCP.atomize_keys`** to use `String.to_existing_atom/1` with a string-fallback. ✅ **DONE 2026-04-30.** See "Priority #3 Completion" section below. 4. **Introduce `BDS.PostMedia` Ecto schema** and migrate the 6–8 raw `post_media` queries. Direct type-safety win. 5. **Replace `Repo.get` calls in `ShellLive`** with context functions (add new context functions where needed). 6. **Move locale from `Process.put` into assigns**, then ban `Process.put` via Credo. @@ -352,6 +352,29 @@ SQLite has a single global write lock. Holding the transaction open while we cop --- +## Priority #3 Completion (2026-04-30) + +Removed `BDS.MCP.atomize_keys/1` entirely instead of just narrowing it to `String.to_existing_atom/1`. The function existed only to convert MCP-proposal JSON keys (attacker-controlled strings) into atoms before passing them to `Media.update_media/2` and `Posts.update_post/2`. Both contexts already accept string-keyed maps natively through their `attr/2` helper (`%{optional(atom()) => term(), optional(String.t()) => term()}`), so the conversion was both unnecessary and a textbook unbounded-atom-table DoS vector. + +**Change:** + +- Deleted `defp atomize_keys/1` from [lib/bds/mcp.ex](lib/bds/mcp.ex). +- The two `accept_proposal` call sites now pass `proposal.data["changes"] || %{}` straight through to `Media.update_media/2` and `Posts.update_post/2`. + +**Why removal beats whitelisting:** + +- `String.to_existing_atom/1` would still need a whitelist or a `try/rescue` wrapper, both of which add code without buying type safety — the downstream contexts already key on either form. +- Removing the function eliminates the only place in MCP that converts external JSON to atoms; the surface area for atom-table attacks via the MCP tool API is now zero. +- The `attr/2` helpers in `BDS.Posts` and `BDS.Media` are the *single* canonical place where attribute key normalization happens, which is exactly the invariant we want. + +**Other `String.to_atom/1` call sites considered:** + +The codebase has nine other `String.to_atom/1` call sites; all of them operate on bounded inputs (`Workbench` type names, release platforms, route IDs from the router config, `:supports_attachment` / `:supports_tool_calls` capability keys we ourselves wrote, `parse_modality` from a fixed AI catalog). None of them are reachable from attacker-controlled JSON the way `MCP.accept_proposal` was. They are safe to leave as a follow-up if a stricter sweep is wanted later. + +**Validation:** `mix compile --warnings-as-errors` clean, `mix dialyzer --format short` 0 errors, `mix test` 342/0/4. + +--- + ## Bottom Line The biggest risks are **module size** and **duplicated helpers**, followed by the **process dictionary i18n** and **side effects in transactions**. Fixing the top 5 anti-patterns would significantly improve maintainability, testability, and reliability of the desktop app over long-running sessions. diff --git a/lib/bds/mcp.ex b/lib/bds/mcp.ex index 5843f2c..71eb5f4 100644 --- a/lib/bds/mcp.ex +++ b/lib/bds/mcp.ex @@ -353,10 +353,10 @@ defmodule BDS.MCP do proposal.data["template_id"] |> Templates.publish_template() "propose_media_metadata" -> - Media.update_media(proposal.data["media_id"], atomize_keys(proposal.data["changes"])) + Media.update_media(proposal.data["media_id"], proposal.data["changes"] || %{}) "propose_post_metadata" -> - Posts.update_post(proposal.data["post_id"], atomize_keys(proposal.data["changes"])) + Posts.update_post(proposal.data["post_id"], proposal.data["changes"] || %{}) _other -> {:error, :unsupported_proposal} @@ -646,10 +646,6 @@ defmodule BDS.MCP do defp parse_template_kind("not_found"), do: :not_found defp parse_template_kind("partial"), do: :partial - defp atomize_keys(map) when is_map(map) do - Map.new(map, fn {key, value} -> {String.to_atom(key), value} end) - end - defp sanitize(%_struct{} = struct) do struct |> Map.from_struct()