diff --git a/libstuff/SHTTPSManager.cpp b/libstuff/SHTTPSManager.cpp index 377d427d2..52a6e3a6f 100644 --- a/libstuff/SHTTPSManager.cpp +++ b/libstuff/SHTTPSManager.cpp @@ -112,24 +112,19 @@ void SStandaloneHTTPSManager::postPoll(fd_map& fdm, SStandaloneHTTPSManager::Tra transaction.s->recvBuffer.consumeFront(size); transaction.finished = now; - // This is supposed to check for a "200" or "204 No Content"response, which it does very poorly. It also checks for message - // content. Why this is the what constitutes a valid response is lost to time. Any well-formed response should - // be valid here, and this should get cleaned up. However, this requires testing anything that might rely on - // the existing behavior, which is an exercise for later. - if ( - SContains(transaction.fullResponse.methodLine, " 200") || - SContains(transaction.fullResponse.methodLine, "204") || - SContains(transaction.fullResponse.methodLine, "202") || - transaction.fullResponse.content.size() - ) { - // Pass the transaction down to the subclass. - _onRecv(transaction); - } else { - // Coercing anything that's not 200 to 500 makes no sense, and should be abandoned with the above. - SWARN("Message failed: '" << transaction.fullResponse.methodLine << "'"); - transaction.response = getHTTPResponseCode(transaction.fullResponse.methodLine, 500); + // Log a hint for non-standard responses that don't follow HTTP format (e.g., a load balancer + // returning a plain-text " Timeout" instead of a proper "HTTP/1.1 504 Gateway Timeout"). + // These are not errors in our code, but they are unexpected from the remote service. + // Note: non-standard responses have no Content-Length and an unparseable statusCode, so they + // only reach this branch once the socket is closed (making completeRequest true via CLOSED state). + if (!SStartsWith(transaction.fullResponse.methodLine, "HTTP/")) { + SHMMM("Received non-standard response: '" << transaction.fullResponse.methodLine << "'"); } + // Pass the transaction down to the subclass for all complete responses. Each subclass's _onRecv + // is responsible for parsing the response code and handling errors appropriately. + _onRecv(transaction); + // Finished with the socket, free it up. delete transaction.s; transaction.s = nullptr; @@ -155,6 +150,10 @@ void SStandaloneHTTPSManager::postPoll(fd_map& fdm, SStandaloneHTTPSManager::Tra transaction.finished = now; if (!transaction.fullResponse.methodLine.empty()) { _onRecv(transaction); + } else { + // Got bytes but no parseable methodLine (e.g., bare "\r\n\r\n"). Use a safe + // fallback rather than leaving response == 0, which would stall wait loops. + transaction.response = 400; } // Clean up the socket diff --git a/test/lib/TestHTTPS.cpp b/test/lib/TestHTTPS.cpp index 88acb9131..222668bc6 100644 --- a/test/lib/TestHTTPS.cpp +++ b/test/lib/TestHTTPS.cpp @@ -21,6 +21,9 @@ bool TestHTTPS::_onRecv(Transaction& transaction) transaction.response = status; } } + if (!transaction.response) { + transaction.response = 400; + } return false; } diff --git a/test/tests/LibStuffTest.cpp b/test/tests/LibStuffTest.cpp index 3e03052e5..94774ecb8 100644 --- a/test/tests/LibStuffTest.cpp +++ b/test/tests/LibStuffTest.cpp @@ -2,6 +2,7 @@ #include #include +#include #include #include #include @@ -10,6 +11,7 @@ struct LibStuff : tpunit::TestFixture { LibStuff() : tpunit::TestFixture(true, "LibStuff", + TEST(LibStuff::testGetHTTPResponseCode), TEST(LibStuff::testEncryptDecrpyt), TEST(LibStuff::testSHMACSHA1), TEST(LibStuff::testSHMACSHA256), @@ -992,4 +994,25 @@ struct LibStuff : tpunit::TestFixture string methodLineWithControlChars = "500 Internal Server Error\r\nContent-Type: application/json"; ASSERT_THROW(SComposeHTTP(methodLineWithControlChars, {}, ""), SException); } + + void testGetHTTPResponseCode() + { + // Standard HTTP response lines parse correctly. + ASSERT_EQUAL(SStandaloneHTTPSManager::getHTTPResponseCode("HTTP/1.1 200 OK"), 200); + ASSERT_EQUAL(SStandaloneHTTPSManager::getHTTPResponseCode("HTTP/1.1 204 No Content"), 204); + ASSERT_EQUAL(SStandaloneHTTPSManager::getHTTPResponseCode("HTTP/1.1 202 Accepted"), 202); + ASSERT_EQUAL(SStandaloneHTTPSManager::getHTTPResponseCode("HTTP/1.1 400 Bad Request"), 400); + ASSERT_EQUAL(SStandaloneHTTPSManager::getHTTPResponseCode("HTTP/1.1 500 Internal Server Error"), 500); + ASSERT_EQUAL(SStandaloneHTTPSManager::getHTTPResponseCode("HTTP/1.1 504 Gateway Timeout"), 504); + + // Non-standard responses (e.g., from a load balancer sending " Timeout" instead of a proper + // HTTP status line) fall back to 400 rather than crashing or producing a misleading result. + ASSERT_EQUAL(SStandaloneHTTPSManager::getHTTPResponseCode(" Timeout"), 400); + ASSERT_EQUAL(SStandaloneHTTPSManager::getHTTPResponseCode("Timeout"), 400); + ASSERT_EQUAL(SStandaloneHTTPSManager::getHTTPResponseCode(""), 400); + + // Explicit defaultStatusCode is used when parsing fails. + ASSERT_EQUAL(SStandaloneHTTPSManager::getHTTPResponseCode("garbage", 502), 502); + ASSERT_EQUAL(SStandaloneHTTPSManager::getHTTPResponseCode("", 500), 500); + } } __LibStuff;