Repository navigation
hub: five audit fixes — four of them a guard missing from its sibling - #3
Merged
Merged
Conversation
A21. /topic set had no membership or ban check. Commands are dispatched before any room gate (handleMsg), so a peer BANNED from a room still reached cmdTopic, and only +t was checked — meaning any connected identity could retitle any non-+t room it knew the name of, persisted to rooms.toml when registered and broadcast to every member. The view path a few lines above already required membership for +p; only the set path was missing it. Now membership (or server-op) is required, for the reason /history states outright: joining already cleared this room's ban, key and invite gates. A22. A command body escaped max_msg_body_bytes. handleMsg dispatches a slash command BEFORE the body-size check, so every command handler was receiving an argument string bounded only by the 8 KiB link frame. Bounded once in handleCommand so a new command cannot reintroduce it, and the topic — the part that reaches disk and every future joiner — carries its own max_topic_bytes as well. The default is 256, not 350, so a maximum-length topic still fits inside the "/topic <room> <text>" that sets it; a test asserts the two limits against the shipped defaults, because a limit that advertises more than it can accept is its own bug. A23. Peer eviction destroyed the mentions it exists to protect. A peer with queued mentions is by definition one not seen lately — that is what "away" means — so oldest-last-seen eviction selected exactly those rows, silently. max_known_peers strangers connecting in SEQUENCE was enough (MaxSessions bounds only concurrent links), which defeats the one guarantee the RRC fallback makes. Eviction now prefers a peer owed nothing, and logs when it cannot find one. A24. A member could not /who their own +p room: the check was `private && !serverOp`, missing the `&& !hasMember` its sibling in cmdTopic's view path has. +p still hides the room from outside. A25. trusted_identities / banned_identities were never validated. isServerOp compares against the hex of a full 16-byte identity hash, so a malformed entry is not a weaker rule but one that can never fire — and the operator's only evidence was a permanent, unexplained "not authorized". The likeliest cause is not a typo but pasting a DESTINATION hash where an IDENTITY hash belongs, which is the same length and also hex. Entries are now parsed with parseHexHash (audit A14's validator, which the config path never got) and anything dropped is logged with the field name. Also: §6.6 LINKIDENTIFY -> §6.7.6 in the two comments touched here. Each of the twelve new tests in security_group5_test.go was verified to FAIL against the pre-fix code by reverting that fix alone, and the positive cases (a member sets a topic, a server-op sets one without joining, eviction still evicts, a valid server-op still authorizes) are there because four of these fixes tighten an authorization check and the cheap way to get one wrong is to lock out the people it is for. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RRgC769HyZWx1YaGoosp9t
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A full security review of the hub. Five findings, each fixed with tests; every test was verified to fail against the pre-fix code by reverting that fix alone.
Four of the five are the same shape: a check that exists somewhere in this codebase and was missing from its sibling.
A21 —
/topicset had no membership or ban check (high)Commands are dispatched before any room gate (
handleMsg), so a peer banned from a room still reachedcmdTopic, where only+twas checked. Any connected identity could retitle any non-+troom it knew the name of — persisted torooms.tomlwhen registered, and broadcast to every member.The view path a few lines above already required membership for
+p. Only the set path was missing it. Now membership (or server-op) is required, for the reason/historystates outright: joining already cleared this room's ban, key and invite gates.A22 — a command body escaped
max_msg_body_bytes(high, compounds A21)handleMsgdispatches a slash command before the body-size check, so every command handler received an argument string bounded only by the 8 KiB link frame. Demonstrated: a 400-byte message refused, then a 7000-byte topic accepted on the same hub.Bounded once in
handleCommandso a new command cannot reintroduce it. The topic — the part that reaches disk and every future joiner — carries its ownmax_topic_bytesas well.max_topic_bytesdefaults to 256, not 350, so a maximum-length topic still fits inside the/topic <room> <text>that sets it.TestAMaximumLengthTopicFitsInsideTheCommandThatSetsItasserts that against the shipped defaults — a limit that advertises more than it can accept is its own bug, and at 350 it reporteda 422-byte command, over the 350-byte body limit — the last 72 bytes of max_topic_bytes are unreachable.A23 — peer eviction destroyed the mentions it exists to protect (high)
A peer with queued mentions is by definition one not seen lately — that is what "away" means and why the mentions are queued. Oldest-last-seen eviction therefore selected exactly those rows, silently.
max_known_peersstrangers connecting in sequence was enough (MaxSessionsbounds only concurrent links), so ordinary churn on a public hub flushed every held mention. That defeats the one delivery guarantee the RRC fallback makes: "held here and handed over when you next connect."Eviction now prefers a peer owed nothing, and logs when every peer is owed something rather than dropping mail unremarked.
A24 — a member could not
/whotheir own+proom (low)private && !serverOp, missing the&& !hasMemberits sibling incmdTopic's view path has.+pstill hides the room from outside — there is a test for that too.A25 — configured identity hashes were never validated (low)
isServerOpcompares against the hex of a full 16-byte identity hash, so a malformedtrusted_identitiesentry is not a weaker rule but one that can never fire — and the operator's only evidence was a permanent, unexplained "not authorized".The likeliest cause is not a typo but pasting a destination hash where an identity hash belongs, which is the same length and also hex. Entries now go through
parseHexHash— audit A14's validator, which the config path never got — and anything dropped is logged with the field name.Also
§6.6 LINKIDENTIFY→§6.7.6in the two comments touched here, per CLAUDE.md's standing rule.Verified sound during the review (not changed)
history.gohex-encodes the name as the directory; dirs0700, files0600..,",[, newline, tab, backslash and emoji; BurntSushi quotes and escapes every one.OnMessage, so the auto-reply cannot be aimed at a third party.+n/+mgates (A20); rate limiting runs before CBOR decode (A19); identity and ban are rechecked per frame (A1/A2).Checks
gofmt,go vet,go test ./..., and the-racesubset all pass.🤖 Generated with Claude Code
https://claude.ai/code/session_01RRgC769HyZWx1YaGoosp9t