[Django] #18150: Uploading a file ending with a backslash fails
Django
--------------------------------------+--------------------
Reporter: Peter Kuma | Owner: nobody
Type: Bug | Status: new
Component: File uploads/storage | Version: 1.4
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------------+--------------------
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`[[BR]]
Exception Value: '`TemporaryFileUploadHandler`' object has no attribute
'file'[[BR]]
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:\`.
--
Ticket URL: <https://code.djangoproject.com/ticket/18150>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Type: Bug | Status: new
Component: File uploads/storage | Version: 1.4
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Changes (by Nuno Maltez <nuno@…>):
* cc: nuno@… (added)
* needs_better_patch: => 0
* needs_docs: => 0
* needs_tests: => 0
* stage: Unreviewed => Accepted
--
Ticket URL: <https://code.djangoproject.com/ticket/18150#comment:1>
Django
Reporter: Peter Kuma | Owner:
Type: Bug | supersteve9219
Component: File | Status: assigned
uploads/storage | Version: 1.4
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by supersteve9219):
* cc: supersteve9219 (added)
* owner: nobody => supersteve9219
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/18150#comment:2>
Django
Reporter: Peter Kuma | Owner:
Type: Bug | supersteve9219
Component: File | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by supersteve9219):
* has_patch: 0 => 1
Comment:
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.
Using IE_sanitize only on files starting with something like {{{X:\}}}
does not fix the issue since {{{:}}} is still a valid unix filename char.
--
Ticket URL: <https://code.djangoproject.com/ticket/18150#comment:3>
Django
Reporter: Peter Kuma | Owner:
Type: Bug | supersteve9219
Component: File | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by supersteve9219):
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)
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/18150#comment:4>
Django
Reporter: Peter Kuma | Owner:
Type: Bug | supersteve9219
Component: File | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/18150#comment:5>
Django
Reporter: Peter Kuma | Owner:
Type: Bug | supersteve9219
Component: File | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by supersteve9219):
Replying to [comment:5 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/18150#comment:6>
Django
Reporter: Peter Kuma | Owner:
Type: Bug | supersteve9219
Component: File | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by timo):
* needs_better_patch: 0 => 1
* needs_docs: 0 => 1
Comment:
Patch no longer applies cleanly. Also, if we drop `IE_santize`, we need to
document the consequences of that in the release notes.
--
Ticket URL: <https://code.djangoproject.com/ticket/18150#comment:7>
Django
Reporter: Peter Kuma | Owner:
Type: Bug | Status: assigned
Component: File | Version: 1.4
uploads/storage |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Needs tests: 0 | Patch needs improvement: 1
Changes (by vigneshsarma):
* Attachment "patch_3_15_15.diff" added.
Should apply cleanly against master now
Django
Reporter: Peter Kuma | Owner:
| supersteve9219
Type: Bug | Status: assigned
Component: File | Version: 1.4
uploads/storage |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Needs tests: 0 | Patch needs improvement: 1
Comment (by vigneshsarma):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/18150#comment:8>
Django
Reporter: Peter Kuma | Owner: (none)
Type: Bug | Status: new
Component: File uploads/storage | Version: 1.4
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Needs tests: 0 | Patch needs improvement: 1
Changes (by Tim Graham):
* owner: supersteve9219 => (none)
* status: assigned => new
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/18150#comment:9>
Django
Reporter: Peter Kuma | Owner: (none)
Component: File uploads/storage | Version: 1.4
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Needs tests: 0 | Patch needs improvement: 1
Comment (by felixxm):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/18150#comment:10>