Merge remote-tracking branch 'origin/datatable-roles-redesign' into datatable-roles-redesign

backend/.sqlx/query-904c81997a1594ca998a4b1af5a5710bcbe114153f9096d0e2e59e429bd5a325.json

@@ -0,0 +1,23 @@
+{
+  "db_name": "PostgreSQL",
+  "query": "SELECT 1 FROM script WHERE parent_hashes[1] = $1 AND workspace_id = $2",
+  "describe": {
+    "columns": [
+      {
+        "ordinal": 0,
+        "name": "?column?",
+        "type_info": "Int4"
+      }
+    ],
+    "parameters": {
+      "Left": [
+        "Int8",
+        "Text"
+      ]
+    },
+    "nullable": [
+      null
+    ]
+  },
+  "hash": "904c81997a1594ca998a4b1af5a5710bcbe114153f9096d0e2e59e429bd5a325"
+}

backend/tests/script_auto_parent_archived.rs

@@ -9,8 +9,9 @@
 //! `lineage must be linear: no 2 scripts can have the same parent` error
 //! whenever that archived hash already had a child from the prior rename.
 //!
-//! The fix clears `parent_hash` to `None` in that case so the push starts a
-//! fresh lineage instead of failing.
+//! Resolution only ever adopts an archived version nothing else descends from,
+//! so here it finds none and leaves `parent_hash` at `None`, starting a fresh
+//! lineage instead of failing.
 
 use serde_json::json;
 use sqlx::{Pool, Postgres};
@@ -146,3 +147,221 @@ async fn test_auto_parent_starts_fresh_lineage_when_all_versions_archived(
 
     Ok(())
 }
