Search result ad click confirmation. #13444

aseren · 2022-05-24T16:18:30Z

Spec: https://docs.google.com/document/d/1ncuyw0Yv_qawR-FM0_e1r4pNU2pGDhS49KfwfHqqrqs/edit#heading=h.txs9x3akav5

Description is here: https://github.com/brave/security/issues/876

Resolves brave/brave-browser#23002

Submitter Checklist:

I confirm that no security/privacy review is needed, or that I have requested one
There is a ticket for my issue
Used Github auto-closing keywords in the PR description above
Wrote a good PR/commit description
Squashed any review feedback or "fixup" commits before merge, so that history is a record of what happened in the repo, not your PR
Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
Checked the PR locally: npm run test -- brave_browser_tests, npm run test -- brave_unit_tests, npm run lint, npm run gn_check, npm run tslint
Ran git rebase master (if needed)

Reviewer Checklist:

A security review is not needed, or a link to one is included in the PR description
New files have MPL-2.0 license header
Adequate test coverage exists to prevent regressions
Major classes, functions and non-trivial code blocks are well-commented
Changes in component dependencies are properly reflected in gn
Code follows the style guide
Test plan is specified in PR before merging

After-merge Checklist:

Test Plan:

Test case 1

Turn off Brave Private Ads.
Do several search requests on search.brave.com until an ad is shown in a SERP.
Click on the ad link.
Check that the page navigates to https://search.anonymous.ads.brave.com/v3/click and then navigates to the ad landing page using a server redirect.

Test case 2

Turn on Brave Private Ads.
Do several search requests on search.brave.com until an ad is shown in a SERP.
Check that an ad viewed event is triggered. There should be Viewed search result ad with placement id message in rewards-internals log.
Click on the ad link.
Check that the page navigates to the ad landing page without navigation to https://search.anonymous.ads.brave.com/v3/click.
Check that an ad clicked event is triggered. There should be Clicked search result ad with placement id message in rewards-internals log.

components/brave_ads/content/browser/search_result_ad/search_result_ad_info.h

components/brave_ads/content/browser/search_result_ad/search_result_ad_parsing.cc

components/brave_ads/content/browser/search_result_ad/search_result_ad_info.h

browser/brave_content_browser_client.cc

components/brave_ads/content/browser/search_result_ad/search_result_ad_redirect_throttle.cc

fmarier

Updates to chromium_src/net/tools/transport_security_state_generator/input_file_parsers.cc look good.

aseren · 2022-06-06T10:37:54Z

components/brave_ads/content/browser/search_result_ad/search_result_ad_redirect_throttle.cc

+
+  request->url = *search_result_ad_target_url;
+  request->credentials_mode = network::mojom::CredentialsMode::kOmit;
+  request->referrer_policy = net::ReferrerPolicy::NO_REFERRER;


Removed site_for_cookies changing code.
Also added credentials_mode and referrer_policy modification as suggested here: https://github.com/brave/security/issues/876#issuecomment-1139044261

cc @ShivanKaul @diracdeltas @bridiver

@aseren not sure that is necessary. Just to verify, this url is the ad landing page, right? The actual confirmation happens in the background somewhere from MaybeTriggerSearchResultAdClickedEvent, correct?

actually looking more closely I think this functionality is not correct. We should not be navigating to the click confirmation link here, we should be navigating directly to the landing page and the confirmation should happen in the background from a non-profile context

so after talking to @pes10k we don't need to do anything special here in terms of referrer, credentials mode, etc..., but there are other issues here because we have all this extra infrastructure and complications that aren't necessary if we're just following the click-through url. We don't need to parse ad info from the page and associate it with the tab, we can just put everything we need in the url.

we should be navigating directly to the landing page and the confirmation should happen in the background from a non-profile context

just a reminder if this functionality is enabled in Tor windows, then any background requests associated with a confirmation that happens in a Tor window needs to still go through Tor; otherwise the ads server can link the user's real IP address to the ID of an ad that they saw in a Tor window

also seriously please add a test for Tor and private

actually we do have tests for private tab: https://github.com/brave/brave-core/pull/13444/files#diff-e2eb5fe1f2ab08f0a20e5e835b489e48211466cdf25bac21263315e165ffc508R432

and SearchResultAdService won't be created for incognito, see factory code:
https://github.com/brave/brave-core/blob/93a3167ac3722808b044d06d46b7ab727382aa9d/browser/brave_ads/search_result_ad/search_result_ad_service_factory.cc

I did look at the factory code and it will be created for incognito and it will have a null AdsService

The way to prevent it from being created in incognito is to return null in SearchResultAdServiceFactory::BuildServiceInstanceFor

When requesting service we run KeyedServiceFactory::GetServiceForContext https://source.chromium.org/chromium/chromium/src/+/main:components/keyed_service/core/keyed_service_factory.cc;l=56?q=KeyedServiceFactory::GetServiceForContext&ss=chromium%2Fchromium%2Fsrc

GetContextToUse call eventually goes to BrowserContextKeyedServiceFactory::GetBrowserContextToUse which returns nullptr for incognito profiles:
https://source.chromium.org/chromium/chromium/src/+/main:components/keyed_service/content/browser_context_keyed_service_factory.cc;l=64

To create service in incognito we need to override GetBrowserContextToUse method, like here:
https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/safe_browsing/advanced_protection_status_manager_factory.cc;l=52

diracdeltas

still have some concerns about the server side of this but this PR lgtm

components/brave_ads/content/browser/search_result_ad/search_result_ad_redirect_throttle.cc

components/brave_ads/content/browser/search_result_ad/search_result_ad_service.cc

components/brave_ads/content/browser/search_result_ad/search_result_ad_redirect_throttle.cc

components/brave_ads/content/browser/search_result_ad/search_result_ad_service.cc

aseren · 2022-06-13T13:18:42Z

components/brave_ads/content/browser/search_result_ad/search_result_ad_service.cc

+  DCHECK(search_result_ad->target_url.is_valid() &&
+         search_result_ad->target_url.SchemeIs(url::kHttpsScheme));
+
+  TriggerSearchResultAdClickedEvent(search_result_ad->Clone());


Added triggering of an ad click event as it was removed during refactoring.

components/brave_ads/content/browser/search_result_ad/search_result_ad_redirect_throttle.cc

bridiver · 2022-10-11T21:46:00Z

components/brave_ads/content/browser/search_result_ad/search_result_ad_redirect_throttle.cc

