[issue9968] Let cgi.FieldStorage have named uploaded file

[Rhodri James]

Change by Rhodri James :


--
nosy:  -Rhodri James

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue9968] Let cgi.FieldStorage have named uploaded file

[Rhodri James]

Change by Rhodri James :


--
nosy: +Rhodri James, ethan.furman

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue9968] Let cgi.FieldStorage have named uploaded file

[Éric Araujo]

Éric Araujo mer...@netwok.org added the comment:

I read somewhere that the fact that TemporaryFile does not expose its name is a 
security feature :/

 On this topic, I was wondering if the changes I propose have any chance of 
 landing some day in 2.7 land - dunno Python workflow precisely enough.
Only bug fixes and doc fixes go into 2.7.  This report is about a new feature, 
which has to target 3.3.

--
title: cgi.FieldStorage: Give control about the directory used for uploads - 
Let cgi.FieldStorage have named uploaded file

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue9968
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue9968] Let cgi.FieldStorage have named uploaded file

[phep]

phep patrice.pil...@teletopie.net added the comment:

These are the changeset details:
changeset:   18337:c2a60de91d2c
branch:  legacy-trunk
user:Guido van Rossum gu...@python.org
date:Fri Jun 29 13:06:06 2001 +
summary: Solve SF bug #231249: cgi.py opens too many (temporary) files.


You're right that we might use environment variables or tempfile.tempdir
to attain the same goal but this would impact _all_ of the code executed
during request data parsing given the monolithic construction of
FieldStorage. This implies that the context of every call to tempfile
members would be impacted during this process.  Presently this is not a
problem at all, but this looks fragile for future developments. 

On the other hand:
1) this has the advantage of not changing FieldStorage interface,
2) this alleviates us of wondering if passing to FieldStorage
constructor all of the arguments to NamedTemporaryFile is a possibility
worth considering ;-).

After pondering this for a while I think the simpler is the better and I
propose to add documentation to inform the reader that changing the
temp directory through os.environ of tempfile.tempdir might worth
consideration.

As for other use cases for changing the temp directory, I thought about
letting the user choose the FS of its choice with regard to
performance or security (crypted FS) or even having the temp files created in a 
directory with 700 permissions. 

Last, you're perfectly right about the None argument.

I fiddled last night with setting an environment to deploy and test a patched 
Python (I had some problem to understand what happened when I encountered 
6755). This now works and the patch does not introduced any regression. I still 
have to add unit tests (I only tested with my embryonic cgi script) and update 
the documentation before to send the patch. I should be able to do that in a 
few days at most.

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue9968
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue9968] Let cgi.FieldStorage have named uploaded file

[phep]

phep patrice.pil...@teletopie.net added the comment:

So, this is the patch.

--
keywords: +patch
Added file: http://bugs.python.org/file22796/fix9968.patch

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue9968
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue9968] Let cgi.FieldStorage have named uploaded file

[Éric Araujo]

Éric Araujo mer...@netwok.org added the comment:

 where does the 1ko barrier come from? Was it only chosen out of 
 performance considerations [...]
 Most certainly.  I’ll look at the history of the file later to try to
 find the developer who decided that.
 Guido van Rossum made the changes. Before that a temporary file was
 created for every form field.

Do you have the changeset ID?

 A last question [...]

For a new feature, it’s okay to change signatures.  Backward compatibility is 
preserved by appending the new argument at the end of the arguments lists, and 
making it optional.

Given that tempfile respects the TMP and TMPDIR environment variables, do you 
think it would be possible for users to control the download dir for uploaded 
files by editing os.environ?  This would require no change to cgi.  If that’s 
not possible, then we’ll have to change the FieldStorage class.

 Also, I believe one can think of other reasons to give this freedom.

Can you think about some of them?

def __init__(self, tempdir=tempfile.gettempdir()):
 or a somewhat more convoluted form that would avoid importing
 tempfile needlessly.

