search

vidyuthd / owasp-esapi-java

Automatically exported from code.google.com/p/owasp-esapi-java
0 stars 0 forks source link

RequestRateThrottleFilter does not work as expected. #141

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago 
What steps will reproduce the problem?
1. Install RequestRateThrottleFilter in web.xml

What is the expected result? 
- A hit rate limit based on the number of hits within a period.
- After a hit rate limit being triggered, the web site should continue to work.

What do you see instead?
- The hit rate limit is based on the oldest request within a period instead of 
the above.

- The website did not function properly because javascript resource requests 
had been intercepted by the hit rate filter such that the hit rate exceeded 
message was cached instead of valid java script (note: this can be fixed by 
adding a no-cache response header.)

- Implementation is also sub-optimal in its use of Date.

What version of the product are you using? On what operating system?
Trial of ESAPI-2.0-RC7

I've attached an improvement, hopefully with all the above problems addressed.

Best Regards,

Lee
email: leewg@fal.co.nz

Original issue reported on code.google.com by blue.re...@gmail.com on 25 Aug 2010 at 9:37

Attachments:

GoogleCodeExporter commented 9 years ago 
This is a solid patch, I'll review it before 2.0 final, thank you sir.

Original comment by manico.james@gmail.com on 1 Nov 2010 at 4:26

GoogleCodeExporter commented 9 years ago 
This patch is Java 1.6 (LinkedBlockingDeque) and ESAPI is 1.5 based. Can you 
suggest a 1.5 patch? This is great code - I'd like to get this fix in before 
2.0 GA. Any help is appreciated!

Thanks kindly,
Jim

Original comment by manico.james@gmail.com on 1 Nov 2010 at 4:42

GoogleCodeExporter commented 9 years ago

Original comment by manico.james@gmail.com on 2 Nov 2010 at 8:06

GoogleCodeExporter commented 9 years ago 
The patch from Lee is 1.6, but we could make this work if we include the JSR166 
packport

http://sourceforge.net/projects/backport-jsr166/files/backport-jsr166/3.1/backpo
rt-util-concurrent-Java50-3.1.zip/download

Original comment by manico.james@gmail.com on 3 Nov 2010 at 5:46

GoogleCodeExporter commented 9 years ago 
It looks like the basic idea here is that we have a blocking deque that is 
being used essentially as a Stack.

So in other words, a request comes in – we get the existing deque off of the 
session (or create a new one if there isn’t one yet) 

We then push the time of the current request (in ms) onto the head of the 
stack. 

We then examine the tail of the stack looking for requests older than whatever 
the maximum threshold is and remove them from the tail of the stack.

Once we are done maintaining the stack, we check the size of the stack and if 
it exceeds the maximum threshold of requests/time then we error the request.

Does this sound like the basic idea?

We could easily do this (using a Queue) in Java 5 without adding the additional 
dependencies using a LinkedBlockingQueue

Request comes in, do same logic – check session for existing queue, if absent 
create a new one

Push the time in MS onto the tail of the queue

Peek and Remove from the head of the queue for anything older than the time 
threshold

Check size of queue and if it exceeds the max threshold of requests/time then 
error the request...

This achieves the same logic, with a minimal hit to performance and keeps it 
thread-safe without having to add external synchronization or locking.  Notice 
that basically all we did was flip the Stack from Lee’s implementation into a 
Queue so it is still a FIFO collection, just the head and tail are reversed 
from the Deque implementation. Also – generally speaking, since you are not 
retrieving and pushing onto both ends of the Collection – I’m not sure it 
makes sense, and could possibly even be a bit of a performance hit to use a 
Deque in this case (although that is largely speculation on my part – it may 
be a NIL effect on performance, but definitely seems odd to use a double-ended 
collection if you are only working one-way)

The only real benefit I see to using the LinkedBlockingDeque is this statement 
from the API Docs: 
    “Most operations run in constant time (ignoring time spent blocking).”

However, I think that this would be a negligable performance difference due to 
the environment – containers are multi-threaded but since we are dealing with 
information local to a specific session, I think it is pretty much an 
impossibility that you would be able to generate enough simultaneous traffic on 
a single session that this would become an issue. 

Thoughts?

Original comment by chrisisbeef on 6 Nov 2010 at 7:54

GoogleCodeExporter commented 9 years ago

Original comment by manico.james@gmail.com on 20 Nov 2010 at 11:30

GoogleCodeExporter commented 9 years ago 
We can't make the assumption about setting cache-control headers in this 
filter. This should be a implementation concern.

Original comment by chrisisbeef on 18 Sep 2014 at 8:51

GoogleCodeExporter commented 9 years ago 
Now that we require at least JDK 1.6, we can do this.
The cache related headers can be driven via Java properties; either a new 
property in ESAPI.properties or a new property file, depending on the 
flexibility required. Given that most of this is already provided, this may 
even make an ideal first bug candidate.

Original comment by kevin.w.wall@gmail.com on 23 Sep 2014 at 1:39