From 21b11ef87e1627492b73d39d5433d7358875a9bb Mon Sep 17 00:00:00 2001 From: Chili Palmer Date: Thu, 11 Jun 2026 12:13:14 +0200 Subject: [PATCH] fix: fixed TD-01 and TD-25 --- TECHDEBTS.md | 39 +-- config/config.exs | 9 +- config/test.exs | 4 + lib/bds/ai/one_shot.ex | 4 +- lib/bds/ai/secret_backend.ex | 90 ++++++- lib/bds/ai/secret_key.ex | 233 ++++++++++++++++++ lib/bds/ai/secret_migration.ex | 67 +++++ lib/bds/application.ex | 5 +- lib/bds/desktop/shell_live/chat_editor.ex | 5 +- .../shell_live/chat_editor/message_build.ex | 4 +- lib/bds/desktop/shell_live/import_editor.ex | 2 +- lib/bds/desktop/shell_live/panel_renderer.ex | 3 +- .../shell_live/post_editor/list_values.ex | 4 +- .../shell_live/settings_editor/ai_settings.ex | 14 +- lib/bds/mac_bundle/dylibs.ex | 10 +- lib/bds/maintenance/diff_computation.ex | 2 +- lib/bds/rendering/template_selection.ex | 22 +- lib/bds/repo_bootstrap.ex | 3 + test/bds/ai/secret_backend_test.exs | 90 +++++++ test/bds/ai/secret_key_test.exs | 173 +++++++++++++ test/bds/ai/secret_migration_test.exs | 112 +++++++++ 21 files changed, 826 insertions(+), 69 deletions(-) create mode 100644 lib/bds/ai/secret_key.ex create mode 100644 lib/bds/ai/secret_migration.ex create mode 100644 test/bds/ai/secret_backend_test.exs create mode 100644 test/bds/ai/secret_key_test.exs create mode 100644 test/bds/ai/secret_migration_test.exs diff --git a/TECHDEBTS.md b/TECHDEBTS.md index 755e8b6..337ff4a 100644 --- a/TECHDEBTS.md +++ b/TECHDEBTS.md @@ -28,10 +28,25 @@ the review snapshot (commit `bf93403`). ## Phase 1 — Top five (security & correctness) -### TD-01: Move the AI secret encryption key out of the repo +### TD-01: Move the AI secret encryption key out of the repo ✅ DONE (2026-06-11) **Severity: High (security).** +**Status: implemented.** `BDS.AI.SecretKey` resolves the master key from the +macOS Keychain (`security` CLI) with a 0600 key-file fallback under the +private app dir; the deterministic node-name fallback is gone (operations +return `{:error, :secret_key_unavailable}`); `BDS.AI.SecretMigration` +re-encrypts legacy rows at every boot from `BDS.RepoBootstrap`; the endpoint +`secret_key_base` is generated per boot. The legacy repo key literal remains +in `SecretBackend`/tests **only** to decrypt and migrate existing user +databases — remove it together with `SecretMigration` in a future release. +The test env pins a deterministic `:ai_secret_key` in `config/test.exs` so +the suite never touches the keyring; that string protects nothing. +Rode along (mandated clean-gates): fixed all 10 pre-existing compiler type +warnings surfaced by the full recompile and the one dialyzer finding +(MapSet opacity in `mac_bundle/dylibs.ex`), so `mix compile +--warnings-as-errors --force` and `mix dialyzer` are now clean baselines. + **Context.** `BDS.AI.SecretBackend` encrypts AI provider API keys at rest with AES-256-GCM, but the key is the hardcoded string in `config/config.exs` (`config :bds, :ai_secret_key, "bds_desktop_shell_secret_key_base_..."`), which @@ -641,24 +656,14 @@ verify whether that section should say LiveView/HEEx). **Acceptance.** AGENTS.md commands all match the Elixir toolchain. -### TD-25: Generate the desktop endpoint secret at runtime +### TD-25: Generate the desktop endpoint secret at runtime ✅ DONE (2026-06-11, shipped with TD-01) -**Context.** Partially covered by TD-01; tracked separately in case TD-01 -ships Keychain-only. The Phoenix endpoint `secret_key_base` (session/LiveView -signing) is a hardcoded repo string in all envs. The endpoint is -loopback-only, which mitigates remote exploitation, but a random per-boot -secret costs nothing for a desktop app (sessions don't need to survive -restarts). +**Context.** The Phoenix endpoint `secret_key_base` (session/LiveView +signing) was a hardcoded repo string in all envs. -**Files.** -- `config/config.exs` (line ~24), `lib/bds/application.ex` (`desktop_secret_key_base/0`) - -**Approach.** In `Application.start/2` (or runtime.exs for prod), generate -`Base.encode64(:crypto.strong_rand_bytes(48))` when no explicit config is -set; keep a fixed value only for `:test` if tests depend on it. - -**Acceptance.** Released app has no static signing secret; LiveView still -connects after restart (fresh session is acceptable and verified). +**Status: implemented as part of TD-01.** `BDS.Application.desktop_secret_key_base/0` +now generates `Base.encode64(:crypto.strong_rand_bytes(48))` per boot when no +explicit config is set; the static value was removed from `config/config.exs`. --- diff --git a/config/config.exs b/config/config.exs index e682335..3554e0f 100644 --- a/config/config.exs +++ b/config/config.exs @@ -16,15 +16,14 @@ config :bds, BDS.Repo, config :bds, BDS.Application, desktop_adapter: :desktop +# No secrets live in this file: the endpoint signing secret is generated per +# boot (BDS.Application) and the AI secret master key comes from the OS +# keyring (BDS.AI.SecretKey). config :bds, :desktop, port: 4010, window_size: {1280, 780}, window_min_size: {800, 600}, - title: "Blogging Desktop Server", - secret_key_base: "bds_desktop_shell_secret_key_base_64_chars_minimum_seed_value_001" - -config :bds, :ai_secret_key, - "bds_desktop_shell_secret_key_base_64_chars_minimum_seed_value_001" + title: "Blogging Desktop Server" config :bds, BDS.Desktop.Endpoint, url: [host: "127.0.0.1"], diff --git a/config/test.exs b/config/test.exs index 495a503..b5e50d2 100644 --- a/config/test.exs +++ b/config/test.exs @@ -9,6 +9,10 @@ config :bds, BDS.Repo, config :logger, level: :warning +# Deterministic, test-only master key so secret round-trips never touch the +# OS keyring (Keychain) or write key files on developer machines. +config :bds, :ai_secret_key, "bds-test-only-ai-secret-key-not-used-outside-the-test-env" + # Tests use the deterministic lexical stub backend so the suite stays offline # and never downloads the ~100 MB neural model. config :bds, :embeddings, diff --git a/lib/bds/ai/one_shot.ex b/lib/bds/ai/one_shot.ex index 46aa9af..7cfb2eb 100644 --- a/lib/bds/ai/one_shot.ex +++ b/lib/bds/ai/one_shot.ex @@ -406,7 +406,9 @@ defmodule BDS.AI.OneShot do title: media.title || "", alt: media.alt || "", caption: media.caption || "", - image_url: Map.get(media, :image_url), + # A stored media row has no remote URL; resolve_image_data_url/1 fills + # this from file_path before an :analyze_image request is built. + image_url: nil, file_path: media.file_path, project_id: media.project_id, language: media.language || "" diff --git a/lib/bds/ai/secret_backend.ex b/lib/bds/ai/secret_backend.ex index 4885f53..32cf838 100644 --- a/lib/bds/ai/secret_backend.ex +++ b/lib/bds/ai/secret_backend.ex @@ -1,25 +1,91 @@ defmodule BDS.AI.SecretBackend do - @moduledoc false + @moduledoc """ + Encrypts and decrypts AI provider secrets (AES-256-GCM) with the + machine-local master key resolved by `BDS.AI.SecretKey`. + + Values written by earlier releases — encrypted with key material that + shipped in the repository, or with the deterministic node-name fallback — + are still readable: `decrypt/1` falls back to the legacy keys, and + `BDS.AI.SecretMigration` re-encrypts such rows at boot. When no master key + can be obtained, both operations return `{:error, :secret_key_unavailable}` + instead of degrading to a weaker key. + """ + + require Logger + + alias BDS.AI.SecretKey @aad "bds-ai-secret" + # Key material shipped in the repository before TD-01. Retained only so + # existing user databases can be read and re-encrypted by + # BDS.AI.SecretMigration; remove both together in a future release. + @legacy_repo_key binary_part( + "bds_desktop_shell_secret_key_base_64_chars_minimum_seed_value_001", + 0, + 32 + ) + + @spec encrypt(String.t()) :: {:ok, String.t()} | {:error, term()} def encrypt(value) when is_binary(value) do - key = secret_key() - iv = :crypto.strong_rand_bytes(12) + with {:ok, key} <- secret_key() do + iv = :crypto.strong_rand_bytes(12) - {ciphertext, tag} = - :crypto.crypto_one_time_aead(:aes_256_gcm, key, iv, value, @aad, true) + {ciphertext, tag} = + :crypto.crypto_one_time_aead(:aes_256_gcm, key, iv, value, @aad, true) - {:ok, Base.encode64(iv <> tag <> ciphertext)} + {:ok, Base.encode64(iv <> tag <> ciphertext)} + end end + @spec decrypt(String.t()) :: {:ok, String.t()} | {:error, term()} def decrypt(encoded) when is_binary(encoded) do + with {:ok, key} <- secret_key() do + case decrypt_with(encoded, key) do + {:ok, plaintext} -> {:ok, plaintext} + {:error, :invalid_ciphertext} -> decrypt_legacy(encoded) + end + end + end + + @doc """ + Decrypts strictly with the current master key — no legacy fallback. Used by + `BDS.AI.SecretMigration` to detect rows that still need re-encryption. + """ + @spec decrypt_with_current_key(String.t()) :: {:ok, String.t()} | {:error, term()} + def decrypt_with_current_key(encoded) when is_binary(encoded) do + with {:ok, key} <- secret_key() do + decrypt_with(encoded, key) + end + end + + @doc """ + Attempts decryption with the legacy keys used by earlier releases. + """ + @spec decrypt_legacy(String.t()) :: {:ok, String.t()} | {:error, :invalid_ciphertext} + def decrypt_legacy(encoded) when is_binary(encoded) do + Enum.find_value(legacy_keys(), {:error, :invalid_ciphertext}, fn key -> + case decrypt_with(encoded, key) do + {:ok, plaintext} -> {:ok, plaintext} + {:error, :invalid_ciphertext} -> nil + end + end) + end + + defp legacy_keys do + [ + @legacy_repo_key, + :crypto.hash(:sha256, Atom.to_string(node()) <> ":bds:ai") + ] + end + + defp decrypt_with(encoded, key) do with {:ok, binary} <- Base.decode64(encoded), <> <- binary, plaintext when is_binary(plaintext) <- :crypto.crypto_one_time_aead( :aes_256_gcm, - secret_key(), + key, iv, ciphertext, @aad, @@ -33,9 +99,13 @@ defmodule BDS.AI.SecretBackend do end defp secret_key do - case Application.get_env(:bds, :ai_secret_key) do - key when is_binary(key) and byte_size(key) >= 32 -> binary_part(key, 0, 32) - _other -> :crypto.hash(:sha256, Atom.to_string(node()) <> ":bds:ai") + case SecretKey.fetch() do + {:ok, key} -> + {:ok, key} + + {:error, reason} -> + Logger.error("AI secret key unavailable: #{inspect(reason)}") + {:error, :secret_key_unavailable} end end end diff --git a/lib/bds/ai/secret_key.ex b/lib/bds/ai/secret_key.ex new file mode 100644 index 0000000..5c4bd3a --- /dev/null +++ b/lib/bds/ai/secret_key.ex @@ -0,0 +1,233 @@ +defmodule BDS.AI.SecretKey do + @moduledoc """ + Resolves the 32-byte machine-local master key that encrypts AI provider + secrets at rest (the `SecureKeyStore` entity in `specs/ai.allium`). + + Resolution order: + + 1. `config :bds, :ai_secret_key` — explicit override. Used by the test + suite for determinism; must be at least 32 bytes (the first 32 are + used). An invalid value is an error, never a silent fallback. + 2. A previously resolved key cached in `:persistent_term`. + 3. The OS keyring: on macOS the login Keychain via the `security` CLI; on + other platforms — or when the Keychain is unavailable — a random key + file under the private app dir, written with `0600` permissions. + + A fresh random key is generated and stored on first use. There is no + deterministic fallback: when no key can be obtained or persisted, `fetch/0` + returns `{:error, reason}` and secret encryption/decryption fails loudly + instead of degrading to obfuscation. + + Config (`config :bds, BDS.AI.SecretKey`): + + * `:strategy` — `:auto` (default; Keychain on macOS, key file elsewhere), + `:keychain`, or `:file` + * `:key_file_path` — overrides the key file location + * `:command_runner` — 3-arity replacement for `System.cmd/3` (tests) + """ + + require Logger + + @key_bytes 32 + @cache_key {__MODULE__, :key} + @keychain_service "bDS2" + @keychain_account "ai-secret-key" + @keychain_not_found_status 44 + @key_file_name "ai_secret.key" + + @spec fetch() :: {:ok, binary()} | {:error, term()} + def fetch do + case configured_key() do + {:ok, key} -> {:ok, key} + {:error, _detail} = error -> error + :unset -> cached_or_resolve() + end + end + + @doc "Clears the cached key so the next fetch re-resolves it (test helper)." + @spec reset_cache() :: :ok + def reset_cache do + :persistent_term.erase(@cache_key) + :ok + end + + defp configured_key do + case Application.get_env(:bds, :ai_secret_key) do + nil -> + :unset + + key when is_binary(key) and byte_size(key) >= @key_bytes -> + {:ok, binary_part(key, 0, @key_bytes)} + + other -> + {:error, {:invalid_configured_key, "expected a binary of at least #{@key_bytes} bytes, got: #{inspect(other)}"}} + end + end + + defp cached_or_resolve do + case :persistent_term.get(@cache_key, nil) do + key when is_binary(key) -> + {:ok, key} + + nil -> + with {:ok, key} <- resolve() do + :persistent_term.put(@cache_key, key) + {:ok, key} + end + end + end + + defp resolve do + case strategy() do + :keychain -> resolve_keychain() + :file -> resolve_file() + end + end + + defp strategy do + case config(:strategy, :auto) do + :auto -> + if match?({:unix, :darwin}, :os.type()), do: :keychain, else: :file + + explicit when explicit in [:keychain, :file] -> + explicit + end + end + + # ─── macOS Keychain ───────────────────────────────────────── + + defp resolve_keychain do + case keychain_find() do + {:ok, key} -> + {:ok, key} + + :not_found -> + keychain_create() + + {:error, reason} -> + Logger.warning( + "AI secret key: macOS Keychain unavailable (#{inspect(reason)}); falling back to the key file" + ) + + resolve_file() + end + end + + defp keychain_find do + case run_security([ + "find-generic-password", + "-s", + @keychain_service, + "-a", + @keychain_account, + "-w" + ]) do + {output, 0} -> + case output |> String.trim() |> Base.decode64() do + {:ok, key} when byte_size(key) == @key_bytes -> {:ok, key} + _other -> {:error, :corrupt_keychain_item} + end + + {_output, @keychain_not_found_status} -> + :not_found + + {output, status} -> + {:error, {:security_failed, status, String.trim(output)}} + end + end + + # The generated key passes through `security`'s argv, which is briefly + # visible to other local processes of the same user. Accepted trade-off for + # a single-user desktop app; `security` offers no non-interactive way to + # take the password on stdin in one shot. + defp keychain_create do + key = :crypto.strong_rand_bytes(@key_bytes) + + case run_security([ + "add-generic-password", + "-U", + "-s", + @keychain_service, + "-a", + @keychain_account, + "-w", + Base.encode64(key) + ]) do + {_output, 0} -> + {:ok, key} + + {output, status} -> + Logger.warning( + "AI secret key: could not store the key in the Keychain " <> + "(status #{status}: #{String.trim(output)}); falling back to the key file" + ) + + resolve_file() + end + end + + defp run_security(args) do + runner = config(:command_runner, &default_runner/3) + runner.("security", args, stderr_to_stdout: true) + end + + defp default_runner(command, args, opts) do + System.cmd(command, args, opts) + rescue + error in ErlangError -> {"#{command} unavailable: #{inspect(error.original)}", 127} + end + + # ─── Key file ─────────────────────────────────────────────── + + defp resolve_file do + path = key_file_path() + + case File.read(path) do + {:ok, contents} -> decode_key_file(contents, path) + {:error, :enoent} -> create_key_file(path) + {:error, reason} -> {:error, {:key_file_unreadable, path, reason}} + end + end + + defp decode_key_file(contents, path) do + case contents |> String.trim() |> Base.decode64() do + {:ok, key} when byte_size(key) == @key_bytes -> {:ok, key} + _other -> {:error, {:key_file_corrupt, path}} + end + end + + defp create_key_file(path) do + key = :crypto.strong_rand_bytes(@key_bytes) + temp_path = path <> ".tmp." <> Integer.to_string(System.unique_integer([:positive])) + + with :ok <- File.mkdir_p(Path.dirname(path)), + :ok <- File.write(temp_path, Base.encode64(key) <> "\n"), + :ok <- File.chmod(temp_path, 0o600), + :ok <- File.rename(temp_path, path) do + {:ok, key} + else + {:error, reason} -> + _ = File.rm(temp_path) + {:error, {:key_file_write_failed, path, reason}} + end + end + + defp key_file_path do + config(:key_file_path, nil) || Path.join(private_app_dir(), @key_file_name) + end + + # Same private app dir as BDS.Projects.private_app_dir/0 — on macOS + # ~/Library/Application Support/BDS2. Duplicated to keep this module free of + # project/DB dependencies. + defp private_app_dir do + case :filename.basedir(:user_config, "BDS2") do + path when is_list(path) -> List.to_string(path) + path -> path + end + |> Path.expand() + end + + defp config(key, default) do + Application.get_env(:bds, __MODULE__, []) |> Keyword.get(key, default) + end +end diff --git a/lib/bds/ai/secret_migration.ex b/lib/bds/ai/secret_migration.ex new file mode 100644 index 0000000..77eeeb9 --- /dev/null +++ b/lib/bds/ai/secret_migration.ex @@ -0,0 +1,67 @@ +defmodule BDS.AI.SecretMigration do + @moduledoc """ + Idempotent boot-time re-encryption of stored AI secrets. + + Earlier releases encrypted secrets with key material shipped in the + repository (or a deterministic node-name fallback). This pass finds the + `__encrypted_*` rows in `settings`, decrypts them with the legacy keys, and + re-encrypts them with the machine-local key from `BDS.AI.SecretKey`. Rows + already encrypted with the current key are left untouched; rows no known + key can decrypt are left in place and reported, so the user can re-enter + the secret. Runs from `BDS.RepoBootstrap` on every boot; on a migrated + database it is a cheap no-op. + """ + + import Ecto.Query + + require Logger + + alias BDS.AI.SecretBackend + alias BDS.Persistence + alias BDS.Repo + alias BDS.Settings.Setting + + @encrypted_prefix "__encrypted_" + + @spec migrate_legacy_secrets(module()) :: + {:ok, %{migrated: non_neg_integer(), failed: non_neg_integer()}} + def migrate_legacy_secrets(repo \\ Repo) do + summary = + from(setting in Setting, where: like(setting.key, ^"#{@encrypted_prefix}%")) + |> repo.all() + |> Enum.reduce(%{migrated: 0, failed: 0}, fn setting, acc -> + case migrate_row(repo, setting) do + :current -> acc + :migrated -> %{acc | migrated: acc.migrated + 1} + :failed -> %{acc | failed: acc.failed + 1} + end + end) + + {:ok, summary} + end + + defp migrate_row(repo, setting) do + with {:error, _no_current_key_match} <- SecretBackend.decrypt_with_current_key(setting.value), + {:ok, plaintext} <- SecretBackend.decrypt_legacy(setting.value), + {:ok, reencrypted} <- SecretBackend.encrypt(plaintext) do + repo.update_all( + from(s in Setting, where: s.key == ^setting.key), + set: [value: reencrypted, updated_at: Persistence.now_ms()] + ) + + Logger.info("AI secret #{setting.key} re-encrypted with the machine-local key") + :migrated + else + {:ok, _already_current} -> + :current + + {:error, reason} -> + Logger.warning( + "AI secret #{setting.key} could not be re-encrypted (#{inspect(reason)}); " <> + "leaving it unchanged — the secret may need to be entered again" + ) + + :failed + end + end +end diff --git a/lib/bds/application.ex b/lib/bds/application.ex index e02e637..5af33b9 100644 --- a/lib/bds/application.ex +++ b/lib/bds/application.ex @@ -83,8 +83,11 @@ defmodule BDS.Application do System.get_env("BDS_DESKTOP_AUTOMATION") in ["1", "true", "TRUE"] end + # The desktop endpoint binds to loopback only and its sessions do not need + # to survive restarts, so without explicit config the signing secret is a + # random per-boot value rather than a static one shipped in the repo. defp desktop_secret_key_base do Application.get_env(:bds, :desktop)[:secret_key_base] || - raise "missing :desktop secret_key_base configuration" + Base.encode64(:crypto.strong_rand_bytes(48)) end end diff --git a/lib/bds/desktop/shell_live/chat_editor.ex b/lib/bds/desktop/shell_live/chat_editor.ex index feccdd4..578cada 100644 --- a/lib/bds/desktop/shell_live/chat_editor.ex +++ b/lib/bds/desktop/shell_live/chat_editor.ex @@ -1027,9 +1027,8 @@ defmodule BDS.Desktop.ShellLive.ChatEditor do defp present?(value) when is_binary(value), do: String.trim(value) != "" defp present?(value), do: not is_nil(value) - defp format_error(%{kind: :endpoint_not_configured}), - do: dgettext("ui", "Configure an API key in Settings to enable AI chat.") - + # :endpoint_not_configured is handled by its own case clause before this is + # reached; the chat surface already shows the configuration hint. defp format_error(reason), do: inspect(reason) defp parse_integer(value) when is_integer(value), do: value diff --git a/lib/bds/desktop/shell_live/chat_editor/message_build.ex b/lib/bds/desktop/shell_live/chat_editor/message_build.ex index 458e6bc..3a10497 100644 --- a/lib/bds/desktop/shell_live/chat_editor/message_build.ex +++ b/lib/bds/desktop/shell_live/chat_editor/message_build.ex @@ -189,6 +189,8 @@ defmodule BDS.Desktop.ShellLive.ChatEditor.MessageBuild do |> mark_surfaces_expanded(assigns) end + # Only called from pending_user_message/2, which already narrows the + # request to %{message: binary}. defp persisted_user_message_for_request?(messages, %{message: message} = request) when is_binary(message) do messages @@ -198,8 +200,6 @@ defmodule BDS.Desktop.ShellLive.ChatEditor.MessageBuild do end) end - defp persisted_user_message_for_request?(_messages, _request), do: false - defp persisted_assistant_content_for_request?(messages, request, content) when is_binary(content) and content != "" do messages diff --git a/lib/bds/desktop/shell_live/import_editor.ex b/lib/bds/desktop/shell_live/import_editor.ex index c3df421..dcc61e8 100644 --- a/lib/bds/desktop/shell_live/import_editor.ex +++ b/lib/bds/desktop/shell_live/import_editor.ex @@ -1356,7 +1356,7 @@ defmodule BDS.Desktop.ShellLive.ImportEditor do class="taxonomy-mapping-input" type="text" name="mapped_to" - value={Map.get(@edit || %{}, :value, Map.get(item, :mapped_to) || "") || ""} + value={Map.get(@edit, :value, Map.get(item, :mapped_to) || "") || ""} placeholder={dgettext("ui", "Map to...")} list={"taxonomy-suggestions-#{@type}"} autocomplete="off" diff --git a/lib/bds/desktop/shell_live/panel_renderer.ex b/lib/bds/desktop/shell_live/panel_renderer.ex index 411aee7..024e7cc 100644 --- a/lib/bds/desktop/shell_live/panel_renderer.ex +++ b/lib/bds/desktop/shell_live/panel_renderer.ex @@ -294,12 +294,11 @@ defmodule BDS.Desktop.ShellLive.PanelRenderer do defp short_commit_hash(hash) when is_binary(hash), do: String.slice(hash, 0, 7) defp short_commit_hash(_hash), do: "-------" + # Only called inside the template's `is_number(task.progress)` guard. defp progress_percent(progress) when is_number(progress) do rounded = progress |> Kernel.*(100) |> Float.round(0) |> trunc() "#{rounded}%" end - defp progress_percent(_), do: "" - defp present?(value), do: value not in [nil, ""] end diff --git a/lib/bds/desktop/shell_live/post_editor/list_values.ex b/lib/bds/desktop/shell_live/post_editor/list_values.ex index 530fa1a..565b609 100644 --- a/lib/bds/desktop/shell_live/post_editor/list_values.ex +++ b/lib/bds/desktop/shell_live/post_editor/list_values.ex @@ -114,9 +114,7 @@ defmodule BDS.Desktop.ShellLive.PostEditor.ListValues do end end - defp normalize_color(nil), do: nil - defp normalize_color(""), do: nil - + # nil is handled by tag_chip_style/1 before this is reached. defp normalize_color("#" <> rest = color) when byte_size(rest) == 6 do if String.match?(rest, ~r/\A[0-9a-fA-F]{6}\z/), do: color, else: nil end diff --git a/lib/bds/desktop/shell_live/settings_editor/ai_settings.ex b/lib/bds/desktop/shell_live/settings_editor/ai_settings.ex index 24deecf..12aab46 100644 --- a/lib/bds/desktop/shell_live/settings_editor/ai_settings.ex +++ b/lib/bds/desktop/shell_live/settings_editor/ai_settings.ex @@ -25,8 +25,8 @@ defmodule BDS.Desktop.ShellLive.SettingsEditor.AISettings do model_disables_reasoning?( get_model_preference(:chat) || Map.get(online_endpoint || %{}, :model, "") ), - "online_title_model" => get_model_preference(:title), - "online_image_analysis_model" => get_model_preference(:image_analysis), + "online_title_model" => get_model_preference(:title) || "", + "online_image_analysis_model" => get_model_preference(:image_analysis) || "", "online_chat_images" => model_supports_images?(get_model_preference(:image_analysis)), "offline_url" => Map.get(airplane_endpoint || %{}, :url, ""), "offline_api_key" => Map.get(airplane_endpoint || %{}, :api_key, ""), @@ -42,8 +42,8 @@ defmodule BDS.Desktop.ShellLive.SettingsEditor.AISettings do model_disables_reasoning?( get_model_preference(:airplane_chat) || Map.get(airplane_endpoint || %{}, :model, "") ), - "offline_title_model" => get_model_preference(:airplane_title), - "offline_image_analysis_model" => get_model_preference(:airplane_image_analysis), + "offline_title_model" => get_model_preference(:airplane_title) || "", + "offline_image_analysis_model" => get_model_preference(:airplane_image_analysis) || "", "offline_chat_images" => model_supports_images?(get_model_preference(:airplane_image_analysis)), "system_prompt" => EditorSettings.get_global_setting("ai.system_prompt") || "" @@ -225,10 +225,12 @@ defmodule BDS.Desktop.ShellLive.SettingsEditor.AISettings do } end + # Returns nil when no preference is stored so `||` fallbacks to the + # endpoint's model actually fire. defp get_model_preference(key) do case AI.get_model_preference(key) do - {:ok, value} -> value || "" - _other -> "" + {:ok, value} when is_binary(value) and value != "" -> value + _other -> nil end end diff --git a/lib/bds/mac_bundle/dylibs.ex b/lib/bds/mac_bundle/dylibs.ex index 7af07f7..dc51f86 100644 --- a/lib/bds/mac_bundle/dylibs.ex +++ b/lib/bds/mac_bundle/dylibs.ex @@ -75,7 +75,7 @@ defmodule BDS.MacBundle.Dylibs do def bundle(nif_path, frameworks_dir, nif_loader_prefix) do File.mkdir_p!(frameworks_dir) - with {:ok, {_seen, externals}} <- collect(nif_path, MapSet.new(), []) do + with {:ok, {_seen, externals}} <- collect(nif_path, %{}, []) do externals = Enum.reverse(externals) {physical, logical_entries, _inode_map} = @@ -119,17 +119,19 @@ defmodule BDS.MacBundle.Dylibs do end # Depth-first transitive collection of external dependency paths. Returns - # `{:ok, {seen, acc}}` where `acc` is the reverse-discovery-order path list. + # `{:ok, {seen, acc}}` where `seen` is a plain map used as a set (a MapSet's + # opaque internals trip dialyzer when threaded through the recursion) and + # `acc` is the reverse-discovery-order path list. defp collect(binary, seen, acc) do case otool(binary) do {:ok, deps} -> deps |> Enum.filter(&external?/1) |> Enum.reduce_while({:ok, {seen, acc}}, fn dep, {:ok, {seen_acc, list_acc}} -> - if MapSet.member?(seen_acc, dep) do + if Map.has_key?(seen_acc, dep) do {:cont, {:ok, {seen_acc, list_acc}}} else - seen_acc = MapSet.put(seen_acc, dep) + seen_acc = Map.put(seen_acc, dep, true) case collect(dep, seen_acc, [dep | list_acc]) do {:ok, _} = ok -> {:cont, ok} error -> {:halt, error} diff --git a/lib/bds/maintenance/diff_computation.ex b/lib/bds/maintenance/diff_computation.ex index 0dcc7a3..88e2cf2 100644 --- a/lib/bds/maintenance/diff_computation.ex +++ b/lib/bds/maintenance/diff_computation.ex @@ -66,8 +66,8 @@ defmodule BDS.Maintenance.DiffComputation do end def stringify_value(nil), do: "" + # Booleans are atoms, so this clause also renders true/false. def stringify_value(value) when is_atom(value), do: Atom.to_string(value) - def stringify_value(value) when is_boolean(value), do: to_string(value) def stringify_value(value) when is_integer(value), do: Integer.to_string(value) def stringify_value(value) when is_binary(value), do: value diff --git a/lib/bds/rendering/template_selection.ex b/lib/bds/rendering/template_selection.ex index 506b6ea..1e26d4c 100644 --- a/lib/bds/rendering/template_selection.ex +++ b/lib/bds/rendering/template_selection.ex @@ -82,20 +82,16 @@ defmodule BDS.Rendering.TemplateSelection do end defp select_template(project_id, :post, nil) do - case StarterTemplates.default_slug(:post) do - nil -> - nil + default_slug = StarterTemplates.default_slug(:post) - default_slug -> - Repo.one( - from template in Template, - where: - template.project_id == ^project_id and template.kind == :post and - template.status == :published and - template.enabled == true and template.slug == ^default_slug, - limit: 1 - ) - end + Repo.one( + from template in Template, + where: + template.project_id == ^project_id and template.kind == :post and + template.status == :published and + template.enabled == true and template.slug == ^default_slug, + limit: 1 + ) end defp select_template(project_id, kind, nil) do diff --git a/lib/bds/repo_bootstrap.ex b/lib/bds/repo_bootstrap.ex index dedd6fb..8146975 100644 --- a/lib/bds/repo_bootstrap.ex +++ b/lib/bds/repo_bootstrap.ex @@ -24,6 +24,9 @@ defmodule BDS.RepoBootstrap do {:ok, _project} -> :ok {:error, reason} -> raise "failed to ensure default project: #{inspect(reason)}" end + + {:ok, _summary} = BDS.AI.SecretMigration.migrate_legacy_secrets() + :ok else :ok end diff --git a/test/bds/ai/secret_backend_test.exs b/test/bds/ai/secret_backend_test.exs new file mode 100644 index 0000000..dfb0a7e --- /dev/null +++ b/test/bds/ai/secret_backend_test.exs @@ -0,0 +1,90 @@ +defmodule BDS.AI.SecretBackendTest do + use ExUnit.Case, async: false + + alias BDS.AI.SecretBackend + alias BDS.AI.SecretKey + + # Key material shipped in the repository before TD-01. Kept here only to + # prove that secrets stored by earlier releases remain readable. + @legacy_repo_key binary_part( + "bds_desktop_shell_secret_key_base_64_chars_minimum_seed_value_001", + 0, + 32 + ) + + @aad "bds-ai-secret" + + setup do + original_key = Application.fetch_env(:bds, :ai_secret_key) + original_config = Application.fetch_env(:bds, SecretKey) + + on_exit(fn -> + restore_env(:ai_secret_key, original_key) + restore_env(SecretKey, original_config) + SecretKey.reset_cache() + end) + + SecretKey.reset_cache() + :ok + end + + defp restore_env(key, {:ok, value}), do: Application.put_env(:bds, key, value) + defp restore_env(key, :error), do: Application.delete_env(:bds, key) + + defp encrypt_with(key, value) do + iv = :crypto.strong_rand_bytes(12) + {ciphertext, tag} = :crypto.crypto_one_time_aead(:aes_256_gcm, key, iv, value, @aad, true) + Base.encode64(iv <> tag <> ciphertext) + end + + test "round-trips a secret with the current key" do + assert {:ok, encrypted} = SecretBackend.encrypt("sk-live-12345") + refute encrypted == "sk-live-12345" + assert {:ok, "sk-live-12345"} = SecretBackend.decrypt(encrypted) + end + + test "rejects garbage ciphertext" do + assert {:error, :invalid_ciphertext} = SecretBackend.decrypt("not-encrypted") + assert {:error, :invalid_ciphertext} = SecretBackend.decrypt(Base.encode64("too-short")) + end + + test "still decrypts values encrypted with the legacy repo-baked key" do + legacy_value = encrypt_with(@legacy_repo_key, "legacy-online-key") + + assert {:ok, "legacy-online-key"} = SecretBackend.decrypt(legacy_value) + end + + test "still decrypts values encrypted with the legacy node-name key" do + node_key = :crypto.hash(:sha256, Atom.to_string(node()) <> ":bds:ai") + legacy_value = encrypt_with(node_key, "legacy-node-key-secret") + + assert {:ok, "legacy-node-key-secret"} = SecretBackend.decrypt(legacy_value) + end + + test "decrypt_with_current_key does not accept legacy ciphertexts" do + legacy_value = encrypt_with(@legacy_repo_key, "legacy-secret") + + assert {:error, :invalid_ciphertext} = SecretBackend.decrypt_with_current_key(legacy_value) + + assert {:ok, current_value} = SecretBackend.encrypt("current-secret") + assert {:ok, "current-secret"} = SecretBackend.decrypt_with_current_key(current_value) + end + + test "fails loudly when no secret key can be obtained" do + Application.delete_env(:bds, :ai_secret_key) + + blocked_dir = Path.join(System.tmp_dir!(), "bds-blocked-#{System.unique_integer([:positive])}") + File.write!(blocked_dir, "occupied") + on_exit(fn -> File.rm(blocked_dir) end) + + Application.put_env(:bds, SecretKey, + strategy: :file, + key_file_path: Path.join(blocked_dir, "ai_secret.key") + ) + + assert {:error, :secret_key_unavailable} = SecretBackend.encrypt("anything") + + valid_looking = encrypt_with(:crypto.strong_rand_bytes(32), "anything") + assert {:error, :secret_key_unavailable} = SecretBackend.decrypt(valid_looking) + end +end diff --git a/test/bds/ai/secret_key_test.exs b/test/bds/ai/secret_key_test.exs new file mode 100644 index 0000000..5afedc3 --- /dev/null +++ b/test/bds/ai/secret_key_test.exs @@ -0,0 +1,173 @@ +defmodule BDS.AI.SecretKeyTest do + use ExUnit.Case, async: false + + import Bitwise + + alias BDS.AI.SecretKey + + setup do + original_key = Application.fetch_env(:bds, :ai_secret_key) + original_config = Application.fetch_env(:bds, SecretKey) + + on_exit(fn -> + restore_env(:ai_secret_key, original_key) + restore_env(SecretKey, original_config) + SecretKey.reset_cache() + end) + + SecretKey.reset_cache() + :ok + end + + defp restore_env(key, {:ok, value}), do: Application.put_env(:bds, key, value) + defp restore_env(key, :error), do: Application.delete_env(:bds, key) + + defp tmp_key_path do + Path.join([ + System.tmp_dir!(), + "bds-secret-key-test-#{System.unique_integer([:positive])}", + "ai_secret.key" + ]) + end + + describe "configured key override" do + test "uses the first 32 bytes of an explicitly configured key" do + Application.put_env(:bds, :ai_secret_key, String.duplicate("k", 40)) + + assert {:ok, key} = SecretKey.fetch() + assert key == String.duplicate("k", 32) + end + + test "rejects a configured key that is too short instead of falling back" do + Application.put_env(:bds, :ai_secret_key, "too-short") + + assert {:error, {:invalid_configured_key, _detail}} = SecretKey.fetch() + end + end + + describe "file strategy" do + setup do + Application.delete_env(:bds, :ai_secret_key) + path = tmp_key_path() + Application.put_env(:bds, SecretKey, strategy: :file, key_file_path: path) + on_exit(fn -> File.rm_rf(Path.dirname(path)) end) + {:ok, path: path} + end + + test "generates a random 32-byte key file with 0600 permissions on first use", %{path: path} do + assert {:ok, key} = SecretKey.fetch() + assert byte_size(key) == 32 + + assert File.exists?(path) + assert (File.stat!(path).mode &&& 0o777) == 0o600 + assert {:ok, ^key} = path |> File.read!() |> String.trim() |> Base.decode64() + end + + test "returns the same key across cache resets by re-reading the file" do + assert {:ok, key} = SecretKey.fetch() + SecretKey.reset_cache() + assert {:ok, ^key} = SecretKey.fetch() + end + + test "reads an existing key file", %{path: path} do + key = :crypto.strong_rand_bytes(32) + File.mkdir_p!(Path.dirname(path)) + File.write!(path, Base.encode64(key) <> "\n") + + assert {:ok, ^key} = SecretKey.fetch() + end + + test "errors on a corrupt key file instead of overwriting it", %{path: path} do + File.mkdir_p!(Path.dirname(path)) + File.write!(path, "not-valid-base64!!") + + assert {:error, {:key_file_corrupt, ^path}} = SecretKey.fetch() + assert File.read!(path) == "not-valid-base64!!" + end + + test "fails loudly when the key file location is not writable", %{path: path} do + blocking_file = Path.dirname(path) + File.mkdir_p!(Path.dirname(blocking_file)) + File.write!(blocking_file, "occupied") + + assert {:error, _reason} = SecretKey.fetch() + end + end + + describe "keychain strategy" do + setup do + Application.delete_env(:bds, :ai_secret_key) + :ok + end + + test "returns the stored key when the keychain item exists" do + key = :crypto.strong_rand_bytes(32) + + runner = fn "security", ["find-generic-password" | _rest], _opts -> + {Base.encode64(key) <> "\n", 0} + end + + Application.put_env(:bds, SecretKey, strategy: :keychain, command_runner: runner) + + assert {:ok, ^key} = SecretKey.fetch() + end + + test "creates and stores a new key when the keychain item is missing" do + test_pid = self() + + runner = fn "security", [verb | rest], _opts -> + case verb do + "find-generic-password" -> + {"security: SecKeychainSearchCopyNext: The specified item could not be found.", 44} + + "add-generic-password" -> + send(test_pid, {:keychain_add, rest}) + {"", 0} + end + end + + Application.put_env(:bds, SecretKey, strategy: :keychain, command_runner: runner) + + assert {:ok, key} = SecretKey.fetch() + assert byte_size(key) == 32 + + assert_received {:keychain_add, add_args} + assert Base.encode64(key) in add_args + end + + test "falls back to the key file when the keychain is unavailable" do + path = tmp_key_path() + on_exit(fn -> File.rm_rf(Path.dirname(path)) end) + + runner = fn "security", _args, _opts -> {"security: unknown error", 1} end + + Application.put_env(:bds, SecretKey, + strategy: :keychain, + command_runner: runner, + key_file_path: path + ) + + assert {:ok, key} = SecretKey.fetch() + assert byte_size(key) == 32 + assert File.exists?(path) + end + + test "caches the resolved key so the keychain is not queried per call" do + test_pid = self() + key = :crypto.strong_rand_bytes(32) + + runner = fn "security", ["find-generic-password" | _rest], _opts -> + send(test_pid, :keychain_find) + {Base.encode64(key), 0} + end + + Application.put_env(:bds, SecretKey, strategy: :keychain, command_runner: runner) + + assert {:ok, ^key} = SecretKey.fetch() + assert {:ok, ^key} = SecretKey.fetch() + + assert_received :keychain_find + refute_received :keychain_find + end + end +end diff --git a/test/bds/ai/secret_migration_test.exs b/test/bds/ai/secret_migration_test.exs new file mode 100644 index 0000000..35738bd --- /dev/null +++ b/test/bds/ai/secret_migration_test.exs @@ -0,0 +1,112 @@ +defmodule BDS.AI.SecretMigrationTest do + use ExUnit.Case, async: false + + import ExUnit.CaptureLog + + alias BDS.AI.SecretBackend + alias BDS.AI.SecretMigration + alias BDS.Persistence + alias BDS.Repo + alias BDS.Settings.Setting + + # Key material shipped in the repository before TD-01; used to seed rows the + # way earlier releases stored them. + @legacy_repo_key binary_part( + "bds_desktop_shell_secret_key_base_64_chars_minimum_seed_value_001", + 0, + 32 + ) + + @aad "bds-ai-secret" + + setup do + :ok = Ecto.Adapters.SQL.Sandbox.checkout(BDS.Repo) + :ok + end + + defp encrypt_with(key, value) do + iv = :crypto.strong_rand_bytes(12) + {ciphertext, tag} = :crypto.crypto_one_time_aead(:aes_256_gcm, key, iv, value, @aad, true) + Base.encode64(iv <> tag <> ciphertext) + end + + defp insert_setting(key, value) do + %Setting{} + |> Setting.changeset(%{key: key, value: value, updated_at: Persistence.now_ms()}) + |> Repo.insert!() + end + + test "re-encrypts secrets stored with the legacy repo key" do + legacy_value = encrypt_with(@legacy_repo_key, "sk-online-123") + insert_setting("__encrypted_ai.online.api_key", legacy_value) + + assert {:ok, %{migrated: 1, failed: 0}} = SecretMigration.migrate_legacy_secrets() + + %Setting{value: new_value} = Repo.get(Setting, "__encrypted_ai.online.api_key") + refute new_value == legacy_value + assert {:ok, "sk-online-123"} = SecretBackend.decrypt_with_current_key(new_value) + end + + test "re-encrypts secrets stored with the legacy node-name key" do + node_key = :crypto.hash(:sha256, Atom.to_string(node()) <> ":bds:ai") + legacy_value = encrypt_with(node_key, "sk-airplane-456") + insert_setting("__encrypted_ai.airplane.api_key", legacy_value) + + assert {:ok, %{migrated: 1, failed: 0}} = SecretMigration.migrate_legacy_secrets() + + %Setting{value: new_value} = Repo.get(Setting, "__encrypted_ai.airplane.api_key") + assert {:ok, "sk-airplane-456"} = SecretBackend.decrypt_with_current_key(new_value) + end + + test "leaves rows already encrypted with the current key untouched" do + {:ok, current_value} = SecretBackend.encrypt("sk-current-789") + insert_setting("__encrypted_ai.online.api_key", current_value) + + assert {:ok, %{migrated: 0, failed: 0}} = SecretMigration.migrate_legacy_secrets() + + assert %Setting{value: ^current_value} = Repo.get(Setting, "__encrypted_ai.online.api_key") + end + + test "is idempotent" do + legacy_value = encrypt_with(@legacy_repo_key, "sk-online-123") + insert_setting("__encrypted_ai.online.api_key", legacy_value) + + assert {:ok, %{migrated: 1, failed: 0}} = SecretMigration.migrate_legacy_secrets() + %Setting{value: migrated_value} = Repo.get(Setting, "__encrypted_ai.online.api_key") + + assert {:ok, %{migrated: 0, failed: 0}} = SecretMigration.migrate_legacy_secrets() + assert %Setting{value: ^migrated_value} = Repo.get(Setting, "__encrypted_ai.online.api_key") + end + + test "leaves undecryptable rows in place and reports them" do + unknown_value = encrypt_with(:crypto.strong_rand_bytes(32), "lost-secret") + insert_setting("__encrypted_ai.online.api_key", unknown_value) + + log = + capture_log(fn -> + assert {:ok, %{migrated: 0, failed: 1}} = SecretMigration.migrate_legacy_secrets() + end) + + assert log =~ "__encrypted_ai.online.api_key" + assert %Setting{value: ^unknown_value} = Repo.get(Setting, "__encrypted_ai.online.api_key") + end + + test "ignores settings that are not encrypted secrets" do + insert_setting("ai.online.url", "https://api.example.test/v1") + + assert {:ok, %{migrated: 0, failed: 0}} = SecretMigration.migrate_legacy_secrets() + + assert %Setting{value: "https://api.example.test/v1"} = Repo.get(Setting, "ai.online.url") + end + + test "runs as part of repo bootstrap" do + legacy_value = encrypt_with(@legacy_repo_key, "sk-bootstrap-123") + insert_setting("__encrypted_ai.online.api_key", legacy_value) + + assert :ok = BDS.RepoBootstrap.ensure_ready(migrate?: false) + + %Setting{value: new_value} = Repo.get(Setting, "__encrypted_ai.online.api_key") + refute new_value == legacy_value + assert {:ok, "sk-bootstrap-123"} = SecretBackend.decrypt_with_current_key(new_value) + end +end