diff --git a/changes/WI-3053.md b/changes/WI-3053.md new file mode 100644 index 0000000..72882e7 --- /dev/null +++ b/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. diff --git a/crates/graft-mirror/src/batch/batcher.rs b/crates/graft-mirror/src/batch/batcher.rs index 34248a8..814f75a 100644 --- a/crates/graft-mirror/src/batch/batcher.rs +++ b/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), diff --git a/crates/graft-mirror/src/batch/tests.rs b/crates/graft-mirror/src/batch/tests.rs index d8a9c19..8e8c578 100644 --- a/crates/graft-mirror/src/batch/tests.rs +++ b/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 ") ); + // 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 ") + ); + 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 { diff --git a/crates/graft-mirror/src/batch/types.rs b/crates/graft-mirror/src/batch/types.rs index 3aad11e..f247499 100644 --- a/crates/graft-mirror/src/batch/types.rs +++ b/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, + /// Email domains a `Claimed-Author` trailer may name, lowercased + /// (`[export] claimed_author_domains`). + pub claimed_author_domains: Vec, /// Lands to drop from the line's batches: each is reverted on the line. pub exclude: Vec, /// 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 { +/// 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, + default_author: Option<&AuthorIdentity>, + domains: &[String], +) -> Option { 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 { 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. diff --git a/crates/graft-mirror/src/config.rs b/crates/graft-mirror/src/config.rs index e8e9009..0b2e034 100644 --- a/crates/graft-mirror/src/config.rs +++ b/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 " +/// +/// # 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 ` used when the authenticated principal has no mapping. #[serde(default)] pub default_author: Option, + /// 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>, } +/// 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 { + 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 { 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!( diff --git a/crates/graft-mirror/src/entry.rs b/crates/graft-mirror/src/entry.rs index 121d8a2..ea83c5a 100644 --- a/crates/graft-mirror/src/entry.rs +++ b/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; diff --git a/crates/graft-mirror/src/sample_mirror.toml b/crates/graft-mirror/src/sample_mirror.toml index d02ba88..15245d6 100644 --- a/crates/graft-mirror/src/sample_mirror.toml +++ b/crates/graft-mirror/src/sample_mirror.toml @@ -76,6 +76,9 @@ token_file = "/path/to/token" import_pulls = false # "Name " used when the authenticated principal has no authors entry. default_author = "Example Author " +# 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] diff --git a/crates/graft-mirror/tests/batch.rs b/crates/graft-mirror/tests/batch.rs index 6e381fd..dc00dfb 100644 --- a/crates/graft-mirror/tests/batch.rs +++ b/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 "), - "{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}"); }