Merge tag '0.25.11' into develop

CenterForOpenScience · Jun 8, 2018 · cadecfe · cadecfe
2 parents d8c0744 + af9f9e0
commit cadecfe
Show file tree

Hide file tree

Showing 11 changed files with 114 additions and 35 deletions.
diff --git a/CHANGELOG b/CHANGELOG
@@ -2,6 +2,10 @@
 ChangeLog
 *********
 
+0.25.11 (2018-06-08)
+====================
+- Fix: Percent-encode quote characters in urls to avoid breaking some renderers.
+
 0.25.10 (2018-06-06)
 ====================
 - Fix: Brown-paper-bag release: actual change version in the code.

diff --git a/mfr/extensions/audio/render.py b/mfr/extensions/audio/render.py
@@ -3,6 +3,7 @@
 from mako.lookup import TemplateLookup
 
 from mfr.core import extension
+from mfr.extensions.utils import escape_url_for_template
 
 
 class AudioRenderer(extension.BaseRenderer):
@@ -13,7 +14,8 @@ class AudioRenderer(extension.BaseRenderer):
         ]).get_template('viewer.mako')
 
     def render(self):
-        return self.TEMPLATE.render(base=self.assets_url, url=self.url)
+        safe_url = escape_url_for_template(self.url)
+        return self.TEMPLATE.render(base=self.assets_url, url=safe_url)
 
     @property
     def file_required(self):

diff --git a/mfr/extensions/image/render.py b/mfr/extensions/image/render.py
@@ -5,7 +5,7 @@
 
 from mfr.core import extension
 from mfr.extensions.image import settings
-from mfr.extensions.utils import munge_url_for_localdev
+from mfr.extensions.utils import munge_url_for_localdev, escape_url_for_template
 
 
 class ImageRenderer(extension.BaseRenderer):
@@ -19,7 +19,8 @@ def render(self):
         self.metrics.add('needs_export', False)
         if self.metadata.ext in settings.EXPORT_EXCLUSIONS:
             download_url = munge_url_for_localdev(self.url)
-            return self.TEMPLATE.render(base=self.assets_url, url=download_url.geturl())
+            safe_url = escape_url_for_template(download_url.geturl())
+            return self.TEMPLATE.render(base=self.assets_url, url=safe_url)
 
         exported_url = furl.furl(self.export_url)
         if settings.EXPORT_MAXIMUM_SIZE and settings.EXPORT_TYPE:
@@ -28,10 +29,12 @@ def render(self):
             exported_url.args['format'] = settings.EXPORT_TYPE
         else:
             download_url = munge_url_for_localdev(self.url)
-            return self.TEMPLATE.render(base=self.assets_url, url=download_url.geturl())
+            safe_url = escape_url_for_template(download_url.geturl())
+            return self.TEMPLATE.render(base=self.assets_url, url=safe_url)
 
         self.metrics.add('needs_export', True)
-        return self.TEMPLATE.render(base=self.assets_url, url=exported_url.url)
+        safe_url = escape_url_for_template(exported_url.url)
+        return self.TEMPLATE.render(base=self.assets_url, url=safe_url)
 
     @property
     def file_required(self):

diff --git a/mfr/extensions/jsc3d/render.py b/mfr/extensions/jsc3d/render.py
@@ -7,7 +7,7 @@
 
 from mfr.core import extension
 from mfr.extensions.jsc3d import settings
-from mfr.extensions.utils import munge_url_for_localdev
+from mfr.extensions.utils import munge_url_for_localdev, escape_url_for_template
 
 
 class JSC3DRenderer(extension.BaseRenderer):
@@ -21,18 +21,20 @@ def render(self):
         self.metrics.add('needs_export', False)
         if self.metadata.ext in settings.EXPORT_EXCLUSIONS:
             download_url = munge_url_for_localdev(self.metadata.download_url)
+            safe_url = escape_url_for_template(download_url.geturl())
             return self.TEMPLATE.render(
                 base=self.assets_url,
-                url=download_url.geturl(),
+                url=safe_url,
                 ext=self.metadata.ext.lower(),
             )
 
         exported_url = furl.furl(self.export_url)
         exported_url.args['format'] = settings.EXPORT_TYPE
         self.metrics.add('needs_export', True)
+        safe_url = escape_url_for_template(exported_url.url)
         return self.TEMPLATE.render(
             base=self.assets_url,
-            url=exported_url.url,
+            url=safe_url,
             ext=settings.EXPORT_TYPE,
         )
 

diff --git a/mfr/extensions/pdb/render.py b/mfr/extensions/pdb/render.py
@@ -6,7 +6,7 @@
 
 from mfr.core import extension
 from mfr.extensions.pdb import settings
-from mfr.extensions.utils import munge_url_for_localdev
+from mfr.extensions.utils import munge_url_for_localdev, escape_url_for_template
 
 
 class PdbRenderer(extension.BaseRenderer):
@@ -18,9 +18,10 @@ class PdbRenderer(extension.BaseRenderer):
 
     def render(self):
         download_url = munge_url_for_localdev(self.metadata.download_url)
