From b2c36f7c344d631da307ddb3723d0f5771cd38c8 Mon Sep 17 00:00:00 2001 From: Aleksandr Barkhatov Date: Fri, 14 Aug 2026 22:53:59 +0800 Subject: [PATCH 1/2] feat(git-cli-proxy): serve distinct commit authors per repository MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A commit records an author as a name and an e-mail; it carries no vendor account, because git has no concept of one. A consumer that needs the account has to ask the vendor, and asking once per commit is what this service exists to avoid — so it now answers with the authors themselves, one row per e-mail, each carrying a commit to look the account up by. The walk reuses the commit reader's record framing rather than the branch reader's: an ident is attacker-written, and 0x1f survives inside one, so only NUL-separated records keep a crafted name from forging another author's row. `since` is applied to the enumerated result for the reason the commit walk documents — `git log --since` is a traversal cutoff, so an author whose only qualifying commit sits behind an older parent would never be reached. Signed-off-by: Aleksandr Barkhatov --- .../backend/git-cli-proxy/openapi.json | 182 +++++++++++++ .../services/git-cli-proxy/src/api/data.rs | 65 ++++- .../services/git-cli-proxy/src/api/mod.rs | 48 ++++ .../git-cli-proxy/src/engine/read/authors.rs | 243 ++++++++++++++++++ .../git-cli-proxy/src/engine/read/commits.rs | 8 +- .../git-cli-proxy/src/engine/read/mod.rs | 79 +++++- 6 files changed, 619 insertions(+), 6 deletions(-) create mode 100644 src/backend/services/git-cli-proxy/src/engine/read/authors.rs diff --git a/docs/components/backend/git-cli-proxy/openapi.json b/docs/components/backend/git-cli-proxy/openapi.json index be6b7b542..a4cb0609e 100644 --- a/docs/components/backend/git-cli-proxy/openapi.json +++ b/docs/components/backend/git-cli-proxy/openapi.json @@ -1,6 +1,57 @@ { "components": { "schemas": { + "AuthorRow": { + "description": "One distinct commit author, with a commit of theirs to look them up by.\n\nGit records an author as a name and an e-mail and knows nothing of vendor\naccounts, so a consumer that needs the account resolves it against the\nvendor — one lookup per author rather than one per commit, which is the\nwhole reason this endpoint exists.", + "properties": { + "author_email": { + "type": "string" + }, + "author_name": { + "type": "string" + }, + "commit_count": { + "format": "int64", + "minimum": 0, + "type": "integer" + }, + "last_committed_date": { + "type": "string" + }, + "sample_sha": { + "description": "A commit this author wrote, for the account lookup. The author's most\nrecent one: a vendor matches an e-mail against the accounts that carry\nit today, so the freshest commit is the likeliest to resolve.", + "type": "string" + } + }, + "required": [ + "author_email", + "author_name", + "sample_sha", + "last_committed_date", + "commit_count" + ], + "type": "object" + }, + "AuthorsPage": { + "properties": { + "items": { + "items": { + "$ref": "#/components/schemas/AuthorRow" + }, + "type": "array" + }, + "next_page_token": { + "type": [ + "string", + "null" + ] + } + }, + "required": [ + "items" + ], + "type": "object" + }, "BranchRow": { "properties": { "head_committed_date": { @@ -282,6 +333,137 @@ }, "openapi": "3.1.0", "paths": { + "/v1/authors": { + "get": { + "operationId": "git_cli_proxy.authors.list", + "parameters": [ + { + "description": "Clone URL of the repository (http/https)", + "in": "query", + "name": "repo", + "required": true, + "schema": { + "type": "string" + } + }, + { + "description": "Lower bound on committed_date; an author whose commits all predate it is omitted", + "in": "query", + "name": "since", + "required": false, + "schema": { + "type": "string" + } + }, + { + "description": "1..=1000, default 1000; larger values are clamped", + "in": "query", + "name": "page_size", + "required": false, + "schema": { + "type": "integer" + } + }, + { + "description": "Cursor from a previous page; pins the snapshot and never fetches", + "in": "query", + "name": "page_token", + "required": false, + "schema": { + "type": "string" + } + } + ], + "responses": { + "200": { + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/AuthorsPage" + } + } + }, + "description": "One page of authors" + }, + "400": { + "content": { + "application/problem+json": { + "schema": { + "$ref": "#/components/schemas/Problem" + } + } + }, + "description": "Bad Request" + }, + "401": { + "content": { + "application/problem+json": { + "schema": { + "$ref": "#/components/schemas/Problem" + } + } + }, + "description": "Unauthorized" + }, + "404": { + "content": { + "application/problem+json": { + "schema": { + "$ref": "#/components/schemas/Problem" + } + } + }, + "description": "Not Found" + }, + "409": { + "content": { + "application/problem+json": { + "schema": { + "$ref": "#/components/schemas/Problem" + } + } + }, + "description": "Conflict" + }, + "413": { + "content": { + "application/problem+json": { + "schema": { + "$ref": "#/components/schemas/Problem" + } + } + }, + "description": "Repository exceeds the configured per-repository size cap" + }, + "429": { + "content": { + "application/problem+json": { + "schema": { + "$ref": "#/components/schemas/Problem" + } + } + }, + "description": "Too Many Requests" + }, + "500": { + "content": { + "application/problem+json": { + "schema": { + "$ref": "#/components/schemas/Problem" + } + } + }, + "description": "Internal Server Error" + } + }, + "security": [ + { + "bearerAuth": [] + } + ], + "summary": "Distinct commit authors, ascending by e-mail" + } + }, "/v1/branches": { "get": { "operationId": "git_cli_proxy.branches.list", diff --git a/src/backend/services/git-cli-proxy/src/api/data.rs b/src/backend/services/git-cli-proxy/src/api/data.rs index 896e587ea..27a487013 100644 --- a/src/backend/services/git-cli-proxy/src/api/data.rs +++ b/src/backend/services/git-cli-proxy/src/api/data.rs @@ -12,7 +12,7 @@ use utoipa::ToSchema; use crate::engine::key::CacheKey; use crate::engine::page::PageToken; -use crate::engine::read::{self, Page, branches, commits, numstat, patches}; +use crate::engine::read::{self, Page, authors, branches, commits, numstat, patches}; use crate::engine::runner::GitError; use crate::engine::store::{Freshness, RepoGuard, StoreError}; @@ -52,6 +52,14 @@ pub struct BranchesQuery { page_token: Option, } +#[derive(Debug, Deserialize)] +pub struct AuthorsQuery { + repo: Option, + since: Option, + page_size: Option, + page_token: Option, +} + /// Concrete page wrappers, one per endpoint: `Page` cannot be a schema /// because the registry keys components on the type's own name, so all three /// instantiations would collide on one component. @@ -76,6 +84,13 @@ pub struct BranchesPage { } impl toolkit::api::api_dto::ResponseApiDto for BranchesPage {} +#[derive(Debug, Serialize, ToSchema)] +pub struct AuthorsPage { + pub items: Vec, + pub next_page_token: Option, +} +impl toolkit::api::api_dto::ResponseApiDto for AuthorsPage {} + impl From> for CommitsPage { fn from(page: Page) -> Self { Self { @@ -103,6 +118,15 @@ impl From> for BranchesPage { } } +impl From> for AuthorsPage { + fn from(page: Page) -> Self { + Self { + items: page.items, + next_page_token: page.next_page_token, + } + } +} + #[derive(Debug, Serialize, ToSchema)] pub struct FileChangeRow { pub sha: String, @@ -406,6 +430,45 @@ pub async fn list_branches( json_page(BranchesPage::from(page)).await } +/// `GET /v1/authors` — one row per distinct commit author. +/// +/// # Errors +/// +/// [`ApiError`] on malformed input or an origin failure. +pub async fn list_authors( + Extension(state): Extension>, + headers: HeaderMap, + ValidatedQuery(query): ValidatedQuery, +) -> Result { + let repo = required_param(query.repo.as_deref(), "repo")?; + let context = RequestContext::from_parts(&headers, repo, state.clone_url_policy())?; + let paging = Paging::parse(query.page_token.as_deref(), query.page_size)?; + + let page = read_snapshot(&state, &context, &paging, |guard: RepoGuard| { + let (state, context, paging) = (&state, &context, &paging); + let since = query.since.as_deref(); + Box::pin(async move { + // Already sorted by e-mail, which is the ascending key the cursor + // pages on — an author has no date to order by, since the walk + // collapses every commit they wrote into one row. + let all = + authors::read(state.store.runner(), guard.git_dir(), &context.creds, since).await?; + let (items, cursor) = + read::slice_page(all, paging.token.as_ref(), paging.page_size, |row| { + (row.author_email.clone(), String::new()) + }); + + Ok(Page { + items, + next_page_token: encode_cursor(cursor, &context.key, &guard), + }) + }) + }) + .await?; + + json_page(AuthorsPage::from(page)).await +} + /// One page of commit keys, its cursor, and — when the index answered — the /// page's default-branch membership. /// diff --git a/src/backend/services/git-cli-proxy/src/api/mod.rs b/src/backend/services/git-cli-proxy/src/api/mod.rs index 1056e13b2..155cb5184 100644 --- a/src/backend/services/git-cli-proxy/src/api/mod.rs +++ b/src/backend/services/git-cli-proxy/src/api/mod.rs @@ -252,6 +252,54 @@ fn build_operations(router: Router, openapi: &dyn OpenApiRegistry) -> Router { .handler(data::list_file_changes) .register(router, openapi); + let router = OperationBuilder::get("/v1/authors") + .operation_id("git_cli_proxy.authors.list") + .summary("Distinct commit authors, ascending by e-mail") + .authenticated() + .no_license_required() + .query_param_typed( + "repo", + true, + "Clone URL of the repository (http/https)", + "string", + ) + .query_param_typed( + "since", + false, + "Lower bound on committed_date; an author whose commits all predate it is omitted", + "string", + ) + .query_param_typed( + "page_size", + false, + "1..=1000, default 1000; larger values are clamped", + "integer", + ) + .query_param_typed( + "page_token", + false, + "Cursor from a previous page; pins the snapshot and never fetches", + "string", + ) + .json_response_with_schema::( + openapi, + StatusCode::OK, + "One page of authors", + ) + .error_400(openapi) + .error_401(openapi) + .error_404(openapi) + .error_409(openapi) + .error_429(openapi) + .error_500(openapi) + .problem_response( + openapi, + StatusCode::PAYLOAD_TOO_LARGE, + "Repository exceeds the configured per-repository size cap", + ) + .handler(data::list_authors) + .register(router, openapi); + OperationBuilder::get("/v1/branches") .operation_id("git_cli_proxy.branches.list") .summary("Branch heads, ascending by name") diff --git a/src/backend/services/git-cli-proxy/src/engine/read/authors.rs b/src/backend/services/git-cli-proxy/src/engine/read/authors.rs new file mode 100644 index 000000000..b9c07e004 --- /dev/null +++ b/src/backend/services/git-cli-proxy/src/engine/read/authors.rs @@ -0,0 +1,243 @@ +use std::collections::HashMap; +use std::path::Path; + +use serde::Serialize; + +use super::commits::{FIELD, RECORD, is_object_id, ordinal_of, parse_instant, scrub}; +use crate::engine::runner::{GitCredentials, GitError, GitRunner}; + +/// One distinct commit author, with a commit of theirs to look them up by. +/// +/// Git records an author as a name and an e-mail and knows nothing of vendor +/// accounts, so a consumer that needs the account resolves it against the +/// vendor — one lookup per author rather than one per commit, which is the +/// whole reason this endpoint exists. +#[derive(Debug, Clone, Serialize, PartialEq, Eq, utoipa::ToSchema)] +pub struct AuthorRow { + pub author_email: String, + pub author_name: String, + /// A commit this author wrote, for the account lookup. The author's most + /// recent one: a vendor matches an e-mail against the accounts that carry + /// it today, so the freshest commit is the likeliest to resolve. + pub sample_sha: String, + pub last_committed_date: String, + pub commit_count: u64, +} + +/// Every distinct author of a commit reachable from any branch, ascending by +/// e-mail. +/// +/// `since` bounds by committed date. It is applied to the enumerated result and +/// NOT passed to `git log --since`, for the reason spelled out in +/// [`super::commits::enumerate`]: `--since` is a traversal cutoff, so an +/// author whose only qualifying commit sits behind an older parent would never +/// be reached. +/// +/// # Errors +/// +/// [`GitError`] when the git invocation fails. +pub async fn read( + runner: &GitRunner, + git_dir: &Path, + creds: &GitCredentials, + since: Option<&str>, +) -> Result, GitError> { + let format = format!("--pretty=format:%H{FIELD}%cI{FIELD}%an{FIELD}%ae"); + // `--branches` for the same reason the commit walk uses it: reachability + // from a branch is the contract, and tags outlive the branch they were cut + // from. + let args = vec!["log", "--branches", "--no-color", "-z", &format]; + + let output = runner.run(Some(git_dir), &args, Some(creds)).await?; + let text = String::from_utf8_lossy(&output.stdout); + + Ok(fold(&text, since)) +} + +/// Collapse the walk to one row per e-mail. +/// +/// The e-mail is the identity: a person commits under one address with their +/// name spelled several ways, and the name is carried only so a consumer can +/// label the row. The most recent commit wins both the name and the sample. +fn fold(text: &str, since: Option<&str>) -> Vec { + let bound = since.and_then(parse_instant); + let mut by_email: HashMap = HashMap::new(); + + for record in text.split(RECORD) { + if record.trim().is_empty() { + continue; + } + let mut fields = record.splitn(4, FIELD); + let Some(sha) = fields.next().map(str::trim) else { + continue; + }; + if !is_object_id(sha) { + continue; + } + let (Some(committed_date), Some(author_name), Some(author_email)) = + (fields.next(), fields.next(), fields.next()) + else { + continue; + }; + // An unparseable date is kept, matching retain_keys_since: a row whose + // date we cannot read is not evidence that it falls outside the window. + if bound.is_some_and(|bound| parse_instant(committed_date).is_some_and(|at| at < bound)) { + continue; + } + let author_email = scrub(author_email); + if author_email.trim().is_empty() { + continue; + } + + let ordinal = ordinal_of(committed_date); + by_email + .entry(author_email.clone()) + .and_modify(|row| { + row.commit_count += 1; + if ordinal > ordinal_of(&row.last_committed_date) { + committed_date.clone_into(&mut row.last_committed_date); + sha.clone_into(&mut row.sample_sha); + row.author_name = scrub(author_name); + } + }) + .or_insert_with(|| AuthorRow { + author_email, + author_name: scrub(author_name), + sample_sha: sha.to_owned(), + last_committed_date: committed_date.to_owned(), + commit_count: 1, + }); + } + + let mut rows: Vec = by_email.into_values().collect(); + rows.sort_by(|a, b| a.author_email.cmp(&b.author_email)); + rows +} + +#[cfg(test)] +mod tests { + use super::*; + + const SHA_A: &str = "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"; + const SHA_B: &str = "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb"; + const SHA_C: &str = "cccccccccccccccccccccccccccccccccccccccc"; + + fn record(sha: &str, date: &str, name: &str, email: &str) -> String { + format!("{sha}\n{date}\n{name}\n{email}\0") + } + + #[test] + fn one_row_per_email_counting_every_commit() { + let text = record(SHA_A, "2026-08-01T10:00:00+00:00", "Ada", "ada@example.com") + + &record( + SHA_B, + "2026-08-02T10:00:00+00:00", + "Ada L", + "ada@example.com", + ) + + &record(SHA_C, "2026-08-03T10:00:00+00:00", "Bo", "bo@example.com"); + + let rows = fold(&text, None); + + assert_eq!(rows.len(), 2, "one row per distinct e-mail"); + assert_eq!(rows[0].author_email, "ada@example.com"); + assert_eq!(rows[0].commit_count, 2); + assert_eq!(rows[1].author_email, "bo@example.com"); + } + + #[test] + fn the_newest_commit_supplies_the_sample_and_the_name() { + let text = record( + SHA_B, + "2026-08-02T10:00:00+00:00", + "Ada L", + "ada@example.com", + ) + &record(SHA_A, "2026-08-01T10:00:00+00:00", "Ada", "ada@example.com"); + + let rows = fold(&text, None); + + assert_eq!(rows[0].sample_sha, SHA_B); + assert_eq!(rows[0].author_name, "Ada L"); + } + + #[test] + fn newest_is_decided_by_instant_not_by_text() { + // +02:00 sorts after Z as text while being the earlier instant. + let text = record(SHA_A, "2026-08-01T09:30:00Z", "Ada", "ada@example.com") + + &record(SHA_B, "2026-08-01T10:00:00+02:00", "Ada", "ada@example.com"); + + let rows = fold(&text, None); + + assert_eq!( + rows[0].sample_sha, SHA_A, + "09:30Z is later than 10:00+02:00" + ); + } + + #[test] + fn since_drops_authors_whose_commits_all_predate_it() { + let text = record(SHA_A, "2026-07-01T10:00:00+00:00", "Old", "old@example.com") + + &record(SHA_B, "2026-08-02T10:00:00+00:00", "New", "new@example.com"); + + let rows = fold(&text, Some("2026-08-01T00:00:00Z")); + + assert_eq!(rows.len(), 1); + assert_eq!(rows[0].author_email, "new@example.com"); + } + + #[test] + fn since_counts_only_the_commits_inside_the_window() { + let text = record(SHA_A, "2026-07-01T10:00:00+00:00", "Ada", "ada@example.com") + + &record(SHA_B, "2026-08-02T10:00:00+00:00", "Ada", "ada@example.com"); + + let rows = fold(&text, Some("2026-08-01T00:00:00Z")); + + assert_eq!(rows[0].commit_count, 1); + assert_eq!(rows[0].sample_sha, SHA_B); + } + + #[test] + fn an_ident_carrying_the_separator_cannot_shift_another_authors_row() { + // The name is attacker-written; a newline in it would take the e-mail + // field's place were records not NUL-terminated and fields scrubbed. + let text = format!("{SHA_A}\n2026-08-01T10:00:00+00:00\nEve\nevil\nfake@example.com\0") + + &record(SHA_B, "2026-08-02T10:00:00+00:00", "Bo", "bo@example.com"); + + let rows = fold(&text, None); + + assert!( + rows.iter() + .all(|row| row.author_email != "fake@example.com"), + "a forged trailing field must not become another author" + ); + assert!(rows.iter().any(|row| row.author_email == "bo@example.com")); + } + + #[test] + fn rows_without_a_usable_identity_are_skipped() { + let cases = vec![ + ("empty listing", String::new()), + ("blank record", "\0".to_owned()), + ( + "short record", + format!("{SHA_A}\n2026-08-01T10:00:00+00:00\0"), + ), + ( + "not an object id", + record( + "nope", + "2026-08-01T10:00:00+00:00", + "Ada", + "ada@example.com", + ), + ), + ( + "empty email", + record(SHA_A, "2026-08-01T10:00:00+00:00", "Ada", " "), + ), + ]; + for (name, text) in cases { + assert!(fold(&text, None).is_empty(), "must skip: {name}"); + } + } +} diff --git a/src/backend/services/git-cli-proxy/src/engine/read/commits.rs b/src/backend/services/git-cli-proxy/src/engine/read/commits.rs index 9352d5380..d7814d045 100644 --- a/src/backend/services/git-cli-proxy/src/engine/read/commits.rs +++ b/src/backend/services/git-cli-proxy/src/engine/read/commits.rs @@ -61,7 +61,7 @@ impl CommitHeader { /// an author named `A<0x1f>B` used to push `B` into the email field and every /// later field one place along, forging a row whose author, email and /// committer are attacker-chosen. -const FIELD: char = '\n'; +pub(super) const FIELD: char = '\n'; /// Records are NUL-separated (`git log -z`), and that choice is load-bearing. /// A commit message is attacker-controlled — anyone who can push to a synced @@ -69,7 +69,7 @@ const FIELD: char = '\n'; /// close its own record and open a forged one, with an attacker-chosen sha, /// author and email. git truncates a commit message at the first NUL, so NUL /// is the one byte a message provably cannot contain. -const RECORD: char = '\0'; +pub(super) const RECORD: char = '\0'; /// One commit's position in the walk, and just enough to filter on. /// @@ -361,7 +361,7 @@ const PATCH_ID_BATCH: usize = 128; /// A full object id: 40 hex characters under SHA-1, 64 under SHA-256. Pinning /// only the SHA-1 length silently discards every commit in a SHA-256 /// repository, because each parsed record fails this check and is dropped. -fn is_object_id(value: &str) -> bool { +pub(super) fn is_object_id(value: &str) -> bool { matches!(value.len(), 40 | 64) && value.chars().all(|c| c.is_ascii_hexdigit()) } @@ -378,7 +378,7 @@ fn parse_headers(text: &str) -> Vec { /// fields of that record. The record still parses and its sha is still its /// own, so the blast radius is the attacker's own row — but the value that /// reaches bronze should not carry control bytes either way. -fn scrub(value: &str) -> String { +pub(super) fn scrub(value: &str) -> String { value.chars().filter(|c| !c.is_control()).collect() } diff --git a/src/backend/services/git-cli-proxy/src/engine/read/mod.rs b/src/backend/services/git-cli-proxy/src/engine/read/mod.rs index b340ec6e5..6dc4e9cc1 100644 --- a/src/backend/services/git-cli-proxy/src/engine/read/mod.rs +++ b/src/backend/services/git-cli-proxy/src/engine/read/mod.rs @@ -1,3 +1,4 @@ +pub mod authors; pub mod blobs; pub mod branches; pub mod commits; @@ -135,7 +136,7 @@ mod live_tests { use crate::engine::store::Freshness; use crate::engine::store::tests::{always_fetch, creds, fixture, key, open_until_ready, sh}; - use super::{blobs, branches, commits, numstat, patches, slice_page}; + use super::{authors, blobs, branches, commits, numstat, patches, slice_page}; use crate::engine::page::PageToken; use crate::engine::read::commits::CommitKey; @@ -259,6 +260,82 @@ mod live_tests { ); } + /// The author walk against a real clone: one row per e-mail whatever the + /// spelling of the name, counted across every branch, and `since` bounded + /// by the same instant comparison the commit walk uses. + #[tokio::test] + async fn authors_fold_the_walk_to_one_row_per_email() { + let f = fixture("authors"); + sh( + &f.root.join("origin"), + "echo two >> a.txt && git add . && \ + GIT_AUTHOR_NAME='Ada' GIT_AUTHOR_EMAIL='ada@example.com' \ + GIT_AUTHOR_DATE='2026-08-02T11:00:00+0000' \ + GIT_COMMITTER_DATE='2026-08-02T11:00:00+0000' git commit -qm second && \ + echo three >> a.txt && git add . && \ + GIT_AUTHOR_NAME='Ada Lovelace' GIT_AUTHOR_EMAIL='ada@example.com' \ + GIT_AUTHOR_DATE='2026-08-04T11:00:00+0000' \ + GIT_COMMITTER_DATE='2026-08-04T11:00:00+0000' git commit -qm third && \ + git checkout -q -b feature && echo four > c.txt && git add c.txt && \ + GIT_AUTHOR_NAME='Bo' GIT_AUTHOR_EMAIL='bo@example.com' \ + GIT_AUTHOR_DATE='2026-08-05T12:00:00+0000' \ + GIT_COMMITTER_DATE='2026-08-05T12:00:00+0000' git commit -qm fourth && \ + git checkout -q main", + ); + + let k = key(&f); + let guard = open_until_ready(&f, &k, refresh()).await; + let runner = f.store.runner(); + let git_dir = guard.git_dir(); + + let rows = match authors::read(runner, git_dir, &creds(), None).await { + Ok(r) => r, + Err(e) => panic!("authors::read: {e}"), + }; + + // The fixture's own root commit is authored by test@example.com. + assert_eq!( + rows.iter() + .map(|r| r.author_email.as_str()) + .collect::>(), + vec!["ada@example.com", "bo@example.com", "test@example.com"], + "distinct authors, ascending by e-mail: {rows:?}" + ); + + let ada = &rows[0]; + assert_eq!(ada.commit_count, 2, "both spellings are one author"); + assert_eq!( + ada.author_name, "Ada Lovelace", + "the newest commit names them" + ); + assert!( + !ada.sample_sha.is_empty(), + "an author carries a commit to look up" + ); + assert!( + rows.iter().any(|r| r.author_email == "bo@example.com"), + "a feature-branch author is walked too" + ); + + let recent = + match authors::read(runner, git_dir, &creds(), Some("2026-08-03T00:00:00Z")).await { + Ok(r) => r, + Err(e) => panic!("authors::read(since): {e}"), + }; + assert_eq!( + recent + .iter() + .map(|r| r.author_email.as_str()) + .collect::>(), + vec!["ada@example.com", "bo@example.com"], + "an author whose commits all predate `since` drops out: {recent:?}" + ); + assert_eq!( + recent[0].commit_count, 1, + "only the commits inside the window count" + ); + } + #[tokio::test] async fn stat_retention_stops_at_the_row_cap_and_totals_stay_whole() { // The per-file map is only needed up to the row cap — nothing past it From 92c41435e10a4c449b26046f1236cab0a5318682 Mon Sep 17 00:00:00 2001 From: Aleksandr Barkhatov Date: Sun, 16 Aug 2026 14:02:09 +0800 Subject: [PATCH 2/2] fix(git-cli-proxy): keep the author e-mail a fully delimited field MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The name is the attacker-written half of an ident, so it goes last and absorbs whatever a record has left over. Git strips newlines out of an ident, so neither field can carry the separator either way — but the e-mail is what every row is keyed and grouped on, and resting that on git's behaviour buys nothing over ordering the fields for it. Signed-off-by: Aleksandr Barkhatov --- .../git-cli-proxy/src/engine/read/authors.rs | 51 +++++++++++++++---- 1 file changed, 40 insertions(+), 11 deletions(-) diff --git a/src/backend/services/git-cli-proxy/src/engine/read/authors.rs b/src/backend/services/git-cli-proxy/src/engine/read/authors.rs index b9c07e004..6feddfd87 100644 --- a/src/backend/services/git-cli-proxy/src/engine/read/authors.rs +++ b/src/backend/services/git-cli-proxy/src/engine/read/authors.rs @@ -42,7 +42,12 @@ pub async fn read( creds: &GitCredentials, since: Option<&str>, ) -> Result, GitError> { - let format = format!("--pretty=format:%H{FIELD}%cI{FIELD}%an{FIELD}%ae"); + // The NAME goes last, so it is the field that absorbs whatever a record + // has left over. Git strips newlines out of an ident, so no field here can + // carry the separator — but the e-mail is the identity every row is keyed + // and grouped on, and it costs nothing to keep it fully delimited rather + // than resting that on git's behaviour. + let format = format!("--pretty=format:%H{FIELD}%cI{FIELD}%ae{FIELD}%an"); // `--branches` for the same reason the commit walk uses it: reachability // from a branch is the contract, and tags outlive the branch they were cut // from. @@ -74,7 +79,7 @@ fn fold(text: &str, since: Option<&str>) -> Vec { if !is_object_id(sha) { continue; } - let (Some(committed_date), Some(author_name), Some(author_email)) = + let (Some(committed_date), Some(author_email), Some(author_name)) = (fields.next(), fields.next(), fields.next()) else { continue; @@ -123,7 +128,7 @@ mod tests { const SHA_C: &str = "cccccccccccccccccccccccccccccccccccccccc"; fn record(sha: &str, date: &str, name: &str, email: &str) -> String { - format!("{sha}\n{date}\n{name}\n{email}\0") + format!("{sha}\n{date}\n{email}\n{name}\0") } #[test] @@ -198,19 +203,43 @@ mod tests { #[test] fn an_ident_carrying_the_separator_cannot_shift_another_authors_row() { - // The name is attacker-written; a newline in it would take the e-mail - // field's place were records not NUL-terminated and fields scrubbed. - let text = format!("{SHA_A}\n2026-08-01T10:00:00+00:00\nEve\nevil\nfake@example.com\0") - + &record(SHA_B, "2026-08-02T10:00:00+00:00", "Bo", "bo@example.com"); + // The name is attacker-written and goes last, so a separator inside it + // is absorbed by the name itself: the row still keys on the real + // e-mail, and no forged value becomes an author of its own. + let text = + format!("{SHA_A}\n2026-08-01T10:00:00+00:00\neve@example.com\nEve\nfake@example.com\0") + + &record(SHA_B, "2026-08-02T10:00:00+00:00", "Bo", "bo@example.com"); let rows = fold(&text, None); - assert!( + assert_eq!( rows.iter() - .all(|row| row.author_email != "fake@example.com"), - "a forged trailing field must not become another author" + .map(|row| row.author_email.as_str()) + .collect::>(), + vec!["bo@example.com", "eve@example.com"], + "a forged trailing field must not become another author: {rows:?}" ); - assert!(rows.iter().any(|row| row.author_email == "bo@example.com")); + let eve = rows + .iter() + .find(|row| row.author_email == "eve@example.com"); + assert!( + eve.is_some_and(|row| row.author_name == "Evefake@example.com"), + "the overflow lands in the name, scrubbed of the separator: {eve:?}" + ); + } + + #[test] + fn an_author_with_no_email_claims_no_row_and_disturbs_no_other() { + // Git records an empty ident e-mail as `<>`; the row has no identity to + // key on, and dropping it must not shift the record that follows. + let text = record(SHA_A, "2026-08-01T10:00:00+00:00", "No Address", "") + + &record(SHA_B, "2026-08-02T10:00:00+00:00", "Bo", "bo@example.com"); + + let rows = fold(&text, None); + + assert_eq!(rows.len(), 1, "the e-mail-less author claims nothing"); + assert_eq!(rows[0].author_email, "bo@example.com"); + assert_eq!(rows[0].author_name, "Bo", "the next record parses intact"); } #[test]