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..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)] @@ -101,6 +115,34 @@ 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 (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(db_error)?; + if let Some(record) = record { + let quarantined = state + .db + .is_repo_quarantined(&record.id) + .await + .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")); + } + } + } 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..aff3faa5 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,109 @@ 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 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; + 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 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()))) + .await; + assert!( + errors(&resp).is_empty(), + "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 430c0600..387ec587 100644 --- a/crates/gitlawb-node/src/test_support.rs +++ b/crates/gitlawb-node/src/test_support.rs @@ -590,6 +590,126 @@ 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}" + ); + + // 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 + .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).