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.

diff --git a/reg-tests/cache/caching_rules.vtc b/reg-tests/cache/caching_rules.vtc
index 61274b4b58a8abf8d55ccf4b6179f1889968c9e5..a488875b30fc795ae2fa6bb58dde121fc269177b 100644 (file)
--- 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 a872e90636e046285bfd408c8040b5eb679b2130..beee63c6ccf19d1f71c3c01713b013224dcee2d6 100644 (file)
--- 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,11 +3758,21 @@ 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,