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

[Maxim Dounin]

Hello!

On Thu, Jun 22, 2017 at 01:05:37AM +0800, 胡聪 (hucc) wrote:

> Hi,
> 
> On Wednesday, Jun 21, 2017 1:38 AM +0300, Maxim Dounin wrote:
> 
> >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.
> 
> Thank you for the detailed explanation. After reading the code again,
> I still have two questions. First, is the following change appropriate ?
> 
> --- a/ngx_http_image_filter_module.c
> +++ b/ngx_http_image_filter_module.c
> @@ -330,6 +330,15 @@ ngx_http_image_body_filter(ngx_http_request_t *r, ngx_chain_t *in)
>                  }
>              }
> 
> +            if (r->method & NGX_HTTP_HEAD
> +                && ctx->length
> +                && ngx_buf_size(in->buf) == 0)
> +            {
> +                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);

I don't think it is.

> Second, Why not restrict the use of the image filter in the following way ?
> 
> --- a/ngx_http_image_filter_module.c
> +++ b/ngx_http_image_filter_module.c
> @@ -224,7 +224,9 @@ ngx_http_image_header_filter(ngx_http_request_t *r)
>      ngx_http_image_filter_ctx_t   *ctx;
>      ngx_http_image_filter_conf_t  *conf;
> 
> -    if (r->headers_out.status == NGX_HTTP_NOT_MODIFIED) {
> +    if (r->headers_out.status != NGX_HTTP_OK
> +        && r->headers_out.status != NGX_HTTP_PARTIAL_CONTENT)
> +    {
>          return ngx_http_next_header_filter(r);
>      }

The goal of the image filter module is to process all resources 
returned, including ones returned with error codes.  Such a change 
will make it useless at least for the use case the module was 
written for.

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