From 340eed8aa3ffcd61d1e33a3e99702298250a394b Mon Sep 17 00:00:00 2001 From: hzhang Date: Thu, 28 May 2026 20:51:19 +0100 Subject: [PATCH] feat(guild): restore system-key bypass + isSystem msg path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Resurrects the x-fabric-system-key bypass + isSystem branch on POST /channels/:id/messages, dropped in ca20df7 when dialectic stopped broadcasting topic lifecycle events to Fabric. Re-enabling now because Fabric.OpenclawPlugin's close-sub-discussion needs to write a callback into a parent channel as a system-authored message (not as the closing host), with an optional precision wakeup so the recruitment workflow can resume immediately after an interview sub-discussion closes. Three coupled bits: 1. ApiKeyGuard pre-Bearer bypass: when x-fabric-system-key matches FABRIC_BACKEND_GUILD_COMMANDS_SYNC_KEY, set req.isSystem=true and skip the Bearer check. Intentionally reuses the existing commands sync env — same shared secret, same consumer (the OpenclawPlugin reads channels.fabric.commandsSyncKey for both paths). One less env to rotate, one less secret to manage. 2. messaging.controller POST /channels/:id/messages adds an isSystem branch (runs before the participant gate): - Looks up the channel directly (not via assertParticipant). - Persists with sentinel author 00000000-0000-0000-0000-000000000000, same UUID the old impl used. - Translates <@user.name:NAME> mentions like the regular path. - When wakeupUserId is set, delivers via emitMessageTargeted so that exactly that one recipient receives wakeup=true; everyone else gets wakeup=false. When omitted, delivers via emitMessageCreated with an empty wakeUserIds set so nobody is woken — silent system log. Two intentional differences from the 985b06a original: - No xType=announce restriction. The original was limited to announce because that was Dialectic's only use case; we now need this on dm / general / discuss / etc. for the sub-discussion callback. Closed channels are still rejected (409) on both paths. - The wakeupUserId field is new — old impl only ever sent silent announces. 3. DTO carries wakeupUserId? optional string. Ignored on the regular user-bearer path; load-bearing on the system path. Shared helper: extracted commands.controller's private safeEqual into src/common/safe-equal.ts so api-key.guard.ts can use the same constant- time check. Vitest spec covers equal / inequal / length-mismatch / empty cases. Existing unit tests still pass. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/commands/commands.controller.ts | 9 +--- src/common/api-key.guard.ts | 20 +++++++++ src/common/safe-equal.spec.ts | 25 +++++++++++ src/common/safe-equal.ts | 12 +++++ src/messaging/dto.create-message.dto.ts | 10 +++++ src/messaging/messaging.controller.ts | 59 ++++++++++++++++++++++++- 6 files changed, 126 insertions(+), 9 deletions(-) create mode 100644 src/common/safe-equal.spec.ts create mode 100644 src/common/safe-equal.ts diff --git a/src/commands/commands.controller.ts b/src/commands/commands.controller.ts index 423b8fc..ca45f75 100644 --- a/src/commands/commands.controller.ts +++ b/src/commands/commands.controller.ts @@ -8,19 +8,12 @@ import { Req, UnauthorizedException, } from '@nestjs/common'; -import { timingSafeEqual } from 'node:crypto'; import { CommandsService } from './commands.service.js'; import { SyncCommandsDto } from './dto.sync-commands.dto.js'; +import { safeEqual } from '../common/safe-equal.js'; type AuthedRequest = { userId?: string }; -function safeEqual(a: string, b: string): boolean { - const ab = Buffer.from(a); - const bb = Buffer.from(b); - if (ab.length !== bb.length) return false; - return timingSafeEqual(ab, bb); -} - @Controller('commands') export class CommandsController { constructor(private readonly commands: CommandsService) {} diff --git a/src/common/api-key.guard.ts b/src/common/api-key.guard.ts index eb27a83..04b44a6 100644 --- a/src/common/api-key.guard.ts +++ b/src/common/api-key.guard.ts @@ -5,6 +5,7 @@ import { UnauthorizedException, } from '@nestjs/common'; import { introspectGuildToken } from './center-auth.js'; +import { safeEqual } from './safe-equal.js'; @Injectable() export class ApiKeyGuard implements CanActivate { @@ -21,6 +22,25 @@ export class ApiKeyGuard implements CanActivate { return true; } + // System-key bypass: when a caller presents x-fabric-system-key + // matching FABRIC_BACKEND_GUILD_COMMANDS_SYNC_KEY (intentionally the + // same shared secret as x-commands-sync-key — both legitimate + // consumers are Fabric.OpenclawPlugin), skip the Bearer requirement + // and mark this as a system caller. Downstream handlers (e.g. + // messaging.controller POST /channels/:id/messages) gate on + // req.isSystem to take the system-author code path. + // + // Empty env → bypass disabled (no system caller ever valid; closed + // by default). Header carries the secret as-is; we constant-time + // compare against the env value. + const sysExpected = (process.env.FABRIC_BACKEND_GUILD_COMMANDS_SYNC_KEY ?? '').trim(); + const sysHeader = req.headers['x-fabric-system-key']; + const sysProvided = Array.isArray(sysHeader) ? sysHeader[0] : sysHeader; + if (sysExpected && sysProvided && safeEqual(sysProvided, sysExpected)) { + (req as { isSystem?: boolean }).isSystem = true; + return true; + } + const auth = req.headers['authorization']; const authValue = Array.isArray(auth) ? auth[0] : auth; let token = authValue?.startsWith('Bearer ') ? authValue.slice(7) : ''; diff --git a/src/common/safe-equal.spec.ts b/src/common/safe-equal.spec.ts new file mode 100644 index 0000000..634f580 --- /dev/null +++ b/src/common/safe-equal.spec.ts @@ -0,0 +1,25 @@ +import { describe, it, expect } from 'vitest'; +import { safeEqual } from './safe-equal.js'; + +describe('safeEqual', () => { + it('returns true for identical non-empty strings', () => { + expect(safeEqual('abc123', 'abc123')).toBe(true); + }); + + it('returns false for different strings of same length', () => { + expect(safeEqual('abc123', 'abc124')).toBe(false); + }); + + it('returns false for differing lengths', () => { + expect(safeEqual('abc', 'abcd')).toBe(false); + }); + + it('handles empty strings', () => { + // both empty is technically equal — but downstream callers should + // explicitly check for empty expected before invoking. We just + // document the constant-time-comparison primitive's behavior. + expect(safeEqual('', '')).toBe(true); + expect(safeEqual('a', '')).toBe(false); + expect(safeEqual('', 'a')).toBe(false); + }); +}); diff --git a/src/common/safe-equal.ts b/src/common/safe-equal.ts new file mode 100644 index 0000000..a4e5c73 --- /dev/null +++ b/src/common/safe-equal.ts @@ -0,0 +1,12 @@ +import { timingSafeEqual } from 'node:crypto'; + +// Constant-time string comparison. Returns false for length mismatch (the +// length difference itself is observable, but the per-byte loop isn't). +// Used for shared-secret header checks (commands-sync-key, system-key, +// etc.) to keep timing-oracle attacks off the table. +export function safeEqual(a: string, b: string): boolean { + const ab = Buffer.from(a); + const bb = Buffer.from(b); + if (ab.length !== bb.length) return false; + return timingSafeEqual(ab, bb); +} diff --git a/src/messaging/dto.create-message.dto.ts b/src/messaging/dto.create-message.dto.ts index 3506e51..6080a8e 100644 --- a/src/messaging/dto.create-message.dto.ts +++ b/src/messaging/dto.create-message.dto.ts @@ -56,4 +56,14 @@ export class CreateMessageDto { @IsString() @MaxLength(64) authorUserId?: string; + + // System-author path only (x-fabric-system-key gated). When set, the + // message is delivered via emitMessageTargeted so this single recipient + // gets wakeup=true; everyone else in the channel sees wakeup=false. For + // regular (user-bearer) posts this field is ignored. Used by + // close-sub-discussion to precisely wake the host on callback. + @IsOptional() + @IsString() + @MaxLength(64) + wakeupUserId?: string; } diff --git a/src/messaging/messaging.controller.ts b/src/messaging/messaging.controller.ts index 5a4a57e..64dec71 100644 --- a/src/messaging/messaging.controller.ts +++ b/src/messaging/messaging.controller.ts @@ -156,13 +156,70 @@ export class MessagingController { async create( @Param('id') channelId: string, @Body() body: CreateMessageDto, - @Req() req: { userId?: string }, + @Req() req: { userId?: string; isSystem?: boolean }, @Headers('idempotency-key') idempotencyKey?: string, ) { const scope = `POST:/channels/${channelId}/messages`; const existed = await this.getIdempotentResponse(scope, idempotencyKey); if (existed) return existed; + // System caller (ApiKeyGuard set isSystem from x-fabric-system-key): + // skip the per-user participant check; resolve channel directly. + // System posts are allowed into any non-closed channel — used by + // Fabric.OpenclawPlugin to write `close-sub-discussion` callbacks + // back to a parent channel that the host agent may not be currently + // "in" from the backend's perspective, and to deliver guide-injected + // system intros into sub-discussion channels without needing to log + // in as a real user. Author is a sentinel UUID that no real user + // ever has; `wakeupUserId` (optional) lets the caller precisely wake + // one recipient (e.g. the host of a closing sub-discussion). + if (req.isSystem) { + const sysChannel = await this.channelRepo.findOne({ where: { id: channelId } }); + if (!sysChannel) throw new NotFoundException('channel not found'); + if (sysChannel.closed) { + throw new ConflictException({ error: 'channel_closed', message: 'channel is closed' }); + } + const SYSTEM_USER_ID = '00000000-0000-0000-0000-000000000000'; + let sysContent = body.content ?? ''; + const sysNames = extractNameMentions(sysContent); + if (sysNames.length) { + const nameMap = await resolveUserNames(sysNames); + sysContent = replaceNameMentions(sysContent, nameMap); + } + const sysMessage = await this.persistMessage(channelId, { + authorUserId: SYSTEM_USER_ID, + content: sysContent, + clientMessageId: body.clientMessageId, + replyToMessageId: body.replyToMessageId, + mentions: body.mentions, + attachments: body.attachments, + }); + const sysView = this.toView(sysMessage) as Record; + await this.saveIdempotentResponse(scope, idempotencyKey, sysView); + await this.events.emit({ + eventType: 'message.created', + channelId, + actorId: SYSTEM_USER_ID, + data: sysView, + }); + // wakeupUserId set -> emitMessageTargeted wakes exactly that user + // (everyone else gets the same message with wakeup=false). + // wakeupUserId omitted/null -> emitMessageCreated routes via the + // channel's xType-specific 3-state delivery with empty wakeSet, so + // nobody is woken (the message lands in history only). + const wakeupUserId = typeof body.wakeupUserId === 'string' ? body.wakeupUserId.trim() : ''; + if (wakeupUserId) { + await this.realtime.emitMessageTargeted(channelId, sysView, wakeupUserId); + } else { + await this.realtime.emitMessageCreated(channelId, sysView, { + xType: sysChannel.xType ?? 'general', + authorUserId: SYSTEM_USER_ID, + wakeUserIds: new Set(), + }); + } + return sysView; + } + // Guild C-1: caller must be a participant of the channel, and the // author is always the authenticated user — body.authorUserId is // ignored so a caller can never post as someone else.