ServletWebRequest.checkNotModified(…) writes Last-Modified header in invalid format [SPR-13090]

[spring-projects-issues]

Oliver Drotbohm opened SPR-13090 and commented

As of version 4.2 RC1 ServletWebRequest has a checkNotModified(String, long) method, that – unexpectedly – sets headers (the method name does not imply that). Even worse, it calls setDateHeader(…) on the response, which writes the given timestamp as is. This is wrong as per HTTP spec the value has to be a formatted date.

So if you set a Last-Modified header in a controller and return a ResponseEntity, the just mentioned check adds an invalid header to the response and later on adds the actually correct value as second value for that parameter.

Trying to access the header value then finally fails with an execption as HttpHeaders.getLastModified() tries to parse the first value found into a Date which it can't because of the invalid format.

I'd argue:

• a check… method that changes the state of the object is weird
• why does that method set headers at all? If the header in question (Last-Modified in this case) is present it will be added later on by definition. Why create a second code path that writes headers and bypasses the code paths making sure headers are in the right format

I'd expect a single header value to be present in the response in the correct format.

The issue can be verified running https://github.com/spring-projects/spring-data-examples/tree/master/rest/headers and changing the Spring version to be used to anything 4.2.

---

Affects: 4.2 RC1

Issue Links:

• ETag/If-None-Match logic in HttpEntityMethodProcessor should not affect methods other than HTTP GET [SPR-13496] #18074 ETag/If-None-Match logic in HttpEntityMethodProcessor should not affect methods other than HTTP GET
• ServletWebRequest.checkNotModified attempts to call HttpServletResponse.getStatus() on Servlet 2.5 [SPR-13396] #17976 ServletWebRequest.checkNotModified attempts to call HttpServletResponse.getStatus() on Servlet 2.5

Referenced from: commits dba46c1, 39d689d, a421bd2, e2c8d37, 0175068

0 votes, 5 watchers

[spring-projects-issues]

Brian Clozel commented

Hey Oliver Drotbohm,

checkNotModified(String, long) is actually a new variant of existing methods checkNotModified(String) and checkNotModified(long), introduced in #15948.
The problems you describe all predate that method, but still need to be fixed:

• the date format is indeed wrong for "Last-Modified" and "Expires" headers
• the methods themselves mutate the HTTP response status and headers even though their name does not suggest that

I've renamed those methods request.processConditional and fixed the date format in all related places - those changes are now living in a private branch here.

I'm not quite sure about one part though; since #15948, HTTP response headers "ETag" / "Last-Modified" are also added for HTTP 200 responses, which is required by the spec. This is done at the ServletWebRequest level, but I'm wondering if this should actually be done by the caller itself. This can be useful for projects (such as spring-data-rest) that want full control over the response, but not so practical for Spring Framework users, who'd need to manually set those headers in Controllers, whereas they just called this very method right before...

I'd be happy to discuss those changes with you and Rossen Stoyanchev.

[spring-projects-issues]

Oliver Drotbohm commented

Couldn't the automatic addition of the header values be guarded by a check whether they're contained in what's returned from the controller? And if so, skip the addition step? I think that if a controller manually sets these values, the values from the controller should be the ones that end up in the request.

The way the suggested commit looks to me, I'd still end up with two values in the response for the Last-Modified and ETag headers, wouldn't I? Also ServletWebRequest seems to duplicate what's implemented in HttpHeaders.setDate(String, long). Wouldn't it be better to reuse the logic from the value object?

[spring-projects-issues]

Brian Clozel commented

This is now fixed and has been validated with a spring-data-rest test.

[spring-projects-issues]

Oliver Drotbohm commented

I still see the header added twice:

MockHttpServletResponse:
              Status = 200
       Error message = null
             Headers = {ETag=["0", "0"], Last-Modified=[Sat, 13 Jun 2015 17:19:50 GMT, Sat, 13 Jun 2015 17:19:50 GMT], Content-Type=[application/hal+json]}
        Content type = application/hal+json
                Body = {
  "firstname" : "Dave",
  "lastname" : "Matthews",
  "gender" : "MALE",
  "address" : {
    "street" : "4711 Some Place",
    "zipCode" : "54321",
    "city" : "Charlottesville",
    "state" : "VA"
  },
  "_links" : {
    "self" : {
      "href" : "http://localhost:8080/customers/1"
    }
  }
}

[spring-projects-issues]

Brian Clozel commented

Reopening as duplicate headers are still being written to the response

[spring-projects-issues]

Oliver Drotbohm commented

Confirmed fixed! \o/ Thanks for your patience. :)

[spring-projects-issues]

Oliver Drotbohm commented

I am still running into a regression with this: Spring Data RESTs MongoWebTests fail to see the ETag in a NotModified response. This seems to be caused by this change. The controller returns a ResponseEntity with ETag and Last-Modified headers. But exactly those are skipped after the instanceof check. The following ifResourceNotModified(…) check returns false. I don't quite get why, as I'd expect it to simply check the HTTP status code but it seems to be messing with the headers, too. The else branch is skipped and I directly run into writeWithMessageConverters(…) without my custom ETag and Last-Modified headers written.

[spring-projects-issues]

Oliver Drotbohm commented

I've altered the headers example we have for Spring Data REST to check for the presence of the headers to show the failure in this branch.

[spring-projects-issues]

Brian Clozel commented

Here is a new fix for this issue.

Oliver Drotbohm, all your tests pass with that one, and I've basically cleared the relationship between ServletWebRequest and HttpEntityMethodProcessor. The only downside is a bit of code duplication, but quite manageable.

Thanks Arjen Poutsma - discussing this issue with you probably avoided further back and forth between framework and data-rest...
I'll let the other team members review this proposal, as we're quite close to the RC2 release - ping Arjen Poutsma, Rossen Stoyanchev, Sébastien Deleuze.

[spring-projects-issues]

Arjen Poutsma commented

Looks good to me.

[spring-projects-issues]

Brian Clozel commented

This is merged and available in the latest SNAPSHOTs.
Oliver Drotbohm, 4.2.RC2 is almost there - hopefully everything runs smoothly this time.

[spring-projects-issues]

Oliver Drotbohm commented

Looks good to me now: https://travis-ci.org/spring-projects/spring-data-examples/builds/68832926

[spring-projects-issues]

Florent Bonamis commented

We have a problem with this bug fixed in our web application.
As @Brian Clozel said, this solution would only work for Servlet 3.x and we are still using Tomcat 6 with Servlet 2.5

[spring-projects-issues]

Brian Clozel commented

Hi Florent Bonamis,

Could you elaborate?
I don't think this fix is using any Servlet 3.x specific API? Could you explain the issue with a stacktrace or a repro project?

Thanks,