diff --git a/changelog.d/branch-names-as-written.breaking.md b/changelog.d/branch-names-as-written.breaking.md new file mode 100644 index 000000000..5f57bae3b --- /dev/null +++ b/changelog.d/branch-names-as-written.breaking.md @@ -0,0 +1,9 @@ +- Branch names are used exactly as written. A name with leading or trailing + whitespace or a control character is now refused as a bad request wherever + a branch is named (HTTP routes, CLI commands and embedded engine calls), as + branch statements already refused it; such a name used to be trimmed, so + `main ` named `main`. Clients that send padded names must send the exact + name. A policy whose `protected_branches` lists such a name now fails + validation. See [branch names][branch-names-as-written]. + +[branch-names-as-written]: ../docs/user/branching/index.md#branch-names diff --git a/crates/omnigraph-cli/src/client.rs b/crates/omnigraph-cli/src/client.rs index 3fe6760a5..90f13d041 100644 --- a/crates/omnigraph-cli/src/client.rs +++ b/crates/omnigraph-cli/src/client.rs @@ -1790,12 +1790,11 @@ impl GraphClient { dry_run: bool, settings: &[(SettingId, SettingValue)], ) -> Result { - // Use the engine's canonical spelling for dispatch, receipt validation, - // and optional deletion. Padding must not cause a post-publication error. - let source = source.trim(); - let into = into.trim(); - if source.is_empty() || into.is_empty() { - bail!("branch merge source and target must not be empty"); + // Dispatch, receipt validation and optional deletion all use the names + // as given, so refuse one the engine and the server would refuse before + // any storage or HTTP work: none can then fail after publication. + for name in [source, into] { + omnigraph_policy::ensure_canonical_branch_name(name)?; } let output = match self { GraphClient::Remote { diff --git a/crates/omnigraph-cli/tests/integration/cli_data.rs b/crates/omnigraph-cli/tests/integration/cli_data.rs index 5dffed46b..34beb0108 100644 --- a/crates/omnigraph-cli/tests/integration/cli_data.rs +++ b/crates/omnigraph-cli/tests/integration/cli_data.rs @@ -3369,7 +3369,7 @@ fn branch_merge_defaults_target_to_main() { .arg("merge") .arg("--uri") .arg(&graph) - .arg(" feature ") + .arg("feature") .arg("--json"), ); let merge_payload: Value = serde_json::from_slice(&merge_output.stdout).unwrap(); @@ -3421,7 +3421,12 @@ fn remote_merge_requires_consistent_receipts_without_replay() { use crate::support::managed_http::{IntentApiFixture, IntentReply}; use serde_json::json; - for (source, target) in [(" ", "main"), ("review", "\t")] { + for (source, target) in [ + (" ", "main"), + ("review", "\t"), + (" review ", "main"), + ("review", " main "), + ] { let server = IntentApiFixture::graph(vec![]); let output = output_failure(cli().args([ "--server", @@ -3434,9 +3439,10 @@ fn remote_merge_requires_consistent_receipts_without_replay() { "--into", target, ])); + let stderr = String::from_utf8_lossy(&output.stderr); assert!( - String::from_utf8_lossy(&output.stderr) - .contains("branch merge source and target must not be empty"), + stderr.contains("has leading or trailing whitespace") + || stderr.contains("contains a control character"), "{output:?}" ); assert!(server.requests().is_empty()); @@ -3450,7 +3456,7 @@ fn remote_merge_requires_consistent_receipts_without_replay() { "created_at": 1234567 }); let mut matrix = Vec::new(); - for (statement, padded) in [(false, false), (false, true), (true, false)] { + for statement in [false, true] { let mut cases = vec![ ("fast_forward", Some(commit.clone()), true), ("merged", Some(commit.clone()), true), @@ -3473,13 +3479,13 @@ fn remote_merge_requires_consistent_receipts_without_replay() { cases.push(("merged", Some(bad_commit), false)); } for (outcome, receipt, valid) in cases { - matrix.push((statement, padded, outcome, receipt, valid)); + matrix.push((statement, outcome, receipt, valid)); } } for_each_concurrently( matrix, CASE_WORKERS, - |(statement, padded, outcome, receipt, valid)| { + |(statement, outcome, receipt, valid)| { let mut body = if statement { json!({"branch":"main", "query_name":"branch merge", "affected_nodes":0, "affected_edges":0, "actor_id":"alice", @@ -3495,8 +3501,6 @@ fn remote_merge_requires_consistent_receipts_without_replay() { command.args(["--server", &server.origin, "--graph", "knowledge"]); if statement { command.args(["mutate", "-e", "branch merge review into main", "--json"]); - } else if padded { - command.args(["branch", "merge", " review ", "--into", " main ", "--json"]); } else { command.args(["branch", "merge", "review", "--json"]); } @@ -3579,9 +3583,9 @@ fn remote_merge_requires_consistent_receipts_without_replay() { "knowledge", "branch", "merge", - " review ", + "review", "--into", - " main ", + "main", "--delete-branch", "--json", ]) diff --git a/crates/omnigraph-cli/tests/integration/parity_matrix.rs b/crates/omnigraph-cli/tests/integration/parity_matrix.rs index 6adea2f65..f04ab0d6f 100644 --- a/crates/omnigraph-cli/tests/integration/parity_matrix.rs +++ b/crates/omnigraph-cli/tests/integration/parity_matrix.rs @@ -403,7 +403,33 @@ fn parity_branch_merge() { "--json", ]); assert_write_parity("merge source write", &l, &r); - let (l, r) = p.run(&["branch", "merge", " feature ", "--into", " main ", "--json"]); + // A name is used as written on both arms: a padded spelling of a branch + // is refused, never read as the branch, before any effect. + for args in [ + &["branch", "merge", " feature ", "--into", " main ", "--json"][..], + &["branch", "merge", "feature", "--into", "main ", "--json"][..], + &[ + "mutate", + "--branch", + " main", + "-e", + "query add() { insert Person { name: \"Padded\", age: 32 } }", + "--json", + ][..], + ] { + let (l, r) = p.run(args); + assert_parity("padded branch name", &l, &r); + for output in [&l, &r] { + // `--json` reports the error on stdout. + assert!( + !output.status.success() + && String::from_utf8_lossy(&output.stdout) + .contains("has leading or trailing whitespace"), + "{args:?}: {output:?}" + ); + } + } + let (l, r) = p.run(&["branch", "merge", "feature", "--into", "main", "--json"]); assert_write_parity("branch merge own publication", &l, &r); for output in [&l, &r] { let payload = parse_stdout_json(output); @@ -426,9 +452,9 @@ fn parity_branch_merge() { let (l, r) = p.run(&[ "branch", "merge", - " feature2 ", + "feature2", "--into", - " main ", + "main", "--delete-branch", "--json", ]); @@ -490,9 +516,10 @@ fn parity_branch_merge() { let (l, r) = p.run(&["branch", "merge", source, "--into", target]); for output in [&l, &r] { assert_eq!(output.status.code(), Some(1)); + let stderr = String::from_utf8_lossy(&output.stderr); assert!( - String::from_utf8_lossy(&output.stderr) - .contains("branch merge source and target must not be empty"), + stderr.contains("has leading or trailing whitespace") + || stderr.contains("contains a control character"), "{output:?}" ); } diff --git a/crates/omnigraph-policy/src/lib.rs b/crates/omnigraph-policy/src/lib.rs index 81b8218ea..bbd488825 100644 --- a/crates/omnigraph-policy/src/lib.rs +++ b/crates/omnigraph-policy/src/lib.rs @@ -291,6 +291,55 @@ pub struct PolicyRequest { pub target_branch: Option, } +/// A branch name refused by [`ensure_canonical_branch_name`]. +#[derive(Debug, Clone, Eq, PartialEq)] +pub enum BranchNameError { + Empty, + ControlCharacter(String), + SurroundingWhitespace(String), +} + +impl fmt::Display for BranchNameError { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + match self { + BranchNameError::Empty => write!(f, "branch name cannot be empty"), + BranchNameError::ControlCharacter(name) => { + write!(f, "branch name {name:?} contains a control character") + } + BranchNameError::SurroundingWhitespace(name) => { + write!(f, "branch name {name:?} has leading or trailing whitespace") + } + } + } +} + +impl std::error::Error for BranchNameError {} + +/// Accept a branch name only in a spelling no layer reads differently. +/// +/// A policy decision compares the request's branch names with +/// `protected_branches` as written, and the engine opens the branch it is +/// given, so the two decide about one branch only while nothing rewrites the +/// name between them. A name that a reader could take for another spelling +/// is refused rather than corrected: an empty name, one with leading or +/// trailing whitespace (the Unicode set `str::trim` removes), or one holding a +/// control character. Every surface that accepts a branch name applies this +/// rule before it authorizes or resolves the name. Lance's `check_valid_branch` +/// refuses whitespace and control characters in a ref name, so no branch +/// created through it is made unreachable by the rule. +pub fn ensure_canonical_branch_name(name: &str) -> Result<(), BranchNameError> { + if name.chars().any(char::is_control) { + return Err(BranchNameError::ControlCharacter(name.to_string())); + } + if name.trim() != name { + return Err(BranchNameError::SurroundingWhitespace(name.to_string())); + } + if name.is_empty() { + return Err(BranchNameError::Empty); + } + Ok(()) +} + #[derive(Debug, Clone)] pub struct PolicyDecision { pub allowed: bool, @@ -344,10 +393,11 @@ impl PolicyConfig { } } + // A request's branch is compared with these names as written, so an + // entry no request can spell would protect nothing. for branch in &self.protected_branches { - if branch.trim().is_empty() { - bail!("protected branch names must not be blank"); - } + ensure_canonical_branch_name(branch) + .map_err(|err| eyre!("protected_branches: {err}"))?; } let mut seen_rule_ids = HashSet::new(); @@ -563,6 +613,15 @@ impl PolicyEngine { /// supplying a `PolicyRequest` cannot smuggle identity through the /// same struct that carries the requested action. pub fn authorize(&self, actor_id: &str, request: &PolicyRequest) -> Result { + // The decision compares these names with `protected_branches` as + // written; one a caller could have meant as another spelling has no + // decision, whoever asks. + for branch in [&request.branch, &request.target_branch] + .into_iter() + .flatten() + { + ensure_canonical_branch_name(branch)?; + } if !self.known_actors.contains(actor_id) { return Ok(self.deny( None, @@ -1170,8 +1229,8 @@ rules: } } use super::{ - PolicyAction, PolicyCompiler, PolicyConfig, PolicyEngine, PolicyExpectation, PolicyRequest, - PolicyResourceKind, PolicyTestCase, PolicyTestConfig, + BranchNameError, PolicyAction, PolicyCompiler, PolicyConfig, PolicyEngine, + PolicyExpectation, PolicyRequest, PolicyResourceKind, PolicyTestCase, PolicyTestConfig, }; #[test] @@ -1309,6 +1368,48 @@ rules: .unwrap(); assert!(admin.allowed); assert_eq!(admin.matched_rule_id.as_deref(), Some("admins-promote")); + + // A spelling of protected main that is not main as written gets no + // decision, on either side of a transition, whoever asks. + for (actor, name) in [ + ("act-bruno", " main"), + ("act-bruno", "main "), + ("act-bruno", "main\u{3000}"), + ("act-bruno", "\tmain"), + ("act-bruno", ""), + ("act-unknown", " main"), + ] { + for (branch, target_branch) in [ + (Some(name.to_string()), None), + (Some("feature".to_string()), Some(name.to_string())), + ] { + let request = PolicyRequest { + action: PolicyAction::BranchMerge, + branch, + target_branch, + }; + let err = engine.authorize(actor, &request).unwrap_err(); + assert!(err.downcast_ref::().is_some(), "{err}"); + } + } + } + + #[test] + fn protected_branches_must_be_canonical() { + for entry in [r#"" main""#, r#""main ""#, r#""\tmain""#, r#""""#] { + let source = format!( + "version: 1\ngroups:\n team: [act-a]\nprotected_branches: [{entry}]\nrules: []\n" + ); + let err = PolicyConfig::from_source(&source).unwrap_err().to_string(); + assert!( + err.starts_with("protected_branches: branch name"), + "{entry}: {err}" + ); + } + PolicyConfig::from_source( + "version: 1\ngroups:\n team: [act-a]\nprotected_branches: [main, release/1.2]\nrules: []\n", + ) + .unwrap(); } #[test] diff --git a/crates/omnigraph-server/src/handlers.rs b/crates/omnigraph-server/src/handlers.rs index d76be84f3..cf70e97c0 100644 --- a/crates/omnigraph-server/src/handlers.rs +++ b/crates/omnigraph-server/src/handlers.rs @@ -602,8 +602,12 @@ pub(crate) enum Authz { } /// HTTP-layer Cedar policy gate, returning the allow/deny [`Authz`] decision -/// and reserving `Err` for operational failures (401 missing bearer, 500 -/// policy-evaluation error). Two sources of the policy engine: +/// and reserving `Err` for operational failures (400 non-canonical branch +/// name, 401 missing bearer, 500 policy-evaluation error). Every route names +/// its branches in the [`PolicyRequest`], so the request's names are refused +/// here, before any rule and in every runtime mode, when they are not +/// canonical: the decision judges a name as written and the engine opens that +/// same name. Two sources of the policy engine: /// * Per-graph handler — passes `handle.policy.as_deref()` so the /// graph's Cedar rules govern read/change/branch_*. /// * Management handler — captures the current registry snapshot policy so @@ -621,6 +625,7 @@ pub(crate) fn authorize( policy: Option<&PolicyEngine>, request: PolicyRequest, ) -> std::result::Result { + require_canonical_branches([request.branch.as_deref(), request.target_branch.as_deref()])?; if let Some(actor) = actor { if actor.source == AuthSource::SignedData && policy.is_none() { return Ok(Authz::Denied( @@ -697,9 +702,24 @@ pub(crate) fn authorize( } } +/// Refuse a request's branch names that are not canonical +/// ([`omnigraph_policy::ensure_canonical_branch_name`]) as a 400. [`authorize`] +/// applies it to every [`PolicyRequest`]; a route that looks a branch up before +/// it authorizes applies it first. +pub(crate) fn require_canonical_branches<'a>( + names: impl IntoIterator>, +) -> std::result::Result<(), ApiError> { + for name in names.into_iter().flatten() { + omnigraph_policy::ensure_canonical_branch_name(name) + .map_err(|error| ApiError::bad_request(error.to_string()))?; + } + Ok(()) +} + /// Thin wrapper over [`authorize`] for the handlers that treat any denial as a /// 403: a denial becomes `ApiError::forbidden`, and operational failures -/// (401 missing bearer, 500 policy-evaluation error) propagate unchanged. The +/// (400 non-canonical branch name, 401 missing bearer, 500 policy-evaluation +/// error) propagate unchanged. The /// stored-query invoke handler does **not** use this — it consumes the /// [`Authz`] decision directly to hide a denial as a 404 while letting an /// operational failure keep its true status. @@ -1403,7 +1423,7 @@ pub(crate) async fn server_export( ) -> std::result::Result { let Json(request) = request .map_err(|rejection| ApiError::json_rejection("invalid export request", rejection))?; - let branch = normalize_change_branch(request.branch.as_deref())?; + let branch = requested_branch(request.branch.as_deref())?; authorize_request( actor.as_ref().map(|Extension(actor)| actor), handle.policy.as_deref(), @@ -2339,6 +2359,7 @@ async fn authorize_load_scope( branch: &str, from: Option<&str>, ) -> std::result::Result<(), ApiError> { + require_canonical_branches([Some(branch), from])?; let branch_exists = handle .engine .branch_list() @@ -4008,10 +4029,26 @@ fn validate_change_http_limit(limit: Option) -> std::result::Result<(), A Ok(()) } -/// A side of a graph diff the caller must spell. +/// The branch a change route names, `main` when it names none, as written: a +/// name that is not canonical is refused before authorization reads it. +fn requested_branch(branch: Option<&str>) -> std::result::Result { + let branch = branch.unwrap_or("main"); + require_canonical_branches([Some(branch)])?; + Ok(branch.to_string()) +} + +/// A side of a graph diff the caller must spell, passed on as written. A +/// branch name and a commit id are never trimmed, so a spelling neither could +/// be is refused here (the change routes' error allowlist would hide the +/// engine's refusal behind a 500). fn required_diff_param(value: Option, name: &str) -> std::result::Result { match value { - Some(value) if !value.trim().is_empty() => Ok(value.trim().to_string()), + Some(value) if !value.trim().is_empty() => { + omnigraph_policy::ensure_canonical_branch_name(&value).map_err(|error| { + ApiError::bad_request(format!("query parameter '{name}': {error}")) + })?; + Ok(value) + } _ => Err(ApiError::bad_request(format!( "query parameter '{name}' is required" ))), @@ -4206,8 +4243,8 @@ pub(crate) async fn branch_diff_page( page_token: Option, max_changes: Option, ) -> std::result::Result { - let source = normalize_change_branch(Some(source))?; - let target = normalize_change_branch(target)?; + let source = requested_branch(Some(source))?; + let target = requested_branch(target)?; authorize_branch_reads(actor, handle.policy.as_deref(), [&source, &target])?; handle .engine @@ -4268,8 +4305,8 @@ pub(crate) async fn merge_base_body( source: &str, target: Option<&str>, ) -> std::result::Result<(String, String, omnigraph::changes::MergeBase), ApiError> { - let source = normalize_change_branch(Some(source))?; - let target = normalize_change_branch(target)?; + let source = requested_branch(Some(source))?; + let target = requested_branch(target)?; authorize_branch_reads(actor, handle.policy.as_deref(), [&source, &target])?; let base = handle .engine @@ -4395,20 +4432,6 @@ fn parse_change_feed_start( } } -/// Normalize a caller-supplied change-surface branch BEFORE authorization so -/// Cedar and the engine classify the same identity. The engine trims late -/// (its own branch normalization), so authorizing the raw string would let a -/// padded spelling like " main " be classified as an unprotected named branch -/// and then resolve to protected main — a policy bypass. Empty-after-trim is -/// a malformed request rather than an implicit main. -fn normalize_change_branch(branch: Option<&str>) -> std::result::Result { - let trimmed = branch.unwrap_or("main").trim(); - if trimmed.is_empty() { - return Err(ApiError::bad_request("branch name cannot be empty")); - } - Ok(trimmed.to_string()) -} - #[utoipa::path( get, path = "/changes", @@ -4443,7 +4466,7 @@ pub(crate) async fn server_changes_feed( ) -> std::result::Result, ApiError> { let params = parse_change_query(raw.as_deref(), CHANGE_FEED_PARAMS)?; validate_change_http_limit(params.limit)?; - let branch = normalize_change_branch(params.branch.as_deref())?; + let branch = requested_branch(params.branch.as_deref())?; authorize_request( actor.as_ref().map(|Extension(actor)| actor), handle.policy.as_deref(), @@ -4521,7 +4544,7 @@ pub(crate) async fn server_changes_baseline( actor: Option>, Json(request): Json, ) -> std::result::Result { - let branch = normalize_change_branch(request.branch.as_deref())?; + let branch = requested_branch(request.branch.as_deref())?; authorize_request( actor.as_ref().map(|Extension(actor)| actor), handle.policy.as_deref(), diff --git a/crates/omnigraph-server/tests/integration/auth_policy.rs b/crates/omnigraph-server/tests/integration/auth_policy.rs index 24d957baa..3d434d972 100644 --- a/crates/omnigraph-server/tests/integration/auth_policy.rs +++ b/crates/omnigraph-server/tests/integration/auth_policy.rs @@ -2896,30 +2896,39 @@ async fn change_routes_enforce_bearer_and_policy() { assert_eq!(response.status(), StatusCode::OK); } -/// A padded branch spelling must not bypass branch-scope policy: the engine -/// trims branch names late, so authorizing the caller's raw string would let -/// `branch=%20main%20` be classified by Cedar as an unprotected named branch -/// and then resolve to protected main. The feed and baseline handlers -/// normalize BEFORE authorization, so every spelling of main is judged as -/// main. +/// The policy judges a branch name as written and the engine acts on the +/// name it is given, so a spelling some layer could read as another branch +/// (surrounding whitespace) is refused as a bad request before any policy +/// decision, on every route that names a branch. An actor allowed to write +/// and control only unprotected branches therefore cannot reach protected +/// main through it, and protected main's head stays put. #[tokio::test(flavor = "multi_thread")] -async fn padded_branch_spelling_cannot_bypass_change_route_policy() { +async fn non_canonical_branch_names_are_refused_before_policy() { let temp = init_loaded_graph().await; let graph = graph_path(temp.path()); + session(Omnigraph::open(graph.to_str().unwrap()).await.unwrap()) + .branch_create("feature") + .await + .unwrap(); let policy_path = temp.path().join("policy.yaml"); fs::write( &policy_path, r#" version: 1 groups: - side-readers: [act-a] + side-writers: [act-a] protected_branches: [main] rules: - - id: unprotected-only + - id: unprotected-data allow: - actors: { group: side-readers } - actions: [read, export] + actors: { group: side-writers } + actions: [read, export, change] branch_scope: unprotected + - id: unprotected-branch-control + allow: + actors: { group: side-writers } + actions: [branch_create, branch_delete, branch_merge] + target_branch_scope: unprotected "#, ) .unwrap(); @@ -2931,58 +2940,158 @@ rules: .await .unwrap(); let app = build_app(state); - - // Plain main is protected: the unprotected-only rule denies it. - let (plain_status, _) = json_response( - &app, + let post = |path: &str, body: Value| { Request::builder() .header(HTTP_API_CONTRACT_HEADER, HTTP_API_CONTRACT) - .uri(g("/changes?branch=main")) - .method(Method::GET) + .uri(g(path)) + .method(Method::POST) .header("authorization", "Bearer token-a") - .body(Body::empty()) - .unwrap(), - ) - .await; - assert_eq!(plain_status, StatusCode::FORBIDDEN); - - // The padded spelling resolves to the same protected main and must be - // denied identically — before normalization it was classified as an - // unprotected named branch and slipped through. - let (padded_status, _) = json_response( - &app, + .header("content-type", "application/json") + .body(Body::from(body.to_string())) + .unwrap() + }; + let get = |path: &str| { Request::builder() .header(HTTP_API_CONTRACT_HEADER, HTTP_API_CONTRACT) - .uri(g("/changes?branch=%20main%20")) + .uri(g(path)) .method(Method::GET) .header("authorization", "Bearer token-a") .body(Body::empty()) - .unwrap(), + .unwrap() + }; + let mutate = |branch: &str| { + json!({ + "query": MUTATION_QUERIES, + "name": "insert_person", + "params": { "name": "Mina", "age": 28 }, + "branch": branch, + }) + }; + let person = r#"{"type": "Person", "data": {"name": "Zed", "age": 40}}"#; + let main_head = || async { + session(Omnigraph::open(graph.to_str().unwrap()).await.unwrap()) + .list_commits(None) + .await + .unwrap()[0] + .graph_commit_id + .clone() + }; + + // The canonical spellings are judged as the policy says: feature takes the + // actor's write, which also gives the merges below something to publish, + // and protected main refuses it. + let (status, body) = json_response(&app, post("/mutate", mutate("feature"))).await; + assert_eq!(status, StatusCode::OK, "{body}"); + let (status, body) = json_response(&app, post("/mutate", mutate("main"))).await; + assert_forbidden(status, body, "a write to protected main"); + let (status, body) = json_response( + &app, + post( + "/branches/merge", + json!({"source": "feature", "target": "main"}), + ), ) .await; - assert_eq!( - padded_status, - StatusCode::FORBIDDEN, - "a padded spelling of main must be authorized as main" - ); + assert_forbidden(status, body, "a merge into protected main"); + let (status, body) = json_response(&app, get("/changes?branch=main")).await; + assert_forbidden(status, body, "the change feed of protected main"); + let before = main_head().await; - // Baseline (export) takes its branch from the JSON body; the same padded - // spelling must be denied the same way. - let (baseline_status, _) = json_response( + for (main, encoded) in [ + (" main", "%20main"), + ("main ", "main%20"), + (" main ", "%20main%20"), + ("main\t", "main%09"), + ] { + let requests = [ + ("POST /mutate", post("/mutate", mutate(main))), + ( + "POST /load", + post("/load", json!({"branch": main, "data": person})), + ), + ( + "POST /load from", + post( + "/load", + json!({"branch": "fresh", "from": main, "data": person}), + ), + ), + ( + "POST /branches/merge", + post( + "/branches/merge", + json!({"source": "feature", "target": main}), + ), + ), + ( + "POST /branches", + post("/branches", json!({"from": main, "name": "fresh"})), + ), + ( + "POST /query", + post( + "/query", + json!({ + "query": "query people() { match { $p: Person } return { $p.name } }", + "branch": main, + }), + ), + ), + ("GET /snapshot", get(&format!("/snapshot?branch={encoded}"))), + ("GET /commits", get(&format!("/commits?branch={encoded}"))), + ("GET /changes", get(&format!("/changes?branch={encoded}"))), + ( + "GET /diff", + get(&format!("/diff?from=feature&to={encoded}")), + ), + ( + "GET /branches/diff", + get(&format!("/branches/diff?source=feature&target={encoded}")), + ), + ( + "GET /branches/merge-base", + get(&format!( + "/branches/merge-base?source=feature&target={encoded}" + )), + ), + ( + "POST /branches/merge dry run", + post( + "/branches/merge", + json!({"source": "feature", "target": main, "dry_run": true}), + ), + ), + ( + "POST /changes/baseline", + post("/changes/baseline", json!({"branch": main})), + ), + ]; + for (route, request) in requests { + let (status, body) = json_response(&app, request).await; + assert_eq!( + main_head().await, + before, + "{route} with branch {main:?} reached protected main: {status} {body}" + ); + assert_eq!( + status, + StatusCode::BAD_REQUEST, + "{route} with branch {main:?}: {body}" + ); + let error: ErrorOutput = serde_json::from_value(body).unwrap(); + assert_eq!( + error.code, + Some(omnigraph_server::api::ErrorCode::BadRequest), + "{route} with branch {main:?}" + ); + } + } + // A branch statement spells the same names and gets the same refusal. + let (status, body) = json_response( &app, - Request::builder() - .header(HTTP_API_CONTRACT_HEADER, HTTP_API_CONTRACT) - .uri(g("/changes/baseline")) - .method(Method::POST) - .header("authorization", "Bearer token-a") - .header("content-type", "application/json") - .body(Body::from(r#"{"branch":" main "}"#)) - .unwrap(), + statement_request("/mutate", "token-a", r#"branch merge feature into " main""#), ) .await; - assert_eq!( - baseline_status, - StatusCode::FORBIDDEN, - "a padded baseline branch must be authorized as main" - ); + assert_eq!(status, StatusCode::BAD_REQUEST, "{body}"); + assert_eq!(main_head().await, before); } diff --git a/crates/omnigraph-server/tests/integration/stored_queries.rs b/crates/omnigraph-server/tests/integration/stored_queries.rs index 8150ab0bb..f0589a601 100644 --- a/crates/omnigraph-server/tests/integration/stored_queries.rs +++ b/crates/omnigraph-server/tests/integration/stored_queries.rs @@ -762,6 +762,20 @@ async fn invoke_stored_query_bad_param_is_400() { "body: {body}" ); } + + // The invocation's branch is used as written: a padded spelling of main + // is refused before the inner read gate, not read as main. + let (status, body) = json_response( + &app, + invoke_request( + "find_person", + "t-invoke", + json!({ "params": { "name": "Alice" }, "branch": "main " }), + ), + ) + .await; + assert_eq!(status, StatusCode::BAD_REQUEST, "body: {body}"); + assert_eq!(body["code"], "bad_request", "body: {body}"); } #[tokio::test(flavor = "multi_thread")] diff --git a/crates/omnigraph/src/db/graph_coordinator.rs b/crates/omnigraph/src/db/graph_coordinator.rs index 17a7cbbe9..a50377dec 100644 --- a/crates/omnigraph/src/db/graph_coordinator.rs +++ b/crates/omnigraph/src/db/graph_coordinator.rs @@ -18,6 +18,7 @@ use super::manifest::{ PublishPrecondition, SchemaContractRow, UnattachedCommit, UnattachedIntent, UnattachedWriteBase, UnpublishedCommit, UnpublishedMergeInputs, }; +use super::omnigraph::normalize_branch_name; use super::snapshot::Snapshot; use crate::seams::{decide_seam, fail}; @@ -1006,20 +1007,6 @@ fn updates_to_changes(updates: &[DatasetUpdate]) -> Vec { .collect() } -fn normalize_branch_name(branch: &str) -> Result> { - let branch = branch.trim(); - if branch.is_empty() { - return Err(OmniError::manifest( - "branch name cannot be empty".to_string(), - )); - } - if branch == "main" { - return Ok(None); - } - crate::branch_names::ensure_logical_branch_name(branch)?; - Ok(Some(branch.to_string())) -} - #[cfg(test)] mod tests { use super::*; diff --git a/crates/omnigraph/src/db/omnigraph.rs b/crates/omnigraph/src/db/omnigraph.rs index 1b8655ba7..daaa6e84a 100644 --- a/crates/omnigraph/src/db/omnigraph.rs +++ b/crates/omnigraph/src/db/omnigraph.rs @@ -1025,12 +1025,20 @@ impl Omnigraph { /// * If policy is installed AND actor is Some → call /// `PolicyChecker::check(action, scope, actor)`; map denial / /// internal failure to `OmniError::Policy(...)`. + /// + /// Before any of these, a branch name in `scope` that is not canonical is + /// refused as a bad request: the decision judges the name as written, and + /// the writer then opens that same name. pub(crate) fn enforce( &self, action: omnigraph_policy::PolicyAction, scope: &omnigraph_policy::ResourceScope, actor: Option<&str>, ) -> Result<()> { + let (branch, target_branch) = scope.to_branch_pair(); + for name in [branch, target_branch].into_iter().flatten() { + ensure_canonical_branch_name(name)?; + } let Some(checker) = self.policy.as_ref() else { return Ok(()); }; @@ -2959,7 +2967,7 @@ impl Omnigraph { match self.classify_diff_read_target(target).await? { DiffReadTarget::Branch(resolved) => { let commit_id = resolved.graph_commit_id.ok_or_else(|| { - OmniError::manifest(format!("branch '{}' has no head commit", target.trim())) + OmniError::manifest(format!("branch '{target}' has no head commit")) })?; Ok((commit_id, resolved.snapshot)) } @@ -2975,10 +2983,12 @@ impl Omnigraph { /// one naming neither is a typed not-found carrying only the caller's /// spelling. async fn classify_diff_read_target(&self, target: &str) -> Result { - let target = target.trim(); if target.is_empty() { return Err(OmniError::manifest("a diff endpoint cannot be empty")); } + // A branch name and a commit id are both taken as written, so a + // spelling neither could be is refused rather than trimmed. + ensure_canonical_branch_name(target)?; // A string the branch-name rules refuse cannot name a branch; that is // not an error here, since it may still name a commit. let as_branch = match normalize_branch_name(target) { @@ -3974,13 +3984,11 @@ impl Omnigraph { } } +/// A caller's branch name as the engine addresses it: `None` for main, the +/// name itself otherwise. The name is taken as written, the spelling a policy +/// decision judged; one that is not canonical is refused, never corrected. pub(crate) fn normalize_branch_name(branch: &str) -> Result> { - let branch = branch.trim(); - if branch.is_empty() { - return Err(OmniError::manifest( - "branch name cannot be empty".to_string(), - )); - } + ensure_canonical_branch_name(branch)?; if branch == "main" { return Ok(None); } @@ -3988,6 +3996,13 @@ pub(crate) fn normalize_branch_name(branch: &str) -> Result> { Ok(Some(branch.to_string())) } +/// Refuse a branch name that is not canonical as a bad request +/// ([`omnigraph_policy::ensure_canonical_branch_name`]). +pub(crate) fn ensure_canonical_branch_name(branch: &str) -> Result<()> { + omnigraph_policy::ensure_canonical_branch_name(branch) + .map_err(|error| OmniError::manifest(error.to_string())) +} + /// Build a `ResolvedTarget` from the warm coordinator without opening the commit /// graph. The live branch snapshot is pinned by the manifest incarnation, so the /// cache id is synthetic `(branch, version, e_tag when available)`. The effective diff --git a/crates/omnigraph/tests/integration/maintenance.rs b/crates/omnigraph/tests/integration/maintenance.rs index 89fb29070..d39e45ac0 100644 --- a/crates/omnigraph/tests/integration/maintenance.rs +++ b/crates/omnigraph/tests/integration/maintenance.rs @@ -2035,7 +2035,7 @@ async fn full_text_rebuild_reports_empty_builds_but_not_no_work() { .await .unwrap(); let before = snapshot_main(&db).await.unwrap(); - let result = db.rebuild_full_text_indices_on(" main ").await.unwrap(); + let result = db.rebuild_full_text_indices_on("main").await.unwrap(); assert_eq!(result.branch, "main"); assert_eq!(result.graph_commit_id, None); assert!(result.rebuilt_indexes.is_empty()); diff --git a/crates/omnigraph/tests/integration/warm_read_cost.rs b/crates/omnigraph/tests/integration/warm_read_cost.rs index 9c3bcf8ac..f583fc18b 100644 --- a/crates/omnigraph/tests/integration/warm_read_cost.rs +++ b/crates/omnigraph/tests/integration/warm_read_cost.rs @@ -95,15 +95,20 @@ async fn warm_same_branch_read_does_no_resolution_opens() { io.version_probes, 1, "warm same-branch read performs exactly one version probe" ); - for branch in ["main", " main "] { + let (resolved, io) = measure(db.resolve_snapshot("main")).await; + let resolved = resolved.unwrap(); + assert_eq!(Some(resolved.as_str()), head.as_deref()); + assert_eq!(io.internal_open_count, 0); + assert_eq!(io.manifest_scan_count, 0); + assert_eq!(io.version_probes, 1); + // A name that is not canonical is refused before any storage request. + for branch in [" main ", " "] { let (resolved, io) = measure(db.resolve_snapshot(branch)).await; - let resolved = resolved.unwrap(); - assert_eq!(Some(resolved.as_str()), head.as_deref()); + assert!(resolved.is_err(), "{branch:?}"); assert_eq!(io.internal_open_count, 0); assert_eq!(io.manifest_scan_count, 0); - assert_eq!(io.version_probes, 1); + assert_eq!(io.version_probes, 0); } - assert!(db.resolve_snapshot(" ").await.is_err()); }) .await; } diff --git a/crates/omnigraph/tests/policy_engine_chassis.rs b/crates/omnigraph/tests/policy_engine_chassis.rs index 20d271692..b17f826d9 100644 --- a/crates/omnigraph/tests/policy_engine_chassis.rs +++ b/crates/omnigraph/tests/policy_engine_chassis.rs @@ -24,7 +24,7 @@ use std::sync::Arc; use omnigraph::Session; use omnigraph::db::{Omnigraph, ReadTarget}; -use omnigraph::error::OmniError; +use omnigraph::error::{ManifestErrorKind, OmniError}; use omnigraph::loader::LoadMode; use omnigraph_policy::{PolicyChecker, PolicyEngine}; @@ -412,6 +412,16 @@ async fn blob_put_and_clear_enforce_change_for_the_actor() { db.clear_blob_at_as("main", cell(), None, Some("act-allowed")) .await .expect("act-allowed may Change on main"); + assert_non_canonical_branch( + db.put_blob_at_as(" main", cell(), bytes(), None, Some("act-allowed")) + .await, + "put_blob_at_as on a padded branch name", + ); + assert_non_canonical_branch( + db.clear_blob_at_as("main ", cell(), None, Some("act-allowed")) + .await, + "clear_blob_at_as on a padded branch name", + ); } #[tokio::test] @@ -607,6 +617,156 @@ async fn branch_merge_as_allows_when_policy_permits_actor() { .expect("act-allowed should be able to BranchMerge"); } +fn assert_non_canonical_branch(result: Result, what: &str) { + match result { + Err(OmniError::Manifest(error)) + if error.kind == ManifestErrorKind::BadRequest + && (error.message.contains("leading or trailing whitespace") + || error.message.contains("contains a control character")) => {} + other => panic!("{what}: expected a non-canonical branch name refusal, got {other:?}"), + } +} + +/// The head commit of each protected branch and the live branch names: what a +/// refused writer must leave as it found them. +async fn protected_state(db: &Session) -> (String, String, Vec) { + let head = |commits: Vec| commits[0].graph_commit_id.clone(); + let mut branches = db.branch_list().await.unwrap(); + branches.sort(); + ( + head(db.list_commits(None).await.unwrap()), + head(db.list_commits(Some("release")).await.unwrap()), + branches, + ) +} + +/// The policy judges a branch name as written, and the engine acts on the +/// name it is given. An actor allowed to write and control only unprotected +/// branches therefore cannot reach a protected one through a spelling the +/// engine could read as it: every writer refuses the spelling as a typed bad +/// request before policy or effects, and both protected heads stay put. +#[tokio::test] +async fn non_canonical_branch_names_cannot_reach_a_protected_branch() { + let dir = tempfile::tempdir().unwrap(); + let uri = dir.path().to_str().unwrap().to_string(); + { + let db = init_and_load(&dir).await; + db.branch_create("feature").await.unwrap(); + db.branch_create("release").await.unwrap(); + let params = mixed_params(&[("$name", "Feature")], &[("$age", 30)]); + db.mutate("feature", MUTATION_QUERIES, "insert_person", ¶ms) + .await + .unwrap(); + } + let policy = POLICY_YAML + .replace( + "protected_branches: [main]", + "protected_branches: [main, release]", + ) + .replace(" branch_scope: any", " branch_scope: unprotected") + .replace( + " target_branch_scope: any", + " target_branch_scope: unprotected", + ); + let (db, _engine) = + install_policy_source(Omnigraph::open(&uri).await.unwrap(), dir.path(), &policy); + let actor = Some("act-allowed"); + let params = mixed_params(&[("$name", "Eve")], &[("$age", 22)]); + // The canonical spellings are judged as the policy says. + assert_denied( + db.mutate_as("main", MUTATION_QUERIES, "insert_person", ¶ms, actor) + .await, + "mutate_as on protected main", + ); + assert_denied( + db.branch_merge_as("feature", "main", actor).await, + "branch_merge_as into protected main", + ); + assert_denied( + db.branch_delete_as("release", actor).await, + "branch_delete_as of protected release", + ); + let before = protected_state(&db).await; + + let check = + |what: String, result: Result<(), OmniError>, after: (String, String, Vec)| { + assert_eq!(after, before, "{what} reached a protected branch"); + assert_non_canonical_branch(result, &what); + }; + for main in [" main", "main ", " main ", "\tmain", "main\u{3000}"] { + let result = db + .mutate_as(main, MUTATION_QUERIES, "insert_person", ¶ms, actor) + .await + .map(drop); + check( + format!("mutate_as({main:?})"), + result, + protected_state(&db).await, + ); + let result = db + .load_as(main, None, ONE_PERSON_JSONL, LoadMode::Merge, actor) + .await + .map(drop); + check( + format!("load_as({main:?})"), + result, + protected_state(&db).await, + ); + let result = db.branch_merge_as("feature", main, actor).await.map(drop); + check( + format!("branch_merge_as(feature -> {main:?})"), + result, + protected_state(&db).await, + ); + let result = db + .rebuild_full_text_indices_on_as(main, actor) + .await + .map(drop); + check( + format!("rebuild_full_text_indices_on_as({main:?})"), + result, + protected_state(&db).await, + ); + let result = db + .load_as( + "fresh", + Some(main), + ONE_PERSON_JSONL, + LoadMode::Merge, + actor, + ) + .await + .map(drop); + check( + format!("load_as(fresh from {main:?})"), + result, + protected_state(&db).await, + ); + let result = db + .branch_create_from_as(ReadTarget::branch(main), "fresh", actor) + .await; + check( + format!("branch_create_from_as({main:?} -> fresh)"), + result, + protected_state(&db).await, + ); + } + for release in [" release", "release "] { + let result = db.branch_delete_as(release, actor).await; + check( + format!("branch_delete_as({release:?})"), + result, + protected_state(&db).await, + ); + let result = db.branch_create_as(release, actor).await; + check( + format!("branch_create_as({release:?})"), + result, + protected_state(&db).await, + ); + } +} + #[tokio::test] async fn full_text_rebuild_enforces_selected_branch_before_effects_and_records_actor() { fn physical_tree( diff --git a/docs/rfcs/0055-gq-branch-statements.md b/docs/rfcs/0055-gq-branch-statements.md index 15d9a03ab..def5eb756 100644 --- a/docs/rfcs/0055-gq-branch-statements.md +++ b/docs/rfcs/0055-gq-branch-statements.md @@ -177,9 +177,9 @@ the target's head and the source are unchanged. Guarantee: a statement and its route produce the same engine effect, the same Cedar decision, the same admission check, and the same error mapping for every input the compiler accepts, because they run one handler body -(Design); a padded or control-character name is refused at the statement -and trimmed, or passed, by the route (Grammar), the one input class where -the fronts differ. They differ +(Design); a padded or control-character name is refused at both fronts, +by the compiler at the statement and by the route before authorization +(Grammar). They differ only in envelope; a merge conflict is the same 409 on both fronts. Guarantee: `branch list` is authorized as `GET /branches` is, `read` with @@ -328,7 +328,8 @@ empty name, a name with leading or trailing whitespace, and a name carrying a control character (Unicode category Cc, `char::is_control`), so that the spelled name and the name the engine acts on are one string; every other -rule is the engine's (`normalize_branch_name` trims and refuses empty, +rule is the engine's (`normalize_branch_name` refuses, never trims, the same +empty, padded and control-character names, `ensure_logical_branch_name` refuses an incarnation-shaped segment, and `ensure_branch_create_namespace_safe` refuses an existing name and an ancestor or descendant of a live name, `crates/omnigraph/src/db/omnigraph.rs:3787-3799, @@ -713,7 +714,8 @@ at that commit). `takes no arguments` text as its prefix, and `branch:` with no name after it with `` `--- {kind} branch:` needs a branch name ``. The name is not checked further at parse time: it passes to the engine, whose - `normalize_branch_name` trims and refuses empty (Grammar). + `normalize_branch_name` refuses an empty, padded or control-character + name (Grammar). 2. A `--- mutate` step may hold a control write and a `--- query` step may hold `branch list`, classified by the compiler. The wrong kind is refused beside the existing read/mutation refusals (`lib.rs:803-812`) @@ -1499,3 +1501,10 @@ None. beside `branch list` and are refused at `POST /mutate` as reads. This supersedes "`branch list` is the only branch statement `POST /query` accepts" in User and operational behavior; the read door stays read-only. +- 2026-10-13: a route and the engine refuse an empty, padded or + control-character branch name as written instead of trimming it, so the + statement and its route refuse the same names. This supersedes, in Design, + "a padded or control-character name is refused at the statement and + trimmed, or passed, by the route (Grammar), the one input class where the + fronts differ", and in Grammar and Logic tests, "`normalize_branch_name` + trims and refuses empty". diff --git a/docs/rfcs/2026-09-30-exact-merge-receipts.md b/docs/rfcs/2026-09-30-exact-merge-receipts.md index b46704e88..0c01887b1 100644 --- a/docs/rfcs/2026-09-30-exact-merge-receipts.md +++ b/docs/rfcs/2026-09-30-exact-merge-receipts.md @@ -7,7 +7,7 @@ implementation: complete authors: - OmniGraph maintainers created: 2026-09-30 -updated: 2026-10-07 +updated: 2026-10-13 discussion: null supersedes: [] superseded_by: [] @@ -40,9 +40,10 @@ Nullable metadata retains the publication's value. Fast-forward publishes its own target commit with the source head as merged parent; it does not return the source commit. `already_up_to_date` explicitly returns `commit: null`. -The dedicated CLI trims source and target names before opening storage or -submitting HTTP work, matching the engine's canonical names. Empty names refuse -before dispatch. Receipt validation and optional deletion use those same names. +The dedicated CLI refuses a source or target name that is empty, padded or +holds a control character before opening storage or submitting HTTP work, as +the engine and the server refuse it; no name is trimmed. Receipt validation and +optional deletion use the names as given. Optional source deletion remains a subsequent, separately authorized effect. When requested, `branch_deleted` is `true` on success or `false` on failure. @@ -114,3 +115,9 @@ executed gates before marking implementation complete. while hardening the reviewed stack. - 2026-10-07: Replaced the Compatibility sentence tying receipts to v0.12 with the current HTTP admission decision; receipt semantics are unchanged. + +- 2026-10-13: Branch names are refused, not trimmed, at the CLI as at the + engine and the server. This supersedes "The dedicated CLI trims source and + target names before opening storage or submitting HTTP work, matching the + engine's canonical names. Empty names refuse before dispatch. Receipt + validation and optional deletion use those same names." diff --git a/docs/user/branching/index.md b/docs/user/branching/index.md index dad7cc783..b9bc9c240 100644 --- a/docs/user/branching/index.md +++ b/docs/user/branching/index.md @@ -68,6 +68,8 @@ A branch remains after a normal merge, so prefer `--delete-branch` or delete it when review is complete. Live branches retain the history they depend on and can prevent `omnigraph cleanup` from reclaiming old data. +## Branch names + Branch names may contain `/`, but live names must not be path prefixes of one another. For example, `review` and `review/alice` cannot coexist. `main` is reserved. A parent branch can be deleted while descendants remain. A path segment @@ -75,6 +77,15 @@ may not end in `.` followed by 26 upper-case letters and digits: that shape is reserved for OmniGraph's internal per-life branch identity. Ordinary dotted names such as `release.1.2` are fine. +A branch name is used exactly as written. A name that is empty, has leading or +trailing whitespace, or contains a control character is refused wherever a +branch is named: the branch a read or write targets, `from`, merge sources and +targets, branch create and delete, Blob writes, change feeds and branch +statements. The server answers `400 Bad Request` before authorization, the +embedded engine returns a bad-request error before any effect, and the CLI +reports either error. Names are never trimmed, so `main ` does not name +`main`. + Deleting a branch and creating another with the same name yields a fresh branch lifetime. Readers that captured the deleted branch fail with a typed error rather than seeing the replacement. Deletion retires the logical name diff --git a/docs/user/operations/policy.md b/docs/user/operations/policy.md index 0383cff74..2a5d58c56 100644 --- a/docs/user/operations/policy.md +++ b/docs/user/operations/policy.md @@ -107,7 +107,18 @@ cluster APIs keep their explicit trust boundary. Graph rules may use `branch_scope` for a source branch or `target_branch_scope` for a destination branch. Values are `any`, `protected`, -or `unprotected`; a rule may not set both. Server actions and graph-wide +or `unprotected`; a rule may not set both. A branch is protected when the +bundle's `protected_branches` list names it exactly: + +```yaml +protected_branches: [main, release] +``` + +Branch names are compared as written. A `protected_branches` entry that is +empty, has leading or trailing whitespace, or contains a control character +fails policy validation, and a request naming such a branch is refused as a +bad request before any rule is evaluated (see +[branch names](../branching/index.md#branch-names)). Server actions and graph-wide `invoke_query` rules do not take branch scopes. `commit_publish` and `commit_drop` take `target_branch_scope`: the scope is the branch the unpublished commit recorded as its target. Writing an unpublished commit