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) <noreply@anthropic.com>
This commit is contained in:
h z
2026-05-16 17:47:01 +01:00
parent aa9d59a952
commit 6afb935302
5 changed files with 100 additions and 33 deletions

View File

@@ -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 : [],
);
}

View File

@@ -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<GuildNode>,
private readonly reflector: Reflector,
) {}
async canActivate(context: ExecutionContext): Promise<boolean> {
const isPublic = this.reflector.getAllAndOverride<boolean>(IS_PUBLIC_KEY, [
context.getHandler(),
context.getClass(),
]);
if (isPublic) return true;
const req = context.switchToHttp().getRequest<{
path?: string;
method?: string;
headers: Record<string, string | string[] | undefined>;
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');

View File

@@ -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) {}

View File

@@ -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);

View File

@@ -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,