You could probably have a None argument that would be passed down to tempfile.

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue9968
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue9968] Let cgi.FieldStorage have named uploaded file

[Éric Araujo]

Éric Araujo mer...@netwok.org added the comment:

 where does the 1ko barrier come from? Was it only chosen out of
 performance considerations [...]

Most certainly.  I’ll look at the history of the file later to try to find the 
developer who decided that.

 tempfile.NamedTemporaryFile was already used in python 3 (fixed in
 r57595, with not many explanations though).

That’s a TemporaryFile, without a name.  For more explanations, follow the 
parents of the changeset and you’ll find aee21e0c9a70 (referencing #1033) and 
cbd50ece3b61, where you can see an XXX note that is probably the “wish” 
referred to in the cryptic commit message.

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue9968
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue9968] Let cgi.FieldStorage have named uploaded file

[phep]

phep patrice.pil...@teletopie.net added the comment:

 where does the 1ko barrier come from? Was it only chosen out of 
 performance considerations [...]
 
 Most certainly.  I’ll look at the history of the file later to try to
 find the developer who decided that.

Guido van Rossum made the changes. Before that a temporary file was created for 
every form field.

 tempfile.NamedTemporaryFile was already used in python 3 (fixed in 
 r57595, with not many explanations though).
 
 That’s a TemporaryFile, without a name.  For more explanations, follow

Yes, my sleepy eyes and drowsy brain fooled me more than once last
night. Sigh...

 the parents of the changeset and you’ll find aee21e0c9a70 
 (referencing #1033) and cbd50ece3b61, where you can see an XXX
 note that is probably the “wish” referred to in the cryptic commit
 message.

Thanks.

A last question before trying to write the patch: In order for the change I 
propose to be interesting, one should let caller code decide where (on disk) 
the NamedTemporaryFile should be created since writing this on a different 
partition than the one the file will eventually reside on would ruin the whole 
trick. Also, I believe one can think of other reasons to give this freedom.

Unfortunately, in order to do that, I can see no other solution than to change 
the FieldStorage constructor signature since the temp files are created as soon 
as the object is instantiated. So, we would end up with something along those 
lines:

class FieldStorage(cgi.FieldStorage):
def __init__(self, tempdir=tempfile.gettempdir()):
self.tempdir = tempdir
...

or a somewhat more convoluted form that would avoid importing tempfile 
needlessly.

Do you think this would be acceptable ?

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue9968
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue9968] Let cgi.FieldStorage have named uploaded file

[phep]

phep patrice.pil...@teletopie.net added the comment:

Hi,

This was test code :-/. It was not under revision control and 
unfortunately, as I feared then, it turned out I had to stop
working shortly after my last message on the low-priority
project that made me report this issue .

Yet, I have a tarball of a presumably-not-so-different version
of the code and I experimented with it this afternoon. Actually,
I could not reproduce the problem I reported in msg117933. So, 
basically, the idea of replacing tempfile.TemporaryFile() by
tempfile.NamedTemporaryFile() seems to really be valid, whatever
the position of the form field.

Yet I found another problem, quite reproducible that one, that
seems to have some fame on Google. Due to the way cgi.__write()
is written, contents of uploaded files with small sizes will be
stored as StringIO. Consequently, sort-of calling
fieldstorage['filenamefield'].file.name will fail. De facto,
this may actually be the error I alluded to in msg117933 - I 
honestly cannot remember what I observed then: « c'est
l'âge...».

So the patch should then be one line longer: we would need
to change __write() to add a check on the value of 'filename' :
-   if self.__file.tell() + len(line)  1000:
+   if self.filename or self.__file.tell() + len(line)  1000:

This way, long textareas (for example), keep being stored on
disk if this has any importance.

There is yet one question I cannot answer: where does the 1ko 
barrier come from? Was it only chosen out of performance
considerations, or was there another reason? As I am quite new 
to Python I fear I may miss something obvious.

