feat(server): add POST /query endpoint for ad-hoc SQL

[BtXin]

Summary

Adds a POST /query endpoint to skardi-server that executes a single ad-hoc SQL statement against the data sources registered from ctx.yaml — bringing the CLI's skardi query --sql "..." capability to the HTTP server.

Design spec and implementation plan: docs/superpowers/specs/2026-07-17-query-endpoint-design.md, docs/superpowers/plans/2026-07-17-query-endpoint.md.

API

POST /query with { "sql": "...", "max_rows": 500 } (max_rows optional, default 1000). Returns the same JSON envelope as pipeline execution, plus a truncated flag:

{ "success": true, "data": [...], "rows": 500, "truncated": true, "execution_time_ms": 42, "timestamp": "..." }

Statement policy

Enforced by the existing sql_validator, run per request against the current ctx.yaml access modes:

• DDL (CREATE/DROP/ALTER/TRUNCATE) and COPY — always rejected.
• DML (INSERT/UPDATE/DELETE) — allowed only against sources with access_mode: read_write; read_only (the default) is rejected.
• SELECT / EXPLAIN-of-query / SHOW / DESCRIBE — allowed.
• Multi-statement input, EXPLAIN ANALYZE <write>, SET, and PREPARE/EXECUTE are rejected — these otherwise smuggle writes/DDL past the access-mode gate via the shared SessionContext.

Result cap

