PLU-550: Postman attachment size limit check (#1199)

## Address VAPT issues
* Change from PUT to POST presigned URL to enforce size limit
* Set `manualUpload` metadata directly from the server to prevent
client-side modification

## How to test?
- [ ] Upload new attachment, should be able to send via Postman
successfully
- [ ] Previously uploaded attachments should be sent via Postman
succesfully
- [ ] Attempt to override by uploading a different file to the generated
presigned POST URL should fail

Author: kevinkim-ogp

package-lock.json

@@ -32,7 +32,7 @@
     "@as-integrations/express4": "1.1.2",
     "@aws-sdk/client-dynamodb": "3.460.0",
     "@aws-sdk/client-s3": "3.369.0",
-    "@aws-sdk/s3-request-presigner": "3.369.0",
+    "@aws-sdk/s3-presigned-post": "3.369.0",
     "@bull-board/express": "5.17.0",
     "@graphql-tools/schema": "10.0.25",
     "@graphql-tools/utils": "10.9.1",

packages/backend/package.json

@@ -2,7 +2,7 @@ import { beforeEach, describe, expect, it, vi } from 'vitest'
 
 import { ForbiddenError } from '@/errors/graphql-errors'
 import { generateMockContext } from '@/graphql/__tests__/mutations/tiles/table.mock'
-import generatePresignedUrl from '@/graphql/mutations/generate-presigned-url'
+import generatePresignedPost from '@/graphql/mutations/generate-presigned-post'
 import Flow from '@/models/flow'
 import Context from '@/types/express/context'
 
@@ -12,59 +12,76 @@ const VALID_PARAMS = {
   fileType: 'text/plain',
   size: 100,
   updatedAt: new Date().toISOString(),
-  manualUpload: true,
 }
 
-const mocks = vi.hoisted(() => ({
-  getSignedUrl: vi.fn(),
+// Mock the s3 helpers module
+vi.mock('@/helpers/s3', () => ({
+  getPresignedPost: vi.fn(),
+  COMMON_S3_BUCKET: 'test-bucket',
+  COMMON_S3_MOCK_FOLDER_PREFIX: 's3:test-bucket:mock/',
+  parseS3Id: vi.fn(),
+  MAX_FILE_SIZE: 1024 * 1024 * 2,
+  ACCEPTED_FILE_TYPES: ['text/plain'],
+  validateObjectKey: vi.fn((objectKey) => {
+    const invalidCharacters = /[\\{}^`%~#<>|[\]]/
+    if (invalidCharacters.test(objectKey)) {
+      return false
+    }
+
+    // validate length of object key
+    const byteLength = Buffer.byteLength(objectKey, 'utf-8')
+    return byteLength <= 1024
+  }),
 }))
 
-vi.mock('@aws-sdk/s3-request-presigner', () => ({
-  getSignedUrl: mocks.getSignedUrl,
-}))
+import { COMMON_S3_BUCKET, getPresignedPost } from '@/helpers/s3'
 
-describe('generatePresignedUrl', () => {
+describe('generatePresignedPost', () => {
   let context: Context
   beforeEach(async () => {
     context = await generateMockContext()
+    vi.clearAllMocks()
   })
 
   it('should generate a presigned url', async () => {
-    const expectedUrl = 'https://presigned-url.example.com'
-    mocks.getSignedUrl.mockResolvedValueOnce(expectedUrl)
-
     await Flow.query().insert({
       id: VALID_PARAMS.flowId,
       name: 'Test Flow',
       userId: context.currentUser.id,
     })
 
-    const result = await generatePresignedUrl(
-      null,
-      { input: VALID_PARAMS },
-      context,
+    await generatePresignedPost(null, { input: VALID_PARAMS }, context)
+    expect(getPresignedPost).toHaveBeenCalledWith(
+      COMMON_S3_BUCKET,
+      expect.stringMatching(
+        new RegExp(
+          `^${VALID_PARAMS.flowId}/[a-f0-9-]+/${VALID_PARAMS.filename}$`,
+        ),
+      ),
+      VALID_PARAMS.fileType,
+      {
+        flowId: VALID_PARAMS.flowId,
+        filename: VALID_PARAMS.filename,
+        size: VALID_PARAMS.size.toString(),
+        updatedAt: VALID_PARAMS.updatedAt,
+      },
     )
-    const expectedKeys = ['url', 's3Id']
-    expect(Object.keys(result)).toEqual(expectedKeys)
-    expect(result.s3Id).toContain(VALID_PARAMS.flowId)
   })
 
   it('should throw an error if the user does not have access to the flow', async () => {
-    vi.fn()
-      .mockRejectedValue(Flow.hasAccess)
-      .mockRejectedValue(
-        new ForbiddenError('You do not have access to this flow'),
-      )
+    const otherUserContext = await generateMockContext()
+    await Flow.query()
+      .patch({
+        userId: otherUserContext.currentUser.id,
+      })
+      .where('id', VALID_PARAMS.flowId)
 
     await expect(
-      generatePresignedUrl(null, { input: VALID_PARAMS }, context),
+      generatePresignedPost(null, { input: VALID_PARAMS }, context),
     ).rejects.toThrow(ForbiddenError)
   })
 
   it('should throw an error if the file size is too large', async () => {
-    const expectedUrl = 'https://presigned-url.example.com'
-    mocks.getSignedUrl.mockResolvedValueOnce(expectedUrl)
-
     await Flow.query().insert({
       id: VALID_PARAMS.flowId,
       name: 'Test Flow',
@@ -73,7 +90,7 @@ describe('generatePresignedUrl', () => {
 
     const tooLargeParams = { ...VALID_PARAMS, size: 2 * 1024 * 1024 + 1 }
     await expect(
-      generatePresignedUrl(null, { input: tooLargeParams }, context),
+      generatePresignedPost(null, { input: tooLargeParams }, context),
     ).rejects.toThrow('Size of attachment exceeds 2MB')
   })
 
@@ -85,9 +102,6 @@ describe('generatePresignedUrl', () => {
   ])(
     'should throw an error if the file type is not supported: %s',
     async (fileType) => {
-      const expectedUrl = 'https://presigned-url.example.com'
-      mocks.getSignedUrl.mockResolvedValueOnce(expectedUrl)
-
       await Flow.query().insert({
         id: VALID_PARAMS.flowId,
         name: 'Test Flow',
@@ -100,7 +114,7 @@ describe('generatePresignedUrl', () => {
       }
 
       await expect(
-        generatePresignedUrl(null, { input: unsupportedParams }, context),
+        generatePresignedPost(null, { input: unsupportedParams }, context),
       ).rejects.toThrow('Unsupported file type')
     },
   )

packages/backend/src/graphql/__tests__/mutations/generate-presigned-url.itest.ts renamed to packages/backend/src/graphql/__tests__/mutations/generate-presigned-post.itest.ts

@@ -13,7 +13,7 @@ import deleteUploadedFile from './mutations/delete-uploaded-file'
 import duplicateFlow from './mutations/duplicate-flow'
 import executeStep from './mutations/execute-step'
 import generateAuthUrl from './mutations/generate-auth-url'
-import generatePresignedUrl from './mutations/generate-presigned-url'
+import generatePresignedPost from './mutations/generate-presigned-post'
 import loginWithSelectedSgid from './mutations/login-with-selected-sgid'
 import loginWithSgid from './mutations/login-with-sgid'
 import loginWithSso from './mutations/login-with-sso'
@@ -82,7 +82,7 @@ export default {
   updateFlowTransferStatus,
   duplicateFlow,
   deleteUploadedFile,
-  generatePresignedUrl,
+  generatePresignedPost,
   ...tilesMutationResolvers,
 
   // This is a special stub that enables us to group all our admin-related

packages/backend/src/graphql/mutation-resolvers.ts

@@ -0,0 +1,52 @@
+import { PresignedPost } from '@aws-sdk/s3-presigned-post'
+import { randomUUID } from 'crypto'
+
+import {
+  ACCEPTED_FILE_TYPES,
+  COMMON_S3_BUCKET,
+  getPresignedPost,
+  MAX_FILE_SIZE,
+  validateObjectKey,
+} from '@/helpers/s3'
+import Flow from '@/models/flow'
+
+import { MutationResolvers } from '../__generated__/types.generated'
+
+const generatePresignedPost: MutationResolvers['generatePresignedPost'] =
+  async (_parent, params, context) => {
+    const { flowId, filename, fileType, size, updatedAt } = params.input
+
+    if (size > MAX_FILE_SIZE) {
+      throw new Error('Size of attachment exceeds 2MB')
+    }
+    if (!ACCEPTED_FILE_TYPES.includes(fileType)) {
+      throw new Error('Unsupported file type')
+    }
+
+    const uuid = randomUUID()
+    const objectKey = `${flowId}/${uuid}/${filename}`
+    if (!validateObjectKey(objectKey)) {
+      throw new Error('File path cannot be longer than 1024 bytes')
+    }
+
+    await Flow.hasAccess(context.currentUser.id, flowId)
+
+    const presignedPost = await getPresignedPost(
+      COMMON_S3_BUCKET,
+      objectKey,
+      fileType,
+      {
+        flowId,
+        filename,
+        size: size.toString(),
+        updatedAt,
+      },
+    )
+
+    return {
+      presignedPost: presignedPost as PresignedPost,
+      s3Id: `s3:${COMMON_S3_BUCKET}:${objectKey}`,
+    }
+  }
+
+export default generatePresignedPost