commit 20427cb8aacdd00c41479e6ca91ae89acdf262a1
parent ef4d71923326a188a37df61edfb981e0510bf412
Author: Valentin Gosu <valentin.gosu@gmail.com>
Date: Sat, 22 Nov 2025 14:17:31 +0000
Bug 1999659 - Fix neqo_glue::parse_headers r=necko-reviewers,kershaw,mxinden
Header values are now parsed as a byte array.
Instead of calling from_utf8 on the entire string of headers,
we first split and only enforce the header names to be utf8.
This also adds a gtest for the parse_headers function.
Indeally the parse_headers function can be removed once we fix bug 1971997
and stop needlessly serializing and deserializing headers for H2 and H3.
Differential Revision: https://phabricator.services.mozilla.com/D273462
Diffstat:
5 files changed, 260 insertions(+), 40 deletions(-)
diff --git a/netwerk/protocol/http/Http3Session.cpp b/netwerk/protocol/http/Http3Session.cpp
@@ -1513,12 +1513,6 @@ nsresult Http3Session::TryActivating(
QueueStream(aStream);
return rv;
}
- if (rv == NS_ERROR_DOM_INVALID_HEADER_VALUE) {
- // neqo_http3conn_fetch may fail if the headers contain non-ascii
- // values. In that case we want to fallback to HTTP/2 right away.
- // HACK: This should be removed when we fix it in bug 1999659
- return NS_ERROR_HTTP2_FALLBACK_TO_HTTP1;
- }
// Previously we always returned NS_OK here, which caused the
// transaction to wait until the quic connection timed out
diff --git a/netwerk/socket/neqo_glue/src/lib.rs b/netwerk/socket/neqo_glue/src/lib.rs
@@ -45,7 +45,7 @@ use nserror::{
NS_ERROR_FILE_ALREADY_EXISTS, NS_ERROR_ILLEGAL_VALUE, NS_ERROR_INVALID_ARG,
NS_ERROR_NET_HTTP3_PROTOCOL_ERROR, NS_ERROR_NET_INTERRUPT, NS_ERROR_NET_RESET,
NS_ERROR_NET_TIMEOUT, NS_ERROR_NOT_AVAILABLE, NS_ERROR_NOT_CONNECTED, NS_ERROR_OUT_OF_MEMORY,
- NS_ERROR_SOCKET_ADDRESS_IN_USE, NS_ERROR_UNEXPECTED, NS_OK, NS_ERROR_DOM_INVALID_HEADER_VALUE,
+ NS_ERROR_SOCKET_ADDRESS_IN_USE, NS_ERROR_UNEXPECTED, NS_OK, NS_ERROR_DOM_INVALID_HEADER_NAME,
};
use nsstring::{nsACString, nsCString};
use thin_vec::ThinVec;
@@ -1145,41 +1145,59 @@ fn parse_headers(headers: &nsACString) -> Result<Vec<Header>, nsresult> {
let mut hdrs = Vec::new();
// this is only used for headers built by Firefox.
// Firefox supplies all headers already prepared for sending over http1.
- // They need to be split into (String, String) pairs.
- match str::from_utf8(headers) {
- Err(_) => {
- // Header names are checked for UTF8 in Necko
- // Values aren't necessaryly UTF-8 - Should be fixed in bug 1999659
- return Err(NS_ERROR_DOM_INVALID_HEADER_VALUE);
+ // They need to be split into (name, value) pairs where name is a String
+ // and value is a Vec<u8>.
+
+ let headers_bytes: &[u8] = headers;
+
+ // Split on either \r or \n. When splitting "\r\n" sequences, this produces
+ // an empty element between them which is filtered out by the is_empty check.
+ // This also handles malformed inputs with bare \r or \n.
+ for elem in headers_bytes.split(|&b| b == b'\r' || b == b'\n').skip(1) {
+ if elem.is_empty() {
+ continue;
+ }
+ if elem.starts_with(b":") {
+ // colon headers are for http/2 and 3 and this is http/1
+ // input, so that is probably a smuggling attack of some
+ // kind.
+ continue;
}
- Ok(h) => {
- for elem in h.split("\r\n").skip(1) {
- if elem.starts_with(':') {
- // colon headers are for http/2 and 3 and this is http/1
- // input, so that is probably a smuggling attack of some
- // kind.
- continue;
- }
- if elem.is_empty() {
- continue;
- }
- let mut hdr_str = elem.splitn(2, ':');
- let name = hdr_str
- .next()
- .expect("`elem` is not empty")
- .trim()
- .to_lowercase();
- if is_excluded_header(&name) {
- continue;
- }
- let value = hdr_str
- .next()
- .map_or_else(String::new, |v| v.trim().to_string());
+ let colon_pos = match elem.iter().position(|&b| b == b':') {
+ Some(pos) => pos,
+ None => continue, // No colon, skip this line
+ };
- hdrs.push(Header::new(name, value));
- }
+ let name_bytes = &elem[..colon_pos];
+ // Safe: if colon is at the end, this yields an empty slice
+ let value_bytes = &elem[colon_pos + 1..];
+
+ // Header names must be valid UTF-8
+ let name = match str::from_utf8(name_bytes) {
+ Ok(n) => n.trim().to_lowercase(),
+ Err(_) => return Err(NS_ERROR_DOM_INVALID_HEADER_NAME),
+ };
+
+ if is_excluded_header(&name) {
+ continue;
}
+
+ // Trim leading and trailing optional whitespace (OWS) from value.
+ // Per RFC 9110, OWS is defined as *( SP / HTAB ), i.e., space and tab only.
+ let value = value_bytes
+ .iter()
+ .position(|&b| b != b' ' && b != b'\t')
+ .map_or(&value_bytes[0..0], |start| {
+ let end = value_bytes
+ .iter()
+ .rposition(|&b| b != b' ' && b != b'\t')
+ .map_or(value_bytes.len(), |pos| pos + 1);
+ &value_bytes[start..end]
+ })
+ .to_vec();
+
+ hdrs.push(Header::new(name, value));
}
Ok(hdrs)
}
@@ -2686,3 +2704,32 @@ pub unsafe extern "C" fn neqo_decoder_offset(decoder: &mut NeqoDecoder) -> u64 {
let decoder = decoder.decoder.as_mut().unwrap();
decoder.offset() as u64
}
+
+// Test function called from C++ gtest
+// Callback signature: fn(user_data, name_ptr, name_len, value_ptr, value_len)
+type HeaderCallback = extern "C" fn(*mut c_void, *const u8, usize, *const u8, usize);
+
+#[no_mangle]
+pub extern "C" fn neqo_glue_test_parse_headers(
+ headers_input: &nsACString,
+ callback: HeaderCallback,
+ user_data: *mut c_void,
+) -> bool {
+ match parse_headers(headers_input) {
+ Ok(headers) => {
+ for header in headers {
+ let name_bytes = header.name().as_bytes();
+ let value_bytes = header.value();
+ callback(
+ user_data,
+ name_bytes.as_ptr(),
+ name_bytes.len(),
+ value_bytes.as_ptr(),
+ value_bytes.len(),
+ );
+ }
+ true
+ }
+ Err(_) => false,
+ }
+}
diff --git a/netwerk/test/gtest/TestParseHeaders.cpp b/netwerk/test/gtest/TestParseHeaders.cpp
@@ -0,0 +1,178 @@
+#include "gtest/gtest.h"
+#include "nsString.h"
+#include <cstring>
+#include <vector>
+#include "nsTArray.h"
+
+struct ParsedHeader {
+ nsCString name;
+ nsTArray<uint8_t> value;
+};
+
+extern "C" {
+using HeaderCallback = void (*)(void* user_data, const uint8_t* name_ptr,
+ size_t name_len, const uint8_t* value_ptr,
+ size_t value_len);
+
+bool neqo_glue_test_parse_headers(const nsACString* headers_input,
+ HeaderCallback callback, void* user_data);
+
+// C callback that collects headers into a vector
+void collect_header_callback(void* user_data, const uint8_t* name_ptr,
+ size_t name_len, const uint8_t* value_ptr,
+ size_t value_len) {
+ auto* headers = static_cast<std::vector<ParsedHeader>*>(user_data);
+ ParsedHeader header;
+ header.name.Assign(reinterpret_cast<const char*>(name_ptr), name_len);
+ header.value.AppendElements(value_ptr, value_len);
+ headers->push_back(std::move(header));
+}
+}
+
+TEST(TestParseHeaders, ParseHeadersBasic)
+{
+ nsAutoCString headers;
+ headers.AssignLiteral(
+ "\r\nContent-Type: text/html\r\nContent-Length: 1234\r\n");
+
+ std::vector<ParsedHeader> parsed_headers;
+ bool success = neqo_glue_test_parse_headers(&headers, collect_header_callback,
+ &parsed_headers);
+
+ EXPECT_TRUE(success);
+ ASSERT_EQ(parsed_headers.size(), 2U);
+
+ EXPECT_TRUE(parsed_headers[0].name.EqualsLiteral("content-type"));
+ EXPECT_EQ(parsed_headers[0].value.Length(), 9U);
+ EXPECT_EQ(0, memcmp(parsed_headers[0].value.Elements(), "text/html", 9));
+
+ EXPECT_TRUE(parsed_headers[1].name.EqualsLiteral("content-length"));
+ EXPECT_EQ(parsed_headers[1].value.Length(), 4U);
+ EXPECT_EQ(0, memcmp(parsed_headers[1].value.Elements(), "1234", 4));
+}
+
+TEST(TestParseHeaders, ParseHeadersWithWhitespace)
+{
+ nsAutoCString headers;
+ headers.AssignLiteral(
+ "\r\n X-Custom : test-value \r\nUser-Agent: Mozilla/5.0 \r\n");
+
+ std::vector<ParsedHeader> parsed_headers;
+ bool success = neqo_glue_test_parse_headers(&headers, collect_header_callback,
+ &parsed_headers);
+
+ EXPECT_TRUE(success);
+ ASSERT_EQ(parsed_headers.size(), 2U);
+
+ EXPECT_TRUE(parsed_headers[0].name.EqualsLiteral("x-custom"));
+ EXPECT_EQ(parsed_headers[0].value.Length(), 10U);
+ EXPECT_EQ(0, memcmp(parsed_headers[0].value.Elements(), "test-value", 10));
+
+ EXPECT_TRUE(parsed_headers[1].name.EqualsLiteral("user-agent"));
+ EXPECT_EQ(parsed_headers[1].value.Length(), 11U);
+ EXPECT_EQ(0, memcmp(parsed_headers[1].value.Elements(), "Mozilla/5.0", 11));
+}
+
+TEST(TestParseHeaders, ParseHeadersWithColonInValue)
+{
+ nsAutoCString headers;
+ headers.AssignLiteral("\r\nLocation: http://example.com:8080/path\r\n");
+
+ std::vector<ParsedHeader> parsed_headers;
+ bool success = neqo_glue_test_parse_headers(&headers, collect_header_callback,
+ &parsed_headers);
+
+ EXPECT_TRUE(success);
+ ASSERT_EQ(parsed_headers.size(), 1U);
+ EXPECT_TRUE(parsed_headers[0].name.EqualsLiteral("location"));
+ EXPECT_EQ(parsed_headers[0].value.Length(), 28U);
+ EXPECT_EQ(0, memcmp(parsed_headers[0].value.Elements(),
+ "http://example.com:8080/path", 28));
+}
+
+TEST(TestParseHeaders, ParseHeadersNonUtf8Value)
+{
+ // Create headers with non-UTF8 bytes in the value
+ nsAutoCString headers;
+ headers.AssignLiteral("\r\nX-Custom: ");
+ headers.Append(char(0xFF));
+ headers.Append(char(0xFE));
+ headers.Append(char(0xFD));
+ headers.AppendLiteral("\r\n");
+
+ std::vector<ParsedHeader> parsed_headers;
+ bool success = neqo_glue_test_parse_headers(&headers, collect_header_callback,
+ &parsed_headers);
+
+ EXPECT_TRUE(success);
+ ASSERT_EQ(parsed_headers.size(), 1U);
+ EXPECT_TRUE(parsed_headers[0].name.EqualsLiteral("x-custom"));
+ EXPECT_EQ(parsed_headers[0].value.Length(), 3U);
+ EXPECT_EQ(parsed_headers[0].value[0], 0xFF);
+ EXPECT_EQ(parsed_headers[0].value[1], 0xFE);
+ EXPECT_EQ(parsed_headers[0].value[2], 0xFD);
+}
+
+TEST(TestParseHeaders, ParseHeadersExcludesColonHeaders)
+{
+ nsAutoCString headers;
+ headers.AssignLiteral("\r\n:method: GET\r\nContent-Type: text/html\r\n");
+
+ std::vector<ParsedHeader> parsed_headers;
+ bool success = neqo_glue_test_parse_headers(&headers, collect_header_callback,
+ &parsed_headers);
+
+ EXPECT_TRUE(success);
+ ASSERT_EQ(parsed_headers.size(), 1U); // :method should be excluded
+ EXPECT_TRUE(parsed_headers[0].name.EqualsLiteral("content-type"));
+}
+
+TEST(TestParseHeaders, ParseHeadersExcludesForbiddenHeaders)
+{
+ nsAutoCString headers;
+ headers.AssignLiteral(
+ "\r\nConnection: keep-alive\r\nContent-Type: text/html\r\nHost: "
+ "example.com\r\n");
+
+ std::vector<ParsedHeader> parsed_headers;
+ bool success = neqo_glue_test_parse_headers(&headers, collect_header_callback,
+ &parsed_headers);
+
+ EXPECT_TRUE(success);
+ ASSERT_EQ(parsed_headers.size(),
+ 1U); // Connection and Host should be excluded
+ EXPECT_TRUE(parsed_headers[0].name.EqualsLiteral("content-type"));
+}
+
+TEST(TestParseHeaders, ParseHeadersEmptyValue)
+{
+ nsAutoCString headers;
+ headers.AssignLiteral("\r\nX-Empty:\r\nX-Spaces: \r\n");
+
+ std::vector<ParsedHeader> parsed_headers;
+ bool success = neqo_glue_test_parse_headers(&headers, collect_header_callback,
+ &parsed_headers);
+
+ EXPECT_TRUE(success);
+ ASSERT_EQ(parsed_headers.size(), 2U);
+ EXPECT_TRUE(parsed_headers[0].name.EqualsLiteral("x-empty"));
+ EXPECT_EQ(parsed_headers[0].value.Length(), 0U);
+ EXPECT_TRUE(parsed_headers[1].name.EqualsLiteral("x-spaces"));
+ EXPECT_EQ(parsed_headers[1].value.Length(), 0U);
+}
+
+TEST(TestParseHeaders, ParseHeadersInvalidUtf8Name)
+{
+ // Create headers with non-UTF8 bytes in the name
+ nsAutoCString headers;
+ headers.AssignLiteral("\r\n");
+ headers.Append(char(0xFF));
+ headers.Append(char(0xFE));
+ headers.AppendLiteral(": value\r\n");
+
+ std::vector<ParsedHeader> parsed_headers;
+ bool success = neqo_glue_test_parse_headers(&headers, collect_header_callback,
+ &parsed_headers);
+
+ EXPECT_FALSE(success); // Should fail with invalid UTF-8 in name
+}
diff --git a/netwerk/test/gtest/moz.build b/netwerk/test/gtest/moz.build
@@ -30,6 +30,7 @@ UNIFIED_SOURCES += [
"TestMIMEInputStream.cpp",
"TestMozURL.cpp",
"TestPACMan.cpp",
+ "TestParseHeaders.cpp",
"TestPollableEvent.cpp",
"TestProtocolProxyService.cpp",
"TestReadStreamToString.cpp",
diff --git a/netwerk/test/unit/test_http3_non_ascii_header.js b/netwerk/test/unit/test_http3_non_ascii_header.js
@@ -68,7 +68,7 @@ add_task(async function test_non_ascii_header() {
);
let protocol1 = req1.protocolVersion;
- Assert.ok(protocol1 === "h3" || protocol1 === "h2", `Using ${protocol1}`);
+ Assert.strictEqual(protocol1, "h3", `Using ${protocol1}`);
Assert.equal(req1.responseStatus, 200);
info(buf1);
@@ -82,6 +82,6 @@ add_task(async function test_non_ascii_header() {
);
let protocol2 = req2.protocolVersion;
- Assert.ok(protocol2 === "h3" || protocol2 === "h2", `Using ${protocol2}`);
+ Assert.strictEqual(protocol2, "h3", `Using ${protocol2}`);
Assert.equal(req2.responseStatus, 200);
});
