Why does the ESAPI ClickjackFilter have to come after the SiteMesh filter?

We have an application using OpenSymphony SiteMesh to assemble pages, and we've added the OWASP ESAPI ClickjackFilter to add the X-FRAME-OPTIONS header to responses.

However, it only works if the ClickjackFilter mapping comes after the SiteMeshFilter mapping in web.xml. If the clickjacking filter comes first, then the X-FRAME-OPTIONS header isn't added.

This works:

<filter-mapping>
    <filter-name>sitemesh</filter-name>
    <url-pattern>/web/*</url-pattern>
</filter-mapping>

<filter-mapping>
    <filter-name>Clickjacking filter</filter-name>
    <url-pattern>/*</url-pattern>
</filter-mapping>

This doesn't work:

<filter-mapping>
    <filter-name>Clickjacking filter</filter-name>
    <url-pattern>/*</url-pattern>
</filter-mapping>

<filter-mapping>
    <filter-name>sitemesh</filter-name>
    <url-pattern>/web/*</url-pattern>
</filter-mapping>

Why would the ordering of these two filters matter?

Solution

In my opinion, I think it is because the ESAPI ClickjackFilter's doFilter() method is written incorrectly. It is implemented like this:

public void doFilter(ServletRequest request, ServletResponse response, FilterChain chain) throws IOException, ServletException
        {
        HttpServletResponse res = (HttpServletResponse)response;
        chain.doFilter(request, response);
        res.addHeader("X-FRAME-OPTIONS", mode );
        }

However, because it is an output filter, it should be first wrapping the response using something like HttpServletResponseWrapper. I think it should be written something like this:

public void doFilter(ServletRequest request, ServletResponse response, FilterChain chain) throws IOException, ServletException
        {
        HttpServletResponse res =
            new javax.servlet.http.HttpServletResponseWrapper(
                                            (HttpServletResponse)response );
        chain.doFilter(request, res);
        res.addHeader("X-FRAME-OPTIONS", mode );
        }

If it were written in that manner, I think it should work regardless of the order the are applied in.

CAVEAT: Note that I've not verified this at all (in fact, I haven't even tried to compile it!), but I think this is likely what is wrong. In theory, the SiteMesh filter could be doing something funky as well, but I think that's less likely. If someone confirms this is what is wrong, let me know and I'll file an ESAPI bug report.