From abdaf99bb457d52f677cd6e3db68763865c7a80f Mon Sep 17 00:00:00 2001 From: Jannis Braun <151788261+TheZwiss@users.noreply.github.com> Date: Tue, 31 Mar 2026 21:01:02 +0200 Subject: [PATCH] fix(federation): address code review findings for FED-011 - Fix race window: store pendingHmacSecret AFTER remote peer confirms, not before (admin endpoint + auto-rotation worker) - Add hex validation on newSecret at /peer/rotate endpoint - Use pending-secret-aware signing in initial sync worker - Add test for corrupt state (pendingHmacSecret set, secretRotationAt null) --- packages/server/src/routes/federation.ts | 32 ++++++------------- .../server/src/utils/federationAuth.test.ts | 10 ++++++ packages/server/src/utils/federationWorker.ts | 32 +++++++------------ 3 files changed, 32 insertions(+), 42 deletions(-) diff --git a/packages/server/src/routes/federation.ts b/packages/server/src/routes/federation.ts index 93ac9a18..5546bcd7 100644 --- a/packages/server/src/routes/federation.ts +++ b/packages/server/src/routes/federation.ts @@ -416,7 +416,7 @@ export async function federationRoutes(app: FastifyInstance): Promise { // 2. Validate request body const { newSecret } = request.body ?? {}; - if (!newSecret || typeof newSecret !== 'string' || newSecret.length !== 64) { + if (!newSecret || typeof newSecret !== 'string' || newSecret.length !== 64 || !/^[0-9a-f]+$/.test(newSecret)) { return reply.code(400).send({ error: 'newSecret must be a 64-character hex string', statusCode: 400 }); } @@ -534,15 +534,6 @@ export async function federationRoutes(app: FastifyInstance): Promise { }); } - // Store pending secret locally BEFORE posting to peer (per audit recommendation) - db.update(schema.federationPeers) - .set({ - pendingHmacSecret: newSecret, - secretRotationAt: Date.now(), - }) - .where(eq(schema.federationPeers.id, peer.id)) - .run(); - // Send rotation request to peer, signed with the CURRENT (old) secret try { const rotateBody = JSON.stringify({ newSecret }); @@ -556,12 +547,6 @@ export async function federationRoutes(app: FastifyInstance): Promise { }); if (!response.ok) { - // Rollback — clear pending secret - db.update(schema.federationPeers) - .set({ pendingHmacSecret: null, secretRotationAt: null }) - .where(eq(schema.federationPeers.id, peer.id)) - .run(); - let errorMessage = `Remote instance rejected rotation (HTTP ${response.status})`; try { const body = await response.json() as { error?: string }; @@ -571,16 +556,19 @@ export async function federationRoutes(app: FastifyInstance): Promise { return reply.code(502).send({ error: errorMessage, statusCode: 502 }); } + // Store pending secret locally AFTER remote peer confirms acceptance + db.update(schema.federationPeers) + .set({ + pendingHmacSecret: newSecret, + secretRotationAt: Date.now(), + }) + .where(eq(schema.federationPeers.id, peer.id)) + .run(); + console.log(`[federation] Secret rotation initiated with peer ${peer.origin}`); return reply.code(200).send({ success: true, gracePeriodMs: 900_000 }); } catch (err: unknown) { - // Rollback — clear pending secret - db.update(schema.federationPeers) - .set({ pendingHmacSecret: null, secretRotationAt: null }) - .where(eq(schema.federationPeers.id, peer.id)) - .run(); - const message = err instanceof Error ? err.message : 'Unknown error'; if (err instanceof DOMException && err.name === 'TimeoutError') { return reply.code(504).send({ diff --git a/packages/server/src/utils/federationAuth.test.ts b/packages/server/src/utils/federationAuth.test.ts index ca85cec6..4b0a55b7 100644 --- a/packages/server/src/utils/federationAuth.test.ts +++ b/packages/server/src/utils/federationAuth.test.ts @@ -67,4 +67,14 @@ describe('verifyPeerSignature', () => { const sig = signRequest(body, primarySecret, timestamp, null); expect(verifyPeerSignature(body, sig, timestamp, null, makePeer())).toBe(true); }); + + it('does not try pending secret when secretRotationAt is null', () => { + const timestamp = Date.now(); + const sig = signRequest(body, pendingSecret, timestamp, nonce); + const peer = makePeer({ + pendingHmacSecret: pendingSecret, + secretRotationAt: null, + }); + expect(verifyPeerSignature(body, sig, timestamp, nonce, peer)).toBe(false); + }); }); diff --git a/packages/server/src/utils/federationWorker.ts b/packages/server/src/utils/federationWorker.ts index 1a89b3cc..ae350cdf 100644 --- a/packages/server/src/utils/federationWorker.ts +++ b/packages/server/src/utils/federationWorker.ts @@ -733,15 +733,6 @@ async function processHealthCheckTick(): Promise { // Time to rotate const newSecret = generateHmacSecret(); - // Store pending locally first - db.update(schema.federationPeers) - .set({ - pendingHmacSecret: newSecret, - secretRotationAt: Date.now(), - }) - .where(eq(schema.federationPeers.id, peer.id)) - .run(); - try { const rotateBody = JSON.stringify({ newSecret }); const headers = buildFederationHeaders(rotateBody, peer.hmacSecret, ourOrigin); @@ -754,21 +745,19 @@ async function processHealthCheckTick(): Promise { }); if (response.ok) { - console.log(`[federation-worker] Auto-rotation initiated with peer ${peer.origin}`); - } else { - // Rollback + // Store pending locally AFTER remote peer confirms acceptance db.update(schema.federationPeers) - .set({ pendingHmacSecret: null, secretRotationAt: null }) + .set({ + pendingHmacSecret: newSecret, + secretRotationAt: Date.now(), + }) .where(eq(schema.federationPeers.id, peer.id)) .run(); + console.log(`[federation-worker] Auto-rotation initiated with peer ${peer.origin}`); + } else { console.warn(`[federation-worker] Auto-rotation rejected by peer ${peer.origin} (HTTP ${response.status})`); } } catch (err) { - // Rollback - db.update(schema.federationPeers) - .set({ pendingHmacSecret: null, secretRotationAt: null }) - .where(eq(schema.federationPeers.id, peer.id)) - .run(); const message = err instanceof Error ? err.message : 'Unknown error'; console.warn(`[federation-worker] Auto-rotation failed for peer ${peer.origin}: ${message}`); } @@ -854,6 +843,9 @@ async function runInitialSyncForNewPeers(): Promise { const ourOrigin = getOurOrigin(); for (const peer of unsyncedPeers) { + const signingSecret = (peer.pendingHmacSecret && peer.secretRotationAt) + ? peer.pendingHmacSecret + : peer.hmacSecret; try { console.log(`[federation-worker] Running initial sync with ${peer.origin}...`); let sinceTimestamp = 0; @@ -862,7 +854,7 @@ async function runInitialSyncForNewPeers(): Promise { // Paginate through all events from the peer while (true) { const body = JSON.stringify({ sinceTimestamp, limit: 100 }); - const headers = buildFederationHeaders(body, peer.hmacSecret, ourOrigin); + const headers = buildFederationHeaders(body, signingSecret, ourOrigin); const response = await fetch(`${peer.origin}/api/federation/sync`, { method: 'POST', @@ -893,7 +885,7 @@ async function runInitialSyncForNewPeers(): Promise { let friendSinceTimestamp = 0; while (true) { const friendBody = JSON.stringify({ sinceTimestamp: friendSinceTimestamp, contextType: 'friend', limit: 100 }); - const friendHeaders = buildFederationHeaders(friendBody, peer.hmacSecret, ourOrigin); + const friendHeaders = buildFederationHeaders(friendBody, signingSecret, ourOrigin); const friendResponse = await fetch(`${peer.origin}/api/federation/sync`, { method: 'POST',