Launchpad itself

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

  1. bug-612779
  2. Merge into db-devel
Proposed by Abel Deuring
Status: Merged
Approved by: Māris Fogels
Approved revision: no longer in the source branch.
Merged at revision: 9612
Proposed branch: lp:~adeuring/launchpad/bug-612779
Merge into: lp:launchpad/db-devel
Diff against target: 157 lines (+57/-9)
4 files modified
lib/canonical/launchpad/browser/librarian.py (+6/-5)
lib/lp/bugs/browser/bugattachment.py (+20/-1)
lib/lp/bugs/browser/configure.zcml (+5/-0)
lib/lp/bugs/browser/tests/test_bugattachment_file_access.py (+26/-3)
To merge this branch: bzr merge lp:~adeuring/launchpad/bug-612779
Reviewer Review Type Date Requested Status
Māris Fogels (community) code Approve
Review via email: mp+31637@code.launchpad.net

Description of the change

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

To post a comment you must log in.
Revision history for this message
Māris Fogels (mars) wrote :

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)
Revision history for this message
Abel Deuring (adeuring) wrote :

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'))

Revision history for this message
Robert Collins (lifeless) 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

Revision history for this message
Abel Deuring (adeuring) wrote :

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

Revision history for this message
Robert Collins (lifeless) wrote :

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

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/canonical/launchpad/browser/librarian.py'
2--- lib/canonical/launchpad/browser/librarian.py 2010-03-26 19:19:49 +0000
3+++ lib/canonical/launchpad/browser/librarian.py 2010-08-03 15:23:48 +0000
4@@ -42,8 +42,8 @@
5 """View to handle redirection for downloading files by URL.
6
7 Rather than reference downloadable files via the obscure Librarian
8- URL, downloadable files can be referenced via the Product Release URL, e.g.
9- http://launchpad.net/firefox/1.0./1.0.0/+download/firefox-1.0.0.tgz.
10+ URL, downloadable files can be referenced via the Product Release URL,
11+ e.g. http://launchpad.net/firefox/1.0./1.0.0/+download/firefox-1.0.0.tgz.
12 """
13
14 __used_for__ = ILibraryFileAlias
15@@ -173,19 +173,20 @@
16 extended in order to allow traversing to multiple files potentially
17 with the same filename (product files or bug attachments).
18 """
19+ view_class = StreamOrRedirectLibraryFileAliasView
20+
21 @stepthrough('+files')
22 def traverse_files(self, filename):
23 """Traverse on filename in the archive domain."""
24 if not check_permission('launchpad.View', self.context):
25 raise Unauthorized()
26- library_file = self.context.getFileByName(filename)
27+ library_file = self.context.getFileByName(filename)
28
29 # Deleted library files result in NotFound-like error.
30 if library_file.deleted:
31 raise DeletedProxiedLibraryFileAlias(filename, self.context)
32
33- return StreamOrRedirectLibraryFileAliasView(
34- library_file, self.request)
35+ return self.view_class(library_file, self.request)
36
37
38 class ProxiedLibraryFileAlias:
39
40=== modified file 'lib/lp/bugs/browser/bugattachment.py'
41--- lib/lp/bugs/browser/bugattachment.py 2010-07-27 14:12:47 +0000
42+++ lib/lp/bugs/browser/bugattachment.py 2010-08-03 15:23:48 +0000
43@@ -10,6 +10,7 @@
44 'BugAttachmentSetNavigation',
45 'BugAttachmentEditView',
46 'BugAttachmentURL',
47+ 'SafeStreamOrRedirectLibraryFileAliasView',
48 ]
49
50 from cStringIO import StringIO
51@@ -18,7 +19,8 @@
52 from zope.component import getUtility
53 from zope.contenttype import guess_content_type
54
55-from canonical.launchpad.browser.librarian import FileNavigationMixin
56+from canonical.launchpad.browser.librarian import (
57+ FileNavigationMixin, StreamOrRedirectLibraryFileAliasView)
58 from canonical.launchpad.webapp import (
59 canonical_url, custom_widget, GetitemNavigation, Navigation)
60 from canonical.launchpad.interfaces.librarian import ILibraryFileAliasSet
61@@ -227,7 +229,24 @@
62 return self.context.type == BugAttachmentType.PATCH
63
64
65+class SafeStreamOrRedirectLibraryFileAliasView(
66+ StreamOrRedirectLibraryFileAliasView):
67+ """A view for Librarian files that sets the content disposion header."""
68+
69+ def __call__(self):
70+ """Stream the content of the context `ILibraryFileAlias`.
71+
72+ Set the content disposition header to the safe value "attachment".
73+ """
74+ self.request.response.setHeader(
75+ 'Content-Disposition', 'attachment')
76+ return super(
77+ SafeStreamOrRedirectLibraryFileAliasView, self).__call__()
78+
79+
80 class BugAttachmentFileNavigation(Navigation, FileNavigationMixin):
81 """Traversal to +files/${filename}."""
82
83 usedfor = IBugAttachment
84+
85+ view_class = SafeStreamOrRedirectLibraryFileAliasView
86
87=== modified file 'lib/lp/bugs/browser/configure.zcml'
88--- lib/lp/bugs/browser/configure.zcml 2010-07-27 14:12:47 +0000
89+++ lib/lp/bugs/browser/configure.zcml 2010-08-03 15:23:48 +0000
90@@ -1159,4 +1159,9 @@
91 classes="
92 BugWatchSetNavigation"/>
93 </facet>
94+ <class class="lp.bugs.browser.bugattachment.SafeStreamOrRedirectLibraryFileAliasView">
95+ <allow attributes="__call__" />
96+ <allow interface="zope.publisher.interfaces.browser.IBrowserPublisher" />
97+ </class>
98+
99 </configure>
100
101=== modified file 'lib/lp/bugs/browser/tests/test_bugattachment_file_access.py'
102--- lib/lp/bugs/browser/tests/test_bugattachment_file_access.py 2010-08-02 10:52:39 +0000
103+++ lib/lp/bugs/browser/tests/test_bugattachment_file_access.py 2010-08-03 15:23:48 +0000
104@@ -19,7 +19,8 @@
105 from canonical.launchpad.webapp.publisher import RedirectionView
106 from canonical.launchpad.webapp.servers import LaunchpadTestRequest
107
108-from lp.bugs.browser.bugattachment import BugAttachmentFileNavigation
109+from lp.bugs.browser.bugattachment import (
110+ BugAttachmentFileNavigation, SafeStreamOrRedirectLibraryFileAliasView)
111 from lp.testing import login_person, TestCaseWithFactory
112
113
114@@ -69,7 +70,7 @@
115 '^http://localhost:58000/\d+/foo.txt$', next_view.target)
116 self.assertIsNot(None, mo)
117
118- def test_access_to_restircted_file(self):
119+ def test_access_to_restricted_file(self):
120 # Requests of restricted files are handled by ProxiedLibraryFileAlias.
121 lfa_with_parent = getMultiAdapter(
122 (self.bugattachment.libraryfile, self.bugattachment),
123@@ -87,7 +88,7 @@
124 file_.seek(0)
125 self.assertEqual('file content', file_.read())
126
127- def test_access_to_restircted_file_unauthorized(self):
128+ def test_access_to_restricted_file_unauthorized(self):
129 # If a user cannot access the bug attachment itself, he can neither
130 # access the restricted Librarian file.
131 lfa_with_parent = getMultiAdapter(
132@@ -104,3 +105,25 @@
133 navigation = BugAttachmentFileNavigation(self.bugattachment, request)
134 self.assertRaises(
135 Unauthorized, navigation.publishTraverse, request, '+files')
136+
137+ def test_content_disposition_of_restricted_file(self):
138+ # The content of restricted Librarian files for bug attachments
139+ # is served by instances of SafeStreamOrRedirectLibraryFileAliasView
140+ # which set the content disposition header of the HTTP response for
141+ # to "attachment".
142+ lfa_with_parent = getMultiAdapter(
143+ (self.bugattachment.libraryfile, self.bugattachment),
144+ ILibraryFileAliasWithParent)
145+ lfa_with_parent.restricted = True
146+ self.bug.setPrivate(True, self.bug_owner)
147+ transaction.commit()
148+ request = LaunchpadTestRequest()
149+ request.setTraversalStack(['foo.txt'])
150+ navigation = BugAttachmentFileNavigation(self.bugattachment, request)
151+ view = navigation.publishTraverse(request, '+files')
152+ next_view, traversal_path = view.browserDefault(request)
153+ self.assertIsInstance(
154+ next_view, SafeStreamOrRedirectLibraryFileAliasView)
155+ next_view()
156+ self.assertEqual(
157+ 'attachment', request.response.getHeader('Content-Disposition'))

Subscribers

People subscribed via source and target branches

to status/vote changes: