diff --git a/lib/mydia/accounts.ex b/lib/mydia/accounts.ex index 0a84145a8..a0d380ac3 100644 --- a/lib/mydia/accounts.ex +++ b/lib/mydia/accounts.ex @@ -395,12 +395,18 @@ defmodule Mydia.Accounts do @doc """ Verifies an API key and returns the associated user and API key. Returns {:error, reason} if the key is invalid, expired, or revoked. + + The candidate set is narrowed by the indexed `key_prefix` column, which is + derived from the presented key. A key whose prefix matches no row costs zero + Argon2 passes, which keeps this endpoint from amplifying CPU under a brute + force attempt. Keys predating the key_prefix column have a NULL prefix, match + nothing, and are revoked by the RevokePrefixlessApiKeys migration. """ def verify_api_key(key) when is_binary(key) do - # Find all API keys and verify against them - # This is not ideal for performance but works for small numbers of keys - # For production, consider using a more efficient lookup mechanism + prefix = extract_key_prefix(key) + ApiKey + |> where([k], k.key_prefix == ^prefix) |> preload(:user) |> Repo.all() |> Enum.find(fn api_key -> diff --git a/lib/mydia_web/schema/common_types.ex b/lib/mydia_web/schema/common_types.ex index 95180c7de..bec88c8ef 100644 --- a/lib/mydia_web/schema/common_types.ex +++ b/lib/mydia_web/schema/common_types.ex @@ -199,7 +199,7 @@ defmodule MydiaWeb.Schema.CommonTypes do object :api_key do field :id, non_null(:id), description: "API key ID" field :name, non_null(:string), description: "User-given name" - field :key_prefix, non_null(:string), description: "Key prefix for identification" + field :key_prefix, :string, description: "Key prefix for identification" field :permissions, non_null(list_of(non_null(:string))), description: "List of granted permissions" diff --git a/lib/mydia_web/schema/resolvers/remote_access_resolver.ex b/lib/mydia_web/schema/resolvers/remote_access_resolver.ex index 956e339c2..306e200e6 100644 --- a/lib/mydia_web/schema/resolvers/remote_access_resolver.ex +++ b/lib/mydia_web/schema/resolvers/remote_access_resolver.ex @@ -67,18 +67,15 @@ defmodule MydiaWeb.Schema.Resolvers.RemoteAccessResolver do end def refresh_media_token(_parent, %{token: token}, _context) do - case MediaToken.refresh_token(token) do - {:ok, new_token, claims} -> - expires_at = DateTime.from_unix!(claims["exp"]) - permissions = Map.get(claims, "permissions", []) - - {:ok, - %{ - token: new_token, - expires_at: expires_at, - permissions: permissions - }} - + with {:ok, new_token, claims} <- MediaToken.refresh_token(token), + {:ok, expires_at} <- expires_at_from_claims(claims) do + {:ok, + %{ + token: new_token, + expires_at: expires_at, + permissions: Map.get(claims, "permissions", []) + }} + else {:error, :token_expired} -> {:error, "Token has expired"} @@ -91,6 +88,9 @@ defmodule MydiaWeb.Schema.Resolvers.RemoteAccessResolver do {:error, :device_revoked} -> {:error, "Device has been revoked"} + {:error, :missing_expiry} -> + {:error, "Failed to refresh token: missing expiry"} + {:error, reason} -> {:error, "Failed to refresh token: #{inspect(reason)}"} end diff --git a/priv/repo/migrations/20260804033644_revoke_prefixless_api_keys.exs b/priv/repo/migrations/20260804033644_revoke_prefixless_api_keys.exs new file mode 100644 index 000000000..642335240 --- /dev/null +++ b/priv/repo/migrations/20260804033644_revoke_prefixless_api_keys.exs @@ -0,0 +1,22 @@ +defmodule Mydia.Repo.Migrations.RevokePrefixlessApiKeys do + use Ecto.Migration + + # API keys created before 20251226020220 added the key_prefix column have no + # prefix, and it cannot be recovered from an Argon2 hash. Verification now + # narrows by prefix, so these keys can no longer authenticate. Revoke them so + # the key's state is inspectable via the API and explicable in release notes, + # instead of the key silently failing. + def up do + now = DateTime.utc_now() |> DateTime.truncate(:second) + + execute(""" + UPDATE api_keys SET revoked_at = '#{now}' + WHERE key_prefix IS NULL AND revoked_at IS NULL + """) + end + + # Not reversible. Rolling back cannot distinguish keys this migration revoked + # from keys the operator revoked by hand, and un-revoking a key on rollback is + # the wrong default for a security action. + def down, do: :ok +end diff --git a/test/mydia/accounts/api_key_test.exs b/test/mydia/accounts/api_key_test.exs index 7f421b785..332dd0610 100644 --- a/test/mydia/accounts/api_key_test.exs +++ b/test/mydia/accounts/api_key_test.exs @@ -153,6 +153,20 @@ defmodule Mydia.Accounts.ApiKeyTest do assert {:error, :invalid_key} = Accounts.verify_api_key(plain_key) end + + test "rejects a key whose stored prefix is NULL", %{plain_key: plain_key, api_key: api_key} do + Mydia.Repo.update_all( + from(k in Mydia.Accounts.ApiKey, where: k.id == ^api_key.id), + set: [key_prefix: nil] + ) + + assert {:error, :invalid_key} = Accounts.verify_api_key(plain_key) + end + + test "rejects a well-formed key whose prefix matches no row" do + unknown = "mydia_ak_" <> String.duplicate("Z", 32) + assert {:error, :invalid_key} = Accounts.verify_api_key(unknown) + end end describe "revoke_api_key/1" do @@ -185,4 +199,52 @@ defmodule Mydia.Accounts.ApiKeyTest do assert Accounts.list_api_keys(user.id) == [] end end + + describe "prefix-less key revocation statement" do + setup do + user = AccountsFixtures.user_fixture() + %{user: user} + end + + test "revokes a prefix-less key and leaves a prefixed key alone", %{user: user} do + {:ok, legacy, _} = Accounts.create_api_key(user.id, %{name: "Legacy Key"}) + {:ok, modern, _} = Accounts.create_api_key(user.id, %{name: "Modern Key"}) + + # Simulate a row created before the key_prefix column existed. + Mydia.Repo.update_all( + from(k in Mydia.Accounts.ApiKey, where: k.id == ^legacy.id), + set: [key_prefix: nil] + ) + + now = DateTime.utc_now() |> DateTime.truncate(:second) + + Mydia.Repo.query!(""" + UPDATE api_keys SET revoked_at = '#{now}' + WHERE key_prefix IS NULL AND revoked_at IS NULL + """) + + assert Accounts.get_api_key!(legacy.id).revoked_at != nil + assert Accounts.get_api_key!(modern.id).revoked_at == nil + end + + test "does not overwrite an existing revocation timestamp", %{user: user} do + {:ok, legacy, _} = Accounts.create_api_key(user.id, %{name: "Already Revoked"}) + {:ok, revoked} = Accounts.revoke_api_key(legacy) + original_revoked_at = revoked.revoked_at + + Mydia.Repo.update_all( + from(k in Mydia.Accounts.ApiKey, where: k.id == ^legacy.id), + set: [key_prefix: nil] + ) + + later = DateTime.utc_now() |> DateTime.add(3600, :second) |> DateTime.truncate(:second) + + Mydia.Repo.query!(""" + UPDATE api_keys SET revoked_at = '#{later}' + WHERE key_prefix IS NULL AND revoked_at IS NULL + """) + + assert Accounts.get_api_key!(legacy.id).revoked_at == original_revoked_at + end + end end