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@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@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@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/
_______________________________________________
nginx mailing list
nginx@nginx.org
https://mailman.nginx.org/mailman/listinfo/nginx