Security, correctness and dead code fixes by drozdzynski · Pull Request #13 · nerdslabs/storex

drozdzynski · 2026-09-07T09:31:54Z

Follow-up to #10 and #11, working through the items that were deliberately left out of those two PRs.

Every item below was verified before being fixed — measured, or reproduced with a throwaway test — and each fix has a regression test that was confirmed to fail against the old code.

Security

Any client could mutate any other session's store. Storex.Socket.message_handle/2 resolved the store with message.session, the session id the client put in the frame, and never compared it against state.session. Reproduced: a socket owning "attacker-session" sent a mutation naming "victim-session" and the victim's counter went 0 → 1, with the diff pushed back to the attacker. The join clause two lines above already used state.session.

Untrusted deserialisation on cowboy binary frames. :erlang.binary_to_term/1 with no [:safe], on raw socket bytes. A hand-built 25-byte ATOM_UTF8_EXT frame adds an atom, so one open socket can grow the atom table as fast as it can write. It also skipped the Storex.Message.cast/1 allowlist entirely. Nothing used the path — the client only ever sends JSON.stringify output, no test sent a binary frame, the README never documented it — so binary frames are now refused with 1003.

While testing that: Storex.Handler.Plug had no :binary clause at all, so any binary frame raised FunctionClauseError and closed the connection with 1011, on the transport the README recommends.

The SSR/HTTP path skipped two of the three module-resolution guards. Storex.HTTP kept its own copy of the store-name resolution with only Module.safe_concat/1 — no Code.ensure_compiled/1, no behaviour check — then called init/2 on whatever came back. GET /storex?store=Any.Module&params=%7B%7D therefore reached init/2 on any loaded module whose name resolved. The defect is the duplication rather than the missing lines, so both transports now share Storex.Store.resolve/1.

Resource exhaustion

One permanent atom per session-store pair. Store processes were named :"#{session}_#{store}", and session ids are unique per connection, so one reconnect was one new atom. They now register through a Registry keyed by the {session, store} tuple, which keeps the duplicate-start guard the name was accidentally providing. Regression test asserts a zero atom delta over 200 joins; against the old code it measures +291.

Crashes reachable from a client

A well-formed error frame passed Storex.Message.cast/1 and then had no clause in message_handle/2. Those frames only travel server to client, so the shape is out of the allowlist.
A mutation for a store the session had not joined piped :undefined straight into GenServer.call/2.

Performance

Storex.Registry reads now run in the calling process against the :protected table instead of a GenServer.call. Every mutation performs at least one lookup, so that process was a global serialisation point. Measured at 500 lookups per reader:

readers	GenServer	direct
1	1.7ms	0.6ms
16	24.8ms	10.5ms
64	98.3ms	16.2ms
256	335.8ms	56.2ms

The round-trip grows linearly with load and the direct path does not, which is the serialisation showing rather than :ets.match being slow.

Correctness and dead code

FunctionClauseError raised inside a matching mutation was reported to the client as No mutation matching ... with the real stacktrace discarded. Only the store's own mutation/5 failing to match produces that error now.
Store state on join was read with :sys.get_state/1, a debug function, on a production path, reaching into the generated Server's internal state shape from another file. The process answers for itself now.
The two handlers closed a malformed payload differently: Storex.Handler.Plug passed the reason as the process exit reason rather than the close payload, so the client saw a bare 1007 and the connection exited abnormally every time.
Removed: Storex.Registry.session_pid/1 (no callers, and its :ets.match/2 pattern was a 4-tuple against 5-tuple records, so it never matched), Storex.Handler.Cowboy.websocket_init/3 (cowboy 1.x signature), and the unreachable {:error, _} branch in Storex.PG.broadcast/1.
Removed: import Supervisor.Spec, warn: false from Storex.start/2 (deprecated since Elixir 1.5, unused, and the warn: false was hiding it) and registry: Storex.Registry.ETS from config/config.exs (that module went away in 0.4.0 and nothing read the key).

Formatting

mix format --check-formatted failed on the @callback terminate/3 line in lib/storex/store.ex, and had for as long as the file has looked like that. It is formatted now, so the check passes on the whole project and CI runs it.

The check is a separate job on one pinned Elixir rather than a step in the test matrix — the formatter's output can change between releases, and a step in the matrix would fail all thirteen rows the first time it does.

Breaking changes

Marked **[BREAKING]** in the changelog:

