From fa9b8ba73ac3a32a7a89233b0e6854cad7ce5430 Mon Sep 17 00:00:00 2001
From: atlas <atlas@hyperhive.local>
Date: Wed, 2 Sep 2026 08:34:37 +0200
Subject: [PATCH] hive-priv: three doc comments that disagree with their own
 validators

Follow-up to the same defect class as the `validate_container_name`
fix: prose on a privsep validator that does not match what the body
does. Found by enumerating all 33 fns in this file whose doc claims a
check or guarantee and reading body against doc. Three disagreed.

`validate_forge_admin_arg` overclaimed, which is the dangerous
direction. Its doc ended "Shell metacharacters are harmless since the
command is spawned directly (no shell), but we reject them
defensively" -- and no metacharacter is rejected anywhere: the body
takes null bytes, newlines and carriage returns only. The first clause
is true and is the actual safety argument, so it stays; the claim of a
defence that does not exist goes. The same doc also said "null bytes
and newlines" while the body rejects `\r` too.

`validate_bind_path` listed "no newlines, null bytes, or
double-quotes" and omitted the colon its body rejects -- the one that
matters most. The other three would corrupt the
`EXTRA_NSPAWN_FLAGS="..."` conf line; a colon corrupts nothing,
because `--bind=SRC:DST` is colon-separated, so a path carrying one
silently produces a different mount than the caller asked for. The
function's own bail! string already named colons.

Checked and left alone because they are accurate: `check_fd_agreement`
(both arms really do bail, so "in either direction" is earned),
`exec_forge_admin`, `validate_credential_name`.

Comments only, no behaviour change. Gated: fmt, clippy -D warnings, 21
tests, rustdoc under CI's docs-rustdoc lints, push-lints.
---
 hive-priv/src/main.rs | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

diff --git a/hive-priv/src/main.rs b/hive-priv/src/main.rs
index aae7b5b0..cdeac79b 100644
--- a/hive-priv/src/main.rs
+++ b/hive-priv/src/main.rs
@@ -2183,10 +2183,12 @@ async fn set_subvolume_quota(
     Ok((String::new(), String::new()))
 }
 
-/// Validate a single argument destined for `forgejo admin`. Rejects
-/// null bytes and newlines (which could corrupt the subprocess args list
-/// or log output). Shell metacharacters are harmless since the command
-/// is spawned directly (no shell), but we reject them defensively.
+/// Validate a single argument destined for `forgejo admin`. Rejects null
+/// bytes, newlines and carriage returns (which could corrupt the
+/// subprocess args list or log output).
+///
+/// Shell metacharacters are **not** rejected, and do not need to be: the
+/// command is spawned directly, with no shell to interpret them.
 fn validate_forge_admin_arg(arg: &str) -> Result<()> {
     if arg.bytes().any(|b| b == 0 || b == b'\n' || b == b'\r') {
         bail!("forge admin arg {arg:?} contains null byte or newline");
@@ -2765,8 +2767,13 @@ fn validate_name_chars(name: &str) -> Result<()> {
 }
 
 /// Validate a bind-mount path: must be absolute, non-empty, and contain
-/// no newlines, null bytes, or double-quotes (which would break the
-/// `EXTRA_NSPAWN_FLAGS="..."` conf line format).
+/// no newlines, null bytes, double-quotes, or colons.
+///
+/// The first three would break the `EXTRA_NSPAWN_FLAGS="..."` conf line
+/// format. The colon is the one that matters most and had been left out
+/// of this list: `--bind=SRC:DST` is colon-separated, so a path carrying
+/// one does not corrupt the line — it silently becomes a *different
+/// mount* than the caller asked for.
 fn validate_bind_path(path: &str) -> Result<()> {
     if path.is_empty()
         || !path.starts_with('/')