git ents: feat: edit all config fields in the browser, harden web edits, and cover them end to end

crates/git-ents-server/src/web/mod.rs @@ -36,6 +36,8 @@ /// The member username this key maps to in the current repo, when it is a /// member there — the gate for showing edit controls. username: Option<String>, + /// The session's CSRF token, echoed in edit forms. + csrf: String, } use self::assets::{COPY_SCRIPT, FONTS, STYLE}; @@ -105,17 +107,27 @@ let cookie = headers .get(axum::http::header::COOKIE) .and_then(|value| value.to_str().ok()); + let secure = is_secure_request(headers); let segments: Vec<&str> = path.split('/').filter(|s| !s.is_empty()).collect(); if segments == ["login"] { return match write::login(&state.sessions, &body) { - Ok(token) => redirect("/login", Some(session_cookie(&token))), + Ok(token) => redirect("/login", Some(session_cookie(&token, secure))), Err(error) => login_page(None, Some(&error)).into_response(), }; } if segments == ["logout"] { + // A cross-site form cannot read the session's CSRF token, so an absent + // or wrong one means the request did not originate from our own page. + if !write::csrf_ok( + &state.sessions, + cookie, + &write::field(&body, "csrf").unwrap_or_default(), + ) { + return redirect("/login", None); + } write::logout(&state.sessions, cookie); - return redirect("/login", Some(cleared_cookie())); + return redirect("/login", Some(cleared_cookie(secure))); } let Some((repo, rel, rest)) = resolve_repo(&state.data_dir, &segments) else { @@ -127,6 +139,16 @@ save_settings(state, &repo, &rel, cookie, body).await } +/// Whether the request reached us over HTTPS — directly, or through a TLS +/// terminator that set `X-Forwarded-Proto`. Gates the cookie `Secure` flag so a +/// plain-HTTP development server still works. +fn is_secure_request(headers: &HeaderMap) -> bool { + headers + .get("X-Forwarded-Proto") + .and_then(|value| value.to_str().ok()) + .is_some_and(|proto| proto.eq_ignore_ascii_case("https")) +} + /// Apply a settings edit, then redirect back to the settings page on success or /// render the reason it was rejected. async fn save_settings( @@ -143,20 +165,31 @@ ) .into_response(); }; - let description = write::field(&body, "description").unwrap_or_default(); + if !write::csrf_ok( + &state.sessions, + cookie, + &write::field(&body, "csrf").unwrap_or_default(), + ) { + return edit_error(rel, "the edit could not be verified; reload and try again") + .into_response(); + } + let edit = write::ConfigEdit { + description: write::field(&body, "description").unwrap_or_default(), + homepage: write::field(&body, "homepage").unwrap_or_default(), + topics: write::field(&body, "topics") + .unwrap_or_default() + .split(',') + .map(str::trim) + .filter(|topic| !topic.is_empty()) + .map(str::to_owned) + .collect(), + }; let sessions = state.sessions.clone(); let cookie = cookie.map(str::to_owned); let repo = repo.to_owned(); let result = tokio::task::spawn_blocking(move || { - write::edit_description( - &sessions, - cookie.as_deref(), - &repo, - &description, - &seed, - &hooks, - ) + write::edit_config(&sessions, cookie.as_deref(), &repo, &edit, &seed, &hooks) }) .await; @@ -180,16 +213,21 @@ .unwrap_or_else(|_| StatusCode::INTERNAL_SERVER_ERROR.into_response()) } -/// The `Set-Cookie` value that opens a session. -fn session_cookie(token: &str) -> String { - format!("{}={token}; Path=/; HttpOnly; SameSite=Lax", write::COOKIE) +/// The `Set-Cookie` value that opens a session, marked `Secure` over HTTPS. +fn session_cookie(token: &str, secure: bool) -> String { + format!( + "{}={token}; Path=/; HttpOnly; SameSite=Lax{}", + write::COOKIE, + if secure { "; Secure" } else { "" } + ) } /// The `Set-Cookie` value that clears a session. -fn cleared_cookie() -> String { +fn cleared_cookie(secure: bool) -> String { format!( - "{}=; Path=/; Max-Age=0; HttpOnly; SameSite=Lax", - write::COOKIE + "{}=; Path=/; Max-Age=0; HttpOnly; SameSite=Lax{}", + write::COOKIE, + if secure { "; Secure" } else { "" } ) } @@ -236,6 +274,7 @@ Some(Auth { label: session.label, username, + csrf: session.csrf, }) } @@ -452,6 +491,7 @@ Some(s) => { span.muted { "Signed in · " (s.label) } form method="post" action="/logout" { + input type="hidden" name="csrf" value=(s.csrf); button.btn.btn-quiet type="submit" { "Sign out" } } }

