Isolate: Download files as their filename instead of hash

appengine/isolate/handlers_frontend.py

Index: appengine/isolate/handlers_frontend.py
diff --git a/appengine/isolate/handlers_frontend.py b/appengine/isolate/handlers_frontend.py
index 531dc7ae2b14d52565f38712f278f9dee4c6f2a1..00b3d143b42d24d7ab3346ebd2173050fee080e1 100644
--- a/appengine/isolate/handlers_frontend.py
+++ b/appengine/isolate/handlers_frontend.py
@@ -8,7 +8,9 @@ import cgi
 import datetime
 import json
 import logging
+import os
 import re
+import urllib
 
 import webapp2
 
@@ -58,6 +60,16 @@ _GVIZ_COLUMNS_ORDER = (
   'contains_lookups',
 )
 
+_ISOLATED_ROOT_MEMBERS = (
+  'algo',
+  'command',
+  'files',
+  'includes',
+  'read_only',
+  'relative_cwd',
+  'version',
+)
+
 
 ### Restricted handlers
 
@@ -191,7 +203,9 @@ class BrowseHandler(auth.AuthenticatingHandler):
     namespace = self.request.get('namespace', 'default-gzip')
     # Support 'hash' for compatibility with old links. To remove eventually.
     digest = self.request.get('digest', '') or self.request.get('hash', '')
+    save_as = self.request.get('as', '')
     params = {
+      u'as': unicode(save_as),
       u'digest': unicode(digest),
       u'namespace': unicode(namespace),
     }
@@ -258,33 +272,67 @@ class ContentHandler(auth.AuthenticatingHandler):
         '    python isolateserver.py download -I %s --namespace %s -f %s %s'
         % (sizeInMib, host, namespace, digest, digest))
       else:
-        self.response.headers['Content-Disposition'] = str('filename=%s'
-                                                            % digest)
-        if content.startswith('{'):
-          # Try to format as JSON.
-          try:
-            content = json.dumps(
-                json.loads(content), sort_keys=True, indent=2,
-                separators=(',', ': '))
-            content = cgi.escape(content)
-            # If we don't wrap this in html, browsers will put content in a pre
-            # tag which is also styled with monospace/pre-wrap.  We can't use
-            # anchor tags in <pre>, so we force it to be a <div>, which happily
-            # accepts links.
-            content = (
-              '<div style="font-family:monospace;white-space:pre-wrap;">%s'
-              '</div>' % content)
-            # Linkify things that look like hashes
-            content = re.sub(r'([0-9a-f]{40})',
-              r'<a target="_blank" href="/browse?namespace=%s' % namespace +
-                r'&digest=\1">\1</a>',
-              content)
+        self.response.headers['Content-Disposition'] = str(
+          'filename=%s' % self.request.get('as') or digest)
+        try:
+          json_data = json.loads(content)
+          if self._is_isolated_format(json_data):
+            content = self._format_isolated_content(json_data, namespace)
             self.response.headers['Content-Type'] = 'text/html; charset=utf-8'
-          except ValueError:
-            pass
+        except ValueError:
+          pass
 
     self.response.write(content)
 
+  @staticmethod
+  def _is_isolated_format(json_data):
+    """Checks if json_data is a valid .isolated format."""
+    if isinstance(json_data, dict):

[M-A Ruel, 2017/02/16 21:29:12]

for "checker" functions, use early return instead;

if not isinstance(json_data, dict):
  return False
...

[jonesmi, 2017/02/22 21:43:18]

Done.

+      actual = set(json_data)
+      return actual.issubset(_ISOLATED_ROOT_MEMBERS) and 'files' in actual
+    return False
+
+  @staticmethod

[M-A Ruel, 2017/02/16 21:29:12]

@classmethod
def _escape_all_dict_strings(cls, d):

[mithro, 2017/02/16 23:21:35]

Guide from security - DON'T TRY AND ESCAPE STUFF YOURSELF!

https://g3doc.corp.google.com/security/g3doc/ise/xss/prevent.md?cl=head#use-a...

https://wiki.corp.google.com/twiki/bin/view/Main/ISETeamSanitizers

[jonesmi, 2017/02/22 21:43:18]

Looks like we've already configured our jinja2 to use its autoescape
functionality.  Looking at refactoring this to use jinja2 template to take the
security team's advise of using a template system for generating the response,
and autoescaping.

https://github.com/luci/luci-py/blob/master/appengine/components/components/t...

+  def _escape_all_dict_strings(d):
+    """Recursively modify every str val in json dict to be cgi.escape()'d."""
+    for k, v in d.iteritems():
+      if isinstance(v, str):

[M-A Ruel, 2017/02/16 21:29:12]

basestring

[jonesmi, 2017/02/22 21:43:18]

Ack, however getting rid of this method in favor of using jinja2's autoescaper

+        d[k] = cgi.escape(v)
+      elif isinstance(v, dict):

[M-A Ruel, 2017/02/16 21:29:12]

you need to iterate in lists too; so the function should look like this instead:


def _escapce_all_strings(cls, d):
  if isinstance(d, list):
    return [cls._escapce_all_strings(i) for i in d]
  if isinstance(d, dict):
    return {cls._escapce_all_strings(k): cls._escapce_all_strings(v) for k, v in
d.iteritems()}
  if isinstance(d, basestring):
     return cgi.escape(d)
  return d

this changes the condition at the call site, instead of inside the loop, which
is easier to manage conceptually as a state machine.

[jonesmi, 2017/02/22 21:43:18]

Ack, however getting rid of this method in favor of using jinja2's autoescaper

+        d[k] = ContentHandler._escape_all_dict_strings(v)

[M-A Ruel, 2017/02/16 21:29:12]

d[k] = cls._escape_all_dict_strings(v)

[jonesmi, 2017/02/22 21:43:18]

Ack, however getting rid of this method in favor of using jinja2's autoescaper

+    return d
+
+  @staticmethod

[M-A Ruel, 2017/02/16 21:29:12]

classmethod

[jonesmi, 2017/02/22 21:43:18]

Done.

+  def _format_isolated_content(json_data, namespace):
+    """Formats .isolated content and returns a string representation of it."""
+    # Ensure we're working with HTML-safe content.  Do this before adding
+    # our own hyperlinks because cgi.escape would replace our anchor symbols.
+    json_data = ContentHandler._escape_all_dict_strings(json_data)
+
+    # Linkify all files
+    if 'files' in json_data:

[M-A Ruel, 2017/02/16 21:29:12]

check is not needed anymore.

[jonesmi, 2017/02/22 21:43:18]

Done.

+      hyperlinked_files = {}
+      for filepath, metadata in json_data['files'].iteritems():
+        if metadata.get('h'):
+          save_as = os.path.basename(filepath)
+          anchor = (r'<a target=_blank" '
+                    r'href=/browse?namespace=%s&digest=%s&as=%s>'
+                    r'%s</a>') % (
+            namespace, metadata['h'], urllib.quote(save_as), filepath)
+          hyperlinked_files[anchor] = metadata
+      json_data['files'] = hyperlinked_files
+
+    content = json.dumps(
+      json_data, sort_keys=True, indent=2, separators=(',', ': '))
+    # If we don't wrap this in html, browsers will put content in a pre
+    # tag which is also styled with monospace/pre-wrap.  We can't use
+    # anchor tags in <pre>, so we force it to be a <div>, which happily
+    # accepts links.
+    content = (
+      '<div style="font-family:monospace;white-space:pre-wrap;">%s'
+      '</div>' % content)
+    return content
+
 
 class StatsHandler(webapp2.RequestHandler):
   """Returns the statistics web page."""