Sanitizer: validate srcset URLs and fail closed without DOMParser

.bundlewatch.config.json

@@ -34,27 +34,27 @@
     },
     {
       "path": "./dist/js/bootstrap.bundle.js",
-      "maxSize": "43.0 kB"
+      "maxSize": "44.0 kB"
     },
     {
       "path": "./dist/js/bootstrap.bundle.min.js",
-      "maxSize": "23.5 kB"
+      "maxSize": "23.75 kB"
     },
     {
       "path": "./dist/js/bootstrap.esm.js",
-      "maxSize": "28.0 kB"
+      "maxSize": "29.0 kB"
     },
     {
       "path": "./dist/js/bootstrap.esm.min.js",
-      "maxSize": "18.25 kB"
+      "maxSize": "18.75 kB"
     },
     {
       "path": "./dist/js/bootstrap.js",
-      "maxSize": "28.75 kB"
+      "maxSize": "29.5 kB"
     },
     {
       "path": "./dist/js/bootstrap.min.js",
-      "maxSize": "16.25 kB"
+      "maxSize": "16.75 kB"
     }
   ],
   "ci": {

js/src/util/sanitizer.js

@@ -54,6 +54,7 @@ const uriAttributes = new Set([
   'longdesc',
   'poster',
   'src',
+  'srcset',
   'xlink:href'
 ])
 
@@ -65,12 +66,75 @@ const uriAttributes = new Set([
  */
 const SAFE_URL_PATTERN = /^(?!javascript:)(?:[a-z0-9+.-]+:|[^&:/?#]*(?:[/?#]|$))/i
 
+const isSafeUrl = url => Boolean(SAFE_URL_PATTERN.test(url))
+
+// `srcset` is a comma-separated list of candidates. The first comma in a
+// `data:` URL starts the payload. A later comma starts the next candidate when
+// what follows looks like a URL (scheme, path, or file), with or without spaces.
+const SRCSET_DESCRIPTOR = /\s+[\d.]+[wx]\s*(?:,|$)/i
+const SRCSET_NEXT_CANDIDATE = /\s*,\s*(?=[^\s,]*(?:[a-z][a-z0-9+.-]*:|\/|\.))/i
+
+const extractSrcsetUrls = value => {
+  const urls = []
+  let rest = String(value).trim()
+
+  while (rest) {
+    if (/^data:/i.test(rest)) {
+      const headerComma = rest.indexOf(',')
+
+      if (headerComma === -1) {
+        urls.push(rest)
+        break
+      }
+
+      const payload = rest.slice(headerComma + 1)
+      const descriptor = payload.match(SRCSET_DESCRIPTOR)
+      const nextCandidate = payload.match(SRCSET_NEXT_CANDIDATE)
+      const descriptorAt = descriptor ? descriptor.index : Number.POSITIVE_INFINITY
+      const nextAt = nextCandidate ? nextCandidate.index : Number.POSITIVE_INFINITY
+
+      if (nextAt !== Number.POSITIVE_INFINITY && nextAt <= descriptorAt) {
+        urls.push(rest.slice(0, headerComma + 1 + nextAt).trim())
+        rest = payload.slice(nextAt).replace(/^,/, '').trim()
+        continue
+      }
+
+      if (descriptor) {
+        urls.push(rest.slice(0, headerComma + 1 + descriptor.index).trim())
+        rest = payload.slice(descriptor.index + descriptor[0].length).replace(/^,/, '').trim()
+        continue
+      }
+
+      urls.push(rest)
+      rest = ''
+      continue
+    }
+
+    const commaIndex = rest.indexOf(',')
+    const candidate = (commaIndex === -1 ? rest : rest.slice(0, commaIndex)).trim()
+    const url = candidate.split(/\s+/, 1)[0]
+
+    if (url) {
+      urls.push(url)
+    }
+
+    rest = commaIndex === -1 ? '' : rest.slice(commaIndex + 1).trim()
+  }
+
+  return urls
+}
+
 const allowedAttribute = (attribute, allowedAttributeList) => {
   const attributeName = attribute.nodeName.toLowerCase()
 
   if (allowedAttributeList.includes(attributeName)) {
+    if (attributeName === 'srcset') {
+      const urls = extractSrcsetUrls(attribute.nodeValue)
+      return urls.length > 0 && urls.every(url => isSafeUrl(url))
+    }
+
     if (uriAttributes.has(attributeName)) {
-      return Boolean(SAFE_URL_PATTERN.test(attribute.nodeValue))
+      return isSafeUrl(attribute.nodeValue)
     }
 
     return true
@@ -90,8 +154,25 @@ export function sanitizeHtml(unsafeHtml, allowList, sanitizeFunction) {
     return sanitizeFunction(unsafeHtml)
   }
 
-  const domParser = new window.DOMParser()
-  const createdDocument = domParser.parseFromString(unsafeHtml, 'text/html')
+  // Fail closed if the parser is missing, clobbered, or throws. Returning the
+  // original string here would skip sanitization (the Bootstrap 3 DOM-clobber
+  // pattern). Returning an empty string drops the HTML instead.
+  let createdDocument
+
+  try {
+    if (typeof window.DOMParser !== 'function') {
+      return ''
+    }
+
+    createdDocument = new window.DOMParser().parseFromString(unsafeHtml, 'text/html')
+
+    if (!createdDocument || !createdDocument.body) {
+      return ''
+    }
+  } catch {
+    return ''
+  }
+
   const elements = [].concat(...createdDocument.body.querySelectorAll('*'))
 
   for (const element of elements) {

js/tests/unit/util/sanitizer.spec.js

@@ -159,5 +159,84 @@ describe('Sanitizer', () => {
       expect(firstResult).toContain('src')
       expect(secondResult).toContain('src')
     })
+
+    it('should keep safe srcset candidates', () => {
+      const template = '<img src="safe.jpg" srcset="safe.jpg 1x, /images/two.png 2x">'
+
+      const result = sanitizeHtml(template, DefaultAllowlist, null)
+
+      expect(result).toContain('srcset=')
+      expect(result).toContain('safe.jpg')
+      expect(result).toContain('/images/two.png')
+    })
+
+    it('should drop srcset when any candidate is a javascript: URL', () => {
+      // eslint-disable-next-line no-script-url
+      const unsafeSrcset = 'javascript:alert(1)'
+      const template = `<img src="safe.jpg" srcset="${unsafeSrcset}">`
+
+      const result = sanitizeHtml(template, DefaultAllowlist, null)
+
+      expect(result).not.toContain('srcset')
+    })
+
+    it('should drop a mixed srcset if one candidate is unsafe', () => {
+      // eslint-disable-next-line no-script-url
+      const unsafeSrcset = 'javascript:alert(1)'
+      const template = `<img src="safe.jpg" srcset="safe.jpg 1x, ${unsafeSrcset} 2x">`
+
+      const result = sanitizeHtml(template, DefaultAllowlist, null)
+
+      expect(result).not.toContain('srcset')
+    })
+
+    it('should drop srcset when a data: candidate is followed by an unsafe candidate', () => {
+      // eslint-disable-next-line no-script-url
+      const unsafeSrcset = 'javascript:alert(1)'
+      const template = `<img src="safe.jpg" srcset="data:image/png;base64,AAAA , ${unsafeSrcset} 2x">`
+
+      const result = sanitizeHtml(template, DefaultAllowlist, null)
+
+      expect(result).not.toContain('srcset')
+    })
+
+    it('should drop srcset when the next candidate follows a data: URL with no space', () => {
+      // eslint-disable-next-line no-script-url
+      const unsafeSrcset = 'javascript:alert(1)'
+      const template = `<img src="safe.jpg" srcset="data:image/png;base64,AAAA,${unsafeSrcset} 2x">`
+
+      const result = sanitizeHtml(template, DefaultAllowlist, null)
+
+      expect(result).not.toContain('srcset')
+    })
+
+    it('should keep a data-URI srcset whose commas belong to the payload', () => {
+      const dataUrl = 'data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAABAAAAAQCAYAAAAf8/'
+      const template = `<img src="safe.jpg" srcset="${dataUrl} 1x">`
+
+      const result = sanitizeHtml(template, DefaultAllowlist, null)
+
+      expect(result).toContain('srcset=')
+      expect(result).toContain('data:image/png;base64,')
+    })
+
+    it('should return an empty string if DOMParser is unavailable', () => {
+      const original = window.DOMParser
+      window.DOMParser = undefined
+
+      const result = sanitizeHtml('<div><a href="https://example.com">x</a></div>', DefaultAllowlist, null)
+
+      window.DOMParser = original
+
+      expect(result).toEqual('')
+    })
+
+    it('should return an empty string if DOMParser throws', () => {
+      spyOn(DOMParser.prototype, 'parseFromString').and.throwError('clobbered')
+
+      const result = sanitizeHtml('<div>content</div>', DefaultAllowlist, null)
+
+      expect(result).toEqual('')
+    })
   })
 })