From 64ef51d775251d791af4d42a90687af795055f72 Mon Sep 17 00:00:00 2001 From: Gravirei Date: Tue, 29 Sep 2026 13:28:21 +0600 Subject: [PATCH 1/4] 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/4] =?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 From be55f8bd467b4235eb4712684b59eee06fe5a5c5 Mon Sep 17 00:00:00 2001 From: Gravirei Date: Tue, 27 Oct 2026 07:49:36 +0600 Subject: [PATCH 3/4] =?UTF-8?q?fix(node):=20address=20P3=20review=20on=20#?= =?UTF-8?q?496=20gate=20=E2=80=94=20shared=20denial=20text,=20honest=20ora?= =?UTF-8?q?cle=20wording,=20db-fault=20pins?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Give the REST and GraphQL copies of the create_task owner gate one denial sentence: add REPO_OWNER_ONLY_MESSAGE, used by require_repo_owner and the REST forbidden() envelope, and pin it from both surface tests. - Carry the corrected one-bit-oracle wording into the GraphQL gate comment and the REST test docstring (drop the "oracle-free" claim). - Fault-inject the gate's repo lookups: drop machine_id (get_repo_by_id) for REST and GraphQL, and quarantined (is_repo_quarantined) for REST, asserting the fixed opaque db_error envelope and no schema text. --- crates/gitlawb-node/src/api/mod.rs | 11 +- crates/gitlawb-node/src/api/tasks.rs | 5 +- crates/gitlawb-node/src/graphql/mutation.rs | 64 ++++++++- crates/gitlawb-node/src/test_support.rs | 136 +++++++++++++++++++- 4 files changed, 204 insertions(+), 12 deletions(-) diff --git a/crates/gitlawb-node/src/api/mod.rs b/crates/gitlawb-node/src/api/mod.rs index 79589939..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())) } } diff --git a/crates/gitlawb-node/src/api/tasks.rs b/crates/gitlawb-node/src/api/tasks.rs index 5a74349b..6d6f8f2c 100644 --- a/crates/gitlawb-node/src/api/tasks.rs +++ b/crates/gitlawb-node/src/api/tasks.rs @@ -139,7 +139,10 @@ pub async fn create_task( .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")); + // 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)); } } } diff --git a/crates/gitlawb-node/src/graphql/mutation.rs b/crates/gitlawb-node/src/graphql/mutation.rs index aff3faa5..e541d710 100644 --- a/crates/gitlawb-node/src/graphql/mutation.rs +++ b/crates/gitlawb-node/src/graphql/mutation.rs @@ -38,9 +38,14 @@ impl MutationRoot { 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. + // 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) stays an opaque label, resolving to no hosted repo + // and thus unscoped under the #268/#464 read contract; repo-less + // tasks unaffected. 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) @@ -396,8 +401,8 @@ mod tests { .await; let errs = errors(&resp); assert!( - errs.contains("repo owner"), - "a non-owner must not file under a foreign repo id: {errs}" + 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), @@ -458,4 +463,53 @@ mod tests { 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 387ec587..cc81dd82 100644 --- a/crates/gitlawb-node/src/test_support.rs +++ b/crates/gitlawb-node/src/test_support.rs @@ -593,9 +593,11 @@ 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). + /// unknown-id tasks stay allowed (unknown ids are opaque labels, unscoped + /// under the #268/#464 read contract); and a quarantined repo id is treated + /// as unknown (no 403 that would confirm it names a real repo). As the + /// handler comment records, the 403-vs-201 split between a hosted id and an + /// unknown one is itself a one-bit oracle. #[sqlx::test] async fn create_task_rejects_foreign_repo_id(pool: PgPool) { let owner = "did:key:zTASKREPOOWNERAAAAAAAAAAAAAAAAAAAAAAAAAA"; @@ -646,6 +648,10 @@ mod tests { 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}" @@ -710,6 +716,130 @@ mod tests { ); } + /// #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). From 37764c7ddb3fc249eab0194b3e33fb9c4ae9765f Mon Sep 17 00:00:00 2001 From: Gravirei Date: Tue, 27 Oct 2026 08:10:40 +0600 Subject: [PATCH 4/4] docs(node): stop claiming caller-scoped task reads in #496 gate comments The create_task gate is a write gate. Unknown/quarantined repo ids are stored verbatim, and the REST/GraphQL task-list paths are caller-independent on this base, so the comments must not imply those ids are read-scoped to the delegator/assignee. Describe only the enforced ownership check; note the one-bit oracle split without asserting a read contract. --- crates/gitlawb-node/src/api/tasks.rs | 22 ++++++++++----------- crates/gitlawb-node/src/graphql/mutation.rs | 21 ++++++++++---------- crates/gitlawb-node/src/test_support.rs | 16 +++++++-------- 3 files changed, 30 insertions(+), 29 deletions(-) diff --git a/crates/gitlawb-node/src/api/tasks.rs b/crates/gitlawb-node/src/api/tasks.rs index 6d6f8f2c..10a2c826 100644 --- a/crates/gitlawb-node/src/api/tasks.rs +++ b/crates/gitlawb-node/src/api/tasks.rs @@ -117,19 +117,19 @@ pub async fn create_task( } // #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 + // 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 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. + // 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 { diff --git a/crates/gitlawb-node/src/graphql/mutation.rs b/crates/gitlawb-node/src/graphql/mutation.rs index e541d710..dedc8d9b 100644 --- a/crates/gitlawb-node/src/graphql/mutation.rs +++ b/crates/gitlawb-node/src/graphql/mutation.rs @@ -36,16 +36,17 @@ 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. An id - // naming no hosted repo (unknown, or quarantined — hidden as if it - // did not exist) stays an opaque label, resolving to no hosted repo - // and thus unscoped under the #268/#464 read contract; repo-less - // tasks unaffected. 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. + // #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) diff --git a/crates/gitlawb-node/src/test_support.rs b/crates/gitlawb-node/src/test_support.rs index cc81dd82..b1523535 100644 --- a/crates/gitlawb-node/src/test_support.rs +++ b/crates/gitlawb-node/src/test_support.rs @@ -590,14 +590,14 @@ 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 opaque labels, unscoped - /// under the #268/#464 read contract); and a quarantined repo id is treated - /// as unknown (no 403 that would confirm it names a real repo). As the - /// handler comment records, the 403-vs-201 split between a hosted id and an - /// unknown one is itself a one-bit oracle. + /// #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";