From 72cffaf4405682d76e52dddfce57137b4a9cd395 Mon Sep 17 00:00:00 2001
From: Remi Tricot-Le Breton <rlebreton@haproxy.com>
Date: Thu, 3 Dec 2020 18:19:31 +0100
Subject: [PATCH] MEDIUM: cache: Remove cache entry in case of POST on the same
 resource

In case of successful unsafe method on a stored resource, the cached entry
must be invalidated (see RFC7234#4.4).
A "non-error response" is one with a 2xx (Successful) or 3xx (Redirection)
status code.
This implies that the primary hash must now be calculated on requests
that have an unsafe method (POST or PUT for instance) so that we can
disable the corresponding entries when we process the response.
---
 reg-tests/cache/post_on_entry.vtc | 66 +++++++++++++++++++++++++++++++
 src/cache.c                       | 45 +++++++++++++++------
 2 files changed, 99 insertions(+), 12 deletions(-)
 create mode 100644 reg-tests/cache/post_on_entry.vtc

diff --git a/reg-tests/cache/post_on_entry.vtc b/reg-tests/cache/post_on_entry.vtc
new file mode 100644
index 000000000..2b807f693
--- /dev/null
+++ b/reg-tests/cache/post_on_entry.vtc
@@ -0,0 +1,66 @@
+varnishtest "A successful unsafe method (POST for instance) on a cached entry must disable it."
+
+#REQUIRE_VERSION=2.3
+
+feature ignore_unknown_macro
+
+server s1 {
+       rxreq
+       expect req.url == "/cached"
+       txresp -hdr "Cache-Control: max-age=5" \
+	    -bodylen 150
+
+       rxreq
+       expect req.url == "/cached"
+       expect req.method == "POST"
+       txresp
+
+       rxreq
+       expect req.url == "/cached"
+       txresp -hdr "Cache-Control: max-age=5" \
+	    -bodylen 100
+
+} -start
+
+haproxy h1 -conf {
+       defaults
+               mode http
+               ${no-htx} option http-use-htx
+               timeout connect 1s
+               timeout client  1s
+               timeout server  1s
+
+       frontend fe
+               bind "fd@${fe}"
+               default_backend test
+
+       backend test
+               http-request cache-use my_cache
+               server www ${s1_addr}:${s1_port}
+               http-response cache-store my_cache
+               http-response set-header X-Cache-Hit %[res.cache_hit]
+
+       cache my_cache
+               total-max-size 3
+               max-age 20
+               max-object-size 3072
+} -start
+
+
+client c1 -connect ${h1_fe_sock} {
+       txreq -url "/cached"
+       rxresp
+       expect resp.status == 200
+       expect resp.bodylen == 150
+
+       txreq -method "POST" -url "/cached" -bodylen 100
+       rxresp
+       expect resp.status == 200
+       expect resp.http.X-Cache-Hit == 0
+
+       txreq -url "/cached"
+       rxresp
+       expect resp.status == 200
+       expect resp.bodylen == 100
+       expect resp.http.X-Cache-Hit == 0
+} -run
diff --git a/src/cache.c b/src/cache.c
index 59a590cb7..e0560e57e 100644
--- a/src/cache.c
+++ b/src/cache.c
@@ -753,8 +753,34 @@ enum act_return http_action_store_cache(struct act_rule *rule, struct proxy *px,
 		goto out;
 
 	/* cache only GET method */
-	if (txn->meth != HTTP_METH_GET)
+	if (txn->meth != HTTP_METH_GET) {
+		/* In case of successful unsafe method on a stored resource, the
+		 * cached entry must be invalidated (see RFC7234#4.4).
+		 * A "non-error response" is one with a 2xx (Successful) or 3xx
+		 * (Redirection) status code. */
+		if (txn->status >= 200 && txn->status < 400) {
+			switch (txn->meth) {
+			case HTTP_METH_OPTIONS:
+			case HTTP_METH_GET:
+			case HTTP_METH_HEAD:
+			case HTTP_METH_TRACE:
+				break;
+
+			default: /* Any unsafe method */
+				/* Discard any corresponding entry in case of sucessful
+				 * unsafe request (such as PUT, POST or DELETE). */
+				shctx_lock(shctx);
+
+				old = entry_exist(cconf->c.cache, txn->cache_hash);
+				if (old) {
+					eb32_delete(&old->eb);
+					old->eb.key = 0;
+				}
+				shctx_unlock(shctx);
+			}
+		}
 		goto out;
+	}
 
 	/* cache key was not computed */
 	if (!key)
@@ -1329,15 +1355,6 @@ int sha1_hosturi(struct stream *s)
 	trash = get_trash_chunk();
 	ctx.blk = NULL;
 
-	switch (txn->meth) {
-	case HTTP_METH_HEAD:
-	case HTTP_METH_GET:
-		chunk_memcat(trash, "GET", 3);
-		break;
-	default:
-		return 0;
-	}
-
 	sl = http_get_stline(htx);
 	uri = htx_sl_req_uri(sl); // whole uri
 	if (!uri.len)
@@ -1475,10 +1492,14 @@ enum act_return http_action_req_cache_use(struct act_rule *rule, struct proxy *p
 
 	http_check_request_for_cacheability(s, &s->req);
 
-	if ((s->txn->flags & (TX_CACHE_IGNORE|TX_CACHEABLE)) == TX_CACHE_IGNORE)
+	/* The request's hash has to be calculated for all requests, even POSTs
+	 * or PUTs for instance because RFC7234 specifies that a sucessful
+	 * "unsafe" method on a stored resource must invalidate it
+	 * (see RFC7234#4.4). */
+	if (!sha1_hosturi(s))
 		return ACT_RET_CONT;
 
-	if (!sha1_hosturi(s))
+	if ((s->txn->flags & (TX_CACHE_IGNORE|TX_CACHEABLE)) == TX_CACHE_IGNORE)
 		return ACT_RET_CONT;
 
 	if (s->txn->flags & TX_CACHE_IGNORE)