[Private Network Access] Merge: Test redirects after preflights.

The change includes eagerly initializing test servers instead of lazily.
This allows the network process to see the same command-line switch as
the browser process, categorizing `b.test` in the `private` IP address
space correctly.

This exercises the bug, where the network service observes a different
IP address space during the preflight and after the redirect.

(cherry picked from commit 5361396)

Bug: chromium:1293891
Change-Id: Ifbec99e8ab900189c54eac8ec68c090cc7472c77
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3440006
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3443384
Commit-Queue: Titouan Rigoudy <titouan@chromium.org>
Cr-Original-Original-Commit-Position: refs/branch-heads/4844@{#304}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3455764
Auto-Submit: Titouan Rigoudy <titouan@chromium.org>
Reviewed-by: Arthur Sonzogni <arthursonzogni@chromium.org>
Commit-Queue: Arthur Sonzogni <arthursonzogni@chromium.org>
Cr-Commit-Position: refs/branch-heads/4758@{#1137}
Cr-Branched-From: 4a2cf4b-refs/heads/main@{#950365}

content/browser/renderer_host/private_network_access_browsertest.cc

@@ -120,53 +120,28 @@ class ContentBrowserClientRegistration {
   const raw_ptr<ContentBrowserClient> old_client_;
 };
 
-// A `net::EmbeddedTestServer` that only starts on demand and pretends to be
-// in a particular IP address space.
+// A `net::EmbeddedTestServer` that pretends to be in a given IP address space.
 //
 // NOTE(titouan): The IP address space overrides CLI switch is copied to utility
-// processes when said processes are started. If we start a lazy server after
-// the network process has started, any updates we make to our own CLI switches
-// will not propagate to the network process, yielding inconsistent results.
-// These tests currently do not rely on this behavior - the IP address space of
-// documents is calculated in the browser process, and all network resources
-// fetched in this test are `local`. If we want to fix this, we can get rid of
-// the fancy lazy-initialization and simply start all servers at test fixture
-// construction time, then set up the CLI switch in `SetUpCommandLine()`.
-class LazyServer {
+// processes when said processes are started. Thus if any server is instantiated
+// after the network process has started, updates we make to our own CLI
+// switches will not propagate to the network process, yielding inconsistent
+// results.
+class FakeAddressSpaceServer {
  public:
-  LazyServer(net::EmbeddedTestServer::Type type,
-             network::mojom::IPAddressSpace ip_address_space,
-             base::FilePath test_data_path)
-      : type_(type),
-        ip_address_space_(ip_address_space),
-        test_data_path_(std::move(test_data_path)) {}
-
-  net::EmbeddedTestServer& Get() {
-    if (!server_) {
-      Initialize();
-    }
-    return *server_;
-  }
-
- private:
-  void Initialize() {
-    server_ = std::make_unique<net::EmbeddedTestServer>(type_);
-
+  FakeAddressSpaceServer(net::EmbeddedTestServer::Type type,
+                         network::mojom::IPAddressSpace ip_address_space,
+                         const base::FilePath& test_data_path)
+      : server_(type) {
     // Use a certificate valid for multiple domains, which we can use to
     // distinguish `local`, `private` and `public` address spaces.
-    server_->SetSSLConfig(net::EmbeddedTestServer::CERT_TEST_NAMES);
+    server_.SetSSLConfig(net::EmbeddedTestServer::CERT_TEST_NAMES);
 
-    server_->AddDefaultHandlers(test_data_path_);
-    EXPECT_TRUE(server_->Start());
-
-    AddCommandLineOverride();
-  }
-
-  // Sets up the command line in order for this server to be considered a part
-  // of `ip_address_space_`, irrespective of the actual IP it binds to.
-  void AddCommandLineOverride() const {
-    DCHECK(server_);
+    server_.AddDefaultHandlers(test_data_path);
+    StartServer(server_);
 
+    // Set up the command line in order for this server to be considered a part
+    // of `ip_address_space`, irrespective of the actual IP it binds to.
     base::CommandLine& command_line = *base::CommandLine::ForCurrentProcess();
     std::string switch_str = command_line.GetSwitchValueASCII(
         network::switches::kIpAddressSpaceOverrides);
@@ -177,15 +152,24 @@ class LazyServer {
     base::StrAppend(&switch_str,
                     {
                         ",",
-                        server_->host_port_pair().ToString(),
+                        server_.host_port_pair().ToString(),
                         "=",
-                        IPAddressSpaceToSwitchValue(ip_address_space_),
+                        IPAddressSpaceToSwitchValue(ip_address_space),
                     });
 
     command_line.AppendSwitchASCII(network::switches::kIpAddressSpaceOverrides,
                                    switch_str);
   }
 
+  net::EmbeddedTestServer& Get() { return server_; }
+
+ private:
+  // Constructor helper.
+  // ASSERT macros can only be used in functions returning void.
+  static void StartServer(net::EmbeddedTestServer& server) {
+    ASSERT_TRUE(server.Start());
+  }
+
   static base::StringPiece IPAddressSpaceToSwitchValue(
       network::mojom::IPAddressSpace space) {
     switch (space) {
@@ -201,10 +185,7 @@ class LazyServer {
     }
   }
 
-  net::EmbeddedTestServer::Type type_;
-  network::mojom::IPAddressSpace ip_address_space_;
-  base::FilePath test_data_path_;
-  std::unique_ptr<net::EmbeddedTestServer> server_;
+  net::EmbeddedTestServer server_;
 };
 
 }  // namespace
@@ -304,12 +285,12 @@ class PrivateNetworkAccessBrowserTestBase : public ContentBrowserTest {
 
   // All servers are started on demand. Most tests require the use of one or
   // two servers, never six at the same time.
-  LazyServer insecure_local_server_;
-  LazyServer insecure_private_server_;
-  LazyServer insecure_public_server_;
-  LazyServer secure_local_server_;
-  LazyServer secure_private_server_;
-  LazyServer secure_public_server_;
+  FakeAddressSpaceServer insecure_local_server_;
+  FakeAddressSpaceServer insecure_private_server_;
+  FakeAddressSpaceServer insecure_public_server_;
+  FakeAddressSpaceServer secure_local_server_;
+  FakeAddressSpaceServer secure_private_server_;
+  FakeAddressSpaceServer secure_public_server_;
 };
 
 // Test with insecure private network subresource requests from the `public`
@@ -2972,6 +2953,20 @@ IN_PROC_BROWSER_TEST_F(PrivateNetworkAccessBrowserTestNoBlocking,
                                                  "", "empty.html"))));
 }
 
+// This test verifies that if a page redirects after responding to a private
+// network request to a server in a different address space, the request does
+// not fail.
+// Regression test for https://crbug.com/1293891.
+IN_PROC_BROWSER_TEST_F(PrivateNetworkAccessBrowserTest, Redirect) {
+  EXPECT_TRUE(NavigateToURL(shell(), SecurePrivateURL(kDefaultPath)));
+
+  GURL target = SecureLocalURL("/server-redirect?" +
+                               SecurePrivateURL(kDefaultPath).spec());
+
+  // TODO(https://crbug.com/1293891): Expect true here.
+  EXPECT_EQ(false, EvalJs(root_frame_host(), FetchSubresourceScript(target)));
+}
+
 // ======================
 // NAVIGATION FETCH TESTS
 // ======================