+
+/// A path whose only versions are archived and childless — what archiving a path
+/// leaves behind, and what a sync push that applies a deletion before the matching
+/// update sees. Resolving `auto_parent` to no parent there hashes the deploy exactly
+/// as the path's first version was hashed, so an unchanged push is rejected as a
+/// duplicate of it.
+#[sqlx::test(fixtures("base"))]
+async fn test_auto_parent_adopts_archived_head_instead_of_colliding(
+    db: Pool<Postgres>,
+) -> anyhow::Result<()> {
+    initialize_tracing().await;
+
+    let server = ApiServer::start(db.clone()).await?;
+    let port = server.addr.port();
+    let base = format!("http://localhost:{port}/api/w/test-workspace");
+
+    let path = "u/test-user/script_archived_head";
+    let body = new_script(path, "export async function main() { return 1; }");
+
+    let resp = authed(
+        client().post(format!("{base}/scripts/create")),
+        "SECRET_TOKEN",
+    )
+    .json(&body)
+    .send()
+    .await?;
+    assert_eq!(resp.status(), 201);
+    let first_hash: i64 =
+        sqlx::query_scalar("SELECT hash FROM script WHERE path = $1 AND workspace_id = $2")
+            .bind(path)
+            .bind("test-workspace")
+            .fetch_one(&db)
+            .await?;
+
+    let resp = authed(
+        client().post(format!("{base}/scripts/archive/p/{path}")),
+        "SECRET_TOKEN",
+    )
+    .send()
+    .await?;
+    assert_eq!(resp.status(), 200, "archiving the path should succeed");
+
+    // Byte-identical to the first deploy: the collision this guards against needs the
+    // pushed body to hash the same way the original one did.
+    let resp = authed(
+        client().post(format!("{base}/scripts/create?skip_if_noop=true")),
+        "SECRET_TOKEN",
+    )
+    .json(&{
+        let mut push = body.clone();
+        push["auto_parent"] = json!(true);
+        push
+    })
+    .send()
+    .await?;
+    let status = resp.status();
+    let response_body = resp.text().await?;
+    assert_eq!(
+        status, 201,
+        "re-pushing an archived path must not collide with its own first version, \
+         got {status}: {response_body}"
+    );
+
+    let active: Vec<Option<Vec<i64>>> = sqlx::query_scalar(
+        "SELECT parent_hashes FROM script \
+         WHERE path = $1 AND archived = false AND workspace_id = $2",
+    )
+    .bind(path)
+    .bind("test-workspace")
+    .fetch_all(&db)
+    .await?;
+    assert_eq!(active.len(), 1, "exactly one active version expected");
+    assert_eq!(
+        active[0].as_deref(),
+        Some(&[first_hash][..]),
+        "the revived version should continue the archived lineage"
+    );
+
+    Ok(())
+}
+
+/// The same redeploy from a caller that names no parent at all — the shape a retried
+/// `wmill sync push` takes, once the archive it applied has committed and the path has
+/// dropped out of the listing it diffs against.
+///
+/// The adopted version supplies the lineage and nothing else: a path is reusable by a
+/// different script, which must not start life holding grants nobody gave it.
+#[sqlx::test(fixtures("base"))]
+async fn test_parentless_redeploy_adopts_the_lineage_but_not_the_grants(
+    db: Pool<Postgres>,
+) -> anyhow::Result<()> {
+    initialize_tracing().await;
+
+    let server = ApiServer::start(db.clone()).await?;
+    let port = server.addr.port();
+    let base = format!("http://localhost:{port}/api/w/test-workspace");
+
+    let path = "u/test-user/script_retired_parentless";
+    let body = new_script(path, "export async function main() { return 1; }");
+
+    let resp = authed(
+        client().post(format!("{base}/scripts/create")),
+        "SECRET_TOKEN",
+    )
+    .json(&body)
+    .send()
+    .await?;
+    assert_eq!(resp.status(), 201);
+    let first_hash: i64 =
+        sqlx::query_scalar("SELECT hash FROM script WHERE path = $1 AND workspace_id = $2")
+            .bind(path)
+            .bind("test-workspace")
+            .fetch_one(&db)
+            .await?;
+
+    sqlx::query("UPDATE script SET extra_perms = $1 WHERE hash = $2 AND workspace_id = $3")
+        .bind(json!({ "u/someone_else": true }))
+        .bind(first_hash)
+        .bind("test-workspace")
+        .execute(&db)
+        .await?;
+
+    let resp = authed(
+        client().post(format!("{base}/scripts/archive/p/{path}")),
+        "SECRET_TOKEN",
+    )
+    .send()
+    .await?;
+    assert_eq!(resp.status(), 200, "archiving the path should succeed");
+
+    // Byte-identical to the first deploy and naming no parent: hashed as a first deploy it
+    // lands on the archived row.
+    let resp = authed(
+        client().post(format!("{base}/scripts/create?skip_if_noop=true")),
+        "SECRET_TOKEN",
+    )
+    .json(&body)
+    .send()
+    .await?;
+    let status = resp.status();
+    let response_body = resp.text().await?;
+    assert_eq!(
+        status, 201,
+        "a parentless redeploy of a retired path must not collide with its own first \
+         version, got {status}: {response_body}"
+    );
+
+    let live: Vec<(Option<Vec<i64>>, serde_json::Value)> = sqlx::query_as(
+        "SELECT parent_hashes, extra_perms FROM script \
+         WHERE path = $1 AND archived = false AND workspace_id = $2",
+    )
+    .bind(path)
+    .bind("test-workspace")
+    .fetch_all(&db)
+    .await?;
+    assert_eq!(live.len(), 1, "exactly one live version expected");
+    assert_eq!(
+        live[0].0.as_deref(),
+        Some(&[first_hash][..]),
+        "the redeploy should continue the retired lineage"
+    );
+    assert_eq!(
+        live[0].1,
+        json!({}),
+        "an adopted version's grants must not carry over to whatever reuses its path"
+    );
+
+    Ok(())
+}
+
+/// A soft delete keeps the row, and with it the hash the version was deployed under, so a
+/// tombstone has to stay adoptable: skip it and the redeploy hashes straight back onto it.
+#[sqlx::test(fixtures("base"))]
+async fn test_redeploy_chains_past_a_deleted_version(db: Pool<Postgres>) -> anyhow::Result<()> {
+    initialize_tracing().await;
+
+    let server = ApiServer::start(db.clone()).await?;
+    let port = server.addr.port();
+    let base = format!("http://localhost:{port}/api/w/test-workspace");
+
+    let path = "u/test-user/script_deleted_version";
+    let body = new_script(path, "export async function main() { return 1; }");
+
+    let resp = authed(
+        client().post(format!("{base}/scripts/create")),
+        "SECRET_TOKEN",
+    )
+    .json(&body)
+    .send()
+    .await?;
+    assert_eq!(resp.status(), 201);
+    let deleted_hash = resp.text().await?;
+
+    let resp = authed(
+        client().post(format!("{base}/scripts/delete/h/{deleted_hash}")),
+        "SECRET_TOKEN",
+    )
+    .send()
+    .await?;
+    assert_eq!(resp.status(), 200, "deleting the version should succeed");
+
+    let resp = authed(
+        client().post(format!("{base}/scripts/create?skip_if_noop=true")),
+        "SECRET_TOKEN",
+    )
+    .json(&body)
+    .send()
+    .await?;
+    let status = resp.status();
+    let response_body = resp.text().await?;
+    assert_eq!(
+        status, 201,
+        "redeploying the content of a deleted version must not collide with its \
+         tombstone, got {status}: {response_body}"
+    );
+
+    Ok(())
+}

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

@@ -1318,6 +1318,47 @@ async fn create_script_internal<'c>(
         }
     }
 
+    // A retired path keeps its versions, and the newest is where a redeploy belongs: hashed
+    // as a first deploy instead, unchanged content lands on the row the path's own first
+    // version already holds. A deleted version still counts — its row keeps the hash it was
+    // deployed under even once the content is wiped, so skipping it is what collides.
+    //
+    // Any parentless deploy, not only an `auto_parent` one: the CLI names no parent for a
+    // path its listing no longer shows, which is where a retried push lands. Gated on
+    // nothing being live there, so a parentless deploy onto a live path still meets the
+    // path conflict the match below raises.
+    let mut parent_adopted_from_retired_path = false;
+    if ns.parent_hash.is_none() && clashing_script.is_none() {
+        // Locked, not merely read: a competing deploy chaining onto this same candidate
+        // takes `FOR UPDATE` on it before inserting, so holding the row is what serializes
+        // the two. Probe first and the child still uncommitted reads as absent.
+        let candidate = sqlx::query_scalar::<_, i64>(
+            "SELECT hash FROM script WHERE path = $1 AND workspace_id = $2 \
+             ORDER BY created_at DESC LIMIT 1 FOR UPDATE",
+        )
+        .bind(&ns.path)
+        .bind(&w_id)
+        .fetch_optional(&mut *tx)
+        .await?;
+        // Adoptable only if nothing already descends from it: a rename leaves its source
+        // path holding a version whose child lives at the destination, and a second child
+        // forks a lineage the guard below requires to be linear. Nothing adoptable means a
+        // fresh lineage, which has no parent to vary its hash and can still collide.
+        ns.parent_hash = match candidate {
+            Some(hash) => sqlx::query_scalar!(
+                "SELECT 1 FROM script WHERE parent_hashes[1] = $1 AND workspace_id = $2",
+                hash,
+                &w_id
+            )
+            .fetch_optional(&db)
+            .await?
+            .is_none()
+            .then_some(ScriptHash(hash)),
+            None => None,
+        };
+        parent_adopted_from_retired_path = ns.parent_hash.is_some();
+    }
+
     // Must stay below the parent resolution above: an auto_parent deploy hashed before
     // it carries a first deploy's lineage, so redeploying content the path has held
     // before collides with that archived version instead of superseding it. The
@@ -1362,20 +1403,42 @@ async fn create_script_internal<'c>(
                 ));
             };
 
+            // Unscoped, and sound only under the lock above: linearity is a property of the
+            // lineage, not of what this caller may read. A child can sit where they cannot
+            // see it — a folder they renamed it into, or grants an adopting deploy reset —
+            // and asked through `tx` it reads as absent, letting the fork through.
             let clashing_hash_o = sqlx::query_scalar!(
                 "SELECT hash FROM script WHERE parent_hashes[1] = $1 AND workspace_id = $2",
                 p_hash.0,
                 &w_id
             )
-            .fetch_optional(&mut *tx)
+            .fetch_optional(&db)
             .await?;
 
             if let Some(clashing_hash) = clashing_hash_o {
-                return Err(Error::BadRequest(format!(
-                    "A script with hash {} with same parent_hash has been found. However, the \
-                         lineage must be linear: no 2 scripts can have the same parent",
-                    ScriptHash(clashing_hash)
-                )));
+                // Named only when the caller could already read it. The probe above has to be
+                // unscoped to be correct, but a hash alone reads a script's content back
+                // through `raw/h/{hash}`, which authorizes nothing per script — so echoing one
+                // the caller cannot see hands them a way to fetch it.
+                let visible_to_caller = sqlx::query_scalar!(
+                    "SELECT 1 FROM script WHERE hash = $1 AND workspace_id = $2",
+                    clashing_hash,
+                    &w_id
+                )
+                .fetch_optional(&mut *tx)
+                .await?
+                .is_some();
+                return Err(Error::BadRequest(if visible_to_caller {
+                    format!(
+                        "A script with hash {} with same parent_hash has been found. However, \
+                         the lineage must be linear: no 2 scripts can have the same parent",
+                        ScriptHash(clashing_hash)
+                    )
+                } else {
+                    "A script with the same parent_hash has been found. However, the lineage \
+                     must be linear: no 2 scripts can have the same parent"
+                        .to_owned()
+                }));
             };
 
             let ScriptWithStarred { script: ps, .. } =
@@ -1438,7 +1501,15 @@ async fn create_script_internal<'c>(
                 }
                 Some(_) | None => Ok(Some(ParentInfo {
                     p_hashes: ph,
-                    perms: ps.extra_perms,
+                    // A version adopted above was taken for its lineage, not its grants: a
+                    // retired path may be reused by a different script, which must not start
+                    // life holding an ACL nobody gave it — including one `delete/h` purged.
+                    // A parent the caller named still carries them, as unarchive expects.
+                    perms: if parent_adopted_from_retired_path {
+                        json!({})
+                    } else {
+                        ps.extra_perms
+                    },
                     p_path: ps.path,
                 })),
             };

