I'm running Roundup 1.4.15 from the Debian testing packages on
http://pyblosxom.bluesock.org/ .

GoogleBot hits the following url on my Roundup instance several times a day:

/issue?@action=export_csv&@columns=kobtoscgsowfa&@group=actor,creator&@pagesize=50&@startwith=50

The problem with that is that kobtoscgsowfa is not a valid column.  I
have no clue where GoogleBot got this link, but it's totally wrong.

Anyhow, every time GoogleBot hits this url, Roundup kicks up a KeyError
because kobtoscgsowfa is not a valid propname in the database and I get
this email:


KeyError: 'kobtoscgsowfa'	Python 2.6.6
/usr/bin/python

A problem occurred while running a Python script. Here is the sequence
of function calls leading up to the error, with the most recent
(innermost) call first. The exception attributes are:

/usr/lib/pymodules/python2.6/roundup/backends/rdbms_common.py in
get(self=<hyperdb.Class "issue">, nodeid='18', propname='kobtoscgsowfa',
default=[], cache=1)
 1561 
 1562         # get the property (raises KeyErorr if invalid)
 1563         prop = self.properties[propname]
      prop = undefined, self = <hyperdb.Class "issue">, global
properties = undefined, propname = 'kobtoscgsowfa'
 1564 
 1565         # handle there being no value in the table for the property

/usr/lib/pymodules/python2.6/roundup/cgi/actions.py in
handle(self=<roundup.cgi.actions.ExportCSVAction instance>)
 1065                         'You do not have permission to view %(class)s'
 1066                     ) % {'class': request.classname})
 1067 
      row = [], global append = undefined, global str = undefined, klass
= <hyperdb.Class "issue">, global get = undefined, itemid = '18', name =
'kobtoscgsowfa'
 1068                 row.append(str(klass.get(itemid, name)))
 1069 

/usr/lib/pymodules/python2.6/roundup/cgi/actions.py in
execute(self=<roundup.cgi.actions.ExportCSVAction instance>)
   37         """Execute the action specified by this object."""
   38         self.permission()
   39         return self.handle()
      self = <roundup.cgi.actions.ExportCSVAction instance>, global
handle = undefined
   40 
   41     name = ''

/usr/lib/pymodules/python2.6/roundup/cgi/client.py in
handle_action(self=<roundup.cgi.client.Client instance>)
 1140                 return getattr(self, action_klass)()
 1141             else:
 1142                 return action_klass(self).execute()
      action_klass = <class roundup.cgi.actions.ExportCSVAction>, self =
<roundup.cgi.client.Client instance>, global execute = undefined
 1143 
 1144         except (ValueError, Reject), err:

/usr/lib/pymodules/python2.6/roundup/cgi/client.py in
inner_main(self=<roundup.cgi.client.Client instance>)
  450                 # possibly handle a form submit action (may change
self.classname
  451                 # and self.template, and may also append
error/ok_messages)
  452                 html = self.handle_action()
      html = undefined, self = <roundup.cgi.client.Client instance>,
global handle_action = undefined
  453 
  454                 if html:


There are two big problems here:

1. The handle method of ExportCSVAction in roundup/cgi/actions.py
doesn't check to see if the columns are valid before it starts doing
things based on them.  It doesn't catch the KeyError, so it fails on bad
input from the url which is uber-bad.

2. At the point where it's failing, it's already sent the HTTP response
headers with a HTTP 200.  So not only does it fail and send me an email,
but the response is an HTTP 200 so GoogleBot doesn't even know it's
doing something stupid.


I think the solution here is to verify that the columns are correct in
ExportCSVAction _before_ it starts sending HTTP headers and if they're
not correct, kick up an error that would get properly handled into an
HTTP 404 or 500 or whatever.

I checked Roundup 1.4.18 and I'm pretty sure the problem is there, too.

If someone can walk me through validating the columns, I can write a patch.

Meanwhile, I did something totally bogus and wrapped the row.append line
in a try/except:

    try:
        row.append(str(klass.get(itemid, name)))
    except KeyError:
        row.append("")

