fix(dm): ownership transfer divergence after back-and-forth — canonicalize ownerHomeInstance + normalize authority checks
Manual ownership transfers between two federated instances diverged because `dm_channels.ownerHomeInstance` was stored as a BARE host (`orbit.ddns.net`) for federated owners — via `transferGroupDmOwnership` copying `users.homeInstance` verbatim — while `sourceInstance` always arrives as a full URL on the wire. `processOwnershipTransferEvent` and `processMemberRemoveEvent` then compared the two with strict equality and rejected legitimate inbound events as `unauthorized_source`, keeping ownership permanently divergent across peers. Live DB inspection on the two test instances confirmed both rows (nova + orbit) had a BARE `owner_home_instance`, matching the bug report exactly. Three compounding fixes: 1. Receiver authority checks now compare via `normalizeOriginForCompare` so legacy bare-vs-full rows accept legitimate transfers (and kicks). 2. New `canonicalizeHomeInstance` helper in `federationAuth.ts`; every write site that persists `ownerHomeInstance` (`transferGroupDmOwnership`, group DM creation, lazy federation in member-add, `processMemberAddEvent` bootstrap, `processOwnershipTransferEvent` receiver storage) routes through it. Full URL is the canonical storage form, matching how `sourceInstance` arrives. 3. `dm_owner_updated` WS event extended with optional `newOwnerHomeUserId` and `newOwnerHomeInstance` fields. Client `updateDmOwner` writes them when present and leaves existing values untouched otherwise (legacy-server safe). Without this, `getOwnerInstanceForDm` returned the previous owner's home after a successful WS broadcast, routing the next owner-only op to the wrong instance. Coverage: new `federation.ownershipTransfer.test.ts` (7 receiver tests including the headline bare-vs-full regression and the dedup replay guard); new bare-vs-full case in `federation.kick.test.ts`; two new client-side cases in `groupDm.ownerRouting.test.ts` covering both the extended-payload write path and the legacy-server passthrough. Tests: 1053 server + 364 web, all green. Specs updated: `dm-system.md` historical bugs + frontend handler table + WS state-change events table; `federation.md` `ownership_transfer` receiver flow; `websocket.md` event-fields table.
This commit is contained in:
@@ -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, normalizeOriginForCompare } from '../utils/federationAuth.js';
|
||||
import { generateHmacSecret, getOurOrigin, parseFederationHeaders, verifySignature, verifyPeerSignature, buildFederationHeaders, normalizeOriginForCompare, canonicalizeHomeInstance } from '../utils/federationAuth.js';
|
||||
import { generateSnowflake } from '../utils/snowflake.js';
|
||||
import { getDb, getRawDb, schema } from '../db/index.js';
|
||||
import { config } from '../config.js';
|
||||
@@ -4120,7 +4120,10 @@ export async function processMemberAddEvent(
|
||||
federatedId: event.federatedId,
|
||||
ownerId,
|
||||
ownerHomeUserId: event.group.owner?.homeUserId ?? null,
|
||||
ownerHomeInstance: event.group.owner?.homeInstance ?? null,
|
||||
// Canonicalize on storage so future authority comparisons against
|
||||
// `sourceInstance` (always a full URL) match cleanly. Defensive: older
|
||||
// peers may have sent a bare host on the wire.
|
||||
ownerHomeInstance: canonicalizeHomeInstance(event.group.owner?.homeInstance) ?? null,
|
||||
createdAt: now,
|
||||
name: bootstrapName,
|
||||
icon: bootstrapResolvedIcon,
|
||||
@@ -4362,8 +4365,20 @@ export function processMemberRemoveEvent(
|
||||
return;
|
||||
}
|
||||
|
||||
// Validate authority: owner's instance for kicks, any instance for self-leave
|
||||
if (event.membership.reason !== 'leave' && channel.ownerHomeInstance && sourceInstance !== channel.ownerHomeInstance) {
|
||||
// Validate authority: owner's instance for kicks, any instance for self-leave.
|
||||
//
|
||||
// `sourceInstance` arrives as a full URL from `federationWorker.ts` (always
|
||||
// `getOurOrigin()` on the sender). `channel.ownerHomeInstance`, however, can be
|
||||
// stored either as a bare host (from `users.homeInstance`, written by
|
||||
// `resolveOrCreateReplicatedUser` and by group DM ownership transfers to a
|
||||
// federated user) OR as a full URL (group DM creation / transfers to a local
|
||||
// user, which fall back to `domainOrigin = getOurOrigin()`). Strict equality
|
||||
// here mis-fires for the bare-vs-full mismatch — see the historical bug entry
|
||||
// in `docs/systems/dm-system.md`. Always compare through
|
||||
// `normalizeOriginForCompare`, matching the established pattern for federation
|
||||
// authority checks.
|
||||
if (event.membership.reason !== 'leave' && channel.ownerHomeInstance &&
|
||||
normalizeOriginForCompare(sourceInstance) !== normalizeOriginForCompare(channel.ownerHomeInstance)) {
|
||||
rejected.push({ messageId: event.messageId, reason: 'unauthorized_source' });
|
||||
return;
|
||||
}
|
||||
@@ -4456,7 +4471,7 @@ export function processMemberRemoveEvent(
|
||||
accepted.push(event.messageId);
|
||||
}
|
||||
|
||||
function processOwnershipTransferEvent(
|
||||
export function processOwnershipTransferEvent(
|
||||
event: FederationRelayEvent,
|
||||
sourceInstance: string,
|
||||
db: ReturnType<typeof getDb>,
|
||||
@@ -4502,8 +4517,14 @@ function processOwnershipTransferEvent(
|
||||
return;
|
||||
}
|
||||
|
||||
// Validate authority: only the current owner's instance can transfer ownership
|
||||
if (channel.ownerHomeInstance && sourceInstance !== channel.ownerHomeInstance) {
|
||||
// Validate authority: only the current owner's instance can transfer ownership.
|
||||
//
|
||||
// See the matching note in `processMemberRemoveEvent`: `sourceInstance` is
|
||||
// always a full URL but `channel.ownerHomeInstance` can be bare or full.
|
||||
// Normalize both sides through `normalizeOriginForCompare` so we don't reject
|
||||
// legitimate back-and-forth transfers that wrote a bare host into the column.
|
||||
if (channel.ownerHomeInstance &&
|
||||
normalizeOriginForCompare(sourceInstance) !== normalizeOriginForCompare(channel.ownerHomeInstance)) {
|
||||
rejected.push({ messageId: event.messageId, reason: 'unauthorized_source' });
|
||||
return;
|
||||
}
|
||||
@@ -4522,11 +4543,19 @@ function processOwnershipTransferEvent(
|
||||
return;
|
||||
}
|
||||
|
||||
// Canonicalize to a full origin URL on storage so future authority checks
|
||||
// can compare cleanly against `sourceInstance` (also a full URL). Mirrors
|
||||
// the canonicalization performed in `transferGroupDmOwnership` on the
|
||||
// sender side. Falls back to the wire value if normalization yields null
|
||||
// (shouldn't happen for valid events; defensive).
|
||||
const canonicalOwnerHome =
|
||||
canonicalizeHomeInstance(event.ownership.newOwner.homeInstance) ?? event.ownership.newOwner.homeInstance;
|
||||
|
||||
db.update(schema.dmChannels)
|
||||
.set({
|
||||
ownerId: newOwnerLocal.id,
|
||||
ownerHomeUserId: event.ownership.newOwner.homeUserId,
|
||||
ownerHomeInstance: event.ownership.newOwner.homeInstance,
|
||||
ownerHomeInstance: canonicalOwnerHome,
|
||||
})
|
||||
.where(eq(schema.dmChannels.id, channel.id))
|
||||
.run();
|
||||
@@ -4535,6 +4564,8 @@ function processOwnershipTransferEvent(
|
||||
type: 'dm_owner_updated',
|
||||
dmChannelId: channel.id,
|
||||
newOwnerId: newOwnerLocal.id,
|
||||
newOwnerHomeUserId: event.ownership.newOwner.homeUserId,
|
||||
newOwnerHomeInstance: canonicalOwnerHome,
|
||||
});
|
||||
|
||||
const prevOwnerLocal = event.ownership.previousOwner
|
||||
|
||||
Reference in New Issue
Block a user