fix: enforce S3 limits and reject negative/fractionals (#1237)

Signed-off-by: ferhat elmas <elmas.ferhat@gmail.com>

Author: ferhatelmas

src/http/routes/s3/commands/complete-multipart-upload.ts

@@ -40,7 +40,7 @@ const CompletedMultipartUpload = {
             items: {
               type: 'object',
               properties: {
-                PartNumber: { type: 'integer' },
+                PartNumber: { type: 'integer', minimum: 1, maximum: 10000 },
                 ETag: { type: 'string' },
               },
               required: ['PartNumber', 'ETag'],

src/http/routes/s3/commands/list-multipart-uploads.ts

@@ -2,8 +2,8 @@ import { S3ProtocolHandler } from '@storage/protocols/s3/s3-handler'
 import { ROUTE_OPERATIONS } from '../../operations'
 import { S3Router } from '../router'
 
-const ListObjectsInput = {
-  summary: 'List Objects',
+const ListMultipartUploadsInput = {
+  summary: 'List Multipart Uploads',
   Params: {
     type: 'object',
     properties: {
@@ -17,7 +17,7 @@ const ListObjectsInput = {
       uploads: { type: 'string' },
       delimiter: { type: 'string' },
       'encoding-type': { type: 'string', enum: ['url'] },
-      'max-uploads': { type: 'number', minimum: 1 },
+      'max-uploads': { type: 'integer', minimum: 1, maximum: 1000 },
       'key-marker': { type: 'string' },
       'upload-id-marker': { type: 'string' },
       prefix: { type: 'string' },
@@ -29,7 +29,7 @@ const ListObjectsInput = {
 export default function ListMultipartUploads(s3Router: S3Router) {
   s3Router.get(
     '/:Bucket?uploads',
-    { schema: ListObjectsInput, operation: ROUTE_OPERATIONS.S3_LIST_MULTIPART },
+    { schema: ListMultipartUploadsInput, operation: ROUTE_OPERATIONS.S3_LIST_MULTIPART },
     async (req, ctx) => {
       const s3Protocol = new S3ProtocolHandler(ctx.storage, ctx.tenantId, ctx.owner)
 

src/http/routes/s3/commands/list-objects.ts

@@ -17,7 +17,7 @@ const ListObjectsV2Input = {
       'list-type': { type: 'string', enum: ['2'] },
       delimiter: { type: 'string' },
       'encoding-type': { type: 'string', enum: ['url'] },
-      'max-keys': { type: 'number' },
+      'max-keys': { type: 'integer', minimum: 0 },
       prefix: { type: 'string' },
       'continuation-token': { type: 'string' },
       'start-after': { type: 'string' },
@@ -40,7 +40,7 @@ const ListObjectsInput = {
     properties: {
       delimiter: { type: 'string' },
       'encoding-type': { type: 'string', enum: ['url'] },
-      'max-keys': { type: 'number' },
+      'max-keys': { type: 'integer', minimum: 0 },
       prefix: { type: 'string' },
       marker: { type: 'string' },
     },

src/http/routes/s3/commands/list-parts.ts

@@ -18,7 +18,7 @@ const ListPartsInput = {
     type: 'object',
     properties: {
       uploadId: { type: 'string' },
-      'max-parts': { type: 'number', minimum: 1, maximum: 1000 },
+      'max-parts': { type: 'integer', minimum: 1, maximum: 1000 },
       'part-number-marker': { type: 'string' },
     },
     required: ['uploadId'],

src/http/routes/s3/commands/upload-part-copy.ts

@@ -16,7 +16,7 @@ const UploadPartCopyInput = {
     type: 'object',
     properties: {
       uploadId: { type: 'string' },
-      partNumber: { type: 'number', minimum: 1, maximum: 1000 },
+      partNumber: { type: 'integer', minimum: 1, maximum: 10000 },
     },
     required: ['uploadId', 'partNumber'],
   },

src/http/routes/s3/commands/upload-part.ts

@@ -19,7 +19,7 @@ const UploadPartInput = {
     type: 'object',
     properties: {
       uploadId: { type: 'string' },
-      partNumber: { type: 'number', minimum: 1, maximum: 10000 },
+      partNumber: { type: 'integer', minimum: 1, maximum: 10000 },
     },
     required: ['uploadId', 'partNumber'],
   },

src/http/routes/s3/router.test.ts

@@ -4,6 +4,11 @@ import { vi } from 'vitest'
 import { S3ProtocolHandler } from '../../../storage/protocols/s3/s3-handler'
 import { Uploader } from '../../../storage/uploader'
 import CompleteMultipartUpload from './commands/complete-multipart-upload'
+import ListMultipartUploads from './commands/list-multipart-uploads'
+import ListObjects from './commands/list-objects'
+import ListParts from './commands/list-parts'
+import UploadPart from './commands/upload-part'
+import UploadPartCopy from './commands/upload-part-copy'
 import { getRouter, type RouteQuery, Router, type S3Router } from './router'
 
 afterEach(() => {
@@ -665,6 +670,36 @@ describe('S3ProtocolHandler.parseMetadataHeaders', () => {
 })
 
 describe('CompleteMultipartUpload route mapping', () => {
+  it.each([
+    ['', false],
+    [0, false],
+    [1, true],
+    [10_000, true],
+    [10_001, false],
+  ])('validates body PartNumber %s against the S3 range', (partNumber, expected) => {
+    const router = new Router()
+    CompleteMultipartUpload(router as unknown as S3Router)
+
+    const route = router
+      .routes()
+      .get('/:Bucket/*')
+      ?.find((candidate) => candidate.method === 'post' && candidate.type === undefined)
+
+    expect(route).toBeDefined()
+    expect(
+      route!.validate({
+        Params: { Bucket: 'bucket', '*': 'object' },
+        Querystring: { uploadId: 'upload-id' },
+        Headers: { authorization: 'authorization' },
+        Body: {
+          CompleteMultipartUpload: {
+            Part: [{ PartNumber: partNumber, ETag: 'etag' }],
+          },
+        },
+      })
+    ).toBe(expected)
+  })
+
   it('maps ChecksumCRC32C from the backend response on iceberg routes', async () => {
     const router = new Router()
     const completeMultipartUpload = vi.fn().mockResolvedValue({
@@ -732,6 +767,148 @@ describe('CompleteMultipartUpload route mapping', () => {
   })
 })
 
+describe('UploadPart route validation', () => {
+  it.each([
+    [0, false],
+    [1, true],
+    ['1', true],
+    [1.5, false],
+    ['1.5', false],
+    [10_000, true],
+    [10_001, false],
+  ])('validates query partNumber %s as an integer within the S3 range', (partNumber, expected) => {
+    const router = new Router()
+    UploadPart(router as unknown as S3Router)
+
+    const route = router
+      .routes()
+      .get('/:Bucket/*')
+      ?.find((candidate) => candidate.method === 'put' && candidate.type === undefined)
+
+    expect(route).toBeDefined()
+    expect(
+      route!.validate({
+        Params: { Bucket: 'bucket', '*': 'object' },
+        Querystring: { uploadId: 'upload-id', partNumber },
+      })
+    ).toBe(expected)
+  })
+})
+
+describe('UploadPartCopy route validation', () => {
+  it.each([
+    [0, false],
+    [1, true],
+    ['1', true],
+    [1.5, false],
+    ['1.5', false],
+    [10_000, true],
+    [10_001, false],
+  ])('validates query partNumber %s as an integer within the S3 range', (partNumber, expected) => {
+    const router = new Router()
+    UploadPartCopy(router as unknown as S3Router)
+
+    const route = router.routes().get('/:Bucket/*')?.[0]
+
+    expect(route).toBeDefined()
+    expect(
+      route!.validate({
+        Params: { Bucket: 'bucket', '*': 'object' },
+        Querystring: { uploadId: 'upload-id', partNumber },
+        Headers: { 'x-amz-copy-source': '/source-bucket/source-key' },
+      })
+    ).toBe(expected)
+  })
+})
+
+describe('ListParts route validation', () => {
+  it.each([
+    [0, false],
+    [1, true],
+    ['1', true],
+    [1.5, false],
+    ['1.5', false],
+    [1_000, true],
+    [1_001, false],
+  ])('validates max-parts %s as an integer within the S3 range', (maxParts, expected) => {
+    const router = new Router()
+    ListParts(router as unknown as S3Router)
+
+    const route = router
+      .routes()
+      .get('/:Bucket/*')
+      ?.find((candidate) => candidate.method === 'get' && candidate.type === undefined)
+
+    expect(route).toBeDefined()
+    expect(
+      route!.validate({
+        Params: { Bucket: 'bucket', '*': 'object' },
+        Querystring: { uploadId: 'upload-id', 'max-parts': maxParts },
+      })
+    ).toBe(expected)
+  })
+})
+
+describe('ListMultipartUploads route validation', () => {
+  it.each([
+    [0, false],
+    [1, true],
+    [1.5, false],
+    [1_000, true],
+    [1_001, false],
+  ])('validates max-uploads %s against the S3 range', (maxUploads, expected) => {
+    const router = new Router()
+    ListMultipartUploads(router as unknown as S3Router)
+
+    const route = router.routes().get('/:Bucket')?.[0]
+
+    expect(route).toBeDefined()
+    expect(
+      route!.validate({
+        Params: { Bucket: 'bucket' },
+        Querystring: { uploads: '', 'max-uploads': maxUploads },
+      })
+    ).toBe(expected)
+  })
+})
+
+describe.each([
+  ['V1', false],
+  ['V2', true],
+])('ListObjects %s route validation', (_version, isV2) => {
+  it.each([
+    [-1, false],
+    [0, true],
+    [1, true],
+    ['1', true],
+    [1.5, false],
+    ['1.5', false],
+  ])('validates max-keys %s as a non-negative integer', (maxKeys, expected) => {
+    const router = new Router()
+    ListObjects(router as unknown as S3Router)
+
+    const route = router
+      .routes()
+      .get('/:Bucket')
+      ?.find((candidate) =>
+        isV2
+          ? candidate.querystringMatches.some((match) => match.key === 'list-type')
+          : candidate.querystringMatches.some((match) => match.key === '*')
+      )
+
+    expect(route).toBeDefined()
+    expect(
+      route!.validate({
+        Params: { Bucket: 'bucket' },
+        Querystring: {
+          ...(isV2 ? { 'list-type': '2' } : {}),
+          'max-keys': maxKeys,
+        },
+      })
+    ).toBe(expected)
+  })
+})
+
 describe('DeleteObject route mapping', () => {
   it('accepts DeleteObjects payloads at the object request cap in router validation', async () => {
     const { default: DeleteObject } = await import('./commands/delete-object')

src/storage/protocols/s3/s3-handler.test.ts

@@ -157,6 +157,56 @@ describe('S3ProtocolHandler.getObject', () => {
   })
 })
 
+describe('S3ProtocolHandler.listMultipartUploads', () => {
+  it('defaults MaxUploads to the S3 limit of 1000', async () => {
+    const findBucket = vi.fn().mockResolvedValue({ id: 'bucket' })
+    const listMultipartUploads = vi.fn().mockResolvedValue([])
+    const storage = {
+      asSuperUser: vi.fn(() => ({ findBucket })),
+      db: { listMultipartUploads },
+    }
+    const handler = new S3ProtocolHandler(storage as never, 'tenant-id')
+
+    const response = await handler.listMultipartUploads({ Bucket: 'bucket' })
+
+    expect(listMultipartUploads).toHaveBeenCalledWith('bucket', {
+      prefix: '',
+      deltimeter: undefined,
+      maxKeys: 1001,
+      nextUploadKeyToken: undefined,
+      nextUploadToken: undefined,
+    })
+    expect(response.responseBody.ListMultipartUploadsResult.MaxUploads).toBe(1000)
+  })
+
+  it.each([
+    0,
+    -1,
+    1.5,
+    1001,
+    Number.NaN,
+    Number.POSITIVE_INFINITY,
+    Number.NEGATIVE_INFINITY,
+  ])('rejects invalid MaxUploads %s before querying storage', async (maxUploads) => {
+    const findBucket = vi.fn().mockResolvedValue({ id: 'bucket' })
+    const listMultipartUploads = vi.fn().mockResolvedValue([])
+    const storage = {
+      asSuperUser: vi.fn(() => ({ findBucket })),
+      db: { listMultipartUploads },
+    }
+    const handler = new S3ProtocolHandler(storage as never, 'tenant-id')
+
+    await expect(
+      handler.listMultipartUploads({ Bucket: 'bucket', MaxUploads: maxUploads })
+    ).rejects.toMatchObject({
+      code: 'InvalidParameter',
+      message: 'Invalid Parameter MaxUploads',
+    })
+    expect(findBucket).not.toHaveBeenCalled()
+    expect(listMultipartUploads).not.toHaveBeenCalled()
+  })
+})
+
 describe('S3ProtocolHandler.abortMultipartUpload', () => {
   it('aborts multipart upload and deletes from database when backend succeeds', async () => {
     const uploadId = 'test-upload-id'

src/storage/protocols/s3/s3-handler.ts

@@ -289,6 +289,11 @@ export class S3ProtocolHandler {
       throw ERRORS.MissingParameter('Bucket')
     }
 
+    const limit = command.MaxUploads ?? 1000
+    if (!Number.isInteger(limit) || limit < 1 || limit > 1000) {
+      throw ERRORS.InvalidParameter('MaxUploads')
+    }
+
     await this.storage.asSuperUser().findBucket(command.Bucket)
 
     const keyContinuationToken = command.KeyMarker
@@ -297,11 +302,8 @@ export class S3ProtocolHandler {
     const encodingType = command.EncodingType
     const delimiter = command.Delimiter
     const prefix = command.Prefix || ''
-    const maxKeys = command.MaxUploads
     const bucket = command.Bucket
 
-    const limit = maxKeys || 200
-
     const multipartUploads = await this.storage.db.listMultipartUploads(bucket, {
       prefix,
       deltimeter: delimiter,