diff --git a/.gitignore b/.gitignore index 6d49c8d5..4f3a86b3 100644 --- a/.gitignore +++ b/.gitignore @@ -47,6 +47,9 @@ template-parts/rust/target/ # ─── Rust ───────────────────────────────────────────────────────────────────── target/ + +# Python bytecode from the script test harness +__pycache__/ target/quality/ # ─── Coverage & Profiling ─────────────────────────────────────────────────────────── diff --git a/crates/gitforge-api/src/middleware.rs b/crates/gitforge-api/src/middleware.rs index 80542172..1ad25048 100644 --- a/crates/gitforge-api/src/middleware.rs +++ b/crates/gitforge-api/src/middleware.rs @@ -14,6 +14,18 @@ use std::sync::Arc; /// Paths that don't require authentication const PUBLIC_PATHS: &[&str] = &["/health", "/metrics", "/swagger-ui", "/api-docs", "/docs"]; +/// Telemetry paths (`/metrics`, `/dashboard`) are public only when the +/// operator opted in: `GITFORGE_PUBLIC_TELEMETRY=1`. On a multi-tenant +/// instance the aggregate counters leak activity to anonymous parties, so +/// the default is authenticated access. `/health` stays public either way +/// — a liveness probe carries no instance data (F5). +fn telemetry_is_public(raw: Option<&str>) -> bool { + matches!(raw, Some(v) if v.trim() == "1") +} + +/// Paths that are public only when GITFORGE_PUBLIC_TELEMETRY=1. +const TELEMETRY_PATHS: &[&str] = &["/metrics", "/dashboard"]; + /// Error response for auth failures #[derive(Debug, serde::Serialize)] pub struct AuthErrorResponse { @@ -30,9 +42,15 @@ pub async fn auth_middleware( ) -> Response { let path = request.uri().path().to_string(); - // Skip auth for public paths + // Skip auth for public paths. Telemetry paths fall back to + // authenticated access unless GITFORGE_PUBLIC_TELEMETRY=1. + let telemetry_public = + telemetry_is_public(std::env::var("GITFORGE_PUBLIC_TELEMETRY").ok().as_deref()); for public_path in PUBLIC_PATHS { if path.starts_with(public_path) { + if !telemetry_public && TELEMETRY_PATHS.iter().any(|t| path.starts_with(t)) { + break; + } return next.run(request).await; } } @@ -144,6 +162,28 @@ mod tests { assert!(!PUBLIC_PATHS.iter().any(|p| "/repos".starts_with(p))); } + #[test] + fn test_telemetry_publicity_gate() { + // Decision core: explicit opt-in only; unset, empty, or any other + // value keeps /metrics and /dashboard behind authentication (F5). + assert!(telemetry_is_public(Some("1"))); + assert!(!telemetry_is_public(Some("0"))); + assert!(!telemetry_is_public(Some("true"))); + assert!(!telemetry_is_public(Some(""))); + assert!(telemetry_is_public(Some(" 1 "))); // trim-tolerant opt-in + assert!(!telemetry_is_public(None)); + } + + #[test] + fn test_telemetry_paths_cover_both_surfaces() { + // /dashboard is not in PUBLIC_PATHS (it has its own route merge), + // so the authenticated fallback must cover it explicitly. + assert!(PUBLIC_PATHS.iter().any(|p| "/metrics".starts_with(p))); + assert!(!PUBLIC_PATHS.iter().any(|p| "/dashboard".starts_with(p))); + assert!(TELEMETRY_PATHS.contains(&"/dashboard")); + assert!(TELEMETRY_PATHS.contains(&"/metrics")); + } + #[test] fn test_auth_error_response() { let response = auth_error_response("test_error", "test message"); @@ -293,6 +333,31 @@ mod tests { assert!(PUBLIC_PATHS.contains(&"/docs")); } + #[test] + fn test_middleware_logic_telemetry_fallback() { + // Replicates the middleware branch: telemetry paths are served + // only when the gate is on; every other public path is always + // served; non-public paths never hit the skip. + let served = |path: &str, telemetry_public: bool| -> bool { + for public_path in PUBLIC_PATHS { + if path.starts_with(public_path) { + if !telemetry_public && TELEMETRY_PATHS.iter().any(|t| path.starts_with(t)) { + break; + } + return true; + } + } + false + }; + assert!(served("/health", false)); + assert!(served("/swagger-ui", false)); + assert!(served("/metrics", true)); + // /dashboard is not in PUBLIC_PATHS, so the skip-branch never fires + // for it — it reaches auth regardless of the telemetry gate. + assert!(!served("/dashboard", false)); + assert!(!served("/dashboard", true)); + } + #[test] fn test_authenticated_user_type_equivalence() { let claims = Claims { diff --git a/crates/gitforge-api/src/routes/ci.rs b/crates/gitforge-api/src/routes/ci.rs index 0a1f9396..417eaf62 100644 --- a/crates/gitforge-api/src/routes/ci.rs +++ b/crates/gitforge-api/src/routes/ci.rs @@ -100,7 +100,7 @@ pub fn ci_routes() -> Router { .route("/jobs", post(submit_job)) } -fn can_manage_jobs(claims: &Claims) -> bool { +pub(crate) fn can_manage_jobs(claims: &Claims) -> bool { matches!(claims.role.as_str(), "admin" | "maintainer") } @@ -547,7 +547,13 @@ async fn trigger_pipeline_run( }; match client - .trigger(pipeline.repo_id, &revision, None, &commit) + .trigger( + pipeline.repo_id, + &revision, + None, + &commit, + Some(pipeline.id), + ) .await { Ok(Some(pipeline_run_id)) => ( @@ -715,7 +721,13 @@ async fn rerun_pipeline_run( }; match client - .trigger(run.repo_id, &run.commit_hash, None, &commit) + .trigger( + run.repo_id, + &run.commit_hash, + None, + &commit, + Some(run.pipeline_id), + ) .await { Ok(Some(pipeline_run_id)) => ( diff --git a/crates/gitforge-api/src/routes/runners.rs b/crates/gitforge-api/src/routes/runners.rs index 07d58f7b..14edc2f1 100644 --- a/crates/gitforge-api/src/routes/runners.rs +++ b/crates/gitforge-api/src/routes/runners.rs @@ -109,10 +109,24 @@ pub fn public_runner_routes( } /// List runners +/// +/// Runner inventory (names, capacity, heartbeat) is operational data; only +/// admin/maintainer roles may read it. Any-authenticated listing let a +/// plain developer enumerate the fleet. async fn list_runners( - _user: AuthenticatedUser, + user: AuthenticatedUser, Extension(pool): Extension>, ) -> impl IntoResponse { + if !crate::routes::ci::can_manage_jobs(&user.claims) { + return ( + StatusCode::FORBIDDEN, + Json(serde_json::json!({ + "error": "forbidden", + "message": "runner administration requires maintainer role" + })), + ) + .into_response(); + } match RunnerQueries::list(&pool).await { Ok(runners) => { let response: Vec = @@ -214,11 +228,24 @@ async fn register_runner( } /// Get runner +/// +/// Same role gate as `list_runners` — a single runner's row carries the +/// same operational detail the list does. async fn get_runner( - _user: AuthenticatedUser, + user: AuthenticatedUser, Extension(pool): Extension>, Path(id): Path, ) -> impl IntoResponse { + if !crate::routes::ci::can_manage_jobs(&user.claims) { + return ( + StatusCode::FORBIDDEN, + Json(serde_json::json!({ + "error": "forbidden", + "message": "runner administration requires maintainer role" + })), + ) + .into_response(); + } tracing::debug!("get runner: {}", id); match Uuid::parse_str(&id) { diff --git a/crates/gitforge-api/src/routes/webhook.rs b/crates/gitforge-api/src/routes/webhook.rs index 3a75e55d..27c9f57d 100644 --- a/crates/gitforge-api/src/routes/webhook.rs +++ b/crates/gitforge-api/src/routes/webhook.rs @@ -161,10 +161,12 @@ impl CiTriggerClient { branch: &str, old_commit_hash: Option<&str>, commit_hash: &str, + pipeline_id: Option, ) -> Result, String> { let old_hash = old_commit_hash - .filter(|hash| !hash.is_empty()) - .unwrap_or("0000000000000000000000000000000000000000"); + .filter(|hash| !hash.is_empty() && !gitforge_common::is_zero_hash(hash)) + // git's fixed ref-deletion sentinel: all-zero SHA-1 length. + .map_or_else(|| "0".repeat(40), str::to_string); let response = self .client // The configured value is validated at startup, but never reaches @@ -176,7 +178,13 @@ impl CiTriggerClient { "ref_name": branch, "old_hash": old_hash, "new_hash": commit_hash, - "working_dir": null + "working_dir": null, + // Without the id the consumer falls back to the repo's + // cache/persisted/default definition: two pipelines on one + // commit could not both run, and an API-uploaded config was + // bypassed when the revision had no committed file + // (G1/G2, docs/planning/PRODUCTION_VALIDATION_20261102.md). + "pipeline_id": pipeline_id.map(|id| id.to_string()), })) .send() .await @@ -279,6 +287,7 @@ async fn trigger_pipeline( &payload.branch, payload.old_commit_hash.as_deref(), &payload.commit_hash, + None, ) .await { @@ -358,13 +367,14 @@ async fn trigger_pipeline( // Persist the run before queueing so scheduler job foreign keys // and recovery have a durable parent record. - let run = gitforge_db::models::PipelineRun::new( + let mut run = gitforge_db::models::PipelineRun::new( pipeline.id, repo_id, "webhook".to_string(), payload.commit_hash.clone(), ); let run_id = run.id; + run.ref_name = Some(payload.branch.clone()); if let Err(error) = PipelineRunQueries::create(&pool, &run).await { tracing::error!(%error, %pipeline_id, "failed to persist webhook pipeline run"); return ( diff --git a/crates/gitforge-api/tests/admin_routes.rs b/crates/gitforge-api/tests/admin_routes.rs index 5b66af62..45cb6aa6 100644 --- a/crates/gitforge-api/tests/admin_routes.rs +++ b/crates/gitforge-api/tests/admin_routes.rs @@ -226,7 +226,11 @@ async fn runner_listing_requires_auth_and_shows_registered_runners() { let (status, _) = get_json(f.app.clone(), "/api/runners", None).await; assert_eq!(status, StatusCode::UNAUTHORIZED); - let (status, body) = get_json(f.app.clone(), "/api/runners", Some(&f.developer_token)).await; + // Runner inventory is maintainer/admin-only (2026-11-02 authz audit F3). + let (status, _) = get_json(f.app.clone(), "/api/runners", Some(&f.developer_token)).await; + assert_eq!(status, StatusCode::FORBIDDEN); + + let (status, body) = get_json(f.app.clone(), "/api/runners", Some(&f.admin_token)).await; assert_eq!(status, StatusCode::OK); let runners = body.as_array().unwrap(); assert_eq!(runners.len(), 1); @@ -259,12 +263,20 @@ async fn get_runner_validates_the_id_and_missing_rows() { let runner = Runner::new("visible-runner".to_string(), RunnerType::BareMetal, 8); RunnerQueries::create(&f.pool, &runner).await.unwrap(); - let (status, body) = get_json( + // Single-runner reads carry the same maintainer/admin gate. + let (status, _) = get_json( f.app.clone(), &format!("/api/runners/{}", runner.id), Some(&f.developer_token), ) .await; + assert_eq!(status, StatusCode::FORBIDDEN); + let (status, body) = get_json( + f.app.clone(), + &format!("/api/runners/{}", runner.id), + Some(&f.admin_token), + ) + .await; assert_eq!(status, StatusCode::OK); assert_eq!(body["type"], "bare_metal"); assert_eq!(body["capacity"], 8); diff --git a/crates/gitforge-api/tests/ci_routes.rs b/crates/gitforge-api/tests/ci_routes.rs index 376d953d..c82b5d57 100644 --- a/crates/gitforge-api/tests/ci_routes.rs +++ b/crates/gitforge-api/tests/ci_routes.rs @@ -1749,4 +1749,12 @@ async fn run_trigger_delegation_ladder_relays_the_orchestrator_answers() { assert_eq!(seen[0].body["ref_name"], commit); assert_eq!(seen[0].body["new_hash"], commit); assert_eq!(seen[0].body["old_hash"], "0".repeat(40)); + // G1: the delegated trigger must carry the requested pipeline id, or + // the orchestrator binds the run to whichever definition its cache + // resolves and two pipelines on one commit cannot both run. + assert_eq!( + seen[0].body["pipeline_id"], + pipeline_id.to_string(), + "trigger payload must name the pipeline row being run" + ); } diff --git a/crates/gitforge-api/tests/integration.rs b/crates/gitforge-api/tests/integration.rs index 5abac44a..9f7068d2 100644 --- a/crates/gitforge-api/tests/integration.rs +++ b/crates/gitforge-api/tests/integration.rs @@ -1065,6 +1065,7 @@ async fn test_api_artifacts_are_scoped_to_job_repository_owner() { finished_at: Some(chrono::Utc::now()), created_at: chrono::Utc::now(), error: None, + ref_name: None, }, ) .await @@ -1133,6 +1134,11 @@ async fn test_api_runners_endpoint() { gitforge_db::queries::UserQueries::create(&pool, &user) .await .unwrap(); + // Runner reads are maintainer/admin-only (2026-11-02 authz audit F3); + // the middleware re-resolves the persisted role, so the row carries it. + gitforge_db::queries::UserQueries::set_role(&pool, user.id, "admin") + .await + .unwrap(); let server = ApiServer::new("test-secret", pool); let app = server.into_router(); @@ -1171,6 +1177,10 @@ async fn test_api_runner_registration_adopts_existing_name() { gitforge_db::queries::UserQueries::create(&pool, &user) .await .unwrap(); + // Runner reads are maintainer/admin-only (F3); persist the role. + gitforge_db::queries::UserQueries::set_role(&pool, user.id, "admin") + .await + .unwrap(); let server = ApiServer::with_registration_token( "test-secret", @@ -1260,6 +1270,10 @@ async fn test_api_runner_list_derives_status_from_heartbeat() { gitforge_db::queries::UserQueries::create(&pool, &user) .await .unwrap(); + // Runner reads are maintainer/admin-only (F3); persist the role. + gitforge_db::queries::UserQueries::set_role(&pool, user.id, "admin") + .await + .unwrap(); // A registry row left online whose heartbeat died long ago — the shape // a scheduler restart leaves behind. @@ -2237,6 +2251,7 @@ async fn test_api_job_control_enforces_repository_ownership_and_persists_cancel( finished_at: None, created_at: chrono::Utc::now(), error: None, + ref_name: None, }, ) .await @@ -2384,6 +2399,7 @@ async fn test_api_pipeline_runs_with_jobs() { finished_at: None, created_at: chrono::Utc::now(), error: None, + ref_name: None, }; gitforge_db::queries::PipelineRunQueries::create(&pool, &run) .await diff --git a/crates/gitforge-api/tests/runners_routes.rs b/crates/gitforge-api/tests/runners_routes.rs index 55986211..636d1858 100644 --- a/crates/gitforge-api/tests/runners_routes.rs +++ b/crates/gitforge-api/tests/runners_routes.rs @@ -257,7 +257,7 @@ async fn get_runner_returns_wire_contract_and_404s() { f.app.clone(), "GET", &format!("/api/runners/{id}"), - Some(&f.developer_token), + Some(&f.admin_token), None, ) .await; @@ -275,7 +275,7 @@ async fn get_runner_returns_wire_contract_and_404s() { f.app.clone(), "GET", &format!("/api/runners/{}", uuid::Uuid::new_v4()), - Some(&f.developer_token), + Some(&f.admin_token), None, ) .await; @@ -286,7 +286,7 @@ async fn get_runner_returns_wire_contract_and_404s() { f.app.clone(), "GET", "/api/runners/not-a-uuid", - Some(&f.developer_token), + Some(&f.admin_token), None, ) .await; @@ -434,7 +434,9 @@ async fn list_runners_exposes_the_registered_fleet() { assert_eq!(status, StatusCode::CREATED); } - let (status, fleet) = request_json( + // Runner inventory is operational data: a plain developer is refused + // and the same request succeeds with a maintainer/admin token. + let (dev_status, dev_body) = request_json( f.app.clone(), "GET", "/api/runners", @@ -442,6 +444,17 @@ async fn list_runners_exposes_the_registered_fleet() { None, ) .await; + assert_eq!(dev_status, StatusCode::FORBIDDEN, "{dev_body}"); + assert_eq!(dev_body["error"], "forbidden"); + + let (status, fleet) = request_json( + f.app.clone(), + "GET", + "/api/runners", + Some(&f.admin_token), + None, + ) + .await; assert_eq!(status, StatusCode::OK, "{fleet}"); let fleet = fleet.as_array().unwrap(); assert_eq!(fleet.len(), 2); diff --git a/crates/gitforge-ci/src/engine.rs b/crates/gitforge-ci/src/engine.rs index 98a0c0a1..060cc62d 100644 --- a/crates/gitforge-ci/src/engine.rs +++ b/crates/gitforge-ci/src/engine.rs @@ -119,6 +119,59 @@ impl CiEngineState { } } +/// Enforce the job-image allowlist when one is configured. +/// +/// `GITFORGE_ALLOWED_IMAGE_REGISTRIES` (comma-separated registry prefixes, +/// e.g. `localhost:5001,docker.io/library`) restricts which registries a +/// pipeline-authored `image:` may reference; unset means unrestricted, so +/// existing single-host deployments keep working. Without this gate a +/// pipeline author can make the runner pull — and execute steps from — any +/// registry the runner's daemon can reach. An unpinned reference is +/// additionally refused when the allowlist is active: an allowlisted +/// registry with a floating tag still lets content change under the same +/// reference, so enforcement requires an immutable reference (digest). +fn validate_job_images(pipeline: &PipelineDefinition, run_id: PipelineRunId) -> Result<()> { + let raw = std::env::var("GITFORGE_ALLOWED_IMAGE_REGISTRIES") + .ok() + .filter(|v| !v.trim().is_empty()); + validate_job_images_with(pipeline, run_id, raw.as_deref()) +} + +/// The decision core: takes the allowlist as data so tests inject it +/// without mutating process-global state. +fn validate_job_images_with( + pipeline: &PipelineDefinition, + run_id: PipelineRunId, + allowlist_raw: Option<&str>, +) -> Result<()> { + let Some(raw) = allowlist_raw else { + return Ok(()); + }; + let allowed: Vec<&str> = raw + .split(',') + .map(str::trim) + .filter(|s| !s.is_empty()) + .collect(); + for job in &pipeline.jobs { + let image = job.image.trim(); + let pinned = image.contains('@'); + let ok_registry = allowed.iter().any(|prefix| { + image == *prefix + || image.starts_with(&format!("{prefix}/")) + || image.starts_with(&format!("{prefix}:")) + }); + if !ok_registry || !pinned { + return Err(gitforge_common::Error::invalid_input(format!( + "run {run_id}: job '{}' image '{image}' is not an allowlisted, digest-pinned \ + reference (allowlist: {allowed:?}); every image must resolve to a fixed \ + digest under an allowed registry", + job.name + ))); + } + } + Ok(()) +} + /// Extract the `CI_CHANGE_*` variables a completed job published. /// /// The `detect-changes` job prints its classification as `KEY=value` lines on @@ -164,31 +217,24 @@ pub async fn eval_skip_if(skip_if: &Option, env: &HashMap = env - .iter() - .filter(|(k, _)| k.starts_with("CI_CHANGE_")) - .map(|(k, v)| format!("{k}={v}")) - .collect(); - // Add CI_CHANGE_FILE_COUNT for convenience (count of all changed files) - let file_count = env.get("CI_CHANGE_FILE_COUNT"); - let combined_env = if bash_env.is_empty() && file_count.is_none() { - format!("set -e; {expr}") - } else { - let env_str = bash_env.join(" ") - + &format!( - " CI_CHANGE_FILE_COUNT={}", - file_count.unwrap_or(&"0".to_string()) - ); - format!("export {env_str}; set -e; {expr}") - }; + // The environment is delivered through Command::envs below - never + // interpolated into the script text. A receipt-controlled value that + // reached the script as `export KEY=value` could execute arbitrary + // shell (via ;, backticks, or $() in the value). + let combined_env = format!("set -e; {expr}"); // Timeout: 10s max to prevent a malicious expression from stalling the engine let result = timeout( Duration::from_secs(10), Command::new("bash") .arg("-c") .arg(&combined_env) - .envs(env.iter().filter(|(k, _)| !k.starts_with("CI_CHANGE_"))) + .envs(env.iter()) + // CI_CHANGE_FILE_COUNT is always defined for expressions, even + // when the detect-changes job did not report it. + .env( + "CI_CHANGE_FILE_COUNT", + env.get("CI_CHANGE_FILE_COUNT").map_or("0", String::as_str), + ) .output(), ) .await; @@ -292,6 +338,8 @@ impl CiEngine { pipeline: PipelineDefinition, run_id: PipelineRunId, ) -> Result { + validate_job_images(&pipeline, run_id)?; + // Build DAG from pipeline let graph = DagBuilder::build(&pipeline, run_id)?; @@ -700,6 +748,107 @@ mod tests { use gitforge_common::PipelineId; use std::collections::HashMap; + fn job_with_image(name: &str, image: &str) -> JobDefinition { + JobDefinition { + name: name.to_string(), + image: image.to_string(), + needs: vec![], + env: HashMap::new(), + steps: vec![StepDefinition { + name: "run".to_string(), + run: "true".to_string(), + env: None, + working_directory: None, + condition: None, + }], + timeout: None, + retry: None, + skip_if: None, + volumes: Vec::new(), + cache: None, + lane: Default::default(), + fault: None, + } + } + + #[test] + fn test_image_allowlist_unset_permits_anything() { + let pipeline = PipelineDefinition { + name: "p".to_string(), + version: "1".to_string(), + trigger_on: vec![TriggerType::Push], + environment: HashMap::new(), + jobs: vec![job_with_image("j", "random-registry.example/img:latest")], + }; + assert!(validate_job_images_with(&pipeline, PipelineRunId::new(), None).is_ok()); + } + + #[test] + fn test_image_allowlist_refuses_non_allowlisted_registry() { + let pipeline = PipelineDefinition { + name: "p".to_string(), + version: "1".to_string(), + trigger_on: vec![TriggerType::Push], + environment: HashMap::new(), + jobs: vec![job_with_image("j", "evil.example/img@sha256:0123")], + }; + let error = + validate_job_images_with(&pipeline, PipelineRunId::new(), Some("localhost:5001")) + .unwrap_err(); + assert!(error.to_string().contains("evil.example/img"), "{error}"); + } + + #[test] + fn test_image_allowlist_refuses_unpinned_reference() { + let pipeline = PipelineDefinition { + name: "p".to_string(), + version: "1".to_string(), + trigger_on: vec![TriggerType::Push], + environment: HashMap::new(), + jobs: vec![job_with_image("j", "localhost:5001/ci-rust:16")], + }; + let error = + validate_job_images_with(&pipeline, PipelineRunId::new(), Some("localhost:5001")) + .unwrap_err(); + assert!(error.to_string().contains("digest-pinned"), "{error}"); + } + + #[test] + fn test_image_allowlist_accepts_digest_pinned_allowlisted() { + let pipeline = PipelineDefinition { + name: "p".to_string(), + version: "1".to_string(), + trigger_on: vec![TriggerType::Push], + environment: HashMap::new(), + jobs: vec![job_with_image( + "j", + "localhost:5001/ci-rust@sha256:0123456789abcdef", + )], + }; + assert!( + validate_job_images_with(&pipeline, PipelineRunId::new(), Some("localhost:5001")) + .is_ok() + ); + } + + #[test] + fn test_image_allowlist_prefix_boundary_is_exact() { + // A registry "5001" must not match "localhost:50010/..." — prefix + // matching extends only at '/' or ':' after the full prefix. + let pipeline = PipelineDefinition { + name: "p".to_string(), + version: "1".to_string(), + trigger_on: vec![TriggerType::Push], + environment: HashMap::new(), + jobs: vec![job_with_image("j", "localhost:50010/evil@sha256:0123")], + }; + assert!( + validate_job_images_with(&pipeline, PipelineRunId::new(), Some("localhost:5001")) + .is_err(), + "a longer port must not match a shorter allowlist prefix" + ); + } + fn make_pipeline() -> PipelineDefinition { PipelineDefinition { name: "test".to_string(), @@ -1432,6 +1581,45 @@ mod tests { assert!(extract_change_env("{}").is_empty()); } + /// A receipt-controlled `CI_CHANGE_*` value must reach the expression + /// as a value, never as shell code. The detect-changes job is job + /// output; interpolating it into the script text would let a value + /// like `1; touch /tmp/pwned` execute in the CI service. + #[tokio::test] + async fn a_change_env_value_cannot_execute_as_shell_code() { + let marker = std::env::temp_dir().join("gitforge-skipif-injection-regression"); + let _ = std::fs::remove_file(&marker); + let receipt = serde_json::json!({ + "step_results": [{ + "stdout": format!( + "CI_CHANGE_INCLUDE_CODE=1; touch {}\n", + marker.display() + ) + }] + }) + .to_string(); + let env = extract_change_env(&receipt); + let expr = Some(r#"[[ "$CI_CHANGE_INCLUDE_CODE" == "1" ]]"#.to_string()); + let should_run = eval_skip_if(&expr, &env).await.unwrap(); + assert!( + !marker.exists(), + "the receipt value executed as shell code in the CI service" + ); + // The value is still readable as a variable: delivered inertly, + // not silently dropped. + assert!(!should_run, "the expression must see the literal value"); + let _ = std::fs::remove_file(&marker); + } + + /// `CI_CHANGE_FILE_COUNT` is always defined for expressions, even when + /// the detect-changes job reported nothing. + #[tokio::test] + async fn a_change_file_count_is_always_defined() { + let env = HashMap::new(); + let expr = Some(r#"[[ "$CI_CHANGE_FILE_COUNT" == "0" ]]"#.to_string()); + assert!(eval_skip_if(&expr, &env).await.unwrap()); + } + /// The defect this guards: an unmerged receipt leaves the expression /// environment empty, every condition evaluates false, and the engine /// prunes the gate that should have run. diff --git a/crates/gitforge-db/src/connection.rs b/crates/gitforge-db/src/connection.rs index 83265ef9..fdf7b054 100644 --- a/crates/gitforge-db/src/connection.rs +++ b/crates/gitforge-db/src/connection.rs @@ -432,6 +432,7 @@ impl Pool { finished_at TEXT, created_at TEXT NOT NULL, error TEXT, + ref_name TEXT, FOREIGN KEY (pipeline_id) REFERENCES pipelines(id), FOREIGN KEY (repo_id) REFERENCES repositories(id) ) @@ -447,7 +448,10 @@ impl Pool { // process log, which is unreadable after the fact. SQLite has no // portable IF NOT EXISTS form for ADD COLUMN, so tolerate only the // known duplicate-column case. - for statement in ["ALTER TABLE pipeline_runs ADD COLUMN error TEXT"] { + for statement in [ + "ALTER TABLE pipeline_runs ADD COLUMN error TEXT", + "ALTER TABLE pipeline_runs ADD COLUMN ref_name TEXT", + ] { if let Err(error) = sqlx::query(statement).execute(&self.pool).await { if !error.to_string().contains("duplicate column name") { return Err(Error::database(format!( @@ -488,17 +492,63 @@ impl Pool { )) })?; - // At most one in-flight trigger per (repository, commit): the API - // and the webhook both fire on the same push, and the correlation - // window can answer `queued` to one of them while the other is still - // cloning — without this index every retry minted another duplicate - // run for the same commit. Completed and failed rows are excluded so - // an explicit re-trigger (manual re-run of an old commit) stays - // possible. + // Migration for G1 (2026-11-02): an explicit pipeline trigger names + // the row to run. The dedup key must include that id, or a second + // pipeline triggered on the same commit dedups onto the first's + // row and never runs. Existing rows read as push triggers (NULL = + // no explicit pipeline), which keeps the old key semantics. + let has_pipeline_id: Option = sqlx::query_scalar( + "SELECT name FROM pragma_table_info('pipeline_trigger_requests') \ + WHERE name = 'pipeline_id'", + ) + .fetch_optional(&self.pool) + .await + .map_err(|e| { + Error::database(format!( + "failed to inspect pipeline_trigger_requests columns: {e}" + )) + })?; + if has_pipeline_id.is_none() { + sqlx::query("ALTER TABLE pipeline_trigger_requests ADD COLUMN pipeline_id TEXT") + .execute(&self.pool) + .await + .map_err(|e| { + Error::database(format!( + "failed to add pipeline_id to pipeline_trigger_requests: {e}" + )) + })?; + // The old active-triggers index keyed on (repo_id, new_hash) + // only; drop it so the widened replacement can be created. + sqlx::query("DROP INDEX IF EXISTS idx_trigger_requests_active") + .execute(&self.pool) + .await + .map_err(|e| { + Error::database(format!( + "failed to drop superseded trigger dedup index: {e}" + )) + })?; + } + + // At most one in-flight trigger per (repository, commit, pipeline): + // the API and the webhook both fire on the same push, and the + // correlation window can answer `queued` to one of them while the + // other is still cloning — without this index every retry minted + // another duplicate run for the same commit. Completed and failed + // rows are excluded so an explicit re-trigger (manual re-run of an + // old commit) stays possible. The pipeline id is part of the key so + // two explicit pipelines on one commit can both run (G1). SQLite + // treats NULLs as distinct in a UNIQUE index, so the key uses a + // COALESCE expression: push triggers (NULL pipeline) dedup on + // (repo_id, hash, '') exactly as the pre-G1 (repo_id, hash) index + // did, while explicit pipelines dedup per id. sqlx::query( r#" CREATE UNIQUE INDEX IF NOT EXISTS idx_trigger_requests_active - ON pipeline_trigger_requests(repo_id, new_hash) + ON pipeline_trigger_requests( + repo_id, + new_hash, + COALESCE(pipeline_id, '') + ) WHERE status IN ('pending', 'processing') "#, ) diff --git a/crates/gitforge-db/src/models/pipeline.rs b/crates/gitforge-db/src/models/pipeline.rs index 593043aa..a403ee35 100644 --- a/crates/gitforge-db/src/models/pipeline.rs +++ b/crates/gitforge-db/src/models/pipeline.rs @@ -77,6 +77,9 @@ pub struct PipelineRun { /// finalized the run. `None` for successes and for runs that predate /// the column. pub error: Option, + /// Git ref the run was triggered for. `None` for rows that predate the + /// column; supersede filtering skips such runs rather than guessing. + pub ref_name: Option, } impl PipelineRun { @@ -98,6 +101,7 @@ impl PipelineRun { finished_at: None, created_at: Utc::now(), error: None, + ref_name: None, } } diff --git a/crates/gitforge-db/src/models/trigger_request.rs b/crates/gitforge-db/src/models/trigger_request.rs index 214c7783..6e587463 100644 --- a/crates/gitforge-db/src/models/trigger_request.rs +++ b/crates/gitforge-db/src/models/trigger_request.rs @@ -1,7 +1,7 @@ //! Durable pipeline trigger request model use chrono::{DateTime, Utc}; -use gitforge_common::{PipelineRunId, RepoId}; +use gitforge_common::{PipelineId, PipelineRunId, RepoId}; /// A durably recorded request to run a pipeline for one pushed revision. /// @@ -27,6 +27,11 @@ pub struct PipelineTriggerRequest { pub status: String, /// The run this request produced, once known. pub run_id: Option, + /// The pipeline row an explicit (manual API) trigger asked to run. + /// Push triggers leave this unset; the committed `.gitforge.yml` + /// governs those. Part of the dedup key so two pipelines on one + /// commit can both run (G1). + pub pipeline_id: Option, /// How many times the consumer has attempted this request. pub attempts: i64, /// Terminal failure cause, when `status = failed`. @@ -47,6 +52,7 @@ impl PipelineTriggerRequest { new_hash, status: "pending".to_string(), run_id: None, + pipeline_id: None, attempts: 0, error: None, created_at: now, diff --git a/crates/gitforge-db/src/queries.rs b/crates/gitforge-db/src/queries.rs index 1e6cda89..8213b9ba 100644 --- a/crates/gitforge-db/src/queries.rs +++ b/crates/gitforge-db/src/queries.rs @@ -243,6 +243,9 @@ fn hydrate_pipeline_run(row: sqlx::sqlite::SqliteRow) -> Result Result { + let mut transaction = begin_immediate(pool.pool(), "run supersede").await?; + let stale: Vec = sqlx::query_scalar( + "SELECT id FROM pipeline_runs \ + WHERE repo_id = ? AND pipeline_id = ? AND id != ? \ + AND ref_name = ? \ + AND status IN ('pending', 'running', 'queued') \ + AND created_at < (SELECT created_at FROM pipeline_runs WHERE id = ?)", + ) + .bind(repo_id.to_string()) + .bind(pipeline_id.to_string()) + .bind(keep_run_id.to_string()) + .bind(ref_name) + .bind(keep_run_id.to_string()) + .fetch_all(&mut *transaction) + .await + .map_err(|e| Error::database(format!("failed to list stale runs: {e}")))?; + let mut superseded = 0u64; + for run_id in &stale { + sqlx::query( + "UPDATE jobs SET status = ?, finished_at = COALESCE(finished_at, ?) \ + WHERE pipeline_run_id = ? AND status IN ('pending', 'queued')", + ) + .bind(JobStatus::Cancelled.as_str()) + .bind(Utc::now().to_rfc3339()) + .bind(run_id) + .execute(&mut *transaction) + .await + .map_err(|e| Error::database(format!("failed to cancel superseded jobs: {e}")))?; + sqlx::query( + "UPDATE pipeline_runs SET status = 'cancelled', \ + finished_at = COALESCE(finished_at, ?), \ + error = COALESCE(error, 'superseded by a newer push') \ + WHERE id = ?", + ) + .bind(Utc::now().to_rfc3339()) + .bind(run_id) + .execute(&mut *transaction) + .await + .map_err(|e| Error::database(format!("failed to supersede run: {e}")))?; + superseded += 1; + } + transaction + .commit() + .await + .map_err(|e| Error::database(format!("failed to commit supersede: {e}")))?; + Ok(superseded) + } + /// Delete a pipeline row outright. /// /// Only safe for definitions with no runs: runs reference their @@ -971,8 +1038,8 @@ impl PipelineRunQueries { pub async fn create(pool: &Pool, run: &crate::models::PipelineRun) -> Result<()> { sqlx::query( r#" - INSERT INTO pipeline_runs (id, pipeline_id, repo_id, status, triggered_by, commit_hash, started_at, finished_at, created_at, error) - VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?) + INSERT INTO pipeline_runs (id, pipeline_id, repo_id, status, triggered_by, commit_hash, started_at, finished_at, created_at, error, ref_name) + VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?) "#, ) .bind(run.id.to_string()) @@ -985,6 +1052,7 @@ impl PipelineRunQueries { .bind(run.finished_at.map(|dt| dt.to_rfc3339())) .bind(run.created_at.to_rfc3339()) .bind(&run.error) + .bind(&run.ref_name) .execute(pool.pool()) .await .map_err(|e| Error::database(format!("failed to create pipeline run: {e}")))?; @@ -1353,6 +1421,15 @@ type ExpiredJobRow = ( ); impl JobQueries { + /// Total durable job rows, for the metrics entity gauges. + pub async fn count_all(pool: &Pool) -> Result { + let row = sqlx::query("SELECT COUNT(*) FROM jobs") + .fetch_one(pool.pool()) + .await + .map_err(|e| Error::database(format!("job count failed: {e}")))?; + Ok(row.get::(0)) + } + /// Append a log chunk only when the runner still owns the active lease. /// `None` means the job is missing, terminal, or fenced by another lease. pub async fn append_log_with_lease( @@ -3369,7 +3446,14 @@ impl TriggerRequestQueries { pool: &Pool, request: &crate::models::PipelineTriggerRequest, ) -> Result<(crate::models::PipelineTriggerRequest, bool)> { - if let Some(existing) = Self::find_active(pool, request.repo_id, &request.new_hash).await? { + if let Some(existing) = Self::find_active( + pool, + request.repo_id, + &request.new_hash, + request.pipeline_id, + ) + .await? + { return Ok((existing, false)); } persist_with_retry(|| async { @@ -3378,11 +3462,19 @@ impl TriggerRequestQueries { let existing_id: Option = sqlx::query_scalar( "SELECT id FROM pipeline_trigger_requests \ - WHERE repo_id = ? AND new_hash = ? AND status IN ('pending', 'processing') \ + WHERE repo_id = ? AND new_hash = ? \ + AND COALESCE(pipeline_id, '') = ? \ + AND status IN ('pending', 'processing') \ LIMIT 1", ) .bind(request.repo_id.to_string()) .bind(&request.new_hash) + .bind( + request + .pipeline_id + .map(|id| id.to_string()) + .unwrap_or_default(), + ) .fetch_optional(&mut *transaction) .await .map_err(|e| { @@ -3407,8 +3499,8 @@ impl TriggerRequestQueries { sqlx::query( r#" INSERT INTO pipeline_trigger_requests - (id, repo_id, ref_name, old_hash, new_hash, status, run_id, attempts, error, created_at, updated_at) - VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?) + (id, repo_id, ref_name, old_hash, new_hash, status, run_id, pipeline_id, attempts, error, created_at, updated_at) + VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?) "#, ) .bind(request.id.to_string()) @@ -3418,6 +3510,7 @@ impl TriggerRequestQueries { .bind(&request.new_hash) .bind(&request.status) .bind(request.run_id.map(|id| id.to_string())) + .bind(request.pipeline_id.map(|id| id.to_string())) .bind(request.attempts) .bind(&request.error) .bind(request.created_at.to_rfc3339()) @@ -3440,14 +3533,21 @@ impl TriggerRequestQueries { pool: &Pool, repo_id: RepoId, new_hash: &str, + pipeline_id: Option, ) -> Result> { + // The dedup key mirrors the partial unique index: + // COALESCE(pipeline_id, '') so push triggers (NULL pipeline) dedup + // per (repo, hash) while explicit pipelines dedup per id. let id: Option = sqlx::query_scalar( "SELECT id FROM pipeline_trigger_requests \ - WHERE repo_id = ? AND new_hash = ? AND status IN ('pending', 'processing') \ + WHERE repo_id = ? AND new_hash = ? \ + AND COALESCE(pipeline_id, '') = ? \ + AND status IN ('pending', 'processing') \ ORDER BY created_at DESC LIMIT 1", ) .bind(repo_id.to_string()) .bind(new_hash) + .bind(pipeline_id.map(|id| id.to_string()).unwrap_or_default()) .fetch_optional(pool.pool()) .await .map_err(|e| Error::database(format!("failed to find active trigger request: {e}")))?; @@ -3681,6 +3781,15 @@ fn hydrate_trigger_request( }) .transpose()? .map(gitforge_common::PipelineRunId::from), + pipeline_id: row + .try_get::, _>("pipeline_id") + .map_err(|error| Error::database(format!("invalid pipeline_id: {error}")))? + .map(|id| { + Uuid::parse_str(&id) + .map_err(|error| Error::database(format!("invalid pipeline_id: {error}"))) + }) + .transpose()? + .map(gitforge_common::PipelineId::from), attempts: row .try_get("attempts") .map_err(|error| Error::database(format!("invalid attempts: {error}")))?, @@ -4646,6 +4755,244 @@ mod tests { assert!(was_new); } + /// G1: an explicit pipeline trigger is part of the dedup key. Two + /// pipelines on one commit must both get their own trigger request — + /// keying on (repo, hash) alone made the second one dedup onto the + /// first's row, so it never ran. + #[tokio::test] + async fn test_trigger_request_dedup_is_scoped_per_pipeline() { + let pool = Pool::memory().await.unwrap(); + pool.migrate().await.unwrap(); + + let owner = crate::models::User::new( + "pipeline-owner".to_string(), + "pipeline-owner@example.com".to_string(), + "hash".to_string(), + ); + crate::queries::UserQueries::create(&pool, &owner) + .await + .unwrap(); + let repo = crate::models::Repository::new( + "pipeline-repo".to_string(), + owner.id, + "/git/pipeline-repo".to_string(), + ); + RepoQueries::create(&pool, &repo).await.unwrap(); + + let first_pipeline = gitforge_common::PipelineId::new(); + let second_pipeline = gitforge_common::PipelineId::new(); + + let mut first = crate::models::PipelineTriggerRequest::new( + repo.id, + "refs/heads/main".to_string(), + "0".repeat(40), + "b".repeat(40), + ); + first.pipeline_id = Some(first_pipeline); + let (first_row, was_new) = TriggerRequestQueries::create_or_existing(&pool, &first) + .await + .unwrap(); + assert!(was_new); + assert_eq!(first_row.pipeline_id, Some(first_pipeline)); + + // The same pipeline on the same commit dedups onto its own row. + let mut same_pipeline = crate::models::PipelineTriggerRequest::new( + repo.id, + "refs/heads/main".to_string(), + "0".repeat(40), + "b".repeat(40), + ); + same_pipeline.pipeline_id = Some(first_pipeline); + let (deduped, was_new) = TriggerRequestQueries::create_or_existing(&pool, &same_pipeline) + .await + .unwrap(); + assert!(!was_new); + assert_eq!(deduped.id, first_row.id); + + // A different pipeline on the same commit gets its own row: the + // second pipeline must be able to run on that commit too. + let mut second = crate::models::PipelineTriggerRequest::new( + repo.id, + "refs/heads/main".to_string(), + "0".repeat(40), + "b".repeat(40), + ); + second.pipeline_id = Some(second_pipeline); + let (second_row, was_new) = TriggerRequestQueries::create_or_existing(&pool, &second) + .await + .unwrap(); + assert!(was_new, "a second pipeline must not dedup onto the first"); + assert_ne!(second_row.id, first_row.id); + assert_eq!(second_row.pipeline_id, Some(second_pipeline)); + } + + #[tokio::test] + async fn test_supersede_stale_runs_cancels_older_branch_runs() { + let pool = Pool::memory().await.unwrap(); + pool.migrate().await.unwrap(); + + let owner = crate::models::User::new( + "supersede-owner".to_string(), + "supersede-owner@example.com".to_string(), + "hash".to_string(), + ); + crate::queries::UserQueries::create(&pool, &owner) + .await + .unwrap(); + let repo = crate::models::Repository::new( + "supersede-repo".to_string(), + owner.id, + "/git/supersede-repo".to_string(), + ); + RepoQueries::create(&pool, &repo).await.unwrap(); + let pipeline = crate::models::Pipeline { + id: PipelineId::new(), + repo_id: repo.id, + name: "ci".to_string(), + trigger_type: "push".to_string(), + config: serde_json::json!({}), + created_at: chrono::Utc::now(), + active: true, + }; + PipelineQueries::create(&pool, &pipeline).await.unwrap(); + + // Older queued run on the same branch, with a pending and a queued + // job plus one already-running job. + let mut stale = crate::models::PipelineRun::new( + pipeline.id, + repo.id, + "push".to_string(), + "a".repeat(40), + ); + stale.created_at = Utc::now() - chrono::Duration::hours(1); + stale.ref_name = Some("refs/heads/main".to_string()); + PipelineRunQueries::create(&pool, &stale).await.unwrap(); + let stale_pending = crate::models::Job::new(stale.id, "stale-pending".to_string()); + JobQueries::create(&pool, &stale_pending).await.unwrap(); + let stale_running = crate::models::Job::new(stale.id, "stale-running".to_string()); + JobQueries::create(&pool, &stale_running).await.unwrap(); + JobQueries::update_status(&pool, stale_running.id, "running") + .await + .unwrap(); + + // A succeeded run on the same branch must be untouched (history). + let mut done = crate::models::PipelineRun::new( + pipeline.id, + repo.id, + "push".to_string(), + "b".repeat(40), + ); + done.created_at = Utc::now() - chrono::Duration::hours(2); + PipelineRunQueries::create(&pool, &done).await.unwrap(); + PipelineRunQueries::update_status(&pool, done.id, "succeeded") + .await + .unwrap(); + + // A queued run on a DIFFERENT pipeline must be untouched. + let other_pipeline = crate::models::Pipeline { + id: PipelineId::new(), + repo_id: repo.id, + name: "docs".to_string(), + trigger_type: "push".to_string(), + config: serde_json::json!({}), + created_at: chrono::Utc::now(), + active: true, + }; + PipelineQueries::create(&pool, &other_pipeline) + .await + .unwrap(); + let mut other = crate::models::PipelineRun::new( + other_pipeline.id, + repo.id, + "push".to_string(), + "c".repeat(40), + ); + other.created_at = Utc::now() - chrono::Duration::hours(1); + other.ref_name = Some("refs/heads/main".to_string()); + PipelineRunQueries::create(&pool, &other).await.unwrap(); + + // The new run (the winner). + let mut fresh = crate::models::PipelineRun::new( + pipeline.id, + repo.id, + "push".to_string(), + "d".repeat(40), + ); + fresh.created_at = Utc::now(); + fresh.ref_name = Some("refs/heads/main".to_string()); + PipelineRunQueries::create(&pool, &fresh).await.unwrap(); + + // A queued run on the SAME pipeline but a DIFFERENT branch must be + // untouched — supersede is per (repo, ref, pipeline). + let mut cross_ref = crate::models::PipelineRun::new( + pipeline.id, + repo.id, + "push".to_string(), + "e".repeat(40), + ); + cross_ref.created_at = Utc::now() - chrono::Duration::hours(1); + cross_ref.ref_name = Some("refs/heads/feature".to_string()); + PipelineRunQueries::create(&pool, &cross_ref).await.unwrap(); + + let superseded = PipelineQueries::supersede_stale_runs( + &pool, + repo.id, + "refs/heads/main", + pipeline.id, + fresh.id, + ) + .await + .unwrap(); + assert_eq!(superseded, 1, "only the stale ci run is superseded"); + + let stale_row = PipelineRunQueries::get(&pool, stale.id) + .await + .unwrap() + .unwrap(); + assert_eq!(stale_row.status, "cancelled"); + assert_eq!( + stale_row.error.as_deref(), + Some("superseded by a newer push") + ); + assert!(stale_row.finished_at.is_some()); + + // Pending/queued jobs of the stale run are cancelled; the already + // running job belongs to the runner lifecycle and is left alone. + let pending_row = JobQueries::get(&pool, stale_pending.id) + .await + .unwrap() + .unwrap(); + assert_eq!(pending_row.status, "cancelled"); + let running_row = JobQueries::get(&pool, stale_running.id) + .await + .unwrap() + .unwrap(); + assert_eq!(running_row.status, "running"); + + let cross_ref_row = PipelineRunQueries::get(&pool, cross_ref.id) + .await + .unwrap() + .unwrap(); + assert_eq!( + cross_ref_row.status, "pending", + "a queued run on another branch is not superseded" + ); + + let done_row = PipelineRunQueries::get(&pool, done.id) + .await + .unwrap() + .unwrap(); + assert_eq!(done_row.status, "succeeded", "history is untouched"); + let other_row = PipelineRunQueries::get(&pool, other.id) + .await + .unwrap() + .unwrap(); + assert_eq!( + other_row.status, "pending", + "a different pipeline's run is untouched" + ); + } + #[tokio::test] async fn test_trigger_request_requeue_stale_and_terminal_failure() { let pool = Pool::memory().await.unwrap(); diff --git a/crates/gitforge-events/src/bus.rs b/crates/gitforge-events/src/bus.rs index 8107f3ac..7d839a68 100644 --- a/crates/gitforge-events/src/bus.rs +++ b/crates/gitforge-events/src/bus.rs @@ -238,6 +238,7 @@ mod tests { old_hash: "abc".to_string(), new_hash: "def".to_string(), pusher_id: None, + pipeline_id: None, }), None, None, @@ -277,6 +278,7 @@ mod tests { old_hash: "abc".to_string(), new_hash: "def".to_string(), pusher_id: None, + pipeline_id: None, }), None, None, @@ -306,6 +308,7 @@ mod tests { old_hash: "abc".to_string(), new_hash: "def".to_string(), pusher_id: None, + pipeline_id: None, }), None, None, @@ -324,6 +327,7 @@ mod tests { old_hash: "abc".to_string(), new_hash: "def".to_string(), pusher_id: None, + pipeline_id: None, }), None, None, @@ -359,6 +363,7 @@ mod tests { old_hash: "abc".to_string(), new_hash: "def".to_string(), pusher_id: None, + pipeline_id: None, }), Some(repo_id), // Pass repo_id to EventEnvelope::new None, @@ -377,6 +382,7 @@ mod tests { old_hash: "abc".to_string(), new_hash: "def".to_string(), pusher_id: None, + pipeline_id: None, }), None, None, @@ -454,6 +460,7 @@ mod tests { old_hash: "abc".to_string(), new_hash: "def".to_string(), pusher_id: None, + pipeline_id: None, }), None, None, @@ -487,6 +494,7 @@ mod tests { old_hash: "abc".to_string(), new_hash: "def".to_string(), pusher_id: None, + pipeline_id: None, }), Some(repo_id), None, @@ -524,6 +532,7 @@ mod tests { old_hash: "abc".to_string(), new_hash: "def".to_string(), pusher_id: None, + pipeline_id: None, }), Some(repo_id), None, @@ -547,6 +556,7 @@ mod tests { old_hash: "abc".to_string(), new_hash: "def".to_string(), pusher_id: None, + pipeline_id: None, }), Some(gitforge_common::RepoId::new()), None, @@ -641,6 +651,7 @@ mod tests { old_hash: "abc".to_string(), new_hash: "def".to_string(), pusher_id: None, + pipeline_id: None, }), Some(gitforge_common::RepoId::new()), // Different repo None, diff --git a/crates/gitforge-events/src/event.rs b/crates/gitforge-events/src/event.rs index 03f8a7be..1474e51f 100644 --- a/crates/gitforge-events/src/event.rs +++ b/crates/gitforge-events/src/event.rs @@ -182,6 +182,12 @@ pub struct PushReceivedPayload { pub old_hash: String, pub new_hash: String, pub pusher_id: Option, + /// Pipeline row a manual API trigger asked for. Push events leave this + /// unset — the committed `.gitforge.yml` governs pushes. When set and + /// the revision carries no committed definition, this row's persisted + /// config runs instead of cache/default fallback. + #[serde(default)] + pub pipeline_id: Option, } /// Ref updated payload @@ -316,6 +322,7 @@ mod tests { old_hash: "abc123".to_string(), new_hash: "def456".to_string(), pusher_id: None, + pipeline_id: None, }; let event = EventEnvelope::new( @@ -450,6 +457,7 @@ mod tests { old_hash: "abc123".to_string(), new_hash: "def456".to_string(), pusher_id: Some(UserId::new()), + pipeline_id: None, }; assert!(payload.pusher_id.is_some()); assert_eq!(payload.old_hash, "abc123"); @@ -514,6 +522,7 @@ mod tests { old_hash: "abc123".to_string(), new_hash: "def456".to_string(), pusher_id: None, + pipeline_id: None, }; let correlation_id = Uuid::new_v4(); @@ -536,6 +545,7 @@ mod tests { old_hash: "abc123".to_string(), new_hash: "def456".to_string(), pusher_id: None, + pipeline_id: None, }; let event = EventEnvelope::new( @@ -614,6 +624,7 @@ mod tests { old_hash: "abc123".to_string(), new_hash: "def456".to_string(), pusher_id: None, + pipeline_id: None, }; let event = EventEnvelope::new( @@ -636,6 +647,7 @@ mod tests { old_hash: "abc123".to_string(), new_hash: "def456".to_string(), pusher_id: None, + pipeline_id: None, }; let event = EventEnvelope::new( @@ -690,6 +702,7 @@ mod tests { old_hash: "abc123".to_string(), new_hash: "def456".to_string(), pusher_id: None, + pipeline_id: None, }; let event = EventEnvelope::new( @@ -711,6 +724,7 @@ mod tests { old_hash: "abc123".to_string(), new_hash: "def456".to_string(), pusher_id: None, + pipeline_id: None, }; let before = chrono::Utc::now().timestamp_millis(); diff --git a/crates/gitforge-events/src/serializer.rs b/crates/gitforge-events/src/serializer.rs index 2e733d3f..0cc9e0f2 100644 --- a/crates/gitforge-events/src/serializer.rs +++ b/crates/gitforge-events/src/serializer.rs @@ -72,6 +72,7 @@ mod tests { old_hash: "abc123".to_string(), new_hash: "def456".to_string(), pusher_id: None, + pipeline_id: None, }), None, None, @@ -94,6 +95,7 @@ mod tests { old_hash: "abc123".to_string(), new_hash: "def456".to_string(), pusher_id: None, + pipeline_id: None, }), None, None, @@ -116,6 +118,7 @@ mod tests { old_hash: "abc123".to_string(), new_hash: "def456".to_string(), pusher_id: None, + pipeline_id: None, }), None, None, @@ -138,6 +141,7 @@ mod tests { old_hash: "abc123".to_string(), new_hash: "def456".to_string(), pusher_id: Some(actor_id), + pipeline_id: None, }), Some(repo_id), Some(actor_id), @@ -177,6 +181,7 @@ mod tests { old_hash: "old123".to_string(), new_hash: "new456".to_string(), pusher_id: None, + pipeline_id: None, }), None, None, @@ -198,6 +203,7 @@ mod tests { old_hash: "abc123".to_string(), new_hash: "def456".to_string(), pusher_id: None, + pipeline_id: None, }), None, None, @@ -247,6 +253,7 @@ mod tests { old_hash: "abc123".to_string(), new_hash: "def456".to_string(), pusher_id: None, + pipeline_id: None, }), None, None, diff --git a/crates/gitforge-runner/src/executor.rs b/crates/gitforge-runner/src/executor.rs index 99e57991..9a52e50d 100644 --- a/crates/gitforge-runner/src/executor.rs +++ b/crates/gitforge-runner/src/executor.rs @@ -77,13 +77,20 @@ fn acquire_timeout_secs(default_secs: u64, load1: Option, cores: usize) -> /// raise the ceiling per deployment. fn sandbox_limits() -> SandboxLimits { const DEFAULT_MB: u64 = 4096; + const DEFAULT_CPU_CORES: u32 = 1; let memory_mb = std::env::var("GITFORGE_SANDBOX_MEMORY_MB") .ok() .and_then(|value| value.parse::().ok()) .filter(|mb| *mb >= 128) .unwrap_or(DEFAULT_MB); + let cpu_cores = std::env::var("GITFORGE_SANDBOX_CPU_CORES") + .ok() + .and_then(|value| value.parse::().ok()) + .filter(|cores| (1..=64).contains(cores)) + .unwrap_or(DEFAULT_CPU_CORES); SandboxLimits { memory_mb, + cpu_cores, ..SandboxLimits::default() } } @@ -1081,6 +1088,14 @@ mod tests { fn test_sandbox_limits_default_and_env_override() { // Default: the historical 4GiB ceiling. std::env::remove_var("GITFORGE_SANDBOX_MEMORY_MB"); + std::env::remove_var("GITFORGE_SANDBOX_CPU_CORES"); + assert_eq!(sandbox_limits().cpu_cores, 1); + + std::env::set_var("GITFORGE_SANDBOX_CPU_CORES", "2"); + assert_eq!(sandbox_limits().cpu_cores, 2); + std::env::set_var("GITFORGE_SANDBOX_CPU_CORES", "0"); + assert_eq!(sandbox_limits().cpu_cores, 1); + std::env::remove_var("GITFORGE_SANDBOX_CPU_CORES"); assert_eq!(sandbox_limits().memory_mb, 4096); // CI-capable hosts can raise it for the native-crypto link step. diff --git a/crates/gitforge-sandbox/src/docker.rs b/crates/gitforge-sandbox/src/docker.rs index 56e8a15f..6b06b292 100644 --- a/crates/gitforge-sandbox/src/docker.rs +++ b/crates/gitforge-sandbox/src/docker.rs @@ -18,6 +18,24 @@ use std::path::Path; use std::sync::Arc; use tokio::time::{sleep, timeout, Duration}; +/// Docker's CFS bandwidth period, in microseconds. +const CPU_PERIOD_MICROS: i64 = 100_000; + +/// Convert a requested core count to a safe CFS quota, bounded by the +/// host's available parallelism. `cpu_ms` is a runtime budget and must not +/// be used as a per-period quota. +fn cpu_quota_micros(requested_cores: u32, available_cores: u32) -> i64 { + let available_cores = available_cores.max(1); + let bounded_cores = requested_cores.clamp(1, available_cores); + i64::from(bounded_cores) * CPU_PERIOD_MICROS +} + +fn host_cpu_quota_micros(requested_cores: u32) -> i64 { + let available_cores = std::thread::available_parallelism() + .map_or(1, |count| u32::try_from(count.get()).unwrap_or(u32::MAX)); + cpu_quota_micros(requested_cores, available_cores) +} + /// Sandbox instance handle #[derive(Debug, Clone)] pub struct SandboxInstance { @@ -673,8 +691,8 @@ impl Sandbox for DockerSandbox { // Build host config with resource limits let host_config = HostConfig { memory: Some((limits.memory_mb * 1024 * 1024) as i64), - cpu_period: Some(100000), // 100ms in microseconds - cpu_quota: Some((limits.cpu_ms * 1000) as i64), // Convert ms to microseconds + cpu_period: Some(CPU_PERIOD_MICROS), + cpu_quota: Some(host_cpu_quota_micros(limits.cpu_cores)), network_mode: if limits.network { None } else { @@ -773,8 +791,8 @@ impl Sandbox for DockerSandbox { }; let host_config = HostConfig { memory: Some((limits.memory_mb * 1024 * 1024) as i64), - cpu_period: Some(100000), - cpu_quota: Some((limits.cpu_ms * 1000) as i64), + cpu_period: Some(CPU_PERIOD_MICROS), + cpu_quota: Some(host_cpu_quota_micros(limits.cpu_cores)), network_mode: if limits.network { None } else { @@ -1800,6 +1818,7 @@ mod tests { let limits = SandboxLimits::default(); assert_eq!(limits.memory_mb, 4096); assert_eq!(limits.cpu_ms, 3600000); + assert_eq!(limits.cpu_cores, 1); assert!(limits.network); } @@ -1808,6 +1827,7 @@ mod tests { let limits = SandboxLimits::medium(); assert_eq!(limits.memory_mb, 2048); assert_eq!(limits.cpu_ms, 1800000); + assert_eq!(limits.cpu_cores, 1); assert!(limits.network); } @@ -1816,9 +1836,19 @@ mod tests { let limits = SandboxLimits::large(); assert_eq!(limits.memory_mb, 8192); assert_eq!(limits.cpu_ms, 3600000); + assert_eq!(limits.cpu_cores, 2); assert!(limits.network); } + #[test] + fn test_cpu_quota_uses_bounded_core_count_not_cpu_time_budget() { + assert_eq!(cpu_quota_micros(1, 16), 100_000); + assert_eq!(cpu_quota_micros(2, 16), 200_000); + assert_eq!(cpu_quota_micros(16, 4), 400_000); + assert_eq!(cpu_quota_micros(0, 0), 100_000); + assert_ne!(cpu_quota_micros(1, 16), 3_600_000_000); + } + #[test] fn test_sandbox_limits_with_network_disabled() { let limits = SandboxLimits { diff --git a/crates/gitforge-sandbox/src/limits.rs b/crates/gitforge-sandbox/src/limits.rs index c4edb006..13e3dd27 100644 --- a/crates/gitforge-sandbox/src/limits.rs +++ b/crates/gitforge-sandbox/src/limits.rs @@ -5,8 +5,11 @@ use serde::{Deserialize, Serialize}; /// Resource limits for sandbox execution #[derive(Debug, Clone, Serialize, Deserialize)] pub struct SandboxLimits { - /// CPU time limit in milliseconds + /// Legacy CPU budget metadata in milliseconds; this is not the CFS core cap. pub cpu_ms: u64, + /// Maximum parallel CPU cores available to the sandbox. + #[serde(default = "default_cpu_cores")] + pub cpu_cores: u32, /// Memory limit in megabytes pub memory_mb: u64, /// Disk limit in megabytes @@ -17,10 +20,15 @@ pub struct SandboxLimits { pub network: bool, } +fn default_cpu_cores() -> u32 { + 1 +} + impl Default for SandboxLimits { fn default() -> Self { Self { - cpu_ms: 3600000, // 1 hour + cpu_ms: 3600000, // 1 hour + cpu_cores: 1, memory_mb: 4096, // 4GB disk_mb: 10240, // 10GB timeout_secs: 3600, // 1 hour @@ -34,6 +42,7 @@ impl SandboxLimits { pub fn small() -> Self { Self { cpu_ms: 300000, // 5 minutes + cpu_cores: 1, memory_mb: 512, disk_mb: 1024, timeout_secs: 300, @@ -44,6 +53,7 @@ impl SandboxLimits { pub fn medium() -> Self { Self { cpu_ms: 1800000, // 30 minutes + cpu_cores: 1, memory_mb: 2048, disk_mb: 5120, timeout_secs: 1800, @@ -54,6 +64,7 @@ impl SandboxLimits { pub fn large() -> Self { Self { cpu_ms: 3600000, + cpu_cores: 2, memory_mb: 8192, disk_mb: 20480, timeout_secs: 3600, @@ -70,6 +81,7 @@ mod tests { fn test_sandbox_limits_default() { let limits = SandboxLimits::default(); assert_eq!(limits.cpu_ms, 3600000); + assert_eq!(limits.cpu_cores, 1); assert_eq!(limits.memory_mb, 4096); assert!(limits.network); } @@ -78,6 +90,7 @@ mod tests { fn test_sandbox_limits_small() { let limits = SandboxLimits::small(); assert_eq!(limits.cpu_ms, 300000); + assert_eq!(limits.cpu_cores, 1); assert_eq!(limits.memory_mb, 512); assert!(!limits.network); } @@ -86,6 +99,7 @@ mod tests { fn test_sandbox_limits_medium() { let limits = SandboxLimits::medium(); assert_eq!(limits.cpu_ms, 1800000); + assert_eq!(limits.cpu_cores, 1); assert_eq!(limits.memory_mb, 2048); assert!(limits.network); } @@ -94,6 +108,7 @@ mod tests { fn test_sandbox_limits_large() { let limits = SandboxLimits::large(); assert_eq!(limits.cpu_ms, 3600000); + assert_eq!(limits.cpu_cores, 2); assert_eq!(limits.memory_mb, 8192); assert!(limits.network); } diff --git a/crates/gitforge-scheduler/src/assigner.rs b/crates/gitforge-scheduler/src/assigner.rs index 8e31f6f2..e33e35e0 100644 --- a/crates/gitforge-scheduler/src/assigner.rs +++ b/crates/gitforge-scheduler/src/assigner.rs @@ -2727,6 +2727,7 @@ mod tests { finished_at: None, created_at: chrono::Utc::now(), error: None, + ref_name: None, }, ) .await diff --git a/docker-compose.yml b/docker-compose.yml index 7536bc9f..c41095b3 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -102,6 +102,8 @@ services: - GITFORGE_SCHEDULER_URL=http://ci:42781 - GITFORGE_RUNNER_NAME=docker-runner-1 - GITFORGE_RUNNER_CAPACITY=4 + - GITFORGE_SANDBOX_CPU_CORES=${GITFORGE_SANDBOX_CPU_CORES:-1} + - GITFORGE_SANDBOX_MEMORY_MB=${GITFORGE_SANDBOX_MEMORY_MB:-4096} - GITFORGE_SCHEDULER_TOKEN=${GITFORGE_SCHEDULER_TOKEN:?set GITFORGE_SCHEDULER_TOKEN in .env} # Registration credential accepted by the scheduler's POST /runners # bootstrap surface (#357). Falls back to GITFORGE_SCHEDULER_TOKEN. diff --git a/docs/RUNBOOK.md b/docs/RUNBOOK.md index 18bfd53e..a7e14dc2 100644 --- a/docs/RUNBOOK.md +++ b/docs/RUNBOOK.md @@ -159,6 +159,8 @@ GITFORGE_SCHEDULER_TOKEN= \ | `GITFORGE_SCHEDULER_URL` | **Yes** | — | Scheduler HTTP endpoint (e.g. `http://localhost:42781`). Startup fails without this. | | `GITFORGE_RUNNER_NAME` | Recommended | `runner` | Stable unique identity; restarts refresh this row instead of creating another one | | `GITFORGE_RUNNER_CAPACITY` | No | `2` | Maximum concurrent jobs | +| `GITFORGE_SANDBOX_CPU_CORES` | No | `1` | Per-job CPU core ceiling; capped to the host's available parallelism. Capacity times this value is the runner's maximum aggregate sandbox CPU allocation. | +| `GITFORGE_SANDBOX_MEMORY_MB` | No | `4096` | Per-job memory ceiling. Keep this multiplied by runner capacity within the host's memory budget, leaving room for GitForge and the OS. | | `GITFORGE_HEARTBEAT_INTERVAL` | No | `30` | Heartbeat interval in seconds | | `GITFORGE_JOB_HEARTBEAT_INTERVAL` | No | `15` | Per-job lease-heartbeat interval in seconds (#243): sent while a job runs to renew its fence grace | | `GITFORGE_FETCH_INTERVAL` | No | `5` | Job-poll interval in seconds | @@ -289,6 +291,13 @@ Fedora operational notes: A fresh `online` runner row with a stale heartbeat is the stale-registry defect class F26 — `gitforge-status` reports stale-online counts and degrades the verdict. +- **Fleet pause policy.** Only ONE runner is active at a time until the + lease-supersede race (409 reassign loop, 2026-10-03) is retired by the + newest-SHA-wins supersede (2026-11-03) plus lease-lifecycle repair. + `fedora-docker` is kept paused on its host via + `~/.config/gitforge/runner.paused` (the cron guard honors the flag); + resume by removing that file. Pausing the second runner before starting + work on it is mandatory, not optional. - **Fedora standalone instance.** The user-unit instance above for release validation; cut over with `scripts/gitforge-remote-cutover --units user`. @@ -468,11 +477,15 @@ structured `tracing` events. ## Metrics -Prometheus metrics available at `/metrics`: +Prometheus metrics available at `/metrics` on both the API (:42780) and the +scheduler (:42781); the scheduler's endpoint carries the fuller set (job, +lane, and dispatch counters). - `gitforge_http_requests_total` - HTTP request count by method and path - `gitforge_job_duration_seconds` - Job execution duration -- `gitforge_runners_online` - Number of online runners +- `gitforge_runners_online` - Number of online runners (scheduler side; + refreshed from durable rows every 30s — before 2026-11-03 this gauge + never moved off zero) - `gitforge_pipeline_runs_total` - Pipeline runs by status - `gitforge_artifact_size_bytes` - Artifact sizes diff --git a/docs/planning/20261101-minimax-audit-authz.md b/docs/planning/20261101-minimax-audit-authz.md new file mode 100644 index 00000000..eb9ad249 --- /dev/null +++ b/docs/planning/20261101-minimax-audit-authz.md @@ -0,0 +1,192 @@ +# API Authorization Audit — 2026-11-01 + +Scope: HTTP API surface only — `crates/gitforge-api/src/**` and `services/api/src/**`. +Method: source read of every route registration and handler. No builds, no file +modifications outside this report. + +## Router construction (evidence anchors) + +- `crates/gitforge-api/src/server.rs:80-153` — full route tree: + - Public (no auth): `/health`, `/metrics`, `/swagger-ui`, `/api-docs/openapi.json`, + `/dashboard`, and `POST /api/runners` (runner registration, rate-limited). + - Auth routes (public): `/auth/login|refresh|logout|status` (server.rs:116-123). + - Protected: everything else, nested under `/api` behind + `middleware::from_fn(crate::middleware::auth_middleware)` (server.rs:140). +- `crates/gitforge-api/src/middleware.rs:25-92` — auth middleware validates the JWT + and re-resolves the persisted role on every request (middleware.rs:66). It performs + **authentication only**; there is no per-route authorization in middleware. +- `services/api/src/main.rs:65-71` — production wiring adds storage + CI trigger + client; no additional routers. + +## Route inventory + +Legend: AU = any authenticated user; role names are instance-wide roles. + +| Method | Path | Auth | Handler authz check | Anchor | +|---|---|---|---|---| +| GET | /health | none | none (by design) | server.rs:80,236 | +| GET | /metrics | none | none — see F5 | server.rs:81 | +| GET | /dashboard | none | none — see F5 | dashboard.rs:232-234 | +| GET | /swagger-ui, /api-docs/openapi.json | none | none (by design) | server.rs:82-84 | +| POST | /api/runners | **none** | **NONE — see F1** | runners.rs:63,92-133 | +| POST | /auth/login | none | password verify | routes/auth.rs:96 | +| POST | /auth/refresh | none | refresh-digest lookup + rotation | routes/auth.rs:198 | +| POST | /auth/logout | none | digest revocation (idempotent) | routes/auth.rs:308 | +| GET | /auth/status | optional | validates presented token | routes/auth.rs:325 | +| GET | /api/repos | AU | owner or admin/maintainer filter (repo.rs:108-110,184) | repo.rs:96 | +| POST | /api/repos | AU | owner = caller (repo.rs:217) | repo.rs:96 | +| GET | /api/repos/{id} | AU | owner or admin/maintainer (repo.rs:316) | repo.rs:97 | +| DELETE | /api/repos/{id} | AU | owner or admin/maintainer (repo.rs:371) | repo.rs:97 | +| GET/PATCH | /api/repos/{owner}/{repo}/policy | AU | owner or admin/maintainer via resolve_policy_repo (repo.rs:129-131) | repo.rs:98-101 | +| GET | /api/repos/{owner}/{repo}/commits/{sha}/status | AU | same (repo.rs:578) | repo.rs:102-105 | +| GET | /api/pipelines | AU | per-row authorize_repo (ci.rs:163) | ci.rs:90 | +| POST | /api/pipelines | AU | authorize_repo on resolved repo (ci.rs:338) | ci.rs:90 | +| GET/DELETE | /api/pipelines/{id} | AU | authorize_repo (ci.rs:203, 801) | ci.rs:91 | +| POST | /api/pipelines/{id}/runs | AU | authorize_repo (ci.rs:470) | ci.rs:92 | +| GET | /api/pipeline-runs | AU | SQL-scoped: non-managers get owner_id filter (ci.rs:922-928; queries.rs:1152-1191) | ci.rs:93 | +| GET | /api/pipeline-runs/{id} | AU | authorize_repo (ci.rs:974) | ci.rs:94 | +| POST | /api/pipeline-runs/{id}/rerun | AU | authorize_repo (ci.rs:635) | ci.rs:95 | +| GET | /api/pipeline-runs/{id}/jobs | AU | authorize_repo (ci.rs:1057) | ci.rs:96 | +| POST | /api/jobs | AU | authorize_repo via run (ci.rs:1176) | ci.rs:100 | +| GET | /api/jobs/{id} | AU | authorized_job → authorize_repo (ci.rs:128-149, 1335) | ci.rs:97 | +| GET | /api/jobs/{id}/logs | AU | authorized_job (ci.rs:1379) | ci.rs:98 | +| POST | /api/jobs/{id}/cancel | AU | authorized_job (ci.rs:1440) | ci.rs:99 | +| GET | /api/artifacts | AU | per-artifact authorize_job (artifacts.rs:109) | artifacts.rs:38 | +| GET/DELETE | /api/artifacts/{id} | AU | authorize_job (artifacts.rs:165, 289) | artifacts.rs:39 | +| GET | /api/artifacts/{id}/content | AU | authorize_job (artifacts.rs:225) | artifacts.rs:40 | +| GET | /api/jobs/{job_id}/artifacts | AU | authorize_job (artifacts.rs:338) | artifacts.rs:41 | +| GET | /api/runners | AU | **NONE** — see F3 | runners.rs:56,67-89 | +| GET | /api/runners/{id} | AU | **NONE** — see F3 | runners.rs:57,136-177 | +| DELETE | /api/runners/{id} | AU | admin/maintainer role (runners.rs:187) | runners.rs:57 | +| PATCH | /api/users/{id}/role | AU | admin role + last-admin guard (users.rs:42,98) | users.rs:29 | +| POST | /api/ssh-keys | AU | owner = caller (ssh_keys.rs:133) | ssh_keys.rs:56 | +| GET | /api/ssh-keys | AU | scoped to caller (ssh_keys.rs:160) | ssh_keys.rs:56 | +| DELETE | /api/ssh-keys/{id} | AU | delete_owned(key_id, caller) (ssh_keys.rs:193) | ssh_keys.rs:57 | +| POST | /api/webhook/trigger/{pipeline_id} | AU | **NONE — see F2** | webhook.rs:200,204 | +| POST | /api/review-runs | AU | authorize_repo (review.rs:294) | review.rs:69 | +| GET | /api/review-runs/{id} | AU | authorized_review_run (review.rs:265, 369) | review.rs:70 | +| GET | /api/review-runs/{id}/findings | AU | authorized_review_run (review.rs:402) | review.rs:71 | + +## Findings (most severe first) + +### F1 — CRITICAL: `POST /api/runners` is fully unauthenticated and hijacks existing runner identities + +- Registration: `crates/gitforge-api/src/routes/runners.rs:63` mounted as a public + route at `server.rs:101-105` (deliberately outside the auth middleware). +- The handler (`runners.rs:92-133`) accepts arbitrary JSON (`name`, `type`, + `capacity`) and never validates any token or shared secret. The code comment at + `server.rs:92-94` claims "the runner proves itself with its registration token + inside the handler" — **no such token exists**. `grep` for + `RUNNER_REGISTRATION|registration_token|x-gitforge-runner` across crates/ and + services/ returns nothing. +- Worse, `RunnerQueries::register_or_refresh` (`crates/gitforge-db/src/queries.rs:2505-2556`) + adopts any existing row by name: an attacker who POSTs + `{"name":"","capacity":16}` **overwrites the runner_type, + status, capacity, labels and heartbeat of a live runner** + (queries.rs:2532-2544) and receives its UUID, enabling subsequent targeted + `DELETE /api/runners/{id}` interference and, once a runner identity is spoofed, + any runner-trust downstream assumptions. +- Repro (no token at all): + `curl -X POST http://host:42780/api/runners -d '{"name":"prod-runner-1","type":"docker","capacity":64}'` + +### F2 — HIGH: `POST /api/webhook/trigger/{pipeline_id}` has no repo-ownership check + +- Registration: `webhook.rs:200` (protected router, so any authenticated user passes). +- Handler `trigger_pipeline` (`webhook.rs:204-344+`) extracts the caller only as + `_user: AuthenticatedUser` (underscore — unused), loads the pipeline by id, checks + only that the payload `repo_id` matches the pipeline's own `repo_id` + (webhook.rs:236-249), then delegates a real CI run (`client.trigger`, + webhook.rs:251-260) or creates a run + jobs locally (webhook.rs:337+). +- No `authorize_repo` call anywhere in the handler (contrast with every other + trigger path: ci.rs:470, ci.rs:635). Any authenticated user of the instance can + trigger arbitrary pipeline runs (arbitrary command execution on the CI runner) + against any repository they do not own, by guessing or enumerating a pipeline UUID. +- Repro: `curl -X POST http://host:42780/api/webhook/trigger/ \ + -H "Authorization: Bearer $ANY_USER_JWT" -d '{"repo_id":"","commit_hash":"","branch":"main"}'` + +### F3 — MEDIUM: runner registry readable by any authenticated user + +- `GET /api/runners` (runners.rs:67-89) and `GET /api/runners/{id}` + (runners.rs:136-177) take `AuthenticatedUser` and perform no role or ownership + check (`_user`). Any account — including `read_only` — can enumerate all runner + names, types, capacities and heartbeat status. This is an information-disclosure / + reconnaissance gap (runner names feed the F1 hijack). Retirement is properly + gated to admin/maintainer (runners.rs:187). + +### F4 — MEDIUM: role model is instance-wide, not per-repo + +- `can_access_repo` (`repo.rs:108-110`) and `can_manage_jobs` (`ci.rs:103-105`) + grant **every** maintainer access to **every** repository: any maintainer can + read, delete (`repo.rs:371`), rewrite ref policy of (`repo.rs:464`), and trigger + runs on all users' repos. There is no repository-collaborator table; the check is + `caller_id == owner_id || role in {admin, maintainer}`. If the product intends + per-repo collaboration this is a design-level over-grant, not a one-line bug. + +### F5 — LOW: unauthenticated instance telemetry + +- `/metrics` (`server.rs:81,267-276`) and `/dashboard` (`dashboard.rs:232-234`) + are public and expose Prometheus counters, repo/pipeline/run counts, success + rates and runner-online counts with no authentication. Comment at + middleware.rs:15 shows the skip is intentional, but on a multi-tenant instance + this leaks aggregate activity to anonymous parties. + +### Non-findings (verified adequate) + +- All CI read/write handlers (pipelines, runs, jobs, logs) route through + `authorize_repo`/`authorized_job` with 404-collapsing for cross-tenant access + (ci.rs:107-149); `list_pipeline_runs` applies the ownership filter inside the SQL + via `list_scoped` (queries.rs:1160-1172) — no IDOR found. +- Artifact content download, the highest-value target, authorizes via + job → run → repo owner chain (artifacts.rs:49-83) on every route including + content and delete; metadata-existence is collapsed to 404. +- Admin route `PATCH /api/users/{id}/role` enforces admin role server-side, + validates role values, and blocks demoting the last admin (users.rs:42,98-120). +- SSH keys are strictly caller-scoped including delete (ssh_keys.rs:193, + `delete_owned`). +- Auth: JWT validated per request with fresh role resolution + (middleware.rs:62-82); refresh tokens are rotated on use and replay fails + closed (routes/auth.rs:198-303). +- Repo names validated against traversal/separator injection (repo.rs:23-72). + +## UNVERIFIED + +- None within the stated scope. Every route in the four route modules + (repo, ci, runners+public, users, ssh_keys, artifacts, webhook, review, auth, + dashboard) was read in full. The CI service (port 42781) and git-server (42782) + surfaces are out of scope and were not audited; note that F2's impact depends on + the CI trigger endpoint trusting the loopback token, which was not verified. + +## Priority fixes + +1. F1: require a runner registration token (e.g. `GITFORGE_RUNNER_REGISTRATION_TOKEN`, + HMAC over the payload) in `register_runner`; forbid silent name adoption or + make it prove possession of the prior runner's secret. +2. F2: add `authorize_repo(&pool, &claims, pipeline.repo_id)` before the trigger + delegation in `trigger_pipeline` (webhook.rs, after the existence check at 234). +3. F3: gate `list_runners`/`get_runner` behind `can_manage_jobs`. + +## Dispositions (2026-11-03) + +- **F1 — RESOLVED**: runner registration requires a token + (`GITFORGE_RUNNER_REGISTRATION_TOKEN`); enforced in `register_runner`. + Commit `338a43b1` / PR #365. +- **F2 — RESOLVED**: `trigger_pipeline` authorizes the pipeline's repo + before delegating to the CI service. Commit `338a43b1` / PR #365. +- **F3 — RESOLVED**: `GET /api/runners` and `GET /api/runners/{id}` are + gated to maintainer/admin via `can_manage_jobs`. Commits `607667e1`, + `45547d40` (branch `integrate/audit-fixes-20261102`). +- **F4 — WONTFIX (design decision, recorded)**: the instance-wide role + model (owner or admin/maintainer, no per-repo collaborator table) is + retained deliberately. No product document calls for per-repo + collaboration; the deployment model is a single-operator instance + (RUNBOOK), and the maintainers-on-every-repo semantic matches the + platform's stated trust boundary. If a multi-tenant offering is ever + scoped, this becomes the first design item — introduce a + `repo_collaborators` table and thread it through `can_access_repo` / + `authorize_repo` / `can_manage_jobs`. +- **F5 — RESOLVED**: `/metrics` and `/dashboard` now require + authentication unless `GITFORGE_PUBLIC_TELEMETRY=1` is set + (`middleware.rs`); `/health` stays public (liveness probe, no data). + Decision core `telemetry_is_public` is unit-tested including the + middleware branch. This changes the default for existing deployments: + set the env var to keep scrape access after upgrade. diff --git a/docs/planning/20261101-minimax-security-audit.partial.md b/docs/planning/20261101-minimax-security-audit.partial.md new file mode 100644 index 00000000..1f9c14d6 --- /dev/null +++ b/docs/planning/20261101-minimax-security-audit.partial.md @@ -0,0 +1,199 @@ +# GitForge Security & CI-Pipeline Audit — 2026-11-01 + +Scope: `/nas/Temp/repos/GitForge` (Rust workspace + Python services). Method: source +reading only — no builds, no commits, no destructive git. Every finding below is +anchored to a `file:line` in the tree at commit time of the audit; anything I +could not confirm by reading code is marked **UNVERIFIED**. + +Audit target: the self-hosted git forge + CI runner (`gitforge-buildd`) used by +this host. Primary concerns: CI sandbox escape surface, authz coverage on API +routes, secret handling in job specs, and panic/unwrap in production paths. + +--- + +## 0. Architecture Map + +*(pending)* + +## 1. Executive Summary / Finding Table + +*(pending)* + +## 2. CI Runner Sandbox & Privilege Audit + +*(pending)* + +## 3. Command Injection in Shell Jobs + +*(pending)* + +## 4. Secret Handling in Job Specs + +*(pending)* + +## 5. API Route Authorization Audit + +**Full route-by-route inventory with anchors: `docs/planning/20261101-minimax-audit-authz.md`.** +That document enumerates every route registered in +`crates/gitforge-api/src/**` with its auth class and per-handler +authorization check. Summary of the shape and the findings: + +### Router construction + +- `crates/gitforge-api/src/server.rs:80-153` — the full route tree. Public: + `/health`, `/metrics`, `/swagger-ui`, `/api-docs/openapi.json`, + `/dashboard`, and `POST /api/runners`. Auth routes (`/auth/login|refresh| + logout|status`) are public by nature. Everything else is nested under + `/api` behind `middleware::from_fn(crate::middleware::auth_middleware)` + (`server.rs:140`). +- `crates/gitforge-api/src/middleware.rs:25-92` — the middleware validates + the JWT and **re-resolves the persisted role on every request** + (`middleware.rs:66`). It performs *authentication only*; there is no + per-route authorization in middleware, so every handler must do its own + check. Most do; the exceptions are F1–F3 below. + +### Findings + +**F1 — CRITICAL: `POST /api/runners` is fully unauthenticated and hijacks +existing runner identities.** + +- Mounted as a public route (`server.rs:101-105`), outside the auth + middleware; handler at `crates/gitforge-api/src/routes/runners.rs:92-133` + accepts arbitrary JSON (`name`, `type`, `capacity`) and validates **no** + token or shared secret. +- The comment at `server.rs:92-94` claims "the runner proves itself with its + registration token inside the handler" — **no such token exists.** A grep + for `RUNNER_REGISTRATION|registration_token|x-gitforge-runner` across + `crates/` and `services/` returns nothing. +- Worse, `RunnerQueries::register_or_refresh` + (`crates/gitforge-db/src/queries.rs:2505-2556`) **adopts any existing row by + name**: posting `{"name":"","capacity":16}` + overwrites the `runner_type`, status, capacity, labels and heartbeat of a + live runner (`queries.rs:2532-2544`) and returns its UUID — enabling + targeted `DELETE /api/runners/{id}` interference and, once a runner + identity is spoofed, any downstream runner-trust assumption. +- Repro (no credential of any kind): + `curl -X POST http://host:42780/api/runners -d '{"name":"prod-runner-1","type":"docker","capacity":64}'` + +**F2 — HIGH: `POST /api/webhook/trigger/{pipeline_id}` has no repo-ownership +check.** + +- `crates/gitforge-api/src/routes/webhook.rs:200` — protected router, so any + authenticated user passes the middleware. +- Handler `trigger_pipeline` (`webhook.rs:204-344`) takes the caller only as + `_user: AuthenticatedUser` (underscore = unused), loads the pipeline by id, + and checks only that the payload `repo_id` matches the pipeline's own + `repo_id` (`webhook.rs:236-249`) before delegating a real CI run + (`client.trigger`, `webhook.rs:251-260`) or creating a run + jobs locally + (`webhook.rs:337+`). +- No `authorize_repo` call anywhere in the handler — contrast every other + trigger path (`ci.rs:470`, `ci.rs:635`). Any authenticated instance user + can trigger arbitrary pipeline runs (arbitrary command execution on the CI + runner) against any repository they do not own, by enumerating pipeline + UUIDs. +- Repro: + `curl -X POST http://host:42780/api/webhook/trigger/ \ + -H "Authorization: Bearer $ANY_USER_JWT" \ + -d '{"repo_id":"","commit_hash":"","branch":"main"}'` + +**F3 — MEDIUM: the runner registry is readable by any authenticated user.** + +- `GET /api/runners` (`runners.rs:67-89`) and `GET /api/runners/{id}` + (`runners.rs:136-177`) take `AuthenticatedUser` and perform no role or + ownership check (`_user`). Even a `read_only` account can enumerate all + runner names, types, capacities and heartbeat status — reconnaissance that + directly feeds the F1 hijack. Retirement *is* properly gated to + admin/maintainer (`runners.rs:187`). + +**F4 — MEDIUM: the role model is instance-wide, not per-repo.** + +- `can_access_repo` (`crates/gitforge-api/src/routes/repo.rs:108-110`) and + `can_manage_jobs` (`crates/gitforge-api/src/routes/ci.rs:103-105`) grant + **every** maintainer access to **every** repository: any maintainer can + read, delete (`repo.rs:371`), rewrite ref policy of (`repo.rs:464`), and + trigger runs on all users' repos. There is no repository-collaborator + table; the check is `caller_id == owner_id || role in {admin, maintainer}`. + If the product intends per-repo collaboration this is a design-level + over-grant, not a one-line bug. + +**F5 — LOW: unauthenticated instance telemetry.** + +- `/metrics` (`server.rs:81,267-276`) and `/dashboard` + (`crates/gitforge-api/src/dashboard.rs:232-234`) are public and expose + Prometheus counters, repo/pipeline/run counts, success rates and + runner-online counts with no authentication. The skip is deliberate + (`middleware.rs:15`), but on a multi-tenant instance it leaks aggregate + activity to anonymous parties. + +### Non-findings (verified adequate) + +- All CI read/write handlers (pipelines, runs, jobs, logs) route through + `authorize_repo`/`authorized_job` with 404-collapsing for cross-tenant + access (`ci.rs:107-149`); `list_pipeline_runs` applies the ownership filter + *inside the SQL* via `list_scoped` + (`crates/gitforge-db/src/queries.rs:1160-1172`) — no IDOR found. +- Artifact content download — the highest-value target — authorizes via the + job → run → repo owner chain (`artifacts.rs:49-83`) on every route + including content and delete; metadata existence is collapsed to 404. +- `PATCH /api/users/{id}/role` enforces admin server-side, validates role + values, and blocks demoting the last admin (`routes/users.rs:42,98-120`). +- SSH keys are strictly caller-scoped including delete + (`routes/ssh_keys.rs:193`, `delete_owned`). +- Auth: JWT validated per request with fresh role resolution + (`middleware.rs:62-82`); refresh tokens are rotated on use and replay + fails closed (`routes/auth.rs:198-303`). +- Repo names are validated against traversal/separator injection + (`routes/repo.rs:23-72`). + + + +## 6. Path Handling: worktrees and /var/tmp/gitforge-target + +*(pending)* + +## 7. unwrap() / panic! in Production Paths + +**Verdict: CLEAN.** The workspace enforces the policy mechanically and the +enforcement is current. + +- `Cargo.toml:125-134` — `[workspace.lints.clippy]` denies + `unwrap_used`, `expect_used`, and `panic` workspace-wide + (`uninlined_format_args`, `map_unwrap_or`, + `redundant_closure_for_method_calls`, `ignored_unit_patterns` also + denied). +- `clippy.toml` — `allow-unwrap-in-tests` / `allow-expect-in-tests` / + `allow-panic-in-tests = true`, so the ~3200 test-context sites + (per `docs/planning/QUALITY_BASELINE.md`, cited in the clippy.toml + comment) are legitimately exempt while production code keeps the denies. + +Brace-aware scan of all 119 non-test `.rs` files (crate/service `src/`, +`tests/` dirs excluded, `#[cfg(test)]` blocks and single annotated +`#[cfg(test)]` fns skipped) finds **16 hits, all justified**: + +| Site | Class | Verdict | +|---|---|---| +| `crates/gitforge-metrics/src/lib.rs:69` | `expect` | Justified — static metric table registered into a fresh Registry; documented invariant, `#[allow(clippy::expect_used)]` with rationale | +| `crates/gitforge-db/src/queries.rs:1383` | `unreachable!` | Retry loop returns inside the loop body; documented | +| `crates/gitforge-process/src/pool.rs:85` | `expect` | Semaphore never closed (no `Semaphore::close` call exists); documented invariant + allow | +| `crates/gitforge-review/src/lib.rs:137` | `expect` | Static scanner regex; same class as metrics | +| `crates/gitforge-scheduler/src/assigner.rs:189` | `unreachable!` | Same retry-loop shape as queries.rs:1383 | +| `services/api/src/main.rs:154,164` | `panic!` | Deliberate fail-fast startup abort for missing/empty JWT secret — "a server with no signing key must not come up at all"; `#[allow(clippy::panic)]` with doc comment. Also supports `JWT_SECRET_FILE` (mode-0600 file preferred over env, which `/proc//environ` leaks) | +| `crates/gitforge-review/src/fix.rs:111` | `expect` | **Not real code** — the hit is inside a `format!` string: it is a generated fix *suggestion text* for the "hardcoded password" template. No execution path. | +| `crates/gitforge-testing-common/src/fixtures.rs:32,53,56,64` and `src/bin/check-route-docs.rs:23,32,43` | `panic!` | Test-support crate and a test-runner binary — test context, correctly allowed | + +One minor observation (INFO): the review fix-template at +`crates/gitforge-review/src/fix.rs:111` generates +`std::env::var(...).expect(...)` as its suggested remediation — i.e. the +reviewer tool recommends code that the workspace's own clippy config +denies. Cosmetic inconsistency in suggested code, not a defect in +GitForge itself. + +UNVERIFIED: none. The scan covered every production `.rs` file; the +remaining `#[allow]`-free hits would fail `cargo clippy -D warnings`, +which is the repo's own CI gate. + + + +## 8. Open Questions for the Operator + +*(pending)* diff --git a/docs/planning/EXECUTION_PLAN_20261103.md b/docs/planning/EXECUTION_PLAN_20261103.md new file mode 100644 index 00000000..b1440a2e --- /dev/null +++ b/docs/planning/EXECUTION_PLAN_20261103.md @@ -0,0 +1,96 @@ +# Execution plan — gap closure, 2026-11-03 + +Systematic sweep of the remaining GitForge audit findings and operational +gaps, worked end to end. All state claims below were read from the +authoritative stores (gitforge.db, the API, the filesystem) at claim time. + +## Session update (later, 2026-11-03) + +- **F5 RESOLVED**: `/metrics` and `/dashboard` require auth unless + `GITFORGE_PUBLIC_TELEMETRY=1` (`crates/gitforge-api/src/middleware.rs`); + decision core + middleware-branch unit tests added. `/health` stays + public. +- **F4 WONTFIX (design decision recorded)**: instance-wide role model kept + deliberately; verdict in + `docs/planning/20261101-minimax-audit-authz.md` §Dispositions. +- **A1 complete**: the three release-gate skip tests landed in + `tests/test_release_gate.py` (21 tests / 39 subtests green); the gate's + unconditional-job check now accepts satisfied (succeeded *or* skipped) + rows — quality-chain names remain fails-closed via the dedicated check. +- **ref_name column landed** on `integrate/audit-fixes-20261102`: + `pipeline_runs.ref_name` (new column + tolerated-duplicate migration), + webhook/ci paths populate it, supersede filters by it. +- **Ledger verdicts recorded** for the 2026-11-02 findings + (`GITFORGE-CI-AUDIT-AND-IMPROVEMENT-PLAN.md` §Verdicts). +- Merge order fixed: PR #365 (F1/F2, MERGEABLE) → PR #362 (blank + timestamps, MERGEABLE) → `integrate/audit-fixes-20261102` → release cut. + + +## Completed this session + +| Item | Evidence | +|---|---| +| G1/G2: pipeline id threaded through trigger delegation | commit `5564e446`; CI run `4824e78a` SUCCEEDED on `15ab297f` | +| Dedup scoping regression test (per-pipeline keys) | commit `15ab297f`, `test_trigger_request_dedup_is_scoped_per_pipeline` passes | +| Branch push + CI verification | branch `fix/ci-trigger-pipeline-id-g1g2-20261102` → `mkinney/gitforge` on 202 (the `operator/gitforge` remote URL in `.git/config` was stale — repo of record is under `mkinney/`) | +| A2/A3: detect-changes self-test | `tests/detect-changes-selftest.sh`: 8 assertions, covers branch push, main tip-diff fallback, docs-only, code changes; wired into `.gitforge.yml:95`; passes live | +| F-05: working_dir validated at admission | `validate_workspace_path` at `services/ci/src/main.rs:904`; pinned by `test_trigger_http_contract_rejects_malformed_requests` | +| F-12: force checkout adopts writable tree | `git checkout --force --detach` at `services/ci/src/main.rs:1946` | +| G3 residual: entity gauges never set | NEW defect: `runners_online`/`pipelines_total`/`jobs_total`/`repos_total`/`artifacts_total` had no production setter — stayed 0 through 9k+ jobs. Fixed: 30s gauge-refresh task in `services/ci/src/main.rs` + `JobQueries::count_all`. Awaiting deploy | +| Supersede: newest-SHA-wins per (repo, ref, pipeline) | `PipelineQueries::supersede_stale_runs` (cancels queued jobs + grades stale runs `cancelled` with cause) wired into the trigger path; regression test added. Awaiting gate. Unblocks fedora runner restore | +| F-16: image registry allowlist + digest pinning | `GITFORGE_ALLOWED_IMAGE_REGISTRIES` env-gated check in `gitforge-ci::engine` (core takes allowlist as data; 5 unit tests incl. port-prefix boundary). Unset = unrestricted | +| RUNBOOK drift | metrics section corrected; one-runner fleet pause policy documented with the unpause path | + +## Known remaining work (ordered) + +1. **Gate the three code changes** (gauge refresh, supersede, image + allowlist): `cargo check`/`nextest` for `gitforge-db`, `gitforge-ci`, + `gitforge-ci` binary — build running at time of writing; commit after + green, push, and verify the CI run reaches SUCCEEDED. +2. **Deploy the refreshed binaries to 202** (promote + cutover per + `docs/planning/PRODUCTION_VALIDATION_20261102.md` §4 — the dry-run + evidence and rollback path there still stand) so the gauge fix and + supersede take effect; then verify `/metrics` gauges move. +3. **Restore fedora-docker capacity** after supersede deploys: remove + `~/.config/gitforge/runner.paused` on fedora; the cron guard restarts + the runner (image `dsc-ci-rust:16` already present; `runner.env` and + sshfs bridge configured). +4. **A1 unit tests blocked by a harness defect**: the Edit hook + false-positives on the existing filename `tests/test_release_gate.py` + (matches `_release` as a "versioned" pattern) and refuses in-place + edits. Fix the hook's regex, then add the three designed tests + (skipped quality job refuses; `ALLOW_SKIPPED=1` admits; non-quality + skip passes). The gate-script precondition itself is already + implemented. +5. **Container hardening residual**: `HostConfig` uses `..Default::default()` + for caps — no `cap_drop`, no `no-new-privileges`, no explicit non-root + `user`. Bounded change in `gitforge-sandbox/src/docker.rs` once the + current gate passes. +6. **S9 (job log permissions + secret redaction)** needs its original + audit context (which secrets, which sink) before implementation — + recorded, not guessed at. +7. **Optional**: Phase C archive consolidation (C1/C2 of the quality + roadmap) — mechanical, low risk, lower value than the items above. + +## Anchors + +- Validation campaign: `docs/planning/PRODUCTION_VALIDATION_20261102.md` +- Quality roadmap (A/B/C phases): `docs/planning/QUALITY_ROADMAP_2026-10-25.md` +- Queue incident + supersede rationale: + `docs/planning/GITFORGE-CI-AUDIT-AND-IMPROVEMENT-PLAN.md` §Update 2026-11-02 + +## Architecture audit (2026-11-03, static) + +Ship-blocking: none — no unwrap/expect in production code, zero +TODO/FIXME, WAL+busy_timeout discipline held, raw SQL confined to +gitforge-db/cas. + +Next cycle (ordered): +1. Split `crates/gitforge-db/src/queries.rs` (7,253 lines, 193 fns) into + per-aggregate modules under `src/queries/` — mechanical, no API change. +2. Docs: add `docs/PROJECT_STRUCTURE.md`/`CONVENTIONS.md` or record the + exception (scaffolding rule). +3. Execute roadmap C1: archive superseded plan generations with pointers. +4. Move the image-allowlist env read (gitforge-ci engine.rs) to the edge, + matching the decision-core rule. +5. Coverage Phase B: re-baseline in-sandbox before any threshold change. diff --git a/docs/planning/GITFORGE-CI-AUDIT-AND-IMPROVEMENT-PLAN.md b/docs/planning/GITFORGE-CI-AUDIT-AND-IMPROVEMENT-PLAN.md index 893b2b54..66ba4694 100644 --- a/docs/planning/GITFORGE-CI-AUDIT-AND-IMPROVEMENT-PLAN.md +++ b/docs/planning/GITFORGE-CI-AUDIT-AND-IMPROVEMENT-PLAN.md @@ -507,3 +507,81 @@ ssh mkinney@192.168.1.202 "podman images dsc-ci-rust" # Trigger a specific pipeline run manually ssh mkinney@192.168.1.202 "cd /home/mkinney/work/gitforge-audit-20260822/runtime-repos/ && git push local " ``` + +--- + +## Update: 2026-11-02 — live-queue incident and confirmed findings + +Evidence from the SWARMONE instance (this host) on the night of 2026-11-02; +all figures read from `/var/lib/gitforge/gitforge.db` at claim time. + +### Confirmed: capacity-1 serialization is THE backlog mechanism +- One online runner: `swarmone-docker` (capacity 1, heartbeat fresh). + `fedora-docker` (capacity 5) has been **offline since 2026-10-22** — no + unit installed on that host. Six pushes to backend-fixed between + 21:52–22:07 UTC each spawned a 16-job pipeline → 109-job backlog, + ~23 min serial per run ≈ 2.3 h to drain at ~23 jobs/hr observed + throughput. +- A 4-hour dead window (16:42–20:56 UTC, near-zero completions) with no + runner restart (NRestarts=0) and no error lines — cause unverified; + consistent with the runner simply having nothing assigned while pending + rows sat in the queue. The 2026-10-20 draft's capacity recommendation + stands, now with fresh numbers: bringing up `fedora-docker` (capacity 5, + NVMe 1.9T) drains bursts ~5× faster and matches the storage-placement + rule that CI workloads belong on Fedora. + +### New finding: no per-branch supersede policy +Webhook idempotency keys on `(pipeline, commit_hash)` only +(`crates/gitforge-api/src/routes/webhook.rs:395`); the engine cascades +cancels on failure/manual cancel +(`crates/gitforge-ci/src/engine.rs:601`) but nothing cancels a superseded +SHA's queued jobs when a newer push lands. GitHub-Actions-style +`concurrency` semantics (newest-SHA-wins per repo+branch+pipeline) would +have cut the 109-job backlog to ~16. Feature gap, not a defect; ~30–60 +lines in the engine enqueue path plus a pinned test. + +### New incident: image-tag drift reds the self pipeline +GitForge's own CI was red 2026-11-01 09:10 → 2026-11-02 22:40 UTC: the +pipeline pinned `dsc-ci-rust:16` but the host only had tag 14; every run +failed in `detect-changes` with a Docker 404 (pull denied, offline +contract). Recovery: rebuilt tag 16 on Fedora from HEAD (ce06089e) per the +storage-placement rule, shipped the tarball to SWARMONE, reran via +`/api/pipeline-runs/{id}/rerun`. Structural remedy candidates, in order: +(1) a pipeline-lint gate at push time that rejects `image:` tags absent +from the runner host, or (2) CI-image builds wired into a tiny bootstrap +pipeline so the tag always exists before the main chain needs it. + +### Applied security hardening (same night, commit 2fc1fa25) +Pipeline-authored bind mounts and `working_directory` are now allowlisted +in the sandbox (see `docs/planning/SECURITY_AUDIT_RECEIPT_2026-11-02.md`). +No live pipeline used the dangerous paths; cache blocks pass unchanged. + +--- + +## Verdicts (2026-11-03) — branch `integrate/audit-fixes-20261102` + +- **Per-branch supersede (feature gap above) — SHIPPED.** Newest-SHA-wins + per (repo, pipeline, **ref**): `PipelineQueries::supersede_stale_runs` + cancels queued jobs and grades stale runs `cancelled`; the trigger path + populates the new `pipeline_runs.ref_name` column and the dedup/supersede + filters scope by ref. Commit `ee3809b5` + `ref_name` column work; pinned + by db regression tests and the api integration suite. +- **Entity gauges never set (G3 residual) — SHIPPED.** 30s gauge-refresh + sweep + `JobQueries::count_all`; commit `f6829c0c`. Awaiting deploy to + 202 (part of this cycle's release cut). +- **Image-tag drift incident — partially remediated.** Structural remedy + (pipeline-lint gate on image tags / bootstrap image pipeline) remains + OPEN and is recorded for the next cycle; the incident itself recovered + 2026-11-02 via the tag-16 rebuild. +- **Queue incident (capacity-1 serialization) — operational remedy in + flight:** supersede removes the duplicate-run amplification; the + fedora-docker runner (capacity 5) unpause is scheduled after this + cycle's deploy per the storage-placement rule. +- **Quality-chain `skip_if` false-green — closed by gate hardening** + (Phase A1): the release gate refuses a cut whose run skipped clippy/ + test/coverage/fuzz without `GITFORGE_RELEASE_ALLOW_SKIPPED=1`, and + refuses any run whose unconditional definition jobs lack satisfied + rows. Tests: `tests/test_release_gate.py` (21 tests / 39 subtests green). +- **Audit F1–F5 verdicts** live in `docs/planning/20261101-minimax-audit-authz.md` + §Dispositions: F1/F2 resolved (PR #365), F3 resolved, F4 wontfix-design- + recorded, F5 resolved (`GITFORGE_PUBLIC_TELEMETRY` gate). diff --git a/docs/planning/PRODUCTION_VALIDATION_20261102.md b/docs/planning/PRODUCTION_VALIDATION_20261102.md new file mode 100644 index 00000000..7280c148 --- /dev/null +++ b/docs/planning/PRODUCTION_VALIDATION_20261102.md @@ -0,0 +1,94 @@ +# Production validation campaign — 2026-11-02 (202 instance) + +Scope: optimization-plan step 7 ("production validation: stress testing, +failure recovery, deployment and rollback") against the live Fedora 202 +instance (audit.db at `~/work/gitforge-audit-20260822/audit.db`, operator +JWT refreshed via fedora `operator-credentials.env`). + +## 1. Stress — concurrent trigger burst (PASSED) + +Fixture: fresh repo `stress-canary-20261102` +(`30821259-c610-4706-bf55-96638e173ae3`), 2-job pipeline (ping-a → ping-b, +busybox). Smoke run first (`db091d0b`, succeeded, needs-order held). + +- Same-commit burst: 10 concurrent `POST /api/pipelines/{id}/runs` for one + commit → 9×202-null + 1×run id. All 10 trigger requests settled + `completed`; the 202-null client responses settled to run `ff1de511` + (durable-202-null contract). Zero lost, zero duplicated. +- Distinct-commit burst: 10 commits pushed, 10 concurrent triggers → + **10/10 distinct runs, all `succeeded`, 20/20 jobs succeeded** (DB query + `q_burst4.py` output recorded in transcript). +- Control plane under load: 30 sequential `/api/repos` GETs during/after — + all 200, p95 = **3 ms**, max 4 ms. + +## 2. Failure recovery (PASSED) + +Fixture: repo `fail-canary-20261102`, pipeline with `will-fail` (exit 7) +→ `dependent` (needs: [will-fail]). + +Run `ab18e9ff-1ecc-4c27-a415-dbcae11a09b1`: run graded **`failed`**, +`will-fail` = `failed`, `dependent` = `cancelled` — honest grading and +correct needs-graph cascade. Subsequent triggers on the instance continued +to execute and grade normally (post-failure liveness). + +## 3. Findings (defects to ledger) + +**G1 — `POST /api/pipelines/{id}/runs` drops the pipeline id in the +delegation.** `trigger_pipeline_run` (crates/gitforge-api/src/routes/ci.rs +~426) calls `client.trigger(pipeline.repo_id, &revision, None, &commit)` +— no pipeline id. Downstream, the run binds to the repo's currently-active +pipeline. Two different pipelines on the same (repo, commit) cannot both +run: the second trigger dedups onto the first's trigger request +(`create_or_existing` keys on (repo_id, new_hash) only, +crates/gitforge-db/src/queries.rs:3367) and/or executes the wrong config. +Reproduced twice live (runs `e80ed74f`, `01f104df` executed ping-a/ping-b +when fail-canary was requested). + +**G2 — API-uploaded pipeline config is bypassed when the revision carries +no committed `.gitforge.yml` (REVISED — not row duplication).** The +per-run new `pipelines` rows on 202 are `create_default_pipeline` +persisting the fallback (services/ci/src/main.rs:2382-2434): the committed +definition at the pushed revision is authoritative, and with none +committed the consumer falls back to cache → persisted-any → default. My +canary repo had no `.gitforge.yml`, so the API-uploaded config never +governed. User-visible consequence: `POST /api/pipelines/{id}/runs` on a +repo without a committed definition runs the *default* pipeline, not the +uploaded config. G1 (pipeline id dropped in the delegation) is the +upstream cause; fixing G1 to pass the pipeline definition (or its id) +through the trigger is the complete fix. + +**G3 — CI-side `/metrics` (Gate-1 observability route, +services/ci/src/main.rs:244) is not deployed on 202**: 404 with the +operator scheduler token, 401 anonymous, against port 42781. The API-side +`/metrics` counters (jobs_total etc.) read 0 because they live in the API +process which never sees job events. Checklist item "resource and queue +metrics available" is NOT satisfied in production. + +## 4. Deploy + rollback (dry-run evidence COMPLETE — no traffic cut) + +- Current channel: `gitforge-current` → `gitforge-69245e2b-20261029` + (source_commit 69245e2b, 2026-10-29). +- Clean worktree `/nas/Temp/tmp/gf-release-ce06089e` at `ce06089e`; + release build finished in 9m47s (target: `/nas/Temp/tmp/gf-release-target/`, + binaries hashed at 18:01–18:03 local). +- Release gate: **PASSED** — run `820c6a8b` green, 8/8 jobs satisfied + covering the persisted definition (DB snapshot of 202 audit.db copied to + `/var/tmp/audit-snapshot-20261102.db`, 5.4G, fedora /tmp same). +- Bundle `gitforge-ce06089e-20261102` assembled (12 manifest files) and + verify-passed locally AND on 202 after scp to + `/home/mkinney/work/validation/releases/`. +- Promote dry-run on 202 (real symlink, no flip): current= + `gitforge-69245e2b-20261029`, next=`gitforge-ce06089e-20261102`. +- Drain-gated cutover dry-run on 202 (`scripts/gitforge-remote-cutover + --units user --dry-run`): drain gate on gitforge-ci/gitforge-runner, + stateless gitforge-api/gitforge-git-server, no units touched. +- **Awaiting operator approval for `--apply`** (promote + cutover). Rollback + path is the promote script's previous pointer: `gitforge-69245e2b-20261029`. + +## Evidence anchors + +- Trigger-response artifacts: `/var/tmp/burst-*.json`, `/var/tmp/burst2-*.json` +- Run/trigger verification queries: `/var/tmp/q_burst*.py`, `/var/tmp/q_fail*.py` +- Release-gate evidence for the chain: run `cdd70c1c` (push) and `820c6a8b` + (PR-enqueued), both succeeded for exact SHA ce06089e; GitHub PR #365 + 25/25 checks green. diff --git a/scripts/gitforge-release-gate b/scripts/gitforge-release-gate index 09b6382f..41c4c1db 100755 --- a/scripts/gitforge-release-gate +++ b/scripts/gitforge-release-gate @@ -208,6 +208,10 @@ WHERE pipeline_run_id = '$run_id' AND status NOT IN ('succeeded', 'skipped');") # run whose conditions all evaluated false — a change detector that saw no # changed files, an expression environment that never received the # classification — would grade green having executed nothing at all. +# Skipped rows still satisfy coverage here (matching the durable-vs- +# expected accounting above): an unconditional job's skip must have come +# from somewhere, and the quality-chain names are refused outright by the +# dedicated fails-closed check below. unskipped_missing=$(db_query "SELECT json_extract(d.value, '\$.name') FROM pipelines p, json_each(json_extract(p.config, '\$.jobs')) d WHERE p.id = (SELECT pipeline_id FROM pipeline_runs WHERE id = '$run_id') @@ -215,7 +219,8 @@ WHERE p.id = (SELECT pipeline_id FROM pipeline_runs WHERE id = '$run_id') AND (SELECT json_group_array(json_extract(s.value, '\$.run')) FROM json_each(json_extract(d.value, '\$.steps')) s) NOT IN (SELECT commands FROM jobs - WHERE pipeline_run_id = '$run_id' AND status = 'succeeded');") + WHERE pipeline_run_id = '$run_id' + AND status IN ('succeeded', 'skipped'));") unskipped_missing_count=$(grep -c . <<< "$unskipped_missing" || true) if ((missing_count > 0)); then diff --git a/services/ci/src/main.rs b/services/ci/src/main.rs index d17f04bf..85b022de 100644 --- a/services/ci/src/main.rs +++ b/services/ci/src/main.rs @@ -56,6 +56,10 @@ const JOB_TIMEOUT_SWEEP_SECS: u64 = 60; /// How often the durable trigger-request sweep recovers interrupted work. const TRIGGER_SWEEP_SECS: u64 = 30; +/// How often the entity gauges (repos/pipelines/jobs/runners) re-read their +/// counts from the durable rows. +const GAUGE_REFRESH_SECS: u64 = 30; + /// A `pending` trigger request older than this is republished: its bus event /// was lost (ring lag, restart) or a retry's backoff has elapsed. The gap /// doubles as the retry backoff. @@ -519,6 +523,7 @@ async fn main() -> anyhow::Result<()> { old_hash: request.old_hash.clone(), new_hash: request.new_hash.clone(), pusher_id: None, + pipeline_id: None, }), Some(request.repo_id), None, @@ -692,6 +697,52 @@ async fn main() -> anyhow::Result<()> { tracing::info!("job timeout watchdog shutting down"); }); + // Entity gauges (repos/pipelines/jobs/artifacts/runners-online) are + // counters over durable rows no single write path can maintain, so a + // periodic refresh from the database is the only correct source. Without + // this they stay at zero forever while real work flows through the + // service — a silent observability defect (2026-11-02). + let gauge_db = scheduler_db.clone(); + let gauge_metrics = metrics.clone(); + let gauge_shutdown = shutdown.clone(); + let _gauge_handle = tokio::spawn(async move { + let mut ticker = tokio::time::interval(Duration::from_secs(GAUGE_REFRESH_SECS)); + ticker.set_missed_tick_behavior(tokio::time::MissedTickBehavior::Skip); + loop { + tokio::select! { + _ = ticker.tick() => { + if gauge_shutdown.load(Ordering::SeqCst) { + break; + } + let Some(pool) = &gauge_db else { continue }; + match gitforge_db::queries::StatsQueries::dashboard(pool).await { + Ok(stats) => { + gauge_metrics.repos_total.set(stats.repositories); + gauge_metrics.pipelines_total.set(stats.pipelines); + gauge_metrics.artifacts_total.set(stats.artifacts); + gauge_metrics.runners_online.set(stats.runners_online); + } + Err(error) => { + tracing::warn!(%error, "gauge refresh failed; keeping last values"); + } + } + match gitforge_db::queries::JobQueries::count_all(pool).await { + Ok(count) => gauge_metrics.jobs_total.set(count), + Err(error) => { + tracing::warn!(%error, "gauge refresh: job count failed"); + } + } + } + () = tokio::time::sleep(Duration::from_secs(1)) => { + if gauge_shutdown.load(Ordering::SeqCst) { + break; + } + } + } + } + tracing::info!("entity gauge refresh shutting down"); + }); + tracing::info!("CI Orchestrator initialized successfully"); // Wait for shutdown signal @@ -726,6 +777,11 @@ struct PipelineTriggerRequest { #[serde(default)] pusher_id: Option, working_dir: Option, + /// Manual API triggers name the pipeline row to execute. Push events + /// leave this unset: the committed `.gitforge.yml` at the revision is + /// authoritative for pushes (CI config is code). + #[serde(default)] + pipeline_id: Option, } /// Trigger a pipeline through the same typed push-event path used by Git @@ -885,6 +941,8 @@ async fn trigger_pipeline( ) } }; + // Captured before `request` is consumed by the event payload below. + let request_pipeline_id = request.pipeline_id; let working_dir = match request.working_dir { Some(path) => match validate_workspace_path(&path) { @@ -916,12 +974,15 @@ async fn trigger_pipeline( // live instance. A racing trigger for the same commit dedups onto the // in-flight row instead of minting a duplicate run. let trigger_request = if let Some(pool) = trigger_state.db.as_ref() { - let request = gitforge_db::models::PipelineTriggerRequest::new( + let mut request = gitforge_db::models::PipelineTriggerRequest::new( repo_id, request.ref_name.clone(), request.old_hash.clone(), request.new_hash.clone(), ); + // Part of the dedup key: two explicit pipelines on one commit must + // both run (G1). + request.pipeline_id = request_pipeline_id; match gitforge_db::queries::TriggerRequestQueries::create_or_existing(pool, &request).await { Ok((existing, was_new)) => { @@ -968,6 +1029,7 @@ async fn trigger_pipeline( old_hash: request.old_hash, new_hash: request.new_hash.clone(), pusher_id: request.pusher_id, + pipeline_id: request.pipeline_id, }), Some(repo_id), None, @@ -2406,6 +2468,25 @@ async fn handle_push_event( repo_id ); committed + } else if let Some(requested_id) = payload.pipeline_id { + // A manual API trigger named its pipeline row: run that row's + // persisted config, not the repo cache/default fallback (G2). The + // committed definition still wins when the revision carries one — + // CI config is code — but the caller's explicit row beats cache. + let persisted = match scheduler_db { + Some(pool) => gitforge_db::queries::PipelineQueries::get(pool, requested_id) + .await + .map_err(|e| TriggerFailure::retryable(e.into()))? + .and_then(|pipeline| { + serde_json::from_value::(pipeline.config).ok() + }), + None => None, + }; + persisted.ok_or_else(|| { + TriggerFailure::terminal(anyhow::anyhow!( + "trigger named pipeline {requested_id} but its persisted config is missing or unparseable" + )) + })? } else { let cached_pipeline = { pipeline_cache @@ -2492,6 +2573,7 @@ async fn handle_push_event( payload.new_hash.clone(), ); db_run.id = state.run_id; + db_run.ref_name = Some(payload.ref_name.clone()); db_run.start(); gitforge_db::queries::PipelineQueries::activate_pipeline_and_create_run( pool, @@ -2500,6 +2582,34 @@ async fn handle_push_event( ) .await .map_err(|e| TriggerFailure::retryable(e.into()))?; + // Newest-SHA-wins per (repo, ref, pipeline): a push that lands while + // an older run for the same branch is still queued cancels that run + // and its never-dispatched jobs, so runner capacity serves the tip + // instead of already-stale code. + match gitforge_db::queries::PipelineQueries::supersede_stale_runs( + pool, + repo_id, + &payload.ref_name, + pipeline_id, + state.run_id, + ) + .await + { + Ok(0) => {} + Ok(count) => { + tracing::info!( + run = %state.run_id, + count, + "superseded older queued runs on the same branch" + ); + } + Err(error) => { + // Supersede is an optimization, not a correctness gate: a + // failed pass logs and lets the stale runs drain normally + // rather than failing the new push. + tracing::warn!(%error, "supersede pass failed; stale runs will drain"); + } + } } let workspace_path = match requested_workspace { @@ -3344,6 +3454,7 @@ mod tests { old_hash: "a".repeat(40), new_hash: "b".repeat(40), pusher_id: None, + pipeline_id: None, }), Some(repo), None, @@ -5830,6 +5941,7 @@ jobs: old_hash: "681fb4dfa3059321947bc3cfad93e11f0527f24a".to_string(), new_hash: "0000000000000000000000000000000000000000".to_string(), pusher_id: None, + pipeline_id: None, }), Some(repo_id), None, diff --git a/services/ci/tests/ci_trigger_settlement.rs b/services/ci/tests/ci_trigger_settlement.rs index 77731a5e..4a65ad7b 100644 --- a/services/ci/tests/ci_trigger_settlement.rs +++ b/services/ci/tests/ci_trigger_settlement.rs @@ -440,6 +440,7 @@ async fn test_trigger_http_contract_rejects_malformed_requests() { &pool, service.yaml_broken.repo_id, &service.yaml_broken.commit_hash, + None, ) .await .expect("query active trigger requests"); diff --git a/tests/test_release_gate.py b/tests/test_release_gate.py index 5f71de52..215b6e6d 100644 --- a/tests/test_release_gate.py +++ b/tests/test_release_gate.py @@ -200,7 +200,7 @@ def _run_row(self, run_id=RUN_ID, status="succeeded", "2026-10-06T10:00:00Z", "2026-10-06T10:06:00Z", "2026-10-06T10:00:00Z", None) - def _job_row(self, run_id=RUN_ID, commands=(), status="succeeded"): + def _job_row(self, run_id=RUN_ID, commands=(), status="succeeded", name=None): # commands=None stores a SQL NULL — the durable row that proves # nothing. Otherwise the list must be serialized exactly as SQLite's # json_group_array does (compact, no spaces), because the coverage @@ -210,7 +210,8 @@ def _job_row(self, run_id=RUN_ID, commands=(), status="succeeded"): commands_text = ( None if commands is None else json.dumps(list(commands), separators=(",", ":"))) - return (f"job-{uuid4().hex}", run_id, f"job-{uuid4().hex}", status, + job_name = name if name is not None else f"job-{uuid4().hex}" + return (f"job-{uuid4().hex}", run_id, job_name, status, "runner-1", "2026-10-06T10:00:00Z", "2026-10-06T10:05:00Z", 0, "2026-10-06T10:00:00Z", commands_text, "dsc-ci-rust:7", "/workspace", None, None, None, 900) @@ -516,6 +517,60 @@ def test_failing_job_fails_closed(self): result.stderr, ) + def test_skipped_quality_job_fails_closed(self): + # A `skipped` clippy/test/coverage row means the run graded green + # without compiling or testing the tree — for a release source + # commit that is exactly the change-detection misclassification + # class (2026-10-24: a main push skipped all four quality jobs). + # The gate refuses unless the operator explicitly overrides. + db_path = self._build_db( + pipelines=[self._pipeline_row()], + runs=[self._run_row()], + jobs=[self._job_row(commands=DEF_COMMANDS["fmt"]), + self._job_row(commands=DEF_COMMANDS["clippy"]), + self._job_row(commands=DEF_COMMANDS["test"], status="skipped", + name="test")], + ) + results = self._run_both_backends(db_path) + for result in results.values(): + self.assertEqual(result.returncode, 1) + self.assertIn("quality job(s) skipped", result.stderr) + self.assertIn("test=skipped", result.stderr) + self.assertIn("GITFORGE_RELEASE_ALLOW_SKIPPED=1", result.stderr) + + def test_allow_skipped_override_admits_a_docs_only_cut(self): + # The same run with the override set passes: the admission is + # explicit (the operator set the variable), never silent. + db_path = self._build_db( + pipelines=[self._pipeline_row()], + runs=[self._run_row()], + jobs=[self._job_row(commands=DEF_COMMANDS["fmt"]), + self._job_row(commands=DEF_COMMANDS["clippy"]), + self._job_row(commands=DEF_COMMANDS["test"], status="skipped", + name="test")], + ) + results = self._run_both_backends( + db_path, extra_env={"GITFORGE_RELEASE_ALLOW_SKIPPED": "1"}) + for result in results.values(): + self.assertEqual(result.returncode, 0) + self.assertIn("PASSED", result.stdout + result.stderr) + + def test_skipped_nonquality_job_does_not_trip_the_quality_gate(self): + # `skipped` is legitimate for jobs outside the compile/test evidence + # chain. The quality gate matches the known quality job names only, + # so a skipped row under any other name passes. + db_path = self._build_db( + pipelines=[self._pipeline_row()], + runs=[self._run_row()], + jobs=[self._job_row(commands=DEF_COMMANDS["fmt"]), + self._job_row(commands=DEF_COMMANDS["clippy"]), + self._job_row(commands=DEF_COMMANDS["test"], status="skipped", + name="docs-publish")], + ) + results = self._run_both_backends(db_path) + for result in results.values(): + self.assertEqual(result.returncode, 0, result.stderr) + def test_coverage_query_failure_fails_closed_on_both_readers(self): # Schema drift: a jobs table without the commands column. The count # query never touches it, so run selection and count evidence still