> On 31 Aug 2023, at 14:28, Maxim Dounin <mdounin@mdounin.ru> wrote:
>
> Hello!
>
> On Wed, Aug 30, 2023 at 04:20:15PM +0400, Sergey Kandaurov wrote:
>
>>> On 29 Aug 2023, at 08:33, Maxim Dounin <mdounin@mdounin.ru> wrote:
>>>
>>> On Mon, Aug 28, 2023 at 08:59:28PM +0400, Sergey Kandaurov wrote:
>>>
>>>>> On 26 Aug 2023, at 18:21, Jérémy Lal <kapouer@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.
>>>
>>
>> Yes, it is quite dubious how this can be parsed correctly.
>> Although it is valid to have a trailing space in Status,
>> this contradicts to header field value syntax per RFC 3875:
>> field-content = *( token | separator | quoted-string )
>
> Note that field-value syntax does no apply to the "Status" header,
> its syntax is defined separately.

Well, per RFC 3875 BNF, Status is CGI-field, which if generalized
to other-field, consists of field-content.

>
> On the other hand, the "Status" header syntax does not allow any
> spaces after the colon, which rules out headers like "Status: 200 OK",
> and makes the "Status" header syntax highly questionable.

Again, if generalize the "Status" header to header-field, then
there's a note about whitespace between the ":" and the field-value.

>
> As already suggested in my response to the original report, I tend
> to think that the best available option is to ignore RFC 3875 idea
> about headers syntax, and use HTTP instead.

I don't mind.

[..]

>>>
>>> (Also note that uwsgi and scgi modules needs to be patched
>>> similarly.)
>>
>> These modules use a distinct routine to parse status line,
>> that is ngx_http_parse_status_line, which respects spaces,
>> and they expect the first line to look like a status line
>> (similar to the proxy module).
>> So I don't see immediate necessity to patch them as well.
>
> Sure, but these are distinct parsing modes: parsing status line as
> in raw HTTP responses (which, in particular, are used by NPH scripts),
> or parsing the Status header, as in CGI responses. Both should
> work correctly.
>
> In particular, current SCGI specification
> (https://python.ca/scgi/protocol.txt) uses "Status: ..." in the
> example (whereas original version used "HTTP/1.0 ...",
> https://web.archive.org/web/20020403050958/http://python.ca/nas/scgi/protocol.txt).
>

Ahh ok, didn't know that.
That explains why all three modules should be patches.

> I see no reasons why "Status: 404 " should work correctly in
> FastCGI, but shouldn't in SCGI.
>
>>>> 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 <pluknet@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.
>>
>> It is little to no benefit to carry reason phrase,
>> also this is consistent with HTTP/1.1:
>> : A client SHOULD ignore the reason-phrase content because
>> : it is not a reliable channel for information
>>
>> Note also that HTTP/2 and then HTTP/3 do not define a way
>> to carry the reason phrase that is included in HTTP/1.1.
>
> In my practice, it is quite usable when debugging and analyzing
> the protocol flow. HTTP/2 decision to drop it looks highly
> questionable to me, and generally reflects the fact that HTTP/2 is
> not designed for debugging.
>
>> Personally I'd proceed with your approach for fastcgi module
>> only, though I won't insist to patch other modules as well.
>> Although this would introduce an extra condition in these
>> other modules without a reason (as per my above analysis),
>> this would keep sources in sync between them, which is fine,
>> and will allow to stop using ngx_http_parse_status_line().
>>
>> For the latter, uwsgi seems to use HTTP syntax in response,
>> so it is ok to call ngx_http_parse_status_line() here, but
>> scgi seems to follows CGI syntax where status line is
>> carried in the Status header field, so the call would
>> always return NGX_ERROR. Luckily this is already handled
>> by falling back to ngx_http_scgi_process_header().
>
> SCGI does not define a response syntax at all, and at least
> "Status: ..." and "HTTP/1.0 ..." were seen in the different
> versions of the specification, see above. As such, I tend to
> think that both variants needs to be (correctly) supported.
>

Sure, it makes sense then to keep both variants.

> (On the other hand, for uwsgi I'm not sure if "Status: ..." is
> allowed, it seems to contradict the protocol (see
> https://uwsgi-docs.readthedocs.io/en/latest/Protocol.html, "Every
> uwsgi request generates a response in the uwsgi format"). We
> might consider removing Status header parsing in uwsgi, but this
> is probably out of the scope of this issue.)

--
Sergey Kandaurov
_______________________________________________
nginx mailing list
nginx@nginx.org
https://mailman.nginx.org/mailman/listinfo/nginx