A FunctionClauseError from inside a matching mutation now propagates instead of coming back as an error message.
Storex.Handler.Cowboy no longer accepts binary frames.
Storex.mutate/3 and Storex.mutate/4 return :ok. They used to return the internal broadcast envelope once per node — [broadcast: {:mutate, "Store", "reload", []}].

Test coverage

A coverage run turned up behaviour with no tests at all, and reading the suite turned up more that coverage cannot see — code that runs during teardown with nothing asserting on it.

terminate/3 had never executed. No fixture implemented it, so the whole optional-callback path was untested.
The registry dropping a row when a store process dies, and a connection closing stopping the session's stores. Both ran during teardown, neither was asserted. The :DOWN handler is one of the match patterns that has to stay in step with the shape of the table, so a silent regression there leaks processes.
Ping and pong. The client sends one every 30 seconds, which makes it the most frequently handled frame in production and the only one with no test.
Storex.mutate/3 reaching every session of a store. The existing tests used one session, so the fan-out — the point of the function — was never exercised.
Re-joining a store already joined in the session, and Storex.Diff comparing a struct against a plain map in both directions.

Writing the first of those found a defect: Storex.Store.__terminate__/4 gated the callback on function_exported?/3, which answers false for a module that is not loaded. Under the normal flow the store is loaded by then, so it never showed up, but whether a documented callback runs should not depend on what the code server happens to hold. It checks Code.ensure_loaded?/1 first now.

Coverage after: Storex.Diff and Storex.Socket at 100% (from 89.7% and 90.5%), Storex.Supervisor 95.2%, Storex.Store 89.3%. What is left uncovered is five defensive fallbacks — catch-all handle_info clauses, the terminate clause for a state with no session, and the malformed-params branch in Storex.HTTP.

Testing

115 non-browser tests pass, up from 54 on master. mix compile --warnings-as-errors and mix format --check-formatted are both clean.

The browser tests could not run locally — chromedriver is not installed in the environment this was written in, so all 21 fail there for that reason alone, on master too. Confirming them on CI is why this is a PR rather than a direct push. They pass: 119 tests, 0 failures on every matrix row.

Not included

The frontend lives in a sibling repo and is untouched here. The most valuable finding there: patch() mutates state in place and returns the same object reference, so React, Vue and Svelte identity checks see no change and do not re-render. Also a per-commit() leak in the pending-request map, promises that never settle when the socket drops, and a reconnect loop with no backoff.

`Storex.Store.__mutation__/6` rescued every `FunctionClauseError` raised under the store's `mutation/5` call and reported it as "No mutation matching ...". That message is only correct when the store itself has no clause for the incoming name and data. An error raised deeper inside a mutation that did match was reported the same way, with the real stacktrace discarded. The two cases are distinguishable: a `FunctionClauseError` carries the module, function and arity that failed to match. Only `{store, :mutation, 5}` means "no such mutation"; anything else is reraised with its original stacktrace. This changes observable behaviour. A crash inside a matching mutation used to reach the client as an error message; it now propagates, stops the store process and takes the socket with it.

`Storex.Supervisor.name/2` built the store process name with `String.to_atom("#{session}_#{store}")`. Session ids come from `Nanoid.generate/0` and are unique per connection, so every join created a new atom, and atoms are never garbage collected. One reconnect is one new atom, which makes this reachable by ordinary traffic rather than abuse: a long-running node with enough connection churn exhausts the atom table and the VM dies. Nothing ever looked a store up by that name. `Storex.Registry` maps `{store, session}` to the pid and is what every operation actually uses. The name was only load-bearing in that registering it twice fails, which keeps a second process for the same `{session, store}` from starting. A `Registry` keyed by the `{session, store}` tuple keeps that guarantee (`start_link` still returns `{:error, {:already_started, pid}}`) and creates no atoms.

`Storex.Socket.message_handle/2` looked the store up by `message.session` — the session id the client put in the frame — and never compared it against `state.session`, the id the handler generated for this connection. Any client could therefore mutate any other session's store by naming its session id, and received the resulting diff (and any `{:reply, message, state}` payload) back on its own socket. The `join` clause a few lines above already used `state.session`. This brings the mutation clause in line with it. The field stays in the frame and is still echoed in the response, but it no longer resolves anything. Deliberate cross-session mutation is what `Storex.mutate/3` and `Storex.mutate/4` are for. Fixing that alone would leave a reachable crash: a mutation for a store the session has not joined makes `Storex.Registry.get_store_pid/2` return `:undefined`, which `Storex.Supervisor.mutate_store/4` piped straight into `GenServer.call/2`, exiting the connection process. It was reachable before this change by naming a session that does not exist, so it is fixed here too — the lookup miss now returns an error the client receives as a normal error frame. `get_store_state/2` has the same shape but is left alone: its only caller runs inside the success branch of the join `with`, right after `add_store` registered the process, so the miss is unreachable there.

