Roundup Tracker - Issues

Issue 2551387

classification
TypeError: not indexable
Type: crash Severity: normal
Components: Web interface Versions: 2.4.0
process
Status: fixed fixed
:
: rouilj : cmeerw, rouilj
Priority: :

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:24rouiljsetstatus: open -> fixed
resolution: remind -> fixed
messages: + msg8263
2025-01-12 19:06:32cmeerwsetstatus: pending -> open
messages: + msg8261
2025-01-12 18:44:45rouiljsetstatus: open -> pending
assignee: rouilj
resolution: remind
messages: + msg8260
2025-01-11 18:00:27cmeerwsetmessages: + msg8257
2025-01-11 16:54:10rouiljsetstatus: new -> open
nosy: + rouilj
messages: + msg8255
2025-01-11 09:24:04cmeerwsetpriority: normal ->
2025-01-11 09:18:37cmeerwcreate