From 7d67cfe6788007d49690ccc350a92dc5c8db8ad2 Mon Sep 17 00:00:00 2001 From: Michael Kinney <14325925+aliasfoxkde@users.noreply.github.com> Date: Mon, 2 Nov 2026 04:12:08 -0600 Subject: [PATCH 1/7] docs(planning): record the 2026-11-01 API authorization audit Route-by-route inventory of the API surface with auth class and per-handler authorization check for every registered route. Anchors the registration and webhook findings fixed in the preceding commit. --- docs/planning/20261101-minimax-audit-authz.md | 26 ------------------- 1 file changed, 26 deletions(-) diff --git a/docs/planning/20261101-minimax-audit-authz.md b/docs/planning/20261101-minimax-audit-authz.md index eb9ad249..de4ef4c9 100644 --- a/docs/planning/20261101-minimax-audit-authz.md +++ b/docs/planning/20261101-minimax-audit-authz.md @@ -164,29 +164,3 @@ Legend: AU = any authenticated user; role names are instance-wide roles. 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. From f000848face8674ef4e235582c660e8c05f68800 Mon Sep 17 00:00:00 2001 From: Michael Kinney <14325925+aliasfoxkde@users.noreply.github.com> Date: Tue, 3 Nov 2026 08:58:24 -0600 Subject: [PATCH 2/7] feat(api): gate public telemetry; per-ref supersede column; audit verdicts - /metrics and /dashboard require auth unless GITFORGE_PUBLIC_TELEMETRY=1 (F5); /health stays public; decision core + middleware tests - pipeline_runs.ref_name column; webhook/ci populate it; supersede and integration/scheduler tests updated - release gate: unconditional-job check accepts satisfied rows (succeeded or skipped); quality-chain names stay fails-closed; gate tests extended - ledger verdicts for F1-F5 and the 2026-11-02 queue incident; F4 recorded as a design decision; architecture audit appended to the execution plan; __pycache__ ignored Co-Authored-By: Claude --- docs/planning/20261101-minimax-audit-authz.md | 26 +++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/docs/planning/20261101-minimax-audit-authz.md b/docs/planning/20261101-minimax-audit-authz.md index de4ef4c9..eb9ad249 100644 --- a/docs/planning/20261101-minimax-audit-authz.md +++ b/docs/planning/20261101-minimax-audit-authz.md @@ -164,3 +164,29 @@ Legend: AU = any authenticated user; role names are instance-wide roles. 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. From 8447bc462917fa64134072668df269fec75760ce Mon Sep 17 00:00:00 2001 From: Michael Kinney <14325925+aliasfoxkde@users.noreply.github.com> Date: Tue, 3 Nov 2026 11:51:29 -0600 Subject: [PATCH 3/7] fix(api): route telemetry behind auth unless opted in /metrics and /dashboard were merged into public_routes, bypassing the auth middleware entirely, so the GITFORGE_PUBLIC_TELEMETRY gate was dead code at the route boundary. Wire both surfaces into the protected router by default and into public_routes only on explicit opt-in. Integration tests now pin the boundary: anonymous 401 by default, 200 with opt-in. --- crates/gitforge-api/src/server.rs | 25 ++++++++++++++----- crates/gitforge-api/tests/integration.rs | 31 +++++++++++++++++++++++- 2 files changed, 49 insertions(+), 7 deletions(-) diff --git a/crates/gitforge-api/src/server.rs b/crates/gitforge-api/src/server.rs index 7c8d701f..8c6a35e7 100644 --- a/crates/gitforge-api/src/server.rs +++ b/crates/gitforge-api/src/server.rs @@ -103,19 +103,25 @@ impl ApiServer { .allow_headers(Any) }; - // Public routes (no auth required) + // Public routes (no auth required). Telemetry surfaces are added + // only when the operator opted in; otherwise they ride behind the + // auth middleware via the protected router (F5). + let telemetry_public = crate::middleware::telemetry_is_public( + std::env::var("GITFORGE_PUBLIC_TELEMETRY").ok().as_deref(), + ); let health = Router::new().route("/health", get(health_check)); let metrics = Router::new().route("/metrics", get(metrics_handler)); let swagger = Router::new() .route("/swagger-ui", get(crate::openapi::swagger_ui)) .route("/api-docs/openapi.json", get(crate::openapi::openapi_spec)); - let dashboard = crate::dashboard::dashboard_routes(); let mut public_routes = Router::new(); public_routes = public_routes.merge(health); - public_routes = public_routes.merge(metrics); - public_routes = public_routes.merge(dashboard); public_routes = public_routes.merge(swagger); + if telemetry_public { + let dashboard = crate::dashboard::dashboard_routes(); + public_routes = public_routes.merge(metrics).merge(dashboard); + } // Runner registration is unauthenticated by design (the runner // proves itself with its registration token inside the handler), so // it is the other public write surface and gets its own rate-limit @@ -151,7 +157,7 @@ impl ApiServer { // Protected routes (auth required) let pool_arc = Arc::new(pool); - let protected_routes = Router::new() + let mut protected_routes = Router::new() .merge(repo_routes()) .merge(ci_routes()) .merge(runner_routes()) @@ -159,7 +165,14 @@ impl ApiServer { .merge(ssh_key_routes()) .merge(artifact_routes()) .merge(webhook_routes()) - .merge(review_routes()) + .merge(review_routes()); + if !telemetry_public { + let dashboard = crate::dashboard::dashboard_routes(); + protected_routes = protected_routes + .merge(metrics) + .merge(dashboard); + } + let protected_routes = protected_routes // Authenticate once at the protected-route boundary. Individual // handlers may still apply resource authorization, but token // parsing and validation must not depend on every handler diff --git a/crates/gitforge-api/tests/integration.rs b/crates/gitforge-api/tests/integration.rs index 9f7068d2..93f3cb7b 100644 --- a/crates/gitforge-api/tests/integration.rs +++ b/crates/gitforge-api/tests/integration.rs @@ -142,6 +142,11 @@ async fn test_health_check_endpoint() { #[tokio::test] async fn test_metrics_endpoint() { + // Opt-in telemetry: with GITFORGE_PUBLIC_TELEMETRY unset, /metrics is + // served behind the auth middleware (F5) and must reject anonymous + // access. The wiring boundary (route placement, not just the + // middleware decision) is what this test pins. + unsafe { std::env::remove_var("GITFORGE_PUBLIC_TELEMETRY") }; let pool = Pool::memory().await.unwrap(); let server = ApiServer::new("test-secret", pool); let app = server.into_router(); @@ -157,7 +162,7 @@ async fn test_metrics_endpoint() { .await .unwrap(); - assert_eq!(response.status(), StatusCode::OK); + assert_eq!(response.status(), StatusCode::UNAUTHORIZED); } #[tokio::test] @@ -675,6 +680,29 @@ async fn test_delete_nonexistent_repo() { #[tokio::test] async fn test_dashboard_endpoint() { + // Default posture: authenticated only (F5 wiring boundary). + unsafe { std::env::remove_var("GITFORGE_PUBLIC_TELEMETRY") }; + let pool = Pool::memory().await.unwrap(); + let server = ApiServer::new("test-secret", pool); + let app = server.into_router(); + + let response = app + .oneshot( + Request::builder() + .uri("/dashboard") + .body(Body::empty()) + .unwrap(), + ) + .await + .unwrap(); + + assert_eq!(response.status(), StatusCode::UNAUTHORIZED); +} + +#[tokio::test] +async fn test_dashboard_endpoint_public_opt_in() { + // Operator opt-in keeps both telemetry surfaces public. + unsafe { std::env::set_var("GITFORGE_PUBLIC_TELEMETRY", "1") }; let pool = Pool::memory().await.unwrap(); let server = ApiServer::new("test-secret", pool); let app = server.into_router(); @@ -690,6 +718,7 @@ async fn test_dashboard_endpoint() { .unwrap(); assert_eq!(response.status(), StatusCode::OK); + unsafe { std::env::remove_var("GITFORGE_PUBLIC_TELEMETRY") }; } #[tokio::test] From d9106e1731e2df2d1acdd191fee6936d3562d5af Mon Sep 17 00:00:00 2001 From: Michael Kinney <14325925+aliasfoxkde@users.noreply.github.com> Date: Tue, 3 Nov 2026 12:14:03 -0600 Subject: [PATCH 4/7] style(api): fix rustfmt drift in telemetry merge --- crates/gitforge-api/src/server.rs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/crates/gitforge-api/src/server.rs b/crates/gitforge-api/src/server.rs index 8c6a35e7..71c863f9 100644 --- a/crates/gitforge-api/src/server.rs +++ b/crates/gitforge-api/src/server.rs @@ -168,9 +168,7 @@ impl ApiServer { .merge(review_routes()); if !telemetry_public { let dashboard = crate::dashboard::dashboard_routes(); - protected_routes = protected_routes - .merge(metrics) - .merge(dashboard); + protected_routes = protected_routes.merge(metrics).merge(dashboard); } let protected_routes = protected_routes // Authenticate once at the protected-route boundary. Individual From 81a70941cfc9325b5c0fc18a0a689834b6272d44 Mon Sep 17 00:00:00 2001 From: Michael Kinney <14325925+aliasfoxkde@users.noreply.github.com> Date: Tue, 3 Nov 2026 13:14:43 -0600 Subject: [PATCH 5/7] fix(api): resolve telemetry routing compile errors Make telemetry_is_public crate-visible and clone the metrics router so it can be merged into either the public or protected router without a double move. --- crates/gitforge-api/src/middleware.rs | 2 +- crates/gitforge-api/src/server.rs | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/crates/gitforge-api/src/middleware.rs b/crates/gitforge-api/src/middleware.rs index 1ad25048..f1f52b79 100644 --- a/crates/gitforge-api/src/middleware.rs +++ b/crates/gitforge-api/src/middleware.rs @@ -19,7 +19,7 @@ const PUBLIC_PATHS: &[&str] = &["/health", "/metrics", "/swagger-ui", "/api-docs /// 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 { +pub(crate) fn telemetry_is_public(raw: Option<&str>) -> bool { matches!(raw, Some(v) if v.trim() == "1") } diff --git a/crates/gitforge-api/src/server.rs b/crates/gitforge-api/src/server.rs index 71c863f9..ec4cc36f 100644 --- a/crates/gitforge-api/src/server.rs +++ b/crates/gitforge-api/src/server.rs @@ -109,8 +109,8 @@ impl ApiServer { let telemetry_public = crate::middleware::telemetry_is_public( std::env::var("GITFORGE_PUBLIC_TELEMETRY").ok().as_deref(), ); + let metrics_router = Router::new().route("/metrics", get(metrics_handler)); let health = Router::new().route("/health", get(health_check)); - let metrics = Router::new().route("/metrics", get(metrics_handler)); let swagger = Router::new() .route("/swagger-ui", get(crate::openapi::swagger_ui)) .route("/api-docs/openapi.json", get(crate::openapi::openapi_spec)); @@ -120,7 +120,7 @@ impl ApiServer { public_routes = public_routes.merge(swagger); if telemetry_public { let dashboard = crate::dashboard::dashboard_routes(); - public_routes = public_routes.merge(metrics).merge(dashboard); + public_routes = public_routes.merge(metrics_router.clone()).merge(dashboard); } // Runner registration is unauthenticated by design (the runner // proves itself with its registration token inside the handler), so @@ -168,7 +168,7 @@ impl ApiServer { .merge(review_routes()); if !telemetry_public { let dashboard = crate::dashboard::dashboard_routes(); - protected_routes = protected_routes.merge(metrics).merge(dashboard); + protected_routes = protected_routes.merge(metrics_router).merge(dashboard); } let protected_routes = protected_routes // Authenticate once at the protected-route boundary. Individual From 97691a4f22d3fce37f9571dab59ae24b6513eeec Mon Sep 17 00:00:00 2001 From: Michael Kinney <14325925+aliasfoxkde@users.noreply.github.com> Date: Tue, 3 Nov 2026 14:12:25 -0600 Subject: [PATCH 6/7] fix(api): mount protected telemetry at top level; serialize tests The default-posture telemetry routes were merged into the /api-nested protected router, which relocated them to /api/metrics and /api/dashboard and left the canonical paths a 404. Mount them at top level behind their own auth-middleware layer instead. Serialize the three env-mutating telemetry tests with a shared mutex: the decision is read from the process env at router-build time, so parallel test threads race and randomly invert the asserted posture. --- crates/gitforge-api/src/server.rs | 24 +++++++++++++++++------- crates/gitforge-api/tests/integration.rs | 9 +++++++++ 2 files changed, 26 insertions(+), 7 deletions(-) diff --git a/crates/gitforge-api/src/server.rs b/crates/gitforge-api/src/server.rs index ec4cc36f..c713d14d 100644 --- a/crates/gitforge-api/src/server.rs +++ b/crates/gitforge-api/src/server.rs @@ -157,7 +157,7 @@ impl ApiServer { // Protected routes (auth required) let pool_arc = Arc::new(pool); - let mut protected_routes = Router::new() + let protected_routes = Router::new() .merge(repo_routes()) .merge(ci_routes()) .merge(runner_routes()) @@ -166,10 +166,6 @@ impl ApiServer { .merge(artifact_routes()) .merge(webhook_routes()) .merge(review_routes()); - if !telemetry_public { - let dashboard = crate::dashboard::dashboard_routes(); - protected_routes = protected_routes.merge(metrics_router).merge(dashboard); - } let protected_routes = protected_routes // Authenticate once at the protected-route boundary. Individual // handlers may still apply resource authorization, but token @@ -179,9 +175,12 @@ impl ApiServer { .layer(Extension(Arc::new(auth.clone()))) .layer(Extension(pool_arc.clone())); - // Metrics layer for automatic request recording + // Telemetry surfaces stay at top level either way; when the + // operator has not opted into public access they are merged in + // behind their own auth-middleware layer (F5). They cannot ride + // the /api-nested protected router — nesting would move them to + // /api/metrics and leave the canonical paths unrouted. let metrics_layer = MetricsLayer::new(metrics_arc.clone()); - let app = public_routes .layer(cors) .layer(metrics_layer) @@ -189,6 +188,17 @@ impl ApiServer { .layer(Extension(metrics_arc)) .merge(auth_routes) .nest("/api", protected_routes); + let app = if telemetry_public { + app + } else { + let dashboard = crate::dashboard::dashboard_routes(); + let telemetry = metrics_router + .merge(dashboard) + .layer(middleware::from_fn(crate::middleware::auth_middleware)) + .layer(Extension(Arc::new(auth.clone()))) + .layer(Extension(pool_arc)); + app.merge(telemetry) + }; Self { router: app, diff --git a/crates/gitforge-api/tests/integration.rs b/crates/gitforge-api/tests/integration.rs index 93f3cb7b..d9ce9ddb 100644 --- a/crates/gitforge-api/tests/integration.rs +++ b/crates/gitforge-api/tests/integration.rs @@ -26,6 +26,12 @@ use gitforge_storage::{Artifact, ArtifactStore}; use std::sync::Arc; use tower::ServiceExt; +/// Serializes the tests that mutate GITFORGE_PUBLIC_TELEMETRY. The +/// decision is read at router-build time from the process env, which is +/// shared by every test thread in this binary — without the lock the +/// default-posture tests race the opt-in test and randomly invert. +static TELEMETRY_ENV_LOCK: std::sync::Mutex<()> = std::sync::Mutex::new(()); + async fn webhook_fixture( config: serde_json::Value, ) -> ( @@ -146,6 +152,7 @@ async fn test_metrics_endpoint() { // served behind the auth middleware (F5) and must reject anonymous // access. The wiring boundary (route placement, not just the // middleware decision) is what this test pins. + let _guard = TELEMETRY_ENV_LOCK.lock().unwrap(); unsafe { std::env::remove_var("GITFORGE_PUBLIC_TELEMETRY") }; let pool = Pool::memory().await.unwrap(); let server = ApiServer::new("test-secret", pool); @@ -681,6 +688,7 @@ async fn test_delete_nonexistent_repo() { #[tokio::test] async fn test_dashboard_endpoint() { // Default posture: authenticated only (F5 wiring boundary). + let _guard = TELEMETRY_ENV_LOCK.lock().unwrap(); unsafe { std::env::remove_var("GITFORGE_PUBLIC_TELEMETRY") }; let pool = Pool::memory().await.unwrap(); let server = ApiServer::new("test-secret", pool); @@ -702,6 +710,7 @@ async fn test_dashboard_endpoint() { #[tokio::test] async fn test_dashboard_endpoint_public_opt_in() { // Operator opt-in keeps both telemetry surfaces public. + let _guard = TELEMETRY_ENV_LOCK.lock().unwrap(); unsafe { std::env::set_var("GITFORGE_PUBLIC_TELEMETRY", "1") }; let pool = Pool::memory().await.unwrap(); let server = ApiServer::new("test-secret", pool); From 9a403a1112884b3ecae9259836a167a829312300 Mon Sep 17 00:00:00 2001 From: Michael Kinney <14325925+aliasfoxkde@users.noreply.github.com> Date: Tue, 3 Nov 2026 14:32:59 -0600 Subject: [PATCH 7/7] fix(api): drop telemetry env lock before await points --- crates/gitforge-api/tests/integration.rs | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/crates/gitforge-api/tests/integration.rs b/crates/gitforge-api/tests/integration.rs index d9ce9ddb..d3f53c07 100644 --- a/crates/gitforge-api/tests/integration.rs +++ b/crates/gitforge-api/tests/integration.rs @@ -152,11 +152,12 @@ async fn test_metrics_endpoint() { // served behind the auth middleware (F5) and must reject anonymous // access. The wiring boundary (route placement, not just the // middleware decision) is what this test pins. + let pool = Pool::memory().await.unwrap(); let _guard = TELEMETRY_ENV_LOCK.lock().unwrap(); unsafe { std::env::remove_var("GITFORGE_PUBLIC_TELEMETRY") }; - let pool = Pool::memory().await.unwrap(); let server = ApiServer::new("test-secret", pool); let app = server.into_router(); + drop(_guard); // Request metrics let response = app @@ -688,11 +689,12 @@ async fn test_delete_nonexistent_repo() { #[tokio::test] async fn test_dashboard_endpoint() { // Default posture: authenticated only (F5 wiring boundary). + let pool = Pool::memory().await.unwrap(); let _guard = TELEMETRY_ENV_LOCK.lock().unwrap(); unsafe { std::env::remove_var("GITFORGE_PUBLIC_TELEMETRY") }; - let pool = Pool::memory().await.unwrap(); let server = ApiServer::new("test-secret", pool); let app = server.into_router(); + drop(_guard); let response = app .oneshot( @@ -710,11 +712,12 @@ async fn test_dashboard_endpoint() { #[tokio::test] async fn test_dashboard_endpoint_public_opt_in() { // Operator opt-in keeps both telemetry surfaces public. + let pool = Pool::memory().await.unwrap(); let _guard = TELEMETRY_ENV_LOCK.lock().unwrap(); unsafe { std::env::set_var("GITFORGE_PUBLIC_TELEMETRY", "1") }; - let pool = Pool::memory().await.unwrap(); let server = ApiServer::new("test-secret", pool); let app = server.into_router(); + drop(_guard); let response = app .oneshot(