Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
588 changes: 294 additions & 294 deletions pnpm-lock.yaml

Large diffs are not rendered by default.

1 change: 0 additions & 1 deletion src/api/review-application/reviewApplication.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,6 @@ export class ReviewApplicationService {
}
// make sure application role matches
if (
// eslint-disable-next-line @typescript-eslint/no-unsafe-enum-comparison
ReviewApplicationRoleOpportunityTypeMap[dto.role] !== opportunity.type
) {
throw new BadRequestException(
Expand Down
3 changes: 2 additions & 1 deletion src/api/submission/submission.controller.ts
Original file line number Diff line number Diff line change
Expand Up @@ -197,7 +197,8 @@ export class SubmissionController {
this.logger.log(
`Getting submissions with filters - ${JSON.stringify(queryDto)}`,
);
const authUser: JwtUser = req['user'] as JwtUser;
const authUser: JwtUser =
(req['user'] as JwtUser) ?? ({ isMachine: false, roles: [] } as JwtUser);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[⚠️ correctness]
Using the nullish coalescing operator (??) to provide a default value for authUser is a good approach. However, ensure that this default value is appropriate for all cases where req['user'] might be null or undefined. If there are specific roles or machine states that should be handled differently, consider refining this default value.

return this.service.listSubmission(
authUser,
queryDto,
Expand Down
60 changes: 59 additions & 1 deletion src/api/submission/submission.service.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -510,6 +510,7 @@ describe('SubmissionService', () => {
reviewType: {
findMany: jest.Mock;
};
$queryRaw: jest.Mock;
};
let prismaErrorServiceMock: { handleError: jest.Mock };
let challengePrismaMock: {
Expand Down Expand Up @@ -537,9 +538,14 @@ describe('SubmissionService', () => {
reviewType: {
findMany: jest.fn().mockResolvedValue([]),
},
$queryRaw: jest.fn().mockResolvedValue([]),

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[❗❗ security]
The use of $queryRaw with jest.fn().mockResolvedValue([]) indicates raw SQL queries are being mocked. Ensure that the raw SQL queries are safe from SQL injection vulnerabilities, especially if any user input is involved.

};
prismaErrorServiceMock = {
handleError: jest.fn(),
handleError: jest.fn().mockReturnValue({
message: 'Unexpected error',
code: 'INTERNAL_ERROR',
details: {},
}),
};
challengePrismaMock = {
$queryRaw: jest.fn().mockResolvedValue([]),
Expand Down Expand Up @@ -624,6 +630,7 @@ describe('SubmissionService', () => {
prismaMock.submission.findFirst.mockResolvedValue({
id: 'submission-new',
});
prismaMock.$queryRaw.mockResolvedValue([{ id: 'submission-new' }]);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[❗❗ security]
The use of $queryRaw.mockResolvedValue([{ id: 'submission-new' }]) suggests that raw SQL queries are being executed. Verify that these queries are parameterized to prevent SQL injection attacks.


const result = await listService.listSubmission(
{ isMachine: false } as any,
Expand Down Expand Up @@ -1241,5 +1248,56 @@ describe('SubmissionService', () => {
expect(screeningReview?.reviewerHandle).toBe('screeningHandle');
expect(screeningReview?.reviewerMaxRating).toBe(2000);
});

it('exposes submitter identity but strips reviews for anonymous challenge queries', async () => {
const now = new Date('2025-02-01T12:00:00Z');
const submissions = [
{
id: 'submission-anon',
challengeId: 'challenge-1',
memberId: '101',
submittedDate: now,
createdAt: now,
updatedAt: now,
type: SubmissionType.CONTEST_SUBMISSION,
status: SubmissionStatus.ACTIVE,
review: [{ id: 'review-public', score: 100 }],
reviewSummation: [{ id: 'summation-public' }],
url: 'https://example.com/submission.zip',
legacyChallengeId: null,
prizeId: null,
},
];

prismaMock.submission.findMany.mockResolvedValue(
submissions.map((entry) => ({ ...entry })),
);
prismaMock.submission.count.mockResolvedValue(submissions.length);
prismaMock.submission.findFirst.mockResolvedValue({
id: 'submission-anon',
});

memberPrismaMock.member.findMany.mockResolvedValue([
{
userId: BigInt(101),
handle: 'anonUser',
maxRating: { rating: 1500 },
},
]);

const result = await listService.listSubmission(

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[❗❗ security]
The test case exposes submitter identity but strips reviews for anonymous challenge queries is handling sensitive data. Ensure that the logic for stripping reviews and URLs is correctly implemented in the actual service to prevent accidental data exposure.

{ isMachine: false, roles: [] } as any,
{ challengeId: 'challenge-1' } as any,
{ page: 1, perPage: 10 } as any,
);

const submissionResult = result.data[0];
expect(submissionResult.memberId).toBe('101');
expect(submissionResult.submitterHandle).toBe('anonUser');
expect(submissionResult.submitterMaxRating).toBe(1500);
expect(submissionResult).not.toHaveProperty('review');
expect(submissionResult).not.toHaveProperty('reviewSummation');
expect(submissionResult.url).toBeNull();
});
});
});
121 changes: 92 additions & 29 deletions src/api/submission/submission.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1812,9 +1812,9 @@ export class SubmissionService {
: undefined;

if (requestedMemberId) {
const userId = authUser.userId ? String(authUser.userId) : undefined;
const userId = authUser?.userId ? String(authUser.userId) : undefined;
const isRequestingMember = userId === requestedMemberId;
const hasCopilotRole = (authUser.roles ?? []).includes(
const hasCopilotRole = (authUser?.roles ?? []).includes(
UserRole.Copilot,
);
const hasElevatedAccess = isAdmin(authUser) || hasCopilotRole;
Expand Down Expand Up @@ -1862,7 +1862,7 @@ export class SubmissionService {
: '';

let restrictedChallengeIds = new Set<string>();
if (!isPrivilegedRequester && requesterUserId) {
if (!isPrivilegedRequester && requesterUserId && !queryDto.challengeId) {
try {
restrictedChallengeIds =
await this.getActiveSubmitterRestrictedChallengeIds(
Expand All @@ -1885,7 +1885,8 @@ export class SubmissionService {
if (
!isPrivilegedRequester &&
requesterUserId &&
restrictedChallengeIds.size
restrictedChallengeIds.size &&
!queryDto.challengeId
) {
const restrictedList = Array.from(restrictedChallengeIds);
const restrictionCriteria: Prisma.submissionWhereInput = {
Expand Down Expand Up @@ -1928,12 +1929,16 @@ export class SubmissionService {
orderBy,
});

// Enrich with submitter handle and max rating for authorized callers
const canViewSubmitter = await this.canViewSubmitterIdentity(
authUser,
queryDto.challengeId,
);
if (canViewSubmitter && submissions.length) {
// Enrich with submitter handle and max rating (always for challenge listings)
const shouldEnrichSubmitter =
submissions.length > 0 &&
(queryDto.challengeId
? true
: await this.canViewSubmitterIdentity(
authUser,
queryDto.challengeId,
));
Comment on lines +1933 to +1940
Copy link

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The conditional logic for enriching submitter identity has a bug. When queryDto.challengeId is provided, the code always enriches submitter data (true), bypassing the canViewSubmitterIdentity check entirely. This means anonymous users querying by challengeId will have submissions enriched with submitter handles and ratings, which may be intentional for this feature. However, the fallback path passes queryDto.challengeId to canViewSubmitterIdentity, which will always be undefined in that branch (since if it were defined, the ternary would have taken the true path). This likely should pass undefined explicitly or restructure the logic to be clearer.

Suggested change
const shouldEnrichSubmitter =
submissions.length > 0 &&
(queryDto.challengeId
? true
: await this.canViewSubmitterIdentity(
authUser,
queryDto.challengeId,
));
let shouldEnrichSubmitter = false;
if (submissions.length > 0) {
if (queryDto.challengeId) {
// For challenge listings, always enrich submitter data
shouldEnrichSubmitter = true;
} else {
shouldEnrichSubmitter = await this.canViewSubmitterIdentity(
authUser,
undefined,
);
}
}

Copilot uses AI. Check for mistakes.
if (shouldEnrichSubmitter) {
try {
const memberIds = Array.from(
new Set(
Expand All @@ -1943,11 +1948,24 @@ export class SubmissionService {
),
);
if (memberIds.length) {
const idsAsBigInt = memberIds.map((id) => BigInt(id));
const members = await this.memberPrisma.member.findMany({
where: { userId: { in: idsAsBigInt } },
include: { maxRating: true },
});
const idsAsBigInt: bigint[] = [];
for (const id of memberIds) {
try {
idsAsBigInt.push(BigInt(id));
} catch (error) {
this.logger.debug(
`[listSubmission] Skipping submitter ${id}: unable to convert to BigInt. ${error}`,
);
}
}

const members =
idsAsBigInt.length > 0
? await this.memberPrisma.member.findMany({
where: { userId: { in: idsAsBigInt } },
include: { maxRating: true },
})
: [];
const map = new Map<
string,
{ handle: string; maxRating: number | null }
Expand Down Expand Up @@ -2001,11 +2019,6 @@ export class SubmissionService {
submissions,
reviewVisibilityContext,
);
this.stripSubmitterMemberIds(
authUser,
submissions,
reviewVisibilityContext,
);
await this.stripIsLatestForUnlimitedChallenges(submissions);

this.logger.log(
Expand Down Expand Up @@ -2408,25 +2421,43 @@ export class SubmissionService {
return emptyContext;
}

const requesterUserId =
authUser?.userId !== undefined && authUser?.userId !== null
? String(authUser.userId).trim()
: '';

const isPrivilegedRequester = authUser?.isMachine || isAdmin(authUser);
if (!isPrivilegedRequester && !requesterUserId) {
for (const submission of submissions) {
if (Object.prototype.hasOwnProperty.call(submission, 'review')) {
delete (submission as any).review;
}
if (
Object.prototype.hasOwnProperty.call(submission, 'reviewSummation')
) {
delete (submission as any).reviewSummation;
}
}
return {
...emptyContext,
requesterUserId,
};
}

if (isPrivilegedRequester) {
const requesterUserId =
authUser?.userId !== undefined && authUser?.userId !== null
? String(authUser.userId).trim()
: '';
return {
...emptyContext,
requesterUserId,
};
}

const uid =
authUser?.userId !== undefined && authUser?.userId !== null
? String(authUser.userId).trim()
: '';
const uid = requesterUserId;

if (!uid) {
return emptyContext;
return {
...emptyContext,
requesterUserId,
};
}
Comment on lines 2456 to 2461
Copy link

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unreachable code detected. The condition if (!uid) at line 2456 will never be true because it's checked after a previous condition at line 2430 that handles the case where !isPrivilegedRequester && !requesterUserId. Since uid is assigned from requesterUserId at line 2454, and privileged requesters return early at line 2447-2452, the only way to reach line 2456 is if requesterUserId has a value (making uid truthy).

Copilot uses AI. Check for mistakes.

const challengeIds = Array.from(
Expand Down Expand Up @@ -3397,7 +3428,26 @@ export class SubmissionService {
}

const uid = visibilityContext.requesterUserId;
this.logger.debug(
`[stripSubmitterSubmissionDetails] requesterUserId=${uid ?? '<undefined>'}`,
);
if (!uid) {
this.logger.debug(
'[stripSubmitterSubmissionDetails] Anonymized requester; removing review metadata and URLs.',
);
for (const submission of submissions) {
if (Object.prototype.hasOwnProperty.call(submission, 'review')) {
delete (submission as any).review;
}
if (
Object.prototype.hasOwnProperty.call(submission, 'reviewSummation')
) {
delete (submission as any).reviewSummation;
}
if (Object.prototype.hasOwnProperty.call(submission, 'url')) {
(submission as any).url = null;
}
}
return;
}

Expand Down Expand Up @@ -3472,6 +3522,19 @@ export class SubmissionService {

const uid = visibilityContext.requesterUserId;
if (!uid) {
for (const submission of submissions) {
if (Object.prototype.hasOwnProperty.call(submission, 'review')) {
delete (submission as any).review;
}
if (
Object.prototype.hasOwnProperty.call(submission, 'reviewSummation')
) {
delete (submission as any).reviewSummation;
}
if (Object.prototype.hasOwnProperty.call(submission, 'url')) {
(submission as any).url = null;
}
}
return;
}

Expand Down
3 changes: 2 additions & 1 deletion src/dto/reviewApplication.dto.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { ApiProperty, ApiPropertyOptional } from '@nestjs/swagger';
import type { ReviewOpportunityType as PrismaReviewOpportunityType } from '@prisma/client';
import { IsIn, IsNotEmpty, IsOptional, IsString } from 'class-validator';
import { ReviewOpportunityType } from './reviewOpportunity.dto';

Expand Down Expand Up @@ -37,7 +38,7 @@ export const ReviewApplicationRoleIds: Record<ReviewApplicationRole, number> = {
// read from review_application_role_lu.review_auction_type_id
export const ReviewApplicationRoleOpportunityTypeMap: Record<
ReviewApplicationRole,
ReviewOpportunityType
PrismaReviewOpportunityType

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[❗❗ correctness]
The change from ReviewOpportunityType to PrismaReviewOpportunityType in the ReviewApplicationRoleOpportunityTypeMap may introduce a mismatch if the enum values in PrismaReviewOpportunityType do not align with those in ReviewOpportunityType. Ensure that the values are compatible or provide a mapping if necessary.

> = {
PRIMARY_REVIEWER: ReviewOpportunityType.COMPONENT_DEV_REVIEW,
SECONDARY_REVIEWER: ReviewOpportunityType.COMPONENT_DEV_REVIEW,
Expand Down
13 changes: 12 additions & 1 deletion src/shared/guards/tokenRoles.guard.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ describe('TokenRolesGuard', () => {
type TestRequest = Record<string, unknown> & {
method: string;
query: Record<string, unknown>;
user: {
user?: {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[❗❗ correctness]
Making the user property optional in TestRequest could lead to runtime errors if the code assumes user is always present. Ensure that all usages of request.user handle the case where user is undefined.

userId: string;
isMachine: boolean;
roles?: unknown[];
Expand Down Expand Up @@ -132,5 +132,16 @@ describe('TokenRolesGuard', () => {

expect(() => guard.canActivate(context)).toThrow(ForbiddenException);
});

it('allows anonymous access when challengeId is provided', () => {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[❗❗ security]
The test case allows anonymous access when challengeId is provided introduces a scenario where anonymous access is allowed based on the presence of challengeId. Ensure that this behavior is intentional and does not introduce a security vulnerability by allowing unauthorized access.

const request = {
method: 'GET',
query: { challengeId: '12345' },
};

const context = createExecutionContext(request as TestRequest);

expect(guard.canActivate(context)).toBe(true);
});
});
});
Loading
Loading