From 64ef51d775251d791af4d42a90687af795055f72 Mon Sep 17 00:00:00 2001 From: Gravirei Date: Tue, 29 Sep 2026 13:28:21 +0600 Subject: [PATCH 1/2] fix(node): owner-gate create_task repo_id against hosted repos (#496) --- crates/gitlawb-node/src/api/mod.rs | 5 + crates/gitlawb-node/src/api/tasks.rs | 34 +++++++ crates/gitlawb-node/src/graphql/mutation.rs | 80 +++++++++++++++ crates/gitlawb-node/src/test_support.rs | 106 ++++++++++++++++++++ 4 files changed, 225 insertions(+) diff --git a/crates/gitlawb-node/src/api/mod.rs b/crates/gitlawb-node/src/api/mod.rs index df10175a..79589939 100644 --- a/crates/gitlawb-node/src/api/mod.rs +++ b/crates/gitlawb-node/src/api/mod.rs @@ -223,6 +223,11 @@ mod authz_guard { (events, "list_repo_events", "authorize_repo_read("), // Bucket C — signer-self: the acting DID is matched/bound to auth.0 (tasks, "create_task", "did_matches("), + // #496: create_task is signer-self AND owner-when-repo-scoped — a + // repo_id naming a hosted repo admits tasks only from its owner. + // Both halves are pinned: the did_matches row guards the signer + // binding, this one the repo-ownership gate. + (tasks, "create_task", "require_repo_owner("), (tasks, "claim_task", "did_matches("), (tasks, "complete_task", "did_matches("), (tasks, "fail_task", "did_matches("), diff --git a/crates/gitlawb-node/src/api/tasks.rs b/crates/gitlawb-node/src/api/tasks.rs index de22134a..9c816326 100644 --- a/crates/gitlawb-node/src/api/tasks.rs +++ b/crates/gitlawb-node/src/api/tasks.rs @@ -101,6 +101,40 @@ pub async fn create_task( if !crate::api::did_matches(&auth.0, &body.delegator_did) { return Err(forbidden("delegator_did must be the authenticated signer")); } + // #496: a caller-supplied repo_id must name a repo the caller owns. Without + // this, any signed caller can plant a task — payload and UCAN included — + // under a foreign repo id, where repo-gated task reads surface it exactly + // to that repo's readers and the named assignee can claim it. Resolve the + // id against hosted repos: a hosted, non-quarantined repo admits tasks + // only from its owner. An id naming no hosted repo (unknown, or + // quarantined — which is hidden as if it did not exist, so 403ing it + // would confirm a real id) is kept as an opaque label with no existence + // oracle either way; such ids resolve to no hosted repo, so repo-scoped + // task read gates treat them as unscoped (delegator/assignee-only under + // the #268/#464 visibility contract). Repo-less tasks are unaffected. + if let Some(repo_id) = body.repo_id.as_deref() { + let record = state.db.get_repo_by_id(repo_id).await.map_err(|e| { + ( + StatusCode::INTERNAL_SERVER_ERROR, + Json(json!({ "error": e.to_string() })), + ) + })?; + if let Some(record) = record { + let quarantined = state + .db + .is_repo_quarantined(&record.id) + .await + .map_err(|e| { + ( + StatusCode::INTERNAL_SERVER_ERROR, + Json(json!({ "error": e.to_string() })), + ) + })?; + if !quarantined && crate::api::require_repo_owner(&record, &auth.0).is_err() { + return Err(forbidden("only the repo owner can file tasks against it")); + } + } + } let now = Utc::now().to_rfc3339(); let task = AgentTask { id: Uuid::new_v4().to_string(), diff --git a/crates/gitlawb-node/src/graphql/mutation.rs b/crates/gitlawb-node/src/graphql/mutation.rs index 7fb7a1dc..6da39fa2 100644 --- a/crates/gitlawb-node/src/graphql/mutation.rs +++ b/crates/gitlawb-node/src/graphql/mutation.rs @@ -36,6 +36,27 @@ impl MutationRoot { } let delegator_did = caller.to_string(); let db = ctx.data_unchecked::>(); + // #496: same repo-ownership gate as the REST handler — a repo_id + // naming a hosted, non-quarantined repo admits tasks only from its + // owner, so a stranger cannot file under a foreign repo id. Unknown + // or quarantined ids stay oracle-free opaque labels (unscoped under + // the #268/#464 read contract); repo-less tasks unaffected. + if let Some(repo_id) = input.repo_id.as_deref() { + let record = db + .get_repo_by_id(repo_id) + .await + .map_err(crate::graphql::graphql_db_err)?; + if let Some(record) = record { + let quarantined = db + .is_repo_quarantined(&record.id) + .await + .map_err(crate::graphql::graphql_db_err)?; + if !quarantined { + crate::api::require_repo_owner(&record, caller) + .map_err(crate::graphql::graphql_app_err)?; + } + } + } let now = Utc::now().to_rfc3339(); let task = AgentTask { id: Uuid::new_v4().to_string(), @@ -332,4 +353,63 @@ mod tests { errors(&resp) ); } + + /// #496 (GraphQL): createTask applies the same repo-ownership gate as the + /// REST handler — a stranger naming a hosted repo is rejected, while the + /// owner files. + #[sqlx::test] + async fn create_task_rejects_foreign_repo_id(pool: PgPool) { + let state = crate::test_support::test_state(pool).await; + let owner = "did:key:zGQLTASKOWNERAAAAAAAAAAAAAAAAAAAAAAAAAA"; + let stranger = "did:key:zGQLTASKSTRANGERBBBBBBBBBBBBBBBBBBBBBB"; + let now = chrono::Utc::now(); + let repo = crate::db::RepoRecord { + id: uuid::Uuid::new_v4().to_string(), + name: "gql-task-gate-repo".to_string(), + owner_did: owner.to_string(), + description: None, + is_public: true, + default_branch: "main".to_string(), + created_at: now, + updated_at: now, + disk_path: "/tmp/gql-task-gate-repo".to_string(), + forked_from: None, + machine_id: None, + }; + state.db.create_repo(&repo).await.expect("seed repo"); + let schema = state.graphql_schema.as_ref(); + + let q = |actor: &str| { + format!( + r#"mutation {{ createTask(delegatorDid: "{actor}", input: {{ kind: "build", capability: "repo:write", repoId: "{}" }}) {{ id }} }}"#, + repo.id + ) + }; + + // Stranger signs as themselves (so the signer binding passes) but + // names the victim's repo → rejected by the ownership gate, leaking + // nothing about the repo. + let resp = schema + .execute(Request::new(q(stranger)).data(AuthenticatedDid(stranger.into()))) + .await; + let errs = errors(&resp); + assert!( + errs.contains("repo owner"), + "a non-owner must not file under a foreign repo id: {errs}" + ); + assert!( + !errs.contains(&repo.id) && !errs.contains(owner), + "denial must leak nothing about the repo: {errs}" + ); + + // The owner files under their own repo id. + let resp = schema + .execute(Request::new(q(owner)).data(AuthenticatedDid(owner.into()))) + .await; + assert!( + errors(&resp).is_empty(), + "the owner should file against their repo: {}", + errors(&resp) + ); + } } diff --git a/crates/gitlawb-node/src/test_support.rs b/crates/gitlawb-node/src/test_support.rs index 430c0600..838d4ffe 100644 --- a/crates/gitlawb-node/src/test_support.rs +++ b/crates/gitlawb-node/src/test_support.rs @@ -590,6 +590,112 @@ mod tests { ); } + /// #496: create_task must not bind a caller-supplied repo_id verbatim. A + /// signed caller naming a hosted repo it does not own is rejected (403, + /// leaking nothing about the repo); the owner files (201); repo-less and + /// unknown-id tasks stay allowed (unknown ids are oracle-free opaque + /// labels, unscoped under the #268/#464 read contract); and a quarantined + /// repo id is treated as unknown (no 403 oracle on quarantined existence). + #[sqlx::test] + async fn create_task_rejects_foreign_repo_id(pool: PgPool) { + let owner = "did:key:zTASKREPOOWNERAAAAAAAAAAAAAAAAAAAAAAAAAA"; + let stranger = "did:key:zTASKREPOSTRANGERBBBBBBBBBBBBBBBBBBBBBB"; + let state = test_state(pool).await; + let repo = seed_repo(owner, "task-gate-repo"); + state.db.create_repo(&repo).await.expect("seed repo"); + + let router = || { + Router::new() + .route( + "/api/v1/tasks", + axum::routing::post(crate::api::tasks::create_task), + ) + .with_state(state.clone()) + }; + let post_as = |signer: &str, body: String| { + router().oneshot(signed_request_as( + signer, + Method::POST, + "/api/v1/tasks", + Body::from(body), + )) + }; + let body_with = |signer: &str, repo_id: Option<&str>| { + let repo_field = repo_id + .map(|id| format!(r#","repo_id":"{id}""#)) + .unwrap_or_default(); + format!( + r#"{{"kind":"build","capability":"repo:write","delegator_did":"{signer}"{repo_field}}}"# + ) + }; + + // Stranger filing under the victim's repo id → exact 403. + let resp = post_as(stranger, body_with(stranger, Some(&repo.id))) + .await + .unwrap(); + assert_eq!( + resp.status(), + StatusCode::FORBIDDEN, + "a non-owner must not file tasks against a foreign repo id" + ); + let bytes = axum::body::to_bytes(resp.into_body(), usize::MAX) + .await + .unwrap(); + let text = String::from_utf8_lossy(&bytes); + assert!( + text.contains(r#""error":"forbidden""#), + "denial must keep the forbidden envelope: {text}" + ); + assert!( + !text.contains(&repo.id) && !text.contains(owner), + "denial must leak nothing about the repo: {text}" + ); + + // Owner filing under their own repo id → 201. + let resp = post_as(owner, body_with(owner, Some(&repo.id))) + .await + .unwrap(); + assert_eq!( + resp.status(), + StatusCode::CREATED, + "the owner must be able to file tasks against their repo" + ); + + // Repo-less task → 201 (unaffected). + let resp = post_as(stranger, body_with(stranger, None)).await.unwrap(); + assert_eq!( + resp.status(), + StatusCode::CREATED, + "repo-less task creation must keep working" + ); + + // Unknown repo id → 201 as an opaque label. + let resp = post_as(stranger, body_with(stranger, Some("no-such-repo-id"))) + .await + .unwrap(); + assert_eq!( + resp.status(), + StatusCode::CREATED, + "an id naming no hosted repo stays an opaque label" + ); + + // Quarantined repo id → treated as unknown (201), never a 403 that + // would confirm the id names a real repo. + state + .db + .set_repo_quarantine(&repo.id, true) + .await + .expect("quarantine"); + let resp = post_as(stranger, body_with(stranger, Some(&repo.id))) + .await + .unwrap(); + assert_eq!( + resp.status(), + StatusCode::CREATED, + "a quarantined repo id must not 403 (existence oracle)" + ); + } + /// N3: get_tree gates on the REQUESTED subtree, not the repo root. A caller /// denied a withheld subtree is rejected there (404) but passes the gate on a /// non-withheld path (so the rejection is path-scoped, not repo-wide). From abc03d65c79f230d5246380159d2fe5c3168eab4 Mon Sep 17 00:00:00 2001 From: Gravirei Date: Thu, 1 Oct 2026 10:02:13 +0600 Subject: [PATCH 2/2] =?UTF-8?q?fix(node):=20address=20review=20on=20#496?= =?UTF-8?q?=20gate=20=E2=80=94=20opaque=20lookup=20errors,=20no-persist=20?= =?UTF-8?q?asserts,=20GraphQL=20arm=20pins,=20honest=20oracle=20comment?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- crates/gitlawb-node/src/api/tasks.rs | 44 ++++++++++-------- crates/gitlawb-node/src/graphql/mutation.rs | 50 ++++++++++++++++++++- crates/gitlawb-node/src/test_support.rs | 14 ++++++ 3 files changed, 88 insertions(+), 20 deletions(-) diff --git a/crates/gitlawb-node/src/api/tasks.rs b/crates/gitlawb-node/src/api/tasks.rs index 9c816326..5a74349b 100644 --- a/crates/gitlawb-node/src/api/tasks.rs +++ b/crates/gitlawb-node/src/api/tasks.rs @@ -30,6 +30,20 @@ fn forbidden(msg: &str) -> (StatusCode, Json) { ) } +/// 500 in this module's error shape with an opaque body: the real error is +/// logged server-side and never serialized (#250), matching the `db_error` +/// envelope `AppError::Db` renders. +fn db_error(e: anyhow::Error) -> (StatusCode, Json) { + tracing::error!(error = %format!("{e:#}"), "task repo gate database error"); + ( + StatusCode::INTERNAL_SERVER_ERROR, + Json(json!({ + "error": "db_error", + "message": crate::error::DB_ERROR_MESSAGE, + })), + ) +} + // ── Request / response types ────────────────────────────────────────────────── #[derive(Deserialize)] @@ -106,30 +120,24 @@ pub async fn create_task( // under a foreign repo id, where repo-gated task reads surface it exactly // to that repo's readers and the named assignee can claim it. Resolve the // id against hosted repos: a hosted, non-quarantined repo admits tasks - // only from its owner. An id naming no hosted repo (unknown, or - // quarantined — which is hidden as if it did not exist, so 403ing it - // would confirm a real id) is kept as an opaque label with no existence - // oracle either way; such ids resolve to no hosted repo, so repo-scoped - // task read gates treat them as unscoped (delegator/assignee-only under - // the #268/#464 visibility contract). Repo-less tasks are unaffected. + // only from its owner (403 otherwise). An id naming no hosted repo + // (unknown, or quarantined — which is hidden as if it did not exist, so + // 403ing it would confirm a real id) is kept as an opaque label; such ids + // resolve to no hosted repo, so repo-scoped task read gates treat them as + // unscoped (delegator/assignee-only under the #268/#464 visibility + // contract). The 403-vs-201 split between a hosted repo id and an unknown + // one is itself a one-bit oracle — a stranger holding a private repo's id + // learns it is hosted here — but denying foreign repos requires drawing + // exactly that line, and repo ids already surface via the task list on + // this base. Repo-less tasks are unaffected. if let Some(repo_id) = body.repo_id.as_deref() { - let record = state.db.get_repo_by_id(repo_id).await.map_err(|e| { - ( - StatusCode::INTERNAL_SERVER_ERROR, - Json(json!({ "error": e.to_string() })), - ) - })?; + let record = state.db.get_repo_by_id(repo_id).await.map_err(db_error)?; if let Some(record) = record { let quarantined = state .db .is_repo_quarantined(&record.id) .await - .map_err(|e| { - ( - StatusCode::INTERNAL_SERVER_ERROR, - Json(json!({ "error": e.to_string() })), - ) - })?; + .map_err(db_error)?; if !quarantined && crate::api::require_repo_owner(&record, &auth.0).is_err() { return Err(forbidden("only the repo owner can file tasks against it")); } diff --git a/crates/gitlawb-node/src/graphql/mutation.rs b/crates/gitlawb-node/src/graphql/mutation.rs index 6da39fa2..aff3faa5 100644 --- a/crates/gitlawb-node/src/graphql/mutation.rs +++ b/crates/gitlawb-node/src/graphql/mutation.rs @@ -355,8 +355,10 @@ mod tests { } /// #496 (GraphQL): createTask applies the same repo-ownership gate as the - /// REST handler — a stranger naming a hosted repo is rejected, while the - /// owner files. + /// REST handler — a stranger naming a hosted repo is rejected without + /// persisting anything, while the owner files. The quarantined and + /// unknown-id arms are pinned here too, so this copy of the gate cannot + /// regress silently. #[sqlx::test] async fn create_task_rejects_foreign_repo_id(pool: PgPool) { let state = crate::test_support::test_state(pool).await; @@ -402,6 +404,20 @@ mod tests { "denial must leak nothing about the repo: {errs}" ); + // The denial must not persist: no task row may carry the victim's + // repo id afterwards. + let stored = state + .db + .list_tasks(None, None, 50) + .await + .expect("list tasks"); + assert!( + stored + .iter() + .all(|t| t.repo_id.as_deref() != Some(repo.id.as_str())), + "a denied write must not leave a task under the foreign repo id" + ); + // The owner files under their own repo id. let resp = schema .execute(Request::new(q(owner)).data(AuthenticatedDid(owner.into()))) @@ -411,5 +427,35 @@ mod tests { "the owner should file against their repo: {}", errors(&resp) ); + + // Unknown repo ids stay accepted as opaque labels. + let q_unknown = format!( + r#"mutation {{ createTask(delegatorDid: "{stranger}", input: {{ kind: "build", capability: "repo:write", repoId: "no-such-repo-id" }}) {{ id }} }}"# + ); + let resp = schema + .execute(Request::new(q_unknown).data(AuthenticatedDid(stranger.into()))) + .await; + assert!( + errors(&resp).is_empty(), + "an id naming no hosted repo stays an opaque label: {}", + errors(&resp) + ); + + // A quarantined repo id is treated as unknown (accepted, never a 403 + // that would confirm the id names a real repo). This pins the + // `!quarantined` arm on this copy of the gate. + state + .db + .set_repo_quarantine(&repo.id, true) + .await + .expect("quarantine"); + let resp = schema + .execute(Request::new(q(stranger)).data(AuthenticatedDid(stranger.into()))) + .await; + assert!( + errors(&resp).is_empty(), + "a quarantined repo id must not 403 (existence oracle): {}", + errors(&resp) + ); } } diff --git a/crates/gitlawb-node/src/test_support.rs b/crates/gitlawb-node/src/test_support.rs index 838d4ffe..387ec587 100644 --- a/crates/gitlawb-node/src/test_support.rs +++ b/crates/gitlawb-node/src/test_support.rs @@ -651,6 +651,20 @@ mod tests { "denial must leak nothing about the repo: {text}" ); + // The denial must not persist: no task row may carry the victim's + // repo id afterwards — a 403 that still writes is the injection. + let stored = state + .db + .list_tasks(None, None, 50) + .await + .expect("list tasks"); + assert!( + stored + .iter() + .all(|t| t.repo_id.as_deref() != Some(repo.id.as_str())), + "a denied write must not leave a task under the foreign repo id" + ); + // Owner filing under their own repo id → 201. let resp = post_as(owner, body_with(owner, Some(&repo.id))) .await