Merge 7883ff5 into 0e33dcb

mfr/extensions/pdf/render.py

@@ -6,7 +6,7 @@
 
 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__)
 
@@ -19,11 +19,16 @@ 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))
+        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=download_url.geturl())
+            return self.TEMPLATE.render(
+                base=self.assets_url,
+                url=escape_url_for_template(download_url.geturl())
+            )
 
         logger.debug('Extension found in supported list!')
         exported_url = furl.furl(self.export_url)
@@ -35,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=download_url.geturl())
+            return self.TEMPLATE.render(
+                base=self.assets_url,
+                url=escape_url_for_template(exported_url.url)
+            )
+
+        # 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,47 @@
+import logging
+import re
+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)
+        url_obj = url_obj._replace(
+            query=urlencode(query_dict, doseq=True),
+            netloc='{}:{}'.format(settings.LOCAL_HOST, url_obj.port)
+        )
+    return url_obj
 
-            # 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)
-            )
+def escape_url_for_template(url: str, logs=False) -> str:
+    """Escape (URL Encode) single and double quote(s) for the given URL.
 
-    return url_obj
+    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 url: the URL to be sent to the mako template
+    :param logs: whether to enable warnings
+    :return: the properly encoded URL
+    """
+
+    safe_url = re.sub(r'\"', '%22', re.sub(r'\'', '%27', url))
+    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,59 +1,110 @@
 import furl
 import pytest
 
-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')
+
+
 @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/test.tif'
 
 
+@pytest.fixture
+def docx_url():
+    return 'http://osf.io/file/te\'st.tif'
+
+
 @pytest.fixture
 def assets_url():
     return 'http://mfr.osf.io/assets'
 
 
 @pytest.fixture
 def export_url():
-    return 'http://mfr.osf.io/export?url=' + 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'
 
 
 @pytest.fixture
 def renderer(metadata, file_path, url, assets_url, export_url):
     return PdfRenderer(metadata, file_path, url, assets_url, export_url)
 
+
+@pytest.fixture
+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_2):
+    return PdfRenderer(docx_metadata, docx_file_path, docx_url, assets_url, export_url_2)
+
+
 class TestPdfRenderer:
 
     def test_render_pdf(self, renderer, metadata, assets_url):
@@ -62,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)
+
+        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,
@@ -71,3 +132,14 @@ def test_render_tif(self, tif_renderer, assets_url):
         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(exported_url.url) in body
+
+    def test_render_docx(self, docx_renderer, assets_url):
+
+        exported_url = furl.furl(docx_renderer.export_url)
+        safe_exported_url = escape_url_for_template(exported_url.url)
+        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(exported_url.url) not in body
+        assert 'DEFAULT_URL = \'{}\''.format(safe_exported_url) in body