On 13/10/2013 14:11, Konstantin Kolinko wrote: <snip/>
>> URL: http://svn.apache.org/r1531115 >> Log: >> Fix https://issues.apache.org/bugzilla/show_bug.cgi?id=54095 >> Add support to the Default Servlet for serving gzipped versions of static >> resources directly from disk as an alternative to Tomcat compressing them on >> each request. Patch by Philippe Marschall. <snip/> > General: > I think this feature should be opt-in, like the listings feature of > DefaultServlet, being off by default. I disagree since: - this is only in 8.0.x and we haven't had a stable release yet. - the user has to create the gzip'd version which is unlikely to exist be default before this feature does anything I agree if it is ever back-ported to earlier versions it needs to be disabled by default. > Concerns: > (1) Excessive disk access to check existence of gz files. The caching mechanism should handle this the same way it does for any other static resource. > (2) Additional access path for ".gz" files, which might be not covered > by security constraints > > (3) Interoperability with filters that may preprocess or postprocess > the response, > including ISE handling in the following lines of DefaultServlet: > > [[[ > try { > ostream = response.getOutputStream(); > } catch (IllegalStateException e) { > ... > writer = response.getWriter(); > ]]] > > (2) and (3) would occur only if both "foo" and "foo.gz" files are present. I've disabled the fall back for (3). > As (2) requires the presence and accessibility of "foo" file (as > "resource.exists()" check is done before gzip processing), I see no > exploitable issue here, though we may mention this on the > "security-howto" page. I think it is worth mentioning that the .gz versions will be directly accessible and need to be protected if the normal version is protected. > Technical issues with the code: > 1) "If-Modified-Since" header processing is inconsistent, Fixed. > 2) I think that if it is an "included" resource, the gzip feature > should be disabled. Fixed. > 3) The following line: >> + gzipResource.setMimeType(contentType); > > I think that line is not needed here, as the "Content-Type" header is > served from a local variable, not property of the resource. Fixed. Thanks for the review, Mark --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
