Skip to content

feat: add datatable read and write MCP tools#10001

Open
rubenfiszel wants to merge 4 commits into
mainfrom
ruben/win-2143-add-datatable-read-and-write-as-mcp-operations
Open

feat: add datatable read and write MCP tools#10001
rubenfiszel wants to merge 4 commits into
mainfrom
ruben/win-2143-add-datatable-read-and-write-as-mcp-operations

Conversation

@rubenfiszel

@rubenfiszel rubenfiszel commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Summary

Fixes WIN-2143.

Exposes Windmill data tables as MCP operations. Data table access is a dedicated, opt-in capability in the MCP URL generator — off by default in every mode — with a read/write sub-toggle and an optional restriction to specific data tables. When enabled, the client gets tools to list tables, read rows (with WHERE conditions), and (if write is on) insert/update rows.

New/existing REST endpoints are marked x-mcp-tool: true in the OpenAPI spec and auto-generated into the Rust tool registry + the frontend endpoint list; the datatable tools are then gated by a dedicated mcp:datatables: scope rather than the generic endpoint scopes.

Tools

Read: listDataTables, listDataTableTables, getDataTableTableSchema (existing endpoints, now MCP-exposed), and new queryDataTable (SELECT with where_clause/select/order_by/limit/offset, capped at 1000).
Write: new insertDataTable (row insert; omitted columns keep their default) and updateDataTable (update rows matching a required where_clause).

Data tables access control

