[patch] Http image_filter: return 405 when method is HEAD andbodyis empty.

[Maxim Dounin]

Hello!

On Wed, Jun 21, 2017 at 12:28:23AM +0800, 胡聪 (hucc) wrote:

> Hi,
> 
> On Tuesday, Jun 20, 2017 11:53 PM +0300, Maxim Dounin wrote:
> 
> >> >On Tue, Jun 20, 2017 at 12:56:15AM +0800, 胡聪 (hucc) wrote:
> >> >
> >> >> Returning 415 does not conform to the HTTP protocol when image and proxy_pass
> >> >> configured in same location.
> >> >>
> >> >> # HG changeset patch
> >> >> # User hucongcong <hucong.c at foxmail.com>
> >> >> # Date 1497890354 -28800
> >> >> #      Tue Jun 20 00:39:14 2017 +0800
> >> >> # Node ID af3a94de6a6549dec5e1205514eda1893313a14c
> >> >> # Parent  d1816a2696de8c2faa1cd913a151e5f62a8620f3
> >> >> Http image_filter: return 405 when method is HEAD and body is empty.
> >> >>
> >> >> diff -r d1816a2696de -r af3a94de6a65 src/http/modules/ngx_http_image_filter_module.c
> >> >> --- a/src/http/modules/ngx_http_image_filter_module.c   Fri Jun 16 18:15:58 2017 +0300
> >> >> +++ b/src/http/modules/ngx_http_image_filter_module.c   Tue Jun 20 00:39:14 2017 +0800
> >> >> @@ -330,6 +330,12 @@ ngx_http_image_body_filter(ngx_http_requ
> >> >>                  }
> >> >>              }
> >> >>
> >> >> +            if (r->method & NGX_HTTP_HEAD) {
> >> >> +                return ngx_http_filter_finalize_request(r,
> >> >> +                                                  &ngx_http_image_filter_module,
> >> >> +                                                  NGX_HTTP_NOT_ALLOWED);
> >> >> +            }
> >> >> +
> >> >>              return ngx_http_filter_finalize_request(r,
> >> >>                                                &ngx_http_image_filter_module,
> >> >>                                                NGX_HTTP_UNSUPPORTED_MEDIA_TYPE);
> >> >
> >> >Please clarify why you think that the current code is wrong.  I
> >> >don't see any problems with returning 415 to HEAD requests as long
> >> >we are going to return 415 to GETs.
> >>
> >> Ok, the problem is that nginx will return 200 to GET request and
> >> 415 to HEAD request.
> >>
> >> The configuration looks like:
> >> #proxy_method    GET;#not configured
> >> location / {
> >>         image    resize   180 360;
> >>         #...
> >>         proxy_pass    http://test_upstream$uri;
> >> }
> >
> >Ok, so the problem appears when using proxy_pass without
> >additional configuration, image_filter, and a HEAD request.
> >
> >Unfortunately, the suggested change will also break the correct
> >behaviour in other cases, for example, when serving static files.
> >Using "proxy_method GET;" might be a better option.
> 
> Thanks for the reply. Which behaviour will be breaked? Is the following change right? 

When the response is read from a static file, not proxied, and/or 
proxied with "proxy_method GET;", an empty response means exacly 
that: an empty response, and returning 415 is perfectly correct.

The ngx_buf_size() test will limit the incorrect behaviour to 
indeed empty responses (previous version of your patch affected 
all non-image responses), but it won't eliminate incorrect behaviour 
for empty responses.

The root cause of the problem is that in the configuration you've 
provided proxy don't know about image filter, and uses the HEAD 
method in the request to upstream filter despite the fact image 
filter needs a body to return proper response.  This doesn't look 
like something easy to fix, as proper fix implies some knowledge 
to be passed between image filter and proxy.

Most trivial solution, as suggested above, would be to use 
"proxy_method GET" explicitly in the configuration.  It might be 
actually a good enough solution, as image filter is a special 
module and it requires proper configuration anyway.

-- 
Maxim Dounin
http://nginx.org/