in roundup/cgi/actions.py
raise exceptions.NotFound(msg) should give the msg to the web browser.
It does not with 4624:21705126dafa (pre 1.4.20).

Should be fixed and then the following code needs to be replaced:
       # check if all columns exist on class
        # the exception must be raised before sending header
        props = klass.getprops()
        for cname in columns:
            if cname not in props:
                # TODO raise exceptions.NotFound(.....) does not give
message
                # so using SeriousError instead
                self.client.response_code = 404
                raise exceptions.SeriousError(
                    self._('Column "%(column)s" not found on
%(class)s')
                    % {'column': cgi.escape(cname), 'class':
request.classname})

in roundup/cgi/actions.py to use NotFound().

The reason the message doesn't show up is that NotFound is supposed to
generate a 404 response to the client. The template is changed to 404
in client.py and the _generic.404.html has no support for messages.

I have made two changes:

 1) The response_code is set to 400 before raising the NotFound
    error. 400 (Bad Request) indicates that there is a client side
    issue.  I.E. it fed a bogus column value to the server.

 2) client.py's NotFound handling is changed to look at the
    response_code and if it is not 400, the usual 404 response_path is
    taken. If the code is 400, the user is presented with the message
    formatted like it is formatted for SeriousError.

The documentation for the NotFound exception doesn't explain when it
should be used. Arguablly SeriousError is the right way to handle this
if NotFound is supposed to be a pure "404" error code.

Suprisingly asking for a non-existent class looks like it doesn't use
the 404 template. E.G. compare:

   http://issues.roundup-tracker.org/bug2550755 (not using template)
to

   http://issues.roundup-tracker.org/issue2550755776 (using the
     _generic.404.html template)

So Bern should this attached patch be committed?

-- rouilj

Ping Bern, any feedback. If none, I am going to apply it as is
returning the 400 error code.

Hi John,
thanks for the analysis and the patch.
Currently I lack the time to do a review.
Your analysis reads fine, so please apply if you are confident
that there is no bad side effect. You have probably improved the
situation. :)

Thanks and best Regards,
Bernhard

Hi Bern:

I needed to change two test cases in test/test_cgi.py as well:

   FormTestCase.testCSVExportBadColumnName
   FormTestCase.testCSVExportFailPermission

they both expected exceptions.SeriousError and I am now raising
exceptions.NotFound.

test_cgi.py::testCSVExportFailPermission was changed on issue2550712
(exportcsvaction errors poorly when given invalid columns). Ced worked
on the patch for this as well as you.

The diff for the test on commit 21705126dafa for issue2550712 is:

     def testCSVExportFailPermission(self):
         cl = self._make_client({'@columns': 'id,email,password'},
nodeid=None,
             userid='2')
@@ -937,7 +947,9 @@
         output = StringIO.StringIO()
         cl.request = MockNull()
         cl.request.wfile = output
-        self.assertRaises(exceptions.Unauthorised,
+        # used to be self.assertRaises(exceptions.Unauthorised,
+        # but not acting like the column name is not found
+        self.assertRaises(exceptions.SeriousError,
             actions.ExportCSVAction(cl).handle)

The name of the test makes me think that Unauthorised should be
raised always. If the user user2 can't access the user class, how
could they find out that the column doesn't exist?

I added ced to this ticket since he worked on the patch.
Maybe the patch for that issue needs to be tweaked?

Thoughts?

I have checked in the change from exceptions.SeriousError to
exceptions.NotFound to get the 2 tests to pass again.

I also renamed

  test/test_cgi.py::FormTestCase::testCSVExportFailPermission

to

  test/test_cgi.py::FormTestCase::testCSVExportFailPermissionBadColumn

and added

 test/test_cgi.py::FormTestCase::testCSVExportFailPermissionValidColumn

that verifies that exceptions.Unauthorised is raised.

I am leaving this ticket open pending a decision on what should be
raised with the testCSVExportFailPermissionBadColumn test.

Hi John,
from briefly reading the issue:
I believe your thinking is good:
If a user misses permissions, she should not able to find
out much about the data structure of the tracker.

Best,
Bernhard

I am closing this.

There was an open question about being able to probe the schema because of different responses between no permissions to access property in schema and the property in the schema doesn't exist.

Hiding the schema smells of security through obscurity. So I am closing this as is.