fix(#2911): keep the forge token out of argv
`forge_git_url` spliced `core:<token>@` between scheme and authority, and that URL is a process argument. `/proc/<pid>/cmdline` is mode 0444 — world-readable — so the core admin token, which provisions every agent's forge account, was published to any local user for the lifetime of each git child. Seven call sites built such a URL. The credential now travels in the environment instead: `git_command_authed` sets `http.extraHeader` via `GIT_CONFIG_*`, which git reads exactly like a config file, and `/proc/<pid>/environ` is 0400 — owner-only. Same credential, materially smaller audience. The remote is a plain `http://forge/<org>/<repo>.git`, and `forge_git_url` no longer takes a token, so the old shape cannot be rebuilt by accident. `knowledge`'s clone was the one place a credentialed URL was stored as a named remote — git persists the clone URL into `.git/config`, so the token sat on disk and every later `pull` authenticated from there. That is the case `forge::repos::push_config` documents as forbidden ("the tokenised URL ... deliberately never stored as a named remote"). `pull` now rewrites `origin` to the plain URL first, which also scrubs the persisted token from existing deployments, and authenticates from the environment when a token is available. The repo is public, so the pull still works without one. Three call sites also stopped spawning `Command::new("git")` directly, so they honour the `HYPERHIVE_GIT` path the NixOS module bakes in and the `kill_on_drop` every other git spawn gets. The two URL-shape tests now assert the *absence* of a credential, and a new one decodes the header back to `core:<token>` — without that, a malformed header would leave every forge operation silently anonymous with the other assertions still green.
This commit is contained in:
parent
3617578341
commit
44572d1e1a
7 changed files with 134 additions and 56 deletions
|
|
@ -6,7 +6,7 @@
|
|||
use anyhow::Context;
|
||||
use forgejo_api::structs::{MergePullRequestOption, MergePullRequestOptionDo, StateType};
|
||||
|
||||
use super::{CONFIG_ORG, api, core_token, forge_git_url};
|
||||
use super::{CONFIG_ORG, api, core_auth_header, core_token, forge_git_url};
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
// PR-based config-flow merge primitives (part of the
|
||||
|
|
@ -74,9 +74,9 @@ fn repo_agent_name(repo: &str) -> &str {
|
|||
pub async fn pr_head_sha(repo: &str, pr: u64) -> Result<String, ForgeMergeError> {
|
||||
let token = core_token()
|
||||
.ok_or_else(|| ForgeMergeError::Other(anyhow::anyhow!("forge core token absent")))?;
|
||||
let url = forge_git_url(&token, repo);
|
||||
let url = forge_git_url(repo);
|
||||
let refspec = format!("refs/pull/{pr}/head");
|
||||
let out = crate::lifecycle::git_command()
|
||||
let out = crate::lifecycle::git_command_authed(&core_auth_header(&token))
|
||||
.args(["ls-remote", &url, &refspec])
|
||||
.output()
|
||||
.await
|
||||
|
|
@ -142,10 +142,10 @@ pub fn config_repo(agent: &str) -> String {
|
|||
pub async fn fetch_pr_head_into_applied(repo: &str, pr: u64) -> Result<(), ForgeMergeError> {
|
||||
let token = core_token()
|
||||
.ok_or_else(|| ForgeMergeError::Other(anyhow::anyhow!("forge core token absent")))?;
|
||||
let url = forge_git_url(&token, repo);
|
||||
let url = forge_git_url(repo);
|
||||
let applied = crate::paths::applied_dir(repo_agent_name(repo));
|
||||
let refspec = format!("refs/pull/{pr}/head");
|
||||
let out = crate::lifecycle::git_command()
|
||||
let out = crate::lifecycle::git_command_authed(&core_auth_header(&token))
|
||||
.current_dir(&applied)
|
||||
.args(["fetch", "--no-tags", &url, &refspec])
|
||||
.output()
|
||||
|
|
@ -262,25 +262,44 @@ mod tests {
|
|||
assert_eq!(repo_agent_name("a/b/c"), "c");
|
||||
}
|
||||
|
||||
/// The remote carries **no credential**. `argv` is world-readable through
|
||||
/// `/proc/<pid>/cmdline`, so a token spliced in here would be published to
|
||||
/// every local user for the life of the git child.
|
||||
///
|
||||
/// Deliberately not via `forge_git_url`, which reads `HIVE_FORGE_URL` —
|
||||
/// setting that here would race every other test in this binary.
|
||||
#[test]
|
||||
fn forge_git_url_shape() {
|
||||
// Tests the pure half: credentials go between scheme and
|
||||
// authority. Deliberately not via `forge_git_url`, which reads
|
||||
// HIVE_FORGE_URL — setting that here would race every other
|
||||
// test in this binary, and there is no fallback to lean on any
|
||||
// more (a guessed base is the bug this issue removes).
|
||||
let url = git_url_with_base("http://forge.example.test", "tok", "a/iris");
|
||||
assert_eq!(url, "http://core:tok@forge.example.test/a/iris.git");
|
||||
fn forge_git_url_carries_no_credential() {
|
||||
let url = git_url_with_base("http://forge.example.test", "a/iris");
|
||||
assert_eq!(url, "http://forge.example.test/a/iris.git");
|
||||
assert!(!url.contains('@'), "no userinfo: {url}");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn forge_git_url_preserves_https() {
|
||||
// The scheme is carried through rather than assumed: a swarm
|
||||
// whose forge is behind TLS must not be downgraded to http.
|
||||
let url = git_url_with_base("https://forge.example.test", "tok", "a/iris");
|
||||
let url = git_url_with_base("https://forge.example.test", "a/iris");
|
||||
assert!(url.starts_with("https://"), "https must survive: {url}");
|
||||
}
|
||||
|
||||
/// The credential goes in an `Authorization` header instead — decodable
|
||||
/// back to `core:<token>`, so the swap is genuinely equivalent auth and
|
||||
/// not a silent downgrade to anonymous.
|
||||
#[test]
|
||||
fn core_auth_header_is_basic_core_token() {
|
||||
use base64::Engine as _;
|
||||
let header = crate::forge::core_auth_header("s3cret");
|
||||
let b64 = header
|
||||
.strip_prefix("Authorization: Basic ")
|
||||
.expect("basic auth header");
|
||||
let decoded = base64::engine::general_purpose::STANDARD
|
||||
.decode(b64)
|
||||
.expect("valid base64");
|
||||
assert_eq!(String::from_utf8_lossy(&decoded), "core:s3cret");
|
||||
assert!(
|
||||
url.starts_with("https://core:tok@"),
|
||||
"https must survive: {url}"
|
||||
!header.contains("s3cret"),
|
||||
"token not in the clear: {header}"
|
||||
);
|
||||
}
|
||||
}
|
||||
|
|
|
|||
Loading…
Reference in a new issue