fix: s3 delete objects improve validation (#505)

Author: fenos

src/http/plugins/xml.ts

@@ -10,12 +10,17 @@ import xmlBodyParser from 'fastify-xml-body-parser'
 
 export const jsonToXml = fastifyPlugin(async function (
   fastify: FastifyInstance,
-  opts: { disableContentParser?: boolean }
+  opts: { disableContentParser?: boolean; parseAsArray?: string[] }
 ) {
   fastify.register(accepts)
 
   if (!opts.disableContentParser) {
-    fastify.register(xmlBodyParser)
+    fastify.register(xmlBodyParser, {
+      contentType: ['text/xml', 'application/xml', '*'],
+      isArray: (_: string, jpath: string) => {
+        return opts.parseAsArray?.includes(jpath)
+      },
+    })
   }
   fastify.addHook('preSerialization', async (req, res, payload) => {
     const accept = req.accepts()

src/http/routes/s3/index.ts

@@ -1,8 +1,9 @@
 import { FastifyInstance, RouteHandlerMethod } from 'fastify'
+import { JSONSchema } from 'json-schema-to-ts'
+import { trace } from '@opentelemetry/api'
 import { db, jsonToXml, signatureV4, storage, tracingMode } from '../../plugins'
-import { getRouter, RequestInput } from './router'
+import { findArrayPathsInSchemas, getRouter, RequestInput } from './router'
 import { s3ErrorHandler } from './error-handler'
-import { trace } from '@opentelemetry/api'
 
 export default async function routes(fastify: FastifyInstance) {
   fastify.register(async (fastify) => {
@@ -102,6 +103,9 @@ export default async function routes(fastify: FastifyInstance) {
 
           fastify.register(jsonToXml, {
             disableContentParser,
+            parseAsArray: findArrayPathsInSchemas(
+              routesByMethod.filter((r) => r.schema.Body).map((r) => r.schema.Body as JSONSchema)
+            ),
           })
           fastify.register(signatureV4)
           fastify.register(db)

src/http/routes/s3/router.ts

@@ -144,7 +144,7 @@ export class Router<Context = unknown, S extends Schema = Schema> {
       schemaToCompile.Params = schema.Params
     }
     if (schema.Body) {
-      schemaToCompile.Body
+      schemaToCompile.Body = schema.Body
     }
     if (schema.Headers) {
       schemaToCompile.Headers = schema.Headers
@@ -154,10 +154,25 @@ export class Router<Context = unknown, S extends Schema = Schema> {
       schemaToCompile.Querystring = schema.Querystring
     }
 
+    // If any of the keys has a required property, then the top level object is also required
+    const required = Object.keys(schemaToCompile).map((key) => {
+      const k = key as keyof typeof schemaToCompile
+      const schemaObj = schemaToCompile[k]
+
+      if (typeof schemaObj === 'boolean') {
+        return
+      }
+
+      if (schemaObj?.required && schemaObj.required.length > 0) {
+        return key as string
+      }
+    })
+
     this.ajv.addSchema(
       {
         type: 'object',
         properties: schemaToCompile,
+        required: required.filter(Boolean),
       },
       method + url
     )
@@ -271,3 +286,32 @@ export class Router<Context = unknown, S extends Schema = Schema> {
     return false
   }
 }
+
+/**
+ * Given a JSONSchema Definition, it returns dotted paths of all array properties
+ * @param schemas
+ */
+export function findArrayPathsInSchemas(schemas: JSONSchema[]): string[] {
+  const arrayPaths: string[] = []
+
+  function traverse(schema: JSONSchema, currentPath = ''): void {
+    if (typeof schema === 'boolean') {
+      return
+    }
+
+    if (schema.type === 'array') {
+      arrayPaths.push(currentPath)
+    }
+
+    if (schema.properties) {
+      for (const key in schema.properties) {
+        const nextSchema = schema.properties[key]
+        traverse(nextSchema, currentPath ? `${currentPath}.${key}` : key)
+      }
+    }
+  }
+
+  schemas.forEach((schema) => traverse(schema))
+
+  return arrayPaths
+}

src/storage/errors.ts

@@ -34,6 +34,7 @@ export enum ErrorCode {
   DatabaseError = 'DatabaseError',
   MissingContentLength = 'MissingContentLength',
   MissingParameter = 'MissingParameter',
+  InvalidParameter = 'InvalidParameter',
   InvalidUploadSignature = 'InvalidUploadSignature',
   LockTimeout = 'LockTimeout',
   S3Error = 'S3Error',
@@ -89,6 +90,14 @@ export const ERRORS = {
       originalError: e,
     }),
 
+  InvalidParameter: (parameter: string, e?: Error) =>
+    new StorageBackendError({
+      code: ErrorCode.MissingParameter,
+      httpStatusCode: 400,
+      message: `Invalid Parameter ${parameter}`,
+      originalError: e,
+    }),
+
   InvalidJWT: (e?: Error) =>
     new StorageBackendError({
       code: ErrorCode.InvalidJWT,

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

@@ -891,8 +891,8 @@ export class S3ProtocolHandler {
       throw ERRORS.MissingParameter('Delete')
     }
 
-    if (!Delete.Objects) {
-      throw ERRORS.MissingParameter('Objects')
+    if (!Array.isArray(Delete.Objects)) {
+      throw ERRORS.InvalidParameter('Objects')
     }
 
     if (Delete.Objects.length === 0) {
@@ -903,28 +903,23 @@ export class S3ProtocolHandler {
       .from(Bucket)
       .deleteObjects(Delete.Objects.map((o) => o.Key || ''))
 
+    const deleted = Delete.Objects.filter((o) => deletedResult.find((d) => d.name === o.Key)).map(
+      (o) => ({ Key: o.Key })
+    )
+
+    const errors = Delete.Objects.filter((o) => !deletedResult.find((d) => d.name === o.Key)).map(
+      (o) => ({
+        Key: o.Key,
+        Code: 'AccessDenied',
+        Message: "You do not have permission to delete this object or the object doesn't exists",
+      })
+    )
+
     return {
       responseBody: {
         DeletedResult: {
-          Deleted: Delete.Objects.map((o) => {
-            const isDeleted = deletedResult.find((d) => d.name === o.Key)
-            if (isDeleted) {
-              return {
-                Deleted: {
-                  Key: o.Key,
-                },
-              }
-            }
-
-            return {
-              Error: {
-                Key: o.Key,
-                Code: 'AccessDenied',
-                Message:
-                  "You do not have permission to delete this object or the object doesn't exists",
-              },
-            }
-          }),
+          Deleted: deleted,
+          Error: errors,
         },
       },
     }

src/test/s3-protocol.test.ts

@@ -701,6 +701,37 @@ describe('S3 Protocol', () => {
     })
 
     describe('DeleteObjectsCommand', () => {
+      it('can delete a single object', async () => {
+        const bucketName = await createBucket(client)
+        await Promise.all([uploadFile(client, bucketName, 'test-1.jpg', 1)])
+
+        const deleteObjectsCommand = new DeleteObjectsCommand({
+          Bucket: bucketName,
+          Delete: {
+            Objects: [
+              {
+                Key: 'test-1.jpg',
+              },
+            ],
+          },
+        })
+
+        const deleteResp = await client.send(deleteObjectsCommand)
+
+        expect(deleteResp.Deleted).toEqual([
+          {
+            Key: 'test-1.jpg',
+          },
+        ])
+
+        const listObjectsCommand = new ListObjectsV2Command({
+          Bucket: bucketName,
+        })
+
+        const resp = await client.send(listObjectsCommand)
+        expect(resp.Contents).toBe(undefined)
+      })
+
       it('can delete multiple objects', async () => {
         const bucketName = await createBucket(client)
         await Promise.all([
@@ -726,7 +757,70 @@ describe('S3 Protocol', () => {
           },
         })
 
-        await client.send(deleteObjectsCommand)
+        const deleteResp = await client.send(deleteObjectsCommand)
+
+        expect(deleteResp.Deleted).toEqual([
+          {
+            Key: 'test-1.jpg',
+          },
+          {
+            Key: 'test-2.jpg',
+          },
+          {
+            Key: 'test-3.jpg',
+          },
+        ])
+
+        const listObjectsCommand = new ListObjectsV2Command({
+          Bucket: bucketName,
+        })
+
+        const resp = await client.send(listObjectsCommand)
+        expect(resp.Contents).toBe(undefined)
+      })
+
+      it('try to delete multiple objects that dont exists', async () => {
+        const bucketName = await createBucket(client)
+
+        await uploadFile(client, bucketName, 'test-1.jpg', 1)
+
+        const deleteObjectsCommand = new DeleteObjectsCommand({
+          Bucket: bucketName,
+          Delete: {
+            Objects: [
+              {
+                Key: 'test-1.jpg',
+              },
+              {
+                Key: 'test-2.jpg',
+              },
+              {
+                Key: 'test-3.jpg',
+              },
+            ],
+          },
+        })
+
+        const deleteResp = await client.send(deleteObjectsCommand)
+        expect(deleteResp.Deleted).toEqual([
+          {
+            Key: 'test-1.jpg',
+          },
+        ])
+        expect(deleteResp.Errors).toEqual([
+          {
+            Key: 'test-2.jpg',
+            Code: 'AccessDenied',
+            Message:
+              "You do not have permission to delete this object or the object doesn't exists",
+          },
+          {
+            Key: 'test-3.jpg',
+            Code: 'AccessDenied',
+            Message:
+              "You do not have permission to delete this object or the object doesn't exists",
+          },
+        ])
 
         const listObjectsCommand = new ListObjectsV2Command({
           Bucket: bucketName,