Skip to content

Deferred review findings from the de-policing refactor (app principals / leases / encryption removal) #940

Description

@chughtapan

Deferred findings from the cross-model pre-landing review of the de-policing refactor (PR: refactor/remove-app-principals-and-leases). Sources: five specialist reviewers, a red-team pass, Claude adversarial, and Codex adversarial + structured review. Fixed-in-PR items are excluded; everything below was deliberately deferred (maintainer call during ship).

Security / hardening (P1-ish)

  • Agent suspension is unenforceable against a live socket. AgentContext.agentStatus freezes at connect; nothing re-reads agents.status or closes matching sockets on suspension. With dispatch admission gone, suspension is the only moderation lever left. Fix direction: re-check status in the ActiveAgent gate (short-TTL cache) or a revocation path over ConnectionManager.agentConnections.
  • moltzapd MCP HTTP listener has no authentication. Loopback-bound with DNS-rebinding guards, but any local process can drive the daemon (the Unix socket equivalent is 0600). Fix direction: per-daemon bearer token persisted 0600, constant-time compare; or serve MCP over the Unix socket.
  • Re-Connect on an authenticated socket corrupts routing state. A second agent/network/connect with a different valid key keeps identity A's auth while hydrating B's conversations and remapping the endpoint resolver to B; the resolver entry leaks permanently on close. Also re-fires connection hooks per frame without validating the supplied key, and the declared AlreadyConnected error is never raised (its schema also still carries a dead "app" principal literal). Fix direction: peek the live arm in connectPreamble, reject cross-identity re-connects, raise AlreadyConnected.
  • Dev mode = 0.0.0.0 + CORS * + open registration. Any web page a developer visits can register an agent cross-origin and read the key from the response; same exposure LAN-wide. Fix direction: bind loopback in dev mode and/or refuse * on the register route.
  • Unix-socket startup window. The daemon socket listens before chmod 0600 and ignores chmod failure. Fix direction: create inside a 0700 directory or fail closed.
  • agents/list returns ownerUserId and status for every agent to any authenticated agent. Decide whether the roster is meant to be global; drop ownerUserId from the wire card otherwise.
  • Notification fan-out is an unbounded-fiber surface. Reverse-RPC notifies are unscoped runFork with no timeout; a peer that withholds ACKs on a second socket accumulates fibers per send, and delivered is recorded at spawn, not ack (pre-existing, easier to drive post-refactor).

Correctness / robustness

  • Send has no idempotency key: a post-insert failure is ambiguous and a retry double-posts (pre-existing wire semantics).
  • MoltZapService.connect() reentrancy leaks the previous scope/client/fiber; beginShutdown has an interruption window that can wedge future connects.
  • Live delivery order can disagree with seq-ordered history under concurrent sends (pre-existing).
  • nextSnowflakeId is process-local: two server instances can collide on UNIQUE(conversation_id, seq) (defect on send; multi-instance deployments only).
  • Channel-core: interceptor sees only the newest message of a coalesced batch while the handler receives all of them; interceptor failures deliberately fail open; a peer can grow the unbounded inbound queue while a handler stalls; drain does O(B²) queue work under multi-conversation backlog; coalesced-turn text allows sender-attribution forgery (queued-message marker lines are not escaped, unlike the cross-conversation block).
  • Client lookups: lastReadRef grows unboundedly per message read; history metadata silently missing past the first conversationList page; agent-name resolution silently caps at 2000 agents.
  • Server: ConnectionManager has no agentId index (O(n) scans per send/disconnect); duplicate idx_messages_conversation_seq index doubles insert write amplification; readSendConversation re-reads a row the send-access gate just read.

Simulator

  • linkPolicy.delay is a rate limit, not link latency (serial lane + inline sleep: N messages take N·D). Docs say "delay every message"; decide which semantic is intended and align.
  • Held deliveries at program end convert to late deliveries racing teardown; hold evidence (LinkMessageHeld) is written once per re-park, so counts overstate; decide terminal-hold semantics.
  • Policy install/disable have interruption windows that can strand policies or desync the controller's release path into a dying finalizer; LinkDriver is exposed to programs alongside LinkController and direct use can defeat lease counting.
  • Endpoint inbox: per-conversation mailboxes retain every message forever; endpoint.messages subscribes at run time and silently misses earlier publishes; ledger-write failure in ingress/fabric surfaces as a clean end-of-stream rather than a failure (bounded only by the kernel-level failure race).

Follow-through from the ship

  • Wire fence: PROTOCOL_VERSION still reads 2026.802.0 on this branch — maintainer decision to let release automation assign the next version; source/docker deploys between merge and publish share a version string across incompatible wires.
  • VidushiS/moltzap-propagation-bench compiles against removed exports; coordinate its upgrade with the release.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions