Re: Spam attack, observations, how to repair

In message <[email protected]>,
"John P. Rouillard" writes:
>In message <[email protected]>,
>Ralf Schlatterbeck writes:
>>On Fri, Jul 04, 2014 at 12:45:51PM +0200, Kay Hayen wrote:
>>> I just checked with your own issue tracker, that both links work:
>>> 
>>> http://issues.roundup-tracker.org/file1496/roundupdb.py/
>>> http://issues.roundup-tracker.org/file1496/roundupdb.py
>>> 
>>> The first ought to 404 from roundup in my mind.
>>No, this always renders if after the fileXXX a '/' follows.
>
>Also '/' should be allowed as in theory you can create a (RESTful)
>link like: [...]

>Now with that understanding, I claim that a trailing '/' in the
>current roundup is an error.
>
>Ralf, is there some historic reason the trailing '/' is allowed for
>files given that it returns the same data as a url without the trailing
>'/'? I.E> both:
>
>http://issues.roundup-tracker.org/file1496/roundupdb.py
>http://issues.roundup-tracker.org/file1496/roundupdb.py/
>
>return the same page.
>
>I would like the trailing / form for files to return a 404 personally
>so in the future it can be used as part of a REST (or other)
>interface.
>
>Also it would be consistent with:
>
>http://issues.roundup-tracker.org/issue2550671/ (returns 404)
>http://issues.roundup-tracker.org/issue2550671 (doesn't return 404)
>
>http://issues.roundup-tracker.org/user5/ (returns 404)
>http://issues.roundup-tracker.org/user5 (doesn't return 404)

Delving into this a little more, I realized from:

http://roundup.sourceforge.net/docs/customizing.html#determining-web-context

the form item_designator/foo is always handled using:

  5) if the path starts with an item designator and is longer than one
     entry (as in example 5, “file1/kitten.png”), then we’re assumed to
     be handling an item of a FileClass, and the extra path information
     gives the filename that the client is going to label the download
     with (i.e. “file1/kitten.png” is nicer to download than
     “file1”). This raises a SendFile exception.

which seems wrong. It should only raise a SendFile if the class
(klass) for the designator matches:

   isinstance(klass, hyperdb.FileClass):


But this explains why we get a 404 returned for .../user5/ as there is
no file object to send at user5.

I think this diff goes part way to fixing this issue:

--- a/roundup/cgi/client.py     Fri Jul 04 15:43:22 2014 +0200
+++ b/roundup/cgi/client.py     Fri Jul 04 20:46:20 2014 -0400
@@ -906,9 +906,6 @@
             raise SendStaticFile(os.path.join(*path[1:]))
         else:
             self.classname = path[0]
-            if len(path) > 1:
-                # send the file identified by the designator in path[0]
-                raise SendFile(path[0])

         # see if we got a designator
         m = dre.match(self.classname)
@@ -921,6 +918,14 @@
                 raise NotFound('%s/%s'%(self.classname, self.nodeid))
             if not klass.hasnode(self.nodeid):
                 raise NotFound('%s/%s'%(self.classname, self.nodeid))
+            if len(path) > 1:
+                if isinstance(klass, hyperdb.FileClass):
+                    # send the file identified by the designator in path[0]
+                    raise SendFile(path[0])
+                else:
+                    self.add_error_message("HINT: try removing a trailing path from %s"%"/".join(path))
+                    raise NotFound("HINT: try removing a trailing path from %s"%"/".join(path))
+
             # with a designator, we default to item view
             self.template = 'item'
         else:

(hopefully that's not too mangled by email to understand.)

However this patch causes another issue.

The 404 that is raised by NotFound("HINT...") uses the
tracker_home/html/_generic.404.html template which doesn't display the
text of the raised NotFound error. Nor does it display the error
message added by add_error_message.

As a result the error for a url like:

   http://localhost:8917/demo/issue1/

is a 404 with the text:

    There is no issue with id 1

which is suboptimal and confusing.

I would almost prefer to have a response of:

  Error response

  Error code 404.

  Message: /demo/issue1/.

  Error code explanation: 404 = Nothing matches the given URI.

which indicates that it didn't even try to parse it. (This also is
error form returned by:

  http://issues.roundup-tracker.org/issue2550671/

which seems to bypass the _generic.404 template. Also a non-existing
class designator like ..//demo/zot1 bypasses the _generic.404 template.)

So I am not quite sure what's the right thing to do here. I do have a
replacement _generic.404.html that will display the argument to
add_error_message, but it seems the value passed to the exception
class when the exception is raised should be used somehow.

Comments, ideas?

--
				-- rouilj
John Rouillard
===========================================================================
My employers don't acknowledge my existence much less my opinions.

------------------------------------------------------------------------------
Open source business process management suite built on Java and Eclipse
Turn processes into business applications with Bonita BPM Community Edition
Quickly connect people, data, and systems into organized workflows
Winner of BOSSIE, CODIE, OW2 and Gartner awards
http://p.sf.net/sfu/Bonitasoft

_______________________________________________
Roundup-users mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/roundup-users