Merge lp:~mkanat/loggerhead/raw-controller into lp:loggerhead

[Max Kanat-Alexander]

This addresses bug 605775 by adding a new "raw" controller that displays raw data. It's surprisingly fast--about 0.05s to display almost any file. It does its best to determine the correct MIME type to serve a file with, although it will often fall back to text/plain or application/octet-stream.

The only thing that this patch is missing is XSS protection (serving the raw content from a different domain), which is something I want to do in a separate bug and MP, because this MP is already complex enough as it is. There's also some preliminary refactoring involved in this patch that was necessary to get a "raw" controller that worked well without copying-and-pasting code. (After this, there's also some refactoring that can be done to make the "download" controller a subclass of RawUI, but that will involve changing the download URLs and how they work, so that's also something I wanted to do separately.)

[John A Meinel (jameinel)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 12/3/2010 1:41 PM, Max Kanat-Alexander wrote:
> Max Kanat-Alexander has proposed merging lp:~mkanat/loggerhead/raw-controller into lp:loggerhead.
>
> Requested reviews:
> Bazaar Developers (bzr)
>
>
> This addresses bug 605775 by adding a new "raw" controller that displays raw data. It's surprisingly fast--about 0.05s to display almost any file. It does its best to determine the correct MIME type to serve a file with, although it will often fall back to text/plain or application/octet-stream.
>
> The only thing that this patch is missing is XSS protection (serving the raw content from a different domain), which is something I want to do in a separate bug and MP, because this MP is already complex enough as it is. There's also some preliminary refactoring involved in this patch that was necessary to get a "raw" controller that worked well without copying-and-pasting code. (After this, there's also some refactoring that can be done to make the "download" controller a subclass of RawUI, but that will involve changing the download URLs and how they work, so that's also something I wanted to do separately.)

If we are worried about XSS, why not start by having raw just return
everything as application/octet-stream until we figure out how to
protect against it?

John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAkz5Y50ACgkQJdeBCYSNAAMItACfUNGl+AHZBIBTQpIfDr9lgiwO
wUkAni4f6kZji70Rq0D8/cRHssArTxBo
=JeEQ
-----END PGP SIGNATURE-----

[Robert Collins (lifeless)]

application/octet-stream is unsafe.

-Rob

[Robert Collins (lifeless)]

See http://msdn.microsoft.com/en-us/library/ms775147(VS.85).aspx for
*one* documented behaviour about content-sniffing.

Basically you need one domain per security context - and for us that
means pretty much one domain per file content. See the
publicrestrictedlibrarian, for instance.

-Rob

[John A Meinel (jameinel)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 12/3/2010 4:07 PM, Robert Collins wrote:
> application/octet-stream is unsafe.
>
> -Rob

We already have a way to get that, though. The 'download' urls do that.
Or are they somehow already protected against XSS, and new stuff
wouldn't be?

John
=:->

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAkz5arEACgkQJdeBCYSNAAOb7gCdH5Il4yjztErqf5GxdNVa7GvU
w5IAoJoQ/Y621aXMNFBRheD2fucpC/lE
=SKtw
-----END PGP SIGNATURE-----

[Robert Collins (lifeless)]

They probably set Content-Disposition: attachment, which (AFAIK) all
browsers honour, at least to the extent of not rendering the thing in
the source context.

-Rob

[Max Kanat-Alexander (mkanat)]

> If we are worried about XSS, why not start by having raw just return
> everything as application/octet-stream until we figure out how to
> protect against it?

  Because we already have /download/ for that. This is about displaying content in the browser.

[Martin Pool (mbp)]

The diff itself looks reasonable to me. The main issue seems to be XSS.

I don't think it is a good idea to merge something into loggerhead trunk that makes it 'dangerous by default'. Some people run from trunk; someone else might make a release without realizing this was not yet finished. If there was an option to enable it, with a clear warning, that would be fine.

(Tangentially, I think we should have a discussion about how we're going to manage qa of the recent loggerhead changes before they're generally available, but that can be separate from this.)

It's great that this is so fast.

John's bug is really two bugs (as he acknowledges, but perhaps we should have split it before continuing.)

> 1) Something like the 'download' link that doesn't require including the file id.

It looks like that is fixed by this, which is great.

Really this is probably mostly independent of the download issue, and it's equally applicable to wanting to make up a url that gives you annotations or any other view.

If Loggerhead still generates urls containing file ids by default it may not be obvious to users they can be left out?

> 2) A way to view raw content in-the-browser, rather than always being an octet stream. So 'raw' vs 'download'.

