From 1649f8909e0c2ca51e202d31de4d7667778006e8 Mon Sep 17 00:00:00 2001 From: euxaristia Date: Wed, 12 Aug 2026 15:45:57 -0400 Subject: [PATCH 01/17] fix(node): gate agent-task reads behind the same visibility rules as repo data list_tasks and get_task had no authorization at all: any anonymous caller could enumerate every task on the node, including another party's repo-less task, its ucan_token, and its payload (#268). Add task_visible, mirroring the repo read-visibility gate already used by the ref-updates feed: the delegator and assignee can always read their own task, a repo-scoped task follows that repo's normal visibility rules, and a task naming no repo (or a repo this node doesn't host) is visible only to its delegator/assignee. Both REST and GraphQL now route through the same collect_visible_tasks/get_visible_task collectors so the two surfaces cannot drift, and neither read path echoes ucan_token back, since the holder already received it via the create/claim response. Fixes #268 --- crates/gitlawb-node/src/api/tasks.rs | 472 ++++++++++++++++++++++- crates/gitlawb-node/src/graphql/query.rs | 45 ++- crates/gitlawb-node/src/graphql/types.rs | 41 ++ crates/gitlawb-node/src/server.rs | 10 +- 4 files changed, 536 insertions(+), 32 deletions(-) diff --git a/crates/gitlawb-node/src/api/tasks.rs b/crates/gitlawb-node/src/api/tasks.rs index de22134a..96f084e5 100644 --- a/crates/gitlawb-node/src/api/tasks.rs +++ b/crates/gitlawb-node/src/api/tasks.rs @@ -8,6 +8,8 @@ //! POST /api/v1/tasks/{id}/complete — complete task //! POST /api/v1/tasks/{id}/fail — fail task +use std::collections::HashMap; + use axum::{ extract::{Extension, Path, Query, State}, http::StatusCode, @@ -19,7 +21,7 @@ use serde_json::{json, Value}; use uuid::Uuid; use crate::auth::AuthenticatedDid; -use crate::db::AgentTask; +use crate::db::{AgentTask, RepoRecord, VisibilityRule}; use crate::state::{AppState, TaskEventBroadcast}; /// 403 in this module's error shape (`(StatusCode, Json)`, not `AppError`). @@ -89,6 +91,145 @@ fn task_to_json(t: &AgentTask) -> Value { }) } +/// Same projection as `task_to_json`, minus `ucan_token` (#268). The read +/// surfaces (`list_tasks`, `get_task`) never need to echo it back to anyone, +/// including the delegator/assignee: it was handed to the assignee at +/// delegation/claim time via the write-side responses, which still use +/// `task_to_json` unchanged. +fn task_to_read_json(t: &AgentTask) -> Value { + json!({ + "id": t.id, + "repo_id": t.repo_id, + "kind": t.kind, + "status": t.status, + "delegator_did": t.delegator_did, + "assignee_did": t.assignee_did, + "capability": t.capability, + "payload": t.payload, + "result": t.result, + "created_at": t.created_at, + "updated_at": t.updated_at, + "deadline": t.deadline, + }) +} + +/// Hard ceiling on rows a task read surface fetches for one request, mirroring +/// `MAX_VISIBLE_REF_UPDATES` in `api/events.rs` (#112/#114) for the same +/// reason: bound the underlying query before an unauthenticated caller's +/// request size controls how much the visibility filter has to scan. +const MAX_VISIBLE_TASKS: i64 = 200; + +/// Whether `task` should be visible to `caller` (`None` = anonymous). +/// +/// The delegator and assignee can always read a task they are already party +/// to — they hold its `payload` (and held `ucan_token`, though reads never +/// echo it back) from creating or being assigned it. Otherwise, a task naming +/// a locally-hosted repo follows that repo's normal read gate, the same way +/// `ref_update_row_visible` (`visibility.rs`) drops a ref-update row for a +/// repo the caller can't read. A task with no `repo_id`, or naming a repo this +/// node does not host, is visible only to its delegator/assignee — fail +/// closed, since an open-to-everyone default is exactly the gap #268 found +/// (`GET /api/v1/tasks` and `/tasks/{id}` had no gate at all). +pub(crate) fn task_visible( + task: &AgentTask, + caller: Option<&str>, + repos_by_id: &HashMap, + rules_by_repo: &HashMap>, +) -> bool { + if let Some(c) = caller { + if crate::api::did_matches(c, &task.delegator_did) { + return true; + } + let assignee_match = task + .assignee_did + .as_deref() + .map(|a| crate::api::did_matches(c, a)) + .unwrap_or(false); + if assignee_match { + return true; + } + } + let Some(record) = task.repo_id.as_deref().and_then(|id| repos_by_id.get(id)) else { + return false; + }; + let rules = rules_by_repo + .get(&record.id) + .map(Vec::as_slice) + .unwrap_or(&[]); + crate::visibility::listable_at_root(rules, record.is_public, &record.owner_did, caller) +} + +/// Collect up to `limit` tasks visible to `caller`, applying the same gate the +/// GraphQL `tasks` query uses (`collect_visible_tasks` is called from both) so +/// the two surfaces cannot drift, matching the `collect_visible_ref_updates` +/// pattern in `api/events.rs`. `limit` is clamped here so a caller-supplied +/// value never reaches SQL unclamped. +/// +/// Unlike the ref-updates feed, this does not page past invisible rows: it +/// fetches one bounded page and filters it, so a request whose newest +/// `MAX_VISIBLE_TASKS` rows are mostly invisible to the caller can return +/// fewer rows than are truly visible further back. Accepted here because, +/// unlike the cross-tenant ref-updates feed, task queries are already scoped +/// by `status`/`assignee_did` up front, which keeps the visible/invisible mix +/// per query far narrower in practice. +pub(crate) async fn collect_visible_tasks( + db: &crate::db::Db, + status: Option<&str>, + assignee_did: Option<&str>, + limit: i64, + caller: Option<&str>, +) -> crate::error::Result> { + let bounded_limit = limit.clamp(0, MAX_VISIBLE_TASKS); + if bounded_limit == 0 { + return Ok(Vec::new()); + } + let tasks = db.list_tasks(status, assignee_did, bounded_limit).await?; + if tasks.is_empty() { + return Ok(tasks); + } + let repos = db.list_all_repos_deduped().await?; + let repos_by_id: HashMap = + repos.into_iter().map(|r| (r.id.clone(), r)).collect(); + let ids: Vec = repos_by_id.keys().cloned().collect(); + let rules_by_repo = db.list_visibility_rules_for_repos(&ids).await?; + Ok(tasks + .into_iter() + .filter(|t| task_visible(t, caller, &repos_by_id, &rules_by_repo)) + .collect()) +} + +/// Fetch a single task gated the same way `collect_visible_tasks` gates a +/// page. Returns `None` both when the task does not exist and when the caller +/// may not see it — the two are indistinguishable to the caller, matching +/// `authorize_repo_read`'s opaque not-found-vs-denied handling, so an +/// unauthorized caller cannot use this to probe which task IDs exist. +pub(crate) async fn get_visible_task( + db: &crate::db::Db, + id: &str, + caller: Option<&str>, +) -> crate::error::Result> { + let Some(task) = db.get_task(id).await? else { + return Ok(None); + }; + let (repos_by_id, rules_by_repo) = match task.repo_id.as_deref() { + Some(repo_id) => { + let repos = db.list_all_repos_deduped().await?; + match repos.into_iter().find(|r| r.id == repo_id) { + Some(record) => { + let rules = db.list_visibility_rules(&record.id).await?; + ( + HashMap::from([(record.id.clone(), record)]), + HashMap::from([(repo_id.to_string(), rules)]), + ) + } + None => (HashMap::new(), HashMap::new()), + } + } + None => (HashMap::new(), HashMap::new()), + }; + Ok(task_visible(&task, caller, &repos_by_id, &rules_by_repo).then_some(task)) +} + // ── Handlers ────────────────────────────────────────────────────────────────── /// POST /api/v1/tasks @@ -127,31 +268,46 @@ pub async fn create_task( } /// GET /api/v1/tasks +/// +/// Open to anonymous callers, but every row is gated by `collect_visible_tasks` +/// (#268): an anonymous or unrelated caller only sees tasks against a repo they +/// can read, never another party's repo-less task or its `ucan_token`/`payload`. pub async fn list_tasks( State(state): State, Query(q): Query, + auth: Option>, ) -> Result, (StatusCode, Json)> { - let tasks = state - .db - .list_tasks(q.status.as_deref(), q.assignee_did.as_deref(), q.limit) - .await - .map_err(|e| { - ( - StatusCode::INTERNAL_SERVER_ERROR, - Json(json!({ "error": e.to_string() })), - ) - })?; - let items: Vec = tasks.iter().map(task_to_json).collect(); + let caller = auth.as_ref().map(|e| e.0 .0.as_str()); + let tasks = collect_visible_tasks( + &state.db, + q.status.as_deref(), + q.assignee_did.as_deref(), + q.limit, + caller, + ) + .await + .map_err(|e| { + ( + StatusCode::INTERNAL_SERVER_ERROR, + Json(json!({ "error": e.to_string() })), + ) + })?; + let items: Vec = tasks.iter().map(task_to_read_json).collect(); Ok(Json(json!({ "tasks": items, "count": items.len() }))) } /// GET /api/v1/tasks/{id} +/// +/// Gated the same way as `list_tasks` (#268): a task the caller may not see +/// 404s, indistinguishable from a task that doesn't exist. pub async fn get_task( State(state): State, Path(id): Path, + auth: Option>, ) -> Result, (StatusCode, Json)> { - match state.db.get_task(&id).await { - Ok(Some(t)) => Ok(Json(task_to_json(&t))), + let caller = auth.as_ref().map(|e| e.0 .0.as_str()); + match get_visible_task(&state.db, &id, caller).await { + Ok(Some(t)) => Ok(Json(task_to_read_json(&t))), Ok(None) => Err(( StatusCode::NOT_FOUND, Json(json!({ "error": "task not found" })), @@ -297,3 +453,291 @@ pub async fn fail_task( }); Ok(Json(task_to_json(&task))) } + +#[cfg(test)] +mod visible_tasks_tests { + use super::*; + use crate::test_support::{signed_request_as, test_state}; + use axum::body::Body; + use axum::http::{Method, Request, StatusCode}; + use axum::Router; + use chrono::Utc; + use sqlx::PgPool; + use tower::ServiceExt; + + const DELEGATOR: &str = "did:key:z6MkDelegator"; + const ASSIGNEE: &str = "did:key:z6MkAssignee"; + const STRANGER: &str = "did:key:z6MkStranger"; + const SECRET_UCAN: &str = "SECRET-UCAN-TOKEN"; + + fn repo(id: &str, owner_did: &str, name: &str, is_public: bool) -> RepoRecord { + let now = Utc::now(); + RepoRecord { + id: id.into(), + name: name.into(), + owner_did: owner_did.into(), + description: None, + is_public, + default_branch: "main".into(), + created_at: now, + updated_at: now, + disk_path: format!("/tmp/{id}"), + forked_from: None, + machine_id: None, + } + } + + fn task(id: &str, repo_id: Option<&str>, delegator: &str) -> AgentTask { + let now = Utc::now().to_rfc3339(); + AgentTask { + id: id.into(), + repo_id: repo_id.map(String::from), + kind: "build".into(), + status: "pending".into(), + delegator_did: delegator.into(), + assignee_did: None, + capability: "repo:write".into(), + ucan_token: Some(SECRET_UCAN.into()), + payload: Some("payload-data".into()), + result: None, + created_at: now.clone(), + updated_at: now, + deadline: None, + } + } + + fn list_router(state: crate::state::AppState) -> Router { + Router::new() + .route("/api/v1/tasks", axum::routing::get(super::list_tasks)) + .route("/api/v1/tasks/{id}", axum::routing::get(super::get_task)) + .with_state(state) + } + + fn anon_get(uri: &str) -> Request { + Request::builder() + .method(Method::GET) + .uri(uri) + .body(Body::empty()) + .expect("request builder") + } + + async fn body_json(resp: axum::response::Response) -> serde_json::Value { + let bytes = axum::body::to_bytes(resp.into_body(), usize::MAX) + .await + .expect("body bytes"); + serde_json::from_slice(&bytes).expect("json body") + } + + /// #268 — load-bearing RED→GREEN: before this fix, `list_tasks`/`get_task` + /// had no gate at all, so an anonymous caller could enumerate every task on + /// the node, including another party's repo-less task, its `ucan_token`, + /// and its `payload`. An anonymous caller must now see neither. + #[sqlx::test] + async fn anon_cannot_list_or_read_repo_less_task_of_another(pool: PgPool) { + let state = test_state(pool).await; + state + .db + .create_task(&task("t1", None, DELEGATOR)) + .await + .unwrap(); + + let resp = list_router(state.clone()) + .oneshot(anon_get("/api/v1/tasks")) + .await + .unwrap(); + let body = body_json(resp).await; + assert_eq!( + body["tasks"].as_array().unwrap().len(), + 0, + "anon must not see another party's repo-less task" + ); + assert_eq!(body["count"], 0); + + let resp = list_router(state) + .oneshot(anon_get("/api/v1/tasks/t1")) + .await + .unwrap(); + assert_eq!( + resp.status(), + StatusCode::NOT_FOUND, + "anon get_task on an invisible task must 404, not leak it" + ); + } + + /// The delegator can always read their own repo-less task — the party who + /// created it is not locked out by the new gate. + #[sqlx::test] + async fn delegator_sees_own_repo_less_task(pool: PgPool) { + let state = test_state(pool).await; + state + .db + .create_task(&task("t1", None, DELEGATOR)) + .await + .unwrap(); + + let resp = list_router(state.clone()) + .oneshot(signed_request_as( + DELEGATOR, + Method::GET, + "/api/v1/tasks", + Body::empty(), + )) + .await + .unwrap(); + let body = body_json(resp).await; + assert_eq!(body["tasks"].as_array().unwrap().len(), 1); + + let resp = list_router(state) + .oneshot(signed_request_as( + DELEGATOR, + Method::GET, + "/api/v1/tasks/t1", + Body::empty(), + )) + .await + .unwrap(); + assert_eq!(resp.status(), StatusCode::OK); + } + + /// The assignee can read a task they were assigned, even though they are + /// not its delegator. + #[sqlx::test] + async fn assignee_sees_assigned_repo_less_task(pool: PgPool) { + let state = test_state(pool).await; + state + .db + .create_task(&task("t1", None, DELEGATOR)) + .await + .unwrap(); + state.db.claim_task("t1", ASSIGNEE).await.unwrap(); + + let resp = list_router(state) + .oneshot(signed_request_as( + ASSIGNEE, + Method::GET, + "/api/v1/tasks/t1", + Body::empty(), + )) + .await + .unwrap(); + assert_eq!(resp.status(), StatusCode::OK); + } + + /// #268 — `ucan_token` must never appear on the read surfaces, even to the + /// delegator who legitimately holds it: they already received it via the + /// write-side `create_task` response, so a read echo is unnecessary + /// exposure, not a feature. + #[sqlx::test] + async fn ucan_token_never_appears_in_read_responses(pool: PgPool) { + let state = test_state(pool).await; + state + .db + .create_task(&task("t1", None, DELEGATOR)) + .await + .unwrap(); + + let resp = list_router(state.clone()) + .oneshot(signed_request_as( + DELEGATOR, + Method::GET, + "/api/v1/tasks/t1", + Body::empty(), + )) + .await + .unwrap(); + let body = body_json(resp).await; + assert!( + body.get("ucan_token").is_none(), + "get_task must never echo ucan_token, got {body:?}" + ); + + let resp = list_router(state) + .oneshot(signed_request_as( + DELEGATOR, + Method::GET, + "/api/v1/tasks", + Body::empty(), + )) + .await + .unwrap(); + let body = body_json(resp).await; + assert!( + !body.to_string().contains(SECRET_UCAN), + "list_tasks must never echo ucan_token, got {body:?}" + ); + } + + /// A repo-scoped task inherits that repo's read-visibility gate: hidden + /// from a stranger, visible to the repo owner even though the owner is + /// neither the task's delegator nor its assignee. + #[sqlx::test] + async fn repo_scoped_private_task_follows_repo_visibility(pool: PgPool) { + const OWNER: &str = "did:key:z6MkRepoOwner"; + const OTHER_DELEGATOR: &str = "did:key:z6MkOtherDelegator"; + let state = test_state(pool).await; + state + .db + .create_repo(&repo("r1", OWNER, "priv", false)) + .await + .unwrap(); + state + .db + .create_task(&task("t1", Some("r1"), OTHER_DELEGATOR)) + .await + .unwrap(); + + let resp = list_router(state.clone()) + .oneshot(signed_request_as( + STRANGER, + Method::GET, + "/api/v1/tasks/t1", + Body::empty(), + )) + .await + .unwrap(); + assert_eq!( + resp.status(), + StatusCode::NOT_FOUND, + "a stranger must not see a private repo's task" + ); + + let resp = list_router(state) + .oneshot(signed_request_as( + OWNER, + Method::GET, + "/api/v1/tasks/t1", + Body::empty(), + )) + .await + .unwrap(); + assert_eq!( + resp.status(), + StatusCode::OK, + "the repo owner must see the task via the repo's read gate" + ); + } + + /// A negative limit must clamp to zero through `collect_visible_tasks`, + /// not fall through to the visible set. + #[sqlx::test] + async fn negative_limit_returns_empty(pool: PgPool) { + let state = test_state(pool).await; + state + .db + .create_task(&task("t1", None, DELEGATOR)) + .await + .unwrap(); + + let resp = list_router(state) + .oneshot(signed_request_as( + DELEGATOR, + Method::GET, + "/api/v1/tasks?limit=-1", + Body::empty(), + )) + .await + .unwrap(); + let body = body_json(resp).await; + assert_eq!(body["count"], 0, "negative limit must clamp to 0"); + } +} diff --git a/crates/gitlawb-node/src/graphql/query.rs b/crates/gitlawb-node/src/graphql/query.rs index 84d7540b..17423aad 100644 --- a/crates/gitlawb-node/src/graphql/query.rs +++ b/crates/gitlawb-node/src/graphql/query.rs @@ -3,7 +3,7 @@ use std::sync::Arc; use crate::db::Db; -use super::types::{AgentTaskType, RefUpdateType, RepoType}; +use super::types::{AgentTaskReadType, RefUpdateType, RepoType}; pub struct QueryRoot; @@ -115,26 +115,39 @@ impl QueryRoot { desc = "Max 200; larger requests are clamped to 200 (no error). Negative values clamp to 0." )] limit: i64, - ) -> Result> { + ) -> Result> { let db = ctx.data_unchecked::>(); - // Clamp before SQL: a negative LIMIT is a client fault that Postgres - // rejects with 2201W, which would otherwise trip the opaque DB path - // and write an error-level log on every probe (#250 review). - let limit = limit.clamp(0, 200); - let tasks = db - .list_tasks(status.as_deref(), assignee_did.as_deref(), limit) - .await - .map_err(crate::graphql::graphql_db_err)?; - Ok(tasks.into_iter().map(AgentTaskType::from).collect()) + // #268: gate rows via the same collector the REST list route uses (like + // `ref_updates` shares `collect_visible_ref_updates` with its REST feed), + // so the two surfaces cannot drift. The collector clamps `limit` itself, + // including the negative-LIMIT case #250 called out for this resolver. + let caller = ctx + .data::() + .ok() + .map(|d| d.0.as_str()); + let tasks = crate::api::tasks::collect_visible_tasks( + db, + status.as_deref(), + assignee_did.as_deref(), + limit, + caller, + ) + .await + .map_err(crate::graphql::graphql_app_err)?; + Ok(tasks.into_iter().map(AgentTaskReadType::from).collect()) } - async fn task(&self, ctx: &Context<'_>, id: String) -> Result> { + async fn task(&self, ctx: &Context<'_>, id: String) -> Result> { let db = ctx.data_unchecked::>(); - let t = db - .get_task(&id) + // #268: same gate as the REST get route, via the shared helper. + let caller = ctx + .data::() + .ok() + .map(|d| d.0.as_str()); + let t = crate::api::tasks::get_visible_task(db, &id, caller) .await - .map_err(crate::graphql::graphql_db_err)?; - Ok(t.map(AgentTaskType::from)) + .map_err(crate::graphql::graphql_app_err)?; + Ok(t.map(AgentTaskReadType::from)) } } diff --git a/crates/gitlawb-node/src/graphql/types.rs b/crates/gitlawb-node/src/graphql/types.rs index 4264a581..ab806aed 100644 --- a/crates/gitlawb-node/src/graphql/types.rs +++ b/crates/gitlawb-node/src/graphql/types.rs @@ -48,6 +48,47 @@ impl From for AgentTaskType { } } +/// Read-only projection of `AgentTask` for the `tasks`/`task` queries, as +/// opposed to `AgentTaskType`, which the task mutations (`createTask`, +/// `claimTask`, `completeTask`, `failTask` — all `require_signer`-gated) +/// return. Identical except for the missing `ucan_token` (#268): a read +/// surface never needs to echo it back, since the assignee already received it +/// at delegation/claim time via the mutation response. +#[derive(SimpleObject, Clone)] +pub struct AgentTaskReadType { + pub id: String, + pub repo_id: Option, + pub kind: String, + pub status: String, + pub delegator_did: String, + pub assignee_did: Option, + pub capability: String, + pub payload: Option, + pub result: Option, + pub created_at: String, + pub updated_at: String, + pub deadline: Option, +} + +impl From for AgentTaskReadType { + fn from(t: AgentTask) -> Self { + Self { + id: t.id, + repo_id: t.repo_id, + kind: t.kind, + status: t.status, + delegator_did: t.delegator_did, + assignee_did: t.assignee_did, + capability: t.capability, + payload: t.payload, + result: t.result, + created_at: t.created_at, + updated_at: t.updated_at, + deadline: t.deadline, + } + } +} + #[derive(SimpleObject, Clone)] pub struct RefUpdateType { pub repo: String, diff --git a/crates/gitlawb-node/src/server.rs b/crates/gitlawb-node/src/server.rs index de61fcbe..f1b3f581 100644 --- a/crates/gitlawb-node/src/server.rs +++ b/crates/gitlawb-node/src/server.rs @@ -76,10 +76,16 @@ pub fn build_router(state: AppState) -> Router { state.clone(), ); - // ── Task routes (read — open) ────────────────────────────────────────── + // ── Task routes (read — open, but scoped) ────────────────────────────── + // `optional_signature` attaches the verified DID when a signature is present + // so the handlers can identify the caller; the routes stay anonymous-reachable, + // but each task/row is gated to its delegator, its assignee, or (for a + // repo-scoped task) whoever can read that repo (#268 — these routes previously + // carried no gate and no identity at all). let task_read_routes = Router::new() .route("/api/v1/tasks", get(tasks::list_tasks)) - .route("/api/v1/tasks/{id}", get(tasks::get_task)); + .route("/api/v1/tasks/{id}", get(tasks::get_task)) + .layer(middleware::from_fn(auth::optional_signature)); // ── Rate-limited creation routes — require HTTP Signature, plus a per-DID // throttle AND a per-IP flood brake. The per-DID limiter (inner) caps a From 499c19d248bb02fc6f1be278f155b6ea49f85bdd Mon Sep 17 00:00:00 2001 From: euxaristia Date: Wed, 12 Aug 2026 18:40:10 -0400 Subject: [PATCH 02/17] fix(node): query the task limit-clamp test as the delegator tasks_limit_ceiling_clamped_to_200 seeded 201 repo-less tasks and read them back anonymously, expecting all 200. That read is exactly the enumeration #268 closes, so the new visibility gate correctly returns none of them and the test went red. The clamp ceiling is what this test pins, not the gate, so query as the tasks' delegator, who can legitimately see all 201 rows. Refs #268 --- crates/gitlawb-node/src/graphql/query.rs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/crates/gitlawb-node/src/graphql/query.rs b/crates/gitlawb-node/src/graphql/query.rs index 17423aad..da176b38 100644 --- a/crates/gitlawb-node/src/graphql/query.rs +++ b/crates/gitlawb-node/src/graphql/query.rs @@ -513,7 +513,11 @@ mod tests { .unwrap(); } let schema = schema(db); - let resp = anon(&schema, "{ tasks(limit: 5000) { id } }").await; + // Queried as the delegator, not anonymously: since #268 the task read + // surface is visibility-gated, and an anonymous caller sees none of + // these repo-less tasks at all. The clamp is what this test pins, so it + // needs a caller who can legitimately see all 201 rows. + let resp = authed(&schema, "{ tasks(limit: 5000) { id } }", OWNER).await; assert_eq!(count_tasks(&resp), 200, "limit above 200 must clamp to 200"); } From 7b6f2d6fc2456a7355d8e40a58f1184cd32850b0 Mon Sep 17 00:00:00 2001 From: euxaristia Date: Wed, 12 Aug 2026 19:20:42 -0400 Subject: [PATCH 03/17] fix(node): scope task visibility lookups to the page's repos collect_visible_tasks loaded every repo on the node and every visibility rule in order to gate at most 200 tasks, so an anonymous request paid for the whole node's repo and rule set. Narrow both lookups to the repo ids the fetched page actually names, and skip them when no task names a repo. The deduped repo snapshot stays the source of truth for resolving a repo_id: it collapses mirror and canonical pairs and omits quarantined repos, and an id missing from it has to keep failing closed. Resolving ids straight from the repos table would surface exactly those withheld rows. Add GraphQL denial tests as well. Nothing pinned that the task resolvers delegate to the shared collectors, so a resolver that queried the database directly would not have gone red. Refs #268 --- crates/gitlawb-node/src/api/tasks.rs | 35 +++++++++-- crates/gitlawb-node/src/graphql/query.rs | 80 ++++++++++++++++++++++++ 2 files changed, 110 insertions(+), 5 deletions(-) diff --git a/crates/gitlawb-node/src/api/tasks.rs b/crates/gitlawb-node/src/api/tasks.rs index 96f084e5..9bd3ee93 100644 --- a/crates/gitlawb-node/src/api/tasks.rs +++ b/crates/gitlawb-node/src/api/tasks.rs @@ -8,7 +8,7 @@ //! POST /api/v1/tasks/{id}/complete — complete task //! POST /api/v1/tasks/{id}/fail — fail task -use std::collections::HashMap; +use std::collections::{HashMap, HashSet}; use axum::{ extract::{Extension, Path, Query, State}, @@ -187,11 +187,36 @@ pub(crate) async fn collect_visible_tasks( if tasks.is_empty() { return Ok(tasks); } - let repos = db.list_all_repos_deduped().await?; - let repos_by_id: HashMap = - repos.into_iter().map(|r| (r.id.clone(), r)).collect(); + // Narrow to the repos this page's tasks actually name, so the rule lookup + // below is bounded by the page (≤ MAX_VISIBLE_TASKS) instead of by how many + // repos the node hosts — otherwise an anonymous request pulls every + // visibility rule on the node. The deduped snapshot stays the source of + // truth for *which* repo a `repo_id` resolves to: it collapses + // mirror/canonical pairs and omits quarantined repos, and an id that is + // absent from it must keep failing closed in `task_visible` (a raw + // repos-by-id lookup would resurrect exactly those rows). + let referenced: HashSet<&str> = tasks.iter().filter_map(|t| t.repo_id.as_deref()).collect(); + if referenced.is_empty() { + let empty_repos = HashMap::new(); + let empty_rules = HashMap::new(); + return Ok(tasks + .into_iter() + .filter(|t| task_visible(t, caller, &empty_repos, &empty_rules)) + .collect()); + } + let repos_by_id: HashMap = db + .list_all_repos_deduped() + .await? + .into_iter() + .filter(|r| referenced.contains(r.id.as_str())) + .map(|r| (r.id.clone(), r)) + .collect(); let ids: Vec = repos_by_id.keys().cloned().collect(); - let rules_by_repo = db.list_visibility_rules_for_repos(&ids).await?; + let rules_by_repo = if ids.is_empty() { + HashMap::new() + } else { + db.list_visibility_rules_for_repos(&ids).await? + }; Ok(tasks .into_iter() .filter(|t| task_visible(t, caller, &repos_by_id, &rules_by_repo)) diff --git a/crates/gitlawb-node/src/graphql/query.rs b/crates/gitlawb-node/src/graphql/query.rs index da176b38..7627830d 100644 --- a/crates/gitlawb-node/src/graphql/query.rs +++ b/crates/gitlawb-node/src/graphql/query.rs @@ -521,6 +521,86 @@ mod tests { assert_eq!(count_tasks(&resp), 200, "limit above 200 must clamp to 200"); } + /// Seed one repo-less task carrying a `ucan_token`, so a leak on any read + /// surface is visible in the response body. + async fn seed_task(db: &Db, id: &str, delegator: &str) { + let now = Utc::now().to_rfc3339(); + db.create_task(&crate::db::AgentTask { + id: id.into(), + repo_id: None, + kind: "build".into(), + status: "pending".into(), + delegator_did: delegator.into(), + assignee_did: None, + capability: "repo:write".into(), + ucan_token: Some("SECRET-UCAN-TOKEN".into()), + payload: None, + result: None, + created_at: now.clone(), + updated_at: now, + deadline: None, + }) + .await + .unwrap(); + } + + /// #268: the `tasks` resolver must delegate to the gated collector, not + /// query the DB directly. A repo-less task belonging to someone else is + /// invisible to an anonymous caller. `tasks_negative_limit_clamped` cannot + /// catch a resolver that stops calling `collect_visible_tasks` because it + /// seeds no rows — this seeds one, so the gate is load-bearing here. + #[sqlx::test] + async fn tasks_repo_less_task_hidden_from_anon(pool: PgPool) { + let db = db(pool).await; + seed_task(&db, "t1", OWNER).await; + let schema = schema(db); + let resp = anon(&schema, "{ tasks { id } }").await; + assert_eq!( + count_tasks(&resp), + 0, + "anon must not enumerate another party's repo-less task" + ); + assert!( + !format!("{:?}", resp.data).contains("SECRET-UCAN-TOKEN"), + "no ucan token may reach an anonymous caller" + ); + } + + /// #268 sibling for the single-task resolver: an invisible task reads as + /// `null`, indistinguishable from one that does not exist. + #[sqlx::test] + async fn task_by_id_is_null_for_anon(pool: PgPool) { + let db = db(pool).await; + seed_task(&db, "t1", OWNER).await; + let schema = schema(db); + let resp = anon(&schema, r#"{ task(id: "t1") { id } }"#).await; + assert!(resp.errors.is_empty(), "graphql errors: {:?}", resp.errors); + let async_graphql::Value::Object(obj) = &resp.data else { + panic!("data not an object: {:?}", resp.data); + }; + assert_eq!( + obj.get("task"), + Some(&async_graphql::Value::Null), + "an invisible task must read as null, got {:?}", + obj.get("task") + ); + } + + /// #268: `ucanToken` is absent from the read type's schema entirely, so the + /// delegator cannot request it either. Asking for it is a validation error, + /// which pins the redaction at the schema level rather than per-resolver. + #[sqlx::test] + async fn task_read_schema_has_no_ucan_token_field(pool: PgPool) { + let db = db(pool).await; + seed_task(&db, "t1", OWNER).await; + let schema = schema(db); + let resp = authed(&schema, r#"{ task(id: "t1") { id ucanToken } }"#, OWNER).await; + assert!( + !resp.errors.is_empty(), + "ucanToken must not exist on the task read type" + ); + } + fn count_tasks(resp: &async_graphql::Response) -> usize { assert!(resp.errors.is_empty(), "graphql errors: {:?}", resp.errors); let async_graphql::Value::Object(obj) = &resp.data else { From ccd00640dbadc6792766009069051e1f73c704ed Mon Sep 17 00:00:00 2001 From: euxaristia Date: Wed, 12 Aug 2026 22:27:31 -0400 Subject: [PATCH 04/17] Preserve authorized task reads across shipped clients. Refs #268 --- crates/gitlawb-node/src/api/tasks.rs | 159 ++++++++++++++++------- crates/gitlawb-node/src/db/mod.rs | 134 +++++++++++++------ crates/gitlawb-node/src/graphql/query.rs | 37 ++++++ crates/gl/src/mcp.rs | 57 +++++++- crates/gl/src/task.rs | 102 +++++++++++++-- 5 files changed, 389 insertions(+), 100 deletions(-) diff --git a/crates/gitlawb-node/src/api/tasks.rs b/crates/gitlawb-node/src/api/tasks.rs index 9bd3ee93..6c1182b1 100644 --- a/crates/gitlawb-node/src/api/tasks.rs +++ b/crates/gitlawb-node/src/api/tasks.rs @@ -149,7 +149,15 @@ pub(crate) fn task_visible( return true; } } - let Some(record) = task.repo_id.as_deref().and_then(|id| repos_by_id.get(id)) else { + let Some(repo_id) = task.repo_id.as_deref() else { + return false; + }; + // Slash-form ids are mirror rows. Mirrors are public placeholders and do + // not replicate visibility rules, so they cannot establish read access. + if repo_id.contains('/') { + return false; + } + let Some(record) = repos_by_id.get(repo_id) else { return false; }; let rules = rules_by_repo @@ -165,13 +173,6 @@ pub(crate) fn task_visible( /// pattern in `api/events.rs`. `limit` is clamped here so a caller-supplied /// value never reaches SQL unclamped. /// -/// Unlike the ref-updates feed, this does not page past invisible rows: it -/// fetches one bounded page and filters it, so a request whose newest -/// `MAX_VISIBLE_TASKS` rows are mostly invisible to the caller can return -/// fewer rows than are truly visible further back. Accepted here because, -/// unlike the cross-tenant ref-updates feed, task queries are already scoped -/// by `status`/`assignee_did` up front, which keeps the visible/invisible mix -/// per query far narrower in practice. pub(crate) async fn collect_visible_tasks( db: &crate::db::Db, status: Option<&str>, @@ -183,44 +184,53 @@ pub(crate) async fn collect_visible_tasks( if bounded_limit == 0 { return Ok(Vec::new()); } - let tasks = db.list_tasks(status, assignee_did, bounded_limit).await?; - if tasks.is_empty() { - return Ok(tasks); - } - // Narrow to the repos this page's tasks actually name, so the rule lookup - // below is bounded by the page (≤ MAX_VISIBLE_TASKS) instead of by how many - // repos the node hosts — otherwise an anonymous request pulls every - // visibility rule on the node. The deduped snapshot stays the source of - // truth for *which* repo a `repo_id` resolves to: it collapses - // mirror/canonical pairs and omits quarantined repos, and an id that is - // absent from it must keep failing closed in `task_visible` (a raw - // repos-by-id lookup would resurrect exactly those rows). - let referenced: HashSet<&str> = tasks.iter().filter_map(|t| t.repo_id.as_deref()).collect(); - if referenced.is_empty() { - let empty_repos = HashMap::new(); - let empty_rules = HashMap::new(); - return Ok(tasks + let mut visible = Vec::with_capacity(bounded_limit as usize); + let mut cursor: Option<(String, String)> = None; + loop { + let tasks = db + .list_tasks_keyset( + status, + assignee_did, + MAX_VISIBLE_TASKS, + cursor + .as_ref() + .map(|(created_at, id)| (created_at.as_str(), id.as_str())), + ) + .await?; + if tasks.is_empty() { + break; + } + let next_cursor = tasks + .last() + .map(|task| (task.created_at.clone(), task.id.clone())); + let referenced: Vec = tasks + .iter() + .filter_map(|task| task.repo_id.clone()) + .collect::>() .into_iter() - .filter(|t| task_visible(t, caller, &empty_repos, &empty_rules)) - .collect()); + .collect(); + let repos_by_id: HashMap = db + .list_repos_deduped_by_ids(&referenced) + .await? + .into_iter() + .map(|repo| (repo.id.clone(), repo)) + .collect(); + let repo_ids: Vec = repos_by_id.keys().cloned().collect(); + let rules_by_repo = db.list_visibility_rules_for_repos(&repo_ids).await?; + + visible.extend( + tasks + .iter() + .filter(|task| task_visible(task, caller, &repos_by_id, &rules_by_repo)) + .take((bounded_limit as usize).saturating_sub(visible.len())) + .cloned(), + ); + if visible.len() == bounded_limit as usize || tasks.len() < MAX_VISIBLE_TASKS as usize { + break; + } + cursor = next_cursor; } - let repos_by_id: HashMap = db - .list_all_repos_deduped() - .await? - .into_iter() - .filter(|r| referenced.contains(r.id.as_str())) - .map(|r| (r.id.clone(), r)) - .collect(); - let ids: Vec = repos_by_id.keys().cloned().collect(); - let rules_by_repo = if ids.is_empty() { - HashMap::new() - } else { - db.list_visibility_rules_for_repos(&ids).await? - }; - Ok(tasks - .into_iter() - .filter(|t| task_visible(t, caller, &repos_by_id, &rules_by_repo)) - .collect()) + Ok(visible) } /// Fetch a single task gated the same way `collect_visible_tasks` gates a @@ -238,7 +248,8 @@ pub(crate) async fn get_visible_task( }; let (repos_by_id, rules_by_repo) = match task.repo_id.as_deref() { Some(repo_id) => { - let repos = db.list_all_repos_deduped().await?; + let ids = [repo_id.to_string()]; + let repos = db.list_repos_deduped_by_ids(&ids).await?; match repos.into_iter().find(|r| r.id == repo_id) { Some(record) => { let rules = db.list_visibility_rules(&record.id).await?; @@ -742,6 +753,35 @@ mod visible_tasks_tests { ); } + #[sqlx::test] + async fn mirror_only_repo_task_is_hidden_from_anonymous_reads(pool: PgPool) { + let state = test_state(pool).await; + state + .db + .upsert_mirror_repo(DELEGATOR, "mirror", "/tmp/mirror", None, false) + .await + .unwrap(); + let mirror_id = format!("{DELEGATOR}/mirror"); + state + .db + .create_task(&task("t1", Some(&mirror_id), DELEGATOR)) + .await + .unwrap(); + + let resp = list_router(state.clone()) + .oneshot(anon_get("/api/v1/tasks")) + .await + .unwrap(); + let body = body_json(resp).await; + assert_eq!(body["count"], 0); + + let resp = list_router(state) + .oneshot(anon_get("/api/v1/tasks/t1")) + .await + .unwrap(); + assert_eq!(resp.status(), StatusCode::NOT_FOUND); + } + /// A negative limit must clamp to zero through `collect_visible_tasks`, /// not fall through to the visible set. #[sqlx::test] @@ -765,4 +805,33 @@ mod visible_tasks_tests { let body = body_json(resp).await; assert_eq!(body["count"], 0, "negative limit must clamp to 0"); } + + #[sqlx::test] + async fn older_visible_task_is_not_hidden_by_newer_denied_window(pool: PgPool) { + let state = test_state(pool).await; + state + .db + .create_repo(&repo("public-repo", DELEGATOR, "public", true)) + .await + .unwrap(); + let mut visible = task("visible", Some("public-repo"), DELEGATOR); + visible.created_at = "2026-01-01T00:00:00Z".into(); + visible.updated_at = visible.created_at.clone(); + state.db.create_task(&visible).await.unwrap(); + + for i in 0..MAX_VISIBLE_TASKS { + let mut hidden = task(&format!("hidden-{i:03}"), None, DELEGATOR); + hidden.created_at = "2026-01-02T00:00:00Z".into(); + hidden.updated_at = hidden.created_at.clone(); + state.db.create_task(&hidden).await.unwrap(); + } + + let resp = list_router(state) + .oneshot(anon_get("/api/v1/tasks?limit=1")) + .await + .unwrap(); + let body = body_json(resp).await; + assert_eq!(body["count"], 1); + assert_eq!(body["tasks"][0]["id"], "visible"); + } } diff --git a/crates/gitlawb-node/src/db/mod.rs b/crates/gitlawb-node/src/db/mod.rs index 99c5d8c6..a388985e 100644 --- a/crates/gitlawb-node/src/db/mod.rs +++ b/crates/gitlawb-node/src/db/mod.rs @@ -1135,11 +1135,13 @@ impl Db { /// Shared dedup CTE: collapses the mirror row and the canonical row of one /// logical repo into a single survivor. `$1` is an optional owner filter - /// (NULL = all rows). Grouping collapses on a did:key-aware owner key: strip a - /// `did:key:` prefix (8 chars, so `substr(owner_did, 9)`) only when the - /// remainder is a bare id with no `:`, otherwise keep the full DID. That is the - /// exact normalization in `crate::api::did_matches`, so `did:key:X` and a bare - /// `X` collapse while distinct DID methods (`did:gitlawb:X`) never merge. The + /// (NULL = all rows). `$2` optionally scopes the work to the logical groups + /// containing the supplied repo ids. Grouping collapses on a did:key-aware + /// owner key: strip a `did:key:` prefix (8 chars, so + /// `substr(owner_did, 9)`) only when the remainder is a bare id with no `:`, + /// otherwise keep the full DID. That is the exact normalization in + /// `crate::api::did_matches`, so `did:key:X` and a bare `X` collapse while + /// distinct DID methods (`did:gitlawb:X`) never merge. The /// CASE is repeated verbatim in `count_repos_deduped` and the v7 index and must /// stay byte-identical or Postgres stops using the index. /// The canonical row wins (mirror rows carry a slash-form `id` written only by @@ -1149,7 +1151,12 @@ impl Db { /// `crate::api::repos::dedupe_canonical_repos` must stay in sync. fn dedup_cte() -> String { format!( - "WITH deduped AS ( + "WITH requested_groups AS ( + SELECT DISTINCT {key} AS owner_key, name + FROM repos + WHERE $2::text[] IS NOT NULL AND id = ANY($2) + ), + deduped AS ( SELECT DISTINCT ON ({key}, name) id, name, owner_did, description, is_public, default_branch, created_at, @@ -1168,6 +1175,11 @@ impl Db { -- Quarantined mirrors (admitted but unvalidated by the iCaptcha -- propagation gate) are withheld from every listing surface. WHERE quarantined = FALSE AND ($1::text IS NULL OR ({key}) = $1) + AND ($2::text[] IS NULL OR EXISTS ( + SELECT 1 FROM requested_groups requested + WHERE requested.owner_key = ({key}) + AND requested.name = repos.name + )) ORDER BY {key}, name, -- mirror rows carry a slash-form id (\"{{owner_short}}/{{name}}\"), -- written only by upsert_mirror_repo; canonical ids are UUIDs. @@ -1215,6 +1227,7 @@ impl Db { ); let rows = sqlx::query(&sql) .bind(owner_key) + .bind(None::<&[String]>) .fetch_all(&self.pool) .await?; @@ -1243,6 +1256,32 @@ impl Db { ); let rows = sqlx::query(&sql) .bind(None::<&str>) + .bind(None::<&[String]>) + .fetch_all(&self.pool) + .await?; + + Ok(rows.into_iter().map(row_to_repo).collect()) + } + + /// Resolve only the requested repository ids through the same canonical + /// survivor and quarantine rules as `list_all_repos_deduped`. + pub async fn list_repos_deduped_by_ids(&self, repo_ids: &[String]) -> Result> { + if repo_ids.is_empty() { + return Ok(Vec::new()); + } + let sql = format!( + "{} + SELECT d.id, d.name, d.owner_did, d.description, d.is_public, + d.default_branch, d.created_at, d.updated_at, d.disk_path, + d.forked_from, d.machine_id + FROM deduped d + WHERE d.id = ANY($2) + ORDER BY d.updated_at DESC", + Self::dedup_cte() + ); + let rows = sqlx::query(&sql) + .bind(None::<&str>) + .bind(repo_ids) .fetch_all(&self.pool) .await?; @@ -2850,46 +2889,29 @@ impl Db { Ok(row.map(row_to_task)) } - pub async fn list_tasks( + pub async fn list_tasks_keyset( &self, status: Option<&str>, assignee_did: Option<&str>, limit: i64, + after: Option<(&str, &str)>, ) -> Result> { - let rows = match (status, assignee_did) { - (Some(s), Some(a)) => sqlx::query( - "SELECT id, repo_id, kind, status, delegator_did, assignee_did, capability, ucan_token, payload, result, created_at, updated_at, deadline - FROM agent_tasks WHERE status=$1 AND assignee_did=$2 ORDER BY created_at DESC LIMIT $3", - ) - .bind(s) - .bind(a) - .bind(limit) - .fetch_all(&self.pool) - .await?, - (Some(s), None) => sqlx::query( - "SELECT id, repo_id, kind, status, delegator_did, assignee_did, capability, ucan_token, payload, result, created_at, updated_at, deadline - FROM agent_tasks WHERE status=$1 ORDER BY created_at DESC LIMIT $2", - ) - .bind(s) - .bind(limit) - .fetch_all(&self.pool) - .await?, - (None, Some(a)) => sqlx::query( - "SELECT id, repo_id, kind, status, delegator_did, assignee_did, capability, ucan_token, payload, result, created_at, updated_at, deadline - FROM agent_tasks WHERE assignee_did=$1 ORDER BY created_at DESC LIMIT $2", - ) - .bind(a) - .bind(limit) - .fetch_all(&self.pool) - .await?, - (None, None) => sqlx::query( - "SELECT id, repo_id, kind, status, delegator_did, assignee_did, capability, ucan_token, payload, result, created_at, updated_at, deadline - FROM agent_tasks ORDER BY created_at DESC LIMIT $1", - ) - .bind(limit) - .fetch_all(&self.pool) - .await?, - }; + let rows = sqlx::query( + "SELECT id, repo_id, kind, status, delegator_did, assignee_did, capability, ucan_token, payload, result, created_at, updated_at, deadline + FROM agent_tasks + WHERE ($1::text IS NULL OR status = $1) + AND ($2::text IS NULL OR assignee_did = $2) + AND ($3::text IS NULL OR (created_at, id) < ($3, $4)) + ORDER BY created_at DESC, id DESC + LIMIT $5", + ) + .bind(status) + .bind(assignee_did) + .bind(after.map(|cursor| cursor.0)) + .bind(after.map(|cursor| cursor.1)) + .bind(limit) + .fetch_all(&self.pool) + .await?; Ok(rows.into_iter().map(row_to_task).collect()) } @@ -4307,6 +4329,36 @@ mod dedup_db_tests { ); } + #[sqlx::test] + async fn deduped_id_lookup_returns_only_requested_repo(pool: PgPool) { + let db = db(pool).await; + let requested = rec( + "requested", + "did:key:z6MkRequested", + "requested", + "requested", + "2026-01-01T00:00:00Z", + "2026-01-01T00:00:00Z", + ); + let unrelated = rec( + "unrelated", + "did:key:z6MkUnrelated", + "unrelated", + "unrelated", + "2026-01-02T00:00:00Z", + "2026-01-02T00:00:00Z", + ); + db.create_repo(&requested).await.unwrap(); + db.create_repo(&unrelated).await.unwrap(); + + let out = db + .list_repos_deduped_by_ids(&[requested.id.clone()]) + .await + .unwrap(); + assert_eq!(out.len(), 1); + assert_eq!(out[0].id, requested.id); + } + /// A PRIVATE canonical repo and a PUBLIC mirror row for the same /// (owner, name) collapse to a single survivor whose `is_public` is the /// canonical `false`, not the mirror's `true`. `upsert_mirror_repo` always diff --git a/crates/gitlawb-node/src/graphql/query.rs b/crates/gitlawb-node/src/graphql/query.rs index 7627830d..cbc92bf3 100644 --- a/crates/gitlawb-node/src/graphql/query.rs +++ b/crates/gitlawb-node/src/graphql/query.rs @@ -601,6 +601,43 @@ mod tests { ); } + #[sqlx::test] + async fn tasks_find_older_visible_row_behind_denied_window(pool: PgPool) { + let db = db(pool).await; + db.create_repo(&repo("public-repo", OWNER, "public", true)) + .await + .unwrap(); + let visible = crate::db::AgentTask { + id: "visible".into(), + repo_id: Some("public-repo".into()), + kind: "build".into(), + status: "pending".into(), + delegator_did: OWNER.into(), + assignee_did: None, + capability: "repo:write".into(), + ucan_token: None, + payload: None, + result: None, + created_at: "2026-01-01T00:00:00Z".into(), + updated_at: "2026-01-01T00:00:00Z".into(), + deadline: None, + }; + db.create_task(&visible).await.unwrap(); + for i in 0..200 { + let mut hidden = visible.clone(); + hidden.id = format!("hidden-{i:03}"); + hidden.repo_id = None; + hidden.created_at = "2026-01-02T00:00:00Z".into(); + hidden.updated_at = hidden.created_at.clone(); + db.create_task(&hidden).await.unwrap(); + } + + let schema = schema(db); + let resp = anon(&schema, "{ tasks(limit: 1) { id } }").await; + assert_eq!(count_tasks(&resp), 1); + assert!(format!("{:?}", resp.data).contains("visible")); + } + fn count_tasks(resp: &async_graphql::Response) -> usize { assert!(resp.errors.is_empty(), "graphql errors: {:?}", resp.errors); let async_graphql::Value::Object(obj) = &resp.data else { diff --git a/crates/gl/src/mcp.rs b/crates/gl/src/mcp.rs index ae319c73..959c4c2c 100644 --- a/crates/gl/src/mcp.rs +++ b/crates/gl/src/mcp.rs @@ -1068,7 +1068,12 @@ async fn call_tool( if let Some(a) = args.get("assignee_did").and_then(|v| v.as_str()) { path.push_str(&format!("&assignee_did={}", urlencoding::encode(a))); } - let resp: Value = client.get(&path).await?.json().await?; + let resp: Value = client + .get_maybe_signed(&path) + .await? + .error_for_status()? + .json() + .await?; Ok(serde_json::to_string_pretty(&resp)?) } @@ -1606,6 +1611,56 @@ mod tests { assert_eq!(parsed["tasks"][0]["id"], "t1"); } + #[tokio::test] + async fn test_task_list_via_mcp_uses_loaded_identity() { + let mut server = mockito::Server::new_async().await; + let dir = tempfile::TempDir::new().unwrap(); + let kp = gitlawb_core::identity::Keypair::generate(); + std::fs::write( + dir.path().join("identity.pem"), + kp.to_pem().unwrap().as_bytes(), + ) + .unwrap(); + let _m = server + .mock( + "GET", + mockito::Matcher::Regex(r"/api/v1/tasks\?".to_string()), + ) + .match_header("signature", mockito::Matcher::Any) + .match_header("signature-input", mockito::Matcher::Any) + .with_status(200) + .with_header("content-type", "application/json") + .with_body(r#"{"tasks":[{"id":"t1"}]}"#) + .create_async() + .await; + + let result = call_tool("task_list", json!({}), &server.url(), Some(dir.path())) + .await + .unwrap(); + let parsed: Value = serde_json::from_str(&result).unwrap(); + assert_eq!(parsed["tasks"][0]["id"], "t1"); + } + + #[tokio::test] + async fn test_task_list_via_mcp_returns_http_errors() { + let mut server = mockito::Server::new_async().await; + let _m = server + .mock( + "GET", + mockito::Matcher::Regex(r"/api/v1/tasks\?".to_string()), + ) + .with_status(500) + .with_header("content-type", "application/json") + .with_body(r#"{"error":"failed"}"#) + .create_async() + .await; + + let err = call_tool("task_list", json!({}), &server.url(), None) + .await + .unwrap_err(); + assert!(err.to_string().contains("500")); + } + #[tokio::test] async fn test_task_create_via_mcp() { let mut server = mockito::Server::new_async().await; diff --git a/crates/gl/src/task.rs b/crates/gl/src/task.rs index c26cb35f..9bfd73f8 100644 --- a/crates/gl/src/task.rs +++ b/crates/gl/src/task.rs @@ -53,12 +53,16 @@ pub enum TaskCmd { limit: i64, #[arg(long, default_value = "https://node.gitlawb.com", env = "GITLAWB_NODE")] node: String, + #[arg(long)] + dir: Option, }, /// View a specific task View { id: String, #[arg(long, default_value = "https://node.gitlawb.com", env = "GITLAWB_NODE")] node: String, + #[arg(long)] + dir: Option, }, /// Claim a pending task Claim { @@ -121,8 +125,9 @@ pub async fn run(args: TaskArgs) -> Result<()> { assignee_did, limit, node, - } => cmd_list(status, assignee_did, limit, node).await, - TaskCmd::View { id, node } => cmd_view(id, node).await, + dir, + } => cmd_list(status, assignee_did, limit, node, dir).await, + TaskCmd::View { id, node, dir } => cmd_view(id, node, dir).await, TaskCmd::Claim { id, node, dir } => cmd_claim(id, node, dir).await, TaskCmd::Complete { id, @@ -182,8 +187,9 @@ async fn cmd_list( assignee_did: Option, limit: i64, node: String, + dir: Option, ) -> Result<()> { - let client = NodeClient::new(&node, None); + let client = NodeClient::new(&node, load_keypair_from_dir(dir.as_deref()).ok()); let mut path = format!("/api/v1/tasks?limit={}", limit); if let Some(s) = &status { path.push_str(&format!("&status={}", urlencoding::encode(s))); @@ -192,9 +198,11 @@ async fn cmd_list( path.push_str(&format!("&assignee_did={}", urlencoding::encode(a))); } let resp: Value = client - .get(&path) + .get_maybe_signed(&path) .await .context("failed to list tasks")? + .error_for_status() + .context("failed to list tasks")? .json() .await .context("invalid JSON response")?; @@ -202,12 +210,14 @@ async fn cmd_list( Ok(()) } -async fn cmd_view(id: String, node: String) -> Result<()> { - let client = NodeClient::new(&node, None); +async fn cmd_view(id: String, node: String, dir: Option) -> Result<()> { + let client = NodeClient::new(&node, load_keypair_from_dir(dir.as_deref()).ok()); let resp: Value = client - .get(&format!("/api/v1/tasks/{}", id)) + .get_maybe_signed(&format!("/api/v1/tasks/{}", id)) .await .context("failed to get task")? + .error_for_status() + .context("failed to get task")? .json() .await .context("invalid JSON response")?; @@ -379,6 +389,7 @@ mod tests { #[tokio::test] async fn test_list_tasks_empty() { let mut server = mockito::Server::new_async().await; + let dir = tempfile::TempDir::new().unwrap(); let _m = server .mock( @@ -391,18 +402,29 @@ mod tests { .create_async() .await; - cmd_list(None, None, 50, server.url()).await.unwrap(); + cmd_list(None, None, 50, server.url(), Some(dir.path().to_path_buf())) + .await + .unwrap(); } #[tokio::test] - async fn test_list_tasks_with_filters() { + async fn test_delegator_list_tasks_is_signed() { let mut server = mockito::Server::new_async().await; + let dir = tempfile::TempDir::new().unwrap(); + let kp = gitlawb_core::identity::Keypair::generate(); + std::fs::write( + dir.path().join("identity.pem"), + kp.to_pem().unwrap().as_bytes(), + ) + .unwrap(); let _m = server .mock( "GET", mockito::Matcher::Regex(r"status=pending".to_string()), ) + .match_header("signature", mockito::Matcher::Any) + .match_header("signature-input", mockito::Matcher::Any) .with_status(200) .with_header("content-type", "application/json") .with_body(r#"{"tasks":[{"id":"t1","kind":"test","status":"pending"}]}"#) @@ -414,6 +436,7 @@ mod tests { Some("did:key:z6Mk_test".to_string()), 10, server.url(), + Some(dir.path().to_path_buf()), ) .await .unwrap(); @@ -422,34 +445,87 @@ mod tests { // ── view ───────────────────────────────────────────────────────── #[tokio::test] - async fn test_view_task_success() { + async fn test_assignee_view_task_is_signed() { let mut server = mockito::Server::new_async().await; + let dir = tempfile::TempDir::new().unwrap(); + let kp = gitlawb_core::identity::Keypair::generate(); + std::fs::write( + dir.path().join("identity.pem"), + kp.to_pem().unwrap().as_bytes(), + ) + .unwrap(); let _m = server .mock("GET", "/api/v1/tasks/task-42") + .match_header("signature", mockito::Matcher::Any) + .match_header("signature-input", mockito::Matcher::Any) .with_status(200) .with_header("content-type", "application/json") .with_body(r#"{"id":"task-42","kind":"deploy","status":"completed","result":"ok"}"#) .create_async() .await; - cmd_view("task-42".to_string(), server.url()).await.unwrap(); + cmd_view( + "task-42".to_string(), + server.url(), + Some(dir.path().to_path_buf()), + ) + .await + .unwrap(); + } + + #[tokio::test] + async fn test_private_repo_task_view_is_signed() { + let mut server = mockito::Server::new_async().await; + let dir = tempfile::TempDir::new().unwrap(); + let kp = gitlawb_core::identity::Keypair::generate(); + std::fs::write( + dir.path().join("identity.pem"), + kp.to_pem().unwrap().as_bytes(), + ) + .unwrap(); + + let _m = server + .mock("GET", "/api/v1/tasks/private-task") + .match_header("signature", mockito::Matcher::Any) + .match_header("signature-input", mockito::Matcher::Any) + .with_status(200) + .with_header("content-type", "application/json") + .with_body(r#"{"id":"private-task","repo_id":"private-repo"}"#) + .create_async() + .await; + + cmd_view( + "private-task".to_string(), + server.url(), + Some(dir.path().to_path_buf()), + ) + .await + .unwrap(); } #[tokio::test] async fn test_view_task_not_found() { let mut server = mockito::Server::new_async().await; + let dir = tempfile::TempDir::new().unwrap(); let _m = server .mock("GET", "/api/v1/tasks/nope") + .match_header("signature", mockito::Matcher::Missing) .with_status(404) .with_header("content-type", "application/json") .with_body(r#"{"message":"not found"}"#) .create_async() .await; - // cmd_view doesn't check status — it prints the JSON - cmd_view("nope".to_string(), server.url()).await.unwrap(); + let err = cmd_view( + "nope".to_string(), + server.url(), + Some(dir.path().to_path_buf()), + ) + .await + .unwrap_err(); + assert!(err.to_string().contains("failed to get task")); } // ── claim ──────────────────────────────────────────────────────── From c4a36e56edddf2c78b37e9f9fffd6878d747eb63 Mon Sep 17 00:00:00 2001 From: euxaristia Date: Wed, 12 Aug 2026 22:46:04 -0400 Subject: [PATCH 05/17] Bound denied task history scans. Refs #268 --- crates/gitlawb-node/src/api/tasks.rs | 86 ++++++++++++++++++++++++++-- 1 file changed, 82 insertions(+), 4 deletions(-) diff --git a/crates/gitlawb-node/src/api/tasks.rs b/crates/gitlawb-node/src/api/tasks.rs index 6c1182b1..e767fd7a 100644 --- a/crates/gitlawb-node/src/api/tasks.rs +++ b/crates/gitlawb-node/src/api/tasks.rs @@ -119,6 +119,10 @@ fn task_to_read_json(t: &AgentTask) -> Value { /// request size controls how much the visibility filter has to scan. const MAX_VISIBLE_TASKS: i64 = 200; +/// Maximum task candidates one list request may inspect while searching for +/// visible rows. This keeps a denied request from walking the full task table. +const MAX_TASK_SCAN_CANDIDATES: i64 = 1_000; + /// Whether `task` should be visible to `caller` (`None` = anonymous). /// /// The delegator and assignee can always read a task they are already party @@ -186,12 +190,14 @@ pub(crate) async fn collect_visible_tasks( } let mut visible = Vec::with_capacity(bounded_limit as usize); let mut cursor: Option<(String, String)> = None; - loop { + let mut scanned = 0; + while scanned < MAX_TASK_SCAN_CANDIDATES { + let batch_limit = MAX_VISIBLE_TASKS.min(MAX_TASK_SCAN_CANDIDATES - scanned); let tasks = db .list_tasks_keyset( status, assignee_did, - MAX_VISIBLE_TASKS, + batch_limit, cursor .as_ref() .map(|(created_at, id)| (created_at.as_str(), id.as_str())), @@ -200,6 +206,7 @@ pub(crate) async fn collect_visible_tasks( if tasks.is_empty() { break; } + scanned += tasks.len() as i64; let next_cursor = tasks .last() .map(|task| (task.created_at.clone(), task.id.clone())); @@ -225,7 +232,7 @@ pub(crate) async fn collect_visible_tasks( .take((bounded_limit as usize).saturating_sub(visible.len())) .cloned(), ); - if visible.len() == bounded_limit as usize || tasks.len() < MAX_VISIBLE_TASKS as usize { + if visible.len() == bounded_limit as usize || tasks.len() < batch_limit as usize { break; } cursor = next_cursor; @@ -588,8 +595,12 @@ mod visible_tasks_tests { "anon must not see another party's repo-less task" ); assert_eq!(body["count"], 0); + let serialized = body.to_string(); + assert!(!serialized.contains("t1")); + assert!(!serialized.contains("payload-data")); + assert!(!serialized.contains(SECRET_UCAN)); - let resp = list_router(state) + let resp = list_router(state.clone()) .oneshot(anon_get("/api/v1/tasks/t1")) .await .unwrap(); @@ -598,6 +609,44 @@ mod visible_tasks_tests { StatusCode::NOT_FOUND, "anon get_task on an invisible task must 404, not leak it" ); + let body = body_json(resp).await; + let serialized = body.to_string(); + assert!(!serialized.contains("t1")); + assert!(!serialized.contains("payload-data")); + assert!(!serialized.contains(SECRET_UCAN)); + + let resp = list_router(state.clone()) + .oneshot(signed_request_as( + STRANGER, + Method::GET, + "/api/v1/tasks", + Body::empty(), + )) + .await + .unwrap(); + let body = body_json(resp).await; + assert_eq!(body["tasks"].as_array().unwrap().len(), 0); + assert_eq!(body["count"], 0); + let serialized = body.to_string(); + assert!(!serialized.contains("t1")); + assert!(!serialized.contains("payload-data")); + assert!(!serialized.contains(SECRET_UCAN)); + + let resp = list_router(state) + .oneshot(signed_request_as( + STRANGER, + Method::GET, + "/api/v1/tasks/t1", + Body::empty(), + )) + .await + .unwrap(); + assert_eq!(resp.status(), StatusCode::NOT_FOUND); + let body = body_json(resp).await; + let serialized = body.to_string(); + assert!(!serialized.contains("t1")); + assert!(!serialized.contains("payload-data")); + assert!(!serialized.contains(SECRET_UCAN)); } /// The delegator can always read their own repo-less task — the party who @@ -834,4 +883,33 @@ mod visible_tasks_tests { assert_eq!(body["count"], 1); assert_eq!(body["tasks"][0]["id"], "visible"); } + + #[sqlx::test] + async fn denied_history_scan_stops_at_candidate_ceiling(pool: PgPool) { + let state = test_state(pool).await; + state + .db + .create_repo(&repo("public-repo", DELEGATOR, "public", true)) + .await + .unwrap(); + let mut visible = task("past-ceiling", Some("public-repo"), DELEGATOR); + visible.created_at = "2026-01-01T00:00:00Z".into(); + visible.updated_at = visible.created_at.clone(); + state.db.create_task(&visible).await.unwrap(); + + for i in 0..MAX_TASK_SCAN_CANDIDATES { + let mut hidden = task(&format!("hidden-{i:04}"), None, DELEGATOR); + hidden.created_at = "2026-01-02T00:00:00Z".into(); + hidden.updated_at = hidden.created_at.clone(); + state.db.create_task(&hidden).await.unwrap(); + } + + let resp = list_router(state) + .oneshot(anon_get("/api/v1/tasks?limit=1")) + .await + .unwrap(); + let body = body_json(resp).await; + assert_eq!(body["count"], 0); + assert!(!body.to_string().contains("past-ceiling")); + } } From 4ab649dbcdb9a9dfe7e28e51cb55661ba7d464dc Mon Sep 17 00:00:00 2001 From: euxaristia Date: Fri, 14 Aug 2026 00:40:49 -0400 Subject: [PATCH 06/17] Signal incomplete task scans with recoverable cursors and map read errors to AppError. Refs #268 --- crates/gitlawb-node/src/api/tasks.rs | 164 ++++++++++++++++++----- crates/gitlawb-node/src/db/mod.rs | 2 +- crates/gitlawb-node/src/graphql/query.rs | 56 +++++++- 3 files changed, 183 insertions(+), 39 deletions(-) diff --git a/crates/gitlawb-node/src/api/tasks.rs b/crates/gitlawb-node/src/api/tasks.rs index e767fd7a..29f7f0f9 100644 --- a/crates/gitlawb-node/src/api/tasks.rs +++ b/crates/gitlawb-node/src/api/tasks.rs @@ -22,6 +22,7 @@ use uuid::Uuid; use crate::auth::AuthenticatedDid; use crate::db::{AgentTask, RepoRecord, VisibilityRule}; +use crate::error::AppError; use crate::state::{AppState, TaskEventBroadcast}; /// 403 in this module's error shape (`(StatusCode, Json)`, not `AppError`). @@ -52,6 +53,10 @@ pub struct ListTasksQuery { pub assignee_did: Option, #[serde(default = "default_limit")] pub limit: i64, + pub after_created_at: Option, + pub after_id: Option, + pub cursor_created_at: Option, + pub cursor_id: Option, } fn default_limit() -> i64 { @@ -171,6 +176,14 @@ pub(crate) fn task_visible( crate::visibility::listable_at_root(rules, record.is_public, &record.owner_did, caller) } +/// Collect up to `limit` tasks visible to `caller`, applying the same gate the +#[derive(Debug, Clone)] +pub(crate) struct VisibleTasks { + pub tasks: Vec, + pub incomplete: bool, + pub next_cursor: Option<(String, String)>, +} + /// Collect up to `limit` tasks visible to `caller`, applying the same gate the /// GraphQL `tasks` query uses (`collect_visible_tasks` is called from both) so /// the two surfaces cannot drift, matching the `collect_visible_ref_updates` @@ -182,15 +195,23 @@ pub(crate) async fn collect_visible_tasks( status: Option<&str>, assignee_did: Option<&str>, limit: i64, + after: Option<(&str, &str)>, caller: Option<&str>, -) -> crate::error::Result> { +) -> crate::error::Result { let bounded_limit = limit.clamp(0, MAX_VISIBLE_TASKS); if bounded_limit == 0 { - return Ok(Vec::new()); + return Ok(VisibleTasks { + tasks: Vec::new(), + incomplete: false, + next_cursor: None, + }); } let mut visible = Vec::with_capacity(bounded_limit as usize); - let mut cursor: Option<(String, String)> = None; + let mut cursor: Option<(String, String)> = + after.map(|(ts, id)| (ts.to_string(), id.to_string())); let mut scanned = 0; + let mut last_examined: Option<(String, String)> = None; + while scanned < MAX_TASK_SCAN_CANDIDATES { let batch_limit = MAX_VISIBLE_TASKS.min(MAX_TASK_SCAN_CANDIDATES - scanned); let tasks = db @@ -210,6 +231,8 @@ pub(crate) async fn collect_visible_tasks( let next_cursor = tasks .last() .map(|task| (task.created_at.clone(), task.id.clone())); + last_examined = next_cursor.clone(); + let referenced: Vec = tasks .iter() .filter_map(|task| task.repo_id.clone()) @@ -225,19 +248,29 @@ pub(crate) async fn collect_visible_tasks( let repo_ids: Vec = repos_by_id.keys().cloned().collect(); let rules_by_repo = db.list_visibility_rules_for_repos(&repo_ids).await?; - visible.extend( - tasks - .iter() - .filter(|task| task_visible(task, caller, &repos_by_id, &rules_by_repo)) - .take((bounded_limit as usize).saturating_sub(visible.len())) - .cloned(), - ); + for task in &tasks { + if task_visible(task, caller, &repos_by_id, &rules_by_repo) { + visible.push(task.clone()); + if visible.len() == bounded_limit as usize { + break; + } + } + } + if visible.len() == bounded_limit as usize || tasks.len() < batch_limit as usize { break; } cursor = next_cursor; } - Ok(visible) + + let incomplete = visible.len() < bounded_limit as usize && scanned >= MAX_TASK_SCAN_CANDIDATES; + let next_cursor = if incomplete { last_examined } else { None }; + + Ok(VisibleTasks { + tasks: visible, + incomplete, + next_cursor, + }) } /// Fetch a single task gated the same way `collect_visible_tasks` gates a @@ -280,7 +313,7 @@ pub async fn create_task( State(state): State, Extension(auth): Extension, Json(body): Json, -) -> Result<(StatusCode, Json), (StatusCode, Json)> { +) -> std::result::Result<(StatusCode, Json), (StatusCode, Json)> { // Bind the delegator to the authenticated signer (N13). if !crate::api::did_matches(&auth.0, &body.delegator_did) { return Err(forbidden("delegator_did must be the authenticated signer")); @@ -319,24 +352,35 @@ pub async fn list_tasks( State(state): State, Query(q): Query, auth: Option>, -) -> Result, (StatusCode, Json)> { +) -> crate::error::Result> { let caller = auth.as_ref().map(|e| e.0 .0.as_str()); - let tasks = collect_visible_tasks( + let after = q + .after_created_at + .as_deref() + .or(q.cursor_created_at.as_deref()) + .zip(q.after_id.as_deref().or(q.cursor_id.as_deref())); + let result = collect_visible_tasks( &state.db, q.status.as_deref(), q.assignee_did.as_deref(), q.limit, + after, caller, ) - .await - .map_err(|e| { - ( - StatusCode::INTERNAL_SERVER_ERROR, - Json(json!({ "error": e.to_string() })), - ) - })?; - let items: Vec = tasks.iter().map(task_to_read_json).collect(); - Ok(Json(json!({ "tasks": items, "count": items.len() }))) + .await?; + let items: Vec = result.tasks.iter().map(task_to_read_json).collect(); + let next_cursor = result.next_cursor.map(|(created_at, id)| { + json!({ + "created_at": created_at, + "id": id, + }) + }); + Ok(Json(json!({ + "tasks": items, + "count": items.len(), + "incomplete": result.incomplete, + "next_cursor": next_cursor, + }))) } /// GET /api/v1/tasks/{id} @@ -347,18 +391,11 @@ pub async fn get_task( State(state): State, Path(id): Path, auth: Option>, -) -> Result, (StatusCode, Json)> { +) -> crate::error::Result> { let caller = auth.as_ref().map(|e| e.0 .0.as_str()); - match get_visible_task(&state.db, &id, caller).await { - Ok(Some(t)) => Ok(Json(task_to_read_json(&t))), - Ok(None) => Err(( - StatusCode::NOT_FOUND, - Json(json!({ "error": "task not found" })), - )), - Err(e) => Err(( - StatusCode::INTERNAL_SERVER_ERROR, - Json(json!({ "error": e.to_string() })), - )), + match get_visible_task(&state.db, &id, caller).await? { + Some(t) => Ok(Json(task_to_read_json(&t))), + None => Err(AppError::NotFound("task not found".into())), } } @@ -885,7 +922,7 @@ mod visible_tasks_tests { } #[sqlx::test] - async fn denied_history_scan_stops_at_candidate_ceiling(pool: PgPool) { + async fn denied_history_scan_stops_at_candidate_ceiling_and_signals_incomplete(pool: PgPool) { let state = test_state(pool).await; state .db @@ -904,12 +941,67 @@ mod visible_tasks_tests { state.db.create_task(&hidden).await.unwrap(); } - let resp = list_router(state) + let resp = list_router(state.clone()) .oneshot(anon_get("/api/v1/tasks?limit=1")) .await .unwrap(); let body = body_json(resp).await; assert_eq!(body["count"], 0); + assert_eq!(body["incomplete"], true); assert!(!body.to_string().contains("past-ceiling")); + + let next_cursor = &body["next_cursor"]; + let cursor_ts = next_cursor["created_at"].as_str().expect("cursor ts"); + let cursor_id = next_cursor["id"].as_str().expect("cursor id"); + + let resp = list_router(state) + .oneshot(anon_get(&format!( + "/api/v1/tasks?limit=1&after_created_at={}&after_id={}", + cursor_ts, cursor_id + ))) + .await + .unwrap(); + let body = body_json(resp).await; + assert_eq!(body["count"], 1); + assert_eq!(body["incomplete"], false); + assert_eq!(body["tasks"][0]["id"], "past-ceiling"); + } + + #[sqlx::test] + async fn list_tasks_closed_pool_returns_503_db_unavailable(pool: PgPool) { + let state = test_state(pool.clone()).await; + pool.close().await; + + let resp = list_router(state) + .oneshot(anon_get("/api/v1/tasks")) + .await + .unwrap(); + + assert_eq!( + resp.status(), + StatusCode::SERVICE_UNAVAILABLE, + "closed-pool outage must be retryable 503, not 500" + ); + let body = body_json(resp).await; + assert_eq!(body["error"], "db_unavailable"); + } + + #[sqlx::test] + async fn get_task_closed_pool_returns_503_db_unavailable(pool: PgPool) { + let state = test_state(pool.clone()).await; + pool.close().await; + + let resp = list_router(state) + .oneshot(anon_get("/api/v1/tasks/t1")) + .await + .unwrap(); + + assert_eq!( + resp.status(), + StatusCode::SERVICE_UNAVAILABLE, + "closed-pool outage must be retryable 503, not 500" + ); + let body = body_json(resp).await; + assert_eq!(body["error"], "db_unavailable"); } } diff --git a/crates/gitlawb-node/src/db/mod.rs b/crates/gitlawb-node/src/db/mod.rs index a388985e..1a79ecd1 100644 --- a/crates/gitlawb-node/src/db/mod.rs +++ b/crates/gitlawb-node/src/db/mod.rs @@ -4352,7 +4352,7 @@ mod dedup_db_tests { db.create_repo(&unrelated).await.unwrap(); let out = db - .list_repos_deduped_by_ids(&[requested.id.clone()]) + .list_repos_deduped_by_ids(std::slice::from_ref(&requested.id)) .await .unwrap(); assert_eq!(out.len(), 1); diff --git a/crates/gitlawb-node/src/graphql/query.rs b/crates/gitlawb-node/src/graphql/query.rs index cbc92bf3..28004681 100644 --- a/crates/gitlawb-node/src/graphql/query.rs +++ b/crates/gitlawb-node/src/graphql/query.rs @@ -115,6 +115,8 @@ impl QueryRoot { desc = "Max 200; larger requests are clamped to 200 (no error). Negative values clamp to 0." )] limit: i64, + after_created_at: Option, + after_id: Option, ) -> Result> { let db = ctx.data_unchecked::>(); // #268: gate rows via the same collector the REST list route uses (like @@ -125,16 +127,22 @@ impl QueryRoot { .data::() .ok() .map(|d| d.0.as_str()); - let tasks = crate::api::tasks::collect_visible_tasks( + let after = after_created_at.as_deref().zip(after_id.as_deref()); + let result = crate::api::tasks::collect_visible_tasks( db, status.as_deref(), assignee_did.as_deref(), limit, + after, caller, ) .await .map_err(crate::graphql::graphql_app_err)?; - Ok(tasks.into_iter().map(AgentTaskReadType::from).collect()) + Ok(result + .tasks + .into_iter() + .map(AgentTaskReadType::from) + .collect()) } async fn task(&self, ctx: &Context<'_>, id: String) -> Result> { @@ -638,6 +646,50 @@ mod tests { assert!(format!("{:?}", resp.data).contains("visible")); } + #[sqlx::test] + async fn tasks_continuation_past_candidate_ceiling(pool: PgPool) { + let db = db(pool).await; + db.create_repo(&repo("public-repo", OWNER, "public", true)) + .await + .unwrap(); + let visible = crate::db::AgentTask { + id: "past-ceiling".into(), + repo_id: Some("public-repo".into()), + kind: "build".into(), + status: "pending".into(), + delegator_did: OWNER.into(), + assignee_did: None, + capability: "repo:write".into(), + ucan_token: None, + payload: None, + result: None, + created_at: "2026-01-01T00:00:00Z".into(), + updated_at: "2026-01-01T00:00:00Z".into(), + deadline: None, + }; + db.create_task(&visible).await.unwrap(); + for i in 0..1000 { + let mut hidden = visible.clone(); + hidden.id = format!("hidden-{i:04}"); + hidden.repo_id = None; + hidden.created_at = "2026-01-02T00:00:00Z".into(); + hidden.updated_at = hidden.created_at.clone(); + db.create_task(&hidden).await.unwrap(); + } + + let schema = schema(db); + let resp = anon(&schema, "{ tasks(limit: 1) { id } }").await; + assert_eq!(count_tasks(&resp), 0); + + let resp = anon( + &schema, + r#"{ tasks(limit: 1, afterCreatedAt: "2026-01-02T00:00:00Z", afterId: "hidden-0999") { id } }"#, + ) + .await; + assert_eq!(count_tasks(&resp), 1); + assert!(format!("{:?}", resp.data).contains("past-ceiling")); + } + fn count_tasks(resp: &async_graphql::Response) -> usize { assert!(resp.errors.is_empty(), "graphql errors: {:?}", resp.errors); let async_graphql::Value::Object(obj) = &resp.data else { From bce8de806e53a80a3a68b0b624d28d9adfaf53d8 Mon Sep 17 00:00:00 2001 From: euxaristia Date: Fri, 14 Aug 2026 22:05:24 -0400 Subject: [PATCH 07/17] Stop disclosing denied task rows in the scan-wall cursor and add GraphQL pagination state. Refs #268 --- crates/gitlawb-node/src/api/tasks.rs | 86 ++++++++++++++++------- crates/gitlawb-node/src/graphql/query.rs | 87 +++++++++++++++++++----- crates/gitlawb-node/src/graphql/types.rs | 12 ++++ 3 files changed, 144 insertions(+), 41 deletions(-) diff --git a/crates/gitlawb-node/src/api/tasks.rs b/crates/gitlawb-node/src/api/tasks.rs index 29f7f0f9..338f6196 100644 --- a/crates/gitlawb-node/src/api/tasks.rs +++ b/crates/gitlawb-node/src/api/tasks.rs @@ -180,8 +180,15 @@ pub(crate) fn task_visible( #[derive(Debug, Clone)] pub(crate) struct VisibleTasks { pub tasks: Vec, + /// True when the candidate scan hit `MAX_TASK_SCAN_CANDIDATES` before + /// filling `limit`, so the caller cannot tell an empty/short page from an + /// exhaustive one. Deliberately carries no cursor: the scan position at + /// that point is the last *examined* candidate, which may be a task the + /// caller was denied, and handing that back would let a denied read leak + /// the id/created_at of a row `GET /tasks/{id}` otherwise 404s (the same + /// id `claim_task` accepts). A caller can still page with + /// `after_created_at`/`after_id` set to the last row *they* received. pub incomplete: bool, - pub next_cursor: Option<(String, String)>, } /// Collect up to `limit` tasks visible to `caller`, applying the same gate the @@ -203,14 +210,12 @@ pub(crate) async fn collect_visible_tasks( return Ok(VisibleTasks { tasks: Vec::new(), incomplete: false, - next_cursor: None, }); } let mut visible = Vec::with_capacity(bounded_limit as usize); let mut cursor: Option<(String, String)> = after.map(|(ts, id)| (ts.to_string(), id.to_string())); let mut scanned = 0; - let mut last_examined: Option<(String, String)> = None; while scanned < MAX_TASK_SCAN_CANDIDATES { let batch_limit = MAX_VISIBLE_TASKS.min(MAX_TASK_SCAN_CANDIDATES - scanned); @@ -231,7 +236,6 @@ pub(crate) async fn collect_visible_tasks( let next_cursor = tasks .last() .map(|task| (task.created_at.clone(), task.id.clone())); - last_examined = next_cursor.clone(); let referenced: Vec = tasks .iter() @@ -264,12 +268,10 @@ pub(crate) async fn collect_visible_tasks( } let incomplete = visible.len() < bounded_limit as usize && scanned >= MAX_TASK_SCAN_CANDIDATES; - let next_cursor = if incomplete { last_examined } else { None }; Ok(VisibleTasks { tasks: visible, incomplete, - next_cursor, }) } @@ -354,11 +356,12 @@ pub async fn list_tasks( auth: Option>, ) -> crate::error::Result> { let caller = auth.as_ref().map(|e| e.0 .0.as_str()); - let after = q - .after_created_at - .as_deref() - .or(q.cursor_created_at.as_deref()) - .zip(q.after_id.as_deref().or(q.cursor_id.as_deref())); + let after = parse_after_cursor( + q.after_created_at + .as_deref() + .or(q.cursor_created_at.as_deref()), + q.after_id.as_deref().or(q.cursor_id.as_deref()), + )?; let result = collect_visible_tasks( &state.db, q.status.as_deref(), @@ -369,20 +372,29 @@ pub async fn list_tasks( ) .await?; let items: Vec = result.tasks.iter().map(task_to_read_json).collect(); - let next_cursor = result.next_cursor.map(|(created_at, id)| { - json!({ - "created_at": created_at, - "id": id, - }) - }); Ok(Json(json!({ "tasks": items, "count": items.len(), "incomplete": result.incomplete, - "next_cursor": next_cursor, }))) } +/// A cursor is two independently optional query fields (`created_at`, `id`); +/// treating a half-supplied pair as absent would silently restart the caller +/// at page one instead of surfacing the lost half. Require both together. +pub(crate) fn parse_after_cursor<'a>( + created_at: Option<&'a str>, + id: Option<&'a str>, +) -> crate::error::Result> { + match (created_at, id) { + (Some(ts), Some(id)) => Ok(Some((ts, id))), + (None, None) => Ok(None), + _ => Err(AppError::BadRequest( + "after_created_at and after_id must be supplied together".into(), + )), + } +} + /// GET /api/v1/tasks/{id} /// /// Gated the same way as `list_tasks` (#268): a task the caller may not see @@ -949,15 +961,23 @@ mod visible_tasks_tests { assert_eq!(body["count"], 0); assert_eq!(body["incomplete"], true); assert!(!body.to_string().contains("past-ceiling")); + assert!( + body.get("next_cursor").is_none(), + "response must not disclose the last examined (denied) row's id/created_at: {body}" + ); + assert!( + !body.to_string().contains("hidden-"), + "response must not leak any denied row's id: {body}" + ); - let next_cursor = &body["next_cursor"]; - let cursor_ts = next_cursor["created_at"].as_str().expect("cursor ts"); - let cursor_id = next_cursor["id"].as_str().expect("cursor id"); - + // No cursor is disclosed above, so resuming past the scan wall in one + // more request requires `after_created_at`/`after_id` the caller + // already legitimately knows. This stands in for that out-of-band + // knowledge with the last seeded row's known position. let resp = list_router(state) .oneshot(anon_get(&format!( - "/api/v1/tasks?limit=1&after_created_at={}&after_id={}", - cursor_ts, cursor_id + "/api/v1/tasks?limit=1&after_created_at=2026-01-02T00:00:00Z&after_id=hidden-{:04}", + MAX_TASK_SCAN_CANDIDATES - 1 ))) .await .unwrap(); @@ -967,6 +987,24 @@ mod visible_tasks_tests { assert_eq!(body["tasks"][0]["id"], "past-ceiling"); } + #[sqlx::test] + async fn list_tasks_rejects_partial_cursor_pair(pool: PgPool) { + let state = test_state(pool).await; + let resp = list_router(state.clone()) + .oneshot(anon_get( + "/api/v1/tasks?after_created_at=2026-01-01T00:00:00Z", + )) + .await + .unwrap(); + assert_eq!(resp.status(), StatusCode::BAD_REQUEST); + + let resp = list_router(state) + .oneshot(anon_get("/api/v1/tasks?after_id=some-id")) + .await + .unwrap(); + assert_eq!(resp.status(), StatusCode::BAD_REQUEST); + } + #[sqlx::test] async fn list_tasks_closed_pool_returns_503_db_unavailable(pool: PgPool) { let state = test_state(pool.clone()).await; diff --git a/crates/gitlawb-node/src/graphql/query.rs b/crates/gitlawb-node/src/graphql/query.rs index 28004681..77038e26 100644 --- a/crates/gitlawb-node/src/graphql/query.rs +++ b/crates/gitlawb-node/src/graphql/query.rs @@ -3,7 +3,7 @@ use std::sync::Arc; use crate::db::Db; -use super::types::{AgentTaskReadType, RefUpdateType, RepoType}; +use super::types::{AgentTaskReadType, RefUpdateType, RepoType, TaskPageType}; pub struct QueryRoot; @@ -117,7 +117,7 @@ impl QueryRoot { limit: i64, after_created_at: Option, after_id: Option, - ) -> Result> { + ) -> Result { let db = ctx.data_unchecked::>(); // #268: gate rows via the same collector the REST list route uses (like // `ref_updates` shares `collect_visible_ref_updates` with its REST feed), @@ -127,7 +127,9 @@ impl QueryRoot { .data::() .ok() .map(|d| d.0.as_str()); - let after = after_created_at.as_deref().zip(after_id.as_deref()); + let after = + crate::api::tasks::parse_after_cursor(after_created_at.as_deref(), after_id.as_deref()) + .map_err(crate::graphql::graphql_app_err)?; let result = crate::api::tasks::collect_visible_tasks( db, status.as_deref(), @@ -138,11 +140,14 @@ impl QueryRoot { ) .await .map_err(crate::graphql::graphql_app_err)?; - Ok(result - .tasks - .into_iter() - .map(AgentTaskReadType::from) - .collect()) + Ok(TaskPageType { + items: result + .tasks + .into_iter() + .map(AgentTaskReadType::from) + .collect(), + incomplete: result.incomplete, + }) } async fn task(&self, ctx: &Context<'_>, id: String) -> Result> { @@ -487,7 +492,7 @@ mod tests { async fn tasks_negative_limit_clamped(pool: PgPool) { let db = db(pool).await; let schema = schema(db); - let resp = anon(&schema, "{ tasks(limit: -1) { id } }").await; + let resp = anon(&schema, "{ tasks(limit: -1) { items { id } } }").await; assert!( resp.errors.is_empty(), "negative limit must clamp, not fail: {:?}", @@ -525,7 +530,7 @@ mod tests { // surface is visibility-gated, and an anonymous caller sees none of // these repo-less tasks at all. The clamp is what this test pins, so it // needs a caller who can legitimately see all 201 rows. - let resp = authed(&schema, "{ tasks(limit: 5000) { id } }", OWNER).await; + let resp = authed(&schema, "{ tasks(limit: 5000) { items { id } } }", OWNER).await; assert_eq!(count_tasks(&resp), 200, "limit above 200 must clamp to 200"); } @@ -562,7 +567,7 @@ mod tests { let db = db(pool).await; seed_task(&db, "t1", OWNER).await; let schema = schema(db); - let resp = anon(&schema, "{ tasks { id } }").await; + let resp = anon(&schema, "{ tasks { items { id } } }").await; assert_eq!( count_tasks(&resp), 0, @@ -641,11 +646,18 @@ mod tests { } let schema = schema(db); - let resp = anon(&schema, "{ tasks(limit: 1) { id } }").await; + let resp = anon(&schema, "{ tasks(limit: 1) { items { id } incomplete } }").await; assert_eq!(count_tasks(&resp), 1); + assert!(!task_incomplete(&resp)); assert!(format!("{:?}", resp.data).contains("visible")); } + /// The candidate scan wall reports `incomplete: true` with no cursor + /// (#268 follow-up review): the server never discloses the id/created_at + /// of a denied row it stopped scanning on. A caller that wants to push + /// past the wall in one more request must supply `after`/`id` it already + /// legitimately knows, which this test stands in for directly rather than + /// reading it from a prior response, since none is offered. #[sqlx::test] async fn tasks_continuation_past_candidate_ceiling(pool: PgPool) { let db = db(pool).await; @@ -678,26 +690,67 @@ mod tests { } let schema = schema(db); - let resp = anon(&schema, "{ tasks(limit: 1) { id } }").await; + let resp = anon(&schema, "{ tasks(limit: 1) { items { id } incomplete } }").await; assert_eq!(count_tasks(&resp), 0); + assert!(task_incomplete(&resp)); + assert!(!format!("{:?}", resp.data).contains("hidden-")); let resp = anon( &schema, - r#"{ tasks(limit: 1, afterCreatedAt: "2026-01-02T00:00:00Z", afterId: "hidden-0999") { id } }"#, + r#"{ tasks(limit: 1, afterCreatedAt: "2026-01-02T00:00:00Z", afterId: "hidden-0999") { items { id } incomplete } }"#, ) .await; assert_eq!(count_tasks(&resp), 1); + assert!(!task_incomplete(&resp)); assert!(format!("{:?}", resp.data).contains("past-ceiling")); } + #[sqlx::test] + async fn tasks_rejects_partial_cursor_pair(pool: PgPool) { + let db = db(pool).await; + let schema = schema(db); + let resp = anon( + &schema, + r#"{ tasks(afterCreatedAt: "2026-01-01T00:00:00Z") { items { id } } }"#, + ) + .await; + assert!( + !resp.errors.is_empty(), + "a half-supplied cursor must be rejected, not treated as page one" + ); + } + + fn task_items(resp: &async_graphql::Response) -> &Vec { + assert!(resp.errors.is_empty(), "graphql errors: {:?}", resp.errors); + let async_graphql::Value::Object(obj) = &resp.data else { + panic!("data not an object: {:?}", resp.data); + }; + let async_graphql::Value::Object(page) = obj.get("tasks").expect("tasks key") else { + panic!("tasks not an object"); + }; + let async_graphql::Value::List(rows) = page.get("items").expect("items key") else { + panic!("items not a list"); + }; + rows + } + fn count_tasks(resp: &async_graphql::Response) -> usize { + task_items(resp).len() + } + + fn task_incomplete(resp: &async_graphql::Response) -> bool { assert!(resp.errors.is_empty(), "graphql errors: {:?}", resp.errors); let async_graphql::Value::Object(obj) = &resp.data else { panic!("data not an object: {:?}", resp.data); }; - let async_graphql::Value::List(rows) = obj.get("tasks").expect("tasks key") else { - panic!("tasks not a list"); + let async_graphql::Value::Object(page) = obj.get("tasks").expect("tasks key") else { + panic!("tasks not an object"); }; - rows.len() + let async_graphql::Value::Boolean(incomplete) = + page.get("incomplete").expect("incomplete key") + else { + panic!("incomplete not a bool"); + }; + *incomplete } } diff --git a/crates/gitlawb-node/src/graphql/types.rs b/crates/gitlawb-node/src/graphql/types.rs index ab806aed..fa6a81e8 100644 --- a/crates/gitlawb-node/src/graphql/types.rs +++ b/crates/gitlawb-node/src/graphql/types.rs @@ -70,6 +70,18 @@ pub struct AgentTaskReadType { pub deadline: Option, } +/// Wraps a `tasks` page with the same truncation signal the REST list route +/// exposes (`incomplete`), so a GraphQL caller behind a denied-row scan wall +/// can tell a short page from an exhaustive one instead of getting an +/// indistinguishable empty/short list. Carries no cursor, for the same reason +/// REST's `list_tasks` response does not: the scan wall lands on the last +/// *examined* candidate, not necessarily one the caller may see. +#[derive(SimpleObject, Clone)] +pub struct TaskPageType { + pub items: Vec, + pub incomplete: bool, +} + impl From for AgentTaskReadType { fn from(t: AgentTask) -> Self { Self { From 6dfbc26777e2d1b6e729c236e299bf3bb6430c38 Mon Sep 17 00:00:00 2001 From: euxaristia Date: Sat, 15 Aug 2026 16:18:27 -0400 Subject: [PATCH 08/17] Normalize task cursors and gate mutation endpoints behind visibility. Canonicalize RFC 3339 timestamps in parse_after_cursor to handle URL-decoded spaces, reject mixed cursor alias families, gate complete_task and fail_task behind get_visible_task so unreadable tasks 404 instead of leaking existence with 403, and only flag incomplete when hitting candidate ceilings on full SQL batches. Refs #268 --- crates/gitlawb-node/src/api/tasks.rs | 207 ++++++++++++++++---- crates/gitlawb-node/src/graphql/mutation.rs | 82 ++++++-- crates/gitlawb-node/src/graphql/query.rs | 13 +- 3 files changed, 250 insertions(+), 52 deletions(-) diff --git a/crates/gitlawb-node/src/api/tasks.rs b/crates/gitlawb-node/src/api/tasks.rs index 338f6196..3ae3ae40 100644 --- a/crates/gitlawb-node/src/api/tasks.rs +++ b/crates/gitlawb-node/src/api/tasks.rs @@ -216,6 +216,7 @@ pub(crate) async fn collect_visible_tasks( let mut cursor: Option<(String, String)> = after.map(|(ts, id)| (ts.to_string(), id.to_string())); let mut scanned = 0; + let mut last_batch_full = false; while scanned < MAX_TASK_SCAN_CANDIDATES { let batch_limit = MAX_VISIBLE_TASKS.min(MAX_TASK_SCAN_CANDIDATES - scanned); @@ -230,6 +231,7 @@ pub(crate) async fn collect_visible_tasks( ) .await?; if tasks.is_empty() { + last_batch_full = false; break; } scanned += tasks.len() as i64; @@ -261,13 +263,20 @@ pub(crate) async fn collect_visible_tasks( } } - if visible.len() == bounded_limit as usize || tasks.len() < batch_limit as usize { + if visible.len() == bounded_limit as usize { break; } + if tasks.len() < batch_limit as usize { + last_batch_full = false; + break; + } + last_batch_full = true; cursor = next_cursor; } - let incomplete = visible.len() < bounded_limit as usize && scanned >= MAX_TASK_SCAN_CANDIDATES; + let incomplete = visible.len() < bounded_limit as usize + && scanned >= MAX_TASK_SCAN_CANDIDATES + && last_batch_full; Ok(VisibleTasks { tasks: visible, @@ -345,6 +354,60 @@ pub async fn create_task( Ok((StatusCode::CREATED, Json(task_to_json(&task)))) } +/// Canonicalize an RFC3339 timestamp. If spaces were introduced by URL query decoding +/// (e.g. `+00:00` decoded as ` 00:00`), convert spaces back to `+` before parsing. +pub(crate) fn canonicalize_timestamp(raw: &str) -> crate::error::Result { + let normalized = if raw.contains(' ') { + raw.replace(' ', "+") + } else { + raw.to_string() + }; + let dt = chrono::DateTime::parse_from_rfc3339(&normalized) + .map_err(|e| AppError::BadRequest(format!("invalid timestamp format '{raw}': {e}")))?; + Ok(dt.to_rfc3339()) +} + +/// A cursor is two query fields (`created_at`, `id`) from either the `after_*` or `cursor_*` family. +/// Reject cross-family alias mixing, require both fields within a family, and canonicalize timestamps. +pub(crate) fn parse_after_cursor( + after_created_at: Option<&str>, + after_id: Option<&str>, + cursor_created_at: Option<&str>, + cursor_id: Option<&str>, +) -> crate::error::Result> { + let has_after = after_created_at.is_some() || after_id.is_some(); + let has_cursor = cursor_created_at.is_some() || cursor_id.is_some(); + if has_after && has_cursor { + return Err(AppError::BadRequest( + "cannot mix after_* and cursor_* parameter aliases".into(), + )); + } + let (raw_ts, raw_id) = if has_after { + match (after_created_at, after_id) { + (Some(ts), Some(id)) => (ts, id), + _ => { + return Err(AppError::BadRequest( + "after_created_at and after_id must be supplied together".into(), + )) + } + } + } else if has_cursor { + match (cursor_created_at, cursor_id) { + (Some(ts), Some(id)) => (ts, id), + _ => { + return Err(AppError::BadRequest( + "cursor_created_at and cursor_id must be supplied together".into(), + )) + } + } + } else { + return Ok(None); + }; + + let canonical_ts = canonicalize_timestamp(raw_ts)?; + Ok(Some((canonical_ts, raw_id.to_string()))) +} + /// GET /api/v1/tasks /// /// Open to anonymous callers, but every row is gated by `collect_visible_tasks` @@ -356,12 +419,15 @@ pub async fn list_tasks( auth: Option>, ) -> crate::error::Result> { let caller = auth.as_ref().map(|e| e.0 .0.as_str()); - let after = parse_after_cursor( - q.after_created_at - .as_deref() - .or(q.cursor_created_at.as_deref()), - q.after_id.as_deref().or(q.cursor_id.as_deref()), + let after_parsed = parse_after_cursor( + q.after_created_at.as_deref(), + q.after_id.as_deref(), + q.cursor_created_at.as_deref(), + q.cursor_id.as_deref(), )?; + let after = after_parsed + .as_ref() + .map(|(ts, id)| (ts.as_str(), id.as_str())); let result = collect_visible_tasks( &state.db, q.status.as_deref(), @@ -379,22 +445,6 @@ pub async fn list_tasks( }))) } -/// A cursor is two independently optional query fields (`created_at`, `id`); -/// treating a half-supplied pair as absent would silently restart the caller -/// at page one instead of surfacing the lost half. Require both together. -pub(crate) fn parse_after_cursor<'a>( - created_at: Option<&'a str>, - id: Option<&'a str>, -) -> crate::error::Result> { - match (created_at, id) { - (Some(ts), Some(id)) => Ok(Some((ts, id))), - (None, None) => Ok(None), - _ => Err(AppError::BadRequest( - "after_created_at and after_id must be supplied together".into(), - )), - } -} - /// GET /api/v1/tasks/{id} /// /// Gated the same way as `list_tasks` (#268): a task the caller may not see @@ -445,13 +495,10 @@ pub async fn complete_task( Path(id): Path, Json(body): Json, ) -> Result, (StatusCode, Json)> { - // Authorize the actor, not just bind their identity: the N13 signer-binding - // proved the caller was whoever they claimed, but never that they were the - // task's assignee. Load the task and require the caller to be its assignee; - // finish_task then transitions only a claimed task. - let existing = state - .db - .get_task(&id) + // Authorize the actor, not just bind their identity: the task must be visible + // to the caller (returning 404 for invisible tasks so existence is not leaked), + // and only the task's assignee may complete it. + let existing = get_visible_task(&state.db, &id, Some(&auth.0)) .await .map_err(|e| { ( @@ -499,12 +546,10 @@ pub async fn fail_task( Path(id): Path, Json(body): Json, ) -> Result, (StatusCode, Json)> { - // Authorize the actor, not just bind their identity (see complete_task): only - // the task's assignee may fail it, and finish_task transitions only a claimed - // task. - let existing = state - .db - .get_task(&id) + // Authorize the actor: the task must be visible to the caller (returning + // 404 for invisible tasks so existence is not leaked), and only the task's + // assignee may fail it. + let existing = get_visible_task(&state.db, &id, Some(&auth.0)) .await .map_err(|e| { ( @@ -1042,4 +1087,94 @@ mod visible_tasks_tests { let body = body_json(resp).await; assert_eq!(body["error"], "db_unavailable"); } + + #[sqlx::test] + async fn list_tasks_rejects_mixed_cursor_alias_families(pool: PgPool) { + let state = test_state(pool).await; + let resp = list_router(state) + .oneshot(anon_get( + "/api/v1/tasks?after_created_at=2026-01-01T00:00:00Z&cursor_id=some-id", + )) + .await + .unwrap(); + assert_eq!(resp.status(), StatusCode::BAD_REQUEST); + let body = body_json(resp).await; + assert_eq!(body["error"], "bad_request"); + } + + #[sqlx::test] + async fn list_tasks_accepts_and_canonicalizes_spaces_in_timestamp(pool: PgPool) { + let state = test_state(pool).await; + state + .db + .create_repo(&repo("public-repo", DELEGATOR, "public", true)) + .await + .unwrap(); + let mut t1 = task("t1", Some("public-repo"), DELEGATOR); + t1.created_at = "2026-01-01T00:00:00+00:00".into(); + state.db.create_task(&t1).await.unwrap(); + + let resp = list_router(state) + .oneshot(anon_get( + "/api/v1/tasks?after_created_at=2026-01-02T00:00:00+00:00&after_id=dummy", + )) + .await + .unwrap(); + assert_eq!(resp.status(), StatusCode::OK); + } + + fn full_task_router(state: crate::state::AppState) -> Router { + Router::new() + .route("/api/v1/tasks", axum::routing::get(super::list_tasks)) + .route("/api/v1/tasks/{id}", axum::routing::get(super::get_task)) + .route( + "/api/v1/tasks/{id}/complete", + axum::routing::post(super::complete_task), + ) + .route( + "/api/v1/tasks/{id}/fail", + axum::routing::post(super::fail_task), + ) + .with_state(state) + } + + #[sqlx::test] + async fn complete_and_fail_task_on_invisible_task_returns_404_not_403(pool: PgPool) { + let state = test_state(pool).await; + state + .db + .create_task(&task("t1", None, DELEGATOR)) + .await + .unwrap(); + + let complete_resp = full_task_router(state.clone()) + .oneshot(signed_request_as( + STRANGER, + Method::POST, + "/api/v1/tasks/t1/complete", + Body::from(r#"{"result":"done"}"#), + )) + .await + .unwrap(); + assert_eq!( + complete_resp.status(), + StatusCode::NOT_FOUND, + "completing an invisible task must 404, not leak existence via 403" + ); + + let fail_resp = full_task_router(state) + .oneshot(signed_request_as( + STRANGER, + Method::POST, + "/api/v1/tasks/t1/fail", + Body::from(r#"{"reason":"error"}"#), + )) + .await + .unwrap(); + assert_eq!( + fail_resp.status(), + StatusCode::NOT_FOUND, + "failing an invisible task must 404, not leak existence via 403" + ); + } } diff --git a/crates/gitlawb-node/src/graphql/mutation.rs b/crates/gitlawb-node/src/graphql/mutation.rs index 7fb7a1dc..d8b11d96 100644 --- a/crates/gitlawb-node/src/graphql/mutation.rs +++ b/crates/gitlawb-node/src/graphql/mutation.rs @@ -103,12 +103,12 @@ impl MutationRoot { let by_did = caller.to_string(); let db = ctx.data_unchecked::>(); let tx = ctx.data_unchecked::>(); - // Authorize the actor: binding by_did to the signer is necessary but not - // sufficient — only the task's assignee may finish it. - let existing = db - .get_task(&id) + // Authorize the actor: the task must be visible to the caller (returning + // not found for invisible tasks so existence is not leaked), and only + // the task's assignee may finish it. + let existing = crate::api::tasks::get_visible_task(db, &id, Some(caller)) .await - .map_err(crate::graphql::graphql_db_err)? + .map_err(crate::graphql::graphql_app_err)? .ok_or_else(|| async_graphql::Error::new("task not found"))?; if !crate::api::did_matches(caller, existing.assignee_did.as_deref().unwrap_or_default()) { return Err(async_graphql::Error::new( @@ -145,11 +145,12 @@ impl MutationRoot { let by_did = caller.to_string(); let db = ctx.data_unchecked::>(); let tx = ctx.data_unchecked::>(); - // Authorize the actor: only the task's assignee may fail it. - let existing = db - .get_task(&id) + // Authorize the actor: the task must be visible to the caller (returning + // not found for invisible tasks so existence is not leaked), and only + // the task's assignee may fail it. + let existing = crate::api::tasks::get_visible_task(db, &id, Some(caller)) .await - .map_err(crate::graphql::graphql_db_err)? + .map_err(crate::graphql::graphql_app_err)? .ok_or_else(|| async_graphql::Error::new("task not found"))?; if !crate::api::did_matches(caller, existing.assignee_did.as_deref().unwrap_or_default()) { return Err(async_graphql::Error::new( @@ -294,7 +295,7 @@ mod tests { payload: None, result: None, created_at: now.clone(), - updated_at: now, + updated_at: now.clone(), deadline: None, }; state.db.create_task(&task).await.expect("seed task"); @@ -311,14 +312,69 @@ mod tests { ) }; - // Stranger signs as themselves and passes byDid=self (so the signer - // binding passes), but is not the assignee → rejected by authorization. + // Stranger signs as themselves on a repo-less task they cannot see: + // invisible task returns "task not found" so existence is not leaked. let resp = schema .execute(Request::new(q(stranger)).data(AuthenticatedDid(stranger.into()))) .await; + assert!( + errors(&resp).contains("task not found"), + "an invisible task must return not found, got: {}", + errors(&resp) + ); + + // Seed a task on a public repo that stranger CAN see, but is not assignee of: + let pub_repo = crate::db::RepoRecord { + id: "pub-r".into(), + name: "pub-r".into(), + owner_did: "did:key:zGQLDELEGATORCCCCCCCCCCCCCCCCCCCCCCCCCC".into(), + description: None, + is_public: true, + default_branch: "main".into(), + created_at: chrono::Utc::now(), + updated_at: chrono::Utc::now(), + disk_path: "/tmp/pub-r".into(), + forked_from: None, + machine_id: None, + }; + state.db.create_repo(&pub_repo).await.expect("create repo"); + let pub_task = crate::db::AgentTask { + id: "task-pub".into(), + repo_id: Some("pub-r".into()), + kind: "build".into(), + status: "pending".into(), + delegator_did: "did:key:zGQLDELEGATORCCCCCCCCCCCCCCCCCCCCCCCCCC".into(), + assignee_did: None, + capability: "repo:write".into(), + ucan_token: None, + payload: None, + result: None, + created_at: now.clone(), + updated_at: now, + deadline: None, + }; + state + .db + .create_task(&pub_task) + .await + .expect("seed pub task"); + state + .db + .claim_task("task-pub", assignee) + .await + .expect("claim pub task"); + + let q_pub = |actor: &str| { + format!( + r#"mutation {{ completeTask(id: "task-pub", byDid: "{actor}", input: {{}}) {{ id status }} }}"# + ) + }; + let resp = schema + .execute(Request::new(q_pub(stranger)).data(AuthenticatedDid(stranger.into()))) + .await; assert!( errors(&resp).contains("assignee"), - "a non-assignee signer must be rejected: {}", + "a non-assignee signer on a visible task must be rejected: {}", errors(&resp) ); diff --git a/crates/gitlawb-node/src/graphql/query.rs b/crates/gitlawb-node/src/graphql/query.rs index 77038e26..96a2735b 100644 --- a/crates/gitlawb-node/src/graphql/query.rs +++ b/crates/gitlawb-node/src/graphql/query.rs @@ -127,9 +127,16 @@ impl QueryRoot { .data::() .ok() .map(|d| d.0.as_str()); - let after = - crate::api::tasks::parse_after_cursor(after_created_at.as_deref(), after_id.as_deref()) - .map_err(crate::graphql::graphql_app_err)?; + let after_parsed = crate::api::tasks::parse_after_cursor( + after_created_at.as_deref(), + after_id.as_deref(), + None, + None, + ) + .map_err(crate::graphql::graphql_app_err)?; + let after = after_parsed + .as_ref() + .map(|(ts, id)| (ts.as_str(), id.as_str())); let result = crate::api::tasks::collect_visible_tasks( db, status.as_deref(), From 017bb8ea60093a3d06aeef60a7c6f225fbef3d10 Mon Sep 17 00:00:00 2001 From: euxaristia Date: Sat, 15 Aug 2026 16:28:42 -0400 Subject: [PATCH 09/17] Update complete_task test in test_support to exercise both 404 on unreadable and 403 on non-assignee tasks. Refs #268 --- crates/gitlawb-node/src/test_support.rs | 28 +++++++++++++++++++++---- 1 file changed, 24 insertions(+), 4 deletions(-) diff --git a/crates/gitlawb-node/src/test_support.rs b/crates/gitlawb-node/src/test_support.rs index 2b5aef95..8fae1cc3 100644 --- a/crates/gitlawb-node/src/test_support.rs +++ b/crates/gitlawb-node/src/test_support.rs @@ -650,9 +650,12 @@ mod tests { let state = test_state(pool).await; state .db - .create_task(&seed_task("task-1", delegator)) + .create_repo(&seed_repo(delegator, "task-pub-repo")) .await - .expect("seed task"); + .expect("seed repo"); + let mut t1 = seed_task("task-1", delegator); + t1.repo_id = Some("task-pub-repo".to_string()); + state.db.create_task(&t1).await.expect("seed task"); // Assignee claims it: pending -> claimed, assignee_did = assignee. state .db @@ -671,8 +674,25 @@ mod tests { let uri = "/api/v1/tasks/task-1/complete"; let body = || Body::from("{}"); - // Stranger (not the assignee) is rejected by the authorization gate, even - // with the empty body that previously bypassed the binding. Exact 403. + // Stranger on an invisible (repo-less) task receives opaque 404 (no existence leak). + let inv_task = seed_task("task-inv", delegator); + state.db.create_task(&inv_task).await.unwrap(); + let inv_resp = router() + .oneshot(signed_request_as( + stranger, + Method::POST, + "/api/v1/tasks/task-inv/complete", + body(), + )) + .await + .unwrap(); + assert_eq!( + inv_resp.status(), + StatusCode::NOT_FOUND, + "an invisible task must 404 so existence is not leaked" + ); + + // Stranger on a visible task is rejected by the authorization gate with exact 403. let resp = router() .oneshot(signed_request_as(stranger, Method::POST, uri, body())) .await From 0be9e26f150cb223a2b48981183ce3f5294a596b Mon Sep 17 00:00:00 2001 From: euxaristia Date: Sat, 15 Aug 2026 16:40:05 -0400 Subject: [PATCH 10/17] Bind task to repo id in complete_task test in test_support. Refs #268 --- crates/gitlawb-node/src/test_support.rs | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/crates/gitlawb-node/src/test_support.rs b/crates/gitlawb-node/src/test_support.rs index 8fae1cc3..58090aa9 100644 --- a/crates/gitlawb-node/src/test_support.rs +++ b/crates/gitlawb-node/src/test_support.rs @@ -648,13 +648,10 @@ mod tests { let assignee = "did:key:zTASKASSIGNEEBBBBBBBBBBBBBBBBBBBBBBBBBBBB"; let stranger = "did:key:zTASKSTRANGERCCCCCCCCCCCCCCCCCCCCCCCCCCC"; let state = test_state(pool).await; - state - .db - .create_repo(&seed_repo(delegator, "task-pub-repo")) - .await - .expect("seed repo"); + let repo = seed_repo(delegator, "task-pub-repo"); + state.db.create_repo(&repo).await.expect("seed repo"); let mut t1 = seed_task("task-1", delegator); - t1.repo_id = Some("task-pub-repo".to_string()); + t1.repo_id = Some(repo.id); state.db.create_task(&t1).await.expect("seed task"); // Assignee claims it: pending -> claimed, assignee_did = assignee. state From f4aebc1fb38c4122ab99675293c8f9765ef0e4ec Mon Sep 17 00:00:00 2001 From: euxaristia Date: Sat, 15 Aug 2026 18:14:12 -0400 Subject: [PATCH 11/17] Preserve timestamp fractional width and anchor continuation tests on legitimate cursors. Refs #268 --- crates/gitlawb-node/src/api/tasks.rs | 197 +++++++++++++++++++---- crates/gitlawb-node/src/graphql/query.rs | 38 +++-- 2 files changed, 197 insertions(+), 38 deletions(-) diff --git a/crates/gitlawb-node/src/api/tasks.rs b/crates/gitlawb-node/src/api/tasks.rs index 3ae3ae40..2f1eb2b1 100644 --- a/crates/gitlawb-node/src/api/tasks.rs +++ b/crates/gitlawb-node/src/api/tasks.rs @@ -186,8 +186,11 @@ pub(crate) struct VisibleTasks { /// that point is the last *examined* candidate, which may be a task the /// caller was denied, and handing that back would let a denied read leak /// the id/created_at of a row `GET /tasks/{id}` otherwise 404s (the same - /// id `claim_task` accepts). A caller can still page with - /// `after_created_at`/`after_id` set to the last row *they* received. + /// id `claim_task` accepts). A caller can page with `after_created_at`/`after_id` + /// set to the last row they actually received; if a window of >= 1,000 + /// consecutive denied tasks intervenes before the next visible row, pagination + /// anchored on that received row stalls at the candidate ceiling and + /// repeatedly returns empty results with `incomplete: true`. pub incomplete: bool, } @@ -216,7 +219,6 @@ pub(crate) async fn collect_visible_tasks( let mut cursor: Option<(String, String)> = after.map(|(ts, id)| (ts.to_string(), id.to_string())); let mut scanned = 0; - let mut last_batch_full = false; while scanned < MAX_TASK_SCAN_CANDIDATES { let batch_limit = MAX_VISIBLE_TASKS.min(MAX_TASK_SCAN_CANDIDATES - scanned); @@ -231,7 +233,6 @@ pub(crate) async fn collect_visible_tasks( ) .await?; if tasks.is_empty() { - last_batch_full = false; break; } scanned += tasks.len() as i64; @@ -267,16 +268,12 @@ pub(crate) async fn collect_visible_tasks( break; } if tasks.len() < batch_limit as usize { - last_batch_full = false; break; } - last_batch_full = true; cursor = next_cursor; } - let incomplete = visible.len() < bounded_limit as usize - && scanned >= MAX_TASK_SCAN_CANDIDATES - && last_batch_full; + let incomplete = visible.len() < bounded_limit as usize && scanned >= MAX_TASK_SCAN_CANDIDATES; Ok(VisibleTasks { tasks: visible, @@ -356,15 +353,17 @@ pub async fn create_task( /// Canonicalize an RFC3339 timestamp. If spaces were introduced by URL query decoding /// (e.g. `+00:00` decoded as ` 00:00`), convert spaces back to `+` before parsing. +/// Validates RFC3339 syntax while preserving the original fractional precision +/// and string representation so comparisons against stored TEXT timestamps remain exact. pub(crate) fn canonicalize_timestamp(raw: &str) -> crate::error::Result { let normalized = if raw.contains(' ') { raw.replace(' ', "+") } else { raw.to_string() }; - let dt = chrono::DateTime::parse_from_rfc3339(&normalized) + chrono::DateTime::parse_from_rfc3339(&normalized) .map_err(|e| AppError::BadRequest(format!("invalid timestamp format '{raw}': {e}")))?; - Ok(dt.to_rfc3339()) + Ok(normalized) } /// A cursor is two query fields (`created_at`, `id`) from either the `after_*` or `cursor_*` family. @@ -986,10 +985,10 @@ mod visible_tasks_tests { .create_repo(&repo("public-repo", DELEGATOR, "public", true)) .await .unwrap(); - let mut visible = task("past-ceiling", Some("public-repo"), DELEGATOR); - visible.created_at = "2026-01-01T00:00:00Z".into(); - visible.updated_at = visible.created_at.clone(); - state.db.create_task(&visible).await.unwrap(); + let mut visible_newer = task("newer-visible", Some("public-repo"), DELEGATOR); + visible_newer.created_at = "2026-01-03T00:00:00Z".into(); + visible_newer.updated_at = visible_newer.created_at.clone(); + state.db.create_task(&visible_newer).await.unwrap(); for i in 0..MAX_TASK_SCAN_CANDIDATES { let mut hidden = task(&format!("hidden-{i:04}"), None, DELEGATOR); @@ -998,11 +997,30 @@ mod visible_tasks_tests { state.db.create_task(&hidden).await.unwrap(); } + let mut visible_older = task("past-ceiling", Some("public-repo"), DELEGATOR); + visible_older.created_at = "2026-01-01T00:00:00Z".into(); + visible_older.updated_at = visible_older.created_at.clone(); + state.db.create_task(&visible_older).await.unwrap(); + + // Page 1 yields the first visible task. let resp = list_router(state.clone()) .oneshot(anon_get("/api/v1/tasks?limit=1")) .await .unwrap(); let body = body_json(resp).await; + assert_eq!(body["count"], 1); + assert_eq!(body["incomplete"], false); + assert_eq!(body["tasks"][0]["id"], "newer-visible"); + + // Page 2 anchored on the last received row hits the 1,000 candidate scan ceiling + // across the intervening denied rows and signals incomplete without leaking any denied row. + let resp = list_router(state.clone()) + .oneshot(anon_get( + "/api/v1/tasks?limit=1&after_created_at=2026-01-03T00:00:00Z&after_id=newer-visible", + )) + .await + .unwrap(); + let body = body_json(resp).await; assert_eq!(body["count"], 0); assert_eq!(body["incomplete"], true); assert!(!body.to_string().contains("past-ceiling")); @@ -1015,26 +1033,23 @@ mod visible_tasks_tests { "response must not leak any denied row's id: {body}" ); - // No cursor is disclosed above, so resuming past the scan wall in one - // more request requires `after_created_at`/`after_id` the caller - // already legitimately knows. This stands in for that out-of-band - // knowledge with the last seeded row's known position. + // A subsequent request anchored on the same legitimately-held row stalls at the + // scan ceiling rather than bypassing authorization. let resp = list_router(state) - .oneshot(anon_get(&format!( - "/api/v1/tasks?limit=1&after_created_at=2026-01-02T00:00:00Z&after_id=hidden-{:04}", - MAX_TASK_SCAN_CANDIDATES - 1 - ))) + .oneshot(anon_get( + "/api/v1/tasks?limit=1&after_created_at=2026-01-03T00:00:00Z&after_id=newer-visible", + )) .await .unwrap(); let body = body_json(resp).await; - assert_eq!(body["count"], 1); - assert_eq!(body["incomplete"], false); - assert_eq!(body["tasks"][0]["id"], "past-ceiling"); + assert_eq!(body["count"], 0); + assert_eq!(body["incomplete"], true); } #[sqlx::test] async fn list_tasks_rejects_partial_cursor_pair(pool: PgPool) { let state = test_state(pool).await; + let resp = list_router(state.clone()) .oneshot(anon_get( "/api/v1/tasks?after_created_at=2026-01-01T00:00:00Z", @@ -1042,12 +1057,46 @@ mod visible_tasks_tests { .await .unwrap(); assert_eq!(resp.status(), StatusCode::BAD_REQUEST); + let body = body_json(resp).await; + assert_eq!( + body["message"], + "after_created_at and after_id must be supplied together" + ); - let resp = list_router(state) + let resp = list_router(state.clone()) .oneshot(anon_get("/api/v1/tasks?after_id=some-id")) .await .unwrap(); assert_eq!(resp.status(), StatusCode::BAD_REQUEST); + let body = body_json(resp).await; + assert_eq!( + body["message"], + "after_created_at and after_id must be supplied together" + ); + + let resp = list_router(state.clone()) + .oneshot(anon_get( + "/api/v1/tasks?cursor_created_at=2026-01-01T00:00:00Z", + )) + .await + .unwrap(); + assert_eq!(resp.status(), StatusCode::BAD_REQUEST); + let body = body_json(resp).await; + assert_eq!( + body["message"], + "cursor_created_at and cursor_id must be supplied together" + ); + + let resp = list_router(state) + .oneshot(anon_get("/api/v1/tasks?cursor_id=some-id")) + .await + .unwrap(); + assert_eq!(resp.status(), StatusCode::BAD_REQUEST); + let body = body_json(resp).await; + assert_eq!( + body["message"], + "cursor_created_at and cursor_id must be supplied together" + ); } #[sqlx::test] @@ -1091,7 +1140,22 @@ mod visible_tasks_tests { #[sqlx::test] async fn list_tasks_rejects_mixed_cursor_alias_families(pool: PgPool) { let state = test_state(pool).await; - let resp = list_router(state) + + let resp = list_router(state.clone()) + .oneshot(anon_get( + "/api/v1/tasks?after_created_at=2026-01-01T00:00:00Z&after_id=a&cursor_created_at=2026-01-01T00:00:00Z&cursor_id=b", + )) + .await + .unwrap(); + assert_eq!(resp.status(), StatusCode::BAD_REQUEST); + let body = body_json(resp).await; + assert_eq!(body["error"], "bad_request"); + assert_eq!( + body["message"], + "cannot mix after_* and cursor_* parameter aliases" + ); + + let resp = list_router(state.clone()) .oneshot(anon_get( "/api/v1/tasks?after_created_at=2026-01-01T00:00:00Z&cursor_id=some-id", )) @@ -1100,6 +1164,24 @@ mod visible_tasks_tests { assert_eq!(resp.status(), StatusCode::BAD_REQUEST); let body = body_json(resp).await; assert_eq!(body["error"], "bad_request"); + assert_eq!( + body["message"], + "cannot mix after_* and cursor_* parameter aliases" + ); + + let resp = list_router(state) + .oneshot(anon_get( + "/api/v1/tasks?cursor_created_at=2026-01-01T00:00:00Z&after_id=some-id", + )) + .await + .unwrap(); + assert_eq!(resp.status(), StatusCode::BAD_REQUEST); + let body = body_json(resp).await; + assert_eq!(body["error"], "bad_request"); + assert_eq!( + body["message"], + "cannot mix after_* and cursor_* parameter aliases" + ); } #[sqlx::test] @@ -1123,6 +1205,65 @@ mod visible_tasks_tests { assert_eq!(resp.status(), StatusCode::OK); } + #[sqlx::test] + async fn list_tasks_keyset_advances_across_trailing_zero_fraction_timestamps(pool: PgPool) { + let state = test_state(pool).await; + state + .db + .create_repo(&repo("public-repo", DELEGATOR, "public", true)) + .await + .unwrap(); + + let ts_sibling = "2026-06-01T00:00:00.000000000+00:00"; + let mut t2 = task("task-2", Some("public-repo"), DELEGATOR); + t2.created_at = ts_sibling.into(); + t2.updated_at = t2.created_at.clone(); + state.db.create_task(&t2).await.unwrap(); + + let mut t1 = task("task-1", Some("public-repo"), DELEGATOR); + t1.created_at = ts_sibling.into(); + t1.updated_at = t1.created_at.clone(); + state.db.create_task(&t1).await.unwrap(); + + let mut t0 = task("task-0", Some("public-repo"), DELEGATOR); + t0.created_at = "2026-05-01T00:00:00.000000000+00:00".into(); + t0.updated_at = t0.created_at.clone(); + state.db.create_task(&t0).await.unwrap(); + + let resp = list_router(state.clone()) + .oneshot(anon_get("/api/v1/tasks?limit=1")) + .await + .unwrap(); + let body = body_json(resp).await; + assert_eq!(body["count"], 1); + assert_eq!(body["tasks"][0]["id"], "task-2"); + let served_ts = body["tasks"][0]["created_at"].as_str().unwrap(); + assert_eq!(served_ts, ts_sibling); + + let resp = list_router(state.clone()) + .oneshot(anon_get(&format!( + "/api/v1/tasks?limit=1&after_created_at={served_ts}&after_id=task-2" + ))) + .await + .unwrap(); + let body = body_json(resp).await; + assert_eq!(body["count"], 1); + assert_eq!( + body["tasks"][0]["id"], "task-1", + "keyset pagination must advance to sibling row with equal timestamp" + ); + + let resp = list_router(state) + .oneshot(anon_get(&format!( + "/api/v1/tasks?limit=1&after_created_at={served_ts}&after_id=task-1" + ))) + .await + .unwrap(); + let body = body_json(resp).await; + assert_eq!(body["count"], 1); + assert_eq!(body["tasks"][0]["id"], "task-0"); + } + fn full_task_router(state: crate::state::AppState) -> Router { Router::new() .route("/api/v1/tasks", axum::routing::get(super::list_tasks)) diff --git a/crates/gitlawb-node/src/graphql/query.rs b/crates/gitlawb-node/src/graphql/query.rs index 96a2735b..bf765ce8 100644 --- a/crates/gitlawb-node/src/graphql/query.rs +++ b/crates/gitlawb-node/src/graphql/query.rs @@ -671,8 +671,8 @@ mod tests { db.create_repo(&repo("public-repo", OWNER, "public", true)) .await .unwrap(); - let visible = crate::db::AgentTask { - id: "past-ceiling".into(), + let mut visible_newer = crate::db::AgentTask { + id: "newer-visible".into(), repo_id: Some("public-repo".into()), kind: "build".into(), status: "pending".into(), @@ -682,13 +682,13 @@ mod tests { ucan_token: None, payload: None, result: None, - created_at: "2026-01-01T00:00:00Z".into(), - updated_at: "2026-01-01T00:00:00Z".into(), + created_at: "2026-01-03T00:00:00Z".into(), + updated_at: "2026-01-03T00:00:00Z".into(), deadline: None, }; - db.create_task(&visible).await.unwrap(); + db.create_task(&visible_newer).await.unwrap(); for i in 0..1000 { - let mut hidden = visible.clone(); + let mut hidden = visible_newer.clone(); hidden.id = format!("hidden-{i:04}"); hidden.repo_id = None; hidden.created_at = "2026-01-02T00:00:00Z".into(); @@ -696,20 +696,38 @@ mod tests { db.create_task(&hidden).await.unwrap(); } + let mut visible_older = visible_newer.clone(); + visible_older.id = "past-ceiling".into(); + visible_older.created_at = "2026-01-01T00:00:00Z".into(); + visible_older.updated_at = visible_older.created_at.clone(); + db.create_task(&visible_older).await.unwrap(); + let schema = schema(db); + // Page 1 returns the first visible task. let resp = anon(&schema, "{ tasks(limit: 1) { items { id } incomplete } }").await; + assert_eq!(count_tasks(&resp), 1); + assert!(!task_incomplete(&resp)); + assert_eq!(task_items(&resp)[0]["id"], "newer-visible"); + + // Page 2 anchored on the visible task hits the candidate ceiling across the denied window. + let resp = anon( + &schema, + r#"{ tasks(limit: 1, afterCreatedAt: "2026-01-03T00:00:00Z", afterId: "newer-visible") { items { id } incomplete } }"#, + ) + .await; assert_eq!(count_tasks(&resp), 0); assert!(task_incomplete(&resp)); assert!(!format!("{:?}", resp.data).contains("hidden-")); + assert!(!format!("{:?}", resp.data).contains("past-ceiling")); + // A repeated query on the same legitimately-held cursor stalls at the ceiling. let resp = anon( &schema, - r#"{ tasks(limit: 1, afterCreatedAt: "2026-01-02T00:00:00Z", afterId: "hidden-0999") { items { id } incomplete } }"#, + r#"{ tasks(limit: 1, afterCreatedAt: "2026-01-03T00:00:00Z", afterId: "newer-visible") { items { id } incomplete } }"#, ) .await; - assert_eq!(count_tasks(&resp), 1); - assert!(!task_incomplete(&resp)); - assert!(format!("{:?}", resp.data).contains("past-ceiling")); + assert_eq!(count_tasks(&resp), 0); + assert!(task_incomplete(&resp)); } #[sqlx::test] From e9cf8f92e76670a902ecb4fa28b4c080368d670a Mon Sep 17 00:00:00 2001 From: euxaristia Date: Sat, 15 Aug 2026 18:44:18 -0400 Subject: [PATCH 12/17] Match on Value::Object to extract task ID in GraphQL continuation test. Refs #268 --- crates/gitlawb-node/src/graphql/query.rs | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/crates/gitlawb-node/src/graphql/query.rs b/crates/gitlawb-node/src/graphql/query.rs index bf765ce8..fe542b2a 100644 --- a/crates/gitlawb-node/src/graphql/query.rs +++ b/crates/gitlawb-node/src/graphql/query.rs @@ -707,7 +707,13 @@ mod tests { let resp = anon(&schema, "{ tasks(limit: 1) { items { id } incomplete } }").await; assert_eq!(count_tasks(&resp), 1); assert!(!task_incomplete(&resp)); - assert_eq!(task_items(&resp)[0]["id"], "newer-visible"); + let async_graphql::Value::Object(first) = &task_items(&resp)[0] else { + panic!("expected task item to be an object"); + }; + assert_eq!( + first.get("id"), + Some(&async_graphql::Value::from("newer-visible")) + ); // Page 2 anchored on the visible task hits the candidate ceiling across the denied window. let resp = anon( From fe5d6c0e86f9f40ba7aabb073417168f3bdb7349 Mon Sep 17 00:00:00 2001 From: euxaristia Date: Sat, 15 Aug 2026 19:24:28 -0400 Subject: [PATCH 13/17] Remove unused mut binding on visible_newer in GraphQL query test. Refs #268 --- crates/gitlawb-node/src/graphql/query.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/gitlawb-node/src/graphql/query.rs b/crates/gitlawb-node/src/graphql/query.rs index fe542b2a..971499e3 100644 --- a/crates/gitlawb-node/src/graphql/query.rs +++ b/crates/gitlawb-node/src/graphql/query.rs @@ -671,7 +671,7 @@ mod tests { db.create_repo(&repo("public-repo", OWNER, "public", true)) .await .unwrap(); - let mut visible_newer = crate::db::AgentTask { + let visible_newer = crate::db::AgentTask { id: "newer-visible".into(), repo_id: Some("public-repo".into()), kind: "build".into(), From 0086337866a801c3ce933b35c7fefef74df6bb68 Mon Sep 17 00:00:00 2001 From: euxaristia Date: Sun, 16 Aug 2026 17:53:00 -0400 Subject: [PATCH 14/17] Hide unauthorized claim and keep assigned tasks from being stolen Gate REST and GraphQL claim behind the same visibility check as complete and fail, refuse claim when another assignee already holds the task, and only broadcast publicly visible task events. Treat a full list page as incomplete when more candidates remain. Surface HTTP errors from CLI and MCP claim and complete helpers. Refs #327 Co-Authored-By: cairn-code --- crates/gitlawb-node/src/api/tasks.rs | 135 ++++++++++++++++---- crates/gitlawb-node/src/db/mod.rs | 1 + crates/gitlawb-node/src/graphql/mutation.rs | 61 ++++++--- crates/gl/src/mcp.rs | 2 + crates/gl/src/task.rs | 6 + 5 files changed, 160 insertions(+), 45 deletions(-) diff --git a/crates/gitlawb-node/src/api/tasks.rs b/crates/gitlawb-node/src/api/tasks.rs index 2f1eb2b1..0d0941d7 100644 --- a/crates/gitlawb-node/src/api/tasks.rs +++ b/crates/gitlawb-node/src/api/tasks.rs @@ -264,16 +264,24 @@ pub(crate) async fn collect_visible_tasks( } } + let last_batch = tasks.len() < batch_limit as usize; if visible.len() == bounded_limit as usize { - break; + // A full page is incomplete when more candidates remain after this + // batch. Stopping here without that flag made a full page look + // like the end of the list. + let incomplete = !last_batch; + return Ok(VisibleTasks { + tasks: visible, + incomplete, + }); } - if tasks.len() < batch_limit as usize { + if last_batch { break; } cursor = next_cursor; } - let incomplete = visible.len() < bounded_limit as usize && scanned >= MAX_TASK_SCAN_CANDIDATES; + let incomplete = scanned >= MAX_TASK_SCAN_CANDIDATES; Ok(VisibleTasks { tasks: visible, @@ -314,6 +322,25 @@ pub(crate) async fn get_visible_task( Ok(task_visible(&task, caller, &repos_by_id, &rules_by_repo).then_some(task)) } +/// Broadcast a task event only when the task is publicly visible. +/// Matches `if announce` on ref updates: private-task status changes stay off +/// the unauthenticated GraphQL subscription. +pub(crate) async fn announce_task_event( + db: &crate::db::Db, + tx: &tokio::sync::broadcast::Sender, + event: TaskEventBroadcast, +) { + match get_visible_task(db, &event.task_id, None).await { + Ok(Some(_)) => { + let _ = tx.send(event); + } + Ok(None) => {} + Err(e) => { + tracing::warn!(error = %e, task_id = %event.task_id, "skipping task event broadcast"); + } + } +} + // ── Handlers ────────────────────────────────────────────────────────────────── /// POST /api/v1/tasks @@ -471,19 +498,40 @@ pub async fn claim_task( if !crate::api::did_matches(&auth.0, &body.assignee_did) { return Err(forbidden("assignee_did must be the authenticated signer")); } + // Same visibility gate as complete/fail: invisible tasks are 404 so + // existence is not leaked via a successful claim or a leaking 409. + get_visible_task(&state.db, &id, Some(&auth.0)) + .await + .map_err(|e| { + ( + StatusCode::INTERNAL_SERVER_ERROR, + Json(json!({ "error": e.to_string() })), + ) + })? + .ok_or_else(|| { + ( + StatusCode::NOT_FOUND, + Json(json!({ "error": "task not found" })), + ) + })?; let task = state.db.claim_task(&id, &auth.0).await.map_err(|e| { ( StatusCode::CONFLICT, Json(json!({ "error": e.to_string() })), ) })?; - let _ = state.task_event_tx.send(TaskEventBroadcast { - task_id: id, - old_status: "pending".to_string(), - new_status: "claimed".to_string(), - by_did: auth.0, - at: Utc::now().to_rfc3339(), - }); + announce_task_event( + &state.db, + &state.task_event_tx, + TaskEventBroadcast { + task_id: id, + old_status: "pending".to_string(), + new_status: "claimed".to_string(), + by_did: auth.0, + at: Utc::now().to_rfc3339(), + }, + ) + .await; Ok(Json(task_to_json(&task))) } @@ -528,13 +576,18 @@ pub async fn complete_task( Json(json!({ "error": e.to_string() })), ) })?; - let _ = state.task_event_tx.send(TaskEventBroadcast { - task_id: id, - old_status: "claimed".to_string(), - new_status: "completed".to_string(), - by_did, - at: Utc::now().to_rfc3339(), - }); + announce_task_event( + &state.db, + &state.task_event_tx, + TaskEventBroadcast { + task_id: id, + old_status: "claimed".to_string(), + new_status: "completed".to_string(), + by_did, + at: Utc::now().to_rfc3339(), + }, + ) + .await; Ok(Json(task_to_json(&task))) } @@ -580,13 +633,18 @@ pub async fn fail_task( Json(json!({ "error": e.to_string() })), ) })?; - let _ = state.task_event_tx.send(TaskEventBroadcast { - task_id: id, - old_status: "claimed".to_string(), - new_status: "failed".to_string(), - by_did, - at: Utc::now().to_rfc3339(), - }); + announce_task_event( + &state.db, + &state.task_event_tx, + TaskEventBroadcast { + task_id: id, + old_status: "claimed".to_string(), + new_status: "failed".to_string(), + by_did, + at: Utc::now().to_rfc3339(), + }, + ) + .await; Ok(Json(task_to_json(&task))) } @@ -1268,6 +1326,10 @@ mod visible_tasks_tests { Router::new() .route("/api/v1/tasks", axum::routing::get(super::list_tasks)) .route("/api/v1/tasks/{id}", axum::routing::get(super::get_task)) + .route( + "/api/v1/tasks/{id}/claim", + axum::routing::post(super::claim_task), + ) .route( "/api/v1/tasks/{id}/complete", axum::routing::post(super::complete_task), @@ -1318,4 +1380,29 @@ mod visible_tasks_tests { "failing an invisible task must 404, not leak existence via 403" ); } + + #[sqlx::test] + async fn claim_task_on_invisible_task_returns_404_not_success_or_409(pool: PgPool) { + let state = test_state(pool).await; + state + .db + .create_task(&task("t1", None, DELEGATOR)) + .await + .unwrap(); + + let claim_resp = full_task_router(state) + .oneshot(signed_request_as( + STRANGER, + Method::POST, + "/api/v1/tasks/t1/claim", + Body::from(format!(r#"{{"assignee_did":"{STRANGER}"}}"#)), + )) + .await + .unwrap(); + assert_eq!( + claim_resp.status(), + StatusCode::NOT_FOUND, + "claiming an invisible task must 404, not succeed or leak via 409" + ); + } } diff --git a/crates/gitlawb-node/src/db/mod.rs b/crates/gitlawb-node/src/db/mod.rs index 1a79ecd1..1d25888c 100644 --- a/crates/gitlawb-node/src/db/mod.rs +++ b/crates/gitlawb-node/src/db/mod.rs @@ -2920,6 +2920,7 @@ impl Db { let row = sqlx::query( "UPDATE agent_tasks SET status='claimed', assignee_did=$2, updated_at=$3 WHERE id=$1 AND status='pending' + AND (assignee_did IS NULL OR assignee_did = $2) RETURNING id, repo_id, kind, status, delegator_did, assignee_did, capability, ucan_token, payload, result, created_at, updated_at, deadline", ) .bind(id) diff --git a/crates/gitlawb-node/src/graphql/mutation.rs b/crates/gitlawb-node/src/graphql/mutation.rs index d8b11d96..b1c59cab 100644 --- a/crates/gitlawb-node/src/graphql/mutation.rs +++ b/crates/gitlawb-node/src/graphql/mutation.rs @@ -73,17 +73,26 @@ impl MutationRoot { let assignee_did = caller.to_string(); let db = ctx.data_unchecked::>(); let tx = ctx.data_unchecked::>(); + crate::api::tasks::get_visible_task(db, &id, Some(caller)) + .await + .map_err(crate::graphql::graphql_app_err)? + .ok_or_else(|| async_graphql::Error::new("task not found"))?; let task = db .claim_task(&id, &assignee_did) .await .map_err(crate::graphql::graphql_db_err)?; - let _ = tx.send(TaskEventBroadcast { - task_id: id, - old_status: "pending".to_string(), - new_status: "claimed".to_string(), - by_did: assignee_did, - at: Utc::now().to_rfc3339(), - }); + crate::api::tasks::announce_task_event( + db, + tx, + TaskEventBroadcast { + task_id: id, + old_status: "pending".to_string(), + new_status: "claimed".to_string(), + by_did: assignee_did, + at: Utc::now().to_rfc3339(), + }, + ) + .await; Ok(AgentTaskType::from(task)) } @@ -119,13 +128,18 @@ impl MutationRoot { .finish_task(&id, "completed", input.result.as_deref()) .await .map_err(crate::graphql::graphql_db_err)?; - let _ = tx.send(TaskEventBroadcast { - task_id: id, - old_status: "claimed".to_string(), - new_status: "completed".to_string(), - by_did, - at: Utc::now().to_rfc3339(), - }); + crate::api::tasks::announce_task_event( + db, + tx, + TaskEventBroadcast { + task_id: id, + old_status: "claimed".to_string(), + new_status: "completed".to_string(), + by_did, + at: Utc::now().to_rfc3339(), + }, + ) + .await; Ok(AgentTaskType::from(task)) } @@ -162,13 +176,18 @@ impl MutationRoot { .finish_task(&id, "failed", Some(&reason)) .await .map_err(crate::graphql::graphql_db_err)?; - let _ = tx.send(TaskEventBroadcast { - task_id: id, - old_status: "claimed".to_string(), - new_status: "failed".to_string(), - by_did, - at: Utc::now().to_rfc3339(), - }); + crate::api::tasks::announce_task_event( + db, + tx, + TaskEventBroadcast { + task_id: id, + old_status: "claimed".to_string(), + new_status: "failed".to_string(), + by_did, + at: Utc::now().to_rfc3339(), + }, + ) + .await; Ok(AgentTaskType::from(task)) } } diff --git a/crates/gl/src/mcp.rs b/crates/gl/src/mcp.rs index 959c4c2c..af13dbaa 100644 --- a/crates/gl/src/mcp.rs +++ b/crates/gl/src/mcp.rs @@ -1102,6 +1102,7 @@ async fn call_tool( let resp: Value = client .post(&format!("/api/v1/tasks/{id}/claim"), &body) .await? + .error_for_status()? .json() .await?; Ok(serde_json::to_string_pretty(&resp)?) @@ -1118,6 +1119,7 @@ async fn call_tool( let resp: Value = client .post(&format!("/api/v1/tasks/{id}/complete"), &body) .await? + .error_for_status()? .json() .await?; Ok(serde_json::to_string_pretty(&resp)?) diff --git a/crates/gl/src/task.rs b/crates/gl/src/task.rs index 9bfd73f8..9b469015 100644 --- a/crates/gl/src/task.rs +++ b/crates/gl/src/task.rs @@ -235,6 +235,8 @@ async fn cmd_claim(id: String, node: String, dir: Option) -> Result<()> .post(&format!("/api/v1/tasks/{}/claim", id), &body) .await .context("failed to claim task")? + .error_for_status() + .context("claim request rejected")? .json() .await .context("invalid JSON response")?; @@ -257,6 +259,8 @@ async fn cmd_complete( .post(&format!("/api/v1/tasks/{}/complete", id), &body) .await .context("failed to complete task")? + .error_for_status() + .context("complete request rejected")? .json() .await .context("invalid JSON response")?; @@ -279,6 +283,8 @@ async fn cmd_fail( .post(&format!("/api/v1/tasks/{}/fail", id), &body) .await .context("failed to fail task")? + .error_for_status() + .context("fail request rejected")? .json() .await .context("invalid JSON response")?; From 9ceab8d768834050b3f2671ba200d4555fe75ca5 Mon Sep 17 00:00:00 2001 From: euxaristia Date: Sun, 16 Aug 2026 19:27:32 -0400 Subject: [PATCH 15/17] Keep task-list incomplete for the scan ceiling only. A full visible page was flagged incomplete whenever the SQL batch was full, so the first page of any list with more than 200 candidates looked stalled. Align the GraphQL claim test with the visibility gate's not-found message. Refs #268 --- crates/gitlawb-node/src/api/tasks.rs | 8 +++----- crates/gitlawb-node/src/graphql/mutation.rs | 11 ++++++----- 2 files changed, 9 insertions(+), 10 deletions(-) diff --git a/crates/gitlawb-node/src/api/tasks.rs b/crates/gitlawb-node/src/api/tasks.rs index 0d0941d7..2507f422 100644 --- a/crates/gitlawb-node/src/api/tasks.rs +++ b/crates/gitlawb-node/src/api/tasks.rs @@ -266,13 +266,11 @@ pub(crate) async fn collect_visible_tasks( let last_batch = tasks.len() < batch_limit as usize; if visible.len() == bounded_limit as usize { - // A full page is incomplete when more candidates remain after this - // batch. Stopping here without that flag made a full page look - // like the end of the list. - let incomplete = !last_batch; + // Page filled before the scan ceiling. The caller pages from the + // last visible row; `incomplete` is reserved for the ceiling path. return Ok(VisibleTasks { tasks: visible, - incomplete, + incomplete: false, }); } if last_batch { diff --git a/crates/gitlawb-node/src/graphql/mutation.rs b/crates/gitlawb-node/src/graphql/mutation.rs index b1c59cab..54e167a2 100644 --- a/crates/gitlawb-node/src/graphql/mutation.rs +++ b/crates/gitlawb-node/src/graphql/mutation.rs @@ -238,9 +238,10 @@ mod tests { errors(&resp) ); - // 3. Signed as the claimed assignee → passes the auth gate. The missing - // task is a business error from claim_task, not a sqlx fault, so the - // actionable message must survive (not the opaque DB string) (#250). + // 3. Signed as the claimed assignee → passes the auth gate. A missing + // task is gated by get_visible_task as "task not found" (same as a + // denied id) before claim_task runs, and must stay a business + // message rather than the opaque DB string (#250). let resp = schema .execute(Request::new(&q).data(AuthenticatedDid(assignee.into()))) .await; @@ -250,8 +251,8 @@ mod tests { "matching signer must pass the auth gate: {errs}" ); assert!( - errs.contains("task not claimable"), - "claim race / missing task must keep its business message: {errs}" + errs.contains("task not found"), + "missing task must keep its business message: {errs}" ); assert!( !errs.contains(crate::graphql::GRAPHQL_DB_ERROR_MESSAGE), From f021f2e39b84080a5cf54a7e95486c702e77f4c0 Mon Sep 17 00:00:00 2001 From: euxaristia Date: Mon, 17 Aug 2026 20:24:01 -0400 Subject: [PATCH 16/17] fix(node): pin claim guards and map mutation errors through AppError Review required tests that go red if the pre-assigned claim predicate or the anonymous announce gate is deleted, and incomplete must not stay true when the candidate stream is exhausted at the scan ceiling. Route claim, complete, and fail through AppError so closed-pool outages stay 503 and 404s match the read envelope. Refs #268 --- crates/gitlawb-node/src/api/tasks.rs | 341 +++++++++++++++++++------ crates/gitlawb-node/src/error.rs | 14 + crates/gitlawb-node/src/graphql/mod.rs | 1 + 3 files changed, 280 insertions(+), 76 deletions(-) diff --git a/crates/gitlawb-node/src/api/tasks.rs b/crates/gitlawb-node/src/api/tasks.rs index 2507f422..61d5c832 100644 --- a/crates/gitlawb-node/src/api/tasks.rs +++ b/crates/gitlawb-node/src/api/tasks.rs @@ -33,6 +33,16 @@ fn forbidden(msg: &str) -> (StatusCode, Json) { ) } +/// Map a db-layer `anyhow` from claim/finish: connection-class sqlx failures +/// stay retryable 503, business "not claimable / not claimed" stays 409 with +/// a fixed message (not the anyhow text). +fn task_write_conflict(err: anyhow::Error, message: &str) -> AppError { + match AppError::from(err) { + db @ AppError::Db(_) => db, + _ => AppError::Conflict(message.into()), + } +} + // ── Request / response types ────────────────────────────────────────────────── #[derive(Deserialize)] @@ -180,17 +190,21 @@ pub(crate) fn task_visible( #[derive(Debug, Clone)] pub(crate) struct VisibleTasks { pub tasks: Vec, - /// True when the candidate scan hit `MAX_TASK_SCAN_CANDIDATES` before - /// filling `limit`, so the caller cannot tell an empty/short page from an - /// exhaustive one. Deliberately carries no cursor: the scan position at - /// that point is the last *examined* candidate, which may be a task the - /// caller was denied, and handing that back would let a denied read leak - /// the id/created_at of a row `GET /tasks/{id}` otherwise 404s (the same - /// id `claim_task` accepts). A caller can page with `after_created_at`/`after_id` - /// set to the last row they actually received; if a window of >= 1,000 - /// consecutive denied tasks intervenes before the next visible row, pagination - /// anchored on that received row stalls at the candidate ceiling and - /// repeatedly returns empty results with `incomplete: true`. + /// True when the candidate scan hit `MAX_TASK_SCAN_CANDIDATES` and more + /// rows remain, so the caller cannot tell an empty/short page from an + /// exhaustive one. False when the last fetched batch was short or a + /// one-row probe past the ceiling is empty: exactly + /// `MAX_TASK_SCAN_CANDIDATES` rows with nothing beyond is a finished + /// stream, not a wall. Deliberately carries no cursor: the scan position + /// at that point is the last *examined* candidate, which may be a task + /// the caller was denied, and handing that back would let a denied read + /// leak the id/created_at of a row `GET /tasks/{id}` otherwise 404s (the + /// same id `claim_task` accepts). A caller can page with + /// `after_created_at`/`after_id` set to the last row they actually + /// received; if a window of >= 1,000 consecutive denied tasks + /// intervenes before the next visible row, pagination anchored on that + /// received row stalls at the candidate ceiling and repeatedly returns + /// empty results with `incomplete: true`. pub incomplete: bool, } @@ -219,6 +233,7 @@ pub(crate) async fn collect_visible_tasks( let mut cursor: Option<(String, String)> = after.map(|(ts, id)| (ts.to_string(), id.to_string())); let mut scanned = 0; + let mut last_batch_full = false; while scanned < MAX_TASK_SCAN_CANDIDATES { let batch_limit = MAX_VISIBLE_TASKS.min(MAX_TASK_SCAN_CANDIDATES - scanned); @@ -233,6 +248,7 @@ pub(crate) async fn collect_visible_tasks( ) .await?; if tasks.is_empty() { + last_batch_full = false; break; } scanned += tasks.len() as i64; @@ -264,7 +280,7 @@ pub(crate) async fn collect_visible_tasks( } } - let last_batch = tasks.len() < batch_limit as usize; + last_batch_full = tasks.len() >= batch_limit as usize; if visible.len() == bounded_limit as usize { // Page filled before the scan ceiling. The caller pages from the // last visible row; `incomplete` is reserved for the ceiling path. @@ -273,13 +289,30 @@ pub(crate) async fn collect_visible_tasks( incomplete: false, }); } - if last_batch { + if !last_batch_full { break; } cursor = next_cursor; } - let incomplete = scanned >= MAX_TASK_SCAN_CANDIDATES; + // A full last batch at the ceiling is ambiguous: either more rows exist, + // or the table ended on an exact multiple of the batch size. Probe one + // more row so `incomplete` is false when the stream is exhausted. + let incomplete = if scanned >= MAX_TASK_SCAN_CANDIDATES && last_batch_full { + let more = db + .list_tasks_keyset( + status, + assignee_did, + 1, + cursor + .as_ref() + .map(|(created_at, id)| (created_at.as_str(), id.as_str())), + ) + .await?; + !more.is_empty() + } else { + false + }; Ok(VisibleTasks { tasks: visible, @@ -491,33 +524,22 @@ pub async fn claim_task( Extension(auth): Extension, Path(id): Path, Json(body): Json, -) -> Result, (StatusCode, Json)> { +) -> crate::error::Result> { // Bind the assignee to the authenticated signer (N13). if !crate::api::did_matches(&auth.0, &body.assignee_did) { - return Err(forbidden("assignee_did must be the authenticated signer")); + return Err(AppError::Forbidden( + "assignee_did must be the authenticated signer".into(), + )); } // Same visibility gate as complete/fail: invisible tasks are 404 so // existence is not leaked via a successful claim or a leaking 409. get_visible_task(&state.db, &id, Some(&auth.0)) - .await - .map_err(|e| { - ( - StatusCode::INTERNAL_SERVER_ERROR, - Json(json!({ "error": e.to_string() })), - ) - })? - .ok_or_else(|| { - ( - StatusCode::NOT_FOUND, - Json(json!({ "error": "task not found" })), - ) + .await? + .ok_or_else(|| AppError::NotFound("task not found".into()))?; + let task = + state.db.claim_task(&id, &auth.0).await.map_err(|e| { + task_write_conflict(e, "task not claimable: not found or already claimed") })?; - let task = state.db.claim_task(&id, &auth.0).await.map_err(|e| { - ( - StatusCode::CONFLICT, - Json(json!({ "error": e.to_string() })), - ) - })?; announce_task_event( &state.db, &state.task_event_tx, @@ -539,41 +561,27 @@ pub async fn complete_task( Extension(auth): Extension, Path(id): Path, Json(body): Json, -) -> Result, (StatusCode, Json)> { +) -> crate::error::Result> { // Authorize the actor, not just bind their identity: the task must be visible // to the caller (returning 404 for invisible tasks so existence is not leaked), // and only the task's assignee may complete it. let existing = get_visible_task(&state.db, &id, Some(&auth.0)) - .await - .map_err(|e| { - ( - StatusCode::INTERNAL_SERVER_ERROR, - Json(json!({ "error": e.to_string() })), - ) - })? - .ok_or_else(|| { - ( - StatusCode::NOT_FOUND, - Json(json!({ "error": "task not found" })), - ) - })?; + .await? + .ok_or_else(|| AppError::NotFound("task not found".into()))?; if !crate::api::did_matches( &auth.0, existing.assignee_did.as_deref().unwrap_or_default(), ) { - return Err(forbidden("only the task assignee can complete it")); + return Err(AppError::Forbidden( + "only the task assignee can complete it".into(), + )); } let by_did = auth.0; let task = state .db .finish_task(&id, "completed", body.result.as_deref()) .await - .map_err(|e| { - ( - StatusCode::CONFLICT, - Json(json!({ "error": e.to_string() })), - ) - })?; + .map_err(|e| task_write_conflict(e, "task not found or not in claimed state"))?; announce_task_event( &state.db, &state.task_event_tx, @@ -595,29 +603,20 @@ pub async fn fail_task( Extension(auth): Extension, Path(id): Path, Json(body): Json, -) -> Result, (StatusCode, Json)> { +) -> crate::error::Result> { // Authorize the actor: the task must be visible to the caller (returning // 404 for invisible tasks so existence is not leaked), and only the task's // assignee may fail it. let existing = get_visible_task(&state.db, &id, Some(&auth.0)) - .await - .map_err(|e| { - ( - StatusCode::INTERNAL_SERVER_ERROR, - Json(json!({ "error": e.to_string() })), - ) - })? - .ok_or_else(|| { - ( - StatusCode::NOT_FOUND, - Json(json!({ "error": "task not found" })), - ) - })?; + .await? + .ok_or_else(|| AppError::NotFound("task not found".into()))?; if !crate::api::did_matches( &auth.0, existing.assignee_did.as_deref().unwrap_or_default(), ) { - return Err(forbidden("only the task assignee can fail it")); + return Err(AppError::Forbidden( + "only the task assignee can fail it".into(), + )); } let by_did = auth.0; let reason = body.reason.unwrap_or_default(); @@ -625,12 +624,7 @@ pub async fn fail_task( .db .finish_task(&id, "failed", Some(&reason)) .await - .map_err(|e| { - ( - StatusCode::CONFLICT, - Json(json!({ "error": e.to_string() })), - ) - })?; + .map_err(|e| task_write_conflict(e, "task not found or not in claimed state"))?; announce_task_event( &state.db, &state.task_event_tx, @@ -1339,6 +1333,14 @@ mod visible_tasks_tests { .with_state(state) } + fn assert_not_found_envelope(body: &serde_json::Value) { + assert_eq!(body["error"], "not_found"); + assert_eq!(body["message"], "task not found"); + let serialized = body.to_string(); + assert!(!serialized.contains(SECRET_UCAN)); + assert!(!serialized.contains("payload-data")); + } + #[sqlx::test] async fn complete_and_fail_task_on_invisible_task_returns_404_not_403(pool: PgPool) { let state = test_state(pool).await; @@ -1362,6 +1364,7 @@ mod visible_tasks_tests { StatusCode::NOT_FOUND, "completing an invisible task must 404, not leak existence via 403" ); + assert_not_found_envelope(&body_json(complete_resp).await); let fail_resp = full_task_router(state) .oneshot(signed_request_as( @@ -1377,6 +1380,7 @@ mod visible_tasks_tests { StatusCode::NOT_FOUND, "failing an invisible task must 404, not leak existence via 403" ); + assert_not_found_envelope(&body_json(fail_resp).await); } #[sqlx::test] @@ -1402,5 +1406,190 @@ mod visible_tasks_tests { StatusCode::NOT_FOUND, "claiming an invisible task must 404, not succeed or leak via 409" ); + assert_not_found_envelope(&body_json(claim_resp).await); + } + + /// Goes RED if `claim_task`'s `assignee_did IS NULL OR assignee_did = $2` + /// predicate is deleted: a public-repo pre-assigned task is visible to a + /// stranger, so only the SQL guard stops them from overwriting the + /// designated assignee. + #[sqlx::test] + async fn claim_task_does_not_steal_preassigned_assignee(pool: PgPool) { + let state = test_state(pool).await; + state + .db + .create_repo(&repo("public-repo", DELEGATOR, "public", true)) + .await + .unwrap(); + let mut assigned = task("preassigned", Some("public-repo"), DELEGATOR); + assigned.assignee_did = Some(ASSIGNEE.into()); + state.db.create_task(&assigned).await.unwrap(); + + let stranger_resp = full_task_router(state.clone()) + .oneshot(signed_request_as( + STRANGER, + Method::POST, + "/api/v1/tasks/preassigned/claim", + Body::from(format!(r#"{{"assignee_did":"{STRANGER}"}}"#)), + )) + .await + .unwrap(); + assert_eq!( + stranger_resp.status(), + StatusCode::CONFLICT, + "a stranger must not claim a task pre-assigned to someone else" + ); + let stranger_body = body_json(stranger_resp).await; + assert!(!stranger_body.to_string().contains(SECRET_UCAN)); + assert_eq!( + state + .db + .get_task("preassigned") + .await + .unwrap() + .unwrap() + .assignee_did + .as_deref(), + Some(ASSIGNEE), + "hostile claim must leave the designated assignee in place" + ); + + let assignee_resp = full_task_router(state.clone()) + .oneshot(signed_request_as( + ASSIGNEE, + Method::POST, + "/api/v1/tasks/preassigned/claim", + Body::from(format!(r#"{{"assignee_did":"{ASSIGNEE}"}}"#)), + )) + .await + .unwrap(); + assert_eq!(assignee_resp.status(), StatusCode::OK); + let claimed = body_json(assignee_resp).await; + assert_eq!(claimed["status"], "claimed"); + assert_eq!(claimed["assignee_did"], ASSIGNEE); + + let second_resp = full_task_router(state) + .oneshot(signed_request_as( + STRANGER, + Method::POST, + "/api/v1/tasks/preassigned/claim", + Body::from(format!(r#"{{"assignee_did":"{STRANGER}"}}"#)), + )) + .await + .unwrap(); + assert_eq!( + second_resp.status(), + StatusCode::CONFLICT, + "a second claim after the assignee took the task must be refused" + ); + } + + /// Goes RED if `announce_task_event` is replaced with a bare `tx.send`: + /// a repo-less claim would then reach an anonymous subscriber. + #[sqlx::test] + async fn announce_task_event_skips_tasks_invisible_to_anonymous(pool: PgPool) { + let state = test_state(pool).await; + state + .db + .create_repo(&repo("public-repo", DELEGATOR, "public", true)) + .await + .unwrap(); + state + .db + .create_task(&task("pub-t", Some("public-repo"), DELEGATOR)) + .await + .unwrap(); + state + .db + .create_task(&task("priv-t", None, DELEGATOR)) + .await + .unwrap(); + + let mut events = state.task_event_tx.subscribe(); + + let pub_resp = full_task_router(state.clone()) + .oneshot(signed_request_as( + ASSIGNEE, + Method::POST, + "/api/v1/tasks/pub-t/claim", + Body::from(format!(r#"{{"assignee_did":"{ASSIGNEE}"}}"#)), + )) + .await + .unwrap(); + assert_eq!(pub_resp.status(), StatusCode::OK); + let broadcast = events + .try_recv() + .expect("a publicly visible claim must broadcast"); + assert_eq!(broadcast.task_id, "pub-t"); + assert_eq!(broadcast.new_status, "claimed"); + + let priv_resp = full_task_router(state) + .oneshot(signed_request_as( + DELEGATOR, + Method::POST, + "/api/v1/tasks/priv-t/claim", + Body::from(format!(r#"{{"assignee_did":"{DELEGATOR}"}}"#)), + )) + .await + .unwrap(); + assert_eq!(priv_resp.status(), StatusCode::OK); + assert!( + events.try_recv().is_err(), + "a repo-less claim must not reach an anonymous subscriber" + ); + } + + #[sqlx::test] + async fn exhausted_scan_of_exactly_ceiling_candidates_is_not_incomplete(pool: PgPool) { + let state = test_state(pool).await; + for i in 0..MAX_TASK_SCAN_CANDIDATES { + let mut hidden = task(&format!("hidden-{i:04}"), None, DELEGATOR); + hidden.created_at = "2026-01-02T00:00:00Z".into(); + hidden.updated_at = hidden.created_at.clone(); + state.db.create_task(&hidden).await.unwrap(); + } + + let resp = list_router(state) + .oneshot(anon_get("/api/v1/tasks?limit=1")) + .await + .unwrap(); + let body = body_json(resp).await; + assert_eq!(body["count"], 0); + assert_eq!( + body["incomplete"], false, + "exactly {MAX_TASK_SCAN_CANDIDATES} denied rows with nothing beyond is a finished stream" + ); + } + + #[sqlx::test] + async fn task_mutations_closed_pool_returns_503_db_unavailable(pool: PgPool) { + let state = test_state(pool.clone()).await; + pool.close().await; + + for (uri, body) in [ + ( + "/api/v1/tasks/t1/claim", + format!(r#"{{"assignee_did":"{ASSIGNEE}"}}"#), + ), + ("/api/v1/tasks/t1/complete", r#"{"result":"done"}"#.into()), + ("/api/v1/tasks/t1/fail", r#"{"reason":"error"}"#.into()), + ] { + let resp = full_task_router(state.clone()) + .oneshot(signed_request_as( + ASSIGNEE, + Method::POST, + uri, + Body::from(body), + )) + .await + .unwrap(); + assert_eq!( + resp.status(), + StatusCode::SERVICE_UNAVAILABLE, + "{uri}: closed-pool outage during visibility pre-check must be retryable 503" + ); + let json = body_json(resp).await; + assert_eq!(json["error"], "db_unavailable", "{uri}"); + } } } diff --git a/crates/gitlawb-node/src/error.rs b/crates/gitlawb-node/src/error.rs index f5e14df1..6d41daf9 100644 --- a/crates/gitlawb-node/src/error.rs +++ b/crates/gitlawb-node/src/error.rs @@ -12,6 +12,9 @@ pub enum AppError { #[error("repo already exists: {0}")] RepoExists(String), + #[error("conflict: {0}")] + Conflict(String), + #[error("not found: {0}")] NotFound(String), @@ -146,6 +149,7 @@ impl IntoResponse for AppError { "repo_exists", format!("repository '{r}' already exists"), ), + AppError::Conflict(msg) => (StatusCode::CONFLICT, "conflict", msg.clone()), AppError::NotFound(msg) => (StatusCode::NOT_FOUND, "not_found", msg.clone()), AppError::Unauthorized(msg) => (StatusCode::UNAUTHORIZED, "not_an_agent", msg.clone()), AppError::Forbidden(msg) => (StatusCode::FORBIDDEN, "forbidden", msg.clone()), @@ -226,6 +230,16 @@ pub type Result = std::result::Result; mod tests { use super::*; + #[test] + fn conflict_maps_to_409() { + assert_eq!( + AppError::Conflict("task not claimable".into()) + .into_response() + .status(), + StatusCode::CONFLICT + ); + } + #[test] fn timeout_maps_to_504_distinct_from_git_500() { assert_eq!( diff --git a/crates/gitlawb-node/src/graphql/mod.rs b/crates/gitlawb-node/src/graphql/mod.rs index 181bcc22..6b16fd17 100644 --- a/crates/gitlawb-node/src/graphql/mod.rs +++ b/crates/gitlawb-node/src/graphql/mod.rs @@ -60,6 +60,7 @@ pub(crate) fn graphql_app_err(e: crate::error::AppError) -> async_graphql::Error // Curated client-safe variants — `Display` is intentional API text. safe @ (crate::error::AppError::RepoNotFound(_) | crate::error::AppError::RepoExists(_) + | crate::error::AppError::Conflict(_) | crate::error::AppError::NotFound(_) | crate::error::AppError::Unauthorized(_) | crate::error::AppError::Forbidden(_) From f5c2d7779f96e41fae5a46ff76c206624a3297ae Mon Sep 17 00:00:00 2001 From: euxaristia Date: Tue, 18 Aug 2026 04:34:33 -0400 Subject: [PATCH 17/17] fix(node): match bare and did:key assignee forms in claim and list create_task stores the supplied assignee unchanged, so a raw SQL equality check drops a designated assignee who presents the other did:key form. Compare the normalized key so claim and filtered list agree with did_matches. Refs #268 --- crates/gitlawb-node/src/api/tasks.rs | 108 +++++++++++++++++++++++++++ crates/gitlawb-node/src/db/mod.rs | 96 +++++++++++++++++++----- 2 files changed, 186 insertions(+), 18 deletions(-) diff --git a/crates/gitlawb-node/src/api/tasks.rs b/crates/gitlawb-node/src/api/tasks.rs index 61d5c832..49ec890c 100644 --- a/crates/gitlawb-node/src/api/tasks.rs +++ b/crates/gitlawb-node/src/api/tasks.rs @@ -1484,6 +1484,114 @@ mod visible_tasks_tests { ); } + /// `create_task` stores the supplied assignee form unchanged. Claim binds + /// the authenticated DID (typically `did:key:...`) and list filters pass + /// the query string through. Both SQL comparisons must collapse the + /// did:key short form, or a designated assignee stored as a bare key + /// cannot claim, and a `?assignee_did=` filter in the other form drops + /// the row. A `did:web:` assignee sharing the same residual must stay + /// unmatched. + #[sqlx::test] + async fn claim_and_list_match_bare_and_did_key_assignee_forms(pool: PgPool) { + let bare_assignee = crate::db::normalize_owner_key(ASSIGNEE); + assert_ne!( + bare_assignee, ASSIGNEE, + "test setup requires ASSIGNEE to be the full did:key form" + ); + + let state = test_state(pool).await; + state + .db + .create_repo(&repo("public-repo", DELEGATOR, "public", true)) + .await + .unwrap(); + + let mut bare = task("bare-assignee", Some("public-repo"), DELEGATOR); + bare.assignee_did = Some(bare_assignee.into()); + state.db.create_task(&bare).await.unwrap(); + + let mut full = task("full-assignee", Some("public-repo"), DELEGATOR); + full.assignee_did = Some(ASSIGNEE.into()); + state.db.create_task(&full).await.unwrap(); + + let mut web = task("web-assignee", Some("public-repo"), DELEGATOR); + web.assignee_did = Some(format!("did:web:{bare_assignee}")); + state.db.create_task(&web).await.unwrap(); + + let listed_ids = |body: &serde_json::Value| -> Vec { + body["tasks"] + .as_array() + .unwrap() + .iter() + .map(|task| task["id"].as_str().unwrap().to_string()) + .collect() + }; + + let full_filter = list_router(state.clone()) + .oneshot(anon_get(&format!("/api/v1/tasks?assignee_did={ASSIGNEE}"))) + .await + .unwrap(); + assert_eq!(full_filter.status(), StatusCode::OK); + let full_ids = listed_ids(&body_json(full_filter).await); + assert!( + full_ids.contains(&"bare-assignee".to_string()), + "a did:key: filter must match a bare stored assignee" + ); + assert!(full_ids.contains(&"full-assignee".to_string())); + assert!( + !full_ids.contains(&"web-assignee".to_string()), + "did:key matching must not collapse a did:web assignee" + ); + + let bare_filter = list_router(state.clone()) + .oneshot(anon_get(&format!( + "/api/v1/tasks?assignee_did={bare_assignee}" + ))) + .await + .unwrap(); + assert_eq!(bare_filter.status(), StatusCode::OK); + let bare_ids = listed_ids(&body_json(bare_filter).await); + assert!( + bare_ids.contains(&"full-assignee".to_string()), + "a bare filter must match a did:key stored assignee" + ); + assert!(bare_ids.contains(&"bare-assignee".to_string())); + assert!( + !bare_ids.contains(&"web-assignee".to_string()), + "did:key matching must not collapse a did:web assignee" + ); + + let claim_full = full_task_router(state.clone()) + .oneshot(signed_request_as( + ASSIGNEE, + Method::POST, + "/api/v1/tasks/bare-assignee/claim", + Body::from(format!(r#"{{"assignee_did":"{ASSIGNEE}"}}"#)), + )) + .await + .unwrap(); + assert_eq!( + claim_full.status(), + StatusCode::OK, + "claim as did:key: form must match a bare stored assignee" + ); + + let claim_bare = full_task_router(state) + .oneshot(signed_request_as( + bare_assignee, + Method::POST, + "/api/v1/tasks/full-assignee/claim", + Body::from(format!(r#"{{"assignee_did":"{bare_assignee}"}}"#)), + )) + .await + .unwrap(); + assert_eq!( + claim_bare.status(), + StatusCode::OK, + "claim as a bare key must match a did:key stored assignee" + ); + } + /// Goes RED if `announce_task_event` is replaced with a bare `tx.send`: /// a repo-less claim would then reach an anonymous subscriber. #[sqlx::test] diff --git a/crates/gitlawb-node/src/db/mod.rs b/crates/gitlawb-node/src/db/mod.rs index 1d25888c..80a51fff 100644 --- a/crates/gitlawb-node/src/db/mod.rs +++ b/crates/gitlawb-node/src/db/mod.rs @@ -921,6 +921,10 @@ const OWNER_KEY_CASE_SQL: &str = "CASE WHEN owner_did LIKE 'did:key:%' AND posit /// named `did` (like in agent_profiles) instead of `owner_did`. const PROFILE_DID_CASE_SQL: &str = "CASE WHEN did LIKE 'did:key:%' AND position(':' in substr(did, 9)) = 0 THEN substr(did, 9) ELSE did END"; +/// SQL CASE expression byte-identical to `normalize_owner_key`, but for the +/// `assignee_did` column on `agent_tasks`. +const ASSIGNEE_DID_CASE_SQL: &str = "CASE WHEN assignee_did LIKE 'did:key:%' AND position(':' in substr(assignee_did, 9)) = 0 THEN substr(assignee_did, 9) ELSE assignee_did END"; + #[cfg(test)] mod normalize_owner_key_tests { use super::normalize_owner_key; @@ -2896,38 +2900,51 @@ impl Db { limit: i64, after: Option<(&str, &str)>, ) -> Result> { - let rows = sqlx::query( + // create_task stores the supplied assignee form unchanged. Compare the + // did:key short form so a `did:key:z...` filter matches a bare `z...` + // row (and the reverse), matching `did_matches` on the read path. + let assignee_key = assignee_did.map(normalize_owner_key); + let sql = format!( "SELECT id, repo_id, kind, status, delegator_did, assignee_did, capability, ucan_token, payload, result, created_at, updated_at, deadline FROM agent_tasks WHERE ($1::text IS NULL OR status = $1) - AND ($2::text IS NULL OR assignee_did = $2) + AND ($2::text IS NULL OR ({key}) = $2) AND ($3::text IS NULL OR (created_at, id) < ($3, $4)) ORDER BY created_at DESC, id DESC LIMIT $5", - ) - .bind(status) - .bind(assignee_did) - .bind(after.map(|cursor| cursor.0)) - .bind(after.map(|cursor| cursor.1)) - .bind(limit) - .fetch_all(&self.pool) - .await?; + key = ASSIGNEE_DID_CASE_SQL + ); + let rows = sqlx::query(&sql) + .bind(status) + .bind(assignee_key) + .bind(after.map(|cursor| cursor.0)) + .bind(after.map(|cursor| cursor.1)) + .bind(limit) + .fetch_all(&self.pool) + .await?; Ok(rows.into_iter().map(row_to_task).collect()) } pub async fn claim_task(&self, id: &str, assignee_did: &str) -> Result { let now = Utc::now().to_rfc3339(); - let row = sqlx::query( + // Bind the presented DID for the write, and the normalized key for the + // pre-assignment guard. A designated assignee stored as a bare key + // must still be able to claim when the signer presents `did:key:...`. + let assignee_key = normalize_owner_key(assignee_did); + let sql = format!( "UPDATE agent_tasks SET status='claimed', assignee_did=$2, updated_at=$3 WHERE id=$1 AND status='pending' - AND (assignee_did IS NULL OR assignee_did = $2) + AND (assignee_did IS NULL OR ({key}) = $4) RETURNING id, repo_id, kind, status, delegator_did, assignee_did, capability, ucan_token, payload, result, created_at, updated_at, deadline", - ) - .bind(id) - .bind(assignee_did) - .bind(&now) - .fetch_optional(&self.pool) - .await?; + key = ASSIGNEE_DID_CASE_SQL + ); + let row = sqlx::query(&sql) + .bind(id) + .bind(assignee_did) + .bind(&now) + .bind(assignee_key) + .fetch_optional(&self.pool) + .await?; row.map(row_to_task) .ok_or_else(|| anyhow::anyhow!("task not claimable: not found or already claimed")) } @@ -5132,6 +5149,49 @@ mod dedup_db_tests { ); } } + + /// Verify that `ASSIGNEE_DID_CASE_SQL` (which aliases `assignee_did`) also + /// agrees with Rust `normalize_owner_key` across the full boundary matrix. + #[sqlx::test] + async fn assignee_did_case_sql_matches_normalize_owner_key(pool: PgPool) { + let boundary_values = [ + "did:key:z6Mkfoo", + "z6Mkfoo", + "did:gitlawb:z6Mkfoo", + "did:web:example.com:alice", + "did:key:did:gitlawb:z6Mkfoo", + "", + "did:key:", + "DID:KEY:z6Mkfoo", + ]; + + let values_sql: String = boundary_values + .iter() + .map(|v| format!("('{}'::text)", v)) + .collect::>() + .join(", "); + let sql = format!( + "WITH data(assignee_did) AS (VALUES {values_sql}) + SELECT assignee_did, ({key}) AS normalized FROM data ORDER BY assignee_did", + key = super::ASSIGNEE_DID_CASE_SQL + ); + + let rows: Vec<(String, String)> = sqlx::query_as(&sql).fetch_all(&pool).await.unwrap(); + + assert_eq!( + rows.len(), + boundary_values.len(), + "every boundary value must produce a row" + ); + + for (val, sql_result) in &rows { + let rust_result = super::normalize_owner_key(val); + assert_eq!( + sql_result, rust_result, + "ASSIGNEE_DID_CASE_SQL(\"{val}\") mismatch: Rust = \"{rust_result}\", SQL CASE = \"{sql_result}\"" + ); + } + } } /// Exercises the iCaptcha single-use proof ledger (`icaptcha_consumed_proofs`),