Issue 2551387
Created on 2025-01-11 09:18 by cmeerw, last changed 2025-01-12 19:52 by rouilj.
Messages | |||
---|---|---|---|
msg8253 | Author: [hidden] (cmeerw) | Date: 2025-01-11 09:18 | |
I get the occasional email from roundup with: Traceback (most recent call last): File "/home/app-roundup/roundup/lib/roundup/cgi/client.py", line 836, in inner_main csrf_ok = self.handle_csrf() ^^^^^^^^^^^^^^^^^^ File "/home/app-roundup/roundup/lib/roundup/cgi/client.py", line 1646, in handle_csrf if '@csrf' in self.form: ^^^^^^^^^^^^^^^^^^^^ File "/usr/lib/python3.12/cgi.py", line 584, in __contains__ raise TypeError("not indexable") TypeError: not indexable Presumably, someone is posting rubbish to the URL; it can be reproduced using: curl -X POST https://issues.example.com/ -H "Content-Type: text/plain" -d "test" (replacing issues.example.com with a roundup instance) |
|||
msg8255 | Author: [hidden] (rouilj) | Date: 2025-01-11 16:54 | |
Hi Christof: Thanks for the report. I don't see that issue current devel. What version of Roundup are you running? How are you running it (cgi, wsgi, proxy to roundup-server)? This info may indicate that this has been fixed already or that my tests are invalid. Line 1691 (https://sourceforge.net/p/roundup/code/ci/default/tree/roundup/cgi/client.py#l1691) in the current development client.py is the same line you reference in your issue I think. If I run your curl command (setting a few extra headers (Origin, Referer etc. to pass checks) I get: > roundup/cgi/client.py(1691)handle_csrf() -> if '@csrf' in self.form: (Pdb) p self.form FieldStorage(None, None, b'test') and I don't get the exception. Can you tell me what self.form is set to when running your code? There is code earlier in handle_csrf (around devel line 1515) that reads: # Assume: never allow changes via GET if self.env['REQUEST_METHOD'] not in ['POST', 'PUT', 'DELETE']: if (self.form.list is not None) and ("@csrf" in self.form): so it looks like a GET can result in self.form.list set to None. (Although curl -X GET https://tracker.com/tracker/ -H "Content-Type: text/plain" has self.form.list set to a FieldStorage.) Apparently I thought self.form.list would always be a FieldStorage object with POST, PUT, DELETE. My testing seems to indicate that's the case. Even: curl -X DELETE https://tracker.com/tracker/ -H "Content-Type: text/plain" has self.form.list equal to a FieldStorage. Further info or thoughts? Thanks. |
|||
msg8257 | Author: [hidden] (cmeerw) | Date: 2025-01-11 18:00 | |
Didn't think it would make a difference how I am running it, but it apparently does. Using roundup 2.4.0 with wsgi_handler (via flup). I don't see the issue with the built-in server from demo.py. So the reason appears to be that in wsgi_handler.py we have if environ['REQUEST_METHOD'] in ("OPTIONS", "DELETE"): # these methods have no data. When we init tracker.Client # set form to None to get a properly initialized empty # form. form = None else: form = BinaryFieldStorage(fp=environ['wsgi.input'], environ=environ) but in client.py we have # see if we need to re-parse the environment for the form (eg Zope) if form is None: # cgi.FieldStorage doesn't special case OPTIONS, DELETE or # PATCH verbs. They are processed like POST. So FieldStorage # hangs on these verbs trying to read posted data that # will never arrive. # If not defined, set CONTENT_LENGTH to 0 so it doesn't # hang reading the data. if self.env['REQUEST_METHOD'] in ['OPTIONS', 'DELETE', 'PATCH'] \ and 'CONTENT_LENGTH' not in self.env: self.env['CONTENT_LENGTH'] = 0 logger.debug("Setting CONTENT_LENGTH to 0 for method: %s", self.env['REQUEST_METHOD']) # cgi.FieldStorage must save all data as # binary/bytes. Subclass BinaryFieldStorage does this. # It's a workaround for a bug in cgi.FieldStorage. See class # def for details. self.form = BinaryFieldStorage(fp=request.rfile, environ=env) # In some case (e.g. content-type application/xml), cgi # will not parse anything. Fake a list property in this case if self.form.list is None: self.form.list = [] else: self.form = form So in the wsgi case we end up with self.form.list == None, but in the demo.py case that's being set to self.form.list = [] And then we get the exception in the wsgi case only. |
|||
msg8260 | Author: [hidden] (rouilj) | Date: 2025-01-12 18:44 | |
Hi Christof: Thanks for the additional analysis. Ok, here is the joke. In client.py the line: # see if we need to re-parse the environment for the form (eg Zope) if form is None: form is not None, it is: FieldStorage(None, None, b'test') but form.list is None because b'test' isn't a list. if I move: # In some case (e.g. content-type application/xml), cgi # will not parse anything. Fake a list property in this case if self.form.list is None: self.form.list = [] after the 'if form is None:' so it applies in both cases of the if statement, things work as expected. This looked safe to me because the form should always be some type of cgi.FieldStorage. However it broke a bunch of poorly written (by me) tests. I have fixed the tests. I have also added a 'try: [...] except AttributeError [...]' block around it and log a request to post to roundup-users when self.form.list generates an AttributeError. I have checked it in on changeset: 8268:05d8806b25ad. It has passed CI on github. Christof can you run the current repository tip and verify that it fixes your issue. Thanks. |
|||
msg8261 | Author: [hidden] (cmeerw) | Date: 2025-01-12 19:06 | |
Thanks, looks good. I have just applied the patch to my 2.4 installation, and can't reproduce it with that change any more. |
|||
msg8263 | Author: [hidden] (rouilj) | Date: 2025-01-12 19:52 | |
Hi Christof, Thanks for the test. Closing as fixed. Have a good day/evening. |
History | |||
---|---|---|---|
Date | User | Action | Args |
2025-01-12 19:52:24 | rouilj | set | status: open -> fixed resolution: remind -> fixed messages: + msg8263 |
2025-01-12 19:06:32 | cmeerw | set | status: pending -> open messages: + msg8261 |
2025-01-12 18:44:45 | rouilj | set | status: open -> pending assignee: rouilj resolution: remind messages: + msg8260 |
2025-01-11 18:00:27 | cmeerw | set | messages: + msg8257 |
2025-01-11 16:54:10 | rouilj | set | status: new -> open nosy: + rouilj messages: + msg8255 |
2025-01-11 09:24:04 | cmeerw | set | priority: normal -> |
2025-01-11 09:18:37 | cmeerw | create |