+        safe_url = escape_url_for_template(download_url.geturl())
         return self.TEMPLATE.render(
             base=self.assets_url,
-            url=download_url.geturl(),
+            url=safe_url,
             options=json.dumps(settings.OPTIONS),
         )
 

diff --git a/mfr/extensions/pdf/render.py b/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,14 +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.lower() 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(),
-                enable_hypothesis=settings.ENABLE_HYPOTHESIS
+                url=escape_url_for_template(download_url.geturl()),
+                enable_hypothesis=settings.ENABLE_HYPOTHESIS,
             )
 
         logger.debug('Extension found in supported list!')
@@ -41,14 +43,15 @@ def render(self):
             self.metrics.add('needs_export', True)
             return self.TEMPLATE.render(
                 base=self.assets_url,
-                url=exported_url.url,
+                url=escape_url_for_template(exported_url.url),
                 enable_hypothesis=settings.ENABLE_HYPOTHESIS
             )
 
+        # TODO: is this dead code? ``settings.EXPORT_TYPE`` is never None or empty
         return self.TEMPLATE.render(
             base=self.assets_url,
-            url=download_url.geturl(),
-            enable_hypothesis=settings.ENABLE_HYPOTHESIS
+            url=escape_url_for_template(download_url.geturl()),
+            enable_hypothesis=settings.ENABLE_HYPOTHESIS,
         )
 
     @property

diff --git a/mfr/extensions/svg/render.py b/mfr/extensions/svg/render.py
@@ -4,6 +4,7 @@
 from mako.lookup import TemplateLookup
 
 from mfr.core import extension
+from mfr.extensions.utils import escape_url_for_template
 
 
 class SvgRenderer(extension.BaseRenderer):
@@ -14,7 +15,8 @@ class SvgRenderer(extension.BaseRenderer):
         ]).get_template('viewer.mako')
 
     def render(self):
-        return self.TEMPLATE.render(base=self.assets_url, url=self.url)
+        safe_url = escape_url_for_template(self.url)
+        return self.TEMPLATE.render(base=self.assets_url, url=safe_url)
 
     @property
     def file_required(self):

diff --git a/mfr/extensions/utils.py b/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: bool=True) -> 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
diff --git a/mfr/extensions/video/render.py b/mfr/extensions/video/render.py
@@ -3,7 +3,7 @@
 from mako.lookup import TemplateLookup
 
 from mfr.core import extension
-from mfr.extensions.utils import munge_url_for_localdev
+from mfr.extensions.utils import munge_url_for_localdev, escape_url_for_template
 
 
 class VideoRenderer(extension.BaseRenderer):
@@ -15,7 +15,8 @@ class VideoRenderer(extension.BaseRenderer):
 
     def render(self):
         download_url = munge_url_for_localdev(self.metadata.download_url)
-        return self.TEMPLATE.render(url=download_url.geturl())
+        safe_url = escape_url_for_template(download_url.geturl())
+        return self.TEMPLATE.render(url=safe_url)
 
     @property
     def file_required(self):

diff --git a/mfr/version.py b/mfr/version.py
@@ -1 +1 @@
-__version__ = '0.25.10'
+__version__ = '0.25.11'
diff --git a/tests/extensions/pdf/test_renderer.py b/tests/extensions/pdf/test_renderer.py
@@ -1,25 +1,28 @@
 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 tif_metadata():
     return ProviderMetadata('test', '.tif', 'text/plain', '1234',
                             'http://wb.osf.io/file/test.tif?token=1234')
 
+
 @pytest.fixture
 def file_path():
     return '/tmp/test.pdf'
 
+
 @pytest.fixture
 def tif_file_path():
     return '/tmp/test.tif'
@@ -42,13 +45,14 @@ def assets_url():
 
 @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 renderer(metadata, file_path, url, assets_url, export_url):
     return PdfRenderer(metadata, file_path, url, assets_url, export_url)
 
+
 @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)
@@ -62,6 +66,23 @@ 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, assets_url):
+
+        download_url = 'http://wb.osf.io/file/te\'st.pdf?token=1234'
+        safe_download_url = 'http://wb.osf.io/file/te%27st.pdf?token=1234'
+
+        metadata = ProviderMetadata('te\'st', '.pdf', 'text/plain', '1234', download_url)
+        renderer = PdfRenderer(metadata, '/tmp/te\'st.pdf', 'http://osf.io/file/te\'st.pdf',
+                               assets_url,
+                               'http://mfr.osf.io/export?url=http://osf.io/file/te\'st.pdf')
+
+        body = 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(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 +92,19 @@ 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, assets_url):
+
+        export_url = 'http://mfr.osf.io/export?url=http://osf.io/file/te\'st.docx&format=pdf'
+        safe_url =  'http://mfr.osf.io/export?url=http://osf.io/file/te%27st.docx&format=pdf'
+
+        metadata = ProviderMetadata('te\'st', '.docx', 'text/plain', '1234', export_url)
+        renderer = PdfRenderer(metadata, '/tmp/te\'st.docx', export_url, assets_url,
+                               'http://mfr.osf.io/export?url=http://osf.io/file/te\'st.docx')
+
+        body = 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(export_url) not in body
+        assert 'DEFAULT_URL = \'{}\''.format(safe_url) in body