fix(federation): harden processFriendRequestCreateEvent receiver-side

Two correctness/defense fixes plus regression tests in the existing in-memory drizzle test file. 1. Reverse-direction idempotency. The sender-side path in social.ts checks BOTH directions of friend_requests and returns 409 incoming_request_exists when an opposite-direction row exists. The receiver only matched from->to, so cross-fire (alice@A and bob@B both click "add friend" near-simultaneously) produced two opposite pending rows on each instance. The receiver now silent-accepts when either direction matches a pending row, mirroring the sender's both-direction check. 2. Self-target guard (defense-in-depth). Reject events whose from-identity equals to-identity (after normalizeOriginForCompare) with a new receiver-acknowledged 4xx code self_target_invalid. Sender's local cannot_friend_self should catch this, but the receiver does not trust upstream validation. Added to TERMINAL_REJECTION_REASONS so the standard rollback fires (mapped client-side to peer_rejected). Logged at console.warn. Spec updates: social.md inbound contract now documents both-direction idempotency and the self-target guard; federation.md and the s2s-friend-add design spec list the new terminal rejection reason.
2026-04-27 00:07:52 +02:00
parent 8cad964809
commit b698ded47d
5 changed files with 198 additions and 14 deletions
@@ -537,6 +537,163 @@ describe('processFriendRequestCreateEvent — branch coverage', () => {
    expect(req?.createdAt).toBeLessThanOrEqual(Date.now());
  });