crates/git-ents-server/src/web/pages.rs @@ -841,21 +841,8 @@ div.card { div.card-header { "General" } (setting_row("Repository name", meta.name())) - (description_setting(meta, auth)) - (setting_row("Homepage", meta.homepage.as_deref().unwrap_or("—"))) (setting_row("Default branch", meta.branch.as_deref().unwrap_or("—"))) - div.card-row { - span.setting-label { "Topics" } - @if meta.topics.is_empty() { - span.muted { "—" } - } @else { - div.topics { - @for topic in &meta.topics { - span.topic { (topic) } - } - } - } - } + (general_settings(meta, auth)) } div.card { @@ -944,20 +931,40 @@ } } -/// The Description row: an inline edit form when the signed-in key is a member, -/// otherwise the read-only value. -fn description_setting(meta: &RepoMeta, auth: Option<&super::Auth>) -> Markup { - let value = meta.description.as_deref().unwrap_or_default(); - if auth.and_then(|a| a.username.as_deref()).is_none() { - return setting_row("Description", meta.description.as_deref().unwrap_or("—")); - } +/// The editable General fields (description, homepage, topics): an edit form when +/// the signed-in key is a member of this repo, otherwise read-only rows. +fn general_settings(meta: &RepoMeta, auth: Option<&super::Auth>) -> Markup { + let Some(auth) = auth.filter(|a| a.username.is_some()) else { + return html! { + (setting_row("Description", meta.description.as_deref().unwrap_or("—"))) + (setting_row("Homepage", meta.homepage.as_deref().unwrap_or("—"))) + div.card-row { + span.setting-label { "Topics" } + @if meta.topics.is_empty() { + span.muted { "—" } + } @else { + div.topics { @for topic in &meta.topics { span.topic { (topic) } } } + } + } + }; + }; + let description = meta.description.as_deref().unwrap_or_default(); + let homepage = meta.homepage.as_deref().unwrap_or_default(); + let topics = meta.topics.join(", "); html! { div.card-row { - span.setting-label { "Description" } - form.inline-edit method="post" action={ "/" (meta.rel) "/settings" } { - input type="text" name="description" value=(value) + form.edit-form method="post" action={ "/" (meta.rel) "/settings" } { + input type="hidden" name="csrf" value=(auth.csrf); + label { "Description" } + input type="text" name="description" value=(description) placeholder="A short description"; - button.btn type="submit" { "Save" } + label { "Homepage" } + input type="text" name="homepage" value=(homepage) + placeholder="https://example.com"; + label { "Topics" } + input type="text" name="topics" value=(topics) + placeholder="comma, separated, topics"; + button.btn type="submit" { "Save changes" } } } }

