fix: address review comments

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

Author: ferhatelmas

migrations/tenant/57-unicode-object-names.sql

@@ -1,5 +1,8 @@
 DO $$
 DECLARE
+  -- SQL_ASCII databases do not have reliable Unicode code point semantics, so
+  -- the migration needs a byte-pattern branch instead of U&'' character checks.
+  -- This is the case for OrioleDB and/or --locale=C databases.
   server_encoding text := current_setting('server_encoding');
 BEGIN
   IF NOT EXISTS (

src/http/plugins/xml.ts

@@ -6,34 +6,36 @@ import xml from 'xml2js'
 type XmlParserOptions = { disableContentParser?: boolean; parseAsArray?: string[] }
 type RequestError = Error & { statusCode?: number }
 
-export function decodeXmlNumericEntities(value: string): string {
-  const isValidXmlCodePoint = (codePoint: number) => {
-    if (!Number.isInteger(codePoint) || codePoint < 0 || codePoint > 0x10ffff) {
-      return false
-    }
-
-    return (
-      codePoint === 0x9 ||
-      codePoint === 0xa ||
-      codePoint === 0xd ||
-      (codePoint >= 0x20 && codePoint <= 0xd7ff) ||
-      (codePoint >= 0xe000 && codePoint <= 0xfffd) ||
-      (codePoint >= 0x10000 && codePoint <= 0x10ffff)
-    )
+function isValidXmlCodePoint(codePoint: number): boolean {
+  if (!Number.isInteger(codePoint) || codePoint < 0 || codePoint > 0x10ffff) {
+    return false
   }
 
-  return value.replace(
-    /&#([xX][0-9a-fA-F]{1,6}|[0-9]{1,7});/g,
-    (match: string, rawValue: string) => {
-      const isHex = rawValue[0].toLowerCase() === 'x'
-      const codePoint = Number.parseInt(isHex ? rawValue.slice(1) : rawValue, isHex ? 16 : 10)
-      if (!isValidXmlCodePoint(codePoint)) {
-        return match
-      }
+  return (
+    codePoint === 0x9 ||
+    codePoint === 0xa ||
+    codePoint === 0xd ||
+    (codePoint >= 0x20 && codePoint <= 0xd7ff) ||
+    (codePoint >= 0xe000 && codePoint <= 0xfffd) ||
+    (codePoint >= 0x10000 && codePoint <= 0x10ffff)
+  )
+}
+
+function getInvalidXmlNumericEntity(value: string): string | undefined {
+  const numericEntityPattern = /&#([xX][0-9a-fA-F]{1,6}|[0-9]{1,7});/g
+
+  let match = numericEntityPattern.exec(value)
+  while (match) {
+    const rawValue = match[1]
+    const isHex = rawValue[0].toLowerCase() === 'x'
+    const codePoint = Number.parseInt(isHex ? rawValue.slice(1) : rawValue, isHex ? 16 : 10)
 
-      return String.fromCodePoint(codePoint)
+    if (!isValidXmlCodePoint(codePoint)) {
+      return match[0]
     }
-  )
+
+    match = numericEntityPattern.exec(value)
+  }
 }
 
 function forcePathAsArray(node: unknown, pathSegments: string[]): void {
@@ -82,16 +84,23 @@ export const xmlParser = fastifyPlugin(
             return
           }
 
+          const xmlBody = typeof body === 'string' ? body : body.toString('utf8')
+          const invalidNumericEntity = getInvalidXmlNumericEntity(xmlBody)
+          if (invalidNumericEntity) {
+            const parseError: RequestError = new Error(
+              `Invalid XML payload: invalid numeric entity ${invalidNumericEntity}`
+            )
+            parseError.statusCode = 400
+            done(parseError)
+            return
+          }
+
           xml.parseString(
-            body,
+            xmlBody,
             {
               explicitArray: false,
               trim: true,
-              valueProcessors: [
-                decodeXmlNumericEntities,
-                xml.processors.parseNumbers,
-                xml.processors.parseBooleans,
-              ],
+              valueProcessors: [xml.processors.parseNumbers, xml.processors.parseBooleans],
             },
             (err: Error | null, parsed: unknown) => {
               if (err) {

src/http/routes/object/getSignedObject.ts

@@ -4,8 +4,8 @@ import { getConfig } from '../../../config'
 import { SignedToken, verifyJWT } from '../../../internal/auth'
 import { getJwtSecret } from '../../../internal/database'
 import { ERRORS } from '../../../internal/errors'
+import { doesSignedTokenMatchRequestPath } from '../../../internal/http'
 import { ROUTE_OPERATIONS } from '../operations'
-import { doesSignedTokenMatchRequestPath } from '../signed-url'
 
 const { storageS3Bucket } = getConfig()
 

src/http/routes/object/uploadSignedObject.ts

@@ -2,10 +2,10 @@ import fastifyMultipart from '@fastify/multipart'
 import { SignedUploadToken, verifyJWT } from '@internal/auth'
 import { getJwtSecret } from '@internal/database'
 import { ERRORS } from '@internal/errors'
+import { doesSignedTokenMatchRequestPath } from '@internal/http'
 import { FastifyInstance } from 'fastify'
 import { FromSchema } from 'json-schema-to-ts'
 import { ROUTE_OPERATIONS } from '../operations'
-import { doesSignedTokenMatchRequestPath } from '../signed-url'
 
 const uploadSignedObjectParamsSchema = {
   type: 'object',
@@ -97,16 +97,12 @@ export default async function routes(fastify: FastifyInstance) {
         throw ERRORS.InvalidJWT(err)
       }
 
-      const { owner, upsert, url, exp } = payload
+      const { owner, upsert, url } = payload
 
       if (!doesSignedTokenMatchRequestPath(request.raw.url, '/object/upload/sign', url)) {
         throw ERRORS.InvalidSignature()
       }
 
-      if (exp * 1000 < Date.now()) {
-        throw ERRORS.ExpiredSignature()
-      }
-
       const { objectMetadata, path } = await request.storage
         .asSuperUser()
         .from(bucketName)

src/http/routes/render/renderSignedImage.ts

@@ -1,12 +1,12 @@
 import { SignedToken, verifyJWT } from '@internal/auth'
 import { getJwtSecret, getTenantConfig } from '@internal/database'
 import { ERRORS } from '@internal/errors'
+import { doesSignedTokenMatchRequestPath } from '@internal/http'
 import { ImageRenderer } from '@storage/renderer'
 import { FastifyInstance } from 'fastify'
 import { FromSchema } from 'json-schema-to-ts'
 import { getConfig } from '../../../config'
 import { ROUTE_OPERATIONS } from '../operations'
-import { doesSignedTokenMatchRequestPath } from '../signed-url'
 
 const { storageS3Bucket, isMultitenant } = getConfig()
 

src/http/routes/signed-url.ts

@@ -1,45 +1,6 @@
+import { safeEncodeURIComponent } from '../http'
 import { StorageBackendError } from './storage-error'
 
-function toWellFormedString(value: string): string {
-  const maybeToWellFormed = (value as unknown as { toWellFormed?: () => string }).toWellFormed
-  if (typeof maybeToWellFormed === 'function') {
-    return maybeToWellFormed.call(value)
-  }
-
-  let normalized = ''
-  for (let i = 0; i < value.length; i++) {
-    const currentCodeUnit = value.charCodeAt(i)
-
-    if (currentCodeUnit >= 0xd800 && currentCodeUnit <= 0xdbff) {
-      const nextCodeUnit = value.charCodeAt(i + 1)
-      if (i + 1 < value.length && nextCodeUnit >= 0xdc00 && nextCodeUnit <= 0xdfff) {
-        normalized += value[i] + value[i + 1]
-        i += 1
-      } else {
-        normalized += '\uFFFD'
-      }
-      continue
-    }
-
-    if (currentCodeUnit >= 0xdc00 && currentCodeUnit <= 0xdfff) {
-      normalized += '\uFFFD'
-      continue
-    }
-
-    normalized += value[i]
-  }
-
-  return normalized
-}
-
-function safeEncodeURIComponent(value: string): string {
-  try {
-    return encodeURIComponent(value)
-  } catch {
-    return encodeURIComponent(toWellFormedString(value))
-  }
-}
-
 export enum ErrorCode {
   NoSuchBucket = 'NoSuchBucket',
   NoSuchKey = 'NoSuchKey',

src/internal/errors/codes.ts

@@ -1 +1,2 @@
 export * from './agent'
+export * from './url'

src/internal/http/index.ts

@@ -0,0 +1,69 @@
+function toWellFormedString(value: string): string {
+  const maybeToWellFormed = (value as unknown as { toWellFormed?: () => string }).toWellFormed
+  if (typeof maybeToWellFormed === 'function') {
+    return maybeToWellFormed.call(value)
+  }
+
+  let normalized = ''
+  for (let i = 0; i < value.length; i++) {
+    const currentCodeUnit = value.charCodeAt(i)
+
+    if (currentCodeUnit >= 0xd800 && currentCodeUnit <= 0xdbff) {
+      const nextCodeUnit = value.charCodeAt(i + 1)
+      if (i + 1 < value.length && nextCodeUnit >= 0xdc00 && nextCodeUnit <= 0xdfff) {
+        normalized += value[i] + value[i + 1]
+        i += 1
+      } else {
+        normalized += '\uFFFD'
+      }
+      continue
+    }
+
+    if (currentCodeUnit >= 0xdc00 && currentCodeUnit <= 0xdfff) {
+      normalized += '\uFFFD'
+      continue
+    }
+
+    normalized += value[i]
+  }
+
+  return normalized
+}
+
+export function safeEncodeURIComponent(value: string): string {
+  try {
+    return encodeURIComponent(value)
+  } catch {
+    return encodeURIComponent(toWellFormedString(value))
+  }
+}
+
+export function encodePathPreservingSeparators(path: string): string {
+  return path
+    .split('/')
+    .map((pathToken) => safeEncodeURIComponent(pathToken))
+    .join('/')
+}
+
+export function encodeBucketAndObjectPath(bucket: string, key: string): string {
+  return `${safeEncodeURIComponent(bucket)}/${encodePathPreservingSeparators(key)}`
+}
+
+function stripQueryString(rawUrl: string): string {
+  const queryIdx = rawUrl.indexOf('?')
+  return queryIdx === -1 ? rawUrl : rawUrl.slice(0, queryIdx)
+}
+
+export function doesSignedTokenMatchRequestPath(
+  rawUrl: string | undefined,
+  routePrefix: string,
+  signedObjectPath: string
+): boolean {
+  if (!rawUrl) {
+    return false
+  }
+
+  const pathname = stripQueryString(rawUrl)
+  const expectedPath = `${routePrefix}/${encodePathPreservingSeparators(signedObjectPath)}`
+  return pathname === expectedPath
+}

src/internal/http/url.ts

@@ -19,7 +19,7 @@ import {
 import { Progress, Upload } from '@aws-sdk/lib-storage'
 import { getSignedUrl } from '@aws-sdk/s3-request-presigner'
 import { ERRORS, StorageBackendError } from '@internal/errors'
-import { createAgent, InstrumentedAgent } from '@internal/http'
+import { createAgent, encodeBucketAndObjectPath, InstrumentedAgent } from '@internal/http'
 import { monitorStream } from '@internal/streams'
 import { NodeHttpHandler } from '@smithy/node-http-handler'
 import { BackupObjectInfo, ObjectBackup } from '@storage/backend/s3/backup'
@@ -32,7 +32,6 @@ import {
   UploadPart,
   withOptionalVersion,
 } from './../adapter'
-import { encodeCopySource } from './copy-source'
 
 const {
   storageS3UploadQueueSize,
@@ -256,7 +255,7 @@ export class S3Backend implements StorageBackendAdapter {
     try {
       const command = new CopyObjectCommand({
         Bucket: bucket,
-        CopySource: encodeCopySource(bucket, withOptionalVersion(source, version)),
+        CopySource: encodeBucketAndObjectPath(bucket, withOptionalVersion(source, version)),
         Key: withOptionalVersion(destination, destinationVersion),
         CopySourceIfMatch: conditions?.ifMatch,
         CopySourceIfNoneMatch: conditions?.ifNoneMatch,
@@ -569,7 +568,7 @@ export class S3Backend implements StorageBackendAdapter {
       Key: withOptionalVersion(key, version),
       UploadId,
       PartNumber,
-      CopySource: encodeCopySource(
+      CopySource: encodeBucketAndObjectPath(
         storageS3Bucket,
         withOptionalVersion(sourceKey, sourceKeyVersion)
       ),