-
Notifications
You must be signed in to change notification settings - Fork 885
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
AI Chat becomes a trusted WebUI with an UntrustedWebUI frame for LLM-generated responses #26855
Conversation
890fbf6
to
2fa7d02
Compare
} | ||
|
||
public addStateChangeListener(callback: (event: Event) => void) { | ||
this.eventTarget.addEventListener('uistatechange', callback) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit:
this.eventTarget.addEventListener('uistatechange', callback) | |
this.eventTarget.addEventListener('statechange', callback) |
import * as React from 'react' | ||
import API from './api' | ||
|
||
export default function useAPIState<T, Y>(api: API<Y>, defaultState: T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 this looks great, I am 100% on board
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe worth
export default function useAPIState<T, Y>(api: API<Y>, defaultState: T) { | |
export default function useAPIState<T extends Y, Y>(api: API<Y>, defaultState: T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, can't this just be:
export default function useAPIState<T>(api: API<T>, defaultState: T) {
8cf1be5
to
efb3d9b
Compare
Issue and Security review incoming... |
components/ai_chat/resources/untrusted_conversation_frame/untrusted_conversation_frame.html
Show resolved
Hide resolved
components/ai_chat/resources/untrusted_conversation_frame/untrusted_conversation_frame.html
Show resolved
Hide resolved
components/ai_chat/resources/untrusted_conversation_frame/untrusted_conversation_frame.html
Show resolved
Hide resolved
components/ai_chat/resources/untrusted_conversation_frame/untrusted_conversation_frame.html
Show resolved
Hide resolved
components/ai_chat/resources/untrusted_conversation_frame/untrusted_conversation_frame.html
Show resolved
Hide resolved
components/ai_chat/resources/untrusted_conversation_frame/untrusted_conversation_frame.html
Show resolved
Hide resolved
// Browser-side handler for a Conversation's UI responsible for displaying | ||
// untrusted content (e.g. content generated by the AI engine). | ||
interface UntrustedConversationHandler { | ||
BindUntrustedConversationUI( | ||
pending_remote<UntrustedConversationUI> untrusted_ui) | ||
=> (ConversationEntriesState conversation_entries_state); | ||
|
||
// Get all visible history entries, including in-progress responses | ||
GetConversationHistory() => (array<ConversationTurn> conversation_history); | ||
|
||
ModifyConversation(uint32 turn_index, string new_text); | ||
}; | ||
|
||
// Untrusted-UI-side handler for a Conversation, responsible for displaying | ||
// content generated by the AI engine. | ||
interface UntrustedConversationUI { | ||
// TODO(petemill): Provide single entry that's been updated so that we don't | ||
// need to fetch (and clone) all conversation entries each time text is added | ||
// to the most recent entry. | ||
OnConversationHistoryUpdate(); | ||
OnEntriesUIStateChanged(ConversationEntriesState state); | ||
OnFaviconImageDataChanged(); | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the API exposed to the untrusted frame from the Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure if it's a good idea to mix them in the same mojom file? Are there other examples of doing this?
interface ParentUIFrame { | ||
// <iframe> cannot be sized to the natural height of its content, so the child | ||
// must let the parent know whenever the body height changes so that the parent | ||
// can manually set the same height on the <iframe> to avoid scrolling. | ||
ChildHeightChanged(uint32 height); | ||
|
||
// This is sent to the UI rather than straight to the browser to allow | ||
// for further rating feedback to be attached. The parent frame deals | ||
// with it instead of the child frame, due to UI space as well as sensitive | ||
// user information being captured. | ||
RateMessage(string turn_uuid, bool is_liked); | ||
}; | ||
|
||
// Browser-side handler for untrusted frame that handles rendering of | ||
// conversation entries. | ||
interface UntrustedUIHandler { | ||
OpenSearchURL(string query); | ||
OpenLearnMoreAboutBraveSearchWithLeo(); | ||
BindParentPage(pending_receiver<ParentUIFrame> parent_frame); | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are the API calls that the Untrusted frame can make to the C++ UI class and the Javascript parent frame
// State required to show the conversations entries UI block | ||
struct ConversationEntriesState { | ||
// Whether an answer generation is in progress | ||
bool is_generating; | ||
// Whether the current model is a built-in Leo model | ||
bool is_leo_model; | ||
// How much of the content has been used by the AI engine, percentage,or null | ||
// if no content is associated. | ||
uint32? content_used_percentage; | ||
// Whether the content has been refined | ||
bool is_content_refined; | ||
// Whether the UI should represent that the user cannot submit new messages | ||
// or edits to the conversation. | ||
bool can_submit_user_entries; | ||
}; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the data, in addition to the conversation history itself, that the untrusted frame receives
source->OverrideContentSecurityPolicy( | ||
network::mojom::CSPDirectiveName::ScriptSrc, | ||
"script-src 'self' chrome-untrusted://resources;"); | ||
source->OverrideContentSecurityPolicy( | ||
network::mojom::CSPDirectiveName::StyleSrc, | ||
"style-src 'self' 'unsafe-inline' chrome-untrusted://resources;"); | ||
source->OverrideContentSecurityPolicy( | ||
network::mojom::CSPDirectiveName::ImgSrc, | ||
"img-src 'self' blob: chrome-untrusted://resources;"); | ||
source->OverrideContentSecurityPolicy( | ||
network::mojom::CSPDirectiveName::FontSrc, | ||
"font-src 'self' data: chrome-untrusted://resources;"); | ||
source->OverrideContentSecurityPolicy( | ||
network::mojom::CSPDirectiveName::FrameAncestors, | ||
base::StringPrintf("frame-ancestors %s;", kAIChatUIURL)); | ||
source->OverrideContentSecurityPolicy( | ||
network::mojom::CSPDirectiveName::TrustedTypes, "trusted-types default;"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CSP for the untrusted frame
source->OverrideContentSecurityPolicy( | ||
network::mojom::CSPDirectiveName::ScriptSrc, | ||
"script-src 'self' chrome-untrusted://resources;"); | ||
untrusted_source->OverrideContentSecurityPolicy( | ||
"script-src 'self' chrome://resources;"); | ||
source->OverrideContentSecurityPolicy( | ||
network::mojom::CSPDirectiveName::StyleSrc, | ||
"style-src 'self' 'unsafe-inline' chrome-untrusted://resources;"); | ||
untrusted_source->OverrideContentSecurityPolicy( | ||
"style-src 'self' 'unsafe-inline' chrome://resources;"); | ||
source->OverrideContentSecurityPolicy( | ||
network::mojom::CSPDirectiveName::ImgSrc, | ||
"img-src 'self' blob: chrome-untrusted://resources;"); | ||
untrusted_source->OverrideContentSecurityPolicy( | ||
"img-src 'self' blob: chrome://resources chrome://favicon2;"); | ||
source->OverrideContentSecurityPolicy( | ||
network::mojom::CSPDirectiveName::FontSrc, | ||
"font-src 'self' data: chrome-untrusted://resources;"); | ||
"font-src 'self' data: chrome://resources;"); | ||
source->OverrideContentSecurityPolicy( | ||
network::mojom::CSPDirectiveName::ChildSrc, | ||
base::StringPrintf("child-src %s;", kAIChatUntrustedConversationUIURL)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CSP changes for the trusted parent frame
inline constexpr char kAIChatUIURL[] = "chrome://leo-ai/"; | ||
inline constexpr char kAIChatUIHost[] = "leo-ai"; | ||
inline constexpr char kAIChatUntrustedConversationUIURL[] = | ||
"chrome-untrusted://leo-ai-conversation-entries/"; | ||
inline constexpr char kAIChatUntrustedConversationUIHost[] = | ||
"leo-ai-conversation-entries"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
new URLs
@@ -5,9 +5,9 @@ | |||
|
|||
module ai_chat.mojom; | |||
|
|||
import "brave/components/ai_chat/core/common/mojom/untrusted_frame.mojom"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file can be split up further. I started doing it but stashed the changes for now because I didn't want to further bloat this PR.
mojo::ReceiverSet<mojom::ConversationHandler> receivers_; | ||
// TODO(petemill): Rename to ConversationUIHandler | ||
mojo::ReceiverSet<mojom::UntrustedConversationHandler> untrusted_receivers_; | ||
mojo::RemoteSet<mojom::ConversationUI> conversation_ui_handlers_; | ||
mojo::RemoteSet<mojom::UntrustedConversationUI> | ||
untrusted_conversation_ui_handlers_; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ConversationHandler has to manage receiver and remote for each parent trusted webui and child untrusted webui
…r and remove and jank
ce3b00c
to
84b6868
Compare
[puLL-Merge] - brave/brave-core@26855 DescriptionThis PR introduces significant changes to the AI Chat feature in the Brave browser. It restructures the UI components, adds new functionality, and improves the overall architecture of the AI Chat system. The changes include splitting the UI into trusted and untrusted parts, implementing a new conversation entries frame, and updating the API and state management for both the main UI and the conversation entries. ChangesChanges
sequenceDiagram
participant User
participant TrustedUI
participant UntrustedUI
participant ConversationHandler
participant AIService
User->>TrustedUI: Interact with AI Chat
TrustedUI->>UntrustedUI: Load conversation entries
UntrustedUI->>ConversationHandler: Request conversation history
ConversationHandler->>AIService: Get conversation data
AIService-->>ConversationHandler: Return conversation data
ConversationHandler-->>UntrustedUI: Send conversation history
UntrustedUI-->>TrustedUI: Update UI with conversation entries
User->>UntrustedUI: Rate a message
UntrustedUI->>TrustedUI: Notify of rating
TrustedUI->>ConversationHandler: Submit rating
ConversationHandler->>AIService: Process rating
AIService-->>ConversationHandler: Confirm rating
ConversationHandler-->>TrustedUI: Update UI with rating result
Possible Issues
Security Hotspots
|
- [AIChat conversation data storage by petemill · Pull Request \#25876 · brave/brave-core](#25876) - [AI Chat conversations should persist browser restart · Issue \#42800 · brave/brave-browser](brave/brave-browser#42800) `QA/Yes` - [\[AIChat\]: Add URL based routing for different chats by fallaciousreasoning · Pull Request \#26050 · brave/brave-core](#26050) - [\[AI Chat\]: URLs for all conversations · Issue \#42055 · brave/brave-browser](brave/brave-browser#42055) `QA/Yes` - test plan: url and header variations specified - [AI Chat fullpage UI notices and polish by petemill · Pull Request \#26678 · brave/brave-core](#26678) - [AI Chat conversation list should prompt to enable history storage if it's disabled · Issue \#42576 · brave/brave-browser](brave/brave-browser#42576) `QA/Yes` - test plan: specified - [AI Chat notice for conversation storage · Issue \#42360 · brave/brave-browser](brave/brave-browser#42360) `QA/Yes` - test plan: specified - [AI Chat FullPage shouldn't show in SidePanel mode when restored at startup · Issue \#42413 · brave/brave-browser](brave/brave-browser#42413) `QA/Yes` - test plan: specified - [AI Chat becomes a trusted WebUI with an UntrustedWebUI frame for LLM-generated responses by petemill · Pull Request \#26855 · brave/brave-core](#26855) - [Change AI Chat url to chrome://leo-ai · Issue \#42817 · brave/brave-browser](brave/brave-browser#42817) `QA/Yes` - [AI Chat conversation entries should be isolated in an untrusted frame · Issue \#42818 · brave/brave-browser](brave/brave-browser#42818) `QA/No` - [\[AI Chat\]: Update copy button label by fallaciousreasoning · Pull Request \#26422 · brave/brave-core](#26422) - [Change copy button name to text if text in code block · Issue \#42117 · brave/brave-browser](brave/brave-browser#42117) `QA/Yes` - [\[AI Chat\]: Conversation starter pack, for unassociated content by fallaciousreasoning · Pull Request \#26379 · brave/brave-core](#26379) - [\[AI Chat\]: Add support for static conversation starters · Issue \#42106 · brave/brave-browser](brave/brave-browser#42106) `QA/Yes` - test plan: needs improvement - [Css tweaks and icon change for sidebar by aguscruiz · Pull Request \#26450 · brave/brave-core](#26450) - [\[Leo full page\] - Tweak to expand/collapse sidebar icons · Issue \#42068 · brave/brave-browser](brave/brave-browser#42068) `QA/Yes` - test plan: specified (check icons are correct) - [\[AI Chat\]: Update styling on suggestions by fallaciousreasoning · Pull Request \#26565 · brave/brave-core](#26565) - [\[AI Chat\]: Update suggestions style to match new design · Issue \#42107 · brave/brave-browser](brave/brave-browser#42107) `QA/Yes` - test plan: specified (check suggestions have new style) - [\[AI Chat\]: Don't show starter suggestions on non-empty chats by fallaciousreasoning · Pull Request \#26677 · brave/brave-core](#26677) - [AI Chat static conversation starters shouldn't show if conversation has chat history · Issue \#42412 · brave/brave-browser](brave/brave-browser#42412) `QA/Yes` - test plan: specified (verify conversation starters only show when applicable) - [fix hit area for leo conversations list by aguscruiz · Pull Request \#26793 · brave/brave-core](#26793) - [Leo full page - Conversation list - Make the whole item clickable instead of excluding the padding · Issue \#42552 · brave/brave-browser](brave/brave-browser#42552) `QA/No` - [\[Nala / @brave/leo\] update dependency by petemill · Pull Request \#26767 · brave/brave-core](#26767) - brave/brave-browser#42545 `QA/No`
- [AIChat conversation data storage by petemill · Pull Request \#25876 · brave/brave-core](#25876) - [AI Chat conversations should persist browser restart · Issue \#42800 · brave/brave-browser](brave/brave-browser#42800) `QA/Yes` - [\[AIChat\]: Add URL based routing for different chats by fallaciousreasoning · Pull Request \#26050 · brave/brave-core](#26050) - [\[AI Chat\]: URLs for all conversations · Issue \#42055 · brave/brave-browser](brave/brave-browser#42055) `QA/Yes` - test plan: url and header variations specified - [AI Chat fullpage UI notices and polish by petemill · Pull Request \#26678 · brave/brave-core](#26678) - [AI Chat conversation list should prompt to enable history storage if it's disabled · Issue \#42576 · brave/brave-browser](brave/brave-browser#42576) `QA/Yes` - test plan: specified - [AI Chat notice for conversation storage · Issue \#42360 · brave/brave-browser](brave/brave-browser#42360) `QA/Yes` - test plan: specified - [AI Chat FullPage shouldn't show in SidePanel mode when restored at startup · Issue \#42413 · brave/brave-browser](brave/brave-browser#42413) `QA/Yes` - test plan: specified - [AI Chat becomes a trusted WebUI with an UntrustedWebUI frame for LLM-generated responses by petemill · Pull Request \#26855 · brave/brave-core](#26855) - [Change AI Chat url to chrome://leo-ai · Issue \#42817 · brave/brave-browser](brave/brave-browser#42817) `QA/Yes` - [AI Chat conversation entries should be isolated in an untrusted frame · Issue \#42818 · brave/brave-browser](brave/brave-browser#42818) `QA/No` - [\[AI Chat\]: Update copy button label by fallaciousreasoning · Pull Request \#26422 · brave/brave-core](#26422) - [Change copy button name to text if text in code block · Issue \#42117 · brave/brave-browser](brave/brave-browser#42117) `QA/Yes` - [\[AI Chat\]: Conversation starter pack, for unassociated content by fallaciousreasoning · Pull Request \#26379 · brave/brave-core](#26379) - [\[AI Chat\]: Add support for static conversation starters · Issue \#42106 · brave/brave-browser](brave/brave-browser#42106) `QA/Yes` - test plan: needs improvement - [Css tweaks and icon change for sidebar by aguscruiz · Pull Request \#26450 · brave/brave-core](#26450) - [\[Leo full page\] - Tweak to expand/collapse sidebar icons · Issue \#42068 · brave/brave-browser](brave/brave-browser#42068) `QA/Yes` - test plan: specified (check icons are correct) - [\[AI Chat\]: Update styling on suggestions by fallaciousreasoning · Pull Request \#26565 · brave/brave-core](#26565) - [\[AI Chat\]: Update suggestions style to match new design · Issue \#42107 · brave/brave-browser](brave/brave-browser#42107) `QA/Yes` - test plan: specified (check suggestions have new style) - [\[AI Chat\]: Don't show starter suggestions on non-empty chats by fallaciousreasoning · Pull Request \#26677 · brave/brave-core](#26677) - [AI Chat static conversation starters shouldn't show if conversation has chat history · Issue \#42412 · brave/brave-browser](brave/brave-browser#42412) `QA/Yes` - test plan: specified (verify conversation starters only show when applicable) - [fix hit area for leo conversations list by aguscruiz · Pull Request \#26793 · brave/brave-core](#26793) - [Leo full page - Conversation list - Make the whole item clickable instead of excluding the padding · Issue \#42552 · brave/brave-browser](brave/brave-browser#42552) `QA/No` - [\[Nala / @brave/leo\] update dependency by petemill · Pull Request \#26767 · brave/brave-core](#26767) - brave/brave-browser#42545 `QA/No`
Released in v1.75.93 |
- [AIChat conversation data storage by petemill · Pull Request \#25876 · brave/brave-core](#25876) - [AI Chat conversations should persist browser restart · Issue \#42800 · brave/brave-browser](brave/brave-browser#42800) `QA/Yes` - [\[AIChat\]: Add URL based routing for different chats by fallaciousreasoning · Pull Request \#26050 · brave/brave-core](#26050) - [\[AI Chat\]: URLs for all conversations · Issue \#42055 · brave/brave-browser](brave/brave-browser#42055) `QA/Yes` - test plan: url and header variations specified - [AI Chat fullpage UI notices and polish by petemill · Pull Request \#26678 · brave/brave-core](#26678) - [AI Chat conversation list should prompt to enable history storage if it's disabled · Issue \#42576 · brave/brave-browser](brave/brave-browser#42576) `QA/Yes` - test plan: specified - [AI Chat notice for conversation storage · Issue \#42360 · brave/brave-browser](brave/brave-browser#42360) `QA/Yes` - test plan: specified - [AI Chat FullPage shouldn't show in SidePanel mode when restored at startup · Issue \#42413 · brave/brave-browser](brave/brave-browser#42413) `QA/Yes` - test plan: specified - [AI Chat becomes a trusted WebUI with an UntrustedWebUI frame for LLM-generated responses by petemill · Pull Request \#26855 · brave/brave-core](#26855) - [Change AI Chat url to chrome://leo-ai · Issue \#42817 · brave/brave-browser](brave/brave-browser#42817) `QA/Yes` - [AI Chat conversation entries should be isolated in an untrusted frame · Issue \#42818 · brave/brave-browser](brave/brave-browser#42818) `QA/No` - [\[AI Chat\]: Update copy button label by fallaciousreasoning · Pull Request \#26422 · brave/brave-core](#26422) - [Change copy button name to text if text in code block · Issue \#42117 · brave/brave-browser](brave/brave-browser#42117) `QA/Yes` - [\[AI Chat\]: Conversation starter pack, for unassociated content by fallaciousreasoning · Pull Request \#26379 · brave/brave-core](#26379) - [\[AI Chat\]: Add support for static conversation starters · Issue \#42106 · brave/brave-browser](brave/brave-browser#42106) `QA/Yes` - test plan: needs improvement - [Css tweaks and icon change for sidebar by aguscruiz · Pull Request \#26450 · brave/brave-core](#26450) - [\[Leo full page\] - Tweak to expand/collapse sidebar icons · Issue \#42068 · brave/brave-browser](brave/brave-browser#42068) `QA/Yes` - test plan: specified (check icons are correct) - [\[AI Chat\]: Update styling on suggestions by fallaciousreasoning · Pull Request \#26565 · brave/brave-core](#26565) - [\[AI Chat\]: Update suggestions style to match new design · Issue \#42107 · brave/brave-browser](brave/brave-browser#42107) `QA/Yes` - test plan: specified (check suggestions have new style) - [\[AI Chat\]: Don't show starter suggestions on non-empty chats by fallaciousreasoning · Pull Request \#26677 · brave/brave-core](#26677) - [AI Chat static conversation starters shouldn't show if conversation has chat history · Issue \#42412 · brave/brave-browser](brave/brave-browser#42412) `QA/Yes` - test plan: specified (verify conversation starters only show when applicable) - [fix hit area for leo conversations list by aguscruiz · Pull Request \#26793 · brave/brave-core](#26793) - [Leo full page - Conversation list - Make the whole item clickable instead of excluding the padding · Issue \#42552 · brave/brave-browser](brave/brave-browser#42552) `QA/No` - [\[Nala / @brave/leo\] update dependency by petemill · Pull Request \#26767 · brave/brave-core](#26767) - brave/brave-browser#42545 `QA/No`
Issues
Description
This was requested by the security team in order to make sure the API footprint exposed to a renderer which is renderering LLM-generated content is as small as possible.
The two frames use the same JS build to optimize bundle size, but do not share the same allowed mojom interfaces. The untrusted frame is limited to
UntrustedConversationHandler
to send calls to the browser andUntrustedUI
to receive calls from the browser.The frames need to communicate with each other in order to solve the main hassle of using an iframe: layout. An iframe is always a fixed width and height, decided by its host and will not expand to the child body size.
Since the iframed content is just part of the scroll area of a Conversation, we must tell the parent whenever the body size changes. Further complicating the situation is continuously knowing when the latest part of a conversation response is rendered, so that we can scroll the container to show it.
The inter-frame communication is done via mojom directly, initially facilitated by the C++ WebUI handlers.
TODO:
Resolves
Submitter Checklist:
QA/Yes
orQA/No
;release-notes/include
orrelease-notes/exclude
;OS/...
) to the associated issuenpm run test -- brave_browser_tests
,npm run test -- brave_unit_tests
wikinpm run presubmit
wiki,npm run gn_check
,npm run tslint
git rebase master
(if needed)Reviewer Checklist:
gn
After-merge Checklist:
changes has landed on
Test Plan: