Opened 12 years ago

Last modified 4 years ago

#18150 new Bug

Uploading a file ending with a backslash fails

Reported by: Peter Kuma Owned by:
Component: File uploads/storage Version: 1.4
Severity: Normal Keywords:
Cc: nuno@…, supersteve9219 Triage Stage: Accepted
Has patch: yes Needs documentation: yes
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

When uploading a file, the filename as supplied in Content-Disposition filename parameter is sanitized by:

def IE_sanitize(self, filename):
    """Cleanup filename from Internet Explorer full paths."""
    return filename and filename[filename.rfind("\\")+1:].strip()

in multipartparser.py. If the filename contains a backslash, only the part following the backslash is retained. Because backslash is a valid character in unix file names, this behavior is not consistent with the expectations of a unix user.

More importantly, uploading a file ending with a backslash results in AttributeError. Consider the following section of MultiPartParser.parse():

file_name = disposition.get('filename')
if not file_name:
    continue
file_name = force_unicode(file_name, encoding, errors='replace')
file_name = self.IE_sanitize(unescape_entities(file_name))

If the filename parameter is empty, the file is ignored. However, if file_name is empty as a result of IE_sanitize stripping away the part before the last backslash character, the processing continues, and later fails with:

AttributeError
Exception Value: 'TemporaryFileUploadHandler' object has no attribute 'file'
Exception Location: /usr/lib/python2.6/site-packages/django/core/files/uploadhandler.py in file_complete, line 141

140.    def file_complete(self, file_size):
141.        self.file.seek(0)

One way of resolving this issue might be by making IE_sanitize less invasive, for example by making it effective only if filename begins with X:\.

Attachments (7)

ie san removed.diff (1.0 KB ) - added by supersteve9219 11 years ago.
tests.py (16.9 KB ) - added by supersteve9219 11 years ago.
updated patch.diff (1.6 KB ) - added by supersteve9219 11 years ago.
updated patch file
patch and tests.diff (3.6 KB ) - added by supersteve9219 11 years ago.
fixed indents.diff (3.6 KB ) - added by supersteve9219 11 years ago.
fix indents from last patch
patch_2_10_13.diff (3.6 KB ) - added by supersteve9219 11 years ago.
Newest patch, last patch contained out of date fixes.
patch_3_15_15.diff (3.7 KB ) - added by Vignesh Sarma K 9 years ago.
Should apply cleanly against master now

Download all attachments as: .zip

Change History (17)

comment:1 by Nuno Maltez <nuno@…>, 12 years ago

Cc: nuno@… added
Triage Stage: UnreviewedAccepted

comment:2 by supersteve9219, 11 years ago

Cc: supersteve9219 added
Owner: changed from nobody to supersteve9219
Status: newassigned

by supersteve9219, 11 years ago

Attachment: ie san removed.diff added

comment:3 by supersteve9219, 11 years ago

Has patch: set

I added a patch.

Given IE_sanitize is only used with files uploaded from Internet Explorer 7 and earlier which represents less than 3% of the total browser share, I don't think it is no longer necessary to use this method. Especially given that using IE_sanitize causes issues with all unix users.

While it is an inconvenience to have files with the entire full path IE 7 provides I think its easier to deal with that slight inconvenience than to use IE_sanitize which introduces issues with unix users.

Using IE_sanitize only on files starting with something like X:\ does not fix the issue since : is still a valid unix filename char.

Last edited 11 years ago by supersteve9219 (previous) (diff)

by supersteve9219, 11 years ago

Attachment: tests.py added

by supersteve9219, 11 years ago

Attachment: updated patch.diff added

updated patch file

comment:4 by supersteve9219, 11 years ago

I added an updated patch. The file name was also being sanitized in django/core/files/uploadedfile.py.

I added an if statement that checks if the last character in a file name is "\", if it does not end in "\" normal sanitation continues, however, if it does end in "\" it replaces the "\" with "0", we could simply strip the "\" from the end of the file name but if there is multiple backslashes at the end or the file name is a single backslash it could result in a empty string for the file name.

I also attached django\tests\regressiontests\file_uploads\test.py with a test method I added to test for this bug, it fails with the same error mentioned in the bug report, but passes without issue after the patch is applied.

Here is the test method alone:

    def test_fail_backslash(self):
        """Tests filename ending with a backslash, issue #18150 reports crashes when a filename ends with a backslash"""
        backSlashName = "backslash.jpg\\"
        payload = client.FakePayload()
        payload.write('\r\n'.join([
            '--' + client.BOUNDARY,
            'Content-Disposition: form-data; name="file1"; filename="%s"' % backSlashName,
            'Content-Type: application/octet-stream',
            '',
            ''
        ]))
        payload.write('\r\n--' + client.BOUNDARY + '--\r\n')

        r = {
            'CONTENT_LENGTH': len(payload),
            'CONTENT_TYPE':   client.MULTIPART_CONTENT,
            'PATH_INFO':      "/file_uploads/echo/",
            'REQUEST_METHOD': 'POST',
            'wsgi.input':     payload,
        }
        response = self.client.request(**r)
        self.assertEqual(response.status_code, 200)

comment:5 by anonymous, 11 years ago

Added a new patch, added the test method to the patch.

Changed os.path to ntpath in uploadedfile.py, os.path has inconsistent behavior depending on operating system. In this case we need os.path to handle filenames with both forward and back slashes, on unix operating systems os.path will not remove backslashes, by forcing ntpath we can avoid this. Since forward and backslashes are now handled in uploadedfile.py we can remove it from multipartparser.py since it is redundant.

in reply to:  5 comment:6 by supersteve9219, 11 years ago

Replying to anonymous:

Added a new patch, added the test method to the patch.

Changed os.path to ntpath in uploadedfile.py, os.path has inconsistent behavior depending on operating system. In this case we need os.path to handle filenames with both forward and back slashes, on unix operating systems os.path will not remove backslashes, by forcing ntpath we can avoid this. Since forward and backslashes are now handled in uploadedfile.py we can remove it from multipartparser.py since it is redundant.

Accidentally posted this while logged out.

by supersteve9219, 11 years ago

Attachment: patch and tests.diff added

by supersteve9219, 11 years ago

Attachment: fixed indents.diff added

fix indents from last patch

by supersteve9219, 11 years ago

Attachment: patch_2_10_13.diff added

Newest patch, last patch contained out of date fixes.

comment:7 by Tim Graham, 10 years ago

Needs documentation: set
Patch needs improvement: set

Patch no longer applies cleanly. Also, if we drop IE_santize, we need to document the consequences of that in the release notes.

by Vignesh Sarma K, 9 years ago

Attachment: patch_3_15_15.diff added

Should apply cleanly against master now

comment:8 by Vignesh Sarma K, 9 years ago

We are facing this issue.

I have updated the patch and should apply cleanly against master.

If there is anything more that needs to be done to get this patch into the next bugfix release I can help with that.

comment:9 by Tim Graham, 7 years ago

Owner: supersteve9219 removed
Status: assignednew

When updating a patch (please send a pull request these days), you also need update the ticket flags (uncheck "Needs documentation" and "Patch needs improvement") so that the ticket appears in the patch review queue.

comment:10 by Mariusz Felisiak, 4 years ago

Filenames with double trailing backslashes are ignored after 4b129ac81f4fa38004950d0b307f81d1e9b44af8, but the ticket is still valid because they should be accepted as a valid filenames on Linux.

Note: See TracTickets for help on using tickets.
Back to Top