launchpad-reviewers team mailing list archive

[Steve Kowalik <stevenk@xxxxxxxxxx>]

Steve Kowalik has proposed merging lp:~stevenk/launchpad/proper-error-on-bug-attachment-bad-filename into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1021611 in Launchpad itself: "IntegrityError adding attachment through the API when file name contains slash"
  https://bugs.launchpad.net/launchpad/+bug/1021611

For more details, see:
https://code.launchpad.net/~stevenk/launchpad/proper-error-on-bug-attachment-bad-filename/+merge/122608

If adding the LFA in IBug.addAttachment raises an IntegrityError, catch it and raise it as BadRequest.
-- 
https://code.launchpad.net/~stevenk/launchpad/proper-error-on-bug-attachment-bad-filename/+merge/122608
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~stevenk/launchpad/proper-error-on-bug-attachment-bad-filename into lp:launchpad.

=== modified file 'lib/lp/bugs/model/bug.py'
--- lib/lp/bugs/model/bug.py	2012-08-30 00:09:11 +0000
+++ lib/lp/bugs/model/bug.py	2012-09-04 02:58:19 +0000
@@ -31,6 +31,7 @@
     ObjectModifiedEvent,
     )
 from lazr.lifecycle.snapshot import Snapshot
+from lazr.restfulclient.errors import BadRequest
 import pytz
 from sqlobject import (
     BoolCol,
@@ -41,6 +42,7 @@
     SQLRelatedJoin,
     StringCol,
     )
+from storm.exceptions import IntegrityError
 from storm.expr import (
     And,
     Desc,
@@ -1250,10 +1252,13 @@
                 content_type, encoding = guess_content_type(
                     name=filename, body=filecontent)
 
-        filealias = getUtility(ILibraryFileAliasSet).create(
-            name=filename, size=len(filecontent),
-            file=StringIO(filecontent), contentType=content_type,
-            restricted=self.private)
+        try:
+            filealias = getUtility(ILibraryFileAliasSet).create(
+                name=filename, size=len(filecontent),
+                file=StringIO(filecontent), contentType=content_type,
+                restricted=self.private)
+        except IntegrityError:
+            raise BadRequest('400', "Invalid filename.")
 
         return self.linkAttachment(
             owner, filealias, comment, is_patch, description)

=== modified file 'lib/lp/bugs/tests/test_bugs_webservice.py'
--- lib/lp/bugs/tests/test_bugs_webservice.py	2012-08-30 06:18:38 +0000
+++ lib/lp/bugs/tests/test_bugs_webservice.py	2012-09-04 02:58:19 +0000
@@ -19,9 +19,7 @@
     Equals,
     LessThan,
     )
-from zope.component import (
-    getMultiAdapter,
-    )
+from zope.component import getMultiAdapter
 
 from lp.bugs.browser.bugtask import get_comments_for_bugtask
 from lp.bugs.interfaces.bug import IBug
@@ -344,7 +342,7 @@
 
 class TestErrorHandling(TestCaseWithFactory):
 
-    layer = DatabaseFunctionalLayer
+    layer = LaunchpadFunctionalLayer
 
     def test_add_duplicate_bugtask_for_project_gives_bad_request(self):
         bug = self.factory.makeBug()
@@ -375,6 +373,18 @@
         self.assertRaises(
             BadRequest, lp_bug.addTask, target=api_url(product2))
 
+    def test_add_attachment_with_bad_filename_gives_bad_request(self):
+        # Test that addAttachment raises BadRequest when the filename given
+        # contains slashes.
+        owner = self.factory.makePerson()
+        bug = self.factory.makeBug(owner=owner)
+        login_person(owner)
+        launchpad = launchpadlib_for('test', owner)
+        lp_bug = launchpad.load(api_url(bug))
+        self.assertRaises(
+            BadRequest, lp_bug.addAttachment, comment='foo', data='foo',
+            filename='/home/foo/bar.txt')
+
 
 class BugSetTestCase(TestCaseWithFactory):