This one seems to have inherent XSS-type problems, and we should perhaps narrow it down in the context of that bug. What do you want to see in the browser? What are the use cases? I would say almost certainly not arbitrary HTML.

To me the most obvious use case is to get plain text files from which you can easily copy/paste. Perhaps that can be accommodated well by escaping them into HTML?

The second most important case is to get a download URL that you could paste into a shell eg wget command. I think that c-d: attachment will still let this work?

The third might be to click through to png/jpg images within loggerhead. That seems problematic wrt abuse, and opens up to content sniffing.

I don't think we should merge this until we understand more clearly what the bug is trying to fix. If we do decide that a plain raw view is useful to some users, it should be off by default.

review: Needs Information

[Max Kanat-Alexander (mkanat)]

I've addressed all of the questions and comments on bug 605775 comment 5, I believe.

The only point that I didn't make there is that most loggerhead installations would not generally be able to do anything harmful to the user by injecting scripts into the page, I'm somewhat sure. I do agree that we shouldn't ship as dangerous by default, though.

Also, to be clear, loggerhead does not generate URLs that use file ids by default, with the only current exception being /download/. However, I was planning on fixing /download/ after this, which will be pretty simple since it can become a subclass of RawUI. But I didn't want to do two changes in one, particularly since one is a behavioral change for URLs.

I don't see why a plain raw view should be off by default--it seems like something that every VCS browser does. I do think that there should be a way for people to protect themselves against XSS, though. But the fix is complex, somewhat, so I wanted to separate it out so that we could do a good security review on just that fix, and not be distracted by the surrounding refactoring and new /raw/ code.

