Fix #10554, #11059: remove check for .pdf in URL extensions, prefer response headers

app/browser/tabs.js

@@ -10,7 +10,7 @@ const tabState = require('../common/state/tabState')
 const {app, extensions, session, ipcMain} = require('electron')
 const {makeImmutable, makeJS} = require('../common/state/immutableUtil')
 const {getExtensionsPath, getTargetAboutUrl, getSourceAboutUrl, isSourceAboutUrl, newFrameUrl, isTargetAboutUrl, isIntermediateAboutPage, isTargetMagnetUrl, getSourceMagnetUrl} = require('../../js/lib/appUrlUtil')
-const {isURL, getUrlFromInput, toPDFJSLocation, getDefaultFaviconUrl, isHttpOrHttps, getLocationIfPDF} = require('../../js/lib/urlutil')
+const {isURL, getUrlFromInput, getDefaultFaviconUrl, isHttpOrHttps, getLocationIfPDF} = require('../../js/lib/urlutil')
 const {isSessionPartition, isTor} = require('../../js/state/frameStateUtil')
 const {getOrigin} = require('../../js/lib/urlutil')
 const settingsStore = require('../../js/settings')
@@ -53,9 +53,6 @@ const normalizeUrl = function (url) {
   if (isURL(url)) {
     url = getUrlFromInput(url)
   }
-  if (settingsStore.getSetting(settings.PDFJS_ENABLED)) {
-    url = toPDFJSLocation(url)
-  }
   return url
 }
 

js/lib/urlutil.js

@@ -375,19 +375,6 @@ const UrlUtil = {
     return `${viewerBaseUrl}?file=${encodeURIComponent(url)}`
   },
 
-  /**
-   * Converts a potential PDF URL to the PDFJS URL.
-   * XXX: This only looks at the URL file extension, not MIME types.
-   * @param {string} url
-   * @return {string}
-   */
-  toPDFJSLocation: function (url) {
-    if (url && UrlUtil.isHttpOrHttps(url) && UrlUtil.isFileType(url, 'pdf')) {
-      return UrlUtil.getPDFViewerUrl(url)
-    }
-    return url
-  },
-
   /**
    * Gets the default favicon URL for a URL.
    * @param {string} url The URL to find a favicon for

test/lib/server.js

@@ -77,6 +77,21 @@ Server.prototype = {
     })
   },
 
+  /**
+   * Allows replacing the default headers sent with a url
+   * @param {String} url for response
+   * @param {Object} new headers to use with response
+   */
+  defineHeaders: function (url, headers) {
+    this.child.send({
+      action: 'defineHeaders',
+      args: {
+        url: url,
+        headers: headers
+      }
+    })
+  },
+
   /**
    * Protects a URL using HTTP authentication.
    * @param {String} url to protect

test/lib/serverChild.js

@@ -2,8 +2,45 @@ var http = require('http')
 var emptyPort = require('empty-port')
 var nodeStatic = require('node-static')
 
+var url = require('url')
 var EventEmitter = require('events').EventEmitter
 
+/**
+ * Patch node-static to allow passing headers. Based on .serve()
+ * https://github.com/cloudhead/node-static/blob/9fabe339698e88594ac81ddc2cb0a7065ad98113/lib/node-static.js#L164-L190
+ */
+nodeStatic.Server.prototype.serveWithHeaders = function (req, res, headers, callback) {
+  var that = this
+  var promise = new EventEmitter()
+  var pathname
+
+  var finish = function (status, headers) {
+    that.finish(status, headers, req, res, promise, callback)
+  }
+
+  try {
+    pathname = decodeURI(url.parse(req.url).pathname)
+  } catch (e) {
+    return process.nextTick(function () {
+      return finish(400, {})
+    })
+  }
+
+  // We assume this is being called in order to pass in headers
+  // but in case not, make sure we have something.
+  headers = headers || {}
+
+  process.nextTick(function () {
+    that.servePath(pathname, 200, headers, req, res, finish)
+    .on('success', function (result) {
+      promise.emit('success', result)
+    }).on('error', function (err) {
+      promise.emit('error', err)
+    })
+  })
+  if (!callback) { return promise }
+}
+
 var root = process.argv[2]
 
 var server
