diff --git a/backend/.sqlx/query-904c81997a1594ca998a4b1af5a5710bcbe114153f9096d0e2e59e429bd5a325.json b/backend/.sqlx/query-904c81997a1594ca998a4b1af5a5710bcbe114153f9096d0e2e59e429bd5a325.json new file mode 100644 index 0000000000..14a20fa112 --- /dev/null +++ b/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" +} diff --git a/backend/tests/script_auto_parent_archived.rs b/backend/tests/script_auto_parent_archived.rs index 5115c516c2..58b926b9c4 100644 --- a/backend/tests/script_auto_parent_archived.rs +++ b/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, +) -> 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>> = 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, +) -> 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>, 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) -> 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(()) +} diff --git a/backend/windmill-api-scripts/src/scripts.rs b/backend/windmill-api-scripts/src/scripts.rs index 97e3019835..a4667e4876 100644 --- a/backend/windmill-api-scripts/src/scripts.rs +++ b/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, })), }; diff --git a/backend/windmill-types/src/scripts.rs b/backend/windmill-types/src/scripts.rs index 19f6a06021..9a059da6e8 100644 --- a/backend/windmill-types/src/scripts.rs +++ b/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); diff --git a/cli/TESTING.md b/cli/TESTING.md index 235a95c4b1..b7a1e701b8 100644 --- a/cli/TESTING.md +++ b/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 diff --git a/cli/src/commands/script/script.ts b/cli/src/commands/script/script.ts index d2afdeeee3..c44d9928f6 100644 --- a/cli/src/commands/script/script.ts +++ b/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", diff --git a/cli/test/schedule_push_permissioned_as_unit.test.ts b/cli/test/schedule_push_permissioned_as_unit.test.ts index 941415409d..17a1731494 100644 --- a/cli/test/schedule_push_permissioned_as_unit.test.ts +++ b/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][] = []; +async function mockModule( + specifier: string, + factory: (real: Record) => Record +): Promise { + const real = { ...((await import(specifier)) as Record) }; + 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 () => ({}), }));