fix(datatables): cut revoked-role logins, drop shared createrole widening, serialize teardown and shared-db checks

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013fBWmrMs2DbpU1iYdCvGX6

backend/windmill-api-workspaces/src/datatable_permissions_api.rs

@@ -26,6 +26,14 @@ use windmill_common::workspaces::{
 };
 use windmill_common::{PgDatabase, DB};
 
+/// Transaction-scoped advisory lock serializing every shared-database
+/// exclusivity check with the config write it protects. The scan in
+/// [`check_shared_database_forbid`] is a plain read: without mutual exclusion,
+/// two concurrent writers targeting the same physical database could both
+/// observe no conflict and both commit.
+pub(crate) const SHARED_DB_CHECK_LOCK: &str =
+    "SELECT pg_advisory_xact_lock(hashtextextended('wm_dt_shared_db_check', 0))";
+
 pub(crate) fn routes() -> Router {
     Router::new()
         .route(
@@ -212,12 +220,17 @@ async fn set_datatable_permissions(
         check_datatable_permissions_ee_license().await?;
     }
     validate_permissions_config(&perms)?;
+
+    let mut tx = db.begin().await?;
     if perms.enabled {
+        // Serialize the exclusivity scan with every other writer of datatable
+        // configs (same lock in `edit_datatable_config`): two concurrent
+        // enables/creations targeting the same physical database must not both
+        // pass the pre-write check. Held until commit.
+        sqlx::query(SHARED_DB_CHECK_LOCK).execute(&mut *tx).await?;
         check_shared_database_forbid(&db, &w_id, &datatable_name, &config.database, true).await?;
         check_owner_can_create_roles(&db, &w_id, &config).await?;
     }
-
-    let mut tx = db.begin().await?;
     let args_for_audit = format!("{:?}", perms);
     audit_log(
         &mut *tx,

backend/windmill-api-workspaces/src/workspaces.rs

@@ -3035,6 +3035,11 @@ async fn edit_datatable_config(
     // physical database (enforcement is per database, config is per data
     // table). Check every data table whose database is new or changed, against
     // both the stored global state and the other entries of this request.
+    // Serialized with every other config writer (held until commit) so two
+    // concurrent saves can't both pass the exclusivity scan.
+    sqlx::query(crate::datatable_permissions_api::SHARED_DB_CHECK_LOCK)
+        .execute(&mut *tx)
+        .await?;
     let mut default_disabled: Vec<String> = vec![];
     let mut database_changed_names: Vec<String> = vec![];
     for (name, dt) in new_config.settings.datatables.iter() {

backend/windmill-common/src/datatable_permissions.rs

@@ -547,7 +547,10 @@ async fn harden_instance_databases(db: &DB, target_dbname: &str) -> Result<()> {
 
 /// Ensure the caller's ephemeral role exists with up-to-date grants and a
 /// fresh sliding expiry. Returns `(role_name, cleartext_password)`.
-pub async fn ensure_ephemeral_role(
+/// Deliberately private: live credentials must only flow out through
+/// [`get_datatable_resource_from_db_checked`], which performs the
+/// authorization this function assumes already happened.
+async fn ensure_ephemeral_role(
     db: &DB,
     w_id: &str,
     datatable: &str,
@@ -796,6 +799,7 @@ async fn cleanup_one_expired_role(db: &DB, role: &str, w_id: &str, datatable: &s
                 .map_err(|e| pg_err("checking active sessions", e))?
                 .get(0);
             if active > 0 {
+                disable_role_login(&client, role).await;
                 return Ok(());
             }
             if role_exists(&client, role).await? {
@@ -862,7 +866,37 @@ pub async fn drop_datatable_ephemeral_roles_best_effort(
     }
 }
 
+/// Strip a live role's ability to open new connections. An ordinary role can
+/// `ALTER ROLE CURRENT_USER PASSWORD ...` — so a caller keeping a session open
+/// could otherwise reconnect with a self-chosen password long after
+/// revocation. Only CREATEROLE can restore LOGIN, so this closes the
+/// reconnect vector while letting in-flight queries finish; the kept
+/// bookkeeping row makes the expiry sweep finish the drop later.
+async fn disable_role_login(client: &tokio_postgres::Client, role: &str) {
+    if !role.starts_with(DATATABLE_EPHEMERAL_ROLE_PREFIX) {
+        return;
+    }
+    if let Err(e) = client
+        .batch_execute(&format!(
+            "ALTER ROLE {} NOLOGIN CONNECTION LIMIT 0 PASSWORD NULL",
+            quote_ident(role)
+        ))
+        .await
+    {
+        tracing::warn!("disabling login of ephemeral role {role}: {e:#}");
+    }
+}
+
 async fn teardown_role(db: &DB, w_id: &str, datatable: &str, role: &str) -> Result<()> {
+    // Same per-role lock as role creation and the expiry sweep: without it,
+    // teardown could observe a stale role while a resolver is recreating it
+    // and drop the fresh role right before its bookkeeping row lands, leaving
+    // callers with credentials for a nonexistent role.
+    let mut tx = db.begin().await?;
+    sqlx::query("SELECT pg_advisory_xact_lock(hashtextextended('wm_dt_role:' || $1, 0))")
+        .bind(role)
+        .execute(&mut *tx)
+        .await?;
     if let Ok(config) = get_datatable_config(db, w_id, datatable).await {
         let owner: PgDatabase =
             serde_json::from_value(datatable_shared_resource(db, w_id, &config).await?)
@@ -881,7 +915,8 @@ async fn teardown_role(db: &DB, w_id: &str, datatable: &str, role: &str) -> Resu
             // and deleting the row would permanently orphan a live role that
             // still holds its old grants (and whose password the caller
             // already has). With the row intact, the expiry sweep retries the
-            // drop once the sessions are gone.
+            // drop once the sessions are gone; meanwhile new logins are cut.
+            disable_role_login(&client, role).await;
             return Ok(());
         }
         if role_exists(&client, role).await? {
@@ -895,8 +930,9 @@ async fn teardown_role(db: &DB, w_id: &str, datatable: &str, role: &str) -> Resu
         "DELETE FROM datatable_ephemeral_role WHERE role_name = $1",
         role
     )
-    .execute(db)
+    .execute(&mut *tx)
     .await?;
+    tx.commit().await?;
     Ok(())
 }
 

backend/windmill-common/src/lib.rs

@@ -1203,18 +1203,21 @@ pub async fn create_custom_instance_database(
 
     if let Err(e) = client
         .batch_execute(&format!(
-            // REVOKE PUBLIC CONNECT + CREATEROLE support the fine-grained data
-            // table permissions feature: ephemeral per-user roles must not be
-            // able to hop to other databases on the cluster, and the shared
-            // owner role creates/drops them.
+            // REVOKE PUBLIC CONNECT supports the fine-grained data table
+            // permissions feature: ephemeral per-user roles must not be able
+            // to hop to other databases on the cluster. CREATEROLE for the
+            // shared role is deliberately NOT granted here — this path is
+            // reachable by non-superadmins (fork databases), and on
+            // Postgres < 16 CREATEROLE would let arbitrary user SQL running
+            // as the shared role manage unrelated cluster roles; only the
+            // superadmin settings path grants it.
             "REVOKE CONNECT ON DATABASE \"{dbname}\" FROM PUBLIC;
              GRANT CONNECT ON DATABASE \"{dbname}\" TO custom_instance_user;
              GRANT USAGE ON SCHEMA public TO custom_instance_user;
              GRANT CREATE ON SCHEMA public TO custom_instance_user;
              GRANT CREATE ON DATABASE \"{dbname}\" TO custom_instance_user;
              ALTER DEFAULT PRIVILEGES IN SCHEMA public
-                 GRANT SELECT, INSERT, UPDATE, DELETE ON TABLES TO custom_instance_user;
-             ALTER ROLE custom_instance_user CREATEROLE;"
+                 GRANT SELECT, INSERT, UPDATE, DELETE ON TABLES TO custom_instance_user;"
         ))
         .await
     {

backend/windmill-common/src/workspaces.rs

@@ -1114,6 +1114,11 @@ pub async fn get_datatable_config(db: &DB, w_id: &str, name: &str) -> Result<Dat
 
 /// Credentials of the shared owner role for a data table: `custom_instance_user`
 /// for instance-type, the resource's own credentials for external ones.
+///
+/// Authorization: returns full-access credentials and performs no
+/// authorization — callers MUST have already authorized the caller against
+/// the data table (admin check or
+/// `datatable_permissions::get_datatable_resource_from_db_checked`).
 pub async fn datatable_shared_resource(
     db: &DB,
     w_id: &str,
@@ -1122,11 +1127,12 @@ pub async fn datatable_shared_resource(
     datatable_shared_resource_inner(db, w_id, datatable, false).await
 }
 
-/// Same as [`datatable_shared_resource`] but for postgres trigger connections:
-/// custom-instance datatables resolve to `custom_instance_replication_user`
-/// rather than `custom_instance_user`. BYO-postgres datatables resolve to the
-/// user's own resource unchanged; configuring it for replication there is the
-/// user's responsibility.
+/// Same as [`datatable_shared_resource`] (including its authorization
+/// contract) but for postgres trigger connections: custom-instance datatables
+/// resolve to `custom_instance_replication_user` rather than
+/// `custom_instance_user`. BYO-postgres datatables resolve to the user's own
+/// resource unchanged; configuring it for replication there is the user's
+/// responsibility.
 pub async fn datatable_shared_replication_resource(
     db: &DB,
     w_id: &str,