A dedicated "Data tables" section (shown in every mode except "All", where mcp:all already grants everything):

  • Master toggle — off by default. Datatable tools are never exposed unless this is on, independent of the favorites/all/folder/endpoint scopes.
  • Write access sub-toggle — off by default (read-only); hidden when the token is read-only. On ⇒ also exposes insert/update.
  • Restrict to specific data tables — optional multiselect; empty = all data tables.
  • A warning that this grants full access to the data (no per-user/row-level permissions on data tables yet — coming soon — so it's meant for trusted/dev use).

Scope grammar: mcp:datatables:read:<names|*> / mcp:datatables:write:<names|*>. Absence ⇒ no datatable access. mcp:all grants read+write on all data tables.

Enforcement:

  • Tool exposure: gated in the MCP runner by datatable_tool_allowed(read|write) — datatable tools are opt-in regardless of mode; write tools also require a non-read-only token (they're POST).
  • Per-datatable name + list filtering: in WindmillBackend::call_endpoint (the only place with the original MCP scopes — the JWT the runner mints for the proxied HTTP call is route-scoped and no longer carries mcp: scopes). Per-call datatable_name is rejected if out of scope; listDataTables/listDataTableTables responses are filtered so a restricted token never sees other names.

Changes

  • backend/windmill-api-workspaces/src/workspaces.rs: query_datatable/insert_datatable/update_datatable handlers + connect_datatable/quote_pg_ident/check_datatable_schema helpers and routes.
  • backend/windmill-api/openapi.yaml: six datatable endpoints marked x-mcp-tool; regenerated auto_generated_endpoints.rs + mcpEndpointTools.ts.
  • backend/windmill-mcp/src/common/scope.rs: datatables_read/datatables_write/datatables on McpScopeConfig; parse mcp:datatables:read|write:; datatable_access_level, datatable_tool_allowed, is_datatable_allowed; OAuth subset (contains); tests.
  • backend/windmill-mcp/src/server/runner.rs: gate datatable tools by the dedicated scope in list_tools + call_tool.
  • backend/windmill-api/src/mcp/core.rs: per-datatable arg check + list-response filtering in call_endpoint.
  • backend/windmill-api/src/mcp/oauth_server.rs: advertise mcp:datatables:read:*/write:*.
  • frontend/src/lib/components/mcp/McpScopeSelector.svelte: dedicated Data tables section (master + write toggle + restriction picker + warning); datatable tools excluded from the generic endpoint picker.
  • tokio-postgres dep on windmill-api-workspaces; unit tests for quote_pg_ident and the datatable scope logic.

Safety

  • Identifiers (schema/table/column/select) always quoted via quote_pg_ident (doubles ", rejects empty/NUL).
  • Values bound as one $1::jsonb param via jsonb_populate_record — Postgres coerces types, no value interpolation.
  • where_clause/order_by are validated with sqlparser and re-serialized from the parsed AST before interpolation: a fragment must be a single self-contained predicate / ORDER-BY list with no set operations, no subqueries, and no trailing tokens. This blocks the UNION/subquery-closing breakout (which would otherwise bypass the system-schema guard and LIMIT) and the error-based/IN (SELECT …) cross-table exfiltration. Complex boolean predicates and multi-column ordering still work.
  • Residual trust boundary: a permitted expression can still call any function the datatable's connection role is granted (pg_sleep, or pg_read_file if that role is superuser) — scope datatable roles accordingly (not superuser).
  • Datatable access requires the explicit mcp:datatables: scope; write additionally requires a non-read-only token, and the REST write endpoints reject operators (require_datatable_writer).

Design decisions

  • Datatable access is off by default and orthogonal to scripts/flows/endpoints — only the dedicated toggle (or mcp:all) grants it.
  • Data tables are PostgreSQL only (DuckLake is a separate substrate); non-postgres fails at connect with a clear error.

Test plan

  • cargo check (windmill-api, windmill-api-workspaces); cargo test -p windmill-mcp scope:: (9 tests) and -p windmill-api-workspaces (quote_pg_ident + where_clause/order_by sanitizer incl. the reported UNION-injection payload)
  • Verified the exact handler SQL against a real Postgres via psql (type coercion, serial default, to_jsonb, per-column update, quoted identifiers)
  • npm run check:fast; svelte-autofixer
  • Browser: "Data tables" toggle is off by default in Favorites mode; enabling reveals the warning + Write toggle + restriction picker; datatable tools are absent from the generic API Endpoints list; generating with write + main produces token scope mcp:favorites mcp:datatables:write:main (verified in the DB)
  • End-to-end MCP client call against a live data table, incl. a read-only token rejected on write and a restricted token rejected on an out-of-scope data table

Screenshots

Dedicated "Data tables" section — opt-in master toggle, write sub-toggle, restriction picker, and access warning:
mcp-datatable-toggle

MCP URL generator — datatable read tools shown (read-only token hides insert/update):
mcp-datatable-readonly

🤖 Generated with Claude Code

@cloudflare-workers-and-pages

cloudflare-workers-and-pages Bot commented Jul 8, 2026

Copy link
Copy Markdown

Deploying windmill with  Cloudflare Pages  Cloudflare Pages

Latest commit: d2a682a
Status: ✅  Deploy successful!
Preview URL: https://9dd51ba6.windmill.pages.dev
Branch Preview URL: https://ruben-win-2143-add-datatable.windmill.pages.dev

View logs

@rubenfiszel rubenfiszel marked this pull request as ready for review July 8, 2026 10:42
@claude

claude Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Claude finished @rubenfiszel's task in 4m 4s —— View job


Mergeable, but should ideally address nits: write endpoints (insert_datatable/update_datatable) reachable by any workspace member via the _unchecked resolver — confirm intended authz level; the raw where_clause/order_by DoS/read surface is worth an explicit note.

cc @rubenfiszel

Review

Solid PR. The SQL construction is careful — identifiers always go through quote_pg_ident (doubles ", rejects empty/NUL), values are bound through a single $1::jsonb + jsonb_populate_record so Postgres does the coercion, and client.query/execute use the extended protocol so a ;-chained second statement is a syntax error rather than a second command. The scope model (opt-in mcp:datatables:, contains monotonicity, list-response filtering) is well tested. I verified the list-filter keys match the actual response shapes:

  • listDataTablesVec<DataTableListItem> with name
  • listDataTableTablesVec<DataTableTables> with datatable_name
  • list_datatable_schemasVec<DataTableSchema> with datatable_name

All are top-level arrays, so the Value::Array filter never silently fails open, and missing-key items are dropped (unwrap_or(false) = fail-closed). Good.

Findings

P2 — write endpoints are member-level, unlike the rest of the datatable admin surface (workspaces.rs:2010, 2065)
insert_datatable/update_datatable take _authed: ApiAuthed (ignored) and resolve credentials via get_datatable_resource_from_db_unchecked, which bypasses per-resource ACLs. By contrast edit_datatable_config (workspaces.rs:2690) does require_admin(...). So any workspace member — including an operator, and including a member who has no direct grant on the underlying DB resource — can now write arbitrary rows to a data table over REST (not just via a scoped MCP token). Reads (get_datatable_*) already used this _unchecked model, so this isn't a brand-new gap, but write is a meaningful widening. Please confirm member-level write is the intended authorization tier (data tables being a shared workspace store, it may well be), or add an operator/write-role gate. Fix this →

P2 — raw where_clause/order_by is a DoS / broad-read surface for an AI-facing tool (workspaces.rs:1975, 2085)
where_clause and order_by are interpolated verbatim into the statement. Statement-chaining is correctly blocked, and the datatable-level scope is the only intended access control (there is no table-level scope), so reading other tables in the same Postgres via a subquery is within the granted scope — not a bypass. The residual risk is that these fragments can call any function the datatable's connection role permits: pg_sleep(...) for a cheap DoS, and pg_read_file/lo_import etc. if that role happens to be superuser. Since the fragments originate from an MCP/AI client (potentially prompt-injected), it's worth an explicit line in the x-mcp-instructions / PR safety notes that the connection role's privileges are the trust boundary here. Not a blocker — consistent with the existing DB-manager idiom.

AGENTS.md compliance

  • OpenAPI x-mcp-tool markers regenerated into both auto_generated_endpoints.rs and mcpEndpointTools.ts per the intended extension path ✅
  • Comments record constraints (why raw fragments are safe, why the per-column jsonb_populate_record avoids a FROM-join ambiguity) rather than narrating — matches the "comments record constraints" rule ✅
  • No customer attribution; randomUUID rule N/A ✅

Test coverage

  • Backend: Good unit coverage for the pure logic — quote_pg_ident (escape/empty/NUL) and the scope parse/allow/contains matrix (9 tests). The handler SQL itself has no automated integration test; the PR notes it was verified manually against real Postgres via psql. Given these are new DB-writing handlers, an integration test (spin up a data table, insert→query→update round-trip, plus a restricted-token rejection) would be the ideal follow-up, but the manual verification is reasonable for now.
  • Frontend: McpScopeSelector.svelte — no component tests expected per repo convention; the encode/decode of mcp:datatables: is simple string logic exercised via the screenshots.
  • Still needs manual E2E (already flagged unchecked in the PR's own test plan): call the tools from a live MCP client, including a restricted token being rejected on an out-of-scope data table, and confirm the read-only toggle actually hides insertDataTable/updateDataTable at runtime.
    · branch

@github-actions

github-actions Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

I've now reviewed the full diff across all 10 changed files. Here's my assessment.


Pi Review

Good to merge.

The implementation is thorough and well-considered. The three new handlers (query_datatable, insert_datatable, update_datatable) properly quote identifiers via quote_pg_ident (with double-quote escaping and NUL/empty rejection), bind values through jsonb_populate_record parameterization, perform system-schema guards, validate required fields, and cap row limits. The per-datatable scope enforcement in McpBackend::call_endpoint runs before the proxied HTTP request (where the original MCP scopes are still available) and the list-response filtering afterwards correctly hides names a restricted token shouldn't see. Scope parsing and the contains containment check both handle the opt-in semantics and edge cases correctly, with unit tests covering them.

The frontend scope selector change integrates cleanly: it lazily loads data table names in custom mode, decodes/encodes the mcp:datatables: scope, and reuses existing patterns for select-all/clear and the multiselect component.

No P0, P1, or P2 findings.

Test coverage

  • Backend: quote_pg_ident has two unit tests (wrapping/escaping and NUL/empty rejection). scope.rs has two new test functions (test_datatable_scope_parsing_and_allow, test_contains_datatables) covering parse, allow, and containment for the datatable restriction. The existing test_contains_subset_and_widening still passes — no regression. No integration tests for the new handlers themselves, but they follow the same tokio_postgres connection idiom as the existing get_datatable_table_schema handler.
  • Frontend: No pure-logic utility files were added; the scope selector is a Svelte component (no component tests expected per policy). The auto-generated mcpEndpointTools.ts is generated output, not hand-written logic.
  • Auto-generated files: auto_generated_endpoints.rs and mcpEndpointTools.ts are generated from the OpenAPI spec — no test expectations for these.

Manual verification still needed

The end-to-end item from the PR's own test plan remains unchecked: calling the tools from an MCP client against a live data table, including a restricted token being rejected on an out-of-scope data table. This should be done before merging, as it exercises the actual MCP stack with a real Postgres backend, confirming that the per-datatable enforcement, list filtering, and the read-only toggle all work end-to-end from an MCP client's perspective.

@github-actions

github-actions Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Codex Review

cc @rubenfiszel
Should address issues before merging: SQL injection in query_datatable

Found 1 issue:

  1. [P0] SQL injection in raw datatable query fragments
    backend/windmill-api-workspaces/src/workspaces.rs:1997

    where_clause and order_by are interpolated into the inner SELECT before the wrapper is closed. A caller can close that subquery and append a UNION in the same prepared statement, so the “single statement” defense does not preserve the intended query shape. For example, a where_clause shaped like false) _wm_row UNION ALL SELECT to_jsonb(s) AS row FROM pg_catalog.pg_tables s -- can bypass the non-system schema check and the LIMIT/OFFSET cap while still being one statement. This is especially risky because queryDataTable is a GET MCP tool and therefore available to read-only MCP tokens. The raw fragments need to be parsed/validated as predicates/order terms, built with a query builder, or removed in favor of structured filters.

Test coverage

Backend: the PR adds unit coverage for identifier quoting and MCP datatable scope parsing, but there is no test that rejects SQL breakout through where_clause/order_by. Add a regression test that proves a crafted fragment cannot escape the intended SELECT ... FROM <quoted schema.table> ... LIMIT/OFFSET shape.

Frontend: no component tests expected for this selector change.

Manual verification still needed: call queryDataTable through an MCP read-only token with a normal predicate and confirm it returns only the selected table rows, then call it with a subquery-closing/UNION payload and confirm it is rejected rather than returning rows from another schema/table.

@github-actions

github-actions Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Now I have all the information needed for the review. Let me write the complete review.

Pi Review

Should address issues before merging: SQL injection via UNION in query_datatable where_clause/order_by; error-based data exfiltration in update_datatable where_clause

cc @rubenfiszel

P0 — SQL injection via UNION in query_datatable raw where_clause and order_by (backend/windmill-api-workspaces/src/workspaces.rs, lines ~1993–2000)

The where_clause and order_by are interpolated verbatim into the inner SELECT before the outer SELECT to_jsonb(...) FROM ({inner}) _wm_row wrapper. An attacker can close the FROM subquery and inject a UNION ALL — all within a single statement, so the extended query protocol's ;-rejection does not help:

Payload for where_clause:

false) _wm_row UNION ALL SELECT to_jsonb(s) AS row FROM pg_catalog.pg_tables s --

Produces:

SELECT to_jsonb(_wm_row) AS row FROM (
  SELECT * FROM "s"."t" WHERE false) _wm_row
UNION ALL
  SELECT to_jsonb(s) AS row FROM pg_catalog.pg_tables s
-- LIMIT 100 OFFSET 0) _wm_row

The -- comments out the trailing LIMIT, OFFSET, and closing ) _wm_row, and the UNION ALL is parsed at the outer SELECT level. The attacker can read any table the connecting role has access to, bypassing the is_system_pg_schema guard and the LIMIT cap entirely. order_by is similarly injectable (same ) + UNION ALL + -- technique, just positioned after ORDER BY instead of WHERE).

