Possible cache_bypass_bug

[John Ferlito]

Howdy,

I'd like to confirm what I think is a bug before I lodge it in the bug
tracker.

I'm seeing pages that shouldn't be cached be cached.

I have a config that looks something like


  upstream app_pool {
    server app.inodes.org;
    fair;
  }

  proxy_cache_path  /srv/www/reverse_proxy/cache levels=1:2 keys_zone=application_cache:8m max_size=1000m inactive=600m;
  proxy_temp_path   /srv/www/reverse_proxy/cache/tmp;

  server {
    listen                  80;
    server_name             inodes.org;

    location / {
      proxy_pass         http://app_pool;
      proxy_pass_header  Set-Cookie;

      proxy_cache        application_cache;
      proxy_cache_key    "$scheme$host$request_uri$query_string";

      proxy_set_header   Host $host;
      proxy_set_header   X-Forwarded-For $proxy_add_x_forwarded_for;
      proxy_set_header   X-Real-IP $remote_addr;

      proxy_cache_valid  200 302 301 60m;
      proxy_cache_valid  404         1m;

      proxy_cache_bypass $http_pragma;
    }
  }


Now the behaviour I'm seeing is specifically for responses with a header like

  Cache-Control: no-store, no-cache, must-revalidate, post-check=0, pre-check=0

That is they shouldn't be cached. They only get cached after a request with

  Pragma: no-cache

set.

If I create a cache log I see something like this.


MISS   "GET /mospace/team/ HTTP/1.1" (200) # First load
BYPASS "GET /mospace/team/ HTTP/1.1" (200) # Shift Reload
HIT    "GET /mospace/team/ HTTP/1.1" (200) # Standard load

I've done some following of the code path and I think I understand
where the bug lies.

In ngx_http_upstream_cache.c we essentially have the following code (snipped
for clarity)

  static void
  ngx_http_upstream_send_response(ngx_http_request_t *r, ngx_http_upstream_t *u)
  {
    switch (ngx_http_test_predicates(r, u->conf->no_cache)) {

    case NGX_DECLINED:
        u->cacheable = 0;
        break;

    default: /* NGX_OK */

        if (u->cache_status == NGX_HTTP_CACHE_BYPASS) {

            r->cache->min_uses = u->conf->cache_min_uses;
            r->cache->body_start = u->conf->buffer_size;
            r->cache->file_cache = u->conf->cache->data;

            if (ngx_http_file_cache_create(r) != NGX_OK) {
                ngx_http_upstream_finalize_request(r, u, 0);
                return;
            }

            u->cacheable = 1;
        }

        break;
    }
  }


So from my understanding if cache_bypass has been set we mark the page as being
able to be cached.

Now this function is called in ngx_http_upstream_process_header, which does the
following

    if (ngx_http_upstream_process_headers(r, u) != NGX_OK) {
        return;
    }

    if (!r->subrequest_in_memory) {
        ngx_http_upstream_send_response(r, u);
        return;
    }


>From what I can tell ngx_http_upstream_process_headers will go through
the headers and when it sees the appropriate Cache-Control header it
will set cacheable to 0.

But later on in ngx_http_upstream_send_response we are setting it to
1. Hence we start caching private pages but only when a bypass header
has been set.

Perhaps we should set cacheable to 1 much earlier in the process when
BYPASS is set? That is in ngx_http_upstream_cache

  switch (ngx_http_test_predicates(r, u->conf->cache_bypass)) {
    // snip
    case NGX_DECLINED:
      u->cache_status = NGX_HTTP_CACHE_BYPASS;
      u-> cachable = 1; // Add this here maybe?
      return NGX_DECLINED;
  }

This is the first time I've looked at the nginx source so perhaps the
above would have some other unintended effect. If not let me know and
I'll submit a patch.

Cheers,
John

-- 
John
Blog                             http://www.inodes.org
LCA2012                          http://lcaunderthestars.org.au