[PATCH] Upstream: prioritise Cache-Control over Expires

[Maxim Dounin]

Hello!

On Sun, Apr 24, 2022 at 07:55:17AM +0300, Maxim Dounin wrote:

[...]

> As far as I can tell, proper behaviour, assuming we parse cache 
> control extensions independently of X-Accel-Expires, can be 
> implemented by using just one flag.

No, that's wrong, as with just one flag it wouldn't be possible to 
correctly disable caching of responses with:

Cache-Control: private
Cache-Control: max-age=10

So it needs at least two flags.  Updated patch below, review and 
testing appreciated.

# HG changeset patch
# User Maxim Dounin <mdounin at mdounin.ru>
# Date 1650814681 -10800
#      Sun Apr 24 18:38:01 2022 +0300
# Node ID 940ba4317a97c72d1ee6700cbf58a543fee04c7a
# Parent  a736a7a613ea6e182ff86fbadcb98bb0f8891c0b
Upstream: fixed X-Accel-Expires/Cache-Control/Expires handling.

Previously, if caching was disabled due to Expires in the past, nginx
failed to cache the response even if it was cacheable as per subsequently
parsed Cache-Control header (ticket #964).

Similarly, if caching was disabled due to Expires in the past,
"Cache-Control: no-cache" or "Cache-Control: max-age=0", caching was not
used if it was cacheable as per subsequently parsed X-Accel-Expires header.

Fix is to avoid disabling caching immediately after parsing Expires in
the past or Cache-Control, but rather set flags which are later checked by
ngx_http_upstream_process_headers() (and cleared by "Cache-Control: max-age"
and X-Accel-Expires).

Additionally, now X-Accel-Expires does not prevent parsing of cache control
extensions, notably stale-while-revalidate and stale-if-error.  This
ensures that order of the X-Accel-Expires and Cache-Control headers is not
important.

Prodded by Vadim Fedorenko and Yugo Horie.

diff --git a/src/http/ngx_http_upstream.c b/src/http/ngx_http_upstream.c
--- a/src/http/ngx_http_upstream.c
+++ b/src/http/ngx_http_upstream.c
@@ -2697,6 +2697,10 @@ ngx_http_upstream_intercept_errors(ngx_h
 
             if (r->cache) {
 
+                if (u->headers_in.no_cache || u->headers_in.expired) {
+                    u->cacheable = 0;
+                }
+
                 if (u->cacheable) {
                     time_t  valid;
 
@@ -2791,6 +2795,10 @@ ngx_http_upstream_process_headers(ngx_ht
 
     umcf = ngx_http_get_module_main_conf(r, ngx_http_upstream_module);
 
+    if (u->headers_in.no_cache || u->headers_in.expired) {
+        u->cacheable = 0;
+    }
+
     if (u->headers_in.x_accel_redirect
         && !(u->conf->ignore_headers & NGX_HTTP_UPSTREAM_IGN_XA_REDIRECT))
     {
@@ -4735,18 +4743,18 @@ ngx_http_upstream_process_cache_control(
         return NGX_OK;
     }
 
-    if (r->cache->valid_sec != 0 && u->headers_in.x_accel_expires != NULL) {
-        return NGX_OK;
-    }
-
     start = h->value.data;
     last = start + h->value.len;
 
+    if (r->cache->valid_sec != 0 && u->headers_in.x_accel_expires != NULL) {
+        goto extensions;
+    }
+
     if (ngx_strlcasestrn(start, last, (u_char *) "no-cache", 8 - 1) != NULL
         || ngx_strlcasestrn(start, last, (u_char *) "no-store", 8 - 1) != NULL
         || ngx_strlcasestrn(start, last, (u_char *) "private", 7 - 1) != NULL)
     {
-        u->cacheable = 0;
+        u->headers_in.no_cache = 1;
         return NGX_OK;
     }
 
@@ -4776,12 +4784,15 @@ ngx_http_upstream_process_cache_control(
         }
 
         if (n == 0) {
-            u->cacheable = 0;
+            u->headers_in.no_cache = 1;
             return NGX_OK;
         }
 
         r->cache->valid_sec = ngx_time() + n;
-    }
+        u->headers_in.expired = 0;
+    }
+
+extensions:
 
     p = ngx_strlcasestrn(start, last, (u_char *) "stale-while-revalidate=",
                          23 - 1);
@@ -4863,7 +4874,7 @@ ngx_http_upstream_process_expires(ngx_ht
     expires = ngx_parse_http_time(h->value.data, h->value.len);
 
     if (expires == NGX_ERROR || expires < ngx_time()) {
-        u->cacheable = 0;
+        u->headers_in.expired = 1;
         return NGX_OK;
     }
 
@@ -4914,6 +4925,8 @@ ngx_http_upstream_process_accel_expires(
 
         default:
             r->cache->valid_sec = ngx_time() + n;
+            u->headers_in.no_cache = 0;
+            u->headers_in.expired = 0;
             return NGX_OK;
         }
     }
@@ -4925,6 +4938,8 @@ ngx_http_upstream_process_accel_expires(
 
     if (n != NGX_ERROR) {
         r->cache->valid_sec = n;
+        u->headers_in.no_cache = 0;
+        u->headers_in.expired = 0;
     }
     }
 #endif
diff --git a/src/http/ngx_http_upstream.h b/src/http/ngx_http_upstream.h
--- a/src/http/ngx_http_upstream.h
+++ b/src/http/ngx_http_upstream.h
@@ -297,6 +297,8 @@ typedef struct {
 
     unsigned                         connection_close:1;
     unsigned                         chunked:1;
+    unsigned                         no_cache:1;
+    unsigned                         expired:1;
 } ngx_http_upstream_headers_in_t;
 
 

-- 
Maxim Dounin
http://mdounin.ru/