Repository navigation
feat(mcp): add native result previews and interactive apps - #8786
waleedlatif1 wants to merge 7 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
|
d0e5107 to
e0b1573
Compare
|
@cubic-dev-ai review this PR |
@waleedlatif1 I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
1 issue found across 74 files
Confidence score: 2/5
- In
apps/sim/lib/mcp/app-frame.ts, split-horizon DNS can make a hostname pass validation while resolving to a private IP, allowing the generated CSP to authorize requests to a LAN host. Enforce the private-address restriction on the resolved destination.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/sim/lib/mcp/app-frame.ts">
<violation number="1" location="apps/sim/lib/mcp/app-frame.ts:19">
P1: A split-horizon hostname can pass this check while resolving to a private IP, after which the generated CSP authorizes App requests to that LAN host. Enforce the private-address restriction against resolved destinations, not only hostname text.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Turn on auto-fix | Re-trigger cubic
| if (url.protocol === 'wss:' && !allowWebSocket) | ||
| throw new OrchestrationError('validation', 'Static App resources require HTTPS') | ||
| const hostname = url.hostname.replace(/\.$/, '') | ||
| if (hostname === 'localhost' || hostname.endsWith('.localhost') || /^[\d.]+$/.test(hostname)) |
There was a problem hiding this comment.
P1: A split-horizon hostname can pass this check while resolving to a private IP, after which the generated CSP authorizes App requests to that LAN host. Enforce the private-address restriction against resolved destinations, not only hostname text.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At apps/sim/lib/mcp/app-frame.ts, line 19:
<comment>A split-horizon hostname can pass this check while resolving to a private IP, after which the generated CSP authorizes App requests to that LAN host. Enforce the private-address restriction against resolved destinations, not only hostname text.</comment>
<file context>
@@ -0,0 +1,79 @@
+ if (url.protocol === 'wss:' && !allowWebSocket)
+ throw new OrchestrationError('validation', 'Static App resources require HTTPS')
+ const hostname = url.hostname.replace(/\.$/, '')
+ if (hostname === 'localhost' || hostname.endsWith('.localhost') || /^[\d.]+$/.test(hostname))
+ throw new OrchestrationError('validation', 'MCP Apps cannot access local network addresses')
+ return domain
</file context>
There was a problem hiding this comment.
Confirmed: the hostname check is not destination-IP isolation. A server-side DNS preflight cannot pin the address used by the browser, so it would not close split-horizon DNS or rebinding across browser engines. Leaving this thread open while the App networking policy is finalized; the current implementation does not guarantee that declared HTTPS origins cannot reach a private network.
|
@cubic-dev-ai review this PR |
@waleedlatif1 I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
1 issue found across 82 files
Confidence score: 4/5
- The message injections in
apps/sim/scripts/test-mcp-app-e2e.tsdon’t verify theevent.sourceguard: the proxy targets itself, and the host message has a different origin from the sandboxed app. Adjust the fixtures to isolate the source check; an origin-only check could pass these tests.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/sim/scripts/test-mcp-app-e2e.ts">
<violation number="1" location="apps/sim/scripts/test-mcp-app-e2e.ts:298">
P2: These injections do not verify the `event.source` guard: the proxy message targets itself, and the host message has a different origin from the sandboxed app. An origin-only check would pass while accepting messages from another opaque-origin frame; send the forged RPC from a second sandboxed frame and assert that no tool call occurs.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Turn on auto-fix | Re-trigger cubic
12c6c28 to
027adbc
Compare
|
@cubic-dev-ai review this PR |
@waleedlatif1 I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
All reported issues were addressed across 83 files
Reply with feedback, questions, or to request a fix.
Turn on auto-fix | Re-trigger cubic
|
@cubic-dev-ai review this PR |
@waleedlatif1 I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
No issues found across 83 files
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
You've manually re-run cubic several times on this PR. Each manual re-review checks the full PR again and counts toward your usage quota. To preserve your usage limits, we recommend letting cubic automatically review new commits.
Turn on auto-fix | Re-trigger cubic
69029ee to
79d39e7
Compare
|
@cubic-dev-ai review this PR |
@waleedlatif1 I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
No issues found across 84 files
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
You've manually re-run cubic several times on this PR. Each manual re-review checks the full PR again and counts toward your usage quota. To preserve your usage limits, we recommend letting cubic automatically review new commits.
Turn on auto-fix | Re-trigger cubic
79d39e7 to
ad03e77
Compare
|
@cubic-dev-ai review this PR |
@waleedlatif1 I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
No issues found across 84 files
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
You've manually re-run cubic several times on this PR. Each manual re-review checks the full PR again and counts toward your usage quota. To preserve your usage limits, we recommend letting cubic automatically review new commits.
Turn on auto-fix | Re-trigger cubic
|
@cubic-dev-ai review this PR |
@waleedlatif1 I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
No issues found across 84 files
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
You've manually re-run cubic several times on this PR. Each manual re-review checks the full PR again and counts toward your usage quota. To preserve your usage limits, we recommend letting cubic automatically review new commits.
Turn on auto-fix | Re-trigger cubic
Summary
Type of Change
Testing
Checklist
test-auditauthoring gate)