From ebfac29096f12c4da2df45d5d82db83d352f3426 Mon Sep 17 00:00:00 2001 From: Ruben Fiszel Date: Sat, 5 Sep 2026 08:05:15 +0000 Subject: [PATCH 01/13] fix: render the MCP OAuth consent page without a workspace (#10988) Claude-Session: https://claude.ai/code/session_014EeEWKSqcnCEcPKe9uUuHC Co-authored-by: Claude Opus 5 (1M context) --- .../{+page.svelte => +page@(root).svelte} | 12 ++++++++++-- .../oauth/mcp_authorize/layoutReset.test.ts | 14 ++++++++++++++ frontend/src/routes/(root)/+layout.svelte | 5 ++++- 3 files changed, 28 insertions(+), 3 deletions(-) rename frontend/src/routes/(root)/(logged)/oauth/mcp_authorize/{+page.svelte => +page@(root).svelte} (90%) create mode 100644 frontend/src/routes/(root)/(logged)/oauth/mcp_authorize/layoutReset.test.ts diff --git a/frontend/src/routes/(root)/(logged)/oauth/mcp_authorize/+page.svelte b/frontend/src/routes/(root)/(logged)/oauth/mcp_authorize/+page@(root).svelte similarity index 90% rename from frontend/src/routes/(root)/(logged)/oauth/mcp_authorize/+page.svelte rename to frontend/src/routes/(root)/(logged)/oauth/mcp_authorize/+page@(root).svelte index 3750c56d97..830e545719 100644 --- a/frontend/src/routes/(root)/(logged)/oauth/mcp_authorize/+page.svelte +++ b/frontend/src/routes/(root)/(logged)/oauth/mcp_authorize/+page@(root).svelte @@ -1,3 +1,7 @@ + {#if !redirectUriValid} -

Error: invalid or unsafe redirect_uri

+ +

Error: invalid or unsafe redirect_uri

+ {:else if !isGateway && !workspaceId} -

Error: missing workspace_id

+ +

Error: missing workspace_id

+ {:else} {#if success} diff --git a/frontend/src/routes/(root)/(logged)/oauth/mcp_authorize/layoutReset.test.ts b/frontend/src/routes/(root)/(logged)/oauth/mcp_authorize/layoutReset.test.ts new file mode 100644 index 0000000000..6ba4469e27 --- /dev/null +++ b/frontend/src/routes/(root)/(logged)/oauth/mcp_authorize/layoutReset.test.ts @@ -0,0 +1,14 @@ +import { readdirSync } from 'node:fs' +import { dirname } from 'node:path' +import { fileURLToPath } from 'node:url' +import { describe, expect, it } from 'vitest' + +// Renaming the page back drops it into the (logged) layout, where it hangs on +// "Loading user..." with no type error and no other failing test — see the page header. +const routeDir = dirname(fileURLToPath(import.meta.url)) + +describe('mcp oauth consent route', () => { + it('escapes the (logged) layout', () => { + expect(readdirSync(routeDir)).toContain('+page@(root).svelte') + }) +}) diff --git a/frontend/src/routes/(root)/+layout.svelte b/frontend/src/routes/(root)/+layout.svelte index 0e404afbcc..4de44442a2 100644 --- a/frontend/src/routes/(root)/+layout.svelte +++ b/frontend/src/routes/(root)/+layout.svelte @@ -147,7 +147,10 @@ } else { if ( (!page.url.pathname.startsWith('/user/') || page.url.pathname.startsWith('/user/cli')) && - !page.url.pathname.startsWith('/oauth/mcp_authorize') && + // The MCP consent page carries its own workspace picker, so it is left to + // run without one. Nothing sets `$userStore` on this branch, which is why + // that page must stay outside the (logged) layout — see its `@(root)` name. + !page.url.pathname.startsWith(`${base}/oauth/mcp_authorize`) && // The hub import wizard asks for the destination itself, and may end in a // workspace that does not exist yet — bouncing it to the picker would // force the very choice it exists to make. From 9d37b6f489b0263d1a45f8066335ac55c2de7643 Mon Sep 17 00:00:00 2001 From: Ruben Fiszel Date: Sat, 5 Sep 2026 09:10:41 +0000 Subject: [PATCH 02/13] test: keep the mcp preprocessor header test off the dependency job (#10989) Claude-Session: https://claude.ai/code/session_01YESK92Dtojyu4XMg19GHfp Co-authored-by: Claude Opus 5 (1M context) --- .../tests/mcp_preprocessor_headers.rs | 23 ++++++++++++------- 1 file changed, 15 insertions(+), 8 deletions(-) diff --git a/backend/windmill-api-integration-tests/tests/mcp_preprocessor_headers.rs b/backend/windmill-api-integration-tests/tests/mcp_preprocessor_headers.rs index 3e1035ff82..25942e845c 100644 --- a/backend/windmill-api-integration-tests/tests/mcp_preprocessor_headers.rs +++ b/backend/windmill-api-integration-tests/tests/mcp_preprocessor_headers.rs @@ -15,6 +15,12 @@ use windmill_test_utils::*; const SCRIPT_PATH: &str = "u/test-user/mcp_hdr_probe"; +/// A bun lock the executor accepts without installing anything: no dependencies +/// in the `package.json` half, `` for the `bun.lock` half. The empty +/// string is not a substitute: a lock carrying no `//bun.lock` separator is +/// rejected at run time. +const EMPTY_BUN_LOCK: &str = "{}\n//bun.lock\n"; + /// Echoes the two halves of the event separately, so the assertions can tell /// which one a value arrived in. const PREPROCESSOR_SCRIPT: &str = r#" @@ -84,7 +90,7 @@ async fn test_mcp_preprocessor_receives_the_callers_headers( "description": "", "content": PREPROCESSOR_SCRIPT, "language": "bun", - "lock": "", + "lock": EMPTY_BUN_LOCK, "schema": { "$schema": "https://json-schema.org/draft/2020-12/schema", "type": "object", @@ -101,13 +107,14 @@ async fn test_mcp_preprocessor_receives_the_callers_headers( resp.text().await.unwrap_or_default() ); - // A script counts as deployed once it has a lock, which normally arrives from - // a dependency job. Planting an empty one keeps the test to the path under - // test instead of a bun resolution whose timing it does not control. - sqlx::query("UPDATE script SET lock = '' WHERE path = $1 AND workspace_id = 'test-workspace'") - .bind(SCRIPT_PATH) - .execute(&db) - .await?; + // A supplied lock queues no dependency job, so the version is deployed (hence + // listable and runnable) as soon as the create returns. + let queued: i64 = sqlx::query_scalar( + "SELECT count(*) FROM v2_job_queue WHERE workspace_id = 'test-workspace'", + ) + .fetch_one(&db) + .await?; + assert_eq!(queued, 0, "the supplied lock must queue no dependency job"); let tools = mcp_post( port, From 54287102b22dd17903cdd4b48c5828875e5b9be4 Mon Sep 17 00:00:00 2001 From: Ruben Fiszel Date: Sat, 5 Sep 2026 09:11:29 +0000 Subject: [PATCH 03/13] fix: meter WAC compute per segment, not the whole sleep (#10985) * fix: clear started_at when a WAC parent suspends Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01AzS5d5Dc3GL49kWZPQAQCg * fix: restore started_at on the WAC dispatch rollback, fail loudly on a no-op suspend Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01AzS5d5Dc3GL49kWZPQAQCg * fix: restore the pulled segment start on the WAC dispatch rollback Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01AzS5d5Dc3GL49kWZPQAQCg * feat: meter WAC execution per segment instead of only the last one Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01AzS5d5Dc3GL49kWZPQAQCg * fix: make the cloud feature self-sufficient per crate Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01AzS5d5Dc3GL49kWZPQAQCg * chore: name windmill-common/cloud directly in the worker cloud feature Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01AzS5d5Dc3GL49kWZPQAQCg --------- Co-authored-by: Claude Opus 5 (1M context) --- ...1369067c41870be4561e1c26733ac30419801.json | 15 ++++ ...b7ab1f2fc29a2ba79a39576551bdf66b592b6.json | 15 ---- ...7cfd7ac33ac2616d9f68ab4cb62da105cae57.json | 25 ++++++ ...3e6e08f808f423c8f2d58b9c849aba7d176f5.json | 14 ---- ...05e6f5c25e23b7c41ce1c0e5a83bf7b118bfd.json | 22 ++++++ ...2808e90cb068d1048717f82992f476377cc20.json | 15 ---- backend/tests/wac_suspend_started_at.rs | 55 ++++++++++++++ backend/windmill-queue/Cargo.toml | 2 +- backend/windmill-queue/src/jobs.rs | 31 +++++++- backend/windmill-worker/Cargo.toml | 2 +- backend/windmill-worker/src/bun_executor.rs | 74 ++++++++++-------- backend/windmill-worker/src/wac_executor.rs | 76 +++++++++++++++++++ .../src/lib/components/runs/RunRow.svelte | 2 + 13 files changed, 267 insertions(+), 81 deletions(-) create mode 100644 backend/.sqlx/query-0e277240d2be50383ac53d844c71369067c41870be4561e1c26733ac30419801.json delete mode 100644 backend/.sqlx/query-10af387fce25f6ea7af275e8e93b7ab1f2fc29a2ba79a39576551bdf66b592b6.json create mode 100644 backend/.sqlx/query-5dc28b4609bff15ca4895e726c27cfd7ac33ac2616d9f68ab4cb62da105cae57.json delete mode 100644 backend/.sqlx/query-a684f160d1a366c1928fef27c613e6e08f808f423c8f2d58b9c849aba7d176f5.json create mode 100644 backend/.sqlx/query-e12f852fa196d16862151ab490b05e6f5c25e23b7c41ce1c0e5a83bf7b118bfd.json delete mode 100644 backend/.sqlx/query-f56c58fea9f27d2e55d33720e032808e90cb068d1048717f82992f476377cc20.json create mode 100644 backend/tests/wac_suspend_started_at.rs diff --git a/backend/.sqlx/query-0e277240d2be50383ac53d844c71369067c41870be4561e1c26733ac30419801.json b/backend/.sqlx/query-0e277240d2be50383ac53d844c71369067c41870be4561e1c26733ac30419801.json new file mode 100644 index 0000000000..359a7eeefd --- /dev/null +++ b/backend/.sqlx/query-0e277240d2be50383ac53d844c71369067c41870be4561e1c26733ac30419801.json @@ -0,0 +1,15 @@ +{ + "db_name": "PostgreSQL", + "query": "UPDATE v2_job_queue\n SET suspend = 0, suspend_until = NULL,\n started_at = coalesce(started_at, $2, now())\n WHERE id = $1", + "describe": { + "columns": [], + "parameters": { + "Left": [ + "Uuid", + "Timestamptz" + ] + }, + "nullable": [] + }, + "hash": "0e277240d2be50383ac53d844c71369067c41870be4561e1c26733ac30419801" +} diff --git a/backend/.sqlx/query-10af387fce25f6ea7af275e8e93b7ab1f2fc29a2ba79a39576551bdf66b592b6.json b/backend/.sqlx/query-10af387fce25f6ea7af275e8e93b7ab1f2fc29a2ba79a39576551bdf66b592b6.json deleted file mode 100644 index 3f39982319..0000000000 --- a/backend/.sqlx/query-10af387fce25f6ea7af275e8e93b7ab1f2fc29a2ba79a39576551bdf66b592b6.json +++ /dev/null @@ -1,15 +0,0 @@ -{ - "db_name": "PostgreSQL", - "query": "UPDATE v2_job_queue SET suspend = $2, suspend_until = now() + interval '14 day' WHERE id = $1", - "describe": { - "columns": [], - "parameters": { - "Left": [ - "Uuid", - "Int4" - ] - }, - "nullable": [] - }, - "hash": "10af387fce25f6ea7af275e8e93b7ab1f2fc29a2ba79a39576551bdf66b592b6" -} diff --git a/backend/.sqlx/query-5dc28b4609bff15ca4895e726c27cfd7ac33ac2616d9f68ab4cb62da105cae57.json b/backend/.sqlx/query-5dc28b4609bff15ca4895e726c27cfd7ac33ac2616d9f68ab4cb62da105cae57.json new file mode 100644 index 0000000000..a93f92c4e2 --- /dev/null +++ b/backend/.sqlx/query-5dc28b4609bff15ca4895e726c27cfd7ac33ac2616d9f68ab4cb62da105cae57.json @@ -0,0 +1,25 @@ +{ + "db_name": "PostgreSQL", + "query": "WITH prev AS (\n SELECT started_at FROM v2_job_queue WHERE id = $1 AND workspace_id = $2\n )\n UPDATE v2_job_queue q\n SET suspend = $3, suspend_until = now() + make_interval(secs => $4), started_at = null\n FROM prev\n WHERE q.id = $1 AND q.workspace_id = $2\n RETURNING (extract(epoch FROM now() - prev.started_at) * 1000)::bigint", + "describe": { + "columns": [ + { + "ordinal": 0, + "name": "int8", + "type_info": "Int8" + } + ], + "parameters": { + "Left": [ + "Uuid", + "Text", + "Int4", + "Float8" + ] + }, + "nullable": [ + null + ] + }, + "hash": "5dc28b4609bff15ca4895e726c27cfd7ac33ac2616d9f68ab4cb62da105cae57" +} diff --git a/backend/.sqlx/query-a684f160d1a366c1928fef27c613e6e08f808f423c8f2d58b9c849aba7d176f5.json b/backend/.sqlx/query-a684f160d1a366c1928fef27c613e6e08f808f423c8f2d58b9c849aba7d176f5.json deleted file mode 100644 index 7a45e6c402..0000000000 --- a/backend/.sqlx/query-a684f160d1a366c1928fef27c613e6e08f808f423c8f2d58b9c849aba7d176f5.json +++ /dev/null @@ -1,14 +0,0 @@ -{ - "db_name": "PostgreSQL", - "query": "UPDATE v2_job_queue SET running = false, started_at = null WHERE id = $1", - "describe": { - "columns": [], - "parameters": { - "Left": [ - "Uuid" - ] - }, - "nullable": [] - }, - "hash": "a684f160d1a366c1928fef27c613e6e08f808f423c8f2d58b9c849aba7d176f5" -} diff --git a/backend/.sqlx/query-e12f852fa196d16862151ab490b05e6f5c25e23b7c41ce1c0e5a83bf7b118bfd.json b/backend/.sqlx/query-e12f852fa196d16862151ab490b05e6f5c25e23b7c41ce1c0e5a83bf7b118bfd.json new file mode 100644 index 0000000000..ae893b6791 --- /dev/null +++ b/backend/.sqlx/query-e12f852fa196d16862151ab490b05e6f5c25e23b7c41ce1c0e5a83bf7b118bfd.json @@ -0,0 +1,22 @@ +{ + "db_name": "PostgreSQL", + "query": "WITH prev AS (SELECT started_at FROM v2_job_queue WHERE id = $1)\n UPDATE v2_job_queue q SET running = false, started_at = null\n FROM prev WHERE q.id = $1\n RETURNING (extract(epoch FROM now() - prev.started_at) * 1000)::bigint", + "describe": { + "columns": [ + { + "ordinal": 0, + "name": "int8", + "type_info": "Int8" + } + ], + "parameters": { + "Left": [ + "Uuid" + ] + }, + "nullable": [ + null + ] + }, + "hash": "e12f852fa196d16862151ab490b05e6f5c25e23b7c41ce1c0e5a83bf7b118bfd" +} diff --git a/backend/.sqlx/query-f56c58fea9f27d2e55d33720e032808e90cb068d1048717f82992f476377cc20.json b/backend/.sqlx/query-f56c58fea9f27d2e55d33720e032808e90cb068d1048717f82992f476377cc20.json deleted file mode 100644 index 72acab6120..0000000000 --- a/backend/.sqlx/query-f56c58fea9f27d2e55d33720e032808e90cb068d1048717f82992f476377cc20.json +++ /dev/null @@ -1,15 +0,0 @@ -{ - "db_name": "PostgreSQL", - "query": "UPDATE v2_job_queue SET suspend = 1, suspend_until = now() + make_interval(secs => $2) WHERE id = $1", - "describe": { - "columns": [], - "parameters": { - "Left": [ - "Uuid", - "Float8" - ] - }, - "nullable": [] - }, - "hash": "f56c58fea9f27d2e55d33720e032808e90cb068d1048717f82992f476377cc20" -} diff --git a/backend/tests/wac_suspend_started_at.rs b/backend/tests/wac_suspend_started_at.rs new file mode 100644 index 0000000000..f374a25220 --- /dev/null +++ b/backend/tests/wac_suspend_started_at.rs @@ -0,0 +1,55 @@ +//! Guards the two things `suspend_wac_parent` promises: the `started_at` invariant +//! documented on it, and the segment length it hands back for metering. + +use sqlx::{Pool, Postgres}; +use uuid::Uuid; +use windmill_worker::wac_executor::suspend_wac_parent; + +#[sqlx::test] +async fn wac_suspend_clears_started_at(db: Pool) -> anyhow::Result<()> { + let job_id = Uuid::new_v4(); + sqlx::query( + "INSERT INTO v2_job_queue (id, workspace_id, scheduled_for, running, started_at) \ + VALUES ($1, 'test-workspace', now(), true, now() - interval '4 days')", + ) + .bind(job_id) + .execute(&db) + .await?; + + let mut tx = db.begin().await?; + let segment_ms = suspend_wac_parent(&mut tx, &job_id, "test-workspace", 1, 3600.0).await?; + tx.commit().await?; + + // The segment is what gets billed, so it must be the run that just ended, measured + // from the pull — not the park ahead of it, and not zero. + let four_days_ms = 4 * 24 * 3600 * 1000; + assert!( + segment_ms.is_some_and(|ms| (ms - four_days_ms).abs() < 60_000), + "expected the ended segment (~{four_days_ms}ms), got {segment_ms:?}" + ); + + let (started_at, running, suspend, suspend_until): ( + Option>, + bool, + i32, + Option>, + ) = sqlx::query_as( + "SELECT started_at, running, suspend, suspend_until FROM v2_job_queue WHERE id = $1", + ) + .bind(job_id) + .fetch_one(&db) + .await?; + + assert_eq!( + started_at, None, + "a parked parent must not carry the previous segment's started_at" + ); + assert_eq!(suspend, 1); + assert!(suspend_until.is_some()); + assert!( + running, + "running stays true so the normal pull query skips the parked row" + ); + + Ok(()) +} diff --git a/backend/windmill-queue/Cargo.toml b/backend/windmill-queue/Cargo.toml index b9bd6fd28e..6dc5d8ba42 100644 --- a/backend/windmill-queue/Cargo.toml +++ b/backend/windmill-queue/Cargo.toml @@ -12,7 +12,7 @@ path = "src/lib.rs" default = [] private = [] enterprise = ["windmill-common/enterprise"] -cloud = [] +cloud = ["windmill-common/cloud"] benchmark = ["windmill-common/benchmark"] failpoints = [] prometheus = ["dep:prometheus"] diff --git a/backend/windmill-queue/src/jobs.rs b/backend/windmill-queue/src/jobs.rs index 8ac4fd390a..d7f847a73d 100644 --- a/backend/windmill-queue/src/jobs.rs +++ b/backend/windmill-queue/src/jobs.rs @@ -2065,10 +2065,35 @@ fn apply_completed_job_cloud_usage( queued_job: &MiniCompletedJob, _duration: i64, ) { - if *CLOUD_HOSTED && !queued_job.is_flow() && _duration > 1000 { + if !queued_job.is_flow() { + meter_execution_seconds( + db, + &queued_job.workspace_id, + &queued_job.permissioned_as_email, + _duration, + ); + } +} + +/// Charge `_duration` of execution time to the cloud usage meters: the workspace's +/// monthly row, plus the per-user row on non-premium plans. +/// +/// The unit is one finished **segment**, not one job. A Workflow-as-Code parent parks on +/// a sleep, an approval or its children and resumes with a fresh timer, so its compute +/// arrives here as several calls; metering only the one at completion would drop +/// everything it ran before its first park. +/// +/// Fire-and-forget, like every other write to `usage`: billing must never hold up the +/// job that produced it. +/// +/// `w_id` and `email` are billed as given and authorize nothing on their own — take them +/// from a job the caller already holds, never from request input. +#[cfg(feature = "cloud")] +pub fn meter_execution_seconds(db: &Pool, w_id: &str, email: &str, _duration: i64) { + if *CLOUD_HOSTED && _duration > 1000 { let db = db.clone(); - let w_id = queued_job.workspace_id.clone(); - let email = queued_job.permissioned_as_email.clone(); + let w_id = w_id.to_string(); + let email = email.to_string(); let w_id2 = w_id.clone(); let email2 = email.clone(); tokio::task::spawn(async move { diff --git a/backend/windmill-worker/Cargo.toml b/backend/windmill-worker/Cargo.toml index 0390e1c366..e75afc053e 100644 --- a/backend/windmill-worker/Cargo.toml +++ b/backend/windmill-worker/Cargo.toml @@ -23,7 +23,7 @@ benchmark = ["windmill-queue/benchmark", "windmill-common/benchmark"] parquet = ["windmill-common/parquet", "windmill-object-store/parquet"] flow_testing = [] failpoints = [] -cloud = [] +cloud = ["windmill-queue/cloud", "windmill-common/cloud"] sqlx = [] deno_core = ["dep:windmill-runtime-nativets"] libffi_mac = ["dep:libffi-sys"] diff --git a/backend/windmill-worker/src/bun_executor.rs b/backend/windmill-worker/src/bun_executor.rs index 77221edf22..ac1eebe723 100644 --- a/backend/windmill-worker/src/bun_executor.rs +++ b/backend/windmill-worker/src/bun_executor.rs @@ -2819,6 +2819,7 @@ pub async fn handle_wac_v2_output( // Step 1: Save checkpoint, suspend parent, and seed child checkpoints // in a single transaction — all BEFORE children become visible. + let segment_ms; { let mut tx = db.begin().await?; @@ -2871,24 +2872,16 @@ pub async fn handle_wac_v2_output( })?; } - // Suspend parent before children become visible. - // Keep running = true so the normal pull query ignores it. - // The suspended pull query picks it up when suspend reaches 0 - // (it checks: suspend_until IS NOT NULL AND suspend <= 0). - let suspend_count = num_steps as i32; - sqlx::query!( - "UPDATE v2_job_queue SET suspend = $2, suspend_until = now() + interval '14 day' WHERE id = $1", - job.id, - suspend_count, + // Suspend parent before children become visible, so a child that + // completes immediately finds a parked parent to decrement. + segment_ms = crate::wac_executor::suspend_wac_parent( + &mut tx, + &job.id, + &job.workspace_id, + num_steps as i32, + 14.0 * 24.0 * 3600.0, ) - .execute(&mut *tx) - .await - .map_err(|e| { - error::Error::internal_err(format!( - "Failed to suspend WAC parent job {}: {e}", - job.id - )) - })?; + .await?; tx.commit().await?; } @@ -3167,10 +3160,17 @@ pub async fn handle_wac_v2_output( .execute(db) .await; - // Unsuspend parent so the error propagates instead of a 14-day hang + // Unsuspend parent so the error propagates instead of a 14-day hang. + // Unlike the other suspend exits this one completes the job for real, so + // it needs its segment start back — the in-memory copy is what the pull + // stamped, before the suspend cleared the column. let _ = sqlx::query!( - "UPDATE v2_job_queue SET suspend = 0, suspend_until = NULL WHERE id = $1", + "UPDATE v2_job_queue + SET suspend = 0, suspend_until = NULL, + started_at = coalesce(started_at, $2, now()) + WHERE id = $1", job.id, + job.started_at, ) .execute(db) .await; @@ -3183,6 +3183,7 @@ pub async fn handle_wac_v2_output( "WAC v2 parent job suspended" ); + crate::wac_executor::end_wac_segment(conn, job, segment_ms); Err(error::Error::WacSuspended(format!( "WAC v2 job {} suspended waiting for {} child job(s)", job.id, num_steps @@ -3361,15 +3362,17 @@ pub async fn handle_wac_v2_output( } // Suspend parent with suspend=1 (waiting for 1 approval event) - sqlx::query!( - "UPDATE v2_job_queue SET suspend = 1, suspend_until = now() + make_interval(secs => $2) WHERE id = $1", - job.id, + let segment_ms = crate::wac_executor::suspend_wac_parent( + &mut tx, + &job.id, + &job.workspace_id, + 1, timeout_secs, ) - .execute(&mut *tx) .await?; tx.commit().await?; + crate::wac_executor::end_wac_segment(conn, job, segment_ms); tracing::info!( job_id = %job.id, @@ -3453,18 +3456,19 @@ pub async fn handle_wac_v2_output( })?; } - // Suspend parent — it will auto-resume when suspend_until passes. // Use suspend=1 (not 0) so the suspended pull query only picks it up // when `suspend_until <= now()`, not via `suspend <= 0`. - sqlx::query!( - "UPDATE v2_job_queue SET suspend = 1, suspend_until = now() + make_interval(secs => $2) WHERE id = $1", - job.id, + let segment_ms = crate::wac_executor::suspend_wac_parent( + &mut tx, + &job.id, + &job.workspace_id, + 1, sleep_secs, ) - .execute(&mut *tx) .await?; tx.commit().await?; + crate::wac_executor::end_wac_segment(conn, job, segment_ms); tracing::info!( job_id = %job.id, @@ -3514,19 +3518,25 @@ pub async fn handle_wac_v2_output( // Reset running=false so the job is immediately eligible for pickup. // Unlike dispatch (which sets suspend>0), inline checkpoints don't suspend — // the job should be re-run right away to continue past the cached step. - sqlx::query!( - "UPDATE v2_job_queue SET running = false, started_at = null WHERE id = $1", + // `prev` holds the pre-update row: RETURNING would see the cleared column. + let segment_ms = sqlx::query_scalar!( + "WITH prev AS (SELECT started_at FROM v2_job_queue WHERE id = $1) + UPDATE v2_job_queue q SET running = false, started_at = null + FROM prev WHERE q.id = $1 + RETURNING (extract(epoch FROM now() - prev.started_at) * 1000)::bigint", job.id, ) - .execute(&mut *tx) + .fetch_optional(&mut *tx) .await .map_err(|e| { error::Error::internal_err(format!( "Failed to reset running state for inline checkpoint: {e}" )) - })?; + })? + .flatten(); tx.commit().await?; + crate::wac_executor::end_wac_segment(conn, job, segment_ms); Err(error::Error::WacSuspended(format!( "WAC v2 job {} inline checkpoint for step {}", diff --git a/backend/windmill-worker/src/wac_executor.rs b/backend/windmill-worker/src/wac_executor.rs index 3e9b7bbc9b..1402c7aed9 100644 --- a/backend/windmill-worker/src/wac_executor.rs +++ b/backend/windmill-worker/src/wac_executor.rs @@ -1,6 +1,7 @@ use serde::Deserialize; use serde_json::value::RawValue; use serde_json::Value; +use sqlx::{Postgres, Transaction}; use uuid::Uuid; use windmill_common::error::{self, Error}; @@ -85,6 +86,81 @@ fn default_dispatch_type() -> String { "inline".to_string() } +/// Park a WAC v2 parent in the queue until `suspend` reaches 0 or `suspend_secs` +/// elapses, whichever comes first. `running` stays true so the normal pull query +/// skips the row; only the suspended pull query takes it back. The `id`/`workspace_id` +/// pair is a consistency check, not an authorization one — callers must already hold +/// the job (every one of them passes a job its own worker pulled). +/// +/// `started_at` is cleared because the parent holds no worker while parked. The pull +/// re-stamps it (`started_at = coalesce(started_at, now())`), and every path that +/// completes a job without a worker-measured duration — a cancel, the child-failure +/// handler — falls back to `now() - started_at`. Left pointing at the first segment, +/// that fallback reports the whole sleep or approval wait as execution time. +/// +/// Returns the segment that just ended, in milliseconds, for the caller to hand to +/// `end_wac_segment`. +pub async fn suspend_wac_parent( + tx: &mut Transaction<'_, Postgres>, + job_id: &Uuid, + w_id: &str, + suspend: i32, + suspend_secs: f64, +) -> error::Result> { + // `prev` holds the pre-update row: RETURNING sees the new one, where `started_at` + // has already been cleared. + let parked = sqlx::query_scalar!( + "WITH prev AS ( + SELECT started_at FROM v2_job_queue WHERE id = $1 AND workspace_id = $2 + ) + UPDATE v2_job_queue q + SET suspend = $3, suspend_until = now() + make_interval(secs => $4), started_at = null + FROM prev + WHERE q.id = $1 AND q.workspace_id = $2 + RETURNING (extract(epoch FROM now() - prev.started_at) * 1000)::bigint", + job_id, + w_id, + suspend, + suspend_secs, + ) + .fetch_optional(&mut **tx) + .await + .map_err(|e| Error::internal_err(format!("Failed to suspend WAC parent job {job_id}: {e}")))?; + + // Silently parking nothing is unrecoverable on the dispatch arm: the children are + // pushed right after and decrement a `suspend` that was never set, so the parent + // sits out its whole suspend window instead of resuming. + match parked { + Some(segment_ms) => Ok(segment_ms), + None => Err(Error::internal_err(format!( + "WAC parent job {job_id} not in the queue of workspace {w_id} to suspend" + ))), + } +} + +/// Charge the execution segment a WAC parent just finished. Segments are metered as they +/// end rather than summed at completion, so a workflow that sleeps for days is billed for +/// the compute it used, when it used it — and the final segment is charged by the ordinary +/// completion path. +/// +/// Call this only where the parent really parks. On a rollback that goes on to complete +/// the job, the completion charges the same segment and it would be billed twice. +pub(crate) fn end_wac_segment( + _conn: &windmill_common::worker::Connection, + _job: &windmill_queue::MiniPulledJob, + _segment_ms: Option, +) { + #[cfg(feature = "cloud")] + if let (windmill_common::worker::Connection::Sql(db), Some(segment_ms)) = (_conn, _segment_ms) { + windmill_queue::meter_execution_seconds( + db, + &_job.workspace_id, + &_job.permissioned_as_email, + segment_ms, + ); + } +} + /// Parse the WAC result from result.json content. pub fn parse_wac_output(result: &RawValue) -> error::Result { serde_json::from_str(result.get()) diff --git a/frontend/src/lib/components/runs/RunRow.svelte b/frontend/src/lib/components/runs/RunRow.svelte index 27341d7743..959ee2fd06 100644 --- a/frontend/src/lib/components/runs/RunRow.svelte +++ b/frontend/src/lib/components/runs/RunRow.svelte @@ -127,6 +127,8 @@ {/if} {:else if `scheduled_for` in job && job.scheduled_for && forLater(job.scheduled_for)} Waiting executor () + {:else if 'running' in job && job.running && job.suspend} + Suspended (created ) {:else} Waiting executor () {/if} From f977f5bf8b1ac70d3afbdc8ad6fcbe072cc51ebc Mon Sep 17 00:00:00 2001 From: Ruben Fiszel Date: Sat, 5 Sep 2026 09:41:15 +0000 Subject: [PATCH 04/13] fix: stand the WAC park down for a cancel that beat it to the row (#10990) * fix: stand the WAC park down for a cancel that beat it to the row Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01GHfNFFJh3ozZYgyyoaEepu * refactor: share the cancel result payload with canceled_job_to_result Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01GHfNFFJh3ozZYgyyoaEepu --------- Co-authored-by: Claude Opus 5 (1M context) --- ...7cfd7ac33ac2616d9f68ab4cb62da105cae57.json | 25 ------ ...d3413ad3c0fbe8f3dca9aea11cb49fd4a28ef.json | 35 ++++++++ ...49bf8b3523d8dde1ef6b8f1950423830cc6ed.json | 17 ++++ backend/tests/wac_suspend_started_at.rs | 64 ++++++++++++- backend/windmill-queue/src/jobs.rs | 16 ++-- backend/windmill-worker/src/bun_executor.rs | 39 ++++++-- .../windmill-worker/src/python_executor.rs | 1 + backend/windmill-worker/src/wac_executor.rs | 90 ++++++++++++++----- 8 files changed, 220 insertions(+), 67 deletions(-) delete mode 100644 backend/.sqlx/query-5dc28b4609bff15ca4895e726c27cfd7ac33ac2616d9f68ab4cb62da105cae57.json create mode 100644 backend/.sqlx/query-e749663a4b9248a9d120f77e86fd3413ad3c0fbe8f3dca9aea11cb49fd4a28ef.json create mode 100644 backend/.sqlx/query-f0498c9bddc0d4d8948167f3ed849bf8b3523d8dde1ef6b8f1950423830cc6ed.json diff --git a/backend/.sqlx/query-5dc28b4609bff15ca4895e726c27cfd7ac33ac2616d9f68ab4cb62da105cae57.json b/backend/.sqlx/query-5dc28b4609bff15ca4895e726c27cfd7ac33ac2616d9f68ab4cb62da105cae57.json deleted file mode 100644 index a93f92c4e2..0000000000 --- a/backend/.sqlx/query-5dc28b4609bff15ca4895e726c27cfd7ac33ac2616d9f68ab4cb62da105cae57.json +++ /dev/null @@ -1,25 +0,0 @@ -{ - "db_name": "PostgreSQL", - "query": "WITH prev AS (\n SELECT started_at FROM v2_job_queue WHERE id = $1 AND workspace_id = $2\n )\n UPDATE v2_job_queue q\n SET suspend = $3, suspend_until = now() + make_interval(secs => $4), started_at = null\n FROM prev\n WHERE q.id = $1 AND q.workspace_id = $2\n RETURNING (extract(epoch FROM now() - prev.started_at) * 1000)::bigint", - "describe": { - "columns": [ - { - "ordinal": 0, - "name": "int8", - "type_info": "Int8" - } - ], - "parameters": { - "Left": [ - "Uuid", - "Text", - "Int4", - "Float8" - ] - }, - "nullable": [ - null - ] - }, - "hash": "5dc28b4609bff15ca4895e726c27cfd7ac33ac2616d9f68ab4cb62da105cae57" -} diff --git a/backend/.sqlx/query-e749663a4b9248a9d120f77e86fd3413ad3c0fbe8f3dca9aea11cb49fd4a28ef.json b/backend/.sqlx/query-e749663a4b9248a9d120f77e86fd3413ad3c0fbe8f3dca9aea11cb49fd4a28ef.json new file mode 100644 index 0000000000..328465fdf1 --- /dev/null +++ b/backend/.sqlx/query-e749663a4b9248a9d120f77e86fd3413ad3c0fbe8f3dca9aea11cb49fd4a28ef.json @@ -0,0 +1,35 @@ +{ + "db_name": "PostgreSQL", + "query": "SELECT canceled_by, canceled_reason,\n (extract(epoch FROM now() - started_at) * 1000)::bigint AS segment_ms\n FROM v2_job_queue WHERE id = $1 AND workspace_id = $2 FOR UPDATE", + "describe": { + "columns": [ + { + "ordinal": 0, + "name": "canceled_by", + "type_info": "Varchar" + }, + { + "ordinal": 1, + "name": "canceled_reason", + "type_info": "Text" + }, + { + "ordinal": 2, + "name": "segment_ms", + "type_info": "Int8" + } + ], + "parameters": { + "Left": [ + "Uuid", + "Text" + ] + }, + "nullable": [ + true, + true, + null + ] + }, + "hash": "e749663a4b9248a9d120f77e86fd3413ad3c0fbe8f3dca9aea11cb49fd4a28ef" +} diff --git a/backend/.sqlx/query-f0498c9bddc0d4d8948167f3ed849bf8b3523d8dde1ef6b8f1950423830cc6ed.json b/backend/.sqlx/query-f0498c9bddc0d4d8948167f3ed849bf8b3523d8dde1ef6b8f1950423830cc6ed.json new file mode 100644 index 0000000000..5f86fb97d8 --- /dev/null +++ b/backend/.sqlx/query-f0498c9bddc0d4d8948167f3ed849bf8b3523d8dde1ef6b8f1950423830cc6ed.json @@ -0,0 +1,17 @@ +{ + "db_name": "PostgreSQL", + "query": "UPDATE v2_job_queue\n SET suspend = $3, suspend_until = now() + make_interval(secs => $4), started_at = null\n WHERE id = $1 AND workspace_id = $2", + "describe": { + "columns": [], + "parameters": { + "Left": [ + "Uuid", + "Text", + "Int4", + "Float8" + ] + }, + "nullable": [] + }, + "hash": "f0498c9bddc0d4d8948167f3ed849bf8b3523d8dde1ef6b8f1950423830cc6ed" +} diff --git a/backend/tests/wac_suspend_started_at.rs b/backend/tests/wac_suspend_started_at.rs index f374a25220..5a83730d24 100644 --- a/backend/tests/wac_suspend_started_at.rs +++ b/backend/tests/wac_suspend_started_at.rs @@ -1,9 +1,10 @@ -//! Guards the two things `suspend_wac_parent` promises: the `started_at` invariant -//! documented on it, and the segment length it hands back for metering. +//! Guards what `suspend_wac_parent` promises: the `started_at` invariant documented on +//! it, the segment length it hands back for metering, and that it stands down for a +//! cancel already on the row. use sqlx::{Pool, Postgres}; use uuid::Uuid; -use windmill_worker::wac_executor::suspend_wac_parent; +use windmill_worker::wac_executor::{suspend_wac_parent, WacPark}; #[sqlx::test] async fn wac_suspend_clears_started_at(db: Pool) -> anyhow::Result<()> { @@ -17,7 +18,11 @@ async fn wac_suspend_clears_started_at(db: Pool) -> anyhow::Result<()> .await?; let mut tx = db.begin().await?; - let segment_ms = suspend_wac_parent(&mut tx, &job_id, "test-workspace", 1, 3600.0).await?; + let WacPark::Parked(segment_ms) = + suspend_wac_parent(&mut tx, &job_id, "test-workspace", 1, 3600.0).await? + else { + panic!("an uncancelled parent must park"); + }; tx.commit().await?; // The segment is what gets billed, so it must be the run that just ended, measured @@ -53,3 +58,54 @@ async fn wac_suspend_clears_started_at(db: Pool) -> anyhow::Result<()> Ok(()) } + +/// A soft cancel sets `canceled_by` and `suspend = 0` and leaves acting on it to the next +/// pull. Parking over that holds the row until `suspend_until` — a whole day on a +/// `sleep(86400)` — so the park has to stand down and let the job complete instead. +#[sqlx::test] +async fn wac_suspend_stands_down_for_a_cancel(db: Pool) -> anyhow::Result<()> { + let job_id = Uuid::new_v4(); + sqlx::query( + "INSERT INTO v2_job_queue \ + (id, workspace_id, scheduled_for, running, started_at, suspend, canceled_by, canceled_reason) \ + VALUES ($1, 'test-workspace', now(), true, now() - interval '30 seconds', 0, 'alice', 'no longer needed')", + ) + .bind(job_id) + .execute(&db) + .await?; + + let mut tx = db.begin().await?; + let parked = suspend_wac_parent(&mut tx, &job_id, "test-workspace", 1, 86400.0).await?; + tx.commit().await?; + + match &parked { + WacPark::Cancelled(cancel) => { + assert_eq!(cancel.username.as_deref(), Some("alice")); + assert_eq!(cancel.reason.as_deref(), Some("no longer needed")); + } + other => panic!("a cancelled parent must not park, got {other:?}"), + } + + let (suspend, suspend_until, started_at): ( + i32, + Option>, + Option>, + ) = sqlx::query_as( + "SELECT suspend, suspend_until, started_at FROM v2_job_queue WHERE id = $1", + ) + .bind(job_id) + .fetch_one(&db) + .await?; + + assert_eq!(suspend, 0, "the cancel's suspend = 0 must survive"); + assert_eq!( + suspend_until, None, + "a suspend_until would hold the row back for the whole park window" + ); + assert!( + started_at.is_some(), + "the segment ran, so its start must stay for the completion's duration" + ); + + Ok(()) +} diff --git a/backend/windmill-queue/src/jobs.rs b/backend/windmill-queue/src/jobs.rs index d7f847a73d..5bb9a1120f 100644 --- a/backend/windmill-queue/src/jobs.rs +++ b/backend/windmill-queue/src/jobs.rs @@ -7308,15 +7308,19 @@ async fn check_workspace_queue_cap<'c>( // Ok(()) // } -pub fn canceled_job_to_result(job: &MiniPulledJob) -> serde_json::Value { - let reason = job - .canceled_reason - .as_deref() - .unwrap_or_else(|| "no reason given"); - let canceler = job.canceled_by.as_deref().unwrap_or_else(|| "unknown"); +/// The result payload a job cancelled anywhere carries. Callers that hold the cancel +/// outside a `MiniPulledJob` — a row read after the pull, say — go through this rather +/// than rebuilding the shape. +pub fn canceled_result(reason: Option<&str>, canceler: Option<&str>) -> serde_json::Value { + let reason = reason.unwrap_or("no reason given"); + let canceler = canceler.unwrap_or("unknown"); serde_json::json!({"message": format!("Job canceled: {reason} by {canceler}"), "name": "Canceled", "reason": reason, "canceler": canceler}) } +pub fn canceled_job_to_result(job: &MiniPulledJob) -> serde_json::Value { + canceled_result(job.canceled_reason.as_deref(), job.canceled_by.as_deref()) +} + /// Helper function to create a restarted module for branch/iteration restart fn create_restarted_module( module: &FlowStatusModule, diff --git a/backend/windmill-worker/src/bun_executor.rs b/backend/windmill-worker/src/bun_executor.rs index ac1eebe723..7c112f5985 100644 --- a/backend/windmill-worker/src/bun_executor.rs +++ b/backend/windmill-worker/src/bun_executor.rs @@ -2572,7 +2572,8 @@ try {{ // WAC v2 post-execution: parse output and handle dispatch/suspend if is_wac_v2 { - return handle_wac_v2_output(result, job, conn, modules, new_args.as_ref()).await; + return handle_wac_v2_output(result, job, conn, canceled_by, modules, new_args.as_ref()) + .await; } Ok(result) @@ -2602,11 +2603,13 @@ pub async fn handle_wac_v2_output( result: Box, job: &MiniPulledJob, conn: &Connection, + canceled_by: &mut Option, modules: &Option>, preprocessed_args: Option<&HashMap>>, ) -> error::Result> { use crate::wac_executor::{ - load_checkpoint, parse_wac_output, update_checkpoint_for_dispatch, WacOutput, + load_checkpoint, parse_wac_output, update_checkpoint_for_dispatch, + wac_cancelled_mid_segment, WacOutput, WacPark, }; use serde_json::Value; use windmill_common::get_latest_flow_version_info_for_path; @@ -2874,14 +2877,22 @@ pub async fn handle_wac_v2_output( // Suspend parent before children become visible, so a child that // completes immediately finds a parked parent to decrement. - segment_ms = crate::wac_executor::suspend_wac_parent( + match crate::wac_executor::suspend_wac_parent( &mut tx, &job.id, &job.workspace_id, num_steps as i32, 14.0 * 24.0 * 3600.0, ) - .await?; + .await? + { + WacPark::Parked(ms) => segment_ms = ms, + // Returning here drops `tx`, unwriting the checkpoint and the timeline + // entries, so no child is ever pushed against a parent that never parked. + WacPark::Cancelled(cancel) => { + return Err(wac_cancelled_mid_segment(cancel, canceled_by)) + } + } tx.commit().await?; } @@ -3362,14 +3373,20 @@ pub async fn handle_wac_v2_output( } // Suspend parent with suspend=1 (waiting for 1 approval event) - let segment_ms = crate::wac_executor::suspend_wac_parent( + let segment_ms = match crate::wac_executor::suspend_wac_parent( &mut tx, &job.id, &job.workspace_id, 1, timeout_secs, ) - .await?; + .await? + { + WacPark::Parked(ms) => ms, + WacPark::Cancelled(cancel) => { + return Err(wac_cancelled_mid_segment(cancel, canceled_by)) + } + }; tx.commit().await?; crate::wac_executor::end_wac_segment(conn, job, segment_ms); @@ -3458,14 +3475,20 @@ pub async fn handle_wac_v2_output( // Use suspend=1 (not 0) so the suspended pull query only picks it up // when `suspend_until <= now()`, not via `suspend <= 0`. - let segment_ms = crate::wac_executor::suspend_wac_parent( + let segment_ms = match crate::wac_executor::suspend_wac_parent( &mut tx, &job.id, &job.workspace_id, 1, sleep_secs, ) - .await?; + .await? + { + WacPark::Parked(ms) => ms, + WacPark::Cancelled(cancel) => { + return Err(wac_cancelled_mid_segment(cancel, canceled_by)) + } + }; tx.commit().await?; crate::wac_executor::end_wac_segment(conn, job, segment_ms); diff --git a/backend/windmill-worker/src/python_executor.rs b/backend/windmill-worker/src/python_executor.rs index 883b035a32..38fb4bca32 100644 --- a/backend/windmill-worker/src/python_executor.rs +++ b/backend/windmill-worker/src/python_executor.rs @@ -1223,6 +1223,7 @@ mount {{ result, job, conn, + canceled_by, modules, new_args.as_ref(), )) diff --git a/backend/windmill-worker/src/wac_executor.rs b/backend/windmill-worker/src/wac_executor.rs index 1402c7aed9..cecf0e316c 100644 --- a/backend/windmill-worker/src/wac_executor.rs +++ b/backend/windmill-worker/src/wac_executor.rs @@ -7,6 +7,7 @@ use uuid::Uuid; use windmill_common::error::{self, Error}; use windmill_common::scripts::ScriptLang; use windmill_common::DB; +use windmill_queue::CanceledBy; // Checkpoint model + persistence primitives live in windmill-common so the // API server can use them without pulling in the full worker crate. Re-export @@ -86,6 +87,16 @@ fn default_dispatch_type() -> String { "inline".to_string() } +/// What `suspend_wac_parent` did with the parent's queue row. +#[derive(Debug)] +pub enum WacPark { + /// Parked. Carries the segment that just ended, in milliseconds, for `end_wac_segment`. + Parked(Option), + /// A cancel reached the row while this segment was running, so the park was skipped. + /// Carries who cancelled, for the completion that must happen instead. + Cancelled(CanceledBy), +} + /// Park a WAC v2 parent in the queue until `suspend` reaches 0 or `suspend_secs` /// elapses, whichever comes first. `running` stays true so the normal pull query /// skips the row; only the suspended pull query takes it back. The `id`/`workspace_id` @@ -97,45 +108,76 @@ fn default_dispatch_type() -> String { /// completes a job without a worker-measured duration — a cancel, the child-failure /// handler — falls back to `now() - started_at`. Left pointing at the first segment, /// that fallback reports the whole sleep or approval wait as execution time. -/// -/// Returns the segment that just ended, in milliseconds, for the caller to hand to -/// `end_wac_segment`. pub async fn suspend_wac_parent( tx: &mut Transaction<'_, Postgres>, job_id: &Uuid, w_id: &str, suspend: i32, suspend_secs: f64, -) -> error::Result> { - // `prev` holds the pre-update row: RETURNING sees the new one, where `started_at` - // has already been cleared. - let parked = sqlx::query_scalar!( - "WITH prev AS ( - SELECT started_at FROM v2_job_queue WHERE id = $1 AND workspace_id = $2 - ) - UPDATE v2_job_queue q +) -> error::Result { + // `FOR UPDATE` orders this against a concurrent soft cancel, which writes `suspend = 0` + // and leaves acting on `canceled_by` to the next pull. Parking on top of that keeps the + // row unpullable until `suspend_until` — up to the full `sleep()` — so a cancel already + // on the row has to stand the park down rather than be overwritten by it. + let prev = sqlx::query!( + "SELECT canceled_by, canceled_reason, + (extract(epoch FROM now() - started_at) * 1000)::bigint AS segment_ms + FROM v2_job_queue WHERE id = $1 AND workspace_id = $2 FOR UPDATE", + job_id, + w_id, + ) + .fetch_optional(&mut **tx) + .await + .map_err(|e| Error::internal_err(format!("Failed to read WAC parent job {job_id}: {e}")))? + // Silently parking nothing is unrecoverable on the dispatch arm: the children are + // pushed right after and decrement a `suspend` that was never set, so the parent + // sits out its whole suspend window instead of resuming. + .ok_or_else(|| { + Error::internal_err(format!( + "WAC parent job {job_id} not in the queue of workspace {w_id} to suspend" + )) + })?; + + if let Some(username) = prev.canceled_by { + return Ok(WacPark::Cancelled(CanceledBy { + username: Some(username), + reason: prev.canceled_reason, + })); + } + + sqlx::query!( + "UPDATE v2_job_queue SET suspend = $3, suspend_until = now() + make_interval(secs => $4), started_at = null - FROM prev - WHERE q.id = $1 AND q.workspace_id = $2 - RETURNING (extract(epoch FROM now() - prev.started_at) * 1000)::bigint", + WHERE id = $1 AND workspace_id = $2", job_id, w_id, suspend, suspend_secs, ) - .fetch_optional(&mut **tx) + .execute(&mut **tx) .await .map_err(|e| Error::internal_err(format!("Failed to suspend WAC parent job {job_id}: {e}")))?; - // Silently parking nothing is unrecoverable on the dispatch arm: the children are - // pushed right after and decrement a `suspend` that was never set, so the parent - // sits out its whole suspend window instead of resuming. - match parked { - Some(segment_ms) => Ok(segment_ms), - None => Err(Error::internal_err(format!( - "WAC parent job {job_id} not in the queue of workspace {w_id} to suspend" - ))), - } + Ok(WacPark::Parked(prev.segment_ms)) +} + +/// Turn a cancel that landed mid-segment into the error the executor returns, so the job +/// completes on this pass instead of parking. Setting the worker's `canceled_by` is what +/// makes it land as `canceled` rather than `failure`: the row was cancelled after this +/// worker pulled the job, so the in-memory copy still reads as uncancelled. +/// +/// The completion charges the segment that just ended, so callers must not also hand it to +/// `end_wac_segment`. +pub(crate) fn wac_cancelled_mid_segment( + cancel: CanceledBy, + canceled_by: &mut Option, +) -> Error { + let payload = windmill_common::worker::to_raw_value(&windmill_queue::canceled_result( + cancel.reason.as_deref(), + cancel.username.as_deref(), + )); + *canceled_by = Some(cancel); + Error::ExecutionRawError(payload) } /// Charge the execution segment a WAC parent just finished. Segments are metered as they From 8aab5034a68a4aafb264b0e86d000ef58f4a8511 Mon Sep 17 00:00:00 2001 From: Ruben Fiszel Date: Sat, 5 Sep 2026 10:23:37 +0000 Subject: [PATCH 05/13] feat: guest JWT entry for embedded apps (#10954) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * feat: guest JWT entry for embedded apps (jwt_guest_) A second way in for a guest, alongside the signed-in guest session: a JWT the embedding customer's backend mints and signs, verified per request against a per-workspace key (a PEM public key or a JWKS URL), resolving to the same seatless guest identity confined to the one app its app_path claim names. Bearer prefix jwt_guest_, stateless (no token row). See PR #10954. Co-Authored-By: Claude Opus 4.8 * feat: surface guest JWT as the embed method in the app deploy drawer The deploy drawer explained the secret-URL embed but not the guest JWT path, so the primary way to embed an app for a customer's own authenticated users was undiscoverable. For a guest-mode app with guests enabled, show how to mint a `jwt_guest_` token and append `guest.` to the app URL, with a copyable iframe template pre-filled with this app's workspace_id and app_path, and a note that new guest emails are refused past the instance's free allowance (the live count is shown just above). Also log a guest JWT allowance refusal at warn, not info: the caller gets a bare 401 (the reason must not leak to an unauthenticated caller), so the log is the admin's signal that the instance hit its guest cap. Co-Authored-By: Claude Opus 4.8 * fix: correct the guest JWT minting instructions in the embed block The block said "sign it with the workspace's guest JWT key", but that setting holds the public verification key. Clarify the keypair relationship (configure the public key or a JWKS URL in the workspace; sign with the matching private key), name the accepted algorithms (RS/PS/ES; HS* refused), and keep the required claims, so an embedder knows how to actually mint the token. Co-Authored-By: Claude Opus 4.8 * feat: fall back to the instance JWT issuer for guest verification (off on cloud) A workspace with no guest key of its own now verifies guest JWTs against the instance issuer (JWT_EXT_JWKS_URL, already used by jwt_ext_), so an operator running one issuer configures it once. Verification and the guest grant are CE; granting a full login from that issuer stays EE (jwt_ext_, unchanged). Disabled under CLOUD_HOSTED, where one instance issuer must not be trusted to mint guests in every tenant's workspace — there the per-workspace key is the only source, which also stays the override everywhere. The workspace settings note (hidden on cloud) explains the fallback. Co-Authored-By: Claude Opus 4.8 * fix: embed instructions cover both the workspace key and instance issuer The embed block said to set the workspace's guest JWT key; now it says Windmill verifies against the workspace key or, off cloud, the instance issuer (JWT_EXT_JWKS_URL) when no workspace key is set. The instance clause is hidden under isCloudHosted(). Co-Authored-By: Claude Opus 4.8 * fix: show the guest JWT embed block only when Embed is toggled It belongs with the iframe snippet, not the plain-URL view, so gate it on embedMode alongside the guest-mode / guests-enabled checks. Co-Authored-By: Claude Opus 4.8 * fix: trust the instance issuer in the guest fallback; refresh stale docs P1 (CI review): the fallback wrapped JWT_EXT_JWKS_URL as a workspace JwksUrl, so it hit validate_guest_jwks_url and was refused for http/private issuers unless ALLOW_PRIVATE_GUEST_JWKS_URLS was also set — a self-hosted internal issuer that works for jwt_ext_ failed for guests, though the UI says setting the env var is enough. fetch_jwks now fetches the instance issuer without the https/private restriction (matching the jwt_ext_ loader; it stays operator-trusted), while a workspace-admin URL is validated and pinned as before. All the size/key/URL bounds still apply to both. P2 (CI review): refresh the stale docs that said a missing workspace key always refuses a guest JWT — the module, bearer, key-source, and EditGuestJwtKey field docs now describe the workspace key with the off-cloud instance-issuer fallback. Co-Authored-By: Claude Opus 4.8 * fix: fetch the trusted instance issuer like the jwt_ext_ loader P1 (CI review): the instance-issuer fetch skipped SSRF validation but still disabled redirects and default cert validation, so an instance issuer that works for jwt_ext_ through a redirect or an operator-approved self-signed cert failed the guest fallback. Fetch it with HTTP_CLIENT_PERMISSIVE (follows redirects, honors ACCEPT_INVALID_CERTS) — the same behavior jwt_ext_ has — while a workspace-admin URL stays validated, DNS-pinned and redirect-free. The body size cap still bounds both. P2 (CI review): the WorkspaceSettings field doc still said None/None means no JWT guests; it now names the off-cloud instance-issuer fallback. Co-Authored-By: Claude Opus 4.8 * docs: schema summary + OpenAPI cover the guest JWT columns and fallback P2 (CI review): summarized_schema.txt was missing guest_activity.jwt_entry and the two workspace_settings guest-JWT key columns (required by docs/validation.md after a schema change). The edit_guest_jwt_key OpenAPI description now notes that clearing the workspace key falls back to the instance issuer (JWT_EXT_JWKS_URL) off cloud rather than necessarily stopping guest JWTs. Co-Authored-By: Claude Opus 4.8 * fix: keep JWKS single-flight locks in a self-cleaning map, not a bounded cache P1 (CI review): JWKS_FETCH_LOCKS was a 200-entry quick_cache. Past 200 cold URLs it can evict a lock whose fetch is still in flight; the next request for that URL then mints a fresh lock and starts a second fetch, so cycling configured workspaces defeats single-flight and can storm the issuers. Replace it with a plain map guarded by a JwksFetchLock RAII handle that removes each entry once its last holder drops, so the map only ever holds the fetches in flight and never evicts an in-flight lock. Add a unit test pinning the shared-lock and self-cleaning invariants. Co-Authored-By: Claude Opus 4.8 * chore: update ee-repo-ref to c2270eb5fe2d9f0968253e6b460c33186363f4e7 This commit updates the EE repository reference after PR #773 was merged in windmill-ee-private. Previous ee-repo-ref: 5a1d9dee34159512c0823fddcd3d096490edbcce New ee-repo-ref: c2270eb5fe2d9f0968253e6b460c33186363f4e7 Automated by sync-ee-ref workflow. --------- Co-authored-by: Claude Opus 4.8 Co-authored-by: windmill-internal-app[bot] --- ...e0e9ac63fc8756aeafac8d279841db117eac.json} | 14 +- ...938a298bfe29afe1fd7111340252877f8b86c.json | 23 + ...27c7ae2ef5f27d665ad9018f5cdea0f6f2cb1.json | 15 + ...450390addbfae85fecc61c991d94167e6e99.json} | 18 +- ...3e3620111119abf8dda4bde5b835ff934e4f1.json | 15 - ...59da15843f466c72159bf76712b124d7554b6.json | 28 + ...f420b03fd23304f78c382d6de14cb31bcd6b1.json | 16 + backend/Cargo.lock | 3 + backend/Cargo.toml | 3 + backend/ee-repo-ref.txt | 2 +- .../20260903071242_guest_jwt_entry.down.sql | 5 + .../20260903071242_guest_jwt_entry.up.sql | 15 + backend/summarized_schema.txt | 4 +- backend/tests/app_guest_execution_mode.rs | 19 +- backend/tests/app_guest_jwt_allowance.rs | 128 +++ backend/tests/app_guest_jwt_entry.rs | 494 ++++++++ backend/tests/postgres_trigger_scope.rs | 1 + backend/tests/trigger_listener_queries.rs | 1 + backend/tests/wm_token_confinement.rs | 1 + .../tests/dbt_pinned_graph.rs | 1 + backend/windmill-api-auth/src/auth.rs | 247 ++++ backend/windmill-api-auth/src/lib.rs | 7 + backend/windmill-api-auth/src/scopes.rs | 27 + .../tests/native_triggers.rs | 1 + backend/windmill-api-users/src/users.rs | 44 +- .../windmill-api-workspaces/src/workspaces.rs | 77 +- .../src/workspaces_extra.rs | 2 +- backend/windmill-api/openapi.yaml | 43 + backend/windmill-api/src/apps.rs | 32 +- backend/windmill-api/src/jobs.rs | 1 + backend/windmill-api/src/lib.rs | 1 + backend/windmill-api/src/mcp/utils.rs | 1 + backend/windmill-common/Cargo.toml | 2 + backend/windmill-common/src/guest_jwt.rs | 1008 +++++++++++++++++ backend/windmill-common/src/lib.rs | 1 + backend/windmill-common/src/ssrf.rs | 38 + backend/windmill-common/src/users.rs | 23 + .../apps/editor/AppEditorHeaderDeploy.svelte | 40 +- .../(logged)/workspace_settings/+page.svelte | 111 +- frontend/src/routes/a/[...path]/+page.svelte | 42 +- .../[workspace]/[...secret]/+page.svelte | 25 +- 41 files changed, 2469 insertions(+), 110 deletions(-) rename backend/.sqlx/{query-8b28332dd5b3932dfdaa9fcb2e3eb6b9c48ec164b05b149b3477351df7a1bd60.json => query-06af616fe4fc61a3b0dcd996a2a1e0e9ac63fc8756aeafac8d279841db117eac.json} (55%) create mode 100644 backend/.sqlx/query-0fc900f73ef119e4c89186cf120938a298bfe29afe1fd7111340252877f8b86c.json create mode 100644 backend/.sqlx/query-77d599e4f7c574dffac4824f37127c7ae2ef5f27d665ad9018f5cdea0f6f2cb1.json rename backend/.sqlx/{query-00a61afc5faa3826c283660417ff1f8a93060fe062a0b727f164329ab56387a2.json => query-dc4a57df3becc610f631ef22c116450390addbfae85fecc61c991d94167e6e99.json} (89%) delete mode 100644 backend/.sqlx/query-e2eee8de61337b7d093f38e3e393e3620111119abf8dda4bde5b835ff934e4f1.json create mode 100644 backend/.sqlx/query-f6fe63ef3518d2d1f321c2941bc59da15843f466c72159bf76712b124d7554b6.json create mode 100644 backend/.sqlx/query-fbee9545c564f6fd611ca1a122cf420b03fd23304f78c382d6de14cb31bcd6b1.json create mode 100644 backend/migrations/20260903071242_guest_jwt_entry.down.sql create mode 100644 backend/migrations/20260903071242_guest_jwt_entry.up.sql create mode 100644 backend/tests/app_guest_jwt_allowance.rs create mode 100644 backend/tests/app_guest_jwt_entry.rs create mode 100644 backend/windmill-common/src/guest_jwt.rs diff --git a/backend/.sqlx/query-8b28332dd5b3932dfdaa9fcb2e3eb6b9c48ec164b05b149b3477351df7a1bd60.json b/backend/.sqlx/query-06af616fe4fc61a3b0dcd996a2a1e0e9ac63fc8756aeafac8d279841db117eac.json similarity index 55% rename from backend/.sqlx/query-8b28332dd5b3932dfdaa9fcb2e3eb6b9c48ec164b05b149b3477351df7a1bd60.json rename to backend/.sqlx/query-06af616fe4fc61a3b0dcd996a2a1e0e9ac63fc8756aeafac8d279841db117eac.json index 8533137a2c..843207dad7 100644 --- a/backend/.sqlx/query-8b28332dd5b3932dfdaa9fcb2e3eb6b9c48ec164b05b149b3477351df7a1bd60.json +++ b/backend/.sqlx/query-06af616fe4fc61a3b0dcd996a2a1e0e9ac63fc8756aeafac8d279841db117eac.json @@ -1,6 +1,6 @@ { "db_name": "PostgreSQL", - "query": "\n SELECT\n (SELECT MIN(day) FROM guest_activity) AS since,\n (SELECT COUNT(DISTINCT email) FROM guest_activity\n WHERE day > CURRENT_DATE - 30)::INT AS \"guest_count!\",\n (SELECT COUNT(DISTINCT workspace_id) FROM guest_activity\n WHERE day > CURRENT_DATE - 30)::INT AS \"guest_workspace_count!\",\n (SELECT COUNT(*) FROM workspace_settings ws JOIN workspace w ON w.id = ws.workspace_id\n WHERE ws.guest_access_enabled AND NOT w.deleted)::INT AS \"guest_enabled_workspace_count!\",\n (SELECT COUNT(*) FROM workspace WHERE NOT deleted)::INT AS \"workspace_count!\"\n ", + "query": "\n SELECT\n (SELECT MIN(day) FROM guest_activity) AS since,\n (SELECT COUNT(DISTINCT email) FROM guest_activity\n WHERE day > CURRENT_DATE - 30)::INT AS \"guest_count!\",\n (SELECT COUNT(DISTINCT email) FROM guest_activity\n WHERE jwt_entry AND day > CURRENT_DATE - 30)::INT AS \"guest_jwt_count!\",\n (SELECT COUNT(DISTINCT workspace_id) FROM guest_activity\n WHERE day > CURRENT_DATE - 30)::INT AS \"guest_workspace_count!\",\n (SELECT COUNT(*) FROM workspace_settings ws JOIN workspace w ON w.id = ws.workspace_id\n WHERE ws.guest_access_enabled AND NOT w.deleted)::INT AS \"guest_enabled_workspace_count!\",\n (SELECT COUNT(*) FROM workspace WHERE NOT deleted)::INT AS \"workspace_count!\"\n ", "describe": { "columns": [ { @@ -15,16 +15,21 @@ }, { "ordinal": 2, - "name": "guest_workspace_count!", + "name": "guest_jwt_count!", "type_info": "Int4" }, { "ordinal": 3, - "name": "guest_enabled_workspace_count!", + "name": "guest_workspace_count!", "type_info": "Int4" }, { "ordinal": 4, + "name": "guest_enabled_workspace_count!", + "type_info": "Int4" + }, + { + "ordinal": 5, "name": "workspace_count!", "type_info": "Int4" } @@ -37,8 +42,9 @@ null, null, null, + null, null ] }, - "hash": "8b28332dd5b3932dfdaa9fcb2e3eb6b9c48ec164b05b149b3477351df7a1bd60" + "hash": "06af616fe4fc61a3b0dcd996a2a1e0e9ac63fc8756aeafac8d279841db117eac" } diff --git a/backend/.sqlx/query-0fc900f73ef119e4c89186cf120938a298bfe29afe1fd7111340252877f8b86c.json b/backend/.sqlx/query-0fc900f73ef119e4c89186cf120938a298bfe29afe1fd7111340252877f8b86c.json new file mode 100644 index 0000000000..abdea8c942 --- /dev/null +++ b/backend/.sqlx/query-0fc900f73ef119e4c89186cf120938a298bfe29afe1fd7111340252877f8b86c.json @@ -0,0 +1,23 @@ +{ + "db_name": "PostgreSQL", + "query": "INSERT INTO guest_activity (email, workspace_id, day, jwt_entry)\n VALUES ($1, $2, CURRENT_DATE, true)\n ON CONFLICT (email, workspace_id, day)\n DO UPDATE SET jwt_entry = true, last_seen_at = now()\n WHERE NOT guest_activity.jwt_entry\n RETURNING 1 AS \"audited!\"", + "describe": { + "columns": [ + { + "ordinal": 0, + "name": "audited!", + "type_info": "Int4" + } + ], + "parameters": { + "Left": [ + "Varchar", + "Varchar" + ] + }, + "nullable": [ + null + ] + }, + "hash": "0fc900f73ef119e4c89186cf120938a298bfe29afe1fd7111340252877f8b86c" +} diff --git a/backend/.sqlx/query-77d599e4f7c574dffac4824f37127c7ae2ef5f27d665ad9018f5cdea0f6f2cb1.json b/backend/.sqlx/query-77d599e4f7c574dffac4824f37127c7ae2ef5f27d665ad9018f5cdea0f6f2cb1.json new file mode 100644 index 0000000000..f504d7a809 --- /dev/null +++ b/backend/.sqlx/query-77d599e4f7c574dffac4824f37127c7ae2ef5f27d665ad9018f5cdea0f6f2cb1.json @@ -0,0 +1,15 @@ +{ + "db_name": "PostgreSQL", + "query": "INSERT INTO workspace_settings (workspace_id, slack_team_id, slack_name, slack_command_script, slack_email, customer_id, plan, webhook, ai_config, large_file_storage, git_sync, default_app, default_scripts, deploy_ui, mute_critical_alerts, color, operator_settings, teams_command_script, teams_team_id, teams_team_name, git_app_installations, ducklake, dbt_warehouses, slack_oauth_client_id, slack_oauth_client_secret, datatable, teams_team_guid, auto_invite, error_handler, success_handler, public_app_execution_limit_per_minute, error_handler_fallback_to_instance_alerts, guest_access_enabled, guest_jwt_public_key, guest_jwt_jwks_url) SELECT $1, slack_team_id, slack_name, slack_command_script, slack_email, customer_id, plan, webhook, ai_config, large_file_storage, git_sync, default_app, default_scripts, deploy_ui, mute_critical_alerts, color, operator_settings, teams_command_script, teams_team_id, teams_team_name, git_app_installations, ducklake, dbt_warehouses, slack_oauth_client_id, slack_oauth_client_secret, datatable, teams_team_guid, auto_invite, error_handler, success_handler, public_app_execution_limit_per_minute, error_handler_fallback_to_instance_alerts, guest_access_enabled, guest_jwt_public_key, guest_jwt_jwks_url FROM workspace_settings WHERE workspace_id = $2", + "describe": { + "columns": [], + "parameters": { + "Left": [ + "Varchar", + "Text" + ] + }, + "nullable": [] + }, + "hash": "77d599e4f7c574dffac4824f37127c7ae2ef5f27d665ad9018f5cdea0f6f2cb1" +} diff --git a/backend/.sqlx/query-00a61afc5faa3826c283660417ff1f8a93060fe062a0b727f164329ab56387a2.json b/backend/.sqlx/query-dc4a57df3becc610f631ef22c116450390addbfae85fecc61c991d94167e6e99.json similarity index 89% rename from backend/.sqlx/query-00a61afc5faa3826c283660417ff1f8a93060fe062a0b727f164329ab56387a2.json rename to backend/.sqlx/query-dc4a57df3becc610f631ef22c116450390addbfae85fecc61c991d94167e6e99.json index a6a2b0c00a..01c0fd19af 100644 --- a/backend/.sqlx/query-00a61afc5faa3826c283660417ff1f8a93060fe062a0b727f164329ab56387a2.json +++ b/backend/.sqlx/query-dc4a57df3becc610f631ef22c116450390addbfae85fecc61c991d94167e6e99.json @@ -1,6 +1,6 @@ { "db_name": "PostgreSQL", - "query": "\n SELECT\n workspace_id,\n slack_team_id,\n teams_team_id,\n teams_team_name,\n teams_team_guid,\n slack_name,\n slack_command_script,\n teams_command_script,\n slack_email,\n slack_oauth_client_id,\n slack_oauth_client_secret,\n customer_id,\n plan,\n webhook,\n ai_config,\n dbt_warehouses,\n large_file_storage,\n datatable,\n ducklake,\n git_sync,\n deploy_ui,\n default_app,\n default_scripts,\n mute_critical_alerts,\n color,\n operator_settings,\n git_app_installations,\n auto_invite,\n error_handler,\n success_handler,\n public_app_execution_limit_per_minute,\n error_handler_fallback_to_instance_alerts,\n guest_access_enabled\n FROM\n workspace_settings\n WHERE\n workspace_id = $1\n ", + "query": "\n SELECT\n workspace_id,\n slack_team_id,\n teams_team_id,\n teams_team_name,\n teams_team_guid,\n slack_name,\n slack_command_script,\n teams_command_script,\n slack_email,\n slack_oauth_client_id,\n slack_oauth_client_secret,\n customer_id,\n plan,\n webhook,\n ai_config,\n dbt_warehouses,\n large_file_storage,\n datatable,\n ducklake,\n git_sync,\n deploy_ui,\n default_app,\n default_scripts,\n mute_critical_alerts,\n color,\n operator_settings,\n git_app_installations,\n auto_invite,\n error_handler,\n success_handler,\n public_app_execution_limit_per_minute,\n error_handler_fallback_to_instance_alerts,\n guest_access_enabled,\n guest_jwt_public_key,\n guest_jwt_jwks_url\n FROM\n workspace_settings\n WHERE\n workspace_id = $1\n ", "describe": { "columns": [ { @@ -167,6 +167,16 @@ "ordinal": 32, "name": "guest_access_enabled", "type_info": "Bool" + }, + { + "ordinal": 33, + "name": "guest_jwt_public_key", + "type_info": "Text" + }, + { + "ordinal": 34, + "name": "guest_jwt_jwks_url", + "type_info": "Text" } ], "parameters": { @@ -207,8 +217,10 @@ true, true, false, - false + false, + true, + true ] }, - "hash": "00a61afc5faa3826c283660417ff1f8a93060fe062a0b727f164329ab56387a2" + "hash": "dc4a57df3becc610f631ef22c116450390addbfae85fecc61c991d94167e6e99" } diff --git a/backend/.sqlx/query-e2eee8de61337b7d093f38e3e393e3620111119abf8dda4bde5b835ff934e4f1.json b/backend/.sqlx/query-e2eee8de61337b7d093f38e3e393e3620111119abf8dda4bde5b835ff934e4f1.json deleted file mode 100644 index 4d49d34d10..0000000000 --- a/backend/.sqlx/query-e2eee8de61337b7d093f38e3e393e3620111119abf8dda4bde5b835ff934e4f1.json +++ /dev/null @@ -1,15 +0,0 @@ -{ - "db_name": "PostgreSQL", - "query": "INSERT INTO workspace_settings (workspace_id, slack_team_id, slack_name, slack_command_script, slack_email, customer_id, plan, webhook, ai_config, large_file_storage, git_sync, default_app, default_scripts, deploy_ui, mute_critical_alerts, color, operator_settings, teams_command_script, teams_team_id, teams_team_name, git_app_installations, ducklake, dbt_warehouses, slack_oauth_client_id, slack_oauth_client_secret, datatable, teams_team_guid, auto_invite, error_handler, success_handler, public_app_execution_limit_per_minute, error_handler_fallback_to_instance_alerts, guest_access_enabled) SELECT $1, slack_team_id, slack_name, slack_command_script, slack_email, customer_id, plan, webhook, ai_config, large_file_storage, git_sync, default_app, default_scripts, deploy_ui, mute_critical_alerts, color, operator_settings, teams_command_script, teams_team_id, teams_team_name, git_app_installations, ducklake, dbt_warehouses, slack_oauth_client_id, slack_oauth_client_secret, datatable, teams_team_guid, auto_invite, error_handler, success_handler, public_app_execution_limit_per_minute, error_handler_fallback_to_instance_alerts, guest_access_enabled FROM workspace_settings WHERE workspace_id = $2", - "describe": { - "columns": [], - "parameters": { - "Left": [ - "Varchar", - "Text" - ] - }, - "nullable": [] - }, - "hash": "e2eee8de61337b7d093f38e3e393e3620111119abf8dda4bde5b835ff934e4f1" -} diff --git a/backend/.sqlx/query-f6fe63ef3518d2d1f321c2941bc59da15843f466c72159bf76712b124d7554b6.json b/backend/.sqlx/query-f6fe63ef3518d2d1f321c2941bc59da15843f466c72159bf76712b124d7554b6.json new file mode 100644 index 0000000000..d9b9aabf58 --- /dev/null +++ b/backend/.sqlx/query-f6fe63ef3518d2d1f321c2941bc59da15843f466c72159bf76712b124d7554b6.json @@ -0,0 +1,28 @@ +{ + "db_name": "PostgreSQL", + "query": "SELECT guest_jwt_public_key, guest_jwt_jwks_url FROM workspace_settings WHERE workspace_id = $1", + "describe": { + "columns": [ + { + "ordinal": 0, + "name": "guest_jwt_public_key", + "type_info": "Text" + }, + { + "ordinal": 1, + "name": "guest_jwt_jwks_url", + "type_info": "Text" + } + ], + "parameters": { + "Left": [ + "Text" + ] + }, + "nullable": [ + true, + true + ] + }, + "hash": "f6fe63ef3518d2d1f321c2941bc59da15843f466c72159bf76712b124d7554b6" +} diff --git a/backend/.sqlx/query-fbee9545c564f6fd611ca1a122cf420b03fd23304f78c382d6de14cb31bcd6b1.json b/backend/.sqlx/query-fbee9545c564f6fd611ca1a122cf420b03fd23304f78c382d6de14cb31bcd6b1.json new file mode 100644 index 0000000000..20b5c53392 --- /dev/null +++ b/backend/.sqlx/query-fbee9545c564f6fd611ca1a122cf420b03fd23304f78c382d6de14cb31bcd6b1.json @@ -0,0 +1,16 @@ +{ + "db_name": "PostgreSQL", + "query": "UPDATE workspace_settings SET guest_jwt_public_key = $1, guest_jwt_jwks_url = $2 WHERE workspace_id = $3", + "describe": { + "columns": [], + "parameters": { + "Left": [ + "Text", + "Text", + "Text" + ] + }, + "nullable": [] + }, + "hash": "fbee9545c564f6fd611ca1a122cf420b03fd23304f78c382d6de14cb31bcd6b1" +} diff --git a/backend/Cargo.lock b/backend/Cargo.lock index fc66ec630d..9678fbe993 100644 --- a/backend/Cargo.lock +++ b/backend/Cargo.lock @@ -14764,6 +14764,7 @@ dependencies = [ "git-version", "hex", "hmac", + "jsonwebtoken 8.3.0", "lazy_static", "once_cell", "opentelemetry 0.30.0", @@ -15601,6 +15602,7 @@ dependencies = [ "pep440_rs", "phf 0.11.3", "pin-project-lite", + "pkcs1", "postgres-native-tls 0.5.3", "prometheus", "quick_cache", @@ -15617,6 +15619,7 @@ dependencies = [ "serde_yml", "sha2 0.10.9", "size", + "spki", "sqlx", "strum", "strum_macros", diff --git a/backend/Cargo.toml b/backend/Cargo.toml index 4fcfcdad6e..8c6b6038b7 100644 --- a/backend/Cargo.toml +++ b/backend/Cargo.toml @@ -367,6 +367,7 @@ aws-config.workspace = true aws-credential-types.workspace = true hmac.workspace = true hex.workspace = true +jsonwebtoken = { workspace = true } [workspace.dependencies] @@ -599,6 +600,8 @@ const_format = { version = "0.2.35", features = ["rust_1_64", "rust_1_51"] } const-str = "0.5" constant_time_eq = "0.3.1" rsa = "^0" +spki = { version = "0.7", features = ["pem"] } +pkcs1 = "0.7" aes-gcm = "0.10.3" async_zip = { version = "0.0.17", features = ["tokio", "tokio-fs", "deflate", "chrono"] } once_cell = "1.17.1" diff --git a/backend/ee-repo-ref.txt b/backend/ee-repo-ref.txt index a7825f4ee7..a07d475cb9 100644 --- a/backend/ee-repo-ref.txt +++ b/backend/ee-repo-ref.txt @@ -1 +1 @@ -fb1c5c109846d6c47aff70ab6cc631f4fd773678 +c2270eb5fe2d9f0968253e6b460c33186363f4e7 diff --git a/backend/migrations/20260903071242_guest_jwt_entry.down.sql b/backend/migrations/20260903071242_guest_jwt_entry.down.sql new file mode 100644 index 0000000000..f3e758819b --- /dev/null +++ b/backend/migrations/20260903071242_guest_jwt_entry.down.sql @@ -0,0 +1,5 @@ +ALTER TABLE guest_activity DROP COLUMN jwt_entry; +ALTER TABLE workspace_settings + DROP CONSTRAINT workspace_settings_guest_jwt_one_key, + DROP COLUMN guest_jwt_public_key, + DROP COLUMN guest_jwt_jwks_url; diff --git a/backend/migrations/20260903071242_guest_jwt_entry.up.sql b/backend/migrations/20260903071242_guest_jwt_entry.up.sql new file mode 100644 index 0000000000..0cc449f539 --- /dev/null +++ b/backend/migrations/20260903071242_guest_jwt_entry.up.sql @@ -0,0 +1,15 @@ +-- A second way in for a guest: a JWT minted by the embedding customer's own backend and +-- verified against a key the workspace admin configured. One key shape per workspace, +-- a PEM public key or a JWKS URL, never both: a token is verified against exactly one +-- source, and two would make "which one refused it" undiagnosable. +ALTER TABLE workspace_settings + ADD COLUMN guest_jwt_public_key TEXT, + ADD COLUMN guest_jwt_jwks_url TEXT, + ADD CONSTRAINT workspace_settings_guest_jwt_one_key + CHECK (guest_jwt_public_key IS NULL OR guest_jwt_jwks_url IS NULL); + +-- Whether the guest came in on a JWT that day (as opposed to, or as well as, an +-- identity-provider sign-in). The seat telemetry reports the two entries apart, since +-- an app-only user routed through a guest JWT is one that `jwt_ext_` would have counted. +ALTER TABLE guest_activity + ADD COLUMN jwt_entry BOOLEAN NOT NULL DEFAULT false; diff --git a/backend/summarized_schema.txt b/backend/summarized_schema.txt index fe889705e4..61f8a66f85 100644 --- a/backend/summarized_schema.txt +++ b/backend/summarized_schema.txt @@ -110,7 +110,7 @@ folder_permission_history: id(bigint), workspace_id(char), folder_name(char), ch FK: (workspace_id, folder_name) -> folder(workspace_id, name) gcp_trigger: gcp_resource_path(char), topic_id(char), subscription_id(char), delivery_type(delivery_mode), delivery_config(jsonb), path(char), script_path(char), is_flow(bool), workspace_id(char), edited_by(char), email(char), edited_at(ts), extra_perms(jsonb), server_id(char), last_server_ping(ts), error(text), subscription_mode(gcp_subscription_mode), error_handler_path(char), error_handler_args(jsonb), retry(jsonb), auto_acknowledge_msg(bool), ack_deadline(int), mode(trigger_mode), labels(text[]) global_settings: name(char), value(jsonb), updated_at(ts) -guest_activity: email(char), workspace_id(char), day(date), last_seen_at(timestamptz) +guest_activity: email(char), workspace_id(char), day(date), last_seen_at(timestamptz), jwt_entry(bool) group_: workspace_id(char), name(char), summary(text), extra_perms(jsonb) FK: (workspace_id) -> workspace(id) group_permission_history: id(bigint), workspace_id(char), group_name(char), changed_by(char), changed_at(ts), change_type(char), member_affected(char) @@ -223,7 +223,7 @@ workspace_protection_rule: workspace_id(char), name(char), rules(int), bypass_gr FK: (workspace_id) -> workspace(id) workspace_runnable_dependencies: flow_path(char), runnable_path(char), script_hash(bigint), runnable_is_flow(bool), workspace_id(char), app_path(char) FK: (app_path, workspace_id) -> app(path, workspace_id) | (flow_path, workspace_id) -> flow(path, workspace_id) -workspace_settings: workspace_id(char), slack_team_id(char), slack_name(char), slack_command_script(char), slack_email(char), customer_id(char), plan(char), webhook(text), ai_config(jsonb), large_file_storage(jsonb), git_sync(jsonb), default_app(char), default_scripts(jsonb), deploy_ui(jsonb), mute_critical_alerts(bool), color(char), operator_settings(jsonb), teams_command_script(text), teams_team_id(text), teams_team_name(text), git_app_installations(jsonb), ducklake(jsonb), slack_oauth_client_id(char), slack_oauth_client_secret(char), datatable(jsonb), teams_team_guid(text), auto_invite(jsonb), error_handler(jsonb), success_handler(jsonb), public_app_execution_limit_per_minute(int), dbt_warehouses(jsonb), guest_access_enabled(bool) +workspace_settings: workspace_id(char), slack_team_id(char), slack_name(char), slack_command_script(char), slack_email(char), customer_id(char), plan(char), webhook(text), ai_config(jsonb), large_file_storage(jsonb), git_sync(jsonb), default_app(char), default_scripts(jsonb), deploy_ui(jsonb), mute_critical_alerts(bool), color(char), operator_settings(jsonb), teams_command_script(text), teams_team_id(text), teams_team_name(text), git_app_installations(jsonb), ducklake(jsonb), slack_oauth_client_id(char), slack_oauth_client_secret(char), datatable(jsonb), teams_team_guid(text), auto_invite(jsonb), error_handler(jsonb), success_handler(jsonb), public_app_execution_limit_per_minute(int), dbt_warehouses(jsonb), guest_access_enabled(bool), guest_jwt_public_key(text), guest_jwt_jwks_url(text) FK: (workspace_id) -> workspace(id) zombie_job_counter: job_id(uuid), counter(int) FK: (job_id) -> v2_job(id) diff --git a/backend/tests/app_guest_execution_mode.rs b/backend/tests/app_guest_execution_mode.rs index a47129e600..7af2e0794e 100644 --- a/backend/tests/app_guest_execution_mode.rs +++ b/backend/tests/app_guest_execution_mode.rs @@ -614,8 +614,8 @@ async fn guests_mode_needs_a_scopable_path(db: Pool) -> anyhow::Result Ok(()) } -/// Renaming a workspace copies its settings; the guest switch must travel with them, -/// or the rename silently shuts every guest app of the workspace. +/// Renaming a workspace copies its settings; the guest switch and the guest JWT key must +/// travel with them, or the rename silently shuts every guest app or drops the key. #[sqlx::test(fixtures("base"))] async fn a_workspace_rename_keeps_the_guest_switch(db: Pool) -> anyhow::Result<()> { initialize_tracing().await; @@ -629,6 +629,11 @@ async fn a_workspace_rename_keeps_the_guest_switch(db: Pool) -> anyhow ) .execute(&db) .await?; + sqlx::query( + "UPDATE workspace_settings SET guest_jwt_public_key = 'test-pem-key' WHERE workspace_id = 'test-workspace'", + ) + .execute(&db) + .await?; let resp = authed( client().post(format!( "http://localhost:{port}/api/w/test-workspace/workspaces/change_workspace_id" @@ -645,6 +650,16 @@ async fn a_workspace_rename_keeps_the_guest_switch(db: Pool) -> anyhow .fetch_one(&db) .await?; assert!(enabled, "the guest switch travels with the workspace"); + let jwt_key: Option = sqlx::query_scalar( + "SELECT guest_jwt_public_key FROM workspace_settings WHERE workspace_id = 'test-workspace-2'", + ) + .fetch_one(&db) + .await?; + assert_eq!( + jwt_key.as_deref(), + Some("test-pem-key"), + "the guest JWT key travels with the workspace" + ); let moved: bool = sqlx::query_scalar( "SELECT EXISTS(SELECT 1 FROM guest_activity WHERE workspace_id = 'test-workspace-2') AND NOT EXISTS(SELECT 1 FROM guest_activity WHERE workspace_id = 'test-workspace')", diff --git a/backend/tests/app_guest_jwt_allowance.rs b/backend/tests/app_guest_jwt_allowance.rs new file mode 100644 index 0000000000..85cb51d39e --- /dev/null +++ b/backend/tests/app_guest_jwt_allowance.rs @@ -0,0 +1,128 @@ +//! The guest allowance reached through a guest JWT (`jwt_guest_`). Its own binary +//! because `set_plan` flips a process-global license key, which a test sharing the +//! process could not tolerate (see `app_guest_allowance.rs`). +//! +//! Users from the `base` fixture: +//! test-user (admin, token SECRET_TOKEN) + +use serde_json::json; +use sqlx::{Pool, Postgres}; +use windmill_common::workspaces::FREE_GUESTS_PER_WINDOW; +use windmill_test_utils::*; + +const ADMIN_TOKEN: &str = "SECRET_TOKEN"; +const APP_PATH: &str = "u/test-user/guest_app"; + +fn client() -> reqwest::Client { + reqwest::Client::new() +} + +fn authed(builder: reqwest::RequestBuilder, token: &str) -> reqwest::RequestBuilder { + builder.header("Authorization", format!("Bearer {}", token)) +} + +/// Community and Pro are capped, Enterprise is metered. Only a build with both +/// `private` and `enterprise` can meter; every other build is capped whatever this says. +fn set_plan(pro: bool) { + #[cfg(feature = "private")] + windmill_common::ee::LICENSE_KEY_ID.store(std::sync::Arc::new( + if pro { "test_pro" } else { "" }.to_string(), + )); + let _ = pro; +} + +const JWT_PUB: &str = "-----BEGIN PUBLIC KEY-----\nMFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAEzAfqyCh34iYOCW0vg4ejq/zzJlzL\nSZScjnVyPjLGTapEwo4gc6/y1Yudd/v54wKh0OdfTfzAKMPWx/2NWx/ugg==\n-----END PUBLIC KEY-----\n"; +const JWT_PRIV: &str = "-----BEGIN PRIVATE KEY-----\nMIGHAgEAMBMGByqGSM49AgEGCCqGSM49AwEHBG0wawIBAQQgu27S2DbSwUh8BmQb\n/i4/VhNdoXV7PJekhnoceMULYLihRANCAATMB+rIKHfiJg4JbS+Dh6Or/PMmXMtJ\nlJyOdXI+MsZNqkTCjiBzr/LVi513+/njAqHQ519N/MAow9bH/Y1bH+6C\n-----END PRIVATE KEY-----\n"; + +fn guest_jwt(email: &str) -> String { + use jsonwebtoken::{encode, Algorithm, EncodingKey, Header}; + let exp = std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .unwrap() + .as_secs() + + 3600; + let claims = json!({ + "email": email, + "workspace_id": "test-workspace", + "app_path": APP_PATH, + "exp": exp, + }); + let jwt = encode( + &Header::new(Algorithm::ES256), + &claims, + &EncodingKey::from_ec_pem(JWT_PRIV.as_bytes()).unwrap(), + ) + .unwrap(); + format!("jwt_guest_{jwt}") +} + +/// A JWT guest is subject to the same allowance as a signed-in one. Past the cap on a +/// capped instance, a stranger's JWT is refused (the auth arm returns 401; the visitor +/// message is only logged, since the arm cannot carry it), while a guest already in the +/// window is let back in. +#[sqlx::test(fixtures("base"))] +async fn a_guest_jwt_is_capped_like_a_signed_in_guest(db: Pool) -> anyhow::Result<()> { + initialize_tracing().await; + let server = ApiServer::start(db.clone()).await?; + let port = server.addr.port(); + let ws = format!("http://localhost:{port}/api/w/test-workspace"); + + authed( + client().post(format!("{ws}/workspaces/edit_guest_access")), + ADMIN_TOKEN, + ) + .json(&json!({ "guest_access_enabled": true })) + .send() + .await?; + let resp = authed( + client().post(format!("{ws}/workspaces/edit_guest_jwt_key")), + ADMIN_TOKEN, + ) + .json(&json!({ "public_key": JWT_PUB })) + .send() + .await?; + assert_eq!(resp.status(), 200, "{}", resp.text().await?); + let resp = authed(client().post(format!("{ws}/apps/create")), ADMIN_TOKEN) + .json(&json!({ + "path": APP_PATH, + "summary": "Guest app", + "value": {}, + "policy": { "execution_mode": "guest", "triggerables_v2": {} } + })) + .send() + .await?; + assert_eq!(resp.status(), 201, "{}", resp.text().await?); + // The whole allowance, used today (g1..gN). + sqlx::query( + "INSERT INTO guest_activity (email, workspace_id, day) + SELECT 'g' || i || '@example.com', 'test-workspace', CURRENT_DATE + FROM generate_series(1, $1) AS i", + ) + .bind(FREE_GUESTS_PER_WINDOW) + .execute(&db) + .await?; + set_plan(true); + + let resp = authed( + client().get(format!("{ws}/users/whoami")), + &guest_jwt("stranger@example.com"), + ) + .send() + .await?; + assert_eq!(resp.status(), 401, "a stranger's JWT is refused past the cap"); + + let resp = authed( + client().get(format!("{ws}/users/whoami")), + &guest_jwt("g1@example.com"), + ) + .send() + .await?; + assert_eq!( + resp.status(), + 200, + "a returning guest's JWT is admitted: {}", + resp.text().await? + ); + + Ok(()) +} diff --git a/backend/tests/app_guest_jwt_entry.rs b/backend/tests/app_guest_jwt_entry.rs new file mode 100644 index 0000000000..7b84130a04 --- /dev/null +++ b/backend/tests/app_guest_jwt_entry.rs @@ -0,0 +1,494 @@ +//! Tests for the guest JWT entry: a guest that enters through a JWT the embedding +//! customer's own backend mints and signs, with no identity-provider round-trip. +//! +//! The key is a per-workspace setting (a PEM public key here), and the token is +//! verified per request against it. A JWT guest is the same identity as a signed-in +//! guest: no `usr` row, no `password` row, no seat, confined to the one app its +//! `app_path` names. These tests pin what a token must carry to be honoured, and the +//! refusals that keep the door narrow: wrong workspace, wrong key, expired, a +//! symmetric algorithm, an email that already has an account, an app not in guest +//! mode, and the workspace switch off. +//! +//! The keys are fixed test vectors (EC P-256, PKCS8), so signing is deterministic and +//! needs no key generation at runtime. + +// Built with these like the sibling guest-execution suite: the guest run executes as +// the publisher through EE on-behalf-of code. CI builds with them. +#![cfg(all(feature = "enterprise", feature = "private"))] + +use std::time::{SystemTime, UNIX_EPOCH}; + +use jsonwebtoken::{encode, Algorithm, EncodingKey, Header}; +use serde::Serialize; +use serde_json::json; +use sqlx::{Pool, Postgres}; +use windmill_test_utils::*; + +const ADMIN_TOKEN: &str = "SECRET_TOKEN"; +const APP_PATH: &str = "u/test-user/guest_app"; +const GUEST_EMAIL: &str = "guest@example.com"; + +// A P-256 keypair the workspace verifies against (PUB1), and a second private key +// (PRIV2) that it does not, for the wrong-key refusal. +const PRIV1: &str = "-----BEGIN PRIVATE KEY-----\nMIGHAgEAMBMGByqGSM49AgEGCCqGSM49AwEHBG0wawIBAQQgu27S2DbSwUh8BmQb\n/i4/VhNdoXV7PJekhnoceMULYLihRANCAATMB+rIKHfiJg4JbS+Dh6Or/PMmXMtJ\nlJyOdXI+MsZNqkTCjiBzr/LVi513+/njAqHQ519N/MAow9bH/Y1bH+6C\n-----END PRIVATE KEY-----\n"; +const PUB1: &str = "-----BEGIN PUBLIC KEY-----\nMFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAEzAfqyCh34iYOCW0vg4ejq/zzJlzL\nSZScjnVyPjLGTapEwo4gc6/y1Yudd/v54wKh0OdfTfzAKMPWx/2NWx/ugg==\n-----END PUBLIC KEY-----\n"; +const PRIV2: &str = "-----BEGIN PRIVATE KEY-----\nMIGHAgEAMBMGByqGSM49AgEGCCqGSM49AwEHBG0wawIBAQQgjyhWYyI2+z5zTT0B\neI9EuJJ7v0tcNXhvHrq9y2AG1LihRANCAAS40dEdO+tTffhGt4YQv0dStkd6VcWN\n+CHI9QqZAHAJMsNS3Ld+sZe2M6Of0CNR300QJtfp4UIdEVbXBCIxL1D0\n-----END PRIVATE KEY-----\n"; + +fn client() -> reqwest::Client { + reqwest::Client::new() +} + +fn authed(builder: reqwest::RequestBuilder, token: &str) -> reqwest::RequestBuilder { + builder.header("Authorization", format!("Bearer {token}")) +} + +fn now() -> u64 { + SystemTime::now() + .duration_since(UNIX_EPOCH) + .unwrap() + .as_secs() +} + +#[derive(Serialize)] +struct Claims { + email: String, + workspace_id: String, + app_path: String, + exp: u64, + #[serde(skip_serializing_if = "Option::is_none")] + nbf: Option, + #[serde(skip_serializing_if = "Option::is_none")] + iat: Option, +} + +impl Claims { + fn valid() -> Self { + Claims { + email: GUEST_EMAIL.to_string(), + workspace_id: "test-workspace".to_string(), + app_path: APP_PATH.to_string(), + exp: now() + 3600, + nbf: None, + iat: None, + } + } +} + +/// Sign as a bearer (`jwt_guest_`). `priv_pem`/`alg` let a test sign with the +/// wrong key or a refused algorithm. +fn bearer(claims: &Claims, priv_pem: &str, alg: Algorithm) -> String { + let key = match alg { + Algorithm::HS256 => EncodingKey::from_secret(b"a-shared-secret"), + _ => EncodingKey::from_ec_pem(priv_pem.as_bytes()).unwrap(), + }; + let jwt = encode(&Header::new(alg), claims, &key).unwrap(); + format!("jwt_guest_{jwt}") +} + +async fn enable_guests(port: u16, ws: &str, on: bool) -> anyhow::Result<()> { + let resp = authed( + client().post(format!( + "http://localhost:{port}/api/w/{ws}/workspaces/edit_guest_access" + )), + ADMIN_TOKEN, + ) + .json(&json!({ "guest_access_enabled": on })) + .send() + .await?; + assert_eq!(resp.status(), 200, "{}", resp.text().await?); + Ok(()) +} + +async fn set_guest_jwt_pem(port: u16, ws: &str, pem: &str) -> anyhow::Result<()> { + let resp = authed( + client().post(format!( + "http://localhost:{port}/api/w/{ws}/workspaces/edit_guest_jwt_key" + )), + ADMIN_TOKEN, + ) + .json(&json!({ "public_key": pem })) + .send() + .await?; + assert_eq!(resp.status(), 200, "{}", resp.text().await?); + Ok(()) +} + +fn app(path: &str, execution_mode: &str, sandbox: bool) -> serde_json::Value { + json!({ + "path": path, + "summary": "App", + "value": {}, + "policy": { + "execution_mode": execution_mode, + "sandbox": sandbox, + "triggerables_v2": { + "script/u/test-user/noop": { "static_inputs": {}, "one_of_inputs": {} } + } + } + }) +} + +async fn create_app(port: u16, ws: &str, v: serde_json::Value) -> anyhow::Result<()> { + let resp = authed( + client().post(format!("http://localhost:{port}/api/w/{ws}/apps/create")), + ADMIN_TOKEN, + ) + .json(&v) + .send() + .await?; + assert_eq!(resp.status(), 201, "{}", resp.text().await?); + Ok(()) +} + +fn whoami(port: u16, ws: &str, token: &str) -> reqwest::RequestBuilder { + authed( + client().get(format!("http://localhost:{port}/api/w/{ws}/users/whoami")), + token, + ) +} + +/// A valid guest JWT opens its app, runs a component as the publisher, reads the run +/// back, reports `role: guest`, and leaves exactly one `guest_activity` row however +/// many requests it makes. +#[sqlx::test(fixtures("base"))] +async fn a_valid_guest_jwt_opens_its_app(db: Pool) -> anyhow::Result<()> { + initialize_tracing().await; + let server = ApiServer::start(db.clone()).await?; + let port = server.addr.port(); + let ws = "test-workspace"; + + enable_guests(port, ws, true).await?; + set_guest_jwt_pem(port, ws, PUB1).await?; + let resp = authed( + client().post(format!("http://localhost:{port}/api/w/{ws}/scripts/create")), + ADMIN_TOKEN, + ) + .json(&json!({ + "path": "u/test-user/noop", + "summary": "", + "description": "", + "content": "echo 42", + "language": "bash", + })) + .send() + .await?; + assert_eq!(resp.status(), 201, "{}", resp.text().await?); + create_app(port, ws, app(APP_PATH, "guest", false)).await?; + + // A distinct email: the activity write is deduplicated by a process-global cache + // keyed on email, workspace and day, and other tests in this binary share the + // guest email, so the count below is only this test's if its email is its own. + let mut claims = Claims::valid(); + claims.email = "activity-guest@example.com".to_string(); + let token = bearer(&claims, PRIV1, Algorithm::ES256); + + let resp = whoami(port, ws, &token).send().await?; + assert_eq!(resp.status(), 200, "guest JWT must authenticate"); + let me: serde_json::Value = resp.json().await?; + assert_eq!(me["role"], json!("guest"), "must read as a guest"); + assert_eq!(me["operator"], json!(true)); + assert_eq!(me["is_admin"], json!(false)); + + let resp = authed( + client().post(format!( + "http://localhost:{port}/api/w/{ws}/apps_u/execute_component/{APP_PATH}" + )), + &token, + ) + .json(&json!({ "component": "a", "path": "script/u/test-user/noop", "args": {} })) + .send() + .await?; + assert_eq!(resp.status(), 200, "{}", resp.text().await?); + let job_id = resp.text().await?; + + let resp = authed( + client().get(format!( + "http://localhost:{port}/api/w/{ws}/jobs_u/getupdate/{job_id}" + )), + &token, + ) + .send() + .await?; + assert_eq!( + resp.status(), + 200, + "the guest that started the run must read it back: {}", + resp.text().await? + ); + + // Several requests, one row: the write is cached per email, workspace and day. + let count: i64 = sqlx::query_scalar( + "SELECT COUNT(*) FROM guest_activity WHERE email = $1 AND workspace_id = $2 AND jwt_entry", + ) + .bind(&claims.email) + .bind(ws) + .fetch_one(&db) + .await?; + assert_eq!(count, 1, "a JWT guest must leave exactly one activity row"); + + Ok(()) +} + +/// The refusals that keep the door narrow. Each presents a bearer on the workspace's +/// own `whoami`, which the arm reaches only after every gate, so a 401 is the arm +/// saying no rather than a handler. +#[sqlx::test(fixtures("base"))] +async fn guest_jwt_refusals(db: Pool) -> anyhow::Result<()> { + initialize_tracing().await; + let server = ApiServer::start(db.clone()).await?; + let port = server.addr.port(); + let ws = "test-workspace"; + + enable_guests(port, ws, true).await?; + set_guest_jwt_pem(port, ws, PUB1).await?; + create_app(port, ws, app(APP_PATH, "guest", false)).await?; + create_app(port, ws, app("u/test-user/members_app", "publisher", false)).await?; + + // Positive control: a token valid against this exact fixture is admitted. Without it a + // broken setup would 401 every bearer below and the whole suite would pass vacuously. + let control = whoami(port, ws, &bearer(&Claims::valid(), PRIV1, Algorithm::ES256)) + .send() + .await?; + assert_eq!(control.status(), 200, "{}", control.text().await?); + + // wrong workspace: the claim must name the route's workspace. + let mut c = Claims::valid(); + c.workspace_id = "other-ws".to_string(); + let wrong_ws = bearer(&c, PRIV1, Algorithm::ES256); + + // wrong key: signed with a key the workspace does not hold. + let wrong_key = bearer(&Claims::valid(), PRIV2, Algorithm::ES256); + + // expired, past the verifier's clock-skew leeway. + let mut c = Claims::valid(); + c.exp = now() - 120; + let expired = bearer(&c, PRIV1, Algorithm::ES256); + + // a symmetric algorithm is never accepted. + let hs256 = bearer(&Claims::valid(), PRIV1, Algorithm::HS256); + + // an email that already has an account is refused, not downgraded. + let mut c = Claims::valid(); + c.email = "test@windmill.dev".to_string(); + let has_account = bearer(&c, PRIV1, Algorithm::ES256); + + // an app not in guest mode. + let mut c = Claims::valid(); + c.app_path = "u/test-user/members_app".to_string(); + let not_guest_app = bearer(&c, PRIV1, Algorithm::ES256); + + // an existing account addressed in a different case still counts as an account: + // the base fixture holds `test@windmill.dev`. + let mut c = Claims::valid(); + c.email = "Test@Windmill.Dev".to_string(); + let mixed_case_account = bearer(&c, PRIV1, Algorithm::ES256); + + // a lifetime past the 24h cap, even with a valid signature. + let mut c = Claims::valid(); + c.exp = now() + 25 * 3600; + let over_lifetime_cap = bearer(&c, PRIV1, Algorithm::ES256); + + // an email with no `@` would become the guest's username and could be read as a + // `u/` or `g/` principal; refused. + let mut c = Claims::valid(); + c.email = "group-admins".to_string(); + let group_shaped_email = bearer(&c, PRIV1, Algorithm::ES256); + + // an email longer than the `guest_activity.email` column: refused before auth, so a + // guest is never admitted without the activity row and audit event the count needs. + let mut c = Claims::valid(); + c.email = format!("{}@example.com", "a".repeat(250)); + let oversized_email = bearer(&c, PRIV1, Algorithm::ES256); + + // an app_path carrying a scope metacharacter would widen the guest's scopes. + let mut c = Claims::valid(); + c.app_path = "u/test-user/*".to_string(); + let wildcard_app_path = bearer(&c, PRIV1, Algorithm::ES256); + + // a valid, signed token past the length cap: without the cap it would deserialize into + // GuestJwtClaims (the extra claim ignored) and verify, so this pins the length check. + let mut payload = serde_json::to_value(Claims::valid()).unwrap(); + payload["padding"] = serde_json::json!("a".repeat(9000)); + let big_jwt = encode( + &Header::new(Algorithm::ES256), + &payload, + &EncodingKey::from_ec_pem(PRIV1.as_bytes()).unwrap(), + ) + .unwrap(); + let oversized_token = format!("jwt_guest_{big_jwt}"); + + // a repeated prefix must not strip down to a valid short token that verifies and is then + // cached under the full bearer key (trim_start_matches would; strip_prefix must not). + let repeated_prefix = format!( + "jwt_guest_{}", + bearer(&Claims::valid(), PRIV1, Algorithm::ES256) + ); + + for (label, token) in [ + ("wrong workspace", wrong_ws), + ("wrong key", wrong_key), + ("expired", expired), + ("HS256", hs256), + ("email with an account", has_account), + ("app not in guest mode", not_guest_app), + ("mixed-case account", mixed_case_account), + ("over the 24h lifetime cap", over_lifetime_cap), + ("group-shaped email", group_shaped_email), + ("oversized email", oversized_email), + ("wildcard app_path", wildcard_app_path), + ("oversized token", oversized_token), + ("repeated prefix", repeated_prefix), + ] { + let resp = whoami(port, ws, &token).send().await?; + assert_eq!(resp.status(), 401, "{label} must be refused"); + } + + Ok(()) +} + +/// The workspace switch gates a JWT guest exactly as it gates a signed-in one, at the +/// auth door, so turning guests off closes the JWT entry too. +#[sqlx::test(fixtures("base"))] +async fn guest_jwt_needs_the_workspace_switch(db: Pool) -> anyhow::Result<()> { + initialize_tracing().await; + let server = ApiServer::start(db.clone()).await?; + let port = server.addr.port(); + let ws = "test-workspace"; + + set_guest_jwt_pem(port, ws, PUB1).await?; + create_app(port, ws, app(APP_PATH, "guest", false)).await?; + let token = bearer(&Claims::valid(), PRIV1, Algorithm::ES256); + + // Switch off (the default): refused. + let resp = whoami(port, ws, &token).send().await?; + assert_eq!( + resp.status(), + 401, + "a JWT guest must be refused while guests are off" + ); + + // Switch on: through. + enable_guests(port, ws, true).await?; + let resp = whoami(port, ws, &token).send().await?; + assert_eq!( + resp.status(), + 200, + "with guests on, the JWT guest is admitted" + ); + + // Off again: closed on the next request. + enable_guests(port, ws, false).await?; + let resp = whoami(port, ws, &token).send().await?; + assert_eq!( + resp.status(), + 401, + "turning guests off closes the JWT guest again" + ); + + Ok(()) +} + +/// A guest JWT is pinned to the workspace its claim names, so it authenticates on no +/// workspace-less route: the arm has no workspace to check the claim against. +#[sqlx::test(fixtures("base"))] +async fn guest_jwt_rejected_on_workspaceless_route(db: Pool) -> anyhow::Result<()> { + initialize_tracing().await; + let server = ApiServer::start(db.clone()).await?; + let port = server.addr.port(); + let ws = "test-workspace"; + + enable_guests(port, ws, true).await?; + set_guest_jwt_pem(port, ws, PUB1).await?; + create_app(port, ws, app(APP_PATH, "guest", false)).await?; + let token = bearer(&Claims::valid(), PRIV1, Algorithm::ES256); + + let resp = authed( + client().get(format!("http://localhost:{port}/api/users/tokens/list")), + &token, + ) + .send() + .await?; + assert_eq!( + resp.status(), + 401, + "a guest JWT must not authenticate on a workspace-less route" + ); + + Ok(()) +} + +/// An embed token a JWT guest mints for a sandboxed app is capped at the JWT's own +/// expiry: a JWT has no token row, so the cap is carried through the auth cache. It +/// must not outlive the JWT, which is the guest's only revocation. +#[sqlx::test(fixtures("base"))] +async fn a_guest_jwt_derived_embed_token_is_capped(db: Pool) -> anyhow::Result<()> { + initialize_tracing().await; + let server = ApiServer::start(db.clone()).await?; + let port = server.addr.port(); + let ws = "test-workspace"; + + enable_guests(port, ws, true).await?; + set_guest_jwt_pem(port, ws, PUB1).await?; + create_app(port, ws, app(APP_PATH, "guest", true)).await?; + let secret: String = authed( + client().get(format!( + "http://localhost:{port}/api/w/{ws}/apps/secret_of/{APP_PATH}" + )), + ADMIN_TOKEN, + ) + .send() + .await? + .text() + .await?; + + let claims = Claims::valid(); + let jwt_exp = claims.exp; + let token = bearer(&claims, PRIV1, Algorithm::ES256); + + let resp = authed( + client().get(format!( + "http://localhost:{port}/api/w/{ws}/apps_u/embed_token/{secret}" + )), + &token, + ) + .send() + .await?; + assert_eq!(resp.status(), 200, "{}", resp.text().await?); + let body: serde_json::Value = resp.json().await?; + let child_exp: chrono::DateTime = body["expiration"] + .as_str() + .and_then(|e| e.parse().ok()) + .expect("mint must return the token's expiration"); + assert!( + child_exp.timestamp() as u64 <= jwt_exp, + "the derived embed token ({child_exp}) must not outlive the JWT (exp {jwt_exp})" + ); + + // And it resolves as a guest. + let embed = body["token"].as_str().expect("mint must return a token"); + let resp = whoami(port, ws, embed).send().await?; + assert_eq!(resp.status(), 200); + let me: serde_json::Value = resp.json().await?; + assert_eq!(me["role"], json!("guest")); + + Ok(()) +} + +/// A workspace with no guest key of its own falls back to the instance issuer +/// (`JWT_EXT_JWKS_URL`), so an operator running one issuer configures it once. Verified as a +/// guest here in CE; a full login from that issuer stays EE (`jwt_ext_`). +#[sqlx::test(fixtures("base"))] +async fn no_workspace_key_falls_back_to_the_instance_issuer( + db: Pool, +) -> anyhow::Result<()> { + use windmill_common::guest_jwt::{key_source, GuestJwtKeySource}; + let url = "https://issuer.example.com/jwks.json"; + unsafe { std::env::set_var("JWT_EXT_JWKS_URL", url) }; + let src = key_source(&db, "test-workspace").await; + unsafe { std::env::remove_var("JWT_EXT_JWKS_URL") }; + assert!( + matches!(src?, Some(GuestJwtKeySource::JwksUrl(u)) if u == url), + "no workspace key falls back to the instance issuer" + ); + Ok(()) +} diff --git a/backend/tests/postgres_trigger_scope.rs b/backend/tests/postgres_trigger_scope.rs index e8ba36ebb9..1080138eb0 100644 --- a/backend/tests/postgres_trigger_scope.rs +++ b/backend/tests/postgres_trigger_scope.rs @@ -25,6 +25,7 @@ fn scoped_authed(scopes: Vec<&str>) -> ApiAuthed { token_prefix: None, read_only: false, job_id: None, + credential_expiry: None, } } diff --git a/backend/tests/trigger_listener_queries.rs b/backend/tests/trigger_listener_queries.rs index 1463138167..0af99136e7 100644 --- a/backend/tests/trigger_listener_queries.rs +++ b/backend/tests/trigger_listener_queries.rs @@ -178,6 +178,7 @@ fn make_authed() -> windmill_api_auth::ApiAuthed { token_prefix: None, read_only: false, job_id: None, + credential_expiry: None, } } diff --git a/backend/tests/wm_token_confinement.rs b/backend/tests/wm_token_confinement.rs index 2d3f1373fe..39b86c59a1 100644 --- a/backend/tests/wm_token_confinement.rs +++ b/backend/tests/wm_token_confinement.rs @@ -1094,6 +1094,7 @@ async fn test_privilege_gates_reject_a_job_token_directly( token_prefix: None, read_only: false, job_id, + credential_expiry: None, } } diff --git a/backend/windmill-api-assets/tests/dbt_pinned_graph.rs b/backend/windmill-api-assets/tests/dbt_pinned_graph.rs index 7f4979a96f..f2bb98a46c 100644 --- a/backend/windmill-api-assets/tests/dbt_pinned_graph.rs +++ b/backend/windmill-api-assets/tests/dbt_pinned_graph.rs @@ -33,6 +33,7 @@ fn outsider() -> ApiAuthed { token_prefix: None, read_only: false, job_id: None, + credential_expiry: None, } } diff --git a/backend/windmill-api-auth/src/auth.rs b/backend/windmill-api-auth/src/auth.rs index 0663c0b97b..6e5aa6b1da 100644 --- a/backend/windmill-api-auth/src/auth.rs +++ b/backend/windmill-api-auth/src/auth.rs @@ -175,6 +175,18 @@ impl AuthCache { if is_no_auth() { return Some(OptJobAuthed { authed: no_auth_admin_authed(), job_id: None }); } + // Reject an oversized guest bearer before the cache key is built from it: the key + // copies and hashes the whole token, so the cap should bound that work too. Log it + // like the other guest refusals, since get_opt_job_authed turns None into a bare 401. + if token.starts_with(windmill_common::guest_jwt::BEARER_PREFIX) + && token.len() > windmill_common::guest_jwt::MAX_GUEST_JWT_LEN + { + tracing::error!( + "guest JWT refused: bearer is longer than {} bytes", + windmill_common::guest_jwt::MAX_GUEST_JWT_LEN + ); + return None; + } let key = ( w_id.as_ref().unwrap_or(&"".to_string()).to_string(), token.to_string(), @@ -216,6 +228,111 @@ impl AuthCache { None } } + _ if token.starts_with(windmill_common::guest_jwt::BEARER_PREFIX) => { + // A workspace-less route never accepts a guest JWT: the identity is + // pinned to the workspace its claim names, like a DB guest session. + let Some(w_id) = w_id.as_deref() else { + return None; + }; + // Strip exactly one prefix: `trim_start_matches` would strip repeated prefixes, + // so `jwt_guest_jwt_guest_` would reduce to a valid token that verifies and + // is then cached under the full, non-canonical bearer key. + let jwt = token + .strip_prefix(windmill_common::guest_jwt::BEARER_PREFIX) + .unwrap_or(token); + let claims = + match windmill_common::guest_jwt::verify_for_workspace(&self.db, w_id, jwt) + .await + { + Ok(c) => c, + Err(e) => { + tracing::error!("guest JWT auth error for {w_id}: {e:#}"); + return None; + } + }; + // The workspace switch, the instance switch and the app being in guest + // mode, in one answer (guest_app_admits). The door re-reads the switches + // and the no-account rule per request through the sentinel below + // (guest_session_stands), so turning any of them off stops a cached JWT + // session on its next call. + match windmill_common::workspaces::guest_app_admits( + &self.db, + w_id, + &claims.app_path, + ) + .await + { + Ok(true) => {} + Ok(false) => return None, + Err(e) => { + tracing::error!("guest JWT admit check failed for {w_id}: {e:#}"); + return None; + } + } + // Resolve on the lowercased email: accounts are stored lowercased, so a + // mixed-case claim would otherwise slip past the no-account gate and + // resolve an account holder to a guest, and split the activity rows the + // seat count reads. + let email = claims.email.to_lowercase(); + // A guest is someone with no account at all; an account holder is refused, + // never downgraded (the same rule as the signed-in guest mint). + match windmill_common::users::has_any_account(&self.db, &email).await { + Ok(false) => {} + Ok(true) => return None, + Err(e) => { + tracing::error!("guest JWT account check failed: {e:#}"); + return None; + } + } + // The instance allowance, checked and recorded transactionally. A stranger + // past the cap on a capped instance is refused here; a returning guest + // always passes. Recording an account holder is avoided by the check above. + if !admit_and_record_guest_jwt(&self.db, w_id, &email, &claims.app_path).await { + return None; + } + // guest_session_scopes already carries the sentinel, and it is the whole + // grant; a JWT has no label, so the sentinel is what governs it. It also + // re-checks the path holds no scope metacharacter (verify already did). + let scopes = match crate::scopes::guest_session_scopes(&claims.app_path) { + Ok(s) => Some(s), + Err(e) => { + tracing::error!("guest JWT app_path cannot be scoped for {w_id}: {e:#}"); + return None; + } + }; + // The JWT's own expiry caps a token minted from this session. The auth + // cache entry itself is capped far shorter (GUEST_JWT_CACHE_TTL) so a + // rotated or cleared key stops the session on re-verification, within + // minutes, rather than only at exp (up to 24h away). + let credential_expiry = + chrono::Utc.timestamp_nanos(claims.exp as i64 * 1_000_000_000); + let cache_expiry = credential_expiry.min(chrono::Utc::now() + GUEST_JWT_CACHE_TTL); + let authed = ApiAuthed { + username: email.clone(), + email, + is_admin: false, + is_operator: true, + groups: vec![], + folders: vec![], + scopes, + username_override: None, + username_override_is_token_label: false, + is_session_token: false, + token_prefix: Some(safe_token_prefix(token)), + read_only: false, + job_id: None, + credential_expiry: Some(credential_expiry), + }; + AUTH_CACHE.insert( + key, + ExpiringAuthCache { + authed: authed.clone(), + expiry: cache_expiry, + job_id: None, + }, + ); + Some(OptJobAuthed { authed, job_id: None }) + } _ if token.starts_with("jwt_") => { let jwt_token = token.trim_start_matches("jwt_"); @@ -249,6 +366,7 @@ impl AuthCache { token_prefix: claims.audit_span, read_only: false, job_id: None, + credential_expiry: None, }; // Fail closed: a `job_id` claim that does not parse must reject // the token rather than resolve to `None`, which would clear the @@ -363,6 +481,7 @@ impl AuthCache { token_prefix: Some(safe_token_prefix(token)), read_only, job_id: None, + credential_expiry: None, }) } else { tracing::warn!( @@ -416,6 +535,7 @@ impl AuthCache { token_prefix: Some(safe_token_prefix(token)), read_only, job_id: None, + credential_expiry: None, }) } else { tracing::warn!( @@ -494,6 +614,7 @@ impl AuthCache { token_prefix: Some(safe_token_prefix(token)), read_only, job_id: None, + credential_expiry: None, }) } None if super_admin => { @@ -518,6 +639,7 @@ impl AuthCache { token_prefix: Some(safe_token_prefix(token)), read_only, job_id: None, + credential_expiry: None, }), Err(e) => { tracing::error!( @@ -555,6 +677,7 @@ impl AuthCache { token_prefix: Some(safe_token_prefix(token)), read_only, job_id: None, + credential_expiry: None, }) } None => None, @@ -574,6 +697,7 @@ impl AuthCache { token_prefix: Some(safe_token_prefix(token)), read_only, job_id: None, + credential_expiry: None, }) } } @@ -612,6 +736,7 @@ impl AuthCache { token_prefix: Some(safe_token_prefix(token)), read_only: false, job_id: None, + credential_expiry: None, }; Some(OptJobAuthed { authed, job_id: None }) } else { @@ -622,6 +747,127 @@ impl AuthCache { } } +/// How long a guest JWT resolves from the auth cache before the arm re-runs (and +/// re-reads the key). A guest JWT is not revocable except by the workspace switch or +/// by rotating the key, so the entry must be short enough that a rotated key bites +/// soon, unlike a normal token whose row can be deleted. Also what makes the +/// day-keyed activity dedupe below reachable across a midnight. +const GUEST_JWT_CACHE_TTL: chrono::Duration = chrono::Duration::minutes(5); + +/// A refused JWT (a stranger past the allowance) is remembered this long so a replayed +/// bearer does not take the instance-wide allowance advisory lock on every request. +/// Short, so a stranger admitted once the window frees is re-checked soon. +const GUEST_JWT_REFUSED_TTL: std::time::Duration = std::time::Duration::from_secs(30); + +lazy_static::lazy_static! { + // One `guest_activity` upsert and one `users.login_guest` audit per email, + // workspace and day: the arm re-runs every GUEST_JWT_CACHE_TTL, and neither the + // seat scan nor the audit trail wants a write each time. LRU-bounded; the day is in + // the key, so a new day writes again. + static ref GUEST_JWT_ACTIVITY_CACHE: Cache = Cache::new(2000); + static ref GUEST_JWT_REFUSED_CACHE: Cache = Cache::new(2000); +} + +/// Admit a JWT guest against the instance allowance and record today's activity, in one +/// transaction so the advisory lock in `guest_admission` spans the count check and the +/// row that changes it. Returns false when the allowance refuses the email or on a DB +/// error, both of which deny the guest. Cached per email, workspace and day: a bearer +/// replayed every request runs this at most once a day, and a refused one is remembered +/// briefly so it does not re-take the allowance lock. `email` is already lowercased. +async fn admit_and_record_guest_jwt(db: &DB, w_id: &str, email: &str, app_path: &str) -> bool { + let cache_key = format!("{email}|{w_id}|{}", chrono::Utc::now().date_naive()); + if GUEST_JWT_ACTIVITY_CACHE.get(&cache_key).is_some() { + return true; + } + if GUEST_JWT_REFUSED_CACHE + .get(&cache_key) + .is_some_and(|at| at.elapsed() < GUEST_JWT_REFUSED_TTL) + { + return false; + } + let mut tx = match db.begin().await { + Ok(tx) => tx, + Err(e) => { + tracing::error!("guest JWT tx begin failed for {w_id}: {e:#}"); + return false; + } + }; + // The allowance and the row that changes it, in one transaction: guest_admission + // takes a transaction-scoped advisory lock, so the count check and the insert cannot + // race two strangers past the cap. Only a real allowance refusal is negative-cached; + // a transient DB error denies this request but must not lock the email out for 30s. + match windmill_common::workspaces::guest_admission(&mut *tx, email).await { + Ok(()) => {} + Err(e @ windmill_common::error::Error::PermissionDenied(_)) => { + // The guest hits a bare 401 (the reason must not leak to an unauthenticated caller); + // warn so an admin sees the cap in logs, since it is the actionable signal here. + tracing::warn!("guest JWT refused (guest allowance) for {w_id}: {e:#}"); + GUEST_JWT_REFUSED_CACHE.insert(cache_key, std::time::Instant::now()); + return false; + } + Err(e) => { + tracing::error!("guest JWT allowance check failed for {w_id}: {e:#}"); + return false; + } + } + // The conditional `WHERE NOT jwt_entry` flips the flag only on its false-to-true + // transition, so the upsert returns a row exactly once per email per day: on the + // fresh insert, or on the first JWT after an identity-provider sign-in created + // today's row with `jwt_entry = false`. The audit is gated on that, decided + // atomically by the conflicting tuple, so concurrent first requests (a metered + // instance takes no advisory lock) audit at most once. + let first_jwt = sqlx::query_scalar!( + r#"INSERT INTO guest_activity (email, workspace_id, day, jwt_entry) + VALUES ($1, $2, CURRENT_DATE, true) + ON CONFLICT (email, workspace_id, day) + DO UPDATE SET jwt_entry = true, last_seen_at = now() + WHERE NOT guest_activity.jwt_entry + RETURNING 1 AS "audited!""#, + email, + w_id, + ) + .fetch_optional(&mut *tx) + .await; + let first_jwt = match first_jwt { + Ok(v) => v.is_some(), + Err(e) => { + tracing::error!("recording guest JWT activity for {w_id}: {e:#}"); + return false; + } + }; + if let Err(e) = tx.commit().await { + tracing::error!("guest JWT tx commit failed for {w_id}: {e:#}"); + return false; + } + GUEST_JWT_ACTIVITY_CACHE.insert(cache_key, ()); + // Audit last, best-effort, on its own connection: the EE writer swallows an + // `audit_partitioned` failure but that failing statement still aborts the + // transaction it runs in, so auditing before the commit would let the whole + // activity row roll back while this returned success, admitting an uncounted guest. + if first_jwt { + let author = windmill_common::audit::AuditAuthor { + email: email.to_string(), + username: email.to_string(), + username_override: None, + token_prefix: None, + }; + if let Err(e) = windmill_audit::audit_oss::audit_log( + db, + &author, + "users.login_guest", + windmill_audit::ActionKind::Create, + w_id, + Some(app_path), + Some([("entry", "jwt")].into()), + ) + .await + { + tracing::error!("auditing guest JWT login for {w_id}: {e:#}"); + } + } + true +} + pub(crate) async fn extract_token(parts: &mut Parts, state: &S) -> Option { let auth_header = parts .headers @@ -822,6 +1068,7 @@ fn no_auth_admin_authed() -> ApiAuthed { token_prefix: None, read_only: false, job_id: None, + credential_expiry: None, } } diff --git a/backend/windmill-api-auth/src/lib.rs b/backend/windmill-api-auth/src/lib.rs index f2392839ba..b9edde3c6d 100644 --- a/backend/windmill-api-auth/src/lib.rs +++ b/backend/windmill-api-auth/src/lib.rs @@ -78,6 +78,11 @@ pub struct ApiAuthed { /// member can point at a superadmin, so it must never be trusted as a global /// superadmin (`require_super_admin`), GHSA-hfh4-cx4h-3fcr. pub job_id: Option, + /// When this credential itself expires, if it carries its own expiry rather than a + /// token row. Set for a guest JWT (its `exp`): a token minted from it is capped at + /// this, since the JWT's expiry is a guest's only revocation and there is no row to + /// look the limit up in. `None` for every credential whose limit lives in `token`. + pub credential_expiry: Option>, } impl ApiAuthed { @@ -165,6 +170,7 @@ impl From for ApiAuthed { token_prefix: value.token_prefix, read_only: false, job_id: None, + credential_expiry: None, } } } @@ -1074,6 +1080,7 @@ pub async fn fetch_api_authed_from_permissioned_as( token_prefix: authed.token_prefix, read_only: false, job_id: None, + credential_expiry: None, }; API_AUTHED_CACHE.insert( diff --git a/backend/windmill-api-auth/src/scopes.rs b/backend/windmill-api-auth/src/scopes.rs index 519d814f15..47ea12c04b 100644 --- a/backend/windmill-api-auth/src/scopes.rs +++ b/backend/windmill-api-auth/src/scopes.rs @@ -793,6 +793,33 @@ pub fn with_guest_sentinel(mut scopes: Vec) -> Vec { scopes } +/// Scopes a guest session carries. The broad-looking reads are narrowed to a route +/// allowlist by the sentinel (`guest_route_denied`), plus the two path-scoped app +/// grants. A guest has no `usr` row, so this list is the whole of what it can do. The +/// single source both the mint (a signed-in guest) and the JWT auth arm build from. +/// +/// The sentinel here only narrows. A signed-in guest is made one by the server-minted +/// label; a JWT guest has no label, so for it the sentinel is what governs. +pub fn guest_session_scopes(app_path: &str) -> windmill_common::error::Result> { + // The path is spliced into a scope, whose grammar reserves `:`, `,`, `*` and a leading + // `/`; app paths may otherwise carry spaces and `@`, so guard only those reserved chars. + if !windmill_common::auth::is_scope_literal_path(app_path) { + return Err(windmill_common::error::Error::BadRequest(format!( + "app path {app_path} is empty or cannot be scoped: `:`, `,` and `*` are reserved \ + in scopes, and a leading `/` never matches a route" + ))); + } + Ok(vec![ + GUEST_SENTINEL.to_string(), + "jobs:read".to_string(), + "resources:run".to_string(), + "users:read".to_string(), + "folders:read".to_string(), + format!("apps:read:{app_path}"), + format!("apps:run:{app_path}"), + ]) +} + /// Sentinel in raw-app SDK tokens. Grants nothing; `check_route_access` uses it /// to narrow the declared scopes to what the viewer's prompt promised. pub const RAW_APP_SDK_SENTINEL: &str = "raw_app_sdk"; diff --git a/backend/windmill-api-integration-tests/tests/native_triggers.rs b/backend/windmill-api-integration-tests/tests/native_triggers.rs index fe80e5fdf4..06c47cbb14 100644 --- a/backend/windmill-api-integration-tests/tests/native_triggers.rs +++ b/backend/windmill-api-integration-tests/tests/native_triggers.rs @@ -63,6 +63,7 @@ fn test_authed() -> ApiAuthed { token_prefix: None, read_only: false, job_id: None, + credential_expiry: None, } } diff --git a/backend/windmill-api-users/src/users.rs b/backend/windmill-api-users/src/users.rs index bda651a8cf..afe7580055 100644 --- a/backend/windmill-api-users/src/users.rs +++ b/backend/windmill-api-users/src/users.rs @@ -2938,31 +2938,6 @@ lazy_static::lazy_static! { .unwrap_or(8 * 60 * 60); } -/// Scopes a guest session carries. Mirrors `APP_EMBED_SCOPES` — the same broad-looking -/// reads narrowed to a route allowlist by the sentinel (`guest_route_denied`) — plus the -/// two path-scoped app grants minted per app. With no ACL of its own, this list is the -/// whole of what a guest can do. -/// -/// The `guest` sentinel here only narrows. What makes the session a guest at all is the -/// server-minted label ([`windmill_common::auth::GUEST_SESSION_LABEL`]). -fn guest_session_scopes(app_path: &str) -> Result> { - if !windmill_common::auth::is_scope_literal_path(app_path) { - return Err(Error::BadRequest(format!( - "app path {app_path} cannot be scoped: `:`, `,` and `*` are reserved in scopes, \ - and a leading `/` never matches a route" - ))); - } - Ok(vec![ - windmill_api_auth::scopes::GUEST_SENTINEL.to_string(), - "jobs:read".to_string(), - "resources:run".to_string(), - "users:read".to_string(), - "folders:read".to_string(), - format!("apps:read:{app_path}"), - format!("apps:run:{app_path}"), - ]) -} - /// Mint a browser session for someone the identity provider authenticated who is a /// member of no workspace, so they can open one guest-mode app. Writes no `password` /// and no `usr` row: that absence is what keeps a guest off every seat counter, so @@ -2994,20 +2969,11 @@ pub async fn create_guest_session_token<'c>( } else { Some(&token) }; - let scopes = guest_session_scopes(app_path)?; + let scopes = windmill_api_auth::scopes::guest_session_scopes(app_path)?; - // No account at all (see `ExecutionMode::Guest`): a deactivated `password` row - // counts, since the sign-in path's own lookup filters on `disabled = false` and a - // SCIM-offboarded account would otherwise read as absent; so does a `usr` row in - // any workspace, which is what a service account has instead of a password. - let has_account: bool = sqlx::query_scalar( - "SELECT EXISTS(SELECT 1 FROM password WHERE email = $1) - OR EXISTS(SELECT 1 FROM usr WHERE email = $1)", - ) - .bind(email) - .fetch_one(&mut **tx) - .await?; - if has_account { + // No account at all (see `has_any_account`): an account holder is refused a guest + // session, never handed a second, cheaper identity. The same helper the JWT arm uses. + if windmill_common::users::has_any_account(&mut **tx, email).await? { return Err(Error::NotAuthorized( "an existing account cannot hold a guest session".to_string(), )); @@ -3061,7 +3027,7 @@ pub async fn create_guest_session_token<'c>( ActionKind::Create, w_id, Some(app_path), - None, + Some([("entry", "idp")].into()), ) .await?; diff --git a/backend/windmill-api-workspaces/src/workspaces.rs b/backend/windmill-api-workspaces/src/workspaces.rs index f8bb95fd7d..620e830a6a 100644 --- a/backend/windmill-api-workspaces/src/workspaces.rs +++ b/backend/windmill-api-workspaces/src/workspaces.rs @@ -12,10 +12,10 @@ use windmill_api_auth::{ }; use windmill_api_users::users::WorkspaceInvite; use windmill_common::email_oss::send_email_if_possible; -use windmill_dep_map::lock_hash::record_lock_hashes_for_workspace; use windmill_common::usernames::{get_instance_username_or_create_pending, VALID_USERNAME}; use windmill_common::webhook::WebhookShared; use windmill_common::{BASE_URL, DB}; +use windmill_dep_map::lock_hash::record_lock_hashes_for_workspace; use axum::{ extract::{Extension, Path, Query}, @@ -152,6 +152,7 @@ pub fn workspaced_service() -> Router { .route("/edit_deploy_ui_config", post(edit_deploy_ui_config)) .route("/edit_default_app", post(edit_default_app)) .route("/edit_guest_access", post(edit_guest_access)) + .route("/edit_guest_jwt_key", post(edit_guest_jwt_key)) .route("/guest_usage", get(get_guest_usage)) .route("/default_app", get(get_default_app)) .route( @@ -322,6 +323,14 @@ pub struct WorkspaceSettings { /// Whether this workspace admits guest sessions (`ExecutionMode::Guest`). An app's /// own `execution_mode: guest` is inert while this is off. pub guest_access_enabled: bool, + /// The key a guest JWT is verified against: a PEM public key, or a JWKS URL, at most + /// one (a DB CHECK enforces it). Public material, not a secret, so it is admin- + /// readable here. `None`/`None` falls back to the instance issuer (`JWT_EXT_JWKS_URL`) + /// off cloud, or accepts no JWT guest if none is set; `guest_access_enabled` is the switch. + #[serde(skip_serializing_if = "Option::is_none")] + pub guest_jwt_public_key: Option, + #[serde(skip_serializing_if = "Option::is_none")] + pub guest_jwt_jwks_url: Option, } /// Subset of `WorkspaceSettings` that is safe to return to any workspace @@ -1082,7 +1091,9 @@ async fn get_settings( success_handler, public_app_execution_limit_per_minute, error_handler_fallback_to_instance_alerts, - guest_access_enabled + guest_access_enabled, + guest_jwt_public_key, + guest_jwt_jwks_url FROM workspace_settings WHERE @@ -4661,6 +4672,66 @@ async fn edit_guest_access( )) } +#[derive(Deserialize)] +struct EditGuestJwtKey { + /// A PEM public key (RS or ES family), or a JWKS URL, at most one. Both empty clears the + /// workspace key; verification then falls back to the instance issuer (`JWT_EXT_JWKS_URL`) + /// off cloud, or refuses the JWT if none is set. The off-switch is `guest_access_enabled`. + public_key: Option, + jwks_url: Option, +} + +/// Configure the key a guest JWT (`jwt_guest_`) is verified against for this workspace. +/// Workspace-admin gated, like the guest switch: guests are free up to the instance +/// allowance on any plan, so configuring their key needs no licence. The key is +/// validated before it is stored so a typo is refused here, not silently on every guest +/// later: a PEM must parse as an RS/ES public key (HS* has no PEM form and is +/// unreachable), and a JWKS URL must be fetchable and hold at least one usable signing key. +async fn edit_guest_jwt_key( + authed: ApiAuthed, + Extension(db): Extension, + Path(w_id): Path, + Json(EditGuestJwtKey { public_key, jwks_url }): Json, +) -> Result { + require_admin(authed.is_admin, &authed.username)?; + let public_key = public_key.filter(|s| !s.trim().is_empty()); + let jwks_url = jwks_url.filter(|s| !s.trim().is_empty()); + if public_key.is_some() && jwks_url.is_some() { + return Err(Error::BadRequest( + "Set a PEM public key or a JWKS URL, not both".to_string(), + )); + } + if let Some(pem) = public_key.as_deref() { + windmill_common::guest_jwt::decoding_key_from_pem(pem)?; + } + if let Some(url) = jwks_url.as_deref() { + windmill_common::guest_jwt::fetch_jwks(url).await?; + } + + let mut tx = db.begin().await?; + sqlx::query!( + "UPDATE workspace_settings SET guest_jwt_public_key = $1, guest_jwt_jwks_url = $2 WHERE workspace_id = $3", + public_key, + jwks_url, + &w_id + ) + .execute(&mut *tx) + .await?; + audit_log( + &mut *tx, + &authed, + "workspaces.edit_guest_jwt_key", + ActionKind::Update, + &w_id, + None, + None, + ) + .await?; + tx.commit().await?; + + Ok(format!("Guest JWT key updated for workspace {w_id}")) +} + async fn edit_default_scripts( authed: ApiAuthed, Extension(db): Extension, @@ -11172,6 +11243,7 @@ async fn load_workspace_authed( token_prefix: base_authed.token_prefix.clone(), read_only: base_authed.read_only, job_id: base_authed.job_id, + credential_expiry: base_authed.credential_expiry, }); }; @@ -11204,6 +11276,7 @@ async fn load_workspace_authed( token_prefix: base_authed.token_prefix.clone(), read_only: base_authed.read_only, job_id: base_authed.job_id, + credential_expiry: base_authed.credential_expiry, }) } diff --git a/backend/windmill-api-workspaces/src/workspaces_extra.rs b/backend/windmill-api-workspaces/src/workspaces_extra.rs index dfb6e49921..b2883e5f9a 100644 --- a/backend/windmill-api-workspaces/src/workspaces_extra.rs +++ b/backend/windmill-api-workspaces/src/workspaces_extra.rs @@ -113,7 +113,7 @@ pub(crate) async fn change_workspace_id( // Duplicate workspace settings (keep copy in old workspace for reference) info!("Duplicating workspace_settings table"); sqlx::query!( - "INSERT INTO workspace_settings (workspace_id, slack_team_id, slack_name, slack_command_script, slack_email, customer_id, plan, webhook, ai_config, large_file_storage, git_sync, default_app, default_scripts, deploy_ui, mute_critical_alerts, color, operator_settings, teams_command_script, teams_team_id, teams_team_name, git_app_installations, ducklake, dbt_warehouses, slack_oauth_client_id, slack_oauth_client_secret, datatable, teams_team_guid, auto_invite, error_handler, success_handler, public_app_execution_limit_per_minute, error_handler_fallback_to_instance_alerts, guest_access_enabled) SELECT $1, slack_team_id, slack_name, slack_command_script, slack_email, customer_id, plan, webhook, ai_config, large_file_storage, git_sync, default_app, default_scripts, deploy_ui, mute_critical_alerts, color, operator_settings, teams_command_script, teams_team_id, teams_team_name, git_app_installations, ducklake, dbt_warehouses, slack_oauth_client_id, slack_oauth_client_secret, datatable, teams_team_guid, auto_invite, error_handler, success_handler, public_app_execution_limit_per_minute, error_handler_fallback_to_instance_alerts, guest_access_enabled FROM workspace_settings WHERE workspace_id = $2", + "INSERT INTO workspace_settings (workspace_id, slack_team_id, slack_name, slack_command_script, slack_email, customer_id, plan, webhook, ai_config, large_file_storage, git_sync, default_app, default_scripts, deploy_ui, mute_critical_alerts, color, operator_settings, teams_command_script, teams_team_id, teams_team_name, git_app_installations, ducklake, dbt_warehouses, slack_oauth_client_id, slack_oauth_client_secret, datatable, teams_team_guid, auto_invite, error_handler, success_handler, public_app_execution_limit_per_minute, error_handler_fallback_to_instance_alerts, guest_access_enabled, guest_jwt_public_key, guest_jwt_jwks_url) SELECT $1, slack_team_id, slack_name, slack_command_script, slack_email, customer_id, plan, webhook, ai_config, large_file_storage, git_sync, default_app, default_scripts, deploy_ui, mute_critical_alerts, color, operator_settings, teams_command_script, teams_team_id, teams_team_name, git_app_installations, ducklake, dbt_warehouses, slack_oauth_client_id, slack_oauth_client_secret, datatable, teams_team_guid, auto_invite, error_handler, success_handler, public_app_execution_limit_per_minute, error_handler_fallback_to_instance_alerts, guest_access_enabled, guest_jwt_public_key, guest_jwt_jwks_url FROM workspace_settings WHERE workspace_id = $2", &rw.new_id, &old_id ) diff --git a/backend/windmill-api/openapi.yaml b/backend/windmill-api/openapi.yaml index a2fe3ee19b..574f1e0fda 100644 --- a/backend/windmill-api/openapi.yaml +++ b/backend/windmill-api/openapi.yaml @@ -3785,6 +3785,12 @@ paths: guest_access_enabled: type: boolean description: Whether this workspace admits guest sessions. An app's own `guest` execution mode is inert while this is false. + guest_jwt_public_key: + type: string + description: PEM public key a guest JWT (`jwt_guest_`) is verified against for this workspace. Mutually exclusive with `guest_jwt_jwks_url`. + guest_jwt_jwks_url: + type: string + description: JWKS URL a guest JWT (`jwt_guest_`) is verified against for this workspace. Mutually exclusive with `guest_jwt_public_key`. /w/{workspace}/workspaces/get_deploy_to: get: @@ -5808,6 +5814,43 @@ paths: schema: type: string + /w/{workspace}/workspaces/edit_guest_jwt_key: + post: + summary: set the key guest JWTs are verified against for this workspace + description: >- + A guest JWT (`jwt_guest_`) is minted by the embedding customer's own backend and + verified against this key: a PEM public key (RS/ES family, HS* refused) or a JWKS + URL, at most one. Both empty clears the workspace key; off cloud, verification then + falls back to the instance issuer (`JWT_EXT_JWKS_URL`) if one is set, else no guest + JWT is accepted (`guest_access_enabled` is the on/off switch). Workspace-admin gated. + The key is validated before it is stored. + operationId: editGuestJwtKey + tags: + - workspace + parameters: + - $ref: "#/components/parameters/WorkspaceId" + requestBody: + description: The guest JWT verification key + required: true + content: + application/json: + schema: + type: object + properties: + public_key: + type: string + description: A PEM public key (RS or ES family). + jwks_url: + type: string + description: A JWKS URL whose keys are fetched and refreshed. + responses: + "200": + description: status + content: + text/plain: + schema: + type: string + /w/{workspace}/workspaces/guest_usage: get: summary: the instance's standing against the guest allowance diff --git a/backend/windmill-api/src/apps.rs b/backend/windmill-api/src/apps.rs index ad8799da3b..a674e6c38a 100644 --- a/backend/windmill-api/src/apps.rs +++ b/backend/windmill-api/src/apps.rs @@ -1520,20 +1520,24 @@ async fn guest_derived_token_constraints( if !windmill_api_auth::scopes::has_guest_sentinel(authed.scopes.as_deref()) { return Ok(None); } - // The minter is known by prefix only; MIN over a (theoretical) prefix collision is - // the conservative side. - let parent: Option>> = sqlx::query_scalar( - "SELECT MIN(expiration) FROM token WHERE token_prefix = $1 AND email = $2 AND label = $3", - ) - .bind(authed.token_prefix.as_deref().unwrap_or("")) - .bind(&authed.email) - .bind(windmill_common::auth::GUEST_SESSION_LABEL) - .fetch_optional(db) - .await?; - let Some(parent_exp) = parent.flatten() else { - return Err(Error::NotAuthorized( - "guest session not found or has no expiry".to_string(), - )); + // A guest JWT carries its own expiry and has no token row to look up; a signed-in + // guest session is a row found by prefix (MIN is the conservative side of a + // theoretical prefix collision). Either way the derived token caps on it, never on + // a fresh interval. + let parent_exp = if let Some(exp) = authed.credential_expiry { + exp + } else { + let parent: Option>> = sqlx::query_scalar( + "SELECT MIN(expiration) FROM token WHERE token_prefix = $1 AND email = $2 AND label = $3", + ) + .bind(authed.token_prefix.as_deref().unwrap_or("")) + .bind(&authed.email) + .bind(windmill_common::auth::GUEST_SESSION_LABEL) + .fetch_optional(db) + .await?; + parent.flatten().ok_or_else(|| { + Error::NotAuthorized("guest session not found or has no expiry".to_string()) + })? }; Ok(Some(( windmill_common::auth::GUEST_SESSION_LABEL.to_string(), diff --git a/backend/windmill-api/src/jobs.rs b/backend/windmill-api/src/jobs.rs index 8b1dfe18d6..dbf33cdddc 100644 --- a/backend/windmill-api/src/jobs.rs +++ b/backend/windmill-api/src/jobs.rs @@ -11716,6 +11716,7 @@ mod approval_view_gate_tests { token_prefix: None, read_only: false, job_id: None, + credential_expiry: None, } } diff --git a/backend/windmill-api/src/lib.rs b/backend/windmill-api/src/lib.rs index 054af0f2fa..4ba60428c9 100644 --- a/backend/windmill-api/src/lib.rs +++ b/backend/windmill-api/src/lib.rs @@ -378,6 +378,7 @@ async fn inject_agent_authed( token_prefix: None, read_only: false, job_id: None, + credential_expiry: None, }, job_id: None, }); diff --git a/backend/windmill-api/src/mcp/utils.rs b/backend/windmill-api/src/mcp/utils.rs index 65c8069b85..ce5cb478cf 100644 --- a/backend/windmill-api/src/mcp/utils.rs +++ b/backend/windmill-api/src/mcp/utils.rs @@ -1421,6 +1421,7 @@ mod tests { token_prefix: None, read_only: false, job_id, + credential_expiry: None, } } diff --git a/backend/windmill-common/Cargo.toml b/backend/windmill-common/Cargo.toml index e2c3233183..dcf3465a94 100644 --- a/backend/windmill-common/Cargo.toml +++ b/backend/windmill-common/Cargo.toml @@ -109,6 +109,8 @@ pep440_rs.workspace = true systemstat.workspace = true size.workspace = true rsa = { workspace = true, optional = true } +spki = { workspace = true } +pkcs1 = { workspace = true } aes-gcm = { workspace = true, optional = true } semver.workspace = true diff --git a/backend/windmill-common/src/guest_jwt.rs b/backend/windmill-common/src/guest_jwt.rs new file mode 100644 index 0000000000..fe05bec6b6 --- /dev/null +++ b/backend/windmill-common/src/guest_jwt.rs @@ -0,0 +1,1008 @@ +//! The guest JWT contract: what a token minted by an embedding customer's own backend +//! must carry to open one guest-mode app, and how it is verified against the workspace's +//! configured key (or, off cloud, the instance issuer). Deliberately narrower than the external JWT scheme +//! (`jwt_ext_`), whose claims can assert admin, groups and folders: a guest key can +//! only ever mint guests, whatever the token says. + +use std::collections::HashMap; +use std::sync::Arc; +use std::time::{Duration, Instant}; + +use jsonwebtoken::{ + jwk::{AlgorithmParameters, Jwk, PublicKeyUse}, + Algorithm, DecodingKey, Validation, +}; +use quick_cache::sync::Cache; +use serde::Deserialize; + +use crate::error::{Error, Result}; +use crate::DB; + +/// A token is honoured at most this long past its issue, however far its `exp` lies: +/// a guest's expiry is its only revocation, and a long-lived token minted by mistake +/// would otherwise stay valid until it leaked. +pub const MAX_LIFETIME_SECS: u64 = 24 * 60 * 60; + +/// Bearer prefix. Stateless: no `token` row. Verified against the workspace's key (or, off +/// cloud, the instance issuer when the workspace set none) and resolved in the auth cache, +/// whose entry is short-lived (not the token's full `exp`) so a rotated key revokes within +/// minutes. See the arm in `windmill-api-auth`. +pub const BEARER_PREFIX: &str = "jwt_guest_"; + +const RSA_ALGORITHMS: [Algorithm; 6] = [ + Algorithm::RS256, + Algorithm::RS384, + Algorithm::RS512, + Algorithm::PS256, + Algorithm::PS384, + Algorithm::PS512, +]; +const EC_ALGORITHMS: [Algorithm; 2] = [Algorithm::ES256, Algorithm::ES384]; + +/// Every claim honoured. Extra claims are ignored; a missing one refuses the token. +/// `app_path` is mandatory: a token opens that one app, exactly as a signed-in guest +/// session does. +#[derive(Deserialize, Debug, Clone, PartialEq, Eq)] +pub struct GuestJwtClaims { + pub email: String, + pub workspace_id: String, + pub app_path: String, + pub exp: u64, + pub nbf: Option, + pub iat: Option, +} + +/// How a guest JWT is verified: the workspace's configured key (a PEM public key or a JWKS +/// URL), or, off cloud, the instance issuer (`JWT_EXT_JWKS_URL`) when the workspace set none — +/// see `key_source`. When neither is set the JWT is refused whatever it carries. +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum GuestJwtKeySource { + Pem(String), + JwksUrl(String), +} + +/// The instance-wide external JWT issuer (`JWT_EXT_JWKS_URL`, also used by `jwt_ext_`). Read +/// fresh rather than cached so it is testable and picks up config regardless of init order. +fn instance_ext_jwks_url() -> Option { + std::env::var("JWT_EXT_JWKS_URL") + .ok() + .filter(|s| !s.trim().is_empty()) +} + +pub async fn key_source(db: &DB, w_id: &str) -> Result> { + let row = sqlx::query!( + "SELECT guest_jwt_public_key, guest_jwt_jwks_url FROM workspace_settings WHERE workspace_id = $1", + w_id + ) + .fetch_optional(db) + .await + .map_err(|e| Error::internal_err(format!("reading guest JWT key of {w_id}: {e:#}")))?; + let per_workspace = row.and_then(|r| match (r.guest_jwt_public_key, r.guest_jwt_jwks_url) { + (Some(pem), _) => Some(GuestJwtKeySource::Pem(pem)), + (None, Some(url)) => Some(GuestJwtKeySource::JwksUrl(url)), + (None, None) => None, + }); + if per_workspace.is_some() { + return Ok(per_workspace); + } + // No workspace key: fall back to the instance issuer, so an operator running one issuer for + // both `jwt_ext_` and guests configures it once. Verifying it (and granting a *guest*) is + // done here in CE; granting a full login from it stays EE (`jwt_ext_`). Not on the shared + // cloud, where one instance issuer must not be trusted to mint guests in every tenant's + // workspace — there the per-workspace key is the only source. + if !*crate::worker::CLOUD_HOSTED { + if let Some(url) = instance_ext_jwks_url() { + return Ok(Some(GuestJwtKeySource::JwksUrl(url))); + } + } + Ok(None) +} + +/// Parse a PEM public key and the algorithms it may verify: RSA keys the RS/PS family, +/// EC keys the ES family. Anything symmetric has no PEM form, so HS* is unreachable +/// from here by construction; the JWKS path refuses it explicitly. +pub fn decoding_key_from_pem(pem: &str) -> Result<(DecodingKey, &'static [Algorithm])> { + use base64::{engine::general_purpose::STANDARD, Engine}; + use spki::der::Decode; + // The key is admin-set into an unbounded `TEXT` column and reparsed on every guest-JWT + // request; a well-formed key with an oversized modulus would pass the checks below. Refuse + // one larger than any real public key before decoding or storing it. Measure the untrimmed + // input: the endpoint stores what the admin sent, so whitespace padding counts too. + if pem.len() > MAX_GUEST_PEM_LEN { + return Err(Error::BadRequest(format!( + "guest key is longer than {MAX_GUEST_PEM_LEN} bytes" + ))); + } + let pem = pem.trim(); + // A verification key must be public. jsonwebtoken 8.3 keys the public/private distinction + // off the PEM label alone and never inspects the DER, so private material relabelled + // `PUBLIC KEY` would be stored and then served back through the settings response. Decode + // the body leniently, as jsonwebtoken does (tolerating any wrapping the strict RFC 7468 + // decoder would refuse), then require it to be a public-key structure: an SPKI (RSA or EC) + // or a PKCS#1 RSA public key. Private-key DER satisfies neither. + let der = STANDARD + .decode( + pem.lines() + .filter(|l| !l.trim_start().starts_with("-----")) + .flat_map(|l| l.split_whitespace()) + .collect::(), + ) + .map_err(|e| Error::BadRequest(format!("guest key is not valid PEM: {e}")))?; + let is_public = spki::SubjectPublicKeyInfoRef::from_der(&der).is_ok() + || pkcs1::RsaPublicKey::from_der(&der).is_ok(); + if !is_public { + return Err(Error::BadRequest( + "expected an RSA or EC public key in PEM form (-----BEGIN PUBLIC KEY-----)".to_string(), + )); + } + if let Ok(key) = DecodingKey::from_rsa_pem(pem.as_bytes()) { + return Ok((key, &RSA_ALGORITHMS)); + } + if let Ok(key) = DecodingKey::from_ec_pem(pem.as_bytes()) { + return Ok((key, &EC_ALGORITHMS)); + } + Err(Error::BadRequest( + "not an RSA or EC public key in PEM form (expected -----BEGIN PUBLIC KEY-----)".to_string(), + )) +} + +/// The algorithms a JWKS key may verify, or `None` if the key is unusable here: a +/// symmetric key (HS*, a shared secret the embedder would then have to hold), an +/// unsupported family, or a key not marked for signatures. A key that names its `alg` +/// pins that one; an RSA key that omits it accepts the whole RSA family, and an EC key +/// the algorithm its curve implies, mirroring how a PEM key is accepted. +pub fn jwk_algorithms(jwk: &Jwk) -> Option> { + if jwk.common.public_key_use.is_some() + && jwk.common.public_key_use != Some(PublicKeyUse::Signature) + { + return None; + } + // A key that lists its operations must allow verifying signatures; otherwise it is + // published for something else (encryption, key wrapping) and is not ours to use. + if jwk + .common + .key_operations + .as_ref() + .is_some_and(|ops| !ops.contains(&jsonwebtoken::jwk::KeyOperations::Verify)) + { + return None; + } + match (&jwk.algorithm, jwk.common.algorithm) { + (AlgorithmParameters::RSA(_), Some(alg)) if RSA_ALGORITHMS.contains(&alg) => { + Some(vec![alg]) + } + (AlgorithmParameters::RSA(_), None) => Some(RSA_ALGORITHMS.to_vec()), + (AlgorithmParameters::EllipticCurve(_), Some(alg)) if EC_ALGORITHMS.contains(&alg) => { + Some(vec![alg]) + } + (AlgorithmParameters::EllipticCurve(p), None) => match p.curve { + jsonwebtoken::jwk::EllipticCurve::P256 => Some(vec![Algorithm::ES256]), + jsonwebtoken::jwk::EllipticCurve::P384 => Some(vec![Algorithm::ES384]), + _ => None, + }, + _ => None, + } +} + +/// Verify `token` against `key`, honouring only the accepted `algorithms`, and check +/// every claim rule that needs no database: signature, `exp` (mandatory), `nbf` and +/// `iat` when present, the lifetime cap, that the token names `w_id`, that `email` is a +/// valid address bounded to 254 bytes, and that `app_path` carries no scope metacharacter. +pub fn verify( + token: &str, + key: &DecodingKey, + algorithms: &[Algorithm], + w_id: &str, +) -> Result { + let mut validation = Validation::new(algorithms[0]); + validation.algorithms = algorithms.to_vec(); + validation.validate_nbf = true; + let claims = jsonwebtoken::decode::(token, key, &validation) + .map_err(|e| Error::NotAuthorized(format!("guest JWT refused: {e}")))? + .claims; + let now = jsonwebtoken::get_current_timestamp(); + if claims.exp > now + MAX_LIFETIME_SECS { + return Err(Error::NotAuthorized(format!( + "guest JWT refused: exp is more than {MAX_LIFETIME_SECS} seconds ahead" + ))); + } + if let Some(iat) = claims.iat { + if iat > now + validation.leeway { + return Err(Error::NotAuthorized( + "guest JWT refused: iat is in the future".to_string(), + )); + } + if claims.exp.saturating_sub(iat) > MAX_LIFETIME_SECS { + return Err(Error::NotAuthorized(format!( + "guest JWT refused: lifetime exceeds {MAX_LIFETIME_SECS} seconds" + ))); + } + } + if claims.workspace_id != w_id { + return Err(Error::NotAuthorized( + "guest JWT refused: workspace_id does not match the workspace".to_string(), + )); + } + // The email becomes the guest's username; require the address shape the `usr` table + // accepts (`VALID_EMAIL`), so it always carries an `@` and `username_to_permissioned_as` + // can only ever read it as its own principal, never a `u/` or `g/`. + // Bound it to fit the `guest_activity.email` column: a longer one fails that insert + // while the guest is admitted uncounted. + if !crate::users::VALID_EMAIL.is_match(&claims.email) || claims.email.len() > 254 { + return Err(Error::NotAuthorized( + "guest JWT refused: email is not a valid, bounded email address".to_string(), + )); + } + // The app path is spliced into `apps:read:` and `apps:run:` scopes, whose + // grammar reserves `:`, `,`, `*` and a leading `/`; refuse those, the same guard + // `guest_session_scopes` applies at the mint. App paths may carry spaces and `@`. + if !crate::auth::is_scope_literal_path(&claims.app_path) { + return Err(Error::NotAuthorized( + "guest JWT refused: app_path is empty or cannot be scoped (`:`, `,`, `*` are \ + reserved and a leading `/` never matches a route)" + .to_string(), + )); + } + Ok(claims) +} + +struct JwksEntry { + keys: Arc>, + /// When this entry stops being served and the next request refetches. A good fetch + /// is served for `JWKS_TTL`, a failed one for `JWKS_NEGATIVE_TTL` (serving the last + /// good keys if there are any), so an unreachable issuer cannot be turned into one + /// outbound fetch per request by unauthenticated traffic. + expires_at: Instant, + /// When these keys were last fetched successfully. Stale keys are served only within + /// `JWKS_MAX_STALE` of this, and a stale re-serve preserves it, so a revoked `kid` or an + /// unreachable issuer stops minting new guest JWTs after a bounded window, not forever. + fetched_at: Instant, +} + +lazy_static::lazy_static! { + static ref JWKS_CACHE: Cache> = Cache::new(200); + /// Per-URL fetch lock: only one refresh per URL is in flight at a time, so a cold + /// or stale entry under a burst triggers one fetch, not one per request. This is a + /// plain map, not a capacity-bounded `Cache`: a `Cache` could evict a lock whose fetch + /// is still running, and the next request for that URL would then mint a fresh lock and + /// start a duplicate fetch, so cycling past 200 cold URLs could defeat single-flight and + /// storm the issuers. `JwksFetchLock` drops each entry once its last holder is gone, so + /// the map only ever holds the fetches in flight (bounded by concurrent distinct URLs). + static ref JWKS_FETCH_LOCKS: std::sync::Mutex>>> = + std::sync::Mutex::new(HashMap::new()); +} + +/// How long a good key set is served before a refresh; also the lag before a +/// rotated-in `kid` is picked up. The cadence of the instance-level external JWKS. +const JWKS_TTL: Duration = Duration::from_secs(15 * 60); +/// How long a failed fetch is remembered before retrying, so an unreachable issuer is +/// hit at most once per this interval however much guest-JWT traffic arrives. +const JWKS_NEGATIVE_TTL: Duration = Duration::from_secs(30); +/// The absolute age past which cached keys are no longer served, even while revalidating: +/// once an issuer has been unreachable (or has revoked a `kid`) for this long, its old keys +/// stop authenticating and the request fails closed rather than trusting them indefinitely. +const JWKS_MAX_STALE: Duration = Duration::from_secs(60 * 60); +/// A JWKS body larger than this is refused rather than buffered: the URL is admin-set +/// but the server it names may be attacker-controlled, and a real key set is a few KB. +const JWKS_MAX_BYTES: usize = 1 << 20; + +/// A cache entry retains only the usable signing keys, but nothing else bounds their combined +/// size: the response cap is 1 MiB and `from_jwk` decodes `n`/`e`/`x`/`y` without a length +/// limit, so one entry could retain ~1 MiB (a few hundred MB across the 200-entry LRU). Cap the +/// retained material instead; a real set is a few KB, so this is invisible to a legitimate one. +const JWKS_MAX_RETAINED_BYTES: usize = 64 * 1024; + +/// The retained-bytes cap counts key material; this caps the number of keys so the per-key +/// fixed cost (each `Jwk` and its map slot) is bounded too, not just their string content. +const JWKS_MAX_KEYS: usize = 50; + +/// Both JWKS caches key on the admin-supplied URL string. The column is unbounded `TEXT`, so +/// without this a workspace admin could grow the caches by the URL bytes alone. Enforced in +/// `fetch_jwks`, which `edit_guest_jwt_key` validates through, so an overlong URL is never +/// stored; the cache only ever sees a URL that was stored, hence a bounded one. +const MAX_JWKS_URL_LEN: usize = 2048; + +/// A guest verification key is admin-set into an unbounded `TEXT` column and reparsed on every +/// guest-JWT request. A real public key PEM is under a few KB (RSA-16384 SPKI is ~2.8 KB), so +/// this bounds the stored and reparsed bytes without refusing any real key. +const MAX_GUEST_PEM_LEN: usize = 8 * 1024; + +/// A guest JWT is refused past this before any signature work or caching: the auth cache keys +/// on the bearer, so an oversized token (unauthenticated at this point) would otherwise be +/// decoded and, if it verified, cached at its full size. A real JWT is well under this. +pub const MAX_GUEST_JWT_LEN: usize = 8 * 1024; + +/// Fetch a JWKS, keeping only the keys usable here. A workspace-admin URL is validated +/// against private ranges and the connect pinned to the validated addresses; redirects are +/// not followed for the same reason. The instance issuer (`JWT_EXT_JWKS_URL`) is exempt from +/// those restrictions — it is operator-configured and trusted (it also backs `jwt_ext_`), so a +/// self-hosted internal (http/private) issuer that works for `jwt_ext_` works for guests too. +/// The body is read with a cap so a hostile endpoint cannot exhaust memory. +pub async fn fetch_jwks(url: &str) -> Result> { + use futures::StreamExt; + if url.len() > MAX_JWKS_URL_LEN { + return Err(Error::BadRequest(format!( + "JWKS URL is longer than {MAX_JWKS_URL_LEN} bytes" + ))); + } + let resp = if instance_ext_jwks_url().as_deref() == Some(url) { + // Operator-trusted issuer: fetch it with the same permissive client `jwt_ext_` uses + // (follows redirects, honors ACCEPT_INVALID_CERTS), so an issuer that works for + // `jwt_ext_` through a redirect or an approved self-signed cert works for guests too. + crate::utils::HTTP_CLIENT_PERMISSIVE.get(url).send().await + } else { + // Workspace-admin URL: validate (https + private ranges), pin the connect to the + // validated addresses, and do not follow redirects — all against SSRF. + let client = crate::ssrf::validate_guest_jwks_url(url) + .await + .map_err(|e| Error::BadRequest(format!("JWKS URL is not allowed: {e}")))? + .apply_dns_pinning(crate::utils::configure_client(reqwest::ClientBuilder::new())) + .user_agent("windmill/beta") + .redirect(reqwest::redirect::Policy::none()) + .connect_timeout(Duration::from_secs(5)) + .timeout(Duration::from_secs(10)) + .build() + .map_err(|e| Error::internal_err(format!("building JWKS client: {e}")))?; + client.get(url).send().await + } + .and_then(|r| r.error_for_status()) + .map_err(|e| Error::BadRequest(format!("could not fetch JWKS: {e}")))?; + let mut stream = resp.bytes_stream(); + let mut body: Vec = Vec::new(); + while let Some(chunk) = stream.next().await { + let chunk = chunk.map_err(|e| Error::BadRequest(format!("reading JWKS: {e}")))?; + if body.len() + chunk.len() > JWKS_MAX_BYTES { + return Err(Error::BadRequest(format!( + "JWKS is larger than {JWKS_MAX_BYTES} bytes" + ))); + } + body.extend_from_slice(&chunk); + } + parse_jwks_keys(&body) +} + +/// The usable signing keys in a JWKS body, by `kid`. Each key is parsed on its own and +/// one that does not model as a JWT key is skipped, not fatal: a set may legitimately +/// carry an encryption key (say `alg: "RSA-OAEP"`, which is not in jsonwebtoken's signing +/// `Algorithm` enum and would fail whole-set deserialization) beside its signing keys. +/// +/// A key is kept only if its material actually decodes (`DecodingKey::from_jwk`): jsonwebtoken +/// carries `n`/`e`/`x`/`y` as strings and defers decoding to auth time, so without this a JWKS +/// whose only key is malformed would be accepted at save time and fail every token later. +fn parse_jwks_keys(body: &[u8]) -> Result> { + let set: serde_json::Value = serde_json::from_slice(body) + .map_err(|e| Error::BadRequest(format!("JWKS is not JSON: {e}")))?; + let entries = set + .get("keys") + .and_then(|k| k.as_array()) + .ok_or_else(|| Error::BadRequest("JWKS has no `keys` array".to_string()))?; + let keys: HashMap = entries + .iter() + .filter_map(|entry| serde_json::from_value::(entry.clone()).ok()) + .filter(|jwk| jwk_algorithms(jwk).is_some()) + .filter(|jwk| DecodingKey::from_jwk(jwk).is_ok()) + .filter_map(|jwk| jwk.common.key_id.clone().map(|kid| (kid, jwk))) + .collect(); + if keys.is_empty() { + return Err(Error::BadRequest( + "JWKS holds no RSA or EC signing key with a kid".to_string(), + )); + } + // Bound the usable keys two ways, both measured after filtering so a large mixed-use set + // (many encryption keys, few signing) is not refused for its size: their count (the per-key + // fixed cost) and their combined material bytes. + if keys.len() > JWKS_MAX_KEYS { + return Err(Error::BadRequest(format!( + "JWKS holds more than {JWKS_MAX_KEYS} usable signing keys" + ))); + } + let retained: usize = keys + .values() + .filter_map(|jwk| serde_json::to_vec(jwk).ok().map(|v| v.len())) + .sum(); + if retained > JWKS_MAX_RETAINED_BYTES { + return Err(Error::BadRequest(format!( + "JWKS signing keys retain more than {JWKS_MAX_RETAINED_BYTES} bytes" + ))); + } + Ok(keys) +} + +/// A cached entry that holds no keys is a remembered failure; serving it would report +/// an unreachable issuer as an unknown `kid`. Map it to an issuer-unreachable error. +fn servable(entry: Arc) -> Result> { + if entry.keys.is_empty() { + Err(Error::NotAuthorized( + "guest JWT refused: the JWKS issuer is unreachable".to_string(), + )) + } else { + Ok(entry) + } +} + +/// A held single-flight lock for one JWKS URL. Dropping it removes the URL from +/// `JWKS_FETCH_LOCKS` once no other holder remains, so the registry never keeps a lock past +/// its fetch and stays bounded by the number of fetches in flight, not by URLs ever seen. +struct JwksFetchLock { + url: String, + lock: Arc>, +} + +impl JwksFetchLock { + /// The lock for `url`, created on first use. Callers sharing a URL get the same `Arc`, + /// so one holds the inner mutex and fetches while the rest wait on it. + fn acquire(url: &str) -> Self { + let mut map = JWKS_FETCH_LOCKS.lock().unwrap(); + let lock = map + .entry(url.to_string()) + .or_insert_with(|| Arc::new(tokio::sync::Mutex::new(()))) + .clone(); + JwksFetchLock { url: url.to_string(), lock } + } +} + +impl Drop for JwksFetchLock { + fn drop(&mut self) { + let mut map = JWKS_FETCH_LOCKS.lock().unwrap(); + // Clones are only taken while holding this same map lock, so the count is stable + // here: two Arcs (the map's and this one's) means we are the last holder and the + // entry can go; more means another request still needs it and will remove it in turn. + if map + .get(&self.url) + .is_some_and(|lock| Arc::strong_count(lock) <= 2) + { + map.remove(&self.url); + } + } +} + +/// Refresh a URL's JWKS off the request path, under the single-flight lock. A held +/// lock means a refresh is already running, so this is a no-op. A failed refresh +/// leaves the served stale keys in place rather than dropping them. +fn spawn_jwks_refresh(url: String) { + tokio::spawn(async move { + let fetch_lock = JwksFetchLock::acquire(&url); + let Ok(_guard) = fetch_lock.lock.try_lock() else { + return; + }; + match fetch_jwks(&url).await { + Ok(keys) => { + let now = Instant::now(); + JWKS_CACHE.insert( + url, + Arc::new(JwksEntry { + keys: Arc::new(keys), + expires_at: now + JWKS_TTL, + fetched_at: now, + }), + ); + } + Err(e) => tracing::warn!("guest JWKS background refresh failed for {url}: {e:#}"), + } + }); +} + +/// The workspace's JWKS. A fresh entry is served directly; a stale-but-good one is +/// served while a refresh runs off the request path (`spawn_jwks_refresh`), so a slow +/// issuer never stalls a request. Only a cold or negative entry blocks, under a per-URL +/// lock so a burst triggers one fetch; a failed fetch there caches a short-lived empty +/// entry that reads as "issuer unreachable", so an unreachable issuer is hit at most +/// once per `JWKS_NEGATIVE_TTL`. Fetches follow a schedule, never a per-request, +/// attacker-chosen `kid`. +async fn cached_jwks(url: &str) -> Result> { + if let Some(entry) = JWKS_CACHE.get(url) { + // Keys past JWKS_MAX_STALE are never served, even while revalidating: a stale re-serve + // bumps expires_at but keeps fetched_at, so a persistently failing refresh would + // otherwise serve revoked keys forever. Too-old keys fall through to the blocking + // refresh, which fails closed if the issuer is still down. + if entry.fetched_at.elapsed() < JWKS_MAX_STALE { + if entry.expires_at > Instant::now() { + return servable(entry); + } + // Stale but still holds good keys: serve them now and refresh off the request + // path, so a slow or hanging issuer adds no latency. Bump the entry first so the + // refresh window does not spawn a task per request. A negative (empty) entry + // falls through to the blocking refresh below. + if !entry.keys.is_empty() { + let served = Arc::new(JwksEntry { + keys: entry.keys.clone(), + expires_at: Instant::now() + JWKS_NEGATIVE_TTL, + fetched_at: entry.fetched_at, + }); + JWKS_CACHE.insert(url.to_string(), served.clone()); + spawn_jwks_refresh(url.to_string()); + return Ok(served); + } + } + } + // Cold or negative entry, nothing good to serve: block on a single-flight refresh. + // `JwksFetchLock::acquire` hands cold requests the same lock, so they share one fetch. + let fetch_lock = JwksFetchLock::acquire(url); + let _guard = fetch_lock.lock.lock().await; + // Another task may have refreshed while we waited for the lock (honour the age limit too, + // so a concurrent stale re-serve of too-old keys is not mistaken for a fresh entry). + if let Some(entry) = JWKS_CACHE.get(url) { + if entry.fetched_at.elapsed() < JWKS_MAX_STALE && entry.expires_at > Instant::now() { + return servable(entry); + } + } + match fetch_jwks(url).await { + Ok(keys) => { + let now = Instant::now(); + let entry = Arc::new(JwksEntry { + keys: Arc::new(keys), + expires_at: now + JWKS_TTL, + fetched_at: now, + }); + JWKS_CACHE.insert(url.to_string(), entry.clone()); + Ok(entry) + } + Err(e) => { + // Reached with no servable keys: either nothing cached, or keys too old to trust. + // Cache a short negative entry so the next requests do not each refetch, and + // surface the error, so an issuer that revoked a key or went down fails closed. + JWKS_CACHE.insert( + url.to_string(), + Arc::new(JwksEntry { + keys: Arc::new(HashMap::new()), + expires_at: Instant::now() + JWKS_NEGATIVE_TTL, + fetched_at: Instant::now(), + }), + ); + Err(e) + } + } +} + +/// The key a token's header selects from the workspace's JWKS, by `kid`, and the +/// algorithms it may verify. An unknown `kid` is refused against the cached set rather +/// than triggering a fetch, so varying `kid` cannot drive outbound requests; a +/// genuinely rotated-in key is picked up within `JWKS_TTL`. +pub async fn jwks_key_for(url: &str, token: &str) -> Result<(DecodingKey, Vec)> { + let header = jsonwebtoken::decode_header(token) + .map_err(|e| Error::NotAuthorized(format!("guest JWT refused: {e}")))?; + let kid = header.kid.ok_or_else(|| { + Error::NotAuthorized("guest JWT refused: no kid in the header".to_string()) + })?; + let entry = cached_jwks(url).await?; + let jwk = entry.keys.get(&kid).ok_or_else(|| { + Error::NotAuthorized(format!("guest JWT refused: kid {kid} is not in the JWKS")) + })?; + let algs = jwk_algorithms(jwk).ok_or_else(|| { + Error::NotAuthorized(format!("guest JWT refused: kid {kid} is not a signing key")) + })?; + let key = DecodingKey::from_jwk(jwk) + .map_err(|e| Error::internal_err(format!("unusable JWK {kid}: {e}")))?; + Ok((key, algs)) +} + +/// Verify `token` for `w_id` against whatever key the workspace configured. A PEM key +/// ignores `kid`; a JWKS selects by it. +pub async fn verify_for_workspace(db: &DB, w_id: &str, token: &str) -> Result { + if token.len() > MAX_GUEST_JWT_LEN { + return Err(Error::NotAuthorized(format!( + "guest JWT refused: token is longer than {MAX_GUEST_JWT_LEN} bytes" + ))); + } + let Some(source) = key_source(db, w_id).await? else { + return Err(Error::NotAuthorized(format!( + "guest JWT refused: workspace {w_id} has no guest JWT key" + ))); + }; + match source { + GuestJwtKeySource::Pem(pem) => { + let (key, algorithms) = decoding_key_from_pem(&pem)?; + verify(token, &key, algorithms, w_id) + } + GuestJwtKeySource::JwksUrl(url) => { + let (key, algorithms) = jwks_key_for(&url, token).await?; + verify(token, &key, &algorithms, w_id) + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + + /// Serializes the tests that mutate the process-wide `ALLOW_PRIVATE_GUEST_JWKS_URLS`, so a + /// concurrent run cannot clear it out from under another (mirrors `ssrf.rs`'s test lock). + static TEST_ENV_LOCK: tokio::sync::Mutex<()> = tokio::sync::Mutex::const_new(()); + + fn jwk(v: serde_json::Value) -> Jwk { + serde_json::from_value(v).unwrap() + } + + #[test] + fn rsa_key_with_alg_pins_it() { + let k = jwk(serde_json::json!({"kty":"RSA","alg":"RS384","n":"aa","e":"AQAB"})); + assert_eq!(jwk_algorithms(&k), Some(vec![Algorithm::RS384])); + } + + #[test] + fn rsa_key_without_alg_takes_the_whole_family() { + // The bug this pins: an alg-less RSA key must not be forced to RS256, which + // would reject valid RS384/512 or PS* tokens. + let k = jwk(serde_json::json!({"kty":"RSA","n":"aa","e":"AQAB"})); + assert_eq!(jwk_algorithms(&k), Some(RSA_ALGORITHMS.to_vec())); + } + + #[test] + fn ec_key_takes_its_curve_algorithm() { + let k = jwk(serde_json::json!({"kty":"EC","crv":"P-256","x":"aa","y":"bb"})); + assert_eq!(jwk_algorithms(&k), Some(vec![Algorithm::ES256])); + } + + #[test] + fn symmetric_key_is_refused() { + let k = jwk(serde_json::json!({"kty":"oct","k":"c2VjcmV0"})); + assert_eq!(jwk_algorithms(&k), None); + } + + #[test] + fn a_key_marked_for_encryption_is_refused() { + let k = jwk(serde_json::json!({"kty":"RSA","use":"enc","n":"aa","e":"AQAB"})); + assert_eq!(jwk_algorithms(&k), None); + } + + #[test] + fn a_mixed_use_jwks_keeps_only_the_signing_keys() { + // An encryption key (RSA-OAEP is not in jsonwebtoken's signing Algorithm enum) + // beside a signing key must not fail the whole set. The signing key carries real + // coordinates so it survives the material check parse_jwks_keys now applies. + let body = serde_json::json!({ + "keys": [ + {"kty":"RSA","alg":"RSA-OAEP","kid":"enc","use":"enc","n":"aa","e":"AQAB"}, + {"kty":"EC","crv":"P-256","kid":"sig","x":PUB1_X,"y":PUB1_Y} + ] + }) + .to_string(); + let keys = parse_jwks_keys(body.as_bytes()).expect("the signing key survives"); + assert!(keys.contains_key("sig")); + assert!(!keys.contains_key("enc")); + } + + #[test] + fn a_jwks_with_only_malformed_key_material_is_refused() { + // Metadata (kty/alg/use) is fine but `n` is not valid base64url, so the key is + // unusable. Since it is the only key, configuring this JWKS must fail at save time + // rather than persist a URL whose tokens all fail later. + let body = serde_json::json!({ + "keys": [{"kty":"RSA","kid":"k1","n":"not base64url!!","e":"AQAB"}] + }) + .to_string(); + assert!(parse_jwks_keys(body.as_bytes()).is_err()); + } + + #[test] + fn too_many_usable_keys_is_refused() { + // All keys are usable (real coordinates), so this trips the count cap, not the + // empty-set path. Small material, so it is the count that refuses them, not the bytes. + let keys: Vec<_> = (0..=JWKS_MAX_KEYS) + .map(|i| { + serde_json::json!({"kty":"EC","crv":"P-256","kid":format!("k{i}"),"x":PUB1_X,"y":PUB1_Y}) + }) + .collect(); + let body = serde_json::json!({ "keys": keys }).to_string(); + let err = parse_jwks_keys(body.as_bytes()).unwrap_err().to_string(); + assert!(err.contains("usable signing keys"), "{err}"); + } + + #[test] + fn keys_retaining_too_many_bytes_is_refused() { + // Under the key count cap, but the material of these usable RSA keys exceeds the byte + // cap. `from_jwk` decodes any-length `n`, so this is reachable without the count cap. + let big_n = "A".repeat(2000); + let keys: Vec<_> = (0..40) + .map(|i| serde_json::json!({"kty":"RSA","kid":format!("k{i}"),"n":big_n,"e":"AQAB"})) + .collect(); + let body = serde_json::json!({ "keys": keys }).to_string(); + let err = parse_jwks_keys(body.as_bytes()).unwrap_err().to_string(); + assert!(err.contains("retain more than"), "{err}"); + } + + #[tokio::test] + async fn an_overlong_jwks_url_is_refused() { + // The cache keys on the URL string, so an unbounded URL is refused before it is cached. + // Assert the length error specifically: a bogus URL would fail the fetch regardless. + let url = format!( + "https://issuer.example.com/{}", + "a".repeat(MAX_JWKS_URL_LEN) + ); + let err = fetch_jwks(&url).await.unwrap_err().to_string(); + assert!(err.contains("longer than"), "{err}"); + } + + #[test] + fn an_oversized_pem_is_refused() { + // A well-formed key body padded past the cap: refused for its length before decoding, + // so an oversized-but-valid key cannot be stored and reparsed on every request. + let big = format!( + "-----BEGIN PUBLIC KEY-----\n{}\n-----END PUBLIC KEY-----\n", + "A".repeat(MAX_GUEST_PEM_LEN) + ); + let err = decoding_key_from_pem(&big).err().unwrap().to_string(); + assert!(err.contains("longer than"), "{err}"); + // Whitespace padding must count: the endpoint stores the untrimmed value, so the cap is + // measured before trimming rather than on the small trimmed key it would otherwise see. + let padded = format!("{}{RSA_PUBLIC}", " ".repeat(MAX_GUEST_PEM_LEN)); + let err = decoding_key_from_pem(&padded).err().unwrap().to_string(); + assert!(err.contains("longer than"), "{err}"); + } + + #[test] + fn key_ops_without_verify_is_refused() { + let enc = jwk(serde_json::json!({"kty":"RSA","key_ops":["encrypt"],"n":"aa","e":"AQAB"})); + assert_eq!(jwk_algorithms(&enc), None); + let ver = jwk(serde_json::json!({"kty":"RSA","key_ops":["verify"],"n":"aa","e":"AQAB"})); + assert_eq!(jwk_algorithms(&ver), Some(RSA_ALGORITHMS.to_vec())); + } + + // PUB1's coordinates and its matching PKCS8 private key, for the JWKS-derived + // verification test. + const PUB1_X: &str = "zAfqyCh34iYOCW0vg4ejq_zzJlzLSZScjnVyPjLGTao"; + const PUB1_Y: &str = "RMKOIHOv8tWLnXf7-eMCodDnX038wCjD1sf9jVsf7oI"; + const PRIV1: &str = "-----BEGIN PRIVATE KEY-----\nMIGHAgEAMBMGByqGSM49AgEGCCqGSM49AwEHBG0wawIBAQQgu27S2DbSwUh8BmQb\n/i4/VhNdoXV7PJekhnoceMULYLihRANCAATMB+rIKHfiJg4JbS+Dh6Or/PMmXMtJ\nlJyOdXI+MsZNqkTCjiBzr/LVi513+/njAqHQ519N/MAow9bH/Y1bH+6C\n-----END PRIVATE KEY-----\n"; + + #[test] + fn a_jwks_key_verifies_a_real_token() { + // The one path the PEM tests do not cover: a key rebuilt from a JWK verifies a + // token signed by its private half, and the algorithms come from the JWK. + let jwk = jwk(serde_json::json!({ + "kty": "EC", "crv": "P-256", "kid": "k1", "x": PUB1_X, "y": PUB1_Y + })); + let algs = jwk_algorithms(&jwk).expect("EC signing key"); + assert_eq!(algs, vec![Algorithm::ES256]); + let key = jsonwebtoken::DecodingKey::from_jwk(&jwk).expect("usable JWK"); + let payload = serde_json::json!({ + "email": "g@example.com", + "workspace_id": "ws", + "app_path": "u/a/app", + "exp": jsonwebtoken::get_current_timestamp() + 600, + }); + let token = jsonwebtoken::encode( + &jsonwebtoken::Header::new(Algorithm::ES256), + &payload, + &jsonwebtoken::EncodingKey::from_ec_pem(PRIV1.as_bytes()).unwrap(), + ) + .unwrap(); + let out = verify(&token, &key, &algs, "ws").expect("verifies"); + assert_eq!(out.email, "g@example.com"); + // The workspace pin is part of verify. + assert!(verify(&token, &key, &algs, "other-ws").is_err()); + } + + #[test] + fn a_non_key_pem_is_rejected() { + assert!(decoding_key_from_pem( + "-----BEGIN CERTIFICATE-----\nnope\n-----END CERTIFICATE-----" + ) + .is_err()); + } + + // A real RSA public key (SPKI). Its private counterpart is RSA_PKCS1_PRIVATE below. + const RSA_PUBLIC: &str = "-----BEGIN PUBLIC KEY-----\n\ +MIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEAx3J0fQcHp2ZlMI4rCVsY\n\ +tirATZPWyPD7exoYWPInhV5xjbY2Fe8IVFaZszQcQbCXZjBtFp2fj0tBTow8BeOy\n\ +X9LJPyKeho/j68FycuDVg7JCzG0TWtsnh/V23WkrlKIfmMqS3+YUyFavROTcAN1T\n\ +5BcFHLAi/4Q2qy0JjXBdZ8avelzZrQ/T67/Kcsoct/pvnEDT2YRsSbA7VMWaxWh8\n\ +MYJ7GNV/10YT2c5CBJGSLbyRSVWk2IwfnM9Cl9n/5NE6TkSetYQ2xlqKTONp5W43\n\ +UzW1NAeqKCxPQfN/ADjwW18nk2o7xj1kMF4rBlhsTm9ClE71nwi5NxsvMOdVxytZ\n\ +ZQIDAQAB\n\ +-----END PUBLIC KEY-----\n"; + + // A complete, valid PKCS#1 RSA *private* key (the counterpart of RSA_PUBLIC) with its + // armor relabelled `RSA PUBLIC KEY`. jsonwebtoken's from_rsa_pem accepts it under that + // label; the structural check refuses it (a 9-field RSAPrivateKey is neither an SPKI nor + // a 2-field RsaPublicKey). Complete on purpose: malformed DER would fail for the wrong + // reason and let a real bypass through unnoticed. + const RSA_PKCS1_PRIVATE_AS_PUBLIC: &str = "-----BEGIN RSA PUBLIC KEY-----\n\ +MIIEogIBAAKCAQEAx3J0fQcHp2ZlMI4rCVsYtirATZPWyPD7exoYWPInhV5xjbY2\n\ +Fe8IVFaZszQcQbCXZjBtFp2fj0tBTow8BeOyX9LJPyKeho/j68FycuDVg7JCzG0T\n\ +Wtsnh/V23WkrlKIfmMqS3+YUyFavROTcAN1T5BcFHLAi/4Q2qy0JjXBdZ8avelzZ\n\ +rQ/T67/Kcsoct/pvnEDT2YRsSbA7VMWaxWh8MYJ7GNV/10YT2c5CBJGSLbyRSVWk\n\ +2IwfnM9Cl9n/5NE6TkSetYQ2xlqKTONp5W43UzW1NAeqKCxPQfN/ADjwW18nk2o7\n\ +xj1kMF4rBlhsTm9ClE71nwi5NxsvMOdVxytZZQIDAQABAoIBAD+IbaQQM7d3Dj/X\n\ +4cyyqJ4K40QzFmXfIfTWXLAkv0MkUR7XzsXQ5YHcLkzgCipAwxGp1m4wWs4OJmkL\n\ +kek8XatZnYLPl9j8iBmm/zqp9Unk5JNzIYm9KwwLvMgOAvRvaopE6WGKTM9+kYls\n\ +L8rUti7/yECZuSRU7Qc9KwBTrWVrXK+RBtBqZYQXb92BFxq0N3Qp+utLNdFcO5sW\n\ +7d8gKp3ipQt5z9ZAB2pYMw7ZTzonF4C7HdyrbYXztvYrxuw1imMkQ9iFFhdn3/76\n\ +qFR7XwaFrld8DECGaH/652kV6zaSQijbBTeXF4zsgwXY4BHMVmZKaXH5unw8Gbmo\n\ +WCoLbLcCgYEA4vTo5kCiKXnlBp3W1Zpg4cml6Wzo0UDldF4kkuXQxhdQA38c0ise\n\ +Cocf6qyAqz1L2TxQ/9WCL2oIP1AY9XqnQ0cJtYIosGWORz4tPe67M8inB/GBotFI\n\ +pmQNVSIjqbgKVi0x+UzmFjitINFPf461lDdJTwhsv9TQRbHrXErvON8CgYEA4PhV\n\ +GYMJu46tqFVtD/koWAQRLmeaZXhxP5lMSmQjdYCa3ys5lccvTlzoF9K8immMQkIx\n\ +gyOazmEtFnK4IXmEY1wg2NIHuJM7/maoM2rozbjXBxsYM7Xw2QX7BHXqG6Ia/Bij\n\ +ZaRJdumCVRJv7OshQTGuqDIzd3l5WEqg11XYgjsCgYAP3v6Wc3ijm+GXN9x5LYWO\n\ +5JIUo8gYMgiZvaejGi0iXSj8RZxXWiqMo+xodc29q9itBVnIuj6TYD/ZZZmJOR2P\n\ +R9128vYzd7aeZsu1JAe1VFfR52KgZzBEaoTAKlYCHVujsR9ohqckcKwyulBr5Cfw\n\ +iHk47KbmN1SlOw7xclAOUwKBgAuxHEsdIk5bFe9fsTFZU51vaK0uuTl4zvntL6fW\n\ +GHms21+p0W5VUcIS1gUW8LGI1r9CzWvxV8RODJfUEnm65QR870AVek0/aajJEQjL\n\ +D5pRdutpnxJg7El7JBaRQj95Z0mexi8sIJ1LeXiOYr6/YZUPzfHz2fTlnUbXahCG\n\ +55+tAoGAI811NTb7kuuIPYuj4raDW88QVNX2xB3+p9lXGolB4jPgsUEjgSvLgH9S\n\ +Q/LwEBiCYVyii8MvWsIZpHvSGyOoty2p19/CAvrAOfpEVlnXQeiX+mh09p1mQbfM\n\ +y9rTR828ADcaZ63Ej1oL4GcqmGhODxCLy1YKKcy0FHzChqPMV6g=\n\ +-----END RSA PUBLIC KEY-----\n"; + + #[test] + fn a_private_pem_is_refused() { + // A verification key must be public and must never be stored otherwise: it is served + // back through the settings response. jsonwebtoken keys the public/private split off + // the PEM label, so private DER relabelled with a public armor slips a label check; + // only parsing the DER as a public-key structure (SPKI or PKCS#1 RSA public) refuses + // it. A real public key still parses, so the guard is not vacuous. + assert!(decoding_key_from_pem(RSA_PUBLIC).is_ok()); + // The same key with its body on one line (not 64-column wrapped): jsonwebtoken accepts + // any wrapping, so the guard must too rather than lean on the strict RFC 7468 decoder. + let body: String = RSA_PUBLIC + .lines() + .filter(|l| !l.starts_with("-----")) + .collect(); + let one_line = format!("-----BEGIN PUBLIC KEY-----\n{body}\n-----END PUBLIC KEY-----\n"); + assert!(decoding_key_from_pem(&one_line).is_ok()); + + assert!(decoding_key_from_pem(PRIV1).is_err()); + // The same PKCS#8 EC private key, relabelled `PUBLIC KEY`. + assert!(decoding_key_from_pem(&PRIV1.replace("PRIVATE KEY", "PUBLIC KEY")).is_err()); + assert!(decoding_key_from_pem(RSA_PKCS1_PRIVATE_AS_PUBLIC).is_err()); + } + + #[tokio::test] + async fn a_concurrent_cold_burst_makes_one_jwks_fetch() { + use std::sync::atomic::{AtomicUsize, Ordering}; + use tokio::io::AsyncWriteExt; + let _env = TEST_ENV_LOCK.lock().await; + // The stub listens on loopback, which SSRF validation refuses without this. + unsafe { std::env::set_var("ALLOW_PRIVATE_GUEST_JWKS_URLS", "true") }; + let listener = tokio::net::TcpListener::bind("127.0.0.1:0").await.unwrap(); + let addr = listener.local_addr().unwrap(); + let body = format!( + r#"{{"keys":[{{"kty":"EC","crv":"P-256","kid":"k1","x":"{PUB1_X}","y":"{PUB1_Y}"}}]}}"# + ); + let hits = std::sync::Arc::new(AtomicUsize::new(0)); + let hits_srv = hits.clone(); + tokio::spawn(async move { + loop { + let (mut sock, _) = listener.accept().await.unwrap(); + hits_srv.fetch_add(1, Ordering::SeqCst); + let resp = format!( + "HTTP/1.1 200 OK\r\nContent-Type: application/json\r\nContent-Length: {}\r\nConnection: close\r\n\r\n{}", + body.len(), + body + ); + // Delay so the other callers pile onto the single-flight lock before the + // leader's fetch returns. + tokio::time::sleep(Duration::from_millis(150)).await; + let _ = sock.write_all(resp.as_bytes()).await; + let _ = sock.shutdown().await; + } + }); + let url = format!("http://127.0.0.1:{}/jwks.json", addr.port()); + let mut handles = Vec::new(); + for _ in 0..10 { + let u = url.clone(); + handles.push(tokio::spawn(async move { + cached_jwks(&u).await.map(|e| e.keys.len()) + })); + } + for h in handles { + assert_eq!( + h.await.unwrap().unwrap(), + 1, + "each caller resolves the one key" + ); + } + assert_eq!( + hits.load(Ordering::SeqCst), + 1, + "single-flight: a concurrent cold burst makes one fetch" + ); + unsafe { std::env::remove_var("ALLOW_PRIVATE_GUEST_JWKS_URLS") }; + } + + #[tokio::test] + async fn jwks_fetch_locks_are_shared_and_self_cleaning() { + // The registry is a plain map, not a capacity-bounded cache: a cache could evict a + // lock mid-fetch, letting a later request for that URL start a duplicate fetch. Pin + // both halves of what keeps single-flight intact under many distinct URLs: the same + // URL hands back one shared lock, and the entry is removed once its last holder drops + // (so nothing evicts an in-flight lock and the map stays bounded by fetches in flight). + let url = "https://example.test/jwks-lock-probe.json"; + { + let a = JwksFetchLock::acquire(url); + let b = JwksFetchLock::acquire(url); + assert!(Arc::ptr_eq(&a.lock, &b.lock), "one lock per URL"); + assert!(JWKS_FETCH_LOCKS.lock().unwrap().contains_key(url)); + } + assert!( + !JWKS_FETCH_LOCKS.lock().unwrap().contains_key(url), + "the lock is dropped once idle" + ); + } + + #[tokio::test] + async fn stale_jwks_keys_stop_being_served_past_the_grace_window() { + let _env = TEST_ENV_LOCK.lock().await; + unsafe { std::env::set_var("ALLOW_PRIVATE_GUEST_JWKS_URLS", "true") }; + // A dead loopback port, so every refresh fails (connection refused). + let listener = tokio::net::TcpListener::bind("127.0.0.1:0").await.unwrap(); + let port = listener.local_addr().unwrap().port(); + drop(listener); + let jwk = jwk(serde_json::json!( + {"kty":"EC","crv":"P-256","kid":"k1","x":PUB1_X,"y":PUB1_Y} + )); + let keys: HashMap = [("k1".to_string(), jwk)].into_iter().collect(); + let stale = Instant::now().checked_sub(Duration::from_secs(1)).unwrap(); + + // Within the grace window, stale keys are still served while a (failing) refresh runs. + let within = format!("http://127.0.0.1:{port}/within"); + JWKS_CACHE.insert( + within.clone(), + Arc::new(JwksEntry { + keys: Arc::new(keys.clone()), + expires_at: stale, + fetched_at: Instant::now(), + }), + ); + assert!( + cached_jwks(&within).await.is_ok(), + "stale keys within the grace window are still served" + ); + + // Past the grace window, the keys are not served: the failing refresh fails closed. + let beyond = format!("http://127.0.0.1:{port}/beyond"); + JWKS_CACHE.insert( + beyond.clone(), + Arc::new(JwksEntry { + keys: Arc::new(keys), + expires_at: stale, + fetched_at: Instant::now() + .checked_sub(JWKS_MAX_STALE + Duration::from_secs(1)) + .unwrap(), + }), + ); + assert!( + cached_jwks(&beyond).await.is_err(), + "keys past the grace window fail closed once the refresh fails" + ); + unsafe { std::env::remove_var("ALLOW_PRIVATE_GUEST_JWKS_URLS") }; + } + + #[tokio::test] + async fn a_plaintext_http_jwks_url_is_refused_by_default() { + let _env = TEST_ENV_LOCK.lock().await; + unsafe { std::env::remove_var("ALLOW_PRIVATE_GUEST_JWKS_URLS") }; + // The JWKS supplies the keys that authenticate guest JWTs; without the operator opt-in, + // a plaintext URL (which an on-path attacker could replace) is refused for its scheme. + assert!(matches!( + crate::ssrf::validate_guest_jwks_url("http://issuer.example.com/jwks.json").await, + Err(crate::ssrf::SsrfValidationError::HttpsRequired) + )); + } + + #[tokio::test] + async fn the_instance_issuer_bypasses_the_https_and_private_restriction() { + let _env = TEST_ENV_LOCK.lock().await; + unsafe { std::env::remove_var("ALLOW_PRIVATE_GUEST_JWKS_URLS") }; + // The instance issuer is operator-trusted (it also backs jwt_ext_), so an http/private + // URL is not refused for its scheme: it reaches the connect and fails there (dead port), + // not at validation. A different URL is not the instance issuer and is still refused. + let instance = "http://127.0.0.1:1/jwks.json"; + unsafe { std::env::set_var("JWT_EXT_JWKS_URL", instance) }; + let trusted = fetch_jwks(instance).await.err().unwrap().to_string(); + let other = fetch_jwks("http://127.0.0.1:1/other.json") + .await + .err() + .unwrap() + .to_string(); + unsafe { std::env::remove_var("JWT_EXT_JWKS_URL") }; + assert!( + !trusted.contains("not allowed") && !trusted.contains("must use https"), + "instance issuer skips validation: {trusted}" + ); + assert!( + other.contains("not allowed") || other.contains("https"), + "a non-instance http url is still refused: {other}" + ); + } +} diff --git a/backend/windmill-common/src/lib.rs b/backend/windmill-common/src/lib.rs index b4128f80d2..7fb5e85a69 100644 --- a/backend/windmill-common/src/lib.rs +++ b/backend/windmill-common/src/lib.rs @@ -67,6 +67,7 @@ pub mod flow_status; pub mod flows; pub mod folders; pub mod global_settings; +pub mod guest_jwt; pub mod indexer; pub mod instance_config; pub mod job_metrics; diff --git a/backend/windmill-common/src/ssrf.rs b/backend/windmill-common/src/ssrf.rs index 4c50f597ff..713b2e0289 100644 --- a/backend/windmill-common/src/ssrf.rs +++ b/backend/windmill-common/src/ssrf.rs @@ -6,6 +6,8 @@ pub const ALLOW_PRIVATE_MCP_SERVER_URLS_ENV: &str = "ALLOW_PRIVATE_MCP_SERVER_UR pub const ALLOW_PRIVATE_SAML_METADATA_URLS_ENV: &str = "ALLOW_PRIVATE_SAML_METADATA_URLS"; +pub const ALLOW_PRIVATE_GUEST_JWKS_URLS_ENV: &str = "ALLOW_PRIVATE_GUEST_JWKS_URLS"; + /// Why a URL failed SSRF validation. /// /// The distinction matters for callers that gate private endpoints behind a @@ -18,6 +20,9 @@ pub enum SsrfValidationError { InvalidUrl(String), /// Scheme is not `http`/`https`. DisallowedScheme(String), + /// The URL uses `http` where `https` is required (guest JWKS). The private-host opt-in + /// also permits `http`, so, unlike the other scheme errors, this one the flag can fix. + HttpsRequired, /// No host in the URL. MissingHost, /// DNS resolution failed for the host. @@ -37,6 +42,9 @@ impl std::fmt::Display for SsrfValidationError { f, "URL scheme '{s}' is not allowed, only http and https are permitted" ), + SsrfValidationError::HttpsRequired => { + write!(f, "URL must use https") + } SsrfValidationError::MissingHost => write!(f, "URL must have a host"), SsrfValidationError::ResolutionFailed { host, source } => { write!(f, "Failed to resolve host '{host}': {source}") @@ -213,6 +221,36 @@ pub async fn validate_saml_metadata_url(url: &str) -> Result Result { + let parsed = + url::Url::parse(url).map_err(|e| SsrfValidationError::InvalidUrl(e.to_string()))?; + + let allow_private = std::env::var(ALLOW_PRIVATE_GUEST_JWKS_URLS_ENV) + .ok() + .is_some_and(|v| v == "true" || v == "1"); + + match parsed.scheme() { + "https" => {} + // Plaintext HTTP only under the explicit operator opt-in that also allows private + // hosts (dev/loopback): the JWKS supplies the keys that authenticate guest JWTs, so an + // on-path attacker who could replace an http response could forge accepted tokens. + "http" if allow_private => {} + "http" => return Err(SsrfValidationError::HttpsRequired), + scheme => return Err(SsrfValidationError::DisallowedScheme(scheme.to_string())), + } + + let host = parsed.host_str().ok_or(SsrfValidationError::MissingHost)?; + + if allow_private { + return Ok(ValidatedTarget::unpinned(host)); + } + + validate_url_for_ssrf(url).await +} + pub async fn validate_mcp_server_url(url: &str) -> Result { let parsed = url::Url::parse(url).map_err(|e| SsrfValidationError::InvalidUrl(e.to_string()))?; diff --git a/backend/windmill-common/src/users.rs b/backend/windmill-common/src/users.rs index 925fe32a4a..3d00c5d312 100644 --- a/backend/windmill-common/src/users.rs +++ b/backend/windmill-common/src/users.rs @@ -31,6 +31,29 @@ pub const USERNAME_GROUP_PREFIX: &str = "group-"; /// columns runnables and triggers store one in. pub const PERMISSIONED_AS_MAX_LEN: usize = 55; +/// Whether any account exists for `email`: a `password` row (deactivated ones +/// included, since the sign-in path filters `disabled = false` and a re-enabled +/// account must not read as absent) or a `usr` row in any workspace (what a service +/// account has instead of a password). A guest is someone with none: the single rule +/// that keeps an account holder from ever holding a cheaper guest identity. +/// +/// The address is lowercased before the lookup: accounts are stored lowercased, so a +/// mixed-case address would otherwise miss an existing account and be let through. The +/// comparison stays a plain equality (not `lower(email)`), so it uses the email index. +pub async fn has_any_account<'c, E: sqlx::Executor<'c, Database = sqlx::Postgres>>( + executor: E, + email: &str, +) -> crate::error::Result { + sqlx::query_scalar::<_, bool>( + "SELECT EXISTS(SELECT 1 FROM password WHERE email = $1) + OR EXISTS(SELECT 1 FROM usr WHERE email = $1)", + ) + .bind(email.to_lowercase()) + .fetch_one(executor) + .await + .map_err(|e| crate::error::Error::internal_err(format!("checking account for {email}: {e:#}"))) +} + /// An email-shaped username is its own principal, which is how a superadmin acting without a /// `usr` row is named (`usr.username` is constrained to `[\w-]+`, so a member never is). It is /// decided before the group convention — an address is never a group's username — and one diff --git a/frontend/src/lib/components/apps/editor/AppEditorHeaderDeploy.svelte b/frontend/src/lib/components/apps/editor/AppEditorHeaderDeploy.svelte index 6d9e353d46..c88a20bc99 100644 --- a/frontend/src/lib/components/apps/editor/AppEditorHeaderDeploy.svelte +++ b/frontend/src/lib/components/apps/editor/AppEditorHeaderDeploy.svelte @@ -176,6 +176,10 @@ }${customPath}` ) + // The app URL a guest JWT rides on: append `guest.` and the viewer authenticates the + // token as a seatless guest. Uses the custom URL when set, else the public secret URL. + let guestJwtBase = $derived(customPath !== undefined ? fullCustomUrl : secretUrlHref) + // When embedding a raw app in an iframe inside another Windmill app (or any // cross-origin-isolated page), the embedded document must set COEP. The // `wm_coep` flag opts the public app into the cross-origin isolation headers. @@ -500,8 +504,8 @@ Anyone your identity provider authenticates can open this app without a Windmill account. They join no workspace. Members of this workspace can open it too. {#if guestUsage} - {guestUsage.guest_count} of {guestUsage.free_allowance} free guests used across this - instance in the last {guestUsage.window_days} days; beyond that, {guestUsage.metered + {guestUsage.guest_count} of {guestUsage.free_allowance} free guests used across this instance + in the last {guestUsage.window_days} days; beyond that, {guestUsage.metered ? 'every four guests count as one seat' : 'new guests are refused until the count drops'}. {/if} @@ -543,6 +547,38 @@ {/if} + {#if embedMode && policy.execution_mode == 'guest' && guestAccessEnabled && guestJwtBase} +
+
+ Embed for your own authenticated users (guest JWT) +
+
+ To open this app for a user your own product already authenticates, mint a short-lived JWT + in your backend and append it to the app URL as guest.<jwt>. Each token + is its own seatless guest, confined to this app — no shared secret and no Windmill + account, unlike the plain secret URL above. +
+
+ Windmill verifies the token against the workspace's guest JWT key (Workspace settings → + Guests) — a PEM public key or a JWKS URL{#if !isCloudHosted()}, or the instance's + configured issuer (JWT_EXT_JWKS_URL) when no workspace key is set{/if}. Set + the public half there; in your backend, sign each token with the matching + private key using RS256/384/512, PS256/384/512 or ES256/384 (symmetric HS* is + refused), carrying email, workspace_id = {opWs}, + app_path = {appPath} and exp (at most 24h ahead). +
+ +
+ Replace YOUR_GUEST_JWT with the token your backend signs per user. Past the instance's + free guest allowance a new guest email is refused (see the count above); guests already seen + in the window keep working. +
+
+ {/if} +
{#if !($userStore?.is_admin || $userStore?.is_super_admin)} diff --git a/frontend/src/routes/(root)/(logged)/workspace_settings/+page.svelte b/frontend/src/routes/(root)/(logged)/workspace_settings/+page.svelte index 6e55c5994b..8590d87603 100644 --- a/frontend/src/routes/(root)/(logged)/workspace_settings/+page.svelte +++ b/frontend/src/routes/(root)/(logged)/workspace_settings/+page.svelte @@ -191,6 +191,19 @@ let guestAccessEnabled: boolean = $state(false) let guestUsage: GuestUsage | undefined = $state(undefined) let initialGuestAccessEnabled: boolean = $state(false) + // A guest JWT is verified against one key: a PEM public key, or a JWKS URL. The + // type picks which field is live; the other is cleared on save. + let guestJwtKeyType = $state<'pem' | 'jwks'>('pem') + let guestJwtPublicKey: string = $state('') + let guestJwtJwksUrl: string = $state('') + let initialGuestJwtPublicKey: string = $state('') + let initialGuestJwtJwksUrl: string = $state('') + // The pair actually saved: only the selected type's field, trimmed. The unselected + // one is empty, so switching type and saving clears what was there. + let effectiveGuestJwt = $derived({ + pem: guestJwtKeyType === 'pem' ? guestJwtPublicKey.trim() : '', + jwks: guestJwtKeyType === 'jwks' ? guestJwtJwksUrl.trim() : '' + }) let initialPublicAppRateLimitPerMinute: number | undefined = $state(undefined) let hasInstanceAiConfig = $state(false) @@ -526,11 +539,17 @@ } async function saveDefaultAppSettings(): Promise { - // Guests first: the only write of this card available on every plan, so a refused - // Enterprise-only write after it cannot swallow it. + // Guest access and the guest JWT key are the writes of this card available on every plan; + // save them first so a refused Enterprise-only write after cannot swallow them. if (guestAccessEnabled !== initialGuestAccessEnabled) { await editGuestAccess() } + if ( + effectiveGuestJwt.pem !== initialGuestJwtPublicKey || + effectiveGuestJwt.jwks !== initialGuestJwtJwksUrl + ) { + await editGuestJwtKey() + } if (workspaceDefaultAppPath !== initialWorkspaceDefaultAppPath) { await editWorkspaceDefaultApp() } @@ -539,6 +558,19 @@ } } + async function editGuestJwtKey(): Promise { + await WorkspaceService.editGuestJwtKey({ + workspace: $workspaceStore!, + requestBody: { + public_key: effectiveGuestJwt.pem || undefined, + jwks_url: effectiveGuestJwt.jwks || undefined + } + }) + initialGuestJwtPublicKey = effectiveGuestJwt.pem + initialGuestJwtJwksUrl = effectiveGuestJwt.jwks + sendUserToast('Guest JWT key updated') + } + async function editGuestAccess(): Promise { await WorkspaceService.editGuestAccess({ workspace: $workspaceStore!, @@ -647,6 +679,11 @@ initialPublicAppRateLimitPerMinute = settings.public_app_execution_limit_per_minute ?? undefined guestAccessEnabled = settings.guest_access_enabled ?? false initialGuestAccessEnabled = settings.guest_access_enabled ?? false + guestJwtPublicKey = settings.guest_jwt_public_key ?? '' + guestJwtJwksUrl = settings.guest_jwt_jwks_url ?? '' + initialGuestJwtPublicKey = guestJwtPublicKey + initialGuestJwtJwksUrl = guestJwtJwksUrl + guestJwtKeyType = guestJwtJwksUrl ? 'jwks' : 'pem' WorkspaceService.getGuestUsage({ workspace: $workspaceStore! }) .then((u) => (guestUsage = u)) .catch(() => (guestUsage = undefined)) @@ -1052,12 +1089,16 @@ savedValue: { defaultAppPath: initialWorkspaceDefaultAppPath, publicAppRateLimitPerMinute: initialPublicAppRateLimitPerMinute, - guestAccessEnabled: initialGuestAccessEnabled + guestAccessEnabled: initialGuestAccessEnabled, + guestJwtPem: initialGuestJwtPublicKey, + guestJwtJwks: initialGuestJwtJwksUrl }, modifiedValue: { defaultAppPath: workspaceDefaultAppPath, publicAppRateLimitPerMinute: publicAppRateLimitPerMinute, - guestAccessEnabled: guestAccessEnabled + guestAccessEnabled: guestAccessEnabled, + guestJwtPem: effectiveGuestJwt.pem, + guestJwtJwks: effectiveGuestJwt.jwks } } } @@ -1067,6 +1108,9 @@ workspaceDefaultAppPath = initialWorkspaceDefaultAppPath publicAppRateLimitPerMinute = initialPublicAppRateLimitPerMinute guestAccessEnabled = initialGuestAccessEnabled + guestJwtPublicKey = initialGuestJwtPublicKey + guestJwtJwksUrl = initialGuestJwtJwksUrl + guestJwtKeyType = initialGuestJwtJwksUrl ? 'jwks' : 'pem' } // Strip keys from extraArgs that are auto-managed by child components: @@ -2184,7 +2228,7 @@ export async function main( {:else if guestUsage} - {guestUsage.guest_count} of {guestUsage.free_allowance} free guests used across - this instance in the last {guestUsage.window_days} days. + {guestUsage.guest_count} of {guestUsage.free_allowance} free guests used across this + instance in the last {guestUsage.window_days} days. {#if guestUsage.metered} Beyond that, every four guests count as one seat{guestUsage.guest_seats > 0 ? ` (${guestUsage.guest_seats} now)` @@ -2210,6 +2254,54 @@ export async function main( {/if} {/if} +
+
+ Guest JWT verification key +
+
+ A guest can also enter through a JWT your own backend mints and signs, with no + identity-provider round-trip, for iframe embedding. The token must carry + email, workspace_id, app_path and + exp (lifetime capped at 24h); it opens only the app named by + app_path. Accepted algorithms: RS256/384/512, PS256/384/512, + ES256/384. Symmetric algorithms (HS*) are refused. Configure one key, a PEM + public key or a JWKS URL (which must be https). Point it at an issuer you + control: any token that key signs carrying these claims is accepted, so a shared + multi-tenant issuer is not a good fit. +
+ + {#snippet children({ item })} + + + {/snippet} + + {#if guestJwtKeyType === 'pem'} + + {:else} + + {/if} + {#if !isCloudHosted()} +
+ Leave empty to fall back to the instance's configured JWT issuer (JWT_EXT_JWKS_URL), if one is set. Set a key here to trust a different issuer for this + workspace. +
+ {/if} +
{:else if tab == 'native_triggers'} {#if $workspaceStore} diff --git a/frontend/src/routes/a/[...path]/+page.svelte b/frontend/src/routes/a/[...path]/+page.svelte index b63d5086f7..e9fd20877c 100644 --- a/frontend/src/routes/a/[...path]/+page.svelte +++ b/frontend/src/routes/a/[...path]/+page.svelte @@ -19,30 +19,46 @@ let jwtError = $state(false) function isJwt(t: string) { - // simply check that the first part is a valid base64 encoded json + // A JWT is three dot-separated base64url segments; check the header decodes to + // JSON. `atob` wants standard base64, so normalise base64url first (a `kid` or a + // signature routinely contains `-`/`_`), or a valid token is taken for a path. try { const parts = t.split('.') - const header = atob(parts[0]) - JSON.parse(header) + if (parts.length !== 3) return false + const b64 = parts[0].replace(/-/g, '+').replace(/_/g, '/') + const pad = b64.length % 4 === 0 ? '' : '='.repeat(4 - (b64.length % 4)) + JSON.parse(atob(b64 + pad)) return true } catch (e) { return false } } - function parseCustomPath(customPath: string): { path: string; jwt: string | undefined } { + // The custom path may carry a trailing credential: an external JWT as its last + // segment, or a guest JWT in a `guest.` last segment (`/guest.`). The + // `guest.` prefix keeps the two apart; `viewerUrl` uses `path` alone, so neither + // reaches the opaque iframe. + function parseCustomPath(customPath: string): { + path: string + jwt: string | undefined + guestJwt: string | undefined + } { const parts = customPath.split('/') - if (parts.length > 1 && isJwt(parts[parts.length - 1])) { + const last = parts[parts.length - 1] + // A guest JWT rides the last segment prefixed `guest.`. The `.` means it can never + // be a valid custom-path segment, so a real path ending in a `guest` segment + // followed by an external JWT (`.../guest/`) is read as before, not hijacked. + if (last.startsWith('guest.') && isJwt(last.slice('guest.'.length))) { return { path: parts.slice(0, -1).join('/'), - jwt: parts[parts.length - 1] - } - } else { - return { - path: customPath, - jwt: undefined + jwt: undefined, + guestJwt: last.slice('guest.'.length) } } + if (parts.length > 1 && isJwt(last)) { + return { path: parts.slice(0, -1).join('/'), jwt: last, guestJwt: undefined } + } + return { path: customPath, jwt: undefined, guestJwt: undefined } } const parsedCustomPath = parseCustomPath(page.params.path ?? '') @@ -102,7 +118,9 @@ // Embedder side: validate access (main session cookie or shared JWT) and mint // a scoped embed token for the opaque iframe (WIN-2006). async function fetchEmbedToken(opts?: { sdkConsent?: boolean }): Promise<{ token?: string }> { - if (parsedCustomPath.jwt) { + if (parsedCustomPath.guestJwt) { + OpenAPI.TOKEN = 'jwt_guest_' + parsedCustomPath.guestJwt + } else if (parsedCustomPath.jwt) { OpenAPI.TOKEN = 'jwt_ext_' + parsedCustomPath.jwt } const headers: Record = {} diff --git a/frontend/src/routes/public/[workspace]/[...secret]/+page.svelte b/frontend/src/routes/public/[workspace]/[...secret]/+page.svelte index de2b4755be..b2a29b6316 100644 --- a/frontend/src/routes/public/[workspace]/[...secret]/+page.svelte +++ b/frontend/src/routes/public/[workspace]/[...secret]/+page.svelte @@ -27,12 +27,25 @@ * offering an ordinary sign-in on a transient fault would provision an account. */ let guestEntry: 'pending' | 'none' | 'guest' | 'error' = $state('pending') - function parseSecret(secret: string): { secret: string; jwt: string | undefined } { + // The share link carries a trailing credential the embedder consumes: an external + // JWT as `/`, or a guest JWT as `/guest.`. The `guest.` + // prefix keeps the two apart with no parsing of the token, which the page cannot + // verify anyway. Either way `viewerUrl` below uses `secret` alone, so no JWT + // reaches the opaque iframe. + function parseSecret(secret: string): { + secret: string + jwt: string | undefined + guestJwt: string | undefined + } { const parts = secret.split('/') - return { - secret: parts[0], - jwt: parts[1] + // The credential rides the segment after the secret: a guest JWT prefixed + // `guest.`, or an external JWT bare. The `guest.` prefix glues the marker to the + // token, so it can never be mistaken for a path or secret segment (which carry no + // `.`), and a bare token keeps the established external-JWT interpretation. + if (parts[1]?.startsWith('guest.')) { + return { secret: parts[0], jwt: undefined, guestJwt: parts[1].slice('guest.'.length) } } + return { secret: parts[0], jwt: parts[1], guestJwt: undefined } } const parsedSecret = parseSecret(page.params.secret ?? '') @@ -52,7 +65,9 @@ // Embedder side: validate access (using the main session cookie or the shared // JWT) and mint a scoped embed token for the opaque iframe (WIN-2006). async function fetchEmbedToken(opts?: { sdkConsent?: boolean }): Promise<{ token?: string }> { - if (parsedSecret.jwt) { + if (parsedSecret.guestJwt) { + OpenAPI.TOKEN = 'jwt_guest_' + parsedSecret.guestJwt + } else if (parsedSecret.jwt) { OpenAPI.TOKEN = 'jwt_ext_' + parsedSecret.jwt } const headers: Record = {} From 130a2f74083ba1bd308beeb86e2cbbaa41fd3345 Mon Sep 17 00:00:00 2001 From: hugocasa Date: Sat, 5 Sep 2026 12:38:20 +0200 Subject: [PATCH 06/13] feat: instrument sandbox isolation, data tables and in-flow script edits (#10981) * feat: instrument sandbox isolation, data tables and in-flow script edits Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01GLnp4v49BozkDd3KeWn5Q3 * fix: address review findings on the new telemetry counters Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01GLnp4v49BozkDd3KeWn5Q3 * refactor: inline single-site telemetry helpers and trim what is collected Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01GLnp4v49BozkDd3KeWn5Q3 * docs: tighten the telemetry disclosure copy Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01GLnp4v49BozkDd3KeWn5Q3 * chore: update ee-repo-ref to 5921c03c8e28642efd1c390f590c0dab9834fa99 This commit updates the EE repository reference after PR #780 was merged in windmill-ee-private. Previous ee-repo-ref: 548b5e0421a04a2d9a76cce6efc6c91b1d8560ee New ee-repo-ref: 5921c03c8e28642efd1c390f590c0dab9834fa99 Automated by sync-ee-ref workflow. --------- Co-authored-by: Claude Opus 5 (1M context) Co-authored-by: windmill-internal-app[bot] Co-authored-by: Ruben Fiszel --- ...23e16b5931c08c1073b09ab01dad205f161ed.json | 26 ++++++++++ ...8f9943111e0bac7d4fb478300f0cd8b799f23.json | 32 ++++++++++++ ...523709f837b7e1969b7364f70cc2a60cdd520.json | 26 ++++++++++ ...da82104a9ee39bfbe5d932340a3c2c209c5d0.json | 26 ++++++++++ ...b6f369eceb079b31ca118cae23c3d15314590.json | 26 ++++++++++ backend/ee-repo-ref.txt | 2 +- .../src/datatable_migrations.rs | 50 +++++++++++++++++++ .../windmill-api-workspaces/src/workspaces.rs | 17 ++++++- docs/feature-telemetry.md | 7 +-- .../lib/components/DdlMigrationGuard.svelte | 4 ++ .../lib/components/InstanceSettings.svelte | 22 +++++--- .../apps/editor/AppEditorHeaderDeploy.svelte | 7 +++ .../flows/common/FlowCardHeader.svelte | 12 ++++- .../AddDataTableWizard.svelte | 8 +++ .../workspaceSettings/datatableTelemetry.ts | 41 +++++++++++++++ 15 files changed, 292 insertions(+), 14 deletions(-) create mode 100644 backend/.sqlx/query-0411a67eb9d88244fa654eda51123e16b5931c08c1073b09ab01dad205f161ed.json create mode 100644 backend/.sqlx/query-209c96d522f9683b39f053707568f9943111e0bac7d4fb478300f0cd8b799f23.json create mode 100644 backend/.sqlx/query-2105d37be923a445933c899bb31523709f837b7e1969b7364f70cc2a60cdd520.json create mode 100644 backend/.sqlx/query-9b727f03e74ea4a35146c9a339cda82104a9ee39bfbe5d932340a3c2c209c5d0.json create mode 100644 backend/.sqlx/query-d3a6a27ece3b5d5071dd8074b8db6f369eceb079b31ca118cae23c3d15314590.json create mode 100644 frontend/src/lib/components/workspaceSettings/datatableTelemetry.ts diff --git a/backend/.sqlx/query-0411a67eb9d88244fa654eda51123e16b5931c08c1073b09ab01dad205f161ed.json b/backend/.sqlx/query-0411a67eb9d88244fa654eda51123e16b5931c08c1073b09ab01dad205f161ed.json new file mode 100644 index 0000000000..67f2bbbe81 --- /dev/null +++ b/backend/.sqlx/query-0411a67eb9d88244fa654eda51123e16b5931c08c1073b09ab01dad205f161ed.json @@ -0,0 +1,26 @@ +{ + "db_name": "PostgreSQL", + "query": "SELECT\n COALESCE(dt.value->'database'->>'resource_type', 'unknown') AS \"kind!\",\n COUNT(*)::BIGINT AS \"count!\"\n FROM workspace_settings ws,\n LATERAL jsonb_each(ws.datatable->'datatables') dt\n WHERE jsonb_typeof(ws.datatable->'datatables') = 'object'\n GROUP BY 1", + "describe": { + "columns": [ + { + "ordinal": 0, + "name": "kind!", + "type_info": "Text" + }, + { + "ordinal": 1, + "name": "count!", + "type_info": "Int8" + } + ], + "parameters": { + "Left": [] + }, + "nullable": [ + null, + null + ] + }, + "hash": "0411a67eb9d88244fa654eda51123e16b5931c08c1073b09ab01dad205f161ed" +} diff --git a/backend/.sqlx/query-209c96d522f9683b39f053707568f9943111e0bac7d4fb478300f0cd8b799f23.json b/backend/.sqlx/query-209c96d522f9683b39f053707568f9943111e0bac7d4fb478300f0cd8b799f23.json new file mode 100644 index 0000000000..56128dab99 --- /dev/null +++ b/backend/.sqlx/query-209c96d522f9683b39f053707568f9943111e0bac7d4fb478300f0cd8b799f23.json @@ -0,0 +1,32 @@ +{ + "db_name": "PostgreSQL", + "query": "SELECT\n COUNT(*) FILTER (WHERE dt.value->>'migrations_enabled' = 'true')::BIGINT AS \"enabled!\",\n COUNT(*) FILTER (WHERE dt.value->>'migrations_enabled' = 'false')::BIGINT AS \"disabled!\",\n COUNT(*) FILTER (WHERE dt.value->>'migrations_enabled' IS NULL)::BIGINT AS \"unset!\"\n FROM workspace_settings ws,\n LATERAL jsonb_each(ws.datatable->'datatables') dt\n WHERE jsonb_typeof(ws.datatable->'datatables') = 'object'", + "describe": { + "columns": [ + { + "ordinal": 0, + "name": "enabled!", + "type_info": "Int8" + }, + { + "ordinal": 1, + "name": "disabled!", + "type_info": "Int8" + }, + { + "ordinal": 2, + "name": "unset!", + "type_info": "Int8" + } + ], + "parameters": { + "Left": [] + }, + "nullable": [ + null, + null, + null + ] + }, + "hash": "209c96d522f9683b39f053707568f9943111e0bac7d4fb478300f0cd8b799f23" +} diff --git a/backend/.sqlx/query-2105d37be923a445933c899bb31523709f837b7e1969b7364f70cc2a60cdd520.json b/backend/.sqlx/query-2105d37be923a445933c899bb31523709f837b7e1969b7364f70cc2a60cdd520.json new file mode 100644 index 0000000000..c1ff273bcb --- /dev/null +++ b/backend/.sqlx/query-2105d37be923a445933c899bb31523709f837b7e1969b7364f70cc2a60cdd520.json @@ -0,0 +1,26 @@ +{ + "db_name": "PostgreSQL", + "query": "SELECT\n COUNT(*)::BIGINT AS \"total!\",\n COUNT(DISTINCT (workspace_id, datatable))::BIGINT AS \"datatables!\"\n FROM datatable_migrations", + "describe": { + "columns": [ + { + "ordinal": 0, + "name": "total!", + "type_info": "Int8" + }, + { + "ordinal": 1, + "name": "datatables!", + "type_info": "Int8" + } + ], + "parameters": { + "Left": [] + }, + "nullable": [ + null, + null + ] + }, + "hash": "2105d37be923a445933c899bb31523709f837b7e1969b7364f70cc2a60cdd520" +} diff --git a/backend/.sqlx/query-9b727f03e74ea4a35146c9a339cda82104a9ee39bfbe5d932340a3c2c209c5d0.json b/backend/.sqlx/query-9b727f03e74ea4a35146c9a339cda82104a9ee39bfbe5d932340a3c2c209c5d0.json new file mode 100644 index 0000000000..30216f04e9 --- /dev/null +++ b/backend/.sqlx/query-9b727f03e74ea4a35146c9a339cda82104a9ee39bfbe5d932340a3c2c209c5d0.json @@ -0,0 +1,26 @@ +{ + "db_name": "PostgreSQL", + "query": "SELECT\n COUNT(*) FILTER (WHERE av.raw_app = false)::BIGINT AS \"low_code!\",\n COUNT(*) FILTER (WHERE av.raw_app = true)::BIGINT AS \"raw!\"\n FROM app a\n JOIN app_version av ON av.id = a.versions[array_upper(a.versions, 1)]\n WHERE a.policy->>'sandbox' = 'true'", + "describe": { + "columns": [ + { + "ordinal": 0, + "name": "low_code!", + "type_info": "Int8" + }, + { + "ordinal": 1, + "name": "raw!", + "type_info": "Int8" + } + ], + "parameters": { + "Left": [] + }, + "nullable": [ + null, + null + ] + }, + "hash": "9b727f03e74ea4a35146c9a339cda82104a9ee39bfbe5d932340a3c2c209c5d0" +} diff --git a/backend/.sqlx/query-d3a6a27ece3b5d5071dd8074b8db6f369eceb079b31ca118cae23c3d15314590.json b/backend/.sqlx/query-d3a6a27ece3b5d5071dd8074b8db6f369eceb079b31ca118cae23c3d15314590.json new file mode 100644 index 0000000000..ad5a3c9f56 --- /dev/null +++ b/backend/.sqlx/query-d3a6a27ece3b5d5071dd8074b8db6f369eceb079b31ca118cae23c3d15314590.json @@ -0,0 +1,26 @@ +{ + "db_name": "PostgreSQL", + "query": "SELECT usage_kind::text AS \"kind!\", COUNT(*)::BIGINT AS \"count!\"\n FROM asset WHERE kind = 'datatable' AND usage_kind <> 'job'\n GROUP BY 1\n UNION ALL\n SELECT 'job_recent'::text, COUNT(DISTINCT (workspace_id, path))::BIGINT\n FROM asset\n WHERE kind = 'datatable' AND usage_kind = 'job'\n AND created_at > now() - interval '30 days'", + "describe": { + "columns": [ + { + "ordinal": 0, + "name": "kind!", + "type_info": "Text" + }, + { + "ordinal": 1, + "name": "count!", + "type_info": "Int8" + } + ], + "parameters": { + "Left": [] + }, + "nullable": [ + null, + null + ] + }, + "hash": "d3a6a27ece3b5d5071dd8074b8db6f369eceb079b31ca118cae23c3d15314590" +} diff --git a/backend/ee-repo-ref.txt b/backend/ee-repo-ref.txt index a07d475cb9..11e45db8a7 100644 --- a/backend/ee-repo-ref.txt +++ b/backend/ee-repo-ref.txt @@ -1 +1 @@ -c2270eb5fe2d9f0968253e6b460c33186363f4e7 +5921c03c8e28642efd1c390f590c0dab9834fa99 diff --git a/backend/windmill-api-workspaces/src/datatable_migrations.rs b/backend/windmill-api-workspaces/src/datatable_migrations.rs index bb4310d1bc..ef5ea37e1d 100644 --- a/backend/windmill-api-workspaces/src/datatable_migrations.rs +++ b/backend/windmill-api-workspaces/src/datatable_migrations.rs @@ -416,6 +416,16 @@ async fn run_datatable_migrations( let applied_versions = read_applied_versions_on_client(&client, &datatable_name).await?; + // How the user scoped the run, for the counter emitted on the first migration + // that lands below. + let scope = if query.only.is_some() { + "only" + } else if query.up_to.is_some() { + "up_to" + } else { + "all" + }; + let mut applied = Vec::new(); for m in migrations { if let Some(only) = query.only { @@ -453,6 +463,14 @@ async fn run_datatable_migrations( )) })?; applied.push(AppliedMigration { version: m.timestamp, name: m.name }); + // One event per run that moved the data table forward, emitted on the + // first migration that lands rather than after the loop: a later one + // failing returns early, and that run still advanced the data table. A + // run with nothing pending stays uncounted — it is the common outcome of + // opening the list and would drown out the runs that did something. + if applied.len() == 1 { + windmill_common::feature_usage::log_feature_usage("datatable", "migration_run", scope); + } } Ok(Json(RunDatatableMigrationsResult { applied })) @@ -594,6 +612,12 @@ async fn rollback_datatable_migrations( )) })?; + windmill_common::feature_usage::log_feature_usage( + "datatable", + "migration_rollback", + if query.only.is_some() { "only" } else { "last" }, + ); + Ok(Json(RollbackDatatableMigrationsResult { rolled_back: vec![RolledBackMigration { version, name: definition.name }], })) @@ -824,6 +848,8 @@ async fn enable_datatable_migrations( ) .await?; + windmill_common::feature_usage::log_feature_usage("datatable", "migrations_toggled", "on"); + Ok(format!( "Enabled migrations for data table {datatable_name}" )) @@ -892,6 +918,8 @@ async fn disable_datatable_migrations( .await?; } + windmill_common::feature_usage::log_feature_usage("datatable", "migrations_toggled", "off"); + Ok(format!( "Disabled migrations for data table {datatable_name} and deleted its migrations" )) @@ -1134,6 +1162,8 @@ async fn create_datatable_migration( ) .await?; + windmill_common::feature_usage::log_feature_usage("datatable", "migration_created", "manual"); + Ok(Json(DatatableMigration { datatable: datatable_name, timestamp, @@ -1371,6 +1401,20 @@ async fn upsert_datatable_migration( ) .await?; + // An unchanged re-push is not counted: `wmill sync push` sends every migration + // on every sync, so counting those would swamp the definitions people write. + if !unchanged { + windmill_common::feature_usage::log_feature_usage( + "datatable", + "migration_created", + if existing.is_none() { + "synced" + } else { + "edited" + }, + ); + } + Ok(format!( "Upserted migration {} in {}", payload.timestamp, datatable_name @@ -1477,6 +1521,12 @@ async fn generate_initial_datatable_migration( ) .await?; + windmill_common::feature_usage::log_feature_usage( + "datatable", + "migration_created", + "initial_snapshot", + ); + Ok(Json(DatatableMigration { datatable: datatable_name, timestamp, diff --git a/backend/windmill-api-workspaces/src/workspaces.rs b/backend/windmill-api-workspaces/src/workspaces.rs index 620e830a6a..c9343840d2 100644 --- a/backend/windmill-api-workspaces/src/workspaces.rs +++ b/backend/windmill-api-workspaces/src/workspaces.rs @@ -3524,6 +3524,9 @@ async fn edit_datatable_config( // Migrations opt-in is owned by the enable/disable endpoints, not this config // form: preserve each existing data table's flag, and default brand-new data // tables to enabled. + // Counted here rather than after the write because this is where a rename is + // still distinguishable from a creation; emitted once the commit lands. + let mut created_substrates: Vec<&'static str> = Vec::new(); for (name, dt) in new_config.settings.datatables.iter_mut() { let lookup = rename_src .get(name.as_str()) @@ -3531,7 +3534,15 @@ async fn edit_datatable_config( .unwrap_or(name.as_str()); dt.migrations_enabled = match old_datatables.get(lookup) { Some(old) => old.migrations_enabled, - None => Some(true), + None => { + // Keyed by how the substrate is serialized into `workspace_settings`, + // so these line up with the `datatable_configured` adoption counts. + created_substrates.push(match dt.database.resource_type { + DataTableCatalogResourceType::Instance => "instance", + DataTableCatalogResourceType::Postgresql => "postgresql", + }); + Some(true) + } }; } @@ -3589,6 +3600,10 @@ async fn edit_datatable_config( tx.commit().await?; + for substrate in created_substrates { + windmill_common::feature_usage::log_feature_usage("datatable", "created", substrate); + } + crate::datatable_migrations::record_datatable_cascade_deployments( &authed, &db, diff --git a/docs/feature-telemetry.md b/docs/feature-telemetry.md index 5cef3c9364..f5ce2357ca 100644 --- a/docs/feature-telemetry.md +++ b/docs/feature-telemetry.md @@ -4,9 +4,10 @@ anonymous usage-stats payload. It answers "does anyone use this, and which variant do they pick" without any identifying data leaving the instance. -It currently carries 32 registered actions across fifteen features (`ai_session`, `ai_chat`, -`ai_fix`, `ai_agent`, `ai_agent_eval`, `flow_editor`, `flow_run`, `flow_step`, `run_form`, -`debugger`, `trigger`, `command_script`, `hub_script`, `usage_meter`, `sso_groups_claim`). Nearly all of the +It currently carries 42 registered actions across seventeen features (`ai_session`, `ai_chat`, +`ai_fix`, `ai_agent`, `ai_agent_eval`, `app_sandbox`, `datatable`, `flow_editor`, `flow_run`, +`flow_step`, `run_form`, `debugger`, `trigger`, `command_script`, `hub_script`, `usage_meter`, +`sso_groups_claim`). Nearly all of the product is uninstrumented, so new user-facing work is the opportunity to change that. ## When to instrument diff --git a/frontend/src/lib/components/DdlMigrationGuard.svelte b/frontend/src/lib/components/DdlMigrationGuard.svelte index b6833521f8..ae5182f560 100644 --- a/frontend/src/lib/components/DdlMigrationGuard.svelte +++ b/frontend/src/lib/components/DdlMigrationGuard.svelte @@ -4,6 +4,7 @@ import NewDataTableMigrationModal from './workspaceSettings/NewDataTableMigrationModal.svelte' import DataTableMigrationsButton from './workspaceSettings/DataTableMigrationsButton.svelte' import { splitSqlStatements, isDdlStatement } from './sqlDdl' + import { logDdlGuardChoice } from './workspaceSettings/datatableTelemetry' import { CornerDownLeft } from 'lucide-svelte' let { workspace, datatable }: { workspace: string; datatable: string } = $props() @@ -97,9 +98,11 @@ for (;;) { const choice = await promptDdl(statement) if (choice === 'cancel') { + logDdlGuardChoice('cancelled') return { proceed: false, code, ranMigration: migrationRan } } if (choice === 'run') { + logDdlGuardChoice('run_anyway') kept.push(statement) break } @@ -107,6 +110,7 @@ // created; if the modal was cancelled, loop back to the prompt. const created = await openMigrationModal(statement) if (created) { + logDdlGuardChoice('migrated') break } } diff --git a/frontend/src/lib/components/InstanceSettings.svelte b/frontend/src/lib/components/InstanceSettings.svelte index 19f2477e37..d124569929 100644 --- a/frontend/src/lib/components/InstanceSettings.svelte +++ b/frontend/src/lib/components/InstanceSettings.svelte @@ -1076,12 +1076,15 @@ model identifiers, the names of public hub scripts used, the languages debug sessions are started for, whether AI chat skills are turned on or off and how often one is loaded, whether SSO logins evaluate an IdP groups claim (SAML or OIDC) and change a - membership, and the plan tier and quota shown when the execution meter is opened, last - 30 days)
  • feature adoption (counts of which flow, script, trigger and worker features your - deployed items use)
    • feature adoption (counts of which flow, script, trigger, worker and data table + features your deployed items use, including how many apps run sandboxed, how many data + tables exist per database kind, how many use migrations, and what references them)
  • resource counts (workspaces, scripts per language, flows, workflows as code, low-code @@ -1135,12 +1138,15 @@ model identifiers, the names of public hub scripts used, the languages debug sessions are started for, whether AI chat skills are turned on or off and how often one is loaded, whether SSO logins evaluate an IdP groups claim (SAML or OIDC) and change a - membership, and the plan tier and quota shown when the execution meter is opened, last - 30 days)
  • feature adoption (counts of which flow, script, trigger and worker features your - deployed items use)
    • feature adoption (counts of which flow, script, trigger, worker and data table + features your deployed items use, including how many apps run sandboxed, how many data + tables exist per database kind, how many use migrations, and what references them)
  • resource counts (workspaces, scripts per language, flows, workflows as code, low-code diff --git a/frontend/src/lib/components/apps/editor/AppEditorHeaderDeploy.svelte b/frontend/src/lib/components/apps/editor/AppEditorHeaderDeploy.svelte index c88a20bc99..7b68389d82 100644 --- a/frontend/src/lib/components/apps/editor/AppEditorHeaderDeploy.svelte +++ b/frontend/src/lib/components/apps/editor/AppEditorHeaderDeploy.svelte @@ -25,6 +25,7 @@ } from '$lib/components/OnBehalfOfSelector.svelte' import { canUserBypassRuleKind, protectionRulesState } from '$lib/workspaceProtectionRules.svelte' import { FRONTEND_SDK_SCOPES } from '$lib/components/raw_apps/sdkScopes' + import { logFeatureUsage } from '$lib/utils/featureUsage' const WM_DEPLOYERS_GROUP = 'wm_deployers' @@ -352,6 +353,12 @@ checked={policy.sandbox == true} on:change={(e) => { policy.sandbox = e.detail || undefined + // Counted where the toggle is flipped rather than where the policy is + // persisted: a not-yet-deployed app only mutates it locally, and skipping + // those would read as unused in the case where it is picked up front. + logFeatureUsage('app_sandbox', 'toggled', { + key: `${rawApp ? 'raw' : 'low_code'}:${e.detail ? 'on' : 'off'}` + }) // Frontend API access exists only for a sandboxed app, so turning // isolation off drops the declared scopes with it rather than leaving // them set but inert. diff --git a/frontend/src/lib/components/flows/common/FlowCardHeader.svelte b/frontend/src/lib/components/flows/common/FlowCardHeader.svelte index 33b47a9cb5..063367cc85 100644 --- a/frontend/src/lib/components/flows/common/FlowCardHeader.svelte +++ b/frontend/src/lib/components/flows/common/FlowCardHeader.svelte @@ -25,6 +25,7 @@ import { sendUserToast, type Item } from '$lib/utils' import { twMerge } from 'tailwind-merge' import { getToolNameError } from '$lib/components/flows/agentToolUtils' + import { logFeatureUsage } from '$lib/utils/featureUsage' import autosize from '$lib/autosize' interface Props { @@ -104,7 +105,16 @@ if (flowModuleValue?.type !== 'script') return const hash = flowModuleValue.hash ?? (await getLatestHashForScript(flowModuleValue.path, opWs)) - $scriptEditorDrawer?.openDrawer(hash, () => { + // Same reason the settings item below is gated: the local-dev editors publish + // the context store but never render the drawer, so an unmounted one makes + // this a no-op — and a no-op must not be counted as an editor open. + const drawer = $scriptEditorDrawer + if (!drawer) return + logFeatureUsage('flow_step', 'script_edit', { key: 'opened' }) + // The drawer only runs this callback once a new version is deployed, so it is + // what separates opening the editor from actually editing the script here. + drawer.openDrawer(hash, () => { + logFeatureUsage('flow_step', 'script_edit', { key: 'saved' }) dispatch('reload') sendUserToast('Script has been updated') }) diff --git a/frontend/src/lib/components/workspaceSettings/AddDataTableWizard.svelte b/frontend/src/lib/components/workspaceSettings/AddDataTableWizard.svelte index 3495bdbee0..0300ddf310 100644 --- a/frontend/src/lib/components/workspaceSettings/AddDataTableWizard.svelte +++ b/frontend/src/lib/components/workspaceSettings/AddDataTableWizard.svelte @@ -34,6 +34,7 @@ import DataTableConnectionReport from './DataTableConnectionReport.svelte' import { useSupabaseOauth } from './supabaseOauth.svelte' import { probeDatatableConnection } from './datatableProbe' + import { logDatatableWizard } from './datatableTelemetry' import { anythingClaimed, claimOf, @@ -526,11 +527,13 @@ await loadTargetUser() reset(parked ?? resume) opened = true + logDatatableWizard({ step: 'opened' }) } function selectProvider(key: Provider) { if (key === wiz.provider) return wiz.provider = key + logDatatableWizard({ step: 'picked', provider: key }) invalidate() if (key === 'instance') wiz.instance.dbName ??= defaultInstanceDbName() } @@ -833,6 +836,11 @@ createdProjects } } + // The setup's own verdict, so a data table that exists counts as done even when the + // caller's appended `onFinishAlso` step failed after it. + if (wiz.provider) { + logDatatableWizard({ step: result?.ok ? 'done' : 'failed', provider: wiz.provider }) + } onDone() } } diff --git a/frontend/src/lib/components/workspaceSettings/datatableTelemetry.ts b/frontend/src/lib/components/workspaceSettings/datatableTelemetry.ts new file mode 100644 index 0000000000..8dab8e71f9 --- /dev/null +++ b/frontend/src/lib/components/workspaceSettings/datatableTelemetry.ts @@ -0,0 +1,41 @@ +import { logFeatureUsage } from '$lib/utils/featureUsage' + +// Anonymous counters for the data table surfaces the backend cannot see: which substrate the +// add-wizard is pointed at and how far a run gets, and what the DDL guard talks people into. +// Same rules as every other `logFeatureUsage` caller: aggregated counts only, and the keys +// below are the whole vocabulary — no data table name, connection string, resource path or SQL +// ever reaches here. + +/** The substrate a wizard run is pointed at. Mirrors the wizard's own `Provider`. */ +export type DatatableWizardProvider = 'supabase' | 'instance' | 'resource' + +export type DatatableWizardEvent = + /** The wizard was opened, including a run resumed from the Supabase redirect. */ + | { step: 'opened' } + /** A substrate was picked. Re-picking a different one counts again, by design: the + * abandoned branch is the interesting half of a funnel. */ + | { step: 'picked'; provider: DatatableWizardProvider } + /** A run finished, with the verdict the checklist reported. */ + | { step: 'done' | 'failed'; provider: DatatableWizardProvider } + +export function logDatatableWizard(event: DatatableWizardEvent): void { + const key = event.step === 'opened' ? 'opened' : `${event.step}_${event.provider}` + logFeatureUsage('datatable', 'wizard', { key }) +} + +export type DdlGuardChoice = + /** The DDL was run ad-hoc, against the guard's advice. */ + | 'run_anyway' + /** The DDL became a migration definition. */ + | 'migrated' + /** The statement was abandoned, so nothing ran. */ + | 'cancelled' + +/** + * Counted once per prompt that reaches a terminal choice. Picking "create a migration" and then + * dismissing the modal loops back to the prompt instead, and is deliberately not counted: it is + * the same statement still undecided, not a fourth outcome. + */ +export function logDdlGuardChoice(choice: DdlGuardChoice): void { + logFeatureUsage('datatable', 'ddl_guard', { key: choice }) +} From 1901d3193bfc6a9e29d0b7c5389fef44ff9d3687 Mon Sep 17 00:00:00 2001 From: Guilhem Date: Sat, 5 Sep 2026 12:38:44 +0200 Subject: [PATCH 07/13] fix: keep the instance user editor popover inside the viewport (#10979) * fix: keep the instance user editor popover inside the viewport Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01A5v4NFqdTaZdkR8Ua1nr13 * fix: drop inert flex and min-h-0 classes from the user editor popover Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01A5v4NFqdTaZdkR8Ua1nr13 --------- Co-authored-by: Claude Opus 5 (1M context) --- .../src/lib/components/InstanceNameEditor.svelte | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/frontend/src/lib/components/InstanceNameEditor.svelte b/frontend/src/lib/components/InstanceNameEditor.svelte index 3b5cce4945..9d21da963c 100644 --- a/frontend/src/lib/components/InstanceNameEditor.svelte +++ b/frontend/src/lib/components/InstanceNameEditor.svelte @@ -6,7 +6,6 @@ import { createEventDispatcher } from 'svelte' import Button from './common/button/Button.svelte' import Popover from './meltComponents/Popover.svelte' - import { offset, flip, shift } from 'svelte-floating-ui/dom' import ChangeInstanceUsernameInner from './ChangeInstanceUsernameInner.svelte' import ChangeInstanceEmailInner from './ChangeInstanceEmailInner.svelte' import { UserService } from '$lib/gen' @@ -53,11 +52,8 @@ {#snippet trigger()} @@ -66,7 +62,10 @@ > {/snippet} {#snippet content()} -
    + +
    Date: Sat, 5 Sep 2026 12:39:44 +0200 Subject: [PATCH 08/13] fix(frontend): render ordered lists in markdown descriptions (#10973) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(frontend): render ordered lists in markdown descriptions `GfmMarkdown` defaulted to `prose-xs`, which Tailwind Typography does not define — the class only ever matched four hand-rolled rules in app.css, all scoped to `ul`. Every surface on that default (script and flow descriptions, flow-graph notes, markdown job results) therefore rendered `
      ` with Preflight's `list-style: none` and no typography at all: no numbers, no heading or paragraph rhythm. Route the default through the shared `markdownProse` stacks instead, and cut the app.css list rules down to the dash glyph so ordered and unordered lists share Tailwind Typography's indentation and rhythm. Fixes #10971 Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01S6G5gDXJnm6uqch4uCPkPE * fix(frontend): address review nits on the markdown prose fix - default `GfmMarkdown` to the `sm` stack rather than `xs`: the AI-agent tool Message pane takes the default and has no ancestor font size, so `xs` left it smaller than its own label. The group note, whose wrapper is `text-2xs`, opts down explicitly. - regenerate `static/tailwind_full.css`, which raw apps are served and which still carried the deleted list rules. - correct the marker-color rationale: the typography config already maps markers to tertiary, so the rule steps them up rather than rescuing them. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01S6G5gDXJnm6uqch4uCPkPE * fix(frontend): make the note color override an arbitrary value `text-inherit` is not generated: this config replaces the Tailwind color palette outright and defines no `inherit` key, so `[&_*]:!text-inherit` compiled to nothing and notes still rendered in the prose stack's `text-primary`. Verified in the browser: a yellow note's list items now compute to `text-yellow-900`, matching the wrapper and the edit-mode textarea, in both themes. Also drop the `static/tailwind_full.css` regeneration. That file was generated with tailwind 3.4.1 against a config predating the typography theme overrides; rebuilding it today sweeps in 250KB of unrelated churn and would flip every raw app's `.prose` palette from stock gray to Windmill tokens. Its staleness predates this PR and is its own change. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01S6G5gDXJnm6uqch4uCPkPE --------- Co-authored-by: Claude Opus 5 (1M context) --- frontend/src/lib/assets/app.css | 23 ++++++------------- .../src/lib/components/DisplayResult.svelte | 3 ++- .../src/lib/components/GfmMarkdown.svelte | 7 +++--- .../lib/components/graph/GroupNoteArea.svelte | 6 ++--- .../src/lib/components/graph/noteColors.ts | 9 ++++++++ .../graph/renderers/nodes/NoteNode.svelte | 4 +++- frontend/src/lib/components/markdownProse.ts | 4 ++-- 7 files changed, 29 insertions(+), 27 deletions(-) diff --git a/frontend/src/lib/assets/app.css b/frontend/src/lib/assets/app.css index 760f4c0261..f6dc648234 100644 --- a/frontend/src/lib/assets/app.css +++ b/frontend/src/lib/assets/app.css @@ -209,33 +209,24 @@ U+1fac6, U+1fae0-1fae6, U+1fae8-1faea, U+1faef-1faf8; } - .prose-xs ul { - margin-top: 0.5rem; - list-style-type: '- '; - padding-left: 1.5rem; - } - + /* Bullets read as a dash rather than a disc. Only the glyph is overridden: + indentation and vertical rhythm stay with Tailwind Typography so ordered + and unordered lists line up with each other. */ .prose ul { - margin-top: 1.5rem; list-style-type: '- '; - padding-left: 3rem; } - /* The '- ' list markers, horizontal rules and blockquote bars otherwise - fall through to Tailwind Typography's default bullet/border colors, which - are nearly invisible on dark backgrounds (e.g. the AI chat). Use - theme-aware tokens so they stay readable in both light and dark mode. */ - .prose-xs ul > li::marker, - .prose ul > li::marker { + /* List markers, horizontal rules and blockquote bars take the tertiary/light + tokens the typography config maps them to, which is too faint to read on + the denser markdown surfaces (e.g. the AI chat). Step them up one. */ + .prose :is(ul, ol) > li::marker { color: rgb(var(--color-text-secondary)); } - .prose-xs hr, .prose hr { border-top-color: rgb(var(--color-border-normal)); } - .prose-xs blockquote, .prose blockquote { border-left-color: rgb(var(--color-border-normal)); } diff --git a/frontend/src/lib/components/DisplayResult.svelte b/frontend/src/lib/components/DisplayResult.svelte index 8208fb193e..47bafa4d5a 100644 --- a/frontend/src/lib/components/DisplayResult.svelte +++ b/frontend/src/lib/components/DisplayResult.svelte @@ -32,6 +32,7 @@ import Alert from './common/alert/Alert.svelte' import AutoDataTable from './table/AutoDataTable.svelte' import Markdown from 'svelte-exmarkdown' + import { markdownProse } from './markdownProse' import Toggle from './Toggle.svelte' import FileDownload from './common/fileDownload/FileDownload.svelte' @@ -1229,7 +1230,7 @@
    {:else if !forceJson && resultKind === 'markdown'} -
    +
    {:else if largeObject || hasBigInt} diff --git a/frontend/src/lib/components/GfmMarkdown.svelte b/frontend/src/lib/components/GfmMarkdown.svelte index 50fa3cda45..7a30490ba9 100644 --- a/frontend/src/lib/components/GfmMarkdown.svelte +++ b/frontend/src/lib/components/GfmMarkdown.svelte @@ -6,12 +6,11 @@ interface Props { md: string noPadding?: boolean - /** Shared prose stack to render with. Omitted keeps the legacy `prose-xs`, - * which the flow-graph notes are laid out against. */ + /** Shared prose stack to render with. */ prose?: MarkdownProseSize } - let { md, noPadding, prose }: Props = $props() + let { md, noPadding, prose = 'sm' }: Props = $props() // Rendering markdown turns `![](url)` into a real ``, i.e. a request. On the // public replay page the source is a recording from an arbitrary origin and the @@ -21,7 +20,7 @@ let asPlainText = $derived(isOfflineReplay()) -
    +
    {#if asPlainText}

    {md}

    {:else} diff --git a/frontend/src/lib/components/graph/GroupNoteArea.svelte b/frontend/src/lib/components/graph/GroupNoteArea.svelte index 1b4562e8d0..ced0f4deae 100644 --- a/frontend/src/lib/components/graph/GroupNoteArea.svelte +++ b/frontend/src/lib/components/graph/GroupNoteArea.svelte @@ -1,7 +1,7 @@