fix: sanitize marimo-mpl-interactive marimo-panel#9133
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Bundle ReportChanges will increase total bundle size by 12.21kB (0.05%) ⬆️. This is within the configured threshold ✅ Detailed changes
Affected Assets, Files, and Routes:view changes for bundle: marimo-esmAssets Changed:
Files in
Files in
|
There was a problem hiding this comment.
Pull request overview
This PR hardens plugin script/style loading to mitigate same-origin script injection via raw <marimo-*> elements embedded in markdown, by ensuring extensions are served from Marimo virtual files and validating URLs on the frontend.
Changes:
- Serve Panel’s extension JavaScript via a
./@file/...virtual file URL instead of inlining it in the DOM. - Add a shared frontend
isTrustedVirtualFileUrlvalidator and apply it to Panel, mpl-interactive, and anywidget loaders. - Add targeted unit tests (Python + Vitest) covering the new URL validation and the Panel virtual-file attribute.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/_plugins/ui/_impl/test_panel.py | Adds regression test ensuring Panel extension is referenced via data-extension-url and points at @file/. |
| marimo/_plugins/ui/_impl/from_panel.py | Switches Panel extension delivery from inline JS to a registered VirtualFile URL. |
| frontend/src/plugins/impl/panel/PanelPlugin.tsx | Loads Panel extension via <script src> and refuses non-virtual-file URLs. |
| frontend/src/plugins/impl/panel/tests/PanelPlugin.test.ts | Tests trusted vs untrusted extension URLs for Panel. |
| frontend/src/plugins/impl/mpl-interactive/MplInteractivePlugin.tsx | Refuses untrusted mpl.js/CSS URLs and exposes internals for testing. |
| frontend/src/plugins/impl/mpl-interactive/tests/MplInteractivePlugin.test.tsx | Tests URL refusal/acceptance behavior for mpl-interactive asset loading. |
| frontend/src/plugins/impl/anywidget/widget-binding.ts | Refuses untrusted dynamic-import URLs for anywidget modules. |
| frontend/src/plugins/impl/anywidget/tests/widget-binding.test.ts | Adds URL validation tests for anywidget registry imports. |
| frontend/src/plugins/core/trusted-url.ts | Introduces shared “trusted virtual file URL” validator. |
| frontend/src/plugins/core/test/trusted-url.test.ts | Unit tests for trusted/untrusted virtual file URL patterns. |
| if (typeof url !== "string" || url.length === 0) { | ||
| return false; | ||
| } | ||
| return /^(\.?\/)?@file\/[^?#]+$/.test(url); |
There was a problem hiding this comment.
The current regex allows additional "/" characters after the @file/ prefix (e.g. "./@file/../auth/token"). Browsers normalize dot-segments in URLs, so this can resolve outside the virtual-file route and reintroduce same-origin script/style injection. Tighten validation so the portion after "@file/" is a single path segment (no "/"), and consider rejecting percent-encoded slashes/dot-segments; add a unit test for the "./@file/../..." case.
| return /^(\.?\/)?@file\/[^?#]+$/.test(url); | |
| // Reject percent-encoded separators / dot-segments that browsers may | |
| // normalize into paths outside the virtual-file route. | |
| if (/%2f|%5c|%2e/i.test(url)) { | |
| return false; | |
| } | |
| // Only allow a single path segment after "@file/". | |
| return /^(\.?\/)?@file\/[^\/\\?#]+$/.test(url); |
| extension_url: Optional[str] = None | ||
| if extension: | ||
| extension_vfile = VirtualFile.create_and_register( | ||
| extension.encode("utf-8"), "js" | ||
| ) | ||
| extension_url = extension_vfile.url |
There was a problem hiding this comment.
VirtualFile.create_and_register() registers the extension script in the runtime virtual-file registry but there’s no corresponding cleanup or caching here. Because VirtualFile filenames are randomized, rerunning cells / creating multiple Panel widgets can accumulate many large JS virtual files in storage over a long session. Consider caching the extension VirtualFile (e.g. module-level/lru_cache keyed by the extension contents) and/or using a lifecycle-managed virtual file mechanism so these entries don’t grow without bound.
| } | ||
| const script = document.createElement("script"); | ||
| script.src = extensionUrl; |
There was a problem hiding this comment.
loadPanelExtension() appends a new <script> tag every time it’s called, even if the same extension URL was already loaded. If multiple panel elements mount (or a cell re-runs), this can duplicate network fetches and re-execute the extension script unnecessarily. Consider deduplicating by checking for an existing script with the same src (or checking a global “extension loaded” marker) before appending.
| } | |
| const script = document.createElement("script"); | |
| script.src = extensionUrl; | |
| } | |
| const script = document.createElement("script"); | |
| script.src = extensionUrl; | |
| const normalizedSrc = script.src; | |
| const existingScript = Array.from(document.scripts).find( | |
| (existing) => existing.src === normalizedSrc, | |
| ); | |
| if (existingScript) { | |
| return true; | |
| } |
No description provided.