diff --git a/docs/systems/dm-system.md b/docs/systems/dm-system.md index ccbdced3..6b66b7bc 100644 --- a/docs/systems/dm-system.md +++ b/docs/systems/dm-system.md @@ -759,7 +759,7 @@ Returns the channel's `ownerHomeInstance`. Used by all owner-only DM operations | `dm_channel_updated` | Call `updateDmMetadata(dmChannelId, { name, icon })` | | `dm_member_added` | Normalize remote user assets, upsert into `userViews`, call `addDmMember(dmChannelId, user)` | | `dm_member_removed` | Call `removeDmMember(dmChannelId, userId)` | -| `dm_owner_updated` | Call `updateDmOwner(dmChannelId, newOwnerId)` | +| `dm_owner_updated` | Call `updateDmOwner(dmChannelId, newOwnerId, newOwnerHomeUserId?, newOwnerHomeInstance?)` — the optional home-identity fields keep the channel's federation routing cache fresh after a manual transfer | | `dm_message_created` / `dm_message_updated` | Normalize message assets, upsert `message.user` and `message.replyTo?.user` into `userViews` | ### New DM Modal (`NewDmModal.tsx`) @@ -869,7 +869,7 @@ For full wire formats, see `docs/systems/websocket.md`. | `dm_channel_updated` | S->C | Group metadata (`name`/`icon`) updated; payload `{ dmChannelId, name, icon }` (no `metadataUpdatedAt` — server-side version vector only) | | `dm_member_added` | S->C | Incremental member add (not bootstrap) | | `dm_member_removed` | S->C | Member leave/kick | -| `dm_owner_updated` | S->C | Ownership transfer (auto on owner-leave OR manual via `POST /api/dm/:id/transfer`) | +| `dm_owner_updated` | S->C | Ownership transfer (auto on owner-leave OR manual via `POST /api/dm/:id/transfer`). Payload: `{ dmChannelId, newOwnerId, newOwnerHomeUserId?, newOwnerHomeInstance? }` — the home-identity fields are populated on every new emission so the client can keep `dmChannel.ownerHomeInstance` (and thus `getOwnerInstanceForDm` routing) in sync without waiting for a `ready` refresh. Receivers tolerate omission for legacy senders. | ### Content Events @@ -895,3 +895,4 @@ For full wire formats, see `docs/systems/websocket.md`. | Raw JSON in DM sidebar previews | DM sidebar showed `{"event":"space_invite",...}` / `{"event":"member_added",...}` as the last-message preview | `DmLastMessagePreview` shape omitted `type`, so the client could not distinguish system from user messages and rendered `lastMessage.content` verbatim. | Added `type` to `DmLastMessagePreview`, populated it from `dm_messages.type` in every server emission site, and routed the sidebar through a single `formatDmSidebarPreview` helper that renders human-readable text for each system event. | | Owner-only requests routed to wrong instance after manual transfer (latent) | After `POST /api/dm/:id/transfer` moved ownership to a member whose `homeInstance` differed from the channel's pinned serving origin, owner-only client calls (`updateMetadata`, `kickMember`, `transferOwnership`) routed via `getChannelOrigin` would emit outbox events with `sourceInstance !== ownerHomeInstance`, and all peers would reject them as `attribution_mismatch`. Latent only because pre-polish there was no kick endpoint and no metadata edit; auto-transfer-on-leave masked the issue (the leaver IS the actor, and `member_remove reason='leave'` accepts any source). | Added `getOwnerInstanceForDm(channelId)` exported next to `getChannelOrigin`. All four owner-only API client methods (`updateMetadata`, `kickMember`, `transferOwnership` — and any future owner-only routes) call `getApiForOrigin(getOwnerInstanceForDm(channelId))` instead of channel origin. Non-owner operations are unchanged. | | Kick / transfer to federated member always failed with "user not a member" | `DELETE /api/dm/:id/members/:targetUserId` and `POST /api/dm/:id/transfer` accepted only a local user id. The client passed `canonical.id` from `useCanonicalUserView`, which returns the user's HOME id when the home view is cached. After owner-routing the request to the owner instance, the owner instance's `dm_members.userId` (its own local replicated id) never matched the home id, so `isDmMember` returned false. | Both endpoints now accept federated identification (`homeUserId` + `homeInstance`) — the transfer endpoint takes them in the body, the kick endpoint reads `homeInstance` from a query string and treats the URL segment as a homeUserId. Server resolves via `resolveOrCreateReplicatedUser` before membership check. Mirrors the `addDmMember` pattern. Client `kickMember` / `transferOwnership` accept an optional `federated` arg and pass it when the target has `homeUserId` + `homeInstance` populated. | +| Ownership transfer back-and-forth diverged between instances | After A→B transfer succeeded, B→A was applied locally but rejected by A's peer with `unauthorized_source`; ownership permanently disagreed between instances. Compounded by the client never updating its in-memory `dmChannel.ownerHomeInstance` from the `dm_owner_updated` WS event, so `getOwnerInstanceForDm` returned the previous owner's origin after the WS broadcast (relevant only if the same session re-attempts an owner-only op). | Two compounding bugs: (1) `transferGroupDmOwnership` wrote `users.homeInstance` verbatim into `dm_channels.ownerHomeInstance` — a BARE host (`orbit.ddns.net`) for federated owners. (2) `processOwnershipTransferEvent` (and `processMemberRemoveEvent` for kicks) compared `sourceInstance` (always full URL) to `channel.ownerHomeInstance` with strict string equality, mis-firing on the bare-vs-full mismatch. (3) `dm_owner_updated` WS event omitted `newOwnerHomeUserId` / `newOwnerHomeInstance`, so the client couldn't refresh its routing cache after a successful transfer. | (a) Both authority checks now compare via `normalizeOriginForCompare`. (b) Every write site that persists `dm_channels.ownerHomeInstance` (`transferGroupDmOwnership`, `POST /api/dm/group` post-create federation, lazy federation in `POST /api/dm/:id/members`, `processMemberAddEvent` bootstrap, `processOwnershipTransferEvent` receiver storage) canonicalizes through a new `canonicalizeHomeInstance` helper in `federationAuth.ts` — full URL is the canonical storage form, matching how `sourceInstance` always arrives. (c) `dm_owner_updated` WS event was extended with optional `newOwnerHomeUserId` and `newOwnerHomeInstance` fields, and the client `updateDmOwner` action writes them when present (guarding against legacy senders by leaving the existing values untouched if omitted). | diff --git a/docs/systems/federation.md b/docs/systems/federation.md index 6cfd3a67..9def1a02 100644 --- a/docs/systems/federation.md +++ b/docs/systems/federation.md @@ -803,10 +803,10 @@ Older peers that omit these fields fall back to safe defaults (null name/icon, ` ### ownership_transfer (`processOwnershipTransferEvent` -- `federation.ts:1938`) 1. Find channel by `federatedId` -- if not found, accept idempotently -2. Validate authority: `sourceInstance === channel.ownerHomeInstance` +2. Validate authority: `normalizeOriginForCompare(sourceInstance) === normalizeOriginForCompare(channel.ownerHomeInstance)`. Both sides are normalized to handle the bare-vs-full storage convention (see `dm-system.md` historical bugs for why this matters). 3. Resolve new owner via `resolveOrCreateReplicatedUser` (**never** `resolveLocalUser` -- must guarantee valid ID) -4. Update `dm_channels`: `ownerId`, `ownerHomeUserId`, `ownerHomeInstance` -5. Broadcast `dm_owner_updated` WebSocket event +4. Update `dm_channels`: `ownerId`, `ownerHomeUserId`, `ownerHomeInstance` (canonicalized to full URL on storage via `canonicalizeHomeInstance` so future authority checks stay stable) +5. Broadcast `dm_owner_updated` WebSocket event with `newOwnerHomeUserId` + `newOwnerHomeInstance` so local clients can refresh their owner-routing cache without reconnecting 6. Insert system message with previous owner as actor Triggered by both auto-transfer-on-leave and the manual `POST /api/dm/:id/transfer` endpoint — the receiver path is the same. diff --git a/docs/systems/websocket.md b/docs/systems/websocket.md index 456daa57..70394d17 100644 --- a/docs/systems/websocket.md +++ b/docs/systems/websocket.md @@ -149,7 +149,7 @@ Source: `packages/server/src/ws/handler.ts`, `packages/server/src/ws/events.ts` | `dm_channel_closed` | dmChannelId | user | | `dm_member_added` | dmChannelId, user | DM members | | `dm_member_removed` | dmChannelId, userId | DM members | -| `dm_owner_updated` | dmChannelId, newOwnerId | DM members | +| `dm_owner_updated` | dmChannelId, newOwnerId, newOwnerHomeUserId?, newOwnerHomeInstance? | DM members | ### Voice | type | fields | scope | diff --git a/packages/server/src/routes/dm.ts b/packages/server/src/routes/dm.ts index 853f40a4..7da3e8ff 100644 --- a/packages/server/src/routes/dm.ts +++ b/packages/server/src/routes/dm.ts @@ -46,7 +46,7 @@ import { sendTypingRelay, normalizeIconForWire, } from '../utils/federationOutbox.js'; -import { getOurOrigin } from '../utils/federationAuth.js'; +import { getOurOrigin, canonicalizeHomeInstance } from '../utils/federationAuth.js'; import type { FederationRelayEvent } from '@backspace/shared'; import { resolveLocalUser, resolveOrCreateReplicatedUser } from './federation.js'; @@ -510,7 +510,16 @@ function transferGroupDmOwnership( const domainOrigin = isFederationRelayEnabled() ? getOurOrigin() : null; const newOwnerHomeUserId = newOwnerRow?.homeUserId || newOwnerId; - const newOwnerHomeInstance = newOwnerRow?.homeInstance || domainOrigin || ''; + // Canonicalize to a full origin URL. `users.homeInstance` is stored as a + // bare host (e.g. `orbit.ddns.net`) for federated users, but + // `dm_channels.ownerHomeInstance` is compared against `sourceInstance` + // (always a full URL) in S2S authority checks. Storing the bare form here + // caused legitimate `ownership_transfer` events to be rejected with + // `unauthorized_source` after back-and-forth transfers — see the historical + // bug entry in `docs/systems/dm-system.md`. + const newOwnerHomeInstance = canonicalizeHomeInstance( + newOwnerRow?.homeInstance || domainOrigin || '', + ); const ownerSysMsgId = generateSnowflake(); const ownerNow = Date.now(); @@ -520,6 +529,11 @@ function transferGroupDmOwnership( newOwnerDisplayName, }); + // Wire homeInstance values are canonicalized to full URLs so receivers store + // the canonical form too. Future authority checks then compare full-URL to + // full-URL without needing defensive normalization at every site. + const previousOwnerHomeInstanceWire = + canonicalizeHomeInstance(previousOwnerRow?.homeInstance || domainOrigin || '') ?? ''; const transferPayload: FederationRelayEvent | null = federationActive ? { eventType: 'ownership_transfer', @@ -531,11 +545,11 @@ function transferGroupDmOwnership( ownership: { newOwner: { homeUserId: newOwnerHomeUserId, - homeInstance: newOwnerHomeInstance || (domainOrigin ?? ''), + homeInstance: newOwnerHomeInstance ?? (domainOrigin ?? ''), }, previousOwner: { homeUserId: previousOwnerRow?.homeUserId || previousOwnerId, - homeInstance: previousOwnerRow?.homeInstance || (domainOrigin ?? ''), + homeInstance: previousOwnerHomeInstanceWire, }, }, } @@ -584,12 +598,17 @@ function transferGroupDmOwnership( .where(eq(schema.dmMembers.dmChannelId, channelId)) .all(); - // Broadcast dm_owner_updated to local members. + // Broadcast dm_owner_updated to local members. Include the new owner's + // home identity so receiving clients can update `dm.ownerHomeInstance` + // (and thus keep `getOwnerInstanceForDm` correct for the next owner-only + // request) without waiting for a fresh `ready` payload on reconnect. for (const member of members) { connectionManager.sendToUser(member.userId, { type: 'dm_owner_updated', dmChannelId: channelId, newOwnerId, + newOwnerHomeUserId, + newOwnerHomeInstance: newOwnerHomeInstance ?? null, }); } @@ -1211,7 +1230,9 @@ export async function dmRoutes(app: FastifyInstance): Promise { .set({ federatedId, ownerHomeUserId: callerUser?.homeUserId || request.userId, - ownerHomeInstance: callerUser?.homeInstance || domainOrigin, + // Canonicalize for federation-authority parity (see + // `transferGroupDmOwnership` for the full rationale). + ownerHomeInstance: canonicalizeHomeInstance(callerUser?.homeInstance || domainOrigin), }) .where(eq(schema.dmChannels.id, dmChannelId)) .run(); @@ -1754,7 +1775,9 @@ export async function dmRoutes(app: FastifyInstance): Promise { .set({ federatedId: newFederatedId, ownerHomeUserId: ownerUser?.homeUserId || dmChannel.ownerId!, - ownerHomeInstance: ownerUser?.homeInstance || domainOrigin, + // Canonicalize for federation-authority parity (see + // `transferGroupDmOwnership` for the full rationale). + ownerHomeInstance: canonicalizeHomeInstance(ownerUser?.homeInstance || domainOrigin), }) .where(eq(schema.dmChannels.id, id)) .run(); @@ -1763,7 +1786,7 @@ export async function dmRoutes(app: FastifyInstance): Promise { ...dmChannel, federatedId: newFederatedId, ownerHomeUserId: ownerUser?.homeUserId || dmChannel.ownerId!, - ownerHomeInstance: ownerUser?.homeInstance || domainOrigin, + ownerHomeInstance: canonicalizeHomeInstance(ownerUser?.homeInstance || domainOrigin), }; } } diff --git a/packages/server/src/routes/federation.kick.test.ts b/packages/server/src/routes/federation.kick.test.ts index a13d0c8e..cbcc70b8 100644 --- a/packages/server/src/routes/federation.kick.test.ts +++ b/packages/server/src/routes/federation.kick.test.ts @@ -287,6 +287,40 @@ describe('processMemberRemoveEvent — kick authority', () => { expect(vi.mocked(connectionManager.sendToDmMembers)).not.toHaveBeenCalled(); }); + it('accepts a kick when ownerHomeInstance is BARE and sourceInstance is FULL (bare-vs-full normalization)', async () => { + // Regression: pre-fix, `processMemberRemoveEvent` compared + // `sourceInstance` (full URL, from outbox worker) to `ownerHomeInstance` + // verbatim. After an `ownership_transfer` to a federated user, the + // column would be written as a bare host (`users.homeInstance`), causing + // legitimate downstream kicks to be rejected with `unauthorized_source`. + // + // The fix normalizes both sides via `normalizeOriginForCompare`. This + // test re-seeds the channel with a bare `ownerHomeInstance` to lock in + // the new behavior. Mirrors the ownership-transfer authority test. + seedChannelAndMembers(); + // Overwrite ownerHomeInstance to the legacy bare form. + testDb.update(schema.dmChannels) + .set({ ownerHomeInstance: 'owner.test' }) + .where(eq(schema.dmChannels.id, CHANNEL_ID)) + .run(); + + const fed = await import('./federation.js'); + const event = buildKickEvent('evt-kick-bare-owner'); + + const accepted: string[] = []; + const rejected: Array<{ messageId: string; reason: string }> = []; + fed.processMemberRemoveEvent(event, OWNER_ORIGIN, testDb, accepted, rejected); + + expect(rejected).toEqual([]); + expect(accepted).toEqual([event.messageId]); + + // Victim's dm_members row was deleted (kick applied) + const victimRow = testDb.select().from(schema.dmMembers) + .where(and(eq(schema.dmMembers.dmChannelId, CHANNEL_ID), eq(schema.dmMembers.userId, VICTIM_USER_ID))) + .get(); + expect(victimRow).toBeUndefined(); + }); + it('accepts a self-leave from any source instance (not the owner instance) and removes the leaver', async () => { seedChannelAndMembers(); const fed = await import('./federation.js'); diff --git a/packages/server/src/routes/federation.ownershipTransfer.test.ts b/packages/server/src/routes/federation.ownershipTransfer.test.ts new file mode 100644 index 00000000..e11793de --- /dev/null +++ b/packages/server/src/routes/federation.ownershipTransfer.test.ts @@ -0,0 +1,352 @@ +// Receiver-side authority and storage tests for `processOwnershipTransferEvent`. +// +// The headline regression these tests pin down: +// +// On a federated back-and-forth — A transfers ownership to B (federated), B +// transfers it back to A — the second event was rejected at the receiver +// with `unauthorized_source` because `dm_channels.ownerHomeInstance` was +// written as a bare host (`orbit.ddns.net`) by +// `transferGroupDmOwnership` (which copies `users.homeInstance`) while +// `sourceInstance` always arrives as a full URL (`https://orbit.ddns.net`). +// The strict-string-equality check fired, the event went back into the +// outbox, and every retry hit the same mismatch — divergent ownership +// between the two instances stuck until manual repair. +// +// The fix is two-fold: +// +// 1. Authority check normalizes both sides via `normalizeOriginForCompare` +// so legacy bare-vs-full rows accept legitimate transfers. +// 2. Both the sender (`transferGroupDmOwnership`) and the receiver +// (`processOwnershipTransferEvent`) canonicalize `ownerHomeInstance` to a +// full origin URL on storage, so going forward the column is uniform. +// +// These tests cover the receiver — sender canonicalization is covered by the +// existing `dm.transfer.test.ts`. +import { describe, it, expect, beforeEach, vi } from 'vitest'; +import Database from 'better-sqlite3'; +import { drizzle } from 'drizzle-orm/better-sqlite3'; +import fs from 'node:fs'; +import path from 'node:path'; +import { fileURLToPath } from 'node:url'; +import { eq } from 'drizzle-orm'; +import * as schema from '../db/schema.js'; +import { setWorkerId } from '../utils/snowflake.js'; +import type { FederationRelayEvent } from '@backspace/shared'; + +setWorkerId(5); + +const __dirname = path.dirname(fileURLToPath(import.meta.url)); + +type TestDb = ReturnType>; +let sqlite: Database.Database; +let testDb: TestDb; + +vi.mock('../db/index.js', () => ({ + getDb: () => testDb, + getRawDb: () => sqlite, + schema, +})); + +vi.mock('../config.js', () => ({ + config: { + domain: 'local.test', + port: 3000, + host: '0.0.0.0', + jwtSecret: 'test-secret-12345678901234567890123456789012', + maxUploadSize: 100 * 1024 * 1024, + registrationOpen: true, + uploadDir: '/tmp/bs-fed-transfer-test', + }, +})); + +vi.mock('../ws/handler.js', () => ({ + connectionManager: { + sendToUser: vi.fn(), + sendToSpace: vi.fn(), + sendToDmMembers: vi.fn(), + sendToAdmins: vi.fn(), + getAllOnlineUserIds: () => [], + evictFederatedCallsForHost: vi.fn(), + federatedCalls: new Map(), + isUserOnline: vi.fn(), + lateBindFederatedCall: vi.fn(), + }, +})); + +import { connectionManager } from '../ws/handler.js'; + +function applyMigrations(db: Database.Database): void { + const migrationsDir = path.resolve(__dirname, '../../drizzle'); + const files = fs.readdirSync(migrationsDir).filter(f => f.endsWith('.sql')).sort(); + for (const f of files) { + const sql = fs.readFileSync(path.join(migrationsDir, f), 'utf8'); + for (const stmt of sql.split(/-->\s*statement-breakpoint/)) { + const clean = stmt.trim(); + if (clean) db.exec(clean); + } + } +} + +// Identities used by these tests. Owner is on `owner.test`, new owner is on +// `new.test`. The previous owner / source for the transfer event is the owner. +const OWNER_ORIGIN_FULL = 'https://owner.test'; +const OWNER_ORIGIN_BARE = 'owner.test'; +const NEW_OWNER_ORIGIN_FULL = 'https://new.test'; +const NEW_OWNER_ORIGIN_BARE = 'new.test'; +const FEDERATED_ID = 'fed-transfer-1'; +const CHANNEL_ID = 'ch-transfer-local-1'; + +const OWNER_USER_ID = 'owner-user-stub'; +const OWNER_HOME_USER_ID = 'home-owner-1'; + +const NEW_OWNER_USER_ID = 'new-owner-stub'; +const NEW_OWNER_HOME_USER_ID = 'home-new-owner-1'; + +function seedUsers(): void { + const now = Date.now(); + testDb.insert(schema.users).values({ + id: OWNER_USER_ID, + username: 'owner@owner.test', + displayName: 'owner', + passwordHash: '!federation-replicated', + status: 'offline', + isAdmin: 0, + homeInstance: OWNER_ORIGIN_BARE, + homeUserId: OWNER_HOME_USER_ID, + createdAt: now, + }).run(); + + testDb.insert(schema.users).values({ + id: NEW_OWNER_USER_ID, + username: 'new@new.test', + displayName: 'new owner', + passwordHash: '!federation-replicated', + status: 'offline', + isAdmin: 0, + homeInstance: NEW_OWNER_ORIGIN_BARE, + homeUserId: NEW_OWNER_HOME_USER_ID, + createdAt: now, + }).run(); +} + +interface SeedOpts { + ownerHomeInstance: string; +} + +function seedChannel(opts: SeedOpts): void { + const now = Date.now(); + testDb.insert(schema.dmChannels).values({ + id: CHANNEL_ID, + federatedId: FEDERATED_ID, + ownerId: OWNER_USER_ID, + ownerHomeUserId: OWNER_HOME_USER_ID, + ownerHomeInstance: opts.ownerHomeInstance, + name: 'group', + icon: null, + metadataUpdatedAt: 1000, + createdAt: now, + }).run(); + testDb.insert(schema.dmMembers).values({ dmChannelId: CHANNEL_ID, userId: OWNER_USER_ID, closed: 0 }).run(); + testDb.insert(schema.dmMembers).values({ dmChannelId: CHANNEL_ID, userId: NEW_OWNER_USER_ID, closed: 0 }).run(); +} + +function buildTransferEvent(opts: { + messageId?: string; + newOwnerHomeInstance?: string; + previousOwnerHomeInstance?: string; +} = {}): FederationRelayEvent { + return { + eventType: 'ownership_transfer', + contextType: 'dm', + messageId: opts.messageId ?? 'evt-transfer-1', + federatedId: FEDERATED_ID, + encryptionVersion: 0, + timestamp: Date.now(), + ownership: { + newOwner: { + homeUserId: NEW_OWNER_HOME_USER_ID, + homeInstance: opts.newOwnerHomeInstance ?? NEW_OWNER_ORIGIN_FULL, + }, + previousOwner: { + homeUserId: OWNER_HOME_USER_ID, + homeInstance: opts.previousOwnerHomeInstance ?? OWNER_ORIGIN_FULL, + }, + }, + }; +} + +beforeEach(() => { + sqlite = new Database(':memory:'); + testDb = drizzle(sqlite, { schema }); + applyMigrations(sqlite); + seedUsers(); + vi.mocked(connectionManager.sendToDmMembers).mockReset(); +}); + +describe('processOwnershipTransferEvent — authority + canonicalization', () => { + it('accepts a transfer when ownerHomeInstance is BARE and sourceInstance is FULL (legacy storage)', async () => { + // Legacy state — `transferGroupDmOwnership` used to copy `users.homeInstance` + // verbatim (bare host) for federated new owners. With the fix, this + // pre-existing row must still accept legitimate inbound transfers. + seedChannel({ ownerHomeInstance: OWNER_ORIGIN_BARE }); + const fed = await import('./federation.js'); + const event = buildTransferEvent(); + + const accepted: string[] = []; + const rejected: Array<{ messageId: string; reason: string }> = []; + fed.processOwnershipTransferEvent(event, OWNER_ORIGIN_FULL, testDb, accepted, rejected); + + expect(rejected).toEqual([]); + expect(accepted).toEqual([event.messageId]); + + const channel = testDb.select().from(schema.dmChannels).where(eq(schema.dmChannels.id, CHANNEL_ID)).get(); + expect(channel?.ownerId).toBe(NEW_OWNER_USER_ID); + expect(channel?.ownerHomeUserId).toBe(NEW_OWNER_HOME_USER_ID); + // Receiver canonicalizes to full URL on storage so future authority + // checks are stable regardless of what the wire format was. + expect(channel?.ownerHomeInstance).toBe(NEW_OWNER_ORIGIN_FULL); + }); + + it('accepts a transfer when ownerHomeInstance is FULL and sourceInstance is FULL (current happy path)', async () => { + seedChannel({ ownerHomeInstance: OWNER_ORIGIN_FULL }); + const fed = await import('./federation.js'); + const event = buildTransferEvent(); + + const accepted: string[] = []; + const rejected: Array<{ messageId: string; reason: string }> = []; + fed.processOwnershipTransferEvent(event, OWNER_ORIGIN_FULL, testDb, accepted, rejected); + + expect(rejected).toEqual([]); + expect(accepted).toEqual([event.messageId]); + const channel = testDb.select().from(schema.dmChannels).where(eq(schema.dmChannels.id, CHANNEL_ID)).get(); + expect(channel?.ownerId).toBe(NEW_OWNER_USER_ID); + expect(channel?.ownerHomeInstance).toBe(NEW_OWNER_ORIGIN_FULL); + }); + + it('rejects a transfer when the source peer cannot attest the previous owner (attribution_mismatch)', async () => { + // First-line attribution check: `previousOwner.homeInstance` must match + // `sourceInstance` (or be us). An attacker peer cannot attest a transfer + // on behalf of a user whose home isn't them. + seedChannel({ ownerHomeInstance: OWNER_ORIGIN_FULL }); + const fed = await import('./federation.js'); + const event = buildTransferEvent({ messageId: 'evt-transfer-bad-source' }); + + const accepted: string[] = []; + const rejected: Array<{ messageId: string; reason: string }> = []; + fed.processOwnershipTransferEvent(event, 'https://attacker.test', testDb, accepted, rejected); + + expect(accepted).toEqual([]); + expect(rejected).toEqual([{ messageId: event.messageId, reason: 'attribution_mismatch' }]); + + // No mutation + const channel = testDb.select().from(schema.dmChannels).where(eq(schema.dmChannels.id, CHANNEL_ID)).get(); + expect(channel?.ownerId).toBe(OWNER_USER_ID); + expect(channel?.ownerHomeInstance).toBe(OWNER_ORIGIN_FULL); + }); + + it('rejects a transfer when the source matches previousOwner but is NOT the channel\'s current owner instance (unauthorized_source)', async () => { + // Authority check at the channel level: even if a peer can attest the + // previous owner, the channel's current `ownerHomeInstance` must still + // match the source. This guards against an outdated peer trying to + // forward an old transfer after ownership has moved on. + // + // Setup: channel currently owned by `new.test` (after some other prior + // transfer this receiver already applied). An event arrives FROM + // `owner.test` claiming `previousOwner` is on `owner.test`. Attribution + // is fine (source attests its own user), but the channel says the + // current authority is `new.test` — reject as `unauthorized_source`. + seedChannel({ ownerHomeInstance: NEW_OWNER_ORIGIN_FULL }); + const fed = await import('./federation.js'); + const event = buildTransferEvent({ + messageId: 'evt-transfer-stale-source', + previousOwnerHomeInstance: OWNER_ORIGIN_FULL, + }); + + const accepted: string[] = []; + const rejected: Array<{ messageId: string; reason: string }> = []; + fed.processOwnershipTransferEvent(event, OWNER_ORIGIN_FULL, testDb, accepted, rejected); + + expect(accepted).toEqual([]); + expect(rejected).toEqual([{ messageId: event.messageId, reason: 'unauthorized_source' }]); + }); + + it('canonicalizes ownerHomeInstance on storage even when the wire payload sends a bare host', async () => { + // Defensive: a legacy peer may still send the bare host on the wire after + // an upgrade. Receiver storage must end up canonical regardless. + seedChannel({ ownerHomeInstance: OWNER_ORIGIN_FULL }); + const fed = await import('./federation.js'); + const event = buildTransferEvent({ + messageId: 'evt-transfer-bare-wire', + newOwnerHomeInstance: NEW_OWNER_ORIGIN_BARE, + }); + + const accepted: string[] = []; + const rejected: Array<{ messageId: string; reason: string }> = []; + fed.processOwnershipTransferEvent(event, OWNER_ORIGIN_FULL, testDb, accepted, rejected); + + expect(rejected).toEqual([]); + expect(accepted).toEqual([event.messageId]); + const channel = testDb.select().from(schema.dmChannels).where(eq(schema.dmChannels.id, CHANNEL_ID)).get(); + expect(channel?.ownerHomeInstance).toBe(NEW_OWNER_ORIGIN_FULL); + }); + + it('broadcasts dm_owner_updated with newOwnerHomeUserId + newOwnerHomeInstance so clients keep owner-routing fresh', async () => { + seedChannel({ ownerHomeInstance: OWNER_ORIGIN_FULL }); + const fed = await import('./federation.js'); + const event = buildTransferEvent({ messageId: 'evt-transfer-broadcast' }); + + const accepted: string[] = []; + const rejected: Array<{ messageId: string; reason: string }> = []; + fed.processOwnershipTransferEvent(event, OWNER_ORIGIN_FULL, testDb, accepted, rejected); + + expect(rejected).toEqual([]); + const sendSpy = vi.mocked(connectionManager.sendToDmMembers); + const ownerUpdatedCalls = sendSpy.mock.calls.filter(c => (c[1] as { type?: string }).type === 'dm_owner_updated'); + expect(ownerUpdatedCalls).toHaveLength(1); + const payload = ownerUpdatedCalls[0]![1] as { + type: string; + dmChannelId: string; + newOwnerId: string; + newOwnerHomeUserId?: string | null; + newOwnerHomeInstance?: string | null; + }; + expect(payload).toMatchObject({ + type: 'dm_owner_updated', + dmChannelId: CHANNEL_ID, + newOwnerId: NEW_OWNER_USER_ID, + newOwnerHomeUserId: NEW_OWNER_HOME_USER_ID, + newOwnerHomeInstance: NEW_OWNER_ORIGIN_FULL, + }); + }); + + it('is idempotent on replay — repeats short-circuit at the (sourceInstance, messageId) dedup', async () => { + seedChannel({ ownerHomeInstance: OWNER_ORIGIN_BARE }); + const fed = await import('./federation.js'); + const event = buildTransferEvent({ messageId: 'evt-transfer-replay' }); + + const accepted1: string[] = []; + const rejected1: Array<{ messageId: string; reason: string }> = []; + fed.processOwnershipTransferEvent(event, OWNER_ORIGIN_FULL, testDb, accepted1, rejected1); + + // Second delivery — the new ownerHomeInstance on the channel is now + // `new.test`; running the same event again must NOT clobber owner back + // because the dedup short-circuits. + const accepted2: string[] = []; + const rejected2: Array<{ messageId: string; reason: string }> = []; + fed.processOwnershipTransferEvent(event, OWNER_ORIGIN_FULL, testDb, accepted2, rejected2); + + expect(accepted1).toEqual([event.messageId]); + expect(accepted2).toEqual([event.messageId]); + expect(rejected1).toEqual([]); + expect(rejected2).toEqual([]); + + const channel = testDb.select().from(schema.dmChannels).where(eq(schema.dmChannels.id, CHANNEL_ID)).get(); + expect(channel?.ownerId).toBe(NEW_OWNER_USER_ID); + + // Only one system message inserted across two deliveries + const sysRows = testDb.select().from(schema.dmMessages) + .where(eq(schema.dmMessages.dmChannelId, CHANNEL_ID)) + .all(); + expect(sysRows.filter(r => r.sourceMessageId === event.messageId)).toHaveLength(1); + }); +}); diff --git a/packages/server/src/routes/federation.ts b/packages/server/src/routes/federation.ts index ec8c4a9d..eac4a29a 100644 --- a/packages/server/src/routes/federation.ts +++ b/packages/server/src/routes/federation.ts @@ -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, @@ -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 diff --git a/packages/server/src/utils/federationAuth.ts b/packages/server/src/utils/federationAuth.ts index 50580089..39a1e6d9 100644 --- a/packages/server/src/utils/federationAuth.ts +++ b/packages/server/src/utils/federationAuth.ts @@ -222,3 +222,28 @@ export function normalizeOriginForCompare(value: string | null | undefined): str if (!s) return null; return s.toLowerCase(); } + +/** + * Canonicalize a homeInstance value to a full origin URL + * (e.g. `https://nova.ddns.net`) for storage on columns whose authority + * checks compare against `sourceInstance` (which is always a full URL). + * + * Accepts bare host (`nova.ddns.net`), scheme-prefixed (`https://...`), or + * `null` / empty / whitespace (returns `null`). `localhost`-style values keep + * `http://` if already present; otherwise the canonical default is `https://`. + * + * Use this at every write site that persists `dm_channels.ownerHomeInstance` + * so that S2S authority checks aren't broken by a bare-vs-full mismatch. + * Read paths should still normalize via `normalizeOriginForCompare` for + * defensive parity with legacy rows. + */ +export function canonicalizeHomeInstance(value: string | null | undefined): string | null { + if (!value) return null; + const s = value.trim(); + if (!s) return null; + if (/^https?:\/\//i.test(s)) { + // Strip trailing slashes only — preserve the explicit scheme. + return s.replace(/\/+$/, ''); + } + return `https://${s.replace(/\/+$/, '')}`; +} diff --git a/packages/shared/src/types.ts b/packages/shared/src/types.ts index 1f8a60e9..5fc261bd 100644 --- a/packages/shared/src/types.ts +++ b/packages/shared/src/types.ts @@ -481,7 +481,20 @@ export type ServerEvent = | { type: 'federation_approval_request_received'; origin: string; instanceName?: string } | { type: 'peering_subscription_changed' } | { type: 'peering_notification_received'; kind: PeeringNotificationKind } - | { type: 'dm_owner_updated'; dmChannelId: string; newOwnerId: string } + | { + type: 'dm_owner_updated'; + dmChannelId: string; + newOwnerId: string; + // Federation routing fields. Required for the client to keep + // `dmChannel.ownerHomeInstance` in sync after a transfer — otherwise + // owner-only API calls (`updateMetadata`, `kickMember`, `transferOwnership`) + // continue to route through the previous owner's home instance via + // `getOwnerInstanceForDm` until the user reconnects and receives a fresh + // `ready` payload. Always populated on new emissions; tolerated as + // optional for receivers connected to an older sender. + newOwnerHomeUserId?: string | null; + newOwnerHomeInstance?: string | null; + } | { type: 'pong' } | { type: 'error'; message: string }; diff --git a/packages/web/src/hooks/useWebSocket.ts b/packages/web/src/hooks/useWebSocket.ts index f2f50427..7cd57db3 100644 --- a/packages/web/src/hooks/useWebSocket.ts +++ b/packages/web/src/hooks/useWebSocket.ts @@ -1113,7 +1113,18 @@ function handleEvent(origin: string, event: ServerEvent): void { case 'dm_owner_updated': { if (!isHome && !activePeerOrigins.has(origin)) break; const { updateDmOwner } = useSpaceStore.getState(); - updateDmOwner(event.dmChannelId, event.newOwnerId); + // Pass the federation routing fields so the DM's `ownerHomeInstance` + // stays in sync with the server. Without this, `getOwnerInstanceForDm` + // routes the next owner-only API call (rename, icon, kick, transfer) + // through the PREVIOUS owner's home instance and the receiving peer + // rejects it as `unauthorized_source`. Older servers omit these fields + // — the store leaves the existing values untouched in that case. + updateDmOwner( + event.dmChannelId, + event.newOwnerId, + event.newOwnerHomeUserId ?? undefined, + event.newOwnerHomeInstance ?? undefined, + ); break; } diff --git a/packages/web/src/stores/spaceStore.ts b/packages/web/src/stores/spaceStore.ts index 018c4486..a24923b6 100644 --- a/packages/web/src/stores/spaceStore.ts +++ b/packages/web/src/stores/spaceStore.ts @@ -129,7 +129,12 @@ interface SpaceState { removeDmChannel: (id: string) => void; addDmMember: (dmChannelId: string, user: User) => void; removeDmMember: (dmChannelId: string, userId: string) => void; - updateDmOwner: (dmChannelId: string, newOwnerId: string) => void; + updateDmOwner: ( + dmChannelId: string, + newOwnerId: string, + newOwnerHomeUserId?: string, + newOwnerHomeInstance?: string, + ) => void; updateDmMetadata: (dmChannelId: string, patch: { name?: string | null; icon?: string | null }) => void; closeDm: (id: string) => Promise; leaveDm: (id: string) => Promise; @@ -321,10 +326,19 @@ export const useSpaceStore = create((set, get) => ({ ), })), - updateDmOwner: (dmChannelId, newOwnerId) => set((state) => ({ - dmChannels: state.dmChannels.map(dm => - dm.id === dmChannelId ? { ...dm, ownerId: newOwnerId } : dm - ), + updateDmOwner: (dmChannelId, newOwnerId, newOwnerHomeUserId, newOwnerHomeInstance) => set((state) => ({ + dmChannels: state.dmChannels.map(dm => { + if (dm.id !== dmChannelId) return dm; + const next = { ...dm, ownerId: newOwnerId }; + // Only overwrite the federation routing fields when the caller supplies + // them. Older servers that omit these fields must not blank out the + // existing values — the DM would otherwise lose its owner-routing data + // and `getOwnerInstanceForDm` would silently fall back to '' (home), + // re-introducing the bug this WS extension fixes. + if (newOwnerHomeUserId !== undefined) next.ownerHomeUserId = newOwnerHomeUserId; + if (newOwnerHomeInstance !== undefined) next.ownerHomeInstance = newOwnerHomeInstance; + return next; + }), })), // Patches the group DM's display metadata (name + icon). Idempotent: a diff --git a/packages/web/src/utils/groupDm.ownerRouting.test.ts b/packages/web/src/utils/groupDm.ownerRouting.test.ts index b0d7aa6b..244cc94f 100644 --- a/packages/web/src/utils/groupDm.ownerRouting.test.ts +++ b/packages/web/src/utils/groupDm.ownerRouting.test.ts @@ -209,6 +209,58 @@ describe('group DM owner routing — api.dm.* (Task 5.2)', () => { }); }); + it('updateDmOwner keeps ownerHomeInstance in sync so the next owner-only op routes correctly', () => { + // Regression: the `dm_owner_updated` WS handler used to call + // updateDmOwner(channelId, newOwnerId) without the home-identity fields. + // After a manual back-and-forth transfer, `getOwnerInstanceForDm` then + // returned the PREVIOUS owner's home origin — the next owner-only call + // routed to the wrong instance and the receiver rejected the resulting + // federation event with `unauthorized_source`. + // + // The fix: WS event carries `newOwnerHomeUserId` + `newOwnerHomeInstance` + // and the store writes them. This test pins that behavior down. + const { updateDmOwner } = useSpaceStore.getState(); + useSpaceStore.setState({ + dmChannels: [{ + ...baseDm, + ownerId: 'old-owner', + ownerHomeUserId: 'old-owner-home', + ownerHomeInstance: 'https://nova.test', + }], + }); + + updateDmOwner('dm-1', 'new-owner', 'new-owner-home', 'https://orbit.test'); + + const dm = useSpaceStore.getState().dmChannels.find(d => d.id === 'dm-1'); + expect(dm?.ownerId).toBe('new-owner'); + expect(dm?.ownerHomeUserId).toBe('new-owner-home'); + expect(dm?.ownerHomeInstance).toBe('https://orbit.test'); + expect(getOwnerInstanceForDm('dm-1')).toBe('https://orbit.test'); + }); + + it('updateDmOwner does NOT clear existing federation routing fields when called without them (legacy server)', () => { + // An older server that hasn't shipped the WS payload extension yet would + // call updateDmOwner with only (channelId, newOwnerId). The store must + // not blank out the existing home fields, or `getOwnerInstanceForDm` + // would silently fall back to '' (home) — re-introducing the bug. + const { updateDmOwner } = useSpaceStore.getState(); + useSpaceStore.setState({ + dmChannels: [{ + ...baseDm, + ownerId: 'old-owner', + ownerHomeUserId: 'old-owner-home', + ownerHomeInstance: 'https://nova.test', + }], + }); + + updateDmOwner('dm-1', 'new-owner'); + + const dm = useSpaceStore.getState().dmChannels.find(d => d.id === 'dm-1'); + expect(dm?.ownerId).toBe('new-owner'); + expect(dm?.ownerHomeUserId).toBe('old-owner-home'); + expect(dm?.ownerHomeInstance).toBe('https://nova.test'); + }); + it('non-owner-only op (sendMessage) is unaffected by ownerHomeInstance', async () => { // Owner routing is opt-in per method — sendMessage on the singleton api // must NOT consult ownerHomeInstance. It uses the channel's pinned origin