From 08bf88a3ca3c8720667eba8bbdb773459da37fac Mon Sep 17 00:00:00 2001 From: Alpha Chen Date: Sun, 26 Apr 2026 15:26:33 -0700 Subject: [PATCH] Tighten post-receive hook based on review MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit Move the push out of the hook into Repo::push_to_mirror so the actual push logic is unit-testable independent of GIT_DIR/stdin orchestration. Reject mirror URLs that embed credentials at deserialization, since a typo would leak the token via tracing or Sentry. Make terminal-stdin invocation an error rather than a silent no-op — running the hook manually is a misuse, not a state. Documents the env-var token exposure (visible in /proc//environ) as a known limitation in the helper's docstring; revisit when the CI runner lands. Updates README to describe the PAT approach instead of the per-repo deploy key the original design called for. Assisted-by: Claude Opus 4.7 via Claude Code --- README.md | 6 +- src/bin/quire/commands/hook.rs | 48 ++++------ src/quire.rs | 160 ++++++++++++++++++++++++++++++++- 3 files changed, 176 insertions(+), 38 deletions(-) diff --git a/README.md b/README.md index e5fef31..ada3cb9 100644 --- a/README.md +++ b/README.md @@ -10,7 +10,7 @@ A Rust binary that runs in a Docker container, fronted by the host's sshd and a - **Git hosting over SSH**, via the host's sshd dispatching into the container. Explicit repo creation (`ssh git@host quire repo new `). - **A read-only web view** for browsing README, tree, history, blame, diffs, and refs. -- **Automatic mirroring to GitHub** on push, when configured per-repo. Each repo carries its own deploy key — no agent socket to plumb across the host/container boundary. +- **Automatic mirroring to GitHub** on push, when configured per-repo. A single GitHub PAT lives in global config and rides on the push as an `http.extraHeader` — no per-repo deploy keys, no agent socket plumbing. - **Fennel-based CI** (Fennel is a Lisp that compiles to Lua), with pipelines defined in `.quire/ci.fnl`. Unsandboxed by default since every pipeline is code I've written; a bubblewrap-based opt-in is available for the day quire ever runs code I haven't. - **Email notifications** for CI failures, recoveries, and mirror-push failures. SMTP via `msmtp`; plain text; per-repo config for what to send and to whom. @@ -36,9 +36,9 @@ Quire's data lives under one volume: ``` /var/quire/ - repos/ bare git repos; each has a .git/quire/ dir with config + mirror deploy key + repos/ bare git repos; per-repo config lives in-tree at .quire/config.fnl runs/ CI run metadata, artifacts, and logs; retention-policied - config.fnl global config + config.fnl global config (GitHub PAT, SMTP creds, etc.) ``` Host-side config (sshd_config block, Caddyfile, docker-compose file) lives on the host, version-controlled separately. See `PLAN.md` for the reference layout. diff --git a/src/bin/quire/commands/hook.rs b/src/bin/quire/commands/hook.rs index 9d2d26c..dc654e9 100644 --- a/src/bin/quire/commands/hook.rs +++ b/src/bin/quire/commands/hook.rs @@ -1,7 +1,7 @@ -use std::io::{self, BufRead, IsTerminal}; +use std::io::{self, IsTerminal}; use std::path::PathBuf; -use miette::{Context, Result, ensure, miette}; +use miette::{Context, Result, bail, ensure, miette}; use quire::Quire; #[derive(Clone, Copy, Debug, clap::ValueEnum)] @@ -25,16 +25,11 @@ pub async fn run(quire: &Quire, hook_name: HookName) -> Result<()> { } fn post_receive(quire: &Quire) -> Result<()> { - // post-receive receives updated refs on stdin. We only care that - // at least one ref was pushed — we don't need to parse them. - let stdin = io::stdin(); - if stdin.is_terminal() { - // Not running as a git hook — nothing to do. - return Ok(()); - } - let has_refs = stdin.lock().lines().any(|line| line.is_ok()); - if !has_refs { - return Ok(()); + // git invokes hooks with refs piped on stdin. A terminal here means + // a human typed `quire hook post-receive` directly — that's a misuse, + // not a no-op. + if io::stdin().is_terminal() { + bail!("quire hook is for git to invoke, not for direct CLI use"); } // GIT_DIR is set by git when running hooks in bare repos. @@ -45,11 +40,15 @@ fn post_receive(quire: &Quire) -> Result<()> { let repo = quire .repo_from_path(&git_dir) .context("hook running in unrecognized repo")?; + ensure!( + repo.exists(), + "GIT_DIR points to a non-existent repo: {}", + git_dir.display() + ); let repo_config = repo.config()?; - let mirror = match repo_config.mirror { - Some(m) => m, - None => return Ok(()), + let Some(mirror) = repo_config.mirror else { + return Ok(()); }; let global_config = quire.global_config()?; @@ -60,24 +59,7 @@ fn post_receive(quire: &Quire) -> Result<()> { .context("failed to resolve GitHub token")?; tracing::info!(url = %mirror.url, "pushing to mirror"); - - // Token is passed via -c flag — never written to disk or visible in - // process arguments (git redacts http.extraHeader in trace output). - let status = repo - .git(&["push", "--porcelain", &mirror.url, "main"]) - .env("GIT_CONFIG_COUNT", "1") - .env("GIT_CONFIG_KEY_0", "http.extraHeader") - .env( - "GIT_CONFIG_VALUE_0", - format!("Authorization: Bearer {token}"), - ) - .stdout(std::process::Stdio::null()) - .status() - .map_err(quire::Error::Io) - .context("failed to run git push")?; - - ensure!(status.success(), "git push to mirror failed"); - + repo.push_to_mirror(&mirror, token)?; tracing::info!(url = %mirror.url, "mirror push complete"); Ok(()) } diff --git a/src/quire.rs b/src/quire.rs index 77eb7e3..78a589e 100644 --- a/src/quire.rs +++ b/src/quire.rs @@ -35,9 +35,30 @@ pub struct RepoConfig { #[derive(serde::Deserialize, Debug, PartialEq)] pub struct MirrorConfig { + #[serde(deserialize_with = "deserialize_mirror_url")] pub url: String, } +/// Reject URLs with embedded user[:password]@ credentials so a misconfigured +/// repo can't leak a token via tracing, Sentry, or git's own error output. +/// Tokens come from global config and ride in `http.extraHeader`. +fn deserialize_mirror_url<'de, D>(deserializer: D) -> std::result::Result +where + D: serde::Deserializer<'de>, +{ + use serde::Deserialize; + let url = String::deserialize(deserializer)?; + if let Some((_, after_scheme)) = url.split_once("://") + && let Some(at) = after_scheme.find('@') + && !after_scheme[..at].contains('/') + { + return Err(serde::de::Error::custom( + "mirror URL must not embed credentials; tokens come from global config", + )); + } + Ok(url) +} + /// A resolved repository path. /// /// Created by `Quire::repo` after validating the name. @@ -64,6 +85,33 @@ impl Repo { cmd } + /// Push `main` to the configured mirror, injecting the GitHub token via + /// `http.extraHeader` so it never appears in the URL or git's error output. + /// + /// The token is passed through `GIT_CONFIG_*` env vars on the child + /// process. This keeps it out of the command line (visible via `ps`), + /// but it remains visible in `/proc//environ` to anything running + /// as the same uid for the lifetime of the push. Acceptable today + /// (single-user container, no CI runner yet); revisit when CI lands. + pub fn push_to_mirror(&self, mirror: &MirrorConfig, token: &str) -> crate::Result<()> { + let status = self + .git(&["push", "--porcelain", &mirror.url, "main"]) + .env("GIT_CONFIG_COUNT", "1") + .env("GIT_CONFIG_KEY_0", "http.extraHeader") + .env( + "GIT_CONFIG_VALUE_0", + format!("Authorization: Bearer {token}"), + ) + .stdout(std::process::Stdio::null()) + .status() + .map_err(crate::Error::Io)?; + + if !status.success() { + return Err(crate::Error::Git(format!("push to {} failed", mirror.url))); + } + Ok(()) + } + /// Load per-repo config from `HEAD:.quire/config.fnl`. /// /// Returns a default (empty) `RepoConfig` when: @@ -141,8 +189,8 @@ impl Quire { /// Load and parse the global Fennel config file. /// - /// Caches the result — subsequent calls return the same instance. - /// Returns a typed error if the file is missing or malformed. + /// Re-reads on every call. Cheap at current call volume; revisit if + /// `quire serve` ends up loading per-request. pub fn global_config(&self) -> crate::Result { let config_path = self.config_path(); if !config_path.exists() { @@ -601,4 +649,112 @@ mod tests { let config = q.global_config().expect("global_config should load"); assert!(config.sentry.is_none()); } + + /// Helper: run a git subcommand in `cwd` with hermetic env, panicking on failure. + fn git_in(cwd: &Path, args: &[&str]) { + let output = std::process::Command::new("git") + .args(args) + .current_dir(cwd) + .env("GIT_AUTHOR_NAME", "test") + .env("GIT_AUTHOR_EMAIL", "test@test") + .env("GIT_COMMITTER_NAME", "test") + .env("GIT_COMMITTER_EMAIL", "test@test") + .env("GIT_CONFIG_GLOBAL", "/dev/null") + .env("GIT_CONFIG_SYSTEM", "/dev/null") + .output() + .expect("git command"); + if !output.status.success() { + panic!( + "git {:?} failed:\n{}", + args, + String::from_utf8_lossy(&output.stderr) + ); + } + } + + /// Helper: create a bare repo at `bare` with `main` and one commit. + fn make_bare_with_main(work: &Path, bare: &Path) { + fs_err::create_dir_all(work).expect("mkdir work"); + git_in(work, &["init", "-b", "main"]); + git_in(work, &["commit", "--allow-empty", "-m", "initial"]); + git_in( + work.parent().unwrap_or(work), + &[ + "clone", + "--bare", + work.to_str().unwrap(), + bare.to_str().unwrap(), + ], + ); + } + + fn rev_parse(repo: &Path, rev: &str) -> String { + let output = std::process::Command::new("git") + .args(["-C", repo.to_str().unwrap(), "rev-parse", rev]) + .output() + .expect("rev-parse"); + assert!(output.status.success(), "rev-parse failed"); + String::from_utf8(output.stdout) + .expect("utf-8") + .trim() + .to_string() + } + + #[test] + fn push_to_mirror_pushes_main_to_file_mirror() { + let dir = tempfile::tempdir().expect("tempdir"); + let work = dir.path().join("work"); + let source = dir.path().join("source.git"); + let target = dir.path().join("target.git"); + + make_bare_with_main(&work, &source); + fs_err::create_dir_all(&target).expect("mkdir target"); + git_in(&target, &["init", "--bare", "-b", "main"]); + + let repo = Repo { + path: source.clone(), + }; + let mirror = MirrorConfig { + url: format!("file://{}", target.display()), + }; + repo.push_to_mirror(&mirror, "ignored-for-file-url") + .expect("push should succeed"); + + assert_eq!(rev_parse(&source, "main"), rev_parse(&target, "main")); + } + + #[test] + fn push_to_mirror_errors_when_target_unreachable() { + let dir = tempfile::tempdir().expect("tempdir"); + let work = dir.path().join("work"); + let source = dir.path().join("source.git"); + + make_bare_with_main(&work, &source); + + let repo = Repo { path: source }; + let mirror = MirrorConfig { + url: "file:///nonexistent/quire-test/target.git".to_string(), + }; + let err = repo.push_to_mirror(&mirror, "x").unwrap_err(); + assert!( + matches!(err, crate::Error::Git(_)), + "expected Git error, got {err:?}" + ); + } + + #[test] + fn mirror_url_rejects_embedded_credentials() { + let dir = bare_repo_with_config( + r#"{:mirror {:url "https://x:token@github.com/owner/repo.git"}}"#, + ); + let bare = dir.path().join("repos").join("test.git"); + let repo = Repo { path: bare }; + + let err = repo.config().unwrap_err(); + let msg = err.to_string(); + assert!( + msg.contains("credentials"), + "expected credential error, got: {msg}" + ); + } } -- 2.54.0