+  DCHECK(web_contents);
+
+  if (!search_result_ad_service || !request.request_initiator ||
+      !request.has_user_gesture || !request.is_outermost_main_frame ||


why are you checking user_gesture here?

also if we only care about the main frame, why are you using URLLoaderThrottle instead of NavigationThrottle?

why are you checking user_gesture here?

I thought there was such a requirement. Will double check it.

also if we only care about the main frame, why are you using URLLoaderThrottle instead of NavigationThrottle?

Yes, NavigationThrottle is also possible here. I will change

bridiver · 2022-10-12T00:37:48Z

components/brave_ads/content/browser/search_result_ad/search_result_ad_redirect_throttle.cc

+
+  SessionID tab_id = sessions::SessionTabHelper::IdForTab(web_contents);
+  content::WebContents* original_web_contents =
+      web_contents->GetFirstWebContentsInLiveOriginalOpenerChain();


the tab id does not change so this is not necessary

Link target can be opened in a new tab if it has target="_blank” attribute. In this case, tab id is changed.
Anyway, in the latest code we moved from KeyedService to TabHelper, so we don’t use Tab id values at all.

fix brave/brave-browser#23002

aseren · 2022-11-30T17:19:41Z

Decline the PR, because basing on the latest spec changes, https://search.anonymous.ads.brave.com/v3/click request interception is not required:
https://docs.google.com/document/d/1ncuyw0Yv_qawR-FM0_e1r4pNU2pGDhS49KfwfHqqrqs/edit#heading=h.hnm67cu7xxpw

aseren · 2022-12-01T14:44:58Z

The new PR, which is based on the recent spec changes: #16179

aseren requested review from a team as code owners May 24, 2022 16:18

github-actions bot assigned aseren May 24, 2022

aseren requested a review from tmancey May 24, 2022 16:21

goodov reviewed May 25, 2022

View reviewed changes

aseren force-pushed the issues/23002 branch from 71c41c7 to 93b5117 Compare May 25, 2022 10:04

aseren requested a review from a team as a code owner May 25, 2022 10:04

aseren requested a review from goodov May 25, 2022 10:41

goodov previously approved these changes May 25, 2022

View reviewed changes

aseren mentioned this pull request May 25, 2022

Search ad redirect click #13272

Closed

25 tasks

diracdeltas reviewed May 25, 2022

View reviewed changes

components/brave_ads/content/browser/search_result_ad/search_result_ad_redirect_throttle.cc Outdated Show resolved Hide resolved

diracdeltas added the CI/run-network-audit Run network-audit label May 25, 2022

fmarier previously approved these changes May 25, 2022

View reviewed changes

tmancey previously approved these changes May 27, 2022

View reviewed changes

aseren force-pushed the issues/23002 branch from 750ca5d to f7368c4 Compare June 6, 2022 10:29

aseren commented Jun 6, 2022

View reviewed changes

aseren force-pushed the issues/23002 branch from f7368c4 to 93a3167 Compare June 6, 2022 11:30

diracdeltas previously approved these changes Jun 7, 2022

View reviewed changes

bridiver reviewed Jun 8, 2022

View reviewed changes

components/brave_ads/content/browser/search_result_ad/search_result_ad_redirect_throttle.cc Outdated Show resolved Hide resolved

bridiver reviewed Jun 8, 2022

View reviewed changes

components/brave_ads/content/browser/search_result_ad/search_result_ad_service.cc Outdated Show resolved Hide resolved

bridiver reviewed Jun 8, 2022

View reviewed changes

components/brave_ads/content/browser/search_result_ad/search_result_ad_service.cc Outdated Show resolved Hide resolved

bridiver reviewed Jun 8, 2022

View reviewed changes

components/brave_ads/content/browser/search_result_ad/search_result_ad_redirect_throttle.cc Outdated Show resolved Hide resolved

bridiver reviewed Jun 8, 2022

View reviewed changes

components/brave_ads/content/browser/search_result_ad/search_result_ad_service.cc Outdated Show resolved Hide resolved

aseren force-pushed the issues/23002 branch from 93a3167 to cc4b150 Compare June 13, 2022 13:15

aseren commented Jun 13, 2022

View reviewed changes

components/brave_ads/content/browser/search_result_ad/search_result_ad_redirect_throttle.cc Outdated Show resolved Hide resolved

wknapik dismissed stale reviews from diracdeltas, tmancey, fmarier, and goodov via cc4b150 June 23, 2022 14:31

bridiver reviewed Oct 11, 2022

View reviewed changes

bridiver reviewed Oct 12, 2022

View reviewed changes

aseren force-pushed the issues/23002 branch from cc4b150 to 32a332a Compare October 21, 2022 16:55

aseren requested a review from a team as a code owner October 21, 2022 16:55

aseren force-pushed the issues/23002 branch 2 times, most recently from a9b393a to fd78efc Compare October 22, 2022 00:15

Search result ad click confirmation.

d8bb7c5

fix brave/brave-browser#23002

aseren force-pushed the issues/23002 branch from fd78efc to d8bb7c5 Compare October 24, 2022 15:31

Added SearchResultAdNavigationThrottle.

e9c67b7

fix brave/brave-browser#23002

aseren force-pushed the issues/23002 branch from aa6d9c3 to e9c67b7 Compare October 25, 2022 00:07

aseren mentioned this pull request Oct 27, 2022

Remove viewed confirmation request canceling for search result ad. brave/brave-browser#25977

Closed

aseren closed this Nov 30, 2022

tmancey deleted the issues/23002 branch February 3, 2023 15:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Search result ad click confirmation. #13444

Search result ad click confirmation. #13444

aseren commented May 24, 2022 •

edited

Loading

fmarier left a comment

aseren Jun 6, 2022 •

edited

Loading

bridiver Jun 8, 2022

bridiver Jun 8, 2022

bridiver Jun 8, 2022 •

edited

Loading

diracdeltas Jun 8, 2022 •

edited

Loading

bridiver Jun 8, 2022

aseren Jun 9, 2022

bridiver Jun 9, 2022 •

edited

Loading

bridiver Jun 9, 2022

aseren Jun 10, 2022 •

edited

Loading

diracdeltas left a comment

aseren Jun 13, 2022

bridiver Oct 11, 2022

bridiver Oct 11, 2022

aseren Oct 11, 2022 •

edited

Loading

bridiver Oct 12, 2022

aseren Oct 21, 2022 •

edited

Loading

aseren commented Nov 30, 2022

aseren commented Dec 1, 2022

Search result ad click confirmation. #13444

Search result ad click confirmation. #13444

Conversation

aseren commented May 24, 2022 • edited Loading

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

Test case 1

Test case 2

fmarier left a comment

Choose a reason for hiding this comment

aseren Jun 6, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bridiver Jun 8, 2022 • edited Loading

Choose a reason for hiding this comment

diracdeltas Jun 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bridiver Jun 9, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aseren Jun 10, 2022 • edited Loading

Choose a reason for hiding this comment

diracdeltas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aseren Oct 11, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aseren Oct 21, 2022 • edited Loading

Choose a reason for hiding this comment

aseren commented Nov 30, 2022

aseren commented Dec 1, 2022

aseren commented May 24, 2022 •

edited

Loading

aseren Jun 6, 2022 •

edited

Loading

bridiver Jun 8, 2022 •

edited

Loading

diracdeltas Jun 8, 2022 •

edited

Loading

bridiver Jun 9, 2022 •

edited

Loading

aseren Jun 10, 2022 •

edited

Loading

aseren Oct 11, 2022 •

edited

Loading

aseren Oct 21, 2022 •

edited

Loading