diff --git a/chrome/browser/plugins/plugin_response_interceptor_url_loader_throttle.cc b/chrome/browser/plugins/plugin_response_interceptor_url_loader_throttle.cc index 4dc383e0f6121..053f3acee5099 100644 --- a/chrome/browser/plugins/plugin_response_interceptor_url_loader_throttle.cc +++ b/chrome/browser/plugins/plugin_response_interceptor_url_loader_throttle.cc @@ -168,7 +168,7 @@ void PluginResponseInterceptorURLLoaderThrottle::WillProcessResponse( // chrome.mimeHandler.abortAndFallbackToNativeHandler() on a prior // navigation for this embedder frame. Peek (not consume) at the // fallback mark so the aborted extension does not re-claim its own - // response across the whole redirect chain -- the mark is cleared in + // response on the reload -- the mark is cleared in // `DidFinishNavigation()` once the re-fetch settles. Route the // application/pdf response to the user agent's built-in PDF viewer. // When the prior stream buffered the response body, take it now and @@ -179,12 +179,12 @@ void PluginResponseInterceptorURLLoaderThrottle::WillProcessResponse( // reload re-fetch from the network. auto* stream_manager = MimeHandlerStreamManager::FromWebContents(web_contents); - if (stream_manager && - stream_manager->IsPendingNativeFallback(frame_tree_node_id_)) { + if (stream_manager && stream_manager->IsPendingNativeFallback( + frame_tree_node_id_, response_url)) { extension_id = extension_misc::kPdfExtensionId; if (chrome_pdf::features::IsOopifPdfEnabled()) { - cached_body = - stream_manager->TakeCachedFallbackBody(frame_tree_node_id_); + cached_body = stream_manager->TakeCachedFallbackBody( + frame_tree_node_id_, response_url); } } } diff --git a/extensions/browser/api/mime_handler/mime_handler_api_unittest.cc b/extensions/browser/api/mime_handler/mime_handler_api_unittest.cc index 229d7b41b29d9..329f96b12f410 100644 --- a/extensions/browser/api/mime_handler/mime_handler_api_unittest.cc +++ b/extensions/browser/api/mime_handler/mime_handler_api_unittest.cc @@ -268,7 +268,7 @@ TEST_F(MimeHandlerApiTest, AbortAndFallbackSuccess) { stream_info->SetDidExtensionFinishNavigation(); const content::FrameTreeNodeId embedder_ftn = embedder->GetFrameTreeNodeId(); - ASSERT_FALSE(manager->IsPendingNativeFallback(embedder_ftn)); + ASSERT_FALSE(manager->IsPendingNativeFallback(embedder_ftn, kOriginalUrl)); auto function = base::MakeRefCounted< MimeHandlerAbortAndFallbackToNativeHandlerFunction>(); @@ -277,7 +277,7 @@ TEST_F(MimeHandlerApiTest, AbortAndFallbackSuccess) { EXPECT_TRUE( api_test_utils::RunFunction(function.get(), "[]", browser_context())); EXPECT_TRUE(function->GetError().empty()) << function->GetError(); - EXPECT_TRUE(manager->IsPendingNativeFallback(embedder_ftn)); + EXPECT_TRUE(manager->IsPendingNativeFallback(embedder_ftn, kOriginalUrl)); } // Built-in MIME handler extensions (e.g. the PDF viewer) are blocked diff --git a/extensions/browser/mime_handler/mime_handler_stream_manager.cc b/extensions/browser/mime_handler/mime_handler_stream_manager.cc index a511871558ab8..09e9f43f255bb 100644 --- a/extensions/browser/mime_handler/mime_handler_stream_manager.cc +++ b/extensions/browser/mime_handler/mime_handler_stream_manager.cc @@ -340,8 +340,8 @@ void MimeHandlerStreamManager::AbortAndFallbackToNativeHandler( stream_info->stream()->GetFallbackDataPipe(); const size_t decoded_body_size = body.is_valid() ? stream_info->stream()->GetCachedBodySize() : 0u; - pending_native_fallback_frames_[embedder_ftn] = - CachedFallbackBody{std::move(body), decoded_body_size}; + pending_native_fallback_frames_[embedder_ftn] = PendingNativeFallback{ + original_url, CachedFallbackBody{std::move(body), decoded_body_size}}; // Re-navigate just the embedder frame -- not the whole WebContents -- // so iframe-hosted MIME handlers fall back without blowing away the @@ -360,19 +360,24 @@ void MimeHandlerStreamManager::AbortAndFallbackToNativeHandler( } bool MimeHandlerStreamManager::IsPendingNativeFallback( - content::FrameTreeNodeId frame_tree_node_id) const { - return pending_native_fallback_frames_.contains(frame_tree_node_id); + content::FrameTreeNodeId frame_tree_node_id, + const GURL& response_url) const { + auto it = pending_native_fallback_frames_.find(frame_tree_node_id); + return it != pending_native_fallback_frames_.end() && + response_url.EqualsIgnoringRef(it->second.original_url); } std::optional MimeHandlerStreamManager::TakeCachedFallbackBody( - content::FrameTreeNodeId frame_tree_node_id) { + content::FrameTreeNodeId frame_tree_node_id, + const GURL& response_url) { auto it = pending_native_fallback_frames_.find(frame_tree_node_id); if (it == pending_native_fallback_frames_.end() || - !it->second.pipe.is_valid()) { + !response_url.EqualsIgnoringRef(it->second.original_url) || + !it->second.body.pipe.is_valid()) { return std::nullopt; } - return std::move(it->second); + return std::move(it->second.body); } bool MimeHandlerStreamManager::PluginCanSave( @@ -568,10 +573,8 @@ void MimeHandlerStreamManager::ReadyToCommitNavigation( void MimeHandlerStreamManager::DidFinishNavigation( content::NavigationHandle* navigation_handle) { // Drop any native-fallback mark for the navigating frame. The mark - // must survive the full redirect chain (so the throttle's peek hits - // on each hop and the PDF extension_id is selected regardless of - // redirects), but by the time the navigation has committed or - // errored the re-fetch is over and the mark is spent. For a canceled + // is held until the re-navigation has committed or errored so the + // throttle can peek it from `WillProcessResponse`. For a canceled // navigation this still fires, so the entry is never leaked. pending_native_fallback_frames_.erase( navigation_handle->GetFrameTreeNodeId()); diff --git a/extensions/browser/mime_handler/mime_handler_stream_manager.h b/extensions/browser/mime_handler/mime_handler_stream_manager.h index 11a8d33327efa..2496bf5e9420b 100644 --- a/extensions/browser/mime_handler/mime_handler_stream_manager.h +++ b/extensions/browser/mime_handler/mime_handler_stream_manager.h @@ -186,12 +186,13 @@ class MimeHandlerStreamManager // Returns true iff `frame_tree_node_id` was previously marked for // native-handler fallback by `AbortAndFallbackToNativeHandler()` and - // the mark has not yet been cleared by navigation completion or frame - // deletion. Non-destructive: redirect chains invoke - // `WillProcessResponse` multiple times, so the mark must survive the - // whole chain. Cleared in `DidFinishNavigation()` / `FrameDeleted()`. - bool IsPendingNativeFallback( - content::FrameTreeNodeId frame_tree_node_id) const; + // `response_url` matches (ignoring ref) the URL the mark was set for. + // The fallback re-navigation may be redirected, so the mark only + // applies when the final response is for the same resource the cached + // body was buffered from. Non-destructive; cleared in + // `DidFinishNavigation()` / `FrameDeleted()`. + bool IsPendingNativeFallback(content::FrameTreeNodeId frame_tree_node_id, + const GURL& response_url) const; // Cached fallback body returned from `TakeCachedFallbackBody`. // `decoded_body_size` is the post-content-decoding byte count of the @@ -204,14 +205,16 @@ class MimeHandlerStreamManager // Moves out the cached response body associated with the // native-fallback mark for `frame_tree_node_id`, if any. Returns - // `std::nullopt` when the mark is absent, no body was buffered, or - // the body has already been taken by a previous call. Single-use: - // the underlying mojo data pipe consumer handle can only be drained - // once, so callers must invoke this only after committing to splicing - // the body. The mark itself is left in place; clearing happens in - // `DidFinishNavigation()` / `FrameDeleted()`. + // `std::nullopt` when the mark is absent, `response_url` does not + // match (ignoring ref) the URL the body was buffered for, no body was + // buffered, or the body has already been taken by a previous call. + // Single-use: the underlying mojo data pipe consumer handle can only + // be drained once, so callers must invoke this only after committing + // to splicing the body. The mark itself is left in place; clearing + // happens in `DidFinishNavigation()` / `FrameDeleted()`. std::optional TakeCachedFallbackBody( - content::FrameTreeNodeId frame_tree_node_id); + content::FrameTreeNodeId frame_tree_node_id, + const GURL& response_url); // Returns whether the handler plugin should handle save events. bool PluginCanSave(const content::RenderFrameHost* embedder_host) const; @@ -363,15 +366,24 @@ class MimeHandlerStreamManager // Stores stream info by embedder host info. StreamInfoMap stream_infos_; + // Value type for `pending_native_fallback_frames_`. `original_url` is + // the URL the cached body was buffered for; lookups must match it so a + // redirected re-navigation does not replay the body under a different + // committed URL. + struct PendingNativeFallback { + GURL original_url; + CachedFallbackBody body; + }; + // Embedder frames marked for native-handler fallback whose pending // re-navigation has not yet completed, mapped to the stream's cached // response body (invalid handle when no body was buffered or the body - // has already been taken). Keyed by `FrameTreeNodeId` (not URL) so two + // has already been taken). Keyed by `FrameTreeNodeId` so two // concurrent iframes handling the same URL are distinguished, and so // the mark survives cross-process RFH swaps during the scoped // re-navigation (the FTN persists across same-frame navigation; only // RFHs within it are replaced). - base::flat_map + base::flat_map pending_native_fallback_frames_; // Needed to avoid use-after-free when setting up beforeunload API support. diff --git a/extensions/browser/mime_handler/mime_handler_stream_manager_unittest.cc b/extensions/browser/mime_handler/mime_handler_stream_manager_unittest.cc index 25c425ff6272d..21e2c8cbd7487 100644 --- a/extensions/browser/mime_handler/mime_handler_stream_manager_unittest.cc +++ b/extensions/browser/mime_handler/mime_handler_stream_manager_unittest.cc @@ -1101,17 +1101,20 @@ TEST_F(MimeHandlerStreamManagerTest, ASSERT_TRUE(stream_info); stream_info->SetDidExtensionFinishNavigation(); - EXPECT_FALSE(manager->IsPendingNativeFallback(embedder_ftn)); + EXPECT_FALSE( + manager->IsPendingNativeFallback(embedder_ftn, GURL(kOriginalUrl1))); manager->AbortAndFallbackToNativeHandler(embedder_host); - // Peek is non-destructive -- the throttle's `WillProcessResponse` - // may fire multiple times in a single re-navigation (redirect chain), - // so the mark must survive until the navigation completes. - EXPECT_TRUE(manager->IsPendingNativeFallback(embedder_ftn)); - EXPECT_TRUE(manager->IsPendingNativeFallback(embedder_ftn)); + // Peek is non-destructive -- the mark must survive until the + // navigation completes. + EXPECT_TRUE( + manager->IsPendingNativeFallback(embedder_ftn, GURL(kOriginalUrl1))); + EXPECT_TRUE( + manager->IsPendingNativeFallback(embedder_ftn, GURL(kOriginalUrl1))); // A different frame is never marked. - EXPECT_FALSE(manager->IsPendingNativeFallback(content::FrameTreeNodeId())); + EXPECT_FALSE(manager->IsPendingNativeFallback(content::FrameTreeNodeId(), + GURL(kOriginalUrl1))); } TEST_F(MimeHandlerStreamManagerTest, @@ -1131,14 +1134,16 @@ TEST_F(MimeHandlerStreamManagerTest, stream_info->SetDidExtensionFinishNavigation(); manager->AbortAndFallbackToNativeHandler(embedder_host); - ASSERT_TRUE(manager->IsPendingNativeFallback(embedder_ftn)); + ASSERT_TRUE( + manager->IsPendingNativeFallback(embedder_ftn, GURL(kOriginalUrl1))); // `DidFinishNavigation` on the embedder FTN clears the mark -- // committed or errored, the re-fetch is over. NiceMock finish_handle(web_contents()); finish_handle.set_render_frame_host(embedder_host); manager->DidFinishNavigation(&finish_handle); - EXPECT_FALSE(manager->IsPendingNativeFallback(embedder_ftn)); + EXPECT_FALSE( + manager->IsPendingNativeFallback(embedder_ftn, GURL(kOriginalUrl1))); } TEST_F(MimeHandlerStreamManagerTest, @@ -1161,8 +1166,11 @@ TEST_F(MimeHandlerStreamManagerTest, stream_info->SetDidExtensionFinishNavigation(); manager->AbortAndFallbackToNativeHandler(embedder_host); - ASSERT_TRUE(manager->IsPendingNativeFallback(embedder_ftn)); - EXPECT_FALSE(manager->TakeCachedFallbackBody(embedder_ftn).has_value()); + ASSERT_TRUE( + manager->IsPendingNativeFallback(embedder_ftn, GURL(kOriginalUrl1))); + EXPECT_FALSE( + manager->TakeCachedFallbackBody(embedder_ftn, GURL(kOriginalUrl1)) + .has_value()); } TEST_F(MimeHandlerStreamManagerTest, @@ -1200,10 +1208,11 @@ TEST_F(MimeHandlerStreamManagerTest, stream_info->SetDidExtensionFinishNavigation(); manager->AbortAndFallbackToNativeHandler(embedder_host); - ASSERT_TRUE(manager->IsPendingNativeFallback(embedder_ftn)); + ASSERT_TRUE( + manager->IsPendingNativeFallback(embedder_ftn, GURL(kOriginalUrl1))); std::optional taken = - manager->TakeCachedFallbackBody(embedder_ftn); + manager->TakeCachedFallbackBody(embedder_ftn, GURL(kOriginalUrl1)); ASSERT_TRUE(taken.has_value()); ASSERT_TRUE(taken->pipe.is_valid()); EXPECT_EQ(std::string_view(kBody).size(), taken->decoded_body_size); @@ -1214,8 +1223,74 @@ TEST_F(MimeHandlerStreamManagerTest, // The mark stays in place until `DidFinishNavigation`/`FrameDeleted`, // but the body is single-use. - EXPECT_TRUE(manager->IsPendingNativeFallback(embedder_ftn)); - EXPECT_FALSE(manager->TakeCachedFallbackBody(embedder_ftn).has_value()); + EXPECT_TRUE( + manager->IsPendingNativeFallback(embedder_ftn, GURL(kOriginalUrl1))); + EXPECT_FALSE( + manager->TakeCachedFallbackBody(embedder_ftn, GURL(kOriginalUrl1)) + .has_value()); +} + +TEST_F(MimeHandlerStreamManagerTest, + AbortAndFallbackToNativeHandler_RedirectedResponseUrl) { + // The fallback re-navigation may be redirected to a different URL. The + // cached body was buffered for `original_url`, so it must not be replayed + // under the redirected response URL -- the throttle should fall through + // to the network body for the new URL instead. + mojo::ScopedDataPipeProducerHandle producer; + mojo::ScopedDataPipeConsumerHandle consumer; + ASSERT_EQ(MOJO_RESULT_OK, mojo::CreateDataPipe(64u, producer, consumer)); + constexpr char kBody[] = "cached-body-bytes"; + ASSERT_EQ(MOJO_RESULT_OK, + producer->WriteAllData(base::as_byte_span(std::string(kBody)))); + producer.reset(); + + auto cache = + extensions::MimeHandlerBodyCache::Create(std::move(consumer), nullptr); + ASSERT_TRUE(cache); + ASSERT_TRUE(base::test::RunUntil([&] { return cache->is_complete(); })); + + content::RenderFrameHost* embedder_host = + NavigateAndCommit(main_rfh(), GURL(kOriginalUrl1)); + const content::FrameTreeNodeId embedder_ftn = + embedder_host->GetFrameTreeNodeId(); + auto* manager = mime_handler_stream_manager(); + auto stream = extensions::mime_handler::GenerateSampleStreamContainer(1); + stream->SetBodyCache(cache); + manager->AddStreamContainer( + embedder_ftn, "internal_id", std::move(stream), + std::make_unique>()); + manager->ClaimStreamInfoForTesting(embedder_host); + auto* stream_info = manager->GetClaimedStreamInfoForTesting(embedder_host); + ASSERT_TRUE(stream_info); + stream_info->SetDidExtensionFinishNavigation(); + + manager->AbortAndFallbackToNativeHandler(embedder_host); + + // The mark only applies to the URL the body was cached for. A response + // for a different URL (e.g. after a server redirect) is not eligible. + EXPECT_FALSE( + manager->IsPendingNativeFallback(embedder_ftn, GURL(kOriginalUrl2))); + EXPECT_FALSE( + manager->TakeCachedFallbackBody(embedder_ftn, GURL(kOriginalUrl2)) + .has_value()); + + // A fragment-only difference is still the same resource. + const GURL original_url_with_fragment = + GURL(kOriginalUrl1).Resolve("#fragment"); + EXPECT_TRUE(manager->IsPendingNativeFallback(embedder_ftn, + original_url_with_fragment)); + + // The cached body remains available for the matching URL. + ASSERT_TRUE( + manager->IsPendingNativeFallback(embedder_ftn, GURL(kOriginalUrl1))); + std::optional taken = + manager->TakeCachedFallbackBody(embedder_ftn, GURL(kOriginalUrl1)); + ASSERT_TRUE(taken.has_value()); + ASSERT_TRUE(taken->pipe.is_valid()); + StringDrainerClient client; + mojo::DataPipeDrainer drainer(&client, std::move(taken->pipe)); + ASSERT_TRUE(base::test::RunUntil([&] { return client.complete(); })); + EXPECT_EQ(kBody, client.TakeAccumulated()); } TEST_F(MimeHandlerStreamManagerTest, @@ -1224,8 +1299,10 @@ TEST_F(MimeHandlerStreamManagerTest, auto* manager = mime_handler_stream_manager(); ASSERT_TRUE(manager); - EXPECT_FALSE( - manager->TakeCachedFallbackBody(content::FrameTreeNodeId()).has_value()); + EXPECT_FALSE(manager + ->TakeCachedFallbackBody(content::FrameTreeNodeId(), + GURL(kOriginalUrl1)) + .has_value()); } // `MimeHandlerStreamManager::GetTopLevelHandlerExtensionId()` returns nullopt