s3: Don't set uploadURL when success_action_status was missing (#900)

* s3: Do not return an uploadURL if S3 doesn't give us one.

* xhrupload: Add test for `this` inside getResponseData()

Because the S3 plugin depends on this now, but it might be easy to miss
if we changed that behaviour in the future.

* Remove debug logs in XHRUpload test.

* Log a warning if success_action_status is (probably) missing.

package-lock.json

@@ -12933,6 +12933,58 @@
       "integrity": "sha1-ICtIAhoMTL3i34DeFaF0Q8i0OYA=",
       "dev": true
     },
+    "nock": {
+      "version": "9.3.2",
+      "resolved": "https://registry.npmjs.org/nock/-/nock-9.3.2.tgz",
+      "integrity": "sha512-pulpsRVFneYGpgktmt99s10fFh10zSpYhydwkG28xLps/p19n39lBSq5kjb7UW2YOPyQtt7FLyXuP+xHyRRI0w==",
+      "dev": true,
+      "requires": {
+        "chai": "^4.1.2",
+        "debug": "^3.1.0",
+        "deep-equal": "^1.0.0",
+        "json-stringify-safe": "^5.0.1",
+        "lodash": "^4.17.5",
+        "mkdirp": "^0.5.0",
+        "propagate": "^1.0.0",
+        "qs": "^6.5.1",
+        "semver": "^5.5.0"
+      },
+      "dependencies": {
+        "debug": {
+          "version": "3.1.0",
+          "resolved": "https://registry.npmjs.org/debug/-/debug-3.1.0.tgz",
+          "integrity": "sha512-OX8XqP7/1a9cqkxYw2yXss15f26NKWBpDXQd0/uK/KPqdQhxbPa994hnzjcE2VqQpDslf55723cKPUOGSmMY3g==",
+          "dev": true,
+          "requires": {
+            "ms": "2.0.0"
+          }
+        },
+        "lodash": {
+          "version": "4.17.10",
+          "resolved": "https://registry.npmjs.org/lodash/-/lodash-4.17.10.tgz",
+          "integrity": "sha512-UejweD1pDoXu+AD825lWwp4ZGtSwgnpZxb3JDViD7StjQz+Nb/6l093lx4OQ0foGWNRoc19mWy7BzL+UAK2iVg==",
+          "dev": true
+        },
+        "ms": {
+          "version": "2.0.0",
+          "resolved": "https://registry.npmjs.org/ms/-/ms-2.0.0.tgz",
+          "integrity": "sha1-VgiurfwAvmwpAd9fmGF4jeDVl8g=",
+          "dev": true
+        },
+        "qs": {
+          "version": "6.5.2",
+          "resolved": "https://registry.npmjs.org/qs/-/qs-6.5.2.tgz",
+          "integrity": "sha512-N5ZAX4/LxJmF+7wN74pUD6qAh9/wnvdQcjq9TZjevvXzSUo7bfmw91saqMjzGS2xq91/odN2dW/WOl7qQHNDGA==",
+          "dev": true
+        },
+        "semver": {
+          "version": "5.5.0",
+          "resolved": "https://registry.npmjs.org/semver/-/semver-5.5.0.tgz",
+          "integrity": "sha512-4SJ3dm0WAwWy/NVeioZh5AntkdJoWKxHxcmyP622fOkgHa4z3R0TdBJICINyaSDE6uNwVc8gZr+ZinwZAH4xIA==",
+          "dev": true
+        }
+      }
+    },
     "node-fetch": {
       "version": "1.7.1",
       "resolved": "https://registry.npmjs.org/node-fetch/-/node-fetch-1.7.1.tgz",
@@ -14736,6 +14788,12 @@
         "object-assign": "^4.1.1"
       }
     },
+    "propagate": {
+      "version": "1.0.0",
+      "resolved": "https://registry.npmjs.org/propagate/-/propagate-1.0.0.tgz",
+      "integrity": "sha1-AMLa7t2iDofjeCs0Stuhzd1q1wk=",
+      "dev": true
+    },
     "property-is-enumerable-x": {
       "version": "1.1.0",
       "resolved": "https://registry.npmjs.org/property-is-enumerable-x/-/property-is-enumerable-x-1.1.0.tgz",

package.json

@@ -87,6 +87,7 @@
     "mkdirp": "0.5.1",
     "multi-glob": "1.0.1",
     "next-update": "^3.6.0",
+    "nock": "^9.3.2",
     "node-sass": "^4.9.0",
     "npm-run-all": "^4.1.2",
     "onchange": "^3.3.0",

src/plugins/AwsS3/index.js

@@ -148,17 +148,33 @@ module.exports = class AwsS3 extends Plugin {
   }
 
   install () {
+    const { log } = this.uppy
     this.uppy.addPreProcessor(this.prepareUpload)
 
+    let warnedSuccessActionStatus = false
     this.uppy.use(XHRUpload, {
       fieldName: 'file',
       responseUrlFieldName: 'location',
       timeout: this.opts.timeout,
       limit: this.opts.limit,
+      // Get the response data from a successful XMLHttpRequest instance.
+      // `content` is the S3 response as a string.
+      // `xhr` is the XMLHttpRequest instance.
       getResponseData (content, xhr) {
+        const opts = this
+
         // If no response, we've hopefully done a PUT request to the file
         // in the bucket on its full URL.
         if (!isXml(xhr)) {
+          if (opts.method.toUpperCase() === 'POST') {
+            if (!warnedSuccessActionStatus) {
+              log('[AwsS3] No response data found, make sure to set the success_action_status AWS SDK option to 201. See https://uppy.io/docs/aws-s3/#POST-Uploads', 'warning')
+              warnedSuccessActionStatus = true
+            }
+            // The responseURL won't contain the object key. Give up.
+            return { location: null }
+          }
+
           // Trim the query string because it's going to be a bunch of presign
           // parameters for a PUT request—doing a GET request with those will
           // always result in an error
@@ -192,6 +208,10 @@ module.exports = class AwsS3 extends Plugin {
           etag: getValue('ETag')
         }
       },
+
+      // Get the error data from a failed XMLHttpRequest instance.
+      // `content` is the S3 response as a string.
+      // `xhr` is the XMLHttpRequest instance.
       getResponseError (content, xhr) {
         // If no response, we don't have a specific error message, use the default.
         if (!isXml(xhr)) {

src/plugins/XHRUpload.test.js

@@ -0,0 +1,37 @@
+const nock = require('nock')
+const Core = require('../core')
+const XHRUpload = require('./XHRUpload')
+
+describe('XHRUpload', () => {
+  describe('getResponseData', () => {
+    it('has the XHRUpload options as its `this`', () => {
+      nock('https://fake-endpoint.uppy.io')
+        .defaultReplyHeaders({
+          'access-control-allow-method': 'POST',
+          'access-control-allow-origin': '*'
+        })
+        .options('/').reply(200, {})
+        .post('/').reply(200, {})
+
+      const core = new Core({ autoProceed: false })
+      const getResponseData = jest.fn(function () {
+        expect(this.some).toEqual('option')
+        return {}
+      })
+      core.use(XHRUpload, {
+        id: 'XHRUpload',
+        endpoint: 'https://fake-endpoint.uppy.io',
+        some: 'option',
+        getResponseData
+      })
+      core.addFile({
+        name: 'test.jpg',
+        data: new Blob([Buffer.alloc(8192)])
+      })
+
+      return core.upload().then(() => {
+        expect(getResponseData).toHaveBeenCalled()
+      })
+    })
+  })
+})