Allow special characters as object key

- Modify the method isValidKey to allow special characters - Create a test case verifying that special characters can be used as object key
supabase · Sep 24, 2024 · adf5767 · adf5767
1 parent 4281329
commit adf5767
Show file tree

Hide file tree

Showing 14 changed files with 2,564 additions and 2,622 deletions.
diff --git a/.eslintrc.js b/.eslintrc.js
@@ -6,4 +6,9 @@ module.exports = {
     ecmaVersion: 2020, // Allows for the parsing of modern ECMAScript features
     sourceType: 'module', // Allows for the use of imports
   },
+  rules: {
+    '@typescript-eslint/no-explicit-any': 'warn',
+    '@typescript-eslint/no-unused-vars': 'warn',
+    '@typescript-eslint/no-require-imports': 'warn',
+  },
 }
diff --git a/package-lock.json b/package-lock.json
diff --git a/package.json b/package.json
@@ -25,9 +25,9 @@
     "node": ">= 14.0.0"
   },
   "dependencies": {
-    "@aws-sdk/client-s3": "3.633.0",
-    "@aws-sdk/lib-storage": "3.633.0",
-    "@aws-sdk/s3-request-presigner": "3.633.0",
+    "@aws-sdk/client-s3": "3.654.0",
+    "@aws-sdk/lib-storage": "3.654.0",
+    "@aws-sdk/s3-request-presigner": "3.654.0",
     "@fastify/accepts": "^4.3.0",
     "@fastify/multipart": "^8.3.0",
     "@fastify/rate-limit": "^7.6.0",
@@ -93,8 +93,8 @@
     "@types/pg": "^8.6.4",
     "@types/stream-buffers": "^3.0.7",
     "@types/xml2js": "^0.4.14",
-    "@typescript-eslint/eslint-plugin": "^5.12.1",
-    "@typescript-eslint/parser": "^5.12.1",
+    "@typescript-eslint/eslint-plugin": "^8.7.0",
+    "@typescript-eslint/parser": "^8.7.0",
     "babel-jest": "^29.2.2",
     "esbuild": "0.21.5",
     "eslint": "^8.9.0",
@@ -113,7 +113,7 @@
     "ts-node-dev": "^1.1.8",
     "tsx": "^4.16.0",
     "tus-js-client": "^3.1.0",
-    "typescript": "^5.6.2"
+    "typescript": "5.2.2"
   },
   "bin": "./dist/server.js"
 }
