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
4 changes: 2 additions & 2 deletions apps/api/src/app/s3.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { GetObjectCommand, S3Client } from '@aws-sdk/client-s3';
import { GetObjectCommand, S3Client, type GetObjectCommandOutput } from '@aws-sdk/client-s3';
import { Logger } from '@nestjs/common';
import '../config/load-env';

Expand Down Expand Up @@ -126,7 +126,7 @@ export async function getFleetAgent({
os,
}: {
os: 'macos' | 'windows' | 'linux';
}) {
}): Promise<GetObjectCommandOutput['Body']> {
if (!s3Client) {
throw new Error('S3 client not configured');
}
Expand Down
11 changes: 11 additions & 0 deletions apps/api/src/attachments/upload-attachment.dto.ts
Original file line number Diff line number Diff line change
Expand Up @@ -69,4 +69,15 @@ export class UploadAttachmentDto {
@IsString()
@MaxLength(500)
description?: string;

@ApiProperty({
description:
'User ID of the user uploading the attachment (required for API key auth, ignored for JWT auth)',
example: 'usr_abc123def456',
required: false,
})
@IsOptional()
@IsString()
@IsNotEmpty()
userId?: string;
}
82 changes: 71 additions & 11 deletions apps/api/src/comments/comments.controller.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,15 +5,14 @@ import {
Controller,
Delete,
Get,
HttpCode,
HttpStatus,
Param,
Post,
Put,
Query,
UseGuards,
} from '@nestjs/common';
import {
ApiBody,
ApiHeader,
ApiOperation,
ApiParam,
Expand All @@ -28,6 +27,7 @@ import type { AuthContext as AuthContextType } from '../auth/types';
import { CommentsService } from './comments.service';
import { CommentResponseDto } from './dto/comment-responses.dto';
import { CreateCommentDto } from './dto/create-comment.dto';
import { DeleteCommentDto } from './dto/delete-comment.dto';
import { UpdateCommentDto } from './dto/update-comment.dto';

@ApiTags('Comments')
Expand Down Expand Up @@ -92,13 +92,28 @@ export class CommentsController {
@AuthContext() authContext: AuthContextType,
@Body() createCommentDto: CreateCommentDto,
): Promise<CommentResponseDto> {
if (!authContext.userId) {
throw new BadRequestException('User ID is required');
// For API key auth, userId must be provided in the request body
// For JWT auth, userId comes from the authenticated session
let userId: string;
if (authContext.isApiKey) {
// For API key auth, userId must be provided in the DTO
if (!createCommentDto.userId) {
throw new BadRequestException(
'User ID is required when using API key authentication. Provide userId in the request body.',
);
}
userId = createCommentDto.userId;
} else {
// For JWT auth, use the authenticated user's ID
if (!authContext.userId) {
throw new BadRequestException('User ID is required');
}
userId = authContext.userId;
}

return await this.commentsService.createComment(
organizationId,
authContext.userId,
userId,
createCommentDto,
);
}
Expand All @@ -124,14 +139,29 @@ export class CommentsController {
@Param('commentId') commentId: string,
@Body() updateCommentDto: UpdateCommentDto,
): Promise<CommentResponseDto> {
if (!authContext.userId) {
throw new BadRequestException('User ID is required');
// For API key auth, userId must be provided in the request body
// For JWT auth, userId comes from the authenticated session
let userId: string;
if (authContext.isApiKey) {
// For API key auth, userId must be provided in the DTO
if (!updateCommentDto.userId) {
throw new BadRequestException(
'User ID is required when using API key authentication. Provide userId in the request body.',
);
}
userId = updateCommentDto.userId;
} else {
// For JWT auth, use the authenticated user's ID
if (!authContext.userId) {
throw new BadRequestException('User ID is required');
}
userId = authContext.userId;
}

return await this.commentsService.updateComment(
organizationId,
commentId,
authContext.userId,
userId,
updateCommentDto.content,
);
}
Expand All @@ -146,6 +176,20 @@ export class CommentsController {
description: 'Unique comment identifier',
example: 'cmt_abc123def456',
})
@ApiBody({
description: 'Delete comment request body',
schema: {
type: 'object',
properties: {
userId: {
type: 'string',
description:
'User ID of the comment author (required for API key auth, ignored for JWT auth)',
example: 'usr_abc123def456',
},
},
},
})
@ApiResponse({
status: 200,
description: 'Comment deleted successfully',
Expand Down Expand Up @@ -198,15 +242,31 @@ export class CommentsController {
@OrganizationId() organizationId: string,
@AuthContext() authContext: AuthContextType,
@Param('commentId') commentId: string,
@Body() deleteDto: DeleteCommentDto,
): Promise<{ success: boolean; deletedCommentId: string; message: string }> {
if (!authContext.userId) {
throw new BadRequestException('User ID is required');
// For API key auth, userId must be provided in the request body
// For JWT auth, userId comes from the authenticated session
let userId: string;
if (authContext.isApiKey) {
Comment on lines 193 to +250
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

DELETE endpoint now requires a request body, which is non-standard for DELETE requests. The OpenAPI spec marks requestBody as required: true, but the DeleteCommentDto has userId as optional. This creates ambiguity:

  1. For JWT auth users, they must send an empty body {} even though userId is ignored
  2. Many HTTP clients, proxies, and tools have poor support for DELETE with body
  3. Some infrastructure may strip bodies from DELETE requests

Impact: JWT authentication users may get errors if their HTTP client doesn't send a body, or if infrastructure strips the body.

Fix: Consider one of these approaches:

// Option 1: Make body truly optional
@Body() deleteDto?: DeleteCommentDto,
// Then handle undefined case

// Option 2: Use query parameter for API key auth
@Query('userId') userId?: string,

// Option 3: Use custom header
Suggested change
@ApiResponse({
status: 200,
description: 'Comment deleted successfully',
})
@Delete(':commentId')
async deleteComment(
@OrganizationId() organizationId: string,
@AuthContext() authContext: AuthContextType,
@Param('commentId') commentId: string,
@Query('userId') userIdParam?: string,
): Promise<{ success: boolean; deletedCommentId: string; message: string }> {
// For API key auth, userId must be provided in the query parameter
// For JWT auth, userId comes from the authenticated session
let userId: string;
if (authContext.isApiKey) {

Spotted by Graphite Agent

Fix in Graphite


Is this helpful? React 👍 or 👎 to let us know.

// For API key auth, userId must be provided in the request body
if (!deleteDto.userId) {
throw new BadRequestException(
'User ID is required when using API key authentication. Provide userId in the request body.',
);
}
userId = deleteDto.userId;
} else {
// For JWT auth, use the authenticated user's ID
if (!authContext.userId) {
throw new BadRequestException('User ID is required');
}
userId = authContext.userId;
}

await this.commentsService.deleteComment(
organizationId,
commentId,
authContext.userId,
userId,
);

return {
Expand Down
11 changes: 11 additions & 0 deletions apps/api/src/comments/dto/create-comment.dto.ts
Original file line number Diff line number Diff line change
Expand Up @@ -49,4 +49,15 @@ export class CreateCommentDto {
@ValidateNested({ each: true })
@Type(() => UploadAttachmentDto)
attachments?: UploadAttachmentDto[];

@ApiProperty({
description:
'User ID of the comment author (required for API key auth, ignored for JWT auth)',
example: 'usr_abc123def456',
required: false,
})
@IsOptional()
@IsString()
@IsNotEmpty()
userId?: string;
}
15 changes: 15 additions & 0 deletions apps/api/src/comments/dto/delete-comment.dto.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
import { ApiProperty } from '@nestjs/swagger';
import { IsNotEmpty, IsOptional, IsString } from 'class-validator';

export class DeleteCommentDto {
@ApiProperty({
description:
'User ID of the comment author (required for API key auth, ignored for JWT auth)',
example: 'usr_abc123def456',
required: false,
})
@IsOptional()
@IsString()
@IsNotEmpty()
userId?: string;
}
13 changes: 12 additions & 1 deletion apps/api/src/comments/dto/update-comment.dto.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { ApiProperty } from '@nestjs/swagger';
import { IsNotEmpty, IsString, MaxLength } from 'class-validator';
import { IsNotEmpty, IsOptional, IsString, MaxLength } from 'class-validator';

export class UpdateCommentDto {
@ApiProperty({
Expand All @@ -11,4 +11,15 @@ export class UpdateCommentDto {
@IsNotEmpty()
@MaxLength(2000)
content: string;

@ApiProperty({
description:
'User ID of the comment author (required for API key auth, ignored for JWT auth)',
example: 'usr_abc123def456',
required: false,
})
@IsOptional()
@IsString()
@IsNotEmpty()
userId?: string;
}
Loading
Loading