Merge lp:~adeuring/launchpad/bug-612779 into lp:launchpad/db-devel

[Abel Deuring]

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

[Māris Fogels (mars)]

Hi Abel,

As noted on IRC, I think the class name StreamOrRedirectLibraryFileAliasView indicates a serious object design problem: it sounds like you should have three polymorphic classes instead (StreamAliasView, SafeStreamAliasView, and RedirectedAliasView). But also as noted on IRC, Robert is is going to overhaul the entire LibrarianFileAlias implementation.

The only other potential issue I see is in the test body: the test says it is "ensuring that restricted files ALWAYS have a disposition of 'attachement'", but this test only checks that the one file served has said disposition. Whether or not the incoming request had the disposition added by the code is a mystery. I would prefer a "before and after" comparison that shows the disposition being added.

With the above points considered, r=mars.

Maris

review: Approve (code)

[Abel Deuring (adeuring)]

Hi Māris,

Agreed, the word "always" is questionable in this context. But a comparison "before/after" is a test is a bit difficult, I think. OK, I could monkey patch the view_class attribute of BugAttachmentFileNavigation so that it is again StreamOrRedirectLibraryFileAliasView, but I think a better comment and an
assertIsInstance() is sufficient:

    def test_content_disposition_of_restricted_file(self):
        # The content of restricted Librarian files for bug attachments
        # is served by instances of SafeStreamOrRedirectLibraryFileAliasView
        # which set the content disposition header of the HTTP response for
        # 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)
        self.assertIsInstance(
            next_view, SafeStreamOrRedirectLibraryFileAliasView)
        next_view()
        self.assertEqual(
            'attachment', request.response.getHeader('Content-Disposition'))

[Robert Collins (lifeless)]

Abel, could you do a small follow on patch for me?

I'd like a little more prose/documentation in the docstrings for the
views so that developers can see when they should use one or the other
view class.

rs=lifeless for such a tweak.

Thanks,
Rob

[Abel Deuring (adeuring)]

On 04.08.2010 02:52, Robert Collins wrote:
> Abel, could you do a small follow on patch for me?
>
> I'd like a little more prose/documentation in the docstrings for the
> views so that developers can see when they should use one or the other
> view class.
>
> rs=lifeless for such a tweak.
>
> Thanks,
> Rob

Robert,

I'd like to postpone this to when PQM is closed ;) reasons:

- I need to land at least two more branches to fix the "private bug
  attachments" bug.
- ProxiedLFA, FileNavigationMixin, RedirectOrStream.*View could need a
  bit more testing (I've added some unit tests for private attachments
  that are in fact more like tests of FileNavigationMixin and
  StreamOrRedirectLibraryFileAliasView)
- StreamOrRedirectLibraryFileAliasView currently lives in
  lib.lp.bugs.browser.bugattachment, but it should move to
  canonical.launchpad.browser.librarian

Abel

[Robert Collins (lifeless)]

Sure, thats fine, its just polish we should do.

=== 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 15:23:48 +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 15:23:48 +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 15:23:48 +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 15:23:48 +0000
@@ -19,7 +19,8 @@
 from canonical.launchpad.webapp.publisher import RedirectionView
 from canonical.launchpad.webapp.servers import LaunchpadTestRequest
 
-from lp.bugs.browser.bugattachment import BugAttachmentFileNavigation
+from lp.bugs.browser.bugattachment import (
+    BugAttachmentFileNavigation, SafeStreamOrRedirectLibraryFileAliasView)
 from lp.testing import login_person, TestCaseWithFactory
 
 
@@ -69,7 +70,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 +88,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 +105,25 @@
         navigation = BugAttachmentFileNavigation(self.bugattachment, request)
         self.assertRaises(
             Unauthorized, navigation.publishTraverse, request, '+files')
+
+    def test_content_disposition_of_restricted_file(self):
+        # The content of restricted Librarian files for bug attachments
+        # is served by instances of SafeStreamOrRedirectLibraryFileAliasView
+        # which set the content disposition header of the HTTP response for
+        # 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)
+        self.assertIsInstance(
+            next_view, SafeStreamOrRedirectLibraryFileAliasView)
+        next_view()
+        self.assertEqual(
+            'attachment', request.response.getHeader('Content-Disposition'))