From f96304e673659cb819441ab7d0e05a6fa12dec52 Mon Sep 17 00:00:00 2001 From: phantomas Date: Thu, 24 Jul 2025 10:47:23 +0200 Subject: [PATCH] http2/stream: fix trailers validation 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. Signed-off-by: phantomas --- http/h2_stream.lua | 20 ++++-- spec/h2_stream_spec.lua | 154 ++++++++++++++++++++++++---------------- 2 files changed, 105 insertions(+), 69 deletions(-) diff --git a/http/h2_stream.lua b/http/h2_stream.lua index fc751294..d58e1630 100644 --- a/http/h2_stream.lua +++ b/http/h2_stream.lua @@ -441,12 +441,20 @@ local function validate_headers(headers, is_request, nth_header, ended_stream) return nil, h2_errors.PROTOCOL_ERROR:new_traceback("An HTTP request consists of maximum 2 HEADER blocks", true), ce.EILSEQ end else - --[[ 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 + if nth_header == 1 then + --[[ 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 + elseif nth_header == 2 then + if not ended_stream then + return nil, h2_errors.PROTOCOL_ERROR:new_traceback("Trailers MUST be at end of stream", true), ce.EILSEQ + end + 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 end return true diff --git a/spec/h2_stream_spec.lua b/spec/h2_stream_spec.lua index 1cfed79d..e4ef215b 100644 --- a/spec/h2_stream_spec.lua +++ b/spec/h2_stream_spec.lua @@ -47,6 +47,34 @@ 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("can send a body", function() local s, c = new_pair() local cq = cqueues.new() @@ -196,73 +224,73 @@ describe("http.h2_stream", function() assert.same(nil, stream:get_next_chunk()) end local pushed_stream do - local req_headers = new_headers() - req_headers:append(":method", "GET") - req_headers:append(":scheme", "http") - req_headers:append(":path", "/foo") - req_headers:append(":authority", "example.com") - pushed_stream = assert(stream:push_promise(req_headers)) - end - do - local req_headers = new_headers() - req_headers:append(":status", "200") - assert(pushed_stream:write_headers(req_headers, true)) - end - assert(s:close()) - end) - assert_loop(cq, TEST_TIMEOUT) - assert.truthy(cq:empty()) - end) - it("handles large header blocks", 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", "/") + req_headers:append(":path", "/foo") req_headers:append(":authority", "example.com") - assert(client_stream:write_headers(req_headers, true)) - local pushed_stream = assert(c:get_next_incoming_stream()) - do - local h = assert(pushed_stream:get_headers()) - assert.same("GET", h:get(":method")) - assert.same("http", h:get(":scheme")) - assert.same("/foo", h:get(":path")) - assert.same(req_headers:get(":authority"), h:get(":authority")) - assert.same(nil, pushed_stream:get_next_chunk()) - end - assert(c:close()) - end) - cq:wrap(function() - local stream = assert(s:get_next_incoming_stream()) - do - local h = assert(stream:get_headers()) - assert.same("GET", h:get(":method")) - assert.same("http", h:get(":scheme")) - assert.same("/", h:get(":path")) - assert.same("example.com", h:get(":authority")) - assert.same(nil, stream:get_next_chunk()) - end - local pushed_stream do - local req_headers = new_headers() - req_headers:append(":method", "GET") - req_headers:append(":scheme", "http") - req_headers:append(":path", "/foo") - req_headers:append(":authority", "example.com") - req_headers:append("unknown", ("a"):rep(16384*3)) -- at least 3 frames worth - pushed_stream = assert(stream:push_promise(req_headers)) - end - do - local req_headers = new_headers() - req_headers:append(":status", "200") - assert(pushed_stream:write_headers(req_headers, true)) - end - assert(s:close()) - end) - assert_loop(cq, TEST_TIMEOUT) - assert.truthy(cq:empty()) + pushed_stream = assert(stream:push_promise(req_headers)) + end + do + local req_headers = new_headers() + req_headers:append(":status", "200") + assert(pushed_stream:write_headers(req_headers, true)) + end + assert(s:close()) + end) + assert_loop(cq, TEST_TIMEOUT) + assert.truthy(cq:empty()) + end) + it("handles large header blocks", 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", "/") + req_headers:append(":authority", "example.com") + assert(client_stream:write_headers(req_headers, true)) + local pushed_stream = assert(c:get_next_incoming_stream()) + do + local h = assert(pushed_stream:get_headers()) + assert.same("GET", h:get(":method")) + assert.same("http", h:get(":scheme")) + assert.same("/foo", h:get(":path")) + assert.same(req_headers:get(":authority"), h:get(":authority")) + assert.same(nil, pushed_stream:get_next_chunk()) + end + assert(c:close()) end) + cq:wrap(function() + local stream = assert(s:get_next_incoming_stream()) + do + local h = assert(stream:get_headers()) + assert.same("GET", h:get(":method")) + assert.same("http", h:get(":scheme")) + assert.same("/", h:get(":path")) + assert.same("example.com", h:get(":authority")) + assert.same(nil, stream:get_next_chunk()) + end + local pushed_stream do + local req_headers = new_headers() + req_headers:append(":method", "GET") + req_headers:append(":scheme", "http") + req_headers:append(":path", "/foo") + req_headers:append(":authority", "example.com") + req_headers:append("unknown", ("a"):rep(16384*3)) -- at least 3 frames worth + pushed_stream = assert(stream:push_promise(req_headers)) + end + do + local req_headers = new_headers() + req_headers:append(":status", "200") + assert(pushed_stream:write_headers(req_headers, true)) + end + assert(s:close()) + end) + assert_loop(cq, TEST_TIMEOUT) + assert.truthy(cq:empty()) +end) end) end)