status-line trailing SP is missing ?

[Sergey Kandaurov]

> On 29 Aug 2023, at 08:14, Maxim Dounin <mdounin at mdounin.ru> wrote:
> 
> Hello!
> 
> On Sat, Aug 26, 2023 at 04:21:07PM +0200, Jérémy Lal 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 ?
> 
> Interesting.
> 
> As you can see from the report referenced, nginx returns in the 
> HTTP status what is sent by the FastCGI application in the 
> "Status" response header.
> 
> [..]
> 
> Summing the above, I tend to think that it is generally a bad idea 
> to use Status header without a reason-phrase, as it is going to 
> result in missing SP sooner or later.  At least if you do care 
> about missing SP in the status line (AFAIK, it causes no practical 
> issues, though I'm not really tested).

Agree.

> 
> As for the nginx behaviour, I don't think we want to try to 
> implement custom parsing for the Status header to preserve 
> trailing SP if it's present.  We can, however, consider using 
> only the status code from such Status headers, so nginx will 
> provide reason phrase by itself.  
> 
> Something like this should do the trick:
> 
> # HG changeset patch
> # User Maxim Dounin <mdounin at mdounin.ru>
> # Date 1693282407 -10800
> #      Tue Aug 29 07:13:27 2023 +0300
> # Node ID 10aec7047ed8c8e429e8e9b9d676a83751899bc6
> # Parent  44536076405cf79ebdd82a6a0ab27bb3aed86b04
> Upstream: fixed handling of Status headers without reason-phrase.
> 
> Status header with an empty reason-phrase, such as "Status: 404 ", is
> valid per CGI specification, but looses the trailing space during parsing.
> Currently, this results in "HTTP/1.1 404" HTTP status line in the response,
> which violates HTTP specification due to missing trailing space.
> 
> With this change, only the status code is used from such short Status
> header lines, so nginx will generate status line itself, with the space
> and appropriate reason phrase if available.
> 
> Reported at:
> https://mailman.nginx.org/pipermail/nginx/2023-August/EX7G4JUUHJWJE5UOAZMO5UD6OJILCYGX.html
> 
> 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,10 @@ 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) {
> +                        u->headers_in.status_line = *status_line;
> +                    }
> 
>                 } else if (u->headers_in.location) {
>                     u->headers_in.status_n = 302;
> diff --git a/src/http/modules/ngx_http_scgi_module.c b/src/http/modules/ngx_http_scgi_module.c
> --- a/src/http/modules/ngx_http_scgi_module.c
> +++ b/src/http/modules/ngx_http_scgi_module.c
> @@ -1153,7 +1153,10 @@ ngx_http_scgi_process_header(ngx_http_re
>                 }
> 
>                 u->headers_in.status_n = status;
> -                u->headers_in.status_line = *status_line;
> +
> +                if (status_line->len > 3) {
> +                    u->headers_in.status_line = *status_line;
> +                }
> 
>             } else if (u->headers_in.location) {
>                 u->headers_in.status_n = 302;
> diff --git a/src/http/modules/ngx_http_uwsgi_module.c b/src/http/modules/ngx_http_uwsgi_module.c
> --- a/src/http/modules/ngx_http_uwsgi_module.c
> +++ b/src/http/modules/ngx_http_uwsgi_module.c
> @@ -1381,7 +1381,10 @@ ngx_http_uwsgi_process_header(ngx_http_r
>                 }
> 
>                 u->headers_in.status_n = status;
> -                u->headers_in.status_line = *status_line;
> +
> +                if (status_line->len > 3) {
> +                    u->headers_in.status_line = *status_line;
> +                }
> 
>             } else if (u->headers_in.location) {
>                 u->headers_in.status_n = 302;
> 
> 

After discussion in the adjacent thread,
I think the change is fine.

-- 
Sergey Kandaurov