launchpad-reviewers team mailing list archive

["j.c.sackett" <jcsackett@xxxxxxxxxxxxx>]

j.c.sackett has proposed merging lp:~jcsackett/launchpad/bug-attachments-with-questionmark into lp:launchpad.

Commit message:
Strips ?'s out of attachment filenames so the files do not become untraversable.

Requested reviews:
  Curtis Hovey (sinzui)
Related bugs:
  Bug #983766 in Launchpad itself: "Error when uploading screenshot incl. a question mark in url"
  https://bugs.launchpad.net/launchpad/+bug/983766

For more details, see:
https://code.launchpad.net/~jcsackett/launchpad/bug-attachments-with-questionmark/+merge/128121

Summary
=======
Attachments in bug comments are untraversable if the filename contains a
questionmark, because a mismatch in handling results in the bugattachment name
being misparsed.

Investigation with Curtis Hovey indicates those files aren't going to become
reachable, but we can prevent the problem from occuring again, by simply
stripping the ? from filenames.

That investigation also indicated bug 174794 could be solved at this time; in
the attempt to create a regex to strip the NT style filepaths from attachment
names, it was discovered that at some point that bug appears to have been
fixed--the path was already being stripped. This can be observed by uploading
a file named something like "C:\Foo\Bar\baz\filename.png" -- the result will
be "filename.png". I'll investigate further to ensure that's fixed and close
the bug if so.

Preimp
======
Spoke with Curtis Hovey.

Implementation
==============
A new utility function is created to clean up filenames for attachments. It is
used in both browser.bugmessage and browser.bugtarget to clean up attachments.
The previous function of simply replacing '/' with '-' is added to this
utility along with stripping questionmarks.

Tests are also added.

Tests
=====
bin/test -vvct TestBugAttachmentSanitization

QA
==
Upload a filename with a ? in it. The ? should be stripped and the file should
be traversable.

LoC
===
I have an LoC credit of more than 400 lines since going on maintenance.

Lint
====

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/bugs/browser/bugmessage.py
  lib/lp/bugs/browser/bugtarget.py
  lib/lp/bugs/model/bugattachment.py
-- 
https://code.launchpad.net/~jcsackett/launchpad/bug-attachments-with-questionmark/+merge/128121
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.

=== modified file 'lib/lp/bugs/browser/bugmessage.py'
--- lib/lp/bugs/browser/bugmessage.py	2012-01-01 02:58:52 +0000
+++ lib/lp/bugs/browser/bugmessage.py	2012-10-04 21:46:23 +0000
@@ -19,6 +19,7 @@
 from lp.bugs.browser.bugattachment import BugAttachmentContentCheck
 from lp.bugs.interfaces.bugmessage import IBugMessageAddForm
 from lp.bugs.interfaces.bugwatch import IBugWatchSet
+from lp.bugs.model.bugattachment import sanitize_attachment_name
 from lp.services.webapp import canonical_url
 
 
@@ -95,9 +96,8 @@
         self.next_url = canonical_url(self.context)
         if file_:
 
-            # Slashes in filenames cause problems, convert them to dashes
-            # instead.
-            filename = file_.filename.replace('/', '-')
+            # Sanitize the filename
+            filename = sanitize_attachment_name(file_.filename)
 
             # if no description was given use the converted filename
             file_description = None

=== modified file 'lib/lp/bugs/browser/bugtarget.py'
--- lib/lp/bugs/browser/bugtarget.py	2012-10-03 18:06:22 +0000
+++ lib/lp/bugs/browser/bugtarget.py	2012-10-04 21:46:23 +0000
@@ -113,6 +113,7 @@
     )
 from lp.bugs.interfaces.bugtracker import IBugTracker
 from lp.bugs.model.bugtask import BugTask
+from lp.bugs.model.bugattachment import sanitize_attachment_name
 from lp.bugs.model.structuralsubscription import (
     get_structural_subscriptions_for_target,
     )
@@ -584,9 +585,8 @@
 
             # Deal with attachments added in the filebug form.
             if attachment:
-                # We convert slashes in filenames to hyphens to avoid
-                # problems.
-                filename = attachment.filename.replace('/', '-')
+                # Remove or replace several bad characters
+                filename = sanitize_attachment_name(attachment.filename)
 
                 # If the user hasn't entered a description for the
                 # attachment we use its name.

=== modified file 'lib/lp/bugs/model/bugattachment.py'
--- lib/lp/bugs/model/bugattachment.py	2011-12-30 06:14:56 +0000
+++ lib/lp/bugs/model/bugattachment.py	2012-10-04 21:46:23 +0000
@@ -4,7 +4,11 @@
 # pylint: disable-msg=E0611,W0212
 
 __metaclass__ = type
-__all__ = ['BugAttachment', 'BugAttachmentSet']
+__all__ = [
+    'BugAttachment',
+    'BugAttachmentSet',
+    'sanitize_attachment_name',
+    ]
 
 from lazr.lifecycle.event import (
     ObjectCreatedEvent,
@@ -118,3 +122,7 @@
         if send_notifications:
             notify(ObjectCreatedEvent(attachment, user=message.owner))
         return attachment
+
+
+def sanitize_attachment_name(name):
+    return name.replace('/', '-').replace('?', '')