[John A Meinel (jameinel)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

> I don't see why a plain raw view should be off by default--it seems like something that every VCS browser does. I do think that there should be a way for people to protect themselves against XSS, though. But the fix is complex, somewhat, so I wanted to separate it out so that we could do a good security review on just that fix, and not be distracted by the surrounding refactoring and new /raw/ code.

I would say as long as something is "text/plain" then you could serve it
by default. Otherwise make it an attachment?

John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAkz+ef8ACgkQJdeBCYSNAAMWpgCcC1L/g+R6sDKShHxTKilDfzLm
k+UAn3TLhE5t3mQ8WJRZbKgam7nmtRC/
=YnD3
-----END PGP SIGNATURE-----

[Martin Pool (mbp)]

Let's get this unblocked.

> > I don't see why a plain raw view should be off by default--it seems like
> something that every VCS browser does. I do think that there should be a way
> for people to protect themselves against XSS, though. But the fix is complex,
> somewhat, so I wanted to separate it out so that we could do a good security
> review on just that fix, and not be distracted by the surrounding refactoring
> and new /raw/ code.

I think it's clear some people will want this on and some will want it off. Therefore we need an option. If there is an option that chooses between "invisibly dangerous" and "obviously broken" I think the second is a much better default.

> I would say as long as something is "text/plain" then you could serve it
> by default. Otherwise make it an attachment?

This is the whole problem with xss and content-sniffing: the server can mark it as text/plain, but some browsers will still interpret it as something else if it looks like it's not actually text. They may interpret it as something that lets malicious content steal people's credentials. See Robert's link.

Can you please add an option?

[John A Meinel (jameinel)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 12/14/2010 9:31 PM, Martin Pool wrote:
> Let's get this unblocked.
>
>>> I don't see why a plain raw view should be off by default--it seems like
>> something that every VCS browser does. I do think that there should be a way
>> for people to protect themselves against XSS, though. But the fix is complex,
>> somewhat, so I wanted to separate it out so that we could do a good security
>> review on just that fix, and not be distracted by the surrounding refactoring
>> and new /raw/ code.
>
> I think it's clear some people will want this on and some will want it off. Therefore we need an option. If there is an option that chooses between "invisibly dangerous" and "obviously broken" I think the second is a much better default.
>
>> I would say as long as something is "text/plain" then you could serve it
>> by default. Otherwise make it an attachment?
>
> This is the whole problem with xss and content-sniffing: the server can mark it as text/plain, but some browsers will still interpret it as something else if it looks like it's not actually text. They may interpret it as something that lets malicious content steal people's credentials. See Robert's link.
>
> Can you please add an option?

Matthew Nuzum just put up a test, and I confirmed,
 http://people.canonical.com/~mnuzum/count.html.txt

Is displayed as a text file in my copies of Firefox and Chrome, but
shows up as an HTML page (showing the js content) from Internet Explorer.

John
=:->

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk0IaQcACgkQJdeBCYSNAAO92wCgpQAL8CSmnAuL3xG9eVJa0mGm
xrwAoKmJJMJRXYS1FJkEZFpc+zUMZifu
=7rJs
-----END PGP SIGNATURE-----

[Martin Pool (mbp)]

Ok, so in another branch max has added an option that seems suitable for use on Launchpad.

Max makes a case that this is in fact safe to be on by default, because most Loggerhead instances either don't serve untrusted content, or don't have anything that could be exploited by XSS. He has handled something similar in bugzilla in <https://bugzilla.mozilla.org/show_bug.cgi?id=38862>.

I think this ought to be mentioned in README but aside from that I'm ok with merging this.

review: Approve

[Max Kanat-Alexander (mkanat)]

Accidentally pushed instead of merged, but the end result should be the same.

=== modified file 'loggerhead/apps/branch.py'
--- loggerhead/apps/branch.py	2010-11-30 10:18:40 +0000
+++ loggerhead/apps/branch.py	2010-12-03 19:41:25 +0000
@@ -36,6 +36,7 @@
 from loggerhead.controllers.download_ui import DownloadUI
 from loggerhead.controllers.filediff_ui import FileDiffUI
 from loggerhead.controllers.inventory_ui import InventoryUI
+from loggerhead.controllers.raw_ui import RawUI
 from loggerhead.controllers.revision_ui import RevisionUI
 from loggerhead.controllers.revlog_ui import RevLogUI
 from loggerhead.controllers.search_ui import SearchUI
@@ -113,6 +114,7 @@
         'diff': DiffUI,
         'download': DownloadUI,
         'files': InventoryUI,
+        'raw': RawUI,
         'revision': RevisionUI,
         'search': SearchUI,
         'view': ViewUI,

=== modified file 'loggerhead/controllers/__init__.py'
--- loggerhead/controllers/__init__.py	2010-04-26 19:37:26 +0000
+++ loggerhead/controllers/__init__.py	2010-12-03 19:41:25 +0000
@@ -17,6 +17,7 @@
 # along with this program; if not, write to the Free Software
 # Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
 
+import os
 import time
 
 from paste.request import path_info_pop, parse_querystring
@@ -66,7 +67,7 @@
         return self.__history
 
     def __call__(self, environ, start_response):
-        z = time.time()
+        self._call_start = time.time()
         kwargs = dict(parse_querystring(environ))
         util.set_context(kwargs)
         args = []
@@ -75,12 +76,15 @@
             if arg is None:
                 break
             args.append(arg)
-
+        self.args = args
+        
         path = None
         if len(args) > 1:
             path = unicode('/'.join(args[1:]), 'utf-8')
-        self.args = args
-
+        
+        return self.get_output(path, kwargs, start_response)
+    
+    def get_output(self, path, kwargs, start_response):
         vals = {
             'static_url': self._branch.static_url,
             'branch': self._branch,
@@ -93,7 +97,7 @@
         vals.update(self.get_values(path, kwargs, headers))
 
         self.log.info('Getting information for %s: %.3f secs' % (
-            self.__class__.__name__, time.time() - z))
+            self.__class__.__name__, time.time() - self._call_start))
         if 'Content-Type' not in headers:
             headers['Content-Type'] = 'text/html'
         writer = start_response("200 OK", headers.items())
@@ -105,8 +109,31 @@
         self.log.info(
             'Rendering %s: %.3f secs, %s bytes' % (
                 self.__class__.__name__, time.time() - z, w.bytes))
-        return []
+        return []        
+    
+    def file_info(self, path, kwargs):
+        history = self._history
+        revid = self.get_revid()
+        revid = history.fix_revid(revid)
+        file_id = kwargs.get('file_id', None)
+        if (file_id is None) and (path is None):
+            raise HTTPBadRequest('No file_id or filename provided')
 
+        if file_id is None:
+            file_id = history.get_file_id(revid, path)
+        
+        if path is None:
+            path = history.get_path(revid, file_id)
+        
+        filename = os.path.basename(path)
+        
+        return {
+            'file_id': file_id,
+            'revid': revid,
+            'filename': filename,
+            'path': path,
+        }
+    
     def get_revid(self):
         h = self._history
         if h is None:
@@ -115,3 +142,7 @@
             return h.fix_revid(self.args[0])
         else:
             return h.last_revid
+
+    def tree_for(self, file_id, revid):
+        file_revid = self._history.get_inventory(revid)[file_id].revision
+        return self._history._branch.repository.revision_tree(file_revid)
\ No newline at end of file

=== modified file 'loggerhead/controllers/download_ui.py'
--- loggerhead/controllers/download_ui.py	2010-05-05 19:03:40 +0000
+++ loggerhead/controllers/download_ui.py	2010-12-03 19:41:25 +0000
@@ -26,23 +26,14 @@
 
 from loggerhead.controllers import TemplatedBranchView
 
-log = logging.getLogger("loggerhead.controllers")
-
-
-class DownloadUI (TemplatedBranchView):
-
-    def __call__(self, environ, start_response):
+class DownloadUI(TemplatedBranchView):
+
+    def get_output(self, path, kwargs, start_response):
         # /download/<rev_id>/<file_id>/[filename]
 
         h = self._history
-
-        args = []
-        while True:
-            arg = path_info_pop(environ)
-            if arg is None:
-                break
-            args.append(arg)
-
+        args = self.args
+        
         if len(args) < 2:
             raise httpexceptions.HTTPMovedPermanently(
                 self._branch.absolute_url('/changes'))

=== added file 'loggerhead/controllers/raw_ui.py'
--- loggerhead/controllers/raw_ui.py	1970-01-01 00:00:00 +0000
+++ loggerhead/controllers/raw_ui.py	2010-12-03 19:41:25 +0000
@@ -0,0 +1,60 @@
+#
+# Copyright (C) 2010 Canonical Ltd.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
+#
+
+import mimetypes
+import time
+
+import bzrlib.errors
+import bzrlib.textfile
+
+from loggerhead.controllers import TemplatedBranchView
+
+class RawUI(TemplatedBranchView):
+
+    def get_output(self, path, kwargs, start_response):
+        # /raw/<revno>/[path]
+        file_info = self.file_info(path, kwargs)
+        revid = file_info['revid']
+        file_id = file_info['file_id']
+        filename = file_info['filename']
+
+        h = self._history
+        tree = self.tree_for(file_id, revid)
+        content = tree.get_file_lines(file_id)
+        size = tree.inventory[file_id].text_size
+
+        mime_type, encoding = mimetypes.guess_type(filename)
+        if mime_type is None:
+            try:
+                bzrlib.textfile.check_text_lines(content)
+            except bzrlib.errors.BinaryFile:
+                mime_type = 'application/octet-stream'
+            else:
+                mime_type = 'text/plain'        
+
+        headers = [
+            ('Content-Type', mime_type),
+            ('Content-Length', str(size)),
+            ('X-Content-Type-Options', 'nosniff'),
+        ]
+        
+        self.log.info('/raw %s @ %s (%d bytes): %.3f secs',
+              file_info['path'], revid, size, time.time() - self._call_start)
+        
+        start_response('200 OK', headers)
+        return content

=== modified file 'loggerhead/controllers/view_ui.py'
--- loggerhead/controllers/view_ui.py	2010-12-01 07:13:52 +0000
+++ loggerhead/controllers/view_ui.py	2010-12-03 19:41:25 +0000
@@ -18,7 +18,6 @@
 #
 
 import cgi
-import os
 import time
 
 import bzrlib.errors
@@ -39,12 +38,9 @@
 
     template_path = 'loggerhead.templates.view'
     
-    def tree_for(self, file_id, revid):
-        file_revid = self._history.get_inventory(revid)[file_id].revision
-        return self._history._branch.repository.revision_tree(file_revid)
-
-    def text_lines(self, file_id, revid):
-        file_name = os.path.basename(self._history.get_path(revid, file_id))
+    def text_lines(self, file_info):
+        file_id = file_info['file_id']
+        revid = file_info['revid']
         
         tree = self.tree_for(file_id, revid)
         file_text = tree.get_file_text(file_id)
@@ -60,7 +56,7 @@
         bzrlib.textfile.check_text_lines(file_lines)
         
         if highlight is not None:
-            hl_lines = highlight(file_name, file_text, encoding)
+            hl_lines = highlight(file_info['filename'], file_text, encoding)
             # highlight strips off extra newlines at the end of the file.
             extra_lines = len(file_lines) - len(hl_lines)
             hl_lines.extend([u''] * extra_lines)
@@ -69,9 +65,9 @@
         
         return hl_lines;
 
-    def file_contents(self, file_id, revid):
+    def file_contents(self, file_info):
         try:
-            file_lines = self.text_lines(file_id, revid)
+            file_lines = self.text_lines(file_info)
         except bzrlib.errors.BinaryFile:
             # bail out; this isn't displayable text
             return ['(This is a binary file.)']
@@ -79,25 +75,15 @@
         return file_lines
 
     def get_values(self, path, kwargs, headers):
+        file_info = self.file_info(path, kwargs)
+        path = file_info['path']
+        revid = file_info['revid']
+
+        # no navbar for revisions
+        navigation = util.Container()
+
         history = self._history
         branch = history._branch
-        revid = self.get_revid()
-        revid = history.fix_revid(revid)
-        file_id = kwargs.get('file_id', None)
-        if (file_id is None) and (path is None):
-            raise HTTPBadRequest('No file_id or filename '
-                                 'provided to view')
-
-        if file_id is None:
-            file_id = history.get_file_id(revid, path)
-
-        # no navbar for revisions
-        navigation = util.Container()
-
-        if path is None:
-            path = history.get_path(revid, file_id)
-        filename = os.path.basename(path)
-
         change = history.get_changes([ revid ])[0]
         # If we're looking at the tip, use head: in the URL instead
         if revid == branch.last_revision():
@@ -126,12 +112,12 @@
             # true or not before using it, so we have to define it here also.
             'annotated': False,
             'revno_url': revno_url,
-            'file_id': file_id,
+            'file_id': file_info['file_id'],
             'file_path': path,
-            'filename': filename,
+            'filename': file_info['filename'],
             'navigation': navigation,
             'change': change,
-            'contents':  self.file_contents(file_id, revid),
+            'contents':  self.file_contents(file_info),
             'fileview_active': True,
             'directory_breadcrumbs': directory_breadcrumbs,
             'branch_breadcrumbs': branch_breadcrumbs,

=== modified file 'loggerhead/static/css/files.css'
--- loggerhead/static/css/files.css	2009-06-17 20:28:20 +0000
+++ loggerhead/static/css/files.css	2010-12-03 19:41:25 +0000
@@ -48,3 +48,10 @@
 .bug {
 	background:url(../images/ico_bug.png) no-repeat top left;
 }
+#inventory .expcell {
+	width: 28px;
+	text-align: left;
+}
+.view_link, .raw_link {
+	margin-right: 10px;
+}
\ No newline at end of file

