@uppy/aws-s3: fix server generated keys not getting returned

.changeset/aws-s3-signrequest-key.md

@@ -0,0 +1,7 @@
+---
+"@uppy/aws-s3": minor
+---
+
+`signRequest` can now return the object key it signed for, as `key` next to `url`. When a signing server stores the object under a different key than the one Uppy proposed (a directory prefix, a server-generated name), returning `{ url, key }` from the request that creates the upload, the single-part `PUT`, or the multipart create, makes Uppy use that key for the rest of the upload and report it in `upload-success`. Previously the client-generated key was reported even when the server had stored the object elsewhere (#6496).
+
+`key` is optional. Signers that return only `{ url }` are unchanged. Requests that carry an `uploadId` must be signed for the key they receive; a `key` returned on those requests is ignored.

examples/aws-nodejs/routes/presign.js

@@ -20,6 +20,10 @@ const { getSignedUrl } = require('@aws-sdk/s3-request-presigner')
 
 const expiresIn = 900 // 15 minutes
 
+// Objects are stored under this directory. Uppy proposes a key (the file
+// name by default); the server decides where it actually goes.
+const directory = 'uppy-nodejs-example'
+
 let s3Client
 function getS3Client() {
   s3Client ??= new S3Client({
@@ -50,6 +54,9 @@ router.post('/s3/presign', async (req, res, next) => {
     }
 
     let command
+    // Set only when this request creates an object. It's returned to Uppy,
+    // which then uses it for every later request of the same upload.
+    let objectKey
 
     if (method === 'PUT' && uploadId && partNumber) {
       // UploadPart (multipart)
@@ -61,16 +68,20 @@ router.post('/s3/presign', async (req, res, next) => {
       })
     } else if (method === 'PUT' && !uploadId && !partNumber) {
       // PutObject (simple upload)
+      objectKey = `${directory}/${key}`
       command = new PutObjectCommand({
         Bucket: bucket,
-        Key: key,
+        Key: objectKey,
         ContentType: contentType || 'application/octet-stream',
       })
     } else if (method === 'POST' && !uploadId) {
-      // CreateMultipartUpload
+      // CreateMultipartUpload. This is the only multipart request where the
+      // key may be changed. Parts, complete, abort and list arrive with the
+      // key returned here and must be signed for it as-is.
+      objectKey = `${directory}/${key}`
       command = new CreateMultipartUploadCommand({
         Bucket: bucket,
-        Key: key,
+        Key: objectKey,
         ContentType: contentType || 'application/octet-stream',
       })
     } else if (method === 'POST' && uploadId) {
@@ -99,7 +110,7 @@ router.post('/s3/presign', async (req, res, next) => {
     }
 
     const url = await getSignedUrl(client, command, { expiresIn })
-    res.json({ url })
+    res.json(objectKey ? { url, key: objectKey } : { url })
   } catch (err) {
     next(err)
   }

examples/aws-php/s3-sign.php

@@ -59,15 +59,21 @@
   exit;
 }
 
+// The object is stored under $directory, not under the key Uppy proposed,
+// so the real key is returned as well. Uppy reports it in `upload-success`.
+// If you add multipart later, only change the key on CreateMultipartUpload;
+// requests carrying an uploadId must be signed for the key they send.
+$objectKey = "{$directory}/{$key}";
+
 $command = $s3->getCommand('putObject', [
   'Bucket' => $bucket,
-  'Key' => "{$directory}/{$key}",
+  'Key' => $objectKey,
 ]);
 
 $request = $s3->createPresignedRequest($command, '+5 minutes');
 
 header('content-type: application/json');
-// signRequest expects a `{ url }` response — no method/fields/headers.
 echo json_encode([
   'url' => (string) $request->getUri(),
+  'key' => $objectKey,
 ]);

packages/@uppy/aws-s3/src/s3-client/S3mini.ts

@@ -21,7 +21,7 @@ import * as U from './utils.js'
  *       method: 'POST',
  *       body: JSON.stringify({ method, key, uploadId, partNumber }),
  *     });
- *     return resp.json(); // { url }
+ *     return resp.json(); // { url } or { url, key }
  *   },
  * });
  *
