aws-s3: better typecheck to prevent accessing undefined var (#2388)

* aws-s3: better typecheck to prevent accessing undefined var

* aws-s3: refactor and test isXml

Co-authored-by: Renée Kooi <renee@kooi.me>

packages/@uppy/aws-s3/src/index.js

@@ -34,6 +34,7 @@ const hasProperty = require('@uppy/utils/lib/hasProperty')
 const { RequestClient } = require('@uppy/companion-client')
 const qsStringify = require('qs-stringify')
 const MiniXHRUpload = require('./MiniXHRUpload')
+const isXml = require('./isXml')
 
 function resolveUrl (origin, link) {
   return origin
@@ -41,33 +42,18 @@ function resolveUrl (origin, link) {
     : new URL_(link).toString()
 }
 
-function isXml (content, xhr) {
-  const rawContentType = (xhr.headers ? xhr.headers['content-type'] : xhr.getResponseHeader('Content-Type'))
-
-  if (rawContentType === null) {
-    return false
-  }
-
-  // Get rid of mime parameters like charset=utf-8
-  const contentType = rawContentType.replace(/;.*$/, '').toLowerCase()
-  if (typeof contentType === 'string') {
-    if (contentType === 'application/xml' || contentType === 'text/xml') {
-      return true
-    }
-    // GCS uses text/html for some reason
-    // https://github.com/transloadit/uppy/issues/896
-    if (contentType === 'text/html' && /^<\?xml /.test(content)) {
-      return true
-    }
-  }
-  return false
-}
-
-function getXmlValue (source, key) {
-  const start = source.indexOf(`<${key}>`)
-  const end = source.indexOf(`</${key}>`, start)
+/**
+ * Get the contents of a named tag in an XML source string.
+ *
+ * @param {string} source - The XML source string.
+ * @param {string} tagName - The name of the tag.
+ * @returns {string} The contents of the tag, or the empty string if the tag does not exist.
+ */
+function getXmlValue (source, tagName) {
+  const start = source.indexOf(`<${tagName}>`)
+  const end = source.indexOf(`</${tagName}>`, start)
   return start !== -1 && end !== -1
-    ? source.slice(start + key.length + 2, end)
+    ? source.slice(start + tagName.length + 2, end)
     : ''
 }
 

packages/@uppy/aws-s3/src/isXml.js

@@ -0,0 +1,39 @@
+/**
+ * Remove parameters like `charset=utf-8` from the end of a mime type string.
+ *
+ * @param {string} mimeType - The mime type string that may have optional parameters.
+ * @returns {string} The "base" mime type, i.e. only 'category/type'.
+ */
+function removeMimeParameters (mimeType) {
+  return mimeType.replace(/;.*$/, '')
+}
+
+/**
+ * Check if a response contains XML based on the response object and its text content.
+ *
+ * @param {string} content - The text body of the response.
+ * @param {object|XMLHttpRequest} xhr - The XHR object or response object from Companion.
+ * @returns {bool} Whether the content is (probably) XML.
+ */
+function isXml (content, xhr) {
+  const rawContentType = (xhr.headers ? xhr.headers['content-type'] : xhr.getResponseHeader('Content-Type'))
+
+  if (rawContentType == null) {
+    return false
+  }
+
+  if (typeof rawContentType === 'string') {
+    const contentType = removeMimeParameters(rawContentType).toLowerCase()
+    if (contentType === 'application/xml' || contentType === 'text/xml') {
+      return true
+    }
+    // GCS uses text/html for some reason
+    // https://github.com/transloadit/uppy/issues/896
+    if (contentType === 'text/html' && /^<\?xml /.test(content)) {
+      return true
+    }
+  }
+  return false
+}
+
+module.exports = isXml

packages/@uppy/aws-s3/src/isXml.test.js

@@ -0,0 +1,64 @@
+const isXml = require('./isXml')
+
+describe('AwsS3', () => {
+  describe('isXml', () => {
+    it('returns true for XML documents', () => {
+      const content = '<?xml version="1.0" encoding="UTF-8"?><Key>image.jpg</Key>'
+      expect(isXml(content, {
+        getResponseHeader: () => 'application/xml'
+      })).toEqual(true)
+      expect(isXml(content, {
+        getResponseHeader: () => 'text/xml'
+      })).toEqual(true)
+      expect(isXml(content, {
+        getResponseHeader: () => 'text/xml; charset=utf-8'
+      })).toEqual(true)
+      expect(isXml(content, {
+        getResponseHeader: () => 'application/xml; charset=iso-8859-1'
+      })).toEqual(true)
+    })
+
+    it('returns true for GCS XML documents', () => {
+      const content = '<?xml version="1.0" encoding="UTF-8"?><Key>image.jpg</Key>'
+      expect(isXml(content, {
+        getResponseHeader: () => 'text/html'
+      })).toEqual(true)
+      expect(isXml(content, {
+        getResponseHeader: () => 'text/html; charset=utf8'
+      })).toEqual(true)
+    })
+
+    it('returns true for remote response objects', () => {
+      const content = '<?xml version="1.0" encoding="UTF-8"?><Key>image.jpg</Key>'
+      expect(isXml(content, {
+        headers: { 'content-type': 'application/xml' }
+      })).toEqual(true)
+      expect(isXml(content, {
+        headers: { 'content-type': 'application/xml' }
+      })).toEqual(true)
+      expect(isXml(content, {
+        headers: { 'content-type': 'text/html' }
+      })).toEqual(true)
+    })
+
+    it('returns false when content-type is missing', () => {
+      const content = '<?xml version="1.0" encoding="UTF-8"?><Key>image.jpg</Key>'
+      expect(isXml(content, {
+        getResponseHeader: () => null
+      })).toEqual(false)
+      expect(isXml(content, {
+        headers: { 'content-type': null }
+      })).toEqual(false)
+      expect(isXml(content, {
+        headers: {}
+      })).toEqual(false)
+    })
+
+    it('returns false for HTML documents', () => {
+      const content = '<!DOCTYPE html><html>'
+      expect(isXml(content, {
+        getResponseHeader: () => 'text/html'
+      })).toEqual(false)
+    })
+  })
+})