fix(policy)!: refuse branch names that are not canonical

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

crates/omnigraph-cli/src/client.rs

@@ -1790,12 +1790,11 @@ impl GraphClient {
         dry_run: bool,
         settings: &[(SettingId, SettingValue)],
     ) -> Result<BranchMergeOutput> {
-        // 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 {

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",
                 ])

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:?}"
             );
         }

crates/omnigraph-policy/src/lib.rs

@@ -291,6 +291,55 @@ pub struct PolicyRequest {
     pub target_branch: Option<String>,
 }
 
+/// 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<PolicyDecision> {
+        // 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::<BranchNameError>().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]