conditional "Expires" header config bug

[Maxim Dounin]

Hello!

On Tue, Dec 07, 2010 at 01:43:16PM -0800, ghazel at gmail.com wrote:

> On Tue, Dec 7, 2010 at 1:36 PM, Maxim Dounin <mdounin at mdounin.ru> wrote:
> > On Tue, Dec 07, 2010 at 01:09:54PM -0800, ghazel at gmail.com wrote:
> >>
> >> I'm trying to conditionally set the "Expires" header. This tiny
> >> snippet does not work as expected:
> >>
> >>     location ~* testing/.*\.png$ {
> >>       if ($request_uri ~* "\.png\?[0-9]+$") {
> >>         expires max;
> >>       }
> >>       try_files $uri /testing/oops.png;
> >>     }
> >>
> >> The basic problem is that if the png does not exist and there is a
> >> query string, I get a 404 instead of oops.png. All other combinations
> >
> > This is expected - try_files isn't inherited into implicit
> > location created by "if".
> 
> I would very much like some more examples of what you mean here. It's
> not clear to me why IfIsEvil occurs, or how to think about what the if
> statement is actually doing.

What "if" statement (inside location) actually doing is creating 
implicit nested location (conditional one), with some hacks to 
make handler modules (proxy_pass, fastcgi_pass, ...) to be 
inherited into this location (normally they aren't).  Basically 
there are two problems:

1. People expect

location / {
    set $true 1;

    if ($true) {
        add_header X-Header-One 1;
    }

    if ($true) {
        add_header X-Header-Two 2;
    }
}

to return both headers.  Though this isn't something going to 
happen, as actually the above config means something like

location / {
    location ...implicit-location-1 {
        add_header X-Header-One 1;
    }

    location ...implicit-location-2 {
        add_header X-Header-Two 2;
    }
}

And as usual nginx will select only one location to process 
request.  Hence only X-Header-Two will be returned.

2. Mentioned hacks are incomplete / broken and may cause various 
problems.  As in your case - they don't handle try_files (which 
isn't normally inherited, too).  E.g. something like

location / {
    try_files /file @fallback;
 
    set $true 1;
 
    if ($true) {
        # nothing
    }
}

is actually means for nginx something like

location / {
    try_files /file @fallback;

    location ...implicit-location-which-matches-everything {
        # no try_files here
    }
}

and try_files will not work - as there is no try_files in location 
which actually used to process request.

More cases are listed on http://wiki.nginx.org/IfIsEvil.

> >> seem to work fine. I have read IfIsEvil, but the alternatives
> >> presented (try_files, return, redirect) do not cover the case of
> >> setting headers.
> >
> > Not "redirect" but "rewrite ... last".  Anyway, they are cover the
> > case in question.
> 
> Er yes, sorry.
> 
> > Generally safe parttern using only "return" looks like the
> > following:
> >
> >    location ~ \.png$ {
> >        recursive_error_pages on;
> >        error_page 418 = @expires;
> >
> >        if ($args ~ "^[0-9]+$") {
> >            return 418;
> >        }
> >
> >        try_files $uri /testing/oops.png;
> >    }
> >
> >    location @expires {
> >        expires max;
> >        try_files $uri /testing/oops.png;
> >    }
> >
> >    location = /testing/oops.png {
> >        # we don't want expires max on oops
> >    }
> 
> This is clever! I didn't know you could set an internal location with
> error_page. Not that it isn't documented, just that I never even
> considered looking there for that sort of thing since the case I'm
> using it for is not an error.

One more hint: using "error_page fallback" like

   log_not_found off;
   error_page 404 = @fallback;

is actually better than try_files in simple cases.  It a) saves 
one stat() syscall and b) isn't racy (try_files aproach has race 
between stat() and then open() in static handler, which makes it 
possible to generate 404 anyway - if you happen to delete file 
between stat() and open()).

On the other hand - try_files is easier to use, especially 
in complex cases.

Maxim Dounin