GitLab

While reviewing acat's rebase work in #33533 (moved) I noticed the following block in our patch for #23247 (moved):

   if (!parentIsHttps) {
+    nsAutoCString parentHost;
+    rv = innerRequestingLocation->GetHost(parentHost);
+    if (NS_FAILED(rv)) {
+      NS_ERROR("requestingLocation->GetHost failed");
+      *aDecision = REJECT_REQUEST;
+      return NS_OK;
+    }
+
+    bool parentIsOnion =
+        StringEndsWith(parentHost, NS_LITERAL_CSTRING(".onion"));
+    if (!parentIsOnion) {
+      *aDecision = ACCEPT;
+      return NS_OK;
+    }
+  }

The corresponding part in Mozilla's code looks like this:

  if (!parentIsHttps) {
    *aDecision = ACCEPT;
    return NS_OK;
  }

Mozilla does not even check the host but is accepting requests at this point outright while we reject them if GetHost() fails.

I wonder whether we should follow them here because I am a little worried that we might introduce a subtle bug by diverging from them.

I guess the question is whether it can be the case that we have a .onion URL but the GetHost() call is failing. Mozilla does not care as there is no decision to be made depending on the host but the scheme alone in this check. However, we do care.

Let's look at that from a different angle: the code block we added is essentially a check for a .onion domain: if we don't have one, accept the request otherwise pass it on for further checks. Let's look what we do elsewhere when checking for a .onion domain, e.g. in IsPotentiallyTrustworthyOnion(). There we have:

  nsAutoCString host;
  nsresult rv = aURL->GetHost(host);
  NS_ENSURE_SUCCESS(rv, false);
  return StringEndsWith(host, NS_LITERAL_CSTRING(".onion"));

This means if the GetHost() check fails we say "no, that's not a .onion". Following that logic we would get to parentIsOnion = false in our code block in question and we should ACCEPT the request as well in case the GetHost() call fails.