Skip to content

Coordination relay is fail-open: undecodable payloads bypass actor-rewrite and are fanned out verbatim #146

Description

@tieguy

What

The coordination relay is fail-open: when the server cannot decode an incoming coordination payload, it still fans the raw bytes out to every peer in the room, incrementing only an invalid_messages counter. ADR-0023 records this as a deliberate forward-compatibility choice (see its Non-goals). Decision: it's too early to pay for forward-compat here — switch to fail-closed (drop undecodable payloads, don't relay them).

Why it matters (more than robustness)

Undecodable payloads bypass the authenticated actor-rewrite that ADR-0023 §3 relies on to stop operators spoofing each other:

  • sanitize_coordination_payload (crates/sp42-server/src/main.rs:1095) decodes the payload to rewrite the actor/winning_actor to the session username. On decode failure it logs a warning and returns the original bytes unchanged (main.rs:1100).
  • CoordinationRegistry::publish (crates/sp42-server/src/coordination.rs:71) then sender.send(envelope) fans the envelope out unconditionally (coordination.rs:91); the decode failure only bumps invalid_messages (coordination.rs:87).

So an authenticated client can craft a payload the server can't decode but a peer might (the client also accepts Text frames and could decode more leniently than the server), carrying an arbitrary actor. The server never sanitizes it, and relays it verbatim. That's a spoofing / injection path, not just fail-open relay hygiene.

Proposed change

  • Fail-closed: in publish, only sender.send when decode_message succeeded; drop otherwise. Keep the invalid_messages counter for observability.
  • Equivalently, never relay a payload that didn't pass the actor-rewrite/decode step.
  • Revisit a real forward-compat/versioning story later, if/when there's an actual need — at which point it should carry an explicit version field and a decode path, not "relay bytes we don't understand."
  • Update ADR-0023's Non-goals bullet to match once the behavior changes.

Notes

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