From 31f256a8d1e04530e19adaed27186f1729c8ac2a Mon Sep 17 00:00:00 2001 From: Terry Mancey Date: Thu, 21 May 2020 16:23:20 +0100 Subject: [PATCH] Fixes Brave Ads crash on iOS for ads with an invalid target URL --- .../src/bat/ads/bundle_state.cc | 2 +- .../src/bat/ads/internal/ads_impl.cc | 2 +- .../src/bat/ads/internal/catalog_state.cc | 8 ++++ .../src/bat/ads/internal/url_util.cc | 13 ------- .../src/bat/ads/internal/url_util.h | 3 -- .../src/bat/ads/internal/url_util_unittest.cc | 39 ------------------- vendor/brave-ios/Ads/BATAdNotification.h | 4 +- vendor/brave-ios/Ads/BATAdNotification.mm | 6 +-- 8 files changed, 15 insertions(+), 62 deletions(-) diff --git a/vendor/bat-native-ads/src/bat/ads/bundle_state.cc b/vendor/bat-native-ads/src/bat/ads/bundle_state.cc index 6d3af29964b6..1e6230d6b774 100644 --- a/vendor/bat-native-ads/src/bat/ads/bundle_state.cc +++ b/vendor/bat-native-ads/src/bat/ads/bundle_state.cc @@ -93,7 +93,7 @@ Result BundleState::FromJson( info.title = creative["title"].GetString(); info.body = creative["body"].GetString(); - info.target_url = GetUrlWithScheme(creative["targetUrl"].GetString()); + info.target_url = creative["targetUrl"].GetString(); info.creative_instance_id = creative["creativeInstanceId"].GetString(); const std::string category = creative_ad_notification.name.GetString(); diff --git a/vendor/bat-native-ads/src/bat/ads/internal/ads_impl.cc b/vendor/bat-native-ads/src/bat/ads/internal/ads_impl.cc index ccda0fbe0c7e..5cf7446e93a7 100644 --- a/vendor/bat-native-ads/src/bat/ads/internal/ads_impl.cc +++ b/vendor/bat-native-ads/src/bat/ads/internal/ads_impl.cc @@ -1181,7 +1181,7 @@ bool AdsImpl::ShowAdNotification( ad_notification->category = info.category; ad_notification->title = info.title; ad_notification->body = info.body; - ad_notification->target_url = GetUrlWithScheme(info.target_url); + ad_notification->target_url = info.target_url; ad_notification->geo_target = info.geo_targets.at(0); BLOG(1, "Ad notification shown:\n" diff --git a/vendor/bat-native-ads/src/bat/ads/internal/catalog_state.cc b/vendor/bat-native-ads/src/bat/ads/internal/catalog_state.cc index 727aaf64bd90..a2130661d5f2 100644 --- a/vendor/bat-native-ads/src/bat/ads/internal/catalog_state.cc +++ b/vendor/bat-native-ads/src/bat/ads/internal/catalog_state.cc @@ -6,6 +6,9 @@ #include "bat/ads/internal/catalog_state.h" #include "bat/ads/internal/json_helper.h" #include "bat/ads/internal/static_values.h" +#include "bat/ads/internal/logging.h" + +#include "url/gurl.h" namespace ads { @@ -157,6 +160,11 @@ Result CatalogState::FromJson( creative_info.payload.body = payload["body"].GetString(); creative_info.payload.title = payload["title"].GetString(); creative_info.payload.target_url = payload["targetUrl"].GetString(); + if (!GURL(creative_info.payload.target_url).is_valid()) { + BLOG(1, "Invalid target URL for creative instance id " + << creative_instance_id); + continue; + } creative_set_info.creative_ad_notifications.push_back(creative_info); } else { diff --git a/vendor/bat-native-ads/src/bat/ads/internal/url_util.cc b/vendor/bat-native-ads/src/bat/ads/internal/url_util.cc index c8f1e0fea316..10515b4985f1 100644 --- a/vendor/bat-native-ads/src/bat/ads/internal/url_util.cc +++ b/vendor/bat-native-ads/src/bat/ads/internal/url_util.cc @@ -14,19 +14,6 @@ namespace ads { -std::string GetUrlWithScheme( - const std::string& url) { - if (!base::StartsWith(url, url::kHttpScheme, - base::CompareCase::INSENSITIVE_ASCII) && - !base::StartsWith(url, url::kHttpsScheme, - base::CompareCase::INSENSITIVE_ASCII)) { - return base::StringPrintf("%s%s%s", url::kHttpsScheme, - url::kStandardSchemeSeparator, url.c_str()); - } - - return url; -} - bool UrlMatchesPattern( const std::string& url, const std::string& pattern) { diff --git a/vendor/bat-native-ads/src/bat/ads/internal/url_util.h b/vendor/bat-native-ads/src/bat/ads/internal/url_util.h index d27c2448aa4b..0e5ecd8c9ceb 100644 --- a/vendor/bat-native-ads/src/bat/ads/internal/url_util.h +++ b/vendor/bat-native-ads/src/bat/ads/internal/url_util.h @@ -12,9 +12,6 @@ namespace ads { -std::string GetUrlWithScheme( - const std::string& url); - bool UrlMatchesPattern( const std::string& url, const std::string& pattern); diff --git a/vendor/bat-native-ads/src/bat/ads/internal/url_util_unittest.cc b/vendor/bat-native-ads/src/bat/ads/internal/url_util_unittest.cc index cef027fbc98c..a9078ae4c852 100644 --- a/vendor/bat-native-ads/src/bat/ads/internal/url_util_unittest.cc +++ b/vendor/bat-native-ads/src/bat/ads/internal/url_util_unittest.cc @@ -37,45 +37,6 @@ class BraveAdsUrlUtilTest : public ::testing::Test { // Objects declared here can be used by all tests in the test case }; -TEST_F(BraveAdsUrlUtilTest, - GetUrlForUrlMissingScheme) { - // Arrange - const std::string url = "www.foobar.com"; - - // Act - const std::string url_with_schema = GetUrlWithScheme(url); - - // Assert - const std::string expected_url_with_scheme = "https://www.foobar.com"; - EXPECT_EQ(expected_url_with_scheme, url_with_schema); -} - -TEST_F(BraveAdsUrlUtilTest, - GetUrlForUrlWithHttpScheme) { - // Arrange - const std::string url = "http://www.foobar.com"; - - // Act - const std::string url_with_schema = GetUrlWithScheme(url); - - // Assert - const std::string expected_url_with_scheme = "http://www.foobar.com"; - EXPECT_EQ(expected_url_with_scheme, url_with_schema); -} - -TEST_F(BraveAdsUrlUtilTest, - GetUrlForUrlWithHttpsScheme) { - // Arrange - const std::string url = "https://www.foobar.com"; - - // Act - const std::string url_with_schema = GetUrlWithScheme(url); - - // Assert - const std::string expected_url_with_scheme = "https://www.foobar.com"; - EXPECT_EQ(expected_url_with_scheme, url_with_schema); -} - TEST_F(BraveAdsUrlUtilTest, UrlMatchesPatternWithNoWildcards) { // Arrange diff --git a/vendor/brave-ios/Ads/BATAdNotification.h b/vendor/brave-ios/Ads/BATAdNotification.h index aca50d7b7436..4f573b12caff 100644 --- a/vendor/brave-ios/Ads/BATAdNotification.h +++ b/vendor/brave-ios/Ads/BATAdNotification.h @@ -27,14 +27,14 @@ NS_SWIFT_NAME(AdsNotification) @property (nonatomic, readonly, copy) NSString *category; @property (nonatomic, readonly, copy) NSString *title; @property (nonatomic, readonly, copy) NSString *body; -@property (nonatomic, readonly, copy) NSURL *targetURL; +@property (nonatomic, readonly, copy) NSString *targetURL; @property (nonatomic, readonly, copy) NSString *geoTarget; @end @interface BATAdNotification (MyFirstAd) + (instancetype)customAdWithTitle:(NSString *)title body:(NSString *)body - url:(NSURL *)url NS_SWIFT_NAME(customAd(title:body:url:)); + url:(NSString *)url NS_SWIFT_NAME(customAd(title:body:url:)); @end NS_ASSUME_NONNULL_END diff --git a/vendor/brave-ios/Ads/BATAdNotification.mm b/vendor/brave-ios/Ads/BATAdNotification.mm index bf332aa918dd..9ae17f186964 100644 --- a/vendor/brave-ios/Ads/BATAdNotification.mm +++ b/vendor/brave-ios/Ads/BATAdNotification.mm @@ -14,7 +14,7 @@ @interface BATAdNotification () @property (nonatomic, copy) NSString *category; @property (nonatomic, copy) NSString *title; @property (nonatomic, copy) NSString *body; -@property (nonatomic, copy) NSURL *targetURL; +@property (nonatomic, copy) NSString *targetURL; @property (nonatomic, copy) NSString *geoTarget; @end @@ -30,7 +30,7 @@ - (instancetype)initWithNotificationInfo:(const ads::AdNotificationInfo&)info self.category = [NSString stringWithUTF8String:info.category.c_str()]; self.title = [NSString stringWithUTF8String:info.title.c_str()]; self.body = [NSString stringWithUTF8String:info.body.c_str()]; - self.targetURL = [NSURL URLWithString:[NSString stringWithUTF8String:info.target_url.c_str()]]; + self.targetURL = [NSString stringWithUTF8String:info.target_url.c_str()]; self.geoTarget = [NSString stringWithUTF8String:info.geo_target.c_str()]; } return self; @@ -39,7 +39,7 @@ - (instancetype)initWithNotificationInfo:(const ads::AdNotificationInfo&)info @end @implementation BATAdNotification (MyFirstAd) -+ (instancetype)customAdWithTitle:(NSString *)title body:(NSString *)body url:(NSURL *)url ++ (instancetype)customAdWithTitle:(NSString *)title body:(NSString *)body url:(NSString *)url { BATAdNotification *notification = [[BATAdNotification alloc] init]; notification.title = title;