fix(server): resolve session names via Registry to prevent atom-exhaustion DoS - #188
Conversation
📝 WalkthroughProblemSession processes were previously named by converting client-controlled SolutionChanged session process naming to avoid atom creation by using Rationale / ImpactThis preserves deterministic, bounded registry naming for internal infrastructure while ensuring untrusted session IDs never get converted into atoms, eliminating the atom-table exhaustion vector. The atom-based adapter naming hook is retained only for custom Walkthrough
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches✨ Simplify code
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]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/anubis/server/registry.ex (1)
34-40:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winP2: Update stale
session_name/2callback docs (current default is no longer atom-based)Line 36 says “default returns a plain atom,” but current code returns
:viatuples (Lines 58 and 92). Line 38’s no-op guidance is also too broad now.✏️ Suggested doc patch
- Returns the GenServer name for a session. Override this to return a `:via` tuple - (e.g. `{:via, Horde.Registry, {name, session_id}}`) when using a distributed registry - that auto-registers processes on `start_link`. The default returns a plain atom. - - When a `:via` tuple is returned, `register_session/3` should be a no-op since - registration happens automatically on process start. + Returns the GenServer name for a session. Override this to return an adapter-specific + name (for example a distributed `:via` tuple) when your registry auto-registers + processes on `start_link`. + + The default resolves to a `:via` tuple through + `Anubis.Server.Session.NameRegistry`. Adapters remain responsible for + `register_session/3` and `lookup_session/2` unless their own registry layer + fully covers that contract.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ad48e64e-4225-4516-b2b8-e9dc4a17ff75
📒 Files selected for processing (6)
lib/anubis/application.exlib/anubis/server/registry.exlib/anubis/server/registry/local.exlib/anubis/server/session/name_registry.extest/anubis/server/registry/session_name_test.exstest/anubis/server/transport/streamable_http/plug_test.exs
💤 Files with no reviewable changes (1)
- lib/anubis/server/registry/local.ex
`Anubis.Server.Registry.resolve_session_name/3` fell back to
`session_name_from_registry_name/2`, which built a fresh atom per
session via `:"#{registry_name}.session.#{session_id}"`. Session ids
come from the client-controlled `mcp-session-id` header and atoms are
never garbage collected, so a client sending many distinct session ids
could grow the atom table without bound and crash the VM (default limit
1,048,576 atoms).
The shipped HTTP adapters hit this path: `Registry.Local` implemented
`session_name/2` with the same dynamic-atom construction, and
`Registry.PG` does not implement the optional callback at all, so it
took the dynamic-atom fallback. Only STDIO (single, fixed "stdio"
session id) was unaffected.
Route the default session naming through an Elixir `Registry` (a `:via`
tuple keyed by the session-id string) instead of minting atoms. The
naming `Registry` is started per server in the HTTP supervision tree and
its name is derived from the compile-time bounded `registry_name`, so no
new atoms are created per session. `{:via, Registry, ...}` also
preserves the existing `{:error, {:already_started, pid}}` semantics
that the transports rely on for concurrent requests to the same session.
- Add `Registry.naming_registry_name/1` and start the naming `Registry`
in `build_http_children/8`.
- Drop `Registry.Local.session_name/2` so it uses the safe default.
- Document `Registry.session_name/2` as test-only (still atom-based).
- Add a regression test asserting `:erlang.system_info(:atom_count)`
stays stable across 50,000 distinct session ids.
4413dce to
223fb63
Compare
neilberkman
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7e4ee212-c7f4-4b7b-9819-f4424f3f21a6
📒 Files selected for processing (5)
lib/anubis/server/registry.exlib/anubis/server/registry/local.exlib/anubis/server/supervisor.extest/anubis/server/registry_test.exstest/anubis/server/transport/streamable_http/plug_test.exs
💤 Files with no reviewable changes (1)
- lib/anubis/server/registry/local.ex
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/anubis/server/registry_test.exs (1)
60-60:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winP3: Test name says concurrent, but the flow is sequential 😄
Line 60’s title implies a race/concurrency assertion, but the body does a straightforward duplicate start check. Rename the test to match behavior so failures are easier to interpret.
Suggested diff
- test "concurrent starts for the same session id yield {:already_started, pid}", ctx do + test "second start for same session id yields {:already_started, pid}", ctx doAs per coding guidelines,
test/**/*.exsshould use descriptive test block names.Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: be52e2a5-4cfb-4f6d-9e7b-3b174073eadf
📒 Files selected for processing (1)
test/anubis/server/registry_test.exs
🚀 Want to release this? --- ## [1.7.0](v1.6.2...v1.7.0) (2026-07-16) ### Features * **streamable_http:** per-subscriber metadata and targeted sends ([#218](#218)) ([c606658](c606658)) ### Bug Fixes * Forward configured :headers on the DELETE session-teardown request (follow-up to [#180](#180)) ([#213](#213)) ([b32134a](b32134a)) * prevent "Server not initialized" race on first request ([#198](#198)) ([e84624c](e84624c)) * **server:** resolve session names via Registry to prevent atom-exhaustion DoS ([#188](#188)) ([17e4a6d](17e4a6d)) * **session:** trap_exit so terminate/2 runs on supervisor shutdown ([#209](#209)) ([6335cf4](6335cf4)) * **streamable_http:** don't close superseded SSE handler to prevent reconnect flap ([#215](#215)) ([a1e0ce6](a1e0ce6)) ### Continuous Integration * add new elixir versions ([3b636a8](3b636a8)) * add pr-quality workflow ([c0ca08f](c0ca08f)) * fix zig correct version for burrito ([6e410bd](6e410bd)) * use mlugg/setup-zig 0.15.2 in release-please auto build job ([2ed6187](2ed6187)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
…stion DoS (#188) ## Summary `Anubis.Server.Registry.resolve_session_name/3` named each session process by interpolating the session id into an atom. For HTTP transports the session id comes from the client-supplied `mcp-session-id` header, and atoms are never garbage collected, so a client sending many distinct session ids grows the global atom table without bound until the BEAM hits its atom limit (default ~1,048,576) and the node crashes with `system_limit`. That is a remote denial-of-service reachable in a default production deployment. This PR routes the default session naming through an Elixir `Registry` (`:via` tuple keyed by the session-id string) so the client-supplied id stays ordinary term data and no atom is minted per session. ## The vulnerability Two code paths built a fresh atom per session id: - The fallback in `resolve_session_name/3`, used when a registry adapter does not implement the optional `session_name/2` callback (`lib/anubis/server/registry.ex`): ```elixir defp session_name_from_registry_name(registry_name, session_id) when is_atom(registry_name) do :"#{registry_name}.session.#{session_id}" end ``` - The shipped `Registry.Local` adapter (`lib/anubis/server/registry/local.ex`), the default for HTTP transports, which implemented `session_name/2` the same way: ```elixir def session_name(registry_name, session_id), do: :"#{registry_name}.session.#{session_id}" ``` `Registry.PG` does not implement `session_name/2`, so it hits the atom-minting fallback. Both shipped HTTP-capable registries are therefore affected — no custom adapter required. STDIO is unaffected because it uses a single, fixed `"stdio"` session id. The name is used only as the `name:` passed to `start_session/2`; after start, lookups go through the adapter's `lookup_session/2` (ETS or `:pg`) by pid. So the atom only ever served as the process registration name, which makes it safe to replace with a `:via` name. ## The fix `resolve_session_name/3` now returns: ```elixir {:via, Registry, {naming_registry_name(registry_name), session_id}} ``` - A per-server Elixir `Registry` (`keys: :unique`) is started in the HTTP supervision tree (`build_http_children/8`). Its name is derived from the compile-time bounded `registry_name` via the new `Registry.naming_registry_name/1`, so it does not itself mint per-session atoms. - The client-supplied `session_id` is the registry key (binary term data), never converted to an atom. - `:via` tuples are drop-in replacements for atom names in `GenServer.start_link/3` / `start_session/2`, and they preserve the existing `{:error, {:already_started, pid}}` semantics that `plug.ex` and `sse.ex` rely on for concurrent requests to the same session. - The optional `session_name/2` callback is kept for adapters that want their own `:via` naming (e.g. Horde). Only the shipped atom-minting `Registry.Local.session_name/2` is removed, so it falls through to the safe default. Internal, compile-time bounded names (transports, supervisors, task stores) keep their atom naming — they are not influenced by client input. The public `Registry.session_name/2` helper still builds an atom and is now documented as test-only / trusted-id-only. ## Changes - `lib/anubis/server/registry.ex` — `resolve_session_name/3` fallback now returns a `:via` `Registry` tuple; added `naming_registry_name/1`; documented why session names must not be atoms and marked `session_name/2` as trusted-id-only. - `lib/anubis/server/supervisor.ex` — start the per-server naming `Registry` in `build_http_children/8`. - `lib/anubis/server/registry/local.ex` — removed the atom-minting `session_name/2`; uses the safe default. - `test/anubis/server/registry_test.exs` — regression test asserting `:erlang.system_info(:atom_count)` stays stable across 50,000 distinct session ids, plus round-trip tests that a process is reachable by its resolved `:via` name and that duplicate starts return `{:already_started, pid}`. - `test/anubis/server/transport/streamable_http/plug_test.exs` — start the naming `Registry` in the session-handling setup (mirrors production wiring). ## Verification ``` mix test # registry/transport/session suites: 63 tests, 0 failures mix format --check-formatted mix credo # no issues on changed files ``` The regression test fails against the previous atom-based naming and passes with the fix. --------- Co-authored-by: zoey <zoey.spessanha@zeetech.io>
🚀 Want to release this? --- ## [1.7.0](v1.6.2...v1.7.0) (2026-07-16) ### Features * **streamable_http:** per-subscriber metadata and targeted sends ([#218](#218)) ([d6cf7b1](d6cf7b1)) ### Bug Fixes * Forward configured :headers on the DELETE session-teardown request (follow-up to [#180](#180)) ([#213](#213)) ([4edd2c0](4edd2c0)) * prevent "Server not initialized" race on first request ([#198](#198)) ([6bb60f9](6bb60f9)) * **server:** resolve session names via Registry to prevent atom-exhaustion DoS ([#188](#188)) ([fdbc238](fdbc238)) * **session:** trap_exit so terminate/2 runs on supervisor shutdown ([#209](#209)) ([a224c00](a224c00)) * **streamable_http:** don't close superseded SSE handler to prevent reconnect flap ([#215](#215)) ([e1cc4a8](e1cc4a8)) ### Continuous Integration * add new elixir versions ([26dd267](26dd267)) * add pr-quality workflow ([d67eaa9](d67eaa9)) * fix zig correct version for burrito ([afec768](afec768)) * use mlugg/setup-zig 0.15.2 in release-please auto build job ([428cace](428cace)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
2 participants
Summary
Anubis.Server.Registry.resolve_session_name/3named each session process by interpolating the session id into an atom. For HTTP transports the session id comes from the client-suppliedmcp-session-idheader, and atoms are never garbage collected, so a client sending many distinct session ids grows the global atom table without bound until the BEAM hits its atom limit (default ~1,048,576) and the node crashes withsystem_limit. That is a remote denial-of-service reachable in a default production deployment.This PR routes the default session naming through an Elixir
Registry(:viatuple keyed by the session-id string) so the client-supplied id stays ordinary term data and no atom is minted per session.The vulnerability
Two code paths built a fresh atom per session id:
The fallback in
resolve_session_name/3, used when a registry adapter does not implement the optionalsession_name/2callback (lib/anubis/server/registry.ex):The shipped
Registry.Localadapter (lib/anubis/server/registry/local.ex), the default for HTTP transports, which implementedsession_name/2the same way:Registry.PGdoes not implementsession_name/2, so it hits the atom-minting fallback. Both shipped HTTP-capable registries are therefore affected — no custom adapter required. STDIO is unaffected because it uses a single, fixed"stdio"session id.The name is used only as the
name:passed tostart_session/2; after start, lookups go through the adapter'slookup_session/2(ETS or:pg) by pid. So the atom only ever served as the process registration name, which makes it safe to replace with a:vianame.The fix
resolve_session_name/3now returns:Registry(keys: :unique) is started in the HTTP supervision tree (build_http_children/8). Its name is derived from the compile-time boundedregistry_namevia the newRegistry.naming_registry_name/1, so it does not itself mint per-session atoms.session_idis the registry key (binary term data), never converted to an atom.:viatuples are drop-in replacements for atom names inGenServer.start_link/3/start_session/2, and they preserve the existing{:error, {:already_started, pid}}semantics thatplug.exandsse.exrely on for concurrent requests to the same session.session_name/2callback is kept for adapters that want their own:vianaming (e.g. Horde). Only the shipped atom-mintingRegistry.Local.session_name/2is removed, so it falls through to the safe default.Internal, compile-time bounded names (transports, supervisors, task stores) keep their atom naming — they are not influenced by client input. The public
Registry.session_name/2helper still builds an atom and is now documented as test-only / trusted-id-only.Changes
lib/anubis/server/registry.ex—resolve_session_name/3fallback now returns a:viaRegistrytuple; addednaming_registry_name/1; documented why session names must not be atoms and markedsession_name/2as trusted-id-only.lib/anubis/server/supervisor.ex— start the per-server namingRegistryinbuild_http_children/8.lib/anubis/server/registry/local.ex— removed the atom-mintingsession_name/2; uses the safe default.test/anubis/server/registry_test.exs— regression test asserting:erlang.system_info(:atom_count)stays stable across 50,000 distinct session ids, plus round-trip tests that a process is reachable by its resolved:vianame and that duplicate starts return{:already_started, pid}.test/anubis/server/transport/streamable_http/plug_test.exs— start the namingRegistryin the session-handling setup (mirrors production wiring).Verification
The regression test fails against the previous atom-based naming and passes with the fix.