launchpad-reviewers team mailing list archive

[Abel Deuring <abel.deuring@xxxxxxxxxxxxx>]

Abel Deuring has proposed merging lp:~adeuring/launchpad/bug-612779 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)


This branch fixes bug 612779:user controlled data will be exposed in the launchpad.net domains at the next release.

It ensures that HTTP responses for restircted LFA (ie.e, the content of bug attachments for private bugs) always have the HTTP header Content-Disposition: attachment.

The branch defines a new class SafeStreamOrRedirectLibraryFileAliasView which inherits from canonical.launchpad.browser.librarian.StreamOrRedirectLibraryFileAliasView. The latter class is by default used to serve the content of restricted Librarian files. The COntent-Disposition header is set in the overloaded method __call__().

Instances of StreamOrRedirectLibraryFileAliasView are created in canonical.launchpad.browser.librarian.FileNavigationMixin.traverse_files(). I changed this method so that the view class used in traverse_files() can now be easily changed in derived classes.

Finally, I set the view class used in BugAttachmentFileNavigation to SafeStreamOrRedirectLibraryFileAliasView.

test: ./bin/test -vvt test_bugattachment_file_access

no lint
-- 
https://code.launchpad.net/~adeuring/launchpad/bug-612779/+merge/31637
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~adeuring/launchpad/bug-612779 into lp:launchpad.

=== modified file 'lib/canonical/launchpad/browser/librarian.py'
--- lib/canonical/launchpad/browser/librarian.py	2010-03-26 19:19:49 +0000
+++ lib/canonical/launchpad/browser/librarian.py	2010-08-03 13:52:47 +0000
@@ -42,8 +42,8 @@
     """View to handle redirection for downloading files by URL.
 
     Rather than reference downloadable files via the obscure Librarian
-    URL, downloadable files can be referenced via the Product Release URL, e.g.
-    http://launchpad.net/firefox/1.0./1.0.0/+download/firefox-1.0.0.tgz.
+    URL, downloadable files can be referenced via the Product Release URL,
+    e.g. http://launchpad.net/firefox/1.0./1.0.0/+download/firefox-1.0.0.tgz.
     """
 
     __used_for__ = ILibraryFileAlias
