From 44210f5403ec32832646e807ebcbb9cfc8a7114f Mon Sep 17 00:00:00 2001 From: Alex Beaman Date: Fri, 27 Mar 2026 12:21:31 -0600 Subject: [PATCH 1/4] Fix Message failed: ' Timeout' SWARN in SHTTPSManager::postPoll MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Remove the broken 200/204/202 check that prevented _onRecv() from being called for valid non-2xx responses. Any complete HTTP response should be passed to the subclass for handling — the existing comment explicitly noted this logic was wrong. Non-standard methodLines (e.g., a load balancer returning " Timeout" instead of a proper HTTP status line) now log at SHMMM severity instead of firing a SWARN and coercing the response to 500. Fixes: https://github.com/Expensify/Expensify/issues/610175 Co-Authored-By: Claude Sonnet 4.6 --- libstuff/SHTTPSManager.cpp | 25 +++++++++---------------- test/tests/LibStuffTest.cpp | 19 +++++++++++++++++++ 2 files changed, 28 insertions(+), 16 deletions(-) diff --git a/libstuff/SHTTPSManager.cpp b/libstuff/SHTTPSManager.cpp index 8dbf31300..7b994a1c8 100644 --- a/libstuff/SHTTPSManager.cpp +++ b/libstuff/SHTTPSManager.cpp @@ -113,24 +113,17 @@ 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 = 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. + 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; diff --git a/test/tests/LibStuffTest.cpp b/test/tests/LibStuffTest.cpp index 3e03052e5..e6f3bc176 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,21 @@ 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); + } } __LibStuff; From c4f5923972966ec6b9e765ca671412c33ae25366 Mon Sep 17 00:00:00 2001 From: Alex Beaman Date: Mon, 30 Mar 2026 15:37:01 -0600 Subject: [PATCH 2/4] Add test cases for getHTTPResponseCode defaultStatusCode parameter Verify that the caller-specified fallback code is respected when the methodLine cannot be parsed as a valid HTTP status. Made-with: Cursor --- test/tests/LibStuffTest.cpp | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/test/tests/LibStuffTest.cpp b/test/tests/LibStuffTest.cpp index e6f3bc176..94774ecb8 100644 --- a/test/tests/LibStuffTest.cpp +++ b/test/tests/LibStuffTest.cpp @@ -1010,5 +1010,9 @@ struct LibStuff : tpunit::TestFixture 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; From fc95e07c922b7910135f31aa513b7f6c1f21cc68 Mon Sep 17 00:00:00 2001 From: Alex Beaman Date: Fri, 10 Apr 2026 14:56:36 -0600 Subject: [PATCH 3/4] Fix TestHTTPS::_onRecv to actually default to 400 as its comment says The comment stated 'if we fail to find such a code, or can't parse it as an integer, we default to 400' but the code left transaction.response at 0. This made wait loops like 'while (!transaction->response)' stall on non-standard method lines (e.g. ' Timeout'). Co-Authored-By: Claude Sonnet 4.6 --- test/lib/TestHTTPS.cpp | 3 +++ 1 file changed, 3 insertions(+) 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; } From 169203083e360a5ef1d4f0657842d602f79bfe83 Mon Sep 17 00:00:00 2001 From: Alex Beaman Date: Fri, 10 Apr 2026 15:07:22 -0600 Subject: [PATCH 4/4] Fix partial-response path: set response=400 when size>0 but methodLine empty If SParseHTTP returns nonzero bytes but leaves methodLine empty (e.g. a bare CRLF response), _onRecv was skipped, leaving transaction.response==0 and stalling any 'while (!transaction->response)' wait loop. Also clarify in comment that non-standard responses (no Content-Length, unparseable status) only reach completeRequest=true once the socket closes. Co-Authored-By: Claude Sonnet 4.6 --- libstuff/SHTTPSManager.cpp | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/libstuff/SHTTPSManager.cpp b/libstuff/SHTTPSManager.cpp index 6bf6929f7..52a6e3a6f 100644 --- a/libstuff/SHTTPSManager.cpp +++ b/libstuff/SHTTPSManager.cpp @@ -115,6 +115,8 @@ void SStandaloneHTTPSManager::postPoll(fd_map& fdm, SStandaloneHTTPSManager::Tra // 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 << "'"); } @@ -148,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