Bug 1806776 - Set scheme for http-first mode redirect instead of find and replace; r=ckerschb,freddyb

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

dom/security/nsHTTPSOnlyUtils.cpp

@@ -504,11 +504,20 @@ nsHTTPSOnlyUtils::PotentiallyDowngradeHttpsFirstRequest(nsIChannel* aChannel,
   nsresult rv = aChannel->GetURI(getter_AddRefs(uri));
   NS_ENSURE_SUCCESS(rv, nullptr);
 
-  // Only downgrade if the current scheme is (a) https or (b) view-source:https
   nsAutoCString spec;
+  nsCOMPtr<nsIURI> newURI;
+
+  // Only downgrade if the current scheme is (a) https or (b) view-source:https
   if (uri->SchemeIs("https")) {
     rv = uri->GetSpec(spec);
     NS_ENSURE_SUCCESS(rv, nullptr);
+
+    rv = NS_NewURI(getter_AddRefs(newURI), spec);
+    NS_ENSURE_SUCCESS(rv, nullptr);
+
+    rv = NS_MutateURI(newURI).SetScheme("http"_ns).Finalize(
+        getter_AddRefs(newURI));
+    NS_ENSURE_SUCCESS(rv, nullptr);
   } else if (uri->SchemeIs("view-source")) {
     nsCOMPtr<nsINestedURI> nestedURI = do_QueryInterface(uri);
     if (!nestedURI) {
@@ -520,27 +529,23 @@ nsHTTPSOnlyUtils::PotentiallyDowngradeHttpsFirstRequest(nsIChannel* aChannel,
     if (!innerURI || !innerURI->SchemeIs("https")) {
       return nullptr;
     }
+    rv = NS_MutateURI(innerURI).SetScheme("http"_ns).Finalize(
+        getter_AddRefs(innerURI));
+    NS_ENSURE_SUCCESS(rv, nullptr);
+
     nsAutoCString innerSpec;
     rv = innerURI->GetSpec(innerSpec);
     NS_ENSURE_SUCCESS(rv, nullptr);
 
     spec.Append("view-source:");
     spec.Append(innerSpec);
+
+    rv = NS_NewURI(getter_AddRefs(newURI), spec);
+    NS_ENSURE_SUCCESS(rv, nullptr);
   } else {
     return nullptr;
   }
 
-  // Change the scheme to http
-  if (spec.Find("https://") < 0) {
-    MOZ_ASSERT(false, "how can we end up here not dealing with an https: URI?");
-    return nullptr;
-  }
-  spec.ReplaceSubstring("https://", "http://");
-
-  nsCOMPtr<nsIURI> newURI;
-  rv = NS_NewURI(getter_AddRefs(newURI), spec);
-  NS_ENSURE_SUCCESS(rv, nullptr);
-
   // Log downgrade to console
   NS_ConvertUTF8toUTF16 reportSpec(uri->GetSpecOrDefault());
   AutoTArray<nsString, 1> params = {reportSpec};

dom/security/test/https-first/browser_downgrade_view_source.js

@@ -44,10 +44,38 @@ add_task(async function() {
     "view-source:http://"
   );
 
+  await runTest(
+    "URL with query 'downgrade' should be http and leave query params untouched:",
+    `view-source:${TEST_PATH_HTTP}/file_downgrade_view_source.sjs?downgrade&https://httpsfirst.com`,
+    `view-source:${TEST_PATH_HTTP}/file_downgrade_view_source.sjs?downgrade&https://httpsfirst.com`,
+    "view-source:http://"
+  );
+
   await runTest(
     "URL with query 'upgrade' should be https:",
     `view-source:${TEST_PATH_HTTP}/file_downgrade_view_source.sjs?upgrade`,
     `view-source:${TEST_PATH_HTTPS}/file_downgrade_view_source.sjs?upgrade`,
     "view-source:https://"
   );
+
+  await runTest(
+    "URL with query 'upgrade' should be https:",
+    `view-source:${TEST_PATH_HTTPS}/file_downgrade_view_source.sjs?upgrade`,
+    `view-source:${TEST_PATH_HTTPS}/file_downgrade_view_source.sjs?upgrade`,
+    "view-source:https://"
+  );
+
+  await runTest(
+    "URL with query 'upgrade' should be https and leave query params untouched:",
+    `view-source:${TEST_PATH_HTTP}/file_downgrade_view_source.sjs?upgrade&https://httpsfirst.com`,
+    `view-source:${TEST_PATH_HTTPS}/file_downgrade_view_source.sjs?upgrade&https://httpsfirst.com`,
+    "view-source:https://"
+  );
+
+  await runTest(
+    "URL with query 'upgrade' should be https and leave query params untouched:",
+    `view-source:${TEST_PATH_HTTPS}/file_downgrade_view_source.sjs?upgrade&https://httpsfirst.com`,
+    `view-source:${TEST_PATH_HTTPS}/file_downgrade_view_source.sjs?upgrade&https://httpsfirst.com`,
+    "view-source:https://"
+  );
 });

dom/security/test/https-first/browser_httpsfirst.js

@@ -53,6 +53,12 @@ add_task(async function() {
     "http://"
   );
 
+  await runPrefTest(
+    "http://httpsfirst.com/?https://httpsfirst.com",
+    "Should downgrade after error and leave query params untouched.",
+    "http://httpsfirst.com/?https://httpsfirst.com"
+  );
+
   await runPrefTest(
     "http://domain.does.not.exist",
     "Should not downgrade on dnsNotFound error.",

dom/security/test/https-first/file_downgrade_view_source.sjs

@@ -5,26 +5,26 @@ function handleRequest(request, response) {
   response.setHeader("Cache-Control", "no-cache", false);
   response.setHeader("Content-Type", "text/html", false);
 
-  let query = request.queryString;
+  let query = request.queryString.split("&");
   let scheme = request.scheme;
 
   if (scheme === "https") {
-    if (query === "downgrade") {
+    if (query.includes("downgrade")) {
       response.setStatusLine("1.1", 400, "Bad Request");
       response.write("Bad Request\n");
       return;
     }
-    if (query === "upgrade") {
+    if (query.includes("upgrade")) {
       response.write("view-source:https://");
       return;
     }
   }
 
-  if (scheme === "http" && query === "downgrade") {
+  if (scheme === "http" && query.includes("downgrade")) {
     response.write("view-source:http://");
     return;
   }
 
   // We should arrive here when the redirection was downraded successful
-  response.write("unexpected");
+  response.write("unexpected scheme and query given");
 }