This is a read-only (GET) MCP tool available to read-only tokens, making it the most exposed attack surface.

Fix direction: the raw fragments need to be parsed and validated as pure SQL expressions before interpolation, built with a query builder, or removed in favor of structured filter parameters. At minimum, disallow ), UNION, and -- / /* comment sequences in these fields.

P1 — Error-based data exfiltration in update_datatable where_clause (backend/windmill-api-workspaces/src/workspaces.rs, line ~2115)

The raw where_clause in update_datatable is interpolated into:

format!("UPDATE {qualified} SET {set_clause} WHERE {}", req.where_clause)

While ;-chained multi-statement injection is blocked by the extended query protocol, an attacker can craft a WHERE condition that triggers a CAST type error embedding data from another table:

true AND 1 = CAST((SELECT password FROM users LIMIT 1) AS INTEGER)

PostgreSQL's error message includes the value that failed to cast — the error is surfaced in the response via Error::BadRequest(format!("Datatable update failed: {}", e)). This leaks data from any table the connecting role can read, bypassing the system-schema guard and the datatable-level scope.

Additional notes

  • The insert_datatable handler is clean — all identifiers go through quote_pg_ident and values are parameterized via $1::jsonb.
  • The listDataTableSchemas endpoint (existing, now MCP-exposed) is marked with x-mcp-tool: true in the OpenAPI spec but I don't see a listDataTableSchemas entry in auto_generated_endpoints.rs or mcpEndpointTools.ts — it's listed in datatable_access_level and datatable_list_filter_key though. This may be intentional if the endpoint's OpenAPI path didn't match the generation pattern, but worth a spot check.
  • The scope enforcement (call_endpoint, list_tools, call_tool) and scope parsing/containment tests are thorough and correct. The datatable list-response filtering (listDataTablesname, listDataTableTables/listDataTableSchemasdatatable_name) is fail-closed (unwrap_or(false) drops items missing the key) — correct.
  • The frontend scope selector integration (lazy loading datatable names, encode/decode of mcp:datatables:, read-only/write sub-toggle) follows existing patterns and correctly excludes datatable tools from the generic endpoint picker.

Test coverage

  • Backend: unit tests for quote_pg_ident (wrapping/escaping, NUL/empty rejection) and datatable scope parsing/allow/containment (9 tests) are present and correct. Missing: a regression test that proves a crafted where_clause cannot escape the intended SELECT ... FROM <quoted schema.table> shape via UNION injection. Add at least one test that sends false) _wm_row UNION ALL SELECT ... -- and asserts the request returns an error (after fixing the handler to reject it).
  • Frontend: no component tests expected per policy. The auto-generated mcpEndpointTools.ts is generated output.
  • No integration tests for the new handlers themselves, but they follow the same tokio_postgres idiom as existing handlers.

Manual verification still needed

The end-to-end item from the PR's own test plan (calling tools from an MCP client against a live data table with restricted token enforcement) should be completed. Additionally, after fixing the injection vectors, verify that: (a) a normal where_clause like status = 'active' in queryDataTable still returns correct filtered rows; (b) a payload containing ), UNION, or -- is rejected with a clear error; and (c) the updateDataTable where_clause rejects the same injection tokens.

@github-actions

github-actions Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Codex Review

cc @rubenfiszel
Should address issues before merging: SQL injection in query_datatable

Found 1 issue:

  1. [P0] backend/windmill-api-workspaces/src/workspaces.rs:1997 — SQL injection in query_datatable

    where_clause and order_by are interpolated into the inner SELECT before it is wrapped as FROM ({inner}) _wm_row. A caller can close that derived table and append a same-statement UNION, so the “single prepared statement” defense does not preserve the intended query shape or the LIMIT/OFFSET cap. For example, a crafted where_clause can return rows from pg_catalog or another table visible to the datatable connection role while still using a read-only MCP token. These fragments need to be parsed/validated as predicates/order terms, replaced with structured filters, or otherwise prevented from closing the intended query.

Test coverage

Backend: the PR adds unit coverage for identifier quoting and datatable scope parsing, but there is no regression test proving where_clause/order_by cannot escape the intended SELECT ... FROM <quoted schema.table> ... LIMIT/OFFSET shape.

Frontend: no component tests expected for the Svelte selector change.

Manual verification still needed: call queryDataTable through a read-only MCP token with a normal predicate and confirm it returns only selected table rows, then call it with a subquery-closing UNION payload and confirm it is rejected rather than returning rows from another schema/table.

@rubenfiszel

Copy link
Copy Markdown
Contributor Author

Addressed the review findings in d2a682a:

P0 — SQL injection via UNION/subquery-closing in query_datatable where_clause/order_by (and the P1 error-based exfiltration in update_datatable): fixed. where_clause and order_by are no longer interpolated raw. Each fragment is now parsed with sqlparser (PostgreSQL dialect) and rejected unless it is a single, self-contained expression / ORDER-BY list:

  • where_clause must parse as one Expr that consumes all tokens — a fragment that closes the surrounding derived table and appends a UNION (false) _wm_row UNION ALL SELECT … --) leaves a trailing ) and is rejected.
  • order_by must parse as the ORDER BY of a plain single SELECT (a SetExpr::Select, not a set operation), so a smuggled UNION fails to parse.
  • Any nested subquery in either fragment is rejected via a Visitor (pre_visit_query), which closes the error-based exfiltration vector (1 = CAST((SELECT secret …) AS INTEGER)) and IN (SELECT …) cross-table reads.
  • The value interpolated is the parser's own re-serialization of the validated AST, not the raw bytes — so a trailing -- comment can't survive to comment out the LIMIT/paren either.

Complex boolean predicates (AND/OR/BETWEEN/ILIKE/function calls/ANY, multi-column ORDER BY with ASC/DESC) still work. Unit tests added: sanitize_predicate_accepts_and_canonicalizes, sanitize_predicate_rejects_injection (incl. the exact reported UNION payload), sanitize_order_by_accepts_and_rejects.

P2 — member-level write authz: insert_datatable/update_datatable now call require_datatable_writer, which rejects operators (the workspace's read-mostly tier). Non-operator members can write (consistent with the member-level read handlers and data tables being a shared workspace store); there is still no per-resource ACL, which is the intended tier for this shared store.

P2 — function-call trust boundary (pg_sleep, and pg_read_file etc. if the datatable role is superuser): a permitted expression can still call any function the datatable's connection role is granted. This is now documented in the sanitize_sql_predicate doc comment and the PR safety notes — the connection role's privileges are the trust boundary, so datatable roles should be scoped (not superuser).

Minor — listDataTableSchemas: it is not marked x-mcp-tool in openapi.yaml (so it isn't exposed); it only appears in the defensive datatable_access_level/datatable_list_filter_key match arms so it's already handled if ever exposed. No change needed.

Still open (as noted in the test plan): end-to-end exercise from a live MCP client. The injection fix itself is covered by the new unit tests + a psql check that a sanitized predicate returns the expected rows.

@github-actions

github-actions Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Now I have a thorough understanding of the entire diff. The author (rubenfiszel) pushed commit d2a682a to address the P0 SQL injection finding and the P2 auth finding from prior reviews. Let me verify the fix stands.

Pi Review

Good to merge.

The prior-review P0 SQL injection finding (raw where_clause/order_by interpolation allowing UNION/subquery breakout) has been thoroughly addressed. The sqlparser-based sanitization (sanitize_sql_predicate, sanitize_sql_order_by) correctly:

  • Parses input as a single expression / ORDER-BY list and rejects trailing tokens (blocking ) _wm_row UNION ALL … closure)
  • Rejects subqueries via a Visitor, which closes both data exfiltration (IN (SELECT …)) and error-based exfiltration (= (SELECT …)) vectors
  • Returns the parser's re-serialization, so comments and other raw-byte tricks don't survive interpolation
  • Rejects SetExpr other than plain SELECT in order_by, blocking UNION/INTERSECT/EXCEPT

The P2 member-level write authz has been addressed with require_datatable_writer, which rejects operators. The function-call trust boundary (pg_sleep etc.) is now documented. All unit tests pass and cover the injection rejection payloads explicitly.

The scope enforcement is layered correctly:

  • runner.rs gates tool exposure/calls on datatable_tool_allowed (opt-in, read/write level)
  • core.rs enforces per-datatable name checks in call_endpoint before the proxied HTTP request (the JWT minted for the proxy call is route-scoped and no longer carries mcp: scopes)
  • core.rs filters list-responses post-return so restricted tokens never see other datatable names
  • List-response filter keys match actual response shapes: listDataTablesname, listDataTableTables/listDataTableSchemasdatatable_name

The frontend scope selector correctly:

  • Excludes datatable tools from the generic endpoint picker (DATATABLE_TOOL_NAMES set)
  • Lazily loads datatable names when the section is enabled
  • Forces write toggle off when token is read-only
  • Emits mcp:datatables:<read|write>:<names|*> only in non-all modes (mcp:all already grants full access)
  • Decodes existing mcp:datatables: scopes on initialization

One minor observation (P2):

P2 — select=* passed to queryDataTable would select a literal column named *, not all columns (workspaces.rs, query_datatable handler)

Each element of the select parameter goes through quote_pg_ident, so select=* produces "*" — a quoted identifier that in PostgreSQL matches a column literally named * rather than the star-wildcard. The default (when select is empty/absent) correctly produces the unquoted *. The OpenAPI x-mcp-instructions say "Comma-separated list of columns to return. Defaults to all columns" — a caller reading this might reasonably pass * and get surprising results. Consider either documenting that * is not special in select (use empty to get all columns), or special-casing the single-* input.

Test coverage

  • Backend: quote_pg_ident has two unit tests (wrapping/escaping, NUL/empty rejection). scope.rs has two new test functions (test_datatable_scope_opt_in_and_levels, test_contains_datatables) covering parse, allow, and containment. The new sanitize_predicate_accepts_and_canonicalizes, sanitize_predicate_rejects_injection, and sanitize_order_by_accepts_and_rejects tests cover the exact UNION payloads and subquery attempts from the prior review. No integration tests for the handlers themselves, but they follow the same tokio_postgres connection idiom as the existing get_datatable_table_schema handler.
  • Frontend: No new pure-logic utility files; the scope selector is a Svelte component (no component tests expected per policy). The auto-generated mcpEndpointTools.ts is generated output.
  • Auto-generated files: auto_generated_endpoints.rs and mcpEndpointTools.ts are generated from the OpenAPI spec — no test expectations for these.

Manual verification still needed

The end-to-end item from the PR's own test plan remains unchecked: calling the tools from an MCP client against a live data table, including a restricted token being rejected on an out-of-scope data table and a read-only token rejected on write. This exercises the actual MCP stack with a real Postgres backend, confirming that the per-datatable enforcement, list filtering, and the read-only/write toggles all work end-to-end from an MCP client's perspective.

@github-actions

github-actions Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Codex Review

cc @rubenfiszel
Mergeable, but should ideally address nits: drafting-history test comment in workspaces.rs

Found 1 issue:

  1. [P2] backend/windmill-api-workspaces/src/workspaces.rs:2328 — The test comment says the reported P0, which records PR review history in repo-tracked code. AGENTS.md says: “Describe the code as it is, never its drafting history”. Please drop the parenthetical or reword it as a timeless case description.

Test coverage

Backend: unit coverage was added for identifier quoting, SQL predicate/order sanitization, and datatable MCP scope parsing/containment. I did not see handler-level integration tests for the new query/insert/update endpoints.

Frontend: no component tests expected for this Svelte selector change.

Manual verification still needed: exercise the generated MCP tools against a live data table with a normal read, an insert/update token, a read-only token attempting write, and a restricted datatable token attempting an out-of-scope table; expected results are success only for in-scope allowed actions and clear denials otherwise.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant