Add hidden renderer for fetching backup search results #27285
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
DJAndries
force-pushed
the
gmix-fix
branch
3 times, most recently
from
0566421 to
f112abc
Compare
That sounds like a great approach to me. |
| request->timeout_timer.Start( | ||
| FROM_HERE, kTimeout, | ||
| base::BindOnce(&BackupResultsServiceImpl::CleanupAndDispatchResult, | ||
| base::Unretained(this), request, std::nullopt)); |
There was a problem hiding this comment.
reported by reviewdog 🐶
[semgrep] base::Unretained is most of the time unrequited, and a weak reference is better suited for secure coding.
Consider swapping Unretained for a weak reference.
base::Unretained usage may be acceptable when a callback owner is guaranteed
to be destroyed with the object base::Unretained is pointing to, for example:
- PrefChangeRegistrar
- base::*Timer
- mojo::Receiver
- any other class member destroyed when the class is deallocated
Source: https://github.com/brave/security-action/blob/main/assets/semgrep_rules/client/chromium-uaf.yaml
Cc @thypon @goodov @iefremov
| pending_request->simple_url_loader->DownloadToString( | ||
| pending_request->shared_url_loader_factory.get(), | ||
| base::BindOnce(&BackupResultsServiceImpl::HandleURLLoaderResponse, | ||
| base::Unretained(this), pending_request), |
There was a problem hiding this comment.
reported by reviewdog 🐶
[semgrep] base::Unretained is most of the time unrequited, and a weak reference is better suited for secure coding.
Consider swapping Unretained for a weak reference.
base::Unretained usage may be acceptable when a callback owner is guaranteed
to be destroyed with the object base::Unretained is pointing to, for example:
- PrefChangeRegistrar
- base::*Timer
- mojo::Receiver
- any other class member destroyed when the class is deallocated
Source: https://github.com/brave/security-action/blob/main/assets/semgrep_rules/client/chromium-uaf.yaml
Cc @thypon @goodov @iefremov
github-actions
bot
added
the
CI/run-audit-deps
|
[puLL-Merge] - brave/brave-core@27285 Here's my review of the PR: DescriptionThis PR introduces a new BackupResultsService for handling backup search results in Brave Search. The service provides functionality to fetch search results from backup providers (like Google) in a secure way, supporting both direct fetching and full page rendering modes. This is used for Brave Search fallback mixing (GMix) and Web Discovery Project features. Security Hotspots
ChangesChangesBy file: browser/brave_search/
components/brave_search/browser/
browser/extensions/api/
Misc Changes
The PR adds significant new functionality around search backup handling while maintaining security through proper URL validation and profile isolation.```mermaid |
|
@DJAndries can you please file follow-up issues/issues ? |
| ] | ||
| } | ||
|
|
||
| if (!is_android) { |
There was a problem hiding this comment.
why does this test not run on android? In a follow-up please convert it to run on android. Also fyi using !is_android around browser tests is not an alternative to adding tests to android_browser_tests.gni. There is code owner for that file specifically to enforce that new tests run on android where applicable.
|
Released in v1.76.41 |
|
Verification PASSED on
|
Example |
Example |
Example |
Example |
|---|---|---|---|
 |
 |
 |
 |
Test Case #2 - Web Discovery on Desktop
Using the STR/Cases mentioned via #27285 (comment), ensured that:
- enabled
WDPviabrave://settings/search - visited
brave://inspectand clicked onInspectviaBrave chrome-extension://mnojpmjdmbbfmejpflffifhffcmidifd/_generated_background_page.html - within
Dev Toolsvia theConsole, pasted the following and waited ~10s:
WDP.modules['web-discovery-project'].background.webDiscoveryProject.patternsLoader.resourceWatcher.forceUpdate()
- visited/accessed https://www.google.com/search?q=this+is+a+test+query and then closed the tab
- waited ~20s and cleared the current log via Fiddler and then pasted the following into the
Consoleas per the following:
WDP.modules['web-discovery-project'].background.webDiscoveryProject.strictQueries.map(x=>x.tDiff=0)
- waited ~10s and ensured there was two
google.comrequests logged with the second being over50kbs
Example |
Example |
Example |
Example |
|---|---|---|---|
 |
 |
 |
 |
Verification PASSED on Pixel 6 Pro running Android 16 using the following build(s):
Brave | 1.76.42 Chromium: 133.0.6943.27 (Official Build) canary (64-bit)
--- | ---
Revision | d76c74413094ca56b777b7efbffe99d6d63ceb48
OS | Android 15; Build/BP22.250103.008; 35; Baklava
Test Case #1 - Brave Search fallback on Android
Using the STR/Cases mentioned via #27285 (comment), ensured that:
- enabled
Google Fallback Mixingviasearch.brave.software - connected the
Androiddevice to the machine and ensured that it was being displayed underbrave://inspect- ensured the website and the service worker were both appearing
- clicked on
Inspecton the service worker - within the
DevToolsviaconsole, pasted/dumped the following and ensured the response was larger than50kbs
await brave.fetchBackupResults('test query', 'en', '', '', true, 0, '')
Example |
Example |
Example |
|---|---|---|
 |
 |
 |
Resolves brave/brave-browser#43399
Security review: https://github.com/brave/reviews/issues/1843
Submitter Checklist:
QA/YesorQA/No;release-notes/includeorrelease-notes/exclude;OS/...) to the associated issuenpm run test -- brave_browser_tests,npm run test -- brave_unit_testswikinpm run presubmitwiki,npm run gn_check,npm run tslintgit rebase master(if needed)Reviewer Checklist:
gnAfter-merge Checklist:
changes has landed on
Test Plan:
For Brave Search fallback on Desktop:
For Brave Search fallback on Android:
await brave.fetchBackupResults('test query', 'en', '', '', true, 0, '')For Web Discovery on Desktop:
WDP.modules['web-discovery-project'].background.webDiscoveryProject.patternsLoader.resourceWatcher.forceUpdate(), wait 10 secondsWDP.modules['web-discovery-project'].background.webDiscoveryProject.strictQueries.map(x=>x.tDiff=0)in the extension DevTools console, wait up to 20 seconds