Repository navigation
Conversation
9ba84b4 to
c8f8282
Compare
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| }, | ||
| }; | ||
|
|
||
| void connectFrame(frame, handlers, controller.signal).then(async (rpc) => { |
There was a problem hiding this comment.
WARNING: Race condition between connection cleanup and promise resolution
The connectFrame promise resolution (line 118-133) checks disposed after awaiting, but the cleanup function (lines 135-140) runs synchronously during unmount. If the component unmounts while connectFrame is pending, the cleanup sets disposed = true and aborts the signal, but the then callback may still execute after cleanup completes. The rpcRef.current?.close() in cleanup runs before rpcRef.current is set, leaving the resolved RPC unclosed. Consider storing the connectFrame promise and awaiting it in cleanup, or using a ref to track the pending connection.
| }); | ||
| // Sandboxed frames have an opaque origin; the source window is checked above | ||
| // and the transferred port is unique to this frame instance. | ||
| frame.contentWindow?.postMessage({ type: "tabularis:init" }, "*", [port2]); |
There was a problem hiding this comment.
SUGGESTION: Use specific target origin instead of * for postMessage
The frame.contentWindow?.postMessage({ type: "tabularis:init" }, "*", [port2]) uses * as target origin when transferring the message port. While the receiving end validates event.source === window.parent, using a specific origin (or at least the frame's known origin) would provide defense-in-depth. Since the frame uses a custom tabularis-plugin: protocol, consider using that origin or storing the frame's origin at connection time.
| frame.contentWindow?.postMessage({ type: "tabularis:init" }, "*", [port2]); | |
| frame.contentWindow?.postMessage({ type: "tabularis:init" }, frame.src, [port2]); |
| port.start(); | ||
| } | ||
|
|
||
| call(method: string, params: unknown[] = []): Promise<unknown> { |
There was a problem hiding this comment.
WARNING: RPC calls have no timeout — potential DoS if guest frame hangs
The call method creates a Promise that only resolves when the guest responds. If the guest frame crashes, hangs, or maliciously stops responding, the host will wait indefinitely. This could block UI interactions or leak memory from pending calls. Add a configurable timeout (e.g., 30s) to call that rejects with a timeout error and cleans up the pending entry.
| document.body.append(frame); | ||
|
|
||
| const controller = new AbortController(); | ||
| const timeout = window.setTimeout(() => controller.abort(), 15_000); |
There was a problem hiding this comment.
WARNING: 15-second hard timeout for parser module load may be too short
The setTimeout(() => controller.abort(), 15_000) gives only 15 seconds for the plugin frame to load, establish RPC, and return parser metadata. Large plugin bundles or slow machines could exceed this, causing false failures. Consider making this configurable or increasing to 30-60 seconds. The 50ms cancellation polling interval (line 69-71) is also aggressive and may impact performance; 100-200ms would suffice.
| ...(entry.label ?? descriptor.label | ||
| ? { label: entry.label ?? descriptor.label } | ||
| : {}), | ||
| parse: async (payload: string): Promise<ExplainPlan> => |
There was a problem hiding this comment.
CRITICAL: session.rpc.call in async parse/sniff functions — session may be closed
The parse and sniff functions (lines 178-185) capture session by reference and call session.rpc.call. If disposePluginExplainParsers is called concurrently (e.g., plugin disabled), session.close() is invoked which closes the RPC port and rejects pending calls. Subsequent parse/sniff calls will throw "Plugin frame closed" errors. The registry doesn't guard against this; callers of parseRawExplain or detectFormat will receive unhandled rejections. Add a check for session.rpc validity or handle "Plugin frame closed" errors gracefully in the proxy functions.
| use tauri::http::{header, Request, Response, StatusCode}; | ||
| use tauri::{AppHandle, Runtime}; | ||
|
|
||
| const GUEST_CSP: &str = "sandbox allow-scripts; default-src 'none'; script-src 'self'; \ |
There was a problem hiding this comment.
SUGGESTION: Consider adding trusted-types directive to guest CSP
The guest CSP (sandbox allow-scripts; default-src 'none'; script-src 'self'; ...) allows 'self' for scripts but doesn't restrict trusted-types. Adding trusted-types 'none' (or a specific policy name) would prevent DOM XSS via innerHTML, document.write, etc., providing defense-in-depth for the sandboxed frame. This is especially valuable since plugin bundles run in this context.
| function loadExplainModule(): Promise<Record<string, unknown>[]> { | ||
| if (explainModulePromise) return explainModulePromise; | ||
| explainModulePromise = new Promise((resolve, reject) => { | ||
| const pluginScript = document.createElement("script"); |
There was a problem hiding this comment.
WARNING: loadExplainModule trusts guestWindow.__tabularis_explain_parser__ without validation
The loadExplainModule function (lines 179-201) reads guestWindow.__tabularis_explain_parser__ set by the plugin bundle and filters for valid parser objects. However, a malicious or buggy plugin could set this to a prototype-polluted object or a getter that throws. The filtering (lines 188-195) checks typeof value === "object" and function properties, but doesn't guard against getters with side effects. Consider using Object.hasOwn and explicit property access to avoid prototype chain issues.
| setHeight(Math.min(MAX_HEIGHT, Math.max(0, requested))); | ||
| }, | ||
| "slot.context": () => structuredClone(contextRef.current), | ||
| "api.query": async (query: unknown) => { |
There was a problem hiding this comment.
SUGGESTION: Query length limit of 100,000 chars should be a named constant
The hardcoded 100_000 character limit for SQL queries (line 60) should be a named constant (e.g., MAX_PLUGIN_QUERY_LENGTH) for maintainability and to allow consistent limits across the codebase. This also makes it easier to adjust if needed.
| "api.setting.set": async (key: unknown, value: unknown) => { | ||
| if (typeof key !== "string" || !key) throw new Error("Setting key is required"); | ||
| const serialized = JSON.stringify(value); | ||
| if (typeof serialized !== "string" || serialized.length > 65_536) { |
There was a problem hiding this comment.
SUGGESTION: Settings size limit of 64 KiB should be a named constant
The 65_536 byte limit (line 86) for serialized plugin settings should be a named constant (e.g., MAX_PLUGIN_SETTING_SIZE) for clarity and consistency.
Code Review SummaryStatus: 20 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (incremental, since `6cc89d83`)
Fix these issues in Kilo Cloud Previous Review Summaries (6 snapshots, latest commit 6cc89d8)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 6cc89d8)Status: 21 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)CRITICAL
WARNING
SUGGESTION
Files Reviewed (incremental, since `81b540c8`)
Fix these issues in Kilo Cloud Previous review (commit 81b540c)Status: 21 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)CRITICAL
WARNING
SUGGESTION
Files Reviewed (incremental, since `ea41be7d`)
Fix these issues in Kilo Cloud Previous review (commit ea41be7)Status: 21 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)CRITICAL
WARNING
SUGGESTION
Files Reviewed (incremental, PR-relevant changes in `ea41be7d`)
Fix these issues in Kilo Cloud Previous review (commit 3e7329a)Status: 18 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)CRITICAL
WARNING
SUGGESTION
Files Reviewed (incremental, PR-relevant changes)
Fix these issues in Kilo Cloud Previous review (commit 04de575)Status: 17 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)CRITICAL
WARNING
SUGGESTION
Files Reviewed (17 files)
Fix these issues in Kilo Cloud Previous review (commit c8f8282)Status: 23 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)CRITICAL
WARNING
SUGGESTION
Files Reviewed (47 files)
[Snapshot truncated.] Additional previous summary content was truncated to keep this comment within platform limits. Reviewed by claude-sonnet-5.5 · Input: 282 · Output: 7.3K · Cached: 1.3M Review guidance: REVIEW.md from base branch |
debba
left a comment
debba
left a comment
There was a problem hiding this comment.
Hey @1-DARK, thanks a lot for this one. Moving UI extensions and EXPLAIN parsers out of the host context into sandboxed iframes, with a MessagePort and an explicit method allow-list, is something I really wanted to see. The guest and host source checks and the confined file reader in sandbox_protocol.rs look solid. I checked out the branch: typecheck, lint, the full vitest suite (5508 tests) and the Rust tests all pass.
Before merging, though, a few things need fixing:
- Slot contexts with callbacks never reach the guest.
connection-modal.connection_contentpassesonDatabaseChangeandconnection-modal.extra_fieldspassessetExtraField(NewConnectionModal.tsx). Functions can't be cloned throughpostMessage, sorpc.call("slot.context", ...)inSandboxedPluginFrame.tsx:125and:145throws DataCloneError. The error is swallowed and the guest keeps an empty context. In practice the Elasticsearch and Google Sheets connection fields render nothing (both checkcontext.driverfirst and then callonDatabaseChange), and the SQL Server extra fields break the same way. Those callbacks need to become host RPC methods, with only plain data sent in the context. - App CSP in
tauri.conf.json:29-30.index.htmlhas an inline<style>. Going by the Tauri source (set_csp/replace_csp_nonce), on release builds Tauri adds a nonce tostyle-src, and once a nonce is there browsers ignore'unsafe-inline', so the<style>tags Monaco injects at runtime would be blocked. Dev builds won't show this, and I haven't confirmed it on a release build yet. Either setdangerousDisableAssetCspModification: ["style-src"]or move that style out ofindex.html.img-srchas nohttps:, so registry and driver icons (raw.githubusercontent.com, codeberg) and the install-modal screenshots would be blocked.
- The read-only check isn't a security boundary.
isReadOnlyQuery(SandboxedPluginFrame.tsx:63) returns true forWITH d AS (DELETE FROM users RETURNING *) SELECT * FROM d,SELECT * INTO t2 FROM t,SELECT ... INTO OUTFILE,PRAGMA writable_schema = 1andSELECT pg_terminate_backend(...). Either enforce read-only at the connection level (a read-only transaction or session) or tone down the claim in the description and docs. - Scope. The
export_connections_payloadchange (secrets off by default plus the native confirm, and the newexport_connections_payload_unconfirmedfor backups) isn't in the description. I'd move it to its own PR, or at least document it here. - Google Sheets.
google-auth.jsdoes its OAuth token exchange withfetch, and the guest CSP (connect-src 'none') blocks that with no host alternative, so it can't migrate as things stand. We need a path for it before this lands.
Smaller things: cargo fmt reports diffs in your hunks of sandbox_protocol.rs, plugins/commands.rs and commands.rs; RPC calls have no timeout (a hung parser leaves EXPLAIN spinning forever); and a plugin file named runtime.js or host.css gets shadowed by the guest assets (sandbox_protocol.rs:99-102).
Once these are sorted I'll run my manual checks on a release build and we can get it in.
Preview buildThe preview build of commit |
|
Hey @debba, regarding the Google Sheets point: the plugin lives in a separate repository outside this PR’s workspace, so I couldn’t migrate it here. I added a host-mediated exchangeGoogleOAuthToken API so the guest can exchange OAuth tokens without making network requests, and documented the migration steps in plugins/PLUGIN_TUTORIAL.md. Would you like me to open a companion PR in the plugin repository, or will that migration be handled separately? |
|
@1-DARK yes feel free to create a companion PR in Google Sheets plugin if you want |
| }; | ||
| }, | ||
| openUrl: (url: string) => callHost("api.openUrl", url), | ||
| exchangeGoogleOAuthToken: (request: unknown) => callHostResult("api.oauth.googleToken", request), |
There was a problem hiding this comment.
WARNING: exchangeGoogleOAuthToken accepts request: unknown without validation
The handler takes request: unknown and passes it directly to the host via callHostResult. While the host validates, the guest runtime should validate the request structure (e.g., grant_type, required fields) before forwarding to fail fast with clear errors and avoid sending malformed requests.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
|
|
||
| function callHostResult<T>(method: string, ...args: unknown[]): Promise<T> { | ||
| if (!rpcPort) return Promise.reject(new Error("Plugin host connection is unavailable")); | ||
| return rpcPort.call(method, args) as Promise<T>; |
There was a problem hiding this comment.
WARNING: callHostResult uses unsafe as Promise<T> cast without runtime validation
The function returns rpcPort.call(method, args) as Promise<T> with a generic type assertion. If the host returns a different shape, callers get a false sense of type safety. Should either narrow the return type based on method or validate the response structure.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| const next = Array.isArray(args) ? args[0] : args; | ||
| if (next && typeof next === "object" && !Array.isArray(next)) { | ||
| currentContext = hydratePluginSlotContext(next as Record<string, unknown>, (callback, ...values) => { | ||
| void rpc.call(callback, values).catch(() => undefined); |
There was a problem hiding this comment.
WARNING: Callback passed to hydratePluginSlotContext silently swallows all errors
The callback (callback, ...values) => { void rpc.call(callback, values).catch(() => undefined); } catches and ignores all RPC failures. This hides communication errors between plugin and host, making debugging difficult and potentially leaving the plugin in an inconsistent state.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| grant_type: "authorization_code" | "refresh_token"; | ||
| code?: string; | ||
| client_id: string; | ||
| client_secret?: string; |
There was a problem hiding this comment.
WARNING: GoogleOAuthTokenRequest marks client_secret as optional but it's required for standard OAuth flows
For grant_type: "authorization_code" and grant_type: "refresh_token", the Google token endpoint requires client_secret unless PKCE (code_verifier) is used. The type should either make it required or use a discriminated union to enforce the correct field combinations.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| @@ -73,6 +73,9 @@ export type SlotContextMap = { | |||
| }; | |||
| "connection-modal.connection_content": { | |||
| driver: string; | |||
There was a problem hiding this comment.
SUGGESTION: Added new required fields to connection-modal.connection_content slot context
The slot context now requires database, connectionName, and onDatabaseChange. This is a breaking change for existing plugins using this slot — they will receive new fields but TypeScript consumers destructuring the context may need updates. Acceptable for 0.3.0 minor bump, but worth noting in migration docs.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| if let Some(ids) = &connection_ids { | ||
| let selected: std::collections::HashSet<&str> = | ||
| ids.iter().map(String::as_str).collect(); | ||
| let selected: std::collections::HashSet<&str> = ids.iter().map(String::as_str).collect(); |
There was a problem hiding this comment.
WARNING: Missing indentation in if block
Line 6794 (let selected: ...) is missing 4 spaces of indentation inside the if let Some(ids) = &connection_ids { block starting at line 6793. The surrounding lines (6795-6799) are properly indented, making this a clear formatting bug that would be caught by rustfmt.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| }; | ||
| }, | ||
| openUrl: (url: string) => callHost("api.openUrl", url), | ||
| exchangeGoogleOAuthToken: (request: unknown) => callHostResult("api.oauth.googleToken", request), |
There was a problem hiding this comment.
WARNING: exchangeGoogleOAuthToken accepts request: unknown without validation
The handler takes request: unknown and passes it directly to the host via callHostResult. While the host validates, the guest runtime should validate the request structure (e.g., grant_type, required fields) before forwarding to fail fast with clear errors and avoid sending malformed requests.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
|
|
||
| function callHostResult<T>(method: string, ...args: unknown[]): Promise<T> { | ||
| if (!rpcPort) return Promise.reject(new Error("Plugin host connection is unavailable")); | ||
| return rpcPort.call(method, args) as Promise<T>; |
There was a problem hiding this comment.
WARNING: callHostResult uses unsafe as Promise<T> cast without runtime validation
The function returns rpcPort.call(method, args) as Promise<T> with a generic type assertion. If the host returns a different shape, callers get a false sense of type safety. Should either narrow the return type based on method or validate the response structure.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| const next = Array.isArray(args) ? args[0] : args; | ||
| if (next && typeof next === "object" && !Array.isArray(next)) { | ||
| currentContext = hydratePluginSlotContext(next as Record<string, unknown>, (callback, ...values) => { | ||
| void rpc.call(callback, values).catch(() => undefined); |
There was a problem hiding this comment.
WARNING: Callback passed to hydratePluginSlotContext silently swallows all errors
The callback (callback, ...values) => { void rpc.call(callback, values).catch(() => undefined); } catches and ignores all RPC failures. This hides communication errors between plugin and host, making debugging difficult and potentially leaving the plugin in an inconsistent state.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| grant_type: "authorization_code" | "refresh_token"; | ||
| code?: string; | ||
| client_id: string; | ||
| client_secret?: string; |
There was a problem hiding this comment.
WARNING: GoogleOAuthTokenRequest marks client_secret as optional but it's required for standard OAuth flows
For grant_type: "authorization_code" and grant_type: "refresh_token", the Google token endpoint requires client_secret unless PKCE (code_verifier) is used. The type should either make it required or use a discriminated union to enforce the correct field combinations.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| @@ -73,6 +73,9 @@ export type SlotContextMap = { | |||
| }; | |||
| "connection-modal.connection_content": { | |||
| driver: string; | |||
There was a problem hiding this comment.
SUGGESTION: Added new required fields to connection-modal.connection_content slot context
The slot context now requires database, connectionName, and onDatabaseChange. This is a breaking change for existing plugins using this slot — they will receive new fields but TypeScript consumers destructuring the context may need updates. Acceptable for 0.3.0 minor bump, but worth noting in migration docs.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| if let Some(ids) = &connection_ids { | ||
| let selected: std::collections::HashSet<&str> = | ||
| ids.iter().map(String::as_str).collect(); | ||
| let selected: std::collections::HashSet<&str> = ids.iter().map(String::as_str).collect(); |
There was a problem hiding this comment.
WARNING: Missing indentation in if block
Line 6794 (let selected: ...) is missing 4 spaces of indentation inside the if let Some(ids) = &connection_ids { block starting at line 6793. The surrounding lines (6795-6799) are properly indented, making this a clear formatting bug that would be caught by rustfmt.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| "script-src": "'self'", | ||
| "worker-src": "'self' blob:", | ||
| "style-src": "'self' 'unsafe-inline'", | ||
| "img-src": "'self' data: blob: asset: http://asset.localhost https:", |
There was a problem hiding this comment.
WARNING: CSP img-src allows broad https: scheme
The img-src directive includes https: which permits loading images from any HTTPS origin. This weakens the CSP by allowing third-party image loads. Should be restricted to specific trusted domains or at minimum https://asset.localhost and https://raw.githubusercontent.com if those are the only sources.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| "style-src": "'self' 'unsafe-inline'", | ||
| "img-src": "'self' data: blob: asset: http://asset.localhost https:", | ||
| "font-src": "'self' data:", | ||
| "connect-src": "ipc: http://ipc.localhost https://raw.githubusercontent.com https://oauth2.googleapis.com", |
There was a problem hiding this comment.
WARNING: CSP connect-src allows full raw.githubusercontent.com domain
The connect-src directive allows connections to https://raw.githubusercontent.com (any path). While used for the plugin registry (compat.rs: registry.json), this permits arbitrary fetches from raw.githubusercontent.com. Should be restricted to the specific registry path if possible, or at least documented as a known broad permission.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| "base-uri": "'self'", | ||
| "form-action": "'none'" | ||
| }, | ||
| "dangerousDisableAssetCspModification": ["style-src"], |
There was a problem hiding this comment.
WARNING: dangerousDisableAssetCspModification disables nonce/hash injection for style-src
The dangerousDisableAssetCspModification: ["style-src"] opts out of Tauri's automatic CSP nonce/hash insertion for the asset protocol. This means inline styles loaded via asset: won't get nonces, relying solely on 'unsafe-inline' in style-src. This reduces CSP effectiveness for asset-protocol resources.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| "verbatimModuleSyntax": true, | ||
| "outDir": "dist", | ||
| "rootDir": "src" | ||
| "rootDir": "../.." |
There was a problem hiding this comment.
WARNING: rootDir points to repo root instead of package source directory
rootDir: "../.." (repo root) while include: ["src/**/*"] resolves to packages/create-plugin/src/**/*. This mismatch causes TypeScript to emit declaration paths like packages/create-plugin/src/... under dist/, which is likely unintended. Should be rootDir: "." or rootDir: "src".
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| getSetting: <T = unknown>(key: string, defaultValue?: T) => T; | ||
| setSetting: (key: string, value: unknown) => void; | ||
| setSettings: (updates: Record<string, unknown>) => void; | ||
| setSetting: (key: string, value: unknown) => Promise<void>; |
There was a problem hiding this comment.
SUGGESTION: Breaking change: setSetting/setSettings now return Promise<void> instead of void
The UsePluginSettingReturn interface changed setSetting and setSettings from synchronous void to async Promise<void>. This is a breaking API change for plugin authors. While documented in PLUGIN_GUIDE.md and PLUGIN_TUTORIAL.md, the version bump to 0.3.0 should be accompanied by a clear migration guide entry. The change is intentional but warrants explicit mention in release notes.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
|
Hey @debba, I’ve pushed the changes for all the points from your review.Whenever you have time, could you take another look? Thanks! |
debba
left a comment
debba
left a comment
There was a problem hiding this comment.
Thanks @1-DARK for the quick turnaround, and for the exchangeGoogleOAuthToken addition. I checked out the branch again and went through 5d3f4816 point by point (the later commits are merges of main).
A lot of it is sorted now:
- Slot callbacks: stripping functions out of the context and rebuilding them in the guest as stubs that call
slot.onDatabaseChange/slot.setExtraField/slot.setCredentialFieldsHiddenon the host is a clean approach. The argument checks on the host side plussandboxSlotContext.test.tsmake it easy to trust. - App CSP:
img-srcnow allowshttps:anddangerousDisableAssetCspModification: ["style-src"]is in place. - RPC timeout: 30s with a test, good.
- Asset shadowing: renaming to
__runtime__.js/__host.css__fixes it. - OAuth exchange: I looked at this one closely since it's a network call made on behalf of a plugin. The URL is hardcoded to Google's token endpoint, fields and
grant_typeare allow-listed, sizes are capped, and only known response fields go back to the frame. That looks right to me.
Typecheck, lint, the vitest suite and the sandbox_protocol Rust tests pass on my side, and CI is green.
What still needs work before I can merge:
-
Read-only enforcement. The commit message mentions it, but only the wording changed: the docs now say
isReadOnlyQueryis not a security boundary, whileapi.querystill checks the text and then calls the regularexecute_query(SandboxedPluginFrame.tsx:84-91). I ran the filter on this branch and then ran what it lets through against real databases:- SQLite:
PRAGMA user_version = 42passes the filter and writes to the database file. - PostgreSQL 16:
WITH d AS (DELETE FROM users RETURNING *) SELECT * FROM dpasses and empties the table;SELECT * INTO t2 FROM userspasses and createst2. pg_terminate_backend(...),lo_export(...)andINTO OUTFILEpass as well.
Since the whole point of the sandbox is that plugin code is untrusted, the host has to enforce this. We already do it for MCP, so you can reuse that path:
ai_activity::classify_query_kindto reject writes, thenexecute_read_only_queryfromdriver_trait.rs(mcp/mod.rs~1330 shows both together). Please use both, not just the read-only transaction. In my testsBEGIN READ ONLYstopped the CTEDELETEandSELECT INTO, butpg_terminate_backendstill ran, and SQLite'squery_onlystill allowedPRAGMA writable_schema = 1. The Rust classifier catches those. A small command that does both, called fromapi.query, would close the gap, and you can keepisReadOnlyQueryas a quick first check. - SQLite:
-
PR description. It still says "Write queries and statements the host can't classify as read-only are rejected" and still has the
TODO: link migration notesplaceholder. Theexport_connections_payloadchange is only mentioned inUI_SANDBOX_DESIGN.md. Please update the description so it covers the export hardening and links the migration notes inPLUGIN_TUTORIAL.md.
Minor, non-blocking:
- Google Sheets companion PR: the host side is there now, but the plugin itself stops working once this ships until it migrates to
exchangeGoogleOAuthToken. If you get the chance to open the companion PR in the Sheets plugin repo (a draft is fine), I can test both together and confirm the token exchange works from the webview. src-tauri/src/commands.rs:6794:let selectedis outdented inside theif letblock andcargo fmtflags it.api.oauth.googleTokenis callable by any sandboxed plugin, not just Sheets. It's harmless as written, but gating it on the plugin id (or a manifest permission) would keep the surface smaller.- The guest runtime uses the same 30s
RpcPorttimeout, so anapi.querythat runs longer than that fails in the plugin while the query keeps running on the host. Probably worth either a longer timeout for queries or a note in the docs. api.toastopens a native modal dialog on every call, so a misbehaving plugin can keep the user stuck clicking OK. Debouncing it or using a non-modal toast would be safer.- The guest modal in
runtime.ts:157hardcodes dark colors (#111827,#f9fafb), so it stays dark on light themes. The theme colors are already in the slot context, so it could use those.
Thanks again, this is very close. Once the query path goes through the database's read-only mode, I'll run my manual checks on a release build.
|
Hi @debba, I addressed the read-only enforcement feedback in commit 2b077ea and fixed a query-classification regression after the upstream merge in 04de575. I updated the PR description with the connection-export hardening and migration notes as well. The checks pass locally; PostgreSQL integration tests need a running PostgreSQL service. |
| "verbatimModuleSyntax": true, | ||
| "outDir": "dist", | ||
| "rootDir": "src" | ||
| "rootDir": "../.." |
There was a problem hiding this comment.
WARNING: rootDir points to repository root (../..) instead of package source directory
This causes TypeScript to include all repo files in compilation, slowing down builds and potentially causing conflicts. Should be "rootDir": "src" like before.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| grant_type: "authorization_code" | "refresh_token"; | ||
| code?: string; | ||
| client_id: string; | ||
| client_secret?: string; |
There was a problem hiding this comment.
WARNING: client_secret marked optional in GoogleOAuthTokenRequest but required for standard OAuth flows
The authorization_code grant type requires client_secret per OAuth 2.0 spec (RFC 6749). Making it optional allows invalid requests that will fail at Google's token endpoint. Consider making it conditionally required based on grant_type, or at minimum document that it's required for authorization_code.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| @@ -73,6 +73,9 @@ export type SlotContextMap = { | |||
| }; | |||
| "connection-modal.connection_content": { | |||
| driver: string; | |||
There was a problem hiding this comment.
WARNING: Breaking change: Added new required fields to connection-modal.connection_content slot context
Added database: string, connectionName: string, and onDatabaseChange: (value: string) => void as required fields. Existing plugins using this slot will break at runtime when these fields are missing. This is a breaking API change that should be documented and versioned.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| callback(hidden); | ||
| }, | ||
| "api.query": async (query: unknown) => { | ||
| if (typeof query !== "string" || query.trim().length === 0 || query.length > 100_000) { |
There was a problem hiding this comment.
SUGGESTION: Query length limit of 100,000 chars should be a named constant
Hardcoded 100_000 appears in both the host handler (line 82) and the Tauri command execute_plugin_read_only_query (commands.rs:4789). Extract to a shared constant (e.g., MAX_PLUGIN_QUERY_LENGTH) for consistency and maintainability.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| "api.setting.set": async (key: unknown, value: unknown) => { | ||
| if (typeof key !== "string" || !key) throw new Error("Setting key is required"); | ||
| const serialized = JSON.stringify(value); | ||
| if (typeof serialized !== "string" || serialized.length > 65_536) { |
There was a problem hiding this comment.
SUGGESTION: Settings size limit of 64 KiB should be a named constant
Hardcoded 65_536 (64 KiB) for serialized settings size. Extract to a named constant (e.g., MAX_PLUGIN_SETTING_SIZE) for clarity and easier adjustment.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| }, | ||
| }; | ||
|
|
||
| void connectFrame(frame, handlers, controller.signal).then(async (rpc) => { |
There was a problem hiding this comment.
WARNING: Race condition between connection cleanup and promise resolution
connectFrame promise resolves, then checks disposed flag. If cleanup runs between resolution and the check, rpc.close() is called but rpcRef.current is still set. The disposed check should be atomic with the RPC assignment, or use a separate initialization state.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| }); | ||
| // Sandboxed frames have an opaque origin; the source window is checked above | ||
| // and the transferred port is unique to this frame instance. | ||
| frame.contentWindow?.postMessage({ type: "tabularis:init" }, "*", [port2]); |
There was a problem hiding this comment.
SUGGESTION: Use specific target origin instead of * for postMessage
frame.contentWindow?.postMessage({ type: "tabularis:init" }, "*", [port2]) uses wildcard target origin. Since the frame URL is known (tabularis-plugin://...), use the exact origin or 'self' equivalent to prevent message leakage if the frame is navigated.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| document.body.append(frame); | ||
|
|
||
| const controller = new AbortController(); | ||
| const timeout = window.setTimeout(() => controller.abort(), 15_000); |
There was a problem hiding this comment.
WARNING: 15-second hard timeout for parser module load may be too short
window.setTimeout(() => controller.abort(), 15_000) gives only 15s for the hidden iframe to load, establish RPC, and return parser metadata. On slow machines or large parser bundles, this can cause false failures. Consider making it configurable or increasing to 30s.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| ...(entry.label ?? descriptor.label | ||
| ? { label: entry.label ?? descriptor.label } | ||
| : {}), | ||
| parse: async (payload: string): Promise<ExplainPlan> => |
There was a problem hiding this comment.
CRITICAL: session.rpc.call in async parse/sniff functions — session may be closed
The registered parse and sniff functions (lines 178-185) capture session by reference. If another plugin registers the same format, replacedSession.close() is called (line 192), which closes the RPC port. Subsequent parse/sniff calls on the old session will fail with "Plugin frame closed". Use a session reference that checks validity at call time, or clone the RPC port.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
debba
left a comment
debba
left a comment
There was a problem hiding this comment.
Thanks @1-DARK, this round closes the main gap. The new execute_plugin_read_only_query command is what I was hoping for: the Rust classifier runs first, then the driver's read-only path, and anything other than SQLite/PostgreSQL/MySQL fails closed. Nice touch adding the classifier test with all the payloads from my last review. I also like that the smaller points went into the same push: OAuth gated to google-sheets, the 5 minute query timeout, toasts instead of native dialogs, and the guest modal following the theme. The updated description reads well now.
Typecheck, lint, vitest (5862 tests) and cargo test --lib all pass on the branch, and CI is green.
Two things before I can merge:
- Current main breaks 7 tests. #1078 landed after your last merge from main and added
tests/fixtures/pluginSlotProbe.tsx, which loads plugins the old way (read_plugin_filereturning a__tabularis_plugin__bundle rendered in the host tree). Git merges cleanly, but when I merged main locally, the plugin-context tests inRowEditorPanel.test.tsx(4) andTableToolbar.test.tsx(3) failed. The slots now renderSandboxedPluginFrame, which throwsi18n.getResourceBundle is not a functionin that setup. Not your fault, just timing, but could you merge main again and adapt the fixture and those tests to the iframe model? Otherwise main goes red on merge. api.querynow caps at 100 rows.execute_plugin_read_only_querypassesSome(100)(commands.rs:4810). Before this PR,usePluginQuerywent throughexecute_querywith no limit. The guest only getscolumns/rows, so a plugin silently sees the first 100 rows without knowing it. I'd passNone(or a much higher cap) and mention it in the docs, or returntruncatedto the plugin.
Minor, non-blocking: the diff has quite a few rustfmt-only hunks in the drivers and ai_activity.rs. They're fine (that formatting debt was already on main), just a bit noisy to review. And whenever you have time, the Sheets companion PR is still welcome, so I can test the token exchange end to end.
Once these two are in, I'll do my manual checks on a release build and we can get it merged.
…ts' into feat/sandbox-plugin-js-csp-secrets
debba
left a comment
debba
left a comment
There was a problem hiding this comment.
Thanks @1-DARK, the slot tests and the row cap are sorted in 8ca6c6e7.
I can't merge this yet though, because it breaks existing plugins that aren't part of this repo. Google Sheets is the clearest case: once this ships, its OAuth flow stops working until the plugin migrates to exchangeGoogleOAuthToken, and other plugins that load UI extensions the old way are affected the same way. Users who update Tabularis would end up with broken plugins and no clear path forward.
Before this can land I need the Sheets companion PR (a draft is fine) so I can test both together, plus a check of the other published plugins that use UI slots, with either a migration for each or a compatibility path so they keep working in the meantime.
| match params.driver.as_str() { | ||
| "sqlite" | "postgres" | "mysql" | "mariadb" => { | ||
| driver | ||
| .execute_read_only_query(¶ms, &query, None, 1, schema.as_deref()) |
There was a problem hiding this comment.
WARNING: Plugin read-only queries are now unbounded
Changing the row limit from Some(100) to None removes the only host-side cap on rows returned to an untrusted sandboxed plugin. A plugin (or a buggy one) can run SELECT * FROM big_table, forcing the host to materialize the full result set in memory, serialize it over Tauri IPC, and then push it through the MessagePort RPC into the iframe. This can freeze or OOM the app. Consider a generous but finite cap (e.g. a named constant like PLUGIN_QUERY_MAX_ROWS) and/or a size limit on the serialized result, and document the truncation so plugins can paginate.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
debba
left a comment
debba
left a comment
There was a problem hiding this comment.
Thanks @1-DARK for sticking with this one, and for going through the published plugins one by one in UI_SANDBOX_DESIGN.md. That audit was what I was after, and I appreciate you opening the Sheets companion PR right away. The defaultValue fallback for Cloudflare D1 is a nice catch too.
I went through the plugins myself to double check. I loaded the released bundles of every plugin in the registry that has UI extensions or EXPLAIN parsers into a page that mirrors SandboxedPluginFrame, with the same guest CSP, sandbox flags and RPC. The good news: DynamoDB, Elasticsearch and SQL Server (extra field plus credential toggle) work as they are, and the Oracle, SQL Server and BigQuery EXPLAIN parsers load fine. On your branch, typecheck, lint, vitest (6375 tests) and the Rust sandbox and classifier tests all pass.
A few things still keep me from merging:
- The Sheets companion fails.
exchangeCodecallsgetHost(), but@tabularis/plugin-apiis mapped to__TABULARIS_API__and the guest API has nogetHost, so the wizard stops withgetHost is not a functionand the host is never called. Either exposegetHostin the guest API or importexchangeGoogleOAuthTokendirectly, since the guest already provides it. Also,min_runtime_versionshould be the release that ships this PR. 0.28.0 is already out and doesn't have the API. - Plugin modals get clipped.
usePluginModalrenders aposition: fixedoverlay inside the iframe, but the iframe height comes from the body's ResizeObserver, which a fixed overlay doesn't change. The Sheets "Connect Google Account" wizard ends up squeezed into a ~150px frame where you only see the title. The companion still uses the modal, so this needs a host-side fix, for example a guest call that lets the frame grow into a full overlay while a modal is open. - BigQuery is missing from the audit and breaks once you merge main. #629 added
setSecretField/secretFieldstoconnection-modal.extra_fields, and BigQuery 0.2.0 relies on them while hiding the Password field.serializePluginSlotContextonly forwardssetExtraField, so the credential input never shows up and only ADC works. Please addsetSecretFieldto the forwarded callbacks with the same kind of validation as the others. - The branch conflicts with main.
ai_activity.rsnow goes throughclassify_query_kind_for_dialect, so the extra side-effect functions and tests need to be carried over onto that.PLUGIN_GUIDE.mdconflicts as well. Also please drop the manual plugin-api bump to 0.3.0: release-please already published 0.3.0 withoutexchangeGoogleOAuthToken, so that version number is taken. - Users on Sheets 0.3.0. After updating Tabularis they can't connect a Google account until the plugin updates. Please make sure the companion is ready to release first, and ideally show a hint in the host when an old Sheets bundle fails to load its OAuth flow.
Smaller things, fine as follow-ups if you prefer:
src-tauri/resources/guest/runtime.jsis out of date withruntime.ts(thedefaultValuechange isn't in it). Release builds regenerate it, but cargo builds and tests embed the committed file. I'd rather not commit it, or at least check it in CI.- The frame doesn't inherit the host text colour, so the Cloudflare D1 help box renders black text on dark themes. Setting
colorand the theme variables in the guest from the theme you already pass would fix it. - The CSP doesn't stop the frame from navigating itself to an external URL. Not a big deal since plugin binaries have network access anyway, but I'd soften the "blocks network access" wording or block that navigation too.
Once these are sorted I'll try the whole thing again on a release build together with the Sheets companion.
|
Adding the screenshots from the harness I mentioned in the review (released plugin bundles loaded into a page that mirrors Google Sheets 0.3.0, "Connect Google Account" wizard. The
Cloudflare D1 help box. It works, but the frame doesn't inherit the host text colour, so the labels and help text come out dark on a dark theme:
|
| const handlers = { | ||
| "ui.modal": (open: unknown) => { | ||
| if (typeof open !== "boolean") throw new Error("Modal state must be a boolean"); | ||
| setModalOpen(open); |
There was a problem hiding this comment.
WARNING: Untrusted guest can take over the whole host window via ui.modal
ui.modal is an unauthenticated, guest-controlled RPC. When open === true, the iframe style (lines 269-278) becomes position: fixed; inset: 0; width: 100vw; height: 100dvh; z-index: 2147483647, covering the entire Tabularis window and intercepting all pointer/keyboard input. A plugin (including a malicious or buggy one) can call ui.modal(true) at any time without ever rendering a dialog, which blocks the host UI (including the means to disable/uninstall the plugin) and enables convincing UI spoofing of host dialogs. The host should only honour ui.modal(true) after a user gesture, and should auto-reset it (e.g. on unmount of the slot, or after a timeout) and give the user a host-rendered way to dismiss.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| aria-hidden="true" | ||
| style={{ width: "100%", height, pointerEvents: "none" }} | ||
| /> | ||
| {createPortal(iframe, document.body)} |
There was a problem hiding this comment.
WARNING: Portaled position: fixed iframe is not clipped or reliably tracked
The iframe is rendered into document.body with position: fixed at a rect measured from an anchor div, and the measurement effect (line 55-72) only re-runs on [height], window resize, and capture-phase scroll. The anchor can move without any of these events (sibling layout changes, sidebar/panel resize, tab switches, a host modal opening/closing, CSS transitions), leaving the iframe visually detached from its slot. Also, because it is outside the slot's DOM subtree it ignores overflow clipping of its scroll containers (e.g. the connection modal body), so the plugin UI can paint over the modal header/footer or outside the modal, and it sits at zIndex: 1, which may fall below or above host overlays unpredictably. A ResizeObserver on the anchor does not detect position-only changes; consider observing ancestors / using IntersectionObserver or a per-frame requestAnimationFrame check while visible, and clip to the nearest scrollable ancestor.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| return rpcPort.call(method, args) as Promise<T>; | ||
| } | ||
|
|
||
| window.addEventListener("securitypolicyviolation", (event) => { |
There was a problem hiding this comment.
SUGGESTION: Plugin-specific hard-coded hack in the generic guest runtime
The securitypolicyviolation listener special-cases pluginId === "google-sheets" and https://oauth2.googleapis.com, and the host mirrors it with api.compatibility.legacyGoogleOAuth. Keeping third-party plugin identifiers in the generic sandbox runtime (and in the host RPC allow-list) couples the platform to one plugin and is easy to forget to remove. Consider a documented, time-boxed removal (TODO with target version) or a generic mechanism (e.g. a manifest-declared legacy shim) instead. Note also blockedURI is only reported for same-origin-visible URLs; if it is "inline"/"" the toast will silently never fire.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
…sktop transport, fix create-plugin smoke
Summary
Plugin UI extensions and custom EXPLAIN parsers previously ran in Tabularis’s JavaScript context. This PR moves them into sandboxed iframes and limits communication with the host to an asynchronous, allow-listed API.
Closes #973
Changes
Security
The guest frame receives no database credentials, filesystem access, direct Tauri commands, or general-purpose backend proxy. Its CSP blocks network access. The host validates all bridge requests and only handles explicitly allowed operations.
Breaking change and migration
Plugins that use host globals, the host React tree, or network requests from UI code must migrate to the sandbox API. See the plugin tutorial, plugin API guide, and sandbox design notes.
Testing
pnpm typecheckpnpm lint