From 2a8d64d05748212a29b73be2bae3e6c8d04b89bd Mon Sep 17 00:00:00 2001 From: Vasanthdev2004 Date: Fri, 14 Aug 2026 18:55:29 +0530 Subject: [PATCH 01/18] feat(core)!: verify_chain returns the proof chain's root issuer MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit verify_chain returned Result<()>, so a caller learned that a chain was internally consistent and nothing else. `did:key` is self-certifying: anyone can mint a keypair, self-issue `*` on `*`, and produce a token that verifies. That is correct UCAN behaviour rather than a defect — a token with no proofs is its own root — and it still verifies after this change. But it does mean "the chain verifies" is not an authorization answer on its own, and without the root identity no caller could ask the question that actually decides a push: does this chain rest on someone this repository trusts? Returning the root means a caller cannot accept a chain without being handed the identity it rests on. Callers that legitimately do not care — the node middleware checking that a bootstrap token is well-formed, for one — discard it explicitly. The doc comment now states outright that this establishes internal consistency and not trust, and names what a caller has to compare the root against. Multi-proof chains are refused: more than one proof means more than one root, and nothing says which root authorized a given capability, so returning any single one would be unsound. The old loop followed every proof but returned nothing, so a two-proof chain verified silently; the new test pinned that by observing the pre-fix code accept one. Ucan::delegate only ever writes one proof, so no token this codebase produces is affected. BREAKING CHANGE: Ucan::verify_chain returns Result instead of Result<()>. --- crates/gitlawb-core/src/ucan.rs | 144 ++++++++++++++++++++++++++------ 1 file changed, 120 insertions(+), 24 deletions(-) diff --git a/crates/gitlawb-core/src/ucan.rs b/crates/gitlawb-core/src/ucan.rs index 86b3dd9f..38f15b0c 100644 --- a/crates/gitlawb-core/src/ucan.rs +++ b/crates/gitlawb-core/src/ucan.rs @@ -248,8 +248,16 @@ impl Ucan { /// 3. Check the proof is not expired /// 4. Recursively verify the proof's own chain /// - /// A UCAN with no proofs (root capability) passes trivially. - pub fn verify_chain(&self) -> Result<()> { + /// A UCAN with no proofs is its own root, so it returns its own issuer. + /// + /// **This establishes internal consistency, not trust.** `did:key` is + /// self-certifying, so anyone can mint a keypair and produce a chain that + /// verifies. A caller making an authorization decision MUST compare the + /// returned root against an identity it trusts for some reason outside this + /// token — a repo owner, a configured value, a registry lookup. Discarding + /// the return value is only correct when the caller is checking that a token + /// is well-formed and deliberately does not care who issued it. + pub fn verify_chain(&self) -> Result { // First verify our own signature self.verify_signature()?; @@ -261,34 +269,44 @@ impl Ucan { return Err(Error::Ucan("token is not yet valid".to_string())); } - for proof_token in &self.payload.prf { - let proof = Self::decode(proof_token) - .map_err(|e| Error::Ucan(format!("failed to decode proof: {e}")))?; + if self.payload.prf.len() > 1 { + return Err(Error::Ucan( + "multi-proof chains are not supported: more than one proof means \ + more than one root, and which root authorized a given capability \ + is ambiguous" + .to_string(), + )); + } + + let Some(proof_token) = self.payload.prf.first() else { + // No proofs: this token is its own root. + return Ok(self.payload.iss.clone()); + }; + + let proof = Self::decode(proof_token) + .map_err(|e| Error::Ucan(format!("failed to decode proof: {e}")))?; + + // The proof's audience must be this UCAN's issuer + if proof.payload.aud != self.payload.iss { + return Err(Error::Ucan(format!( + "proof chain broken: proof audience {} does not match issuer {}", + proof.payload.aud, self.payload.iss + ))); + } - // The proof's audience must be this UCAN's issuer - if proof.payload.aud != self.payload.iss { + // Every delegated capability must be covered by the proof (attenuation). + for cap in &self.payload.att { + let covered = proof.payload.att.iter().any(|p| cap.is_attenuated_by(p)); + if !covered { return Err(Error::Ucan(format!( - "proof chain broken: proof audience {} does not match issuer {}", - proof.payload.aud, self.payload.iss + "capability attenuation violated: '{}' on '{}' not covered by proof", + cap.can, cap.with ))); } - - // Every delegated capability must be covered by the proof (attenuation). - for cap in &self.payload.att { - let covered = proof.payload.att.iter().any(|p| cap.is_attenuated_by(p)); - if !covered { - return Err(Error::Ucan(format!( - "capability attenuation violated: '{}' on '{}' not covered by proof", - cap.can, cap.with - ))); - } - } - - // Verify the proof's signature and chain recursively - proof.verify_chain()?; } - Ok(()) + // Recurse; the root of the proof's chain is the root of ours. + proof.verify_chain() } } @@ -664,4 +682,82 @@ mod tests { delegated.verify_chain().unwrap(); } + + #[test] + fn verify_chain_returns_the_root_issuer_of_a_delegated_chain() { + // owner -> agent (delegation), agent -> node (invocation). + // The root is the owner: that is the identity the whole chain rests on, + // and the only one a caller can meaningfully anchor a trust decision to. + let owner = Keypair::generate(); + let agent = Keypair::generate(); + let node = Keypair::generate(); + let caps_vec = vec![Capability::new("gitlawb://repos/zowner/r", caps::GIT_PUSH)]; + + let delegation = + Ucan::issue(&owner, agent.did(), caps_vec.clone(), None).expect("issue delegation"); + let invocation = Ucan::delegate(&agent, node.did(), caps_vec, None, &delegation) + .expect("wrap invocation"); + + assert_eq!( + invocation.verify_chain().expect("chain must verify"), + owner.did(), + "the root issuer is the owner who started the chain, not the agent presenting it" + ); + } + + #[test] + fn verify_chain_returns_self_as_root_for_a_self_issued_token() { + // A token with no proofs roots at its own issuer. This is what makes a + // self-minted token useless: the caller compares this against the repo + // owner and it will only ever match when the presenter IS the owner. + let agent = Keypair::generate(); + let node = Keypair::generate(); + let ucan = + Ucan::issue(&agent, node.did(), vec![Capability::new("*", "*")], None).expect("issue"); + + assert_eq!( + ucan.verify_chain().expect("a root token still verifies"), + agent.did(), + "a self-minted token roots at the minter, however permissive its capabilities" + ); + } + + #[test] + fn verify_chain_rejects_a_multi_proof_chain() { + // Two proofs mean two roots, and nothing says which root authorized a + // given capability. Returning either one would be unsound, so refuse. + let owner_a = Keypair::generate(); + let owner_b = Keypair::generate(); + let agent = Keypair::generate(); + let node = Keypair::generate(); + let caps_vec = vec![Capability::new("gitlawb://repos/zowner/r", caps::GIT_PUSH)]; + + let proof_a = Ucan::issue(&owner_a, agent.did(), caps_vec.clone(), None).expect("issue a"); + let proof_b = Ucan::issue(&owner_b, agent.did(), caps_vec.clone(), None).expect("issue b"); + + // `delegate` only ever writes one proof, so build the two-proof payload by hand. + let payload = UcanPayload { + ucan: "1.0.0".to_string(), + iss: agent.did(), + aud: node.did(), + att: caps_vec, + exp: None, + nbf: None, + prf: vec![ + proof_a.encode().expect("encode a"), + proof_b.encode().expect("encode b"), + ], + }; + let signing_bytes = serde_json::to_vec(&payload).expect("serialize payload"); + let s = agent.sign_b64(&signing_bytes); + let multi = Ucan { payload, s }; + + let err = multi + .verify_chain() + .expect_err("a two-proof chain must be refused"); + assert!( + err.to_string().contains("multi-proof"), + "the error must name the reason, got: {err}" + ); + } } From 13a24240a96e3371f7090dfd13d13bdd9b3544aa Mon Sep 17 00:00:00 2001 From: Vasanthdev2004 Date: Fri, 14 Aug 2026 17:16:13 +0530 Subject: [PATCH 02/18] fix(node): gate two Unix-only tests so the crate compiles on Windows Two tests build a fake git as a `/bin/sh` script, mark it executable through `std::os::unix::fs::PermissionsExt`, and reap the hung `rev-list` with `libc::kill(SIGKILL)`. None of that exists on Windows, and neither test was cfg-gated, so `cargo test -p gitlawb-node` failed to compile there with six errors before running anything. A Windows checkout could not run a single test in the crate, including the ones that are platform-independent. Gate both with `#[cfg(unix)]`. The attribute is a no-op on Linux, so CI keeps running them exactly as before; only the Windows build changes, from "does not compile" to "runs the platform-independent tests". Refs #228 --- crates/gitlawb-node/src/api/repos.rs | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/crates/gitlawb-node/src/api/repos.rs b/crates/gitlawb-node/src/api/repos.rs index b09cb6da..e8f17497 100644 --- a/crates/gitlawb-node/src/api/repos.rs +++ b/crates/gitlawb-node/src/api/repos.rs @@ -4993,6 +4993,13 @@ mod tests { /// task the global slot stays occupied until the walk finishes; on the pre-fix code /// the handler-local permits drop on future-drop and the slot frees instantly (RED), /// letting disconnect-spam exceed the cap while real git work keeps running. + /// + /// Unix-only: the fake git is a `/bin/sh` script made executable through + /// `PermissionsExt::set_mode`, and the hung `rev-list` is reaped with + /// `libc::kill(SIGKILL)`. Neither exists on Windows, so without this gate the + /// whole `gitlawb-node` test target fails to compile there (#228) and no test + /// in the crate can run on a Windows checkout. + #[cfg(unix)] #[sqlx::test] async fn upload_pack_permit_held_through_walk_after_disconnect(pool: sqlx::PgPool) { use axum::body::Body; @@ -5860,6 +5867,11 @@ mod tests { /// sheds — releasing the permit lets the SAME walk run and pin (durability stays /// fail-closed). Exercises the gating seam directly; the detached push task calls /// this exact helper. + /// + /// Unix-only for the same reason as + /// `upload_pack_permit_held_through_walk_after_disconnect`: the fake git is a + /// `/bin/sh` script made executable through `PermissionsExt::set_mode` (#228). + #[cfg(unix)] #[tokio::test] async fn encrypt_walk_defers_when_pool_exhausted() { use std::sync::Arc; From 1b1c9726540e2a287b65f159dedaf6bd7678df34 Mon Sep 17 00:00:00 2001 From: Vasanthdev2004 Date: Fri, 14 Aug 2026 19:03:27 +0530 Subject: [PATCH 03/18] feat(node): carry the verified UCAN and its root into the request MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit require_ucan_chain validated a presented token and discarded it, so no handler could read the result and Ucan::can had no call site anywhere in the node. A UCAN could only ever fail a request, never authorize one. The middleware's own behaviour is unchanged — an absent header passes through, an invalid chain is still 401. It now also parks the verified token and the root issuer its chain rests on in request extensions, so an authorization decision downstream can use them. The root is stored rather than recomputed so the chain is walked once per request. Holding a VerifiedUcan is deliberately not an authorization decision. did:key is self-certifying, so a chain that verifies proves only internal consistency; the caller has to compare the root against an identity it trusts for the resource being touched. The next commit adds that comparison for git/push. --- crates/gitlawb-node/src/auth/mod.rs | 66 ++++++++++++++++++++++++++--- 1 file changed, 59 insertions(+), 7 deletions(-) diff --git a/crates/gitlawb-node/src/auth/mod.rs b/crates/gitlawb-node/src/auth/mod.rs index eee8fd8b..956e9b65 100644 --- a/crates/gitlawb-node/src/auth/mod.rs +++ b/crates/gitlawb-node/src/auth/mod.rs @@ -17,6 +17,20 @@ use crate::state::AppState; #[derive(Clone, Debug)] pub struct AuthenticatedDid(pub String); +/// A UCAN that passed full chain validation, with the root issuer the chain +/// rests on. Inserted into request extensions by [`require_ucan_chain`] when +/// `X-Ucan` is present; absent when the header is. +/// +/// `root` is carried rather than recomputed so the chain is walked once per +/// request. Holding this is not itself an authorization decision — a caller must +/// still compare `root` against an identity it independently trusts, because +/// `did:key` is self-certifying and anyone can mint a chain that verifies. +#[derive(Clone, Debug)] +pub struct VerifiedUcan { + pub ucan: Ucan, + pub root: Did, +} + /// Whether `caller` is authorized to push to `record`. /// /// Phase 1 (`GITLAWB_ENFORCE_OWNER_PUSH`): owner-only, via the canonical @@ -270,7 +284,7 @@ fn validate_ucan_chain( token: &str, expected_aud: &Did, signer_did: &Did, -) -> Result<(), (StatusCode, Json)> { +) -> Result)> { let ucan = Ucan::decode(token).map_err(|e| { ( StatusCode::UNAUTHORIZED, @@ -298,14 +312,14 @@ fn validate_ucan_chain( ) })?; - ucan.verify_chain().map_err(|e| { + let root = ucan.verify_chain().map_err(|e| { ( StatusCode::UNAUTHORIZED, Json(json!({ "error": "invalid_ucan", "message": e.to_string() })), ) })?; - Ok(()) + Ok(VerifiedUcan { ucan, root }) } /// Axum middleware that validates a UCAN chain when `X-Ucan` is present. @@ -358,11 +372,18 @@ pub async fn require_ucan_chain( } }; - if let Err((status, body)) = validate_ucan_chain(&token, &state.node_did, &signer_did) { - return (status, body).into_response(); - } + let verified = match validate_ucan_chain(&token, &state.node_did, &signer_did) { + Ok(v) => v, + Err((status, body)) => return (status, body).into_response(), + }; - tracing::debug!(did = %signer_did, "UCAN chain validated"); + tracing::debug!(did = %signer_did, root = %verified.root, "UCAN chain validated"); + + // Park the verified token where a handler can reach it. Validation alone + // grants nothing; the authorization decision is made downstream, by a caller + // that knows which identity it trusts for the resource being touched. + let mut request = request; + request.extensions_mut().insert(verified); next.run(request).await } @@ -398,6 +419,37 @@ mod tests { Ucan::bootstrap(node, agent_did).unwrap() } + /// The middleware validated a token and threw the result away, so no handler + /// could ever read it and `Ucan::can` had no call site in the node. Validation + /// must hand back both the token and the root the chain rests on. + #[test] + fn validate_ucan_chain_hands_back_the_root_and_the_token() { + let owner = Keypair::generate(); + let agent = Keypair::generate(); + let node = Keypair::generate(); + let caps_vec = vec![Capability::new("gitlawb://repos/zowner/r", caps::GIT_PUSH)]; + + let delegation = + Ucan::issue(&owner, agent.did(), caps_vec.clone(), None).expect("issue delegation"); + let invocation = Ucan::delegate(&agent, node.did(), caps_vec, None, &delegation) + .expect("wrap invocation"); + let token = invocation.encode().expect("encode"); + + let verified = validate_ucan_chain(&token, &node.did(), &agent.did()) + .expect("a well-formed owner-rooted invocation must validate"); + + assert_eq!( + verified.root, + owner.did(), + "the root must be the owner, so a caller can anchor against the repo record" + ); + assert_eq!( + verified.ucan.payload.iss, + agent.did(), + "the token itself must come back so a caller can read its capabilities" + ); + } + fn delegation_ucan(agent: &Keypair, node_did: Did, proof: &Ucan) -> Ucan { Ucan::delegate( agent, From f8b9cf11ea5ff4ff1062e5663e55170a72a628fe Mon Sep 17 00:00:00 2001 From: Vasanthdev2004 Date: Fri, 14 Aug 2026 19:06:52 +0530 Subject: [PATCH 04/18] feat(node): add ucan_grants_push, anchored at the repo owner MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The predicate that decides whether a delegated capability authorizes a push. The chain must root at the repo owner — data the node holds independently of the token, which is what a self-minted chain cannot forge — and the leaf must carry git/push for this repo. Only the leaf is examined for the capability. verify_chain has already established that each leaf capability is attenuated by its proof, transitively to the root, so a surviving leaf capability is no broader than what the root granted; re-walking the chain would duplicate that guarantee. Resource matching is structural rather than a string compare because owner_did is stored full on canonical rows and bare on mirror rows; a literal match would deny valid delegations for every mirror, which is a defect that would have looked like a permissions bug rather than a parsing one. A capability carrying nb authorizes nothing while constraints stay uninterpreted, so an owner who writes a ref restriction never accidentally grants repo-wide push. Not yet wired into the push path — the next commit does that — so a non-test build still reports VerifiedUcan::ucan as unread. --- crates/gitlawb-node/src/auth/mod.rs | 193 ++++++++++++++++++++++++++++ 1 file changed, 193 insertions(+) diff --git a/crates/gitlawb-node/src/auth/mod.rs b/crates/gitlawb-node/src/auth/mod.rs index 956e9b65..703c2da0 100644 --- a/crates/gitlawb-node/src/auth/mod.rs +++ b/crates/gitlawb-node/src/auth/mod.rs @@ -43,6 +43,58 @@ pub fn caller_authorized_to_push(record: &crate::db::RepoRecord, caller: &str) - crate::api::did_matches(caller, &record.owner_did) } +/// Whether `with` names this repository. +/// +/// Structural, not a string compare: `owner_did` is stored as a full +/// `did:key:z6Mk…` on canonical rows and as a bare `z6Mk…` on mirror rows, so a +/// literal match would deny a valid delegation for every mirror. `"*"` keeps the +/// wildcard meaning [`gitlawb_core::ucan::Capability::is_attenuated_by`] gives it. +fn repo_capability_matches(with: &str, record: &crate::db::RepoRecord) -> bool { + if with == "*" { + return true; + } + let Some(rest) = with.strip_prefix("gitlawb://repos/") else { + return false; + }; + // The owner segment is a DID and may contain ':' but never '/', so the last + // separator splits owner from name. + let Some((owner_seg, name_seg)) = rest.rsplit_once('/') else { + return false; + }; + !owner_seg.is_empty() + && crate::api::did_matches(owner_seg, &record.owner_did) + && name_seg == record.name +} + +/// Whether a verified UCAN authorizes a push to `record`. +/// +/// Two conditions, both required: +/// 1. The chain roots at this repo's owner. This is the trust anchor — the +/// repo record is data the node holds independently of the token, so a +/// self-minted chain cannot satisfy it. +/// 2. Some capability in the leaf covers `git/push` on this repo. +/// +/// Only the leaf is examined for (2): [`gitlawb_core::ucan::Ucan::verify_chain`] +/// has already established that each leaf capability is attenuated by its proof, +/// transitively to the root, so a surviving leaf capability is no broader than +/// what the root granted. +/// +/// A capability carrying `nb` (constraints) authorizes nothing. Constraints are +/// not interpreted yet, and an owner who writes them means to restrict; granting +/// while ignoring them would be strictly more permissive than intended. +pub fn ucan_grants_push(record: &crate::db::RepoRecord, verified: &VerifiedUcan) -> bool { + if !crate::api::did_matches(&verified.root.to_string(), &record.owner_did) { + return false; + } + verified.ucan.payload.att.iter().any(|cap| { + cap.constraints.is_none() + && (cap.can == gitlawb_core::ucan::caps::GIT_PUSH + || cap.can == "*" + || cap.can == gitlawb_core::ucan::caps::REPO_ADMIN) + && repo_capability_matches(&cap.with, record) + }) +} + use gitlawb_core::http_sig::{ build_signing_string, compute_content_digest, HttpSignature, COVERED_COMPONENTS, }; @@ -679,3 +731,144 @@ mod tests { assert_eq!(body_json["error"], "invalid_ucan"); } } + +#[cfg(test)] +mod ucan_push_tests { + use super::*; + use gitlawb_core::identity::Keypair; + use gitlawb_core::ucan::{caps, Capability, Ucan}; + + const OWNER_KEY: &str = "z6MkOwnerAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA"; + + /// `RepoRecord` does not derive `Default`, and adding the derive to a + /// production DB type purely to serve a test is the wrong direction. + fn repo(owner_did: &str, name: &str) -> crate::db::RepoRecord { + crate::db::RepoRecord { + id: "repo-id".to_string(), + name: name.to_string(), + owner_did: owner_did.to_string(), + description: None, + is_public: true, + default_branch: "main".to_string(), + created_at: chrono::Utc::now(), + updated_at: chrono::Utc::now(), + disk_path: "/unused".to_string(), + forked_from: None, + machine_id: None, + } + } + + /// The token's own issuer and audience are irrelevant to this predicate: the + /// middleware has already bound `iss` to the request signer and `aud` to this + /// node. Only the capabilities and the chain's root matter here. + fn verified(root: &str, caps_vec: Vec) -> VerifiedUcan { + let agent = Keypair::generate(); + let node = Keypair::generate(); + let ucan = Ucan::issue(&agent, node.did(), caps_vec, None).expect("issue"); + VerifiedUcan { + ucan, + root: root.parse().expect("root DID must parse"), + } + } + + fn owner_full() -> String { + format!("did:key:{OWNER_KEY}") + } + + fn push_cap_for(owner: &str, name: &str) -> Capability { + Capability::new(format!("gitlawb://repos/{owner}/{name}"), caps::GIT_PUSH) + } + + #[test] + fn grants_push_when_the_chain_roots_at_the_owner_and_names_the_repo() { + let rec = repo(&owner_full(), "myrepo"); + let v = verified(&owner_full(), vec![push_cap_for(&owner_full(), "myrepo")]); + assert!(ucan_grants_push(&rec, &v)); + } + + #[test] + fn matches_a_bare_owner_key_against_a_full_did_record() { + // Mirror rows store the bare key. A literal string compare would fail + // here, denying a delegation that is in fact valid. + let rec = repo(OWNER_KEY, "myrepo"); + let v = verified(&owner_full(), vec![push_cap_for(&owner_full(), "myrepo")]); + assert!(ucan_grants_push(&rec, &v)); + } + + #[test] + fn refuses_a_self_minted_root() { + // The whole point: a token nobody delegated grants nothing, however + // permissive its capabilities look. + let stranger = Keypair::generate(); + let rec = repo(&owner_full(), "myrepo"); + let v = verified(&stranger.did().to_string(), vec![Capability::new("*", "*")]); + assert!(!ucan_grants_push(&rec, &v)); + } + + #[test] + fn refuses_a_capability_for_a_different_repo() { + let rec = repo(&owner_full(), "myrepo"); + let v = verified( + &owner_full(), + vec![push_cap_for(&owner_full(), "otherrepo")], + ); + assert!(!ucan_grants_push(&rec, &v)); + } + + #[test] + fn refuses_a_capability_carrying_constraints() { + // `nb` is not interpreted yet. An owner who writes {"refs": [...]} means + // to restrict; honouring the capability while ignoring nb would grant + // strictly more than they intended, so it authorizes nothing. + let rec = repo(&owner_full(), "myrepo"); + let v = verified( + &owner_full(), + vec![push_cap_for(&owner_full(), "myrepo") + .with_constraints(serde_json::json!({ "refs": ["refs/heads/feat/*"] }))], + ); + assert!(!ucan_grants_push(&rec, &v)); + } + + #[test] + fn refuses_a_non_push_capability() { + let rec = repo(&owner_full(), "myrepo"); + let v = verified( + &owner_full(), + vec![Capability::new( + format!("gitlawb://repos/{}/myrepo", owner_full()), + caps::ISSUE_CREATE, + )], + ); + assert!(!ucan_grants_push(&rec, &v)); + } + + #[test] + fn honours_the_resource_wildcard_and_repo_admin() { + let rec = repo(&owner_full(), "myrepo"); + let wildcard = verified(&owner_full(), vec![Capability::new("*", caps::GIT_PUSH)]); + assert!(ucan_grants_push(&rec, &wildcard)); + + let admin = verified( + &owner_full(), + vec![Capability::new( + format!("gitlawb://repos/{}/myrepo", owner_full()), + caps::REPO_ADMIN, + )], + ); + assert!(ucan_grants_push(&rec, &admin)); + } + + #[test] + fn refuses_a_malformed_resource_uri() { + let rec = repo(&owner_full(), "myrepo"); + for bad in [ + "", + "myrepo", + "https://repos/x/myrepo", + "gitlawb://repos/myrepo", + ] { + let v = verified(&owner_full(), vec![Capability::new(bad, caps::GIT_PUSH)]); + assert!(!ucan_grants_push(&rec, &v), "{bad} must not grant push"); + } + } +} From d27f0bec1b9450a57390a4526415913d4a9a8880 Mon Sep 17 00:00:00 2001 From: Vasanthdev2004 Date: Fri, 14 Aug 2026 19:21:50 +0530 Subject: [PATCH 05/18] feat(node): honour a delegated git/push capability on the push path caller_authorized_to_push becomes owner || delegated, exactly the Phase 2 its own doc comment described. The owner check is unconditional and runs first, so the UCAN path can only ever turn a 403 into a 200, never the reverse. A push carrying no X-Ucan reaches the same owner-only decision it did before. git_receive_pack takes the verified token as Option>; axum extracts an absent extension as None rather than rejecting, so the optional header stays optional. The denial message is deliberately identical whether a delegation was absent, expired, or named a different repository. A behavioural test asserts the two bodies are byte-identical, because a difference would turn the refusal into an oracle for which capabilities exist. The behavioural test drives both auth layers with a real RFC 9421 signature and a real invocation, and discriminates on status: 500 means the request passed require_signature, passed require_ucan_chain, cleared the owner gate, and reached git on a repo with no disk backing. A bare `!= 403` would let a 401 regression through. It needs no fake-git shim, so unlike the rest of the push path it is not cfg(unix) and runs on every platform. Both new tests were verified to fail: with the delegation branch removed the behavioural test reports 403 where it wants 500, and the unit test's assertion fires. A test written after its implementation proves nothing until it has been watched failing. --- crates/gitlawb-node/src/api/repos.rs | 101 ++++++++++++++++-- crates/gitlawb-node/src/auth/mod.rs | 20 ++-- crates/gitlawb-node/src/test_support.rs | 136 ++++++++++++++++++++++++ 3 files changed, 240 insertions(+), 17 deletions(-) diff --git a/crates/gitlawb-node/src/api/repos.rs b/crates/gitlawb-node/src/api/repos.rs index e8f17497..abcaad60 100644 --- a/crates/gitlawb-node/src/api/repos.rs +++ b/crates/gitlawb-node/src/api/repos.rs @@ -1596,12 +1596,13 @@ fn owner_push_rejection( enforce: bool, record: &crate::db::RepoRecord, caller: Option<&str>, + verified: Option<&crate::auth::VerifiedUcan>, ) -> Option { if !enforce { return None; } match caller { - Some(did) if caller_authorized_to_push(record, did) => None, + Some(did) if caller_authorized_to_push(record, did, verified) => None, _ => Some(AppError::Forbidden( "push rejected — only the repo owner may push to this repository \ (GITLAWB_ENFORCE_OWNER_PUSH is enabled)" @@ -1743,6 +1744,10 @@ pub async fn git_receive_pack( State(state): State, Path((owner, repo)): Path<(String, String)>, Extension(auth): Extension, + // `X-Ucan` is optional, so the extension may be absent: axum extracts that as + // `None` rather than rejecting the request. Present only when the middleware + // validated a chain. + verified: Option>, crate::rate_limit::PeerAddr(peer): crate::rate_limit::PeerAddr, headers: axum::http::HeaderMap, body: Bytes, @@ -1790,6 +1795,7 @@ pub async fn git_receive_pack( state.config.enforce_owner_push, &record, Some(auth.0.as_str()), + verified.as_ref().map(|Extension(v)| v), ) { tracing::warn!( repo = %name, @@ -3326,7 +3332,7 @@ mod tests { #[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()); + assert!(owner_push_rejection(true, &repo, Some(OWNER_DID), None).is_none()); } #[test] @@ -3334,36 +3340,90 @@ mod tests { // 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()); + assert!(owner_push_rejection(true, &repo, Some(OWNER_SHORT), None).is_none()); } #[test] fn enforced_rejects_non_owner_with_forbidden() { let repo = repo_owned_by(OWNER_DID); - assert_forbidden(owner_push_rejection(true, &repo, Some(STRANGER_DID))); + assert_forbidden(owner_push_rejection(true, &repo, Some(STRANGER_DID), None)); } #[test] 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_forbidden(owner_push_rejection(true, &repo, None)); + assert_forbidden(owner_push_rejection(true, &repo, None, None)); } #[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()); + assert!(owner_push_rejection(false, &repo, Some(STRANGER_DID), None).is_none()); + assert!(owner_push_rejection(false, &repo, None, None).is_none()); + } + + /// Build a VerifiedUcan whose chain roots at `root_did` and which carries + /// `git/push` for `repo`. The token's own issuer and audience do not matter + /// here: the middleware has already bound them before this gate is reached. + fn push_delegation(root_did: &str, repo: &crate::db::RepoRecord) -> crate::auth::VerifiedUcan { + let agent = gitlawb_core::identity::Keypair::generate(); + let node = gitlawb_core::identity::Keypair::generate(); + let ucan = gitlawb_core::ucan::Ucan::issue( + &agent, + node.did(), + vec![gitlawb_core::ucan::Capability::new( + format!("gitlawb://repos/{}/{}", repo.owner_did, repo.name), + gitlawb_core::ucan::caps::GIT_PUSH, + )], + None, + ) + .expect("issue delegation"); + crate::auth::VerifiedUcan { + ucan, + root: root_did.parse().expect("root DID must parse"), + } + } + + #[test] + fn enforced_allows_a_non_owner_holding_an_owner_rooted_push_capability() { + // The regression owner-only push introduced: a CI or delegated key with a + // valid capability was refused exactly like a stranger. + let repo = repo_owned_by(OWNER_DID); + let verified = push_delegation(OWNER_DID, &repo); + assert!( + owner_push_rejection(true, &repo, Some(STRANGER_DID), Some(&verified)).is_none(), + "a delegation rooted at the owner must let a non-owner push" + ); + } + + #[test] + fn enforced_rejects_a_delegation_rooted_at_a_stranger() { + // Anchoring is the whole point: a chain nobody the repo trusts started + // grants nothing, even carrying a perfectly formed push capability. + let repo = repo_owned_by(OWNER_DID); + let verified = push_delegation(STRANGER_DID, &repo); + assert_forbidden(owner_push_rejection( + true, + &repo, + Some(STRANGER_DID), + Some(&verified), + )); + } + + #[test] + fn enforced_still_rejects_a_non_owner_with_no_capability() { + let repo = repo_owned_by(OWNER_DID); + assert_forbidden(owner_push_rejection(true, &repo, Some(STRANGER_DID), None)); } #[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)); + assert!(caller_authorized_to_push(&repo, OWNER_DID, None)); + assert!(caller_authorized_to_push(&repo, OWNER_SHORT, None)); + assert!(!caller_authorized_to_push(&repo, STRANGER_DID, None)); } // ── fork_withheld_blocks (#98 path-scoped fork gate) ── @@ -5639,6 +5699,7 @@ mod tests { State(state.clone()), Path(("z6rp4wr".to_string(), "rp4".to_string())), Extension(crate::auth::AuthenticatedDid(did.to_string())), + None, crate::rate_limit::PeerAddr(Some(capped)), axum::http::HeaderMap::new(), axum::body::Bytes::from_static(b"0000"), @@ -5657,6 +5718,7 @@ mod tests { State(state.clone()), Path(("z6rp4wr".to_string(), "rp4".to_string())), Extension(crate::auth::AuthenticatedDid(did.to_string())), + None, crate::rate_limit::PeerAddr(Some(other)), axum::http::HeaderMap::new(), axum::body::Bytes::from_static(b"0000"), @@ -5760,6 +5822,7 @@ mod tests { State(state_for_task), Path((owner.to_string(), name.to_string())), Extension(crate::auth::AuthenticatedDid(did.to_string())), + None, crate::rate_limit::PeerAddr(Some(peer)), axum::http::HeaderMap::new(), axum::body::Bytes::from_static(b"0000"), @@ -5847,6 +5910,7 @@ mod tests { State(state.clone()), Path((owner.to_string(), name.to_string())), Extension(crate::auth::AuthenticatedDid(did.to_string())), + None, crate::rate_limit::PeerAddr(Some("203.0.113.62:5000".parse().unwrap())), axum::http::HeaderMap::new(), axum::body::Bytes::from_static(b"0000"), @@ -6381,6 +6445,7 @@ mod tests { State(state), Path((owner.to_string(), name.to_string())), Extension(crate::auth::AuthenticatedDid(did.to_string())), + None, crate::rate_limit::PeerAddr(Some(peer.parse::().unwrap())), axum::http::HeaderMap::new(), ref_update_body(new_sha), @@ -6475,6 +6540,7 @@ mod tests { Extension(crate::auth::AuthenticatedDid( "did:key:z6MkF4FastPusherAAAAAAAAAAAAAAAAAAAAAAAAA".to_string(), )), + None, crate::rate_limit::PeerAddr(Some(peer)), axum::http::HeaderMap::new(), axum::body::Bytes::from_static(b"0000"), @@ -6535,6 +6601,7 @@ mod tests { Extension(crate::auth::AuthenticatedDid( "did:key:z6MkF4ParkPusherAAAAAAAAAAAAAAAAAAAAAAAAA".to_string(), )), + None, crate::rate_limit::PeerAddr(Some(peer)), axum::http::HeaderMap::new(), ref_update_body("2222222222222222222222222222222222222222"), @@ -7760,6 +7827,7 @@ mod tests { State(state.clone()), Path(("z6f3repo".to_string(), "r1".to_string())), Extension(crate::auth::AuthenticatedDid(did.to_string())), + None, crate::rate_limit::PeerAddr(Some("203.0.113.81:5000".parse::().unwrap())), axum::http::HeaderMap::new(), axum::body::Bytes::from_static(b"0000"), @@ -7788,6 +7856,7 @@ mod tests { State(state_b), Path(("z6f3repo".to_string(), "r1".to_string())), Extension(crate::auth::AuthenticatedDid(did.to_string())), + None, crate::rate_limit::PeerAddr(Some( "203.0.113.82:5000".parse::().unwrap(), )), @@ -7884,6 +7953,7 @@ mod tests { State(st), Path(("z6f3clean".to_string(), "c1".to_string())), Extension(crate::auth::AuthenticatedDid(did.to_string())), + None, crate::rate_limit::PeerAddr(Some(peer.parse::().unwrap())), axum::http::HeaderMap::new(), axum::body::Bytes::from_static(b"0000"), @@ -7977,6 +8047,7 @@ mod tests { State(state.clone()), Path(("z6f3dos".to_string(), "d1".to_string())), Extension(crate::auth::AuthenticatedDid(did.to_string())), + None, crate::rate_limit::PeerAddr(Some("203.0.113.71:5000".parse::().unwrap())), axum::http::HeaderMap::new(), axum::body::Bytes::from_static(b"0000"), @@ -8004,6 +8075,7 @@ mod tests { State(state_b), Path(("z6f3dos".to_string(), "d1".to_string())), Extension(crate::auth::AuthenticatedDid(did.to_string())), + None, crate::rate_limit::PeerAddr(Some( "203.0.113.72:5000".parse::().unwrap(), )), @@ -8188,6 +8260,7 @@ mod tests { State(st), Path(("z6u2key".to_string(), "k1".to_string())), Extension(crate::auth::AuthenticatedDid(did)), + None, crate::rate_limit::PeerAddr(Some(peer)), axum::http::HeaderMap::new(), axum::body::Bytes::from_static(b"0000"), @@ -8261,6 +8334,7 @@ mod tests { Extension(crate::auth::AuthenticatedDid( "did:key:z6MkOverflowPusherAAAAAAAAAAAAAAAAAAAAAA".to_string(), )), + None, crate::rate_limit::PeerAddr(Some("203.0.113.90:5000".parse::().unwrap())), axum::http::HeaderMap::new(), ref_update_body("1111111111111111111111111111111111111111"), @@ -8315,6 +8389,7 @@ mod tests { State(st), Path(("z6u1cap".to_string(), "c1".to_string())), Extension(crate::auth::AuthenticatedDid(did)), + None, crate::rate_limit::PeerAddr(Some(peer)), axum::http::HeaderMap::new(), axum::body::Bytes::from_static(b"0000"), @@ -8420,6 +8495,7 @@ mod tests { State(st), Path(("z6u1two".to_string(), repo.to_string())), Extension(crate::auth::AuthenticatedDid(did)), + None, crate::rate_limit::PeerAddr(Some(src)), axum::http::HeaderMap::new(), axum::body::Bytes::from_static(b"0000"), @@ -8501,6 +8577,7 @@ mod tests { State(st), Path(("z6u1nat".to_string(), repo.to_string())), Extension(crate::auth::AuthenticatedDid(pusher.to_string())), + None, crate::rate_limit::PeerAddr(Some(edge)), axum::http::HeaderMap::new(), axum::body::Bytes::from_static(b"0000"), @@ -8599,6 +8676,7 @@ mod tests { State(st), Path(("z6f1key".to_string(), repo.to_string())), Extension(crate::auth::AuthenticatedDid(did.to_string())), + None, crate::rate_limit::PeerAddr(Some(peer)), axum::http::HeaderMap::new(), axum::body::Bytes::from_static(b"0000"), @@ -8656,6 +8734,7 @@ mod tests { Extension(crate::auth::AuthenticatedDid( "did:key:z6MkF1NoKeyPusherAAAAAAAAAAAAAAAAAAAAAAA".to_string(), )), + None, crate::rate_limit::PeerAddr(None), axum::http::HeaderMap::new(), axum::body::Bytes::from_static(b"0000"), @@ -8695,6 +8774,7 @@ mod tests { State(state.clone()), Path(("z6f1seq".to_string(), "s1".to_string())), Extension(crate::auth::AuthenticatedDid(did.to_string())), + None, crate::rate_limit::PeerAddr(Some(src)), axum::http::HeaderMap::new(), axum::body::Bytes::from_static(b"0000"), @@ -9203,6 +9283,7 @@ mod tests { State(state.clone()), Path((owner.to_string(), name.to_string())), Extension(crate::auth::AuthenticatedDid(P2_PUSHER.to_string())), + None, crate::rate_limit::PeerAddr(Some("203.0.113.90:5000".parse::().unwrap())), axum::http::HeaderMap::new(), axum::body::Bytes::from_static(b"0000"), diff --git a/crates/gitlawb-node/src/auth/mod.rs b/crates/gitlawb-node/src/auth/mod.rs index 703c2da0..4cccd3b4 100644 --- a/crates/gitlawb-node/src/auth/mod.rs +++ b/crates/gitlawb-node/src/auth/mod.rs @@ -33,14 +33,20 @@ pub struct VerifiedUcan { /// Whether `caller` is authorized to push to `record`. /// -/// Phase 1 (`GITLAWB_ENFORCE_OWNER_PUSH`): owner-only, via the canonical -/// [`crate::api::did_matches`] owner comparison (DID-safe on both sides). 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 (`did_matches(..) || ucan_grants_push(..)`) without rewriting -/// call sites. -pub fn caller_authorized_to_push(record: &crate::db::RepoRecord, caller: &str) -> bool { +/// The repo owner, or a caller presenting a verified UCAN whose chain roots at +/// that owner and which carries `git/push` for this repo. +/// +/// `verified` is optional because `X-Ucan` is: a push carrying no token reaches +/// the same owner-only decision it always did. The owner check is unconditional +/// and runs first, so this can only ever turn a refusal into an acceptance, +/// never the reverse. +pub fn caller_authorized_to_push( + record: &crate::db::RepoRecord, + caller: &str, + verified: Option<&VerifiedUcan>, +) -> bool { crate::api::did_matches(caller, &record.owner_did) + || verified.is_some_and(|v| ucan_grants_push(record, v)) } /// Whether `with` names this repository. diff --git a/crates/gitlawb-node/src/test_support.rs b/crates/gitlawb-node/src/test_support.rs index 2b5aef95..5b3b7aba 100644 --- a/crates/gitlawb-node/src/test_support.rs +++ b/crates/gitlawb-node/src/test_support.rs @@ -2172,6 +2172,142 @@ mod tests { ); } + /// Delegated push, end to end through both auth layers. + /// + /// A non-owner presenting an invocation whose chain roots at the repo owner + /// clears the owner-push gate; the same signer without one, and with one that + /// names a different repository, are both refused. This is the regression the + /// owner-push default introduced: a CI or delegated key holding a valid + /// `git/push` capability was refused exactly like a stranger. + /// + /// Status codes are the discriminators. 500 means the request passed + /// `require_signature` (not 401), passed `require_ucan_chain` (not 401), and + /// cleared the owner gate (not 403), then reached git on a repo with no disk + /// backing — the same shape `git_upload_pack_post_is_read_gated_on_private_repo` + /// relies on. A bare `!= 403` would let a 401 regression pass. + /// + /// Not `#[cfg(unix)]`: no fake-git shim is involved, only HTTP and the gate. + #[sqlx::test] + async fn delegated_push_clears_the_owner_gate(pool: PgPool) { + use gitlawb_core::http_sig::sign_request; + use gitlawb_core::identity::Keypair; + use gitlawb_core::ucan::{caps, Capability, Ucan}; + use std::sync::Arc; + + let owner = Keypair::generate(); + let agent = Keypair::generate(); + let owner_did = owner.did().to_string(); + let short = owner_did.split(':').next_back().unwrap().to_string(); + + let mut state = test_state(pool).await; + // Explicit rather than relying on the shipped default, so this test states + // the configuration it is about. + let mut cfg = (*state.config).clone(); + cfg.enforce_owner_push = true; + state.config = Arc::new(cfg); + + state + .db + .create_repo(&seed_repo(&owner_did, "deleg")) + .await + .expect("seed repo"); + + let router = || { + Router::new() + .route( + "/{owner}/{repo}/git-receive-pack", + axum::routing::post(crate::api::repos::git_receive_pack), + ) + .layer(axum::middleware::from_fn_with_state( + state.clone(), + crate::auth::require_ucan_chain, + )) + .layer(axum::middleware::from_fn(crate::auth::require_signature)) + .with_state(state.clone()) + }; + + let path = format!("/{short}/deleg.git/git-receive-pack"); + let body = b"0000".to_vec(); + + // owner -> agent delegation, then agent -> node invocation carrying it. + let invocation_for = |resource: String| { + let delegation = Ucan::issue( + &owner, + agent.did(), + vec![Capability::new(resource, caps::GIT_PUSH)], + None, + ) + .expect("issue delegation"); + Ucan::delegate( + &agent, + state.node_did.clone(), + delegation.payload.att.clone(), + None, + &delegation, + ) + .expect("wrap invocation") + .encode() + .expect("encode invocation") + }; + + let signed_push = |ucan: Option| { + let signed = sign_request(&agent, "POST", &path, &body); + let mut req = Request::builder() + .method(Method::POST) + .uri(&path) + .header("content-type", "application/x-git-receive-pack-request") + .header("content-digest", signed.content_digest) + .header("signature-input", signed.signature_input) + .header("signature", signed.signature); + if let Some(token) = ucan { + req = req.header("x-ucan", token); + } + req.body(Body::from(body.clone())).expect("request") + }; + + // 1. Valid delegation for THIS repo: clears the gate. + let resp = router() + .oneshot(signed_push(Some(invocation_for(format!( + "gitlawb://repos/{owner_did}/deleg" + ))))) + .await + .unwrap(); + assert_eq!( + resp.status(), + StatusCode::INTERNAL_SERVER_ERROR, + "an owner-rooted git/push delegation must clear the owner gate and reach git" + ); + + // 2. Delegation naming a DIFFERENT repo: refused. + let other = router() + .oneshot(signed_push(Some(invocation_for(format!( + "gitlawb://repos/{owner_did}/someotherrepo" + ))))) + .await + .unwrap(); + assert_eq!( + other.status(), + StatusCode::FORBIDDEN, + "a delegation for another repository must not authorize this push" + ); + let other_body = axum::body::to_bytes(other.into_body(), 4096).await.unwrap(); + + // 3. No delegation at all: refused, with a byte-identical body. A caller + // must not be able to tell a non-applicable delegation from none, or the + // denial becomes an oracle for which capabilities exist. + let none = router().oneshot(signed_push(None)).await.unwrap(); + assert_eq!( + none.status(), + StatusCode::FORBIDDEN, + "a non-owner with no delegation must still be refused" + ); + let none_body = axum::body::to_bytes(none.into_body(), 4096).await.unwrap(); + assert_eq!( + other_body, none_body, + "an inapplicable delegation and no delegation must be indistinguishable" + ); + } + /// A1 Phase-2 contract: the `git-upload-pack` POST (the actual fetch, after /// the advertisement) is itself read-visibility gated. An ANONYMOUS upload-pack /// POST against a private repo is denied (404), so signing only the Phase-1 From d141e59dc2d9104eaf9e4ef473219d66375d8466 Mon Sep 17 00:00:00 2001 From: Vasanthdev2004 Date: Fri, 14 Aug 2026 19:24:42 +0530 Subject: [PATCH 06/18] feat(gl): gl ucan import stores a delegation for the push helper MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The git remote helper needs a delegation on disk in a location it can derive from a gitlawb:// URL alone. Files key on the bare base58 owner key because did:key contains a colon, which Windows will not accept in a filename, and the same identity appears in both full and bare form across this codebase — storing under one and looking up by the other would silently miss. Import decodes the token so a malformed delegation fails here, where the error is actionable, rather than surfacing as an unexplained 403 in the middle of a git push. A delegation naming no repository is rejected with the resources it did carry, since that is almost always a wrong --cap argument. --- crates/gl/src/identity.rs | 4 +- crates/gl/src/ucan_cmd.rs | 107 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 110 insertions(+), 1 deletion(-) diff --git a/crates/gl/src/identity.rs b/crates/gl/src/identity.rs index bde5c94c..cab929bd 100644 --- a/crates/gl/src/identity.rs +++ b/crates/gl/src/identity.rs @@ -63,7 +63,9 @@ pub async fn run(cmd: IdentityCmd) -> Result<()> { } } -fn gitlawb_dir(override_dir: Option) -> Result { +/// Resolve the identity directory, honouring an explicit override. +/// Public so sibling commands (`gl ucan import`) store alongside the identity. +pub fn gitlawb_dir(override_dir: Option) -> Result { if let Some(d) = override_dir { return Ok(d); } diff --git a/crates/gl/src/ucan_cmd.rs b/crates/gl/src/ucan_cmd.rs index 99d8841c..08194b3b 100644 --- a/crates/gl/src/ucan_cmd.rs +++ b/crates/gl/src/ucan_cmd.rs @@ -53,6 +53,38 @@ pub enum UcanCmd { /// UCAN JSON token (or path to file containing it) token: String, }, + /// Store a delegation received from a repo owner, so `git push` can present it + Import { + /// UCAN JSON token (or path to a file containing it) + token: String, + /// Identity directory + #[arg(long)] + dir: Option, + }, +} + +/// Where a delegation for `owner_did`/`repo` is stored. +/// +/// Keyed on the bare base58 key rather than the full DID: `did:key:` contains a +/// colon, which is not a legal filename character on Windows, and the same +/// identity appears in both forms across this codebase — storing under one form +/// and looking up by the other would silently miss. +/// +/// `git-remote-gitlawb` derives the same path from a `gitlawb://` URL alone; the +/// two must agree, and the helper carries a pointer back to this function. +pub fn delegation_path(dir: &std::path::Path, owner_did: &str, repo: &str) -> PathBuf { + let bare = owner_did.strip_prefix("did:key:").unwrap_or(owner_did); + dir.join("delegations").join(format!("{bare}__{repo}.ucan")) +} + +/// Pull the repo this capability names out of `gitlawb://repos//`. +fn repo_from_resource(with: &str) -> Option<(String, String)> { + let rest = with.strip_prefix("gitlawb://repos/")?; + let (owner, name) = rest.rsplit_once('/')?; + if owner.is_empty() || name.is_empty() { + return None; + } + Some((owner.to_string(), name.to_string())) } pub async fn run(args: UcanArgs) -> Result<()> { @@ -68,7 +100,57 @@ pub async fn run(args: UcanArgs) -> Result<()> { } => cmd_delegate(to, cap, can, expiry, out, dir, json_out).await, UcanCmd::Show { dir } => cmd_show(dir).await, UcanCmd::Verify { token } => cmd_verify(token).await, + UcanCmd::Import { token, dir } => cmd_import(token, dir).await, + } +} + +/// Store a delegation where `git-remote-gitlawb` will look for it on push. +/// +/// The token is decoded here rather than at push time so a malformed delegation +/// fails where the error is actionable, instead of surfacing as an unexplained +/// 403 in the middle of a `git push`. +async fn cmd_import(token: String, dir: Option) -> Result<()> { + let raw = match std::fs::read_to_string(&token) { + Ok(contents) => contents.trim().to_string(), + Err(_) => token.clone(), + }; + + let ucan = Ucan::decode(&raw).context( + "not a valid UCAN token — pass the JSON emitted by `gl ucan delegate`, or a path to it", + )?; + + let push_caps: Vec<(String, String)> = ucan + .payload + .att + .iter() + .filter_map(|cap| repo_from_resource(&cap.with)) + .collect(); + + if push_caps.is_empty() { + anyhow::bail!( + "this delegation names no repository — expected a capability on \ + gitlawb://repos//, found: {}", + ucan.payload + .att + .iter() + .map(|c| c.with.as_str()) + .collect::>() + .join(", ") + ); + } + + let base = crate::identity::gitlawb_dir(dir)?; + std::fs::create_dir_all(base.join("delegations")) + .with_context(|| format!("could not create {}", base.join("delegations").display()))?; + + for (owner, repo) in &push_caps { + let path = delegation_path(&base, owner, repo); + std::fs::write(&path, &raw) + .with_context(|| format!("could not write {}", path.display()))?; + println!("Stored delegation for {owner}/{repo} at {}", path.display()); } + + Ok(()) } async fn cmd_delegate( @@ -348,3 +430,28 @@ mod tests { .unwrap(); } } + +#[cfg(test)] +mod delegation_store_tests { + use super::*; + + #[test] + fn delegation_path_strips_the_did_prefix_and_separates_owner_from_repo() { + let base = std::path::Path::new("/tmp/id"); + let expected = base.join("delegations").join("z6MkAbc__myrepo.ucan"); + + assert_eq!( + delegation_path(base, "did:key:z6MkAbc", "myrepo"), + expected, + "the bare key keys the file: `did:key:` contains ':', which is not a \ + legal filename character on Windows" + ); + // A bare owner and a full DID must resolve to the same file, or a + // delegation stored under one form is invisible to a lookup by the other. + assert_eq!( + delegation_path(base, "z6MkAbc", "myrepo"), + expected, + "bare and full owner forms must address the same delegation" + ); + } +} From 3d81f578ee4fc269b3405bbfb6debd8f46f5f9c2 Mon Sep 17 00:00:00 2001 From: Vasanthdev2004 Date: Fri, 14 Aug 2026 19:33:20 +0530 Subject: [PATCH 07/18] feat(git-remote): send a delegated push capability as X-Ucan MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Wraps a stored delegation into an invocation (iss=agent, aud=node, prf=[delegation]) and attaches it to the receive-pack POST, which is what lets a CI or delegated key push once owner-only push is enforced. Only on the push: a fetch is gated by read visibility, not by git/push. Capabilities are copied from the delegation unchanged, so an invocation can never be broader than what was delegated. No expiry is set on the invocation itself — the delegation`s own exp still bounds the chain because verify_chain checks each proof`s expiry as it recurses. A test proves that rather than asserting it: an already-expired delegation still fails the chain after wrapping, so an expired grant cannot be laundered into an open-ended one. The whole path is best-effort. A missing delegation, an unreachable node, or an unreadable stored token all send the request without the header. The node decides whether one was required, and a node denial has to reach the user instead of being pre-empted by a local guess. delegation_path duplicates gl`s six-line version because gl is not a dependency of this crate; both carry a pointer to the other. --- crates/git-remote-gitlawb/Cargo.toml | 6 + crates/git-remote-gitlawb/src/main.rs | 193 ++++++++++++++++++++++++++ 2 files changed, 199 insertions(+) diff --git a/crates/git-remote-gitlawb/Cargo.toml b/crates/git-remote-gitlawb/Cargo.toml index b6b9e76c..49ac7186 100644 --- a/crates/git-remote-gitlawb/Cargo.toml +++ b/crates/git-remote-gitlawb/Cargo.toml @@ -14,12 +14,18 @@ path = "src/main.rs" gitlawb-core = { path = "../gitlawb-core" } anyhow = { workspace = true } reqwest = { workspace = true } +# Reading the node's DID from `GET /` so a delegated push can address its +# invocation to the right executor. +serde_json = { workspace = true } tracing = { workspace = true } tracing-subscriber = { workspace = true } [dev-dependencies] mockito = "1" tempfile = "3" +# Test-only: constructing an already-expired delegation needs a timestamp. The +# helper itself never sets an expiry — the proof's own exp bounds the chain. +chrono = { workspace = true } [target.'cfg(unix)'.dev-dependencies] libc = "0.2" diff --git a/crates/git-remote-gitlawb/src/main.rs b/crates/git-remote-gitlawb/src/main.rs index 02e39c3e..886cb2ce 100644 --- a/crates/git-remote-gitlawb/src/main.rs +++ b/crates/git-remote-gitlawb/src/main.rs @@ -358,6 +358,115 @@ fn build_advertisement_request( /// Public-repo fetch still works anonymously when no keypair is present. The body /// is signed (content-digest) but NOT attached here, so the caller can move the /// (possibly large) pack bytes into `.body()` rather than clone them. +/// Where a delegation for `owner_did`/`repo` is stored. +/// +/// Must agree with `gl`'s `ucan_cmd::delegation_path`, which writes these files. +/// `gl` is not a dependency of this crate, so the six lines are duplicated rather +/// than shared; change both together. Keyed on the bare base58 key because +/// `did:key:` contains a colon, which Windows rejects in a filename. +fn delegation_path(dir: &std::path::Path, owner_did: &str, repo: &str) -> std::path::PathBuf { + let bare = owner_did.strip_prefix("did:key:").unwrap_or(owner_did); + dir.join("delegations").join(format!("{bare}__{repo}.ucan")) +} + +/// Wrap a stored delegation into an invocation addressed to the node. +/// +/// `iss=agent, aud=node, prf=[delegation]` is exactly the shape the node's +/// `validate_ucan_chain` expects: it binds `iss` to the request signer and `aud` +/// to its own DID, then walks `prf` to the root. +/// +/// Capabilities are copied from the delegation unchanged, so the invocation is +/// never broader than what was delegated and cannot fail attenuation. No expiry +/// is set: the delegation's own `exp` still bounds the chain, because +/// `verify_chain` checks each proof's expiry as it recurses. +fn build_invocation( + agent: &Keypair, + node_did: &gitlawb_core::did::Did, + delegation: &gitlawb_core::ucan::Ucan, +) -> Result { + gitlawb_core::ucan::Ucan::delegate( + agent, + node_did.clone(), + delegation.payload.att.clone(), + None, + delegation, + ) + .map_err(|e| anyhow::anyhow!("failed to build UCAN invocation: {e}")) +} + +/// Split a receive-pack POST URL into `(origin, owner, repo)`. +/// +/// `https://node/zOwner/myrepo.git/git-receive-pack` +/// -> ("https://node", "zOwner", "myrepo") +fn split_pack_post_url(post_url: &str) -> Option<(String, String, String)> { + let path = url_path(post_url); + let origin = post_url.strip_suffix(&path)?.to_string(); + let mut segs = path.trim_start_matches('/').split('/'); + let owner = segs.next()?; + let repo = segs.next()?; + if owner.is_empty() || repo.is_empty() { + return None; + } + let repo = repo.strip_suffix(".git").unwrap_or(repo); + Some((origin, owner.to_string(), repo.to_string())) +} + +/// Build the `X-Ucan` value for a delegated push, or `None` when this push does +/// not need one. +/// +/// Entirely best-effort. A missing delegation, an unreachable node, or an +/// unreadable stored token all yield `None` and the push proceeds without the +/// header — the node decides whether one was required, and a node denial must +/// reach the user rather than being pre-empted by a local guess. +fn delegation_header( + client: &reqwest::blocking::Client, + post_url: &str, + keypair: &Keypair, +) -> Option { + let (origin, owner, repo) = split_pack_post_url(post_url)?; + + // The owner pushes on their own authority; no delegation is involved. + let bare = |d: &str| d.strip_prefix("did:key:").unwrap_or(d).to_string(); + if bare(&keypair.did().to_string()) == bare(&owner) { + return None; + } + + let dir = resolve_key_path().parent()?.to_path_buf(); + let path = delegation_path(&dir, &owner, &repo); + let raw = std::fs::read_to_string(&path).ok()?; + + let delegation = match gitlawb_core::ucan::Ucan::decode(raw.trim()) { + Ok(u) => u, + Err(e) => { + tracing::warn!("stored delegation at {path:?} is unreadable: {e}"); + return None; + } + }; + + // The invocation must be addressed to the node that will execute it. + let node_did: gitlawb_core::did::Did = client + .get(&origin) + .header("User-Agent", USER_AGENT) + .send() + .ok() + .and_then(|r| r.json::().ok()) + .and_then(|v| v.get("did")?.as_str().map(str::to_owned)) + .or_else(|| { + tracing::warn!("could not read the node DID from {origin}; pushing without X-Ucan"); + None + })? + .parse() + .ok()?; + + match build_invocation(keypair, &node_did, &delegation) { + Ok(inv) => inv.encode().ok(), + Err(e) => { + tracing::warn!("could not build the UCAN invocation: {e}"); + None + } + } +} + fn build_pack_post_request( client: &reqwest::blocking::Client, post_url: &str, @@ -376,6 +485,15 @@ fn build_pack_post_request( .header("Signature-Input", signed.signature_input) .header("Signature", signed.signature); tracing::debug!("signed {service} POST (DID: {})", kp.did()); + + // A non-owner pushing under a delegation presents it here. Only on the + // push: a fetch is gated by read visibility, not by git/push. + if service == "git-receive-pack" { + if let Some(token) = delegation_header(client, post_url, kp) { + tracing::debug!("attaching a delegated push capability"); + req = req.header("X-Ucan", token); + } + } } else if service == "git-receive-pack" { tracing::warn!("no identity keypair found, push will be unsigned (v0.1 local alpha only)"); } @@ -2170,3 +2288,78 @@ mod tests { ); } } + +#[cfg(test)] +mod delegated_push_tests { + use super::*; + use gitlawb_core::ucan::{caps, Capability, Ucan}; + + #[test] + fn invocation_wraps_the_delegation_and_targets_the_node() { + let owner = Keypair::generate(); + let agent = Keypair::generate(); + let node = Keypair::generate(); + let delegation = Ucan::issue( + &owner, + agent.did(), + vec![Capability::new("gitlawb://repos/zowner/r", caps::GIT_PUSH)], + None, + ) + .expect("issue"); + + let invocation = build_invocation(&agent, &node.did(), &delegation).expect("wrap"); + + assert_eq!(invocation.payload.iss, agent.did(), "the agent invokes"); + assert_eq!(invocation.payload.aud, node.did(), "the node executes"); + assert_eq!( + invocation.payload.prf.len(), + 1, + "exactly one proof: chains are linear" + ); + assert_eq!( + invocation.verify_chain().expect("must verify"), + owner.did(), + "the chain must still root at the owner after wrapping" + ); + } + + /// The invocation deliberately carries no expiry of its own. That is only safe + /// because `verify_chain` recurses into the proof and checks the delegation's + /// expiry there — so an expired delegation cannot be laundered into an + /// open-ended push capability by wrapping it. + #[test] + fn an_expired_delegation_cannot_be_laundered_by_wrapping_it() { + let owner = Keypair::generate(); + let agent = Keypair::generate(); + let node = Keypair::generate(); + let expired = Ucan::issue( + &owner, + agent.did(), + vec![Capability::new("gitlawb://repos/zowner/r", caps::GIT_PUSH)], + Some(chrono::Utc::now() - chrono::Duration::hours(1)), + ) + .expect("issue expired"); + + let invocation = build_invocation(&agent, &node.did(), &expired).expect("wrap"); + + assert!( + invocation.payload.exp.is_none(), + "the invocation sets no expiry of its own" + ); + let err = invocation + .verify_chain() + .expect_err("an expired proof must fail the chain"); + assert!( + err.to_string().contains("expired"), + "the failure must name expiry, got: {err}" + ); + } + + #[test] + fn delegation_path_matches_the_gl_layout() { + let base = std::path::Path::new("/tmp/id"); + let expected = base.join("delegations").join("z6MkAbc__myrepo.ucan"); + assert_eq!(delegation_path(base, "did:key:z6MkAbc", "myrepo"), expected); + assert_eq!(delegation_path(base, "z6MkAbc", "myrepo"), expected); + } +} From 8843c4ffdfbab34ea64281602b967c0338e6cdbc Mon Sep 17 00:00:00 2001 From: Vasanthdev2004 Date: Fri, 14 Aug 2026 19:34:54 +0530 Subject: [PATCH 08/18] test(git-remote): cover pack-POST URL splitting split_pack_post_url decides which repo a delegation is looked up for, so a parsing slip silently means no delegation was found, surfacing as a confusing 403 rather than a visible error. Covers the optional .git suffix, a repo genuinely named x.git, and the malformed shapes that must not parse. --- crates/git-remote-gitlawb/src/main.rs | 37 +++++++++++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/crates/git-remote-gitlawb/src/main.rs b/crates/git-remote-gitlawb/src/main.rs index 886cb2ce..795293d5 100644 --- a/crates/git-remote-gitlawb/src/main.rs +++ b/crates/git-remote-gitlawb/src/main.rs @@ -2355,6 +2355,43 @@ mod delegated_push_tests { ); } + #[test] + fn split_pack_post_url_separates_origin_owner_and_repo() { + assert_eq!( + split_pack_post_url("http://127.0.0.1:7545/z6Mk/myrepo.git/git-receive-pack"), + Some(( + "http://127.0.0.1:7545".to_string(), + "z6Mk".to_string(), + "myrepo".to_string() + )), + "the origin must come back intact so the node DID can be fetched from it" + ); + // The .git suffix is optional on the wire; the delegation is stored under + // the bare repo name either way, so both forms must resolve identically. + assert_eq!( + split_pack_post_url("https://node.example/z6Mk/myrepo/git-receive-pack") + .map(|(_, _, r)| r), + Some("myrepo".to_string()) + ); + // A repo genuinely named "x.git" keeps its name: only one suffix is stripped. + assert_eq!( + split_pack_post_url("https://node.example/z6Mk/x.git.git/git-receive-pack") + .map(|(_, _, r)| r), + Some("x.git".to_string()) + ); + for bad in [ + "not-a-url", + "https://node.example", + "https://node.example/", + "https://node.example/onlyowner", + ] { + assert!( + split_pack_post_url(bad).is_none(), + "{bad} must not parse as a pack POST URL" + ); + } + } + #[test] fn delegation_path_matches_the_gl_layout() { let base = std::path::Path::new("/tmp/id"); From a9e9c4813cfa32dde068ffbb537e90312ca6552c Mon Sep 17 00:00:00 2001 From: Vasanthdev2004 Date: Fri, 14 Aug 2026 19:45:27 +0530 Subject: [PATCH 09/18] build: sync Cargo.lock for the git-remote-gitlawb dependencies Adding serde_json and chrono to crates/git-remote-gitlawb/Cargo.toml updated Cargo.lock locally, but the lockfile lives at the repo root and was never staged. Every CI job builds with --locked, so all seven compiling jobs failed immediately with "cannot update the lock file ... because --locked was passed". --- Cargo.lock | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Cargo.lock b/Cargo.lock index b7050bc6..31d6038a 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3377,10 +3377,12 @@ name = "git-remote-gitlawb" version = "0.7.1" dependencies = [ "anyhow", + "chrono", "gitlawb-core", "libc", "mockito", "reqwest", + "serde_json", "tempfile", "tracing", "tracing-subscriber", From f455726ef3e4daf392ef1db7fb0fbcab229af86f Mon Sep 17 00:00:00 2001 From: Vasanthdev2004 Date: Fri, 14 Aug 2026 20:21:50 +0530 Subject: [PATCH 10/18] fix(core,gl,git-remote): close two review findings on delegated push MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both are mine, both were found in review, and both are load-bearing. 1. Stripping `nb` mid-chain escalated a constrained delegation. `Capability::is_attenuated_by` compared only `with` and `can`, so the holder of a capability constrained by `nb` could re-delegate the same resource and action with the constraints removed and `verify_chain` still accepted the chain. The node's push gate refuses constrained capabilities at the LEAF, so it then saw an unconstrained one and granted repo-wide push — guarding the leaf guarded the wrong end of the chain. Constraints now participate in attenuation: an unconstrained parent permits anything, an identical child is attenuated, and both a differing child and a child that drops the constraints are refused. `nb` has no interpreted semantics yet, so "different" cannot be shown to be narrower and is refused with the stripping case. Tests cover the forged strip, the legal unchanged case, and adding constraints under an unconstrained parent. 2. `gl ucan import` wrote to a path taken from an untrusted token. `repo_from_resource` split the resource with `rsplit_once('/')`, so the owner half could carry separators, `..`, or an absolute prefix. `Path::join` with an absolute component discards the base entirely, so a resource of `gitlawb://repos/C:/Windows/System32/x` did not merely climb out of the delegations directory — it replaced it. The value then reached `std::fs::write`. The resource must now be exactly `gitlawb://repos//`, with each half an allow-listed component: alphanumerics plus `.`, `-`, `_` and `:` (a DID carries colons), never `.` or `..` or anything containing `..`. An allow-list rather than a deny-list of separators, which would miss whichever ones the next platform introduces. The same shape existed in the helper's `split_pack_post_url`. There the value comes from the remote URL and the path is only read, but a traversing owner would read an arbitrary file and send its contents to the node as `X-Ucan`, so it carries the same guard. Also: the node-DID probe now uses a 5s timeout instead of inheriting the shared client's 300s. It is best-effort metadata, and a stalled node should not delay every delegated push by five minutes before falling back to sending no header. --- crates/git-remote-gitlawb/src/main.rs | 32 +++++++++ crates/gitlawb-core/src/ucan.rs | 98 ++++++++++++++++++++++++++- crates/gl/src/ucan_cmd.rs | 79 ++++++++++++++++++++- 3 files changed, 206 insertions(+), 3 deletions(-) diff --git a/crates/git-remote-gitlawb/src/main.rs b/crates/git-remote-gitlawb/src/main.rs index 795293d5..18f2f54a 100644 --- a/crates/git-remote-gitlawb/src/main.rs +++ b/crates/git-remote-gitlawb/src/main.rs @@ -358,6 +358,26 @@ fn build_advertisement_request( /// Public-repo fetch still works anonymously when no keypair is present. The body /// is signed (content-digest) but NOT attached here, so the caller can move the /// (possibly large) pack bytes into `.body()` rather than clone them. +/// How long to wait for the node's DID before giving up and pushing without a +/// delegation. Short on purpose: the answer is optional, and the node decides +/// whether the header was required. +const NODE_DID_TIMEOUT_SECS: u64 = 5; + +/// A path component safe to build a filename from. Mirrors `gl`'s +/// `ucan_cmd::is_safe_component`; change both together. +/// +/// Here the value comes from the remote URL rather than a token, and the path is +/// only read — but a `..` owner would still read an arbitrary file and send its +/// contents to the node as `X-Ucan`, so the same allow-list applies. +fn is_safe_component(s: &str) -> bool { + !s.is_empty() + && s != "." + && s != ".." + && !s.contains("..") + && s.chars() + .all(|c| c.is_ascii_alphanumeric() || matches!(c, '.' | '-' | '_' | ':')) +} + /// Where a delegation for `owner_did`/`repo` is stored. /// /// Must agree with `gl`'s `ucan_cmd::delegation_path`, which writes these files. @@ -408,6 +428,9 @@ fn split_pack_post_url(post_url: &str) -> Option<(String, String, String)> { return None; } let repo = repo.strip_suffix(".git").unwrap_or(repo); + if !is_safe_component(owner) || !is_safe_component(repo) { + return None; + } Some((origin, owner.to_string(), repo.to_string())) } @@ -444,8 +467,12 @@ fn delegation_header( }; // The invocation must be addressed to the node that will execute it. + // The shared client carries a 300s timeout, which is right for a pack transfer + // and wrong for a best-effort metadata probe: a stalled node would delay every + // delegated push by five minutes before falling back to sending no header. let node_did: gitlawb_core::did::Did = client .get(&origin) + .timeout(std::time::Duration::from_secs(NODE_DID_TIMEOUT_SECS)) .header("User-Agent", USER_AGENT) .send() .ok() @@ -2384,6 +2411,11 @@ mod delegated_push_tests { "https://node.example", "https://node.example/", "https://node.example/onlyowner", + // A traversing owner would read an arbitrary file and send it to the + // node as X-Ucan, so it must not resolve to a lookup at all. + "https://node.example/../../etc/passwd/git-receive-pack", + "https://node.example/../x/git-receive-pack", + "https://node.example/a%2Fb/x/git-receive-pack", ] { assert!( split_pack_post_url(bad).is_none(), diff --git a/crates/gitlawb-core/src/ucan.rs b/crates/gitlawb-core/src/ucan.rs index 38f15b0c..be78b269 100644 --- a/crates/gitlawb-core/src/ucan.rs +++ b/crates/gitlawb-core/src/ucan.rs @@ -49,11 +49,32 @@ impl Capability { /// action field and `repo/admin` in the parent's action position act as /// wildcards that cover any delegated value; wildcards on `self` carry no /// special meaning. + /// + /// Constraints (`nb`) participate, and conservatively: + /// + /// | parent | child | verdict | + /// |---|---|---| + /// | none | anything | attenuated — adding constraints narrows | + /// | some | identical | attenuated | + /// | some | different | refused — narrowing is unprovable without semantics | + /// | some | none | refused — dropping constraints widens | + /// + /// The last row is the one that matters. Ignoring `nb` here let a holder of a + /// constrained capability re-delegate the same resource and action with the + /// constraints removed, and the chain still verified — so a consumer that + /// refuses constrained capabilities at the leaf saw an unconstrained one and + /// granted it. Since `nb` has no interpreted semantics yet, "different" cannot + /// be shown to be narrower and is refused with it. pub fn is_attenuated_by(&self, parent: &Capability) -> bool { let resource_ok = parent.with == self.with || parent.with == "*"; let action_ok = parent.can == self.can || parent.can == "*" || parent.can == caps::REPO_ADMIN; - resource_ok && action_ok + let constraints_ok = match (&parent.constraints, &self.constraints) { + (None, _) => true, + (Some(p), Some(c)) => p == c, + (Some(_), None) => false, + }; + resource_ok && action_ok && constraints_ok } } @@ -722,6 +743,81 @@ mod tests { ); } + /// Stripping `nb` is a widening, and a widening must fail attenuation. + /// + /// Without this, a constrained delegation is trivially escalated: the holder + /// re-delegates the same resource and action with the constraints removed, + /// `verify_chain` accepts the chain because attenuation only compared `with` + /// and `can`, and a consumer that refuses constrained capabilities at the leaf + /// (as the node's push gate does) then sees an unconstrained one and grants it. + /// Guarding only the leaf guards the wrong end of the chain. + #[test] + fn verify_chain_rejects_a_child_that_strips_the_parents_constraints() { + let owner = Keypair::generate(); + let agent = Keypair::generate(); + let node = Keypair::generate(); + + let constrained = Capability::new("gitlawb://repos/zowner/r", caps::GIT_PUSH) + .with_constraints(serde_json::json!({ "refs": ["refs/heads/feat/*"] })); + let delegation = Ucan::issue(&owner, agent.did(), vec![constrained], None) + .expect("issue constrained delegation"); + + // Same resource, same action, constraints dropped. + let widened = Capability::new("gitlawb://repos/zowner/r", caps::GIT_PUSH); + let forged = + Ucan::delegate(&agent, node.did(), vec![widened], None, &delegation).expect("wrap"); + + let err = forged + .verify_chain() + .expect_err("dropping the parent's constraints must fail attenuation"); + assert!( + err.to_string().contains("attenuation"), + "the failure must name attenuation, got: {err}" + ); + } + + #[test] + fn verify_chain_accepts_a_child_that_keeps_the_parents_constraints() { + let owner = Keypair::generate(); + let agent = Keypair::generate(); + let node = Keypair::generate(); + + let nb = serde_json::json!({ "refs": ["refs/heads/feat/*"] }); + let constrained = Capability::new("gitlawb://repos/zowner/r", caps::GIT_PUSH) + .with_constraints(nb.clone()); + let delegation = + Ucan::issue(&owner, agent.did(), vec![constrained.clone()], None).expect("issue"); + let invocation = + Ucan::delegate(&agent, node.did(), vec![constrained], None, &delegation).expect("wrap"); + + assert_eq!( + invocation + .verify_chain() + .expect("an unchanged constraint must verify"), + owner.did() + ); + } + + #[test] + fn an_unconstrained_parent_still_allows_a_child_to_add_constraints() { + // Adding `nb` narrows, which is always a legal attenuation. + let owner = Keypair::generate(); + let agent = Keypair::generate(); + let node = Keypair::generate(); + + let open = Capability::new("gitlawb://repos/zowner/r", caps::GIT_PUSH); + let delegation = Ucan::issue(&owner, agent.did(), vec![open], None).expect("issue"); + let narrowed = Capability::new("gitlawb://repos/zowner/r", caps::GIT_PUSH) + .with_constraints(serde_json::json!({ "refs": ["refs/heads/main"] })); + let invocation = + Ucan::delegate(&agent, node.did(), vec![narrowed], None, &delegation).expect("wrap"); + + assert_eq!( + invocation.verify_chain().expect("narrowing must verify"), + owner.did() + ); + } + #[test] fn verify_chain_rejects_a_multi_proof_chain() { // Two proofs mean two roots, and nothing says which root authorized a diff --git a/crates/gl/src/ucan_cmd.rs b/crates/gl/src/ucan_cmd.rs index 08194b3b..f0534784 100644 --- a/crates/gl/src/ucan_cmd.rs +++ b/crates/gl/src/ucan_cmd.rs @@ -77,11 +77,40 @@ pub fn delegation_path(dir: &std::path::Path, owner_did: &str, repo: &str) -> Pa dir.join("delegations").join(format!("{bare}__{repo}.ucan")) } +/// A path component that is safe to build a filename from. +/// +/// This is load-bearing, not defensive tidiness: the values it guards flow into +/// [`delegation_path`], which `gl ucan import` WRITES to, and they come from a +/// field of an untrusted token. `Path::join` with an absolute component discards +/// the base entirely, so an owner of `/etc/cron.d/x` or `C:/Windows/...` escapes +/// the delegations directory completely rather than merely climbing out of it. +/// +/// Deliberately an allow-list. A DID carries `:` (`did:key:z6Mk…`) and repo names +/// carry `.`, `-` and `_`; nothing else is needed, and a deny-list of separators +/// would miss whichever ones the next platform introduces. +fn is_safe_component(s: &str) -> bool { + !s.is_empty() + && s != "." + && s != ".." + && !s.contains("..") + && s.chars() + .all(|c| c.is_ascii_alphanumeric() || matches!(c, '.' | '-' | '_' | ':')) +} + /// Pull the repo this capability names out of `gitlawb://repos//`. +/// +/// Requires exactly two components after the prefix. Anything else — extra +/// segments, a trailing slash, an empty half — is refused rather than +/// interpreted, so no input can address a location the caller did not intend. fn repo_from_resource(with: &str) -> Option<(String, String)> { let rest = with.strip_prefix("gitlawb://repos/")?; - let (owner, name) = rest.rsplit_once('/')?; - if owner.is_empty() || name.is_empty() { + let mut parts = rest.split('/'); + let owner = parts.next()?; + let name = parts.next()?; + if parts.next().is_some() { + return None; + } + if !is_safe_component(owner) || !is_safe_component(name) { return None; } Some((owner.to_string(), name.to_string())) @@ -435,6 +464,52 @@ mod tests { mod delegation_store_tests { use super::*; + /// `repo_from_resource` feeds `delegation_path`, which builds a filesystem + /// path that `gl ucan import` then WRITES to — from a field of an untrusted + /// token. A separator, a parent-directory hop, or an absolute prefix in the + /// owner escapes the delegations directory; `Path::join` with an absolute + /// component discards the base entirely, so an absolute owner writes anywhere + /// the user can write. + #[test] + fn repo_from_resource_rejects_anything_that_could_escape_the_store() { + for bad in [ + "gitlawb://repos/../../evil/x", + "gitlawb://repos/../x", + "gitlawb://repos/a/../../x", + "gitlawb://repos//x", + "gitlawb://repos/C:/Windows/System32/x", + "gitlawb://repos//etc/cron.d/x", + "gitlawb://repos/a\\b/x", + "gitlawb://repos/owner/sub/dir/x", + "gitlawb://repos/owner/x/", + "gitlawb://repos/owner/", + "gitlawb://repos/owner", + "gitlawb://repos/", + "gitlawb://repos/owner/..", + "gitlawb://repos/owner/.", + "gitlawb://repos/./x", + "https://repos/owner/x", + "", + ] { + assert!( + repo_from_resource(bad).is_none(), + "{bad:?} must not yield a storable owner/repo pair" + ); + } + } + + #[test] + fn repo_from_resource_accepts_the_canonical_shape() { + assert_eq!( + repo_from_resource("gitlawb://repos/did:key:z6MkAbc/myrepo"), + Some(("did:key:z6MkAbc".to_string(), "myrepo".to_string())) + ); + assert_eq!( + repo_from_resource("gitlawb://repos/z6MkAbc/my-repo.rs"), + Some(("z6MkAbc".to_string(), "my-repo.rs".to_string())) + ); + } + #[test] fn delegation_path_strips_the_did_prefix_and_separates_owner_from_repo() { let base = std::path::Path::new("/tmp/id"); From a2502fcd879e1c49767fffa4871f94db642b2295 Mon Sep 17 00:00:00 2001 From: Vasanthdev2004 Date: Fri, 14 Aug 2026 22:35:14 +0530 Subject: [PATCH 11/18] fix(core,node,gl,git-remote): close the second review round on delegated push MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Each finding was reproduced against the code before being changed; two were confirmed with the mutation the reviewer described. A delegation could be perpetual -------------------------------- `exp` is optional, `is_expired` reports false when it is absent, `gl ucan delegate` defaulted to no expiry, and there is no revocation path. So the default flow minted a permanent push grant: once the token leaked, the owner's only remedy was rotating the DID the repo is keyed on. The PR body claimed the damage window was the token's `exp`, which was simply untrue when `exp` was `None`. `Ucan::chain_lifetime_is_bounded` walks every link, and `ucan_grants_push` requires it. It is deliberately NOT enforced inside `verify_chain`: an unbounded token is well-formed and may suit a read-only capability; whether an unbounded grant is acceptable is the consumer's policy, not the format's. `gl ucan delegate` now defaults to 720 hours with an explicit `--no-expiry` opt-out, and the helper carries the delegation's expiry onto the invocation so the leaf is bounded too. The recursion to the root was untested --------------------------------------- Every chain in the suite was depth two, where the immediate proof IS the root, so nothing distinguished walking to the true root from returning the proof's issuer. Confirmed by mutation: keeping full recursive validation but returning `proof.payload.iss` left gitlawb-core at 92 passed, the node's UCAN tests at 17, and the e2e green. A three-link owner -> lead -> agent test now pins it, with `assert_ne!` against the middle issuer as well as `assert_eq!` against the root — without the former, returning the middle would still pass. A path-prefixed node base broke delegated push entirely -------------------------------------------------------- `GITLAWB_NODE` may carry a path (`https://host/gitlawb` behind a proxy), and `repo_base` passes it through verbatim. `split_pack_post_url` read the FIRST two segments as owner/repo, so the prefix became the owner: the delegation lookup missed, the DID probe hit the wrong URL, no `X-Ucan` was sent, and a valid delegate got a 403 — silently, because every failure on that path is best-effort. It now strips the known trailing `//` instead, which is correct at any prefix depth, and prefix segments carry the same allow-list so a `..` cannot redirect the probe. A wildcard delegation grew without limit ----------------------------------------- `with: "*"` covered every repo the owner created AFTER signing, a scope nobody chose. `build_invocation` now narrows to the concrete repo, which `is_attenuated_by` accepts under a `*` parent, so a captured invocation is worth one repo rather than all of them. Constraints are copied from the covering capability rather than dropped, since dropping them is a widening. Branch protection and the owner gate now disagree, on purpose -------------------------------------------------------------- A delegate clears the owner gate and is still refused on a protected branch. That is the intended policy: a protected branch is the owner's explicit marker that even routine writes should stop, and if a delegation overrode it, issuing any capability would weaken every protection already set. The comment claiming non-owners never reach that loop is corrected, and `delegated_push_is_still_refused_on_a_protected_branch` pins it — asserting the body names the branch, so the refusal is provably branch protection rather than the owner gate. Smaller items ------------- The denial body no longer says "only the repo owner may push", which stopped being true once a delegation could authorize one. It stays a single unconditional message: varying it by whether a delegation was presented, expired, or named another repo would turn the refusal into an oracle for which capabilities exist. `gl ucan import` writes the delegation 0600, matching the sibling identity key. The token alone cannot push — the node requires `iss` to equal the request signer — but it discloses the delegation graph. `docs/RUN-A-NODE.md` told operators not to enable owner-push until every pusher was the owner, which is the workflow this change adds. It now documents the delegation flow, the four requirements the node enforces and why, that a delegation does not override branch protection, and that withdrawal is by expiry only. `README.md` no longer describes UCAN as a future workflow. --- README.md | 2 +- crates/git-remote-gitlawb/Cargo.toml | 6 +- crates/git-remote-gitlawb/src/main.rs | 141 +++++++++++++++++++----- crates/gitlawb-core/src/ucan.rs | 95 ++++++++++++++++ crates/gitlawb-node/src/api/repos.rs | 26 ++++- crates/gitlawb-node/src/auth/mod.rs | 36 +++++- crates/gitlawb-node/src/test_support.rs | 130 +++++++++++++++++++++- crates/gl/src/ucan_cmd.rs | 34 +++++- docs/RUN-A-NODE.md | 47 ++++++-- 9 files changed, 466 insertions(+), 51 deletions(-) diff --git a/README.md b/README.md index 643992c2..ca7330ee 100644 --- a/README.md +++ b/README.md @@ -264,7 +264,7 @@ metadata local disk / optional S3 | DID | A user, agent, or node identity derived from an Ed25519 public key. | | HTTP Signature | RFC 9421 signature proving control of the DID key for write requests. | | Ref certificate | Signed record of a ref update. Useful for audit and replication. | -| UCAN | Delegation token for future capability-based workflows. | +| UCAN | Capability token. An owner delegates `git/push` on a repo to another DID; the node honors it when the proof chain roots at that owner. See [`docs/RUN-A-NODE.md`](docs/RUN-A-NODE.md). | | Peer announce | Node-to-node HTTP announcement of DID + public URL. | | Gossipsub | libp2p topic for ref-update events. | | Smart HTTP | Standard git protocol over HTTP for clone/fetch/push. | diff --git a/crates/git-remote-gitlawb/Cargo.toml b/crates/git-remote-gitlawb/Cargo.toml index 49ac7186..bcace360 100644 --- a/crates/git-remote-gitlawb/Cargo.toml +++ b/crates/git-remote-gitlawb/Cargo.toml @@ -17,15 +17,15 @@ reqwest = { workspace = true } # Reading the node's DID from `GET /` so a delegated push can address its # invocation to the right executor. serde_json = { workspace = true } +# The invocation carries the delegation's expiry, so an unbounded write capability +# is never minted; converting the stored i64 timestamp needs chrono. +chrono = { workspace = true } tracing = { workspace = true } tracing-subscriber = { workspace = true } [dev-dependencies] mockito = "1" tempfile = "3" -# Test-only: constructing an already-expired delegation needs a timestamp. The -# helper itself never sets an expiry — the proof's own exp bounds the chain. -chrono = { workspace = true } [target.'cfg(unix)'.dev-dependencies] libc = "0.2" diff --git a/crates/git-remote-gitlawb/src/main.rs b/crates/git-remote-gitlawb/src/main.rs index 18f2f54a..6807f881 100644 --- a/crates/git-remote-gitlawb/src/main.rs +++ b/crates/git-remote-gitlawb/src/main.rs @@ -403,35 +403,92 @@ fn build_invocation( agent: &Keypair, node_did: &gitlawb_core::did::Did, delegation: &gitlawb_core::ucan::Ucan, + owner: &str, + repo: &str, ) -> Result { - gitlawb_core::ucan::Ucan::delegate( - agent, - node_did.clone(), - delegation.payload.att.clone(), - None, - delegation, - ) - .map_err(|e| anyhow::anyhow!("failed to build UCAN invocation: {e}")) + use gitlawb_core::ucan::{caps, Capability}; + + // Narrow to the repo this push actually targets rather than copying `att` + // wholesale. A delegation written as `with: "*"` otherwise grows without + // limit — it would cover every repo the owner creates AFTER signing, a scope + // nobody chose. `is_attenuated_by` accepts a concrete resource under a `*` + // parent, so narrowing is always a legal attenuation. + // + // Constraints are copied from the covering capability, not dropped: dropping + // them is a widening and `verify_chain` refuses it. + let resource = format!("gitlawb://repos/{owner}/{repo}"); + let source = delegation + .payload + .att + .iter() + .find(|c| { + (c.with == resource || c.with == "*") + && (c.can == caps::GIT_PUSH || c.can == "*" || c.can == caps::REPO_ADMIN) + }) + .ok_or_else(|| { + anyhow::anyhow!("stored delegation carries no git/push capability for {resource}") + })?; + + let mut narrowed = Capability::new(resource, caps::GIT_PUSH); + narrowed.constraints = source.constraints.clone(); + + // Carry the delegation's own expiry onto the invocation. The node refuses a + // chain with any unbounded link, because without a revocation path an + // unbounded write capability can never be withdrawn. + let exp = delegation + .payload + .exp + .and_then(|e| chrono::DateTime::from_timestamp(e, 0)); + + gitlawb_core::ucan::Ucan::delegate(agent, node_did.clone(), vec![narrowed], exp, delegation) + .map_err(|e| anyhow::anyhow!("failed to build UCAN invocation: {e}")) } -/// Split a receive-pack POST URL into `(origin, owner, repo)`. +/// Split a pack POST URL into `(node_base, owner, repo)`. +/// +/// ```text +/// https://node/zOwner/myrepo/git-receive-pack -> ("https://node", "zOwner", "myrepo") +/// https://node/gitlawb/zOwner/myrepo/git-receive-pack -> ("https://node/gitlawb", "zOwner", "myrepo") +/// ``` /// -/// `https://node/zOwner/myrepo.git/git-receive-pack` -/// -> ("https://node", "zOwner", "myrepo") +/// Strips the KNOWN trailing `//` rather than reading the +/// first two segments, because `GITLAWB_NODE` may carry a path prefix — a +/// reverse-proxied `https://host/gitlawb` is a supported base, and `repo_base` is +/// built as `{node_base}/{owner}/{repo}` with the service appended after it. +/// Reading from the front makes the prefix the owner, which fails the delegation +/// lookup and probes the wrong URL for the node DID. +/// +/// This is coupled to how `repo_base` is constructed in `main`; the two must +/// change together. fn split_pack_post_url(post_url: &str) -> Option<(String, String, String)> { let path = url_path(post_url); - let origin = post_url.strip_suffix(&path)?.to_string(); - let mut segs = path.trim_start_matches('/').split('/'); - let owner = segs.next()?; - let repo = segs.next()?; - if owner.is_empty() || repo.is_empty() { + let origin = post_url.strip_suffix(&path)?; + + let segs: Vec<&str> = path.trim_start_matches('/').split('/').collect(); + // owner, repo, service — plus any base-path prefix ahead of them. + if segs.len() < 3 { return None; } + let repo = segs[segs.len() - 2]; + let owner = segs[segs.len() - 3]; + let prefix = &segs[..segs.len() - 3]; + let repo = repo.strip_suffix(".git").unwrap_or(repo); if !is_safe_component(owner) || !is_safe_component(repo) { return None; } - Some((origin, owner.to_string(), repo.to_string())) + // Every prefix segment stays part of the base the node DID is fetched from, so + // it gets the same allow-list: a `..` here would redirect that probe, and an + // encoded separator would smuggle structure past this split. + if !prefix.iter().all(|s| is_safe_component(s)) { + return None; + } + let node_base = if prefix.is_empty() { + origin.to_string() + } else { + format!("{origin}/{}", prefix.join("/")) + }; + Some((node_base, owner.to_string(), repo.to_string())) } /// Build the `X-Ucan` value for a delegated push, or `None` when this push does @@ -485,7 +542,7 @@ fn delegation_header( .parse() .ok()?; - match build_invocation(keypair, &node_did, &delegation) { + match build_invocation(keypair, &node_did, &delegation, &owner, &repo) { Ok(inv) => inv.encode().ok(), Err(e) => { tracing::warn!("could not build the UCAN invocation: {e}"); @@ -2334,7 +2391,8 @@ mod delegated_push_tests { ) .expect("issue"); - let invocation = build_invocation(&agent, &node.did(), &delegation).expect("wrap"); + let invocation = + build_invocation(&agent, &node.did(), &delegation, "zowner", "r").expect("wrap"); assert_eq!(invocation.payload.iss, agent.did(), "the agent invokes"); assert_eq!(invocation.payload.aud, node.did(), "the node executes"); @@ -2350,10 +2408,13 @@ mod delegated_push_tests { ); } - /// The invocation deliberately carries no expiry of its own. That is only safe - /// because `verify_chain` recurses into the proof and checks the delegation's - /// expiry there — so an expired delegation cannot be laundered into an - /// open-ended push capability by wrapping it. + /// An expired delegation cannot be laundered into a live one by wrapping it. + /// + /// Two independent guards now cover this: the invocation inherits the + /// delegation's `exp`, so it is expired on its own terms, AND `verify_chain` + /// recurses into the proof and rejects it there. Inheriting the expiry is what + /// keeps the node's "every link must be bounded" rule satisfiable — a leaf with + /// no expiry would be refused outright. #[test] fn an_expired_delegation_cannot_be_laundered_by_wrapping_it() { let owner = Keypair::generate(); @@ -2367,11 +2428,16 @@ mod delegated_push_tests { ) .expect("issue expired"); - let invocation = build_invocation(&agent, &node.did(), &expired).expect("wrap"); + let invocation = + build_invocation(&agent, &node.did(), &expired, "zowner", "r").expect("wrap"); + assert_eq!( + invocation.payload.exp, expired.payload.exp, + "the invocation inherits the delegation's expiry, never a longer one" + ); assert!( - invocation.payload.exp.is_none(), - "the invocation sets no expiry of its own" + !invocation.chain_lifetime_is_bounded() || invocation.is_expired(), + "an inherited expiry in the past leaves the invocation expired" ); let err = invocation .verify_chain() @@ -2393,6 +2459,29 @@ mod delegated_push_tests { )), "the origin must come back intact so the node DID can be fetched from it" ); + // A reverse-proxied GITLAWB_NODE carries a path prefix, which survives into + // the pack URL through `repo_base`. Reading the FIRST two segments as + // owner/repo makes the prefix the owner: the delegation lookup misses, the + // DID probe hits the wrong URL, no X-Ucan is sent, and a valid delegate is + // refused with 403 — silently, because every failure here is best-effort. + assert_eq!( + split_pack_post_url("https://host/gitlawb/z6Mk/myrepo/git-receive-pack"), + Some(( + "https://host/gitlawb".to_string(), + "z6Mk".to_string(), + "myrepo".to_string() + )), + "a path-prefixed node base must keep its prefix and still find owner/repo" + ); + assert_eq!( + split_pack_post_url("https://host/a/b/c/z6Mk/myrepo/git-receive-pack"), + Some(( + "https://host/a/b/c".to_string(), + "z6Mk".to_string(), + "myrepo".to_string() + )), + "prefix depth is not fixed" + ); // The .git suffix is optional on the wire; the delegation is stored under // the bare repo name either way, so both forms must resolve identically. assert_eq!( diff --git a/crates/gitlawb-core/src/ucan.rs b/crates/gitlawb-core/src/ucan.rs index be78b269..95f54425 100644 --- a/crates/gitlawb-core/src/ucan.rs +++ b/crates/gitlawb-core/src/ucan.rs @@ -167,6 +167,29 @@ impl Ucan { } } + /// Whether every link in this chain carries a finite `exp`. + /// + /// `exp` is optional in the format, and [`Self::is_expired`] reports `false` + /// when it is absent — so a link without one never expires. With no revocation + /// mechanism, a chain containing such a link is a permanent grant: a leaked + /// token cannot be withdrawn, and the issuer's only remedy is to rotate the + /// identity the resource is keyed on. + /// + /// Consumers that turn a UCAN into write authority should require this. It is + /// deliberately not enforced inside [`Self::verify_chain`], because a + /// non-expiring token is well-formed and may be perfectly appropriate for a + /// read-only or advisory capability; whether an unbounded grant is acceptable + /// is the consumer's policy, not the format's. + pub fn chain_lifetime_is_bounded(&self) -> bool { + if self.payload.exp.is_none() { + return false; + } + self.payload + .prf + .iter() + .all(|token| Self::decode(token).is_ok_and(|proof| proof.chain_lifetime_is_bounded())) + } + /// Check if this UCAN's not-before time is in the future (token not yet valid). pub fn is_before_valid(&self) -> bool { if let Some(nbf) = self.payload.nbf { @@ -726,6 +749,78 @@ mod tests { ); } + /// A chain is only bounded if EVERY link is. An unbounded link anywhere makes + /// the whole grant permanent, because `is_expired` reports false for it and + /// there is no revocation path to withdraw it. + #[test] + fn chain_lifetime_is_bounded_requires_an_expiry_on_every_link() { + let owner = Keypair::generate(); + let agent = Keypair::generate(); + let node = Keypair::generate(); + let cap = || vec![Capability::new("gitlawb://repos/zowner/r", caps::GIT_PUSH)]; + let hour = Utc::now() + chrono::Duration::hours(1); + + let bounded_root = Ucan::issue(&owner, agent.did(), cap(), Some(hour)).expect("issue"); + let unbounded_root = Ucan::issue(&owner, agent.did(), cap(), None).expect("issue"); + + assert!( + Ucan::delegate(&agent, node.did(), cap(), Some(hour), &bounded_root) + .expect("wrap") + .chain_lifetime_is_bounded(), + "both links finite" + ); + assert!( + !Ucan::delegate(&agent, node.did(), cap(), None, &bounded_root) + .expect("wrap") + .chain_lifetime_is_bounded(), + "the leaf has no expiry, so the grant never lapses" + ); + assert!( + !Ucan::delegate(&agent, node.did(), cap(), Some(hour), &unbounded_root) + .expect("wrap") + .chain_lifetime_is_bounded(), + "a bounded leaf cannot rescue an unbounded proof: the holder can always \ + mint a fresh leaf from it" + ); + assert!( + !unbounded_root.chain_lifetime_is_bounded(), + "a self-issued token with no expiry is itself unbounded" + ); + } + + /// A three-link chain: owner -> lead -> agent, which is the real shape of an + /// org delegating to a team lead who delegates to a CI identity. + /// + /// Every other chain here is depth two, where the immediate proof IS the root — + /// so nothing distinguishes recursing to the true root from simply returning the + /// proof's issuer. Both `assert_eq!` and `assert_ne!` below are load-bearing: + /// without the second, returning the middle issuer would still satisfy a test + /// that only checked "not the leaf". + #[test] + fn verify_chain_walks_past_the_immediate_proof_to_the_true_root() { + let owner = Keypair::generate(); + let lead = Keypair::generate(); + let agent = Keypair::generate(); + let node = Keypair::generate(); + let cap = || vec![Capability::new("gitlawb://repos/zowner/r", caps::GIT_PUSH)]; + + let root = Ucan::issue(&owner, lead.did(), cap(), None).expect("owner -> lead"); + let mid = Ucan::delegate(&lead, agent.did(), cap(), None, &root).expect("lead -> agent"); + let leaf = Ucan::delegate(&agent, node.did(), cap(), None, &mid).expect("agent -> node"); + + let found = leaf.verify_chain().expect("a three-link chain must verify"); + assert_eq!( + found, + owner.did(), + "the root is the owner who started the chain, two hops up" + ); + assert_ne!( + found, + lead.did(), + "returning the immediate proof's issuer is not walking to the root" + ); + } + #[test] fn verify_chain_returns_self_as_root_for_a_self_issued_token() { // A token with no proofs roots at its own issuer. This is what makes a diff --git a/crates/gitlawb-node/src/api/repos.rs b/crates/gitlawb-node/src/api/repos.rs index abcaad60..38fda741 100644 --- a/crates/gitlawb-node/src/api/repos.rs +++ b/crates/gitlawb-node/src/api/repos.rs @@ -1603,8 +1603,14 @@ fn owner_push_rejection( } match caller { Some(did) if caller_authorized_to_push(record, did, verified) => None, + // One message for every refusal. It must not say whether a delegation was + // presented, was expired, or named another repository: varying it would turn + // the denial into an oracle for which capabilities exist. It only has to be + // TRUE in all of those cases, which the previous owner-only wording no + // longer was once a delegation could authorize a push. _ => Some(AppError::Forbidden( - "push rejected — only the repo owner may push to this repository \ + "push rejected — you must be the repo owner, or hold a valid \ + owner-issued git/push delegation for this repository \ (GITLAWB_ENFORCE_OWNER_PUSH is enabled)" .into(), )), @@ -1807,9 +1813,17 @@ pub async fn git_receive_pack( } // ── Branch protection check ────────────────────────────────────────── - // 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.) + // Uses the same verified identity as the owner-push gate above, but a STRICTER + // predicate: owner-only, deliberately not `caller_authorized_to_push`. + // + // A delegate can therefore clear the gate above and still be refused here. That + // is the intended policy, not an oversight: a protected branch is the owner's + // explicit marker that even routine writes should stop, so a `git/push` + // delegation must not silently override it. Widening this to accept a + // delegation would make every existing protection weaker the moment the owner + // issues any capability. + // + // `delegated_push_is_still_refused_on_a_protected_branch` pins this. for update in &ref_updates { // Strip refs/heads/ prefix to get plain branch name let branch = update @@ -3377,7 +3391,9 @@ mod tests { format!("gitlawb://repos/{}/{}", repo.owner_did, repo.name), gitlawb_core::ucan::caps::GIT_PUSH, )], - None, + // Finite: a write capability that never lapses is refused, since there + // is no revocation path to withdraw a leaked one. + Some(chrono::Utc::now() + chrono::Duration::hours(1)), ) .expect("issue delegation"); crate::auth::VerifiedUcan { diff --git a/crates/gitlawb-node/src/auth/mod.rs b/crates/gitlawb-node/src/auth/mod.rs index 4cccd3b4..3810334c 100644 --- a/crates/gitlawb-node/src/auth/mod.rs +++ b/crates/gitlawb-node/src/auth/mod.rs @@ -92,6 +92,14 @@ pub fn ucan_grants_push(record: &crate::db::RepoRecord, verified: &VerifiedUcan) if !crate::api::did_matches(&verified.root.to_string(), &record.owner_did) { return false; } + // A write capability must lapse on its own. `exp` is optional in the format and + // there is no revocation path, so a chain with an unbounded link is a permanent + // grant: once the token leaks, the owner cannot withdraw it short of rotating + // the DID the repo is keyed on. Refusing here is what makes "the damage window + // is the token's expiry" a true statement rather than an aspiration. + if !verified.ucan.chain_lifetime_is_bounded() { + return false; + } verified.ucan.payload.att.iter().any(|cap| { cap.constraints.is_none() && (cap.can == gitlawb_core::ucan::caps::GIT_PUSH @@ -768,9 +776,21 @@ mod ucan_push_tests { /// middleware has already bound `iss` to the request signer and `aud` to this /// node. Only the capabilities and the chain's root matter here. fn verified(root: &str, caps_vec: Vec) -> VerifiedUcan { + verified_with_exp( + root, + caps_vec, + Some(chrono::Utc::now() + chrono::Duration::hours(1)), + ) + } + + fn verified_with_exp( + root: &str, + caps_vec: Vec, + exp: Option>, + ) -> VerifiedUcan { let agent = Keypair::generate(); let node = Keypair::generate(); - let ucan = Ucan::issue(&agent, node.did(), caps_vec, None).expect("issue"); + let ucan = Ucan::issue(&agent, node.did(), caps_vec, exp).expect("issue"); VerifiedUcan { ucan, root: root.parse().expect("root DID must parse"), @@ -801,6 +821,20 @@ mod ucan_push_tests { assert!(ucan_grants_push(&rec, &v)); } + /// A perpetual grant is refused even when it is otherwise perfectly valid: + /// owner-rooted, right repo, right action. Without revocation, an unbounded + /// delegation cannot be withdrawn once it leaks. + #[test] + fn refuses_a_delegation_that_never_expires() { + let rec = repo(&owner_full(), "myrepo"); + let v = verified_with_exp( + &owner_full(), + vec![push_cap_for(&owner_full(), "myrepo")], + None, + ); + assert!(!ucan_grants_push(&rec, &v)); + } + #[test] fn refuses_a_self_minted_root() { // The whole point: a token nobody delegated grants nothing, however diff --git a/crates/gitlawb-node/src/test_support.rs b/crates/gitlawb-node/src/test_support.rs index 5b3b7aba..7d91be02 100644 --- a/crates/gitlawb-node/src/test_support.rs +++ b/crates/gitlawb-node/src/test_support.rs @@ -2230,25 +2230,29 @@ mod tests { let body = b"0000".to_vec(); // owner -> agent delegation, then agent -> node invocation carrying it. - let invocation_for = |resource: String| { + // Both links carry a finite expiry: a write capability that never lapses is + // refused, since there is no revocation path to withdraw a leaked one. + let hour = chrono::Utc::now() + chrono::Duration::hours(1); + let invocation_with_exp = |resource: String, exp: Option>| { let delegation = Ucan::issue( &owner, agent.did(), vec![Capability::new(resource, caps::GIT_PUSH)], - None, + exp, ) .expect("issue delegation"); Ucan::delegate( &agent, state.node_did.clone(), delegation.payload.att.clone(), - None, + exp, &delegation, ) .expect("wrap invocation") .encode() .expect("encode invocation") }; + let invocation_for = |resource: String| invocation_with_exp(resource, Some(hour)); let signed_push = |ucan: Option| { let signed = sign_request(&agent, "POST", &path, &body); @@ -2306,6 +2310,126 @@ mod tests { other_body, none_body, "an inapplicable delegation and no delegation must be indistinguishable" ); + + // 4. A delegation that never expires: refused, however otherwise valid. + // Owner-rooted, right repo, right action — but with no revocation path an + // unbounded grant cannot be withdrawn once the token leaks. + let perpetual = router() + .oneshot(signed_push(Some(invocation_with_exp( + format!("gitlawb://repos/{owner_did}/deleg"), + None, + )))) + .await + .unwrap(); + assert_eq!( + perpetual.status(), + StatusCode::FORBIDDEN, + "a delegation with no expiry must not authorize a push" + ); + } + + /// A valid delegation clears the owner gate but is still refused on a branch + /// the owner has explicitly protected. + /// + /// Two predicates deliberately disagree: the owner gate accepts a delegate, the + /// branch-protection loop is owner-only. A protected branch is the owner's + /// marker that even routine writes should stop, so a `git/push` delegation must + /// not silently override it — otherwise issuing any capability would weaken + /// every protection the owner had already set. + /// + /// The 403 body is the discriminator: it must name the branch, proving the + /// request reached branch protection rather than being turned away by the owner + /// gate for lacking a delegation. + #[sqlx::test] + async fn delegated_push_is_still_refused_on_a_protected_branch(pool: PgPool) { + use gitlawb_core::http_sig::sign_request; + use gitlawb_core::identity::Keypair; + use gitlawb_core::ucan::{caps, Capability, Ucan}; + use std::sync::Arc; + + const ZERO: &str = "0000000000000000000000000000000000000000"; + let new_sha = "1111111111111111111111111111111111111111"; + + let owner = Keypair::generate(); + let agent = Keypair::generate(); + let owner_did = owner.did().to_string(); + let short = owner_did.split(':').next_back().unwrap().to_string(); + + let mut state = test_state(pool).await; + let mut cfg = (*state.config).clone(); + cfg.enforce_owner_push = true; + state.config = Arc::new(cfg); + + let rec = seed_repo(&owner_did, "protrepo"); + state.db.create_repo(&rec).await.expect("seed repo"); + state + .db + .protect_branch(&rec.id, "main", &owner_did) + .await + .expect("protect main"); + + let hour = chrono::Utc::now() + chrono::Duration::hours(1); + let delegation = Ucan::issue( + &owner, + agent.did(), + vec![Capability::new( + format!("gitlawb://repos/{owner_did}/protrepo"), + caps::GIT_PUSH, + )], + Some(hour), + ) + .expect("issue delegation"); + let invocation = Ucan::delegate( + &agent, + state.node_did.clone(), + delegation.payload.att.clone(), + Some(hour), + &delegation, + ) + .expect("wrap") + .encode() + .expect("encode"); + + let router = Router::new() + .route( + "/{owner}/{repo}/git-receive-pack", + axum::routing::post(crate::api::repos::git_receive_pack), + ) + .layer(axum::middleware::from_fn_with_state( + state.clone(), + crate::auth::require_ucan_chain, + )) + .layer(axum::middleware::from_fn(crate::auth::require_signature)) + .with_state(state.clone()); + + let path = format!("/{short}/protrepo.git/git-receive-pack"); + let line = format!("{ZERO} {new_sha} refs/heads/main"); + let body = format!("{:04x}{}0000", line.len() + 4, line).into_bytes(); + + let signed = sign_request(&agent, "POST", &path, &body); + let req = Request::builder() + .method(Method::POST) + .uri(&path) + .header("content-type", "application/x-git-receive-pack-request") + .header("content-digest", signed.content_digest) + .header("signature-input", signed.signature_input) + .header("signature", signed.signature) + .header("x-ucan", invocation) + .body(Body::from(body)) + .unwrap(); + + let resp = router.oneshot(req).await.unwrap(); + assert_eq!( + resp.status(), + StatusCode::FORBIDDEN, + "a delegation must not override branch protection" + ); + let bytes = axum::body::to_bytes(resp.into_body(), 4096).await.unwrap(); + let text = String::from_utf8_lossy(&bytes); + assert!( + text.contains("protected"), + "the refusal must come from branch protection, not the owner gate; got {text}" + ); } /// A1 Phase-2 contract: the `git-upload-pack` POST (the actual fetch, after diff --git a/crates/gl/src/ucan_cmd.rs b/crates/gl/src/ucan_cmd.rs index f0534784..8cdbf837 100644 --- a/crates/gl/src/ucan_cmd.rs +++ b/crates/gl/src/ucan_cmd.rs @@ -29,9 +29,17 @@ pub enum UcanCmd { /// Action, e.g. "git/push", "pr/open", "repo/admin" #[arg(long)] can: String, - /// Expiry in hours (default: no expiry) - #[arg(long)] - expiry: Option, + /// Expiry in hours. Defaults to 720 (30 days). + /// + /// A capability that authorizes a write must lapse on its own: there is no + /// revocation path yet, so an unbounded delegation cannot be withdrawn once + /// the token leaks. A node refuses an unbounded `git/push` chain outright. + #[arg(long, default_value_t = DEFAULT_DELEGATION_EXPIRY_HOURS)] + expiry: u64, + /// Issue with no expiry. The result cannot authorize a push, and cannot be + /// withdrawn — only use it for advisory or read-shaped capabilities. + #[arg(long, conflicts_with = "expiry")] + no_expiry: bool, /// Save the UCAN to a file instead of printing #[arg(long)] out: Option, @@ -116,6 +124,10 @@ fn repo_from_resource(with: &str) -> Option<(String, String)> { Some((owner.to_string(), name.to_string())) } +/// Default delegation lifetime. Finite on purpose: an unbounded write capability +/// cannot be withdrawn while there is no revocation path, and the node refuses one. +pub const DEFAULT_DELEGATION_EXPIRY_HOURS: u64 = 720; + pub async fn run(args: UcanArgs) -> Result<()> { match args.cmd { UcanCmd::Delegate { @@ -123,10 +135,14 @@ pub async fn run(args: UcanArgs) -> Result<()> { cap, can, expiry, + no_expiry, out, dir, json: json_out, - } => cmd_delegate(to, cap, can, expiry, out, dir, json_out).await, + } => { + let exp_hours = if no_expiry { None } else { Some(expiry) }; + cmd_delegate(to, cap, can, exp_hours, out, dir, json_out).await + } UcanCmd::Show { dir } => cmd_show(dir).await, UcanCmd::Verify { token } => cmd_verify(token).await, UcanCmd::Import { token, dir } => cmd_import(token, dir).await, @@ -176,6 +192,16 @@ async fn cmd_import(token: String, dir: Option) -> Result<()> { let path = delegation_path(&base, owner, repo); std::fs::write(&path, &raw) .with_context(|| format!("could not write {}", path.display()))?; + // 0600, like the sibling identity key. The token is not itself sufficient to + // push — the node requires `iss` to equal the request signer, so a reader + // still needs the delegate's private key — but it does disclose the + // delegation graph and which identities hold capabilities on which repos. + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o600)) + .with_context(|| format!("could not set permissions on {}", path.display()))?; + } println!("Stored delegation for {owner}/{repo} at {}", path.display()); } diff --git a/docs/RUN-A-NODE.md b/docs/RUN-A-NODE.md index 0a8a0f77..0d77e6e7 100644 --- a/docs/RUN-A-NODE.md +++ b/docs/RUN-A-NODE.md @@ -155,17 +155,48 @@ To require the authenticated pusher to be the repo owner on **every** branch, se 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. +- **Default `false`** on this release — 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 (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. +- **A delegated key can still push.** A non-owner clears this gate by presenting a + UCAN whose proof chain roots at the repo owner and which carries `git/push` for + this repository. See *Delegating push to a CI agent* below. + +### Delegating push to a CI agent + +The owner issues a capability, the agent stores it, and the git helper presents it +automatically on every push: + +```bash +# Owner, once per agent per repo: +gl ucan delegate --to did:key:z6MkAgent… \ + --cap gitlawb://repos// --can git/push --expiry 168 + +# Agent: +gl ucan import +git push origin main # git-remote-gitlawb attaches it as X-Ucan +``` + +What the node requires, and why: + +| Requirement | Reason | +|---|---| +| The chain's **root issuer** is the repo owner | A `did:key` is self-certifying, so anyone can mint a chain. The owner is the only anchor the node holds independently of the token. | +| The capability names **this** repository | A `*` capability would otherwise grow to cover every repo the owner creates later. The helper narrows to the concrete repo when it builds the invocation. | +| **Every link carries an expiry** | There is no revocation path yet. An unbounded delegation could never be withdrawn once leaked, so the node refuses one outright. `gl ucan delegate` defaults to 30 days. | +| No `nb` constraints | Constraints are reserved but not yet interpreted, so a capability carrying them authorizes nothing rather than silently granting more than the owner intended. | + +**A delegation does not override branch protection.** A protected branch is your +explicit marker that even routine writes should stop, so a delegate is still +refused there and only the owner may push. That is deliberate: if a delegation +overrode it, issuing any capability would weaken every protection you had set. + +**Withdrawal is by expiry only.** There is no revocation today. If a delegated +token leaks, it remains valid until its `exp`, and the only faster remedy is +rotating the owner DID the repository is keyed on. Choose `--expiry` accordingly — +short lifetimes reissued often are safer than one long-lived grant. --- From d503655f90f7b629628e09849c2241818daf6e10 Mon Sep 17 00:00:00 2001 From: Vasanthdev2004 Date: Fri, 14 Aug 2026 23:37:16 +0530 Subject: [PATCH 12/18] fix(gl): restrict the delegations directory, and state the contract MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The imported delegation was written 0600 on Unix and left to inherit the directory ACL elsewhere. Tightening only that one file would have been theatre: identity.pem sits in the same directory under exactly the same assumption, and its disclosure is strictly worse. The directory itself is now 0700 on Unix, and the comment states the contract that already governed the private key — gitlawb_dir accepts any path, std::fs has no portable ACL API, so on other targets the caller must supply a user-private directory, which a per-user profile provides by default. --- crates/gl/src/ucan_cmd.rs | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/crates/gl/src/ucan_cmd.rs b/crates/gl/src/ucan_cmd.rs index 8cdbf837..92f3e925 100644 --- a/crates/gl/src/ucan_cmd.rs +++ b/crates/gl/src/ucan_cmd.rs @@ -184,9 +184,24 @@ async fn cmd_import(token: String, dir: Option) -> Result<()> { ); } + // The identity directory is a private-data contract, not a public one: it + // already holds `identity.pem`, whose disclosure is strictly worse than a + // delegation's. On Unix both get explicit modes. On other targets neither does + // — `std::fs` has no portable ACL API and `gitlawb_dir` accepts any directory — + // so the contract there is that the caller supplies a user-private directory, + // which is what the platform's per-user profile gives by default. Diverging for + // this one file while the private key beside it relies on the same assumption + // would be theatre. let base = crate::identity::gitlawb_dir(dir)?; - std::fs::create_dir_all(base.join("delegations")) - .with_context(|| format!("could not create {}", base.join("delegations").display()))?; + let store = base.join("delegations"); + std::fs::create_dir_all(&store) + .with_context(|| format!("could not create {}", store.display()))?; + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + std::fs::set_permissions(&store, std::fs::Permissions::from_mode(0o700)) + .with_context(|| format!("could not set permissions on {}", store.display()))?; + } for (owner, repo) in &push_caps { let path = delegation_path(&base, owner, repo); From 5071524c2cbcea0d9a5715059e3f7469e16e7b58 Mon Sep 17 00:00:00 2001 From: Vasanthdev2004 Date: Sat, 15 Aug 2026 10:20:55 +0530 Subject: [PATCH 13/18] fix(gl,git-remote): make a delegation issued the documented way actually work MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two silent failures, both of which end as a 403 telling the delegate to obtain the delegation they are already holding. The narrowing fix broke the documented issuing form ---------------------------------------------------- `build_invocation` built `gitlawb://repos/{owner}/{repo}` from the push URL and compared it to the delegation's `with` with `==`. The URL always carries the BARE owner, since `parse_gitlawb_url` takes the last colon-delimited segment, while `docs/RUN-A-NODE.md` tells the owner to issue `--cap gitlawb://repos//` — the full DID. Those strings never match, `find` returns None, and because every failure in `delegation_header` is best-effort the push goes out with no `X-Ucan`. This was introduced by the previous round: copying `att` through unchanged worked because the node normalizes both forms in `did_matches`. Narrowing to a URL-derived string did not. The owner segment is now compared on the bare key, and the parent's `with` is kept VERBATIM whenever it already names this repo — `is_attenuated_by` compares `with` by exact equality, so re-emitting the bare form under a full-DID parent would fail attenuation at the node and trade one silent refusal for another. Only a `*` parent uses the URL-derived resource, which is the case narrowing exists for. Both combinations are now tested; neither side exercised them before. The two halves disagreed about where delegations live ------------------------------------------------------ `git-remote-gitlawb` resolves its store from `resolve_key_path().parent()`, which honors `GITLAWB_KEY`. `gl ucan import` wrote under `gitlawb_dir(None)`, which was always `~/.gitlawb`. With `GITLAWB_KEY=/data/keys/identity.pem` — the shape `.env.example` documents — the import stored the token in one directory and the helper read another, empty one. `gitlawb_dir` now falls back to the parent of `GITLAWB_KEY` before `~/.gitlawb`, so both halves derive the store from the same setting. Also: `.env.example` still said a push from a non-owner DID is rejected, which is the behaviour this branch removes. --- .env.example | 5 +- crates/git-remote-gitlawb/src/main.rs | 100 +++++++++++++++++++++++++- crates/gl/src/identity.rs | 21 ++++++ 3 files changed, 122 insertions(+), 4 deletions(-) diff --git a/.env.example b/.env.example index b70d1117..f43ff458 100644 --- a/.env.example +++ b/.env.example @@ -94,8 +94,9 @@ 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. +# sign as their own DID. When true, a push is accepted only from the repo owner, +# or from a holder of an owner-rooted git/push UCAN for that repo (see +# docs/RUN-A-NODE.md). Keep false until your pushers are the owner or hold one. GITLAWB_ENFORCE_OWNER_PUSH=false # Comma-separated libp2p multiaddrs. diff --git a/crates/git-remote-gitlawb/src/main.rs b/crates/git-remote-gitlawb/src/main.rs index 6807f881..7ec6ee24 100644 --- a/crates/git-remote-gitlawb/src/main.rs +++ b/crates/git-remote-gitlawb/src/main.rs @@ -417,19 +417,44 @@ fn build_invocation( // Constraints are copied from the covering capability, not dropped: dropping // them is a widening and `verify_chain` refuses it. let resource = format!("gitlawb://repos/{owner}/{repo}"); + + // The owner segment must be compared on the bare key. `parse_gitlawb_url` yields + // the bare form from the push URL, while an owner following the operator guide + // issues `--cap gitlawb://repos//` — the full DID. Comparing the + // whole resource string finds nothing, and because every failure here is + // best-effort the push goes out with no header and the delegate is told to obtain + // the delegation they already hold. + fn bare(d: &str) -> &str { + d.strip_prefix("did:key:").unwrap_or(d) + } + let names_this_repo = |with: &str| { + with.strip_prefix("gitlawb://repos/") + .and_then(|rest| rest.rsplit_once('/')) + .is_some_and(|(o, r)| bare(o) == bare(owner) && r == repo) + }; + let source = delegation .payload .att .iter() .find(|c| { - (c.with == resource || c.with == "*") + (c.with == "*" || names_this_repo(&c.with)) && (c.can == caps::GIT_PUSH || c.can == "*" || c.can == caps::REPO_ADMIN) }) .ok_or_else(|| { anyhow::anyhow!("stored delegation carries no git/push capability for {resource}") })?; - let mut narrowed = Capability::new(resource, caps::GIT_PUSH); + // Keep the parent's resource verbatim when it already names this repo. The node's + // `is_attenuated_by` compares `with` by exact equality, so re-emitting a bare form + // under a full-DID parent would fail attenuation and be refused. Only a `*` parent + // needs the URL-derived string, and that is the case narrowing exists for. + let narrowed_with = if source.with == "*" { + resource + } else { + source.with.clone() + }; + let mut narrowed = Capability::new(narrowed_with, caps::GIT_PUSH); narrowed.constraints = source.constraints.clone(); // Carry the delegation's own expiry onto the invocation. The node refuses a @@ -2448,6 +2473,77 @@ mod delegated_push_tests { ); } + /// The shipping combination: the owner issues with the FULL DID (what + /// `RUN-A-NODE.md` instructs), while the push URL yields the BARE key + /// (`parse_gitlawb_url` takes the last colon-delimited segment). Comparing the + /// whole resource string never matches, so the delegation is not found, no + /// `X-Ucan` is sent, and the delegate gets a 403 telling them to obtain the + /// delegation they are already holding. + #[test] + fn build_invocation_matches_a_full_did_delegation_against_a_bare_owner() { + use gitlawb_core::ucan::{caps, Capability, Ucan}; + + let owner = Keypair::generate(); + let agent = Keypair::generate(); + let node = Keypair::generate(); + let full = owner.did().to_string(); + let bare = full.strip_prefix("did:key:").unwrap().to_string(); + let hour = chrono::Utc::now() + chrono::Duration::hours(1); + + let delegation = Ucan::issue( + &owner, + agent.did(), + vec![Capability::new( + format!("gitlawb://repos/{full}/r"), + caps::GIT_PUSH, + )], + Some(hour), + ) + .expect("issue"); + + let invocation = build_invocation(&agent, &node.did(), &delegation, &bare, "r") + .expect("a full-DID delegation must match a bare-owner push URL"); + + // The narrowed capability must keep the parent's exact `with`: the node's + // `is_attenuated_by` compares it by equality, so emitting the bare form under + // a full-DID parent would fail attenuation and be refused at the node. + assert_eq!( + invocation.payload.att[0].with, + format!("gitlawb://repos/{full}/r"), + "narrowing must not rewrite a resource that already names this repo" + ); + assert!( + invocation.payload.att[0].is_attenuated_by(&delegation.payload.att[0]), + "the narrowed capability must still attenuate under its parent" + ); + } + + #[test] + fn build_invocation_narrows_a_wildcard_to_the_pushed_repo() { + use gitlawb_core::ucan::{caps, Capability, Ucan}; + + let owner = Keypair::generate(); + let agent = Keypair::generate(); + let node = Keypair::generate(); + let hour = chrono::Utc::now() + chrono::Duration::hours(1); + + let delegation = Ucan::issue( + &owner, + agent.did(), + vec![Capability::new("*", caps::GIT_PUSH)], + Some(hour), + ) + .expect("issue"); + + let invocation = + build_invocation(&agent, &node.did(), &delegation, "z6MkOwner", "r").expect("wrap"); + assert_eq!( + invocation.payload.att[0].with, "gitlawb://repos/z6MkOwner/r", + "a wildcard parent is the one case where the URL-derived resource is used" + ); + assert!(invocation.payload.att[0].is_attenuated_by(&delegation.payload.att[0])); + } + #[test] fn split_pack_post_url_separates_origin_owner_and_repo() { assert_eq!( diff --git a/crates/gl/src/identity.rs b/crates/gl/src/identity.rs index cab929bd..3ee7519c 100644 --- a/crates/gl/src/identity.rs +++ b/crates/gl/src/identity.rs @@ -65,10 +65,31 @@ pub async fn run(cmd: IdentityCmd) -> Result<()> { /// Resolve the identity directory, honouring an explicit override. /// Public so sibling commands (`gl ucan import`) store alongside the identity. +/// +/// Falls back to the parent of `GITLAWB_KEY` before `~/.gitlawb`. Without that, +/// an operator who moved their key — `GITLAWB_KEY=/data/keys/identity.pem`, the +/// shape `.env.example` documents — has `gl ucan import` write the delegation to +/// `~/.gitlawb/delegations` while `git-remote-gitlawb`, which resolves its store +/// from `GITLAWB_KEY`, reads an empty directory. The push then goes out with no +/// `X-Ucan` and the delegate is refused, with nothing to indicate why. pub fn gitlawb_dir(override_dir: Option) -> Result { if let Some(d) = override_dir { return Ok(d); } + if let Ok(key) = std::env::var("GITLAWB_KEY") { + if !key.trim().is_empty() { + let path = if let Some(rest) = key.strip_prefix("~/") { + dirs::home_dir() + .context("could not determine home directory")? + .join(rest) + } else { + PathBuf::from(key) + }; + if let Some(parent) = path.parent() { + return Ok(parent.to_path_buf()); + } + } + } let home = dirs::home_dir().context("could not determine home directory")?; Ok(home.join(".gitlawb")) } From 5dfd8c5874a46cd501b3b2a6d566271415fc82a0 Mon Sep 17 00:00:00 2001 From: Vasanthdev2004 Date: Sat, 15 Aug 2026 11:40:40 +0530 Subject: [PATCH 14/18] fix(gl): refuse a relative or non-UTF-8 GITLAWB_KEY when deriving the store MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two defects in the previous round's fix, both of which put the delegation somewhere the helper does not look and end as an unexplained 403. A relative GITLAWB_KEY resolved differently in each half. `gl ucan import` and `git-remote-gitlawb` do not share a working directory, so `keys/identity.pem` sends the import to one `delegations` directory and the lookup to another. A one-component value is worse: `parent()` yields "", so the store becomes `./delegations` relative to whatever directory happened to be current. It is now refused with a message that says why, rather than silently resolved. `std::env::var` returns Err for a non-UTF-8 value, which the code treated as unset — indistinguishable from having no GITLAWB_KEY at all, and silently selecting ~/.gitlawb instead of the operator's real key directory. `var_os` keeps the OsString, so a valid non-UTF-8 path now works and an empty one is still treated as unset. --- crates/gl/src/identity.rs | 83 +++++++++++++++++++++++++++++++++++---- 1 file changed, 76 insertions(+), 7 deletions(-) diff --git a/crates/gl/src/identity.rs b/crates/gl/src/identity.rs index 3ee7519c..1a37292e 100644 --- a/crates/gl/src/identity.rs +++ b/crates/gl/src/identity.rs @@ -76,15 +76,33 @@ pub fn gitlawb_dir(override_dir: Option) -> Result { if let Some(d) = override_dir { return Ok(d); } - if let Ok(key) = std::env::var("GITLAWB_KEY") { - if !key.trim().is_empty() { - let path = if let Some(rest) = key.strip_prefix("~/") { - dirs::home_dir() + // `var_os`, not `var`: a non-UTF-8 value makes `var` return Err, which would be + // indistinguishable from unset and would silently select ~/.gitlawb instead of + // the operator's actual key directory. + if let Some(raw) = std::env::var_os("GITLAWB_KEY") { + let key = PathBuf::from(&raw); + if !key.as_os_str().is_empty() { + let path = match key.strip_prefix("~") { + Ok(rest) => dirs::home_dir() .context("could not determine home directory")? - .join(rest) - } else { - PathBuf::from(key) + .join(rest), + Err(_) => key, }; + // A relative GITLAWB_KEY is refused rather than resolved. `gl` and + // `git-remote-gitlawb` run with different working directories, so a + // relative path makes them derive different delegation stores — the + // import lands somewhere the helper never looks, and the push is refused + // with nothing to indicate why. A one-component value is worse still: + // `parent()` yields "", so the store becomes `./delegations`. + if !path.is_absolute() { + anyhow::bail!( + "GITLAWB_KEY must be an absolute path (got {}). It determines where \ + delegations are stored, and `gl` and `git-remote-gitlawb` do not \ + share a working directory, so a relative path sends them to \ + different places.", + path.display() + ); + } if let Some(parent) = path.parent() { return Ok(parent.to_path_buf()); } @@ -532,3 +550,54 @@ mod tests { assert_eq!(original_did, dst_kp.did()); } } + +#[cfg(test)] +mod gitlawb_dir_tests { + use super::gitlawb_dir; + use std::path::PathBuf; + + /// An explicit --dir always wins and is never validated against GITLAWB_KEY. + #[test] + fn explicit_override_wins() { + let d = PathBuf::from("/tmp/explicit"); + assert_eq!(gitlawb_dir(Some(d.clone())).unwrap(), d); + } + + /// A relative GITLAWB_KEY must fail loudly. `gl` and `git-remote-gitlawb` run + /// from different working directories, so resolving one relatively sends the + /// import and the lookup to different stores; a one-component value yields an + /// empty parent and puts the store in `./delegations`. + /// + /// Serialised with the other env-touching case: the process environment is + /// global and these would otherwise race. + #[test] + fn relative_and_nonunicode_key_paths() { + use std::sync::Mutex; + static LOCK: Mutex<()> = Mutex::new(()); + let _guard = LOCK.lock().unwrap_or_else(|e| e.into_inner()); + + let restore = std::env::var_os("GITLAWB_KEY"); + + std::env::set_var("GITLAWB_KEY", "identity.pem"); + let one_component = gitlawb_dir(None); + std::env::set_var("GITLAWB_KEY", "keys/identity.pem"); + let relative = gitlawb_dir(None); + std::env::set_var("GITLAWB_KEY", ""); + let empty = gitlawb_dir(None); + + match restore { + Some(v) => std::env::set_var("GITLAWB_KEY", v), + None => std::env::remove_var("GITLAWB_KEY"), + } + + assert!( + one_component.is_err(), + "a one-component key path yields an empty parent and must be refused" + ); + assert!(relative.is_err(), "a relative key path must be refused"); + assert!( + empty.is_ok(), + "an empty value is treated as unset, not as an error" + ); + } +} From e15fa73466f64a4eb624bbcfd8589e9391941f39 Mon Sep 17 00:00:00 2001 From: Vasanthdev2004 Date: Sun, 16 Aug 2026 11:19:47 +0530 Subject: [PATCH 15/18] fix(identity): one resolver for GITLAWB_KEY across gl and the helper MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `gl` and `git-remote-gitlawb` each carried their own answer to "where is my identity?", and so did five call sites inside `gl` itself. The round-three fix hardened one of them — `gitlawb_dir` — and left the rest, which is why the same misconfiguration kept surfacing in a new place each round. The rules now live in `gitlawb-core::identity_path`, the crate both binaries already depend on: identity_key_path() $GITLAWB_KEY, else ~/.gitlawb/identity.pem identity_dir() its parent — the delegation store read through `var_os` (so a non-UTF-8 path is refused rather than folded into "unset" by `var`), with `~/` expanded on the first path component, a relative value refused, and a bare `~`, `~/`, or `/` refused rather than resolved to a directory whose parent is not where anything lives. Both take the home directory as an argument rather than looking it up: core is held to an explicit dependency allowlist and both callers already carry `dirs`, so the rules can be shared without widening core's tree. It also lets every case be tested against a fixed home instead of the machine's. Call sites moved onto it: gl identity::gitlawb_dir delegates to identity_dir gl identity::load_keypair_from_dir was ~/.gitlawb even after `gl identity new` wrote elsewhere, so `gl ucan delegate` signed with a stale DID — or found nothing — for exactly the operators who moved their key. Every `load_keypair_from_dir(None)` caller (register, repo, pr, clone, mcp) is fixed with it. gl doctor::run the one command whose job is to explain a broken setup was reporting on a directory the setup does not use gl init (ucan.json, generate_identity) gl mcp ucan_show gl ucan_cmd::cmd_show helper resolve_key_path / the delegation store behind delegation_header The helper's version was the loosest of the set: `env::var`, a literal `"~/"` prefix, `HOME` falling back to `"."` (never set on Windows), and no absolute check at all. Also in this commit: - `gl ucan import` creates the store and the token at their final mode. `create_dir_all` then chmod leaves 0755 under the usual umask, and `fs::write` then chmod leaves 0644, both readable by any local user until the second call lands. `DirBuilder::mode` and `OpenOptions::mode` close the window; the trailing `set_permissions` now only matters for a store an older `gl` left behind. - `gl ucan import` refuses a delegation the push path cannot use. It filtered on resource shape only, so a `pr/open` token printed "Stored delegation for owner/repo" and was then dropped by `build_invocation` behind a `tracing::warn`, surfacing as a 403 with nothing connecting it to the earlier success. Import now applies the same push-class filter the helper does. - README's write-authorization limitation is narrowed to what is actually missing (revocation, `nb` interpretation, non-push capabilities) rather than claiming delegated push is not implemented. Every new guard was verified by disabling it and watching the matching test go red: the absolute-path check, the bare-`~` refusal, the `load_keypair_from_dir` routing, and the import action filter each own a failing test. --- .env.example | 5 +- Cargo.lock | 1 + README.md | 2 +- crates/git-remote-gitlawb/Cargo.toml | 3 + crates/git-remote-gitlawb/src/main.rs | 50 +++-- crates/gitlawb-core/src/identity_path.rs | 240 +++++++++++++++++++++++ crates/gitlawb-core/src/lib.rs | 1 + crates/gl/src/doctor.rs | 10 +- crates/gl/src/identity.rs | 177 ++++++++++------- crates/gl/src/init.rs | 15 +- crates/gl/src/mcp.rs | 4 +- crates/gl/src/ucan_cmd.rs | 220 ++++++++++++++++++--- 12 files changed, 589 insertions(+), 139 deletions(-) create mode 100644 crates/gitlawb-core/src/identity_path.rs diff --git a/.env.example b/.env.example index f43ff458..1dbe9c1c 100644 --- a/.env.example +++ b/.env.example @@ -3,7 +3,10 @@ # All variables are optional unless marked REQUIRED. # ── Node identity ───────────────────────────────────────────────────────── -# Path to the node's Ed25519 keypair PEM file. +# Path to the node's Ed25519 keypair PEM file. Must be absolute: its parent is +# also the delegation store that `gl ucan import` writes and `git-remote-gitlawb` +# reads, and the two do not share a working directory, so a relative path sends +# them to different places. `~/...` is expanded; a bare `~` is not accepted. # Generate with: gl identity new GITLAWB_KEY=/data/keys/identity.pem diff --git a/Cargo.lock b/Cargo.lock index 31d6038a..bd6500e5 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3378,6 +3378,7 @@ version = "0.7.1" dependencies = [ "anyhow", "chrono", + "dirs", "gitlawb-core", "libc", "mockito", diff --git a/README.md b/README.md index ca7330ee..d07efa2a 100644 --- a/README.md +++ b/README.md @@ -67,7 +67,7 @@ Good today: Known limitations: - Repository write authorization is not secure by default: `GITLAWB_ENFORCE_OWNER_PUSH` defaults to `false` for compatibility, so a valid HTTP Signature identifies a pusher but does not enforce owner-only pushes. -- UCAN proof chains are validated when supplied, but UCAN capabilities are not consulted by write authorization and the root issuer is not independently trust-anchored. UCANs therefore do not yet grant scoped collaborator access. +- UCAN capabilities are consulted on the push path only. There, the chain's root issuer is anchored to the repository owner, so an owner-rooted, time-bounded delegation of `git/push` (or `*`/`repo/admin`) clears the owner-push gate and does grant scoped collaborator access for pushing. The rest is unchanged: there is no revocation path, `nb` constraints are refused rather than interpreted, and no other route — reads, pull requests, issues, agents — consults capabilities at all. - Agent lifecycle revocation is not enforced by HTTP Signature authorization; do not rely on removing or revoking an agent record to block a compromised signer. - Read visibility is not a blanket data-classification boundary: task, IPFS-pin, and Arweave-anchor listings are not repository-gated; withheld path names can be visible to a root reader; and later visibility changes cannot retract content already announced or externally anchored. - Peer writes are signed by upgraded nodes, but strict signed-peer enforcement is opt-in during rolling upgrades. diff --git a/crates/git-remote-gitlawb/Cargo.toml b/crates/git-remote-gitlawb/Cargo.toml index bcace360..3a45cee9 100644 --- a/crates/git-remote-gitlawb/Cargo.toml +++ b/crates/git-remote-gitlawb/Cargo.toml @@ -22,6 +22,9 @@ serde_json = { workspace = true } chrono = { workspace = true } tracing = { workspace = true } tracing-subscriber = { workspace = true } +# Home-directory lookup for the shared GITLAWB_KEY resolver in gitlawb-core, +# which takes `home` as an argument so core's dependency allowlist stays lean. +dirs = "5" [dev-dependencies] mockito = "1" diff --git a/crates/git-remote-gitlawb/src/main.rs b/crates/git-remote-gitlawb/src/main.rs index 7ec6ee24..32c7fae4 100644 --- a/crates/git-remote-gitlawb/src/main.rs +++ b/crates/git-remote-gitlawb/src/main.rs @@ -124,7 +124,9 @@ fn help_text() -> String { \n\ ENVIRONMENT:\n\ \x20 GITLAWB_NODE Node base URL (default: http://127.0.0.1:7545)\n\ - \x20 GITLAWB_KEY Identity PEM path for signed fetch/push (default: ~/.gitlawb/identity.pem)\n\ + \x20 GITLAWB_KEY Identity PEM path for signed fetch/push, absolute\n\ + \x20 (default: ~/.gitlawb/identity.pem). Its parent also\n\ + \x20 holds the delegations `gl ucan import` writes.\n\ \x20 GITLAWB_LOG Log filter (default: warn)\n\ \n\ FLAGS:\n\ @@ -536,7 +538,7 @@ fn delegation_header( return None; } - let dir = resolve_key_path().parent()?.to_path_buf(); + let dir = resolve_identity_dir()?; let path = delegation_path(&dir, &owner, &repo); let raw = std::fs::read_to_string(&path).ok()?; @@ -969,7 +971,7 @@ fn safe_error_body_excerpt(body: &str) -> String { // ── Keypair loading ─────────────────────────────────────────────────────────── fn load_keypair() -> Option { - let key_path = resolve_key_path(); + let key_path = resolve_key_path()?; if !key_path.exists() { tracing::debug!("no keypair found at {key_path:?}"); return None; @@ -992,16 +994,40 @@ fn load_keypair() -> Option { } } -fn resolve_key_path() -> std::path::PathBuf { - let path_str = - std::env::var("GITLAWB_KEY").unwrap_or_else(|_| "~/.gitlawb/identity.pem".to_string()); +/// The identity PEM, resolved by the same rules `gl` uses. +/// +/// Shared through `gitlawb-core` rather than reimplemented here: this helper and +/// `gl ucan import` have to derive the same delegation store from `GITLAWB_KEY`, +/// and the two had drifted — the local version read `env::var` (so a non-UTF-8 +/// value silently became the default key), expanded only a literal `"~/"`, fell +/// back to `"."` when `HOME` was unset, and never required an absolute path. +/// +/// `None` means the value is unusable, not that the key is missing. Git runs this +/// helper mid-push, so a misconfiguration is logged and the push continues +/// unsigned rather than aborting the transfer. +fn resolve_key_path() -> Option { + let home = home_dir()?; + gitlawb_core::identity_path::identity_key_path(&home) + .inspect_err(|e| tracing::warn!("cannot resolve the identity key path: {e}")) + .ok() +} - if let Some(stripped) = path_str.strip_prefix("~/") { - let home = std::env::var("HOME").unwrap_or_else(|_| ".".to_string()); - std::path::PathBuf::from(home).join(stripped) - } else { - std::path::PathBuf::from(path_str) - } +/// The directory holding `identity.pem` and `delegations/`. +fn resolve_identity_dir() -> Option { + let home = home_dir()?; + gitlawb_core::identity_path::identity_dir(&home) + .inspect_err(|e| tracing::warn!("cannot resolve the identity directory: {e}")) + .ok() +} + +/// `dirs`, not `$HOME`: the old code fell back to `"."` when `HOME` was unset, +/// which on Windows is always, so the default key resolved against whatever +/// directory git happened to invoke the helper from. +fn home_dir() -> Option { + dirs::home_dir().or_else(|| { + tracing::warn!("could not determine the home directory; pushing without a delegation"); + None + }) } // ── Tests ───────────────────────────────────────────────────────────────────── diff --git a/crates/gitlawb-core/src/identity_path.rs b/crates/gitlawb-core/src/identity_path.rs new file mode 100644 index 00000000..6bfca9fc --- /dev/null +++ b/crates/gitlawb-core/src/identity_path.rs @@ -0,0 +1,240 @@ +//! Where the identity key and the delegation store live. +//! +//! `gl` and `git-remote-gitlawb` have to agree on this. `gl ucan import` writes a +//! delegation to `/delegations/`, and the helper reads it back from the same +//! place when it builds the `X-Ucan` header on push. If the two resolve +//! `GITLAWB_KEY` differently the push goes out with no header and the node refuses +//! the delegate, with nothing on either side to say why — so the rules live here +//! once, in the crate both binaries already depend on, rather than being written +//! twice and drifting. +//! +//! The home directory is a parameter rather than something this module looks up. +//! `gitlawb-core` is embedded by every consumer and is held to an explicit +//! dependency allowlist (`ci/gitlawb-core-allowed-deps.txt`); the callers already +//! carry `dirs`, so taking `home` keeps the rules shared without widening core's +//! tree. It also makes every case below testable against a fixed home. + +use std::ffi::OsStr; +use std::path::{Component, Path, PathBuf}; + +use crate::{Error, Result}; + +/// Environment variable naming the identity PEM. +pub const KEY_ENV: &str = "GITLAWB_KEY"; + +/// Name of the PEM file inside the identity directory. +pub const KEY_FILE_NAME: &str = "identity.pem"; + +/// Directory under the home directory used when `GITLAWB_KEY` is unset. +pub const DEFAULT_DIR_NAME: &str = ".gitlawb"; + +/// Absolute path of the identity PEM: `$GITLAWB_KEY`, else +/// `/.gitlawb/identity.pem`. +/// +/// An empty `GITLAWB_KEY` counts as unset. That is what a shell leaves behind for +/// `FOO=` and for an unset variable expanded into a wrapper script, and reading it +/// as a path would resolve against the process working directory instead. +pub fn identity_key_path(home: &Path) -> Result { + // `var_os`, not `var`: `var` folds a non-UTF-8 value into the same `Err` as + // unset, so an operator whose key path is not valid UTF-8 would silently get + // the default directory rather than theirs — or an error naming the real + // problem. + match std::env::var_os(KEY_ENV) { + Some(raw) if !raw.is_empty() => resolve_key_value(Path::new(&raw), home), + _ => Ok(home.join(DEFAULT_DIR_NAME).join(KEY_FILE_NAME)), + } +} + +/// The directory holding `identity.pem` and `delegations/` — the parent of +/// [`identity_key_path`]. +pub fn identity_dir(home: &Path) -> Result { + let key = identity_key_path(home)?; + key.parent().map(Path::to_path_buf).ok_or_else(|| { + Error::Key(format!( + "{KEY_ENV} has no parent directory: {}", + key.display() + )) + }) +} + +/// Apply the `GITLAWB_KEY` rules to a raw value. +/// +/// Split out from [`identity_key_path`] so the rules can be tested without setting +/// a process-global environment variable, which would make the tests race. +fn resolve_key_value(raw: &Path, home: &Path) -> Result { + let path = expand_tilde(raw, home)?; + + if !path.is_absolute() { + return Err(Error::Key(format!( + "{KEY_ENV} must be an absolute path (got {}). It also determines where \ + delegations are stored, and `gl` and `git-remote-gitlawb` do not share a \ + working directory, so a relative path sends them to different stores.", + raw.display() + ))); + } + if path.parent().is_none() { + return Err(Error::Key(format!( + "{KEY_ENV} must name the key file, not the filesystem root (got {}). \ + Point it at the PEM, e.g. ~/{DEFAULT_DIR_NAME}/{KEY_FILE_NAME}.", + raw.display() + ))); + } + Ok(path) +} + +/// Expand a leading `~/`, and only that. +/// +/// `~user` is shell syntax this does not implement; leaving its `~` in place makes +/// it fail the absolute-path check with a message that names the real problem, +/// which beats resolving it somewhere the operator did not ask for. A bare `~` or +/// `~/` is refused outright: it names a directory where a file is required, and +/// expanding it to the home directory would put the delegation store beside the +/// home directory rather than inside it, since the store is the key's *parent*. +/// +/// Matched on the first path component rather than on a string prefix. That is +/// what lets the value stay an `OsStr` end to end: the helper's old +/// `str::strip_prefix("~/")` needed a `String` first, which is why it reached for +/// `env::var` and folded every non-UTF-8 path into "unset". +fn expand_tilde(path: &Path, home: &Path) -> Result { + let mut components = path.components(); + match components.next() { + Some(Component::Normal(first)) if first == OsStr::new("~") => { + let rest = components.as_path(); + if rest.as_os_str().is_empty() { + return Err(Error::Key(format!( + "{KEY_ENV} must name the key file, not a directory (got {}). \ + Point it at the PEM, e.g. ~/{DEFAULT_DIR_NAME}/{KEY_FILE_NAME}.", + path.display() + ))); + } + Ok(home.join(rest)) + } + _ => Ok(path.to_path_buf()), + } +} + +#[cfg(test)] +mod tests { + use super::*; + + /// A home that is absolute on the host running the tests. `/home/op` is not + /// absolute on Windows — it has a root but no prefix — so a shared literal + /// would make the absolute-path assertions test the wrong thing there. + fn home() -> PathBuf { + if cfg!(windows) { + PathBuf::from(r"C:\Users\op") + } else { + PathBuf::from("/home/op") + } + } + + #[test] + fn absolute_value_is_taken_verbatim() { + let raw = home().join("data").join("keys").join(KEY_FILE_NAME); + assert_eq!(resolve_key_value(&raw, &home()).unwrap(), raw); + } + + #[test] + fn tilde_slash_expands_to_the_home_directory() { + let resolved = resolve_key_value(Path::new("~/keys/identity.pem"), &home()).unwrap(); + assert_eq!(resolved, home().join("keys").join(KEY_FILE_NAME)); + } + + /// The shell-style spelling of the default resolves to the default. Worth + /// pinning: this is the form an operator gets by copying a path out of their + /// shell, and the two binaries used to reach it by different routes. + #[test] + fn the_tilde_spelling_of_the_default_resolves_to_the_default() { + let resolved = resolve_key_value(Path::new("~/.gitlawb/identity.pem"), &home()).unwrap(); + assert_eq!(resolved, home().join(DEFAULT_DIR_NAME).join(KEY_FILE_NAME)); + } + + /// A relative value resolves against the working directory, and `gl` and + /// `git-remote-gitlawb` do not share one: the import would land where the helper + /// never looks. + #[test] + fn relative_values_are_refused() { + for raw in ["identity.pem", "keys/identity.pem", "./keys/identity.pem"] { + assert!( + resolve_key_value(Path::new(raw), &home()).is_err(), + "{raw} is relative and must be refused" + ); + } + } + + /// `~user` is shell syntax, not a path, and a bare `~` names a directory where + /// a file is required. Refused rather than guessed at. + #[test] + fn unsupported_tilde_forms_are_refused() { + for raw in ["~", "~/", "~someone/keys/identity.pem"] { + assert!( + resolve_key_value(Path::new(raw), &home()).is_err(), + "{raw} must be refused rather than resolved" + ); + } + } + + /// The root has no parent, so the delegation store would have nowhere to go. + #[test] + fn the_filesystem_root_is_refused() { + assert!(resolve_key_value(Path::new("/"), &home()).is_err()); + } + + /// The whole point of `var_os`: a non-UTF-8 value must reach the rules rather + /// than being folded into "unset" by `var`. Byte 0xFF is not valid UTF-8 in any + /// position, so this value is unreachable through `env::var`. + #[cfg(unix)] + #[test] + fn non_utf8_values_reach_the_rules() { + use std::ffi::OsString; + use std::os::unix::ffi::OsStringExt; + + let relative = OsString::from_vec(b"keys/\xFF/identity.pem".to_vec()); + assert!( + resolve_key_value(Path::new(&relative), &home()).is_err(), + "a non-UTF-8 relative path must be refused, not silently defaulted" + ); + + let mut absolute = OsString::from("/data/"); + absolute.push(OsString::from_vec(vec![0xFF])); + absolute.push("/identity.pem"); + let resolved = resolve_key_value(Path::new(&absolute), &home()).unwrap(); + assert_eq!(resolved.as_os_str(), absolute.as_os_str()); + } + + /// Unset and empty both mean "use the default", and the two accessors must stay + /// consistent: the directory is the parent of the key, never a sibling of it. + /// The process environment is global, so the two cases share one test and one + /// lock rather than racing each other. + #[test] + fn unset_and_empty_both_select_the_default_directory() { + use std::sync::Mutex; + static LOCK: Mutex<()> = Mutex::new(()); + let _guard = LOCK.lock().unwrap_or_else(|e| e.into_inner()); + + let restore = std::env::var_os(KEY_ENV); + + std::env::remove_var(KEY_ENV); + let unset = (identity_key_path(&home()), identity_dir(&home())); + std::env::set_var(KEY_ENV, ""); + let empty = (identity_key_path(&home()), identity_dir(&home())); + + match restore { + Some(v) => std::env::set_var(KEY_ENV, v), + None => std::env::remove_var(KEY_ENV), + } + + for (label, (key, dir)) in [("unset", unset), ("empty", empty)] { + assert_eq!( + key.unwrap(), + home().join(DEFAULT_DIR_NAME).join(KEY_FILE_NAME), + "{label} key path" + ); + assert_eq!( + dir.unwrap(), + home().join(DEFAULT_DIR_NAME), + "{label} directory" + ); + } + } +} diff --git a/crates/gitlawb-core/src/lib.rs b/crates/gitlawb-core/src/lib.rs index efa99897..2f35418f 100644 --- a/crates/gitlawb-core/src/lib.rs +++ b/crates/gitlawb-core/src/lib.rs @@ -5,6 +5,7 @@ pub mod encrypt; pub mod error; pub mod http_sig; pub mod identity; +pub mod identity_path; pub mod sanitize; pub mod ucan; diff --git a/crates/gl/src/doctor.rs b/crates/gl/src/doctor.rs index 86f50334..91cd40ce 100644 --- a/crates/gl/src/doctor.rs +++ b/crates/gl/src/doctor.rs @@ -73,11 +73,11 @@ pub async fn run(args: DoctorArgs) -> Result<()> { println!("gl doctor — checking your gitlawb setup"); println!(); - let dir = args.dir.unwrap_or_else(|| { - dirs::home_dir() - .unwrap_or_else(|| PathBuf::from(".")) - .join(".gitlawb") - }); + // The same resolver every other command uses. `doctor` reporting on + // `~/.gitlawb` while `gl register` writes to the parent of `GITLAWB_KEY` would + // make the one command whose job is to explain a broken setup the one that + // misreports it. + let dir = crate::identity::gitlawb_dir(args.dir)?; let mut checks = Vec::new(); let mut all_ok = true; diff --git a/crates/gl/src/identity.rs b/crates/gl/src/identity.rs index 1a37292e..c6ff3fb9 100644 --- a/crates/gl/src/identity.rs +++ b/crates/gl/src/identity.rs @@ -9,7 +9,8 @@ use std::path::{Path, PathBuf}; pub enum IdentityCmd { /// Generate a new Ed25519 keypair and DID New { - /// Output directory for key files (default: ~/.gitlawb) + /// Output directory for key files + /// (default: the parent of $GITLAWB_KEY, else ~/.gitlawb) #[arg(long)] dir: Option, /// Overwrite existing keys if present @@ -64,56 +65,27 @@ pub async fn run(cmd: IdentityCmd) -> Result<()> { } /// Resolve the identity directory, honouring an explicit override. -/// Public so sibling commands (`gl ucan import`) store alongside the identity. +/// Public so sibling commands (`gl ucan import`, `gl doctor`) look in the same +/// place the identity itself lives. /// -/// Falls back to the parent of `GITLAWB_KEY` before `~/.gitlawb`. Without that, -/// an operator who moved their key — `GITLAWB_KEY=/data/keys/identity.pem`, the -/// shape `.env.example` documents — has `gl ucan import` write the delegation to -/// `~/.gitlawb/delegations` while `git-remote-gitlawb`, which resolves its store -/// from `GITLAWB_KEY`, reads an empty directory. The push then goes out with no -/// `X-Ucan` and the delegate is refused, with nothing to indicate why. +/// Without an override this is [`gitlawb_core::identity_path::identity_dir`] — the +/// parent of `GITLAWB_KEY`, else `~/.gitlawb`. The rules live in `gitlawb-core` +/// because `git-remote-gitlawb` needs the identical answer: an operator who moved +/// their key (`GITLAWB_KEY=/data/keys/identity.pem`, the shape `.env.example` +/// documents) would otherwise have `gl ucan import` write the delegation to +/// `~/.gitlawb/delegations` while the helper reads `/data/keys/delegations` and +/// finds it empty. The push then goes out with no `X-Ucan` and the delegate is +/// refused, with nothing on either side to indicate why. pub fn gitlawb_dir(override_dir: Option) -> Result { if let Some(d) = override_dir { return Ok(d); } - // `var_os`, not `var`: a non-UTF-8 value makes `var` return Err, which would be - // indistinguishable from unset and would silently select ~/.gitlawb instead of - // the operator's actual key directory. - if let Some(raw) = std::env::var_os("GITLAWB_KEY") { - let key = PathBuf::from(&raw); - if !key.as_os_str().is_empty() { - let path = match key.strip_prefix("~") { - Ok(rest) => dirs::home_dir() - .context("could not determine home directory")? - .join(rest), - Err(_) => key, - }; - // A relative GITLAWB_KEY is refused rather than resolved. `gl` and - // `git-remote-gitlawb` run with different working directories, so a - // relative path makes them derive different delegation stores — the - // import lands somewhere the helper never looks, and the push is refused - // with nothing to indicate why. A one-component value is worse still: - // `parent()` yields "", so the store becomes `./delegations`. - if !path.is_absolute() { - anyhow::bail!( - "GITLAWB_KEY must be an absolute path (got {}). It determines where \ - delegations are stored, and `gl` and `git-remote-gitlawb` do not \ - share a working directory, so a relative path sends them to \ - different places.", - path.display() - ); - } - if let Some(parent) = path.parent() { - return Ok(parent.to_path_buf()); - } - } - } - let home = dirs::home_dir().context("could not determine home directory")?; - Ok(home.join(".gitlawb")) + let home = dirs::home_dir().context("could not determine the home directory")?; + gitlawb_core::identity_path::identity_dir(&home).map_err(|e| anyhow::anyhow!("{e}")) } fn key_path(dir: &Path) -> PathBuf { - dir.join("identity.pem") + dir.join(gitlawb_core::identity_path::KEY_FILE_NAME) } fn load_keypair(dir: Option) -> Result { @@ -122,14 +94,14 @@ fn load_keypair(dir: Option) -> Result { /// Load keypair from an optional directory override. /// Used by other modules (register, repo, mcp). +/// +/// Routed through [`gitlawb_dir`] rather than reaching for `~/.gitlawb` directly: +/// `gl identity new` writes the key wherever `GITLAWB_KEY` points, so a second +/// resolver here would have every other command read a different file than the one +/// just created — `gl ucan delegate` would either fail to find an identity or sign +/// with a stale DID that is not the repo owner. pub fn load_keypair_from_dir(dir: Option<&std::path::Path>) -> Result { - let base = if let Some(d) = dir { - d.to_path_buf() - } else { - dirs::home_dir() - .context("could not determine home directory")? - .join(".gitlawb") - }; + let base = gitlawb_dir(dir.map(Path::to_path_buf))?; let path = key_path(&base); let pem = fs::read_to_string(&path).with_context(|| { format!( @@ -553,8 +525,31 @@ mod tests { #[cfg(test)] mod gitlawb_dir_tests { - use super::gitlawb_dir; + use super::{gitlawb_dir, load_keypair_from_dir}; + use std::ffi::OsString; use std::path::PathBuf; + use std::sync::Mutex; + + /// The process environment is global. Every case that touches `GITLAWB_KEY` + /// takes this lock so they cannot race each other. + static ENV_LOCK: Mutex<()> = Mutex::new(()); + + /// Run `f` with `GITLAWB_KEY` set to `value` (or removed for `None`), restoring + /// whatever was there before. + fn with_key_env(value: Option, f: impl FnOnce() -> T) -> T { + let _guard = ENV_LOCK.lock().unwrap_or_else(|e| e.into_inner()); + let restore = std::env::var_os("GITLAWB_KEY"); + match &value { + Some(v) => std::env::set_var("GITLAWB_KEY", v), + None => std::env::remove_var("GITLAWB_KEY"), + } + let out = f(); + match restore { + Some(v) => std::env::set_var("GITLAWB_KEY", v), + None => std::env::remove_var("GITLAWB_KEY"), + } + out + } /// An explicit --dir always wins and is never validated against GITLAWB_KEY. #[test] @@ -567,37 +562,69 @@ mod gitlawb_dir_tests { /// from different working directories, so resolving one relatively sends the /// import and the lookup to different stores; a one-component value yields an /// empty parent and puts the store in `./delegations`. - /// - /// Serialised with the other env-touching case: the process environment is - /// global and these would otherwise race. #[test] - fn relative_and_nonunicode_key_paths() { - use std::sync::Mutex; - static LOCK: Mutex<()> = Mutex::new(()); - let _guard = LOCK.lock().unwrap_or_else(|e| e.into_inner()); + fn relative_key_paths_are_refused() { + for raw in ["identity.pem", "keys/identity.pem"] { + let result = with_key_env(Some(OsString::from(raw)), || gitlawb_dir(None)); + assert!(result.is_err(), "{raw} is relative and must be refused"); + } + } - let restore = std::env::var_os("GITLAWB_KEY"); + /// An empty value is what a shell leaves behind for `FOO=` and for an unset + /// variable expanded into a wrapper script. Treated as unset, not as an error. + #[test] + fn an_empty_key_path_is_treated_as_unset() { + let result = with_key_env(Some(OsString::new()), || gitlawb_dir(None)); + assert_eq!( + result.unwrap(), + dirs::home_dir().unwrap().join(".gitlawb"), + "an empty value selects the default directory" + ); + } - std::env::set_var("GITLAWB_KEY", "identity.pem"); - let one_component = gitlawb_dir(None); - std::env::set_var("GITLAWB_KEY", "keys/identity.pem"); - let relative = gitlawb_dir(None); - std::env::set_var("GITLAWB_KEY", ""); - let empty = gitlawb_dir(None); + /// The reason `gitlawb_dir` reads through `var_os`: `env::var` folds a non-UTF-8 + /// value into the same `Err` as unset, so a bad path would silently resolve to + /// `~/.gitlawb` instead of being reported. Byte 0xFF is not valid UTF-8 in any + /// position, so this value is unreachable through `env::var`. + #[cfg(unix)] + #[test] + fn a_non_utf8_key_path_is_not_mistaken_for_unset() { + use std::os::unix::ffi::OsStringExt; - match restore { - Some(v) => std::env::set_var("GITLAWB_KEY", v), - None => std::env::remove_var("GITLAWB_KEY"), - } + let raw = OsString::from_vec(b"keys/\xFF/identity.pem".to_vec()); + let result = with_key_env(Some(raw), || gitlawb_dir(None)); + let err = result.expect_err("a non-UTF-8 relative path must be refused"); assert!( - one_component.is_err(), - "a one-component key path yields an empty parent and must be refused" + err.to_string().contains("absolute"), + "the error must name the real problem, not fall back to the default: {err}" ); - assert!(relative.is_err(), "a relative key path must be refused"); - assert!( - empty.is_ok(), - "an empty value is treated as unset, not as an error" + + let mut absolute = OsString::from("/data/"); + absolute.push(OsString::from_vec(vec![0xFF])); + absolute.push("/identity.pem"); + let resolved = with_key_env(Some(absolute.clone()), || gitlawb_dir(None)).unwrap(); + assert_eq!( + resolved, + PathBuf::from(&absolute).parent().unwrap(), + "an absolute non-UTF-8 path resolves to its own parent, not to ~/.gitlawb" ); } + + /// `gl identity new` writes the key wherever `GITLAWB_KEY` points, so every + /// other command has to read it back from there. `load_keypair_from_dir(None)` + /// used to hardcode `~/.gitlawb`, which made `gl ucan delegate` sign with a + /// stale DID — or fail outright — for exactly the operators who moved the key. + #[test] + fn load_keypair_from_dir_honours_the_key_env() { + let dir = tempfile::tempdir().unwrap(); + let key = dir.path().join("identity.pem"); + let expected = gitlawb_core::identity::Keypair::generate(); + std::fs::write(&key, expected.to_pem().unwrap()).unwrap(); + + let loaded = with_key_env(Some(key.into_os_string()), || load_keypair_from_dir(None)) + .expect("the identity beside GITLAWB_KEY must be found"); + + assert_eq!(loaded.did(), expected.did()); + } } diff --git a/crates/gl/src/init.rs b/crates/gl/src/init.rs index 1bc3c406..545a49dd 100644 --- a/crates/gl/src/init.rs +++ b/crates/gl/src/init.rs @@ -22,7 +22,7 @@ pub struct InitArgs { #[arg(long, default_value = "https://node.gitlawb.com", env = "GITLAWB_NODE")] pub node: String, - /// Identity directory (default: ~/.gitlawb) + /// Identity directory (default: the parent of $GITLAWB_KEY, else ~/.gitlawb) #[arg(long)] pub dir: Option, @@ -101,10 +101,7 @@ pub async fn run(args: InitArgs) -> Result<()> { // Save UCAN if returned if let Some(ucan) = payload.get("ucan").and_then(|v| v.as_str()) { if !ucan.is_empty() { - let ucan_dir = args - .dir - .clone() - .unwrap_or_else(|| dirs::home_dir().unwrap_or_default().join(".gitlawb")); + let ucan_dir = crate::identity::gitlawb_dir(args.dir.clone())?; std::fs::create_dir_all(&ucan_dir)?; let record = json!({ "ucan": ucan, @@ -221,13 +218,7 @@ pub async fn run(args: InitArgs) -> Result<()> { } fn generate_identity(dir: Option<&std::path::Path>) -> Result { - let base = if let Some(d) = dir { - d.to_path_buf() - } else { - dirs::home_dir() - .context("could not determine home directory")? - .join(".gitlawb") - }; + let base = crate::identity::gitlawb_dir(dir.map(std::path::Path::to_path_buf))?; std::fs::create_dir_all(&base)?; let keypair = gitlawb_core::identity::Keypair::generate(); diff --git a/crates/gl/src/mcp.rs b/crates/gl/src/mcp.rs index ae319c73..d01a99bf 100644 --- a/crates/gl/src/mcp.rs +++ b/crates/gl/src/mcp.rs @@ -759,9 +759,7 @@ async fn call_tool( ]))?), "ucan_show" => { - let ucan_path = dirs::home_dir() - .context("no home dir")? - .join(".gitlawb/ucan.json"); + let ucan_path = crate::identity::gitlawb_dir(None)?.join("ucan.json"); if ucan_path.exists() { let content = std::fs::read_to_string(ucan_path)?; Ok(content) diff --git a/crates/gl/src/ucan_cmd.rs b/crates/gl/src/ucan_cmd.rs index 92f3e925..93450b7f 100644 --- a/crates/gl/src/ucan_cmd.rs +++ b/crates/gl/src/ucan_cmd.rs @@ -6,7 +6,7 @@ use serde_json::json; use std::path::PathBuf; use gitlawb_core::did::Did; -use gitlawb_core::ucan::{Capability, Ucan}; +use gitlawb_core::ucan::{caps, Capability, Ucan}; use crate::identity::load_keypair_from_dir; @@ -164,21 +164,32 @@ async fn cmd_import(token: String, dir: Option) -> Result<()> { "not a valid UCAN token — pass the JSON emitted by `gl ucan delegate`, or a path to it", )?; + // Import admits only what the push path can actually use. `build_invocation` + // requires a push-class action on a concrete repository resource, so a token + // carrying only `pr/open` would import "successfully", print a stored path, and + // then be dropped at push time behind a `tracing::warn` the operator never + // sees — a denial wearing the shape of an empty success. let push_caps: Vec<(String, String)> = ucan .payload .att .iter() + .filter(|cap| is_push_class(&cap.can)) .filter_map(|cap| repo_from_resource(&cap.with)) .collect(); if push_caps.is_empty() { anyhow::bail!( - "this delegation names no repository — expected a capability on \ - gitlawb://repos//, found: {}", + "this delegation carries no storable push capability — expected {} or {} \ + (or \"*\") on gitlawb://repos//, found: {}\n\ + A delegation whose resource is \"*\" cannot be imported either: the store \ + is keyed by repository, so re-issue it against the repository you intend \ + to push to.", + caps::GIT_PUSH, + caps::REPO_ADMIN, ucan.payload .att .iter() - .map(|c| c.with.as_str()) + .map(|c| format!("{} -> {}", c.with, c.can)) .collect::>() .join(", ") ); @@ -186,43 +197,86 @@ async fn cmd_import(token: String, dir: Option) -> Result<()> { // The identity directory is a private-data contract, not a public one: it // already holds `identity.pem`, whose disclosure is strictly worse than a - // delegation's. On Unix both get explicit modes. On other targets neither does - // — `std::fs` has no portable ACL API and `gitlawb_dir` accepts any directory — - // so the contract there is that the caller supplies a user-private directory, - // which is what the platform's per-user profile gives by default. Diverging for - // this one file while the private key beside it relies on the same assumption - // would be theatre. + // delegation's. `create_private_dir` and `write_private_file` below carry the + // per-platform reasoning. let base = crate::identity::gitlawb_dir(dir)?; let store = base.join("delegations"); - std::fs::create_dir_all(&store) - .with_context(|| format!("could not create {}", store.display()))?; - #[cfg(unix)] - { - use std::os::unix::fs::PermissionsExt; - std::fs::set_permissions(&store, std::fs::Permissions::from_mode(0o700)) - .with_context(|| format!("could not set permissions on {}", store.display()))?; - } + create_private_dir(&store).with_context(|| format!("could not create {}", store.display()))?; for (owner, repo) in &push_caps { let path = delegation_path(&base, owner, repo); - std::fs::write(&path, &raw) - .with_context(|| format!("could not write {}", path.display()))?; // 0600, like the sibling identity key. The token is not itself sufficient to // push — the node requires `iss` to equal the request signer, so a reader // still needs the delegate's private key — but it does disclose the // delegation graph and which identities hold capabilities on which repos. - #[cfg(unix)] - { - use std::os::unix::fs::PermissionsExt; - std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o600)) - .with_context(|| format!("could not set permissions on {}", path.display()))?; - } + write_private_file(&path, raw.as_bytes()) + .with_context(|| format!("could not write {}", path.display()))?; println!("Stored delegation for {owner}/{repo} at {}", path.display()); } Ok(()) } +/// Actions the push path accepts. Kept in step with the filter in +/// `git-remote-gitlawb`'s `build_invocation`, which is what actually mints an +/// invocation from a stored delegation. +fn is_push_class(can: &str) -> bool { + can == caps::GIT_PUSH || can == "*" || can == caps::REPO_ADMIN +} + +/// Create the delegation store owner-only, with no window at a wider mode. +/// +/// `create_dir_all` followed by `set_permissions` leaves the directory at the +/// process umask — 0755 under the usual 022 — until the second call lands, which +/// is long enough for another local user to open it. The mode rides on the +/// creating syscall instead. The follow-up `set_permissions` is not the window +/// reopening: it only matters when the directory already existed, and repairs a +/// 0755 store left behind by an older `gl`. +#[cfg(unix)] +fn create_private_dir(path: &std::path::Path) -> std::io::Result<()> { + use std::os::unix::fs::{DirBuilderExt, PermissionsExt}; + std::fs::DirBuilder::new() + .mode(0o700) + .recursive(true) + .create(path)?; + std::fs::set_permissions(path, std::fs::Permissions::from_mode(0o700)) +} + +/// Write `contents`, owner-only from the moment the file exists. +/// +/// Not `create_new`: re-importing a refreshed delegation has to overwrite the +/// stored one. `mode` applies only when the file is created, so the trailing +/// `set_permissions` covers a 0644 file written by an older `gl`. +#[cfg(unix)] +fn write_private_file(path: &std::path::Path, contents: &[u8]) -> std::io::Result<()> { + use std::io::Write; + use std::os::unix::fs::{OpenOptionsExt, PermissionsExt}; + let mut file = std::fs::OpenOptions::new() + .write(true) + .create(true) + .truncate(true) + .mode(0o600) + .open(path)?; + file.write_all(contents)?; + std::fs::set_permissions(path, std::fs::Permissions::from_mode(0o600)) +} + +// `std::fs` has no portable ACL API, and `gitlawb_dir` accepts any directory, so +// the contract off Unix is that the caller supplies a user-private directory — +// which is what the platform's per-user profile gives by default. The private key +// sits in the same directory under the same assumption, and its disclosure is +// strictly worse than a delegation's, so hardening this one file alone would be +// theatre. +#[cfg(not(unix))] +fn create_private_dir(path: &std::path::Path) -> std::io::Result<()> { + std::fs::create_dir_all(path) +} + +#[cfg(not(unix))] +fn write_private_file(path: &std::path::Path, contents: &[u8]) -> std::io::Result<()> { + std::fs::write(path, contents) +} + async fn cmd_delegate( to: String, cap: String, @@ -279,10 +333,7 @@ async fn cmd_delegate( } async fn cmd_show(dir: Option) -> Result<()> { - let home = dir - .or_else(|| dirs::home_dir().map(|h| h.join(".gitlawb"))) - .context("cannot find identity directory")?; - let ucan_path = home.join("ucan.json"); + let ucan_path = crate::identity::gitlawb_dir(dir)?.join("ucan.json"); if !ucan_path.exists() { println!("No UCAN saved. Run `gl register` first."); @@ -570,4 +621,113 @@ mod delegation_store_tests { "bare and full owner forms must address the same delegation" ); } + + fn token_for(can: &str, with: &str) -> String { + let owner = gitlawb_core::identity::Keypair::generate(); + let agent = gitlawb_core::identity::Keypair::generate(); + Ucan::issue( + &owner, + agent.did(), + vec![Capability::new(with, can)], + Some(chrono::Utc::now() + chrono::Duration::hours(1)), + ) + .unwrap() + .encode() + .unwrap() + } + + /// A delegation the push path cannot use must fail at import, where the + /// operator is watching. `build_invocation` requires a push-class action, so a + /// `pr/open` token that imported "successfully" would be silently dropped from + /// the push behind a `tracing::warn` and surface only as a 403 with no + /// connection to the earlier success. + #[tokio::test] + async fn import_refuses_a_delegation_the_push_path_cannot_use() { + for can in ["pr/open", "issue/create", "git/fetch"] { + let dir = tempfile::tempdir().unwrap(); + let token = token_for(can, "gitlawb://repos/z6MkAbc/myrepo"); + + let err = cmd_import(token, Some(dir.path().to_path_buf())) + .await + .expect_err("{can} is not a push capability and must be refused"); + + assert!( + err.to_string().contains(caps::GIT_PUSH), + "the error must name the action the push path needs: {err}" + ); + assert!( + !dir.path().join("delegations").exists(), + "nothing may be written before the capability is accepted" + ); + } + } + + /// The resource is `*`, so the store — which is keyed by repository — has no + /// filename to write under. Refused with an explanation rather than reported as + /// an import that stored nothing. + #[tokio::test] + async fn import_refuses_a_wildcard_resource() { + let dir = tempfile::tempdir().unwrap(); + let token = token_for(caps::GIT_PUSH, "*"); + + let err = cmd_import(token, Some(dir.path().to_path_buf())) + .await + .expect_err("a wildcard resource cannot be keyed by repository"); + + assert!( + err.to_string().contains("re-issue"), + "the error must say what to do instead: {err}" + ); + assert!(!dir.path().join("delegations").exists()); + } + + #[tokio::test] + async fn import_accepts_every_push_class_action() { + for can in [caps::GIT_PUSH, caps::REPO_ADMIN, "*"] { + let dir = tempfile::tempdir().unwrap(); + let token = token_for(can, "gitlawb://repos/z6MkAbc/myrepo"); + + cmd_import(token, Some(dir.path().to_path_buf())) + .await + .unwrap_or_else(|e| panic!("{can} must import: {e}")); + + let stored = delegation_path(dir.path(), "z6MkAbc", "myrepo"); + assert!(stored.exists(), "{can} must leave a stored delegation"); + } + } + + /// The store and the token file must never exist at a wider mode, not even + /// briefly: `create_dir_all` then chmod leaves 0755 under the usual umask, and + /// the token discloses the delegation graph. + #[cfg(unix)] + #[tokio::test] + async fn import_creates_the_store_and_token_owner_only() { + use std::os::unix::fs::PermissionsExt; + + let dir = tempfile::tempdir().unwrap(); + let token = token_for(caps::GIT_PUSH, "gitlawb://repos/z6MkAbc/myrepo"); + cmd_import(token.clone(), Some(dir.path().to_path_buf())) + .await + .unwrap(); + + let store = dir.path().join("delegations"); + let stored = delegation_path(dir.path(), "z6MkAbc", "myrepo"); + assert_eq!( + std::fs::metadata(&store).unwrap().permissions().mode() & 0o777, + 0o700 + ); + assert_eq!( + std::fs::metadata(&stored).unwrap().permissions().mode() & 0o777, + 0o600 + ); + + // Re-import has to overwrite, which is why this is not `create_new`. + cmd_import(token, Some(dir.path().to_path_buf())) + .await + .unwrap(); + assert_eq!( + std::fs::metadata(&stored).unwrap().permissions().mode() & 0o777, + 0o600 + ); + } } From 05fc52db534daafbe68987fc60eb980d4f442f8f Mon Sep 17 00:00:00 2001 From: Vasanthdev2004 Date: Sun, 16 Aug 2026 12:46:51 +0530 Subject: [PATCH 16/18] test(git-remote): cover delegation_header end to end MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `delegation_header` was the one piece of the delegated push path with no test. Its parts each had one — `split_pack_post_url`, `build_invocation`, `delegation_path` — but nothing checked they compose, and this is the function where a regression is silent by construction: every failure inside it returns `None` and the push goes out without `X-Ucan`, so a break surfaces as a 403 at the node with nothing locally to connect it to. Five cases, driven against a mockito node with the store seeded where `gl ucan import` would have left it: - the delegated push: a stored token becomes an invocation issued by the agent, addressed to the node's DID, rooted at the repo owner, with every link bounded - the owner's own push: no store read, no node round-trip. The comparison has to survive the form mismatch — the keypair holds `did:key:z…` while the URL carries the bare key — so the mock asserts zero hits - no usable delegation: an empty store, a token that does not decode, and a `git/fetch` capability that carries nothing to wrap. All three yield no header rather than an error - an unreadable node DID: a 500, a JSON body with no `did`, a proxy's HTML error page, and a `did` that does not parse. The push loses its header, never aborts - a path-prefixed node base: the DID probe must go to `/gitlawb`, not `/`. `split_pack_post_url` is unit-tested for the prefix, but nothing checked the probe followed it; the mock on `/` asserts zero hits Each case was watched failing before it was kept, against five separate mutations: the owner short-circuit removed, the prefix dropped from the node base, `build_invocation`'s push-class filter widened, the node-DID lookup given a fallback, and the invocation addressed to the agent instead of the node. --- crates/git-remote-gitlawb/src/main.rs | 297 ++++++++++++++++++++++++++ 1 file changed, 297 insertions(+) diff --git a/crates/git-remote-gitlawb/src/main.rs b/crates/git-remote-gitlawb/src/main.rs index 32c7fae4..2f71a8dd 100644 --- a/crates/git-remote-gitlawb/src/main.rs +++ b/crates/git-remote-gitlawb/src/main.rs @@ -2642,4 +2642,301 @@ mod delegated_push_tests { assert_eq!(delegation_path(base, "did:key:z6MkAbc", "myrepo"), expected); assert_eq!(delegation_path(base, "z6MkAbc", "myrepo"), expected); } + + // ── delegation_header ───────────────────────────────────────────────────── + // + // Everything above tests one piece in isolation. `delegation_header` is where + // they compose — URL split, owner comparison, store lookup, node-DID probe, + // invocation build — and it is the piece with no safety net: every failure + // inside it is deliberately silent, so a regression does not fail loudly, it + // just stops attaching `X-Ucan` and the delegate starts getting 403s with no + // local explanation. + + /// `delegation_header` resolves its store from `GITLAWB_KEY`, which is + /// process-global. Every case that sets it takes this lock. + /// + /// Only these cases need it. `advertisement_and_pack_post_are_signed_…` also + /// reaches `delegation_header` (through `build_pack_post_request` on + /// `git-receive-pack`), but it asserts on signature headers alone and is + /// unaffected by whichever store is in scope. + static KEY_ENV_LOCK: std::sync::Mutex<()> = std::sync::Mutex::new(()); + + /// Point `GITLAWB_KEY` at `dir/identity.pem` for the duration of `f`, so the + /// delegation store resolves to `dir/delegations`. + fn with_identity_dir(dir: &std::path::Path, f: impl FnOnce() -> T) -> T { + let _guard = KEY_ENV_LOCK.lock().unwrap_or_else(|e| e.into_inner()); + let restore = std::env::var_os("GITLAWB_KEY"); + std::env::set_var("GITLAWB_KEY", dir.join("identity.pem")); + let out = f(); + match restore { + Some(v) => std::env::set_var("GITLAWB_KEY", v), + None => std::env::remove_var("GITLAWB_KEY"), + } + out + } + + /// Write a token where `gl ucan import` would have left it. + fn store_delegation(dir: &std::path::Path, owner: &str, repo: &str, raw: &str) { + let path = delegation_path(dir, owner, repo); + std::fs::create_dir_all(path.parent().expect("delegations dir")).expect("mkdir"); + std::fs::write(path, raw).expect("write delegation"); + } + + fn bare(did: &gitlawb_core::did::Did) -> String { + did.to_string() + .strip_prefix("did:key:") + .expect("did:key") + .to_string() + } + + fn push_delegation(owner: &Keypair, agent: &Keypair, resource: &str) -> Ucan { + Ucan::issue( + owner, + agent.did(), + vec![Capability::new(resource, caps::GIT_PUSH)], + Some(chrono::Utc::now() + chrono::Duration::hours(1)), + ) + .expect("issue") + } + + fn did_body(node: &Keypair) -> String { + format!(r#"{{"did":"{}"}}"#, node.did()) + } + + #[test] + fn delegation_header_wraps_a_stored_delegation_and_targets_the_node() { + let owner = Keypair::generate(); + let agent = Keypair::generate(); + let node = Keypair::generate(); + let owner_key = bare(&owner.did()); + + let delegation = push_delegation(&owner, &agent, &format!("gitlawb://repos/{owner_key}/r")); + + let mut server = mockito::Server::new(); + let did_probe = server + .mock("GET", "/") + .with_header("content-type", "application/json") + .with_body(did_body(&node)) + .create(); + + let dir = tempfile::tempdir().expect("tempdir"); + store_delegation( + dir.path(), + &owner_key, + "r", + &delegation.encode().expect("encode"), + ); + + let post_url = format!("{}/{owner_key}/r/git-receive-pack", server.url()); + let client = reqwest::blocking::Client::new(); + let header = + with_identity_dir(dir.path(), || delegation_header(&client, &post_url, &agent)) + .expect("a stored delegation must produce an X-Ucan"); + + did_probe.assert(); + + let invocation = Ucan::decode(&header).expect("the header must decode as a UCAN"); + assert_eq!(invocation.payload.iss, agent.did(), "the agent invokes"); + assert_eq!( + invocation.payload.aud, + node.did(), + "addressed to the node that will execute it" + ); + assert_eq!( + invocation.verify_chain().expect("chain must verify"), + owner.did(), + "the chain must root at the repo owner" + ); + assert!( + invocation.chain_lifetime_is_bounded(), + "the node refuses an unbounded push chain, so every link must carry an expiry" + ); + } + + /// The owner pushes on their own authority. The comparison has to survive the + /// form mismatch — the keypair holds `did:key:z…`, the URL carries the bare key + /// — or the owner takes the delegate path, finds nothing, and pays a node + /// round-trip on every push. `.expect(0)` is the assertion that matters here. + #[test] + fn delegation_header_is_skipped_when_the_pusher_is_the_owner() { + let owner = Keypair::generate(); + let node = Keypair::generate(); + let owner_key = bare(&owner.did()); + + let mut server = mockito::Server::new(); + let did_probe = server + .mock("GET", "/") + .with_body(did_body(&node)) + .expect(0) + .create(); + + // A delegation the owner does not need. Present so the assertion is about + // the owner check and not about an empty store. + let dir = tempfile::tempdir().expect("tempdir"); + let delegate = Keypair::generate(); + store_delegation( + dir.path(), + &owner_key, + "r", + &push_delegation(&owner, &delegate, &format!("gitlawb://repos/{owner_key}/r")) + .encode() + .expect("encode"), + ); + + let post_url = format!("{}/{owner_key}/r/git-receive-pack", server.url()); + let client = reqwest::blocking::Client::new(); + let header = + with_identity_dir(dir.path(), || delegation_header(&client, &post_url, &owner)); + + assert!(header.is_none(), "the owner needs no delegation"); + did_probe.assert(); + } + + /// Best-effort means best-effort: nothing here may panic or block the push. The + /// node decides whether a delegation was required, and its denial has to reach + /// the user instead of being pre-empted by a local guess. + #[test] + fn delegation_header_is_absent_without_a_usable_stored_delegation() { + let agent = Keypair::generate(); + let node = Keypair::generate(); + let owner_key = bare(&Keypair::generate().did()); + + let mut server = mockito::Server::new(); + let _did = server.mock("GET", "/").with_body(did_body(&node)).create(); + let post_url = format!("{}/{owner_key}/r/git-receive-pack", server.url()); + let client = reqwest::blocking::Client::new(); + + // Nothing stored at all. + let empty = tempfile::tempdir().expect("tempdir"); + assert!( + with_identity_dir(empty.path(), || delegation_header( + &client, &post_url, &agent + )) + .is_none(), + "an empty store must yield no header, not an error" + ); + + // Stored, but not a UCAN — a truncated write or a hand-edited file. + let garbage = tempfile::tempdir().expect("tempdir"); + store_delegation(garbage.path(), &owner_key, "r", "not a ucan"); + assert!( + with_identity_dir(garbage.path(), || delegation_header( + &client, &post_url, &agent + )) + .is_none(), + "an unreadable stored token must yield no header, not a panic" + ); + + // Stored and valid, but for a capability the push path cannot use. `gl ucan + // import` refuses these now; a store written by an older `gl` still holds them. + let owner = Keypair::generate(); + let wrong_owner_key = bare(&owner.did()); + let unusable = tempfile::tempdir().expect("tempdir"); + let fetch_only = Ucan::issue( + &owner, + agent.did(), + vec![Capability::new( + format!("gitlawb://repos/{wrong_owner_key}/r"), + caps::GIT_FETCH, + )], + Some(chrono::Utc::now() + chrono::Duration::hours(1)), + ) + .expect("issue"); + store_delegation( + unusable.path(), + &wrong_owner_key, + "r", + &fetch_only.encode().expect("encode"), + ); + let fetch_url = format!("{}/{wrong_owner_key}/r/git-receive-pack", server.url()); + assert!( + with_identity_dir(unusable.path(), || delegation_header( + &client, &fetch_url, &agent + )) + .is_none(), + "a git/fetch delegation carries no push capability to wrap" + ); + } + + /// The node DID addresses the invocation, so without it there is nothing to + /// build. A node that is down, slow, or serving something other than JSON must + /// cost the push a header, never an abort. + #[test] + fn delegation_header_is_absent_when_the_node_did_cannot_be_read() { + let owner = Keypair::generate(); + let agent = Keypair::generate(); + let owner_key = bare(&owner.did()); + let encoded = push_delegation(&owner, &agent, &format!("gitlawb://repos/{owner_key}/r")) + .encode() + .expect("encode"); + let client = reqwest::blocking::Client::new(); + + for (label, status, body) in [ + ("a 500 from the node", 500, "boom"), + ("a JSON body with no did", 200, r#"{"name":"gitlawb"}"#), + ("an HTML error page from a proxy", 200, "502"), + ("a did that does not parse", 200, r#"{"did":"not-a-did"}"#), + ] { + let mut server = mockito::Server::new(); + let _did = server + .mock("GET", "/") + .with_status(status) + .with_body(body) + .create(); + + let dir = tempfile::tempdir().expect("tempdir"); + store_delegation(dir.path(), &owner_key, "r", &encoded); + let post_url = format!("{}/{owner_key}/r/git-receive-pack", server.url()); + + assert!( + with_identity_dir(dir.path(), || delegation_header(&client, &post_url, &agent)) + .is_none(), + "{label} must drop the header, not fail the push" + ); + } + } + + /// A reverse-proxied `GITLAWB_NODE` carries a path prefix, and that prefix + /// survives into the pack URL. `split_pack_post_url` is unit-tested for it, but + /// nothing checked that the probe actually goes to the prefixed base — a + /// regression there would GET `/` on the proxy host, read whatever landing page + /// it serves, and silently drop the header. `.expect(0)` on `/` is the half that + /// catches it. + #[test] + fn delegation_header_probes_the_prefixed_node_base() { + let owner = Keypair::generate(); + let agent = Keypair::generate(); + let node = Keypair::generate(); + let owner_key = bare(&owner.did()); + + let mut server = mockito::Server::new(); + let prefixed = server + .mock("GET", "/gitlawb") + .with_body(did_body(&node)) + .create(); + let root = server.mock("GET", "/").expect(0).create(); + + let dir = tempfile::tempdir().expect("tempdir"); + store_delegation( + dir.path(), + &owner_key, + "r", + &push_delegation(&owner, &agent, &format!("gitlawb://repos/{owner_key}/r")) + .encode() + .expect("encode"), + ); + + let post_url = format!("{}/gitlawb/{owner_key}/r/git-receive-pack", server.url()); + let client = reqwest::blocking::Client::new(); + let header = + with_identity_dir(dir.path(), || delegation_header(&client, &post_url, &agent)) + .expect("a path-prefixed node base must still yield an X-Ucan"); + + prefixed.assert(); + root.assert(); + assert_eq!( + Ucan::decode(&header).expect("decode").payload.aud, + node.did() + ); + } } From 75aa1a0a4ebe8c778cdedfceef9ea692f3567a4e Mon Sep 17 00:00:00 2001 From: Vasanthdev2004 Date: Sun, 16 Aug 2026 21:11:08 +0530 Subject: [PATCH 17/18] fix(gl): finish the identity-path sweep on the write side MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous commit unified where `gl` READS the identity from and stopped there. Four resolvers were left holding their own `~/.gitlawb`, and one of them is a write: register.rs ucan_path the bootstrap UCAN from POST /api/register quickstart.rs the wizard dir identity generation AND the UCAN it stores name.rs identity_dir reads identity.pem node_stake.rs load_did reads identity.pem `register.rs` is the one that actually breaks a working setup. With `GITLAWB_KEY=/data/keys/identity.pem` — the shape `.env.example` documents — `gl register` loaded the key from `/data/keys/identity.pem` and then wrote `ucan.json` to `~/.gitlawb/`. Registration reported success; `gl doctor`, `gl ucan show`, `gl init`, and `gl mcp ucan_show` all read `ucan.json` from the key's directory, found nothing, and reported an unregistered identity. Split storage, no error anywhere. `quickstart` had the same shape end to end: it generates the identity and stores the bootstrap token, so an operator who ran the wizard under `GITLAWB_KEY` got both in a directory nothing else consults. `name.rs` and `node_stake.rs` only read, but both fell back to `PathBuf::from(".")` when the home directory could not be determined, which makes the identity path depend on the working directory — the same defect the helper's `resolve_key_path` carried before it moved to the shared resolver. All four now go through `crate::identity::gitlawb_dir`. Afterwards the only `dirs::home_dir()` calls left in `gl` are the one inside `gitlawb_dir` itself, one test expectation, and `doctor`'s shell-rc scan, which is unrelated to the identity directory. `gl register` also no longer prints a path it did not use: the success line was hardcoded to `~/.gitlawb/ucan.json`, so an operator with `GITLAWB_KEY` set was told to look somewhere the file was not. It now prints the real destination, and says so plainly when the node returned no token at all. The `GITLAWB_KEY` test guard moves to `identity::test_env` so the resolver's own cases and the new register case share one lock. Two suites each holding their own mutex over the same process-global variable would not serialise against each other. Verified by reverting `ucan_path` to the hardcoded form and watching `register_saves_the_bootstrap_ucan_beside_the_key` fail on exactly that assertion. --- crates/gl/src/identity.rs | 62 +++++++++++++++++++-------- crates/gl/src/name.rs | 15 ++++--- crates/gl/src/node_stake.rs | 9 ++-- crates/gl/src/quickstart.rs | 9 ++-- crates/gl/src/register.rs | 83 ++++++++++++++++++++++++++++++------- 5 files changed, 128 insertions(+), 50 deletions(-) diff --git a/crates/gl/src/identity.rs b/crates/gl/src/identity.rs index c6ff3fb9..3cf97ab7 100644 --- a/crates/gl/src/identity.rs +++ b/crates/gl/src/identity.rs @@ -523,32 +523,60 @@ mod tests { } } +/// Scoped `GITLAWB_KEY` for tests, shared crate-wide. +/// +/// The process environment is global and more than one suite in this crate +/// depends on it — the resolver's own cases here, and `gl register`'s check that +/// the bootstrap token lands beside the key. They take one lock rather than each +/// declaring its own, which would not serialise them against each other. #[cfg(test)] -mod gitlawb_dir_tests { - use super::{gitlawb_dir, load_keypair_from_dir}; - use std::ffi::OsString; - use std::path::PathBuf; - use std::sync::Mutex; +pub(crate) mod test_env { + use std::ffi::{OsStr, OsString}; + use std::sync::{Mutex, MutexGuard}; - /// The process environment is global. Every case that touches `GITLAWB_KEY` - /// takes this lock so they cannot race each other. static ENV_LOCK: Mutex<()> = Mutex::new(()); - /// Run `f` with `GITLAWB_KEY` set to `value` (or removed for `None`), restoring - /// whatever was there before. - fn with_key_env(value: Option, f: impl FnOnce() -> T) -> T { - let _guard = ENV_LOCK.lock().unwrap_or_else(|e| e.into_inner()); + /// Restores the previous value and releases the lock on drop. + pub(crate) struct KeyEnv { + _guard: MutexGuard<'static, ()>, + restore: Option, + } + + impl Drop for KeyEnv { + fn drop(&mut self) { + match self.restore.take() { + Some(v) => std::env::set_var("GITLAWB_KEY", v), + None => std::env::remove_var("GITLAWB_KEY"), + } + } + } + + /// Set `GITLAWB_KEY` (or remove it, for `None`) until the guard drops. + pub(crate) fn set_key>(value: Option) -> KeyEnv { + let guard = ENV_LOCK.lock().unwrap_or_else(|e| e.into_inner()); let restore = std::env::var_os("GITLAWB_KEY"); - match &value { + match value { Some(v) => std::env::set_var("GITLAWB_KEY", v), None => std::env::remove_var("GITLAWB_KEY"), } - let out = f(); - match restore { - Some(v) => std::env::set_var("GITLAWB_KEY", v), - None => std::env::remove_var("GITLAWB_KEY"), + KeyEnv { + _guard: guard, + restore, } - out + } +} + +#[cfg(test)] +mod gitlawb_dir_tests { + use super::{gitlawb_dir, load_keypair_from_dir}; + use std::ffi::OsString; + use std::path::PathBuf; + + /// Run `f` with `GITLAWB_KEY` set to `value` (or removed for `None`), restoring + /// whatever was there before. + fn with_key_env(value: Option, f: impl FnOnce() -> T) -> T { + let _env = crate::identity::test_env::set_key(value); + f() } /// An explicit --dir always wins and is never validated against GITLAWB_KEY. diff --git a/crates/gl/src/name.rs b/crates/gl/src/name.rs index 9840518b..48a073b3 100644 --- a/crates/gl/src/name.rs +++ b/crates/gl/src/name.rs @@ -168,16 +168,15 @@ pub async fn run(args: NameArgs) -> Result<()> { // ── Helpers ─────────────────────────────────────────────────────────────────── -fn identity_dir(dir: Option) -> PathBuf { - dir.unwrap_or_else(|| { - dirs::home_dir() - .unwrap_or_else(|| PathBuf::from(".")) - .join(".gitlawb") - }) +fn identity_dir(dir: Option) -> Result { + // The shared resolver, not a local `~/.gitlawb`: `gl identity new` writes the + // key wherever `GITLAWB_KEY` points, and the old fallback to `.` on a missing + // home made the answer depend on the working directory. + crate::identity::gitlawb_dir(dir) } fn load_did(dir: Option) -> Result { - let path = identity_dir(dir).join("identity.pem"); + let path = identity_dir(dir)?.join("identity.pem"); let pem = std::fs::read_to_string(&path).with_context(|| { format!( "No identity at {} — run `gl identity new` first", @@ -190,7 +189,7 @@ fn load_did(dir: Option) -> Result { } fn load_did_and_document(dir: Option) -> Result<(String, String)> { - let path = identity_dir(dir).join("identity.pem"); + let path = identity_dir(dir)?.join("identity.pem"); let pem = std::fs::read_to_string(&path).with_context(|| { format!( "No identity at {} — run `gl identity new` first", diff --git a/crates/gl/src/node_stake.rs b/crates/gl/src/node_stake.rs index 869afcf8..7a65af4e 100644 --- a/crates/gl/src/node_stake.rs +++ b/crates/gl/src/node_stake.rs @@ -328,11 +328,10 @@ pub async fn cmd_unstake( // ── Helpers ───────────────────────────────────────────────────────────────── fn load_did(dir: Option) -> Result { - let base = dir.unwrap_or_else(|| { - dirs::home_dir() - .unwrap_or_else(|| PathBuf::from(".")) - .join(".gitlawb") - }); + // The shared resolver, not a local `~/.gitlawb`: `gl identity new` writes the + // key wherever `GITLAWB_KEY` points, and the old fallback to `.` on a missing + // home made the answer depend on the working directory. + let base = crate::identity::gitlawb_dir(dir)?; let path = base.join("identity.pem"); let pem = std::fs::read_to_string(&path).with_context(|| { format!( diff --git a/crates/gl/src/quickstart.rs b/crates/gl/src/quickstart.rs index 8b901ad9..0654df74 100644 --- a/crates/gl/src/quickstart.rs +++ b/crates/gl/src/quickstart.rs @@ -39,11 +39,10 @@ pub async fn run(args: QuickstartArgs) -> Result<()> { println!("and create your first repository."); println!(); - let dir = args.dir.clone().unwrap_or_else(|| { - dirs::home_dir() - .unwrap_or_else(|| PathBuf::from(".")) - .join(".gitlawb") - }); + // The wizard generates the identity AND stores the bootstrap UCAN, so it has to + // land where every later command reads from — the parent of `GITLAWB_KEY`, not + // an unconditional `~/.gitlawb`. + let dir = crate::identity::gitlawb_dir(args.dir.clone())?; // ── Step 1: Identity ────────────────────────────────────────────────── println!("── Step 1: Identity ─────────────────────────────────────────────────"); diff --git a/crates/gl/src/register.rs b/crates/gl/src/register.rs index 8a17a77e..94a2f712 100644 --- a/crates/gl/src/register.rs +++ b/crates/gl/src/register.rs @@ -1,7 +1,8 @@ //! `gl register` — register this agent identity with a gitlawb node. //! //! Sends a signed POST /api/register request and saves the returned bootstrap -//! UCAN token to `~/.gitlawb/ucan.json` for use by other commands. +//! UCAN token as `ucan.json` beside the identity key, where the other commands +//! look for it. use anyhow::{Context, Result}; use clap::Args; @@ -69,17 +70,20 @@ pub async fn run(args: RegisterArgs) -> Result<()> { // Save bootstrap UCAN let ucan = payload.get("ucan").and_then(|v| v.as_str()).unwrap_or(""); - if !ucan.is_empty() { - let ucan_path = ucan_path(args.dir.as_deref())?; + let saved_to = if ucan.is_empty() { + None + } else { + let path = ucan_path(args.dir.as_deref())?; let record = json!({ "ucan": ucan, "node": args.node, "did": did.to_string(), "saved_at": chrono::Utc::now().to_rfc3339(), }); - std::fs::write(&ucan_path, serde_json::to_string_pretty(&record)?)?; - tracing::debug!("saved UCAN to {}", ucan_path.display()); - } + std::fs::write(&path, serde_json::to_string_pretty(&record)?)?; + tracing::debug!("saved UCAN to {}", path.display()); + Some(path) + }; let trust = payload .get("trust_score") @@ -99,21 +103,28 @@ pub async fn run(args: RegisterArgs) -> Result<()> { println!(" Trust score: {trust:.2}"); println!(" UCAN expires: {expires}"); println!(); - println!(" Bootstrap UCAN saved to ~/.gitlawb/ucan.json"); + // The real path, not the default one: `GITLAWB_KEY` moves it, and an operator + // told to look in `~/.gitlawb` would find nothing there. + match &saved_to { + Some(path) => println!(" Bootstrap UCAN saved to {}", path.display()), + None => println!(" The node returned no bootstrap UCAN."), + } println!(" You are now a verified agent on the gitlawb network."); Ok(()) } +/// Where the bootstrap UCAN is stored: beside the identity key, always. +/// +/// Routed through `gitlawb_dir` rather than resolving `~/.gitlawb` locally. The +/// key is READ from the parent of `GITLAWB_KEY`, so writing the token anywhere +/// else splits the two: registration succeeds, and `gl doctor`, `gl ucan show`, +/// `gl init`, and `gl mcp ucan_show` all read `ucan.json` from the key's +/// directory and report an unregistered identity. fn ucan_path(dir: Option<&std::path::Path>) -> Result { - let base = if let Some(d) = dir { - d.to_path_buf() - } else { - dirs::home_dir() - .context("could not determine home directory")? - .join(".gitlawb") - }; - std::fs::create_dir_all(&base)?; + let base = crate::identity::gitlawb_dir(dir.map(std::path::Path::to_path_buf))?; + std::fs::create_dir_all(&base) + .with_context(|| format!("failed to create {}", base.display()))?; Ok(base.join("ucan.json")) } @@ -160,6 +171,48 @@ mod tests { assert_eq!(content["node"].as_str().unwrap(), server.url()); } + /// `gl register` READS the identity from the parent of `GITLAWB_KEY`, so it has + /// to WRITE the bootstrap token there too. Sending it to `~/.gitlawb` instead is + /// silent split-brain: registration prints success, and every command that later + /// reads `ucan.json` — `gl doctor`, `gl ucan show`, `gl init`, `gl mcp` — looks + /// beside the key, finds nothing, and reports an unregistered identity. + #[tokio::test] + async fn register_saves_the_bootstrap_ucan_beside_the_key() { + let dir = TempDir::new().unwrap(); + write_identity(&dir); + // No --dir: the destination has to come from GITLAWB_KEY alone. + let _env = crate::identity::test_env::set_key(Some(dir.path().join("identity.pem"))); + + let mut server = mockito::Server::new_async().await; + let _m = server + .mock("POST", "/api/register") + .with_status(200) + .with_header("content-type", "application/json") + .with_body( + r#"{"message":"Welcome","ucan":"eyJhbGci.test.token","trust_score":0.5,"expires":"2026-12-31"}"#, + ) + .create_async() + .await; + + run(RegisterArgs { + node: server.url(), + capabilities: vec!["git:push".to_string()], + model: None, + dir: None, + }) + .await + .unwrap(); + + let beside_the_key = dir.path().join("ucan.json"); + assert!( + beside_the_key.exists(), + "the bootstrap UCAN must land beside GITLAWB_KEY, not in ~/.gitlawb" + ); + let content: serde_json::Value = + serde_json::from_str(&std::fs::read_to_string(beside_the_key).unwrap()).unwrap(); + assert_eq!(content["ucan"].as_str().unwrap(), "eyJhbGci.test.token"); + } + #[tokio::test] async fn test_register_server_error() { let dir = TempDir::new().unwrap(); From fad75a65797bfadfe549ca2596ae13eac2d3013d Mon Sep 17 00:00:00 2001 From: Vasanthdev2004 Date: Tue, 18 Aug 2026 15:28:10 +0530 Subject: [PATCH 18/18] =?UTF-8?q?fix(gl,core):=20close=20round=20five=20?= =?UTF-8?q?=E2=80=94=20lazy=20home,=20envelope=20reads,=20honest=20registe?= =?UTF-8?q?r?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Five findings from the round-five review, four of them introduced by round four's own centralisation. resolve_key_path and resolve_identity_dir opened with `home_dir()?`, but identity_key_path only needs a home when the key is unset, empty, or `~/`-prefixed. On a host where `dirs::home_dir()` returns None — no HOME and no passwd entry, an ordinary container shape — a perfectly valid absolute GITLAWB_KEY was discarded and the push went out unsigned, blaming the home directory for a setting the operator had got right. The pre-round code resolved an absolute key without consulting home at all, so this was a regression, not an inherited gap. The core resolvers now take `Option<&Path>` and demand a home only where the value being resolved needs one. `gl ucan show` called Ucan::decode on the whole of ucan.json, but register, init, and quickstart all write an envelope — {"ucan", "node", "did", "saved_at"} — and doctor and quickstart already read it as one. Ucan is {payload, s}, so show failed with "missing field `payload`" immediately after a successful `gl register`. It now reads the envelope like every other reader, and still accepts a bare token so files written by an older gl stay readable. The predating mismatch belongs to this round because this round rerouted cmd_show and claimed to unify the identity flow. `gl register` printed "You are now a verified agent" even when the node returned no bootstrap UCAN — the exact shape the previous commit message said it existed to remove, one line below the fix. The closing line is now inside the branch that actually stored a token. The MCP `ucan_show` tool called gitlawb_dir(None) while every sibling tool in call_tool honours the server's --dir, so one session read two identity directories. The parameter was already in scope. Six `--dir` help texts still promised "default: ~/.gitlawb" after the resolver stopped having that default. Both behavioural fixes were verified by mutation: demanding a home unconditionally turns an_absolute_key_resolves_without_a_home_directory red, and dropping the envelope branch turns the_register_envelope_decodes red. --- crates/git-remote-gitlawb/src/main.rs | 14 ++-- crates/gitlawb-core/src/identity_path.rs | 91 +++++++++++++++++++----- crates/gl/src/doctor.rs | 2 +- crates/gl/src/identity.rs | 7 +- crates/gl/src/ipfs_cmd.rs | 2 +- crates/gl/src/mcp.rs | 6 +- crates/gl/src/node.rs | 2 +- crates/gl/src/quickstart.rs | 2 +- crates/gl/src/register.rs | 15 ++-- crates/gl/src/ucan_cmd.rs | 79 +++++++++++++++++++- crates/gl/src/whoami.rs | 2 +- 11 files changed, 185 insertions(+), 37 deletions(-) diff --git a/crates/git-remote-gitlawb/src/main.rs b/crates/git-remote-gitlawb/src/main.rs index 2f71a8dd..0198a210 100644 --- a/crates/git-remote-gitlawb/src/main.rs +++ b/crates/git-remote-gitlawb/src/main.rs @@ -1006,16 +1006,14 @@ fn load_keypair() -> Option { /// helper mid-push, so a misconfiguration is logged and the push continues /// unsigned rather than aborting the transfer. fn resolve_key_path() -> Option { - let home = home_dir()?; - gitlawb_core::identity_path::identity_key_path(&home) + gitlawb_core::identity_path::identity_key_path(home_dir().as_deref()) .inspect_err(|e| tracing::warn!("cannot resolve the identity key path: {e}")) .ok() } /// The directory holding `identity.pem` and `delegations/`. fn resolve_identity_dir() -> Option { - let home = home_dir()?; - gitlawb_core::identity_path::identity_dir(&home) + gitlawb_core::identity_path::identity_dir(home_dir().as_deref()) .inspect_err(|e| tracing::warn!("cannot resolve the identity directory: {e}")) .ok() } @@ -1023,11 +1021,11 @@ fn resolve_identity_dir() -> Option { /// `dirs`, not `$HOME`: the old code fell back to `"."` when `HOME` was unset, /// which on Windows is always, so the default key resolved against whatever /// directory git happened to invoke the helper from. +/// +/// `None` is not fatal — an absolute `GITLAWB_KEY` resolves without it, and only +/// the default and `~/`-prefixed forms need a home at all. fn home_dir() -> Option { - dirs::home_dir().or_else(|| { - tracing::warn!("could not determine the home directory; pushing without a delegation"); - None - }) + dirs::home_dir() } // ── Tests ───────────────────────────────────────────────────────────────────── diff --git a/crates/gitlawb-core/src/identity_path.rs b/crates/gitlawb-core/src/identity_path.rs index 6bfca9fc..64026798 100644 --- a/crates/gitlawb-core/src/identity_path.rs +++ b/crates/gitlawb-core/src/identity_path.rs @@ -34,20 +34,22 @@ pub const DEFAULT_DIR_NAME: &str = ".gitlawb"; /// An empty `GITLAWB_KEY` counts as unset. That is what a shell leaves behind for /// `FOO=` and for an unset variable expanded into a wrapper script, and reading it /// as a path would resolve against the process working directory instead. -pub fn identity_key_path(home: &Path) -> Result { +pub fn identity_key_path(home: Option<&Path>) -> Result { // `var_os`, not `var`: `var` folds a non-UTF-8 value into the same `Err` as // unset, so an operator whose key path is not valid UTF-8 would silently get // the default directory rather than theirs — or an error naming the real // problem. match std::env::var_os(KEY_ENV) { Some(raw) if !raw.is_empty() => resolve_key_value(Path::new(&raw), home), - _ => Ok(home.join(DEFAULT_DIR_NAME).join(KEY_FILE_NAME)), + _ => Ok(require_home(home)? + .join(DEFAULT_DIR_NAME) + .join(KEY_FILE_NAME)), } } /// The directory holding `identity.pem` and `delegations/` — the parent of /// [`identity_key_path`]. -pub fn identity_dir(home: &Path) -> Result { +pub fn identity_dir(home: Option<&Path>) -> Result { let key = identity_key_path(home)?; key.parent().map(Path::to_path_buf).ok_or_else(|| { Error::Key(format!( @@ -57,11 +59,27 @@ pub fn identity_dir(home: &Path) -> Result { }) } +/// The home directory, demanded only where the value being resolved needs it. +/// +/// An absolute `GITLAWB_KEY` never needs home, so requiring it up front would +/// discard a perfectly good key on a host where `dirs::home_dir()` returns `None` +/// — no `HOME` and no passwd entry, which is an ordinary container shape. The +/// helper would then push unsigned, blaming the home directory for a setting the +/// operator had configured correctly. +fn require_home(home: Option<&Path>) -> Result<&Path> { + home.ok_or_else(|| { + Error::Key(format!( + "could not determine the home directory, which is needed to resolve this \ + {KEY_ENV} value. Set {KEY_ENV} to an absolute path to avoid needing it." + )) + }) +} + /// Apply the `GITLAWB_KEY` rules to a raw value. /// /// Split out from [`identity_key_path`] so the rules can be tested without setting /// a process-global environment variable, which would make the tests race. -fn resolve_key_value(raw: &Path, home: &Path) -> Result { +fn resolve_key_value(raw: &Path, home: Option<&Path>) -> Result { let path = expand_tilde(raw, home)?; if !path.is_absolute() { @@ -95,7 +113,7 @@ fn resolve_key_value(raw: &Path, home: &Path) -> Result { /// what lets the value stay an `OsStr` end to end: the helper's old /// `str::strip_prefix("~/")` needed a `String` first, which is why it reached for /// `env::var` and folded every non-UTF-8 path into "unset". -fn expand_tilde(path: &Path, home: &Path) -> Result { +fn expand_tilde(path: &Path, home: Option<&Path>) -> Result { let mut components = path.components(); match components.next() { Some(Component::Normal(first)) if first == OsStr::new("~") => { @@ -107,7 +125,7 @@ fn expand_tilde(path: &Path, home: &Path) -> Result { path.display() ))); } - Ok(home.join(rest)) + Ok(require_home(home)?.join(rest)) } _ => Ok(path.to_path_buf()), } @@ -131,12 +149,12 @@ mod tests { #[test] fn absolute_value_is_taken_verbatim() { let raw = home().join("data").join("keys").join(KEY_FILE_NAME); - assert_eq!(resolve_key_value(&raw, &home()).unwrap(), raw); + assert_eq!(resolve_key_value(&raw, Some(&home())).unwrap(), raw); } #[test] fn tilde_slash_expands_to_the_home_directory() { - let resolved = resolve_key_value(Path::new("~/keys/identity.pem"), &home()).unwrap(); + let resolved = resolve_key_value(Path::new("~/keys/identity.pem"), Some(&home())).unwrap(); assert_eq!(resolved, home().join("keys").join(KEY_FILE_NAME)); } @@ -145,7 +163,8 @@ mod tests { /// shell, and the two binaries used to reach it by different routes. #[test] fn the_tilde_spelling_of_the_default_resolves_to_the_default() { - let resolved = resolve_key_value(Path::new("~/.gitlawb/identity.pem"), &home()).unwrap(); + let resolved = + resolve_key_value(Path::new("~/.gitlawb/identity.pem"), Some(&home())).unwrap(); assert_eq!(resolved, home().join(DEFAULT_DIR_NAME).join(KEY_FILE_NAME)); } @@ -156,7 +175,7 @@ mod tests { fn relative_values_are_refused() { for raw in ["identity.pem", "keys/identity.pem", "./keys/identity.pem"] { assert!( - resolve_key_value(Path::new(raw), &home()).is_err(), + resolve_key_value(Path::new(raw), Some(&home())).is_err(), "{raw} is relative and must be refused" ); } @@ -168,7 +187,7 @@ mod tests { fn unsupported_tilde_forms_are_refused() { for raw in ["~", "~/", "~someone/keys/identity.pem"] { assert!( - resolve_key_value(Path::new(raw), &home()).is_err(), + resolve_key_value(Path::new(raw), Some(&home())).is_err(), "{raw} must be refused rather than resolved" ); } @@ -177,7 +196,7 @@ mod tests { /// The root has no parent, so the delegation store would have nowhere to go. #[test] fn the_filesystem_root_is_refused() { - assert!(resolve_key_value(Path::new("/"), &home()).is_err()); + assert!(resolve_key_value(Path::new("/"), Some(&home())).is_err()); } /// The whole point of `var_os`: a non-UTF-8 value must reach the rules rather @@ -191,14 +210,14 @@ mod tests { let relative = OsString::from_vec(b"keys/\xFF/identity.pem".to_vec()); assert!( - resolve_key_value(Path::new(&relative), &home()).is_err(), + resolve_key_value(Path::new(&relative), Some(&home())).is_err(), "a non-UTF-8 relative path must be refused, not silently defaulted" ); let mut absolute = OsString::from("/data/"); absolute.push(OsString::from_vec(vec![0xFF])); absolute.push("/identity.pem"); - let resolved = resolve_key_value(Path::new(&absolute), &home()).unwrap(); + let resolved = resolve_key_value(Path::new(&absolute), Some(&home())).unwrap(); assert_eq!(resolved.as_os_str(), absolute.as_os_str()); } @@ -215,9 +234,15 @@ mod tests { let restore = std::env::var_os(KEY_ENV); std::env::remove_var(KEY_ENV); - let unset = (identity_key_path(&home()), identity_dir(&home())); + let unset = ( + identity_key_path(Some(&home())), + identity_dir(Some(&home())), + ); std::env::set_var(KEY_ENV, ""); - let empty = (identity_key_path(&home()), identity_dir(&home())); + let empty = ( + identity_key_path(Some(&home())), + identity_dir(Some(&home())), + ); match restore { Some(v) => std::env::set_var(KEY_ENV, v), @@ -238,3 +263,37 @@ mod tests { } } } + +#[cfg(test)] +mod no_home_tests { + use super::*; + + /// An absolute key needs no home directory. Demanding one up front discarded a + /// correctly-configured `GITLAWB_KEY` on any host where `dirs::home_dir()` + /// returns `None` — no `HOME` and no passwd entry, an ordinary container shape — + /// and the helper then pushed unsigned while blaming the home directory. + #[test] + fn an_absolute_key_resolves_without_a_home_directory() { + let raw = if cfg!(windows) { + r"C:\data\keys\identity.pem" + } else { + "/data/keys/identity.pem" + }; + let resolved = resolve_key_value(Path::new(raw), None) + .expect("an absolute key must not need a home directory"); + assert_eq!(resolved, PathBuf::from(raw)); + assert_eq!(resolved.parent().unwrap(), Path::new(raw).parent().unwrap()); + } + + /// The forms that genuinely need a home still say so, rather than resolving + /// somewhere arbitrary. + #[test] + fn the_forms_that_need_a_home_report_its_absence() { + let err = resolve_key_value(Path::new("~/keys/identity.pem"), None) + .expect_err("a ~/ path cannot resolve without a home directory"); + assert!( + err.to_string().contains("home directory"), + "the error must name the missing home directory, got: {err}" + ); + } +} diff --git a/crates/gl/src/doctor.rs b/crates/gl/src/doctor.rs index 91cd40ce..3d51fefc 100644 --- a/crates/gl/src/doctor.rs +++ b/crates/gl/src/doctor.rs @@ -24,7 +24,7 @@ pub struct DoctorArgs { #[arg(long, default_value = PUBLIC_NODE, env = "GITLAWB_NODE")] pub node: String, - /// Identity directory (default: ~/.gitlawb) + /// Identity directory (default: the parent of $GITLAWB_KEY, else ~/.gitlawb) #[arg(long)] pub dir: Option, } diff --git a/crates/gl/src/identity.rs b/crates/gl/src/identity.rs index 3cf97ab7..e1cb2831 100644 --- a/crates/gl/src/identity.rs +++ b/crates/gl/src/identity.rs @@ -80,8 +80,11 @@ pub fn gitlawb_dir(override_dir: Option) -> Result { if let Some(d) = override_dir { return Ok(d); } - let home = dirs::home_dir().context("could not determine the home directory")?; - gitlawb_core::identity_path::identity_dir(&home).map_err(|e| anyhow::anyhow!("{e}")) + // `home_dir()` is passed as an Option rather than demanded here: an absolute + // GITLAWB_KEY resolves without a home directory, and a host that has none is a + // normal container shape, not a reason to refuse a correctly-configured key. + let home = dirs::home_dir(); + gitlawb_core::identity_path::identity_dir(home.as_deref()).map_err(|e| anyhow::anyhow!("{e}")) } fn key_path(dir: &Path) -> PathBuf { diff --git a/crates/gl/src/ipfs_cmd.rs b/crates/gl/src/ipfs_cmd.rs index d4cb64fc..782cd8cd 100644 --- a/crates/gl/src/ipfs_cmd.rs +++ b/crates/gl/src/ipfs_cmd.rs @@ -23,7 +23,7 @@ pub enum IpfsCmd { List { #[arg(long, default_value = "https://node.gitlawb.com", env = "GITLAWB_NODE")] node: String, - /// Identity directory (default: ~/.gitlawb) + /// Identity directory (default: the parent of $GITLAWB_KEY, else ~/.gitlawb) #[arg(long)] dir: Option, }, diff --git a/crates/gl/src/mcp.rs b/crates/gl/src/mcp.rs index d01a99bf..5aaaa070 100644 --- a/crates/gl/src/mcp.rs +++ b/crates/gl/src/mcp.rs @@ -759,7 +759,11 @@ async fn call_tool( ]))?), "ucan_show" => { - let ucan_path = crate::identity::gitlawb_dir(None)?.join("ucan.json"); + // The directory the server was started with, like every sibling tool. + // Reading the default here while the rest honour `--dir` splits one MCP + // session across two identity directories. + let ucan_path = crate::identity::gitlawb_dir(dir.map(std::path::Path::to_path_buf))? + .join("ucan.json"); if ucan_path.exists() { let content = std::fs::read_to_string(ucan_path)?; Ok(content) diff --git a/crates/gl/src/node.rs b/crates/gl/src/node.rs index 367ba576..abd1de98 100644 --- a/crates/gl/src/node.rs +++ b/crates/gl/src/node.rs @@ -22,7 +22,7 @@ pub enum NodeCmd { Status { #[arg(long, default_value = "https://node.gitlawb.com", env = "GITLAWB_NODE")] node: String, - /// Identity directory (default: ~/.gitlawb) + /// Identity directory (default: the parent of $GITLAWB_KEY, else ~/.gitlawb) #[arg(long)] dir: Option, }, diff --git a/crates/gl/src/quickstart.rs b/crates/gl/src/quickstart.rs index 0654df74..725385a1 100644 --- a/crates/gl/src/quickstart.rs +++ b/crates/gl/src/quickstart.rs @@ -23,7 +23,7 @@ pub struct QuickstartArgs { #[arg(long, default_value = PUBLIC_NODE, env = "GITLAWB_NODE")] pub node: String, - /// Identity directory (default: ~/.gitlawb) + /// Identity directory (default: the parent of $GITLAWB_KEY, else ~/.gitlawb) #[arg(long)] pub dir: Option, diff --git a/crates/gl/src/register.rs b/crates/gl/src/register.rs index 94a2f712..466f87d2 100644 --- a/crates/gl/src/register.rs +++ b/crates/gl/src/register.rs @@ -30,7 +30,7 @@ pub struct RegisterArgs { #[arg(long)] pub model: Option, - /// Identity directory (default: ~/.gitlawb) + /// Identity directory (default: the parent of $GITLAWB_KEY, else ~/.gitlawb) #[arg(long)] pub dir: Option, } @@ -105,11 +105,18 @@ pub async fn run(args: RegisterArgs) -> Result<()> { println!(); // The real path, not the default one: `GITLAWB_KEY` moves it, and an operator // told to look in `~/.gitlawb` would find nothing there. + // Registration without a stored token means the registration-gated + // capabilities never arrived, so the closing line must not claim they did. match &saved_to { - Some(path) => println!(" Bootstrap UCAN saved to {}", path.display()), - None => println!(" The node returned no bootstrap UCAN."), + Some(path) => { + println!(" Bootstrap UCAN saved to {}", path.display()); + println!(" You are now a verified agent on the gitlawb network."); + } + None => { + println!(" The node returned no bootstrap UCAN, so this identity is not"); + println!(" a verified agent yet. Re-run `gl register` once the node issues one."); + } } - println!(" You are now a verified agent on the gitlawb network."); Ok(()) } diff --git a/crates/gl/src/ucan_cmd.rs b/crates/gl/src/ucan_cmd.rs index 93450b7f..1d3644fd 100644 --- a/crates/gl/src/ucan_cmd.rs +++ b/crates/gl/src/ucan_cmd.rs @@ -341,7 +341,8 @@ async fn cmd_show(dir: Option) -> Result<()> { } let content = std::fs::read_to_string(&ucan_path)?; - let ucan = Ucan::decode(&content)?; + let ucan = decode_saved_ucan(&content) + .with_context(|| format!("could not read the saved UCAN at {}", ucan_path.display()))?; println!("Issuer: {}", ucan.payload.iss); println!("Audience: {}", ucan.payload.aud); @@ -731,3 +732,79 @@ mod delegation_store_tests { ); } } + +/// Decode the token out of a saved `ucan.json`. +/// +/// `gl register`, `gl init`, and `gl quickstart` all write an envelope — +/// `{"ucan": "", "node": ..., "did": ..., "saved_at": ...}` — and `doctor` +/// and `quickstart` read it back as one. `cmd_show` was the only reader calling +/// `Ucan::decode` on the whole file, and `Ucan` is `{payload, s}`, so it failed +/// with "missing field `payload`" immediately after a successful `gl register`. +/// +/// The bare-token form is still accepted: a file written by an older `gl`, or by +/// hand, should not stop being readable just because the envelope is now canonical. +fn decode_saved_ucan(content: &str) -> Result { + if let Ok(envelope) = serde_json::from_str::(content) { + if let Some(token) = envelope.get("ucan").and_then(|v| v.as_str()) { + return Ucan::decode(token).map_err(Into::into); + } + } + Ucan::decode(content.trim()).map_err(Into::into) +} + +#[cfg(test)] +mod saved_ucan_tests { + use super::*; + + fn a_token() -> String { + let kp = gitlawb_core::identity::Keypair::generate(); + let aud = gitlawb_core::identity::Keypair::generate(); + Ucan::issue( + &kp, + aud.did(), + vec![Capability::new("*", caps::GIT_PUSH)], + None, + ) + .unwrap() + .encode() + .unwrap() + } + + /// The shape `gl register`, `gl init`, and `gl quickstart` all write, and the + /// shape `doctor` and `quickstart` already read back. `cmd_show` used to call + /// `Ucan::decode` on the whole file and failed with "missing field `payload`" + /// immediately after a successful `gl register`. + #[test] + fn the_register_envelope_decodes() { + let token = a_token(); + let envelope = serde_json::json!({ + "ucan": token, + "node": "https://node.gitlawb.com", + "did": "did:key:z6MkAbc", + "saved_at": "2026-08-17T00:00:00Z", + }) + .to_string(); + + let decoded = decode_saved_ucan(&envelope).expect("the written envelope must decode"); + assert_eq!(decoded.encode().unwrap(), token); + } + + /// A file written by an older `gl`, or by hand, stays readable. + #[test] + fn a_bare_token_still_decodes() { + let token = a_token(); + assert_eq!( + decode_saved_ucan(&format!(" {token}\n")) + .expect("a bare token must still decode") + .encode() + .unwrap(), + token + ); + } + + #[test] + fn neither_shape_swallows_garbage() { + assert!(decode_saved_ucan("not a ucan").is_err()); + assert!(decode_saved_ucan(r#"{"node":"x"}"#).is_err()); + } +} diff --git a/crates/gl/src/whoami.rs b/crates/gl/src/whoami.rs index 66c1438c..699e8875 100644 --- a/crates/gl/src/whoami.rs +++ b/crates/gl/src/whoami.rs @@ -10,7 +10,7 @@ use crate::identity::load_keypair_from_dir; #[derive(Args)] pub struct WhoamiArgs { - /// Identity directory (default: ~/.gitlawb) + /// Identity directory (default: the parent of $GITLAWB_KEY, else ~/.gitlawb) #[arg(long)] dir: Option, /// Node URL to query for registration info