[iOS] Ignore invalid frameIds

Additionally, update tests to use correctly formatted frame ids.

Fixed: 1098606
Change-Id: I0d0b9cfa473d20433c228863af3a434ffd50b5f1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2264468
Commit-Queue: Mike Dougherty <michaeldo@chromium.org>
Auto-Submit: Mike Dougherty <michaeldo@chromium.org>
Reviewed-by: Eugene But <eugenebut@chromium.org>
Cr-Commit-Position: refs/heads/master@{#782598}

ios/chrome/browser/autofill/form_suggestion_controller_unittest.mm

@@ -257,7 +257,7 @@ TEST_F(FormSuggestionControllerTest,
   SetUpController(@[ [TestSuggestionProvider providerWithSuggestions] ]);
   GURL url("http://foo.com");
   test_web_state_.SetCurrentURL(url);
-  web::FakeWebFrame main_frame("main_frame", /*is_main_frame=*/true, url);
+  web::FakeMainWebFrame main_frame(url);
 
   // Trigger form activity, which should set up the suggestions view.
   autofill::FormActivityParams params;
@@ -280,7 +280,7 @@ TEST_F(FormSuggestionControllerTest, FormActivityBlurShouldBeIgnored) {
   SetUpController(@[ [TestSuggestionProvider providerWithSuggestions] ]);
   GURL url("http://foo.com");
   test_web_state_.SetCurrentURL(url);
-  web::FakeWebFrame main_frame("main_frame", true, url);
+  web::FakeMainWebFrame main_frame(url);
 
   autofill::FormActivityParams params;
   params.form_name = "form";
@@ -300,7 +300,7 @@ TEST_F(FormSuggestionControllerTest,
   SetUpController(@[]);
   GURL url("http://foo.com");
   test_web_state_.SetCurrentURL(url);
-  web::FakeWebFrame main_frame("main_frame", true, url);
+  web::FakeMainWebFrame main_frame(url);
 
   autofill::FormActivityParams params;
   params.form_name = "form";
@@ -329,7 +329,7 @@ TEST_F(FormSuggestionControllerTest,
   SetUpController(@[ provider1, provider2 ]);
   GURL url("http://foo.com");
   test_web_state_.SetCurrentURL(url);
-  web::FakeWebFrame main_frame("main_frame", true, url);
+  web::FakeMainWebFrame main_frame(url);
 
   autofill::FormActivityParams params;
   params.form_name = "form";
@@ -378,7 +378,7 @@ TEST_F(FormSuggestionControllerTest,
   SetUpController(@[ provider1, provider2 ]);
   GURL url("http://foo.com");
   test_web_state_.SetCurrentURL(url);
-  web::FakeWebFrame main_frame("main_frame", true, url);
+  web::FakeMainWebFrame main_frame(url);
 
   autofill::FormActivityParams params;
   params.form_name = "form";
@@ -419,7 +419,7 @@ TEST_F(FormSuggestionControllerTest, SelectingSuggestionShouldNotifyDelegate) {
   SetUpController(@[ provider ]);
   GURL url("http://foo.com");
   test_web_state_.SetCurrentURL(url);
-  web::FakeWebFrame main_frame("main_frame", true, url);
+  web::FakeMainWebFrame main_frame(url);
 
   autofill::FormActivityParams params;
   params.form_name = "form";

ios/chrome/browser/ui/autofill/manual_fill/full_card_requester_unittest.mm

@@ -77,8 +77,7 @@ class PaymentRequestFullCardRequesterTest : public PlatformTest {
     web_state()->SetJSInjectionReceiver(injectionReceiver);
 
     auto frames_manager = std::make_unique<web::FakeWebFramesManager>();
-    auto main_frame = std::make_unique<web::FakeWebFrame>(
-        /*frame_id=*/"main", /*is_main_frame=*/true,
+    auto main_frame = std::make_unique<web::FakeMainWebFrame>(
         /*security_origin=*/GURL());
     frames_manager->AddWebFrame(std::move(main_frame));
     web_state()->SetWebFramesManager(std::move(frames_manager));

ios/chrome/browser/web/font_size_tab_helper_unittest.mm

@@ -51,7 +51,7 @@ class FontSizeTabHelperTest : public PlatformTest {
 
     GURL url("https://example.com");
     web_state_.SetCurrentURL(url);
-    auto main_frame = std::make_unique<web::FakeWebFrame>("frameID", true, url);
+    auto main_frame = std::make_unique<web::FakeMainWebFrame>(url);
     fake_main_frame_ = main_frame.get();
     AddWebFrame(std::move(main_frame));
 
@@ -194,8 +194,7 @@ TEST_F(FontSizeTabHelperTest, ZoomInAllFrames) {
   preferred_content_size_category_ = UIContentSizeCategoryExtraLarge;
 
   std::unique_ptr<web::FakeWebFrame> other_frame =
-      std::make_unique<web::FakeWebFrame>("frameID2", false,
-                                          GURL("https://example.com"));
+      std::make_unique<web::FakeChildWebFrame>(GURL("https://example.com"));
   web::FakeWebFrame* fake_other_frame = other_frame.get();
   AddWebFrame(std::move(other_frame));
 

ios/web/find_in_page/find_in_page_request_unittest.mm

@@ -12,32 +12,19 @@
 #error "This file requires ARC support."
 #endif
 
-namespace {
-const char kOneMatchFrameId[] = "frame_with_one_match";
-const char kTwoMatchesFrameId[] = "frame_with_two_matches";
-}  // namespace
-
 namespace web {
 
 class FindInPageRequestTest : public WebTest {
  protected:
-  // Returns a FakeWebFrame with id |frame_id|.
-  std::unique_ptr<FakeWebFrame> CreateWebFrame(const std::string& frame_id,
-                                               bool is_main_frame) {
-    return std::make_unique<FakeWebFrame>(frame_id, is_main_frame,
-                                          GURL::EmptyGURL());
-  }
-
   FindInPageRequestTest() {
-    auto main_frame = CreateWebFrame(kOneMatchFrameId,
-                                     /*is_main_frame=*/true);
+    auto main_frame = std::make_unique<FakeMainWebFrame>(GURL::EmptyGURL());
     request_.AddFrame(main_frame.get());
     auto frame_with_two_matches =
-        CreateWebFrame(kTwoMatchesFrameId, /*is_main_frame=*/false);
+        std::make_unique<FakeChildWebFrame>(GURL::EmptyGURL());
     request_.AddFrame(frame_with_two_matches.get());
     request_.Reset(@"foo", 2);
-    request_.SetMatchCountForFrame(1, kOneMatchFrameId);
-    request_.SetMatchCountForFrame(2, kTwoMatchesFrameId);
+    request_.SetMatchCountForFrame(1, kMainFakeFrameId);
+    request_.SetMatchCountForFrame(2, kChildFakeFrameId);
   }
   FindInPageRequest request_;
 };
@@ -144,7 +131,7 @@ TEST_F(FindInPageRequestTest, RemoveFrame) {
   EXPECT_EQ(3, request_.GetTotalMatchCount());
   EXPECT_EQ(1, request_.GetMatchCountForSelectedFrame());
 
-  request_.RemoveFrame(kOneMatchFrameId);
+  request_.RemoveFrame(kMainFakeFrameId);
 
   EXPECT_EQ(2, request_.GetTotalMatchCount());
 
@@ -186,7 +173,7 @@ TEST_F(FindInPageRequestTest, SetMatchCountForFrame) {
   EXPECT_EQ(3, request_.GetTotalMatchCount());
   EXPECT_EQ(1, request_.GetMatchCountForSelectedFrame());
 
-  request_.SetMatchCountForFrame(5, kTwoMatchesFrameId);
+  request_.SetMatchCountForFrame(5, kChildFakeFrameId);
 
   EXPECT_EQ(6, request_.GetTotalMatchCount());
   EXPECT_EQ(1, request_.GetMatchCountForSelectedFrame());

ios/web/js_messaging/web_frame_util_unittest.mm

@@ -34,14 +34,12 @@ class WebFrameUtilTest : public PlatformTest {
 TEST_F(WebFrameUtilTest, GetMainFrame) {
   // Still no main frame.
   EXPECT_EQ(nullptr, GetMainFrame(&test_web_state_));
-  auto iframe =
-      std::make_unique<FakeWebFrame>("iframe", false, GURL::EmptyGURL());
+  auto iframe = std::make_unique<FakeChildWebFrame>(GURL::EmptyGURL());
   fake_web_frames_manager_->AddWebFrame(std::move(iframe));
   // Still no main frame.
   EXPECT_EQ(nullptr, GetMainFrame(&test_web_state_));
 
-  auto main_frame =
-      std::make_unique<FakeWebFrame>("main_frame", true, GURL::EmptyGURL());
+  auto main_frame = std::make_unique<FakeMainWebFrame>(GURL::EmptyGURL());
   FakeWebFrame* main_frame_ptr = main_frame.get();
   fake_web_frames_manager_->AddWebFrame(std::move(main_frame));
   // Now there is a main frame.
@@ -56,18 +54,16 @@ TEST_F(WebFrameUtilTest, GetMainFrame) {
 TEST_F(WebFrameUtilTest, GetMainWebFrameId) {
   // Still no main frame.
   EXPECT_TRUE(GetMainWebFrameId(&test_web_state_).empty());
-  auto iframe =
-      std::make_unique<FakeWebFrame>("iframe", false, GURL::EmptyGURL());
+  auto iframe = std::make_unique<FakeChildWebFrame>(GURL::EmptyGURL());
   fake_web_frames_manager_->AddWebFrame(std::move(iframe));
   // Still no main frame.
   EXPECT_TRUE(GetMainWebFrameId(&test_web_state_).empty());
 
-  auto main_frame =
-      std::make_unique<FakeWebFrame>("main_frame", true, GURL::EmptyGURL());
+  auto main_frame = std::make_unique<FakeMainWebFrame>(GURL::EmptyGURL());
   FakeWebFrame* main_frame_ptr = main_frame.get();
   fake_web_frames_manager_->AddWebFrame(std::move(main_frame));
   // Now there is a main frame.
-  EXPECT_EQ("main_frame", GetMainWebFrameId(&test_web_state_));
+  EXPECT_EQ(kMainFakeFrameId, GetMainWebFrameId(&test_web_state_));
 
   fake_web_frames_manager_->RemoveWebFrame(main_frame_ptr->GetFrameId());
   // Now there is no main frame.
@@ -77,37 +73,36 @@ TEST_F(WebFrameUtilTest, GetMainWebFrameId) {
 // Tests the GetWebFrameWithId function.
 TEST_F(WebFrameUtilTest, GetWebFrameWithId) {
   // Still no main frame.
-  EXPECT_EQ(nullptr, GetWebFrameWithId(&test_web_state_, "iframe"));
-  EXPECT_EQ(nullptr, GetWebFrameWithId(&test_web_state_, "main_frame"));
+  EXPECT_EQ(nullptr, GetWebFrameWithId(&test_web_state_, kChildFakeFrameId));
+  EXPECT_EQ(nullptr, GetWebFrameWithId(&test_web_state_, kMainFakeFrameId));
   EXPECT_EQ(nullptr, GetWebFrameWithId(&test_web_state_, "unused"));
-  auto iframe =
-      std::make_unique<FakeWebFrame>("iframe", false, GURL::EmptyGURL());
+  auto iframe = std::make_unique<FakeChildWebFrame>(GURL::EmptyGURL());
   FakeWebFrame* iframe_ptr = iframe.get();
   fake_web_frames_manager_->AddWebFrame(std::move(iframe));
   // There is an iframe.
-  EXPECT_EQ(iframe_ptr, GetWebFrameWithId(&test_web_state_, "iframe"));
-  EXPECT_EQ(nullptr, GetWebFrameWithId(&test_web_state_, "main_frame"));
+  EXPECT_EQ(iframe_ptr, GetWebFrameWithId(&test_web_state_, kChildFakeFrameId));
+  EXPECT_EQ(nullptr, GetWebFrameWithId(&test_web_state_, kMainFakeFrameId));
   EXPECT_EQ(nullptr, GetWebFrameWithId(&test_web_state_, "unused"));
 
-  auto main_frame =
-      std::make_unique<FakeWebFrame>("main_frame", true, GURL::EmptyGURL());
+  auto main_frame = std::make_unique<FakeMainWebFrame>(GURL::EmptyGURL());
   FakeWebFrame* main_frame_ptr = main_frame.get();
   fake_web_frames_manager_->AddWebFrame(std::move(main_frame));
   // Now there is a main frame.
-  EXPECT_EQ(iframe_ptr, GetWebFrameWithId(&test_web_state_, "iframe"));
-  EXPECT_EQ(main_frame_ptr, GetWebFrameWithId(&test_web_state_, "main_frame"));
+  EXPECT_EQ(iframe_ptr, GetWebFrameWithId(&test_web_state_, kChildFakeFrameId));
+  EXPECT_EQ(main_frame_ptr,
+            GetWebFrameWithId(&test_web_state_, kMainFakeFrameId));
   EXPECT_EQ(nullptr, GetWebFrameWithId(&test_web_state_, "unused"));
 
   fake_web_frames_manager_->RemoveWebFrame(main_frame_ptr->GetFrameId());
   // Now there is only an iframe.
-  EXPECT_EQ(iframe_ptr, GetWebFrameWithId(&test_web_state_, "iframe"));
-  EXPECT_EQ(nullptr, GetWebFrameWithId(&test_web_state_, "main_frame"));
+  EXPECT_EQ(iframe_ptr, GetWebFrameWithId(&test_web_state_, kChildFakeFrameId));
+  EXPECT_EQ(nullptr, GetWebFrameWithId(&test_web_state_, kMainFakeFrameId));
   EXPECT_EQ(nullptr, GetWebFrameWithId(&test_web_state_, "unused"));
 
   // Now there nothing left.
   fake_web_frames_manager_->RemoveWebFrame(iframe_ptr->GetFrameId());
-  EXPECT_EQ(nullptr, GetWebFrameWithId(&test_web_state_, "iframe"));
-  EXPECT_EQ(nullptr, GetWebFrameWithId(&test_web_state_, "main_frame"));
+  EXPECT_EQ(nullptr, GetWebFrameWithId(&test_web_state_, kChildFakeFrameId));
+  EXPECT_EQ(nullptr, GetWebFrameWithId(&test_web_state_, kMainFakeFrameId));
   EXPECT_EQ(nullptr, GetWebFrameWithId(&test_web_state_, "unused"));
 
   // Test that GetWebFrameWithId returns nullptr for the empty string.
@@ -117,20 +112,18 @@ TEST_F(WebFrameUtilTest, GetWebFrameWithId) {
 // Tests the GetWebFrameId GetWebFrameId function.
 TEST_F(WebFrameUtilTest, GetWebFrameId) {
   EXPECT_EQ(std::string(), GetWebFrameId(nullptr));
-  FakeWebFrame frame("frame", true, GURL::EmptyGURL());
-  EXPECT_EQ("frame", GetWebFrameId(&frame));
+  FakeMainWebFrame frame(GURL::EmptyGURL());
+  EXPECT_EQ(kMainFakeFrameId, GetWebFrameId(&frame));
 }
 
 // Tests the GetAllWebFrames function.
 TEST_F(WebFrameUtilTest, GetAllWebFrames) {
   EXPECT_EQ(0U,
             test_web_state_.GetWebFramesManager()->GetAllWebFrames().size());
-  auto main_frame =
-      std::make_unique<FakeWebFrame>("main_frame", true, GURL::EmptyGURL());
+  auto main_frame = std::make_unique<FakeMainWebFrame>(GURL::EmptyGURL());
   FakeWebFrame* main_frame_ptr = main_frame.get();
   fake_web_frames_manager_->AddWebFrame(std::move(main_frame));
-  auto iframe =
-      std::make_unique<FakeWebFrame>("iframe", false, GURL::EmptyGURL());
+  auto iframe = std::make_unique<FakeChildWebFrame>(GURL::EmptyGURL());
   FakeWebFrame* iframe_ptr = iframe.get();
   fake_web_frames_manager_->AddWebFrame(std::move(iframe));
   std::set<WebFrame*> all_frames =

ios/web/js_messaging/web_frames_manager_impl.mm

@@ -5,6 +5,7 @@
 #include "ios/web/js_messaging/web_frames_manager_impl.h"
 
 #include "base/base64.h"
+#include "base/strings/string_util.h"
 #include "base/strings/sys_string_conversions.h"
 #include "base/strings/utf_string_conversions.h"
 #include "crypto/symmetric_key.h"
@@ -161,6 +162,14 @@ void WebFramesManagerImpl::OnFrameBecameAvailable(WKScriptMessage* message) {
 
   std::string frame_id = base::SysNSStringToUTF8(message.body[@"crwFrameId"]);
   if (!GetFrameWithId(frame_id)) {
+    // Validate |frame_id| is a proper hex string.
+    for (const char& c : frame_id) {
+      if (!base::IsHexDigit(c)) {
+        // Ignore frame if |frame_id| is malformed.
+        return;
+      }
+    }
+
     GURL message_frame_origin =
         web::GURLOriginWithWKSecurityOrigin(message.frameInfo.securityOrigin);
 

ios/web/js_messaging/web_frames_manager_impl_unittest.mm

@@ -20,11 +20,6 @@
 
 namespace {
 
-// Frame ids are base16 string of 128 bit numbers.
-const char kMainFrameId[] = "1effd8f52a067c8d3a01762d3c41dfd1";
-const char kFrame1Id[] = "1effd8f52a067c8d3a01762d3c41dfd2";
-const char kFrame2Id[] = "1effd8f52a067c8d3a01762d3c41dfd3";
-
 // A base64 encoded sample key.
 const char kFrameKey[] = "R7lsXtR74c6R9A9k691gUQ8JAd0be+w//Lntgcbjwrc=";
 
@@ -46,13 +41,13 @@ class WebFramesManagerImplTest : public PlatformTest,
         router_([[CRWWKScriptMessageRouter alloc]
             initWithUserContentController:user_content_controller_]),
         web_view_(OCMClassMock([WKWebView class])),
-        main_frame_(kMainFrameId,
+        main_frame_(kMainFakeFrameId,
                     /*is_main_frame=*/true,
                     GURL("https://www.main.test")),
-        frame_1_(kFrame1Id,
+        frame_1_(kChildFakeFrameId,
                  /*is_main_frame=*/false,
                  GURL("https://www.frame1.test")),
-        frame_2_(kFrame2Id,
+        frame_2_(kChildFakeFrameId2,
                  /*is_main_frame=*/false,
                  GURL("https://www.frame2.test")) {
     // Notify |frames_manager_| that its associated WKWebView has changed from
@@ -163,6 +158,16 @@ TEST_F(WebFramesManagerImplTest, MainWebFrame) {
   EXPECT_FALSE(frames_manager_.GetFrameWithId(main_frame_.GetFrameId()));
 }
 
+// Tests that a WebFrame can not be registered with a malformed frame id.
+TEST_F(WebFramesManagerImplTest, WebFrameWithInvalidId) {
+  FakeWebFrame frame_with_invalid_id(kInvalidFrameId,
+                                     /*is_main_frame=*/true,
+                                     GURL("https://www.main.test"));
+  SendFrameBecameAvailableMessage(frame_with_invalid_id);
+
+  EXPECT_EQ(0ul, frames_manager_.GetAllWebFrames().size());
+}
+
 // Tests multiple web frames construction/destruction.
 TEST_F(WebFramesManagerImplTest, MultipleWebFrame) {
   // Add main frame.

ios/web/public/test/fakes/fake_web_frame.cc

@@ -16,6 +16,12 @@
 
 namespace web {
 
+// Frame ids are base16 string of 128 bit numbers.
+const char kMainFakeFrameId[] = "1effd8f52a067c8d3a01762d3c41dfd1";
+const char kInvalidFrameId[] = "1effd8f52a067c8d3a01762d3c4;dfd1";
+const char kChildFakeFrameId[] = "1effd8f52a067c8d3a01762d3c41dfd2";
+const char kChildFakeFrameId2[] = "1effd8f52a067c8d3a01762d3c41dfd3";
+
 FakeWebFrame::FakeWebFrame(const std::string& frame_id,
                            bool is_main_frame,
                            GURL security_origin)
@@ -81,4 +87,18 @@ void FakeWebFrame::AddJsResultForFunctionCall(
   result_map_[function_name] = std::move(js_result);
 }
 
+// FakeMainWebFrame
+FakeMainWebFrame::FakeMainWebFrame(GURL security_origin)
+    : FakeWebFrame(kMainFakeFrameId, /*is_main_frame=*/true, security_origin) {}
+
+FakeMainWebFrame::~FakeMainWebFrame() {}
+
+// FakeChildWebFrame
+FakeChildWebFrame::FakeChildWebFrame(GURL security_origin)
+    : FakeWebFrame(kChildFakeFrameId,
+                   /*is_main_frame=*/false,
+                   security_origin) {}
+
+FakeChildWebFrame::~FakeChildWebFrame() {}
+
 }  // namespace web

ios/web/public/test/fakes/fake_web_frame.h

@@ -13,8 +13,18 @@
 
 namespace web {
 
+// Frame id constants which can be used to initialize FakeWebFrame.
+// Frame ids are base16 string of 128 bit numbers.
+extern const char kMainFakeFrameId[];
+extern const char kInvalidFrameId[];
+extern const char kChildFakeFrameId[];
+extern const char kChildFakeFrameId2[];
+
+// A fake web frame to use for testing.
 class FakeWebFrame : public WebFrame {
  public:
+  // Creates a web frame. |frame_id| must be a string representing a valid
+  // hexadecimal number.
   FakeWebFrame(const std::string& frame_id,
                bool is_main_frame,
                GURL security_origin);
@@ -77,6 +87,22 @@ class FakeWebFrame : public WebFrame {
   bool can_call_function_ = true;
 };
 
+// A fake web frame representing the main frame with a |frame_id_| of
+// |kMainFakeFrameId|.
+class FakeMainWebFrame : public FakeWebFrame {
+ public:
+  explicit FakeMainWebFrame(GURL security_origin);
+  ~FakeMainWebFrame() override;
+};
+
+// A fake web frame representing a child frame with a |frame_id_| of
+// |kChildFakeFrameId|.
+class FakeChildWebFrame : public FakeWebFrame {
+ public:
+  explicit FakeChildWebFrame(GURL security_origin);
+  ~FakeChildWebFrame() override;
+};
+
 }  // namespace web
 
 #endif  // IOS_WEB_PUBLIC_TEST_FAKES_FAKE_WEB_FRAME_H_

ios/web/web_state/web_state_impl_unittest.mm

@@ -312,7 +312,7 @@ TEST_F(WebStateImplTest, ObserverTest) {
 
   // Test that WebFrameDidBecomeAvailable() is called.
   ASSERT_FALSE(observer->web_frame_available_info());
-  web::FakeWebFrame main_frame("main", true, GURL());
+  web::FakeMainWebFrame main_frame(GURL::EmptyGURL());
   web_state_->OnWebFrameAvailable(&main_frame);
   ASSERT_TRUE(observer->web_frame_available_info());
   EXPECT_EQ(web_state_.get(), observer->web_frame_available_info()->web_state);
@@ -815,7 +815,7 @@ TEST_F(WebStateImplTest, ScriptCommand) {
   value_1.SetString("a", "b");
   const GURL kUrl1("http://foo");
   bool is_called_1 = false;
-  web::FakeWebFrame main_frame("main", true, GURL());
+  web::FakeMainWebFrame main_frame(GURL::EmptyGURL());
   auto subscription_1 = web_state_->AddScriptCommandCallback(
       base::BindRepeating(&HandleScriptCommand, &is_called_1, &value_1, kUrl1,
                           /*expected_user_is_interacting*/ false, &main_frame),
@@ -838,7 +838,7 @@ TEST_F(WebStateImplTest, ScriptCommand) {
   value_3.SetString("e", "f");
   const GURL kUrl3("http://iframe");
   bool is_called_3 = false;
-  web::FakeWebFrame subframe("subframe", false, GURL());
+  web::FakeChildWebFrame subframe(GURL::EmptyGURL());
   auto subscription_3 = web_state_->AddScriptCommandCallback(
       base::BindRepeating(&HandleScriptCommand, &is_called_3, &value_3, kUrl3,
                           /*expected_user_is_interacting*/ false, &subframe),