Skip to content

OnCommittedResponseWrapper fails on static resources served by Tomcat 8.5 #7261

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
danielwegener opened this issue Aug 15, 2019 · 4 comments
Closed
Assignees
Labels
in: web An issue in web modules (web, webmvc) status: backported An issue that has been backported to maintenance branches type: bug A general bug
Milestone

Comments

@danielwegener
Copy link
Contributor

danielwegener commented Aug 15, 2019

Component: spring-security-web-5.1.3.RELEASE.jar (also with latest 5.1.6)

TL;DR: Filters relying on OnCommittedResponseWrapper do not work when a response length is announced with ServletResponse.setContentLengthLong before it is written.

Hey guys. I've noticed, that my HeaderWriters are not triggered for responses served by Tomcats 8.5's default servlet. I've found that the OnCommittedResponseWrapper.onResponseCommitted is never triggered. Debugging through the filter chain I've noticed that tomcats org.apache.catalina.servlets.DefaultServlet uses ServletResponse.setContentLengthLong (see https://github.com/apache/tomcat/blob/8.5.45/java/org/apache/catalina/servlets/DefaultServlet.java#L1063) instead of ServletResponse.setContentLength, which is not covered by OnCommittedResponseWrapper (see

). Is there any reason why this method is not covered by committed-detection or is it simply an overlooked method during migration to servlet 3.1?

Keep up the good work!

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Aug 15, 2019
@danielwegener
Copy link
Contributor Author

danielwegener commented Aug 15, 2019

Confirmed. I've stopped with the debugger at https://github.com/apache/tomcat/blob/8.5.45/java/org/apache/catalina/servlets/DefaultServlet.java#L1063
and manually executed a response.setContentLength((int)length) which caused the HeaderWriter to write it's headers before commit as expected. Not the kind of proof I prefer, but I could not track down the actual commit change in the connector.Response.

To give some context: In my spring-mvc web-app, I let tomcat serve static resources (from web-fragment jar dependencies from classpath:META-INF/resources) using the containers default Servlet. I'd love to use spring security to add security relevant headers - especially CSP headers for html-files. I think a workaround would be to let spring handle static resources (which probably also is what spring boot is doing as a default).

@rwinch
Copy link
Member

rwinch commented Aug 15, 2019

@danielwegener Thanks for the report. Can you please put together a minimal sample to reproduce the issue?

@rwinch rwinch added status: waiting-for-feedback We need additional information before we can continue and removed status: waiting-for-triage An issue we've not yet triaged labels Aug 15, 2019
@danielwegener
Copy link
Contributor Author

danielwegener commented Aug 15, 2019

Here comes the reproducer (sort of, not necessarily tomcat specific): https://gist.github.com/danielwegener/6fe53c3f4625c1761de4507a3dbefe2b
Note that the underscores in filenames are directory separators.

When calling GET http://localhost:8080/test.html (served by tomcats DefaultServlet, important: call it without a cached version!) or GET http://localhost:8080/resourcelike/sucks you won't see any CSP response header.

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Aug 15, 2019
@danielwegener danielwegener changed the title OnCommittedResponseWrapper fails on Static resources served by Tomcat 8.5 OnCommittedResponseWrapper fails on static resources served by Tomcat 8.5 Aug 15, 2019
@rwinch
Copy link
Member

rwinch commented Aug 19, 2019

Thanks @danielwegener! Can you please turn that into a sample application that I can run rather than a gist?

@jzheaux jzheaux self-assigned this Aug 22, 2019
@jzheaux jzheaux added in: web An issue in web modules (web, webmvc) status: duplicate A duplicate of another issue type: bug A general bug and removed status: feedback-provided Feedback has been provided labels Aug 22, 2019
@jzheaux jzheaux added this to the 5.2.0.RC1 milestone Aug 22, 2019
@jzheaux jzheaux removed the status: duplicate A duplicate of another issue label Aug 22, 2019
jzheaux pushed a commit that referenced this issue Aug 22, 2019
Add setContentLengthLong tracking to OnCommittedResponseWrapper in
order to detect commits on servlets that use setContentLengthLong to
announce the entity size they are about to write (as used in the
Apache Tomcat's DefaultServlet).

Fixes gh-7261
@spring-projects-issues spring-projects-issues added the status: backported An issue that has been backported to maintenance branches label Aug 22, 2019
kostya05983 pushed a commit to kostya05983/spring-security that referenced this issue Aug 26, 2019
Add setContentLengthLong tracking to OnCommittedResponseWrapper in
order to detect commits on servlets that use setContentLengthLong to
announce the entity size they are about to write (as used in the
Apache Tomcat's DefaultServlet).

Fixes spring-projectsgh-7261
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: web An issue in web modules (web, webmvc) status: backported An issue that has been backported to maintenance branches type: bug A general bug
Projects
None yet
Development

No branches or pull requests

4 participants