Extract mumble_protocol library and refactor server architecture #7
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughRestructures the project into a Rebar3 multi-app layout: adds a mumble_protocol and ocb128_crypto app with protocol, client/server, crypto, cert, and tests; introduces new erlmur application modules and supervisors; removes legacy monolithic erlmur implementation and many src/test files. Changes
Sequence Diagram(s)sequenceDiagram
participant App as erlmur_app
participant Sup as erlmur_sup
participant UserMgr as erlmur_user_manager
participant Server as mumble_server
App->>App: start/2 (application callback)
App->>Server: start_server(opts)
Server->>Sup: start_link(Options)
Sup->>UserMgr: start_link()
UserMgr-->>Sup: {ok, Pid}
Sup-->>Server: supervisor started
Server-->>App: {ok, {mumble_server, SupPid, ListenerRef}}
sequenceDiagram
participant Client as Mumble Client
participant Conn as mumble_client_conn
participant ServerConn as mumble_server_conn
participant Handler as erlmur_server_handler
participant UserMgr as erlmur_user_manager
Client->>Conn: start_link(Host,Port,Opts)
Conn->>ServerConn: TLS connect / send Version
ServerConn->>Handler: init(Opts)
Handler->>UserMgr: register_user(SessionId, Pid)
Handler-->>ServerConn: {ok, NewState}
ServerConn->>Conn: ServerSync
Client->>Conn: send(TextMessage)
Conn->>ServerConn: forward TextMessage
ServerConn->>Handler: handle_msg(TextMessage)
Handler->>UserMgr: broadcast_text(SessionId, Text)
sequenceDiagram
participant UDP as External UDP
participant UDPSrv as mumble_udp_server
participant UserMgr as erlmur_user_manager
participant Sess as mumble_server_conn
UDP->>UDPSrv: incoming packet
UDPSrv->>UserMgr: get_session_by_udp(IP,Port)
UserMgr-->>UDPSrv: {ok, SessionId}
UDPSrv->>Sess: udp_packet(SessionId, Bin)
Sess->>Sess: decrypt & parse (OCB-AES128)
Sess->>UserMgr: broadcast_voice(SessionId, Frames)
UserMgr->>UDP: forward voice to peers
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
coderabbitai
bot
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/erlmur/src/erlmur.app.src (1)
13-17:⚠️ Potential issue | 🟠 MajorRemove unused env configuration or update code to use it.
The
envconfig inerlmur.app.src(lines 14–15) definescert_pemandkey_pem, buterlmur_app.erllines 27–28 ignore these settings and hardcode the paths instead:CertFile = filename:join(PrivDir, "server.pem"), KeyFile = filename:join(PrivDir, "server.key"),The configuration variables are not read from the environment at all. Additionally, only
server.keyexists inpriv/;cert.pemis missing. Either remove the unused env config or refactor the code to read fromcert_pemandkey_pemas intended.apps/ocb128_crypto/test/ocb128_crypto_benchmark_SUITE.erl (1)
3-11:⚠️ Potential issue | 🔴 CriticalMissing exports for individual test case functions — CT will fail to invoke them.
all/0returns atoms likeencrypt_16b,decrypt_16b, etc., but none of these functions (lines 75–93) are exported. Common Test requires test case functions to be exported to call them.Proposed fix
-export([ all/0, groups/0, init_per_suite/1, end_per_suite/1, init_per_testcase/2, end_per_testcase/2, - run_benchmark/2 + run_benchmark/2, + encrypt_16b/1, encrypt_32b/1, encrypt_128b/1, + encrypt_1kb/1, encrypt_4kb/1, encrypt_16kb/1, + encrypt_128kb/1, encrypt_1mb/1, encrypt_4mb/1, + decrypt_16b/1, decrypt_32b/1, decrypt_128b/1, + decrypt_1kb/1, decrypt_4kb/1, decrypt_16kb/1, + decrypt_128kb/1, decrypt_1mb/1, decrypt_4mb/1 ]).
🤖 Fix all issues with AI agents
In `@apps/erlmur/priv/server.key`:
- Around line 1-28: A plaintext private key (apps/erlmur/priv/server.key) was
committed—remove it from the repo and history, stop tracking new .key files, and
replace with a secure provisioning flow: remove the file and purge it from Git
history (e.g., use git filter-repo or equivalent to delete
apps/erlmur/priv/server.key and its blobs), add a rule to .gitignore to cover
*.key so future keys are not committed, and update build/deploy to generate or
inject keys from secure sources/env vars (or reference secrets manager) instead
of bundling server.key in source control.
In `@apps/erlmur/src/erlmur_app.erl`:
- Around line 32-36: The start/2 function currently returns {ok, self()} and
starts ServerRef outside the supervision tree; change it to call
erlmur_sup:start_link() and capture its return ({ok, SupPid} or {ok, SupPid,
Info}), return {ok, SupPid} instead of self(), and remove the ad-hoc start of
ServerRef from erlmur_app:start/2; instead, add the Mumble server as a
supervised child in erlmur_sup (or ensure erlmur_sup starts ServerRef during
init/1) so the supervisor owns and can restart ServerRef on crash (adjust
erlmur_sup child_spec and startup logic accordingly).
In `@apps/mumble_protocol/src/mumble_client_conn.erl`:
- Around line 113-129: The authenticating/3 clause currently sends a hardcoded
username <<"TestUser">> instead of using credentials supplied to start_link/3;
update the code to store the passed-in Opts into the connection State (e.g. add
an opts field on the state record when initializing in init/start_link), then in
authenticating/3 read the username (and optional password/token) from State
(e.g. State#state.opts.username or similar) and use that value when building
AuthMsg in place of <<"TestUser">>; ensure start_link/3 and init thread the Opts
into State so authenticating/3 can access them.
In `@apps/mumble_protocol/src/mumble_server_conn.erl`:
- Around line 339-354: The handler sets Caps = #{} when Mod does not export
get_caps/1 which then crashes version_enc(Caps) because version_enc/1 only
matches #{major := Major, minor := Minor, patch := Patch}; fix by ensuring
version_enc always receives a map with defaults or by adding a fallback clause
in version_enc/1: either change handle_protocol_msg (where Caps is set) to
supply default keys (major, minor, patch) when get_caps/1 is absent, or add a
new version_enc(Caps) clause that reads missing keys via maps:get/3 (providing
sane defaults) so a function_clause error cannot occur; reference functions:
handle_protocol_msg, get_caps/1, version_enc/1 and the Caps variable when making
the change.
- Around line 315-329: process_messages currently only matches {ok, _} and
{transition, _, _} from handle_protocol_msg, so a {stop, Reason, NewStateData}
return (from handle_protocol_msg) will cause a case_clause crash; update
process_messages to handle the {stop, Reason, NewStateData} tuple (e.g., add a
branch in the case that returns or propagates {stop, Reason, NewStateData}
instead of falling through), and ensure process_messages_in_new_state and/or the
caller of process_messages can accept and handle that {stop, ...} result;
reference: process_messages/3, handle_protocol_msg/2,
process_messages_in_new_state/3.
In `@apps/mumble_protocol/src/mumble_server.erl`:
- Around line 58-73: The current sequence calls ranch:stop_listener then
attempts to terminate children and uses exit(SupPid, normal) which supervisors
ignore; instead stop the supervisor atomically using the supervisor API (e.g.
call supervisor:stop(SupPid) or exit(SupPid, shutdown)) so the supervisor and
its children are terminated cleanly, and only then call ranch:stop_listener;
replace the exit(SupPid, normal) usage in stop_listener/1 with
supervisor:stop(SupPid) (or an explicit shutdown exit) and reorder/error-handle
so ranch:stop_listener is invoked after a successful supervisor stop (and return
appropriate {ok, ...} / {error, Reason} tuples on failures).
In `@apps/mumble_protocol/src/mumble_tcp_proto.erl`:
- Around line 110-126: The unpack/2 function (unpack) lacks a clause for when
the input binary is non-empty but too short for a full header/payload, causing
function_clause crashes on partial TCP frames; add a trailing clause like
unpack(PartialBin, Acc) -> {lists:reverse(Acc), PartialBin} so callers can
buffer leftover bytes, update callers to accept the new return shape ({Messages,
RestBinary}) or alternatively log+discard if you intentionally want to drop
partial frames; ensure references to unpack/2 in surrounding code are updated to
handle the tuple return and to prepend RestBinary to the next received chunk.
In `@apps/mumble_protocol/src/mumble_udp_server.erl`:
- Around line 102-140: The handler passes the 8-bit-matched Data directly to
'MumbleUDP_gpb':decode_msg but the protocol uses a 3-bit type + 5-bit target
header; update the handle_info clause to strip the 3+5 bit header before
decoding and reattach the same header (<<1:3,0:5>>) to the encoded response
before gen_udp:send. Specifically, change the pattern matching on Data in the
handle_info clause to extract the protobuf payload (e.g., match the 3-bit type
and 5-bit target then Payload) before calling
'MumbleUDP_gpb':decode_msg(Payload, 'Ping') and when sending use
gen_udp:send(Socket, IP, PortNo, <<1:3,0:5, EncodedResponse/binary>>) so the
logic in handle_info mirrors mumble_udp_proto:handle_ping/3.
In `@apps/mumble_protocol/test/e2e_connection_SUITE.erl`:
- Around line 13-31: The tests generate certs into ?config(priv_dir, Config) and
set application env vars in init_per_suite, but erlmur_app:start/2 currently
ignores those and hardcodes CertFile = filename:join(code:priv_dir(erlmur),
"server.pem"); update erlmur_app:start/2 to first attempt to read certificate
paths via application:get_env(erlmur, cert_pem) and application:get_env(erlmur,
key_pem) (use those when set and readable), and only fall back to
filename:join(code:priv_dir(erlmur), "server.pem") / "server.key" when env
values are absent or files missing; additionally, in init_per_suite replace the
raw os:cmd("openssl ...") call with a checked invocation (capture exit/status
and fail the test early or ct:fail if openssl fails) so missing openssl is
detected. Ensure file existence checks are performed before starting erl mur in
erlmur_app:start/2.
In `@apps/mumble_protocol/test/mock_mumble_handler.erl`:
- Around line 1-41: The module lacks an -export directive so its callbacks are
not callable; add an export list that exports start_link/1, stop/1, init/1,
handle_msg/2, get_caps/1, and authenticate/2 (matching the functions
implemented) so the behaviour callbacks (e.g., calls to
mock_mumble_handler:init/1, start_link/1, handle_msg/2, get_caps/1,
authenticate/2) are available to other modules like mumble_server_conn.
In `@apps/mumble_protocol/test/mock_transport.erl`:
- Line 156: The messages/0 function returns the wrong tuple shape; update
messages/0 to return a flat 3-tuple of atoms as Ranch expects (the message tag
triple), e.g., change the current nested {ok, {ssl, ssl_closed}} to the correct
{ssl, ssl_closed, ssl_error} so that the messages/0 function provides the
expected {Tag, Closed, Error} tuple for ranch_transport.
In `@apps/mumble_protocol/test/prop_protocol_tests.erl`:
- Around line 1-3: The module prop_protocol_tests is missing an export directive
so PropEr can't discover the properties; add an -export([...]) clause that
exports prop_varint_roundtrip/0 and prop_tcp_proto_roundtrip/0 (e.g.
-export([prop_varint_roundtrip/0, prop_tcp_proto_roundtrip/0]).) Place this
export in the prop_protocol_tests module alongside the existing -module and
-include_lib directives so the test runner can find and execute those property
functions.
🟠 Major comments (23)
justfile-41-41 (1)
41-41:⚠️ Potential issue | 🟠 Major
find -name "doc"is overly broad and may delete source documentation.This
findsearches the entire project directory for any directory nameddoc, which could delete source documentation (e.g.,apps/mumble_protocol/docs/wouldn't match, but any directory literally nameddocwould). Consider scoping to generated output only:Proposed fix — scope to _build
- find {{justfile_directory()}} -name "doc" -exec rm -rf {} + + find {{justfile_directory()}}/_build -name "doc" -exec rm -rf {} +apps/mumble_protocol/test/prop_protocol_tests.erl-4-8 (1)
4-8:⚠️ Potential issue | 🟠 MajorUnbounded
integer()generator will produce values that overflow the 64-bit varint encoding.PropEr's
integer()generator can produce arbitrarily large integers. The encoder's final clause packs into 64 bits (Num:64), silently truncating values outside that range and breaking the roundtrip. Constrain the generator to the representable range.Proposed fix
prop_varint_roundtrip() -> - ?FORALL(V, integer(), + ?FORALL(V, integer(-16#8000000000000000, 16#7FFFFFFFFFFFFFFF), begin {V, <<>>} == mumble_varint:decode(mumble_varint:encode(V)) end).apps/mumble_protocol/src/mumble_client_conn.erl-151-153 (1)
151-153:⚠️ Potential issue | 🟠 Major
ssl:send/2return value is ignored throughout the module.
ssl:send/2can return{error, Reason}(e.g., on closed socket), but the return value is discarded on lines 124, 127, 152, 174, and 215. At minimum, log errors or propagate them to avoid silent data loss.apps/mumble_protocol/src/mumble_cert.erl-101-122 (1)
101-122:⚠️ Potential issue | 🟠 MajorShell command injection risk via unsanitized inputs in
io_lib:format.
Subject,KeyFile, andCertFileare interpolated directly into a shell command string passed toos:cmd/1. If any of these contain shell metacharacters (e.g., a Subject like"/CN=foo'; rm -rf /; echo '"), arbitrary commands can be executed.Consider using
open_port({spawn_executable, ...}, [{args, [...]}])instead ofos:cmd/1to avoid shell interpretation entirely, or at minimum sanitize/validate inputs.🔒 Proposed fix using open_port for safe argument passing
generate_self_signed_cert(CertFile, KeyFile, Subject, Days) -> case ensure_cert_dir(CertFile) of ok -> - Cmd = io_lib:format( - "openssl req -x509 -newkey rsa:2048 -keyout ~s -out ~s -days ~B -nodes -subj '~s' 2>&1", - [KeyFile, CertFile, Days, Subject] - ), - - _Output = os:cmd(Cmd), + OpenSSL = os:find_executable("openssl"), + DaysStr = integer_to_list(Days), + Port = open_port({spawn_executable, OpenSSL}, [ + {args, ["req", "-x509", "-newkey", "rsa:2048", + "-keyout", KeyFile, "-out", CertFile, + "-days", DaysStr, "-nodes", "-subj", Subject]}, + exit_status, stderr_to_stdout, binary + ]), + collect_port_output(Port), case {filelib:is_file(CertFile), filelib:is_file(KeyFile)} ofYou'd also need a helper to drain the port:
collect_port_output(Port) -> receive {Port, {exit_status, _Status}} -> ok; {Port, {data, _Data}} -> collect_port_output(Port) after 30000 -> port_close(Port), {error, timeout} end.apps/mumble_protocol/test/mumble_cert_tests.erl-56-202 (1)
56-202:⚠️ Potential issue | 🟠 MajorTests exercise the local helper, not the production
mumble_certmodule.All certificate tests call the local
generate_cert/2,4helper (which shells out toopenssldirectly) rather than the production functionsmumble_cert:generate_self_signed_cert/4,mumble_cert:validate_cert_files/2, ormumble_cert:ensure_auto_certs/2. This means the actual certificate management code has no unit test coverage from this file.Consider adding tests that exercise the production module's API directly.
apps/mumble_protocol/test/mumble_client_api_tests.erl-7-11 (1)
7-11:⚠️ Potential issue | 🟠 MajorFive test functions are defined but not registered in the test generator — they will never run.
Only
client_start_invalid_certsandclient_invalid_refare listed. The following functions are dead test code:client_start_no_certs,client_send_message,client_send_voice,client_get_state, andclient_stop. If these are intentionally excluded (e.g., they require a running server), consider adding a comment explaining why, or wire them up with appropriate fixtures.apps/ocb128_crypto/test/prop_ocb128_crypto.erl-91-95 (1)
91-95:⚠️ Potential issue | 🟠 MajorLine 94:
=:=comparison doesn't assert — theloststat check silently passes on mismatch.Lines 92–93 correctly use
=(pattern match) which will crash on mismatch. Line 94 uses=:=which evaluates totrue/falsebut the result is discarded, so a wronglostcount will never be caught.Proposed fix
- (N - 1 - I) =:= Stats#crypto_stats.lost + (N - 1 - I) = Stats#crypto_stats.lostapps/mumble_protocol/src/mumble_msg.erl-217-232 (1)
217-232:⚠️ Potential issue | 🟠 MajorList comprehensions over potentially
undefinedlists will crash.
raw_to_mapforBanList,ACL(groups/acls),UserList, andVoiceTargetiterates the nested list fields with[ ... || B <- Bans]. If the protobuf field isundefined(not set), the comprehension crashes with abad_generatorerror.Guard against this by defaulting
undefinedto[]:Example fix for BanList
raw_to_map(#'BanList'{bans = Bans, query = Query}) -> #{ message_type => 'BanList', bans => [ #{ address => B#'BanList.BanEntry'.address, mask => B#'BanList.BanEntry'.mask, name => B#'BanList.BanEntry'.name, hash => B#'BanList.BanEntry'.hash, reason => B#'BanList.BanEntry'.reason, start => B#'BanList.BanEntry'.start, duration => B#'BanList.BanEntry'.duration } - || B <- Bans + || B <- (case Bans of undefined -> []; _ -> Bans end) ], query => Query };Apply the same pattern for
ACL(groups, acls),UserList(users), andVoiceTarget(targets).Also applies to: 266-302, 340-367
apps/mumble_protocol/src/mumble_msg.erl-38-39 (1)
38-39:⚠️ Potential issue | 🟠 MajorNo catch-all clause — unknown message types crash with
function_clause.Both
raw_to_map/1andfrom_map/1will raisefunction_clauseon any unrecognized record or map. If the protobuf schema adds a new message type, or an unknown record arrives from the wire, the entire connection process crashes.Consider adding a catch-all that returns an
{error, {unknown_message, ...}}tuple or at least a tagged unknown map, so callers can handle it gracefully.Also applies to: 499-499
apps/mumble_protocol/src/mumble_server_sup.erl-57-79 (1)
57-79: 🛠️ Refactor suggestion | 🟠 MajorNo validation that
Port,CertFile, andKeyFileare present and non-undefined.If any required option is missing,
proplists:get_valuereturnsundefinedand the children will crash at startup with a confusing error. Validate early ininit/1and return a clear error.Also, the log messages on lines 77–78 ("Starting UDP server…", "Starting TCP listener…") fire inside
init/1before the supervisor actually starts its children, which is misleading when reading logs.apps/mumble_protocol/src/mumble_server_sup.erl-61-61 (1)
61-61:⚠️ Potential issue | 🟠 MajorDefault
server_handlerismock_mumble_handler— a test module.If
server_handleris omitted from the options, production code silently uses a test mock handler. This should either have no default (crash early on missing config) or default to a real handler module.Proposed fix
- ServerHandler = proplists:get_value(server_handler, Options, mock_mumble_handler), + ServerHandler = case proplists:get_value(server_handler, Options) of + undefined -> error({missing_required_option, server_handler}); + Handler -> Handler + end,apps/mumble_protocol/src/mumble_udp_server.erl-141-160 (1)
141-160:⚠️ Potential issue | 🟠 MajorPositional record access with
element(3, User)is fragile and hard to read.If the
#user{}record definition inerlmur_user_manageris reordered, this will silently extract the wrong field. The record isn't available here because it's local toerlmur_user_manager.erlrather than in a shared header.Consider either:
- Moving the
#user{}record to a shared.hrlfile and using record syntax, or- Exposing an accessor like
erlmur_user_manager:user_pid(User), or- Having
get_all_users/0return a list of maps/tuples with named keys.apps/erlmur/src/erlmur_server_handler.erl-61-68 (1)
61-68:⚠️ Potential issue | 🟠 MajorGranting all permissions (
0xFFFFFFFF) is a security risk even for an MVP.Any connected client will have every permission including destructive ones (e.g., banning users, deleting channels). Consider starting with a sane default (e.g., speak + text + traverse) and expanding as features are implemented, to avoid accidental misuse during testing or early deployment.
apps/erlmur/src/erlmur_user_manager.erl-52-54 (1)
52-54: 🛠️ Refactor suggestion | 🟠 Major
get_all_users/0exposes an internal#user{}record to callers.The
#user{}record is defined locally in this module, so callers must use positional access (e.g.,element(3, User)inmumble_udp_server). This is fragile and a maintenance hazard.Consider returning a list of maps or moving the record to a shared
.hrlfile.apps/erlmur/src/erlmur_user_manager.erl-143-152 (1)
143-152:⚠️ Potential issue | 🟠 Major
register_udp_addrdoes not remove the previous address fromudp_map.If a user's UDP address changes (e.g., NAT rebinding), the old
{Addr, Pid}entry remains inudp_map. Subsequent lookups for the stale address will return the wrong pid, and the stale entry is never cleaned up.Proposed fix — remove old addr before inserting new one
handle_cast({register_udp_addr, SessionId, Addr}, State = `#state`{users = Users, udp_map = UdpMap}) -> case ets:lookup(Users, SessionId) of - [User] -> + [User = `#user`{udp_addr = OldAddr}] -> + case OldAddr of + undefined -> ok; + _ -> ets:delete(UdpMap, OldAddr) + end, ets:insert(Users, User#user{udp_addr = Addr}), ets:insert(UdpMap, {Addr, User#user.pid}), logger:debug("Registered UDP addr ~p for session ~p", [Addr, SessionId]);apps/mumble_protocol/src/mumble_server.erl-38-38 (1)
38-38:⚠️ Potential issue | 🟠 MajorDefault
server_handlerismock_mumble_handler— a test module.If a caller omits
server_handlerfrom the options map, the server silently starts with a mock/test handler. This should either be the production handler (erlmur_server_handler) or have no default so the caller is forced to specify one.Proposed fix
- ServerHandler = maps:get(server_handler, Options, mock_mumble_handler), + case maps:find(server_handler, Options) of + {ok, ServerHandler} -> ok; + error -> error(missing_server_handler) + end,apps/mumble_protocol/src/mumble_udp_proto.erl-34-41 (1)
34-41:⚠️ Potential issue | 🟠 MajorMalformed UDP packets will crash the caller with
badmatch/function_clause.UDP input is untrusted. Several paths crash on malformed data:
- Line 69:
<<T:64>> = Timestampcrashes if the remaining bits ≠ 64.- Line 61:
<<V:Len/binary, R1/binary>> = R0crashes ifR0is shorter thanLen.split_voice_positional_speex_celthas no fallback clause for truncated frames.Since
handle/2is called frommumble_server_conn, an unhandled crash will propagate up. Consider wrapping the body in atry/catchor adding guard clauses to avoid crashing on invalid input.Proposed fix — wrap in try/catch at the entry point
-spec handle(`#session`{}, binary()) -> ok. -handle(Session, <<1:3, Timestamp/bits>>) -> - handle_ping(Session, Timestamp, false); -handle(Session, <<Type:3, Target:5, Rest/binary>>) -> +handle(Session, <<1:3, Timestamp/bits>>) -> + try + handle_ping(Session, Timestamp, false) + catch + _:Reason -> + logger:warning("[mumble_udp_proto] Malformed ping packet: ~p", [Reason]), + ok + end; +handle(Session, <<Type:3, Target:5, Rest/binary>>) -> + try logger:debug("DataMsg~nType ~p~nTarget ~p", [Type, Target]), {Counter, R} = mumble_varint:decode(Rest), {Voice, Positional} = split_voice_positional(Type, R), mumble_server_conn:voice_data(Session#session.session_pid, - {voice_data, Type, Target, Counter, Voice, Positional}). + {voice_data, Type, Target, Counter, Voice, Positional}) + catch + _:Reason -> + logger:warning("[mumble_udp_proto] Malformed voice packet (type ~p): ~p", [Type, Reason]), + ok + end.apps/mumble_protocol/test/mumble_SUITE.erl-353-378 (1)
353-378:⚠️ Potential issue | 🟠 Major
auto_cert_basic_testfails in CI — address pipeline failure.The pipeline reports
auto_cert_basic_test failed: cert_generation_failed, "Cert file not created". This meansmumble_cert:ensure_auto_certs/2is failing in the CI environment, likely because OpenSSL isn't available there or thepriv_dirdoesn't exist at runtime. Consider:
- Adding a precondition check (skip test if OpenSSL unavailable).
- Ensuring the
privdirectory is created before cert generation.- Using an Erlang-native cert generation approach (e.g.,
public_keymodule) to avoid the OpenSSL CLI dependency.apps/mumble_protocol/src/mumble.erl-130-130 (1)
130-130:⚠️ Potential issue | 🟠 MajorProduction API defaults to
mock_mumble_handler— test module should not be a production default.
mock_mumble_handleris a test-only module. Using it as the defaultserver_handlerin the publicstart_server/1API means any caller that omits the option will get mock behavior. This also applies tostart_server/2andstart_server/3(lines 168, 178) which hard-code the same default.Either require the handler to be explicitly provided (no default) or use a real default handler module.
apps/mumble_protocol/test/mock_transport.erl-176-185 (1)
176-185:⚠️ Potential issue | 🟠 MajorBlocking
receiveinsidehandle_calldeadlocks the gen_server.The
recvhandler (line 178-185) blocks the gen_server process in areceiveloop when the message queue is empty. No other calls (send,setopts,get_sent_messages, etc.) can be processed during this wait. For a mock that needs concurrent inject + recv, consider returning{error, empty}immediately or using a continuation-based approach.apps/mumble_protocol/test/mumble_SUITE.erl-238-240 (1)
238-240:⚠️ Potential issue | 🟠 MajorAccessing UDP server state by
element(3, State)is extremely brittle.If the
#state{}record inmumble_udp_serverchanges (field added, reordered, etc.), this silently reads the wrong field. Usesys:get_state/1with pattern matching on the record, or add a public API function tomumble_udp_serverthat returns the port.Recommended approach
Add a
get_port/1orport/0API tomumble_udp_serverand call it here instead of reaching into internal state:- State = sys:get_state(UDPPid), - Port = element(3, State), %% port is the 3rd field in `#state`{} + Port = mumble_udp_server:get_port(UDPPid),apps/mumble_protocol/src/mumble_server_conn.erl-191-195 (1)
191-195:⚠️ Potential issue | 🟠 MajorCatch-all in
authenticatingstops the process on any unhandled event.A stray or unexpected message (e.g., a late timer, system message, or monitor notification) will
{stop, unhandled}the connection. Contrast with theestablishedcatch-all (line 284-286) which useskeep_state_and_data. Consider makingauthenticatingequally resilient.Proposed fix
authenticating(Type, Msg, StateData) -> logger:warning("State: authenticating~nUnhandled ~p~n~p~n~p", [ Type, Msg, StateData ]), - {stop, unhandled}. + {keep_state_and_data, ?TIMEOUT}.apps/mumble_protocol/include/mumble_protocol.hrl-1-3 (1)
1-3:⚠️ Potential issue | 🟠 MajorMissing include guard — re-inclusion will cause "record already defined" errors.
This header defines records and macros but has no
-ifndef/-define/-endifguard. If it's transitively included more than once (e.g., from multiple headers), compilation will fail.Proposed fix
+-ifndef(MUMBLE_PROTOCOL_HRL). +-define(MUMBLE_PROTOCOL_HRL, true). + -define(MUMBLE_PROTOCOL_VERSION_MAJOR, 1). -define(MUMBLE_PROTOCOL_VERSION_MINOR, 2). -define(MUMBLE_PROTOCOL_VERSION_PATCH, 4). ... %% rest of the file + +-endif.
🟡 Minor comments (29)
AGENTS.md-63-67 (1)
63-67:⚠️ Potential issue | 🟡 MinorIncorrect Erlang syntax in error handling guideline.
Line 65 states
try...of...else, but Erlang'stryexpression does not have anelseclause. The correct form for cleanup istry Expr of Pattern -> Body catch Class:Reason -> Handler after Cleanup -> end. Consider correcting totry...of...catch...after..gitignore-35-37 (1)
35-37:⚠️ Potential issue | 🟡 MinorLGTM — but consider adding
*.keyto this file as well.The new ignore entries for editor/agent directories are appropriate. Note that
*.pemis already ignored (Line 18), but*.keyis not — this allowedapps/erlmur/priv/server.keyto be committed. Adding*.keyhere would prevent future accidental commits of private key files.apps/mumble_protocol/docs/establishing_connection.md-99-139 (1)
99-139:⚠️ Potential issue | 🟡 MinorDiagram section numbering is inconsistent with the document headings.
The document defines five phases numbered 1–5 (Transport → Initial Exchange → Crypto → State Sync → Finalization), but the Mermaid diagram uses a different numbering: Version/Authenticate messages appear outside any numbered note, "2" is Crypto & Config, "3" is State Sync, "4" is Finalization, and "5" is Maintenance. This will confuse readers cross-referencing the text with the diagram.
Also, Line 120 says
ChannelState (removed links)— the phrase "removed links" is ambiguous. Consider "without links" or "initial channel state" to clarify that links haven't been stripped but simply aren't included yet.apps/mumble_protocol/src/mumble_varint.erl-56-59 (1)
56-59:⚠️ Potential issue | 🟡 MinorLarge positive integers (≥ 2⁶⁴) silently truncate in the 64-bit encoding.
When
Num >= 16#10000000000000000, theNum:64bit-syntax silently wraps, sodecode(encode(N))≠{N, <<>>}. Consider adding a guard or raising an error for out-of-range values.Proposed fix
-encode(Num) -> - <<2#11110100:8, Num:64>>. +encode(Num) when Num < 16#10000000000000000 -> + <<2#11110100:8, Num:64>>; +encode(Num) -> + error({varint_out_of_range, Num}).apps/mumble_protocol/src/mumble_client_conn.erl-217-224 (1)
217-224:⚠️ Potential issue | 🟡 MinorUDP send functions are stubs, but timers actively invoke them.
send_udp_ping/1andsend_udp/2only log debug messages. However, line 194 starts a repeating 5-second timer that callssend_udp_ping, and line 183 re-arms it. This creates perpetual no-op timer cycles. Consider either implementing UDP or not starting the timer until it's ready.Do you want me to open an issue to track the UDP implementation?
apps/ocb128_crypto/src/ocb128_crypto.erl-256-260 (1)
256-260:⚠️ Potential issue | 🟡 MinorAdd
minimum_otp_vsnto rebar.config to enforce OTP 25+ requirement.The project's README documents "Erlang/OTP 25 or later" as a prerequisite, but this requirement is not enforced in
rebar.config. Add{minimum_otp_vsn, "25"}to ensure the build fails early on incompatible OTP versions. Additionally, if supporting OTP 25 specifically, explicitly enable themaybe_exprfeature inerl_optssince it was experimental and disabled by default in OTP 25:{erl_opts, [ debug_info, -enable-feature maybe_expr, {i, "apps/mumble_protocol/include"}, {i, "_build/default/plugins/gpb/include"} ]}.apps/mumble_protocol/test/mumble_cert_tests.erl-138-158 (1)
138-158:⚠️ Potential issue | 🟡 MinorTest doesn't verify any reuse logic.
reuse_existing_cert_testgenerates a cert, sleeps, then reads the same file's mtime — confirming only that a file doesn't change by itself. It never calls any productionmumble_certfunction (e.g.,ensure_auto_certs/2orvalidate_cert_files/2) to verify that existing certs are actually reused rather than regenerated.apps/erlmur/src/erlmur_sup.erl-23-32 (1)
23-32:⚠️ Potential issue | 🟡 MinorMisleading log message: child hasn't started yet at line 31.
init/1only builds the child specification — the supervisor starts children afterinit/1returns. The log on line 31 ("User manager started") is inaccurate and could mislead during debugging. Line 24 is similarly premature.Proposed fix
init([]) -> - logger:info("[erlmur_sup] Starting user manager..."), + logger:info("[erlmur_sup] Initializing supervision tree"), UserManager = #{ id => erlmur_user_manager, start => {erlmur_user_manager, start_link, []}, restart => permanent, type => worker }, - logger:info("[erlmur_sup] User manager started"), {ok, {{one_for_one, 5, 10}, [UserManager]}}.apps/mumble_protocol/test/mumble_cert_tests.erl-114-117 (1)
114-117:⚠️ Potential issue | 🟡 MinorWeak assertion:
"test"matches almost any string.The fallback check
string:find(Subject, "test")on line 116 is too broad and would pass even if the subject were completely wrong. Consider matching a more specific substring like"test.example.com"or"TestOrg"only.Proposed fix
?assert( (string:find(Subject, "test.example.com") =/= nomatch) orelse - (string:find(Subject, "test") =/= nomatch) + (string:find(Subject, "TestOrg") =/= nomatch) )apps/mumble_protocol/README.md-31-37 (1)
31-37:⚠️ Potential issue | 🟡 MinorSeveral spelling errors in the documentation.
- Line 31: "recieved" → "received"
- Line 37: "reciveing" → "receiving"
- Line 45: "explicitely" → "explicitly"
Proposed fix (lines 31, 37)
-If no `Ping` is recieved for 30sec the client will be disconnected from the server. +If no `Ping` is received for 30 seconds the client will be disconnected from the server.-If reciveing a `UDPTunnel` message the connection is marked "UDP verified" false, until new UDP verification is done. +If receiving a `UDPTunnel` message the connection is marked "UDP verified" false, until new UDP verification is done.apps/mumble_protocol/rebar.config-20-28 (1)
20-28:⚠️ Potential issue | 🟡 MinorNormalize indentation on lines 26–27.
Lines 26–27 use tabs while the rest of the file uses spaces. For consistency, use spaces throughout.
apps/erlmur/src/erlmur_app.erl-18-21 (1)
18-21:⚠️ Potential issue | 🟡 Minor
list_to_atom/1on environment variable input risks atom table exhaustion.While this is a startup-only path and unlikely to be exploited in practice, using
list_to_existing_atom/1with a try/catch fallback (or a whitelist) would be safer.Proposed fix
- LevelStr -> list_to_atom(LevelStr) + LevelStr -> + try list_to_existing_atom(LevelStr) + catch error:badarg -> + logger:warning("[erlmur] Invalid log level '~s', defaulting to info", [LevelStr]), + info + endapps/mumble_protocol/test/mumble_api_tests.erl-62-74 (1)
62-74:⚠️ Potential issue | 🟡 MinorTests depend on validation order in
mumble:start_server/2— may break if file-existence checks run first.
start_server_error_cert_only_testpasses"/tmp/cert.pem"(which may or may not exist) withundefinedas the key file, expecting{error, cert_key_mismatch}. If the production code checks file existence before the undefined-pairing check, this will return{error, {cert_file_not_found, ...}}when/tmp/cert.pemdoesn't exist, or succeed past file validation when it does. Same issue forstart_server_error_key_only_test.Use clearly non-existent but deterministic paths, or generate temp files in the test, to avoid environment-dependent behavior.
apps/mumble_protocol/test/e2e_connection_SUITE.erl-160-165 (1)
160-165:⚠️ Potential issue | 🟡 MinorFragile: accessing record fields by positional index with
element/7.If the
-record(state, ...)definition inmumble_client_connchanges (field added, removed, or reordered),element(7, StateData)will silently read the wrong field or crash. The comment on lines 163-164 documenting the assumed layout is helpful but doesn't prevent breakage.Consider exporting a helper or using a test-only header to include the record definition, or querying the state through a public API (e.g., a dedicated
is_udp_verified/1function).apps/mumble_protocol/test/e2e_connection_SUITE.erl-71-88 (1)
71-88:⚠️ Potential issue | 🟡 MinorRecursive
wait_for_*helpers can loop indefinitely on a stream of unexpected messages.
wait_for_ping/1,wait_for_text/1, andwait_for_udp_tunnel_voice/2recurse when they receive an unexpected{mumble_msg, _}. Each recursive call resets theaftertimeout, so a steady stream of non-matching messages would keep these spinning without ever timing out.Pass a deadline (e.g.,
erlang:monotonic_time(millisecond) + Timeout) and compute the remaining time on each recursion, or use a monotonic deadline andmax(0, Deadline - Now)as theaftervalue.Example fix for wait_for_ping
-wait_for_ping(Timestamp) -> +wait_for_ping(Timestamp) -> + wait_for_ping(Timestamp, erlang:monotonic_time(millisecond) + 2000). + +wait_for_ping(Timestamp, Deadline) -> + Remaining = max(0, Deadline - erlang:monotonic_time(millisecond)), receive {mumble_msg, #{message_type := 'Ping', timestamp := TS} = M} when TS == Timestamp -> ct:pal("Received expected Ping: ~p", [M]), verify_ping_response(M), ok; {mumble_msg, #{message_type := Type}} when Type == 'Version'; Type == 'CryptSetup'; Type == 'CodecVersion'; Type == 'ServerSync' -> - wait_for_ping(Timestamp); + wait_for_ping(Timestamp, Deadline); {mumble_msg, M} -> ct:pal("Received unexpected msg while waiting for ping: ~p", [M]), - wait_for_ping(Timestamp) - after 2000 -> + wait_for_ping(Timestamp, Deadline) + after Remaining -> ct:fail(ping_timeout) end.Also applies to: 108-123, 171-197
apps/mumble_protocol/src/mumble_udp_server.erl-38-40 (1)
38-40:⚠️ Potential issue | 🟡 Minor
send/3spec is misleading —gen_server:castalways returnsok.The typespec declares
ok | {error, term()}but the implementation usesgen_server:cast, which unconditionally returnsok. The caller can never observe an error. Either change the spec to-spec send(...) -> ok.or switch togen_server:callif you need error propagation.Proposed fix
--spec send(IP :: inet:ip_address(), inet:port_number(), binary()) -> ok | {error, term()}. +-spec send(IP :: inet:ip_address(), inet:port_number(), binary()) -> ok.apps/mumble_protocol/src/mumble_server.erl-86-95 (1)
86-95:⚠️ Potential issue | 🟡 Minor
erlang:system_info(otp_release)returns a charlist, not a binary.The
#version{}record'sos_versionfield likely expects a binary (consistent with thereleaseandosfields on lines 92-93). Same issue as inerlmur_server_handler:get_caps/1.Proposed fix
- os_version = erlang:system_info(otp_release) + os_version = list_to_binary(erlang:system_info(otp_release))apps/mumble_protocol/src/mumble_version.erl-36-46 (1)
36-46:⚠️ Potential issue | 🟡 Minor
decode/1conflates V1 and V2 formats for versions wheremajor = 0andminor = 0.The heuristic "≤ 0xFFFFFFFF → V1, else → V2" fails for V2-encoded versions with
major = 0, minor = 0: e.g., version0.0.1encodes as V2 =0x0000_0000_0001_0000(= 65536), which falls into the V1 guard and is incorrectly decoded as{major=0, minor=1, patch=0}.For real Mumble versions (≥ 1.0.0) this won't trigger, but it makes the API subtly incorrect. Consider requiring the caller to specify the format explicitly:
Proposed fix
--spec decode(non_neg_integer()) -> `#version`{}. -decode(V1) when V1 >= 0, V1 =< 16#FFFFFFFF -> +-spec decode(v1 | v2, non_neg_integer()) -> `#version`{}. +decode(v1, V1) when V1 >= 0, V1 =< 16#FFFFFFFF -> <<Major:16, Minor:8, Patch:8>> = <<V1:32>>, `#version`{major = Major, minor = Minor, patch = Patch}; -decode(V2) when V2 > 16#FFFFFFFF, V2 =< 16#FFFFFFFFFFFFFFFF -> +decode(v2, V2) when V2 >= 0, V2 =< 16#FFFFFFFFFFFFFFFF -> <<Major:16, Minor:16, Patch:16, _Zero:16>> = <<V2:64>>, `#version`{major = Major, minor = Minor, patch = Patch}.apps/erlmur/src/erlmur_server_handler.erl-78-86 (1)
78-86:⚠️ Potential issue | 🟡 MinorConvert
erlang:system_info(otp_release)to binary for consistency with documented behavior.
erlang:system_info(otp_release)returns a charlist, but theget_caps()callback documentation and all other implementations in the codebase use binaries foros_version. This causes a type inconsistency that may cause encoding failures in downstream protobuf serialization.Proposed fix
- os_version => erlang:system_info(otp_release) + os_version => list_to_binary(erlang:system_info(otp_release))Note: The same issue exists in
mumble_server.erlline 94 and should be corrected there as well.apps/mumble_protocol/test/mock_transport.erl-190-201 (1)
190-201:⚠️ Potential issue | 🟡 Minor
setoptswith{active, true}sends spuriousssl_passiveand empty data messages.Sending
{ssl_passive, self()}followed by{ssl, self(), <<>>}when active mode is enabled doesn't match real SSL behavior and could confuse tested code. Consider removing these notifications or only sending them when there's actual queued data to deliver.apps/mumble_protocol/include/mumble_protocol.hrl-85-85 (1)
85-85:⚠️ Potential issue | 🟡 MinorType spec
regular | typelooks like a placeholder — the atomtypeis likely a copy-paste artifact.type = regular :: regular | typeDid you mean a different atom here, such as
temporaryor another connection type?apps/mumble_protocol/src/mumble_server_conn.erl-302-311 (1)
302-311:⚠️ Potential issue | 🟡 MinorNo clause for empty TCP data —
byte_size(Data) >= 1guard will crash on empty binary.If
{ssl, Socket, <<>>}is delivered (unlikely but possible with certain TLS implementations), the guard fails and there is no matching clause, causing afunction_clausecrash propagated to the state callback. Consider adding a clause or removing the guard (an empty decode is harmless).apps/mumble_protocol/test/mumble_SUITE.erl-77-96 (1)
77-96:⚠️ Potential issue | 🟡 MinorOpenSSL availability check and error detection are fragile.
os:cmd("which openssl")doesn't work on Windows and may return unexpected output on some Unix systems. The error check"Error" ++ _at line 94 only catches outputs starting with that literal prefix — OpenSSL errors on different platforms may have different formats. Consider a more robust check, such as verifying the exit code or checking for the generated file's existence immediately after.apps/mumble_protocol/test/mumble_SUITE.erl-566-571 (1)
566-571:⚠️ Potential issue | 🟡 Minor
cleanup_auto_generated_certs/0deletes all.pem/.keyfiles inpriv_dir.The wildcard
*.pemand*.keywill delete any PEM/key file, not just auto-generated ones. Ifpriv_dircontains other certificate files, they would be inadvertently removed. Consider targeting specific filenames (e.g.,auto_server.pem,auto_server.key).Proposed fix
cleanup_auto_generated_certs() -> - PrivDir = code:priv_dir(mumble_protocol), - Files = filelib:wildcard(filename:join(PrivDir, "*.pem")) ++ - filelib:wildcard(filename:join(PrivDir, "*.key")), - [file:delete(F) || F <- Files], - ok. + case code:priv_dir(mumble_protocol) of + {error, _} -> ok; + PrivDir -> + file:delete(filename:join(PrivDir, "auto_server.pem")), + file:delete(filename:join(PrivDir, "auto_server.key")), + ok + end.apps/mumble_protocol/test/mock_transport.erl-68-70 (1)
68-70:⚠️ Potential issue | 🟡 Minor
accept/2parameter order is swapped vs.ranch_transportcallback.Ranch calls
Transport:accept(ListenSocket, Timeout), but here the arguments areaccept(_Timeout, ListenSocket)— reversed. While the mock ignores both values, this mismatch could mask bugs if the mock is ever extended to use the parameters, and doesn't match the behaviour spec.Proposed fix
-accept(_Timeout, ListenSocket) -> +accept(ListenSocket, _Timeout) -> %% Mock implementation {ok, {mock_accepted, ListenSocket}}.apps/mumble_protocol/src/mumble_server_conn.erl-498-499 (1)
498-499:⚠️ Potential issue | 🟡 Minor
maybe_undefined(0)converts legitimate zero counts toundefinedin Ping responses.A client with 0 late or 0 lost packets is a valid state. Converting it to
undefinedwill cause these fields to be omitted from the protobuf message, which misrepresents the data. The Mumble protocol expects integer values here;undefinedtypically means "not set" / "no data", not "zero".Proposed fix — remove the conversion or only omit truly unknown values
-maybe_undefined(0) -> undefined; -maybe_undefined(V) -> V. +maybe_undefined(undefined) -> undefined; +maybe_undefined(V) -> V.apps/mumble_protocol/src/mumble_server_conn.erl-447-457 (1)
447-457:⚠️ Potential issue | 🟡 Minor
send_msg/2has no fallback clause ifmumble_tcp_proto:pack/1returns a non-binary (e.g., error tuple).The single
casearmBin when is_binary(Bin)will raisecase_clauseifpack/1returns{error, _}or any other non-binary value. Consider adding a catch-all clause to handle packing failures gracefully.Proposed fix
send_msg(`#state`{socket = Socket, transport = Transport}, Map) -> case mumble_tcp_proto:pack(Map) of Bin when is_binary(Bin) -> logger:notice("Sending ~p (~p bytes)", [maps:get(message_type, Map), byte_size(Bin)]), case Transport:send(Socket, Bin) of ok -> ok; {error, Reason} -> logger:error("Failed to send ~p: ~p", [maps:get(message_type, Map), Reason]), {error, Reason} - end + end; + Other -> + logger:error("Failed to pack ~p: ~p", [maps:get(message_type, Map), Other]), + {error, {pack_failed, Other}} end.apps/mumble_protocol/test/mock_transport.erl-223-239 (1)
223-239:⚠️ Potential issue | 🟡 MinorMessages in
{active, once}mode are both queued and delivered — can cause double processing.When
inject_messagefires with{active, once}, the data is appended tomessage_queueand sent to the owner as{ssl, self(), Data}. A subsequentrecvcall will return the same data from the queue. Real SSL active-once delivers the data and resets to passive without queuing. Consider removing from queue when delivering in active mode, or not queueing at all.Proposed fix — don't queue when delivering in active mode
handle_cast({inject_message, Data}, State = #{owner := Owner, opts := Opts}) -> - NewQueue = case State of - #{message_queue := Queue} -> Queue ++ [Data]; - _ -> [Data] - end, - %% Check if we should notify the owner (active mode) case lists:keyfind(active, 1, Opts) of {active, true} when Owner =/= undefined -> - Owner ! {ssl, self(), Data}; + Owner ! {ssl, self(), Data}, + {noreply, State}; {active, once} when Owner =/= undefined -> - Owner ! {ssl, self(), Data}; + Owner ! {ssl, self(), Data}, + %% Reset to passive after delivery + {noreply, State#{opts := lists:keyreplace(active, 1, Opts, {active, false})}}; _ -> - ok - end, - - {noreply, State#{message_queue := NewQueue}}; + NewQueue = maps:get(message_queue, State, []) ++ [Data], + {noreply, State#{message_queue := NewQueue}} + end;apps/mumble_protocol/src/mumble.erl-125-158 (1)
125-158:⚠️ Potential issue | 🟡 MinorPartial cert specification (e.g., cert without key) silently falls through to the catch-all branch.
When a user provides
cert_filebut notkey_file(or vice versa), the_ ->branch at line 149 passesundefinedfor the missing file tomumble_server:start_server/1, which will likely produce a confusing error downstream. Consider validating that both files are provided together or neither is.Proposed fix — validate pairing
case {CertFile, KeyFile, AutoCreate} of {undefined, undefined, true} -> %% Auto-generate certificates ...; + {undefined, undefined, false} -> + {error, {missing_certificates, "Either provide cert_file and key_file, or set auto_create_cert => true"}}; + {undefined, _, _} -> + {error, {missing_option, cert_file}}; + {_, undefined, _} -> + {error, {missing_option, key_file}}; _ -> %% Use provided files ... end.
apps/erlmur/priv/server.key
Outdated
| -----BEGIN PRIVATE KEY----- | ||
| MIIEvgIBADANBgkqhkiG9w0BAQEFAASCBKgwggSkAgEAAoIBAQDDy+ryTbu84Ntf | ||
| DvTfpZh/TokUxA5Y2gFIIbj6KYjrmPsh2JDSgxuBAL6iYgVazd6IK/+WPEpc7eUR | ||
| e+9T2Yr00EAsM6rCaQ/uQFBOvvR7ngPtpZ2pFbfVtoJSA9dwfUjdpyhvRiZx4rl2 | ||
| VwWamqEEuCI+xkLKTSNXFtvY5h8Yg0LLpHDT6+wTff68lrhttBevjkHvhNKe/php | ||
| Ys1ivz4OmLpVHOH6wfMWH6jErOql8I2N6vQ2GmumcGh9DzZRQc4stJU5biTFWCRp | ||
| Ez7iAZsA5PY/prRUSRezdECnFClphzK/gSegJcXFYC6pozHAAEAoR7GzQo6zndkc | ||
| c27P417fAgMBAAECggEAMoDgVuS/Fq6nVyMr/Z4gskHHMSCLFNcSupDcPkbFZwPc | ||
| TzQl0u73grR297qx5+SvUE+NCyr9AXBphDOEHYxA//nRe7VOB/yCm/Fh5ZXyyQoO | ||
| uFdeTfVoqEr93XrZMM5YI7hQwTcGhpy1OvWiX+kfvHBkgO2x3626/mVKiu18utz4 | ||
| Swap3rbPhcRv0FHMX4P5FLKe++kATl/OPDg/pHVcAlEQ2plvjMbOeMZ4XeQ2xrrq | ||
| s6ZI3NHnbvLbo/hLyAKcee7NvRYzEc0Hl9A7D2LWHAfUKgbhBP31+JUK2TAMdCiH | ||
| EeRtQX4S5zrjOiVFJnQDbU86AcD9XCf8V+ousH7vSQKBgQD+D9hNEjJiiWhyjgIZ | ||
| icQD3BhpwA7DcasUPH1BWsie+iiq+YQul8DFPiQPat/utbLktrh84pkaXRNWPnb6 | ||
| lKd/MEiGqFxY5Z6kbOFMMTg6tRPzdX+hNzp2YeO7xhvwE+5N5beTNi6iWkNEmt0X | ||
| 2HlhiPz/Ifs3VkrCnEj3dZC5dwKBgQDFSkl43galeowR78EgK0h0rRzowJm6OrBk | ||
| ZUFCsZMx1D/LNyAnLmtq1aD5zepSqSrplGuallDkHdAYaxLAUSDPK74WuxwjJfAS | ||
| lJc/WOKskgZE30r4+iK4FjkUSbqlkI1+pVJJ/Y91DfdMKMx1B4fudyaLVHfp4Sl+ | ||
| xJP60HVf2QKBgQDnAvNUCRLFZFoTp6qjHBCezddo+NizwDymFuZxsaGwZJjr9hvJ | ||
| 3fTLpNhXKH/b51119+tCqyw5u54kkoHPEbJHxVh9EMN/lbKtoc8uX6bYQuSUQCfA | ||
| KJ7BG/teNyIaJ5VNQW6trduzhk2aKRKWZyDfnhclyflKdO1fjYoPcNgauwKBgC4t | ||
| 0TzFW3DoWljocaACAZet3ZJtYo5ApoEZYkkmH3oOYFBEuvXw25vKHT/bfvmSN0Ut | ||
| RHKOtKhwggD1LxJs6u0TJA9WBKQFgUgYF77wIz6Zf3YnffkBM5c7PGkPSOzwBO6b | ||
| 68L94tJXToc5FYJfHOYoZZE/WEIaPgg6IxkWKiMhAoGBAI/KEfGc+4nOs2ZwgNxB | ||
| uC4d15nIu6ukLLe8U7M++wJsQDW+0NAY3LSNtm8J23ytUki8kGQzJjH8mL8cPTN8 | ||
| S/BqeelrOmml3WMKPw/F4FzHFXwQDTMYMjqMAhLjBNBGj6dG18LA19gmjawnQUpN | ||
| +noCfn6GIXzXpAhYLbyhoPhk | ||
| -----END PRIVATE KEY----- |
There was a problem hiding this comment.
Private key committed to the repository — security risk.
This file contains a plaintext private key checked into version control. Even if intended only for development/testing, committed secrets persist in Git history and can be inadvertently deployed to production. Gitleaks also flagged this.
Recommendations:
- Remove this file from the repository and Git history (e.g.,
git filter-repo). - Add
*.keyto.gitignore(note:*.pemis already ignored but*.keyis not). - Generate keys at build/deploy time or provide them via environment configuration.
🧰 Tools
🪛 Gitleaks (8.30.0)
[high] 1-28: Identified a Private Key, which may compromise cryptographic security and sensitive data encryption.
(private-key)
🤖 Prompt for AI Agents
In `@apps/erlmur/priv/server.key` around lines 1 - 28, A plaintext private key
(apps/erlmur/priv/server.key) was committed—remove it from the repo and history,
stop tracking new .key files, and replace with a secure provisioning flow:
remove the file and purge it from Git history (e.g., use git filter-repo or
equivalent to delete apps/erlmur/priv/server.key and its blobs), add a rule to
.gitignore to cover *.key so future keys are not committed, and update
build/deploy to generate or inject keys from secure sources/env vars (or
reference secrets manager) instead of bundling server.key in source control.
| authenticating(enter, _, State) -> | ||
| %% Send Version | ||
| V = #version{major = 1, | ||
| minor = 2, | ||
| patch = 4}, | ||
| {V1, V2} = mumble_version:encode(V), | ||
| VerMsg = | ||
| #{message_type => 'Version', | ||
| version_v1 => V1, | ||
| version_v2 => V2, | ||
| release => <<"erlmur-client">>}, | ||
| ssl:send(State#state.socket, mumble_tcp_proto:pack(VerMsg)), | ||
| %% Send Authenticate | ||
| AuthMsg = #{message_type => 'Authenticate', username => <<"TestUser">>}, | ||
| ssl:send(State#state.socket, mumble_tcp_proto:pack(AuthMsg)), | ||
| ssl:setopts(State#state.socket, [{active, once}]), | ||
| keep_state_and_data; |
There was a problem hiding this comment.
Hardcoded username <<"TestUser">> — authentication ignores user-provided credentials.
Line 126 always sends <<"TestUser">> regardless of what was passed in Opts. The username (and optionally password/tokens) should be extracted from the options map passed through start_link/3.
🐛 Proposed fix
authenticating(enter, _, State) ->
%% Send Version
V = `#version`{major = 1,
minor = 2,
patch = 4},
{V1, V2} = mumble_version:encode(V),
VerMsg =
#{message_type => 'Version',
version_v1 => V1,
version_v2 => V2,
release => <<"erlmur-client">>},
ssl:send(State#state.socket, mumble_tcp_proto:pack(VerMsg)),
%% Send Authenticate
- AuthMsg = #{message_type => 'Authenticate', username => <<"TestUser">>},
+ AuthMsg = #{message_type => 'Authenticate', username => maps:get(username, State#state.opts, <<"TestUser">>)},
ssl:send(State#state.socket, mumble_tcp_proto:pack(AuthMsg)),
ssl:setopts(State#state.socket, [{active, once}]),
keep_state_and_data;This requires storing Opts in the state record:
--record(state, {socket, transport = ssl, session_id, parent, stats = `#stats`{}, udp_verified = false, udp_timer}).
+-record(state, {socket, transport = ssl, session_id, parent, opts = #{}, stats = `#stats`{}, udp_verified = false, udp_timer}).And threading it through init:
init({Host, Port, Opts, Parent}) ->
- {ok, connecting, `#state`{parent = Parent}, {next_event, internal, {connect, Host, Port, Opts}}}.
+ {ok, connecting, `#state`{parent = Parent, opts = Opts}, {next_event, internal, {connect, Host, Port, Opts}}}.🤖 Prompt for AI Agents
In `@apps/mumble_protocol/src/mumble_client_conn.erl` around lines 113 - 129, The
authenticating/3 clause currently sends a hardcoded username <<"TestUser">>
instead of using credentials supplied to start_link/3; update the code to store
the passed-in Opts into the connection State (e.g. add an opts field on the
state record when initializing in init/start_link), then in authenticating/3
read the username (and optional password/token) from State (e.g.
State#state.opts.username or similar) and use that value when building AuthMsg
in place of <<"TestUser">>; ensure start_link/3 and init thread the Opts into
State so authenticating/3 can access them.
| process_messages([Msg | Rest], Allowed, StateData) -> | ||
| MsgName = maps:get(message_type, Msg), | ||
| IsAllowed = Allowed == all orelse lists:member(MsgName, Allowed), | ||
| if | ||
| IsAllowed -> | ||
| case handle_protocol_msg(Msg, StateData) of | ||
| {ok, NewStateData} -> | ||
| process_messages(Rest, Allowed, NewStateData); | ||
| {transition, NextState, NewStateData} -> | ||
| process_messages_in_new_state(Rest, NextState, NewStateData) | ||
| end; | ||
| true -> | ||
| logger:warning("Message ~p not allowed", [MsgName]), | ||
| process_messages(Rest, Allowed, StateData) | ||
| end. |
There was a problem hiding this comment.
{stop, ...} return from handle_protocol_msg is unhandled — will crash with case_clause.
handle_protocol_msg can return {stop, normal, StateData} (lines 407 and 444), but process_messages only matches {ok, _} and {transition, _, _}. This will raise a case_clause exception whenever authentication fails or a handler returns {stop, ...}.
🐛 Proposed fix
process_messages([Msg | Rest], Allowed, StateData) ->
MsgName = maps:get(message_type, Msg),
IsAllowed = Allowed == all orelse lists:member(MsgName, Allowed),
if
IsAllowed ->
case handle_protocol_msg(Msg, StateData) of
{ok, NewStateData} ->
process_messages(Rest, Allowed, NewStateData);
{transition, NextState, NewStateData} ->
- process_messages_in_new_state(Rest, NextState, NewStateData)
+ process_messages_in_new_state(Rest, NextState, NewStateData);
+ {stop, Reason, NewStateData} ->
+ {stop, Reason, NewStateData}
end;
true ->
logger:warning("Message ~p not allowed", [MsgName]),
process_messages(Rest, Allowed, StateData)
end.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| process_messages([Msg | Rest], Allowed, StateData) -> | |
| MsgName = maps:get(message_type, Msg), | |
| IsAllowed = Allowed == all orelse lists:member(MsgName, Allowed), | |
| if | |
| IsAllowed -> | |
| case handle_protocol_msg(Msg, StateData) of | |
| {ok, NewStateData} -> | |
| process_messages(Rest, Allowed, NewStateData); | |
| {transition, NextState, NewStateData} -> | |
| process_messages_in_new_state(Rest, NextState, NewStateData) | |
| end; | |
| true -> | |
| logger:warning("Message ~p not allowed", [MsgName]), | |
| process_messages(Rest, Allowed, StateData) | |
| end. | |
| process_messages([Msg | Rest], Allowed, StateData) -> | |
| MsgName = maps:get(message_type, Msg), | |
| IsAllowed = Allowed == all orelse lists:member(MsgName, Allowed), | |
| if | |
| IsAllowed -> | |
| case handle_protocol_msg(Msg, StateData) of | |
| {ok, NewStateData} -> | |
| process_messages(Rest, Allowed, NewStateData); | |
| {transition, NextState, NewStateData} -> | |
| process_messages_in_new_state(Rest, NextState, NewStateData); | |
| {stop, Reason, NewStateData} -> | |
| {stop, Reason, NewStateData} | |
| end; | |
| true -> | |
| logger:warning("Message ~p not allowed", [MsgName]), | |
| process_messages(Rest, Allowed, StateData) | |
| end. |
🤖 Prompt for AI Agents
In `@apps/mumble_protocol/src/mumble_server_conn.erl` around lines 315 - 329,
process_messages currently only matches {ok, _} and {transition, _, _} from
handle_protocol_msg, so a {stop, Reason, NewStateData} return (from
handle_protocol_msg) will cause a case_clause crash; update process_messages to
handle the {stop, Reason, NewStateData} tuple (e.g., add a branch in the case
that returns or propagates {stop, Reason, NewStateData} instead of falling
through), and ensure process_messages_in_new_state and/or the caller of
process_messages can accept and handle that {stop, ...} result; reference:
process_messages/3, handle_protocol_msg/2, process_messages_in_new_state/3.
| handle_protocol_msg(#{message_type := 'Version'}, StateData = #state{handler_mod = Mod}) -> | ||
| Caps = case erlang:function_exported(Mod, get_caps, 1) of | ||
| true -> Mod:get_caps(StateData#state.handler_state); | ||
| false -> #{} | ||
| end, | ||
| {V1, V2} = version_enc(Caps), | ||
| ServerVersion = #{ | ||
| message_type => 'Version', | ||
| version_v1 => V1, | ||
| version_v2 => V2, | ||
| os => maps:get(os, Caps, ~"Linux"), | ||
| release => maps:get(release, Caps, ~"1.2.4"), | ||
| os_version => maps:get(os_version, Caps, ~"1.0") | ||
| }, | ||
| send_msg(StateData, ServerVersion), | ||
| {ok, StateData}; |
There was a problem hiding this comment.
version_enc(#{}) will crash with function_clause when handler doesn't export get_caps/1.
When erlang:function_exported(Mod, get_caps, 1) returns false, Caps is set to #{}. Then version_enc(Caps) is called, but version_enc/1 (line 475) only matches #{major := Major, minor := Minor, patch := Patch} — an empty map won't match, causing a function_clause crash.
🐛 Proposed fix — add a default clause or provide default caps
+version_enc(#{major := Major, minor := Minor, patch := Patch}) ->
+ V1 = (Major bsl 16) bor (Minor bsl 8) bor Patch,
+ V2 = (Major bsl 48) bor (Minor bsl 32) bor (Patch bsl 16),
+ {V1, V2};
+version_enc(_) ->
+ Major = ?MUMBLE_PROTOCOL_VERSION_MAJOR,
+ Minor = ?MUMBLE_PROTOCOL_VERSION_MINOR,
+ Patch = ?MUMBLE_PROTOCOL_VERSION_PATCH,
+ V1 = (Major bsl 16) bor (Minor bsl 8) bor Patch,
+ V2 = (Major bsl 48) bor (Minor bsl 32) bor (Patch bsl 16),
+ {V1, V2}.
-version_enc(#{major := Major, minor := Minor, patch := Patch}) ->
- V1 = (Major bsl 16) bor (Minor bsl 8) bor Patch,
- V2 = (Major bsl 48) bor (Minor bsl 32) bor (Patch bsl 16),
- {V1, V2}.🤖 Prompt for AI Agents
In `@apps/mumble_protocol/src/mumble_server_conn.erl` around lines 339 - 354, The
handler sets Caps = #{} when Mod does not export get_caps/1 which then crashes
version_enc(Caps) because version_enc/1 only matches #{major := Major, minor :=
Minor, patch := Patch}; fix by ensuring version_enc always receives a map with
defaults or by adding a fallback clause in version_enc/1: either change
handle_protocol_msg (where Caps is set) to supply default keys (major, minor,
patch) when get_caps/1 is absent, or add a new version_enc(Caps) clause that
reads missing keys via maps:get/3 (providing sane defaults) so a function_clause
error cannot occur; reference functions: handle_protocol_msg, get_caps/1,
version_enc/1 and the Caps variable when making the change.
| -module(mock_mumble_handler). | ||
| -behaviour(mumble_server_behaviour). | ||
|
|
||
| start_link(_Opts) -> | ||
| %% Simple mock - just return {ok, self()} as a placeholder pid | ||
| {ok, self()}. | ||
|
|
||
| stop(Pid) when is_pid(Pid) -> | ||
| catch exit(Pid, normal), | ||
| ok. | ||
|
|
||
| init(_Opts) -> | ||
| {ok, #{}}. | ||
|
|
||
| handle_msg(Msg, State) -> | ||
| case maps:get(message_type, Msg) of | ||
| 'TextMessage' -> | ||
| %% Reply with another text message | ||
| Reply = #{ | ||
| message_type => 'TextMessage', | ||
| message => <<"Echo: ", (maps:get(message, Msg))/binary>> | ||
| }, | ||
| %% We need to know who to send it to, but mock doesn't have session pid easily. | ||
| %% mumble_server_conn handles casting back if we return it? | ||
| %% Actually mumble_server_conn:handle_protocol_msg calls Mod:handle_msg. | ||
| %% If we want to send something back, we'd typically use mumble_server_conn:send(self(), Reply), | ||
| mumble_server_conn:send(self(), Reply), | ||
| {ok, State}; | ||
| _ -> | ||
| {ok, State} | ||
| end. | ||
|
|
||
| get_caps(_State) -> | ||
| #{major => 1, minor => 2, patch => 4, | ||
| os => <<"MockOS">>, | ||
| release => <<"1.0">>, | ||
| os_version => <<"1.0">>}. | ||
|
|
||
| authenticate(_AuthMsg, State) -> | ||
| UserInfo = #{session_id => 1, name => <<"MockUser">>}, | ||
| {ok, UserInfo, State}. |
There was a problem hiding this comment.
Critical: Missing -export directive — all callbacks are unexported.
The module declares -behaviour(mumble_server_behaviour) but never exports any function. In Erlang, without an -export([...]) or -compile(export_all) directive, all functions are module-local. Any external call (e.g., mock_mumble_handler:init([]) from mumble_server_conn) will fail with undef.
Proposed fix
-module(mock_mumble_handler).
-behaviour(mumble_server_behaviour).
+
+-export([start_link/1, stop/1, init/1, handle_msg/2, get_caps/1, authenticate/2]).🤖 Prompt for AI Agents
In `@apps/mumble_protocol/test/mock_mumble_handler.erl` around lines 1 - 41, The
module lacks an -export directive so its callbacks are not callable; add an
export list that exports start_link/1, stop/1, init/1, handle_msg/2, get_caps/1,
and authenticate/2 (matching the functions implemented) so the behaviour
callbacks (e.g., calls to mock_mumble_handler:init/1, start_link/1,
handle_msg/2, get_caps/1, authenticate/2) are available to other modules like
mumble_server_conn.
| {ok, #mock_socket{pid = Pid}}. | ||
|
|
||
| cleanup(_) -> ok. | ||
| messages() -> {ok, {ssl, ssl_closed}}. |
There was a problem hiding this comment.
messages/0 returns wrong format — Ranch expects {OK, Closed, Error} 3-tuple of atoms.
ranch_transport:messages/0 should return {ssl, ssl_closed, ssl_error} (a 3-tuple of message tag atoms). The current return {ok, {ssl, ssl_closed}} is a 2-tuple containing a nested tuple, which will cause Ranch to malfunction if it tries to destructure the message tags.
Proposed fix
-messages() -> {ok, {ssl, ssl_closed}}.
+messages() -> {ssl, ssl_closed, ssl_error}.🤖 Prompt for AI Agents
In `@apps/mumble_protocol/test/mock_transport.erl` at line 156, The messages/0
function returns the wrong tuple shape; update messages/0 to return a flat
3-tuple of atoms as Ranch expects (the message tag triple), e.g., change the
current nested {ok, {ssl, ssl_closed}} to the correct {ssl, ssl_closed,
ssl_error} so that the messages/0 function provides the expected {Tag, Closed,
Error} tuple for ranch_transport.
| -module(prop_protocol_tests). | ||
| -include_lib("proper/include/proper.hrl"). | ||
|
|
There was a problem hiding this comment.
Missing -export directive — properties won't be discovered.
Without exporting prop_varint_roundtrip/0 and prop_tcp_proto_roundtrip/0, the rebar3 proper plugin (and PropEr directly) cannot find or execute them.
Proposed fix
-module(prop_protocol_tests).
-include_lib("proper/include/proper.hrl").
+
+-export([prop_varint_roundtrip/0, prop_tcp_proto_roundtrip/0]).📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| -module(prop_protocol_tests). | |
| -include_lib("proper/include/proper.hrl"). | |
| -module(prop_protocol_tests). | |
| -include_lib("proper/include/proper.hrl"). | |
| -export([prop_varint_roundtrip/0, prop_tcp_proto_roundtrip/0]). |
🤖 Prompt for AI Agents
In `@apps/mumble_protocol/test/prop_protocol_tests.erl` around lines 1 - 3, The
module prop_protocol_tests is missing an export directive so PropEr can't
discover the properties; add an -export([...]) clause that exports
prop_varint_roundtrip/0 and prop_tcp_proto_roundtrip/0 (e.g.
-export([prop_varint_roundtrip/0, prop_tcp_proto_roundtrip/0]).) Place this
export in the prop_protocol_tests module alongside the existing -module and
-include_lib directives so the test runner can find and execute those property
functions.
There was a problem hiding this comment.
Actionable comments posted: 11
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
🤖 Fix all issues with AI agents
In `@apps/erlmur/src/erlmur_app.erl`:
- Line 30: Logger call in erlmur_app.erl passes a four-element list (PrivDir,
Port, CertFile, KeyFile) but the format string has no specifiers; update the
logger:info call so the format string contains four matching placeholders (e.g.
~p or ~s) and retain the existing argument list so each of PrivDir, Port,
CertFile, KeyFile is interpolated; locate the logger:info invocation in
erlmur_app.erl and replace the current plain string with one that includes four
specifiers.
In `@apps/mumble_protocol/rebar.config`:
- Around line 20-28: Update the ex_doc configuration: in the ex_doc tuple
(ex_doc) change the extras entries under extras from repo-root paths to
app-relative ones ("README.md" and "docs/establishing_connection.md") so ex_doc
resolves correctly for the app, remove the before_closing_body_tag entry if it's
unused (the before_closing_body_tag key), and normalize indentation by replacing
the tab characters on the lines containing with_mermaid and
before_closing_body_tag with spaces for consistent formatting.
In `@apps/mumble_protocol/src/mumble_cert.erl`:
- Around line 101-122: The generate_self_signed_cert function currently builds a
shell string with CertFile, KeyFile, Days, and Subject and calls os:cmd/1,
creating a command-injection risk; fix by replacing the os:cmd usage with an
exec-style call (open_port/2 or erl's port with {spawn_executable, "openssl"}
and {args, [...]}) to pass CertFile, KeyFile, Days and Subject as separate argv
entries (or, if you must keep os:cmd, strictly validate/escape shell
metacharacters for Subject/CertFile/KeyFile and reject unsafe input); update
generate_self_signed_cert (and callers like ensure_auto_certs/2) to either
validate inputs or use open_port with explicit args so no shell interpolation
occurs and then keep the existing file existence checks.
In `@apps/mumble_protocol/src/mumble_server_conn.erl`:
- Around line 191-195: The catch-all clause in function authenticating/3
currently matches every event (including the ?TIMEOUT timer) and returns {stop,
unhandled}; add an explicit clause that matches the timeout event (e.g.,
authenticating(?TIMEOUT, Msg, StateData) or authenticating(timeout, Msg,
StateData) depending on how ?TIMEOUT is delivered) placed before the catch-all
so the timeout case performs a clean shutdown (for example returning {stop,
timeout} or {stop, normal}) and does not fall through to the unhandled
logger/return in authenticating/3.
- Around line 480-490: The update_connection_stats/3 currently adds cumulative
values returned by ocb128_crypto:stats/1 directly into the running Stats
(server_ping), causing double-counting; change it to apply deltas by tracking
the previous crypto stats per-connection (e.g., add a prev_crypto_stats field to
the `#stats` or connection state) and compute Delta = CurrentCryptoStats -
PrevCryptoStats before adding to P#ping, then update PrevCryptoStats to
CurrentCryptoStats; alternatively, clear/reset the crypto counters after reading
if ocb128_crypto supports it, but do not add the raw cumulative CryptoStats to
server_ping in update_connection_stats/3.
In `@apps/mumble_protocol/src/mumble_udp_proto.erl`:
- Around line 34-35: The handler pattern currently captures the 5-bit target
into Timestamp (<<1:3, Timestamp/bits>>) which makes Timestamp 69 bits and
breaks the later v1.5 unpack (<<T:64>> = Timestamp); update the pattern in
handle/2 (and the other similar occurrence the comment mentions) to explicitly
extract the 5-bit target and the 64-bit timestamp (e.g., match <<1:3, Target:5,
T:64, _Rest/bits>> or at minimum <<1:3, Target:5, Timestamp:64>>) so the v1.5
branch can safely do <<T:64>> = Timestamp or use T directly; ensure any other
clause that previously used Timestamp/bits is adjusted consistently to bind
Target and a 64-bit timestamp.
In `@apps/mumble_protocol/src/mumble_udp_server.erl`:
- Around line 141-160: The handler in handle_info/2 is pulling the wrong field
with element(3, User) (which yields username) and passing a binary to
mumble_server_conn:udp_packet/3; change the lookup to use the user record pid
accessor (e.g. User#user.pid) or element(2, User) so Pid is an actual pid,
updating the lists:foreach fun to bind Pid = User#user.pid before calling
mumble_server_conn:udp_packet(Pid, Data, Addr).
In `@apps/mumble_protocol/src/mumble_varint.erl`:
- Around line 44-45: The small-negative varint handling in encode/decode is
off-spec: update encode(Num) to accept Num >= -4 andalso Num < 0 and encode
using the bitwise NOT convention by emitting (-(Num + 1)) into the 2-bit field
(so −1..−4 map to 0xFC..0xFF), and change the corresponding decode branch (the
clause matching the 2-bit prefix currently guarded with I > 0) to remove the I >
0 guard and return -(I + 1) when decoding that 2-bit prefix; adjust the guards
and arithmetic in the encode and decode clauses that reference encode/Num and
the decode variable I accordingly so −1..−4 are encoded/decoded to 0xFC..0xFF
per the Mumble spec.
In `@apps/mumble_protocol/src/mumble.erl`:
- Around line 125-158: The start_server/1 function improperly defaults
ServerHandler to the test module mock_mumble_handler and fails to validate that
both cert_file and key_file are present when auto_create_cert is false; update
the maps:get(server_handler, ...) call to require an explicit handler (returning
an error like {error, missing_server_handler} or raising) instead of defaulting
to mock_mumble_handler, and add validation before calling
mumble_server:start_server/1 to ensure either AutoCreate=true or both CertFile
and KeyFile are not undefined (if one or both are missing return a clear {error,
missing_cert_or_key}), and apply the same handler-default/validation policy to
the other start_server/2 and start_server/3 variants that currently hard-code
the mock.
In `@apps/mumble_protocol/test/mock_transport.erl`:
- Around line 68-70: The accept/2 function currently has its parameters reversed
(accept(_Timeout, ListenSocket)), which violates Ranch's ranch_transport
callback signature accept(Listener, Timeout); update the accept/2 definition to
accept(ListenSocket, Timeout) (and rename internal variables accordingly) so it
matches the expected order and return the same {ok, {mock_accepted,
ListenSocket}} tuple; also search for and adjust any internal or test calls to
accept/2 to use the corrected parameter order to avoid pattern-match failures.
In `@apps/ocb128_crypto/test/prop_ocb128_crypto.erl`:
- Around line 91-95: The anonymous function verifying crypto stats uses the
equality operator =:= which returns a boolean and doesn't fail the test; change
the comparison "(N - 1 - I) =:= Stats#crypto_stats.lost" to a pattern match
using "=" so mismatches raise an error and actually assert the lost value
(update the expression involving N, I and Stats#crypto_stats.lost inside the
fun({{_S, Stats}, I}) -> ... end).
🟠 Major comments (10)
apps/mumble_protocol/test/prop_protocol_tests.erl-4-8 (1)
4-8:⚠️ Potential issue | 🟠 MajorConstrain the
integer()generator to Mumble's supported range.The unbounded
integer()generator can produce values outside varint's 64-bit range, causing encode/1 to fail. Based on themumble_varint:encode/1implementation, the valid range is approximately-(2^63)to2^64 - 1. Useinteger(-(1 bsl 63), (1 bsl 64) - 1)to match the encoder's actual capabilities.apps/mumble_protocol/src/mumble_client_conn.erl-94-109 (1)
94-109:⚠️ Potential issue | 🟠 Major
ssl:connectcalled without a timeout — can block indefinitely.If the remote host is unreachable or firewalled,
ssl:connect/3(which defaults toinfinitytimeout) will block thegen_stateminitpath forever. Specify an explicit timeout (e.g., fromOptsor a default like 10 seconds).🐛 Proposed fix
- case ssl:connect(Host, Port, SslOptsList) of + Timeout = maps:get(connect_timeout, Opts, 10000), + case ssl:connect(Host, Port, SslOptsList, Timeout) ofapps/erlmur/src/erlmur_user_manager.erl-143-152 (1)
143-152:⚠️ Potential issue | 🟠 MajorStale UDP mapping when a user's address changes.
When
register_udp_addris called for a session that already has audp_addr, the old address is not removed fromudp_map. The old{OldAddr, Pid}entry remains, meaningget_session_by_udpcould return a stale mapping for the old address.🐛 Proposed fix — remove old mapping before inserting new one
handle_cast({register_udp_addr, SessionId, Addr}, State = `#state`{users = Users, udp_map = UdpMap}) -> case ets:lookup(Users, SessionId) of - [User] -> + [User = `#user`{udp_addr = OldAddr}] -> + case OldAddr of + undefined -> ok; + _ -> ets:delete(UdpMap, OldAddr) + end, ets:insert(Users, User#user{udp_addr = Addr}), ets:insert(UdpMap, {Addr, User#user.pid}), logger:debug("Registered UDP addr ~p for session ~p", [Addr, SessionId]);apps/mumble_protocol/README.md-87-88 (1)
87-88:⚠️ Potential issue | 🟠 MajorFix README example to match the actual
start_link/3API signature.The README line 95 example is incompatible with the module's API. The function expects:
- First argument: a
string(), not a tuple- Third argument: a
map(), not a proplistCorrect the example from:
{ok, ClientPid} = mumble_client_conn:start_link({127, 0, 0, 1}, 64738, Opts).to:
Opts = #{certfile => "client_cert.pem", keyfile => "client_key.pem"}, {ok, ClientPid} = mumble_client_conn:start_link("127.0.0.1", 64738, Opts).The README's arity reference to
start_link/3is correct; only the argument types and format are wrong.apps/mumble_protocol/test/mumble_client_api_tests.erl-7-11 (1)
7-11:⚠️ Potential issue | 🟠 MajorOnly 2 of 7 test functions are included in the test generator — the rest are dead code.
client_start_no_certs,client_send_message,client_send_voice,client_get_state, andclient_stopare defined but never referenced inmumble_client_api_test_/0, so EUnit will not execute them. Additionally,setup/0andcleanup/1are not wired into any EUnit fixture ({setup, ...}or{foreach, ...}).If these tests require a running server (which they seem to, since they call
mumble:start_clientwith"localhost"), wrap them in a{setup, fun setup/0, fun cleanup/1, [...]}fixture. Otherwise, add them to the test list.Proposed fix — include all tests
mumble_client_api_test_() -> - [ - {"client start with invalid certs", fun client_start_invalid_certs/0}, - {"client invalid ref handling", fun client_invalid_ref/0} - ]. + [ + {"client start with invalid certs", fun client_start_invalid_certs/0}, + {"client invalid ref handling", fun client_invalid_ref/0}, + {"client start no certs", fun client_start_no_certs/0}, + {"client send message", fun client_send_message/0}, + {"client send voice", fun client_send_voice/0}, + {"client get state", fun client_get_state/0}, + {"client stop", fun client_stop/0} + ].apps/mumble_protocol/src/mumble_server_sup.erl-71-79 (1)
71-79:⚠️ Potential issue | 🟠 MajorReplace custom MFA wrapper with
ranch:child_spec/5for proper listener embedding.The
start_tcp_listener/5function directly callsranch:start_listener/5, which starts the listener under Ranch's supervision tree (not under this supervisor). Using it as a child spec MFA creates an improper relationship:
- The supervisor receives
{ok, Pid}from the MFA and links to it, but Ranch owns the process viaranch_sup.- If Ranch restarts the listener, this supervisor doesn't see the restart.
- If both try to restart simultaneously, the second attempt may fail because the listener name is already registered.
Use
ranch:child_spec/5instead to generate a proper child spec that embeds the listener directly in your supervision tree:RanchListener = ranch:child_spec( mumble_tcp_listener, ranch_ssl, #{ socket_opts => [...], num_acceptors => 10 }, mumble_server_conn, [Handler] )Then add
RanchListenerdirectly to the children list. This gives your supervisor true ownership and proper restart semantics.apps/mumble_protocol/src/mumble_server.erl-34-46 (1)
34-46:⚠️ Potential issue | 🟠 MajorProduction default should not be a test mock module.
Line 38 defaults
server_handlertomock_mumble_handler. This means callingstart_server(#{})in production uses a test mock. This should either have no default (requiring the caller to provide it) or default to a real handler module.Suggested fix
- ServerHandler = maps:get(server_handler, Options, mock_mumble_handler), + ServerHandler = maps:get(server_handler, Options),apps/mumble_protocol/src/mumble_tcp_proto.erl-44-61 (1)
44-61:⚠️ Potential issue | 🟠 Major
MESSAGE_TABLEis missing ten message types supported bymumble_msg.
mumble_msg.erlsupportsReject,PermissionDenied,ACL,QueryUsers,ContextActionModify,ContextAction,VoiceTarget,SuggestConfig,PluginDataTransmission, andRequestBlob, but these are absent fromMESSAGE_TABLE. Callingpack/1for any of these will raiseerror({unknown_type, ...}), and incoming messages of these types will be silently skipped duringunpack(with an error log).apps/mumble_protocol/test/mumble_SUITE.erl-286-302 (1)
286-302:⚠️ Potential issue | 🟠 MajorMultiple tests are stubs that don't test their named behavior.
client_connect_disconnect_test,client_with_callback_test,client_server_communication_test, andmultiple_clients_testall only start and stop a server — they never create a client, exercise callbacks, or test multi-client scenarios. As written, these duplicateserver_start_stop_testand give false confidence in test coverage.Consider either implementing the actual test logic or marking them as
{skip, "Not yet implemented"}to make their status explicit.Also applies to: 305-320, 456-471, 474-489
apps/mumble_protocol/test/mock_transport.erl-176-185 (1)
176-185:⚠️ Potential issue | 🟠 MajorBlocking
receiveinsidehandle_callstalls the gen_server.When the message queue is empty (line 178), the
handle_callforrecventers a rawreceiveblock that suspends the entire gen_server process. While blocked, no otherhandle_call,handle_cast, orhandle_infocan be processed — including the{inject_message, Data}cast that would unblock the receive. This creates a deadlock:recvwaits for data that can only arrive via a cast the server can't process.For a test mock this may not surface if
recvis never called on an empty queue, but it's a latent deadlock.Proposed fix — reply asynchronously or queue the caller
One approach: store the pending caller and reply from
handle_castwhen data arrives.-handle_call({recv, _Length, _Timeout}, _From, State) -> - %% Wait for a message to be injected - receive - {inject_message, Data} -> - {reply, {ok, Data}, State} - after Timeout -> - {reply, {error, timeout}, State} - end; +handle_call({recv, _Length, Timeout}, From, State) -> + %% No data available — park the caller and reply later + TRef = case Timeout of + infinity -> undefined; + T -> erlang:send_after(T, self(), {recv_timeout, From}) + end, + {noreply, State#{pending_recv => {From, TRef}}};Then in
handle_cast({inject_message, ...}), check forpending_recvand reply to the parked caller.
🟡 Minor comments (22)
justfile-41-41 (1)
41-41:⚠️ Potential issue | 🟡 MinorOverly broad
docdirectory deletion may remove source documentation.
find ... -name "doc"will match any directory nameddocin the entire project tree, including potential source documentation directories underapps/*/doc. Consider scoping this to the known output location (e.g.,rm -rf {{justfile_directory()}}/doc) or excludingapps/paths.justfile-44-45 (1)
44-45:⚠️ Potential issue | 🟡 MinorBroadened glob patterns could delete user-provided certificates.
Switching from specific filenames (
cert.pem,key.pem) to*.pem/*.keymeans any PEM or key file anywhere in the tree will be removed, including manually placed or third-party certificates. Consider limiting the search to specific directories or using more specific filename patterns.apps/mumble_protocol/src/mumble_cert.erl-72-84 (1)
72-84:⚠️ Potential issue | 🟡 Minor
code:priv_dir/1will fail if the application isn't loaded.If
mumble_protocolisn't yet loaded/started whenensure_auto_certs/2is called,code:priv_dir(mumble_protocol)returns{error, bad_name}(not a string), which will crash downstream atfilename:join/2. Consider handling this error case.AGENTS.md-7-7 (1)
7-7:⚠️ Potential issue | 🟡 Minor
just devtarget doesn't exist — should bejust shell.The justfile defines a
shelltarget, notdev. This will confuse developers following the guide.Proposed fix
-just dev # Start development shell +just shell # Start development shellAGENTS.md-65-65 (1)
65-65:⚠️ Potential issue | 🟡 MinorIncorrect Erlang syntax:
try...of...elsedoesn't exist.Erlang's exception handling is
try Expr of Pattern -> Body catch Class:Reason -> Handler end. There is noelseclause. This could mislead developers.Proposed fix
-- Use `try...of...else` for exceptions that need cleanup +- Use `try...of...catch...after...end` for exceptions that need cleanupapps/mumble_protocol/src/mumble_cert.erl-127-133 (1)
127-133:⚠️ Potential issue | 🟡 Minor
file:make_dir/1is not recursive — nested directories will fail.If the parent of the certificate directory doesn't exist,
file:make_dir/1will return{error, enoent}. Consider usingfilelib:ensure_dir/1(which creates all parent directories for a file path) orfilelib:ensure_path/1(OTP 25+) for the directory itself.Proposed fix
ensure_cert_dir(CertFile) -> - Dir = filename:dirname(CertFile), - case file:make_dir(Dir) of - ok -> ok; - {error, eexist} -> ok; - {error, Reason} -> {error, {dir_creation_failed, Reason}} - end. + case filelib:ensure_dir(CertFile) of + ok -> ok; + {error, Reason} -> {error, {dir_creation_failed, Reason}} + end.rebar.config-2-2 (1)
2-2:⚠️ Potential issue | 🟡 MinorRemove hardcoded profile name from GPB include path.
The path
_build/default/plugins/gpb/includeexplicitly references thedefaultprofile. If the project is built with a different profile (e.g.,test), this path becomes invalid. While the test profile currently overrideserl_optsentirely and doesn't use this path, the GPB-generated headers are already available locally atapps/mumble_protocol/include, making the hardcoded path potentially redundant. For correctness and to avoid future issues, consider either removing this path if the local include is sufficient, or using a profile-independent approach.apps/mumble_protocol/src/mumble_udp_proto.erl-9-21 (1)
9-21:⚠️ Potential issue | 🟡 MinorDocumentation contradicts itself on message types.
Line 12 says "0=Ping, 1=Voice" but lines 19–20 say "Type 1: Ping" and "Type 0, 2-7: Voice". The code treats Type 1 as Ping (line 34), which aligns with the Mumble protocol spec.
Fix the doc
-- **Type (3 bits)**: Message type (0=Ping, 1=Voice, etc.) +- **Type (3 bits)**: Message type (1=Ping, 0/2-7=Voice codecs)apps/mumble_protocol/src/mumble_client_conn.erl-130-134 (1)
130-134:⚠️ Potential issue | 🟡 MinorStats field names are misleading — tracking received data but naming it
from_client_*.In a client module,
from_client_tcp_packetsandfrom_client_tcp_bytesare incremented when data is received from the server. The naming suggests these track outbound client traffic. Consider using field names that reflect the direction correctly (e.g.,received_tcp_packets), or check whether the#stats{}record fields are designed for the server's perspective.Also applies to: 154-159
apps/mumble_protocol/src/mumble_client_conn.erl-217-224 (1)
217-224:⚠️ Potential issue | 🟡 MinorUDP send functions are completely stubbed —
udp_verifiedcan never becometrue.
send_udp_ping/1andsend_udp/2are no-ops (just log). No UDP socket is created anywhere in this module, so theestablished(info, {udp, ...}, ...)handler on line 185 is unreachable, andudp_verifiedwill never becometrue. The voice-over-UDP path indo_send_voiceis dead code.Consider either implementing the UDP socket lifecycle or removing the dead branches to avoid confusion.
Do you want me to open an issue to track UDP implementation for the client connection?
apps/erlmur/src/erlmur_server_handler.erl-78-86 (1)
78-86:⚠️ Potential issue | 🟡 Minor
erlang:system_info(otp_release)returns a charlist, not a binary.The other values in the caps map (
release,os) are binaries.otp_releasereturns a string like"27", which may cause encoding failures if downstream code expects all values to be binaries.🐛 Proposed fix
#{ major => 1, minor => 2, patch => 4, release => <<"Erlmur MVP">>, os => <<"Erlang/OTP">>, - os_version => erlang:system_info(otp_release) + os_version => list_to_binary(erlang:system_info(otp_release)) }.apps/mumble_protocol/test/mumble_cert_tests.erl-114-117 (1)
114-117:⚠️ Potential issue | 🟡 MinorOverly weak fallback assertion —
"test"will match almost anything.The fallback
string:find(Subject, "test") =/= nomatchis too broad; it would match the word "test" in any context (e.g., directory names, other fields). This makes the assertion nearly vacuous when the primary check fails.🐛 Proposed fix — tighten the assertion
?assert( - (string:find(Subject, "test.example.com") =/= nomatch) orelse - (string:find(Subject, "test") =/= nomatch) + (string:find(Subject, "test.example.com") =/= nomatch) orelse + (string:find(Subject, "CN = test.example.com") =/= nomatch) )apps/mumble_protocol/test/mumble_cert_tests.erl-138-158 (1)
138-158:⚠️ Potential issue | 🟡 Minor
reuse_existing_cert_testdoesn't actually test reuse — it only asserts the file is unchanged during a sleep.The test generates a cert, reads its mtime, sleeps 100ms, then reads mtime again. It never calls
generate_cert/2a second time, so it doesn't verify that an existing certificate is reused rather than regenerated. The test will always pass trivially.To test reuse, call
generate_cert(CertFile, KeyFile)again after the sleep and then assert that the mtime is unchanged.🐛 Proposed fix
%% First generation generate_cert(CertFile, KeyFile), {ok, FileInfo1} = file:read_file_info(CertFile), MTime1 = FileInfo1#file_info.mtime, timer:sleep(1100), - %% Check files still exist + %% Second generation — should reuse existing cert + generate_cert(CertFile, KeyFile), {ok, FileInfo2} = file:read_file_info(CertFile), MTime2 = FileInfo2#file_info.mtime, ?assertEqual(MTime1, MTime2)Note: The local
generate_cert/2helper always overwrites the file unconditionally (it just callsopenssl req). If the intent is to test reuse logic, this test should callmumble_cert:generate_self_signed_cert/4ormumble_cert:ensure_auto_certs/2instead, which presumably has the skip-if-exists logic.apps/erlmur/src/erlmur_user_manager.erl-81-86 (1)
81-86:⚠️ Potential issue | 🟡 MinorAdd wrapping or validation for
next_session_idto respect Mumble's uint32 constraint.The Mumble protocol requires session IDs to be
uint32(max value0xFFFFFFFFor 4,294,967,295). Currently,next_session_idgrows unboundedly as apos_integer()without any wrapping or overflow guards. After approximately 4.3 billion registrations, the value will exceed the valid uint32 range, potentially causing serialization failures or collisions when clients receive out-of-range session IDs.Implement either:
- Wrap-around logic:
(SessionId rem 16#FFFFFFFF) + 1(ensuring non-zero IDs), or- Guard clause with server restart/reset, or
- Modulo arithmetic to keep values within uint32 bounds
apps/mumble_protocol/README.md-31-31 (1)
31-31:⚠️ Potential issue | 🟡 MinorFix spelling errors.
- Line 31: "recieved" → "received"
- Line 37: "reciveing" → "receiving"
- Line 45: "explicitely" → "explicitly"
📝 Proposed fix
-If no `Ping` is recieved for 30sec the client will be disconnected from the server. +If no `Ping` is received for 30 seconds, the client will be disconnected from the server.-If reciveing a `UDPTunnel` message the connection is marked "UDP verified" false, until new UDP verification is done. +If receiving a `UDPTunnel` message, the connection is marked "UDP verified" false, until new UDP verification is done.-- **Automatic Fallback**: If a server hasn't received a UDP packet from a client recently (or if the client explicitely +- **Automatic Fallback**: If a server hasn't received a UDP packet from a client recently (or if the client explicitlyAlso applies to: 37-37, 45-45
apps/mumble_protocol/docs/establishing_connection.md-79-81 (1)
79-81:⚠️ Potential issue | 🟡 MinorMinor: CodecVersion ordering differs between prose and diagram.
In the prose (sections 4.x), CodecVersion is described after UserState (line 79–81). In the Mermaid diagram (line 116), it appears before ChannelState/UserState under "Cryptographic & Config Setup." If the server can send these in any order, consider adding a note; otherwise, align the two for consistency.
Also applies to: 112-130
apps/erlmur/src/erlmur_sup.erl-23-32 (1)
23-32:⚠️ Potential issue | 🟡 MinorLog on line 31 is misleading — user manager hasn't started yet.
init/1only returns the child specification; the supervisor starts children afterinit/1returns. The log"User manager started"fires before the child process actually starts. Consider removing it or changing to"Supervisor initialized with user_manager child spec".apps/mumble_protocol/test/mock_mumble_handler.erl-4-6 (1)
4-6:⚠️ Potential issue | 🟡 Minor
start_link/1returns{ok, self()}— does not start a new process.This returns the caller's pid rather than spawning and linking a new process. For a mock this may be acceptable if callers don't rely on the returned pid being a distinct, linked process. If any caller monitors or links to this pid expecting independent lifecycle management, it will behave incorrectly.
apps/mumble_protocol/src/mumble_server_sup.erl-61-61 (1)
61-61:⚠️ Potential issue | 🟡 MinorDefault handler is
mock_mumble_handler— risky for production.If
server_handleris accidentally omitted from the options, production will silently use the mock handler. Consider requiring this option (crash if absent) or defaulting to a production handler.Proposed fix — require the option
- ServerHandler = proplists:get_value(server_handler, Options, mock_mumble_handler), + ServerHandler = case proplists:get_value(server_handler, Options) of + undefined -> error({missing_required_option, server_handler}); + H -> H + end,apps/mumble_protocol/src/mumble_udp_server.erl-38-40 (1)
38-40:⚠️ Potential issue | 🟡 Minor
send/3spec promises{error, term()}butgen_server:castalways returnsok.The caller never sees an error — any send failure is logged inside
handle_castbut swallowed. The spec should beokonly, or switch togen_server:callif the caller needs error feedback.apps/mumble_protocol/test/mock_transport.erl-190-202 (1)
190-202:⚠️ Potential issue | 🟡 Minor
setoptsactive-mode simulation sends unexpected messages.When
{active, true}is set, line 194 sends{ssl_passive, self()}followed by{ssl, self(), <<>>}. Real SSL sockets don't emitssl_passivewhen activating active mode, and sending an empty binary<<>>has no analog in real behavior. This could confuse tests that assert on message patterns. Thessl_passivemessage is only sent by real SSL when the active-N counter reaches zero.apps/mumble_protocol/test/mumble_SUITE.erl-77-82 (1)
77-82:⚠️ Potential issue | 🟡 MinorOpenSSL availability check is not portable.
whichis not available on Windows and may behave differently across Unix variants. Additionally,os:cmd/1returns a non-empty error string (e.g.,"which: no openssl...") rather than[]on some systems when the command is not found.Consider using
os:find_executable("openssl")which returnsfalsewhen the binary isn't found, and works across platforms.Proposed fix
- case os:cmd("which openssl") of - [] -> - ct:fail(openssl_not_available); - _ -> - ok - end, + case os:find_executable("openssl") of + false -> + ct:fail(openssl_not_available); + _ -> + ok + end,
🧹 Nitpick comments (34)
AGENTS.md (1)
20-93: Markdown lint: missing blank lines around headings and fenced code blocks.Multiple headings (Lines 20, 25, 31, 37, 45, 57, 63, 69, 76, 82, 88) and code fences (Lines 46, 99, 106) lack required surrounding blank lines per MD022/MD031. Adding blank lines before each
###heading and around fenced code blocks would fix these.rebar.config (1)
34-35: All dependencies are pinned to branch heads — builds are not reproducible.
ranch,eqwalizer_support,meck, andproperall reference{branch, "master"}or{branch, "main"}. A breaking upstream commit will silently break your build. Consider pinning to a specific tag or commit SHA. Therebar.lockfile provides some protection, but only if it's consistently committed and not regenerated carelessly.Also applies to: 40-45
apps/mumble_protocol/src/mumble_varint.erl (1)
1-59: No catch-all / error clause for malformed input indecode/1.If
decode/1is called with a binary that doesn't match any clause (e.g., truncated data), the caller gets an uninformativefunction_clausecrash. Consider adding a final catch-all clause that returns a tagged error for better diagnostics, especially since this processes network input.apps/mumble_protocol/src/mumble_client.erl (2)
43-62: Consider guarding against mismatched cert/key arguments.When only one of
CertFile/KeyFileisundefinedand the other is not, this falls into the{CF, KF}branch and relies onmumble_cert:validate_cert_files/2to reject it. An explicit clause or early guard would make the contract clearer and give a better error message at this layer.Example
case {CertFile, KeyFile} of {undefined, undefined} -> start_client_impl(Host, Port, Options); + {undefined, _} -> + {error, {missing_cert_file, "cert_file required when key_file is provided"}}; + {_, undefined} -> + {error, {missing_key_file, "key_file required when cert_file is provided"}}; {CF, KF} ->
131-141:start_linkcouples the client lifetime to the caller.
mumble_client_conn:start_link/3links the client to the calling process. If the caller exits (e.g., a short-lived shell or test process), the client dies silently. If this is intentional supervision, it's fine; otherwise considerstart/3orstart_monitor/3for callers that aren't supervisors.apps/mumble_protocol/src/mumble_udp_proto.erl (1)
52-56:split_voice_positional_speex_celt/1crashes on truncated input.The function only matches
<<Cont:1, Len:7, V:Len/binary, Rest/binary>>. If the binary is truncated mid-frame (common with UDP), this produces an uninformativefunction_clauseerror instead of a graceful failure.apps/mumble_protocol/rebar.config (1)
26-27: Inconsistent indentation: tabs on lines 26–27, spaces elsewhere.Lines 26–27 use tab indentation while the rest of the file uses spaces. Minor formatting nit.
apps/mumble_protocol/test/mumble_test_callback.erl (1)
6-14:messagesfield in state is initialized but never updated.
init/1setsmessages => [](line 8) buthandle_msg/2never appends to it — messages are forwarded to the parent pid instead. If accumulation isn't needed, consider dropping the field to avoid confusion.apps/mumble_protocol/src/mumble_version.erl (1)
36-46:decode/1auto-detects V1 vs V2 by value range — fragile for small versions.The threshold
V1 =< 16#FFFFFFFFvsV2 > 16#FFFFFFFFworks for real-world Mumble versions (≥ 1.0.0), but a V2-encoded version like 0.0.1 (0x0000000000010000= 65536) falls into the V1 branch and decodes as 0.1.0. Consider either:
- Documenting the assumption that Major ≥ 1 for V2, or
- Providing separate
decode_v1/1/decode_v2/1for callers that know the source field format.apps/mumble_protocol/test/mumble_cert_tests.erl (2)
56-68: Tests exercise local OpenSSL helpers instead of themumble_certmodule under test.All tests call the local
generate_cert/2,4helpers (which shell out toopenssl req), but the AI summary indicates the production modulemumble_certexposesvalidate_cert_files/2,ensure_auto_certs/2, andgenerate_self_signed_cert/4. These tests verify that OpenSSL works, not that the application's certificate management logic is correct.Consider adding tests that exercise the actual
mumble_certAPI to ensure coverage of validation, auto-generation, and edge-case handling in the production code.Also applies to: 70-83, 85-102, 104-120, 122-136, 160-182, 184-201
48-50:which opensslis not portable to all platforms.
whichis not POSIX-mandated and may not exist on some systems (e.g., certain minimal containers). Consider usingos:find_executable("openssl")which is Erlang-native and cross-platform.♻️ Proposed fix
openssl_available_test() -> - Result = os:cmd("which openssl"), - ?assert(length(Result) > 0). + ?assertNotEqual(false, os:find_executable("openssl")).apps/erlmur/src/erlmur_user_manager.erl (2)
157-181:handle_infoDOWN handler performs O(n) full-table scan to find user by PID.
ets:foldliterates the entire users table to find the user matching the downed PID. This is acceptable for an MVP with a small user count, but consider adding a PID-indexed secondary ETS table (orets:match/2which at least stops internally) if you expect scaling.
40-50: The#user{}record type is opaque to callers outside this module.
get_user/1returns{ok,#user{}}in its spec, but since the record is defined locally (not in a header), callers cannot pattern-match on the result. Consider either moving the record to a shared header, exporting a map representation, or defining accessor functions.apps/erlmur/src/erlmur_server_handler.erl (2)
61-68: All permissions granted unconditionally — flag for hardening before production.
16#FFFFFFFFgrants every permission to every user. This is acknowledged as MVP, but should be tracked as a TODO to implement proper permission checking.Do you want me to open an issue to track implementing proper permission management?
26-34: No validation or rate-limiting on authentication — all users accepted.
authenticate/2unconditionally accepts everyAuthenticatemessage. While the%% MVPcomment acknowledges this, there's no guard against resource exhaustion (e.g., a client repeatedly connecting and registering). Consider adding at minimum a TODO comment to track this for hardening.apps/mumble_protocol/src/mumble_client_conn.erl (1)
192-195: UDP ping timer starts immediately on enteringestablishedwith a 1-second delay, firing every 5 seconds thereafter.The first timer fires after 1 second (line 194), but subsequent ones fire every 5 seconds (line 183). Since
send_udp_pingis a no-op stub, this timer just creates recurring log noise. If UDP is not yet implemented, consider deferring the timer start until UDP support is in place.apps/ocb128_crypto/src/ocb128_crypto.erl (1)
423-436:repeatanddropclassifications don't update stats.When a packet is classified as
repeatordrop, the state is returned without incrementing any stats counter. Consider tracking these events (e.g., addingrepeatanddropfields tocrypto_stats) for observability — this data is valuable for diagnosing network issues.apps/mumble_protocol/test/client_logic_tests.erl (4)
312-317: Placeholder tests provide false confidence.
established_transition_test,client_close_test, andinvalid_msg_testall just asserttrue. They pass unconditionally and give the appearance of coverage without testing anything. Consider either implementing them with mock transports or removing them and tracking the gaps with TODO comments / issues.Would you like me to open an issue to track these placeholder tests for future implementation?
Also applies to: 344-350, 425-430
407-415:network_error_testdoesn't test error handling.Iterating over a list of atoms and asserting
is_atom/1is tautologically true and verifies nothing about how the client handles these errors.
417-423:ssl_close_testonly reconstructs and matches a tuple it just created.This does not exercise the client's
handle_commonhandler for{ssl_closed, _}messages.
328-342:established_receive_testtests Erlang mailbox mechanics, not client logic.Sending
{mumble_msg, Msg}toself()and receiving it verifies message passing, not thatmumble_client_connactually forwards messages to the parent in the established state.apps/mumble_protocol/test/protocol_varint_tests.erl (1)
8-12: Negative varint tests will break the test suite if decode truly lacks negative support.The comment on line 10 acknowledges that
decodemay not handle negatives, yet the tests use?_assertEqualwhich will produce failures (not skips). If this is intentional to track a known gap, consider guarding with atry/catchand using?debugMsgto document the skip, or exclude these from the default test suite until the fix lands. Otherwise every CI run will report failures for a known limitation.apps/erlmur/src/erlmur_app.erl (1)
18-21:list_to_atom/1on environment variable input is an atom table pollution risk.
list_to_atom(LevelStr)on an arbitrary env var value will create atoms that are never garbage-collected. Consider usinglist_to_existing_atom/1with a fallback, or validate against a known set of log levels.Proposed fix
- LevelStr -> list_to_atom(LevelStr) + LevelStr -> + try list_to_existing_atom(LevelStr) + catch error:badarg -> + logger:warning("[erlmur] Invalid log level '~s', defaulting to info", [LevelStr]), + info + endapps/mumble_protocol/src/mumble_msg.erl (2)
38-42: No catch-all clause for unknown message types —function_clausecrash.Both
raw_to_map/1andfrom_map/1lack a fallback clause. If an unrecognized record or a map with an unknownmessage_typeis passed, this will crash withfunction_clause. Consider adding a catch-all that returns{error, {unknown_message, ...}}or at least a clear error.Suggested fallback for from_map
from_map(#{message_type := 'PluginDataTransmission'} = Map) -> #'PluginDataTransmission'{ senderSession = maps:get(senderSession, Map, undefined), receiverSessions = maps:get(receiverSessions, Map, []), data = maps:get(data, Map, undefined), dataID = maps:get(dataID, Map, undefined) - }. + }; +from_map(#{message_type := Type}) -> + error({unknown_message_type, Type}).
519-532: Side-effectful default fortimestampin Ping conversion.Line 521 uses
erlang:system_time(second)as a default value. This means callingfrom_map(#{message_type => 'Ping'})silently inserts the current time, making this conversion function impure. If this is intentional, a comment would help; otherwise,undefinedmay be a safer default to let the caller decide.apps/mumble_protocol/src/mumble_tcp_proto.erl (1)
137-144:find_msg_by_tagdoesn't short-circuit on match.
lists:foldlcontinues scanning the entire list even after finding the matching tag. Consider usinglists:keyfind/3orlists:search/2for an early exit, consistent with howfind_msg_by_record/1already useslists:search/2.Suggested refactor
find_msg_by_tag(Tag) -> - lists:foldl(fun ({T, Record}, _Acc) when T =:= Tag -> - {ok, Tag, Record}; - (_, Acc) -> - Acc - end, - error, - ?MESSAGE_TABLE). + case lists:keyfind(Tag, 1, ?MESSAGE_TABLE) of + {Tag, Record} -> {ok, Tag, Record}; + false -> error + end.apps/mumble_protocol/src/mumble_server_conn.erl (1)
447-457:notice-level logging on every sent message is too verbose for production.Line 450 logs at
noticelevel for every message sent. In a busy server, this will flood logs. Considerdebuglevel.Suggested fix
- logger:notice("Sending ~p (~p bytes)", [maps:get(message_type, Map), byte_size(Bin)]), + logger:debug("Sending ~p (~p bytes)", [maps:get(message_type, Map), byte_size(Bin)]),apps/mumble_protocol/test/test_utils.erl (1)
68-74:wait_for_message/2only supports exact-value matching, not pattern matching.Since
Patternis a bound variable,receive Pattern = Msg ->matches only messages exactly equal toPattern. It cannot match partial patterns (e.g., tuples with wildcards). If flexible matching is needed, consider a predicate-based approach:wait_for_message(fun(Msg) -> ... end, Timeout).This may be sufficient for current test usage, but worth noting as a limitation.
apps/mumble_protocol/test/mumble_SUITE.erl (4)
93-96: Fragile error detection for certificate generation.Matching only
"Error" ++ _won't catch OpenSSL failures that begin with other prefixes (e.g.,"Can't","unable to","problems making", etc.). Relying onfilelib:is_filechecks on lines 99–100 is a more reliable indicator. Consider checking the exit status or just relying on the file-existence assertions below.
237-240: Brittle positional access into opaque server state.
element(3, State)will silently break if the#state{}record inmumble_udp_serveris reordered or extended. Prefer usingsys:get_state/1with a record pattern, or expose a dedicated API (e.g.,mumble_udp_server:get_port/1) to query the port.
518-576: Dead helper functions:get_server_port/1,get_ranch_port/1,find_port_in_listeners/1, andwait_for_client_connection/2are unused.These helpers are never called from any test case.
get_server_port/1always returns a hardcoded64738, andwait_for_client_connection/2is a no-op placeholder. They add maintenance burden without providing value.Either remove them or, if they're planned for future tests, annotate them clearly (e.g., with a
TODOand suppress unused-function warnings).
380-413:auto_cert_reuse_testimplicitly depends onauto_cert_basic_testhaving created the cert files.If
auto_cert_basic_testfails or is skipped, this test will crash at line 387 with{error, enoent}instead of a clear skip/failure. While the[sequence]group property will stop after a failure, the error message won't be obvious. Consider adding a guard:case file:read_file_info(CertFile) of {ok, FileInfo1} -> ...; {error, _} -> ct:fail(prerequisite_auto_cert_basic_test_did_not_run) endapps/mumble_protocol/test/mock_transport.erl (2)
87-88:recv/3passesTimeoutas thegen_server:calltimeout, but the server may block internally for the same duration.If
Timeoutis finite, both thegen_server:calland the internalreceive(line 180) race on the same deadline. Thegen_server:calltimeout could fire first, leaving an orphaned reply in the gen_server's mailbox. Consider using a slightly larger call timeout orinfinityfor the call and letting only the server-side timeout govern.
40-42:start_link/0starts an unregistered gen_server — ownership tracking is absent.The
init/1setsowner => undefined, andowneris only set viacontrolling_process. Ifcontrolling_processis never called (which is the case for sockets created inhandshake/2,connect/4, andmock_handshake/1), the owner staysundefinedand active-mode notifications are silently dropped. This may cause tests to silently pass when they shouldn't receive data.Also applies to: 167-174
| Port = application:get_env(erlmur, listen_port, 64738), | ||
| CertFile = application:get_env(erlmur, cert_pem, filename:join(PrivDir, "server.pem")), | ||
| KeyFile = application:get_env(erlmur, key_pem, filename:join(PrivDir, "server.key")), | ||
| logger:info("PrivDir Port CertFile KeyFile",[PrivDir, Port,CertFile,KeyFile]), |
There was a problem hiding this comment.
Logger format string has no placeholders — will crash or produce a logger error at runtime.
The format string "PrivDir Port CertFile KeyFile" contains no ~p/~s specifiers, but a 4-element list is passed as the second argument. This will cause a logger formatting error.
Proposed fix
- logger:info("PrivDir Port CertFile KeyFile",[PrivDir, Port,CertFile,KeyFile]),
+ logger:info("PrivDir: ~p Port: ~p CertFile: ~p KeyFile: ~p", [PrivDir, Port, CertFile, KeyFile]),🤖 Prompt for AI Agents
In `@apps/erlmur/src/erlmur_app.erl` at line 30, Logger call in erlmur_app.erl
passes a four-element list (PrivDir, Port, CertFile, KeyFile) but the format
string has no specifiers; update the logger:info call so the format string
contains four matching placeholders (e.g. ~p or ~s) and retain the existing
argument list so each of PrivDir, Port, CertFile, KeyFile is interpolated;
locate the logger:info invocation in erlmur_app.erl and replace the current
plain string with one that includes four specifiers.
| generate_self_signed_cert(CertFile, KeyFile, Subject, Days) -> | ||
| %% Ensure directory exists (ignore eexist error) | ||
| case ensure_cert_dir(CertFile) of | ||
| ok -> | ||
| Cmd = io_lib:format( | ||
| "openssl req -x509 -newkey rsa:2048 -keyout ~s -out ~s -days ~B -nodes -subj '~s' 2>&1", | ||
| [KeyFile, CertFile, Days, Subject] | ||
| ), | ||
|
|
||
| _Output = os:cmd(Cmd), | ||
| %% Check if files were created (OpenSSL outputs progress to stderr) | ||
| case {filelib:is_file(CertFile), filelib:is_file(KeyFile)} of | ||
| {true, true} -> | ||
| {ok, CertFile, KeyFile}; | ||
| {false, _} -> | ||
| {error, {cert_generation_failed, "Cert file not created"}}; | ||
| {_, false} -> | ||
| {error, {cert_generation_failed, "Key file not created"}} | ||
| end; | ||
| {error, Reason} -> | ||
| {error, Reason} | ||
| end. |
There was a problem hiding this comment.
Command injection vulnerability via unsanitized shell arguments.
Subject, KeyFile, and CertFile are interpolated directly into a shell command string passed to os:cmd/1. If any parameter contains shell metacharacters (e.g., a Subject like "/CN=test; rm -rf /"), arbitrary commands can be executed. This is especially concerning since Subject originates from caller input in ensure_auto_certs/2.
Consider either:
- Sanitizing/validating all inputs before interpolation (reject or escape shell-special characters), or
- Using Erlang's
open_port/2with{args, [...]}to avoid shell interpretation entirely.
Sketch using open_port to avoid shell injection
- Cmd = io_lib:format(
- "openssl req -x509 -newkey rsa:2048 -keyout ~s -out ~s -days ~B -nodes -subj '~s' 2>&1",
- [KeyFile, CertFile, Days, Subject]
- ),
-
- _Output = os:cmd(Cmd),
+ DaysStr = integer_to_list(Days),
+ Args = ["req", "-x509", "-newkey", "rsa:2048",
+ "-keyout", KeyFile, "-out", CertFile,
+ "-days", DaysStr, "-nodes", "-subj", Subject],
+ Port = open_port({spawn_executable, os:find_executable("openssl")},
+ [{args, Args}, exit_status, stderr_to_stdout]),
+ collect_port_output(Port),🤖 Prompt for AI Agents
In `@apps/mumble_protocol/src/mumble_cert.erl` around lines 101 - 122, The
generate_self_signed_cert function currently builds a shell string with
CertFile, KeyFile, Days, and Subject and calls os:cmd/1, creating a
command-injection risk; fix by replacing the os:cmd usage with an exec-style
call (open_port/2 or erl's port with {spawn_executable, "openssl"} and {args,
[...]}) to pass CertFile, KeyFile, Days and Subject as separate argv entries
(or, if you must keep os:cmd, strictly validate/escape shell metacharacters for
Subject/CertFile/KeyFile and reject unsafe input); update
generate_self_signed_cert (and callers like ensure_auto_certs/2) to either
validate inputs or use open_port with explicit args so no shell interpolation
occurs and then keep the existing file existence checks.
| authenticating(Type, Msg, StateData) -> | ||
| logger:warning("State: authenticating~nUnhandled ~p~n~p~n~p", [ | ||
| Type, Msg, StateData | ||
| ]), | ||
| {stop, unhandled}. |
There was a problem hiding this comment.
Timeout during authentication is treated as an unhandled event.
The catch-all clause at line 191 matches all event types including timeout. When the ?TIMEOUT timer fires during authentication, this will {stop, unhandled} instead of a clean shutdown. Add an explicit timeout clause before the catch-all.
Suggested fix
+authenticating(timeout, _Msg, _StateData) ->
+ logger:warning("Authentication timeout"),
+ {stop, normal};
authenticating(Type, Msg, StateData) ->
logger:warning("State: authenticating~nUnhandled ~p~n~p~n~p", [
Type, Msg, StateData
]),
{stop, unhandled}.🤖 Prompt for AI Agents
In `@apps/mumble_protocol/src/mumble_server_conn.erl` around lines 191 - 195, The
catch-all clause in function authenticating/3 currently matches every event
(including the ?TIMEOUT timer) and returns {stop, unhandled}; add an explicit
clause that matches the timeout event (e.g., authenticating(?TIMEOUT, Msg,
StateData) or authenticating(timeout, Msg, StateData) depending on how ?TIMEOUT
is delivered) placed before the catch-all so the timeout case performs a clean
shutdown (for example returning {stop, timeout} or {stop, normal}) and does not
fall through to the unhandled logger/return in authenticating/3.
| handle(Session, <<1:3, Timestamp/bits>>) -> | ||
| handle_ping(Session, Timestamp, false); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd /tmp && find . -name "mumble_udp_proto.erl" -type f 2>/dev/null | head -5Repository: freke/erlmur
Length of output: 38
🏁 Script executed:
git ls-files | grep -i mumble | head -20Repository: freke/erlmur
Length of output: 945
🏁 Script executed:
# Search for the file in the repository
find . -name "*.erl" -type f 2>/dev/null | head -20Repository: freke/erlmur
Length of output: 944
🏁 Script executed:
git ls-files | grep -E "\.erl$"Repository: freke/erlmur
Length of output: 1836
🏁 Script executed:
# List the repository structure
ls -laRepository: freke/erlmur
Length of output: 1445
🏁 Script executed:
# Find the mumble protocol file
find . -type f -name "*mumble*" 2>/dev/nullRepository: freke/erlmur
Length of output: 1170
🏁 Script executed:
wc -l apps/mumble_protocol/src/mumble_udp_proto.erlRepository: freke/erlmur
Length of output: 104
🏁 Script executed:
cat -n apps/mumble_protocol/src/mumble_udp_proto.erl | head -100Repository: freke/erlmur
Length of output: 3415
🏁 Script executed:
# Search for handle_ping function
ast-grep --pattern $'handle_ping($_, $_, $_)'Repository: freke/erlmur
Length of output: 38
🏁 Script executed:
cat apps/mumble_protocol/proto/MumbleUDP.protoRepository: freke/erlmur
Length of output: 4256
🏁 Script executed:
# Check if there are any tests that would reveal the actual packet format
grep -r "1:3.*Timestamp\|handle_ping\|Ping" apps/mumble_protocol/test/ --include="*.erl" -A 3 | head -40Repository: freke/erlmur
Length of output: 2996
The pattern at line 34 captures the 5-bit target field in Timestamp, causing a match error at line 69.
When the first 3 bits match type 1, Timestamp/bits captures everything after those 3 bits. For a standard 9-byte ping packet (3 bits type + 5 bits target + 64 bits timestamp = 72 bits), Timestamp becomes 69 bits. The v1.5 branch then fails at line 69 with <<T:64>> = Timestamp because 69 ≠ 64.
Fix both occurrences:
Proposed changes
-handle(Session, <<1:3, Timestamp/bits>>) ->
- handle_ping(Session, Timestamp, false);
+handle(Session, <<1:3, _Target:5, Timestamp/binary>>) ->
+ handle_ping(Session, Timestamp, false); _ ->
- <<1:3, Timestamp/bits>>
+ <<1:3, 0:5, Timestamp/binary>>🤖 Prompt for AI Agents
In `@apps/mumble_protocol/src/mumble_udp_proto.erl` around lines 34 - 35, The
handler pattern currently captures the 5-bit target into Timestamp (<<1:3,
Timestamp/bits>>) which makes Timestamp 69 bits and breaks the later v1.5 unpack
(<<T:64>> = Timestamp); update the pattern in handle/2 (and the other similar
occurrence the comment mentions) to explicitly extract the 5-bit target and the
64-bit timestamp (e.g., match <<1:3, Target:5, T:64, _Rest/bits>> or at minimum
<<1:3, Target:5, Timestamp:64>>) so the v1.5 branch can safely do <<T:64>> =
Timestamp or use T directly; ensure any other clause that previously used
Timestamp/bits is adjusted consistently to bind Target and a 64-bit timestamp.
| encode(Num) when -4 < Num andalso Num < 0 -> | ||
| <<2#111111:6, (-Num):2>>; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Mumble protocol varint encoding specification negative numbers
💡 Result:
In Mumble’s custom voice-channel varint (not Protobuf varint), “negative numbers” are represented by encoding the bitwise NOT of the value and using special leading-byte patterns:
Decode rules (what the first byte means)
Let b0 be the first byte:
- If
(b0 & 0xFC) == 0xF8(i.e.111110xx), then the value is negative and is decoded as:
value = ~read_varint()(bitwise NOT of the following varint). [1] - If
(b0 & 0xFC) == 0xFC(i.e.111111xx), then this is an inline “small negative” and decodes as:
value = ~(b0 & 0x03)which yields exactly-1, -2, -3, -4forb0 = 0xFC, 0xFD, 0xFE, 0xFF. [1]
Encode rules (how to emit negative values)
When encoding a u64 that you intend to interpret as signed (two’s-complement):
- For
-1..-4(i.e.valuein0xFFFF...FFFCto0xFFFF...FFFF), encode as a single byte:
0xFC | (~value as u8)→-1→FC,-2→FD,-3→FE,-4→FF. [1] - For any other negative (i.e. top bit set,
value & 0x8000_0000_0000_0000 != 0), encode:
0xF8followed bywrite_varint(~value).
Example:-5asu64is...FFFB,~value = 4, so it encodes asF8 04. [1]
Source: This behavior is implemented (and thus precisely specified) by Mumble varint read/write logic here. [1]
References
[1] https://docs.rs/mumble-protocol/latest/src/mumble_protocol/varint.rs.html
Small-negative varint encoding violates Mumble protocol specification and breaks interoperability.
Lines 44–45 and 79–80 use an incorrect convention for encoding/decoding small negatives (−3…−1) instead of the standard Mumble range (−4…−1). The official spec defines these as:
- Encode:
−1 → 0xFC,−2 → 0xFD,−3 → 0xFE,−4 → 0xFF - Decode:
0xFC → −1,0xFD → −2,0xFE → −3,0xFF → −4
The current code instead produces −1 → 0xFD, −3 → 0xFF, and never encodes −4 or 0xFC. This causes:
- Incompatibility with standard Mumble clients/servers: packets sent by or to compliant implementations will be misinterpreted.
- −4 inefficiently encoded: uses 9 bytes instead of 1 byte.
- Crash on receipt of spec-compliant packets: when decoding
0xFC(which spec clients send for −1), the guardI > 0on line 79 excludes the match, falling through to line 81, which attempts to read 64 trailing bits and crashes if the packet is malformed or ends.
Fix by using the standard ~ (bitwise NOT) convention:
- Line 44: change guard to
Num >= -4 andalso Num < 0and use(-(Num + 1)):2 - Line 79: remove guard
when I > 0and use-(I + 1)for decoding
🤖 Prompt for AI Agents
In `@apps/mumble_protocol/src/mumble_varint.erl` around lines 44 - 45, The
small-negative varint handling in encode/decode is off-spec: update encode(Num)
to accept Num >= -4 andalso Num < 0 and encode using the bitwise NOT convention
by emitting (-(Num + 1)) into the 2-bit field (so −1..−4 map to 0xFC..0xFF), and
change the corresponding decode branch (the clause matching the 2-bit prefix
currently guarded with I > 0) to remove the I > 0 guard and return -(I + 1) when
decoding that 2-bit prefix; adjust the guards and arithmetic in the encode and
decode clauses that reference encode/Num and the decode variable I accordingly
so −1..−4 are encoded/decoded to 0xFC..0xFF per the Mumble spec.
| start_server(Options) when is_map(Options) -> | ||
| CertFile = maps:get(cert_file, Options, undefined), | ||
| KeyFile = maps:get(key_file, Options, undefined), | ||
| Port = maps:get(port, Options, ?DEFAULT_PORT), | ||
| AutoCreate = maps:get(auto_create_cert, Options, false), | ||
| ServerHandler = maps:get(server_handler, Options, mock_mumble_handler), | ||
|
|
||
| case {CertFile, KeyFile, AutoCreate} of | ||
| {undefined, undefined, true} -> | ||
| %% Auto-generate certificates | ||
| CertSubject = maps:get(cert_subject, Options, ?DEFAULT_CERT_SUBJECT), | ||
| CertDays = maps:get(cert_days, Options, ?DEFAULT_CERT_DAYS), | ||
| case mumble_cert:ensure_auto_certs(CertSubject, CertDays) of | ||
| {ok, AutoCertFile, AutoKeyFile} -> | ||
| ServerOpts = #{ | ||
| cert_file => AutoCertFile, | ||
| key_file => AutoKeyFile, | ||
| port => Port, | ||
| server_handler => ServerHandler | ||
| }, | ||
| mumble_server:start_server(ServerOpts); | ||
| {error, Reason} -> | ||
| {error, Reason} | ||
| end; | ||
| _ -> | ||
| %% Use provided files | ||
| ServerOpts = #{ | ||
| cert_file => CertFile, | ||
| key_file => KeyFile, | ||
| port => Port, | ||
| server_handler => ServerHandler | ||
| }, | ||
| mumble_server:start_server(ServerOpts) | ||
| end. |
There was a problem hiding this comment.
Default server_handler is mock_mumble_handler — a test module.
Line 130 defaults ServerHandler to mock_mumble_handler, and the positional start_server/2,3 variants on lines 168 and 178 hard-code this same default. A production API module should not default to a test mock. If no real handler exists yet, consider requiring the handler explicitly or raising an error when it's absent, rather than silently wiring in a mock.
Additionally, the catch-all branch on line 149 doesn't validate that both CertFile and KeyFile are non-undefined. If only one is provided (or neither, with auto_create_cert defaulting to false), undefined will be passed to mumble_server:start_server/1, likely causing a confusing failure downstream.
Proposed fix for validation
_ ->
- %% Use provided files
- ServerOpts = #{
- cert_file => CertFile,
- key_file => KeyFile,
- port => Port,
- server_handler => ServerHandler
- },
- mumble_server:start_server(ServerOpts)
+ %% Validate both cert files are provided
+ case {CertFile, KeyFile} of
+ {undefined, _} ->
+ {error, {missing_option, cert_file}};
+ {_, undefined} ->
+ {error, {missing_option, key_file}};
+ _ ->
+ ServerOpts = #{
+ cert_file => CertFile,
+ key_file => KeyFile,
+ port => Port,
+ server_handler => ServerHandler
+ },
+ mumble_server:start_server(ServerOpts)
+ end
end.🤖 Prompt for AI Agents
In `@apps/mumble_protocol/src/mumble.erl` around lines 125 - 158, The
start_server/1 function improperly defaults ServerHandler to the test module
mock_mumble_handler and fails to validate that both cert_file and key_file are
present when auto_create_cert is false; update the maps:get(server_handler, ...)
call to require an explicit handler (returning an error like {error,
missing_server_handler} or raising) instead of defaulting to
mock_mumble_handler, and add validation before calling
mumble_server:start_server/1 to ensure either AutoCreate=true or both CertFile
and KeyFile are not undefined (if one or both are missing return a clear {error,
missing_cert_or_key}), and apply the same handler-default/validation policy to
the other start_server/2 and start_server/3 variants that currently hard-code
the mock.
| accept(_Timeout, ListenSocket) -> | ||
| %% Mock implementation | ||
| {ok, {mock_accepted, ListenSocket}}. |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's locate and examine the mock_transport.erl file
find . -name "mock_transport.erl" -type fRepository: freke/erlmur
Length of output: 102
🌐 Web query:
Ranch transport callback accept signature Erlang
💡 Result:
In Ranch, the transport behavior callback for accepting connections is accept/2:
accept(LSocket :: socket(), Timeout :: timeout()) ->
{ok, Socket :: socket()}
| {error, closed | timeout | atom()}.You call it with the listening socket (returned by listen/1) and a timeout (ms); it returns the newly accepted connection socket (or an error). [1]
Concrete implementations follow the same signature, e.g. ranch_etls:accept/2 and ranch_ssl2:accept/2. [2][3]
Sources: [1] [2] [3]
🏁 Script executed:
#!/bin/bash
# Search for ranch transport callback specifications in the codebase
rg -n "callback accept" --type erlang
# Also search for any documentation or specs about accept
rg -n "accept.*listener.*timeout" --type erlang
# Look for other implementations or usages of accept/2
rg -n "accept\(" --type erlang -B 1 -A 2 | head -50Repository: freke/erlmur
Length of output: 350
🏁 Script executed:
#!/bin/bash
# Check for usages of the accept function being called
rg -n "accept\(" --type erlang -B 2 -A 2 | grep -E "(mock_transport|accept\()" | head -30
# Also check if mock_transport module is used anywhere
rg -n "mock_transport" --type erlangRepository: freke/erlmur
Length of output: 2718
🏁 Script executed:
#!/bin/bash
# Check how mumble_server_conn uses the transport module
find . -name "mumble_server_conn.erl" -type f
cat apps/mumble_protocol/src/mumble_server_conn.erl | head -100Repository: freke/erlmur
Length of output: 3026
🏁 Script executed:
#!/bin/bash
# Search for where accept is called with the transport module
rg -n "Transport:accept\|ranch.*accept\|accept\(.*Timeout" --type erlang -B 2 -A 2Repository: freke/erlmur
Length of output: 38
🏁 Script executed:
#!/bin/bash
# Search for where Transport module's accept is called
rg -n "Transport:" apps/mumble_protocol/src/mumble_server_conn.erl
# Also look for ranch protocol callback invocations
rg -n "init\|handle_continue" apps/mumble_protocol/src/mumble_server_conn.erl -A 5Repository: freke/erlmur
Length of output: 256
🏁 Script executed:
#!/bin/bash
# Let's see the init function of mumble_server_conn to understand Ranch protocol flow
sed -n '70,150p' apps/mumble_protocol/src/mumble_server_conn.erlRepository: freke/erlmur
Length of output: 2722
🏁 Script executed:
#!/bin/bash
# Look at the full init function and see if there's any Transport:accept call
sed -n '130,200p' apps/mumble_protocol/src/mumble_server_conn.erl
# Also check if Ranch automatically calls accept before starting the protocol
rg -n "accept\|ranch_server\|ranch:add_connection" --type erlangRepository: freke/erlmur
Length of output: 2517
🏁 Script executed:
#!/bin/bash
# Check the Ranch documentation or look for how the mock_transport.setup_ranch_mock works
cat apps/mumble_protocol/test/mock_transport.erlRepository: freke/erlmur
Length of output: 7331
Fix accept/2 parameter order to match Ranch transport callback signature.
Ranch's ranch_transport behavior specifies accept(listener(), timeout()), but the current implementation has parameters reversed as accept(_Timeout, ListenSocket). This violates the callback contract and will cause pattern-match failures if the callback is invoked by Ranch or used with production code.
Proposed fix
-accept(_Timeout, ListenSocket) ->
+accept(ListenSocket, _Timeout) ->
%% Mock implementation
{ok, {mock_accepted, ListenSocket}}.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| accept(_Timeout, ListenSocket) -> | |
| %% Mock implementation | |
| {ok, {mock_accepted, ListenSocket}}. | |
| accept(ListenSocket, _Timeout) -> | |
| %% Mock implementation | |
| {ok, {mock_accepted, ListenSocket}}. |
🤖 Prompt for AI Agents
In `@apps/mumble_protocol/test/mock_transport.erl` around lines 68 - 70, The
accept/2 function currently has its parameters reversed (accept(_Timeout,
ListenSocket)), which violates Ranch's ranch_transport callback signature
accept(Listener, Timeout); update the accept/2 definition to
accept(ListenSocket, Timeout) (and rename internal variables accordingly) so it
matches the expected order and return the same {ok, {mock_accepted,
ListenSocket}} tuple; also search for and adjust any internal or test calls to
accept/2 to use the corrected parameter order to avoid pattern-match failures.
| fun({{_S, Stats}, I}) -> | ||
| 1 = Stats#crypto_stats.good, | ||
| I = Stats#crypto_stats.late, | ||
| (N - 1 - I) =:= Stats#crypto_stats.lost | ||
| end, |
There was a problem hiding this comment.
Bug: =:= does not assert — use = for pattern match assertion.
Line 94 uses =:= (exact equality comparison) which evaluates to a boolean that is silently discarded. Unlike = on lines 92–93, this does not crash on mismatch, so the lost stat is never actually verified.
🐛 Proposed fix
fun({{_S, Stats}, I}) ->
1 = Stats#crypto_stats.good,
I = Stats#crypto_stats.late,
- (N - 1 - I) =:= Stats#crypto_stats.lost
+ (N - 1 - I) = Stats#crypto_stats.lost
end,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fun({{_S, Stats}, I}) -> | |
| 1 = Stats#crypto_stats.good, | |
| I = Stats#crypto_stats.late, | |
| (N - 1 - I) =:= Stats#crypto_stats.lost | |
| end, | |
| fun({{_S, Stats}, I}) -> | |
| 1 = Stats#crypto_stats.good, | |
| I = Stats#crypto_stats.late, | |
| (N - 1 - I) = Stats#crypto_stats.lost | |
| end, |
🤖 Prompt for AI Agents
In `@apps/ocb128_crypto/test/prop_ocb128_crypto.erl` around lines 91 - 95, The
anonymous function verifying crypto stats uses the equality operator =:= which
returns a boolean and doesn't fail the test; change the comparison "(N - 1 - I)
=:= Stats#crypto_stats.lost" to a pattern match using "=" so mismatches raise an
error and actually assert the lost value (update the expression involving N, I
and Stats#crypto_stats.lost inside the fun({{_S, Stats}, I}) -> ... end).
There was a problem hiding this comment.
Actionable comments posted: 10
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/ocb128_crypto/test/ocb128_crypto_benchmark_SUITE.erl (1)
3-11:⚠️ Potential issue | 🔴 CriticalBug: Individual benchmark test functions are not exported — CT will fail when
RUN_BENCHMARKS=true.
all/0returns atoms likeencrypt_16b,decrypt_16b, etc. viamaps:keys(test_params()), but the corresponding functions (Lines 78–96) are not in the-exportlist. Common Test requires test case functions to be exported to invoke them.🐛 Proposed fix — add missing exports
-export([ all/0, groups/0, init_per_suite/1, end_per_suite/1, init_per_testcase/2, end_per_testcase/2, - run_benchmark/2 + run_benchmark/2, + encrypt_16b/1, encrypt_32b/1, encrypt_128b/1, encrypt_1kb/1, + encrypt_4kb/1, encrypt_16kb/1, encrypt_128kb/1, encrypt_1mb/1, encrypt_4mb/1, + decrypt_16b/1, decrypt_32b/1, decrypt_128b/1, decrypt_1kb/1, + decrypt_4kb/1, decrypt_16kb/1, decrypt_128kb/1, decrypt_1mb/1, decrypt_4mb/1 ]).Also applies to: 78-96
🤖 Fix all issues with AI agents
In `@apps/erlmur/src/erlmur_server_handler.erl`:
- Around line 61-68: The PermissionQuery handler in function handle_msg
currently grants full admin rights by sending permissions => 16#FFFFFFFF; change
this to a minimal safe permission bitmask (e.g. only read/speak/text bits)
before sending via mumble_server_conn:send in the handle_msg clause that matches
#{message_type := 'PermissionQuery'} so clients are not given
kick/ban/manage/channel rights; update the permissions value and add a short
comment explaining the reduced default.
In `@apps/erlmur/src/erlmur_user_manager.erl`:
- Around line 48-54: The specs for get_user/1 and get_all_users/0 reference the
non-exported record `#user`{}, making them unusable to callers; update the
implementation and specs to either export the record or (preferred) return maps:
change the spec types to {ok, map()} | {error, not_found} for get_user/1 and
[map()] for get_all_users/0, and convert any returned `#user`{} values to maps
before returning (or replace internal usage of `#user`{} with maps). Update any
pattern matches or helper functions that construct/return `#user`{} (e.g., the
gen_server handlers that return {get_user, SessionId} and get_all_users) to
build maps so the public API and specs remain consistent.
In `@apps/mumble_protocol/include/mumble_protocol.hrl`:
- Around line 73-79: Remove the unused record field listening_volume_adjust from
the record definition in mumble_protocol.hrl (it is redundant with the active
field listening_volume_adjustment); update the record tuple so only
listening_volume_adjustment remains, run a project-wide search for
listening_volume_adjust to ensure there are no remaining references, and update
any comments or docs that mention the removed field.
In `@apps/mumble_protocol/src/mumble_server_conn.erl`:
- Around line 447-457: send_msg/2 can return {error, Reason} but all its callers
ignore that result, so send failures (especially during handshake messages like
CryptSetup or ServerSync) leave the connection inconsistent; update send_msg
callers to check its return and propagate or handle errors: where functions such
as the handshake/auth routines invoke send_msg, change their code to
pattern-match on ok vs {error, Reason} and on error either log and close the
socket/terminate the connection or return {error, Reason} up the call chain so
the connection state is not advanced; ensure send_msg remains returning ok |
{error, Reason} and that the calling functions propagate the error (or perform
cleanup) rather than discarding it.
- Around line 244-250: Replace direct calls to
erlmur_user_manager:register_udp_addr/2 with an invocation on the configurable
handler module so mumble_protocol has no hard dependency on erlmur_user_manager;
specifically, in mumble_server_conn.erl (NewStateData block) and the two places
in mumble_udp_server.erl, call the handler module stored on the connection state
(e.g. StateData#state.handler_mod or StateData.handler_mod) like
HandlerMod:register_udp_addr(SessionId, Addr) (or invoke via apply/3 if needed)
and remove the direct erlmur_user_manager reference so user-manager behavior is
routed through the handler_mod callback.
In `@apps/mumble_protocol/src/mumble_server_sup.erl`:
- Line 61: The supervisor currently defaults ServerHandler to the test-only mock
via proplists:get_value(server_handler, Options, mock_mumble_handler); change
this so production can't silently use the mock—either remove the default (use
proplists:get_value(server_handler, Options) and explicitly handle the undefined
case) or replace the default with a sentinel (e.g. undefined_server_handler) and
make the supervisor's init/start function validate ServerHandler and crash with
a clear error if it's not provided or is the sentinel; update the code paths
that reference ServerHandler so they rely on the validated value.
In `@apps/mumble_protocol/src/mumble_udp_server.erl`:
- Around line 141-143: The handler handle_info/2 currently logs every incoming
UDP packet with logger:info("UDP Message ~p", [Msg]) which will flood production
logs (especially for voice packets) and also uses a tab for indentation; change
the log level to debug or remove the per-packet log (e.g., use logger:debug/2)
and only log at info on notable events or errors, keep message variable Msg for
context if needed, and normalize the line indentation to match the file’s spaces
style so handle_info/2 and the Addr = {IP, Port} line use consistent spacing.
In `@apps/mumble_protocol/src/mumble_varint.erl`:
- Around line 17-20: The moduledoc and implementation incorrectly document and
use the large-negative varint prefix 0xFC (11111100) which overlaps the
small-negative range 111111xx; update the documentation and any constants/logic
in mumble_varint.erl to use 0xF8 (11111000) as the large-negative prefix and
make sure the encode/decode paths (the varint handling functions in this module)
implement the protocol: when encountering 111110xx, follow with the recursive
varint of the bitwise-NOT value (not a fixed 64-bit payload), and adjust any
pattern-matching, prefix constants, and tests to reflect 0xF8/111110xx for large
negatives and 111111xx (0xFC–0xFF) for the small-negative two-bit values.
In `@apps/mumble_protocol/test/mumble_cert_tests.erl`:
- Around line 1-202: Add real unit tests that call the public API in mumble_cert
instead of only the local OpenSSL helpers: write tests that (1) call
mumble_cert:generate_self_signed_cert(TestDirCertPath, TestDirKeyPath, Subject,
Days) and assert the cert/key files are created and OpenSSL reports valid X.509
dates/subject, (2) call mumble_cert:validate_cert_files(CertPath, KeyPath) to
assert it returns ok for valid files and an error tuple when files are missing
or malformed, and (3) call mumble_cert:ensure_auto_certs(Dir, Options) to assert
it creates cert/key when absent, reuses existing files (mtime unchanged) and
regenerates missing ones; use the existing
get_test_dir/ensure_test_dir/cleanup_test_dir helpers to manage temp dirs and
filelib:is_file/file:read_file_info to check existence and mtimes, referencing
the functions mumble_cert:generate_self_signed_cert/4,
mumble_cert:validate_cert_files/2, and mumble_cert:ensure_auto_certs/2 in your
new tests.
In `@rebar.config`:
- Line 2: The test profile's erl_opts is overwriting the base erl_opts and
losing include paths; update the test profile's {erl_opts, ...} to merge base
options by including debug_info and the include tuple {i,
"apps/mumble_protocol/include"} (instead of the hardcoded _build path) so gpb
headers generated per gpb_opts are found, or remove the redundant {i,
"_build/default/plugins/gpb/include"} from the base erl_opts; change references
to erl_opts in rebar.config and ensure the test profile keeps export_all and
nowarn_export_all plus debug_info and {i, "apps/mumble_protocol/include"}.
🟡 Minor comments (22)
apps/erlmur/src/erlmur_sup.erl-24-31 (1)
24-31:⚠️ Potential issue | 🟡 MinorMisleading log: "User manager started" is emitted before the child is actually started.
init/1only builds the child spec and returns it to the supervisor framework. The actual child process is started afterinit/1returns. Line 31 will log "started" prematurely — or even if the child subsequently fails to start.Proposed fix — remove the misleading log
}, - logger:info("[erlmur_sup] User manager started"), {ok, {{one_for_one, 5, 10}, [UserManager]}}.apps/erlmur/src/erlmur_server_handler.erl-78-86 (1)
78-86:⚠️ Potential issue | 🟡 Minor
erlang:system_info(otp_release)returns a charlist, not a binary.All other fields in the map (
release,os) are binaries, and the protocol expects consistent types. Wrap withlist_to_binary/1.Proposed fix
- os_version => erlang:system_info(otp_release) + os_version => list_to_binary(erlang:system_info(otp_release))apps/mumble_protocol/test/mumble_cert_tests.erl-138-158 (1)
138-158:⚠️ Potential issue | 🟡 Minor
reuse_existing_cert_testdoes not test any reuse logic.This test generates a certificate, reads its mtime, sleeps 100ms, then reads the mtime again and asserts it hasn't changed. Since nothing attempted to regenerate the certificate in between, this assertion is trivially true. To meaningfully test reuse, call
mumble_cert:ensure_auto_certs/2twice and verify the second call returns the same files without regeneration.AGENTS.md-65-65 (1)
65-65:⚠️ Potential issue | 🟡 MinorIncorrect Erlang syntax:
try...of...elsedoes not exist.Erlang's
tryexpression usestry Expr of Pattern -> Body catch Class:Reason -> Handler end. There is noelseclause. This should readtry...of...catch(ortry...catch...after...end).Proposed fix
-- Use `try...of...else` for exceptions that need cleanup +- Use `try...of...catch...after...end` for exceptions that need cleanupapps/mumble_protocol/src/mumble_udp_server.erl-38-40 (1)
38-40:⚠️ Potential issue | 🟡 Minor
send/3spec claims{error, term()}butgen_server:cast/2always returnsok.The spec
-spec send(...) -> ok | {error, term()}.is misleading —gen_server:castis fire-and-forget and always returnsok. The actual send error is logged insidehandle_castbut never propagated to the caller. Either update the spec to-> okor switch togen_server:callif the caller needs error feedback.Proposed fix (if fire-and-forget is intentional)
--spec send(IP :: inet:ip_address(), inet:port_number(), binary()) -> ok | {error, term()}. +-spec send(IP :: inet:ip_address(), inet:port_number(), binary()) -> ok.apps/mumble_protocol/src/mumble_cert.erl-127-133 (1)
127-133:⚠️ Potential issue | 🟡 Minor
file:make_dir/1won't create intermediate directories.If the parent path doesn't exist (e.g., priv dir not yet created),
file:make_dir/1will fail with{error, enoent}. Usefilelib:ensure_dir/1which creates all intermediate directories for a given file path.Proposed fix
ensure_cert_dir(CertFile) -> - Dir = filename:dirname(CertFile), - case file:make_dir(Dir) of - ok -> ok; - {error, eexist} -> ok; - {error, Reason} -> {error, {dir_creation_failed, Reason}} + case filelib:ensure_dir(CertFile) of + ok -> ok; + {error, Reason} -> {error, {dir_creation_failed, Reason}} end.AGENTS.md-20-93 (1)
20-93:⚠️ Potential issue | 🟡 MinorFix markdown formatting: add blank lines around headings and fenced code blocks.
The markdownlint tool flags multiple MD022 and MD031 violations throughout the file. Each
###heading needs a blank line both above and below it, and fenced code blocks need blank lines surrounding them. For example, lines 20, 25, 31, 37, 45–46, 57, 63, 69, 76, 82, 88 all need an extra blank line inserted.Example fix for one section
### Imports and Includes + - Use `-include("module.hrl")` for local recordsApply the same pattern (blank line after heading, blank lines around fenced code blocks) to all flagged headings.
apps/mumble_protocol/src/mumble_client.erl-133-141 (1)
133-141:⚠️ Potential issue | 🟡 MinorDocument the ownership model: client processes are linked to their parent and will terminate if the parent exits.
The
mumble_client_conn:start_link/3creates a bidirectional link between the calling process and the client connection process. This coupling is intentional—the client stores the parent PID and sends messages back to it throughout its lifecycle. However, this behavior is not documented in the public API. Callers must understand that:
- If the caller process crashes, the client connection terminates with it
- If the client connection crashes, it propagates to the caller
Add documentation to
mumble_client.erl:start_client/5clarifying this ownership requirement, or consider usinggen_statem:start/4instead ofstart_linkif looser coupling is preferred.apps/mumble_protocol/include/mumble_protocol.hrl-85-85 (1)
85-85:⚠️ Potential issue | 🟡 MinorConfusing type spec:
regular | type— istypethe intended atom?The type annotation
:: regular | typeuses the atomtypeas a variant, which is both confusing (looks like a keyword) and likely not the intended domain value. Iftypeis genuinely a valid session kind in the Mumble protocol, consider renaming it to something more descriptive (e.g.,server_loopback,bot, etc.) to avoid ambiguity.apps/mumble_protocol/README.md-37-37 (1)
37-37:⚠️ Potential issue | 🟡 MinorTypo: "reciveing" → "receiving".
✏️ Proposed fix
-If reciveing a `UDPTunnel` message the connection is marked "UDP verified" false, until new UDP verification is done. +If receiving a `UDPTunnel` message the connection is marked "UDP verified" false, until new UDP verification is done.apps/mumble_protocol/README.md-31-31 (1)
31-31:⚠️ Potential issue | 🟡 MinorTypo: "recieved" → "received".
Also, "30sec" should be "30 seconds" or "30 s" for readability.
✏️ Proposed fix
-If no `Ping` is recieved for 30sec the client will be disconnected from the server. +If no `Ping` is received for 30 seconds the client will be disconnected from the server.apps/mumble_protocol/README.md-45-45 (1)
45-45:⚠️ Potential issue | 🟡 MinorTypo: "explicitely" → "explicitly".
✏️ Proposed fix
-- **Automatic Fallback**: If a server hasn't received a UDP packet from a client recently (or if the client explicitely +- **Automatic Fallback**: If a server hasn't received a UDP packet from a client recently (or if the client explicitlyapps/mumble_protocol/src/mumble_server_sup.erl-57-79 (1)
57-79:⚠️ Potential issue | 🟡 MinorNo validation of required options —
undefinedvalues will cause child crashes.If
port,cert_file, orkey_fileare missing fromOptions,proplists:get_valuereturnsundefined, which will propagate tomumble_udp_server:start_link(undefined)andranch:start_listenerwith undefined cert/key paths. This produces confusing downstream errors. Validate required options upfront ininit/1.apps/mumble_protocol/src/mumble_msg.erl-519-532 (1)
519-532:⚠️ Potential issue | 🟡 Minor
from_mapfor'Ping'injectserlang:system_time(second)as a default — hidden side effect.A conversion function shouldn't have time-dependent side effects. This means calling
from_map(#{message_type => 'Ping'})twice yields records with different timestamps, and round-trippingto_map → from_mapwon't preserveundefined— it silently fills in "now". Useundefinedas the default and let the caller set the timestamp explicitly.- timestamp = maps:get(timestamp, Map, erlang:system_time(second)), + timestamp = maps:get(timestamp, Map, undefined),apps/mumble_protocol/test/server_logic_tests.erl-16-29 (1)
16-29:⚠️ Potential issue | 🟡 MinorMissing
process_flag(trap_exit, true)— linked process crash will take down test.
start_linklinks the spawnedgen_statemto the test process. If the server crashes (e.g., mock setup issue), the test process dies with it instead of producing a clear test failure. The PR objectives mention addingtrap_exitto fix EUnit failures, but this file doesn't include it.Proposed fix (apply to all test funs)
{"Server connection initialization", fun() -> + process_flag(trap_exit, true), mock_transport:setup_ranch_mock(),apps/mumble_protocol/test/mumble_SUITE.erl-238-241 (1)
238-241:⚠️ Potential issue | 🟡 Minor
element(3, State)is fragile — breaks silently ifmumble_udp_server's record changes.Accessing internal gen_server state by tuple position is brittle. If a field is added before
Portin the#state{}record ofmumble_udp_server, this index becomes wrong without any compile-time warning.Consider using a dedicated API on
mumble_udp_serverto retrieve the port, or usingsys:get_state/1with a record-aware pattern if the record is accessible.apps/mumble_protocol/test/mock_transport.erl-176-185 (1)
176-185:⚠️ Potential issue | 🟡 MinorBlocking
receiveinsidehandle_callfreezes the gen_server.Lines 180-184 perform a raw
receiveinside ahandle_callcallback, which blocks the entire gen_server process until a message arrives or the timeout expires. During this time, all other calls (e.g.,send,setopts,get_sent_messages) will queue and potentially time out. This can cause hard-to-debug test hangs.Consider using a continuation pattern: if no message is in the queue, return
{error, empty}or store the caller'sFromand reply asynchronously when a message is injected.apps/mumble_protocol/test/mumble_SUITE.erl-287-302 (1)
287-302:⚠️ Potential issue | 🟡 MinorMultiple test cases are stubs that don't test their stated purpose.
client_connect_disconnect_test,client_with_callback_test,client_server_communication_test, andmultiple_clients_testonly start and stop the server — they never create a client, send messages, or test callbacks. These pass trivially and give a false sense of coverage.Consider either implementing the actual test logic or renaming/marking them as
{skip, "not yet implemented"}to be honest about coverage.Also applies to: 305-320, 456-471, 474-489
apps/mumble_protocol/src/mumble_client_conn.erl-180-184 (1)
180-184:⚠️ Potential issue | 🟡 MinorUDP ping timer runs every 5 seconds but
send_udp_ping/1is a no-op stub.The timer at line 183 re-arms every 5 seconds and at line 194 starts immediately on entering
established, butsend_udp_ping/1(line 217) andsend_udp/2(line 222) only log debug messages. This creates unnecessary timer overhead and misleadingudp_verifiedstate (line 188 would setudp_verified = trueif a UDP packet arrives, but none can since nothing is sent).Consider either implementing the UDP send path or removing the timer until it's functional.
Also applies to: 217-224
apps/mumble_protocol/test/test_utils.erl-85-95 (1)
85-95:⚠️ Potential issue | 🟡 MinorNegative timeout causes
badarg—Timeout - Elapsedcan go below zero.If processing a matched message takes longer than the remaining
Timeout,Timeout - Elapsedbecomes negative. Erlang'sreceive ... afterclause requires a non-negative integer, so a negative value will crash withbadarg.Proposed fix
wait_for_messages(Pattern, Timeout, Acc) -> Start = erlang:monotonic_time(millisecond), receive Pattern = Msg -> Elapsed = erlang:monotonic_time(millisecond) - Start, - wait_for_messages(Pattern, Timeout - Elapsed, [Msg | Acc]) + wait_for_messages(Pattern, max(0, Timeout - Elapsed), [Msg | Acc]) after Timeout -> lists:reverse(Acc) end.apps/mumble_protocol/test/mock_transport.erl-223-239 (1)
223-239:⚠️ Potential issue | 🟡 MinorSSL notifications use
self()(gen_server pid) instead of#mock_socket{}— tests may fail to pattern-match.Real SSL sends
{ssl, SslSocket, Data}whereSslSocketis the socket reference. Here,self()returns the raw gen_server pid, but callers (likemumble_server_conn) likely expect the#mock_socket{pid = Pid}record or a reference that matches whathandshakereturned. This mismatch can cause unhandled message warnings or silent message drops in tests.Proposed fix
handle_cast({inject_message, Data}, State = #{owner := Owner, opts := Opts}) -> NewQueue = case State of #{message_queue := Queue} -> Queue ++ [Data]; _ -> [Data] end, + Socket = `#mock_socket`{pid = self()}, %% Check if we should notify the owner (active mode) case lists:keyfind(active, 1, Opts) of {active, true} when Owner =/= undefined -> - Owner ! {ssl, self(), Data}; + Owner ! {ssl, Socket, Data}; {active, once} when Owner =/= undefined -> - Owner ! {ssl, self(), Data}; + Owner ! {ssl, Socket, Data}; _ -> ok end,apps/mumble_protocol/src/mumble_server.erl-87-95 (1)
87-95:⚠️ Potential issue | 🟡 MinorConvert
os_versionto a binary to match other string fields in the version record.
erlang:system_info(otp_release)returns a charlist, butreleaseandosfields are binaries. All test fixtures and documentation examples expectos_versionas a binary. This inconsistency will cause type mismatches during encoding.Fix
- os_version = erlang:system_info(otp_release) + os_version = list_to_binary(erlang:system_info(otp_release))
🧹 Nitpick comments (30)
.github/workflows/erlang.yml (1)
17-18: Consider pinning the Erlang container image to a specific OTP version.Using
erlang:latestmeans CI builds can break silently when a new OTP major version is released with breaking changes. Pinning (e.g.,erlang:27) gives reproducible builds while still allowing controlled upgrades.apps/erlmur/src/erlmur_server_handler.erl (1)
26-34: MVP authentication accepts all users with no validation — ensure this is tracked.No password check, no certificate validation, no rate limiting. The
logger:warningon invalid auth is good, but there's no mechanism to reject or limit connections. Make sure there's a follow-up task to add real authentication before any non-local deployment.apps/erlmur/src/erlmur.erl (1)
14-15:register_user/1is a no-op stub — consider removing or wiring it up.This function returns
okwithout doing anything. The actual user registration is handled byerlmur_user_manager:register_user/2. Keeping this dead code in the public API module is misleading.apps/erlmur/src/erlmur_user_manager.erl (3)
81-86: Monitor reference is discarded — consider storing it for cleanup.
monitor(process, Pid)returns a reference that's dropped. If you ever need todemonitor(e.g., on explicit unregister via cast), you won't be able to cancel the monitor, potentially causing a spuriousDOWNhandling after the user is already removed inunregister_user.Sketch: store monitor ref in the user record
-record(user, { session_id :: pos_integer(), pid :: pid(), username :: binary(), - udp_addr :: {inet:ip_address(), inet:port_number()} | undefined + udp_addr :: {inet:ip_address(), inet:port_number()} | undefined, + mon_ref :: reference() | undefined }).Then in
register_user:+ MonRef = monitor(process, Pid), - User = `#user`{session_id = SessionId, pid = Pid, username = Username}, + User = `#user`{session_id = SessionId, pid = Pid, username = Username, mon_ref = MonRef},And in
unregister_user, calldemonitor(MonRef, [flush])before deleting.
107-115:unregister_usercast doesn't demonitor — may trigger a spuriousDOWNhandler.After explicitly unregistering, the monitor is still active. When the process eventually exits,
handle_info('DOWN', ...)will fire, scan the ETS table (user already deleted), and silently succeed — but this is wasted work and could be confusing during debugging. Pair this with storing the monitor ref (see previous comment).
157-181:DOWNhandler scans entire ETS table viafoldl— O(n) per disconnect.For an MVP with a small number of users this is fine, but it won't scale. A secondary ETS index on
pid(or storing the monitor ref mapped to session_id) would give O(1) cleanup.apps/ocb128_crypto/include/ocb128_crypto.hrl (1)
1-6: Add an include guard to prevent duplicate record definitions.If this header is included both directly and transitively (e.g., via another
.hrl), the compiler will error on the duplicate-recorddefinition. Standard Erlang practice is to wrap headers with-ifndef/-define/-endif.Proposed fix
+-ifndef(OCB128_CRYPTO_HRL). +-define(OCB128_CRYPTO_HRL, true). + -record(crypto_stats, { good = 0 :: non_neg_integer(), late = 0 :: non_neg_integer(), lost = 0 :: integer() }). -type crypto_stats() :: `#crypto_stats`{}. + +-endif.apps/mumble_protocol/src/mumble_client_behaviour.erl (1)
44-68: Consider adding aterminate/1callback for connection-close cleanup.Currently there's no way for the handler to be notified when the connection closes (gracefully or due to error). A callback like
terminate(Reason, State) -> okwould let implementations release resources or log disconnection. This can be deferred, but it's a common pattern in OTP-style behaviours.rebar.config (1)
34-46: All dependencies use unpinned branch references — builds are not reproducible.
ranch,meck,proper, andeqwalizer_supportall point to{branch, "master"}or{branch, "main"}. A breaking change or force-push upstream will silently break your build. Pin to a specific tag or commit hash for reproducibility.Example
- {ranch, {git, "https://github.com/ninenines/ranch.git", {branch, "master"}}}, + {ranch, {git, "https://github.com/ninenines/ranch.git", {tag, "2.1.0"}}},apps/mumble_protocol/src/mumble_version.erl (1)
37-46: Ambiguity in V1/V2 auto-detection for low version numbers.The guard-based heuristic (
V1 =< 16#FFFFFFFFvsV2 > 16#FFFFFFFF) works for real Mumble versions (1.x+) but would misinterpret a V2-encoded version like0.0.1(value65536, within 32-bit range) as a V1 value, decoding it as1.0.0instead.If this function is only ever called with known-format inputs, consider either:
- Splitting into
decode_v1/1anddecode_v2/1, or- Adding a
@docnote that the heuristic assumes Major ≥ 1.This is unlikely to matter in practice for the Mumble protocol.
apps/mumble_protocol/test/client_logic_tests.erl (4)
312-351: Multiple tests are no-ops that always pass — they provide no coverage.
established_transition_test,client_close_test, andinvalid_msg_testall just?assert(true). Similarly,session_id_testextracts a value from a map it constructs locally, andestablished_receive_testsends a message to itself. None of these exercisemumble_client_connbehavior.Consider either implementing these with a mock transport or removing them and tracking them as TODOs in an issue, so they don't give a false sense of coverage.
Would you like me to open an issue to track implementing these stub tests with proper mock transport support?
407-431: Error-handling tests don't test error handling.
network_error_testasserts that atoms are atoms,ssl_close_testpattern-matches a tuple it constructs, andinvalid_msg_testassertstrue. These do not exercise anymumble_client_connerror paths.
130-211: Connection tests always pass regardless of outcome — no assertions on behavior.Tests like
init_test,init_with_options_test, andconnection_success_testaccept both{ok, _}and{error, _}results and returnokfor either case. They exercise the code path but never assert expected behavior, so they can't catch regressions.
365-375:send_voice_testonly destructures a local tuple — no module code is exercised.This test creates a
voice_datatuple and asserts the values it just set. It doesn't call anymumble_client_connormumble_udp_protofunction.apps/ocb128_crypto/test/ocb128_crypto_benchmark_SUITE.erl (1)
18-22: Inconsistent indentation — tabs used insideall/0while the rest of the file uses spaces.✏️ Proposed fix
all() -> - case os:getenv("RUN_BENCHMARKS") of - "true" -> maps:keys(test_params()); - _ -> [] - end. + case os:getenv("RUN_BENCHMARKS") of + "true" -> maps:keys(test_params()); + _ -> [] + end.apps/ocb128_crypto/test/prop_ocb128_crypto.erl (1)
47-47: Nit: Typo in variable nameEncyptedMsgs— should beEncryptedMsgs.The variable is consistently named (also on Line 57, 67), so it compiles fine, but it's a readability nit.
apps/mumble_protocol/test/server_logic_tests.erl (1)
101-119: Test name "Invalid state transitions" doesn't test an invalid transition.The test sends a
voice_datamessage and asserts the process is still alive — this is the same asvoice_data_test_/0. There's no actual invalid state transition being triggered (e.g., sending a message that's invalid for the current state). Consider either renaming to reflect what it actually tests or adding a genuinely invalid event.apps/mumble_protocol/src/mumble_server_sup.erl (2)
77-78: Log messages are premature — children haven't started yet.These log lines execute during
init/1, before the supervisor has actually started the children. They're misleading. The supervisor starts children afterinit/1returns. Consider removing them or moving the logging into the child start functions.
81-96:start_tcp_listener/5lacks TLS hardening options.The TLS
socket_optsonly specifyport,certfile, andkeyfile. For a production Mumble server, consider adding TLS version constraints and cipher suite restrictions (e.g.,{versions, ['tlsv1.2', 'tlsv1.3']}) to prevent downgrade attacks. This can be deferred, but is worth noting.apps/mumble_protocol/test/mumble_api_tests.erl (1)
9-15: Fragile OpenSSL detection —whichoutput varies by platform.
os:cmd("which openssl")may return an error message (not empty) on systems wherewhichis available but OpenSSL is not, andwhichdoesn't exist on Windows. A more reliable check would be to runopenssl versionand check the exit code or output.Suggested improvement
setup() -> - case os:cmd("which openssl") of - [] -> error(openssl_not_available); - _ -> ok - end, + case os:cmd("openssl version") of + "OpenSSL" ++ _ -> ok; + "LibreSSL" ++ _ -> ok; + _ -> error(openssl_not_available) + end, ok.apps/mumble_protocol/src/mumble_tcp_proto.erl (2)
137-144:find_msg_by_tag/1doesn't short-circuit —lists:foldlalways traverses the full list.After finding a match, the fold continues iterating the rest of
MESSAGE_TABLE. Uselists:search(asfind_msg_by_recordalready does) orlists:keyfindfor early termination.Proposed fix
find_msg_by_tag(Tag) -> - lists:foldl(fun ({T, Record}, _Acc) when T =:= Tag -> - {ok, Tag, Record}; - (_, Acc) -> - Acc - end, - error, - ?MESSAGE_TABLE). + case lists:keyfind(Tag, 1, ?MESSAGE_TABLE) of + {Tag, Record} -> {ok, Tag, Record}; + false -> error + end.
98-108:pack/1spec claimsbinary()return but raises on unknown type.The
-spec pack(map()) -> binary()doesn't reflect theerror({unknown_type, Record})crash path. Consider either returning{ok, binary()} | {error, term()}or documenting the crash behavior in the doc string.apps/mumble_protocol/src/mumble_msg.erl (1)
499-499: No catch-all clause infrom_map/1— unknownmessage_typecrashes withfunction_clause.If a map with an unrecognized
message_typeis passed, the crash provides no context. Consider adding a catch-all that returns a clear error.Suggested addition after line 774
from_map(#{message_type := Type}) -> error({unknown_message_type, Type}); from_map(Map) -> error({missing_message_type, Map}).apps/mumble_protocol/src/mumble_server.erl (1)
113-130: Supervisor started viastart_linkis not part of a supervision tree.
mumble_server_sup:start_link(SupOptions)links the new supervisor to the calling process. If that process exits (e.g., a shell session or a short-lived caller), the entire server supervision tree is torn down. Consider whetherstart_linkis the right call here, or whether the supervisor should be started under a top-level application supervisor.apps/mumble_protocol/src/mumble_client_conn.erl (2)
52-57: Redundant case expression —gen_statem:start_linkalready returns the matched tuples.Proposed simplification
start_link(Host, Port, Opts) -> Parent = self(), - case gen_statem:start_link(?MODULE, {Host, Port, Opts, Parent}, []) of - {ok, Pid} -> {ok, Pid}; - {error, Reason} -> {error, Reason} - end. + gen_statem:start_link(?MODULE, {Host, Port, Opts, Parent}, []).
130-134: Stats field namesfrom_client_tcp_*are misleading in a client module.These fields track packets received from the server by the client, but the field names suggest traffic originating from the client. This appears to be a reuse of the server-side
#stats{}record without adapting the semantics.Also applies to: 154-158
apps/mumble_protocol/test/test_utils.erl (1)
68-74:wait_for_message/2only supports exact term equality, not Erlang pattern matching.Since
Patternis a bound variable in the receive clause, it performs exact value comparison (not structural pattern matching with wildcards). For example,wait_for_message({mumble_msg, '_'}, 1000)would only match the literal atom'_', not any message. The doc and function name suggest pattern matching, which is misleading.This is an inherent limitation of Erlang's receive with bound variables — just worth noting in the doc or function name (e.g.,
wait_for_exact_message).apps/ocb128_crypto/src/ocb128_crypto.erl (1)
14-17:encrypt_ocb/3anddecrypt_ocb/3are exported but appear to be internal primitives.These are low-level OCB block cipher operations without public documentation. If they're exported only for testing or benchmarking, consider moving them to a test-only export or documenting them. Exporting them widens the public API surface that must remain stable.
apps/mumble_protocol/test/mumble_SUITE.erl (2)
520-576: Unused helper functions — dead code.
get_server_port/1,get_ranch_port/1,find_port_in_listeners/1, andwait_for_client_connection/2are defined but never called by any test case. They add maintenance burden and clutter. Remove them or wire them into actual tests.
89-96: Fragile OpenSSL error detection — only matches"Error"prefix.OpenSSL command failures may produce output starting with
"unable to","Can't open","problems making Certificate Request", etc. Checking only for"Error" ++ _misses most failure modes. A more reliable approach is to check the file existence (which you already do at lines 99-100) and drop this fragile prefix check.Proposed fix — rely on file existence check
Cmd = io_lib:format( "openssl req -x509 -newkey rsa:2048 -keyout ~s -out ~s -days 1 -nodes -subj '/CN=localhost' 2>&1", [KeyFile, CertFile] ), - case os:cmd(Cmd) of - "Error" ++ _ -> ct:fail(cert_generation_failed); - _ -> ok - end, + os:cmd(Cmd), %% Verify certs were created true = filelib:is_file(CertFile), true = filelib:is_file(KeyFile),
| handle_msg(#{message_type := 'PermissionQuery'}, State) -> | ||
| %% Respond with default permissions for MVP | ||
| mumble_server_conn:send(self(), #{ | ||
| message_type => 'PermissionQuery', | ||
| channel_id => 0, | ||
| permissions => 16#FFFFFFFF %% All permissions | ||
| }), | ||
| {ok, State}; |
There was a problem hiding this comment.
Security: granting all permissions unconditionally.
16#FFFFFFFF gives every client full admin rights (kick, ban, move, manage channels, etc.). Even for an MVP, this is risky if exposed to untrusted networks. Consider returning a minimal read/speak/text permission set now and expanding later.
🤖 Prompt for AI Agents
In `@apps/erlmur/src/erlmur_server_handler.erl` around lines 61 - 68, The
PermissionQuery handler in function handle_msg currently grants full admin
rights by sending permissions => 16#FFFFFFFF; change this to a minimal safe
permission bitmask (e.g. only read/speak/text bits) before sending via
mumble_server_conn:send in the handle_msg clause that matches #{message_type :=
'PermissionQuery'} so clients are not given kick/ban/manage/channel rights;
update the permissions value and add a short comment explaining the reduced
default.
| -spec get_user(SessionId :: pos_integer()) -> {ok, #user{}} | {error, not_found}. | ||
| get_user(SessionId) -> | ||
| gen_server:call(?MODULE, {get_user, SessionId}). | ||
|
|
||
| -spec get_all_users() -> [#user{}]. | ||
| get_all_users() -> | ||
| gen_server:call(?MODULE, get_all_users). |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
#user{} record is not exported — specs referencing it are unusable by callers.
get_user/1 and get_all_users/0 specs return #user{}, but the record is private to this module. External callers have no way to type-match or destructure the result. Either export the record (or a map-based type), or return a map from these functions.
🤖 Prompt for AI Agents
In `@apps/erlmur/src/erlmur_user_manager.erl` around lines 48 - 54, The specs for
get_user/1 and get_all_users/0 reference the non-exported record `#user`{}, making
them unusable to callers; update the implementation and specs to either export
the record or (preferred) return maps: change the spec types to {ok, map()} |
{error, not_found} for get_user/1 and [map()] for get_all_users/0, and convert
any returned `#user`{} values to maps before returning (or replace internal usage
of `#user`{} with maps). Update any pattern matches or helper functions that
construct/return `#user`{} (e.g., the gen_server handlers that return {get_user,
SessionId} and get_all_users) to build maps so the public API and specs remain
consistent.
| listening_volume_adjust = 0, | ||
| codec_version, | ||
| texture_hash, | ||
| plugin_context, | ||
| plugin_identity, | ||
| temporary_access_tokens, | ||
| listening_volume_adjustment, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for usage of both fields to determine if they serve different purposes
echo "=== listening_volume_adjust ==="
rg -n 'listening_volume_adjust[^i]' --type=erlang
echo ""
echo "=== listening_volume_adjustment ==="
rg -n 'listening_volume_adjustment' --type=erlangRepository: freke/erlmur
Length of output: 999
Remove the unused field listening_volume_adjust (line 73).
The field listening_volume_adjust is defined in the record but never referenced anywhere in the codebase. The active field is listening_volume_adjustment (line 79), which is used in mumble_msg.erl at multiple locations. The unused field should be removed to avoid confusion and dead code.
🤖 Prompt for AI Agents
In `@apps/mumble_protocol/include/mumble_protocol.hrl` around lines 73 - 79,
Remove the unused record field listening_volume_adjust from the record
definition in mumble_protocol.hrl (it is redundant with the active field
listening_volume_adjustment); update the record tuple so only
listening_volume_adjustment remains, run a project-wide search for
listening_volume_adjust to ensure there are no remaining references, and update
any comments or docs that mention the removed field.
| NewStateData = case StateData#state.udp_addr of | ||
| undefined -> | ||
| erlmur_user_manager:register_udp_addr(SessionId, Addr), | ||
| StateData#state{crypto_state = NewCryptoState, udp_addr = Addr}; | ||
| _ -> | ||
| StateData#state{crypto_state = NewCryptoState} | ||
| end, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if erlmur_user_manager exists in the mumble_protocol app or is in a separate app
fd "erlmur_user_manager" --type f
# Also check all erlmur_ references in mumble_protocol
rg -n 'erlmur_' apps/mumble_protocol/src/ --type erlangRepository: freke/erlmur
Length of output: 442
Remove hard dependency on erlmur_user_manager — breaks standalone library goal.
erlmur_user_manager is located in apps/erlmur/src/ (separate app), yet mumble_protocol calls it directly at three locations: mumble_server_conn.erl:246, mumble_udp_server.erl:144, and mumble_udp_server.erl:151. This cross-app coupling prevents mumble_protocol from functioning as a standalone library. Route user manager operations through the handler_mod callback instead.
🤖 Prompt for AI Agents
In `@apps/mumble_protocol/src/mumble_server_conn.erl` around lines 244 - 250,
Replace direct calls to erlmur_user_manager:register_udp_addr/2 with an
invocation on the configurable handler module so mumble_protocol has no hard
dependency on erlmur_user_manager; specifically, in mumble_server_conn.erl
(NewStateData block) and the two places in mumble_udp_server.erl, call the
handler module stored on the connection state (e.g. StateData#state.handler_mod
or StateData.handler_mod) like HandlerMod:register_udp_addr(SessionId, Addr) (or
invoke via apply/3 if needed) and remove the direct erlmur_user_manager
reference so user-manager behavior is routed through the handler_mod callback.
| send_msg(#state{socket = Socket, transport = Transport}, Map) -> | ||
| case mumble_tcp_proto:pack(Map) of | ||
| Bin when is_binary(Bin) -> | ||
| logger:notice("Sending ~p (~p bytes)", [maps:get(message_type, Map), byte_size(Bin)]), | ||
| case Transport:send(Socket, Bin) of | ||
| ok -> ok; | ||
| {error, Reason} -> | ||
| logger:error("Failed to send ~p: ~p", [maps:get(message_type, Map), Reason]), | ||
| {error, Reason} | ||
| end | ||
| end. |
There was a problem hiding this comment.
send_msg/2 error return is silently discarded by all callers.
send_msg can return {error, Reason} (line 455), but every call site (lines 178, 200, 353, 364, 373, 379, 387, 394, 400) ignores the return value. A send failure during authentication handshake (e.g., CryptSetup, ServerSync) would leave the connection in an inconsistent state — the server thinks it sent the messages but the client never received them.
🤖 Prompt for AI Agents
In `@apps/mumble_protocol/src/mumble_server_conn.erl` around lines 447 - 457,
send_msg/2 can return {error, Reason} but all its callers ignore that result, so
send failures (especially during handshake messages like CryptSetup or
ServerSync) leave the connection inconsistent; update send_msg callers to check
its return and propagate or handle errors: where functions such as the
handshake/auth routines invoke send_msg, change their code to pattern-match on
ok vs {error, Reason} and on error either log and close the socket/terminate the
connection or return {error, Reason} up the call chain so the connection state
is not advanced; ensure send_msg remains returning ok | {error, Reason} and that
the calling functions propagate the error (or perform cleanup) rather than
discarding it.
| Port = proplists:get_value(port, Options), | ||
| CertFile = proplists:get_value(cert_file, Options), | ||
| KeyFile = proplists:get_value(key_file, Options), | ||
| ServerHandler = proplists:get_value(server_handler, Options, mock_mumble_handler), |
There was a problem hiding this comment.
Production default server_handler is the test mock.
mock_mumble_handler is a test-only module. If a caller omits the server_handler option, the supervisor silently falls back to a mock, which will misbehave in production. Consider removing the default or using a sentinel that fails fast.
- ServerHandler = proplists:get_value(server_handler, Options, mock_mumble_handler),
+ case proplists:get_value(server_handler, Options) of
+ undefined -> error(missing_server_handler);
+ ServerHandler -> ServerHandler
+ end,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ServerHandler = proplists:get_value(server_handler, Options, mock_mumble_handler), | |
| case proplists:get_value(server_handler, Options) of | |
| undefined -> error(missing_server_handler); | |
| ServerHandler -> ServerHandler | |
| end, |
🤖 Prompt for AI Agents
In `@apps/mumble_protocol/src/mumble_server_sup.erl` at line 61, The supervisor
currently defaults ServerHandler to the test-only mock via
proplists:get_value(server_handler, Options, mock_mumble_handler); change this
so production can't silently use the mock—either remove the default (use
proplists:get_value(server_handler, Options) and explicitly handle the undefined
case) or replace the default with a sentinel (e.g. undefined_server_handler) and
make the supervisor's init/start function validate ServerHandler and crash with
a clear error if it's not provided or is the sentinel; update the code paths
that reference ServerHandler so they rely on the validated value.
| handle_info({udp, _Socket, IP, Port, Data} = Msg, State) -> | ||
| logger:info("UDP Message ~p", [Msg]), | ||
| Addr = {IP, Port}, |
There was a problem hiding this comment.
Logging every UDP packet at info level will be very noisy.
Line 142 logs every incoming UDP message (including voice packets) at info level. Voice traffic is high-frequency; this will flood logs in production. Additionally, line 142 uses tab indentation inconsistent with the rest of the file.
Proposed fix
handle_info({udp, _Socket, IP, Port, Data} = Msg, State) ->
- logger:info("UDP Message ~p", [Msg]),
+ logger:debug("[mumble_udp_server] UDP packet from ~p:~p (~B bytes)", [IP, Port, byte_size(Data)]),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| handle_info({udp, _Socket, IP, Port, Data} = Msg, State) -> | |
| logger:info("UDP Message ~p", [Msg]), | |
| Addr = {IP, Port}, | |
| handle_info({udp, _Socket, IP, Port, Data} = Msg, State) -> | |
| logger:debug("[mumble_udp_server] UDP packet from ~p:~p (~B bytes)", [IP, Port, byte_size(Data)]), | |
| Addr = {IP, Port}, |
🤖 Prompt for AI Agents
In `@apps/mumble_protocol/src/mumble_udp_server.erl` around lines 141 - 143, The
handler handle_info/2 currently logs every incoming UDP packet with
logger:info("UDP Message ~p", [Msg]) which will flood production logs
(especially for voice packets) and also uses a tab for indentation; change the
log level to debug or remove the per-packet log (e.g., use logger:debug/2) and
only log at info on notable events or errors, keep message variable Msg for
context if needed, and normalize the line indentation to match the file’s spaces
style so handle_info/2 and the Addr = {IP, Port} line use consistent spacing.
| - `11110000` - 5 bytes (32 bits value) | ||
| - `11110100` - 9 bytes (64 bits value) | ||
| - `111111xx` - 1 byte (2 bits value, negative numbers -4 to -1) | ||
| - `11111100` - 9 bytes (64 bits, negative numbers) |
There was a problem hiding this comment.
Moduledoc prefix 11111100 on line 20 overlaps with 111111xx on line 19.
11111100 (0xFC) falls within the 111111xx (0xFC–0xFF) range documented on line 19 for small negatives. Per the Mumble protocol, large negative numbers use prefix 111110xx (0xF8–0xFB), followed by a recursive varint of the bitwise-NOT value — not a fixed 64-bit payload. The doc and code should both use 0xF8 (11111000) for the large-negative prefix.
🤖 Prompt for AI Agents
In `@apps/mumble_protocol/src/mumble_varint.erl` around lines 17 - 20, The
moduledoc and implementation incorrectly document and use the large-negative
varint prefix 0xFC (11111100) which overlaps the small-negative range 111111xx;
update the documentation and any constants/logic in mumble_varint.erl to use
0xF8 (11111000) as the large-negative prefix and make sure the encode/decode
paths (the varint handling functions in this module) implement the protocol:
when encountering 111110xx, follow with the recursive varint of the bitwise-NOT
value (not a fixed 64-bit payload), and adjust any pattern-matching, prefix
constants, and tests to reflect 0xF8/111110xx for large negatives and 111111xx
(0xFC–0xFF) for the small-negative two-bit values.
| -module(mumble_cert_tests). | ||
| -include_lib("eunit/include/eunit.hrl"). | ||
| -include_lib("kernel/include/file.hrl"). | ||
| -include("mumble_protocol.hrl"). | ||
|
|
||
| %%%%%%%%%%%%%%%%%%%%%%% | ||
| %% Helper Functions | ||
| %%%%%%%%%%%%%%%%%%%%%%% | ||
|
|
||
| get_test_dir() -> | ||
| "/tmp/mumble_test_certs_" ++ integer_to_list(erlang:phash2(make_ref())). | ||
|
|
||
| ensure_test_dir() -> | ||
| TestDir = get_test_dir(), | ||
| case file:make_dir(TestDir) of | ||
| ok -> ok; | ||
| {error, eexist} -> ok | ||
| end, | ||
| TestDir. | ||
|
|
||
| cleanup_test_dir(TestDir) -> | ||
| case filelib:is_dir(TestDir) of | ||
| true -> | ||
| Files = filelib:wildcard(filename:join(TestDir, "*")), | ||
| [file:delete(F) || F <- Files], | ||
| file:del_dir(TestDir); | ||
| false -> ok | ||
| end. | ||
|
|
||
| generate_cert(CertFile, KeyFile) -> | ||
| Cmd = io_lib:format( | ||
| "openssl req -x509 -newkey rsa:2048 -keyout ~s -out ~s -days 1 -nodes -subj '/CN=localhost' 2>&1", | ||
| [KeyFile, CertFile] | ||
| ), | ||
| os:cmd(Cmd). | ||
|
|
||
| generate_cert(CertFile, KeyFile, Subject, Days) -> | ||
| Cmd = io_lib:format( | ||
| "openssl req -x509 -newkey rsa:2048 -keyout ~s -out ~s -days ~B -nodes -subj '~s' 2>&1", | ||
| [KeyFile, CertFile, Days, Subject] | ||
| ), | ||
| os:cmd(Cmd). | ||
|
|
||
| %%%%%%%%%%%%%%%%%%%%%%% | ||
| %% Setup Check | ||
| %%%%%%%%%%%%%%%%%%%%%%% | ||
|
|
||
| openssl_available_test() -> | ||
| Result = os:cmd("which openssl"), | ||
| ?assert(length(Result) > 0). | ||
|
|
||
| %%%%%%%%%%%%%%%%%%%%%%% | ||
| %% Certificate Generation Tests | ||
| %%%%%%%%%%%%%%%%%%%%%%% | ||
|
|
||
| generate_cert_creates_files_test() -> | ||
| TestDir = ensure_test_dir(), | ||
| try | ||
| CertFile = filename:join(TestDir, "test.pem"), | ||
| KeyFile = filename:join(TestDir, "test.key"), | ||
|
|
||
| generate_cert(CertFile, KeyFile), | ||
|
|
||
| ?assert(filelib:is_file(CertFile)), | ||
| ?assert(filelib:is_file(KeyFile)) | ||
| after | ||
| cleanup_test_dir(TestDir) | ||
| end. | ||
|
|
||
| generate_cert_creates_valid_x509_test() -> | ||
| TestDir = ensure_test_dir(), | ||
| try | ||
| CertFile = filename:join(TestDir, "test.pem"), | ||
| KeyFile = filename:join(TestDir, "test.key"), | ||
|
|
||
| generate_cert(CertFile, KeyFile), | ||
|
|
||
| VerifyCmd = io_lib:format("openssl x509 -in ~s -text -noout 2>&1", [CertFile]), | ||
| Output = os:cmd(VerifyCmd), | ||
| ?assert(string:find(Output, "Certificate") =/= nomatch) | ||
| after | ||
| cleanup_test_dir(TestDir) | ||
| end. | ||
|
|
||
| generate_cert_with_default_subject_test() -> | ||
| TestDir = ensure_test_dir(), | ||
| try | ||
| CertFile = filename:join(TestDir, "test.pem"), | ||
| KeyFile = filename:join(TestDir, "test.key"), | ||
|
|
||
| generate_cert(CertFile, KeyFile), | ||
|
|
||
| SubjectCmd = io_lib:format("openssl x509 -in ~s -subject -noout 2>&1", [CertFile]), | ||
| Subject = os:cmd(SubjectCmd), | ||
| %% OpenSSL format can be "subject=CN = localhost" or "CN=localhost" | ||
| ?assert( | ||
| (string:find(Subject, "CN = localhost") =/= nomatch) orelse | ||
| (string:find(Subject, "CN=localhost") =/= nomatch) | ||
| ) | ||
| after | ||
| cleanup_test_dir(TestDir) | ||
| end. | ||
|
|
||
| generate_cert_with_custom_subject_test() -> | ||
| TestDir = ensure_test_dir(), | ||
| try | ||
| CertFile = filename:join(TestDir, "test.pem"), | ||
| KeyFile = filename:join(TestDir, "test.key"), | ||
|
|
||
| generate_cert(CertFile, KeyFile, "/CN=test.example.com/O=TestOrg", 1), | ||
|
|
||
| SubjectCmd = io_lib:format("openssl x509 -in ~s -subject -noout 2>&1", [CertFile]), | ||
| Subject = os:cmd(SubjectCmd), | ||
| ?assert( | ||
| (string:find(Subject, "test.example.com") =/= nomatch) orelse | ||
| (string:find(Subject, "test") =/= nomatch) | ||
| ) | ||
| after | ||
| cleanup_test_dir(TestDir) | ||
| end. | ||
|
|
||
| generate_cert_with_custom_days_test() -> | ||
| TestDir = ensure_test_dir(), | ||
| try | ||
| CertFile = filename:join(TestDir, "test.pem"), | ||
| KeyFile = filename:join(TestDir, "test.key"), | ||
|
|
||
| generate_cert(CertFile, KeyFile, "/CN=localhost", 30), | ||
|
|
||
| DatesCmd = io_lib:format("openssl x509 -in ~s -dates -noout 2>&1", [CertFile]), | ||
| Dates = os:cmd(DatesCmd), | ||
| ?assert(string:find(Dates, "notBefore") =/= nomatch), | ||
| ?assert(string:find(Dates, "notAfter") =/= nomatch) | ||
| after | ||
| cleanup_test_dir(TestDir) | ||
| end. | ||
|
|
||
| reuse_existing_cert_test() -> | ||
| TestDir = ensure_test_dir(), | ||
| try | ||
| CertFile = filename:join(TestDir, "test.pem"), | ||
| KeyFile = filename:join(TestDir, "test.key"), | ||
|
|
||
| %% First generation | ||
| generate_cert(CertFile, KeyFile), | ||
| {ok, FileInfo1} = file:read_file_info(CertFile), | ||
| MTime1 = FileInfo1#file_info.mtime, | ||
|
|
||
| timer:sleep(100), | ||
|
|
||
| %% Check files still exist | ||
| {ok, FileInfo2} = file:read_file_info(CertFile), | ||
| MTime2 = FileInfo2#file_info.mtime, | ||
|
|
||
| ?assertEqual(MTime1, MTime2) | ||
| after | ||
| cleanup_test_dir(TestDir) | ||
| end. | ||
|
|
||
| regenerate_missing_cert_test() -> | ||
| TestDir = ensure_test_dir(), | ||
| try | ||
| CertFile = filename:join(TestDir, "test.pem"), | ||
| KeyFile = filename:join(TestDir, "test.key"), | ||
|
|
||
| %% Generate both | ||
| generate_cert(CertFile, KeyFile), | ||
| ?assert(filelib:is_file(CertFile)), | ||
| ?assert(filelib:is_file(KeyFile)), | ||
|
|
||
| %% Delete cert only | ||
| ok = file:delete(CertFile), | ||
| ?assertNot(filelib:is_file(CertFile)), | ||
|
|
||
| %% Regenerate | ||
| generate_cert(CertFile, KeyFile), | ||
|
|
||
| ?assert(filelib:is_file(CertFile)), | ||
| ?assert(filelib:is_file(KeyFile)) | ||
| after | ||
| cleanup_test_dir(TestDir) | ||
| end. | ||
|
|
||
| cert_cleanup_test() -> | ||
| TestDir = ensure_test_dir(), | ||
| try | ||
| CertFile = filename:join(TestDir, "test.pem"), | ||
| KeyFile = filename:join(TestDir, "test.key"), | ||
|
|
||
| generate_cert(CertFile, KeyFile), | ||
| ?assert(filelib:is_file(CertFile)), | ||
| ?assert(filelib:is_file(KeyFile)), | ||
|
|
||
| %% Cleanup | ||
| Files = filelib:wildcard(filename:join(TestDir, "*")), | ||
| [file:delete(F) || F <- Files], | ||
| file:del_dir(TestDir), | ||
|
|
||
| ?assertNot(filelib:is_dir(TestDir)) | ||
| after | ||
| cleanup_test_dir(TestDir) | ||
| end. |
There was a problem hiding this comment.
Tests don't exercise the mumble_cert module — only local OpenSSL helpers.
Despite the module name mumble_cert_tests, none of these tests call mumble_cert:validate_cert_files/2, mumble_cert:ensure_auto_certs/2, or mumble_cert:generate_self_signed_cert/4. They only test the locally defined generate_cert/2,4 helpers which shell out to OpenSSL directly. The production code in mumble_cert.erl (including its directory-creation logic, error handling, and file validation) remains untested.
Consider adding tests that exercise the actual public API of mumble_cert.
Would you like me to generate a set of tests that exercise mumble_cert:validate_cert_files/2, mumble_cert:ensure_auto_certs/2, and mumble_cert:generate_self_signed_cert/4?
🤖 Prompt for AI Agents
In `@apps/mumble_protocol/test/mumble_cert_tests.erl` around lines 1 - 202, Add
real unit tests that call the public API in mumble_cert instead of only the
local OpenSSL helpers: write tests that (1) call
mumble_cert:generate_self_signed_cert(TestDirCertPath, TestDirKeyPath, Subject,
Days) and assert the cert/key files are created and OpenSSL reports valid X.509
dates/subject, (2) call mumble_cert:validate_cert_files(CertPath, KeyPath) to
assert it returns ok for valid files and an error tuple when files are missing
or malformed, and (3) call mumble_cert:ensure_auto_certs(Dir, Options) to assert
it creates cert/key when absent, reuses existing files (mtime unchanged) and
regenerates missing ones; use the existing
get_test_dir/ensure_test_dir/cleanup_test_dir helpers to manage temp dirs and
filelib:is_file/file:read_file_info to check existence and mtimes, referencing
the functions mumble_cert:generate_self_signed_cert/4,
mumble_cert:validate_cert_files/2, and mumble_cert:ensure_auto_certs/2 in your
new tests.
freke
force-pushed
the
umbrella
branch
3 times, most recently
from
691a2ba to
bc7392b
Compare
There was a problem hiding this comment.
Actionable comments posted: 13
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/ocb128_crypto/test/ocb128_crypto_benchmark_SUITE.erl (1)
3-11:⚠️ Potential issue | 🟠 MajorBug: Individual benchmark test functions are not exported.
all/0returns atoms likeencrypt_16b,decrypt_16b, etc., but the corresponding functions (lines 78–96) are missing from the-exportlist. WhenRUN_BENCHMARKS=true, Common Test will fail withundeferrors because it cannot call unexported functions.🐛 Proposed fix — add exports for all benchmark functions
-export([ all/0, groups/0, init_per_suite/1, end_per_suite/1, init_per_testcase/2, end_per_testcase/2, - run_benchmark/2 + run_benchmark/2, + encrypt_16b/1, encrypt_32b/1, encrypt_128b/1, + encrypt_1kb/1, encrypt_4kb/1, encrypt_16kb/1, + encrypt_128kb/1, encrypt_1mb/1, encrypt_4mb/1, + decrypt_16b/1, decrypt_32b/1, decrypt_128b/1, + decrypt_1kb/1, decrypt_4kb/1, decrypt_16kb/1, + decrypt_128kb/1, decrypt_1mb/1, decrypt_4mb/1 ]).Also applies to: 78-96
rebar.config (1)
30-38:⚠️ Potential issue | 🔴 CriticalTest profile
erl_optsmust include the base include paths to avoid compilation failures.The test profile on line 32 only contains
[export_all, nowarn_export_all]without the include paths from the base config. Multiple test files include"Mumble_gpb.hrl"and"mumble_protocol.hrl", which require the include paths"apps/mumble_protocol/include"and"_build/default/plugins/gpb/include"to resolve. Since rebar3 replaces (rather than merges) profileerl_opts, test compilation will fail.Update the test profile
erl_optsto include:{erl_opts, [export_all, nowarn_export_all, {i, "apps/mumble_protocol/include"}, {i, "_build/default/plugins/gpb/include"}]}
🤖 Fix all issues with AI agents
In `@apps/mumble_protocol/README.md`:
- Around line 92-124: Update the README examples to use maps (the actual client
API) instead of Erlang record syntax: replace occurrences of #'TextMessage'{...}
and #'ServerSync'{...} in the client examples with map literals and map
pattern-matching, and update the send/send_voice calls and the parent loop()
receive clauses to expect {mumble_msg, Map} and match map keys (e.g.,
<<"message">> or atom keys used by the API) rather than record fields;
specifically adjust the examples around mumble_client_conn:start_link,
mumble_client_conn:send, mumble_client_conn:send_voice and the loop() receive
patterns so they show sending/receiving maps consistent with the
e2e_connection_SUITE tests.
- Around line 68-83: The README examples use Erlang record syntax
(#'TextMessage'{...}) but the runtime passes and expects maps
(mumble_tcp_proto:decode returns a map and mumble_server_conn:send forwards maps
to mumble_tcp_proto:pack), so update the handler example in
my_handler_mod:handle_msg to construct and match on maps (e.g., %{"message" =>
<<"ping">>} and %{"message" => <<"pong">>}) instead of records, and ensure the
call to mumble_server_conn:send/2 sends a map; change any pattern matches and
sent payloads accordingly so examples match the map-based API.
In `@apps/mumble_protocol/src/mumble_client_conn.erl`:
- Around line 217-224: The send_udp_ping/1 and send_udp/2 functions are
currently no-ops while established(enter, ...) starts a recurring 5s timer,
causing needless timers and an unused udp_timer state field; either implement
real UDP sending or remove the timer and related stubs. To fix, choose one: (A)
implement UDP support by opening a gen_udp socket (or using existing socket
helper), replace logger:debug in send_udp_ping/1 and send_udp/2 with actual
gen_udp:send calls, ensure the socket is stored in the state and udp_timer is
set to the timer reference created in established(enter, ...), and cancel the
timer on teardown; or (B) remove the recurring timer setup in established(enter,
...) and delete or noop the send_udp_ping/1 and send_udp/2 stubs (and remove
udp_timer from the state) so no timer churn occurs until UDP is implemented.
Ensure changes reference the functions send_udp_ping/1, send_udp/2, the
established(enter, ...) clause, and the udp_timer field so reviewers can find
and validate the fix.
- Around line 103-108: The call to ssl:connect/3 in the gen_statem state
transition can block indefinitely; update the call in the connect branch to use
ssl:connect/4 with an explicit timeout value (e.g. a configured connect_timeout
from the state or a reasonable default like 5000 ms) so the process cannot hang
forever. Locate the ssl:connect(Host, Port, SslOptsList) invocation and replace
it with ssl:connect(Host, Port, SslOptsList, Timeout) (pass the Timeout from
State such as State#state.connect_timeout or add one to the state), keeping the
same {ok, Socket} and {error, Reason} handling and return values ({next_state,
authenticating, State#state{socket = Socket}} and {stop, Reason}). Ensure any
new field introduced (connect_timeout) is initialized where State is built.
In `@apps/mumble_protocol/src/mumble_msg.erl`:
- Around line 519-532: The from_map clause for 'Ping' currently uses
erlang:system_time(second) as a default for timestamp which introduces a side
effect and uses seconds (likely wrong unit); change the default to undefined
(consistent with other optional fields) so from_map('Ping') is pure and callers
must set timestamp explicitly, updating the pattern in the from_map/1 function
and the #'Ping'{timestamp = ...} construction accordingly.
In `@apps/mumble_protocol/src/mumble_server_conn.erl`:
- Around line 492-496: notify_status/4 currently discards any return from
Mod:handle_msg/2 that isn't {ok, NewState}, which swallows {stop, Reason,
NewState} signals; update notify_status to explicitly pattern-match and
propagate a {stop, Reason, NewState} return (return it directly) and keep the
existing {ok, NewState} -> {ok, NewState} branch, falling back to {ok, State}
only for truly unexpected results—locate the notify_status function and the call
to Mod:handle_msg to add the explicit {stop, Reason, NewState} clause so stop
signals from the handler are not lost.
In `@apps/mumble_protocol/src/mumble_server_sup.erl`:
- Around line 57-62: The init/1 function uses proplists:get_value for Port,
CertFile, and KeyFile without validation, letting undefined propagate; add a
validation helper (e.g., require_opt/2 or require_opt/1 as suggested) and call
it for port, cert_file, and key_file inside init/1 so missing options return a
clear error ({error, {missing_required_option, Key}}) or crash fast with a
descriptive reason instead of passing undefined into
mumble_udp_server:start_link or ranch; update any callers or specs as needed to
handle the explicit error.
- Around line 71-76: Replace the manual RanchListener map and the indirect
start_tcp_listener approach in mumble_server_sup:init/1 with a proper ranch
child spec by calling ranch:child_spec/5 so the listener is supervised under
mumble_server_sup; specifically, remove or stop using start_tcp_listener and
instead add ranch:child_spec(mumble_tcp_listener, TransportModule,
TransportOpts, ServerHandler, ListenerOptions) (using the correct
TransportModule and TransportOpts that include Port/CertFile/KeyFile) into the
children list so crashes propagate to the supervisor and restarts won’t
conflict.
In `@apps/mumble_protocol/src/mumble_server.erl`:
- Around line 34-46: The current start_server/1 function defaults server_handler
to test-only mock_mumble_handler which will be undefined in releases; update
start_server to require the server_handler option (i.e. remove the mock default)
or change the default to a production handler (e.g. prod_mumble_handler) instead
of mock_mumble_handler so it won’t crash at runtime — locate the
maps:get(server_handler, Options, mock_mumble_handler) in start_server/1 and
either remove the third argument and return a clear {error,
missing_server_handler} when absent, or replace the default symbol with your
production handler module name.
In `@apps/mumble_protocol/src/mumble_udp_server.erl`:
- Around line 141-160: The current handler in handle_info/2 logs every UDP
packet at info level and iterates AllUsers using element(3, User), which is
fragile and the suggested element(`#user.pid`, User) would reference a different
`#user` record; fix by changing logger:info("UDP Message ~p", [Msg]) to
logger:debug("UDP Message ~p", [Msg]) and replace the anonymous function in
lists:foreach to use pattern matching over the erlmur user tuple returned by
erlmur_user_manager:get_all_users(), e.g. fun({user, _SessionId, Pid, _Username,
_UdpAddr}) -> mumble_server_conn:udp_packet(Pid, Data, Addr) end so you
explicitly extract Pid (alternatively add
-include_lib("erlmur/include/erlmur.hrl") if that header exists or keep
element(3, User) but add a clear comment documenting the assumed tuple layout).
In `@apps/mumble_protocol/test/client_logic_tests.erl`:
- Around line 312-317: The three tests established_transition_test,
client_close_test, and invalid_msg_test are only ?assert(true) placeholders and
provide no coverage; replace each placeholder with a real test or remove and
mark TODO: for established_transition_test implement sending a ServerSync-like
mock message to the client logic and assert the state becomes 'established' (use
the same test harness helpers used elsewhere in the file), for client_close_test
simulate a server/client close sequence and assert proper cleanup/closed state
and expected messages, and for invalid_msg_test send an invalid/garbage message
and assert the client drops it or logs an error (matching existing error/assert
helpers); if you cannot implement now, delete the placeholder tests and add a
TODO comment referencing these test names so they are not silently passing.
In `@apps/mumble_protocol/test/mock_transport.erl`:
- Around line 176-185: The handle_call clause for recv must not perform a raw
receive (it will never match the {'$gen_cast',...} wrapper and blocks the
gen_server); instead remove the blocking receive in handle_call({recv, ...},
...) and either (a) immediately return {reply, {error, timeout}, State} when
message_queue is empty, or (b) implement asynchronous waiting: store the caller
From (and timeout metadata) in State (e.g. pending_recv key) and return
{noreply, StateUpdated}, then in handle_cast({inject_message, Data}, State) (the
handler for send_message/2 casts) check for a pending_recv and call
gen_server:reply(From, {ok, Data}) (or enqueue if none) before updating State —
ensure handle_cast matches the inject_message tuple and updates message_queue
accordingly.
In `@apps/mumble_protocol/test/mumble_SUITE.erl`:
- Around line 287-302: The tests named client_connect_disconnect_test,
client_with_callback_test, client_server_communication_test, and
multiple_clients_test currently only start/stop the server and do not exercise
the client behaviors their names promise; update each test to either implement
the intended behavior (e.g., use mumble:connect/3 or equivalent to create a
client, perform connect/disconnect flows, register and assert callback
invocations in client_with_callback_test, send/receive messages in
client_server_communication_test, and spawn/connect multiple clients and assert
concurrency in multiple_clients_test) or explicitly mark the test as skipped by
returning {skip, "Not yet implemented"} (matching the pattern used by
server_multiple_instances_test); locate and edit the test functions
(client_connect_disconnect_test, client_with_callback_test,
client_server_communication_test, multiple_clients_test) to apply one of these
fixes so the suite no longer gives false confidence.
🟡 Minor comments (21)
justfile-48-49 (1)
48-49:⚠️ Potential issue | 🟡 MinorWildcard
*.pem/*.keydeletion is broader than before — verify intent.The previous version deleted specific files (
cert.pem,key.pem). The new wildcards will remove all.pemand.keyfiles found recursively under the project root, which could catch files you want to keep (e.g., manually provisioned certs outside_build). If the intent is only to clean generated test certificates, consider restricting the search path or using more specific patterns.apps/erlmur/src/erlmur_sup.erl-23-32 (1)
23-32:⚠️ Potential issue | 🟡 MinorMisleading log messages — children haven't started yet during
init/1.The supervisor starts its children after
init/1returns. Line 31's "User manager started" log fires before the child is actually started, which can be confusing when debugging startup issues.Proposed fix
init([]) -> - logger:info("[erlmur_sup] Starting user manager..."), UserManager = #{ id => erlmur_user_manager, start => {erlmur_user_manager, start_link, []}, restart => permanent, type => worker }, - logger:info("[erlmur_sup] User manager started"), + logger:info("[erlmur_sup] Initializing supervisor with children: [erlmur_user_manager]"), {ok, {{one_for_one, 5, 10}, [UserManager]}}.apps/mumble_protocol/src/mumble_version.erl-37-46 (1)
37-46:⚠️ Potential issue | 🟡 Minor
decode/1has an ambiguity: V2-encoded values that fit in 32 bits are decoded as V1.The range-based dispatch (
V1 =< 16#FFFFFFFFvsV2 > 16#FFFFFFFF) can misclassify a V2-encoded integer if the version components are small enough to fit in 32 bits. For example, version0.0.1encodes to V2 =16#0000000000010000(65536), which falls in the V1 range and would be decoded as{1, 0, 0}instead of{0, 0, 1}.In practice, real Mumble versions (≥1.x) always produce V2 > 32-bit max, so this is safe today. Consider adding a two-arity variant
decode(Format, Value)if this ever needs to be robust for edge cases, or at minimum document the assumption.apps/mumble_protocol/src/mumble_udp_proto.erl-9-21 (1)
9-21:⚠️ Potential issue | 🟡 MinorDocumentation inconsistency: Type field description contradicts actual code.
Line 12 states "Type (3 bits): Message type (0=Ping, 1=Voice, etc.)" but Line 19 says "Type 1: Ping" and the code at Line 34 matches
<<1:3, ...>>as ping. The Mumble protocol uses Type 1 for ping in the legacy UDP format. The doc header at Line 12 should be corrected.📝 Proposed fix
-- **Type (3 bits)**: Message type (0=Ping, 1=Voice, etc.) +- **Type (3 bits)**: Message type (1=Ping, 0/2-7=Voice with various codecs)apps/mumble_protocol/src/mumble_client_conn.erl-130-134 (1)
130-134:⚠️ Potential issue | 🟡 MinorRename counter fields or clarify stats semantics for client-side data tracking.
In
mumble_client_conn.erl(lines 132-133 and 157-158),from_client_tcp_packetsandfrom_client_tcp_bytesare incremented when receiving data from the server. The stats record'sfrom_client_*fields are semantically intended for data originating from clients (as confirmed bymumble_server_conn.erlwhich correctly uses them for incoming client data). Either rename these fields tofrom_server_tcp_*for the client context, or implement separate tracking fields with clear semantics to distinguish client-received vs. server-received data.apps/mumble_protocol/docs/establishing_connection.md-94-97 (1)
94-97:⚠️ Potential issue | 🟡 MinorPing timeout value disagrees with the implementation.
The doc says "If the server doesn't receive a Ping for 30 seconds, it will drop the connection", but
mumble_server_conn.erldefines?TIMEOUTas 60000 ms (60 seconds).apps/mumble_protocol/test/mumble_client_api_tests.erl-7-11 (1)
7-11:⚠️ Potential issue | 🟡 MinorOnly 2 of 7 test functions are included in the test group — 5 are orphaned.
client_start_no_certs,client_send_message,client_send_voice,client_get_state, andclient_stopare defined but never referenced bymumble_client_api_test_/0. They won't run duringrebar3 eunit. Either add them to the group (with proper test infrastructure) or remove them to avoid confusion.apps/mumble_protocol/docs/establishing_connection.md-83-91 (1)
83-91:⚠️ Potential issue | 🟡 MinorMinor discrepancy: doc says ServerSync includes
permissions, code doesn't send it.Line 89 states ServerSync fields include "permissions for the root channel", but the implementation at
mumble_server_conn.erl:394-399only sendssession,max_bandwidth, andwelcome_text.apps/mumble_protocol/docs/establishing_connection.md-58-63 (1)
58-63:⚠️ Potential issue | 🟡 MinorDocumentation mentions
ServerConfigandSuggestConfigbut they're not sent by the implementation.The doc states the server sends
ServerConfigandSuggestConfigduring state synchronization (also shown in the Mermaid diagram at lines 114-115). However,mumble_server_conn.erllines 355-408 only sendsCryptSetup,CodecVersion,ChannelState,UserState, andServerSync— neitherServerConfignorSuggestConfigis sent. Either update the code to send them or revise the docs to match the current implementation.apps/mumble_protocol/test/mumble_client_api_tests.erl-13-17 (1)
13-17:⚠️ Potential issue | 🟡 Minor
setup/0andcleanup/1are defined but never wired into the test group.The test generator at line 7 is a plain list, not a
{setup, ...}fixture. These functions are dead code.apps/mumble_protocol/src/mumble_server_conn.erl-459-464 (1)
459-464:⚠️ Potential issue | 🟡 Minor
send_voiceTCP fallback discardssend_msgresult.When
udp_verifiedisfalse,send_msgis called at line 464, but its return value (which may be{error, Reason}) is discarded. Sincesend_voiceis called fromestablished(cast, {voice_data, ...})which expects{keep_state_and_data, ...}, a send failure will go unnoticed.apps/mumble_protocol/README.md-44-48 (1)
44-48:⚠️ Potential issue | 🟡 MinorTypo: "explicitely" → "explicitly" (line 45).
📝 Proposed fix
-- **Automatic Fallback**: If a server hasn't received a UDP packet from a client recently (or if the client explicitely +- **Automatic Fallback**: If a server hasn't received a UDP packet from a client recently (or if the client explicitlyapps/mumble_protocol/README.md-31-37 (1)
31-37:⚠️ Potential issue | 🟡 MinorFix spelling errors flagged by static analysis.
- Line 31: "recieved" → "received"
- Line 37: "reciveing" → "receiving"
📝 Proposed fix
-If no `Ping` is recieved for 30sec the client will be disconnected from the server. +If no `Ping` is received for 30 seconds the client will be disconnected from the server.-If reciveing a `UDPTunnel` message the connection is marked "UDP verified" false, until new UDP verification is done. +If receiving a `UDPTunnel` message the connection is marked "UDP verified" false, until new UDP verification is done.apps/mumble_protocol/test/mumble_cert_tests.erl-114-117 (1)
114-117:⚠️ Potential issue | 🟡 MinorWeak assertion in custom subject test —
string:find(Subject, "test")matches almost anything.The fallback check
string:find(Subject, "test") =/= nomatchis too permissive. It would pass even if the subject contained none of the expected fields, as long as any word containing "test" appears in the output.🐛 Use a more specific match
?assert( - (string:find(Subject, "test.example.com") =/= nomatch) orelse - (string:find(Subject, "test") =/= nomatch) + (string:find(Subject, "test.example.com") =/= nomatch) orelse + (string:find(Subject, "CN = test.example.com") =/= nomatch) )apps/mumble_protocol/test/mumble_cert_tests.erl-138-158 (1)
138-158:⚠️ Potential issue | 🟡 Minor
reuse_existing_cert_testdoesn't actually test reuse — it only proves the file isn't spontaneously modified.The test generates a cert, sleeps 100ms, then re-reads the same file's mtime. Since
generate_certis never called a second time, this asserts nothing about reuse behavior. To test reuse, callgenerate_cert(CertFile, KeyFile)again after the sleep and verify the mtime is unchanged.🐛 Proposed fix
%% First generation generate_cert(CertFile, KeyFile), {ok, FileInfo1} = file:read_file_info(CertFile), MTime1 = FileInfo1#file_info.mtime, timer:sleep(100), - %% Check files still exist + %% Second generation — should reuse existing + generate_cert(CertFile, KeyFile), {ok, FileInfo2} = file:read_file_info(CertFile), MTime2 = FileInfo2#file_info.mtime, - ?assertEqual(MTime1, MTime2) + %% NOTE: This will FAIL because generate_cert always overwrites. + %% If the intent is to test mumble_cert:ensure_auto_certs reuse, + %% call that function instead of the local generate_cert helper. + ?assertNotEqual(MTime1, MTime2)apps/mumble_protocol/src/mumble_udp_server.erl-38-40 (1)
38-40:⚠️ Potential issue | 🟡 Minor
send/3spec claims{error, term()}butgen_server:castalways returnsok.The
-specsaysok | {error, term()}, butgen_server:cast/2always returnsokimmediately. The error handling happens asynchronously insidehandle_cast(Line 67) and is only logged — never returned to the caller. The spec should beok.Proposed fix
--spec send(IP :: inet:ip_address(), inet:port_number(), binary()) -> ok | {error, term()}. +-spec send(IP :: inet:ip_address(), inet:port_number(), binary()) -> ok.apps/mumble_protocol/src/mumble_server.erl-86-95 (1)
86-95:⚠️ Potential issue | 🟡 Minor
erlang:system_info(otp_release)returns a charlist, not a binary.
os_versionin the#version{}record will be a charlist while other string fields are binaries. If downstream encoding (e.g., protobuf) expects binaries, this will fail.Proposed fix
- os_version = erlang:system_info(otp_release) + os_version = list_to_binary(erlang:system_info(otp_release))apps/erlmur/src/erlmur_server_handler.erl-78-86 (1)
78-86:⚠️ Potential issue | 🟡 MinorConvert
erlang:system_info(otp_release)to binary for protobuf compatibility.
erlang:system_info(otp_release)returns a charlist (e.g.,"27"), but the protobuf schema definesos_versionas a string field. The map contains binaries forreleaseandos, creating a type inconsistency. When protobuf encoding serializes this map, the charlist will cause encoding errors.Proposed fix
- os_version => erlang:system_info(otp_release) + os_version => list_to_binary(erlang:system_info(otp_release))Note: The same issue exists in
apps/mumble_protocol/src/mumble_server.erl:94.apps/mumble_protocol/test/mumble_SUITE.erl-169-174 (1)
169-174:⚠️ Potential issue | 🟡 Minor
end_per_testcasecomment promises cleanup but body is a no-op.The comment on line 173 says "Ensure no lingering processes" but no cleanup is performed. If a test fails after starting a server but before stopping it, the server process leaks and may cause subsequent tests to fail (e.g., port conflicts, registered name clashes with
mumble_udp_server).Consider stopping any leftover server references stored in
Config, or at minimum removing the misleading comment.apps/mumble_protocol/src/mumble.erl-247-250 (1)
247-250:⚠️ Potential issue | 🟡 MinorType spec doesn't permit
undefinedfor cert/key, but the doc example passes it.Line 36 of the moduledoc shows
mumble:start_client(undefined, undefined, "localhost", 64738), but the spec on line 247 declares both parameters asfile:filename_all(), which does not includeundefined. Dialyzer would flag callers passingundefined. Either update the spec tofile:filename_all() | undefinedor fix the doc example.Suggested spec fix
--spec start_client(file:filename_all(), file:filename_all(), string(), inet:port_number()) -> +-spec start_client(file:filename_all() | undefined, file:filename_all() | undefined, string(), inet:port_number()) -> {ok, client_ref()} | {error, term()}.And correspondingly for
start_client/5:--spec start_client(file:filename_all(), file:filename_all(), string(), inet:port_number(), client_options()) -> +-spec start_client(file:filename_all() | undefined, file:filename_all() | undefined, string(), inet:port_number(), client_options()) -> {ok, client_ref()} | {error, term()}.apps/mumble_protocol/test/mumble_SUITE.erl-237-241 (1)
237-241:⚠️ Potential issue | 🟡 MinorUse the public
get_port/0API instead of accessing internal state.The
mumble_udp_servermodule already exportsget_port/0which safely retrieves the port via a propergen_server:call. Accessing internal state viasys:get_state()andelement(3, State)is brittle and unnecessary. Replace withmumble_udp_server:get_port().
🧹 Nitpick comments (33)
justfile (1)
45-45: Broadfind -name "doc"may delete unintended directories.This will recursively remove any directory named
docunder the project root, including those inside dependencies or vendored code. Consider scoping the search to only the top-level generateddocdirectory.Proposed fix
- find {{justfile_directory()}} -name "doc" -exec rm -rf {} + + rm -rf {{justfile_directory()}}/docapps/ocb128_crypto/include/ocb128_crypto.hrl (1)
1-6: Consider adding an include guard.If this header is included from multiple paths (directly and transitively), the record redefinition will cause a compilation error. A standard Erlang include guard prevents this.
♻️ Proposed fix
+-ifndef(OCB128_CRYPTO_HRL). +-define(OCB128_CRYPTO_HRL, true). + -record(crypto_stats, { good = 0 :: non_neg_integer(), late = 0 :: non_neg_integer(), lost = 0 :: integer() }). -type crypto_stats() :: `#crypto_stats`{}. + +-endif.apps/ocb128_crypto/src/ocb128_crypto.erl (2)
14-17: Exportedencrypt_ocb/3anddecrypt_ocb/3lack-specand-docannotations.All other exported functions have specs and docs. These two are public API surface (used by tests and property tests) but have neither, which will cause Dialyzer to infer less precise types and leave gaps in generated documentation.
♻️ Proposed addition above the private section
+-doc """ +Low-level OCB encryption. Returns a map with `tag` and `ciphertext` keys. +""". +-spec encrypt_ocb(binary(), key(), nonce()) -> #{tag := binary(), ciphertext := binary()}. encrypt_ocb(Plain, Key, IV) -> ... +-doc """ +Low-level OCB decryption. Returns a map with `tag` and `text` keys. +""". +-spec decrypt_ocb(binary(), key(), nonce()) -> #{tag := binary(), text := binary()}. decrypt_ocb(Encrypted, Key, IV) -> ...
251-263: Debug logging exposes plaintext and packet contents.Lines 253, 257, 261, and 263 log the full packet, internal history state, and decrypted plaintext at debug level. If debug logging is accidentally enabled in production, this leaks sensitive data. Consider removing or reducing the verbosity of these statements, especially the plaintext log on Line 263.
apps/ocb128_crypto/test/ocb128_crypto_benchmark_SUITE.erl (1)
19-22: Inconsistent indentation — tabs mixed with spaces.Lines 20–22 use tab indentation while the rest of the file uses spaces. This is a minor cosmetic issue but may trip up linters or editors with different tab-width settings.
rebar.config (1)
40-45: Dependencies pinned to branch heads — no reproducible builds.
ranchandeqwalizer_supportare pulled frommaster/mainbranches, meaning builds are not reproducible across time. The same applies to test depsmeckandproperon lines 34–35. Consider pinning to a tag or commit SHA.apps/mumble_protocol/src/mumble_tcp_proto.erl (2)
137-144:find_msg_by_tag/1doesn't short-circuit on match.
lists:foldlalways traverses the fullMESSAGE_TABLEeven after finding the matching tag. Consider usinglists:keyfind/3orlists:search/2for early termination. The table is small, so this is a minor efficiency nit.Proposed fix
find_msg_by_tag(Tag) -> - lists:foldl(fun ({T, Record}, _Acc) when T =:= Tag -> - {ok, Tag, Record}; - (_, Acc) -> - Acc - end, - error, - ?MESSAGE_TABLE). + case lists:keyfind(Tag, 1, ?MESSAGE_TABLE) of + {Tag, Record} -> {ok, Tag, Record}; + false -> error + end.
98-108:pack/1spec doesn't reflect error return.The spec says
-spec pack(map()) -> binary()but line 107 callserror({unknown_type, Record})which raises an exception rather than returning a value. The spec is technically correct (it never returns a non-binary), but the@docon line 88 says "Output: Binary encoded for TCP transmission" without mentioning the exception. Consider documenting the error case.apps/mumble_protocol/src/mumble_udp_proto.erl (1)
36-41: Nohandle/2clause for unrecognized or malformed packets.If the binary doesn't match either the ping pattern or the
<<Type:3, Target:5, Rest/binary>>pattern (e.g., a packet shorter than 1 byte), this will crash with afunction_clauseerror. Consider adding a catch-all clause that logs and discards malformed packets to improve resilience.🛡️ Proposed addition
handle(Session, <<Type:3, Target:5, Rest/binary>>) -> logger:debug("DataMsg~nType ~p~nTarget ~p", [Type, Target]), {Counter, R} = mumble_varint:decode(Rest), {Voice, Positional} = split_voice_positional(Type, R), mumble_server_conn:voice_data(Session#session.session_pid, - {voice_data, Type, Target, Counter, Voice, Positional}). + {voice_data, Type, Target, Counter, Voice, Positional}); +handle(_Session, Packet) -> + logger:warning("Ignoring malformed UDP packet: ~p bytes", [byte_size(Packet)]), + ok.apps/mumble_protocol/src/mumble_msg.erl (1)
38-39: No catch-all clause for unrecognizedmessage_typevalues.Both
raw_to_map/1andfrom_map/1will crash with afunction_clauseerror if an unknown record or map with an unrecognizedmessage_typeis passed. While this may be intentional (fail-fast), consider whether a more descriptive error (e.g.,error({unknown_message_type, Type})) would aid debugging in production.Also applies to: 499-499
apps/mumble_protocol/src/mumble_client_conn.erl (4)
82-84:get_state/1exposes the internal#state{}record to callers.The
#state{}record is private to this module (not exported via.hrl). Callers cannot pattern-match on the returned value. Consider returning a map with relevant fields instead, or exporting the record definition if intentional.
52-57:start_link/3case expression is a no-op wrapper.The
case gen_statem:start_link(...)just re-wraps{ok, Pid}and{error, Reason}unchanged. This can be simplified to a direct return.♻️ Proposed fix
start_link(Host, Port, Opts) -> Parent = self(), - case gen_statem:start_link(?MODULE, {Host, Port, Opts, Parent}, []) of - {ok, Pid} -> {ok, Pid}; - {error, Reason} -> {error, Reason} - end. + gen_statem:start_link(?MODULE, {Host, Port, Opts, Parent}, []).
151-153: No guard against sending when not yet inestablishedstate.
send/2andsend_voice/2usegen_statem:castwhich is fire-and-forget. If called before the FSM reachesestablished, the cast will hithandle_commonand be silently logged as "unhandled". Consider either documenting this limitation or returning an error viagen_statem:callforsend/2.
96-102: TLS connection uses{verify, verify_none}— no server certificate verification.This is common for Mumble (self-signed certs), but worth documenting that the client trusts any server certificate. Consider allowing callers to pass custom
verifyoptions viaOptsfor deployments that require certificate validation.apps/mumble_protocol/src/mumble_server_conn.erl (1)
449-450:logger:noticeon every sent message is too verbose for production.Line 450 logs at
noticelevel for every outgoing message including routine pings. This will flood logs under normal operation. Consider usingdebuglevel.♻️ Proposed change
- logger:notice("Sending ~p (~p bytes)", [maps:get(message_type, Map), byte_size(Bin)]), + logger:debug("Sending ~p (~p bytes)", [maps:get(message_type, Map), byte_size(Bin)]),apps/mumble_protocol/test/mumble_cert_tests.erl (1)
48-50:which opensslis platform-specific and doesn't fail gracefully.On minimal containers or Windows,
whichmay not exist or return unexpected output. If openssl is absent, this test fails but all subsequent tests will also fail with unhelpful errors. Consider making the test group conditional on openssl availability.apps/mumble_protocol/test/client_logic_tests.erl (4)
407-423:network_error_testandssl_close_testtest only Erlang primitives, not client behavior.
network_error_testasserts that atoms are atoms.ssl_close_testpattern-matches a literal tuple it just constructed. Neither exercises any code frommumble_client_conn. These are false-positive coverage signals.
328-342:established_receive_testtests Erlang'sreceive, not the client.Sending a message to
self()and receiving it proves only that Erlang message passing works. This doesn't validate thatmumble_client_connforwards messages to its parent.
182-211: Test named "connection_success_test" always expects connection failure.The test connects to
127.0.0.1:64738without starting a server, so it always takes the{error, econnrefused}path. The name is misleading. Consider renaming toconnection_attempt_without_server_testor similar.
268-285:version_sent_testandauthenticate_sent_testtest protocol utilities, not client state transitions.These are valid protocol roundtrip tests, but they're grouped under "authenticating state" which implies they test the client's authenticating state behavior. Consider moving them to a protocol utility test group for clarity.
apps/mumble_protocol/test/e2e_connection_SUITE.erl (1)
13-33:os:cmd("openssl ...")lacks error checking — silent failure if openssl is unavailable.If
opensslis not installed,os:cmdreturns an empty string or error text, and the suite proceeds with missing cert files. This would cause a confusing failure later inapplication:ensure_all_started(erlmur). Add a check for the generated files or openssl exit code.♻️ Add cert file validation after generation
os:cmd("openssl req -x509 -newkey rsa:2048 -keyout " ++ KeyFile ++ " -out " ++ CertFile ++ " -days 1 -nodes -subj '/CN=localhost'"), + true = filelib:is_file(CertFile), + true = filelib:is_file(KeyFile), ct:log("Key and Cert file ~p ~p",[KeyFile, CertFile]),apps/mumble_protocol/src/mumble_server_sup.erl (1)
77-78: Log messages claim children are starting, but they're emitted beforeinit/1returns.Lines 77-78 log "Starting UDP server..." and "Starting TCP listener..." before the supervisor has actually started any children. The children are started by the supervisor framework after
init/1returns. This is misleading during debugging. Consider removing these or moving to debug level with "will start" phrasing.apps/mumble_protocol/test/mumble_api_tests.erl (1)
9-15: OpenSSL availability check is fragile.
os:cmd("which openssl")is Unix-only and on some systems may return a "not found" message string rather than[]. Consider usingos:find_executable("openssl")which returnsfalsewhen not found—portable and unambiguous.Proposed fix
setup() -> - %% Ensure OpenSSL is available - case os:cmd("which openssl") of - [] -> error(openssl_not_available); - _ -> ok - end, + case os:find_executable("openssl") of + false -> error(openssl_not_available); + _ -> ok + end, ok.apps/mumble_protocol/src/mumble_server.erl (1)
111-130:start_linkcouples the supervisor lifetime to the caller.Same concern as with the client:
mumble_server_sup:start_link/1links the supervision tree to whatever process callsstart_server/1. If that process exits, the entire server tree goes down. If called from an application supervisor, this is fine; if called from a shell or one-off process, it's risky.apps/erlmur/src/erlmur_user_manager.erl (1)
157-181: O(n) full table scan inDOWNhandler to find user by PID.
ets:foldlscans every user record to find the one matching the downed PID. With many concurrent users, this runs on every disconnect inside the gen_server process, blocking other calls. Consider maintaining a secondarypid -> session_idETS table or map for O(1) lookup.Acceptable for an MVP with a small user count, but will become a bottleneck at scale.
apps/mumble_protocol/src/mumble_client.erl (1)
131-141:gen_statem:start_linkcreates an implicit link that couples caller lifetime to the connection process.
mumble_client_conn:start_link/3links the connection to the calling process viagen_statem:start_link. If the caller isn't trapping exits, an unexpected connection crash will terminate the caller (and vice versa). Since the public API (mumble:start_client) is designed for direct application-level use and there is no supervisor managing client connections, callers should be given more control. Consider usinggen_statem:start/3with explicitmonitor/2instead, allowing callers to decide how to handle connection failures.apps/mumble_protocol/test/test_utils.erl (1)
85-95: Minor:Timeout - Elapsedcan go negative, bypassing the0guard clause.When
Elapsed > Timeout, the recursive call passes a negative value. While Erlang'safterwith a negative timeout fires immediately (so no bug), the explicit_Pattern, 0, Accclause on line 85 is only reachable via a directwait_for_messages(P, 0)call, never from the recursion. Consider using awhen Timeout =< 0guard instead for clarity.Suggested fix
-wait_for_messages(_Pattern, 0, Acc) -> +wait_for_messages(_Pattern, Timeout, Acc) when Timeout =< 0 -> lists:reverse(Acc);apps/mumble_protocol/test/mumble_SUITE.erl (4)
75-96: OpenSSL availability check is fragile and Unix-specific.Line 77:
os:cmd("which openssl")returns empty only on Unix when not found; on Windows it would return an error string, not[]. Additionally, line 94 only matches errors starting with"Error"— OpenSSL failures may produce different prefixes (e.g.,"Can't","unable to").Consider using
os:find_executable("openssl")which returnsfalsewhen not found, and checking that the generated files exist (which you already do on lines 99-100) rather than parsing stderr.Suggested fix for OpenSSL check
- case os:cmd("which openssl") of - [] -> - ct:fail(openssl_not_available); - _ -> - ok - end, + case os:find_executable("openssl") of + false -> + ct:fail(openssl_not_available); + _ -> + ok + end,
380-413:auto_cert_reuse_testdepends on prior test's side effects.This test assumes
auto_cert_basic_testran successfully and left certs on disk (line 387 reads file info). While[sequence]ordering guarantees execution order, ifauto_cert_basic_testfails, this test will crash with{error, enoent}atfile:read_file_info/1rather than producing a clear skip or descriptive error.Consider guarding with a file existence check or adding
{require, auto_cert_basic_test}semantics.
566-571:cleanup_auto_generated_certsdeletes all.pemand.keyfiles in the app's priv_dir.This wildcard deletion is aggressive — it removes every
.pemand.keyfile, not just auto-generated ones. If the priv_dir contains other legitimate certificate files (e.g., shipped CA bundles), they'd be lost. Consider targeting only the known auto-generated filenames (e.g.,auto_server.pem,auto_server.key).Suggested fix
cleanup_auto_generated_certs() -> PrivDir = code:priv_dir(mumble_protocol), - Files = filelib:wildcard(filename:join(PrivDir, "*.pem")) ++ - filelib:wildcard(filename:join(PrivDir, "*.key")), + Files = [filename:join(PrivDir, "auto_server.pem"), + filename:join(PrivDir, "auto_server.key")], [file:delete(F) || F <- Files], ok.
518-563: Remove unused helper functionsget_server_port/1,get_ranch_port/1, andfind_port_in_listeners/1.These internal helper functions are never called within the test suite.
get_server_port/1only returns a hardcoded value, andget_ranch_port/1contains complex logic that is never exercised. Dead code should be removed to reduce maintenance burden.apps/mumble_protocol/test/mock_transport.erl (2)
87-88:recv/3uses the sameTimeoutfor both the gen_server call and the internal recv — potential double-timeout.
gen_server:call(Pid, {recv, Length, Timeout}, Timeout)setsTimeoutas both the gen_server call deadline and the value forwarded to the handler. If the handler blocks (current code), the gen_server:call will raise{timeout, ...}at roughly the same time the internalreceive afterfires, creating a race between a crash and a normal{error, timeout}reply.Even after fixing the blocking recv (per the comment above), consider whether the gen_server:call timeout should be slightly larger than the logical recv timeout, or use
infinityfor the call timeout since the handler now replies immediately.
40-42: Eachhandshake,connect, andmock_handshakecall spawns an unlinked, unsupervised gen_server.
handshake/2(line 77),connect/4(line 84), andmock_handshake/1(line 152) all callgen_server:start_link/3, linking the new process to the caller. In test scenarios where the caller exits or crashes before the mock socket is explicitly closed, these processes will be cleaned up by the link. However,start_linkhere links to whatever process happens to call it (often the test process), which may or may not be the intended owner. This is probably fine for tests but worth noting.Also applies to: 72-78, 83-85, 150-153
| ```erlang | ||
| -module(my_handler_mod). | ||
| -behaviour(mumble_server_behaviour). | ||
|
|
||
| % ... init and other callbacks ... | ||
|
|
||
| handle_msg(#'TextMessage'{message = <<"ping">>}, State) -> | ||
| % Send a message back to the client | ||
| % The server connection process (self() in this context) can be reached by | ||
| % storing it in your state or using a known registered name if applicable, | ||
| % but usually you have the connection PID. | ||
| mumble_server_conn:send(self(), #'TextMessage'{message = <<"pong">>}), | ||
| {ok, State}; | ||
| handle_msg(_Msg, State) -> | ||
| {ok, State}. | ||
| ``` |
There was a problem hiding this comment.
Code examples use Erlang records but the API expects maps — these examples will crash.
The server handler example at lines 74 and 79 uses record syntax (#'TextMessage'{...}), but mumble_server_conn passes decoded messages as maps (see mumble_server_conn.erl line 441 where Msg is a map from mumble_tcp_proto:decode). Similarly, send/2 at line 79 expects a map for packing via mumble_tcp_proto:pack. Users following this example will get runtime errors.
📝 Corrected example
-handle_msg(#'TextMessage'{message = <<"ping">>}, State) ->
- % Send a message back to the client
- % The server connection process (self() in this context) can be reached by
- % storing it in your state or using a known registered name if applicable,
- % but usually you have the connection PID.
- mumble_server_conn:send(self(), #'TextMessage'{message = <<"pong">>}),
- {ok, State};
+handle_msg(#{message_type := 'TextMessage', message := <<"ping">>}, State) ->
+ mumble_server_conn:send(self(), #{message_type => 'TextMessage', message => <<"pong">>}),
+ {ok, State};🤖 Prompt for AI Agents
In `@apps/mumble_protocol/README.md` around lines 68 - 83, The README examples use
Erlang record syntax (#'TextMessage'{...}) but the runtime passes and expects
maps (mumble_tcp_proto:decode returns a map and mumble_server_conn:send forwards
maps to mumble_tcp_proto:pack), so update the handler example in
my_handler_mod:handle_msg to construct and match on maps (e.g., %{"message" =>
<<"ping">>} and %{"message" => <<"pong">>}) instead of records, and ensure the
call to mumble_server_conn:send/2 sends a map; change any pattern matches and
sent payloads accordingly so examples match the map-based API.
| ```erlang | ||
| % Example: Starting a client | ||
| Opts = [{certfile, "client_cert.pem"}, {keyfile, "client_key.pem"}], | ||
| {ok, ClientPid} = mumble_client_conn:start_link({127, 0, 0, 1}, 64738, Opts). | ||
|
|
||
| % Sending a TCP message | ||
| mumble_client_conn:send(ClientPid, #'TextMessage'{message = <<"Hello Server!">>}). | ||
|
|
||
| % Sending voice data (binary) | ||
| % Packet: {voice_data, Type, Target, Counter, VoiceBin, PositionalBin} | ||
| Voice = {voice_data, 0, 0, 1, <<"audio bytes">>, undefined}, | ||
| mumble_client_conn:send_voice(ClientPid, Voice). | ||
| ``` | ||
|
|
||
| **Receiving Messages:** | ||
|
|
||
| The client forwards all decoded messages to the process that started it (its parent) as `{mumble_msg, Record}`. | ||
|
|
||
| ```erlang | ||
| % Example: Receiving in the parent process | ||
| loop() -> | ||
| receive | ||
| {mumble_msg, #'TextMessage'{message = Msg}} -> | ||
| io:format("Received text: ~s~n", [Msg]), | ||
| loop(); | ||
| {mumble_msg, #'ServerSync'{} = Sync} -> | ||
| io:format("Logged in. My session ID is ~p~n", | ||
| [Sync#'ServerSync'.session]), | ||
| loop(); | ||
| _Other -> | ||
| loop() | ||
| end. | ||
| ``` |
There was a problem hiding this comment.
Client example also uses records; should use maps for consistency with the actual API.
Lines 98, 102–103, and 114–119 all reference record syntax. The client API appears to use maps as well (the e2e test at e2e_connection_SUITE.erl:63,100 uses maps). Update the examples accordingly.
🤖 Prompt for AI Agents
In `@apps/mumble_protocol/README.md` around lines 92 - 124, Update the README
examples to use maps (the actual client API) instead of Erlang record syntax:
replace occurrences of #'TextMessage'{...} and #'ServerSync'{...} in the client
examples with map literals and map pattern-matching, and update the
send/send_voice calls and the parent loop() receive clauses to expect
{mumble_msg, Map} and match map keys (e.g., <<"message">> or atom keys used by
the API) rather than record fields; specifically adjust the examples around
mumble_client_conn:start_link, mumble_client_conn:send,
mumble_client_conn:send_voice and the loop() receive patterns so they show
sending/receiving maps consistent with the e2e_connection_SUITE tests.
| case ssl:connect(Host, Port, SslOptsList) of | ||
| {ok, Socket} -> | ||
| {next_state, authenticating, State#state{socket = Socket}}; | ||
| {error, Reason} -> | ||
| logger:error("Client failed to connect to ~p:~p reason: ~p", [Host, Port, Reason]), | ||
| {stop, Reason} |
There was a problem hiding this comment.
ssl:connect/3 called without a timeout — can block indefinitely.
If the remote host is unreachable or firewalled, this call can hang the gen_statem process with no upper bound. Use ssl:connect/4 with an explicit timeout.
🐛 Proposed fix
- case ssl:connect(Host, Port, SslOptsList) of
+ Timeout = maps:get(connect_timeout, Opts, 10000),
+ case ssl:connect(Host, Port, SslOptsList, Timeout) of🤖 Prompt for AI Agents
In `@apps/mumble_protocol/src/mumble_client_conn.erl` around lines 103 - 108, The
call to ssl:connect/3 in the gen_statem state transition can block indefinitely;
update the call in the connect branch to use ssl:connect/4 with an explicit
timeout value (e.g. a configured connect_timeout from the state or a reasonable
default like 5000 ms) so the process cannot hang forever. Locate the
ssl:connect(Host, Port, SslOptsList) invocation and replace it with
ssl:connect(Host, Port, SslOptsList, Timeout) (pass the Timeout from State such
as State#state.connect_timeout or add one to the state), keeping the same {ok,
Socket} and {error, Reason} handling and return values ({next_state,
authenticating, State#state{socket = Socket}} and {stop, Reason}). Ensure any
new field introduced (connect_timeout) is initialized where State is built.
| send_udp_ping(_State) -> | ||
| %% NOTE: Actual UDP send should happen here. | ||
| %% Mumble UDP Ping is <<1:3, 0:5, (64-bit timestamp)>> or similar. | ||
| logger:debug("Client would send UDP Ping here"). | ||
|
|
||
| send_udp(_State, _Payload) -> | ||
| %% NOTE: Actual UDP send should happen here. | ||
| logger:debug("Client would send UDP packet here"). |
There was a problem hiding this comment.
UDP send functions are stubs — send_udp_ping and send_udp are no-ops.
Both functions only log a debug message and never actually send any UDP traffic. Meanwhile, established(enter, ...) starts a recurring 5-second timer (Line 183) that calls send_udp_ping indefinitely. This creates unnecessary timer churn with no actual functionality.
Consider either implementing the UDP path or removing the timer and stubs until UDP support is ready. The udp_timer field in the state record is also never populated.
🤖 Prompt for AI Agents
In `@apps/mumble_protocol/src/mumble_client_conn.erl` around lines 217 - 224, The
send_udp_ping/1 and send_udp/2 functions are currently no-ops while
established(enter, ...) starts a recurring 5s timer, causing needless timers and
an unused udp_timer state field; either implement real UDP sending or remove the
timer and related stubs. To fix, choose one: (A) implement UDP support by
opening a gen_udp socket (or using existing socket helper), replace logger:debug
in send_udp_ping/1 and send_udp/2 with actual gen_udp:send calls, ensure the
socket is stored in the state and udp_timer is set to the timer reference
created in established(enter, ...), and cancel the timer on teardown; or (B)
remove the recurring timer setup in established(enter, ...) and delete or noop
the send_udp_ping/1 and send_udp/2 stubs (and remove udp_timer from the state)
so no timer churn occurs until UDP is implemented. Ensure changes reference the
functions send_udp_ping/1, send_udp/2, the established(enter, ...) clause, and
the udp_timer field so reviewers can find and validate the fix.
| from_map(#{message_type := 'Ping'} = Map) -> | ||
| #'Ping'{ | ||
| timestamp = maps:get(timestamp, Map, erlang:system_time(second)), | ||
| good = maps:get(good, Map, undefined), | ||
| late = maps:get(late, Map, undefined), | ||
| lost = maps:get(lost, Map, undefined), | ||
| resync = maps:get(resync, Map, undefined), | ||
| udp_packets = maps:get(udp_packets, Map, undefined), | ||
| tcp_packets = maps:get(tcp_packets, Map, undefined), | ||
| udp_ping_avg = maps:get(udp_ping_avg, Map, undefined), | ||
| udp_ping_var = maps:get(udp_ping_var, Map, undefined), | ||
| tcp_ping_avg = maps:get(tcp_ping_avg, Map, undefined), | ||
| tcp_ping_var = maps:get(tcp_ping_var, Map, undefined) | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Side-effectful default for timestamp in from_map for 'Ping'.
Line 521 uses erlang:system_time(second) as the default when the timestamp key is missing. This introduces a side effect in a pure conversion function, making it non-deterministic and harder to test. Additionally, Mumble ping timestamps are typically in microseconds (or at least milliseconds), not seconds — this default could produce unexpected values.
Consider using undefined as the default (consistent with other optional fields) and letting callers set the timestamp explicitly.
♻️ Proposed fix
from_map(#{message_type := 'Ping'} = Map) ->
#'Ping'{
- timestamp = maps:get(timestamp, Map, erlang:system_time(second)),
+ timestamp = maps:get(timestamp, Map, undefined),
good = maps:get(good, Map, undefined),🤖 Prompt for AI Agents
In `@apps/mumble_protocol/src/mumble_msg.erl` around lines 519 - 532, The from_map
clause for 'Ping' currently uses erlang:system_time(second) as a default for
timestamp which introduces a side effect and uses seconds (likely wrong unit);
change the default to undefined (consistent with other optional fields) so
from_map('Ping') is pure and callers must set timestamp explicitly, updating the
pattern in the from_map/1 function and the #'Ping'{timestamp = ...} construction
accordingly.
| start_server(Options) -> | ||
| CertFile = maps:get(cert_file, Options, undefined), | ||
| KeyFile = maps:get(key_file, Options, undefined), | ||
| Port = maps:get(port, Options, ?DEFAULT_PORT), | ||
| ServerHandler = maps:get(server_handler, Options, mock_mumble_handler), | ||
|
|
||
| %% Validate certificate files | ||
| case mumble_cert:validate_cert_files(CertFile, KeyFile) of | ||
| ok -> | ||
| start_server_with_certs(CertFile, KeyFile, Port, ServerHandler); | ||
| {error, Reason} -> | ||
| {error, Reason} | ||
| end. |
There was a problem hiding this comment.
Defaulting server_handler to mock_mumble_handler is unsafe for production.
mock_mumble_handler lives in the test directory and won't be available in a release build. If a caller omits server_handler from the options map, this will crash with undef at runtime. Either require the option or default to a production-ready handler.
Proposed fix — require the option explicitly
- ServerHandler = maps:get(server_handler, Options, mock_mumble_handler),
+ case maps:find(server_handler, Options) of
+ {ok, ServerHandler} ->
+ ok;
+ error ->
+ error({missing_required_option, server_handler})
+ end,Or alternatively, if you want to keep the map extraction pattern:
- ServerHandler = maps:get(server_handler, Options, mock_mumble_handler),
+ ServerHandler = maps:get(server_handler, Options, undefined),
+ ServerHandler =:= undefined andalso error({missing_required_option, server_handler}),🤖 Prompt for AI Agents
In `@apps/mumble_protocol/src/mumble_server.erl` around lines 34 - 46, The current
start_server/1 function defaults server_handler to test-only mock_mumble_handler
which will be undefined in releases; update start_server to require the
server_handler option (i.e. remove the mock default) or change the default to a
production handler (e.g. prod_mumble_handler) instead of mock_mumble_handler so
it won’t crash at runtime — locate the maps:get(server_handler, Options,
mock_mumble_handler) in start_server/1 and either remove the third argument and
return a clear {error, missing_server_handler} when absent, or replace the
default symbol with your production handler module name.
| established_transition_test() -> | ||
| %% Test that receiving ServerSync triggers established state | ||
| %% This requires a mock server | ||
|
|
||
| %% For now, verify the test structure | ||
| ?assert(true). |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Multiple tests are ?assert(true) placeholders providing zero coverage.
established_transition_test, client_close_test, and invalid_msg_test all just assert true. They give the impression of coverage while testing nothing. They'll silently pass even if the code they claim to test is completely broken. Either implement them with real assertions or remove them and track as TODO.
Also applies to: 344-350, 425-430
🤖 Prompt for AI Agents
In `@apps/mumble_protocol/test/client_logic_tests.erl` around lines 312 - 317, The
three tests established_transition_test, client_close_test, and invalid_msg_test
are only ?assert(true) placeholders and provide no coverage; replace each
placeholder with a real test or remove and mark TODO: for
established_transition_test implement sending a ServerSync-like mock message to
the client logic and assert the state becomes 'established' (use the same test
harness helpers used elsewhere in the file), for client_close_test simulate a
server/client close sequence and assert proper cleanup/closed state and expected
messages, and for invalid_msg_test send an invalid/garbage message and assert
the client drops it or logs an error (matching existing error/assert helpers);
if you cannot implement now, delete the placeholder tests and add a TODO comment
referencing these test names so they are not silently passing.
| handle_call({recv, _Length, _Timeout}, _From, State = #{message_queue := [Msg | Rest]}) -> | ||
| {reply, {ok, Msg}, State#{message_queue := Rest}}; | ||
| handle_call({recv, _Length, Timeout}, _From, State) -> | ||
| %% Wait for a message to be injected | ||
| receive | ||
| {inject_message, Data} -> | ||
| {reply, {ok, Data}, State} | ||
| after Timeout -> | ||
| {reply, {error, timeout}, State} | ||
| end; |
There was a problem hiding this comment.
Blocking receive inside handle_call will never match send_message/2 injections — deadlock risk.
When the message queue is empty, handle_call({recv, ...}) enters a raw receive waiting for {inject_message, Data}. However, send_message/2 uses gen_server:cast/2, which delivers the message as {'$gen_cast', {inject_message, Data}}. The raw receive will never match this wrapped form, so recv will always time out when the queue is empty.
Furthermore, this raw receive blocks the gen_server process, preventing it from processing any other calls or casts until the timeout fires. This means even if the inject arrives via a different path, the process is stuck.
Suggested fix — only serve from the queue, don't block
handle_call({recv, _Length, _Timeout}, _From, State = #{message_queue := [Msg | Rest]}) ->
{reply, {ok, Msg}, State#{message_queue := Rest}};
-handle_call({recv, _Length, Timeout}, _From, State) ->
- %% Wait for a message to be injected
- receive
- {inject_message, Data} ->
- {reply, {ok, Data}, State}
- after Timeout ->
- {reply, {error, timeout}, State}
- end;
+handle_call({recv, _Length, _Timeout}, _From, State) ->
+ {reply, {error, timeout}, State};If blocking recv semantics are truly needed, consider restructuring to reply asynchronously: store the From reference in state, return {noreply, State}, and reply from handle_cast({inject_message, ...}) when data arrives.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| handle_call({recv, _Length, _Timeout}, _From, State = #{message_queue := [Msg | Rest]}) -> | |
| {reply, {ok, Msg}, State#{message_queue := Rest}}; | |
| handle_call({recv, _Length, Timeout}, _From, State) -> | |
| %% Wait for a message to be injected | |
| receive | |
| {inject_message, Data} -> | |
| {reply, {ok, Data}, State} | |
| after Timeout -> | |
| {reply, {error, timeout}, State} | |
| end; | |
| handle_call({recv, _Length, _Timeout}, _From, State = #{message_queue := [Msg | Rest]}) -> | |
| {reply, {ok, Msg}, State#{message_queue := Rest}}; | |
| handle_call({recv, _Length, _Timeout}, _From, State) -> | |
| {reply, {error, timeout}, State}; |
🤖 Prompt for AI Agents
In `@apps/mumble_protocol/test/mock_transport.erl` around lines 176 - 185, The
handle_call clause for recv must not perform a raw receive (it will never match
the {'$gen_cast',...} wrapper and blocks the gen_server); instead remove the
blocking receive in handle_call({recv, ...}, ...) and either (a) immediately
return {reply, {error, timeout}, State} when message_queue is empty, or (b)
implement asynchronous waiting: store the caller From (and timeout metadata) in
State (e.g. pending_recv key) and return {noreply, StateUpdated}, then in
handle_cast({inject_message, Data}, State) (the handler for send_message/2
casts) check for a pending_recv and call gen_server:reply(From, {ok, Data}) (or
enqueue if none) before updating State — ensure handle_cast matches the
inject_message tuple and updates message_queue accordingly.
| client_connect_disconnect_test(Config) -> | ||
| CertFile = ?config(cert_file, Config), | ||
| KeyFile = ?config(key_file, Config), | ||
|
|
||
| %% Start server | ||
| case mumble:start_server(CertFile, KeyFile, 0) of | ||
| {ok, {mumble_server, _, _} = ServerRef} -> | ||
| %% Server started successfully | ||
| ct:pal("Server started: ~p", [ServerRef]), | ||
|
|
||
| %% Cleanup | ||
| ok = mumble:stop_listener(ServerRef), | ||
| ok; | ||
| Other -> | ||
| ct:fail({unexpected_server_result, Other}) | ||
| end. |
There was a problem hiding this comment.
Multiple "client" and "full flow" tests are stubs that only test server start/stop.
client_connect_disconnect_test, client_with_callback_test, client_server_communication_test, and multiple_clients_test all share the same body: start a server, log, stop. They don't connect any client, test callbacks, send messages, or exercise multiple clients as their names promise. This inflates perceived coverage without testing the advertised functionality.
Consider either implementing the actual test logic or clearly marking them as {skip, "Not yet implemented"} (like server_multiple_instances_test) to avoid false confidence.
🤖 Prompt for AI Agents
In `@apps/mumble_protocol/test/mumble_SUITE.erl` around lines 287 - 302, The tests
named client_connect_disconnect_test, client_with_callback_test,
client_server_communication_test, and multiple_clients_test currently only
start/stop the server and do not exercise the client behaviors their names
promise; update each test to either implement the intended behavior (e.g., use
mumble:connect/3 or equivalent to create a client, perform connect/disconnect
flows, register and assert callback invocations in client_with_callback_test,
send/receive messages in client_server_communication_test, and spawn/connect
multiple clients and assert concurrency in multiple_clients_test) or explicitly
mark the test as skipped by returning {skip, "Not yet implemented"} (matching
the pattern used by server_multiple_instances_test); locate and edit the test
functions (client_connect_disconnect_test, client_with_callback_test,
client_server_communication_test, multiple_clients_test) to apply one of these
fixes so the suite no longer gives false confidence.
ocb128 keep track of stats feat: implement mumble.erl API with comprehensive documentation Add complete Mumble protocol implementation including: API Implementation: - start_server/1,2,3 with certificate validation and auto-generation - start_client/4,5 with TLS support and callback handling - stop_listener/1 and stop_client/1 for cleanup - send/2, send_voice/2, get_state/1 for client operations - server_version/0 and serverconfig/0 for server info Connection Handling: - mumble_client_conn: gen_statem-based client connection handler - mumble_server_conn: server-side connection with OCB-AES128 encryption - mumble_server_sup: supervisor managing TCP and UDP servers - mumble_udp_server: UDP voice traffic handling Protocol Support: - mumble_tcp_proto: TCP message packing/unpacking - mumble_udp_proto: UDP packet handling - mumble_varint: Variable-length integer encoding - mumble_msg: Record to map conversions Behaviours: - mumble_client_behaviour: client callback interface - mumble_server_behaviour: server handler interface Documentation: - Complete -moduledoc for all modules - Function -doc attributes with inputs/outputs - Type documentation for public types - Usage examples throughout Testing: - mumble_api_tests: API validation tests - mumble_client_api_tests: client functionality tests - mumble_cert_tests: certificate generation tests - mumble_SUITE: integration tests All 128 EUnit tests, 34 CT tests, and 5 property tests pass. Dialyzer type checking passes with no warnings. Refs: erlmur architecture v1.0 refactor: split mumble.erl into specialized modules test: add comprehensive connection logic unit tests Fix eunit tests - Fixed client connection tests that were failing due to unhandled process exits - Added trap_exit and logger suppression to prevent test failures - Fixed mock handler and transport setup for server tests - Client tests now pass successfully Add mock transport support for server tests - Fixed client connection tests that were failing due to unhandled process exits - Added trap_exit and logger suppression to prevent test failures - Fixed mock handler and transport setup for server tests - Client tests now pass successfully Add mock transport support for server tests - Fixed client connection tests that were failing due to unhandled process exits - Added trap_exit and logger suppression to prevent test failures - Fixed mock handler and transport setup for server tests - Client tests now pass successfully Fixed build warnings Fixed ping
There was a problem hiding this comment.
Actionable comments posted: 5
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/ocb128_crypto/test/ocb128_crypto_benchmark_SUITE.erl (1)
3-11:⚠️ Potential issue | 🔴 CriticalMissing exports for individual benchmark test functions.
The 18 test functions (
encrypt_16b/1throughdecrypt_4mb/1, lines 78–96) are defined but not exported. WhenRUN_BENCHMARKS=true,all/0returns their names and Common Test will attempt to call them, resulting inundefinederrors.🐛 Add missing exports
-export([ all/0, groups/0, init_per_suite/1, end_per_suite/1, init_per_testcase/2, end_per_testcase/2, - run_benchmark/2 + run_benchmark/2, + encrypt_16b/1, encrypt_32b/1, encrypt_128b/1, + encrypt_1kb/1, encrypt_4kb/1, encrypt_16kb/1, + encrypt_128kb/1, encrypt_1mb/1, encrypt_4mb/1, + decrypt_16b/1, decrypt_32b/1, decrypt_128b/1, + decrypt_1kb/1, decrypt_4kb/1, decrypt_16kb/1, + decrypt_128kb/1, decrypt_1mb/1, decrypt_4mb/1 ]).
🤖 Fix all issues with AI agents
In `@apps/erlmur/src/erlmur_server_handler.erl`:
- Around line 26-34: The authenticate/2 function currently does a destructive
match on the call to erlmur_user_manager:register_user(self(), Username) which
will crash on {error,_}; update authenticate/2 to handle both successful and
error tuples from erlmur_user_manager:register_user/2 instead of using a direct
{ok, SessionId} = ... match: on {ok, SessionId} proceed to build UserInfo, log
with logger:info and return {ok, UserInfo, State#state{session_id = SessionId,
username = Username}}; on {error, Reason} log a warning or error with the Reason
(using logger:warning/2 or logger:error/2) and return a non-crashing error tuple
such as {error, {register_failed, Reason}, State} (or {error, invalid_auth,
State} if you prefer), ensuring you reference the authenticate/2 function,
erlmur_user_manager:register_user/2, SessionId, UserInfo and State#state when
making the change.
In `@apps/mumble_protocol/include/mumble_protocol.hrl`:
- Around line 42-56: The record stats currently types udp_ping_avg,
udp_ping_var, tcp_ping_avg and tcp_ping_var as non_neg_integer(), which
conflicts with the protobuf Ping definition that emits floats; update the stats
record (record name: stats) to declare those four fields as float() and set
their defaults to 0.0 (udp_ping_avg, udp_ping_var, tcp_ping_avg, tcp_ping_var)
so the types match gpb-decoded values and avoid runtime/type-mismatch issues.
In `@apps/mumble_protocol/src/mumble_server_behaviour.erl`:
- Around line 61-62: The callback spec for authenticate/2 is incorrect: it
currently lists an error return as {error, Reason} but the implementation
erlmur_server_handler:authenticate/2 returns {error, invalid_auth, State};
update the behaviour spec to allow a 3-tuple error that includes the preserved
State (e.g. {error, Reason :: any(), State :: any()}) so the contract matches
the implementation and callers can recover state after failed auth.
In `@apps/mumble_protocol/test/mock_transport.erl`:
- Around line 223-239: handle_cast/2's inject_message currently both sends the
message to Owner (active mode) and appends it to message_queue, causing recv/3
to return duplicates; change the logic in handle_cast({inject_message, Data},
State = #{owner := Owner, opts := Opts}) so that when Opts contains {active,
true} or {active, once} and Owner =/= undefined you do NOT append Data to
message_queue (i.e., set NewQueue = Queue unchanged or skip insertion), and only
enqueue Data when not in active mode; ensure State#{message_queue := NewQueue}
still updates correctly and that the active-once semantics (if any) are
preserved for subsequent deliveries.
In `@apps/mumble_protocol/test/mumble_client_api_tests.erl`:
- Around line 7-11: The test generator mumble_client_api_test_/0 currently only
lists client_start_invalid_certs/0 and client_invalid_ref/0 while the functions
client_start_no_certs/0, client_send_message/0, client_send_voice/0,
client_get_state/0, and client_stop/0 are defined but never executed; update
mumble_client_api_test_/0 to include all those test function references
(client_start_no_certs, client_send_message, client_send_voice,
client_get_state, client_stop) so EUnit will run them, or alternatively remove
the unused function definitions if they are intentionally deprecated.
🟡 Minor comments (18)
apps/mumble_protocol/README.md-31-31 (1)
31-31:⚠️ Potential issue | 🟡 MinorTypo: "recieved" → "received".
Proposed fix
-If no `Ping` is recieved for 30sec the client will be disconnected from the server. +If no `Ping` is received for 30 seconds, the client will be disconnected from the server.AGENTS.md-63-67 (1)
63-67:⚠️ Potential issue | 🟡 MinorIncorrect Erlang syntax in error handling guideline.
Line 65: Erlang's
tryexpression doesn't have anelseclause. The correct form istry Expr of Pattern -> Body catch Class:Reason -> Handler after Cleanup end. Consider correcting totry...of...catch...after.Proposed fix
-- Use `try...of...else` for exceptions that need cleanup +- Use `try...of...catch...after` for exceptions that need cleanupapps/mumble_protocol/README.md-37-37 (1)
37-37:⚠️ Potential issue | 🟡 MinorTypo: "reciveing" → "receiving".
Proposed fix
-If reciveing a `UDPTunnel` message the connection is marked "UDP verified" false, until new UDP verification is done. +If receiving a `UDPTunnel` message, the connection is marked "UDP verified" false until new UDP verification is done.AGENTS.md-82-86 (1)
82-86:⚠️ Potential issue | 🟡 MinorInaccurate Common Test guideline.
Line 84: Erlang Common Test does not define a
-suite()attribute. CT suites exportsuite/0,all/0,init_per_suite/1, etc. as functions. Consider correcting the guidance.Proposed fix
-- Common Test: Define `-suite()` and `-export([suite/0, ...])` +- Common Test: Export `suite/0`, `all/0`, `init_per_suite/1`, `end_per_suite/1`, etc.justfile-45-45 (1)
45-45:⚠️ Potential issue | 🟡 Minor
find -name "doc"may delete unintended directories.This will recursively find and remove any directory named
docunder the project root, not just build-generated documentation. Consider scoping it to the known output location (e.g.,rm -rf docat the project root) or excluding source directories.Proposed fix
- find {{justfile_directory()}} -name "doc" -exec rm -rf {} + + rm -rf {{justfile_directory()}}/docapps/erlmur/src/erlmur_sup.erl-23-32 (1)
23-32:⚠️ Potential issue | 🟡 MinorMisleading log message — child hasn't started yet at Line 31.
init/1only builds the child spec; the supervisor starts children afterinit/1returns. The log at Line 31 will print "User manager started" before the process is actually started, which is misleading during troubleshooting.Proposed fix
init([]) -> logger:info("[erlmur_sup] Starting user manager..."), UserManager = #{ id => erlmur_user_manager, start => {erlmur_user_manager, start_link, []}, restart => permanent, type => worker }, - logger:info("[erlmur_sup] User manager started"), + logger:info("[erlmur_sup] Supervisor init complete, children will be started"), {ok, {{one_for_one, 5, 10}, [UserManager]}}.apps/mumble_protocol/src/mumble_version.erl-37-46 (1)
37-46:⚠️ Potential issue | 🟡 Minor
decode/1V1/V2 discrimination can misinterpret small V2 values.The heuristic
V1 =< 16#FFFFFFFFvsV2 > 16#FFFFFFFFbreaks for V2-encoded versions with small fields. For example, V2 for0.0.1encodes to65536(fits in 32 bits), sodecode(65536)would parse it as V1 and yield0.1.0instead. In practice this may not occur if Mumble versions are always ≥ 1.x, but the ambiguity is worth documenting.Consider adding a two-argument variant
decode(v1, Int)/decode(v2, Int)so callers can be explicit when the source format is known (as it is in protobuf messages whereversion_v1andversion_v2are separate fields).apps/erlmur/src/erlmur_server_handler.erl-78-86 (1)
78-86:⚠️ Potential issue | 🟡 Minor
os_versionis a string (list) while other fields are binaries — type inconsistency.
erlang:system_info(otp_release)returns a character list (e.g.,"26"), butreleaseandosare binaries. This may cause issues if downstream code expects all values to be binaries.♻️ Proposed fix
- os_version => erlang:system_info(otp_release) + os_version => list_to_binary(erlang:system_info(otp_release))apps/mumble_protocol/src/mumble_server.erl-86-95 (1)
86-95:⚠️ Potential issue | 🟡 Minor
os_versionis a string (list) — should be binary for consistency.
erlang:system_info(otp_release)returns a character list (e.g.,"27"), but all other string-like fields in the version record (release,os) are binaries. The same inconsistency appears inerlmur_server_handler:get_caps/1.♻️ Proposed fix
- os_version = erlang:system_info(otp_release) + os_version = list_to_binary(erlang:system_info(otp_release))apps/erlmur/src/erlmur_user_manager.erl-81-86 (1)
81-86:⚠️ Potential issue | 🟡 MinorUsername logged at
infolevel — consider PII implications.Line 85 logs the username in plain text. Depending on your compliance requirements, user-identifying information in logs may need to be masked or logged at
debuglevel only.apps/mumble_protocol/src/mumble_udp_server.erl-38-40 (1)
38-40:⚠️ Potential issue | 🟡 Minor
send/3spec promises{error, term()}butgen_server:cast/2always returnsok.The spec
-> ok | {error, term()}is misleading. Sincesend/3delegates togen_server:cast, it will always returnokto the caller — any send error is handled asynchronously insidehandle_cast. Either update the spec to-> okor switch togen_server:callif the caller needs error feedback.🔧 Proposed fix (if async is intended)
--spec send(IP :: inet:ip_address(), inet:port_number(), binary()) -> ok | {error, term()}. +-spec send(IP :: inet:ip_address(), inet:port_number(), binary()) -> ok.apps/mumble_protocol/src/mumble_udp_server.erl-169-171 (1)
169-171:⚠️ Potential issue | 🟡 Minor
get_port/0spec returnsinet:port_number()but the function returns{ok, Port}.The
handle_call(get_port, ...)at line 58-59 replies with{ok, Port}, soget_port/0actually returns{ok, inet:port_number()}. The spec should match.🔧 Proposed fix
--spec get_port() -> inet:port_number(). +-spec get_port() -> {ok, inet:port_number()}. get_port() -> gen_server:call(?MODULE, get_port).apps/mumble_protocol/src/mumble_client_conn.erl-154-177 (1)
154-177:⚠️ Potential issue | 🟡 MinorEvery received message logged at
infolevel — will be noisy in production.Line 162 logs every message type at
infolevel. For an active connection receiving frequent Ping and voice data messages, this will flood logs. Downgrade todebug.🔧 Proposed fix
lists:foreach(fun(M) -> - logger:info("Client received: ~p", [maps:get(message_type, M)]), + logger:debug("Client received: ~p", [maps:get(message_type, M)]),apps/mumble_protocol/src/mumble_server_conn.erl-447-457 (1)
447-457:⚠️ Potential issue | 🟡 Minor
send_msglogs every outgoing message atnoticelevel.Line 450 (
logger:notice("Sending ~p (~p bytes)", ...)) runs for every message sent, including frequent Ping responses. This will clutter production logs. Downgrade todebug.🔧 Proposed fix
- logger:notice("Sending ~p (~p bytes)", [maps:get(message_type, Map), byte_size(Bin)]), + logger:debug("Sending ~p (~p bytes)", [maps:get(message_type, Map), byte_size(Bin)]),apps/mumble_protocol/test/e2e_connection_SUITE.erl-40-44 (1)
40-44:⚠️ Potential issue | 🟡 MinorTest listeners may leak if a test crashes before
ranch:stop_listener.Each test manually calls
ranch:stop_listener(Name)at the end, but if the test fails before reaching that line, the listener remains running. Useend_per_testcaseto store and clean up the listener name.♻️ Suggested approach
Store the listener name in the Config during
init_per_testcase(or return it from the test setup), then stop it inend_per_testcase:+end_per_testcase(_TC, Config) -> + case ?config(listener_name, Config) of + undefined -> ok; + Name -> catch ranch:stop_listener(Name) + end, + ok.apps/mumble_protocol/test/mumble_SUITE.erl-238-240 (1)
238-240:⚠️ Potential issue | 🟡 MinorBrittle record field access via
element/3.
element(3, State)assumes the port is at a fixed position in the#state{}record tuple. If the record definition inmumble_udp_serveris reordered or fields are added beforeport, this silently reads the wrong value. Usesys:get_state/1with a record pattern or export an accessor frommumble_udp_serverinstead.apps/mumble_protocol/test/mumble_SUITE.erl-380-413 (1)
380-413:⚠️ Potential issue | 🟡 Minor
auto_cert_reuse_testdepends on side effects from the previous test.This test assumes
auto_cert_basic_testalready ran and left cert files on disk (line 387: reads file info of the auto-generated cert). Because the group uses[sequence], this works in the happy path, but ifauto_cert_basic_testfails or is skipped, this test will crash with a confusing{error, enoent}onfile:read_file_info. Consider generating the cert explicitly in this test's setup or guarding with a file-existence check.apps/mumble_protocol/include/mumble_protocol.hrl-85-85 (1)
85-85:⚠️ Potential issue | 🟡 MinorRemove or clarify the unused
typefield in the session record.The type spec
type = regular :: regular | typecontains an ambiguous union alternative that shadows the Erlang keywordtype. The field is not used anywhere in the codebase. Either remove this field entirely or, if it's intended for future use, replace the uncleartypeatom with a descriptive name (e.g.,regular | special,regular | bot, or similar) that clarifies the semantic distinction.
🧹 Nitpick comments (25)
apps/ocb128_crypto/src/ocb128_crypto.erl (2)
1-18: Exportedencrypt_ocb/3anddecrypt_ocb/3lack-specand-docannotations.These two functions are part of the public API (exported on lines 16–17) and are exercised in tests and property checks, yet they have no type specifications or documentation, unlike every other exported function in this module. Adding specs improves Dialyzer coverage and keeps the public surface consistent.
♻️ Proposed specs
+ +-doc """ +Encrypts a binary payload using AES-128 in OCB mode and returns a map +containing the ciphertext and authentication tag. +""". +-spec encrypt_ocb(binary(), key(), nonce()) -> #{ciphertext := binary(), tag := binary()}. encrypt_ocb(Plain, Key, IV) -> ... + +-doc """ +Decrypts a binary ciphertext using AES-128 in OCB mode and returns a map +containing the plaintext and authentication tag. +""". +-spec decrypt_ocb(binary(), key(), nonce()) -> #{text := binary(), tag := binary()}. decrypt_ocb(Encrypted, Key, IV) -> ...
423-458: Stats accounting forlatelooks correct but is subtle — consider a brief inline comment.Line 444:
#crypto_stats{lost = -1, late = 1}adjusts for a previously-counted lost packet that has now arrived late. The negativelostdelta is correct (it reverts the earlier "lost" assumption), but this is non-obvious to future readers.apps/ocb128_crypto/test/prop_ocb128_crypto.erl (1)
67-86: Verify that theLateDecryptedStatere-binding on line 78 is intentional.Line 68 binds
LateDecryptedStateviamapfoldl, and line 78 re-uses the same variable name in afoldl. In Erlang, this is a match assertion — it asserts thefoldlresult equals themapfoldlaccumulator. If thedroperrors return an unmodified state (which they do per the currentupdate_state_by_classificationfordrop), this assertion will pass. This is correct but subtle.apps/ocb128_crypto/test/ocb128_crypto_benchmark_SUITE.erl (1)
18-22: Inconsistent indentation: tabs insideall/0, spaces elsewhere.Lines 20–22 use tab indentation while the rest of the file uses spaces.
♻️ Proposed fix
all() -> case os:getenv("RUN_BENCHMARKS") of - "true" -> maps:keys(test_params()); - _ -> [] - end. + "true" -> maps:keys(test_params()); + _ -> [] + end.apps/ocb128_crypto/test/ocb128_crypto_tests.erl (1)
134-135:_Stateis used on the next line — drop the underscore prefix.By Erlang convention,
_Varsignals the variable is intentionally unused. Referencing_Stateon line 135 contradicts that signal.♻️ Proposed fix
- fun({{ok, <<>>, _State}, I}) -> - Stats = ocb128_crypto:stats(_State), + fun({{ok, <<>>, DecState}, I}) -> + Stats = ocb128_crypto:stats(DecState),apps/mumble_protocol/test/prop_protocol_tests.erl (1)
10-13: Placeholder property provides no value.
prop_tcp_proto_roundtrip/0always returnstrue, which means it silently passes without testing anything. Consider either implementing it or removing it to avoid giving a false sense of coverage.rebar.config (1)
40-45: Dependencies pinned to branch heads — consider using tags or commit SHAs.Both
ranchandeqwalizer_supportare pinned to branch heads (master/main), which means builds are not reproducible and could break unexpectedly when upstream pushes breaking changes.apps/mumble_protocol/docs/establishing_connection.md (1)
1-140: Well-written protocol documentation. Minor markdown formatting nits flagged by linter.The content is technically thorough and the Mermaid sequence diagram is a great addition. Static analysis flagged multiple
MD022(blanks-around-headings) andMD031(blanks-around-fences) violations throughout the file. Adding blank lines before and after headings and fenced code blocks would resolve these.apps/mumble_protocol/test/mumble_client_api_tests.erl (1)
13-17:setup/0andcleanup/1are defined but unused.These hooks are not referenced by the test generator. Either wire them into the fixture or remove them.
apps/mumble_protocol/test/client_logic_tests.erl (1)
407-423:network_error_testandssl_close_testprovide no meaningful coverage.
network_error_testonly asserts that atoms are atoms.ssl_close_testpattern-matches a tuple it just constructed. Neither exercises any code inmumble_client_conn. Consider implementing real tests or marking them as TODO stubs explicitly.apps/mumble_protocol/src/mumble_cert.erl (1)
102-104: Inconsistent indentation — mixed tabs and spaces.Lines 103–104 and 121 use tab indentation while the rest of the file uses spaces. This hinders readability.
Proposed fix
maybe - {ok,_} ?= {filelib:ensure_dir(KeyFile),KeyFile}, - {ok,_} ?= {filelib:ensure_dir(CertFile),CertFile}, + {ok, _} ?= {filelib:ensure_dir(KeyFile), KeyFile}, + {ok, _} ?= {filelib:ensure_dir(CertFile), CertFile},else {false, Reason} -> {error, {cert_generation_failed, string:trim(Reason)}}; - {{error, Reason}, File} -> + {{error, Reason}, File} -> {error, {auto_cert_failed, Reason, File}} end.Also applies to: 121-121
apps/mumble_protocol/test/mumble_api_tests.erl (1)
9-15:os:cmd("which openssl")may not return[]on all failure paths.On some systems,
whichwrites errors to stdout (e.g.,"which: no openssl in ..."), so the check against[]would pass even when OpenSSL is absent. Consider checking the exit code or verifying the returned path exists.♻️ Suggested improvement
setup() -> - %% Ensure OpenSSL is available - case os:cmd("which openssl") of - [] -> error(openssl_not_available); - _ -> ok - end, + %% Ensure OpenSSL is available + Output = string:trim(os:cmd("which openssl")), + case Output of + "" -> error(openssl_not_available); + Path -> + case filelib:is_file(Path) of + true -> ok; + false -> error(openssl_not_available) + end + end, ok.apps/mumble_protocol/test/mock_mumble_handler.erl (1)
15-31: Remove speculative comments; mock sends toself()which may not be the connection process.Lines 23–26 contain uncertain comments about how
mumble_server_connworks. In a mock,self()is the test process (or whatever callshandle_msg), not the connection pid. If this mock is only used in integration tests where the call originates frommumble_server_conn, thenself()is correct — but if used elsewhere the send target will be wrong. Clean up the speculative comments regardless.♻️ Clean up comments
'TextMessage' -> - %% Reply with another text message Reply = #{ message_type => 'TextMessage', message => <<"Echo: ", (maps:get(message, Msg))/binary>> }, - %% We need to know who to send it to, but mock doesn't have session pid easily. - %% mumble_server_conn handles casting back if we return it? - %% Actually mumble_server_conn:handle_protocol_msg calls Mod:handle_msg. - %% If we want to send something back, we'd typically use mumble_server_conn:send(self(), Reply), mumble_server_conn:send(self(), Reply), {ok, State};apps/mumble_protocol/src/mumble_tcp_proto.erl (2)
137-144:find_msg_by_tag/1always traverses the full list — uselists:keyfind/3instead.
foldlcannot short-circuit: once the tag is found, it still iterates over remaining entries.lists:keyfind/3(orlists:search/2likefind_msg_by_record/1uses) stops on first match.♻️ Proposed refactor
find_msg_by_tag(Tag) -> - lists:foldl(fun ({T, Record}, _Acc) when T =:= Tag -> - {ok, Tag, Record}; - (_, Acc) -> - Acc - end, - error, - ?MESSAGE_TABLE). + case lists:keyfind(Tag, 1, ?MESSAGE_TABLE) of + {Tag, Record} -> {ok, Tag, Record}; + false -> error + end.
98-108: Spec forpack/1claims it always returnsbinary(), but it can throw.Line 107 calls
error({unknown_type, Record})for unknown message types, yet the spec on line 98 declares-spec pack(map()) -> binary(). Either update the spec to includeno_return()or return an error tuple.♻️ Option: return error tuple instead of throwing
--spec pack(map()) -> binary(). +-spec pack(map()) -> binary() | {error, {unknown_type, term()}}. pack(MessageMap) when is_map(MessageMap) -> logger:debug("pack ~p", [MessageMap]), Record = mumble_msg:from_map(MessageMap), case find_msg_by_record(Record) of {ok, Tag, _} -> Bin = 'Mumble_gpb':encode_msg(Record), encode_message(Tag, Bin); error -> - error({unknown_type, Record}) + {error, {unknown_type, Record}} end.apps/mumble_protocol/src/mumble_msg.erl (1)
499-774: No catch-all clause infrom_map/1— unknownmessage_typegives an opaquefunction_clauseerror.If
from_mapreceives a map with an unrecognizedmessage_type, the crash will be a genericfunction_clausethat doesn't indicate which type was unexpected. A catch-all clause would improve debuggability.♻️ Proposed addition at the end of from_map
from_map(#{message_type := Type}) -> error({unknown_message_type, Type}); from_map(Map) -> error({missing_message_type, Map}).apps/mumble_protocol/src/mumble_server.erl (1)
113-130: Callingstart_linkoutside a supervisor links the supervision tree to the caller.
mumble_server_sup:start_link(SupOptions)links the new supervisor to whichever process callsstart_server/1. If that caller terminates, the entire supervision tree goes down. Consider usingstart_linkonly from a supervisor, or usesupervisor:start_childto attach it under an existing supervision tree.apps/erlmur/src/erlmur_user_manager.erl (2)
157-180: DOWN handler performs a full ETS table scan to find a user by PID.
ets:foldliterates every entry even after finding the target. For a small user base this is fine, but it won't scale well. Consider maintaining a secondarypid → session_idETS table (orets:match_object/2with a match spec on thepidfield) for O(1) lookup.
107-115:unregister_userdoes not demonitor the process.When a user is explicitly unregistered, the monitor set up during
register_user(line 84) is never cancelled. The monitor ref isn't stored, sodemonitor/1can't be called. This means a subsequentDOWNmessage will still arrive and trigger thehandle_infocleanup path (which will be a no-op since the user is already deleted). Not harmful, but slightly wasteful.♻️ Suggested approach
Store the monitor reference in the
#user{}record and callerlang:demonitor(MonRef, [flush])in theunregister_userhandler.-record(user, { session_id :: pos_integer(), pid :: pid(), username :: binary(), - udp_addr :: {inet:ip_address(), inet:port_number()} | undefined + udp_addr :: {inet:ip_address(), inet:port_number()} | undefined, + mon_ref :: reference() | undefined }).apps/mumble_protocol/test/server_logic_tests.erl (1)
14-14: Unused?TEST_TIMEOUTmacro.The macro is defined but never referenced. All timeouts in the test functions use hardcoded values. Either use it or remove it.
apps/mumble_protocol/src/mumble_client_conn.erl (1)
130-147: Stats field naming is misleading on the client side.Fields like
from_client_tcp_packetsandfrom_client_tcp_bytes(lines 132-133) are being incremented when the client receives data from the server. The naming implies traffic originating from the client. Consider renaming to something directionally clear (e.g.,received_tcp_packets/received_tcp_bytes) or documenting that the field names are reused from the shared#stats{}record.apps/mumble_protocol/test/test_utils.erl (1)
85-95:wait_for_messagescan pass a negative timeout to the recursive call.If
Elapsed > Timeout, the computedTimeout - Elapsedgoes negative. While Erlang treats negativeaftervalues as 0, the_Pattern, 0base case at line 85 won't match a negative value, causing one extra unnecessary receive call. Consider usingmax(0, Timeout - Elapsed).♻️ Proposed fix
- wait_for_messages(Pattern, Timeout - Elapsed, [Msg | Acc]) + wait_for_messages(Pattern, max(0, Timeout - Elapsed), [Msg | Acc])apps/mumble_protocol/test/mumble_SUITE.erl (2)
75-96: Fragile OpenSSL error detection.Line 78:
os:cmd("which openssl")returns""on macOS/Linux when not found, but may include a trailing newline on some shells. On Windows it won't work at all. Line 94: only matching"Error" ++ _misses other OpenSSL failure modes (e.g., exit codes, stderr-only output, or different error prefixes).Consider using
os:find_executable("openssl")for a more portable check, and verifying the generated cert files exist (which you already do at lines 99-100) as the definitive success signal rather than parsing stderr.Suggested improvement
- case os:cmd("which openssl") of - [] -> - ct:fail(openssl_not_available); - _ -> - ok - end, + case os:find_executable("openssl") of + false -> + ct:fail(openssl_not_available); + _ -> + ok + end,
518-576: Dead helper functions.
get_server_port/1(hardcoded return),get_ranch_port/1(unused),find_port_in_listeners/1(only called byget_ranch_port), andwait_for_client_connection/2(placeholder) are all unreferenced dead code. Remove them or mark with a TODO if they're intended for future use.apps/mumble_protocol/test/mock_transport.erl (1)
87-88:gen_server:calltimeout matches the recv timeout — potential premature timeout.
gen_server:call(Pid, {recv, Length, Timeout}, Timeout)uses the sameTimeoutfor both the gen_server call envelope and the internal recv logic. If the internalreceive(lines 180-185) consumes most of the timeout, thegen_server:callwrapper may also timeout, resulting in a confusing{error, timeout}from gen_server rather than the clean{error, timeout}reply. Consider adding a small buffer to the call timeout (e.g.,Timeout + 500) or usinginfinityfor the call timeout since the internal receive already handles the deadline.
| authenticate(#{message_type := 'Authenticate', username := Username}, State) -> | ||
| %% MVP: Accept all users unconditionally | ||
| {ok, SessionId} = erlmur_user_manager:register_user(self(), Username), | ||
| UserInfo = #{session_id => SessionId, username => Username}, | ||
| logger:info("User ~s authenticated with session ~p", [Username, SessionId]), | ||
| {ok, UserInfo, State#state{session_id = SessionId, username = Username}}; | ||
| authenticate(Msg, State) -> | ||
| logger:warning("Invalid authenticate message: ~p", [Msg]), | ||
| {error, invalid_auth, State}. |
There was a problem hiding this comment.
Unhandled error from erlmur_user_manager:register_user/2 will crash the connection.
Line 28 uses {ok, SessionId} = erlmur_user_manager:register_user(self(), Username). If register_user returns {error, _} (e.g., duplicate user, manager down), the match fails and the process crashes with a badmatch. Handle the error case gracefully.
♻️ Proposed fix
authenticate(#{message_type := 'Authenticate', username := Username}, State) ->
- %% MVP: Accept all users unconditionally
- {ok, SessionId} = erlmur_user_manager:register_user(self(), Username),
- UserInfo = #{session_id => SessionId, username => Username},
- logger:info("User ~s authenticated with session ~p", [Username, SessionId]),
- {ok, UserInfo, State#state{session_id = SessionId, username = Username}};
+ %% MVP: Accept all users unconditionally
+ case erlmur_user_manager:register_user(self(), Username) of
+ {ok, SessionId} ->
+ UserInfo = #{session_id => SessionId, username => Username},
+ logger:info("User ~s authenticated with session ~p", [Username, SessionId]),
+ {ok, UserInfo, State#state{session_id = SessionId, username = Username}};
+ {error, Reason} ->
+ logger:error("Failed to register user ~s: ~p", [Username, Reason]),
+ {error, registration_failed, State}
+ end;🤖 Prompt for AI Agents
In `@apps/erlmur/src/erlmur_server_handler.erl` around lines 26 - 34, The
authenticate/2 function currently does a destructive match on the call to
erlmur_user_manager:register_user(self(), Username) which will crash on
{error,_}; update authenticate/2 to handle both successful and error tuples from
erlmur_user_manager:register_user/2 instead of using a direct {ok, SessionId} =
... match: on {ok, SessionId} proceed to build UserInfo, log with logger:info
and return {ok, UserInfo, State#state{session_id = SessionId, username =
Username}}; on {error, Reason} log a warning or error with the Reason (using
logger:warning/2 or logger:error/2) and return a non-crashing error tuple such
as {error, {register_failed, Reason}, State} (or {error, invalid_auth, State} if
you prefer), ensuring you reference the authenticate/2 function,
erlmur_user_manager:register_user/2, SessionId, UserInfo and State#state when
making the change.
| -record(stats, | ||
| {server_ping = #ping{}, | ||
| client_ping = #ping{}, | ||
| udp_packets = 0 :: non_neg_integer(), | ||
| tcp_packets = 0 :: non_neg_integer(), | ||
| udp_ping_avg = 0 :: non_neg_integer(), | ||
| udp_ping_var = 0 :: non_neg_integer(), | ||
| tcp_ping_avg = 0 :: non_neg_integer(), | ||
| tcp_ping_var = 0 :: non_neg_integer(), | ||
| onlinesecs = 0 :: non_neg_integer(), | ||
| idlesecs = 0 :: non_neg_integer(), | ||
| from_client_tcp_packets = 0 :: non_neg_integer(), | ||
| from_client_udp_packets = 0 :: non_neg_integer(), | ||
| from_client_tcp_bytes = 0 :: non_neg_integer(), | ||
| from_client_udp_bytes = 0 :: non_neg_integer()}). |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Mumble protocol ping statistics fields type specification
💡 Result:
In the Mumble TCP control protocol (protobuf), the Ping message carries the client/server ping statistics with the following field numbers and protobuf types:
| Field # | Name | Protobuf type | Meaning |
|---|---|---|---|
| 1 | timestamp |
uint64 |
Client timestamp (opaque; server echoes same value back) |
| 2 | good |
uint32 |
Count of “good” packets received |
| 3 | late |
uint32 |
Count of “late” packets received |
| 4 | lost |
uint32 |
Count of packets never received |
| 5 | resync |
uint32 |
Count of nonce resyncs |
| 6 | udp_packets |
uint32 |
Total UDP packets received |
| 7 | tcp_packets |
uint32 |
Total TCP packets received |
| 8 | udp_ping_avg |
float |
UDP ping average |
| 9 | udp_ping_var |
float |
UDP ping variance |
| 10 | tcp_ping_avg |
float |
TCP ping average |
| 11 | tcp_ping_var |
float |
TCP ping variance |
Notes:
- It’s defined as
message PinginMumble.proto; all fields areoptionalin the proto2 schema, and the server replies echoingtimestampand providing its own stats. [1] - The same field numbering/types appear in generated bindings (e.g., Go), confirming the wire-format types (
varintfor the integers;fixed32for the floats). [2]
Sources: [1] [2]
🏁 Script executed:
# Search for how these ping fields are used in the codebase
rg -i "udp_ping_avg|udp_ping_var|tcp_ping_avg|tcp_ping_var" --type erlangRepository: freke/erlmur
Length of output: 2376
🏁 Script executed:
# Also check if there are any .proto files or documentation about the protocol
find . -type f \( -name "*.proto" -o -name "*mumble*" -o -name "*protocol*" \) | head -20Repository: freke/erlmur
Length of output: 1004
🏁 Script executed:
# Check the Mumble.proto file to confirm field types
cat apps/mumble_protocol/proto/Mumble.proto | grep -A 20 "message Ping"Repository: freke/erlmur
Length of output: 778
🏁 Script executed:
# Check mumble_msg.erl to see how these float values are decoded/assigned
sed -n '1,50p' apps/mumble_protocol/src/mumble_msg.erlRepository: freke/erlmur
Length of output: 1365
🏁 Script executed:
# Look for the section handling Ping message decoding, particularly around the float fields
rg -A 5 -B 5 "udp_ping_avg|UdpAvg" apps/mumble_protocol/src/mumble_msg.erl | head -60Repository: freke/erlmur
Length of output: 1843
Change ping average/variance fields from non_neg_integer() to float
The Mumble protocol specification (Mumble.proto in the repo and official spec) defines udp_ping_avg, udp_ping_var, tcp_ping_avg, and tcp_ping_var as float type in the Ping message. The current type annotation of non_neg_integer() contradicts the protocol definition and will cause type mismatches when the protobuf decoder (gpb) produces float values.
🤖 Prompt for AI Agents
In `@apps/mumble_protocol/include/mumble_protocol.hrl` around lines 42 - 56, The
record stats currently types udp_ping_avg, udp_ping_var, tcp_ping_avg and
tcp_ping_var as non_neg_integer(), which conflicts with the protobuf Ping
definition that emits floats; update the stats record (record name: stats) to
declare those four fields as float() and set their defaults to 0.0
(udp_ping_avg, udp_ping_var, tcp_ping_avg, tcp_ping_var) so the types match
gpb-decoded values and avoid runtime/type-mismatch issues.
| -callback authenticate(AuthMsg :: map(), State :: any()) -> | ||
| {ok, UserInfo :: any(), NewState :: any()} | {error, Reason :: any()}. |
There was a problem hiding this comment.
authenticate/2 callback spec doesn't include a State return on error — mismatch with implementation.
The spec declares {error, Reason :: any()} (2-tuple), but erlmur_server_handler:authenticate/2 returns {error, invalid_auth, State} (3-tuple). This means either:
- The caller cannot recover the handler state after a failed auth, or
- The implementation violates the behaviour contract (Dialyzer should catch this).
If the state needs to be preserved on error, update the callback spec:
♻️ Proposed fix
-callback authenticate(AuthMsg :: map(), State :: any()) ->
- {ok, UserInfo :: any(), NewState :: any()} | {error, Reason :: any()}.
+ {ok, UserInfo :: any(), NewState :: any()} | {error, Reason :: any(), NewState :: any()}.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| -callback authenticate(AuthMsg :: map(), State :: any()) -> | |
| {ok, UserInfo :: any(), NewState :: any()} | {error, Reason :: any()}. | |
| -callback authenticate(AuthMsg :: map(), State :: any()) -> | |
| {ok, UserInfo :: any(), NewState :: any()} | {error, Reason :: any(), NewState :: any()}. |
🤖 Prompt for AI Agents
In `@apps/mumble_protocol/src/mumble_server_behaviour.erl` around lines 61 - 62,
The callback spec for authenticate/2 is incorrect: it currently lists an error
return as {error, Reason} but the implementation
erlmur_server_handler:authenticate/2 returns {error, invalid_auth, State};
update the behaviour spec to allow a 3-tuple error that includes the preserved
State (e.g. {error, Reason :: any(), State :: any()}) so the contract matches
the implementation and callers can recover state after failed auth.
| handle_cast({inject_message, Data}, State = #{owner := Owner, opts := Opts}) -> | ||
| NewQueue = case State of | ||
| #{message_queue := Queue} -> Queue ++ [Data]; | ||
| _ -> [Data] | ||
| end, | ||
|
|
||
| %% Check if we should notify the owner (active mode) | ||
| case lists:keyfind(active, 1, Opts) of | ||
| {active, true} when Owner =/= undefined -> | ||
| Owner ! {ssl, self(), Data}; | ||
| {active, once} when Owner =/= undefined -> | ||
| Owner ! {ssl, self(), Data}; | ||
| _ -> | ||
| ok | ||
| end, | ||
|
|
||
| {noreply, State#{message_queue := NewQueue}}; |
There was a problem hiding this comment.
Active-mode messages are enqueued but never dequeued — stale data in queue.
When inject_message arrives and active mode is enabled, the data is both sent to the owner via Owner ! {ssl, self(), Data} (line 232/234) and appended to message_queue (line 225). However, since it was already delivered, a subsequent recv/3 call will return the same data again from the queue. In active mode, the message should not be enqueued (or should be dequeued upon active delivery).
Suggested fix
handle_cast({inject_message, Data}, State = #{owner := Owner, opts := Opts}) ->
- NewQueue = case State of
- #{message_queue := Queue} -> Queue ++ [Data];
- _ -> [Data]
- end,
-
%% Check if we should notify the owner (active mode)
case lists:keyfind(active, 1, Opts) of
{active, true} when Owner =/= undefined ->
- Owner ! {ssl, self(), Data};
+ Owner ! {ssl, self(), Data},
+ {noreply, State};
{active, once} when Owner =/= undefined ->
- Owner ! {ssl, self(), Data};
+ Owner ! {ssl, self(), Data},
+ {noreply, State};
_ ->
- ok
- end,
-
- {noreply, State#{message_queue := NewQueue}};
+ NewQueue = maps:get(message_queue, State, []) ++ [Data],
+ {noreply, State#{message_queue := NewQueue}}
+ end;🤖 Prompt for AI Agents
In `@apps/mumble_protocol/test/mock_transport.erl` around lines 223 - 239,
handle_cast/2's inject_message currently both sends the message to Owner (active
mode) and appends it to message_queue, causing recv/3 to return duplicates;
change the logic in handle_cast({inject_message, Data}, State = #{owner :=
Owner, opts := Opts}) so that when Opts contains {active, true} or {active,
once} and Owner =/= undefined you do NOT append Data to message_queue (i.e., set
NewQueue = Queue unchanged or skip insertion), and only enqueue Data when not in
active mode; ensure State#{message_queue := NewQueue} still updates correctly
and that the active-once semantics (if any) are preserved for subsequent
deliveries.
| mumble_client_api_test_() -> | ||
| [ | ||
| {"client start with invalid certs", fun client_start_invalid_certs/0}, | ||
| {"client invalid ref handling", fun client_invalid_ref/0} | ||
| ]. |
There was a problem hiding this comment.
Five test functions are defined but never executed.
The test generator mumble_client_api_test_/0 only includes client_start_invalid_certs and client_invalid_ref. The remaining functions — client_start_no_certs, client_send_message, client_send_voice, client_get_state, and client_stop (Lines 19, 31, 42, 53, 64) — are dead code that EUnit will never discover or run.
Either add them to the test generator or remove them to avoid false coverage confidence.
Proposed fix — include all tests
mumble_client_api_test_() ->
[
+ {"client start without certs", fun client_start_no_certs/0},
{"client start with invalid certs", fun client_start_invalid_certs/0},
- {"client invalid ref handling", fun client_invalid_ref/0}
+ {"client send message", fun client_send_message/0},
+ {"client send voice", fun client_send_voice/0},
+ {"client get state", fun client_get_state/0},
+ {"client stop and double-stop", fun client_stop/0},
+ {"client invalid ref handling", fun client_invalid_ref/0}
].📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| mumble_client_api_test_() -> | |
| [ | |
| {"client start with invalid certs", fun client_start_invalid_certs/0}, | |
| {"client invalid ref handling", fun client_invalid_ref/0} | |
| ]. | |
| mumble_client_api_test_() -> | |
| [ | |
| {"client start without certs", fun client_start_no_certs/0}, | |
| {"client start with invalid certs", fun client_start_invalid_certs/0}, | |
| {"client send message", fun client_send_message/0}, | |
| {"client send voice", fun client_send_voice/0}, | |
| {"client get state", fun client_get_state/0}, | |
| {"client stop and double-stop", fun client_stop/0}, | |
| {"client invalid ref handling", fun client_invalid_ref/0} | |
| ]. |
🤖 Prompt for AI Agents
In `@apps/mumble_protocol/test/mumble_client_api_tests.erl` around lines 7 - 11,
The test generator mumble_client_api_test_/0 currently only lists
client_start_invalid_certs/0 and client_invalid_ref/0 while the functions
client_start_no_certs/0, client_send_message/0, client_send_voice/0,
client_get_state/0, and client_stop/0 are defined but never executed; update
mumble_client_api_test_/0 to include all those test function references
(client_start_no_certs, client_send_message, client_send_voice,
client_get_state, client_stop) so EUnit will run them, or alternatively remove
the unused function definitions if they are intentionally deprecated.
1 participant
ocb128 keep track of stats
feat: implement mumble.erl API with comprehensive documentation
Add complete Mumble protocol implementation including:
API Implementation:
Connection Handling:
Protocol Support:
Behaviours:
Documentation:
Testing:
All 128 EUnit tests, 34 CT tests, and 5 property tests pass. Dialyzer type checking passes with no warnings.
Refs: erlmur architecture v1.0
refactor: split mumble.erl into specialized modules
test: add comprehensive connection logic unit tests
Fix eunit tests
Add mock transport support for server tests
Add mock transport support for server tests
Fixed build warnings
Fixed ping
Summary by CodeRabbit