diff --git a/crates/gitlawb-node/src/api/mod.rs b/crates/gitlawb-node/src/api/mod.rs index df10175a..b5a428ad 100644 --- a/crates/gitlawb-node/src/api/mod.rs +++ b/crates/gitlawb-node/src/api/mod.rs @@ -80,15 +80,20 @@ pub(crate) fn did_matches(a: &str, b: &str) -> bool { !ka.contains(':') && !kb.contains(':') && ka == kb } +/// Client-facing denial shared by every repo-owner gate copy. `require_repo_owner` +/// renders it as `AppError::Forbidden`; a handler that emits its own +/// `(StatusCode, Json)` envelope instead of `AppError` (e.g. +/// `api::tasks::create_task`) must use this constant too, so two copies of one +/// gate cannot drift in observable output. +pub(crate) const REPO_OWNER_ONLY_MESSAGE: &str = "only the repo owner can perform this action"; + /// 403 unless `caller` is the repo owner. Uses [`did_matches`] so the owner check /// and the author check (close policy) share one normalization. pub(crate) fn require_repo_owner(record: &RepoRecord, caller: &str) -> Result<()> { if did_matches(caller, &record.owner_did) { Ok(()) } else { - Err(AppError::Forbidden( - "only the repo owner can perform this action".into(), - )) + Err(AppError::Forbidden(REPO_OWNER_ONLY_MESSAGE.into())) } } @@ -223,6 +228,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..10a2c826 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,37 @@ 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 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 stored verbatim as an opaque + // label: this is a write gate and does not bind an unresolved id to a + // repo. Task reads are caller-independent on this base (the list endpoints + // are not repo-scoped), before and after this change. 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() { + // Same denial text the GraphQL copy relays from + // `require_repo_owner`, so the two copies of this gate do not + // disagree in observable output. + return Err(forbidden(crate::api::REPO_OWNER_ONLY_MESSAGE)); + } + } + } 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..dedc8d9b 100644 --- a/crates/gitlawb-node/src/graphql/mutation.rs +++ b/crates/gitlawb-node/src/graphql/mutation.rs @@ -36,6 +36,33 @@ impl MutationRoot { } let delegator_did = caller.to_string(); let db = ctx.data_unchecked::>(); + // #496: same write 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. An id naming no hosted + // repo (unknown, or quarantined — hidden as if it did not exist) is + // stored verbatim as an opaque label; the gate does not bind an + // unresolved id to a repo. Task reads are caller-independent on this + // base (the tasks resolver is not repo-scoped), before and after this + // change. As in tasks.rs, the success-vs-denial 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. + 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 +359,158 @@ 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(crate::api::REPO_OWNER_ONLY_MESSAGE), + "a non-owner must not file under a foreign repo id (shared denial text): {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) + ); + } + + /// #496 (GraphQL): a repo-lookup fault in createTask's gate must stay + /// opaque. Dropping a column `get_repo_by_id` selects faults that lookup; + /// the resolver must return the fixed DB message and leak no schema text + /// (#250), as `create_task_db_error_message_is_opaque` pins for the + /// agent_tasks fault. + #[sqlx::test] + async fn create_task_gate_repo_lookup_fault_is_opaque(pool: PgPool) { + let state = crate::test_support::test_state(pool.clone()).await; + let owner = "did:key:zGQLTASKFAULTOWNERAAAAAAAAAAAAAAAAAAAAAAAA"; + let now = chrono::Utc::now(); + let repo = crate::db::RepoRecord { + id: uuid::Uuid::new_v4().to_string(), + name: "gql-task-gate-fault-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-fault-repo".to_string(), + forked_from: None, + machine_id: None, + }; + state.db.create_repo(&repo).await.expect("seed repo"); + + sqlx::query("ALTER TABLE repos DROP COLUMN machine_id") + .execute(&pool) + .await + .expect("drop machine_id"); + + let q = format!( + r#"mutation {{ createTask(delegatorDid: "{owner}", input: {{ kind: "build", capability: "repo:write", repoId: "{}" }}) {{ id }} }}"#, + repo.id + ); + let resp = state + .graphql_schema + .execute(Request::new(&q).data(AuthenticatedDid(owner.into()))) + .await; + let errs = errors(&resp); + assert!( + errs.contains(crate::graphql::GRAPHQL_DB_ERROR_MESSAGE), + "gate sqlx fault must be opaque: {errs}" + ); + assert!( + !errs.contains("machine_id") && !errs.contains("column"), + "schema text leaked through the gate: {errs}" + ); + } } diff --git a/crates/gitlawb-node/src/test_support.rs b/crates/gitlawb-node/src/test_support.rs index 430c0600..b1523535 100644 --- a/crates/gitlawb-node/src/test_support.rs +++ b/crates/gitlawb-node/src/test_support.rs @@ -590,6 +590,256 @@ mod tests { ); } + /// #496: create_task must reject a caller-supplied repo_id that names a + /// hosted repo the caller does not own. A stranger naming the victim's repo + /// gets 403, leaking nothing about the repo and persisting nothing; the + /// owner files (201). Repo-less tasks stay allowed; an id naming no hosted + /// repo (unknown, or quarantined, hidden as if it did not exist) is stored + /// verbatim as an opaque label, and the 403-vs-201 split between a hosted + /// id and an unknown one is itself a one-bit oracle. This is a write gate: + /// task reads are caller-independent and not repo-scoped on this base. + #[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(crate::api::REPO_OWNER_ONLY_MESSAGE), + "denial must carry the shared owner-denial text (same as GraphQL): {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)" + ); + } + + /// #496 (REST): a repo-lookup fault in the create_task gate must render the + /// module's opaque `db_error` envelope, not the raw sqlx text. Dropping a + /// column `get_repo_by_id` selects faults that first lookup; the 500 body + /// stays fixed and names no column (#250/#226). + #[sqlx::test] + async fn create_task_gate_repo_lookup_fault_is_opaque(pool: PgPool) { + let owner = "did:key:zTASKFAULTOWNERAAAAAAAAAAAAAAAAAAAAAAAAA"; + let state = test_state(pool.clone()).await; + let repo = seed_repo(owner, "task-gate-fault-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 body = format!( + r#"{{"kind":"build","capability":"repo:write","delegator_did":"{owner}","repo_id":"{}"}}"#, + repo.id + ); + + // Drop a column the lookup's SELECT names so get_repo_by_id faults. + sqlx::query("ALTER TABLE repos DROP COLUMN machine_id") + .execute(&pool) + .await + .expect("drop machine_id"); + + let resp = router() + .oneshot(signed_request_as( + owner, + Method::POST, + "/api/v1/tasks", + Body::from(body), + )) + .await + .unwrap(); + assert_eq!( + resp.status(), + StatusCode::INTERNAL_SERVER_ERROR, + "a faulted repo lookup must be a 500" + ); + 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":"db_error""#), + "gate DB fault must keep the opaque envelope: {text}" + ); + assert!( + text.contains(crate::error::DB_ERROR_MESSAGE), + "gate DB fault must carry the fixed message: {text}" + ); + assert!( + !text.contains("machine_id") && !text.contains("column"), + "schema text leaked through the gate: {text}" + ); + } + + /// #496 (REST): the gate's second lookup (`is_repo_quarantined`) runs through + /// the same opaque helper. Dropping the column only it selects leaves + /// `get_repo_by_id` succeeding, so this pins that second call site too. + #[sqlx::test] + async fn create_task_gate_quarantine_lookup_fault_is_opaque(pool: PgPool) { + let owner = "did:key:zTASKFAULT2OWNERAAAAAAAAAAAAAAAAAAAAAAAA"; + let state = test_state(pool.clone()).await; + let repo = seed_repo(owner, "task-gate-fault2-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 body = format!( + r#"{{"kind":"build","capability":"repo:write","delegator_did":"{owner}","repo_id":"{}"}}"#, + repo.id + ); + + // `quarantined` is not in get_repo_by_id's SELECT, so that lookup still + // succeeds and the fault lands on is_repo_quarantined. + sqlx::query("ALTER TABLE repos DROP COLUMN quarantined") + .execute(&pool) + .await + .expect("drop quarantined"); + + let resp = router() + .oneshot(signed_request_as( + owner, + Method::POST, + "/api/v1/tasks", + Body::from(body), + )) + .await + .unwrap(); + assert_eq!( + resp.status(), + StatusCode::INTERNAL_SERVER_ERROR, + "a faulted quarantine lookup must be a 500" + ); + 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":"db_error""#), + "quarantine DB fault must keep the opaque envelope: {text}" + ); + assert!( + text.contains(crate::error::DB_ERROR_MESSAGE), + "quarantine DB fault must carry the fixed message: {text}" + ); + assert!( + !text.contains("quarantined") && !text.contains("column"), + "schema text leaked through the gate: {text}" + ); + } + /// 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).