http2/stream: fix trailers validation

[Ph4ntomas]

problem: h2_stream validate_header reject valid trailers

cause: for response headers, validate_header ignores the nth_header parameter, and checked the pseudo-header :status, even for response trailers.

fix: only check for :status on the first header.

[Ph4ntomas]

@daurnimator this should address your comment in #236

[daurnimator]

I don't think this is the correct way to check this.

[Ph4ntomas]

I disagree, but let me explain why I've implemented the fix like this (quote are from RFC 9113 8.1, unless otherwise specified). If you don´t agree after that, I´ll go with the fix in the next change request.

The standard describe a message as:

An HTTP message (request or response) consists of:

1. one HEADERS frame (followed by zero or more CONTINUATION frames) containing the header section (see Section 6.3 of [HTTP]),
2. zero or more DATA frames containing the message content (see Section 6.4 of [HTTP]), and
3. optionally, one HEADERS frame (followed by zero or more CONTINUATION frames) containing the trailer section, if present (see Section 6.5 of [HTTP]).

Since 2. is optional, we can't rely on the presence of data to know the next header frame will be a trailer (the message could be bunch of interim response, followed by a single final header, followed by a trailer).

We can't rely on the number of headers either because of interim responses.

An interim response consists of a HEADERS frame (which might be followed by zero or more CONTINUATION frames) containing the control data and header section of an interim (1xx) HTTP response

From RFC 9110:

A 1xx response is terminated by the end of the header section; it cannot contain content or trailers.

This means that regardless of the amount of interim responses, there will be at most one trailer, which will be held in a header field with the END_STREAM flag set.

For a response only, a server MAY send any number of interim responses before the HEADERS frame containing a final response.
[...]
An endpoint that receives a HEADERS frame without the END_STREAM flag set after receiving the HEADERS frame that opens a request or after receiving a final (non-informational) status code MUST treat the corresponding request or response as malformed (Section 8.1.1).

Since only interim responses are allowed before the "final" response, once we've seen the final response (i.e. a non 1xx status code), we know that the next header frame (if any) must be a trailer.

From all of this, my understanding is that only trailers can appear after a non-informational status code was receive. Since all responses (interim or final) must include a ":status" pseudo header, and everything else is optional, this code is the only thing that can reliably identify the next HEADER as a trailer on a response.

[daurnimator]

An endpoint that receives a HEADERS frame without the END_STREAM flag set after receiving the HEADERS frame that opens a request or after receiving a final (non-informational) status code MUST treat the corresponding request or response as malformed (Section 8.1.1).

Ah, that's a better rule to quote+live by!
So not string.find(status, "^1%d%d$") is indeed a reasonable check.
However that's only a stream level error.

vs the nth_header == 1 change I suggested should still be a protocol error I think? Or at least I vaguely remember clients/a test suite expecting it to be a protocol error

[Ph4ntomas]

However that's only a stream level error.

vs the nth_header == 1 change I suggested should still be a protocol error I think?

I'm not sure I follow. This just mark the stream so we know we're in the last message, but doesn't trigger any error by itself. The mark is then forwarded to validate_headers as its expect_trailer parameter.

A protocol error is warranted (and returned) we then see a header:

• with a status pseudo-header (disallowed in the context of trailers)

  lua-http/http/h2_stream.lua

  Lines 395 to 397 in e879651

  	if (is_request and nth_header ~= 1) or valid_pseudo_headers[name] ~= is_request or expect_trailer then
  	return nil, h2_errors.PROTOCOL_ERROR:new_traceback("Pseudo-header fields are only valid in the context in which they are defined", true), ce.EILSEQ
  	end

• without the END_STREAM flag (since trailer MUST end the stream)

  lua-http/http/h2_stream.lua

  Lines 455 to 457 in e879651

  	if not ended_stream then
  	return nil, h2_errors.PROTOCOL_ERROR:new_traceback("Trailers MUST be at end of stream", true), ce.EILSEQ
  	end

[daurnimator]

I think a better valid fix to this whole issue might be just changing this line to:

Suggested change

	elseif not expect_trailer then
	elseif nth_header == 1 then

and fixing up the comment to say that not all header blocks (such as trailers) will have a status field

[Ph4ntomas]

Doing this will let invalid header go through if at least one interim response was received. If anything, the status check should only be removed when ended_stream is true, as this will limit the amount of false positive.