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
| pending_request->timeout_timer.Stop(); | ||
| pending_request->shared_url_loader_factory = | ||
| pending_request->otr_profile->GetDefaultStoragePartition() | ||
| ->GetURLLoaderFactoryForBrowserProcess(); |
There was a problem hiding this comment.
- Do we really need to create OTR profile for each new request? Creating This seems a bit overkill, because even if nothing is stored on disk, the CPU usage can still be high. A lot of KeyedServices are created for each profile.
- Do we really need to use OTR profile for these
SimpleURLLoaderrequests? Can we use use a single OTR profile for cookie fetch and then use the cookie in the normal profile?
There was a problem hiding this comment.
Do we really need to create OTR profile for each new request? Creating This seems a bit overkill, because even if nothing is stored on disk, the CPU usage can still be high. A lot of KeyedServices are created for each profile.
Creating a fresh OTR profile makes it easy to ensure that state is not shared between individual requests, which is a desirable property. Are there a lot of KeyedServices that are created specifically for an OTR profile (not just redirected to the service for the original profile)?
Do we really need to use OTR profile for these SimpleURLLoader requests? Can we use use a single OTR profile for cookie fetch and then use the cookie in the normal profile?
We need the cookie state for subsequent requests. I'm not sure how we would use those cookies when making a request with the normal profile, unless we stored them in the normal profile CookieManager or we override the cookie header. Using an OTR profile for the SimpleURLLoader seems more convenient in this respect
|
To summarize, I'd say we can continue with this approach, given that for now this machinery has limited usage (google search results) and the number of potentially created OTR profiles / renderers is estimates as low. Also having in mind the high priority of the problem. So once the lifetime issues found for now are solved, I'm ok to merge. In the follow-ups we have to investigate the possibility of
|
|
[puLL-Merge] - brave/brave-core@27285 DescriptionThis PR adds a new backup results service to Brave Search, allowing it to fetch search results from a backup provider (Google) for use in fallback mixing and Web Discovery Project. The service handles cookie management and rendering through isolated OTR (Off The Record) profiles. Possible Issues
Security Hotspots
ChangesChangesBy filename:
sequenceDiagram
participant E as Extension
participant BS as BackupResultsService
participant OTR as OTR Profile
participant WC as WebContents
participant BP as Backup Provider
E->>BS: FetchBackupResults(url)
BS->>OTR: Create Profile
BS->>WC: Create WebContents
WC->>BP: Initial Request
BP-->>WC: Response with Redirect
WC->>BP: Follow Redirect
BP-->>WC: Final Results
WC-->>BS: Extract HTML
BS-->>E: Return Results
BS->>OTR: Cleanup Profile
|
| 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
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
| } | ||
|
|
||
| std::string potential_tld = base::JoinString( | ||
| std::vector<std::string>(google_it + 1, host_parts.end()), "."); |
There was a problem hiding this comment.
agree with @fmarier.
I think this all can be replaced with a call to GetDomainAndRegistry and splitting the output on the first . symbol. The part before the dot is either google or not, the rest is either an acceptable domain or not.
doing all these joins and splits should not be necessary here.
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: