Skip to content

[app] Reorder dedup-before-trust check in handle_op #125

@intendednull

Description

@intendednull

Parent: #108

Problem

crates/app/src/ui/chat.rs:645-666:

// Dedup: skip if we've already seen this op.
if op_log.seen_ids.contains(&stamped.op_id) {
    return false;
}

// Trust check: only for non-chat ops (server state mutations).
let needs_trust = !matches!(stamped.op, Op::ChatMessage { .. });
if needs_trust {
    let owner = server_state.server.as_ref().map(|s| s.owner.to_string()).unwrap_or_default();
    if !op_log.is_trusted(from, &owner) {
        warn!("untrusted op from {from}, recording id only");
        op_log.seen_ids.insert(stamped.op_id.clone());
        return false;
    }
}

Dedup runs before the trust check. Then an untrusted op inserts its op_id into seen_ids anyway. A trusted peer reusing that op_id later would be dedup-blocked.

With random UUID op_ids this is not practically exploitable — an attacker can't predict which op_ids future legitimate senders will use. But the ordering is still a footgun and makes the code harder to reason about.

Fix

Reorder: trust check first, then dedup, then insert into seen_ids only on the happy path.

// Trust check first.
let needs_trust = !matches!(stamped.op, Op::ChatMessage { .. });
if needs_trust {
    let owner = server_state.server.as_ref().map(|s| s.owner.to_string()).unwrap_or_default();
    if !op_log.is_trusted(from, &owner) {
        warn!("untrusted op from {from}, dropping");
        return false;
    }
}

// Dedup.
if op_log.seen_ids.contains(&stamped.op_id) {
    return false;
}

// ... apply ...

op_log.record(stamped.clone());

This way we never insert an untrusted op's id into seen_ids at all.

Test

Add a test: untrusted peer sends op with op_id = X, then trusted peer sends a different op with the same op_id = X. With the fix, the trusted op should be accepted (assuming it's a legitimate first-time use by that peer). Document that op_ids are globally unique UUIDs so collision is not a concern in practice.

Note

This is a code-smell fix, not a security fix. Scheduling it alongside critical work is fine but it's not blocking.

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