Fix PDF renderer with better URL escaping

 - Add a helper method for escaping URL which (1) escapes both
   single and double quote(s) to %27 and %22 respectively and
   (2) logs whenever an unsafe URL is detected and replaced
 - Fix all three render actions and update tests

Author: cslzchen

mfr/extensions/pdf/render.py

@@ -1,13 +1,12 @@
 import logging
 import os
-import re
 
 import furl
 from mako.lookup import TemplateLookup
 
 from mfr.core import extension
 from mfr.extensions.pdf import settings
-from mfr.extensions.utils import munge_url_for_localdev
+from mfr.extensions.utils import munge_url_for_localdev, escape_url_for_template
 
 logger = logging.getLogger(__name__)
 
@@ -20,14 +19,15 @@ class PdfRenderer(extension.BaseRenderer):
         ]).get_template('viewer.mako')
 
     def render(self):
+
         download_url = munge_url_for_localdev(self.metadata.download_url)
         logger.debug('extension::{}  supported-list::{}'.format(self.metadata.ext,
                                                                 settings.EXPORT_SUPPORTED))
         if self.metadata.ext not in settings.EXPORT_SUPPORTED:
             logger.debug('Extension not found in supported list!')
             return self.TEMPLATE.render(
                 base=self.assets_url,
-                url=re.sub(r'\'', '\\\'', download_url.geturl())
+                url=escape_url_for_template(download_url.geturl())
             )
 
         logger.debug('Extension found in supported list!')
@@ -40,9 +40,16 @@ def render(self):
                 exported_url.args['format'] = settings.EXPORT_TYPE
 
             self.metrics.add('needs_export', True)
-            return self.TEMPLATE.render(base=self.assets_url, url=exported_url.url)
+            return self.TEMPLATE.render(
+                base=self.assets_url,
+                url=escape_url_for_template(exported_url.url)
+            )
 
-        return self.TEMPLATE.render(base=self.assets_url, url=download_url.geturl())
+        # TODO: is this dead code? ``settings.EXPORT_TYPE`` is never None or empty
+        return self.TEMPLATE.render(
+            base=self.assets_url,
+            url=escape_url_for_template(download_url.geturl())
+        )
 
     @property
     def file_required(self):

mfr/extensions/utils.py

@@ -1,25 +1,49 @@
+import logging
+from typing import Tuple
 from urllib.parse import parse_qs, urlencode, urlparse
 
 from mfr.extensions import settings
 
+logger = logging.getLogger(__name__)
 
