diff --git a/apps/api/src/app/endpoints/mcp/mcp.controller.spec.ts b/apps/api/src/app/endpoints/mcp/mcp.controller.spec.ts index ad7bc93e3..2c84df6d0 100644 --- a/apps/api/src/app/endpoints/mcp/mcp.controller.spec.ts +++ b/apps/api/src/app/endpoints/mcp/mcp.controller.spec.ts @@ -2,6 +2,8 @@ import { ImportValidationError } from '@ghostfolio/api/app/import/errors/import- import { ImportService } from '@ghostfolio/api/app/import/import.service'; import { UserService } from '@ghostfolio/api/app/user/user.service'; import { REQUIRES_SCOPE_KEY } from '@ghostfolio/api/decorators/requires-scope.decorator'; +import { McpToolExceptionFilter } from '@ghostfolio/api/filters/mcp-tool-exception.filter'; +import { AccessGuard } from '@ghostfolio/api/guards/access.guard'; import { ConfigurationService } from '@ghostfolio/api/services/configuration/configuration.service'; import { MCP_MAX_ACTIVITIES } from '@ghostfolio/common/config'; import { Activity } from '@ghostfolio/common/interfaces'; @@ -13,6 +15,10 @@ import type { } from '@ghostfolio/common/types'; import { HttpException } from '@nestjs/common'; +import { + EXCEPTION_FILTERS_METADATA, + GUARDS_METADATA +} from '@nestjs/common/constants'; import { DataSource, Type as ActivityType } from '@prisma/client'; import { MCP_TOOL_METADATA_KEY, ToolMetadata } from '@rekog/mcp-nest'; @@ -133,6 +139,34 @@ describe('GhostfolioMcpController', () => { expect(toolMethodNamesWithoutScope).toEqual([]); }); + + // The decorator RequiresScope sets the same metadata as the decorator + // RequiresScopeOfAccess, but applies AuthGuard('jwt'), which a request of + // an access cannot pass, hence the guards tell the two decorators apart + it('Applies the guard of the access to each tool', () => { + const toolMethodNames = getToolMethodNames(); + + expect(toolMethodNames.length).toBeGreaterThan(0); + + const toolMethodNamesWithoutGuardOfAccess = toolMethodNames.filter( + (methodName) => { + return !getMetadataOfMethod( + GUARDS_METADATA, + methodName + )?.includes(AccessGuard); + } + ); + + expect(toolMethodNamesWithoutGuardOfAccess).toEqual([]); + }); + + // The tools have no try and catch, hence the filter is the only guarantee + // that an unexpected exception does not expose internals + it('Applies the filter of the exceptions of the tools', () => { + expect( + Reflect.getMetadata(EXCEPTION_FILTERS_METADATA, GhostfolioMcpController) + ).toEqual([McpToolExceptionFilter]); + }); }); describe('Import activities', () => { diff --git a/apps/api/src/app/endpoints/mcp/mcp.controller.ts b/apps/api/src/app/endpoints/mcp/mcp.controller.ts index 951436c73..484ecb12b 100644 --- a/apps/api/src/app/endpoints/mcp/mcp.controller.ts +++ b/apps/api/src/app/endpoints/mcp/mcp.controller.ts @@ -202,7 +202,7 @@ export class GhostfolioMcpController { }; }); - // The filter passes on the message of an ImportValidationError, which is + // The filter passes on the message of a CallerFacingError, which is // written for the caller, and hides the message of every other error const importedActivities = await this.importService.import({ activitiesDto, diff --git a/apps/api/src/app/endpoints/mcp/mcp.module.ts b/apps/api/src/app/endpoints/mcp/mcp.module.ts index 3f3cfdd32..0c054c7fb 100644 --- a/apps/api/src/app/endpoints/mcp/mcp.module.ts +++ b/apps/api/src/app/endpoints/mcp/mcp.module.ts @@ -21,6 +21,7 @@ import { McpService } from './mcp.service'; controllers: [GhostfolioMcpController], imports: [AiModule, ApiModule, ConfigurationModule, ImportModule, UserModule], providers: [ + McpService, { inject: [ConfigurationService], provide: MCP_STRATEGY, @@ -45,8 +46,7 @@ import { McpService } from './mcp.service'; websiteUrl: 'https://ghostfol.io' }); } - }, - McpService + } ] }) export class McpModule {} diff --git a/apps/api/src/filters/mcp-tool-exception.filter.spec.ts b/apps/api/src/filters/mcp-tool-exception.filter.spec.ts index e75033762..23eb6ff92 100644 --- a/apps/api/src/filters/mcp-tool-exception.filter.spec.ts +++ b/apps/api/src/filters/mcp-tool-exception.filter.spec.ts @@ -2,7 +2,6 @@ import { ImportValidationError } from '@ghostfolio/api/app/import/errors/import- import { PortfolioSnapshotComputationError } from '@ghostfolio/api/app/portfolio/errors/portfolio-snapshot-computation.error'; import { ForbiddenException, Logger } from '@nestjs/common'; -import { RpcException } from '@nestjs/microservices'; import { getReasonPhrase, StatusCodes } from 'http-status-codes'; import { firstValueFrom } from 'rxjs'; @@ -18,6 +17,8 @@ describe('McpToolExceptionFilter', () => { } catch (error) { return error; } + + throw new Error('The filter gave no error'); } beforeEach(() => { @@ -43,16 +44,6 @@ describe('McpToolExceptionFilter', () => { expect(logError).not.toHaveBeenCalled(); }); - it('Passes on the error of an RpcException', async () => { - const exception = new RpcException('The access cannot be resolved'); - - expect(await getErrorOfException(exception)).toBe( - 'The access cannot be resolved' - ); - - expect(logError).not.toHaveBeenCalled(); - }); - it('Hides the message of an unexpected error and writes it to the log', async () => { const exception = new Error( 'Unique constraint failed on the fields: (dataSource)' @@ -66,11 +57,15 @@ describe('McpToolExceptionFilter', () => { expect(logError).toHaveBeenCalledWith(exception); }); - it('Gives the reason phrase of the status of an HttpException', async () => { + // An access without the scope of a tool causes a refused call at each + // attempt, which would fill the log + it('Gives the reason phrase of the status of an HttpException and writes no log', async () => { expect(await getErrorOfException(new ForbiddenException())).toEqual({ message: getReasonPhrase(StatusCodes.FORBIDDEN), status: 'error' }); + + expect(logError).not.toHaveBeenCalled(); }); it('Gives the reason phrase of a service which is not available if a snapshot cannot be computed', async () => { @@ -82,5 +77,7 @@ describe('McpToolExceptionFilter', () => { message: getReasonPhrase(StatusCodes.SERVICE_UNAVAILABLE), status: 'error' }); + + expect(logError).toHaveBeenCalledWith(exception); }); }); diff --git a/apps/api/src/filters/mcp-tool-exception.filter.ts b/apps/api/src/filters/mcp-tool-exception.filter.ts index 4ed3233ce..6630c1c1c 100644 --- a/apps/api/src/filters/mcp-tool-exception.filter.ts +++ b/apps/api/src/filters/mcp-tool-exception.filter.ts @@ -7,7 +7,6 @@ import { Logger, RpcExceptionFilter } from '@nestjs/common'; -import { RpcException } from '@nestjs/microservices'; import { getReasonPhrase, StatusCodes } from 'http-status-codes'; import { Observable, throwError } from 'rxjs'; @@ -21,32 +20,32 @@ export class McpToolExceptionFilter implements RpcExceptionFilter { private readonly logger = new Logger(McpToolExceptionFilter.name); public catch(exception: unknown): Observable { - // The message of these exceptions is written for the caller, hence it is + // The message of this exception is written for the caller, hence it is // passed on and is not written to the log - if (exception instanceof RpcException) { - return throwError(() => { - return exception.getError(); - }); - } - if (exception instanceof CallerFacingError) { return throwError(() => { return { message: exception.message, status: 'error' }; }); } - this.logger.error(exception); + const statusCode = this.getStatus(exception); - return throwError(() => { - return { message: this.getMessage(exception), status: 'error' }; - }); - } + // An exception which the caller causes, for example a refused call, is + // expected, hence only an exception of the application is written to the + // log + if (statusCode >= StatusCodes.INTERNAL_SERVER_ERROR) { + this.logger.error(exception); + } - private getMessage(exception: unknown) { // The message of an exception can carry internals, for example the // property names of a data transfer object of a failed validation, hence // the reason phrase of the status is passed on instead - return this.getReasonPhraseOfStatus(this.getStatus(exception)); + return throwError(() => { + return { + message: this.getReasonPhraseOfStatus(statusCode), + status: 'error' + }; + }); } private getReasonPhraseOfStatus(statusCode: number) {