From e8bc042a0b4b55437f48ef2af4253d7a830dfcfb Mon Sep 17 00:00:00 2001 From: Dominic Letz Date: Thu, 23 Jul 2026 07:29:59 +0800 Subject: [PATCH 1/3] Fix Desktop.Auth key race that can show Unauthorized Concurrent first access from prepare_url and the Auth plug could mint different keys via non-atomic persistent_term check-then-set. Elect a single winner with ETS insert_new while keeping persistent_term as the fast read path. Co-authored-by: Cursor --- lib/desktop/auth.ex | 58 +++++++++++++++++++++++++++---- test/auth_test.exs | 85 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 136 insertions(+), 7 deletions(-) create mode 100644 test/auth_test.exs diff --git a/lib/desktop/auth.ex b/lib/desktop/auth.ex index 5accb53..f3702ba 100644 --- a/lib/desktop/auth.ex +++ b/lib/desktop/auth.ex @@ -9,22 +9,66 @@ defmodule Desktop.Auth do alias Desktop.OS @behaviour Plug + @table __MODULE__ + @key {__MODULE__, :key} + defp key() do # key should stay the same during application run, # but be different on each instance - case :persistent_term.get({__MODULE__, :key}, nil) do - nil -> + case :persistent_term.get(@key, nil) do + nil -> init_key() + key -> key + end + end + + # `persistent_term` put is not atomic with get: under concurrent first access + # (e.g. Window.prepare_url/1 and this plug) two processes can mint different + # keys, leave the webview with a stale `?k=`, and serve a blank "Unauthorized". + # ETS insert_new elects a single winner; persistent_term remains the fast path. + defp init_key() do + table = table!() + + case :ets.lookup(table, :key) do + [{:key, key}] -> + store_key(key) + + [] -> key = :crypto.strong_rand_bytes(32) - :persistent_term.put({__MODULE__, :key}, key) - key - key -> - key + if :ets.insert_new(table, {:key, key}) do + store_key(key) + else + [{:key, key}] = :ets.lookup(table, :key) + store_key(key) + end + end + end + + defp store_key(key) do + :persistent_term.put(@key, key) + key + end + + defp table!() do + case :ets.whereis(@table) do + :undefined -> + try do + :ets.new(@table, [:named_table, :public, :set, read_concurrency: true]) + rescue + ArgumentError -> + # Concurrent create — the other process owns the table now. + table!() + end + + tid -> + tid end end def set_key(key) do - :persistent_term.put({__MODULE__, :key}, Base.decode32!(key)) + decoded = Base.decode32!(key) + :ets.insert(table!(), {:key, decoded}) + :persistent_term.put(@key, decoded) end def login_key() do diff --git a/test/auth_test.exs b/test/auth_test.exs new file mode 100644 index 0000000..313209c --- /dev/null +++ b/test/auth_test.exs @@ -0,0 +1,85 @@ +defmodule Desktop.AuthTest do + use ExUnit.Case, async: false + + setup do + reset_auth_key!() + :ok + end + + test "login_key/0 is idempotent" do + key1 = Desktop.Auth.login_key() + key2 = Desktop.Auth.login_key() + + assert key1 == key2 + assert byte_size(key1) > 0 + end + + test "login_key/0 returns one key under concurrent first access" do + reset_auth_key!() + parent = self() + + pids = + for _ <- 1..40 do + spawn(fn -> + send(parent, {:key, Desktop.Auth.login_key()}) + end) + end + + keys = + Enum.map(pids, fn _ -> + receive do + {:key, key} -> key + after + 5_000 -> flunk("timeout waiting for auth key") + end + end) + + assert length(Enum.uniq(keys)) == 1 + assert hd(keys) == Desktop.Auth.login_key() + end + + test "set_key/1 is visible to login_key/0 and Auth plug" do + raw = :crypto.strong_rand_bytes(32) + encoded = Base.encode32(raw) + login = Base.encode32(raw, padding: false) + + Desktop.Auth.set_key(encoded) + assert Desktop.Auth.login_key() == login + + opts = Desktop.Auth.init([]) + + conn = + Plug.Test.conn(:get, "/?k=#{login}") + |> Plug.Test.init_test_session(%{}) + |> Desktop.Auth.call(opts) + + refute conn.halted + end + + test "Auth plug rejects missing key" do + _ = Desktop.Auth.login_key() + opts = Desktop.Auth.init([]) + + conn = + Plug.Test.conn(:get, "/") + |> Plug.Test.init_test_session(%{}) + |> Desktop.Auth.call(opts) + + assert conn.halted + assert conn.status == 401 + assert conn.resp_body == "Unauthorized" + end + + defp reset_auth_key!() do + try do + :persistent_term.erase({Desktop.Auth, :key}) + rescue + ArgumentError -> :ok + end + + case :ets.whereis(Desktop.Auth) do + :undefined -> :ok + _tid -> :ets.delete_all_objects(Desktop.Auth) + end + end +end From a50101f0bf6e800a1af55d0a689dc4e1269dc727 Mon Sep 17 00:00:00 2001 From: Dominic Letz Date: Thu, 23 Jul 2026 07:37:40 +0800 Subject: [PATCH 2/3] Simplify Auth key init paths and tests Drop the redundant ETS pre-lookup before insert_new, trim defensive test reset code, and keep persistent_term as the hot-path cache. Co-authored-by: Cursor --- lib/desktop/auth.ex | 31 +++++++++++-------------------- test/auth_test.exs | 22 ++++++++-------------- 2 files changed, 19 insertions(+), 34 deletions(-) diff --git a/lib/desktop/auth.ex b/lib/desktop/auth.ex index f3702ba..0760ee6 100644 --- a/lib/desktop/auth.ex +++ b/lib/desktop/auth.ex @@ -21,26 +21,19 @@ defmodule Desktop.Auth do end end - # `persistent_term` put is not atomic with get: under concurrent first access - # (e.g. Window.prepare_url/1 and this plug) two processes can mint different - # keys, leave the webview with a stale `?k=`, and serve a blank "Unauthorized". - # ETS insert_new elects a single winner; persistent_term remains the fast path. + # `persistent_term` get/put is not atomic: concurrent first access from + # Window.prepare_url/1 and this plug can mint different keys and leave the + # webview on a blank "Unauthorized" page. ETS insert_new elects one winner; + # persistent_term remains the fast read path. defp init_key() do table = table!() + key = :crypto.strong_rand_bytes(32) - case :ets.lookup(table, :key) do - [{:key, key}] -> - store_key(key) - - [] -> - key = :crypto.strong_rand_bytes(32) - - if :ets.insert_new(table, {:key, key}) do - store_key(key) - else - [{:key, key}] = :ets.lookup(table, :key) - store_key(key) - end + if :ets.insert_new(table, {:key, key}) do + store_key(key) + else + [{:key, key}] = :ets.lookup(table, :key) + store_key(key) end end @@ -55,9 +48,7 @@ defmodule Desktop.Auth do try do :ets.new(@table, [:named_table, :public, :set, read_concurrency: true]) rescue - ArgumentError -> - # Concurrent create — the other process owns the table now. - table!() + ArgumentError -> table!() end tid -> diff --git a/test/auth_test.exs b/test/auth_test.exs index 313209c..1b3c350 100644 --- a/test/auth_test.exs +++ b/test/auth_test.exs @@ -15,24 +15,22 @@ defmodule Desktop.AuthTest do end test "login_key/0 returns one key under concurrent first access" do - reset_auth_key!() parent = self() - pids = - for _ <- 1..40 do - spawn(fn -> - send(parent, {:key, Desktop.Auth.login_key()}) - end) - end + for _ <- 1..40 do + spawn(fn -> + send(parent, {:key, Desktop.Auth.login_key()}) + end) + end keys = - Enum.map(pids, fn _ -> + for _ <- 1..40 do receive do {:key, key} -> key after 5_000 -> flunk("timeout waiting for auth key") end - end) + end assert length(Enum.uniq(keys)) == 1 assert hd(keys) == Desktop.Auth.login_key() @@ -71,11 +69,7 @@ defmodule Desktop.AuthTest do end defp reset_auth_key!() do - try do - :persistent_term.erase({Desktop.Auth, :key}) - rescue - ArgumentError -> :ok - end + :persistent_term.erase({Desktop.Auth, :key}) case :ets.whereis(Desktop.Auth) do :undefined -> :ok From f8b234d10ae59c69e8465a9d3f9920ec80bfc674 Mon Sep 17 00:00:00 2001 From: Dominic Letz Date: Thu, 23 Jul 2026 07:38:37 +0800 Subject: [PATCH 3/3] Log a warning when Desktop.Auth rejects a request Mirror the ddrive diagnostics: path, whether a k was present, and peer, so stale-key Unauthorized pages are visible in logs. Co-authored-by: Cursor --- lib/desktop/auth.ex | 10 +++++++++- test/auth_test.exs | 45 +++++++++++++++++++++++++++++++++++++-------- 2 files changed, 46 insertions(+), 9 deletions(-) diff --git a/lib/desktop/auth.ex b/lib/desktop/auth.ex index 0760ee6..79a22b9 100644 --- a/lib/desktop/auth.ex +++ b/lib/desktop/auth.ex @@ -7,6 +7,7 @@ defmodule Desktop.Auth do import Plug.Conn alias Desktop.OS + require Logger @behaviour Plug @table __MODULE__ @@ -79,10 +80,17 @@ defmodule Desktop.Auth do defp require_auth(conn) do conn = fetch_query_params(conn) + k = conn.query_params["k"] || "" - if OS.mobile?() or Plug.Crypto.secure_compare(login_key(), conn.query_params["k"] || "") do + if OS.mobile?() or Plug.Crypto.secure_compare(login_key(), k) do put_session(conn, :user, true) else + has_k? = k != "" + + Logger.warning( + "Desktop.Auth Unauthorized path=#{conn.request_path} has_k=#{has_k?} peer=#{inspect(conn.remote_ip)}" + ) + conn |> resp(401, "Unauthorized") |> halt() diff --git a/test/auth_test.exs b/test/auth_test.exs index 1b3c350..5c4ad45 100644 --- a/test/auth_test.exs +++ b/test/auth_test.exs @@ -54,18 +54,47 @@ defmodule Desktop.AuthTest do refute conn.halted end - test "Auth plug rejects missing key" do + test "Auth plug rejects missing key and logs a warning" do + import ExUnit.CaptureLog + _ = Desktop.Auth.login_key() opts = Desktop.Auth.init([]) - conn = - Plug.Test.conn(:get, "/") - |> Plug.Test.init_test_session(%{}) - |> Desktop.Auth.call(opts) + log = + capture_log(fn -> + conn = + Plug.Test.conn(:get, "/") + |> Plug.Test.init_test_session(%{}) + |> Desktop.Auth.call(opts) + + assert conn.halted + assert conn.status == 401 + assert conn.resp_body == "Unauthorized" + end) + + assert log =~ "Desktop.Auth Unauthorized" + assert log =~ "has_k=false" + end + + test "Auth plug rejects wrong key and logs has_k=true" do + import ExUnit.CaptureLog + + _ = Desktop.Auth.login_key() + opts = Desktop.Auth.init([]) + + log = + capture_log(fn -> + conn = + Plug.Test.conn(:get, "/dashboard?k=WRONG") + |> Plug.Test.init_test_session(%{}) + |> Desktop.Auth.call(opts) + + assert conn.halted + assert conn.status == 401 + end) - assert conn.halted - assert conn.status == 401 - assert conn.resp_body == "Unauthorized" + assert log =~ "Desktop.Auth Unauthorized path=/dashboard" + assert log =~ "has_k=true" end defp reset_auth_key!() do