Hi Will,
thanks for the report.
Indeed the next step, as I see it, is to make sure that the defect
is there in current svn and then ideally write a test that
fails to define the behabiour and then write the test.

Do you think you can write a test for this?

Best,
Bernhard

I spent a few hours trying to write a test last month and wasn't able to
get the test working, so I gave up.

The code hasn't changed between the version I was using and what was in
svn when I reported the bug, thus I have no reason to believe that it's
fixed.  Having said that, I didn't spend the time to set it all up with
the latest Roundup in svn to see if it's still a problem.

Between this issue and another issue I'm now having where Roundup is
emailing me error reports that are due to invalidated user-supplied
querystrings, I'm probably going to switch to a different system and am
thus unlikely to do any further work on this ticket.

Sorry about that.  Hope you figure it out.

Hi Will,
thanks for trying to write a test!
Sorry to here that roundup is not working for you.

What would we need to do to make it work?
Fix this issue and the other one you are having?
Is there already a report for it?

Best Regards,
Bernhard

Here is a patch that fixes the issue.
It checks before sending the header that all columns are properties of
the klass.

Here is a better one that return 404 when the columns name are wrong.

Cédric, thanks for contributing the patch!
I wonder if it could be improved further my using the phrase:
  for name in columns:
    if not props.has_key(name):
      raise exceptions.NotFound(name)

Yes, here is a new version (I used "not in" instead of "has_key").

Cool! (I was just thinking aloud. Note that my python doc from 2.5 says
that has_key() should be used in modern code, I did not look up why.)

On 02/05/12 14:34 +0000, Bernhard Reiter wrote:
> Cool! (I was just thinking aloud. Note that my python doc from 2.5 says
> that has_key() should be used in modern code, I did not look up why.)

See http://docs.python.org/library/stdtypes.html#dict.has_key
Indeed has_key was removed in Python3 and “key in d” is more Pythonic ;-)

Wow, http://docs.python.org/release/2.5.4/lib/typesmapping.html
still has "a.has_key(k)  Equivalent to k in a, use that form in new
code". But yes, since 2.6 it is called deprecated.

Testing with current tip in hg (pre 1.4.20) and python 2.7.2 and I get the issue
when running demo.py and trying to csv export something with a column that does 
not exist. I can see it with curl and a webbrowser.

However I don't like the resulting error message, somehow the
expection.NotFound does not propagate a message up.
I believe that ./cgi/client.py will actually reraise raise NotFound(e)
but I haven't tracked it to roundup_server. So what do we need to change
to get the message out that we could not find that column?

To clarify my last comment: I mean the patch improves the situation, but
comes out with an error message like: 

<title>Item Not Found</title>
</head>

<body>
There is no <span>issue</span> with id <span></span>

While I believe it should be mentioned that we could not find a column.

I could have a look to improve the error message.

Here is a new version of the patch.
I used SeriousError exception instead of NotFound because NotFound seems
to be used only when an item is not found.

Cédric, ah good idea.
I've tried to find out why NotFound does not use the message,
which in itself may be an issue with the codebase.

However we may still have an issue with your patch:
The column name may contain arbitrary user input, thus it may be used
to do a cross scripting attack, we should check if it is properly
sanitized. What do you think?

On 14/05/12 08:20 +0000, Bernhard Reiter wrote:
> Cédric, ah good idea.
> I've tried to find out why NotFound does not use the message,
> which in itself may be an issue with the codebase.

I don't think, there is an issue with NotFound indeed I was
misunderstanding his usage.

> However we may still have an issue with your patch:
> The column name may contain arbitrary user input, thus it may be used
> to do a cross scripting attack, we should check if it is properly
> sanitized. What do you think?

Is there any sanitize method available in roundup?

I believe that NotFound does get a message sometimes
and the class itself looks like it was made for this kind of 
situation. On the other hand, I don't know the indention of the writers.
To me it looked natural.

As for a sanitize method: I don't know either.
We should ask both on the devel-list.

Okay, pushing a slightly improved patch into the repository now.

I've added a test case and also fixed another one.
Änderung:        4624:21705126dafa