@@ -185,7 +185,7 @@ class S3mini extends S3Client {
   }: IT.PutObjectParams) {
     this._checkKey(key)
 
-    const { xhr, url } = await this.request({
+    const { xhr, url, signedKey } = await this.request({
       request: { method: 'PUT', key },
       data,
       onProgress,
@@ -196,7 +196,7 @@ class S3mini extends S3Client {
     return {
       location: U.removeQueryString(url),
       etag: U.sanitizeETag(xhr.getResponseHeader('etag')),
-      key,
+      key: signedKey,
     }
   }
 
@@ -212,7 +212,7 @@ class S3mini extends S3Client {
       throw new TypeError(`${C.ERROR_PREFIX}fileType must be a string`)
     }
 
-    const { xhr } = await this.request({
+    const { xhr, signedKey } = await this.request({
       request: { method: 'POST', key },
       contentType: fileType,
       signal,
@@ -231,7 +231,7 @@ class S3mini extends S3Client {
         const uploadId = uploadResult.uploadId || uploadResult.UploadId
 
         if (uploadId && typeof uploadId === 'string') {
-          return { uploadId, key }
+          return { uploadId, key: signedKey }
         }
       }
     }
@@ -297,7 +297,7 @@ class S3mini extends S3Client {
     signal?: AbortSignal
     contentType?: string
     shouldRetryCredentials?: boolean
-  }): Promise<{ xhr: XMLHttpRequest; url: string }> {
+  }): Promise<{ xhr: XMLHttpRequest; url: string; signedKey: string }> {
     // Wait for online before starting
     await this.waitForOnline(signal)
 
@@ -307,7 +307,7 @@ class S3mini extends S3Client {
     }
 
     try {
-      const { url } = await this.signRequest(request)
+      const { url, key: signedKey } = await this.signRequest(request)
 
       const xhr = await this.xhr({
         url,
@@ -318,7 +318,7 @@ class S3mini extends S3Client {
         contentType,
       })
 
-      return { xhr, url }
+      return { xhr, url, signedKey: signedKey || request.key }
     } catch (err: unknown) {
       // NetworkError or errors with attached XHR (from onAfterResponse throws)
       if (

packages/@uppy/aws-s3/src/s3-client/types.ts

@@ -43,6 +43,12 @@ export type PresignableRequest =
 /** Response with the pre-signed URL */
 export type PresignedResponse = {
   url: string
+  /**
+   * Key the request was signed for, if the server changed it (e.g. added a
+   * prefix). Defaults to the requested key. Only honored on `putObject` and
+   * `createMultipartUpload`; later requests already carry the right key.
+   */
+  key?: string
 }
 
 /** Function that generates a pre-signed URL for a request */

packages/@uppy/aws-s3/tests/index.test.ts

@@ -56,7 +56,7 @@ function createMultipartMocks(
   const signRequest = vi.fn().mockImplementation(async (req: any) => {
     const params = new URLSearchParams()
     if (req.uploadId) params.set('uploadId', req.uploadId)
-    if (req.partNumber) params.set('partNumber', String(req.partNumber))
+    if (req.partNumber != null) params.set('partNumber', String(req.partNumber))
     params.set('method', req.method)
     return {
       url: `https://test-bucket.s3.us-east-1.amazonaws.com/${req.key || key}?${params}`,
@@ -234,6 +234,203 @@ describe('AwsS3', () => {
     })
   })
 
+  describe('server-generated object key (#6496)', () => {
+    const bucketUrl = 'https://test-bucket.s3.us-east-1.amazonaws.com'
+
+    test('single-part: reports the key the signer actually used', async ({
+      worker,
+    }) => {
+      const { signRequest, registerHandlers } = createMultipartMocks(worker)
+      registerHandlers()
+      signRequest.mockImplementation(async (req: any) => {
+        const serverKey = `server-${req.key}`
+        return {
+          url: `${bucketUrl}/${serverKey}?method=${req.method}`,
+          key: serverKey,
+        }
+      })
+
+      const core = new Core().use(AwsS3, {
+        s3Endpoint: bucketUrl,
+        region: 'us-east-1',
+        signRequest,
+        shouldUseMultipart: false,
+        generateObjectKey: () => 'client-photo.jpg',
+      })
+      core.addFile({
+        source: 'test',
+        name: 'photo.jpg',
+        type: 'image/jpeg',
+        data: new File([new Uint8Array(KB)], 'photo.jpg'),
+      })
+
+      const onSuccess = vi.fn()
+      core.on('upload-success', onSuccess)
+      await core.upload()
+
+      expect(onSuccess).toHaveBeenCalledTimes(1)
+      const response = onSuccess.mock.calls[0][1]
+      expect(response.body.key).toBe('server-client-photo.jpg')
+      expect(response.uploadURL).toBe(`${bucketUrl}/server-client-photo.jpg`)
+    })
+
+    test('multipart: uses the key returned on create for every later request', async ({
+      worker,
+    }) => {
+      const serverKey = 'server-big.dat'
+      const { signRequest, registerHandlers } = createMultipartMocks(worker, {
+        key: serverKey,
+      })
+      registerHandlers()
+      // Mints the key on create, then only signs for that key.
+      signRequest.mockImplementation(async (req: any) => {
+        const isCreate = req.method === 'POST' && !req.uploadId
+        if (!isCreate && req.key !== serverKey) {
+          throw new Error(`unexpected key: ${req.key}`)
+        }
+        const params = new URLSearchParams({ method: req.method })
+        if (req.uploadId) params.set('uploadId', req.uploadId)
+        if (req.partNumber != null)
+          params.set('partNumber', String(req.partNumber))
+        return {
+          url: `${bucketUrl}/${serverKey}?${params}`,
+          ...(isCreate ? { key: serverKey } : {}),
+        }
+      })
+
+      const core = new Core().use(AwsS3, {
+        s3Endpoint: bucketUrl,
+        region: 'us-east-1',
+        signRequest,
+        shouldUseMultipart: true,
+        generateObjectKey: () => 'client-big.dat',
+      })
+      core.addFile({
+        source: 'test',
+        name: 'big.dat',
+        type: 'application/octet-stream',
+        data: new File([new Uint8Array(6 * MB)], 'big.dat'),
+      })
+
+      const onSuccess = vi.fn()
+      core.on('upload-success', onSuccess)
+      await core.upload()
+
+      expect(onSuccess).toHaveBeenCalledTimes(1)
+      expect(onSuccess.mock.calls[0][1].body.key).toBe(serverKey)
+      const keys = signRequest.mock.calls.map((c: any) => c[0].key)
+      expect(keys[0]).toBe('client-big.dat')
+      expect(keys.length).toBeGreaterThan(1)
+      expect(keys.slice(1).every((k: string) => k === serverKey)).toBe(true)
+    })
+
+    test('multipart: persists the key returned on create in s3Multipart', async ({
+      worker,
+    }) => {
+      const serverKey = 'server-big.dat'
+      const { signRequest, registerHandlers } = createMultipartMocks(worker, {
+        key: serverKey,
+      })
+      registerHandlers({ hangNonCreate: true })
+      signRequest.mockImplementation(async (req: any) => {
+        const params = new URLSearchParams({ method: req.method })
+        if (req.uploadId) params.set('uploadId', req.uploadId)
+        return {
+          url: `${bucketUrl}/${serverKey}?${params}`,
+          ...(req.uploadId ? {} : { key: serverKey }),
+        }
+      })
+
+      const core = new Core().use(AwsS3, {
+        s3Endpoint: bucketUrl,
+        region: 'us-east-1',
+        signRequest,
+        shouldUseMultipart: true,
+        generateObjectKey: () => 'client-big.dat',
+      })
+      const fileId = core.addFile({
+        source: 'test',
+        name: 'big.dat',
+        type: 'application/octet-stream',
+        data: new File([new Uint8Array(6 * MB)], 'big.dat'),
+      })
+
+      const uploadPromise = core.upload()
+      await new Promise((resolve) => setTimeout(resolve, 100))
+
+      expect(core.getFile(fileId).s3Multipart?.key).toBe(serverKey)
+
+      core.cancelAll()
+      await uploadPromise
+    })
+
+    test('multipart: a signer that only returns url keeps getting the client key', async ({
+      worker,
+    }) => {
+      // Prefixes on every request. Works today because the client key is
+      // re-sent each time, so the plugin must not adopt S3's echoed <Key>.
+      const { signRequest, registerHandlers } = createMultipartMocks(worker, {
+        key: 'dir-client-big.dat',
+      })
+      registerHandlers()
+      signRequest.mockImplementation(async (req: any) => {
+        const params = new URLSearchParams({ method: req.method })
+        if (req.uploadId) params.set('uploadId', req.uploadId)
+        if (req.partNumber != null)
+          params.set('partNumber', String(req.partNumber))
+        return { url: `${bucketUrl}/dir-${req.key}?${params}` }
+      })
+
+      const core = new Core().use(AwsS3, {
+        s3Endpoint: bucketUrl,
+        region: 'us-east-1',
+        signRequest,
+        shouldUseMultipart: true,
+        generateObjectKey: () => 'client-big.dat',
+      })
+      core.addFile({
+        source: 'test',
+        name: 'big.dat',
+        type: 'application/octet-stream',
+        data: new File([new Uint8Array(6 * MB)], 'big.dat'),
+      })
+      await core.upload()
+
+      const keys = signRequest.mock.calls.map((c: any) => c[0].key)
+      expect(keys.length).toBeGreaterThan(1)
+      expect(keys.every((k: string) => k === 'client-big.dat')).toBe(true)
+    })
+
+    test('ignores an empty key from the signer', async ({ worker }) => {
+      const { signRequest, registerHandlers } = createMultipartMocks(worker)
+      registerHandlers()
+      signRequest.mockImplementation(async (req: any) => ({
+        url: `${bucketUrl}/${req.key}?method=${req.method}`,
+        key: '',
+      }))
+
+      const core = new Core().use(AwsS3, {
+        s3Endpoint: bucketUrl,
+        region: 'us-east-1',
+        signRequest,
+        shouldUseMultipart: false,
+        generateObjectKey: () => 'client-photo.jpg',
+      })
+      core.addFile({
+        source: 'test',
+        name: 'photo.jpg',
+        type: 'image/jpeg',
+        data: new File([new Uint8Array(KB)], 'photo.jpg'),
+      })
+
+      const onSuccess = vi.fn()
+      core.on('upload-success', onSuccess)
+      await core.upload()
+
+      expect(onSuccess.mock.calls[0][1].body.key).toBe('client-photo.jpg')
+    })
+  })
+
   describe('upload events', () => {
     test('emits upload-start when upload begins', async () => {
       const signRequest = vi.fn().mockRejectedValue(new Error('Test stop'))