diff --git a/src/config.ts b/src/config.ts
@@ -26,8 +26,6 @@ type StorageConfigType = {
   storageS3ForcePathStyle?: boolean
   storageS3Region: string
   storageS3ClientTimeout: number
-  storageS3UploadTimeout: number
-  storageS3DownloadTimeout: number
   isMultitenant: boolean
   jwtSecret: string
   jwtAlgorithm: string
@@ -277,15 +275,7 @@ export function getConfig(options?: { reload?: boolean }): StorageConfigType {
       getOptionalConfigFromEnv('STORAGE_S3_FORCE_PATH_STYLE', 'GLOBAL_S3_FORCE_PATH_STYLE') ===
       'true',
     storageS3Region: getOptionalConfigFromEnv('STORAGE_S3_REGION', 'REGION') as string,
-    storageS3ClientTimeout: Number(
-      getOptionalConfigFromEnv('STORAGE_S3_CLIENT_TIMEOUT') || `${1000 * 600}` // 10m
-    ),
-    storageS3DownloadTimeout: Number(
-      getOptionalConfigFromEnv('STORAGE_S3_DOWNLOAD_TIMEOUT') || `${1000 * 43200}` //12h
-    ),
-    storageS3UploadTimeout: Number(
-      getOptionalConfigFromEnv('STORAGE_S3_UPLOAD_TIMEOUT') || `${1000 * 1200}` // 20m
-    ),
+    storageS3ClientTimeout: Number(getOptionalConfigFromEnv('STORAGE_S3_CLIENT_TIMEOUT') || `0`),
 
     // DB - Migrations
     dbAnonRole: getOptionalConfigFromEnv('DB_ANON_ROLE') || 'anon',

diff --git a/src/http/plugins/signals.ts b/src/http/plugins/signals.ts
@@ -22,10 +22,13 @@ export const signals = fastifyPlugin(
 
       // Client terminated the request before the body was fully received
       res.raw.once('close', () => {
-        req.signals.response.abort()
+        const aborted = !res.raw.writableFinished
+        if (aborted) {
+          req.signals.response.abort()
 
-        if (!req.signals.disconnect.signal.aborted) {
-          req.signals.disconnect.abort()
+          if (!req.signals.disconnect.signal.aborted) {
+            req.signals.disconnect.abort()
+          }
         }
       })
     })

diff --git a/src/http/plugins/tenant-id.ts b/src/http/plugins/tenant-id.ts
@@ -7,10 +7,16 @@ declare module 'fastify' {
   }
 }
 
+const {
+  version,
+  isMultitenant,
+  tenantId: defaultTenantId,
+  requestXForwardedHostRegExp,
+} = getConfig()
+
 export const tenantId = fastifyPlugin(
   async (fastify) => {
-    const { isMultitenant, tenantId, requestXForwardedHostRegExp } = getConfig()
-    fastify.decorateRequest('tenantId', tenantId)
+    fastify.decorateRequest('tenantId', defaultTenantId)
     fastify.addHook('onRequest', async (request) => {
       if (!isMultitenant || !requestXForwardedHostRegExp) return
       const xForwardedHost = request.headers['x-forwarded-host']
@@ -26,6 +32,7 @@ export const tenantId = fastifyPlugin(
         tenantId: request.tenantId,
         project: request.tenantId,
         reqId: request.id,
+        appVersion: version,
       })
     })
   },

diff --git a/src/http/plugins/tracing.ts b/src/http/plugins/tracing.ts
@@ -32,24 +32,28 @@ export const tracing = fastifyPlugin(
     fastify.register(traceServerTime)
 
     fastify.addHook('onRequest', async (request) => {
-      if (isMultitenant && request.tenantId) {
-        const tenantConfig = await getTenantConfig(request.tenantId)
-        request.tracingMode = tenantConfig.tracingMode
-      } else {
-        request.tracingMode = defaultTracingMode
-      }
+      try {
+        if (isMultitenant && request.tenantId) {
+          const tenantConfig = await getTenantConfig(request.tenantId)
+          request.tracingMode = tenantConfig.tracingMode
+        } else {
+          request.tracingMode = defaultTracingMode
+        }
 
-      const span = trace.getSpan(context.active())
+        const span = trace.getSpan(context.active())
 
-      if (span) {
-        // We collect logs only in full,logs,debug mode
-        if (
-          tracingEnabled &&
-          request.tracingMode &&
-          !['full', 'logs', 'debug'].includes(request.tracingMode)
-        ) {
-          traceCollector.clearTrace(span.spanContext().traceId)
+        if (span) {
+          // We collect logs only in full,logs,debug mode
+          if (
+            tracingEnabled &&
+            request.tracingMode &&
+            !['full', 'logs', 'debug'].includes(request.tracingMode)
+          ) {
+            traceCollector.clearTrace(span.spanContext().traceId)
+          }
         }
+      } catch (e) {
+        logSchema.error(request.log, 'failed setting tracing mode', { error: e, type: 'tracing' })
       }
     })
   },
@@ -62,12 +66,12 @@ export const traceServerTime = fastifyPlugin(
       return
     }
     fastify.addHook('onResponse', async (request, reply) => {
-      const traceId = trace.getSpan(context.active())?.spanContext().traceId
+      try {
+        const traceId = trace.getSpan(context.active())?.spanContext().traceId
 
-      if (traceId) {
-        const spans = traceCollector.getSpansForTrace(traceId)
-        if (spans) {
-          try {
+        if (traceId) {
+          const spans = traceCollector.getSpansForTrace(traceId)
+          if (spans) {
             const serverTimingHeaders = spansToServerTimings(spans, reply.statusCode >= 500)
 
             request.serverTimings = serverTimingHeaders
@@ -84,27 +88,30 @@ export const traceServerTime = fastifyPlugin(
                 .join(',')
               reply.header('Server-Timing', httpServerTimes)
             }
-          } catch (e) {
-            logSchema.error(logger, 'failed parsing server times', { error: e, type: 'otel' })
+            traceCollector.clearTrace(traceId)
           }
-
-          traceCollector.clearTrace(traceId)
         }
+      } catch (e) {
+        logSchema.error(request.log, 'failed tracing on response', { error: e, type: 'tracing' })
       }
     })
 
     fastify.addHook('onRequestAbort', async (req) => {
-      const span = trace.getSpan(context.active())
-      const traceId = span?.spanContext().traceId
+      try {
+        const span = trace.getSpan(context.active())
+        const traceId = span?.spanContext().traceId
 
-      span?.setAttribute('req_aborted', true)
+        span?.setAttribute('req_aborted', true)
 
-      if (traceId) {
-        const spans = traceCollector.getSpansForTrace(traceId)
-        if (spans) {
-          req.serverTimings = spansToServerTimings(spans, true)
+        if (traceId) {
+          const spans = traceCollector.getSpansForTrace(traceId)
+          if (spans) {
+            req.serverTimings = spansToServerTimings(spans, true)
+          }
+          traceCollector.clearTrace(traceId)
         }
-        traceCollector.clearTrace(traceId)
+      } catch (e) {
+        logSchema.error(logger, 'failed parsing server times on abort', { error: e, type: 'otel' })
       }
     })
   },

diff --git a/src/http/routes/tus/index.ts b/src/http/routes/tus/index.ts
@@ -35,7 +35,7 @@ const {
   storageS3Endpoint,
   storageS3ForcePathStyle,
   storageS3Region,
-  storageS3UploadTimeout,
+  storageS3ClientTimeout,
   tusUrlExpiryMs,
   tusPath,
   tusPartSize,
@@ -69,7 +69,7 @@ function createTusStore() {
         requestHandler: new NodeHttpHandler({
           ...agent,
           connectionTimeout: 5000,
-          requestTimeout: storageS3UploadTimeout,
+          requestTimeout: storageS3ClientTimeout,
         }),
         bucket: storageS3Bucket,
         region: storageS3Region,

diff --git a/src/storage/backend/index.ts b/src/storage/backend/index.ts
@@ -7,14 +7,8 @@ export * from './s3'
 export * from './file'
 export * from './adapter'
 
-const {
-  storageS3Region,
-  storageS3Endpoint,
-  storageS3ForcePathStyle,
-  storageS3ClientTimeout,
-  storageS3UploadTimeout,
-  storageS3DownloadTimeout,
-} = getConfig()
+const { storageS3Region, storageS3Endpoint, storageS3ForcePathStyle, storageS3ClientTimeout } =
+  getConfig()
 
 type ConfigForStorage<Type extends StorageBackendType> = Type extends 's3'
   ? S3ClientOptions
@@ -34,8 +28,6 @@ export function createStorageBackend<Type extends StorageBackendType>(
       endpoint: storageS3Endpoint,
       forcePathStyle: storageS3ForcePathStyle,
       requestTimeout: storageS3ClientTimeout,
-      uploadTimeout: storageS3UploadTimeout,
-      downloadTimeout: storageS3DownloadTimeout,
       ...(config ? config : {}),
     }
     storageBackend = new S3Backend(defaultOptions)

diff --git a/src/storage/backend/s3.ts b/src/storage/backend/s3.ts
@@ -126,8 +126,6 @@ export interface S3ClientOptions {
   role?: string
   httpAgent?: { httpAgent: Agent; httpsAgent: HttpsAgent }
   requestTimeout?: number
-  downloadTimeout?: number
-  uploadTimeout?: number
 }
 
 /**
@@ -136,29 +134,12 @@ export interface S3ClientOptions {
  */
 export class S3Backend implements StorageBackendAdapter {
   client: S3Client
-  uploadClient: S3Client
-  downloadClient: S3Client
 
   constructor(options: S3ClientOptions) {
     // Default client for API operations
     this.client = this.createS3Client({
       ...options,
       name: 's3_default',
-      requestTimeout: options.requestTimeout,
-    })
-
-    // Upload client exclusively for upload operations
-    this.uploadClient = this.createS3Client({
-      ...options,
-      name: 's3_upload',
-      requestTimeout: options.uploadTimeout,
-    })
-
-    // Download client exclusively for download operations
-    this.downloadClient = this.createS3Client({
-      ...options,
-      name: 's3_download',
-      requestTimeout: options.downloadTimeout,
     })
   }
 
@@ -187,7 +168,7 @@ export class S3Backend implements StorageBackendAdapter {
       input.IfModifiedSince = new Date(headers.ifModifiedSince)
     }
     const command = new GetObjectCommand(input)
-    const data = await this.downloadClient.send(command, {
+    const data = await this.client.send(command, {
       abortSignal: signal,
     })
 
@@ -228,7 +209,7 @@ export class S3Backend implements StorageBackendAdapter {
   ): Promise<ObjectMetadata> {
     try {
       const paralellUploadS3 = new Upload({
-        client: this.uploadClient,
+        client: this.client,
         params: {
           Bucket: bucketName,
           Key: withOptionalVersion(key, version),
@@ -312,7 +293,7 @@ export class S3Backend implements StorageBackendAdapter {
         CopySourceIfModifiedSince: conditions?.ifModifiedSince,
         CopySourceIfUnmodifiedSince: conditions?.ifUnmodifiedSince,
       })
-      const data = await this.uploadClient.send(command)
+      const data = await this.client.send(command)
       return {
         httpStatusCode: data.$metadata.httpStatusCode || 200,
         eTag: data.CopyObjectResult?.ETag || '',
@@ -438,9 +419,7 @@ export class S3Backend implements StorageBackendAdapter {
       ContentLength: length,
     })
 
-    const resp = await this.uploadClient.send(paralellUploadS3, {
-      // overwriting the requestTimeout here to avoid the request being cancelled, as the upload can take a long time for a max 5GB upload
-      requestTimeout: 0,
+    const resp = await this.client.send(paralellUploadS3, {
       abortSignal: signal,
     })
 
@@ -524,7 +503,7 @@ export class S3Backend implements StorageBackendAdapter {
       CopySourceRange: bytesRange ? `bytes=${bytesRange.fromByte}-${bytesRange.toByte}` : undefined,
     })
 
-    const part = await this.uploadClient.send(uploadPartCopy)
+    const part = await this.client.send(uploadPartCopy)
 
     return {
       eTag: part.CopyPartResult?.ETag,

diff --git a/src/storage/limits.ts b/src/storage/limits.ts
@@ -49,7 +49,7 @@ export async function isImageTransformationEnabled(tenantId: string) {
 export function isValidKey(key: string): boolean {
   // only allow s3 safe characters and characters which require special handling for now
   // https://docs.aws.amazon.com/AmazonS3/latest/userguide/object-keys.html
-  return key.length > 0 && /^(\w|\/|!|-|\.|\*|'|\(|\)| |&|\$|@|=|;|:|\+|,|\?)*$/.test(key)
+  return key.length > 0 && /^[\p{L}\p{N}\p{M}\/!.\-*'()& $@=;:+,?]+\.[\p{L}\p{N}\p{M}]+$/u.test(key)
 }
 
 /**

diff --git a/src/test/jwt.test.ts b/src/test/jwt.test.ts
@@ -43,7 +43,7 @@ describe('JWT', () => {
       kid: keys.length.toString(),
       privateKey: Buffer.from(hmacPrivateKeyWithKid, 'utf-8'),
       publicKey: {
-        export: (options?: any) => ({
+        export: () => ({
           kty: 'oct',
           k: Buffer.from(hmacPrivateKeyWithKid, 'utf-8').toString('base64url'),
         }),
@@ -52,7 +52,7 @@ describe('JWT', () => {
 
     const jwks = {
       keys: keys.map(
-        ({ publicKey, kid }, i) =>
+        ({ publicKey, kid }) =>
           ({
             ...(publicKey as unknown as crypto.KeyObject).export({ format: 'jwk' }),
             kid,