Feat/auth #3
Feat/auth #3
Conversation
DIsable auth
PM-1437 - Use canActivate for tools
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| version: 2.1 | ||
| defaults: &defaults | ||
| docker: | ||
| - image: cimg/python:3.13.5-browsers |
There was a problem hiding this comment.
high
correctness
The Docker image cimg/python:3.13.5-browsers does not exist. Consider using a valid version or checking for typos in the version number.
| install_deploysuite: &install_deploysuite | ||
| name: Installation of install_deploysuite. | ||
| command: | | ||
| git clone --branch v1.4.17 https://github.com/topcoder-platform/tc-deploy-scripts ../buildscript |
There was a problem hiding this comment.
medium
maintainability
Cloning a specific branch from a public repository without pinning to a commit hash can lead to unexpected changes if the branch is updated. Consider using a commit hash to ensure consistency.
| - deploy: | ||
| name: Running MasterScript. | ||
| command: | | ||
| ./awsconfiguration.sh $DEPLOY_ENV |
There was a problem hiding this comment.
high
security
The script awsconfiguration.sh is executed with a variable $DEPLOY_ENV that is not validated within this context. Ensure that $DEPLOY_ENV is sanitized to prevent potential security issues.
| source awsenvconf | ||
| ./psvar-processor.sh -t appenv -p /config/${APPNAME}/deployvar | ||
| source deployvar_env | ||
| ./master_deploy.sh -d ECS -e $DEPLOY_ENV -t latest -j /config/${APPNAME}/appvar -i ${APPNAME} -p FARGATE |
There was a problem hiding this comment.
high
security
The master_deploy.sh script is executed with several environment variables. Ensure these variables are properly validated and sanitized to prevent injection vulnerabilities.
| docker buildx build --no-cache=true -t ${APPNAME}:latest . |
There was a problem hiding this comment.
high
correctness
There is a typo in the variable substitution syntax. The closing bracket should be removed. It should be ${APPNAME}:latest instead of ${APPNAME}}:latest.
| specifier: ^1.6.2 | ||
| version: 1.6.2(@modelcontextprotocol/[email protected])(@nestjs/[email protected]([email protected])([email protected])([email protected])([email protected]))(@nestjs/[email protected])([email protected])([email protected])([email protected]([email protected]))([email protected]) | ||
| '@tc/mcp-nest': | ||
| specifier: topcoder-platform/MCP-Nest.git |
There was a problem hiding this comment.
medium
maintainability
Switching from a versioned package to a Git repository URL for @tc/mcp-nest could lead to potential issues with version consistency and reproducibility. Consider specifying a version tag or commit hash to ensure stability and avoid unexpected changes in dependencies.
| @@ -842,8 +842,9 @@ packages: | |||
| resolution: {integrity: sha512-YLT9Zo3oNPJoBjBc4q8G2mjU4tqIbf5CEOORbUUr48dCD9q3umJ3IPlVqOqDakPfd2HuwccBaqlGhN4Gmr5OWg==} | |||
| engines: {node: ^12.20.0 || ^14.18.0 || >=16.0.0} | |||
|
|
|||
| '@rekog/[email protected]': | |||
| resolution: {integrity: sha512-swcu99a/woQ5T1f4E/YJct1w/xzszH83bhzEuZwXh+cYRnQUJKhaJd5Aey/XyOtt/D9lQTKSdnoKdpija8kuWA==} | |||
| '@rekog/mcp-nest@https://codeload.github.com/topcoder-platform/MCP-Nest/tar.gz/8950083c999f7636fed9f62b26330748aed9ed30': | |||
There was a problem hiding this comment.
medium
maintainability
Switching from a versioned package to a tarball URL may affect reproducibility and dependency management. Consider using a versioned package to ensure consistent builds and easier updates.
| @@ -4386,7 +4387,7 @@ snapshots: | |||
|
|
|||
| '@pkgr/[email protected]': {} | |||
|
|
|||
| '@rekog/mcp-nest@1.6.2(@modelcontextprotocol/[email protected])(@nestjs/[email protected]([email protected])([email protected])([email protected])([email protected]))(@nestjs/[email protected])([email protected])([email protected])([email protected]([email protected]))([email protected])': | |||
| '@rekog/mcp-nest@https://codeload.github.com/topcoder-platform/MCP-Nest/tar.gz/8950083c999f7636fed9f62b26330748aed9ed30(@modelcontextprotocol/[email protected])(@nestjs/[email protected]([email protected])([email protected])([email protected])([email protected]))(@nestjs/[email protected])([email protected])([email protected])([email protected]([email protected]))([email protected])': | |||
There was a problem hiding this comment.
medium
maintainability
The change from a versioned package to a direct GitHub tarball URL could impact reproducibility and maintainability. Consider using a specific version tag or commit hash to ensure consistent builds.
| @@ -19,7 +19,7 @@ export class HealthCheckController { | |||
|
|
|||
| @Public() | |||
| @Version([VERSION_NEUTRAL, '1']) | |||
| @Get('/healthcheck') | |||
| @Get('/health') | |||
There was a problem hiding this comment.
medium
maintainability
The endpoint path has been changed from /healthcheck to /health. Ensure that any clients or documentation referencing the old path are updated accordingly to prevent potential disruptions.
| @@ -1,14 +1,11 @@ | |||
| import { MiddlewareConsumer, Module, NestModule } from '@nestjs/common'; | |||
| import { McpModule } from '@rekog/mcp-nest'; | |||
| import { McpModule } from '@tc/mcp-nest'; | |||
There was a problem hiding this comment.
high
correctness
The import path change from @rekog/mcp-nest to @tc/mcp-nest should be verified to ensure that the new module path is correct and that all dependencies are properly updated. Incorrect module paths can lead to runtime errors.
| import { CreateRequestStoreMiddleware } from './core/request/createRequestStore.middleware'; | ||
| import { AuthGuard, RolesGuard } from './core/auth/guards'; | ||
| import { APP_GUARD } from '@nestjs/core'; | ||
| import { nanoid } from 'nanoid'; |
There was a problem hiding this comment.
medium
security
The change from randomUUID to nanoid should be reviewed to ensure that nanoid provides the necessary uniqueness and security properties required by the application. If randomUUID was chosen for its cryptographic properties, ensure nanoid is a suitable replacement.
| @@ -17,30 +14,18 @@ import { APP_GUARD } from '@nestjs/core'; | |||
| version: '1.0.0', | |||
| streamableHttp: { | |||
| enableJsonResponse: false, | |||
| sessionIdGenerator: () => randomUUID(), | |||
| sessionIdGenerator: () => nanoid(), | |||
There was a problem hiding this comment.
high
security
Switching from randomUUID() to nanoid() for session ID generation may have implications on the uniqueness and security of the IDs. Ensure nanoid() provides sufficient entropy and uniqueness for your use case.
| }) | ||
| export class AppModule implements NestModule { | ||
| configure(consumer: MiddlewareConsumer) { | ||
| // consumer.apply(TokenValidatorMiddleware).forRoutes('*'); | ||
| // consumer.apply(CreateRequestStoreMiddleware).forRoutes('*'); | ||
| consumer.apply(TokenValidatorMiddleware).forRoutes('*'); |
There was a problem hiding this comment.
medium
correctness
Re-enabling TokenValidatorMiddleware without CreateRequestStoreMiddleware might lead to issues if the latter is required for proper request handling. Verify if CreateRequestStoreMiddleware is necessary for the application's functionality.
|
|
||
| @IsString() | ||
| @IsOptional() | ||
| API_BASE = '/v6/mcp'; |
There was a problem hiding this comment.
medium
maintainability
Consider using a constant or configuration file for the default value of API_BASE to enhance maintainability and avoid hardcoding paths directly in the class.
| @@ -1,5 +1,8 @@ | |||
| export enum Role { | |||
| Admin = 'administrator', | |||
There was a problem hiding this comment.
medium
maintainability
Consider using a consistent naming convention for enum values. The Admin value uses a different naming style compared to User. This could lead to confusion and maintenance challenges in the future.
| * @returns A promise that resolves to `true` if the authorization token is valid, otherwise `false`. | ||
| */ | ||
| export const authGuard = async (req: Request) => { | ||
| if (!(await decodeAuthToken(req.headers.authorization ?? ''))) { |
There was a problem hiding this comment.
high
correctness
The use of await without a try-catch block could lead to unhandled promise rejections if decodeAuthToken throws an error. Consider wrapping this in a try-catch to handle potential errors gracefully.
| * @returns A promise that resolves to `true` if the authorization token is valid, otherwise `false`. | ||
| */ | ||
| export const authGuard = async (req: Request) => { | ||
| if (!(await decodeAuthToken(req.headers.authorization ?? ''))) { |
There was a problem hiding this comment.
medium
correctness
The use of req.headers.authorization ?? '' assumes that an empty string is a valid fallback. If decodeAuthToken does not handle empty strings appropriately, this could lead to unexpected behavior. Ensure that decodeAuthToken can handle this case or provide a more suitable default value.
| export const decodeAuthToken = async ( | ||
| authHeader: string, | ||
| ): Promise<boolean | jwt.JwtPayload | string> => { | ||
| const [type, idToken] = authHeader?.split(' ') ?? []; |
There was a problem hiding this comment.
low
readability
Consider using a more descriptive variable name than idToken to avoid confusion with ID tokens, as this variable represents a JWT access token.
| let decoded: jwt.JwtPayload | string; | ||
| try { | ||
| const signingKey = await getSigningKey(idToken); | ||
| decoded = jwt.verify(idToken, signingKey); |
There was a problem hiding this comment.
medium
maintainability
The jwt.verify function can throw errors for various reasons, including token expiration and invalid signature. Consider handling specific error types to provide more informative logging and potentially different handling strategies.
| @@ -1,2 +1,3 @@ | |||
| export * from './auth.guard'; | |||
| export * from './roles.guard'; | |||
| export * from './m2m-scope.guard'; | |||
There was a problem hiding this comment.
high
correctness
Ensure that the new m2m-scope.guard module is correctly implemented and tested, as it replaces the previous roles.guard. This change could impact the authorization logic if not properly handled.
| @@ -1,2 +1,3 @@ | |||
| export * from './auth.guard'; | |||
| export * from './roles.guard'; | |||
| export * from './m2m-scope.guard'; | |||
| export * from './role.guard'; | |||
There was a problem hiding this comment.
high
security
Verify that the new role.guard module is correctly integrated and does not introduce any regressions or security issues. Ensure it is thoroughly tested.
| export const checkM2MScope = | ||
| (...requiredM2mScopes: M2mScope[]) => | ||
| async (req: Request) => { | ||
| const decodedAuth = await decodeAuthToken(req.headers.authorization ?? ''); |
There was a problem hiding this comment.
high
correctness
Consider handling the case where decodeAuthToken might throw an error, such as when the token is invalid or malformed. This will prevent unhandled promise rejections and improve the robustness of the function.
| async (req: Request) => { | ||
| const decodedAuth = await decodeAuthToken(req.headers.authorization ?? ''); | ||
|
|
||
| const authorizedScopes = ((decodedAuth as JwtPayload).scope ?? '').split( |
There was a problem hiding this comment.
medium
correctness
The scope property is assumed to be a string. Ensure that decodedAuth is always a JwtPayload and that scope is a string to avoid runtime errors. Consider adding type checks or assertions.
| export const checkHasUserRole = | ||
| (...requiredUserRoles: Role[]) => | ||
| async (req: Request) => { | ||
| const decodedAuth = await decodeAuthToken(req.headers.authorization ?? ''); |
There was a problem hiding this comment.
high
correctness
The decodeAuthToken function is called with req.headers.authorization ?? ''. If the authorization header is missing, this will pass an empty string to decodeAuthToken. Ensure that decodeAuthToken can handle an empty string appropriately, or add a check to handle missing authorization headers before calling this function.
| async (req: Request) => { | ||
| const decodedAuth = await decodeAuthToken(req.headers.authorization ?? ''); | ||
|
|
||
| const decodedUserRoles = Object.keys(decodedAuth).reduce((roles, key) => { |
There was a problem hiding this comment.
medium
correctness
The reduce function is used to accumulate roles, but it returns the first matching claims/roles value found. If there are multiple claims with roles, only the last one will be used. Consider whether this behavior is intended or if you need to accumulate roles from multiple claims.
| return roles; | ||
| }, []); | ||
|
|
||
| if (!requiredUserRoles.some((role) => decodedUserRoles.includes(role))) { |
There was a problem hiding this comment.
medium
correctness
The some method is used to check if any required role is present in decodedUserRoles. If decodedUserRoles is empty due to a missing or malformed token, this will always return false. Ensure that the function handles cases where decodedUserRoles is unexpectedly empty.
| @@ -4,10 +4,15 @@ import { Logger } from 'src/shared/global'; | |||
| import { ENV_CONFIG } from './config'; | |||
|
|
|||
| async function bootstrap() { | |||
| const app = await NestFactory.create(AppModule); | |||
| const app = await NestFactory.create(AppModule, { | |||
| logger: ['error', 'warn', 'log'], | |||
There was a problem hiding this comment.
low
maintainability
Consider configuring the logger to include 'debug' level logs if detailed information is needed during development or troubleshooting.
|
|
||
| const logger = new Logger('bootstrap()'); | ||
|
|
||
| // Global prefix for all routes | ||
| app.setGlobalPrefix(ENV_CONFIG.API_BASE); |
There was a problem hiding this comment.
high
security
Ensure that ENV_CONFIG.API_BASE is correctly defined and sanitized to prevent potential routing issues or security vulnerabilities.
| import { Injectable, Inject } from '@nestjs/common'; | ||
| import { Tool } from '@rekog/mcp-nest'; | ||
| import { Tool } from '@tc/mcp-nest'; |
There was a problem hiding this comment.
high
correctness
Ensure that the import path @tc/mcp-nest is correct and intended, as it differs from the previous @rekog/mcp-nest. This change could affect module resolution if the package structure has not been updated accordingly.
| import { REQUEST } from '@nestjs/core'; | ||
| import { QUERY_CHALLENGES_TOOL_PARAMETERS } from './queryChallenges.parameters'; | ||
| import { TopcoderChallengesService } from 'src/shared/topcoder/challenges.service'; | ||
| import { Logger } from 'src/shared/global'; | ||
| import { QUERY_CHALLENGES_TOOL_OUTPUT_SCHEMA } from './queryChallenges.output'; | ||
| import { |
There was a problem hiding this comment.
high
security
Consider verifying that the newly imported authentication guards (authGuard, checkHasUserRole, checkM2MScope) are correctly integrated into the tool's logic. Without proper integration, the intended authentication layer may not function as expected.
| }, | ||
| }) | ||
| async queryChallenges(params) { | ||
| private async _queryChallenges(params) { |
There was a problem hiding this comment.
medium
maintainability
Renaming queryChallenges to _queryChallenges suggests it's intended to be private, but TypeScript does not enforce privacy for methods prefixed with an underscore. Consider using the private keyword for actual privacy enforcement.
|
|
||
| @Tool({ | ||
| name: 'query-tc-challenges-private', | ||
| description: |
There was a problem hiding this comment.
medium
readability
The description field for query-tc-challenges-private mentions 'public Topcoder challenges', which seems incorrect given the method name implies private challenges. Ensure the description accurately reflects the functionality to avoid confusion.
| } | ||
|
|
||
| @Tool({ | ||
| name: 'query-tc-challenges-protected', |
There was a problem hiding this comment.
medium
readability
The description field for query-tc-challenges-protected mentions 'public Topcoder challenges', which seems incorrect given the method name implies protected challenges. Ensure the description accurately reflects the functionality to avoid confusion.
| } | ||
|
|
||
| @Tool({ | ||
| name: 'query-tc-challenges-m2m', |
There was a problem hiding this comment.
medium
readability
The description field for query-tc-challenges-m2m mentions 'public Topcoder challenges', which seems incorrect given the method name implies M2M challenges. Ensure the description accurately reflects the functionality to avoid confusion.
| ? [`{${this.store.requestId}}`] | ||
| : []; | ||
| return [...requestIdPrefix, ...messages] | ||
| return [...messages] |
There was a problem hiding this comment.
medium
maintainability
Removing the requestIdPrefix could impact traceability of logs if requestId was used to correlate log messages with specific requests. Consider whether this change affects the ability to debug or audit logs effectively.
|
Wrong base. |
Adds auth layer to the MCP server:
https://topcoder.atlassian.net/browse/PM-1437