PM-1437 Feat/auth #4
Conversation
PM-1437 - Use canActivate for tools
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| export const decodeAuthToken = async ( | ||
| authHeader: string, | ||
| ): Promise<boolean | jwt.JwtPayload | string> => { | ||
| const [type, idToken] = authHeader?.split(' ') ?? []; |
There was a problem hiding this comment.
high
correctness
The use of optional chaining (authHeader?.split(' ')) may lead to a runtime error if authHeader is undefined. Consider adding a check for authHeader before attempting to split it.
| 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 an error if the token is expired or invalid. While the error is caught, consider handling specific error types to provide more informative logging or responses.
| const signingKey = await getSigningKey(idToken); | ||
| decoded = jwt.verify(idToken, signingKey); | ||
| } catch (error) { | ||
| logger.error('Error verifying JWT', error); |
There was a problem hiding this comment.
high
security
Logging the error object directly may expose sensitive information. Consider sanitizing the error message before logging.
| export const checkM2MScope = | ||
| (...requiredM2mScopes: M2mScope[]) => | ||
| async (req: Request) => { | ||
| const decodedAuth = 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 input for decodeAuthToken. Consider handling cases where the authorization header is missing more explicitly, potentially returning an error or a specific response.
| 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 claim is split by spaces, which assumes that scopes are space-separated. Ensure this assumption aligns with the JWT format used in your application, as different formats might use different separators.
| export const checkHasUserRole = | ||
| (...requiredUserRoles: Role[]) => | ||
| 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 returns null or an unexpected structure to avoid runtime errors. This could be done by adding a check after decoding the token.
| const decodedAuth = await decodeAuthToken(req.headers.authorization ?? ''); | ||
|
|
||
| const decodedUserRoles = Object.keys(decodedAuth).reduce((roles, key) => { | ||
| if (key.match(/claims\/roles$/gi)) { |
There was a problem hiding this comment.
medium
correctness
The use of Object.keys(decodedAuth) and reduce may lead to unexpected behavior if decodedAuth is not an object or if the keys do not match the expected pattern. Consider validating the structure of decodedAuth before processing.
| return roles; | ||
| }, []); | ||
|
|
||
| if (!requiredUserRoles.some((role) => decodedUserRoles.includes(role))) { |
There was a problem hiding this comment.
medium
performance
The use of some with includes could be optimized for performance by using a set for decodedUserRoles if the list is large, as includes has O(n) complexity.
| // greeting.tool.ts | ||
| 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
The import path for Tool has changed from @rekog/mcp-nest to @tc/mcp-nest. Ensure that this change is intentional and that the new module provides the same functionality as the previous one.
| 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 and used within the tool. This is crucial for ensuring the security layer is properly enforced.
| }, | ||
| }) | ||
| async queryChallenges(params) { | ||
| private async _queryChallenges(params) { |
There was a problem hiding this comment.
medium
design
Renaming queryChallenges to _queryChallenges suggests it is intended to be private. Ensure that this change aligns with the intended design and usage within the application, as it may affect how this method is accessed or overridden.
|
|
||
| @Tool({ | ||
| name: 'query-tc-challenges-private', | ||
| description: |
There was a problem hiding this comment.
medium
correctness
The description for query-tc-challenges-private mentions public challenges, which seems inconsistent with the tool name suggesting private challenges. Ensure the description accurately reflects the functionality.
|
|
||
| @Tool({ | ||
| name: 'query-tc-challenges-protected', | ||
| description: |
There was a problem hiding this comment.
medium
correctness
The description for query-tc-challenges-protected mentions public challenges, which may not align with the intended functionality for protected challenges. Verify that the description matches the tool's purpose.
| } | ||
|
|
||
| @Tool({ | ||
| name: 'query-tc-challenges-m2m', |
There was a problem hiding this comment.
medium
correctness
The description for query-tc-challenges-m2m mentions public challenges, which might not be accurate for machine-to-machine scope. Confirm that the description is appropriate for the intended use case.
| } | ||
|
|
||
| @Tool({ | ||
| name: 'query-tc-challenges-public', |
There was a problem hiding this comment.
medium
security
Consider adding an authentication guard to query-tc-challenges-public to ensure consistent security practices across all query methods.
| super.error(this.formatMessages(messages)); | ||
| } | ||
|
|
||
| private formatMessages(messages: any[]): string { |
There was a problem hiding this comment.
medium
maintainability
The removal of the requestIdPrefix could impact the ability to trace logs back to specific requests. If this was intentional, consider documenting how request tracing will be handled elsewhere.
2 participants
Adds auth layer to the MCP server: https://topcoder.atlassian.net/browse/PM-1437