diff --git a/ROADMAP.md b/ROADMAP.md index 69a7b599..4f28da4b 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -705,7 +705,8 @@ capture) through F6 (acceptance on the first deployment); F0, F1 (server skeleton, users, organizations, tokens, `admin` CLI), F2a (repositories API, lifecycle and reconciler, collaborators, branches) and F2b (smart-HTTP git transport via `git http-backend`, `pre-receive` ref policy, admission control, threat-model pass #1) and F3a (content reads: refs, tags, commits, -trees, contents, archives) and F3b (contents writes: create, update, delete as server-made commits) are built. Hosted repos are +trees, contents, archives) and F3b (contents writes: create, update, delete as server-made commits) and F4a (branch protection +enforced on push, contents API and branch delete; commit statuses) are built. Hosted repos are bare Git and get none of the `.sc/` features. Other follow-up work is tracked as GitHub issues and listed under Deferred diff --git a/crates/forge/conformance/departures.json b/crates/forge/conformance/departures.json index c7d75b88..22d1d980 100644 --- a/crates/forge/conformance/departures.json +++ b/crates/forge/conformance/departures.json @@ -349,7 +349,7 @@ "expect": { "status": 404, "message_contains": [ - "not found" + "couldn't be found" ] }, "reason": "Gitea answers 500 for an unknown sha; sc-forge answers 404" @@ -362,7 +362,7 @@ "expect": { "status": 404, "message_contains": [ - "not found" + "couldn't be found" ] }, "reason": "Gitea answers 500 for an unknown sha; sc-forge answers 404" @@ -1285,7 +1285,7 @@ "status": 404, "ignore_body": true, "message_contains": [ - "not found" + "couldn't be found" ], "ignore_headers": [ "link", @@ -1441,5 +1441,89 @@ "value": "" }, "reason": "HEAD stays on the unborn default branch, so ls-remote --symref HEAD prints nothing (Gitea moved HEAD to the first pushed branch)" + }, + { + "scenario": "H", + "exchange": 37, + "kind": "http", + "match": "POST /api/v1/repos/acme/status/statuses/", + "expect": { + "status": 422, + "message_contains": [ + "state" + ], + "ignore_body": true + }, + "reason": "sc-forge requires a state: a status without one is 422 [ignore_body: the recorded 201 body has another shape than the departed answer]" + }, + { + "scenario": "H", + "exchange": 50, + "kind": "http", + "match": "GET /api/v1/repos/acme/status/statuses/main?limit=100", + "expect": { + "status": 200, + "ignore_body": true, + "fields": { + "$len": 0 + }, + "ignore_headers": [ + "link", + "x-total-count" + ] + }, + "reason": "the only status on this commit was the refused bogus state, so the list is empty; ignore_body: the recorded body is a one-element list" + }, + { + "scenario": "H", + "exchange": 1, + "kind": "note", + "match": "combined_state_no_statuses", + "expect": { + "value": "pending" + }, + "reason": "existing commit with no statuses: combined state pending (1.26.2 behaviour); 1.22.6's empty string is an empty answer" + }, + { + "scenario": "I", + "exchange": 7, + "kind": "git", + "match": "push-protected-write-collab", + "expect": { + "ok": false, + "message_contains": [ + "protected branch", + "pre-receive hook declined" + ] + }, + "reason": "sc-forge's pre-receive words the refusal 'refusing to update protected branch : direct pushes are not allowed'; Gitea says 'gitea: not allowed to push to protected branch '. Both end in 'pre-receive hook declined'" + }, + { + "scenario": "I", + "exchange": 8, + "kind": "git", + "match": "push-protected-admin", + "expect": { + "ok": false, + "message_contains": [ + "protected branch", + "pre-receive hook declined" + ] + }, + "reason": "sc-forge's pre-receive words the refusal 'refusing to update protected branch : direct pushes are not allowed'; Gitea says 'gitea: not allowed to push to protected branch '. Both end in 'pre-receive hook declined'" + }, + { + "scenario": "I", + "exchange": 10, + "kind": "git", + "match": "delete-protected-by-push", + "expect": { + "ok": false, + "message_contains": [ + "default branch", + "pre-receive hook declined" + ] + }, + "reason": "sc-forge's hook words the refusal 'refusing to delete the default branch'; Gitea says 'gitea: branch main is the default branch and cannot be deleted'. Both end in 'pre-receive hook declined'" } ] diff --git a/crates/forge/conformance/forge-check.sh b/crates/forge/conformance/forge-check.sh index 5a5a6d59..f61beb6b 100755 --- a/crates/forge/conformance/forge-check.sh +++ b/crates/forge/conformance/forge-check.sh @@ -25,4 +25,4 @@ for _ in $(seq 50); do done python3 -I "$here/run.py" check --base-url "http://127.0.0.1:$port" --admin-token "$token" \ --fixtures "$here/fixtures/1.22.6" --departures "$here/departures.json" --pending "$here/slice-pending.json" \ - --only A,B,C,D,E,J,K,L,M,N,R,V "$@" || { echo "--- server log"; cat "$data/serve.log"; exit 1; } + --only A,B,C,D,E,H,I,J,K,L,M,N,Q,R,V,W "$@" || { echo "--- server log"; cat "$data/serve.log"; exit 1; } diff --git a/crates/forge/conformance/slice-pending.json b/crates/forge/conformance/slice-pending.json index b1698cbd..f4c474b9 100644 --- a/crates/forge/conformance/slice-pending.json +++ b/crates/forge/conformance/slice-pending.json @@ -1,12 +1,4 @@ { - "B": { - "defer": [ - [ - 30, - "POST /api/v1/repos/alice/scoped/statuses/" - ] - ] - }, "D": { "defer": [ [ @@ -26,5 +18,11 @@ "GET /api/v1/orgs/acme/repos?limit=50" ] ] + }, + "Q": { + "stop": [ + 5, + "expected [201]" + ] } } diff --git a/crates/forge/src/api.rs b/crates/forge/src/api.rs index e46b53ea..a21a368a 100644 --- a/crates/forge/src/api.rs +++ b/crates/forge/src/api.rs @@ -148,6 +148,22 @@ pub(crate) fn paged_with( extra: &[(&str, String)], items: Vec, ) -> Resp { + let mut r = Resp::json(200, &Value::Array(items)).with("x-total-count", total.to_string()); + if let Some(l) = link_header(ctx, path, total, page, limit, extra) { + r = r.with("link", l); + } + r +} + +/// Gitea's `Link` header value for page `page` of `total` rows, or `None` when there is one page. +pub(crate) fn link_header( + ctx: &Ctx, + path: &str, + total: i64, + page: i64, + limit: i64, + extra: &[(&str, String)], +) -> Option { let tail: String = extra.iter().map(|(k, v)| format!("&{k}={v}")).collect(); let last = ((total + limit - 1) / limit).max(1); let url = |p: i64, rel: &str| { @@ -165,11 +181,7 @@ pub(crate) fn paged_with( links.push(url(1, "first")); links.push(url(page - 1, "prev")); } - let mut r = Resp::json(200, &Value::Array(items)).with("x-total-count", total.to_string()); - if !links.is_empty() { - r = r.with("link", links.join(",")); - } - r + (!links.is_empty()).then(|| links.join(",")) } /// Like [`paged`], but a page link repeats *every* request parameter, alphabetically by name (what diff --git a/crates/forge/src/cgi.rs b/crates/forge/src/cgi.rs index 5cc5a5bb..c3a32b6d 100644 --- a/crates/forge/src/cgi.rs +++ b/crates/forge/src/cgi.rs @@ -496,6 +496,7 @@ fn environment(l: &Launch, marker: &std::path::Path) -> Vec<(String, String)> { var("GIT_HTTP_MAX_REQUEST_BUFFER", cfg.max_request.to_string()); var("FORGE_DATA_DIR", git.data_dir().display().to_string()); var("FORGE_REPO_ID", id.to_string()); + var("FORGE_GIT_BIN", git.bin().display().to_string()); var("FORGE_USER_ID", l.plan.user.id.to_string()); var("FORGE_ACCEPT_MARKER", marker.display().to_string()); env diff --git a/crates/forge/src/content_write.rs b/crates/forge/src/content_write.rs index 7160bce5..9ee42ab7 100644 --- a/crates/forge/src/content_write.rs +++ b/crates/forge/src/content_write.rs @@ -250,7 +250,12 @@ fn run(ctx: &Ctx, a: &Access, row: &Repo, w: &Write) -> Resp { let new_branch = w.new_branch.clone().filter(|n| *n != base_name); let target_name = new_branch.clone().unwrap_or_else(|| base_name.clone()); // Gitea checks protection before it looks at the tree. - if policy::check_contents_write(&target_name, new_branch.is_some()).is_err() { + // The rules are read here, under the write gate, so a rule change is ordered against this write. + let rules = match crate::protection::load(ctx.conn, a.repo.id) { + Ok(r) => r, + Err(e) => return ctx.app.internal(&e), + }; + if policy::check_contents_write(&rules, &target_name, new_branch.is_some()).is_err() { return denied(ctx); } let comps: Vec<&str> = w.path.split('/').collect(); @@ -387,7 +392,11 @@ fn run(ctx: &Ctx, a: &Access, row: &Repo, w: &Write) -> Resp { return error(&ctx.app.base, 503, "the write took too long, try again"); } // The linearization point of branch protection: just before the ref moves. - if policy::check_contents_write(&target_name, creating).is_err() { + let rules = match crate::protection::load(ctx.conn, a.repo.id) { + Ok(r) => r, + Err(e) => return ctx.app.internal(&e), + }; + if policy::check_contents_write(&rules, &target_name, creating).is_err() { return denied(ctx); } let failure = match gw::update_ref(git, &d, &full, &commit, &expected) { diff --git a/crates/forge/src/db.rs b/crates/forge/src/db.rs index 0d56eff5..7119119d 100644 --- a/crates/forge/src/db.rs +++ b/crates/forge/src/db.rs @@ -70,11 +70,49 @@ const MIGRATIONS: &[&str] = &[ mode TEXT NOT NULL CHECK (mode IN ('read', 'write', 'admin')), PRIMARY KEY (repo_id, user_id) );", + // 3: branch protection rules (exact branch names; `lower_branch` is the Unicode-lowercased + // name the case-insensitive uniqueness and matching use; `status_check_contexts` is a JSON + // array, NULL when the request gave none) and commit statuses (latest per context is the + // highest id). + "CREATE TABLE branch_protections ( + id INTEGER PRIMARY KEY AUTOINCREMENT, + repo_id INTEGER NOT NULL REFERENCES repos(id) ON DELETE CASCADE, + branch TEXT NOT NULL, + lower_branch TEXT NOT NULL, + enable_push INTEGER NOT NULL DEFAULT 0, + enable_status_check INTEGER NOT NULL DEFAULT 0, + status_check_contexts TEXT, + created_unix INTEGER NOT NULL, + updated_unix INTEGER NOT NULL, + UNIQUE (repo_id, lower_branch) + ); + CREATE TABLE commit_statuses ( + id INTEGER PRIMARY KEY AUTOINCREMENT, + repo_id INTEGER NOT NULL REFERENCES repos(id) ON DELETE CASCADE, + sha TEXT NOT NULL, + context TEXT NOT NULL, + state TEXT NOT NULL + CHECK (state IN ('pending', 'success', 'error', 'failure', 'warning', 'skipped')), + description TEXT NOT NULL DEFAULT '', + target_url TEXT NOT NULL DEFAULT '', + creator_id INTEGER NOT NULL REFERENCES users(id), + created_unix INTEGER NOT NULL, + updated_unix INTEGER NOT NULL + ); + CREATE INDEX commit_statuses_by_sha ON commit_statuses (repo_id, sha, context, id);", ]; /// Every table the current schema creates; `/healthz` probes each, so a missing or unreadable one /// (whatever the recorded version says) reads as not ready. -pub const TABLES: &[&str] = &["users", "org_members", "tokens", "repos", "collaborators"]; +pub const TABLES: &[&str] = &[ + "users", + "org_members", + "tokens", + "repos", + "collaborators", + "branch_protections", + "commit_statuses", +]; /// Database failures a caller reports to the operator. #[derive(Debug, thiserror::Error)] diff --git a/crates/forge/src/gitproc.rs b/crates/forge/src/gitproc.rs index 91bcb9aa..9d807c45 100644 --- a/crates/forge/src/gitproc.rs +++ b/crates/forge/src/gitproc.rs @@ -102,6 +102,9 @@ pub struct Spec<'a> { pub max_output: Option, /// Longer deadline than the runner's default. pub timeout: Option, + /// Extra environment for this spawn, added after the pinned one (the `pre-receive` hook passes + /// the object directories of git's quarantine this way). + pub extra_env: &'a [(&'a str, &'a str)], } /// The runner. @@ -217,7 +220,9 @@ impl Git { .acquire_timeout(self.admission) .ok_or(GitError::Busy)?; let mut cmd = Command::new(program); - cmd.env_clear().envs(self.env(spec.identity)); + cmd.env_clear() + .envs(self.env(spec.identity)) + .envs(spec.extra_env.iter().copied()); if let Some(repo) = spec.repo { cmd.arg(format!("--git-dir={}", repo.display())); } diff --git a/crates/forge/src/gitwrite.rs b/crates/forge/src/gitwrite.rs index 4e221a81..569fd9d7 100644 --- a/crates/forge/src/gitwrite.rs +++ b/crates/forge/src/gitwrite.rs @@ -176,6 +176,7 @@ fn run( identity: id, max_output: Some(cap), timeout: Some(left.min(Duration::from_secs(60))), + ..Default::default() })?; if out.ok() { Ok(out.stdout) diff --git a/crates/forge/src/hook.rs b/crates/forge/src/hook.rs index 29c3674f..22b7192d 100644 --- a/crates/forge/src/hook.rs +++ b/crates/forge/src/hook.rs @@ -8,9 +8,11 @@ //! cannot be replaced by repository content (`core.hooksPath` is pinned per spawn and repositories //! are created without a hooks directory), and it trusts nothing from git but the three stdin fields. -use crate::{db, policy, repos}; +use crate::gitproc::{Git, Spec}; +use crate::policy::Rules; +use crate::{db, policy, protection, repos}; use std::io::Read; -use std::path::Path; +use std::path::{Component, Path, PathBuf}; /// Read git's update lines from `r` as raw bytes. Any read error or any line that is not valid UTF-8 /// refuses the whole push: stopping at the first bad line would leave the rest unchecked (fail open). @@ -28,7 +30,15 @@ pub fn read_updates(mut r: impl Read) -> Result, String> { } /// Decide a whole push. `lines` are git's ` ` records; returns every refusal text. -pub fn decide(lines: &[String], default_branch: &str, live: bool) -> Vec { +/// `is_ff(old, new)` answers whether `new` descends from `old`; it is asked only for an update of +/// an existing protected branch that allows pushes. +pub fn decide( + lines: &[String], + default_branch: &str, + live: bool, + rules: &Rules, + is_ff: &dyn Fn(&str, &str) -> Result, +) -> Vec { if !live { return vec!["refusing the push: the repository is not available".to_string()]; } @@ -36,8 +46,18 @@ pub fn decide(lines: &[String], default_branch: &str, live: bool) -> Vec for l in lines { let mut it = l.splitn(3, ' '); match (it.next(), it.next(), it.next()) { - (Some(_old), Some(new), Some(name)) if !name.is_empty() => { - if let Err(e) = policy::check_update(name, new, default_branch) { + (Some(old), Some(new), Some(name)) if !name.is_empty() => { + let verdict = policy::check_update(name, new, default_branch).and_then(|()| { + match name.strip_prefix("refs/heads/") { + Some(branch) => { + policy::check_protected_push(rules, branch, old, new, || { + is_ff(old, new) + }) + } + None => Ok(()), + } + }); + if let Err(e) = verdict { out.push(e); } } @@ -47,15 +67,85 @@ pub fn decide(lines: &[String], default_branch: &str, live: bool) -> Vec out } -/// Look up the repository the hook runs for: `(default_branch, live)`. -pub fn lookup(data_dir: &Path, repo_id: i64) -> Result<(String, bool), String> { +/// What the hook needs to know about the repository. +pub struct RepoInfo { + pub default_branch: String, + pub live: bool, + pub rules: Rules, +} + +/// Look up the repository the hook runs for. +pub fn lookup(data_dir: &Path, repo_id: i64) -> Result { let conn = db::open_readonly(data_dir).map_err(|e| e.to_string())?; match repos::find_by_id(&conn, repo_id).map_err(|e| e.to_string())? { - Some(r) => Ok((r.default_branch, r.state == "live")), + Some(r) => Ok(RepoInfo { + rules: protection::load(&conn, r.id).map_err(|e| e.to_string())?, + live: r.state == "live", + default_branch: r.default_branch, + }), None => Err(format!("repository {repo_id} does not exist")), } } +fn full_id(s: &str) -> bool { + matches!(s.len(), 40 | 64) && s.bytes().all(|b| b.is_ascii_hexdigit()) +} + +/// An object directory git handed the hook is acceptable only when it lies under the +/// repository's own `objects/` (receive-pack's quarantine does). +fn under(objects: &Path, p: &Path) -> bool { + p.is_absolute() + && !p.components().any(|c| matches!(c, Component::ParentDir)) + && p.starts_with(objects) +} + +/// Is `new` a descendant of `old`? Runs `git merge-base --is-ancestor` with the sanitized runner +/// environment plus the quarantine's object directories (the pushed objects are only there until +/// the push is accepted). Anything unexpected refuses the update. +fn is_ff_in( + data_dir: &Path, + repo_id: i64, + git_bin: &Path, + old: &str, + new: &str, +) -> Result { + if !full_id(old) || !full_id(new) { + return Err("malformed object id".into()); + } + let git = Git::new(data_dir, git_bin.to_path_buf(), 1).map_err(|e| e.to_string())?; + let repo: PathBuf = git.data_dir().join("repos").join(format!("{repo_id}.git")); + let objects = repo.join("objects"); + let incoming = std::env::var("GIT_OBJECT_DIRECTORY").ok(); + let alternates = std::env::var("GIT_ALTERNATE_OBJECT_DIRECTORIES").ok(); + let mut extra: Vec<(&str, &str)> = Vec::new(); + if let Some(d) = incoming.as_deref() { + if !under(&objects, Path::new(d)) { + return Err("unexpected object directory".into()); + } + extra.push(("GIT_OBJECT_DIRECTORY", d)); + } + if let Some(a) = alternates.as_deref() { + // A colon-separated list; every entry must be one of the repository's own. + if !a.split(':').all(|d| under(&objects, Path::new(d))) { + return Err("unexpected alternate object directory".into()); + } + extra.push(("GIT_ALTERNATE_OBJECT_DIRECTORIES", a)); + } + let out = git + .run(&Spec { + repo: Some(&repo), + args: &["merge-base", "--is-ancestor", old, new], + extra_env: &extra, + ..Default::default() + }) + .map_err(|e| e.to_string())?; + match out.status.code() { + Some(0) => Ok(true), + Some(1) => Ok(false), + _ => Err(String::from_utf8_lossy(&out.stderr).trim().to_string()), + } +} + /// Point `/hooks/pre-receive` at `exe` (this binary): a symlink, so git runs the hook with no /// shell and the path needs no quoting. Replaced atomically, at every start, because the binary's /// path can change between runs. @@ -88,14 +178,19 @@ pub fn run() -> i32 { return 1; } }; - let (default_branch, live) = match lookup(Path::new(&data), id) { + let info = match lookup(Path::new(&data), id) { Ok(x) => x, Err(e) => { eprintln!("sc-forge pre-receive: refusing the push: {e}"); return 1; } }; - let refusals = decide(&lines, &default_branch, live); + let git_bin = std::env::var_os("FORGE_GIT_BIN").map(PathBuf::from); + let is_ff = |old: &str, new: &str| match &git_bin { + Some(b) => is_ff_in(Path::new(&data), id, b, old, new), + None => Err("FORGE_GIT_BIN is not set".into()), + }; + let refusals = decide(&lines, &info.default_branch, info.live, &info.rules, &is_ff); for r in &refusals { eprintln!("{r}"); } @@ -120,12 +215,55 @@ mod tests { format!("{A} {new} {r}") } + fn ff(v: bool) -> impl Fn(&str, &str) -> Result { + move |_, _| Ok(v) + } + + fn rules(push: bool) -> Rules { + Rules(vec![policy::Rule { + branch: "rel".into(), + enable_push: push, + enable_status_check: false, + contexts: None, + }]) + } + + #[test] + fn protection_rules_decide_pushes_and_the_default_branch_text_comes_first() { + let one = + |rules: &Rules, line: String, f: bool| decide(&[line], "main", true, rules, &ff(f)); + assert_eq!(one(&rules(false), l(A, "refs/heads/rel"), true).len(), 1); + assert_eq!(one(&rules(true), l(A, "refs/heads/rel"), true).len(), 0); + assert_eq!(one(&rules(true), l(A, "refs/heads/rel"), false).len(), 1); + assert_eq!(one(&rules(true), l(Z, "refs/heads/rel"), true).len(), 1); + assert_eq!(one(&rules(true), l(A, "refs/heads/other"), false).len(), 0); + assert_eq!(one(&rules(true), l(A, "refs/tags/rel"), false).len(), 0); + let bad = decide( + &[l(A, "refs/heads/rel")], + "main", + true, + &rules(true), + &|_, _| Err("boom".into()), + ); + assert_eq!(bad.len(), 1, "a failed ancestry check refuses"); + let main = Rules(vec![policy::Rule { + branch: "main".into(), + enable_push: true, + enable_status_check: false, + contexts: None, + }]); + let out = one(&main, l(Z, "refs/heads/main"), true); + assert!(out[0].contains("default branch"), "{out:?}"); + } + #[test] fn a_clean_push_is_accepted() { assert!(decide( &[l(A, "refs/heads/feat"), l(A, "refs/tags/v1")], "main", - true + true, + &Rules::default(), + &ff(true) ) .is_empty()); } @@ -141,6 +279,8 @@ mod tests { ], "main", true, + &Rules::default(), + &ff(true), ); assert_eq!(out.len(), 3, "{out:?}"); assert!( @@ -152,13 +292,34 @@ mod tests { #[test] fn a_repository_that_is_not_live_refuses_everything() { - assert_eq!(decide(&[l(A, "refs/heads/x")], "main", false).len(), 1); + assert_eq!( + decide( + &[l(A, "refs/heads/x")], + "main", + false, + &Rules::default(), + &ff(true) + ) + .len(), + 1 + ); } #[test] fn malformed_lines_fail_closed() { for bad in ["", "onlyone", "a b", "a b "] { - assert_eq!(decide(&[bad.to_string()], "main", true).len(), 1, "{bad:?}"); + assert_eq!( + decide( + &[bad.to_string()], + "main", + true, + &Rules::default(), + &ff(true) + ) + .len(), + 1, + "{bad:?}" + ); } } @@ -200,14 +361,42 @@ mod tests { ) .unwrap(); let r = repos::insert_creating(&conn, owner.id, "r", "trunk", "", "", 1).unwrap(); + let i = lookup(&d, r.id).unwrap(); assert_eq!( - lookup(&d, r.id).unwrap(), - ("trunk".to_string(), false), + (i.default_branch.as_str(), i.live), + ("trunk", false), "creating is not live" ); repos::set_state(&conn, r.id, "creating", "live", 2).unwrap(); - assert_eq!(lookup(&d, r.id).unwrap(), ("trunk".to_string(), true)); + let i = lookup(&d, r.id).unwrap(); + assert_eq!((i.default_branch.as_str(), i.live), ("trunk", true)); + assert!(i.rules.0.is_empty()); assert!(lookup(&d, 999).is_err()); let _ = std::fs::remove_dir_all(&d); } + + #[test] + fn object_directories_must_lie_under_the_repository_and_ids_must_be_full_hex() { + let objects = Path::new("/data/repos/7.git/objects"); + for ok in [ + "/data/repos/7.git/objects", + "/data/repos/7.git/objects/incoming-AbC123", + ] { + assert!(under(objects, Path::new(ok)), "{ok}"); + } + for bad in [ + "/data/repos/7.git/objects/../../8.git/objects", + "/data/repos/7.git/objects2", + "/data/repos/8.git/objects", + "/data/repos/7.git", + "objects/incoming", + "", + ] { + assert!(!under(objects, Path::new(bad)), "{bad:?}"); + } + assert!(full_id(A) && full_id(&"a".repeat(64))); + for bad in ["", "abc", &"g".repeat(40), &"a".repeat(41), "-q"] { + assert!(!full_id(bad), "{bad:?}"); + } + } } diff --git a/crates/forge/src/lib.rs b/crates/forge/src/lib.rs index 643967c2..d2b2d137 100644 --- a/crates/forge/src/lib.rs +++ b/crates/forge/src/lib.rs @@ -25,6 +25,7 @@ pub mod lifecycle; pub mod limits; pub mod names; pub mod policy; +pub mod protection; pub mod refname; pub mod registry; pub mod repo_api; @@ -32,6 +33,7 @@ pub mod repos; pub mod routes; pub mod scope; pub mod server; +pub mod statuses; pub mod store; pub mod timefmt; pub mod transport; diff --git a/crates/forge/src/lifecycle.rs b/crates/forge/src/lifecycle.rs index 54c8080a..c36b2263 100644 --- a/crates/forge/src/lifecycle.rs +++ b/crates/forge/src/lifecycle.rs @@ -72,6 +72,12 @@ pub enum LifecycleError { /// The default branch name is not a valid branch name. #[error("invalid branch name")] BadBranch, + /// A branch protection rule forbids the operation. + #[error("{0}")] + Protected(String), + /// A concurrent change kept winning; the caller may retry. + #[error("{0}")] + Conflict(String), /// A git command that must succeed failed. #[error("git failed: {0}")] GitFailed(String), @@ -467,7 +473,8 @@ impl Lifecycle { Ok(()) } - /// Delete a branch (the caller has refused the default branch). `NoSuchBranch` when absent. + /// Delete a branch (the caller has refused the default branch). `NoSuchBranch` when absent; + /// `Protected` when a protection rule covers it. pub fn delete_branch( &self, conn: &Connection, @@ -485,13 +492,40 @@ impl Lifecycle { "refusing to delete the default branch".into(), )); } + // The exclusive lock is held, so no gated writer (rule change, contents write) is running. + crate::policy::check_branch_delete(&crate::protection::load(conn, current.id)?, branch) + .map_err(LifecycleError::Protected)?; let dir = self.repo_dir(current.id); let full = refname::branch_ref(branch); - if !self.branch_exists(&dir, &full)? { - return Err(LifecycleError::NoSuchBranch); + // Compare-and-swap: receive-pack is not excluded by this lock, so a push that moves the + // branch between the read and the delete must not be deleted along with it. + for _ in 0..3 { + let out = self.git.run(&Spec { + repo: Some(&dir), + args: &["rev-parse", "--verify", "-q", "--end-of-options", &full], + ..Default::default() + })?; + let tip = match out.status.code() { + Some(0) => out.text(), + Some(1) => return Err(LifecycleError::NoSuchBranch), + _ => { + return Err(LifecycleError::GitFailed( + String::from_utf8_lossy(&out.stderr).trim().to_string(), + )) + } + }; + let out = self.git.run(&Spec { + repo: Some(&dir), + args: &["update-ref", "-d", &full, &tip], + ..Default::default() + })?; + if out.ok() { + return Ok(()); + } } - self.run(&dir, &["update-ref", "-d", &full], None, None)?; - Ok(()) + Err(LifecycleError::Conflict( + "the branch was changed concurrently, try again".into(), + )) } /// Move `p` out of the way into `orphans/` (never deleting it). diff --git a/crates/forge/src/policy.rs b/crates/forge/src/policy.rs index 5066d0c4..8eee970b 100644 --- a/crates/forge/src/policy.rs +++ b/crates/forge/src/policy.rs @@ -38,12 +38,85 @@ pub fn check_update(name: &str, new: &str, default_branch: &str) -> Result<(), R )) } +/// One branch protection rule: exact branch name (compared like [`same_branch_name`]), whether +/// direct updates are allowed at all (`enable_push`), and the status contexts a merge needs. +#[derive(Clone, Debug, PartialEq, Eq)] +pub struct Rule { + pub branch: String, + pub enable_push: bool, + pub enable_status_check: bool, + /// `None` when the request gave none (recorded: the API then reports `null`). + pub contexts: Option>, +} + +/// The protection rules of one repository. +#[derive(Clone, Debug, Default)] +pub struct Rules(pub Vec); + +impl Rules { + /// The rule that protects `name`, if any. + pub fn for_branch(&self, name: &str) -> Option<&Rule> { + self.0.iter().find(|r| same_branch_name(&r.branch, name)) + } +} + +/// May a client push `old` -> `new` to `refs/heads/` under `rules`? Applied after +/// [`check_update`]. A rule with `enable_push` off refuses every update (the branch can only move +/// through a merge), administrators included; with it on, a fast-forward is fine and a deletion +/// or a non-fast-forward update is refused for everyone. `is_ff` runs only when it matters +/// (an update of an existing branch under a rule that allows pushes) and any failure to decide +/// refuses the update. +pub fn check_protected_push( + rules: &Rules, + branch: &str, + old: &str, + new: &str, + is_ff: impl FnOnce() -> Result, +) -> Result<(), Refusal> { + let Some(rule) = rules.for_branch(branch) else { + return Ok(()); + }; + if !rule.enable_push { + return Err(format!( + "refusing to update protected branch {branch}: direct pushes are not allowed" + )); + } + if is_delete(new) { + return Err(format!("refusing to delete protected branch {branch}")); + } + if is_delete(old) { + return Ok(()); + } + match is_ff() { + Ok(true) => Ok(()), + Ok(false) => Err(format!("refusing to force-push protected branch {branch}")), + Err(e) => Err(format!( + "refusing to update protected branch {branch}: cannot tell whether it is a fast-forward ({e})" + )), + } +} + /// May a contents write put a commit on branch `target`? `creating` is true when the write creates /// the branch (`new_branch`, or the first commit of an unborn default). Called before the objects -/// are built and again immediately before the ref update, on every attempt, so a rule added in F4 -/// is evaluated at the moment the ref moves (the spec's linearization point). -pub fn check_contents_write(_target: &str, _creating: bool) -> Result<(), Refusal> { - Ok(()) +/// are built and again immediately before the ref update, on every attempt, with the rules read +/// under the repository's write gate, so a rule change is ordered against the write (the spec's +/// linearization point). Rules refuse when `enable_push` is off, whether or not the branch +/// exists yet: a rule reserves its name (stricter than Gitea, which checks only an existing +/// branch). +pub fn check_contents_write(rules: &Rules, target: &str, _creating: bool) -> Result<(), Refusal> { + match rules.for_branch(target) { + Some(r) if !r.enable_push => Err(format!("branch {target} is protected")), + _ => Ok(()), + } +} + +/// May the API delete branch `name`? A protected branch is never deleted by the API (the default +/// branch is refused before this is asked). +pub fn check_branch_delete(rules: &Rules, name: &str) -> Result<(), Refusal> { + match rules.for_branch(name) { + Some(_) => Err(format!("branch {name} is protected and cannot be deleted")), + None => Ok(()), + } } #[cfg(test)] @@ -130,4 +203,55 @@ mod tests { && !is_delete("0000000000000000000000000000000000000001") ); } + + fn rules(push: bool) -> Rules { + Rules(vec![Rule { + branch: "Release".into(), + enable_push: push, + enable_status_check: false, + contexts: None, + }]) + } + + #[test] + fn a_rule_with_pushes_off_refuses_every_update_including_creation() { + let r = rules(false); + for (old, new) in [(A, A), (Z, A), (A, Z)] { + assert!( + check_protected_push(&r, "release", old, new, || Ok(true)).is_err(), + "{old} {new}" + ); + } + assert!(check_contents_write(&r, "RELEASE", true).is_err()); + assert!(check_contents_write(&r, "other", true).is_ok()); + } + + #[test] + fn a_rule_with_pushes_on_allows_fast_forwards_only() { + let r = rules(true); + assert!(check_protected_push(&r, "release", A, A, || Ok(true)).is_ok()); + assert!( + check_protected_push(&r, "release", Z, A, || panic!("creation needs no ancestry")) + .is_ok() + ); + let e = check_protected_push(&r, "release", A, A, || Ok(false)).unwrap_err(); + assert!(e.contains("force-push"), "{e}"); + let e = check_protected_push(&r, "release", A, Z, || panic!("deletion needs no ancestry")) + .unwrap_err(); + assert!(e.contains("delete"), "{e}"); + assert!( + check_protected_push(&r, "release", A, A, || Err("boom".into())).is_err(), + "a failure to decide refuses" + ); + assert!(check_contents_write(&r, "release", false).is_ok()); + } + + #[test] + fn unruled_branches_and_branch_delete() { + let r = rules(true); + assert!(check_protected_push(&r, "dev", A, Z, || Ok(false)).is_ok()); + assert!(check_branch_delete(&r, "RELEASE").is_err()); + assert!(check_branch_delete(&r, "dev").is_ok()); + assert!(check_branch_delete(&Rules::default(), "release").is_ok()); + } } diff --git a/crates/forge/src/protection.rs b/crates/forge/src/protection.rs new file mode 100644 index 00000000..f1535ebe --- /dev/null +++ b/crates/forge/src/protection.rs @@ -0,0 +1,539 @@ +//! Branch protection rules: storage and the `/branch_protections` API. +//! +//! A rule names one branch exactly (case-insensitively, like [`policy::same_branch_name`]); glob +//! patterns, which Gitea also accepts, are refused rather than stored as a literal that protects +//! nothing. Only `enable_push`, `enable_status_check` and `status_check_contexts` are enforced; a +//! request that sets any other protection setting to a non-default value is refused, because +//! echoing a setting nothing enforces would read as protection that is not there. Rule changes run +//! under the repository's write gate, so they are ordered against server-made ref writes (see +//! `lifecycle`'s lock order); a push in flight is not coordinated (it reads the rules in the +//! `pre-receive` hook). + +use crate::api::{body, str_field}; +use crate::http::{error, not_found, Resp}; +use crate::lifecycle::WriteRun; +use crate::policy::{Rule, Rules}; +use crate::refname; +use crate::repo_api::acc; +use crate::repos::{Access, Perm}; +use crate::routes::Ctx; +use crate::timefmt::{now, rfc3339}; +use rusqlite::{params, Connection, OptionalExtension}; +use serde_json::{json, Value}; +use std::time::Duration; + +/// How long a rule change waits for the repository's write gate. +const GATE_WAIT: Duration = Duration::from_secs(10); +const MAX_CONTEXTS: usize = 64; +const MAX_CONTEXT_LEN: usize = 255; + +/// A stored rule with its timestamps. +pub struct Stored { + pub rule: Rule, + pub created: i64, + pub updated: i64, +} + +fn row(r: &rusqlite::Row) -> rusqlite::Result { + let contexts: Option = r.get(3)?; + Ok(Stored { + rule: Rule { + branch: r.get(0)?, + enable_push: r.get::<_, i64>(1)? != 0, + enable_status_check: r.get::<_, i64>(2)? != 0, + // The column is only ever written from a validated Vec; an unreadable value + // reads as "no contexts" and the merge gate (which requires some when the check is on) + // then refuses rather than passes. + contexts: contexts.and_then(|c| serde_json::from_str(&c).ok()), + }, + created: r.get(4)?, + updated: r.get(5)?, + }) +} + +const COLS: &str = + "branch, enable_push, enable_status_check, status_check_contexts, created_unix, updated_unix"; + +/// Every rule of the repository, oldest first. +pub fn list(conn: &Connection, repo_id: i64) -> rusqlite::Result> { + let mut st = conn.prepare(&format!( + "SELECT {COLS} FROM branch_protections WHERE repo_id = ?1 ORDER BY id" + ))?; + let rows = st.query_map([repo_id], row)?.collect(); + rows +} + +/// The rules the policy functions take. +pub fn load(conn: &Connection, repo_id: i64) -> rusqlite::Result { + Ok(Rules( + list(conn, repo_id)?.into_iter().map(|s| s.rule).collect(), + )) +} + +fn find(conn: &Connection, repo_id: i64, name: &str) -> rusqlite::Result> { + let lower = name.to_lowercase(); + conn.query_row( + &format!("SELECT {COLS} FROM branch_protections WHERE repo_id = ?1 AND lower_branch = ?2"), + params![repo_id, lower], + row, + ) + .optional() +} + +fn write_contexts(c: &Option>) -> Option { + c.as_ref() + .map(|v| serde_json::to_string(v).expect("strings serialize")) +} + +fn rule_json(s: &Stored) -> Value { + let r = &s.rule; + json!({ + "branch_name": r.branch, + "rule_name": r.branch, + "enable_push": r.enable_push, + "enable_push_whitelist": false, + "push_whitelist_usernames": [], + "push_whitelist_teams": [], + "push_whitelist_deploy_keys": false, + "enable_merge_whitelist": false, + "merge_whitelist_usernames": [], + "merge_whitelist_teams": [], + "enable_status_check": r.enable_status_check, + "status_check_contexts": r.contexts, + "required_approvals": 0, + "enable_approvals_whitelist": false, + "approvals_whitelist_username": [], + "approvals_whitelist_teams": [], + "block_on_rejected_reviews": false, + "block_on_official_review_requests": false, + "block_on_outdated_branch": false, + "dismiss_stale_approvals": false, + "ignore_stale_approvals": false, + "require_signed_commits": false, + "protected_file_patterns": "", + "unprotected_file_patterns": "", + "created_at": rfc3339(s.created), + "updated_at": rfc3339(s.updated), + }) +} + +/// Settings of Gitea's protection object that `sc-forge` does not enforce. Present with a +/// non-default value they are refused (the 1.26.2 additions included). +const UNSUPPORTED: &[&str] = &[ + "enable_push_whitelist", + "push_whitelist_usernames", + "push_whitelist_teams", + "push_whitelist_deploy_keys", + "enable_merge_whitelist", + "merge_whitelist_usernames", + "merge_whitelist_teams", + "required_approvals", + "enable_approvals_whitelist", + "approvals_whitelist_username", + "approvals_whitelist_teams", + "block_on_rejected_reviews", + "block_on_official_review_requests", + "block_on_outdated_branch", + "dismiss_stale_approvals", + "ignore_stale_approvals", + "require_signed_commits", + "protected_file_patterns", + "unprotected_file_patterns", + "priority", + "enable_force_push", + "enable_force_push_allowlist", + "force_push_allowlist_usernames", + "force_push_allowlist_teams", + "force_push_allowlist_deploy_keys", + "block_admin_merge_override", +]; + +fn is_default(v: &Value) -> bool { + match v { + Value::Null => true, + Value::Bool(b) => !b, + Value::Number(n) => n.as_i64() == Some(0), + Value::String(s) => s.is_empty(), + Value::Array(a) => a.is_empty(), + Value::Object(o) => o.is_empty(), + } +} + +fn has_glob(s: &str) -> bool { + s.contains(['*', '?', '[']) +} + +fn contexts_of(v: &Value) -> Result>, String> { + let Value::Array(items) = v else { + return if v.is_null() { + Ok(None) + } else { + Err("[StatusCheckContexts]: must be a list of strings".into()) + }; + }; + if items.len() > MAX_CONTEXTS { + return Err(format!( + "[StatusCheckContexts]: at most {MAX_CONTEXTS} contexts" + )); + } + let mut out = Vec::new(); + for i in items { + let Some(c) = i.as_str() else { + return Err("[StatusCheckContexts]: must be a list of strings".into()); + }; + if c.is_empty() || c.len() > MAX_CONTEXT_LEN || c.chars().any(char::is_control) { + return Err(format!( + "[StatusCheckContexts]: a context is 1 to {MAX_CONTEXT_LEN} bytes without control characters" + )); + } + if has_glob(c) { + return Err("[StatusCheckContexts]: context patterns are not supported".into()); + } + if !out.contains(&c.to_string()) { + out.push(c.to_string()); + } + } + Ok(Some(out)) +} + +/// The enforced settings out of a request body, applied over `base` (a PATCH) or the defaults +/// (a create). Every refusal is a 422 text. +fn settings(b: &Value, mut base: Rule) -> Result { + for k in UNSUPPORTED { + if b.get(k).is_some_and(|v| !is_default(v)) { + return Err(format!("unsupported branch protection setting: {k}")); + } + } + for (k, slot) in [ + ("enable_push", &mut base.enable_push), + ("enable_status_check", &mut base.enable_status_check), + ] { + match b.get(k) { + None | Some(Value::Null) => {} + Some(Value::Bool(v)) => *slot = *v, + Some(_) => return Err(format!("[{k}]: must be a boolean")), + } + } + if let Some(v) = b.get("status_check_contexts") { + base.contexts = contexts_of(v)?; + } + if base.enable_status_check && base.contexts.as_ref().is_none_or(Vec::is_empty) { + return Err("status_check_contexts is required when enable_status_check is set".into()); + } + Ok(base) +} + +fn require_admin(ctx: &Ctx, a: &Access) -> Result<(), Resp> { + if a.perm >= Perm::Admin { + return Ok(()); + } + Err(error( + &ctx.app.base, + 403, + "user should be an owner or a collaborator with admin write of a repository", + )) +} + +fn missing(ctx: &Ctx) -> Resp { + not_found(&ctx.app.base, "The target couldn't be found.", None) +} + +/// Run `f` under the write gate and answer for the gate's outcomes. +fn gated(ctx: &Ctx, a: &Access, f: impl FnOnce() -> Resp) -> Resp { + match ctx + .app + .life + .with_write_row(ctx.conn, a.repo.id, GATE_WAIT, |_| f()) + { + Ok(WriteRun::Ran(r)) => r, + Ok(WriteRun::Busy) => error( + &ctx.app.base, + 503, + "this repository is being written to, try again", + ), + Ok(WriteRun::Gone) => missing(ctx), + Err(e) => ctx.app.internal(&e), + } +} + +/// `GET /repos/{o}/{r}/branch_protections`. +pub fn list_rules(ctx: &mut Ctx) -> Resp { + let a = match acc(ctx) { + Ok(a) => a.clone(), + Err(r) => return r, + }; + match list(ctx.conn, a.repo.id) { + Ok(rows) => Resp::json(200, &Value::Array(rows.iter().map(rule_json).collect())), + Err(e) => ctx.app.internal(&e), + } +} + +/// `GET /repos/{o}/{r}/branch_protections/{b..}`. +pub fn get_rule(ctx: &mut Ctx) -> Resp { + let a = match acc(ctx) { + Ok(a) => a.clone(), + Err(r) => return r, + }; + match find(ctx.conn, a.repo.id, ctx.param("b")) { + Ok(Some(s)) => Resp::json(200, &rule_json(&s)), + Ok(None) => missing(ctx), + Err(e) => ctx.app.internal(&e), + } +} + +/// `POST /repos/{o}/{r}/branch_protections`. +pub fn create_rule(ctx: &mut Ctx) -> Resp { + let a = match acc(ctx) { + Ok(a) => a.clone(), + Err(r) => return r, + }; + if let Err(r) = require_admin(ctx, &a) { + return r; + } + let b = match body(ctx) { + Ok(b) => b, + Err(r) => return r, + }; + let (name, rule_name) = (str_field(&b, "branch_name"), str_field(&b, "rule_name")); + let name = match (name, rule_name) { + (Some(x), Some(y)) if x != y => { + return error( + &ctx.app.base, + 422, + "branch_name and rule_name must be the same", + ) + } + (Some(x), _) | (None, Some(x)) if !x.is_empty() => x, + _ => return error(&ctx.app.base, 422, "[BranchName]: Required"), + }; + if has_glob(&name) { + return error( + &ctx.app.base, + 422, + "branch name patterns are not supported: name one branch", + ); + } + if !refname::valid_branch(&name) { + return error(&ctx.app.base, 422, "[BranchName]: not a valid branch name"); + } + let rule = match settings( + &b, + Rule { + branch: name, + enable_push: false, + enable_status_check: false, + contexts: None, + }, + ) { + Ok(r) => r, + Err(m) => return error(&ctx.app.base, 422, &m), + }; + gated(ctx, &a, || { + match find(ctx.conn, a.repo.id, &rule.branch) { + Ok(Some(_)) => return error(&ctx.app.base, 403, "Branch protection already exist"), + Ok(None) => {} + Err(e) => return ctx.app.internal(&e), + } + let t = now(); + let inserted = ctx.conn.execute( + "INSERT INTO branch_protections (repo_id, branch, lower_branch, enable_push, \ + enable_status_check, status_check_contexts, created_unix, updated_unix) \ + VALUES (?1, ?2, ?3, ?4, ?5, ?6, ?7, ?7)", + params![ + a.repo.id, + rule.branch, + rule.branch.to_lowercase(), + rule.enable_push, + rule.enable_status_check, + write_contexts(&rule.contexts), + t + ], + ); + match inserted { + Ok(_) => Resp::json( + 201, + &rule_json(&Stored { + rule, + created: t, + updated: t, + }), + ), + Err(e) => ctx.app.internal(&e), + } + }) +} + +/// `PATCH /repos/{o}/{r}/branch_protections/{b..}`: the settings present in the body replace the +/// stored ones; the rule's name does not change. +pub fn patch_rule(ctx: &mut Ctx) -> Resp { + let a = match acc(ctx) { + Ok(a) => a.clone(), + Err(r) => return r, + }; + if let Err(r) = require_admin(ctx, &a) { + return r; + } + let b = match body(ctx) { + Ok(b) => b, + Err(r) => return r, + }; + let name = ctx.param("b").to_string(); + // The rule's name does not change, but a name that is sent is validated like a create's. + let (bn, rn) = (str_field(&b, "branch_name"), str_field(&b, "rule_name")); + if let (Some(x), Some(y)) = (&bn, &rn) { + if x != y { + return error( + &ctx.app.base, + 422, + "branch_name and rule_name must be the same", + ); + } + } + for n in [&bn, &rn].into_iter().flatten() { + if has_glob(n) { + return error( + &ctx.app.base, + 422, + "branch name patterns are not supported: name one branch", + ); + } + if n.to_lowercase() != name.to_lowercase() { + return error( + &ctx.app.base, + 422, + "a rule cannot be renamed: delete it and create another", + ); + } + } + gated(ctx, &a, || { + let cur = match find(ctx.conn, a.repo.id, &name) { + Ok(Some(s)) => s, + Ok(None) => return missing(ctx), + Err(e) => return ctx.app.internal(&e), + }; + let rule = match settings(&b, cur.rule.clone()) { + Ok(r) => r, + Err(m) => return error(&ctx.app.base, 422, &m), + }; + let t = now(); + let done = ctx.conn.execute( + "UPDATE branch_protections SET enable_push = ?1, enable_status_check = ?2, \ + status_check_contexts = ?3, updated_unix = ?4 WHERE repo_id = ?5 AND lower_branch = ?6", + params![ + rule.enable_push, + rule.enable_status_check, + write_contexts(&rule.contexts), + t, + a.repo.id, + rule.branch.to_lowercase() + ], + ); + match done { + Ok(_) => Resp::json( + 200, + &rule_json(&Stored { + rule, + created: cur.created, + updated: t, + }), + ), + Err(e) => ctx.app.internal(&e), + } + }) +} + +/// `DELETE /repos/{o}/{r}/branch_protections/{b..}`. +pub fn delete_rule(ctx: &mut Ctx) -> Resp { + let a = match acc(ctx) { + Ok(a) => a.clone(), + Err(r) => return r, + }; + if let Err(r) = require_admin(ctx, &a) { + return r; + } + let lower = ctx.param("b").to_lowercase(); + gated(ctx, &a, || { + match ctx.conn.execute( + "DELETE FROM branch_protections WHERE repo_id = ?1 AND lower_branch = ?2", + params![a.repo.id, lower], + ) { + Ok(0) => missing(ctx), + Ok(_) => Resp::no_content(), + Err(e) => ctx.app.internal(&e), + } + }) +} + +#[cfg(test)] +mod tests { + use super::*; + + fn base() -> Rule { + Rule { + branch: "main".into(), + enable_push: false, + enable_status_check: false, + contexts: None, + } + } + + #[test] + fn defaults_and_flags_apply_over_the_base() { + let r = settings(&json!({"enable_push": true}), base()).unwrap(); + assert!(r.enable_push && !r.enable_status_check && r.contexts.is_none()); + let r = settings( + &json!({"enable_status_check": true, "status_check_contexts": ["ci/test", "ci/test"]}), + base(), + ) + .unwrap(); + assert_eq!( + r.contexts, + Some(vec!["ci/test".to_string()]), + "deduplicated" + ); + let r = settings(&json!({"enable_status_check": false}), r).unwrap(); + assert_eq!(r.contexts.as_deref(), Some(&["ci/test".to_string()][..])); + } + + #[test] + fn unsupported_settings_are_refused_only_when_set() { + assert!(settings(&json!({"required_approvals": 0, "enable_push_whitelist": false, "protected_file_patterns": ""}), base()).is_ok()); + for (k, v) in [ + ("required_approvals", json!(2)), + ("enable_merge_whitelist", json!(true)), + ("require_signed_commits", json!(true)), + ("block_on_outdated_branch", json!(true)), + ("push_whitelist_usernames", json!(["a"])), + ("protected_file_patterns", json!("*.lock")), + ("enable_force_push", json!(true)), + ] { + let e = settings(&json!({ k: v }), base()).unwrap_err(); + assert!(e.contains(k), "{k}: {e}"); + } + } + + #[test] + fn status_check_needs_contexts_and_contexts_are_literal() { + assert!(settings(&json!({"enable_status_check": true}), base()).is_err()); + assert!(settings( + &json!({"enable_status_check": true, "status_check_contexts": []}), + base() + ) + .is_err()); + for bad in [ + json!(["ci/*"]), + json!(["a?"]), + json!([""]), + json!([1]), + json!("x"), + ] { + assert!( + settings(&json!({"status_check_contexts": bad}), base()).is_err(), + "{bad}" + ); + } + let many: Vec = (0..=MAX_CONTEXTS).map(|i| format!("c{i}")).collect(); + assert!(settings(&json!({ "status_check_contexts": many }), base()).is_err()); + assert!(settings(&json!({"enable_push": "yes"}), base()).is_err()); + } +} diff --git a/crates/forge/src/repo_api.rs b/crates/forge/src/repo_api.rs index f8662cfb..54bafaf0 100644 --- a/crates/forge/src/repo_api.rs +++ b/crates/forge/src/repo_api.rs @@ -30,6 +30,8 @@ fn life_err(app: &App, e: LifecycleError) -> Resp { "The repository with the same name already exists.", ), LifecycleError::BadBranch => error(&app.base, 422, "[DefaultBranch]: invalid branch name"), + LifecycleError::Protected(m) => error(&app.base, 403, &m), + LifecycleError::Conflict(m) => error(&app.base, 409, &m), LifecycleError::Gone => not_found(&app.base, "The target couldn't be found.", None), LifecycleError::Git(crate::gitproc::GitError::Busy) => { error(&app.base, 503, "too many git operations in flight") @@ -81,7 +83,12 @@ fn compute_empty(app: &App, conn: &rusqlite::Connection, r: &Repo) -> Result Result { +pub(crate) fn repo_json( + app: &App, + conn: &rusqlite::Connection, + r: &Repo, + perm: Perm, +) -> Result { let empty = compute_empty(app, conn, r)?; repo_json_known(app, conn, r, perm, empty) } @@ -618,7 +625,12 @@ pub fn collaborator_permission(ctx: &mut Ctx) -> Resp { ) } -fn branch_json(ctx: &Ctx, a: &Access, br: &gitread::Branch) -> Result { +fn branch_json( + ctx: &Ctx, + a: &Access, + br: &gitread::Branch, + rules: &crate::policy::Rules, +) -> Result { let dir = ctx.app.life.repo_dir(a.repo.id); let c = gitread::commit(&ctx.app.life.git, &dir, &br.oid) .map_err(|e| git_read_err(ctx.app, ctx.conn, a.repo.id, e))? @@ -629,6 +641,7 @@ fn branch_json(ctx: &Ctx, a: &Access, br: &gitread::Branch) -> Result| json!({"name": p.name, "email": p.email, "username": login.unwrap_or_default()}); let full = format!("{}/{}", a.repo.owner, a.repo.name); + let rule = rules.for_branch(&br.name); Ok(json!({ "name": br.name, "commit": { @@ -643,13 +656,13 @@ fn branch_json(ctx: &Ctx, a: &Access, br: &gitread::Branch) -> Result= Perm::Write, + "enable_status_check": rule.is_some_and(|r| r.enable_status_check), + "status_check_contexts": rule.map_or_else(|| json!([]), |r| json!(r.contexts)), + "user_can_push": a.perm >= Perm::Write && rule.is_none_or(|r| r.enable_push), "user_can_merge": a.perm >= Perm::Write, - "effective_branch_protection_name": "", + "effective_branch_protection_name": rule.map_or("", |r| r.branch.as_str()), })) } @@ -692,7 +705,14 @@ fn list_branches_inner(ctx: &Ctx, a: &Access) -> Resp { let total = all.len() as i64; let start = (((page - 1) * limit) as usize).min(all.len()); let rows = &all[start..(start + limit as usize).min(all.len())]; - let items: Result, Resp> = rows.iter().map(|b| branch_json(ctx, a, b)).collect(); + let rules = match crate::protection::load(ctx.conn, a.repo.id) { + Ok(r) => r, + Err(e) => return ctx.app.internal(&e), + }; + let items: Result, Resp> = rows + .iter() + .map(|b| branch_json(ctx, a, b, &rules)) + .collect(); match items { Ok(items) => paged( ctx, @@ -718,7 +738,10 @@ pub fn get_branch(ctx: &mut Ctx) -> Resp { fn get_branch_inner(ctx: &Ctx, a: &Access) -> Resp { let name = ctx.param("b").to_string(); match gitread::branch_oid(&ctx.app.life.git, &ctx.app.life.repo_dir(a.repo.id), &name) { - Ok(Some(oid)) => match branch_json(ctx, a, &gitread::Branch { name, oid }) { + Ok(Some(oid)) => match crate::protection::load(ctx.conn, a.repo.id) + .map_err(|e| ctx.app.internal(&e)) + .and_then(|rules| branch_json(ctx, a, &gitread::Branch { name, oid }, &rules)) + { Ok(v) => Resp::json(200, &v), Err(r) => r, }, diff --git a/crates/forge/src/routes.rs b/crates/forge/src/routes.rs index c4c95de0..c8373856 100644 --- a/crates/forge/src/routes.rs +++ b/crates/forge/src/routes.rs @@ -9,7 +9,7 @@ use crate::lifecycle::Lifecycle; use crate::limits::{Backoff, Gate, Proxies}; use crate::repos::{self, Access, Perm}; use crate::scope::{self, Cat, Level, Need}; -use crate::{api, content_api, content_write, db, repo_api, store}; +use crate::{api, content_api, content_write, db, protection, repo_api, statuses, store}; use rusqlite::Connection; use std::path::PathBuf; @@ -452,6 +452,78 @@ pub static ROUTES: &[Route] = &[ Repo(Perm::Read), content_api::get_archive, ), + r( + "GET", + "/repos/{o}/{r}/branch_protections", + Token, + n(Cat::Repository, Level::Read), + Repo(Perm::Read), + protection::list_rules, + ), + r( + "POST", + "/repos/{o}/{r}/branch_protections", + Token, + n(Cat::Repository, Level::Write), + Repo(Perm::Read), + protection::create_rule, + ), + r( + "GET", + "/repos/{o}/{r}/branch_protections/{b..}", + Token, + n(Cat::Repository, Level::Read), + Repo(Perm::Read), + protection::get_rule, + ), + r( + "PATCH", + "/repos/{o}/{r}/branch_protections/{b..}", + Token, + n(Cat::Repository, Level::Write), + Repo(Perm::Read), + protection::patch_rule, + ), + r( + "DELETE", + "/repos/{o}/{r}/branch_protections/{b..}", + Token, + n(Cat::Repository, Level::Write), + Repo(Perm::Read), + protection::delete_rule, + ), + r( + "POST", + "/repos/{o}/{r}/statuses/{sha..}", + Token, + n(Cat::Repository, Level::Write), + Repo(Perm::Read), + statuses::create_status, + ), + r( + "GET", + "/repos/{o}/{r}/statuses/{sha..}", + Token, + n(Cat::Repository, Level::Read), + Repo(Perm::Read), + statuses::list_statuses, + ), + r( + "GET", + "/repos/{o}/{r}/commits/{sha}/statuses", + Token, + n(Cat::Repository, Level::Read), + Repo(Perm::Read), + statuses::list_statuses, + ), + r( + "GET", + "/repos/{o}/{r}/commits/{sha}/status", + Token, + n(Cat::Repository, Level::Read), + Repo(Perm::Read), + statuses::combined_status, + ), ]; /// Captured path parameters, in pattern order. diff --git a/crates/forge/src/statuses.rs b/crates/forge/src/statuses.rs new file mode 100644 index 00000000..4fe70efa --- /dev/null +++ b/crates/forge/src/statuses.rs @@ -0,0 +1,540 @@ +//! Commit statuses: `POST /statuses/{sha}`, the history lists and the combined status. +//! +//! Any caller with write access may post any context (Gitea's trust boundary: the writer is +//! trusted, a context is not an attestation). "Latest" per context is the highest id. Storage is +//! bounded per commit: at most [`MAX_CONTEXTS`] distinct contexts and [`MAX_HISTORY`] rows per +//! context (the oldest rows go), a departure from Gitea, which keeps everything. The merge gate +//! reads [`latest`] directly, never the paged combined view. + +use crate::api::{body, link_header, page_params, paged, user_json}; +use crate::content_api::{api_base, dir}; +use crate::gitobj::{self, Peel}; +use crate::http::{error, not_found, Resp}; +use crate::repo_api::{acc, read_locked, repo_json}; +use crate::repos::{Access, Perm}; +use crate::routes::Ctx; +use crate::store; +use crate::timefmt::{now, rfc3339}; +use rusqlite::{params, Connection, Transaction, TransactionBehavior}; +use serde_json::{json, Value}; +use std::collections::HashMap; + +pub const STATES: &[&str] = &[ + "pending", "success", "error", "failure", "warning", "skipped", +]; +/// Distinct contexts per commit. +pub const MAX_CONTEXTS: i64 = 100; +/// Rows kept per (commit, context). +pub const MAX_HISTORY: i64 = 100; +const MAX_CONTEXT: usize = 1024; +const MAX_DESCRIPTION: usize = 1024; +const MAX_URL: usize = 2048; + +/// A stored status. +#[derive(Clone, Debug)] +pub struct Status { + pub id: i64, + pub sha: String, + pub context: String, + pub state: String, + pub description: String, + pub target_url: String, + pub creator_id: i64, + pub created: i64, + pub updated: i64, +} + +const COLS: &str = + "id, sha, context, state, description, target_url, creator_id, created_unix, updated_unix"; + +fn row(r: &rusqlite::Row) -> rusqlite::Result { + Ok(Status { + id: r.get(0)?, + sha: r.get(1)?, + context: r.get(2)?, + state: r.get(3)?, + description: r.get(4)?, + target_url: r.get(5)?, + creator_id: r.get(6)?, + created: r.get(7)?, + updated: r.get(8)?, + }) +} + +/// How an insert ended. +pub enum Inserted { + Done(Status), + /// The commit already has [`MAX_CONTEXTS`] distinct contexts and this one is new. + TooManyContexts, +} + +/// Insert a status and trim the context's history, in one immediate transaction (so concurrent +/// posters cannot both pass the context cap). +#[allow(clippy::too_many_arguments)] +pub fn insert( + conn: &Connection, + repo_id: i64, + sha: &str, + context: &str, + state: &str, + description: &str, + target_url: &str, + creator_id: i64, + now: i64, +) -> rusqlite::Result { + let tx = Transaction::new_unchecked(conn, TransactionBehavior::Immediate)?; + let known: i64 = tx.query_row( + "SELECT COUNT(*) FROM commit_statuses WHERE repo_id = ?1 AND sha = ?2 AND context = ?3", + params![repo_id, sha, context], + |r| r.get(0), + )?; + if known == 0 { + let contexts: i64 = tx.query_row( + "SELECT COUNT(DISTINCT context) FROM commit_statuses WHERE repo_id = ?1 AND sha = ?2", + params![repo_id, sha], + |r| r.get(0), + )?; + if contexts >= MAX_CONTEXTS { + return Ok(Inserted::TooManyContexts); + } + } + tx.execute( + "INSERT INTO commit_statuses (repo_id, sha, context, state, description, target_url, \ + creator_id, created_unix, updated_unix) VALUES (?1, ?2, ?3, ?4, ?5, ?6, ?7, ?8, ?8)", + params![ + repo_id, + sha, + context, + state, + description, + target_url, + creator_id, + now + ], + )?; + let id = tx.last_insert_rowid(); + tx.execute( + "DELETE FROM commit_statuses WHERE repo_id = ?1 AND sha = ?2 AND context = ?3 AND id NOT IN \ + (SELECT id FROM commit_statuses WHERE repo_id = ?1 AND sha = ?2 AND context = ?3 \ + ORDER BY id DESC LIMIT ?4)", + params![repo_id, sha, context, MAX_HISTORY], + )?; + let s = tx.query_row( + &format!("SELECT {COLS} FROM commit_statuses WHERE id = ?1"), + [id], + row, + )?; + tx.commit()?; + Ok(Inserted::Done(s)) +} + +/// Every status of the commit, newest first. +pub fn history(conn: &Connection, repo_id: i64, sha: &str) -> rusqlite::Result> { + let mut st = conn.prepare(&format!( + "SELECT {COLS} FROM commit_statuses WHERE repo_id = ?1 AND sha = ?2 ORDER BY id DESC" + ))?; + let rows = st.query_map(params![repo_id, sha], row)?.collect(); + rows +} + +/// The latest status of each context of the commit, oldest first (the order Gitea lists them in). +pub fn latest(conn: &Connection, repo_id: i64, sha: &str) -> rusqlite::Result> { + let mut st = conn.prepare(&format!( + "SELECT {COLS} FROM commit_statuses WHERE id IN \ + (SELECT MAX(id) FROM commit_statuses WHERE repo_id = ?1 AND sha = ?2 GROUP BY context) \ + ORDER BY id" + ))?; + let rows = st.query_map(params![repo_id, sha], row)?.collect(); + rows +} + +/// The combined state of the latest statuses, as recorded (H): any `error` or `failure` reads +/// `failure`, else any `pending` reads `pending`, else `warning`, else `success`; only `skipped` +/// (or nothing) reads `skipped` / `pending`. The unrecorded combinations (`warning` alone, all +/// `skipped`) follow that severity order. +pub fn combined_state(latest: &[Status]) -> &'static str { + let any = |s: &str| latest.iter().any(|x| x.state == s); + if latest.is_empty() { + "pending" + } else if any("error") || any("failure") { + "failure" + } else if any("pending") { + "pending" + } else if any("warning") { + "warning" + } else if any("success") { + "success" + } else { + "skipped" + } +} + +/// Does a required context have a `success` or `skipped` latest status? The merge gate's test. +pub fn satisfied(latest: &[Status], context: &str) -> bool { + latest + .iter() + .any(|s| s.context == context && matches!(s.state.as_str(), "success" | "skipped")) +} + +fn status_json( + ctx: &Ctx, + a: &Access, + s: &Status, + creators: &mut HashMap, +) -> Result { + if let std::collections::hash_map::Entry::Vacant(slot) = creators.entry(s.creator_id) { + let u = store::find_by_id(ctx.conn, s.creator_id) + .map_err(|e| ctx.app.internal(&e))? + .ok_or_else(|| ctx.app.internal(&"status without creator"))?; + let mut j = user_json(&ctx.app.base, &u); + // Recorded: the nested creator is the public view of the account. The stored address is + // never shown (a placeholder derived from the login stands in), nor the site-admin flag. + j["active"] = json!(false); + j["is_admin"] = json!(false); + j["login_name"] = json!(""); + j["email"] = json!(format!("{}@noreply.localhost", u.login)); + slot.insert(j); + } + Ok(json!({ + "id": s.id, + "status": s.state, + "target_url": s.target_url, + "description": s.description, + "url": format!("{}/statuses/{}", api_base(ctx, a), s.sha), + "context": s.context, + "creator": creators[&s.creator_id], + "created_at": rfc3339(s.created), + "updated_at": rfc3339(s.updated), + })) +} + +fn no_commit(ctx: &Ctx) -> Resp { + not_found(&ctx.app.base, "The target couldn't be found.", None) +} + +/// Resolve `rev` to a commit id in the repository, or answer for it. +fn commit_of(ctx: &Ctx, a: &Access, rev: &str) -> Result { + match gitobj::resolve( + &ctx.app.life.git, + &dir(ctx, a), + &a.repo.default_branch, + rev, + Peel::Commit, + ) { + Ok(Some(r)) => Ok(r.oid), + Ok(None) => Err(no_commit(ctx)), + Err(e) => Err(crate::content_api::read_err(ctx, a, e)), + } +} + +fn valid_url(u: &str) -> bool { + u.is_empty() + || ((u.starts_with("http://") || u.starts_with("https://")) + && !u.chars().any(|c| c.is_control() || c.is_whitespace())) +} + +/// `POST /repos/{o}/{r}/statuses/{sha..}`. +pub fn create_status(ctx: &mut Ctx) -> Resp { + let a = match acc(ctx) { + Ok(a) => a.clone(), + Err(r) => return r, + }; + if a.perm < Perm::Write { + return error( + &ctx.app.base, + 403, + "user should have a permission to write to a repo", + ); + } + let b = match body(ctx) { + Ok(b) => b, + Err(r) => return r, + }; + let text = |k: &str| match b.get(k) { + None | Some(Value::Null) => Ok(String::new()), + Some(Value::String(s)) => Ok(s.clone()), + Some(_) => Err(format!("[{k}]: must be a string")), + }; + let (state, context, description, url) = match ( + text("state"), + text("context"), + text("description"), + text("target_url"), + ) { + (Ok(s), Ok(c), Ok(d), Ok(u)) => (s, c, d, u), + (Err(m), ..) | (_, Err(m), ..) | (_, _, Err(m), _) | (_, _, _, Err(m)) => { + return error(&ctx.app.base, 422, &m) + } + }; + if !STATES.contains(&state.as_str()) { + return error( + &ctx.app.base, + 422, + &format!("[State]: must be one of {}", STATES.join(", ")), + ); + } + if context.len() > MAX_CONTEXT + || description.chars().count() > MAX_DESCRIPTION + || url.len() > MAX_URL + || !valid_url(&url) + || context.chars().any(char::is_control) + { + return error( + &ctx.app.base, + 422, + "[Context/Description/TargetURL]: too long, or not a plain http(s) URL", + ); + } + let rev = ctx.param("sha").to_string(); + read_locked(ctx, &a, |ctx| { + let sha = match commit_of(ctx, &a, &rev) { + Ok(s) => s, + Err(r) => return r, + }; + match insert( + ctx.conn, + a.repo.id, + &sha, + &context, + &state, + &description, + &url, + ctx.who.user.id, + now(), + ) { + Ok(Inserted::Done(s)) => match status_json(ctx, &a, &s, &mut HashMap::new()) { + Ok(v) => Resp::json(201, &v), + Err(r) => r, + }, + Ok(Inserted::TooManyContexts) => error( + &ctx.app.base, + 422, + &format!("a commit has at most {MAX_CONTEXTS} status contexts"), + ), + Err(e) => ctx.app.internal(&e), + } + }) +} + +/// `GET /repos/{o}/{r}/statuses/{ref..}` and `GET /repos/{o}/{r}/commits/{ref}/statuses`: the +/// whole history, newest first, paged. +pub fn list_statuses(ctx: &mut Ctx) -> Resp { + let a = match acc(ctx) { + Ok(a) => a.clone(), + Err(r) => return r, + }; + let rev = ctx.param("sha").to_string(); + read_locked(ctx, &a, |ctx| { + let sha = match commit_of(ctx, &a, &rev) { + Ok(s) => s, + Err(r) => return r, + }; + let all = match history(ctx.conn, a.repo.id, &sha) { + Ok(v) => v, + Err(e) => return ctx.app.internal(&e), + }; + let (page, limit) = page_params(ctx); + let mut creators = HashMap::new(); + let items: Result, Resp> = all + .iter() + .skip(((page - 1) * limit) as usize) + .take(limit as usize) + .map(|s| status_json(ctx, &a, s, &mut creators)) + .collect(); + match items { + Ok(items) => paged( + ctx, + &format!( + "/repos/{}/{}/commits/{}/statuses", + a.repo.owner, + a.repo.name, + crate::api::pct_encode(&rev) + ), + all.len() as i64, + page, + limit, + items, + ), + Err(r) => r, + } + }) +} + +/// `GET /repos/{o}/{r}/commits/{ref}/status`: the latest status per context, with the combined +/// state over all of them; the listed statuses are paged. Only the resolution of the revision +/// runs under the shared lifecycle lock: `repo_json` takes that lock itself, and a second shared +/// acquisition behind a queued exclusive one would deadlock (see `lifecycle`'s lock order). +pub fn combined_status(ctx: &mut Ctx) -> Resp { + let a = match acc(ctx) { + Ok(a) => a.clone(), + Err(r) => return r, + }; + let rev = ctx.param("sha").to_string(); + let mut resolved = None; + let early = read_locked(ctx, &a, |ctx| match commit_of(ctx, &a, &rev) { + Ok(s) => { + resolved = Some(s); + Resp::no_content() + } + Err(r) => r, + }); + let Some(sha) = resolved else { + return early; + }; + let ctx: &Ctx = ctx; + let all = match latest(ctx.conn, a.repo.id, &sha) { + Ok(v) => v, + Err(e) => return ctx.app.internal(&e), + }; + let (page, limit) = page_params(ctx); + let mut creators = HashMap::new(); + let shown: Result, Resp> = all + .iter() + .skip(((page - 1) * limit) as usize) + .take(limit as usize) + .map(|s| status_json(ctx, &a, s, &mut creators)) + .collect(); + let shown = match shown { + Ok(v) => v, + Err(r) => return r, + }; + // Recorded: a commit with no statuses carries no repository object. + let repo = if all.is_empty() { + Value::Null + } else { + match repo_json(ctx.app, ctx.conn, &a.repo, a.perm) { + Ok(v) => v, + Err(r) => return r, + } + }; + let total = all.len() as i64; + let body = json!({ + "state": combined_state(&all), + "sha": sha, + "total_count": shown.len(), + "statuses": if shown.is_empty() { Value::Null } else { Value::Array(shown) }, + "repository": repo, + "commit_url": "", + "url": "", + }); + let mut r = Resp::json(200, &body); + if total > 0 { + r = r.with("x-total-count", total.to_string()); + } + let path = format!( + "/repos/{}/{}/commits/{}/status", + a.repo.owner, + a.repo.name, + crate::api::pct_encode(&rev) + ); + if let Some(l) = link_header(ctx, &path, total, page, limit, &[]) { + r = r.with("link", l); + } + r +} + +#[cfg(test)] +mod tests { + use super::*; + + fn s(context: &str, state: &str) -> Status { + Status { + id: 0, + sha: String::new(), + context: context.into(), + state: state.into(), + description: String::new(), + target_url: String::new(), + creator_id: 1, + created: 0, + updated: 0, + } + } + + #[test] + fn the_combined_state_follows_the_recorded_severity_order() { + let c = |v: &[(&str, &str)]| { + combined_state(&v.iter().map(|(c, st)| s(c, st)).collect::>()) + }; + assert_eq!(c(&[]), "pending"); + assert_eq!(c(&[("a", "success")]), "success"); + assert_eq!(c(&[("a", "success"), ("b", "failure")]), "failure"); + assert_eq!(c(&[("a", "success"), ("b", "pending")]), "pending"); + assert_eq!(c(&[("a", "pending"), ("b", "error")]), "failure"); + assert_eq!(c(&[("a", "success"), ("b", "warning")]), "warning"); + assert_eq!(c(&[("a", "skipped")]), "skipped"); + assert_eq!(c(&[("a", "skipped"), ("b", "success")]), "success"); + } + + #[test] + fn only_success_and_skipped_satisfy_a_required_context() { + let l = [ + s("ci/test", "skipped"), + s("ci/lint", "failure"), + s("ci/x", "warning"), + ]; + assert!(satisfied(&l, "ci/test")); + assert!(!satisfied(&l, "ci/lint") && !satisfied(&l, "ci/x") && !satisfied(&l, "ci/none")); + } + + #[test] + fn urls_must_be_plain_http() { + assert!(valid_url("") && valid_url("https://ci.example/run/1")); + for bad in [ + "javascript:alert(1)", + "ftp://x", + "http://a b", + "//x", + "http://a\n", + ] { + assert!(!valid_url(bad), "{bad}"); + } + } + + #[test] + fn history_and_contexts_are_bounded() { + let d = std::env::temp_dir().join(format!("scl-forge-status-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&d); + let conn = crate::db::open_for_admin(&d).unwrap(); + let owner = store::create_account( + &conn, + &store::NewAccount { + login: "o".into(), + visibility: "public".into(), + ..Default::default() + }, + 1, + ) + .unwrap(); + let repo = crate::repos::insert_creating(&conn, owner.id, "r", "main", "", "", 1).unwrap(); + let ins = |ctxt: &str, st: &str| { + insert(&conn, repo.id, "abc", ctxt, st, "", "", owner.id, 5).unwrap() + }; + for i in 0..MAX_HISTORY + 20 { + assert!(matches!( + ins("ci", if i % 2 == 0 { "success" } else { "failure" }), + Inserted::Done(_) + )); + } + assert_eq!( + history(&conn, repo.id, "abc").unwrap().len() as i64, + MAX_HISTORY + ); + assert_eq!(latest(&conn, repo.id, "abc").unwrap().len(), 1); + for i in 1..MAX_CONTEXTS { + assert!(matches!( + ins(&format!("c{i}"), "success"), + Inserted::Done(_) + )); + } + assert!(matches!( + ins("one-too-many", "success"), + Inserted::TooManyContexts + )); + assert!( + matches!(ins("ci", "pending"), Inserted::Done(_)), + "an existing context may still post" + ); + let _ = std::fs::remove_dir_all(&d); + } +} diff --git a/crates/forge/tests/e2e_protection.rs b/crates/forge/tests/e2e_protection.rs new file mode 100644 index 00000000..f10c16ca --- /dev/null +++ b/crates/forge/tests/e2e_protection.rs @@ -0,0 +1,634 @@ +//! F4a: branch protection and commit statuses, checked with the real `git` client and the HTTP API. +//! Delete and force-push cases use a protected branch that is **not** the default: on the default +//! branch the default-branch rule answers first and would hide a missing protection check. +#![cfg(unix)] + +mod common; +use common::*; +use serde_json::{json, Value}; +use std::path::{Path, PathBuf}; +use std::process::Command; + +struct Client { + dir: PathBuf, +} + +impl Client { + fn new(tag: &str) -> Client { + let dir = std::env::temp_dir().join(format!("scl-forge-prot-{tag}-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&dir); + std::fs::create_dir_all(&dir).unwrap(); + Client { dir } + } + + fn git_in(&self, cwd: &Path, header: Option<&str>, args: &[&str]) -> (bool, String) { + let mut cmd = Command::new("git"); + cmd.env_clear() + .env("PATH", "/usr/bin:/bin:/usr/local/bin:/opt/homebrew/bin") + .env("HOME", &self.dir) + .env("GIT_CONFIG_NOSYSTEM", "1") + .env("GIT_TERMINAL_PROMPT", "0") + .env("GIT_AUTHOR_NAME", "T") + .env("GIT_AUTHOR_EMAIL", "t@example.invalid") + .env("GIT_COMMITTER_NAME", "T") + .env("GIT_COMMITTER_EMAIL", "t@example.invalid") + .current_dir(cwd); + if let Some(h) = header { + cmd.arg("-c") + .arg(format!("http.extraHeader=Authorization: {h}")); + } + let out = cmd.args(args).output().expect("run git"); + let text = format!( + "{}{}", + String::from_utf8_lossy(&out.stdout), + String::from_utf8_lossy(&out.stderr) + ); + (out.status.success(), text) + } + + fn ok(&self, cwd: &Path, header: Option<&str>, args: &[&str]) -> String { + let (ok, text) = self.git_in(cwd, header, args); + assert!(ok, "git {args:?}: {text}"); + text + } + + fn commit(&self, work: &Path, file: &str, text: &str) { + std::fs::write(work.join(file), text).unwrap(); + self.ok(work, None, &["add", "."]); + self.ok(work, None, &["commit", "-q", "-m", file]); + } +} + +impl Drop for Client { + fn drop(&mut self) { + let _ = std::fs::remove_dir_all(&self.dir); + } +} + +struct W { + s: Srv, + auth: String, + id: i64, +} + +/// `siteadmin/w` with the README commit on `main`. +fn world() -> W { + let s = Srv::start(); + let auth = token(&s.admin); + let r = s.post( + "/api/v1/user/repos", + &auth, + json!({"name": "w", "default_branch": "main", "auto_init": true}), + ); + assert_eq!(r.status, 201, "{}", r.msg()); + let id = r.json()["id"].as_i64().unwrap(); + W { s, auth, id } +} + +impl W { + fn api(&self, method: &str, path: &str, body: Option) -> Resp { + self.s.send( + method, + &format!("/api/v1/repos/siteadmin/w{path}"), + Some(&self.auth), + body.as_ref(), + ) + } + fn git(&self, args: &[&str]) -> String { + self.s.git(self.id, args, None) + } + fn url(&self) -> String { + format!("{}/siteadmin/w.git", self.s.base) + } + fn rule(&self, branch: &str, push: bool) { + let r = self.api( + "POST", + "/branch_protections", + Some(json!({"branch_name": branch, "enable_push": push})), + ); + assert_eq!(r.status, 201, "{}", r.msg()); + } + /// A working copy cloned from `main` with `rel` created from it and pushed. + fn clone_with_rel(&self, c: &Client) -> PathBuf { + let work = c.dir.join("work"); + c.ok( + &c.dir, + Some(&self.auth), + &["clone", "-q", &self.url(), work.to_str().unwrap()], + ); + c.ok(&work, None, &["checkout", "-q", "-b", "rel"]); + c.ok(&work, Some(&self.auth), &["push", "-q", "origin", "rel"]); + work + } +} + +fn b64s(s: &str) -> String { + b64(s) +} + +#[test] +fn rule_crud_validation_and_the_branch_object() { + let w = world(); + let r = w.api( + "POST", + "/branch_protections", + Some(json!({ + "branch_name": "rel", "enable_push": false, + "enable_status_check": true, "status_check_contexts": ["ci/test"] + })), + ); + assert_eq!(r.status, 201, "{}", r.msg()); + let j = r.json(); + assert_eq!(j["branch_name"], "rel"); + assert_eq!(j["rule_name"], "rel"); + assert_eq!(j["status_check_contexts"], json!(["ci/test"])); + assert_eq!(j["required_approvals"], 0); + + for (name, body, status, text) in [ + ( + "duplicate", + json!({"branch_name": "rel", "enable_push": true}), + 403, + "Branch protection already exist", + ), + ( + "case alias", + json!({"branch_name": "REL"}), + 403, + "Branch protection already exist", + ), + ( + "glob", + json!({"branch_name": "release/*"}), + 422, + "patterns are not supported", + ), + ( + "approvals", + json!({"branch_name": "x", "required_approvals": 2}), + 422, + "unsupported branch protection setting: required_approvals", + ), + ( + "checks without contexts", + json!({"branch_name": "x", "enable_status_check": true}), + 422, + "status_check_contexts is required", + ), + ( + "context pattern", + json!({"branch_name": "x", "status_check_contexts": ["ci/*"]}), + 422, + "patterns are not supported", + ), + ( + "two names", + json!({"branch_name": "a", "rule_name": "b"}), + 422, + "must be the same", + ), + ("no name", json!({}), 422, "Required"), + ] { + let r = w.api("POST", "/branch_protections", Some(body)); + assert_eq!( + (r.status, r.msg().contains(text)), + (status, true), + "{name}: {}", + r.msg() + ); + } + + // the branch object reports the rule + let b = w.api("GET", "/branches/main", None).json(); + assert_eq!(b["protected"], false); + w.git(&["update-ref", "refs/heads/rel", "refs/heads/main"]); + let b = w.api("GET", "/branches/rel", None).json(); + assert_eq!(b["protected"], true); + assert_eq!(b["user_can_push"], false); + assert_eq!(b["enable_status_check"], true); + assert_eq!(b["status_check_contexts"], json!(["ci/test"])); + assert_eq!(b["effective_branch_protection_name"], "rel"); + + let p = w.api( + "PATCH", + "/branch_protections/rel", + Some(json!({"enable_push": true})), + ); + assert_eq!(p.status, 200); + assert_eq!(p.json()["enable_push"], true); + assert_eq!(p.json()["status_check_contexts"], json!(["ci/test"])); + let bad = w.api( + "PATCH", + "/branch_protections/rel", + Some(json!({"status_check_contexts": []})), + ); + assert_eq!( + bad.status, 422, + "checks stay on, so contexts cannot be emptied" + ); + assert_eq!( + w.api("GET", "/branch_protections", None) + .json() + .as_array() + .unwrap() + .len(), + 1 + ); + assert_eq!(w.api("GET", "/branch_protections/REL", None).status, 200); + assert_eq!(w.api("DELETE", "/branch_protections/rel", None).status, 204); + assert_eq!(w.api("GET", "/branch_protections/rel", None).status, 404); + assert_eq!(w.api("DELETE", "/branch_protections/rel", None).status, 404); +} + +#[test] +fn only_a_repository_admin_manages_rules() { + let w = world(); + let pw = w.s.mk_user("alice"); + let t = w.s.mk_token("alice", &pw, "t", &["all"]); + let put = w.api( + "PUT", + "/collaborators/alice", + Some(json!({"permission": "write"})), + ); + assert_eq!(put.status, 204); + let tok = token(&t); + let try_ = |m: &str, p: &str, b: Option| { + w.s.send( + m, + &format!("/api/v1/repos/siteadmin/w{p}"), + Some(&tok), + b.as_ref(), + ) + }; + let r = try_( + "POST", + "/branch_protections", + Some(json!({"branch_name": "x"})), + ); + assert_eq!(r.status, 403); + assert!( + r.msg().contains("owner or a collaborator with admin"), + "{}", + r.msg() + ); + assert_eq!( + try_("GET", "/branch_protections", None).status, + 200, + "a writer may read" + ); + w.rule("rel", false); + assert_eq!( + try_( + "PATCH", + "/branch_protections/rel", + Some(json!({"enable_push": true})) + ) + .status, + 403 + ); + assert_eq!(try_("DELETE", "/branch_protections/rel", None).status, 403); + assert_eq!( + w.api("GET", "/branch_protections/rel", None).json()["enable_push"], + false + ); +} + +#[test] +fn pushes_under_a_rule() { + let w = world(); + let c = Client::new("push"); + let work = w.clone_with_rel(&c); + w.rule("rel", false); + + // enable_push=false: nothing moves the branch, administrators included + c.commit(&work, "a.txt", "a\n"); + let (ok, out) = c.git_in(&work, Some(&w.auth), &["push", "origin", "rel"]); + assert!( + !ok && out.contains("direct pushes are not allowed"), + "{out}" + ); + let (ok, out) = c.git_in( + &work, + Some(&w.auth), + &["push", "origin", "rel:refs/heads/Rel"], + ); + assert!( + !ok && out.contains("protected branch Rel"), + "a case alias is the same rule: {out}" + ); + let (ok, out) = c.git_in(&work, Some(&w.auth), &["push", "origin", ":rel"]); + assert!(!ok && out.contains("protected branch rel"), "{out}"); + + // enable_push=true: fast-forwards (carrying new objects, which sit in git's quarantine) pass + let p = w.api( + "PATCH", + "/branch_protections/rel", + Some(json!({"enable_push": true})), + ); + assert_eq!(p.status, 200); + let ff = c.ok(&work, Some(&w.auth), &["push", "origin", "rel"]); + assert!(ff.contains("rel -> rel"), "{ff}"); + // a forced rewrite and a deletion are refused for everyone + let tip = w.git(&["rev-parse", "refs/heads/rel"]); + c.ok(&work, None, &["reset", "-q", "--hard", "HEAD~1"]); + c.commit(&work, "b.txt", "b\n"); + let (ok, out) = c.git_in(&work, Some(&w.auth), &["push", "-f", "origin", "rel"]); + assert!( + !ok && out.contains("force-push protected branch rel"), + "{out}" + ); + let (ok, out) = c.git_in(&work, Some(&w.auth), &["push", "origin", ":rel"]); + assert!(!ok && out.contains("delete protected branch rel"), "{out}"); + assert_eq!( + w.git(&["rev-parse", "refs/heads/rel"]), + tip, + "neither attempt moved the branch" + ); + + // an unruled branch is unaffected, including force pushes and deletion + c.ok( + &work, + Some(&w.auth), + &["push", "-q", "origin", "HEAD:refs/heads/free"], + ); + c.ok(&work, None, &["commit", "-q", "--amend", "-m", "rewritten"]); + c.ok( + &work, + Some(&w.auth), + &["push", "-q", "-f", "origin", "HEAD:refs/heads/free"], + ); + c.ok(&work, Some(&w.auth), &["push", "-q", "origin", ":free"]); + + // removing the rule lifts it + assert_eq!(w.api("DELETE", "/branch_protections/rel", None).status, 204); + c.ok(&work, Some(&w.auth), &["push", "-q", "-f", "origin", "rel"]); +} + +#[test] +fn a_rule_reserves_its_branch_name_for_a_push_that_creates_it() { + let w = world(); + let c = Client::new("create"); + let work = c.dir.join("work"); + c.ok( + &c.dir, + Some(&w.auth), + &["clone", "-q", &w.url(), work.to_str().unwrap()], + ); + w.rule("future", false); + let (ok, out) = c.git_in( + &work, + Some(&w.auth), + &["push", "origin", "HEAD:refs/heads/future"], + ); + assert!(!ok && out.contains("protected branch future"), "{out}"); + let r = w.api("GET", "/branches/future", None); + assert_eq!(r.status, 404); +} + +#[test] +fn contents_writes_under_a_rule() { + let w = world(); + w.git(&["update-ref", "refs/heads/rel", "refs/heads/main"]); + w.rule("rel", false); + let write = |body: Value| w.api("POST", "/contents/f.txt", Some(body)); + let denied = write(json!({"content": b64s("x\n"), "message": "m", "branch": "rel"})); + assert_eq!(denied.status, 403); + assert!( + denied.msg().contains("user cannot commit to repo"), + "{}", + denied.msg() + ); + // a new branch cut from a protected base is fine (the rule covers the target), the name of a ruled branch is not + let ok = write( + json!({"content": b64s("x\n"), "message": "m", "branch": "rel", "new_branch": "topic"}), + ); + assert_eq!(ok.status, 201, "{}", ok.msg()); + w.rule("reserved", false); + let r = w.api( + "POST", + "/contents/g.txt", + Some(json!({"content": b64s("x\n"), "message": "m", "branch": "main", "new_branch": "reserved"})), + ); + assert_eq!(r.status, 403, "{}", r.msg()); + assert_eq!(w.api("GET", "/branches/reserved", None).status, 404); + // enable_push=true allows the write + w.api( + "PATCH", + "/branch_protections/rel", + Some(json!({"enable_push": true})), + ); + let ok = write(json!({"content": b64s("y\n"), "message": "m", "branch": "rel"})); + assert_eq!(ok.status, 201, "{}", ok.msg()); +} + +#[test] +fn deleting_a_protected_branch_through_the_api_is_refused() { + let w = world(); + w.git(&["update-ref", "refs/heads/rel", "refs/heads/main"]); + w.git(&["update-ref", "refs/heads/loose", "refs/heads/main"]); + w.rule("rel", true); + let r = w.api("DELETE", "/branches/rel", None); + assert_eq!(r.status, 403); + assert!(r.msg().contains("protected"), "{}", r.msg()); + assert_eq!(w.api("GET", "/branches/rel", None).status, 200); + assert_eq!(w.api("DELETE", "/branches/loose", None).status, 204); + assert_eq!( + w.api("DELETE", "/branches/main", None).msg(), + "can not delete default branch" + ); +} + +#[test] +fn a_rule_change_is_ordered_against_contents_writes() { + let w = world(); + w.git(&["update-ref", "refs/heads/rel", "refs/heads/main"]); + w.rule("rel", true); + let stop = std::sync::atomic::AtomicBool::new(false); + let (tip_after_change, ok_count) = std::thread::scope(|sc| { + let writer = sc.spawn(|| { + let mut ok = 0; + let mut i = 0; + while !stop.load(std::sync::atomic::Ordering::SeqCst) { + i += 1; + let r = w.api( + "POST", + &format!("/contents/n{i}.txt"), + Some(json!({"content": b64s("x\n"), "message": "m", "branch": "rel"})), + ); + match r.status { + 201 => ok += 1, + 403 | 409 | 503 => {} + s => panic!("unexpected {s}: {}", r.msg()), + } + } + ok + }); + std::thread::sleep(std::time::Duration::from_millis(400)); + let p = w.api( + "PATCH", + "/branch_protections/rel", + Some(json!({"enable_push": false})), + ); + assert_eq!(p.status, 200, "{}", p.msg()); + let tip = w.git(&["rev-parse", "refs/heads/rel"]); + // a write attempted after the change returned is refused (and the writer keeps trying) + let after = w.api( + "POST", + "/contents/after.txt", + Some(json!({"content": b64s("x\n"), "message": "m", "branch": "rel"})), + ); + assert_eq!(after.status, 403, "{}", after.msg()); + std::thread::sleep(std::time::Duration::from_millis(400)); + stop.store(true, std::sync::atomic::Ordering::SeqCst); + (tip, writer.join().unwrap()) + }); + assert!(ok_count > 0, "the writer made progress before the change"); + assert_eq!( + w.git(&["rev-parse", "refs/heads/rel"]), + tip_after_change, + "no write lands after the rule change returned" + ); +} + +#[test] +fn statuses_round_trip_and_their_limits() { + let w = world(); + let sha = w.git(&["rev-parse", "refs/heads/main"]); + let post = |p: &str, b: Value| w.api("POST", p, Some(b)); + let r = post( + &format!("/statuses/{sha}"), + json!({"state": "success", "context": "ci/test", "description": "ok", "target_url": "https://ci.example/1"}), + ); + assert_eq!(r.status, 201, "{}", r.msg()); + let j = r.json(); + assert_eq!(j["status"], "success"); + assert_eq!(j["creator"]["login"], "siteadmin"); + assert_eq!( + j["creator"]["email"], "siteadmin@noreply.localhost", + "the stored address is never shown" + ); + assert_eq!(j["creator"]["is_admin"], false); + post( + &format!("/statuses/{sha}"), + json!({"state": "failure", "context": "ci/lint"}), + ); + post( + "/statuses/main", + json!({"state": "pending", "context": "ci/test"}), + ); + let c = w.api("GET", &format!("/commits/{sha}/status"), None).json(); + assert_eq!(c["state"], "failure"); + assert_eq!(c["sha"], sha.as_str()); + assert_eq!( + c["statuses"].as_array().unwrap().len(), + 2, + "latest per context" + ); + assert_eq!( + c["statuses"][0]["context"], "ci/lint", + "oldest latest first" + ); + let h = w.api("GET", &format!("/commits/{sha}/statuses"), None); + assert_eq!(h.json().as_array().unwrap().len(), 3); + assert_eq!(h.json()[0]["status"], "pending", "newest first"); + assert_eq!( + w.api("GET", &format!("/statuses/{sha}?limit=2&page=2"), None) + .json() + .as_array() + .unwrap() + .len(), + 1 + ); + + for (name, body, status) in [ + ("unknown state", json!({"state": "bogus"}), 422), + ("missing state", json!({"context": "x"}), 422), + ( + "script url", + json!({"state": "success", "target_url": "javascript:alert(1)"}), + 422, + ), + ( + "long context", + json!({"state": "success", "context": "x".repeat(1025)}), + 422, + ), + ( + "bad type", + json!({"state": "success", "description": 5}), + 422, + ), + ] { + let r = post(&format!("/statuses/{sha}"), body); + assert_eq!(r.status, status, "{name}: {}", r.msg()); + } + let nope = post("/statuses/nope", json!({"state": "success"})); + assert_eq!(nope.status, 404); + let unknown = post( + &format!("/statuses/{}", "0".repeat(40)), + json!({"state": "success"}), + ); + assert_eq!(unknown.status, 404); + assert_eq!( + w.api("GET", &format!("/commits/{}/status", "0".repeat(40)), None) + .status, + 404 + ); + + // a read-only collaborator cannot post + let pw = w.s.mk_user("carol"); + let t = token(&w.s.mk_token("carol", &pw, "t", &["all"])); + w.api( + "PUT", + "/collaborators/carol", + Some(json!({"permission": "read"})), + ); + let r = w.s.send( + "POST", + &format!("/api/v1/repos/siteadmin/w/statuses/{sha}"), + Some(&t), + Some(&json!({"state": "success"})), + ); + assert_eq!(r.status, 403); + assert!(r.msg().contains("permission to write"), "{}", r.msg()); +} + +#[test] +fn combined_status_does_not_deadlock_against_exclusive_repository_operations() { + // The combined status reads the repository twice (the revision, then the repository object); + // an exclusive operation (branch delete) queued between the two must not wedge either. + let w = world(); + let sha = w.git(&["rev-parse", "refs/heads/main"]); + let r = w.api( + "POST", + &format!("/statuses/{sha}"), + Some(json!({"state": "success", "context": "c"})), + ); + assert_eq!(r.status, 201); + let stop = std::sync::atomic::AtomicBool::new(false); + let (tx, rx) = std::sync::mpsc::channel(); + std::thread::scope(|sc| { + for _ in 0..3 { + sc.spawn(|| { + while !stop.load(std::sync::atomic::Ordering::SeqCst) { + let r = w.api("GET", &format!("/commits/{sha}/status"), None); + assert_eq!(r.status, 200, "{}", r.msg()); + } + }); + } + let (wr, shar, stopr) = (&w, &sha, &stop); + sc.spawn(move || { + for i in 0..40 { + wr.git(&["update-ref", &format!("refs/heads/t{i}"), shar]); + let r = wr.api("DELETE", &format!("/branches/t{i}"), None); + assert_eq!(r.status, 204, "{}", r.msg()); + } + stopr.store(true, std::sync::atomic::Ordering::SeqCst); + tx.send(()).unwrap(); + }); + assert!( + rx.recv_timeout(std::time::Duration::from_secs(60)).is_ok(), + "the deletes finished: no reader wedged the exclusive lock" + ); + }); +} diff --git a/docs/THREAT-MODEL.md b/docs/THREAT-MODEL.md index bc4b0e60..029a1706 100644 --- a/docs/THREAT-MODEL.md +++ b/docs/THREAT-MODEL.md @@ -992,6 +992,37 @@ Tests are in `crates/forge/tests/e2e_write.rs` and the `gitwrite` unit tests. authenticated user's. A write collaborator can still push any identity over git, as in Gitea. - **Unreferenced objects.** A refused or lost write leaves blobs/trees/commits no ref reaches; automatic gc is off (F5 adds gc). +### Branch protection and commit statuses (F4a) + +Threat-model pass #2, second part (protection across write paths); the merge path and the bypass matrix come with F4c. +Tests: `crates/forge/tests/e2e_protection.rs`, the `policy`, `hook`, `protection` and `statuses` unit tests. + +- **One policy for every ref move.** Push (`pre-receive`), contents API and branch DELETE call the same + `policy` functions on rules read from SQLite. Administrators are not exempt: `enable_push: false` refuses them too + (`pushes_under_a_rule`, `contents_writes_under_a_rule`). Tests use a non-default protected branch so the default-branch + rule cannot mask a missing check. +- **Fail closed.** An unreadable database, a hook without `FORGE_GIT_BIN`, an unexpected object directory, a malformed object + id, or an ancestry check that errors refuses the update. The ancestry check runs `git merge-base --is-ancestor` with the + runner's pinned environment (no replace objects, no system or global config, hooks directory fixed) and only the + quarantine's object directories added, each required to be under the repository's `objects/`. +- **Names.** Matching is exact and case-insensitive (a case alias of a protected name is protected); glob rule names, glob + context names and every unenforced protection setting are refused rather than stored, so a rule never looks stronger than it is. + A rule on a branch that does not exist yet reserves the name against push and contents creation. +- **Ordering.** Rule create/PATCH/DELETE hold the repository's write gate, as do contents writes, which re-read the rules + immediately before `update-ref`: after a PATCH returns, no contents write lands under the old rule + (`a_rule_change_is_ordered_against_contents_writes`). `receive-pack` does not take the gate: a rule created while a push is + between `pre-receive` and its ref transaction does not stop that push. Rule changes answer 503 while a write holds the gate. +- **Statuses are not attestations.** Any caller with write access may post any context for any commit of the repository + (Gitea's trust boundary). They gate merges only (F4c). Posting is bounded: 100 contexts per commit, 100 rows per context, + field length caps, `target_url` limited to plain `http(s)` so a status link cannot be a `javascript:` URL in a web client. + The author's stored email is not shown in the creator object. +- **Operator note.** The hook accepts the quarantine's object directories only when they are plain absolute paths under the + repository's `objects/`. A data directory whose path contains `:` or starts with `"` makes git quote or split them, and every + fast-forward push to a protected branch that allows pushes is then refused (fail closed, but easy to miss): keep such + characters out of `--data-dir`. +- **Branch delete.** The API refuses a protected branch and deletes with compare-and-swap on the observed tip, so a concurrent + push is not erased by a stale delete. + ## Reporting Security reports go to **toni@git-agentic.com** or GitHub private vulnerability diff --git a/docs/superpowers/plans/2026-10-24-p38-f4-pulls-statuses-merge-protection.md b/docs/superpowers/plans/2026-10-24-p38-f4-pulls-statuses-merge-protection.md new file mode 100644 index 00000000..87a3bb3f --- /dev/null +++ b/docs/superpowers/plans/2026-10-24-p38-f4-pulls-statuses-merge-protection.md @@ -0,0 +1,319 @@ +# P38 F4: pull requests, statuses, branch protection and merge + +Status: plan, before review. Spec: `docs/superpowers/specs/2026-10-22-p38-forge-design.md` +(sections "Merge", "Statuses", "Server-side Git operations", Appendix A). Contract: the +recorded fixtures (scenarios F, G, H, I, O, Q, T, U, W; P for paging) and `departures.json`, +which already carries the reviewed F4 departures. Built on F3b's seams: `policy::check_contents_write`, +`Lifecycle::with_write_row` (per-repository write gate, then the shared lifecycle lock), `gitwrite` +(`update_ref` CAS, `is_ancestor`, `classify`), `gitobj::resolve`. + +## Intent + +A Git forge that Engine can switch to by config: PRs with the Gitea object shapes, commit statuses, +branch protection that holds on every write path (push, contents API, merge), and a merge that is +crash-safe. F4 is too large for one reviewable PR (F2 and F3 were split the same way), so it ships as +three PRs, each with its own conformance gate letters, tests, independent review and docs: + +| PR | Scope | Gate letters added | Pending removed | +|---|---|---|---| +| F4a | migration 3, branch protection CRUD, enforcement in the `pre-receive` hook, contents API and branch delete; commit statuses (POST, combined, lists) | H, I, W | B#30 | +| F4b | migration 4, pulls (create, list, get, PATCH, diff, patch, commits, files), reviews, review comments, issue comments, `refs/forge/pull//head` retention refs, mergeability cache; merge probe `GET /pulls/{n}/merge` | F, O (merge-independent exchanges), P | D#13 | +| F4c | `POST /pulls/{n}/merge`, merge intents, recovery at startup and under the write gate, required-status gating, kill -9 restore test, threat-model pass #2, protection-bypass tests across all paths | G, O, Q, T, U (all) | none | + +Order matters: F4a's protection and statuses are inputs to F4c's merge gate; F4b's PR rows are inputs to +F4c's intents. Each PR leaves `main` consistent (routes of a later slice stay `not_implemented`, listed in +`slice-pending.json`). + +## Cross-cutting decisions + +1. **One decision point for ref moves.** `policy` grows `Rules` (the protection rules of one repository, + loaded from SQLite) and `check_push(rules, name, old, new, is_ff)`, `check_contents_write(rules, target, + creating)`, `check_branch_delete(rules, name)`. The hook, the contents API and the branch DELETE call these + and nothing else decides. The merge (F4c) is deliberately *not* subject to `enable_push` (recorded: a PR merge + into a protected branch is allowed), only to the status-check gate. +2. **Rule matching is exact, case-insensitive on the branch name** (`policy::same_branch_name`, the same + alias rule F2b uses). Gitea also supports glob patterns in `rule_name`; the recorded clients use plain + names. A `branch_name` or `rule_name` containing `* ? [ ]` is refused with 422 instead of being stored as a + literal that protects nothing (fail closed). Departure entry. +3. **Unsupported protection settings are refused, not ignored.** The 1.22.6 object has ~30 keys. We implement + `branch_name`/`rule_name`, `enable_push`, `enable_status_check`, `status_check_contexts`. Any other request + field that is present with a non-default value (`required_approvals > 0`, `enable_push_whitelist`, + `enable_merge_whitelist`, `block_on_*`, `dismiss_stale_approvals`, `require_signed_commits`, `protected_file_patterns`, + ...) answers 422 `unsupported branch protection setting: `. Echoing a setting we do not enforce would give + a false sense of security. Responses carry the full recorded key set with the defaults of the unsupported + keys. +4. **Semantics of `enable_push`** (recorded I, Q, W): `false`: nothing may move the branch except a PR merge — no + push (admin included), no contents write, no deletion; branch creation by push is refused too (the name is + reserved). `true`: a fast-forward push by anyone with write is accepted (also when `enable_status_check` is + on and unmet: required checks gate merges, not pushes, W#8); a non-fast-forward push and a deletion are refused + for everyone, admin included (Q#11, Q#12); contents writes are allowed (W). +5. **Linearization.** Rule create/PATCH/DELETE run under the per-repository write gate (`with_write_row`), so a + server-made ref write (contents, F4c merge) checks its rules under the same gate immediately before + `update-ref` and cannot interleave with a rule change. `receive-pack` does not take the gate; the hook reads + the rules at `pre-receive` time. A rule added during an in-flight push can therefore be bypassed by that one + push; git's ref transaction, not our gate, is the arbiter there (spec: "does not exclude receive-pack"). Stated + in the threat model rather than closed. +6. **Hook fast-forward check.** `check_push` needs `is_ff` for `old != 0 && new != 0` on a protected branch with + `enable_push=true`. The hook runs `git merge-base --is-ancestor ` in the repository, with + `GIT_DIR`, `GIT_OBJECT_DIRECTORY`, `GIT_ALTERNATE_OBJECT_DIRECTORIES` taken from its own environment + (receive-pack's quarantine holds the incoming objects) and everything else scrubbed (same sanitized env as the + runner, no config, no hooks). Exit 0 = ff, 1 = not ff, anything else or a failure to spawn = refuse + (fail closed). Only spawned when a rule applies to that ref. +7. **Database access.** Every handler keeps the F1 rule: database errors are logged and answered as a bare 500. + The hook opens the database read-only (existing `db::open_readonly`). +8. **Departures** (each gets a `departures.json` entry with reason where a recorded exchange differs): glob rule + names refused (2), unsupported settings refused (3), status POST with unknown/missing `state` is 422 and an + unknown commit is 404 (already recorded in departures), squash/rebase/ff-only merge styles 422 (already + recorded). + +## F4a: branch protection and statuses + +### Schema (migration 3) + +``` +CREATE TABLE branch_protections ( + id INTEGER PRIMARY KEY AUTOINCREMENT, + repo_id INTEGER NOT NULL REFERENCES repos(id) ON DELETE CASCADE, + branch TEXT NOT NULL, lower_branch TEXT NOT NULL, + enable_push INTEGER NOT NULL DEFAULT 0, enable_status_check INTEGER NOT NULL DEFAULT 0, + status_check_contexts TEXT NOT NULL DEFAULT '[]', -- JSON array of strings + created_unix INTEGER NOT NULL, updated_unix INTEGER NOT NULL, + UNIQUE (repo_id, lower_branch)); +CREATE TABLE commit_statuses ( + id INTEGER PRIMARY KEY AUTOINCREMENT, + repo_id INTEGER NOT NULL REFERENCES repos(id) ON DELETE CASCADE, + sha TEXT NOT NULL, context TEXT NOT NULL, state TEXT NOT NULL + CHECK (state IN ('pending','success','error','failure','warning','skipped')), + description TEXT NOT NULL DEFAULT '', target_url TEXT NOT NULL DEFAULT '', + creator_id INTEGER NOT NULL REFERENCES users(id), + created_unix INTEGER NOT NULL, updated_unix INTEGER NOT NULL); +CREATE INDEX commit_statuses_by_sha ON commit_statuses (repo_id, sha, context, id); +``` +`TABLES` in `db.rs` gains both names (healthz probes them). `repos::delete_row` already cascades. + +### Protection API (`protection_api.rs`) + +`GET|POST /repos/{o}/{r}/branch_protections`, `GET|PATCH|DELETE .../{name}` (name may contain `/`: a `{name..}` +tail). Actor `Repo(Admin)` for writes (recorded: write collaborator gets 403 "user should be an owner or a +collaborator with admin write of a repository"), `Repo(Read)` for reads. Scope `write:repository` / +`read:repository`. Status/texts as recorded: 201 create, 403 "Branch protection already exist", 404 "The +target couldn't be found." (use the existing `not_found`), 204 delete; GET list is an array (not paginated by +Gitea; none here). Body validated by key (3). `status_check_contexts` entries: strings, each ≤ 255 bytes, at +most 64 entries, no empty string (422). `enable_status_check` with no contexts is stored as given (recorded +Gitea accepts it; the merge gate then requires nothing). + +`branches` JSON (`repo_api::branch_json`) reads the rule: `protected`, `enable_status_check`, +`status_check_contexts` (null when no rule, as recorded in Q), `user_can_push` (recorded false when +`enable_push=false` even for admin), `effective_branch_protection_name`, `required_approvals` 0. + +### Enforcement + +* `policy.rs`: `Rules` (list of `{branch, enable_push, enable_status_check, contexts}`), `Rules::for_branch(name)`, + and the three checks of decision 1. Texts: push to a protected branch with `enable_push=false` → + `Not allowed to push to protected branch ` (recorded text minus Gitea's `Gitea:` prefix); force push / + delete with `enable_push=true` → `Not allowed to force-push to protected branch ` / + `Not allowed to delete protected branch `; contents API → 403 `user cannot commit to repo [user: ]` + (recorded Q#1-3); branch DELETE of a protected branch → 403 `protected branch, can not be deleted` (unrecorded; only the default + branch's `can not delete default branch` is recorded and already built). +* `hook.rs`: `lookup` returns default branch, live flag and `Rules`; `decide` takes the rules plus an `is_ff` callback. +* `content_write.rs`: replace the two `check_contents_write(_, _)` calls with the rules-carrying form; the second + call (immediately before `update-ref`) reloads the rules from SQLite on every attempt (a fresh read inside the + write gate, which also serializes with rule changes). +* `lifecycle::delete_branch`: refuse a protected branch under the gate. + +### Statuses (`status_api.rs`) + +`POST /repos/{o}/{r}/statuses/{sha..}` (Write; scope `write:repository`), `GET .../commits/{ref..}/status` +(combined), `GET .../commits/{ref..}/statuses` and `GET .../statuses/{ref..}` (history), newest first +(highest id), paged like Gitea (`page`, `limit` ≤ 50, `X-Total-Count`; recorded H uses `limit=100`, capped). +`{sha}` resolves through `gitobj::resolve` (F3a grammar: HEAD, branch, tag, hex id), peeled to a commit; the +stored `sha` is the full commit id. Unknown → 404 (departure recorded). Reads of an existing commit with no +statuses: combined `state:"pending"`, real `sha`, `total_count:0` (recorded departure/1.26.2). + +POST body: `state` (required, one of six, else 422), `context` (default `""`, as recorded H#36; ≤ 1024 bytes +else 422 — the recorded 300-byte context is accepted), `description` (≤ 1024 chars, 422 above), `target_url` (≤ 2048, +must be empty or an `http(s)://` URL, 422 otherwise; stops `javascript:` URLs that web clients render). +Response 201, key set recorded (`id,status,target_url,description,url,context,creator,created_at,updated_at`), +`creator` is the existing user JSON. + +Combined state (recorded H#13-H#35): over the latest status per context, any `error` or `failure` → `failure`; +else any `pending` → `pending`; else any `warning` → `warning`; else `success` if any success; only `skipped` → +`skipped`. Unrecorded combos (`warning` alone, all-`skipped`) follow that order, noted in the spec. +Latest per context = highest `id`. Bounded growth: a (repo, sha, context) keeps at most 100 rows; inserting the +101st deletes the oldest in the same transaction (departure: history is bounded; a writer posting statuses in a +loop cannot grow the database without limit). + +### F4a tests + +* Unit: `policy` matrix (rules × push/force/delete/create/contents/branch delete, case aliases, glob refused), + combined-state function (all pairs), body validators, hook `decide` with rules and an injected `is_ff`. +* E2E (`e2e_protection.rs`, real git): protected push refused for write collaborator and admin; ff push allowed with + `enable_push=true`, force/delete refused; unprotected branch force-push allowed; contents API 403/allowed with + `new_branch`; branch DELETE refused; rule change under concurrent contents writes (writer sees the new rule + immediately after the PATCH returns); branch-name alias `MAIN` rule; statuses round trip incl. 100-row cap. +* Gate: add H, I, W; shrink `slice-pending.json` (B#30 off; Q/U/G still wait for merge, which F4c gates). + +## F4b: pull requests, reviews, comments + +### Schema (migration 4) + +`repo_counters(repo_id PK, next_index)` (issue/PR numbers share one sequence per repository, as recorded: +`pulls#1` is `issues#1`), `pulls(repo_id, number, head_ref, base_ref, head_sha_at_open, title, body, +state open|closed, merged, merge_commit_sha, final_head_sha, changed_files_at_merge, creator_id, created_unix, +updated_unix, closed_unix, merged_unix, merged_by)`, `reviews(id, repo_id, pull_number, reviewer_id, state, body, +commit_id, official, created_unix)`, `review_comments(id, review_id, path, line, body, ...)`, +`issue_comments(id, repo_id, number, author_id, body, created_unix, updated_unix)`, +`mergeability_cache(repo_id, base_sha, head_sha, mergeable, changed_files)`. Indexed for newest-first listing and +the open `(repo, head_ref, base_ref)` uniqueness (409 "pull request already exists for these targets"). + +### Behaviours (recorded F, O) + +* Create: head and base are branches of this repository (a `owner:branch` head naming this repo is accepted; another repo + is 404 — forks are a non-goal). Errors as recorded: 404 `BaseNotExist`/target not found, 422 "There are no changes between + the head and the base", 422 `[Title]: Required`, 409 duplicate open PR. Pushing the head to base later does not mark the + PR merged (O#28). +* `head.sha` of an open PR is the live branch tip; a merged PR's `head.sha` is `final_head_sha` (departure O#20-22 recorded); + a closed PR shows its last head (retention ref). +* PATCH returns **201** (recorded F#22): `state`, `title`, `body`, `base`. Reopening a PR whose head branch is gone is refused 404. +* List: newest first, `state=open|closed|all`, `head` ignored as in Gitea (clients filter), default limit 30, max 50, invalid + limit falls back (recorded P), `X-Total-Count`. +* Mergeability and `changed_files` (open PRs): `merge-tree --write-tree` result cached by `(base_sha, head_sha)`; misses are + computed under a concurrency limit of 2 and are skipped on list endpoints beyond the first 10 misses per request (the + field is then the last cached value or null with `mergeable:true`: the recorded type-only comparison). +* `pulls/{n}.diff`, `.patch`, `/commits`, `/files` from `git diff`/`log` through `gitobj` (F3a has the diff and patch text + helpers; `base...head` merge-base form). Capped by F3a's output caps (413). +* Retention refs `refs/forge/pull//head`: written by the server (never by clients; the hook already refuses the namespace) + at create, at close and merge, and by `delete_branch` before it deletes a branch that is the head of an open PR. The ref is + hidden by `transfer.hideRefs` already (F2b); `allowAnySHA1InWant` keeps fetch-by-sha working. A delete of such a branch by + *push* is handled in `pre-receive` by writing the retention ref before the delete lands (the hook runs as the server user; a + failed push leaves a harmless extra ref). +* Reviews: states map `APPROVED|REQUEST_CHANGES|COMMENT|PENDING` as recorded; unknown `event` is `PENDING` (F#27-30); own-PR + approval 422 "approve your own pull is not allowed"; `official` is true only for write access holders. Review comments list + from `reviews/{id}/comments`; inline comments in the review POST body stored (path, line, body). +* Issue comments `GET|POST /issues/{n}/comments`: scopes `read:issue`/`write:issue` (recorded 403 text), 201 on create. +* Locking: PR create/PATCH take the per-repository write gate (`with_write_row`) because they write retention refs; reads use + `with_read`. Number allocation inside the same SQLite transaction as the insert. + +### F4b tests/gate + +Unit: PR JSON shape, list paging, review state machine. E2E (`e2e_pulls.rs`, real git): create/duplicate/no-changes, +force-push head follows, branch delete keeps PR and retention ref, close/reopen, reviews by role, comments, diff/patch bytes against +`git diff`. Gate: add F, O (merge exchanges stay pending until F4c), P. + +## F4c: merge, intents, recovery + +Implements the spec's "Merge" section as written (steps 1-7, crash recovery, `merge_intents` journal, stale-head and +nothing-to-merge answers, explicit 409 conflict, unrelated-histories 409, `Do` other than `merge` → 422, no admin bypass of +status checks). Plan-level decisions beyond the spec: + +* **Schema (migration 5):** `merge_intents(id, repo_id, pull_number, base_ref, base_oid, head_ref, head_oid, merge_oid, + state pending|done|aborted, created_unix)`. At most one pending intent per PR (UNIQUE partial index). +* **Lock order:** `with_write_row` gate (10 s, 503) → SQLite. The gate is held for steps 5-7. Recovery under the gate for a repository + also runs in the startup reconciler before the listener accepts. +* **Merge commit** built by `commit-tree -p base -p head` on the `merge-tree --write-tree` tree, message and identity as spec; `GIT_*_DATE` pinned + from the same clock as the intent row. +* **Status gate (step 3)** reads `commit_statuses` for `head_oid` only; latest per context; `success`/`skipped` satisfy. No bypass for admins. +* **Atomic ref step:** `update-ref --stdin` transaction with `verify` of the head and `update` of the base (spec step 6). Failure + classification reuses `gitwrite::classify` (landed/lost/locked). Bounded recompute (2), then 405 "try again later". +* **Kill -9 test:** a harness that runs merges and pushes, sends SIGKILL at randomized points, restarts, and asserts for every PR that + `merged` agrees with `merge_oid` being an ancestor of base and that no `pending` intent survives reconcile. +* **Threat-model pass #2** in `docs/THREAT-MODEL.md`: server-side writes, merge races, contents/push/merge protection parity, retention + ref namespace, status trust, comment/body limits. +* **Protection-bypass matrix test** (`e2e_bypass.rs`): for each of {push, force push, delete, contents POST/PUT/DELETE with/without + `new_branch`, branch DELETE API, merge} × {enable_push false/true} × {admin, write, read} against the expected outcome table. + +## Verification and process (all three PRs) + +* `cargo test -p scl-forge`, `forge-check.sh` (gate grows each PR), `cargo clippy -D warnings`, fmt; CI toolchain 1.96.1. +* Independent review per PR: Opus `code-reviewer` then Sol (`gpt-6.1-sol`) on F4a (auth/permissions) and F4c (data integrity); Opus on F4b. +* Docs per PR: spec "F4x decisions (as built)", THREAT-MODEL sections, ROADMAP, departures entries, memory update. + +## Questions for the reviewers + +1. Is "exact name, case-insensitive, globs refused" the right fail-closed choice, or should globs be implemented now? +2. Hook `merge-base --is-ancestor` in the quarantine environment: any hole (replace refs, grafts, `GIT_*` smuggling, alternates)? +3. Is bounding status history per context (100) acceptable, or does some client need the whole history? +4. Retention-ref writing in `pre-receive` for pushed deletes: acceptable, or should deletions of an open PR's head be refused/handled elsewhere? +5. Slicing: is moving the status gate into F4c (needs F4a statuses + F4b PRs) the right order? + +## Decisions after independent review (Opus + Sol; this section overrides earlier text where they differ) + +Both reviewers confirmed the protection semantics table (I, Q, W), exact-name matching with globs refused, the hook's +`merge-base --is-ancestor` check (with the conditions in R9) and the slicing order. Findings that changed the plan: + +**Merge (F4c)** + +* **R1. Revalidate under the gate.** Merge takes the gate first, then: run recovery for pending intents of this repository, + re-read the PR (state, base, head ref), the protection rules and the required statuses, and only then insert the intent. + Rule CRUD, PR PATCH and merges all hold the gate, so they are totally ordered; statuses do not take the gate, so the + status check linearizes at its read inside the gate (a status posted after it is not counted). Rule CRUD answers 503 + while a merge holds the gate (documented). +* **R2. Durable receipt.** The step-6 `update-ref --stdin` transaction also creates `refs/forge/merge/` = + `merge_oid` (server-only namespace, already hidden and refused to clients). Recovery tests for the receipt, not for + ancestry of the current base: receipt present = the transaction committed (finalize, even when the base was force-pushed + away or deleted afterwards); receipt absent and intent pending = aborted. The receipt is deleted after the SQLite + finalization (and by recovery). The kill -9 invariant is "no pending intent survives reconcile, `merged` ⇔ intent done, + no receipt without a done intent", not "merge_oid is an ancestor of base" (a permitted force push breaks that). +* **R3. Complete intent row.** `merge_intents` also stores `merger_id`, `merged_unix`, `changed_files`, `additions`, + `deletions`; `(base_oid, merge_oid)` is rewritten in a committed transaction before every `update-ref` attempt (a + recompute changes `merge_oid`); recovery finalizes with exactly the fields a normal merge writes; the retention-ref write + in finalization is idempotent; the table cascades on repository delete. +* **R4. Stale-head classification is merge's own.** `gitwrite::classify` is single-ref. The merge uses its own outcome + function: reconcile the receipt first, then re-read head and base separately; changed head → 409 `head out of date` + (never silently substituted), changed base → bounded recompute, then 405 `try again later`. +* **R5. Step 7 failure.** If the SQLite finalization fails after the ref landed, retry inside the gate; a read that finds a + pending intent runs recovery inline when it can take the gate without waiting (otherwise reports not merged and leaves + the intent), so the next request converges. + +**Pull requests (F4b)** + +* **R6. Deleting the head branch closes the PR** (fixtures O#6-O#10, O#24-O#25, both Gitea versions; the spec's and + FINDINGS' "leaves the PR open" is wrong and is corrected in F4b's docs). Retention ref is written first, the PR is + closed, `head.ref` reads `refs/pull//head` with `head.label` the branch name. Reopen is allowed (201) even when the + branch is gone (O#26). Merge of such a PR is 404. +* **R7. Retention without a second writer process.** Instead of a `post-receive` hook doing database writes (the hook has a + read-only handle), the server runs `pulls::sync(repo)` under the gate after every accepted push (accept marker, but + refs are compared, not the marker trusted), contents write, branch API delete and at startup reconcile: for each open PR + it CAS-updates `refs/forge/pull//head` to the head branch tip, or, when the branch is gone, keeps the retained head, + closes the PR and records `final_head_sha`; for closed/merged PRs it repairs the ref to `final_head_sha`. The spec's + `freezing` state is unnecessary because refresh and freeze are both gate-ordered in one process; spec updated when built. + The `pre-receive` hook still records `old` into the retention ref (scrubbed env, quarantine variables removed, no object + from the push is referenced) when a push deletes the head of an open PR, so a crash before `sync` cannot lose the tip; + startup reconcile repairs any disagreement. Create writes the ref before inserting the row. +* **R8. Branch delete is a CAS.** `delete_branch` reads the tip, retains it, then deletes with expected old id; on mismatch it + re-reads and retries (≤3) or answers 409. It reads protection rules under the exclusive lifecycle lock it already holds + (never `with_write_row` from inside it: self-deadlock). + +**Protection and hook (F4a)** + +* **R9. Hook conditions.** Environment from `Git::env_with` (keeps `GIT_NO_REPLACE_OBJECTS`, `core.useReplaceRefs=false`); + git binary path passed explicitly (the pinned `PATH` may not contain it); repository directory derived from + `FORGE_DATA_DIR` + repo id, never from `GIT_DIR`; only `GIT_OBJECT_DIRECTORY` and `GIT_ALTERNATE_OBJECT_DIRECTORIES` are + taken from the inherited environment and must lie under that repository; `old`/`new` validated as hex before argv; + `GIT_PUSH_OPTION_*` and everything else scrubbed. Git refuses ref updates while `GIT_QUARANTINE_PATH` is set, so the + retention write in R7 spawns without any `GIT_QUARANTINE_PATH`/object-dir variables. +* **R10. Refusal texts and departures.** The gate compares the refusal lines. Use the recorded text where it is ours to say + (`not allowed to push to protected branch `, `branch is protected from force push`) and add a departure entry for + the dropped `gitea:` prefix and for the default-branch delete wording (like the existing J#62 entry); every text that + differs gets an entry, decided by running the gate. +* **R11. Protection tests use a non-default protected branch** for delete and force-push cases (Q#12/Q#15-16 target + `main`, where the default-branch rule fires first); also creation of a previously absent protected name and contents + `new_branch` targeting a protected name (stricter than Gitea, which checks only the old branch: departure note). +* **R12. Rule body.** `status_check_contexts`/`rule_name` handling per the fixtures: a branch with no rule reports + `status_check_contexts: null`, a rule without contexts reports what Q#0/W#0 show (the gate decides; the column is + nullable so the distinction survives); conflicting `branch_name` vs `rule_name` → 422; glob metacharacters refused in + rule names on create and PATCH and in context names. `enable_status_check` with no contexts → 422 (departure; a gate that + requires nothing would fail open). +* **R13. Statuses.** Cap distinct contexts per sha (100, 422 above) besides the 100-row history per context, so the table is + bounded per commit; the merge gate reads the table directly, never the paged combined view (P#40-41: combined is paged, + `X-Total-Count` is the full count). A POST without `state` is 422 (H#37 recorded 201: add the departure entry). +* **R14. Gate mechanics.** F4a adds `"Q": {"stop": [5, "expected [201]"]}` to `slice-pending.json` (gating the contents-API + protection contract Q#0-4 now); F4b derives O's `defer` list by running the scenario (a deferred exchange that passes + is reported FAIL). + +**Contract details F4b must cover** (from Opus's fixture pass): PR create is allowed unless head sha equals base sha (G#35, +T#5, T#9: contained or same-tree heads still get 201); 422 text `Invalid PullRequest: There are no changes between the head +and the base`; PR object keys `additions`, `deletions`, `merge_base`, `comments`, `review_comments`; review POST answers +200; other users' PENDING reviews are hidden from lists but counted in `X-Total-Count`; `official` follows the recorded +transitions (F#28 vs F#33), computed at read time; list `sort=oldest|newest|recentupdate` and `Link` headers (P#0-4, +P#16-18). diff --git a/docs/superpowers/specs/2026-10-22-p38-forge-design.md b/docs/superpowers/specs/2026-10-22-p38-forge-design.md index 4353277d..4b2f278e 100644 --- a/docs/superpowers/specs/2026-10-22-p38-forge-design.md +++ b/docs/superpowers/specs/2026-10-22-p38-forge-design.md @@ -901,6 +901,46 @@ F3b is the write half of the content API (plan: `docs/superpowers/plans/2026-10- deterministic interfering writer); the F4 `refs/forge/pull//head` refresh for a contents write to a PR branch; portable-name collision rules; a disk reservation sized to the exact rebuilt trees. +### F4a decisions (as built) + +F4 ships as three PRs (F4a protection and statuses, F4b pulls, reviews and comments, F4c merge and recovery; plan: +`docs/superpowers/plans/2026-10-24-p38-f4-pulls-statuses-merge-protection.md` with its "Decisions after independent +review"). F4a adds migration 3 (`branch_protections`, `commit_statuses`) and gates scenarios H, I, W and the first five +exchanges of Q. + +- **Rules.** `/branch_protections` CRUD (admin to write, read to read). A rule names one branch, compared + case-insensitively like `same_branch_name`; glob names and glob context names are refused (422), as is any setting + `sc-forge` does not enforce when set to a non-default value (`required_approvals`, whitelists, `block_on_*`, + `require_signed_commits`, `protected_file_patterns`, the 1.26.2 force-push keys). `enable_status_check` needs a + non-empty `status_check_contexts` (a gate that requires nothing would fail open). Responses carry the recorded key set; + a rule created without contexts reports `status_check_contexts: null`, a branch with no rule reports `[]` on the + branch object. Rule changes run under the write gate, so they are ordered against contents writes. +- **`enable_push` false:** no push (administrators included), no contents write, no deletion; the rule also reserves + its name, so a push or a contents `new_branch` cannot create the branch (stricter than Gitea, which checks only an + existing branch). **True:** a fast-forward push and a contents write are allowed (even with required checks unmet, + W#8); a non-fast-forward push and a deletion are refused for everyone. A merge is not subject to `enable_push`. +- **One policy.** `policy::{check_protected_push, check_contents_write, check_branch_delete}` take the loaded `Rules`; + the `pre-receive` hook, the contents API (twice per attempt, the second read immediately before `update-ref`, under + the gate) and the branch DELETE call them. The hook decides fast-forward with `git merge-base --is-ancestor` under the + runner's sanitized environment plus the quarantine's `GIT_OBJECT_DIRECTORY` and `GIT_ALTERNATE_OBJECT_DIRECTORIES` + (each must lie under the repository's own `objects/`), the git binary from `FORGE_GIT_BIN`; any doubt refuses. + A push is not excluded by the gate, so a rule created while a push is in flight can be bypassed by that one push. +- **Branch DELETE** refuses a protected branch (403 `branch is protected and cannot be deleted`) and deletes with + the observed tip as the expected old id (retrying a few times), so a push that lands between the read and the delete + is not deleted with it. +- **Statuses.** `POST /statuses/{sha..}` (write; the sha may be a branch, tag, `HEAD` or id, resolved to a commit), + `GET /statuses/{ref..}` and `GET /commits/{ref}/statuses` (history, newest first, paged), `GET /commits/{ref}/status` + (latest per context, oldest first, paged; `total_count` is the page, `X-Total-Count` the whole). Unknown state or a + missing `state` is 422, unknown commit 404 (departures); `context` up to 1024 bytes and not defaulted (recorded H#36), + `description` up to 1024 characters, `target_url` empty or plain `http(s)`. The combined state is `failure` for any + `error`/`failure`, else `pending`, `warning`, `success`, `skipped` (only the first three are recorded). A commit with + no statuses reads `pending` with the real sha and `repository: null`. The creator object shows + `@noreply.localhost`, never the stored address, and `is_admin: false`. Storage is bounded per commit: 100 + distinct contexts and 100 rows per context (oldest dropped; a departure from Gitea, which keeps everything). +- **Refusal texts.** Pushes are refused with `refusing to update protected branch : direct pushes are not allowed`, + `refusing to force-push protected branch ` and `refusing to delete protected branch ` (departure entries for + I#7, I#8 and I#10; the default branch keeps F2b's wording and is checked first). + ## Open questions 1. The merge of a PR whose head is already in base (recorded: 405 `try again later`,