@@ -36,6 +73,11 @@ Server.prototype = {
    */
   authUrls: {},
 
+  /**
+   * A map of URLs for which we need custom Headers sent
+   */
+  urlsWithHeaders: {},
+
   stop: function () {
     if (this.http) {
       try {
@@ -110,8 +152,13 @@ Server.prototype = {
           return
         }
 
-        // hand off request to node-static
-        file.serve(req, res)
+        // If we have been asked to send our own headers for this URL, do that
+        if (server.urlsWithHeaders[fullUrl]) {
+          file.serveWithHeaders(req, res, server.urlsWithHeaders[fullUrl])
+        } else {
+          // Otherwise, hand off request to regular node-static handling
+          file.serve(req, res)
+        }
       }).resume()
     }).listen(port)
   },
@@ -137,6 +184,10 @@ Server.prototype = {
     this.authUrls[url] = true
   },
 
+  defineHeaders: function (options) {
+    this.urlsWithHeaders[options.url] = options.headers
+  },
+
   unprotect: function (url) {
     delete this.authUrls[url]
   }
@@ -174,6 +225,9 @@ process.on('message', function (data) {
     case 'unprotect':
       server.unprotect(data.args)
       break
+    case 'defineHeaders':
+      server.defineHeaders(data.args)
+      break
     case 'stop':
       server.stop()
       break

test/tab-components/frameTest.js

@@ -2,6 +2,7 @@
 
 const Brave = require('../lib/brave')
 const {urlInput,
+  activeTabTitle,
   pinnedTabsTabs,
   backButtonEnabled,
   backButtonDisabled,
@@ -278,6 +279,21 @@ describe('frame tests', function () {
         .url(url)
         .waitForVisible('#viewerContainer')
     })
+
+    it('loads HTML properly despite having .pdf extension', function * () {
+      // Add custom headers for this request, so we can override the
+      // default mime type that will otherwise get added for a .pdf file.
+      let customHeaders = { 'Content-Type': 'text/html; charset=utf-8' }
+      let url = Brave.server.url('html_with_pdf_extension.pdf')
+      Brave.server.defineHeaders(url, customHeaders)
+
+      yield this.app.client
+        .tabByIndex(0)
+        .url(url)
+        .waitForUrl(url)
+        .windowByUrl(Brave.browserWindowUrl)
+        .waitForTextValue(activeTabTitle, 'HTML using .pdf Extension')
+    })
   })
 })
 

test/unit/lib/urlutilTestComponents.js

@@ -210,22 +210,6 @@ module.exports = {
     }
   },
 
-  'toPDFJSLocation': {
-    'pdf': (test) => {
-      const baseUrl = 'chrome-extension://jdbefljfgobbmcidnmpjamcbhnbphjnb/'
-      test.equal(urlUtil().toPDFJSLocation('http://abc.com/test.pdf'), baseUrl + 'content/web/viewer.html?file=http%3A%2F%2Fabc.com%2Ftest.pdf')
-    },
-    'non-pdf': (test) => {
-      test.equal(urlUtil().toPDFJSLocation('http://abc.com/test.pdf.txt'), 'http://abc.com/test.pdf.txt')
-    },
-    'file url': (test) => {
-      test.equal(urlUtil().toPDFJSLocation('file://abc.com/test.pdf.txt'), 'file://abc.com/test.pdf.txt')
-    },
-    'empty': (test) => {
-      test.equal(urlUtil().toPDFJSLocation(''), '')
-    }
-  },
-
   'getPDFViewerUrl': {
     'regular url': (test) => {
       const baseUrl = 'chrome-extension://jdbefljfgobbmcidnmpjamcbhnbphjnb/content/web/viewer.html?file='