details: http://freenginx.org/hg/nginx/rev/c5623963c29e branches: changeset: 9295:c5623963c29e user: Maxim Dounin <[email protected]> date: Tue Jun 25 21:44:50 2024 +0300 description: Upstream: fixed proxy_no_cache when caching errors.
Caching errors, notably intercepted errors and internally generated 502/504 errors, as well as handling of cache revalidation with 304, did not take into account u->conf->no_cache predicates configured. As a result, an error might be cached even if configuration explicitly says not to. Fix is to check u->conf->no_cache in these cases. To simplify usage in multiple places, checking u->conf->no_cache is now done in a separate function. As a minor optimization, u->conf->no_cache is only checked if u->cacheable is set. As a side effect, this change also fixes caching errors after proxy_cache_bypass. Also, during cache revalidation u->cacheable is now tested, so 304 responses which disable caching won't extend cacheability of stored responses. Additionally, when caching internally generated 502/504 errors u->cacheable is now explicitly updated from u->headers_in.no_cache and u->headers_in.expired, restoring the behaviour before 8041:0784ab86ad08 (1.23.0) when an error happens while reading the response headers. Reported by Kirill A. Korinsky, https://freenginx.org/pipermail/nginx/2024-April/000082.html diffstat: src/http/ngx_http_upstream.c | 136 ++++++++++++++++++++++++++++-------------- 1 files changed, 89 insertions(+), 47 deletions(-) diffs (193 lines): 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 @@ -21,6 +21,8 @@ static ngx_int_t ngx_http_upstream_cache ngx_http_request_t *r, ngx_http_upstream_t *u); static ngx_int_t ngx_http_upstream_cache_check_range(ngx_http_request_t *r, ngx_http_upstream_t *u); +static ngx_int_t ngx_http_upstream_no_cache(ngx_http_request_t *r, + ngx_http_upstream_t *u); static ngx_int_t ngx_http_upstream_cache_status(ngx_http_request_t *r, ngx_http_variable_value_t *v, uintptr_t data); static ngx_int_t ngx_http_upstream_cache_key(ngx_http_request_t *r, @@ -2632,6 +2634,12 @@ ngx_http_upstream_test_next(ngx_http_req updating = r->cache->updating_sec; error = r->cache->error_sec; + if (ngx_http_upstream_no_cache(r, u) != NGX_OK) { + ngx_http_upstream_finalize_request(r, u, + NGX_HTTP_INTERNAL_SERVER_ERROR); + return NGX_OK; + } + rc = u->reinit_request(r); if (rc != NGX_OK) { @@ -2650,30 +2658,33 @@ ngx_http_upstream_test_next(ngx_http_req rc = NGX_HTTP_INTERNAL_SERVER_ERROR; } - if (valid == 0) { - valid = r->cache->valid_sec; - updating = r->cache->updating_sec; - error = r->cache->error_sec; - } - - if (valid == 0) { - valid = ngx_http_file_cache_valid(u->conf->cache_valid, - u->headers_in.status_n); + if (u->cacheable) { + + if (valid == 0) { + valid = r->cache->valid_sec; + updating = r->cache->updating_sec; + error = r->cache->error_sec; + } + + if (valid == 0) { + valid = ngx_http_file_cache_valid(u->conf->cache_valid, + u->headers_in.status_n); + if (valid) { + valid = now + valid; + } + } + if (valid) { - valid = now + valid; + r->cache->valid_sec = valid; + r->cache->updating_sec = updating; + r->cache->error_sec = error; + + r->cache->date = now; + + ngx_http_file_cache_update_header(r); } } - if (valid) { - r->cache->valid_sec = valid; - r->cache->updating_sec = updating; - r->cache->error_sec = error; - - r->cache->date = now; - - ngx_http_file_cache_update_header(r); - } - ngx_http_upstream_finalize_request(r, u, rc); return NGX_OK; } @@ -2745,8 +2756,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 (ngx_http_upstream_no_cache(r, u) != NGX_OK) { + ngx_http_upstream_finalize_request(r, u, + NGX_HTTP_INTERNAL_SERVER_ERROR); + return NGX_OK; } if (u->cacheable) { @@ -3159,29 +3172,8 @@ ngx_http_upstream_send_response(ngx_http r->cache->file.fd = NGX_INVALID_FILE; } - switch (ngx_http_test_predicates(r, u->conf->no_cache)) { - - case NGX_ERROR: + if (ngx_http_upstream_no_cache(r, u) != NGX_OK) { ngx_http_upstream_finalize_request(r, u, NGX_ERROR); - return; - - case NGX_DECLINED: - u->cacheable = 0; - break; - - default: /* NGX_OK */ - - if (u->cache_status == NGX_HTTP_CACHE_BYPASS) { - - /* create cache if previously bypassed */ - - if (ngx_http_file_cache_create(r) != NGX_OK) { - ngx_http_upstream_finalize_request(r, u, NGX_ERROR); - return; - } - } - - break; } if (u->cacheable) { @@ -3372,6 +3364,50 @@ ngx_http_upstream_send_response(ngx_http } +#if (NGX_HTTP_CACHE) + +static ngx_int_t +ngx_http_upstream_no_cache(ngx_http_request_t *r, ngx_http_upstream_t *u) +{ + ngx_int_t rc; + + if (!u->cacheable) { + return NGX_OK; + } + + if (u->headers_in.no_cache || u->headers_in.expired) { + u->cacheable = 0; + return NGX_OK; + } + + rc = ngx_http_test_predicates(r, u->conf->no_cache); + + if (rc == NGX_ERROR) { + return NGX_ERROR; + } + + if (rc == NGX_DECLINED) { + u->cacheable = 0; + return NGX_OK; + } + + /* rc == NGX_OK */ + + if (u->cache_status == NGX_HTTP_CACHE_BYPASS) { + + /* create cache if previously bypassed */ + + if (ngx_http_file_cache_create(r) != NGX_OK) { + return NGX_ERROR; + } + } + + return NGX_OK; +} + +#endif + + static void ngx_http_upstream_upgrade(ngx_http_request_t *r, ngx_http_upstream_t *u) { @@ -4619,9 +4655,15 @@ ngx_http_upstream_finalize_request(ngx_h if (r->cache) { - if (u->cacheable) { - - if (rc == NGX_HTTP_BAD_GATEWAY || rc == NGX_HTTP_GATEWAY_TIME_OUT) { + if (rc == NGX_HTTP_BAD_GATEWAY || rc == NGX_HTTP_GATEWAY_TIME_OUT) { + + if (!u->header_sent) { + if (ngx_http_upstream_no_cache(r, u) != NGX_OK) { + u->cacheable = 0; + } + } + + if (u->cacheable) { time_t valid; valid = ngx_http_file_cache_valid(u->conf->cache_valid, rc);