Default 1000 rows, overridable via max_rows (no server-side maximum). The cap is pushed into the DataFusion plan via a new DataFusionEngine::execute_with_limit; the endpoint fetches max_rows + 1 to detect truncation, then slices. Overflow of max_rows is guarded (saturating_add + i64::MAX clamp for DataFusion's i64 LIMIT literal).

Other changes

• Extracted response/JSON helpers from the 1,600-line pipeline_handlers.rs into a shared response.rs; pipeline responses remain byte-identical.
• Extracted a shared require_session auth helper used by both the query and pipeline handlers.
• Hardened the SQL validator (COPY, single-statement, EXPLAIN recursion, SET, PREPARE/EXECUTE) — this also tightens pipeline config-load validation.

Testing

• Validator unit tests: 26 passing (COPY, single-statement, EXPLAIN/SET/PREPARE bypass coverage).
• Engine unit tests: execute_with_limit truncation / full-result / empty-result.
• HTTP integration (crates/server/tests/query_http.rs): 15 passing — SELECT envelope, truncation boundaries, DDL/COPY/multi-statement/parse rejection, read-only vs read-write DML, EXPLAIN ANALYZE/PREPARE bypass rejection, usize::MAX overflow guard, execution error, and a 401 with the real better-auth layer.
• cargo fmt --all clean; cargo test --workspace green.

Notes for reviewers

The validator is a denylist (_ => Ok(()) catch-all with explicit rejections). Three same-class bypasses (EXPLAIN ANALYZE, SET, PREPARE/EXECUTE) were found and patched during review. A follow-up to convert it to an explicit read-only allowlist — to prevent a future DataFusion/sqlparser bump from reintroducing an executable statement type — is worth tracking separately.

🤖 Generated with Claude Code

[codecov]

Codecov Report

❌ Patch coverage is 94.27711% with 38 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
crates/server/src/query_handlers.rs	84.25%	17 Missing ⚠️
crates/skardi/src/sources/sql_validator.rs	96.85%	11 Missing ⚠️
crates/server/src/config.rs	84.21%	6 Missing ⚠️
crates/skardi/src/engine/datafusion.rs	95.16%	3 Missing ⚠️
crates/server/src/server.rs	95.45%	1 Missing ⚠️

📢 Thoughts on this report? Let us know!

[abbccdda]

Automated review of the POST /query endpoint (7 finder angles, each finding independently verified). Comments below, most severe first. The most important one is the auth.sessions token read — worth resolving before merge. CSRF and an uncapped-Other-path candidate were both investigated and refuted (JSON content-type + wildcard-CORS-without-credentials + SameSite=Lax block the former; DataFusion rejects RETURNING at plan time for the latter).

[abbccdda]

One clarification request on the design doc (docs/superpowers/specs/2026-07-17-query-endpoint-design.md): the Auth section says /query uses "the same session gate as pipeline execution (verify_session)", but the doc never explains where a session comes from — the endpoint only validates a token; creation happens in the pre-existing better-auth routes (POST /api/auth/sign-in/*), which issue the token/cookie that verify_session then checks against auth.sessions (expiry + active flag). Could you add a line making that explicit, so the doc stands alone?

Related: the doc also predates the review follow-ups in b6ed69b (the auth-schema read denial and the statement allowlist), so its statement-policy section no longer matches the implementation. Worth a small update while you're in there.

[abbccdda]

Second review round — commits b6ed69b (scoped policy / auth-schema denial / allowlist) + b0d9169 (sqlparser 0.59)

Reviewed only the two commits landed since the last round (1f8c0bb..HEAD), focused on whether they actually close the prior findings. The auth-schema denial and the fail-closed statement allowlist are solid — visit_relations covers FROM/JOIN/subquery/CTE/INSERT-target/DESCRIBE/SHOW, quoted/AUTH/datafusion.auth.sessions variants are all caught, and other => Err(StatementNotAllowed) correctly fails closed for future sqlparser variants. But the access-mode rework introduced a real regression. Findings ranked most-severe first.

1. 🔴 Read-only enforcement bypassed by default-schema-qualifying a flat source — crates/skardi/src/sources/sql_validator.rs:368

validator_config_from_sources keys table_access_modes by the bare source name (config.rs:890), and flat sources register into DataFusion's default datafusion.public catalog/schema. check_write_access looks up the full joined name, then falls back to only the first dotted segment. So for a read_only source users:

• INSERT INTO public.users … → lookup public.users misses, fallback get("public") misses → write allowed
• INSERT INTO datafusion.public.users … → fallback get("datafusion") misses → write allowed

DataFusion resolves all three forms to the same read-only table, so the control is defeated (same for UPDATE/DELETE). The old .last()-segment logic caught this; the test test_qualified_write_does_not_match_unrelated_flat_source (line 759) actually locks in the behavior that enables the bypass. Fix: strip the default datafusion/public qualifiers before lookup, or check the last segment as well as the first.

2. 🟠 INSERT with a non-TableName target fails open — sql_validator.rs:294

The Statement::Insert arm matches TableObject::TableName and routes everything else to _ => Ok(()), while INSERT is on the strict /query allowlist. Any INSERT form that parses to a TableObject::TableFunction (e.g. INSERT INTO FUNCTION …) skips check_write_access entirely and reaches the engine unvetted. Low exploitability today, but it's a fail-open on the path whose whole point is to fail closed.

3. 🟡 /query validates against a startup snapshot of access modes — server.rs:61/:224

validator_config is now built once into AppState; the old per-request rebuild under config.read() is gone. ServerConfig sits behind an RwLock "for runtime updates", so if a source's access_mode is ever flipped to read_only at runtime, /query keeps serving the stale (writable) policy until restart. Latent today (no production writer mutates data_sources), but the snapshot silently couples correctness to that staying true.

4. 🟡 Auth denial is honored by only one of the two validator entry points — sql_validator.rs (denied_schemas field)

denied_schemas lives on the shared SqlValidatorConfig but is enforced only by validate_single_sql; validate_sql (pipeline path) ignores it. The trust boundary is encoded in which function you call, not in the type. A future endpoint author validating untrusted SQL who reaches for the obviously-named validate_sql gets neither the allowlist nor the auth denial — reopening both original vulns. Consider distinct trusted/untrusted policy types, or pass denied schemas explicitly at the /query boundary.

5. 🟡 Auth-schema denial is a syntactic denylist over a shared SessionContext — config.rs:888

Auth registers into the same SessionContext /query executes against, and the denial enumerates syntactic paths to the auth name. Any indirect handle that doesn't surface as an auth-named relation (a later view/semantic-model/federated alias over auth.sessions, or a table-reference form visit_relations doesn't descend into) leaks bearer tokens with zero validator change. A /query-scoped context (or a catalog that never exposes auth) would make it structurally unreachable rather than name-filtered. Worth at least a coverage test asserting visit_relations reaches table-function arguments.

6. 🟡 sqlparser is a parallel pin, not DataFusion's re-export — Cargo.toml

Unifying to a single workspace 0.59 is good, but it's manually kept in lockstep with DataFusion's internal sqlparser. The next DataFusion bump can desync them and reopen the exact validator/engine parse-divergence class this commit closed. Importing datafusion::sql::sqlparser would make them share one parser by construction.

7. Cleanup — validator_config derived state duplicated at ~9 sites

Arc::new(validator_config_from_sources(&config.data_sources)) is copy-pasted across server.rs:224, pipeline_handlers.rs:861/:954, two #[cfg(test)] helpers in auth/routes.rs, and four integration-test builders. It's a pure function of config.data_sources that AppState already holds — an AppState constructor that derives it internally removes the boilerplate and the drift risk.

8. Cleanup — reserved "auth" schema name duplicated

config.rs:888 denies the literal "auth" independently of where auth/bridge.rs registers it. Export a pub const AUTH_SCHEMA and use it at both sites so the security policy can't silently stop covering the tables it's meant to protect.

9. Cleanup — check_denied_schemas re-derives the table name inconsistently — sql_validator.rs:~196

It builds the error's table via relation.to_string().to_lowercase() (keeps quote chars) while WriteNotAllowed uses extract_table_name (strips quoting). Quoted references get reported as "auth"."sessions" in one error and auth.sessions in the other. Call extract_table_name here too.

10. Efficiency — statement_keyword renders the whole AST for one word — sql_validator.rs:~169

On every allowlist rejection statement.to_string() Display-renders the entire statement just to split off the first token. Map the enum variant to a &'static str keyword instead. (Same micro-cost note applies to the per-part to_lowercase() allocations in check_denied_schemas/extract_table_name on the success path — eq_ignore_ascii_case avoids them.)

---

Findings 1–3 are the ones I'd block on. #1 is a genuine security regression with a test pinning the wrong behavior.

[BtXin]

Second-round general review — resolutions for findings #3–#10 (findings #1–#2 were fixed in f4b9475 with inline replies). Landed in 66818c9; full workspace suite green, clippy clean on touched files.

#3 — startup snapshot of access modes. Kept the snapshot (it was round-1's explicit ask) and documented the invariant it rests on: no runtime writer mutates a source's access_mode — verified the only config.write() is test-only. The doc now lives on both AppState.adhoc_policy and config::adhoc_policy_from_sources, stating that any future access-mode writer must rebuild the policy or the snapshot goes stale.

#4 — trust boundary in the type, not the function name. Split the policy types. SqlValidatorConfig now holds only access modes (no trust-boundary state); the untrusted /query path validates against a new AdhocSqlPolicy (access modes + denied schemas) that validate_single_sql requires. A future caller reaching for the trusted validate_sql with a bare config can no longer get the untrusted input past the allowlist + schema denial by accident.

#5 — syntactic denylist over a shared context. Added coverage tests proving visit_relations descends into indirect relations (UNION, scalar/IN/EXISTS subqueries), so the guard is complete for any single ad-hoc statement. Documented the true residual — an operator-defined view or federated alias over auth.sessions, which ad-hoc SQL cannot create (DDL is rejected) and which only trusted operator config could introduce. The structural fix (a /query-scoped SessionContext) is filed as #164: it's deferred because auth.* is a real feature (docs/auth/pipelines/active-users.yaml reads it via the shared engine context), so isolating /query needs a second data-source registration pass that risks doubling DB connection pools — disproportionate to a trusted-only residual. Happy to pull #164 forward if you'd rather it block here.

#6 — parallel sqlparser pin. Removed the standalone sqlparser dependency entirely. The validator now parses via datafusion::sql::sqlparser (DataFusion enables the visitor feature, so visit_relations is available). Validator and engine now share one parser by construction; a DataFusion bump that moves sqlparser becomes a compile error here, not a silent parse divergence.

#7 — duplicated derived state. Added AppState::new(config, engine, session_ctx, auth_layer, jobs), which derives the policy (and metrics) once. Removed the copy-pasted Arc::new(validator_config_from_sources(..)) from ~9 call sites.

#8 — duplicated "auth" literal. Exported pub const AUTH_SCHEMA from auth::bridge; both the registration site and the deny site (adhoc_policy_from_sources) reference it.

#9 — inconsistent table name in error. check_denied_schemas now builds the reported table via extract_table_name (quote-stripped), matching WriteNotAllowed.

#10 — statement_keyword rendered the whole AST. Replaced with a direct variant → &'static str match; no more Display of the full statement on each rejection.

🤖 Generated with Claude Code

[abbccdda]

Follow-up: hardening the /query security story

Did another security-focused pass on the branch after the last review round. No exploitable HIGH/MEDIUM issue — the validator is tight (type-split trusted/untrusted policies, strict statement allowlist that fails closed on unknown/future statement types, auth-schema denial via visit_relations, default-qualifier stripping, quoted-identifier normalization, shared parser with the engine). The remaining items are defense-in-depth, left as inline comments below by value.

Happy to take the DELETE and error-shape items as a fast-follow.

[abbccdda]

Nice work！Can't wait to see this go live