[PATCH] Fixed 413 custom error page for HTTP/2 and HTTP/3 (ticket #2609)

[Roman Arutyunyan]

Hi Maxim,

> On 6 Mar 2024, at 12:28 AM, Maxim Dounin <mdounin at mdounin.ru> wrote:
> 
> Hello!
> 
> On Mon, Mar 04, 2024 at 06:46:23PM +0400, Roman Arutyunyan wrote:
> 
>> # HG changeset patch
>> # User Roman Arutyunyan <arut at nginx.com>
>> # Date 1709563405 -14400
>> #      Mon Mar 04 18:43:25 2024 +0400
>> # Node ID 3b0be477ab7246caba4c5152286b8be520ee0418
>> # Parent  44da04c2d4db94ad4eefa84b299e07c5fa4a00b9
>> Fixed 413 custom error page for HTTP/2 and HTTP/3 (ticket #2609).
>> 
>> Previously an attempt to return a custom 413 error page for these protocols
>> resulted in the standard 413 page (if recursive_error_pages was off) or
>> otherwise internal redirection cycle followed by the 500 error.
>> 
>> Discarding request body for HTTP/1 starts by setting r->discard_body which
>> indicates the body is currently being discarded.  If and when the entire body
>> is read and discarded, the flag is cleared and r->headers_in.content_length_n
>> is set to zero.  Both r->discard_body and r->headers_in.content_length_n
>> prevent nginx from re-generating 413 error after internal redirect in
>> ngx_http_core_find_config_phase().
>> 
>> However the above does not work for HTTP/2 and HTTP/3.  Discarding request
>> body for these protocols does not affect the above mentioned fields, which is
>> why there's no protection against re-generating the 413 error.  The fix is to
>> assign zero to r->headers_in.content_length_n much like in HTTP/1 case after
>> the body is entirely read and discarded, except for these protocols no active
>> discard is needed.
>> 
>> diff --git a/src/http/ngx_http_request_body.c b/src/http/ngx_http_request_body.c
>> --- a/src/http/ngx_http_request_body.c
>> +++ b/src/http/ngx_http_request_body.c
>> @@ -640,12 +640,14 @@ ngx_http_discard_request_body(ngx_http_r
>> #if (NGX_HTTP_V2)
>>     if (r->stream) {
>>         r->stream->skip_data = 1;
>> +        r->headers_in.content_length_n = 0;
>>         return NGX_OK;
>>     }
>> #endif
>> 
>> #if (NGX_HTTP_V3)
>>     if (r->http_version == NGX_HTTP_VERSION_30) {
>> +        r->headers_in.content_length_n = 0;
>>         return NGX_OK;
>>     }
>> #endif
> 
> The patch is wrong, see here:
> 
> https://trac.nginx.org/nginx/ticket/1152#comment:6

Thanks for your kind comment, I read that before.

The patch fixes exactly what it fixes.
Accessing the request body after it was discarded (for example from a 413 custom handler) is a sign of misconfiguration.
Indeed for a misconfigured nginx there is a problem with a HTTP/1 body currently being discarded as well as other inconsistencies.
This may or may not be fixed in the future, but anyway it's a separate issue, which does not exist in a properly configured nginx.

> The issue is in my TODO list.  Once properly fixed, you'll be able 
> to merge the fix from freenginx.
> 
> Alternatively, consider submitting patches to 
> the nginx-devel at freenginx.org list for proper review.

We understand your hard feelings about leaving the project. Hope you'll be ok.

> -- 
> Maxim Dounin
> http://mdounin.ru/
> _______________________________________________
> nginx-devel mailing list
> nginx-devel at nginx.org
> https://mailman.nginx.org/mailman/listinfo/nginx-devel

----
Roman Arutyunyan
arut at nginx.com




-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.nginx.org/pipermail/nginx-devel/attachments/20240306/89566532/attachment.htm>