From ca4fd73938f3eeb7bb81bc85d49b3076e2e26178 Mon Sep 17 00:00:00 2001
From: Remi Tricot-Le Breton <rlebreton@haproxy.com>
Date: Tue, 4 Jul 2023 17:13:28 +0200
Subject: [PATCH] BUG/MINOR: cache: A 'max-age=0' cache-control directive can
 be overriden by a s-maxage

When a s-maxage cache-control directive is present, it overrides any
other max-age or expires value (see section 5.2.2.9 of RFC7234). So if
we have a max-age=0 alongside a strictly positive s-maxage, the response
should be cached.

This bug was raised in GitHub issue #2203.
The fix can be backported to all stable branches.
---
 reg-tests/cache/caching_rules.vtc | 64 +++++++++++++++++++++++++++++++
 src/http_ana.c                    | 23 +++++++++--
 2 files changed, 84 insertions(+), 3 deletions(-)

diff --git a/reg-tests/cache/caching_rules.vtc b/reg-tests/cache/caching_rules.vtc
index 61274b4b5..a488875b3 100644
--- a/reg-tests/cache/caching_rules.vtc
+++ b/reg-tests/cache/caching_rules.vtc
@@ -79,6 +79,30 @@ server s1 {
     txresp -hdr "Cache-Control: max-age=500" \
         -hdr "Age: 100" -bodylen 140
 
+
+    # max-age=0
+    rxreq
+    expect req.url == "/maxage_zero"
+    txresp -hdr "Cache-Control: max-age=0" \
+        -bodylen 150
+
+    rxreq
+    expect req.url == "/maxage_zero"
+    txresp -hdr "Cache-Control: max-age=0" \
+        -bodylen 150
+
+    # Overridden null max-age
+    rxreq
+    expect req.url == "/overridden"
+    txresp -hdr "Cache-Control: max-age=1, s-maxage=5" \
+        -bodylen 160
+
+    rxreq
+    expect req.url == "/overridden_null_maxage"
+    txresp -hdr "Cache-Control: max-age=0, s-maxage=5" \
+        -bodylen 190
+
+
 } -start
 
 server s2 {
@@ -252,5 +276,45 @@ client c1 -connect ${h1_fe_sock} {
         expect resp.bodylen == 140
         expect resp.http.X-Cache-Hit == 1
 
+        # max-age=0 (control test for the overridden null max-age test below)
+        txreq -url "/maxage_zero"
+        rxresp
+        expect resp.status == 200
+        expect resp.bodylen == 150
+        expect resp.http.X-Cache-Hit == 0
+
+        txreq -url "/maxage_zero"
+        rxresp
+        expect resp.status == 200
+        expect resp.bodylen == 150
+        expect resp.http.X-Cache-Hit == 0
+
+        # Overridden max-age directive
+        txreq -url "/overridden"
+        rxresp
+        expect resp.status == 200
+        expect resp.bodylen == 160
+        expect resp.http.X-Cache-Hit == 0
+
+        txreq -url "/overridden"
+        rxresp
+        expect resp.status == 200
+        expect resp.bodylen == 160
+        expect resp.http.X-Cache-Hit == 1
+
+        txreq -url "/overridden_null_maxage"
+        rxresp
+        expect resp.status == 200
+        expect resp.bodylen == 190
+        expect resp.http.X-Cache-Hit == 0
+
+        # The previous response should have been cached even if it had
+        # a max-age=0 since it also had a positive s-maxage
+        txreq -url "/overridden_null_maxage"
+        rxresp
+        expect resp.status == 200
+        expect resp.bodylen == 190
+        expect resp.http.X-Cache-Hit == 1
+
 
 } -run
diff --git a/src/http_ana.c b/src/http_ana.c
index a872e9063..beee63c6c 100644
--- a/src/http_ana.c
+++ b/src/http_ana.c
@@ -3713,6 +3713,7 @@ void http_check_response_for_cacheability(struct stream *s, struct channel *res)
 	struct htx *htx;
 	int has_freshness_info = 0;
 	int has_validator = 0;
+	int has_null_maxage = 0;
 
 	if (txn->status < 200) {
 		/* do not try to cache interim responses! */
@@ -3737,10 +3738,16 @@ void http_check_response_for_cacheability(struct stream *s, struct channel *res)
 			txn->flags |= TX_CACHEABLE | TX_CACHE_COOK;
 			continue;
 		}
+		/* This max-age might be overridden by a s-maxage directive, do
+		 * not unset the TX_CACHEABLE yet. */
+		if (isteqi(ctx.value, ist("max-age=0"))) {
+			has_null_maxage = 1;
+			continue;
+		}
+
 		if (isteqi(ctx.value, ist("private")) ||
 		    isteqi(ctx.value, ist("no-cache")) ||
 		    isteqi(ctx.value, ist("no-store")) ||
-		    isteqi(ctx.value, ist("max-age=0")) ||
 		    isteqi(ctx.value, ist("s-maxage=0"))) {
 			txn->flags &= ~TX_CACHEABLE & ~TX_CACHE_COOK;
 			continue;
@@ -3751,13 +3758,23 @@ void http_check_response_for_cacheability(struct stream *s, struct channel *res)
 			continue;
 		}
 
-		if (istmatchi(ctx.value, ist("s-maxage")) ||
-		    istmatchi(ctx.value, ist("max-age"))) {
+		if (istmatchi(ctx.value, ist("s-maxage"))) {
+			has_freshness_info = 1;
+			has_null_maxage = 0;	/* The null max-age is overridden, ignore it */
+			continue;
+		}
+		if (istmatchi(ctx.value, ist("max-age"))) {
 			has_freshness_info = 1;
 			continue;
 		}
 	}
 
+	/* We had a 'max-age=0' directive but no extra s-maxage, do not cache
+	 * the response. */
+	if (has_null_maxage) {
+		txn->flags &= ~TX_CACHEABLE & ~TX_CACHE_COOK;
+	}
+
 	/* If no freshness information could be found in Cache-Control values,
 	 * look for an Expires header. */
 	if (!has_freshness_info) {