Bug 1687903 - Enable stricter HTTP response status line parsing; r=va…

…lentin,necko-reviewers

Differential Revision: https://phabricator.services.mozilla.com/D186229

devtools/client/netmonitor/test/browser_net_http3_request_details.js

@@ -50,7 +50,7 @@ add_task(async function () {
   );
   is(
     tabpanel.querySelector(".status").childNodes[1].textContent,
-    "OK",
+    "", // HTTP/2 and 3 send no status text, only a code
     "The status summary value is incorrect."
   );
   // Version
@@ -139,7 +139,7 @@ add_task(async function () {
 
   const responseHeadersText = rawHeadersElements[0].textContent;
   const rawResponseHeaderFirstLine = responseHeadersText.split(/\r\n|\n|\r/)[0];
-  is(rawResponseHeaderFirstLine, "HTTP/3 200 OK");
+  is(rawResponseHeaderFirstLine, "HTTP/3 200 "); // H2/3 send no status text
 
   info("Assert the content of the protocol column");
   const target = document.querySelectorAll(".request-list-item")[0];

dom/xhr/tests/mochitest.ini

@@ -115,7 +115,7 @@ skip-if =
 [test_worker_xhrAbort.html]
 skip-if = (os == "win") || (os == "mac")
 [test_XHR.html]
-skip-if = http2
+skip-if = http2 || http3
 [test_xhr_abort_after_load.html]
 [test_XHR_anon.html]
 skip-if =
@@ -127,7 +127,7 @@ skip-if =
   http3
   http2
 [test_XHR_http2.html]
-skip-if = !http2
+skip-if = !http2 && !http3
 [test_XHR_onuploadprogress.html]
 [test_xhr_overridemimetype_throws_on_invalid_state.html]
 [test_XHR_parameters.html]

modules/libpref/init/StaticPrefList.yaml

@@ -12613,6 +12613,12 @@
   value: true
   mirror: always
 
+  # If true, will be more strict with status code parsing
+- name: network.http.strict_response_status_line_parsing
+  type: RelaxedAtomicBool
+  value: true
+  mirror: always
+
   # If true, remove the resumption token when 0RTT failed.
 - name: network.http.remove_resumption_token_when_early_data_failed
   type: RelaxedAtomicBool

netwerk/protocol/http/InterceptedHttpChannel.cpp

@@ -848,7 +848,8 @@ InterceptedHttpChannel::SynthesizeStatus(uint16_t aStatus,
   statusLine.AppendLiteral(" ");
   statusLine.Append(aReason);
 
-  mSynthesizedResponseHead->ParseStatusLine(statusLine);
+  NS_ENSURE_SUCCESS(mSynthesizedResponseHead->ParseStatusLine(statusLine),
+                    NS_ERROR_FAILURE);
   return NS_OK;
 }
 

netwerk/protocol/http/nsHttpResponseHead.cpp

@@ -8,6 +8,7 @@
 #include "HttpLog.h"
 
 #include "mozilla/StaticPrefs_network.h"
+#include "mozilla/TextUtils.h"
 #include "mozilla/Unused.h"
 #include "nsHttpResponseHead.h"
 #include "nsIHttpHeaderVisitor.h"
@@ -351,19 +352,18 @@ void nsHttpResponseHead::AssignDefaultStatusText() {
   net_GetDefaultStatusTextForCode(mStatus, mStatusText);
 }
 
-void nsHttpResponseHead::ParseStatusLine(const nsACString& line) {
+nsresult nsHttpResponseHead::ParseStatusLine(const nsACString& line) {
   RecursiveMutexAutoLock monitor(mRecursiveMutex);
-  ParseStatusLine_locked(line);
+  return ParseStatusLine_locked(line);
 }
 
-void nsHttpResponseHead::ParseStatusLine_locked(const nsACString& line) {
+nsresult nsHttpResponseHead::ParseStatusLine_locked(const nsACString& line) {
   //
   // Parse Status-Line:: HTTP-Version SP Status-Code SP Reason-Phrase CRLF
   //
 
   const char* start = line.BeginReading();
   const char* end = line.EndReading();
-  const char* p = start;
 
   // HTTP-Version
   ParseVersion(start);
@@ -373,9 +373,38 @@ void nsHttpResponseHead::ParseStatusLine_locked(const nsACString& line) {
   if ((mVersion == HttpVersion::v0_9) || (index == -1)) {
     mStatus = 200;
     AssignDefaultStatusText();
+  } else if (StaticPrefs::network_http_strict_response_status_line_parsing()) {
+    // Status-Code: all ASCII digits after any whitespace
+    const char* p = start + index + 1;
+    while (p < end && NS_IsHTTPWhitespace(*p)) ++p;
+    if (p == end || !mozilla::IsAsciiDigit(*p)) {
+      return NS_ERROR_FAILURE;
+    }
+    const char* codeStart = p;
+    while (p < end && mozilla::IsAsciiDigit(*p)) ++p;
+
+    // Only accept 3 digits (including all leading zeros)
+    // Also if next char isn't whitespace, fail (ie, code is 0x2)
+    if (p - codeStart > 3 || (p < end && !NS_IsHTTPWhitespace(*p))) {
+      return NS_ERROR_FAILURE;
+    }
+
+    // At this point the code is between 0 and 999 inclusive
+    nsDependentCSubstring strCode(codeStart, p - codeStart);
+    nsresult rv;
+    mStatus = strCode.ToInteger(&rv);
+    if (NS_FAILED(rv)) {
+      return NS_ERROR_FAILURE;
+    }
+
+    // Reason-Phrase: whatever remains after any whitespace (even empty)
+    while (p < end && NS_IsHTTPWhitespace(*p)) ++p;
+    if (p != end) {
+      mStatusText = nsDependentCSubstring(p, end - p);
+    }
   } else {
     // Status-Code
-    p += index + 1;
+    const char* p = start + index + 1;
     mStatus = (uint16_t)atoi(p);
     if (mStatus == 0) {
       LOG(("mal-formed response status; assuming status = 200\n"));
@@ -394,6 +423,7 @@ void nsHttpResponseHead::ParseStatusLine_locked(const nsACString& line) {
 
   LOG1(("Have status line [version=%u status=%u statusText=%s]\n",
         unsigned(mVersion), unsigned(mStatus), mStatusText.get()));
+  return NS_OK;
 }
 
 nsresult nsHttpResponseHead::ParseHeaderLine(const nsACString& line) {

netwerk/protocol/http/nsHttpResponseHead.h

@@ -101,7 +101,7 @@ class nsHttpResponseHead {
   [[nodiscard]] nsresult ParseCachedOriginalHeaders(char* block);
 
   // parse the status line.
-  void ParseStatusLine(const nsACString& line);
+  nsresult ParseStatusLine(const nsACString& line);
 
   // parse a header line.
   [[nodiscard]] nsresult ParseHeaderLine(const nsACString& line);
@@ -160,7 +160,7 @@ class nsHttpResponseHead {
   nsresult ParseResponseContentLength(const nsACString& aHeaderStr)
       MOZ_REQUIRES(mRecursiveMutex);
 
-  void ParseStatusLine_locked(const nsACString& line)
+  nsresult ParseStatusLine_locked(const nsACString& line)
       MOZ_REQUIRES(mRecursiveMutex);
   [[nodiscard]] nsresult ParseHeaderLine_locked(const nsACString& line,
                                                 bool originalFromNetHeaders)

netwerk/protocol/http/nsHttpTransaction.cpp

@@ -1961,8 +1961,10 @@ nsresult nsHttpTransaction::ParseLine(nsACString& line) {
   nsresult rv = NS_OK;
 
   if (!mHaveStatusLine) {
-    mResponseHead->ParseStatusLine(line);
-    mHaveStatusLine = true;
+    rv = mResponseHead->ParseStatusLine(line);
+    if (NS_SUCCEEDED(rv)) {
+      mHaveStatusLine = true;
+    }
     // XXX this should probably never happen
     if (mResponseHead->Version() == HttpVersion::v0_9) mHaveAllHeaders = true;
   } else {
@@ -2086,7 +2088,9 @@ nsresult nsHttpTransaction::ParseHead(char* buf, uint32_t count,
         // Treat any 0.9 style response of a put as a failure.
         if (mRequestHead->IsPut()) return NS_ERROR_ABORT;
 
-        mResponseHead->ParseStatusLine(""_ns);
+        if (NS_FAILED(mResponseHead->ParseStatusLine(""_ns))) {
+          return NS_ERROR_FAILURE;
+        }
         mHaveStatusLine = true;
         mHaveAllHeaders = true;
         return NS_OK;

netwerk/test/gtest/TestHttpResponseHead.cpp

@@ -4,6 +4,8 @@
 #include "mozilla/net/PHttpChannelParams.h"
 #include "mozilla/Unused.h"
 #include "nsHttp.h"
+#include "nsIPrefBranch.h"
+#include "nsIPrefService.h"
 
 namespace mozilla {
 namespace net {
@@ -44,7 +46,7 @@ TEST(TestHttpResponseHead, Bug1636930)
 {
   nsHttpResponseHead head;
 
-  head.ParseStatusLine("HTTP/1.1 200 OK"_ns);
+  Unused << head.ParseStatusLine("HTTP/1.1 200 OK"_ns);
   Unused << head.ParseHeaderLine("content-type: text/plain"_ns);
   Unused << head.ParseHeaderLine("etag: Just testing"_ns);
   Unused << head.ParseHeaderLine("cache-control: max-age=99999"_ns);
@@ -61,7 +63,7 @@ TEST(TestHttpResponseHead, bug1649807)
 {
   nsHttpResponseHead head;
 
-  head.ParseStatusLine("HTTP/1.1 200 OK"_ns);
+  Unused << head.ParseStatusLine("HTTP/1.1 200 OK"_ns);
   Unused << head.ParseHeaderLine("content-type: text/plain"_ns);
   Unused << head.ParseHeaderLine("etag: Just testing"_ns);
   Unused << head.ParseHeaderLine("cache-control: age=99999"_ns);
@@ -81,7 +83,7 @@ TEST(TestHttpResponseHead, bug1660200)
 {
   nsHttpResponseHead head;
 
-  head.ParseStatusLine("HTTP/1.1 200 OK"_ns);
+  Unused << head.ParseStatusLine("HTTP/1.1 200 OK"_ns);
   Unused << head.ParseHeaderLine("content-type: text/plain"_ns);
   Unused << head.ParseHeaderLine("etag: Just testing"_ns);
   Unused << head.ParseHeaderLine("cache-control: no-cache"_ns);
@@ -94,6 +96,30 @@ TEST(TestHttpResponseHead, bug1660200)
   AssertRoundTrips(head);
 }
 
+TEST(TestHttpResponseHead, bug1687903)
+{
+  nsHttpResponseHead head;
+
+  bool usingStrictParsing = false;
+  nsCOMPtr<nsIPrefBranch> prefs = do_GetService(NS_PREFSERVICE_CONTRACTID);
+  if (prefs) {
+    prefs->GetBoolPref("network.http.strict_response_status_line_parsing",
+                       &usingStrictParsing);
+  }
+
+  nsresult expectation = usingStrictParsing ? NS_ERROR_FAILURE : NS_OK;
+
+  ASSERT_EQ(expectation, head.ParseStatusLine("HTTP/1.1 "_ns));
+  ASSERT_EQ(expectation, head.ParseStatusLine("HTTP/1.1 BLAH"_ns));
+  ASSERT_EQ(expectation, head.ParseStatusLine("HTTP/1.1 1000 BOO"_ns));
+  ASSERT_EQ(expectation, head.ParseStatusLine("HTTP/1.1 0200 BOO"_ns));
+  ASSERT_EQ(expectation, head.ParseStatusLine("HTTP/1.1 60200 200"_ns));
+  ASSERT_EQ(expectation, head.ParseStatusLine("HTTP/1.1 131072 HIOK"_ns));
+  ASSERT_EQ(expectation, head.ParseStatusLine("HTTP/1.1 -200 OK"_ns));
+  ASSERT_EQ(expectation, head.ParseStatusLine("HTTP/1.1 0x9 OK"_ns));
+  ASSERT_EQ(expectation, head.ParseStatusLine("HTTP/1.1 C8 OK"_ns));
+}
+
 TEST(TestHttpResponseHead, atoms)
 {
   // Test that the resolving the content-type atom returns the initial static

testing/web-platform/meta/fetch/h1-parsing/__dir__.ini

@@ -1 +1,2 @@
 leak-threshold: [default:3020800]
+prefs: [network.http.strict_response_status_line_parsing:true]

testing/web-platform/meta/fetch/h1-parsing/lone-cr.window.js.ini

@@ -1,7 +1,4 @@
 [lone-cr.window.html]
-  [Parsing response with a lone CR before message-body (HTTP/1.1\r200 OK\n\nBODY)]
-    expected: FAIL
-
   [Parsing response with a lone CR before message-body (HTTP/1.1 200\rOK\n\nBODY)]
     expected: FAIL
 
@@ -28,4 +25,3 @@
 
   [Parsing response with a lone CR before message-body (HTTP/1.1 200 OK\nHeader: Value\n\r)]
     expected: FAIL
-

testing/web-platform/meta/fetch/h1-parsing/status-code.window.js.ini

@@ -2,35 +2,3 @@
   expected:
     if (os == "android") and debug and fission: [TIMEOUT, OK]
     if (os == "android") and debug and not fission: [OK, TIMEOUT]
-  [HTTP/1.1  (network error)]
-    expected: FAIL
-
-  [HTTP/1.1 BLAH (network error)]
-    expected: FAIL
-
-  [HTTP/1.1 0 OK ]
-    expected: FAIL
-
-  [HTTP/1.1 200 ]
-    expected: FAIL
-
-  [HTTP/1.1 1000 BOO (network error)]
-    expected: FAIL
-
-  [HTTP/1.1 0200 BOO (network error)]
-    expected: FAIL
-
-  [HTTP/1.1 65736 NOT 200 OR SOME SUCH (network error)]
-    expected: FAIL
-
-  [HTTP/1.1 131072 HI (network error)]
-    expected: FAIL
-
-  [HTTP/1.1 -200 TEST (network error)]
-    expected: FAIL
-
-  [HTTP/1.1 0xA (network error)]
-    expected: FAIL
-
-  [HTTP/1.1 C8 (network error)]
-    expected: FAIL

testing/web-platform/meta/wasm/webapi/status.any.js.ini

@@ -1,38 +1,15 @@
 [status.any.html]
   expected:
     if (os == "android") and fission: [OK, TIMEOUT]
-  [Response with status 0: compileStreaming]
-    expected: FAIL
-
-  [Response with status 0: instantiateStreaming]
-    expected: FAIL
-
 
 [status.any.serviceworker.html]
   expected:
     if (os == "android") and fission: [OK, TIMEOUT]
-  [Response with status 0: compileStreaming]
-    expected: FAIL
-
-  [Response with status 0: instantiateStreaming]
-    expected: FAIL
-
 
 [status.any.sharedworker.html]
   expected:
     if (os == "android") and fission: [OK, TIMEOUT]
-  [Response with status 0: compileStreaming]
-    expected: FAIL
-
-  [Response with status 0: instantiateStreaming]
-    expected: FAIL
-
 
 [status.any.worker.html]
   expected:
     if (os == "android") and fission: [OK, TIMEOUT]
-  [Response with status 0: compileStreaming]
-    expected: FAIL
-
-  [Response with status 0: instantiateStreaming]
-    expected: FAIL