feat: custom chart plugins ("vibe coded charts")#945
Conversation
🧹 Preview RemovedThe preview deployment for this PR has been cleaned up. |
There was a problem hiding this comment.
14 issues found across 37 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
- require auth on chart plugin routes and scope plugins to the request's project - initialize plugin service on the SSE /events route - make plugin service init retry-safe and project-aware (no permanent disable) - retry chart plugin manifest fetch after transient failures; avoid unhandled rejection - preserve date x-axis semantics for custom charts - surface failed chart renders to automation email/slack tool callers - show a fallback note in Slack when a chart image can't be rendered - guard story export against plugin init failures - make server-side chart CDN configurable via NAO_CHART_CDN_URL - only async-close headless browser on SIGINT/SIGTERM - avoid Math.max spread RangeError and empty-colors modulo in example charts - drop unused variable in debug example chart
f508966 to
b621e16
Compare
Addresses three HIGH-severity security findings on the custom chart plugin system: - Replace the process-global singleton plugin state with per-project state so concurrent requests for different projects can't switch the shared selection between init and read (cross-tenant metadata/source leak). All reads now take a projectId. - Containment-check plugin files with realpath before reading, so a .js symlink inside agent/charts can't disclose arbitrary host files via the source route. - Thread the owning project's id through story export / custom chart rendering (story-download, embed-story, automation/messaging senders) instead of falling back to getDefaultProject(), so a story renders only its own project's plugins.
There was a problem hiding this comment.
2 issues found across 18 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Pre-existing formatting drift (from #952) that fails the repo-wide `prettier --check` in CI; reformat to unblock the pipeline.
…mlink check - Restrict the headless-browser render of custom chart plugins to public http(s) hosts: requests to loopback/private/link-local/reserved addresses (e.g. cloud metadata 169.254.169.254, internal services) are aborted via Puppeteer request interception, preventing SSRF / internal exfiltration when untrusted project plugins run server-side. Adds `private-network` guard with DNS resolution (fail-closed). - Reject a project `agent/charts` folder that resolves (via symlink) outside the canonical project root, closing the remaining plugin-source traversal vector.
Move the custom chart plugin guide from .claude/skills/chart-plugins to skills/build-custom-charts so it ships in the nao skills registry and can be installed with the skills CLI (npx skills add / nao skills). Adds registry `name`/`description` frontmatter, starter-example pointers, and a README entry.
There was a problem hiding this comment.
2 issues found across 4 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
isPrivateIpv6 only matched the dotted-quad ::ffff:a.b.c.d form, so hex (::ffff:7f00:1), uncompressed, IPv4-compatible (::a.b.c.d) and NAT64 addresses bypassed the private-host block and could reach loopback, link-local (metadata) and RFC1918 targets. Expand any IPv6 literal to its eight hextets and validate the embedded IPv4 for every mapped/compatible form.
The egress guard resolved url.hostname in Node and then let Chromium re-resolve and connect via request.continue(), leaving a TOCTOU window where a name could resolve to a public IP at check time and a private IP at fetch time (DNS rebinding to internal/metadata services). Route plugin egress through a local forward proxy bound to an isolated browser context. The proxy resolves each host exactly once, blocks private/reserved targets, and connects to that pinned IP, so Chromium never performs its own DNS resolution. proxyBypassList '<-loopback>' forces loopback through the proxy too. Request interception now only filters URL schemes; IP enforcement and pinning live in the proxy.
There was a problem hiding this comment.
2 issues found across 4 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/backend/src/utils/render-custom-chart.ts">
<violation number="1" location="apps/backend/src/utils/render-custom-chart.ts:49">
P2: Browser context is not cleaned up if `context.newPage()` throws before the `try` block. If creating the new page fails (e.g., resource exhaustion), the `context` created on the previous line leaks because `context.close()` is only called in the `finally` block which is never reached.</violation>
</file>
<file name="apps/backend/src/utils/ssrf-proxy.ts">
<violation number="1" location="apps/backend/src/utils/ssrf-proxy.ts:111">
P1: Port parsing in `parseAuthority()` does not enforce valid TCP port bounds (1–65535). Out-of-range integer ports pass validation and are forwarded to `net.connect()` / `http.request()`, which synchronously throw `ERR_SOCKET_BAD_PORT`. Because the handlers use uncaught `void (async () => {...})()` IIFEs, these throws become unhandled promise rejections and can leave `clientSocket` resources dangling instead of returning a clean error response to the browser.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| })(); | ||
| } | ||
|
|
||
| function parseAuthority(authority: string, defaultPort: number): { host: string; port: number } { |
There was a problem hiding this comment.
P1: Port parsing in parseAuthority() does not enforce valid TCP port bounds (1–65535). Out-of-range integer ports pass validation and are forwarded to net.connect() / http.request(), which synchronously throw ERR_SOCKET_BAD_PORT. Because the handlers use uncaught void (async () => {...})() IIFEs, these throws become unhandled promise rejections and can leave clientSocket resources dangling instead of returning a clean error response to the browser.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/backend/src/utils/ssrf-proxy.ts, line 111:
<comment>Port parsing in `parseAuthority()` does not enforce valid TCP port bounds (1–65535). Out-of-range integer ports pass validation and are forwarded to `net.connect()` / `http.request()`, which synchronously throw `ERR_SOCKET_BAD_PORT`. Because the handlers use uncaught `void (async () => {...})()` IIFEs, these throws become unhandled promise rejections and can leave `clientSocket` resources dangling instead of returning a clean error response to the browser.</comment>
<file context>
@@ -0,0 +1,130 @@
+ })();
+}
+
+function parseAuthority(authority: string, defaultPort: number): { host: string; port: number } {
+ const lastColon = authority.lastIndexOf(':');
+ if (authority.startsWith('[')) {
</file context>
| // `<-loopback>` cancels Chromium's implicit loopback bypass so even | ||
| // localhost/127.0.0.1 targets are forced through (and blocked by) the proxy. | ||
| const context = await browser.createBrowserContext({ proxyServer, proxyBypassList: ['<-loopback>'] }); | ||
| const page = await context.newPage(); |
There was a problem hiding this comment.
P2: Browser context is not cleaned up if context.newPage() throws before the try block. If creating the new page fails (e.g., resource exhaustion), the context created on the previous line leaks because context.close() is only called in the finally block which is never reached.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/backend/src/utils/render-custom-chart.ts, line 49:
<comment>Browser context is not cleaned up if `context.newPage()` throws before the `try` block. If creating the new page fails (e.g., resource exhaustion), the `context` created on the previous line leaks because `context.close()` is only called in the `finally` block which is never reached.</comment>
<file context>
@@ -41,7 +41,12 @@ export async function renderCustomChartImage(input: RenderCustomChartInput): Pro
+ // `<-loopback>` cancels Chromium's implicit loopback bypass so even
+ // localhost/127.0.0.1 targets are forced through (and blocked by) the proxy.
+ const context = await browser.createBrowserContext({ proxyServer, proxyBypassList: ['<-loopback>'] });
+ const page = await context.newPage();
try {
await restrictNetworkEgress(page);
</file context>
Resolve conflicts: - embed.routes.ts: combine main's download analytics event with per-project projectId threading for custom chart plugin scoping. - skills/README.md: keep main's improved write-context-rules description plus the build-custom-charts skill row. - embed-story.ts: drop duplicate projectId field from an auto-merge artifact (both branches added it).
socallmebertille
left a comment
There was a problem hiding this comment.
The PR is great, I noticed a few details that I'll let you take a look at
| export async function closeSsrfProxy(): Promise<void> { | ||
| const server = proxyServer; | ||
| proxyServer = null; | ||
| proxyPromise = null; | ||
| if (server) { | ||
| await new Promise<void>((resolve) => server.close(() => resolve())); | ||
| } | ||
| } |
There was a problem hiding this comment.
Function currently unused, unlike the headless browser it serves, this isn't wired to SIGINT/SIGTERM, so wiring it there would free the listening socket on shutdown
| // `chart_type` is intentionally not validated against a fixed list: | ||
| // besides the built-in types, projects can define custom chart plugins | ||
| // in `agent/charts/`, whose names are only known at runtime. |
There was a problem hiding this comment.
Function validateStoryCode no longer validates chart_type at all — the VALID_CHART_TYPES check was removed, so typos on built-ins and references to non-existent custom charts now pass silently, I think you should restore validation by passing the known types (built-ins ∪ custom plugins)
| // Avoid re-attempting (and re-failing) the same chart on later stream chunks. | ||
| state.renderedChartIds.add(part.toolCallId); | ||
| const chartName = part.input.title ? `"${part.input.title}"` : 'a chart'; | ||
| ctx.textBlockIndex = -1; | ||
| ctx.blocks.push(createTextBlock(`_⚠️ Could not render ${chartName} as an image._`)); | ||
| await ctx.convMessage?.edit(Card({ children: ctx.blocks })); |
There was a problem hiding this comment.
This fallback is only done in Slack, not in Teams, Telegram or WhatsApp
| try { | ||
| const displaySettings = await projectQueries.getDisplaySettings(this.projectId); | ||
| const png = generateChartImage({ | ||
| const png = await renderChartImage({ |
There was a problem hiding this comment.
trpc/chart.routes.ts still uses generateChartImage and the renderChartToSvg that throw for a non-builtin type
| return; | ||
| } | ||
| try { | ||
| state.watcher = watch(state.pluginsFolderPath, { recursive: true }, (eventType) => { |
There was a problem hiding this comment.
The recursive boolean is useless because all the custom charts live in the same folder, there is no need to be watch the directory tree
| state.watcher = watch(state.pluginsFolderPath, { recursive: true }, (eventType) => { | |
| state.watcher = watch(state.pluginsFolderPath, (eventType) => { |
| import * as React from '${CDN}/react@19.2.0'; | ||
| import * as ReactDOM from '${CDN}/react-dom@19.2.0/client'; | ||
| import * as Recharts from '${CDN}/recharts@2.15.4?deps=react@19.2.0,react-dom@19.2.0'; |
There was a problem hiding this comment.
It would be great to derive these versions from apps/frontend/package.json (or a shared constant) so the server-side render can never de-synchronise from the browser bundle when react or recharts get updated
|
done in #1242 |


What & why
Adds a chart extension/plugin system so users can define their own chart types ("vibe coded charts") without modifying nao. Plugins live in a project's
agent/charts/folder and are rendered through the existing chart interface — the agent still callsdisplay_chartwith aquery_id+series, only the set of possiblechart_types is extended.Plugins are plain ES modules (vanilla JS, Recharts via injected libs, or any CDN library) — no bundler or build step. When running locally via
nao chat, edits to plugin files hot reload in the browser. Hot reload is not required for Docker deployments (custom charts still render there; only the live-reload watcher is gated off).Walkthrough
Custom
bubble(Recharts) andprogress_bars(vanilla JS) plugins rendering through the normal chat chart interface:Custom bubble and progress-bar charts rendered
Editing a plugin file hot reloads the chart in place (no page refresh) — bubbles change color/size at ~00:20:
custom_chart_hot_reload_demo.mp4
How it works
agent/charts/<type>.jsdefines a chart; the file name is thechart_type.chartPluginServicediscovers plugins, lists them to the agent in the system prompt, and serves them at/api/charts/plugins/<type>.js(manifest at/api/charts/plugins, hot-reload SSE at/api/charts/events).display_chartwithchart_type: "<type>".CustomChartdynamically imports the module and callsrender(element, ctx), injecting{ React, ReactDOM, Recharts }, data, config, theme and the color palette.Plugin contract
Key changes
apps/shared: newchart-plugin.tscontract;chart_typerelaxed from a strict enum to a string (withisBuiltinChartType/BUILTIN_CHART_TYPEShelpers); story validation no longer flags non-built-in types.apps/backend:chart-plugin.service.ts(discovery + watcher + reload events),routes/chart-plugins.ts(manifest/source/SSE),NAO_CHART_HOT_RELOADenv +chartHotReloadEnabled, system-prompt section listing available custom charts,display_charttool validates custom types, server-side PNG guarded.apps/frontend:lib/chart-plugins.ts(loader + hot-reload subscription),CustomChartrenderer integrated intoChartDisplay; PNG/edit hidden for custom charts.example/agent/charts/:bubble.js(Recharts) andprogress_bars.js(vanilla) examples..claude/skills/chart-plugins/SKILL.md: documents the system for coding agents.Testing
npm run lint(backend, frontend, shared) passes.npm run -w @nao/shared testpasses (incl. updated story-validation test)./api/charts/pluginsmanifest +/api/charts/plugins/<type>.jsserving +/api/charts/eventsSSE (version bumps on file change).bubble+progress_barscharts; both render through the realdisplay_chartpipeline; editing the plugin hot reloads the chart in place (see video).Note: backend test suite has pre-existing failures unrelated to this change (sqlite/auth/formatDate), confirmed on the clean base commit.
To show artifacts inline, enable in settings.