'Save as' should download real PDF location

Fix #2954

Auditors: @bbondy

js/components/frame.js

@@ -394,8 +394,11 @@ class Frame extends ImmutableComponent {
         // TODO: Make the URL bar show the view-source: prefix
         break
       case 'save':
+        const downloadLocation = getSetting(settings.PDFJS_ENABLED)
+          ? UrlUtil.getLocationIfPDF(this.webview.getURL())
+          : this.webview.getURL()
         // TODO: Sometimes this tries to save in a non-existent directory
-        this.webview.downloadURL(this.webview.getURL())
+        this.webview.downloadURL(downloadLocation)
         break
       case 'print':
         this.webview.print()

js/components/urlBar.js

@@ -19,10 +19,10 @@ const { getSetting } = require('../settings')
 const settings = require('../constants/settings')
 const contextMenus = require('../contextMenus')
 const dndData = require('../dndData')
-const pdfjsExtensionId = require('../constants/config').PDFJSExtensionId
 const windowStore = require('../stores/windowStore')
 var searchProviders = require('../data/searchProviders')
 const searchIconSize = 16
+const UrlUtil = require('../lib/urlutil')
 
 const { isUrl, isIntermediateAboutPage } = require('../lib/appUrlUtil')
 
@@ -316,9 +316,8 @@ class UrlBar extends ImmutableComponent {
 
     // We can extend the conditions if there are more chrome-extension to
     // truncate
-    if (getSetting(settings.PDFJS_ENABLED) &&
-        location.startsWith(`chrome-extension://${pdfjsExtensionId}/`)) {
-      location = location.replace(/^chrome-extension:\/\/.+\/(\w+:\/\/.+)/, '$1')
+    if (getSetting(settings.PDFJS_ENABLED)) {
+      location = UrlUtil.getLocationIfPDF(location)
     }
 
     return ['about:blank', 'about:newtab'].includes(this.props.urlbar.get('location'))

js/lib/urlutil.js

@@ -10,6 +10,7 @@ const defaultScheme = 'http://'
 const fileScheme = 'file://'
 const os = require('os')
 const urlParse = require('url').parse
+const pdfjsExtensionId = require('../constants/config').PDFJSExtensionId
 
 /**
  * A simple class for parsing and dealing with URLs.
@@ -287,6 +288,19 @@ const UrlUtil = {
       !['/search', '/search/'].includes(parsed.pathname)
   },
 
+  /**
+   * Gets PDF location from a potential PDFJS URL
+   * @param {string} url
+   * @return {string}
+   */
+  getLocationIfPDF: function (url) {
+    if (url && url.startsWith(`chrome-extension://${pdfjsExtensionId}/`)) {
+      return url.replace(/^chrome-extension:\/\/.+\/(\w+:\/\/.+)/, '$1')
+    } else {
+      return url
+    }
+  },
+
   /**
    * Gets the default favicon URL for a URL.
    * @param {string} url The URL to find a favicon for

test/unit/lib/urlutilTest.js

@@ -184,6 +184,19 @@ describe('urlutil', function () {
     })
   })
 
+  describe('getLocationIfPDF', function () {
+    it('gets location for PDF JS URL', function () {
+      assert.equal(UrlUtil.getLocationIfPDF('chrome-extension://oemmndcbldboiebfnladdacbdfmadadm/https://www.blackhat.co…king-Kernel-Address-Space-Layout-Randomization-KASLR-With-Intel-TSX-wp.pdf'),
+        'https://www.blackhat.co…king-Kernel-Address-Space-Layout-Randomization-KASLR-With-Intel-TSX-wp.pdf')
+    })
+    it('does not modify location for non-pdf URL', function () {
+      assert.equal(UrlUtil.getLocationIfPDF('https://www.blackhat.co…king-Kernel-Address-Space-Layout-Randomization-KASLR-With-Intel-TSX-wp.pdf'),
+        'https://www.blackhat.co…king-Kernel-Address-Space-Layout-Randomization-KASLR-With-Intel-TSX-wp.pdf')
+      assert.equal(UrlUtil.getLocationIfPDF('chrome-extension://blank'), 'chrome-extension://blank')
+      assert.equal(UrlUtil.getLocationIfPDF(null), null)
+    })
+  })
+
   describe('getDefaultFaviconUrl', function () {
     it('returns empty string if input is not a URL', function () {
       assert.equal(UrlUtil.getDefaultFaviconUrl('invalid-url-goes-here'), '')