feat: allow RSC server actions in sandbox #1133
Conversation
dosubot
bot
added
size:XS
WalkthroughThe sandboxed iframe component now derives the outer iframe's 📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🔍 Remote MCPSummary of Relevant Context for PR ReviewTechnical Background on RSC Server Actions and Form HandlingServer Functions (formerly called Server Actions) can be passed to a Form to automatically submit the form to the server. On the browser, the HTML form element is the traditional approach for a user to submit a mutation, and with React Server Components, React introduces first-class support for Server Functions as Actions in forms. The PR's claim about form semantics being critical is well-founded: When a Server Function is passed to a form, and a user submits that form, there is a network request to the server function, and React will supply the form's FormData as the first argument to the Server Function. Nested iframe Sandbox RestrictionsThe PR's technical rationale about outer and inner iframe sandboxing is accurate. Forms are disabled because the inner iframe does not have the allow-forms keyword set. More specifically, the sandboxing flags applied to a frame also apply to any windows or frames created in the sandbox, meaning that you have to add allow-forms to the frame's sandbox, even though the form only exists in the window that the frame pops up. This confirms the PR's assessment: while the inner iframe already had Repository ContextMCPJam Inspector is the local development client for ChatGPT apps, MCP apps (Claude), and MCP servers, used to build and test apps with a full widget emulator, chat with any LLM, and inspect your server's tools, resources, prompts, and OAuth flows. The change to support RSC server actions in the sandboxed iframe aligns with enabling modern React features in the inspector's embedded widget environment. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
client/src/components/ui/sandboxed-iframe.tsx
🧰 Additional context used
📓 Path-based instructions (1)
client/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Browser
console.*methods are acceptable for client-side debugging in client code
Files:
client/src/components/ui/sandboxed-iframe.tsx
🔍 Remote MCP
Summary of Relevant Context for PR Review
React Server Components & Forms Context
When a form is rendered by a Server Component and a Server Function is passed to the form's action prop, the form is progressively enhanced. React extends the HTML
element to allow Server Actions to be invoked with the action attribute, and when used in a form, the function automatically receives the FormData object. This is the core use case the PR addresses.iframe Sandbox Security Implications
Critical Security Concern: When the embedded document has the same origin as the embedding page, it is strongly discouraged to use both allow-scripts and allow-same-origin, as that lets the embedded document remove the sandbox attribute — making it no more secure than not using the sandbox attribute at all. This is particularly relevant since the PR's component uses both allow-scripts and allow-same-origin.
Form Submission Blocking Issue: If a page embedded within an <iframe> without a sandbox="allow-forms" or sandbox="allow-popups-to-escape-sandbox" attribute set on it opens a new site in a separate tab, form submission in that new browsing context will silently fail. This directly supports the PR's justification for adding allow-forms.
Security Best Practice: The sandbox attribute works like parental controls for iframes. It restricts potentially dangerous activities and lets you selectively grant permissions only when necessary. Additionally, the safest approach is to enable as few permissions as possible to reduce the security risk surface.
MCPJam Inspector Context
MCP Inspector acts as an MCP client that connects to MCP servers and provides a web interface for testing and debugging, with a frontend using React-based UI with TypeScript for type safety and an API Layer using Next.js API routes that handle MCP communications. MCPJam Inspector is the local development client for ChatGPT apps, MCP apps (Claude), and MCP servers, allowing you to build and test apps with a full widget emulator, chat with any LLM, and inspect your server's tools, resources, prompts, and OAuth flows.
Key Review Considerations
-
Functional Necessity: The PR correctly identifies that outer iframe sandbox restrictions block form submissions needed for RSC server actions, even when the inner iframe permits forms.
-
Security Trade-offs: Reviewers should verify that the conditional application of
allow-formsis scoped appropriately (only when the provided sandbox prop includes it), and consider whether the existing combination ofallow-scripts+allow-same-originrepresents an acceptable risk for this use case. -
Suggested Enhancement: The PR description mentions
allow-popupsmay need similar consideration—reviewers may want to determine if this should be addressed in this PR or as a separate concern.
🔇 Additional comments (2)
client/src/components/ui/sandboxed-iframe.tsx (2)
194-197: Consider conditional handling forallow-popups.The PR description notes that
allow-popupsmay require similar treatment. The defaultsandboxprop (line 74) includesallow-popups, yetouterSandboxdoes not conditionally propagate it. Should this PR address both permissions, or isallow-popupsdeferred to separate work?Based on external tools context: form submissions can silently fail in new browsing contexts opened from iframes without
allow-popups-to-escape-sandbox, which may relate to the popup consideration mentioned in your PR description.
200-208: Verify that different-origin strategy sufficiently mitigates sandbox escape risk.Combining
allow-scriptsandallow-same-origincan permit the embedded document to remove thesandboxattribute entirely. While this component enforces different origins (localhost ↔ 127.0.0.1, lines 89-108), confirming that this approach provides robust protection against sandbox escape would be prudent.Based on external tools context: it is strongly discouraged to use both
allow-scriptsandallow-same-originwhen the embedded document shares the same origin as the embedding page. Your different-origin enforcement should mitigate this, but verification against known bypass techniques would strengthen confidence.
dosubot
bot
added
size:S
|
Hi Jan @hybrist, really appreciate you taking the time to create this PR. We recently did a force push that reset the project. This was to fix the 200MB project size issue. Now all PRs are facing this issue where PR's are recommiting the old inspector. Could you delete your inspector project and re-fork, then create the PR? Apologies for the inconvenience. Happy to also make this change myself. |
|
@matteo8p Did a quick reset+cherry-pick of the commit. Does that work for you? |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
client/src/components/ui/sandboxed-iframe.tsx (1)
118-125: Token-based matching correctly addresses the past review concern.The Set-based approach ensures exact token matching and eliminates false positives from substring matches.
Consider extending conditional logic to
allow-popups.The PR description notes "a similar consideration may apply to allow-popups," and the default
sandboxprop (line 74) includes bothallow-formsandallow-popups. If the inner iframe requestsallow-popupsbut the outer iframe doesn't propagate it, popup-based interactions will fail.Should
allow-popupsalso be conditionally added toouterSandboxwhen present in thesandboxprop?Optional refinements for clarity.
🔎 Minor improvements
const outerSandbox = useMemo(() => { - const tokens = new Set(sandbox.split(/\s+/g)) - let outerSandbox = "allow-scripts allow-same-origin"; + const tokens = new Set(sandbox.split(/\s+/)); + let result = "allow-scripts allow-same-origin"; if (tokens.has("allow-forms")) { - outerSandbox += " allow-forms"; + result += " allow-forms"; } - return outerSandbox; + return result; }, [sandbox]);
- The
/gflag has no effect insplit()as it already processes the entire string- Renaming the local variable avoids shadowing the outer
outerSandboxidentifier
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
client/src/components/ui/sandboxed-iframe.tsx
🧰 Additional context used
📓 Path-based instructions (1)
client/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Browser
console.*methods are acceptable for client-side debugging in client code
Files:
client/src/components/ui/sandboxed-iframe.tsx
🔍 Remote MCP
Summary of Additional Context Found
Based on my research, here is the relevant context for reviewing this PR:
React Server Components & Form Semantics Context
Server Functions can be passed to a Form to automatically submit the form to the server, and by passing a Server Function to the form action, React can progressively enhance the form, allowing forms to be submitted before the JavaScript bundle is loaded. This validates the PR's rationale that form semantics are foundational to RSC server action invocation patterns.
iframe Sandbox Security Considerations
Critical Security Context: When the embedded document has the same origin as the embedding page, it is strongly discouraged to use both allow-scripts and allow-same-origin, as that lets the embedded document remove the sandbox attribute. This is relevant because the PR adds dynamic form support to an outer iframe that already uses allow-scripts and allow-same-origin.
Form Submissions & Nested Frames: If a page embedded within an iframe without allow-forms or allow-popups-to-escape-sandbox opens a new site in a separate tab, form submission in that new browsing context will silently fail. Additionally, sandboxing flags applied to a frame also apply to any windows or frames created in the sandbox, so allow-forms must be added to the frame's sandbox even if the form only exists in a window the frame pops up.
Best Practices: Only enable the minimum necessary permissions to reduce the attack surface, and the safest approach is to enable as few permissions as possible to reduce the security risk surface.
Project Context (MCPJam Inspector)
The MCPJam Inspector UI loads HTML contents from MCP resources into a sandboxed iFrame, where the UI in the iFrame can talk to the MCP server via JSON-RPC. This explains why the sandboxed iframe component is critical to the project's functionality for building and testing MCP apps.
🔇 Additional comments (1)
client/src/components/ui/sandboxed-iframe.tsx (1)
207-207: Correct application of the computed sandbox attribute.The dynamic
outerSandboxvalue enables the intended progressive form permission alignment while preserving the existing security model.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
client/src/components/ui/sandboxed-iframe.tsx (1)
118-125: Token-based matching correctly resolves the previous concern.The Set-based approach with split on whitespace properly handles exact token matching, eliminating false positives from substrings like "disallow-forms".
Minor style note: Line 119 lacks a semicolon, inconsistent with the surrounding code (line 120).
Optional: Consider extending to allow-popups
The PR description mentions
allow-popupsas a similar consideration. To future-proof, you might extend the logic:const outerSandbox = useMemo(() => { const tokens = new Set(sandbox.split(/\s+/)); let result = "allow-scripts allow-same-origin"; if (tokens.has("allow-forms")) { result += " allow-forms"; } + if (tokens.has("allow-popups")) { + result += " allow-popups"; + } return result; }, [sandbox]);
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
client/src/components/ui/sandboxed-iframe.tsx
🧰 Additional context used
📓 Path-based instructions (1)
client/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Browser
console.*methods are acceptable for client-side debugging in client code
Files:
client/src/components/ui/sandboxed-iframe.tsx
🔇 Additional comments (1)
client/src/components/ui/sandboxed-iframe.tsx (1)
207-207: Dynamic sandbox attribute enables the intended functionality.Replacing the static string with the computed
outerSandboxcorrectly propagates the conditionalallow-formspermission, fulfilling the PR's goal of enabling RSC server actions while maintaining security through minimal default permissions.
2 participants
The inner
<iframe>already setallow-formsbut that was effectively ignored because the outer<iframe>blocked all forms. Allowing forms is relevant for React setups that attempt to invoke server actions. A typical pattern for invoking server actions is layered on top of form submissions, e.g.:Even though these won't necessarily submit an actual form client-side, the event handling is associated with form submissions. Blocking forms prevents the JS logic from even seeing the form submissions.
I'm mostly sending this PR because the inner sandbox already defaulted to
allow-formswhich made it look like the intention was to allow forms inside of the sandbox. A similar argument may apply toallow-popups.