HTH

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue9968
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue9968] Let cgi.FieldStorage have named uploaded file

[phep]

phep patrice.pil...@teletopie.net added the comment:

Oh, my...

As we are working with python2.6 on our servers, I overlooked the fact that 
tempfile.NamedTemporaryFile was already used in python 3 (fixed in r57595, with 
not many explanations though).

Yet, the problem with short length files (cf. msg141179) is still valid and the 
documentation has not yet been updated.

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue9968
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue9968] Let cgi.FieldStorage have named uploaded file

[phep]

phep patrice.pil...@teletopie.net added the comment:

I got my head in a brown paper bag 

This is obviously not fixed... except in a locally edited checkout :-(

Please be kind to the tired me, forget my last message and don't talk about 
this to my boss, will you ?

It's closing time, really.

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue9968
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue9968] Let cgi.FieldStorage have named uploaded file

[Éric Araujo]

Éric Araujo mer...@netwok.org added the comment:

 Well, this is actually somewhat more complicated than what my first
 tests showed due to the way multipart/form-data is dealt with in
 FieldStorage.read_multi().

 The solution I proposed last time only works if the uploaded file is
 passed as the first form field.

Can you copy here the tests you did and the error output you got?

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue9968
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue9968] Let cgi.FieldStorage have named uploaded file

[Éric Araujo]

Changes by Éric Araujo mer...@netwok.org:


--
versions: +Python 3.3 -Python 3.2

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue9968
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue9968] Let cgi.FieldStorage have named uploaded file

[phep]

phep patrice.pil...@teletopie.net added the comment:

Well, this is actually somewhat more complicated than what my first tests 
showed due to the way multipart/form-data is dealt with in 
FieldStorage.read_multi().

The solution I proposed last time only works if the uploaded file is passed as 
the first form field.

I have to further study how things work in order to find some coherent solution.

I may not have time to go back to this issue before next week unfortunately.

phep

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue9968
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue9968] Let cgi.FieldStorage have named uploaded file

[Éric Araujo]

Éric Araujo mer...@netwok.org added the comment:

Thanks for the report.  Do you want to work on a patch?

--
components: +Library (Lib) -Extension Modules
nosy: +eric.araujo, orsenthil
stage:  - needs patch
versions: +Python 3.2

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue9968
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue9968] Let cgi.FieldStorage have named uploaded file

[phep]

New submission from phep patrice.pil...@teletopie.net:

Hi,

Presently, in cgi.FieldStorage, uploaded file are accessible through a 
file-like object created by a call to tempfile.TemporaryFile(), probably in 
order to ensure the file is deleted when the process terminates.

The problem is that when one wants to save the data to some permanent location, 
one does not have other choice thant read() from this file descriptor then 
write() to the desired place, which means the file data shall be duplicated. 
This is greatly undesirable when the uploaded file is huge.

Replacing the call to tempfile.TemporaryFile() by a call to 
tempfile.NamedTemporaryFile() (available from 2.3) would have the following 
advantages :
1) no impact on existing code,
2) saving a file would be as simple as invoking 
os.link(fieldstorage['filenamefield'].file.name, /some/path)
3) There would be no need to duplicate data anymore.

The sole real inconvenience of this change would be to update the documentation 
accordingly.

tia,

phep

--
components: Extension Modules
messages: 117512
nosy: phep
priority: normal
severity: normal
status: open
title: Let cgi.FieldStorage have named uploaded file
type: feature request

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue9968
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue9968] Let cgi.FieldStorage have named uploaded file

[phep]

phep patrice.pil...@teletopie.net added the comment:

Oops. I forgot to aknowledge the fact that presently cgi.FieldStorage class 
documentation (but not the cgi module documentation) tells about the 
possibility to override the make_file() method in a user subclass to change the 
present limitation (which is actually how I work around the problem presently).

Yet, this does not (IMHO) render this feature request invalid.

phep

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue9968
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com