=== modified file 'loggerhead/templates/inventory.pt'
--- loggerhead/templates/inventory.pt	2010-11-30 10:18:40 +0000
+++ loggerhead/templates/inventory.pt	2010-12-03 19:41:25 +0000
@@ -27,7 +27,7 @@
       </tal:block>
     </h1>
 
-    <div metal:fill-slot="content">
+    <div metal:fill-slot="content" id="inventory">
         <tal:branch-info replace="structure python:branchinfo(branch)" />
 
         <p tal:condition="python:not change">
@@ -52,7 +52,6 @@
               <th class="datecell"><a tal:attributes="href python:url(['/files', revid], sort='date')">Last Changed</a></th>
               <th class="timedate"><a tal:attributes="href python:url(['/files', revid], sort='size')">Size</a></th>
               <th class="expandcell"></th>
-              <th class="expandcell"></th>
             </tr>
 
             <tr class="blueRow0" tal:condition="python:updir is not None">
@@ -84,7 +83,6 @@
                                       <img tal:attributes="src python:branch.static_url('/static/images/ico_planilla.gif')" alt="Diff" />
                                      </a>
               </td>
-                <td class="expcell"></td>
             </tr>
 
             <!-- Show this if it's a symlink -->
@@ -108,7 +106,6 @@
                                                                title string:Show revision ${file/change/revno}" />
                                      </a>
               </td>
