fix(security): close Critical IDOR/authz gaps (C-1/C-2)
C-1: messaging endpoints now enforce channel participation (public
channels open; private require channel_members). authorUserId is
forced to the authenticated user (no more author spoofing); edit/
delete require message-author ownership; history read gated too.
C-2: PUT /commands body strictly validated + size-capped via
SyncCommandsDto (kills catalog poisoning / DoS). Optional
FABRIC_BACKEND_GUILD_COMMANDS_SYNC_KEY restricts the write to the
plugin when set; never weaker than before when unset.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -3,6 +3,7 @@ import {
|
||||
ConflictException,
|
||||
Controller,
|
||||
Delete,
|
||||
ForbiddenException,
|
||||
Get,
|
||||
Headers,
|
||||
NotFoundException,
|
||||
@@ -10,11 +11,13 @@ import {
|
||||
Patch,
|
||||
Post,
|
||||
Query,
|
||||
Req,
|
||||
} from '@nestjs/common';
|
||||
import { InjectRepository } from '@nestjs/typeorm';
|
||||
import { DataSource, Repository } from 'typeorm';
|
||||
import { CreateMessageDto } from './dto.create-message.dto.js';
|
||||
import { Channel } from '../entities/channel.entity.js';
|
||||
import { ChannelMember } from '../entities/channel-member.entity.js';
|
||||
import { Message } from '../entities/message.entity.js';
|
||||
import { IdempotencyRecord } from '../entities/idempotency-record.entity.js';
|
||||
import { WakeMapping } from '../entities/wake-mapping.entity.js';
|
||||
@@ -36,6 +39,8 @@ export class MessagingController {
|
||||
private readonly dataSource: DataSource,
|
||||
@InjectRepository(Channel)
|
||||
private readonly channelRepo: Repository<Channel>,
|
||||
@InjectRepository(ChannelMember)
|
||||
private readonly memberRepo: Repository<ChannelMember>,
|
||||
@InjectRepository(Message)
|
||||
private readonly messageRepo: Repository<Message>,
|
||||
@InjectRepository(IdempotencyRecord)
|
||||
@@ -86,6 +91,19 @@ export class MessagingController {
|
||||
};
|
||||
}
|
||||
|
||||
// Channel-participant gate (Guild C-1): public channels are readable/
|
||||
// writable by any authenticated user; private channels require explicit
|
||||
// channel_members membership. Returns the channel so callers can reuse it.
|
||||
private async assertParticipant(channelId: string, userId: string): Promise<Channel> {
|
||||
const channel = await this.channelRepo.findOne({ where: { id: channelId } });
|
||||
if (!channel) throw new NotFoundException('channel not found');
|
||||
if (channel.isPublic) return channel;
|
||||
if (!userId) throw new ForbiddenException('not a channel member');
|
||||
const member = await this.memberRepo.findOne({ where: { channelId, userId } });
|
||||
if (!member) throw new ForbiddenException('not a channel member');
|
||||
return channel;
|
||||
}
|
||||
|
||||
// Persists one message (allocates a seq under a channel row lock) and
|
||||
// returns its view. Used for normal messages and for guild /ack messages.
|
||||
private async persistMessage(
|
||||
@@ -136,20 +154,25 @@ export class MessagingController {
|
||||
async create(
|
||||
@Param('id') channelId: string,
|
||||
@Body() body: CreateMessageDto,
|
||||
@Req() req: { userId?: string },
|
||||
@Headers('idempotency-key') idempotencyKey?: string,
|
||||
) {
|
||||
const scope = `POST:/channels/${channelId}/messages`;
|
||||
const existed = await this.getIdempotentResponse(scope, idempotencyKey);
|
||||
if (existed) return existed;
|
||||
|
||||
const channel = await this.channelRepo.findOne({ where: { id: channelId } });
|
||||
if (!channel) throw new NotFoundException('channel not found');
|
||||
// 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.
|
||||
const userId = String(req.userId ?? '');
|
||||
if (!userId) throw new ForbiddenException('missing user');
|
||||
const channel = await this.assertParticipant(channelId, userId);
|
||||
if (channel.closed) {
|
||||
throw new ConflictException({ error: 'channel_closed', message: 'channel is closed' });
|
||||
}
|
||||
const xType = channel.xType ?? 'general';
|
||||
const isRotating = xType === 'discuss' || xType === 'work';
|
||||
const authorUserId = String(body.authorUserId ?? 'anonymous');
|
||||
const authorUserId = userId;
|
||||
|
||||
// ---- translate <@user.name:NAME> -> <@userId> (outside backticks) via
|
||||
// Center before anything else persists/parses the content
|
||||
@@ -223,14 +246,23 @@ export class MessagingController {
|
||||
@Param('id') channelId: string,
|
||||
@Param('messageId') messageId: string,
|
||||
@Body() body: { content?: string },
|
||||
@Req() req: { userId?: string },
|
||||
@Headers('idempotency-key') idempotencyKey?: string,
|
||||
) {
|
||||
const scope = `PATCH:/channels/${channelId}/messages/${messageId}`;
|
||||
const existed = await this.getIdempotentResponse(scope, idempotencyKey);
|
||||
if (existed) return existed;
|
||||
|
||||
// Guild C-1: participant + author-ownership.
|
||||
const userId = String(req.userId ?? '');
|
||||
if (!userId) throw new ForbiddenException('missing user');
|
||||
await this.assertParticipant(channelId, userId);
|
||||
|
||||
const item = await this.messageRepo.findOne({ where: { channelId, messageId } });
|
||||
if (!item) return { status: 'not_found' };
|
||||
if (item.authorUserId !== userId) {
|
||||
throw new ForbiddenException('not the message author');
|
||||
}
|
||||
|
||||
const now = Date.now();
|
||||
const createdAt = new Date(item.createdAt).getTime();
|
||||
@@ -259,14 +291,23 @@ export class MessagingController {
|
||||
async remove(
|
||||
@Param('id') channelId: string,
|
||||
@Param('messageId') messageId: string,
|
||||
@Req() req: { userId?: string },
|
||||
@Headers('idempotency-key') idempotencyKey?: string,
|
||||
) {
|
||||
const scope = `DELETE:/channels/${channelId}/messages/${messageId}`;
|
||||
const existed = await this.getIdempotentResponse(scope, idempotencyKey);
|
||||
if (existed) return existed;
|
||||
|
||||
// Guild C-1: participant + author-ownership.
|
||||
const userId = String(req.userId ?? '');
|
||||
if (!userId) throw new ForbiddenException('missing user');
|
||||
await this.assertParticipant(channelId, userId);
|
||||
|
||||
const item = await this.messageRepo.findOne({ where: { channelId, messageId } });
|
||||
if (!item) return { status: 'not_found' };
|
||||
if (item.authorUserId !== userId) {
|
||||
throw new ForbiddenException('not the message author');
|
||||
}
|
||||
|
||||
item.isDeleted = true;
|
||||
item.deletedAt = new Date();
|
||||
@@ -304,10 +345,14 @@ export class MessagingController {
|
||||
@Get()
|
||||
async listBySeq(
|
||||
@Param('id') channelId: string,
|
||||
@Req() req: { userId?: string },
|
||||
@Query('seq_from') seqFrom?: string,
|
||||
@Query('seq_to') seqTo?: string,
|
||||
@Query('limit') limit?: string,
|
||||
) {
|
||||
// Guild C-1: only participants may read channel history.
|
||||
const userId = String(req.userId ?? '');
|
||||
if (!userId) throw new ForbiddenException('missing user');
|
||||
const from = seqFrom ? Number(seqFrom) : 1;
|
||||
const to = seqTo ? Number(seqTo) : Number.MAX_SAFE_INTEGER;
|
||||
const safeLimit = clampLimit(limit, DEFAULT_PAGE_LIMIT, MAX_PAGE_LIMIT);
|
||||
@@ -327,10 +372,7 @@ export class MessagingController {
|
||||
};
|
||||
}
|
||||
|
||||
const channel = await this.channelRepo.findOne({ where: { id: channelId } });
|
||||
if (!channel) {
|
||||
throw new NotFoundException('channel not found');
|
||||
}
|
||||
const channel = await this.assertParticipant(channelId, userId);
|
||||
|
||||
const qb = this.messageRepo
|
||||
.createQueryBuilder('m')
|
||||
|
||||
Reference in New Issue
Block a user