crates/git-ents-server/src/web/write.rs @@ -31,6 +31,9 @@ public_key: String, /// A human label for the key — its given name, or its type. label: String, + /// A per-session token that state-changing form posts must echo back, so a + /// cross-site request (which cannot read it) cannot act as the user. + csrf: String, } /// A cheap, cloneable view of a session for rendering and authorization, without @@ -39,6 +42,14 @@ pub(super) struct SessionSnapshot { pub(super) label: String, pub(super) public_key: String, + pub(super) csrf: String, +} + +/// The fields a settings edit may change on `refs/meta/config`. +pub(super) struct ConfigEdit { + pub(super) description: String, + pub(super) homepage: String, + pub(super) topics: Vec<String>, } /// Create an empty session table. @@ -54,6 +65,7 @@ Some(SessionSnapshot { label: session.label.clone(), public_key: session.public_key.clone(), + csrf: session.csrf.clone(), }) } @@ -81,6 +93,7 @@ .unwrap_or_else(|| key_type(&public_key)); let token = random_token()?; + let csrf = random_token()?; let mut table = sessions .lock() .map_err(|_poisoned| "session store unavailable".to_owned())?; @@ -90,11 +103,24 @@ private_key, public_key, label, + csrf, }, ); Ok(token) } +/// Whether `cookie`'s session exists and its CSRF token matches `token`. +pub(super) fn csrf_ok(sessions: &Sessions, cookie: Option<&str>, token: &str) -> bool { + let Some(session_token) = cookie.and_then(self::token) else { + return false; + }; + sessions + .lock() + .ok() + .and_then(|table| table.get(&session_token).map(|s| s.csrf == token)) + .unwrap_or(false) +} + /// Drop the session a `Cookie` header points at, if any. pub(super) fn logout(sessions: &Sessions, cookie: Option<&str>) { let Some(token) = cookie.and_then(token) else { @@ -105,18 +131,18 @@ } } -/// Land a new repository description by staging it on a throwaway ref and pushing -/// it, signed with the session's web key, onto `refs/meta/config` — through the +/// Land a configuration change by staging it on a throwaway ref and pushing it, +/// signed with the session's web key, onto `refs/meta/config` — through the /// `pre-receive` gate. Returns `Ok` only when the gate accepts the push. /// /// `seed` and `hooks` are the server's signed-push nonce seed and hooks /// directory; both are required, so a web edit is never a way around a server /// that is not enforcing the gate. -pub(super) fn edit_description( +pub(super) fn edit_config( sessions: &Sessions, cookie: Option<&str>, repo: &Path, - new_description: &str, + edit: &ConfigEdit, seed: &str, hooks: &Path, ) -> Result<(), String> { @@ -138,7 +164,9 @@ let mut config = git_ents::config::load(repo).map_err(|e| format!("could not read config: {e}"))?; - config.description = new_description.to_owned(); + config.description = edit.description.clone(); + config.homepage = edit.homepage.clone(); + config.topics = edit.topics.clone(); let staging = format!("refs/web-staging/{}", random_token()?); let result = stage_and_push(

