status-line trailing SP is missing ?

[Maxim Dounin]

Hello!

On Mon, Aug 28, 2023 at 08:59:28PM +0400, Sergey Kandaurov wrote:

> 
> > On 26 Aug 2023, at 18:21, Jérémy Lal <kapouer at melix.org> wrote:
> > 
> > Hi,
> > 
> > https://bugs.debian.org/1050571
> > reports that the Status line doesn't always end with space, which seems contradictory to RFC9112 which states:
> > "A server MUST send the space that separates the status-code from the reason-phrase even when the reason-phrase is absent (i.e., the status-line would end with the space)."
> > 
> > Is it a documented nginx 1.24.0 behavior ?
> > 
> 
> Note that the response line with empty reason phrase
> is properly generated since nginx 1.5.6.
> The exception remains is proxying FastCGI responses
> as there is no distinguished response line in CGI syntax.
> 
> The reason is that Status is a CGI header field, and hence
> it is parsed by a generic routine that cuts trailing spaces.
> But it can have a trailing space per RFC 3875, section 6.3.3.
> So it needs a special treatment to preserve SP before empty reason
> phrase.  The below patch should help; although it doesn't look
> efficient and can be polished, I think this is quite enough for
> valid use cases.

I very much doubt that RFC 3875 properly defines whitespace 
handling, see my response to the original report.  In this 
particular case, it seems to define a header which cannot be 
correctly parsed if reason-phrase is empty.

> 
> # HG changeset patch
> # User Sergey Kandaurov <pluknet at nginx.com>
> # Date 1693238094 -14400
> #      Mon Aug 28 19:54:54 2023 +0400
> # Node ID f621c60dfa277774ab5fadb3c8b805957ca3f281
> # Parent  2880f60a80c3e2706151dc7b48dc1267e39c47a9
> FastCGI: preserved SP for empty Status header reason phrase.
> 
> diff --git a/src/http/modules/ngx_http_fastcgi_module.c b/src/http/modules/ngx_http_fastcgi_module.c
> --- a/src/http/modules/ngx_http_fastcgi_module.c
> +++ b/src/http/modules/ngx_http_fastcgi_module.c
> @@ -2048,7 +2048,25 @@ ngx_http_fastcgi_process_header(ngx_http
>                      }
>  
>                      u->headers_in.status_n = status;
> -                    u->headers_in.status_line = *status_line;
> +
> +                    if (status_line->len == 3) {
> +                        /* preserve SP for empty reason phrase */
> +
> +                        u->headers_in.status_line.data = ngx_pnalloc(r->pool,
> +                                                                     5);
> +                        if (u->headers_in.status_line.data == NULL) {
> +                            return NGX_ERROR;
> +                        }
> +
> +                        ngx_memcpy(u->headers_in.status_line.data,
> +                                   status_line->data, 3);
> +                        u->headers_in.status_line.data[3] = ' ';
> +                        u->headers_in.status_line.data[4] = '\0';
> +                        u->headers_in.status_line.len = 4;
> +
> +                    } else {
> +                        u->headers_in.status_line = *status_line;
> +                    }
>  
>                  } else if (u->headers_in.location) {
>                      u->headers_in.status_n = 302;

Do you think we really need this complexity here?  I tend to think 
that

    if (status_line->len > 3) {
        u->headers_in.status_line = *status_line;
    }

would be enough, so nginx will generate status line by itself for 
such headers.

The only downside I can see is that it will provide no way to 
actually generate a response with well-known status code, yet with 
an empty reason phrase.

(Also note that uwsgi and scgi modules needs to be patched 
similarly.)

> Alternatively, the reason-phrase value from FastCGI response 
> can be ignored, nginx will translate it with its own value:
> 
> # HG changeset patch
> # User Sergey Kandaurov <pluknet at nginx.com>
> # Date 1693241054 -14400
> #      Mon Aug 28 20:44:14 2023 +0400
> # Node ID f1a452cbd57ff4fba1caf3f36cb3624bd45411ea
> # Parent  2880f60a80c3e2706151dc7b48dc1267e39c47a9
> FastCGI: preserved SP for empty Status header reason phrase.
> 
> As per RFC 3875 sec 6.3.3, the status code is always followed by SP.
> The header is parsed by the common routine, and it can be challenging
> to properly translate empty reason phrase to HTTP/1.1 status line, as
> that would require an additional memory allocation.  For simplicity,
> the reason phrase is now ignored; the header filter will take care of
> it by inserting its own reason phrase for well known status codes.
> 
> diff --git a/src/http/modules/ngx_http_fastcgi_module.c b/src/http/modules/ngx_http_fastcgi_module.c
> --- a/src/http/modules/ngx_http_fastcgi_module.c
> +++ b/src/http/modules/ngx_http_fastcgi_module.c
> @@ -2048,7 +2048,6 @@ ngx_http_fastcgi_process_header(ngx_http
>                      }
>  
>                      u->headers_in.status_n = status;
> -                    u->headers_in.status_line = *status_line;
>  
>                  } else if (u->headers_in.location) {
>                      u->headers_in.status_n = 302;

I don't think it's a good idea, since this always drops the 
reason phrase provided by the upstream server.  It can contain 
some meaningful information which will be lost as a result, most 
notably for non-standard error codes.

-- 
Maxim Dounin
http://mdounin.ru/