`Storex.Handler.Cowboy` decoded `:binary` frames with `:erlang.binary_to_term/1`. Two problems in one line. It creates atoms out of bytes the client controls. A hand-built 25-byte external term format frame is enough to add one, so a single open socket can grow the atom table as fast as it can write, with no connection churn needed. It is also the only unauthenticated `binary_to_term` in the library. It also bypassed `Storex.Message.cast/1`, the allowlist every other entry point runs its payload through, and handed a raw term straight to `Storex.Socket.message_handle/2`. The `rescue ArgumentError` only covered a malformed encoding; a well-formed term of an unexpected shape raised `FunctionClauseError` and closed the connection with a 1011. Nothing uses binary frames. The client only ever sends `JSON.stringify` output, no test sends one (the `send_binary_frame/3` helper had no callers until now), and the README never documented the path. Keeping it would mean maintaining a second set of `cast/1` clauses for atom-keyed maps, for no caller. So the frame is refused with a 1003 instead. `Storex.Handler.Plug` had no `:binary` clause at all, which made any binary frame raise `FunctionClauseError` and kill the connection with a 1011 on the default transport. It now refuses the frame the same way.

A store name arrives as a string from the client, so resolving it to a module needs three checks together: `Module.safe_concat/1` so no new atom is created, `Code.ensure_compiled/1` so the name maps to a real module, and a check that the module declares the `Storex.Store` behaviour. `Storex.Socket` did all three. `Storex.HTTP` kept its own copy that had drifted down to `Module.safe_concat/1` alone, and then called `apply(module, :init, ["SSR", params])` on whatever came back. So `GET /storex?store=Any.Module&params=%7B%7D` invoked `init/2` on any loaded module whose name resolved and serialised the return value to the caller, and a module whose `init/2` returned an unexpected shape raised, which made the endpoint a probe for which modules exist. The defect is the duplication, not the missing lines, so both callers now share `Storex.Store.resolve/1` and the copies are gone. `Storex.HTTP.get_state/2` had the same problem on a smaller scale: its own `apply/3` plus a three clause `result/1` where `Storex.Store.__init__/3` already exists. It now uses the shared dispatcher, so SSR accepts the same return values as the WebSocket path and reports the same error for anything else, instead of a bare `FunctionClauseError`. The SSR error message interpolated the store name with `inspect/1`, rendering it double quoted. It now matches the WebSocket wording.

Three unrelated pieces of code that were either unreachable or wrong. `Storex.Message.cast/1` accepted an `error` frame, but `Storex.Socket.message_handle/2` has no clause for one, so a client could kill its connection process with a well-formed frame. The direction is server to client, and those frames are built as plain maps in `Storex.Socket` rather than through the struct, so nothing produced or consumed the shape. It is out of the allowlist, which sends the frame down the same path as any other unknown type: closed with 1007. `Storex.Registry.session_pid/1` had no callers anywhere, and could not have worked if it did: it matched `{:_, session, :_, :"$1"}`, a 4-tuple, against a table of 5-tuples, with `session` in the `store_pid` position. Its `:DOWN` handler also replaced the `%{}` state with the atom `:ok`, which is harmless only for as long as nothing reads it. `Storex.Handler.Cowboy.websocket_init/3` is the cowboy 1.x callback signature. Cowboy 2.x calls `websocket_init/1`, and `init/2` already returns the real state, so it never ran. Testing the first of those turned up a fourth: the two handlers closed a malformed payload differently. `Storex.Handler.Plug` passed the reason as the process exit reason rather than the close payload, so the client saw a bare 1007 while cowboy sent 1007 plus the reason, and the connection process exited abnormally every time. It now uses the same shape as the rest of its close responses.

`Storex.Supervisor.get_store_state/2` read a store's state with `:sys.get_state/1` and then reached into the result with `Map.get(:state)`. That is a debug function, it runs on the join path in production, it uses the `:sys` timeout rather than the caller's, and it couples the supervisor to the internal state shape of the `Server` module generated in `Storex.Store` — a module in a different file that cannot change its representation without breaking this caller. The generated `Server` now answers `handle_call(:get_state, ...)` for itself, and the supervisor calls that. The return type becomes `{:ok, state} | {:error, reason}`, matching `mutate_store/4`. Besides being one contract instead of two in the same module, it closes the race left open when `mutate_store/4` was guarded: the store process can be gone between `add_store/4` returning and the join reading the state, since it is `restart: :transient`. That used to be `:sys.get_state(:undefined)`, which exits and takes the connection with it. The join now answers with an error frame.