crates/git-ents-server/tests/web_edit.rs @@ -1,0 +1,440 @@ +#![allow( + missing_docs, + clippy::unwrap_used, + clippy::panic, + clippy::arithmetic_side_effects, + reason = "integration test binary" +)] + +//! End-to-end coverage for authenticated browser edits: signing in with a web +//! key, then saving a settings change that must travel through the real +//! `pre-receive` gate as a signed push before it lands on `refs/meta/config`. + +use std::io::{Read, Write}; +use std::net::{TcpListener, TcpStream}; +use std::path::{Path, PathBuf}; +use std::process::{Child, Command, Stdio}; + +const BIN: &str = env!("CARGO_BIN_EXE_git-ents-server"); + +#[test] +fn a_member_edits_settings_through_the_browser() { + let env = Server::start(); + let bare = env.create_repo("repo.git"); + let key = keygen(env.scratch(), "web"); + env.add_member(&bare, "alice", &pubkey(&key)); + + // Sign in with the web key; the server derives its public half and opens a + // session whose cookie we carry from here on. + let signin = env.post("/login", "", &form(&[("private_key", &read(&key))])); + assert_eq!(signin.status, 303, "sign-in should redirect"); + let cookie = signin.session_cookie().unwrap(); + + // The settings page now offers an edit form; read its CSRF token. + let page = env.get("/repo.git/settings", &cookie); + assert!( + page.body.contains("name=\"csrf\""), + "edit form should render" + ); + let csrf = page.csrf().unwrap(); + + // Save a change to every General field. + let edit = env.post( + "/repo.git/settings", + &cookie, + &form(&[ + ("csrf", &csrf), + ("description", "Edited from the browser"), + ("homepage", "https://ents.example"), + ("topics", "rust, git, forge"), + ]), + ); + assert_eq!( + edit.status, 303, + "a valid edit should redirect: {}", + edit.body + ); + + // The change is reflected because it landed on `refs/meta/config`: the page + // re-reads the ref, it is not echoing the submitted form. + let after = env.get("/repo.git/settings", &cookie); + assert!( + after.body.contains("Edited from the browser"), + "description did not land" + ); + assert!( + after.body.contains("https://ents.example"), + "homepage did not land" + ); + assert!(after.body.contains("forge"), "topics did not land"); + + // And the gate really moved the ref: a fresh signed commit now sits on it. + assert!( + git( + &bare, + &["rev-parse", "--verify", "--quiet", "refs/meta/config"] + ) + .is_some(), + "refs/meta/config should exist after the edit" + ); +} + +#[test] +fn an_edit_without_a_valid_csrf_token_is_refused() { + let env = Server::start(); + let bare = env.create_repo("repo.git"); + let key = keygen(env.scratch(), "web"); + env.add_member(&bare, "alice", &pubkey(&key)); + + let cookie = env + .post("/login", "", &form(&[("private_key", &read(&key))])) + .session_cookie() + .unwrap(); + + let edit = env.post( + "/repo.git/settings", + &cookie, + &form(&[("csrf", "not-the-token"), ("description", "sneaky")]), + ); + assert_eq!(edit.status, 200, "a bad-CSRF edit should not redirect"); + assert!( + env.get("/repo.git/settings", &cookie) + .body + .contains("name=\"csrf\"") + && !page_description_is(&env, &cookie, "sneaky"), + "the description must be unchanged" + ); +} + +#[test] +fn a_non_member_is_not_offered_an_edit_form() { + let env = Server::start(); + let bare = env.create_repo("repo.git"); + let member = keygen(env.scratch(), "member"); + env.add_member(&bare, "alice", &pubkey(&member)); + + // Sign in with a key that is *not* a member of this repo. + let intruder = keygen(env.scratch(), "intruder"); + let cookie = env + .post("/login", "", &form(&[("private_key", &read(&intruder))])) + .session_cookie() + .unwrap(); + + let page = env.get("/repo.git/settings", &cookie); + assert!( + page.body.contains("not a member"), + "a non-member should be told they cannot edit" + ); + assert!( + !page.body.contains("Save changes"), + "a non-member should not see the edit form" + ); +} + +/// Whether the rendered settings page shows `value` as the description. +fn page_description_is(env: &Server, cookie: &str, value: &str) -> bool { + env.get("/repo.git/settings", cookie).body.contains(value) +} + +/// A running server with its data and hooks directories. +struct Server { + child: Child, + port: u16, + data: tempfile::TempDir, + scratch: tempfile::TempDir, + _hooks: tempfile::TempDir, +} + +impl Server { + /// Start a server enforcing the signed-push gate: a nonce seed plus a hooks + /// directory whose `pre-receive` is the compiled verifier. + fn start() -> Self { + let data = tempfile::tempdir().unwrap(); + let scratch = tempfile::tempdir().unwrap(); + let hooks = tempfile::tempdir().unwrap(); + let hook = hooks.path().join("pre-receive"); + std::fs::write(&hook, format!("#!/bin/sh\nexec \"{BIN}\" pre-receive\n")).unwrap(); + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + std::fs::set_permissions(&hook, std::fs::Permissions::from_mode(0o755)).unwrap(); + } + + let port = free_port(); + let child = Command::new(BIN) + .arg("--port") + .arg(port.to_string()) + .arg("--data-dir") + .arg(data.path()) + .arg("--cert-nonce-seed") + .arg("test-seed") + .arg("--hooks-dir") + .arg(hooks.path()) + .spawn() + .unwrap(); + wait_for_port(port); + Self { + child, + port, + data, + scratch, + _hooks: hooks, + } + } + + fn scratch(&self) -> &Path { + self.scratch.path() + } + + /// Create a bare repo by pushing an initial commit to it (auto-init), and + /// return its on-disk path. + fn create_repo(&self, name: &str) -> PathBuf { + let work = self.scratch.path().join(format!("work-{name}")); + std::fs::create_dir_all(&work).unwrap(); + run(&work, "git", &["init", "-q", "-b", "main"]); + std::fs::write(work.join("README.md"), "hello\n").unwrap(); + run(&work, "git", &["add", "."]); + run( + &work, + "git", + &["-c", "commit.gpgsign=false", "commit", "-q", "-m", "init"], + ); + let url = format!("http://127.0.0.1:{}/{name}", self.port); + run(&work, "git", &["push", "-q", &url, "main"]); + self.data.path().join(name) + } + + /// Write a member ref `refs/meta/member/<username>` into `bare` directly, in + /// the on-disk layout the loader reads: a `principal` blob, empty + /// `valid_after`/`valid_before` subtrees, and a `trust/Keys/key` blob. + fn add_member(&self, bare: &Path, username: &str, public_key: &str) { + let principal = hash_object(bare, username.as_bytes()); + let key_blob = hash_object(bare, public_key.as_bytes()); + let keys = mktree(bare, &format!("100644 blob {key_blob}\tkey\n")); + let trust = mktree(bare, &format!("040000 tree {keys}\tKeys\n")); + let empty = mktree(bare, ""); + let root = mktree( + bare, + &format!( + "100644 blob {principal}\tprincipal\n\ + 040000 tree {empty}\tvalid_after\n\ + 040000 tree {empty}\tvalid_before\n\ + 040000 tree {trust}\ttrust\n" + ), + ); + let commit = git(bare, &["commit-tree", &root, "-m", "member"]).unwrap(); + git( + bare, + &[ + "update-ref", + &format!("refs/meta/member/{username}"), + &commit, + ], + ) + .unwrap(); + } + + fn get(&self, path: &str, cookie: &str) -> Http { + self.request("GET", path, &[("Cookie", cookie)], "") + } + + fn post(&self, path: &str, cookie: &str, body: &str) -> Http { + let mut headers = vec![("Content-Type", "application/x-www-form-urlencoded")]; + if !cookie.is_empty() { + headers.push(("Cookie", cookie)); + } + self.request("POST", path, &headers, body) + } + + /// Send one HTTP/1.0 request and parse the response. + fn request(&self, method: &str, path: &str, headers: &[(&str, &str)], body: &str) -> Http { + let mut request = format!("{method} {path} HTTP/1.0\r\nHost: 127.0.0.1\r\n"); + for (name, value) in headers { + request.push_str(&format!("{name}: {value}\r\n")); + } + request.push_str(&format!("Content-Length: {}\r\n\r\n", body.len())); + request.push_str(body); + + let mut stream = TcpStream::connect(format!("127.0.0.1:{}", self.port)).unwrap(); + stream.write_all(request.as_bytes()).unwrap(); + let mut raw = Vec::new(); + stream.read_to_end(&mut raw).unwrap(); + Http::parse(&String::from_utf8_lossy(&raw)) + } +} + +impl Drop for Server { + fn drop(&mut self) { + self.child.kill().unwrap(); + let _wait = self.child.wait(); + } +} + +/// A parsed HTTP response. +struct Http { + status: u16, + headers: Vec<(String, String)>, + body: String, +} + +impl Http { + fn parse(raw: &str) -> Self { + let (head, body) = raw.split_once("\r\n\r\n").unwrap_or((raw, "")); + let mut lines = head.lines(); + let status = lines + .next() + .and_then(|line| line.split_whitespace().nth(1)) + .and_then(|code| code.parse().ok()) + .unwrap_or(0); + let headers = lines + .filter_map(|line| line.split_once(':')) + .map(|(name, value)| (name.trim().to_lowercase(), value.trim().to_owned())) + .collect(); + Self { + status, + headers, + body: body.to_owned(), + } + } + + /// The `ents_session=<token>` pair from a `Set-Cookie` header, ready to send + /// back as a `Cookie` value. + fn session_cookie(&self) -> Option<String> { + self.headers + .iter() + .filter(|(name, _)| name == "set-cookie") + .find_map(|(_, value)| value.split(';').next()) + .filter(|pair| pair.starts_with("ents_session=")) + .map(str::to_owned) + } + + /// The CSRF token rendered in the page's hidden field. + fn csrf(&self) -> Option<String> { + let marker = "name=\"csrf\" value=\""; + let start = self.body.find(marker)? + marker.len(); + let rest = self.body.get(start..)?; + let end = rest.find('"')?; + rest.get(..end).map(str::to_owned) + } +} + +fn free_port() -> u16 { + let probe = TcpListener::bind("127.0.0.1:0").unwrap(); + let port = probe.local_addr().unwrap().port(); + drop(probe); + port +} + +fn wait_for_port(port: u16) { + let deadline = std::time::Instant::now() + std::time::Duration::from_secs(5); + loop { + match TcpStream::connect(format!("127.0.0.1:{port}")) { + Ok(_) => return, + Err(_) if std::time::Instant::now() < deadline => { + std::thread::sleep(std::time::Duration::from_millis(10)); + } + Err(e) => panic!("server never accepted connections: {e}"), + } + } +} + +/// Generate an ed25519 keypair at `base/<name>`, returning the private key path. +fn keygen(base: &Path, name: &str) -> PathBuf { + let key = base.join(name); + let status = Command::new("ssh-keygen") + .args(["-q", "-t", "ed25519", "-N", "", "-C", name, "-f"]) + .arg(&key) + .status() + .unwrap(); + assert!(status.success(), "ssh-keygen failed"); + key +} + +fn pubkey(private: &Path) -> String { + read(&private.with_extension("pub")).trim().to_owned() +} + +fn read(path: &Path) -> String { + std::fs::read_to_string(path).unwrap() +} + +/// Encode form fields as `application/x-www-form-urlencoded`. +fn form(fields: &[(&str, &str)]) -> String { + fields + .iter() + .map(|(key, value)| format!("{key}={}", encode(value))) + .collect::<Vec<_>>() + .join("&") +} + +/// Percent-encode one form value. +fn encode(value: &str) -> String { + value + .bytes() + .map(|byte| { + if byte.is_ascii_alphanumeric() || matches!(byte, b'-' | b'_' | b'.' | b'~') { + (byte as char).to_string() + } else { + format!("%{byte:02X}") + } + }) + .collect() +} + +fn run(dir: &Path, program: &str, args: &[&str]) { + let output = Command::new(program) + .current_dir(dir) + .args(args) + .env("GIT_CONFIG_GLOBAL", "/dev/null") + .env("GIT_CONFIG_SYSTEM", "/dev/null") + .env("GIT_AUTHOR_NAME", "T") + .env("GIT_AUTHOR_EMAIL", "t@e") + .env("GIT_COMMITTER_NAME", "T") + .env("GIT_COMMITTER_EMAIL", "t@e") + .output() + .unwrap(); + assert!( + output.status.success(), + "{program} {args:?} failed: {}", + String::from_utf8_lossy(&output.stderr) + ); +} + +/// Run `git -C bare <args>`, returning trimmed stdout on success. +fn git(bare: &Path, args: &[&str]) -> Option<String> { + let output = Command::new("git") + .arg("-C") + .arg(bare) + .args(args) + .stdin(Stdio::null()) + .output() + .unwrap(); + output + .status + .success() + .then(|| String::from_utf8_lossy(&output.stdout).trim().to_owned()) + .filter(|out| !out.is_empty() || args.first() == Some(&"update-ref")) +} + +fn hash_object(bare: &Path, bytes: &[u8]) -> String { + pipe(bare, &["hash-object", "-w", "--stdin"], bytes) +} + +fn mktree(bare: &Path, spec: &str) -> String { + pipe(bare, &["mktree"], spec.as_bytes()) +} + +fn pipe(bare: &Path, args: &[&str], input: &[u8]) -> String { + let mut child = Command::new("git") + .arg("-C") + .arg(bare) + .args(args) + .stdin(Stdio::piped()) + .stdout(Stdio::piped()) + .spawn() + .unwrap(); + child.stdin.take().unwrap().write_all(input).unwrap(); + let output = child.wait_with_output().unwrap(); + assert!(output.status.success(), "git {args:?} failed"); + String::from_utf8_lossy(&output.stdout).trim().to_owned() +}

feat: edit all config fields in the browser, harden web edits, and cover them end to end

Reviews

Start a review