From 5c5de1561a7defd25f4df21a847b784ab3d7a594 Mon Sep 17 00:00:00 2001 From: FrozenRaspberry <3733873+FrozenRaspberry@users.noreply.github.com> Date: Sat, 20 Jun 2026 12:19:58 +0800 Subject: [PATCH 1/2] feat(node): owner-only push enforcement behind GITLAWB_ENFORCE_OWNER_PUSH (#31) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A valid did:key signature on git-receive-pack is authentication, not authorization: any party can generate a key, derive its DID, sign, and push to an unprotected branch. Owner checks only ran on protected branches, so a non-owner could write to unprotected branches of an arbitrary repo. Add an opt-in flag GITLAWB_ENFORCE_OWNER_PUSH (default false, mirroring GITLAWB_REQUIRE_SIGNED_PEER_WRITES) that requires the authenticated pusher to be the repo owner on every branch. Default-off keeps live nodes unaffected. Design follows the maintainer's review on #31: - Authorize on the canonical AuthenticatedDid injected by require_signature, via Extension on the handler — not a re-parse of the headers. The legacy branch-protection check is left as-is. - Fail closed: an absent identity or a non-owner caller is rejected. The policy lives in a pure fn (owner_push_rejection) so it is unit-tested without a DB. - The gate runs before branch protection, so one rejection yields one error body. - Factor the duplicated owner-match into auth::is_repo_owner, reused by receive-pack, protect.rs, and visibility.rs (each keeps its own message). events.rs / peers.rs are left out — they aren't owner checks. - Name the push gate by intent (caller_authorized_to_push) so the planned Phase 2 UCAN git/push capability is a pure addition, not a rewrite. Tests: owner (full + short DID) allowed, non-owner rejected, missing-DID rejected, flag-off legacy. Docs: .env.example + docs/RUN-A-NODE.md. Co-Authored-By: Claude Opus 4.8 --- .env.example | 6 ++ crates/gitlawb-node/src/api/protect.rs | 16 +--- crates/gitlawb-node/src/api/repos.rs | 111 +++++++++++++++++++++- crates/gitlawb-node/src/api/visibility.rs | 9 +- crates/gitlawb-node/src/auth/mod.rs | 25 +++++ crates/gitlawb-node/src/config.rs | 8 ++ docs/RUN-A-NODE.md | 24 +++++ 7 files changed, 178 insertions(+), 21 deletions(-) diff --git a/.env.example b/.env.example index b8765d74..cd5b5aa6 100644 --- a/.env.example +++ b/.env.example @@ -72,6 +72,12 @@ GITLAWB_BOOTSTRAP_DISABLE_SEEDS=false # rolling upgrades so existing live nodes can still communicate. GITLAWB_REQUIRE_SIGNED_PEER_WRITES=false +# Require the authenticated pusher to be the repo owner on git-receive-pack. +# A valid did:key signature is authentication, not authorization: anyone can +# sign as their own DID. When true, pushes from a non-owner DID are rejected. +# Keep false until the repo owner is ready for owner-only writes. +GITLAWB_ENFORCE_OWNER_PUSH=false + # Comma-separated libp2p multiaddrs. # Example: /ip4/1.2.3.4/udp/7546/quic-v1/p2p/12D3KooW... GITLAWB_P2P_BOOTSTRAP= diff --git a/crates/gitlawb-node/src/api/protect.rs b/crates/gitlawb-node/src/api/protect.rs index f0844a0f..435dc899 100644 --- a/crates/gitlawb-node/src/api/protect.rs +++ b/crates/gitlawb-node/src/api/protect.rs @@ -7,7 +7,7 @@ use axum::extract::{Extension, Path, State}; use axum::http::StatusCode; use axum::Json; -use crate::auth::AuthenticatedDid; +use crate::auth::{is_repo_owner, AuthenticatedDid}; use crate::error::{AppError, Result}; use crate::state::AppState; @@ -25,12 +25,7 @@ pub async fn protect_branch( // Only the repo owner can protect branches let caller = &auth.0; - let owner_short = record - .owner_did - .split(':') - .next_back() - .unwrap_or(&record.owner_did); - if caller != &record.owner_did && caller != owner_short { + if !is_repo_owner(&record, caller) { return Err(AppError::BadRequest( "only the repo owner can protect branches".into(), )); @@ -63,12 +58,7 @@ pub async fn unprotect_branch( .ok_or_else(|| AppError::RepoNotFound(format!("{owner}/{repo}")))?; let caller = &auth.0; - let owner_short = record - .owner_did - .split(':') - .next_back() - .unwrap_or(&record.owner_did); - if caller != &record.owner_did && caller != owner_short { + if !is_repo_owner(&record, caller) { return Err(AppError::BadRequest( "only the repo owner can unprotect branches".into(), )); diff --git a/crates/gitlawb-node/src/api/repos.rs b/crates/gitlawb-node/src/api/repos.rs index 2886926f..7adcf53d 100644 --- a/crates/gitlawb-node/src/api/repos.rs +++ b/crates/gitlawb-node/src/api/repos.rs @@ -5,7 +5,7 @@ use axum::Json; use bytes::Bytes; use std::sync::Arc; -use crate::auth::AuthenticatedDid; +use crate::auth::{caller_authorized_to_push, AuthenticatedDid}; use chrono::Utc; use serde::{Deserialize, Serialize}; use uuid::Uuid; @@ -461,10 +461,38 @@ pub async fn git_upload_pack( Ok(resp) } +/// Decide whether the owner-push gate rejects a `git-receive-pack` request. +/// +/// Returns `Some(error)` when the push must be rejected, `None` when it may +/// proceed. Pure function so the policy is unit-testable without a database or a +/// live git backend. +/// +/// Fails closed: when `enforce` is on, an absent identity (`None`) or a caller +/// that is not authorized to push is rejected. When `enforce` is off it always +/// allows, preserving the legacy (authentication-only) behavior. +fn owner_push_rejection( + enforce: bool, + record: &crate::db::RepoRecord, + caller: Option<&str>, +) -> Option { + if !enforce { + return None; + } + match caller { + Some(did) if caller_authorized_to_push(record, did) => None, + _ => Some(AppError::BadRequest( + "push rejected — only the repo owner may push to this repository \ + (GITLAWB_ENFORCE_OWNER_PUSH is enabled)" + .into(), + )), + } +} + /// POST /:owner/:repo.git/git-receive-pack (AUTH REQUIRED — enforced by middleware) pub async fn git_receive_pack( State(state): State, Path((owner, repo)): Path<(String, String)>, + Extension(auth): Extension, headers: HeaderMap, body: Bytes, ) -> Result { @@ -483,6 +511,26 @@ pub async fn git_receive_pack( "parsed ref updates from pack" ); + // ── Owner-only push enforcement (opt-in: GITLAWB_ENFORCE_OWNER_PUSH) ── + // Runs before branch protection on purpose: when enabled, a non-owner is + // rejected here regardless of whether the target branch is protected, so a + // single rejection never yields two different error bodies. The identity is + // the canonical DID injected by `require_signature`, not a re-parse of the + // request headers. Fails closed (see `owner_push_rejection`). + if let Some(err) = owner_push_rejection( + state.config.enforce_owner_push, + &record, + Some(auth.0.as_str()), + ) { + tracing::warn!( + repo = %name, + pusher = %auth.0, + owner_did = %record.owner_did, + "owner-push enforcement: rejecting push from non-owner" + ); + return Err(err); + } + // ── Branch protection check ────────────────────────────────────────── let pusher_did_for_check = extract_did_from_auth(&headers); tracing::debug!(pusher_did = ?pusher_did_for_check, "extracted pusher DID from auth headers"); @@ -1221,3 +1269,64 @@ fn to_response(record: &crate::db::RepoRecord, state: &AppState, star_count: i64 forked_from: record.forked_from.clone(), } } + +#[cfg(test)] +mod tests { + use super::owner_push_rejection; + + const OWNER_DID: &str = "did:key:z6MkpTHR8VNsBxYAAWHut2Geadd9jSwuBV8xRoAnwWsdvktH"; + const OWNER_SHORT: &str = "z6MkpTHR8VNsBxYAAWHut2Geadd9jSwuBV8xRoAnwWsdvktH"; + const STRANGER_DID: &str = "did:key:z6Mkffonly5tranger0000000000000000000000000000000"; + + fn repo_owned_by(owner_did: &str) -> crate::db::RepoRecord { + let now = chrono::Utc::now(); + crate::db::RepoRecord { + id: "repo-id".into(), + name: "demo".into(), + owner_did: owner_did.into(), + description: None, + is_public: true, + default_branch: "main".into(), + created_at: now, + updated_at: now, + disk_path: "/tmp/demo".into(), + forked_from: None, + machine_id: None, + } + } + + #[test] + fn enforced_allows_owner_full_did() { + let repo = repo_owned_by(OWNER_DID); + assert!(owner_push_rejection(true, &repo, Some(OWNER_DID)).is_none()); + } + + #[test] + fn enforced_allows_owner_short_did() { + // Owners are accepted in bare-multibase form, matching the rest of the + // codebase's owner comparisons. + let repo = repo_owned_by(OWNER_DID); + assert!(owner_push_rejection(true, &repo, Some(OWNER_SHORT)).is_none()); + } + + #[test] + fn enforced_rejects_non_owner() { + let repo = repo_owned_by(OWNER_DID); + assert!(owner_push_rejection(true, &repo, Some(STRANGER_DID)).is_some()); + } + + #[test] + fn enforced_rejects_missing_did() { + // Fail closed: an absent authenticated identity is rejected, not allowed. + let repo = repo_owned_by(OWNER_DID); + assert!(owner_push_rejection(true, &repo, None).is_some()); + } + + #[test] + fn disabled_allows_non_owner_and_missing_did() { + // Flag off → legacy behavior: authentication-only, no owner gate. + let repo = repo_owned_by(OWNER_DID); + assert!(owner_push_rejection(false, &repo, Some(STRANGER_DID)).is_none()); + assert!(owner_push_rejection(false, &repo, None).is_none()); + } +} diff --git a/crates/gitlawb-node/src/api/visibility.rs b/crates/gitlawb-node/src/api/visibility.rs index 6665b9e8..09f4e913 100644 --- a/crates/gitlawb-node/src/api/visibility.rs +++ b/crates/gitlawb-node/src/api/visibility.rs @@ -5,7 +5,7 @@ use axum::http::StatusCode; use axum::Json; use serde::Deserialize; -use crate::auth::AuthenticatedDid; +use crate::auth::{is_repo_owner, AuthenticatedDid}; use crate::db::VisibilityMode; use crate::error::{AppError, Result}; use crate::state::AppState; @@ -26,12 +26,7 @@ pub struct RemoveVisibilityRequest { } fn require_owner(record: &crate::db::RepoRecord, caller: &str) -> Result<()> { - let owner_short = record - .owner_did - .split(':') - .next_back() - .unwrap_or(&record.owner_did); - if caller != record.owner_did && caller != owner_short { + if !is_repo_owner(record, caller) { return Err(AppError::BadRequest( "only the repo owner can manage visibility".into(), )); diff --git a/crates/gitlawb-node/src/auth/mod.rs b/crates/gitlawb-node/src/auth/mod.rs index 8cbe9f93..7838fdf9 100644 --- a/crates/gitlawb-node/src/auth/mod.rs +++ b/crates/gitlawb-node/src/auth/mod.rs @@ -17,6 +17,31 @@ use crate::state::AppState; #[derive(Clone, Debug)] pub struct AuthenticatedDid(pub String); +/// Whether `caller` is the owner of `record`. +/// +/// Owners are compared in both forms the rest of the codebase accepts: the full +/// `did:key:z6Mk…` string and its bare multibase suffix (`z6Mk…`). This is the +/// single source of truth for the owner-match logic that previously lived inline +/// in the receive-pack, branch-protection, and visibility paths. +pub fn is_repo_owner(record: &crate::db::RepoRecord, caller: &str) -> bool { + let owner_short = record + .owner_did + .split(':') + .next_back() + .unwrap_or(&record.owner_did); + caller == record.owner_did || caller == owner_short +} + +/// Whether `caller` is authorized to push to `record`. +/// +/// Phase 1 (`GITLAWB_ENFORCE_OWNER_PUSH`): owner-only. This is intentionally a +/// distinct, intent-named gate rather than a bare owner check so that Phase 2 can +/// extend it to honor a verified UCAN `git/push` capability as a pure addition +/// (`is_repo_owner(..) || ucan_grants_push(..)`) without rewriting call sites. +pub fn caller_authorized_to_push(record: &crate::db::RepoRecord, caller: &str) -> bool { + is_repo_owner(record, caller) +} + use gitlawb_core::http_sig::{ build_signing_string, compute_content_digest, HttpSignature, COVERED_COMPONENTS, }; diff --git a/crates/gitlawb-node/src/config.rs b/crates/gitlawb-node/src/config.rs index 929471a0..3a9a507f 100644 --- a/crates/gitlawb-node/src/config.rs +++ b/crates/gitlawb-node/src/config.rs @@ -49,6 +49,14 @@ pub struct Config { )] pub require_signed_peer_writes: bool, + /// Require the authenticated pusher to be the repo owner on `git-receive-pack`. + /// Authentication (a valid did:key signature) is not authorization on its own: + /// any party can sign as their own DID. When true, pushes whose authenticated + /// DID is not the repo owner are rejected. Keep false during rolling upgrades; + /// flip it on once owners are ready for owner-only writes. + #[arg(long, env = "GITLAWB_ENFORCE_OWNER_PUSH", default_value_t = false)] + pub enforce_owner_push: bool, + /// URL of local IPFS/Kubo node HTTP API (e.g. http://127.0.0.1:5001) #[arg(long, env = "GITLAWB_IPFS_API", default_value = "")] pub ipfs_api: String, diff --git a/docs/RUN-A-NODE.md b/docs/RUN-A-NODE.md index 987919fe..b730d5fe 100644 --- a/docs/RUN-A-NODE.md +++ b/docs/RUN-A-NODE.md @@ -141,6 +141,30 @@ During the cooldown your node still earns rewards if it keeps heartbeating. --- +## Hardening: owner-only push + +By default the node authenticates every `git-receive-pack` push (a valid RFC 9421 +`did:key` signature) but does **not** check that the pusher owns the repo, except +on branches that are explicitly protected. Because `did:key` is self-certifying, +any party can generate a key, derive its DID, sign, and push to an unprotected +branch — authentication is not authorization. + +To require the authenticated pusher to be the repo owner on **every** branch, set: + +```bash +GITLAWB_ENFORCE_OWNER_PUSH=true +``` + +- **Default `false`** — preserves current behavior so live nodes are unaffected by + an upgrade. Turn it on once you're ready for owner-only writes. +- **When `true`** — a push whose authenticated DID is not the repo owner is + rejected before any ref update is applied. The owner is matched in both the full + `did:key:z6Mk…` form and its bare `z6Mk…` suffix. +- Collaborator and UCAN-delegated push rights are a separate, planned follow-up; + today this is strictly owner-only. + +--- + ## Operational checklist | Concern | Recommendation | From f9df6eb6542fabc9bee043f85ec8027c992eb5c6 Mon Sep 17 00:00:00 2001 From: FrozenRaspberry <3733873+FrozenRaspberry@users.noreply.github.com> Date: Mon, 22 Jun 2026 08:13:50 +0800 Subject: [PATCH 2/2] =?UTF-8?q?fix(node):=20address=20review=20on=20#68=20?= =?UTF-8?q?=E2=80=94=20403=20status,=20finish=20owner-match=20consolidatio?= =?UTF-8?q?n?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Per @beardthelion's review: - Return 403 Forbidden (not 400) for authorization denials: the owner-push gate, the branch-protection denial, and the owner checks in protect.rs / visibility.rs. 400 reads as a malformed request and some git/CI clients retry it. - Finish the is_repo_owner consolidation: route the branch-protection check and the repos list filter through the helper. The branch-protection check now uses the verified AuthenticatedDid (auth.0) instead of re-parsing the headers. - Drop the second identity source on the push path entirely: the post-push event recording also uses auth.0, so extract_did_from_auth (and the handler's HeaderMap param + import) are removed. - Tests assert the Forbidden variant (so a future status regression fails loudly) and add direct coverage for is_repo_owner / caller_authorized_to_push. - Docs: warn that enabling the flag blocks delegated and CI agents until the Phase 2 UCAN git/push capability is honored. Co-Authored-By: Claude Opus 4.8 --- crates/gitlawb-node/src/api/protect.rs | 4 +- crates/gitlawb-node/src/api/repos.rs | 116 ++++++++++------------ crates/gitlawb-node/src/api/visibility.rs | 2 +- docs/RUN-A-NODE.md | 12 ++- 4 files changed, 66 insertions(+), 68 deletions(-) diff --git a/crates/gitlawb-node/src/api/protect.rs b/crates/gitlawb-node/src/api/protect.rs index 435dc899..fdfa6fad 100644 --- a/crates/gitlawb-node/src/api/protect.rs +++ b/crates/gitlawb-node/src/api/protect.rs @@ -26,7 +26,7 @@ pub async fn protect_branch( // Only the repo owner can protect branches let caller = &auth.0; if !is_repo_owner(&record, caller) { - return Err(AppError::BadRequest( + return Err(AppError::Forbidden( "only the repo owner can protect branches".into(), )); } @@ -59,7 +59,7 @@ pub async fn unprotect_branch( let caller = &auth.0; if !is_repo_owner(&record, caller) { - return Err(AppError::BadRequest( + return Err(AppError::Forbidden( "only the repo owner can unprotect branches".into(), )); } diff --git a/crates/gitlawb-node/src/api/repos.rs b/crates/gitlawb-node/src/api/repos.rs index 7adcf53d..0c53f087 100644 --- a/crates/gitlawb-node/src/api/repos.rs +++ b/crates/gitlawb-node/src/api/repos.rs @@ -1,11 +1,11 @@ use axum::extract::{Extension, Path, Query, State}; -use axum::http::{HeaderMap, StatusCode}; +use axum::http::StatusCode; use axum::response::Response; use axum::Json; use bytes::Bytes; use std::sync::Arc; -use crate::auth::{caller_authorized_to_push, AuthenticatedDid}; +use crate::auth::{caller_authorized_to_push, is_repo_owner, AuthenticatedDid}; use chrono::Utc; use serde::{Deserialize, Serialize}; use uuid::Uuid; @@ -162,8 +162,7 @@ pub async fn list_repos( .iter() .filter(|(r, _)| { if let Some(owner) = &query.owner { - let short = r.owner_did.split(':').next_back().unwrap_or(&r.owner_did); - short == owner.as_str() || r.owner_did == owner.as_str() + is_repo_owner(r, owner.as_str()) } else { true } @@ -480,7 +479,7 @@ fn owner_push_rejection( } match caller { Some(did) if caller_authorized_to_push(record, did) => None, - _ => Some(AppError::BadRequest( + _ => Some(AppError::Forbidden( "push rejected — only the repo owner may push to this repository \ (GITLAWB_ENFORCE_OWNER_PUSH is enabled)" .into(), @@ -493,7 +492,6 @@ pub async fn git_receive_pack( State(state): State, Path((owner, repo)): Path<(String, String)>, Extension(auth): Extension, - headers: HeaderMap, body: Bytes, ) -> Result { let name = repo.trim_end_matches(".git"); @@ -532,8 +530,9 @@ pub async fn git_receive_pack( } // ── Branch protection check ────────────────────────────────────────── - let pusher_did_for_check = extract_did_from_auth(&headers); - tracing::debug!(pusher_did = ?pusher_did_for_check, "extracted pusher DID from auth headers"); + // Uses the same verified identity as the owner-push gate above. (When that + // gate is enabled a non-owner never reaches here; this still applies when it + // is off, gating only the branches an owner has explicitly protected.) for update in &ref_updates { // Strip refs/heads/ prefix to get plain branch name let branch = update @@ -545,27 +544,17 @@ pub async fn git_receive_pack( .is_branch_protected(&record.id, branch) .await .unwrap_or(false) + && !is_repo_owner(&record, &auth.0) { - let owner_short = record - .owner_did - .split(':') - .next_back() - .unwrap_or(&record.owner_did); - let is_owner = pusher_did_for_check - .as_deref() - .map(|did| did == record.owner_did || did == owner_short) - .unwrap_or(false); - if !is_owner { - tracing::warn!( - branch = %branch, - pusher = ?pusher_did_for_check, - owner_did = %record.owner_did, - "branch protection: rejecting push from non-owner" - ); - return Err(AppError::BadRequest(format!( - "branch '{branch}' is protected — only the repo owner can push to it" - ))); - } + tracing::warn!( + branch = %branch, + pusher = %auth.0, + owner_did = %record.owner_did, + "branch protection: rejecting push from non-owner" + ); + return Err(AppError::Forbidden(format!( + "branch '{branch}' is protected — only the repo owner can push to it" + ))); } } @@ -601,9 +590,11 @@ pub async fn git_receive_pack( crate::metrics::record_push(&record.id); crate::metrics::observe_pack_size(body_len as f64); - // Record push event for trust score and issue a signed ref certificate - let pusher_did = extract_did_from_auth(&headers); - if let Some(ref did) = pusher_did { + // Record push event for trust score and issue a signed ref certificate. + // The route is behind `require_signature`, so the verified pusher identity is + // always present; use it directly rather than re-parsing the headers. + let did = auth.0.as_str(); + { // Use the first new commit hash we parsed, fall back to timestamp let commit_hash = ref_updates .first() @@ -661,7 +652,7 @@ pub async fn git_receive_pack( "created": update.old_sha == "0000000000000000000000000000000000000000", "forced": false, "pusher": { - "did": pusher_did.as_deref().unwrap_or("unknown"), + "did": did, }, "repository": { "id": record.id, @@ -773,7 +764,7 @@ pub async fn git_receive_pack( .map(|u| (u.ref_name.clone(), u.new_sha.clone())) .collect::>(); let p2p_handle = state.p2p.clone(); - let pusher_did_clone = pusher_did.clone().unwrap_or_default(); + let pusher_did_clone = did.to_string(); let db_for_peers = state.db.clone(); let ref_update_tx = state.ref_update_tx.clone(); let irys_url = state.config.irys_url.clone(); @@ -1215,30 +1206,6 @@ fn parse_ref_updates(body: &[u8]) -> Vec { updates } -/// Extract the DID from RFC 9421 Signature-Input header (keyid="..."). -/// Falls back to draft-cavage Authorization header for old clients. -fn extract_did_from_auth(headers: &HeaderMap) -> Option { - // RFC 9421: Signature-Input: sig1=(...);keyid="did:key:z6Mk...";... - if let Some(sig_input) = headers.get("signature-input").and_then(|v| v.to_str().ok()) { - if let Some(start) = sig_input.find("keyid=\"") { - let rest = &sig_input[start + 7..]; - if let Some(end) = rest.find('"') { - return Some(rest[..end].to_string()); - } - } - } - // Fallback: draft-cavage Authorization: Signature keyId="..." - if let Some(auth) = headers.get("authorization").and_then(|v| v.to_str().ok()) { - if let Some(start) = auth.find("keyId=\"") { - let rest = &auth[start + 7..]; - if let Some(end) = rest.find('"') { - return Some(rest[..end].to_string()); - } - } - } - None -} - // ── Helpers ─────────────────────────────────────────────────────────────── fn to_response(record: &crate::db::RepoRecord, state: &AppState, star_count: i64) -> RepoResponse { @@ -1273,6 +1240,8 @@ fn to_response(record: &crate::db::RepoRecord, state: &AppState, star_count: i64 #[cfg(test)] mod tests { use super::owner_push_rejection; + use crate::auth::{caller_authorized_to_push, is_repo_owner}; + use crate::error::AppError; const OWNER_DID: &str = "did:key:z6MkpTHR8VNsBxYAAWHut2Geadd9jSwuBV8xRoAnwWsdvktH"; const OWNER_SHORT: &str = "z6MkpTHR8VNsBxYAAWHut2Geadd9jSwuBV8xRoAnwWsdvktH"; @@ -1295,6 +1264,15 @@ mod tests { } } + /// A rejection must be a 403 Forbidden (authenticated but not authorized), + /// not a 400 — some git/CI clients retry 400s. + fn assert_forbidden(rejection: Option) { + assert!( + matches!(rejection, Some(AppError::Forbidden(_))), + "expected Some(Forbidden), got {rejection:?}" + ); + } + #[test] fn enforced_allows_owner_full_did() { let repo = repo_owned_by(OWNER_DID); @@ -1310,16 +1288,16 @@ mod tests { } #[test] - fn enforced_rejects_non_owner() { + fn enforced_rejects_non_owner_with_forbidden() { let repo = repo_owned_by(OWNER_DID); - assert!(owner_push_rejection(true, &repo, Some(STRANGER_DID)).is_some()); + assert_forbidden(owner_push_rejection(true, &repo, Some(STRANGER_DID))); } #[test] - fn enforced_rejects_missing_did() { + fn enforced_rejects_missing_did_with_forbidden() { // Fail closed: an absent authenticated identity is rejected, not allowed. let repo = repo_owned_by(OWNER_DID); - assert!(owner_push_rejection(true, &repo, None).is_some()); + assert_forbidden(owner_push_rejection(true, &repo, None)); } #[test] @@ -1329,4 +1307,20 @@ mod tests { assert!(owner_push_rejection(false, &repo, Some(STRANGER_DID)).is_none()); assert!(owner_push_rejection(false, &repo, None).is_none()); } + + #[test] + fn is_repo_owner_matches_full_and_short_did_only() { + let repo = repo_owned_by(OWNER_DID); + assert!(is_repo_owner(&repo, OWNER_DID)); + assert!(is_repo_owner(&repo, OWNER_SHORT)); + assert!(!is_repo_owner(&repo, STRANGER_DID)); + } + + #[test] + fn caller_authorized_to_push_is_owner_only_in_phase_1() { + let repo = repo_owned_by(OWNER_DID); + assert!(caller_authorized_to_push(&repo, OWNER_DID)); + assert!(caller_authorized_to_push(&repo, OWNER_SHORT)); + assert!(!caller_authorized_to_push(&repo, STRANGER_DID)); + } } diff --git a/crates/gitlawb-node/src/api/visibility.rs b/crates/gitlawb-node/src/api/visibility.rs index 09f4e913..94424669 100644 --- a/crates/gitlawb-node/src/api/visibility.rs +++ b/crates/gitlawb-node/src/api/visibility.rs @@ -27,7 +27,7 @@ pub struct RemoveVisibilityRequest { fn require_owner(record: &crate::db::RepoRecord, caller: &str) -> Result<()> { if !is_repo_owner(record, caller) { - return Err(AppError::BadRequest( + return Err(AppError::Forbidden( "only the repo owner can manage visibility".into(), )); } diff --git a/docs/RUN-A-NODE.md b/docs/RUN-A-NODE.md index b730d5fe..5ec5357e 100644 --- a/docs/RUN-A-NODE.md +++ b/docs/RUN-A-NODE.md @@ -158,10 +158,14 @@ GITLAWB_ENFORCE_OWNER_PUSH=true - **Default `false`** — preserves current behavior so live nodes are unaffected by an upgrade. Turn it on once you're ready for owner-only writes. - **When `true`** — a push whose authenticated DID is not the repo owner is - rejected before any ref update is applied. The owner is matched in both the full - `did:key:z6Mk…` form and its bare `z6Mk…` suffix. -- Collaborator and UCAN-delegated push rights are a separate, planned follow-up; - today this is strictly owner-only. + rejected (HTTP 403) before any ref update is applied. The owner is matched in + both the full `did:key:z6Mk…` form and its bare `z6Mk…` suffix. +- **Caution: this blocks every non-owner pusher, including your own delegated and + CI agents.** Push authorization is owner-only today — a UCAN `git/push` + capability is verified but not yet honored for authorization, so delegated keys + cannot push while this is on. Don't enable it until every identity that pushes + to your repos is the owner, or you'll lock out your own automation. Scoped + collaborator / UCAN-delegated push rights are a planned follow-up. ---