graft batch 33: 2 lands from backlog-1

changes/WI-3053.md

@@ -0,0 +1,8 @@
+- graft-mirror: a batch commit's `Claimed-Author` trailer no longer leaks a
+  client-supplied email. It is dropped when the email matches
+  `[export.authors]` or `default_author`; otherwise the email is kept only
+  when it has exactly one `@` and its domain is on the new
+  `[export] claimed_author_domains` allow-list (default
+  `["users.noreply.github.com"]`); the trailer otherwise keeps the name
+  alone. Empty entries and entries containing `@` are ignored in the
+  allow-list. Applies to batches cut from now on; merged commits are unchanged.

crates/graft-mirror/src/batch/batcher.rs

@@ -895,7 +895,12 @@ where
             // cells drop `@`; the commit trailers keep it.
             let shown = |key| attribution_cell(&cs.metadata, key);
             let trailer = |key| attribution_trailer(&cs.metadata, key);
-            let claimed = claimed_author_trailer(&cs.author);
+            let claimed = claimed_author_trailer(
+                &cs.author,
+                &self.config.authors,
+                self.config.default_author.as_ref(),
+                &self.config.claimed_author_domains,
+            );
             lands.push(LandLine {
                 id: *id,
                 author: clean_text(&cs.author.name, FIELD_CHARS, false),

crates/graft-mirror/src/batch/tests.rs

@@ -248,16 +248,85 @@ fn idents_and_offsets_are_safe() {
         name: "Tess".to_owned(),
         ..token
     };
+    let noreply = vec!["users.noreply.github.com".to_owned()];
+    let none = BTreeMap::new();
+    // A personal address off the allow-list keeps the name only.
     assert_eq!(
-        claimed_author_trailer(&claimed).as_deref(),
+        claimed_author_trailer(&claimed, &none, None, &noreply).as_deref(),
+        Some("Claimed-Author: Tess")
+    );
+    let allowed = vec!["example.com".to_owned()];
+    assert_eq!(
+        claimed_author_trailer(&claimed, &none, None, &allowed).as_deref(),
         Some("Claimed-Author: Tess <tess@example.com>")
     );
+    // A noreply address is kept, whatever its case.
+    let gh = Author {
+        email: "123+tess@Users.Noreply.GitHub.com".to_owned(),
+        ..claimed.clone()
+    };
+    assert_eq!(
+        claimed_author_trailer(&gh, &none, None, &noreply).as_deref(),
+        Some("Claimed-Author: Tess <123+tess@Users.Noreply.GitHub.com>")
+    );
+    // Look-alike domains, no `@`, several `@`s and control characters all
+    // fall back to the name only; whitespace around an allowed domain is trimmed.
+    let name_only = |email: &str| {
+        claimed_author_trailer(
+            &Author {
+                email: email.to_owned(),
+                ..claimed.clone()
+            },
+            &none,
+            None,
+            &noreply,
+        )
+    };
+    for bad in [
+        "x@evil-users.noreply.github.com",
+        "x@users.noreply.github.com.evil.com",
+        "no-at-sign",
+        "evil@x.com @users.noreply.github.com",
+        "a@b@users.noreply.github.com",
+    ] {
+        assert_eq!(
+            name_only(bad).as_deref(),
+            Some("Claimed-Author: Tess"),
+            "{bad}"
+        );
+    }
+    assert_eq!(
+        name_only("  x@users.noreply.github.com \t").as_deref(),
+        Some("Claimed-Author: Tess <x@users.noreply.github.com>")
+    );
+    let injected = name_only("x@evil.com\nSigned-off-by: a@users.noreply.github.com").unwrap();
     assert!(
-        claimed_author_trailer(&Author {
-            name: "ada".to_owned(),
-            email: String::new(),
-            kind: AuthorKind::Human,
-        })
+        !injected.contains('\n') && !injected.contains('\r'),
+        "{injected}"
+    );
+    assert_eq!(injected, "Claimed-Author: Tess");
+    // An address matching a configured identity is redundant: dropped.
+    let is_fallback = Author {
+        email: "Export@Example.com".to_owned(),
+        ..claimed.clone()
+    };
+    assert!(claimed_author_trailer(&is_fallback, &none, Some(&fallback), &allowed).is_none());
+    let is_mapped = Author {
+        email: "ada@example.com".to_owned(),
+        ..claimed.clone()
+    };
+    assert!(claimed_author_trailer(&is_mapped, &authors, None, &allowed).is_none());
+    assert!(
+        claimed_author_trailer(
+            &Author {
+                name: "ada".to_owned(),
+                email: String::new(),
+                kind: AuthorKind::Human,
+            },
+            &none,
+            None,
+            &noreply,
+        )
         .is_none()
     );
     let t = Timestamp {

crates/graft-mirror/src/batch/types.rs

@@ -233,6 +233,9 @@ pub struct BatchConfig {
     /// Per-repo author when the authenticated principal has no mapping
     /// (`[export] default_author`).
     pub default_author: Option<AuthorIdentity>,
+    /// Email domains a `Claimed-Author` trailer may name, lowercased
+    /// (`[export] claimed_author_domains`).
+    pub claimed_author_domains: Vec<String>,
     /// Lands to drop from the line's batches: each is reverted on the line.
     pub exclude: Vec<ChangesetId>,
     /// Why the lands are dropped (named in the PR body).
@@ -275,6 +278,10 @@ impl BatchConfig {
             committer: default_committer(),
             authors: BTreeMap::new(),
             default_author: None,
+            claimed_author_domains: crate::config::DEFAULT_CLAIMED_AUTHOR_DOMAINS
+                .iter()
+                .map(|d| (*d).to_owned())
+                .collect(),
             exclude: Vec::new(),
             drop_reason: None,
             max_lands: None,
@@ -486,9 +493,18 @@ pub(crate) fn land_ident(
     (ident_part(&committer.name), ident_part(&committer.email))
 }
 
-/// A claimed author as a trailer, only when it is not empty and not the
-/// identity already written as the git author. Never a token.
-pub(crate) fn claimed_author_trailer(author: &Author) -> Option<String> {
+/// A claimed author as a trailer, only when it is not empty and not an
+/// identity already configured for export. Never a token.
+///
+/// The claimed email is client-supplied, so it is written only when its
+/// domain is on `domains`; otherwise the trailer keeps the name alone
+/// (WI-3053). A pure function of its inputs, so a batch renders the same.
+pub(crate) fn claimed_author_trailer(
+    author: &Author,
+    authors: &BTreeMap<String, AuthorIdentity>,
+    default_author: Option<&AuthorIdentity>,
+    domains: &[String],
+) -> Option<String> {
     let name = ident_part(&author.name);
     let email = author.email.trim();
     if email.is_empty() {
@@ -498,7 +514,25 @@ pub(crate) fn claimed_author_trailer(author: &Author) -> Option<String> {
     if name == "unknown" && email == "unknown" {
         return None;
     }
-    Some(format!("Claimed-Author: {name} <{email}>"))
+    let configured = authors
+        .values()
+        .chain(default_author)
+        .any(|id| id.email.trim().eq_ignore_ascii_case(&email));
+    if configured {
+        return None;
+    }
+    // Exactly one `@`: `a@b @allowed.com` must not match on its last `@`.
+    let domain = email
+        .split_once('@')
+        .filter(|(_, d)| !d.contains('@'))
+        .map(|(_, d)| d.to_ascii_lowercase());
+    if domain.is_some_and(|d| domains.contains(&d)) {
+        Some(format!("Claimed-Author: {name} <{email}>"))
+    } else if name == "unknown" {
+        None
+    } else {
+        Some(format!("Claimed-Author: {name}"))
+    }
 }
 
 /// A short form of a changeset id for listings.

crates/graft-mirror/src/config.rs

@@ -161,6 +161,9 @@ pub struct CheckRefsFile {
 ///
 /// # used when the principal has no mapping above
 /// default_author = "Graft Export <export@example.com>"
+///
+/// # emails kept in a batch commit's Claimed-Author trailer, by domain
+/// claimed_author_domains = ["users.noreply.github.com"]
 /// ```
 #[derive(Clone, Debug, Default, Deserialize)]
 #[serde(deny_unknown_fields)]
@@ -176,8 +179,16 @@ pub struct ExportFile {
     /// `Name <email>` used when the authenticated principal has no mapping.
     #[serde(default)]
     pub default_author: Option<String>,
+    /// Email domains a `Claimed-Author` trailer may name. Any other claimed
+    /// email is dropped and only the name is kept (WI-3053). Unset means
+    /// [`DEFAULT_CLAIMED_AUTHOR_DOMAINS`].
+    #[serde(default)]
+    pub claimed_author_domains: Option<Vec<String>>,
 }
 
+/// The `[export] claimed_author_domains` default.
+pub const DEFAULT_CLAIMED_AUTHOR_DOMAINS: &[&str] = &["users.noreply.github.com"];
+
 /// A git identity in `[export.authors]`.
 #[derive(Clone, Debug, Deserialize, PartialEq, Eq)]
 #[serde(deny_unknown_fields)]
@@ -423,6 +434,26 @@ impl FileConfig {
         parse_author_identity(raw).map(Some)
     }
 
+    /// The `[export] claimed_author_domains` allow-list, lowercased, with
+    /// its default when unset.
+    pub fn claimed_author_domains(&self) -> Vec<String> {
+        match self
+            .export
+            .as_ref()
+            .and_then(|e| e.claimed_author_domains.as_ref())
+        {
+            Some(list) => list
+                .iter()
+                .map(|d| d.trim().to_ascii_lowercase())
+                .filter(|d| !d.is_empty() && !d.contains('@'))
+                .collect(),
+            None => DEFAULT_CLAIMED_AUTHOR_DOMAINS
+                .iter()
+                .map(|d| (*d).to_owned())
+                .collect(),
+        }
+    }
+
     /// Git identities from `[export.authors]`, keyed by principal.
     pub fn author_map(&self) -> BTreeMap<String, crate::batch::AuthorIdentity> {
         let Some(export) = &self.export else {
@@ -552,6 +583,52 @@ mirror_dir = "/var/lib/m.git"
 db = "/var/lib/m.sqlite"
 "#;
 
+    #[test]
+    fn claimed_author_domains_parse_with_default() {
+        let none = format!("{BASE}[[lines]]\nbranch = \"main\"\n");
+        assert_eq!(
+            FileConfig::parse(&none).unwrap().claimed_author_domains(),
+            vec!["users.noreply.github.com".to_owned()]
+        );
+        let set = format!(
+            "{BASE}[export]\nclaimed_author_domains = [\"Example.COM\", \"users.noreply.github.com\"]\n[[lines]]\nbranch = \"main\"\n"
+        );
+        assert_eq!(
+            FileConfig::parse(&set).unwrap().claimed_author_domains(),
+            vec![
+                "example.com".to_owned(),
+                "users.noreply.github.com".to_owned()
+            ]
+        );
+        let empty =
+            format!("{BASE}[export]\nclaimed_author_domains = []\n[[lines]]\nbranch = \"main\"\n");
+        assert!(
+            FileConfig::parse(&empty)
+                .unwrap()
+                .claimed_author_domains()
+                .is_empty()
+        );
+    }
+
+    #[test]
+    fn claimed_author_domains_drop_empty_and_at_entries() {
+        let set = format!(
+            "{BASE}[export]\nclaimed_author_domains = [\"\", \"  \", \"@x\", \"a@x.com\", \" Ok.com \"]\n[[lines]]\nbranch = \"main\"\n"
+        );
+        assert_eq!(
+            FileConfig::parse(&set).unwrap().claimed_author_domains(),
+            vec!["ok.com".to_owned()]
+        );
+    }
+
+    #[test]
+    fn claimed_author_domains_wrong_type_fails() {
+        let bad = format!(
+            "{BASE}[export]\nclaimed_author_domains = \"x\"\n[[lines]]\nbranch = \"main\"\n"
+        );
+        assert!(FileConfig::parse(&bad).is_err());
+    }
+
     #[test]
     fn default_author_is_name_and_email() {
         let ok = format!(

crates/graft-mirror/src/entry.rs

@@ -1041,6 +1041,7 @@ async fn cmd_pr_batch(args: &[String]) -> Result<(), CmdError> {
     let mut batch = BatchConfig::new(line, base, file.follow()?);
     batch.authors = file.author_map();
     batch.default_author = file.default_author()?;
+    batch.claimed_author_domains = file.claimed_author_domains();
     batch.exclude = exclude;
     batch.drop_reason = reason;
     batch.max_lands = max_lands;

crates/graft-mirror/src/sample_mirror.toml

@@ -76,6 +76,9 @@ token_file = "/path/to/token"
 import_pulls = false
 # "Name <email>" used when the authenticated principal has no authors entry.
 default_author = "Example Author <author@example.com>"
+# Email domains a batch commit's Claimed-Author trailer may name; any other
+# claimed email is dropped and the name kept. Default shown.
+# claimed_author_domains = ["users.noreply.github.com"]
 
 # Git identity per authenticated principal.
 [export.authors]

crates/graft-mirror/tests/batch.rs

@@ -1706,6 +1706,7 @@ async fn claimed_author_is_only_a_trailer_of_the_authenticated_principals() {
             ]
             .into_iter()
             .collect();
+            c.claimed_author_domains = vec!["example.com".to_owned()];
         })
         .run()
         .await
@@ -1781,10 +1782,10 @@ async fn an_unmapped_principal_exports_as_default_author() {
         &w.upstream,
         &["log", "-1", "--format=%B", &commits[0].sha.to_hex()],
     );
-    assert!(
-        message.contains("Claimed-Author: Zed <z@example.com>"),
-        "{message}"
-    );
+    // example.com is not on the default allow-list: the name stays, the
+    // client-supplied email does not (WI-3053).
+    assert!(message.contains("Claimed-Author: Zed\n"), "{message}");
+    assert!(!message.contains("z@example.com"), "{message}");
     assert!(!message.contains("subject-z"), "{message}");
 }