-def munge_url_for_localdev(url):
-    """
-    If MFR is being run in a local development environment (i.e. LOCAL_DEVELOPMENT is True), we
+
+def munge_url_for_localdev(url: str) -> Tuple:
+    """If MFR is being run in a local development environment (i.e. LOCAL_DEVELOPMENT is True), we
     need to replace the internal host (the one the backend services communicate on, default:
     192.168.168.167) with the external host (the one the user provides, default: "localhost")
     e.g. http://192.168.168.167:7777/foo/bar => http://localhost:7777/foo/bar
     """
+
     url_obj = urlparse(url)
-    if (settings.LOCAL_DEVELOPMENT and url_obj.hostname == settings.DOCKER_LOCAL_HOST):
-            query_dict = parse_qs(url_obj.query, keep_blank_values=True)
+    if settings.LOCAL_DEVELOPMENT and url_obj.hostname == settings.DOCKER_LOCAL_HOST:
+        query_dict = parse_qs(url_obj.query, keep_blank_values=True)
 
-            # the 'mode' param will break image downloads from the osf
-            query_dict.pop('mode', None)
+        # the 'mode' param will break image downloads from the osf
+        query_dict.pop('mode', None)
 
-            url_obj = url_obj._replace(
-                query=urlencode(query_dict, doseq=True),
-                netloc='{}:{}'.format(settings.LOCAL_HOST, url_obj.port)
-            )
+        url_obj = url_obj._replace(
+            query=urlencode(query_dict, doseq=True),
+            netloc='{}:{}'.format(settings.LOCAL_HOST, url_obj.port)
+        )
 
     return url_obj
+
+
+def escape_url_for_template(url: str, logs=False) -> str:
+    """Escape (URL Encode) single and double quote(s) for the given URL.
+
+    Download and export URLs may end up not properly encoded right before they are about to be sent
+    to the mako template due to issues including (but not limited to) (1) ``furl`` dropping encoding
+    for single quote (2) URL (provided by users or constructed by scripts) not having the correct
+    encoding. This helper method must be called for each render request that sends URL to the mako
+    template.
+
+    :param str url: the URL to be sent to the mako template
+    :param bool logs: whether to enable warnings, default is `True` and is set to `False` for tests
+    :return: the properly encoded URL
+    """
+
+    safe_url = url.replace('"', '%22').replace("'", '%27')
+    if url != safe_url and logs:
+        logger.warning('Unsafe URL containing unescaped single (double) quote(s) has been replaced')
+    return safe_url

tests/extensions/pdf/test_renderer.py

@@ -1,53 +1,68 @@
 import furl
 import pytest
-import re
 
-from mfr.extensions.pdf import (settings,
-                                PdfRenderer)
 from mfr.core.provider import ProviderMetadata
+from mfr.extensions.pdf import settings, PdfRenderer
+from mfr.extensions.utils import escape_url_for_template
 
 
 @pytest.fixture
 def metadata():
     return ProviderMetadata('test', '.pdf', 'text/plain', '1234',
                             'http://wb.osf.io/file/test.pdf?token=1234')
 
+
+@pytest.fixture
+def metadata_2():
+    return ProviderMetadata('te\'st', '.pdf', 'text/plain', '1234',
+                            'http://wb.osf.io/file/te\'st.pdf?token=1234')
+
+
 @pytest.fixture
 def tif_metadata():
     return ProviderMetadata('test', '.tif', 'text/plain', '1234',
                             'http://wb.osf.io/file/test.tif?token=1234')
 
+
 @pytest.fixture
 def docx_metadata():
-    return ProviderMetadata(
-        'te\'st',
-        '.docx',
-        'text/plain',
-        '1234',
-        'http://mfr.osf.io/export?url=http://osf.io/file/te\'st.pdf'
-    )
+    return ProviderMetadata('te\'st', '.docx', 'text/plain','1234',
+                            'http://mfr.osf.io/export?url=http://osf.io/file/te\'st.pdf')
+
 
 @pytest.fixture
 def file_path():
     return '/tmp/test.pdf'
 
+
+@pytest.fixture
+def file_path_2():
+    return '/tmp/te\'st.pdf'
+
+
 @pytest.fixture
 def tif_file_path():
     return '/tmp/test.tif'
 
+
 @pytest.fixture
 def docx_file_path():
     return '/tmp/te\'st.docx'
 
 
 @pytest.fixture
 def url():
+    return 'http://osf.io/file/test.pdf'
+
+
+@pytest.fixture
+def url_2():
     return 'http://osf.io/file/te\'st.pdf'
 
 
 @pytest.fixture
 def tif_url():
-    return 'http://osf.io/file/te\'st.tif'
+    return 'http://osf.io/file/test.tif'
 
 
 @pytest.fixture
@@ -62,6 +77,11 @@ def assets_url():
 
 @pytest.fixture
 def export_url():
+    return 'http://mfr.osf.io/export?url=http://osf.io/file/test.pdf'
+
+
+@pytest.fixture
+def export_url_2():
     return 'http://mfr.osf.io/export?url=http://osf.io/file/te\'st.pdf'
 
 
@@ -71,31 +91,18 @@ def renderer(metadata, file_path, url, assets_url, export_url):
 
 
 @pytest.fixture
-def tif_renderer(
-    tif_metadata,
-    tif_file_path,
-    tif_url,
-    assets_url,
-    export_url
-):
+def renderer_2(metadata_2, file_path_2, url_2, assets_url, export_url_2):
+    return PdfRenderer(metadata_2, file_path_2, url_2, assets_url, export_url_2)
+
+
+@pytest.fixture
+def tif_renderer(tif_metadata, tif_file_path, tif_url, assets_url, export_url):
     return PdfRenderer(tif_metadata, tif_file_path, tif_url, assets_url, export_url)
 
 
 @pytest.fixture
-def docx_renderer(
-    docx_metadata,
-    docx_file_path,
-    docx_url,
-    assets_url,
-    export_url
-):
-    return PdfRenderer(
-        docx_metadata,
-        docx_file_path,
-        docx_url,
-        assets_url,
-        export_url
-    )
+def docx_renderer(docx_metadata, docx_file_path, docx_url, assets_url, export_url_2):
+    return PdfRenderer(docx_metadata, docx_file_path, docx_url, assets_url, export_url_2)
 
 
 class TestPdfRenderer:
@@ -106,6 +113,16 @@ def test_render_pdf(self, renderer, metadata, assets_url):
         assert '<div id="viewer" class="pdfViewer"></div>' in body
         assert 'DEFAULT_URL = \'{}\''.format(metadata.download_url) in body
 
+    def test_render_pdf_with_single_quote_in_name(self, renderer_2, metadata_2, assets_url):
+
+        body = renderer_2.render()
+        safe_download_url = escape_url_for_template(metadata_2.download_url, logs=False)
+
+        assert '<base href="{}/{}/web/" target="_blank">'.format(assets_url, 'pdf') in body
+        assert '<div id="viewer" class="pdfViewer"></div>' in body
+        assert 'DEFAULT_URL = \'{}\''.format(metadata_2.download_url) not in body
+        assert 'DEFAULT_URL = \'{}\''.format(safe_download_url) in body
+
     def test_render_tif(self, tif_renderer, assets_url):
         exported_url = furl.furl(tif_renderer.export_url)
         exported_url.args['format'] = '{}.{}'.format(settings.EXPORT_MAXIMUM_SIZE,
@@ -117,10 +134,12 @@ def test_render_tif(self, tif_renderer, assets_url):
         assert 'DEFAULT_URL = \'{}\''.format(exported_url.url) in body
 
     def test_render_docx(self, docx_renderer, assets_url):
-        exported_url = furl.furl(docx_renderer.export_url)
 
+        exported_url = furl.furl(docx_renderer.export_url)
+        safe_exported_url = escape_url_for_template(exported_url.url, logs=False)
         body = docx_renderer.render()
+
         assert '<base href="{}/{}/web/" target="_blank">'.format(assets_url, 'pdf') in body
         assert '<div id="viewer" class="pdfViewer"></div>' in body
-        assert 'DEFAULT_URL = \'{}\''.format(re.sub(r'\'', '\\\'', exported_url.url)) in body
-
+        assert 'DEFAULT_URL = \'{}\''.format(exported_url.url) not in body
+        assert 'DEFAULT_URL = \'{}\''.format(safe_exported_url) in body