AkkomaGang/akkoma
AkkomaGang/akkoma
17
70
Fork 115
Code Issues 147 Pull requests 14 Projects 2 Releases 23 Packages Wiki Activity

ensure utf-8 nicknames on nickname GETs and user validator #1057

Merged
floatingghost merged 4 commits from user-utf8 into develop 2026-02-07 19:41:27 +00:00
Conversation 2 Commits 4 Files changed 3 +24
2 changed files with 24 additions and 0 deletions
Download patch file Download diff file Expand all files Collapse all files
Showing only changes of commit 0d7bbab384 - Show all commits

ensure utf-8 nicknames on nickname gets and user validator

Prev Next
FloatingGhost 2026-01-25 01:29:10 +00:00
commit 0d7bbab384

13
lib/pleroma/user.ex
View file

@ -1211,6 +1211,7 @@ defmodule Pleroma.User do
end
def get_cached_by_nickname(nickname) do
if String.valid?(nickname) do
key = "nickname:#{nickname}"
@cachex.fetch!(:user_cache, key, fn _ ->
@ -1219,9 +1220,14 @@ defmodule Pleroma.User do
{:error, _error} -> {:ignore, nil}
end
end)
else
:error
end
end
def get_cached_by_nickname_or_id(nickname_or_id, opts \\ []) do
if String.valid?(nickname_or_id) do
floatingghost marked this conversation as resolved
Oneric commented 2026-01-25 15:32:19 +00:00
Copy link

Since FlakeId can handle this and id-type arguments are presumably much more common it might be good to optimise this by only testing String.valid if there was no id match. E.g. something like:

with idlike <- is_integer(nickname_or_id) or FlakeId.flake_id?(nickname_or_id),
     nil <- if idlike, do: get_cached_by_id(nickname_or_id), else: nil,
     true <- String.valid?(nickname_or_id) do
  cond do
        restrict_to_local == false or not String.contains?(nickname_or_id, "@") ->
          get_cached_by_nickname(nickname_or_id)

        restrict_to_local == :unauthenticated and match?(%User{}, opts[:for]) ->
          get_cached_by_nickname(nickname_or_id)

        true ->
          nil
  end
else
  %User{} = user -> user
  _ -> nil
end

Though it might be nicer to split the "allowed to lookup" checks handled in the cond into a private function returning a boolean and then also handle this as just another with clause:

with idlike <- is_integer(nickname_or_id) or FlakeId.flake_id?(nickname_or_id),
     nil <- if idlike, do: get_cached_by_id(nickname_or_id), else: nil,
     true <- String.valid?(nickname_or_id),
     true <- allowed_to_lookup_nick(restrict_to_local, nickname_or_id, opts) do
  get_cached_by_nickname(nickname_or_id)
else
  %User{} = user -> user
  _ -> nil
end
Since `FlakeId` can handle this and id-type arguments are presumably much more common it might be good to optimise this by only testing `String.valid` if there was no id match. E.g. something like: ```elixir with idlike <- is_integer(nickname_or_id) or FlakeId.flake_id?(nickname_or_id), nil <- if idlike, do: get_cached_by_id(nickname_or_id), else: nil, true <- String.valid?(nickname_or_id) do cond do restrict_to_local == false or not String.contains?(nickname_or_id, "@") -> get_cached_by_nickname(nickname_or_id) restrict_to_local == :unauthenticated and match?(%User{}, opts[:for]) -> get_cached_by_nickname(nickname_or_id) true -> nil end else %User{} = user -> user _ -> nil end ``` Though it might be nicer to split the "allowed to lookup" checks handled in the `cond` into a private function returning a boolean and then also handle this as just another `with` clause: ```elixir with idlike <- is_integer(nickname_or_id) or FlakeId.flake_id?(nickname_or_id), nil <- if idlike, do: get_cached_by_id(nickname_or_id), else: nil, true <- String.valid?(nickname_or_id), true <- allowed_to_lookup_nick(restrict_to_local, nickname_or_id, opts) do get_cached_by_nickname(nickname_or_id) else %User{} = user -> user _ -> nil end ```
floatingghost commented 2026-02-07 19:10:19 +00:00
Copy link

i just removed the check in nickname_by_id and let it live in the get_cached_by_nickname which should be suitable

i just removed the check in nickname_by_id and let it live in the get_cached_by_nickname which should be suitable
restrict_to_local = Config.get([:instance, :limit_to_local_content])
cond do
@ -1237,14 +1243,21 @@ defmodule Pleroma.User do
true ->
nil
end
else
:error
end
end
@spec get_by_nickname(String.t()) :: User.t() | nil
def get_by_nickname(nickname) do
if String.valid?(nickname) do
Repo.get_by(User, nickname: nickname) ||
if Regex.match?(~r(@#{Pleroma.Web.Endpoint.host()})i, nickname) do
Repo.get_by(User, nickname: local_nickname(nickname))
end
else
:error
floatingghost marked this conversation as resolved
Oneric commented 2026-01-25 15:31:03 +00:00
Copy link

Returning :error here breaks the documented type spec. It apparently still works fine anyway for /api/v1/accounts/:nick, but might still crash for other users of this function. I don’t think it’s necessary to distinguish this from a normal "not found" response though, after all no such user exists in the db and so just returning nil seems fine.

The same applies to the other two modified functions

Returning `:error` here breaks the documented type spec. It apparently still works fine anyway for `/api/v1/accounts/:nick`, but might still crash for other users of this function. I don’t think it’s necessary to distinguish this from a normal "not found" response though, after all no such user exists in the db and so just returning `nil` seems fine. The same applies to the other two modified functions
end
end
def get_by_email(email), do: Repo.get_by(User, email: email)

11
test/pleroma/web/mastodon_api/controllers/account_controller_test.exs
View file

@ -124,6 +124,7 @@ defmodule Pleroma.Web.MastodonAPI.AccountControllerTest do
assert %{"id" => id} = json_response_and_validate_schema(conn, 200)
assert id == user.id
end
test "accounts fetches correct account for nicknames beginning with numbers", %{conn: conn} do
# Need to set an old-style integer ID to reproduce the problem
@ -177,6 +178,16 @@ defmodule Pleroma.Web.MastodonAPI.AccountControllerTest do
|> get("/api/v1/accounts/#{user.id}")
|> json_response_and_validate_schema(:not_found)
end
test "returns 404 when the username is not valid utf8" do
nick = <<0xc0>>
refute String.valid?(nick)
assert %{"error" => "Can't find user"} =
build_conn()
|> get("/api/v1/accounts/#{nick}")
|> json_response_and_validate_schema(404)
end
end
defp local_and_remote_users do