status-line trailing SP is missing ?

[Maxim Dounin]

Hello!

On Thu, Aug 31, 2023 at 03:45:18PM +0400, Sergey Kandaurov wrote:

> > 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.

Pushed to http://mdounin.ru/hg/nginx, thanks for the review.

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