ContentCachingResponseWrapper no longer honors Content-Type and Content-Length #32317

yikuo123 · 2024-02-23T04:09:51Z

As of Spring Framework 6.1.4, ContentCachingResponseWrapper returns null for Content-Type and Content-Length via getHeader("Content-Type"), getContentType(), etc.

The text was updated successfully, but these errors were encountered:

yikuo123 · 2024-02-23T04:18:05Z

It may be caused by this commit 375e0e6

amuttsch · 2024-02-23T11:14:16Z

We have the problem that our rest controller, which sits behind a ShallowEtagHeaderFilter, does not return the Content-Type header anymore. I guess this is also related to this change.

jhoeller · 2024-02-23T12:18:18Z

Do you specifically see a difference for programmatic calls on the HttpServletResponse there? Where do the header values (that you expect to see there) come from, are they being set programmatically elsewhere (outside of ContentCachingResponseWrapper)?

tjalling-ran · 2024-02-23T14:19:12Z

spring-framework 5.3.32 may also be affected.

After upgrading to it (because of CVE-2024-22243), we no longer had content type headers in responses. So I downgraded back to 5.3.29. (luckily, we don't think we have any endpoints that are vulnerable to the CVE)

amuttsch · 2024-02-23T14:24:49Z

@jhoeller We set them through the @GetMapping(produces = [MediaType.APPLICATION_JSON_VALUE]) annotation. We return a ResponseEntity.ok() and also tried to set the content type using .contentType(MediaType.APPLICATION_JSON_VALUE), which did not work either.

sbrannen · 2024-02-23T16:23:09Z

Thanks for all of the feedback!

We're looking into a fix.

Commit 375e0e6 introduced a regression in ContentCachingResponseWrapper (CCRW). Specifically, CCRW no longer honors Content-Type and Content-Length headers that have been set on the wrapped response and returns null for those header values if they have not been set directly on the CCRW. This commit fixes this regression as follows. - The Content-Type and Content-Length headers set in the wrapped response are honored in getContentType(), containsHeader(), getHeader(), and getHeaders() unless those headers have been set directly on the CCRW. - In copyBodyToResponse(), the Content-Type in the wrapped response is only overridden if the the Content-Type has been set directly on the CCRW. See spring-projectsgh-32039 Closes spring-projectsgh-32317

sbrannen · 2024-02-24T15:54:45Z

I've pushed a potential fix to 6.1.x and main (see commit 5680d20).

@yikuo123, @amuttsch, @tjalling-ran, and anybody else interested in helping out...

We would be grateful if you could try out one of the upcoming 6.1.5-SNAPSHOT builds and let us know if the proposed fix addresses the regressions you've encountered!

If you confirm the fix works, we'll then backport to 6.0.x and 5.3.x.

Cheers,

Sam

p.s. you can find information on accessing snapshot builds here.

sbrannen · 2024-02-25T15:27:04Z

That was fast! 🚀

😎

Appreciate the clear analysis / explanation in 5680d20

You're welcome, and glad you found it useful.

I'd be happy to try out a 5.3.x backport build if/when testers are desired for that. Unfortunately, we don't have any 6+ project that I can test with.

Thanks for offering.

We'll let you know as soon as we've backported anything to 5.3.x.

Prior to this commit, getHeaderNames() returned duplicates for the Content-Type and Content-Length headers if they were set in the wrapped response as well as in ContentCachingResponseWrapper. This commit fixes that by returning a unique set from getHeaderNames(). In addition, this commit introduces a new test in ContentCachingResponseWrapperTests to verify the expected behavior for Content-Type and Content-Length headers that are set in the wrapped response as well as in ContentCachingResponseWrapper. See gh-32039 See gh-32317

Commit 375e0e6 introduced a regression in ContentCachingResponseWrapper (CCRW). Specifically, CCRW no longer honors Content-Type and Content-Length headers that have been set in the wrapped response and now incorrectly returns null for those header values if they have not been set directly in the CCRW. This commit fixes this regression as follows. - The Content-Type and Content-Length headers set in the wrapped response are honored in getContentType(), containsHeader(), getHeader(), and getHeaders() unless those headers have been set directly in the CCRW. - In copyBodyToResponse(), the Content-Type in the wrapped response is only overridden if the Content-Type has been set directly in the CCRW. Furthermore, prior to this commit, getHeaderNames() returned duplicates for the Content-Type and Content-Length headers if they were set in the wrapped response as well as in CCRW. This commit fixes that by returning a unique set from getHeaderNames(). This commit also updates ContentCachingResponseWrapperTests to verify the expected behavior for Content-Type and Content-Length headers that are set in the wrapped response as well as in CCRW. See gh-32039 See gh-32317 Closes gh-32321

Commit 375e0e6 introduced a regression in ContentCachingResponseWrapper (CCRW). Specifically, CCRW no longer honors Content-Type and Content-Length headers that have been set in the wrapped response and now incorrectly returns null for those header values if they have not been set directly in the CCRW. This commit fixes this regression as follows. - The Content-Type and Content-Length headers set in the wrapped response are honored in getContentType(), containsHeader(), getHeader(), and getHeaders() unless those headers have been set directly in the CCRW. - In copyBodyToResponse(), the Content-Type in the wrapped response is only overridden if the Content-Type has been set directly in the CCRW. Furthermore, prior to this commit, getHeaderNames() returned duplicates for the Content-Type and Content-Length headers if they were set in the wrapped response as well as in CCRW. This commit fixes that by returning a unique set from getHeaderNames(). This commit also updates ContentCachingResponseWrapperTests to verify the expected behavior for Content-Type and Content-Length headers that are set in the wrapped response as well as in CCRW. See gh-32039 See gh-32317 Closes gh-32322

sbrannen · 2024-02-25T17:22:30Z

@yikuo123, @amuttsch, @tjalling-ran, and anybody else interested in helping out...

The proposed fix is now available in 6.1.5-SNAPSHOT builds and will be available in upcoming 6.0.18-SNAPSHOT and 5.3.33-SNAPSHOT builds.

As mentioned in #32317 (comment), we'd be grateful if you can test against the snapshots and report back here to let us know if the fixes work for you.

Thanks!

Sam

p.s. In the interim, we are leaving this issue "open" until we receive confirmations that the fixes work.

tjalling-ran · 2024-02-25T19:59:03Z

@sbrannen I tried the 5.3.33-SNAPSHOT build shown in the screenshot below. It still has the issue - that is, there is no ContentType header in the response of the GET-request I tested with, whereas the same request on 5.3.29 does result in a response with the expected ContentType.

(screenshot of https://repo.spring.io/ui/native/snapshot/org/springframework/spring-framework-bom/5.3.33-SNAPSHOT/)

Judging from the decompiled class in IntelliJ, the snapshot build already has the fix:

tjalling-ran · 2024-02-26T00:17:35Z

I may have found the issue. (Though I'm not familiar enough with the code to be fully confident that I didn't miss anything.)

As you know, ConditionalContentCachingResponseWrapper.setContentType() recently started setting its own ContentType instead of delegating to the wrapped Response.

As long as ConditionalContentCachingResponseWrapper.copyBodyToResponse() does indeed do copying, this is fine, as it will copy its ContentType to the wrapped response. However, when ConditionalContentCachingResponseWrapper.content.size() is 0, no copying is done.

So then the question is: how can ConditionalContentCachingResponseWrapper.content.size() be 0? The answer is that ConditionalContentCachingResponseWrapper.getOutputStream() only returns its own output stream if content caching is enabled. If content caching is disabled, it will delegate to the wrapped response's output stream. Thus, ConditionalContentCachingResponseWrapper will not have any content.

It will not delegate to the wrapped response's setContentType(), however (as stated above). So now we have a situation where the content is stored in the wrapped response, but the content type is stored in the wrapper.

And because a wrapper without content doesn't copy any of its own properties to the wrapped response, the wrapped response's content type (and possibly other properties) will remain null.

yikuo123 · 2024-02-26T02:01:26Z

I've pushed a potential fix to 6.1.x and main (see commit 5680d20).

@yikuo123, @amuttsch, @tjalling-ran, and anybody else interested in helping out...

We would be grateful if you could try out one of the upcoming 6.1.5-SNAPSHOT builds and let us know if the proposed fix addresses the regressions you've encountered!

If you confirm the fix works, we'll then backport to 6.0.x and 5.3.x.

Cheers,

Sam

p.s. you can find information on accessing snapshot builds here.

Thank you very much!

I tried the 6.1.5-SNAPSHOT build and the fix works.

amuttsch · 2024-02-26T08:30:54Z

@yikuo123, @amuttsch, @tjalling-ran, and anybody else interested in helping out...

The proposed fix is now available in 6.1.5-SNAPSHOT builds and will be available in upcoming 6.0.18-SNAPSHOT and 5.3.33-SNAPSHOT builds.

As mentioned in #32317 (comment), we'd be grateful if you can test against the snapshots and report back here to let us know if the fixes work for you.

Thanks!

Sam

p.s. In the interim, we are leaving this issue "open" until we receive confirmations that the fixes work.

I tried the 6.1.5-SNAPSHOT but it did not work, the Content-Type header is still missing. To rule out any configuration error on my side: I added the repository

repositories {
  maven { url = uri("https://repo.spring.io/snapshot") }
}

and overwrote the version using ext["spring-framework.version"] = "6.1.5-SNAPSHOT". At least IntelliJ shows me that the ShallowEtagHeaderFilter was loaded from spring-web-6.1.5-SNAPSHOT and I synced, cleared and rebuild my gradle project.

amuttsch · 2024-02-26T09:40:08Z

I created a reproducible setup here: https://github.com/amuttsch/spring-boot-etag-content-header

sbrannen · 2024-02-26T18:14:23Z

Hi everybody,

Thanks a lot for all of the prompt feedback!

In the end, we decided to revert the Content-Type caching in ContentCachingResponseWrapper.

So, hopefully that will address all remaining issues. 🤞

See cca440e for details.

The latest fix is now available in 6.1.5-SNAPSHOT builds.

So, if you're using 6.1.x, please try out the snapshot and let us know if the latest fix addresses the regressions you've encountered.

Once the fix is confirmed by a number of you, we'll then backport to 6.0.x and 5.3.x.

Cheers,

Sam

amuttsch · 2024-02-26T18:24:42Z

Thanks for the quick response. My demo code works now, I can check my production code tomorrow. But I think it will most likely work too.

sbrannen · 2024-02-26T18:31:00Z

Thanks for the quick response.

You're very welcome.

My demo code works now,

Awesome! Thanks for testing that with lightning fast speed and reporting back. 🚀

I can check my production code tomorrow. But I think it will most likely work too.

That would be much appreciated.

Thanks

resmo · 2024-02-26T18:45:04Z

Just for the record: as mentioned in spring-projects/spring-boot#39745 (comment): spring-core-6.1.5-20240226.173247-18.jar fixes my issue, umlauts are shown correctly (content-length size is as it was in 6.1.3)

Thanks!

amuttsch · 2024-02-27T08:40:28Z

I can check my production code tomorrow. But I think it will most likely work too.

That would be much appreciated.

My production code works as well 👍

sbrannen · 2024-02-27T09:02:56Z

@amuttsch and @resmo, thanks for confirming that the latest snapshots work! 👍

I'll likely backport to 6.0.x and 5.3.x tomorrow.

Based on feedback from several members of the community, we have decided to revert the caching of the Content-Type header that was introduced in ContentCachingResponseWrapper in 375e0e6. This commit therefore completely removes Content-Type caching in ContentCachingResponseWrapper and updates the existing tests accordingly. To provide guards against future regressions in this area, this commit also introduces explicit tests for the 6 ways to set the content length in ContentCachingResponseWrapper and modifies a test in ShallowEtagHeaderFilterTests to ensure that a Content-Type header set directly on ContentCachingResponseWrapper is propagated to the underlying response even if content caching is disabled for the ShallowEtagHeaderFilter. See gh-32039 See gh-32317 Closes gh-32321

Based on feedback from several members of the community, we have decided to revert the caching of the Content-Type header that was introduced in ContentCachingResponseWrapper in 375e0e6. This commit therefore completely removes Content-Type caching in ContentCachingResponseWrapper and updates the existing tests accordingly. To provide guards against future regressions in this area, this commit also introduces explicit tests for the 6 ways to set the content length in ContentCachingResponseWrapper and modifies a test in ShallowEtagHeaderFilterTests to ensure that a Content-Type header set directly on ContentCachingResponseWrapper is propagated to the underlying response even if content caching is disabled for the ShallowEtagHeaderFilter. See gh-32039 See gh-32317 Closes gh-32322

sbrannen · 2024-02-28T10:21:06Z

The fix has now been backported to 6.0.x and 5.3.x and will be available in upcoming 6.0.18-SNAPSHOT and 5.3.33-SNAPSHOT builds.

If you can test your applications against those snapshots and report back here, that would be great.

Thanks,

Sam

tjalling-ran · 2024-02-28T17:28:36Z

I can confirm that the issue is fixed in today's 5.3.33-SNAPSHOT build 😎

Thank you for the prompt fix @sbrannen.

sbrannen · 2024-02-28T17:30:01Z

Glad to hear it, @tjalling-ran!

And thanks for letting us know.

pperalta · 2024-03-01T17:00:04Z

Just tested with 5.3.33-SNAPSHOT, works for us too!

sbrannen · 2024-03-01T17:20:27Z

Thanks for letting us know it works for you with the 5.3.33 snapshots, @pperalta!

spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Feb 23, 2024

jhoeller assigned poutsma Feb 23, 2024

jhoeller added the in: web Issues in web modules (web, webmvc, webflux, websocket) label Feb 23, 2024

jhoeller added type: regression A bug that is also a regression and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels Feb 23, 2024

jhoeller added this to the 6.1.5 milestone Feb 23, 2024

jhoeller changed the title ~~ContentCachingResponseWrapper return null Content-Type and Content-Length~~ ContentCachingResponseWrapper does not return Content-Type and Content-Length anymore Feb 23, 2024

jhoeller added the for: backport-to-6.0.x label Feb 23, 2024

github-actions bot added status: backported An issue that has been backported to maintenance branches and removed for: backport-to-6.0.x labels Feb 23, 2024

github-actions bot mentioned this issue Feb 23, 2024

ContentCachingResponseWrapper no longer honors Content-Type and Content-Length #32321

Closed

jhoeller added the for: backport-to-5.3.x label Feb 23, 2024

github-actions bot removed the for: backport-to-5.3.x label Feb 23, 2024

github-actions bot mentioned this issue Feb 23, 2024

ContentCachingResponseWrapper no longer honors Content-Type and Content-Length #32322

Closed

sbrannen self-assigned this Feb 23, 2024

sbrannen changed the title ~~ContentCachingResponseWrapper does not return Content-Type and Content-Length anymore~~ ContentCachingResponseWrapper no longer returns Content-Type and Content-Length Feb 23, 2024

This comment was marked as outdated.

Sign in to view

sbrannen unassigned poutsma Feb 23, 2024

sbrannen closed this as completed in 5680d20 Feb 24, 2024

resmo mentioned this issue Feb 25, 2024

[3.2.3] broken umlauts in htmx req/resp spring-projects/spring-boot#39745

Closed

This comment was marked as outdated.

Sign in to view

resmo mentioned this issue Feb 26, 2024

spring boot 3.2.3: umlaut encoding issue? wimdeblauwe/htmx-spring-boot#99

Closed

sbrannen closed this as completed in cca440e Feb 26, 2024

wilkinsona mentioned this issue Mar 1, 2024

in spring boot 3.2.3 ContentCachingResponseWrapper is not reading content-type and content-length spring-projects/spring-boot#39818

Closed

snicoll mentioned this issue Mar 4, 2024

Regression in SpringWeb 6.1.4 causes ContentType to be missing if ShallowEtagHeaderFilter is used and HTTP response is streamed #32363

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ContentCachingResponseWrapper no longer honors Content-Type and Content-Length #32317

ContentCachingResponseWrapper no longer honors Content-Type and Content-Length #32317

yikuo123 commented Feb 23, 2024 •

edited by sbrannen

Loading

yikuo123 commented Feb 23, 2024

amuttsch commented Feb 23, 2024

jhoeller commented Feb 23, 2024

tjalling-ran commented Feb 23, 2024

amuttsch commented Feb 23, 2024 •

edited by sbrannen

Loading

sbrannen commented Feb 23, 2024

This comment was marked as outdated.

sbrannen commented Feb 24, 2024

sbrannen commented Feb 25, 2024

This comment was marked as outdated.

sbrannen commented Feb 25, 2024

tjalling-ran commented Feb 25, 2024 •

edited

Loading

tjalling-ran commented Feb 26, 2024 •

edited

Loading

yikuo123 commented Feb 26, 2024

amuttsch commented Feb 26, 2024

amuttsch commented Feb 26, 2024

sbrannen commented Feb 26, 2024

amuttsch commented Feb 26, 2024

sbrannen commented Feb 26, 2024

resmo commented Feb 26, 2024

amuttsch commented Feb 27, 2024

sbrannen commented Feb 27, 2024

sbrannen commented Feb 28, 2024

tjalling-ran commented Feb 28, 2024

sbrannen commented Feb 28, 2024

pperalta commented Mar 1, 2024

sbrannen commented Mar 1, 2024

ContentCachingResponseWrapper no longer honors Content-Type and Content-Length #32317

ContentCachingResponseWrapper no longer honors Content-Type and Content-Length #32317

Comments

yikuo123 commented Feb 23, 2024 • edited by sbrannen Loading

yikuo123 commented Feb 23, 2024

amuttsch commented Feb 23, 2024

jhoeller commented Feb 23, 2024

tjalling-ran commented Feb 23, 2024

amuttsch commented Feb 23, 2024 • edited by sbrannen Loading

sbrannen commented Feb 23, 2024

This comment was marked as outdated.

sbrannen commented Feb 24, 2024

sbrannen commented Feb 25, 2024

This comment was marked as outdated.

sbrannen commented Feb 25, 2024

tjalling-ran commented Feb 25, 2024 • edited Loading

tjalling-ran commented Feb 26, 2024 • edited Loading

yikuo123 commented Feb 26, 2024

amuttsch commented Feb 26, 2024

amuttsch commented Feb 26, 2024

sbrannen commented Feb 26, 2024

amuttsch commented Feb 26, 2024

sbrannen commented Feb 26, 2024

resmo commented Feb 26, 2024

amuttsch commented Feb 27, 2024

sbrannen commented Feb 27, 2024

sbrannen commented Feb 28, 2024

tjalling-ran commented Feb 28, 2024

sbrannen commented Feb 28, 2024

pperalta commented Mar 1, 2024

sbrannen commented Mar 1, 2024

yikuo123 commented Feb 23, 2024 •

edited by sbrannen

Loading

amuttsch commented Feb 23, 2024 •

edited by sbrannen

Loading

tjalling-ran commented Feb 25, 2024 •

edited

Loading

tjalling-ran commented Feb 26, 2024 •

edited

Loading