Open Leo from search #26082
Open Leo from search #26082
Conversation
github-actions
bot
added
CI/storybook-url
| kOsDesktop | kOsAndroid, \ | ||
| FEATURE_VALUE_TYPE(ai_chat::features::kPageContentRefine), \ | ||
| }) | ||
| #define BRAVE_AI_CHAT_FEATURE_ENTRIES \ |
There was a problem hiding this comment.
This is changed because BRAVE_AI_CHAT would cause some naming collision.
It also seems better grouping these together rather than separate defines anyway.
yrliou
force-pushed
the
open_leo_from_search
branch
3 times, most recently
from
dce4a3c
to
1f2c4c1
Compare
|
A Storybook has been deployed to preview UI for the latest push |
yrliou
force-pushed
the
open_leo_from_search
branch
5 times, most recently
from
c93e8bb
to
0c99be2
Compare
Also adds ValidateOpenLeoButtonNonce in PageContentFetcher
yrliou
force-pushed
the
open_leo_from_search
branch
from
0c99be2
to
9ef822a
Compare
| <!-- AI chat setting --> | ||
| <message name="IDS_SETTINGS_SITE_SETTINGS_BRAVE_AI_CHAT" desc="Label for Leo AI chat site settings."> | ||
| Leo AI chat | ||
| </message> | ||
| <message name="IDS_SETTINGS_SITE_SETTINGS_BRAVE_AI_CHAT_ASK" desc="Label for Leo AI chat site settings."> | ||
| Sites can ask to open Leo AI chat | ||
| </message> | ||
| <message name="IDS_SETTINGS_SITE_SETTINGS_BRAVE_AI_CHAT_BLOCK" desc="Label for Leo AI chat site settings."> | ||
| Don't allow site to open Leo AI chat | ||
| </message> |
There was a problem hiding this comment.
The three entries are using "Label for Leo AI chat site settings." Can it be a bit more specific so the person doing the translation has a better idea what the distinction between these texts are,
| std::string script = R"( | ||
| var link = document.getElementById('continue-with-leo') | ||
| var url = new URL(link.href) | ||
| url.port = '$1' | ||
| link.href = url.href | ||
| link.click() | ||
| )"; | ||
| std::string port = base::NumberToString(https_server_.port()); | ||
|
|
||
| ASSERT_TRUE(content::ExecJs( | ||
| GetActiveWebContents()->GetPrimaryMainFrame(), | ||
| base::ReplaceStringPlaceholders(script, {port}, nullptr))); |
There was a problem hiding this comment.
Maybe this could be just:
ASSERT_TRUE(content::ExecJs(
GetActiveWebContents()->GetPrimaryMainFrame(), content::JsReplace(R"(
var link = document.getElementById('continue-with-leo')
var url = new URL(link.href)
url.port = '$1'
link.href = url.href
link.click()
)", https_server_.port()
)));| NavigateToTestPage(FROM_HERE, kBraveSearchHost, kOpenLeoButtonValidPath, | ||
| cur_prompt_count); | ||
| ClickOpenLeoAndCheckLeoOpenedAndNavigationCancelled(FROM_HERE, | ||
| ++cur_prompt_count, true); |
There was a problem hiding this comment.
I think it would help annotate this boolean in these call sites, like:
/*expected_leo_opened=*/true| void ValidateOpenLeoButtonNonce(const base::Location& location, | ||
| const std::string& nonce, | ||
| bool expected_is_valid) { |
There was a problem hiding this comment.
I think it would help readability if we had something like this if we dropped the expected_is_valid and just returned a bool, like:
bool ValidateOpenLeoButtonNonce(const base::Location& location,
const std::string& nonce)So rather than having:
ValidateOpenLeoButtonNonce(FROM_HERE, "5566", false);We would have:
EXPECT_FALSE(ValidateOpenLeoButtonNonce(FROM_HERE, "5566"));| ValidateOpenLeoButtonNonce(FROM_HERE, "", false); | ||
|
|
||
| // Test invalid cases. | ||
| std::vector<std::string> invalid_cases = { |
There was a problem hiding this comment.
const auto invalid_cases = std::to_array({ ... }));| SCOPED_TRACE(testing::Message() << "Invalid case: " << invalid_case); | ||
| NavigateURL(url); | ||
| ASSERT_TRUE(content::ExecJs(GetActiveWebContents()->GetPrimaryMainFrame(), | ||
| base::ReplaceStringPlaceholders( |
There was a problem hiding this comment.
content::JsReplace ditto
| AIChatBraveSearchThrottleDelegateImpl( | ||
| const AIChatBraveSearchThrottleDelegateImpl&) = delete; | ||
| AIChatBraveSearchThrottleDelegateImpl& operator=( | ||
| const AIChatBraveSearchThrottleDelegateImpl&) = delete; |
There was a problem hiding this comment.
I think you could remove these constructors, as this class has no data, so there are no concerns in this class about copy.
browser/ui/sidebar/sidebar_utils.cc
Outdated
| @@ -276,4 +277,24 @@ SidebarService::ShowSidebarOption GetDefaultShowSidebarOption( | |||
| return ShowSidebarOption::kShowNever; | |||
| } | |||
|
|
|||
| void ActivatePanelItem(content::WebContents* web_contents, | |||
| SidebarItem::BuiltInItemType panel_item) { | |||
| if (!web_contents) { | |||
There was a problem hiding this comment.
In general, we should avoid unreachable branches. We should have a semantic understanding of each path of control flow. How is this reached? If we don't know, then we should not have a conditional. It may even be worth considering passing WebContents&. However, if we do know why this branch is needed, there should be a comment.
|
[puLL-Merge] - brave/brave-core@26082 DescriptionThis PR implements a new feature for handling Leo AI chat requests from Brave Search. It adds a new permission type for AI chat, updates the UI to include settings for this permission, and implements a throttle mechanism to handle Leo AI chat requests from Brave Search. ChangesChanges
Possible Issues
Security Hotspots
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| base::BindOnce(on_script_executed, nonce, std::move(callback)), | ||
| blink::BackForwardCacheAware::kAllow, | ||
| blink::mojom::WantResultOption::kWantResult, | ||
| blink::mojom::PromiseResultOption::kAwait); |
There was a problem hiding this comment.
reported by reviewdog 🐶
[semgrep] RequestExecuteScript usages should be vet by the security-team.
References:
- https://github.com/brave/brave-browser/wiki/Security-reviews (point 13)
Source: https://github.com/brave/security-action/blob/main/assets/semgrep_rules/client/brave-execute-script.yaml
Cc @thypon @diracdeltas @bridiver
| return; | ||
| } | ||
| content_extractor_.set_disconnect_handler(base::BindOnce( | ||
| &PageContentFetcherInternal::DeleteSelf, base::Unretained(this))); |
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
yrliou
force-pushed
the
open_leo_from_search
branch
from
34a1841
to
6c1b3fa
Compare
There was a problem hiding this comment.
Hey @yrliou, I've started taking a look at this - I've left a few comments on the spec but in general its looking pretty good.
- How will Brave search know if Brave supports this API? They can't just show it for all Brave browsers. It shouldn't show for:
- Old versions of Brave
- Versions of Brave with the feature disabled
- Users who have denied the permission
- It feels slightly weird this permission will show for all sites (in site settings) when it can only apply to Brave Search
- I'm a bit confused about what the point of checking the
noncematches whats in the url is? Presumably if someone can change one of them they can change both. I'm just struggling to imagine what the attack scenario would be? - More nit-pickily, I wonder if
data-noncewould be a better attribute? https://developer.mozilla.org/en-US/docs/Learn/HTML/Howto/Use_data_attributes - The spec doesn't seem to specify an id for the
atag (maybe just needs an update)
| @@ -75,6 +75,10 @@ export default function addBraveRoutes(r: Partial<SettingsRoutes>) { | |||
| r.SITE_SETTINGS_LOCALHOST_ACCESS = r.SITE_SETTINGS | |||
| .createChild('localhostAccess') | |||
| } | |||
| const isBraveAIChatFeatureEnabled = loadTimeData.getBoolean('isBraveAIChatFeatureEnabled') | |||
There was a problem hiding this comment.
it feels slightly odd to me that this is going to live in Site Settings - the only site this really applies to is Brave Search, right?
| auto element = render_frame()->GetWebFrame()->GetDocument().GetElementById( | ||
| "continue-with-leo"); |
There was a problem hiding this comment.
Hey this continue-with-leo id doesn't seem to mentioned in the spec
|
|
||
| auto element = render_frame()->GetWebFrame()->GetDocument().GetElementById( | ||
| "continue-with-leo"); | ||
| if (element.IsNull() || !element.HasHTMLTagName("a")) { |
There was a problem hiding this comment.
Maybe a call to QuerySelector would make more sense instead of all this?
render_frame()->GetWebFrame()->GetDocument()->QuerySelector("a#continue-with-leo[href][nonce]")
and then you wouldn't need to check
- The tagName
- Whether there's a href
- Whether there's a nonce
| return; | ||
| } | ||
|
|
||
| GURL url(element.GetAttribute("href").Utf8()); |
There was a problem hiding this comment.
We should probably check whether there is a href first - it looks like GetAttribute will return WTF::g_null_atom when the attribute isn't present and I don't know how that will interact with GURL
(not necessary if we use QuerySelector as above)
There was a problem hiding this comment.
I believe it's covered in test case, it would be empty, hence an invalid URL.
This function correctly runs callback with false without any unexpected behavior or crashes.
| } | ||
|
|
||
| GURL url(element.GetAttribute("href").Utf8()); | ||
| if (!IsOpenLeoButtonFromBraveSearchURL(url) || |
There was a problem hiding this comment.
optional as there would be some StrCat, but this IsOpenLeoButtonFromBraveSearchURL could be made into part of the QuerySelector using a startsWith attribute selector:
[href^="https://search.brave.com/leo#"]
additionally, this check doesn't test whether we have the #$nonce part of the url, just whether the /leo part of the path is present - probably isn't too important though
| // to execute a script to get it. | ||
| blink::WebScriptSource source = | ||
| blink::WebScriptSource(blink::WebString::FromUTF8( | ||
| "document.getElementById('continue-with-leo').nonce")); |
There was a problem hiding this comment.
Interesting, I wonder why that is - does it work with a data-* attribute (i.e. data-nonce)?
I think this would be more idiomatic anyway, as data-* attributes are namespaced for custom attributes
| } | ||
|
|
||
| bool IsOpenLeoButtonFromBraveSearchURL(const GURL& url) { | ||
| return IsBraveSearchURL(url) && url.path_piece() == "/leo"; |
There was a problem hiding this comment.
should we also check:
- There is a fragment
- The fragment is non-empty?
as I understand it from the spec, the OpenLeo url is invalid unless it has a nonce, and the nonce matches whats in the url
| // Check if origin is https://search.brave.com. | ||
| GURL& origin = request_data.requesting_origin; | ||
| if (origin.SchemeIs(url::kHttpsScheme) && | ||
| origin.host_piece() == brave_domains::GetServicesDomain("search")) { |
There was a problem hiding this comment.
nit:
| origin.host_piece() == brave_domains::GetServicesDomain("search")) { | |
| origin.host_piece() == brave_domains::GetServicesDomain(kBraveSearchURLPrefix)) { |
Resolves brave/brave-browser#41711
open_leo_from_search_480p.mov
See Requirements
#2and#3in https://docs.google.com/document/d/1idelFPpUEcKDNcyKYf3M5yw91tuIddjrlYfpaWEJjNk/edit?tab=t.0 for reference.TODO: Open a security review for this PR.
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:
Answer with AI