-
Notifications
You must be signed in to change notification settings - Fork 2
PROD RELEASE #16
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
PROD RELEASE #16
Conversation
DIsable auth
PM-1437 - Use canActivate for tools
PM-1437 Feat/auth
update pnpm-lock
Feat/tc agent
…nges add more logs
…nges Use axios for fetching challenges
…nges package updated - add axios
export const decodeAuthToken = async ( | ||
authHeader: string, | ||
): Promise<boolean | jwt.JwtPayload | string> => { | ||
const [type, idToken] = authHeader?.split(' ') ?? []; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
high
correctness
The use of optional chaining with authHeader?.split(' ')
can lead to undefined
being destructured into type
and idToken
, which might not be the intended behavior. Consider explicitly checking if authHeader
is defined before splitting.
let decoded: jwt.JwtPayload | string; | ||
try { | ||
const signingKey = await getSigningKey(idToken); | ||
decoded = jwt.verify(idToken, signingKey); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
medium
performance
The jwt.verify
method can throw an error if the token is invalid. While this is caught and handled, consider using jwt.verify
's callback form to avoid using try-catch
for flow control, which can improve readability and performance.
export const checkM2MScope = | ||
(...requiredM2mScopes: M2mScope[]) => | ||
async (req: Request) => { | ||
const decodedAuth = await decodeAuthToken(req.headers.authorization ?? ''); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
high
correctness
Consider handling the case where decodeAuthToken
might throw an error, which could lead to unhandled promise rejections or unexpected behavior.
async (req: Request) => { | ||
const decodedAuth = await decodeAuthToken(req.headers.authorization ?? ''); | ||
|
||
const authorizedScopes = ((decodedAuth as JwtPayload).scope ?? '').split( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
medium
correctness
The use of split(' ')
assumes that scopes are space-separated. Ensure that this assumption aligns with the token structure, or consider making it configurable.
export const checkHasUserRole = | ||
(...requiredUserRoles: Role[]) => | ||
async (req: Request) => { | ||
const decodedAuth = await decodeAuthToken(req.headers.authorization ?? ''); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
high
correctness
Consider handling the case where decodeAuthToken
might throw an error due to invalid or malformed tokens. This could prevent potential runtime exceptions and improve robustness.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
medium
maintainability
The use of Object.keys(decodedAuth).reduce(...)
assumes that the roles are stored in a specific key format. If the structure of decodedAuth
changes, this could lead to incorrect behavior. Consider validating the structure of decodedAuth
or using a more explicit approach to extract roles.
|
||
const decodedUserRoles = Object.keys(decodedAuth).reduce((roles, key) => { | ||
if (key.match(/claims\/roles$/gi)) { | ||
return decodedAuth[key] as string[]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
high
correctness
The type assertion as string[]
could lead to runtime errors if the value is not actually an array of strings. Consider adding type checks or validation to ensure the expected format.
async function bootstrap() { | ||
const app = await NestFactory.create(AppModule); | ||
const app = await NestFactory.create(AppModule, { | ||
logger: ['error', 'warn', 'log', 'debug'], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
high
security
Consider restricting the logger levels in production to avoid potentially sensitive information being logged. For example, you might want to exclude 'debug' and 'log' levels in a production environment.
const logger = new Logger('bootstrap()'); | ||
|
||
// Global prefix for all routes | ||
app.setGlobalPrefix(ENV_CONFIG.API_BASE); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
high
security
Ensure that ENV_CONFIG.API_BASE
is validated and sanitized to prevent any potential security vulnerabilities, such as path traversal attacks.
async getChallengesApiSwagger() { | ||
this.logger.debug('Fetching Challenges V6 API Swagger'); | ||
// Fetch the content from the URI and return it. | ||
const rawContent = await axios.get(SPEC_URL); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
high
correctness
Consider adding error handling for the axios.get
request to manage potential network errors or invalid responses. This will improve the robustness of the method.
import { Logger } from 'src/shared/global'; | ||
import { LogTime } from 'src/shared/global/logTime.decorator'; | ||
|
||
const SPEC_URL = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
medium
maintainability
Consider moving SPEC_URL
to a configuration file or environment variable to allow easier updates and avoid hardcoding URLs directly in the code.
async getIdentityApiSwagger() { | ||
this.logger.debug('Fetching Identity V6 API Swagger'); | ||
// Fetch the content from the URI and return it. | ||
const rawContent = await axios.get(SPEC_URL); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
high
correctness
Add error handling for the axios.get(SPEC_URL)
call to manage potential network errors or invalid responses gracefully.
async getMemberApiSwagger() { | ||
this.logger.debug('Fetching Member V6 API Swagger'); | ||
// Fetch the content from the URI and return it. | ||
const rawContent = await axios.get(SPEC_URL); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
high
correctness
Consider adding error handling for the axios.get
call to manage potential network errors or invalid responses. This will improve the robustness of the method.
async getReviewApiSwagger() { | ||
this.logger.debug('Fetching Review V6 API Swagger'); | ||
// Fetch the content from the URI and return it. | ||
const rawContent = await axios.get(SPEC_URL); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
high
correctness
Consider adding error handling for the axios.get(SPEC_URL)
call to manage potential network errors or unexpected responses. This will improve the robustness of the function.
page: z.number().describe('Current page number in the paginated response'), | ||
nextPage: z | ||
.number() | ||
.nullable() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
medium
correctness
Consider adding a test case to ensure that nextPage
being null
is handled correctly in the application logic. This will help prevent potential runtime errors or unexpected behavior when nextPage
is null
.
name: z.string().describe('Challenge title'), | ||
typeId: z.string().describe('Type identifier for the challenge'), | ||
trackId: z.string().describe('Track identifier for the challenge'), | ||
legacy: z |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
medium
correctness
Consider whether the legacy
object should be required or optional. Making it optional might lead to challenges missing legacy data, which could affect downstream processes relying on this information.
.optional() | ||
.describe('Legacy sub-track identifier for the challenge'), | ||
forumId: z | ||
.number() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
medium
correctness
Ensure that the forumId
being optional and a number does not conflict with any existing logic that expects a non-null value. This could lead to runtime errors if not handled properly.
.optional() | ||
.describe('Legacy forum ID for the challenge'), | ||
directProjectId: z | ||
.number() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
medium
correctness
Verify that the optional directProjectId
does not cause issues in parts of the application that might assume its presence. Consider adding validation or default handling where this field is used.
.optional() | ||
.describe('Legacy confidentiality type for the challenge'), | ||
pureV5Task: z | ||
.boolean() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
medium
correctness
The pureV5Task
flag being optional could lead to ambiguity in logic that checks for this flag. Ensure that the absence of this flag is handled correctly in the application logic.
.number() | ||
.optional() | ||
.describe('Legacy screening scorecard ID for the challenge'), | ||
selfService: z |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
medium
correctness
Consider the implications of the selfService
flag being optional. If this flag is critical for certain operations, ensure that its absence is properly managed.
// Additional metadata fields can be added here | ||
// For example, you can include fields like 'clientId', 'prize', etc | ||
}) | ||
.array( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
medium
correctness
Consider validating the name
and value
fields more strictly if they have specific constraints or expected formats. This can prevent potential issues with incorrect metadata entries.
z.object({ | ||
name: z.string().describe('Constraint name'), | ||
value: z.string().describe('Constraint value'), | ||
value: z.number().describe('Constraint value'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
high
correctness
Changing the type of value
from string
to number
may impact existing data or integrations that expect a string. Ensure that all dependent systems are updated accordingly and validate that this change does not introduce issues with data parsing or handling.
]) | ||
.describe('Current status of the challenge'), | ||
attachments: z | ||
.array(z.object({}).optional()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
medium
correctness
The schema for attachments
is defined as an array of empty objects, which might not be useful. Consider specifying the structure of the objects within the array to ensure data integrity and clarity.
.object({ | ||
totalPrizes: z.number().describe('Total prize amount'), | ||
types: z.string().describe('Challenge prizes currency'), | ||
type: z.string().optional().describe('Challenge prizes currency'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
low
readability
Consider renaming type
to currencyType
or prizeCurrencyType
for clarity, as type
is a generic term and may lead to confusion about what it represents.
type: z.string().optional().describe('Challenge prizes currency'), | ||
totalPrizesInCents: z | ||
.number() | ||
.optional() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
medium
correctness
Ensure that the optional totalPrizesInCents
field is handled correctly in downstream logic, as its absence could lead to unexpected behavior if not properly checked.
.string() | ||
.describe('Winner handle on Topcoder platform'), | ||
userId: z.string().describe('Unique identifier for the user'), | ||
userId: z.number().describe('Unique identifier for the user'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
high
correctness
Changing userId
from string
to number
could lead to issues if there are existing user IDs stored as strings, especially if they contain non-numeric characters. Ensure that all user IDs are numeric and that this change won't break existing functionality.
.number() | ||
.describe('Current phase duration in seconds'), | ||
scheduledStartDate: z | ||
.string() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
medium
correctness
The scheduledStartDate
is described as a string in ISO format. Consider validating the format explicitly to ensure it adheres to ISO standards, which can prevent potential parsing errors.
'Scheduled start date of the current phase (ISO format)', | ||
), | ||
scheduledEndDate: z | ||
.string() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
medium
correctness
Similar to scheduledStartDate
, ensure scheduledEndDate
is validated for ISO format compliance to avoid parsing issues.
.optional() | ||
.describe('Description of the current phase (optional)'), | ||
actualStartDate: z | ||
.string() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
medium
correctness
Consider adding validation for actualStartDate
to ensure it is in ISO format, even though it's optional, to maintain consistency and prevent parsing errors.
.string() | ||
.optional() | ||
.describe('Identifier of the predecessor phase (optional)'), | ||
actualEndDate: z |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
medium
correctness
Ensure actualEndDate
is validated for ISO format compliance, similar to other date fields, to prevent parsing errors.
.describe('Indicates if the task is assigned'), | ||
memberId: z | ||
.number() | ||
.describe('Member ID of the assigned user (optional)'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
medium
correctness
The memberId
is described as a number but is marked as optional. If memberId
can be undefined
, ensure that any logic using this field can handle undefined
values appropriately.
.describe( | ||
'Filter by tag name, case-insensitive, partial matches are allowed.', | ||
), | ||
tags: z |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
medium
performance
Consider validating the array length to ensure it doesn't exceed a reasonable number of tags, as excessively large arrays could impact performance.
// 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
high
correctness
Ensure that the import path @tc/mcp-nest
is correct and intended. If this is a new package or module, verify that it is properly configured and available in the production environment.
}, | ||
}) | ||
async queryChallenges(params) { | ||
private async _queryChallenges(params) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
high
correctness
Changing the method queryChallenges
to _queryChallenges
makes it private, which is a good practice for encapsulation. However, ensure that there are no external dependencies relying on this method being public, as it could break functionality elsewhere.
); | ||
|
||
if (!challenges.ok) { | ||
if (challenges.status < 200 || challenges.status >= 300) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
medium
maintainability
The condition challenges.status < 200 || challenges.status >= 300
is correct for checking HTTP error statuses, but consider using a helper function or a constant to improve readability and maintainability.
`Failed to fetch challenges from Topcoder API: ${challenges.statusText}`, | ||
); | ||
try { | ||
this.logger.error(challenges.data); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
medium
correctness
Logging challenges.data
without checking its existence or format could lead to runtime errors if challenges.data
is undefined or not a string. Consider adding a check or formatting the log output.
}; | ||
} catch (error) { | ||
this.logger.error(`Error fetching challenges: ${error.message}`); | ||
this.logger.error(`Error fetching challenges: ${error.message}`, error); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
medium
maintainability
Consider logging the error stack trace or additional error details to aid in debugging, rather than just the error message.
}, | ||
}) | ||
@LogTime('ChallengesTool') | ||
async queryChallenges(params) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
medium
maintainability
The params
parameter lacks a type definition. Consider specifying a type to improve type safety and maintainability.
.optional() | ||
.describe('Filter by skill names, exact match.'), | ||
skillId: z | ||
.array(z.string().uuid()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
medium
correctness
Using uuid()
for skillId
assumes all IDs are valid UUIDs. If IDs can be non-UUIDs, consider using string()
instead and validate UUIDs separately.
perPage: z | ||
.number() | ||
.gte(1) | ||
.lte(100) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
medium
performance
The upper limit of perPage
is set to 100. Ensure this limit is appropriate for the expected data load and performance considerations.
// Get the challenges from the Topcoder challenges API | ||
// and handle any errors that may occur | ||
try { | ||
const accessToken = this.request.headers['authorization']?.split(' ')[1]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
high
correctness
Consider checking if the authorization
header exists before attempting to split it. This will prevent potential runtime errors if the header is missing.
accessToken, | ||
); | ||
|
||
if (skills.status < 200 || skills.status >= 300) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
medium
correctness
The check for skills.status
should use a more specific range or handle specific status codes to better manage different types of HTTP errors (e.g., 4xx vs 5xx).
{ | ||
type: 'text', | ||
text: JSON.stringify({ | ||
page: Number(skills.headers['x-page']) || 1, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
low
maintainability
Consider using a utility function to parse headers and default values to improve readability and maintainability, as this logic is repeated multiple times.
@@ -0,0 +1,32 @@ | |||
import { Logger } from '@nestjs/common'; | |||
|
|||
export function LogTime(label?: string) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
low
maintainability
Consider specifying the type for the label
parameter to improve type safety and clarity.
|
||
return function ( | ||
target: any, | ||
propertyKey: string, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
medium
maintainability
Consider using more specific types instead of any
for target
and args
to enhance type safety and maintainability.
) { | ||
const originalMethod = descriptor.value; | ||
|
||
descriptor.value = async function (...args: any[]) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
medium
correctness
The use of async
here assumes that originalMethod
is always asynchronous. Consider handling both synchronous and asynchronous methods to make the decorator more robust.
? [`{${this.store.requestId}}`] | ||
: []; | ||
return [...requestIdPrefix, ...messages] | ||
return [...messages] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
high
correctness
Removing the requestIdPrefix
could lead to loss of context in logs, especially if requestId
is used for tracing requests across systems. Consider whether this change impacts the ability to trace logs effectively.
|
||
next(); | ||
} | ||
} No newline at end of file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
low
style
Consider adding a newline at the end of the file to adhere to POSIX standards, which can help prevent issues with some tools and version control systems.
import { Logger } from 'src/shared/global'; | ||
import { QUERY_CHALLENGES_TOOL_PARAMETERS } from 'src/mcp/tools/challenges/queryChallenges.parameters'; | ||
import { z } from 'zod'; | ||
import axios from 'axios'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
medium
maintainability
Consider using a specific instance of axios
with custom configuration rather than the default import. This can help manage base URLs, headers, and interceptors more effectively, improving maintainability and security.
if (value !== undefined && value !== null) { | ||
url.searchParams.append(key, value.toString()); | ||
if (Array.isArray(value)) { | ||
value.forEach((v) => url.searchParams.append(`${key}[]`, v)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
medium
correctness
Appending array values to URL parameters using the format key[]
is a common practice, but ensure that the server-side logic correctly interprets this format. If the server expects a different format, this could lead to unexpected behavior.
|
||
this.logger.debug(`Fetching challenges from: ${url.toString()}`); | ||
const stringUrl = url.toString(); | ||
this.logger.log(`Fetching challenges from: "${stringUrl}"`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
medium
maintainability
Changing the log level from debug
to log
may increase the verbosity of logs in production. Ensure that this change is intentional and that the increased log level is acceptable for production environments.
headers, | ||
}); | ||
try { | ||
const response = await axios.get(stringUrl, { headers }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
high
correctness
Consider using axios.get(url.toString(), { headers })
instead of axios.get(stringUrl, { headers })
to ensure consistency with the URL construction logic.
return response; | ||
} catch (error) { | ||
this.logger.error( | ||
`Error fetching challenges: ${JSON.stringify(error)}`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
medium
readability
Using JSON.stringify(error)
may not provide meaningful information for all error types. Consider logging error.message
or error.toString()
for better clarity.
return response; | ||
} catch (error) { | ||
this.logger.error( | ||
`Error fetching challenges: ${JSON.stringify(error)}`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
medium
readability
Consider using error.message
instead of JSON.stringify(error)
for logging the error message. JSON.stringify
may not provide useful information for Error objects, as it does not include non-enumerable properties like message
and stack
.
|
||
try { | ||
const response = await axios.get(stringUrl, { headers }); | ||
return response; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
medium
design
The function returns the entire Axios response object. Consider returning only the necessary data (e.g., response.data
) to avoid exposing unnecessary information and to simplify the function's interface.
TC MCP to production.