backend/windmill-types/src/scripts.rs

@@ -612,6 +612,7 @@ impl Hash for NewScript {
         self.priority.hash(state);
         self.timeout.hash(state);
         self.delete_after_use.hash(state);
+        self.delete_after_secs.hash(state);
         self.restart_unless_cancelled.hash(state);
         self.deployment_message.hash(state);
         self.visible_to_runner_only.hash(state);

cli/TESTING.md

@@ -22,6 +22,12 @@ Pure local tests — no backend, no database. Uses `bunfig.unit.toml` (no preloa
 
 Examples: `git_unit`, `lint_command_unit`, `tar_creation_unit`, `workspace_conflicts_unit`
 
+`mock.module()` mocks the module for the whole `bun test` process, not for the file
+that installs it, and `mock.restore()` does not undo it. A file that mocks a module
+must hand back its real exports in `afterAll` (see
+`schedule_push_permissioned_as_unit`), or it silently rewires whichever file runs
+next — and the run order is the directory's, so it differs between Linux and Windows.
+
 ### Integration tests
 
 Require a running backend and PostgreSQL. The `setup.ts` preload builds the backend

cli/src/commands/script/script.ts

@@ -753,14 +753,10 @@ export async function handleFile(
     // create_script (which would bump the script hash) and instead route
     // through /acls/* via applyExtraPermsDiff.
     //
-    // No refetch is needed:
-    //  - folder perms are additive at auth time, never merged onto item rows;
-    //  - the body sent to create_script doesn't carry extra_perms, so a fresh
-    //    deploy of an existing path inherits the previous version's perms
-    //    unchanged. The diff against `remote` (captured before the deploy)
-    //    therefore matches what `wmill acl remove` would do — and the granular
-    //    ACL endpoint updates every matching row, so the inheritance on the
-    //    new version doesn't leave ghost entries.
+    // No refetch is needed: folder perms are additive at auth time and never merged
+    // onto item rows, and each branch above leaves the new version's perms where the
+    // diff expects them — the update branch names a parent, which carries them over,
+    // while the create branch has no `remote` to diff against and sends the whole set.
     await applyExtraPermsDiff(
       workspaceId,
       "script",

cli/test/schedule_push_permissioned_as_unit.test.ts

@@ -5,7 +5,7 @@
  * that builds none silently reassigns the schedule to whoever ran it.
  */
 
-import { expect, test, describe, beforeEach, mock } from "bun:test";
+import { expect, test, describe, afterAll, beforeEach, mock } from "bun:test";
 import { mkdtemp, writeFile } from "node:fs/promises";
 import { tmpdir } from "node:os";
 import { join } from "node:path";
@@ -25,7 +25,27 @@ const REMOTE_SCHEDULE = () => ({
   permissioned_as: remotePermissionedAs,
 });
 
-mock.module("../gen/services.gen.ts", () => ({
+// A module mock is process-global and outlives the file that installs it, and
+// `mock.restore()` does not undo one: every mocked module has to be handed back
+// its real exports here, or whichever file `bun test` happens to run next gets
+// this file's stubs.
+const realModules: [string, Record<string, unknown>][] = [];
+async function mockModule(
+  specifier: string,
+  factory: (real: Record<string, unknown>) => Record<string, unknown>
+): Promise<void> {
+  const real = { ...((await import(specifier)) as Record<string, unknown>) };
+  realModules.push([specifier, real]);
+  mock.module(specifier, () => factory(real));
+}
+
+afterAll(() => {
+  for (const [specifier, real] of realModules) {
+    mock.module(specifier, () => real);
+  }
+});
+
+await mockModule("../gen/services.gen.ts", () => ({
   getSchedule: async () => REMOTE_SCHEDULE(),
   updateSchedule: async (a: unknown) => {
     updateScheduleCalls.push(a);
@@ -38,9 +58,8 @@ mock.module("../gen/services.gen.ts", () => ({
   }),
 }));
 
-const realContext = await import("../src/core/context.ts");
-mock.module("../src/core/context.ts", () => ({
-  ...realContext,
+await mockModule("../src/core/context.ts", (real) => ({
+  ...real,
   resolveWorkspace: async () => ({
     workspaceId: "w",
     name: "w",
@@ -49,9 +68,8 @@ mock.module("../src/core/context.ts", () => ({
   }),
 }));
 
-const realAuth = await import("../src/core/auth.ts");
-mock.module("../src/core/auth.ts", () => ({
-  ...realAuth,
+await mockModule("../src/core/auth.ts", (real) => ({
+  ...real,
   requireLogin: async () => ({}),
 }));