-                <td class="expcell"></td>
             </tr>
 
             <!-- Show this if it's a regular file -->
@@ -131,13 +128,15 @@
               </td>
               <td class="date" tal:content="python:util.date_time(file.change.date)"></td>
               <td class="timedate2" tal:content="python:util.human_size(file.size)"></td>
-              <td class="expcell"><a tal:attributes="href python:url(['/view', revno_url, file.absolutepath]);
-                                                     title string:View ${file/filename}">
-                                      <img tal:attributes="src python:branch.static_url('/static/images/ico_planilla.gif')" alt="Diff" />
-                                     </a>
-              </td>
               <td class="expcell">
-                 <a tal:attributes="href python:url(['/download', file.revid, file.file_id, file.filename]);
+                <a class="raw_link"
+                   tal:attributes="href python:url(['/raw', revno_url, file.absolutepath]);
+                                   title string:Raw content of ${file/filename}">
+                  <img tal:attributes="src python:branch.static_url('/static/images/ico_file.gif')"
+                       alt="View raw file" />
+                </a>
+                 <a class="download_link"
+                    tal:attributes="href python:url(['/download', file.revid, file.file_id, file.filename]);
                                     title string:Download ${file/filename} at revision ${file/change/revno}">
                    <img tal:attributes="src python:branch.static_url('/static/images/ico_file_download.gif')" alt="Download File" />
                  </a>

=== modified file 'loggerhead/templates/view.pt'
--- loggerhead/templates/view.pt	2010-12-01 08:10:46 +0000
+++ loggerhead/templates/view.pt	2010-12-03 19:41:25 +0000
@@ -51,6 +51,10 @@
                 <a tal:attributes="href python:url(['/changes'], clear=1, filter_file_id=file_id)"
                 >view changes to this file</a>
             </li>
+            <li>
+              <a tal:attributes="href python:url(['/raw', revno_url, file_path], clear=1)"
+              >view raw content</a>
+            </li>
             <li id="last">
                 <a tal:attributes="href python:url(['/download',
                                     revno_url, file_id, filename])"