Skip to content
Merged
22 changes: 18 additions & 4 deletions crates/gitlawb-node/src/api/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -80,15 +80,20 @@ pub(crate) fn did_matches(a: &str, b: &str) -> bool {
!ka.contains(':') && !kb.contains(':') && ka == kb
}

/// Client-facing denial shared by every repo-owner gate copy. `require_repo_owner`
/// renders it as `AppError::Forbidden`; a handler that emits its own
/// `(StatusCode, Json)` envelope instead of `AppError` (e.g.
/// `api::tasks::create_task`) must use this constant too, so two copies of one
/// gate cannot drift in observable output.
pub(crate) const REPO_OWNER_ONLY_MESSAGE: &str = "only the repo owner can perform this action";

/// 403 unless `caller` is the repo owner. Uses [`did_matches`] so the owner check
/// and the author check (close policy) share one normalization.
pub(crate) fn require_repo_owner(record: &RepoRecord, caller: &str) -> Result<()> {
if did_matches(caller, &record.owner_did) {
Ok(())
} else {
Err(AppError::Forbidden(
"only the repo owner can perform this action".into(),
))
Err(AppError::Forbidden(REPO_OWNER_ONLY_MESSAGE.into()))
}
}

Expand Down Expand Up @@ -242,6 +247,11 @@ mod authz_guard {
(events, "list_repo_events", "authorize_repo_read("),
// Bucket C — signer-self: the acting DID is matched/bound to auth.0
(tasks, "create_task", "did_matches("),
// #496: create_task is signer-self AND owner-when-repo-scoped — a
// repo_id naming a hosted repo admits tasks only from its owner.
// Both halves are pinned: the did_matches row guards the signer
// binding, this one the repo-ownership gate.
(tasks, "create_task", "require_repo_owner("),
(tasks, "claim_task", "did_matches("),
(tasks, "complete_task", "did_matches("),
(tasks, "fail_task", "did_matches("),
Expand Down Expand Up @@ -341,7 +351,11 @@ mod authz_guard {
(
"create_task",
"createTask",
&["require_signer(ctx)?", "did_matches("],
&[
"require_signer(ctx)?",
"did_matches(",
"require_repo_owner(",
],
),
(
"claim_task",
Expand Down
98 changes: 92 additions & 6 deletions crates/gitlawb-node/src/api/tasks.rs
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,33 @@ fn forbidden(msg: &str) -> (StatusCode, Json<Value>) {
)
}

/// This module's error shape with an opaque body: the real error is logged
/// server-side and never serialized (#250), matching the envelope
/// `AppError::Db` renders. Also mirrors its status split: pool-class failures
/// are a retryable 503 `db_unavailable`, everything else the opaque 500
/// `db_error`, so a transient outage is not reported as permanent.
fn db_error(e: anyhow::Error) -> (StatusCode, Json<Value>) {
tracing::error!(error = %format!("{e:#}"), "task database error");
if let Some(sql) = e.chain().find_map(|c| c.downcast_ref::<sqlx::Error>()) {
if crate::error::db_unavailable(sql) {
return (
StatusCode::SERVICE_UNAVAILABLE,
Json(json!({
"error": crate::error::DB_UNAVAILABLE_CODE,
"message": crate::error::DB_UNAVAILABLE_MESSAGE,
})),
);
}
}
(
StatusCode::INTERNAL_SERVER_ERROR,
Json(json!({
"error": "db_error",
"message": crate::error::DB_ERROR_MESSAGE,
})),
)
}

// ── Request / response types ──────────────────────────────────────────────────

#[derive(Deserialize)]
Expand Down Expand Up @@ -101,6 +128,37 @@ pub async fn create_task(
if !crate::api::did_matches(&auth.0, &body.delegator_did) {
return Err(forbidden("delegator_did must be the authenticated signer"));
}
// #496: a caller-supplied repo_id must name a repo the caller owns. Without
// this, any signed caller can plant a task — payload and UCAN included —
// under a foreign repo id, where the named assignee can claim it. Resolve
// the id against hosted repos: a hosted, non-quarantined repo admits tasks
// only from its owner (403 otherwise). An id naming no hosted repo
// (unknown, or quarantined — which is hidden as if it did not exist, so
// 403ing it would confirm a real id) is stored verbatim as an opaque
// label: this is a write gate and does not bind an unresolved id to a
// repo. Task reads are caller-independent on this base (the list endpoints
// are not repo-scoped), before and after this change. The 403-vs-201 split
// between a hosted repo id and an unknown one is itself a one-bit oracle —
// a stranger holding a private repo's id learns it is hosted here — but
// denying foreign repos requires drawing exactly that line, and repo ids
// already surface via the task list on this base. Repo-less tasks are
// unaffected.
if let Some(repo_id) = body.repo_id.as_deref() {
let record = state.db.get_repo_by_id(repo_id).await.map_err(db_error)?;
if let Some(record) = record {
let quarantined = state
.db
.is_repo_quarantined(&record.id)
.await
.map_err(db_error)?;
if !quarantined && crate::api::require_repo_owner(&record, &auth.0).is_err() {
// Same denial text the GraphQL copy relays from
// `require_repo_owner`, so the two copies of this gate do not
// disagree in observable output.
return Err(forbidden(crate::api::REPO_OWNER_ONLY_MESSAGE));
}
}
}
let now = Utc::now().to_rfc3339();
let task = AgentTask {
id: Uuid::new_v4().to_string(),
Expand All @@ -117,12 +175,7 @@ pub async fn create_task(
updated_at: now,
deadline: body.deadline,
};
state.db.create_task(&task).await.map_err(|e| {
(
StatusCode::INTERNAL_SERVER_ERROR,
Json(json!({ "error": e.to_string() })),
)
})?;
state.db.create_task(&task).await.map_err(db_error)?;
Ok((StatusCode::CREATED, Json(task_to_json(&task))))
}

Expand Down Expand Up @@ -297,3 +350,36 @@ pub async fn fail_task(
});
Ok(Json(task_to_json(&task)))
}

#[cfg(test)]
mod tests {
use super::*;

/// The gate's DB helper must mirror `AppError::Db`: a pool-level failure is
/// the retryable 503 `db_unavailable`, a query fault the opaque 500.
#[test]
fn db_error_matches_app_error_status_split() {
let unavailable = db_error(anyhow::Error::from(sqlx::Error::PoolTimedOut));
assert_eq!(unavailable.0, StatusCode::SERVICE_UNAVAILABLE);
assert_eq!(unavailable.1 .0["error"], crate::error::DB_UNAVAILABLE_CODE);
assert_eq!(
unavailable.1 .0["message"],
crate::error::DB_UNAVAILABLE_MESSAGE
);

let query = db_error(anyhow::Error::from(sqlx::Error::Protocol(
"column \"machine_id\" does not exist".into(),
)));
assert_eq!(query.0, StatusCode::INTERNAL_SERVER_ERROR);
assert_eq!(query.1 .0["error"], "db_error");
assert_eq!(query.1 .0["message"], crate::error::DB_ERROR_MESSAGE);
assert!(!query.1 .0.to_string().contains("machine_id"));
}

/// `.context()` layers must not hide the sqlx error from the status split.
#[test]
fn db_error_sees_sqlx_through_context() {
let wrapped = anyhow::Error::from(sqlx::Error::PoolClosed).context("repo lookup failed");
assert_eq!(db_error(wrapped).0, StatusCode::SERVICE_UNAVAILABLE);
}
}
2 changes: 1 addition & 1 deletion crates/gitlawb-node/src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ pub const DB_ERROR_MESSAGE: &str = "a database error occurred";

/// Connection-level sqlx failures that mean the database is unreachable right
/// now (retryable, 503), as opposed to server-reported query errors.
fn db_unavailable(e: &sqlx::Error) -> bool {
pub(crate) fn db_unavailable(e: &sqlx::Error) -> bool {
matches!(
e,
sqlx::Error::PoolTimedOut
Expand Down
Loading
Loading