Hello!

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

> > On 29 Aug 2023, at 08:14, Maxim Dounin <mdounin@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@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/
_______________________________________________
nginx mailing list
nginx@nginx.org
https://mailman.nginx.org/mailman/listinfo/nginx