bottlepy / bottle

bottle.py is a fast and simple micro-framework for python web-applications.
http://bottlepy.org/
MIT License
8.38k stars 1.46k forks source link

Lowercasing in FileUpload.filename #582

Open p opened 10 years ago

p commented 10 years ago

pycurl tests broke on travis recently (https://travis-ci.org/pycurl/pycurl/builds/18246021). I tracked the breakage down to b18a938b220a7a8120a5e2aebc5745b which makes filename attribute of file uploads be all lowercase. I reproduced the breakage on my debian jessie system as well. I'm guessing travis/pypi just upgraded from bottle 0.11 to 0.12.

While I agree with unicode normalization and path separator removal as security features, benefits of case conversion are not as clear. Suppose I rely on the fact that bottle lowercases file names and don't lowercase them in my application, and subsequently assume that names of files on the filesystem are all lowercased. What if a user renamed some files outside of my application to have mixed case? Now my application would have a potential security hole. If I cared for security I would perform case insensitive comparisons (everywhere) rather than relying on file names being in a particular case anywhere, be that file system or library output.

If I was on unix, I would want to receive files in the correct case. If I was writing an application where file names were meaningful, I would have to forego using filename entirely and instead perform my own normalization and path separator etc. filtering, i.e., by copy pasting filename implementation sans the lowercasing.

Finally lowercasing is a behavior change from 0.11 to 0.12. Despite being in the tree for a year apparently this change is only now trickling to production systems. Of course, changing code back to not lowercase is a behavior change as well.

I hope that you consider leaving the case alone in filename.

defnull commented 10 years ago

tl;dr: You are right.

In the end it depends on the expected behavior and what we call a behavior change or a bug fix.

Unsafe characters in filenames are a risk and I cannot imagine a scenario where you would need them to be present. Filtering them out should not break any reasonable application code. Perhaps some safeguards on application side are now useless but they should still work as expected.

Lowercasing the filename, on the other side, is not justified by security considerations. On case sensitive file systems it is perfectly fine to have two files with the same name but different capitalization. On case insensitive file systems you get errors from the os library and os.path.exists() warns you about duplicates as it should. If you need to copy from one file system to the other, you have to handle that anyway. Using the user provided file name as some kind of unique key is a bad idea, too. So, yes, you are right, doing the lowercase-stuff on filenames in bottle was a bad idea.

And because 0.12 is very young (released a couple of days ago) and we always promised backwards compatibility, I would say that this unexpected behavior change is actually a bug and should be fixed. It does less harm to fix it now than to carry that burden two more releases into the future.

I hope anyone that built an application in the last two days that depends on lowercase filenames is OK with this ;)

p commented 10 years ago

Great, thanks!