-
Notifications
You must be signed in to change notification settings - Fork 0
[GENAI-2335] Implement Policy Engine for Smart Window tool execution #124
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
base: smart-window
Are you sure you want to change the base?
Conversation
9574528 to
6e9dc59
Compare
Yes, thanks for the reminder about the name change 😅 I'll make updates to the files/names in a separate commit 😬 (as well as the refactor in a separate commit for the bug you found above!). |
2062562 to
ce28b69
Compare
…nent for LLM calls and tool dispatch. r=tarek,ai-ondevice-reviewers - Moved JSActor pattern from security-specific JSActor back to parent MLEngine JSActor - Move validation logic for both request/response from child to parent to avoid IPC round trip (improve performance) Differential Revision: https://phabricator.services.mozilla.com/D271495
Add security layer policy enforcement for Smart Window tool execution with policy to prevent exfiltration links Components: - SecurityOrchestrator: Central coordination with pref switch - PolicyEvaluator/ConditionEvaluator: JSON-based policy evaluation - SecurityUtils: URL normalization and session-scoped ledgers - SmartWindowMeta actors: Page metadata extraction (canonical/og:url) - DecisionTypes: Structured allow/deny decisions Security model: Explicit seeding with deterministic URL validation, fail-closed behavior, and pref switch (browser.smartwindow.security.enabled).
Addressed comments in PR: - Add EFFECT_ALLOW/EFFECT_DENY constants to DecisionTypes.sys.mjs - Add PolicyEffect typedef for type hints - Replace hardcoded effect strings across security modules - Add generateId(prefix) utility to SecurityUtils.sys.mjs - Update SmartWindowIntegration and smartwindow.mjs to use generateId
597e9af to
55c9c0d
Compare
|
I split out the security layer-related code to land in |
- Fixes multi-window bug where closing one AI Window would reset security state for all windows. - Fixes minor bug for allowedUrls and other lint errors - Update SecurityOrchestrator instantiation in xpcshell tests - Remove deprecated files and tests (SmartWindowIntegration.sys.mjs)
55c9c0d to
3f76c54
Compare
| if (added) { | ||
| console.log(`[Security] Seeded @mentioned URL: ${url} for tab ${tabId}`); | ||
| } else { | ||
| console.warn(`[Security] Failed to seed @mentioned URL: ${url} for tab ${tabId}`); |
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.
question: Would it make sense for this to be considered an error and brought to the user's attention that the permission access request failed?
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.
Great question! I've given this some thought. Since these log messages are intended for developers rather than users, I don't think we need to surface this to the user directly. They'll see a denial when the tool call is evaluated by the security layer (fail-closed behavior).
However, I've tightened up the logging to give better signal: "no ledger available" is now console.error, while "already in ledger" is console.debug. This should give developers better visibility into the unexpected failures.
| */ | ||
| static async #loadPolicies() { | ||
| // Add more policy files here as they're created: | ||
| async #loadPolicies() { |
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.
thought: This is probably not necessary but a nice optimization might be to keep this method static, add a static loadedPolicies = null attribute, and only call this in the create method if loadedPolicies is null? Something like
if (SecurityOrchestrator.#loadedPolicies === null) {
SecurityOrchestrator.#loadedPolicies = SecurityOrchestrator.#loadPolicies();
}
this.#policies = SecurityOrchestrator.#loadedPoliciesBut if loadPolicies isn't very expensive to run it might not be worth it
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.
Good call on the optimization 🙏 Since I'll need to update this and all the test files, I'll defer this optimization in this POC for now. The next phase, I'll be refactoring the orchestrator to a singleton pattern where caching should happen naturally at the service level!
tarekziade
left a comment
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.
pretty nice work thanks;
I think it's worth documenting the flow in the top level class
also, not sure about using classes as pure namespaces, and I also wonder about introducing a new child process just for extracting the metadata. I'd like to get clarity on what you have in mind in terms of IPC for the longer term
| * | ||
| * @returns {object} Metadata with pageUrl, canonical, and ogUrl (raw strings) | ||
| */ | ||
| getMetadata() { |
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.
i feel like we probably already have an existing API for this
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.
Yes, good point. I'll research more if there's an existing API I can use for this. 🕵️
| }; | ||
|
|
||
| try { | ||
| const metadata = await this.sendQuery("SmartWindowMeta:GetMetadata"); |
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.
what is the incentive to run this in the child process?
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.
Good question! My understanding is that the parent process wouldn't be able to query the DOM directly (i.e., document.querySelector() ) since the DOM lives in the child process.
However, I should definitely look into the existing API you mentioned before that would already handle this 😅
|
|
||
| const { pageUrl, canonical, ogUrl } = metadata; | ||
|
|
||
| const normalizedPageUrl = normalizeUrl(pageUrl); |
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.
more page info data extraction here, I feel like this belong to the previous function
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.
Good catch! I can consolidate this!
| } else { | ||
| const result = await actor.seedLedgerForTab(sessionLedger, tabId); | ||
|
|
||
| if (result.success) { |
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.
we're not doing anything with result.error, so you will silent problems
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.
Ah, nice catch. I'll add the appropriate logging here. 💯
| let targetTab = gBrowser.tabs.find(tab => { | ||
| const tabUrl = tab.linkedBrowser.currentURI.spec; | ||
| return ( | ||
| tabUrl === toolParams.url || |
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.
add a comment here on what you are doing
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.
Good call, I'll add a comment to explain the logic. 💯
| * Safe condition evaluator for JSON-based security policies. | ||
| * Evaluates policy conditions against action and context. | ||
| */ | ||
| export class ConditionEvaluator { |
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.
what's the point of having a class here if all methods are static?
if it's for the namespace you can do this directly when exporting
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.
Ah, yes. I had started writing these as classes and just kept the pattern, but you're right with exports making more sense. Thanks for the recommendation, I'll refactor this!
| * Evaluates JSON-based security policies using "first deny wins" strategy. | ||
| * Delegates condition evaluation to ConditionEvaluator. | ||
| */ | ||
| export class PolicyEvaluator { |
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.
same question for using a class just for the namespace
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.
+1 I'll refactor this!
| * @returns {boolean} True if policy applies to this action | ||
| */ | ||
| static checkMatch(matchCriteria, action) { | ||
| console.warn( |
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.
why warn here? shouln't it be debug?
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.
Yes, that makes sense. I will change this to debug 🐛
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.
Ah, right, I remembered why I changed this to warn: the linter doesn't like this :( When I run ./mach lint, I get the following lint error:
34:5 error Unexpected console statement. Only these console methods are allowed: createInstance, error, warn. no-console (eslint)
For now, I can add the comment to disable the lint error since this is a log intentionally for debugging purposes.
| } | ||
| } | ||
|
|
||
| if (policy.effect === EFFECT_DENY) { |
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.
can't this be placed before the for loop and short cur ? I am not sure to follow the logic
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.
Really great catch here. The flow here is a bit hard to follow after looking at this again 🤔 Your suggestion makes sense, I can refactor this!
| * Security audit logger (stub - console only). | ||
| * TODO: Full implementation in separate ticket (NDJSON, Glean, field hashing). | ||
| */ | ||
| export class SecurityLogger { |
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.
why using a class?
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.
+1 I'll changes this as mentioned in previous comments 👍
- Security logs errors on no ledger available + debug if already in ledger - Reorganized imports and security functions
- Consolidate URL processing into #processMetadataUrls() in SmartWindowMetaParent - Add console.warn when security policy blocks tool execution - Add comment explaining targetTab URL matching logic - Add default case to tool switch
Thanks again for your feedback, @tarekziade! 🙏 Here's a summary of the changes I've made: Phabricator Revisions (ready for review)
Changes For This PR
Metadata API: I'm still looking into using an existing API instead of the SmartWindowMeta actor. I'll follow up once I've investigated. |
Summary
Implements the Phase 2 POC security layer with policy enforcement for Smart Window tool execution. Enforces a single policy (
block-unseen-links) that prevents tools from accessing URLs not in the trusted page context, protecting against data exfiltration via prompt injection.Changes
New modules (
toolkit/components/ml/security/):SecurityOrchestrator.sys.mjs— Central coordination with kill switchPolicyEvaluator.sys.mjs/ConditionEvaluator.sys.mjs— JSON-based policy evaluationSecurityUtils.sys.mjs— URL normalization and session-scoped ledgersDecisionTypes.sys.mjs— Structured allow/deny decisionsSecurityLogger.sys.mjs— Audit logging (stub)Smart Window integration (
browser/components/smartwindow/):SmartWindowMetaChild/Parentactors — Page metadata extraction (canonical/og:url)utils.mjs— Tool dispatch integration viacheckToolSecurity()Policy (
policies/tool-execution-policies.json):block-unseen-links— Blocks tool calls to URLs not in trusted page contextSecurity Model
browser.smartwindow.security.enabledpreferenceTesting
Screenshots
Security Layer + policy loaded up with Smart Window

Accessing untrusted link and enforced with deny action

Accessing trusted link and validated with allow action
