[fuchsia] Fetch CORS-exempt headers list before starting components.

Publishing the fuchsia.web.FrameHost introduces the possibility that
it causes the main web.Context to be created before the list of CORS-
exempt headers has been fetched. This can prevent headers being applied
based on UrlRequestRewriteRules, breaking later users of that Context.

StartComponent() is split into the public API which verifies arguments
and triggers fetch of the CORS exempt headers list, if necessary, and
a separate internal part called when the headers are available, which
actually creates the FrameHostComponent, or PendingCastComponent.

Bug: 1150488
Bug: b/173386485
Change-Id: I2f294e9f0acbf43c971004882461c0f38a4c37b5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2547580
Commit-Queue: Wez <wez@chromium.org>
Reviewed-by: Kevin Marshall <kmarshall@chromium.org>
Cr-Commit-Position: refs/heads/master@{#828844}

fuchsia/runners/cast/cast_runner.cc

@@ -76,22 +76,24 @@ const uint16_t kEphemeralRemoteDebuggingPort = 0;
 class FrameHostComponent : public fuchsia::sys::ComponentController {
  public:
   // Creates a FrameHostComponent with lifetime managed by |controller_request|.
-  static void Start(fuchsia::sys::StartupInfo startup_info,
-                    fidl::InterfaceRequest<fuchsia::sys::ComponentController>
-                        controller_request,
-                    fuchsia::web::FrameHost* frame_host_impl) {
-    new FrameHostComponent(std::move(startup_info),
+  static void Start(
+      std::unique_ptr<base::fuchsia::StartupContext> startup_context,
+      fidl::InterfaceRequest<fuchsia::sys::ComponentController>
+          controller_request,
+      fuchsia::web::FrameHost* frame_host_impl) {
+    new FrameHostComponent(std::move(startup_context),
                            std::move(controller_request), frame_host_impl);
   }
 
  private:
-  FrameHostComponent(fuchsia::sys::StartupInfo startup_info,
-                     fidl::InterfaceRequest<fuchsia::sys::ComponentController>
-                         controller_request,
-                     fuchsia::web::FrameHost* frame_host_impl)
-      : startup_context_(std::move(startup_info)),
-        frame_host_binding_(startup_context_.outgoing(), frame_host_impl) {
-    startup_context_.ServeOutgoingDirectory();
+  FrameHostComponent(
+      std::unique_ptr<base::fuchsia::StartupContext> startup_context,
+      fidl::InterfaceRequest<fuchsia::sys::ComponentController>
+          controller_request,
+      fuchsia::web::FrameHost* frame_host_impl)
+      : startup_context_(std::move(startup_context)),
+        frame_host_binding_(startup_context_->outgoing(), frame_host_impl) {
+    startup_context_->ServeOutgoingDirectory();
     binding_.Bind(std::move(controller_request));
     binding_.set_error_handler([this](zx_status_t) { Kill(); });
   }
@@ -104,7 +106,7 @@ class FrameHostComponent : public fuchsia::sys::ComponentController {
     delete this;
   }
 
-  base::fuchsia::StartupContext startup_context_;
+  const std::unique_ptr<base::fuchsia::StartupContext> startup_context_;
   const base::fuchsia::ScopedServiceBinding<fuchsia::web::FrameHost>
       frame_host_binding_;
   fidl::Binding<fuchsia::sys::ComponentController> binding_{this};
@@ -166,27 +168,48 @@ void CastRunner::StartComponent(
     return;
   }
 
-  // TODO(crbug.com/1120914): Remove this once Component Framework v2 can be
-  // used to route fuchsia.web.FrameHost capabilities cleanly.
-  if (enable_frame_host_component_ &&
-      (cast_url.spec() == kFrameHostComponentName)) {
-    FrameHostComponent::Start(std::move(startup_info),
-                              std::move(controller_request),
-                              main_context_.get());
+  auto startup_context =
+      std::make_unique<base::fuchsia::StartupContext>(std::move(startup_info));
+
+  if (cors_exempt_headers_) {
+    StartComponentInternal(cast_url, std::move(startup_context),
+                           std::move(controller_request));
     return;
   }
 
-  pending_components_.emplace(std::make_unique<PendingCastComponent>(
-      this,
-      std::make_unique<base::fuchsia::StartupContext>(std::move(startup_info)),
-      std::move(controller_request), cast_url.GetContent()));
+  // Start a request for the CORS-exempt headers list via the component's
+  // incoming service-directory, unless a request is already in-progress.
+  // This assumes that the set of CORS-exempt headers is the same for all
+  // components hosted by this Runner.
+  if (!cors_exempt_headers_provider_) {
+    startup_context->svc()->Connect(cors_exempt_headers_provider_.NewRequest());
+
+    cors_exempt_headers_provider_.set_error_handler([this](zx_status_t status) {
+      ZX_LOG(ERROR, status) << "CorsExemptHeaderProvider disconnected.";
+      // Clearing queued callbacks closes resources associated with those
+      // component launch requests, effectively causing them to fail.
+      on_have_cors_exempt_headers_.clear();
+    });
+
+    cors_exempt_headers_provider_->GetCorsExemptHeaderNames(
+        [this](std::vector<std::vector<uint8_t>> header_names) {
+          cors_exempt_headers_provider_.Unbind();
+          cors_exempt_headers_ = std::move(header_names);
+          for (auto& callback : on_have_cors_exempt_headers_)
+            std::move(callback).Run();
+          on_have_cors_exempt_headers_.clear();
+        });
+  }
+
+  // Queue the component launch to be resumed once the header list is available.
+  on_have_cors_exempt_headers_.push_back(base::BindOnce(
+      &CastRunner::StartComponentInternal, base::Unretained(this), cast_url,
+      std::move(startup_context), std::move(controller_request)));
 }
 
 void CastRunner::LaunchPendingComponent(PendingCastComponent* pending_component,
                                         CastComponent::Params params) {
-  // Save the list of CORS exemptions so that they can be used in Context
-  // creation parameters.
-  cors_exempt_headers_ = pending_component->TakeCorsExemptHeaders();
+  DCHECK(cors_exempt_headers_);
 
   // TODO(crbug.com/1082821): Remove |web_content_url| once the Cast Streaming
   // Receiver component has been implemented.
@@ -290,8 +313,9 @@ fuchsia::web::CreateContextParams CastRunner::GetCommonContextParams() {
 
   // If there is a list of headers to exempt from CORS checks, pass the list
   // along to the Context.
-  if (!cors_exempt_headers_.empty())
-    params.set_cors_exempt_headers(cors_exempt_headers_);
+  CHECK(cors_exempt_headers_);
+  if (!cors_exempt_headers_->empty())
+    params.set_cors_exempt_headers(*cors_exempt_headers_);
 
   return params;
 }
@@ -431,3 +455,22 @@ void CastRunner::OnMetricsRecorderServiceRequest(
 
   component->startup_context()->svc()->Connect(std::move(request));
 }
+
+void CastRunner::StartComponentInternal(
+    const GURL& url,
+    std::unique_ptr<base::fuchsia::StartupContext> startup_context,
+    fidl::InterfaceRequest<fuchsia::sys::ComponentController>
+        controller_request) {
+  // TODO(crbug.com/1120914): Remove this once Component Framework v2 can be
+  // used to route fuchsia.web.FrameHost capabilities cleanly.
+  if (enable_frame_host_component_ && (url.spec() == kFrameHostComponentName)) {
+    FrameHostComponent::Start(std::move(startup_context),
+                              std::move(controller_request),
+                              main_context_.get());
+    return;
+  }
+
+  pending_components_.emplace(std::make_unique<PendingCastComponent>(
+      this, std::move(startup_context), std::move(controller_request),
+      url.GetContent()));
+}

fuchsia/runners/cast/cast_runner.h

@@ -20,6 +20,7 @@
 #include "base/containers/flat_set.h"
 #include "base/containers/unique_ptr_adapters.h"
 #include "base/fuchsia/startup_context.h"
+#include "base/optional.h"
 #include "fuchsia/runners/cast/cast_component.h"
 #include "fuchsia/runners/cast/pending_cast_component.h"
 
@@ -102,6 +103,14 @@ class CastRunner : public fuchsia::sys::Runner,
   void OnMetricsRecorderServiceRequest(
       fidl::InterfaceRequest<fuchsia::legacymetrics::MetricsRecorder> request);
 
+  // Internal implementation of StartComponent(), called after validating the
+  // component URL and ensuring that CORS-exempt headers have been fetched.
+  void StartComponentInternal(
+      const GURL& url,
+      std::unique_ptr<base::fuchsia::StartupContext> startup_context,
+      fidl::InterfaceRequest<fuchsia::sys::ComponentController>
+          controller_request);
+
   // True if this Runner uses Context(s) with the HEADLESS feature set.
   const bool is_headless_;
 
@@ -128,8 +137,11 @@ class CastRunner : public fuchsia::sys::Runner,
   // True if this Runner should offer the fuchsia.web.FrameHost component.
   bool enable_frame_host_component_ = false;
 
-  // List of HTTP headers to exempt from CORS checks.
-  std::vector<std::vector<uint8_t>> cors_exempt_headers_;
+  // Used to fetch & cache the list of CORS exempt HTTP headers to configure
+  // each web.Context with.
+  base::Optional<std::vector<std::vector<uint8_t>>> cors_exempt_headers_;
+  chromium::cast::CorsExemptHeaderProviderPtr cors_exempt_headers_provider_;
+  std::vector<base::OnceClosure> on_have_cors_exempt_headers_;
 
   // Last component that was created with permission to access MICROPHONE.
   CastComponent* audio_capturer_component_ = nullptr;

fuchsia/runners/cast/cast_runner_integration_test.cc

@@ -62,11 +62,29 @@ constexpr char kDummyAgentUrl[] =
 
 constexpr char kEnableFrameHostComponent[] = "enable-frame-host-component";
 
+class FakeCorsExemptHeaderProvider
+    : public chromium::cast::CorsExemptHeaderProvider {
+ public:
+  FakeCorsExemptHeaderProvider() = default;
+  ~FakeCorsExemptHeaderProvider() final = default;
+
+  FakeCorsExemptHeaderProvider(const FakeCorsExemptHeaderProvider&) = delete;
+  FakeCorsExemptHeaderProvider& operator=(const FakeCorsExemptHeaderProvider&) =
+      delete;
+
+ private:
+  void GetCorsExemptHeaderNames(
+      GetCorsExemptHeaderNamesCallback callback) final {
+    callback({cr_fuchsia::StringToBytes("Test")});
+  }
+};
+
 class FakeUrlRequestRewriteRulesProvider
     : public chromium::cast::UrlRequestRewriteRulesProvider {
  public:
   FakeUrlRequestRewriteRulesProvider() = default;
-  ~FakeUrlRequestRewriteRulesProvider() override = default;
+  ~FakeUrlRequestRewriteRulesProvider() final = default;
+
   FakeUrlRequestRewriteRulesProvider(
       const FakeUrlRequestRewriteRulesProvider&) = delete;
   FakeUrlRequestRewriteRulesProvider& operator=(
@@ -74,7 +92,7 @@ class FakeUrlRequestRewriteRulesProvider
 
  private:
   void GetUrlRequestRewriteRules(
-      GetUrlRequestRewriteRulesCallback callback) override {
+      GetUrlRequestRewriteRulesCallback callback) final {
     // Only send the rules once. They do not expire
     if (rules_sent_)
       return;
@@ -195,6 +213,7 @@ class FakeComponentState : public cr_fuchsia::AgentImpl::ComponentStateBase {
   base::Optional<base::fuchsia::ScopedServiceBinding<
       chromium::cast::UrlRequestRewriteRulesProvider>>
       url_request_rules_provider_binding_;
+
   FakeApplicationContext application_context_;
   const base::fuchsia::ScopedServiceBinding<chromium::cast::ApplicationContext>
       context_binding_;
@@ -398,10 +417,6 @@ class CastRunnerIntegrationTest : public testing::Test {
   void StartCastComponent(base::StringPiece component_url) {
     // Configure the Runner, including a service directory channel to publish
     // services to.
-    fidl::InterfaceHandle<fuchsia::io::Directory> directory;
-    component_services_.GetOrCreateDirectory("svc")->Serve(
-        fuchsia::io::OPEN_RIGHT_READABLE | fuchsia::io::OPEN_RIGHT_WRITABLE,
-        directory.NewRequest().TakeChannel());
     fuchsia::sys::StartupInfo startup_info;
     startup_info.launch_info.url = component_url.as_string();
 
@@ -418,7 +433,16 @@ class CastRunnerIntegrationTest : public testing::Test {
     component_services_client_ =
         std::make_unique<sys::ServiceDirectory>(std::move(svc_directory));
 
-    // Place the ServiceDirectory in the |flat_namespace|.
+    // Populate |component_services_| with services for the component to use.
+    fidl::InterfaceHandle<fuchsia::io::Directory> directory;
+    component_services_.GetOrCreateDirectory("svc")->Serve(
+        fuchsia::io::OPEN_RIGHT_READABLE | fuchsia::io::OPEN_RIGHT_WRITABLE,
+        directory.NewRequest().TakeChannel());
+    component_services_.AddPublicService(
+        cors_exempt_header_provider_binding_.GetHandler(
+            &cors_exempt_header_provider_));
+
+    // Provide the directory of services in the |flat_namespace|.
     startup_info.flat_namespace.paths.emplace_back(base::kServiceDirectoryPath);
     startup_info.flat_namespace.directories.emplace_back(
         directory.TakeChannel());
@@ -533,6 +557,10 @@ class CastRunnerIntegrationTest : public testing::Test {
   std::unique_ptr<FakeUrlRequestRewriteRulesProvider>
       url_request_rewrite_rules_provider_;
 
+  FakeCorsExemptHeaderProvider cors_exempt_header_provider_;
+  fidl::BindingSet<chromium::cast::CorsExemptHeaderProvider>
+      cors_exempt_header_provider_binding_;
+
   // Incoming service directory, ComponentContext and per-component state.
   sys::OutgoingDirectory component_services_;
   base::fuchsia::ScopedServiceBinding<chromium::cast::ApplicationConfigManager>
@@ -1098,6 +1126,25 @@ TEST_F(CastRunnerIntegrationTest,
   EXPECT_EQ(ExecuteJavaScript("document.title"), "absent");
 }
 
+// Verifies that starting a component fails if CORS exempt headers cannot be
+// fetched.
+TEST_F(CastRunnerIntegrationTest, MissingCorsExemptHeaderProvider) {
+  GURL app_url = test_server_.GetURL(kBlankAppUrl);
+  app_config_manager_.AddApp(kTestAppId, app_url);
+
+  // Start the Cast component, and wait for the controller to disconnect.
+  StartCastComponent(base::StringPrintf("cast:%s", kTestAppId));
+
+  base::RunLoop run_loop;
+  component_controller_.set_error_handler([&run_loop](zx_status_t status) {
+    EXPECT_EQ(status, ZX_ERR_PEER_CLOSED);
+    run_loop.Quit();
+  });
+  run_loop.Run();
+
+  EXPECT_TRUE(!component_state_);
+}
+
 class CastRunnerFrameHostIntegrationTest : public CastRunnerIntegrationTest {
  public:
   CastRunnerFrameHostIntegrationTest()

fuchsia/runners/cast/pending_cast_component.cc

@@ -44,43 +44,10 @@ PendingCastComponent::PendingCastComponent(
   // API and remove the AgentManager.
   params_.agent_manager = std::make_unique<cr_fuchsia::AgentManager>(
       params_.startup_context->component_context()->svc().get());
-
-  RequestCorsExemptHeaders();
 }
 
 PendingCastComponent::~PendingCastComponent() = default;
 
-std::vector<std::vector<uint8_t>>
-PendingCastComponent::TakeCorsExemptHeaders() {
-  std::vector<std::vector<uint8_t>> headers;
-  cors_exempt_headers_->swap(headers);
-  return headers;
-}
-
-void PendingCastComponent::RequestCorsExemptHeaders() {
-  params_.startup_context->svc()->Connect(
-      cors_exempt_headers_provider_.NewRequest());
-
-  cors_exempt_headers_provider_.set_error_handler([this](zx_status_t status) {
-    if (status != ZX_ERR_PEER_CLOSED) {
-      ZX_LOG(ERROR, status) << "CorsExemptHeaderProvider disconnected.";
-      delegate_->CancelPendingComponent(this);
-      return;
-    }
-
-    ZX_LOG(WARNING, status) << "CorsExemptHeaderProvider unsupported.";
-    cors_exempt_headers_.emplace();  // Set an empty header list.
-    MaybeLaunchComponent();
-  });
-
-  cors_exempt_headers_provider_->GetCorsExemptHeaderNames(
-      [this](std::vector<std::vector<uint8_t>> header_names) {
-        cors_exempt_headers_provider_.Unbind();
-        cors_exempt_headers_ = header_names;
-        MaybeLaunchComponent();
-      });
-}
-
 void PendingCastComponent::OnApplicationConfigReceived(
     chromium::cast::ApplicationConfig application_config) {
   if (application_config.IsEmpty()) {
@@ -156,7 +123,7 @@ void PendingCastComponent::OnApiBindingsInitialized() {
 }
 
 void PendingCastComponent::MaybeLaunchComponent() {
-  if (!params_.AreComplete() || !cors_exempt_headers_)
+  if (!params_.AreComplete())
     return;
 
   // Clear the error handlers on InterfacePtr<>s before passing them, to avoid

fuchsia/runners/cast/pending_cast_component.h

@@ -50,9 +50,6 @@ class PendingCastComponent {
   PendingCastComponent(const PendingCastComponent&) = delete;
   PendingCastComponent& operator=(const PendingCastComponent&) = delete;
 
-  // Returns the list of headers which will be exempted from CORS checks.
-  std::vector<std::vector<uint8_t>> TakeCorsExemptHeaders();
-
  private:
   void RequestCorsExemptHeaders();
 
@@ -60,7 +57,6 @@ class PendingCastComponent {
   void OnApplicationConfigReceived(
       chromium::cast::ApplicationConfig app_config);
   void OnApiBindingsInitialized();
-  void OnCorsHeadersReceived(std::vector<std::vector<uint8_t>> header_names);
 
   // Passes |params_| to |delegate_| if they are complete, to use to launch the
   // component. |this| will be deleted before the call returns in that case.
@@ -73,14 +69,9 @@ class PendingCastComponent {
   // Parameters required to construct the CastComponent.
   CastComponent::Params params_;
 
-  // The list of HTTP headers to exempt from CORS checked. The value is used
-  // when a Context is created to host this Component.
-  base::Optional<std::vector<std::vector<uint8_t>>> cors_exempt_headers_;
-
   // Used to receive the media session Id and ApplicationConfig.
   chromium::cast::ApplicationContextPtr application_context_;
   chromium::cast::ApplicationConfigManagerPtr application_config_manager_;
-  chromium::cast::CorsExemptHeaderProviderPtr cors_exempt_headers_provider_;
 };
 
 #endif  // FUCHSIA_RUNNERS_CAST_PENDING_CAST_COMPONENT_H_