From 6afb935302440fcf0d6e80801e5e0e9e2933bec4 Mon Sep 17 00:00:00 2001 From: hzhang Date: Sat, 16 May 2026 17:47:01 +0100 Subject: [PATCH] fix(security): close Critical auth gaps (C1/C2/C3) C1: replace fragile path.endsWith() guard whitelist with a metadata @Public() decorator + Reflector (no more path-shape bypass surface). C2: CenterApiKeyGuard attaches the authenticated GuildNode; introspect & resolve-names now reject when body.guildNodeId != that node (stops one node probing/enumerating another guild's identities). C3: heartbeat/status are self-only (a node can't revoke/hijack another); GET /nodes no longer returns apiKeyHash (credential-hash leak). Co-Authored-By: Claude Opus 4.7 (1M context) --- src/auth/auth.controller.ts | 46 ++++++++++++++++++++++++------ src/common/center-api-key.guard.ts | 44 ++++++++++++++-------------- src/common/health.controller.ts | 2 ++ src/common/public.decorator.ts | 8 ++++++ src/nodes/nodes.controller.ts | 33 +++++++++++++++++++-- 5 files changed, 100 insertions(+), 33 deletions(-) create mode 100644 src/common/public.decorator.ts diff --git a/src/auth/auth.controller.ts b/src/auth/auth.controller.ts index 9337e89..d6d12a7 100644 --- a/src/auth/auth.controller.ts +++ b/src/auth/auth.controller.ts @@ -1,9 +1,11 @@ -import { Body, Controller, Get, Headers, Param, Patch, Post, UnauthorizedException } from '@nestjs/common'; +import { Body, Controller, ForbiddenException, Get, Headers, Param, Patch, Post, Req, UnauthorizedException } from '@nestjs/common'; import { AuthService } from './auth.service.js'; import { LoginDto } from './dto.login.dto.js'; import { RefreshDto } from './dto.refresh.dto.js'; import { LogoutDto } from './dto.logout.dto.js'; import { UpdateMeDto } from './dto.update-me.dto.js'; +import { Public } from '../common/public.decorator.js'; +import type { AuthedGuildNode } from '../common/center-api-key.guard.js'; function bearer(authorization?: string): string { return authorization?.startsWith('Bearer ') ? authorization.slice(7) : ''; @@ -13,21 +15,25 @@ function bearer(authorization?: string): string { export class AuthController { constructor(private readonly authService: AuthService) {} + @Public() @Post('login') login(@Body() body: LoginDto) { return this.authService.login(body); } + @Public() @Post('refresh') refresh(@Body() body: RefreshDto) { return this.authService.refresh(body.refreshToken); } + @Public() @Post('logout') logout(@Body() body: LogoutDto) { return this.authService.logout(body.refreshToken); } + @Public() @Get('me') me(@Headers('authorization') authorization?: string) { const token = bearer(authorization); @@ -35,6 +41,7 @@ export class AuthController { return this.authService.getMe(token); } + @Public() @Patch('me') updateMe(@Headers('authorization') authorization: string | undefined, @Body() body: UpdateMeDto) { const token = bearer(authorization); @@ -42,41 +49,64 @@ export class AuthController { return this.authService.updateMe(token, body.name); } + @Public() @Get('me/guilds') meGuilds(@Headers('authorization') authorization?: string) { - const token = authorization?.startsWith('Bearer ') ? authorization.slice(7) : ''; + const token = bearer(authorization); if (!token) throw new UnauthorizedException('missing bearer token'); return this.authService.listMyGuilds(token); } + @Public() @Post('me/guilds/join') joinGuild(@Headers('authorization') authorization: string | undefined, @Body() body: { guildNodeId?: string }) { - const token = authorization?.startsWith('Bearer ') ? authorization.slice(7) : ''; + const token = bearer(authorization); if (!token) throw new UnauthorizedException('missing bearer token'); return this.authService.joinGuild(token, String(body?.guildNodeId ?? '')); } + @Public() @Get('guilds/:guildNodeId/members') guildMembers(@Headers('authorization') authorization: string | undefined, @Param('guildNodeId') guildNodeId: string) { - const token = authorization?.startsWith('Bearer ') ? authorization.slice(7) : ''; + const token = bearer(authorization); if (!token) throw new UnauthorizedException('missing bearer token'); return this.authService.listGuildMembers(token, guildNodeId); } + @Public() @Post('agent/login') agentLogin(@Body() body: { apiKey?: string }) { return this.authService.agentLogin(String(body?.apiKey ?? '')); } + // Not @Public(): requires a guild node api key. The guard attaches the + // authenticated node; a node may only introspect tokens for its own + // guild (Center C2 — prevents cross-tenant identity probing). @Post('introspect') - introspect(@Body() body: { token?: string; guildNodeId?: string }) { - return this.authService.introspectGuildToken(body?.token ?? '', body?.guildNodeId ?? ''); + introspect( + @Req() req: { guildNode?: AuthedGuildNode }, + @Body() body: { token?: string; guildNodeId?: string }, + ) { + const requested = String(body?.guildNodeId ?? ''); + if (!req.guildNode || req.guildNode.nodeId !== requested) { + throw new ForbiddenException('api key not authorized for this guild node'); + } + return this.authService.introspectGuildToken(body?.token ?? '', requested); } + // Not @Public(): same node-ownership rule as introspect (Center C2 — + // prevents one node enumerating another guild's member identities). @Post('resolve-names') - resolveNames(@Body() body: { guildNodeId?: string; names?: string[] }) { + resolveNames( + @Req() req: { guildNode?: AuthedGuildNode }, + @Body() body: { guildNodeId?: string; names?: string[] }, + ) { + const requested = String(body?.guildNodeId ?? ''); + if (!req.guildNode || req.guildNode.nodeId !== requested) { + throw new ForbiddenException('api key not authorized for this guild node'); + } return this.authService.resolveNames( - String(body?.guildNodeId ?? ''), + requested, Array.isArray(body?.names) ? body.names : [], ); } diff --git a/src/common/center-api-key.guard.ts b/src/common/center-api-key.guard.ts index 88f9380..5026b9c 100644 --- a/src/common/center-api-key.guard.ts +++ b/src/common/center-api-key.guard.ts @@ -1,43 +1,36 @@ import { CanActivate, ExecutionContext, Injectable, UnauthorizedException } from '@nestjs/common'; +import { Reflector } from '@nestjs/core'; import { InjectRepository } from '@nestjs/typeorm'; import { Repository } from 'typeorm'; import bcrypt from 'bcryptjs'; import { GuildNode } from '../entities/guild-node.entity.js'; +import { IS_PUBLIC_KEY } from './public.decorator.js'; + +// The authenticated guild node, attached to the request by this guard so +// downstream handlers can enforce node-ownership (a node may only act on +// its own nodeId / introspect tokens for its own guild). +export type AuthedGuildNode = { id: string; nodeId: string }; @Injectable() export class CenterApiKeyGuard implements CanActivate { constructor( @InjectRepository(GuildNode) private readonly nodeRepo: Repository, + private readonly reflector: Reflector, ) {} async canActivate(context: ExecutionContext): Promise { + const isPublic = this.reflector.getAllAndOverride(IS_PUBLIC_KEY, [ + context.getHandler(), + context.getClass(), + ]); + if (isPublic) return true; + const req = context.switchToHttp().getRequest<{ - path?: string; - method?: string; headers: Record; + guildNode?: AuthedGuildNode; }>(); - const path = req.path ?? ''; - const method = (req.method ?? 'GET').toUpperCase(); - - const noApiKeyRequired = - path === '/healthz' || - path.endsWith('/healthz') || - (method === 'POST' && (path === '/auth/login' || path.endsWith('/auth/login'))) || - (method === 'POST' && (path === '/auth/agent/login' || path.endsWith('/auth/agent/login'))) || - (method === 'POST' && (path === '/auth/refresh' || path.endsWith('/auth/refresh'))) || - (method === 'POST' && (path === '/auth/logout' || path.endsWith('/auth/logout'))) || - (method === 'GET' && (path === '/auth/me' || path.endsWith('/auth/me'))) || - (method === 'PATCH' && (path === '/auth/me' || path.endsWith('/auth/me'))) || - (method === 'GET' && (path === '/auth/me/guilds' || path.endsWith('/auth/me/guilds'))) || - (method === 'POST' && (path === '/auth/me/guilds/join' || path.endsWith('/auth/me/guilds/join'))) || - (method === 'GET' && (path.includes('/auth/guilds/') && path.endsWith('/members'))); - - if (noApiKeyRequired) { - return true; - } - const received = req.headers['x-api-key']; const apiKey = Array.isArray(received) ? received[0] : received; if (!apiKey) throw new UnauthorizedException('missing api key'); @@ -46,7 +39,12 @@ export class CenterApiKeyGuard implements CanActivate { for (const node of nodes) { if (!node.apiKeyHash) continue; const ok = await bcrypt.compare(apiKey, node.apiKeyHash); - if (ok) return true; + if (ok) { + // Identify which node this key belongs to so handlers can enforce + // that a node only acts within its own scope (Center C2/C3). + req.guildNode = { id: node.id, nodeId: node.nodeId }; + return true; + } } throw new UnauthorizedException('invalid api key'); diff --git a/src/common/health.controller.ts b/src/common/health.controller.ts index 7bb949a..bc02644 100644 --- a/src/common/health.controller.ts +++ b/src/common/health.controller.ts @@ -1,6 +1,8 @@ import { Controller, Get, ServiceUnavailableException } from '@nestjs/common'; import { DataSource } from 'typeorm'; +import { Public } from './public.decorator.js'; +@Public() @Controller('healthz') export class HealthController { constructor(private readonly dataSource: DataSource) {} diff --git a/src/common/public.decorator.ts b/src/common/public.decorator.ts new file mode 100644 index 0000000..2c5adaf --- /dev/null +++ b/src/common/public.decorator.ts @@ -0,0 +1,8 @@ +import { SetMetadata } from '@nestjs/common'; + +// Routes annotated with @Public() skip the global CenterApiKeyGuard +// (api-key + node-identity) check. This is metadata-driven on purpose: +// the previous string-matching whitelist (path.endsWith(...)) was a +// bypass surface. Only the route's own decorator opens it. +export const IS_PUBLIC_KEY = 'fabric:isPublic'; +export const Public = () => SetMetadata(IS_PUBLIC_KEY, true); diff --git a/src/nodes/nodes.controller.ts b/src/nodes/nodes.controller.ts index 6f1160a..86b9af6 100644 --- a/src/nodes/nodes.controller.ts +++ b/src/nodes/nodes.controller.ts @@ -2,6 +2,7 @@ import { Body, Controller, DefaultValuePipe, + ForbiddenException, Get, NotFoundException, Param, @@ -9,12 +10,14 @@ import { Patch, Post, Query, + Req, } from '@nestjs/common'; import { InjectRepository } from '@nestjs/typeorm'; import { Repository } from 'typeorm'; import { GuildNode } from '../entities/guild-node.entity.js'; import { AuditService } from '../audit/audit.service.js'; import { UpdateNodeStatusDto } from './dto.update-node-status.dto.js'; +import type { AuthedGuildNode } from '../common/center-api-key.guard.js'; @Controller('nodes') export class NodesController { @@ -24,8 +27,21 @@ export class NodesController { private readonly audit: AuditService, ) {} + // A node may only act on itself. The guard authenticates the caller via + // its api key and attaches the matching node; reject if it tries to + // touch any other nodeId (Center C3 — prevents node hijack/DoS). + private assertSelf(req: { guildNode?: AuthedGuildNode }, nodeId: string) { + if (!req.guildNode || req.guildNode.nodeId !== nodeId) { + throw new ForbiddenException('api key not authorized for this node'); + } + } + @Post(':nodeId/heartbeat') - async heartbeat(@Param('nodeId') nodeId: string) { + async heartbeat( + @Req() req: { guildNode?: AuthedGuildNode }, + @Param('nodeId') nodeId: string, + ) { + this.assertSelf(req, nodeId); const node = await this.nodeRepo.findOne({ where: { nodeId } }); if (!node) { throw new NotFoundException('node not found'); @@ -53,9 +69,11 @@ export class NodesController { @Patch(':nodeId/status') async updateStatus( + @Req() req: { guildNode?: AuthedGuildNode }, @Param('nodeId') nodeId: string, @Body() body: UpdateNodeStatusDto, ) { + this.assertSelf(req, nodeId); const node = await this.nodeRepo.findOne({ where: { nodeId } }); if (!node) { throw new NotFoundException('node not found'); @@ -93,7 +111,18 @@ export class NodesController { }); return { - items, + // Never expose apiKeyHash (Center C3 — credential hash leak / offline + // brute-force). Project to an explicit safe shape. + items: items.map((n: GuildNode) => ({ + id: n.id, + nodeId: n.nodeId, + name: n.name, + endpoint: n.endpoint, + status: n.status, + lastHeartbeatAt: n.lastHeartbeatAt, + createdAt: n.createdAt, + updatedAt: n.updatedAt, + })), page: safePage, pageSize: safePageSize, total,