Merge lp:~adeuring/launchpad/bug-612779 into lp:launchpad/db-devel
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 |
Related bugs: |
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-
The branch defines a new class SafeStreamOrRed
Instances of StreamOrRedirec
Finally, I set the view class used in BugAttachmentFi
test: ./bin/test -vvt test_bugattachm
no lint
Hi Abel,
As noted on IRC, I think the class name StreamOrRedirec tLibraryFileAli asView indicates a serious object design problem: it sounds like you should have three polymorphic classes instead (StreamAliasView, SafeStreamAlias View, and RedirectedAlias View). 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