From dd563f231092b4c9832f56475a597c7dddd8e825 Mon Sep 17 00:00:00 2001 From: Toni Bergholm Date: Sun, 25 Oct 2026 11:07:18 +0200 Subject: [PATCH 1/2] fix(forge): serve extensionless Git HTTP URLs --- crates/forge/src/transport.rs | 23 +++++--- crates/forge/tests/e2e_git.rs | 54 +++++++++++-------- .../specs/2026-10-22-p38-forge-design.md | 3 +- 3 files changed, 49 insertions(+), 31 deletions(-) diff --git a/crates/forge/src/transport.rs b/crates/forge/src/transport.rs index 19e022c2..ce10ea07 100644 --- a/crates/forge/src/transport.rs +++ b/crates/forge/src/transport.rs @@ -1,4 +1,4 @@ -//! Smart-HTTP git transport: `/{owner}/{repo}.git/...` served by `git http-backend` as a CGI. +//! Smart-HTTP git transport with an optional `.git` suffix, served by `git http-backend` as a CGI. //! //! This module holds the parts that decide *what may be asked of git* and *how git's answer is //! read*, as plain functions with tests: the path whitelist, the git-specific credential and @@ -50,7 +50,7 @@ impl Sub { } } -/// A parsed `/{owner}/{repo}.git/{sub}` request path. +/// A parsed Git request path, with or without the repository's `.git` suffix. #[derive(Debug, Clone, PartialEq, Eq)] pub struct GitPath { pub owner: String, @@ -63,7 +63,7 @@ pub struct GitPath { pub enum Route { /// Not a git path at all (the ordinary router handles it). NotGit, - /// A `/{o}/{r}.git/...` path that is not on the whitelist: 404. + /// A Git path that is not on the whitelist: 404. Unknown, Git(GitPath), } @@ -110,6 +110,9 @@ fn sub_of(rest: &str) -> Option { /// plain name characters (so no `/`, `..` or control character can come out of a decode); the /// rest must match the whitelist exactly, byte for byte, with no escapes. pub fn parse(path: &str) -> Route { + if path.starts_with("/api/") { + return Route::NotGit; + } let Some(p) = path.strip_prefix('/') else { return Route::NotGit; }; @@ -117,10 +120,13 @@ pub fn parse(path: &str) -> Route { let (Some(owner), Some(repo_git)) = (it.next(), it.next()) else { return Route::NotGit; }; - let Some(repo_enc) = repo_git.strip_suffix(".git") else { - return Route::NotGit; - }; let rest = it.next().unwrap_or(""); + let sub = sub_of(rest); + let repo_enc = match repo_git.strip_suffix(".git") { + Some(repo) => repo, + None if sub.is_some() => repo_git, + None => return Route::NotGit, + }; let (Some(owner), Some(repo)) = ( crate::routes::percent_decode(owner), crate::routes::percent_decode(repo_enc), @@ -133,7 +139,7 @@ pub fn parse(path: &str) -> Route { { return Route::Unknown; } - match sub_of(rest) { + match sub { Some(sub) => Route::Git(GitPath { owner, repo, sub }), None => Route::Unknown, } @@ -565,9 +571,10 @@ mod tests { for p in [ "/", "/api/v1/user", + "/api/v1/HEAD", + "/api/v1/git-upload-pack", "/acme/alpha", "/acme", - "/acme/alpha/info/refs", "healthz", "/api/healthz", ] { diff --git a/crates/forge/tests/e2e_git.rs b/crates/forge/tests/e2e_git.rs index 044c6740..b3135fb5 100644 --- a/crates/forge/tests/e2e_git.rs +++ b/crates/forge/tests/e2e_git.rs @@ -85,16 +85,24 @@ fn push_then_clone_round_trips() { c.ok(&work, None, &["add", "."]); c.ok(&work, None, &["commit", "-q", "-m", "one"]); c.ok(&work, Some(&auth), &["push", &url, "main"]); - let clone = c.dir.join("clone"); - c.ok( - &c.dir, - Some(&auth), - &["clone", "-q", &url, clone.to_str().unwrap()], - ); - assert_eq!( - std::fs::read_to_string(clone.join("a.txt")).unwrap(), - "hello\n" - ); + let plain = url.strip_suffix(".git").unwrap(); + for (label, remote) in [("suffix", url.as_str()), ("plain", plain)] { + let clone = c.dir.join(format!("clone-{label}")); + c.ok( + &c.dir, + Some(&auth), + &["clone", "-q", remote, clone.to_str().unwrap()], + ); + assert_eq!( + std::fs::read_to_string(clone.join("a.txt")).unwrap(), + "hello\n" + ); + c.ok( + &clone, + Some(&auth), + &["push", "-q", remote, &format!("main:refs/heads/{label}")], + ); + } } // ---- helpers ---- @@ -202,18 +210,20 @@ fn missing_and_invisible_repositories_answer_identically() { ); assert_eq!(r.status, 201); for repo in ["secret", "nothere"] { - for q in [UP, RP] { - let r = s.send( - "GET", - &format!("/siteadmin/{repo}.git{q}"), - Some(&bob), - None, - ); - assert_eq!( - (r.status, body_text(&r).as_str()), - (404, "Repository not found"), - "{repo}{q}" - ); + for suffix in [".git", ""] { + for q in [UP, RP] { + let r = s.send( + "GET", + &format!("/siteadmin/{repo}{suffix}{q}"), + Some(&bob), + None, + ); + assert_eq!( + (r.status, body_text(&r).as_str()), + (404, "Repository not found"), + "{repo}{suffix}{q}" + ); + } } } } diff --git a/docs/superpowers/specs/2026-10-22-p38-forge-design.md b/docs/superpowers/specs/2026-10-22-p38-forge-design.md index 74c72ee3..27ed610e 100644 --- a/docs/superpowers/specs/2026-10-22-p38-forge-design.md +++ b/docs/superpowers/specs/2026-10-22-p38-forge-design.md @@ -768,7 +768,8 @@ F2b is the git half of F2 (plan: `docs/superpowers/plans/2026-10-23-p38-f2b-git- objects/pack/pack-.{pack,idx}}` only, served by `git http-backend` run as a CGI; everything else under `.git/` is 404, a wrong method 405, a dumb `info/refs` (no `service=`) 404 (git's dumb listing ignores `hideRefs`). The transport has its own whitelist (`transport::parse`) beside `routes::ROUTES`: an explicit, recorded exception to "every route is in - the table". Gitea also serves the path without `.git`; sc-forge does not. + the table". The `.git` suffix is optional, as in Gitea: both forms resolve to the same repository + and pass through the same authorization and CGI path construction. REST paths remain on the API router. - **Order of checks** on a git request: credentials (401), method (405), scope (403), repository and existence (404), permission (403), then encoding (415), declared body size (413), admission (503), gzip inflation (400/413), disk reservation (507), process start. From 9d053be3c7137377622bc02c53e4d968cc6e6336 Mon Sep 17 00:00:00 2001 From: Toni Bergholm Date: Sun, 25 Oct 2026 13:45:08 +0200 Subject: [PATCH 2/2] test(forge): stop GC before propagating writer thread failures --- crates/forge/tests/e2e_maintenance.rs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/crates/forge/tests/e2e_maintenance.rs b/crates/forge/tests/e2e_maintenance.rs index 4910fe40..118317d3 100644 --- a/crates/forge/tests/e2e_maintenance.rs +++ b/crates/forge/tests/e2e_maintenance.rs @@ -561,11 +561,12 @@ fn passes_beside_writers_lose_nothing() { }) }) .collect(); - for h in writers { - h.join().unwrap(); - } + let results: Vec<_> = writers.into_iter().map(|h| h.join()).collect(); stop.store(true, Ordering::SeqCst); passes.join().unwrap(); + for result in results { + result.unwrap(); + } }); assert!(!acknowledged.lock().unwrap().is_empty()); assert!(