@@ -173,19 +173,20 @@
     extended in order to allow traversing to multiple files potentially
     with the same filename (product files or bug attachments).
     """
+    view_class = StreamOrRedirectLibraryFileAliasView
+
     @stepthrough('+files')
     def traverse_files(self, filename):
         """Traverse on filename in the archive domain."""
         if not check_permission('launchpad.View', self.context):
             raise Unauthorized()
-        library_file  = self.context.getFileByName(filename)
+        library_file = self.context.getFileByName(filename)
 
         # Deleted library files result in NotFound-like error.
         if library_file.deleted:
             raise DeletedProxiedLibraryFileAlias(filename, self.context)
 
-        return StreamOrRedirectLibraryFileAliasView(
-            library_file, self.request)
+        return self.view_class(library_file, self.request)
 
 
 class ProxiedLibraryFileAlias:

=== modified file 'lib/lp/bugs/browser/bugattachment.py'
--- lib/lp/bugs/browser/bugattachment.py	2010-07-27 14:12:47 +0000
+++ lib/lp/bugs/browser/bugattachment.py	2010-08-03 13:52:47 +0000
@@ -10,6 +10,7 @@
     'BugAttachmentSetNavigation',
     'BugAttachmentEditView',
     'BugAttachmentURL',
+    'SafeStreamOrRedirectLibraryFileAliasView',
     ]
 
 from cStringIO import StringIO
@@ -18,7 +19,8 @@
 from zope.component import getUtility
 from zope.contenttype import guess_content_type
 
-from canonical.launchpad.browser.librarian import FileNavigationMixin
+from canonical.launchpad.browser.librarian import (
+    FileNavigationMixin, StreamOrRedirectLibraryFileAliasView)
 from canonical.launchpad.webapp import (
     canonical_url, custom_widget, GetitemNavigation, Navigation)
 from canonical.launchpad.interfaces.librarian import ILibraryFileAliasSet
@@ -227,7 +229,24 @@
         return self.context.type == BugAttachmentType.PATCH
 
 
+class SafeStreamOrRedirectLibraryFileAliasView(
+    StreamOrRedirectLibraryFileAliasView):
+    """A view for Librarian files that sets the content disposion header."""
+
+    def __call__(self):
+        """Stream the content of the context `ILibraryFileAlias`.
+
+        Set the content disposition header to the safe value "attachment".
+        """
+        self.request.response.setHeader(
+            'Content-Disposition', 'attachment')
+        return super(
+            SafeStreamOrRedirectLibraryFileAliasView, self).__call__()
+
+
 class BugAttachmentFileNavigation(Navigation, FileNavigationMixin):
     """Traversal to +files/${filename}."""
 
     usedfor = IBugAttachment
+
+    view_class = SafeStreamOrRedirectLibraryFileAliasView

=== modified file 'lib/lp/bugs/browser/configure.zcml'
--- lib/lp/bugs/browser/configure.zcml	2010-07-27 14:12:47 +0000
+++ lib/lp/bugs/browser/configure.zcml	2010-08-03 13:52:47 +0000
@@ -1159,4 +1159,9 @@
             classes="
                 BugWatchSetNavigation"/>
     </facet>
+    <class class="lp.bugs.browser.bugattachment.SafeStreamOrRedirectLibraryFileAliasView">
+      <allow attributes="__call__" />
+      <allow interface="zope.publisher.interfaces.browser.IBrowserPublisher" />
+    </class>
+
 </configure>

=== modified file 'lib/lp/bugs/browser/tests/test_bugattachment_file_access.py'
--- lib/lp/bugs/browser/tests/test_bugattachment_file_access.py	2010-08-02 10:52:39 +0000
+++ lib/lp/bugs/browser/tests/test_bugattachment_file_access.py	2010-08-03 13:52:47 +0000
@@ -69,7 +69,7 @@
             '^http://localhost:58000/\d+/foo.txt$', next_view.target)
         self.assertIsNot(None, mo)
 
-    def test_access_to_restircted_file(self):
+    def test_access_to_restricted_file(self):
         # Requests of restricted files are handled by ProxiedLibraryFileAlias.
         lfa_with_parent = getMultiAdapter(
             (self.bugattachment.libraryfile, self.bugattachment),
@@ -87,7 +87,7 @@
         file_.seek(0)
         self.assertEqual('file content', file_.read())
 
-    def test_access_to_restircted_file_unauthorized(self):
+    def test_access_to_restricted_file_unauthorized(self):
         # If a user cannot access the bug attachment itself, he can neither
         # access the restricted Librarian file.
         lfa_with_parent = getMultiAdapter(
@@ -104,3 +104,21 @@
         navigation = BugAttachmentFileNavigation(self.bugattachment, request)
         self.assertRaises(
             Unauthorized, navigation.publishTraverse, request, '+files')
+
+    def test_content_disposition_of_restricted_file(self):
+        # The content disposition header of the HTTP response for
+        # restricted Librarian files is always set to "attachment".
+        lfa_with_parent = getMultiAdapter(
+            (self.bugattachment.libraryfile, self.bugattachment),
+            ILibraryFileAliasWithParent)
+        lfa_with_parent.restricted = True
+        self.bug.setPrivate(True, self.bug_owner)
+        transaction.commit()
+        request = LaunchpadTestRequest()
+        request.setTraversalStack(['foo.txt'])
+        navigation = BugAttachmentFileNavigation(self.bugattachment, request)
+        view = navigation.publishTraverse(request, '+files')
+        next_view, traversal_path = view.browserDefault(request)
+        next_view()
+        self.assertEqual(
+            'attachment', request.response.getHeader('Content-Disposition'))