Hello! On Thu, Aug 31, 2023 at 03:45:18PM +0400, Sergey Kandaurov wrote:
> > On 29 Aug 2023, at 08:14, Maxim Dounin <mdou...@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 <mdou...@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
