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 <kapo...@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 <pluk...@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 <pluk...@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
