From kevin.w.wall@gmail.com on January 01, 2014 21:25:37

I think that there's an implicit assumption that 'hits' would never be configured to be set to one as that would mean that no request could be made at all within any time period, but you have a valid point for when 'hits' is 2.

I've not yet scrutinized your patch in any depth, but this is also the first time that I've really took anything more than a very superficial look at RequestRateThrottleFilter. That said, your patch is more in line with how I would have written this. I will take a look at it when I get a bit more time. Thanks.

From eric.cit...@gmail.com on January 03, 2014 06:56:50

Thank you for your reply.

IMHO, hits=1 is a valid value. The way I understand it is that "you can do no more than 1 request in any period of 2 seconds". In other words: you just made a request, fine! now wait for 2 seconds before making another one.

From kevin.w.wall@gmail.com on January 22, 2014 22:45:25

Eric,
Would you like this work to be considered for the ESAPI hackathon contest? If so, please email me ASAP. Thanks.
-kevin wall kevin.w.wall@gmail.com

From kevin.w.wall@gmail.com on September 18, 2014 13:49:52

See related issue # 141.

From kevin.w.wall@gmail.com on September 18, 2014 13:51:20

Disregard previous comment. Won't fix 141.

@kwwall Are we taking this patch as-is? What would you like me to do here?

Actually, another small bug here. In the servlet init:

    //Note that we set hardcoded defaults in the class:
    private int hits = 5;

    private int period = 10;
    // stuff
    // Here we should be checking for null, so that the defaults we already specified are used 
    // instead of trying to possibly parse nulls.  

    public void init(FilterConfig filterConfig)
    {
        hits = Integer.parseInt(filterConfig.getInitParameter(HITS));
        period = Integer.parseInt(filterConfig.getInitParameter(PERIOD));
    }

I've attached a patch to handle the null check.
RequestRateThrottle.txt

One more question about the original code here @kwwall , I noticed that we're calling intern() on the session ID that we get. Is this wise? This usually signifies a memory leak.

   public void doFilter(ServletRequest request, ServletResponse response, FilterChain chain) throws IOException, ServletException
    {
        HttpServletRequest httpRequest = (HttpServletRequest) request;
        HttpSession session = httpRequest.getSession(true);

        synchronized( session.getId().intern() ) {
            Stack<Date> times = ESAPI.httpUtilities().getSessionAttribute("times");

@xeno6696 Not sure why the call to String.intern() here. However, it's being used at as a synchronization lock, so maybe it's being used as a fine-grained lock on a specific string in the String pool. I don't think it's going to cause any memory leak issues though.

@planetlevel - Jeff, do you know what String.intern() is used here?

String.intern() forces the string into permanently non-garbage collected memory. For session ids on a server that handles thousands of requests, and randomly generated strings small chance this won't cause a memory leak.

The only time you want to use that method is in embedded systems with limited processor speed, and even then, you need to make sure that all string comparisons use it.

@xeno6696 @planetlevel Would be nice of Oracle would document that nasty side effect in the Javadoc for String.intern() then. Waiting for Jeff's to reply. (Sent him separate email and CC'd you, Matt.) It seems like

synchronized( session.getId() ) {

would have worked fine here, but I want to make sure I'm not missing something more subtle in terms of lock contention or whatever.

Did some research, this bug appears to have been resolved: http://bugs.java.com/view_bug.do?bug_id=6962931