diff --git a/CHANGELOG.md b/CHANGELOG.md index 6ead2b10..683c8bd2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,12 @@ ## [Unreleased] +### Changes + +- Schema-changing SQL statements now require `tinycloud.sql/schema` (TC-1581). `tinycloud.sql/write` covers data statements only (`INSERT`, `UPDATE`, `DELETE`, `SELECT`). `CREATE`, `ALTER` and `DROP` of tables, views and indexes need a schema-conferring ability: `sql/schema`, `sql/admin` or `sql/*`. This is judged for every statement of a request against all the SQL abilities the invocation holds, in `query`, `execute`, the `schema` list of `execute`, `batch` and named statements, and again by the SQLite authorizer, which now also refuses `CREATE TRIGGER`. A client that holds both `write` and `schema` (the SDK sends both for `execute({ schema })` and for migrations) is unaffected; the SDK has requested `sql/schema` for DDL since 2.5.0. `PRAGMA` still needs `sql/admin`. The storage-quota gate classifies a statement as a write whoever may run it. Configurable with `[global.storage.sql] ddl_requires_schema = "enforce" | "log" | "off"` (default `enforce`): `log` runs the statement and records it, with one constant warning per request and a counter by service and statement kind, and no SQL text, space ids, DIDs or table names; `off` restores the earlier rule that `write` also suffices for the statements `log` counts; the other changes in this release (the `CREATE TRIGGER` refusal, the nested-block-comment refusal, the CTE write classification, the schema-table and PRAGMA restrictions) apply in every mode. `log` counts only statements the parser classifies as schema-changing. A request that mixes `schema` entries and `sql` is not transactional: the schema entries run first and can stay applied if a later statement is refused. No migration; the default needs no compose change. See `docs/sql-schema-authority.md`. +- Fixed `WITH … INSERT` and `WITH … UPDATE` SQL statements being classified as reads (TC-1585). They skipped the storage-quota gate, emitted no write-hook events and reported no touched table, so a write that changed the database was not made durable until a later write did. Any statement tree that contains an `INSERT` or `UPDATE` is now a write: it is quota-gated, publishes write events for its table, is persisted, and is refused to a read-tier grant and to a `readOnly` caveat. `WITH … DELETE` is still not accepted by the SQL parser, for any ability. +- Connection-level SQLite state no longer outlives a request on a database's shared connection (TC-1586). An `sql/admin` holder may now run only an allow-list of PRAGMAs: the plain schema reports, the database header values `user_version` and `application_id` (set or read), the check pragmas `integrity_check`, `quick_check` and `foreign_key_check`, and read-only reports such as `journal_mode`, `foreign_keys` and `page_count`. Any other PRAGMA, including every form that sets connection-level or process-wide settings, is refused. After any request in which the authorizer allowed a PRAGMA outside the plain schema reports, the node also resets the connection to its opening state, for in-memory and file-backed databases alike; if that reset fails, the database's actor is retired and the next request rebuilds it from the durable copy. The SQLite authorizer also no longer lets a data statement write `sqlite_master`, `sqlite_schema` or their temp counterparts: only a schema change (which writes them internally), a pure query, and `admin` do. `sql/write` keeps writing application tables. + ## [1.21.5] - 2026-10-20 ### Release notes diff --git a/docs/sql-schema-authority.md b/docs/sql-schema-authority.md new file mode 100644 index 00000000..5896632f --- /dev/null +++ b/docs/sql-schema-authority.md @@ -0,0 +1,54 @@ +# SQL schema authority (TC-1581) + +`tinycloud.sql/write` covers data statements. Statements that change the schema need `tinycloud.sql/schema`. + +| Statement | Needs | +|---|---| +| `SELECT` | `read`, `write`, `admin` or `*` | +| `INSERT`, `UPDATE`, `DELETE`, `REPLACE`, and `WITH … INSERT/UPDATE` | `write`, `admin` or `*` | +| `CREATE`/`ALTER`/`DROP` of tables, views and indexes (including `CREATE TABLE … AS SELECT` and temp objects) | `schema`, `admin` or `*` | +| `PRAGMA` | `admin` or `*` (unchanged) | +| `ATTACH`, `DETACH`, `VACUUM`, `ANALYZE`, `REINDEX`, `CREATE VIRTUAL TABLE`, `CREATE TRIGGER`, transaction control | never | + +`admin` implies `schema` in the capability registry (`capabilities.json`); `*` implies every SQL ability. `write` does not imply `schema`, and `schema` does not imply `write` or `read`: a holder of `schema` alone can run schema statements only. + +## How a request is judged + +An invocation can carry several SQL abilities. The node judges each statement against all of them, so a request that holds both `write` and `schema` can run data and schema statements together. That is what the SDK sends for `execute({ schema: [...] })` and for `migrations.apply`. A request that mixes the two statement kinds and holds only one of the abilities is refused. + +This applies to every request path that runs SQL: `query`, `execute`, each entry of the `schema` list of `execute`, `batch`, and named statements (`executeStatement`). A named statement that holds DDL needs `schema` on the invocation like the same SQL sent directly. + +The SQLite authorizer enforces the same rule on the engine's own callbacks, so a statement the parser misclassifies is still refused. + +## Configuration + +`[global.storage.sql] ddl_requires_schema` in `tinycloud.toml` (env `TINYCLOUD_STORAGE__SQL__DDL_REQUIRES_SCHEMA`; a new CVM env name needs `allowed_envs`): + +| Value | Behaviour | +|---|---| +| `"enforce"` (default) | A schema statement without a schema-conferring ability is refused with `403`. | +| `"log"` | The statement runs as before. Each such statement increments `tinycloud_ddl_without_schema_total{service="sql",kind=…}` and the request logs one constant warning (`sql ddl allowed without schema ability (ddl_requires_schema = log)`). Neither carries SQL text, space ids, DIDs or table names. `kind` is one of `create_table`, `create_view`, `create_index`, `alter_table`, `drop`. | +| `"off"` | The earlier rule: `write` also suffices for the schema statements `log` counts. The other changes in this document (the `CREATE TRIGGER` refusal, nested block comments, CTE write classification, schema-table and PRAGMA restrictions) apply in every mode. | + +`log` counts only statements the parser classifies as schema-changing. A statement the parser does not recognise as such is still refused by the SQLite authorizer unless the policy allows it, and is not counted. + +A request that carries both `schema` entries and `sql` is not transactional. The `schema` entries run first and can stay applied if a later statement is refused (only the storage-quota guard rolls a refused request back). + +A statement that nests block comments (`/* … /* … */ … */`) is refused for every ability: SQLite ends a comment at the first `*/`, the SQL parser nests them, and the two would read different statements. + +The warning is not mapped in the log shipper (`log-shipper/diagnostics.json`), so it ships as an unclassified `node_warning` until it is. + +## Clients + +The SDK has requested `tinycloud.sql/schema` for DDL since js-sdk 2.5.0 and sends `[write, schema]` for requests that mix both. A client or manifest that runs DDL under `sql/write` only needs `tinycloud.sql/schema` added to its grant. + +## Connection state and SQLite's schema tables (TC-1586) + +A database's SQLite connection is shared by every request on it, so connection-level state must not outlive the request that set it. + +- The SQLite authorizer allows anyone only the plain schema reports (`table_info`, `table_list`, `table_xinfo`, `database_list`, `index_list`, `index_info`, `foreign_key_list`). +- An `admin` (or `*`) holder may also set and read `user_version` and `application_id` (database header values), run the check pragmas `integrity_check`, `quick_check` and `foreign_key_check`, and read (not set) `schema_version`, `page_count`, `page_size`, `freelist_count`, `data_version`, `encoding`, `journal_mode`, `foreign_keys`, `writable_schema`, `query_only`, `collation_list`, `compile_options` and `pragma_list`. +- Every other PRAGMA is refused for everyone, including all forms that set connection-level or process-wide settings and maintenance forms such as `wal_checkpoint`, `optimize` and `incremental_vacuum`. +- After any request in which the authorizer allowed a PRAGMA outside the plain schema reports, the node resets the connection to the state it was opened in, for an in-memory database (journal mode `memory`) and a file-backed one (WAL). If the reset fails, the database's actor is retired and the next request rebuilds it from the durable copy. + +The SQLite authorizer also refuses a data statement that writes `sqlite_master`, `sqlite_schema` or their temp counterparts. Only a schema change (which writes them internally), a pure query (a `pragma_*` table-valued function reads the schema through them) and `admin` may. diff --git a/test/m1-realdata-e2e/tests/cross_layer_contract.rs b/test/m1-realdata-e2e/tests/cross_layer_contract.rs index c7c07eeb..c898e646 100644 --- a/test/m1-realdata-e2e/tests/cross_layer_contract.rs +++ b/test/m1-realdata-e2e/tests/cross_layer_contract.rs @@ -1180,7 +1180,7 @@ async fn seed_listen_sql(sql: &SqlService, space: &SpaceId) -> Result<()> { ], }, None, - "tinycloud.sql/write".to_string(), + "tinycloud.sql/admin".to_string(), ) .await?; sql.execute( @@ -1861,7 +1861,7 @@ async fn seed_grant_vector_sql(sql: &SqlService, space: &SpaceId) -> Result<()> ], }, None, - "tinycloud.sql/write".to_string(), + "tinycloud.sql/admin".to_string(), ) .await?; Ok(()) diff --git a/test/n4-mounted-e2e/src/main.rs b/test/n4-mounted-e2e/src/main.rs index 6c11c205..62d34cee 100644 --- a/test/n4-mounted-e2e/src/main.rs +++ b/test/n4-mounted-e2e/src/main.rs @@ -551,7 +551,7 @@ async fn seed_sql(rocket: &Rocket) -> Result<()> { .state::() .context("SQL service missing")?; use tinycloud_core::sql::{SqlRequest, SqlValue}; - service.execute(&space, "documents", SqlRequest::Execute { sql: "CREATE TABLE IF NOT EXISTS shared_documents (document_id INTEGER PRIMARY KEY, markdown TEXT NOT NULL)".into(), params: vec![], schema: None }, None, "tinycloud.sql/write".into()).await.map_err(|error| anyhow::anyhow!("SQL schema seed: {error}"))?; + service.execute(&space, "documents", SqlRequest::Execute { sql: "CREATE TABLE IF NOT EXISTS shared_documents (document_id INTEGER PRIMARY KEY, markdown TEXT NOT NULL)".into(), params: vec![], schema: None }, None, "tinycloud.sql/schema".into()).await.map_err(|error| anyhow::anyhow!("SQL schema seed: {error}"))?; service .execute( &space, diff --git a/test/w5-policy-runtime-node-e2e/tests/challenge_to_native_read.rs b/test/w5-policy-runtime-node-e2e/tests/challenge_to_native_read.rs index 60cf0238..8fb059c7 100644 --- a/test/w5-policy-runtime-node-e2e/tests/challenge_to_native_read.rs +++ b/test/w5-policy-runtime-node-e2e/tests/challenge_to_native_read.rs @@ -168,7 +168,7 @@ secret = "{}" params: vec![SqlValue::Text("alpha".to_string()), SqlValue::Integer(111)], }, None, - "tinycloud.sql/write".to_string(), + "tinycloud.sql/admin".to_string(), ) .await?; sql_service diff --git a/test/w5-policy-runtime-node-e2e/tests/sql_isolation.rs b/test/w5-policy-runtime-node-e2e/tests/sql_isolation.rs index 336ee63d..4b0d7b92 100644 --- a/test/w5-policy-runtime-node-e2e/tests/sql_isolation.rs +++ b/test/w5-policy-runtime-node-e2e/tests/sql_isolation.rs @@ -150,7 +150,7 @@ async fn legacy_web_artifacts_load_via_invoke_after_alias_cutover_and_restart() physical, serde_json::from_value(sql_write_body(row))?, None, - "tinycloud.sql/write".into(), + "tinycloud.sql/admin".into(), ) .await?; } @@ -322,7 +322,9 @@ async fn legacy_web_artifacts_load_via_invoke_after_alias_cutover_and_restart() &owner_vm, vec![], &fresh_resource, - "tinycloud.sql/write", + // `sql_write_body` creates its table and inserts a row: `admin` + // carries both (schema changes need `schema`, TC-1581). + "tinycloud.sql/admin", "urn:uuid:00000000-0000-4000-8000-000000000024", )?; let (status, response) = invoke_json(&client, fresh_write, &sql_write_body("new-row")).await; @@ -528,7 +530,7 @@ async fn legacy_alias_collision_refuses_the_whole_transaction() -> Result<()> { name, serde_json::from_value(sql_write_body("row"))?, None, - "tinycloud.sql/write".into(), + "tinycloud.sql/admin".into(), ) .await?; } @@ -575,7 +577,7 @@ async fn legacy_inventory_attributes_from_invoked_abilities_without_a_grant() -> &owner_vm, vec![], &resource, - "tinycloud.sql/write", + "tinycloud.sql/admin", "urn:uuid:00000000-0000-4000-8000-000000000025", )?; let (status, response) = invoke_json(&client, header, &sql_write_body("digest-row")).await; @@ -588,7 +590,7 @@ async fn legacy_inventory_attributes_from_invoked_abilities_without_a_grant() -> "only", serde_json::from_value(sql_write_body("legacy-row"))?, None, - "tinycloud.sql/write".into(), + "tinycloud.sql/admin".into(), ) .await?; let items = database_migration::inventory(&conn, &datadir).await?; @@ -608,7 +610,7 @@ async fn legacy_inventory_attributes_from_invoked_abilities_without_a_grant() -> &owner_vm, vec![], &resource, - "tinycloud.sql/write", + "tinycloud.sql/admin", "urn:uuid:00000000-0000-4000-8000-000000000026", )?; let (status, response) = invoke_json(&client, unfenced_write, &sql_write_body("blocked")).await; @@ -994,7 +996,7 @@ async fn sql_paths_do_not_share_databases_and_grants_are_exact() -> Result<()> { let tempdir = TempDir::new()?; let engine = Engine { service: "sql", - write_ability: "tinycloud.sql/write", + write_ability: "tinycloud.sql/admin", read_ability: "tinycloud.sql/read", grant_seed: b"tc780-n1-sql-read-appA-connectors", nonce_prefix: "000000000a", @@ -1046,7 +1048,7 @@ fn regression_engine(service: &'static str) -> Result { Ok(match service { "sql" => Engine { service, - write_ability: "tinycloud.sql/write", + write_ability: "tinycloud.sql/admin", read_ability: "tinycloud.sql/read", grant_seed: b"tc780-n2-ancestor-sql", nonce_prefix: "000000000c", diff --git a/test/w5-policy-runtime-node-e2e/tests/tc119_registry_wire_paths.rs b/test/w5-policy-runtime-node-e2e/tests/tc119_registry_wire_paths.rs index 0c08af62..55a4253a 100644 --- a/test/w5-policy-runtime-node-e2e/tests/tc119_registry_wire_paths.rs +++ b/test/w5-policy-runtime-node-e2e/tests/tc119_registry_wire_paths.rs @@ -200,7 +200,7 @@ secret = "{}" params: vec![SqlValue::Integer(1)], }, None, - "tinycloud.sql/write".to_string(), + "tinycloud.sql/admin".to_string(), ) .await?; diff --git a/tinycloud-core/src/data_owners.rs b/tinycloud-core/src/data_owners.rs index c40625fc..c812bc94 100644 --- a/tinycloud-core/src/data_owners.rs +++ b/tinycloud-core/src/data_owners.rs @@ -898,7 +898,7 @@ mod tests { &other_sql, "default", "main", - "tinycloud.sql/write", + "tinycloud.sql/schema", crate::sql::SqlRequest::Execute { sql: "CREATE TABLE notes (id INTEGER)".to_string(), params: vec![], diff --git a/tinycloud-core/src/sql/authority.rs b/tinycloud-core/src/sql/authority.rs new file mode 100644 index 00000000..bf9ece53 --- /dev/null +++ b/tinycloud-core/src/sql/authority.rs @@ -0,0 +1,307 @@ +//! The SQL abilities one invocation holds, and the node's policy for schema +//! changes (TC-1581). +//! +//! An invocation can carry several `tinycloud.sql/*` capabilities at once (the +//! SDK sends `[write, schema]` for `execute({ schema: [...] })` and for its +//! migrations helper). Authorization therefore cannot be decided from a single +//! "preferred" ability: a data statement is judged against the data +//! abilities held, a schema-changing statement against a schema-conferring +//! ability (`schema`, `admin`, `*`), and a request that mixes the two needs both. + +use prometheus::{register_int_counter_vec, IntCounterVec}; +use serde::{Deserialize, Serialize}; +use std::sync::LazyLock; + +use crate::policy_capability::ability_matches; + +const ADMIN: &str = "tinycloud.sql/admin"; +const SCHEMA: &str = "tinycloud.sql/schema"; +const WRITE: &str = "tinycloud.sql/write"; +const READ: &str = "tinycloud.sql/read"; + +/// Whether a schema-changing statement needs a schema-conferring ability. +/// +/// Configured as `[global.storage.sql] ddl_requires_schema`. +#[derive(Clone, Copy, Debug, Default, Deserialize, Eq, Hash, PartialEq, Serialize)] +#[serde(rename_all = "lowercase")] +pub enum DdlPolicy { + /// A schema-changing statement runs with `write` or `schema` (the + /// behaviour before TC-1581). + Off, + /// As `Off`, but a statement that `Enforce` would refuse is counted and + /// logged once per request, with no request content in either. + Log, + /// A schema-changing statement needs `schema`, `admin` or `*`. + #[default] + Enforce, +} + +/// The statement kind recorded in `Log` mode. A closed set: it is the only +/// label on the counter and never carries a name from the request. +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +pub enum DdlKind { + CreateTable, + CreateView, + CreateIndex, + AlterTable, + Drop, +} + +impl DdlKind { + pub fn as_str(self) -> &'static str { + match self { + DdlKind::CreateTable => "create_table", + DdlKind::CreateView => "create_view", + DdlKind::CreateIndex => "create_index", + DdlKind::AlterTable => "alter_table", + DdlKind::Drop => "drop", + } + } +} + +static DDL_LOG_COUNTER: LazyLock = LazyLock::new(|| { + register_int_counter_vec!( + "tinycloud_ddl_without_schema_total", + "Schema-changing statements that were allowed only because ddl_requires_schema is \"log\".", + &["service", "kind"] + ) + .expect("ddl counter should register exactly once") +}); + +/// Count one statement that `Enforce` would have refused. +pub fn record_ddl_without_schema(kind: DdlKind) { + DDL_LOG_COUNTER + .with_label_values(&["sql", kind.as_str()]) + .inc(); +} + +/// Current value of the `Log`-mode counter for `kind` (tests). +pub fn ddl_without_schema_count(kind: DdlKind) -> u64 { + DDL_LOG_COUNTER + .with_label_values(&["sql", kind.as_str()]) + .get() +} + +/// Tests that run statements under `DdlPolicy::Log` bump the one process-wide +/// counter; those that read it hold this so deltas are exact. +#[cfg(test)] +pub(crate) static LOG_COUNTER_LOCK: tokio::sync::Mutex<()> = tokio::sync::Mutex::const_new(()); + +/// Every `tinycloud.sql/*` ability the invocation holds, plus the node's DDL +/// policy. +#[derive(Clone, Debug, Default, Eq, PartialEq)] +pub struct SqlAuthority { + held: Vec, + ddl_policy: DdlPolicy, +} + +impl SqlAuthority { + pub fn new(held: I) -> Self + where + I: IntoIterator, + S: Into, + { + Self { + held: held.into_iter().map(Into::into).collect(), + ddl_policy: DdlPolicy::default(), + } + } + + pub fn with_ddl_policy(mut self, policy: DdlPolicy) -> Self { + self.ddl_policy = policy; + self + } + + pub fn ddl_policy(&self) -> DdlPolicy { + self.ddl_policy + } + + fn confers(&self, required: &str) -> bool { + self.held.iter().any(|held| ability_matches(held, required)) + } + + /// `admin` or `*`. + pub fn is_admin(&self) -> bool { + self.confers(ADMIN) + } + + /// `schema`, or `admin` / `*` through the registry. + pub fn confers_schema(&self) -> bool { + self.confers(SCHEMA) + } + + /// `write`, `*` or `admin`: may change data. + pub fn confers_data_write(&self) -> bool { + self.is_admin() || self.confers(WRITE) + } + + /// Holds an ability that confers data access: `read` (or its `select` + /// alias), `write`, `admin` or `*`. Only registered abilities count: an + /// unrecognised `tinycloud.sql/*` URN confers nothing, so it neither + /// widens nor narrows what the holder may do. + pub fn has_non_schema_ability(&self) -> bool { + self.is_admin() || self.confers(WRITE) || self.confers(READ) + } + + /// Holds schema authority and nothing that confers data access, so it may + /// run schema statements only. This is not a confers-check: `admin` holds + /// `schema` but is not restricted here. + pub fn is_schema_only(&self) -> bool { + self.confers_schema() && !self.has_non_schema_ability() + } + + /// Reads but cannot write: `read` (or `select`) without `write`, `admin` or + /// `*`. `sql/*` implies read but must still permit writes. + pub fn is_read_tier_only(&self) -> bool { + self.confers(READ) && !self.confers_data_write() + } + + /// Whether a schema-changing statement may run for this holder. + /// + /// `Enforce` needs a schema-conferring ability. `Log` and `Off` keep the + /// pre-TC-1581 rule, where `write` also sufficed. + pub fn may_change_schema(&self) -> bool { + match self.ddl_policy { + DdlPolicy::Enforce => self.confers_schema(), + DdlPolicy::Log | DdlPolicy::Off => { + self.confers_schema() || self.confers(WRITE) || self.is_admin() + } + } + } + + /// The statement would be refused under `Enforce` but runs under `Log`. + pub fn schema_change_only_by_legacy_rule(&self) -> bool { + self.ddl_policy == DdlPolicy::Log && self.may_change_schema() && !self.confers_schema() + } +} + +impl From<&str> for SqlAuthority { + fn from(ability: &str) -> Self { + Self::new([ability]) + } +} + +impl From for SqlAuthority { + fn from(ability: String) -> Self { + Self::new([ability]) + } +} + +impl From<[&str; N]> for SqlAuthority { + fn from(held: [&str; N]) -> Self { + Self::new(held) + } +} + +impl From> for SqlAuthority { + fn from(held: Vec) -> Self { + Self::new(held) + } +} + +#[cfg(test)] +mod tests { + use super::*; + + fn held(abilities: &[&str]) -> SqlAuthority { + SqlAuthority::new(abilities.iter().copied()) + } + + #[test] + fn schema_is_conferred_by_schema_admin_and_wildcard_only() { + for (abilities, expected) in [ + (&["tinycloud.sql/read"][..], false), + (&["tinycloud.sql/select"][..], false), + (&["tinycloud.sql/write"][..], false), + (&["tinycloud.sql/read", "tinycloud.sql/write"][..], false), + (&["tinycloud.sql/schema"][..], true), + (&["tinycloud.sql/write", "tinycloud.sql/schema"][..], true), + (&["tinycloud.sql/admin"][..], true), + (&["tinycloud.sql/*"][..], true), + (&["tinycloud.sql/ddl"][..], false), + (&["tinycloud.duckdb/admin"][..], false), + ] { + assert_eq!(held(abilities).confers_schema(), expected, "{abilities:?}"); + } + } + + #[test] + fn schema_does_not_confer_data_authority() { + let schema_only = held(&["tinycloud.sql/schema"]); + assert!(!schema_only.confers_data_write()); + assert!(!schema_only.has_non_schema_ability()); + let both = held(&["tinycloud.sql/schema", "tinycloud.sql/write"]); + assert!(both.confers_data_write()); + assert!(both.has_non_schema_ability()); + } + + #[test] + fn read_tier_is_exact() { + assert!(held(&["tinycloud.sql/read"]).is_read_tier_only()); + assert!(held(&["tinycloud.sql/select"]).is_read_tier_only()); + assert!(held(&["tinycloud.sql/read", "tinycloud.sql/schema"]).is_read_tier_only()); + assert!(!held(&["tinycloud.sql/read", "tinycloud.sql/write"]).is_read_tier_only()); + assert!(!held(&["tinycloud.sql/*"]).is_read_tier_only()); + assert!(!held(&["tinycloud.sql/schema"]).is_read_tier_only()); + } + + #[test] + fn unregistered_abilities_confer_nothing() { + const UNREGISTERED: &str = "tinycloud.sql/not-a-registered-ability"; + let schema_alone = held(&["tinycloud.sql/schema"]); + let with_unregistered = held(&["tinycloud.sql/schema", UNREGISTERED]); + for authority in [&schema_alone, &with_unregistered] { + assert!(authority.is_schema_only()); + assert!(!authority.has_non_schema_ability()); + assert!(!authority.is_read_tier_only()); + assert!(!authority.confers_data_write()); + } + assert!(held(&["tinycloud.sql/read", UNREGISTERED]).is_read_tier_only()); + assert!(!held(&["tinycloud.sql/write", UNREGISTERED]).is_read_tier_only()); + let nothing = held(&[UNREGISTERED]); + assert!(!nothing.is_schema_only()); + assert!(!nothing.has_non_schema_ability()); + assert!(!nothing.confers_schema()); + } + + #[test] + fn policy_selects_who_may_change_schema() { + let write = held(&["tinycloud.sql/write"]); + assert!(!write + .clone() + .with_ddl_policy(DdlPolicy::Enforce) + .may_change_schema()); + assert!(write + .clone() + .with_ddl_policy(DdlPolicy::Log) + .may_change_schema()); + assert!(write + .clone() + .with_ddl_policy(DdlPolicy::Off) + .may_change_schema()); + assert!(write + .with_ddl_policy(DdlPolicy::Log) + .schema_change_only_by_legacy_rule()); + let both = held(&["tinycloud.sql/write", "tinycloud.sql/schema"]); + assert!(!both + .with_ddl_policy(DdlPolicy::Log) + .schema_change_only_by_legacy_rule()); + assert!(!held(&["tinycloud.sql/read"]) + .with_ddl_policy(DdlPolicy::Off) + .may_change_schema()); + } + + #[test] + fn policy_is_enforce_by_default_and_parses_lowercase() { + assert_eq!(DdlPolicy::default(), DdlPolicy::Enforce); + assert_eq!(SqlAuthority::default().ddl_policy(), DdlPolicy::Enforce); + for (text, expected) in [ + ("\"off\"", DdlPolicy::Off), + ("\"log\"", DdlPolicy::Log), + ("\"enforce\"", DdlPolicy::Enforce), + ] { + assert_eq!(serde_json::from_str::(text).unwrap(), expected); + } + assert!(serde_json::from_str::("\"warn\"").is_err()); + } +} diff --git a/tinycloud-core/src/sql/authorizer.rs b/tinycloud-core/src/sql/authorizer.rs index e1ecbcf8..7a9b9bc5 100644 --- a/tinycloud-core/src/sql/authorizer.rs +++ b/tinycloud-core/src/sql/authorizer.rs @@ -1,15 +1,72 @@ +use std::sync::{ + atomic::{AtomicBool, Ordering}, + Arc, +}; + use rusqlite::hooks::{AuthAction, AuthContext, Authorization}; +use super::authority::SqlAuthority; use super::caveats::SqlCaveats; -use crate::policy_capability::{ability_matches, resolve_alias}; - -fn can_write_data(ability: &str, is_admin: bool) -> bool { - // TC-119: confers-write gate (registry-aware). `admin` is covered by the - // `is_admin` flag; `write` matches directly; `sql/*` matches via the - // registry implication `* ⊃ write`. Identical to the prior - // `admin | write | *` set. NOTE: `schema` does NOT confer write here (the - // registry declares `admin ⊃ schema`, not `schema ⊃ write`). - is_admin || ability_matches(ability, "tinycloud.sql/write") +use super::parser::ParsedQuery; + +/// What the statement being authorized is, as the parser classified it. +/// +/// The authorizer is the authoritative defence. It judges every SQLite +/// action against the abilities held, and uses the scope only to decide +/// which restrictions apply to this statement. +#[derive(Clone, Copy, Debug, Default, Eq, PartialEq)] +pub struct StatementScope { + pub is_ddl: bool, + pub is_read_only: bool, +} + +impl StatementScope { + pub fn of(parsed: &ParsedQuery) -> Self { + Self { + is_ddl: parsed.is_ddl, + is_read_only: parsed.is_read_only, + } + } +} + +/// What the abilities held let this statement do. +#[derive(Clone, Copy, Debug)] +struct Tier { + is_admin: bool, + /// `write` (or admin / `*`): may change data. + can_write_data: bool, + /// A schema-changing action may run. Under `DdlPolicy::Enforce` this needs + /// a schema-conferring ability (`schema`, `admin`, `*`); `write` is not + /// enough (TC-1581). + can_change_schema: bool, + /// The statement runs with schema authority and nothing that writes data: + /// it may not read or write application tables. + schema_restricted: bool, + /// The statement may write SQLite's own schema tables: a schema change + /// writes them internally. A data statement may not (TC-1586): `write` + /// does not edit `sqlite_master`. + may_write_schema_tables: bool, + /// The parser classified the statement as a pure read. Only SQLite's own + /// schema bookkeeping UPDATE for a `pragma_*` function is allowed to it. + read_only_scope: bool, +} + +impl Tier { + fn new(authority: &SqlAuthority, scope: StatementScope) -> Self { + let is_admin = authority.is_admin(); + let can_write_data = authority.confers_data_write(); + let can_change_schema = authority.may_change_schema(); + Self { + is_admin, + can_write_data, + can_change_schema, + schema_restricted: !is_admin + && !can_write_data + && (scope.is_ddl || authority.is_schema_only()), + may_write_schema_tables: is_admin || (scope.is_ddl && can_change_schema), + read_only_scope: scope.is_read_only, + } + } } fn is_sqlite_schema_table(table_name: &str) -> bool { @@ -19,12 +76,13 @@ fn is_sqlite_schema_table(table_name: &str) -> bool { ) } -fn can_write_table(ability: &str, is_admin: bool, table_name: &str) -> bool { - // The second clause is an exact-tier match on `schema` (writes to the - // sqlite schema tables during DDL), NOT a confers-check — so compare the - // alias-resolved URN rather than using `ability_matches`. - can_write_data(ability, is_admin) - || (resolve_alias(ability) == "tinycloud.sql/schema" && is_sqlite_schema_table(table_name)) +fn can_write_table(tier: &Tier, table_name: &str) -> bool { + if is_sqlite_schema_table(table_name) { + // The second clause lets schema-only authority write the sqlite + // schema tables while it runs DDL. + return tier.may_write_schema_tables || tier.schema_restricted; + } + tier.can_write_data } #[derive(Clone, Copy, Debug, Eq, PartialEq)] @@ -68,14 +126,17 @@ fn is_sqlite_stat_table(table_name: &str) -> bool { } fn can_update_table( - ability: &str, - is_admin: bool, + tier: &Tier, database_name: Option<&str>, table_name: &str, schema_ddl_state: Option<&SchemaDdlState>, ) -> bool { - can_write_table(ability, is_admin, table_name) - || (resolve_alias(ability) == "tinycloud.sql/schema" + can_write_table(tier, table_name) + // SQLite's first use of a `pragma_*` table-valued function in a + // connection touches the schema table with an UPDATE, and nothing + // else: a pure query gets that one action, not INSERT or DELETE. + || (tier.read_only_scope && is_sqlite_schema_table(table_name)) + || (tier.schema_restricted && matches!( table_name, "sqlite_sequence" | "sqlite_stat1" | "sqlite_stat4" @@ -88,15 +149,14 @@ fn can_update_table( } fn can_delete_table( - ability: &str, - is_admin: bool, + tier: &Tier, database_name: Option<&str>, table_name: &str, schema_ddl_state: Option<&SchemaDdlState>, schema_drop_target: Option<&SchemaDropTarget>, ) -> bool { - can_write_table(ability, is_admin, table_name) - || (resolve_alias(ability) == "tinycloud.sql/schema" + can_write_table(tier, table_name) + || (tier.schema_restricted && (schema_drop_target.is_some_and(|target| { target.database_name.as_deref() == database_name && target.object_name == table_name }) || (table_name == "sqlite_sequence" @@ -114,11 +174,98 @@ fn can_delete_table( )))) } +/// Authorize with one ability string (plus the `is_admin` flag), for a +/// statement the caller has not classified. Judges DDL by the abilities +/// alone; use [`create_authorizer_scoped`] when the parser's classification +/// of the statement is known. pub fn create_authorizer( caveats: Option, ability: String, is_admin: bool, ) -> impl FnMut(AuthContext<'_>) -> Authorization { + let mut held = vec![ability]; + if is_admin { + held.push("tinycloud.sql/admin".to_string()); + } + create_authorizer_scoped(caveats, &SqlAuthority::new(held), StatementScope::default()) +} + +/// Set once the authorizer has allowed a PRAGMA that is not one of the plain +/// read-only schema reports. The actor resets the connection after any request +/// in which this was set (TC-1586). +pub type PragmaTracker = Arc; + +/// PRAGMAs anyone may run: reports on the schema that change nothing. +const READ_ONLY_PRAGMAS: [&str; 7] = [ + "table_info", + "table_list", + "table_xinfo", + "database_list", + "index_list", + "index_info", + "foreign_key_list", +]; + +/// PRAGMAs an admin may also set. They live in the database header, not in +/// connection state. +const ADMIN_SETTABLE_PRAGMAS: [&str; 2] = ["user_version", "application_id"]; + +/// PRAGMAs an admin may run in their reporting form (no value). +const ADMIN_REPORT_PRAGMAS: [&str; 15] = [ + "user_version", + "application_id", + "schema_version", + "page_count", + "page_size", + "freelist_count", + "data_version", + "encoding", + "journal_mode", + "foreign_keys", + "writable_schema", + "query_only", + "collation_list", + "compile_options", + "pragma_list", +]; + +/// PRAGMAs that check the database; they take an argument and change nothing. +const ADMIN_CHECK_PRAGMAS: [&str; 3] = ["integrity_check", "quick_check", "foreign_key_check"]; + +fn pragma_in(list: &[&str], name: &str) -> bool { + list.iter().any(|known| known.eq_ignore_ascii_case(name)) +} + +/// Whether an admin may run this PRAGMA. Everything that would change +/// connection-level or process-wide SQLite settings is refused: the allow-list +/// is the point, so nothing outside it is reachable by name. +fn admin_pragma_allowed(name: &str, has_value: bool) -> bool { + pragma_in(&ADMIN_SETTABLE_PRAGMAS, name) + || pragma_in(&ADMIN_CHECK_PRAGMAS, name) + || (!has_value && pragma_in(&ADMIN_REPORT_PRAGMAS, name)) +} + +/// The SQLite authorizer for one statement (TC-1581): every action is judged +/// against all the SQL abilities the invocation holds, and `scope` says what +/// the parser made of the statement. +pub fn create_authorizer_scoped( + caveats: Option, + authority: &SqlAuthority, + scope: StatementScope, +) -> impl FnMut(AuthContext<'_>) -> Authorization { + create_authorizer_tracked(caveats, authority, scope, PragmaTracker::default()) +} + +/// As [`create_authorizer_scoped`], and records in `pragmas` that a PRAGMA +/// outside the read-only reports was allowed. +pub fn create_authorizer_tracked( + caveats: Option, + authority: &SqlAuthority, + scope: StatementScope, + pragmas: PragmaTracker, +) -> impl FnMut(AuthContext<'_>) -> Authorization { + let tier = Tier::new(authority, scope); + let is_admin = tier.is_admin; let mut schema_ddl_authorized = false; let mut schema_ddl_state: Option = None; let mut schema_drop_target: Option = None; @@ -127,24 +274,23 @@ pub fn create_authorizer( // Always deny attach/detach AuthAction::Attach { .. } | AuthAction::Detach { .. } => Authorization::Deny, - // Pragma whitelist - AuthAction::Pragma { pragma_name, .. } => { - let readonly_pragmas = [ - "table_info", - "table_list", - "table_xinfo", - "database_list", - "index_list", - "index_info", - "foreign_key_list", - ]; - if readonly_pragmas.contains(&pragma_name) || is_admin { + // PRAGMA: plain schema reports for everyone; for an admin, the + // allow-list above. Any other PRAGMA is refused. + AuthAction::Pragma { + pragma_name, + pragma_value, + } => { + if pragma_in(&READ_ONLY_PRAGMAS, pragma_name) { + Authorization::Allow + } else if is_admin && admin_pragma_allowed(pragma_name, pragma_value.is_some()) { + pragmas.store(true, Ordering::Relaxed); Authorization::Allow } else { Authorization::Deny } } + // Function whitelist AuthAction::Function { function_name } => { let allowed_functions = [ @@ -269,8 +415,7 @@ pub fn create_authorizer( table_name, column_name, } => { - if !is_admin - && resolve_alias(ability.as_str()) == "tinycloud.sql/schema" + if tier.schema_restricted && !schema_ddl_authorized && !is_sqlite_schema_table(table_name) { @@ -289,7 +434,7 @@ pub fn create_authorizer( // Write operations AuthAction::Insert { table_name } => { - if !can_write_table(ability.as_str(), is_admin, table_name) { + if !can_write_table(&tier, table_name) { return Authorization::Deny; } if let Some(ref caveats) = caveats { @@ -305,8 +450,7 @@ pub fn create_authorizer( AuthAction::Delete { table_name } => { if !can_delete_table( - ability.as_str(), - is_admin, + &tier, ctx.database_name, table_name, schema_ddl_state.as_ref(), @@ -330,8 +474,7 @@ pub fn create_authorizer( column_name, } => { if !can_update_table( - ability.as_str(), - is_admin, + &tier, ctx.database_name, table_name, schema_ddl_state.as_ref(), @@ -352,15 +495,14 @@ pub fn create_authorizer( Authorization::Allow } - // DDL operations + // DDL operations (TC-1581). Every schema-changing action needs the + // schema authority that `tier.can_change_schema` encodes, so a + // statement the parser misclassified as data is still refused here. AuthAction::CreateTable { .. } | AuthAction::CreateTempTable { .. } => { // Do not enable application-table reads for CREATE TABLE. SQLite's // authorizer does not emit an Insert callback for CTAS result rows, // so allowing its source SELECT would let schema authority copy data. - if is_admin - || ability_matches(ability.as_str(), "tinycloud.sql/write") - || ability_matches(ability.as_str(), "tinycloud.sql/schema") - { + if tier.can_change_schema { Authorization::Allow } else { Authorization::Deny @@ -373,13 +515,10 @@ pub fn create_authorizer( | AuthAction::DropTempTable { table_name: object_name, } => { - if !(is_admin - || ability_matches(ability.as_str(), "tinycloud.sql/write") - || ability_matches(ability.as_str(), "tinycloud.sql/schema")) - { + if !tier.can_change_schema { Authorization::Deny } else { - if !is_admin && resolve_alias(ability.as_str()) == "tinycloud.sql/schema" { + if tier.schema_restricted { schema_ddl_authorized = true; schema_ddl_state = Some(SchemaDdlState { database_name: ctx.database_name.map(str::to_owned), @@ -401,13 +540,10 @@ pub fn create_authorizer( | AuthAction::DropTempView { view_name: object_name, } => { - if !(is_admin - || ability_matches(ability.as_str(), "tinycloud.sql/write") - || ability_matches(ability.as_str(), "tinycloud.sql/schema")) - { + if !tier.can_change_schema { Authorization::Deny } else { - if !is_admin && resolve_alias(ability.as_str()) == "tinycloud.sql/schema" { + if tier.schema_restricted { schema_ddl_authorized = true; schema_drop_target = Some(SchemaDropTarget { database_name: ctx.database_name.map(str::to_owned), @@ -420,14 +556,11 @@ pub fn create_authorizer( } AuthAction::AlterTable { database_name, .. } => { - if !(is_admin - || ability_matches(ability.as_str(), "tinycloud.sql/write") - || ability_matches(ability.as_str(), "tinycloud.sql/schema")) - { + if !tier.can_change_schema { Authorization::Deny } else { alter_table_authorized = true; - if !is_admin && resolve_alias(ability.as_str()) == "tinycloud.sql/schema" { + if tier.schema_restricted { schema_ddl_authorized = true; schema_ddl_state = Some(SchemaDdlState { database_name: Some(database_name.to_owned()), @@ -439,13 +572,10 @@ pub fn create_authorizer( } AuthAction::DropIndex { .. } | AuthAction::DropTempIndex { .. } => { - if !(is_admin - || ability_matches(ability.as_str(), "tinycloud.sql/write") - || ability_matches(ability.as_str(), "tinycloud.sql/schema")) - { + if !tier.can_change_schema { Authorization::Deny } else { - if !is_admin && resolve_alias(ability.as_str()) == "tinycloud.sql/schema" { + if tier.schema_restricted { schema_ddl_authorized = true; schema_ddl_state = Some(SchemaDdlState { database_name: ctx.database_name.map(str::to_owned), @@ -456,27 +586,26 @@ pub fn create_authorizer( } } + // The parser cannot parse CREATE TRIGGER; deny it here as well so the + // authorizer does not rely on that. A trigger runs later, with the + // authority of whoever fires it. + AuthAction::CreateTrigger { .. } | AuthAction::CreateTempTrigger { .. } => { + Authorization::Deny + } + AuthAction::CreateIndex { .. } - | AuthAction::CreateTrigger { .. } | AuthAction::DropTrigger { .. } | AuthAction::CreateView { .. } | AuthAction::CreateTempIndex { .. } - | AuthAction::CreateTempTrigger { .. } | AuthAction::DropTempTrigger { .. } | AuthAction::CreateTempView { .. } // SQLite fires SQLITE_REINDEX while building an index; gate it like the // CreateIndex it accompanies so an authorized CREATE INDEX can complete. | AuthAction::Reindex { .. } => { - // TC-119: DDL is permitted for abilities that confer write OR - // schema (`sql/*` implies both). Equivalent to the prior - // `write | schema | *` set; `admin` is handled by `is_admin`. - if !(is_admin - || ability_matches(ability.as_str(), "tinycloud.sql/write") - || ability_matches(ability.as_str(), "tinycloud.sql/schema")) - { + if !tier.can_change_schema { Authorization::Deny } else { - if !is_admin && resolve_alias(ability.as_str()) == "tinycloud.sql/schema" { + if tier.schema_restricted { schema_ddl_authorized = true; } Authorization::Allow @@ -488,10 +617,7 @@ pub fn create_authorizer( AuthAction::Transaction { .. } | AuthAction::Savepoint { .. } => Authorization::Deny, AuthAction::Select => { - if !is_admin - && resolve_alias(ability.as_str()) == "tinycloud.sql/schema" - && !schema_ddl_authorized - { + if tier.schema_restricted && !schema_ddl_authorized { return Authorization::Deny; } Authorization::Allow @@ -516,9 +642,9 @@ mod tests { } #[test] - fn create_index_allowed_with_write_ability() { - // CREATE [UNIQUE] INDEX fires CreateIndex followed by Reindex while the - // index is built; both must pass for the statement to succeed. + fn create_index_denied_with_write_ability() { + // TC-1581: `write` no longer runs schema-changing actions. CREATE + // [UNIQUE] INDEX fires CreateIndex followed by Reindex. let create_index = AuthAction::CreateIndex { index_name: "uq_interaction_nonce", table_name: "interaction", @@ -529,11 +655,11 @@ mod tests { assert_eq!( authorize("tinycloud.sql/write", false, create_index), - Authorization::Allow + Authorization::Deny ); assert_eq!( authorize("tinycloud.sql/write", false, reindex), - Authorization::Allow + Authorization::Deny ); } @@ -717,25 +843,20 @@ mod tests { } #[test] - fn create_unique_index_executes_with_write_cap() { - // End-to-end against real SQLite: the CreateIndex -> Reindex callback - // sequence fired while building the index must pass under a write cap. + fn create_unique_index_blocked_with_write_cap() { + // End-to-end against real SQLite: `write` alone does not build an index. let conn = rusqlite::Connection::open_in_memory().unwrap(); - execute_under_authorizer( - &conn, - "tinycloud.sql/write", - false, - "CREATE TABLE interaction (reader_did TEXT, nonce TEXT)", - ) - .unwrap(); + conn.execute_batch("CREATE TABLE interaction (reader_did TEXT, nonce TEXT)") + .unwrap(); - execute_under_authorizer( + let err = execute_under_authorizer( &conn, "tinycloud.sql/write", false, "CREATE UNIQUE INDEX uq_interaction_nonce ON interaction (reader_did, nonce)", ) - .expect("CREATE UNIQUE INDEX should succeed with a write cap"); + .expect_err("CREATE UNIQUE INDEX must be denied for a write cap"); + assert!(err.to_string().contains("not authorized"), "{err}"); } #[test] @@ -760,16 +881,11 @@ mod tests { #[test] fn create_unique_index_blocked_with_read_only_cap() { - // Set up the table with a write cap, then attempt the index under a + // Set up the table directly, then attempt the index under a // read-only cap: SQLite must report "not authorized". let conn = rusqlite::Connection::open_in_memory().unwrap(); - execute_under_authorizer( - &conn, - "tinycloud.sql/write", - false, - "CREATE TABLE interaction (reader_did TEXT, nonce TEXT)", - ) - .unwrap(); + conn.execute_batch("CREATE TABLE interaction (reader_did TEXT, nonce TEXT)") + .unwrap(); let err = execute_under_authorizer( &conn, @@ -788,7 +904,6 @@ mod tests { fn drop_table_obeys_schema_authority() { for (ability, is_admin) in [ ("tinycloud.sql/schema", false), - ("tinycloud.sql/write", false), ("tinycloud.sql/admin", true), ] { let conn = rusqlite::Connection::open_in_memory().unwrap(); @@ -1255,3 +1370,719 @@ mod tests { ); } } + +/// TC-1581: schema-changing statements need `schema`; `write` is data only. +#[cfg(test)] +mod ddl_authority_tests { + use super::*; + + fn authorize(ability: &str, is_admin: bool, action: AuthAction<'_>) -> Authorization { + let mut auth = create_authorizer_scoped( + None, + &authority_for(&[ability], is_admin), + StatementScope::default(), + ); + auth(AuthContext { + action, + database_name: Some("main"), + accessor: None, + }) + } + + fn execute_under_authorizer( + conn: &rusqlite::Connection, + ability: &str, + is_admin: bool, + sql: &str, + ) -> rusqlite::Result<()> { + execute_under_authority(conn, &authority_for(&[ability], is_admin), sql) + } + + fn authority_for(abilities: &[&str], is_admin: bool) -> SqlAuthority { + let mut held: Vec = abilities.iter().map(|a| a.to_string()).collect(); + if is_admin { + held.push("tinycloud.sql/admin".to_string()); + } + SqlAuthority::new(held) + } + + fn authorize_held(abilities: &[&str], action: AuthAction<'_>) -> Authorization { + let mut auth = create_authorizer_scoped( + None, + &authority_for(abilities, false), + StatementScope::default(), + ); + auth(AuthContext { + action, + database_name: Some("main"), + accessor: None, + }) + } + + #[test] + fn create_index_needs_schema_authority_not_write() { + // CREATE [UNIQUE] INDEX fires CreateIndex followed by Reindex while the + // index is built; both must pass for the statement to succeed. + let create_index = || AuthAction::CreateIndex { + index_name: "uq_interaction_nonce", + table_name: "interaction", + }; + let reindex = || AuthAction::Reindex { + index_name: "uq_interaction_nonce", + }; + + // TC-1581: `write` no longer runs schema-changing actions. + assert_eq!( + authorize("tinycloud.sql/write", false, create_index()), + Authorization::Deny + ); + assert_eq!( + authorize("tinycloud.sql/write", false, reindex()), + Authorization::Deny + ); + // A holder of both abilities is unaffected. + let both = ["tinycloud.sql/write", "tinycloud.sql/schema"]; + assert_eq!(authorize_held(&both, create_index()), Authorization::Allow); + assert_eq!(authorize_held(&both, reindex()), Authorization::Allow); + } + + /// Install the authorizer on a real rusqlite connection (mirroring the + /// wiring in database.rs) and run `sql` under the given abilities. The + /// statement scope comes from the parser, as in `handle_message`. + fn execute_under_held( + conn: &rusqlite::Connection, + abilities: &[&str], + sql: &str, + ) -> rusqlite::Result<()> { + execute_under_authority(conn, &authority_for(abilities, false), sql) + } + + fn execute_under_authority( + conn: &rusqlite::Connection, + authority: &SqlAuthority, + sql: &str, + ) -> rusqlite::Result<()> { + let scope = crate::sql::parser::classify_sql(sql) + .map(|class| StatementScope { + is_ddl: class.is_ddl, + is_read_only: class.is_read_only, + }) + .unwrap_or_default(); + conn.authorizer(Some(create_authorizer_scoped(None, authority, scope))); + let result = conn.execute_batch(sql); + conn.authorizer(None::) -> Authorization>); + result + } + + #[test] + fn create_unique_index_executes_with_write_and_schema() { + // End-to-end against real SQLite: the CreateIndex -> Reindex callback + // sequence fired while building the index must pass for a holder of + // both abilities (the SDK's `[write, schema]` invocation). + let both = ["tinycloud.sql/write", "tinycloud.sql/schema"]; + let conn = rusqlite::Connection::open_in_memory().unwrap(); + execute_under_held( + &conn, + &both, + "CREATE TABLE interaction (reader_did TEXT, nonce TEXT)", + ) + .unwrap(); + + execute_under_held( + &conn, + &both, + "CREATE UNIQUE INDEX uq_interaction_nonce ON interaction (reader_did, nonce)", + ) + .expect("CREATE UNIQUE INDEX should succeed with write and schema"); + } + + #[test] + fn create_unique_index_blocked_without_schema_authority() { + // Set up the table directly, then attempt the index under a read-only + // cap and under write alone: SQLite must report "not authorized". + for ability in ["tinycloud.sql/read", "tinycloud.sql/write"] { + let conn = rusqlite::Connection::open_in_memory().unwrap(); + conn.execute_batch("CREATE TABLE interaction (reader_did TEXT, nonce TEXT)") + .unwrap(); + + let err = execute_under_authorizer( + &conn, + ability, + false, + "CREATE UNIQUE INDEX uq_interaction_nonce ON interaction (reader_did, nonce)", + ) + .expect_err("CREATE UNIQUE INDEX must be denied without schema authority"); + assert!( + err.to_string().contains("not authorized"), + "expected an authorization error for {ability}, got: {err}" + ); + } + } + + const WRITE: &str = "tinycloud.sql/write"; + const SCHEMA: &str = "tinycloud.sql/schema"; + const READ: &str = "tinycloud.sql/read"; + + /// One SQLite action per schema-changing callback the authorizer gates. + fn schema_changing_actions() -> Vec> { + vec![ + AuthAction::CreateTable { table_name: "t" }, + AuthAction::CreateTempTable { table_name: "t" }, + AuthAction::CreateIndex { + index_name: "i", + table_name: "t", + }, + AuthAction::CreateTempIndex { + index_name: "i", + table_name: "t", + }, + AuthAction::CreateView { view_name: "v" }, + AuthAction::CreateTempView { view_name: "v" }, + AuthAction::DropTable { table_name: "t" }, + AuthAction::DropTempTable { table_name: "t" }, + AuthAction::DropIndex { + index_name: "i", + table_name: "t", + }, + AuthAction::DropTempIndex { + index_name: "i", + table_name: "t", + }, + AuthAction::DropView { view_name: "v" }, + AuthAction::DropTempView { view_name: "v" }, + AuthAction::AlterTable { + database_name: "main", + table_name: "t", + }, + AuthAction::DropTrigger { + trigger_name: "g", + table_name: "t", + }, + AuthAction::DropTempTrigger { + trigger_name: "g", + table_name: "t", + }, + AuthAction::Reindex { index_name: "i" }, + ] + } + + #[test] + fn every_schema_changing_action_needs_a_schema_conferring_ability() { + for (abilities, expected) in [ + (&[WRITE][..], Authorization::Deny), + (&[READ][..], Authorization::Deny), + (&[READ, WRITE][..], Authorization::Deny), + (&["tinycloud.sql/ddl"][..], Authorization::Deny), + (&[SCHEMA][..], Authorization::Allow), + (&[WRITE, SCHEMA][..], Authorization::Allow), + (&["tinycloud.sql/admin"][..], Authorization::Allow), + (&["tinycloud.sql/*"][..], Authorization::Allow), + ] { + for action in schema_changing_actions() { + let label = format!("{abilities:?} {action:?}"); + assert_eq!(authorize_held(abilities, action), expected, "{label}"); + } + } + } + + #[test] + fn the_authorizer_denies_create_trigger_for_every_ability() { + for abilities in [ + &[WRITE][..], + &[SCHEMA][..], + &[WRITE, SCHEMA][..], + &["tinycloud.sql/admin"][..], + &["tinycloud.sql/*"][..], + ] { + for action in [ + AuthAction::CreateTrigger { + trigger_name: "g", + table_name: "t", + }, + AuthAction::CreateTempTrigger { + trigger_name: "g", + table_name: "t", + }, + ] { + assert_eq!( + authorize_held(abilities, action), + Authorization::Deny, + "{abilities:?}" + ); + } + } + + // End to end: SQLite really calls the authorizer for CREATE TRIGGER, + // so a statement the parser could one day accept is still refused. + for abilities in [&[WRITE, SCHEMA][..], &["tinycloud.sql/admin"][..]] { + let conn = rusqlite::Connection::open_in_memory().unwrap(); + conn.execute_batch("CREATE TABLE t (a); CREATE TABLE log (a)") + .unwrap(); + let error = execute_under_held( + &conn, + abilities, + "CREATE TRIGGER g AFTER INSERT ON t BEGIN INSERT INTO log VALUES (1); END", + ) + .expect_err("CREATE TRIGGER must be denied"); + assert!(error.to_string().contains("not authorized"), "{error}"); + let triggers: i64 = conn + .query_row( + "SELECT count(*) FROM sqlite_schema WHERE type = 'trigger'", + [], + |row| row.get(0), + ) + .unwrap(); + assert_eq!(triggers, 0); + } + } + + #[test] + fn a_misclassifying_parser_cannot_give_write_the_schema_actions() { + // Defence in depth: hand the authorizer a scope in which the parser + // called every statement plain data (or read-only). Write alone must + // still be refused by SQLite's own callbacks, and nothing may change. + let setup = "CREATE TABLE t (a INTEGER PRIMARY KEY, b TEXT); + CREATE VIEW v AS SELECT a FROM t; + CREATE INDEX ix ON t (b); + INSERT INTO t VALUES (1, 'x')"; + let attacks = [ + "CREATE TABLE made (a)", + "CREATE TEMP TABLE made (a)", + "CREATE TABLE made AS SELECT a FROM t", + "CREATE VIEW w AS SELECT a FROM t", + "CREATE INDEX iy ON t (a)", + "ALTER TABLE t ADD COLUMN c", + "ALTER TABLE t RENAME TO renamed", + "DROP TABLE t", + "DROP VIEW v", + "DROP INDEX ix", + ]; + for scope in [ + StatementScope::default(), + StatementScope { + is_ddl: false, + is_read_only: true, + }, + ] { + for sql in attacks { + let conn = rusqlite::Connection::open_in_memory().unwrap(); + conn.execute_batch(setup).unwrap(); + let before: String = conn + .query_row( + "SELECT group_concat(name) FROM sqlite_schema ORDER BY name", + [], + |row| row.get(0), + ) + .unwrap(); + + conn.authorizer(Some(create_authorizer_scoped( + None, + &authority_for(&[WRITE], false), + scope, + ))); + let result = conn.execute_batch(sql); + conn.authorizer(None::) -> Authorization>); + let error = result.expect_err(sql); + assert!( + error.to_string().contains("not authorized"), + "{sql}: {error}" + ); + + let after: String = conn + .query_row( + "SELECT group_concat(name) FROM sqlite_schema ORDER BY name", + [], + |row| row.get(0), + ) + .unwrap(); + assert_eq!(before, after, "{sql} changed the schema"); + let rows: i64 = conn + .query_row("SELECT count(*) FROM t", [], |row| row.get(0)) + .unwrap_or(1); + assert_eq!(rows, 1, "{sql} changed the data"); + } + } + } + + #[test] + fn write_alone_still_runs_data_statements_end_to_end() { + let conn = rusqlite::Connection::open_in_memory().unwrap(); + conn.execute_batch("CREATE TABLE t (a INTEGER PRIMARY KEY AUTOINCREMENT, b TEXT)") + .unwrap(); + for sql in [ + "INSERT INTO t (b) VALUES ('one')", + "INSERT OR REPLACE INTO t (a, b) VALUES (1, 'uno')", + "UPDATE t SET b = 'two' WHERE a = 1", + "WITH c AS (SELECT 'three' AS b) INSERT INTO t (b) SELECT b FROM c", + "SELECT a, b FROM t", + "DELETE FROM t WHERE a = 1", + ] { + execute_under_held(&conn, &[WRITE], sql) + .unwrap_or_else(|error| panic!("write must run {sql}: {error}")); + } + let rows: i64 = conn + .query_row("SELECT count(*) FROM t", [], |row| row.get(0)) + .unwrap(); + assert_eq!(rows, 1); + } + + #[test] + fn write_and_schema_together_run_schema_changes_with_write_tier_reads() { + // The SDK's `[write, schema]` invocation: DDL runs under the holder's + // data authority too, so CREATE TABLE AS SELECT keeps working. + let both = [WRITE, SCHEMA]; + let conn = rusqlite::Connection::open_in_memory().unwrap(); + conn.execute_batch("CREATE TABLE source (id INTEGER); INSERT INTO source VALUES (1)") + .unwrap(); + for sql in [ + "CREATE TABLE copied AS SELECT id FROM source", + "CREATE INDEX copied_id ON copied (id)", + "ALTER TABLE copied ADD COLUMN extra TEXT", + "CREATE VIEW copied_ids AS SELECT id FROM copied", + "DROP VIEW copied_ids", + "DROP TABLE copied", + ] { + execute_under_held(&conn, &both, sql) + .unwrap_or_else(|error| panic!("{both:?} must run {sql}: {error}")); + } + } + + #[test] + fn log_and_off_policies_keep_write_able_to_change_the_schema() { + use crate::sql::authority::DdlPolicy; + for policy in [DdlPolicy::Log, DdlPolicy::Off] { + let authority = authority_for(&[WRITE], false).with_ddl_policy(policy); + let conn = rusqlite::Connection::open_in_memory().unwrap(); + for sql in [ + "CREATE TABLE t (a)", + "ALTER TABLE t ADD COLUMN b", + "DROP TABLE t", + ] { + execute_under_authority(&conn, &authority, sql) + .unwrap_or_else(|error| panic!("{policy:?} must run {sql}: {error}")); + } + // Read still cannot. + let read = authority_for(&[READ], false).with_ddl_policy(policy); + assert!(execute_under_authority(&conn, &read, "CREATE TABLE u (a)").is_err()); + } + } + + const SCHEMA_TABLES: [&str; 4] = [ + "sqlite_master", + "sqlite_schema", + "sqlite_temp_master", + "sqlite_temp_schema", + ]; + + fn schema_table_writes(table: &'static str) -> [AuthAction<'static>; 3] { + [ + AuthAction::Insert { table_name: table }, + AuthAction::Update { + table_name: table, + column_name: "sql", + }, + AuthAction::Delete { table_name: table }, + ] + } + + fn authorize_scoped( + abilities: &[&str], + scope: StatementScope, + action: AuthAction<'_>, + ) -> Authorization { + let mut auth = create_authorizer_scoped(None, &authority_for(abilities, false), scope); + auth(AuthContext { + action, + database_name: Some("main"), + accessor: None, + }) + } + + /// TC-1586: `write` is a data ability. It does not write SQLite's own + /// schema tables, whatever the connection's `writable_schema` says. + #[test] + fn write_does_not_write_the_sqlite_schema_tables() { + let data = StatementScope { + is_ddl: false, + is_read_only: false, + }; + for table in SCHEMA_TABLES { + for abilities in [&[WRITE][..], &[READ, WRITE][..], &["tinycloud.sql/*"][..]] { + // `*` is admin-equivalent and keeps its authority. + let expected = if abilities == ["tinycloud.sql/*"] { + Authorization::Allow + } else { + Authorization::Deny + }; + for action in schema_table_writes(table) { + let label = format!("{abilities:?} {action:?}"); + assert_eq!( + authorize_scoped(abilities, data, action), + expected, + "{label}" + ); + } + } + // Application tables are unaffected. + assert_eq!( + authorize_scoped( + &[WRITE], + data, + AuthAction::Insert { + table_name: "notes" + } + ), + Authorization::Allow + ); + } + } + + #[test] + fn schema_changes_still_write_the_sqlite_schema_tables() { + let ddl = StatementScope { + is_ddl: true, + is_read_only: false, + }; + for table in SCHEMA_TABLES { + for abilities in [ + &[SCHEMA][..], + &[WRITE, SCHEMA][..], + &["tinycloud.sql/admin"][..], + ] { + for action in schema_table_writes(table) { + let label = format!("{abilities:?} {action:?}"); + assert_eq!( + authorize_scoped(abilities, ddl, action), + Authorization::Allow, + "{label}" + ); + } + } + // Write alone is refused the schema change itself, so it never + // reaches these callbacks with DDL authority. + for action in schema_table_writes(table) { + assert_eq!(authorize_scoped(&[WRITE], ddl, action), Authorization::Deny); + } + } + } + + /// With `writable_schema` ON a connection lets SQL edit `sqlite_master`. + /// The authorizer still refuses a data statement. + #[test] + fn write_cannot_edit_sqlite_master_even_when_the_connection_allows_it() { + for sql in [ + "UPDATE sqlite_master SET name = 'renamed' WHERE name = 'items'", + "UPDATE sqlite_master SET sql = 'CREATE TABLE items (id)' WHERE name = 'items'", + "DELETE FROM sqlite_master WHERE name = 'items'", + "INSERT INTO sqlite_master (type, name, tbl_name, rootpage, sql) VALUES ('table', 'fake', 'fake', 0, 'CREATE TABLE fake (id)')", + ] { + for abilities in [&[WRITE][..], &[READ, WRITE][..]] { + let conn = rusqlite::Connection::open_in_memory().unwrap(); + conn.execute_batch( + "CREATE TABLE items (id INTEGER); INSERT INTO items VALUES (1); PRAGMA writable_schema = ON;", + ) + .unwrap(); + let before: String = conn + .query_row( + "SELECT group_concat(name || ':' || sql) FROM sqlite_schema", + [], + |row| row.get(0), + ) + .unwrap(); + + let error = execute_under_held(&conn, abilities, sql) + .expect_err("write must not edit sqlite_master"); + assert!(error.to_string().contains("not authorized"), "{sql}: {error}"); + + let after: String = conn + .query_row( + "SELECT group_concat(name || ':' || sql) FROM sqlite_schema", + [], + |row| row.get(0), + ) + .unwrap(); + assert_eq!(before, after, "{sql}"); + } + } + } + + /// A table-valued `pragma_*` function reads SQLite's schema through + /// callbacks on the schema tables the first time a connection uses it. + /// Closing the schema tables to data statements must not break it. + #[test] + fn pragma_table_functions_still_work_for_write() { + let conn = rusqlite::Connection::open_in_memory().unwrap(); + conn.execute_batch("CREATE TABLE items (id INTEGER, body TEXT)") + .unwrap(); + for abilities in [&[WRITE][..], &[READ][..], &[READ, WRITE][..]] { + execute_under_held( + &conn, + abilities, + "SELECT name FROM pragma_table_info('items')", + ) + .unwrap_or_else(|e| panic!("{abilities:?}: {e}")); + } + // Only the whitelisted PRAGMAs are reachable that way. + let error = execute_under_held(&conn, &[WRITE], "SELECT * FROM pragma_function_list") + .expect_err("a PRAGMA outside the whitelist stays denied"); + assert!(error.to_string().contains("not authorized"), "{error}"); + } + + /// An unregistered `tinycloud.sql/*` URN confers nothing: beside `schema` + /// the holder is still schema-only (no data reads or writes, no CTAS copy). + #[test] + fn an_unregistered_ability_beside_schema_keeps_the_schema_only_rules() { + const UNREGISTERED: &str = "tinycloud.sql/not-a-registered-ability"; + for abilities in [&[SCHEMA][..], &[SCHEMA, UNREGISTERED][..]] { + let conn = rusqlite::Connection::open_in_memory().unwrap(); + conn.execute_batch("CREATE TABLE source (id INTEGER); INSERT INTO source VALUES (1)") + .unwrap(); + for sql in [ + "INSERT INTO source VALUES (2)", + "UPDATE source SET id = 3", + "DELETE FROM source", + "SELECT id FROM source", + "CREATE TABLE copied AS SELECT id FROM source", + ] { + let error = execute_under_held(&conn, abilities, sql) + .expect_err("schema authority stays schema-only"); + let message = error.to_string(); + assert!( + message.contains("not authorized") || message.contains("is prohibited"), + "{abilities:?} {sql}: {error}" + ); + } + execute_under_held(&conn, abilities, "CREATE TABLE made (id INTEGER)").unwrap(); + } + } + + fn pragma_action(abilities: &[&str], name: &str, value: Option<&str>) -> (Authorization, bool) { + let tracker = PragmaTracker::default(); + let mut auth = create_authorizer_tracked( + None, + &authority_for(abilities, false), + StatementScope::default(), + tracker.clone(), + ); + let verdict = auth(AuthContext { + action: AuthAction::Pragma { + pragma_name: name, + pragma_value: value, + }, + database_name: None, + accessor: None, + }); + (verdict, tracker.load(Ordering::Relaxed)) + } + + #[test] + fn plain_schema_reports_are_open_and_not_tracked() { + for abilities in [&[WRITE][..], &[READ][..], &["tinycloud.sql/admin"][..]] { + for name in ["table_info", "TABLE_INFO", "index_list", "foreign_key_list"] { + assert_eq!( + pragma_action(abilities, name, Some("t")), + (Authorization::Allow, false), + "{abilities:?} {name}" + ); + } + } + } + + #[test] + fn an_admin_may_run_only_the_allow_listed_pragmas() { + for abilities in [&["tinycloud.sql/admin"][..], &["tinycloud.sql/*"][..]] { + // Header values, checks and reports: allowed, and tracked so the + // connection is reset after the request. + for (name, value) in [ + ("user_version", Some("3")), + ("USER_VERSION", None), + ("application_id", Some("1")), + ("integrity_check", Some("10")), + ("quick_check", None), + ("foreign_key_check", Some("t")), + ("journal_mode", None), + ("foreign_keys", None), + ] { + assert_eq!( + pragma_action(abilities, name, value), + (Authorization::Allow, true), + "{abilities:?} {name}" + ); + } + // Anything that would set connection-level or process-wide + // state, or run maintenance, is refused. + for (name, value) in [ + ("writable_schema", Some("ON")), + ("journal_mode", Some("DELETE")), + ("foreign_keys", Some("OFF")), + ("query_only", Some("1")), + ("max_page_count", Some("1")), + ("cache_spill", Some("0")), + ("locking_mode", Some("EXCLUSIVE")), + ("synchronous", Some("OFF")), + ("soft_heap_limit", Some("1")), + ("hard_heap_limit", Some("1")), + ("temp_store_directory", Some("x")), + ("wal_checkpoint", Some("TRUNCATE")), + ("wal_checkpoint", None), + ("incremental_vacuum", None), + ("optimize", None), + ("not_a_pragma", None), + ] { + assert_eq!( + pragma_action(abilities, name, value), + (Authorization::Deny, false), + "{abilities:?} {name} {value:?}" + ); + } + } + } + + #[test] + fn only_an_admin_runs_pragmas_beyond_the_schema_reports() { + for abilities in [ + &[WRITE][..], + &[SCHEMA][..], + &[WRITE, SCHEMA][..], + &[READ][..], + ] { + for (name, value) in [("user_version", Some("3")), ("journal_mode", None)] { + assert_eq!( + pragma_action(abilities, name, value), + (Authorization::Deny, false), + "{abilities:?} {name}" + ); + } + } + } + + /// A pure query gets only the one schema-table UPDATE SQLite makes for a + /// `pragma_*` function; it never gets INSERT or DELETE there. + #[test] + fn a_read_only_statement_may_only_update_the_schema_tables() { + let read = StatementScope { + is_ddl: false, + is_read_only: true, + }; + for table in SCHEMA_TABLES { + for abilities in [&[READ][..], &[WRITE][..], &[READ, WRITE][..]] { + let [insert, update, delete] = schema_table_writes(table); + assert_eq!( + authorize_scoped(abilities, read, update), + Authorization::Allow, + "{abilities:?} {table}" + ); + assert_eq!( + authorize_scoped(abilities, read, insert), + Authorization::Deny, + "{abilities:?} {table}" + ); + assert_eq!( + authorize_scoped(abilities, read, delete), + Authorization::Deny, + "{abilities:?} {table}" + ); + } + } + } +} diff --git a/tinycloud-core/src/sql/database.rs b/tinycloud-core/src/sql/database.rs index 56d8a657..1efdc553 100644 --- a/tinycloud-core/src/sql/database.rs +++ b/tinycloud-core/src/sql/database.rs @@ -16,6 +16,8 @@ use tokio::sync::{mpsc, oneshot}; use crate::request_telemetry::{self, QueryTimer, RequestContext}; use super::{ + authority::{record_ddl_without_schema, SqlAuthority}, + authorizer::{PragmaTracker, StatementScope}, caveats::SqlCaveats, parser, storage::{self, StorageMode}, @@ -35,7 +37,7 @@ enum DbMessage { Execute { request: SqlRequest, caveats: Option, - ability: String, + authority: SqlAuthority, growth: GrowthPolicy, response_tx: oneshot::Sender>, }, @@ -114,6 +116,19 @@ impl DatabaseHandle { caveats: Option, ability: String, growth: GrowthPolicy, + ) -> Result { + self.execute_authorized(request, caveats, SqlAuthority::from(ability), growth) + .await + } + + /// As [`Self::execute`], judging each statement against every SQL ability + /// the invocation holds (TC-1581). + pub async fn execute_authorized( + &self, + request: SqlRequest, + caveats: Option, + authority: SqlAuthority, + growth: GrowthPolicy, ) -> Result { let (response_tx, response_rx) = oneshot::channel(); self.tx @@ -122,7 +137,7 @@ impl DatabaseHandle { DbMessage::Execute { request, caveats, - ability, + authority, growth, response_tx, }, @@ -240,18 +255,43 @@ pub fn spawn_actor( DbMessage::Execute { request, caveats, - ability, + authority, growth, response_tx, } => { + let pragmas = PragmaTracker::default(); let result = match growth { - GrowthPolicy::Allow => handle_message(&conn, &request, &caveats, &ability), - GrowthPolicy::Forbid => { - handle_message_without_growth(&conn, &request, &caveats, &ability) - } + GrowthPolicy::Allow => handle_message_authorized( + &conn, &request, &caveats, &authority, &pragmas, + ), + GrowthPolicy::Forbid => handle_message_without_growth_authorized( + &conn, &request, &caveats, &authority, &pragmas, + ), }; conn.authorizer(None::) -> Authorization>); + // TC-1586: the connection is shared by every request on + // this database. After any request in which the + // authorizer allowed a PRAGMA outside the plain schema + // reports, reset it; if that fails, serve no further + // request on this connection (the next one rebuilds it). + if pragmas.load(Ordering::Relaxed) + && connection_reset_failed(&conn, &mode, &space_id, &db_name) + { + tracing::error!( + "SQL connection state could not be reset; retiring the database actor" + ); + // Deregister first, so the request that follows the + // error builds a new actor instead of finding this one. + databases.remove_if(&(space_id.clone(), db_name.clone()), |_, handle| { + handle.id == id + }); + let _ = response_tx.send(Err(SqlError::Internal( + "SQL connection state could not be reset".into(), + ))); + break None; + } + // Post-write promotion check if result.is_ok() && matches!(mode, StorageMode::InMemory) { if let Ok(size) = storage::database_size(&conn) { @@ -381,11 +421,28 @@ const GUARD_UNDO: &str = "ROLLBACK TO tinycloud_storage_guard; RELEASE tinycloud /// database did not grow (`PRAGMA page_count` reflects uncommitted pages on /// this connection). Every failed guarded request is rolled back, including /// schema entries and batch statements that ran before a later rejection. +#[cfg(test)] fn handle_message_without_growth( conn: &rusqlite::Connection, request: &SqlRequest, caveats: &Option, ability: &str, +) -> Result { + handle_message_without_growth_authorized( + conn, + request, + caveats, + &SqlAuthority::from(ability), + &PragmaTracker::default(), + ) +} + +fn handle_message_without_growth_authorized( + conn: &rusqlite::Connection, + request: &SqlRequest, + caveats: &Option, + authority: &SqlAuthority, + pragmas: &PragmaTracker, ) -> Result { let before = page_count(conn)?; { @@ -393,7 +450,7 @@ fn handle_message_without_growth( conn.execute_batch(GUARD_BEGIN) .map_err(|e| SqlError::Internal(e.to_string()))?; } - let result = handle_message(conn, request, caveats, ability); + let result = handle_message_authorized(conn, request, caveats, authority, pragmas); // An early error return can leave the request's authorizer installed. conn.authorizer(None::) -> Authorization>); // ROLLBACK conflict handling can already have undone the transaction. @@ -433,15 +490,60 @@ fn page_count(conn: &rusqlite::Connection) -> Result { .map_err(|e| SqlError::Internal(e.to_string())) } +/// `DdlPolicy::Log` bookkeeping for one request: count every statement that +/// only `write` let through, and warn once. The message is constant and the +/// counter is labelled by statement kind alone, so neither carries SQL text, +/// space ids, DIDs or table names. +#[derive(Default)] +struct DdlLog { + warned: bool, +} + +impl DdlLog { + fn note(&mut self, parsed: &parser::ParsedQuery) { + let Some(kind) = parsed.ddl_without_schema else { + return; + }; + record_ddl_without_schema(kind); + if !self.warned { + self.warned = true; + tracing::warn!("sql ddl allowed without schema ability (ddl_requires_schema = log)"); + } + } +} + +#[cfg(test)] fn handle_message( conn: &rusqlite::Connection, request: &SqlRequest, caveats: &Option, ability: &str, ) -> Result { - // TC-119: confers-admin gate (registry-aware). `sql/admin` and `sql/*` - // (implies admin) pass; identical to the prior `admin | *` match. - let is_admin = crate::policy_capability::ability_matches(ability, "tinycloud.sql/admin"); + handle_message_authorized( + conn, + request, + caveats, + &SqlAuthority::from(ability), + &PragmaTracker::default(), + ) +} + +fn handle_message_authorized( + conn: &rusqlite::Connection, + request: &SqlRequest, + caveats: &Option, + authority: &SqlAuthority, + pragmas: &PragmaTracker, +) -> Result { + let mut ddl_log = DdlLog::default(); + let authorizer = |parsed: &parser::ParsedQuery| { + super::authorizer::create_authorizer_tracked( + caveats.clone(), + authority, + StatementScope::of(parsed), + pragmas.clone(), + ) + }; match request { SqlRequest::Query { @@ -450,14 +552,10 @@ fn handle_message( max_rows, max_bytes, } => { - let parsed = parser::validate_sql(sql, caveats, ability)?; + let parsed = parser::validate_sql_authorized(sql, caveats, authority)?; + ddl_log.note(&parsed); - let auth = super::authorizer::create_authorizer( - caveats.clone(), - ability.to_string(), - is_admin, - ); - conn.authorizer(Some(auth)); + conn.authorizer(Some(authorizer(&parsed))); let result = execute_query(conn, sql, params, *max_rows, *max_bytes); @@ -474,17 +572,14 @@ fn handle_message( schema, } => { let mut write_targets = Vec::new(); - // Schema init + // Schema init. Each entry is judged like any other statement: a + // schema-changing entry needs a schema-conferring ability. if let Some(schema_stmts) = schema { for stmt_sql in schema_stmts { - let parsed = parser::validate_sql(stmt_sql, caveats, ability)?; - write_targets.extend(parsed.write_targets); - let auth = super::authorizer::create_authorizer( - caveats.clone(), - ability.to_string(), - is_admin, - ); - conn.authorizer(Some(auth)); + let parsed = parser::validate_sql_authorized(stmt_sql, caveats, authority)?; + ddl_log.note(&parsed); + write_targets.extend(parsed.write_targets.iter().cloned()); + conn.authorizer(Some(authorizer(&parsed))); { let _timer = QueryTimer::new(); conn.execute_batch(stmt_sql) @@ -494,13 +589,9 @@ fn handle_message( } } - let parsed = parser::validate_sql(sql, caveats, ability)?; - let auth = super::authorizer::create_authorizer( - caveats.clone(), - ability.to_string(), - is_admin, - ); - conn.authorizer(Some(auth)); + let parsed = parser::validate_sql_authorized(sql, caveats, authority)?; + ddl_log.note(&parsed); + conn.authorizer(Some(authorizer(&parsed))); let result = execute_statement(conn, sql, params, is_insert_statement(&parsed)); @@ -516,21 +607,25 @@ fn handle_message( SqlRequest::Batch { statements } => { let mut write_targets = Vec::new(); let mut insert_statements = Vec::with_capacity(statements.len()); + let mut scopes = Vec::with_capacity(statements.len()); // Hooks are emitted only after this branch returns Ok to the caller. // If a later statement fails after some earlier statements applied, // MVP intentionally under-emits rather than guessing partial success. for stmt in statements { - let parsed = parser::validate_sql(&stmt.sql, caveats, ability)?; + let parsed = parser::validate_sql_authorized(&stmt.sql, caveats, authority)?; + ddl_log.note(&parsed); insert_statements.push(is_insert_statement(&parsed)); + scopes.push(StatementScope::of(&parsed)); write_targets.extend(parsed.write_targets); } let mut results = Vec::new(); - for (stmt, is_insert) in statements.iter().zip(insert_statements) { - let auth = super::authorizer::create_authorizer( + for ((stmt, is_insert), scope) in statements.iter().zip(insert_statements).zip(scopes) { + let auth = super::authorizer::create_authorizer_tracked( caveats.clone(), - ability.to_string(), - is_admin, + authority, + scope, + pragmas.clone(), ); conn.authorizer(Some(auth)); let result = execute_statement(conn, &stmt.sql, &stmt.params, is_insert); @@ -551,14 +646,13 @@ fn handle_message( SqlError::InvalidStatement(format!("Statement '{}' not found", name)) })?; - let parsed = parser::validate_sql(&prepared.sql, caveats, ability)?; + // A named statement is judged as the SQL it holds: one that + // resolves to DDL needs a schema-conferring ability on the + // invocation, like the same SQL sent directly. + let parsed = parser::validate_sql_authorized(&prepared.sql, caveats, authority)?; + ddl_log.note(&parsed); - let auth = super::authorizer::create_authorizer( - caveats.clone(), - ability.to_string(), - is_admin, - ); - conn.authorizer(Some(auth)); + conn.authorizer(Some(authorizer(&parsed))); let result = if is_query_statement(&parsed) { execute_query(conn, &prepared.sql, params, None, None).map(SqlResponse::Query) @@ -591,10 +685,81 @@ fn row_to_sql_value(row: &rusqlite::Row, idx: usize) -> Result bool { - matches!( - parsed.statements.as_slice(), - [sqlparser::ast::Statement::Query(_)] + // A `WITH … INSERT/UPDATE` is a Query to the parser but runs as a write. + parsed.is_read_only + && matches!( + parsed.statements.as_slice(), + [sqlparser::ast::Statement::Query(_)] + ) +} + +/// Return the connection to the state `storage::open_connection` leaves it in +/// (TC-1586). +/// +/// The actor's connection is shared by every request on a database, so +/// connection-level state must not outlive the request that changed it. The +/// authorizer allows an admin only a short list of PRAGMAs, none of which +/// change connection state; this reset is the backstop for anything that +/// list misses. It covers in-memory and file-backed connections alike. +fn reset_connection_state(conn: &rusqlite::Connection, mode: &StorageMode) -> Result<(), SqlError> { + let _timer = QueryTimer::new(); + conn.execute_batch( + "PRAGMA writable_schema = OFF; + PRAGMA query_only = OFF; + PRAGMA ignore_check_constraints = OFF; + PRAGMA recursive_triggers = OFF; + PRAGMA case_sensitive_like = OFF; + PRAGMA defer_foreign_keys = OFF; + PRAGMA reverse_unordered_selects = OFF; + PRAGMA automatic_index = ON; + PRAGMA read_uncommitted = OFF; + PRAGMA cell_size_check = OFF; + PRAGMA legacy_alter_table = OFF; + PRAGMA locking_mode = NORMAL; + PRAGMA temp_store = DEFAULT; + PRAGMA foreign_keys = ON;", ) + .map_err(|e| SqlError::Internal(e.to_string()))?; + let (journal_mode, wal) = match mode { + StorageMode::File(_) => ("wal", true), + StorageMode::InMemory => ("memory", false), + }; + conn.pragma_update(None, "journal_mode", journal_mode) + .map_err(|e| SqlError::Internal(e.to_string()))?; + if wal { + conn.pragma_update(None, "wal_autocheckpoint", 0) + .map_err(|e| SqlError::Internal(e.to_string()))?; + } + Ok(()) +} + +/// Reset after a request that was allowed a PRAGMA. `true` when it failed, in +/// which case the actor must not serve another request on this connection. +fn connection_reset_failed( + conn: &rusqlite::Connection, + mode: &StorageMode, + space_id: &str, + db_name: &str, +) -> bool { + injected_reset_failure(space_id, db_name) || reset_connection_state(conn, mode).is_err() +} + +#[cfg(test)] +pub(crate) static RESET_FAILURES: std::sync::Mutex> = + std::sync::Mutex::new(Vec::new()); + +#[cfg(test)] +fn injected_reset_failure(space_id: &str, db_name: &str) -> bool { + RESET_FAILURES + .lock() + .unwrap() + .iter() + .any(|(space, db)| space == space_id && db == db_name) +} + +#[cfg(not(test))] +fn injected_reset_failure(_space_id: &str, _db_name: &str) -> bool { + false } fn is_insert_statement(parsed: &parser::ParsedQuery) -> bool { @@ -1216,3 +1381,656 @@ mod publication_rollback_tests { ); } } + +/// TC-1581: schema-changing statements need `schema`; `write` is data only. +/// Every request variant is judged statement by statement against all the +/// abilities held, not against one preferred ability. +#[cfg(test)] +mod ddl_authority_tests { + use super::*; + use crate::sql::authority::{ddl_without_schema_count, DdlKind, DdlPolicy}; + use std::sync::{Arc, Mutex}; + + const READ: &str = "tinycloud.sql/read"; + const WRITE: &str = "tinycloud.sql/write"; + const SCHEMA: &str = "tinycloud.sql/schema"; + const ADMIN: &str = "tinycloud.sql/admin"; + const STAR: &str = "tinycloud.sql/*"; + + fn held(abilities: &[&str]) -> SqlAuthority { + SqlAuthority::new(abilities.iter().copied()) + } + + fn execute(sql: &str, schema: Option<&[&str]>) -> SqlRequest { + SqlRequest::Execute { + sql: sql.to_string(), + params: vec![], + schema: schema.map(|s| s.iter().map(|s| s.to_string()).collect()), + } + } + + fn batch(statements: &[&str]) -> SqlRequest { + SqlRequest::Batch { + statements: statements + .iter() + .map(|sql| SqlStatement { + sql: sql.to_string(), + params: vec![], + }) + .collect(), + } + } + + fn query(sql: &str) -> SqlRequest { + SqlRequest::Query { + sql: sql.to_string(), + params: vec![], + max_rows: None, + max_bytes: None, + } + } + + fn caveats_with(statements: &[(&str, &str)]) -> Option { + Some(SqlCaveats { + statements: Some( + statements + .iter() + .map(|(name, sql)| crate::sql::caveats::PreparedStatement { + name: name.to_string(), + sql: sql.to_string(), + }) + .collect(), + ), + ..SqlCaveats::default() + }) + } + + fn run( + conn: &rusqlite::Connection, + request: &SqlRequest, + caveats: &Option, + authority: &SqlAuthority, + ) -> Result { + handle_message_authorized(conn, request, caveats, authority, &PragmaTracker::default()) + } + + fn table_exists(conn: &rusqlite::Connection, name: &str) -> bool { + conn.query_row( + "SELECT count(*) FROM sqlite_schema WHERE name = ?", + [name], + |row| row.get::<_, i64>(0), + ) + .unwrap() + > 0 + } + + fn is_denied(result: &Result) -> bool { + matches!( + result, + Err(SqlError::PermissionDenied(_)) | Err(SqlError::ReadOnlyViolation) + ) + } + + #[test] + fn write_alone_cannot_change_the_schema_on_any_request_path() { + let ddl = [ + "CREATE TABLE made (id INTEGER)", + "DROP TABLE data", + "ALTER TABLE data ADD COLUMN extra TEXT", + "CREATE INDEX data_value ON data (value)", + "CREATE VIEW data_view AS SELECT value FROM data", + ]; + for sql in ddl { + for abilities in [&[WRITE][..], &[READ, WRITE][..], &[READ][..]] { + let authority = held(abilities); + let conn = rusqlite::Connection::open_in_memory().unwrap(); + conn.execute_batch( + "CREATE TABLE data (value TEXT); INSERT INTO data VALUES ('kept')", + ) + .unwrap(); + let schema_before: i64 = conn + .query_row("SELECT count(*) FROM sqlite_schema", [], |row| row.get(0)) + .unwrap(); + + let named = caveats_with(&[("change", sql)]); + let attempts = [ + ("query", run(&conn, &query(sql), &None, &authority)), + ( + "execute", + run(&conn, &execute(sql, None), &None, &authority), + ), + ( + "execute schema[]", + run(&conn, &execute("SELECT 1", Some(&[sql])), &None, &authority), + ), + ( + "batch", + run( + &conn, + &batch(&["INSERT INTO data VALUES ('second')", sql]), + &None, + &authority, + ), + ), + ( + "named statement", + run( + &conn, + &SqlRequest::ExecuteStatement { + name: "change".into(), + params: vec![], + }, + &named, + &authority, + ), + ), + ]; + for (path, result) in attempts { + assert!(is_denied(&result), "{abilities:?} {path} {sql}: {result:?}"); + } + + let schema_after: i64 = conn + .query_row("SELECT count(*) FROM sqlite_schema", [], |row| row.get(0)) + .unwrap(); + assert_eq!( + schema_before, schema_after, + "{abilities:?} changed the schema: {sql}" + ); + let rows: i64 = conn + .query_row("SELECT count(*) FROM data", [], |row| row.get(0)) + .unwrap(); + assert_eq!( + rows, 1, + "a refused batch must not run its earlier statements" + ); + } + } + } + + #[test] + fn schema_conferring_abilities_run_ddl_on_every_request_path() { + for abilities in [ + &[SCHEMA][..], + &[WRITE, SCHEMA][..], + &[ADMIN][..], + &[STAR][..], + ] { + let authority = held(abilities); + let conn = rusqlite::Connection::open_in_memory().unwrap(); + let named = caveats_with(&[("mk", "CREATE TABLE IF NOT EXISTS named (id INTEGER)")]); + run( + &conn, + &query("CREATE TABLE via_query (id INTEGER)"), + &None, + &authority, + ) + .unwrap_or_else(|e| panic!("{abilities:?} query: {e:?}")); + run( + &conn, + &execute("CREATE TABLE via_execute (id INTEGER)", None), + &None, + &authority, + ) + .unwrap_or_else(|e| panic!("{abilities:?} execute: {e:?}")); + run( + &conn, + &batch(&["CREATE TABLE via_batch (id INTEGER)"]), + &None, + &authority, + ) + .unwrap_or_else(|e| panic!("{abilities:?} batch: {e:?}")); + run( + &conn, + &SqlRequest::ExecuteStatement { + name: "mk".into(), + params: vec![], + }, + &named, + &authority, + ) + .unwrap_or_else(|e| panic!("{abilities:?} named: {e:?}")); + for table in ["via_query", "via_execute", "via_batch", "named"] { + assert!(table_exists(&conn, table), "{abilities:?} {table}"); + } + run( + &conn, + &execute("DROP TABLE via_query", None), + &None, + &authority, + ) + .unwrap_or_else(|e| panic!("{abilities:?} drop: {e:?}")); + } + } + + #[test] + fn mixed_requests_need_both_abilities_or_admin() { + let create = "CREATE TABLE IF NOT EXISTS notes (id INTEGER PRIMARY KEY, body TEXT)"; + let insert = "INSERT INTO notes (body) VALUES ('hello')"; + for (abilities, expected_ok) in [ + (&[WRITE, SCHEMA][..], true), + (&[ADMIN][..], true), + (&[STAR][..], true), + (&[WRITE][..], false), + (&[SCHEMA][..], false), + (&[READ][..], false), + (&[READ, SCHEMA][..], false), + ] { + let authority = held(abilities); + + // execute { schema: [DDL], sql: DML }: the SDK's migration shape. + let conn = rusqlite::Connection::open_in_memory().unwrap(); + let result = run(&conn, &execute(insert, Some(&[create])), &None, &authority); + assert_eq!( + result.is_ok(), + expected_ok, + "{abilities:?} execute: {result:?}" + ); + + // batch [DDL, DML] is validated before anything runs. + let conn = rusqlite::Connection::open_in_memory().unwrap(); + let result = run(&conn, &batch(&[create, insert]), &None, &authority); + assert_eq!( + result.is_ok(), + expected_ok, + "{abilities:?} batch: {result:?}" + ); + if !expected_ok { + assert!( + !table_exists(&conn, "notes"), + "{abilities:?}: a refused batch ran its DDL" + ); + } + } + } + + #[test] + fn data_statements_still_run_on_write_alone() { + let conn = rusqlite::Connection::open_in_memory().unwrap(); + conn.execute_batch("CREATE TABLE data (id INTEGER PRIMARY KEY, value TEXT)") + .unwrap(); + let authority = held(&[WRITE]); + for sql in [ + "INSERT INTO data (value) VALUES ('a')", + "INSERT OR REPLACE INTO data (id, value) VALUES (1, 'b')", + "UPDATE data SET value = 'c' WHERE id = 1", + "WITH x AS (SELECT 'd' AS value) INSERT INTO data (value) SELECT value FROM x", + "DELETE FROM data WHERE id = 1", + ] { + run(&conn, &execute(sql, None), &None, &authority) + .unwrap_or_else(|e| panic!("write must run {sql}: {e:?}")); + } + run( + &conn, + &batch(&["INSERT INTO data (value) VALUES ('e')"]), + &None, + &authority, + ) + .unwrap(); + let read = run( + &conn, + &query("SELECT value FROM data ORDER BY id"), + &None, + &authority, + ) + .unwrap(); + let SqlResponse::Query(rows) = read.response else { + panic!("expected rows"); + }; + assert_eq!(rows.row_count, 2); + + // A named statement that is data runs on write alone as well. + let named = caveats_with(&[("put", "INSERT INTO data (value) VALUES ('f')")]); + run( + &conn, + &SqlRequest::ExecuteStatement { + name: "put".into(), + params: vec![], + }, + &named, + &authority, + ) + .unwrap(); + } + + #[test] + fn read_stays_read_only() { + let conn = rusqlite::Connection::open_in_memory().unwrap(); + conn.execute_batch("CREATE TABLE data (value TEXT)") + .unwrap(); + let authority = held(&[READ]); + run(&conn, &query("SELECT value FROM data"), &None, &authority).unwrap(); + let result = run( + &conn, + &execute("INSERT INTO data VALUES ('x')", None), + &None, + &authority, + ); + assert!( + matches!(result, Err(SqlError::ReadOnlyViolation)), + "{result:?}" + ); + } + + #[test] + fn pragma_is_unchanged_for_write_and_schema() { + let conn = rusqlite::Connection::open_in_memory().unwrap(); + for abilities in [&[WRITE][..], &[SCHEMA][..], &[WRITE, SCHEMA][..]] { + let authority = held(abilities); + for request in [ + query("PRAGMA user_version = 9"), + execute("PRAGMA user_version = 9", None), + execute("SELECT 1", Some(&["PRAGMA user_version = 9"])), + ] { + let result = run(&conn, &request, &None, &authority); + assert!(is_denied(&result), "{abilities:?}: {result:?}"); + } + } + for abilities in [&[ADMIN][..], &[STAR][..]] { + run( + &conn, + &execute("PRAGMA user_version = 3", None), + &None, + &held(abilities), + ) + .unwrap(); + } + let version: i64 = conn + .query_row("PRAGMA user_version", [], |row| row.get(0)) + .unwrap(); + assert_eq!(version, 3); + } + + #[test] + fn ddl_policy_switch_off_log_enforce() { + let _serial = crate::sql::authority::LOG_COUNTER_LOCK.blocking_lock(); + let create = "CREATE TABLE IF NOT EXISTS made (id INTEGER)"; + for (policy, write_only_may_ddl) in [ + (DdlPolicy::Off, true), + (DdlPolicy::Log, true), + (DdlPolicy::Enforce, false), + ] { + let conn = rusqlite::Connection::open_in_memory().unwrap(); + let write_only = held(&[WRITE]).with_ddl_policy(policy); + let result = run(&conn, &execute(create, None), &None, &write_only); + assert_eq!(result.is_ok(), write_only_may_ddl, "{policy:?}: {result:?}"); + assert_eq!( + table_exists(&conn, "made"), + write_only_may_ddl, + "{policy:?}" + ); + + // Holders of schema and read are judged the same in every mode. + let conn = rusqlite::Connection::open_in_memory().unwrap(); + let both = held(&[WRITE, SCHEMA]).with_ddl_policy(policy); + run(&conn, &execute(create, None), &None, &both).unwrap(); + let read = held(&[READ]).with_ddl_policy(policy); + assert!(is_denied(&run( + &conn, + &execute("DROP TABLE made", None), + &None, + &read + ))); + } + } + + #[derive(Clone, Default)] + struct Captured(Arc>>); + + impl std::io::Write for Captured { + fn write(&mut self, buf: &[u8]) -> std::io::Result { + self.0.lock().unwrap().extend_from_slice(buf); + Ok(buf.len()) + } + fn flush(&mut self) -> std::io::Result<()> { + Ok(()) + } + } + + impl<'a> tracing_subscriber::fmt::MakeWriter<'a> for Captured { + type Writer = Captured; + fn make_writer(&'a self) -> Self::Writer { + self.clone() + } + } + + #[test] + fn log_mode_counts_each_statement_and_warns_once_without_request_content() { + let _serial = crate::sql::authority::LOG_COUNTER_LOCK.blocking_lock(); + let captured = Captured::default(); + let subscriber = tracing_subscriber::fmt() + .with_writer(captured.clone()) + .with_ansi(false) + .finish(); + let _guard = tracing::subscriber::set_default(subscriber); + + let conn = rusqlite::Connection::open_in_memory().unwrap(); + let authority = held(&[WRITE]).with_ddl_policy(DdlPolicy::Log); + let secret_table = "tc1581_secret_table_name"; + let create_before = ddl_without_schema_count(DdlKind::CreateTable); + let drop_before = ddl_without_schema_count(DdlKind::Drop); + + run( + &conn, + &batch(&[ + &format!("CREATE TABLE {secret_table} (id INTEGER)"), + &format!("CREATE TABLE {secret_table}_two (id INTEGER)"), + &format!("DROP TABLE {secret_table}_two"), + &format!("INSERT INTO {secret_table} (id) VALUES (1)"), + ]), + &None, + &authority, + ) + .unwrap(); + + assert_eq!( + ddl_without_schema_count(DdlKind::CreateTable) - create_before, + 2 + ); + assert_eq!(ddl_without_schema_count(DdlKind::Drop) - drop_before, 1); + + let logged = String::from_utf8(captured.0.lock().unwrap().clone()).unwrap(); + assert_eq!( + logged.matches("ddl_requires_schema = log").count(), + 1, + "one warning per request: {logged}" + ); + assert!( + !logged.contains(secret_table), + "log carried request content: {logged}" + ); + assert!(!logged.to_lowercase().contains("create table"), "{logged}"); + + // A holder of schema is not counted, and data statements never are. + let before = ddl_without_schema_count(DdlKind::CreateTable); + let both = held(&[WRITE, SCHEMA]).with_ddl_policy(DdlPolicy::Log); + run( + &conn, + &execute("CREATE TABLE fine (id INTEGER)", None), + &None, + &both, + ) + .unwrap(); + run( + &conn, + &execute("INSERT INTO fine VALUES (1)", None), + &None, + &authority, + ) + .unwrap(); + assert_eq!(ddl_without_schema_count(DdlKind::CreateTable), before); + let after_logged = String::from_utf8(captured.0.lock().unwrap().clone()).unwrap(); + assert_eq!(after_logged.matches("ddl_requires_schema = log").count(), 1); + } + + #[test] + fn enforce_and_off_neither_count_nor_warn() { + let _serial = crate::sql::authority::LOG_COUNTER_LOCK.blocking_lock(); + let captured = Captured::default(); + let subscriber = tracing_subscriber::fmt() + .with_writer(captured.clone()) + .with_ansi(false) + .finish(); + let _guard = tracing::subscriber::set_default(subscriber); + let before = ddl_without_schema_count(DdlKind::AlterTable); + + let conn = rusqlite::Connection::open_in_memory().unwrap(); + conn.execute_batch("CREATE TABLE t (a)").unwrap(); + let off = held(&[WRITE]).with_ddl_policy(DdlPolicy::Off); + run( + &conn, + &execute("ALTER TABLE t ADD COLUMN b", None), + &None, + &off, + ) + .unwrap(); + let enforce = held(&[WRITE]).with_ddl_policy(DdlPolicy::Enforce); + let denied = run( + &conn, + &execute("ALTER TABLE t ADD COLUMN c", None), + &None, + &enforce, + ); + assert!(is_denied(&denied)); + + assert_eq!(ddl_without_schema_count(DdlKind::AlterTable), before); + assert!(captured.0.lock().unwrap().is_empty()); + } + + #[test] + fn an_unregistered_ability_beside_schema_runs_no_data_statement() { + const UNREGISTERED: &str = "tinycloud.sql/not-a-registered-ability"; + let conn = rusqlite::Connection::open_in_memory().unwrap(); + conn.execute_batch("CREATE TABLE data (value TEXT)") + .unwrap(); + for abilities in [&[SCHEMA][..], &[SCHEMA, UNREGISTERED][..]] { + let authority = held(abilities); + for request in [ + query("SELECT value FROM data"), + execute("INSERT INTO data VALUES ('x')", None), + batch(&["DELETE FROM data"]), + execute( + "SELECT 1", + Some(&["CREATE TABLE IF NOT EXISTS made (id INTEGER)"]), + ), + ] { + let result = run(&conn, &request, &None, &authority); + // The schema entry of the last request is the one statement + // schema authority may run; its trailing SELECT is refused. + assert!(is_denied(&result), "{abilities:?} {request:?}: {result:?}"); + } + } + } + + /// TC-1586: the reset returns a connection to the state `open_connection` + /// leaves it in, for an in-memory connection (journal mode included) and a + /// file-backed one. + #[test] + fn reset_restores_the_opening_state_for_memory_and_file_connections() { + let dir = tempfile::TempDir::new().unwrap(); + for mode in [ + StorageMode::InMemory, + StorageMode::File(dir.path().join("reset.db")), + ] { + let conn = storage::open_connection(&mode).unwrap(); + conn.execute_batch("CREATE TABLE t (a)").unwrap(); + let read = |pragma: &str| -> String { + conn.query_row(&format!("PRAGMA {pragma}"), [], |row| { + row.get::<_, rusqlite::types::Value>(0) + .map(|value| match value { + rusqlite::types::Value::Integer(n) => n.to_string(), + rusqlite::types::Value::Text(s) => s, + other => format!("{other:?}"), + }) + }) + .unwrap() + }; + let opening: Vec = [ + "journal_mode", + "foreign_keys", + "writable_schema", + "query_only", + "ignore_check_constraints", + "recursive_triggers", + "reverse_unordered_selects", + ] + .iter() + .map(|pragma| read(pragma)) + .collect(); + + let other_journal_mode = if matches!(mode, StorageMode::InMemory) { + "OFF" + } else { + "DELETE" + }; + conn.execute_batch(&format!( + "PRAGMA writable_schema = ON; PRAGMA query_only = ON; + PRAGMA foreign_keys = OFF; PRAGMA ignore_check_constraints = ON; + PRAGMA recursive_triggers = ON; PRAGMA reverse_unordered_selects = ON; + PRAGMA journal_mode = {other_journal_mode};" + )) + .unwrap(); + assert_ne!(read("journal_mode"), opening[0], "the test must change it"); + assert_eq!(read("foreign_keys"), "0"); + + reset_connection_state(&conn, &mode).unwrap(); + let after: Vec = [ + "journal_mode", + "foreign_keys", + "writable_schema", + "query_only", + "ignore_check_constraints", + "recursive_triggers", + "reverse_unordered_selects", + ] + .iter() + .map(|pragma| read(pragma)) + .collect(); + assert_eq!(opening, after, "{mode:?}"); + } + } + + #[test] + fn nested_block_comments_hide_nothing_on_any_request_path() { + let hidden = + "CREATE TABLE IF NOT EXISTS made (id INTEGER) /* /* */ ; DELETE FROM data; /* */"; + for abilities in [&[SCHEMA][..], &[WRITE, SCHEMA][..], &[ADMIN][..]] { + let authority = held(abilities); + let conn = rusqlite::Connection::open_in_memory().unwrap(); + conn.execute_batch("CREATE TABLE data (value TEXT); INSERT INTO data VALUES ('kept')") + .unwrap(); + let named = caveats_with(&[("change", hidden)]); + for (label, result) in [ + ("query", run(&conn, &query(hidden), &None, &authority)), + ( + "schema entry", + run( + &conn, + &execute("SELECT 1", Some(&[hidden])), + &None, + &authority, + ), + ), + ("batch", run(&conn, &batch(&[hidden]), &None, &authority)), + ( + "named", + run( + &conn, + &SqlRequest::ExecuteStatement { + name: "change".into(), + params: vec![], + }, + &named, + &authority, + ), + ), + ] { + assert!(result.is_err(), "{abilities:?} {label}: {result:?}"); + } + let rows: i64 = conn + .query_row("SELECT count(*) FROM data", [], |row| row.get(0)) + .unwrap(); + assert_eq!(rows, 1, "{abilities:?}"); + assert!(!table_exists(&conn, "made"), "{abilities:?}"); + } + } +} diff --git a/tinycloud-core/src/sql/mod.rs b/tinycloud-core/src/sql/mod.rs index 7e755e61..94b8c016 100644 --- a/tinycloud-core/src/sql/mod.rs +++ b/tinycloud-core/src/sql/mod.rs @@ -1,3 +1,4 @@ +pub mod authority; pub mod authorizer; pub mod caveats; pub mod database; @@ -6,6 +7,7 @@ pub mod service; pub mod storage; pub mod types; +pub use authority::{DdlPolicy, SqlAuthority}; pub use caveats::SqlCaveats; pub use service::{SqlService, SqlWaitBounds}; pub use types::{ diff --git a/tinycloud-core/src/sql/parser.rs b/tinycloud-core/src/sql/parser.rs index f9dde052..47092a12 100644 --- a/tinycloud-core/src/sql/parser.rs +++ b/tinycloud-core/src/sql/parser.rs @@ -1,11 +1,13 @@ +use std::ops::ControlFlow; + use sqlparser::ast::*; use sqlparser::dialect::SQLiteDialect; use sqlparser::parser::Parser; use sqlparser::tokenizer::{Token, Tokenizer, Whitespace}; +use super::authority::{DdlKind, DdlPolicy, SqlAuthority}; use super::caveats::SqlCaveats; use super::types::SqlError; -use crate::policy_capability::{ability_matches, resolve_alias}; use crate::write_hooks::TouchedTables; #[derive(Debug)] @@ -16,17 +18,70 @@ pub struct ParsedQuery { pub write_targets: Vec, pub is_read_only: bool, pub is_ddl: bool, + /// `DdlPolicy::Log` only: the statement is schema-changing, and it runs + /// only because `write` still sufficed. The caller counts and logs it. + pub ddl_without_schema: Option, +} + +/// What a SQL string does, whoever runs it. +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +pub struct SqlClass { + pub is_read_only: bool, + pub is_ddl: bool, +} + +/// The classification of one parsed statement list, before any ability or +/// caveat is applied. +struct Analysis { + statements: Vec, + tables: Vec, + columns: Vec, + write_targets: Vec, + is_read_only: bool, + is_ddl: bool, + has_non_ddl: bool, + ddl_kind: Option, +} + +/// Classify `sql` independently of the caller's abilities. +/// +/// A denial must never read as "not a write" (the storage-quota gate asks +/// this question), so the answer cannot depend on `validate_sql`'s ability +/// checks. Statements that do not parse are an `Err`; execution surfaces +/// the real error. +pub fn classify_sql(sql: &str) -> Result { + if is_pragma_sql(sql) { + return Ok(SqlClass { + is_read_only: true, + is_ddl: false, + }); + } + let analysis = analyze_statements(sql)?; + Ok(SqlClass { + is_read_only: analysis.is_read_only, + is_ddl: analysis.is_ddl, + }) } pub fn validate_sql( sql: &str, caveats: &Option, ability: &str, +) -> Result { + validate_sql_authorized(sql, caveats, &SqlAuthority::from(ability)) +} + +/// As [`validate_sql`], judging the statement against every SQL ability the +/// invocation holds and the node's DDL policy (TC-1581). +pub fn validate_sql_authorized( + sql: &str, + caveats: &Option, + authority: &SqlAuthority, ) -> Result { if is_pragma_sql(sql) { // TC-119: confers-admin gate. `sql/admin` and `sql/*` (implies admin) // pass; identical to the prior `admin | *` match. - if !ability_matches(ability, "tinycloud.sql/admin") { + if !authority.is_admin() { return Err(SqlError::PermissionDenied( "PRAGMA operations require admin ability".to_string(), )); @@ -38,18 +93,10 @@ pub fn validate_sql( let tokens = Tokenizer::new(&SQLiteDialect {}, sql) .tokenize() .map_err(|e| SqlError::ParseError(e.to_string()))?; + reject_nested_block_comments(&tokens)?; let mut terminated = false; for token in tokens { match token { - // sqlparser nests block comments, but SQLite ends at the first - // */. Fail closed rather than hiding executable SQL in a token. - Token::Whitespace(Whitespace::MultiLineComment(comment)) - if comment.contains("*/") => - { - return Err(SqlError::InvalidStatement( - "Nested block comments are not supported by SQLite".into(), - )); - } Token::Whitespace(_) | Token::EOF => {} Token::SemiColon => terminated = true, _ if terminated => { @@ -68,10 +115,122 @@ pub fn validate_sql( write_targets: Vec::new(), is_read_only: true, is_ddl: false, + ddl_without_schema: None, }); } + let Analysis { + statements, + mut tables, + mut columns, + write_targets, + is_read_only, + is_ddl, + has_non_ddl, + ddl_kind, + } = analyze_statements(sql)?; + + // TC-1581: schema-changing statements need a schema-conferring ability + // (`schema`; `admin` through the registry's `admin ⊃ schema`; `sql/*`). + // `write` covers data statements only. `DdlPolicy::Log` and `Off` keep + // the earlier rule, under which `write` also sufficed. + if is_ddl && !authority.may_change_schema() { + return Err(SqlError::PermissionDenied(match authority.ddl_policy() { + DdlPolicy::Enforce => { + "Schema-changing statements require the tinycloud.sql/schema ability".to_string() + } + DdlPolicy::Log | DdlPolicy::Off => { + "DDL operations require admin, schema, or write ability".to_string() + } + })); + } + let ddl_without_schema = if is_ddl && authority.schema_change_only_by_legacy_rule() { + ddl_kind + } else { + None + }; + + // Exact-tier restriction: the `schema` ability is DDL-only. A holder of + // nothing else cannot run a data statement. This is not a confers-check: + // a broader ability like `admin` is not restricted here. + if (!is_ddl || has_non_ddl) && authority.is_schema_only() { + return Err(SqlError::PermissionDenied( + "Schema ability only permits DDL operations".to_string(), + )); + } + + // Exact-tier restriction: a holder of the read tier only (`read`, or its + // `select` alias) cannot run a data write. Exact by design: `sql/*` + // implies read but must still permit writes, so this must not use + // `ability_matches`. + if !is_read_only && !is_ddl && authority.is_read_tier_only() { + return Err(SqlError::ReadOnlyViolation); + } + + // Validate caveats + if let Some(caveats) = caveats { + if caveats.read_only.unwrap_or(false) && !is_read_only { + return Err(SqlError::ReadOnlyViolation); + } + + for table in &tables { + if !caveats.is_table_allowed(table) { + return Err(SqlError::PermissionDenied(format!( + "Access to table '{}' is not allowed", + table + ))); + } + } + + for column in &columns { + if !caveats.is_column_allowed(column) { + return Err(SqlError::PermissionDenied(format!( + "Access to column '{}' is not allowed", + column + ))); + } + } + } + + tables.dedup(); + columns.dedup(); + + Ok(ParsedQuery { + statements, + referenced_tables: tables, + referenced_columns: columns, + write_targets, + is_read_only, + is_ddl, + ddl_without_schema, + }) +} + +/// sqlparser nests block comments, but SQLite ends a comment at the first +/// `*/`. Text after that point is executable SQL to SQLite and a comment to +/// the parser, so a statement that nests them is refused: fail closed rather +/// than analyze something other than what runs. +fn reject_nested_block_comments(tokens: &[Token]) -> Result<(), SqlError> { + let nested = tokens.iter().any(|token| { + matches!( + token, + Token::Whitespace(Whitespace::MultiLineComment(comment)) if comment.contains("*/") + ) + }); + if nested { + return Err(SqlError::InvalidStatement( + "Nested block comments are not supported by SQLite".into(), + )); + } + Ok(()) +} + +fn analyze_statements(sql: &str) -> Result { let dialect = SQLiteDialect {}; + // A tokenizer error is left for the parser to report. + if let Ok(tokens) = Tokenizer::new(&dialect, sql).tokenize() { + reject_nested_block_comments(&tokens)?; + } let statements = Parser::parse_sql(&dialect, sql).map_err(|e| SqlError::ParseError(e.to_string()))?; @@ -90,13 +249,26 @@ pub fn validate_sql( let mut is_read_only = true; let mut is_ddl = false; let mut has_non_ddl = false; + let mut ddl_kind = None; for stmt in &statements { match stmt { - Statement::Query(_) => { + Statement::Query(query) => { has_non_ddl = true; extract_tables_from_statement(stmt, &mut tables); extract_columns_from_statement(stmt, &mut columns); + // TC-1585: `WITH … INSERT/UPDATE` parses as a Query whose body + // is a write. A write anywhere in the tree makes the statement + // a write: it is quota-gated, emits write hooks and is made + // durable like any other write, and a read tier cannot run it. + for write in writes_in_query(query) { + is_read_only = false; + extract_tables_from_statement(&write, &mut tables); + extract_columns_from_statement(&write, &mut columns); + if let Some(targets) = extract_write_targets_from_statement(&write) { + write_targets.push(targets); + } + } } Statement::Insert { .. } => { has_non_ddl = true; @@ -130,6 +302,7 @@ pub fn validate_sql( | Statement::CreateIndex { .. } => { is_read_only = false; is_ddl = true; + ddl_kind = Some(ddl_kind_of(stmt)); extract_tables_from_statement(stmt, &mut tables); if let Some(targets) = extract_write_targets_from_statement(stmt) { write_targets.push(targets); @@ -149,74 +322,44 @@ pub fn validate_sql( } } - // Validate ability vs operation type - // TC-119: DDL requires an ability that confers write OR schema. `sql/admin` - // qualifies because the registry declares `admin ⊃ schema`; `sql/*` qualifies - // because it implies both. This reproduces the prior `admin|write|schema|*` - // set exactly, now sourced from the registry implication table. - if is_ddl - && !(ability_matches(ability, "tinycloud.sql/write") - || ability_matches(ability, "tinycloud.sql/schema")) - { - return Err(SqlError::PermissionDenied( - "DDL operations require admin, schema, or write ability".to_string(), - )); - } - - // Exact-tier restriction: the `schema` ability is DDL-only. This is NOT a - // confers-check — it must stay exact (a broader ability like `admin` is not - // restricted here), so we compare the alias-resolved URN, not `ability_matches`. - if resolve_alias(ability) == "tinycloud.sql/schema" && (!is_ddl || has_non_ddl) { - return Err(SqlError::PermissionDenied( - "Schema ability only permits DDL operations".to_string(), - )); - } - - // Exact-tier restriction: read-tier (`read`, or its `select` alias) cannot - // run a write. Exact by design — `sql/*` implies read but must still permit - // writes, so this must not use `ability_matches`. - if !is_read_only && resolve_alias(ability) == "tinycloud.sql/read" { - return Err(SqlError::ReadOnlyViolation); - } - - // Validate caveats - if let Some(caveats) = caveats { - if caveats.read_only.unwrap_or(false) && !is_read_only { - return Err(SqlError::ReadOnlyViolation); - } - - for table in &tables { - if !caveats.is_table_allowed(table) { - return Err(SqlError::PermissionDenied(format!( - "Access to table '{}' is not allowed", - table - ))); - } - } - - for column in &columns { - if !caveats.is_column_allowed(column) { - return Err(SqlError::PermissionDenied(format!( - "Access to column '{}' is not allowed", - column - ))); - } - } - } - - tables.dedup(); - columns.dedup(); - - Ok(ParsedQuery { + Ok(Analysis { statements, - referenced_tables: tables, - referenced_columns: columns, + tables, + columns, write_targets, is_read_only, is_ddl, + has_non_ddl, + ddl_kind, }) } +fn ddl_kind_of(stmt: &Statement) -> DdlKind { + match stmt { + Statement::CreateView { .. } => DdlKind::CreateView, + Statement::CreateIndex { .. } => DdlKind::CreateIndex, + Statement::AlterTable { .. } => DdlKind::AlterTable, + Statement::Drop { .. } => DdlKind::Drop, + _ => DdlKind::CreateTable, + } +} + +/// The INSERT, UPDATE and DELETE statements anywhere inside a query tree +/// (its CTE bodies, sub-queries and set operations). +fn writes_in_query(query: &Query) -> Vec { + let mut writes = Vec::new(); + let _ = visit_statements(query, |statement| { + if matches!( + statement, + Statement::Insert { .. } | Statement::Update { .. } | Statement::Delete { .. } + ) { + writes.push(statement.clone()); + } + ControlFlow::<()>::Continue(()) + }); + writes +} + pub fn is_pragma_sql(sql: &str) -> bool { first_sql_token(sql).as_deref() == Some("pragma") } @@ -602,3 +745,460 @@ mod tests { ); } } + +/// TC-1581: schema-changing statements need `schema`; `write` is data only. +#[cfg(test)] +mod ddl_authority_tests { + use super::*; + use crate::sql::authority::DdlPolicy; + use crate::write_hooks::TouchedTables; + + const READ: &str = "tinycloud.sql/read"; + const WRITE: &str = "tinycloud.sql/write"; + const SCHEMA: &str = "tinycloud.sql/schema"; + const ADMIN: &str = "tinycloud.sql/admin"; + const STAR: &str = "tinycloud.sql/*"; + + fn held(abilities: &[&str]) -> SqlAuthority { + SqlAuthority::new(abilities.iter().copied()) + } + + fn validate(sql: &str, abilities: &[&str]) -> Result { + validate_sql_authorized(sql, &None, &held(abilities)) + } + + /// One statement of every schema-changing kind the parser classifies. + const DDL: [&str; 10] = [ + "CREATE TABLE notes (id INTEGER PRIMARY KEY, body TEXT)", + "CREATE TABLE IF NOT EXISTS notes (id INTEGER PRIMARY KEY)", + "CREATE TEMP TABLE scratch (id INTEGER)", + "CREATE TABLE copied AS SELECT id FROM notes", + "CREATE VIEW note_ids AS SELECT id FROM notes", + "CREATE INDEX notes_body ON notes (body)", + "CREATE UNIQUE INDEX notes_id ON notes (id)", + "ALTER TABLE notes ADD COLUMN extra TEXT", + "DROP TABLE notes", + "DROP VIEW note_ids", + ]; + + const DML: [&str; 3] = [ + "INSERT INTO notes (id) VALUES (1)", + "UPDATE notes SET body = 'x'", + "DELETE FROM notes WHERE id = 1", + ]; + + #[test] + fn pragma_stays_admin_only_whatever_else_is_held() { + for sql in ["PRAGMA user_version = 3", "PRAGMA table_info(notes)"] { + for abilities in [ + &[WRITE][..], + &[SCHEMA][..], + &[WRITE, SCHEMA][..], + &[READ, WRITE, SCHEMA][..], + ] { + for policy in [DdlPolicy::Enforce, DdlPolicy::Log, DdlPolicy::Off] { + let authority = held(abilities).with_ddl_policy(policy); + assert!( + matches!( + validate_sql_authorized(sql, &None, &authority), + Err(SqlError::PermissionDenied(_)) + ), + "{abilities:?} must not run {sql} under {policy:?}" + ); + } + } + for admin in [ADMIN, STAR] { + validate(sql, &[admin]).expect("admin and * may run PRAGMA"); + } + } + } + + #[test] + fn ddl_needs_a_schema_conferring_ability() { + for sql in DDL { + for (abilities, allowed) in [ + (&[WRITE][..], false), + (&[READ][..], false), + (&[READ, WRITE][..], false), + (&["tinycloud.sql/select"][..], false), + (&["tinycloud.sql/ddl"][..], false), + (&[SCHEMA][..], true), + (&[WRITE, SCHEMA][..], true), + (&[READ, SCHEMA][..], true), + (&[ADMIN][..], true), + (&[STAR][..], true), + (&[WRITE, ADMIN][..], true), + ] { + let result = validate(sql, abilities); + if allowed { + let parsed = + result.unwrap_or_else(|e| panic!("{abilities:?} must run {sql}: {e:?}")); + assert!(parsed.is_ddl && !parsed.is_read_only, "{sql}"); + assert_eq!(parsed.ddl_without_schema, None, "{sql}"); + } else { + assert!( + matches!(result, Err(SqlError::PermissionDenied(_))), + "{abilities:?} must not run {sql}, got {result:?}" + ); + } + } + } + } + + #[test] + fn write_alone_is_refused_with_a_schema_hint() { + let err = validate("DROP TABLE notes", &[WRITE]).unwrap_err(); + assert!( + matches!(&err, SqlError::PermissionDenied(message) if message.contains("tinycloud.sql/schema")), + "{err:?}" + ); + } + + #[test] + fn data_statements_keep_their_tiers() { + for sql in DML { + for (abilities, expected) in [ + (&[WRITE][..], "ok"), + (&[READ, WRITE][..], "ok"), + (&[WRITE, SCHEMA][..], "ok"), + (&[ADMIN][..], "ok"), + (&[STAR][..], "ok"), + (&[READ][..], "read-only"), + (&["tinycloud.sql/select"][..], "read-only"), + (&[READ, SCHEMA][..], "read-only"), + (&[SCHEMA][..], "schema-only"), + ] { + let result = validate(sql, abilities); + match expected { + "ok" => { + let parsed = result + .unwrap_or_else(|e| panic!("{abilities:?} must run {sql}: {e:?}")); + assert!(!parsed.is_ddl && !parsed.is_read_only); + } + "read-only" => assert!( + matches!(result, Err(SqlError::ReadOnlyViolation)), + "{abilities:?} {sql}: {result:?}" + ), + _ => assert!( + matches!(&result, Err(SqlError::PermissionDenied(m)) if m.contains("only permits DDL")), + "{abilities:?} {sql}: {result:?}" + ), + } + } + } + } + + #[test] + fn select_needs_a_data_ability_not_schema_alone() { + for abilities in [ + &[READ][..], + &[WRITE][..], + &[ADMIN][..], + &[STAR][..], + &[READ, SCHEMA][..], + ] { + validate("SELECT id FROM notes", abilities) + .unwrap_or_else(|e| panic!("{abilities:?} must read: {e:?}")); + } + let err = validate("SELECT id FROM notes", &[SCHEMA]).unwrap_err(); + assert!(matches!(err, SqlError::PermissionDenied(m) if m.contains("only permits DDL"))); + } + + #[test] + fn statements_outside_the_allow_list_stay_denied_for_every_ability() { + for sql in [ + "ATTACH DATABASE 'other.db' AS other", + "DETACH DATABASE other", + "VACUUM", + "VACUUM INTO 'copy.db'", + "REINDEX", + "ANALYZE TABLE notes", + "CREATE VIRTUAL TABLE docs USING fts5(body)", + "CREATE TRIGGER t AFTER INSERT ON notes BEGIN DELETE FROM notes; END", + "DROP TRIGGER t", + "BEGIN", + "COMMIT", + "SAVEPOINT s", + "EXPLAIN SELECT 1", + ] { + for abilities in [ + &[WRITE][..], + &[WRITE, SCHEMA][..], + &[ADMIN][..], + &[STAR][..], + ] { + assert!( + validate(sql, abilities).is_err(), + "{abilities:?} must not run {sql}" + ); + } + } + } + + #[test] + fn mixed_requests_need_both_abilities_per_statement() { + // A request that runs DDL and DML is validated statement by statement + // against the same held set. + let ddl = "CREATE TABLE IF NOT EXISTS notes (id INTEGER PRIMARY KEY)"; + let dml = "INSERT INTO notes (id) VALUES (1)"; + for (abilities, ddl_ok, dml_ok) in [ + (&[WRITE][..], false, true), + (&[SCHEMA][..], true, false), + (&[WRITE, SCHEMA][..], true, true), + (&[ADMIN][..], true, true), + (&[STAR][..], true, true), + (&[READ][..], false, false), + ] { + assert_eq!( + validate(ddl, abilities).is_ok(), + ddl_ok, + "{abilities:?} ddl" + ); + assert_eq!( + validate(dml, abilities).is_ok(), + dml_ok, + "{abilities:?} dml" + ); + } + } + + #[test] + fn policy_off_and_log_keep_the_earlier_rule_and_log_marks_the_statement() { + for policy in [DdlPolicy::Off, DdlPolicy::Log] { + for sql in DDL { + let write_only = held(&[WRITE]).with_ddl_policy(policy); + let parsed = validate_sql_authorized(sql, &None, &write_only) + .unwrap_or_else(|e| panic!("{policy:?} must run {sql}: {e:?}")); + assert_eq!( + parsed.ddl_without_schema.is_some(), + policy == DdlPolicy::Log, + "{policy:?} {sql}" + ); + + let both = held(&[WRITE, SCHEMA]).with_ddl_policy(policy); + assert_eq!( + validate_sql_authorized(sql, &None, &both) + .unwrap() + .ddl_without_schema, + None + ); + + let read_only = held(&[READ]).with_ddl_policy(policy); + assert!(matches!( + validate_sql_authorized(sql, &None, &read_only), + Err(SqlError::PermissionDenied(_)) + )); + } + let data = held(&[WRITE]).with_ddl_policy(policy); + assert_eq!( + validate_sql_authorized("INSERT INTO notes (id) VALUES (1)", &None, &data) + .unwrap() + .ddl_without_schema, + None + ); + } + } + + #[test] + fn log_marks_each_statement_kind() { + let write_only = held(&[WRITE]).with_ddl_policy(DdlPolicy::Log); + for (sql, kind) in [ + ("CREATE TABLE t (a)", DdlKind::CreateTable), + ("CREATE VIEW v AS SELECT 1", DdlKind::CreateView), + ("CREATE INDEX i ON t (a)", DdlKind::CreateIndex), + ("ALTER TABLE t ADD COLUMN b", DdlKind::AlterTable), + ("DROP TABLE t", DdlKind::Drop), + ] { + assert_eq!( + validate_sql_authorized(sql, &None, &write_only) + .unwrap() + .ddl_without_schema, + Some(kind), + "{sql}" + ); + } + } + + /// One CTE-wrapped write per kind the parser can represent, with the + /// table it touches. + const CTE_WRITES: [(&str, &str); 3] = [ + ( + "WITH c AS (SELECT 1 AS id) INSERT INTO notes (id) SELECT id FROM c", + "notes", + ), + ( + "WITH RECURSIVE n(i) AS (SELECT 1 UNION ALL SELECT i + 1 FROM n WHERE i < 3) INSERT INTO notes (id) SELECT i FROM n", + "notes", + ), + ( + "WITH c AS (SELECT 'x' AS body) UPDATE notes SET body = (SELECT body FROM c)", + "notes", + ), + ]; + + #[test] + fn cte_wrapped_writes_are_writes_with_their_targets() { + for (sql, table) in CTE_WRITES { + let class = classify_sql(sql).unwrap(); + assert!(!class.is_read_only && !class.is_ddl, "{sql}"); + + for abilities in [ + &[WRITE][..], + &[WRITE, SCHEMA][..], + &[ADMIN][..], + &[STAR][..], + ] { + let parsed = validate(sql, abilities).unwrap(); + assert!(!parsed.is_read_only && !parsed.is_ddl, "{sql}"); + assert_eq!( + parsed.write_targets, + vec![TouchedTables::supported(vec![table.to_string()])], + "{sql} must report its target for write hooks and durability" + ); + } + } + } + + #[test] + fn cte_wrapped_writes_are_refused_to_read_and_schema_only_holders() { + for (sql, _) in CTE_WRITES { + for abilities in [ + &[READ][..], + &["tinycloud.sql/select"][..], + &[READ, SCHEMA][..], + ] { + assert!( + matches!(validate(sql, abilities), Err(SqlError::ReadOnlyViolation)), + "{abilities:?} {sql}" + ); + } + let err = validate(sql, &[SCHEMA]).unwrap_err(); + assert!(matches!(err, SqlError::PermissionDenied(m) if m.contains("only permits DDL"))); + } + } + + #[test] + fn cte_wrapped_writes_obey_the_caveats_of_a_write() { + for (sql, _) in CTE_WRITES { + let read_only = Some(SqlCaveats { + read_only: Some(true), + ..SqlCaveats::default() + }); + assert!(matches!( + validate_sql_authorized(sql, &read_only, &held(&[WRITE])), + Err(SqlError::ReadOnlyViolation) + )); + let other_table = Some(SqlCaveats { + tables: Some(vec!["other".to_string()]), + ..SqlCaveats::default() + }); + assert!(matches!( + validate_sql_authorized(sql, &other_table, &held(&[WRITE])), + Err(SqlError::PermissionDenied(_)) + )); + let this_table = Some(SqlCaveats { + tables: Some(vec!["notes".to_string()]), + ..SqlCaveats::default() + }); + validate_sql_authorized(sql, &this_table, &held(&[WRITE])).unwrap(); + } + } + + #[test] + fn a_cte_without_a_write_stays_read_only() { + for sql in [ + "WITH c AS (SELECT id FROM notes) SELECT id FROM c", + "WITH RECURSIVE n(i) AS (SELECT 1 UNION ALL SELECT i + 1 FROM n WHERE i < 3) SELECT i FROM n", + "SELECT id FROM notes WHERE id IN (WITH c AS (SELECT 1) SELECT * FROM c)", + ] { + let parsed = validate(sql, &[READ]).unwrap(); + assert!(parsed.is_read_only && parsed.write_targets.is_empty(), "{sql}"); + assert!(classify_sql(sql).unwrap().is_read_only, "{sql}"); + } + } + + #[test] + fn a_cte_wrapped_delete_is_not_accepted_for_any_ability() { + // sqlparser 0.44 cannot represent `WITH … DELETE`, so it is rejected + // outright rather than classified as a read. + let sql = "WITH c AS (SELECT 1 AS id) DELETE FROM notes WHERE id IN (SELECT id FROM c)"; + assert!(classify_sql(sql).is_err()); + for abilities in [&[READ][..], &[WRITE][..], &[ADMIN][..], &[STAR][..]] { + assert!(validate(sql, abilities).is_err(), "{abilities:?}"); + } + } + + #[test] + fn classification_does_not_depend_on_abilities() { + // A denial must never read as "not a write" (quota gate). + for sql in DDL { + let class = classify_sql(sql).unwrap(); + assert!(class.is_ddl && !class.is_read_only, "{sql}"); + assert!(validate(sql, &[WRITE]).is_err()); + } + for sql in DML { + let class = classify_sql(sql).unwrap(); + assert!(!class.is_ddl && !class.is_read_only, "{sql}"); + } + let select = classify_sql("SELECT 1").unwrap(); + assert!(select.is_read_only && !select.is_ddl); + // PRAGMA is classified read-only, as `validate_sql` always did. + assert!(classify_sql("PRAGMA table_info(t)").unwrap().is_read_only); + assert!(classify_sql("NOT SQL AT ALL").is_err()); + } + + #[test] + fn read_only_caveat_still_refuses_ddl_for_a_schema_holder() { + let caveats = Some(SqlCaveats { + read_only: Some(true), + ..SqlCaveats::default() + }); + let err = + validate_sql_authorized("CREATE TABLE t (a)", &caveats, &held(&[SCHEMA])).unwrap_err(); + assert!(matches!(err, SqlError::ReadOnlyViolation)); + } + + #[test] + fn an_unregistered_ability_beside_schema_behaves_like_schema_alone() { + const UNREGISTERED: &str = "tinycloud.sql/not-a-registered-ability"; + for sql in DML + .iter() + .chain(&["SELECT id FROM notes"]) + .chain(&CTE_WRITES.map(|(sql, _)| sql)) + { + for abilities in [&[SCHEMA][..], &[SCHEMA, UNREGISTERED][..]] { + let err = validate(sql, abilities).unwrap_err(); + assert!( + matches!(&err, SqlError::PermissionDenied(m) if m.contains("only permits DDL")), + "{abilities:?} {sql}: {err:?}" + ); + } + } + for sql in DDL { + validate(sql, &[SCHEMA, UNREGISTERED]).unwrap(); + } + } + + /// sqlparser nests block comments and SQLite does not, so a statement + /// that nests them is read differently by the two. It is refused on every + /// path, for every ability. + #[test] + fn nested_block_comments_are_refused_for_every_statement() { + for sql in [ + "SELECT 1 /* a /* b */ ; DELETE FROM notes; /* c */", + "INSERT INTO notes (id) VALUES (1) /* /* */ ; DELETE FROM notes; /* */", + "CREATE TABLE IF NOT EXISTS made (id INTEGER) /* /* */ ; DELETE FROM notes; /* */", + "PRAGMA user_version /* /* */ ; DELETE FROM notes; /* */", + ] { + for abilities in [&[READ][..], &[WRITE][..], &[SCHEMA][..], &[ADMIN][..]] { + let result = validate(sql, abilities); + assert!(result.is_err(), "{abilities:?} {sql}: {result:?}"); + if !is_pragma_sql(sql) { + assert!(classify_sql(sql).is_err(), "{sql}"); + } + } + } + // An ordinary comment is unaffected. + assert!(validate("SELECT 1 /* plain */", &[READ]).is_ok()); + } +} diff --git a/tinycloud-core/src/sql/service.rs b/tinycloud-core/src/sql/service.rs index 5967ced8..f569c0ae 100644 --- a/tinycloud-core/src/sql/service.rs +++ b/tinycloud-core/src/sql/service.rs @@ -21,6 +21,7 @@ use crate::db_pool::{is_transient_error, track_writes, transient_error_class}; use crate::{request_cancel, request_telemetry}; use super::{ + authority::{DdlPolicy, SqlAuthority}, caveats::SqlCaveats, database::{spawn_actor, DatabaseHandle, GrowthPolicy}, types::*, @@ -156,6 +157,7 @@ pub struct SqlService { memory_threshold: u64, artifact_repository: Arc, bounds: SqlWaitBounds, + ddl_policy: DdlPolicy, } impl SqlService { @@ -173,9 +175,17 @@ impl SqlService { memory_threshold, artifact_repository, bounds: SqlWaitBounds::default(), + ddl_policy: DdlPolicy::default(), } } + /// Whether a schema-changing statement needs `tinycloud.sql/schema` + /// (TC-1581). Defaults to [`DdlPolicy::Enforce`]. + pub fn with_ddl_policy(mut self, policy: DdlPolicy) -> Self { + self.ddl_policy = policy; + self + } + pub fn with_wait_bounds(mut self, bounds: SqlWaitBounds) -> Self { self.bounds = bounds; self @@ -194,7 +204,28 @@ impl SqlService { db_name, request, caveats, - ability, + SqlAuthority::from(ability).with_ddl_policy(self.ddl_policy), + GrowthPolicy::Allow, + ) + .await + } + + /// As [`Self::execute`], judging each statement against every SQL ability + /// the invocation holds (TC-1581). + pub async fn execute_authorized( + &self, + space: &SpaceId, + db_name: &str, + request: SqlRequest, + caveats: Option, + authority: SqlAuthority, + ) -> Result { + self.execute_with_growth( + space, + db_name, + request, + caveats, + authority.with_ddl_policy(self.ddl_policy), GrowthPolicy::Allow, ) .await @@ -217,7 +248,28 @@ impl SqlService { db_name, request, caveats, - ability, + SqlAuthority::from(ability).with_ddl_policy(self.ddl_policy), + GrowthPolicy::Forbid, + ) + .await + } + + /// As [`Self::execute_without_growth`], judging each statement against + /// every SQL ability the invocation holds (TC-1581). + pub async fn execute_without_growth_authorized( + &self, + space: &SpaceId, + db_name: &str, + request: SqlRequest, + caveats: Option, + authority: SqlAuthority, + ) -> Result { + self.execute_with_growth( + space, + db_name, + request, + caveats, + authority.with_ddl_policy(self.ddl_policy), GrowthPolicy::Forbid, ) .await @@ -229,7 +281,7 @@ impl SqlService { db_name: &str, request: SqlRequest, caveats: Option, - ability: String, + authority: SqlAuthority, growth: GrowthPolicy, ) -> Result { crate::request_telemetry::observe_space(space); @@ -237,7 +289,7 @@ impl SqlService { let (space, db_name) = (space.clone(), db_name.to_string()); self.run_locked(guard, move |service| async move { service - .execute_locked(&space, &db_name, request, caveats, ability, growth) + .execute_locked(&space, &db_name, request, caveats, authority, growth) .await }) .await @@ -250,7 +302,7 @@ impl SqlService { db_name: &str, request: SqlRequest, caveats: Option, - ability: String, + authority: SqlAuthority, growth: GrowthPolicy, ) -> Result { let key = (space.to_string(), db_name.to_string()); @@ -258,7 +310,7 @@ impl SqlService { begin_write()?; let result = match handle - .execute(request.clone(), caveats.clone(), ability.clone(), growth) + .execute_authorized(request.clone(), caveats.clone(), authority.clone(), growth) .await { Err(SqlError::Internal(ref msg)) if msg.contains("Database actor not available") => { @@ -266,7 +318,9 @@ impl SqlService { tracing::warn!("Dead SQL actor detected, respawning"); self.databases.remove(&key); handle = self.handle(space, db_name).await?; - handle.execute(request, caveats, ability, growth).await + handle + .execute_authorized(request, caveats, authority, growth) + .await } other => other, }; @@ -1109,7 +1163,7 @@ mod tests { params: vec![], }, None, - "tinycloud.sql/write".into(), + "tinycloud.sql/admin".into(), ) .await; match name { @@ -1224,7 +1278,7 @@ mod tests { params: vec![], }, None, - "tinycloud.sql/write".into(), + "tinycloud.sql/admin".into(), ) .await .expect("dropping an absent table is a successful no-op"); @@ -1267,7 +1321,7 @@ mod tests { params: vec![], }, None, - "tinycloud.sql/write".into(), + "tinycloud.sql/admin".into(), ) .await .unwrap(); @@ -1291,7 +1345,7 @@ mod tests { params: vec![], }, None, - "tinycloud.sql/write".into(), + "tinycloud.sql/admin".into(), ) .await; assert!(matches!(failed, Err(SqlError::Sqlite(_)))); @@ -1305,7 +1359,7 @@ mod tests { params: vec![], }, None, - "tinycloud.sql/write".into(), + "tinycloud.sql/admin".into(), ) .await; assert!( @@ -1349,7 +1403,7 @@ mod tests { params: vec![], }, None, - "tinycloud.sql/write".into(), + "tinycloud.sql/admin".into(), ) .await .unwrap(); @@ -1397,6 +1451,598 @@ mod tests { .expect("schema ability should create tables"); } + fn ddl(sql: &str) -> SqlRequest { + SqlRequest::Execute { + schema: None, + sql: sql.to_string(), + params: Vec::new(), + } + } + + async fn table_count(service: &SqlService, space: &SpaceId) -> i64 { + let result = service + .execute( + space, + "main", + SqlRequest::Query { + sql: "SELECT count(*) FROM items".to_string(), + params: Vec::new(), + max_rows: None, + max_bytes: None, + }, + None, + "tinycloud.sql/read".to_string(), + ) + .await + .expect("count rows"); + let SqlResponse::Query(rows) = result.response else { + panic!("expected rows"); + }; + let SqlValue::Integer(count) = rows.rows[0][0] else { + panic!("expected an integer"); + }; + count + } + + /// TC-1581 through the service: `write` alone runs data statements and + /// is refused schema-changing ones; the data it wrote is durable. + #[tokio::test] + async fn sql_write_alone_cannot_change_the_schema_but_writes_data() { + let repo = artifact_repository().await; + let cache = TempDir::new().unwrap(); + let service = SqlService::new(cache.path().to_string_lossy().to_string(), u64::MAX, repo); + let space = test_space_id("sql-ddl-needs-schema"); + + let refused = service + .execute( + &space, + "main", + ddl("CREATE TABLE items (id INTEGER PRIMARY KEY, name TEXT)"), + None, + "tinycloud.sql/write".to_string(), + ) + .await; + assert!( + matches!(refused, Err(SqlError::PermissionDenied(_))), + "{refused:?}" + ); + + service + .execute( + &space, + "main", + ddl("CREATE TABLE items (id INTEGER PRIMARY KEY, name TEXT)"), + None, + "tinycloud.sql/schema".to_string(), + ) + .await + .expect("schema creates the table"); + for sql in [ + "INSERT INTO items (name) VALUES ('a')", + "UPDATE items SET name = 'b'", + "WITH c AS (SELECT 'c' AS name) INSERT INTO items (name) SELECT name FROM c", + ] { + service + .execute( + &space, + "main", + ddl(sql), + None, + "tinycloud.sql/write".to_string(), + ) + .await + .unwrap_or_else(|e| panic!("write runs {sql}: {e:?}")); + } + for sql in [ + "DROP TABLE items", + "ALTER TABLE items ADD COLUMN extra TEXT", + "CREATE INDEX items_name ON items (name)", + ] { + let refused = service + .execute( + &space, + "main", + ddl(sql), + None, + "tinycloud.sql/write".to_string(), + ) + .await; + assert!( + matches!(refused, Err(SqlError::PermissionDenied(_))), + "{sql}: {refused:?}" + ); + // A full space answers the same, not a storage error. + let refused = service + .execute_without_growth( + &space, + "main", + ddl(sql), + None, + "tinycloud.sql/write".to_string(), + ) + .await; + assert!( + matches!(refused, Err(SqlError::PermissionDenied(_))), + "{sql}: {refused:?}" + ); + } + assert_eq!(table_count(&service, &space).await, 2); + + // Mixed `[write, schema]` request, the SDK's migration shape. + service + .execute_authorized( + &space, + "main", + SqlRequest::Execute { + schema: Some(vec!["ALTER TABLE items ADD COLUMN extra TEXT".to_string()]), + sql: "INSERT INTO items (name, extra) VALUES ('d', 'x')".to_string(), + params: Vec::new(), + }, + None, + SqlAuthority::new(["tinycloud.sql/write", "tinycloud.sql/schema"]), + ) + .await + .expect("write and schema together run both statement kinds"); + assert_eq!(table_count(&service, &space).await, 3); + } + + /// TC-1581 `ddl_requires_schema`: `off` and `log` keep the earlier rule + /// for `write`; the default is `enforce`. + #[tokio::test] + async fn ddl_policy_reaches_the_actor_through_the_service() { + let _serial = crate::sql::authority::LOG_COUNTER_LOCK.lock().await; + use crate::sql::authority::DdlPolicy; + for (policy, write_may_change_schema) in [ + (None, false), + (Some(DdlPolicy::Enforce), false), + (Some(DdlPolicy::Log), true), + (Some(DdlPolicy::Off), true), + ] { + let repo = artifact_repository().await; + let cache = TempDir::new().unwrap(); + let mut service = + SqlService::new(cache.path().to_string_lossy().to_string(), u64::MAX, repo); + if let Some(policy) = policy { + service = service.with_ddl_policy(policy); + } + let space = test_space_id("sql-ddl-policy"); + let result = service + .execute( + &space, + "main", + ddl("CREATE TABLE items (id INTEGER)"), + None, + "tinycloud.sql/write".to_string(), + ) + .await; + assert_eq!( + result.is_ok(), + write_may_change_schema, + "{policy:?}: {result:?}" + ); + } + } + + /// TC-1585: a `WITH … INSERT/UPDATE` is a write. It is made durable like + /// any other write (a fresh node hydrating the artifact sees it) and + /// reports its target tables for write hooks. + #[tokio::test] + async fn cte_wrapped_writes_are_durable_and_reported_as_writes() { + let repo = artifact_repository().await; + let cache_one = TempDir::new().unwrap(); + let cache_two = TempDir::new().unwrap(); + let space = test_space_id("sql-cte-write"); + let service = SqlService::new( + cache_one.path().to_string_lossy().to_string(), + u64::MAX, + repo.clone(), + ); + service + .execute( + &space, + "main", + ddl("CREATE TABLE items (id INTEGER PRIMARY KEY, name TEXT NOT NULL)"), + None, + "tinycloud.sql/schema".to_string(), + ) + .await + .unwrap(); + + for (sql, kind) in [ + ( + "WITH c AS (SELECT 'inserted' AS name) INSERT INTO items (name) SELECT name FROM c", + "insert", + ), + ( + "WITH c AS (SELECT 'updated' AS name) UPDATE items SET name = (SELECT name FROM c)", + "update", + ), + ] { + let result = service + .execute( + &space, + "main", + ddl(sql), + None, + "tinycloud.sql/write".to_string(), + ) + .await + .unwrap_or_else(|e| panic!("{kind}: {e:?}")); + assert_eq!( + result.write_targets, + vec![crate::write_hooks::TouchedTables::supported(vec![ + "items".to_string() + ])], + "{kind} must report its target table for write hooks" + ); + let SqlResponse::Execute(response) = result.response else { + panic!("{kind}: expected an execute response"); + }; + assert_eq!(response.changes, 1, "{kind}"); + } + drop(service); + + let recreated = SqlService::new( + cache_two.path().to_string_lossy().to_string(), + u64::MAX, + repo, + ); + let result = recreated + .execute( + &space, + "main", + SqlRequest::Query { + sql: "SELECT name FROM items".to_string(), + params: Vec::new(), + max_rows: None, + max_bytes: None, + }, + None, + "tinycloud.sql/read".to_string(), + ) + .await + .unwrap(); + let SqlResponse::Query(rows) = result.response else { + panic!("expected rows"); + }; + assert_eq!(rows.rows, vec![vec![SqlValue::Text("updated".to_string())]]); + } + + async fn pragma_value(service: &SqlService, space: &SpaceId, pragma: &str) -> SqlValue { + let result = service + .execute( + space, + "main", + SqlRequest::Query { + sql: format!("PRAGMA {pragma}"), + params: Vec::new(), + max_rows: None, + max_bytes: None, + }, + None, + "tinycloud.sql/admin".to_string(), + ) + .await + .expect("admin reads a pragma"); + let SqlResponse::Query(rows) = result.response else { + panic!("expected rows"); + }; + rows.rows[0][0].clone() + } + + /// The connection-level PRAGMAs that request SQL must not be able to change. + const STATE_PRAGMAS: [&str; 9] = [ + "PRAGMA writable_schema = ON", + "PRAGMA query_only = ON", + "PRAGMA foreign_keys = OFF", + "PRAGMA ignore_check_constraints = ON", + "PRAGMA recursive_triggers = ON", + "PRAGMA max_page_count = 1", + "PRAGMA cache_spill = 0", + "PRAGMA soft_heap_limit = 1", + "PRAGMA temp_store_directory = ''", + ]; + + async fn seeded_items(service: &SqlService, space: &SpaceId) { + service + .execute( + space, + "main", + ddl("CREATE TABLE items (id INTEGER PRIMARY KEY, name TEXT)"), + None, + "tinycloud.sql/schema".to_string(), + ) + .await + .unwrap(); + service + .execute( + space, + "main", + ddl("INSERT INTO items (name) VALUES ('kept')"), + None, + "tinycloud.sql/write".to_string(), + ) + .await + .unwrap(); + } + + /// TC-1586: connection state is shared by every request on a database, so + /// no request may change it. An admin may run only a short list of + /// PRAGMAs (none of which change connection state); the rest are refused + /// on every request path, and a later `write` request sees the opening + /// state and cannot edit `sqlite_master`. Covers an in-memory database and + /// one promoted to a WAL-backed file. + #[tokio::test] + async fn an_admin_request_cannot_change_connection_state() { + for (name, memory_threshold) in [("sql-pragma-mem", u64::MAX), ("sql-pragma-file", 0)] { + let repo = artifact_repository().await; + let cache = TempDir::new().unwrap(); + let service = SqlService::new( + cache.path().to_string_lossy().to_string(), + memory_threshold, + repo, + ); + let space = test_space_id(name); + seeded_items(&service, &space).await; + let journal_mode = pragma_value(&service, &space, "journal_mode").await; + let admin = || "tinycloud.sql/admin".to_string(); + + for pragma in STATE_PRAGMAS { + let query = SqlRequest::Query { + sql: pragma.to_string(), + params: Vec::new(), + max_rows: None, + max_bytes: None, + }; + let batch = SqlRequest::Batch { + statements: vec![SqlStatement { + sql: pragma.to_string(), + params: Vec::new(), + }], + }; + let in_schema_list = SqlRequest::Execute { + schema: Some(vec![pragma.to_string()]), + sql: "SELECT 1".to_string(), + params: Vec::new(), + }; + for request in [ddl(pragma), query, batch, in_schema_list] { + let refused = service + .execute(&space, "main", request, None, admin()) + .await; + assert!(refused.is_err(), "{name}: {pragma}: {refused:?}"); + } + } + for pragma in [ + "journal_mode = DELETE", + "journal_mode = OFF", + "locking_mode = EXCLUSIVE", + ] { + let refused = service + .execute( + &space, + "main", + SqlRequest::Query { + sql: format!("PRAGMA {pragma}"), + params: Vec::new(), + max_rows: None, + max_bytes: None, + }, + None, + admin(), + ) + .await; + assert!(refused.is_err(), "{name}: {pragma}: {refused:?}"); + } + + // A named statement is judged the same way. + let named = Some(SqlCaveats { + statements: Some(vec![crate::sql::caveats::PreparedStatement { + name: "state".into(), + sql: "PRAGMA writable_schema = ON".into(), + }]), + ..SqlCaveats::default() + }); + let refused = service + .execute( + &space, + "main", + SqlRequest::ExecuteStatement { + name: "state".into(), + params: Vec::new(), + }, + named, + admin(), + ) + .await; + assert!(refused.is_err(), "{name}: named statement: {refused:?}"); + + // The next request starts from the opening state. + assert_eq!( + pragma_value(&service, &space, "writable_schema").await, + SqlValue::Integer(0), + "{name}" + ); + assert_eq!( + pragma_value(&service, &space, "query_only").await, + SqlValue::Integer(0), + "{name}" + ); + assert_eq!( + pragma_value(&service, &space, "foreign_keys").await, + SqlValue::Integer(1), + "{name}" + ); + assert_eq!( + pragma_value(&service, &space, "journal_mode").await, + journal_mode, + "{name}" + ); + + // A holder of write only can write data, not the schema table. + service + .execute( + &space, + "main", + ddl("INSERT INTO items (name) VALUES ('more')"), + None, + "tinycloud.sql/write".to_string(), + ) + .await + .unwrap_or_else(|e| panic!("{name}: write must still write data: {e:?}")); + for sql in [ + "UPDATE sqlite_master SET name = 'renamed' WHERE name = 'items'", + "DELETE FROM sqlite_master WHERE name = 'items'", + ] { + let refused = service + .execute( + &space, + "main", + ddl(sql), + None, + "tinycloud.sql/write".to_string(), + ) + .await; + assert!( + refused.is_err(), + "{name}: write must not run {sql}: {refused:?}" + ); + } + assert_eq!(table_count(&service, &space).await, 2, "{name}"); + } + } + + /// TC-1586: an admin request that is allowed a PRAGMA (the database header + /// values) leaves the connection as it found it, and the growth guard + /// still works on the next request. + #[tokio::test] + async fn an_allowed_admin_pragma_leaves_the_growth_guard_working() { + for (name, memory_threshold) in [ + ("sql-pragma-guard-mem", u64::MAX), + ("sql-pragma-guard-file", 0), + ] { + let repo = artifact_repository().await; + let cache = TempDir::new().unwrap(); + let service = SqlService::new( + cache.path().to_string_lossy().to_string(), + memory_threshold, + repo, + ); + let space = test_space_id(name); + seeded_items(&service, &space).await; + + service + .execute( + &space, + "main", + ddl("PRAGMA user_version = 7"), + None, + "tinycloud.sql/admin".to_string(), + ) + .await + .unwrap_or_else(|e| panic!("{name}: user_version is allowed: {e:?}")); + assert_eq!( + pragma_value(&service, &space, "user_version").await, + SqlValue::Integer(7), + "{name}" + ); + assert_eq!( + pragma_value(&service, &space, "foreign_keys").await, + SqlValue::Integer(1), + "{name}" + ); + + let grown = service + .execute_without_growth( + &space, + "main", + SqlRequest::Execute { + schema: None, + sql: "INSERT INTO items (name) VALUES (?)".to_string(), + params: vec![SqlValue::Text("y".repeat(256 * 1024))], + }, + None, + "tinycloud.sql/write".to_string(), + ) + .await; + assert!( + matches!(grown, Err(SqlError::StorageWouldGrow)), + "{name}: the growth guard must still refuse growth: {grown:?}" + ); + assert_eq!( + table_count(&service, &space).await, + 1, + "{name}: refused write undone" + ); + service + .execute_without_growth( + &space, + "main", + ddl("DELETE FROM items"), + None, + "tinycloud.sql/write".to_string(), + ) + .await + .unwrap_or_else(|e| panic!("{name}: a shrinking write passes: {e:?}")); + } + } + + /// TC-1586: when the reset after a PRAGMA fails, the actor is retired + /// rather than serving another request on a connection in unknown state. + /// The next request rebuilds it from the durable copy, and nothing + /// persisted before is lost. + #[tokio::test] + async fn a_failed_reset_retires_the_actor() { + for (name, memory_threshold) in [ + ("sql-reset-fails-mem", u64::MAX), + ("sql-reset-fails-file", 0), + ] { + let repo = artifact_repository().await; + let cache = TempDir::new().unwrap(); + let service = SqlService::new( + cache.path().to_string_lossy().to_string(), + memory_threshold, + repo, + ); + let space = test_space_id(name); + seeded_items(&service, &space).await; + + let key = (space.to_string(), "main".to_string()); + crate::sql::database::RESET_FAILURES + .lock() + .unwrap() + .push(key.clone()); + let result = service + .execute( + &space, + "main", + ddl("PRAGMA user_version = 5"), + None, + "tinycloud.sql/admin".to_string(), + ) + .await; + crate::sql::database::RESET_FAILURES + .lock() + .unwrap() + .retain(|failing| *failing != key); + assert!( + matches!(result, Err(SqlError::Internal(_))), + "{name}: the request must fail closed: {result:?}" + ); + + service + .execute( + &space, + "main", + ddl("INSERT INTO items (name) VALUES ('after')"), + None, + "tinycloud.sql/write".to_string(), + ) + .await + .unwrap_or_else(|e| panic!("{name}: the next request rebuilds the actor: {e:?}")); + assert_eq!(table_count(&service, &space).await, 2, "{name}"); + } + } + /// TC-626: on a full space, writes that cannot grow the database run and /// a growing write is undone. Covers both an in-memory database and one /// promoted to a WAL-backed file (threshold 0), which is what large @@ -1417,7 +2063,7 @@ mod tests { sql: sql.to_string(), params, }; - let ability = || "tinycloud.sql/write".to_string(); + let ability = || "tinycloud.sql/admin".to_string(); for row in ["a", "b", "c"] { service .execute( @@ -1601,7 +2247,7 @@ mod tests { params: vec![], }, None, - "tinycloud.sql/write".into(), + "tinycloud.sql/admin".into(), ) .await .unwrap(); @@ -1634,7 +2280,7 @@ mod tests { "main", SqlRequest::Batch { statements }, None, - "tinycloud.sql/write".into(), + "tinycloud.sql/admin".into(), ) .await; assert!(matches!(rejected, Err(SqlError::StorageWouldGrow))); @@ -1652,7 +2298,7 @@ mod tests { params: vec![], }, None, - "tinycloud.sql/write".into(), + "tinycloud.sql/admin".into(), ) .await .unwrap(); @@ -1730,7 +2376,7 @@ mod tests { params: Vec::new(), }, None, - "tinycloud.sql/write".to_string(), + "tinycloud.sql/admin".to_string(), ) .await .expect("grow database"); @@ -1746,7 +2392,7 @@ mod tests { params: Vec::new(), }, None, - "tinycloud.sql/write".to_string(), + "tinycloud.sql/admin".to_string(), ) .await .expect("small write"); @@ -1782,7 +2428,7 @@ mod tests { params: vec![SqlValue::Text("durable".to_string())], }, None, - "tinycloud.sql/write".to_string(), + "tinycloud.sql/admin".to_string(), ) .await .unwrap(); @@ -1796,7 +2442,7 @@ mod tests { params: vec![SqlValue::Text("updated".to_string())], }, None, - "tinycloud.sql/write".to_string(), + "tinycloud.sql/admin".to_string(), ) .await .unwrap(); @@ -1876,7 +2522,7 @@ mod tests { params: Vec::new(), }, None, - "tinycloud.sql/write".to_string(), + "tinycloud.sql/admin".to_string(), ) .await .unwrap(); @@ -1920,7 +2566,7 @@ mod tests { params: Vec::new(), }, None, - "tinycloud.sql/write".to_string(), + "tinycloud.sql/admin".to_string(), ) .await .unwrap(); @@ -2122,7 +2768,7 @@ mod tests { params: Vec::new(), }, None, - "tinycloud.sql/write".to_string(), + "tinycloud.sql/admin".to_string(), ) .await .unwrap(); @@ -2278,7 +2924,7 @@ mod tests { "a", insert(value), None, - "tinycloud.sql/write".into(), + "tinycloud.sql/admin".into(), ) }; write("first").await.unwrap(); @@ -2419,21 +3065,21 @@ mod tests { } }; for db in ["a", "b"] { - run(db, insert("seed"), "tinycloud.sql/write") + run(db, insert("seed"), "tinycloud.sql/admin") .await .unwrap(); } // Persistence fails: the write is refused as retryable. repo.failures.store(1, Ordering::SeqCst); - let err = run("a", insert("lost"), "tinycloud.sql/write") + let err = run("a", insert("lost"), "tinycloud.sql/admin") .await .expect_err("a write whose persist failed must fail"); assert!(matches!(err, SqlError::Unavailable), "{err:?}"); // The lock was released and the failed write's local state // discarded: the next write on `a` re-hydrates and succeeds, // and `b` was never affected. - run("a", insert("retried"), "tinycloud.sql/write") + run("a", insert("retried"), "tinycloud.sql/admin") .await .unwrap(); run("b", count(), "tinycloud.sql/read").await.unwrap(); @@ -2512,7 +3158,7 @@ mod tests { params: Vec::new(), }, None, - "tinycloud.sql/write".to_string(), + "tinycloud.sql/admin".to_string(), ) .await .expect_err("write must fail when durable save fails"); @@ -2574,7 +3220,7 @@ mod tests { "connectors", update("First"), None, - "tinycloud.sql/write".into(), + "tinycloud.sql/admin".into(), ) .await .unwrap(); @@ -2584,7 +3230,7 @@ mod tests { "connectors", update("Stale"), None, - "tinycloud.sql/write".into() + "tinycloud.sql/admin".into() ) .await .is_err()); @@ -2594,7 +3240,7 @@ mod tests { "connectors", update("Recovered"), None, - "tinycloud.sql/write".into(), + "tinycloud.sql/admin".into(), ) .await .unwrap(); @@ -2724,7 +3370,7 @@ mod tests { "a", insert("seed"), None, - "tinycloud.sql/write".into(), + "tinycloud.sql/admin".into(), ) .await .unwrap(); @@ -2772,7 +3418,7 @@ mod tests { // Another database in the same space is unaffected. service - .execute(&space, "b", insert("x"), None, "tinycloud.sql/write".into()) + .execute(&space, "b", insert("x"), None, "tinycloud.sql/admin".into()) .await .unwrap(); drop(held); @@ -2882,7 +3528,7 @@ mod tests { "a", insert("late"), None, - "tinycloud.sql/write".into(), + "tinycloud.sql/admin".into(), ) .await }, @@ -2928,7 +3574,7 @@ mod tests { "a", insert("second"), None, - "tinycloud.sql/write".into(), + "tinycloud.sql/admin".into(), ) .await }); @@ -3004,7 +3650,7 @@ mod tests { "a", insert(value), None, - "tinycloud.sql/write".into(), + "tinycloud.sql/admin".into(), ) .await .unwrap(); @@ -3023,7 +3669,7 @@ mod tests { "a", insert("once"), None, - "tinycloud.sql/write".into(), + "tinycloud.sql/admin".into(), ), ) .await; @@ -3255,7 +3901,7 @@ mod tests { "a", insert("late"), None, - "tinycloud.sql/write".into(), + "tinycloud.sql/admin".into(), ), ) .await diff --git a/tinycloud-node-server/src/config.rs b/tinycloud-node-server/src/config.rs index 4f58a82f..8e2c2dc2 100644 --- a/tinycloud-node-server/src/config.rs +++ b/tinycloud-node-server/src/config.rs @@ -14,6 +14,7 @@ use serde_with::{ use std::{fs, path::PathBuf, time::Duration}; use tinycloud_core::db_pool::{StatementTimeoutMode, StatementTimeouts}; use tinycloud_core::keys::StaticSecret; +use tinycloud_core::sql::DdlPolicy; /// The OpenCredentials service that consumes a node-authorized generic /// credential-invitation receipt in production. Keep this exact origin pinned: @@ -1162,6 +1163,13 @@ pub struct SqlStorageConfig { /// callers that gave up on it, so the database still warms. #[serde(default = "default_sql_hydration_max_ms")] pub hydration_max_ms: u64, + /// Whether a schema-changing statement (CREATE, ALTER, DROP of tables, + /// views and indexes) needs `tinycloud.sql/schema` (TC-1581). `sql/write` + /// covers data statements only. `"enforce"` (the default) refuses the + /// statement; `"log"` runs it and records it without any request content; + /// `"off"` keeps the earlier behaviour, where `write` also sufficed. + #[serde(default)] + pub ddl_requires_schema: DdlPolicy, } fn default_sql_memory_threshold() -> ByteUnit { @@ -1204,6 +1212,7 @@ impl Default for SqlStorageConfig { hydration_wait_timeout_ms: default_sql_hydration_wait_timeout_ms(), artifact_transfer_timeout_ms: default_sql_artifact_transfer_timeout_ms(), hydration_max_ms: default_sql_hydration_max_ms(), + ddl_requires_schema: DdlPolicy::default(), } } } @@ -1646,6 +1655,40 @@ mod tests { use std::io::Write; use tempfile::NamedTempFile; + /// TC-1581: `[global.storage.sql] ddl_requires_schema` defaults to + /// `enforce` and accepts only `off`, `log` and `enforce`. + #[test] + fn tc1581_ddl_requires_schema_defaults_to_enforce_and_parses_its_values() { + assert_eq!( + SqlStorageConfig::default().ddl_requires_schema, + DdlPolicy::Enforce + ); + assert_eq!( + serde_json::from_str::("{}") + .unwrap() + .ddl_requires_schema, + DdlPolicy::Enforce + ); + for (value, expected) in [ + ("off", DdlPolicy::Off), + ("log", DdlPolicy::Log), + ("enforce", DdlPolicy::Enforce), + ] { + let config: SqlStorageConfig = + serde_json::from_str(&format!(r#"{{"ddl_requires_schema":"{value}"}}"#)).unwrap(); + assert_eq!(config.ddl_requires_schema, expected, "{value}"); + } + for invalid in ["warn", "true", "Enforce", ""] { + assert!( + serde_json::from_str::(&format!( + r#"{{"ddl_requires_schema":"{invalid}"}}"# + )) + .is_err(), + "{invalid:?} must be rejected" + ); + } + } + #[test] fn tc1132_startup_validates_retry_budget_bounds() { for attempts in [0, 1, 10] { diff --git a/tinycloud-node-server/src/lib.rs b/tinycloud-node-server/src/lib.rs index 2657ae26..4a65c225 100644 --- a/tinycloud-node-server/src/lib.rs +++ b/tinycloud-node-server/src/lib.rs @@ -711,7 +711,8 @@ pub async fn app_with_control( tinycloud_config.storage.sql.memory_threshold.as_u64(), database_artifact_repository.clone(), ) - .with_wait_bounds(tinycloud_config.storage.sql.wait_bounds()); + .with_wait_bounds(tinycloud_config.storage.sql.wait_bounds()) + .with_ddl_policy(tinycloud_config.storage.sql.ddl_requires_schema); let mut policy_v3_runtime = PolicyV3Runtime::new(seed_conn.clone(), key_setup.node_did(), key_setup.clone()) diff --git a/tinycloud-node-server/src/routes/mod.rs b/tinycloud-node-server/src/routes/mod.rs index 36c14f16..e5810911 100644 --- a/tinycloud-node-server/src/routes/mod.rs +++ b/tinycloud-node-server/src/routes/mod.rs @@ -55,7 +55,7 @@ use tinycloud_core::{ }, request_cancel, sea_orm::{ColumnTrait, DbErr, EntityTrait, QueryFilter, QueryOrder}, - sql::{SqlCaveats, SqlError, SqlRequest, SqlService}, + sql::{SqlAuthority, SqlCaveats, SqlError, SqlRequest, SqlService}, storage::{HashBuffer, ImmutableReadStore, ImmutableStaging}, types::{Ability, DelegationQuery, DelegationQueryPage, Metadata, Resource}, util::{Capability, DelegationInfo, InvocationInfo, RevocationInfo}, @@ -2547,20 +2547,27 @@ async fn read_json_body_limited( /// Classify a SQL request as write-class for the storage quota gate. /// -/// Write-class = `!is_read_only` from `validate_sql`, gating DML + DDL -/// uniformly. A parse/permission error is NOT a quota decision: -/// `sql_service.execute` re-validates and surfaces the true error, so -/// unparseable SQL is treated as non-write here (never masked as 402). +/// Write-class = `!is_read_only` from `classify_sql`, gating DML + DDL +/// uniformly. The answer does not depend on the caller's abilities (TC-1581): +/// a statement the caller may not run is still a write, so a denial is never +/// read as "not a write". Only a statement that does not parse is treated as +/// non-write here (never masked as 402): `sql_service.execute` re-validates +/// and surfaces the true error. /// `ExecuteStatement` resolves the named statement from the SAME caveats /// the execution path uses (chain caveat first, invocation facts as /// fallback) and classifies its SQL: on the facts fallback the statement /// body is invoker-controlled, so it must be parsed, never assumed /// read-only. An unresolvable name is non-write (execution fails with /// "statement not found" before any storage grows). `Export` is a read. -fn sql_request_is_write(request: &SqlRequest, caveats: &Option, ability: &str) -> bool { +/// `_ability` is not consulted; it keeps the signature of the duckdb twin. +fn sql_request_is_write( + request: &SqlRequest, + caveats: &Option, + _ability: &str, +) -> bool { let is_write_sql = |sql: &str| { - tinycloud_core::sql::parser::validate_sql(sql, caveats, ability) - .map(|p| !p.is_read_only) + tinycloud_core::sql::parser::classify_sql(sql) + .map(|class| !class.is_read_only) .unwrap_or(false) }; match request { @@ -2641,6 +2648,11 @@ async fn handle_sql_invoke( let body_str = body_result?; let (space, path, ability) = select_database_scope(sql_caps, "sql")?; + // TC-1581: the preferred ability above only gates the publication + // compatibility command. Statement authorization sees every SQL ability + // the invocation holds, so a `[write, schema]` request can run both data + // and schema-changing statements. + let authority = SqlAuthority::new(sql_caps.iter().map(|(_, _, held)| held.as_str())); // N3 will resolve this logical identity through an explicit alias table. // Do not fall back to the legacy final-segment selector here. let space_id = space.to_string(); @@ -2729,23 +2741,17 @@ async fn handle_sql_invoke( meeting_publication::execute(tinycloud, sql_service, space, command).await } else if full_space.is_some() { sql_service - .execute_without_growth( + .execute_without_growth_authorized( space, &db_name, sql_request, exec_caveats, - ability.to_string(), + authority, ) .await } else { sql_service - .execute( - space, - &db_name, - sql_request, - exec_caveats, - ability.to_string(), - ) + .execute_authorized(space, &db_name, sql_request, exec_caveats, authority) .await }; crate::prometheus::observe_span( @@ -4901,7 +4907,7 @@ mod tests { params: vec![SqlValue::Text("alpha".to_string()), SqlValue::Integer(111)], }, None, - "tinycloud.sql/write".to_string(), + "tinycloud.sql/admin".to_string(), ) .await .unwrap(); @@ -5009,7 +5015,7 @@ mod tests { params: vec![SqlValue::Text("alpha".to_string()), SqlValue::Integer(111)], }, None, - "tinycloud.sql/write".to_string(), + "tinycloud.sql/admin".to_string(), ) .await?; sql_service @@ -5282,7 +5288,7 @@ mod tests { params: vec![SqlValue::Text("alpha".to_string()), SqlValue::Integer(111)], }, None, - "tinycloud.sql/write".to_string(), + "tinycloud.sql/admin".to_string(), ) .await?; sql_service @@ -6603,6 +6609,52 @@ mod tests { assert!(!sql_request_is_write(&SqlRequest::Export, caveats, ability)); } + /// TC-1581: classification does not depend on the abilities held, so a + /// statement the caller may not run (DDL under `write`) is still a write + /// for the storage-quota gate, not "unparseable, therefore not a write". + #[tokio::test] + async fn sql_request_is_write_counts_ddl_whoever_may_run_it() { + use tinycloud_core::sql::types::SqlStatement; + let caveats = &None; + for ddl in [ + "CREATE TABLE t (a)", + "ALTER TABLE t ADD COLUMN b", + "DROP TABLE t", + "CREATE INDEX i ON t (a)", + "CREATE VIEW v AS SELECT 1", + ] { + assert!(sql_request_is_write( + &SqlRequest::Query { + sql: ddl.to_string(), + params: vec![], + max_rows: None, + max_bytes: None, + }, + caveats, + "tinycloud.sql/write", + )); + assert!(sql_request_is_write( + &SqlRequest::Execute { + schema: None, + sql: ddl.to_string(), + params: vec![], + }, + caveats, + "tinycloud.sql/write", + )); + assert!(sql_request_is_write( + &SqlRequest::Batch { + statements: vec![SqlStatement { + sql: ddl.to_string(), + params: vec![], + }], + }, + caveats, + "tinycloud.sql/write", + )); + } + } + /// A metered SQL HTTP stack: one shared `SqlSizes` feeds both the SQL /// artifact save path (via `SizeTrackingArtifactRepository`) and /// `TinyCloud::store_size` (via `.with_sql_sizes`), so a direct SQL @@ -6694,7 +6746,7 @@ mod tests { params: vec![SqlValue::Text("alpha".to_string()), SqlValue::Integer(111)], }, None, - "tinycloud.sql/write".to_string(), + "tinycloud.sql/admin".to_string(), ) .await?; @@ -6743,10 +6795,15 @@ mod tests { None, ); - // Grant BOTH read and write with EMPTY caveats on the same parent - // delegation so either invocation ability is authorized and the raw - // request reaches the gate unconstrained. - for ability in ["tinycloud.sql/read", "tinycloud.sql/write"] { + // Grant read, write AND schema with EMPTY caveats on the same parent + // delegation so any invocation ability (or a `[write, schema]` + // pair) is authorized and the raw request reaches the gate + // unconstrained. + for ability in [ + "tinycloud.sql/read", + "tinycloud.sql/write", + "tinycloud.sql/schema", + ] { abilities::ActiveModel { delegation: Set(parent_hash), resource: Set(Resource::TinyCloud(resource.clone())), @@ -6792,16 +6849,29 @@ mod tests { ability: &str, nonce: &str, facts: Vec, + ) -> Result { + sql_invocation_header_for_abilities(setup, &[ability], nonce, facts) + } + + /// One invocation carrying several SQL abilities on the database, as the + /// SDK's `[write, schema]` requests do. + fn sql_invocation_header_for_abilities( + setup: &MeteredSqlHttp, + abilities: &[&str], + nonce: &str, + facts: Vec, ) -> Result { use tinycloud_auth::ssi::{dids::DIDURLBuf, ucan::Payload}; use tinycloud_auth::ucan_capabilities_object::Capabilities; let mut invocation_caps = Capabilities::new(); - invocation_caps.with_action( - setup.resource.as_uri(), - ability.parse::()?, - [std::collections::BTreeMap::::new()], - ); + for ability in abilities { + invocation_caps.with_action( + setup.resource.as_uri(), + ability.parse::()?, + [std::collections::BTreeMap::::new()], + ); + } let invocation = Payload { issuer: setup.verification_method.parse::()?, audience: setup @@ -7023,10 +7093,12 @@ mod tests { let usage_header = space_info_invocation_header(&mut setup, "urn:uuid:tc626-full-usage").await?; let write = |nonce: &str| sql_invocation_header(&setup, "tinycloud.sql/write", nonce); - let ensure = write("urn:uuid:tc626-full-ensure")?; + // TC-1581: schema-changing statements ride a `schema` invocation. + let schema = |nonce: &str| sql_invocation_header(&setup, "tinycloud.sql/schema", nonce); + let ensure = schema("urn:uuid:tc626-full-ensure")?; let grow = write("urn:uuid:tc626-full-grow")?; let delete = write("urn:uuid:tc626-full-delete")?; - let drop_table = write("urn:uuid:tc626-full-drop")?; + let drop_table = schema("urn:uuid:tc626-full-drop")?; let count = sql_invocation_header(&setup, "tinycloud.sql/read", "urn:uuid:tc626-full-count")?; @@ -7117,6 +7189,345 @@ mod tests { Ok(()) } + /// TC-1581 over `/invoke`: an invocation holding `write` alone cannot run + /// a schema-changing statement, on any request path; one holding + /// `[write, schema]` (the SDK's shape) runs data and schema statements in + /// one request; `schema` alone runs schema statements only. + #[tokio::test] + async fn tc1581_invoke_write_alone_cannot_change_the_schema() -> Result<()> { + use rocket::data::ByteUnit; + use rocket::http::{ContentType, Header, Status}; + use rocket::local::asynchronous::Client; + + const WRITE: &str = "tinycloud.sql/write"; + const SCHEMA: &str = "tinycloud.sql/schema"; + let setup = metered_sql_http_setup("tc1581-invoke").await?; + let mut nonce = 0u32; + let mut header = |abilities: &[&str]| -> Result { + nonce += 1; + sql_invocation_header_for_abilities( + &setup, + abilities, + &format!("urn:uuid:tc1581-{nonce}"), + Vec::new(), + ) + }; + let write_only = [header(&[WRITE])?, header(&[WRITE])?, header(&[WRITE])?]; + let write_only_more = [ + header(&[WRITE])?, + header(&[WRITE])?, + header(&[WRITE])?, + header(&[WRITE])?, + ]; + let both = [header(&[WRITE, SCHEMA])?, header(&[WRITE, SCHEMA])?]; + let both_more = [header(&[WRITE, SCHEMA])?, header(&[WRITE, SCHEMA])?]; + let schema_only = [header(&[SCHEMA])?, header(&[SCHEMA])?]; + let read = header(&["tinycloud.sql/read"])?; + + let client = Client::tracked(metered_sql_rocket(setup, ByteUnit::Mebibyte(64))).await?; + let send = |auth: &String, request: &SqlRequest| { + let client = &client; + let auth = auth.clone(); + let body = serde_json::to_string(request); + async move { + let response = client + .post("/invoke") + .header(Header::new("Authorization", auth)) + .header(ContentType::JSON) + .body(body.expect("request serializes")) + .dispatch() + .await; + let status = response.status(); + (status, response.into_string().await.unwrap_or_default()) + } + }; + let execute = |sql: &str, schema: Option<&str>| SqlRequest::Execute { + sql: sql.to_string(), + params: vec![], + schema: schema.map(|s| vec![s.to_string()]), + }; + + // `write` alone: data statements run, schema-changing ones are 403. + let (status, body) = send( + &write_only[0], + &execute("INSERT INTO labels (label, val) VALUES ('beta', 2)", None), + ) + .await; + assert_eq!(status, Status::Ok, "write runs data statements: {body}"); + for (auth, request) in write_only[1..].iter().zip([ + execute("DROP TABLE labels", None), + execute("CREATE TABLE intruder (id INTEGER)", None), + ]) { + let (status, body) = send(auth, &request).await; + assert_eq!(status, Status::Forbidden, "{request:?}: {body}"); + assert!(body.contains("tinycloud.sql/schema"), "{body}"); + } + for (auth, request) in write_only_more.iter().zip([ + execute("ALTER TABLE labels ADD COLUMN extra TEXT", None), + // Request-level `schema` entries are judged like any statement. + execute( + "INSERT INTO labels (label, val) VALUES ('gamma', 3)", + Some("CREATE TABLE IF NOT EXISTS intruder (id INTEGER)"), + ), + SqlRequest::Batch { + statements: vec![tinycloud_core::sql::types::SqlStatement { + sql: "CREATE INDEX labels_val ON labels (val)".into(), + params: vec![], + }], + }, + SqlRequest::Query { + sql: "DROP VIEW IF EXISTS labels".into(), + params: vec![], + max_rows: None, + max_bytes: None, + }, + ]) { + let (status, body) = send(auth, &request).await; + assert_eq!(status, Status::Forbidden, "{request:?}: {body}"); + } + + // `[write, schema]` in one request: the migration shape. + let (status, body) = send( + &both[0], + &execute( + "INSERT INTO extra (v) VALUES ('x')", + Some("CREATE TABLE IF NOT EXISTS extra (v TEXT)"), + ), + ) + .await; + assert_eq!(status, Status::Ok, "[write, schema] mixed execute: {body}"); + let (status, body) = send(&both[1], &execute("DROP TABLE extra", None)).await; + assert_eq!(status, Status::Ok, "[write, schema] drop: {body}"); + let (status, body) = send( + &both_more[0], + &execute("ALTER TABLE labels ADD COLUMN note TEXT", None), + ) + .await; + assert_eq!(status, Status::Ok, "[write, schema] alter: {body}"); + + // `schema` alone: schema statements only. + let (status, body) = send( + &schema_only[0], + &execute("CREATE TABLE IF NOT EXISTS from_schema (id INTEGER)", None), + ) + .await; + assert_eq!(status, Status::Ok, "schema runs DDL: {body}"); + let (status, body) = send( + &schema_only[1], + &execute("INSERT INTO labels (label, val) VALUES ('delta', 4)", None), + ) + .await; + assert_eq!( + status, + Status::Forbidden, + "schema alone writes nothing: {body}" + ); + + // The refused statements changed nothing: `labels` kept its rows + // and `intruder` was never created. + let (status, body) = send( + &read, + &SqlRequest::Query { + sql: "SELECT (SELECT count(*) FROM labels), \ + (SELECT count(*) FROM sqlite_schema WHERE name = 'intruder')" + .to_string(), + params: vec![], + max_rows: None, + max_bytes: None, + }, + ) + .await; + assert_eq!(status, Status::Ok, "{body}"); + assert_eq!( + serde_json::from_str::(&body)?["rows"], + serde_json::json!([[2, 0]]), + "{body}" + ); + Ok(()) + } + + /// TC-1585 over `/invoke`: a `WITH … INSERT/UPDATE` is a write. On a full + /// space the quota gate refuses a growing one (it used to be classed + /// read-only and skip the gate), a non-growing one still runs, and a + /// read-tier grant is refused. + #[tokio::test] + async fn tc1585_cte_wrapped_writes_are_quota_gated_and_refused_to_read() -> Result<()> { + use rocket::data::ByteUnit; + use rocket::http::{ContentType, Header, Status}; + use rocket::local::asynchronous::Client; + + let setup = metered_sql_http_setup("tc1585-cte").await?; + let mut nonce = 0u32; + let mut header = |ability: &str| -> Result { + nonce += 1; + sql_invocation_header(&setup, ability, &format!("urn:uuid:tc1585-{nonce}")) + }; + let growing_insert_auth = header("tinycloud.sql/write")?; + let growing_update_auth = header("tinycloud.sql/write")?; + let cte_update = header("tinycloud.sql/write")?; + let read_cte_insert = header("tinycloud.sql/read")?; + let count = header("tinycloud.sql/read")?; + + let client = Client::tracked(metered_sql_rocket(setup, ByteUnit::Byte(1))).await?; + let send = |auth: &String, request: &SqlRequest| { + let client = &client; + let auth = auth.clone(); + let body = serde_json::to_string(request); + async move { + let response = client + .post("/invoke") + .header(Header::new("Authorization", auth)) + .header(ContentType::JSON) + .body(body.expect("request serializes")) + .dispatch() + .await; + let status = response.status(); + (status, response.into_string().await.unwrap_or_default()) + } + }; + let execute = |sql: &str, params: Vec| SqlRequest::Execute { + sql: sql.to_string(), + params, + schema: None, + }; + + // Growing CTE-wrapped writes on a full space are refused with 402. + let big = || vec![SqlValue::Text("g".repeat(64 * 1024))]; + for (auth, sql) in [ + ( + &growing_insert_auth, + "WITH c AS (SELECT ? AS body) INSERT INTO labels (label, val) SELECT body, 1 FROM c", + ), + ( + &growing_update_auth, + "WITH c AS (SELECT ? AS body) UPDATE labels SET label = (SELECT body FROM c)", + ), + ] { + let (status, body) = send(auth, &execute(sql, big())).await; + assert_eq!(status, Status::new(402), "{sql}: {body}"); + assert!(body.contains("storage_quota_exceeded"), "{body}"); + } + + // A non-growing CTE-wrapped write still runs on a full space. + let (status, body) = send( + &cte_update, + &execute( + "WITH c AS (SELECT 5 AS bump) UPDATE labels SET val = val + (SELECT bump FROM c)", + vec![], + ), + ) + .await; + assert_eq!(status, Status::Ok, "{body}"); + + // A read-tier grant cannot run one. + let (status, body) = send( + &read_cte_insert, + &execute( + "WITH c AS (SELECT 'r' AS body) INSERT INTO labels (label, val) SELECT body, 1 FROM c", + vec![], + ), + ) + .await; + assert_eq!(status, Status::Forbidden, "{body}"); + + // The refused statements changed nothing: the seeded row (val 111) + // was bumped once by the CTE-wrapped update that ran. + let (status, body) = send( + &count, + &SqlRequest::Query { + sql: "SELECT count(*), sum(val) FROM labels".to_string(), + params: vec![], + max_rows: None, + max_bytes: None, + }, + ) + .await; + assert_eq!(status, Status::Ok, "{body}"); + assert_eq!( + serde_json::from_str::(&body)?["rows"], + serde_json::json!([[1, 116]]), + "{body}" + ); + Ok(()) + } + + /// TC-1585: a CTE-wrapped write reports the table it changes, so the route + /// publishes the same write event as for a plain write (the events are + /// built from the service's `write_targets`; before the fix these writes + /// reported none and published nothing). + #[tokio::test] + async fn tc1585_cte_wrapped_writes_report_targets_that_become_write_events() -> Result<()> { + let service = fresh_sql_service().await; + let space = test_space_id("tc1585-hooks"); + let hooks = HookRuntime::new(HooksConfig::default(), [5u8; 32]); + let mut events = hooks.bus().subscribe(); + let run = |sql: &str, schema: Option<&str>| { + let service = service.clone(); + let space = space.clone(); + let request = SqlRequest::Execute { + sql: sql.to_string(), + params: vec![], + schema: schema.map(|s| vec![s.to_string()]), + }; + async move { + service + .execute_authorized( + &space, + "main", + request, + None, + SqlAuthority::new(["tinycloud.sql/write", "tinycloud.sql/schema"]), + ) + .await + } + }; + + run( + "CREATE TABLE notes (id INTEGER PRIMARY KEY, body TEXT)", + None, + ) + .await?; + + let mut published = Vec::new(); + for (sql, schema, what) in [ + ( + "INSERT INTO notes (body) VALUES ('plain')", + None, + "plain insert", + ), + ( + "WITH c AS (SELECT 'cte' AS body) INSERT INTO notes (body) SELECT body FROM c", + None, + "cte insert", + ), + ( + "WITH c AS (SELECT 'cte2' AS body) UPDATE notes SET body = (SELECT body FROM c)", + None, + "cte update", + ), + ] { + let result = run(sql, schema).await?; + let batch = database_write_events( + &space.to_string(), + "sql", + Some("main.db"), + "did:key:test", + what, + "2026-01-01T00:00:00Z", + &result.write_targets, + ); + assert_eq!(batch.len(), 1, "{what}: one event for the one table"); + publish_database_hook_events(&hooks, &batch); + let event = timeout(Duration::from_secs(1), events.recv()).await??; + assert_eq!(event.table_name.as_deref(), Some("notes"), "{what}"); + assert_eq!(event.ability, "tinycloud.sql/write", "{what}"); + assert_eq!(event.database_path.as_deref(), Some("main.db"), "{what}"); + published.push(what); + } + assert_eq!(published.len(), 3); + Ok(()) + } + /// TC-626: billing's account totals reach both the usage read and the /// 402 body, and the quota fetch carries the node's service secret. #[tokio::test] @@ -8672,7 +9083,9 @@ mod tests { &signer, &issuer_vm, "sql", - "tinycloud.sql/write", + // The body creates a table and inserts into it. `admin` carries + // both: schema changes need `schema` (TC-1581), data needs `write`. + "tinycloud.sql/admin", None, nonce, )?; @@ -9416,13 +9829,19 @@ mod tests { } async fn owner_sql(&self, path: &str, request: SqlRequest) -> Result { - let action = if matches!(request, SqlRequest::Query { .. }) { - "tinycloud.sql/read" + // The owner holds every ability: a mutating request carries + // `write` and `schema` like the SDK's `execute({ schema })`. + let actions: &[&str] = if matches!(request, SqlRequest::Query { .. }) { + &["tinycloud.sql/read"] } else { - "tinycloud.sql/write" + &["tinycloud.sql/write", "tinycloud.sql/schema"] }; let uri = self.resource_in(&self.space, "sql", path)?.to_string(); - self.invoke_json(self.owner_invocation(&[(uri, action)])?, &request) + let capabilities: Vec<(String, &str)> = actions + .iter() + .map(|action| (uri.clone(), *action)) + .collect(); + self.invoke_json(self.owner_invocation(&capabilities)?, &request) .await } diff --git a/tinycloud-node-server/tests/postgres_pool_timeouts.rs b/tinycloud-node-server/tests/postgres_pool_timeouts.rs index 85bf76b0..d1a151fe 100644 --- a/tinycloud-node-server/tests/postgres_pool_timeouts.rs +++ b/tinycloud-node-server/tests/postgres_pool_timeouts.rs @@ -224,7 +224,7 @@ async fn write( db, insert_blob(bytes), None, - "tinycloud.sql/write".into(), + "tinycloud.sql/admin".into(), ) .await .map(|_| ()) diff --git a/tinycloud-node-server/tests/postgres_round_trip_budget.rs b/tinycloud-node-server/tests/postgres_round_trip_budget.rs index 6d789559..d41daa43 100644 --- a/tinycloud-node-server/tests/postgres_round_trip_budget.rs +++ b/tinycloud-node-server/tests/postgres_round_trip_budget.rs @@ -765,13 +765,27 @@ fn authorization(headers: &impl serde::Serialize) -> Result { } fn invocation(session: &Session, service: &str, path: &str, ability: &str) -> Result { + invocation_with(session, service, path, &[ability]) +} + +/// An invocation that holds several abilities on one resource, as the SDK +/// sends for the account index (`write` for its rows, `schema` for its DDL). +fn invocation_with( + session: &Session, + service: &str, + path: &str, + abilities: &[&str], +) -> Result { let invocation = session.invoke( [( service.parse::()?, path.parse::()?, None, None, - [ability.parse::()?], + abilities + .iter() + .map(|ability| ability.parse::()) + .collect::, _>>()?, )], None, )?; @@ -846,6 +860,7 @@ async fn round(node: &Node, account: &Account, out: &mut Vec) -> Resul vec![ "tinycloud.sql/read".parse()?, "tinycloud.sql/write".parse()?, + "tinycloud.sql/schema".parse()?, ], )]), ), @@ -899,7 +914,12 @@ async fn round(node: &Node, account: &Account, out: &mut Vec) -> Resul .post("/invoke") .header(Header::new( "Authorization", - invocation(&session, "sql", ACCOUNT_DATABASE, "tinycloud.sql/write")?, + invocation_with( + &session, + "sql", + ACCOUNT_DATABASE, + &["tinycloud.sql/write", "tinycloud.sql/schema"], + )?, )) .header(ContentType::JSON) .body(serde_json::to_string(&sql)?), diff --git a/tinycloud-node-server/tests/request_telemetry_authenticated.rs b/tinycloud-node-server/tests/request_telemetry_authenticated.rs index 53d8e483..0e3280ac 100644 --- a/tinycloud-node-server/tests/request_telemetry_authenticated.rs +++ b/tinycloud-node-server/tests/request_telemetry_authenticated.rs @@ -541,6 +541,7 @@ fn sign_in(node: &Node, account: &Account) -> Result { vec![ "tinycloud.sql/read".parse()?, "tinycloud.sql/write".parse()?, + "tinycloud.sql/schema".parse()?, ], )]), ), @@ -775,7 +776,7 @@ fn authenticated_kv_sql_failure_privacy_forwarding_and_restart() -> Result<()> { &session, "sql", DATABASE, - "tinycloud.sql/write", + "tinycloud.sql/schema", Some(&create), None, ); diff --git a/tinycloud-node-server/tests/space_size_restart.rs b/tinycloud-node-server/tests/space_size_restart.rs index 6a3e45b1..62eebac1 100644 --- a/tinycloud-node-server/tests/space_size_restart.rs +++ b/tinycloud-node-server/tests/space_size_restart.rs @@ -262,7 +262,7 @@ impl Account { let now = time::OffsetDateTime::now_utc(); let mut abilities = json!({ "kv": {"": ["tinycloud.kv/put", "tinycloud.kv/get", "tinycloud.kv/del"]}, - "sql": {"main": ["tinycloud.sql/read", "tinycloud.sql/write"]}, + "sql": {"main": ["tinycloud.sql/read", "tinycloud.sql/write", "tinycloud.sql/schema"]}, }); if cfg!(feature = "duckdb") { abilities["duckdb"] = @@ -371,7 +371,7 @@ async fn sql_write(client: &Client, session: &Session) -> Result { session, "sql", "main", - "tinycloud.sql/write", + "tinycloud.sql/schema", ContentType::JSON, json!({"action": "execute", "sql": "CREATE TABLE t (v TEXT)", "params": []}) .to_string() diff --git a/tinycloud.toml b/tinycloud.toml index 7f646f00..4f5a143b 100644 --- a/tinycloud.toml +++ b/tinycloud.toml @@ -26,6 +26,13 @@ cors = true # hydration_wait_timeout_ms = 30000 # artifact_transfer_timeout_ms = 45000 # hydration_max_ms = 600000 # a hydration keeps warming after its callers give up + ## Schema-changing SQL (CREATE/ALTER/DROP of tables, views and indexes) needs + ## tinycloud.sql/schema (TC-1581); tinycloud.sql/write covers data statements. + ## "enforce" (default) refuses the statement, "log" runs it and records it + ## (a constant warning and a counter by statement kind, no request content), + ## "off" restores the earlier rule that write also suffices for those + ## statements; the release's other restrictions apply in every mode. See docs/sql-schema-authority.md. + # ddl_requires_schema = "enforce" # [global.storage.duckdb] # path = "./data/duckdb"