From 14a89a2f4d1081010c8c9ae67f7d3b8540b9c8da Mon Sep 17 00:00:00 2001 From: Brian Neradt Date: Sat, 15 Apr 2023 17:13:56 +0000 Subject: [PATCH] Ensure a reason phrase when sending an HTTP/1 response With an HTTP/1 client and an HTTP/2 origin, responses will come from the origin without reason phrases because reason phrases are explicitly removed from the HTTP/2 RFC. When expanding test coverage of reason phrases it was noticed that when converting responses from HTTP/2 to HTTP/1 to send toward the client, the reason phrases were empty. This updates the conversion logic to ensure that HTTP/1 clients receive a reason phrase. --- proxy/http/HttpTransact.cc | 24 +++++++++++++++++-- proxy/http/HttpTransactHeaders.cc | 22 +++++++++++++---- proxy/http/HttpTransactHeaders.h | 6 ++--- .../replay_h2origin/h1-client-h2-origin.yaml | 15 ++++++++---- 4 files changed, 53 insertions(+), 14 deletions(-) diff --git a/proxy/http/HttpTransact.cc b/proxy/http/HttpTransact.cc index 6599dca1472..d38e2a265c1 100644 --- a/proxy/http/HttpTransact.cc +++ b/proxy/http/HttpTransact.cc @@ -7834,7 +7834,19 @@ HttpTransact::build_response(State *s, HTTPHdr *base_response, HTTPHdr *outgoing HTTPStatus status_code, const char *reason_phrase) { if (reason_phrase == nullptr) { - reason_phrase = http_hdr_reason_lookup(status_code); + if (status_code != HTTP_STATUS_NONE) { + reason_phrase = http_hdr_reason_lookup(status_code); + Debug("http_transact", "Using reason phrase from status %d: %s", status_code, reason_phrase); + } else if (base_response != nullptr && base_response->status_get() != HTTP_STATUS_NONE) { + HTTPStatus const base_response_status = base_response->status_get(); + reason_phrase = http_hdr_reason_lookup(base_response_status); + Debug("http_transact", "Using reason phrase from base_response status %d: %s", base_response_status, reason_phrase); + } else { + // We have to set some value for build_base_response which expects a + // non-nullptr reason_phrase. + reason_phrase = http_hdr_reason_lookup(status_code); + Debug("http_transact", "Using HTTP_STATUS_NONE reason phrase %d: %s", status_code, reason_phrase); + } } if (base_response == nullptr) { @@ -7942,7 +7954,15 @@ HttpTransact::build_response(State *s, HTTPHdr *base_response, HTTPHdr *outgoing HttpTransactHeaders::insert_via_header_in_response(s, outgoing_response); } - HttpTransactHeaders::convert_response(outgoing_version, outgoing_response); + // When converting a response, only set a reason phrase if one was not already + // set via some explicit call above. + char const *reason_phrase_for_convert = nullptr; + int outgoing_reason_phrase_len = 0; + char const *outgoing_reason_phrase = outgoing_response->reason_get(&outgoing_reason_phrase_len); + if (outgoing_reason_phrase == nullptr || outgoing_reason_phrase_len == 0) { + reason_phrase_for_convert = reason_phrase; + } + HttpTransactHeaders::convert_response(outgoing_version, outgoing_response, reason_phrase_for_convert); // process reverse mappings on the location header // TS-1364: do this regardless of response code diff --git a/proxy/http/HttpTransactHeaders.cc b/proxy/http/HttpTransactHeaders.cc index 47e16d86482..369ef8168ab 100644 --- a/proxy/http/HttpTransactHeaders.cc +++ b/proxy/http/HttpTransactHeaders.cc @@ -269,12 +269,12 @@ HttpTransactHeaders::convert_request(HTTPVersion outgoing_ver, HTTPHdr *outgoing //////////////////////////////////////////////////////////////////////// // Just convert the outgoing response to the appropriate version void -HttpTransactHeaders::convert_response(HTTPVersion outgoing_ver, HTTPHdr *outgoing_response) +HttpTransactHeaders::convert_response(HTTPVersion outgoing_ver, HTTPHdr *outgoing_response, char const *reason_phrase) { if (outgoing_ver == HTTPVersion(1, 1)) { - convert_to_1_1_response_header(outgoing_response); + convert_to_1_1_response_header(outgoing_response, reason_phrase); } else if (outgoing_ver == HTTPVersion(1, 0)) { - convert_to_1_0_response_header(outgoing_response); + convert_to_1_0_response_header(outgoing_response, reason_phrase); } else { Debug("http_trans", "[HttpTransactHeaders::convert_response]" "Unsupported Version - passing through"); @@ -325,7 +325,7 @@ HttpTransactHeaders::convert_to_1_1_request_header(HTTPHdr *outgoing_request) //////////////////////////////////////////////////////////////////////// // Take an existing outgoing response header and make it HTTP/1.0 void -HttpTransactHeaders::convert_to_1_0_response_header(HTTPHdr *outgoing_response) +HttpTransactHeaders::convert_to_1_0_response_header(HTTPHdr *outgoing_response, char const *reason_phrase) { // // These are required // ink_assert(outgoing_response->status_get()); @@ -334,6 +334,12 @@ HttpTransactHeaders::convert_to_1_0_response_header(HTTPHdr *outgoing_response) // Set HTTP version to 1.0 outgoing_response->version_set(HTTPVersion(1, 0)); + // Set reason phrase if passed in. + if (reason_phrase != nullptr) { + Debug("http_transact_headers", "Setting HTTP/1.0 reason phrase to '%s'", reason_phrase); + outgoing_response->reason_set(reason_phrase, strlen(reason_phrase)); + } + // Keep-Alive? // Cache-Control? @@ -342,13 +348,19 @@ HttpTransactHeaders::convert_to_1_0_response_header(HTTPHdr *outgoing_response) //////////////////////////////////////////////////////////////////////// // Take an existing outgoing response header and make it HTTP/1.1 void -HttpTransactHeaders::convert_to_1_1_response_header(HTTPHdr *outgoing_response) +HttpTransactHeaders::convert_to_1_1_response_header(HTTPHdr *outgoing_response, char const *reason_phrase) { // These are required ink_assert(outgoing_response->status_get()); // Set HTTP version to 1.1 outgoing_response->version_set(HTTPVersion(1, 1)); + + // Set reason phrase if passed in. + if (reason_phrase != nullptr) { + Debug("http_transact_headers", "Setting HTTP/1.1 reason phrase to '%s'", reason_phrase); + outgoing_response->reason_set(reason_phrase, strlen(reason_phrase)); + } } /////////////////////////////////////////////////////////////////////////////// diff --git a/proxy/http/HttpTransactHeaders.h b/proxy/http/HttpTransactHeaders.h index f53174a052c..ffabdc53b10 100644 --- a/proxy/http/HttpTransactHeaders.h +++ b/proxy/http/HttpTransactHeaders.h @@ -44,11 +44,11 @@ class HttpTransactHeaders static void copy_header_fields(HTTPHdr *src_hdr, HTTPHdr *new_hdr, bool retain_proxy_auth_hdrs, ink_time_t date = 0); static void convert_request(HTTPVersion outgoing_ver, HTTPHdr *outgoing_request); - static void convert_response(HTTPVersion outgoing_ver, HTTPHdr *outgoing_response); + static void convert_response(HTTPVersion outgoing_ver, HTTPHdr *outgoing_response, char const *reason_phrase = nullptr); static void convert_to_1_0_request_header(HTTPHdr *outgoing_request); static void convert_to_1_1_request_header(HTTPHdr *outgoing_request); - static void convert_to_1_0_response_header(HTTPHdr *outgoing_response); - static void convert_to_1_1_response_header(HTTPHdr *outgoing_response); + static void convert_to_1_0_response_header(HTTPHdr *outgoing_response, char const *reason_phrase = nullptr); + static void convert_to_1_1_response_header(HTTPHdr *outgoing_response, char const *reason_phrase = nullptr); static ink_time_t calculate_document_age(ink_time_t request_time, ink_time_t response_time, HTTPHdr *base_response, ink_time_t base_response_date, ink_time_t now); diff --git a/tests/gold_tests/h2/replay_h2origin/h1-client-h2-origin.yaml b/tests/gold_tests/h2/replay_h2origin/h1-client-h2-origin.yaml index 21e6a30b10b..e7c77da175c 100644 --- a/tests/gold_tests/h2/replay_h2origin/h1-client-h2-origin.yaml +++ b/tests/gold_tests/h2/replay_h2origin/h1-client-h2-origin.yaml @@ -72,8 +72,9 @@ sessions: size: 0 proxy-response: - version: '2' + version: '1.1' status: 200 + reason: OK headers: encoding: esc_json fields: @@ -142,6 +143,7 @@ sessions: proxy-response: version: '1.1' status: 200 + reason: OK headers: encoding: esc_json fields: @@ -220,8 +222,9 @@ sessions: size: 0 proxy-response: - version: '2' + version: '1.1' status: 404 + reason: Not Found headers: encoding: esc_json fields: @@ -286,8 +289,9 @@ sessions: size: 32 proxy-response: - version: '2' + version: '1.1' status: 200 + reason: OK headers: encoding: esc_json fields: @@ -353,8 +357,9 @@ sessions: size: 1600 proxy-response: - version: '2' + version: '1.1' status: 200 + reason: OK headers: encoding: esc_json fields: @@ -422,6 +427,7 @@ sessions: proxy-response: version: '1.1' status: 200 + reason: OK headers: encoding: esc_json fields: @@ -489,6 +495,7 @@ sessions: proxy-response: version: '1.1' status: 200 + reason: OK headers: encoding: esc_json fields: