michal/tit

Browse tree · Show commit · Download archive

Diff

8cf42ce97a49 → b70485ede680

@@ -435,3 +435,17 @@
 migration. Read the
 [pull-request merge architectural decision record](docs/adr/0025-pull-request-merge.md)
 for the merge, permission, intent, and recovery contracts.
+
+## Milestone 5.5 gate
+
+Run the branch-rule gate:
+
+```text
+./scripts/check-m5-5
+```
+
+This command tests protected-ref access, fast-forward-only branches,
+force-push rejection, protected-ref deletion rejection, topic-branch access,
+and pull-request merge permission. Read the
+[branch-rule architectural decision record](docs/adr/0026-branch-rules.md) for
+the ref, role, transport, and merge contracts.

@@ -20,8 +20,8 @@
 heads, stale revisions, and a method that does not match the mergeability
 state.
 
-Only a repository owner or maintainer can merge in this milestone. Milestone
-5.5 will move this rule into the common branch-policy service.
+Only a repository owner or maintainer can merge. The common repository policy
+service enforces this rule.
 
 Create a merge commit without a worktree. Use `gix` rename detection and tree
 merge code. Keep Git modes and byte paths. Use the base commit as the first

@@ -1,0 +1,75 @@
+# Architectural decision record 0026: Branch rules
+
+Status: Accepted
+
+Date: 2026-07-23
+
+## Context
+
+Git transport and pull-request merge code had separate authorization checks.
+This made it possible for the two paths to apply different rules. A repository
+writer also had permission to update `refs/heads/main` directly.
+
+## Decision
+
+Use `RepositoryPolicy` as the common service for ref changes and pull-request
+merges. The service reads the current repository state and collaborator role
+for each operation. Do not cache this decision.
+
+Apply these rules:
+
+- `refs/heads/main` is the protected ref.
+- An owner or maintainer can create or fast-forward the protected ref.
+- A writer cannot change the protected ref.
+- No role can delete the protected ref.
+- A branch update must be a fast-forward. No role can force-push a branch.
+- An owner, maintainer, or writer can create, fast-forward, or delete a topic
+  branch.
+- An owner, maintainer, or writer can create, update, or delete a tag.
+- Only an owner or maintainer can merge a pull request.
+
+The receive-pack service classifies each proposed ref change after it validates
+the objects and before it writes an intent or makes an object visible. It sends
+each classification to `RepositoryPolicy`. An unmanaged feasibility test
+service keeps its original branch fast-forward rule.
+
+The pull-request service asks `RepositoryPolicy` for merge permission before it
+prepares a merge. The SQLite intent transaction checks the current role again.
+This second check prevents a merge when a collaborator loses permission during
+merge preparation.
+
+This milestone does not add configurable branch-rule records. The first stable
+release has one protected default ref, and all repositories use
+`refs/heads/main` as that ref. A subsequent milestone must add stored rule
+configuration before it lets an administrator select a different protected
+ref.
+
+## Failure and threat cases
+
+Reject a policy failure before object promotion and before intent creation.
+Return an accurate non-fast-forward status for a forced branch update. Return a
+reference-policy status for a protected-ref or role failure.
+
+Check all commands in an atomic push before an update starts. One rejected
+command rejects the complete push. Read the account, repository, and
+collaborator state from SQLite for the operation so a suspended account,
+archived repository, or removed role takes effect immediately.
+
+## Evidence
+
+The policy test checks owner, maintainer, and writer behavior for the protected
+ref, a topic branch, tags, force-push, deletion, and merge permission.
+
+The production SSH test uses a stock Git client. It checks that a writer can
+create, update, and delete a topic branch but cannot update the protected ref.
+It checks that an owner can fast-forward the protected ref but cannot delete it
+or force-push a topic branch. Existing receive-pack tests check SHA-1 and
+SHA-256 fast-forward rules and atomic rejection. Pull-request tests check that
+a writer cannot merge and that an owner can merge.
+
+## Consequences
+
+All production ref changes and pull-request merges use one policy service.
+Repositories cannot yet select a protected ref other than
+`refs/heads/main`. Tags remain mutable, and users with write permission can
+delete topic branches and tags.

@@ -1,0 +1,8 @@
+#!/bin/sh
+set -eu
+
+./scripts/check
+cargo test --locked --release --test repository_policy
+cargo test --locked --release --test git_push_ssh
+cargo test --locked --release --test serve enforces_account_roles_and_ref_policy_through_the_production_ssh_server
+cargo test --locked --release --test pull_requests fast_forwards_pull_requests_with_one_durable_merge_event_for_both_hashes

@@ -16,6 +16,7 @@
 
 use super::packetline::{Packet, PacketLineError, decode, encode_data, encode_flush};
 use super::upload_pack::hash_name;
+use crate::policy::{PolicyError, RefChange, RepositoryPolicy};
 use crate::store::{GitIntentRecord, GitOperationIntent, NewAuditEvent, Store};
 
 const MAX_COMMANDS: usize = 256;
@@ -35,6 +36,7 @@
     quarantine: PathBuf,
     incoming_pack: PathBuf,
     cleanup_on_drop: bool,
+    policy_context: Option<(String, String)>,
 }
 
 #[derive(Clone, Debug, Eq, PartialEq)]
@@ -49,6 +51,30 @@
         repository_path: &Path,
         database_path: &Path,
         actor: String,
+    ) -> Result<Self, ReceivePackError> {
+        Self::open_inner(repository_path, database_path, actor, None)
+    }
+
+    pub(crate) fn open_authorized(
+        repository_path: &Path,
+        database_path: &Path,
+        actor: String,
+        owner: String,
+        repository: String,
+    ) -> Result<Self, ReceivePackError> {
+        Self::open_inner(
+            repository_path,
+            database_path,
+            actor,
+            Some((owner, repository)),
+        )
+    }
+
+    fn open_inner(
+        repository_path: &Path,
+        database_path: &Path,
+        actor: String,
+        policy_context: Option<(String, String)>,
     ) -> Result<Self, ReceivePackError> {
         let repository = open_bare(repository_path)?;
         let object_format = repository.object_hash();
@@ -68,6 +94,7 @@
             quarantine,
             incoming_pack,
             cleanup_on_drop: true,
+            policy_context,
         })
     }
 
@@ -159,13 +186,11 @@
         let result = (|| {
             let has_pack =
                 self.incoming_pack.exists() && fs::metadata(&self.incoming_pack)?.len() != 0;
-            let needs_objects = commands.iter().any(|command| !command.new.is_null());
             let pack_name = if has_pack {
                 self.index_and_validate_pack(&repository, &commands)?
             } else {
-                if needs_objects {
-                    validate_proposed_objects(&repository, None, &commands)?;
-                }
+                let changes = validate_proposed_objects(&repository, None, &commands)?;
+                self.validate_policy(&commands, &changes)?;
                 None
             };
 
@@ -295,7 +320,8 @@
             return Err(ReceivePackError::ObjectLimit);
         }
         if outcome.index.num_objects == 0 {
-            validate_proposed_objects(repository, None, commands)?;
+            let changes = validate_proposed_objects(repository, None, commands)?;
+            self.validate_policy(commands, &changes)?;
             if let Some(path) = outcome.data_path {
                 let _ = fs::remove_file(path);
             }
@@ -312,7 +338,8 @@
             .ok_or(ReceivePackError::MissingPack)?
             .map_err(|error| ReceivePackError::Pack(error.to_string()))?;
         validate_delta_depth(&bundle)?;
-        validate_proposed_objects(repository, Some(&bundle), commands)?;
+        let changes = validate_proposed_objects(repository, Some(&bundle), commands)?;
+        self.validate_policy(commands, &changes)?;
 
         let source_pack = outcome.data_path.ok_or(ReceivePackError::MissingPack)?;
         let source_index = outcome.index_path.ok_or(ReceivePackError::MissingPack)?;
@@ -349,6 +376,41 @@
         }
         sync_directory(&destination)?;
         Ok(promoted.then_some(destination_pack))
+    }
+
+    fn validate_policy(
+        &self,
+        commands: &[RefCommand],
+        changes: &[RefChange],
+    ) -> Result<(), ReceivePackError> {
+        let Some((owner, repository)) = &self.policy_context else {
+            if changes
+                .iter()
+                .any(|change| matches!(change, RefChange::Force))
+            {
+                return Err(ReceivePackError::NonFastForward);
+            }
+            return Ok(());
+        };
+        let policy = RepositoryPolicy::new(&self.database_path);
+        for (command, change) in commands.iter().zip(changes) {
+            let authorization = policy.authorize_ref_change(
+                &self.actor,
+                owner,
+                repository,
+                command.name.as_bstr(),
+                *change,
+            );
+            match authorization {
+                Ok(_) => {}
+                Err(PolicyError::Denied) if matches!(change, RefChange::Force) => {
+                    return Err(ReceivePackError::NonFastForward);
+                }
+                Err(PolicyError::Denied) => return Err(ReceivePackError::RefPolicy),
+                Err(error) => return Err(ReceivePackError::Policy(error)),
+            }
+        }
+        Ok(())
     }
 }
 
@@ -605,10 +667,12 @@
     repository: &gix::Repository,
     bundle: Option<&gix_pack::Bundle>,
     commands: &[RefCommand],
-) -> Result<(), ReceivePackError> {
+) -> Result<Vec<RefChange>, ReceivePackError> {
     let mut finder = CombinedObjects::new(repository, bundle);
+    let mut changes = Vec::with_capacity(commands.len());
     for command in commands {
         if command.new.is_null() {
+            changes.push(RefChange::Delete);
             continue;
         }
         let kind = finder.kind_and_links(command.new)?.0;
@@ -616,14 +680,18 @@
             return Err(ReceivePackError::BranchNotCommit);
         }
         finder.validate_reachable(command.new)?;
-        if !command.old.is_null()
-            && command.name.as_bstr().starts_with(b"refs/heads/")
-            && !finder.is_ancestor(command.old, command.new)?
-        {
-            return Err(ReceivePackError::NonFastForward);
-        }
+        let change = if command.old.is_null() {
+            RefChange::Create
+        } else if command.name.as_bstr().starts_with(b"refs/tags/") {
+            RefChange::TagUpdate
+        } else if finder.is_ancestor(command.old, command.new)? {
+            RefChange::FastForward
+        } else {
+            RefChange::Force
+        };
+        changes.push(change);
     }
-    Ok(())
+    Ok(changes)
 }
 
 struct CombinedObjects<'a> {
@@ -889,6 +957,10 @@
     BranchNotCommit,
     #[error("a branch update is not a fast-forward")]
     NonFastForward,
+    #[error("reference policy rejects the update")]
+    RefPolicy,
+    #[error("cannot check reference policy: {0}")]
+    Policy(PolicyError),
     #[error("cannot update Git references: {0}")]
     RefTransaction(String),
     #[error("incomplete Git operation {0} has mixed reference state")]
@@ -922,6 +994,8 @@
         match self {
             Self::StaleRef => "stale reference",
             Self::NonFastForward => "non-fast-forward",
+            Self::RefPolicy => "reference policy rejected the update",
+            Self::Policy(_) => "reference policy is unavailable",
             Self::BranchNotCommit => "branch target is not a commit",
             Self::RefName => "reference name is not allowed",
             Self::ObjectFormat => "object format does not match",

@@ -32,6 +32,46 @@
         }
     }
 
+    pub(crate) fn authorize_ref_change(
+        &self,
+        actor: &str,
+        owner: &str,
+        repository: &str,
+        ref_name: &[u8],
+        change: RefChange,
+    ) -> Result<RepositoryRecord, PolicyError> {
+        let record = Store::open(&self.database)?.repository_authorization(
+            owner,
+            repository,
+            Some(actor),
+        )?;
+        if !allows(&record, RepositoryOperation::Write)? {
+            return Err(PolicyError::Denied);
+        }
+        let protected = ref_name == b"refs/heads/main";
+        if matches!(change, RefChange::Force)
+            || (protected && matches!(change, RefChange::Delete))
+            || (protected && !allows(&record, RepositoryOperation::Maintain)?)
+        {
+            return Err(PolicyError::Denied);
+        }
+        Ok(record.repository)
+    }
+
+    pub(crate) fn authorize_merge(
+        &self,
+        actor: &str,
+        owner: &str,
+        repository: &str,
+    ) -> Result<RepositoryRecord, PolicyError> {
+        self.authorize(
+            Some(actor),
+            owner,
+            repository,
+            RepositoryOperation::Maintain,
+        )
+    }
+
     #[allow(
         dead_code,
         reason = "policy tests verify anonymous catalog behavior independently"
@@ -53,6 +93,15 @@
             })
             .collect()
     }
+}
+
+#[derive(Clone, Copy, Debug, Eq, PartialEq)]
+pub(crate) enum RefChange {
+    Create,
+    FastForward,
+    Force,
+    Delete,
+    TagUpdate,
 }
 
 #[derive(Clone, Copy, Debug, Eq, PartialEq)]

@@ -13,6 +13,7 @@
     Comparison, ReadCancellation, ReadError, ReadLimits, RepositoryReadService,
 };
 use crate::git::repository::{GitRepository, GitRepositoryError};
+use crate::policy::{PolicyError, RepositoryPolicy};
 use crate::store::{
     GitOperationIntent, NewPullRequestMerge, NewPullRequestRefIntent, NewPullRequestReview,
     PullRequestDetail, PullRequestRecord, PullRequestRefIntentRecord, PullRequestRevisionRecord,
@@ -353,9 +354,12 @@
         if detail.pull_request.state != "open" {
             return Err(StoreError::PullRequestState.into());
         }
-        if !detail.can_merge {
-            return Err(StoreError::PullRequestDenied.into());
-        }
+        RepositoryPolicy::new(&self.database)
+            .authorize_merge(actor, owner, repository)
+            .map_err(|error| match error {
+                PolicyError::Denied => PullRequestError::Store(StoreError::PullRequestDenied),
+                other => PullRequestError::Policy(other),
+            })?;
         let revision = detail.revisions.last().ok_or(PullRequestError::Revision)?;
         let path = self.repository_path(&detail.repository.id)?;
         let git = GitRepository::open(&path)?;
@@ -715,6 +719,8 @@
     Io(#[from] std::io::Error),
     #[error(transparent)]
     Read(#[from] ReadError),
+    #[error(transparent)]
+    Policy(#[from] PolicyError),
     #[error(transparent)]
     ReceivePack(#[from] crate::git::receive_pack::ReceivePackError),
     #[error("cannot create a random pull-request ID")]

@@ -1399,10 +1399,21 @@
                 let database = repositories.push_database()?.to_owned();
                 let actor = identity.username.clone();
                 let public_key = self.authenticated_key.clone()?;
+                let uses_policy = repositories.uses_policy();
                 let permit = repositories.blocking_permit().await.ok()?;
                 tokio::task::spawn_blocking(move || {
                     let _permit = permit;
-                    let service = ReceivePack::open(&path, &database, actor)?;
+                    let service = if uses_policy {
+                        ReceivePack::open_authorized(
+                            &path,
+                            &database,
+                            actor,
+                            owner.clone(),
+                            repository.clone(),
+                        )?
+                    } else {
+                        ReceivePack::open(&path, &database, actor)?
+                    };
                     let advertisement = service.advertisement()?;
                     Ok::<_, ReceivePackError>(InitialGitService::Receive(Box::new(
                         InitialReceiveService {

@@ -270,6 +270,19 @@
             service.merge("alice", "project", 1, "bob", "fast-forward"),
             Err(PullRequestError::Store(StoreError::PullRequestDenied))
         ));
+        Store::open(&fixture.database)
+            .expect("open the collaborator store")
+            .connection()
+            .execute(
+                "UPDATE repository_collaborator SET role = 'writer'
+                 WHERE account_id = (SELECT id FROM account WHERE username = 'bob')",
+                [],
+            )
+            .expect("make the collaborator a writer");
+        assert!(matches!(
+            service.merge("alice", "project", 1, "bob", "fast-forward"),
+            Err(PullRequestError::Store(StoreError::PullRequestDenied))
+        ));
 
         let merged = service
             .merge("alice", "project", 1, "alice", "fast-forward")

@@ -4,7 +4,7 @@
 #[path = "../src/store/mod.rs"]
 mod store;
 
-use policy::{PolicyError, RepositoryOperation, RepositoryPolicy};
+use policy::{PolicyError, RefChange, RepositoryOperation, RepositoryPolicy};
 use store::{AuditContext, NewRepository, RepositoryOrigin, Store, StoreError};
 use tempfile::TempDir;
 
@@ -159,6 +159,108 @@
     for operation in operations() {
         assert_denied(&policy, Some("owner"), operation);
     }
+}
+
+#[test]
+fn applies_common_protected_ref_and_merge_rules() {
+    let directory = TempDir::new().expect("create a ref-policy fixture directory");
+    let database = directory.path().join("tit.sqlite3");
+    let mut store = Store::open(&database).expect("create the ref-policy database");
+    for (id, username) in [(1, "owner"), (2, "maintainer"), (3, "writer")] {
+        store
+            .connection()
+            .execute(
+                "INSERT INTO account (id, username, is_administrator, state, created_at)
+                 VALUES (?1, ?2, 0, 'active', 1)",
+                rusqlite::params![id, username],
+            )
+            .expect("create a ref-policy account");
+    }
+    store
+        .create_repository(&NewRepository {
+            id: "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa",
+            owner: "owner",
+            slug: "project",
+            object_format: "sha1",
+            created_at: 2,
+            origin: RepositoryOrigin::Created,
+            initial_references: &[],
+            actor: "admin-cli",
+            correlation_id: "test",
+        })
+        .expect("create a ref-policy repository");
+    for (username, role) in [("maintainer", "maintainer"), ("writer", "writer")] {
+        store
+            .set_repository_collaborator("owner", "project", username, role, &audit(3))
+            .expect("set a ref-policy collaborator");
+    }
+    let policy = RepositoryPolicy::new(&database);
+    for actor in ["owner", "maintainer"] {
+        policy
+            .authorize_ref_change(
+                actor,
+                "owner",
+                "project",
+                b"refs/heads/main",
+                RefChange::FastForward,
+            )
+            .expect("allow a maintainer fast-forward on main");
+        policy
+            .authorize_merge(actor, "owner", "project")
+            .expect("allow a maintainer merge");
+    }
+    assert!(matches!(
+        policy.authorize_ref_change(
+            "writer",
+            "owner",
+            "project",
+            b"refs/heads/main",
+            RefChange::FastForward,
+        ),
+        Err(PolicyError::Denied)
+    ));
+    assert!(matches!(
+        policy.authorize_ref_change(
+            "owner",
+            "owner",
+            "project",
+            b"refs/heads/main",
+            RefChange::Delete,
+        ),
+        Err(PolicyError::Denied)
+    ));
+    assert!(matches!(
+        policy.authorize_ref_change(
+            "owner",
+            "owner",
+            "project",
+            b"refs/heads/topic",
+            RefChange::Force,
+        ),
+        Err(PolicyError::Denied)
+    ));
+    policy
+        .authorize_ref_change(
+            "writer",
+            "owner",
+            "project",
+            b"refs/heads/topic",
+            RefChange::Create,
+        )
+        .expect("allow a writer topic branch");
+    policy
+        .authorize_ref_change(
+            "writer",
+            "owner",
+            "project",
+            b"refs/tags/v1",
+            RefChange::TagUpdate,
+        )
+        .expect("allow a writer tag update");
+    assert!(matches!(
+        policy.authorize_merge("writer", "owner", "project"),
+        Err(PolicyError::Denied)
+    ));
 }
 
 fn operations() -> [RepositoryOperation; 4] {

@@ -995,12 +995,34 @@
     ));
 
     let writer_clone = instance.path().join("writer-private");
+    command(&writer_clone, ["switch", "-q", "-c", "topic"]);
     fs::write(writer_clone.join("writer.txt"), b"writer update\n").expect("write a writer change");
     git_commit(&writer_clone, "writer update");
-    assert!(git_push(&writer_key, &writer_clone, &["main"]).success());
+    assert!(git_push(&writer_key, &writer_clone, &["topic"]).success());
+    fs::write(writer_clone.join("writer-2.txt"), b"second writer update\n")
+        .expect("write a second writer change");
+    git_commit(&writer_clone, "second writer update");
+    assert!(git_push(&writer_key, &writer_clone, &["topic"]).success());
+    assert!(git_push(&writer_key, &writer_clone, &["--delete", "topic"]).success());
+    assert!(!git_push(&writer_key, &writer_clone, &["HEAD:main"]).success());
     assert!(!git_push(&writer_key, &writer_clone, &["HEAD:refs/notes/test"]).success());
-    command(&writer_clone, ["reset", "--hard", "HEAD~1"]);
-    assert!(!git_push(&writer_key, &writer_clone, &["--force", "main"]).success());
+
+    let owner_clone = instance.path().join("owner-private");
+    fs::write(owner_clone.join("owner.txt"), b"owner update\n").expect("write an owner change");
+    git_commit(&owner_clone, "owner update");
+    assert!(git_push(&owner_key, &owner_clone, &["main"]).success());
+    assert!(!git_push(&owner_key, &owner_clone, &["--delete", "main"]).success());
+    command(&owner_clone, ["switch", "-q", "-c", "force-test"]);
+    fs::write(owner_clone.join("force.txt"), b"first history\n").expect("write a branch change");
+    git_commit(&owner_clone, "first branch history");
+    assert!(git_push(&owner_key, &owner_clone, &["force-test"]).success());
+    command(&owner_clone, ["reset", "--hard", "HEAD~1"]);
+    fs::write(owner_clone.join("force.txt"), b"replacement history\n")
+        .expect("write replacement history");
+    git_commit(&owner_clone, "replacement branch history");
+    let force_result = git_push_output(&owner_key, &owner_clone, &["--force", "force-test"]);
+    assert!(!force_result.status.success());
+    assert!(String::from_utf8_lossy(&force_result.stderr).contains("non-fast-forward"));
 
     let reader_clone = instance.path().join("reader-write-private");
     assert!(ssh_clone_repository_succeeds(
@@ -1014,7 +1036,7 @@
     git_commit(&reader_clone, "reader update");
     assert!(!git_push(&reader_key, &reader_clone, &["main"]).success());
 
-    command(&writer_clone, ["reset", "--hard", "origin/main"]);
+    command(&writer_clone, ["switch", "-q", "main"]);
     fs::write(writer_clone.join("removed-role.txt"), b"removed role\n")
         .expect("write a change before role removal");
     git_commit(&writer_clone, "change before role removal");
@@ -1028,7 +1050,14 @@
         )
         .expect("remove the writer role");
     drop(database);
-    assert!(!git_push(&writer_key, &writer_clone, &["main"]).success());
+    assert!(
+        !git_push(
+            &writer_key,
+            &writer_clone,
+            &["HEAD:refs/heads/removed-role"]
+        )
+        .success()
+    );
     server.terminate();
     let database = rusqlite::Connection::open(instance.path().join("tit.sqlite3"))
         .expect("open the push audit database");
@@ -1048,7 +1077,7 @@
             |row| row.get(0),
         )
         .expect("count failed push audit events");
-    assert_eq!(successful, 1);
+    assert_eq!(successful, 3);
     assert!(failed >= 2);
 }
 
@@ -1505,6 +1534,10 @@
 }
 
 fn git_push(private_key: &Path, worktree: &Path, refspecs: &[&str]) -> ExitStatus {
+    git_push_output(private_key, worktree, refspecs).status
+}
+
+fn git_push_output(private_key: &Path, worktree: &Path, refspecs: &[&str]) -> Output {
     let ssh_command = format!(
         "ssh -F /dev/null -i {} -o IdentitiesOnly=yes -o StrictHostKeyChecking=no -o UserKnownHostsFile=/dev/null",
         private_key.display()
@@ -1515,7 +1548,7 @@
         .args(refspecs)
         .env("GIT_SSH_COMMAND", ssh_command)
         .current_dir(worktree)
-        .status()
+        .output()
         .expect("push through the tit SSH server")
 }