Skip to content

Conversation

@randy-concepcion
Copy link

@randy-concepcion randy-concepcion commented Nov 24, 2025

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 switch
  • PolicyEvaluator.sys.mjs / ConditionEvaluator.sys.mjs — JSON-based policy evaluation
  • SecurityUtils.sys.mjs — URL normalization and session-scoped ledgers
  • DecisionTypes.sys.mjs — Structured allow/deny decisions
  • SecurityLogger.sys.mjs — Audit logging (stub)

Smart Window integration (browser/components/smartwindow/):

  • SmartWindowMetaChild/Parent actors — Page metadata extraction (canonical/og:url)
  • utils.mjs — Tool dispatch integration via checkToolSecurity()

Policy (policies/tool-execution-policies.json):

  • block-unseen-links — Blocks tool calls to URLs not in trusted page context

Security Model

  • Explicit seeding: AI only accesses URLs when user explicitly requests them
  • Deterministic validation: Exact URL matching against session ledger
  • Fail-closed: Security errors deny tool execution
  • Kill switch: browser.smartwindow.security.enabled preference

Testing

  • Unit tests for policy evaluation, URL normalization, ledger management
  • Integration tests for allowed/denied scenarios, kill switch, fail-closed behavior

Screenshots

Security Layer + policy loaded up with Smart Window
image

Accessing untrusted link and enforced with deny action
image

Accessing trusted link and validated with allow action
image

@randy-concepcion randy-concepcion changed the title [WIP] [GENAI-2335] Implement Policy Engine for Smart Window tool execution [GENAI-2335] Implement Policy Engine for Smart Window tool execution Nov 24, 2025
@randy-concepcion randy-concepcion marked this pull request as ready for review November 24, 2025 16:48
@noahpodgurski
Copy link

Also https://mozilla.slack.com/archives/C09AR1FJCJ1/p1763724678792239 🫠

@randy-concepcion
Copy link
Author

Also https://mozilla.slack.com/archives/C09AR1FJCJ1/p1763724678792239 🫠

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!).

@randy-concepcion randy-concepcion force-pushed the GENAI-2335-policy-engine branch 2 times, most recently from 2062562 to ce28b69 Compare November 25, 2025 23:15
…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
@randy-concepcion randy-concepcion force-pushed the GENAI-2335-policy-engine branch 2 times, most recently from 597e9af to 55c9c0d Compare November 30, 2025 04:50
@randy-concepcion
Copy link
Author

I split out the security layer-related code to land in mozilla-central . This is separate from the AI Window integration. The Phab change can be found here: https://phabricator.services.mozilla.com/D274523.

- 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)
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}`);
Copy link

@noahpodgurski noahpodgurski Dec 2, 2025

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?

Copy link
Author

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() {

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.#loadedPolicies

But if loadPolicies isn't very expensive to run it might not be worth it

Copy link
Author

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!

Copy link

@tarekziade tarekziade left a 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() {

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

Copy link
Author

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");

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?

Copy link
Author

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);

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

Copy link
Author

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) {

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

Copy link
Author

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 ||

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

Copy link
Author

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 {

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

Copy link
Author

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 {

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

Copy link
Author

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(

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?

Copy link
Author

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 🐛

Copy link
Author

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) {

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

Copy link
Author

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 {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why using a class?

Copy link
Author

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
@randy-concepcion
Copy link
Author

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

Thanks again for your feedback, @tarekziade! 🙏 Here's a summary of the changes I've made:

Phabricator Revisions (ready for review)
These are ready for review to land in mozilla-central . Afterwards, they can get pulled into this PR with the AI Window integrations in browser/components/smartwindow.

Changes For This PR

  • Consolidated URL processing into #processMetadataUrls() in SmartWindowMetaParent
  • Added console.warn when security policy blocks tool execution
  • Added comment explaining targetTab URL matching logic
  • Added default case to tool switch for defense-in-depth

Metadata API: I'm still looking into using an existing API instead of the SmartWindowMeta actor. I'll follow up once I've investigated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants