Skip to content

http2/stream: fix trailers validation #237

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

http2/stream: fix trailers validation #237

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9c73b7b
spec/h2_stream_spec: make sure trailers are properly handled on response
Ph4ntomas Nov 9, 2025
e879651
http/h2_stream: fix trailers validation
Ph4ntomas Nov 9, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 23 additions & 7 deletions http/h2_stream.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,9 @@ local function new_stream(connection)
recv_headers_fifo = new_fifo();
recv_headers_cond = cc.new();

recv_final_header = false;
sent_final_header = false;

chunk_fifo = new_fifo();
chunk_cond = cc.new();

Expand Down Expand Up @@ -371,7 +374,7 @@ local valid_pseudo_headers = {
[":authority"] = true;
[":status"] = false;
}
local function validate_headers(headers, is_request, nth_header, ended_stream)
local function validate_headers(headers, is_request, nth_header, ended_stream, expect_trailer)
-- Section 8.1.2: A request or response containing uppercase header field names MUST be treated as malformed
for name in headers:each() do
if name:lower() ~= name then
Expand All @@ -389,7 +392,7 @@ local function validate_headers(headers, is_request, nth_header, ended_stream)
Pseudo-header fields MUST NOT appear in trailers.
Endpoints MUST treat a request or response that contains
undefined or invalid pseudo-header fields as malformed]]
if (is_request and nth_header ~= 1) or valid_pseudo_headers[name] ~= is_request then
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
if seen_non_colon then
Expand Down Expand Up @@ -440,14 +443,18 @@ local function validate_headers(headers, is_request, nth_header, ended_stream)
elseif nth_header > 2 then
return nil, h2_errors.PROTOCOL_ERROR:new_traceback("An HTTP request consists of maximum 2 HEADER blocks", true), ce.EILSEQ
end
else
elseif not expect_trailer then
Copy link
Owner

@daurnimator daurnimator Nov 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

@Ph4ntomas Ph4ntomas Nov 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

--[[ For HTTP/2 responses, a single :status pseudo-header field is
defined that carries the HTTP status code field (RFC7231, Section 6).
This pseudo-header field MUST be included in all responses; otherwise,
the response is malformed (Section 8.1.2.6)]]
if not headers:has(":status") then
return nil, h2_errors.PROTOCOL_ERROR:new_traceback(":status pseudo-header field MUST be included in all responses", true), ce.EILSEQ
end
else
if not ended_stream then
return nil, h2_errors.PROTOCOL_ERROR:new_traceback("Trailers MUST be at end of stream", true), ce.EILSEQ
end
end
return true
end
Expand All @@ -474,13 +481,17 @@ local function process_end_headers(stream, end_stream, pad_len, pos, promised_st

if not promised_stream then
stream.stats_recv_headers = stream.stats_recv_headers + 1
local validate_ok, validate_err, errno2 = validate_headers(headers, stream.type ~= "client", stream.stats_recv_headers, end_stream)
local validate_ok, validate_err, errno2 = validate_headers(headers, stream.type ~= "client", stream.stats_recv_headers, end_stream, stream.recv_final_header)
if not validate_ok then
return nil, validate_err, errno2
end
if headers:has("content-length") then
stream.content_length = tonumber(headers:get("content-length"), 10)
end
if stream.type == "client" and not stream.recv_final_header then
local status = headers:get(":status")
stream.recv_final_header = string.match(status, "1%d%d") == nil
Copy link
Owner

@daurnimator daurnimator Nov 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

@Ph4ntomas Ph4ntomas Nov 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Owner

@daurnimator daurnimator Nov 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

@Ph4ntomas Ph4ntomas Nov 11, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

end
stream.recv_headers_fifo:push(headers)

if end_stream then
Expand All @@ -498,7 +509,7 @@ local function process_end_headers(stream, end_stream, pad_len, pos, promised_st
end
end
else
local validate_ok, validate_err, errno2 = validate_headers(headers, true, 1, false)
local validate_ok, validate_err, errno2 = validate_headers(headers, true, 1, false, false)
if not validate_ok then
return nil, validate_err, errno2
end
Expand Down Expand Up @@ -1321,9 +1332,14 @@ end

function stream_methods:write_headers(headers, end_stream, timeout)
assert(headers, "missing argument: headers")
assert(validate_headers(headers, self.type == "client", self.stats_sent_headers+1, end_stream))
assert(validate_headers(headers, self.type == "client", self.stats_sent_headers+1, end_stream, self.sent_final_header))
assert(type(end_stream) == "boolean", "'end_stream' MUST be a boolean")

if self.type ~= "client" and not self.sent_final_header then
local status = headers:get(":status")
self.sent_final_header = string.match(status, "1%d%d") == nil
end

local padded, exclusive, stream_dep, weight = nil, nil, nil, nil
return write_headers(self, function(payload, end_headers, deadline)
return self:write_headers_frame(payload, end_stream, end_headers, padded, exclusive, stream_dep, weight, deadline and deadline-monotime())
Expand All @@ -1333,7 +1349,7 @@ end
function stream_methods:push_promise(headers, timeout)
assert(self.type == "server")
assert(headers, "missing argument: headers")
assert(validate_headers(headers, true, 1, false))
assert(validate_headers(headers, true, 1, false, false))
assert(headers:has(":authority"), "PUSH_PROMISE must have an :authority")

local promised_stream = self.connection:new_stream()
Expand Down
60 changes: 60 additions & 0 deletions spec/h2_stream_spec.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,66 @@ describe("http.h2_stream", function()
assert_loop(cq, TEST_TIMEOUT)
assert.truthy(cq:empty())
end)
it("accepts trailers without pseudo_header", function()
local s, c = new_pair()
local cq = cqueues.new()
cq:wrap(function()
local client_stream = c:new_stream()
local req_headers = new_headers()
req_headers:append(":method", "GET")
req_headers:append(":scheme", "http")
req_headers:append(":path", "/")
assert(client_stream:write_headers(req_headers, true))
assert(client_stream:get_headers())
assert(client_stream:get_headers())
assert(c:close())
end)
cq:wrap(function()
local stream = assert(s:get_next_incoming_stream())
assert(stream:get_headers())
local res_headers = new_headers()
res_headers:append(":status", "200")
local res_trailers = new_headers()
res_trailers:append("trailer-status", "0")
assert(stream:write_headers(res_headers, false))
assert(stream:write_headers(res_trailers, true))
assert(s:close())
end)
assert_loop(cq, TEST_TIMEOUT)
assert.truthy(cq:empty())
end)
it("accepts trailers after interim responses", function()
local s, c = new_pair()
local cq = cqueues.new()
cq:wrap(function()
local client_stream = c:new_stream()
local req_headers = new_headers()
req_headers:append(":method", "GET")
req_headers:append(":scheme", "http")
req_headers:append(":path", "/")
assert(client_stream:write_headers(req_headers, true))
assert(client_stream:get_headers()) -- 100 continue
assert(client_stream:get_headers()) -- 200 OK
assert(client_stream:get_headers()) -- trailer
assert(c:close())
end)
cq:wrap(function()
local stream = assert(s:get_next_incoming_stream())
assert(stream:get_headers())
local info_headers = new_headers()
info_headers:append(":status", "100")
local res_headers = new_headers()
res_headers:append(":status", "200")
local res_trailers = new_headers()
res_trailers:append("trailer-status", "0")
assert(stream:write_headers(info_headers, false))
assert(stream:write_headers(res_headers, false))
assert(stream:write_headers(res_trailers, true))
assert(s:close())
end)
assert_loop(cq, TEST_TIMEOUT)
assert.truthy(cq:empty())
end)
it("can send a body", function()
local s, c = new_pair()
local cq = cqueues.new()
Expand Down