Submissions performance #173
Conversation
… affect dev and prod
…h the submissions endpoint
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| } | ||
|
|
||
| const payload = this.buildReviewSummationTsv(results); | ||
| const completeResults = await this.loadAllReviewSummationsForExport( |
There was a problem hiding this comment.
[performance]
The method loadAllReviewSummationsForExport is called without checking if completeResults is needed. Consider checking if totalCount is greater than currentCount before calling this method to avoid unnecessary database queries, which could improve performance.
| initialResults.meta?.totalCount ?? initialResults.data.length; | ||
| const currentCount = initialResults.data.length; | ||
|
|
||
| if (!Number.isFinite(totalCount) || totalCount <= currentCount) { |
There was a problem hiding this comment.
[correctness]
The check if (!Number.isFinite(totalCount) || totalCount <= currentCount) is used to determine if additional data should be fetched. Ensure that totalCount is correctly set in all cases, as relying on initialResults.data.length might not always reflect the total available records, potentially leading to incomplete data exports.
| validIssuers: | ||
| process.env.VALID_ISSUERS || | ||
| '["https://testsachin.topcoder-dev.com/","https://test-sachin-rs256.auth0.com/","https://api.topcoder.com","https://api.topcoder-dev.com","https://topcoder-dev.auth0.com/", "https://auth.topcoder-dev.com/"]', | ||
| '["https://testsachin.topcoder-dev.com/","https://test-sachin-rs256.auth0.com/","https://api.topcoder.com","https://api.topcoder-dev.com","https://topcoder-dev.auth0.com/","https://auth.topcoder-dev.com/","https://topcoder.auth0.com/","https://auth.topcoder.com/"]', |
There was a problem hiding this comment.
[❗❗ security]
Consider validating the new issuers added to the validIssuers list to ensure they are trusted and necessary. Adding unnecessary or untrusted issuers could pose a security risk.
|
|
||
| @Injectable() | ||
| export class TokenRolesGuard implements CanActivate { | ||
| private readonly logger = LoggerService.forRoot(TokenRolesGuard.name); |
There was a problem hiding this comment.
[design]
Consider using dependency injection for LoggerService instead of calling forRoot. This would improve testability and adhere to the dependency inversion principle.
| } | ||
|
|
||
| private buildRequestLogMeta( | ||
| request: any, |
There was a problem hiding this comment.
[maintainability]
The buildRequestLogMeta method uses any for the request parameter. Consider using a more specific type to improve type safety and maintainability.
| }, | ||
| }; | ||
|
|
||
| const waitLogger = setInterval(() => { |
There was a problem hiding this comment.
[maintainability]
The setInterval used for logging slow validation warnings should be cleared in all code paths to prevent potential memory leaks or unintended behavior. Ensure clearInterval(waitLogger) is called in every exit path of the promise, including error handling.
| return Array.from(expandedScopes); | ||
| } | ||
|
|
||
| private anonymizeToken(token: string): string { |
There was a problem hiding this comment.
[security]
The anonymizeToken method uses SHA-256 to hash the token and then truncates it to 16 characters. Consider whether this truncation could lead to hash collisions, which might affect logging accuracy. If full anonymity isn't required, consider logging more of the hash or using a different approach to ensure uniqueness.
| } | ||
| | undefined { | ||
| try { | ||
| const decodedRaw = jwt.decode(token, { complete: true }); |
There was a problem hiding this comment.
[❗❗ security]
The decodeTokenMetadata function uses jwt.decode without verifying the token's signature. This could lead to processing invalid or tampered tokens. Consider using jwt.verify to ensure the token is valid before decoding it.
| } | ||
| } | ||
|
|
||
| private getValidIssuersList(): string[] { |
There was a problem hiding this comment.
[correctness]
The getValidIssuersList method attempts to parse a string as JSON to extract issuers. If the input is not intended to be JSON, this could lead to unexpected behavior. Consider validating the input format more strictly or documenting the expected input format.
| } | ||
|
|
||
| const [type, idToken] = request.headers.authorization.split(' ') ?? []; | ||
| const [type, idToken] = normalizedAuthHeader.split(' ') ?? []; |
There was a problem hiding this comment.
[💡 correctness]
The use of normalizedAuthHeader.split(' ') ?? [] is redundant because split will always return an array. Consider removing the nullish coalescing operator.
| }); | ||
| return next(); | ||
| } | ||
|
|
There was a problem hiding this comment.
[correctness]
Consider adding a check for idToken after splitting the normalizedAuthHeader. If idToken is undefined or empty, it might lead to unexpected behavior in validateToken and subsequent operations.
| }; | ||
| } | ||
|
|
||
| private anonymizeToken(token?: string): string | undefined { |
There was a problem hiding this comment.
[security]
The anonymizeToken method uses SHA-256 to hash the token and then slices the hash to 16 characters. Ensure that this level of anonymization is sufficient for your security requirements, as it may not fully anonymize the token.
JWT validation fixes, and /v6/submissions endpoint performance.