diff --git a/app/src/lib/notificationRouter.test.ts b/app/src/lib/notificationRouter.test.ts index 02a57b757..0ed2448b9 100644 --- a/app/src/lib/notificationRouter.test.ts +++ b/app/src/lib/notificationRouter.test.ts @@ -66,6 +66,22 @@ describe('resolveIntegrationRoute', () => { const n = makeIntegration({ provider: 'slack', deep_link: '/skills' }); expect(resolveIntegrationRoute(n)).toBe('/skills'); }); + + it.each([ + 'javascript:alert(1)', + '//evil.example.com', + 'https://evil.example.com/x', + 'http://evil.example.com', + 'data:text/html,x', + 'mailto:x@y.z', + 'relative/no/leading/slash', + '\\evil', + ])('ignores unsafe deep_link %s and uses the provider default', deep_link => { + // `deep_link` derives from untrusted inbound provider content, so a value + // that is not a relative in-app path must be dropped, not navigated to. + const n = makeIntegration({ provider: 'slack', deep_link }); + expect(resolveIntegrationRoute(n)).toBe('/chat'); + }); }); // ───────────────────────────────────────────────────────────────────────────── @@ -110,4 +126,16 @@ describe('resolveSystemRoute', () => { const item = makeSystem({ category: 'messages', deepLink: '/notifications' }); expect(resolveSystemRoute(item)).toBe('/notifications'); }); + + it.each([ + 'javascript:alert(1)', + '//evil.example.com', + 'https://evil.example.com/x', + 'data:text/html,x', + 'relative/no/leading/slash', + '\\evil', + ])('ignores unsafe deepLink %s and uses the category default', deepLink => { + const item = makeSystem({ category: 'messages', deepLink }); + expect(resolveSystemRoute(item)).toBe('/chat'); + }); }); diff --git a/app/src/lib/notificationRouter.ts b/app/src/lib/notificationRouter.ts index dd4305a3d..4c584121f 100644 --- a/app/src/lib/notificationRouter.ts +++ b/app/src/lib/notificationRouter.ts @@ -33,6 +33,23 @@ const MESSAGE_PROVIDERS = new Set([ 'twitter', ]); +/** + * A `deep_link` / `deepLink` is set by the core triage pipeline, which derives + * it from untrusted inbound provider content. Callers feed the result straight + * to react-router `navigate()`, so accept it only when it is a relative in-app + * path: a single leading `/`, no protocol-relative `//`, no scheme, no + * backslash. This blocks `javascript:` / `http(s)://evil` / `//host` from + * reaching the router (HashRouter already contains most of these, but the + * allowlist keeps navigation strictly in-app). Unsafe values fall through to + * the provider/category default. + */ +function isSafeInAppPath(path: string): boolean { + // A single leading slash, no protocol-relative `//`, no backslash. Keeps the + // value a relative in-app route, so `javascript:` / `http(s)://evil` / `//host` + // can never reach react-router `navigate()`. + return path.startsWith('/') && path[1] !== '/' && path[1] !== '\\' && !path.includes('\\'); +} + // ───────────────────────────────────────────────────────────────────────────── // Route resolvers // ───────────────────────────────────────────────────────────────────────────── @@ -46,10 +63,13 @@ const MESSAGE_PROVIDERS = new Set([ * 3. `/notifications` fallback. */ export function resolveIntegrationRoute(n: IntegrationNotification): string { - if (n.deep_link) { + if (n.deep_link && isSafeInAppPath(n.deep_link)) { log('[notification-router] integration id=%s explicit deep_link=%s', n.id, n.deep_link); return n.deep_link; } + if (n.deep_link) { + log('[notification-router] integration id=%s ignored unsafe deep_link=%s', n.id, n.deep_link); + } if (MESSAGE_PROVIDERS.has(n.provider)) { log('[notification-router] integration id=%s provider=%s → /chat', n.id, n.provider); @@ -73,10 +93,13 @@ export function resolveIntegrationRoute(n: IntegrationNotification): string { * 3. `/notifications` fallback. */ export function resolveSystemRoute(item: NotificationItem): string { - if (item.deepLink) { + if (item.deepLink && isSafeInAppPath(item.deepLink)) { log('[notification-router] system id=%s explicit deepLink=%s', item.id, item.deepLink); return item.deepLink; } + if (item.deepLink) { + log('[notification-router] system id=%s ignored unsafe deepLink=%s', item.id, item.deepLink); + } switch (item.category) { case 'messages': diff --git a/app/src/utils/__tests__/toolTimelineFormatting.test.ts b/app/src/utils/__tests__/toolTimelineFormatting.test.ts index 28a5b7376..a08f28bec 100644 --- a/app/src/utils/__tests__/toolTimelineFormatting.test.ts +++ b/app/src/utils/__tests__/toolTimelineFormatting.test.ts @@ -5,6 +5,7 @@ import type { PersistedTranscriptItem } from '../../types/turnState'; import { buildProcessingBlocks, categorizeTool, + extractAgentSources, formatTimelineEntry, formatToolName, isKnownClientTool, @@ -373,3 +374,38 @@ describe('formatter null-safety (malformed / legacy snapshot guard)', () => { expect(stripToolCallEnvelopes(null)).toBe(''); }); }); + +describe('extractAgentSources', () => { + const source = (id: string, url: string): ToolTimelineEntry => + entry({ id, name: 'web_fetch', argsBuffer: JSON.stringify({ url }) }); + + it('surfaces http(s) sources with hostname titles, deduped in first-seen order', () => { + const out = extractAgentSources([ + source('a', 'https://example.com/docs'), + source('b', 'http://blog.example.org/post'), + source('c', 'https://example.com/docs'), // dup URL — dropped + ]); + expect(out).toEqual([ + { id: 'a', title: 'example.com', url: 'https://example.com/docs' }, + { id: 'b', title: 'blog.example.org', url: 'http://blog.example.org/post' }, + ]); + }); + + it('drops non-http(s) URLs so a hostile scheme never reaches an anchor href', () => { + const out = extractAgentSources([ + source('js', 'javascript:alert(1)'), + source('data', 'data:text/html,'), + source('file', 'file:///etc/passwd'), + source('ok', 'https://safe.example.com'), + ]); + expect(out).toEqual([{ id: 'ok', title: 'safe.example.com', url: 'https://safe.example.com' }]); + }); + + it('ignores entries from non-URL tools', () => { + expect( + extractAgentSources([ + entry({ id: 'g', name: 'grep', argsBuffer: JSON.stringify({ url: 'https://x.com' }) }), + ]) + ).toEqual([]); + }); +}); diff --git a/app/src/utils/toolTimelineFormatting.ts b/app/src/utils/toolTimelineFormatting.ts index b12ccb363..527308651 100644 --- a/app/src/utils/toolTimelineFormatting.ts +++ b/app/src/utils/toolTimelineFormatting.ts @@ -420,7 +420,11 @@ export function extractAgentSources(entries: ToolTimelineEntry[]): AgentSource[] const baseName = entry.name.replace(/^subagent:/, ''); if (!URL_SOURCE_TOOLS.has(baseName)) continue; const url = parseToolArgs(entry.argsBuffer)?.url?.trim(); - if (!url || seen.has(url)) continue; + // `url` is the raw tool-call argument the model emitted — it is + // prompt-injection-influenceable and not guaranteed to be a real web + // address. Only surface http(s) sources as clickable links so a + // `javascript:` / `data:` / `file:` value can never reach an ``. + if (!url || seen.has(url) || !isHttpUrl(url)) continue; seen.add(url); sources.push({ id: entry.id, title: hostnameFromUrl(url) ?? url, url }); } @@ -443,6 +447,16 @@ function hostnameFromUrl(url: string): string | undefined { } } +/** True only for well-formed http(s) URLs — the schemes safe to link out to. */ +function isHttpUrl(url: string): boolean { + try { + const { protocol } = new URL(url); + return protocol === 'http:' || protocol === 'https:'; + } catch { + return false; + } +} + function shortenPath(filePath: string): string { const parts = filePath.split('/'); if (parts.length <= 3) return filePath; diff --git a/src/openhuman/security/policy/policy_command.rs b/src/openhuman/security/policy/policy_command.rs index 996bf367f..6e06304e5 100644 --- a/src/openhuman/security/policy/policy_command.rs +++ b/src/openhuman/security/policy/policy_command.rs @@ -779,12 +779,29 @@ pub(super) fn classify_segment(base: &str, args: &[String], joined: &str) -> Com if is_command_executor(base) { return CommandClass::Write; } - // `find` is read-only unless it executes commands or deletes files. + // `find` is read-only unless it executes commands, deletes, or writes + // files. -fprintf / -fprint / -fprint0 / -fls write their output to a + // named file rather than stdout — an arbitrary-path write that side-steps + // the gated file-write tools (and their workspace confinement), so they + // must be classified Write (approval-gated) alongside -delete. + // + // `args` is built with `split_whitespace()`, which keeps shell quotes that + // the shell itself strips before `find` runs — `find . '-fprint' out` + // arrives as the literal `'-fprint'`. Trim surrounding single/double quotes + // before matching so a quoted predicate cannot slip the write past the gate. if base == "find" { if args.iter().any(|a| { matches!( - a.as_str(), - "-exec" | "-execdir" | "-ok" | "-okdir" | "-delete" + a.trim_matches(|c| c == '\'' || c == '"'), + "-exec" + | "-execdir" + | "-ok" + | "-okdir" + | "-delete" + | "-fprintf" + | "-fprint" + | "-fprint0" + | "-fls" ) }) { return CommandClass::Write; diff --git a/src/openhuman/security/policy/policy_tests.rs b/src/openhuman/security/policy/policy_tests.rs index 2dd8a2037..132ac71ea 100644 --- a/src/openhuman/security/policy/policy_tests.rs +++ b/src/openhuman/security/policy/policy_tests.rs @@ -398,6 +398,37 @@ fn classify_writes_are_write() { } } +#[test] +fn classify_find_file_write_actions_are_write() { + let p = default_policy(); + // -fprintf / -fprint / -fprint0 / -fls write find's output to a named + // file (an arbitrary-path write side-channel), so they must be gated as + // Write rather than slipping through as a read-only search. + for c in [ + "find . -maxdepth 0 -fprintf /tmp/out.txt '%p\\n'", + "find . -name '*.rs' -fprint /tmp/list.txt", + "find . -fprint0 /tmp/list0.txt", + "find . -fls /tmp/ls.txt", + "find . -delete", + ] { + assert_eq!(p.classify_command(c), CommandClass::Write, "{c}"); + } + // Quoting the predicate must not slip the write past the gate — the shell + // strips the quotes before find runs, so `'-fprint'` is the same write. + for c in [ + "find . '-fprint' /tmp/list.txt", + "find . \"-fprintf\" /tmp/out.txt '%p\\n'", + "find . '-delete'", + ] { + assert_eq!(p.classify_command(c), CommandClass::Write, "{c}"); + } + // A plain search stays read-only. + assert_eq!( + p.classify_command("find . -name '*.rs' -print"), + CommandClass::Read + ); +} + #[test] fn classify_network_is_network() { let p = default_policy();