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}
| 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
| content::GetUIThreadTaskRunner({})->PostTask( | ||
| FROM_HERE, | ||
| base::BindOnce(&BackupResultsServiceImpl::CleanupAndDispatchResult, | ||
| base::Unretained(this), pending_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
| GURL(*url_str), std::nullopt, | ||
| base::BindOnce( | ||
| &WebDiscoveryRetrieveBackupResultsFunction::HandleBackupResults, | ||
| this)); |
There was a problem hiding this comment.
reported by reviewdog 🐶
[semgrep] BindOnce/BindRepeating may allow callers to access objects which may already be freed in the C++ lifecycle.
Verify the occurrences manually.
Source: https://github.com/brave/security-action/blob/main/assets/semgrep_rules/client/chromium-uaf.yaml
Cc @thypon @goodov @iefremov
| public: | ||
| void DidFinishLoad(content::RenderFrameHost* render_frame_host, | ||
| const GURL& validated_url) override { | ||
| if (!validated_url.SchemeIsHTTPOrHTTPS()) { |
There was a problem hiding this comment.
can limit to only HTTPS here?
There was a problem hiding this comment.
actually we can limit it to only google? the API should not be able to initiate a load to any other origin.
There was a problem hiding this comment.
can limit to only HTTPS here?
done
actually we can limit it to only google? the API should not be able to initiate a load to any other origin.
We may invoke requests using various TLDs depending on the user's locale, which makes this less straightforward. In HandleWebContentsStartRequest, we do ensure that the redirected requests have the same origin as the original URL.
There was a problem hiding this comment.
Shouldn't we do the same thing as WDP extension here?
There was a problem hiding this comment.
@DJAndries can you point to the logic that decides on the TLD based on locale? it would be really bad if we assumed google.$TLD was owned by google and allowed that to go through but it actually wasn't. i think we need to be stricter than the original WDP logic @ShivanKaul given the risks are higher here.
| 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
| content::GetUIThreadTaskRunner({})->PostTask( | ||
| FROM_HERE, | ||
| base::BindOnce(&BackupResultsServiceImpl::CleanupAndDispatchResult, | ||
| base::Unretained(this), pending_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
| GURL(*url_str), std::nullopt, | ||
| base::BindOnce( | ||
| &WebDiscoveryRetrieveBackupResultsFunction::HandleBackupResults, | ||
| this)); |
There was a problem hiding this comment.
reported by reviewdog 🐶
[semgrep] BindOnce/BindRepeating may allow callers to access objects which may already be freed in the C++ lifecycle.
Verify the occurrences manually.
Source: https://github.com/brave/security-action/blob/main/assets/semgrep_rules/client/chromium-uaf.yaml
Cc @thypon @goodov @iefremov
| 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
| content::GetUIThreadTaskRunner({})->PostTask( | ||
| FROM_HERE, | ||
| base::BindOnce(&BackupResultsServiceImpl::CleanupAndDispatchResult, | ||
| base::Unretained(this), pending_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
|
[puLL-Merge] - brave/brave-core@27285 DescriptionThis PR implements a backup results service for Brave Search, allowing it to fetch and process search results from backup providers. It includes functionality for both direct URL fetching and full page rendering modes, with support for cookie handling and request throttling. This is used to support features like fallback mixing (GMix) and Web Discovery Project. Security Hotspots
ChangesChanges
sequenceDiagram
participant Extension
participant BackupResultsService
participant OTRProfile
participant WebContents
participant BackupProvider
Extension->>BackupResultsService: retrieveBackupResults(url)
BackupResultsService->>OTRProfile: Create
alt Full Render Mode
BackupResultsService->>WebContents: Create
WebContents->>BackupProvider: Initial Request
BackupProvider->>WebContents: Response with Redirect
WebContents->>BackupProvider: Follow Redirect
BackupProvider->>WebContents: Final Results
WebContents->>BackupResultsService: Extract HTML
else Direct Mode
BackupResultsService->>BackupProvider: Direct Request
BackupProvider->>BackupResultsService: Results
end
BackupResultsService->>Extension: Return Results
BackupResultsService->>OTRProfile: Cleanup
|
Resolves brave/brave-browser#43399
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: