From edcb4cb72ad99da6c366af1fb5c947a9eb1602f4 Mon Sep 17 00:00:00 2001 From: Jannis Braun <151788261+TheZwiss@users.noreply.github.com> Date: Thu, 2 Jul 2026 12:40:16 +0200 Subject: [PATCH] fix(federation): /peer/accept returns honest 409 instead of false 200 for existing peer (BUG-1a) --- .../src/routes/federation.peerAccept.test.ts | 166 +++++++++++++++++- packages/server/src/routes/federation.ts | 14 +- 2 files changed, 175 insertions(+), 5 deletions(-) diff --git a/packages/server/src/routes/federation.peerAccept.test.ts b/packages/server/src/routes/federation.peerAccept.test.ts index 0e9bad04..d26433dd 100644 --- a/packages/server/src/routes/federation.peerAccept.test.ts +++ b/packages/server/src/routes/federation.peerAccept.test.ts @@ -185,7 +185,7 @@ describe('POST /api/federation/peer/accept — instance_name persistence', () => expect(row?.status).toBe('active'); }); - it('does NOT overwrite instance_name on the active idempotent early-return path', async () => { + it('does NOT overwrite instance_name on the active idempotent early-return path (now 409)', async () => { testDb.insert(schema.federationPeers).values({ id: 'peer-active', origin: 'https://remote.example', @@ -205,7 +205,9 @@ describe('POST /api/federation/peer/accept — instance_name persistence', () => }, }); - expect(response.statusCode).toBe(200); + // BUG-1a: existing active peer now returns an honest 409 refusal, not a + // false 200. Anti-hijack unchanged — name/secret must be untouched. + expect(response.statusCode).toBe(409); const row = testDb.select().from(schema.federationPeers) .where(eq(schema.federationPeers.id, 'peer-active')).get(); expect(row?.instanceName).toBe('Original Name'); @@ -257,7 +259,7 @@ describe('POST /api/federation/peer/accept — response body carries instanceNam expect(body.instanceName).toBe('Local Backspace'); }); - it('returns our instanceName on the active idempotent path too', async () => { + it('returns our instanceName on the active refusal path too (409)', async () => { testDb.insert(schema.federationPeers).values({ id: 'peer-active', origin: 'https://remote.example', @@ -277,7 +279,9 @@ describe('POST /api/federation/peer/accept — response body carries instanceNam }, }); - expect(response.statusCode).toBe(200); + // BUG-1a: honest 409 refusal still carries our own instanceName so the + // initiator can surface a useful "reset required" message. + expect(response.statusCode).toBe(409); const body = response.json() as { accepted: boolean; instanceName?: string | null }; expect(body.instanceName).toBe('Local Backspace'); }); @@ -300,6 +304,160 @@ describe('POST /api/federation/peer/accept — response body carries instanceNam }); }); +describe('POST /api/federation/peer/accept — BUG-1a: honest 409 refusal for existing active/needs_attention peer', () => { + let app: FastifyInstance; + + beforeEach(async () => { + sqlite = new Database(':memory:'); + testDb = drizzle(sqlite, { schema }); + applyMigrations(sqlite); + seedInstanceSettings('Local Backspace'); + app = await buildApp(); + }); + + it('returns 409 PEER_EXISTS_RESET_REQUIRED and keeps S0 when an active peer receives a different secret (matching epoch)', async () => { + testDb.insert(schema.federationPeers).values({ + id: 'peer-active', + origin: 'https://caller.example', + hmacSecret: 'S0', + status: 'active', + peerInstanceId: 'epoch-A', + instanceName: 'Caller', + createdAt: Date.now(), + }).run(); + + const response = await app.inject({ + method: 'POST', + url: '/api/federation/peer/accept', + remoteAddress: '10.0.1.1', // isolate rate-limit bucket (endpoint is IP-keyed) + payload: { + sourceOrigin: 'https://caller.example', + hmacSecret: 'S1-different', + instanceId: 'epoch-A', // same epoch → no reset, pure idempotent refusal + }, + }); + + expect(response.statusCode).toBe(409); + const body = response.json() as { + accepted: boolean; + code: string; + error: string; + instanceName?: string | null; + instanceId?: string | null; + statusCode: number; + }; + expect(body.accepted).toBe(false); + expect(body.code).toBe('PEER_EXISTS_RESET_REQUIRED'); + expect(body.statusCode).toBe(409); + expect(body.instanceName).toBe('Local Backspace'); + + // Anti-hijack: stored secret unchanged, status unchanged (same epoch). + const row = testDb.select().from(schema.federationPeers) + .where(eq(schema.federationPeers.id, 'peer-active')).get(); + expect(row?.hmacSecret).toBe('S0'); + expect(row?.status).toBe('active'); + }); + + it('returns 409 and keeps S0 when instanceId is omitted (legacy caller, no epoch)', async () => { + testDb.insert(schema.federationPeers).values({ + id: 'peer-active', + origin: 'https://caller.example', + hmacSecret: 'S0', + status: 'active', + peerInstanceId: 'epoch-A', + createdAt: Date.now(), + }).run(); + + const response = await app.inject({ + method: 'POST', + url: '/api/federation/peer/accept', + remoteAddress: '10.0.1.2', // isolate rate-limit bucket (endpoint is IP-keyed) + payload: { + sourceOrigin: 'https://caller.example', + hmacSecret: 'S1-different', + // no instanceId → epoch-mismatch guard cannot fire + }, + }); + + expect(response.statusCode).toBe(409); + const body = response.json() as { code: string }; + expect(body.code).toBe('PEER_EXISTS_RESET_REQUIRED'); + + const row = testDb.select().from(schema.federationPeers) + .where(eq(schema.federationPeers.id, 'peer-active')).get(); + expect(row?.hmacSecret).toBe('S0'); + expect(row?.status).toBe('active'); + }); + + it('epoch MISMATCH: fires markPeerReset (row → needs_attention) AND still returns 409 with S0 unchanged', async () => { + testDb.insert(schema.federationPeers).values({ + id: 'peer-active', + origin: 'https://caller.example', + hmacSecret: 'S0', + status: 'active', + peerInstanceId: 'epoch-A', + createdAt: Date.now(), + }).run(); + + const response = await app.inject({ + method: 'POST', + url: '/api/federation/peer/accept', + remoteAddress: '10.0.1.3', // isolate rate-limit bucket (endpoint is IP-keyed) + payload: { + sourceOrigin: 'https://caller.example', + hmacSecret: 'S1-different', + instanceId: 'epoch-B-new-incarnation', // differs from stored epoch-A + }, + }); + + expect(response.statusCode).toBe(409); + const body = response.json() as { code: string }; + expect(body.code).toBe('PEER_EXISTS_RESET_REQUIRED'); + + // Detection still fires: row routed to needs_attention, observed epoch recorded, + // but trusted baseline (peerInstanceId) and secret are NOT rekeyed. + const row = testDb.select().from(schema.federationPeers) + .where(eq(schema.federationPeers.id, 'peer-active')).get(); + expect(row?.status).toBe('needs_attention'); + expect(row?.needsAttentionReason).toBe('peer_reset_detected'); + expect(row?.observedPeerInstanceId).toBe('epoch-B-new-incarnation'); + expect(row?.peerInstanceId).toBe('epoch-A'); + expect(row?.hmacSecret).toBe('S0'); + }); + + it('existing needs_attention peer: same 409 refusal, secret unchanged', async () => { + testDb.insert(schema.federationPeers).values({ + id: 'peer-na', + origin: 'https://caller.example', + hmacSecret: 'S0', + status: 'needs_attention', + needsAttentionReason: 'peer_reset_detected', + peerInstanceId: 'epoch-A', + createdAt: Date.now(), + }).run(); + + const response = await app.inject({ + method: 'POST', + url: '/api/federation/peer/accept', + remoteAddress: '10.0.1.4', // isolate rate-limit bucket (endpoint is IP-keyed) + payload: { + sourceOrigin: 'https://caller.example', + hmacSecret: 'S1-different', + instanceId: 'epoch-A', + }, + }); + + expect(response.statusCode).toBe(409); + const body = response.json() as { code: string }; + expect(body.code).toBe('PEER_EXISTS_RESET_REQUIRED'); + + const row = testDb.select().from(schema.federationPeers) + .where(eq(schema.federationPeers.id, 'peer-na')).get(); + expect(row?.hmacSecret).toBe('S0'); + expect(row?.status).toBe('needs_attention'); + }); +}); + describe('POST /api/federation/peer/initiate — persists remote instanceName from handshake response', () => { let app: FastifyInstance; diff --git a/packages/server/src/routes/federation.ts b/packages/server/src/routes/federation.ts index ddf4bb20..1d4b0144 100644 --- a/packages/server/src/routes/federation.ts +++ b/packages/server/src/routes/federation.ts @@ -1130,7 +1130,19 @@ export async function federationRoutes(app: FastifyInstance): Promise { if (reqInstanceId && existing.peerInstanceId && reqInstanceId !== existing.peerInstanceId) { markPeerReset(existing.id, sourceOrigin, existing.peerInstanceId, reqInstanceId); } - return reply.code(200).send({ accepted: true, instanceName: ourInstanceName, instanceId: ourInstanceId }); + // Anti-hijack: we did NOT adopt the caller's secret. Report that + // honestly (409) instead of a false success (was 200 {accepted:true}), + // so the initiator does not false-activate into a permanent HMAC + // desync. Legacy initiators read only response.ok → they fail loudly + // (never a silent desync); new initiators special-case this code. + return reply.code(409).send({ + accepted: false, + code: 'PEER_EXISTS_RESET_REQUIRED', + error: 'This instance already holds peering for you; its admin must reset that peering before a new handshake can be accepted.', + instanceName: ourInstanceName, + instanceId: ourInstanceId, + statusCode: 409, + }); } if (existing.status === 'revoked') { return reply.code(403).send({