The registry table is created `:protected`, which already lets any process read it, yet every accessor went through `GenServer.call` to a single process that then ran the `:ets.match` itself. Every mutation performs at least one lookup, so that process was a global serialisation point for the whole library. Reads now run in the caller. Writes and the `:DOWN` cleanup stay in the process, which still owns the table. Measured at 500 lookups per reader, comparing the round-trip against reading directly: readers GenServer direct 1 1.7ms 0.6ms 4 6.7ms 1.8ms 16 24.8ms 10.5ms 64 98.3ms 16.2ms 256 335.8ms 56.2ms The GenServer path grows linearly with load while the direct path does not, which is the serialisation showing up rather than `:ets.match` being slow. Also removes the unreachable `{:error, _}` branch in `Storex.PG.broadcast/1`. `:pg.get_members/2` always returns a list; the error tuple was `:pg2`'s contract and `:pg2` went away in this release. That branch was the reason `broadcast/1` used a comprehension, and `send/2` returns the message it sent, so `Storex.mutate/3` and `Storex.mutate/4` were handing callers the internal broadcast envelope once per node: Storex.mutate("Some.Store", "reload", ["x"]) #=> [broadcast: {:mutate, "Some.Store", "reload", ["x"]}] They return `:ok` now.

`Storex.start/2` carried `import Supervisor.Spec, warn: false`. `Supervisor.Spec` has been deprecated since Elixir 1.5 and nothing in the function used it; the `warn: false` is what kept that quiet. `config/config.exs` still set `registry: Storex.Registry.ETS`. That module was removed in 0.4.0 and nothing reads the key. `mix format --check-formatted` failed on the `@callback terminate/3` line in `lib/storex/store.ex`, and had for as long as the file has looked like that. With it formatted the check passes on the whole project, so CI can run it. The check is a separate job on one pinned Elixir rather than a step in the test matrix. The formatter's output can change between releases, and a step in the matrix would fail all thirteen rows the first time it does. `.formatter.exs` declares no `import_deps`, so the job needs no dependencies.

Coverage found four gaps, and reading the tests found three more that coverage cannot see because the code runs during teardown without anything asserting on it. - `terminate/3` had never been executed. No fixture implemented it, so the whole optional-callback path was untested. There is a fixture for it now, and tests that it runs on `remove_store/2` with the state as of the last mutation. - The registry drops a row when the store process dies. That ran during teardown but nothing asserted it, which matters because the `:DOWN` handler is one of the match patterns that has to stay in step with the shape of the table. - Closing a connection stops the session's stores. Same situation. - Ping and pong. The client sends one every 30 seconds, making it the most frequently handled frame in production and the only one with no test. - `Storex.mutate/3` reaching every session of a store. The existing tests used a single session, so the fan-out — the entire point of the function — was never exercised. Also covers a session on a different store not being reached. - Joining a store that is already joined in the session returns the existing key and starts nothing. - `Storex.Diff` comparing a struct against a plain map, in both directions. The existing struct test compared two structs. Writing the first of those turned up a real defect. `Storex.Store.__terminate__/4` gated the callback on `function_exported?/3`, which answers `false` for a module that is not loaded. Under the normal flow the store is loaded by the time it terminates, because `init/2` was applied on it, so this never showed up — but whether a documented callback runs should not depend on what the code server happens to be holding. It checks `Code.ensure_loaded?/1` first now. Every test here was confirmed to fail against the unfixed code: seven deliberate breakages produce twelve red tests, and none on the code as it stands.

drozdzynski added 10 commits September 7, 2026 06:50

drozdzynski merged commit 708a766 into master Sep 11, 2026
14 checks passed

drozdzynski deleted the chore/improvements branch September 11, 2026 12:20

Navigation Menu

Uh oh!

Security, correctness and dead code fixes - #13

Security, correctness and dead code fixes#13
drozdzynski merged 10 commits into
masterfrom
chore/improvements

drozdzynski commented Sep 7, 2026 •

edited

Loading

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

drozdzynski commented Sep 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Security

Resource exhaustion

Crashes reachable from a client

Performance

Correctness and dead code

Formatting

Breaking changes

Test coverage

Testing

Not included

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

drozdzynski commented Sep 7, 2026 •

edited

Loading