Affects: 6.0.8
Problem: ServletWebRequest::validateIfMatch(String eTag) compares etag of post-commit state with etag in request, which necessarily is pre-commit state, and sets response status to 412 if they don't match (which is the normal use case for PUTs). For PUTs, validation should occur prior to performing the method, and then need not occur after.
Additional details: I have a filter that extends ShallowEtagHeaderFilter, overriding isEligibleForEtag(...) and generateETagHeaderValue(...). This worked fine with spring 5.x, but now is broken due to changes in https://github.com/spring-projects/spring-framework/commit/0783f0762d72df70572aaceeee099c3ec4b7e019.
Thank you.
Comment From: bclozel
Hello @larsonmp could you elaborate a bit on this issue? Does the problem happen in controllers as well or only in your custom filter? Could you share a sample application that shows the issue and share the request to use (e.g. a curl command) and the response you would expect?
Thanks!
Comment From: larsonmp
Hello @bclozel.
The problem doesn't happen in my custom filter or in controllers, it happens in ServletWebRequest. Here's a smallish example.
@RestController
@RequestMapping(value = "/api/v1/games")
public class GameController {
private ETagUtil eTagUtil;
private GameService gameService;
private GameMapper gameMapper = GameMapper.INSTANCE;
public GameController(ETagUtil eTagUtil, GameService gameService) {
this.eTagUtil = eTagUtil;
this.gameService = gameService;
}
@RequestMapping(method = RequestMethod.POST)
public ResponseEntity<Game> create(@RequestBody @Valid Game dto) {
com.example.model.Game model = gameService.create(toModel(dto));
return ResponseEntity.
status(HttpStatus.CREATED).
header(HttpHeaders.LOCATION, "/api/v1/games/" + model.getId()).
body(toDTO(model));
}
@RequestMapping(value = "/{id}", method = RequestMethod.PUT)
public ResponseEntity<Game> update(
@PathVariable String id,
@RequestHeader(value = "If-Match") String etag,
@RequestBody @Valid Game dto) {
if (eTagIsValid(id, etag)) {
return ResponseEntity.ok(toDTO(gameService.update(id, toModel(dto))));
} else {
return ResponseEntity.status(HttpStatus.PRECONDITION_FAILED).build();
}
}
@RequestMapping(value = "/{id}", method = RequestMethod.GET)
@ResponseBody
public ResponseEntity<Game> read(@PathVariable String id) {
return ResponseEntity.ok(toDTO(gameService.read(id)));
}
private Game toDTO(com.example.model.Game model) {
return gameMapper.map(model);
}
private com.example.model.Game toModel(Game dto) {
return gameMapper.map(dto);
}
private boolean eTagIsValid(String id, String eTag) {
return eTagUtil.eTagMatches(toDTO(gameService.read(id)), eTag);
}
}
@Component
public class SampleETagHeaderFilter extends ShallowEtagHeaderFilter {
private ETagUtil eTagUtil;
public SampleETagHeaderFilter(ETagUtil eTagUtil) {
this.eTagUtil = eTagUtil;
}
@Override
protected boolean isEligibleForEtag(@Nonnull HttpServletRequest request, @Nonnull HttpServletResponse response, int responseStatusCode, @Nonnull InputStream inputStream) {
String method = request.getMethod();
return eTagUtil.canHandle(inputStream) && 200 <= responseStatusCode && responseStatusCode < 300 &&
( HttpMethod.GET.matches(method) || HttpMethod.PUT.matches(method) );
}
@Nonnull
@Override
protected String generateETagHeaderValue(@Nonnull InputStream inputStream, boolean isWeak) throws IOException {
return eTagUtil.eTag(StreamUtils.copyToString(inputStream, Charset.defaultCharset()));
}
}
@Component
public class ETagUtil {
private final ObjectMapper objectMapper;
public ETagUtil(ObjectMapper objectMapper) {
this.objectMapper = objectMapper;
}
public boolean canHandle(@Nonnull InputStream stream) {
try {
objectMapper.writeValueAsBytes(StreamUtils.copyToString(stream, Charset.defaultCharset()));
} catch (Exception e) {
return false;
}
return true;
}
public boolean eTagMatches(@Nonnull Game game, @Nonnull String eTag) {
return eTag.equals(eTag(game));
}
public String eTag(Game game) {
return "\"" + sha256(game) + "\"";
}
private String sha256(Object object) {
try {
byte[] bytes = objectMapper.writeValueAsBytes(object);
byte[] digest = MessageDigest.getInstance("SHA-256").digest(bytes);
return String.valueOf(Hex.encode(digest));
} catch (Exception e) {
throw new RuntimeException("unable to generate eTag", e);
}
}
public String eTag(String json) {
return "\"" + sha256(json) + "\"";
}
private String sha256(String json) {
try {
byte[] digest = MessageDigest.getInstance("SHA-256").digest(json.getBytes(StandardCharsets.UTF_8));
return String.valueOf(Hex.encode(digest));
} catch (NoSuchAlgorithmException e) {
throw new RuntimeException("unable to initialize hash algorithm", e);
}
}
}
For an update (PUT), the eTag in the If-Match header is checked against the current value pulled from the database in the controller before proceeding with the update.
The filter updates the value of the ETag header for GETs and PUTs, based on the current value (for GETs, that's the preexisting value, for PUTs that's the updated/committed version).
Here is a series of cURL commands with response snippets that illustrate expected behavior:
% curl "http://localhost:8080/api/v1/games" --header 'content-type: application/json' --data '{"title": "botw"}' -i
HTTP/1.1 201
Location: /api/v1/games/913395ce-37a3-46b9-b0af-97054eac4f14
{"title":"botw"}
% curl "http://localhost:8080/api/v1/games/913395ce-37a3-46b9-b0af-97054eac4f14" -i
HTTP/1.1 200
ETag: "e9cb013818bdcc39def0848da8064d12809b67ac313b6a4ac9edd2ea8db49d7f"
{"title":"botw"}
% curl -X PUT "http://localhost:8080/api/v1/games/913395ce-37a3-46b9-b0af-97054eac4f14" --header "If-Match: \"wrong-eTag\"" --header 'content-type: application/json' --data-raw '{"title": "totk"}' -i
HTTP/1.1 412
% curl -X PUT "http://localhost:8080/api/v1/games/913395ce-37a3-46b9-b0af-97054eac4f14" --header "If-Match: \"e9cb013818bdcc39def0848da8064d12809b67ac313b6a4ac9edd2ea8db49d7f\"" --header 'content-type: application/json' --data-raw '{"title": "totk"}' -i
HTTP/1.1 200
ETag: "7cae910c5ff9ce3003426793b8772bd63828e6e7154c38b77a0b1fc761af3eb1"
{"title":"totk"}
However, ServletWebRequest now prevents this approach. Consider this snippet:
@Override
public boolean checkNotModified(@Nullable String eTag, long lastModifiedTimestamp) {
HttpServletResponse response = getResponse();
if (this.notModified || (response != null && HttpStatus.OK.value() != response.getStatus())) {
return this.notModified;
}
// Evaluate conditions in order of precedence.
// See https://datatracker.ietf.org/doc/html/rfc9110#section-13.2.2
if (validateIfMatch(eTag)) {
updateResponseStateChanging(eTag, lastModifiedTimestamp);
return this.notModified;
}
// 2) If-Unmodified-Since
else if (validateIfUnmodifiedSince(lastModifiedTimestamp)) {
updateResponseStateChanging(eTag, lastModifiedTimestamp);
return this.notModified;
}
// 3) If-None-Match
if (!validateIfNoneMatch(eTag)) {
// 4) If-Modified-Since
validateIfModifiedSince(lastModifiedTimestamp);
}
updateResponseIdempotent(eTag, lastModifiedTimestamp);
return this.notModified;
}
private boolean validateIfMatch(@Nullable String eTag) {
if (SAFE_METHODS.contains(getRequest().getMethod())) {
return false;
}
Enumeration<String> ifMatchHeaders = getRequest().getHeaders(HttpHeaders.IF_MATCH);
if (!ifMatchHeaders.hasMoreElements()) {
return false;
}
this.notModified = matchRequestedETags(ifMatchHeaders, eTag, false);
return true;
}
For a PUT, eTag is based on the value of the newly committed entity, but If-Match is going to have the value of the original entity, so that won't match unless there was no change to the entity. As a result, even a PUT with the correct value in the If-Match header results in a 412 response. I can't think of a use case where we'd want to compare the new eTag to the value of If-Match for a PUT.
Does that make sense?
Comment From: bclozel
Thanks for the detailed response. I managed to turn that into a sample application. There are two issues we need to consider here.
First, the ShallowEtagHeaderFilter offers a limited strategy for HTTP caching, because it's based on the content of the HTTP response. This has no knowledge of the actual entity backing the REST resource. As explained in the reference documentation for the ETag filter, this merely saves bandwidth but not CPU. I think we should highlight even more the limitations of the filter in the documentation. This filter should only be involved with safe HTTP methods that have a body (this means: only "GET") as it has no knowledge about the resource being served. With that in mind, I think that overriding isEligibleForEtag and allowing "PUT" methods is a bug in your implementation.
Second, I think that the use case you've shown in the sample doesn't match the typical usage for the ETag header filter. I think this should be better covered by direct caching integration in your controllers:
@PutMapping( "/{id}")
public ResponseEntity<Game> update(
@PathVariable String id,
WebRequest request,
@RequestBody Game dto) {
String eTag = eTagUtil.eTag(gameService.read(id));
if (request.checkNotModified(eTag)) {
return null;
}
return ResponseEntity.ok(gameService.update(id, dto));
}
There, we have both access to the resource being updated and its new representation.
I am going to turn this issue into a documentation improvement to better warn developers against the limitations of the filter approach. Thanks for raising this issue!
Comment From: larsonmp
@bclozel - Thanks for detailed explanation. I can make due with an implementation that eschews the filter for an in-controller solution.
One thing that still confuses me is your view on the "typical usage" of ETag header filter, and that might be because you're coming at it from a cache-control/bandwidth-saving perspective and I'm coming at it from a data-loss/accidental-overwrite perspective. It sounds like this filter is designed to be used with GETs and HEADs only, but the If-Match header is designed to be used with POSTs, PUTs, and DELETEs. From the RFC:
If-Match is most often used with state-changing methods (e.g., POST, PUT, DELETE) to prevent accidental overwrites when multiple user agents might be acting in parallel on the same resource (i.e., to prevent the "lost update" problem). In general, it can be used with any method that involves the selection or modification of a representation to abort the request if the selected representation's current entity tag is not a member within the If-Match field value. So I guess I don't understand why the filter checks the value of
If-Matchat all. What am I missing here?
Comment From: bclozel
Yes you're right, the If-Match header is only meant to be used with state changing methods. It's just that we're using WebRequest.checkNotModified in the Servlet filter to avoid duplicating a lot of code.
Comment From: larsonmp
Oh, ok. Thank you.