+  it('accepts idempotently when a reverse-direction pending request already exists (cross-fire race)', async () => {
+    // Race scenario: alice@home and bob@orbit both click "add friend" near-simultaneously.
+    // Each sender's local both-direction check passes (no rows yet anywhere). When events cross,
+    // alice's outbound creates the bob-stub→alice row first; bob's inbound (this event) must
+    // detect the existing alice→bob-stub row in the REVERSE direction and silent-accept.
+    seedLocalUser('alice-id', 'alice');
+    seedReplicatedUser({
+      id: 'bob-stub',
+      username: 'remote-bob@orbit.test',
+      homeUserId: 'remote-bob',
+      homeInstance: 'orbit.test',
+    });
+    // Pre-existing reverse-direction row: alice (local) → bob-stub. Equivalent to alice having
+    // already sent her own outbound friend request to bob just before bob's event arrived.
+    testDb.insert(schema.friendRequests).values({
+      id: 'alice-outbound',
+      fromId: 'alice-id',
+      toId: 'bob-stub',
+      status: 'pending',
+      createdAt: 1000,
+    }).run();
+
+    const event = makeEvent({
+      messageId: 'reverse-direction-collision',
+      friendship: {
+        from: { homeUserId: 'remote-bob', homeInstance: 'https://orbit.test' },
+        to: { homeUserId: 'alice-id', homeInstance: 'https://home.test' },
+        fromProfile: { username: 'bob' },
+      },
+    });
+
+    const { processRelayEvents } = await import('./federation.js');
+    const result = await processRelayEvents([event], 'https://orbit.test', 'https://orbit.test', testDb);
+
+    // Idempotent silent-accept: no new row, no broadcast, original alice-outbound preserved.
+    expect(result.accepted).toEqual(['reverse-direction-collision']);
+    expect(result.rejected).toEqual([]);
+    const reqs = testDb.select().from(schema.friendRequests).all();
+    expect(reqs).toHaveLength(1);
+    expect(reqs[0]!.id).toBe('alice-outbound');
+    expect(reqs[0]!.fromId).toBe('alice-id');
+    expect(reqs[0]!.toId).toBe('bob-stub');
+    expect(sendToUser).not.toHaveBeenCalled();
+  });
+
+  it('does not block on a non-pending reverse-direction row (declined)', async () => {
+    // The reverse-direction idempotency must still be gated on status='pending'.
+    // A previously declined request from alice→bob-stub does NOT make this event idempotent.
+    seedLocalUser('alice-id', 'alice');
+    seedReplicatedUser({
+      id: 'bob-stub',
+      username: 'remote-bob@orbit.test',
+      homeUserId: 'remote-bob',
+      homeInstance: 'orbit.test',
+    });
+    testDb.insert(schema.friendRequests).values({
+      id: 'old-reverse-declined',
+      fromId: 'alice-id',
+      toId: 'bob-stub',
+      status: 'declined',
+      createdAt: 1000,
+    }).run();
+
+    const event = makeEvent({
+      messageId: 'fresh-after-reverse-decline',
+      friendship: {
+        from: { homeUserId: 'remote-bob', homeInstance: 'https://orbit.test' },
+        to: { homeUserId: 'alice-id', homeInstance: 'https://home.test' },
+        fromProfile: { username: 'bob' },
+        createdAt: 5000,
+      },
+    });
+
+    const { processRelayEvents } = await import('./federation.js');
+    const result = await processRelayEvents([event], 'https://orbit.test', 'https://orbit.test', testDb);
+
+    expect(result.accepted).toEqual(['fresh-after-reverse-decline']);
+    expect(result.rejected).toEqual([]);
+    const pending = testDb.select().from(schema.friendRequests)
+      .where(eq(schema.friendRequests.status, 'pending')).all();
+    expect(pending).toHaveLength(1);
+    expect(pending[0]!.fromId).toBe('bob-stub');
+    expect(pending[0]!.toId).toBe('alice-id');
+    expect(sendToUser).toHaveBeenCalledOnce();
+  });
+
+  it('rejects with self_target_invalid when from-identity equals to-identity (defense-in-depth)', async () => {
+    // Malformed/malicious event where the from and to identities collapse. The sender's
+    // local cannot_friend_self check should prevent this, but the receiver must not trust it.
+    // Pre-resolution rejection: no stub created, no row inserted, no broadcast.
+    const event = makeEvent({
+      messageId: 'self-target-raw',
+      friendship: {
+        from: { homeUserId: 'remote-bob', homeInstance: 'https://orbit.test' },
+        to: { homeUserId: 'remote-bob', homeInstance: 'https://orbit.test' },
+        fromProfile: { username: 'bob' },
+      },
+    });
+
+    const { processRelayEvents } = await import('./federation.js');
+    const result = await processRelayEvents([event], 'https://orbit.test', 'https://orbit.test', testDb);
+
+    expect(result.rejected).toEqual([{ messageId: 'self-target-raw', reason: 'self_target_invalid' }]);
+    expect(result.accepted).toEqual([]);
+    expect(testDb.select().from(schema.friendRequests).all()).toHaveLength(0);
+    // No side-effect stub creation for the malformed identity.
+    expect(testDb.select().from(schema.users).where(eq(schema.users.homeUserId, 'remote-bob')).all()).toHaveLength(0);
+    expect(sendToUser).not.toHaveBeenCalled();
+  });
+
+  it('rejects self-target even when origin shapes differ (URL vs bare host, trailing slash)', async () => {
+    // normalizeOriginForCompare must collapse "https://orbit.test", "orbit.test", and
+    // "https://orbit.test/" to the same canonical form so the guard is not bypassable
+    // by surface formatting of homeInstance.
+    const event = makeEvent({
+      messageId: 'self-target-normalized',
+      friendship: {
+        from: { homeUserId: 'remote-bob', homeInstance: 'https://orbit.test' },
+        to: { homeUserId: 'remote-bob', homeInstance: 'orbit.test/' },
+        fromProfile: { username: 'bob' },
+      },
+    });
+
+    const { processRelayEvents } = await import('./federation.js');
+    const result = await processRelayEvents([event], 'https://orbit.test', 'https://orbit.test', testDb);
+
+    expect(result.rejected).toEqual([{ messageId: 'self-target-normalized', reason: 'self_target_invalid' }]);
+    expect(result.accepted).toEqual([]);
+    expect(testDb.select().from(schema.friendRequests).all()).toHaveLength(0);
+    expect(sendToUser).not.toHaveBeenCalled();
+  });
+
+  it('does not falsely flag self-target when only homeUserId matches across different instances', async () => {
+    // Two different users on different instances who happen to share a homeUserId string
+    // must NOT be treated as self-target. This protects against a too-aggressive guard.
+    seedLocalUser('shared-id', 'alice');
+
+    const event = makeEvent({
+      messageId: 'shared-id-cross-instance',
+      friendship: {
+        from: { homeUserId: 'shared-id', homeInstance: 'https://orbit.test' },
+        to: { homeUserId: 'shared-id', homeInstance: 'https://home.test' },
+        fromProfile: { username: 'bob' },
+      },
+    });
+
+    const { processRelayEvents } = await import('./federation.js');
+    const result = await processRelayEvents([event], 'https://orbit.test', 'https://orbit.test', testDb);
+
+    expect(result.accepted).toEqual(['shared-id-cross-instance']);
+    expect(result.rejected).toEqual([]);
+    const reqs = testDb.select().from(schema.friendRequests).all();
+    expect(reqs).toHaveLength(1);
+    expect(reqs[0]!.toId).toBe('shared-id');
+    expect(sendToUser).toHaveBeenCalledOnce();
+  });
+
  it('isolates per-event success/failure within a batch (mixed accepted/rejected)', async () => {
    seedLocalUser('alice-id', 'alice');

@@ -6,7 +6,7 @@ import { pipeline } from 'node:stream/promises';
 import { Readable } from 'node:stream';
 import { eq, and, or, isNull, inArray, sql, desc } from 'drizzle-orm';
 import { authenticate, requireAdmin } from '../utils/auth.js';
-import { generateHmacSecret, getOurOrigin, parseFederationHeaders, verifySignature, verifyPeerSignature, buildFederationHeaders } from '../utils/federationAuth.js';
+import { generateHmacSecret, getOurOrigin, parseFederationHeaders, verifySignature, verifyPeerSignature, buildFederationHeaders, normalizeOriginForCompare } from '../utils/federationAuth.js';
 import { generateSnowflake } from '../utils/snowflake.js';
 import { getDb, getRawDb, schema } from '../db/index.js';
 import { config } from '../config.js';
@@ -4529,6 +4529,17 @@ function processFriendRequestCreateEvent(
    return;
  }

+  // Self-target guard (defense-in-depth): from-identity must not equal to-identity.
+  // Sender's local cannot_friend_self check should catch this, but the receiver must not trust it.
+  if (
+    from.homeUserId === to.homeUserId &&
+    normalizeOriginForCompare(from.homeInstance) === normalizeOriginForCompare(to.homeInstance)
+  ) {
+    console.warn(`[federation] Self-target friend_request_create rejected: homeUserId=${from.homeUserId} homeInstance=${extractDomain(from.homeInstance)} source=${extractDomain(sourceInstance)}`);
+    rejected.push({ messageId: event.messageId, reason: 'self_target_invalid' });
+    return;
+  }
+
  // Resolve the sender (create stub if needed — they're on a remote instance)
  const fromUserResolved = resolveOrCreateReplicatedUser(from.homeUserId, from.homeInstance, db, { username: event.friendship.fromProfile?.username });
  if (!fromUserResolved) {
@@ -4562,14 +4573,23 @@ function processFriendRequestCreateEvent(
    return;
  }

-  // Idempotency: if a pending request already exists from this sender to this recipient, accept as no-op
+  // Idempotency: a pending request in EITHER direction makes this event a no-op.
+  //   Forward (from→to): re-delivery of an event we've already processed.
+  //   Reverse (to→from): the local user has already sent a request TO this remote sender.
+  //     Race window: both sides click "add friend" near-simultaneously. Each sender's both-direction
+  //     check passes locally (no rows yet anywhere). When the events cross, each receiver must
+  //     treat the reverse-direction collision as idempotent — otherwise both instances end up
+  //     with two opposite-direction pending rows for the same logical pair. Mirror the
+  //     sender-side both-direction check (`incoming_request_exists` in social.ts).
  const existingRequest = db
    .select()
    .from(schema.friendRequests)
    .where(
      and(
-        eq(schema.friendRequests.fromId, fromUser.id),
-        eq(schema.friendRequests.toId, toUser.id),
+        or(
+          and(eq(schema.friendRequests.fromId, fromUser.id), eq(schema.friendRequests.toId, toUser.id)),
+          and(eq(schema.friendRequests.fromId, toUser.id), eq(schema.friendRequests.toId, fromUser.id)),
+        ),
        eq(schema.friendRequests.status, 'pending'),
      ),
    )