From 533670f577b47c8688fcab1946fc7843e76beeb5 Mon Sep 17 00:00:00 2001 From: Ruben Fiszel Date: Fri, 16 Oct 2026 23:34:40 +0200 Subject: [PATCH] fix the protection rules race and the Windows nsjail profile test (#11835) * test: run the isolated-forks rule test in its own workspace PROTECTION_RULES_CACHE is process-wide and keyed by workspace id, so the other base-fixture tests in protection_rules.rs, running concurrently on their own databases, could cache test-workspace's rules without the RequireIsolatedForks ruleset and let the fork through to the data table copy (400 instead of 403) on main. Co-Authored-By: Claude Opus 5.5 * test: match nsjail profile lines so the envar check passes on CRLF checkouts script_profiles_take_envars_before_their_fixed_ones searched for "\n{ENVARS}\n", which a Windows checkout (CRLF) never contains, failing cargo_test_windows since v1.834.0. Co-Authored-By: Claude Opus 5.5 --------- Co-authored-by: Claude Opus 5.5 --- backend/tests/protection_rules.rs | 22 ++++++++++++++-------- backend/windmill-worker/src/common.rs | 9 ++++++--- 2 files changed, 20 insertions(+), 11 deletions(-) diff --git a/backend/tests/protection_rules.rs b/backend/tests/protection_rules.rs index 6a96a66f8f..bb9ef29fa2 100644 --- a/backend/tests/protection_rules.rs +++ b/backend/tests/protection_rules.rs @@ -692,21 +692,30 @@ async fn test_protection_rule_rename(db: Pool) -> anyhow::Result<()> { #[sqlx::test(fixtures("base"))] async fn test_require_isolated_forks(db: Pool) -> anyhow::Result<()> { initialize_tracing().await; - invalidate_protection_rules_cache("test-workspace"); + // Not `test-workspace`: PROTECTION_RULES_CACHE is process-wide and keyed by workspace id, so + // the other tests here, running concurrently on their own databases, would overwrite its rules. + invalidate_protection_rules_cache("iso-ws"); // `dt2` is a clone governed from `gov`, but its database is this workspace's; `dt3` points // into `gov`'s database. let pg = r#"{"resource_type": "postgresql", "resource_path": "u/test-user/pg"}"#; let gov = r#"{"workspace_id": "gov", "datatable": "src"}"#; sqlx::raw_sql( - "INSERT INTO workspace (id, name, owner) VALUES ('gov', 'gov', 'test-user'); - INSERT INTO workspace_settings (workspace_id) VALUES ('gov')", + "INSERT INTO workspace (id, name, owner) VALUES + ('gov', 'gov', 'test-user'), ('iso-ws', 'iso-ws', 'test-user'); + INSERT INTO workspace_settings (workspace_id) VALUES ('gov'), ('iso-ws'); + INSERT INTO workspace_key (workspace_id, kind, key) VALUES ('iso-ws', 'cloud', 'iso-key'); + INSERT INTO group_ (workspace_id, name, summary, extra_perms) VALUES + ('iso-ws', 'all', 'All users', '{}'); + INSERT INTO usr (workspace_id, email, username, is_admin, role) VALUES + ('iso-ws', 'test@windmill.dev', 'test-user', true, 'Admin'), + ('iso-ws', 'test2@windmill.dev', 'test-user-2', false, 'User')", ) .execute(&db) .await?; for (w_id, datatables) in [ ("gov", format!(r#"{{"src": {{"database": {pg}}}}}"#)), ( - "test-workspace", + "iso-ws", format!( r#"{{"dt1": {{"database": {pg}}}, "dt2": {{"database": {pg}, "governed_by": {gov}}}, "dt3": {{"reference": {gov}}}}}"# @@ -724,10 +733,7 @@ async fn test_require_isolated_forks(db: Pool) -> anyhow::Result<()> { } let server = ApiServer::start(db.clone()).await?; - let base = format!( - "http://localhost:{}/api/w/test-workspace", - server.addr.port() - ); + let base = format!("http://localhost:{}/api/w/iso-ws", server.addr.port()); let resp = authed( client().post(format!("{base}/workspaces/protection_rules")), "SECRET_TOKEN", diff --git a/backend/windmill-worker/src/common.rs b/backend/windmill-worker/src/common.rs index 63c5d8d647..32bd6dc7e9 100644 --- a/backend/windmill-worker/src/common.rs +++ b/backend/windmill-worker/src/common.rs @@ -2771,11 +2771,14 @@ mod tests { include_str!("../nsjail/download.ruby.config.proto"), ), ] { - let at = profile - .find("\n{ENVARS}\n") + // Line by line: a Windows checkout gives the profiles CRLF endings. + let lines = profile.lines().collect::>(); + let at = lines + .iter() + .position(|l| *l == "{ENVARS}") .unwrap_or_else(|| panic!("{name}: no {{ENVARS}}")); assert!( - !profile[..at].contains("\nenvar:"), + !lines[..at].iter().any(|l| l.starts_with("envar:")), "{name}: a fixed envar precedes {{ENVARS}}" ); }