fix(mcp-apps): always enable unsafe-eval and wasm-unsafe-eval in CSP#2533
fix(mcp-apps): always enable unsafe-eval and wasm-unsafe-eval in CSP#2533
Conversation
🧪 BenchmarkShould we run the Virtual MCP strategy benchmark for this PR? React with 👍 to run the benchmark.
Benchmark will run on the next push after you react. |
Release OptionsShould a new version be published when this PR is merged? React with an emoji to vote on the release type:
Current version: Deployment
|
There was a problem hiding this comment.
6 issues found across 24 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/mesh/src/web/components/details/connection/resources-tab.tsx">
<violation number="1" location="apps/mesh/src/web/components/details/connection/resources-tab.tsx:93">
P2: Filter state can persist to "ui-apps" while the filter badges are hidden (`hasUIApps` false), leaving users stuck with an empty list and no way to reset the filter after switching connections. Reset the filter to "all" when `hasUIApps` becomes false or when no UI app resources are present.</violation>
</file>
<file name="apps/mesh/src/web/components/chat/message/parts/tool-call-part/generic.tsx">
<violation number="1" location="apps/mesh/src/web/components/chat/message/parts/tool-call-part/generic.tsx:98">
P2: Avoid stringifying null/undefined connectionIds. Guard for an actual value before converting so MCPAppLoader isn’t called with "null"/"undefined".</violation>
</file>
<file name="packages/mesh-sdk/src/lib/mcp-oauth.ts">
<violation number="1" location="packages/mesh-sdk/src/lib/mcp-oauth.ts:664">
P2: Same-origin detection uses string equality on the full base URL, so same-origin URLs that include a trailing slash or path will be treated as cross-origin and omit cookies, breaking OAuth status checks for same-origin apps that pass a full meshUrl.</violation>
</file>
<file name="apps/mesh/src/mcp-apps/csp-injector.ts">
<violation number="1" location="apps/mesh/src/mcp-apps/csp-injector.ts:10">
P2: DEFAULT_CSP is missing `frame-src` and `base-uri` directives that the custom `buildCSPPolicy` explicitly sets to `'none'`. The omission of `base-uri` means the default policy silently allows `<base>` tag injection, which is inconsistent with the stricter custom policy.</violation>
</file>
<file name="apps/mesh/src/mcp-apps/mcp-app-renderer.tsx">
<violation number="1" location="apps/mesh/src/mcp-apps/mcp-app-renderer.tsx:118">
P1: Ref callback captures frequently-changing props (`toolResult`, `toolInput`), causing the AppBridge and iframe to be torn down and rebuilt whenever these props update.
Consider decoupling the bridge lifecycle from data updates: store the iframe element in a stable `useRef`, manage the bridge setup/teardown in a `useEffect` with only structural dependencies (`displayMode`, `maxHeight`), and send `toolInput`/`toolResult` in a separate `useEffect` via the existing bridge when those values change.</violation>
</file>
<file name="apps/mesh/src/mcp-apps/use-ui-resource-loader.ts">
<violation number="1" location="apps/mesh/src/mcp-apps/use-ui-resource-loader.ts:62">
P1: Async side effect runs during render instead of `useEffect`, causing the hook to never re-fetch when `uri` changes (stale data bug). `loadStartedRef` is set to `true` on first load and never reset, so subsequent renders with a different `uri` silently return stale HTML. Moving this to a `useEffect` with `[uri]` in the dependency array would fix both the staleness issue and provide a cleanup path on unmount.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| // Iframe ref callback — sets up bridge on mount, tears down on unmount | ||
| // ----------------------------------------------------------------------- | ||
|
|
||
| const handleIframeRef = (iframe: HTMLIFrameElement | null) => { |
There was a problem hiding this comment.
P1: Ref callback captures frequently-changing props (toolResult, toolInput), causing the AppBridge and iframe to be torn down and rebuilt whenever these props update.
Consider decoupling the bridge lifecycle from data updates: store the iframe element in a stable useRef, manage the bridge setup/teardown in a useEffect with only structural dependencies (displayMode, maxHeight), and send toolInput/toolResult in a separate useEffect via the existing bridge when those values change.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/mesh/src/mcp-apps/mcp-app-renderer.tsx, line 118:
<comment>Ref callback captures frequently-changing props (`toolResult`, `toolInput`), causing the AppBridge and iframe to be torn down and rebuilt whenever these props update.
Consider decoupling the bridge lifecycle from data updates: store the iframe element in a stable `useRef`, manage the bridge setup/teardown in a `useEffect` with only structural dependencies (`displayMode`, `maxHeight`), and send `toolInput`/`toolResult` in a separate `useEffect` via the existing bridge when those values change.</comment>
<file context>
@@ -0,0 +1,260 @@
+ // Iframe ref callback — sets up bridge on mount, tears down on unmount
+ // -----------------------------------------------------------------------
+
+ const handleIframeRef = (iframe: HTMLIFrameElement | null) => {
+ if (bridgeRef.current) {
+ disposedRef.current = true;
</file context>
There was a problem hiding this comment.
This codebase uses React Compiler which handles memoization automatically (useMemo/useCallback/memo are banned). The compiler will memoize handleIframeRef based on its dependencies. In practice, toolInput/toolResult are set once per tool call and don't change afterward, so bridge recreation won't happen. Skipping this one.
There was a problem hiding this comment.
Thanks for the feedback! I've saved this as a new learning to improve future reviews.
apps/mesh/src/web/components/details/connection/resources-tab.tsx
Outdated
Show resolved
Hide resolved
apps/mesh/src/web/components/chat/message/parts/tool-call-part/generic.tsx
Show resolved
Hide resolved
77b6b68 to
8cdea99
Compare
There was a problem hiding this comment.
2 issues found across 9 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/mesh/src/web/components/details/connection/resources-tab.tsx">
<violation number="1" location="apps/mesh/src/web/components/details/connection/resources-tab.tsx:70">
P2: Calling setFilter during render can cause render-loop warnings and unnecessary rerenders. Move this reset into a useEffect that runs when hasUIApps or filter changes.</violation>
</file>
<file name="apps/mesh/src/mcp-apps/use-ui-resource-loader.ts">
<violation number="1" location="apps/mesh/src/mcp-apps/use-ui-resource-loader.ts:37">
P2: Changing `uri` only resets `loadStartedRef`, but leaves `html/error/loading` untouched. After a previous load, `html` stays non-null so the loader never re-runs for the new URI, returning stale content.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| const [search, setSearch] = useState(""); | ||
| const [filter, setFilter] = useState<ResourceFilter>("all"); | ||
| // Reset filter when UI apps disappear (e.g., switching connections) | ||
| if (!hasUIApps && filter !== "all") { |
There was a problem hiding this comment.
P2: Calling setFilter during render can cause render-loop warnings and unnecessary rerenders. Move this reset into a useEffect that runs when hasUIApps or filter changes.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/mesh/src/web/components/details/connection/resources-tab.tsx, line 70:
<comment>Calling setFilter during render can cause render-loop warnings and unnecessary rerenders. Move this reset into a useEffect that runs when hasUIApps or filter changes.</comment>
<file context>
@@ -66,6 +66,10 @@ function ResourcesList({
const [search, setSearch] = useState("");
const [filter, setFilter] = useState<ResourceFilter>("all");
+ // Reset filter when UI apps disappear (e.g., switching connections)
+ if (!hasUIApps && filter !== "all") {
+ setFilter("all");
+ }
</file context>
There was a problem hiding this comment.
This is the idiomatic React 19 pattern for derived state resets. useEffect is banned in this codebase (ban-use-effect.ts oxlint plugin). Calling setState during render with a guard condition is how React 19 / React Compiler handles this — React bails out and re-renders synchronously without committing the intermediate state. No render loop occurs because the condition (\!hasUIApps && filter \!== "all") becomes false after the first setFilter("all").
There was a problem hiding this comment.
Got it—thanks for the context. I’ll apply this React 19 pattern and avoid suggesting useEffect in this codebase.
|
@cubic-dev-ai All issues from both reviews have been addressed — please re-review. |
@tlgimenes I have started the AI code review. It will take a few minutes to complete. |
Introduces the core mcp-apps module: - types.ts: shared type re-exports and helpers (getUIResourceUri, isUIResourceUri, MCP_APP_DISPLAY_MODES) - use-app-bridge.ts: BridgeStore class + useAppBridge hook wiring AppBridge lifecycle to React via useSyncExternalStore - mcp-app-renderer.tsx: MCPAppRenderer component that fetches the HTML resource and renders it in a sandboxed iframe via AppBridge
…n views - Add _meta field to Tool interface to carry tool metadata - Show LayersTwo01 badge in ToolAnnotationBadges when tool has a UI resource - Pass _meta through connection inspector and dependency selection dialog
- Tool details panel: add UI/JSON view toggle, render MCPAppRenderer in the result pane when tool has a UI resource; replace XClose clear button with a Cancel button during execution - Chat tool call parts: render MCPAppRenderer below tool call output when tool has a UI resource; add hasMCPApp-aware icon switching (LayersTwo01 vs Atom02); propagate toolMeta to AnnotationBadges
The previous logic omitted credentials whenever an explicit apiBaseUrl was provided, which incorrectly blocked cookie-based auth for same-origin API calls in embedded contexts. Now credentials are included only when the resolved URL shares the current page origin.
The resource-loader module it tested has been superseded by the new MCPAppRenderer / useAppBridge architecture.
0581656 to
e86b59f
Compare
…n chat Wraps the MCPAppRenderer Suspense boundary with an ErrorBoundary in chat tool call parts. The error fallback uses a dashed banner (matching StatusHighlight pattern) with AlertCircle icon, tool name context, and a styled Retry button that resets the error state.
Add two new CSP directives to support libraries that require runtime code generation: - unsafeEval: enables 'unsafe-eval' in script-src for libraries like knockout.js that use eval() - wasmEval: enables 'wasm-unsafe-eval' in script-src for WebAssembly compilation (e.g., CesiumJS) Also add worker-src blob: to support Web Workers.
Create a new 'UI' tab in the connection detail view that displays only tools with interactive UI metadata (_meta containing UI resource URI). Features: - Gallery grid layout with responsive card sizing (320px min) - Each card shows a scaled-down preview of the tool's UI using MCPAppRenderer in inline display mode - Tool name, description, and annotation badges below preview - Search/filter by tool name or description - Click card to navigate to tool detail page - Shows empty state when no UI tools available The UI tab appears after Resources tab when authenticated and at least one tool has UI metadata.
Bake 'unsafe-eval' and 'wasm-unsafe-eval' into the default Content Security Policy for sandboxed iframe rendering. Libraries like CesiumJS require these capabilities for runtime code generation (knockout.js) and WebAssembly compilation. Rather than adding non-standard fields to McpUiResourceCsp, always emit these tokens for any MCP app rendered in Mesh. The iframe sandbox remains the primary security boundary. Also ensure 'worker-src blob:' is always present to allow Web Workers created from blob: URLs.
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/mesh/src/mcp-apps/csp-injector.ts">
<violation number="1" location="apps/mesh/src/mcp-apps/csp-injector.ts:5">
P1: Unconditionally enabling `'unsafe-eval'` in the CSP for all MCP app iframes removes the previous opt-in granularity and violates the principle of least privilege. If any embedded MCP app has an injection vulnerability, `unsafe-eval` allows an attacker to execute arbitrary code via `eval()`, `new Function()`, etc.
The previous design — gating on `rc.unsafeEval` / `rc.wasmEval` — was strictly safer because only apps that declared a need for eval got it. Consider restoring the opt-in behavior and only adding these directives when the app's resource CSP requests them.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
|
||
| export const DEFAULT_CSP = [ | ||
| "default-src 'none'", | ||
| "script-src 'unsafe-inline' 'unsafe-eval' 'wasm-unsafe-eval'", |
There was a problem hiding this comment.
P1: Unconditionally enabling 'unsafe-eval' in the CSP for all MCP app iframes removes the previous opt-in granularity and violates the principle of least privilege. If any embedded MCP app has an injection vulnerability, unsafe-eval allows an attacker to execute arbitrary code via eval(), new Function(), etc.
The previous design — gating on rc.unsafeEval / rc.wasmEval — was strictly safer because only apps that declared a need for eval got it. Consider restoring the opt-in behavior and only adding these directives when the app's resource CSP requests them.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/mesh/src/mcp-apps/csp-injector.ts, line 5:
<comment>Unconditionally enabling `'unsafe-eval'` in the CSP for all MCP app iframes removes the previous opt-in granularity and violates the principle of least privilege. If any embedded MCP app has an injection vulnerability, `unsafe-eval` allows an attacker to execute arbitrary code via `eval()`, `new Function()`, etc.
The previous design — gating on `rc.unsafeEval` / `rc.wasmEval` — was strictly safer because only apps that declared a need for eval got it. Consider restoring the opt-in behavior and only adding these directives when the app's resource CSP requests them.</comment>
<file context>
@@ -2,7 +2,7 @@ import type { McpUiResourceCsp } from "./types.ts";
export const DEFAULT_CSP = [
"default-src 'none'",
- "script-src 'unsafe-inline'",
+ "script-src 'unsafe-inline' 'unsafe-eval' 'wasm-unsafe-eval'",
"style-src 'unsafe-inline'",
"img-src * data: blob:",
</file context>
There was a problem hiding this comment.
let's make it work with less setup right now
There was a problem hiding this comment.
Understood—I'll defer to the current approach and won’t push for the opt-in CSP change right now.
What is this contribution about?
This fix bakes
'unsafe-eval'and'wasm-unsafe-eval'into the default Content Security Policy (CSP) for all MCP apps rendered in Mesh's sandboxed iframes.Libraries like CesiumJS require these capabilities:
'unsafe-eval'for runtime code generation (knockout.js bindings)'wasm-unsafe-eval'for WebAssembly compilation'worker-src blob:'for Web Workers created from blob URLsRather than adding non-standard fields to the upstream
McpUiResourceCsptype, we now emit these tokens unconditionally. The iframe sandbox (allow-scripts allow-same-origin allow-forms) remains the primary security boundary.Screenshots/Demonstration
N/A
How to Test
bun test apps/mesh/src/mcp-apps/grep "script-src.*unsafe-eval.*wasm-unsafe-eval" apps/mesh/src/mcp-apps/csp-injector.tsMigration Notes
N/A
Review Checklist